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

[Imre Deak <imre.deak@intel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3038 bytes --]

On Wed, 2013-10-16 at 08:08 -0700, Jesse Barnes wrote:
> On Wed, 16 Oct 2013 14:10:13 +0300
> Imre Deak <imre.deak@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@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.

Afaics, on VLV for example we'd ask for pipe A/B and transcoder A/B
power domains, which is still correct. It's true that there the
pipe-transcoder connection is fixed, and so we'll always ask for the
same pipe/transcoder power domain pair, but I think it's still ok
conceptually.

So atm the power domains as defined are platform independent, which is
great. If we can't avoid adding a platform specific ones in the future,
we could still handle those in platform specific code.

--Imre

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx