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 <[email protected]> > > Cc: Tvrtko Ursulin <[email protected]> > > Signed-off-by: Matt Roper <[email protected]> > > --- > > 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 > > [email protected] > > 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 [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
