public inbox for drm-ai-reviews@public-inbox.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH v2] drm/bridge: dw-hdmi-qp: compute audio CTS from N when not in TMDS table
@ 2026-05-20  9:10 Simon Wright
  2026-05-20  9:38 ` Jonas Karlman
  0 siblings, 1 reply; 5+ messages in thread
From: Simon Wright @ 2026-05-20  9:10 UTC (permalink / raw)
  To: Cristian Ciocaltea
  Cc: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
	Jonas Karlman, Jernej Skrabec, Luca Ceresoli, Maarten Lankhorst,
	Maxime Ripard, Thomas Zimmermann, David Airlie, Simona Vetter,
	Heiko Stuebner, Andy Yan, Dmitry Baryshkov, Algea Cao, dri-devel,
	linux-rockchip, linux-kernel

dw_hdmi_qp_find_cts() returns 0 for any TMDS character rate not
present in common_tmds_cts_table[], which terminates at 148.5 MHz.
In that case dw_hdmi_qp_set_sample_rate() calls dw_hdmi_qp_set_cts_n()
with cts == 0, which leaves AUDPKT_ACR_CTS_OVR_EN clear and falls
back to the controller's internal CTS auto-measurement.

On at least the RK3576 hdptx integration, the auto-measure path
produces incorrect ACR timing on the wire at TMDS rates above the
table's coverage.  Strict HDMI sinks that cross-check the ACR CTS
against the AVI/GCP and the actual TMDS clock then mute audio.

The issue is reproducible at any rate not present in the table, and
is not specific to HDMI 2.x: 1920x1080@60 with 10-bit deep colour
(185.625 MHz, HDMI 1.4) is affected, as is 3840x2160@60 8-bit
(594 MHz, HDMI 2.0).

The driver already has the symmetric machinery for the N-table-miss
case: dw_hdmi_qp_compute_n() falls back to a dynamic search via
dw_hdmi_qp_audio_math_diff() ((pixel_clk * n) / (128 * freq)) when
no table entry matches.  The CTS path lacks the equivalent fallback.

Compute CTS inline in dw_hdmi_qp_set_sample_rate() from N per the
HDMI spec (CTS = TMDS * N / (128 * Fs)) when find_cts() returns 0.
The standard override path then supplies the correct value on the
wire instead of falling through to auto-measure.

The legacy DesignWare HDMI driver (drivers/gpu/drm/bridge/synopsys/
dw-hdmi.c, hdmi_set_clk_regenerator()) computes CTS from N via the
same formula when AHB or GP audio is active, so the pattern is
already established within the family.

Tested on R76S (RK3576) on Armbian-edge mainline 7.0.1 with Cristian
Ciocaltea's hdptx-clk-fixes v1 series applied, against four sinks
at four TMDS rates spanning HDMI 1.4 and HDMI 2.0:

  TMDS         Mode                In table?  G3     C4     TCL    Kogan
  148.5 MHz    1080p60  8-bit      yes        audio  audio  audio  audio
  185.625 MHz  1080p60 10-bit      no         audio  audio  audio  audio
  297 MHz      3840p30  8-bit      no         audio  audio  audio  audio
  594 MHz      3840p60  8-bit      no         audio  N/A    audio  audio

Without this fix, all four sinks mute audio at the rates marked "no"
above.  With the fix, audio plays cleanly.  The 148.5 MHz row is a
regression check confirming the in-table path is unchanged.

The LG C4 OLED's CTA-861 SVDs do not advertise 3840x2160@60 over
TMDS (it is signalled FRL-only in this model's EDID), so that
specific case is untestable on the C4 over the TMDS path.  The
same audio-path code is exercised on the C4 at 297 MHz and behaves
identically to the G3.

More-permissive sinks (older HDMI 2.0 TVs tested informally) play
audio at all rates with or without the fix because they do not
strictly cross-check ACR CTS against the TMDS clock.

Reported-by: Simon Wright <simon@symple.nz>
Closes: https://lore.kernel.org/linux-rockchip/ME3P282MB21960D9D68BFF520316BDFCEA83E2@ME3P282MB2196.AUSP282.PROD.OUTLOOK.COM/
Suggested-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
Tested-by: Simon Wright <simon@symple.nz>
Assisted-by: Claude:claude-opus-4-7
Signed-off-by: Simon Wright <simon@symple.nz>
---
Changes in v2:
  - Resend after v1 was mailer-mangled and unapplyable (reported by
    Luca Ceresoli).
  - Body split into shorter paragraphs for readability per Luca's
    review.
  - Original LG G3 report (linux-rockchip 070633) moved from inline
    prose to a Closes: trailer paired with Reported-by:, per
    Documentation/process/submitting-patches.rst.
  - Add Assisted-by: trailer per
    Documentation/process/coding-assistants.rst.
  - No code or test-result changes.

v1: https://lore.kernel.org/linux-rockchip/92aa4191-2a2a-41e3-badb-c0a5b1fbb957@symple.nz/

 drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
index 0dbb1274360..b7203787057 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
@@ -461,6 +461,23 @@ static void dw_hdmi_qp_set_sample_rate(struct dw_hdmi_qp *hdmi, unsigned long lo
 	n = dw_hdmi_qp_find_n(hdmi, tmds_char_rate, sample_rate);
 	cts = dw_hdmi_qp_find_cts(hdmi, tmds_char_rate, sample_rate);
 
+	/*
+	 * When no CTS table entry exists for the given TMDS rate, compute
+	 * CTS from N rather than letting the hardware auto-measure.  The
+	 * auto-CTS circuit produces incorrect audio timing at out-of-table
+	 * rates (e.g. 185.625 MHz, 297 MHz, 594 MHz), causing strict HDMI
+	 * sinks to mute audio.  Computed CTS = (TMDS * N) / (128 * Fs) per
+	 * HDMI spec; the standard override path then supplies it on the
+	 * wire.  Mirrors hdmi_set_clk_regenerator() in the legacy dw-hdmi
+	 * driver.
+	 */
+	if (!cts && n) {
+		u64 computed = (u64)tmds_char_rate * n;
+
+		do_div(computed, 128ULL * sample_rate);
+		cts = (unsigned int)computed;
+	}
+
 	dw_hdmi_qp_set_cts_n(hdmi, cts, n);
 }
 

base-commit: c1079aebb4de218caa86c44f9a53700d1a582683
-- 
2.53.0

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] drm/bridge: dw-hdmi-qp: compute audio CTS from N when not in TMDS table
  2026-05-20  9:10 [PATCH v2] drm/bridge: dw-hdmi-qp: compute audio CTS from N when not in TMDS table Simon Wright
@ 2026-05-20  9:38 ` Jonas Karlman
  2026-05-21  7:44   ` Simon Wright
                     ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Jonas Karlman @ 2026-05-20  9:38 UTC (permalink / raw)
  To: Simon Wright, Cristian Ciocaltea
  Cc: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
	Jernej Skrabec, Luca Ceresoli, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Simona Vetter, Heiko Stuebner,
	Andy Yan, Dmitry Baryshkov, Algea Cao, dri-devel, linux-rockchip,
	linux-kernel

Hi Simon,

On 5/20/2026 11:10 AM, Simon Wright wrote:
> dw_hdmi_qp_find_cts() returns 0 for any TMDS character rate not
> present in common_tmds_cts_table[], which terminates at 148.5 MHz.
> In that case dw_hdmi_qp_set_sample_rate() calls dw_hdmi_qp_set_cts_n()
> with cts == 0, which leaves AUDPKT_ACR_CTS_OVR_EN clear and falls
> back to the controller's internal CTS auto-measurement.
> 
> On at least the RK3576 hdptx integration, the auto-measure path
> produces incorrect ACR timing on the wire at TMDS rates above the
> table's coverage.  Strict HDMI sinks that cross-check the ACR CTS
> against the AVI/GCP and the actual TMDS clock then mute audio.
> 
> The issue is reproducible at any rate not present in the table, and
> is not specific to HDMI 2.x: 1920x1080@60 with 10-bit deep colour
> (185.625 MHz, HDMI 1.4) is affected, as is 3840x2160@60 8-bit
> (594 MHz, HDMI 2.0).
> 
> The driver already has the symmetric machinery for the N-table-miss
> case: dw_hdmi_qp_compute_n() falls back to a dynamic search via
> dw_hdmi_qp_audio_math_diff() ((pixel_clk * n) / (128 * freq)) when
> no table entry matches.  The CTS path lacks the equivalent fallback.
> 
> Compute CTS inline in dw_hdmi_qp_set_sample_rate() from N per the
> HDMI spec (CTS = TMDS * N / (128 * Fs)) when find_cts() returns 0.
> The standard override path then supplies the correct value on the
> wire instead of falling through to auto-measure.
> 
> The legacy DesignWare HDMI driver (drivers/gpu/drm/bridge/synopsys/
> dw-hdmi.c, hdmi_set_clk_regenerator()) computes CTS from N via the
> same formula when AHB or GP audio is active, so the pattern is
> already established within the family.
> 
> Tested on R76S (RK3576) on Armbian-edge mainline 7.0.1 with Cristian
> Ciocaltea's hdptx-clk-fixes v1 series applied, against four sinks
> at four TMDS rates spanning HDMI 1.4 and HDMI 2.0:
> 
>   TMDS         Mode                In table?  G3     C4     TCL    Kogan
>   148.5 MHz    1080p60  8-bit      yes        audio  audio  audio  audio
>   185.625 MHz  1080p60 10-bit      no         audio  audio  audio  audio
>   297 MHz      3840p30  8-bit      no         audio  audio  audio  audio
>   594 MHz      3840p60  8-bit      no         audio  N/A    audio  audio
> 
> Without this fix, all four sinks mute audio at the rates marked "no"
> above.  With the fix, audio plays cleanly.  The 148.5 MHz row is a
> regression check confirming the in-table path is unchanged.
> 
> The LG C4 OLED's CTA-861 SVDs do not advertise 3840x2160@60 over
> TMDS (it is signalled FRL-only in this model's EDID), so that
> specific case is untestable on the C4 over the TMDS path.  The
> same audio-path code is exercised on the C4 at 297 MHz and behaves
> identically to the G3.
> 
> More-permissive sinks (older HDMI 2.0 TVs tested informally) play
> audio at all rates with or without the fix because they do not
> strictly cross-check ACR CTS against the TMDS clock.
> 
> Reported-by: Simon Wright <simon@symple.nz>
> Closes: https://lore.kernel.org/linux-rockchip/ME3P282MB21960D9D68BFF520316BDFCEA83E2@ME3P282MB2196.AUSP282.PROD.OUTLOOK.COM/
> Suggested-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> Tested-by: Simon Wright <simon@symple.nz>
> Assisted-by: Claude:claude-opus-4-7
> Signed-off-by: Simon Wright <simon@symple.nz>
> ---
> Changes in v2:
>   - Resend after v1 was mailer-mangled and unapplyable (reported by
>     Luca Ceresoli).
>   - Body split into shorter paragraphs for readability per Luca's
>     review.
>   - Original LG G3 report (linux-rockchip 070633) moved from inline
>     prose to a Closes: trailer paired with Reported-by:, per
>     Documentation/process/submitting-patches.rst.
>   - Add Assisted-by: trailer per
>     Documentation/process/coding-assistants.rst.
>   - No code or test-result changes.
> 
> v1: https://lore.kernel.org/linux-rockchip/92aa4191-2a2a-41e3-badb-c0a5b1fbb957@symple.nz/
> 
>  drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
> index 0dbb1274360..b7203787057 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
> @@ -461,6 +461,23 @@ static void dw_hdmi_qp_set_sample_rate(struct dw_hdmi_qp *hdmi, unsigned long lo
>  	n = dw_hdmi_qp_find_n(hdmi, tmds_char_rate, sample_rate);
>  	cts = dw_hdmi_qp_find_cts(hdmi, tmds_char_rate, sample_rate);
>  
> +	/*
> +	 * When no CTS table entry exists for the given TMDS rate, compute
> +	 * CTS from N rather than letting the hardware auto-measure.  The
> +	 * auto-CTS circuit produces incorrect audio timing at out-of-table
> +	 * rates (e.g. 185.625 MHz, 297 MHz, 594 MHz), causing strict HDMI
> +	 * sinks to mute audio.  Computed CTS = (TMDS * N) / (128 * Fs) per
> +	 * HDMI spec; the standard override path then supplies it on the
> +	 * wire.  Mirrors hdmi_set_clk_regenerator() in the legacy dw-hdmi
> +	 * driver.
> +	 */
> +	if (!cts && n) {
> +		u64 computed = (u64)tmds_char_rate * n;
> +
> +		do_div(computed, 128ULL * sample_rate);
> +		cts = (unsigned int)computed;
> +	}

Could this function instead be converted to use the helper
drm_hdmi_acr_get_n_cts() to lookup/calculate the n and cts value?

At first glance it looks to produce same/similar values as this driver
tries to do open-coded.

Regards,
Jonas

> +
>  	dw_hdmi_qp_set_cts_n(hdmi, cts, n);
>  }
>  
> 
> base-commit: c1079aebb4de218caa86c44f9a53700d1a582683


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] drm/bridge: dw-hdmi-qp: compute audio CTS from N when not in TMDS table
  2026-05-20  9:38 ` Jonas Karlman
@ 2026-05-21  7:44   ` Simon Wright
  2026-05-25 12:06   ` Claude review: " Claude Code Review Bot
  2026-05-25 12:06   ` Claude Code Review Bot
  2 siblings, 0 replies; 5+ messages in thread
From: Simon Wright @ 2026-05-21  7:44 UTC (permalink / raw)
  To: Jonas Karlman, Cristian Ciocaltea
  Cc: Andrzej Hajda, Neil Armstrong, Robert Foss, Laurent Pinchart,
	Jernej Skrabec, Luca Ceresoli, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Simona Vetter, Heiko Stuebner,
	Andy Yan, Dmitry Baryshkov, Algea Cao, dri-devel, linux-rockchip,
	linux-kernel

On 20/05/2026 9:38 pm, Jonas Karlman wrote:
> Could this function instead be converted to use the helper
> drm_hdmi_acr_get_n_cts() to lookup/calculate the n and cts value?

Done in v3:

https://lore.kernel.org/linux-rockchip/86fcf349-0a7a-4618-9001-612371b0f71b@symple.nz/

Re-tested on the LG G3 OLED (the original reporting sink) at
148.5 / 185.625 / 297 / 594 MHz; audio plays at all four.  v3
also drops ~200 lines of open-coded N/CTS tables and search
functions that the helper makes redundant.

Simon

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Claude review: Re: [PATCH v2] drm/bridge: dw-hdmi-qp: compute audio CTS from N when not in TMDS table
  2026-05-20  9:38 ` Jonas Karlman
  2026-05-21  7:44   ` Simon Wright
@ 2026-05-25 12:06   ` Claude Code Review Bot
  2026-05-25 12:06   ` Claude Code Review Bot
  2 siblings, 0 replies; 5+ messages in thread
From: Claude Code Review Bot @ 2026-05-25 12:06 UTC (permalink / raw)
  To: dri-devel-reviews

Overall Series Review

Subject: Re: [PATCH v2] drm/bridge: dw-hdmi-qp: compute audio CTS from N when not in TMDS table
Author: Jonas Karlman <jonas@kwiboo.se>
Patches: 3
Reviewed: 2026-05-25T22:06:43.782096

---

This is a single, well-motivated patch that fixes an audio mute issue on HDMI sinks when using TMDS character rates not covered by the static CTS lookup table (`common_tmds_cts_table[]`). The commit message is excellent — clear problem statement, root cause analysis, the fix rationale, precedent in the legacy driver, and comprehensive test results across multiple sinks and rates.

The code change is small, correct, and follows an established pattern from the legacy `dw-hdmi.c` driver. The formula `CTS = (TMDS * N) / (128 * Fs)` is straight from the HDMI specification.

**Verdict: This patch looks good and is ready to apply.** One minor observation below, but nothing blocking.

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Claude review: Re: [PATCH v2] drm/bridge: dw-hdmi-qp: compute audio CTS from N when not in TMDS table
  2026-05-20  9:38 ` Jonas Karlman
  2026-05-21  7:44   ` Simon Wright
  2026-05-25 12:06   ` Claude review: " Claude Code Review Bot
@ 2026-05-25 12:06   ` Claude Code Review Bot
  2 siblings, 0 replies; 5+ messages in thread
From: Claude Code Review Bot @ 2026-05-25 12:06 UTC (permalink / raw)
  To: dri-devel-reviews

Patch Review

**Correctness of the formula:**

The computation mirrors the legacy driver at `dw-hdmi.c:673-675`:
```c
tmp = (u64)ftdms * n;
do_div(tmp, 128 * sample_rate);
cts = tmp;
```

The patch does the equivalent:
```c
u64 computed = (u64)tmds_char_rate * n;
do_div(computed, 128ULL * sample_rate);
cts = (unsigned int)computed;
```

This is correct. The `u64` cast prevents overflow since `tmds_char_rate` (up to ~600MHz) times `n` (up to ~10240) can exceed 32 bits.

**Guard condition:**

```c
if (!cts && n) {
```

This is the right check — only compute when `find_cts()` returned 0 (no table entry) and `n` is non-zero (so the division is meaningful). If `n` were somehow 0, the computation would produce 0 anyway, so the `n` guard is defensive but harmless.

**Minor observation — `dw_hdmi_qp_find_cts` can return `-ENOENT`:**

Looking at `dw_hdmi_qp_find_cts()` at line 338:
```c
default:
    return -ENOENT;
```

If an unsupported sample rate is passed (not 32k/44.1k/48k or their multiples), `find_cts()` returns `-ENOENT` (cast to `unsigned int`, a very large value). In that case `!cts` would be false, so the fallback wouldn't trigger — which is actually fine, since the CTS value would be garbage and the subsequent `dw_hdmi_qp_set_cts_n()` would enable CTS override with that garbage. However, this is a pre-existing bug in `dw_hdmi_qp_find_cts()` unrelated to this patch: the same problematic `-ENOENT` return exists for `dw_hdmi_qp_match_tmds_n_table()` too, and both pre-date this change. Not something this patch needs to fix.

**`128ULL` vs `128`:**

The patch uses `128ULL * sample_rate` in the divisor. The legacy driver uses `128 * sample_rate`. With `sample_rate` at most 192000, `128 * 192000 = 24,576,000`, well within 32-bit range, so the `ULL` suffix is unnecessary but harmless — it's slightly more defensive. Fine either way.

**Comment quality:**

The inline comment is longer than strictly necessary (9 lines), but given this is a non-obvious fallback path that compensates for hardware deficiency, the explanation of *why* auto-CTS fails and what the fix does is warranted. This is a case where the "why" truly is non-obvious.

**No issues found. The patch is correct and well-tested.**

---
Generated by Claude Code Patch Reviewer

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2026-05-25 12:06 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-05-20  9:10 [PATCH v2] drm/bridge: dw-hdmi-qp: compute audio CTS from N when not in TMDS table Simon Wright
2026-05-20  9:38 ` Jonas Karlman
2026-05-21  7:44   ` Simon Wright
2026-05-25 12:06   ` Claude review: " Claude Code Review Bot
2026-05-25 12:06   ` Claude Code Review Bot

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox