Message-ID: <3c5d43a2-fdbe-4cbe-bf45-09bfcb18dcb3@samsung.com>
Date: Wed, 18 Jun 2025 12:01:49 +0200
From: Marek Szyprowski <m.szyprowski@...sung.com>
To: Maxime Ripard <mripard@...nel.org>
Cc: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>, Aradhya Bhatia
	<aradhya.bhatia@...ux.dev>, DRI Development List
	<dri-devel@...ts.freedesktop.org>, Linux Kernel List
	<linux-kernel@...r.kernel.org>, Nishanth Menon <nm@...com>, Vignesh
	Raghavendra <vigneshr@...com>, Devarsh Thakkar <devarsht@...com>, Jayesh
	Choudhary <j-choudhary@...com>, Alexander Sverdlin
	<alexander.sverdlin@...mens.com>, Dmitry Baryshkov <lumag@...nel.org>,
	Andrzej Hajda <andrzej.hajda@...el.com>, Neil Armstrong
	<neil.armstrong@...aro.org>, Robert Foss <rfoss@...nel.org>, Laurent
	Pinchart <Laurent.pinchart@...asonboard.com>, Jonas Karlman
	<jonas@...boo.se>, Jernej Skrabec <jernej.skrabec@...il.com>, Maarten
	Lankhorst <maarten.lankhorst@...ux.intel.com>, Thomas Zimmermann
	<tzimmermann@...e.de>, David Airlie <airlied@...il.com>, Simona Vetter
	<simona@...ll.ch>
Subject: Re: [PATCH v13 3/4] drm/atomic-helper: Re-order bridge chain
 pre-enable and post-disable

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@...com>
>>>>>>>
>>>>>>> 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@...aro.org>
>>>>>>> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
>>>>>>> Reviewed-by: Thomas Zimmermann <tzimmermann@...e.de>
>>>>>>> Tested-by: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
>>>>>>> Tested-by: Alexander Sverdlin <alexander.sverdlin@...mens.com>
>>>>>>> Signed-off-by: Aradhya Bhatia <a-bhatia1@...com>
>>>>>>> Signed-off-by: Aradhya Bhatia <aradhya.bhatia@...ux.dev>
>>>>>> 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.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Powered by blists - more mailing lists