From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/bridge: dw-hdmi-qp: Add high TMDS clock ratio and scrambling support Date: Wed, 25 Mar 2026 07:36:46 +1000 Message-ID: In-Reply-To: <20260323-dw-hdmi-qp-scramb-v4-3-1d99df84883e@collabora.com> References: <20260323-dw-hdmi-qp-scramb-v4-0-1d99df84883e@collabora.com> <20260323-dw-hdmi-qp-scramb-v4-3-1d99df84883e@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. Several observations:** 1. **Pre-existing EDID leak in `no_hpd` detect path (line 927-931):** ```c drm_edid =3D drm_edid_read_ddc(connector, bridge->ddc); if (drm_edid) return connector_status_connected; ``` The `drm_edid` is never freed. This is a pre-existing bug, but since thi= s patch changes the function signature and touches surrounding code, it wou= ld be good to fix it here with a `drm_edid_free(drm_edid)` before the retur= n. 2. **`curr_conn` lifetime concerns:** `hdmi->curr_conn` is set in `atomic_e= nable` and cleared in `atomic_disable`. The `scramb_work` delayed work acce= sses `hdmi->curr_conn` (line 910 via `dw_hdmi_qp_scramb_work` =E2=86=92 `dw= _hdmi_qp_set_scramb`). While `cancel_delayed_work_sync` is called in `disab= le_scramb`, there's a window: if the work fires and calls `dw_hdmi_qp_set_s= cramb()` which calls `schedule_delayed_work()` *after* the cancel but befor= e `scramb_enabled =3D false` =E2=80=94 actually no, `cancel_delayed_work_sy= nc` waits for completion, and `scramb_enabled` is set to false before cance= l. Wait, looking at the order in `dw_hdmi_qp_disable_scramb()`: ```c hdmi->scramb_enabled =3D false; cancel_delayed_work_sync(&hdmi->scramb_work); ``` The work function `dw_hdmi_qp_scramb_work` doesn't check `scramb_enabled= `, so if the work is currently executing when `disable_scramb` sets `scramb= _enabled =3D false`, the work could re-schedule itself via `dw_hdmi_qp_set_= scramb()` =E2=86=92 `schedule_delayed_work()` *before* `cancel_delayed_work= _sync` completes. `cancel_delayed_work_sync` will wait for the currently-ru= nning work to finish, but if the work re-schedules itself, the cancellation= might not catch the newly scheduled instance. This is a potential race. Co= nsider checking `scramb_enabled` in `dw_hdmi_qp_scramb_work()` before re-sc= heduling, or using a different cancellation pattern. 3. **`SCDC_MIN_SOURCE_VERSION` naming:** The constant is named `SCDC_MIN_SO= URCE_VERSION` with value `0x1`, but it's used as the maximum version the so= urce supports (line 922-923: `min_t(u8, ver, SCDC_MIN_SOURCE_VERSION)`). Th= e `min_t` clamps the source version to at most `0x1`. The naming is confusi= ng =E2=80=94 something like `SCDC_MAX_SOURCE_VERSION` would be clearer. 4. **Error handling in `dw_hdmi_qp_set_scramb` (lines 795-799):** Neither `= drm_scdc_set_high_tmds_clock_ratio()` nor `drm_scdc_set_scrambling()` have = their return values checked. If these I2C writes fail (e.g., due to a flaky= connection), scrambling is assumed to be configured but it isn't. At minim= um, log a warning on failure. 5. **`dw_hdmi_qp_reset_crtc` error propagation:** When called from `dw_hdmi= _qp_bridge_detect()` (line 940), the return value of `dw_hdmi_qp_reset_crtc= ()` is ignored. If the CRTC reset fails, the function still returns `connec= tor_status_connected`. This seems intentional (detection succeeded, reset i= s best-effort), but worth a comment. 6. **`dw_hdmi_qp_enable_scramb` called before PHY init:** In `atomic_enable= `, `dw_hdmi_qp_enable_scramb()` is called (line 851) *before* `hdmi->phy.op= s->init()` (line 859). The `enable_scramb` function does I2C writes to the = sink via SCDC and writes to hardware register `SCRAMB_CONFIG0`, then sleeps= 1ms. The PHY init happens after. Is this ordering correct? The HDMI 2.0 sp= ec requires the source to stop TMDS transmission, configure scrambling, the= n resume =E2=80=94 so doing scramb setup before PHY init (which presumably = starts TMDS) seems correct. 7. **`HDMI20_MAX_TMDSRATE` value of 600 MHz:** HDMI 2.0 specifies a maximum= of 600 MHz TMDS character rate (corresponding to 18 Gbps). This is correct. --- Generated by Claude Code Patch Reviewer