Message-ID: <2c51cf39-13cb-413f-8dd5-53bc1c11467a@samsung.com>
Date: Wed, 11 Jun 2025 12:45:10 +0200
From: Marek Szyprowski <m.szyprowski@...sung.com>
To: Aradhya Bhatia <aradhya.bhatia@...ux.dev>, Tomi Valkeinen
	<tomi.valkeinen@...asonboard.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>, Maxime Ripard
	<mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>, David Airlie
	<airlied@...il.com>, Simona Vetter <simona@...ll.ch>
Cc: 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>
Subject: Re: [PATCH v13 3/4] drm/atomic-helper: Re-order bridge chain
 pre-enable and post-disable

Hi,

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.

> ---
>   drivers/gpu/drm/drm_atomic_helper.c |   8 +-
>   include/drm/drm_bridge.h            | 249 ++++++++++++++++++++--------
>   2 files changed, 187 insertions(+), 70 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 539b7f072c72..2fe6c91910a1 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1336,9 +1336,9 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *state)
>   {
>   	encoder_bridge_disable(dev, state);
>   
> -	encoder_bridge_post_disable(dev, state);
> -
>   	crtc_disable(dev, state);
> +
> +	encoder_bridge_post_disable(dev, state);
>   }
>   
>   /**
> @@ -1674,10 +1674,10 @@ encoder_bridge_enable(struct drm_device *dev, struct drm_atomic_state *state)
>   void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
>   					      struct drm_atomic_state *state)
>   {
> -	crtc_enable(dev, state);
> -
>   	encoder_bridge_pre_enable(dev, state);
>   
> +	crtc_enable(dev, state);
> +
>   	encoder_bridge_enable(dev, state);
>   
>   	drm_atomic_helper_commit_writebacks(dev, state);
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 0af5db244db8..ecdeb90e5586 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -165,17 +165,33 @@ struct drm_bridge_funcs {
>   	/**
>   	 * @disable:
>   	 *
> -	 * This callback should disable the bridge. It is called right before
> -	 * the preceding element in the display pipe is disabled. If the
> -	 * preceding element is a bridge this means it's called before that
> -	 * bridge's @disable vfunc. If the preceding element is a &drm_encoder
> -	 * it's called right before the &drm_encoder_helper_funcs.disable,
> -	 * &drm_encoder_helper_funcs.prepare or &drm_encoder_helper_funcs.dpms
> -	 * hook.
> +	 * The @disable callback should disable the bridge.
>   	 *
>   	 * The bridge can assume that the display pipe (i.e. clocks and timing
>   	 * signals) feeding it is still running when this callback is called.
>   	 *
> +	 *
> +	 * If the preceding element is a &drm_bridge, then this is called before
> +	 * that bridge is disabled via one of:
> +	 *
> +	 * - &drm_bridge_funcs.disable
> +	 * - &drm_bridge_funcs.atomic_disable
> +	 *
> +	 * If the preceding element of the bridge is a display controller, then
> +	 * this callback is called before the encoder is disabled via one of:
> +	 *
> +	 * - &drm_encoder_helper_funcs.atomic_disable
> +	 * - &drm_encoder_helper_funcs.prepare
> +	 * - &drm_encoder_helper_funcs.disable
> +	 * - &drm_encoder_helper_funcs.dpms
> +	 *
> +	 * and the CRTC is disabled via one of:
> +	 *
> +	 * - &drm_crtc_helper_funcs.prepare
> +	 * - &drm_crtc_helper_funcs.atomic_disable
> +	 * - &drm_crtc_helper_funcs.disable
> +	 * - &drm_crtc_helper_funcs.dpms.
> +	 *
>   	 * The @disable callback is optional.
>   	 *
>   	 * NOTE:
> @@ -188,17 +204,34 @@ struct drm_bridge_funcs {
>   	/**
>   	 * @post_disable:
>   	 *
> -	 * This callback should disable the bridge. It is called right after the
> -	 * preceding element in the display pipe is disabled. If the preceding
> -	 * element is a bridge this means it's called after that bridge's
> -	 * @post_disable function. If the preceding element is a &drm_encoder
> -	 * it's called right after the encoder's
> -	 * &drm_encoder_helper_funcs.disable, &drm_encoder_helper_funcs.prepare
> -	 * or &drm_encoder_helper_funcs.dpms hook.
> -	 *
>   	 * The bridge must assume that the display pipe (i.e. clocks and timing
> -	 * signals) feeding it is no longer running when this callback is
> -	 * called.
> +	 * signals) feeding this bridge is no longer running when the
> +	 * @post_disable is called.
> +	 *
> +	 * This callback should perform all the actions required by the hardware
> +	 * after it has stopped receiving signals from the preceding element.
> +	 *
> +	 * If the preceding element is a &drm_bridge, then this is called after
> +	 * that bridge is post-disabled (unless marked otherwise by the
> +	 * @pre_enable_prev_first flag) via one of:
> +	 *
> +	 * - &drm_bridge_funcs.post_disable
> +	 * - &drm_bridge_funcs.atomic_post_disable
> +	 *
> +	 * If the preceding element of the bridge is a display controller, then
> +	 * this callback is called after the encoder is disabled via one of:
> +	 *
> +	 * - &drm_encoder_helper_funcs.atomic_disable
> +	 * - &drm_encoder_helper_funcs.prepare
> +	 * - &drm_encoder_helper_funcs.disable
> +	 * - &drm_encoder_helper_funcs.dpms
> +	 *
> +	 * and the CRTC is disabled via one of:
> +	 *
> +	 * - &drm_crtc_helper_funcs.prepare
> +	 * - &drm_crtc_helper_funcs.atomic_disable
> +	 * - &drm_crtc_helper_funcs.disable
> +	 * - &drm_crtc_helper_funcs.dpms
>   	 *
>   	 * The @post_disable callback is optional.
>   	 *
> @@ -241,18 +274,30 @@ struct drm_bridge_funcs {
>   	/**
>   	 * @pre_enable:
>   	 *
> -	 * This callback should enable the bridge. It is called right before
> -	 * the preceding element in the display pipe is enabled. If the
> -	 * preceding element is a bridge this means it's called before that
> -	 * bridge's @pre_enable function. If the preceding element is a
> -	 * &drm_encoder it's called right before the encoder's
> -	 * &drm_encoder_helper_funcs.enable, &drm_encoder_helper_funcs.commit or
> -	 * &drm_encoder_helper_funcs.dpms hook.
> -	 *
>   	 * The display pipe (i.e. clocks and timing signals) feeding this bridge
> -	 * will not yet be running when this callback is called. The bridge must
> -	 * not enable the display link feeding the next bridge in the chain (if
> -	 * there is one) when this callback is called.
> +	 * will not yet be running when the @pre_enable is called.
> +	 *
> +	 * This callback should perform all the necessary actions to prepare the
> +	 * bridge to accept signals from the preceding element.
> +	 *
> +	 * If the preceding element is a &drm_bridge, then this is called before
> +	 * that bridge is pre-enabled (unless marked otherwise by
> +	 * @pre_enable_prev_first flag) via one of:
> +	 *
> +	 * - &drm_bridge_funcs.pre_enable
> +	 * - &drm_bridge_funcs.atomic_pre_enable
> +	 *
> +	 * If the preceding element of the bridge is a display controller, then
> +	 * this callback is called before the CRTC is enabled via one of:
> +	 *
> +	 * - &drm_crtc_helper_funcs.atomic_enable
> +	 * - &drm_crtc_helper_funcs.commit
> +	 *
> +	 * and the encoder is enabled via one of:
> +	 *
> +	 * - &drm_encoder_helper_funcs.atomic_enable
> +	 * - &drm_encoder_helper_funcs.enable
> +	 * - &drm_encoder_helper_funcs.commit
>   	 *
>   	 * The @pre_enable callback is optional.
>   	 *
> @@ -266,19 +311,31 @@ struct drm_bridge_funcs {
>   	/**
>   	 * @enable:
>   	 *
> -	 * This callback should enable the bridge. It is called right after
> -	 * the preceding element in the display pipe is enabled. If the
> -	 * preceding element is a bridge this means it's called after that
> -	 * bridge's @enable function. If the preceding element is a
> -	 * &drm_encoder it's called right after the encoder's
> -	 * &drm_encoder_helper_funcs.enable, &drm_encoder_helper_funcs.commit or
> -	 * &drm_encoder_helper_funcs.dpms hook.
> +	 * The @enable callback should enable the bridge.
>   	 *
>   	 * The bridge can assume that the display pipe (i.e. clocks and timing
>   	 * signals) feeding it is running when this callback is called. This
>   	 * callback must enable the display link feeding the next bridge in the
>   	 * chain if there is one.
>   	 *
> +	 * If the preceding element is a &drm_bridge, then this is called after
> +	 * that bridge is enabled via one of:
> +	 *
> +	 * - &drm_bridge_funcs.enable
> +	 * - &drm_bridge_funcs.atomic_enable
> +	 *
> +	 * If the preceding element of the bridge is a display controller, then
> +	 * this callback is called after the CRTC is enabled via one of:
> +	 *
> +	 * - &drm_crtc_helper_funcs.atomic_enable
> +	 * - &drm_crtc_helper_funcs.commit
> +	 *
> +	 * and the encoder is enabled via one of:
> +	 *
> +	 * - &drm_encoder_helper_funcs.atomic_enable
> +	 * - &drm_encoder_helper_funcs.enable
> +	 * - drm_encoder_helper_funcs.commit
> +	 *
>   	 * The @enable callback is optional.
>   	 *
>   	 * NOTE:
> @@ -291,17 +348,30 @@ struct drm_bridge_funcs {
>   	/**
>   	 * @atomic_pre_enable:
>   	 *
> -	 * This callback should enable the bridge. It is called right before
> -	 * the preceding element in the display pipe is enabled. If the
> -	 * preceding element is a bridge this means it's called before that
> -	 * bridge's @atomic_pre_enable or @pre_enable function. If the preceding
> -	 * element is a &drm_encoder it's called right before the encoder's
> -	 * &drm_encoder_helper_funcs.atomic_enable hook.
> -	 *
>   	 * The display pipe (i.e. clocks and timing signals) feeding this bridge
> -	 * will not yet be running when this callback is called. The bridge must
> -	 * not enable the display link feeding the next bridge in the chain (if
> -	 * there is one) when this callback is called.
> +	 * will not yet be running when the @atomic_pre_enable is called.
> +	 *
> +	 * This callback should perform all the necessary actions to prepare the
> +	 * bridge to accept signals from the preceding element.
> +	 *
> +	 * If the preceding element is a &drm_bridge, then this is called before
> +	 * that bridge is pre-enabled (unless marked otherwise by
> +	 * @pre_enable_prev_first flag) via one of:
> +	 *
> +	 * - &drm_bridge_funcs.pre_enable
> +	 * - &drm_bridge_funcs.atomic_pre_enable
> +	 *
> +	 * If the preceding element of the bridge is a display controller, then
> +	 * this callback is called before the CRTC is enabled via one of:
> +	 *
> +	 * - &drm_crtc_helper_funcs.atomic_enable
> +	 * - &drm_crtc_helper_funcs.commit
> +	 *
> +	 * and the encoder is enabled via one of:
> +	 *
> +	 * - &drm_encoder_helper_funcs.atomic_enable
> +	 * - &drm_encoder_helper_funcs.enable
> +	 * - &drm_encoder_helper_funcs.commit
>   	 *
>   	 * The @atomic_pre_enable callback is optional.
>   	 */
> @@ -311,18 +381,31 @@ struct drm_bridge_funcs {
>   	/**
>   	 * @atomic_enable:
>   	 *
> -	 * This callback should enable the bridge. It is called right after
> -	 * the preceding element in the display pipe is enabled. If the
> -	 * preceding element is a bridge this means it's called after that
> -	 * bridge's @atomic_enable or @enable function. If the preceding element
> -	 * is a &drm_encoder it's called right after the encoder's
> -	 * &drm_encoder_helper_funcs.atomic_enable hook.
> +	 * The @atomic_enable callback should enable the bridge.
>   	 *
>   	 * The bridge can assume that the display pipe (i.e. clocks and timing
>   	 * signals) feeding it is running when this callback is called. This
>   	 * callback must enable the display link feeding the next bridge in the
>   	 * chain if there is one.
>   	 *
> +	 * If the preceding element is a &drm_bridge, then this is called after
> +	 * that bridge is enabled via one of:
> +	 *
> +	 * - &drm_bridge_funcs.enable
> +	 * - &drm_bridge_funcs.atomic_enable
> +	 *
> +	 * If the preceding element of the bridge is a display controller, then
> +	 * this callback is called after the CRTC is enabled via one of:
> +	 *
> +	 * - &drm_crtc_helper_funcs.atomic_enable
> +	 * - &drm_crtc_helper_funcs.commit
> +	 *
> +	 * and the encoder is enabled via one of:
> +	 *
> +	 * - &drm_encoder_helper_funcs.atomic_enable
> +	 * - &drm_encoder_helper_funcs.enable
> +	 * - drm_encoder_helper_funcs.commit
> +	 *
>   	 * The @atomic_enable callback is optional.
>   	 */
>   	void (*atomic_enable)(struct drm_bridge *bridge,
> @@ -330,16 +413,32 @@ struct drm_bridge_funcs {
>   	/**
>   	 * @atomic_disable:
>   	 *
> -	 * This callback should disable the bridge. It is called right before
> -	 * the preceding element in the display pipe is disabled. If the
> -	 * preceding element is a bridge this means it's called before that
> -	 * bridge's @atomic_disable or @disable vfunc. If the preceding element
> -	 * is a &drm_encoder it's called right before the
> -	 * &drm_encoder_helper_funcs.atomic_disable hook.
> +	 * The @atomic_disable callback should disable the bridge.
>   	 *
>   	 * The bridge can assume that the display pipe (i.e. clocks and timing
>   	 * signals) feeding it is still running when this callback is called.
>   	 *
> +	 * If the preceding element is a &drm_bridge, then this is called before
> +	 * that bridge is disabled via one of:
> +	 *
> +	 * - &drm_bridge_funcs.disable
> +	 * - &drm_bridge_funcs.atomic_disable
> +	 *
> +	 * If the preceding element of the bridge is a display controller, then
> +	 * this callback is called before the encoder is disabled via one of:
> +	 *
> +	 * - &drm_encoder_helper_funcs.atomic_disable
> +	 * - &drm_encoder_helper_funcs.prepare
> +	 * - &drm_encoder_helper_funcs.disable
> +	 * - &drm_encoder_helper_funcs.dpms
> +	 *
> +	 * and the CRTC is disabled via one of:
> +	 *
> +	 * - &drm_crtc_helper_funcs.prepare
> +	 * - &drm_crtc_helper_funcs.atomic_disable
> +	 * - &drm_crtc_helper_funcs.disable
> +	 * - &drm_crtc_helper_funcs.dpms.
> +	 *
>   	 * The @atomic_disable callback is optional.
>   	 */
>   	void (*atomic_disable)(struct drm_bridge *bridge,
> @@ -348,16 +447,34 @@ struct drm_bridge_funcs {
>   	/**
>   	 * @atomic_post_disable:
>   	 *
> -	 * This callback should disable the bridge. It is called right after the
> -	 * preceding element in the display pipe is disabled. If the preceding
> -	 * element is a bridge this means it's called after that bridge's
> -	 * @atomic_post_disable or @post_disable function. If the preceding
> -	 * element is a &drm_encoder it's called right after the encoder's
> -	 * &drm_encoder_helper_funcs.atomic_disable hook.
> -	 *
>   	 * The bridge must assume that the display pipe (i.e. clocks and timing
> -	 * signals) feeding it is no longer running when this callback is
> -	 * called.
> +	 * signals) feeding this bridge is no longer running when the
> +	 * @atomic_post_disable is called.
> +	 *
> +	 * This callback should perform all the actions required by the hardware
> +	 * after it has stopped receiving signals from the preceding element.
> +	 *
> +	 * If the preceding element is a &drm_bridge, then this is called after
> +	 * that bridge is post-disabled (unless marked otherwise by the
> +	 * @pre_enable_prev_first flag) via one of:
> +	 *
> +	 * - &drm_bridge_funcs.post_disable
> +	 * - &drm_bridge_funcs.atomic_post_disable
> +	 *
> +	 * If the preceding element of the bridge is a display controller, then
> +	 * this callback is called after the encoder is disabled via one of:
> +	 *
> +	 * - &drm_encoder_helper_funcs.atomic_disable
> +	 * - &drm_encoder_helper_funcs.prepare
> +	 * - &drm_encoder_helper_funcs.disable
> +	 * - &drm_encoder_helper_funcs.dpms
> +	 *
> +	 * and the CRTC is disabled via one of:
> +	 *
> +	 * - &drm_crtc_helper_funcs.prepare
> +	 * - &drm_crtc_helper_funcs.atomic_disable
> +	 * - &drm_crtc_helper_funcs.disable
> +	 * - &drm_crtc_helper_funcs.dpms
>   	 *
>   	 * The @atomic_post_disable callback is optional.
>   	 */

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

Powered by blists - more mailing lists