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

[Daniel Stone]

Hi,

On 12 May 2015 at 17:57, Daniel Vetter <daniel at 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 at 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?

Cheers,
Daniel