[PATCH] imx-drm: Fix probe failure

[PATCH] imx-drm: Fix probe failure

[Fabio Estevam]

From: Fabio Estevam <fabio.estevam@freescale.com>

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.

Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
 drivers/staging/imx-drm/imx-drm-core.c | 25 +------------------------
 1 file changed, 1 insertion(+), 24 deletions(-)

diff --git a/drivers/staging/imx-drm/imx-drm-core.c b/drivers/staging/imx-drm/imx-drm-core.c
index 47c5888..98eac33 100644
--- a/drivers/staging/imx-drm/imx-drm-core.c
+++ b/drivers/staging/imx-drm/imx-drm-core.c
@@ -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

Re: [PATCH] imx-drm: Fix probe failure

[Greg KH]

On Tue, Aug 27, 2013 at 11:32:43PM -0300, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@freescale.com>
> 
> Since commit b5dc0d10 (drm/imx: kill firstopen callback) the following probe
> failure is seen:

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

Re: [PATCH] imx-drm: Fix probe failure

[Fabio Estevam]

Hi Greg,

On Wed, Aug 28, 2013 at 12:11 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Tue, Aug 27, 2013 at 11:32:43PM -0300, Fabio Estevam wrote:
>> From: Fabio Estevam <fabio.estevam@freescale.com>
>>
>> Since commit b5dc0d10 (drm/imx: kill firstopen callback) the following probe
>> failure is seen:
>
> I don't have that commit in any tree I control, so I can't apply this
> patch.  What tree is this commit in?

Ok, understood.

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

commit b5dc0d108cd3c0b50ddcb6f6c54be1
bea4c39e01
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
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.

    Cc: Sascha Hauer <s.hauer@pengutronix.de>
    Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
    Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
    Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
    Signed-off-by: Dave Airlie <airlied@redhat.com>

Will resend it to Dave then.

Thanks,

Fabio Estevam

Re: [PATCH] imx-drm: Fix probe failure

[Sascha Hauer]

On Tue, Aug 27, 2013 at 11:32:43PM -0300, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@freescale.com>
> 
> 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.
> 
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
> ---
>  drivers/staging/imx-drm/imx-drm-core.c | 25 +------------------------
>  1 file changed, 1 insertion(+), 24 deletions(-)
> 
> diff --git a/drivers/staging/imx-drm/imx-drm-core.c b/drivers/staging/imx-drm/imx-drm-core.c
> index 47c5888..98eac33 100644
> --- a/drivers/staging/imx-drm/imx-drm-core.c
> +++ b/drivers/staging/imx-drm/imx-drm-core.c
> @@ -41,7 +41,6 @@ struct imx_drm_device {
>  	struct list_head			encoder_list;
>  	struct list_head			connector_list;
>  	struct mutex				mutex;
> -	int					references;

Nack. We need the references to prevent that encoders/connectors are
(de)registered while the device is active. Simply dropping this code is
no solution.

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 |

Re: [PATCH] imx-drm: Fix probe failure

[Fabio Estevam]

On Wed, Aug 28, 2013 at 2:27 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:

> Nack. We need the references to prevent that encoders/connectors are
> (de)registered while the device is active. Simply dropping this code is
> no solution.

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

Re: [PATCH] imx-drm: Fix probe failure

[Sascha Hauer]

On Wed, Aug 28, 2013 at 11:39:11AM -0300, Fabio Estevam wrote:
> On Wed, Aug 28, 2013 at 2:27 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> 
> > Nack. We need the references to prevent that encoders/connectors are
> > (de)registered while the device is active. Simply dropping this code is
> > no solution.
> 
> 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.

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 |

Re: [PATCH] imx-drm: Fix probe failure

[Fabio Estevam]

On Wed, Aug 28, 2013 at 3:54 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote:

> Do the open/close callbacks work?

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?

Re: [PATCH] imx-drm: Fix probe failure

[Sascha Hauer]

On Wed, Aug 28, 2013 at 05:10:16PM -0300, Fabio Estevam wrote:
> On Wed, Aug 28, 2013 at 3:54 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> 
> > Do the open/close callbacks work?
> 
> The 'open' callback maps to the generic 'drm_open', so we cannot use it.

We could replace this, but...

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

...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 |

Re: [PATCH] imx-drm: Fix probe failure

[Fabio Estevam]

On Wed, Aug 28, 2013 at 5:34 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> On Wed, Aug 28, 2013 at 05:10:16PM -0300, Fabio Estevam wrote:
>> On Wed, Aug 28, 2013 at 3:54 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
>>
>> > Do the open/close callbacks work?
>>
>> The 'open' callback maps to the generic 'drm_open', so we cannot use it.
>
> We could replace this, but...
>
>>
>> Shouldn't we rely on the refcount done by the drm core instead of
>> doing this manually in imx-drm?
>
> ...This could be a better solution.
>
> you mean (struct drm_device)->open_count, right?

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