On Tue, Nov 01, 2016 at 09:57:43AM +0100, Maarten Lankhorst wrote:
> Op 28-10-16 om 18:59 schreef ville.syrjala@xxxxxxxxxxxxxxx:
> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxx>
> > Cc: Mika Kahola <mika.kahola@xxxxxxxxx>
> > Cc: bruno.pagani@xxxxxxxxxxxx
> > Cc: Daniel J Blueman <daniel.blueman@xxxxxxxxx>
> > Cc: Paul Bolle <pebolle@xxxxxxxxxx>
> > Cc: Joseph Yasi <joe.yasi@xxxxxxxxx>
> > Tested-by: Paul Bolle <pebolle@xxxxxxxxxx>
> > Tested-by: Joseph Yasi <joe.yasi@xxxxxxxxx>
> > Cc: stable@xxxxxxxxxxxxxxx
> > 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@xxxxxxxxxxxxxxx>
> > ---
> > 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.
>
> 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@xxxxxxxxxxxxxxx>
--
Ville Syrjälä
Intel OTC
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html