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 high TMDS clock ratio and scrambling support
Date: Tue, 03 Mar 2026 12:42:55 +1000
Message-ID: <review-patch3-20260303-dw-hdmi-qp-scramb-v4-3-317d3b8bd219@collabora.com>
In-Reply-To: <20260303-dw-hdmi-qp-scramb-v4-3-317d3b8bd219@collabora.com>
References: <20260303-dw-hdmi-qp-scramb-v4-0-317d3b8bd219@collabora.com> <20260303-dw-hdmi-qp-scramb-v4-3-317d3b8bd219@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 core patch. It has important functionality but also a significa=
nt locking concern.

**Locking issue in `dw_hdmi_qp_reset_crtc`:** This function is called from =
`dw_hdmi_qp_bridge_detect` (the `detect_ctx` callback), which is invoked wi=
th `connection_mutex` already held via `ctx`. Inside, it calls:

```c
retry:
    ret =3D drm_bridge_helper_reset_crtc(&hdmi->bridge, ctx);
    if (ret =3D=3D -EDEADLK) {
        drm_modeset_backoff(ctx);
        goto retry;
    }
```

`drm_bridge_helper_reset_crtc` acquires `connection_mutex` with `ctx` (retu=
rns 0 due to `-EALREADY` handling in `modeset_lock()` at `drm_modeset_lock.=
c:321`), but then **unconditionally releases** it at the end:

```c
out:
    drm_modeset_unlock(&dev->mode_config.connection_mutex);
```

`drm_modeset_unlock` does `list_del_init(&lock->head)` + `ww_mutex_unlock(&=
lock->mutex)`, which fully releases the lock =E2=80=94 even though the call=
er's framework (`drm_helper_probe_detect_ctx`) still expects it to be held.=
 This is a locking protocol violation. After `drm_bridge_helper_reset_crtc`=
 returns, `connection_mutex` is no longer held, creating a window where con=
current threads can modify connector state.

Additionally, calling `drm_modeset_backoff(ctx)` on a context that the func=
tion does not own is incorrect. `drm_modeset_backoff` drops **all** locks h=
eld by `ctx`, not just the contended one. The EDEADLK should be propagated =
back to the framework's retry loop rather than handled internally. The corr=
ect approach would be:

```c
ret =3D drm_bridge_helper_reset_crtc(&hdmi->bridge, ctx);
if (ret)
    return ret;  /* Let caller handle EDEADLK and other errors */
```

This would also require `dw_hdmi_qp_bridge_detect` to propagate the error:
```c
if (status =3D=3D connector_status_connected && hdmi->scramb_enabled) {
    ret =3D dw_hdmi_qp_reset_crtc(hdmi, connector, ctx);
    if (ret)
        return ret;
}
```

However, even with this fix, `drm_bridge_helper_reset_crtc` still unlocks `=
connection_mutex` at line 57 after the EALREADY re-lock. This seems like `d=
rm_bridge_helper_reset_crtc` is designed to be called **without** `connecti=
on_mutex` already held. The detect_ctx callback may need to be restructured=
 to release `connection_mutex` before calling reset, or `drm_bridge_helper_=
reset_crtc` needs a variant that doesn't manage `connection_mutex` itself.

**Scrambling work queue safety:** The `scramb_work` delayed work accesses `=
hdmi->curr_conn`:

```c
static void dw_hdmi_qp_scramb_work(struct work_struct *work)
{
    ...
    if (!drm_scdc_get_scrambling_status(hdmi->curr_conn))
        dw_hdmi_qp_set_scramb(hdmi);
}
```

The lifecycle looks correct =E2=80=94 `cancel_delayed_work_sync` in `dw_hdm=
i_qp_disable_scramb` runs before `curr_conn` is set to NULL in `atomic_disa=
ble`. However, consider: if the sink disconnects and HPD triggers detect_ct=
x (which resets the CRTC, triggering atomic_disable), the `cancel_delayed_w=
ork_sync` could block on a work item that is in the middle of writing SCDC.=
 This should be fine functionally but might cause a delay.

**`SCDC_MIN_SOURCE_VERSION` naming:** The constant is used as:
```c
drm_scdc_writeb(hdmi->bridge.ddc, SCDC_SOURCE_VERSION,
                min_t(u8, ver, SCDC_MIN_SOURCE_VERSION));
```
The name `MIN` is misleading =E2=80=94 this is the **maximum** source versi=
on the driver supports. `SCDC_SOURCE_VERSION` or `SCDC_MAX_SOURCE_VERSION` =
would be clearer.

**`SCRAMB_POLL_DELAY_MS` change from 3000 to 5000:** Not explained in the c=
ommit message. What motivated this change? It increases the window during w=
hich scrambling could be desynchronized from the sink.

**Pre-existing `drm_edid` leak in `no_hpd` path** (not introduced by this p=
atch but worth noting):
```c
if (hdmi->no_hpd) {
    drm_edid =3D drm_edid_read_ddc(connector, bridge->ddc);
    if (drm_edid)
        return connector_status_connected;  /* drm_edid leaked */
```

**Ignored return value:** `dw_hdmi_qp_bridge_detect` ignores the return val=
ue of `dw_hdmi_qp_reset_crtc`:
```c
if (status =3D=3D connector_status_connected && hdmi->scramb_enabled)
    dw_hdmi_qp_reset_crtc(hdmi, connector, ctx);

return status;
```
If the reset fails, the function still reports connected status. Errors sho=
uld either be propagated or explicitly documented as intentionally ignored.

---
Generated by Claude Code Patch Reviewer