LinuxLists.cc - [PATCH v2] imx-drm: Fix probe failure

2013-08-29 13:59:40

Subject: [PATCH v2] imx-drm: Fix probe failure

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

The reason for the probe failure is that now 'imxdrm->references' is incremented
early in imx_drm_driver_load(), so the following checks in imx_drm_add_crtc()
and imx_drm_add_encoder():

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

,will always fail.

Let the drm core handle the references instead of doing this manually in
imx-drm. In imx-drm-core the open and close callbacks map to drm_open and
drm_close, which handle the references via open_count.

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

Signed-off-by: Fabio Estevam <[email protected]>
---
Changes since v1:
- Improved commit log by providing an explanation to the probe failure
- Cc'ed Dave/Daniel

drivers/staging/imx-drm/imx-drm-core.c | 23 -----------------------
1 file changed, 23 deletions(-)

diff --git a/drivers/staging/imx-drm/imx-drm-core.c b/drivers/staging/imx-drm/imx-drm-core.c
index 47c5888..6f0d24c 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;
@@ -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

2013-08-29 15:21:22

by Daniel Vetter

[permalink] [raw]

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

On Thu, Aug 29, 2013 at 10:59:22AM -0300, Fabio Estevam wrote:
> 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
>
> The reason for the probe failure is that now 'imxdrm->references' is incremented
> early in imx_drm_driver_load(), so the following checks in imx_drm_add_crtc()
> and imx_drm_add_encoder():
>
> if (imxdrm->references) {
> ret = -EBUSY;
> goto err_busy;
> }
>
> ,will always fail.
>
> Let the drm core handle the references instead of doing this manually in
> imx-drm. In imx-drm-core the open and close callbacks map to drm_open and
> drm_close, which handle the references via open_count.
>
> After this patch, lvds panel is functional on a mx6qsabrelite board.
>
> Signed-off-by: Fabio Estevam <[email protected]>

Yeah, just ripping out the imxdrm->references stuff will restore imx, but
of course all the funny races are still fundamentally there. So we still
rely on userspace being really careful to obey the right module loading
sequence and not start any drm client before that has all completed. So

Reviewed-by: Daniel Vetter <[email protected]>

Greg, when you slurp this in can you pls also add the imx todo entry from
the other thread?

http://www.mail-archive.com/[email protected]/msg43698.html

Or do you want me to send in a real patch?

Thanks, Daniel

> ---
> Changes since v1:
> - Improved commit log by providing an explanation to the probe failure
> - Cc'ed Dave/Daniel
>
> drivers/staging/imx-drm/imx-drm-core.c | 23 -----------------------
> 1 file changed, 23 deletions(-)
>
> diff --git a/drivers/staging/imx-drm/imx-drm-core.c b/drivers/staging/imx-drm/imx-drm-core.c
> index 47c5888..6f0d24c 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;
> @@ -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
>
>

--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

2013-08-29 18:14:23

by Sascha Hauer

[permalink] [raw]

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

2013-08-29 18:30:15

by Fabio Estevam

[permalink] [raw]

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

On Thu, Aug 29, 2013 at 3:14 PM, Sascha Hauer <[email protected]> wrote:
> On Thu, Aug 29, 2013 at 10:59:22AM -0300, Fabio Estevam wrote:
>> 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
>>
>> The reason for the probe failure is that now 'imxdrm->references' is incremented
>> early in imx_drm_driver_load(), so the following checks in imx_drm_add_crtc()
>> and imx_drm_add_encoder():
>>
>> if (imxdrm->references) {
>> ret = -EBUSY;
>> goto err_busy;
>> }
>>
>> ,will always fail.
>>
>> Let the drm core handle the references instead of doing this manually in
>> imx-drm. In imx-drm-core the open and close callbacks map to drm_open and
>> drm_close, which handle the references via open_count.
>>
>> After this patch, lvds panel is functional on a mx6qsabrelite board.
>>
>> Signed-off-by: Fabio Estevam <[email protected]>
>> ---
>> Changes since v1:
>> - Improved commit log by providing an explanation to the probe failure
>> - Cc'ed Dave/Daniel
>>
>> @@ -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;
>> - }
>
> As I told in v1: Simply removing the code is not an option. We agreed
> that we use the drm core reference counting, so you must actually test
> for it here...

Yes, we did, but after reading a separate thread on this topic, I
learned that drm core does not support it currently.

Daniel mentions [1]: "And I wouldn't worry about module unloading and
refcounting for
now since core drm is terminally broken in that area already anyway"

[1] http://www.mail-archive.com/[email protected]/msg43698.html

Can't we just apply this patch so that imx-drm can be functional again
and fix module unloading/refcount later?

2013-08-29 18:31:21

by Sascha Hauer

[permalink] [raw]

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

On Thu, Aug 29, 2013 at 05:21:30PM +0200, Daniel Vetter wrote:
> On Thu, Aug 29, 2013 at 10:59:22AM -0300, Fabio Estevam wrote:
> > 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
> >
> > The reason for the probe failure is that now 'imxdrm->references' is incremented
> > early in imx_drm_driver_load(), so the following checks in imx_drm_add_crtc()
> > and imx_drm_add_encoder():
> >
> > if (imxdrm->references) {
> > ret = -EBUSY;
> > goto err_busy;
> > }
> >
> > ,will always fail.
> >
> > Let the drm core handle the references instead of doing this manually in
> > imx-drm. In imx-drm-core the open and close callbacks map to drm_open and
> > drm_close, which handle the references via open_count.
> >
> > After this patch, lvds panel is functional on a mx6qsabrelite board.
> >
> > Signed-off-by: Fabio Estevam <[email protected]>
>
> Yeah, just ripping out the imxdrm->references stuff will restore imx, but
> of course all the funny races are still fundamentally there. So we still
> rely on userspace being really careful to obey the right module loading
> sequence and not start any drm client before that has all completed. So
>
> Reviewed-by: Daniel Vetter <[email protected]>
>
> Greg, when you slurp this in can you pls also add the imx todo entry from
> the other thread?
>
> http://www.mail-archive.com/[email protected]/msg43698.html

You said there:

> +- Fix up the driver load sequence to make sure all submodules for encoders/crtcs
> + are fully loaded before the drm driver is registered.

We can't know when all submodules are registered and really I think this
is not necessary. The driver will use the components available during
open time. When being opened no components will be registered/deregistered.
So a user will always get a functional drm device with the components he
actually loaded.

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 |

2013-08-29 18:37:13

by Sascha Hauer

[permalink] [raw]

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

On Thu, Aug 29, 2013 at 03:30:12PM -0300, Fabio Estevam wrote:
> >> @@ -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;
> >> - }
> >
> > As I told in v1: Simply removing the code is not an option. We agreed
> > that we use the drm core reference counting, so you must actually test
> > for it here...
>
> Yes, we did, but after reading a separate thread on this topic, I
> learned that drm core does not support it currently.
>
> Daniel mentions [1]: "And I wouldn't worry about module unloading and
> refcounting for
> now since core drm is terminally broken in that area already anyway"
>
> [1] http://www.mail-archive.com/[email protected]/msg43698.html
>
> Can't we just apply this patch so that imx-drm can be functional again
> and fix module unloading/refcount later?

It's not about the module refcount. It's only that a on a drm device
which is in use you better do not add/remove components.

Again: The imx-drm driver does *not* do hotplugging. It will *not* add
or remove components when the device is opened. The code you remove
exactly makes (or made before Daniels patch) that sure.

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 |

2013-08-30 16:08:17

by Fabio Estevam

[permalink] [raw]

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

Hi Sascha,

On Thu, Aug 29, 2013 at 3:37 PM, Sascha Hauer <[email protected]> wrote:
> It's not about the module refcount. It's only that a on a drm device
> which is in use you better do not add/remove components.
>
> Again: The imx-drm driver does *not* do hotplugging. It will *not* add
> or remove components when the device is opened. The code you remove
> exactly makes (or made before Daniels patch) that sure.

I am not sure what would be the correct fix for this.

What about this?

--- a/drivers/staging/imx-drm/imx-drm-core.c
+++ b/drivers/staging/imx-drm/imx-drm-core.c
@@ -438,6 +438,8 @@ static int imx_drm_driver_load(struct drm_device
*drm, unsigned long flags)
ret = -EINVAL;

ret = 0;
+
+ imxdrm->references = -1;

err_init:
mutex_unlock(&imxdrm->mutex);
@@ -485,7 +487,7 @@ int imx_drm_add_crtc(struct drm_crtc *crtc,

mutex_lock(&imxdrm->mutex);

- if (imxdrm->references) {
+ if (imxdrm->references > 0) {
ret = -EBUSY;
goto err_busy;
}
@@ -564,7 +566,7 @@ int imx_drm_add_encoder(struct drm_encoder *encoder,

mutex_lock(&imxdrm->mutex);

- if (imxdrm->references) {
+ if (imxdrm->references > 0) {
ret = -EBUSY;
goto err_busy;
}
@@ -709,7 +711,7 @@ int imx_drm_add_connector(struct drm_connector *connector,

mutex_lock(&imxdrm->mutex);

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

2013-08-30 17:06:26

by Sascha Hauer

[permalink] [raw]

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

On Fri, Aug 30, 2013 at 01:08:14PM -0300, Fabio Estevam wrote:
> Hi Sascha,
>
> On Thu, Aug 29, 2013 at 3:37 PM, Sascha Hauer <[email protected]> wrote:
> > It's not about the module refcount. It's only that a on a drm device
> > which is in use you better do not add/remove components.
> >
> > Again: The imx-drm driver does *not* do hotplugging. It will *not* add
> > or remove components when the device is opened. The code you remove
> > exactly makes (or made before Daniels patch) that sure.
>
> I am not sure what would be the correct fix for this.

Why not use (struct drm_device)->open_count as suggested earlier?

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 |