Re: [RESEND PATCH v2] drm/bridge: dw-hdmi-i2s: set insert_pcuv bit if hardware supports it

[Hugh Cole-Baker <sigmaris@gmail.com>]

Hi Geraldo, and apologies for resurrecting a 2 year old thread...

On 17/10/2022 13:04, Geraldo Nascimento wrote:
> Hi Mark, resending this as it failed to apply in my last submission. Added
> Neil Armstrong to Cc: as hopefully he will be able to better review this.
> 
> Thanks,
> Geraldo Nascimento
> 
> ---
> 
> Starting with version 2.10a of Synopsys DesignWare HDMI controller the
> insert_pcuv bit was introduced. On RK3399pro SoM (Radxa Rock Pi N10),
> for example, if we neglect to set this bit and proceed to enable hdmi_sound
> and i2s2 on the device tree there will be extreme clipping of sound
> output, to the point that music sounds like white noise. Problem
> could also manifest as just mild cracking depending of HDMI audio
> implementation of sink. Setting insert_pcuv bit (bit 2 of
> aud_conf2 Audio Sample register) fixes this.
> 
> Signed-off-by: Geraldo Nascimento <geraldogabriel@gmail.com>

I also had the HDMI audio clipping issue described here, on a RK3399. This was
on a 6.1.23 kernel based on the one used by LibreELEC.tv with their out-of-tree
patches for video decoding, 4k HDMI support, etc. When testing this patch I
also updated my kernel tree to 6.10.3, and found that even without this patch,
on 6.10.3 the problem no longer happens.

I added printk to show the value of AUD_CONF2, and found that on 6.1.23, the
value is 0 before the code in this patch sets the insert_pcuv bit. On 6.10.3
the value is 4, i.e. insert_pcuv is already set.

According to the RK3399 TRM, the value-after-reset of the insert_pcuv bit is 1,
so apparently on the 6.1.23 kernel something is clearing the bit after HW reset
but before this driver sets the hw_params, and this patch sets it back to the
correct value. On 6.10.3 the bit is not cleared, i.e. this patch is seemingly
no longer necessary (but is a harmless no-op).

> 
> ---
> 
> v1->v2: SoC->SoM on description, better commenting, minor style changes,
> 	conditional application of fix for L-PCM only
> 
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio-20221017.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio-20221017.c
> @@ -42,6 +42,7 @@ static int dw_hdmi_i2s_hw_params(struct device *dev, void *data,
>  	struct dw_hdmi *hdmi = audio->hdmi;
>  	u8 conf0 = 0;
>  	u8 conf1 = 0;
> +	u8 conf2 = 0;
>  	u8 inputclkfs = 0;
>  
>  	/* it cares I2S only */
> @@ -101,6 +102,28 @@ static int dw_hdmi_i2s_hw_params(struct device *dev, void *data,
>  		return -EINVAL;
>  	}
>  
> +	/*
> +	 * dw-hdmi introduced insert_pcuv bit in
> +	 * version 2.10a.
> +	 *
> +	 * This single bit (bit 2 of HDMI_AUD_CONF2)
> +	 * when set to 1 will enable the insertion of the PCUV
> +	 * (Parity, Channel Status, User bit and Validity)
> +	 * bits on the incoming audio stream.
> +	 * 
> +	 * Support is limited to Linear PCM audio. If
> +	 * neglected, the lack of valid PCUV bits
> +	 * on L-PCM streams will cause anything from
> +	 * mild cracking to full blown extreme
> +	 * clipping depending on the HDMI audio
> +	 * implementation of the sink.
> +	 *
> +	 */
> +
> +	if (hdmi_read(audio, HDMI_DESIGN_ID) >= 0x21 &&
> +			!(hparms->iec.status[0] & IEC958_AES0_NONAUDIO))
> +		conf2 = HDMI_AUD_CONF2_INSERT_PCUV;
> +
>  	dw_hdmi_set_sample_rate(hdmi, hparms->sample_rate);
>  	dw_hdmi_set_channel_status(hdmi, hparms->iec.status);
>  	dw_hdmi_set_channel_count(hdmi, hparms->channels);
> @@ -109,6 +120,7 @@ static int dw_hdmi_i2s_hw_params(struct device *dev, void *data,
>  	hdmi_write(audio, inputclkfs, HDMI_AUD_INPUTCLKFS);
>  	hdmi_write(audio, conf0, HDMI_AUD_CONF0);
>  	hdmi_write(audio, conf1, HDMI_AUD_CONF1);
> +	hdmi_write(audio, conf2, HDMI_AUD_CONF2);
>  
>  	return 0;
>  }
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-20221017.h
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-20221017.h

btw, this patch doesn't apply without edits as these filenames are incorrect.

> @@ -931,6 +931,11 @@ enum {
>  	HDMI_AUD_CONF1_WIDTH_16 = 0x10,
>  	HDMI_AUD_CONF1_WIDTH_24 = 0x18,
>  
> +/* AUD_CONF2 field values */
> +	HDMI_AUD_CONF2_HBR = 0x01,
> +	HDMI_AUD_CONF2_NLPCM = 0x02,
> +	HDMI_AUD_CONF2_INSERT_PCUV = 0x04,
> +
>  /* AUD_CTS3 field values */
>  	HDMI_AUD_CTS3_N_SHIFT_OFFSET = 5,
>  	HDMI_AUD_CTS3_N_SHIFT_MASK = 0xe0,

Best regards,
Hugh