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

Ville Syrjälä
2019-06-13T09:25:03Z

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