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

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

[Geraldo Nascimento]

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>

---

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
@@ -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,

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

[Neil Armstrong]

Hi,


On 17/10/2022 14: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.


I did some research and this insert_pcuv is already present in the 1.40a version
of the dw-hdmi databook, so I wonder why suddenly this is needed.

The insert_pcuv is documented as:
-------------------------------------------------------
When set (1'b1), it enables the insertion of the PCUV (Parity, Channel Status, User
bit and Validity) bits on the incoming audio stream (support limited to Linear PCM
audio).
If disabled, the incomming audio stream must contain the PCUV bits, mapped
acording to 2.6.4.2 Data Mapping Examples
--------------------------------------------------------


What's interesting is this register is only present if thre DW-HDMI IP is configured
as GPAUD or GDOUBLE, meaning it must have GPAUD enabled. So it has
something to do with it, so what's value of it when GPAUD isn't present in the IP ?

And HDMI2 spec added this, even PCVU were required before:
--------------------------------------------------------
Note that PCUV refers to the parity bit (P), channel status (C), user data (U), and validity bit (V) as defined in IEC
60958-1.
--------------------------------------------------------

So it has something to do with IEC60958-1 stream format, do maybe this
insert_pcuv should only be enforced when the input stream is _not_ IEC60958-1 ?

Neil

> 
> Signed-off-by: Geraldo Nascimento <geraldogabriel@gmail.com>
> 
> ---
> 
> 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
> @@ -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,

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

[Geraldo Nascimento]

On Mon, Oct 31, 2022 at 09:20:36AM +0100, Neil Armstrong wrote:
> Hi,
>

Hi Neil and thanks for caring.

> 
> On 17/10/2022 14: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.
> 
> 
> I did some research and this insert_pcuv is already present in the 1.40a version
> of the dw-hdmi databook, so I wonder why suddenly this is needed.

Like I told you, I was unaware of this fact, but I have a hunch this bit
was being set to 1 as default before the 2.10a version of dw-hdmi. It
remains to be checked, I see you added Christian Hewitt to the Cc:,
maybe he can use i2cdump or printk() on older Rockchip  hardware, the
Radxa Rock Pi N10 is the only thing from Rockchip I own.

> 
> The insert_pcuv is documented as:
> -------------------------------------------------------
> When set (1'b1), it enables the insertion of the PCUV (Parity, Channel Status, User
> bit and Validity) bits on the incoming audio stream (support limited to Linear PCM
> audio).
> If disabled, the incomming audio stream must contain the PCUV bits, mapped
> acording to 2.6.4.2 Data Mapping Examples
> --------------------------------------------------------
> 
> 
> What's interesting is this register is only present if thre DW-HDMI IP is configured
> as GPAUD or GDOUBLE, meaning it must have GPAUD enabled. So it has
> something to do with it, so what's value of it when GPAUD isn't present in the IP ?

Would you like me to inject some printk() or dump some memory value
with i2cdump? I'm not sure I follow you because you obviously know much
more about this driver than me, but if you could elaborate a bit I'd be
happy to provide some answers.

> 
> And HDMI2 spec added this, even PCVU were required before:
> --------------------------------------------------------
> Note that PCUV refers to the parity bit (P), channel status (C), user data (U), and validity bit (V) as defined in IEC
> 60958-1.
> --------------------------------------------------------
> 
> So it has something to do with IEC60958-1 stream format, do maybe this
> insert_pcuv should only be enforced when the input stream is _not_ IEC60958-1 ?

Yes, the HDMI2 spec requires PCUV audio bits, and they borrow the idea
from IEC 60958-1 (Digital Audio Interface - DAI), but insert_pcuv definitely
needs to be set for newer Synopsys Designware HDMI hardware when we're
talking about Linear PCM - that's what my patch attempts to address.

Thanks,
Geraldo Nascimento

> 
> Neil
> 
> > 
> > Signed-off-by: Geraldo Nascimento <geraldogabriel@gmail.com>
> > 
> > ---
> > 
> > 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
> > @@ -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,
> 

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

[Hugh Cole-Baker]

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

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

[Geraldo Nascimento]

On Fri, Sep 13, 2024 at 10:12:39PM +0100, Hugh Cole-Baker wrote:
> 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).

Hi Hugh,

Thank you for your extensive testing. It seems then there's no action we
need to take for mainline, as it's already fixed there.

> > --- 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.
> 

Yeah, I see. My bad.

Thank you,
Geraldo Nascimento

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

[Geraldo Nascimento]

On Fri, Sep 13, 2024 at 06:28:34PM -0300, Geraldo Nascimento wrote:
> On Fri, Sep 13, 2024 at 10:12:39PM +0100, Hugh Cole-Baker wrote:
> > 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).
> 
> Hi Hugh,
> 
> Thank you for your extensive testing. It seems then there's no action we
> need to take for mainline, as it's already fixed there.

Unless Neil wants to pick-up it up for Stable? Neil, although not a
regression, this is definitely a show-stopper for sound on RK3399
for older, still supported kernels. And thanks to Hugh detailed
report we now have confirmation that this happens on vanilla
RK3399 and is not a quirk of my Rock Pi N10 board or sink.

Thanks,
Geraldo Nascimento

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

[Geraldo Nascimento]

On Fri, Sep 13, 2024 at 10:12:39PM +0100, Hugh Cole-Baker wrote:
> 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).
>

I found out that for my hardware even with 6.13-rc5 I still suffer from
really distorted audio if I don't apply this patch.

>From 2cd8a7432afa9a02b4498832c912b8f90e347a5d Mon Sep 17 00:00:00 2001
From: Geraldo Nascimento <geraldogabriel@gmail.com>
Date: Sun, 29 Dec 2024 23:26:47 -0300
Subject: [PATCH 7/7] drm/bridge/synopsys: dw-hdmi: enforce PCUV bits for newer
 versions

For some strange reason my hardware boots without this set to
the default, testing has shown mixed results with different
kernel versions on different hardware, truth is, I need
this patch or more compliant sinks will misbehave when the
PCUV bits are not set

Signed-off-by: Geraldo Nascimento <geraldogabriel@gmail.com>
---
 drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c | 13 +++++++++++++
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.h           |  5 +++++
 2 files changed, 18 insertions(+)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
index f1c5a8d0fa90..fc45d10c7d42 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-i2s-audio.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,17 @@ static int dw_hdmi_i2s_hw_params(struct device *dev, void *data,
 		return -EINVAL;
 	}
 
+	/*
+	 * dw-hdmi introduced insert_pcuv bit in version 2.10a.
+	 * When set (1'b1), this bit enables the insertion of the PCUV
+	 * (Parity, Channel Status, User bit and Validity) bits on the
+	 * incoming audio stream (support limited to Linear PCM audio)
+	 */
+	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 +121,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;
 }
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
index af43a0414b78..d7cbdc42d501 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
@@ -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 = 0x1,
+	HDMI_AUD_CONF2_NLPCM = 0x2,
+	HDMI_AUD_CONF2_INSERT_PCUV = 0x04,
+
 /* AUD_CTS3 field values */
 	HDMI_AUD_CTS3_N_SHIFT_OFFSET = 5,
 	HDMI_AUD_CTS3_N_SHIFT_MASK = 0xe0,
-- 
2.47.1