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

[Geraldo Nascimento <geraldogabriel@gmail.com>]

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