On Tue, May 12, 2015 at 06:01:40PM +0100, Daniel Stone wrote:
> Hi,
>
> On 12 May 2015 at 17:57, Daniel Vetter <daniel@xxxxxxxx> 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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
