imx-drm: Fix probe failure

From: Fabio Estevam <[email protected]>

Since commit b5dc0d10 (drm/imx: kill firstopen callback) the following probe failure is seen:

[drm] Supports vblank timestamp caching Rev 1 (10.10.2010). [drm] No driver support for vblank timestamp query. [drm] Initialized imx-drm 1.0.0 20120507 on minor 0 imx-ldb ldb.10: adding encoder failed with -16
imx-ldb: probe of ldb.10 failed with error -16 imx-ipuv3 2400000.ipu: IPUv3H probed
imx-ipuv3 2800000.ipu: IPUv3H probed
imx-ipuv3-crtc imx-ipuv3-crtc.0: adding crtc failed with -16.
imx-ipuv3-crtc: probe of imx-ipuv3-crtc.0 failed with error -16 imx-ipuv3-crtc imx-ipuv3-crtc.1: adding crtc failed with -16.
imx-ipuv3-crtc: probe of imx-ipuv3-crtc.1 failed with error -16 imx-ipuv3-crtc imx-ipuv3-crtc.2: adding crtc failed with -16.
imx-ipuv3-crtc: probe of imx-ipuv3-crtc.2 failed with error -16 imx-ipuv3-crtc imx-ipuv3-crtc.3: adding crtc failed with -16.
imx-ipuv3-crtc: probe of imx-ipuv3-crtc.3 failed with error -16

'imxdrm->references' is not keeping the references correctly, causing the probe to fail, so let's get rid of it.

After this patch, lvds panel is functional on a mx6qsabrelite board.

---
 drivers/staging/imx-drm/imx-drm-core.c | 25 +------------------------
 1 file changed, 1 insertion(+), 24 deletions(-)

index 47c5888..98eac33 100644
@@ -41,7 +41,6 @@ struct imx_drm_device {
 	struct list_head			encoder_list;
 	struct list_head			connector_list;
 	struct mutex				mutex;
-	int					references;
 	int					pipes;
 	struct drm_fbdev_cma			*fbhelper;
 };

@@ -241,8 +240,6 @@ struct drm_device *imx_drm_device_get(void)
 		}
 	}
 
-	imxdrm->references++;
-
 	return imxdrm->drm;
 
 unwind_crtc:

@@ -280,8 +277,6 @@ void imx_drm_device_put(void)
 	list_for_each_entry(enc, &imxdrm->encoder_list, list)
 		module_put(enc->owner);
 
-	imxdrm->references--;
-
 	mutex_unlock(&imxdrm->mutex);
 }
 EXPORT_SYMBOL_GPL(imx_drm_device_put);

@@ -485,11 +480,6 @@ int imx_drm_add_crtc(struct drm_crtc *crtc,
 
 	mutex_lock(&imxdrm->mutex);
 
-	if (imxdrm->references) {
-		ret = -EBUSY;
-		goto err_busy;
-	}
-
 	imx_drm_crtc = kzalloc(sizeof(*imx_drm_crtc), GFP_KERNEL);
 	if (!imx_drm_crtc) {
 		ret = -ENOMEM;

@@ -523,7 +513,6 @@ int imx_drm_add_crtc(struct drm_crtc *crtc,
 err_register:
 	kfree(imx_drm_crtc);
 err_alloc:
-err_busy:
 	mutex_unlock(&imxdrm->mutex);
 	return ret;
 }

@@ -564,11 +553,6 @@ int imx_drm_add_encoder(struct drm_encoder *encoder,
 
 	mutex_lock(&imxdrm->mutex);
 
-	if (imxdrm->references) {
-		ret = -EBUSY;
-		goto err_busy;
-	}
-
 	imx_drm_encoder = kzalloc(sizeof(*imx_drm_encoder), GFP_KERNEL);
 	if (!imx_drm_encoder) {
 		ret = -ENOMEM;

@@ -583,7 +567,7 @@ int imx_drm_add_encoder(struct drm_encoder *encoder,
 		ret = -ENOMEM;
 		goto err_register;
 	}
-
+
 	list_add_tail(&imx_drm_encoder->list, &imxdrm->encoder_list);
 
 	*newenc = imx_drm_encoder;

@@ -595,7 +579,6 @@ int imx_drm_add_encoder(struct drm_encoder *encoder,
 err_register:
 	kfree(imx_drm_encoder);
 err_alloc:
-err_busy:
 	mutex_unlock(&imxdrm->mutex);
 
 	return ret;

@@ -709,11 +692,6 @@ int imx_drm_add_connector(struct drm_connector *connector,
 
 	mutex_lock(&imxdrm->mutex);
 
-	if (imxdrm->references) {
-		ret = -EBUSY;
-		goto err_busy;
-	}
-
 	imx_drm_connector = kzalloc(sizeof(*imx_drm_connector), GFP_KERNEL);
 	if (!imx_drm_connector) {
 		ret = -ENOMEM;

@@ -738,7 +716,6 @@ int imx_drm_add_connector(struct drm_connector *connector,
 err_register:
 	kfree(imx_drm_connector);
 err_alloc:
-err_busy:
 	mutex_unlock(&imxdrm->mutex);
 
 	return ret;

1.8.1.2

I don't have that commit in any tree I control, so I can't apply this patch. What tree is this commit in?

thanks,

greg k-h

Hi Greg,

Ok, understood.

Just realized that this commit reached linux-next via Dave Airlie's tree:

commit b5dc0d108cd3c0b50ddcb6f6c54be1
bea4c39e01

Author: Daniel Vetter <[email protected]>
Date:   Thu Aug 8 15:41:13 2013 +0200

drm/imx: kill firstopen callback

This thing seems to do some kind of delayed setup. Really, real kms drivers shouldn't do that at all. Either stuff needs to be dynamically hotplugged or the driver setup sequence needs to be fixed.

This patch here just moves the setup at the very end of the driver load callback, with the locking adjusted accordingly.

v2: Also move the corresponding put from ->lastclose to ->unload.

Will resend it to Dave then.

Thanks,

Fabio Estevam

Any suggestions about a proper way of handling this?

After b5dc0d10, imx_drm_device_get() is called from imx_drm_driver_load(), which increments the 'references' very early now.

Then when we check:

    if (imxdrm->references) {
        ret = -EBUSY;
        goto err_busy;
    }

,it will fail always.

Not sure where is a proper location to increment/decrement 'imxdrm->references' though.

Tried to look at other drm drivers, but could not find examples on how to deal with such references.

Regards,

Fabio Estevam

Do the open/close callbacks work?

Sascha

Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

The 'open' callback maps to the generic 'drm_open', so we cannot use it.

Shouldn't we rely on the refcount done by the drm core instead of doing this manually in imx-drm?

We could replace this, but...

...This could be a better solution.

you mean (struct drm_device)->open_count, right?

Sascha

Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Yes, and open_count is already handled by drm_open/drm_release callbacks, which is something we currently use inside imx-drm-core.