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