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

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

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@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 | 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.

> 
> 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.


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@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