[PATCH v2 0/3] drm/atomic-helper: Fix atomic modesetting regression

[PATCH v2 0/3] drm/atomic-helper: Fix atomic modesetting regression

[Linus Walleij]

This fixes a regression experienced in the R-Car and MCDE 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.

Signed-off-by: Linus Walleij <linus.walleij@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 (2):
      drm/atomic-helper: Add disable CRTC late variant
      drm/atomic-helper: Add special quirk tail function

Marek Vasut (1):
      drm/atomic-helper: rcar-du: Enable CRTC early on R-Car DU

 drivers/gpu/drm/drm_atomic_helper.c           | 98 +++++++++++++++++++++++++--
 drivers/gpu/drm/mcde/mcde_drv.c               |  6 +-
 drivers/gpu/drm/renesas/rcar-du/rcar_du_kms.c |  2 +-
 include/drm/drm_atomic_helper.h               |  5 ++
 4 files changed, 103 insertions(+), 8 deletions(-)
---
base-commit: 6548d364a3e850326831799d7e3ea2d7bb97ba08
change-id: 20251118-mcde-drm-regression-33deb78a968f

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

[PATCH v2 1/3] drm/atomic-helper: rcar-du: Enable CRTC early on R-Car DU

[Linus Walleij]

From: Marek Vasut <marek.vasut+renesas@mailbox.org>

Introduce a variant of drm_atomic_helper_commit_modeset_enables()
which enables CRTC before encoder/bridge. 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. 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.

Fix this by restoring the enable ordering introduced in commit
c9b1150a68d9 ("drm/atomic-helper: Re-order bridge chain pre-enable
and post-disable"), to enable CRTC early.

Fixes: c9b1150a68d9 ("drm/atomic-helper: Re-order bridge chain pre-enable and post-disable")
Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
---
 drivers/gpu/drm/drm_atomic_helper.c           | 24 ++++++++++++++++++++++++
 drivers/gpu/drm/renesas/rcar-du/rcar_du_kms.c |  2 +-
 include/drm/drm_atomic_helper.h               |  2 ++
 3 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index d5ebe6ea0acb..f03b93c72b8f 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1692,6 +1692,30 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
 }
 EXPORT_SYMBOL(drm_atomic_helper_commit_modeset_enables);
 
+/**
+ * drm_atomic_helper_commit_modeset_enables_crtc_early - modeset commit to enable outputs, start CRTC early
+ * @dev: DRM device
+ * @state: atomic state object being committed
+ *
+ * This function is a variant of drm_atomic_helper_commit_modeset_enables()
+ * which enables CRTC before encoder/bridge. 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. DSI transfer.
+ */
+void
+drm_atomic_helper_commit_modeset_enables_crtc_early(struct drm_device *dev,
+						    struct drm_atomic_state *state)
+{
+	crtc_enable(dev, state);
+
+	encoder_bridge_pre_enable(dev, state);
+
+	encoder_bridge_enable(dev, state);
+
+	drm_atomic_helper_commit_writebacks(dev, state);
+}
+EXPORT_SYMBOL(drm_atomic_helper_commit_modeset_enables_crtc_early);
+
 /*
  * For atomic updates which touch just a single CRTC, calculate the time of the
  * next vblank, and inform all the fences of the deadline.
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..b2403be4436b 100644
--- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_kms.c
+++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_kms.c
@@ -544,7 +544,7 @@ static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state)
 	drm_atomic_helper_commit_modeset_disables(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);
+	drm_atomic_helper_commit_modeset_enables_crtc_early(dev, old_state);
 
 	drm_atomic_helper_commit_hw_done(old_state);
 	drm_atomic_helper_wait_for_flip_done(dev, old_state);
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
index 53382fe93537..d7fb473db343 100644
--- a/include/drm/drm_atomic_helper.h
+++ b/include/drm/drm_atomic_helper.h
@@ -93,6 +93,8 @@ void drm_atomic_helper_commit_modeset_disables(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);
+void drm_atomic_helper_commit_modeset_enables_crtc_early(struct drm_device *dev,
+							 struct drm_atomic_state *old_state);
 
 int drm_atomic_helper_prepare_planes(struct drm_device *dev,
 				     struct drm_atomic_state *state);

-- 
2.51.1

Re: [PATCH v2 1/3] drm/atomic-helper: rcar-du: Enable CRTC early on R-Car DU

[Laurent Pinchart]

Hi Linus, Marek,

Thank you for the patch.

On Tue, Nov 18, 2025 at 03:36:03PM +0100, Linus Walleij wrote:
> From: Marek Vasut <marek.vasut+renesas@mailbox.org>
> 
> Introduce a variant of drm_atomic_helper_commit_modeset_enables()
> which enables CRTC before encoder/bridge. 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. 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.
> 
> Fix this by restoring the enable ordering introduced in commit
> c9b1150a68d9 ("drm/atomic-helper: Re-order bridge chain pre-enable
> and post-disable"), to enable CRTC early.

This will need to be tested on Gen3 and Gen4 hardware, with different
types of output in addition to DSI. Unfortunately you're catching me at
a bad time as I'm about to board a plane and won't have access to test
hardware for a month :-/ We'll need volunteers.

> Fixes: c9b1150a68d9 ("drm/atomic-helper: Re-order bridge chain pre-enable and post-disable")
> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c           | 24 ++++++++++++++++++++++++
>  drivers/gpu/drm/renesas/rcar-du/rcar_du_kms.c |  2 +-
>  include/drm/drm_atomic_helper.h               |  2 ++
>  3 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index d5ebe6ea0acb..f03b93c72b8f 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1692,6 +1692,30 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_commit_modeset_enables);
>  
> +/**
> + * drm_atomic_helper_commit_modeset_enables_crtc_early - modeset commit to enable outputs, start CRTC early
> + * @dev: DRM device
> + * @state: atomic state object being committed
> + *
> + * This function is a variant of drm_atomic_helper_commit_modeset_enables()
> + * which enables CRTC before encoder/bridge. 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. DSI transfer.
> + */
> +void
> +drm_atomic_helper_commit_modeset_enables_crtc_early(struct drm_device *dev,
> +						    struct drm_atomic_state *state)
> +{
> +	crtc_enable(dev, state);
> +
> +	encoder_bridge_pre_enable(dev, state);
> +
> +	encoder_bridge_enable(dev, state);
> +
> +	drm_atomic_helper_commit_writebacks(dev, state);
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_commit_modeset_enables_crtc_early);
> +
>  /*
>   * For atomic updates which touch just a single CRTC, calculate the time of the
>   * next vblank, and inform all the fences of the deadline.
> 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..b2403be4436b 100644
> --- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_kms.c
> +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_kms.c
> @@ -544,7 +544,7 @@ static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state)
>  	drm_atomic_helper_commit_modeset_disables(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);
> +	drm_atomic_helper_commit_modeset_enables_crtc_early(dev, old_state);
>  
>  	drm_atomic_helper_commit_hw_done(old_state);
>  	drm_atomic_helper_wait_for_flip_done(dev, old_state);
> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> index 53382fe93537..d7fb473db343 100644
> --- a/include/drm/drm_atomic_helper.h
> +++ b/include/drm/drm_atomic_helper.h
> @@ -93,6 +93,8 @@ void drm_atomic_helper_commit_modeset_disables(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);
> +void drm_atomic_helper_commit_modeset_enables_crtc_early(struct drm_device *dev,
> +							 struct drm_atomic_state *old_state);
>  
>  int drm_atomic_helper_prepare_planes(struct drm_device *dev,
>  				     struct drm_atomic_state *state);

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 1/3] drm/atomic-helper: rcar-du: Enable CRTC early on R-Car DU

[Tomi Valkeinen]

Hi,

On 18/11/2025 16:50, Laurent Pinchart wrote:
> Hi Linus, Marek,
> 
> Thank you for the patch.
> 
> On Tue, Nov 18, 2025 at 03:36:03PM +0100, Linus Walleij wrote:
>> From: Marek Vasut <marek.vasut+renesas@mailbox.org>
>>
>> Introduce a variant of drm_atomic_helper_commit_modeset_enables()
>> which enables CRTC before encoder/bridge. 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. 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.
>>
>> Fix this by restoring the enable ordering introduced in commit
>> c9b1150a68d9 ("drm/atomic-helper: Re-order bridge chain pre-enable
>> and post-disable"), to enable CRTC early.
> 
> This will need to be tested on Gen3 and Gen4 hardware, with different
> types of output in addition to DSI. Unfortunately you're catching me at
> a bad time as I'm about to board a plane and won't have access to test
> hardware for a month :-/ We'll need volunteers.

I can do some tests on my whitehawk. But I'm not too worried, this is
just restoring the order we had earlier.

 Tomi

>> Fixes: c9b1150a68d9 ("drm/atomic-helper: Re-order bridge chain pre-enable and post-disable")
>> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
>> ---
>>  drivers/gpu/drm/drm_atomic_helper.c           | 24 ++++++++++++++++++++++++
>>  drivers/gpu/drm/renesas/rcar-du/rcar_du_kms.c |  2 +-
>>  include/drm/drm_atomic_helper.h               |  2 ++
>>  3 files changed, 27 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>> index d5ebe6ea0acb..f03b93c72b8f 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -1692,6 +1692,30 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
>>  }
>>  EXPORT_SYMBOL(drm_atomic_helper_commit_modeset_enables);
>>  
>> +/**
>> + * drm_atomic_helper_commit_modeset_enables_crtc_early - modeset commit to enable outputs, start CRTC early
>> + * @dev: DRM device
>> + * @state: atomic state object being committed
>> + *
>> + * This function is a variant of drm_atomic_helper_commit_modeset_enables()
>> + * which enables CRTC before encoder/bridge. 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. DSI transfer.
>> + */
>> +void
>> +drm_atomic_helper_commit_modeset_enables_crtc_early(struct drm_device *dev,
>> +						    struct drm_atomic_state *state)
>> +{
>> +	crtc_enable(dev, state);
>> +
>> +	encoder_bridge_pre_enable(dev, state);
>> +
>> +	encoder_bridge_enable(dev, state);
>> +
>> +	drm_atomic_helper_commit_writebacks(dev, state);
>> +}
>> +EXPORT_SYMBOL(drm_atomic_helper_commit_modeset_enables_crtc_early);
>> +
>>  /*
>>   * For atomic updates which touch just a single CRTC, calculate the time of the
>>   * next vblank, and inform all the fences of the deadline.
>> 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..b2403be4436b 100644
>> --- a/drivers/gpu/drm/renesas/rcar-du/rcar_du_kms.c
>> +++ b/drivers/gpu/drm/renesas/rcar-du/rcar_du_kms.c
>> @@ -544,7 +544,7 @@ static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state)
>>  	drm_atomic_helper_commit_modeset_disables(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);
>> +	drm_atomic_helper_commit_modeset_enables_crtc_early(dev, old_state);
>>  
>>  	drm_atomic_helper_commit_hw_done(old_state);
>>  	drm_atomic_helper_wait_for_flip_done(dev, old_state);
>> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
>> index 53382fe93537..d7fb473db343 100644
>> --- a/include/drm/drm_atomic_helper.h
>> +++ b/include/drm/drm_atomic_helper.h
>> @@ -93,6 +93,8 @@ void drm_atomic_helper_commit_modeset_disables(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);
>> +void drm_atomic_helper_commit_modeset_enables_crtc_early(struct drm_device *dev,
>> +							 struct drm_atomic_state *old_state);
>>  
>>  int drm_atomic_helper_prepare_planes(struct drm_device *dev,
>>  				     struct drm_atomic_state *state);
> 

[PATCH v2 2/3] drm/atomic-helper: Add disable CRTC late variant

[Linus Walleij]

This augments the previous patch that provide an alternate semantic
to enable the CRTC early adding a function to also disable the CRTC
late, essentially restoring the entire old sequencing if you
use both these helpers.

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

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index f03b93c72b8f..eb47883be153 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1337,13 +1337,17 @@ encoder_bridge_post_disable(struct drm_device *dev, struct drm_atomic_state *sta
 }
 
 static void
-disable_outputs(struct drm_device *dev, struct drm_atomic_state *state)
+disable_outputs(struct drm_device *dev, struct drm_atomic_state *state, bool late_crtc)
 {
 	encoder_bridge_disable(dev, state);
 
-	crtc_disable(dev, state);
-
-	encoder_bridge_post_disable(dev, state);
+	if (!late_crtc) {
+		crtc_disable(dev, state);
+		encoder_bridge_post_disable(dev, state);
+	} else {
+		encoder_bridge_post_disable(dev, state);
+		crtc_disable(dev, state);
+	}
 }
 
 /**
@@ -1526,7 +1530,7 @@ 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)
 {
-	disable_outputs(dev, state);
+	disable_outputs(dev, state, false);
 
 	drm_atomic_helper_update_legacy_modeset_state(dev, state);
 	drm_atomic_helper_calc_timestamping_constants(state);
@@ -1535,6 +1539,31 @@ void drm_atomic_helper_commit_modeset_disables(struct drm_device *dev,
 }
 EXPORT_SYMBOL(drm_atomic_helper_commit_modeset_disables);
 
+/**
+ * drm_atomic_helper_commit_modeset_disables_crtc_late - modeset disable outputs
+ * @dev: DRM device
+ * @state: atomic state object being committed
+ *
+ * This function shuts down all the outputs that need to be shut down with
+ * CRTC last in the disablement chain and prepares them (if required) with the
+ * new mode.
+ *
+ * This is a version of @drm_atomic_helper_commit_modeset_disables() that disables
+ * the CRTC last in the chain of disablement calls instead of the intuitive
+ * order to disable the bridges before the CRTC.
+ */
+void drm_atomic_helper_commit_modeset_disables_crtc_late(struct drm_device *dev,
+							 struct drm_atomic_state *state)
+{
+	disable_outputs(dev, state, true);
+
+	drm_atomic_helper_update_legacy_modeset_state(dev, state);
+	drm_atomic_helper_calc_timestamping_constants(state);
+
+	crtc_set_mode(dev, state);
+}
+EXPORT_SYMBOL(drm_atomic_helper_commit_modeset_disables_crtc_late);
+
 static void drm_atomic_helper_commit_writebacks(struct drm_device *dev,
 						struct drm_atomic_state *state)
 {
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
index d7fb473db343..d479afcd1637 100644
--- a/include/drm/drm_atomic_helper.h
+++ b/include/drm/drm_atomic_helper.h
@@ -91,6 +91,8 @@ drm_atomic_helper_calc_timestamping_constants(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_modeset_disables_crtc_late(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);
 void drm_atomic_helper_commit_modeset_enables_crtc_early(struct drm_device *dev,

-- 
2.51.1

[PATCH v2 3/3] drm/atomic-helper: Add special quirk tail function

[Linus Walleij]

This adds (yet another) variant of the
drm_atomic_helper_commit_tail_crtc_early_late() helper function
to deal with regressions caused by reordering the bridge
prepare/enablement sequence.

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.

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

This patch from Marek Vasut:
https://lore.kernel.org/all/20251107230517.471894-1-marek.vasut%2Brenesas%40mailbox.org/
solves part of the problem for drivers using custom
tail functions, since MCDE is using helpers only, we
add a new helper function that exploits the new
drm_atomic_helper_commit_modeset_enables_crtc_early()
and use that in MCDE.

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/drm_atomic_helper.c | 35 +++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/mcde/mcde_drv.c     |  6 ++++--
 include/drm/drm_atomic_helper.h     |  1 +
 3 files changed, 40 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index eb47883be153..23fa27f21c4e 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -2005,6 +2005,41 @@ void drm_atomic_helper_commit_tail_rpm(struct drm_atomic_state *state)
 }
 EXPORT_SYMBOL(drm_atomic_helper_commit_tail_rpm);
 
+/**
+ * drm_atomic_helper_commit_tail_crtc_early_late - commit atomic update
+ * @state: new modeset state to be committed
+ *
+ * This is an alternative implementation for the
+ * &drm_mode_config_helper_funcs.atomic_commit_tail hook, for drivers
+ * that support runtime_pm or need the CRTC to be enabled to perform a
+ * commit, and also need the CRTC to be enabled before preparing any
+ * bridges, as well as leaving the CRTC enabled while unpreparing
+ * any bridges.
+ *
+ * Otherwise, one should use the default implementation
+ * drm_atomic_helper_commit_tail().
+ */
+void drm_atomic_helper_commit_tail_crtc_early_late(struct drm_atomic_state *state)
+{
+	struct drm_device *dev = state->dev;
+
+	drm_atomic_helper_commit_modeset_disables_crtc_late(dev, state);
+
+	drm_atomic_helper_commit_modeset_enables_crtc_early(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);
+}
+EXPORT_SYMBOL(drm_atomic_helper_commit_tail_crtc_early_late);
+
 static void commit_tail(struct drm_atomic_state *state)
 {
 	struct drm_device *dev = state->dev;
diff --git a/drivers/gpu/drm/mcde/mcde_drv.c b/drivers/gpu/drm/mcde/mcde_drv.c
index 5f2c462bad7e..f3833d20c0fa 100644
--- a/drivers/gpu/drm/mcde/mcde_drv.c
+++ b/drivers/gpu/drm/mcde/mcde_drv.c
@@ -104,9 +104,11 @@ 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.
+	 * the case with e.g. DSI displays, and also make sure that the
+	 * CRTC is enabled before any bridges are prepared and disabled
+	 * after any bridges are disabled.
 	 */
-	.atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
+	.atomic_commit_tail = drm_atomic_helper_commit_tail_crtc_early_late,
 };
 
 static irqreturn_t mcde_irq(int irq, void *data)
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
index d479afcd1637..1e85df5eea4f 100644
--- a/include/drm/drm_atomic_helper.h
+++ b/include/drm/drm_atomic_helper.h
@@ -64,6 +64,7 @@ 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);
 void drm_atomic_helper_commit_tail_rpm(struct drm_atomic_state *state);
+void drm_atomic_helper_commit_tail_crtc_early_late(struct drm_atomic_state *state);
 int drm_atomic_helper_commit(struct drm_device *dev,
 			     struct drm_atomic_state *state,
 			     bool nonblock);

-- 
2.51.1

Re: [PATCH v2 3/3] drm/atomic-helper: Add special quirk tail function

[Laurent Pinchart]

Hi Linus,

Thank you for the patch.

On Tue, Nov 18, 2025 at 03:36:05PM +0100, Linus Walleij wrote:
> This adds (yet another) variant of the
> drm_atomic_helper_commit_tail_crtc_early_late() helper function
> to deal with regressions caused by reordering the bridge
> prepare/enablement sequence.
> 
> 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.
> 
> 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().
> 
> This patch from Marek Vasut:
> https://lore.kernel.org/all/20251107230517.471894-1-marek.vasut%2Brenesas%40mailbox.org/
> solves part of the problem for drivers using custom
> tail functions, since MCDE is using helpers only, we
> add a new helper function that exploits the new
> drm_atomic_helper_commit_modeset_enables_crtc_early()
> and use that in MCDE.
> 
> 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/drm_atomic_helper.c | 35 +++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/mcde/mcde_drv.c     |  6 ++++--
>  include/drm/drm_atomic_helper.h     |  1 +
>  3 files changed, 40 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index eb47883be153..23fa27f21c4e 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2005,6 +2005,41 @@ void drm_atomic_helper_commit_tail_rpm(struct drm_atomic_state *state)
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_commit_tail_rpm);
>  
> +/**
> + * drm_atomic_helper_commit_tail_crtc_early_late - commit atomic update

Based on the function name, it feels that the nem commit tail and
modeset enable/disable helpers reached a point where we may want to
reconsider the design instead of adding new functions with small
differences in behaviour that will end up confusing driver developers.

> + * @state: new modeset state to be committed
> + *
> + * This is an alternative implementation for the
> + * &drm_mode_config_helper_funcs.atomic_commit_tail hook, for drivers
> + * that support runtime_pm or need the CRTC to be enabled to perform a
> + * commit, and also need the CRTC to be enabled before preparing any
> + * bridges, as well as leaving the CRTC enabled while unpreparing
> + * any bridges.
> + *
> + * Otherwise, one should use the default implementation
> + * drm_atomic_helper_commit_tail().
> + */
> +void drm_atomic_helper_commit_tail_crtc_early_late(struct drm_atomic_state *state)
> +{
> +	struct drm_device *dev = state->dev;
> +
> +	drm_atomic_helper_commit_modeset_disables_crtc_late(dev, state);
> +
> +	drm_atomic_helper_commit_modeset_enables_crtc_early(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);
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_commit_tail_crtc_early_late);
> +
>  static void commit_tail(struct drm_atomic_state *state)
>  {
>  	struct drm_device *dev = state->dev;
> diff --git a/drivers/gpu/drm/mcde/mcde_drv.c b/drivers/gpu/drm/mcde/mcde_drv.c
> index 5f2c462bad7e..f3833d20c0fa 100644
> --- a/drivers/gpu/drm/mcde/mcde_drv.c
> +++ b/drivers/gpu/drm/mcde/mcde_drv.c
> @@ -104,9 +104,11 @@ 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.
> +	 * the case with e.g. DSI displays, and also make sure that the
> +	 * CRTC is enabled before any bridges are prepared and disabled
> +	 * after any bridges are disabled.
>  	 */
> -	.atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
> +	.atomic_commit_tail = drm_atomic_helper_commit_tail_crtc_early_late,
>  };
>  
>  static irqreturn_t mcde_irq(int irq, void *data)
> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> index d479afcd1637..1e85df5eea4f 100644
> --- a/include/drm/drm_atomic_helper.h
> +++ b/include/drm/drm_atomic_helper.h
> @@ -64,6 +64,7 @@ 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);
>  void drm_atomic_helper_commit_tail_rpm(struct drm_atomic_state *state);
> +void drm_atomic_helper_commit_tail_crtc_early_late(struct drm_atomic_state *state);
>  int drm_atomic_helper_commit(struct drm_device *dev,
>  			     struct drm_atomic_state *state,
>  			     bool nonblock);

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 3/3] drm/atomic-helper: Add special quirk tail function

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3265 bytes --]

On Tue, Nov 18, 2025 at 05:01:28PM +0200, Laurent Pinchart wrote:
> Hi Linus,
> 
> Thank you for the patch.
> 
> On Tue, Nov 18, 2025 at 03:36:05PM +0100, Linus Walleij wrote:
> > This adds (yet another) variant of the
> > drm_atomic_helper_commit_tail_crtc_early_late() helper function
> > to deal with regressions caused by reordering the bridge
> > prepare/enablement sequence.
> > 
> > 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.
> > 
> > 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().
> > 
> > This patch from Marek Vasut:
> > https://lore.kernel.org/all/20251107230517.471894-1-marek.vasut%2Brenesas%40mailbox.org/
> > solves part of the problem for drivers using custom
> > tail functions, since MCDE is using helpers only, we
> > add a new helper function that exploits the new
> > drm_atomic_helper_commit_modeset_enables_crtc_early()
> > and use that in MCDE.
> > 
> > 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/drm_atomic_helper.c | 35 +++++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/mcde/mcde_drv.c     |  6 ++++--
> >  include/drm/drm_atomic_helper.h     |  1 +
> >  3 files changed, 40 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index eb47883be153..23fa27f21c4e 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -2005,6 +2005,41 @@ void drm_atomic_helper_commit_tail_rpm(struct drm_atomic_state *state)
> >  }
> >  EXPORT_SYMBOL(drm_atomic_helper_commit_tail_rpm);
> >  
> > +/**
> > + * drm_atomic_helper_commit_tail_crtc_early_late - commit atomic update
> 
> Based on the function name, it feels that the nem commit tail and
> modeset enable/disable helpers reached a point where we may want to
> reconsider the design instead of adding new functions with small
> differences in behaviour that will end up confusing driver developers.

Agreed, and I'd go even further than that: we don't want every odd order
in the core. And if some driver has to break the order we document in
some way it should be very obvious.

If you have a single user for all these new helpers variants, you
shouldn't be using the helpers at all. Go with a driver specific
implementation.

Maxime

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

Re: [PATCH v2 3/3] drm/atomic-helper: Add special quirk tail function

[Linus Walleij]

On Tue, Nov 18, 2025 at 4:44 PM Maxime Ripard <mripard@kernel.org> wrote:
> On Tue, Nov 18, 2025 at 05:01:28PM +0200, Laurent Pinchart wrote:
> > On Tue, Nov 18, 2025 at 03:36:05PM +0100, Linus Walleij wrote:

> > > +/**
> > > + * drm_atomic_helper_commit_tail_crtc_early_late - commit atomic update
> >
> > Based on the function name, it feels that the nem commit tail and
> > modeset enable/disable helpers reached a point where we may want to
> > reconsider the design instead of adding new functions with small
> > differences in behaviour that will end up confusing driver developers.
>
> Agreed, and I'd go even further than that: we don't want every odd order
> in the core. And if some driver has to break the order we document in
> some way it should be very obvious.

Is this just a comment on this patch 3/3?

Or do you mean that Mareks new callback
drm_atomic_helper_commit_modeset_enables_crtc_early()
from patch 1/2 should go straight into the R-Car driver as well
and that
drm_atomic_helper_commit_modeset_disables_crtc_late()
patch 2/2 should also go into my driver, even if this
is a comment on patch 3/3?

Both patches 1 & 2 have a lot to do with ordering, this is
why I ask.

We already have
drm_atomic_helper_commit_tail()
drm_atomic_helper_commit_tail_rpm()

Does one more or less really matter? Maybe, I'm not sure,
but if it's just this one patch that is the problem I can surely
do it that way since we're only calling public functions.

Pushing the first two patches would be more problematic,
because they call a lot of functions that are local to the
drm atomic helpers.

Yours,
Linus Walleij

Re: [PATCH v2 3/3] drm/atomic-helper: Add special quirk tail function

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2280 bytes --]

On Tue, Nov 18, 2025 at 07:10:47PM +0100, Linus Walleij wrote:
> On Tue, Nov 18, 2025 at 4:44 PM Maxime Ripard <mripard@kernel.org> wrote:
> > On Tue, Nov 18, 2025 at 05:01:28PM +0200, Laurent Pinchart wrote:
> > > On Tue, Nov 18, 2025 at 03:36:05PM +0100, Linus Walleij wrote:
> 
> > > > +/**
> > > > + * drm_atomic_helper_commit_tail_crtc_early_late - commit atomic update
> > >
> > > Based on the function name, it feels that the nem commit tail and
> > > modeset enable/disable helpers reached a point where we may want to
> > > reconsider the design instead of adding new functions with small
> > > differences in behaviour that will end up confusing driver developers.
> >
> > Agreed, and I'd go even further than that: we don't want every odd order
> > in the core. And if some driver has to break the order we document in
> > some way it should be very obvious.
> 
> Is this just a comment on this patch 3/3?
> 
> Or do you mean that Mareks new callback
> drm_atomic_helper_commit_modeset_enables_crtc_early()
> from patch 1/2 should go straight into the R-Car driver as well
> and that
> drm_atomic_helper_commit_modeset_disables_crtc_late()
> patch 2/2 should also go into my driver, even if this
> is a comment on patch 3/3?
> 
> Both patches 1 & 2 have a lot to do with ordering, this is
> why I ask.

I mean, it applies to all your three patches and Marek's: helpers are
here to provide a default implementation. We shouldn't provide a default
implementation for a single user. All your patches enable to create
defaults for a single user.

So my point is that none of those functions should be helpers.

> We already have
> drm_atomic_helper_commit_tail()
> drm_atomic_helper_commit_tail_rpm()

The former has 5 users, the latter 13. And it's already confusing enough
and regression-prone as it is.

> Does one more or less really matter? Maybe, I'm not sure,
> but if it's just this one patch that is the problem I can surely
> do it that way since we're only calling public functions.
> 
> Pushing the first two patches would be more problematic,
> because they call a lot of functions that are local to the
> drm atomic helpers.

I'm totally fine with making more internal functions public though.

Maxime

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

Re: [PATCH v2 3/3] drm/atomic-helper: Add special quirk tail function

[Tomi Valkeinen]

Hi,

On 19/11/2025 11:19, Maxime Ripard wrote:
> On Tue, Nov 18, 2025 at 07:10:47PM +0100, Linus Walleij wrote:
>> On Tue, Nov 18, 2025 at 4:44 PM Maxime Ripard <mripard@kernel.org> wrote:
>>> On Tue, Nov 18, 2025 at 05:01:28PM +0200, Laurent Pinchart wrote:
>>>> On Tue, Nov 18, 2025 at 03:36:05PM +0100, Linus Walleij wrote:
>>
>>>>> +/**
>>>>> + * drm_atomic_helper_commit_tail_crtc_early_late - commit atomic update
>>>>
>>>> Based on the function name, it feels that the nem commit tail and
>>>> modeset enable/disable helpers reached a point where we may want to
>>>> reconsider the design instead of adding new functions with small
>>>> differences in behaviour that will end up confusing driver developers.
>>>
>>> Agreed, and I'd go even further than that: we don't want every odd order
>>> in the core. And if some driver has to break the order we document in
>>> some way it should be very obvious.
>>
>> Is this just a comment on this patch 3/3?
>>
>> Or do you mean that Mareks new callback
>> drm_atomic_helper_commit_modeset_enables_crtc_early()
>> from patch 1/2 should go straight into the R-Car driver as well
>> and that
>> drm_atomic_helper_commit_modeset_disables_crtc_late()
>> patch 2/2 should also go into my driver, even if this
>> is a comment on patch 3/3?
>>
>> Both patches 1 & 2 have a lot to do with ordering, this is
>> why I ask.
> 
> I mean, it applies to all your three patches and Marek's: helpers are
> here to provide a default implementation. We shouldn't provide a default
> implementation for a single user. All your patches enable to create
> defaults for a single user.

Two users so far: Renesas and ST-Ericsson.

> So my point is that none of those functions should be helpers.
> 
>> We already have
>> drm_atomic_helper_commit_tail()
>> drm_atomic_helper_commit_tail_rpm()
> 
> The former has 5 users, the latter 13. And it's already confusing enough
> and regression-prone as it is.
> 
>> Does one more or less really matter? Maybe, I'm not sure,
>> but if it's just this one patch that is the problem I can surely
>> do it that way since we're only calling public functions.
>>
>> Pushing the first two patches would be more problematic,
>> because they call a lot of functions that are local to the
>> drm atomic helpers.
> 
> I'm totally fine with making more internal functions public though.
While I generally agree with that, I still wonder if an implementation
in the core is better here. Perhaps a flag in struct drm_driver, instead
of new set of helpers.

Moving this to the driver would require (with a quick glance) exposing
the following functions:

crtc_enable
crtc_disable
crtc_set_mode
encoder_bridge_pre_enable
encoder_bridge_enable
encoder_bridge_disable
encoder_bridge_post_disable

Not impossible to expose, but making a private function public does
require work in validating the function for more general use, and adding
kernel docs.

Handling this in the core would act as documentation too, so instead of
the driver doing things in a different way "hidden" inside the driver,
it would be a standard quirk, clearly documented.

Also, I'm also not sure how rare this quirk is. In fact, I feel we're
missing ways to handle the enable/disable related issues in the core
framework. In these patches we're talking about the case where the SoC's
DSI host needs an incoming pclk to operate, and panels need to do
configuration before the video stream is enabled. But the exact same
problem could be present with an external DSI bridge, and then we can't
fix it in the crtc driver.

So the question becomes "does any component in the pipeline need the
video stream's clock to operate". But then, it doesn't help if the crtc
output is enabled early if any bridge in between does not also enable
its output early. So it all gets a bit complex.

And sometimes the clocks go backward: the entity on the downstream side
provides a clock backwards, to the source entity...

But I digress. I think initially we should just look for a clean fix for
the platforms affected:

- Add the implementation into the drivers?
- Add helpers to the core?
- Add a flag of some kind so the core can do the right thing?

I made a quick test with the flag approach, below. It's not many lines,
but... Ugh, it does feel like a hack.

> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index d5ebe6ea0acb..8225aae43e3b 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1341,9 +1341,13 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *state)
>  {
>         encoder_bridge_disable(dev, state);
>  
> -       crtc_disable(dev, state);
> +       if (!dev->driver->crtc_early_on)
> +               crtc_disable(dev, state);
>  
>         encoder_bridge_post_disable(dev, state);
> +
> +       if (dev->driver->crtc_early_on)
> +               crtc_disable(dev, state);
>  }
>  
>  /**
> @@ -1682,9 +1686,13 @@ 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)
>  {
> +       if (dev->driver->crtc_early_on)
> +               crtc_enable(dev, state);
> +
>         encoder_bridge_pre_enable(dev, state);
>  
> -       crtc_enable(dev, state);
> +       if (!dev->driver->crtc_early_on)
> +               crtc_enable(dev, state);
>  

 Tomi

Re: [PATCH v2 3/3] drm/atomic-helper: Add special quirk tail function

[Linus Walleij]

On Wed, Nov 19, 2025 at 11:41 AM Tomi Valkeinen
<tomi.valkeinen@ideasonboard.com> wrote:

> But I digress. I think initially we should just look for a clean fix for
> the platforms affected:
>
> - Add the implementation into the drivers?
> - Add helpers to the core?
> - Add a flag of some kind so the core can do the right thing?
>
> I made a quick test with the flag approach, below. It's not many lines,
> but... Ugh, it does feel like a hack.

I did something similar but didn't submit it because I had similar
feelings.

I still feel it's the lesser evil compared to reverting the offending
patch, somehow the core needs to be aware about hardware
behaviours/limitations, we can't keep pretending that the map
is a good approximation of reality if we instead push voluminous
quirks into different drivers.

Yours,
Linus Walleij

Re: [PATCH v2 3/3] drm/atomic-helper: Add special quirk tail function

[Laurent Pinchart]

On Wed, Nov 19, 2025 at 12:41:52PM +0200, Tomi Valkeinen wrote:
> On 19/11/2025 11:19, Maxime Ripard wrote:
> > On Tue, Nov 18, 2025 at 07:10:47PM +0100, Linus Walleij wrote:
> >> On Tue, Nov 18, 2025 at 4:44 PM Maxime Ripard <mripard@kernel.org> wrote:
> >>> On Tue, Nov 18, 2025 at 05:01:28PM +0200, Laurent Pinchart wrote:
> >>>> On Tue, Nov 18, 2025 at 03:36:05PM +0100, Linus Walleij wrote:
> >>
> >>>>> +/**
> >>>>> + * drm_atomic_helper_commit_tail_crtc_early_late - commit atomic update
> >>>>
> >>>> Based on the function name, it feels that the nem commit tail and
> >>>> modeset enable/disable helpers reached a point where we may want to
> >>>> reconsider the design instead of adding new functions with small
> >>>> differences in behaviour that will end up confusing driver developers.
> >>>
> >>> Agreed, and I'd go even further than that: we don't want every odd order
> >>> in the core. And if some driver has to break the order we document in
> >>> some way it should be very obvious.
> >>
> >> Is this just a comment on this patch 3/3?
> >>
> >> Or do you mean that Mareks new callback
> >> drm_atomic_helper_commit_modeset_enables_crtc_early()
> >> from patch 1/2 should go straight into the R-Car driver as well
> >> and that
> >> drm_atomic_helper_commit_modeset_disables_crtc_late()
> >> patch 2/2 should also go into my driver, even if this
> >> is a comment on patch 3/3?
> >>
> >> Both patches 1 & 2 have a lot to do with ordering, this is
> >> why I ask.
> > 
> > I mean, it applies to all your three patches and Marek's: helpers are
> > here to provide a default implementation. We shouldn't provide a default
> > implementation for a single user. All your patches enable to create
> > defaults for a single user.
> 
> Two users so far: Renesas and ST-Ericsson.

Only MCDE uses the new drm_atomic_helper_commit_tail_crtc_early_late()
function, while the new
drm_atomic_helper_commit_modeset_enables_crtc_early() helper is used
directly by R-Car DU to implement its commit tail handler, and by
drm_atomic_helper_commit_tail_crtc_early_late().

> > So my point is that none of those functions should be helpers.
> > 
> >> We already have
> >> drm_atomic_helper_commit_tail()
> >> drm_atomic_helper_commit_tail_rpm()
> > 
> > The former has 5 users, the latter 13. And it's already confusing enough
> > and regression-prone as it is.
> > 
> >> Does one more or less really matter? Maybe, I'm not sure,
> >> but if it's just this one patch that is the problem I can surely
> >> do it that way since we're only calling public functions.
> >>
> >> Pushing the first two patches would be more problematic,
> >> because they call a lot of functions that are local to the
> >> drm atomic helpers.
> > 
> > I'm totally fine with making more internal functions public though.
> 
> While I generally agree with that, I still wonder if an implementation
> in the core is better here. Perhaps a flag in struct drm_driver, instead
> of new set of helpers.
> 
> Moving this to the driver would require (with a quick glance) exposing
> the following functions:
> 
> crtc_enable
> crtc_disable
> crtc_set_mode
> encoder_bridge_pre_enable
> encoder_bridge_enable
> encoder_bridge_disable
> encoder_bridge_post_disable
> 
> Not impossible to expose, but making a private function public does
> require work in validating the function for more general use, and adding
> kernel docs.
> 
> Handling this in the core would act as documentation too, so instead of
> the driver doing things in a different way "hidden" inside the driver,
> it would be a standard quirk, clearly documented.
> 
> Also, I'm also not sure how rare this quirk is. In fact, I feel we're
> missing ways to handle the enable/disable related issues in the core
> framework. In these patches we're talking about the case where the SoC's
> DSI host needs an incoming pclk to operate, and panels need to do
> configuration before the video stream is enabled. But the exact same
> problem could be present with an external DSI bridge, and then we can't
> fix it in the crtc driver.
> 
> So the question becomes "does any component in the pipeline need the
> video stream's clock to operate". But then, it doesn't help if the crtc
> output is enabled early if any bridge in between does not also enable
> its output early. So it all gets a bit complex.

Are we getting close to a point where we all know the bridge model will
need to be reworked extensively, and everybody hopes someone else will
do it ? :-)

> And sometimes the clocks go backward: the entity on the downstream side
> provides a clock backwards, to the source entity...
> 
> But I digress. I think initially we should just look for a clean fix for
> the platforms affected:
> 
> - Add the implementation into the drivers?
> - Add helpers to the core?
> - Add a flag of some kind so the core can do the right thing?

drm_atomic_helper_commit_modeset_enables_crtc_early() would be more
cumbersome to implement manually in drivers as most of the functions it
calls are not exported. drm_atomic_helper_commit_tail_crtc_early_late()
shouldn't be difficult to implement in the MCDE driver.

> I made a quick test with the flag approach, below. It's not many lines,
> but... Ugh, it does feel like a hack.

Without seeing the code I can already imagine how this would feel like a
hack, so I agree not to go that way.

> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index d5ebe6ea0acb..8225aae43e3b 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -1341,9 +1341,13 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *state)
> >  {
> >         encoder_bridge_disable(dev, state);
> >  
> > -       crtc_disable(dev, state);
> > +       if (!dev->driver->crtc_early_on)
> > +               crtc_disable(dev, state);
> >  
> >         encoder_bridge_post_disable(dev, state);
> > +
> > +       if (dev->driver->crtc_early_on)
> > +               crtc_disable(dev, state);
> >  }
> >  
> >  /**
> > @@ -1682,9 +1686,13 @@ 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)
> >  {
> > +       if (dev->driver->crtc_early_on)
> > +               crtc_enable(dev, state);
> > +
> >         encoder_bridge_pre_enable(dev, state);
> >  
> > -       crtc_enable(dev, state);
> > +       if (!dev->driver->crtc_early_on)
> > +               crtc_enable(dev, state);
> >  

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 3/3] drm/atomic-helper: Add special quirk tail function

[Linus Walleij]

On Thu, Nov 20, 2025 at 3:45 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:

> > But I digress. I think initially we should just look for a clean fix for
> > the platforms affected:
> >
> > - Add the implementation into the drivers?
> > - Add helpers to the core?
> > - Add a flag of some kind so the core can do the right thing?
>
> drm_atomic_helper_commit_modeset_enables_crtc_early() would be more
> cumbersome to implement manually in drivers as most of the functions it
> calls are not exported. drm_atomic_helper_commit_tail_crtc_early_late()
> shouldn't be difficult to implement in the MCDE driver.

But the second patch patch adding
drm_atomic_helper_commit_modeset_disables_crtc_late()
would be needed for symmetry.

> > I made a quick test with the flag approach, below. It's not many lines,
> > but... Ugh, it does feel like a hack.
>
> Without seeing the code I can already imagine how this would feel like a
> hack, so I agree not to go that way.

It seems we cannot reach consensus on a regression fix that doesn't
require large re-architecturing, so I'll go ahead and propose a
revert instead, it's the only sensible thing. I hope nothing breaks
from the revert...

Yours,
Linus Walleij

Re: [PATCH v2 3/3] drm/atomic-helper: Add special quirk tail function

[Tomi Valkeinen]

Hi,

On 20/11/2025 16:07, Linus Walleij wrote:
> On Thu, Nov 20, 2025 at 3:45 AM Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> 
>>> But I digress. I think initially we should just look for a clean fix for
>>> the platforms affected:
>>>
>>> - Add the implementation into the drivers?
>>> - Add helpers to the core?
>>> - Add a flag of some kind so the core can do the right thing?
>>
>> drm_atomic_helper_commit_modeset_enables_crtc_early() would be more
>> cumbersome to implement manually in drivers as most of the functions it
>> calls are not exported. drm_atomic_helper_commit_tail_crtc_early_late()
>> shouldn't be difficult to implement in the MCDE driver.
> 
> But the second patch patch adding
> drm_atomic_helper_commit_modeset_disables_crtc_late()
> would be needed for symmetry.
> 
>>> I made a quick test with the flag approach, below. It's not many lines,
>>> but... Ugh, it does feel like a hack.
>>
>> Without seeing the code I can already imagine how this would feel like a
>> hack, so I agree not to go that way.

I didn't mean we shouldn't go that way. I think it's the best option so
far for a fix. The partial diff was included in the mail, so the bulk of
the code was there. Only part missing was adding the flag into the
struct and setting it in the driver.

> It seems we cannot reach consensus on a regression fix that doesn't
> require large re-architecturing, so I'll go ahead and propose a
> revert instead, it's the only sensible thing. I hope nothing breaks
> from the revert...
Well, that will break DSI and OLDI on a bunch of boards using TI SoCs,
so we might need to revert more than that patch. But if they were
already in v6.17, reverting would be a regression... And there are least
these:

f5b1819193667bf62c3c99d3921b9429997a14b2
5d91394f236167ac624b823820faf4aa928b889e

that fix the ordering for Mediatek and Samsung. So my guess is that
those also need to be reverted.

I really think we should just add the flag to resolve the regression. We
can then continue pondering on a design change to somehow manage the
bridges better.

 Tomi

Re: [PATCH v2 3/3] drm/atomic-helper: Add special quirk tail function

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 5709 bytes --]

On Wed, Nov 19, 2025 at 12:41:52PM +0200, Tomi Valkeinen wrote:
> Hi,
> 
> On 19/11/2025 11:19, Maxime Ripard wrote:
> > On Tue, Nov 18, 2025 at 07:10:47PM +0100, Linus Walleij wrote:
> >> On Tue, Nov 18, 2025 at 4:44 PM Maxime Ripard <mripard@kernel.org> wrote:
> >>> On Tue, Nov 18, 2025 at 05:01:28PM +0200, Laurent Pinchart wrote:
> >>>> On Tue, Nov 18, 2025 at 03:36:05PM +0100, Linus Walleij wrote:
> >>
> >>>>> +/**
> >>>>> + * drm_atomic_helper_commit_tail_crtc_early_late - commit atomic update
> >>>>
> >>>> Based on the function name, it feels that the nem commit tail and
> >>>> modeset enable/disable helpers reached a point where we may want to
> >>>> reconsider the design instead of adding new functions with small
> >>>> differences in behaviour that will end up confusing driver developers.
> >>>
> >>> Agreed, and I'd go even further than that: we don't want every odd order
> >>> in the core. And if some driver has to break the order we document in
> >>> some way it should be very obvious.
> >>
> >> Is this just a comment on this patch 3/3?
> >>
> >> Or do you mean that Mareks new callback
> >> drm_atomic_helper_commit_modeset_enables_crtc_early()
> >> from patch 1/2 should go straight into the R-Car driver as well
> >> and that
> >> drm_atomic_helper_commit_modeset_disables_crtc_late()
> >> patch 2/2 should also go into my driver, even if this
> >> is a comment on patch 3/3?
> >>
> >> Both patches 1 & 2 have a lot to do with ordering, this is
> >> why I ask.
> > 
> > I mean, it applies to all your three patches and Marek's: helpers are
> > here to provide a default implementation. We shouldn't provide a default
> > implementation for a single user. All your patches enable to create
> > defaults for a single user.
> 
> Two users so far: Renesas and ST-Ericsson.
> 
> > So my point is that none of those functions should be helpers.
> > 
> >> We already have
> >> drm_atomic_helper_commit_tail()
> >> drm_atomic_helper_commit_tail_rpm()
> > 
> > The former has 5 users, the latter 13. And it's already confusing enough
> > and regression-prone as it is.
> > 
> >> Does one more or less really matter? Maybe, I'm not sure,
> >> but if it's just this one patch that is the problem I can surely
> >> do it that way since we're only calling public functions.
> >>
> >> Pushing the first two patches would be more problematic,
> >> because they call a lot of functions that are local to the
> >> drm atomic helpers.
> > 
> > I'm totally fine with making more internal functions public though.
> While I generally agree with that, I still wonder if an implementation
> in the core is better here. Perhaps a flag in struct drm_driver, instead
> of new set of helpers.
> 
> Moving this to the driver would require (with a quick glance) exposing
> the following functions:
> 
> crtc_enable
> crtc_disable
> crtc_set_mode
> encoder_bridge_pre_enable
> encoder_bridge_enable
> encoder_bridge_disable
> encoder_bridge_post_disable
> 
> Not impossible to expose, but making a private function public does
> require work in validating the function for more general use, and adding
> kernel docs.

Those are pretty trivial to document though, compared to document how
the new variants differ from drm_atomic_helper_commit_tail() and
drm_atomic_helper_commit_tail_rpm(), and then validating that it does
indeed stay that way.

> Handling this in the core would act as documentation too, so instead of
> the driver doing things in a different way "hidden" inside the driver,
> it would be a standard quirk, clearly documented.

We've had the "let's not introduce helpers for a single user" rule for
like a decade at this point, because it simply doesn't scale. Plenty of
drivers have opted-out for very specific use-case already. I'm not sure
why we should create this precedent.

> Also, I'm also not sure how rare this quirk is. In fact, I feel we're
> missing ways to handle the enable/disable related issues in the core
> framework. In these patches we're talking about the case where the SoC's
> DSI host needs an incoming pclk to operate, and panels need to do
> configuration before the video stream is enabled. But the exact same
> problem could be present with an external DSI bridge, and then we can't
> fix it in the crtc driver.
> 
> So the question becomes "does any component in the pipeline need the
> video stream's clock to operate". But then, it doesn't help if the crtc
> output is enabled early if any bridge in between does not also enable
> its output early. So it all gets a bit complex.
> 
> And sometimes the clocks go backward: the entity on the downstream side
> provides a clock backwards, to the source entity...

Yes, you're right, this is why it's so fragile. Do you want to create
the test suite to check that all combinations are properly tested before
reworking the whole thing?

> But I digress. I think initially we should just look for a clean fix for
> the platforms affected:
> 
> - Add the implementation into the drivers?
> - Add helpers to the core?
> - Add a flag of some kind so the core can do the right thing?
> 
> I made a quick test with the flag approach, below. It's not many lines,
> but... Ugh, it does feel like a hack.

Because it is.

Really, I don't get it. I gave you a free pass to do whatever you wanted
in your driver. It doesn't add any maintenance burden on anyone. It
doesn't risk regressing other drivers in the process. It doesn't come
with any testing requirement. It doesn't even have to be reviewed by us,
really.

Why do you argue for a more bothersome (for everyone) solution?

Maxime

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

Re: [PATCH v2 3/3] drm/atomic-helper: Add special quirk tail function

[Tomi Valkeinen]

Hi,

On 20/11/2025 18:19, Maxime Ripard wrote:
> On Wed, Nov 19, 2025 at 12:41:52PM +0200, Tomi Valkeinen wrote:
>> Hi,
>>
>> On 19/11/2025 11:19, Maxime Ripard wrote:
>>> On Tue, Nov 18, 2025 at 07:10:47PM +0100, Linus Walleij wrote:
>>>> On Tue, Nov 18, 2025 at 4:44 PM Maxime Ripard <mripard@kernel.org> wrote:
>>>>> On Tue, Nov 18, 2025 at 05:01:28PM +0200, Laurent Pinchart wrote:
>>>>>> On Tue, Nov 18, 2025 at 03:36:05PM +0100, Linus Walleij wrote:
>>>>
>>>>>>> +/**
>>>>>>> + * drm_atomic_helper_commit_tail_crtc_early_late - commit atomic update
>>>>>>
>>>>>> Based on the function name, it feels that the nem commit tail and
>>>>>> modeset enable/disable helpers reached a point where we may want to
>>>>>> reconsider the design instead of adding new functions with small
>>>>>> differences in behaviour that will end up confusing driver developers.
>>>>>
>>>>> Agreed, and I'd go even further than that: we don't want every odd order
>>>>> in the core. And if some driver has to break the order we document in
>>>>> some way it should be very obvious.
>>>>
>>>> Is this just a comment on this patch 3/3?
>>>>
>>>> Or do you mean that Mareks new callback
>>>> drm_atomic_helper_commit_modeset_enables_crtc_early()
>>>> from patch 1/2 should go straight into the R-Car driver as well
>>>> and that
>>>> drm_atomic_helper_commit_modeset_disables_crtc_late()
>>>> patch 2/2 should also go into my driver, even if this
>>>> is a comment on patch 3/3?
>>>>
>>>> Both patches 1 & 2 have a lot to do with ordering, this is
>>>> why I ask.
>>>
>>> I mean, it applies to all your three patches and Marek's: helpers are
>>> here to provide a default implementation. We shouldn't provide a default
>>> implementation for a single user. All your patches enable to create
>>> defaults for a single user.
>>
>> Two users so far: Renesas and ST-Ericsson.
>>
>>> So my point is that none of those functions should be helpers.
>>>
>>>> We already have
>>>> drm_atomic_helper_commit_tail()
>>>> drm_atomic_helper_commit_tail_rpm()
>>>
>>> The former has 5 users, the latter 13. And it's already confusing enough
>>> and regression-prone as it is.
>>>
>>>> Does one more or less really matter? Maybe, I'm not sure,
>>>> but if it's just this one patch that is the problem I can surely
>>>> do it that way since we're only calling public functions.
>>>>
>>>> Pushing the first two patches would be more problematic,
>>>> because they call a lot of functions that are local to the
>>>> drm atomic helpers.
>>>
>>> I'm totally fine with making more internal functions public though.
>> While I generally agree with that, I still wonder if an implementation
>> in the core is better here. Perhaps a flag in struct drm_driver, instead
>> of new set of helpers.
>>
>> Moving this to the driver would require (with a quick glance) exposing
>> the following functions:
>>
>> crtc_enable
>> crtc_disable
>> crtc_set_mode
>> encoder_bridge_pre_enable
>> encoder_bridge_enable
>> encoder_bridge_disable
>> encoder_bridge_post_disable
>>
>> Not impossible to expose, but making a private function public does
>> require work in validating the function for more general use, and adding
>> kernel docs.
> 
> Those are pretty trivial to document though, compared to document how
> the new variants differ from drm_atomic_helper_commit_tail() and
> drm_atomic_helper_commit_tail_rpm(), and then validating that it does
> indeed stay that way.

I agree.

>> Handling this in the core would act as documentation too, so instead of
>> the driver doing things in a different way "hidden" inside the driver,
>> it would be a standard quirk, clearly documented.
> 
> We've had the "let's not introduce helpers for a single user" rule for
> like a decade at this point, because it simply doesn't scale. Plenty of
> drivers have opted-out for very specific use-case already. I'm not sure
> why we should create this precedent.

Ok.

>> Also, I'm also not sure how rare this quirk is. In fact, I feel we're
>> missing ways to handle the enable/disable related issues in the core
>> framework. In these patches we're talking about the case where the SoC's
>> DSI host needs an incoming pclk to operate, and panels need to do
>> configuration before the video stream is enabled. But the exact same
>> problem could be present with an external DSI bridge, and then we can't
>> fix it in the crtc driver.
>>
>> So the question becomes "does any component in the pipeline need the
>> video stream's clock to operate". But then, it doesn't help if the crtc
>> output is enabled early if any bridge in between does not also enable
>> its output early. So it all gets a bit complex.
>>
>> And sometimes the clocks go backward: the entity on the downstream side
>> provides a clock backwards, to the source entity...
> 
> Yes, you're right, this is why it's so fragile. Do you want to create
> the test suite to check that all combinations are properly tested before
> reworking the whole thing?

Yes, right after I finish rewriting V4L2 to fix the mistakes there! =)

>> But I digress. I think initially we should just look for a clean fix for
>> the platforms affected:
>>
>> - Add the implementation into the drivers?
>> - Add helpers to the core?
>> - Add a flag of some kind so the core can do the right thing?
>>
>> I made a quick test with the flag approach, below. It's not many lines,
>> but... Ugh, it does feel like a hack.
> 
> Because it is.
> 
> Really, I don't get it. I gave you a free pass to do whatever you wanted
> in your driver. It doesn't add any maintenance burden on anyone. It
> doesn't risk regressing other drivers in the process. It doesn't come
> with any testing requirement. It doesn't even have to be reviewed by us,
> really.
> 
> Why do you argue for a more bothersome (for everyone) solution?
I don't argue for it. I presented the easy-ish options I see to fix
this. I think there are valid reasons to have this, in a way or another,
in the core. But as I said, it feels like a hack so I'm not too happy
with it.

In any case, it makes sense to fix these in the respective drivers (with
some core functions exported). It will get the issue sorted out for now,
without needing elaborate reverts, and without hacking the core.

Linus, Marek, is this ok for you?

 Tomi

Re: [PATCH v2 3/3] drm/atomic-helper: Add special quirk tail function

[Linus Walleij]

On Thu, Nov 20, 2025 at 6:08 PM Tomi Valkeinen
<tomi.valkeinen@ideasonboard.com> wrote:

> In any case, it makes sense to fix these in the respective drivers (with
> some core functions exported). It will get the issue sorted out for now,
> without needing elaborate reverts, and without hacking the core.
>
> Linus, Marek, is this ok for you?

Yes sure, I'll take a stab at it and try to fix Marek's regression too.

I guess I should namespace the next exported helpers drm_*
and export them so that will be a separate patch.

Yours,
Linus Walleij