| Message ID | 20170720110450.7435-1-Liviu.Dudau@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jul 20, 2017 at 12:04:50PM +0100, Liviu Dudau wrote: > 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(-) > > 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); This breaks the locking strategy. tda998x_audio_get_eld() takes the mutex to safely read the ELD. The ELD is updated in tda998x_connector_get_modes(), which is called within drm_helper_probe_single_connector_modes(). Moving the mutex in this way means that the update of the ELD happens outside the lock, which then means there's no protection between reading and writing the ELD. What could be done instead is to move the locking as you have above, but also move the call to drm_edid_to_eld(connector, edid) into this function, also within the lock. This has the advantage that if the user needs to override the EDID, then the overriden EDID is also used for the ELD. IMHO that's more correct as the reason for overriding the EDID may be to correct the audio information.
On Thu, Jul 20, 2017 at 12:38:53PM +0100, Russell King - ARM Linux wrote: > On Thu, Jul 20, 2017 at 12:04:50PM +0100, Liviu Dudau wrote: > > 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(-) > > > > 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); > > This breaks the locking strategy. > > tda998x_audio_get_eld() takes the mutex to safely read the ELD. The > ELD is updated in tda998x_connector_get_modes(), which is called > within drm_helper_probe_single_connector_modes(). Moving the mutex > in this way means that the update of the ELD happens outside the lock, > which then means there's no protection between reading and writing > the ELD. > > What could be done instead is to move the locking as you have above, > but also move the call to drm_edid_to_eld(connector, edid) into this > function, also within the lock. This has the advantage that if the > user needs to override the EDID, then the overriden EDID is also used > for the ELD. > > IMHO that's more correct as the reason for overriding the EDID may be > to correct the audio information. 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.
On Thu, Jul 20, 2017 at 12:44:49PM +0100, Russell King - ARM Linux wrote: > On Thu, Jul 20, 2017 at 12:38:53PM +0100, Russell King - ARM Linux wrote: > > On Thu, Jul 20, 2017 at 12:04:50PM +0100, Liviu Dudau wrote: > > > 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(-) > > > > > > 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); > > Hi Russell, > > This breaks the locking strategy. TBH I don't claim that I'm familiar with the locking strategy here, I was just parsing the warning's stack dump. > > > > tda998x_audio_get_eld() takes the mutex to safely read the ELD. The > > ELD is updated in tda998x_connector_get_modes(), which is called > > within drm_helper_probe_single_connector_modes(). Moving the mutex > > in this way means that the update of the ELD happens outside the lock, > > which then means there's no protection between reading and writing > > the ELD. First, please note that tda998x_audio_get_eld() is only involved as far as the name of the patch that I claim to fix, it doesn't seem to be present in the call trace. I picked that patch as the likely place where the order of the calls was last introduced. Second, maybe I'm missing something obvious, but drm_edid_to_eld() updates connector->eld without taking any lock, so I don't see how tda998x_audio_get_eld() is prevented from reading an ELD that is being updated at the same time. > > > > What could be done instead is to move the locking as you have above, > > but also move the call to drm_edid_to_eld(connector, edid) into this > > function, also within the lock. This has the advantage that if the > > user needs to override the EDID, then the overriden EDID is also used > > for the ELD. > > > > IMHO that's more correct as the reason for overriding the EDID may be > > to correct the audio information. > > 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? Best regards, Liviu > > -- > RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ > FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up > according to speedtest.net.
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.
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. v2 patch on the way shortly. Best regards, Liviu > > -- > RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ > FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up > according to speedtest.net.
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.
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. Best regards, Liviu [1] https://lists.freedesktop.org/archives/dri-devel/2017-July/147940.html > > -- > RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ > FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up > according to speedtest.net.
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(-)