stable.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/4] drm/i915: Don't clobber M/N values during fastset check
@ 2019-06-12 13:07 Ville Syrjala
  2019-06-12 17:24 ` [PATCH v2 " Ville Syrjala
  0 siblings, 1 reply; 4+ messages in thread
From: Ville Syrjala @ 2019-06-12 13:07 UTC (permalink / raw)
  To: intel-gfx; +Cc: stable, Blubberbub, Maarten Lankhorst, Hans de Goede

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

We're now calling intel_pipe_config_compare(..., true) uncoditionally
which means we're always going clobber the calculated M/N values with
the old values if the fuzzy M/N check passes. That causes problems
because the fuzzy check allows for a huge difference in the values.

I'm actually tempted to just make the M/N checks exact, but that might
prevent fastboot from kicking in when people want it. So for now let's
overwrite the computed values with the old values only if decide to skip
the modeset.

Cc: stable@vger.kernel.org
Cc: Blubberbub@protonmail.com
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Hans de Goede <hdegoede@redhat.com>
Tested-by: Blubberbub@protonmail.com
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=110782
Fixes: d19f958db23c ("drm/i915: Enable fastset for non-boot modesets.")
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 35 +++++++++++++++++++++-------
 1 file changed, 27 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 1b1ddb48ca7a..73b3e92b7ed5 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12299,9 +12299,6 @@ intel_compare_link_m_n(const struct intel_link_m_n *m_n,
 			      m2_n2->gmch_m, m2_n2->gmch_n, !adjust) &&
 	    intel_compare_m_n(m_n->link_m, m_n->link_n,
 			      m2_n2->link_m, m2_n2->link_n, !adjust)) {
-		if (adjust)
-			*m2_n2 = *m_n;
-
 		return true;
 	}
 
@@ -13433,6 +13430,32 @@ static int calc_watermark_data(struct intel_atomic_state *state)
 	return 0;
 }
 
+static void intel_crtc_check_fastset(struct intel_crtc_state *old_crtc_state,
+				     struct intel_crtc_state *new_crtc_state)
+{
+	struct drm_i915_private *dev_priv =
+		to_i915(new_crtc_state->base.crtc->dev);
+
+	if (!intel_pipe_config_compare(dev_priv, old_crtc_state,
+				       new_crtc_state, true))
+		return;
+
+	new_crtc_state->base.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;
+}
+
 /**
  * intel_atomic_check - validate state object
  * @dev: drm device
@@ -13474,11 +13497,7 @@ static int intel_atomic_check(struct drm_device *dev,
 		if (ret)
 			goto fail;
 
-		if (intel_pipe_config_compare(dev_priv, old_crtc_state,
-					      new_crtc_state, true)) {
-			new_crtc_state->base.mode_changed = false;
-			new_crtc_state->update_pipe = true;
-		}
+		intel_crtc_check_fastset(old_crtc_state, new_crtc_state);
 
 		if (needs_modeset(&new_crtc_state->base))
 			any_ms = true;
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 4+ messages in thread
* [PATCH v2 1/4] drm/i915: Don't clobber M/N values during fastset check
  2019-06-12 13:07 [PATCH 1/4] drm/i915: Don't clobber M/N values during fastset check Ville Syrjala
@ 2019-06-12 17:24 ` Ville Syrjala
  2019-06-13  9:24   ` Ville Syrjälä
  0 siblings, 1 reply; 4+ messages in thread
From: Ville Syrjala @ 2019-06-12 17:24 UTC (permalink / raw)
  To: intel-gfx; +Cc: stable, Blubberbub, Maarten Lankhorst, Hans de Goede

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

We're now calling intel_pipe_config_compare(..., true) uncoditionally
which means we're always going clobber the calculated M/N values with
the old values if the fuzzy M/N check passes. That causes problems
because the fuzzy check allows for a huge difference in the values.

I'm actually tempted to just make the M/N checks exact, but that might
prevent fastboot from kicking in when people want it. So for now let's
overwrite the computed values with the old values only if decide to skip
the modeset.

v2: Copy has_drrs along with M/N M2/N2 values

Cc: stable@vger.kernel.org
Cc: Blubberbub@protonmail.com
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Hans de Goede <hdegoede@redhat.com>
Tested-by: Blubberbub@protonmail.com
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=110782
Fixes: d19f958db23c ("drm/i915: Enable fastset for non-boot modesets.")
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 36 +++++++++++++++++++++-------
 1 file changed, 28 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 1b1ddb48ca7a..3d8ed1cf0ab7 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12299,9 +12299,6 @@ intel_compare_link_m_n(const struct intel_link_m_n *m_n,
 			      m2_n2->gmch_m, m2_n2->gmch_n, !adjust) &&
 	    intel_compare_m_n(m_n->link_m, m_n->link_n,
 			      m2_n2->link_m, m2_n2->link_n, !adjust)) {
-		if (adjust)
-			*m2_n2 = *m_n;
-
 		return true;
 	}
 
@@ -13433,6 +13430,33 @@ static int calc_watermark_data(struct intel_atomic_state *state)
 	return 0;
 }
 
+static void intel_crtc_check_fastset(struct intel_crtc_state *old_crtc_state,
+				     struct intel_crtc_state *new_crtc_state)
+{
+	struct drm_i915_private *dev_priv =
+		to_i915(new_crtc_state->base.crtc->dev);
+
+	if (!intel_pipe_config_compare(dev_priv, old_crtc_state,
+				       new_crtc_state, true))
+		return;
+
+	new_crtc_state->base.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;
+}
+
 /**
  * intel_atomic_check - validate state object
  * @dev: drm device
@@ -13474,11 +13498,7 @@ static int intel_atomic_check(struct drm_device *dev,
 		if (ret)
 			goto fail;
 
-		if (intel_pipe_config_compare(dev_priv, old_crtc_state,
-					      new_crtc_state, true)) {
-			new_crtc_state->base.mode_changed = false;
-			new_crtc_state->update_pipe = true;
-		}
+		intel_crtc_check_fastset(old_crtc_state, new_crtc_state);
 
 		if (needs_modeset(&new_crtc_state->base))
 			any_ms = true;
-- 
2.21.0


^ permalink raw reply related	[flat|nested] 4+ messages in thread
* Re: [PATCH v2 1/4] drm/i915: Don't clobber M/N values during fastset check
  2019-06-12 17:24 ` [PATCH v2 " Ville Syrjala
@ 2019-06-13  9:24   ` Ville Syrjälä
  2019-06-18 12:19     ` [Intel-gfx] " Imre Deak
  0 siblings, 1 reply; 4+ messages in thread
From: Ville Syrjälä @ 2019-06-13  9:24 UTC (permalink / raw)
  To: intel-gfx; +Cc: stable, Blubberbub, Maarten Lankhorst, Hans de Goede

On Wed, Jun 12, 2019 at 08:24:23PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> We're now calling intel_pipe_config_compare(..., true) uncoditionally
> which means we're always going clobber the calculated M/N values with
> the old values if the fuzzy M/N check passes. That causes problems
> because the fuzzy check allows for a huge difference in the values.
> 
> I'm actually tempted to just make the M/N checks exact, but that might
> prevent fastboot from kicking in when people want it. So for now let's
> overwrite the computed values with the old values only if decide to skip
> the modeset.
> 
> v2: Copy has_drrs along with M/N M2/N2 values
> 
> Cc: stable@vger.kernel.org
> Cc: Blubberbub@protonmail.com
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Tested-by: Blubberbub@protonmail.com
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=110782
> Fixes: d19f958db23c ("drm/i915: Enable fastset for non-boot modesets.")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Looks like also
https://bugs.freedesktop.org/show_bug.cgi?id=110675

> ---
>  drivers/gpu/drm/i915/intel_display.c | 36 +++++++++++++++++++++-------
>  1 file changed, 28 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 1b1ddb48ca7a..3d8ed1cf0ab7 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12299,9 +12299,6 @@ intel_compare_link_m_n(const struct intel_link_m_n *m_n,
>  			      m2_n2->gmch_m, m2_n2->gmch_n, !adjust) &&
>  	    intel_compare_m_n(m_n->link_m, m_n->link_n,
>  			      m2_n2->link_m, m2_n2->link_n, !adjust)) {
> -		if (adjust)
> -			*m2_n2 = *m_n;
> -
>  		return true;
>  	}
>  
> @@ -13433,6 +13430,33 @@ static int calc_watermark_data(struct intel_atomic_state *state)
>  	return 0;
>  }
>  
> +static void intel_crtc_check_fastset(struct intel_crtc_state *old_crtc_state,
> +				     struct intel_crtc_state *new_crtc_state)
> +{
> +	struct drm_i915_private *dev_priv =
> +		to_i915(new_crtc_state->base.crtc->dev);
> +
> +	if (!intel_pipe_config_compare(dev_priv, old_crtc_state,
> +				       new_crtc_state, true))
> +		return;
> +
> +	new_crtc_state->base.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;
> +}
> +
>  /**
>   * intel_atomic_check - validate state object
>   * @dev: drm device
> @@ -13474,11 +13498,7 @@ static int intel_atomic_check(struct drm_device *dev,
>  		if (ret)
>  			goto fail;
>  
> -		if (intel_pipe_config_compare(dev_priv, old_crtc_state,
> -					      new_crtc_state, true)) {
> -			new_crtc_state->base.mode_changed = false;
> -			new_crtc_state->update_pipe = true;
> -		}
> +		intel_crtc_check_fastset(old_crtc_state, new_crtc_state);
>  
>  		if (needs_modeset(&new_crtc_state->base))
>  			any_ms = true;
> -- 
> 2.21.0

-- 
Ville Syrjälä
Intel

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [Intel-gfx] [PATCH v2 1/4] drm/i915: Don't clobber M/N values during fastset check
  2019-06-13  9:24   ` Ville Syrjälä
@ 2019-06-18 12:19     ` Imre Deak
  0 siblings, 0 replies; 4+ messages in thread
From: Imre Deak @ 2019-06-18 12:19 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: intel-gfx, Blubberbub, stable

On Thu, Jun 13, 2019 at 12:24:59PM +0300, Ville Syrjälä wrote:
> On Wed, Jun 12, 2019 at 08:24:23PM +0300, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > We're now calling intel_pipe_config_compare(..., true) uncoditionally
> > which means we're always going clobber the calculated M/N values with
> > the old values if the fuzzy M/N check passes. That causes problems
> > because the fuzzy check allows for a huge difference in the values.
> > 
> > I'm actually tempted to just make the M/N checks exact, but that might
> > prevent fastboot from kicking in when people want it. So for now let's
> > overwrite the computed values with the old values only if decide to skip
> > the modeset.
> > 
> > v2: Copy has_drrs along with M/N M2/N2 values
> > 
> > Cc: stable@vger.kernel.org
> > Cc: Blubberbub@protonmail.com
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Hans de Goede <hdegoede@redhat.com>
> > Tested-by: Blubberbub@protonmail.com
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=110782
> > Fixes: d19f958db23c ("drm/i915: Enable fastset for non-boot modesets.")
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Looks like also
> https://bugs.freedesktop.org/show_bug.cgi?id=110675

Ok, the copying from old-state to new-state is needed to keep HW/SW
state verification later pass, but we want to preserve the calculated
state if we'll need to reprogram everything based on that. Makes sense:

Reviewed-by: Imre Deak <imre.deak@intel.com>

> 
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 36 +++++++++++++++++++++-------
> >  1 file changed, 28 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 1b1ddb48ca7a..3d8ed1cf0ab7 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -12299,9 +12299,6 @@ intel_compare_link_m_n(const struct intel_link_m_n *m_n,
> >  			      m2_n2->gmch_m, m2_n2->gmch_n, !adjust) &&
> >  	    intel_compare_m_n(m_n->link_m, m_n->link_n,
> >  			      m2_n2->link_m, m2_n2->link_n, !adjust)) {
> > -		if (adjust)
> > -			*m2_n2 = *m_n;
> > -
> >  		return true;
> >  	}
> >  
> > @@ -13433,6 +13430,33 @@ static int calc_watermark_data(struct intel_atomic_state *state)
> >  	return 0;
> >  }
> >  
> > +static void intel_crtc_check_fastset(struct intel_crtc_state *old_crtc_state,
> > +				     struct intel_crtc_state *new_crtc_state)
> > +{
> > +	struct drm_i915_private *dev_priv =
> > +		to_i915(new_crtc_state->base.crtc->dev);
> > +
> > +	if (!intel_pipe_config_compare(dev_priv, old_crtc_state,
> > +				       new_crtc_state, true))
> > +		return;
> > +
> > +	new_crtc_state->base.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;
> > +}
> > +
> >  /**
> >   * intel_atomic_check - validate state object
> >   * @dev: drm device
> > @@ -13474,11 +13498,7 @@ static int intel_atomic_check(struct drm_device *dev,
> >  		if (ret)
> >  			goto fail;
> >  
> > -		if (intel_pipe_config_compare(dev_priv, old_crtc_state,
> > -					      new_crtc_state, true)) {
> > -			new_crtc_state->base.mode_changed = false;
> > -			new_crtc_state->update_pipe = true;
> > -		}
> > +		intel_crtc_check_fastset(old_crtc_state, new_crtc_state);
> >  
> >  		if (needs_modeset(&new_crtc_state->base))
> >  			any_ms = true;
> > -- 
> > 2.21.0
> 
> -- 
> Ville Syrjälä
> Intel
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-06-18 12:19 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-12 13:07 [PATCH 1/4] drm/i915: Don't clobber M/N values during fastset check Ville Syrjala
2019-06-12 17:24 ` [PATCH v2 " Ville Syrjala
2019-06-13  9:24   ` Ville Syrjälä
2019-06-18 12:19     ` [Intel-gfx] " Imre Deak

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).