Re: [PATCH 2/3] drm/i915/gen9: Compute data rates for all planes on first commit

[Matt Roper <matthew.d.roper@intel.com>]

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@linux.intel.com>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > ---
> >  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@lists.freedesktop.org
> > 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx