[PATCH v12 0/5] drm/atomic-helper: Re-order CRTC and Bridge ops

[PATCH v12 0/5] drm/atomic-helper: Re-order CRTC and Bridge ops

[Aradhya Bhatia]

Hello all,

This series re-orders the sequences in which the drm CRTC and the drm
Bridge get enabled and disabled with respect to each other.

The bridge pre_enable calls have been shifted before the crtc_enable and
the bridge post_disable calls have been shifted after the crtc_disable.

This has been done as per the definition of bridge pre_enable.
"The display pipe (i.e. clocks and timing signals) feeding this bridge will
not yet be running when this callback is called".

Since CRTC is also a source feeding the bridge, it should not be enabled
before the bridges in the pipeline are pre_enabled.

The original sequence. for display pipe enable looks like:

      crtc_enable

      bridge[n]_pre_enable
      ...
      bridge[1]_pre_enable

      encoder_enable

      bridge[1]_enable
      ...
      bridge[n]_enable

The sequence of enable after this patch-set will look like:

      bridge[n]_pre_enable
      ...
      bridge[1]_pre_enable

      crtc_enable
      encoder_enable

      bridge[1]_enable
      ...
      bridge[n]_enable


For the disable sequence, this is what the original looks like:

      bridge[n]_disable
      ...
      bridge[1]_disable

      encoder_disable

      bridge[1]_post_disable
      ...
      bridge[n]_post_disable

      crtc_disable
	        
This is what the disable sequence will be, after this series of patches:

      bridge[n]_disable
      ...
      bridge[1]_disable

      encoder_disable
      crtc_disable

      bridge[1]_post_disable
      ...
      bridge[n]_post_disable

This series further updates the bridge API definitions to accurately
reflect the updated scenario.

This series is a subset of its v11[0] which had 14 patches in the revision.
9 of those 14 patches (which were specific to the cdns-dsi bridge driver)
were merged[1].

Regards
Aradhya

---
* Note on checkpatch warning in patch 2/4 *
Patch 2/4 causes the checkpatch to flare up for 1 checkpatch 'check' -

CHECK: Lines should not end with a '('
#79: FILE: drivers/gpu/drm/drm_atomic_helper.c:1304:
+                       new_crtc_state = drm_atomic_get_new_crtc_state(

This patch is largely duplicating the original code, with minor differences to
perform different operations. This line of code pre-exists in the file and
have simply been duplicated. I have decided to keep it as is to maintain the
uniformity and the originally intended readability. Should perhaps a fix be
required, this patch/series is not the right place, and another patch can be
created to fix this across the whole file.

References:
[0]: Revision v11 of this series.
https://lore.kernel.org/all/20250329113925.68204-1-aradhya.bhatia@linux.dev/

[1]: Patches 1 through 9 getting merged.
https://lore.kernel.org/all/174335361171.2556605.12634785416741695829.b4-ty@oss.qualcomm.com/


---
Change Log:

  - Changes in v12:
    - Drop patches 1 through 9 since they have been merged.
    - Rebase onto newer drm-misc-next.
    - Re-word the patch 3/4, ("drm/bridge: Update the bridge enable/disable doc")
      to make it more readable.

  - Changes in v11:
    - Add patch v11:13/14 ("drm/bridge: Update the bridge enable/disable doc"),
      that updates the documentation about the order of the various bridge
      enable/disable hooks being called wrt the CRTC and encoder hooks.
    - Rebase on drm-misc-next instead of linux-next.
      As part of rebase, accommodate the following change:
      - Change patch v10:08/13 ("drm/bridge: cdns-dsi: Support atomic bridge
        APIs") to v11:08/13 ("drm/bridge: cdns-dsi: Add input format
        negotiation"), since Maxime has already updated the bridge hooks to
        their atomic versions in commit 68c98e227a96 ("drm/bridge: cdns-csi:
        Switch to atomic helpers").
        My new patch now only adds the format negotiation hook for the cdns-dsi.
        (Note: Since the new patch is now only a subset of the old one, without
        any change in logic, I decided to carry forward the R-b and T-b tags.)
    - Add Alexander Sverdlin's T-b in patches 10, 11, 12.

  - Changes in v10:
    - Rebase on latest linux-next (next-20250226).
    - As part of rebase, update the patches to accommodate a couple of
      widespread changes in DRM Framework -
        - All the ("drm/atomic-helper: Change parameter name of ***") commits.
        - All the ("drm/bridge: Pass full state to ***") commits.
      (These updates are only trivial substitutions.)
    - Add Tomi Valkeinen's T-b tags in all the patches.

  - Changes in v9:
    - Fix the oops in 11/13 - where the encoder_bridge_enable _was_ pre_enabling
      the bridges instead of enabling.
    - Add the following tags:
      - Dmitry Baryshkov's R-b in patches 2, 10, 11, and A-b in patch 12.
      - Jayesh Choudhary's R-b in patch 12.
      - Tomi Valkeinen's R-b in patches 2, 10, 11, 12.

  - Changes in v8:
    - Move the phy de-initialization to bridge post_disable() instead of bridge
      disable() in patch-3.
    - Copy the private bridge state (dsi_cfg), in addition to the bridge_state,
      in patch-9.
    - Split patch v7:11/12 into three patches, v8:{10,11,12}/13, to separate out
      different refactorings into different patches, and improve bisectability.
    - Move patch v7:02/12 down to v8:06/12, to keep the initial patches for
      fixes only.
    - Drop patch v7:04/12 as it doesn't become relevant until patch v7:12/12.
    - Add R-b tags of Dmitry Baryshkov in patch-9 and patch-3, and of
      Tomi Valkeinen in patch-9.
   
  - Changes in v7:
    - phy_init()/exit() were called from the PM path in v6. Change it back to
      the bridge enable/disable path in patch-3, so that the phy_init() can go
      back to being called after D-Phy reset assert.
    - Reword commit text in patch-5 to explain the need of the fix.
    - Drop the stray code in patch-10.
    - Add R-b tag of Dmitry Baryshkov in patch-6.

  - Changes in v6:
    - Reword patch 3 to better explain the fixes around phy de-init.
    - Fix the Lane ready timeout condition in patch 7.
    - Fix the dsi _bridge_atomic_check() implementation by adding a new
      bridge state structure in patch 10.
    - Rework and combine patches v5:11/13 and v5:12/13 to v6:11/12.
    - Generate the patches of these series using the "patience" algorithm.
      Note: All patches, except v6:11/12, *do not* differ from their default
      (greedy) algorithm variants.
      For patch 11, the patience algorithm significantly improves the readability.
    - Rename and move the Bridge enable/disable enums from public to private
      in patch 11.
    - Add R-b tags of Tomi Valkeinen in patch 6, and Dmitry Baryshkov in patch 2.

  - Changes in v5:
    - Fix subject and description in patch 1/13.
    - Add patch to check the return value of
      phy_mipi_dphy_get_default_config() (patch: 6/13).
    - Change the Clk and Data Lane ready timeout from forever to 5s.
    - Print an error instead of calling WARN_ON_ONCE in patch 7/13.
    - Drop patch v4-07/11: "drm/bridge: cdns-dsi: Reset the DCS write FIFO".
      There has been some inconsistencies found with this patch upon further
      testing. This patch was being used to enable a DSI panel based on ILITEK
      ILI9881C bridge. This will be debugged separately.
    - Add patch to move the DSI mode check from _atomic_enable() to
      _atomic_check() (patch: 10/13).
    - Split patch v4-10/11 into 2 patches - 11/13 and 12/13.
      Patch 11/13 separates out the Encoder-Bridge operations into a helper
      function *without* changing the logic. Patch 12/13 then changes the order
      of the encoder-bridge operations as was intended in the original patch.
    - Add detailed comment for patch 13/13.
    - Add Tomi Valkeinen's R-b in patches 1, 2, 4, 5, 7, 8, 9, 13.

  - Changes in v4:
    - Add new patch, "drm/bridge: cdns-dsi: Move to devm_drm_of_get_bridge()",
      to update to an auto-managed way of finding next bridge in the chain.
    - Drop patch "drm/bridge: cdns-dsi: Fix the phy_initialized variable" and
      add "drm/bridge: cdns-dsi: Fix Phy _init() and _exit()" that properly
      de-initializes the Phy and maintains the initialization state.
    - Reword patch "drm/bridge: cdns-dsi: Reset the DCS write FIFO" to explain
      the HW concerns better.
    - Add R-b tag from Dmitry Baryshkov for patches 1/11 and 8/11.

  - Changes in v3:
    - Reword the commit message for patch "drm/bridge: cdns-dsi: Fix OF node
      pointer".
    - Add a new helper API to figure out DSI host input pixel format
      in patch "drm/mipi-dsi: Add helper to find input format".
    - Use a common function for bridge pre-enable and enable, and bridge disable
      and post-disable, to avoid code duplication.
    - Add T-b tag from Dominik Haller in patch 5/10. (Missed to add it in v2).
    - Add R-b tag from Dmitry Baryshkov for patch 8/10.

  - Changes in v2:
    - Drop patch "drm/tidss: Add CRTC mode_fixup"
    - Split patch "drm/bridge: cdns-dsi: Fix minor bugs" into 4 separate ones
    - Drop support for early_enable/late_disable APIs and instead re-order the
      pre_enable / post_disable APIs to be called before / after crtc_enable /
      crtc_disable.
    - Drop support for early_enable/late_disable in cdns-dsi and use
      pre_enable/post_disable APIs instead to do bridge enable/disable.


Previous versions:

v1:  https://lore.kernel.org/all/20240511153051.1355825-1-a-bhatia1@ti.com/
v2:  https://lore.kernel.org/all/20240530093621.1925863-1-a-bhatia1@ti.com/
v3:  https://lore.kernel.org/all/20240617105311.1587489-1-a-bhatia1@ti.com/
v4:  https://lore.kernel.org/all/20240622110929.3115714-1-a-bhatia1@ti.com/
v5:  https://lore.kernel.org/all/20241019195411.266860-1-aradhya.bhatia@linux.dev/
v6:  https://lore.kernel.org/all/20250111192738.308889-1-aradhya.bhatia@linux.dev/
v7:  https://lore.kernel.org/all/20250114055626.18816-1-aradhya.bhatia@linux.dev/
v8:  https://lore.kernel.org/all/20250126191551.741957-1-aradhya.bhatia@linux.dev/
v9:  https://lore.kernel.org/all/20250209121032.32655-1-aradhya.bhatia@linux.dev/
v10: https://lore.kernel.org/all/20250226155228.564289-1-aradhya.bhatia@linux.dev/
v11: https://lore.kernel.org/all/20250329113925.68204-1-aradhya.bhatia@linux.dev/

---

Aradhya Bhatia (5):
  drm/atomic-helper: Refactor crtc & encoder-bridge op loops into
    separate functions
  drm/atomic-helper: Separate out bridge pre_enable/post_disable from
    enable/disable
  drm/atomic-helper: Re-order bridge chain pre-enable and post-disable
  drm/bridge: Update the bridge enable/disable doc
  drm/bridge: cdns-dsi: Use pre_enable/post_disable to enable/disable

 .../gpu/drm/bridge/cadence/cdns-dsi-core.c    |  64 +++--
 drivers/gpu/drm/drm_atomic_helper.c           | 160 +++++++++--
 include/drm/drm_bridge.h                      | 249 +++++++++++++-----
 3 files changed, 355 insertions(+), 118 deletions(-)


base-commit: dd717762761807452ca25634652e180a80349cd8
-- 
2.34.1

[PATCH v12 1/5] drm/atomic-helper: Refactor crtc & encoder-bridge op loops into separate functions

[Aradhya Bhatia]

From: Aradhya Bhatia <a-bhatia1@ti.com>

The way any singular display pipeline, in need of a modeset, gets
enabled is as follows -

	crtc enable
	(all) bridge pre-enable
	encoder enable
	(all) bridge enable

- and the disable sequence is exactly the reverse of this.

The crtc operations occur by looping over the old and new crtc states,
while the encoder and bridge operations occur together, by looping over
the connector states of the display pipelines.

Refactor these operations - crtc enable/disable, and encoder & bridge
(pre/post) enable/disable - into separate functions each, to make way
for the re-ordering of the enable/disable sequences.

This patch doesn't alter the sequence of crtc/encoder/bridge operations
in any way, but helps to cleanly pave the way for the next two patches,
by maintaining logical bisectability.

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Tested-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Tested-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
Signed-off-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>
---
 drivers/gpu/drm/drm_atomic_helper.c | 69 ++++++++++++++++++++---------
 1 file changed, 49 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index ee64ca1b1bec..d185486071c5 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1160,11 +1160,10 @@ crtc_needs_disable(struct drm_crtc_state *old_state,
 }
 
 static void
-disable_outputs(struct drm_device *dev, struct drm_atomic_state *state)
+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;
-	struct drm_crtc *crtc;
 	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
 	int i;
 
@@ -1227,6 +1226,14 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *state)
 
 		drm_atomic_bridge_chain_post_disable(bridge, state);
 	}
+}
+
+static void
+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;
+	int i;
 
 	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
 		const struct drm_crtc_helper_funcs *funcs;
@@ -1274,6 +1281,14 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *state)
 	}
 }
 
+static void
+disable_outputs(struct drm_device *dev, struct drm_atomic_state *state)
+{
+	encoder_bridge_disable(dev, state);
+
+	crtc_disable(dev, state);
+}
+
 /**
  * drm_atomic_helper_update_legacy_modeset_state - update legacy modeset state
  * @dev: DRM device
@@ -1483,28 +1498,12 @@ static void drm_atomic_helper_commit_writebacks(struct drm_device *dev,
 	}
 }
 
-/**
- * drm_atomic_helper_commit_modeset_enables - modeset commit to enable outputs
- * @dev: DRM device
- * @state: atomic state object being committed
- *
- * This function enables all the outputs with the new configuration which had to
- * be turned off for the update.
- *
- * For compatibility with legacy CRTC helpers this should be called after
- * drm_atomic_helper_commit_planes(), which is what the default commit function
- * does. But drivers with different needs can group the modeset commits together
- * and do the plane commits at the end. This is useful for drivers doing runtime
- * PM since planes updates then only happen when the CRTC is actually enabled.
- */
-void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
-					      struct drm_atomic_state *state)
+static void
+crtc_enable(struct drm_device *dev, struct drm_atomic_state *state)
 {
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *old_crtc_state;
 	struct drm_crtc_state *new_crtc_state;
-	struct drm_connector *connector;
-	struct drm_connector_state *new_conn_state;
 	int i;
 
 	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
@@ -1528,6 +1527,14 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
 				funcs->commit(crtc);
 		}
 	}
+}
+
+static void
+encoder_bridge_enable(struct drm_device *dev, struct drm_atomic_state *state)
+{
+	struct drm_connector *connector;
+	struct drm_connector_state *new_conn_state;
+	int i;
 
 	for_each_new_connector_in_state(state, connector, new_conn_state, i) {
 		const struct drm_encoder_helper_funcs *funcs;
@@ -1565,6 +1572,28 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
 
 		drm_atomic_bridge_chain_enable(bridge, state);
 	}
+}
+
+/**
+ * drm_atomic_helper_commit_modeset_enables - modeset commit to enable outputs
+ * @dev: DRM device
+ * @state: atomic state object being committed
+ *
+ * This function enables all the outputs with the new configuration which had to
+ * be turned off for the update.
+ *
+ * For compatibility with legacy CRTC helpers this should be called after
+ * drm_atomic_helper_commit_planes(), which is what the default commit function
+ * does. But drivers with different needs can group the modeset commits together
+ * and do the plane commits at the end. This is useful for drivers doing runtime
+ * PM since planes updates then only happen when the CRTC is actually enabled.
+ */
+void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
+					      struct drm_atomic_state *state)
+{
+	crtc_enable(dev, state);
+
+	encoder_bridge_enable(dev, state);
 
 	drm_atomic_helper_commit_writebacks(dev, state);
 }
-- 
2.34.1

[PATCH v12 2/5] drm/atomic-helper: Separate out bridge pre_enable/post_disable from enable/disable

[Aradhya Bhatia]

The encoder-bridge ops occur by looping over the new connector states of
the display pipelines. The enable sequence runs as follows -

	- pre_enable(bridge),
	- enable(encoder),
	- enable(bridge),

while the disable sequnce runs as follows -

	- disable(bridge),
	- disable(encoder),
	- post_disable(bridge).

Separate out the pre_enable(bridge), and the post_disable(bridge)
operations into separate functions each.

This patch keeps the sequence same for any singular disaplay pipe, but
changes the sequence across multiple display pipelines.

This patch is meant to be an interim patch, to cleanly pave the way for
the sequence re-ordering patch, and maintain bisectability in the
process.

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Reviewed-by: Jayesh Choudhary <j-choudhary@ti.com>
Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Tested-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Tested-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
Signed-off-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>
---
 drivers/gpu/drm/drm_atomic_helper.c | 91 ++++++++++++++++++++++++++++-
 1 file changed, 88 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index d185486071c5..86824f769623 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1223,8 +1223,6 @@ encoder_bridge_disable(struct drm_device *dev, struct drm_atomic_state *state)
 			else if (funcs->dpms)
 				funcs->dpms(encoder, DRM_MODE_DPMS_OFF);
 		}
-
-		drm_atomic_bridge_chain_post_disable(bridge, state);
 	}
 }
 
@@ -1281,11 +1279,65 @@ crtc_disable(struct drm_device *dev, struct drm_atomic_state *state)
 	}
 }
 
+static void
+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;
+	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
+	int i;
+
+	for_each_oldnew_connector_in_state(state, connector, old_conn_state, new_conn_state, i) {
+		struct drm_encoder *encoder;
+		struct drm_bridge *bridge;
+
+		/*
+		 * Shut down everything that's in the changeset and currently
+		 * still on. So need to check the old, saved state.
+		 */
+		if (!old_conn_state->crtc)
+			continue;
+
+		old_crtc_state = drm_atomic_get_old_crtc_state(state, old_conn_state->crtc);
+
+		if (new_conn_state->crtc)
+			new_crtc_state = drm_atomic_get_new_crtc_state(
+						state,
+						new_conn_state->crtc);
+		else
+			new_crtc_state = NULL;
+
+		if (!crtc_needs_disable(old_crtc_state, new_crtc_state) ||
+		    !drm_atomic_crtc_needs_modeset(old_conn_state->crtc->state))
+			continue;
+
+		encoder = old_conn_state->best_encoder;
+
+		/* We shouldn't get this far if we didn't previously have
+		 * an encoder.. but WARN_ON() rather than explode.
+		 */
+		if (WARN_ON(!encoder))
+			continue;
+
+		drm_dbg_atomic(dev, "post-disabling bridges [ENCODER:%d:%s]\n",
+			       encoder->base.id, encoder->name);
+
+		/*
+		 * Each encoder has at most one connector (since we always steal
+		 * it away), so we won't call disable hooks twice.
+		 */
+		bridge = drm_bridge_chain_get_first_bridge(encoder);
+		drm_atomic_bridge_chain_post_disable(bridge, state);
+	}
+}
+
 static void
 disable_outputs(struct drm_device *dev, struct drm_atomic_state *state)
 {
 	encoder_bridge_disable(dev, state);
 
+	encoder_bridge_post_disable(dev, state);
+
 	crtc_disable(dev, state);
 }
 
@@ -1498,6 +1550,38 @@ static void drm_atomic_helper_commit_writebacks(struct drm_device *dev,
 	}
 }
 
+static void
+encoder_bridge_pre_enable(struct drm_device *dev, struct drm_atomic_state *state)
+{
+	struct drm_connector *connector;
+	struct drm_connector_state *new_conn_state;
+	int i;
+
+	for_each_new_connector_in_state(state, connector, new_conn_state, i) {
+		struct drm_encoder *encoder;
+		struct drm_bridge *bridge;
+
+		if (!new_conn_state->best_encoder)
+			continue;
+
+		if (!new_conn_state->crtc->state->active ||
+		    !drm_atomic_crtc_needs_modeset(new_conn_state->crtc->state))
+			continue;
+
+		encoder = new_conn_state->best_encoder;
+
+		drm_dbg_atomic(dev, "pre-enabling bridges [ENCODER:%d:%s]\n",
+			       encoder->base.id, encoder->name);
+
+		/*
+		 * Each encoder has at most one connector (since we always steal
+		 * it away), so we won't call enable hooks twice.
+		 */
+		bridge = drm_bridge_chain_get_first_bridge(encoder);
+		drm_atomic_bridge_chain_pre_enable(bridge, state);
+	}
+}
+
 static void
 crtc_enable(struct drm_device *dev, struct drm_atomic_state *state)
 {
@@ -1559,7 +1643,6 @@ encoder_bridge_enable(struct drm_device *dev, struct drm_atomic_state *state)
 		 * it away), so we won't call enable hooks twice.
 		 */
 		bridge = drm_bridge_chain_get_first_bridge(encoder);
-		drm_atomic_bridge_chain_pre_enable(bridge, state);
 
 		if (funcs) {
 			if (funcs->atomic_enable)
@@ -1593,6 +1676,8 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
 {
 	crtc_enable(dev, state);
 
+	encoder_bridge_pre_enable(dev, state);
+
 	encoder_bridge_enable(dev, state);
 
 	drm_atomic_helper_commit_writebacks(dev, state);
-- 
2.34.1

[PATCH v12 3/5] drm/atomic-helper: Re-order bridge chain pre-enable and post-disable

[Aradhya Bhatia]

Move the bridge pre_enable call before crtc enable, and the bridge
post_disable call after the crtc disable.

The sequence of enable after this patch will look like:

	bridge[n]_pre_enable
	...
	bridge[1]_pre_enable

	crtc_enable
	encoder_enable

	bridge[1]_enable
	...
	bridge[n]_enable

And, the disable sequence for the display pipeline will look like:

	bridge[n]_disable
	...
	bridge[1]_disable

	encoder_disable
	crtc_disable

	bridge[1]_post_disable
	...
	bridge[n]_post_disable

The definition of bridge pre_enable hook says that,
"The display pipe (i.e. clocks and timing signals) feeding this bridge
will not yet be running when this callback is called".

Since CRTC is also a source feeding the bridge, it should not be enabled
before the bridges in the pipeline are pre_enabled. Fix that by
re-ordering the sequence of bridge pre_enable and bridge post_disable.

Acked-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Tested-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Tested-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
Signed-off-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>
---
 drivers/gpu/drm/drm_atomic_helper.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 86824f769623..db5aae15e75d 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1336,9 +1336,9 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *state)
 {
 	encoder_bridge_disable(dev, state);
 
-	encoder_bridge_post_disable(dev, state);
-
 	crtc_disable(dev, state);
+
+	encoder_bridge_post_disable(dev, state);
 }
 
 /**
@@ -1674,10 +1674,10 @@ encoder_bridge_enable(struct drm_device *dev, struct drm_atomic_state *state)
 void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
 					      struct drm_atomic_state *state)
 {
-	crtc_enable(dev, state);
-
 	encoder_bridge_pre_enable(dev, state);
 
+	crtc_enable(dev, state);
+
 	encoder_bridge_enable(dev, state);
 
 	drm_atomic_helper_commit_writebacks(dev, state);
-- 
2.34.1

[PATCH v12 4/5] drm/bridge: Update the bridge enable/disable doc

[Aradhya Bhatia]

Now that the bridges get pre-enabled before the CRTC is enabled, and get
post-disabled after the CRTC is disabled, update the function
descriptions to accurately reflect the updated scenario.

The enable sequence for the display pipeline looks like:

	bridge[n]_pre_enable
	...
	bridge[1]_pre_enable

	crtc_enable
	encoder_enable

	bridge[1]_enable
	...
	bridge[n]_enable

And, the disable sequence for the display pipeline looks like:

	bridge[n]_disable
	...
	bridge[1]_disable

	encoder_disable
	crtc_disable

	bridge[1]_post_disable
	...
	bridge[n]_post_disable

Signed-off-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>
---
 include/drm/drm_bridge.h | 249 ++++++++++++++++++++++++++++-----------
 1 file changed, 183 insertions(+), 66 deletions(-)

diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index cdad3b78a195..e5ccbefa60a8 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -164,17 +164,33 @@ struct drm_bridge_funcs {
 	/**
 	 * @disable:
 	 *
-	 * This callback should disable the bridge. It is called right before
-	 * the preceding element in the display pipe is disabled. If the
-	 * preceding element is a bridge this means it's called before that
-	 * bridge's @disable vfunc. If the preceding element is a &drm_encoder
-	 * it's called right before the &drm_encoder_helper_funcs.disable,
-	 * &drm_encoder_helper_funcs.prepare or &drm_encoder_helper_funcs.dpms
-	 * hook.
+	 * The @disable callback should disable the bridge.
 	 *
 	 * The bridge can assume that the display pipe (i.e. clocks and timing
 	 * signals) feeding it is still running when this callback is called.
 	 *
+	 *
+	 * If the preceding element is a &drm_bridge, then this is called before
+	 * that bridge is disabled via one of:
+	 *
+	 * - &drm_bridge_funcs.disable
+	 * - &drm_bridge_funcs.atomic_disable
+	 *
+	 * If the preceding element of the bridge is a display controller, then
+	 * this callback is called before the encoder is disabled via one of:
+	 *
+	 * - &drm_encoder_helper_funcs.atomic_disable
+	 * - &drm_encoder_helper_funcs.prepare
+	 * - &drm_encoder_helper_funcs.disable
+	 * - &drm_encoder_helper_funcs.dpms
+	 *
+	 * and the CRTC is disabled via one of:
+	 *
+	 * - &drm_crtc_helper_funcs.prepare
+	 * - &drm_crtc_helper_funcs.atomic_disable
+	 * - &drm_crtc_helper_funcs.disable
+	 * - &drm_crtc_helper_funcs.dpms.
+	 *
 	 * The @disable callback is optional.
 	 *
 	 * NOTE:
@@ -187,17 +203,34 @@ struct drm_bridge_funcs {
 	/**
 	 * @post_disable:
 	 *
-	 * This callback should disable the bridge. It is called right after the
-	 * preceding element in the display pipe is disabled. If the preceding
-	 * element is a bridge this means it's called after that bridge's
-	 * @post_disable function. If the preceding element is a &drm_encoder
-	 * it's called right after the encoder's
-	 * &drm_encoder_helper_funcs.disable, &drm_encoder_helper_funcs.prepare
-	 * or &drm_encoder_helper_funcs.dpms hook.
-	 *
 	 * The bridge must assume that the display pipe (i.e. clocks and timing
-	 * signals) feeding it is no longer running when this callback is
-	 * called.
+	 * signals) feeding this bridge is no longer running when the
+	 * @post_disable is called.
+	 *
+	 * This callback should perform all the actions required by the hardware
+	 * after it has stopped receiving signals from the preceding element.
+	 *
+	 * If the preceding element is a &drm_bridge, then this is called after
+	 * that bridge is post-disabled (unless marked otherwise by the
+	 * @pre_enable_prev_first flag) via one of:
+	 *
+	 * - &drm_bridge_funcs.post_disable
+	 * - &drm_bridge_funcs.atomic_post_disable
+	 *
+	 * If the preceding element of the bridge is a display controller, then
+	 * this callback is called after the encoder is disabled via one of:
+	 *
+	 * - &drm_encoder_helper_funcs.atomic_disable
+	 * - &drm_encoder_helper_funcs.prepare
+	 * - &drm_encoder_helper_funcs.disable
+	 * - &drm_encoder_helper_funcs.dpms
+	 *
+	 * and the CRTC is disabled via one of:
+	 *
+	 * - &drm_crtc_helper_funcs.prepare
+	 * - &drm_crtc_helper_funcs.atomic_disable
+	 * - &drm_crtc_helper_funcs.disable
+	 * - &drm_crtc_helper_funcs.dpms
 	 *
 	 * The @post_disable callback is optional.
 	 *
@@ -240,18 +273,30 @@ struct drm_bridge_funcs {
 	/**
 	 * @pre_enable:
 	 *
-	 * This callback should enable the bridge. It is called right before
-	 * the preceding element in the display pipe is enabled. If the
-	 * preceding element is a bridge this means it's called before that
-	 * bridge's @pre_enable function. If the preceding element is a
-	 * &drm_encoder it's called right before the encoder's
-	 * &drm_encoder_helper_funcs.enable, &drm_encoder_helper_funcs.commit or
-	 * &drm_encoder_helper_funcs.dpms hook.
-	 *
 	 * The display pipe (i.e. clocks and timing signals) feeding this bridge
-	 * will not yet be running when this callback is called. The bridge must
-	 * not enable the display link feeding the next bridge in the chain (if
-	 * there is one) when this callback is called.
+	 * will not yet be running when the @pre_enable is called.
+	 *
+	 * This callback should perform all the necessary actions to prepare the
+	 * bridge to accept signals from the preceding element.
+	 *
+	 * If the preceding element is a &drm_bridge, then this is called before
+	 * that bridge is pre-enabled (unless marked otherwise by
+	 * @pre_enable_prev_first flag) via one of:
+	 *
+	 * - &drm_bridge_funcs.pre_enable
+	 * - &drm_bridge_funcs.atomic_pre_enable
+	 *
+	 * If the preceding element of the bridge is a display controller, then
+	 * this callback is called before the CRTC is enabled via one of:
+	 *
+	 * - &drm_crtc_helper_funcs.atomic_enable
+	 * - &drm_crtc_helper_funcs.commit
+	 *
+	 * and the encoder is enabled via one of:
+	 *
+	 * - &drm_encoder_helper_funcs.atomic_enable
+	 * - &drm_encoder_helper_funcs.enable
+	 * - &drm_encoder_helper_funcs.commit
 	 *
 	 * The @pre_enable callback is optional.
 	 *
@@ -265,19 +310,31 @@ struct drm_bridge_funcs {
 	/**
 	 * @enable:
 	 *
-	 * This callback should enable the bridge. It is called right after
-	 * the preceding element in the display pipe is enabled. If the
-	 * preceding element is a bridge this means it's called after that
-	 * bridge's @enable function. If the preceding element is a
-	 * &drm_encoder it's called right after the encoder's
-	 * &drm_encoder_helper_funcs.enable, &drm_encoder_helper_funcs.commit or
-	 * &drm_encoder_helper_funcs.dpms hook.
+	 * The @enable callback should enable the bridge.
 	 *
 	 * The bridge can assume that the display pipe (i.e. clocks and timing
 	 * signals) feeding it is running when this callback is called. This
 	 * callback must enable the display link feeding the next bridge in the
 	 * chain if there is one.
 	 *
+	 * If the preceding element is a &drm_bridge, then this is called after
+	 * that bridge is enabled via one of:
+	 *
+	 * - &drm_bridge_funcs.enable
+	 * - &drm_bridge_funcs.atomic_enable
+	 *
+	 * If the preceding element of the bridge is a display controller, then
+	 * this callback is called after the CRTC is enabled via one of:
+	 *
+	 * - &drm_crtc_helper_funcs.atomic_enable
+	 * - &drm_crtc_helper_funcs.commit
+	 *
+	 * and the encoder is enabled via one of:
+	 *
+	 * - &drm_encoder_helper_funcs.atomic_enable
+	 * - &drm_encoder_helper_funcs.enable
+	 * - drm_encoder_helper_funcs.commit
+	 *
 	 * The @enable callback is optional.
 	 *
 	 * NOTE:
@@ -290,17 +347,30 @@ struct drm_bridge_funcs {
 	/**
 	 * @atomic_pre_enable:
 	 *
-	 * This callback should enable the bridge. It is called right before
-	 * the preceding element in the display pipe is enabled. If the
-	 * preceding element is a bridge this means it's called before that
-	 * bridge's @atomic_pre_enable or @pre_enable function. If the preceding
-	 * element is a &drm_encoder it's called right before the encoder's
-	 * &drm_encoder_helper_funcs.atomic_enable hook.
-	 *
 	 * The display pipe (i.e. clocks and timing signals) feeding this bridge
-	 * will not yet be running when this callback is called. The bridge must
-	 * not enable the display link feeding the next bridge in the chain (if
-	 * there is one) when this callback is called.
+	 * will not yet be running when the @atomic_pre_enable is called.
+	 *
+	 * This callback should perform all the necessary actions to prepare the
+	 * bridge to accept signals from the preceding element.
+	 *
+	 * If the preceding element is a &drm_bridge, then this is called before
+	 * that bridge is pre-enabled (unless marked otherwise by
+	 * @pre_enable_prev_first flag) via one of:
+	 *
+	 * - &drm_bridge_funcs.pre_enable
+	 * - &drm_bridge_funcs.atomic_pre_enable
+	 *
+	 * If the preceding element of the bridge is a display controller, then
+	 * this callback is called before the CRTC is enabled via one of:
+	 *
+	 * - &drm_crtc_helper_funcs.atomic_enable
+	 * - &drm_crtc_helper_funcs.commit
+	 *
+	 * and the encoder is enabled via one of:
+	 *
+	 * - &drm_encoder_helper_funcs.atomic_enable
+	 * - &drm_encoder_helper_funcs.enable
+	 * - &drm_encoder_helper_funcs.commit
 	 *
 	 * The @atomic_pre_enable callback is optional.
 	 */
@@ -310,18 +380,31 @@ struct drm_bridge_funcs {
 	/**
 	 * @atomic_enable:
 	 *
-	 * This callback should enable the bridge. It is called right after
-	 * the preceding element in the display pipe is enabled. If the
-	 * preceding element is a bridge this means it's called after that
-	 * bridge's @atomic_enable or @enable function. If the preceding element
-	 * is a &drm_encoder it's called right after the encoder's
-	 * &drm_encoder_helper_funcs.atomic_enable hook.
+	 * The @atomic_enable callback should enable the bridge.
 	 *
 	 * The bridge can assume that the display pipe (i.e. clocks and timing
 	 * signals) feeding it is running when this callback is called. This
 	 * callback must enable the display link feeding the next bridge in the
 	 * chain if there is one.
 	 *
+	 * If the preceding element is a &drm_bridge, then this is called after
+	 * that bridge is enabled via one of:
+	 *
+	 * - &drm_bridge_funcs.enable
+	 * - &drm_bridge_funcs.atomic_enable
+	 *
+	 * If the preceding element of the bridge is a display controller, then
+	 * this callback is called after the CRTC is enabled via one of:
+	 *
+	 * - &drm_crtc_helper_funcs.atomic_enable
+	 * - &drm_crtc_helper_funcs.commit
+	 *
+	 * and the encoder is enabled via one of:
+	 *
+	 * - &drm_encoder_helper_funcs.atomic_enable
+	 * - &drm_encoder_helper_funcs.enable
+	 * - drm_encoder_helper_funcs.commit
+	 *
 	 * The @atomic_enable callback is optional.
 	 */
 	void (*atomic_enable)(struct drm_bridge *bridge,
@@ -329,16 +412,32 @@ struct drm_bridge_funcs {
 	/**
 	 * @atomic_disable:
 	 *
-	 * This callback should disable the bridge. It is called right before
-	 * the preceding element in the display pipe is disabled. If the
-	 * preceding element is a bridge this means it's called before that
-	 * bridge's @atomic_disable or @disable vfunc. If the preceding element
-	 * is a &drm_encoder it's called right before the
-	 * &drm_encoder_helper_funcs.atomic_disable hook.
+	 * The @atomic_disable callback should disable the bridge.
 	 *
 	 * The bridge can assume that the display pipe (i.e. clocks and timing
 	 * signals) feeding it is still running when this callback is called.
 	 *
+	 * If the preceding element is a &drm_bridge, then this is called before
+	 * that bridge is disabled via one of:
+	 *
+	 * - &drm_bridge_funcs.disable
+	 * - &drm_bridge_funcs.atomic_disable
+	 *
+	 * If the preceding element of the bridge is a display controller, then
+	 * this callback is called before the encoder is disabled via one of:
+	 *
+	 * - &drm_encoder_helper_funcs.atomic_disable
+	 * - &drm_encoder_helper_funcs.prepare
+	 * - &drm_encoder_helper_funcs.disable
+	 * - &drm_encoder_helper_funcs.dpms
+	 *
+	 * and the CRTC is disabled via one of:
+	 *
+	 * - &drm_crtc_helper_funcs.prepare
+	 * - &drm_crtc_helper_funcs.atomic_disable
+	 * - &drm_crtc_helper_funcs.disable
+	 * - &drm_crtc_helper_funcs.dpms.
+	 *
 	 * The @atomic_disable callback is optional.
 	 */
 	void (*atomic_disable)(struct drm_bridge *bridge,
@@ -347,16 +446,34 @@ struct drm_bridge_funcs {
 	/**
 	 * @atomic_post_disable:
 	 *
-	 * This callback should disable the bridge. It is called right after the
-	 * preceding element in the display pipe is disabled. If the preceding
-	 * element is a bridge this means it's called after that bridge's
-	 * @atomic_post_disable or @post_disable function. If the preceding
-	 * element is a &drm_encoder it's called right after the encoder's
-	 * &drm_encoder_helper_funcs.atomic_disable hook.
-	 *
 	 * The bridge must assume that the display pipe (i.e. clocks and timing
-	 * signals) feeding it is no longer running when this callback is
-	 * called.
+	 * signals) feeding this bridge is no longer running when the
+	 * @atomic_post_disable is called.
+	 *
+	 * This callback should perform all the actions required by the hardware
+	 * after it has stopped receiving signals from the preceding element.
+	 *
+	 * If the preceding element is a &drm_bridge, then this is called after
+	 * that bridge is post-disabled (unless marked otherwise by the
+	 * @pre_enable_prev_first flag) via one of:
+	 *
+	 * - &drm_bridge_funcs.post_disable
+	 * - &drm_bridge_funcs.atomic_post_disable
+	 *
+	 * If the preceding element of the bridge is a display controller, then
+	 * this callback is called after the encoder is disabled via one of:
+	 *
+	 * - &drm_encoder_helper_funcs.atomic_disable
+	 * - &drm_encoder_helper_funcs.prepare
+	 * - &drm_encoder_helper_funcs.disable
+	 * - &drm_encoder_helper_funcs.dpms
+	 *
+	 * and the CRTC is disabled via one of:
+	 *
+	 * - &drm_crtc_helper_funcs.prepare
+	 * - &drm_crtc_helper_funcs.atomic_disable
+	 * - &drm_crtc_helper_funcs.disable
+	 * - &drm_crtc_helper_funcs.dpms
 	 *
 	 * The @atomic_post_disable callback is optional.
 	 */
-- 
2.34.1

[PATCH v12 5/5] drm/bridge: cdns-dsi: Use pre_enable/post_disable to enable/disable

[Aradhya Bhatia]

From: Aradhya Bhatia <a-bhatia1@ti.com>

The cdns-dsi controller requires that it be turned on completely before
the input DPI's source has begun streaming[0]. Not having that, allows
for a small window before cdns-dsi enable and after cdns-dsi disable
where the previous entity (in this case tidss's videoport) to continue
streaming DPI video signals. This small window where cdns-dsi is
disabled but is still receiving signals causes the input FIFO of
cdns-dsi to get corrupted. This causes the colors to shift on the output
display. The colors can either shift by one color component (R->G, G->B,
B->R), or by two color components (R->B, G->R, B->G).

Since tidss's videoport starts streaming via crtc enable hooks, we need
cdns-dsi to be up and running before that. Now that the bridges are
pre_enabled before crtc is enabled, and post_disabled after crtc is
disabled, use the pre_enable and post_disable hooks to get cdns-dsi
ready and running before the tidss videoport to get pass the color shift
issues.

[0]: See section 12.6.5.7.3 "Start-up Procedure" in J721E SoC TRM
     TRM Link: http://www.ti.com/lit/pdf/spruil1

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Tested-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
Signed-off-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>
---
 .../gpu/drm/bridge/cadence/cdns-dsi-core.c    | 64 ++++++++++---------
 1 file changed, 35 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index b022dd6e6b6e..47435f2624a8 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -670,13 +670,28 @@ cdns_dsi_bridge_mode_valid(struct drm_bridge *bridge,
 	return MODE_OK;
 }
 
-static void cdns_dsi_bridge_atomic_disable(struct drm_bridge *bridge,
-					   struct drm_atomic_state *state)
+static void cdns_dsi_bridge_atomic_post_disable(struct drm_bridge *bridge,
+						struct drm_atomic_state *state)
 {
 	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
 	struct cdns_dsi *dsi = input_to_dsi(input);
 	u32 val;
 
+	/*
+	 * The cdns-dsi controller needs to be disabled after it's DPI source
+	 * has stopped streaming. If this is not followed, there is a brief
+	 * window before DPI source is disabled and after cdns-dsi controller
+	 * has been disabled where the DPI stream is still on, but the cdns-dsi
+	 * controller is not ready anymore to accept the incoming signals. This
+	 * is one of the reasons why a shift in pixel colors is observed on
+	 * displays that have cdns-dsi as one of the bridges.
+	 *
+	 * To mitigate this, disable this bridge from the bridge post_disable()
+	 * hook, instead of the bridge _disable() hook. The bridge post_disable()
+	 * hook gets called after the CRTC disable, where often many DPI sources
+	 * disable their streams.
+	 */
+
 	val = readl(dsi->regs + MCTL_MAIN_DATA_CTL);
 	val &= ~(IF_VID_SELECT_MASK | IF_VID_MODE | VID_EN | HOST_EOT_GEN |
 		 DISP_EOT_GEN);
@@ -688,15 +703,6 @@ static void cdns_dsi_bridge_atomic_disable(struct drm_bridge *bridge,
 	if (dsi->platform_ops && dsi->platform_ops->disable)
 		dsi->platform_ops->disable(dsi);
 
-	pm_runtime_put(dsi->base.dev);
-}
-
-static void cdns_dsi_bridge_atomic_post_disable(struct drm_bridge *bridge,
-						struct drm_atomic_state *state)
-{
-	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
-	struct cdns_dsi *dsi = input_to_dsi(input);
-
 	dsi->phy_initialized = false;
 	dsi->link_initialized = false;
 	phy_power_off(dsi->dphy);
@@ -774,8 +780,8 @@ static void cdns_dsi_init_link(struct cdns_dsi *dsi)
 	dsi->link_initialized = true;
 }
 
-static void cdns_dsi_bridge_atomic_enable(struct drm_bridge *bridge,
-					  struct drm_atomic_state *state)
+static void cdns_dsi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
+					      struct drm_atomic_state *state)
 {
 	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
 	struct cdns_dsi *dsi = input_to_dsi(input);
@@ -792,6 +798,21 @@ static void cdns_dsi_bridge_atomic_enable(struct drm_bridge *bridge,
 	u32 tmp, reg_wakeup, div, status;
 	int nlanes;
 
+	/*
+	 * The cdns-dsi controller needs to be enabled before it's DPI source
+	 * has begun streaming. If this is not followed, there is a brief window
+	 * after DPI source enable and before cdns-dsi controller enable where
+	 * the DPI stream is on, but the cdns-dsi controller is not ready to
+	 * accept the incoming signals. This is one of the reasons why a shift
+	 * in pixel colors is observed on displays that have cdns-dsi as one of
+	 * the bridges.
+	 *
+	 * To mitigate this, enable this bridge from the bridge pre_enable()
+	 * hook, instead of the bridge _enable() hook. The bridge pre_enable()
+	 * hook gets called before the CRTC enable, where often many DPI sources
+	 * enable their streams.
+	 */
+
 	if (WARN_ON(pm_runtime_get_sync(dsi->base.dev) < 0))
 		return;
 
@@ -811,8 +832,8 @@ static void cdns_dsi_bridge_atomic_enable(struct drm_bridge *bridge,
 	mode = &crtc_state->adjusted_mode;
 	nlanes = output->dev->lanes;
 
-	cdns_dsi_hs_init(dsi);
 	cdns_dsi_init_link(dsi);
+	cdns_dsi_hs_init(dsi);
 
 	/*
 	 * Now that the DSI Link and DSI Phy are initialized,
@@ -941,19 +962,6 @@ static void cdns_dsi_bridge_atomic_enable(struct drm_bridge *bridge,
 	writel(tmp, dsi->regs + MCTL_MAIN_EN);
 }
 
-static void cdns_dsi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
-					      struct drm_atomic_state *state)
-{
-	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
-	struct cdns_dsi *dsi = input_to_dsi(input);
-
-	if (WARN_ON(pm_runtime_get_sync(dsi->base.dev) < 0))
-		return;
-
-	cdns_dsi_init_link(dsi);
-	cdns_dsi_hs_init(dsi);
-}
-
 static u32 *cdns_dsi_bridge_get_input_bus_fmts(struct drm_bridge *bridge,
 					       struct drm_bridge_state *bridge_state,
 					       struct drm_crtc_state *crtc_state,
@@ -1048,9 +1056,7 @@ cdns_dsi_bridge_atomic_reset(struct drm_bridge *bridge)
 static const struct drm_bridge_funcs cdns_dsi_bridge_funcs = {
 	.attach = cdns_dsi_bridge_attach,
 	.mode_valid = cdns_dsi_bridge_mode_valid,
-	.atomic_disable = cdns_dsi_bridge_atomic_disable,
 	.atomic_pre_enable = cdns_dsi_bridge_atomic_pre_enable,
-	.atomic_enable = cdns_dsi_bridge_atomic_enable,
 	.atomic_post_disable = cdns_dsi_bridge_atomic_post_disable,
 	.atomic_check = cdns_dsi_bridge_atomic_check,
 	.atomic_reset = cdns_dsi_bridge_atomic_reset,
-- 
2.34.1

Re: [PATCH v12 4/5] drm/bridge: Update the bridge enable/disable doc

[Tomi Valkeinen]

Hi,

On 06/04/2025 16:16, Aradhya Bhatia wrote:
> Now that the bridges get pre-enabled before the CRTC is enabled, and get
> post-disabled after the CRTC is disabled, update the function
> descriptions to accurately reflect the updated scenario.
> 
> The enable sequence for the display pipeline looks like:
> 
> 	bridge[n]_pre_enable
> 	...
> 	bridge[1]_pre_enable
> 
> 	crtc_enable
> 	encoder_enable
> 
> 	bridge[1]_enable
> 	...
> 	bridge[n]_enable
> 
> And, the disable sequence for the display pipeline looks like:
> 
> 	bridge[n]_disable
> 	...
> 	bridge[1]_disable
> 
> 	encoder_disable
> 	crtc_disable
> 
> 	bridge[1]_post_disable
> 	...
> 	bridge[n]_post_disable
> 
> Signed-off-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>
> ---
>   include/drm/drm_bridge.h | 249 ++++++++++++++++++++++++++++-----------
>   1 file changed, 183 insertions(+), 66 deletions(-)

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

  Tomi

Re: [PATCH v12 0/5] drm/atomic-helper: Re-order CRTC and Bridge ops

[Tomi Valkeinen]

Hi all,

On 06/04/2025 16:16, Aradhya Bhatia wrote:
> Hello all,
> 
> This series re-orders the sequences in which the drm CRTC and the drm
> Bridge get enabled and disabled with respect to each other.
> 
> The bridge pre_enable calls have been shifted before the crtc_enable and
> the bridge post_disable calls have been shifted after the crtc_disable.
> 
> This has been done as per the definition of bridge pre_enable.
> "The display pipe (i.e. clocks and timing signals) feeding this bridge will
> not yet be running when this callback is called".
> 
> Since CRTC is also a source feeding the bridge, it should not be enabled
> before the bridges in the pipeline are pre_enabled.

Any further comments to this?

All the patches have been reviewed and tested. I can push this, along 
with the serieses that depend on this, via drm-misc, but I'll need an 
ack from a maintainer.

  Tomi


> The original sequence. for display pipe enable looks like:
> 
>        crtc_enable
> 
>        bridge[n]_pre_enable
>        ...
>        bridge[1]_pre_enable
> 
>        encoder_enable
> 
>        bridge[1]_enable
>        ...
>        bridge[n]_enable
> 
> The sequence of enable after this patch-set will look like:
> 
>        bridge[n]_pre_enable
>        ...
>        bridge[1]_pre_enable
> 
>        crtc_enable
>        encoder_enable
> 
>        bridge[1]_enable
>        ...
>        bridge[n]_enable
> 
> 
> For the disable sequence, this is what the original looks like:
> 
>        bridge[n]_disable
>        ...
>        bridge[1]_disable
> 
>        encoder_disable
> 
>        bridge[1]_post_disable
>        ...
>        bridge[n]_post_disable
> 
>        crtc_disable
> 	
> This is what the disable sequence will be, after this series of patches:
> 
>        bridge[n]_disable
>        ...
>        bridge[1]_disable
> 
>        encoder_disable
>        crtc_disable
> 
>        bridge[1]_post_disable
>        ...
>        bridge[n]_post_disable
> 
> This series further updates the bridge API definitions to accurately
> reflect the updated scenario.
> 
> This series is a subset of its v11[0] which had 14 patches in the revision.
> 9 of those 14 patches (which were specific to the cdns-dsi bridge driver)
> were merged[1].
> 
> Regards
> Aradhya
> 
> ---
> * Note on checkpatch warning in patch 2/4 *
> Patch 2/4 causes the checkpatch to flare up for 1 checkpatch 'check' -
> 
> CHECK: Lines should not end with a '('
> #79: FILE: drivers/gpu/drm/drm_atomic_helper.c:1304:
> +                       new_crtc_state = drm_atomic_get_new_crtc_state(
> 
> This patch is largely duplicating the original code, with minor differences to
> perform different operations. This line of code pre-exists in the file and
> have simply been duplicated. I have decided to keep it as is to maintain the
> uniformity and the originally intended readability. Should perhaps a fix be
> required, this patch/series is not the right place, and another patch can be
> created to fix this across the whole file.
> 
> References:
> [0]: Revision v11 of this series.
> https://lore.kernel.org/all/20250329113925.68204-1-aradhya.bhatia@linux.dev/
> 
> [1]: Patches 1 through 9 getting merged.
> https://lore.kernel.org/all/174335361171.2556605.12634785416741695829.b4-ty@oss.qualcomm.com/
> 
> 
> ---
> Change Log:
> 
>    - Changes in v12:
>      - Drop patches 1 through 9 since they have been merged.
>      - Rebase onto newer drm-misc-next.
>      - Re-word the patch 3/4, ("drm/bridge: Update the bridge enable/disable doc")
>        to make it more readable.
> 
>    - Changes in v11:
>      - Add patch v11:13/14 ("drm/bridge: Update the bridge enable/disable doc"),
>        that updates the documentation about the order of the various bridge
>        enable/disable hooks being called wrt the CRTC and encoder hooks.
>      - Rebase on drm-misc-next instead of linux-next.
>        As part of rebase, accommodate the following change:
>        - Change patch v10:08/13 ("drm/bridge: cdns-dsi: Support atomic bridge
>          APIs") to v11:08/13 ("drm/bridge: cdns-dsi: Add input format
>          negotiation"), since Maxime has already updated the bridge hooks to
>          their atomic versions in commit 68c98e227a96 ("drm/bridge: cdns-csi:
>          Switch to atomic helpers").
>          My new patch now only adds the format negotiation hook for the cdns-dsi.
>          (Note: Since the new patch is now only a subset of the old one, without
>          any change in logic, I decided to carry forward the R-b and T-b tags.)
>      - Add Alexander Sverdlin's T-b in patches 10, 11, 12.
> 
>    - Changes in v10:
>      - Rebase on latest linux-next (next-20250226).
>      - As part of rebase, update the patches to accommodate a couple of
>        widespread changes in DRM Framework -
>          - All the ("drm/atomic-helper: Change parameter name of ***") commits.
>          - All the ("drm/bridge: Pass full state to ***") commits.
>        (These updates are only trivial substitutions.)
>      - Add Tomi Valkeinen's T-b tags in all the patches.
> 
>    - Changes in v9:
>      - Fix the oops in 11/13 - where the encoder_bridge_enable _was_ pre_enabling
>        the bridges instead of enabling.
>      - Add the following tags:
>        - Dmitry Baryshkov's R-b in patches 2, 10, 11, and A-b in patch 12.
>        - Jayesh Choudhary's R-b in patch 12.
>        - Tomi Valkeinen's R-b in patches 2, 10, 11, 12.
> 
>    - Changes in v8:
>      - Move the phy de-initialization to bridge post_disable() instead of bridge
>        disable() in patch-3.
>      - Copy the private bridge state (dsi_cfg), in addition to the bridge_state,
>        in patch-9.
>      - Split patch v7:11/12 into three patches, v8:{10,11,12}/13, to separate out
>        different refactorings into different patches, and improve bisectability.
>      - Move patch v7:02/12 down to v8:06/12, to keep the initial patches for
>        fixes only.
>      - Drop patch v7:04/12 as it doesn't become relevant until patch v7:12/12.
>      - Add R-b tags of Dmitry Baryshkov in patch-9 and patch-3, and of
>        Tomi Valkeinen in patch-9.
>     
>    - Changes in v7:
>      - phy_init()/exit() were called from the PM path in v6. Change it back to
>        the bridge enable/disable path in patch-3, so that the phy_init() can go
>        back to being called after D-Phy reset assert.
>      - Reword commit text in patch-5 to explain the need of the fix.
>      - Drop the stray code in patch-10.
>      - Add R-b tag of Dmitry Baryshkov in patch-6.
> 
>    - Changes in v6:
>      - Reword patch 3 to better explain the fixes around phy de-init.
>      - Fix the Lane ready timeout condition in patch 7.
>      - Fix the dsi _bridge_atomic_check() implementation by adding a new
>        bridge state structure in patch 10.
>      - Rework and combine patches v5:11/13 and v5:12/13 to v6:11/12.
>      - Generate the patches of these series using the "patience" algorithm.
>        Note: All patches, except v6:11/12, *do not* differ from their default
>        (greedy) algorithm variants.
>        For patch 11, the patience algorithm significantly improves the readability.
>      - Rename and move the Bridge enable/disable enums from public to private
>        in patch 11.
>      - Add R-b tags of Tomi Valkeinen in patch 6, and Dmitry Baryshkov in patch 2.
> 
>    - Changes in v5:
>      - Fix subject and description in patch 1/13.
>      - Add patch to check the return value of
>        phy_mipi_dphy_get_default_config() (patch: 6/13).
>      - Change the Clk and Data Lane ready timeout from forever to 5s.
>      - Print an error instead of calling WARN_ON_ONCE in patch 7/13.
>      - Drop patch v4-07/11: "drm/bridge: cdns-dsi: Reset the DCS write FIFO".
>        There has been some inconsistencies found with this patch upon further
>        testing. This patch was being used to enable a DSI panel based on ILITEK
>        ILI9881C bridge. This will be debugged separately.
>      - Add patch to move the DSI mode check from _atomic_enable() to
>        _atomic_check() (patch: 10/13).
>      - Split patch v4-10/11 into 2 patches - 11/13 and 12/13.
>        Patch 11/13 separates out the Encoder-Bridge operations into a helper
>        function *without* changing the logic. Patch 12/13 then changes the order
>        of the encoder-bridge operations as was intended in the original patch.
>      - Add detailed comment for patch 13/13.
>      - Add Tomi Valkeinen's R-b in patches 1, 2, 4, 5, 7, 8, 9, 13.
> 
>    - Changes in v4:
>      - Add new patch, "drm/bridge: cdns-dsi: Move to devm_drm_of_get_bridge()",
>        to update to an auto-managed way of finding next bridge in the chain.
>      - Drop patch "drm/bridge: cdns-dsi: Fix the phy_initialized variable" and
>        add "drm/bridge: cdns-dsi: Fix Phy _init() and _exit()" that properly
>        de-initializes the Phy and maintains the initialization state.
>      - Reword patch "drm/bridge: cdns-dsi: Reset the DCS write FIFO" to explain
>        the HW concerns better.
>      - Add R-b tag from Dmitry Baryshkov for patches 1/11 and 8/11.
> 
>    - Changes in v3:
>      - Reword the commit message for patch "drm/bridge: cdns-dsi: Fix OF node
>        pointer".
>      - Add a new helper API to figure out DSI host input pixel format
>        in patch "drm/mipi-dsi: Add helper to find input format".
>      - Use a common function for bridge pre-enable and enable, and bridge disable
>        and post-disable, to avoid code duplication.
>      - Add T-b tag from Dominik Haller in patch 5/10. (Missed to add it in v2).
>      - Add R-b tag from Dmitry Baryshkov for patch 8/10.
> 
>    - Changes in v2:
>      - Drop patch "drm/tidss: Add CRTC mode_fixup"
>      - Split patch "drm/bridge: cdns-dsi: Fix minor bugs" into 4 separate ones
>      - Drop support for early_enable/late_disable APIs and instead re-order the
>        pre_enable / post_disable APIs to be called before / after crtc_enable /
>        crtc_disable.
>      - Drop support for early_enable/late_disable in cdns-dsi and use
>        pre_enable/post_disable APIs instead to do bridge enable/disable.
> 
> 
> Previous versions:
> 
> v1:  https://lore.kernel.org/all/20240511153051.1355825-1-a-bhatia1@ti.com/
> v2:  https://lore.kernel.org/all/20240530093621.1925863-1-a-bhatia1@ti.com/
> v3:  https://lore.kernel.org/all/20240617105311.1587489-1-a-bhatia1@ti.com/
> v4:  https://lore.kernel.org/all/20240622110929.3115714-1-a-bhatia1@ti.com/
> v5:  https://lore.kernel.org/all/20241019195411.266860-1-aradhya.bhatia@linux.dev/
> v6:  https://lore.kernel.org/all/20250111192738.308889-1-aradhya.bhatia@linux.dev/
> v7:  https://lore.kernel.org/all/20250114055626.18816-1-aradhya.bhatia@linux.dev/
> v8:  https://lore.kernel.org/all/20250126191551.741957-1-aradhya.bhatia@linux.dev/
> v9:  https://lore.kernel.org/all/20250209121032.32655-1-aradhya.bhatia@linux.dev/
> v10: https://lore.kernel.org/all/20250226155228.564289-1-aradhya.bhatia@linux.dev/
> v11: https://lore.kernel.org/all/20250329113925.68204-1-aradhya.bhatia@linux.dev/
> 
> ---
> 
> Aradhya Bhatia (5):
>    drm/atomic-helper: Refactor crtc & encoder-bridge op loops into
>      separate functions
>    drm/atomic-helper: Separate out bridge pre_enable/post_disable from
>      enable/disable
>    drm/atomic-helper: Re-order bridge chain pre-enable and post-disable
>    drm/bridge: Update the bridge enable/disable doc
>    drm/bridge: cdns-dsi: Use pre_enable/post_disable to enable/disable
> 
>   .../gpu/drm/bridge/cadence/cdns-dsi-core.c    |  64 +++--
>   drivers/gpu/drm/drm_atomic_helper.c           | 160 +++++++++--
>   include/drm/drm_bridge.h                      | 249 +++++++++++++-----
>   3 files changed, 355 insertions(+), 118 deletions(-)
> 
> 
> base-commit: dd717762761807452ca25634652e180a80349cd8

Re: [PATCH v12 0/5] drm/atomic-helper: Re-order CRTC and Bridge ops

[Tomi Valkeinen]

Hi Maxime, Maarten, Thomas,

On 16/05/2025 15:23, Tomi Valkeinen wrote:
> Hi all,
> 
> On 06/04/2025 16:16, Aradhya Bhatia wrote:
>> Hello all,
>>
>> This series re-orders the sequences in which the drm CRTC and the drm
>> Bridge get enabled and disabled with respect to each other.
>>
>> The bridge pre_enable calls have been shifted before the crtc_enable and
>> the bridge post_disable calls have been shifted after the crtc_disable.
>>
>> This has been done as per the definition of bridge pre_enable.
>> "The display pipe (i.e. clocks and timing signals) feeding this bridge
>> will
>> not yet be running when this callback is called".
>>
>> Since CRTC is also a source feeding the bridge, it should not be enabled
>> before the bridges in the pipeline are pre_enabled.
> 
> Any further comments to this?
> 
> All the patches have been reviewed and tested. I can push this, along
> with the serieses that depend on this, via drm-misc, but I'll need an
> ack from a maintainer.

Would any of you have a moment to check this out and ack/apply? This
series is blocking further development on DSI and OLDI.

 Tomi

> 
>  Tomi
> 
> 
>> The original sequence. for display pipe enable looks like:
>>
>>        crtc_enable
>>
>>        bridge[n]_pre_enable
>>        ...
>>        bridge[1]_pre_enable
>>
>>        encoder_enable
>>
>>        bridge[1]_enable
>>        ...
>>        bridge[n]_enable
>>
>> The sequence of enable after this patch-set will look like:
>>
>>        bridge[n]_pre_enable
>>        ...
>>        bridge[1]_pre_enable
>>
>>        crtc_enable
>>        encoder_enable
>>
>>        bridge[1]_enable
>>        ...
>>        bridge[n]_enable
>>
>>
>> For the disable sequence, this is what the original looks like:
>>
>>        bridge[n]_disable
>>        ...
>>        bridge[1]_disable
>>
>>        encoder_disable
>>
>>        bridge[1]_post_disable
>>        ...
>>        bridge[n]_post_disable
>>
>>        crtc_disable
>>     
>> This is what the disable sequence will be, after this series of patches:
>>
>>        bridge[n]_disable
>>        ...
>>        bridge[1]_disable
>>
>>        encoder_disable
>>        crtc_disable
>>
>>        bridge[1]_post_disable
>>        ...
>>        bridge[n]_post_disable
>>
>> This series further updates the bridge API definitions to accurately
>> reflect the updated scenario.
>>
>> This series is a subset of its v11[0] which had 14 patches in the
>> revision.
>> 9 of those 14 patches (which were specific to the cdns-dsi bridge driver)
>> were merged[1].
>>
>> Regards
>> Aradhya
>>
>> ---
>> * Note on checkpatch warning in patch 2/4 *
>> Patch 2/4 causes the checkpatch to flare up for 1 checkpatch 'check' -
>>
>> CHECK: Lines should not end with a '('
>> #79: FILE: drivers/gpu/drm/drm_atomic_helper.c:1304:
>> +                       new_crtc_state = drm_atomic_get_new_crtc_state(
>>
>> This patch is largely duplicating the original code, with minor
>> differences to
>> perform different operations. This line of code pre-exists in the file
>> and
>> have simply been duplicated. I have decided to keep it as is to
>> maintain the
>> uniformity and the originally intended readability. Should perhaps a
>> fix be
>> required, this patch/series is not the right place, and another patch
>> can be
>> created to fix this across the whole file.
>>
>> References:
>> [0]: Revision v11 of this series.
>> https://lore.kernel.org/all/20250329113925.68204-1-
>> aradhya.bhatia@linux.dev/
>>
>> [1]: Patches 1 through 9 getting merged.
>> https://lore.kernel.org/
>> all/174335361171.2556605.12634785416741695829.b4-ty@oss.qualcomm.com/
>>
>>
>> ---
>> Change Log:
>>
>>    - Changes in v12:
>>      - Drop patches 1 through 9 since they have been merged.
>>      - Rebase onto newer drm-misc-next.
>>      - Re-word the patch 3/4, ("drm/bridge: Update the bridge enable/
>> disable doc")
>>        to make it more readable.
>>
>>    - Changes in v11:
>>      - Add patch v11:13/14 ("drm/bridge: Update the bridge enable/
>> disable doc"),
>>        that updates the documentation about the order of the various
>> bridge
>>        enable/disable hooks being called wrt the CRTC and encoder hooks.
>>      - Rebase on drm-misc-next instead of linux-next.
>>        As part of rebase, accommodate the following change:
>>        - Change patch v10:08/13 ("drm/bridge: cdns-dsi: Support atomic
>> bridge
>>          APIs") to v11:08/13 ("drm/bridge: cdns-dsi: Add input format
>>          negotiation"), since Maxime has already updated the bridge
>> hooks to
>>          their atomic versions in commit 68c98e227a96 ("drm/bridge:
>> cdns-csi:
>>          Switch to atomic helpers").
>>          My new patch now only adds the format negotiation hook for
>> the cdns-dsi.
>>          (Note: Since the new patch is now only a subset of the old
>> one, without
>>          any change in logic, I decided to carry forward the R-b and
>> T-b tags.)
>>      - Add Alexander Sverdlin's T-b in patches 10, 11, 12.
>>
>>    - Changes in v10:
>>      - Rebase on latest linux-next (next-20250226).
>>      - As part of rebase, update the patches to accommodate a couple of
>>        widespread changes in DRM Framework -
>>          - All the ("drm/atomic-helper: Change parameter name of ***")
>> commits.
>>          - All the ("drm/bridge: Pass full state to ***") commits.
>>        (These updates are only trivial substitutions.)
>>      - Add Tomi Valkeinen's T-b tags in all the patches.
>>
>>    - Changes in v9:
>>      - Fix the oops in 11/13 - where the encoder_bridge_enable _was_
>> pre_enabling
>>        the bridges instead of enabling.
>>      - Add the following tags:
>>        - Dmitry Baryshkov's R-b in patches 2, 10, 11, and A-b in patch
>> 12.
>>        - Jayesh Choudhary's R-b in patch 12.
>>        - Tomi Valkeinen's R-b in patches 2, 10, 11, 12.
>>
>>    - Changes in v8:
>>      - Move the phy de-initialization to bridge post_disable() instead
>> of bridge
>>        disable() in patch-3.
>>      - Copy the private bridge state (dsi_cfg), in addition to the
>> bridge_state,
>>        in patch-9.
>>      - Split patch v7:11/12 into three patches, v8:{10,11,12}/13, to
>> separate out
>>        different refactorings into different patches, and improve
>> bisectability.
>>      - Move patch v7:02/12 down to v8:06/12, to keep the initial
>> patches for
>>        fixes only.
>>      - Drop patch v7:04/12 as it doesn't become relevant until patch
>> v7:12/12.
>>      - Add R-b tags of Dmitry Baryshkov in patch-9 and patch-3, and of
>>        Tomi Valkeinen in patch-9.
>>        - Changes in v7:
>>      - phy_init()/exit() were called from the PM path in v6. Change it
>> back to
>>        the bridge enable/disable path in patch-3, so that the
>> phy_init() can go
>>        back to being called after D-Phy reset assert.
>>      - Reword commit text in patch-5 to explain the need of the fix.
>>      - Drop the stray code in patch-10.
>>      - Add R-b tag of Dmitry Baryshkov in patch-6.
>>
>>    - Changes in v6:
>>      - Reword patch 3 to better explain the fixes around phy de-init.
>>      - Fix the Lane ready timeout condition in patch 7.
>>      - Fix the dsi _bridge_atomic_check() implementation by adding a new
>>        bridge state structure in patch 10.
>>      - Rework and combine patches v5:11/13 and v5:12/13 to v6:11/12.
>>      - Generate the patches of these series using the "patience"
>> algorithm.
>>        Note: All patches, except v6:11/12, *do not* differ from their
>> default
>>        (greedy) algorithm variants.
>>        For patch 11, the patience algorithm significantly improves the
>> readability.
>>      - Rename and move the Bridge enable/disable enums from public to
>> private
>>        in patch 11.
>>      - Add R-b tags of Tomi Valkeinen in patch 6, and Dmitry Baryshkov
>> in patch 2.
>>
>>    - Changes in v5:
>>      - Fix subject and description in patch 1/13.
>>      - Add patch to check the return value of
>>        phy_mipi_dphy_get_default_config() (patch: 6/13).
>>      - Change the Clk and Data Lane ready timeout from forever to 5s.
>>      - Print an error instead of calling WARN_ON_ONCE in patch 7/13.
>>      - Drop patch v4-07/11: "drm/bridge: cdns-dsi: Reset the DCS write
>> FIFO".
>>        There has been some inconsistencies found with this patch upon
>> further
>>        testing. This patch was being used to enable a DSI panel based
>> on ILITEK
>>        ILI9881C bridge. This will be debugged separately.
>>      - Add patch to move the DSI mode check from _atomic_enable() to
>>        _atomic_check() (patch: 10/13).
>>      - Split patch v4-10/11 into 2 patches - 11/13 and 12/13.
>>        Patch 11/13 separates out the Encoder-Bridge operations into a
>> helper
>>        function *without* changing the logic. Patch 12/13 then changes
>> the order
>>        of the encoder-bridge operations as was intended in the
>> original patch.
>>      - Add detailed comment for patch 13/13.
>>      - Add Tomi Valkeinen's R-b in patches 1, 2, 4, 5, 7, 8, 9, 13.
>>
>>    - Changes in v4:
>>      - Add new patch, "drm/bridge: cdns-dsi: Move to
>> devm_drm_of_get_bridge()",
>>        to update to an auto-managed way of finding next bridge in the
>> chain.
>>      - Drop patch "drm/bridge: cdns-dsi: Fix the phy_initialized
>> variable" and
>>        add "drm/bridge: cdns-dsi: Fix Phy _init() and _exit()" that
>> properly
>>        de-initializes the Phy and maintains the initialization state.
>>      - Reword patch "drm/bridge: cdns-dsi: Reset the DCS write FIFO"
>> to explain
>>        the HW concerns better.
>>      - Add R-b tag from Dmitry Baryshkov for patches 1/11 and 8/11.
>>
>>    - Changes in v3:
>>      - Reword the commit message for patch "drm/bridge: cdns-dsi: Fix
>> OF node
>>        pointer".
>>      - Add a new helper API to figure out DSI host input pixel format
>>        in patch "drm/mipi-dsi: Add helper to find input format".
>>      - Use a common function for bridge pre-enable and enable, and
>> bridge disable
>>        and post-disable, to avoid code duplication.
>>      - Add T-b tag from Dominik Haller in patch 5/10. (Missed to add
>> it in v2).
>>      - Add R-b tag from Dmitry Baryshkov for patch 8/10.
>>
>>    - Changes in v2:
>>      - Drop patch "drm/tidss: Add CRTC mode_fixup"
>>      - Split patch "drm/bridge: cdns-dsi: Fix minor bugs" into 4
>> separate ones
>>      - Drop support for early_enable/late_disable APIs and instead re-
>> order the
>>        pre_enable / post_disable APIs to be called before / after
>> crtc_enable /
>>        crtc_disable.
>>      - Drop support for early_enable/late_disable in cdns-dsi and use
>>        pre_enable/post_disable APIs instead to do bridge enable/disable.
>>
>>
>> Previous versions:
>>
>> v1:  https://lore.kernel.org/all/20240511153051.1355825-1-a-
>> bhatia1@ti.com/
>> v2:  https://lore.kernel.org/all/20240530093621.1925863-1-a-
>> bhatia1@ti.com/
>> v3:  https://lore.kernel.org/all/20240617105311.1587489-1-a-
>> bhatia1@ti.com/
>> v4:  https://lore.kernel.org/all/20240622110929.3115714-1-a-
>> bhatia1@ti.com/
>> v5:  https://lore.kernel.org/all/20241019195411.266860-1-
>> aradhya.bhatia@linux.dev/
>> v6:  https://lore.kernel.org/all/20250111192738.308889-1-
>> aradhya.bhatia@linux.dev/
>> v7:  https://lore.kernel.org/all/20250114055626.18816-1-
>> aradhya.bhatia@linux.dev/
>> v8:  https://lore.kernel.org/all/20250126191551.741957-1-
>> aradhya.bhatia@linux.dev/
>> v9:  https://lore.kernel.org/all/20250209121032.32655-1-
>> aradhya.bhatia@linux.dev/
>> v10: https://lore.kernel.org/all/20250226155228.564289-1-
>> aradhya.bhatia@linux.dev/
>> v11: https://lore.kernel.org/all/20250329113925.68204-1-
>> aradhya.bhatia@linux.dev/
>>
>> ---
>>
>> Aradhya Bhatia (5):
>>    drm/atomic-helper: Refactor crtc & encoder-bridge op loops into
>>      separate functions
>>    drm/atomic-helper: Separate out bridge pre_enable/post_disable from
>>      enable/disable
>>    drm/atomic-helper: Re-order bridge chain pre-enable and post-disable
>>    drm/bridge: Update the bridge enable/disable doc
>>    drm/bridge: cdns-dsi: Use pre_enable/post_disable to enable/disable
>>
>>   .../gpu/drm/bridge/cadence/cdns-dsi-core.c    |  64 +++--
>>   drivers/gpu/drm/drm_atomic_helper.c           | 160 +++++++++--
>>   include/drm/drm_bridge.h                      | 249 +++++++++++++-----
>>   3 files changed, 355 insertions(+), 118 deletions(-)
>>
>>
>> base-commit: dd717762761807452ca25634652e180a80349cd8
> 

Re: [PATCH v12 0/5] drm/atomic-helper: Re-order CRTC and Bridge ops

[Tomi Valkeinen]

Hi Maxime, Maarten, Thomas,

(You're the first three names returned with get_maintainers.pl =), so:)

Ping on this.

 Tomi

On 26/05/2025 13:40, Tomi Valkeinen wrote:
> Hi Maxime, Maarten, Thomas,
> 
> On 16/05/2025 15:23, Tomi Valkeinen wrote:
>> Hi all,
>>
>> On 06/04/2025 16:16, Aradhya Bhatia wrote:
>>> Hello all,
>>>
>>> This series re-orders the sequences in which the drm CRTC and the drm
>>> Bridge get enabled and disabled with respect to each other.
>>>
>>> The bridge pre_enable calls have been shifted before the crtc_enable and
>>> the bridge post_disable calls have been shifted after the crtc_disable.
>>>
>>> This has been done as per the definition of bridge pre_enable.
>>> "The display pipe (i.e. clocks and timing signals) feeding this bridge
>>> will
>>> not yet be running when this callback is called".
>>>
>>> Since CRTC is also a source feeding the bridge, it should not be enabled
>>> before the bridges in the pipeline are pre_enabled.
>>
>> Any further comments to this?
>>
>> All the patches have been reviewed and tested. I can push this, along
>> with the serieses that depend on this, via drm-misc, but I'll need an
>> ack from a maintainer.
> 
> Would any of you have a moment to check this out and ack/apply? This
> series is blocking further development on DSI and OLDI.
> 
>  Tomi
> 
>>
>>  Tomi
>>
>>
>>> The original sequence. for display pipe enable looks like:
>>>
>>>        crtc_enable
>>>
>>>        bridge[n]_pre_enable
>>>        ...
>>>        bridge[1]_pre_enable
>>>
>>>        encoder_enable
>>>
>>>        bridge[1]_enable
>>>        ...
>>>        bridge[n]_enable
>>>
>>> The sequence of enable after this patch-set will look like:
>>>
>>>        bridge[n]_pre_enable
>>>        ...
>>>        bridge[1]_pre_enable
>>>
>>>        crtc_enable
>>>        encoder_enable
>>>
>>>        bridge[1]_enable
>>>        ...
>>>        bridge[n]_enable
>>>
>>>
>>> For the disable sequence, this is what the original looks like:
>>>
>>>        bridge[n]_disable
>>>        ...
>>>        bridge[1]_disable
>>>
>>>        encoder_disable
>>>
>>>        bridge[1]_post_disable
>>>        ...
>>>        bridge[n]_post_disable
>>>
>>>        crtc_disable
>>>     
>>> This is what the disable sequence will be, after this series of patches:
>>>
>>>        bridge[n]_disable
>>>        ...
>>>        bridge[1]_disable
>>>
>>>        encoder_disable
>>>        crtc_disable
>>>
>>>        bridge[1]_post_disable
>>>        ...
>>>        bridge[n]_post_disable
>>>
>>> This series further updates the bridge API definitions to accurately
>>> reflect the updated scenario.
>>>
>>> This series is a subset of its v11[0] which had 14 patches in the
>>> revision.
>>> 9 of those 14 patches (which were specific to the cdns-dsi bridge driver)
>>> were merged[1].
>>>
>>> Regards
>>> Aradhya
>>>
>>> ---
>>> * Note on checkpatch warning in patch 2/4 *
>>> Patch 2/4 causes the checkpatch to flare up for 1 checkpatch 'check' -
>>>
>>> CHECK: Lines should not end with a '('
>>> #79: FILE: drivers/gpu/drm/drm_atomic_helper.c:1304:
>>> +                       new_crtc_state = drm_atomic_get_new_crtc_state(
>>>
>>> This patch is largely duplicating the original code, with minor
>>> differences to
>>> perform different operations. This line of code pre-exists in the file
>>> and
>>> have simply been duplicated. I have decided to keep it as is to
>>> maintain the
>>> uniformity and the originally intended readability. Should perhaps a
>>> fix be
>>> required, this patch/series is not the right place, and another patch
>>> can be
>>> created to fix this across the whole file.
>>>
>>> References:
>>> [0]: Revision v11 of this series.
>>> https://lore.kernel.org/all/20250329113925.68204-1-
>>> aradhya.bhatia@linux.dev/
>>>
>>> [1]: Patches 1 through 9 getting merged.
>>> https://lore.kernel.org/
>>> all/174335361171.2556605.12634785416741695829.b4-ty@oss.qualcomm.com/
>>>
>>>
>>> ---
>>> Change Log:
>>>
>>>    - Changes in v12:
>>>      - Drop patches 1 through 9 since they have been merged.
>>>      - Rebase onto newer drm-misc-next.
>>>      - Re-word the patch 3/4, ("drm/bridge: Update the bridge enable/
>>> disable doc")
>>>        to make it more readable.
>>>
>>>    - Changes in v11:
>>>      - Add patch v11:13/14 ("drm/bridge: Update the bridge enable/
>>> disable doc"),
>>>        that updates the documentation about the order of the various
>>> bridge
>>>        enable/disable hooks being called wrt the CRTC and encoder hooks.
>>>      - Rebase on drm-misc-next instead of linux-next.
>>>        As part of rebase, accommodate the following change:
>>>        - Change patch v10:08/13 ("drm/bridge: cdns-dsi: Support atomic
>>> bridge
>>>          APIs") to v11:08/13 ("drm/bridge: cdns-dsi: Add input format
>>>          negotiation"), since Maxime has already updated the bridge
>>> hooks to
>>>          their atomic versions in commit 68c98e227a96 ("drm/bridge:
>>> cdns-csi:
>>>          Switch to atomic helpers").
>>>          My new patch now only adds the format negotiation hook for
>>> the cdns-dsi.
>>>          (Note: Since the new patch is now only a subset of the old
>>> one, without
>>>          any change in logic, I decided to carry forward the R-b and
>>> T-b tags.)
>>>      - Add Alexander Sverdlin's T-b in patches 10, 11, 12.
>>>
>>>    - Changes in v10:
>>>      - Rebase on latest linux-next (next-20250226).
>>>      - As part of rebase, update the patches to accommodate a couple of
>>>        widespread changes in DRM Framework -
>>>          - All the ("drm/atomic-helper: Change parameter name of ***")
>>> commits.
>>>          - All the ("drm/bridge: Pass full state to ***") commits.
>>>        (These updates are only trivial substitutions.)
>>>      - Add Tomi Valkeinen's T-b tags in all the patches.
>>>
>>>    - Changes in v9:
>>>      - Fix the oops in 11/13 - where the encoder_bridge_enable _was_
>>> pre_enabling
>>>        the bridges instead of enabling.
>>>      - Add the following tags:
>>>        - Dmitry Baryshkov's R-b in patches 2, 10, 11, and A-b in patch
>>> 12.
>>>        - Jayesh Choudhary's R-b in patch 12.
>>>        - Tomi Valkeinen's R-b in patches 2, 10, 11, 12.
>>>
>>>    - Changes in v8:
>>>      - Move the phy de-initialization to bridge post_disable() instead
>>> of bridge
>>>        disable() in patch-3.
>>>      - Copy the private bridge state (dsi_cfg), in addition to the
>>> bridge_state,
>>>        in patch-9.
>>>      - Split patch v7:11/12 into three patches, v8:{10,11,12}/13, to
>>> separate out
>>>        different refactorings into different patches, and improve
>>> bisectability.
>>>      - Move patch v7:02/12 down to v8:06/12, to keep the initial
>>> patches for
>>>        fixes only.
>>>      - Drop patch v7:04/12 as it doesn't become relevant until patch
>>> v7:12/12.
>>>      - Add R-b tags of Dmitry Baryshkov in patch-9 and patch-3, and of
>>>        Tomi Valkeinen in patch-9.
>>>        - Changes in v7:
>>>      - phy_init()/exit() were called from the PM path in v6. Change it
>>> back to
>>>        the bridge enable/disable path in patch-3, so that the
>>> phy_init() can go
>>>        back to being called after D-Phy reset assert.
>>>      - Reword commit text in patch-5 to explain the need of the fix.
>>>      - Drop the stray code in patch-10.
>>>      - Add R-b tag of Dmitry Baryshkov in patch-6.
>>>
>>>    - Changes in v6:
>>>      - Reword patch 3 to better explain the fixes around phy de-init.
>>>      - Fix the Lane ready timeout condition in patch 7.
>>>      - Fix the dsi _bridge_atomic_check() implementation by adding a new
>>>        bridge state structure in patch 10.
>>>      - Rework and combine patches v5:11/13 and v5:12/13 to v6:11/12.
>>>      - Generate the patches of these series using the "patience"
>>> algorithm.
>>>        Note: All patches, except v6:11/12, *do not* differ from their
>>> default
>>>        (greedy) algorithm variants.
>>>        For patch 11, the patience algorithm significantly improves the
>>> readability.
>>>      - Rename and move the Bridge enable/disable enums from public to
>>> private
>>>        in patch 11.
>>>      - Add R-b tags of Tomi Valkeinen in patch 6, and Dmitry Baryshkov
>>> in patch 2.
>>>
>>>    - Changes in v5:
>>>      - Fix subject and description in patch 1/13.
>>>      - Add patch to check the return value of
>>>        phy_mipi_dphy_get_default_config() (patch: 6/13).
>>>      - Change the Clk and Data Lane ready timeout from forever to 5s.
>>>      - Print an error instead of calling WARN_ON_ONCE in patch 7/13.
>>>      - Drop patch v4-07/11: "drm/bridge: cdns-dsi: Reset the DCS write
>>> FIFO".
>>>        There has been some inconsistencies found with this patch upon
>>> further
>>>        testing. This patch was being used to enable a DSI panel based
>>> on ILITEK
>>>        ILI9881C bridge. This will be debugged separately.
>>>      - Add patch to move the DSI mode check from _atomic_enable() to
>>>        _atomic_check() (patch: 10/13).
>>>      - Split patch v4-10/11 into 2 patches - 11/13 and 12/13.
>>>        Patch 11/13 separates out the Encoder-Bridge operations into a
>>> helper
>>>        function *without* changing the logic. Patch 12/13 then changes
>>> the order
>>>        of the encoder-bridge operations as was intended in the
>>> original patch.
>>>      - Add detailed comment for patch 13/13.
>>>      - Add Tomi Valkeinen's R-b in patches 1, 2, 4, 5, 7, 8, 9, 13.
>>>
>>>    - Changes in v4:
>>>      - Add new patch, "drm/bridge: cdns-dsi: Move to
>>> devm_drm_of_get_bridge()",
>>>        to update to an auto-managed way of finding next bridge in the
>>> chain.
>>>      - Drop patch "drm/bridge: cdns-dsi: Fix the phy_initialized
>>> variable" and
>>>        add "drm/bridge: cdns-dsi: Fix Phy _init() and _exit()" that
>>> properly
>>>        de-initializes the Phy and maintains the initialization state.
>>>      - Reword patch "drm/bridge: cdns-dsi: Reset the DCS write FIFO"
>>> to explain
>>>        the HW concerns better.
>>>      - Add R-b tag from Dmitry Baryshkov for patches 1/11 and 8/11.
>>>
>>>    - Changes in v3:
>>>      - Reword the commit message for patch "drm/bridge: cdns-dsi: Fix
>>> OF node
>>>        pointer".
>>>      - Add a new helper API to figure out DSI host input pixel format
>>>        in patch "drm/mipi-dsi: Add helper to find input format".
>>>      - Use a common function for bridge pre-enable and enable, and
>>> bridge disable
>>>        and post-disable, to avoid code duplication.
>>>      - Add T-b tag from Dominik Haller in patch 5/10. (Missed to add
>>> it in v2).
>>>      - Add R-b tag from Dmitry Baryshkov for patch 8/10.
>>>
>>>    - Changes in v2:
>>>      - Drop patch "drm/tidss: Add CRTC mode_fixup"
>>>      - Split patch "drm/bridge: cdns-dsi: Fix minor bugs" into 4
>>> separate ones
>>>      - Drop support for early_enable/late_disable APIs and instead re-
>>> order the
>>>        pre_enable / post_disable APIs to be called before / after
>>> crtc_enable /
>>>        crtc_disable.
>>>      - Drop support for early_enable/late_disable in cdns-dsi and use
>>>        pre_enable/post_disable APIs instead to do bridge enable/disable.
>>>
>>>
>>> Previous versions:
>>>
>>> v1:  https://lore.kernel.org/all/20240511153051.1355825-1-a-
>>> bhatia1@ti.com/
>>> v2:  https://lore.kernel.org/all/20240530093621.1925863-1-a-
>>> bhatia1@ti.com/
>>> v3:  https://lore.kernel.org/all/20240617105311.1587489-1-a-
>>> bhatia1@ti.com/
>>> v4:  https://lore.kernel.org/all/20240622110929.3115714-1-a-
>>> bhatia1@ti.com/
>>> v5:  https://lore.kernel.org/all/20241019195411.266860-1-
>>> aradhya.bhatia@linux.dev/
>>> v6:  https://lore.kernel.org/all/20250111192738.308889-1-
>>> aradhya.bhatia@linux.dev/
>>> v7:  https://lore.kernel.org/all/20250114055626.18816-1-
>>> aradhya.bhatia@linux.dev/
>>> v8:  https://lore.kernel.org/all/20250126191551.741957-1-
>>> aradhya.bhatia@linux.dev/
>>> v9:  https://lore.kernel.org/all/20250209121032.32655-1-
>>> aradhya.bhatia@linux.dev/
>>> v10: https://lore.kernel.org/all/20250226155228.564289-1-
>>> aradhya.bhatia@linux.dev/
>>> v11: https://lore.kernel.org/all/20250329113925.68204-1-
>>> aradhya.bhatia@linux.dev/
>>>
>>> ---
>>>
>>> Aradhya Bhatia (5):
>>>    drm/atomic-helper: Refactor crtc & encoder-bridge op loops into
>>>      separate functions
>>>    drm/atomic-helper: Separate out bridge pre_enable/post_disable from
>>>      enable/disable
>>>    drm/atomic-helper: Re-order bridge chain pre-enable and post-disable
>>>    drm/bridge: Update the bridge enable/disable doc
>>>    drm/bridge: cdns-dsi: Use pre_enable/post_disable to enable/disable
>>>
>>>   .../gpu/drm/bridge/cadence/cdns-dsi-core.c    |  64 +++--
>>>   drivers/gpu/drm/drm_atomic_helper.c           | 160 +++++++++--
>>>   include/drm/drm_bridge.h                      | 249 +++++++++++++-----
>>>   3 files changed, 355 insertions(+), 118 deletions(-)
>>>
>>>
>>> base-commit: dd717762761807452ca25634652e180a80349cd8
>>
> 

Re: [PATCH v12 0/5] drm/atomic-helper: Re-order CRTC and Bridge ops

[Thomas Zimmermann]

Hi

Am 03.06.25 um 14:48 schrieb Tomi Valkeinen:
> Hi Maxime, Maarten, Thomas,
>
> (You're the first three names returned with get_maintainers.pl =), so:)
>
> Ping on this.

I don't really dare to R-b this, as it can backfire quite a bit. Anyway, 
I'll look over the patches.

Best regards
Thomas

>
>   Tomi
>
> On 26/05/2025 13:40, Tomi Valkeinen wrote:
>> Hi Maxime, Maarten, Thomas,
>>
>> On 16/05/2025 15:23, Tomi Valkeinen wrote:
>>> Hi all,
>>>
>>> On 06/04/2025 16:16, Aradhya Bhatia wrote:
>>>> Hello all,
>>>>
>>>> This series re-orders the sequences in which the drm CRTC and the drm
>>>> Bridge get enabled and disabled with respect to each other.
>>>>
>>>> The bridge pre_enable calls have been shifted before the crtc_enable and
>>>> the bridge post_disable calls have been shifted after the crtc_disable.
>>>>
>>>> This has been done as per the definition of bridge pre_enable.
>>>> "The display pipe (i.e. clocks and timing signals) feeding this bridge
>>>> will
>>>> not yet be running when this callback is called".
>>>>
>>>> Since CRTC is also a source feeding the bridge, it should not be enabled
>>>> before the bridges in the pipeline are pre_enabled.
>>> Any further comments to this?
>>>
>>> All the patches have been reviewed and tested. I can push this, along
>>> with the serieses that depend on this, via drm-misc, but I'll need an
>>> ack from a maintainer.
>> Would any of you have a moment to check this out and ack/apply? This
>> series is blocking further development on DSI and OLDI.
>>
>>   Tomi
>>
>>>   Tomi
>>>
>>>
>>>> The original sequence. for display pipe enable looks like:
>>>>
>>>>         crtc_enable
>>>>
>>>>         bridge[n]_pre_enable
>>>>         ...
>>>>         bridge[1]_pre_enable
>>>>
>>>>         encoder_enable
>>>>
>>>>         bridge[1]_enable
>>>>         ...
>>>>         bridge[n]_enable
>>>>
>>>> The sequence of enable after this patch-set will look like:
>>>>
>>>>         bridge[n]_pre_enable
>>>>         ...
>>>>         bridge[1]_pre_enable
>>>>
>>>>         crtc_enable
>>>>         encoder_enable
>>>>
>>>>         bridge[1]_enable
>>>>         ...
>>>>         bridge[n]_enable
>>>>
>>>>
>>>> For the disable sequence, this is what the original looks like:
>>>>
>>>>         bridge[n]_disable
>>>>         ...
>>>>         bridge[1]_disable
>>>>
>>>>         encoder_disable
>>>>
>>>>         bridge[1]_post_disable
>>>>         ...
>>>>         bridge[n]_post_disable
>>>>
>>>>         crtc_disable
>>>>      
>>>> This is what the disable sequence will be, after this series of patches:
>>>>
>>>>         bridge[n]_disable
>>>>         ...
>>>>         bridge[1]_disable
>>>>
>>>>         encoder_disable
>>>>         crtc_disable
>>>>
>>>>         bridge[1]_post_disable
>>>>         ...
>>>>         bridge[n]_post_disable
>>>>
>>>> This series further updates the bridge API definitions to accurately
>>>> reflect the updated scenario.
>>>>
>>>> This series is a subset of its v11[0] which had 14 patches in the
>>>> revision.
>>>> 9 of those 14 patches (which were specific to the cdns-dsi bridge driver)
>>>> were merged[1].
>>>>
>>>> Regards
>>>> Aradhya
>>>>
>>>> ---
>>>> * Note on checkpatch warning in patch 2/4 *
>>>> Patch 2/4 causes the checkpatch to flare up for 1 checkpatch 'check' -
>>>>
>>>> CHECK: Lines should not end with a '('
>>>> #79: FILE: drivers/gpu/drm/drm_atomic_helper.c:1304:
>>>> +                       new_crtc_state = drm_atomic_get_new_crtc_state(
>>>>
>>>> This patch is largely duplicating the original code, with minor
>>>> differences to
>>>> perform different operations. This line of code pre-exists in the file
>>>> and
>>>> have simply been duplicated. I have decided to keep it as is to
>>>> maintain the
>>>> uniformity and the originally intended readability. Should perhaps a
>>>> fix be
>>>> required, this patch/series is not the right place, and another patch
>>>> can be
>>>> created to fix this across the whole file.
>>>>
>>>> References:
>>>> [0]: Revision v11 of this series.
>>>> https://lore.kernel.org/all/20250329113925.68204-1-
>>>> aradhya.bhatia@linux.dev/
>>>>
>>>> [1]: Patches 1 through 9 getting merged.
>>>> https://lore.kernel.org/
>>>> all/174335361171.2556605.12634785416741695829.b4-ty@oss.qualcomm.com/
>>>>
>>>>
>>>> ---
>>>> Change Log:
>>>>
>>>>     - Changes in v12:
>>>>       - Drop patches 1 through 9 since they have been merged.
>>>>       - Rebase onto newer drm-misc-next.
>>>>       - Re-word the patch 3/4, ("drm/bridge: Update the bridge enable/
>>>> disable doc")
>>>>         to make it more readable.
>>>>
>>>>     - Changes in v11:
>>>>       - Add patch v11:13/14 ("drm/bridge: Update the bridge enable/
>>>> disable doc"),
>>>>         that updates the documentation about the order of the various
>>>> bridge
>>>>         enable/disable hooks being called wrt the CRTC and encoder hooks.
>>>>       - Rebase on drm-misc-next instead of linux-next.
>>>>         As part of rebase, accommodate the following change:
>>>>         - Change patch v10:08/13 ("drm/bridge: cdns-dsi: Support atomic
>>>> bridge
>>>>           APIs") to v11:08/13 ("drm/bridge: cdns-dsi: Add input format
>>>>           negotiation"), since Maxime has already updated the bridge
>>>> hooks to
>>>>           their atomic versions in commit 68c98e227a96 ("drm/bridge:
>>>> cdns-csi:
>>>>           Switch to atomic helpers").
>>>>           My new patch now only adds the format negotiation hook for
>>>> the cdns-dsi.
>>>>           (Note: Since the new patch is now only a subset of the old
>>>> one, without
>>>>           any change in logic, I decided to carry forward the R-b and
>>>> T-b tags.)
>>>>       - Add Alexander Sverdlin's T-b in patches 10, 11, 12.
>>>>
>>>>     - Changes in v10:
>>>>       - Rebase on latest linux-next (next-20250226).
>>>>       - As part of rebase, update the patches to accommodate a couple of
>>>>         widespread changes in DRM Framework -
>>>>           - All the ("drm/atomic-helper: Change parameter name of ***")
>>>> commits.
>>>>           - All the ("drm/bridge: Pass full state to ***") commits.
>>>>         (These updates are only trivial substitutions.)
>>>>       - Add Tomi Valkeinen's T-b tags in all the patches.
>>>>
>>>>     - Changes in v9:
>>>>       - Fix the oops in 11/13 - where the encoder_bridge_enable _was_
>>>> pre_enabling
>>>>         the bridges instead of enabling.
>>>>       - Add the following tags:
>>>>         - Dmitry Baryshkov's R-b in patches 2, 10, 11, and A-b in patch
>>>> 12.
>>>>         - Jayesh Choudhary's R-b in patch 12.
>>>>         - Tomi Valkeinen's R-b in patches 2, 10, 11, 12.
>>>>
>>>>     - Changes in v8:
>>>>       - Move the phy de-initialization to bridge post_disable() instead
>>>> of bridge
>>>>         disable() in patch-3.
>>>>       - Copy the private bridge state (dsi_cfg), in addition to the
>>>> bridge_state,
>>>>         in patch-9.
>>>>       - Split patch v7:11/12 into three patches, v8:{10,11,12}/13, to
>>>> separate out
>>>>         different refactorings into different patches, and improve
>>>> bisectability.
>>>>       - Move patch v7:02/12 down to v8:06/12, to keep the initial
>>>> patches for
>>>>         fixes only.
>>>>       - Drop patch v7:04/12 as it doesn't become relevant until patch
>>>> v7:12/12.
>>>>       - Add R-b tags of Dmitry Baryshkov in patch-9 and patch-3, and of
>>>>         Tomi Valkeinen in patch-9.
>>>>         - Changes in v7:
>>>>       - phy_init()/exit() were called from the PM path in v6. Change it
>>>> back to
>>>>         the bridge enable/disable path in patch-3, so that the
>>>> phy_init() can go
>>>>         back to being called after D-Phy reset assert.
>>>>       - Reword commit text in patch-5 to explain the need of the fix.
>>>>       - Drop the stray code in patch-10.
>>>>       - Add R-b tag of Dmitry Baryshkov in patch-6.
>>>>
>>>>     - Changes in v6:
>>>>       - Reword patch 3 to better explain the fixes around phy de-init.
>>>>       - Fix the Lane ready timeout condition in patch 7.
>>>>       - Fix the dsi _bridge_atomic_check() implementation by adding a new
>>>>         bridge state structure in patch 10.
>>>>       - Rework and combine patches v5:11/13 and v5:12/13 to v6:11/12.
>>>>       - Generate the patches of these series using the "patience"
>>>> algorithm.
>>>>         Note: All patches, except v6:11/12, *do not* differ from their
>>>> default
>>>>         (greedy) algorithm variants.
>>>>         For patch 11, the patience algorithm significantly improves the
>>>> readability.
>>>>       - Rename and move the Bridge enable/disable enums from public to
>>>> private
>>>>         in patch 11.
>>>>       - Add R-b tags of Tomi Valkeinen in patch 6, and Dmitry Baryshkov
>>>> in patch 2.
>>>>
>>>>     - Changes in v5:
>>>>       - Fix subject and description in patch 1/13.
>>>>       - Add patch to check the return value of
>>>>         phy_mipi_dphy_get_default_config() (patch: 6/13).
>>>>       - Change the Clk and Data Lane ready timeout from forever to 5s.
>>>>       - Print an error instead of calling WARN_ON_ONCE in patch 7/13.
>>>>       - Drop patch v4-07/11: "drm/bridge: cdns-dsi: Reset the DCS write
>>>> FIFO".
>>>>         There has been some inconsistencies found with this patch upon
>>>> further
>>>>         testing. This patch was being used to enable a DSI panel based
>>>> on ILITEK
>>>>         ILI9881C bridge. This will be debugged separately.
>>>>       - Add patch to move the DSI mode check from _atomic_enable() to
>>>>         _atomic_check() (patch: 10/13).
>>>>       - Split patch v4-10/11 into 2 patches - 11/13 and 12/13.
>>>>         Patch 11/13 separates out the Encoder-Bridge operations into a
>>>> helper
>>>>         function *without* changing the logic. Patch 12/13 then changes
>>>> the order
>>>>         of the encoder-bridge operations as was intended in the
>>>> original patch.
>>>>       - Add detailed comment for patch 13/13.
>>>>       - Add Tomi Valkeinen's R-b in patches 1, 2, 4, 5, 7, 8, 9, 13.
>>>>
>>>>     - Changes in v4:
>>>>       - Add new patch, "drm/bridge: cdns-dsi: Move to
>>>> devm_drm_of_get_bridge()",
>>>>         to update to an auto-managed way of finding next bridge in the
>>>> chain.
>>>>       - Drop patch "drm/bridge: cdns-dsi: Fix the phy_initialized
>>>> variable" and
>>>>         add "drm/bridge: cdns-dsi: Fix Phy _init() and _exit()" that
>>>> properly
>>>>         de-initializes the Phy and maintains the initialization state.
>>>>       - Reword patch "drm/bridge: cdns-dsi: Reset the DCS write FIFO"
>>>> to explain
>>>>         the HW concerns better.
>>>>       - Add R-b tag from Dmitry Baryshkov for patches 1/11 and 8/11.
>>>>
>>>>     - Changes in v3:
>>>>       - Reword the commit message for patch "drm/bridge: cdns-dsi: Fix
>>>> OF node
>>>>         pointer".
>>>>       - Add a new helper API to figure out DSI host input pixel format
>>>>         in patch "drm/mipi-dsi: Add helper to find input format".
>>>>       - Use a common function for bridge pre-enable and enable, and
>>>> bridge disable
>>>>         and post-disable, to avoid code duplication.
>>>>       - Add T-b tag from Dominik Haller in patch 5/10. (Missed to add
>>>> it in v2).
>>>>       - Add R-b tag from Dmitry Baryshkov for patch 8/10.
>>>>
>>>>     - Changes in v2:
>>>>       - Drop patch "drm/tidss: Add CRTC mode_fixup"
>>>>       - Split patch "drm/bridge: cdns-dsi: Fix minor bugs" into 4
>>>> separate ones
>>>>       - Drop support for early_enable/late_disable APIs and instead re-
>>>> order the
>>>>         pre_enable / post_disable APIs to be called before / after
>>>> crtc_enable /
>>>>         crtc_disable.
>>>>       - Drop support for early_enable/late_disable in cdns-dsi and use
>>>>         pre_enable/post_disable APIs instead to do bridge enable/disable.
>>>>
>>>>
>>>> Previous versions:
>>>>
>>>> v1:  https://lore.kernel.org/all/20240511153051.1355825-1-a-
>>>> bhatia1@ti.com/
>>>> v2:  https://lore.kernel.org/all/20240530093621.1925863-1-a-
>>>> bhatia1@ti.com/
>>>> v3:  https://lore.kernel.org/all/20240617105311.1587489-1-a-
>>>> bhatia1@ti.com/
>>>> v4:  https://lore.kernel.org/all/20240622110929.3115714-1-a-
>>>> bhatia1@ti.com/
>>>> v5:  https://lore.kernel.org/all/20241019195411.266860-1-
>>>> aradhya.bhatia@linux.dev/
>>>> v6:  https://lore.kernel.org/all/20250111192738.308889-1-
>>>> aradhya.bhatia@linux.dev/
>>>> v7:  https://lore.kernel.org/all/20250114055626.18816-1-
>>>> aradhya.bhatia@linux.dev/
>>>> v8:  https://lore.kernel.org/all/20250126191551.741957-1-
>>>> aradhya.bhatia@linux.dev/
>>>> v9:  https://lore.kernel.org/all/20250209121032.32655-1-
>>>> aradhya.bhatia@linux.dev/
>>>> v10: https://lore.kernel.org/all/20250226155228.564289-1-
>>>> aradhya.bhatia@linux.dev/
>>>> v11: https://lore.kernel.org/all/20250329113925.68204-1-
>>>> aradhya.bhatia@linux.dev/
>>>>
>>>> ---
>>>>
>>>> Aradhya Bhatia (5):
>>>>     drm/atomic-helper: Refactor crtc & encoder-bridge op loops into
>>>>       separate functions
>>>>     drm/atomic-helper: Separate out bridge pre_enable/post_disable from
>>>>       enable/disable
>>>>     drm/atomic-helper: Re-order bridge chain pre-enable and post-disable
>>>>     drm/bridge: Update the bridge enable/disable doc
>>>>     drm/bridge: cdns-dsi: Use pre_enable/post_disable to enable/disable
>>>>
>>>>    .../gpu/drm/bridge/cadence/cdns-dsi-core.c    |  64 +++--
>>>>    drivers/gpu/drm/drm_atomic_helper.c           | 160 +++++++++--
>>>>    include/drm/drm_bridge.h                      | 249 +++++++++++++-----
>>>>    3 files changed, 355 insertions(+), 118 deletions(-)
>>>>
>>>>
>>>> base-commit: dd717762761807452ca25634652e180a80349cd8

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Re: [PATCH v12 1/5] drm/atomic-helper: Refactor crtc & encoder-bridge op loops into separate functions

[Thomas Zimmermann]

Am 06.04.25 um 15:16 schrieb Aradhya Bhatia:
> From: Aradhya Bhatia <a-bhatia1@ti.com>
>
> The way any singular display pipeline, in need of a modeset, gets
> enabled is as follows -
>
> 	crtc enable
> 	(all) bridge pre-enable
> 	encoder enable
> 	(all) bridge enable
>
> - and the disable sequence is exactly the reverse of this.
>
> The crtc operations occur by looping over the old and new crtc states,
> while the encoder and bridge operations occur together, by looping over
> the connector states of the display pipelines.
>
> Refactor these operations - crtc enable/disable, and encoder & bridge
> (pre/post) enable/disable - into separate functions each, to make way
> for the re-ordering of the enable/disable sequences.
>
> This patch doesn't alter the sequence of crtc/encoder/bridge operations
> in any way, but helps to cleanly pave the way for the next two patches,
> by maintaining logical bisectability.
>
> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Tested-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> Tested-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
> Signed-off-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

> ---
>   drivers/gpu/drm/drm_atomic_helper.c | 69 ++++++++++++++++++++---------
>   1 file changed, 49 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index ee64ca1b1bec..d185486071c5 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1160,11 +1160,10 @@ crtc_needs_disable(struct drm_crtc_state *old_state,
>   }
>   
>   static void
> -disable_outputs(struct drm_device *dev, struct drm_atomic_state *state)
> +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;
> -	struct drm_crtc *crtc;
>   	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
>   	int i;
>   
> @@ -1227,6 +1226,14 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *state)
>   
>   		drm_atomic_bridge_chain_post_disable(bridge, state);
>   	}
> +}
> +
> +static void
> +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;
> +	int i;
>   
>   	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
>   		const struct drm_crtc_helper_funcs *funcs;
> @@ -1274,6 +1281,14 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *state)
>   	}
>   }
>   
> +static void
> +disable_outputs(struct drm_device *dev, struct drm_atomic_state *state)
> +{
> +	encoder_bridge_disable(dev, state);
> +
> +	crtc_disable(dev, state);
> +}
> +
>   /**
>    * drm_atomic_helper_update_legacy_modeset_state - update legacy modeset state
>    * @dev: DRM device
> @@ -1483,28 +1498,12 @@ static void drm_atomic_helper_commit_writebacks(struct drm_device *dev,
>   	}
>   }
>   
> -/**
> - * drm_atomic_helper_commit_modeset_enables - modeset commit to enable outputs
> - * @dev: DRM device
> - * @state: atomic state object being committed
> - *
> - * This function enables all the outputs with the new configuration which had to
> - * be turned off for the update.
> - *
> - * For compatibility with legacy CRTC helpers this should be called after
> - * drm_atomic_helper_commit_planes(), which is what the default commit function
> - * does. But drivers with different needs can group the modeset commits together
> - * and do the plane commits at the end. This is useful for drivers doing runtime
> - * PM since planes updates then only happen when the CRTC is actually enabled.
> - */
> -void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
> -					      struct drm_atomic_state *state)
> +static void
> +crtc_enable(struct drm_device *dev, struct drm_atomic_state *state)
>   {
>   	struct drm_crtc *crtc;
>   	struct drm_crtc_state *old_crtc_state;
>   	struct drm_crtc_state *new_crtc_state;
> -	struct drm_connector *connector;
> -	struct drm_connector_state *new_conn_state;
>   	int i;
>   
>   	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> @@ -1528,6 +1527,14 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
>   				funcs->commit(crtc);
>   		}
>   	}
> +}
> +
> +static void
> +encoder_bridge_enable(struct drm_device *dev, struct drm_atomic_state *state)
> +{
> +	struct drm_connector *connector;
> +	struct drm_connector_state *new_conn_state;
> +	int i;
>   
>   	for_each_new_connector_in_state(state, connector, new_conn_state, i) {
>   		const struct drm_encoder_helper_funcs *funcs;
> @@ -1565,6 +1572,28 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
>   
>   		drm_atomic_bridge_chain_enable(bridge, state);
>   	}
> +}
> +
> +/**
> + * drm_atomic_helper_commit_modeset_enables - modeset commit to enable outputs
> + * @dev: DRM device
> + * @state: atomic state object being committed
> + *
> + * This function enables all the outputs with the new configuration which had to
> + * be turned off for the update.
> + *
> + * For compatibility with legacy CRTC helpers this should be called after
> + * drm_atomic_helper_commit_planes(), which is what the default commit function
> + * does. But drivers with different needs can group the modeset commits together
> + * and do the plane commits at the end. This is useful for drivers doing runtime
> + * PM since planes updates then only happen when the CRTC is actually enabled.
> + */
> +void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
> +					      struct drm_atomic_state *state)
> +{
> +	crtc_enable(dev, state);
> +
> +	encoder_bridge_enable(dev, state);
>   
>   	drm_atomic_helper_commit_writebacks(dev, state);
>   }

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Re: [PATCH v12 2/5] drm/atomic-helper: Separate out bridge pre_enable/post_disable from enable/disable

[Thomas Zimmermann]

Am 06.04.25 um 15:16 schrieb Aradhya Bhatia:
> The encoder-bridge ops occur by looping over the new connector states of
> the display pipelines. The enable sequence runs as follows -
>
> 	- pre_enable(bridge),
> 	- enable(encoder),
> 	- enable(bridge),
>
> while the disable sequnce runs as follows -
>
> 	- disable(bridge),
> 	- disable(encoder),
> 	- post_disable(bridge).
>
> Separate out the pre_enable(bridge), and the post_disable(bridge)
> operations into separate functions each.
>
> This patch keeps the sequence same for any singular disaplay pipe, but
> changes the sequence across multiple display pipelines.
>
> This patch is meant to be an interim patch, to cleanly pave the way for
> the sequence re-ordering patch, and maintain bisectability in the
> process.
>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Reviewed-by: Jayesh Choudhary <j-choudhary@ti.com>
> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> Tested-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> Tested-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> Signed-off-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>
> ---
>   drivers/gpu/drm/drm_atomic_helper.c | 91 ++++++++++++++++++++++++++++-
>   1 file changed, 88 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index d185486071c5..86824f769623 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1223,8 +1223,6 @@ encoder_bridge_disable(struct drm_device *dev, struct drm_atomic_state *state)
>   			else if (funcs->dpms)
>   				funcs->dpms(encoder, DRM_MODE_DPMS_OFF);
>   		}
> -
> -		drm_atomic_bridge_chain_post_disable(bridge, state);
>   	}
>   }
>   
> @@ -1281,11 +1279,65 @@ crtc_disable(struct drm_device *dev, struct drm_atomic_state *state)
>   	}
>   }
>   
> +static void
> +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;
> +	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
> +	int i;
> +
> +	for_each_oldnew_connector_in_state(state, connector, old_conn_state, new_conn_state, i) {
> +		struct drm_encoder *encoder;
> +		struct drm_bridge *bridge;
> +
> +		/*
> +		 * Shut down everything that's in the changeset and currently
> +		 * still on. So need to check the old, saved state.
> +		 */
> +		if (!old_conn_state->crtc)
> +			continue;
> +
> +		old_crtc_state = drm_atomic_get_old_crtc_state(state, old_conn_state->crtc);
> +
> +		if (new_conn_state->crtc)
> +			new_crtc_state = drm_atomic_get_new_crtc_state(
> +						state,
> +						new_conn_state->crtc);

Indention is odd here. You can use up to 100 chars per line.

> +		else
> +			new_crtc_state = NULL;
> +
> +		if (!crtc_needs_disable(old_crtc_state, new_crtc_state) ||
> +		    !drm_atomic_crtc_needs_modeset(old_conn_state->crtc->state))
> +			continue;
> +
> +		encoder = old_conn_state->best_encoder;
> +
> +		/* We shouldn't get this far if we didn't previously have
> +		 * an encoder.. but WARN_ON() rather than explode.
> +		 */

Comment style should use an empty line first

  /*
   * foo bla
   */

With style issues fixed:

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

> +		if (WARN_ON(!encoder))
> +			continue;
> +
> +		drm_dbg_atomic(dev, "post-disabling bridges [ENCODER:%d:%s]\n",
> +			       encoder->base.id, encoder->name);
> +
> +		/*
> +		 * Each encoder has at most one connector (since we always steal
> +		 * it away), so we won't call disable hooks twice.
> +		 */
> +		bridge = drm_bridge_chain_get_first_bridge(encoder);
> +		drm_atomic_bridge_chain_post_disable(bridge, state);
> +	}
> +}
> +
>   static void
>   disable_outputs(struct drm_device *dev, struct drm_atomic_state *state)
>   {
>   	encoder_bridge_disable(dev, state);
>   
> +	encoder_bridge_post_disable(dev, state);
> +
>   	crtc_disable(dev, state);
>   }
>   
> @@ -1498,6 +1550,38 @@ static void drm_atomic_helper_commit_writebacks(struct drm_device *dev,
>   	}
>   }
>   
> +static void
> +encoder_bridge_pre_enable(struct drm_device *dev, struct drm_atomic_state *state)
> +{
> +	struct drm_connector *connector;
> +	struct drm_connector_state *new_conn_state;
> +	int i;
> +
> +	for_each_new_connector_in_state(state, connector, new_conn_state, i) {
> +		struct drm_encoder *encoder;
> +		struct drm_bridge *bridge;
> +
> +		if (!new_conn_state->best_encoder)
> +			continue;
> +
> +		if (!new_conn_state->crtc->state->active ||
> +		    !drm_atomic_crtc_needs_modeset(new_conn_state->crtc->state))
> +			continue;
> +
> +		encoder = new_conn_state->best_encoder;
> +
> +		drm_dbg_atomic(dev, "pre-enabling bridges [ENCODER:%d:%s]\n",
> +			       encoder->base.id, encoder->name);
> +
> +		/*
> +		 * Each encoder has at most one connector (since we always steal
> +		 * it away), so we won't call enable hooks twice.
> +		 */
> +		bridge = drm_bridge_chain_get_first_bridge(encoder);
> +		drm_atomic_bridge_chain_pre_enable(bridge, state);
> +	}
> +}
> +
>   static void
>   crtc_enable(struct drm_device *dev, struct drm_atomic_state *state)
>   {
> @@ -1559,7 +1643,6 @@ encoder_bridge_enable(struct drm_device *dev, struct drm_atomic_state *state)
>   		 * it away), so we won't call enable hooks twice.
>   		 */
>   		bridge = drm_bridge_chain_get_first_bridge(encoder);
> -		drm_atomic_bridge_chain_pre_enable(bridge, state);
>   
>   		if (funcs) {
>   			if (funcs->atomic_enable)
> @@ -1593,6 +1676,8 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
>   {
>   	crtc_enable(dev, state);
>   
> +	encoder_bridge_pre_enable(dev, state);
> +
>   	encoder_bridge_enable(dev, state);
>   
>   	drm_atomic_helper_commit_writebacks(dev, state);

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Re: [PATCH v12 3/5] drm/atomic-helper: Re-order bridge chain pre-enable and post-disable

[Thomas Zimmermann]

Am 06.04.25 um 15:16 schrieb Aradhya Bhatia:
> Move the bridge pre_enable call before crtc enable, and the bridge
> post_disable call after the crtc disable.
>
> The sequence of enable after this patch will look like:
>
> 	bridge[n]_pre_enable
> 	...
> 	bridge[1]_pre_enable
>
> 	crtc_enable
> 	encoder_enable
>
> 	bridge[1]_enable
> 	...
> 	bridge[n]_enable
>
> And, the disable sequence for the display pipeline will look like:
>
> 	bridge[n]_disable
> 	...
> 	bridge[1]_disable
>
> 	encoder_disable
> 	crtc_disable
>
> 	bridge[1]_post_disable
> 	...
> 	bridge[n]_post_disable
>
> The definition of bridge pre_enable hook says that,
> "The display pipe (i.e. clocks and timing signals) feeding this bridge
> will not yet be running when this callback is called".
>
> Since CRTC is also a source feeding the bridge, it should not be enabled
> before the bridges in the pipeline are pre_enabled. Fix that by
> re-ordering the sequence of bridge pre_enable and bridge post_disable.
>
> Acked-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> Tested-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> Tested-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
> Signed-off-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

> ---
>   drivers/gpu/drm/drm_atomic_helper.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 86824f769623..db5aae15e75d 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1336,9 +1336,9 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *state)
>   {
>   	encoder_bridge_disable(dev, state);
>   
> -	encoder_bridge_post_disable(dev, state);
> -
>   	crtc_disable(dev, state);
> +
> +	encoder_bridge_post_disable(dev, state);
>   }
>   
>   /**
> @@ -1674,10 +1674,10 @@ encoder_bridge_enable(struct drm_device *dev, struct drm_atomic_state *state)
>   void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
>   					      struct drm_atomic_state *state)
>   {
> -	crtc_enable(dev, state);
> -
>   	encoder_bridge_pre_enable(dev, state);
>   
> +	crtc_enable(dev, state);
> +
>   	encoder_bridge_enable(dev, state);
>   
>   	drm_atomic_helper_commit_writebacks(dev, state);

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Re: [PATCH v12 4/5] drm/bridge: Update the bridge enable/disable doc

[Thomas Zimmermann]

Am 06.04.25 um 15:16 schrieb Aradhya Bhatia:
> Now that the bridges get pre-enabled before the CRTC is enabled, and get
> post-disabled after the CRTC is disabled, update the function
> descriptions to accurately reflect the updated scenario.
>
> The enable sequence for the display pipeline looks like:
>
> 	bridge[n]_pre_enable
> 	...
> 	bridge[1]_pre_enable
>
> 	crtc_enable
> 	encoder_enable
>
> 	bridge[1]_enable
> 	...
> 	bridge[n]_enable
>
> And, the disable sequence for the display pipeline looks like:
>
> 	bridge[n]_disable
> 	...
> 	bridge[1]_disable
>
> 	encoder_disable
> 	crtc_disable
>
> 	bridge[1]_post_disable
> 	...
> 	bridge[n]_post_disable
>
> Signed-off-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

I think this patch could be merged with patch 3.

> ---
>   include/drm/drm_bridge.h | 249 ++++++++++++++++++++++++++++-----------
>   1 file changed, 183 insertions(+), 66 deletions(-)
>
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index cdad3b78a195..e5ccbefa60a8 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -164,17 +164,33 @@ struct drm_bridge_funcs {
>   	/**
>   	 * @disable:
>   	 *
> -	 * This callback should disable the bridge. It is called right before
> -	 * the preceding element in the display pipe is disabled. If the
> -	 * preceding element is a bridge this means it's called before that
> -	 * bridge's @disable vfunc. If the preceding element is a &drm_encoder
> -	 * it's called right before the &drm_encoder_helper_funcs.disable,
> -	 * &drm_encoder_helper_funcs.prepare or &drm_encoder_helper_funcs.dpms
> -	 * hook.
> +	 * The @disable callback should disable the bridge.
>   	 *
>   	 * The bridge can assume that the display pipe (i.e. clocks and timing
>   	 * signals) feeding it is still running when this callback is called.
>   	 *
> +	 *
> +	 * If the preceding element is a &drm_bridge, then this is called before
> +	 * that bridge is disabled via one of:
> +	 *
> +	 * - &drm_bridge_funcs.disable
> +	 * - &drm_bridge_funcs.atomic_disable
> +	 *
> +	 * If the preceding element of the bridge is a display controller, then
> +	 * this callback is called before the encoder is disabled via one of:
> +	 *
> +	 * - &drm_encoder_helper_funcs.atomic_disable
> +	 * - &drm_encoder_helper_funcs.prepare
> +	 * - &drm_encoder_helper_funcs.disable
> +	 * - &drm_encoder_helper_funcs.dpms
> +	 *
> +	 * and the CRTC is disabled via one of:
> +	 *
> +	 * - &drm_crtc_helper_funcs.prepare
> +	 * - &drm_crtc_helper_funcs.atomic_disable
> +	 * - &drm_crtc_helper_funcs.disable
> +	 * - &drm_crtc_helper_funcs.dpms.
> +	 *
>   	 * The @disable callback is optional.
>   	 *
>   	 * NOTE:
> @@ -187,17 +203,34 @@ struct drm_bridge_funcs {
>   	/**
>   	 * @post_disable:
>   	 *
> -	 * This callback should disable the bridge. It is called right after the
> -	 * preceding element in the display pipe is disabled. If the preceding
> -	 * element is a bridge this means it's called after that bridge's
> -	 * @post_disable function. If the preceding element is a &drm_encoder
> -	 * it's called right after the encoder's
> -	 * &drm_encoder_helper_funcs.disable, &drm_encoder_helper_funcs.prepare
> -	 * or &drm_encoder_helper_funcs.dpms hook.
> -	 *
>   	 * The bridge must assume that the display pipe (i.e. clocks and timing
> -	 * signals) feeding it is no longer running when this callback is
> -	 * called.
> +	 * signals) feeding this bridge is no longer running when the
> +	 * @post_disable is called.
> +	 *
> +	 * This callback should perform all the actions required by the hardware
> +	 * after it has stopped receiving signals from the preceding element.
> +	 *
> +	 * If the preceding element is a &drm_bridge, then this is called after
> +	 * that bridge is post-disabled (unless marked otherwise by the
> +	 * @pre_enable_prev_first flag) via one of:
> +	 *
> +	 * - &drm_bridge_funcs.post_disable
> +	 * - &drm_bridge_funcs.atomic_post_disable
> +	 *
> +	 * If the preceding element of the bridge is a display controller, then
> +	 * this callback is called after the encoder is disabled via one of:
> +	 *
> +	 * - &drm_encoder_helper_funcs.atomic_disable
> +	 * - &drm_encoder_helper_funcs.prepare
> +	 * - &drm_encoder_helper_funcs.disable
> +	 * - &drm_encoder_helper_funcs.dpms
> +	 *
> +	 * and the CRTC is disabled via one of:
> +	 *
> +	 * - &drm_crtc_helper_funcs.prepare
> +	 * - &drm_crtc_helper_funcs.atomic_disable
> +	 * - &drm_crtc_helper_funcs.disable
> +	 * - &drm_crtc_helper_funcs.dpms
>   	 *
>   	 * The @post_disable callback is optional.
>   	 *
> @@ -240,18 +273,30 @@ struct drm_bridge_funcs {
>   	/**
>   	 * @pre_enable:
>   	 *
> -	 * This callback should enable the bridge. It is called right before
> -	 * the preceding element in the display pipe is enabled. If the
> -	 * preceding element is a bridge this means it's called before that
> -	 * bridge's @pre_enable function. If the preceding element is a
> -	 * &drm_encoder it's called right before the encoder's
> -	 * &drm_encoder_helper_funcs.enable, &drm_encoder_helper_funcs.commit or
> -	 * &drm_encoder_helper_funcs.dpms hook.
> -	 *
>   	 * The display pipe (i.e. clocks and timing signals) feeding this bridge
> -	 * will not yet be running when this callback is called. The bridge must
> -	 * not enable the display link feeding the next bridge in the chain (if
> -	 * there is one) when this callback is called.
> +	 * will not yet be running when the @pre_enable is called.
> +	 *
> +	 * This callback should perform all the necessary actions to prepare the
> +	 * bridge to accept signals from the preceding element.
> +	 *
> +	 * If the preceding element is a &drm_bridge, then this is called before
> +	 * that bridge is pre-enabled (unless marked otherwise by
> +	 * @pre_enable_prev_first flag) via one of:
> +	 *
> +	 * - &drm_bridge_funcs.pre_enable
> +	 * - &drm_bridge_funcs.atomic_pre_enable
> +	 *
> +	 * If the preceding element of the bridge is a display controller, then
> +	 * this callback is called before the CRTC is enabled via one of:
> +	 *
> +	 * - &drm_crtc_helper_funcs.atomic_enable
> +	 * - &drm_crtc_helper_funcs.commit
> +	 *
> +	 * and the encoder is enabled via one of:
> +	 *
> +	 * - &drm_encoder_helper_funcs.atomic_enable
> +	 * - &drm_encoder_helper_funcs.enable
> +	 * - &drm_encoder_helper_funcs.commit
>   	 *
>   	 * The @pre_enable callback is optional.
>   	 *
> @@ -265,19 +310,31 @@ struct drm_bridge_funcs {
>   	/**
>   	 * @enable:
>   	 *
> -	 * This callback should enable the bridge. It is called right after
> -	 * the preceding element in the display pipe is enabled. If the
> -	 * preceding element is a bridge this means it's called after that
> -	 * bridge's @enable function. If the preceding element is a
> -	 * &drm_encoder it's called right after the encoder's
> -	 * &drm_encoder_helper_funcs.enable, &drm_encoder_helper_funcs.commit or
> -	 * &drm_encoder_helper_funcs.dpms hook.
> +	 * The @enable callback should enable the bridge.
>   	 *
>   	 * The bridge can assume that the display pipe (i.e. clocks and timing
>   	 * signals) feeding it is running when this callback is called. This
>   	 * callback must enable the display link feeding the next bridge in the
>   	 * chain if there is one.
>   	 *
> +	 * If the preceding element is a &drm_bridge, then this is called after
> +	 * that bridge is enabled via one of:
> +	 *
> +	 * - &drm_bridge_funcs.enable
> +	 * - &drm_bridge_funcs.atomic_enable
> +	 *
> +	 * If the preceding element of the bridge is a display controller, then
> +	 * this callback is called after the CRTC is enabled via one of:
> +	 *
> +	 * - &drm_crtc_helper_funcs.atomic_enable
> +	 * - &drm_crtc_helper_funcs.commit
> +	 *
> +	 * and the encoder is enabled via one of:
> +	 *
> +	 * - &drm_encoder_helper_funcs.atomic_enable
> +	 * - &drm_encoder_helper_funcs.enable
> +	 * - drm_encoder_helper_funcs.commit
> +	 *
>   	 * The @enable callback is optional.
>   	 *
>   	 * NOTE:
> @@ -290,17 +347,30 @@ struct drm_bridge_funcs {
>   	/**
>   	 * @atomic_pre_enable:
>   	 *
> -	 * This callback should enable the bridge. It is called right before
> -	 * the preceding element in the display pipe is enabled. If the
> -	 * preceding element is a bridge this means it's called before that
> -	 * bridge's @atomic_pre_enable or @pre_enable function. If the preceding
> -	 * element is a &drm_encoder it's called right before the encoder's
> -	 * &drm_encoder_helper_funcs.atomic_enable hook.
> -	 *
>   	 * The display pipe (i.e. clocks and timing signals) feeding this bridge
> -	 * will not yet be running when this callback is called. The bridge must
> -	 * not enable the display link feeding the next bridge in the chain (if
> -	 * there is one) when this callback is called.
> +	 * will not yet be running when the @atomic_pre_enable is called.
> +	 *
> +	 * This callback should perform all the necessary actions to prepare the
> +	 * bridge to accept signals from the preceding element.
> +	 *
> +	 * If the preceding element is a &drm_bridge, then this is called before
> +	 * that bridge is pre-enabled (unless marked otherwise by
> +	 * @pre_enable_prev_first flag) via one of:
> +	 *
> +	 * - &drm_bridge_funcs.pre_enable
> +	 * - &drm_bridge_funcs.atomic_pre_enable
> +	 *
> +	 * If the preceding element of the bridge is a display controller, then
> +	 * this callback is called before the CRTC is enabled via one of:
> +	 *
> +	 * - &drm_crtc_helper_funcs.atomic_enable
> +	 * - &drm_crtc_helper_funcs.commit
> +	 *
> +	 * and the encoder is enabled via one of:
> +	 *
> +	 * - &drm_encoder_helper_funcs.atomic_enable
> +	 * - &drm_encoder_helper_funcs.enable
> +	 * - &drm_encoder_helper_funcs.commit
>   	 *
>   	 * The @atomic_pre_enable callback is optional.
>   	 */
> @@ -310,18 +380,31 @@ struct drm_bridge_funcs {
>   	/**
>   	 * @atomic_enable:
>   	 *
> -	 * This callback should enable the bridge. It is called right after
> -	 * the preceding element in the display pipe is enabled. If the
> -	 * preceding element is a bridge this means it's called after that
> -	 * bridge's @atomic_enable or @enable function. If the preceding element
> -	 * is a &drm_encoder it's called right after the encoder's
> -	 * &drm_encoder_helper_funcs.atomic_enable hook.
> +	 * The @atomic_enable callback should enable the bridge.
>   	 *
>   	 * The bridge can assume that the display pipe (i.e. clocks and timing
>   	 * signals) feeding it is running when this callback is called. This
>   	 * callback must enable the display link feeding the next bridge in the
>   	 * chain if there is one.
>   	 *
> +	 * If the preceding element is a &drm_bridge, then this is called after
> +	 * that bridge is enabled via one of:
> +	 *
> +	 * - &drm_bridge_funcs.enable
> +	 * - &drm_bridge_funcs.atomic_enable
> +	 *
> +	 * If the preceding element of the bridge is a display controller, then
> +	 * this callback is called after the CRTC is enabled via one of:
> +	 *
> +	 * - &drm_crtc_helper_funcs.atomic_enable
> +	 * - &drm_crtc_helper_funcs.commit
> +	 *
> +	 * and the encoder is enabled via one of:
> +	 *
> +	 * - &drm_encoder_helper_funcs.atomic_enable
> +	 * - &drm_encoder_helper_funcs.enable
> +	 * - drm_encoder_helper_funcs.commit
> +	 *
>   	 * The @atomic_enable callback is optional.
>   	 */
>   	void (*atomic_enable)(struct drm_bridge *bridge,
> @@ -329,16 +412,32 @@ struct drm_bridge_funcs {
>   	/**
>   	 * @atomic_disable:
>   	 *
> -	 * This callback should disable the bridge. It is called right before
> -	 * the preceding element in the display pipe is disabled. If the
> -	 * preceding element is a bridge this means it's called before that
> -	 * bridge's @atomic_disable or @disable vfunc. If the preceding element
> -	 * is a &drm_encoder it's called right before the
> -	 * &drm_encoder_helper_funcs.atomic_disable hook.
> +	 * The @atomic_disable callback should disable the bridge.
>   	 *
>   	 * The bridge can assume that the display pipe (i.e. clocks and timing
>   	 * signals) feeding it is still running when this callback is called.
>   	 *
> +	 * If the preceding element is a &drm_bridge, then this is called before
> +	 * that bridge is disabled via one of:
> +	 *
> +	 * - &drm_bridge_funcs.disable
> +	 * - &drm_bridge_funcs.atomic_disable
> +	 *
> +	 * If the preceding element of the bridge is a display controller, then
> +	 * this callback is called before the encoder is disabled via one of:
> +	 *
> +	 * - &drm_encoder_helper_funcs.atomic_disable
> +	 * - &drm_encoder_helper_funcs.prepare
> +	 * - &drm_encoder_helper_funcs.disable
> +	 * - &drm_encoder_helper_funcs.dpms
> +	 *
> +	 * and the CRTC is disabled via one of:
> +	 *
> +	 * - &drm_crtc_helper_funcs.prepare
> +	 * - &drm_crtc_helper_funcs.atomic_disable
> +	 * - &drm_crtc_helper_funcs.disable
> +	 * - &drm_crtc_helper_funcs.dpms.
> +	 *
>   	 * The @atomic_disable callback is optional.
>   	 */
>   	void (*atomic_disable)(struct drm_bridge *bridge,
> @@ -347,16 +446,34 @@ struct drm_bridge_funcs {
>   	/**
>   	 * @atomic_post_disable:
>   	 *
> -	 * This callback should disable the bridge. It is called right after the
> -	 * preceding element in the display pipe is disabled. If the preceding
> -	 * element is a bridge this means it's called after that bridge's
> -	 * @atomic_post_disable or @post_disable function. If the preceding
> -	 * element is a &drm_encoder it's called right after the encoder's
> -	 * &drm_encoder_helper_funcs.atomic_disable hook.
> -	 *
>   	 * The bridge must assume that the display pipe (i.e. clocks and timing
> -	 * signals) feeding it is no longer running when this callback is
> -	 * called.
> +	 * signals) feeding this bridge is no longer running when the
> +	 * @atomic_post_disable is called.
> +	 *
> +	 * This callback should perform all the actions required by the hardware
> +	 * after it has stopped receiving signals from the preceding element.
> +	 *
> +	 * If the preceding element is a &drm_bridge, then this is called after
> +	 * that bridge is post-disabled (unless marked otherwise by the
> +	 * @pre_enable_prev_first flag) via one of:
> +	 *
> +	 * - &drm_bridge_funcs.post_disable
> +	 * - &drm_bridge_funcs.atomic_post_disable
> +	 *
> +	 * If the preceding element of the bridge is a display controller, then
> +	 * this callback is called after the encoder is disabled via one of:
> +	 *
> +	 * - &drm_encoder_helper_funcs.atomic_disable
> +	 * - &drm_encoder_helper_funcs.prepare
> +	 * - &drm_encoder_helper_funcs.disable
> +	 * - &drm_encoder_helper_funcs.dpms
> +	 *
> +	 * and the CRTC is disabled via one of:
> +	 *
> +	 * - &drm_crtc_helper_funcs.prepare
> +	 * - &drm_crtc_helper_funcs.atomic_disable
> +	 * - &drm_crtc_helper_funcs.disable
> +	 * - &drm_crtc_helper_funcs.dpms
>   	 *
>   	 * The @atomic_post_disable callback is optional.
>   	 */

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)