| Message ID | 1460501568-29102-1-git-send-email-bob.j.paauwe@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@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. > intel_mode_from_pipe_config(&crtc->base.mode, crtc->config); > intel_mode_from_pipe_config(&crtc->base.state->adjusted_mode, crtc->config); > WARN_ON(drm_atomic_set_mode_for_crtc(crtc->base.state, &crtc->base.mode)); > -- > 2.5.5 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, 13 Apr 2016 11:59:43 +0300 Ville Syrjälä <ville.syrjala@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@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. Bob > > intel_mode_from_pipe_config(&crtc->base.mode, crtc->config); > > intel_mode_from_pipe_config(&crtc->base.state->adjusted_mode, crtc->config); > > WARN_ON(drm_atomic_set_mode_for_crtc(crtc->base.state, &crtc->base.mode)); > > -- > > 2.5.5 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx >
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@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@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?
On Wed, 13 Apr 2016 21:27:17 +0300 Ville Syrjälä <ville.syrjala@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@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@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 >
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); + intel_mode_from_pipe_config(&crtc->base.mode, crtc->config); intel_mode_from_pipe_config(&crtc->base.state->adjusted_mode, crtc->config); WARN_ON(drm_atomic_set_mode_for_crtc(crtc->base.state, &crtc->base.mode));
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@intel.com> --- drivers/gpu/drm/i915/intel_display.c | 5 +++++ 1 file changed, 5 insertions(+)