Re: [PATCH v13 3/4] drm/atomic-helper: Re-order bridge chain pre-enable and post-disable

[Tomi Valkeinen]

Hi,

On 18/06/2025 13:01, Marek Szyprowski wrote:
> On 18.06.2025 10:27, Maxime Ripard wrote:
>> On Wed, Jun 18, 2025 at 08:30:39AM +0200, Marek Szyprowski wrote:
>>> On 16.06.2025 17:40, Tomi Valkeinen wrote:
>>>> On 12/06/2025 09:31, Marek Szyprowski wrote:
>>>>> On 12.06.2025 07:49, Tomi Valkeinen wrote:
>>>>>> On 11/06/2025 13:45, Marek Szyprowski wrote:
>>>>>>> On 05.06.2025 19:15, Aradhya Bhatia wrote:
>>>>>>>> From: Aradhya Bhatia <a-bhatia1@xxxxxx>
>>>>>>>>
>>>>>>>> Move the bridge pre_enable call before crtc enable, and the bridge
>>>>>>>> post_disable call after the crtc disable.
>>>>>>>>
>>>>>>>> The sequence of enable after this patch will look like:
>>>>>>>>
>>>>>>>> 	bridge[n]_pre_enable
>>>>>>>> 	...
>>>>>>>> 	bridge[1]_pre_enable
>>>>>>>>
>>>>>>>> 	crtc_enable
>>>>>>>> 	encoder_enable
>>>>>>>>
>>>>>>>> 	bridge[1]_enable
>>>>>>>> 	...
>>>>>>>> 	bridge[n]_enable
>>>>>>>>
>>>>>>>> And, the disable sequence for the display pipeline will look like:
>>>>>>>>
>>>>>>>> 	bridge[n]_disable
>>>>>>>> 	...
>>>>>>>> 	bridge[1]_disable
>>>>>>>>
>>>>>>>> 	encoder_disable
>>>>>>>> 	crtc_disable
>>>>>>>>
>>>>>>>> 	bridge[1]_post_disable
>>>>>>>> 	...
>>>>>>>> 	bridge[n]_post_disable
>>>>>>>>
>>>>>>>> The definition of bridge pre_enable hook says that,
>>>>>>>> "The display pipe (i.e. clocks and timing signals) feeding this bridge
>>>>>>>> will not yet be running when this callback is called".
>>>>>>>>
>>>>>>>> Since CRTC is also a source feeding the bridge, it should not be enabled
>>>>>>>> before the bridges in the pipeline are pre_enabled. Fix that by
>>>>>>>> re-ordering the sequence of bridge pre_enable and bridge post_disable.
>>>>>>>>
>>>>>>>> While at it, update the drm bridge API documentation as well.
>>>>>>>>
>>>>>>>> Acked-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
>>>>>>>> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>
>>>>>>>> Reviewed-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
>>>>>>>> Tested-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>
>>>>>>>> Tested-by: Alexander Sverdlin <alexander.sverdlin@xxxxxxxxxxx>
>>>>>>>> Signed-off-by: Aradhya Bhatia <a-bhatia1@xxxxxx>
>>>>>>>> Signed-off-by: Aradhya Bhatia <aradhya.bhatia@xxxxxxxxx>
>>>>>>> This patch landed in today's linux-next as commit c9b1150a68d9
>>>>>>> ("drm/atomic-helper: Re-order bridge chain pre-enable and
>>>>>>> post-disable"). In my tests I found that it breaks booting of Samsung
>>>>>>> Exynos 5420/5800 based Chromebooks (Peach-Pit and Peach-Pi). Both of
>>>>>>> them use Exynos DRM with Exynos_DP sub-driver (Analogix DP) and EDP
>>>>>>> panel. Booting stops at '[drm] Initialized exynos 1.1.0 for exynos-drm
>>>>>>> on minor 0' message. On the other hand, the Samsung Exynos5250 based
>>>>>>> Snow Chromebook boots fine, but it uses dp-lvds nxp,ptn3460 bridge and
>>>>>>> lvds panel instead of edp panels. This looks like some sort of deadlock,
>>>>>>> because if I disable FBDEV emulation, those boards boots fine and I'm
>>>>>>> able to run modetest and enable the display. Also the DRM kernel logger
>>>>>>> seems to be working fine, although I didn't check the screen output yet,
>>>>>>> as I only have a remote access to those boards. I will investigate it
>>>>>>> further and let You know.
>>>>>> Thanks for the report. I was trying to understand the pipeline, but I'm
>>>>>> a bit confused. Above you say Peach-Pit uses DP and EDP panel, but if I
>>>>>> look at arch/arm/boot/dts/samsung/exynos5420-peach-pit.dts, it connects
>>>>>> a dp->lvds bridge (parade,ps8625). Peach-Pi seems to connect to an eDP
>>>>>> panel.
>>>>>>
>>>>>> Is the above correct? Do both Peach-Pi and Peach-Pit fail?
>>>>> Yes, sorry, my fault. I much have checked the same (peach-pi) dts 2
>>>>> times. Both Peach-Pi and Peach-Pit fails, while Snow works fine. All
>>>>> three use the same Exynos DP (based on analogix dp) driver. I will try
>>>>> to play a bit more with those boards in the afternoon, hopefully getting
>>>>> some more hints where the issue is.
>>>> Did you get a chance to test this more? Any hints what happens will help =)
>>> I've spent some time debugging this issue, but so far I only got
>>> something I don't really understand. This issue is somehow related with
>>> the DP clock enabling and disabling, what is being done from
>>> exynos_dp_poweron() and exynos_dp_poweroff() functions, which are called
>>> from analogix_dp_resume() and analogix_dp_suspend(). The lockup happens
>>> somewhere while registering the fbdev console, with console lock held,
>>> what makes debugging much harder.
>> You can skip the locking part with the fb.lockless_register_fb=1 kernel
>> parameter. It's of course not meant for anything but debugging, but it's
>> useful :)
> 
> I've finally found where is the problem! It turned out that the issue is 
> in the exynos_drm_fimd driver, in the code for enabling the display port 
> clock (fimd_dp_clock_enable()
>   function). It touches FIMD regs, but it was not guarded with FIMD's 
> runtime pm calls and after the $subject change it happened that 
> exynos_dp/analogix_dp code called the clock enable/disable when FIMD 
> driver (which implements the CRTC object) was not runtime resumed. 
> Previously all dp clock handling was done when CRTC was enabled, thus 
> the FIMD device was in the resumed runtime pm state.
> 
> I will send a patch fixing this soon.

Alright, sounds like we don't need to revert the re-ordering patch after
all =). Thanks for debugging this.

 Tomi