Re: [PATCH] drm/i915: Fix cdclk vs. dev_cdclk mess when not recomputing things

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Op 28-10-16 om 18:59 schreef ville.syrjala@linux.intel.com:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> When we end up not recomputing the cdclk, we need to populate
> intel_state->cdclk with the "atomic_cdclk_freq" instead of the
> current cdclk_freq. When no pipes are active, the actual cdclk_freq
> may be lower than what the configuration of the planes and
> pipes would require from the point of view of the software state.
>
> intel_state->dev_cdclk is the computed actual cdclk in such cases,
> so let's populate that with the current cdclk value. Although basically
> nothing should ever use dev_cdclk for any checks and whatnot.
>
> This fixes bogus WARNS from skl_max_scale() which is trying to check
> the plane software state against the cdclk frequency. So any time
> it got called during DPMS off for instance, we might have tripped
> the warn if the current mode would have required a higher than
> minimum cdclk.
>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Mika Kahola <mika.kahola@intel.com>
> Cc: bruno.pagani@ens-lyon.org
> Cc: Daniel J Blueman <daniel.blueman@gmail.com>
> Cc: Paul Bolle <pebolle@tiscali.nl>
> Cc: Joseph Yasi <joe.yasi@gmail.com>
> Tested-by: Paul Bolle <pebolle@tiscali.nl>
> Tested-by: Joseph Yasi <joe.yasi@gmail.com>
> Cc: stable@vger.kernel.org
> Fixes: 1a617b77658e ("drm/i915: Keep track of the cdclk as if all crtc's were active.")
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98214
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 895b3dc50e3f..f010e154e33e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14040,8 +14040,10 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
>  
>  		DRM_DEBUG_KMS("New cdclk calculated to be atomic %u, actual %u\n",
>  			      intel_state->cdclk, intel_state->dev_cdclk);
> -	} else
> +	} else {
>  		to_intel_atomic_state(state)->cdclk = dev_priv->atomic_cdclk_freq;
> +		to_intel_atomic_state(state)->dev_cdclk = dev_priv->cdclk_freq;
> +	}
This shouldn't be required in this case, but might as well do so since it doesn't hurt either.
>  	intel_modeset_clear_plls(state);
>  
> @@ -14142,8 +14144,10 @@ static int intel_atomic_check(struct drm_device *dev,
>  
>  		if (ret)
>  			return ret;
> -	} else
> -		intel_state->cdclk = dev_priv->cdclk_freq;
> +	} else {
> +		intel_state->cdclk = dev_priv->atomic_cdclk_freq;
> +		intel_state->dev_cdclk = dev_priv->cdclk_freq;
> +	}
We shouldn't rely on dev_cdclk being valid for the !modeset case.
Best to keep it zero there, the global cdclk can't be changed and the non-modeset case shouldn't rely on the current setting.

Otherwise looks sane, I have a similar patch in my tree. I didn't submit it yet but the fix was similar. Except for the
dev_cdclk stuff.

With the last dev_cdclk assignment removed:

Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>