Re: [Intel-gfx] [PATCH v2 1/4] drm/i915: Don't clobber M/N values during fastset check

[Imre Deak <imre.deak@intel.com>]

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