diff mbox series

[v5,6/7] drm/i915/display: Check if pipe fastset is allowed by external dependencies

Message ID 20191220152954.83276-6-jose.souza@intel.com (mailing list archive)
State New, archived
Headers show
Series [v5,1/7] drm/i915: Introduce intel_crtc_state_alloc() | expand

Commit Message

Souza, Jose Dec. 20, 2019, 3:29 p.m. UTC 
Check if fastset is allowed by external dependencies like other pipes
and transcoders.

Right now this patch only forces a fullmodeset in MST slaves of MST
masters that needs a fullmodeset but it will be needed for port sync
as well.

v3:
- moved handling to intel_atomic_check() this way is guarantee that
all pipes will have its state computed

v4:
- added a function to return if MST master neeeds modeset to simply
code in intel_atomic_check()

v5:
- fixed and moved code to check if MST master needs a modeset

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Manasi Navare <manasi.d.navare@intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 70 ++++++++++++++++----
 1 file changed, 57 insertions(+), 13 deletions(-)

Comments

Ville Syrjälä Dec. 20, 2019, 4:04 p.m. UTC | #1 
On Fri, Dec 20, 2019 at 07:29:53AM -0800, José Roberto de Souza wrote:
> Check if fastset is allowed by external dependencies like other pipes
> and transcoders.
> 
> Right now this patch only forces a fullmodeset in MST slaves of MST
> masters that needs a fullmodeset but it will be needed for port sync
> as well.
> 
> v3:
> - moved handling to intel_atomic_check() this way is guarantee that
> all pipes will have its state computed
> 
> v4:
> - added a function to return if MST master neeeds modeset to simply
> code in intel_atomic_check()
> 
> v5:
> - fixed and moved code to check if MST master needs a modeset
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Manasi Navare <manasi.d.navare@intel.com>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 70 ++++++++++++++++----
>  1 file changed, 57 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 24841dde490b..11f2c13ec23e 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -13909,19 +13909,6 @@ static void intel_crtc_check_fastset(const struct intel_crtc_state *old_crtc_sta
>  
>  	new_crtc_state->uapi.mode_changed = false;
>  	new_crtc_state->update_pipe = true;
> -
> -	/*
> -	 * If we're not doing the full modeset we want to
> -	 * keep the current M/N values as they may be
> -	 * sufficiently different to the computed values
> -	 * to cause problems.
> -	 *
> -	 * FIXME: should really copy more fuzzy state here
> -	 */
> -	new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n;
> -	new_crtc_state->dp_m_n = old_crtc_state->dp_m_n;
> -	new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2;
> -	new_crtc_state->has_drrs = old_crtc_state->has_drrs;
>  }
>  
>  static int intel_crtc_add_planes_to_state(struct intel_atomic_state *state,
> @@ -14032,6 +14019,20 @@ static int intel_atomic_check_crtcs(struct intel_atomic_state *state)
>  	return 0;
>  }
>  
> +static bool intel_cpu_transcoder_needs_modeset(struct intel_atomic_state *state,
> +					       enum transcoder transcoder)
> +{
> +	struct intel_crtc_state *new_crtc_state;
> +	struct intel_crtc *crtc;
> +	int i;
> +
> +	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i)
> +		if (new_crtc_state->cpu_transcoder == transcoder)
> +			return needs_modeset(new_crtc_state);
> +
> +	return false;
> +}

I would highly recommend splitting this patch into two before pushing
so that the check vs. copy split goes in first, then the MST thing on
top. That way if we have to revert either MST or port sync then at
least we keep this part and won't break the other feature in the
process.

> +
>  /**
>   * intel_atomic_check - validate state object
>   * @dev: drm device
> @@ -14089,6 +14090,49 @@ static int intel_atomic_check(struct drm_device *dev,
>  			any_ms = true;
>  	}

Was going to say this any_ms thing has to move, but I guess not really
since the below loop won't do anything unless we still have at least
one crtc needing a modeset. Probably should move it, but can be done
later.

>  
> +	/**
> +	 * Check if fastset is allowed by external dependencies like other
> +	 * pipes and transcoders.
> +	 *
> +	 * Right now it only forces a fullmodeset when the MST master
> +	 * transcoder did not changed but the pipe of the master transcoder
> +	 * needs a fullmodeset so all slaves also needs to do a fullmodeset.
> +	 */
> +	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
> +		enum transcoder master = new_crtc_state->mst_master_transcoder;
> +
> +		if (!new_crtc_state->uapi.enable ||

These uapi.enable checks seem fishy. Should at least be hw.enable since
we're past .compute_config() stage already.

> +		    !intel_dp_mst_is_slave_trans(new_crtc_state) ||
> +		    needs_modeset(new_crtc_state))
> +			continue;
> +
> +		if (intel_cpu_transcoder_needs_modeset(state, master)) {
> +			new_crtc_state->uapi.mode_changed = true;
> +			new_crtc_state->update_pipe = false;
> +		}
> +	}
> +
> +	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> +					    new_crtc_state, i) {
> +		if (!new_crtc_state->uapi.enable ||
> +		    needs_modeset(new_crtc_state) ||
> +		    !new_crtc_state->update_pipe)
> +			continue;
> +
> +		/*
> +		 * If we're not doing the full modeset we want to
> +		 * keep the current M/N values as they may be
> +		 * sufficiently different to the computed values
> +		 * to cause problems.
> +		 *
> +		 * FIXME: should really copy more fuzzy state here
> +		 */
> +		new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n;
> +		new_crtc_state->dp_m_n = old_crtc_state->dp_m_n;
> +		new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2;
> +		new_crtc_state->has_drrs = old_crtc_state->has_drrs;

Still sad.

> +	}
> +
>  	if (any_ms && !check_digital_port_conflicts(state)) {
>  		DRM_DEBUG_KMS("rejecting conflicting digital port configuration\n");
>  		ret = EINVAL;
> -- 
> 2.24.1
Souza, Jose Dec. 20, 2019, 6:27 p.m. UTC | #2 
On Fri, 2019-12-20 at 18:04 +0200, Ville Syrjälä wrote:
> On Fri, Dec 20, 2019 at 07:29:53AM -0800, José Roberto de Souza
> wrote:
> > Check if fastset is allowed by external dependencies like other
> > pipes
> > and transcoders.
> > 
> > Right now this patch only forces a fullmodeset in MST slaves of MST
> > masters that needs a fullmodeset but it will be needed for port
> > sync
> > as well.
> > 
> > v3:
> > - moved handling to intel_atomic_check() this way is guarantee that
> > all pipes will have its state computed
> > 
> > v4:
> > - added a function to return if MST master neeeds modeset to simply
> > code in intel_atomic_check()
> > 
> > v5:
> > - fixed and moved code to check if MST master needs a modeset
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Manasi Navare <manasi.d.navare@intel.com>
> > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 70
> > ++++++++++++++++----
> >  1 file changed, 57 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index 24841dde490b..11f2c13ec23e 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -13909,19 +13909,6 @@ static void intel_crtc_check_fastset(const
> > struct intel_crtc_state *old_crtc_sta
> >  
> >  	new_crtc_state->uapi.mode_changed = false;
> >  	new_crtc_state->update_pipe = true;
> > -
> > -	/*
> > -	 * If we're not doing the full modeset we want to
> > -	 * keep the current M/N values as they may be
> > -	 * sufficiently different to the computed values
> > -	 * to cause problems.
> > -	 *
> > -	 * FIXME: should really copy more fuzzy state here
> > -	 */
> > -	new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n;
> > -	new_crtc_state->dp_m_n = old_crtc_state->dp_m_n;
> > -	new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2;
> > -	new_crtc_state->has_drrs = old_crtc_state->has_drrs;
> >  }
> >  
> >  static int intel_crtc_add_planes_to_state(struct
> > intel_atomic_state *state,
> > @@ -14032,6 +14019,20 @@ static int intel_atomic_check_crtcs(struct
> > intel_atomic_state *state)
> >  	return 0;
> >  }
> >  
> > +static bool intel_cpu_transcoder_needs_modeset(struct
> > intel_atomic_state *state,
> > +					       enum transcoder
> > transcoder)
> > +{
> > +	struct intel_crtc_state *new_crtc_state;
> > +	struct intel_crtc *crtc;
> > +	int i;
> > +
> > +	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state,
> > i)
> > +		if (new_crtc_state->cpu_transcoder == transcoder)
> > +			return needs_modeset(new_crtc_state);
> > +
> > +	return false;
> > +}
> 
> I would highly recommend splitting this patch into two before pushing
> so that the check vs. copy split goes in first, then the MST thing on
> top. That way if we have to revert either MST or port sync then at
> least we keep this part and won't break the other feature in the
> process.

okay, doing that

> 
> > +
> >  /**
> >   * intel_atomic_check - validate state object
> >   * @dev: drm device
> > @@ -14089,6 +14090,49 @@ static int intel_atomic_check(struct
> > drm_device *dev,
> >  			any_ms = true;
> >  	}
> 
> Was going to say this any_ms thing has to move, but I guess not
> really
> since the below loop won't do anything unless we still have at least
> one crtc needing a modeset. Probably should move it, but can be done
> later.

Okay

> 
> >  
> > +	/**
> > +	 * Check if fastset is allowed by external dependencies like
> > other
> > +	 * pipes and transcoders.
> > +	 *
> > +	 * Right now it only forces a fullmodeset when the MST master
> > +	 * transcoder did not changed but the pipe of the master
> > transcoder
> > +	 * needs a fullmodeset so all slaves also needs to do a
> > fullmodeset.
> > +	 */
> > +	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state,
> > i) {
> > +		enum transcoder master = new_crtc_state-
> > >mst_master_transcoder;
> > +
> > +		if (!new_crtc_state->uapi.enable ||
> 
> These uapi.enable checks seem fishy. Should at least be hw.enable
> since
> we're past .compute_config() stage already.

Okay

> 
> > +		    !intel_dp_mst_is_slave_trans(new_crtc_state) ||
> > +		    needs_modeset(new_crtc_state))
> > +			continue;
> > +
> > +		if (intel_cpu_transcoder_needs_modeset(state, master))
> > {
> > +			new_crtc_state->uapi.mode_changed = true;
> > +			new_crtc_state->update_pipe = false;
> > +		}
> > +	}
> > +
> > +	for_each_oldnew_intel_crtc_in_state(state, crtc,
> > old_crtc_state,
> > +					    new_crtc_state, i) {
> > +		if (!new_crtc_state->uapi.enable ||
> > +		    needs_modeset(new_crtc_state) ||
> > +		    !new_crtc_state->update_pipe)
> > +			continue;
> > +
> > +		/*
> > +		 * If we're not doing the full modeset we want to
> > +		 * keep the current M/N values as they may be
> > +		 * sufficiently different to the computed values
> > +		 * to cause problems.
> > +		 *
> > +		 * FIXME: should really copy more fuzzy state here
> > +		 */
> > +		new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n;
> > +		new_crtc_state->dp_m_n = old_crtc_state->dp_m_n;
> > +		new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2;
> > +		new_crtc_state->has_drrs = old_crtc_state->has_drrs;
> 
> Still sad.


Any suggestion how to fix this? I can work on this after this MST fire.

> 
> > +	}
> > +
> >  	if (any_ms && !check_digital_port_conflicts(state)) {
> >  		DRM_DEBUG_KMS("rejecting conflicting digital port
> > configuration\n");
> >  		ret = EINVAL;
> > -- 
> > 2.24.1
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 24841dde490b..11f2c13ec23e 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -13909,19 +13909,6 @@  static void intel_crtc_check_fastset(const struct intel_crtc_state *old_crtc_sta
 
 	new_crtc_state->uapi.mode_changed = false;
 	new_crtc_state->update_pipe = true;
-
-	/*
-	 * If we're not doing the full modeset we want to
-	 * keep the current M/N values as they may be
-	 * sufficiently different to the computed values
-	 * to cause problems.
-	 *
-	 * FIXME: should really copy more fuzzy state here
-	 */
-	new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n;
-	new_crtc_state->dp_m_n = old_crtc_state->dp_m_n;
-	new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2;
-	new_crtc_state->has_drrs = old_crtc_state->has_drrs;
 }
 
 static int intel_crtc_add_planes_to_state(struct intel_atomic_state *state,
@@ -14032,6 +14019,20 @@  static int intel_atomic_check_crtcs(struct intel_atomic_state *state)
 	return 0;
 }
 
+static bool intel_cpu_transcoder_needs_modeset(struct intel_atomic_state *state,
+					       enum transcoder transcoder)
+{
+	struct intel_crtc_state *new_crtc_state;
+	struct intel_crtc *crtc;
+	int i;
+
+	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i)
+		if (new_crtc_state->cpu_transcoder == transcoder)
+			return needs_modeset(new_crtc_state);
+
+	return false;
+}
+
 /**
  * intel_atomic_check - validate state object
  * @dev: drm device
@@ -14089,6 +14090,49 @@  static int intel_atomic_check(struct drm_device *dev,
 			any_ms = true;
 	}
 
+	/**
+	 * Check if fastset is allowed by external dependencies like other
+	 * pipes and transcoders.
+	 *
+	 * Right now it only forces a fullmodeset when the MST master
+	 * transcoder did not changed but the pipe of the master transcoder
+	 * needs a fullmodeset so all slaves also needs to do a fullmodeset.
+	 */
+	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
+		enum transcoder master = new_crtc_state->mst_master_transcoder;
+
+		if (!new_crtc_state->uapi.enable ||
+		    !intel_dp_mst_is_slave_trans(new_crtc_state) ||
+		    needs_modeset(new_crtc_state))
+			continue;
+
+		if (intel_cpu_transcoder_needs_modeset(state, master)) {
+			new_crtc_state->uapi.mode_changed = true;
+			new_crtc_state->update_pipe = false;
+		}
+	}
+
+	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
+					    new_crtc_state, i) {
+		if (!new_crtc_state->uapi.enable ||
+		    needs_modeset(new_crtc_state) ||
+		    !new_crtc_state->update_pipe)
+			continue;
+
+		/*
+		 * If we're not doing the full modeset we want to
+		 * keep the current M/N values as they may be
+		 * sufficiently different to the computed values
+		 * to cause problems.
+		 *
+		 * FIXME: should really copy more fuzzy state here
+		 */
+		new_crtc_state->fdi_m_n = old_crtc_state->fdi_m_n;
+		new_crtc_state->dp_m_n = old_crtc_state->dp_m_n;
+		new_crtc_state->dp_m2_n2 = old_crtc_state->dp_m2_n2;
+		new_crtc_state->has_drrs = old_crtc_state->has_drrs;
+	}
+
 	if (any_ms && !check_digital_port_conflicts(state)) {
 		DRM_DEBUG_KMS("rejecting conflicting digital port configuration\n");
 		ret = EINVAL;