* Re: [PATCH v2 1/3] drm/i915: Fix cdclk vs. dev_cdclk mess when not recomputing things
2016-11-14 16:35 ` [PATCH v2 1/3] drm/i915: Fix cdclk vs. dev_cdclk mess when not recomputing things ville.syrjala
@ 2016-11-15 10:15 ` Maarten Lankhorst
2016-11-17 8:17 ` Paul Bolle
1 sibling, 0 replies; 3+ messages in thread
From: Maarten Lankhorst @ 2016-11-15 10:15 UTC (permalink / raw)
To: ville.syrjala, intel-gfx
Cc: Mika Kahola, bruno.pagani, Daniel J Blueman, Paul Bolle,
Joseph Yasi, stable
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>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH v2 1/3] drm/i915: Fix cdclk vs. dev_cdclk mess when not recomputing things
2016-11-14 16:35 ` [PATCH v2 1/3] drm/i915: Fix cdclk vs. dev_cdclk mess when not recomputing things ville.syrjala
2016-11-15 10:15 ` Maarten Lankhorst
@ 2016-11-17 8:17 ` Paul Bolle
1 sibling, 0 replies; 3+ messages in thread
From: Paul Bolle @ 2016-11-17 8:17 UTC (permalink / raw)
To: ville.syrjala, intel-gfx
Cc: Maarten Lankhorst, Mika Kahola, bruno.pagani, Daniel J Blueman,
Joseph Yasi, stable
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
^ permalink raw reply [flat|nested] 3+ messages in thread