[Intel-gfx] [PATCH 1/3] drm/i915/gen9: Initialize intel_state->active_crtcs during WM sanitization

[Daniel Vetter]

On Fri, Jun 17, 2016 at 02:00:02PM -0700, Matt Roper wrote:
> On Mon, Jun 13, 2016 at 04:49:49PM +0200, Daniel Vetter wrote:
> > On Thu, Jun 09, 2016 at 03:14:53PM -0700, Matt Roper wrote:
> > > intel_state->active_crtcs is usually only initialized when doing a
> > > modeset.  During our first atomic commit after boot, we're effectively
> > > faking a modeset to sanitize the DDB/wm setup, so ensure that this field
> > > gets initialized before use.
> > > 
> > > Reported-by: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>
> > > Cc: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>
> > > Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_pm.c | 11 ++++++++++-
> > >  1 file changed, 10 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > index 658a756..0cd38ca 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -3896,9 +3896,18 @@ skl_compute_ddb(struct drm_atomic_state *state)
> > >  	 * pretend that all pipes switched active status so that we'll
> > >  	 * ensure a full DDB recompute.
> > >  	 */
> > > -	if (dev_priv->wm.distrust_bios_wm)
> > > +	if (dev_priv->wm.distrust_bios_wm) {
> > >  		intel_state->active_pipe_changes = ~0;
> > >  
> > > +		/*
> > > +		 * We usually only initialize intel_state->active_crtcs if we
> > > +		 * we're doing a modeset; make sure this field is always
> > > +		 * initialized during the sanitization process that happens
> > > +		 * on the first commit too.
> > > +		 */
> > > +		intel_state->active_crtcs = dev_priv->active_crtcs;
> > > +	}
> > 
> > Can't we move input sanitization out of this? Imo mixing up atomic
> > check/compute config code with hw state restoring is way too fragile.
> 
> Handling this at readout time is tricky since we don't actually
> reconstruct things like framebuffers until much later in the process.
> Also, if I remember correctly, we don't actually read out sufficient
> hardware state on any platform right now (e.g., non-primary planes
> aren't read, gen9 plane scalers and such are never read, etc.).  So to
> truly sanitize properly/safely, we need to run through a real atomic
> commit to make sure that our sanitized values actually match how the
> hardware is programmed.  I decided to do the sanitization steps as part
> of the first real commit rather than trigger an extra "modeset" on
> driver boot, but I can look at the other approach if you think it's
> better.
> 
> Long term we should probably try harder to read out more complete
> hardware state, but for now I figured it was better to get a short-term
> fix since there are real regressions (as reported by Tvrtko) and I might
> not have time to do a more complicated hw-readout rework series for a
> little while.

I'm just unhappy with gunking up the atomic check/commit paths. Could we
stuff this into the plane state readout/fixup paths instead? That also has
the benefit of keeping all the readout logic more together in one place.

> > Also, why exactly do we have active_crtcs? Seems to just be duplicated
> > state that can get out of sync, we have too many of those already ...
> > -Daniel
> 
> I think it would be harder to reconstruct this information without the
> active_crtcs field.  You'd have to grab the CRTC state (and related
> locks) for CRTC's that weren't actually part of the current commit.
> Technically we have to do that anyway on gen9 for DDB allocation
> reasons, but I don't think that's the case for other platforms iirc.

Yeah, if the use case is strictly read-only (like we have active_planes
on the crtc) then I think it's ok. Might even be something the core
helpers could/should track for us in drm_atomic_state. But for that we
first need to create a real drm_device->mode_config->state pointer. Seems
a bit excessive for just active_crtcs.
-Daniel

> 
> 
> Matt
> 
> > 
> > > +
> > >  	/*
> > >  	 * If the modeset changes which CRTC's are active, we need to
> > >  	 * recompute the DDB allocation for *all* active pipes, even
> > > -- 
> > > 2.1.4
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx at 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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch