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