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

[Jesse Barnes]

On Wed, 16 Oct 2013 14:10:13 +0300
Imre Deak <imre.deak at intel.com> wrote:

> On Tue, 2013-10-15 at 13:40 -0700, Jesse Barnes wrote:
> > On Tue, 15 Oct 2013 16:54:00 -0300
> > Paulo Zanoni <przanoni at gmail.com> wrote:
> > [...]
> > 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.
> 
> Isn't the power domain abstraction a neat idea exactly for the above
> case? Generic code just asks for the domain it needs and doesn't care
> how it maps to power wells on the given platform. So for transcoder_edp
> +pipe_a it'd end up asking for POWER_DOMAIN_PIPE_A and
> POWER_DOMAIN_TRANSCODER_EDP, both of which is a nop on HSW, and for the
> other case POWER_DOMAIN_PIPE_A and POWER_DOMAIN_TRANSCODER_A which would
> enable the power well. You also have the POWER_DOMAIN_PIPE,
> POWER_DOMAIN_TRANSCODER, POWER_DOMAIN_PIPE_PANEL_FITTER helpers already.

Yeah I think it can work.  I missed your function that takes a crtc
though as well, so we don't end up polluting the generic functions with
TRANSCODER references that don't exist on the Atom platforms for
example.  That's the main thing I'm worried about, since as we get more
and more wells I think it'll get easier to get it wrong in the generic
code, if we have to use all the required domains for all platforms
there.

-- 
Jesse Barnes, Intel Open Source Technology Center