Re: [PATCH 4/5] drm/i915: take power well refs when needed

Paulo Zanoni
2013-10-15T20:47:20Z

2013/10/15 Jesse Barnes <jbarnes@virtuousgeek.org>:
> On Tue, 15 Oct 2013 16:54:00 -0300
> Paulo Zanoni <przanoni@gmail.com> wrote:
>
>> 2013/10/14 Jesse Barnes <jbarnes@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