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

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

[ville.syrjala]

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.

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.

v2: Drop the dev_cdclk stuff (Maarten)

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> (v1)
Tested-by: Joseph Yasi <joe.yasi@gmail.com> (v1)
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>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e48d9571c99d..70f3f0b70263 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14027,8 +14027,9 @@ 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;
+	}
 
 	intel_modeset_clear_plls(state);
 
@@ -14129,8 +14130,9 @@ 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;
+	}
 
 	ret = drm_atomic_helper_check_planes(dev, state);
 	if (ret)
-- 
2.7.4

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

[Maarten Lankhorst]

Op 14-11-16 om 17:35 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.
>
> 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.
>
> v2: Drop the dev_cdclk stuff (Maarten)
>
> 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> (v1)
> Tested-by: Joseph Yasi <joe.yasi@gmail.com> (v1)
> 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>
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index e48d9571c99d..70f3f0b70263 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14027,8 +14027,9 @@ 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;
> +	}
>  
>  	intel_modeset_clear_plls(state);
>  
> @@ -14129,8 +14130,9 @@ 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;
> +	}
>  
>  	ret = drm_atomic_helper_check_planes(dev, state);
>  	if (ret)

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

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

[Paul Bolle]

On Mon, 2016-11-14 at 18:35 +0200, ville.syrjala@linux.intel.com wrote:
> 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.
> 
> 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.
> 
> v2: Drop the dev_cdclk stuff (Maarten)
> 
> 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> (v1)

I've run v2 of this patch (on top of v4.8.8) for over a day now without
hitting the WARN_ON_ONCE. Of course, my machine was suspended for large parts
of that period. But still, the WARN_ON_ONCE used to be triggered much quicker.
So in short: you can drop "(v1)" as I tested both versions now.

By the way, the scary i915 *ERROR*s are gone now too, as are the visual
glitches that accompanied those *ERROR*s. Apparently the v4.8.y series picked
up a few fixes. Those made i915 a much better experience. Nice! 

> Tested-by: Joseph Yasi <joe.yasi@gmail.com> (v1)
> Cc: stable@vger.kernel.org
> Fixes: 1a617b77658e ("drm/i915: Keep track of the cdclk as if all crtc's were active.")

(I seem to remember discussing the reasons why a v4.6 bug was first noticed on
v4.8. I haven't looked into that yet. By now it's unlikely I ever will. Sorry
about that.)

> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98214
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Thanks,


Paul Bolle