[PATCH 0/4] drm/display: hdmi: provide common code to get Audio Clock Recovery params

[PATCH 0/4] drm/display: hdmi: provide common code to get Audio Clock Recovery params

[Dmitry Baryshkov]

HDMI standards define a recommended set of values to be used for Audio
Clock Regeneration. Nevertheless, each HDMI driver dealing with audio
implements its own way to determine those values. Implement a common
helper and use it for MSM HDMI (tested), VC4 and DW-HDMI (compile-tested
only) drivers.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
Dmitry Baryshkov (4):
      drm/display: hdmi: provide central data authority for ACR params
      drm/msm/hdmi: use new helper for ACR tables
      drm/vc4: use new helper to get ACR values
      drm: bridge: dw-hdmi: use new helper to get ACR values

 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c |  90 ++--------------
 drivers/gpu/drm/display/drm_hdmi_helper.c | 164 ++++++++++++++++++++++++++++++
 drivers/gpu/drm/msm/hdmi/hdmi_audio.c     | 107 ++-----------------
 drivers/gpu/drm/vc4/vc4_hdmi.c            |  10 +-
 drivers/gpu/drm/vc4/vc4_hdmi.h            |   7 ++
 include/drm/display/drm_hdmi_helper.h     |   6 ++
 6 files changed, 197 insertions(+), 187 deletions(-)
---
base-commit: 0a2f889128969dab41861b6e40111aa03dc57014
change-id: 20250308-drm-hdmi-acr-7ad1f0d012df

Best regards,
-- 
Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

[PATCH 1/4] drm/display: hdmi: provide central data authority for ACR params

[Dmitry Baryshkov]

From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

HDMI standard defines recommended N and CTS values for Audio Clock
Regeneration. Currently each driver implements those, frequently in
somewhat unique way. Provide a generic helper for getting those values
to be used by the HDMI drivers.

The helper is added to drm_hdmi_helper.c rather than drm_hdmi_audio.c
since HDMI drivers can be using this helper function even without
switching to DRM HDMI Audio helpers.

Note: currently this only handles the values per HDMI 1.4b Section 7.2
and HDMI 2.0 Section 9.2.1. Later the table can be expanded to
accommodate for Deep Color TMDS char rates per HDMI 1.4 Appendix D
and/or HDMI 2.0 / 2.1 Appendix C).

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/display/drm_hdmi_helper.c | 164 ++++++++++++++++++++++++++++++
 include/drm/display/drm_hdmi_helper.h     |   6 ++
 2 files changed, 170 insertions(+)

diff --git a/drivers/gpu/drm/display/drm_hdmi_helper.c b/drivers/gpu/drm/display/drm_hdmi_helper.c
index 74dd4d01dd9bb2c9e69ec1c60b0056bd69417e8a..89d25571bfd21c56c6835821d2272a12c816a76e 100644
--- a/drivers/gpu/drm/display/drm_hdmi_helper.c
+++ b/drivers/gpu/drm/display/drm_hdmi_helper.c
@@ -256,3 +256,167 @@ drm_hdmi_compute_mode_clock(const struct drm_display_mode *mode,
 	return DIV_ROUND_CLOSEST_ULL(clock * bpc, 8);
 }
 EXPORT_SYMBOL(drm_hdmi_compute_mode_clock);
+
+struct drm_hdmi_acr_n_cts_entry {
+	unsigned int n;
+	unsigned int cts;
+};
+
+struct drm_hdmi_acr_data {
+	unsigned long tmds_clock_khz;
+	struct drm_hdmi_acr_n_cts_entry n_cts_32k,
+					n_cts_44k1,
+					n_cts_48k;
+};
+
+static const struct drm_hdmi_acr_data hdmi_acr_n_cts[] = {
+	{
+		/* "Other" entry */
+		.n_cts_32k =  { .n = 4096, },
+		.n_cts_44k1 = { .n = 6272, },
+		.n_cts_48k =  { .n = 6144, },
+	}, {
+		.tmds_clock_khz = 25175,
+		.n_cts_32k =  { .n = 4576,  .cts = 28125, },
+		.n_cts_44k1 = { .n = 7007,  .cts = 31250, },
+		.n_cts_48k =  { .n = 6864,  .cts = 28125, },
+	}, {
+		.tmds_clock_khz = 25200,
+		.n_cts_32k =  { .n = 4096,  .cts = 25200, },
+		.n_cts_44k1 = { .n = 6272,  .cts = 28000, },
+		.n_cts_48k =  { .n = 6144,  .cts = 25200, },
+	}, {
+		.tmds_clock_khz = 27000,
+		.n_cts_32k =  { .n = 4096,  .cts = 27000, },
+		.n_cts_44k1 = { .n = 6272,  .cts = 30000, },
+		.n_cts_48k =  { .n = 6144,  .cts = 27000, },
+	}, {
+		.tmds_clock_khz = 27027,
+		.n_cts_32k =  { .n = 4096,  .cts = 27027, },
+		.n_cts_44k1 = { .n = 6272,  .cts = 30030, },
+		.n_cts_48k =  { .n = 6144,  .cts = 27027, },
+	}, {
+		.tmds_clock_khz = 54000,
+		.n_cts_32k =  { .n = 4096,  .cts = 54000, },
+		.n_cts_44k1 = { .n = 6272,  .cts = 60000, },
+		.n_cts_48k =  { .n = 6144,  .cts = 54000, },
+	}, {
+		.tmds_clock_khz = 54054,
+		.n_cts_32k =  { .n = 4096,  .cts = 54054, },
+		.n_cts_44k1 = { .n = 6272,  .cts = 60060, },
+		.n_cts_48k =  { .n = 6144,  .cts = 54054, },
+	}, {
+		.tmds_clock_khz = 74176,
+		.n_cts_32k =  { .n = 11648, .cts = 210937, }, /* and 210938 */
+		.n_cts_44k1 = { .n = 17836, .cts = 234375, },
+		.n_cts_48k =  { .n = 11648, .cts = 140625, },
+	}, {
+		.tmds_clock_khz = 74250,
+		.n_cts_32k =  { .n = 4096,  .cts = 74250, },
+		.n_cts_44k1 = { .n = 6272,  .cts = 82500, },
+		.n_cts_48k =  { .n = 6144,  .cts = 74250, },
+	}, {
+		.tmds_clock_khz = 148352,
+		.n_cts_32k =  { .n = 11648, .cts = 421875, },
+		.n_cts_44k1 = { .n = 8918,  .cts = 234375, },
+		.n_cts_48k =  { .n = 5824,  .cts = 140625, },
+	}, {
+		.tmds_clock_khz = 148500,
+		.n_cts_32k =  { .n = 4096,  .cts = 148500, },
+		.n_cts_44k1 = { .n = 6272,  .cts = 165000, },
+		.n_cts_48k =  { .n = 6144,  .cts = 148500, },
+	}, {
+		.tmds_clock_khz = 296703,
+		.n_cts_32k =  { .n = 5824,  .cts = 421875, },
+		.n_cts_44k1 = { .n = 4459,  .cts = 234375, },
+		.n_cts_48k =  { .n = 5824,  .cts = 281250, },
+	}, {
+		.tmds_clock_khz = 297000,
+		.n_cts_32k =  { .n = 3072,  .cts = 222750, },
+		.n_cts_44k1 = { .n = 4704,  .cts = 247500, },
+		.n_cts_48k =  { .n = 5120,  .cts = 247500, },
+	}, {
+		.tmds_clock_khz = 593407,
+		.n_cts_32k =  { .n = 5824,  .cts = 843750, },
+		.n_cts_44k1 = { .n = 8918,  .cts = 937500, },
+		.n_cts_48k =  { .n = 5824,  .cts = 562500, },
+	}, {
+		.tmds_clock_khz = 594000,
+		.n_cts_32k =  { .n = 3072,  .cts = 445500, },
+		.n_cts_44k1 = { .n = 9408,  .cts = 990000, },
+		.n_cts_48k =  { .n = 6144,  .cts = 594000, },
+	},
+};
+
+static int drm_hdmi_acr_find_tmds_entry(unsigned long tmds_clock_khz)
+{
+	int i;
+
+	/* skip the "other" entry */
+	for (i = 1; i < ARRAY_SIZE(hdmi_acr_n_cts); i++) {
+		if (hdmi_acr_n_cts[i].tmds_clock_khz == tmds_clock_khz)
+			return i;
+	}
+
+	return 0;
+}
+
+/**
+ * drm_hdmi_acr_get_n_cts() - get N and CTS values for Audio Clock Regeneration
+ *
+ * @tmds_char_rate: TMDS clock (char rate) as used by the HDMI connector
+ * @sample_rate: audio sample rate
+ * @out_n: a pointer to write the N value
+ * @out_cts: a pointer to write the CTS value
+ *
+ * Get the N and CTS values (either by calculating them or by returning data
+ * from the tables. This follows the HDMI 1.4b Section 7.2 "Audio Sample Clock
+ * Capture and Regeneration".
+ */
+void
+drm_hdmi_acr_get_n_cts(unsigned long long tmds_char_rate,
+		       unsigned int sample_rate,
+		       unsigned int *out_n,
+		       unsigned int *out_cts)
+{
+	/* be a bit more tolerant, especially for the 1.001 entries */
+	unsigned long tmds_clock_khz = DIV_ROUND_CLOSEST_ULL(tmds_char_rate, 1000);
+	const struct drm_hdmi_acr_n_cts_entry *entry;
+	unsigned int n, cts, mult;
+	int tmds_idx;
+
+	tmds_idx = drm_hdmi_acr_find_tmds_entry(tmds_clock_khz);
+
+	/*
+	 * Don't change the order, 192 kHz is divisible by 48k and 32k, but it
+	 * should use 48k entry.
+	 */
+	if (sample_rate % 48000 == 0) {
+		entry = &hdmi_acr_n_cts[tmds_idx].n_cts_48k;
+		mult = sample_rate / 48000;
+	} else if (sample_rate % 44100 == 0) {
+		entry = &hdmi_acr_n_cts[tmds_idx].n_cts_44k1;
+		mult = sample_rate / 44100;
+	} else if (sample_rate % 32000 == 0) {
+		entry = &hdmi_acr_n_cts[tmds_idx].n_cts_32k;
+		mult = sample_rate / 32000;
+	} else {
+		entry = NULL;
+	}
+
+	if (entry) {
+		n = entry->n * mult;
+		cts = entry->cts;
+	} else {
+		/* Recommended optimal value, HDMI 1.4b, Section 7.2.1 */
+		n = 128 * sample_rate / 1000;
+		cts = 0;
+	}
+
+	if (!cts)
+		cts = DIV_ROUND_CLOSEST_ULL(tmds_char_rate * n,
+					    128 * sample_rate);
+
+	*out_n = n;
+	*out_cts = cts;
+}
diff --git a/include/drm/display/drm_hdmi_helper.h b/include/drm/display/drm_hdmi_helper.h
index 57e3b18c15ec79636d89267aba0e88f434c5d4db..09145c9ee9fc0cd839242f2373b305940e06e157 100644
--- a/include/drm/display/drm_hdmi_helper.h
+++ b/include/drm/display/drm_hdmi_helper.h
@@ -28,4 +28,10 @@ unsigned long long
 drm_hdmi_compute_mode_clock(const struct drm_display_mode *mode,
 			    unsigned int bpc, enum hdmi_colorspace fmt);
 
+void
+drm_hdmi_acr_get_n_cts(unsigned long long tmds_char_rate,
+		       unsigned int sample_rate,
+		       unsigned int *out_n,
+		       unsigned int *out_cts);
+
 #endif

-- 
2.39.5

Re: [PATCH 1/4] drm/display: hdmi: provide central data authority for ACR params

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 7615 bytes --]

On Sun, Mar 09, 2025 at 10:13:56AM +0200, Dmitry Baryshkov wrote:
> From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> 
> HDMI standard defines recommended N and CTS values for Audio Clock
> Regeneration. Currently each driver implements those, frequently in
> somewhat unique way. Provide a generic helper for getting those values
> to be used by the HDMI drivers.
> 
> The helper is added to drm_hdmi_helper.c rather than drm_hdmi_audio.c
> since HDMI drivers can be using this helper function even without
> switching to DRM HDMI Audio helpers.
> 
> Note: currently this only handles the values per HDMI 1.4b Section 7.2
> and HDMI 2.0 Section 9.2.1. Later the table can be expanded to
> accommodate for Deep Color TMDS char rates per HDMI 1.4 Appendix D
> and/or HDMI 2.0 / 2.1 Appendix C).
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/gpu/drm/display/drm_hdmi_helper.c | 164 ++++++++++++++++++++++++++++++
>  include/drm/display/drm_hdmi_helper.h     |   6 ++
>  2 files changed, 170 insertions(+)
> 
> diff --git a/drivers/gpu/drm/display/drm_hdmi_helper.c b/drivers/gpu/drm/display/drm_hdmi_helper.c
> index 74dd4d01dd9bb2c9e69ec1c60b0056bd69417e8a..89d25571bfd21c56c6835821d2272a12c816a76e 100644
> --- a/drivers/gpu/drm/display/drm_hdmi_helper.c
> +++ b/drivers/gpu/drm/display/drm_hdmi_helper.c
> @@ -256,3 +256,167 @@ drm_hdmi_compute_mode_clock(const struct drm_display_mode *mode,
>  	return DIV_ROUND_CLOSEST_ULL(clock * bpc, 8);
>  }
>  EXPORT_SYMBOL(drm_hdmi_compute_mode_clock);
> +
> +struct drm_hdmi_acr_n_cts_entry {
> +	unsigned int n;
> +	unsigned int cts;
> +};
> +
> +struct drm_hdmi_acr_data {
> +	unsigned long tmds_clock_khz;
> +	struct drm_hdmi_acr_n_cts_entry n_cts_32k,
> +					n_cts_44k1,
> +					n_cts_48k;
> +};
> +
> +static const struct drm_hdmi_acr_data hdmi_acr_n_cts[] = {
> +	{
> +		/* "Other" entry */
> +		.n_cts_32k =  { .n = 4096, },
> +		.n_cts_44k1 = { .n = 6272, },
> +		.n_cts_48k =  { .n = 6144, },
> +	}, {
> +		.tmds_clock_khz = 25175,
> +		.n_cts_32k =  { .n = 4576,  .cts = 28125, },
> +		.n_cts_44k1 = { .n = 7007,  .cts = 31250, },
> +		.n_cts_48k =  { .n = 6864,  .cts = 28125, },
> +	}, {
> +		.tmds_clock_khz = 25200,
> +		.n_cts_32k =  { .n = 4096,  .cts = 25200, },
> +		.n_cts_44k1 = { .n = 6272,  .cts = 28000, },
> +		.n_cts_48k =  { .n = 6144,  .cts = 25200, },
> +	}, {
> +		.tmds_clock_khz = 27000,
> +		.n_cts_32k =  { .n = 4096,  .cts = 27000, },
> +		.n_cts_44k1 = { .n = 6272,  .cts = 30000, },
> +		.n_cts_48k =  { .n = 6144,  .cts = 27000, },
> +	}, {
> +		.tmds_clock_khz = 27027,
> +		.n_cts_32k =  { .n = 4096,  .cts = 27027, },
> +		.n_cts_44k1 = { .n = 6272,  .cts = 30030, },
> +		.n_cts_48k =  { .n = 6144,  .cts = 27027, },
> +	}, {
> +		.tmds_clock_khz = 54000,
> +		.n_cts_32k =  { .n = 4096,  .cts = 54000, },
> +		.n_cts_44k1 = { .n = 6272,  .cts = 60000, },
> +		.n_cts_48k =  { .n = 6144,  .cts = 54000, },
> +	}, {
> +		.tmds_clock_khz = 54054,
> +		.n_cts_32k =  { .n = 4096,  .cts = 54054, },
> +		.n_cts_44k1 = { .n = 6272,  .cts = 60060, },
> +		.n_cts_48k =  { .n = 6144,  .cts = 54054, },
> +	}, {
> +		.tmds_clock_khz = 74176,
> +		.n_cts_32k =  { .n = 11648, .cts = 210937, }, /* and 210938 */
> +		.n_cts_44k1 = { .n = 17836, .cts = 234375, },
> +		.n_cts_48k =  { .n = 11648, .cts = 140625, },
> +	}, {
> +		.tmds_clock_khz = 74250,
> +		.n_cts_32k =  { .n = 4096,  .cts = 74250, },
> +		.n_cts_44k1 = { .n = 6272,  .cts = 82500, },
> +		.n_cts_48k =  { .n = 6144,  .cts = 74250, },
> +	}, {
> +		.tmds_clock_khz = 148352,
> +		.n_cts_32k =  { .n = 11648, .cts = 421875, },
> +		.n_cts_44k1 = { .n = 8918,  .cts = 234375, },
> +		.n_cts_48k =  { .n = 5824,  .cts = 140625, },
> +	}, {
> +		.tmds_clock_khz = 148500,
> +		.n_cts_32k =  { .n = 4096,  .cts = 148500, },
> +		.n_cts_44k1 = { .n = 6272,  .cts = 165000, },
> +		.n_cts_48k =  { .n = 6144,  .cts = 148500, },
> +	}, {
> +		.tmds_clock_khz = 296703,
> +		.n_cts_32k =  { .n = 5824,  .cts = 421875, },
> +		.n_cts_44k1 = { .n = 4459,  .cts = 234375, },
> +		.n_cts_48k =  { .n = 5824,  .cts = 281250, },
> +	}, {
> +		.tmds_clock_khz = 297000,
> +		.n_cts_32k =  { .n = 3072,  .cts = 222750, },
> +		.n_cts_44k1 = { .n = 4704,  .cts = 247500, },
> +		.n_cts_48k =  { .n = 5120,  .cts = 247500, },
> +	}, {
> +		.tmds_clock_khz = 593407,
> +		.n_cts_32k =  { .n = 5824,  .cts = 843750, },
> +		.n_cts_44k1 = { .n = 8918,  .cts = 937500, },
> +		.n_cts_48k =  { .n = 5824,  .cts = 562500, },
> +	}, {
> +		.tmds_clock_khz = 594000,
> +		.n_cts_32k =  { .n = 3072,  .cts = 445500, },
> +		.n_cts_44k1 = { .n = 9408,  .cts = 990000, },
> +		.n_cts_48k =  { .n = 6144,  .cts = 594000, },
> +	},
> +};
> +
> +static int drm_hdmi_acr_find_tmds_entry(unsigned long tmds_clock_khz)
> +{
> +	int i;
> +
> +	/* skip the "other" entry */
> +	for (i = 1; i < ARRAY_SIZE(hdmi_acr_n_cts); i++) {
> +		if (hdmi_acr_n_cts[i].tmds_clock_khz == tmds_clock_khz)
> +			return i;
> +	}
> +
> +	return 0;
> +}
> +
> +/**
> + * drm_hdmi_acr_get_n_cts() - get N and CTS values for Audio Clock Regeneration
> + *
> + * @tmds_char_rate: TMDS clock (char rate) as used by the HDMI connector
> + * @sample_rate: audio sample rate
> + * @out_n: a pointer to write the N value
> + * @out_cts: a pointer to write the CTS value
> + *
> + * Get the N and CTS values (either by calculating them or by returning data
> + * from the tables. This follows the HDMI 1.4b Section 7.2 "Audio Sample Clock
> + * Capture and Regeneration".
> + */

I think we need to make it clear that it's for L-PCM only (I think?),
either through a format parameter or through the documentation.

> +void
> +drm_hdmi_acr_get_n_cts(unsigned long long tmds_char_rate,
> +		       unsigned int sample_rate,
> +		       unsigned int *out_n,
> +		       unsigned int *out_cts)

And we should probably take the connector (or EDID) to make sure the
monitor can support the format and sample rates.

> +{
> +	/* be a bit more tolerant, especially for the 1.001 entries */
> +	unsigned long tmds_clock_khz = DIV_ROUND_CLOSEST_ULL(tmds_char_rate, 1000);
> +	const struct drm_hdmi_acr_n_cts_entry *entry;
> +	unsigned int n, cts, mult;
> +	int tmds_idx;
> +
> +	tmds_idx = drm_hdmi_acr_find_tmds_entry(tmds_clock_khz);
> +
> +	/*
> +	 * Don't change the order, 192 kHz is divisible by 48k and 32k, but it
> +	 * should use 48k entry.
> +	 */
> +	if (sample_rate % 48000 == 0) {
> +		entry = &hdmi_acr_n_cts[tmds_idx].n_cts_48k;
> +		mult = sample_rate / 48000;
> +	} else if (sample_rate % 44100 == 0) {
> +		entry = &hdmi_acr_n_cts[tmds_idx].n_cts_44k1;
> +		mult = sample_rate / 44100;
> +	} else if (sample_rate % 32000 == 0) {
> +		entry = &hdmi_acr_n_cts[tmds_idx].n_cts_32k;
> +		mult = sample_rate / 32000;
> +	} else {
> +		entry = NULL;
> +	}
> +
> +	if (entry) {
> +		n = entry->n * mult;
> +		cts = entry->cts;
> +	} else {
> +		/* Recommended optimal value, HDMI 1.4b, Section 7.2.1 */
> +		n = 128 * sample_rate / 1000;
> +		cts = 0;
> +	}
> +
> +	if (!cts)
> +		cts = DIV_ROUND_CLOSEST_ULL(tmds_char_rate * n,
> +					    128 * sample_rate);
> +
> +	*out_n = n;
> +	*out_cts = cts;
> +}

EXPORT_SYMBOL?

Also, I'd really like to have some unit tests for this. Not for all the
combinations, obviously, but testing that, say, 44.1kHz with a 148.5 MHz
char rate works as expected, and then all the failure conditions
depending on the monitor capabilities.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 1/4] drm/display: hdmi: provide central data authority for ACR params

[Dmitry Baryshkov]

On Mon, Mar 10, 2025 at 03:46:33PM +0100, Maxime Ripard wrote:
> On Sun, Mar 09, 2025 at 10:13:56AM +0200, Dmitry Baryshkov wrote:
> > From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > 
> > HDMI standard defines recommended N and CTS values for Audio Clock
> > Regeneration. Currently each driver implements those, frequently in
> > somewhat unique way. Provide a generic helper for getting those values
> > to be used by the HDMI drivers.
> > 
> > The helper is added to drm_hdmi_helper.c rather than drm_hdmi_audio.c
> > since HDMI drivers can be using this helper function even without
> > switching to DRM HDMI Audio helpers.
> > 
> > Note: currently this only handles the values per HDMI 1.4b Section 7.2
> > and HDMI 2.0 Section 9.2.1. Later the table can be expanded to
> > accommodate for Deep Color TMDS char rates per HDMI 1.4 Appendix D
> > and/or HDMI 2.0 / 2.1 Appendix C).
> > 
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >  drivers/gpu/drm/display/drm_hdmi_helper.c | 164 ++++++++++++++++++++++++++++++
> >  include/drm/display/drm_hdmi_helper.h     |   6 ++
> >  2 files changed, 170 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/display/drm_hdmi_helper.c b/drivers/gpu/drm/display/drm_hdmi_helper.c
> > index 74dd4d01dd9bb2c9e69ec1c60b0056bd69417e8a..89d25571bfd21c56c6835821d2272a12c816a76e 100644
> > --- a/drivers/gpu/drm/display/drm_hdmi_helper.c
> > +++ b/drivers/gpu/drm/display/drm_hdmi_helper.c
> > @@ -256,3 +256,167 @@ drm_hdmi_compute_mode_clock(const struct drm_display_mode *mode,
> >  	return DIV_ROUND_CLOSEST_ULL(clock * bpc, 8);
> >  }
> >  EXPORT_SYMBOL(drm_hdmi_compute_mode_clock);
> > +
> > +struct drm_hdmi_acr_n_cts_entry {
> > +	unsigned int n;
> > +	unsigned int cts;
> > +};
> > +
> > +struct drm_hdmi_acr_data {
> > +	unsigned long tmds_clock_khz;
> > +	struct drm_hdmi_acr_n_cts_entry n_cts_32k,
> > +					n_cts_44k1,
> > +					n_cts_48k;
> > +};
> > +
> > +static const struct drm_hdmi_acr_data hdmi_acr_n_cts[] = {
> > +	{
> > +		/* "Other" entry */
> > +		.n_cts_32k =  { .n = 4096, },
> > +		.n_cts_44k1 = { .n = 6272, },
> > +		.n_cts_48k =  { .n = 6144, },
> > +	}, {
> > +		.tmds_clock_khz = 25175,
> > +		.n_cts_32k =  { .n = 4576,  .cts = 28125, },
> > +		.n_cts_44k1 = { .n = 7007,  .cts = 31250, },
> > +		.n_cts_48k =  { .n = 6864,  .cts = 28125, },
> > +	}, {
> > +		.tmds_clock_khz = 25200,
> > +		.n_cts_32k =  { .n = 4096,  .cts = 25200, },
> > +		.n_cts_44k1 = { .n = 6272,  .cts = 28000, },
> > +		.n_cts_48k =  { .n = 6144,  .cts = 25200, },
> > +	}, {
> > +		.tmds_clock_khz = 27000,
> > +		.n_cts_32k =  { .n = 4096,  .cts = 27000, },
> > +		.n_cts_44k1 = { .n = 6272,  .cts = 30000, },
> > +		.n_cts_48k =  { .n = 6144,  .cts = 27000, },
> > +	}, {
> > +		.tmds_clock_khz = 27027,
> > +		.n_cts_32k =  { .n = 4096,  .cts = 27027, },
> > +		.n_cts_44k1 = { .n = 6272,  .cts = 30030, },
> > +		.n_cts_48k =  { .n = 6144,  .cts = 27027, },
> > +	}, {
> > +		.tmds_clock_khz = 54000,
> > +		.n_cts_32k =  { .n = 4096,  .cts = 54000, },
> > +		.n_cts_44k1 = { .n = 6272,  .cts = 60000, },
> > +		.n_cts_48k =  { .n = 6144,  .cts = 54000, },
> > +	}, {
> > +		.tmds_clock_khz = 54054,
> > +		.n_cts_32k =  { .n = 4096,  .cts = 54054, },
> > +		.n_cts_44k1 = { .n = 6272,  .cts = 60060, },
> > +		.n_cts_48k =  { .n = 6144,  .cts = 54054, },
> > +	}, {
> > +		.tmds_clock_khz = 74176,
> > +		.n_cts_32k =  { .n = 11648, .cts = 210937, }, /* and 210938 */
> > +		.n_cts_44k1 = { .n = 17836, .cts = 234375, },
> > +		.n_cts_48k =  { .n = 11648, .cts = 140625, },
> > +	}, {
> > +		.tmds_clock_khz = 74250,
> > +		.n_cts_32k =  { .n = 4096,  .cts = 74250, },
> > +		.n_cts_44k1 = { .n = 6272,  .cts = 82500, },
> > +		.n_cts_48k =  { .n = 6144,  .cts = 74250, },
> > +	}, {
> > +		.tmds_clock_khz = 148352,
> > +		.n_cts_32k =  { .n = 11648, .cts = 421875, },
> > +		.n_cts_44k1 = { .n = 8918,  .cts = 234375, },
> > +		.n_cts_48k =  { .n = 5824,  .cts = 140625, },
> > +	}, {
> > +		.tmds_clock_khz = 148500,
> > +		.n_cts_32k =  { .n = 4096,  .cts = 148500, },
> > +		.n_cts_44k1 = { .n = 6272,  .cts = 165000, },
> > +		.n_cts_48k =  { .n = 6144,  .cts = 148500, },
> > +	}, {
> > +		.tmds_clock_khz = 296703,
> > +		.n_cts_32k =  { .n = 5824,  .cts = 421875, },
> > +		.n_cts_44k1 = { .n = 4459,  .cts = 234375, },
> > +		.n_cts_48k =  { .n = 5824,  .cts = 281250, },
> > +	}, {
> > +		.tmds_clock_khz = 297000,
> > +		.n_cts_32k =  { .n = 3072,  .cts = 222750, },
> > +		.n_cts_44k1 = { .n = 4704,  .cts = 247500, },
> > +		.n_cts_48k =  { .n = 5120,  .cts = 247500, },
> > +	}, {
> > +		.tmds_clock_khz = 593407,
> > +		.n_cts_32k =  { .n = 5824,  .cts = 843750, },
> > +		.n_cts_44k1 = { .n = 8918,  .cts = 937500, },
> > +		.n_cts_48k =  { .n = 5824,  .cts = 562500, },
> > +	}, {
> > +		.tmds_clock_khz = 594000,
> > +		.n_cts_32k =  { .n = 3072,  .cts = 445500, },
> > +		.n_cts_44k1 = { .n = 9408,  .cts = 990000, },
> > +		.n_cts_48k =  { .n = 6144,  .cts = 594000, },
> > +	},
> > +};
> > +
> > +static int drm_hdmi_acr_find_tmds_entry(unsigned long tmds_clock_khz)
> > +{
> > +	int i;
> > +
> > +	/* skip the "other" entry */
> > +	for (i = 1; i < ARRAY_SIZE(hdmi_acr_n_cts); i++) {
> > +		if (hdmi_acr_n_cts[i].tmds_clock_khz == tmds_clock_khz)
> > +			return i;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * drm_hdmi_acr_get_n_cts() - get N and CTS values for Audio Clock Regeneration
> > + *
> > + * @tmds_char_rate: TMDS clock (char rate) as used by the HDMI connector
> > + * @sample_rate: audio sample rate
> > + * @out_n: a pointer to write the N value
> > + * @out_cts: a pointer to write the CTS value
> > + *
> > + * Get the N and CTS values (either by calculating them or by returning data
> > + * from the tables. This follows the HDMI 1.4b Section 7.2 "Audio Sample Clock
> > + * Capture and Regeneration".
> > + */
> 
> I think we need to make it clear that it's for L-PCM only (I think?),
> either through a format parameter or through the documentation.

Ack

> 
> > +void
> > +drm_hdmi_acr_get_n_cts(unsigned long long tmds_char_rate,
> > +		       unsigned int sample_rate,
> > +		       unsigned int *out_n,
> > +		       unsigned int *out_cts)
> 
> And we should probably take the connector (or EDID) to make sure the
> monitor can support the format and sample rates.

Interesting perspective, I'll give it a thought. I was really just
trying to get rid of the duplication.

I think that 'supported' parts should be implemented in the hdmi-codec
instead, parsing the ELD and updating hw constraints. WDYT?

> 
> > +{
> > +	/* be a bit more tolerant, especially for the 1.001 entries */
> > +	unsigned long tmds_clock_khz = DIV_ROUND_CLOSEST_ULL(tmds_char_rate, 1000);
> > +	const struct drm_hdmi_acr_n_cts_entry *entry;
> > +	unsigned int n, cts, mult;
> > +	int tmds_idx;
> > +
> > +	tmds_idx = drm_hdmi_acr_find_tmds_entry(tmds_clock_khz);
> > +
> > +	/*
> > +	 * Don't change the order, 192 kHz is divisible by 48k and 32k, but it
> > +	 * should use 48k entry.
> > +	 */
> > +	if (sample_rate % 48000 == 0) {
> > +		entry = &hdmi_acr_n_cts[tmds_idx].n_cts_48k;
> > +		mult = sample_rate / 48000;
> > +	} else if (sample_rate % 44100 == 0) {
> > +		entry = &hdmi_acr_n_cts[tmds_idx].n_cts_44k1;
> > +		mult = sample_rate / 44100;
> > +	} else if (sample_rate % 32000 == 0) {
> > +		entry = &hdmi_acr_n_cts[tmds_idx].n_cts_32k;
> > +		mult = sample_rate / 32000;
> > +	} else {
> > +		entry = NULL;
> > +	}
> > +
> > +	if (entry) {
> > +		n = entry->n * mult;
> > +		cts = entry->cts;
> > +	} else {
> > +		/* Recommended optimal value, HDMI 1.4b, Section 7.2.1 */
> > +		n = 128 * sample_rate / 1000;
> > +		cts = 0;
> > +	}
> > +
> > +	if (!cts)
> > +		cts = DIV_ROUND_CLOSEST_ULL(tmds_char_rate * n,
> > +					    128 * sample_rate);
> > +
> > +	*out_n = n;
> > +	*out_cts = cts;
> > +}
> 
> EXPORT_SYMBOL?

Yes, I forgot it.

> 
> Also, I'd really like to have some unit tests for this. Not for all the
> combinations, obviously, but testing that, say, 44.1kHz with a 148.5 MHz
> char rate works as expected, and then all the failure conditions
> depending on the monitor capabilities.

Ack for the tests. For the monitor capabilities, let's finish the
discussion above first.



-- 
With best wishes
Dmitry

Re: [PATCH 1/4] drm/display: hdmi: provide central data authority for ACR params

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 7422 bytes --]

On Mon, Mar 10, 2025 at 10:14:52PM +0200, Dmitry Baryshkov wrote:
> On Mon, Mar 10, 2025 at 03:46:33PM +0100, Maxime Ripard wrote:
> > On Sun, Mar 09, 2025 at 10:13:56AM +0200, Dmitry Baryshkov wrote:
> > > From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > 
> > > HDMI standard defines recommended N and CTS values for Audio Clock
> > > Regeneration. Currently each driver implements those, frequently in
> > > somewhat unique way. Provide a generic helper for getting those values
> > > to be used by the HDMI drivers.
> > > 
> > > The helper is added to drm_hdmi_helper.c rather than drm_hdmi_audio.c
> > > since HDMI drivers can be using this helper function even without
> > > switching to DRM HDMI Audio helpers.
> > > 
> > > Note: currently this only handles the values per HDMI 1.4b Section 7.2
> > > and HDMI 2.0 Section 9.2.1. Later the table can be expanded to
> > > accommodate for Deep Color TMDS char rates per HDMI 1.4 Appendix D
> > > and/or HDMI 2.0 / 2.1 Appendix C).
> > > 
> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > ---
> > >  drivers/gpu/drm/display/drm_hdmi_helper.c | 164 ++++++++++++++++++++++++++++++
> > >  include/drm/display/drm_hdmi_helper.h     |   6 ++
> > >  2 files changed, 170 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/display/drm_hdmi_helper.c b/drivers/gpu/drm/display/drm_hdmi_helper.c
> > > index 74dd4d01dd9bb2c9e69ec1c60b0056bd69417e8a..89d25571bfd21c56c6835821d2272a12c816a76e 100644
> > > --- a/drivers/gpu/drm/display/drm_hdmi_helper.c
> > > +++ b/drivers/gpu/drm/display/drm_hdmi_helper.c
> > > @@ -256,3 +256,167 @@ drm_hdmi_compute_mode_clock(const struct drm_display_mode *mode,
> > >  	return DIV_ROUND_CLOSEST_ULL(clock * bpc, 8);
> > >  }
> > >  EXPORT_SYMBOL(drm_hdmi_compute_mode_clock);
> > > +
> > > +struct drm_hdmi_acr_n_cts_entry {
> > > +	unsigned int n;
> > > +	unsigned int cts;
> > > +};
> > > +
> > > +struct drm_hdmi_acr_data {
> > > +	unsigned long tmds_clock_khz;
> > > +	struct drm_hdmi_acr_n_cts_entry n_cts_32k,
> > > +					n_cts_44k1,
> > > +					n_cts_48k;
> > > +};
> > > +
> > > +static const struct drm_hdmi_acr_data hdmi_acr_n_cts[] = {
> > > +	{
> > > +		/* "Other" entry */
> > > +		.n_cts_32k =  { .n = 4096, },
> > > +		.n_cts_44k1 = { .n = 6272, },
> > > +		.n_cts_48k =  { .n = 6144, },
> > > +	}, {
> > > +		.tmds_clock_khz = 25175,
> > > +		.n_cts_32k =  { .n = 4576,  .cts = 28125, },
> > > +		.n_cts_44k1 = { .n = 7007,  .cts = 31250, },
> > > +		.n_cts_48k =  { .n = 6864,  .cts = 28125, },
> > > +	}, {
> > > +		.tmds_clock_khz = 25200,
> > > +		.n_cts_32k =  { .n = 4096,  .cts = 25200, },
> > > +		.n_cts_44k1 = { .n = 6272,  .cts = 28000, },
> > > +		.n_cts_48k =  { .n = 6144,  .cts = 25200, },
> > > +	}, {
> > > +		.tmds_clock_khz = 27000,
> > > +		.n_cts_32k =  { .n = 4096,  .cts = 27000, },
> > > +		.n_cts_44k1 = { .n = 6272,  .cts = 30000, },
> > > +		.n_cts_48k =  { .n = 6144,  .cts = 27000, },
> > > +	}, {
> > > +		.tmds_clock_khz = 27027,
> > > +		.n_cts_32k =  { .n = 4096,  .cts = 27027, },
> > > +		.n_cts_44k1 = { .n = 6272,  .cts = 30030, },
> > > +		.n_cts_48k =  { .n = 6144,  .cts = 27027, },
> > > +	}, {
> > > +		.tmds_clock_khz = 54000,
> > > +		.n_cts_32k =  { .n = 4096,  .cts = 54000, },
> > > +		.n_cts_44k1 = { .n = 6272,  .cts = 60000, },
> > > +		.n_cts_48k =  { .n = 6144,  .cts = 54000, },
> > > +	}, {
> > > +		.tmds_clock_khz = 54054,
> > > +		.n_cts_32k =  { .n = 4096,  .cts = 54054, },
> > > +		.n_cts_44k1 = { .n = 6272,  .cts = 60060, },
> > > +		.n_cts_48k =  { .n = 6144,  .cts = 54054, },
> > > +	}, {
> > > +		.tmds_clock_khz = 74176,
> > > +		.n_cts_32k =  { .n = 11648, .cts = 210937, }, /* and 210938 */
> > > +		.n_cts_44k1 = { .n = 17836, .cts = 234375, },
> > > +		.n_cts_48k =  { .n = 11648, .cts = 140625, },
> > > +	}, {
> > > +		.tmds_clock_khz = 74250,
> > > +		.n_cts_32k =  { .n = 4096,  .cts = 74250, },
> > > +		.n_cts_44k1 = { .n = 6272,  .cts = 82500, },
> > > +		.n_cts_48k =  { .n = 6144,  .cts = 74250, },
> > > +	}, {
> > > +		.tmds_clock_khz = 148352,
> > > +		.n_cts_32k =  { .n = 11648, .cts = 421875, },
> > > +		.n_cts_44k1 = { .n = 8918,  .cts = 234375, },
> > > +		.n_cts_48k =  { .n = 5824,  .cts = 140625, },
> > > +	}, {
> > > +		.tmds_clock_khz = 148500,
> > > +		.n_cts_32k =  { .n = 4096,  .cts = 148500, },
> > > +		.n_cts_44k1 = { .n = 6272,  .cts = 165000, },
> > > +		.n_cts_48k =  { .n = 6144,  .cts = 148500, },
> > > +	}, {
> > > +		.tmds_clock_khz = 296703,
> > > +		.n_cts_32k =  { .n = 5824,  .cts = 421875, },
> > > +		.n_cts_44k1 = { .n = 4459,  .cts = 234375, },
> > > +		.n_cts_48k =  { .n = 5824,  .cts = 281250, },
> > > +	}, {
> > > +		.tmds_clock_khz = 297000,
> > > +		.n_cts_32k =  { .n = 3072,  .cts = 222750, },
> > > +		.n_cts_44k1 = { .n = 4704,  .cts = 247500, },
> > > +		.n_cts_48k =  { .n = 5120,  .cts = 247500, },
> > > +	}, {
> > > +		.tmds_clock_khz = 593407,
> > > +		.n_cts_32k =  { .n = 5824,  .cts = 843750, },
> > > +		.n_cts_44k1 = { .n = 8918,  .cts = 937500, },
> > > +		.n_cts_48k =  { .n = 5824,  .cts = 562500, },
> > > +	}, {
> > > +		.tmds_clock_khz = 594000,
> > > +		.n_cts_32k =  { .n = 3072,  .cts = 445500, },
> > > +		.n_cts_44k1 = { .n = 9408,  .cts = 990000, },
> > > +		.n_cts_48k =  { .n = 6144,  .cts = 594000, },
> > > +	},
> > > +};
> > > +
> > > +static int drm_hdmi_acr_find_tmds_entry(unsigned long tmds_clock_khz)
> > > +{
> > > +	int i;
> > > +
> > > +	/* skip the "other" entry */
> > > +	for (i = 1; i < ARRAY_SIZE(hdmi_acr_n_cts); i++) {
> > > +		if (hdmi_acr_n_cts[i].tmds_clock_khz == tmds_clock_khz)
> > > +			return i;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +/**
> > > + * drm_hdmi_acr_get_n_cts() - get N and CTS values for Audio Clock Regeneration
> > > + *
> > > + * @tmds_char_rate: TMDS clock (char rate) as used by the HDMI connector
> > > + * @sample_rate: audio sample rate
> > > + * @out_n: a pointer to write the N value
> > > + * @out_cts: a pointer to write the CTS value
> > > + *
> > > + * Get the N and CTS values (either by calculating them or by returning data
> > > + * from the tables. This follows the HDMI 1.4b Section 7.2 "Audio Sample Clock
> > > + * Capture and Regeneration".
> > > + */
> > 
> > I think we need to make it clear that it's for L-PCM only (I think?),
> > either through a format parameter or through the documentation.
> 
> Ack
> 
> > 
> > > +void
> > > +drm_hdmi_acr_get_n_cts(unsigned long long tmds_char_rate,
> > > +		       unsigned int sample_rate,
> > > +		       unsigned int *out_n,
> > > +		       unsigned int *out_cts)
> > 
> > And we should probably take the connector (or EDID) to make sure the
> > monitor can support the format and sample rates.
> 
> Interesting perspective, I'll give it a thought. I was really just
> trying to get rid of the duplication.
> 
> I think that 'supported' parts should be implemented in the hdmi-codec
> instead, parsing the ELD and updating hw constraints. WDYT?

Basically, I want to make sure we cover section 7.3 of HDMI 1.4, ie,
make sure we can't end up (or validate) in a situation that isn't
allowed by the spec.

If ALSA covers it already, then I guess it's fine, but we should
document it and point to where it's dealt with.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 1/4] drm/display: hdmi: provide central data authority for ACR params

[Dmitry Baryshkov]

On Tue, Mar 11, 2025 at 08:59:45AM +0100, Maxime Ripard wrote:
> On Mon, Mar 10, 2025 at 10:14:52PM +0200, Dmitry Baryshkov wrote:
> > On Mon, Mar 10, 2025 at 03:46:33PM +0100, Maxime Ripard wrote:
> > > On Sun, Mar 09, 2025 at 10:13:56AM +0200, Dmitry Baryshkov wrote:
> > > > From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > 
> > > > HDMI standard defines recommended N and CTS values for Audio Clock
> > > > Regeneration. Currently each driver implements those, frequently in
> > > > somewhat unique way. Provide a generic helper for getting those values
> > > > to be used by the HDMI drivers.
> > > > 
> > > > The helper is added to drm_hdmi_helper.c rather than drm_hdmi_audio.c
> > > > since HDMI drivers can be using this helper function even without
> > > > switching to DRM HDMI Audio helpers.
> > > > 
> > > > Note: currently this only handles the values per HDMI 1.4b Section 7.2
> > > > and HDMI 2.0 Section 9.2.1. Later the table can be expanded to
> > > > accommodate for Deep Color TMDS char rates per HDMI 1.4 Appendix D
> > > > and/or HDMI 2.0 / 2.1 Appendix C).
> > > > 
> > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > ---
> > > >  drivers/gpu/drm/display/drm_hdmi_helper.c | 164 ++++++++++++++++++++++++++++++
> > > >  include/drm/display/drm_hdmi_helper.h     |   6 ++
> > > >  2 files changed, 170 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/display/drm_hdmi_helper.c b/drivers/gpu/drm/display/drm_hdmi_helper.c
> > > > index 74dd4d01dd9bb2c9e69ec1c60b0056bd69417e8a..89d25571bfd21c56c6835821d2272a12c816a76e 100644
> > > > --- a/drivers/gpu/drm/display/drm_hdmi_helper.c
> > > > +++ b/drivers/gpu/drm/display/drm_hdmi_helper.c
> > > > @@ -256,3 +256,167 @@ drm_hdmi_compute_mode_clock(const struct drm_display_mode *mode,
> > > >  	return DIV_ROUND_CLOSEST_ULL(clock * bpc, 8);
> > > >  }
> > > >  EXPORT_SYMBOL(drm_hdmi_compute_mode_clock);
> > > > +
> > > > +struct drm_hdmi_acr_n_cts_entry {
> > > > +	unsigned int n;
> > > > +	unsigned int cts;
> > > > +};
> > > > +
> > > > +struct drm_hdmi_acr_data {
> > > > +	unsigned long tmds_clock_khz;
> > > > +	struct drm_hdmi_acr_n_cts_entry n_cts_32k,
> > > > +					n_cts_44k1,
> > > > +					n_cts_48k;
> > > > +};
> > > > +
> > > > +static const struct drm_hdmi_acr_data hdmi_acr_n_cts[] = {
> > > > +	{
> > > > +		/* "Other" entry */
> > > > +		.n_cts_32k =  { .n = 4096, },
> > > > +		.n_cts_44k1 = { .n = 6272, },
> > > > +		.n_cts_48k =  { .n = 6144, },
> > > > +	}, {
> > > > +		.tmds_clock_khz = 25175,
> > > > +		.n_cts_32k =  { .n = 4576,  .cts = 28125, },
> > > > +		.n_cts_44k1 = { .n = 7007,  .cts = 31250, },
> > > > +		.n_cts_48k =  { .n = 6864,  .cts = 28125, },
> > > > +	}, {
> > > > +		.tmds_clock_khz = 25200,
> > > > +		.n_cts_32k =  { .n = 4096,  .cts = 25200, },
> > > > +		.n_cts_44k1 = { .n = 6272,  .cts = 28000, },
> > > > +		.n_cts_48k =  { .n = 6144,  .cts = 25200, },
> > > > +	}, {
> > > > +		.tmds_clock_khz = 27000,
> > > > +		.n_cts_32k =  { .n = 4096,  .cts = 27000, },
> > > > +		.n_cts_44k1 = { .n = 6272,  .cts = 30000, },
> > > > +		.n_cts_48k =  { .n = 6144,  .cts = 27000, },
> > > > +	}, {
> > > > +		.tmds_clock_khz = 27027,
> > > > +		.n_cts_32k =  { .n = 4096,  .cts = 27027, },
> > > > +		.n_cts_44k1 = { .n = 6272,  .cts = 30030, },
> > > > +		.n_cts_48k =  { .n = 6144,  .cts = 27027, },
> > > > +	}, {
> > > > +		.tmds_clock_khz = 54000,
> > > > +		.n_cts_32k =  { .n = 4096,  .cts = 54000, },
> > > > +		.n_cts_44k1 = { .n = 6272,  .cts = 60000, },
> > > > +		.n_cts_48k =  { .n = 6144,  .cts = 54000, },
> > > > +	}, {
> > > > +		.tmds_clock_khz = 54054,
> > > > +		.n_cts_32k =  { .n = 4096,  .cts = 54054, },
> > > > +		.n_cts_44k1 = { .n = 6272,  .cts = 60060, },
> > > > +		.n_cts_48k =  { .n = 6144,  .cts = 54054, },
> > > > +	}, {
> > > > +		.tmds_clock_khz = 74176,
> > > > +		.n_cts_32k =  { .n = 11648, .cts = 210937, }, /* and 210938 */
> > > > +		.n_cts_44k1 = { .n = 17836, .cts = 234375, },
> > > > +		.n_cts_48k =  { .n = 11648, .cts = 140625, },
> > > > +	}, {
> > > > +		.tmds_clock_khz = 74250,
> > > > +		.n_cts_32k =  { .n = 4096,  .cts = 74250, },
> > > > +		.n_cts_44k1 = { .n = 6272,  .cts = 82500, },
> > > > +		.n_cts_48k =  { .n = 6144,  .cts = 74250, },
> > > > +	}, {
> > > > +		.tmds_clock_khz = 148352,
> > > > +		.n_cts_32k =  { .n = 11648, .cts = 421875, },
> > > > +		.n_cts_44k1 = { .n = 8918,  .cts = 234375, },
> > > > +		.n_cts_48k =  { .n = 5824,  .cts = 140625, },
> > > > +	}, {
> > > > +		.tmds_clock_khz = 148500,
> > > > +		.n_cts_32k =  { .n = 4096,  .cts = 148500, },
> > > > +		.n_cts_44k1 = { .n = 6272,  .cts = 165000, },
> > > > +		.n_cts_48k =  { .n = 6144,  .cts = 148500, },
> > > > +	}, {
> > > > +		.tmds_clock_khz = 296703,
> > > > +		.n_cts_32k =  { .n = 5824,  .cts = 421875, },
> > > > +		.n_cts_44k1 = { .n = 4459,  .cts = 234375, },
> > > > +		.n_cts_48k =  { .n = 5824,  .cts = 281250, },
> > > > +	}, {
> > > > +		.tmds_clock_khz = 297000,
> > > > +		.n_cts_32k =  { .n = 3072,  .cts = 222750, },
> > > > +		.n_cts_44k1 = { .n = 4704,  .cts = 247500, },
> > > > +		.n_cts_48k =  { .n = 5120,  .cts = 247500, },
> > > > +	}, {
> > > > +		.tmds_clock_khz = 593407,
> > > > +		.n_cts_32k =  { .n = 5824,  .cts = 843750, },
> > > > +		.n_cts_44k1 = { .n = 8918,  .cts = 937500, },
> > > > +		.n_cts_48k =  { .n = 5824,  .cts = 562500, },
> > > > +	}, {
> > > > +		.tmds_clock_khz = 594000,
> > > > +		.n_cts_32k =  { .n = 3072,  .cts = 445500, },
> > > > +		.n_cts_44k1 = { .n = 9408,  .cts = 990000, },
> > > > +		.n_cts_48k =  { .n = 6144,  .cts = 594000, },
> > > > +	},
> > > > +};
> > > > +
> > > > +static int drm_hdmi_acr_find_tmds_entry(unsigned long tmds_clock_khz)
> > > > +{
> > > > +	int i;
> > > > +
> > > > +	/* skip the "other" entry */
> > > > +	for (i = 1; i < ARRAY_SIZE(hdmi_acr_n_cts); i++) {
> > > > +		if (hdmi_acr_n_cts[i].tmds_clock_khz == tmds_clock_khz)
> > > > +			return i;
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +/**
> > > > + * drm_hdmi_acr_get_n_cts() - get N and CTS values for Audio Clock Regeneration
> > > > + *
> > > > + * @tmds_char_rate: TMDS clock (char rate) as used by the HDMI connector
> > > > + * @sample_rate: audio sample rate
> > > > + * @out_n: a pointer to write the N value
> > > > + * @out_cts: a pointer to write the CTS value
> > > > + *
> > > > + * Get the N and CTS values (either by calculating them or by returning data
> > > > + * from the tables. This follows the HDMI 1.4b Section 7.2 "Audio Sample Clock
> > > > + * Capture and Regeneration".
> > > > + */
> > > 
> > > I think we need to make it clear that it's for L-PCM only (I think?),
> > > either through a format parameter or through the documentation.
> > 
> > Ack
> > 
> > > 
> > > > +void
> > > > +drm_hdmi_acr_get_n_cts(unsigned long long tmds_char_rate,
> > > > +		       unsigned int sample_rate,
> > > > +		       unsigned int *out_n,
> > > > +		       unsigned int *out_cts)
> > > 
> > > And we should probably take the connector (or EDID) to make sure the
> > > monitor can support the format and sample rates.
> > 
> > Interesting perspective, I'll give it a thought. I was really just
> > trying to get rid of the duplication.
> > 
> > I think that 'supported' parts should be implemented in the hdmi-codec
> > instead, parsing the ELD and updating hw constraints. WDYT?
> 
> Basically, I want to make sure we cover section 7.3 of HDMI 1.4, ie,
> make sure we can't end up (or validate) in a situation that isn't
> allowed by the spec.

I think that's a question for a separate function. This one really
targets 7.2 rather than 7.3.

> If ALSA covers it already, then I guess it's fine, but we should
> document it and point to where it's dealt with.

I'm not sure if it covers that right now, but it should be handled on
ALSA side. For example, see sound/pci/hda/patch_hdmi.c, I think it is
handling those bits. We are providing ELD to hdmi-codec, it can
implement and propagate HW constraints.


-- 
With best wishes
Dmitry

[PATCH 2/4] drm/msm/hdmi: use new helper for ACR tables

[Dmitry Baryshkov]

From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Use new drm_hdmi_acr_get_n_cts() helper instead of hand-coding the
tables. Instead of storing the rate 'index', store the audio sample rate
in hdmi->audio.rate, removing the need for even more defines.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/hdmi/hdmi_audio.c | 107 +++-------------------------------
 1 file changed, 9 insertions(+), 98 deletions(-)

diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_audio.c b/drivers/gpu/drm/msm/hdmi/hdmi_audio.c
index 8bb975e82c17c1d77217128e9ddbd6a0575bb33d..b9ec14ef2c20ebfa03c30994eb2395f21b9502bb 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi_audio.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi_audio.c
@@ -4,6 +4,7 @@
  * Author: Rob Clark <robdclark@gmail.com>
  */
 
+#include <drm/display/drm_hdmi_helper.h>
 #include <drm/display/drm_hdmi_state_helper.h>
 
 #include <linux/hdmi.h>
@@ -12,71 +13,9 @@
 
 #include "hdmi.h"
 
-/* Supported HDMI Audio sample rates */
-#define MSM_HDMI_SAMPLE_RATE_32KHZ		0
-#define MSM_HDMI_SAMPLE_RATE_44_1KHZ		1
-#define MSM_HDMI_SAMPLE_RATE_48KHZ		2
-#define MSM_HDMI_SAMPLE_RATE_88_2KHZ		3
-#define MSM_HDMI_SAMPLE_RATE_96KHZ		4
-#define MSM_HDMI_SAMPLE_RATE_176_4KHZ		5
-#define MSM_HDMI_SAMPLE_RATE_192KHZ		6
-#define MSM_HDMI_SAMPLE_RATE_MAX		7
-
-
-struct hdmi_msm_audio_acr {
-	uint32_t n;	/* N parameter for clock regeneration */
-	uint32_t cts;	/* CTS parameter for clock regeneration */
-};
-
-struct hdmi_msm_audio_arcs {
-	unsigned long int pixclock;
-	struct hdmi_msm_audio_acr lut[MSM_HDMI_SAMPLE_RATE_MAX];
-};
-
-#define HDMI_MSM_AUDIO_ARCS(pclk, ...) { (1000 * (pclk)), __VA_ARGS__ }
-
-/* Audio constants lookup table for hdmi_msm_audio_acr_setup */
-/* Valid Pixel-Clock rates: 25.2MHz, 27MHz, 27.03MHz, 74.25MHz, 148.5MHz */
-static const struct hdmi_msm_audio_arcs acr_lut[] = {
-	/*  25.200MHz  */
-	HDMI_MSM_AUDIO_ARCS(25200, {
-		{4096, 25200}, {6272, 28000}, {6144, 25200}, {12544, 28000},
-		{12288, 25200}, {25088, 28000}, {24576, 25200} }),
-	/*  27.000MHz  */
-	HDMI_MSM_AUDIO_ARCS(27000, {
-		{4096, 27000}, {6272, 30000}, {6144, 27000}, {12544, 30000},
-		{12288, 27000}, {25088, 30000}, {24576, 27000} }),
-	/*  27.027MHz */
-	HDMI_MSM_AUDIO_ARCS(27030, {
-		{4096, 27027}, {6272, 30030}, {6144, 27027}, {12544, 30030},
-		{12288, 27027}, {25088, 30030}, {24576, 27027} }),
-	/*  74.250MHz */
-	HDMI_MSM_AUDIO_ARCS(74250, {
-		{4096, 74250}, {6272, 82500}, {6144, 74250}, {12544, 82500},
-		{12288, 74250}, {25088, 82500}, {24576, 74250} }),
-	/* 148.500MHz */
-	HDMI_MSM_AUDIO_ARCS(148500, {
-		{4096, 148500}, {6272, 165000}, {6144, 148500}, {12544, 165000},
-		{12288, 148500}, {25088, 165000}, {24576, 148500} }),
-};
-
-static const struct hdmi_msm_audio_arcs *get_arcs(unsigned long int pixclock)
-{
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(acr_lut); i++) {
-		const struct hdmi_msm_audio_arcs *arcs = &acr_lut[i];
-		if (arcs->pixclock == pixclock)
-			return arcs;
-	}
-
-	return NULL;
-}
-
 int msm_hdmi_audio_update(struct hdmi *hdmi)
 {
 	struct hdmi_audio *audio = &hdmi->audio;
-	const struct hdmi_msm_audio_arcs *arcs = NULL;
 	bool enabled = audio->enabled;
 	uint32_t acr_pkt_ctrl, vbi_pkt_ctrl, aud_pkt_ctrl;
 	uint32_t audio_config;
@@ -94,15 +33,6 @@ int msm_hdmi_audio_update(struct hdmi *hdmi)
 		enabled = false;
 	}
 
-	if (enabled) {
-		arcs = get_arcs(hdmi->pixclock);
-		if (!arcs) {
-			DBG("disabling audio: unsupported pixclock: %lu",
-					hdmi->pixclock);
-			enabled = false;
-		}
-	}
-
 	/* Read first before writing */
 	acr_pkt_ctrl = hdmi_read(hdmi, REG_HDMI_ACR_PKT_CTRL);
 	vbi_pkt_ctrl = hdmi_read(hdmi, REG_HDMI_VBI_PKT_CTRL);
@@ -116,15 +46,12 @@ int msm_hdmi_audio_update(struct hdmi *hdmi)
 		uint32_t n, cts, multiplier;
 		enum hdmi_acr_cts select;
 
-		n   = arcs->lut[audio->rate].n;
-		cts = arcs->lut[audio->rate].cts;
+		drm_hdmi_acr_get_n_cts(hdmi->pixclock, audio->rate, &n, &cts);
 
-		if ((MSM_HDMI_SAMPLE_RATE_192KHZ == audio->rate) ||
-				(MSM_HDMI_SAMPLE_RATE_176_4KHZ == audio->rate)) {
+		if (audio->rate == 192000 || audio->rate == 176400) {
 			multiplier = 4;
 			n >>= 2; /* divide N by 4 and use multiplier */
-		} else if ((MSM_HDMI_SAMPLE_RATE_96KHZ == audio->rate) ||
-				(MSM_HDMI_SAMPLE_RATE_88_2KHZ == audio->rate)) {
+		} else if (audio->rate == 96000 || audio->rate == 88200) {
 			multiplier = 2;
 			n >>= 1; /* divide N by 2 and use multiplier */
 		} else {
@@ -137,13 +64,11 @@ int msm_hdmi_audio_update(struct hdmi *hdmi)
 		acr_pkt_ctrl |= HDMI_ACR_PKT_CTRL_AUDIO_PRIORITY;
 		acr_pkt_ctrl |= HDMI_ACR_PKT_CTRL_N_MULTIPLIER(multiplier);
 
-		if ((MSM_HDMI_SAMPLE_RATE_48KHZ == audio->rate) ||
-				(MSM_HDMI_SAMPLE_RATE_96KHZ == audio->rate) ||
-				(MSM_HDMI_SAMPLE_RATE_192KHZ == audio->rate))
+		if (audio->rate == 48000 || audio->rate == 96000 ||
+		    audio->rate == 192000)
 			select = ACR_48;
-		else if ((MSM_HDMI_SAMPLE_RATE_44_1KHZ == audio->rate) ||
-				(MSM_HDMI_SAMPLE_RATE_88_2KHZ == audio->rate) ||
-				(MSM_HDMI_SAMPLE_RATE_176_4KHZ == audio->rate))
+		else if (audio->rate == 44100 || audio->rate == 88200 ||
+			 audio->rate == 176400)
 			select = ACR_44;
 		else /* default to 32k */
 			select = ACR_32;
@@ -204,7 +129,6 @@ int msm_hdmi_bridge_audio_prepare(struct drm_connector *connector,
 {
 	struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge);
 	struct hdmi *hdmi = hdmi_bridge->hdmi;
-	unsigned int rate;
 	int ret;
 
 	drm_dbg_driver(bridge->dev, "%u Hz, %d bit, %d channels\n",
@@ -214,25 +138,12 @@ int msm_hdmi_bridge_audio_prepare(struct drm_connector *connector,
 
 	switch (params->sample_rate) {
 	case 32000:
-		rate = MSM_HDMI_SAMPLE_RATE_32KHZ;
-		break;
 	case 44100:
-		rate = MSM_HDMI_SAMPLE_RATE_44_1KHZ;
-		break;
 	case 48000:
-		rate = MSM_HDMI_SAMPLE_RATE_48KHZ;
-		break;
 	case 88200:
-		rate = MSM_HDMI_SAMPLE_RATE_88_2KHZ;
-		break;
 	case 96000:
-		rate = MSM_HDMI_SAMPLE_RATE_96KHZ;
-		break;
 	case 176400:
-		rate = MSM_HDMI_SAMPLE_RATE_176_4KHZ;
-		break;
 	case 192000:
-		rate = MSM_HDMI_SAMPLE_RATE_192KHZ;
 		break;
 	default:
 		drm_err(bridge->dev, "rate[%d] not supported!\n",
@@ -245,7 +156,7 @@ int msm_hdmi_bridge_audio_prepare(struct drm_connector *connector,
 	if (ret)
 		return ret;
 
-	hdmi->audio.rate = rate;
+	hdmi->audio.rate = params->sample_rate;
 	hdmi->audio.channels = params->cea.channels;
 	hdmi->audio.enabled = true;
 

-- 
2.39.5

Re: [PATCH 2/4] drm/msm/hdmi: use new helper for ACR tables

[kernel test robot]

Hi Dmitry,

kernel test robot noticed the following build errors:

[auto build test ERROR on 0a2f889128969dab41861b6e40111aa03dc57014]

url:    https://github.com/intel-lab-lkp/linux/commits/Dmitry-Baryshkov/drm-display-hdmi-provide-central-data-authority-for-ACR-params/20250309-161610
base:   0a2f889128969dab41861b6e40111aa03dc57014
patch link:    https://lore.kernel.org/r/20250309-drm-hdmi-acr-v1-2-bb9c242f4d4b%40linaro.org
patch subject: [PATCH 2/4] drm/msm/hdmi: use new helper for ACR tables
config: arm-randconfig-004-20250310 (https://download.01.org/0day-ci/archive/20250310/202503100745.KWEAWjFD-lkp@intel.com/config)
compiler: clang version 21.0.0git (https://github.com/llvm/llvm-project e15545cad8297ec7555f26e5ae74a9f0511203e7)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250310/202503100745.KWEAWjFD-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202503100745.KWEAWjFD-lkp@intel.com/

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> ERROR: modpost: "drm_hdmi_acr_get_n_cts" [drivers/gpu/drm/msm/msm.ko] undefined!

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH 3/4] drm/vc4: use new helper to get ACR values

[Dmitry Baryshkov]

From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Use drm_hdmi_acr_get_n_cts() helper instead of calculating N and CTS
values in the VC4 driver.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/vc4/vc4_hdmi.c | 10 +++-------
 drivers/gpu/drm/vc4/vc4_hdmi.h |  7 +++++++
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 37238a12baa58a06a5d6f40d1ab64abc7fac60d7..f24bcc2f3a2ac39aaea061b809940978341472f4 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -1637,6 +1637,7 @@ static void vc4_hdmi_encoder_atomic_mode_set(struct drm_encoder *encoder,
 		      &crtc_state->adjusted_mode);
 	vc4_hdmi->output_bpc = conn_state->hdmi.output_bpc;
 	vc4_hdmi->output_format = conn_state->hdmi.output_format;
+	vc4_hdmi->tmds_char_rate = conn_state->hdmi.tmds_char_rate;
 	mutex_unlock(&vc4_hdmi->mutex);
 }
 
@@ -1829,17 +1830,12 @@ static void vc4_hdmi_audio_set_mai_clock(struct vc4_hdmi *vc4_hdmi,
 
 static void vc4_hdmi_set_n_cts(struct vc4_hdmi *vc4_hdmi, unsigned int samplerate)
 {
-	const struct drm_display_mode *mode = &vc4_hdmi->saved_adjusted_mode;
-	u32 n, cts;
-	u64 tmp;
+	unsigned int n, cts;
 
 	lockdep_assert_held(&vc4_hdmi->mutex);
 	lockdep_assert_held(&vc4_hdmi->hw_lock);
 
-	n = 128 * samplerate / 1000;
-	tmp = (u64)(mode->clock * 1000) * n;
-	do_div(tmp, 128 * samplerate);
-	cts = tmp;
+	drm_hdmi_acr_get_n_cts(vc4_hdmi->tmds_char_rate, samplerate, &n, &cts);
 
 	HDMI_WRITE(HDMI_CRP_CFG,
 		   VC4_HDMI_CRP_CFG_EXTERNAL_CTS_EN |
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h
index e3d989ca302b72533c374dfa3fd0d5bd7fe64a82..0a775dbfe99d45521f3d0a2016555aefa81d7934 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.h
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.h
@@ -211,6 +211,13 @@ struct vc4_hdmi {
 	 * KMS hooks. Protected by @mutex.
 	 */
 	enum hdmi_colorspace output_format;
+
+	/**
+	 * @tmds_char_rate: Copy of
+	 * @drm_connector_state.hdmi.tmds_char_rate for use outside of
+	 * KMS hooks. Protected by @mutex.
+	 */
+	unsigned long long tmds_char_rate;
 };
 
 #define connector_to_vc4_hdmi(_connector)				\

-- 
2.39.5

Re: [PATCH 3/4] drm/vc4: use new helper to get ACR values

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2469 bytes --]

On Sun, Mar 09, 2025 at 10:13:58AM +0200, Dmitry Baryshkov wrote:
> From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> 
> Use drm_hdmi_acr_get_n_cts() helper instead of calculating N and CTS
> values in the VC4 driver.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/gpu/drm/vc4/vc4_hdmi.c | 10 +++-------
>  drivers/gpu/drm/vc4/vc4_hdmi.h |  7 +++++++
>  2 files changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index 37238a12baa58a06a5d6f40d1ab64abc7fac60d7..f24bcc2f3a2ac39aaea061b809940978341472f4 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -1637,6 +1637,7 @@ static void vc4_hdmi_encoder_atomic_mode_set(struct drm_encoder *encoder,
>  		      &crtc_state->adjusted_mode);
>  	vc4_hdmi->output_bpc = conn_state->hdmi.output_bpc;
>  	vc4_hdmi->output_format = conn_state->hdmi.output_format;
> +	vc4_hdmi->tmds_char_rate = conn_state->hdmi.tmds_char_rate;
>  	mutex_unlock(&vc4_hdmi->mutex);
>  }
>  
> @@ -1829,17 +1830,12 @@ static void vc4_hdmi_audio_set_mai_clock(struct vc4_hdmi *vc4_hdmi,
>  
>  static void vc4_hdmi_set_n_cts(struct vc4_hdmi *vc4_hdmi, unsigned int samplerate)
>  {
> -	const struct drm_display_mode *mode = &vc4_hdmi->saved_adjusted_mode;
> -	u32 n, cts;
> -	u64 tmp;
> +	unsigned int n, cts;
>  
>  	lockdep_assert_held(&vc4_hdmi->mutex);
>  	lockdep_assert_held(&vc4_hdmi->hw_lock);
>  
> -	n = 128 * samplerate / 1000;
> -	tmp = (u64)(mode->clock * 1000) * n;
> -	do_div(tmp, 128 * samplerate);
> -	cts = tmp;
> +	drm_hdmi_acr_get_n_cts(vc4_hdmi->tmds_char_rate, samplerate, &n, &cts);
>  
>  	HDMI_WRITE(HDMI_CRP_CFG,
>  		   VC4_HDMI_CRP_CFG_EXTERNAL_CTS_EN |
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h
> index e3d989ca302b72533c374dfa3fd0d5bd7fe64a82..0a775dbfe99d45521f3d0a2016555aefa81d7934 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.h
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.h
> @@ -211,6 +211,13 @@ struct vc4_hdmi {
>  	 * KMS hooks. Protected by @mutex.
>  	 */
>  	enum hdmi_colorspace output_format;
> +
> +	/**
> +	 * @tmds_char_rate: Copy of
> +	 * @drm_connector_state.hdmi.tmds_char_rate for use outside of
> +	 * KMS hooks. Protected by @mutex.
> +	 */
> +	unsigned long long tmds_char_rate;
>  };

This should be in drm_connector_hdmi if it's useful

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 3/4] drm/vc4: use new helper to get ACR values

[Dmitry Baryshkov]

On Mon, Mar 10, 2025 at 03:51:53PM +0100, Maxime Ripard wrote:
> On Sun, Mar 09, 2025 at 10:13:58AM +0200, Dmitry Baryshkov wrote:
> > From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > 
> > Use drm_hdmi_acr_get_n_cts() helper instead of calculating N and CTS
> > values in the VC4 driver.
> > 
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >  drivers/gpu/drm/vc4/vc4_hdmi.c | 10 +++-------
> >  drivers/gpu/drm/vc4/vc4_hdmi.h |  7 +++++++
> >  2 files changed, 10 insertions(+), 7 deletions(-)
> > 

> > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h
> > index e3d989ca302b72533c374dfa3fd0d5bd7fe64a82..0a775dbfe99d45521f3d0a2016555aefa81d7934 100644
> > --- a/drivers/gpu/drm/vc4/vc4_hdmi.h
> > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.h
> > @@ -211,6 +211,13 @@ struct vc4_hdmi {
> >  	 * KMS hooks. Protected by @mutex.
> >  	 */
> >  	enum hdmi_colorspace output_format;
> > +
> > +	/**
> > +	 * @tmds_char_rate: Copy of
> > +	 * @drm_connector_state.hdmi.tmds_char_rate for use outside of
> > +	 * KMS hooks. Protected by @mutex.
> > +	 */
> > +	unsigned long long tmds_char_rate;
> >  };
> 
> This should be in drm_connector_hdmi if it's useful

That would mean bringing the state to a non-state structure on the
framework level. Is it fine from your POV? Is it also fine to use
drm_connector.mutex for protecting this? Or should we be using something
like drm_connector_hdmi.infoframes.mutex (maybe after moving it from
.infoframes to the top level)?

-- 
With best wishes
Dmitry

Re: [PATCH 3/4] drm/vc4: use new helper to get ACR values

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1905 bytes --]

On Mon, Mar 10, 2025 at 10:18:04PM +0200, Dmitry Baryshkov wrote:
> On Mon, Mar 10, 2025 at 03:51:53PM +0100, Maxime Ripard wrote:
> > On Sun, Mar 09, 2025 at 10:13:58AM +0200, Dmitry Baryshkov wrote:
> > > From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > 
> > > Use drm_hdmi_acr_get_n_cts() helper instead of calculating N and CTS
> > > values in the VC4 driver.
> > > 
> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > ---
> > >  drivers/gpu/drm/vc4/vc4_hdmi.c | 10 +++-------
> > >  drivers/gpu/drm/vc4/vc4_hdmi.h |  7 +++++++
> > >  2 files changed, 10 insertions(+), 7 deletions(-)
> > > 
> 
> > > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h
> > > index e3d989ca302b72533c374dfa3fd0d5bd7fe64a82..0a775dbfe99d45521f3d0a2016555aefa81d7934 100644
> > > --- a/drivers/gpu/drm/vc4/vc4_hdmi.h
> > > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.h
> > > @@ -211,6 +211,13 @@ struct vc4_hdmi {
> > >  	 * KMS hooks. Protected by @mutex.
> > >  	 */
> > >  	enum hdmi_colorspace output_format;
> > > +
> > > +	/**
> > > +	 * @tmds_char_rate: Copy of
> > > +	 * @drm_connector_state.hdmi.tmds_char_rate for use outside of
> > > +	 * KMS hooks. Protected by @mutex.
> > > +	 */
> > > +	unsigned long long tmds_char_rate;
> > >  };
> > 
> > This should be in drm_connector_hdmi if it's useful
> 
> That would mean bringing the state to a non-state structure on the
> framework level. Is it fine from your POV?

Sorry, I'm changing my mind a little bit, but it's pretty much the same
case than for accessing the infoframes from debugfs: we want to get some
information stored in the state from outside of KMS.

What we did for the infoframes is that we're actually just taking the
connection_mutex from the DRM device and access the drm_connector->state
pointer.

I guess it would also work for ALSA?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 3/4] drm/vc4: use new helper to get ACR values

[Dmitry Baryshkov]

On Tue, Mar 11, 2025 at 09:07:10AM +0100, Maxime Ripard wrote:
> On Mon, Mar 10, 2025 at 10:18:04PM +0200, Dmitry Baryshkov wrote:
> > On Mon, Mar 10, 2025 at 03:51:53PM +0100, Maxime Ripard wrote:
> > > On Sun, Mar 09, 2025 at 10:13:58AM +0200, Dmitry Baryshkov wrote:
> > > > From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > 
> > > > Use drm_hdmi_acr_get_n_cts() helper instead of calculating N and CTS
> > > > values in the VC4 driver.
> > > > 
> > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > ---
> > > >  drivers/gpu/drm/vc4/vc4_hdmi.c | 10 +++-------
> > > >  drivers/gpu/drm/vc4/vc4_hdmi.h |  7 +++++++
> > > >  2 files changed, 10 insertions(+), 7 deletions(-)
> > > > 
> > 
> > > > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h
> > > > index e3d989ca302b72533c374dfa3fd0d5bd7fe64a82..0a775dbfe99d45521f3d0a2016555aefa81d7934 100644
> > > > --- a/drivers/gpu/drm/vc4/vc4_hdmi.h
> > > > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.h
> > > > @@ -211,6 +211,13 @@ struct vc4_hdmi {
> > > >  	 * KMS hooks. Protected by @mutex.
> > > >  	 */
> > > >  	enum hdmi_colorspace output_format;
> > > > +
> > > > +	/**
> > > > +	 * @tmds_char_rate: Copy of
> > > > +	 * @drm_connector_state.hdmi.tmds_char_rate for use outside of
> > > > +	 * KMS hooks. Protected by @mutex.
> > > > +	 */
> > > > +	unsigned long long tmds_char_rate;
> > > >  };
> > > 
> > > This should be in drm_connector_hdmi if it's useful
> > 
> > That would mean bringing the state to a non-state structure on the
> > framework level. Is it fine from your POV?
> 
> Sorry, I'm changing my mind a little bit, but it's pretty much the same
> case than for accessing the infoframes from debugfs: we want to get some
> information stored in the state from outside of KMS.
> 
> What we did for the infoframes is that we're actually just taking the
> connection_mutex from the DRM device and access the drm_connector->state
> pointer.
> 
> I guess it would also work for ALSA?

I'd really prefer to follow the drm_connector.infoframes.audio. It makes
sense to group all ALSA-related functionality together. Maybe I should
refactor it to:

struct drm_connector {
    struct {
	struct mutex lock;
	struct drm_connector_hdmi_infoframe audio_infoframe;
	unsigned long long tmds_char_rate;
    } audio;
};

WDYT? If that doesn't sound appealing, I'll go the connetion_mutex and
drm_connector_state way.

-- 
With best wishes
Dmitry

Re: [PATCH 3/4] drm/vc4: use new helper to get ACR values

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2631 bytes --]

On Tue, Mar 11, 2025 at 06:28:50PM +0200, Dmitry Baryshkov wrote:
> On Tue, Mar 11, 2025 at 09:07:10AM +0100, Maxime Ripard wrote:
> > On Mon, Mar 10, 2025 at 10:18:04PM +0200, Dmitry Baryshkov wrote:
> > > On Mon, Mar 10, 2025 at 03:51:53PM +0100, Maxime Ripard wrote:
> > > > On Sun, Mar 09, 2025 at 10:13:58AM +0200, Dmitry Baryshkov wrote:
> > > > > From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > > 
> > > > > Use drm_hdmi_acr_get_n_cts() helper instead of calculating N and CTS
> > > > > values in the VC4 driver.
> > > > > 
> > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > > ---
> > > > >  drivers/gpu/drm/vc4/vc4_hdmi.c | 10 +++-------
> > > > >  drivers/gpu/drm/vc4/vc4_hdmi.h |  7 +++++++
> > > > >  2 files changed, 10 insertions(+), 7 deletions(-)
> > > > > 
> > > 
> > > > > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h
> > > > > index e3d989ca302b72533c374dfa3fd0d5bd7fe64a82..0a775dbfe99d45521f3d0a2016555aefa81d7934 100644
> > > > > --- a/drivers/gpu/drm/vc4/vc4_hdmi.h
> > > > > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.h
> > > > > @@ -211,6 +211,13 @@ struct vc4_hdmi {
> > > > >  	 * KMS hooks. Protected by @mutex.
> > > > >  	 */
> > > > >  	enum hdmi_colorspace output_format;
> > > > > +
> > > > > +	/**
> > > > > +	 * @tmds_char_rate: Copy of
> > > > > +	 * @drm_connector_state.hdmi.tmds_char_rate for use outside of
> > > > > +	 * KMS hooks. Protected by @mutex.
> > > > > +	 */
> > > > > +	unsigned long long tmds_char_rate;
> > > > >  };
> > > > 
> > > > This should be in drm_connector_hdmi if it's useful
> > > 
> > > That would mean bringing the state to a non-state structure on the
> > > framework level. Is it fine from your POV?
> > 
> > Sorry, I'm changing my mind a little bit, but it's pretty much the same
> > case than for accessing the infoframes from debugfs: we want to get some
> > information stored in the state from outside of KMS.
> > 
> > What we did for the infoframes is that we're actually just taking the
> > connection_mutex from the DRM device and access the drm_connector->state
> > pointer.
> > 
> > I guess it would also work for ALSA?
> 
> I'd really prefer to follow the drm_connector.infoframes.audio. It makes
> sense to group all ALSA-related functionality together. Maybe I should
> refactor it to:

That's the thing though: the tmds_char_rate has nothing to do with ALSA.
It's useful to derive the parameters, but KMS controls it, it's part of
its state, and that's where it belongs.

Just like any infoframe but the audio one.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH 3/4] drm/vc4: use new helper to get ACR values

[Dmitry Baryshkov]

On Fri, Mar 14, 2025 at 02:46:09PM +0100, Maxime Ripard wrote:
> On Tue, Mar 11, 2025 at 06:28:50PM +0200, Dmitry Baryshkov wrote:
> > On Tue, Mar 11, 2025 at 09:07:10AM +0100, Maxime Ripard wrote:
> > > On Mon, Mar 10, 2025 at 10:18:04PM +0200, Dmitry Baryshkov wrote:
> > > > On Mon, Mar 10, 2025 at 03:51:53PM +0100, Maxime Ripard wrote:
> > > > > On Sun, Mar 09, 2025 at 10:13:58AM +0200, Dmitry Baryshkov wrote:
> > > > > > From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > > > 
> > > > > > Use drm_hdmi_acr_get_n_cts() helper instead of calculating N and CTS
> > > > > > values in the VC4 driver.
> > > > > > 
> > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > > > ---
> > > > > >  drivers/gpu/drm/vc4/vc4_hdmi.c | 10 +++-------
> > > > > >  drivers/gpu/drm/vc4/vc4_hdmi.h |  7 +++++++
> > > > > >  2 files changed, 10 insertions(+), 7 deletions(-)
> > > > > > 
> > > > 
> > > > > > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h
> > > > > > index e3d989ca302b72533c374dfa3fd0d5bd7fe64a82..0a775dbfe99d45521f3d0a2016555aefa81d7934 100644
> > > > > > --- a/drivers/gpu/drm/vc4/vc4_hdmi.h
> > > > > > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.h
> > > > > > @@ -211,6 +211,13 @@ struct vc4_hdmi {
> > > > > >  	 * KMS hooks. Protected by @mutex.
> > > > > >  	 */
> > > > > >  	enum hdmi_colorspace output_format;
> > > > > > +
> > > > > > +	/**
> > > > > > +	 * @tmds_char_rate: Copy of
> > > > > > +	 * @drm_connector_state.hdmi.tmds_char_rate for use outside of
> > > > > > +	 * KMS hooks. Protected by @mutex.
> > > > > > +	 */
> > > > > > +	unsigned long long tmds_char_rate;
> > > > > >  };
> > > > > 
> > > > > This should be in drm_connector_hdmi if it's useful
> > > > 
> > > > That would mean bringing the state to a non-state structure on the
> > > > framework level. Is it fine from your POV?
> > > 
> > > Sorry, I'm changing my mind a little bit, but it's pretty much the same
> > > case than for accessing the infoframes from debugfs: we want to get some
> > > information stored in the state from outside of KMS.
> > > 
> > > What we did for the infoframes is that we're actually just taking the
> > > connection_mutex from the DRM device and access the drm_connector->state
> > > pointer.
> > > 
> > > I guess it would also work for ALSA?
> > 
> > I'd really prefer to follow the drm_connector.infoframes.audio. It makes
> > sense to group all ALSA-related functionality together. Maybe I should
> > refactor it to:
> 
> That's the thing though: the tmds_char_rate has nothing to do with ALSA.
> It's useful to derive the parameters, but KMS controls it, it's part of
> its state, and that's where it belongs.
> 
> Just like any infoframe but the audio one.

Ack, I'll take a look.


-- 
With best wishes
Dmitry

[PATCH 4/4] drm: bridge: dw-hdmi: use new helper to get ACR values

[Dmitry Baryshkov]

From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Use drm_hdmi_acr_get_n_cts() helper instead of calculating N and CTS
values in the DW-HDMI driver.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 90 +++----------------------------
 1 file changed, 8 insertions(+), 82 deletions(-)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index 0890add5f7070f13fefad923526e92f516f06764..b8775e677233ca96c2d4a06fb5697aa3c0bd45c3 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -558,68 +558,6 @@ static void hdmi_set_cts_n(struct dw_hdmi *hdmi, unsigned int cts,
 	hdmi_writeb(hdmi, n & 0xff, HDMI_AUD_N1);
 }
 
-static unsigned int hdmi_compute_n(unsigned int freq, unsigned long pixel_clk)
-{
-	unsigned int n = (128 * freq) / 1000;
-	unsigned int mult = 1;
-
-	while (freq > 48000) {
-		mult *= 2;
-		freq /= 2;
-	}
-
-	switch (freq) {
-	case 32000:
-		if (pixel_clk == 25175000)
-			n = 4576;
-		else if (pixel_clk == 27027000)
-			n = 4096;
-		else if (pixel_clk == 74176000 || pixel_clk == 148352000)
-			n = 11648;
-		else if (pixel_clk == 297000000)
-			n = 3072;
-		else
-			n = 4096;
-		n *= mult;
-		break;
-
-	case 44100:
-		if (pixel_clk == 25175000)
-			n = 7007;
-		else if (pixel_clk == 74176000)
-			n = 17836;
-		else if (pixel_clk == 148352000)
-			n = 8918;
-		else if (pixel_clk == 297000000)
-			n = 4704;
-		else
-			n = 6272;
-		n *= mult;
-		break;
-
-	case 48000:
-		if (pixel_clk == 25175000)
-			n = 6864;
-		else if (pixel_clk == 27027000)
-			n = 6144;
-		else if (pixel_clk == 74176000)
-			n = 11648;
-		else if (pixel_clk == 148352000)
-			n = 5824;
-		else if (pixel_clk == 297000000)
-			n = 5120;
-		else
-			n = 6144;
-		n *= mult;
-		break;
-
-	default:
-		break;
-	}
-
-	return n;
-}
-
 /*
  * When transmitting IEC60958 linear PCM audio, these registers allow to
  * configure the channel status information of all the channel status
@@ -646,32 +584,20 @@ static void hdmi_set_clk_regenerator(struct dw_hdmi *hdmi,
 	unsigned long ftdms = pixel_clk;
 	unsigned int n, cts;
 	u8 config3;
-	u64 tmp;
 
-	n = hdmi_compute_n(sample_rate, pixel_clk);
+	drm_hdmi_acr_get_n_cts(ftdms, sample_rate, &n, &cts);
 
 	config3 = hdmi_readb(hdmi, HDMI_CONFIG3_ID);
 
 	/* Compute CTS when using internal AHB audio or General Parallel audio*/
-	if ((config3 & HDMI_CONFIG3_AHBAUDDMA) || (config3 & HDMI_CONFIG3_GPAUD)) {
-		/*
-		 * Compute the CTS value from the N value.  Note that CTS and N
-		 * can be up to 20 bits in total, so we need 64-bit math.  Also
-		 * note that our TDMS clock is not fully accurate; it is
-		 * accurate to kHz.  This can introduce an unnecessary remainder
-		 * in the calculation below, so we don't try to warn about that.
-		 */
-		tmp = (u64)ftdms * n;
-		do_div(tmp, 128 * sample_rate);
-		cts = tmp;
-
-		dev_dbg(hdmi->dev, "%s: fs=%uHz ftdms=%lu.%03luMHz N=%d cts=%d\n",
-			__func__, sample_rate,
-			ftdms / 1000000, (ftdms / 1000) % 1000,
-			n, cts);
-	} else {
+	if (!(config3 & HDMI_CONFIG3_AHBAUDDMA) &&
+	    !(config3 & HDMI_CONFIG3_GPAUD))
 		cts = 0;
-	}
+
+	dev_dbg(hdmi->dev, "%s: fs=%uHz ftdms=%lu.%03luMHz N=%d cts=%d\n",
+		__func__, sample_rate,
+		ftdms / 1000000, (ftdms / 1000) % 1000,
+		n, cts);
 
 	spin_lock_irq(&hdmi->audio_lock);
 	hdmi->audio_n = n;

-- 
2.39.5

Re: [PATCH 4/4] drm: bridge: dw-hdmi: use new helper to get ACR values

[kernel test robot]

Hi Dmitry,

kernel test robot noticed the following build errors:

[auto build test ERROR on 0a2f889128969dab41861b6e40111aa03dc57014]

url:    https://github.com/intel-lab-lkp/linux/commits/Dmitry-Baryshkov/drm-display-hdmi-provide-central-data-authority-for-ACR-params/20250309-161610
base:   0a2f889128969dab41861b6e40111aa03dc57014
patch link:    https://lore.kernel.org/r/20250309-drm-hdmi-acr-v1-4-bb9c242f4d4b%40linaro.org
patch subject: [PATCH 4/4] drm: bridge: dw-hdmi: use new helper to get ACR values
config: arm64-randconfig-002-20250310 (https://download.01.org/0day-ci/archive/20250310/202503100501.SlwYOb9U-lkp@intel.com/config)
compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250310/202503100501.SlwYOb9U-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202503100501.SlwYOb9U-lkp@intel.com/

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> ERROR: modpost: "drm_hdmi_acr_get_n_cts" [drivers/gpu/drm/bridge/synopsys/dw-hdmi.ko] undefined!

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki