Re: [PATCH 33/42] drm/i915: remove crtc->active tracking completely

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, May 12, 2015 at 06:01:40PM +0100, Daniel Stone wrote:
> Hi,
> 
> On 12 May 2015 at 17:57, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Tue, May 12, 2015 at 04:07:14PM +0200, Maarten Lankhorst wrote:
> >> Op 12-05-15 om 12:03 schreef Daniel Vetter:
> >> > On Mon, May 11, 2015 at 4:25 PM, Maarten Lankhorst
> >> > <maarten.lankhorst@linux.intel.com> wrote:
> >> >> @@ -11953,16 +11930,14 @@ check_shared_dpll_state(struct drm_device *dev)
> >> >>                 for_each_intel_crtc(dev, crtc) {
> >> >>                         if (crtc->base.state->active && intel_crtc_to_shared_dpll(crtc) == pll)
> >> >> -                               enabled_crtcs++;
> >> >> -                       if (crtc->active && intel_crtc_to_shared_dpll(crtc) == pll)
> >> >>                                 active_crtcs++;
> >> >>                 }
> >> >>                 I915_STATE_WARN(pll->active != active_crtcs,
> >> >>                      "pll active crtcs mismatch (expected %i, found %i)\n",
> >> >>                      pll->active, active_crtcs);
> >> >> -               I915_STATE_WARN(hweight32(pll->config.crtc_mask) != enabled_crtcs,
> >> >> +               I915_STATE_WARN(hweight32(pll->config.crtc_mask) != active_crtcs,
> >> >>                      "pll enabled crtcs mismatch (expected %i, found %i)\n",
> >> >> -                    hweight32(pll->config.crtc_mask), enabled_crtcs);
> >> >> +                    hweight32(pll->config.crtc_mask), active_crtcs);
> >> >
> >> > Missed one: Why do you remove this? Imo that's a fairly crucial
> >> > consistency check.
> >> > -Daniel
> >> It's not removed, but crtc->active is the same as crtc->base.state->active now. The check still works as intended. :-)
> >
> > Oh there's a confusion (maybe from ealier patches?). You derive
> > enabled_crtcs from state->active, but it should be derived from
> > state->enable. That's at least the case in current -nightly.
> >
> > And active_crtcs should be derived from state->active ofc (currently
> > looking at intel_crtc->active). If I'm piecing the patches together the
> > active_crtcs case gets removed and the enabled_crtcs is mixing things up
> > after your series. We definitely need both of them still I think.
> 
> I'll freely admit to getting lost in the maze, but is it that
> crtc->base.state->active / enabled_crtcs is testing the to-be-enabled
> set (pending state), and crtc->active / active_crtcs is testing the
> was-previously-enabled set (old state)? If so, these checks are indeed
> different, but it's kind of hard to tell. And we'd need to capture the
> entire previous state to pass in. Maybe those would be better as two
> separate checks; one before we swap the state, and one after?

This is state config cross-checks done at the end, so never looks at
transitions but only at snapshots.

enabled = logically enabled, i.e. resources are reserved to make sure dpms
on will succeed
active = hw actually running

And active always implies enabled.

We have that distinction both on the crtc and by necessity also on the
dplls used by them. But somehow that got a bit mixed up in Maarten's
series here. Current -nightly still keeps them nicely separate.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx