On Wed, 13 Apr 2016 21:27:17 +0300
Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> 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@xxxxxxxxx>
> > > > ---
> > > > 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@xxxxxxxxx
IOTG / PED Software Organization
Intel Corp. Folsom, CA
(916) 356-6193
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
