Claude review: drm/bridge: dw-hdmi-qp: Add high TMDS clock ratio and scrambling support

[Claude Code Review Bot <claude-review@example.com>]

Patch Review

This is the core patch. It has important functionality but also a significant 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 with `connection_mutex` already held via `ctx`. Inside, it calls:

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

`drm_bridge_helper_reset_crtc` acquires `connection_mutex` with `ctx` (returns 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 — even though the caller'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 concurrent threads can modify connector state.

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

```c
ret = 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 == connector_status_connected && hdmi->scramb_enabled) {
    ret = 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 `drm_bridge_helper_reset_crtc` is designed to be called **without** `connection_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 — `cancel_delayed_work_sync` in `dw_hdmi_qp_disable_scramb` runs before `curr_conn` is set to NULL in `atomic_disable`. However, consider: if the sink disconnects and HPD triggers detect_ctx (which resets the CRTC, triggering atomic_disable), the `cancel_delayed_work_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 — this is the **maximum** source version 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 commit message. What motivated this change? It increases the window during which scrambling could be desynchronized from the sink.

**Pre-existing `drm_edid` leak in `no_hpd` path** (not introduced by this patch but worth noting):
```c
if (hdmi->no_hpd) {
    drm_edid = 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 value of `dw_hdmi_qp_reset_crtc`:
```c
if (status == 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 should either be propagated or explicitly documented as intentionally ignored.

---
Generated by Claude Code Patch Reviewer