Message-ID: <357d8e74-eb6c-48d0-b9cc-eab4ed26904d@ideasonboard.com>
Date: Wed, 18 Jun 2025 13:05:35 +0300
From: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
To: Marek Szyprowski <m.szyprowski@...sung.com>,
 Maxime Ripard <mripard@...nel.org>
Cc: 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

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

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

 Tomi

Powered by blists - more mailing lists