Re: [v3] drm/i915: Implement Audio WA_14020863754

[Jani Nikula <jani.nikula@linux.intel.com>]

On Mon, 06 May 2024, Uma Shankar <uma.shankar@intel.com> wrote:
> WA_14020863754: Corner case with Min Hblank Fix can cause
> audio hang
>
> Issue: Previously a fix was made to avoid issues with extremely
> small hblanks, called the "Min Hblank Fix". However, this can
> potentially cause an audio hang.
>
> Workaround :
> During "Audio Programming Sequence" Audio Enabling -
> When DP mode is enabled Set mmio offset 0x65F1C bit 18 = 1b,
> before step #1 "Enable audio Presence Detect"
>
> During "Audio Programming Sequence" Audio Disabling -
> When DP mode is enabled Clear mmio offset 0x65F1C bit 18 = 0b,
> after step #6 "Disable Audio PD (Presence Detect)"
> If not clearing PD bit, must also not clear 0x65F1C bit 18 (leave = 1b)
>
> v2: Update the platform checks (Jani Nikula)
>
> v3: Limited the WA to LNL and BMG, added a helper (Matt Roper)
>
> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_audio.c     | 18 ++++++++++++++++++
>  .../gpu/drm/i915/display/intel_audio_regs.h    |  3 +++
>  2 files changed, 21 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_audio.c b/drivers/gpu/drm/i915/display/intel_audio.c
> index ed81e1466c4b..cb055c16dd98 100644
> --- a/drivers/gpu/drm/i915/display/intel_audio.c
> +++ b/drivers/gpu/drm/i915/display/intel_audio.c
> @@ -183,6 +183,18 @@ static const struct hdmi_aud_ncts hdmi_aud_ncts_36bpp[] = {
>  	{ 192000, TMDS_445_5M, 20480, 371250 },
>  };
>  
> +/*
> + * WA_14020863754: Implement Audio Workaround
> + * Corner case with Min Hblank Fix can cause audio hang
> + */
> +static bool needs_wa_14020863754(struct drm_i915_private *i915)
> +{
> +	if (DISPLAY_VER(i915) == 20 || IS_BATTLEMAGE(i915))

Why do we need to check for both display version and the platform?
Especially weird that it's || and not &&.

> +		return true;
> +
> +	return false;

The whole function is just "return ver == 20 || is_bmg", there's no need
to have the if and two different return locations.

BR,
Jani.


> +}
> +
>  /* get AUD_CONFIG_PIXEL_CLOCK_HDMI_* value for mode */
>  static u32 audio_config_hdmi_pixel_clock(const struct intel_crtc_state *crtc_state)
>  {
> @@ -415,6 +427,9 @@ static void hsw_audio_codec_disable(struct intel_encoder *encoder,
>  	intel_de_rmw(i915, HSW_AUD_PIN_ELD_CP_VLD,
>  		     AUDIO_OUTPUT_ENABLE(cpu_transcoder), 0);
>  
> +	if (needs_wa_14020863754(i915))
> +		intel_de_rmw(i915, AUD_CHICKENBIT_REG3, CHICKEN3_SPARE18, 0);
> +
>  	mutex_unlock(&i915->display.audio.mutex);
>  }
>  
> @@ -540,6 +555,9 @@ static void hsw_audio_codec_enable(struct intel_encoder *encoder,
>  	if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP))
>  		enable_audio_dsc_wa(encoder, crtc_state);
>  
> +	if (needs_wa_14020863754(i915))
> +		intel_de_rmw(i915, AUD_CHICKENBIT_REG3, 0, CHICKEN3_SPARE18);
> +
>  	/* Enable audio presence detect */
>  	intel_de_rmw(i915, HSW_AUD_PIN_ELD_CP_VLD,
>  		     0, AUDIO_OUTPUT_ENABLE(cpu_transcoder));
> diff --git a/drivers/gpu/drm/i915/display/intel_audio_regs.h b/drivers/gpu/drm/i915/display/intel_audio_regs.h
> index 88ea2740365d..7de82cd3380e 100644
> --- a/drivers/gpu/drm/i915/display/intel_audio_regs.h
> +++ b/drivers/gpu/drm/i915/display/intel_audio_regs.h
> @@ -164,4 +164,7 @@
>  							 _VLV_AUD_PORT_EN_D_DBG)
>  #define VLV_AMP_MUTE		        (1 << 1)
>  
> +#define AUD_CHICKENBIT_REG3		_MMIO(0x65F1C)
> +#define  CHICKEN3_SPARE18		REG_BIT(18)
> +
>  #endif /* __INTEL_AUDIO_REGS_H__ */

-- 
Jani Nikula, Intel