From: Aradhya Bhatia <a-bhatia1@ti.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@linaro.org>
Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
Tested-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Tested-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
Signed-off-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>
---
 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.
 	 */
-- 
2.34.1

On Thu, Jun 5, 2025 at 7:15 PM Aradhya Bhatia <aradhya.bhatia@linux.dev> wrote:

> From: Aradhya Bhatia <a-bhatia1@ti.com>
>
> Move the bridge pre_enable call before crtc enable, and the bridge
> post_disable call after the crtc disable.

This commit regresses the MCDE driver to a hard boot failure.
Reverting the patch fixes the issue.

I think it has something to do with the internal DSI bridge in
drivers/gpu/drm/mcde/mcde_dsi.c

Any hints to what needs to be fixed here?

Yours,
Linus Walleij

Hi,

On 05.06.2025 19:15, Aradhya Bhatia wrote:
> From: Aradhya Bhatia <a-bhatia1@ti.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@linaro.org>
> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
> Tested-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> Tested-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
> Signed-off-by: Aradhya Bhatia <aradhya.bhatia@linux.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

Hi,

On 11/06/2025 13:45, Marek Szyprowski wrote:
> Hi,
> 
> On 05.06.2025 19:15, Aradhya Bhatia wrote:
>> From: Aradhya Bhatia <a-bhatia1@ti.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@linaro.org>
>> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Tested-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> Tested-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>> Signed-off-by: Aradhya Bhatia <aradhya.bhatia@linux.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?

 Tomi

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@ti.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@linaro.org>
>>> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
>>> Tested-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>> Tested-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
>>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>>> Signed-off-by: Aradhya Bhatia <aradhya.bhatia@linux.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.

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

Hi,

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@ti.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@linaro.org>
>>>> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>>> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>> Tested-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>>> Tested-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
>>>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>>>> Signed-off-by: Aradhya Bhatia <aradhya.bhatia@linux.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 =)

 Tomi

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@ti.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@linaro.org>
>>>>> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>>>> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>>> Tested-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>>>> Tested-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
>>>>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>>>>> Signed-off-by: Aradhya Bhatia <aradhya.bhatia@linux.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.

I've did some experiments with pm runtime of the analogix_dp_core driver 
and increased autosuspend delay to 200 msec. This magically fixed the 
issue, but I still see no direct calls to analogix_dp without proper pm 
runtime guards.


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

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@ti.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@linaro.org>
> >>>>> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> >>>>> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
> >>>>> Tested-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> >>>>> Tested-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> >>>>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
> >>>>> Signed-off-by: Aradhya Bhatia <aradhya.bhatia@linux.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 :)

Maxime

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@ti.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@linaro.org>
>>>>>>> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>>>>>> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>>>>> Tested-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>>>>>> Tested-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
>>>>>>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>>>>>>> Signed-off-by: Aradhya Bhatia <aradhya.bhatia@linux.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

On 18/06/2025 13:01, Marek Szyprowski wrote:

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

We have another issue the this patch: i.MX8MM (and MP) seem to have a
problem with DSI, which is using the samsung-dsim.c driver. For me, the
first time I run kmstest I get a black screen. The second time I get a
display, but it's horizontally in the wrong place.

Perhaps you have an idea about this if you're familiar with the
samsung-dsim?.

The issue seems to be that earlier the order was:

mxsfb_crtc_atomic_enable()
samsung_dsim_atomic_pre_enable()
samsung_dsim_atomic_enable()

now the order is:

samsung_dsim_atomic_pre_enable()
mxsfb_crtc_atomic_enable()
samsung_dsim_atomic_enable()

If I move the samsung_dsim_init() call from
samsung_dsim_atomic_pre_enable() to samsung_dsim_atomic_enable(), it
starts to work.

My guess is that the DSI init requires a pixel stream from the crtc, and
now that the (pre)init is done without the pixel stream, it goes wrong.
But this is purely a guess so far.

I also see that the samsung_dsim_init() call is behind
"!samsung_dsim_hw_is_exynos()", so you probably don't hit this issue...

 Tomi

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@ti.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@linaro.org>
>>>>>>>> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>>>>>>> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>>>>>> Tested-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>>>>>>>> Tested-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
>>>>>>>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>>>>>>>> Signed-off-by: Aradhya Bhatia <aradhya.bhatia@linux.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