* Re: [PATCH] drm: Fix deadlock due to getconnector locking changes
2015-02-22 10:38 [PATCH] drm: Fix deadlock due to getconnector locking changes Daniel Vetter
@ 2015-02-22 13:55 ` shuang.he
2015-02-23 8:52 ` Jani Nikula
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: shuang.he @ 2015-02-22 13:55 UTC (permalink / raw)
To: shuang.he, ethan.gao, intel-gfx, daniel.vetter
Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 5803
-------------------------------------Summary-------------------------------------
Platform Delta drm-intel-nightly Series Applied
PNV -5 277/277 272/277
ILK 313/313 313/313
SNB 309/309 309/309
IVB 382/382 382/382
BYT 296/296 296/296
HSW 425/425 425/425
BDW -1 318/318 317/318
-------------------------------------Detailed-------------------------------------
Platform Test drm-intel-nightly Series Applied
PNV igt_gem_userptr_blits_coherency-sync NO_RESULT(1)CRASH(7)NRUN(1)PASS(6) CRASH(2)
PNV igt_gem_userptr_blits_coherency-unsync CRASH(5)NRUN(1)PASS(4) CRASH(1)NRUN(1)
*PNV igt_gem_userptr_blits_create-destroy-sync PASS(4) NRUN(1)PASS(1)
*PNV igt_gem_userptr_blits_dmabuf-sync PASS(3) NRUN(1)PASS(1)
*PNV igt_gem_userptr_blits_forked-sync-interruptible PASS(3) NRUN(1)PASS(1)
*BDW igt_gem_gtt_hog PASS(20) DMESG_WARN(1)PASS(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] drm: Fix deadlock due to getconnector locking changes
2015-02-22 10:38 [PATCH] drm: Fix deadlock due to getconnector locking changes Daniel Vetter
2015-02-22 13:55 ` shuang.he
@ 2015-02-23 8:52 ` Jani Nikula
2015-02-23 12:28 ` Rob Clark
2015-02-23 23:55 ` Marc Finet
3 siblings, 0 replies; 6+ messages in thread
From: Jani Nikula @ 2015-02-23 8:52 UTC (permalink / raw)
To: DRI Development
Cc: Marc Finet, Daniel Vetter, Intel Graphics Development,
Daniel Vetter
On Sun, 22 Feb 2015, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> In
>
> daniel@phenom:~/linux/src$ git show ccfc08655
copy-paste fail?
J.
> commit ccfc08655d5fd5076828f45fb09194c070f2f63a
> Author: Rob Clark <robdclark@gmail.com>
> Date: Thu Dec 18 16:01:48 2014 -0500
>
> drm: tweak getconnector locking
>
> We need to extend the locking to cover connector->state reading for
> atomic drivers, but the above commit was a bit too eager and also
> included the fill_modes callback. Which on i915 on old platforms using
> load detection needs to acquire modeset locks, resulting in a deadlock
> on output probing.
>
> Reported-by: Marc Finet <m.dreadlock@gmail.com>
> Cc: Marc Finet <m.dreadlock@gmail.com>
> Cc: robdclark@gmail.com
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
> drivers/gpu/drm/drm_crtc.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index b15d720eda4c..ce5f1193ecd6 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -2180,7 +2180,6 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
> DRM_DEBUG_KMS("[CONNECTOR:%d:?]\n", out_resp->connector_id);
>
> mutex_lock(&dev->mode_config.mutex);
> - drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>
> connector = drm_connector_find(dev, out_resp->connector_id);
> if (!connector) {
> @@ -2210,6 +2209,8 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
> out_resp->mm_height = connector->display_info.height_mm;
> out_resp->subpixel = connector->display_info.subpixel_order;
> out_resp->connection = connector->status;
> +
> + drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> encoder = drm_connector_get_encoder(connector);
> if (encoder)
> out_resp->encoder_id = encoder->base.id;
> --
> 2.1.4
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] drm: Fix deadlock due to getconnector locking changes
2015-02-22 10:38 [PATCH] drm: Fix deadlock due to getconnector locking changes Daniel Vetter
2015-02-22 13:55 ` shuang.he
2015-02-23 8:52 ` Jani Nikula
@ 2015-02-23 12:28 ` Rob Clark
2015-02-23 23:55 ` Marc Finet
3 siblings, 0 replies; 6+ messages in thread
From: Rob Clark @ 2015-02-23 12:28 UTC (permalink / raw)
To: Daniel Vetter
Cc: Marc Finet, Daniel Vetter, Intel Graphics Development,
DRI Development
On Sun, Feb 22, 2015 at 5:38 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> In
>
> daniel@phenom:~/linux/src$ git show ccfc08655
> commit ccfc08655d5fd5076828f45fb09194c070f2f63a
> Author: Rob Clark <robdclark@gmail.com>
> Date: Thu Dec 18 16:01:48 2014 -0500
>
> drm: tweak getconnector locking
>
> We need to extend the locking to cover connector->state reading for
> atomic drivers, but the above commit was a bit too eager and also
> included the fill_modes callback. Which on i915 on old platforms using
> load detection needs to acquire modeset locks, resulting in a deadlock
> on output probing.
>
> Reported-by: Marc Finet <m.dreadlock@gmail.com>
> Cc: Marc Finet <m.dreadlock@gmail.com>
> Cc: robdclark@gmail.com
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Reviewed-by: Rob Clark <robdclark@gmail.com>
> ---
> drivers/gpu/drm/drm_crtc.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index b15d720eda4c..ce5f1193ecd6 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -2180,7 +2180,6 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
> DRM_DEBUG_KMS("[CONNECTOR:%d:?]\n", out_resp->connector_id);
>
> mutex_lock(&dev->mode_config.mutex);
> - drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>
> connector = drm_connector_find(dev, out_resp->connector_id);
> if (!connector) {
> @@ -2210,6 +2209,8 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
> out_resp->mm_height = connector->display_info.height_mm;
> out_resp->subpixel = connector->display_info.subpixel_order;
> out_resp->connection = connector->status;
> +
> + drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> encoder = drm_connector_get_encoder(connector);
> if (encoder)
> out_resp->encoder_id = encoder->base.id;
> --
> 2.1.4
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] drm: Fix deadlock due to getconnector locking changes
2015-02-22 10:38 [PATCH] drm: Fix deadlock due to getconnector locking changes Daniel Vetter
` (2 preceding siblings ...)
2015-02-23 12:28 ` Rob Clark
@ 2015-02-23 23:55 ` Marc Finet
2015-03-02 20:03 ` Tommi Rantala
3 siblings, 1 reply; 6+ messages in thread
From: Marc Finet @ 2015-02-23 23:55 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Daniel Vetter, Intel Graphics Development, DRI Development
On Sun, Feb 22, 2015 at 11:38:36AM +0100, Daniel Vetter wrote:
> In
>
> daniel@phenom:~/linux/src$ git show ccfc08655
> commit ccfc08655d5fd5076828f45fb09194c070f2f63a
> Author: Rob Clark <robdclark@gmail.com>
> Date: Thu Dec 18 16:01:48 2014 -0500
>
> drm: tweak getconnector locking
>
> We need to extend the locking to cover connector->state reading for
> atomic drivers, but the above commit was a bit too eager and also
> included the fill_modes callback. Which on i915 on old platforms using
> load detection needs to acquire modeset locks, resulting in a deadlock
> on output probing.
>
> Reported-by: Marc Finet <m.dreadlock@gmail.com>
> Cc: Marc Finet <m.dreadlock@gmail.com>
> Cc: robdclark@gmail.com
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Tested-by: Marc Finet <m.dreadlock@gmail.com>
Thanks.
> ---
> drivers/gpu/drm/drm_crtc.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index b15d720eda4c..ce5f1193ecd6 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -2180,7 +2180,6 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
> DRM_DEBUG_KMS("[CONNECTOR:%d:?]\n", out_resp->connector_id);
>
> mutex_lock(&dev->mode_config.mutex);
> - drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>
> connector = drm_connector_find(dev, out_resp->connector_id);
> if (!connector) {
> @@ -2210,6 +2209,8 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
> out_resp->mm_height = connector->display_info.height_mm;
> out_resp->subpixel = connector->display_info.subpixel_order;
> out_resp->connection = connector->status;
> +
> + drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> encoder = drm_connector_get_encoder(connector);
> if (encoder)
> out_resp->encoder_id = encoder->base.id;
> --
> 2.1.4
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] drm: Fix deadlock due to getconnector locking changes
2015-02-23 23:55 ` Marc Finet
@ 2015-03-02 20:03 ` Tommi Rantala
0 siblings, 0 replies; 6+ messages in thread
From: Tommi Rantala @ 2015-03-02 20:03 UTC (permalink / raw)
To: Marc Finet
Cc: Daniel Vetter, Intel Graphics Development, trinity,
DRI Development, Daniel Vetter
2015-02-24 1:55 GMT+02:00 Marc Finet <m.dreadlock@gmail.com>:
> On Sun, Feb 22, 2015 at 11:38:36AM +0100, Daniel Vetter wrote:
>> In
>>
>> daniel@phenom:~/linux/src$ git show ccfc08655
>> commit ccfc08655d5fd5076828f45fb09194c070f2f63a
>> Author: Rob Clark <robdclark@gmail.com>
>> Date: Thu Dec 18 16:01:48 2014 -0500
>>
>> drm: tweak getconnector locking
>>
>> We need to extend the locking to cover connector->state reading for
>> atomic drivers, but the above commit was a bit too eager and also
>> included the fill_modes callback. Which on i915 on old platforms using
>> load detection needs to acquire modeset locks, resulting in a deadlock
>> on output probing.
>>
>> Reported-by: Marc Finet <m.dreadlock@gmail.com>
>> Cc: Marc Finet <m.dreadlock@gmail.com>
>> Cc: robdclark@gmail.com
>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>
> Tested-by: Marc Finet <m.dreadlock@gmail.com>
>
> Thanks.
>> ---
>> drivers/gpu/drm/drm_crtc.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>> index b15d720eda4c..ce5f1193ecd6 100644
>> --- a/drivers/gpu/drm/drm_crtc.c
>> +++ b/drivers/gpu/drm/drm_crtc.c
>> @@ -2180,7 +2180,6 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
>> DRM_DEBUG_KMS("[CONNECTOR:%d:?]\n", out_resp->connector_id);
>>
>> mutex_lock(&dev->mode_config.mutex);
>> - drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>>
>> connector = drm_connector_find(dev, out_resp->connector_id);
>> if (!connector) {
>> @@ -2210,6 +2209,8 @@ int drm_mode_getconnector(struct drm_device *dev, void *data,
>> out_resp->mm_height = connector->display_info.height_mm;
>> out_resp->subpixel = connector->display_info.subpixel_order;
>> out_resp->connection = connector->status;
>> +
>> + drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>> encoder = drm_connector_get_encoder(connector);
>> if (encoder)
>> out_resp->encoder_id = encoder->base.id;
>> --
>> 2.1.4
>>
Hi,
I think this patch introduced the following.
If the drm_connector_find() call fails, we jump to the 'out' label and
call drm_modeset_unlock(), before the drm_modeset_lock() call.
[ 59.076011] =====================================
[ 59.076011] [ BUG: bad unlock balance detected! ]
[ 59.076011] 4.0.0-rc1+ #60 Tainted: G W
[ 59.076011] -------------------------------------
[ 59.076011] trinity-c14/2418 is trying to release lock
(crtc_ww_class_mutex) at:
[ 59.076011] [<ffffffff81dc2872>] ww_mutex_unlock+0x42/0xb0
[ 59.076011] but there are no more locks to release!
[ 59.076011]
[ 59.076011] other info that might help us debug this:
[ 59.076011] 1 lock held by trinity-c14/2418:
[ 59.076011] #0: (&dev->mode_config.mutex){+.+.+.}, at:
[<ffffffff815ef253>] drm_mode_getconnector+0x83/0x400
[ 59.076011]
[ 59.076011] stack backtrace:
[ 59.076011] CPU: 0 PID: 2418 Comm: trinity-c14 Tainted: G W
4.0.0-rc1+ #60
[ 59.076011] Hardware name: Hewlett-Packard HP Compaq dc5750 Small
Form Factor/0A64h, BIOS 786E3 v02.10 01/25/2007
[ 59.076011] ffffffff81dc2872 ffff88005b777b48 ffffffff81db6706
0000000000000000
[ 59.076011] ffff88005b719440 ffff88005b777b78 ffffffff810f8074
00000000001d51c0
[ 59.076011] ffff880069d3a5d0 ffff88005b719440 ffffffff81dc2872
ffff88005b777bf8
[ 59.076011] Call Trace:
[ 59.076011] [<ffffffff81dc2872>] ? ww_mutex_unlock+0x42/0xb0
[ 59.076011] [<ffffffff81db6706>] dump_stack+0x4c/0x65
[ 59.076011] [<ffffffff810f8074>] print_unlock_imbalance_bug+0xf4/0x100
[ 59.076011] [<ffffffff81dc2872>] ? ww_mutex_unlock+0x42/0xb0
[ 59.076011] [<ffffffff810fc76e>] lock_release_non_nested+0x24e/0x350
[ 59.076011] [<ffffffff810f8eb5>] ? mark_held_locks+0x75/0xa0
[ 59.076011] [<ffffffff81dc275d>] ? __mutex_unlock_slowpath+0xad/0x170
[ 59.076011] [<ffffffff81dc2872>] ? ww_mutex_unlock+0x42/0xb0
[ 59.076011] [<ffffffff810fcd7c>] lock_release+0xbc/0x380
[ 59.076011] [<ffffffff81dc2721>] __mutex_unlock_slowpath+0x71/0x170
[ 59.076011] [<ffffffff815ef253>] ? drm_mode_getconnector+0x83/0x400
[ 59.076011] [<ffffffff81dc2872>] ww_mutex_unlock+0x42/0xb0
[ 59.076011] [<ffffffff815fc21f>] drm_modeset_unlock+0x2f/0x40
[ 59.076011] [<ffffffff815ef450>] drm_mode_getconnector+0x280/0x400
[ 59.076011] [<ffffffff811cfb07>] ? might_fault+0x57/0xb0
[ 59.076011] [<ffffffff815e1f7c>] drm_ioctl+0x19c/0x640
[ 59.076011] [<ffffffff810f90ad>] ? trace_hardirqs_on+0xd/0x10
[ 59.076011] [<ffffffff8160c066>] radeon_drm_ioctl+0x46/0x80
[ 59.076011] [<ffffffff81211868>] do_vfs_ioctl+0x318/0x570
[ 59.076011] [<ffffffff81462ef6>] ? selinux_file_ioctl+0x56/0x110
[ 59.076011] [<ffffffff81211b41>] SyS_ioctl+0x81/0xa0
[ 59.076011] [<ffffffff81dc6492>] system_call_fastpath+0x12/0x17
[ 59.316190] [drm:radeon_gem_pread_ioctl] *ERROR* unimplemented
radeon_gem_pread_ioctl
[ 59.579952] [drm:radeon_info_ioctl] *ERROR* copy_to_user
radeon_info_ioctl:555
[watchdog] kernel became tainted! (512/0) Last seed was 1541132817
Tommi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 6+ messages in thread