[Intel-gfx] [PATCH] drm/i915: Fix cdclk vs. dev_cdclk mess when not recomputing things
Ville Syrjälä
ville.syrjala at linux.intel.com
Tue Nov 1 12:53:13 UTC 2016
On Tue, Nov 01, 2016 at 02:40:35PM +0200, Ville Syrjälä wrote:
> On Tue, Nov 01, 2016 at 09:57:43AM +0100, Maarten Lankhorst wrote:
> > Op 28-10-16 om 18:59 schreef ville.syrjala at linux.intel.com:
> > > From: Ville Syrjälä <ville.syrjala at 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 at linux.intel.com>
> > > Cc: Mika Kahola <mika.kahola at intel.com>
> > > Cc: bruno.pagani at ens-lyon.org
> > > Cc: Daniel J Blueman <daniel.blueman at gmail.com>
> > > Cc: Paul Bolle <pebolle at tiscali.nl>
> > > Cc: Joseph Yasi <joe.yasi at gmail.com>
> > > Tested-by: Paul Bolle <pebolle at tiscali.nl>
> > > Tested-by: Joseph Yasi <joe.yasi at gmail.com>
> > > Cc: stable at 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 at 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.
>
> It should pretty much be protected by any of the crtc locks, at least
> for now since we don't allow changing it w/o modesetting all the pipes.
> But yeah, nothing should be using it for any checks so could just leave
> it unset.
>
> But this got me thinking about dev_priv->atomic_cdclk_freq. Essentially
> that one is protected by connection_mutex, which we won't be holding
> for the !modeset case. So I think using it there is a bit dubious. I
> guess it would require a modeset on one pipe that doesn't actually
> end up changing the cdclk frequency but which changes atomic_cdclk_freq,
> and a parallel plane update on another pipe. I guess that would mean
> both pipes would have be !active at the time so that dev_cdclk remains
> stable. Seems to me that we'd need to lock all the crtcs (without
> forcing a modeset on them) when atomic_cdclk changes.
Something like this:
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 093af6e4ab40..532a932a1ff9 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13921,6 +13921,21 @@ static int haswell_mode_set_planes_workaround(struct drm_atomic_state *state)
return 0;
}
+static int intel_lock_all_pipes(struct drm_atomic_state *state)
+{
+ struct drm_crtc *crtc;
+ struct drm_crtc_state *crtc_state;
+
+ /* add all pipes to the state */
+ for_each_crtc(state->dev, crtc) {
+ crtc_state = drm_atomic_get_crtc_state(state, crtc);
+ if (IS_ERR(crtc_state))
+ return PTR_ERR(crtc_state);
+ }
+
+ return 0;
+}
+
static int intel_modeset_all_pipes(struct drm_atomic_state *state)
{
struct drm_crtc *crtc;
@@ -13993,17 +14008,33 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
if (ret < 0)
return ret;
+ /*
+ * All pipes must be switched off while we change the cdclk.
+ *
+ * Even if we don't end up changing the actual cdclk frequency
+ * (dev_priv->cdclk_freq) we must lock all the crtcs if the
+ * logical cdclk frequency (dev_priv->atomic_cdclk_freq) needs to
+ * be updated since non-modeset operations will depend on its
+ * current value.
+ */
if (intel_state->dev_cdclk != dev_priv->cdclk_freq ||
intel_state->cdclk_pll_vco != dev_priv->cdclk_pll.vco)
ret = intel_modeset_all_pipes(state);
+ else if (intel_state->cdclk != dev_priv->atomic_cdclk_freq)
+ ret = intel_lock_all_pipes(state);
if (ret < 0)
return ret;
DRM_DEBUG_KMS("New cdclk calculated to be atomic %u, actual %u\n",
intel_state->cdclk, intel_state->dev_cdclk);
- } else
+ } else {
+ /*
+ * We know that this well never change,
+ * so no need to lock all the crtcs here.
+ */
to_intel_atomic_state(state)->cdclk = dev_priv->atomic_cdclk_freq;
+ }
intel_modeset_clear_plls(state);
>
> >
> > 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 at linux.intel.com>
>
> --
> Ville Syrjälä
> Intel OTC
--
Ville Syrjälä
Intel OTC
More information about the Intel-gfx
mailing list