[PATCH v3 0/3] drm/atomic-helpers: Fix MCDE/R-Car DU regressions

[PATCH v3 0/3] drm/atomic-helpers: Fix MCDE/R-Car DU regressions

[Linus Walleij]

This fixes two regressions experienced in the MCDE and
R-Car DU DRM drivers after
commit c9b1150a68d9362a0827609fc0dc1664c0d8bfe1
"drm/atomic-helper: Re-order bridge chain pre-enable and post-disable"
caused a series of regressions in all panels that send
DSI commands in their .prepare() and .unprepare()
callbacks.

This series make it possible to selectively bring back the
old behaviour with explicit semantics and implements
the old behaviour as modified commit tails in MCDE and
R-Car DU.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
Changes in v3:
- Switch to a new approach: export helper functions and create the
  special helper directly in the driver instead.
- Drop Marek's patch and write a new RFT patch with the same
  semantic content as the MCDE patch.
- Link to v2: https://lore.kernel.org/r/20251118-mcde-drm-regression-v2-0-4fedf10b18f6@linaro.org

Changes in v2:
- Queue Marek's patch first in the series for coherency.
- Add a patch to also preserve the late CRTC disablement
  semantic.
- Rename helper function to reflect the new semantic.
- Update the MCDE patch to use the new callbacks.
- Link to v1: https://lore.kernel.org/r/20251118-mcde-drm-regression-v1-1-ed5583efbd68@linaro.org

---
Linus Walleij (3):
      drm/atomic-helper: Export and namespace some functions
      drm/mcde: Create custom commit tail
      RFT: drm/rcar-du: Modify custom commit tail

 drivers/gpu/drm/drm_atomic_helper.c           | 54 +++++++++++++++------------
 drivers/gpu/drm/mcde/mcde_drv.c               | 37 +++++++++++++++++-
 drivers/gpu/drm/renesas/rcar-du/rcar_du_kms.c | 25 +++++++++++--
 include/drm/drm_atomic_helper.h               | 19 ++++++++++
 4 files changed, 108 insertions(+), 27 deletions(-)
---
base-commit: 6548d364a3e850326831799d7e3ea2d7bb97ba08
change-id: 20251120-mcde-drm-regression-thirdfix-1b0abfb52209

Best regards,
-- 
Linus Walleij <linus.walleij@linaro.org>

[PATCH v3 1/3] drm/atomic-helper: Export and namespace some functions

[Linus Walleij]

Export and namespace those not prefixed with drm_* so
it becomes possible to write custom commit tail functions
in individual drivers using the helper infrastructure.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpu/drm/drm_atomic_helper.c | 54 +++++++++++++++++++++----------------
 include/drm/drm_atomic_helper.h     | 19 +++++++++++++
 2 files changed, 50 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index d5ebe6ea0acb..906eb4b0852c 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1162,8 +1162,8 @@ crtc_needs_disable(struct drm_crtc_state *old_state,
 	       new_state->self_refresh_active;
 }
 
-static void
-encoder_bridge_disable(struct drm_device *dev, struct drm_atomic_state *state)
+void
+drm_encoder_bridge_disable(struct drm_device *dev, struct drm_atomic_state *state)
 {
 	struct drm_connector *connector;
 	struct drm_connector_state *old_conn_state, *new_conn_state;
@@ -1229,9 +1229,10 @@ encoder_bridge_disable(struct drm_device *dev, struct drm_atomic_state *state)
 		}
 	}
 }
+EXPORT_SYMBOL(drm_encoder_bridge_disable);
 
-static void
-crtc_disable(struct drm_device *dev, struct drm_atomic_state *state)
+void
+drm_crtc_disable(struct drm_device *dev, struct drm_atomic_state *state)
 {
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
@@ -1282,9 +1283,10 @@ crtc_disable(struct drm_device *dev, struct drm_atomic_state *state)
 			drm_crtc_vblank_put(crtc);
 	}
 }
+EXPORT_SYMBOL(drm_crtc_disable);
 
-static void
-encoder_bridge_post_disable(struct drm_device *dev, struct drm_atomic_state *state)
+void
+drm_encoder_bridge_post_disable(struct drm_device *dev, struct drm_atomic_state *state)
 {
 	struct drm_connector *connector;
 	struct drm_connector_state *old_conn_state, *new_conn_state;
@@ -1335,15 +1337,16 @@ encoder_bridge_post_disable(struct drm_device *dev, struct drm_atomic_state *sta
 		drm_bridge_put(bridge);
 	}
 }
+EXPORT_SYMBOL(drm_encoder_bridge_post_disable);
 
 static void
 disable_outputs(struct drm_device *dev, struct drm_atomic_state *state)
 {
-	encoder_bridge_disable(dev, state);
+	drm_encoder_bridge_disable(dev, state);
 
-	crtc_disable(dev, state);
+	drm_crtc_disable(dev, state);
 
-	encoder_bridge_post_disable(dev, state);
+	drm_encoder_bridge_post_disable(dev, state);
 }
 
 /**
@@ -1446,8 +1449,8 @@ void drm_atomic_helper_calc_timestamping_constants(struct drm_atomic_state *stat
 }
 EXPORT_SYMBOL(drm_atomic_helper_calc_timestamping_constants);
 
-static void
-crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *state)
+void
+drm_crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *state)
 {
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *new_crtc_state;
@@ -1508,6 +1511,7 @@ crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *state)
 		drm_bridge_put(bridge);
 	}
 }
+EXPORT_SYMBOL(drm_crtc_set_mode);
 
 /**
  * drm_atomic_helper_commit_modeset_disables - modeset commit to disable outputs
@@ -1531,12 +1535,12 @@ void drm_atomic_helper_commit_modeset_disables(struct drm_device *dev,
 	drm_atomic_helper_update_legacy_modeset_state(dev, state);
 	drm_atomic_helper_calc_timestamping_constants(state);
 
-	crtc_set_mode(dev, state);
+	drm_crtc_set_mode(dev, state);
 }
 EXPORT_SYMBOL(drm_atomic_helper_commit_modeset_disables);
 
-static void drm_atomic_helper_commit_writebacks(struct drm_device *dev,
-						struct drm_atomic_state *state)
+void drm_atomic_helper_commit_writebacks(struct drm_device *dev,
+					 struct drm_atomic_state *state)
 {
 	struct drm_connector *connector;
 	struct drm_connector_state *new_conn_state;
@@ -1555,9 +1559,10 @@ static void drm_atomic_helper_commit_writebacks(struct drm_device *dev,
 		}
 	}
 }
+EXPORT_SYMBOL(drm_atomic_helper_commit_writebacks);
 
-static void
-encoder_bridge_pre_enable(struct drm_device *dev, struct drm_atomic_state *state)
+void
+drm_encoder_bridge_pre_enable(struct drm_device *dev, struct drm_atomic_state *state)
 {
 	struct drm_connector *connector;
 	struct drm_connector_state *new_conn_state;
@@ -1588,9 +1593,10 @@ encoder_bridge_pre_enable(struct drm_device *dev, struct drm_atomic_state *state
 		drm_bridge_put(bridge);
 	}
 }
+EXPORT_SYMBOL(drm_encoder_bridge_pre_enable);
 
-static void
-crtc_enable(struct drm_device *dev, struct drm_atomic_state *state)
+void
+drm_crtc_enable(struct drm_device *dev, struct drm_atomic_state *state)
 {
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *old_crtc_state;
@@ -1619,9 +1625,10 @@ crtc_enable(struct drm_device *dev, struct drm_atomic_state *state)
 		}
 	}
 }
+EXPORT_SYMBOL(drm_crtc_enable);
 
-static void
-encoder_bridge_enable(struct drm_device *dev, struct drm_atomic_state *state)
+void
+drm_encoder_bridge_enable(struct drm_device *dev, struct drm_atomic_state *state)
 {
 	struct drm_connector *connector;
 	struct drm_connector_state *new_conn_state;
@@ -1664,6 +1671,7 @@ encoder_bridge_enable(struct drm_device *dev, struct drm_atomic_state *state)
 		drm_bridge_put(bridge);
 	}
 }
+EXPORT_SYMBOL(drm_encoder_bridge_enable);
 
 /**
  * drm_atomic_helper_commit_modeset_enables - modeset commit to enable outputs
@@ -1682,11 +1690,11 @@ 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)
 {
-	encoder_bridge_pre_enable(dev, state);
+	drm_encoder_bridge_pre_enable(dev, state);
 
-	crtc_enable(dev, state);
+	drm_crtc_enable(dev, state);
 
-	encoder_bridge_enable(dev, state);
+	drm_encoder_bridge_enable(dev, state);
 
 	drm_atomic_helper_commit_writebacks(dev, state);
 }
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
index 53382fe93537..39878aa485c3 100644
--- a/include/drm/drm_atomic_helper.h
+++ b/include/drm/drm_atomic_helper.h
@@ -60,6 +60,11 @@ int drm_atomic_helper_check_plane_state(struct drm_plane_state *plane_state,
 int drm_atomic_helper_check_planes(struct drm_device *dev,
 			       struct drm_atomic_state *state);
 int drm_atomic_helper_check_crtc_primary_plane(struct drm_crtc_state *crtc_state);
+void drm_encoder_bridge_disable(struct drm_device *dev,
+				struct drm_atomic_state *state);
+void drm_crtc_disable(struct drm_device *dev, struct drm_atomic_state *state);
+void drm_encoder_bridge_post_disable(struct drm_device *dev,
+				     struct drm_atomic_state *state);
 int drm_atomic_helper_check(struct drm_device *dev,
 			    struct drm_atomic_state *state);
 void drm_atomic_helper_commit_tail(struct drm_atomic_state *state);
@@ -89,8 +94,22 @@ drm_atomic_helper_update_legacy_modeset_state(struct drm_device *dev,
 void
 drm_atomic_helper_calc_timestamping_constants(struct drm_atomic_state *state);
 
+void drm_crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *state);
+
 void drm_atomic_helper_commit_modeset_disables(struct drm_device *dev,
 					       struct drm_atomic_state *state);
+
+void drm_atomic_helper_commit_writebacks(struct drm_device *dev,
+					 struct drm_atomic_state *state);
+
+void drm_encoder_bridge_pre_enable(struct drm_device *dev,
+				   struct drm_atomic_state *state);
+
+void drm_crtc_enable(struct drm_device *dev, struct drm_atomic_state *state);
+
+void drm_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 *old_state);
 

-- 
2.51.1

[PATCH v3 2/3] drm/mcde: Create custom commit tail

[Linus Walleij]

commit c9b1150a68d9362a0827609fc0dc1664c0d8bfe1
"drm/atomic-helper: Re-order bridge chain pre-enable and post-disable"
caused a series of regressions in all panels that send
DSI commands in their .prepare() and .unprepare()
callbacks when used with MCDE.

As the CRTC is no longer online at bridge_pre_enable()
and gone at brige_post_disable() which maps to the panel
bridge .prepare()/.unprepare() callbacks, any CRTC that
enable/disable the DSI transmitter in it's enable/disable
callbacks will be unable to send any DSI commands in the
.prepare() and .unprepare() callbacks.

However the MCDE driver definitely need the CRTC to be
enabled during .prepare()/.unprepare().

Solve this by implementing a custom commit tail function
in the MCDE driver that always enables the CRTC first
and disables it last, using the newly exported helpers.

Link: https://lore.kernel.org/dri-devel/20251026-fix-mcde-drm-regression-v2-0-8d799e488cf9@linaro.org/
Link: https://lore.kernel.org/all/20251107230517.471894-1-marek.vasut%2Brenesas%40mailbox.org/
Fixes: c9b1150a68d9 ("drm/atomic-helper: Re-order bridge chain pre-enable and post-disable")
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpu/drm/mcde/mcde_drv.c | 37 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 36 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/mcde/mcde_drv.c b/drivers/gpu/drm/mcde/mcde_drv.c
index 5f2c462bad7e..290082c86100 100644
--- a/drivers/gpu/drm/mcde/mcde_drv.c
+++ b/drivers/gpu/drm/mcde/mcde_drv.c
@@ -100,13 +100,48 @@ static const struct drm_mode_config_funcs mcde_mode_config_funcs = {
 	.atomic_commit = drm_atomic_helper_commit,
 };
 
+/*
+ * This commit tail explicitly copies and changes the behaviour of
+ * the related core DRM atomic helper instead of trying to make
+ * the core helpers overly generic.
+ */
+static void mcde_atomic_commit_tail(struct drm_atomic_state *state)
+{
+	struct drm_device *dev = state->dev;
+
+	/* Variant of drm_atomic_helper_commit_modeset_disables() */
+	drm_encoder_bridge_disable(dev, state);
+	drm_encoder_bridge_post_disable(dev, state);
+	drm_crtc_disable(dev, state);
+	drm_atomic_helper_update_legacy_modeset_state(dev, state);
+	drm_atomic_helper_calc_timestamping_constants(state);
+	drm_crtc_set_mode(dev, state);
+
+	/* Variant of drm_atomic_helper_commit_modeset_enables() */
+	drm_crtc_enable(dev, state);
+	drm_encoder_bridge_pre_enable(dev, state);
+	drm_encoder_bridge_enable(dev, state);
+	drm_atomic_helper_commit_writebacks(dev, state);
+
+	drm_atomic_helper_commit_planes(dev, state,
+					DRM_PLANE_COMMIT_ACTIVE_ONLY);
+
+	drm_atomic_helper_fake_vblank(state);
+
+	drm_atomic_helper_commit_hw_done(state);
+
+	drm_atomic_helper_wait_for_vblanks(dev, state);
+
+	drm_atomic_helper_cleanup_planes(dev, state);
+}
+
 static const struct drm_mode_config_helper_funcs mcde_mode_config_helpers = {
 	/*
 	 * Using this function is necessary to commit atomic updates
 	 * that need the CRTC to be enabled before a commit, as is
 	 * the case with e.g. DSI displays.
 	 */
-	.atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
+	.atomic_commit_tail = mcde_atomic_commit_tail,
 };
 
 static irqreturn_t mcde_irq(int irq, void *data)

-- 
2.51.1

[PATCH v3 3/3] RFT: drm/rcar-du: Modify custom commit tail

[Linus Walleij]

commit c9b1150a68d9362a0827609fc0dc1664c0d8bfe1
"drm/atomic-helper: Re-order bridge chain pre-enable and post-disable"
caused regressions in all bridges that e.g. send DSI commands in
their .prepare() and .unprepare() callbacks when used with R-Car DU.

This is needed on R-Car DU, where the CRTC provides clock to LVDS
and DSI, and has to be started before a bridge may call .prepare,
which may trigger e.g. a DSI transfer.

This specifically fixes the case where ILI9881C is connected to R-Car
DU DSI. The ILI9881C panel driver does DSI command transfer in its
struct drm_panel_funcs .prepare function, which is currently called
before R-Car DU rcar_du_crtc_atomic_enable() rcar_mipi_dsi_pclk_enable()
and the DSI command transfer times out.

Fixes: c9b1150a68d9 ("drm/atomic-helper: Re-order bridge chain pre-enable and post-disable")
Link: https://lore.kernel.org/all/20251107230517.471894-1-marek.vasut%2Brenesas%40mailbox.org/
Co-developed-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
This is a modified version of Marek's patch using the approach
from MCDE. I'm pretty sure this driver also needs the original
semantic ording during disablement, and it surely doesn't hurt
to restore it too.
---
 drivers/gpu/drm/renesas/rcar-du/rcar_du_kms.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/renesas/rcar-du/rcar_du_kms.c
index 216219accfd9..d1d756f40fc6 100644
--- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_kms.c
+++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_kms.c
@@ -540,11 +540,30 @@ static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state)
 			rcdu->dpad1_source = rcrtc->index;
 	}
 
-	/* Apply the atomic update. */
-	drm_atomic_helper_commit_modeset_disables(dev, old_state);
+	/*
+	 * Apply the atomic update.
+	 *
+	 * We need special ordering to make sure the CRTC disabled last
+	 * and enabled first. We do this with modified versions of the
+	 * common modeset_disables/enables functions.
+	 */
+
+	/* Variant of drm_atomic_helper_commit_modeset_disables() */
+	drm_encoder_bridge_disable(dev, state);
+	drm_encoder_bridge_post_disable(dev, state);
+	drm_crtc_disable(dev, state);
+	drm_atomic_helper_update_legacy_modeset_state(dev, state);
+	drm_atomic_helper_calc_timestamping_constants(state);
+	drm_crtc_set_mode(dev, state);
+
 	drm_atomic_helper_commit_planes(dev, old_state,
 					DRM_PLANE_COMMIT_ACTIVE_ONLY);
-	drm_atomic_helper_commit_modeset_enables(dev, old_state);
+
+	/* Variant of drm_atomic_helper_commit_modeset_enables() */
+	drm_crtc_enable(dev, state);
+	drm_encoder_bridge_pre_enable(dev, state);
+	drm_encoder_bridge_enable(dev, state);
+	drm_atomic_helper_commit_writebacks(dev, state);
 
 	drm_atomic_helper_commit_hw_done(old_state);
 	drm_atomic_helper_wait_for_flip_done(dev, old_state);

-- 
2.51.1

Re: [PATCH v3 3/3] RFT: drm/rcar-du: Modify custom commit tail

[Laurent Pinchart]

Hi Linus,

Thank you for the patch.

On Thu, Nov 20, 2025 at 11:55:34PM +0100, Linus Walleij wrote:
> commit c9b1150a68d9362a0827609fc0dc1664c0d8bfe1
> "drm/atomic-helper: Re-order bridge chain pre-enable and post-disable"
> caused regressions in all bridges that e.g. send DSI commands in
> their .prepare() and .unprepare() callbacks when used with R-Car DU.
> 
> This is needed on R-Car DU, where the CRTC provides clock to LVDS
> and DSI, and has to be started before a bridge may call .prepare,
> which may trigger e.g. a DSI transfer.

Is there an issue with LVDS ? The LVDS encoder receivers its pixel clock
from the CRTC the same way any encoder does (except on R-Car D3 and E3
where the encoder *provides* the pixel clock to the CRTC, which is
handled through explicit function calls from the CRTC to the LVDS
encoder). There's no command mode with LVDS. Is the concern that we may
have an external LVDS to DSI bridge ?

> This specifically fixes the case where ILI9881C is connected to R-Car
> DU DSI. The ILI9881C panel driver does DSI command transfer in its
> struct drm_panel_funcs .prepare function, which is currently called
> before R-Car DU rcar_du_crtc_atomic_enable() rcar_mipi_dsi_pclk_enable()
> and the DSI command transfer times out.
> 
> Fixes: c9b1150a68d9 ("drm/atomic-helper: Re-order bridge chain pre-enable and post-disable")
> Link: https://lore.kernel.org/all/20251107230517.471894-1-marek.vasut%2Brenesas%40mailbox.org/
> Co-developed-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> This is a modified version of Marek's patch using the approach
> from MCDE. I'm pretty sure this driver also needs the original
> semantic ording during disablement, and it surely doesn't hurt
> to restore it too.
> ---
>  drivers/gpu/drm/renesas/rcar-du/rcar_du_kms.c | 25 ++++++++++++++++++++++---
>  1 file changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/renesas/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/renesas/rcar-du/rcar_du_kms.c
> index 216219accfd9..d1d756f40fc6 100644
> --- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_kms.c
> +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_kms.c
> @@ -540,11 +540,30 @@ static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state)
>  			rcdu->dpad1_source = rcrtc->index;
>  	}
>  
> -	/* Apply the atomic update. */
> -	drm_atomic_helper_commit_modeset_disables(dev, old_state);
> +	/*
> +	 * Apply the atomic update.
> +	 *
> +	 * We need special ordering to make sure the CRTC disabled last
> +	 * and enabled first. We do this with modified versions of the
> +	 * common modeset_disables/enables functions.
> +	 */
> +
> +	/* Variant of drm_atomic_helper_commit_modeset_disables() */
> +	drm_encoder_bridge_disable(dev, state);
> +	drm_encoder_bridge_post_disable(dev, state);
> +	drm_crtc_disable(dev, state);

I think we have a fundamental issue here. Commit c9b1150a68d9
("drm/atomic-helper: Re-order bridge chain pre-enable and post-disable")
states that

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

This is right, and the above sequence does not comply with the
documentation, which is a concern. Quite clearly the bridge API isn't up
to the task here. I don't know how we'll fix it, the pre/post
enable/disable operations are really a hack and don't scale, and fixing
that will likely not be a simple task.

The short term question is how to deal with the regression that
c9b1150a68d9 caused in the MCDE and R-Car DU drivers. This patch
probably works. The complexity makes me worry that we'll introduce other
regressions, but it can be argued that we're merely restoring the
previous order of operations, which should therefore be safe. I'm still
concerned about maintainability though. Commit c9b1150a68d9 should
probably have been rejected, we should have developed a proper solution
instead :-(

> +	drm_atomic_helper_update_legacy_modeset_state(dev, state);
> +	drm_atomic_helper_calc_timestamping_constants(state);
> +	drm_crtc_set_mode(dev, state);
> +
>  	drm_atomic_helper_commit_planes(dev, old_state,
>  					DRM_PLANE_COMMIT_ACTIVE_ONLY);
> -	drm_atomic_helper_commit_modeset_enables(dev, old_state);
> +
> +	/* Variant of drm_atomic_helper_commit_modeset_enables() */
> +	drm_crtc_enable(dev, state);
> +	drm_encoder_bridge_pre_enable(dev, state);
> +	drm_encoder_bridge_enable(dev, state);
> +	drm_atomic_helper_commit_writebacks(dev, state);

That looks pretty horrible :-/ Not your fault of course.

Maxime, what's your opinion ?

>  
>  	drm_atomic_helper_commit_hw_done(old_state);
>  	drm_atomic_helper_wait_for_flip_done(dev, old_state);

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 3/3] RFT: drm/rcar-du: Modify custom commit tail

[Linus Walleij]

Hi Laurent,

On Fri, Nov 21, 2025 at 3:42 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:

> > This is needed on R-Car DU, where the CRTC provides clock to LVDS
> > and DSI, and has to be started before a bridge may call .prepare,
> > which may trigger e.g. a DSI transfer.
>
> Is there an issue with LVDS ? The LVDS encoder receivers its pixel clock
> from the CRTC the same way any encoder does (except on R-Car D3 and E3
> where the encoder *provides* the pixel clock to the CRTC, which is
> handled through explicit function calls from the CRTC to the LVDS
> encoder). There's no command mode with LVDS. Is the concern that we may
> have an external LVDS to DSI bridge ?

Question to Marek, this commit text is from his original patch (which
I modified heavily so almost only the commit message is left...)

> > -     /* Apply the atomic update. */
> > -     drm_atomic_helper_commit_modeset_disables(dev, old_state);
> > +     /*
> > +      * Apply the atomic update.
> > +      *
> > +      * We need special ordering to make sure the CRTC disabled last
> > +      * and enabled first. We do this with modified versions of the
> > +      * common modeset_disables/enables functions.
> > +      */
> > +
> > +     /* Variant of drm_atomic_helper_commit_modeset_disables() */
> > +     drm_encoder_bridge_disable(dev, state);
> > +     drm_encoder_bridge_post_disable(dev, state);
> > +     drm_crtc_disable(dev, state);
>
> I think we have a fundamental issue here. Commit c9b1150a68d9
> ("drm/atomic-helper: Re-order bridge chain pre-enable and post-disable")
> states that
>
>     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".
>
> This is right, and the above sequence does not comply with the
> documentation, which is a concern. Quite clearly the bridge API isn't up
> to the task here. I don't know how we'll fix it, the pre/post
> enable/disable operations are really a hack and don't scale, and fixing
> that will likely not be a simple task.

Well in the v1 patch I tried to go with this definition, if:

1. The display pipe is not running and
2. The hardware is such that DSI will not work unless the display
    pipe is running then it follows logically that:

3. We cannot send DSI commands in bridge prepare()/unprepare()
   execution paths.

Thus the v1 patch moves all DSI commands to the enable/disable
callbacks. It fixes the regression, too.

We would need to comb over the existing DSI bridges and panels
to convert them to this semantic if we wanna be strict, what I
did was to just fix all panels used by this one hardware. I'm pretty
sure the same can be done of any R-Car DU-related panel/bridge.

> The short term question is how to deal with the regression that
> c9b1150a68d9 caused in the MCDE and R-Car DU drivers. This patch
> probably works. The complexity makes me worry that we'll introduce other
> regressions, but it can be argued that we're merely restoring the
> previous order of operations, which should therefore be safe. I'm still
> concerned about maintainability though. Commit c9b1150a68d9 should
> probably have been rejected, we should have developed a proper solution
> instead :-(

Yeah, it's a bit of a mess when regressions get detected late.
I'm also worried about more regressions popping up. They will
all be with DSI panels/bridges I think.

Yours,
Linus Walleij

Re: [PATCH v3 3/3] RFT: drm/rcar-du: Modify custom commit tail

[Geert Uytterhoeven]

Hi Linus,

On Thu, 20 Nov 2025 at 23:56, Linus Walleij <linus.walleij@linaro.org> wrote:
> commit c9b1150a68d9362a0827609fc0dc1664c0d8bfe1
> "drm/atomic-helper: Re-order bridge chain pre-enable and post-disable"
> caused regressions in all bridges that e.g. send DSI commands in
> their .prepare() and .unprepare() callbacks when used with R-Car DU.
>
> This is needed on R-Car DU, where the CRTC provides clock to LVDS
> and DSI, and has to be started before a bridge may call .prepare,
> which may trigger e.g. a DSI transfer.
>
> This specifically fixes the case where ILI9881C is connected to R-Car
> DU DSI. The ILI9881C panel driver does DSI command transfer in its
> struct drm_panel_funcs .prepare function, which is currently called
> before R-Car DU rcar_du_crtc_atomic_enable() rcar_mipi_dsi_pclk_enable()
> and the DSI command transfer times out.
>
> Fixes: c9b1150a68d9 ("drm/atomic-helper: Re-order bridge chain pre-enable and post-disable")
> Link: https://lore.kernel.org/all/20251107230517.471894-1-marek.vasut%2Brenesas%40mailbox.org/
> Co-developed-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

Thanks for your patch!

> --- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_kms.c
> +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_kms.c
> @@ -540,11 +540,30 @@ static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state)
>                         rcdu->dpad1_source = rcrtc->index;
>         }
>
> -       /* Apply the atomic update. */
> -       drm_atomic_helper_commit_modeset_disables(dev, old_state);
> +       /*
> +        * Apply the atomic update.
> +        *
> +        * We need special ordering to make sure the CRTC disabled last
> +        * and enabled first. We do this with modified versions of the
> +        * common modeset_disables/enables functions.
> +        */
> +
> +       /* Variant of drm_atomic_helper_commit_modeset_disables() */
> +       drm_encoder_bridge_disable(dev, state);

drivers/gpu/drm/renesas/rcar-du/rcar_du_kms.c:555:41: error: ‘state’
undeclared (first use in this function); did you mean ‘statx’?

old_state (everywhere)?

After making that change, it still works on Koelsch (R-Car M2-W),
which was not affected by the breakage.

> +       drm_encoder_bridge_post_disable(dev, state);
> +       drm_crtc_disable(dev, state);
> +       drm_atomic_helper_update_legacy_modeset_state(dev, state);
> +       drm_atomic_helper_calc_timestamping_constants(state);
> +       drm_crtc_set_mode(dev, state);
> +
>         drm_atomic_helper_commit_planes(dev, old_state,
>                                         DRM_PLANE_COMMIT_ACTIVE_ONLY);
> -       drm_atomic_helper_commit_modeset_enables(dev, old_state);
> +
> +       /* Variant of drm_atomic_helper_commit_modeset_enables() */
> +       drm_crtc_enable(dev, state);
> +       drm_encoder_bridge_pre_enable(dev, state);
> +       drm_encoder_bridge_enable(dev, state);
> +       drm_atomic_helper_commit_writebacks(dev, state);
>
>         drm_atomic_helper_commit_hw_done(old_state);
>         drm_atomic_helper_wait_for_flip_done(dev, old_state);
>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v3 3/3] RFT: drm/rcar-du: Modify custom commit tail

[Linus Walleij]

Hi Geert,

thanks for your help!

On Fri, Nov 21, 2025 at 9:52 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> > +       /* Variant of drm_atomic_helper_commit_modeset_disables() */
> > +       drm_encoder_bridge_disable(dev, state);
>
> drivers/gpu/drm/renesas/rcar-du/rcar_du_kms.c:555:41: error: ‘state’
> undeclared (first use in this function); did you mean ‘statx’?
>
> old_state (everywhere)?

Fixing it up and sending v4 FWIW (we still don't know if we go this route).

> After making that change, it still works on Koelsch (R-Car M2-W),
> which was not affected by the breakage.

Recording this as Tested-by!

Yours,
Linus Walleij

Re: [PATCH v3 3/3] RFT: drm/rcar-du: Modify custom commit tail

[Laurent Pinchart]

On Fri, Nov 21, 2025 at 09:17:26AM +0100, Linus Walleij wrote:
> On Fri, Nov 22, 2025 at 3:42 AM Laurent Pinchart wrote:
> 
> > > This is needed on R-Car DU, where the CRTC provides clock to LVDS
> > > and DSI, and has to be started before a bridge may call .prepare,
> > > which may trigger e.g. a DSI transfer.
> >
> > Is there an issue with LVDS ? The LVDS encoder receivers its pixel clock
> > from the CRTC the same way any encoder does (except on R-Car D3 and E3
> > where the encoder *provides* the pixel clock to the CRTC, which is
> > handled through explicit function calls from the CRTC to the LVDS
> > encoder). There's no command mode with LVDS. Is the concern that we may
> > have an external LVDS to DSI bridge ?
> 
> Question to Marek, this commit text is from his original patch (which
> I modified heavily so almost only the commit message is left...)

Marek, could you comment on this ?

> > > -     /* Apply the atomic update. */
> > > -     drm_atomic_helper_commit_modeset_disables(dev, old_state);
> > > +     /*
> > > +      * Apply the atomic update.
> > > +      *
> > > +      * We need special ordering to make sure the CRTC disabled last
> > > +      * and enabled first. We do this with modified versions of the
> > > +      * common modeset_disables/enables functions.
> > > +      */
> > > +
> > > +     /* Variant of drm_atomic_helper_commit_modeset_disables() */
> > > +     drm_encoder_bridge_disable(dev, state);
> > > +     drm_encoder_bridge_post_disable(dev, state);
> > > +     drm_crtc_disable(dev, state);
> >
> > I think we have a fundamental issue here. Commit c9b1150a68d9
> > ("drm/atomic-helper: Re-order bridge chain pre-enable and post-disable")
> > states that
> >
> >     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".
> >
> > This is right, and the above sequence does not comply with the
> > documentation, which is a concern. Quite clearly the bridge API isn't up
> > to the task here. I don't know how we'll fix it, the pre/post
> > enable/disable operations are really a hack and don't scale, and fixing
> > that will likely not be a simple task.
> 
> Well in the v1 patch I tried to go with this definition, if:
> 
> 1. The display pipe is not running and
> 2. The hardware is such that DSI will not work unless the display
>     pipe is running then it follows logically that:
> 
> 3. We cannot send DSI commands in bridge prepare()/unprepare()
>    execution paths.
> 
> Thus the v1 patch moves all DSI commands to the enable/disable
> callbacks. It fixes the regression, too.
> 
> We would need to comb over the existing DSI bridges and panels
> to convert them to this semantic if we wanna be strict, what I
> did was to just fix all panels used by this one hardware. I'm pretty
> sure the same can be done of any R-Car DU-related panel/bridge.

I just noticed a similar issue was reported for the Rockchip display
drivers, see [1]. That's three platforms broken by commit
c9b1150a68d9362a0827609fc0dc1664c0d8bfe1 and there may be more we
haven't noticed yet. I think we should revert the commit and work on a
proper solution on top.

[1] https://lore.kernel.org/r/CAAMcf8Di8sc_XVZAnzQ9sUiUf-Ayvg2yjhx2dWmvvCnfF3pBRA@mail.gmail.com

> > The short term question is how to deal with the regression that
> > c9b1150a68d9 caused in the MCDE and R-Car DU drivers. This patch
> > probably works. The complexity makes me worry that we'll introduce other
> > regressions, but it can be argued that we're merely restoring the
> > previous order of operations, which should therefore be safe. I'm still
> > concerned about maintainability though. Commit c9b1150a68d9 should
> > probably have been rejected, we should have developed a proper solution
> > instead :-(
> 
> Yeah, it's a bit of a mess when regressions get detected late.
> I'm also worried about more regressions popping up. They will
> all be with DSI panels/bridges I think.

-- 
Regards,

Laurent Pinchart