Re: [PATCH v5 1/3] drm/bridge: synopsys: Add audio support for dw-hdmi-qp

[Detlev Casanova <detlev.casanova@collabora.com>]

Hi Quentin,

On Tuesday, 4 February 2025 04:59:12 EST Quentin Schulz wrote:
> Hi Detlev,
> 
> Just some drive-by comment inline.
> 
> On 2/3/25 6:16 PM, Detlev Casanova wrote:
> > From: Sugar Zhang <sugar.zhang@rock-chips.com>
> > 
> > Register the dw-hdmi-qp bridge driver as an HDMI audio codec.
> > 
> > The register values computation functions (for n) are based on the
> > downstream driver, as well as the register writing functions.
> > 
> > The driver uses the generic HDMI Codec framework in order to implement
> > the HDMI audio support.
> > 
> > Signed-off-by: Sugar Zhang <sugar.zhang@rock-chips.com>
> > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > Tested-by: Quentin Schulz <quentin.schulz@cherry.de>
> > Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
> > ---
> > 
> >   drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c | 497 +++++++++++++++++++
> >   1 file changed, 497 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
> > b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c index
> > b281cabfe992e..ea5d8599cb56a 100644
> > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
> > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
> 
> [...]
> 
> > +static int dw_hdmi_qp_match_tmds_n_table(struct dw_hdmi_qp *hdmi,
> > +				   unsigned long pixel_clk,
> > +				   unsigned long freq)
> > +{
> > +	const struct dw_hdmi_audio_tmds_n *tmds_n = NULL;
> > +	int i;
> > +
> > +	for (i = 0; common_tmds_n_table[i].tmds != 0; i++) {
> > +		if (pixel_clk == common_tmds_n_table[i].tmds) {
> > +			tmds_n = &common_tmds_n_table[i];
> > +			break;
> > +		}
> > +	}
> > +
> > +	if (tmds_n == NULL)
> > +		return -ENOENT;
> > +
> > +	switch (freq) {
> > +	case 32000:
> > +		return tmds_n->n_32k;
> > +	case 44100:
> > +	case 88200:
> > +	case 176400:
> > +		return (freq / 44100) * tmds_n->n_44k1;
> > +	case 48000:
> > +	case 96000:
> > +	case 192000:
> > +		return (freq / 48000) * tmds_n->n_48k;
> > +	default:
> > +		return -ENOENT;
> > +	}
> > +}
> > +
> > +static u64 dw_hdmi_qp_audio_math_diff(unsigned int freq, unsigned int n,
> > +				unsigned int pixel_clk)
> > +{
> > +	u64 final, diff;
> > +	u64 cts;
> > +
> > +	final = (u64)pixel_clk * n;
> > +
> > +	cts = final;
> > +	do_div(cts, 128 * freq);
> > +
> > +	diff = final - (u64)cts * (128 * freq);
> > +
> > +	return diff;
> 
> I believe
> 
> final = (u64)pixel_clk * n;
> do_div(cts, 128 * freq);
> diff = final - (u64)cts * (128 * freq);
> 
> is just an elaborate way of doing
> 
> ((u64)pixel_clk * n) % (128 * freq)
> 
> ? If that's the case, then I believe do_div(a, b) actually already
> returns the remainder of a by b.
> 
> If that's the case, isn't that function simply:
> 
> return do_div(mul_u32_u32(pixel_clk, n), mul_u32_u32(freq, 128))
> 
> ?
> 
> We could probably replace mul_u32_u32(freq, 128) with freq * 128
> considering that dw_hdmi_qp_match_tmds_n_table expects freq to be <
> 192000, so we won't get an overflow with that.
> 
> mul_u32_u32(pixel_clk, n) does the u32*u32->u64 without a temporary C
> variable.

I tested this and it is indeed doing the same. I'll change it and also change 
the function return type to u32 as that is what do_div() returns.
That makes sense as the reminder of a/b can't be wider than the size of b.

We just need one variable as the first argument of do_div() is used to store 
the integer division result.

> [...]
> 
> > +static void dw_hdmi_qp_set_audio_interface(struct dw_hdmi_qp *hdmi,
> > +					   struct hdmi_codec_daifmt 
*fmt,
> > +					   struct hdmi_codec_params 
*hparms)
> > +{
> > +	u32 conf0 = 0;
> > +
> > +	/* Reset the audio data path of the AVP */
> > +	dw_hdmi_qp_write(hdmi, AVP_DATAPATH_PACKET_AUDIO_SWINIT_P,
> > GLOBAL_SWRESET_REQUEST); +
> > +	/* Disable AUDS, ACR, AUDI */
> > +	dw_hdmi_qp_mod(hdmi, 0,
> > +		  PKTSCHED_ACR_TX_EN | PKTSCHED_AUDS_TX_EN | 
PKTSCHED_AUDI_TX_EN,
> > +		  PKTSCHED_PKT_EN);
> > +
> > +	/* Clear the audio FIFO */
> > +	dw_hdmi_qp_write(hdmi, AUDIO_FIFO_CLR_P, AUDIO_INTERFACE_CONTROL0);
> > +
> > +	/* Select I2S interface as the audio source */
> > +	dw_hdmi_qp_mod(hdmi, AUD_IF_I2S, AUD_IF_SEL_MSK,
> > AUDIO_INTERFACE_CONFIG0); +
> > +	/* Enable the active i2s lanes */
> > +	switch (hparms->channels) {
> > +	case 7 ... 8:
> > +		conf0 |= I2S_LINES_EN(3);
> > +		fallthrough;
> > +	case 5 ... 6:
> > +		conf0 |= I2S_LINES_EN(2);
> > +		fallthrough;
> > +	case 3 ... 4:
> > +		conf0 |= I2S_LINES_EN(1);
> > +		fallthrough;
> > +	default:
> > +		conf0 |= I2S_LINES_EN(0);
> > +		break;
> > +	}
> > +
> > +	dw_hdmi_qp_mod(hdmi, conf0, I2S_LINES_EN_MSK, 
AUDIO_INTERFACE_CONFIG0);
> > +
> > +	/*
> > +	 * Enable bpcuv generated internally for L-PCM, or received
> > +	 * from stream for NLPCM/HBR.
> > +	 */
> > +	switch (fmt->bit_fmt) {
> > +	case SNDRV_PCM_FORMAT_IEC958_SUBFRAME_LE:
> > +		conf0 = (hparms->channels == 8) ? AUD_HBR : AUD_ASP;
> > +		conf0 |= I2S_BPCUV_RCV_EN;
> > +		break;
> > +	default:
> > +		conf0 = AUD_ASP | I2S_BPCUV_RCV_DIS;
> > +		break;
> > +	}
> > +
> > +	dw_hdmi_qp_mod(hdmi, conf0, I2S_BPCUV_RCV_MSK | AUD_FORMAT_MSK,
> > +		  AUDIO_INTERFACE_CONFIG0);
> > +
> > +	/* Enable audio FIFO auto clear when overflow */
> > +	dw_hdmi_qp_mod(hdmi, AUD_FIFO_INIT_ON_OVF_EN, 
AUD_FIFO_INIT_ON_OVF_MSK,
> > +		  AUDIO_INTERFACE_CONFIG0);
> 
> This is all very I2S-centric while the HDMI controllers on RK3588 do
> have the ability (according to the TRM) to use S/PDIF instead of I2S. I
> assume the driver should be able to know which format to use based on
> simple-audio-card,format property? Is that correct? Then current support
> which doesn't even check for I2S would be fine and not conflict with a
> later commit which would add support for S/PDIF? (Essentially asking if
> we need another DT property for the HDMI controller node or elsewhere to
> specify which mode to run in instead of expecting it to always be I2S).

The hdmi_codec_daifmt::fmt field already has this information, based on the 
simple-audio-card,format = "i2s"; field in the device tree.

I could add a condition in dw_hdmi_qp_audio_prepare() to fail with -EINVAL if 
the devicetree specifies anything else than "i2s" for now.

I'm not willing to implement support for the SPDIF path for now, mainly 
because there's no need for that yet (I2S works well) and the downstream 
kernel doesn't implemented it, meaning it hasn't been tested a lot anyway.

Regards,
Detlev.