Re: [Intel-gfx] [PATCH 2/2] drm/i915: move audio CDCLK constraint setup to bind/unbind

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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@linux.intel.com>
> ---
>  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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx