diff mbox series

[01/13] drm/i915/mst: Use .compute_config_late() to compute master transcoder

Message ID 20200313164831.5980-2-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915: Port sync for skl+ | expand

Commit Message

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

Use the recently introduced encoder .compute_config_late() hook to
do the MST master transcoder assignment. Avoids having to do it
in a funny way before we know the CPU transcoder of each pipe.

And now we can also properly use hw.active instead of uapi.active
since it too has been calculated earlier for everyone.

Cc: José Roberto de Souza <jose.souza@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_dp_mst.c | 98 +++++++++++----------
 1 file changed, 51 insertions(+), 47 deletions(-)

Comments

Souza, Jose March 20, 2020, 11:37 p.m. UTC | #1 
This will not work for MST, here a example

Previous state:
MST master pipe A
MST slave pipe B

New state:
Pipe A being disabled

On drm_atomic_helper_check_modeset() both intel_crtc_states will be
added to the state with crtc_state->uapi.mode_changed=true.
Then on the regular for_each_oldnew_intel_crtc_in_state() loop config
of CRTC B will have mst_master_transcoder=INVALID_TRANSCODER that
differs from TRANSCODER_A and will keep mode_changed set.
Then on the config_late loop it will skip the interation on the
needs_modeset() check keeping CRTC B with
mst_master_transcoder=INVALID_TRANSCODER.

That would be caugh by CI if this tests were merged and the TGL machine
with MST is still on: https://patchwork.freedesktop.org/series/72211/

On Fri, 2020-03-13 at 18:48 +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Use the recently introduced encoder .compute_config_late() hook to
> do the MST master transcoder assignment. Avoids having to do it
> in a funny way before we know the CPU transcoder of each pipe.
> 
> And now we can also properly use hw.active instead of uapi.active
> since it too has been calculated earlier for everyone.
> 
> Cc: José Roberto de Souza <jose.souza@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dp_mst.c | 98 +++++++++++------
> ----
>  1 file changed, 51 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index 44f3fd251ca1..b9afc1135b9b 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -88,56 +88,10 @@ static int
> intel_dp_mst_compute_link_config(struct intel_encoder *encoder,
>  	return 0;
>  }
>  
> -/*
> - * Iterate over all connectors and return the smallest transcoder in
> the MST
> - * stream
> - */
> -static enum transcoder
> -intel_dp_mst_master_trans_compute(struct intel_atomic_state *state,
> -				  struct intel_dp *mst_port)
> -{
> -	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> -	struct intel_digital_connector_state *conn_state;
> -	struct intel_connector *connector;
> -	enum pipe ret = I915_MAX_PIPES;
> -	int i;
> -
> -	if (INTEL_GEN(dev_priv) < 12)
> -		return INVALID_TRANSCODER;
> -
> -	for_each_new_intel_connector_in_state(state, connector,
> conn_state, i) {
> -		struct intel_crtc_state *crtc_state;
> -		struct intel_crtc *crtc;
> -
> -		if (connector->mst_port != mst_port || !conn_state-
> >base.crtc)
> -			continue;
> -
> -		crtc = to_intel_crtc(conn_state->base.crtc);
> -		crtc_state = intel_atomic_get_new_crtc_state(state,
> crtc);
> -		if (!crtc_state->uapi.active)
> -			continue;
> -
> -		/*
> -		 * Using crtc->pipe because crtc_state->cpu_transcoder
> is
> -		 * computed, so others CRTCs could have non-computed
> -		 * cpu_transcoder
> -		 */
> -		if (crtc->pipe < ret)
> -			ret = crtc->pipe;
> -	}
> -
> -	if (ret == I915_MAX_PIPES)
> -		return INVALID_TRANSCODER;
> -
> -	/* Simple cast works because TGL don't have a eDP transcoder */
> -	return (enum transcoder)ret;
> -}
> -
>  static int intel_dp_mst_compute_config(struct intel_encoder
> *encoder,
>  				       struct intel_crtc_state
> *pipe_config,
>  				       struct drm_connector_state
> *conn_state)
>  {
> -	struct intel_atomic_state *state =
> to_intel_atomic_state(conn_state->state);
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>  	struct intel_dp_mst_encoder *intel_mst = enc_to_mst(encoder);
>  	struct intel_dp *intel_dp = &intel_mst->primary->dp;
> @@ -201,7 +155,56 @@ static int intel_dp_mst_compute_config(struct
> intel_encoder *encoder,
>  
>  	intel_ddi_compute_min_voltage_level(dev_priv, pipe_config);
>  
> -	pipe_config->mst_master_transcoder =
> intel_dp_mst_master_trans_compute(state, intel_dp);
> +	return 0;
> +}
> +
> +/*
> + * Iterate over all connectors and return a mask of
> + * all CPU transcoders streaming over the same DP link.
> + */
> +static unsigned int
> +intel_dp_mst_transcoder_mask(struct intel_atomic_state *state,
> +			     struct intel_dp *mst_port)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> +	const struct intel_digital_connector_state *conn_state;
> +	struct intel_connector *connector;
> +	u8 transcoders = 0;
> +	int i;
> +
> +	if (INTEL_GEN(dev_priv) < 12)
> +		return 0;
> +
> +	for_each_new_intel_connector_in_state(state, connector,
> conn_state, i) {
> +		const struct intel_crtc_state *crtc_state;
> +		struct intel_crtc *crtc;
> +
> +		if (connector->mst_port != mst_port || !conn_state-
> >base.crtc)
> +			continue;
> +
> +		crtc = to_intel_crtc(conn_state->base.crtc);
> +		crtc_state = intel_atomic_get_new_crtc_state(state,
> crtc);
> +
> +		if (!crtc_state->hw.active)
> +			continue;
> +
> +		transcoders |= BIT(crtc_state->cpu_transcoder);
> +	}
> +
> +	return transcoders;
> +}
> +
> +static int intel_dp_mst_compute_config_late(struct intel_encoder
> *encoder,
> +					    struct intel_crtc_state
> *crtc_state,
> +					    struct drm_connector_state
> *conn_state)
> +{
> +	struct intel_atomic_state *state =
> to_intel_atomic_state(conn_state->state);
> +	struct intel_dp_mst_encoder *intel_mst = enc_to_mst(encoder);
> +	struct intel_dp *intel_dp = &intel_mst->primary->dp;
> +
> +	/* lowest numbered transcoder will be designated master */
> +	crtc_state->mst_master_transcoder =
> +		ffs(intel_dp_mst_transcoder_mask(state, intel_dp)) - 1;
>  
>  	return 0;
>  }
> @@ -786,6 +789,7 @@ intel_dp_create_fake_mst_encoder(struct
> intel_digital_port *intel_dig_port, enum
>  	intel_encoder->pipe_mask = ~0;
>  
>  	intel_encoder->compute_config = intel_dp_mst_compute_config;
> +	intel_encoder->compute_config_late =
> intel_dp_mst_compute_config_late;
>  	intel_encoder->disable = intel_mst_disable_dp;
>  	intel_encoder->post_disable = intel_mst_post_disable_dp;
>  	intel_encoder->pre_pll_enable = intel_mst_pre_pll_enable_dp;
Souza, Jose March 20, 2020, 11:54 p.m. UTC | #2 
Never mind, read the code again it will work.

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

On Fri, 2020-03-20 at 23:37 +0000, Souza, Jose wrote:
> This will not work for MST, here a example
> 
> Previous state:
> MST master pipe A
> MST slave pipe B
> 
> New state:
> Pipe A being disabled
> 
> On drm_atomic_helper_check_modeset() both intel_crtc_states will be
> added to the state with crtc_state->uapi.mode_changed=true.
> Then on the regular for_each_oldnew_intel_crtc_in_state() loop config
> of CRTC B will have mst_master_transcoder=INVALID_TRANSCODER that
> differs from TRANSCODER_A and will keep mode_changed set.
> Then on the config_late loop it will skip the interation on the
> needs_modeset() check keeping CRTC B with
> mst_master_transcoder=INVALID_TRANSCODER.
> 
> That would be caugh by CI if this tests were merged and the TGL
> machine
> with MST is still on: https://patchwork.freedesktop.org/series/72211/
> 
> On Fri, 2020-03-13 at 18:48 +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Use the recently introduced encoder .compute_config_late() hook to
> > do the MST master transcoder assignment. Avoids having to do it
> > in a funny way before we know the CPU transcoder of each pipe.
> > 
> > And now we can also properly use hw.active instead of uapi.active
> > since it too has been calculated earlier for everyone.
> > 
> > Cc: José Roberto de Souza <jose.souza@intel.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_dp_mst.c | 98 +++++++++++------
> > ----
> >  1 file changed, 51 insertions(+), 47 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > index 44f3fd251ca1..b9afc1135b9b 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > @@ -88,56 +88,10 @@ static int
> > intel_dp_mst_compute_link_config(struct intel_encoder *encoder,
> >  	return 0;
> >  }
> >  
> > -/*
> > - * Iterate over all connectors and return the smallest transcoder
> > in
> > the MST
> > - * stream
> > - */
> > -static enum transcoder
> > -intel_dp_mst_master_trans_compute(struct intel_atomic_state
> > *state,
> > -				  struct intel_dp *mst_port)
> > -{
> > -	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > -	struct intel_digital_connector_state *conn_state;
> > -	struct intel_connector *connector;
> > -	enum pipe ret = I915_MAX_PIPES;
> > -	int i;
> > -
> > -	if (INTEL_GEN(dev_priv) < 12)
> > -		return INVALID_TRANSCODER;
> > -
> > -	for_each_new_intel_connector_in_state(state, connector,
> > conn_state, i) {
> > -		struct intel_crtc_state *crtc_state;
> > -		struct intel_crtc *crtc;
> > -
> > -		if (connector->mst_port != mst_port || !conn_state-
> > > base.crtc)
> > -			continue;
> > -
> > -		crtc = to_intel_crtc(conn_state->base.crtc);
> > -		crtc_state = intel_atomic_get_new_crtc_state(state,
> > crtc);
> > -		if (!crtc_state->uapi.active)
> > -			continue;
> > -
> > -		/*
> > -		 * Using crtc->pipe because crtc_state->cpu_transcoder
> > is
> > -		 * computed, so others CRTCs could have non-computed
> > -		 * cpu_transcoder
> > -		 */
> > -		if (crtc->pipe < ret)
> > -			ret = crtc->pipe;
> > -	}
> > -
> > -	if (ret == I915_MAX_PIPES)
> > -		return INVALID_TRANSCODER;
> > -
> > -	/* Simple cast works because TGL don't have a eDP transcoder */
> > -	return (enum transcoder)ret;
> > -}
> > -
> >  static int intel_dp_mst_compute_config(struct intel_encoder
> > *encoder,
> >  				       struct intel_crtc_state
> > *pipe_config,
> >  				       struct drm_connector_state
> > *conn_state)
> >  {
> > -	struct intel_atomic_state *state =
> > to_intel_atomic_state(conn_state->state);
> >  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> >  	struct intel_dp_mst_encoder *intel_mst = enc_to_mst(encoder);
> >  	struct intel_dp *intel_dp = &intel_mst->primary->dp;
> > @@ -201,7 +155,56 @@ static int intel_dp_mst_compute_config(struct
> > intel_encoder *encoder,
> >  
> >  	intel_ddi_compute_min_voltage_level(dev_priv, pipe_config);
> >  
> > -	pipe_config->mst_master_transcoder =
> > intel_dp_mst_master_trans_compute(state, intel_dp);
> > +	return 0;
> > +}
> > +
> > +/*
> > + * Iterate over all connectors and return a mask of
> > + * all CPU transcoders streaming over the same DP link.
> > + */
> > +static unsigned int
> > +intel_dp_mst_transcoder_mask(struct intel_atomic_state *state,
> > +			     struct intel_dp *mst_port)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > +	const struct intel_digital_connector_state *conn_state;
> > +	struct intel_connector *connector;
> > +	u8 transcoders = 0;
> > +	int i;
> > +
> > +	if (INTEL_GEN(dev_priv) < 12)
> > +		return 0;
> > +
> > +	for_each_new_intel_connector_in_state(state, connector,
> > conn_state, i) {
> > +		const struct intel_crtc_state *crtc_state;
> > +		struct intel_crtc *crtc;
> > +
> > +		if (connector->mst_port != mst_port || !conn_state-
> > > base.crtc)
> > +			continue;
> > +
> > +		crtc = to_intel_crtc(conn_state->base.crtc);
> > +		crtc_state = intel_atomic_get_new_crtc_state(state,
> > crtc);
> > +
> > +		if (!crtc_state->hw.active)
> > +			continue;
> > +
> > +		transcoders |= BIT(crtc_state->cpu_transcoder);
> > +	}
> > +
> > +	return transcoders;
> > +}
> > +
> > +static int intel_dp_mst_compute_config_late(struct intel_encoder
> > *encoder,
> > +					    struct intel_crtc_state
> > *crtc_state,
> > +					    struct drm_connector_state
> > *conn_state)
> > +{
> > +	struct intel_atomic_state *state =
> > to_intel_atomic_state(conn_state->state);
> > +	struct intel_dp_mst_encoder *intel_mst = enc_to_mst(encoder);
> > +	struct intel_dp *intel_dp = &intel_mst->primary->dp;
> > +
> > +	/* lowest numbered transcoder will be designated master */
> > +	crtc_state->mst_master_transcoder =
> > +		ffs(intel_dp_mst_transcoder_mask(state, intel_dp)) - 1;
> >  
> >  	return 0;
> >  }
> > @@ -786,6 +789,7 @@ intel_dp_create_fake_mst_encoder(struct
> > intel_digital_port *intel_dig_port, enum
> >  	intel_encoder->pipe_mask = ~0;
> >  
> >  	intel_encoder->compute_config = intel_dp_mst_compute_config;
> > +	intel_encoder->compute_config_late =
> > intel_dp_mst_compute_config_late;
> >  	intel_encoder->disable = intel_mst_disable_dp;
> >  	intel_encoder->post_disable = intel_mst_post_disable_dp;
> >  	intel_encoder->pre_pll_enable = intel_mst_pre_pll_enable_dp;
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
index 44f3fd251ca1..b9afc1135b9b 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
@@ -88,56 +88,10 @@  static int intel_dp_mst_compute_link_config(struct intel_encoder *encoder,
 	return 0;
 }
 
-/*
- * Iterate over all connectors and return the smallest transcoder in the MST
- * stream
- */
-static enum transcoder
-intel_dp_mst_master_trans_compute(struct intel_atomic_state *state,
-				  struct intel_dp *mst_port)
-{
-	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
-	struct intel_digital_connector_state *conn_state;
-	struct intel_connector *connector;
-	enum pipe ret = I915_MAX_PIPES;
-	int i;
-
-	if (INTEL_GEN(dev_priv) < 12)
-		return INVALID_TRANSCODER;
-
-	for_each_new_intel_connector_in_state(state, connector, conn_state, i) {
-		struct intel_crtc_state *crtc_state;
-		struct intel_crtc *crtc;
-
-		if (connector->mst_port != mst_port || !conn_state->base.crtc)
-			continue;
-
-		crtc = to_intel_crtc(conn_state->base.crtc);
-		crtc_state = intel_atomic_get_new_crtc_state(state, crtc);
-		if (!crtc_state->uapi.active)
-			continue;
-
-		/*
-		 * Using crtc->pipe because crtc_state->cpu_transcoder is
-		 * computed, so others CRTCs could have non-computed
-		 * cpu_transcoder
-		 */
-		if (crtc->pipe < ret)
-			ret = crtc->pipe;
-	}
-
-	if (ret == I915_MAX_PIPES)
-		return INVALID_TRANSCODER;
-
-	/* Simple cast works because TGL don't have a eDP transcoder */
-	return (enum transcoder)ret;
-}
-
 static int intel_dp_mst_compute_config(struct intel_encoder *encoder,
 				       struct intel_crtc_state *pipe_config,
 				       struct drm_connector_state *conn_state)
 {
-	struct intel_atomic_state *state = to_intel_atomic_state(conn_state->state);
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
 	struct intel_dp_mst_encoder *intel_mst = enc_to_mst(encoder);
 	struct intel_dp *intel_dp = &intel_mst->primary->dp;
@@ -201,7 +155,56 @@  static int intel_dp_mst_compute_config(struct intel_encoder *encoder,
 
 	intel_ddi_compute_min_voltage_level(dev_priv, pipe_config);
 
-	pipe_config->mst_master_transcoder = intel_dp_mst_master_trans_compute(state, intel_dp);
+	return 0;
+}
+
+/*
+ * Iterate over all connectors and return a mask of
+ * all CPU transcoders streaming over the same DP link.
+ */
+static unsigned int
+intel_dp_mst_transcoder_mask(struct intel_atomic_state *state,
+			     struct intel_dp *mst_port)
+{
+	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
+	const struct intel_digital_connector_state *conn_state;
+	struct intel_connector *connector;
+	u8 transcoders = 0;
+	int i;
+
+	if (INTEL_GEN(dev_priv) < 12)
+		return 0;
+
+	for_each_new_intel_connector_in_state(state, connector, conn_state, i) {
+		const struct intel_crtc_state *crtc_state;
+		struct intel_crtc *crtc;
+
+		if (connector->mst_port != mst_port || !conn_state->base.crtc)
+			continue;
+
+		crtc = to_intel_crtc(conn_state->base.crtc);
+		crtc_state = intel_atomic_get_new_crtc_state(state, crtc);
+
+		if (!crtc_state->hw.active)
+			continue;
+
+		transcoders |= BIT(crtc_state->cpu_transcoder);
+	}
+
+	return transcoders;
+}
+
+static int intel_dp_mst_compute_config_late(struct intel_encoder *encoder,
+					    struct intel_crtc_state *crtc_state,
+					    struct drm_connector_state *conn_state)
+{
+	struct intel_atomic_state *state = to_intel_atomic_state(conn_state->state);
+	struct intel_dp_mst_encoder *intel_mst = enc_to_mst(encoder);
+	struct intel_dp *intel_dp = &intel_mst->primary->dp;
+
+	/* lowest numbered transcoder will be designated master */
+	crtc_state->mst_master_transcoder =
+		ffs(intel_dp_mst_transcoder_mask(state, intel_dp)) - 1;
 
 	return 0;
 }
@@ -786,6 +789,7 @@  intel_dp_create_fake_mst_encoder(struct intel_digital_port *intel_dig_port, enum
 	intel_encoder->pipe_mask = ~0;
 
 	intel_encoder->compute_config = intel_dp_mst_compute_config;
+	intel_encoder->compute_config_late = intel_dp_mst_compute_config_late;
 	intel_encoder->disable = intel_mst_disable_dp;
 	intel_encoder->post_disable = intel_mst_post_disable_dp;
 	intel_encoder->pre_pll_enable = intel_mst_pre_pll_enable_dp;