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ä <[email protected]>
> > 
> > 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: [email protected]
> > Cc: [email protected]
> > Cc: Maarten Lankhorst <[email protected]>
> > Cc: Hans de Goede <[email protected]>
> > Tested-by: [email protected]
> > 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ä <[email protected]>
> 
> 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 <[email protected]>

> 
> > ---
> >  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
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Reply via email to