[10/13] drm/i915: Fix port sync code to work with >2 pipes
diff mbox series

Message ID 20200313164831.5980-11-ville.syrjala@linux.intel.com
State New
Headers show
Series
  • drm/i915: Port sync for skl+
Related show

Commit Message

Ville Syrjälä March 13, 2020, 4:48 p.m. UTC
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Don't assume there is just one port sync slave. We might have several.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 98 ++++++++++----------
 1 file changed, 49 insertions(+), 49 deletions(-)

Comments

Souza, Jose April 3, 2020, 12:32 a.m. UTC | #1
On Fri, 2020-03-13 at 18:48 +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Don't assume there is just one port sync slave. We might have
> several.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 98 ++++++++++------
> ----
>  1 file changed, 49 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index b56a5a49418f..33f38c8a5da4 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -15009,18 +15009,6 @@ static void intel_update_crtc(struct
> intel_atomic_state *state,
>  		intel_crtc_arm_fifo_underrun(crtc, new_crtc_state);
>  }
>  
> -static struct intel_crtc *intel_get_slave_crtc(const struct
> intel_crtc_state *new_crtc_state)
> -{
> -	struct drm_i915_private *dev_priv = to_i915(new_crtc_state-
> >uapi.crtc->dev);
> -	enum transcoder slave_transcoder;
> -
> -	drm_WARN_ON(&dev_priv->drm,
> -		    !is_power_of_2(new_crtc_state-
> >sync_mode_slaves_mask));
> -
> -	slave_transcoder = ffs(new_crtc_state->sync_mode_slaves_mask) -
> 1;
> -	return intel_get_crtc_for_pipe(dev_priv,
> -				       (enum pipe)slave_transcoder);
> -}
>  
>  static void intel_old_crtc_state_disables(struct intel_atomic_state
> *state,
>  					  struct intel_crtc_state
> *old_crtc_state,
> @@ -15109,8 +15097,8 @@ static void
> intel_commit_modeset_enables(struct intel_atomic_state *state)
>  	}
>  }
>  
> -static void intel_set_dp_tp_ctl_normal(struct intel_crtc *crtc,
> -				       struct intel_atomic_state
> *state)
> +static void intel_set_dp_tp_ctl_normal(struct intel_atomic_state
> *state,
> +				       struct intel_crtc *crtc)
>  {
>  	struct drm_connector *uninitialized_var(conn);
>  	struct drm_connector_state *conn_state;
> @@ -15125,45 +15113,55 @@ static void
> intel_set_dp_tp_ctl_normal(struct intel_crtc *crtc,
>  	intel_dp_stop_link_train(intel_dp);
>  }
>  
> -static void intel_update_trans_port_sync_crtcs(struct intel_crtc
> *crtc,
> -					       struct
> intel_atomic_state *state,
> -					       struct intel_crtc_state
> *old_crtc_state,
> -					       struct intel_crtc_state
> *new_crtc_state)
> +static void intel_update_trans_port_sync_crtcs(struct
> intel_atomic_state *state,
> +					       struct intel_crtc *crtc)
>  {
> -	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
> -	struct intel_crtc *slave_crtc =
> intel_get_slave_crtc(new_crtc_state);
> -	struct intel_crtc_state *new_slave_crtc_state =
> -		intel_atomic_get_new_crtc_state(state, slave_crtc);
> -	struct intel_crtc_state *old_slave_crtc_state =
> -		intel_atomic_get_old_crtc_state(state, slave_crtc);
> +	struct drm_i915_private *i915 = to_i915(state->base.dev);
> +	const struct intel_crtc_state *new_slave_crtc_state;
> +	const struct intel_crtc_state *new_crtc_state;
> +	struct intel_crtc *slave_crtc;
> +	int i;
>  
> -	drm_WARN_ON(&i915->drm, !slave_crtc || !new_slave_crtc_state ||
> -		    !old_slave_crtc_state);
> +	for_each_new_intel_crtc_in_state(state, slave_crtc,
> +					 new_slave_crtc_state, i) {
> +		if (new_slave_crtc_state->master_transcoder !=
> +		    new_crtc_state->cpu_transcoder)

Missing new_crtc_state initialization.

With that:
Reviewed-by: José Roberto de Souza <jose.souza@intel.com>

> +			continue;
> +
> +		drm_dbg_kms(&i915->drm,
> +			    "Updating transcoder port sync slave
> [CRTC:%d:%s]\n",
> +			    slave_crtc->base.base.id, slave_crtc-
> >base.name);
> +
> +		intel_enable_crtc(state, slave_crtc);
> +	}
>  
>  	drm_dbg_kms(&i915->drm,
> -		    "Updating Transcoder Port Sync Master CRTC = %d %s
> and Slave CRTC %d %s\n",
> -		    crtc->base.base.id, crtc->base.name,
> -		    slave_crtc->base.base.id, slave_crtc->base.name);
> +		    "Updating transcoder port sync master
> [CRTC:%d:%s]\n",
> +		    crtc->base.base.id, crtc->base.name);
>  
> -	/* Enable seq for slave with with DP_TP_CTL left Idle until the
> -	 * master is ready
> -	 */
> -	intel_enable_crtc(state, slave_crtc);
> -
> -	/* Enable seq for master with with DP_TP_CTL left Idle */
>  	intel_enable_crtc(state, crtc);
>  
> -	/* Set Slave's DP_TP_CTL to Normal */
> -	intel_set_dp_tp_ctl_normal(slave_crtc,
> -				   state);
> +	for_each_new_intel_crtc_in_state(state, slave_crtc,
> +					 new_slave_crtc_state, i) {
> +		if (new_slave_crtc_state->master_transcoder !=
> +		    new_crtc_state->cpu_transcoder)
> +			continue;
> +
> +		intel_set_dp_tp_ctl_normal(state, slave_crtc);
> +	}
>  
> -	/* Set Master's DP_TP_CTL To Normal */
>  	usleep_range(200, 400);
> -	intel_set_dp_tp_ctl_normal(crtc,
> -				   state);
> +	intel_set_dp_tp_ctl_normal(state, crtc);
> +
> +	for_each_new_intel_crtc_in_state(state, slave_crtc,
> +					 new_slave_crtc_state, i) {
> +		if (new_slave_crtc_state->master_transcoder !=
> +		    new_crtc_state->cpu_transcoder)
> +			continue;
> +
> +		intel_update_crtc(state, slave_crtc);
> +	}
>  
> -	/* Now do the post crtc enable for all master and slaves */
> -	intel_update_crtc(state, slave_crtc);
>  	intel_update_crtc(state, crtc);
>  }
>  
> @@ -15275,16 +15273,18 @@ static void
> skl_commit_modeset_enables(struct intel_atomic_state *state)
>  		modeset_pipes &= ~BIT(pipe);
>  
>  		if (is_trans_port_sync_mode(new_crtc_state)) {
> +			const struct intel_crtc_state
> *new_slave_crtc_state;
>  			struct intel_crtc *slave_crtc;
> +			int i;
>  
> -			intel_update_trans_port_sync_crtcs(crtc, state,
> -							   old_crtc_sta
> te,
> -							   new_crtc_sta
> te);
> +			intel_update_trans_port_sync_crtcs(state,
> crtc);
>  
> -			slave_crtc =
> intel_get_slave_crtc(new_crtc_state);
> -			/* TODO: update entries[] of slave */
> -			modeset_pipes &= ~BIT(slave_crtc->pipe);
> +			for_each_new_intel_crtc_in_state(state,
> slave_crtc,
> +							 new_slave_crtc
> _state, i) {
>  
> +				/* TODO: update entries[] of slave */
> +				modeset_pipes &= ~BIT(slave_crtc-
> >pipe);
> +			}
>  		} else {
>  			intel_enable_crtc(state, crtc);
>  			intel_update_crtc(state, crtc);
Ville Syrjälä April 3, 2020, 5:25 p.m. UTC | #2
On Fri, Apr 03, 2020 at 12:32:20AM +0000, Souza, Jose wrote:
> On Fri, 2020-03-13 at 18:48 +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Don't assume there is just one port sync slave. We might have
> > several.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 98 ++++++++++------
> > ----
> >  1 file changed, 49 insertions(+), 49 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index b56a5a49418f..33f38c8a5da4 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -15009,18 +15009,6 @@ static void intel_update_crtc(struct
> > intel_atomic_state *state,
> >  		intel_crtc_arm_fifo_underrun(crtc, new_crtc_state);
> >  }
> >  
> > -static struct intel_crtc *intel_get_slave_crtc(const struct
> > intel_crtc_state *new_crtc_state)
> > -{
> > -	struct drm_i915_private *dev_priv = to_i915(new_crtc_state-
> > >uapi.crtc->dev);
> > -	enum transcoder slave_transcoder;
> > -
> > -	drm_WARN_ON(&dev_priv->drm,
> > -		    !is_power_of_2(new_crtc_state-
> > >sync_mode_slaves_mask));
> > -
> > -	slave_transcoder = ffs(new_crtc_state->sync_mode_slaves_mask) -
> > 1;
> > -	return intel_get_crtc_for_pipe(dev_priv,
> > -				       (enum pipe)slave_transcoder);
> > -}
> >  
> >  static void intel_old_crtc_state_disables(struct intel_atomic_state
> > *state,
> >  					  struct intel_crtc_state
> > *old_crtc_state,
> > @@ -15109,8 +15097,8 @@ static void
> > intel_commit_modeset_enables(struct intel_atomic_state *state)
> >  	}
> >  }
> >  
> > -static void intel_set_dp_tp_ctl_normal(struct intel_crtc *crtc,
> > -				       struct intel_atomic_state
> > *state)
> > +static void intel_set_dp_tp_ctl_normal(struct intel_atomic_state
> > *state,
> > +				       struct intel_crtc *crtc)
> >  {
> >  	struct drm_connector *uninitialized_var(conn);
> >  	struct drm_connector_state *conn_state;
> > @@ -15125,45 +15113,55 @@ static void
> > intel_set_dp_tp_ctl_normal(struct intel_crtc *crtc,
> >  	intel_dp_stop_link_train(intel_dp);
> >  }
> >  
> > -static void intel_update_trans_port_sync_crtcs(struct intel_crtc
> > *crtc,
> > -					       struct
> > intel_atomic_state *state,
> > -					       struct intel_crtc_state
> > *old_crtc_state,
> > -					       struct intel_crtc_state
> > *new_crtc_state)
> > +static void intel_update_trans_port_sync_crtcs(struct
> > intel_atomic_state *state,
> > +					       struct intel_crtc *crtc)
> >  {
> > -	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
> > -	struct intel_crtc *slave_crtc =
> > intel_get_slave_crtc(new_crtc_state);
> > -	struct intel_crtc_state *new_slave_crtc_state =
> > -		intel_atomic_get_new_crtc_state(state, slave_crtc);
> > -	struct intel_crtc_state *old_slave_crtc_state =
> > -		intel_atomic_get_old_crtc_state(state, slave_crtc);
> > +	struct drm_i915_private *i915 = to_i915(state->base.dev);
> > +	const struct intel_crtc_state *new_slave_crtc_state;
> > +	const struct intel_crtc_state *new_crtc_state;
> > +	struct intel_crtc *slave_crtc;
> > +	int i;
> >  
> > -	drm_WARN_ON(&i915->drm, !slave_crtc || !new_slave_crtc_state ||
> > -		    !old_slave_crtc_state);
> > +	for_each_new_intel_crtc_in_state(state, slave_crtc,
> > +					 new_slave_crtc_state, i) {
> > +		if (new_slave_crtc_state->master_transcoder !=
> > +		    new_crtc_state->cpu_transcoder)
> 
> Missing new_crtc_state initialization.

Whoops. Fixed.

> 
> With that:
> Reviewed-by: José Roberto de Souza <jose.souza@intel.com>
>
<snip>
> > -			/* TODO: update entries[] of slave */
> > -			modeset_pipes &= ~BIT(slave_crtc->pipe);
> > +			for_each_new_intel_crtc_in_state(state,
> > slave_crtc,
> > +							 new_slave_crtc
> > _state, i) {
> >  
> > +				/* TODO: update entries[] of slave */
> > +				modeset_pipes &= ~BIT(slave_crtc-
> > >pipe);

Noticed another problem here. Instead of clearing modeset_pipes for
the slaves of the current crtc we clear it for all crtcs. Fixed to do
the same master_transcoder check as above.

> > +			}
> >  		} else {
> >  			intel_enable_crtc(state, crtc);
> >  			intel_update_crtc(state, crtc);

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index b56a5a49418f..33f38c8a5da4 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -15009,18 +15009,6 @@  static void intel_update_crtc(struct intel_atomic_state *state,
 		intel_crtc_arm_fifo_underrun(crtc, new_crtc_state);
 }
 
-static struct intel_crtc *intel_get_slave_crtc(const struct intel_crtc_state *new_crtc_state)
-{
-	struct drm_i915_private *dev_priv = to_i915(new_crtc_state->uapi.crtc->dev);
-	enum transcoder slave_transcoder;
-
-	drm_WARN_ON(&dev_priv->drm,
-		    !is_power_of_2(new_crtc_state->sync_mode_slaves_mask));
-
-	slave_transcoder = ffs(new_crtc_state->sync_mode_slaves_mask) - 1;
-	return intel_get_crtc_for_pipe(dev_priv,
-				       (enum pipe)slave_transcoder);
-}
 
 static void intel_old_crtc_state_disables(struct intel_atomic_state *state,
 					  struct intel_crtc_state *old_crtc_state,
@@ -15109,8 +15097,8 @@  static void intel_commit_modeset_enables(struct intel_atomic_state *state)
 	}
 }
 
-static void intel_set_dp_tp_ctl_normal(struct intel_crtc *crtc,
-				       struct intel_atomic_state *state)
+static void intel_set_dp_tp_ctl_normal(struct intel_atomic_state *state,
+				       struct intel_crtc *crtc)
 {
 	struct drm_connector *uninitialized_var(conn);
 	struct drm_connector_state *conn_state;
@@ -15125,45 +15113,55 @@  static void intel_set_dp_tp_ctl_normal(struct intel_crtc *crtc,
 	intel_dp_stop_link_train(intel_dp);
 }
 
-static void intel_update_trans_port_sync_crtcs(struct intel_crtc *crtc,
-					       struct intel_atomic_state *state,
-					       struct intel_crtc_state *old_crtc_state,
-					       struct intel_crtc_state *new_crtc_state)
+static void intel_update_trans_port_sync_crtcs(struct intel_atomic_state *state,
+					       struct intel_crtc *crtc)
 {
-	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
-	struct intel_crtc *slave_crtc = intel_get_slave_crtc(new_crtc_state);
-	struct intel_crtc_state *new_slave_crtc_state =
-		intel_atomic_get_new_crtc_state(state, slave_crtc);
-	struct intel_crtc_state *old_slave_crtc_state =
-		intel_atomic_get_old_crtc_state(state, slave_crtc);
+	struct drm_i915_private *i915 = to_i915(state->base.dev);
+	const struct intel_crtc_state *new_slave_crtc_state;
+	const struct intel_crtc_state *new_crtc_state;
+	struct intel_crtc *slave_crtc;
+	int i;
 
-	drm_WARN_ON(&i915->drm, !slave_crtc || !new_slave_crtc_state ||
-		    !old_slave_crtc_state);
+	for_each_new_intel_crtc_in_state(state, slave_crtc,
+					 new_slave_crtc_state, i) {
+		if (new_slave_crtc_state->master_transcoder !=
+		    new_crtc_state->cpu_transcoder)
+			continue;
+
+		drm_dbg_kms(&i915->drm,
+			    "Updating transcoder port sync slave [CRTC:%d:%s]\n",
+			    slave_crtc->base.base.id, slave_crtc->base.name);
+
+		intel_enable_crtc(state, slave_crtc);
+	}
 
 	drm_dbg_kms(&i915->drm,
-		    "Updating Transcoder Port Sync Master CRTC = %d %s and Slave CRTC %d %s\n",
-		    crtc->base.base.id, crtc->base.name,
-		    slave_crtc->base.base.id, slave_crtc->base.name);
+		    "Updating transcoder port sync master [CRTC:%d:%s]\n",
+		    crtc->base.base.id, crtc->base.name);
 
-	/* Enable seq for slave with with DP_TP_CTL left Idle until the
-	 * master is ready
-	 */
-	intel_enable_crtc(state, slave_crtc);
-
-	/* Enable seq for master with with DP_TP_CTL left Idle */
 	intel_enable_crtc(state, crtc);
 
-	/* Set Slave's DP_TP_CTL to Normal */
-	intel_set_dp_tp_ctl_normal(slave_crtc,
-				   state);
+	for_each_new_intel_crtc_in_state(state, slave_crtc,
+					 new_slave_crtc_state, i) {
+		if (new_slave_crtc_state->master_transcoder !=
+		    new_crtc_state->cpu_transcoder)
+			continue;
+
+		intel_set_dp_tp_ctl_normal(state, slave_crtc);
+	}
 
-	/* Set Master's DP_TP_CTL To Normal */
 	usleep_range(200, 400);
-	intel_set_dp_tp_ctl_normal(crtc,
-				   state);
+	intel_set_dp_tp_ctl_normal(state, crtc);
+
+	for_each_new_intel_crtc_in_state(state, slave_crtc,
+					 new_slave_crtc_state, i) {
+		if (new_slave_crtc_state->master_transcoder !=
+		    new_crtc_state->cpu_transcoder)
+			continue;
+
+		intel_update_crtc(state, slave_crtc);
+	}
 
-	/* Now do the post crtc enable for all master and slaves */
-	intel_update_crtc(state, slave_crtc);
 	intel_update_crtc(state, crtc);
 }
 
@@ -15275,16 +15273,18 @@  static void skl_commit_modeset_enables(struct intel_atomic_state *state)
 		modeset_pipes &= ~BIT(pipe);
 
 		if (is_trans_port_sync_mode(new_crtc_state)) {
+			const struct intel_crtc_state *new_slave_crtc_state;
 			struct intel_crtc *slave_crtc;
+			int i;
 
-			intel_update_trans_port_sync_crtcs(crtc, state,
-							   old_crtc_state,
-							   new_crtc_state);
+			intel_update_trans_port_sync_crtcs(state, crtc);
 
-			slave_crtc = intel_get_slave_crtc(new_crtc_state);
-			/* TODO: update entries[] of slave */
-			modeset_pipes &= ~BIT(slave_crtc->pipe);
+			for_each_new_intel_crtc_in_state(state, slave_crtc,
+							 new_slave_crtc_state, i) {
 
+				/* TODO: update entries[] of slave */
+				modeset_pipes &= ~BIT(slave_crtc->pipe);
+			}
 		} else {
 			intel_enable_crtc(state, crtc);
 			intel_update_crtc(state, crtc);