* [PATCH] drm: Reverse lock order in pan_display_legacy()
@ 2019-06-11 11:57 Thomas Zimmermann
2019-06-11 12:37 ` Daniel Vetter
0 siblings, 1 reply; 4+ messages in thread
From: Thomas Zimmermann @ 2019-06-11 11:57 UTC (permalink / raw)
To: noralf, sam, daniel; +Cc: Thomas Zimmermann, dri-devel
Acquiring drm_client_dev.modeset_mutex after the locks in drm_fb_helper.dev
creates a deadlock with drm_setup_crtcs() as shown below:
[ 4.959319] fbcon: radeondrmfb (fb0) is primary device
[ 4.993952] Console: switching to colour frame buffer device 240x67
[ 4.994040]
[ 4.994041] ======================================================
[ 4.994041] WARNING: possible circular locking dependency detected
[ 4.994042] 5.2.0-rc4-1-default+ #39 Tainted: G E
[ 4.994043] ------------------------------------------------------
[ 4.994043] systemd-udevd/369 is trying to acquire lock:
[ 4.994044] 00000000fb622acb (&client->modeset_mutex){+.+.}, at: drm_fb_helper_pan_display+0x103/0x1f0 [drm_kms_helper]
[ 4.994055]
[ 4.994055] but task is already holding lock:
[ 4.994055] 0000000028767ae4 (crtc_ww_class_mutex){+.+.}, at: drm_modeset_lock+0x42/0xf0 [drm]
[ 4.994072]
[ 4.994072] which lock already depends on the new lock.
[ 4.994072]
[ 4.994072]
[ 4.994072] the existing dependency chain (in reverse order) is:
[ 4.994073]
[ 4.994073] -> #3 (crtc_ww_class_mutex){+.+.}:
[ 4.994076] lock_acquire+0x9e/0x170
[ 4.994079] __ww_mutex_lock.constprop.18+0x97/0xf40
[ 4.994080] ww_mutex_lock+0x30/0x90
[ 4.994091] drm_modeset_lock+0x42/0xf0 [drm]
[ 4.994102] drm_modeset_lock_all_ctx+0x1f/0xe0 [drm]
[ 4.994113] drm_modeset_lock_all+0x5e/0x1a0 [drm]
[ 4.994163] intel_modeset_init+0x60b/0xda0 [i915]
..
[ 4.994253]
[ 4.994253] -> #2 (crtc_ww_class_acquire){+.+.}:
[ 4.994255] lock_acquire+0x9e/0x170
[ 4.994270] drm_modeset_acquire_init+0xcc/0x100 [drm]
[ 4.994280] drm_modeset_lock_all+0x44/0x1a0 [drm]
[ 4.994320] intel_modeset_init+0x60b/0xda0 [i915]
..
[ 4.994403]
[ 4.994403] -> #1 (&dev->mode_config.mutex){+.+.}:
[ 4.994405] lock_acquire+0x9e/0x170
[ 4.994408] __mutex_lock+0x62/0x8c0
[ 4.994413] drm_setup_crtcs+0x17c/0xc50 [drm_kms_helper]
[ 4.994418] __drm_fb_helper_initial_config_and_unlock+0x34/0x530 [drm_kms_helper]
[ 4.994450] radeon_fbdev_init+0x110/0x130 [radeon]
..
[ 4.994535]
[ 4.994535] -> #0 (&client->modeset_mutex){+.+.}:
[ 4.994537] __lock_acquire+0xa85/0xe90
[ 4.994538] lock_acquire+0x9e/0x170
[ 4.994540] __mutex_lock+0x62/0x8c0
[ 4.994545] drm_fb_helper_pan_display+0x103/0x1f0 [drm_kms_helper]
[ 4.994547] fb_pan_display+0x92/0x120
[ 4.994549] bit_update_start+0x1a/0x40
[ 4.994550] fbcon_switch+0x392/0x580
[ 4.994552] redraw_screen+0x12c/0x220
[ 4.994553] do_bind_con_driver.cold.30+0xe1/0x10d
[ 4.994554] do_take_over_console+0x113/0x190
[ 4.994555] do_fbcon_takeover+0x58/0xb0
[ 4.994557] notifier_call_chain+0x47/0x70
[ 4.994558] blocking_notifier_call_chain+0x44/0x60
[ 4.994559] register_framebuffer+0x231/0x310
[ 4.994564] __drm_fb_helper_initial_config_and_unlock+0x2fd/0x530 [drm_kms_helper]
[ 4.994590] radeon_fbdev_init+0x110/0x130 [radeon]
..
This problem was introduced in
d81294afe drm/fb-helper: Remove drm_fb_helper_crtc
Reversing the lock ordering in pan_display_legacy() fixes the issue. The fix
was suggested by Daniel Vetter.
Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Fixes: d81294afeecdacc8d84804ba0bcb3d39e64d0f27
---
drivers/gpu/drm/drm_fb_helper.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 7b388674a456..d6991f07cb17 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -1586,8 +1586,8 @@ static int pan_display_legacy(struct fb_var_screeninfo *var,
struct drm_mode_set *modeset;
int ret = 0;
- drm_modeset_lock_all(fb_helper->dev);
mutex_lock(&client->modeset_mutex);
+ drm_modeset_lock_all(fb_helper->dev);
drm_client_for_each_modeset(modeset, client) {
modeset->x = var->xoffset;
modeset->y = var->yoffset;
@@ -1600,8 +1600,8 @@ static int pan_display_legacy(struct fb_var_screeninfo *var,
}
}
}
- mutex_unlock(&client->modeset_mutex);
drm_modeset_unlock_all(fb_helper->dev);
+ mutex_unlock(&client->modeset_mutex);
return ret;
}
--
2.21.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] drm: Reverse lock order in pan_display_legacy()
2019-06-11 11:57 [PATCH] drm: Reverse lock order in pan_display_legacy() Thomas Zimmermann
@ 2019-06-11 12:37 ` Daniel Vetter
2019-06-11 13:29 ` Noralf Trønnes
0 siblings, 1 reply; 4+ messages in thread
From: Daniel Vetter @ 2019-06-11 12:37 UTC (permalink / raw)
To: Thomas Zimmermann; +Cc: sam, dri-devel
On Tue, Jun 11, 2019 at 01:57:16PM +0200, Thomas Zimmermann wrote:
> Acquiring drm_client_dev.modeset_mutex after the locks in drm_fb_helper.dev
> creates a deadlock with drm_setup_crtcs() as shown below:
>
> [ 4.959319] fbcon: radeondrmfb (fb0) is primary device
> [ 4.993952] Console: switching to colour frame buffer device 240x67
> [ 4.994040]
> [ 4.994041] ======================================================
> [ 4.994041] WARNING: possible circular locking dependency detected
> [ 4.994042] 5.2.0-rc4-1-default+ #39 Tainted: G E
> [ 4.994043] ------------------------------------------------------
> [ 4.994043] systemd-udevd/369 is trying to acquire lock:
> [ 4.994044] 00000000fb622acb (&client->modeset_mutex){+.+.}, at: drm_fb_helper_pan_display+0x103/0x1f0 [drm_kms_helper]
> [ 4.994055]
> [ 4.994055] but task is already holding lock:
> [ 4.994055] 0000000028767ae4 (crtc_ww_class_mutex){+.+.}, at: drm_modeset_lock+0x42/0xf0 [drm]
> [ 4.994072]
> [ 4.994072] which lock already depends on the new lock.
> [ 4.994072]
> [ 4.994072]
> [ 4.994072] the existing dependency chain (in reverse order) is:
> [ 4.994073]
> [ 4.994073] -> #3 (crtc_ww_class_mutex){+.+.}:
> [ 4.994076] lock_acquire+0x9e/0x170
> [ 4.994079] __ww_mutex_lock.constprop.18+0x97/0xf40
> [ 4.994080] ww_mutex_lock+0x30/0x90
> [ 4.994091] drm_modeset_lock+0x42/0xf0 [drm]
> [ 4.994102] drm_modeset_lock_all_ctx+0x1f/0xe0 [drm]
> [ 4.994113] drm_modeset_lock_all+0x5e/0x1a0 [drm]
> [ 4.994163] intel_modeset_init+0x60b/0xda0 [i915]
> ..
> [ 4.994253]
> [ 4.994253] -> #2 (crtc_ww_class_acquire){+.+.}:
> [ 4.994255] lock_acquire+0x9e/0x170
> [ 4.994270] drm_modeset_acquire_init+0xcc/0x100 [drm]
> [ 4.994280] drm_modeset_lock_all+0x44/0x1a0 [drm]
> [ 4.994320] intel_modeset_init+0x60b/0xda0 [i915]
> ..
> [ 4.994403]
> [ 4.994403] -> #1 (&dev->mode_config.mutex){+.+.}:
> [ 4.994405] lock_acquire+0x9e/0x170
> [ 4.994408] __mutex_lock+0x62/0x8c0
> [ 4.994413] drm_setup_crtcs+0x17c/0xc50 [drm_kms_helper]
> [ 4.994418] __drm_fb_helper_initial_config_and_unlock+0x34/0x530 [drm_kms_helper]
> [ 4.994450] radeon_fbdev_init+0x110/0x130 [radeon]
> ..
> [ 4.994535]
> [ 4.994535] -> #0 (&client->modeset_mutex){+.+.}:
> [ 4.994537] __lock_acquire+0xa85/0xe90
> [ 4.994538] lock_acquire+0x9e/0x170
> [ 4.994540] __mutex_lock+0x62/0x8c0
> [ 4.994545] drm_fb_helper_pan_display+0x103/0x1f0 [drm_kms_helper]
> [ 4.994547] fb_pan_display+0x92/0x120
> [ 4.994549] bit_update_start+0x1a/0x40
> [ 4.994550] fbcon_switch+0x392/0x580
> [ 4.994552] redraw_screen+0x12c/0x220
> [ 4.994553] do_bind_con_driver.cold.30+0xe1/0x10d
> [ 4.994554] do_take_over_console+0x113/0x190
> [ 4.994555] do_fbcon_takeover+0x58/0xb0
> [ 4.994557] notifier_call_chain+0x47/0x70
> [ 4.994558] blocking_notifier_call_chain+0x44/0x60
> [ 4.994559] register_framebuffer+0x231/0x310
> [ 4.994564] __drm_fb_helper_initial_config_and_unlock+0x2fd/0x530 [drm_kms_helper]
> [ 4.994590] radeon_fbdev_init+0x110/0x130 [radeon]
> ..
>
> This problem was introduced in
>
> d81294afe drm/fb-helper: Remove drm_fb_helper_crtc
>
> Reversing the lock ordering in pan_display_legacy() fixes the issue. The fix
> was suggested by Daniel Vetter.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Fixes: d81294afeecdacc8d84804ba0bcb3d39e64d0f27
I think for ocd consistency it be nice to pull the lock out from both
pan_display_atomic and pan_disaply_legacy and move it into
drm_fb_helper_pan_display. Like we do drm_fb_helper_dpms or
drm_fb_helper_setcmap or restore_fbdev_mode_force.
Either way Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
> drivers/gpu/drm/drm_fb_helper.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 7b388674a456..d6991f07cb17 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -1586,8 +1586,8 @@ static int pan_display_legacy(struct fb_var_screeninfo *var,
> struct drm_mode_set *modeset;
> int ret = 0;
>
> - drm_modeset_lock_all(fb_helper->dev);
> mutex_lock(&client->modeset_mutex);
> + drm_modeset_lock_all(fb_helper->dev);
> drm_client_for_each_modeset(modeset, client) {
> modeset->x = var->xoffset;
> modeset->y = var->yoffset;
> @@ -1600,8 +1600,8 @@ static int pan_display_legacy(struct fb_var_screeninfo *var,
> }
> }
> }
> - mutex_unlock(&client->modeset_mutex);
> drm_modeset_unlock_all(fb_helper->dev);
> + mutex_unlock(&client->modeset_mutex);
>
> return ret;
> }
> --
> 2.21.0
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] drm: Reverse lock order in pan_display_legacy()
2019-06-11 12:37 ` Daniel Vetter
@ 2019-06-11 13:29 ` Noralf Trønnes
2019-06-11 13:52 ` Thomas Zimmermann
0 siblings, 1 reply; 4+ messages in thread
From: Noralf Trønnes @ 2019-06-11 13:29 UTC (permalink / raw)
To: Daniel Vetter, Thomas Zimmermann; +Cc: sam, dri-devel
Den 11.06.2019 14.37, skrev Daniel Vetter:
> On Tue, Jun 11, 2019 at 01:57:16PM +0200, Thomas Zimmermann wrote:
>> Acquiring drm_client_dev.modeset_mutex after the locks in drm_fb_helper.dev
>> creates a deadlock with drm_setup_crtcs() as shown below:
>>
>> [ 4.959319] fbcon: radeondrmfb (fb0) is primary device
>> [ 4.993952] Console: switching to colour frame buffer device 240x67
>> [ 4.994040]
>> [ 4.994041] ======================================================
>> [ 4.994041] WARNING: possible circular locking dependency detected
>> [ 4.994042] 5.2.0-rc4-1-default+ #39 Tainted: G E
>> [ 4.994043] ------------------------------------------------------
>> [ 4.994043] systemd-udevd/369 is trying to acquire lock:
>> [ 4.994044] 00000000fb622acb (&client->modeset_mutex){+.+.}, at: drm_fb_helper_pan_display+0x103/0x1f0 [drm_kms_helper]
>> [ 4.994055]
>> [ 4.994055] but task is already holding lock:
>> [ 4.994055] 0000000028767ae4 (crtc_ww_class_mutex){+.+.}, at: drm_modeset_lock+0x42/0xf0 [drm]
>> [ 4.994072]
>> [ 4.994072] which lock already depends on the new lock.
>> [ 4.994072]
>> [ 4.994072]
>> [ 4.994072] the existing dependency chain (in reverse order) is:
>> [ 4.994073]
>> [ 4.994073] -> #3 (crtc_ww_class_mutex){+.+.}:
>> [ 4.994076] lock_acquire+0x9e/0x170
>> [ 4.994079] __ww_mutex_lock.constprop.18+0x97/0xf40
>> [ 4.994080] ww_mutex_lock+0x30/0x90
>> [ 4.994091] drm_modeset_lock+0x42/0xf0 [drm]
>> [ 4.994102] drm_modeset_lock_all_ctx+0x1f/0xe0 [drm]
>> [ 4.994113] drm_modeset_lock_all+0x5e/0x1a0 [drm]
>> [ 4.994163] intel_modeset_init+0x60b/0xda0 [i915]
>> ..
>> [ 4.994253]
>> [ 4.994253] -> #2 (crtc_ww_class_acquire){+.+.}:
>> [ 4.994255] lock_acquire+0x9e/0x170
>> [ 4.994270] drm_modeset_acquire_init+0xcc/0x100 [drm]
>> [ 4.994280] drm_modeset_lock_all+0x44/0x1a0 [drm]
>> [ 4.994320] intel_modeset_init+0x60b/0xda0 [i915]
>> ..
>> [ 4.994403]
>> [ 4.994403] -> #1 (&dev->mode_config.mutex){+.+.}:
>> [ 4.994405] lock_acquire+0x9e/0x170
>> [ 4.994408] __mutex_lock+0x62/0x8c0
>> [ 4.994413] drm_setup_crtcs+0x17c/0xc50 [drm_kms_helper]
>> [ 4.994418] __drm_fb_helper_initial_config_and_unlock+0x34/0x530 [drm_kms_helper]
>> [ 4.994450] radeon_fbdev_init+0x110/0x130 [radeon]
>> ..
>> [ 4.994535]
>> [ 4.994535] -> #0 (&client->modeset_mutex){+.+.}:
>> [ 4.994537] __lock_acquire+0xa85/0xe90
>> [ 4.994538] lock_acquire+0x9e/0x170
>> [ 4.994540] __mutex_lock+0x62/0x8c0
>> [ 4.994545] drm_fb_helper_pan_display+0x103/0x1f0 [drm_kms_helper]
>> [ 4.994547] fb_pan_display+0x92/0x120
>> [ 4.994549] bit_update_start+0x1a/0x40
>> [ 4.994550] fbcon_switch+0x392/0x580
>> [ 4.994552] redraw_screen+0x12c/0x220
>> [ 4.994553] do_bind_con_driver.cold.30+0xe1/0x10d
>> [ 4.994554] do_take_over_console+0x113/0x190
>> [ 4.994555] do_fbcon_takeover+0x58/0xb0
>> [ 4.994557] notifier_call_chain+0x47/0x70
>> [ 4.994558] blocking_notifier_call_chain+0x44/0x60
>> [ 4.994559] register_framebuffer+0x231/0x310
>> [ 4.994564] __drm_fb_helper_initial_config_and_unlock+0x2fd/0x530 [drm_kms_helper]
>> [ 4.994590] radeon_fbdev_init+0x110/0x130 [radeon]
>> ..
>>
>> This problem was introduced in
>>
>> d81294afe drm/fb-helper: Remove drm_fb_helper_crtc
>>
>> Reversing the lock ordering in pan_display_legacy() fixes the issue. The fix
>> was suggested by Daniel Vetter.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Fixes: d81294afeecdacc8d84804ba0bcb3d39e64d0f27
>
> I think for ocd consistency it be nice to pull the lock out from both
> pan_display_atomic and pan_disaply_legacy and move it into
> drm_fb_helper_pan_display. Like we do drm_fb_helper_dpms or
> drm_fb_helper_setcmap or restore_fbdev_mode_force.
Is 'lock' referring to modeset_mutex? If so it can't be moved out
because pan_display_atomic() calls drm_client_modeset_commit_force()
which in turn takes the modeset_mutex lock.
The locking in _pan_display isn't so nice looking, but I figured that no
other client would need to do panning so I kept the ugliness in
drm_fb_helper instead of adding complexity to drm_client.
Thanks for fixing this Thomas.
Do you have commit rights or should I apply this?
Acked-by: Noralf Trønnes <noralf@tronnes.org>
Noralf.
>
> Either way Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
>> ---
>> drivers/gpu/drm/drm_fb_helper.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>> index 7b388674a456..d6991f07cb17 100644
>> --- a/drivers/gpu/drm/drm_fb_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>> @@ -1586,8 +1586,8 @@ static int pan_display_legacy(struct fb_var_screeninfo *var,
>> struct drm_mode_set *modeset;
>> int ret = 0;
>>
>> - drm_modeset_lock_all(fb_helper->dev);
>> mutex_lock(&client->modeset_mutex);
>> + drm_modeset_lock_all(fb_helper->dev);
>> drm_client_for_each_modeset(modeset, client) {
>> modeset->x = var->xoffset;
>> modeset->y = var->yoffset;
>> @@ -1600,8 +1600,8 @@ static int pan_display_legacy(struct fb_var_screeninfo *var,
>> }
>> }
>> }
>> - mutex_unlock(&client->modeset_mutex);
>> drm_modeset_unlock_all(fb_helper->dev);
>> + mutex_unlock(&client->modeset_mutex);
>>
>> return ret;
>> }
>> --
>> 2.21.0
>>
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] drm: Reverse lock order in pan_display_legacy()
2019-06-11 13:29 ` Noralf Trønnes
@ 2019-06-11 13:52 ` Thomas Zimmermann
0 siblings, 0 replies; 4+ messages in thread
From: Thomas Zimmermann @ 2019-06-11 13:52 UTC (permalink / raw)
To: Noralf Trønnes, Daniel Vetter; +Cc: sam, dri-devel
[-- Attachment #1.1.1: Type: text/plain, Size: 6648 bytes --]
Hi
Am 11.06.19 um 15:29 schrieb Noralf Trønnes:
>
>
> Den 11.06.2019 14.37, skrev Daniel Vetter:
>> On Tue, Jun 11, 2019 at 01:57:16PM +0200, Thomas Zimmermann wrote:
>>> Acquiring drm_client_dev.modeset_mutex after the locks in drm_fb_helper.dev
>>> creates a deadlock with drm_setup_crtcs() as shown below:
>>>
>>> [ 4.959319] fbcon: radeondrmfb (fb0) is primary device
>>> [ 4.993952] Console: switching to colour frame buffer device 240x67
>>> [ 4.994040]
>>> [ 4.994041] ======================================================
>>> [ 4.994041] WARNING: possible circular locking dependency detected
>>> [ 4.994042] 5.2.0-rc4-1-default+ #39 Tainted: G E
>>> [ 4.994043] ------------------------------------------------------
>>> [ 4.994043] systemd-udevd/369 is trying to acquire lock:
>>> [ 4.994044] 00000000fb622acb (&client->modeset_mutex){+.+.}, at: drm_fb_helper_pan_display+0x103/0x1f0 [drm_kms_helper]
>>> [ 4.994055]
>>> [ 4.994055] but task is already holding lock:
>>> [ 4.994055] 0000000028767ae4 (crtc_ww_class_mutex){+.+.}, at: drm_modeset_lock+0x42/0xf0 [drm]
>>> [ 4.994072]
>>> [ 4.994072] which lock already depends on the new lock.
>>> [ 4.994072]
>>> [ 4.994072]
>>> [ 4.994072] the existing dependency chain (in reverse order) is:
>>> [ 4.994073]
>>> [ 4.994073] -> #3 (crtc_ww_class_mutex){+.+.}:
>>> [ 4.994076] lock_acquire+0x9e/0x170
>>> [ 4.994079] __ww_mutex_lock.constprop.18+0x97/0xf40
>>> [ 4.994080] ww_mutex_lock+0x30/0x90
>>> [ 4.994091] drm_modeset_lock+0x42/0xf0 [drm]
>>> [ 4.994102] drm_modeset_lock_all_ctx+0x1f/0xe0 [drm]
>>> [ 4.994113] drm_modeset_lock_all+0x5e/0x1a0 [drm]
>>> [ 4.994163] intel_modeset_init+0x60b/0xda0 [i915]
>>> ..
>>> [ 4.994253]
>>> [ 4.994253] -> #2 (crtc_ww_class_acquire){+.+.}:
>>> [ 4.994255] lock_acquire+0x9e/0x170
>>> [ 4.994270] drm_modeset_acquire_init+0xcc/0x100 [drm]
>>> [ 4.994280] drm_modeset_lock_all+0x44/0x1a0 [drm]
>>> [ 4.994320] intel_modeset_init+0x60b/0xda0 [i915]
>>> ..
>>> [ 4.994403]
>>> [ 4.994403] -> #1 (&dev->mode_config.mutex){+.+.}:
>>> [ 4.994405] lock_acquire+0x9e/0x170
>>> [ 4.994408] __mutex_lock+0x62/0x8c0
>>> [ 4.994413] drm_setup_crtcs+0x17c/0xc50 [drm_kms_helper]
>>> [ 4.994418] __drm_fb_helper_initial_config_and_unlock+0x34/0x530 [drm_kms_helper]
>>> [ 4.994450] radeon_fbdev_init+0x110/0x130 [radeon]
>>> ..
>>> [ 4.994535]
>>> [ 4.994535] -> #0 (&client->modeset_mutex){+.+.}:
>>> [ 4.994537] __lock_acquire+0xa85/0xe90
>>> [ 4.994538] lock_acquire+0x9e/0x170
>>> [ 4.994540] __mutex_lock+0x62/0x8c0
>>> [ 4.994545] drm_fb_helper_pan_display+0x103/0x1f0 [drm_kms_helper]
>>> [ 4.994547] fb_pan_display+0x92/0x120
>>> [ 4.994549] bit_update_start+0x1a/0x40
>>> [ 4.994550] fbcon_switch+0x392/0x580
>>> [ 4.994552] redraw_screen+0x12c/0x220
>>> [ 4.994553] do_bind_con_driver.cold.30+0xe1/0x10d
>>> [ 4.994554] do_take_over_console+0x113/0x190
>>> [ 4.994555] do_fbcon_takeover+0x58/0xb0
>>> [ 4.994557] notifier_call_chain+0x47/0x70
>>> [ 4.994558] blocking_notifier_call_chain+0x44/0x60
>>> [ 4.994559] register_framebuffer+0x231/0x310
>>> [ 4.994564] __drm_fb_helper_initial_config_and_unlock+0x2fd/0x530 [drm_kms_helper]
>>> [ 4.994590] radeon_fbdev_init+0x110/0x130 [radeon]
>>> ..
>>>
>>> This problem was introduced in
>>>
>>> d81294afe drm/fb-helper: Remove drm_fb_helper_crtc
>>>
>>> Reversing the lock ordering in pan_display_legacy() fixes the issue. The fix
>>> was suggested by Daniel Vetter.
>>>
>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>> Fixes: d81294afeecdacc8d84804ba0bcb3d39e64d0f27
>>
>> I think for ocd consistency it be nice to pull the lock out from both
>> pan_display_atomic and pan_disaply_legacy and move it into
>> drm_fb_helper_pan_display. Like we do drm_fb_helper_dpms or
>> drm_fb_helper_setcmap or restore_fbdev_mode_force.
>
> Is 'lock' referring to modeset_mutex? If so it can't be moved out
> because pan_display_atomic() calls drm_client_modeset_commit_force()
> which in turn takes the modeset_mutex lock.
>
> The locking in _pan_display isn't so nice looking, but I figured that no
> other client would need to do panning so I kept the ugliness in
> drm_fb_helper instead of adding complexity to drm_client.
>
> Thanks for fixing this Thomas.
> Do you have commit rights or should I apply this?
I do, but wait a second. After going through Daniel's reply, I made a
patch that moves the panning code to the DRM client. I'll post it in a bit.
Best regards
Thomas
>
> Acked-by: Noralf Trønnes <noralf@tronnes.org>
>
> Noralf.
>
>>
>> Either way Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>
>>> ---
>>> drivers/gpu/drm/drm_fb_helper.c | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>>> index 7b388674a456..d6991f07cb17 100644
>>> --- a/drivers/gpu/drm/drm_fb_helper.c
>>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>>> @@ -1586,8 +1586,8 @@ static int pan_display_legacy(struct fb_var_screeninfo *var,
>>> struct drm_mode_set *modeset;
>>> int ret = 0;
>>>
>>> - drm_modeset_lock_all(fb_helper->dev);
>>> mutex_lock(&client->modeset_mutex);
>>> + drm_modeset_lock_all(fb_helper->dev);
>>> drm_client_for_each_modeset(modeset, client) {
>>> modeset->x = var->xoffset;
>>> modeset->y = var->yoffset;
>>> @@ -1600,8 +1600,8 @@ static int pan_display_legacy(struct fb_var_screeninfo *var,
>>> }
>>> }
>>> }
>>> - mutex_unlock(&client->modeset_mutex);
>>> drm_modeset_unlock_all(fb_helper->dev);
>>> + mutex_unlock(&client->modeset_mutex);
>>>
>>> return ret;
>>> }
>>> --
>>> 2.21.0
>>>
>>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)
[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-06-11 13:52 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-11 11:57 [PATCH] drm: Reverse lock order in pan_display_legacy() Thomas Zimmermann
2019-06-11 12:37 ` Daniel Vetter
2019-06-11 13:29 ` Noralf Trønnes
2019-06-11 13:52 ` Thomas Zimmermann
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.