diff mbox series

[v5,11/13] drm/atomic-helper: Separate out Encoder-Bridge enable and disable

Message ID 20241019200530.270738-4-aradhya.bhatia@linux.dev (mailing list archive)
State New
Headers show
Series drm/bridge: cdns-dsi: Fix the color-shift issue | expand

Commit Message

Aradhya Bhatia Oct. 19, 2024, 8:05 p.m. UTC 
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 path is exactly the reverse of this.

The CRTC enable/disable occurs by looping over the old and new CRTC
states, while the bridge pre-enable, encoder enable, and bridge enable
occur by looping through the new connector states of the display
pipelines.

At the moment, the encoder and bridge operations are grouped together
and occur under the same loops.

Separate out the enable/disable loops of the encoder and bridge
operations into a different function, to make way for the re-ordering of
the enable and disable sequence of all these display elements.

This patch doesn't alter in any way the ordering of CRTC/encoder/bridge
enable and disable, but merely helps to cleanly set up for the next
patch and to make sure that the bisectibility remains intact.

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 | 97 +++++++++++++++++------------
 1 file changed, 57 insertions(+), 40 deletions(-)

Comments

Dmitry Baryshkov Oct. 20, 2024, 11:47 a.m. UTC | #1 
On Sun, Oct 20, 2024 at 01:35:28AM +0530, Aradhya Bhatia wrote:
> 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 path is exactly the reverse of this.
> 
> The CRTC enable/disable occurs by looping over the old and new CRTC
> states, while the bridge pre-enable, encoder enable, and bridge enable
> occur by looping through the new connector states of the display
> pipelines.
> 
> At the moment, the encoder and bridge operations are grouped together
> and occur under the same loops.
> 
> Separate out the enable/disable loops of the encoder and bridge
> operations into a different function, to make way for the re-ordering of
> the enable and disable sequence of all these display elements.
> 
> This patch doesn't alter in any way the ordering of CRTC/encoder/bridge
> enable and disable, but merely helps to cleanly set up for the next
> patch and to make sure that the bisectibility remains intact.
> 
> 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 | 97 +++++++++++++++++------------
>  1 file changed, 57 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 5186d2114a50..7741fbcc8fc7 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1122,11 +1122,10 @@ crtc_needs_disable(struct drm_crtc_state *old_state,
>  }
>  
>  static void
> -disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
> +encoder_bridge_chain_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;
>  
> @@ -1189,6 +1188,16 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
>  
>  		drm_atomic_bridge_chain_post_disable(bridge, old_state);
>  	}
> +}
> +
> +static void
> +disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
> +{
> +	struct drm_crtc *crtc;
> +	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
> +	int i;
> +
> +	encoder_bridge_chain_disable(dev, 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;
> @@ -1445,6 +1454,51 @@ static void drm_atomic_helper_commit_writebacks(struct drm_device *dev,
>  	}
>  }
>  
> +static void
> +encoder_bridge_chain_enable(struct drm_device *dev, struct drm_atomic_state *old_state)
> +{
> +	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;
> +		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
> @@ -1465,8 +1519,6 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
>  	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) {
> @@ -1491,42 +1543,7 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
>  		}
>  	}

I'd say, if you want for it to be cleaner, split both loops from
drm_atomic_helper_commit_modeset_enables() and call them separately.
This save you from the code move, which is harder to review (and also
helps with the function complexity).

>  
> -	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);
> -	}
> +	encoder_bridge_chain_enable(dev, old_state);
>  
>  	drm_atomic_helper_commit_writebacks(dev, old_state);
>  }
> -- 
> 2.34.1
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 5186d2114a50..7741fbcc8fc7 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1122,11 +1122,10 @@  crtc_needs_disable(struct drm_crtc_state *old_state,
 }
 
 static void
-disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
+encoder_bridge_chain_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;
 
@@ -1189,6 +1188,16 @@  disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
 
 		drm_atomic_bridge_chain_post_disable(bridge, old_state);
 	}
+}
+
+static void
+disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
+{
+	struct drm_crtc *crtc;
+	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
+	int i;
+
+	encoder_bridge_chain_disable(dev, 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;
@@ -1445,6 +1454,51 @@  static void drm_atomic_helper_commit_writebacks(struct drm_device *dev,
 	}
 }
 
+static void
+encoder_bridge_chain_enable(struct drm_device *dev, struct drm_atomic_state *old_state)
+{
+	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;
+		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
@@ -1465,8 +1519,6 @@  void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
 	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) {
@@ -1491,42 +1543,7 @@  void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
 		}
 	}
 
-	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);
-	}
+	encoder_bridge_chain_enable(dev, old_state);
 
 	drm_atomic_helper_commit_writebacks(dev, old_state);
 }