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

[PATCH v6 0/4] 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 <linusw@kernel.org>
---
Changes in v6:
- As this problem has (probably) also appeared in the Rockchip
  driver, append an RFT patch for the Rockchip DRI driver at the
  end of the series.
- Ask Rockchip people if this solves their issue too.
- I was about to apply this with all the ACKs it had gotten,
  but let's decide what to do based on the Rockchip situation that
  appeared right now.
- Link to v5: https://lore.kernel.org/r/20251130-mcde-drm-regression-thirdfix-v5-0-aed71a32981d@kernel.org

Changes in v5:
- Prefix all exported atomic commit helpers with drm_atomic_helper_commit_*
- Add kerneldoc to all new exported atmomic commit helpers.
- Add comments into the MCDE and Rcar DU quirks explaining what is
  altered as compared to the standard helper functions.
- Link to v4: https://lore.kernel.org/r/20251121-mcde-drm-regression-thirdfix-v4-0-d89bf8c17f85@linaro.org

Changes in v4:
- Fix a copypaste-bug in the Renesas Rcar-DU driver.
- Actually compile this using the shmobile defconfig and make
  sure it works.
- Collect Geert's Tested-by.
- Link to v3: https://lore.kernel.org/r/20251120-mcde-drm-regression-thirdfix-v3-0-24b1e9886bbf@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 (4):
      drm/atomic-helper: Export and namespace some functions
      drm/mcde: Create custom commit tail
      drm/rcar-du: Modify custom commit tail
      RFT: drm/rockchip: Create custom commit tail

 drivers/gpu/drm/drm_atomic_helper.c           | 122 +++++++++++++++++++++-----
 drivers/gpu/drm/mcde/mcde_drv.c               |  45 +++++++++-
 drivers/gpu/drm/renesas/rcar-du/rcar_du_kms.c |  33 ++++++-
 drivers/gpu/drm/rockchip/rockchip_drm_fb.c    |  50 ++++++++++-
 include/drm/drm_atomic_helper.h               |  22 +++++
 5 files changed, 244 insertions(+), 28 deletions(-)
---
base-commit: 6548d364a3e850326831799d7e3ea2d7bb97ba08
change-id: 20251120-mcde-drm-regression-thirdfix-1b0abfb52209

Best regards,
-- 
Linus Walleij <linusw@kernel.org>


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v6 1/4] 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.

Tested-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
Reviewed-by: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Linus Walleij <linusw@kernel.org>
---
 drivers/gpu/drm/drm_atomic_helper.c | 122 +++++++++++++++++++++++++++++-------
 include/drm/drm_atomic_helper.h     |  22 +++++++
 2 files changed, 121 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index d5ebe6ea0acb..bfbe3a0ee178 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1162,8 +1162,18 @@ 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)
+/**
+ * drm_atomic_helper_commit_encoder_bridge_disable - disable bridges and encoder
+ * @dev: DRM device
+ * @state: the driver state object
+ *
+ * Loops over all connectors in the current state and if the CRTC needs
+ * it, disables the bridge chain all the way, then disables the encoder
+ * afterwards.
+ */
+void
+drm_atomic_helper_commit_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 +1239,18 @@ encoder_bridge_disable(struct drm_device *dev, struct drm_atomic_state *state)
 		}
 	}
 }
+EXPORT_SYMBOL(drm_atomic_helper_commit_encoder_bridge_disable);
 
-static void
-crtc_disable(struct drm_device *dev, struct drm_atomic_state *state)
+/**
+ * drm_atomic_helper_commit_crtc_disable - disable CRTSs
+ * @dev: DRM device
+ * @state: the driver state object
+ *
+ * Loops over all CRTCs in the current state and if the CRTC needs
+ * it, disables it.
+ */
+void
+drm_atomic_helper_commit_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 +1301,18 @@ crtc_disable(struct drm_device *dev, struct drm_atomic_state *state)
 			drm_crtc_vblank_put(crtc);
 	}
 }
+EXPORT_SYMBOL(drm_atomic_helper_commit_crtc_disable);
 
-static void
-encoder_bridge_post_disable(struct drm_device *dev, struct drm_atomic_state *state)
+/**
+ * drm_atomic_helper_commit_encoder_bridge_post_disable - post-disable encoder bridges
+ * @dev: DRM device
+ * @state: the driver state object
+ *
+ * Loops over all connectors in the current state and if the CRTC needs
+ * it, post-disables all encoder bridges.
+ */
+void
+drm_atomic_helper_commit_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 +1363,16 @@ encoder_bridge_post_disable(struct drm_device *dev, struct drm_atomic_state *sta
 		drm_bridge_put(bridge);
 	}
 }
+EXPORT_SYMBOL(drm_atomic_helper_commit_encoder_bridge_post_disable);
 
 static void
 disable_outputs(struct drm_device *dev, struct drm_atomic_state *state)
 {
-	encoder_bridge_disable(dev, state);
+	drm_atomic_helper_commit_encoder_bridge_disable(dev, state);
 
-	crtc_disable(dev, state);
+	drm_atomic_helper_commit_crtc_disable(dev, state);
 
-	encoder_bridge_post_disable(dev, state);
+	drm_atomic_helper_commit_encoder_bridge_post_disable(dev, state);
 }
 
 /**
@@ -1446,8 +1475,17 @@ 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)
+/**
+ * drm_atomic_helper_commit_crtc_set_mode - set the new mode
+ * @dev: DRM device
+ * @state: the driver state object
+ *
+ * Loops over all connectors in the current state and if the mode has
+ * changed, change the mode of the CRTC, then call down the bridge
+ * chain and change the mode in all bridges as well.
+ */
+void
+drm_atomic_helper_commit_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 +1546,7 @@ crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *state)
 		drm_bridge_put(bridge);
 	}
 }
+EXPORT_SYMBOL(drm_atomic_helper_commit_crtc_set_mode);
 
 /**
  * drm_atomic_helper_commit_modeset_disables - modeset commit to disable outputs
@@ -1531,12 +1570,21 @@ 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_atomic_helper_commit_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)
+/**
+ * drm_atomic_helper_commit_writebacks - issue writebacks
+ * @dev: DRM device
+ * @state: atomic state object being committed
+ *
+ * This loops over the connectors, checks if the new state requires
+ * a writeback job to be issued and in that case issues an atomic
+ * commit on each connector.
+ */
+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 +1603,18 @@ 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)
+/**
+ * drm_atomic_helper_commit_encoder_bridge_pre_enable - pre-enable bridges
+ * @dev: DRM device
+ * @state: atomic state object being committed
+ *
+ * This loops over the connectors and if the CRTC needs it, pre-enables
+ * the entire bridge chain.
+ */
+void
+drm_atomic_helper_commit_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 +1645,18 @@ encoder_bridge_pre_enable(struct drm_device *dev, struct drm_atomic_state *state
 		drm_bridge_put(bridge);
 	}
 }
+EXPORT_SYMBOL(drm_atomic_helper_commit_encoder_bridge_pre_enable);
 
-static void
-crtc_enable(struct drm_device *dev, struct drm_atomic_state *state)
+/**
+ * drm_atomic_helper_commit_crtc_enable - enables the CRTCs
+ * @dev: DRM device
+ * @state: atomic state object being committed
+ *
+ * This loops over CRTCs in the new state, and of the CRTC needs
+ * it, enables it.
+ */
+void
+drm_atomic_helper_commit_crtc_enable(struct drm_device *dev, struct drm_atomic_state *state)
 {
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *old_crtc_state;
@@ -1619,9 +1685,18 @@ crtc_enable(struct drm_device *dev, struct drm_atomic_state *state)
 		}
 	}
 }
+EXPORT_SYMBOL(drm_atomic_helper_commit_crtc_enable);
 
-static void
-encoder_bridge_enable(struct drm_device *dev, struct drm_atomic_state *state)
+/**
+ * drm_atomic_helper_commit_encoder_bridge_enable - enables the bridges
+ * @dev: DRM device
+ * @state: atomic state object being committed
+ *
+ * This loops over all connectors in the new state, and of the CRTC needs
+ * it, enables the entire bridge chain.
+ */
+void
+drm_atomic_helper_commit_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 +1739,7 @@ encoder_bridge_enable(struct drm_device *dev, struct drm_atomic_state *state)
 		drm_bridge_put(bridge);
 	}
 }
+EXPORT_SYMBOL(drm_atomic_helper_commit_encoder_bridge_enable);
 
 /**
  * drm_atomic_helper_commit_modeset_enables - modeset commit to enable outputs
@@ -1682,11 +1758,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_atomic_helper_commit_encoder_bridge_pre_enable(dev, state);
 
-	crtc_enable(dev, state);
+	drm_atomic_helper_commit_crtc_enable(dev, state);
 
-	encoder_bridge_enable(dev, state);
+	drm_atomic_helper_commit_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..9afc2e1e24c7 100644
--- a/include/drm/drm_atomic_helper.h
+++ b/include/drm/drm_atomic_helper.h
@@ -60,6 +60,12 @@ 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_atomic_helper_commit_encoder_bridge_disable(struct drm_device *dev,
+				struct drm_atomic_state *state);
+void drm_atomic_helper_commit_crtc_disable(struct drm_device *dev,
+				struct drm_atomic_state *state);
+void drm_atomic_helper_commit_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 +95,24 @@ 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_atomic_helper_commit_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_atomic_helper_commit_encoder_bridge_pre_enable(struct drm_device *dev,
+					struct drm_atomic_state *state);
+
+void drm_atomic_helper_commit_crtc_enable(struct drm_device *dev,
+					  struct drm_atomic_state *state);
+
+void drm_atomic_helper_commit_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


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v6 2/4] 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")
Tested-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
Reviewed-by: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Linus Walleij <linusw@kernel.org>
---
 drivers/gpu/drm/mcde/mcde_drv.c | 45 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 44 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/mcde/mcde_drv.c b/drivers/gpu/drm/mcde/mcde_drv.c
index 5f2c462bad7e..eb3a8bb8766b 100644
--- a/drivers/gpu/drm/mcde/mcde_drv.c
+++ b/drivers/gpu/drm/mcde/mcde_drv.c
@@ -100,13 +100,56 @@ 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()
+	 * that will disable and post-disable all bridges BEFORE
+	 * disabling the CRTC.
+	 */
+	drm_atomic_helper_commit_encoder_bridge_disable(dev, state);
+	drm_atomic_helper_commit_encoder_bridge_post_disable(dev, state);
+	drm_atomic_helper_commit_crtc_disable(dev, state);
+	drm_atomic_helper_update_legacy_modeset_state(dev, state);
+	drm_atomic_helper_calc_timestamping_constants(state);
+	drm_atomic_helper_commit_crtc_set_mode(dev, state);
+
+	/*
+	 * Variant of drm_atomic_helper_commit_modeset_enables()
+	 * that will enable the CRTC BEFORE pre-enabling and
+	 * enabling the bridges.
+	 */
+	drm_atomic_helper_commit_crtc_enable(dev, state);
+	drm_atomic_helper_commit_encoder_bridge_pre_enable(dev, state);
+	drm_atomic_helper_commit_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


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v6 3/4] 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/
Tested-by: Geert Uytterhoeven <geert@linux-m68k.org>
Co-developed-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
Tested-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
Reviewed-by: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Linus Walleij <linusw@kernel.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 | 33 ++++++++++++++++++++++++---
 1 file changed, 30 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..299d14ec486f 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,38 @@ 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()
+	 * that will disable and post-disable all bridges BEFORE
+	 * disabling the CRTC.
+	 */
+	drm_atomic_helper_commit_encoder_bridge_disable(dev, old_state);
+	drm_atomic_helper_commit_encoder_bridge_post_disable(dev, old_state);
+	drm_atomic_helper_commit_crtc_disable(dev, old_state);
+	drm_atomic_helper_update_legacy_modeset_state(dev, old_state);
+	drm_atomic_helper_calc_timestamping_constants(old_state);
+	drm_atomic_helper_commit_crtc_set_mode(dev, old_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()
+	 * that will enable the CRTC BEFORE pre-enabling and
+	 * enabling the bridges.
+	 */
+	drm_atomic_helper_commit_crtc_enable(dev, old_state);
+	drm_atomic_helper_commit_encoder_bridge_pre_enable(dev, old_state);
+	drm_atomic_helper_commit_encoder_bridge_enable(dev, old_state);
+	drm_atomic_helper_commit_writebacks(dev, old_state);
 
 	drm_atomic_helper_commit_hw_done(old_state);
 	drm_atomic_helper_wait_for_flip_done(dev, old_state);

-- 
2.51.1


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

[PATCH v6 4/4] RFT: drm/rockchip: 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 the Rockchip driver.

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 Rockchip driver definitely need the CRTC to be
enabled during .prepare()/.unprepare().

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

This patch is an edited carbon-copy of the same patch to
the ST-Ericsson MCDE driver.

Link: https://lore.kernel.org/all/CAAMcf8Di8sc_XVZAnzQ9sUiUf-Ayvg2yjhx2dWmvvCnfF3pBRA@mail.gmail.com/
Reported-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>
Reported-by: Vicente Bergas <vicencb@gmail.com>
Signed-off-by: Linus Walleij <linusw@kernel.org>
---
Rockchip people: can you please test this patch (along
with patch 1 of course).
---
 drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 50 +++++++++++++++++++++++++++++-
 1 file changed, 49 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
index 2f469d370021..63e50ea00920 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
@@ -24,8 +24,56 @@ static const struct drm_framebuffer_funcs rockchip_drm_fb_funcs = {
 	.dirty	       = drm_atomic_helper_dirtyfb,
 };
 
+/*
+ * 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 rockchip_drm_atomic_commit_tail(struct drm_atomic_state *state)
+{
+	struct drm_device *dev = state->dev;
+
+	/*
+	 * Variant of drm_atomic_helper_commit_modeset_disables()
+	 * that will disable and post-disable all bridges BEFORE
+	 * disabling the CRTC.
+	 */
+	drm_atomic_helper_commit_encoder_bridge_disable(dev, state);
+	drm_atomic_helper_commit_encoder_bridge_post_disable(dev, state);
+	drm_atomic_helper_commit_crtc_disable(dev, state);
+	drm_atomic_helper_update_legacy_modeset_state(dev, state);
+	drm_atomic_helper_calc_timestamping_constants(state);
+	drm_atomic_helper_commit_crtc_set_mode(dev, state);
+
+	/*
+	 * Variant of drm_atomic_helper_commit_modeset_enables()
+	 * that will enable the CRTC BEFORE pre-enabling and
+	 * enabling the bridges.
+	 */
+	drm_atomic_helper_commit_crtc_enable(dev, state);
+	drm_atomic_helper_commit_encoder_bridge_pre_enable(dev, state);
+	drm_atomic_helper_commit_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 rockchip_mode_config_helpers = {
-	.atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
+	/*
+	 * 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 = rockchip_drm_atomic_commit_tail,
 };
 
 static struct drm_framebuffer *

-- 
2.51.1


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v6 4/4] RFT: drm/rockchip: Create custom commit tail

[Chaoyi Chen]

Hi Linus,

On 12/3/2025 5:02 AM, Linus Walleij wrote:
> 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 the Rockchip driver.
> 
> 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 Rockchip driver definitely need the CRTC to be
> enabled during .prepare()/.unprepare().
> 
> Solve this by implementing a custom commit tail function
> in the Rockchip driver that always enables the CRTC first
> and disables it last, using the newly exported helpers.
> 
> This patch is an edited carbon-copy of the same patch to
> the ST-Ericsson MCDE driver.
> 
> Link: https://lore.kernel.org/all/CAAMcf8Di8sc_XVZAnzQ9sUiUf-Ayvg2yjhx2dWmvvCnfF3pBRA@mail.gmail.com/
> Reported-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>
> Reported-by: Vicente Bergas <vicencb@gmail.com>
> Signed-off-by: Linus Walleij <linusw@kernel.org>
> ---
> Rockchip people: can you please test this patch (along
> with patch 1 of course).
> ---
>  drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 50 +++++++++++++++++++++++++++++-
>  1 file changed, 49 insertions(+), 1 deletion(-)
> 

It seems that multiple drivers currently depend on the CRTC being
enabled and they implement the same atomic_commit_tail(). 

Why not implement this in drm_atomic_helper_commit_tail_rpm() instead?
Or why not use another common helper function for this?

-- 
Best, 
Chaoyi

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v6 2/4] drm/mcde: Create custom commit tail

[Chaoyi Chen]

Hi Linus,

On 12/3/2025 5:02 AM, Linus Walleij wrote:
> 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")
> Tested-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
> Reviewed-by: Maxime Ripard <mripard@kernel.org>
> Signed-off-by: Linus Walleij <linusw@kernel.org>
> ---
>  drivers/gpu/drm/mcde/mcde_drv.c | 45 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 44 insertions(+), 1 deletion(-)
> 

The bridge document says: "The display pipe (i.e. clocks and timing
signals) feeding this bridge will not yet be running when the 
@atomic_pre_enable is called."

Therefore, your approach seems to be merely a workaround and does not
meet the current requirements of the bridge.

And document also says: "The bridge can assume that the display pipe
(i.e. clocks and timing signals) feeding it is running when 
@atomic_enable callback is called."

If DSI commands need to wait for the display pipe (CRTC) to be ready,
why not perform them inside @atomic_enable instead of @atomic_pre_enable?

-- 
Best, 
Chaoyi

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v6 4/4] RFT: drm/rockchip: Create custom commit tail

[Linus Walleij]

On Wed, Dec 3, 2025 at 4:10 AM Chaoyi Chen <chaoyi.chen@rock-chips.com> wrote:

> It seems that multiple drivers currently depend on the CRTC being
> enabled and they implement the same atomic_commit_tail().
>
> Why not implement this in drm_atomic_helper_commit_tail_rpm() instead?
> Or why not use another common helper function for this?

So my v2 version of the patch series added a new special case
helper tail function to do that:
https://lore.kernel.org/dri-devel/20251118-mcde-drm-regression-v2-3-4fedf10b18f6@linaro.org/

It was politely NACKed for complicating the helpers library (short story).

It's part of the trail of how we got to this current patch series.

Yours,
Linus Walleij

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v6 4/4] RFT: drm/rockchip: Create custom commit tail

[Chaoyi Chen]

On 12/3/2025 5:54 PM, Linus Walleij wrote:
> On Wed, Dec 3, 2025 at 4:10 AM Chaoyi Chen <chaoyi.chen@rock-chips.com> wrote:
> 
>> It seems that multiple drivers currently depend on the CRTC being
>> enabled and they implement the same atomic_commit_tail().
>>
>> Why not implement this in drm_atomic_helper_commit_tail_rpm() instead?
>> Or why not use another common helper function for this?
> 
> So my v2 version of the patch series added a new special case
> helper tail function to do that:
> https://lore.kernel.org/dri-devel/20251118-mcde-drm-regression-v2-3-4fedf10b18f6@linaro.org/
> 
> It was politely NACKed for complicating the helpers library (short story).
> 
> It's part of the trail of how we got to this current patch series.
>

Ah, got it. Please take a look at my other comments to see if there's
a chance to change the bridge functions to achieve this. Thank you.

-- 
Best, 
Chaoyi

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v6 4/4] RFT: drm/rockchip: Create custom commit tail

[Vicente Bergas]

On Tue, Dec 2, 2025 at 10:03 PM Linus Walleij <linusw@kernel.org> wrote:
>
> 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 the Rockchip driver.
>
> 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 Rockchip driver definitely need the CRTC to be
> enabled during .prepare()/.unprepare().
>
> Solve this by implementing a custom commit tail function
> in the Rockchip driver that always enables the CRTC first
> and disables it last, using the newly exported helpers.
>
> This patch is an edited carbon-copy of the same patch to
> the ST-Ericsson MCDE driver.
>
> Link: https://lore.kernel.org/all/CAAMcf8Di8sc_XVZAnzQ9sUiUf-Ayvg2yjhx2dWmvvCnfF3pBRA@mail.gmail.com/
> Reported-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>
> Reported-by: Vicente Bergas <vicencb@gmail.com>
> Signed-off-by: Linus Walleij <linusw@kernel.org>
> ---
> Rockchip people: can you please test this patch (along
> with patch 1 of course).

Hi Linus,
i've applied all 4 patches from the V6 patch series on top of v6.18
and tested on the rk3399-gru-kevin platform.
It indeed fixes the reported issue.

Tested-by: Vicente Bergas <vicencb@gmail.com>

Regards,
  Vicente.

> ---
>  drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 50 +++++++++++++++++++++++++++++-
>  1 file changed, 49 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> index 2f469d370021..63e50ea00920 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> @@ -24,8 +24,56 @@ static const struct drm_framebuffer_funcs rockchip_drm_fb_funcs = {
>         .dirty         = drm_atomic_helper_dirtyfb,
>  };
>
> +/*
> + * 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 rockchip_drm_atomic_commit_tail(struct drm_atomic_state *state)
> +{
> +       struct drm_device *dev = state->dev;
> +
> +       /*
> +        * Variant of drm_atomic_helper_commit_modeset_disables()
> +        * that will disable and post-disable all bridges BEFORE
> +        * disabling the CRTC.
> +        */
> +       drm_atomic_helper_commit_encoder_bridge_disable(dev, state);
> +       drm_atomic_helper_commit_encoder_bridge_post_disable(dev, state);
> +       drm_atomic_helper_commit_crtc_disable(dev, state);
> +       drm_atomic_helper_update_legacy_modeset_state(dev, state);
> +       drm_atomic_helper_calc_timestamping_constants(state);
> +       drm_atomic_helper_commit_crtc_set_mode(dev, state);
> +
> +       /*
> +        * Variant of drm_atomic_helper_commit_modeset_enables()
> +        * that will enable the CRTC BEFORE pre-enabling and
> +        * enabling the bridges.
> +        */
> +       drm_atomic_helper_commit_crtc_enable(dev, state);
> +       drm_atomic_helper_commit_encoder_bridge_pre_enable(dev, state);
> +       drm_atomic_helper_commit_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 rockchip_mode_config_helpers = {
> -       .atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
> +       /*
> +        * 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 = rockchip_drm_atomic_commit_tail,
>  };
>
>  static struct drm_framebuffer *
>
> --
> 2.51.1
>

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v6 2/4] drm/mcde: Create custom commit tail

[Linus Walleij]

On Wed, Dec 3, 2025 at 7:27 AM Chaoyi Chen <chaoyi.chen@rock-chips.com> wrote:

> The bridge document says: "The display pipe (i.e. clocks and timing
> signals) feeding this bridge will not yet be running when the
> @atomic_pre_enable is called."
>
> Therefore, your approach seems to be merely a workaround and does not
> meet the current requirements of the bridge.
>
> And document also says: "The bridge can assume that the display pipe
> (i.e. clocks and timing signals) feeding it is running when
> @atomic_enable callback is called."
>
> If DSI commands need to wait for the display pipe (CRTC) to be ready,
> why not perform them inside @atomic_enable instead of @atomic_pre_enable?

That was exactly what the v1 and v2 versions of this patch set was
doing, and it was (politely) NACKed, and that is why we are
here.
https://lore.kernel.org/dri-devel/20251023-fix-mcde-drm-regression-v1-0-ed9a925db8c7@linaro.org/
https://lore.kernel.org/dri-devel/20251026-fix-mcde-drm-regression-v2-0-8d799e488cf9@linaro.org/

In essence, Tomi remarked that drivers should be able to send DSI
commands at atomic_pre_enable() which is for example
mapped to the .prepare() callback in the DSI panel bridge.
And he has a good point in this, I just converted the few panel
drivers that I was affected by, but there are many more such
and probably some bridges as well.

Yours,
Linus Walleij

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v6 2/4] drm/mcde: Create custom commit tail

[Chaoyi Chen]

On 12/4/2025 7:13 AM, Linus Walleij wrote:
> On Wed, Dec 3, 2025 at 7:27 AM Chaoyi Chen <chaoyi.chen@rock-chips.com> wrote:
> 
>> The bridge document says: "The display pipe (i.e. clocks and timing
>> signals) feeding this bridge will not yet be running when the
>> @atomic_pre_enable is called."
>>
>> Therefore, your approach seems to be merely a workaround and does not
>> meet the current requirements of the bridge.
>>
>> And document also says: "The bridge can assume that the display pipe
>> (i.e. clocks and timing signals) feeding it is running when
>> @atomic_enable callback is called."
>>
>> If DSI commands need to wait for the display pipe (CRTC) to be ready,
>> why not perform them inside @atomic_enable instead of @atomic_pre_enable?
> 
> That was exactly what the v1 and v2 versions of this patch set was
> doing, and it was (politely) NACKed, and that is why we are
> here.
> https://lore.kernel.org/dri-devel/20251023-fix-mcde-drm-regression-v1-0-ed9a925db8c7@linaro.org/
> https://lore.kernel.org/dri-devel/20251026-fix-mcde-drm-regression-v2-0-8d799e488cf9@linaro.org/
>

Hmm, I'm afraid there really isn't a common solution at this point.

The logical enable order of the CRTC, bridge, and panel may be
different from the actual physical enable order. And there is no
mechanism to change this order :(

So I believe your current patch to the mcde makes sense,
because you are indeed requiring the CRTC to be enabled at the
hardware level. But for other platforms, you may need to distinguish
whether this is a software dependency order problem caused by changes
to the bridge enable order.

> In essence, Tomi remarked that drivers should be able to send DSI
> commands at atomic_pre_enable() which is for example
> mapped to the .prepare() callback in the DSI panel bridge.
> And he has a good point in this, I just converted the few panel
> drivers that I was affected by, but there are many more such
> and probably some bridges as well.
> 
> Yours,
> Linus Walleij
> 
> 

-- 
Best, 
Chaoyi

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v6 2/4] drm/mcde: Create custom commit tail

[Tomi Valkeinen]

Hi,

On 04/12/2025 04:07, Chaoyi Chen wrote:
> On 12/4/2025 7:13 AM, Linus Walleij wrote:
>> On Wed, Dec 3, 2025 at 7:27 AM Chaoyi Chen <chaoyi.chen@rock-chips.com> wrote:
>>
>>> The bridge document says: "The display pipe (i.e. clocks and timing
>>> signals) feeding this bridge will not yet be running when the
>>> @atomic_pre_enable is called."
>>>
>>> Therefore, your approach seems to be merely a workaround and does not
>>> meet the current requirements of the bridge.
>>>
>>> And document also says: "The bridge can assume that the display pipe
>>> (i.e. clocks and timing signals) feeding it is running when
>>> @atomic_enable callback is called."
>>>
>>> If DSI commands need to wait for the display pipe (CRTC) to be ready,
>>> why not perform them inside @atomic_enable instead of @atomic_pre_enable?
>>
>> That was exactly what the v1 and v2 versions of this patch set was
>> doing, and it was (politely) NACKed, and that is why we are
>> here.
>> https://lore.kernel.org/dri-devel/20251023-fix-mcde-drm-regression-v1-0-ed9a925db8c7@linaro.org/
>> https://lore.kernel.org/dri-devel/20251026-fix-mcde-drm-regression-v2-0-8d799e488cf9@linaro.org/
>>
> 
> Hmm, I'm afraid there really isn't a common solution at this point.
> 
> The logical enable order of the CRTC, bridge, and panel may be
> different from the actual physical enable order. And there is no
> mechanism to change this order :(

I'm not sure what you mean with logical and physical enable orders...
But the fact seems to be that during enable sequence different
platforms, bridges and panels either 1) specifically require an incoming
stream from the crtc 2) specifically prohibit that, or 3) don't care.
And we don't have any means to deal with this.

In this series Linus changes the platforms' crtc to behave in a
particular way. But if you would connect a bridge or panel to those
platforms, which specifically does not want incoming stream during
pre_enable, those wouldn't work.

In any case, we need to get the current setups working. I made a quick
test series which:
- Reverts c9b1150a68d9362a0827609fc0dc1664c0d8bfe1
- Applies patch 1 from this series (with some conflict fixes due to the
above revert)
- Implements "pre-enable-before-crtc-enable" for tidss

This works ok, afaics, and resembles very much the platform patches in
this series.

That is a smaller series, and has the potential benefit that if there
are other platforms that have gotten broken but no one has noticed, they
would again get fixed.

Thoughts? Shall I send it, or shall we just go with this series?

One more point: even if the platforms in this series get fixed by
enabling the crtc first, I'm not sure if anyone else but Linus studied
the platforms to see if it would be possible for the DSI (or DP in one
case, if I recall right) to work without enabling the DRM crtc.

I think optimally the control bus should work independently from the
video data bus. E.g. one should be able to do DSI commands any time
after the DSI peripheral has been attached to the bridge, even outside
the enable sequence.

This could be possible by just enabling some clock on the crtc side, or
perhaps enabling some bypass clock mode.

Of course, there could also be other reasons to require/prohibit the
video stream...

 Tomi


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v6 0/4] drm/atomic-helpers: Fix MCDE/R-Car DU regressions

[Aradhya Bhatia]

Hi,


On 02/12/2025 21:02, Linus Walleij wrote:
> 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 <linusw@kernel.org>
> ---

Thank you for your patches, Linus. I am sorry multiple drivers had
regressed due the original patches.

I haven't been tracking the mailing lists actively past few weeks, and
only came across this when Laurent mentioned a patch from the v3 of this
series a couple days back in a discussion for the Rockchip drivers.

My email-id that is indeed used in all those conversations is long gone,
and I had hoped having the newer email-id in the sign-off-by tags would
make up for it. Perhaps that wasn't the case.


I am a little late, but I did give a look at the v6. I don't for-see any
concerns with it.


--
Regards
Aradhya



_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v6 1/4] drm/atomic-helper: Export and namespace some functions

[Aradhya Bhatia]

On 02/12/2025 21:02, Linus Walleij wrote:
> 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.
>
> Tested-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
> Reviewed-by: Maxime Ripard <mripard@kernel.org>
> Signed-off-by: Linus Walleij <linusw@kernel.org>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 122 +++++++++++++++++++++++++++++-------
>  include/drm/drm_atomic_helper.h     |  22 +++++++
>  2 files changed, 121 insertions(+), 23 deletions(-)
>

Reviewed-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v6 2/4] drm/mcde: Create custom commit tail

[Aradhya Bhatia]

Hi,

On 02/12/2025 21:02, Linus Walleij wrote:
> 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")
> Tested-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
> Reviewed-by: Maxime Ripard <mripard@kernel.org>
> Signed-off-by: Linus Walleij <linusw@kernel.org>
> ---
>  drivers/gpu/drm/mcde/mcde_drv.c | 45 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 44 insertions(+), 1 deletion(-)
>

Acked-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v6 3/4] drm/rcar-du: Modify custom commit tail

[Aradhya Bhatia]

Hi,

On 02/12/2025 21:02, 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.
>
> 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/
> Tested-by: Geert Uytterhoeven <geert@linux-m68k.org>
> Co-developed-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
> Tested-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
> Reviewed-by: Maxime Ripard <mripard@kernel.org>
> Signed-off-by: Linus Walleij <linusw@kernel.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 | 33 ++++++++++++++++++++++++---
>  1 file changed, 30 insertions(+), 3 deletions(-)
>

Acked-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v6 4/4] RFT: drm/rockchip: Create custom commit tail

[Aradhya Bhatia]

Hi,

On 02/12/2025 21:02, Linus Walleij wrote:
> 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 the Rockchip driver.
> 
> 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 Rockchip driver definitely need the CRTC to be
> enabled during .prepare()/.unprepare().
> 
> Solve this by implementing a custom commit tail function
> in the Rockchip driver that always enables the CRTC first
> and disables it last, using the newly exported helpers.
> 
> This patch is an edited carbon-copy of the same patch to
> the ST-Ericsson MCDE driver.
> 
> Link: https://lore.kernel.org/all/CAAMcf8Di8sc_XVZAnzQ9sUiUf-Ayvg2yjhx2dWmvvCnfF3pBRA@mail.gmail.com/
> Reported-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>
> Reported-by: Vicente Bergas <vicencb@gmail.com>
> Signed-off-by: Linus Walleij <linusw@kernel.org>
> ---
> Rockchip people: can you please test this patch (along
> with patch 1 of course).
> ---
>  drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 50 +++++++++++++++++++++++++++++-
>  1 file changed, 49 insertions(+), 1 deletion(-)
> 
Acked-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v6 2/4] drm/mcde: Create custom commit tail

[Chaoyi Chen]

Hello Tomi,

On 12/4/2025 9:54 PM, Tomi Valkeinen wrote:
> Hi,
> 
> On 04/12/2025 04:07, Chaoyi Chen wrote:
>> On 12/4/2025 7:13 AM, Linus Walleij wrote:
>>> On Wed, Dec 3, 2025 at 7:27 AM Chaoyi Chen <chaoyi.chen@rock-chips.com> wrote:
>>>
>>>> The bridge document says: "The display pipe (i.e. clocks and timing
>>>> signals) feeding this bridge will not yet be running when the
>>>> @atomic_pre_enable is called."
>>>>
>>>> Therefore, your approach seems to be merely a workaround and does not
>>>> meet the current requirements of the bridge.
>>>>
>>>> And document also says: "The bridge can assume that the display pipe
>>>> (i.e. clocks and timing signals) feeding it is running when
>>>> @atomic_enable callback is called."
>>>>
>>>> If DSI commands need to wait for the display pipe (CRTC) to be ready,
>>>> why not perform them inside @atomic_enable instead of @atomic_pre_enable?
>>>
>>> That was exactly what the v1 and v2 versions of this patch set was
>>> doing, and it was (politely) NACKed, and that is why we are
>>> here.
>>> https://lore.kernel.org/dri-devel/20251023-fix-mcde-drm-regression-v1-0-ed9a925db8c7@linaro.org/
>>> https://lore.kernel.org/dri-devel/20251026-fix-mcde-drm-regression-v2-0-8d799e488cf9@linaro.org/
>>>
>>
>> Hmm, I'm afraid there really isn't a common solution at this point.
>>
>> The logical enable order of the CRTC, bridge, and panel may be
>> different from the actual physical enable order. And there is no
>> mechanism to change this order :(
> 
> I'm not sure what you mean with logical and physical enable orders...
> But the fact seems to be that during enable sequence different
> platforms, bridges and panels either 1) specifically require an incoming
> stream from the crtc 2) specifically prohibit that, or 3) don't care.
> And we don't have any means to deal with this.
>

Yes, that's exactly what I meant. I'm referring to the fact that the
enable sequence is fixed in terms of code logic, but different 
hardware may have different requirements.

> In this series Linus changes the platforms' crtc to behave in a
> particular way. But if you would connect a bridge or panel to those
> platforms, which specifically does not want incoming stream during
> pre_enable, those wouldn't work.
> 
> In any case, we need to get the current setups working. I made a quick
> test series which:
> - Reverts c9b1150a68d9362a0827609fc0dc1664c0d8bfe1
> - Applies patch 1 from this series (with some conflict fixes due to the
> above revert)
> - Implements "pre-enable-before-crtc-enable" for tidss
> 
> This works ok, afaics, and resembles very much the platform patches in
> this series.
> 
> That is a smaller series, and has the potential benefit that if there
> are other platforms that have gotten broken but no one has noticed, they
> would again get fixed.
> 
> Thoughts? Shall I send it, or shall we just go with this series?
> 

Sounds good. I think we should compare the two to see which one is
better. So please send it.

> One more point: even if the platforms in this series get fixed by
> enabling the crtc first, I'm not sure if anyone else but Linus studied
> the platforms to see if it would be possible for the DSI (or DP in one
> case, if I recall right) to work without enabling the DRM crtc.
> 
> I think optimally the control bus should work independently from the
> video data bus. E.g. one should be able to do DSI commands any time
> after the DSI peripheral has been attached to the bridge, even outside
> the enable sequence.
> 
> This could be possible by just enabling some clock on the crtc side, or
> perhaps enabling some bypass clock mode.
> 
> Of course, there could also be other reasons to require/prohibit the
> video stream...

Yep, it all comes down to the hardware design. Different designs can 
naturally have different requirements.

-- 
Best, 
Chaoyi

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v6 4/4] RFT: drm/rockchip: Create custom commit tail

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 1313 bytes --]

On Wed, Dec 03, 2025 at 10:54:45AM +0100, Linus Walleij wrote:
> On Wed, Dec 3, 2025 at 4:10 AM Chaoyi Chen <chaoyi.chen@rock-chips.com> wrote:
> 
> > It seems that multiple drivers currently depend on the CRTC being
> > enabled and they implement the same atomic_commit_tail().
> >
> > Why not implement this in drm_atomic_helper_commit_tail_rpm() instead?
> > Or why not use another common helper function for this?
> 
> So my v2 version of the patch series added a new special case
> helper tail function to do that:
> https://lore.kernel.org/dri-devel/20251118-mcde-drm-regression-v2-3-4fedf10b18f6@linaro.org/
> 
> It was politely NACKed for complicating the helpers library (short story).

The longer story is that it wasn't NACKed at all, but if we want to do that:

- We need to figure out the bridge ordering mess in the first place

- Improve the commit_tail helpers doc to make it clearer what each are
  doing

- Probably change their name too, since the rpm suffix is a gross
  simplification of what is happening

- Create a few kunit tests to make sure whatever order they guarantee is
  properly implemented

- And maybe create a few helpers and convert drivers to that.

None of that have any place for a regression fix, so it wasn't a
reasonable ask.

Maxime

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v6 4/4] RFT: drm/rockchip: Create custom commit tail

[Linus Walleij]

On Fri, Dec 5, 2025 at 2:53 PM Maxime Ripard <mripard@kernel.org> wrote:

> - We need to figure out the bridge ordering mess in the first place

I was thinking about what we can do here, adding various flags was
discussed and deemed too kludgy.

What exists in the kernel are things such as the MMC power sequencer
which can be found in

drivers/mmc/core/pwrseq_simple.c
drivers/mmc/core/pwrseq_emmc.c
drivers/mmc/core/pwrseq_sd8787.c

with some DT bindings in

Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.yaml
Documentation/devicetree/bindings/mmc/mmc-pwrseq-emmc.yaml
Documentation/devicetree/bindings/mmc/mmc-pwrseq-sd8787.yaml

So here the approach is that the specific sequencing requirements
are added to the hardware description (the device tree) but there the
resources are really flat, then the driver for each type of sequence
takes care of the semantics, i.e. the actual sequencing and ordering.

Maybe we want to look into something like this?

Yours,
Linus Walleij

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v6 4/4] RFT: drm/rockchip: Create custom commit tail

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 1734 bytes --]

On Fri, Dec 05, 2025 at 03:11:06PM +0100, Linus Walleij wrote:
> On Fri, Dec 5, 2025 at 2:53 PM Maxime Ripard <mripard@kernel.org> wrote:
> 
> > - We need to figure out the bridge ordering mess in the first place
> 
> I was thinking about what we can do here, adding various flags was
> discussed and deemed too kludgy.
> 
> What exists in the kernel are things such as the MMC power sequencer
> which can be found in
> 
> drivers/mmc/core/pwrseq_simple.c
> drivers/mmc/core/pwrseq_emmc.c
> drivers/mmc/core/pwrseq_sd8787.c
> 
> with some DT bindings in
> 
> Documentation/devicetree/bindings/mmc/mmc-pwrseq-simple.yaml
> Documentation/devicetree/bindings/mmc/mmc-pwrseq-emmc.yaml
> Documentation/devicetree/bindings/mmc/mmc-pwrseq-sd8787.yaml
> 
> So here the approach is that the specific sequencing requirements
> are added to the hardware description (the device tree) but there the
> resources are really flat, then the driver for each type of sequence
> takes care of the semantics, i.e. the actual sequencing and ordering.
> 
> Maybe we want to look into something like this?

It's much more complicated than that, and it has nothing to do in the
device tree. The DSI bus has different power states, and DSI devices
have various requirements depending on the power state (in particular,
when you can send commands to the device).

Thus, there needs to be some synchronization between the DSI controller
driver and the DSI device driver to allow the device driver to perform
something in an expected power state.

It's been shoehorned into the bridge API (with pre_enable vs enable) and
hacked a bit with pre_enable_prev_first, but it really should be fixed
once and for all.

Maxime

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip