[Intel-gfx] [PATCH 4/5] drm/i915: take power well refs when needed

[Paulo Zanoni]

2013/10/15 Jesse Barnes <jbarnes at virtuousgeek.org>:
> On Tue, 15 Oct 2013 16:54:00 -0300
> Paulo Zanoni <przanoni at gmail.com> wrote:
>
>> 2013/10/14 Jesse Barnes <jbarnes at virtuousgeek.org>:
>> > When accessing the display regs for hw state readout or cross check, we
>> > need to make sure the power well is enabled so we can read valid
>> > register state.
>>
>> On the current code (HSW) we already check for the power wells in the
>> HW state readout code: if the power well is disabled, then transcoders
>> A/B/C are disabled. The current code takes care to not touch registers
>> on a disabled power well.
>
> Yeah and we should probably preserve that behavior, for the readout
> code at least.
>
>> > +       intel_display_power_put(dev, POWER_DOMAIN_PIPE(0));
>> > +
>>
>> Why is this here? It certainly deserves a comment in the code.
>
> It probably needs to be removed.  The refcounting for the initial load
> is still screwy due to the unconditional enable later on.
>
>> So now HSW uses global_resources while VLV uses crtc enable/disable. I
>> really think both platforms should try to do the same thing.
>
> Definitely agreed.
>
>> By the way, at least on Haswell, if we do an equivalent change we'll
>> have problems, because when you disable the power well at
>> crtc_disable, then everything you did at crtc_mode_set will be undone,
>> and it won't be redone at crtc_enable. When you reenable the power
>> well, the registers go back to their default values, not the values
>> that were previously there. Did you check if VLV behaves the same?
>
> No that's taken into account here.  In __intel_set_mode we take a
> private ref on the appropriate power well so that we'll preserve state
> until we do the first crtc_enable.  From then on, the ref is tracked
> there and we drop the private one in __intel_set_mode

Yeah, but then after all this is done, at some point we'll call
crtc_disable, then we'll put the last ref back and then the power well
will be disabled. Then the user moves the mouse and we call
crtc_enable, so we enable the power well, all its registers to back to
their reset state, then crtc_enable sets some of the registers, but
that won't really be enough since no one called crtc_mode_set again,
and all the state set by crtc_mode_set (not touched by crtc_enable) is
back to the default values. No? What am I missing?

>
>> > +               if (crtc->active)
>> > +                       intel_display_power_get(dev,
>> > +                                               POWER_DOMAIN_PIPE(crtc->pipe));
>> > +
>>
>> What about the panel fitter power domains? Sometimes the panel fitter
>> is the thing that makes you require a power well, even though you're
>> on a pipe that doesn't need it.
>>
>> And on Haswell you also have to take into account
>> TRANSCODER_EDP+PIPE_A versus TRANSCODER_A+PIPE_A, where the first
>> doesn't need the power well but the second needs it.
>
> Yeah I'm still not sure how to handle this in generic code.  Maybe the
> power well mapping function Imre added will be enough, but it
> definitely gets tricky when we look at all the different platforms we
> have to (and will have to) handle.
>
> Thanks,
> --
> Jesse Barnes, Intel Open Source Technology Center



-- 
Paulo Zanoni