Re: [PATCH v10 0/17] Add Analogix Core Display Port Driver

[Yakir Yang <ykk@rock-chips.com>]

[-- Attachment #1.1: Type: text/plain, Size: 14578 bytes --]

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
>
>
>


[-- Attachment #1.2: Type: text/html, Size: 16251 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel