On Wed, Sep 12, 2018 at 08:18:37PM +0200, Takashi Iwai wrote:
> On Wed, 12 Sep 2018 15:18:47 +0200,
> Imre Deak wrote:
> >
> > +Takashi
> >
> > On Wed, Sep 12, 2018 at 04:18:12PM +0300, Imre Deak wrote:
> > > Hi Kumar, Takashi,
> > >
> > > On Tue, Jun 19, 2018 at 03:01:11PM -0700, Abhay Kumar wrote:
> > > > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > >
> > > > CDCLK has to be at least twice the BLCK regardless of audio. Audio
> > > > driver has to probe using this hook and increase the clock even in
> > > > absence of any display.
> > > >
> > > > v2: Use atomic refcount for get_power, put_power so that we can
> > > > call each once(Abhay).
> > > > v3: Reset power well 2 to avoid any transaction on iDisp link
> > > > during cdclk change(Abhay).
> > > > v4: Remove Power well 2 reset workaround(Ville).
> > > > v5: Remove unwanted Power well 2 register defined in v4(Abhay).
> > > >
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > > Signed-off-by: Abhay Kumar <abhay.kumar@xxxxxxxxx>
> > > > ---
> > > > drivers/gpu/drm/i915/i915_drv.h | 3 ++
> > > > drivers/gpu/drm/i915/intel_audio.c | 67 +++++++++++++++++++++++++++++++++---
> > > > drivers/gpu/drm/i915/intel_cdclk.c | 29 +++++-----------
> > > > drivers/gpu/drm/i915/intel_display.c | 7 +++-
> > > > drivers/gpu/drm/i915/intel_drv.h | 2 ++
> > > > 5 files changed, 83 insertions(+), 25 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > > index 6104d7115054..a4a386a5db69 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > @@ -1702,6 +1702,7 @@ struct drm_i915_private {
> > > > unsigned int hpll_freq;
> > > > unsigned int fdi_pll_freq;
> > > > unsigned int czclk_freq;
> > > > + u32 get_put_refcount;
> > > >
> > > > struct {
> > > > /*
> > > > @@ -1719,6 +1720,8 @@ struct drm_i915_private {
> > > > struct intel_cdclk_state actual;
> > > > /* The current hardware cdclk state */
> > > > struct intel_cdclk_state hw;
> > > > +
> > > > + int force_min_cdclk;
> > > > } cdclk;
> > > >
> > > > /**
> > > > diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> > > > index 3ea566f99450..ca8f04c7cbb3 100644
> > > > --- a/drivers/gpu/drm/i915/intel_audio.c
> > > > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > > > @@ -618,7 +618,6 @@ void intel_audio_codec_enable(struct intel_encoder *encoder,
> > > >
> > > > if (!connector->eld[0])
> > > > return;
> > > > -
> > > > DRM_DEBUG_DRIVER("ELD on [CONNECTOR:%d:%s], [ENCODER:%d:%s]\n",
> > > > connector->base.id,
> > > > connector->name,
> > > > @@ -713,14 +712,74 @@ void intel_init_audio_hooks(struct drm_i915_private *dev_priv)
> > > > }
> > > > }
> > > >
> > > > +static void glk_force_audio_cdclk(struct drm_i915_private *dev_priv,
> > > > + bool enable)
> > > > +{
> > > > + struct drm_modeset_acquire_ctx ctx;
> > > > + struct drm_atomic_state *state;
> > > > + int ret;
> > > > +
> > > > + drm_modeset_acquire_init(&ctx, 0);
> > > > + state = drm_atomic_state_alloc(&dev_priv->drm);
> > > > + if (WARN_ON(!state))
> > > > + return;
> > > > +
> > > > + state->acquire_ctx = &ctx;
> > > > +
> > > > +retry:
> > > > + to_intel_atomic_state(state)->modeset = true;
> > > > + to_intel_atomic_state(state)->cdclk.force_min_cdclk =
> > > > + enable ? 2 * 96000 : 0;
> > > > +
> > > > + /*
> > > > + * Protects dev_priv->cdclk.force_min_cdclk
> > > > + * Need to lock this here in case we have no active pipes
> > > > + * and thus wouldn't lock it during the commit otherwise.
> > > > + */
> > > > + ret = drm_modeset_lock(&dev_priv->drm.mode_config.connection_mutex, &ctx);
> > > > + if (!ret)
> > > > + ret = drm_atomic_commit(state);
> > >
> > > Ville mentioned a potential dead-lock problem here: a normal modeset
> > > enabling an HDMI/DP output with an audio sink will call the
> > > drm_audio_component_audio_ops::pin_eld_notify() callback with the
> > > connection_mutex already held. This callback in turn could call
> > > drm_audio_component_ops::get_power()/put_power() callbacks and
> > > dead-lock at the above drm_modeset_lock() call.
> > >
> > > Looks like that
> > >
> > > sound/pci/hda/patch_hdmi.c / intel_pin_eld_notify()->
> > > check_presence_and_report()->
> > > hdmi_present_sense()
> > >
> > > would prevent this, but for instance
> > >
> > > sound/soc/codecs/hdac_hdmi.c / hdac_hdmi_eld_notify_cb()->
> > > hdac_hdmi_present_sense()->
> > > hdac_hdmi_jack_report()->
> > > snd_soc_jack_report()->
> > > snd_soc_dapm_sync()->
> > > snd_soc_dapm_sync_unlocked()->
> > > dapm_power_widgets()->
> > > dapm_pre_sequence_async()
> > >
> > > could call pm_runtime_get_sync() and so I guess also the aops
> > > get_power() hook.
> > >
> > > Could someone in your team check if the above can indeed happen?
>
> Through a quick glance, I'm afraid that it's indeed possible. We need
> to off-load either the hdac_hdmi jack handling or this new
> force_audio_cdclk stuff, I suppose.
Ok. We need the force_audio_cdclk thing whenever the audio driver needs
power and calls the aops::get_power() hook. IIUC this happens from various
places in the HDA driver and get_power() should enable power
synchronously, so not sure how we could off-load that..
How about the same for the jack handling, perhaps only in the case it's
done from hdac_hdmi_eld_notify_cb()?
Also could you explain why patch_hdmi.c can do without get_power()?
Could the same thing be done in hdac_hdmi.c?
Thanks,
Imre
>
>
> thanks,
>
> Takashi
>
> > > > +
> > > > + if (ret == -EDEADLK) {
> > > > + drm_atomic_state_clear(state);
> > > > + drm_modeset_backoff(&ctx);
> > > > + goto retry;
> > > > + }
> > > > +
> > > > + WARN_ON(ret);
> > > > +
> > > > + drm_atomic_state_put(state);
> > > > +
> > > > + drm_modeset_drop_locks(&ctx);
> > > > + drm_modeset_acquire_fini(&ctx);
> > > > +}
> > > > +
> > > > static void i915_audio_component_get_power(struct device *kdev)
> > > > {
> > > > - intel_display_power_get(kdev_to_i915(kdev), POWER_DOMAIN_AUDIO);
> > > > + struct drm_i915_private *dev_priv = kdev_to_i915(kdev);
> > > > +
> > > > + dev_priv->get_put_refcount++;
> > > > +
> > > > + /* Force cdclk to 2*BCLK during first time get power call */
> > > > + if (dev_priv->get_put_refcount == 1)
> > > > + if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))
> > > > + glk_force_audio_cdclk(dev_priv, true);
> > > > +
> > > > + intel_display_power_get(dev_priv, POWER_DOMAIN_AUDIO);
> > >
> > > We can't guarantee that the power wells belonging to the audio power
> > > domain are off at this point (since an external output can keep them on).
> > > But according to the tests you did Kumar, this doesn't even seem to be
> > > required, the only requirement is that CDCLK is raised.
> > >
> > > So to make the sequence behave the same all the time (and not hide some
> > > problem happening occasionally) and to have it symmetric with the one
> > > in i915_audio_component_put_power() we should get the audio power domain
> > > before bumping CDCLK.
> > >
> > > > }
> > > >
> > > > static void i915_audio_component_put_power(struct device *kdev)
> > > > {
> > > > - intel_display_power_put(kdev_to_i915(kdev), POWER_DOMAIN_AUDIO);
> > > > + struct drm_i915_private *dev_priv = kdev_to_i915(kdev);
> > > > +
> > > > + dev_priv->get_put_refcount--;
> > > > +
> > > > + /* Force required cdclk during last time put power call */
> > > > + if (dev_priv->get_put_refcount == 0)
> > > > + if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv))
> > > > + glk_force_audio_cdclk(dev_priv, false);
> > > > +
> > > > + intel_display_power_put(dev_priv, POWER_DOMAIN_AUDIO);
> > > > }
> > > >
> > > > static void i915_audio_component_codec_wake_override(struct device *kdev,
> > > > @@ -959,7 +1018,7 @@ void i915_audio_component_init(struct drm_i915_private *dev_priv)
> > > > /* continue with reduced functionality */
> > > > return;
> > > > }
> > > > -
> > > > + dev_priv->get_put_refcount = 0;
> > > > dev_priv->audio_component_registered = true;
> > > > }
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
> > > > index 8ed7bd052e46..0f0aea900ceb 100644
> > > > --- a/drivers/gpu/drm/i915/intel_cdclk.c
> > > > +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> > > > @@ -2153,19 +2153,8 @@ int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state)
> > > > /*
> > > > * According to BSpec, "The CD clock frequency must be at least twice
> > > > * the frequency of the Azalia BCLK." and BCLK is 96 MHz by default.
> > > > - *
> > > > - * FIXME: Check the actual, not default, BCLK being used.
> > > > - *
> > > > - * FIXME: This does not depend on ->has_audio because the higher CDCLK
> > > > - * is required for audio probe, also when there are no audio capable
> > > > - * displays connected at probe time. This leads to unnecessarily high
> > > > - * CDCLK when audio is not required.
> > > > - *
> > > > - * FIXME: This limit is only applied when there are displays connected
> > > > - * at probe time. If we probe without displays, we'll still end up using
> > > > - * the platform minimum CDCLK, failing audio probe.
> > > > */
> > > > - if (INTEL_GEN(dev_priv) >= 9)
> > > > + if (crtc_state->has_audio && INTEL_GEN(dev_priv) >= 9)
> > > > min_cdclk = max(2 * 96000, min_cdclk);
> > > >
> > > > /*
> > > > @@ -2205,7 +2194,7 @@ static int intel_compute_min_cdclk(struct drm_atomic_state *state)
> > > > intel_state->min_cdclk[i] = min_cdclk;
> > > > }
> > > >
> > > > - min_cdclk = 0;
> > > > + min_cdclk = intel_state->cdclk.force_min_cdclk;
> > > > for_each_pipe(dev_priv, pipe)
> > > > min_cdclk = max(intel_state->min_cdclk[pipe], min_cdclk);
> > > >
> > > > @@ -2266,7 +2255,7 @@ static int vlv_modeset_calc_cdclk(struct drm_atomic_state *state)
> > > > vlv_calc_voltage_level(dev_priv, cdclk);
> > > >
> > > > if (!intel_state->active_crtcs) {
> > > > - cdclk = vlv_calc_cdclk(dev_priv, 0);
> > > > + cdclk = vlv_calc_cdclk(dev_priv, intel_state->cdclk.force_min_cdclk);
> > > >
> > > > intel_state->cdclk.actual.cdclk = cdclk;
> > > > intel_state->cdclk.actual.voltage_level =
> > > > @@ -2299,7 +2288,7 @@ static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state)
> > > > bdw_calc_voltage_level(cdclk);
> > > >
> > > > if (!intel_state->active_crtcs) {
> > > > - cdclk = bdw_calc_cdclk(0);
> > > > + cdclk = bdw_calc_cdclk(intel_state->cdclk.force_min_cdclk);
> > > >
> > > > intel_state->cdclk.actual.cdclk = cdclk;
> > > > intel_state->cdclk.actual.voltage_level =
> > > > @@ -2371,7 +2360,7 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
> > > > skl_calc_voltage_level(cdclk);
> > > >
> > > > if (!intel_state->active_crtcs) {
> > > > - cdclk = skl_calc_cdclk(0, vco);
> > > > + cdclk = skl_calc_cdclk(intel_state->cdclk.force_min_cdclk, vco);
> > > >
> > > > intel_state->cdclk.actual.vco = vco;
> > > > intel_state->cdclk.actual.cdclk = cdclk;
> > > > @@ -2410,10 +2399,10 @@ static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)
> > > >
> > > > if (!intel_state->active_crtcs) {
> > > > if (IS_GEMINILAKE(dev_priv)) {
> > > > - cdclk = glk_calc_cdclk(0);
> > > > + cdclk = glk_calc_cdclk(intel_state->cdclk.force_min_cdclk);
> > > > vco = glk_de_pll_vco(dev_priv, cdclk);
> > > > } else {
> > > > - cdclk = bxt_calc_cdclk(0);
> > > > + cdclk = bxt_calc_cdclk(intel_state->cdclk.force_min_cdclk);
> > > > vco = bxt_de_pll_vco(dev_priv, cdclk);
> > > > }
> > > >
> > > > @@ -2449,7 +2438,7 @@ static int cnl_modeset_calc_cdclk(struct drm_atomic_state *state)
> > > > cnl_compute_min_voltage_level(intel_state));
> > > >
> > > > if (!intel_state->active_crtcs) {
> > > > - cdclk = cnl_calc_cdclk(0);
> > > > + cdclk = cnl_calc_cdclk(intel_state->cdclk.force_min_cdclk);
> > > > vco = cnl_cdclk_pll_vco(dev_priv, cdclk);
> > > >
> > > > intel_state->cdclk.actual.vco = vco;
> > > > @@ -2482,7 +2471,7 @@ static int icl_modeset_calc_cdclk(struct drm_atomic_state *state)
> > > > intel_state->cdclk.logical.cdclk = cdclk;
> > > >
> > > > if (!intel_state->active_crtcs) {
> > > > - cdclk = icl_calc_cdclk(0, ref);
> > > > + cdclk = icl_calc_cdclk(intel_state->cdclk.force_min_cdclk, ref);
> > > > vco = icl_calc_cdclk_pll_vco(dev_priv, cdclk);
> > > >
> > > > intel_state->cdclk.actual.vco = vco;
> > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > > index 17c590b42fd7..3ee1c1f5419d 100644
> > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > @@ -12162,6 +12162,10 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
> > > > return -EINVAL;
> > > > }
> > > >
> > > > + /* keep the current setting */
> > > > + if (!intel_state->modeset)
> > >
> > > Ville, I'd use here a dedicated flag instead.
> > >
> > > > + intel_state->cdclk.force_min_cdclk = dev_priv->cdclk.force_min_cdclk;
> > > > +
> > > > intel_state->modeset = true;
> > > > intel_state->active_crtcs = dev_priv->active_crtcs;
> > > > intel_state->cdclk.logical = dev_priv->cdclk.logical;
> > > > @@ -12257,7 +12261,7 @@ static int intel_atomic_check(struct drm_device *dev,
> > > > struct drm_crtc *crtc;
> > > > struct drm_crtc_state *old_crtc_state, *crtc_state;
> > > > int ret, i;
> > > > - bool any_ms = false;
> > > > + bool any_ms = intel_state->modeset;
> > > >
> > > > /* Catch I915_MODE_FLAG_INHERITED */
> > > > for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state,
> > > > @@ -12805,6 +12809,7 @@ static int intel_atomic_commit(struct drm_device *dev,
> > > > dev_priv->active_crtcs = intel_state->active_crtcs;
> > > > dev_priv->cdclk.logical = intel_state->cdclk.logical;
> > > > dev_priv->cdclk.actual = intel_state->cdclk.actual;
> > > > + dev_priv->cdclk.force_min_cdclk = intel_state->cdclk.force_min_cdclk;
> > > > }
> > > >
> > > > drm_atomic_state_get(state);
> > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > > index 8641583842be..0da17ad056ec 100644
> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > @@ -459,6 +459,8 @@ struct intel_atomic_state {
> > > > * state only when all crtc's are DPMS off.
> > > > */
> > > > struct intel_cdclk_state actual;
> > > > +
> > > > + int force_min_cdclk;
> > > > } cdclk;
> > > >
> > > > bool dpll_set, modeset;
> > > > --
> > > > 2.7.4
> > > >
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
