[Intel-gfx] [PATCH] drm/i915: Get audio power domain during initial hw readout

[Bob Paauwe]

On Wed, 13 Apr 2016 21:27:17 +0300
Ville Syrjälä <ville.syrjala at linux.intel.com> wrote:

> On Wed, Apr 13, 2016 at 11:19:20AM -0700, Bob Paauwe wrote:
> > On Wed, 13 Apr 2016 11:59:43 +0300
> > Ville Syrjälä <ville.syrjala at linux.intel.com> wrote:
> >   
> > > On Tue, Apr 12, 2016 at 03:52:48PM -0700, Bob Paauwe wrote:  
> > > > if the crtc has audio is enabled. Otherwise, when the first atomic
> > > > modeset happens it will warn when trying to drop the audio power
> > > > domain.
> > > > 
> > > > Signed-off-by: Bob Paauwe <bob.j.paauwe at intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_display.c | 5 +++++
> > > >  1 file changed, 5 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > > index 5155efb6..caeb3e1 100644
> > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > @@ -10561,6 +10561,7 @@ found:
> > > >  	}
> > > >  
> > > >  	ret = intel_modeset_setup_plane_state(state, crtc, mode, fb, 0, 0);
> > > > +
> > > >  	if (ret)
> > > >  		goto fail;
> > > >  
> > > > @@ -15998,6 +15999,10 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
> > > >  
> > > >  		memset(&crtc->base.mode, 0, sizeof(crtc->base.mode));
> > > >  		if (crtc->base.state->active) {
> > > > +			if (crtc->config->has_audio)
> > > > +				intel_display_power_get(dev_priv,
> > > > +							POWER_DOMAIN_AUDIO);
> > > > +    
> > > 
> > > Hmm. Ideally we would do this alongside the other power doamin stuff in
> > > intel_modeset_setup_hw_state(), but that's too late for
> > > intel_sanitize_{crtc,encoder}(). So I guess we'll have to do it before
> > > those, or we have to pull the audio power domain stuff out from the
> > > encoder hooks. Or perhaps even throw it out entirely, since I'm not sure
> > > it's doing anything for us.
> > > 
> > > If we do leave the power domain handling in the encoder hooks, then
> > > this is not quite the right place to grab the reference. We could have
> > > an encoder enabled w/o an active pipe, in which case
> > > intel_sanitize_encoder() would shut off the encoder, so we'd still end
> > > up with a bad refcount.
> > >  
> > 
> > Thanks Ville for the details.  I'm not at all familiar with how this is
> > supposed to work so this helps some.  But I'm still not sure I follow.
> > 
> > If I move this to get_crtc_power_domains() by sticking 
> > 
> >    if (crtc_state->has_audio)
> > 	mask |= BIT(POWER_DOMAIN_AUDIO);
> > 
> > at the end, which is where I think you're suggesting initially, it
> > seems to work.  So I'm not sure why that's too late.  Also, I'm not
> > seeing where shutting off the encoder will effect the power domain
> > refcounts.  
> 
> Some of the encoder hooks will do the POWER_DOMAIN_AUDIO put/get, so if
> sanitize_{crtc,encoder}() call just the disable hooks, we'll drop one to
> many power references.
> 
> That said, I'm still not sure why we do the audio power domain frobbing
> in the encoder code. That is, how can we end up enabling/disabling a port
> with has_audio==true without the relevant power wells being on already.
> 
> Looks like some of this stuff was added in
> commit d45a0bf549cd ("drm/i915: grab the audio power domain when enabling audio on HSW+")
> so maybe Paulo knows?

I tried removing audio power domain frobbing from the encoder code (for
DP) and it didn't cause any problems on BXT. But I haven't been able to
run the pm_rpm tests on my BXT yet to verify that it doesn't cause any
regressions that way.

In any case, I sent a v2 that should have it setting the mask in the
correct place to at least resolve the warning messages I'm seeing.

Bob
> 



-- 
--
Bob Paauwe                  
Bob.J.Paauwe at intel.com
IOTG / PED Software Organization
Intel Corp.  Folsom, CA
(916) 356-6193