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@xxxxxxxxxxxxxxx>
> > > Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>
> > > Signed-off-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
> > > ---
> > > 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@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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
