On Fri, Mar 13, 2020 at 04:48:21PM +0200, Kai Vehmanen wrote:
> When the iDisp HDA interface between display and audio is brought
> out from reset, the link parameters must be correctly set before
> reset. This requires the audio driver to call i915 get_power()
> whenever it brings the HDA audio controller from reset. This is
> e.g. done every time audio controller is resumed from runtime
> suspend.
>
> The current solution of modifying min_cdclk in get_power()/put_power()
> can lead to display flicker as events such as playback of UI sounds
> may indirectly cause a modeset change.
>
> As we need to extend the CDCLK>=2*BCLK constraint to more platforms
> beyond GLK, a more robust solution is needed to this problem.
>
> This patch moves modifying the min_cdclk at audio component bind
> phase and extends coverage to all gen9+ platforms. This effectively
> guarantees that whenever audio driver is loaded and bound to i915,
> CDCLK is guaranteed to be such that calls to get_power()/put_power()
> do not result in display artifacts.
So this will now force BXT to never use the 144 MHz CDCLK too. Is that
actually required? I don't remember any reports of failures on BXT.
>
> If 2*BCLK is below lowest CDCLK, this patch has no effect.
>
> If a future platform provides means to change CDCLK without
> a modeset, the constraint code can be moved to get/put_power()
> for these platforms.
>
> Signed-off-by: Kai Vehmanen <kai.vehmanen@xxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/i915/display/intel_audio.c | 20 +++++++++++---------
> 1 file changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_audio.c b/drivers/gpu/drm/i915/display/intel_audio.c
> index e6389b9c2044..4e4832741ecf 100644
> --- a/drivers/gpu/drm/i915/display/intel_audio.c
> +++ b/drivers/gpu/drm/i915/display/intel_audio.c
> @@ -902,10 +902,6 @@ static unsigned long i915_audio_component_get_power(struct device *kdev)
> dev_priv->audio_freq_cntrl);
> }
>
> - /* Force CDCLK to 2*BCLK as long as we need audio powered. */
> - if (IS_GEMINILAKE(dev_priv))
> - glk_force_audio_cdclk(dev_priv, true);
> -
> if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
> intel_de_write(dev_priv, AUD_PIN_BUF_CTL,
> (intel_de_read(dev_priv, AUD_PIN_BUF_CTL) | AUD_PIN_BUF_ENABLE));
> @@ -919,11 +915,7 @@ static void i915_audio_component_put_power(struct device *kdev,
> {
> struct drm_i915_private *dev_priv = kdev_to_i915(kdev);
>
> - /* Stop forcing CDCLK to 2*BCLK if no need for audio to be powered. */
> - if (--dev_priv->audio_power_refcount == 0)
> - if (IS_GEMINILAKE(dev_priv))
> - glk_force_audio_cdclk(dev_priv, false);
> -
> + dev_priv->audio_power_refcount--;
> intel_display_power_put(dev_priv, POWER_DOMAIN_AUDIO, cookie);
> }
>
> @@ -1114,6 +1106,13 @@ static int i915_audio_component_bind(struct device *i915_kdev,
> DL_FLAG_STATELESS)))
> return -ENOMEM;
>
> + /*
> + * To avoid display flicker due to frequent CDCLK changes from
> + * get/put_power(), set up CDCLK>=2*BCLK constraint here.
> + */
> + if (INTEL_GEN(dev_priv) >= 9)
> + glk_force_audio_cdclk(dev_priv, true);
Ah, so we still keep it on the i915 side. That seems fine. We can then
stop doing this once we get nicer hardware and put it back into to
get/put power.
The name of function is rather misleading now though. I guess we should
just s/glk/intel/ since there's nothing actually hardware specific in
there.
> +
> drm_modeset_lock_all(&dev_priv->drm);
> acomp->base.ops = &i915_audio_component_ops;
> acomp->base.dev = i915_kdev;
> @@ -1132,6 +1131,9 @@ static void i915_audio_component_unbind(struct device *i915_kdev,
> struct i915_audio_component *acomp = data;
> struct drm_i915_private *dev_priv = kdev_to_i915(i915_kdev);
>
> + if (INTEL_GEN(dev_priv) >= 9)
> + glk_force_audio_cdclk(dev_priv, false);
> +
> drm_modeset_lock_all(&dev_priv->drm);
> acomp->base.ops = NULL;
> acomp->base.dev = NULL;
> --
> 2.17.1
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
