On Mon, Jun 13, 2016 at 04:50:31PM +0200, Daniel Vetter wrote:
> On Thu, Jun 09, 2016 at 03:14:54PM -0700, Matt Roper wrote:
> > When we sanitize our DDB and watermark info during the first atomic
> > commit, we need to calculate the total data rate. Since we haven't
> > explicitly added the planes for each CRTC to our atomic state, the total
> > data rate calculation will try to use the cached values from a previous
> > commit (which are 0 since there was no previous commit); this result is
> > incorrect if we inherited any active planes from the BIOS.
> >
> > During our very first atomic commit, we need to explicitly add all
> > active planes to the atomic state to ensure that valid data rate values
> > are calculated for them. Subsequent commits will then have valid cached
> > values to fall back on.
> >
> > Reported-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>
> > Signed-off-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/intel_pm.c | 12 ++++++++++++
> > 1 file changed, 12 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 0cd38ca..ba08639 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3933,6 +3933,18 @@ skl_compute_ddb(struct drm_atomic_state *state)
> > if (IS_ERR(cstate))
> > return PTR_ERR(cstate);
> >
> > + /*
> > + * If this is our first commit after hw readout, we don't have
> > + * valid data rate values cached. Add all planes to ensure we
> > + * calculate a valid data rate.
> > + */
> > + if (dev_priv->wm.distrust_bios_wm) {
> > + ret = drm_atomic_add_affected_planes(state,
> > + &intel_crtc->base);
>
> Again, imo should be pulled out. Other wm code probably has the exact same
> bug.
> -Daniel
By "pulled out" do you mean ensure the initial data rates are calculated
at readout time? As I mentioned on the other email, we don't actually
read out non-primary plane states, plane scaler states, etc., so we
don't actually have all the information we need to handle this at
readout time; out hardware and software states aren't truly in sync
until the first atomic commit happens.
We should get better at reading out more hardware state, but that's a
bit more involved and I've been short on upstream development time
lately so I wanted to make sure I had a timely fix for the regressions
Tvrtko reported.
Matt
>
> > + if (ret)
> > + return ret;
> > + }
> > +
> > ret = skl_allocate_pipe_ddb(cstate, ddb);
> > if (ret)
> > return ret;
> > --
> > 2.1.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
--
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
