| Message ID | 20170720110450.7435-1-Liviu.Dudau@arm.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Jul 20, 2017 at 4:40 PM, Liviu Dudau <Liviu.Dudau@arm.com> wrote: > On Thu, Jul 20, 2017 at 03:24:13PM +0100, Russell King - ARM Linux wrote: >> On Thu, Jul 20, 2017 at 03:19:10PM +0100, Liviu Dudau wrote: >> > On Thu, Jul 20, 2017 at 02:08:29PM +0100, Russell King - ARM Linux wrote: >> > > On Thu, Jul 20, 2017 at 01:54:04PM +0100, Liviu Dudau wrote: >> > > > On Thu, Jul 20, 2017 at 12:44:49PM +0100, Russell King - ARM Linux wrote: >> > > > > Actually, scrub that idea - drm_helper_probe_single_connector_modes() >> > > > > calls drm_edid_to_eld() for these cases anyway, so we must call >> > > > > drm_helper_probe_single_connector_modes() with the audio_mutex held. >> > > > >> > > > OK, so the lockdep warning is spurious? >> > > >> > > I don't think so. I think there's two ways to solve this: >> > > >> > > 1. replace the audio_mutex in tda998x_audio_get_eld() and >> > > tda998x_connector_fill_modes() with a new mutex (eld_mutex) to >> > > protect just the ELD. >> > > >> > > 2. remove the mutex from these two functions, and take the connection_mutex >> > > modeset lock in tda998x_audio_get_eld(). >> > > >> > > However, I don't have a view on which would be best. >> > >> > If you don't mind, I took the liberty of picking option 2, just because >> > I don't like adding new locks when existing ones might do the job. >> >> I don't mind - but one question for the DRM people in connection with >> your patch is whether we need the acquire context for this relatively >> simple lock/copy/unlock sequence. This path for getting the ELD >> shouldn't be holding any other DRM locks. > > Cc-ing Daniel Vetter in hope of clarifications / nod of approval. > However, I can only see my emails in the online dri-devel archive, not > yours, so I can't point him to the whole discussion. > > danvet: a while ago while I was debugging the delayed fb setup I found > a lockdep warning with the tda998x driver. Now I've had some more time > to investigate so I have created a patch trying to fix the issue, which > was on v1 just a re-ordering of places where tda998x's audio_mutex lock > was taken. Russell suggested a different approach, which I have > implemented in [1], but we wonder if we really have to go through the > whole dance. If all you do is take only one ww mutex (wrapped up in drm_modeset_lock for kms) then you can pass a NULL acquire context. The context is only needed when you want to take multiple locks at the same time (to be able to resolve deadlocks). Taking a single lock within the modeset lock class can't deadlock. Reading the kerneldoc that's not explained at all :-( Can you pls type a patch to improve the docs for drm_modeset_lock? Thanks, Daniel
On Thu, Jul 20, 2017 at 04:57:12PM +0200, Daniel Vetter wrote: > On Thu, Jul 20, 2017 at 4:40 PM, Liviu Dudau <Liviu.Dudau@arm.com> wrote: > > On Thu, Jul 20, 2017 at 03:24:13PM +0100, Russell King - ARM Linux wrote: > >> On Thu, Jul 20, 2017 at 03:19:10PM +0100, Liviu Dudau wrote: > >> > On Thu, Jul 20, 2017 at 02:08:29PM +0100, Russell King - ARM Linux wrote: > >> > > On Thu, Jul 20, 2017 at 01:54:04PM +0100, Liviu Dudau wrote: > >> > > > On Thu, Jul 20, 2017 at 12:44:49PM +0100, Russell King - ARM Linux wrote: > >> > > > > Actually, scrub that idea - drm_helper_probe_single_connector_modes() > >> > > > > calls drm_edid_to_eld() for these cases anyway, so we must call > >> > > > > drm_helper_probe_single_connector_modes() with the audio_mutex held. > >> > > > > >> > > > OK, so the lockdep warning is spurious? > >> > > > >> > > I don't think so. I think there's two ways to solve this: > >> > > > >> > > 1. replace the audio_mutex in tda998x_audio_get_eld() and > >> > > tda998x_connector_fill_modes() with a new mutex (eld_mutex) to > >> > > protect just the ELD. > >> > > > >> > > 2. remove the mutex from these two functions, and take the connection_mutex > >> > > modeset lock in tda998x_audio_get_eld(). > >> > > > >> > > However, I don't have a view on which would be best. > >> > > >> > If you don't mind, I took the liberty of picking option 2, just because > >> > I don't like adding new locks when existing ones might do the job. > >> > >> I don't mind - but one question for the DRM people in connection with > >> your patch is whether we need the acquire context for this relatively > >> simple lock/copy/unlock sequence. This path for getting the ELD > >> shouldn't be holding any other DRM locks. > > > > Cc-ing Daniel Vetter in hope of clarifications / nod of approval. > > However, I can only see my emails in the online dri-devel archive, not > > yours, so I can't point him to the whole discussion. > > > > danvet: a while ago while I was debugging the delayed fb setup I found > > a lockdep warning with the tda998x driver. Now I've had some more time > > to investigate so I have created a patch trying to fix the issue, which > > was on v1 just a re-ordering of places where tda998x's audio_mutex lock > > was taken. Russell suggested a different approach, which I have > > implemented in [1], but we wonder if we really have to go through the > > whole dance. > > If all you do is take only one ww mutex (wrapped up in > drm_modeset_lock for kms) then you can pass a NULL acquire context. > The context is only needed when you want to take multiple locks at the > same time (to be able to resolve deadlocks). Taking a single lock > within the modeset lock class can't deadlock. Hi Daniel, Thanks for clarification. I'll post a v3 with a NULL acquire context. Best regards, Liviu > > Reading the kerneldoc that's not explained at all :-( Can you pls type > a patch to improve the docs for drm_modeset_lock? > > Thanks, Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index d1e7ac540199..006ffeb50f34 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -983,9 +983,9 @@ static int tda998x_connector_fill_modes(struct drm_connector *connector, struct tda998x_priv *priv = conn_to_tda998x_priv(connector); int ret; - mutex_lock(&priv->audio_mutex); ret = drm_helper_probe_single_connector_modes(connector, maxX, maxY); + mutex_lock(&priv->audio_mutex); if (connector->edid_blob_ptr) { struct edid *edid = (void *)connector->edid_blob_ptr->data;
When enabling lockdep debugging on Juno platform with HDLCD and TDA998x I get the following warning from the system: [ 25.990733] ====================================================== [ 25.998637] WARNING: possible circular locking dependency detected [ 26.006531] 4.13.0-rc1-00284-g28c0a682ecbf-dirty #17 Not tainted [ 26.014246] ------------------------------------------------------ [ 26.022142] kworker/1:2/140 is trying to acquire lock: [ 26.029001] (&priv->audio_mutex){+.+.+.}, at: [<ffff000000d0319c>] tda998x_encoder_mode_set+0x12c/0x5a0 [tda998x] [ 26.041100] [ 26.041100] but task is already holding lock: [ 26.050436] (crtc_ww_class_mutex){+.+.+.}, at: [<ffff000000eaefe4>] drm_modeset_lock+0x64/0xf8 [drm] [ 26.061531] [ 26.061531] which lock already depends on the new lock. [ 26.061531] [ 26.075063] [ 26.075063] the existing dependency chain (in reverse order) is: [ 26.086031] [ 26.086031] -> #2 (crtc_ww_class_mutex){+.+.+.}: [ 26.095657] __lock_acquire+0x18a0/0x19b8 [ 26.101918] lock_acquire+0xd0/0x2b0 [ 26.107731] __ww_mutex_lock.constprop.3+0x90/0xe78 [ 26.114817] ww_mutex_lock+0x54/0xe0 [ 26.120672] drm_modeset_lock+0x64/0xf8 [drm] [ 26.127253] drm_helper_probe_single_connector_modes+0x7c/0x6b8 [drm_kms_helper] [ 26.136829] tda998x_connector_fill_modes+0x44/0xa8 [tda998x] [ 26.144797] drm_setup_crtcs+0x19c/0xba0 [drm_kms_helper] [ 26.152429] drm_fb_helper_initial_config+0x70/0x440 [drm_kms_helper] [ 26.161097] drm_fbdev_cma_init_with_funcs+0x94/0x168 [drm_kms_helper] [ 26.169857] drm_fbdev_cma_init+0x38/0x50 [drm_kms_helper] [ 26.177559] hdlcd_drm_bind+0x1f8/0x4a8 [hdlcd] [ 26.184310] try_to_bring_up_master+0x180/0x1e0 [ 26.191043] component_master_add_with_match+0xb0/0x108 [ 26.198458] hdlcd_probe+0x58/0x80 [hdlcd] [ 26.204735] platform_drv_probe+0x60/0xc0 [ 26.210913] driver_probe_device+0x23c/0x2e8 [ 26.217350] __driver_attach+0xd4/0xd8 [ 26.223256] bus_for_each_dev+0x5c/0xa8 [ 26.229232] driver_attach+0x30/0x40 [ 26.234917] bus_add_driver+0x1d8/0x248 [ 26.240831] driver_register+0x6c/0x118 [ 26.246715] __platform_driver_register+0x54/0x60 [ 26.253461] 0xffff000000e1b018 [ 26.258644] do_one_initcall+0x44/0x138 [ 26.264503] do_init_module+0x64/0x1d4 [ 26.270238] load_module+0x1f90/0x2590 [ 26.275957] SyS_finit_module+0xb0/0xc8 [ 26.281765] __sys_trace_return+0x0/0x4 [ 26.281767] [ 26.281767] -> #1 (crtc_ww_class_acquire){+.+.+.}: [ 26.281778] __lock_acquire+0x18a0/0x19b8 [ 26.281782] lock_acquire+0xd0/0x2b0 [ 26.281877] drm_modeset_acquire_init+0xa8/0xe0 [drm] [ 26.281921] drm_helper_probe_single_connector_modes+0x48/0x6b8 [drm_kms_helper] [ 26.281929] tda998x_connector_fill_modes+0x44/0xa8 [tda998x] [ 26.281970] drm_setup_crtcs+0x19c/0xba0 [drm_kms_helper] [ 26.282009] drm_fb_helper_initial_config+0x70/0x440 [drm_kms_helper] [ 26.282049] drm_fbdev_cma_init_with_funcs+0x94/0x168 [drm_kms_helper] [ 26.282088] drm_fbdev_cma_init+0x38/0x50 [drm_kms_helper] [ 26.282095] hdlcd_drm_bind+0x1f8/0x4a8 [hdlcd] [ 26.282099] try_to_bring_up_master+0x180/0x1e0 [ 26.282104] component_master_add_with_match+0xb0/0x108 [ 26.282110] hdlcd_probe+0x58/0x80 [hdlcd] [ 26.282114] platform_drv_probe+0x60/0xc0 [ 26.282117] driver_probe_device+0x23c/0x2e8 [ 26.282121] __driver_attach+0xd4/0xd8 [ 26.282124] bus_for_each_dev+0x5c/0xa8 [ 26.282127] driver_attach+0x30/0x40 [ 26.282130] bus_add_driver+0x1d8/0x248 [ 26.282134] driver_register+0x6c/0x118 [ 26.282138] __platform_driver_register+0x54/0x60 [ 26.282141] 0xffff000000e1b018 [ 26.282145] do_one_initcall+0x44/0x138 [ 26.282149] do_init_module+0x64/0x1d4 [ 26.282152] load_module+0x1f90/0x2590 [ 26.282156] SyS_finit_module+0xb0/0xc8 [ 26.282159] __sys_trace_return+0x0/0x4 [ 26.282161] [ 26.282161] -> #0 (&priv->audio_mutex){+.+.+.}: [ 26.282172] print_circular_bug+0x80/0x2e0 [ 26.282176] __lock_acquire+0x15a8/0x19b8 [ 26.282180] lock_acquire+0xd0/0x2b0 [ 26.282184] __mutex_lock+0x78/0x8e0 [ 26.282188] mutex_lock_nested+0x3c/0x50 [ 26.282196] tda998x_encoder_mode_set+0x12c/0x5a0 [tda998x] [ 26.282237] drm_atomic_helper_commit_modeset_disables+0x328/0x3a0 [drm_kms_helper] [ 26.282251] malidp_atomic_commit_tail+0x44/0x6b0 [mali_dp] [ 26.282292] commit_tail+0x4c/0x80 [drm_kms_helper] [ 26.282333] drm_atomic_helper_commit+0xe8/0x180 [drm_kms_helper] [ 26.282427] drm_atomic_commit+0x54/0x70 [drm] [ 26.282467] restore_fbdev_mode_atomic+0x1f0/0x220 [drm_kms_helper] [ 26.282507] restore_fbdev_mode+0x38/0x188 [drm_kms_helper] [ 26.282547] drm_fb_helper_restore_fbdev_mode_unlocked+0x44/0xd0 [drm_kms_helper] [ 26.282586] drm_fb_helper_set_par+0x34/0x80 [drm_kms_helper] [ 26.282625] drm_fb_helper_hotplug_event.part.19+0x94/0xb0 [drm_kms_helper] [ 26.282665] drm_fb_helper_hotplug_event+0x2c/0x48 [drm_kms_helper] [ 26.282704] drm_fbdev_cma_hotplug_event+0x24/0x30 [drm_kms_helper] [ 26.282716] malidp_output_poll_changed+0x24/0x30 [mali_dp] [ 26.282757] drm_kms_helper_hotplug_event+0x34/0x40 [drm_kms_helper] [ 26.282797] output_poll_execute+0x1a0/0x1f0 [drm_kms_helper] [ 26.282803] process_one_work+0x280/0x790 [ 26.282808] worker_thread+0x48/0x450 [ 26.282812] kthread+0x138/0x140 [ 26.282815] ret_from_fork+0x10/0x40 [ 26.282817] [ 26.282817] other info that might help us debug this: [ 26.282817] [ 26.282819] Chain exists of: [ 26.282819] &priv->audio_mutex --> crtc_ww_class_acquire --> crtc_ww_class_mutex [ 26.282819] [ 26.282830] Possible unsafe locking scenario: [ 26.282830] [ 26.282832] CPU0 CPU1 [ 26.282834] ---- ---- [ 26.282835] lock(crtc_ww_class_mutex); [ 26.282840] lock(crtc_ww_class_acquire); [ 26.282845] lock(crtc_ww_class_mutex); [ 26.282850] lock(&priv->audio_mutex); [ 26.282854] [ 26.282854] *** DEADLOCK *** [ 26.282854] [ 26.282858] 5 locks held by kworker/1:2/140: [ 26.282859] #0: ("events"){.+.+.+}, at: [<ffff0000080f8500>] process_one_work+0x1d8/0x790 [ 26.282871] #1: ((&(&dev->mode_config.output_poll_work)->work)){+.+.+.}, at: [<ffff0000080f8500>] process_one_work+0x1d8/0x790 [ 26.282883] #2: (&helper->lock){+.+.+.}, at: [<ffff000000c0631c>] drm_fb_helper_restore_fbdev_mode_unlocked+0x3c/0xd0 [drm_kms_helper] [ 26.282929] #3: (crtc_ww_class_acquire){+.+.+.}, at: [<ffff000000c02d80>] restore_fbdev_mode_atomic+0x38/0x220 [drm_kms_helper] [ 26.282976] #4: (crtc_ww_class_mutex){+.+.+.}, at: [<ffff000000eaefe4>] drm_modeset_lock+0x64/0xf8 [drm] [ 26.283077] [ 26.283077] stack backtrace: [ 26.283082] CPU: 1 PID: 140 Comm: kworker/1:2 Not tainted 4.13.0-rc1-00284-g28c0a682ecbf-dirty #17 [ 26.283084] Hardware name: ARM Juno development board (r0) (DT) [ 26.283127] Workqueue: events output_poll_execute [drm_kms_helper] [ 26.283131] Call trace: [ 26.283137] [<ffff00000808a778>] dump_backtrace+0x0/0x268 [ 26.283142] [<ffff00000808aabc>] show_stack+0x24/0x30 [ 26.283146] [<ffff000008aa36a8>] dump_stack+0xbc/0xf4 [ 26.283151] [<ffff00000812f454>] print_circular_bug+0x1d4/0x2e0 [ 26.283155] [<ffff000008132480>] __lock_acquire+0x15a8/0x19b8 [ 26.283159] [<ffff000008133008>] lock_acquire+0xd0/0x2b0 [ 26.283163] [<ffff000008aba060>] __mutex_lock+0x78/0x8e0 [ 26.283168] [<ffff000008aba904>] mutex_lock_nested+0x3c/0x50 [ 26.283176] [<ffff000000d0319c>] tda998x_encoder_mode_set+0x12c/0x5a0 [tda998x] [ 26.283217] [<ffff000000c00050>] drm_atomic_helper_commit_modeset_disables+0x328/0x3a0 [drm_kms_helper] [ 26.283230] [<ffff000000f1bd0c>] malidp_atomic_commit_tail+0x44/0x6b0 [mali_dp] [ 26.283271] [<ffff000000c0045c>] commit_tail+0x4c/0x80 [drm_kms_helper] [ 26.283312] [<ffff000000c00630>] drm_atomic_helper_commit+0xe8/0x180 [drm_kms_helper] [ 26.283406] [<ffff000000eb1604>] drm_atomic_commit+0x54/0x70 [drm] [ 26.283447] [<ffff000000c02f38>] restore_fbdev_mode_atomic+0x1f0/0x220 [drm_kms_helper] [ 26.283487] [<ffff000000c03cf0>] restore_fbdev_mode+0x38/0x188 [drm_kms_helper] [ 26.283526] [<ffff000000c06324>] drm_fb_helper_restore_fbdev_mode_unlocked+0x44/0xd0 [drm_kms_helper] [ 26.283566] [<ffff000000c0619c>] drm_fb_helper_set_par+0x34/0x80 [drm_kms_helper] [ 26.283606] [<ffff000000c0627c>] drm_fb_helper_hotplug_event.part.19+0x94/0xb0 [drm_kms_helper] [ 26.283645] [<ffff000000c062c4>] drm_fb_helper_hotplug_event+0x2c/0x48 [drm_kms_helper] [ 26.283685] [<ffff000000c07124>] drm_fbdev_cma_hotplug_event+0x24/0x30 [drm_kms_helper] [ 26.283697] [<ffff000000f1b44c>] malidp_output_poll_changed+0x24/0x30 [mali_dp] [ 26.283738] [<ffff000000bf5264>] drm_kms_helper_hotplug_event+0x34/0x40 [drm_kms_helper] [ 26.283779] [<ffff000000bf5480>] output_poll_execute+0x1a0/0x1f0 [drm_kms_helper] [ 26.283784] [<ffff0000080f85a8>] process_one_work+0x280/0x790 [ 26.283788] [<ffff0000080f8b00>] worker_thread+0x48/0x450 [ 26.283792] [<ffff000008100430>] kthread+0x138/0x140 [ 26.283796] [<ffff000008083710>] ret_from_fork+0x10/0x40 Fix the warning by moving the acquiring of the priv->audio_mutex in tda998x_connector_fill_modes() after the drm_helper_probe_single_connector_modes(). Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com> Cc: Russell King <linux@armlinux.org.uk> Fixes: 02efac0fbfd5 ("drm/i2c: tda998x: remove complexity from tda998x_audio_get_eld()") --- drivers/gpu/drm/i2c/tda998x_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)