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

Imre Deak
2019-06-18T12:19:17Z

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