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

[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

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