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.

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 | 300 +++++++++++++++++-----------
 1 file changed, 181 insertions(+), 119 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 5186d2114a50..ad6290a4a528 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -74,6 +74,12 @@
  * also shares the &struct drm_plane_helper_funcs function table with the plane
  * helpers.
  */
+
+enum bridge_chain_operation_type {
+	DRM_BRIDGE_PRE_ENABLE_OR_POST_DISABLE,
+	DRM_BRIDGE_ENABLE_OR_DISABLE,
+};
+
 static void
 drm_atomic_helper_plane_changed(struct drm_atomic_state *state,
 				struct drm_plane_state *old_plane_state,
@@ -1122,74 +1128,12 @@ crtc_needs_disable(struct drm_crtc_state *old_state,
 }
 
 static void
-disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
+crtc_disable(struct drm_device *dev, struct drm_atomic_state *old_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;
 
-	for_each_oldnew_connector_in_state(old_state, connector, old_conn_state, new_conn_state, i) {
-		const struct drm_encoder_helper_funcs *funcs;
-		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(old_state, old_conn_state->crtc);
-
-		if (new_conn_state->crtc)
-			new_crtc_state = drm_atomic_get_new_crtc_state(
-						old_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;
-
-		funcs = encoder->helper_private;
-
-		drm_dbg_atomic(dev, "disabling [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_disable(bridge, old_state);
-
-		/* Right function depends upon target state. */
-		if (funcs) {
-			if (funcs->atomic_disable)
-				funcs->atomic_disable(encoder, old_state);
-			else if (new_conn_state->crtc && funcs->prepare)
-				funcs->prepare(encoder);
-			else if (funcs->disable)
-				funcs->disable(encoder);
-			else if (funcs->dpms)
-				funcs->dpms(encoder, DRM_MODE_DPMS_OFF);
-		}
-
-		drm_atomic_bridge_chain_post_disable(bridge, old_state);
-	}
-
 	for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) {
 		const struct drm_crtc_helper_funcs *funcs;
 		int ret;
@@ -1206,7 +1150,6 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
 		drm_dbg_atomic(dev, "disabling [CRTC:%d:%s]\n",
 			       crtc->base.id, crtc->name);
 
-
 		/* Right function depends upon target state. */
 		if (new_crtc_state->enable && funcs->prepare)
 			funcs->prepare(crtc);
@@ -1236,6 +1179,97 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
 	}
 }
 
+static void
+encoder_bridge_chain_disable(struct drm_device *dev, struct drm_atomic_state *old_state,
+			     enum bridge_chain_operation_type op_type)
+{
+	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(old_state, connector, old_conn_state, new_conn_state, i) {
+		const struct drm_encoder_helper_funcs *funcs;
+		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(old_state, old_conn_state->crtc);
+
+		if (new_conn_state->crtc)
+			new_crtc_state = drm_atomic_get_new_crtc_state(
+						old_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;
+
+		/*
+		 * 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);
+
+		switch (op_type) {
+		case DRM_BRIDGE_ENABLE_OR_DISABLE:
+			funcs = encoder->helper_private;
+
+			drm_dbg_atomic(dev, "disabling [ENCODER:%d:%s]\n",
+				       encoder->base.id, encoder->name);
+
+			drm_atomic_bridge_chain_disable(bridge, old_state);
+
+			/* Right function depends upon target state. */
+			if (funcs) {
+				if (funcs->atomic_disable)
+					funcs->atomic_disable(encoder, old_state);
+				else if (new_conn_state->crtc && funcs->prepare)
+					funcs->prepare(encoder);
+				else if (funcs->disable)
+					funcs->disable(encoder);
+				else if (funcs->dpms)
+					funcs->dpms(encoder, DRM_MODE_DPMS_OFF);
+			}
+
+			break;
+
+		case DRM_BRIDGE_PRE_ENABLE_OR_POST_DISABLE:
+			drm_atomic_bridge_chain_post_disable(bridge, old_state);
+			break;
+
+		default:
+			drm_err(dev, "Unrecognized Encoder/Bridge operation (%d).\n", op_type);
+		}
+	}
+}
+
+static void
+disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
+{
+	encoder_bridge_chain_disable(dev, old_state, DRM_BRIDGE_ENABLE_OR_DISABLE);
+
+	crtc_disable(dev, old_state);
+
+	encoder_bridge_chain_disable(dev, old_state, DRM_BRIDGE_PRE_ENABLE_OR_POST_DISABLE);
+}
+
 /**
  * drm_atomic_helper_update_legacy_modeset_state - update legacy modeset state
  * @dev: DRM device
@@ -1445,28 +1479,69 @@ 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
- * @old_state: atomic state object with old state structures
- *
- * 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 *old_state)
+static void
+encoder_bridge_chain_enable(struct drm_device *dev, struct drm_atomic_state *old_state,
+			    enum bridge_chain_operation_type op_type)
+{
+	struct drm_connector *connector;
+	struct drm_connector_state *new_conn_state;
+	int i;
+
+	for_each_new_connector_in_state(old_state, connector, new_conn_state, i) {
+		const struct drm_encoder_helper_funcs *funcs;
+		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;
+		/*
+		 * 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);
+
+		switch (op_type) {
+		case DRM_BRIDGE_PRE_ENABLE_OR_POST_DISABLE:
+			drm_atomic_bridge_chain_pre_enable(bridge, old_state);
+			break;
+
+		case DRM_BRIDGE_ENABLE_OR_DISABLE:
+			funcs = encoder->helper_private;
+
+			drm_dbg_atomic(dev, "enabling [ENCODER:%d:%s]\n",
+				       encoder->base.id, encoder->name);
+
+			if (funcs) {
+				if (funcs->atomic_enable)
+					funcs->atomic_enable(encoder, old_state);
+				else if (funcs->enable)
+					funcs->enable(encoder);
+				else if (funcs->commit)
+					funcs->commit(encoder);
+			}
+
+			drm_atomic_bridge_chain_enable(bridge, old_state);
+			break;
+
+		default:
+			drm_err(dev, "Unrecognized Encoder/Bridge Operation (%d).\n", op_type);
+			break;
+		}
+	}
+}
+
+static void
+crtc_enable(struct drm_device *dev, struct drm_atomic_state *old_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(old_state, crtc, old_crtc_state, new_crtc_state, i) {
@@ -1490,43 +1565,30 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
 				funcs->commit(crtc);
 		}
 	}
-
-	for_each_new_connector_in_state(old_state, connector, new_conn_state, i) {
-		const struct drm_encoder_helper_funcs *funcs;
-		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;
-		funcs = encoder->helper_private;
-
-		drm_dbg_atomic(dev, "enabling [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, old_state);
-
-		if (funcs) {
-			if (funcs->atomic_enable)
-				funcs->atomic_enable(encoder, old_state);
-			else if (funcs->enable)
-				funcs->enable(encoder);
-			else if (funcs->commit)
-				funcs->commit(encoder);
-		}
-
-		drm_atomic_bridge_chain_enable(bridge, old_state);
-	}
+}
+
+/**
+ * drm_atomic_helper_commit_modeset_enables - modeset commit to enable outputs
+ * @dev: DRM device
+ * @old_state: atomic state object with old state structures
+ *
+ * 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 *old_state)
+{
+	encoder_bridge_chain_enable(dev, old_state, DRM_BRIDGE_PRE_ENABLE_OR_POST_DISABLE);
+
+	crtc_enable(dev, old_state);
+
+	encoder_bridge_chain_enable(dev, old_state, DRM_BRIDGE_ENABLE_OR_DISABLE);
 
 	drm_atomic_helper_commit_writebacks(dev, old_state);
 }
-- 
2.34.1

On Tue, Jan 14, 2025 at 11:26:25AM +0530, Aradhya Bhatia wrote:
> 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.

The patch contains both refactoring of the corresponding functions and
changing of the order. Please split it into two separate commits.

> 
> 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 | 300 +++++++++++++++++-----------
>  1 file changed, 181 insertions(+), 119 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 5186d2114a50..ad6290a4a528 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -74,6 +74,12 @@
>   * also shares the &struct drm_plane_helper_funcs function table with the plane
>   * helpers.
>   */
> +
> +enum bridge_chain_operation_type {
> +	DRM_BRIDGE_PRE_ENABLE_OR_POST_DISABLE,
> +	DRM_BRIDGE_ENABLE_OR_DISABLE,
> +};
> +

I have mixed feelings towards this approach. I doubt that it actually
helps. Would you mind replacing it with just 'bool pre_enable' / 'bool
post_disable' arguments?

>  static void
>  drm_atomic_helper_plane_changed(struct drm_atomic_state *state,
>  				struct drm_plane_state *old_plane_state,
> @@ -1122,74 +1128,12 @@ crtc_needs_disable(struct drm_crtc_state *old_state,
>  }
>  
>  static void
> -disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
> +crtc_disable(struct drm_device *dev, struct drm_atomic_state *old_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;
>  
> -	for_each_oldnew_connector_in_state(old_state, connector, old_conn_state, new_conn_state, i) {
> -		const struct drm_encoder_helper_funcs *funcs;
> -		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(old_state, old_conn_state->crtc);
> -
> -		if (new_conn_state->crtc)
> -			new_crtc_state = drm_atomic_get_new_crtc_state(
> -						old_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;
> -
> -		funcs = encoder->helper_private;
> -
> -		drm_dbg_atomic(dev, "disabling [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_disable(bridge, old_state);
> -
> -		/* Right function depends upon target state. */
> -		if (funcs) {
> -			if (funcs->atomic_disable)
> -				funcs->atomic_disable(encoder, old_state);
> -			else if (new_conn_state->crtc && funcs->prepare)
> -				funcs->prepare(encoder);
> -			else if (funcs->disable)
> -				funcs->disable(encoder);
> -			else if (funcs->dpms)
> -				funcs->dpms(encoder, DRM_MODE_DPMS_OFF);
> -		}
> -
> -		drm_atomic_bridge_chain_post_disable(bridge, old_state);
> -	}
> -
>  	for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) {
>  		const struct drm_crtc_helper_funcs *funcs;
>  		int ret;
> @@ -1206,7 +1150,6 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
>  		drm_dbg_atomic(dev, "disabling [CRTC:%d:%s]\n",
>  			       crtc->base.id, crtc->name);
>  
> -

Irrelevant, please drop.

>  		/* Right function depends upon target state. */
>  		if (new_crtc_state->enable && funcs->prepare)
>  			funcs->prepare(crtc);
> @@ -1236,6 +1179,97 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
>  	}
>  }
>  
> +static void
> +encoder_bridge_chain_disable(struct drm_device *dev, struct drm_atomic_state *old_state,
> +			     enum bridge_chain_operation_type op_type)
> +{
> +	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(old_state, connector, old_conn_state, new_conn_state, i) {
> +		const struct drm_encoder_helper_funcs *funcs;
> +		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(old_state, old_conn_state->crtc);
> +
> +		if (new_conn_state->crtc)
> +			new_crtc_state = drm_atomic_get_new_crtc_state(
> +						old_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;
> +
> +		/*
> +		 * 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);
> +
> +		switch (op_type) {
> +		case DRM_BRIDGE_ENABLE_OR_DISABLE:
> +			funcs = encoder->helper_private;
> +
> +			drm_dbg_atomic(dev, "disabling [ENCODER:%d:%s]\n",
> +				       encoder->base.id, encoder->name);
> +
> +			drm_atomic_bridge_chain_disable(bridge, old_state);
> +
> +			/* Right function depends upon target state. */
> +			if (funcs) {
> +				if (funcs->atomic_disable)
> +					funcs->atomic_disable(encoder, old_state);
> +				else if (new_conn_state->crtc && funcs->prepare)
> +					funcs->prepare(encoder);
> +				else if (funcs->disable)
> +					funcs->disable(encoder);
> +				else if (funcs->dpms)
> +					funcs->dpms(encoder, DRM_MODE_DPMS_OFF);
> +			}
> +
> +			break;
> +
> +		case DRM_BRIDGE_PRE_ENABLE_OR_POST_DISABLE:
> +			drm_atomic_bridge_chain_post_disable(bridge, old_state);
> +			break;
> +
> +		default:
> +			drm_err(dev, "Unrecognized Encoder/Bridge operation (%d).\n", op_type);
> +		}
> +	}
> +}
> +
> +static void
> +disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
> +{
> +	encoder_bridge_chain_disable(dev, old_state, DRM_BRIDGE_ENABLE_OR_DISABLE);
> +
> +	crtc_disable(dev, old_state);
> +
> +	encoder_bridge_chain_disable(dev, old_state, DRM_BRIDGE_PRE_ENABLE_OR_POST_DISABLE);
> +}
> +
>  /**
>   * drm_atomic_helper_update_legacy_modeset_state - update legacy modeset state
>   * @dev: DRM device
> @@ -1445,28 +1479,69 @@ 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
> - * @old_state: atomic state object with old state structures
> - *
> - * 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 *old_state)
> +static void
> +encoder_bridge_chain_enable(struct drm_device *dev, struct drm_atomic_state *old_state,
> +			    enum bridge_chain_operation_type op_type)
> +{
> +	struct drm_connector *connector;
> +	struct drm_connector_state *new_conn_state;
> +	int i;
> +
> +	for_each_new_connector_in_state(old_state, connector, new_conn_state, i) {
> +		const struct drm_encoder_helper_funcs *funcs;
> +		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;
> +		/*
> +		 * 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);
> +
> +		switch (op_type) {
> +		case DRM_BRIDGE_PRE_ENABLE_OR_POST_DISABLE:
> +			drm_atomic_bridge_chain_pre_enable(bridge, old_state);
> +			break;
> +
> +		case DRM_BRIDGE_ENABLE_OR_DISABLE:
> +			funcs = encoder->helper_private;
> +
> +			drm_dbg_atomic(dev, "enabling [ENCODER:%d:%s]\n",
> +				       encoder->base.id, encoder->name);
> +
> +			if (funcs) {
> +				if (funcs->atomic_enable)
> +					funcs->atomic_enable(encoder, old_state);
> +				else if (funcs->enable)
> +					funcs->enable(encoder);
> +				else if (funcs->commit)
> +					funcs->commit(encoder);
> +			}
> +
> +			drm_atomic_bridge_chain_enable(bridge, old_state);
> +			break;
> +
> +		default:
> +			drm_err(dev, "Unrecognized Encoder/Bridge Operation (%d).\n", op_type);
> +			break;
> +		}
> +	}
> +}
> +
> +static void
> +crtc_enable(struct drm_device *dev, struct drm_atomic_state *old_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(old_state, crtc, old_crtc_state, new_crtc_state, i) {
> @@ -1490,43 +1565,30 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
>  				funcs->commit(crtc);
>  		}
>  	}
> -
> -	for_each_new_connector_in_state(old_state, connector, new_conn_state, i) {
> -		const struct drm_encoder_helper_funcs *funcs;
> -		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;
> -		funcs = encoder->helper_private;
> -
> -		drm_dbg_atomic(dev, "enabling [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, old_state);
> -
> -		if (funcs) {
> -			if (funcs->atomic_enable)
> -				funcs->atomic_enable(encoder, old_state);
> -			else if (funcs->enable)
> -				funcs->enable(encoder);
> -			else if (funcs->commit)
> -				funcs->commit(encoder);
> -		}
> -
> -		drm_atomic_bridge_chain_enable(bridge, old_state);
> -	}
> +}
> +
> +/**
> + * drm_atomic_helper_commit_modeset_enables - modeset commit to enable outputs
> + * @dev: DRM device
> + * @old_state: atomic state object with old state structures
> + *
> + * 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 *old_state)
> +{
> +	encoder_bridge_chain_enable(dev, old_state, DRM_BRIDGE_PRE_ENABLE_OR_POST_DISABLE);
> +
> +	crtc_enable(dev, old_state);
> +
> +	encoder_bridge_chain_enable(dev, old_state, DRM_BRIDGE_ENABLE_OR_DISABLE);
>  
>  	drm_atomic_helper_commit_writebacks(dev, old_state);
>  }
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

Hi Dmitry,

On 14/01/25 16:54, Dmitry Baryshkov wrote:
> On Tue, Jan 14, 2025 at 11:26:25AM +0530, Aradhya Bhatia wrote:
>> 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.
> 
> The patch contains both refactoring of the corresponding functions and
> changing of the order. Please split it into two separate commits.
> 

I assume that you already understand this, so this is just for the
record -

There is no trivial way to do this. Initially, this is what I had in my
mind - about what the split patches would look like.

#1
 * refactoring of entire loops into separate functions.
 * Separate out the pre_enable and enable operations inside the
   encoder_bridge_enable().
 * call them in their (seemingly) original sequences
	- crtc_enable
	- encoder_bridge_enable(pre_enable)
	- encoder_bridge_enable(!pre_enable)

#2
 * rearrange the calls to re-order the operation
	- encoder_bridge_enable(pre_enable)
	- crtc_enable
	- encoder_bridge_enable(!pre_enable)

This would have made the patch#2 seem quite trivial, as patch#1 would
take the brunt of changes, while keeping the functionality intact.


What I have now realized is that, the above isn't possible,
unfortunately. The moment we separate out pre_enable and enable into 2
different calls, the overall sequence gets even changed when there are
multiple pipelines in the system.

So to implement the split properly, the first patch would look like this

#1
 * refactoring of entire loops into separate functions.
 * The calling sequences will be as follows -
 	- crtc_enable()
	- encoder_bridge_enable()
		- this will do both pre_enable and enable
		  unconditionally.

The pre_enable and enable operations wouldn't be separated in patch 1,
just that the crtc enable and encoder bridge pre_enable/enable loops
would be put into individual functions.

The next patch will then introduce booleans, and separate out pre_enable
and enable, and implement the new order -

#2
 * pre_enable and enable operations will be conditionally segregated
   inside encoder_bridge_enable(), based on the pre_enable boolean.
 * The calling sequences will then be updated to -
	- encoder_bridge_enable(pre_enable)
	- crtc_enable()
	- encoder_bridge_enable(!pre_enable)

This wouldn't be all that much trivial as I had imagined it to be earlier.

It would also *kind of* like these patches in a previous revision,
v5:11/13 [0], and v5:12/13 [1]. The only differences being,

1) these older patches separated out only the bridge/encoder operations
into a function, and not the crtc operations, and

2) An enum is being used instead of the booleans.


I believe this is what you are looking for? If I have misunderstood
something, do let me know.


Regards
Aradhya


[0]: v5:11/13
drm/atomic-helper: Separate out Encoder-Bridge enable and disable
https://lore.kernel.org/all/20241019200530.270738-4-aradhya.bhatia@linux.dev/

[1]: v5:12/13
drm/atomic-helper: Re-order bridge chain pre-enable and post-disable
https://lore.kernel.org/all/20241019200530.270738-5-aradhya.bhatia@linux.dev/

On Fri, Jan 17, 2025 at 06:37:00PM +0530, Aradhya Bhatia wrote:
> Hi Dmitry,
> 
> On 14/01/25 16:54, Dmitry Baryshkov wrote:
> > On Tue, Jan 14, 2025 at 11:26:25AM +0530, Aradhya Bhatia wrote:
> >> 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.
> > 
> > The patch contains both refactoring of the corresponding functions and
> > changing of the order. Please split it into two separate commits.
> > 
> 
> I assume that you already understand this, so this is just for the
> record -
> 
> There is no trivial way to do this. Initially, this is what I had in my
> mind - about what the split patches would look like.
> 
> #1
>  * refactoring of entire loops into separate functions.
>  * Separate out the pre_enable and enable operations inside the
>    encoder_bridge_enable().
>  * call them in their (seemingly) original sequences
> 	- crtc_enable
> 	- encoder_bridge_enable(pre_enable)
> 	- encoder_bridge_enable(!pre_enable)
> 
> #2
>  * rearrange the calls to re-order the operation
> 	- encoder_bridge_enable(pre_enable)
> 	- crtc_enable
> 	- encoder_bridge_enable(!pre_enable)
> 
> This would have made the patch#2 seem quite trivial, as patch#1 would
> take the brunt of changes, while keeping the functionality intact.
> 
> 
> What I have now realized is that, the above isn't possible,
> unfortunately. The moment we separate out pre_enable and enable into 2
> different calls, the overall sequence gets even changed when there are
> multiple pipelines in the system.
> 
> So to implement the split properly, the first patch would look like this
> 
> #1
>  * refactoring of entire loops into separate functions.
>  * The calling sequences will be as follows -
>  	- crtc_enable()
> 	- encoder_bridge_enable()
> 		- this will do both pre_enable and enable
> 		  unconditionally.
> 
> The pre_enable and enable operations wouldn't be separated in patch 1,
> just that the crtc enable and encoder bridge pre_enable/enable loops
> would be put into individual functions.
> 
> The next patch will then introduce booleans, and separate out pre_enable
> and enable, and implement the new order -
> 
> #2
>  * pre_enable and enable operations will be conditionally segregated
>    inside encoder_bridge_enable(), based on the pre_enable boolean.
>  * The calling sequences will then be updated to -
> 	- encoder_bridge_enable(pre_enable)
> 	- crtc_enable()
> 	- encoder_bridge_enable(!pre_enable)


I'd say slightly differently:

#1 Refactor loops into separate functions:
  - crtc_enable()
  - encoder_bridge_enable(), loops over encoders and calls
    pre_enable(bridges), enable(encoder), enable(bridges)

#2 Split loops into separate functions:
  - crtc_enable()
  - encoder_bridge_pre_enable(), loops over encoders and calls
    pre_enable(bridges),
  - encoder_bridge_enable(), loops over encoders and calls
    enable(encoder), enable(bridges)

#3 Move crtc_enable() calls:
  - encoder_bridge_pre_enable(), loops over encoders and calls
    pre_enable(bridges),
  - crtc_enable()
  - encoder_bridge_enable(), loops over encoders and calls
    enable(encoder), enable(bridges)

You might use enum or booleans to implement encoder_bridge_pre_enable(),
or just keep it as a completely separate function (might be a better
option).

The reason why I'm suggesting it is pretty easy: your patch performs two
refactorings at the same time. If one of the drivers breaks, we have no
way to identify, which of the refactorings caused the break.

> 
> This wouldn't be all that much trivial as I had imagined it to be earlier.
> 
> It would also *kind of* like these patches in a previous revision,
> v5:11/13 [0], and v5:12/13 [1]. The only differences being,
> 
> 1) these older patches separated out only the bridge/encoder operations
> into a function, and not the crtc operations, and
> 
> 2) An enum is being used instead of the booleans.
> 
> 
> I believe this is what you are looking for? If I have misunderstood
> something, do let me know.
> 
> 
> Regards
> Aradhya
> 
> 
> [0]: v5:11/13
> drm/atomic-helper: Separate out Encoder-Bridge enable and disable
> https://lore.kernel.org/all/20241019200530.270738-4-aradhya.bhatia@linux.dev/
> 
> [1]: v5:12/13
> drm/atomic-helper: Re-order bridge chain pre-enable and post-disable
> https://lore.kernel.org/all/20241019200530.270738-5-aradhya.bhatia@linux.dev/
> 

-- 
With best wishes
Dmitry

Hi Dmitry,

On 20/01/25 14:08, Dmitry Baryshkov wrote:
> On Fri, Jan 17, 2025 at 06:37:00PM +0530, Aradhya Bhatia wrote:
>> Hi Dmitry,
>>
>> On 14/01/25 16:54, Dmitry Baryshkov wrote:
>>> On Tue, Jan 14, 2025 at 11:26:25AM +0530, Aradhya Bhatia wrote:
>>>> 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.
>>>
>>> The patch contains both refactoring of the corresponding functions and
>>> changing of the order. Please split it into two separate commits.
>>>
>>
>> I assume that you already understand this, so this is just for the
>> record -
>>
>> There is no trivial way to do this. Initially, this is what I had in my
>> mind - about what the split patches would look like.
>>
>> #1
>>  * refactoring of entire loops into separate functions.
>>  * Separate out the pre_enable and enable operations inside the
>>    encoder_bridge_enable().
>>  * call them in their (seemingly) original sequences
>> 	- crtc_enable
>> 	- encoder_bridge_enable(pre_enable)
>> 	- encoder_bridge_enable(!pre_enable)
>>
>> #2
>>  * rearrange the calls to re-order the operation
>> 	- encoder_bridge_enable(pre_enable)
>> 	- crtc_enable
>> 	- encoder_bridge_enable(!pre_enable)
>>
>> This would have made the patch#2 seem quite trivial, as patch#1 would
>> take the brunt of changes, while keeping the functionality intact.
>>
>>
>> What I have now realized is that, the above isn't possible,
>> unfortunately. The moment we separate out pre_enable and enable into 2
>> different calls, the overall sequence gets even changed when there are
>> multiple pipelines in the system.
>>
>> So to implement the split properly, the first patch would look like this
>>
>> #1
>>  * refactoring of entire loops into separate functions.
>>  * The calling sequences will be as follows -
>>  	- crtc_enable()
>> 	- encoder_bridge_enable()
>> 		- this will do both pre_enable and enable
>> 		  unconditionally.
>>
>> The pre_enable and enable operations wouldn't be separated in patch 1,
>> just that the crtc enable and encoder bridge pre_enable/enable loops
>> would be put into individual functions.
>>
>> The next patch will then introduce booleans, and separate out pre_enable
>> and enable, and implement the new order -
>>
>> #2
>>  * pre_enable and enable operations will be conditionally segregated
>>    inside encoder_bridge_enable(), based on the pre_enable boolean.
>>  * The calling sequences will then be updated to -
>> 	- encoder_bridge_enable(pre_enable)
>> 	- crtc_enable()
>> 	- encoder_bridge_enable(!pre_enable)
> 
> 
> I'd say slightly differently:
> 
> #1 Refactor loops into separate functions:
>   - crtc_enable()
>   - encoder_bridge_enable(), loops over encoders and calls
>     pre_enable(bridges), enable(encoder), enable(bridges)
> 
> #2 Split loops into separate functions:
>   - crtc_enable()
>   - encoder_bridge_pre_enable(), loops over encoders and calls
>     pre_enable(bridges),
>   - encoder_bridge_enable(), loops over encoders and calls
>     enable(encoder), enable(bridges)
> 

When we consider setups with multiple independent displays, there are
often multiple crtcs in the system, each with their own encoder-bridge
chain.

In such a scenario, the sequence of crtc/encoder/bridge enable calls
after the #2 that you suggested, will not the remain same as that after
#1 (which is the _original_ sequence of calls).

Do we still require #2 as an intermediate step between the original
sequence, and the intended new sequence? Wouldn't having the sequence
change in 2 half-steps add to the confusion (should some driver break
due to either of the refactorings)?

> #3 Move crtc_enable() calls:
>   - encoder_bridge_pre_enable(), loops over encoders and calls
>     pre_enable(bridges),
>   - crtc_enable()
>   - encoder_bridge_enable(), loops over encoders and calls
>     enable(encoder), enable(bridges)
> 
> You might use enum or booleans to implement encoder_bridge_pre_enable(),
> or just keep it as a completely separate function (might be a better
> option).

Yeah, I suppose a separate function may be better. There will, however,
be some code duplication when we loop over the encoder twice, once for
pre_enable(bridge) and the other for enable(encoder) and enable(bridge).

I hope that will be acceptable?

> 
> The reason why I'm suggesting it is pretty easy: your patch performs two
> refactorings at the same time. If one of the drivers breaks, we have no
> way to identify, which of the refactorings caused the break.>
>>
>> This wouldn't be all that much trivial as I had imagined it to be earlier.
>>
>> It would also *kind of* like these patches in a previous revision,
>> v5:11/13 [0], and v5:12/13 [1]. The only differences being,
>>
>> 1) these older patches separated out only the bridge/encoder operations
>> into a function, and not the crtc operations, and
>>
>> 2) An enum is being used instead of the booleans.
>>
>>
>> I believe this is what you are looking for? If I have misunderstood
>> something, do let me know.
>>
>>
>> Regards
>> Aradhya
>>
>>
>> [0]: v5:11/13
>> drm/atomic-helper: Separate out Encoder-Bridge enable and disable
>> https://lore.kernel.org/all/20241019200530.270738-4-aradhya.bhatia@linux.dev/
>>
>> [1]: v5:12/13
>> drm/atomic-helper: Re-order bridge chain pre-enable and post-disable
>> https://lore.kernel.org/all/20241019200530.270738-5-aradhya.bhatia@linux.dev/
>>
> 


Regards
Aradhya

On Mon, Jan 20, 2025 at 11:18:22PM +0530, Aradhya Bhatia wrote:
> Hi Dmitry,
> 
> On 20/01/25 14:08, Dmitry Baryshkov wrote:
> > On Fri, Jan 17, 2025 at 06:37:00PM +0530, Aradhya Bhatia wrote:
> >> Hi Dmitry,
> >>
> >> On 14/01/25 16:54, Dmitry Baryshkov wrote:
> >>> On Tue, Jan 14, 2025 at 11:26:25AM +0530, Aradhya Bhatia wrote:
> >>>> 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.
> >>>
> >>> The patch contains both refactoring of the corresponding functions and
> >>> changing of the order. Please split it into two separate commits.
> >>>
> >>
> >> I assume that you already understand this, so this is just for the
> >> record -
> >>
> >> There is no trivial way to do this. Initially, this is what I had in my
> >> mind - about what the split patches would look like.
> >>
> >> #1
> >>  * refactoring of entire loops into separate functions.
> >>  * Separate out the pre_enable and enable operations inside the
> >>    encoder_bridge_enable().
> >>  * call them in their (seemingly) original sequences
> >> 	- crtc_enable
> >> 	- encoder_bridge_enable(pre_enable)
> >> 	- encoder_bridge_enable(!pre_enable)
> >>
> >> #2
> >>  * rearrange the calls to re-order the operation
> >> 	- encoder_bridge_enable(pre_enable)
> >> 	- crtc_enable
> >> 	- encoder_bridge_enable(!pre_enable)
> >>
> >> This would have made the patch#2 seem quite trivial, as patch#1 would
> >> take the brunt of changes, while keeping the functionality intact.
> >>
> >>
> >> What I have now realized is that, the above isn't possible,
> >> unfortunately. The moment we separate out pre_enable and enable into 2
> >> different calls, the overall sequence gets even changed when there are
> >> multiple pipelines in the system.
> >>
> >> So to implement the split properly, the first patch would look like this
> >>
> >> #1
> >>  * refactoring of entire loops into separate functions.
> >>  * The calling sequences will be as follows -
> >>  	- crtc_enable()
> >> 	- encoder_bridge_enable()
> >> 		- this will do both pre_enable and enable
> >> 		  unconditionally.
> >>
> >> The pre_enable and enable operations wouldn't be separated in patch 1,
> >> just that the crtc enable and encoder bridge pre_enable/enable loops
> >> would be put into individual functions.
> >>
> >> The next patch will then introduce booleans, and separate out pre_enable
> >> and enable, and implement the new order -
> >>
> >> #2
> >>  * pre_enable and enable operations will be conditionally segregated
> >>    inside encoder_bridge_enable(), based on the pre_enable boolean.
> >>  * The calling sequences will then be updated to -
> >> 	- encoder_bridge_enable(pre_enable)
> >> 	- crtc_enable()
> >> 	- encoder_bridge_enable(!pre_enable)
> > 
> > 
> > I'd say slightly differently:
> > 
> > #1 Refactor loops into separate functions:
> >   - crtc_enable()
> >   - encoder_bridge_enable(), loops over encoders and calls
> >     pre_enable(bridges), enable(encoder), enable(bridges)
> > 
> > #2 Split loops into separate functions:
> >   - crtc_enable()
> >   - encoder_bridge_pre_enable(), loops over encoders and calls
> >     pre_enable(bridges),
> >   - encoder_bridge_enable(), loops over encoders and calls
> >     enable(encoder), enable(bridges)
> > 
> 
> When we consider setups with multiple independent displays, there are
> often multiple crtcs in the system, each with their own encoder-bridge
> chain.
> 
> In such a scenario, the sequence of crtc/encoder/bridge enable calls
> after the #2 that you suggested, will not the remain same as that after
> #1 (which is the _original_ sequence of calls).

Yes. The order of ops between display has changed, but each display is
still programmed in exactly the same way as before.

> 
> Do we still require #2 as an intermediate step between the original
> sequence, and the intended new sequence? Wouldn't having the sequence
> change in 2 half-steps add to the confusion (should some driver break
> due to either of the refactorings)?

That's the point. Having two refactorings in one commit makes it harder
to understand, which one broke the driver. Having two separate commits
allows users to revert the latter commit and check what caused the
issue.

> 
> > #3 Move crtc_enable() calls:
> >   - encoder_bridge_pre_enable(), loops over encoders and calls
> >     pre_enable(bridges),
> >   - crtc_enable()
> >   - encoder_bridge_enable(), loops over encoders and calls
> >     enable(encoder), enable(bridges)
> > 
> > You might use enum or booleans to implement encoder_bridge_pre_enable(),
> > or just keep it as a completely separate function (might be a better
> > option).
> 
> Yeah, I suppose a separate function may be better. There will, however,
> be some code duplication when we loop over the encoder twice, once for
> pre_enable(bridge) and the other for enable(encoder) and enable(bridge).
> 
> I hope that will be acceptable?

I'd prefer two separate functions, but I won't insist on that.

> 
> > 
> > The reason why I'm suggesting it is pretty easy: your patch performs two
> > refactorings at the same time. If one of the drivers breaks, we have no
> > way to identify, which of the refactorings caused the break.>
> >>
> >> This wouldn't be all that much trivial as I had imagined it to be earlier.
> >>
> >> It would also *kind of* like these patches in a previous revision,
> >> v5:11/13 [0], and v5:12/13 [1]. The only differences being,
> >>
> >> 1) these older patches separated out only the bridge/encoder operations
> >> into a function, and not the crtc operations, and
> >>
> >> 2) An enum is being used instead of the booleans.
> >>
> >>
> >> I believe this is what you are looking for? If I have misunderstood
> >> something, do let me know.
> >>
> >>
> >> Regards
> >> Aradhya
> >>
> >>
> >> [0]: v5:11/13
> >> drm/atomic-helper: Separate out Encoder-Bridge enable and disable
> >> https://lore.kernel.org/all/20241019200530.270738-4-aradhya.bhatia@linux.dev/
> >>
> >> [1]: v5:12/13
> >> drm/atomic-helper: Re-order bridge chain pre-enable and post-disable
> >> https://lore.kernel.org/all/20241019200530.270738-5-aradhya.bhatia@linux.dev/
> >>
> > 
> 
> 
> Regards
> Aradhya
> 

-- 
With best wishes
Dmitry

Hi Dmitry,

On 21/01/25 16:20, Dmitry Baryshkov wrote:
> On Mon, Jan 20, 2025 at 11:18:22PM +0530, Aradhya Bhatia wrote:
>> Hi Dmitry,
>>
>> On 20/01/25 14:08, Dmitry Baryshkov wrote:
>>> On Fri, Jan 17, 2025 at 06:37:00PM +0530, Aradhya Bhatia wrote:
>>>> Hi Dmitry,
>>>>
>>>> On 14/01/25 16:54, Dmitry Baryshkov wrote:
>>>>> On Tue, Jan 14, 2025 at 11:26:25AM +0530, Aradhya Bhatia wrote:
>>>>>> 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.
>>>>>
>>>>> The patch contains both refactoring of the corresponding functions and
>>>>> changing of the order. Please split it into two separate commits.
>>>>>
>>>>
>>>> I assume that you already understand this, so this is just for the
>>>> record -
>>>>
>>>> There is no trivial way to do this. Initially, this is what I had in my
>>>> mind - about what the split patches would look like.
>>>>
>>>> #1
>>>>  * refactoring of entire loops into separate functions.
>>>>  * Separate out the pre_enable and enable operations inside the
>>>>    encoder_bridge_enable().
>>>>  * call them in their (seemingly) original sequences
>>>> 	- crtc_enable
>>>> 	- encoder_bridge_enable(pre_enable)
>>>> 	- encoder_bridge_enable(!pre_enable)
>>>>
>>>> #2
>>>>  * rearrange the calls to re-order the operation
>>>> 	- encoder_bridge_enable(pre_enable)
>>>> 	- crtc_enable
>>>> 	- encoder_bridge_enable(!pre_enable)
>>>>
>>>> This would have made the patch#2 seem quite trivial, as patch#1 would
>>>> take the brunt of changes, while keeping the functionality intact.
>>>>
>>>>
>>>> What I have now realized is that, the above isn't possible,
>>>> unfortunately. The moment we separate out pre_enable and enable into 2
>>>> different calls, the overall sequence gets even changed when there are
>>>> multiple pipelines in the system.
>>>>
>>>> So to implement the split properly, the first patch would look like this
>>>>
>>>> #1
>>>>  * refactoring of entire loops into separate functions.
>>>>  * The calling sequences will be as follows -
>>>>  	- crtc_enable()
>>>> 	- encoder_bridge_enable()
>>>> 		- this will do both pre_enable and enable
>>>> 		  unconditionally.
>>>>
>>>> The pre_enable and enable operations wouldn't be separated in patch 1,
>>>> just that the crtc enable and encoder bridge pre_enable/enable loops
>>>> would be put into individual functions.
>>>>
>>>> The next patch will then introduce booleans, and separate out pre_enable
>>>> and enable, and implement the new order -
>>>>
>>>> #2
>>>>  * pre_enable and enable operations will be conditionally segregated
>>>>    inside encoder_bridge_enable(), based on the pre_enable boolean.
>>>>  * The calling sequences will then be updated to -
>>>> 	- encoder_bridge_enable(pre_enable)
>>>> 	- crtc_enable()
>>>> 	- encoder_bridge_enable(!pre_enable)
>>>
>>>
>>> I'd say slightly differently:
>>>
>>> #1 Refactor loops into separate functions:
>>>   - crtc_enable()
>>>   - encoder_bridge_enable(), loops over encoders and calls
>>>     pre_enable(bridges), enable(encoder), enable(bridges)
>>>
>>> #2 Split loops into separate functions:
>>>   - crtc_enable()
>>>   - encoder_bridge_pre_enable(), loops over encoders and calls
>>>     pre_enable(bridges),
>>>   - encoder_bridge_enable(), loops over encoders and calls
>>>     enable(encoder), enable(bridges)
>>>
>>
>> When we consider setups with multiple independent displays, there are
>> often multiple crtcs in the system, each with their own encoder-bridge
>> chain.
>>
>> In such a scenario, the sequence of crtc/encoder/bridge enable calls
>> after the #2 that you suggested, will not the remain same as that after
>> #1 (which is the _original_ sequence of calls).
> 
> Yes. The order of ops between display has changed, but each display is
> still programmed in exactly the same way as before.

Yes, that's right. Sequence for each display will remain the same as
before.

> 
>>
>> Do we still require #2 as an intermediate step between the original
>> sequence, and the intended new sequence? Wouldn't having the sequence
>> change in 2 half-steps add to the confusion (should some driver break
>> due to either of the refactorings)?
> 
> That's the point. Having two refactorings in one commit makes it harder
> to understand, which one broke the driver. Having two separate commits
> allows users to revert the latter commit and check what caused the
> issue.

Right. It's easier to debug each display independently in multi-display
setups, and I can now understand how #2 will be able to help.

This explanation helped a lot. Thank you!

>>
>>> #3 Move crtc_enable() calls:
>>>   - encoder_bridge_pre_enable(), loops over encoders and calls
>>>     pre_enable(bridges),
>>>   - crtc_enable()
>>>   - encoder_bridge_enable(), loops over encoders and calls
>>>     enable(encoder), enable(bridges)
>>>
>>> You might use enum or booleans to implement encoder_bridge_pre_enable(),
>>> or just keep it as a completely separate function (might be a better
>>> option).
>>
>> Yeah, I suppose a separate function may be better. There will, however,
>> be some code duplication when we loop over the encoder twice, once for
>> pre_enable(bridge) and the other for enable(encoder) and enable(bridge).
>>
>> I hope that will be acceptable?
> 
> I'd prefer two separate functions, but I won't insist on that.

Alright!

I have my work cut out for me for the next revision.

> 
>>
>>>
>>> The reason why I'm suggesting it is pretty easy: your patch performs two
>>> refactorings at the same time. If one of the drivers breaks, we have no
>>> way to identify, which of the refactorings caused the break.>
>>>>
>>>> This wouldn't be all that much trivial as I had imagined it to be earlier.
>>>>
>>>> It would also *kind of* like these patches in a previous revision,
>>>> v5:11/13 [0], and v5:12/13 [1]. The only differences being,
>>>>
>>>> 1) these older patches separated out only the bridge/encoder operations
>>>> into a function, and not the crtc operations, and
>>>>
>>>> 2) An enum is being used instead of the booleans.
>>>>
>>>>
>>>> I believe this is what you are looking for? If I have misunderstood
>>>> something, do let me know.
>>>>
>>>>
>>>> Regards
>>>> Aradhya
>>>>
>>>>
>>>> [0]: v5:11/13
>>>> drm/atomic-helper: Separate out Encoder-Bridge enable and disable
>>>> https://lore.kernel.org/all/20241019200530.270738-4-aradhya.bhatia@linux.dev/
>>>>
>>>> [1]: v5:12/13
>>>> drm/atomic-helper: Re-order bridge chain pre-enable and post-disable
>>>> https://lore.kernel.org/all/20241019200530.270738-5-aradhya.bhatia@linux.dev/
>>>>
>>>
>>
>>
>> Regards
>> Aradhya
>>
> 

--
Regards
Aradhya

Hi,

On 14/01/2025 13:24, Dmitry Baryshkov wrote:
> On Tue, Jan 14, 2025 at 11:26:25AM +0530, Aradhya Bhatia wrote:
>> 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.
> 
> The patch contains both refactoring of the corresponding functions and
> changing of the order. Please split it into two separate commits.
> 
>>
>> 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 | 300 +++++++++++++++++-----------
>>   1 file changed, 181 insertions(+), 119 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>> index 5186d2114a50..ad6290a4a528 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -74,6 +74,12 @@
>>    * also shares the &struct drm_plane_helper_funcs function table with the plane
>>    * helpers.
>>    */
>> +
>> +enum bridge_chain_operation_type {
>> +	DRM_BRIDGE_PRE_ENABLE_OR_POST_DISABLE,
>> +	DRM_BRIDGE_ENABLE_OR_DISABLE,
>> +};
>> +
> 
> I have mixed feelings towards this approach. I doubt that it actually
> helps. Would you mind replacing it with just 'bool pre_enable' / 'bool
> post_disable' arguments?

If my memory serves, I suggested the enum. I don't like it too much 
either. But neither do I like the boolean that much, as this is not a 
yes/no, on/off case... Then again, maybe boolean is fine here, as the 
"off" case is the "normal/default" case so it's still ok-ish.

But this doesn't matter much, I think. It's internal, and can be 
trivially adjusted later.

  Tomi

On 1/14/25 18:34, Tomi Valkeinen wrote:
> Hi,
> 
> On 14/01/2025 13:24, Dmitry Baryshkov wrote:
>> On Tue, Jan 14, 2025 at 11:26:25AM +0530, Aradhya Bhatia wrote:
>>> 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.
>>
>> The patch contains both refactoring of the corresponding functions and
>> changing of the order. Please split it into two separate commits.
>>
>>>
>>> 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 | 300 +++++++++++++++++-----------
>>>   1 file changed, 181 insertions(+), 119 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
>>> b/drivers/gpu/drm/drm_atomic_helper.c
>>> index 5186d2114a50..ad6290a4a528 100644
>>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>>> @@ -74,6 +74,12 @@
>>>    * also shares the &struct drm_plane_helper_funcs function table
>>> with the plane
>>>    * helpers.
>>>    */
>>> +
>>> +enum bridge_chain_operation_type {
>>> +    DRM_BRIDGE_PRE_ENABLE_OR_POST_DISABLE,
>>> +    DRM_BRIDGE_ENABLE_OR_DISABLE,
>>> +};
>>> +
>>
>> I have mixed feelings towards this approach. I doubt that it actually
>> helps. Would you mind replacing it with just 'bool pre_enable' / 'bool
>> post_disable' arguments?
> 
> If my memory serves, I suggested the enum. I don't like it too much
> either. But neither do I like the boolean that much, as this is not a
> yes/no, on/off case... Then again, maybe boolean is fine here, as the
> "off" case is the "normal/default" case so it's still ok-ish.
> 
> But this doesn't matter much, I think. It's internal, and can be
> trivially adjusted later.
> 

Alright! I will drop the enum reference entirely, and just use the
booleans.

Regards
Aradhya

On Tue, Jan 14, 2025 at 10:05:56PM +0530, Aradhya Bhatia wrote:
> 
> 
> On 1/14/25 18:34, Tomi Valkeinen wrote:
> > Hi,
> > 
> > On 14/01/2025 13:24, Dmitry Baryshkov wrote:
> >> On Tue, Jan 14, 2025 at 11:26:25AM +0530, Aradhya Bhatia wrote:
> >>> 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.
> >>
> >> The patch contains both refactoring of the corresponding functions and
> >> changing of the order. Please split it into two separate commits.
> >>
> >>>
> >>> 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 | 300 +++++++++++++++++-----------
> >>>   1 file changed, 181 insertions(+), 119 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> >>> b/drivers/gpu/drm/drm_atomic_helper.c
> >>> index 5186d2114a50..ad6290a4a528 100644
> >>> --- a/drivers/gpu/drm/drm_atomic_helper.c
> >>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> >>> @@ -74,6 +74,12 @@
> >>>    * also shares the &struct drm_plane_helper_funcs function table
> >>> with the plane
> >>>    * helpers.
> >>>    */
> >>> +
> >>> +enum bridge_chain_operation_type {
> >>> +    DRM_BRIDGE_PRE_ENABLE_OR_POST_DISABLE,
> >>> +    DRM_BRIDGE_ENABLE_OR_DISABLE,
> >>> +};
> >>> +
> >>
> >> I have mixed feelings towards this approach. I doubt that it actually
> >> helps. Would you mind replacing it with just 'bool pre_enable' / 'bool
> >> post_disable' arguments?
> > 
> > If my memory serves, I suggested the enum. I don't like it too much
> > either. But neither do I like the boolean that much, as this is not a
> > yes/no, on/off case... Then again, maybe boolean is fine here, as the
> > "off" case is the "normal/default" case so it's still ok-ish.
> > 
> > But this doesn't matter much, I think. It's internal, and can be
> > trivially adjusted later.
> > 
> 
> Alright! I will drop the enum reference entirely, and just use the
> booleans.

Whatever you do, I think that we're at a point where the bridge chain
traversal is complicated enough that we'll want to have tests for all
cases.

Maxime

On 1/14/25 22:13, Maxime Ripard wrote:
> On Tue, Jan 14, 2025 at 10:05:56PM +0530, Aradhya Bhatia wrote:
>>
>>
>> On 1/14/25 18:34, Tomi Valkeinen wrote:
>>> Hi,
>>>
>>> On 14/01/2025 13:24, Dmitry Baryshkov wrote:
>>>> On Tue, Jan 14, 2025 at 11:26:25AM +0530, Aradhya Bhatia wrote:
>>>>> 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.
>>>>
>>>> The patch contains both refactoring of the corresponding functions and
>>>> changing of the order. Please split it into two separate commits.
>>>>
>>>>>
>>>>> 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 | 300 +++++++++++++++++-----------
>>>>>   1 file changed, 181 insertions(+), 119 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
>>>>> b/drivers/gpu/drm/drm_atomic_helper.c
>>>>> index 5186d2114a50..ad6290a4a528 100644
>>>>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>>>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>>>>> @@ -74,6 +74,12 @@
>>>>>    * also shares the &struct drm_plane_helper_funcs function table
>>>>> with the plane
>>>>>    * helpers.
>>>>>    */
>>>>> +
>>>>> +enum bridge_chain_operation_type {
>>>>> +    DRM_BRIDGE_PRE_ENABLE_OR_POST_DISABLE,
>>>>> +    DRM_BRIDGE_ENABLE_OR_DISABLE,
>>>>> +};
>>>>> +
>>>>
>>>> I have mixed feelings towards this approach. I doubt that it actually
>>>> helps. Would you mind replacing it with just 'bool pre_enable' / 'bool
>>>> post_disable' arguments?
>>>
>>> If my memory serves, I suggested the enum. I don't like it too much
>>> either. But neither do I like the boolean that much, as this is not a
>>> yes/no, on/off case... Then again, maybe boolean is fine here, as the
>>> "off" case is the "normal/default" case so it's still ok-ish.
>>>
>>> But this doesn't matter much, I think. It's internal, and can be
>>> trivially adjusted later.
>>>
>>
>> Alright! I will drop the enum reference entirely, and just use the
>> booleans.
> 
> Whatever you do, I think that we're at a point where the bridge chain
> traversal is complicated enough that we'll want to have tests for all
> cases.
>

I don't think I follow. Which all cases are you referring to?


Regards
Aradhya