| Message ID | 1982691.ZDuzTFTUUe@diego (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Heiko, Thanks a lot for great debugging. On 12/08/2015 11:33 PM, Heiko Stübner wrote: > Hi Yakir, > > Am Montag, 7. Dezember 2015, 14:37:19 schrieb Yakir Yang: >> The Samsung Exynos eDP controller and Rockchip RK3288 eDP controller >> share the same IP, so a lot of parts can be re-used. I split the common >> code into bridge directory, then rk3288 and exynos only need to keep >> some platform code. Cause I can't find the exact IP name of exynos dp >> controller, so I decide to name dp core driver with "analogix" which I >> find in rk3288 eDP TRM > [...] > >> Changes in v10: >> - Correct the ROCKCHIP_ANALOGIX_DP indentation in Kconfig to tabs here >> (Heiko) - Fix the wrong macro value of >> GRF_EDP_REF_CLK_SEL_INTER_HIWORD_MASK BIT(4) -> BIT(20) >> - Remove the surplus "plat_data" check. (Heiko) >> - switch (dp->plat_data && dp->plat_data->dev_type) { >> + switch (dp->plat_data->dev_type) { >> - Revert parts of Gustavo Padovan's changes in commit: >> drm/exynos: do not start enabling DP at bind() phase >> Add dp phy poweron function in bind time. > The hotplug issue is still present, but I think I found the cause. When > the first detect call happens, the display simply is still off. I just did > some very basic tracing [0] and it seems the display simply is not enabled > when it is supposed to get detected. Aha, thanks, make a lot of sense. > And it seems injecting a drm_panel_prepare early for _testing_ [1] really > did make the hotplug work on both my jerry and minnie. > > So I guess we should somehow make sure the panel is actually powered when > detection is running. Although I'm not sure yet, how that should look like. Agree, panel should be powered up before DP controller start to detect hotplug signal. > Intuition suggests, making drm_panel calls nestable (similar to > clk_prepare/unprepare, etc) and simply wrapping the detection code > in a prepare-unprepare calls, but I'm not sure if Thierry might have other > ideas ;-) Due to the panel power status would influence the hotplug status, so I think we don't need to unprepared the panel unless in driver enter into suspend time. Things I want: 1. Prepared the panel in driver *bind time* 2. Enable the panel in driver *bridge->enable time* 3. Disable the panel in driver *bridge->disable time* 4. Unprepared the panel in driver*suspend time * 5. Re-prepared the panel in driver *resume time* > Also my "log" below suggests some sort of mismatch between > prepare/unprepare calls, as there are a lot more of the prepare-side. Yes, it's a typo too. I shouldn't place the panel->prepare in connector->get_modes, cause userspace would try to call get_modes once it receive the hotplug event, so there wouldn't have a match between panel prepare/unprepare. Previously, I just want to ensure that panel should be power-up when driver try to read the EDID from panel, so for now must remove the prepare from get_modes time :) > And the locking issue also seems to be still there [2]. Hmm, I haven't meet this dead lock on my chromebook (ChromeOS + 4.4-rc3 Kernel) After look at the dead lock trace, I guess this dead lock would happened when hotplug event happened in bridge->disable time, not sure. Would try to find more in trace log and try to reproduce this. - Yakir > > Heiko > > > [0] > [ 2.797383] analogix_dp_reset > [ 2.800709] analogix_dp_init_hpd > [ 2.803960] analogix_dp_init_video > [ 2.807653] rockchip-drm display-subsystem: bound ff970000.dp (ops rockchip_dp_component_ops) > [ 2.817176] [drm] Supports vblank timestamp caching Rev 2 (21.10.2013). > [ 2.823799] [drm] No driver support for vblank timestamp query. > [ 2.829947] analogix_dp_detect > [ 2.833015] analogix_dp_get_plug_in_status: hpd status 0 > ... > [ 2.893425] analogix_dp_get_plug_in_status: hpd status 0 > [ 2.893456] rockchip-dp ff970000.dp: failed to get hpd plug status, try to force hpd > [ 2.893458] analogix_dp_force_hpd > [ 2.893464] analogix_dp_get_plug_in_status: hpd status 112 > [ 2.893470] panel_simple_prepare > [ 2.952183] rockchip-dp ff970000.dp: EDID data does not include any extensions. > [ 2.961727] panel_simple_get_modes > [ 3.432154] analogix_dp_detect > [ 3.432158] analogix_dp_get_plug_in_status: hpd status 120 > [ 3.432160] panel_simple_prepare > [ 3.433731] rockchip-dp ff970000.dp: EDID data does not include any extensions. > [ 3.443268] panel_simple_get_modes > [ 3.444668] panel_simple_prepare > [ 3.444755] analogix_dp_reset > [ 3.445078] analogix_dp_init_hpd > [ 3.445096] panel_simple_disable > [ 3.455349] analogix_dp_init_video > [ 3.558323] rockchip-dp ff970000.dp: Timeout of video streamclk ok > [ 3.558326] rockchip-dp ff970000.dp: unable to config video > [ 3.558328] panel_simple_enable > [ 3.573915] analogix_dp_detect > [ 3.573919] analogix_dp_get_plug_in_status: hpd status 72 > [ 3.573921] panel_simple_prepare > > > [1] > diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c > index 3990951..0c2dca5 100644 > --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c > +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c > @@ -399,6 +399,8 @@ static int rockchip_dp_probe(struct platform_device *pdev) > > dp->plat_data.panel = panel; > > +drm_panel_prepare(dp->plat_data.panel); > + > /* > * We just use the drvdata until driver run into component > * add function, and then we would set drvdata to null, so > > > [2] > [ 11.971277] panel_simple_get_modes > [ OK ] Started LSB: X display manager for KDE. > [ OK ] Started LSB: Speech Dispatcher. > [ 12.007120] panel_simple_disable > [ 12.012323] > [ 12.013820] ====================================================== > [ 12.019993] [ INFO: possible circular locking dependency detected ] > [ 12.026250] 4.4.0-rc3+ #2755 Not tainted > [ 12.030165] ------------------------------------------------------- > [ 12.036417] Xorg/793 is trying to acquire lock: > [ 12.040855] ((&dp->hotplug_work)){+.+...}[ 12.040870] > [ 12.040870] but task is already holding lock: > [ 12.040871] (crtc_ww_class_mutex){+.+.+.}, at: [<c0383600>] drm_modeset_lock+0x84/0x104 > [ 12.040881] > [ 12.040881] which lock already depends on the new lock. > [ 12.040881] > [ 12.040882] > [ 12.040882] the existing dependency chain (in reverse order) is: > [ 12.040883] > [ 12.040883] -> #2 (crtc_ww_class_mutex){+.+.+.}: > [ 12.040887] [<c0756374>] mutex_lock_nested+0x78/0x41c > [ 12.040893] [<c038365c>] drm_modeset_lock+0xe0/0x104 > [ 12.040896] [<c0377888>] drm_mode_getconnector+0x168/0x38c > [ 12.040902] [<c036ae58>] drm_ioctl+0x274/0x408 > [ 12.040907] [<c017cbc8>] do_vfs_ioctl+0x670/0x750 > [ 12.040911] [<c017cd04>] SyS_ioctl+0x5c/0x84 > [ 12.040914] [<c000ff80>] ret_fast_syscall+0x0/0x1c > [ 12.040920] > [ 12.040920] -> #1 (&dev->mode_config.mutex){+.+.+.}: > [ 12.040924] [<c0756374>] mutex_lock_nested+0x78/0x41c > [ 12.040928] [<c035b3b4>] drm_helper_hpd_irq_event+0x40/0x150 > [ 12.040934] [<c038e0b0>] analogix_dp_hotplug+0x24/0x28 > [ 12.040938] [<c00450c4>] process_one_work+0x328/0x668 > [ 12.040942] [<c0046334>] worker_thread+0x2cc/0x41c > [ 12.040945] [<c004bbe8>] kthread+0xf4/0x10c > [ 12.040950] [<c0010010>] ret_from_fork+0x14/0x24 > [ 12.040954] > [ 12.040954] -> #0 ((&dp->hotplug_work)){+.+...}: > [ 12.040958] [<c007f4ac>] lock_acquire+0x178/0x218 > [ 12.040963] [<c00437d0>] flush_work+0x4c/0x22c > [ 12.040966] [<c038e45c>] analogix_dp_bridge_disable+0x74/0xf0 > [ 12.040970] [<c0385ef4>] drm_bridge_disable+0x34/0x38 > [ 12.040973] [<c0359044>] drm_crtc_helper_set_mode+0x200/0x424 > [ 12.040977] [<c0359a38>] drm_crtc_helper_set_config+0x6c0/0x988 > [ 12.040981] [<c0373ba8>] drm_mode_set_config_internal+0x60/0xdc > [ 12.040984] [<c0378938>] drm_mode_setcrtc+0x3cc/0x474 > [ 12.040988] [<c036ae58>] drm_ioctl+0x274/0x408 > [ 12.040991] [<c017cbc8>] do_vfs_ioctl+0x670/0x750 > [ 12.040994] [<c017cd04>] SyS_ioctl+0x5c/0x84 > [ 12.040997] [<c000ff80>] ret_fast_syscall+0x0/0x1c > [ 12.041001] > [ 12.041001] other info that might help us debug this: > [ 12.041001] > [ 12.041002] Chain exists of: > [ 12.041002] (&dp->hotplug_work) --> &dev->mode_config.mutex --> crtc_ww_class_mutex > [ 12.041007] > [ 12.041008] Possible unsafe locking scenario: > [ 12.041008] > [ 12.041009] CPU0 CPU1 > [ 12.041010] ---- ---- > [ 12.041011] lock(crtc_ww_class_mutex); > [ 12.041013] lock(&dev->mode_config.mutex); > [ 12.041016] lock(crtc_ww_class_mutex); > [ 12.041018] lock((&dp->hotplug_work)); > [ 12.041020] > [ 12.041020] *** DEADLOCK *** > [ 12.041020] > [ 12.041023] 3 locks held by Xorg/793: > [ 12.041023] #0: (&dev->mode_config.mutex){+.+.+.}, at: [<c0383d48>] drm_modeset_lock_all+0x50/0xd8 > [ 12.041030] #1: (crtc_ww_class_acquire){+.+.+.}, at: [<c0383d58>] drm_modeset_lock_all+0x60/0xd8 > [ 12.041037] #2: (crtc_ww_class_mutex){+.+.+.}, at: [<c0383600>] drm_modeset_lock+0x84/0x104 > [ 12.041044] > [ 12.041044] stack backtrace: > [ 12.041048] CPU: 3 PID: 793 Comm: Xorg Not tainted 4.4.0-rc3+ #2755 > [ 12.041049] Hardware name: Rockchip (Device Tree) > [ 12.041059] [<c0019914>] (unwind_backtrace) from [<c0014bcc>] (show_stack+0x20/0x24) > [ 12.041066] [<c0014bcc>] (show_stack) from [<c02c4484>] (dump_stack+0x84/0xb8) > [ 12.041072] [<c02c4484>] (dump_stack) from [<c007a2b8>] (print_circular_bug+0x278/0x2cc) > [ 12.041078] [<c007a2b8>] (print_circular_bug) from [<c007e850>] (__lock_acquire+0x14a0/0x1aec) > [ 12.041082] [<c007e850>] (__lock_acquire) from [<c007f4ac>] (lock_acquire+0x178/0x218) > [ 12.041086] [<c007f4ac>] (lock_acquire) from [<c00437d0>] (flush_work+0x4c/0x22c) > [ 12.041091] [<c00437d0>] (flush_work) from [<c038e45c>] (analogix_dp_bridge_disable+0x74/0xf0) > [ 12.041097] [<c038e45c>] (analogix_dp_bridge_disable) from [<c0385ef4>] (drm_bridge_disable+0x34/0x38) > [ 12.041102] [<c0385ef4>] (drm_bridge_disable) from [<c0359044>] (drm_crtc_helper_set_mode+0x200/0x424) > [ 12.041108] [<c0359044>] (drm_crtc_helper_set_mode) from [<c0359a38>] (drm_crtc_helper_set_config+0x6c0/0x988) > [ 12.041114] [<c0359a38>] (drm_crtc_helper_set_config) from [<c0373ba8>] (drm_mode_set_config_internal+0x60/0xdc) > [ 12.041119] [<c0373ba8>] (drm_mode_set_config_internal) from [<c0378938>] (drm_mode_setcrtc+0x3cc/0x474) > [ 12.041124] [<c0378938>] (drm_mode_setcrtc) from [<c036ae58>] (drm_ioctl+0x274/0x408) > [ 12.041129] [<c036ae58>] (drm_ioctl) from [<c017cbc8>] (do_vfs_ioctl+0x670/0x750) > [ 12.041134] [<c017cbc8>] (do_vfs_ioctl) from [<c017cd04>] (SyS_ioctl+0x5c/0x84) > [ 12.041138] [<c017cd04>] (SyS_ioctl) from [<c000ff80>] (ret_fast_syscall+0x0/0x1c) > [ 12.041210] panel_simple_unprepare > [ 12.041359] panel_simple_prepare > [ 12.101638] analogix_dp_reset > [ 12.101879] analogix_dp_init_hpd > [ 12.101897] panel_simple_disable > [ 12.108543] analogix_dp_irq_handler: irq-type 0 > [ 12.110452] rockchip-dp ff970000.dp: Link Training Clock Recovery success > [ 12.111900] rockchip-dp ff970000.dp: Link Training success! > [ 12.113384] analogix_dp_init_video > [ 12.215825] rockchip-dp ff970000.dp: Timeout of video streamclk ok > [ 12.215829] rockchip-dp ff970000.dp: unable to config video > [ 12.215832] panel_simple_enable > ,at: [<c0043784>] flush_work+0x0/0x22c > > > >
Hi Yakir, Am Mittwoch, 9. Dezember 2015, 11:49:10 schrieb Yakir Yang: > Thanks a lot for great debugging. > > On 12/08/2015 11:33 PM, Heiko Stübner wrote: > > Hi Yakir, > > > > Am Montag, 7. Dezember 2015, 14:37:19 schrieb Yakir Yang: > >> The Samsung Exynos eDP controller and Rockchip RK3288 eDP controller > >> > >> share the same IP, so a lot of parts can be re-used. I split the common > >> code into bridge directory, then rk3288 and exynos only need to keep > >> some platform code. Cause I can't find the exact IP name of exynos dp > >> controller, so I decide to name dp core driver with "analogix" which I > >> find in rk3288 eDP TRM > > > > [...] > > > >> Changes in v10: > >> - Correct the ROCKCHIP_ANALOGIX_DP indentation in Kconfig to tabs here > >> (Heiko) - Fix the wrong macro value of > >> GRF_EDP_REF_CLK_SEL_INTER_HIWORD_MASK BIT(4) -> BIT(20) > >> - Remove the surplus "plat_data" check. (Heiko) > >> - switch (dp->plat_data && dp->plat_data->dev_type) { > >> + switch (dp->plat_data->dev_type) { > >> > >> - Revert parts of Gustavo Padovan's changes in commit: > >> drm/exynos: do not start enabling DP at bind() phase > >> > >> Add dp phy poweron function in bind time. > > > > The hotplug issue is still present, but I think I found the cause. When > > the first detect call happens, the display simply is still off. I just did > > some very basic tracing [0] and it seems the display simply is not enabled > > when it is supposed to get detected. > > Aha, thanks, make a lot of sense. > > > And it seems injecting a drm_panel_prepare early for _testing_ [1] really > > did make the hotplug work on both my jerry and minnie. > > > > So I guess we should somehow make sure the panel is actually powered when > > detection is running. Although I'm not sure yet, how that should look > > like. > > Agree, panel should be powered up before DP controller start to detect > hotplug signal. > > > Intuition suggests, making drm_panel calls nestable (similar to > > clk_prepare/unprepare, etc) and simply wrapping the detection code > > in a prepare-unprepare calls, but I'm not sure if Thierry might have other > > ideas ;-) > > Due to the panel power status would influence the hotplug status, so I > think we don't > need to unprepared the panel unless in driver enter into suspend time. > Things I want: > > 1. Prepared the panel in driver *bind time* > 2. Enable the panel in driver *bridge->enable time* > 3. Disable the panel in driver *bridge->disable time* > 4. Unprepared the panel in driver*suspend time * > 5. Re-prepared the panel in driver *resume time* 6. Unprepare the panel in driver at *unbind time* otherwise going that way looks nice. > > Also my "log" below suggests some sort of mismatch between > > prepare/unprepare calls, as there are a lot more of the prepare-side. > > Yes, it's a typo too. I shouldn't place the panel->prepare in > connector->get_modes, > cause userspace would try to call get_modes once it receive the hotplug > event, so > there wouldn't have a match between panel prepare/unprepare. > > Previously, I just want to ensure that panel should be power-up when > driver try to > read the EDID from panel, so for now must remove the prepare from > get_modes time :) > > > And the locking issue also seems to be still there [2]. > > Hmm, I haven't meet this dead lock on my chromebook (ChromeOS + 4.4-rc3 > Kernel) > > After look at the dead lock trace, I guess this dead lock would happened > when hotplug > event happened in bridge->disable time, not sure. Would try to find > more in trace log > and try to reproduce this. It is not an actual deadlock, but a warning that a deadlock might happen. So you need to have LOCKDEP on. My kernels are currently running with CONFIG_DEBUG_LOCK_ALLOC=y CONFIG_PROVE_LOCKING=y CONFIG_LOCKDEP=y CONFIG_DEBUG_LOCKDEP=y CONFIG_DEBUG_ATOMIC_SLEEP=y and I see this mostly when changing between X11, console and back to X11. Heiko > > Heiko > > > > > > [0] > > [ 2.797383] analogix_dp_reset > > [ 2.800709] analogix_dp_init_hpd > > [ 2.803960] analogix_dp_init_video > > [ 2.807653] rockchip-drm display-subsystem: bound ff970000.dp (ops > > rockchip_dp_component_ops) [ 2.817176] [drm] Supports vblank timestamp > > caching Rev 2 (21.10.2013). [ 2.823799] [drm] No driver support for > > vblank timestamp query. [ 2.829947] analogix_dp_detect > > [ 2.833015] analogix_dp_get_plug_in_status: hpd status 0 > > ... > > [ 2.893425] analogix_dp_get_plug_in_status: hpd status 0 > > [ 2.893456] rockchip-dp ff970000.dp: failed to get hpd plug status, try > > to force hpd [ 2.893458] analogix_dp_force_hpd > > [ 2.893464] analogix_dp_get_plug_in_status: hpd status 112 > > [ 2.893470] panel_simple_prepare > > [ 2.952183] rockchip-dp ff970000.dp: EDID data does not include any > > extensions. [ 2.961727] panel_simple_get_modes > > [ 3.432154] analogix_dp_detect > > [ 3.432158] analogix_dp_get_plug_in_status: hpd status 120 > > [ 3.432160] panel_simple_prepare > > [ 3.433731] rockchip-dp ff970000.dp: EDID data does not include any > > extensions. [ 3.443268] panel_simple_get_modes > > [ 3.444668] panel_simple_prepare > > [ 3.444755] analogix_dp_reset > > [ 3.445078] analogix_dp_init_hpd > > [ 3.445096] panel_simple_disable > > [ 3.455349] analogix_dp_init_video > > [ 3.558323] rockchip-dp ff970000.dp: Timeout of video streamclk ok > > [ 3.558326] rockchip-dp ff970000.dp: unable to config video > > [ 3.558328] panel_simple_enable > > [ 3.573915] analogix_dp_detect > > [ 3.573919] analogix_dp_get_plug_in_status: hpd status 72 > > [ 3.573921] panel_simple_prepare > > > > > > [1] > > diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c > > b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c index 3990951..0c2dca5 > > 100644 > > --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c > > +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c > > @@ -399,6 +399,8 @@ static int rockchip_dp_probe(struct platform_device > > *pdev)> > > dp->plat_data.panel = panel; > > > > +drm_panel_prepare(dp->plat_data.panel); > > + > > > > /* > > > > * We just use the drvdata until driver run into component > > * add function, and then we would set drvdata to null, so > > > > [2] > > [ 11.971277] panel_simple_get_modes > > [ OK ] Started LSB: X display manager for KDE. > > [ OK ] Started LSB: Speech Dispatcher. > > [ 12.007120] panel_simple_disable > > [ 12.012323] > > [ 12.013820] ====================================================== > > [ 12.019993] [ INFO: possible circular locking dependency detected ] > > [ 12.026250] 4.4.0-rc3+ #2755 Not tainted > > [ 12.030165] ------------------------------------------------------- > > [ 12.036417] Xorg/793 is trying to acquire lock: > > [ 12.040855] ((&dp->hotplug_work)){+.+...}[ 12.040870] > > [ 12.040870] but task is already holding lock: > > [ 12.040871] (crtc_ww_class_mutex){+.+.+.}, at: [<c0383600>] > > drm_modeset_lock+0x84/0x104 [ 12.040881] > > [ 12.040881] which lock already depends on the new lock. > > [ 12.040881] > > [ 12.040882] > > [ 12.040882] the existing dependency chain (in reverse order) is: > > [ 12.040883] > > [ 12.040883] -> #2 (crtc_ww_class_mutex){+.+.+.}: > > [ 12.040887] [<c0756374>] mutex_lock_nested+0x78/0x41c > > [ 12.040893] [<c038365c>] drm_modeset_lock+0xe0/0x104 > > [ 12.040896] [<c0377888>] drm_mode_getconnector+0x168/0x38c > > [ 12.040902] [<c036ae58>] drm_ioctl+0x274/0x408 > > [ 12.040907] [<c017cbc8>] do_vfs_ioctl+0x670/0x750 > > [ 12.040911] [<c017cd04>] SyS_ioctl+0x5c/0x84 > > [ 12.040914] [<c000ff80>] ret_fast_syscall+0x0/0x1c > > [ 12.040920] > > [ 12.040920] -> #1 (&dev->mode_config.mutex){+.+.+.}: > > [ 12.040924] [<c0756374>] mutex_lock_nested+0x78/0x41c > > [ 12.040928] [<c035b3b4>] drm_helper_hpd_irq_event+0x40/0x150 > > [ 12.040934] [<c038e0b0>] analogix_dp_hotplug+0x24/0x28 > > [ 12.040938] [<c00450c4>] process_one_work+0x328/0x668 > > [ 12.040942] [<c0046334>] worker_thread+0x2cc/0x41c > > [ 12.040945] [<c004bbe8>] kthread+0xf4/0x10c > > [ 12.040950] [<c0010010>] ret_from_fork+0x14/0x24 > > [ 12.040954] > > [ 12.040954] -> #0 ((&dp->hotplug_work)){+.+...}: > > [ 12.040958] [<c007f4ac>] lock_acquire+0x178/0x218 > > [ 12.040963] [<c00437d0>] flush_work+0x4c/0x22c > > [ 12.040966] [<c038e45c>] analogix_dp_bridge_disable+0x74/0xf0 > > [ 12.040970] [<c0385ef4>] drm_bridge_disable+0x34/0x38 > > [ 12.040973] [<c0359044>] drm_crtc_helper_set_mode+0x200/0x424 > > [ 12.040977] [<c0359a38>] drm_crtc_helper_set_config+0x6c0/0x988 > > [ 12.040981] [<c0373ba8>] drm_mode_set_config_internal+0x60/0xdc > > [ 12.040984] [<c0378938>] drm_mode_setcrtc+0x3cc/0x474 > > [ 12.040988] [<c036ae58>] drm_ioctl+0x274/0x408 > > [ 12.040991] [<c017cbc8>] do_vfs_ioctl+0x670/0x750 > > [ 12.040994] [<c017cd04>] SyS_ioctl+0x5c/0x84 > > [ 12.040997] [<c000ff80>] ret_fast_syscall+0x0/0x1c > > [ 12.041001] > > [ 12.041001] other info that might help us debug this: > > [ 12.041001] > > [ 12.041002] Chain exists of: > > [ 12.041002] (&dp->hotplug_work) --> &dev->mode_config.mutex --> > > crtc_ww_class_mutex [ 12.041007] > > [ 12.041008] Possible unsafe locking scenario: > > [ 12.041008] > > [ 12.041009] CPU0 CPU1 > > [ 12.041010] ---- ---- > > [ 12.041011] lock(crtc_ww_class_mutex); > > [ 12.041013] > > lock(&dev->mode_config.mutex); [ 12.041016] > > lock(crtc_ww_class_mutex); [ 12.041018] lock((&dp->hotplug_work)); > > [ 12.041020] > > [ 12.041020] *** DEADLOCK *** > > [ 12.041020] > > [ 12.041023] 3 locks held by Xorg/793: > > [ 12.041023] #0: (&dev->mode_config.mutex){+.+.+.}, at: [<c0383d48>] > > drm_modeset_lock_all+0x50/0xd8 [ 12.041030] #1: > > (crtc_ww_class_acquire){+.+.+.}, at: [<c0383d58>] > > drm_modeset_lock_all+0x60/0xd8 [ 12.041037] #2: > > (crtc_ww_class_mutex){+.+.+.}, at: [<c0383600>] > > drm_modeset_lock+0x84/0x104 [ 12.041044] > > [ 12.041044] stack backtrace: > > [ 12.041048] CPU: 3 PID: 793 Comm: Xorg Not tainted 4.4.0-rc3+ #2755 > > [ 12.041049] Hardware name: Rockchip (Device Tree) > > [ 12.041059] [<c0019914>] (unwind_backtrace) from [<c0014bcc>] > > (show_stack+0x20/0x24) [ 12.041066] [<c0014bcc>] (show_stack) from > > [<c02c4484>] (dump_stack+0x84/0xb8) [ 12.041072] [<c02c4484>] > > (dump_stack) from [<c007a2b8>] (print_circular_bug+0x278/0x2cc) [ > > 12.041078] [<c007a2b8>] (print_circular_bug) from [<c007e850>] > > (__lock_acquire+0x14a0/0x1aec) [ 12.041082] [<c007e850>] > > (__lock_acquire) from [<c007f4ac>] (lock_acquire+0x178/0x218) [ > > 12.041086] [<c007f4ac>] (lock_acquire) from [<c00437d0>] > > (flush_work+0x4c/0x22c) [ 12.041091] [<c00437d0>] (flush_work) from > > [<c038e45c>] (analogix_dp_bridge_disable+0x74/0xf0) [ 12.041097] > > [<c038e45c>] (analogix_dp_bridge_disable) from [<c0385ef4>] > > (drm_bridge_disable+0x34/0x38) [ 12.041102] [<c0385ef4>] > > (drm_bridge_disable) from [<c0359044>] > > (drm_crtc_helper_set_mode+0x200/0x424) [ 12.041108] [<c0359044>] > > (drm_crtc_helper_set_mode) from [<c0359a38>] > > (drm_crtc_helper_set_config+0x6c0/0x988) [ 12.041114] [<c0359a38>] > > (drm_crtc_helper_set_config) from [<c0373ba8>] > > (drm_mode_set_config_internal+0x60/0xdc) [ 12.041119] [<c0373ba8>] > > (drm_mode_set_config_internal) from [<c0378938>] > > (drm_mode_setcrtc+0x3cc/0x474) [ 12.041124] [<c0378938>] > > (drm_mode_setcrtc) from [<c036ae58>] (drm_ioctl+0x274/0x408) [ > > 12.041129] [<c036ae58>] (drm_ioctl) from [<c017cbc8>] > > (do_vfs_ioctl+0x670/0x750) [ 12.041134] [<c017cbc8>] (do_vfs_ioctl) > > from [<c017cd04>] (SyS_ioctl+0x5c/0x84) [ 12.041138] [<c017cd04>] > > (SyS_ioctl) from [<c000ff80>] (ret_fast_syscall+0x0/0x1c) [ 12.041210] > > panel_simple_unprepare > > [ 12.041359] panel_simple_prepare > > [ 12.101638] analogix_dp_reset > > [ 12.101879] analogix_dp_init_hpd > > [ 12.101897] panel_simple_disable > > [ 12.108543] analogix_dp_irq_handler: irq-type 0 > > [ 12.110452] rockchip-dp ff970000.dp: Link Training Clock Recovery > > success [ 12.111900] rockchip-dp ff970000.dp: Link Training success! > > [ 12.113384] analogix_dp_init_video > > [ 12.215825] rockchip-dp ff970000.dp: Timeout of video streamclk ok > > [ 12.215829] rockchip-dp ff970000.dp: unable to config video > > [ 12.215832] panel_simple_enable > > ,at: [<c0043784>] flush_work+0x0/0x22c
Hi Heiko, On 12/09/2015 10:51 PM, Heiko Stübner wrote: > Hi Yakir, > > Am Mittwoch, 9. Dezember 2015, 11:49:10 schrieb Yakir Yang: >> Thanks a lot for great debugging. >> >> On 12/08/2015 11:33 PM, Heiko Stübner wrote: >>> Hi Yakir, >>> >>> Am Montag, 7. Dezember 2015, 14:37:19 schrieb Yakir Yang: >>>> The Samsung Exynos eDP controller and Rockchip RK3288 eDP controller >>>> >>>> share the same IP, so a lot of parts can be re-used. I split the common >>>> code into bridge directory, then rk3288 and exynos only need to keep >>>> some platform code. Cause I can't find the exact IP name of exynos dp >>>> controller, so I decide to name dp core driver with "analogix" which I >>>> find in rk3288 eDP TRM >>> [...] >>> >>>> Changes in v10: >>>> - Correct the ROCKCHIP_ANALOGIX_DP indentation in Kconfig to tabs here >>>> (Heiko) - Fix the wrong macro value of >>>> GRF_EDP_REF_CLK_SEL_INTER_HIWORD_MASK BIT(4) -> BIT(20) >>>> - Remove the surplus "plat_data" check. (Heiko) >>>> - switch (dp->plat_data && dp->plat_data->dev_type) { >>>> + switch (dp->plat_data->dev_type) { >>>> >>>> - Revert parts of Gustavo Padovan's changes in commit: >>>> drm/exynos: do not start enabling DP at bind() phase >>>> >>>> Add dp phy poweron function in bind time. >>> The hotplug issue is still present, but I think I found the cause. When >>> the first detect call happens, the display simply is still off. I just did >>> some very basic tracing [0] and it seems the display simply is not enabled >>> when it is supposed to get detected. >> Aha, thanks, make a lot of sense. >> >>> And it seems injecting a drm_panel_prepare early for _testing_ [1] really >>> did make the hotplug work on both my jerry and minnie. >>> >>> So I guess we should somehow make sure the panel is actually powered when >>> detection is running. Although I'm not sure yet, how that should look >>> like. >> Agree, panel should be powered up before DP controller start to detect >> hotplug signal. >> >>> Intuition suggests, making drm_panel calls nestable (similar to >>> clk_prepare/unprepare, etc) and simply wrapping the detection code >>> in a prepare-unprepare calls, but I'm not sure if Thierry might have other >>> ideas ;-) >> Due to the panel power status would influence the hotplug status, so I >> think we don't >> need to unprepared the panel unless in driver enter into suspend time. >> Things I want: >> >> 1. Prepared the panel in driver *bind time* >> 2. Enable the panel in driver *bridge->enable time* >> 3. Disable the panel in driver *bridge->disable time* >> 4. Unprepared the panel in driver*suspend time * >> 5. Re-prepared the panel in driver *resume time* > 6. Unprepare the panel in driver at *unbind time* > > otherwise going that way looks nice. Yes, >>> Also my "log" below suggests some sort of mismatch between >>> prepare/unprepare calls, as there are a lot more of the prepare-side. >> Yes, it's a typo too. I shouldn't place the panel->prepare in >> connector->get_modes, >> cause userspace would try to call get_modes once it receive the hotplug >> event, so >> there wouldn't have a match between panel prepare/unprepare. >> >> Previously, I just want to ensure that panel should be power-up when >> driver try to >> read the EDID from panel, so for now must remove the prepare from >> get_modes time :) >> >>> And the locking issue also seems to be still there [2]. >> Hmm, I haven't meet this dead lock on my chromebook (ChromeOS + 4.4-rc3 >> Kernel) >> >> After look at the dead lock trace, I guess this dead lock would happened >> when hotplug >> event happened in bridge->disable time, not sure. Would try to find >> more in trace log >> and try to reproduce this. > It is not an actual deadlock, but a warning that a deadlock might happen. > So you need to have LOCKDEP on. My kernels are currently running with > > CONFIG_DEBUG_LOCK_ALLOC=y > CONFIG_PROVE_LOCKING=y > CONFIG_LOCKDEP=y > CONFIG_DEBUG_LOCKDEP=y > CONFIG_DEBUG_ATOMIC_SLEEP=y > > and I see this mostly when changing between X11, console and back to X11. > Oops, I have reproduced this issue. I didn't fully understand the potential dead lock, but i guess this is relate the an situation that when hotplug event happened in bridge->disable time. *The normal flow would like:* IN --> DRM IOCTL 1. Acquire crtc_ww_class_mutex (DRM IOCTL) IN --> analogix_dp_bridge 2. Acquire hpd work lock (Flush hpd work) 3. HPD work already in idle, no need to call drm_helper_hpd_irq_event() OUT <-- analogix_dp_bridge OUT <-- DRM IOCTL *The dead lock flow would like:* IN --> DRM IOCTL 1. Acquire crtc_ww_class_mutex (DRM IOCTL) IN --> analogix_dp_bridge 2. Acquire hpd work lock (Flush hpd work) IN --> analogix_dp_hotplug IN --> drm_helper_hpd_irq_event 3. Acquire mode_config lock (This lock already have been acquire in previous step 1) ** Dead Lock Now ** Hmm... In other words, dead lock would happened if we call drm_helper_hpd_irq_event in bridge->disbale time. It's wrong to flush the hpd work in bridge->disable time, I guess the original code just want to ensure the delay work must be finish before encoder disabled. For now i think it's better to delete the delay work way to update HPD event, we can take the fast interrupt thread way to update DRM HPD event (like dw_hdmi interrupt code). Would send the fix patch soon. - Yakir > Heiko > >>> Heiko >>> >>> >>> [0] >>> [ 2.797383] analogix_dp_reset >>> [ 2.800709] analogix_dp_init_hpd >>> [ 2.803960] analogix_dp_init_video >>> [ 2.807653] rockchip-drm display-subsystem: bound ff970000.dp (ops >>> rockchip_dp_component_ops) [ 2.817176] [drm] Supports vblank timestamp >>> caching Rev 2 (21.10.2013). [ 2.823799] [drm] No driver support for >>> vblank timestamp query. [ 2.829947] analogix_dp_detect >>> [ 2.833015] analogix_dp_get_plug_in_status: hpd status 0 >>> ... >>> [ 2.893425] analogix_dp_get_plug_in_status: hpd status 0 >>> [ 2.893456] rockchip-dp ff970000.dp: failed to get hpd plug status, try >>> to force hpd [ 2.893458] analogix_dp_force_hpd >>> [ 2.893464] analogix_dp_get_plug_in_status: hpd status 112 >>> [ 2.893470] panel_simple_prepare >>> [ 2.952183] rockchip-dp ff970000.dp: EDID data does not include any >>> extensions. [ 2.961727] panel_simple_get_modes >>> [ 3.432154] analogix_dp_detect >>> [ 3.432158] analogix_dp_get_plug_in_status: hpd status 120 >>> [ 3.432160] panel_simple_prepare >>> [ 3.433731] rockchip-dp ff970000.dp: EDID data does not include any >>> extensions. [ 3.443268] panel_simple_get_modes >>> [ 3.444668] panel_simple_prepare >>> [ 3.444755] analogix_dp_reset >>> [ 3.445078] analogix_dp_init_hpd >>> [ 3.445096] panel_simple_disable >>> [ 3.455349] analogix_dp_init_video >>> [ 3.558323] rockchip-dp ff970000.dp: Timeout of video streamclk ok >>> [ 3.558326] rockchip-dp ff970000.dp: unable to config video >>> [ 3.558328] panel_simple_enable >>> [ 3.573915] analogix_dp_detect >>> [ 3.573919] analogix_dp_get_plug_in_status: hpd status 72 >>> [ 3.573921] panel_simple_prepare >>> >>> >>> [1] >>> diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c >>> b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c index 3990951..0c2dca5 >>> 100644 >>> --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c >>> +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c >>> @@ -399,6 +399,8 @@ static int rockchip_dp_probe(struct platform_device >>> *pdev)> >>> dp->plat_data.panel = panel; >>> >>> +drm_panel_prepare(dp->plat_data.panel); >>> + >>> >>> /* >>> >>> * We just use the drvdata until driver run into component >>> * add function, and then we would set drvdata to null, so >>> >>> [2] >>> [ 11.971277] panel_simple_get_modes >>> [ OK ] Started LSB: X display manager for KDE. >>> [ OK ] Started LSB: Speech Dispatcher. >>> [ 12.007120] panel_simple_disable >>> [ 12.012323] >>> [ 12.013820] ====================================================== >>> [ 12.019993] [ INFO: possible circular locking dependency detected ] >>> [ 12.026250] 4.4.0-rc3+ #2755 Not tainted >>> [ 12.030165] ------------------------------------------------------- >>> [ 12.036417] Xorg/793 is trying to acquire lock: >>> [ 12.040855] ((&dp->hotplug_work)){+.+...}[ 12.040870] >>> [ 12.040870] but task is already holding lock: >>> [ 12.040871] (crtc_ww_class_mutex){+.+.+.}, at: [<c0383600>] >>> drm_modeset_lock+0x84/0x104 [ 12.040881] >>> [ 12.040881] which lock already depends on the new lock. >>> [ 12.040881] >>> [ 12.040882] >>> [ 12.040882] the existing dependency chain (in reverse order) is: >>> [ 12.040883] >>> [ 12.040883] -> #2 (crtc_ww_class_mutex){+.+.+.}: >>> [ 12.040887] [<c0756374>] mutex_lock_nested+0x78/0x41c >>> [ 12.040893] [<c038365c>] drm_modeset_lock+0xe0/0x104 >>> [ 12.040896] [<c0377888>] drm_mode_getconnector+0x168/0x38c >>> [ 12.040902] [<c036ae58>] drm_ioctl+0x274/0x408 >>> [ 12.040907] [<c017cbc8>] do_vfs_ioctl+0x670/0x750 >>> [ 12.040911] [<c017cd04>] SyS_ioctl+0x5c/0x84 >>> [ 12.040914] [<c000ff80>] ret_fast_syscall+0x0/0x1c >>> [ 12.040920] >>> [ 12.040920] -> #1 (&dev->mode_config.mutex){+.+.+.}: >>> [ 12.040924] [<c0756374>] mutex_lock_nested+0x78/0x41c >>> [ 12.040928] [<c035b3b4>] drm_helper_hpd_irq_event+0x40/0x150 >>> [ 12.040934] [<c038e0b0>] analogix_dp_hotplug+0x24/0x28 >>> [ 12.040938] [<c00450c4>] process_one_work+0x328/0x668 >>> [ 12.040942] [<c0046334>] worker_thread+0x2cc/0x41c >>> [ 12.040945] [<c004bbe8>] kthread+0xf4/0x10c >>> [ 12.040950] [<c0010010>] ret_from_fork+0x14/0x24 >>> [ 12.040954] >>> [ 12.040954] -> #0 ((&dp->hotplug_work)){+.+...}: >>> [ 12.040958] [<c007f4ac>] lock_acquire+0x178/0x218 >>> [ 12.040963] [<c00437d0>] flush_work+0x4c/0x22c >>> [ 12.040966] [<c038e45c>] analogix_dp_bridge_disable+0x74/0xf0 >>> [ 12.040970] [<c0385ef4>] drm_bridge_disable+0x34/0x38 >>> [ 12.040973] [<c0359044>] drm_crtc_helper_set_mode+0x200/0x424 >>> [ 12.040977] [<c0359a38>] drm_crtc_helper_set_config+0x6c0/0x988 >>> [ 12.040981] [<c0373ba8>] drm_mode_set_config_internal+0x60/0xdc >>> [ 12.040984] [<c0378938>] drm_mode_setcrtc+0x3cc/0x474 >>> [ 12.040988] [<c036ae58>] drm_ioctl+0x274/0x408 >>> [ 12.040991] [<c017cbc8>] do_vfs_ioctl+0x670/0x750 >>> [ 12.040994] [<c017cd04>] SyS_ioctl+0x5c/0x84 >>> [ 12.040997] [<c000ff80>] ret_fast_syscall+0x0/0x1c >>> [ 12.041001] >>> [ 12.041001] other info that might help us debug this: >>> [ 12.041001] >>> [ 12.041002] Chain exists of: >>> [ 12.041002] (&dp->hotplug_work) --> &dev->mode_config.mutex --> >>> crtc_ww_class_mutex [ 12.041007] >>> [ 12.041008] Possible unsafe locking scenario: >>> [ 12.041008] >>> [ 12.041009] CPU0 CPU1 >>> [ 12.041010] ---- ---- >>> [ 12.041011] lock(crtc_ww_class_mutex); >>> [ 12.041013] >>> lock(&dev->mode_config.mutex); [ 12.041016] >>> lock(crtc_ww_class_mutex); [ 12.041018] lock((&dp->hotplug_work)); >>> [ 12.041020] >>> [ 12.041020] *** DEADLOCK *** >>> [ 12.041020] >>> [ 12.041023] 3 locks held by Xorg/793: >>> [ 12.041023] #0: (&dev->mode_config.mutex){+.+.+.}, at: [<c0383d48>] >>> drm_modeset_lock_all+0x50/0xd8 [ 12.041030] #1: >>> (crtc_ww_class_acquire){+.+.+.}, at: [<c0383d58>] >>> drm_modeset_lock_all+0x60/0xd8 [ 12.041037] #2: >>> (crtc_ww_class_mutex){+.+.+.}, at: [<c0383600>] >>> drm_modeset_lock+0x84/0x104 [ 12.041044] >>> [ 12.041044] stack backtrace: >>> [ 12.041048] CPU: 3 PID: 793 Comm: Xorg Not tainted 4.4.0-rc3+ #2755 >>> [ 12.041049] Hardware name: Rockchip (Device Tree) >>> [ 12.041059] [<c0019914>] (unwind_backtrace) from [<c0014bcc>] >>> (show_stack+0x20/0x24) [ 12.041066] [<c0014bcc>] (show_stack) from >>> [<c02c4484>] (dump_stack+0x84/0xb8) [ 12.041072] [<c02c4484>] >>> (dump_stack) from [<c007a2b8>] (print_circular_bug+0x278/0x2cc) [ >>> 12.041078] [<c007a2b8>] (print_circular_bug) from [<c007e850>] >>> (__lock_acquire+0x14a0/0x1aec) [ 12.041082] [<c007e850>] >>> (__lock_acquire) from [<c007f4ac>] (lock_acquire+0x178/0x218) [ >>> 12.041086] [<c007f4ac>] (lock_acquire) from [<c00437d0>] >>> (flush_work+0x4c/0x22c) [ 12.041091] [<c00437d0>] (flush_work) from >>> [<c038e45c>] (analogix_dp_bridge_disable+0x74/0xf0) [ 12.041097] >>> [<c038e45c>] (analogix_dp_bridge_disable) from [<c0385ef4>] >>> (drm_bridge_disable+0x34/0x38) [ 12.041102] [<c0385ef4>] >>> (drm_bridge_disable) from [<c0359044>] >>> (drm_crtc_helper_set_mode+0x200/0x424) [ 12.041108] [<c0359044>] >>> (drm_crtc_helper_set_mode) from [<c0359a38>] >>> (drm_crtc_helper_set_config+0x6c0/0x988) [ 12.041114] [<c0359a38>] >>> (drm_crtc_helper_set_config) from [<c0373ba8>] >>> (drm_mode_set_config_internal+0x60/0xdc) [ 12.041119] [<c0373ba8>] >>> (drm_mode_set_config_internal) from [<c0378938>] >>> (drm_mode_setcrtc+0x3cc/0x474) [ 12.041124] [<c0378938>] >>> (drm_mode_setcrtc) from [<c036ae58>] (drm_ioctl+0x274/0x408) [ >>> 12.041129] [<c036ae58>] (drm_ioctl) from [<c017cbc8>] >>> (do_vfs_ioctl+0x670/0x750) [ 12.041134] [<c017cbc8>] (do_vfs_ioctl) >>> from [<c017cd04>] (SyS_ioctl+0x5c/0x84) [ 12.041138] [<c017cd04>] >>> (SyS_ioctl) from [<c000ff80>] (ret_fast_syscall+0x0/0x1c) [ 12.041210] >>> panel_simple_unprepare >>> [ 12.041359] panel_simple_prepare >>> [ 12.101638] analogix_dp_reset >>> [ 12.101879] analogix_dp_init_hpd >>> [ 12.101897] panel_simple_disable >>> [ 12.108543] analogix_dp_irq_handler: irq-type 0 >>> [ 12.110452] rockchip-dp ff970000.dp: Link Training Clock Recovery >>> success [ 12.111900] rockchip-dp ff970000.dp: Link Training success! >>> [ 12.113384] analogix_dp_init_video >>> [ 12.215825] rockchip-dp ff970000.dp: Timeout of video streamclk ok >>> [ 12.215829] rockchip-dp ff970000.dp: unable to config video >>> [ 12.215832] panel_simple_enable >>> ,at: [<c0043784>] flush_work+0x0/0x22c > > >
diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c index 3990951..0c2dca5 100644 --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c @@ -399,6 +399,8 @@ static int rockchip_dp_probe(struct platform_device *pdev) dp->plat_data.panel = panel; +drm_panel_prepare(dp->plat_data.panel); + /* * We just use the drvdata until driver run into component * add function, and then we would set drvdata to null, so