From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: drm/bridge: dw-hdmi-qp: Add HDMI 2.0 SCDC scrambling and high TMDS clock ratio support
Date: Tue, 28 Apr 2026 15:25:20 +1000
Message-ID: <review-patch5-20260426-dw-hdmi-qp-scramb-v5-5-d778e70c317b@collabora.com>
In-Reply-To: <20260426-dw-hdmi-qp-scramb-v5-5-d778e70c317b@collabora.com>
References: <20260426-dw-hdmi-qp-scramb-v5-0-d778e70c317b@collabora.com> <20260426-dw-hdmi-qp-scramb-v5-5-d778e70c317b@collabora.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

Patch Review

This is the main feature patch and the most complex. Several aspects to exa=
mine:

**Struct additions:**
```c
	struct drm_connector *curr_conn;
	struct delayed_work scramb_work;
	bool scramb_enabled;
```

**Concurrency concern with `curr_conn`:** The `curr_conn` pointer is set in=
 `atomic_enable` and cleared in `atomic_disable`. It's used without locking=
 in the delayed work item (`dw_hdmi_qp_scramb_work`) and in `detect_ctx`. T=
here's a potential race:

1. `scramb_work` fires and reads `hdmi->curr_conn`
2. Concurrently, `atomic_disable` runs and sets `hdmi->curr_conn =3D NULL`
3. The work dereferences a NULL or stale pointer

The `cancel_delayed_work_sync()` in `dw_hdmi_qp_disable_scramb()` should pr=
event this because it's called before `hdmi->curr_conn =3D NULL`:

```c
	WRITE_ONCE(hdmi->scramb_enabled, false);
	cancel_delayed_work_sync(&hdmi->scramb_work);
	...
	// (in atomic_disable, after disable_scramb returns)
	hdmi->curr_conn =3D NULL;
```

The `WRITE_ONCE(false)` + `cancel_delayed_work_sync()` ensures the work won=
't re-schedule and any in-flight execution completes before `curr_conn` is =
cleared. This looks correct.

**However**, `curr_conn` is also accessed in `detect_ctx` which runs from t=
he connector detect path (HPD work). There's no synchronization between `de=
tect_ctx` reading `hdmi->scramb_enabled` / `hdmi->curr_conn` and `atomic_di=
sable` clearing them. If a detect fires while a modeset is tearing down, `d=
etect_ctx` could see `scramb_enabled =3D=3D true` but `curr_conn` could be =
in-flight being set to NULL. In practice, the connection_mutex should seria=
lize these paths, but it might be worth adding a comment or a `READ_ONCE` f=
or `curr_conn` reads as well, similar to what was done for `scramb_enabled`.

**SCDC rollback logic in `dw_hdmi_qp_set_scramb`:**
```c
	done =3D drm_scdc_set_high_tmds_clock_ratio(hdmi->curr_conn, true);
	if (!done)
		return -EIO;

	done =3D drm_scdc_set_scrambling(hdmi->curr_conn, true);
	if (!done) {
		drm_scdc_set_high_tmds_clock_ratio(hdmi->curr_conn, false);
		return -EIO;
	}
```
Good =E2=80=94 if scrambling fails, the high TMDS clock ratio is rolled bac=
k. The v5 changelog mentions this was added in response to review feedback.

**In `dw_hdmi_qp_enable_scramb`:**
```c
	WRITE_ONCE(hdmi->scramb_enabled, true);

	ret =3D dw_hdmi_qp_set_scramb(hdmi);
	if (ret) {
		hdmi->scramb_enabled =3D false;
		return;
	}
```

Minor: The set uses `WRITE_ONCE` but the reset on failure uses a plain stor=
e. Since the work item reads with `READ_ONCE`, the reset should also use `W=
RITE_ONCE` for consistency. Though in practice, at this point the work hasn=
't been scheduled yet (it's scheduled inside `set_scramb` which failed), so=
 it's not a live bug.

**`dw_hdmi_qp_reset_crtc` in detect_ctx:**
```c
	if (!!(config & SCDC_SCRAMBLING_ENABLE) =3D=3D hdmi->scramb_enabled)
		return 0;
```
This compares the sink's current SCDC scrambling state with the driver's ex=
pectation. If they mismatch (e.g., sink was power-cycled), trigger a CRTC r=
eset. This is the key logic for recovering from sink-side resets.

**`dw_hdmi_qp_bridge_tmds_char_rate_valid` changes:**
```c
	if (hdmi->no_hpd && rate > HDMI14_MAX_TMDSCLK) {
		...
		return MODE_CLOCK_HIGH;
	}

	if (rate > HDMI20_MAX_TMDSRATE) {
		...
		return MODE_CLOCK_HIGH;
	}
```
This correctly resolves the TODO that was in the original code: no_hpd mode=
 is limited to 340 MHz (no SCDC without a connected sink), and the overall =
cap is 600 MHz (HDMI 2.0 TMDS limit, above which FRL is needed).

**`INIT_DELAYED_WORK` placement:** It's initialized in `dw_hdmi_qp_bind()` =
before `hdmi->dev` is assigned:
```c
	INIT_DELAYED_WORK(&hdmi->scramb_work, dw_hdmi_qp_scramb_work);

	hdmi->dev =3D dev;
```
This is fine =E2=80=94 `INIT_DELAYED_WORK` only sets up the work struct met=
adata and the callback pointer, it doesn't use `hdmi->dev`.

**Overall for patch 5:** The design is solid. The main nit is the inconsist=
ent `WRITE_ONCE` on the `scramb_enabled =3D false` failure path, and it wou=
ld be good to have a brief comment about the locking assumptions for `curr_=
conn` access in `detect_ctx` vs `atomic_disable`.

---

---
Generated by Claude Code Patch Reviewer