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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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