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/vc4: hdmi: Convert to common HDMI 2.0 SCDC scrambling helpers Date: Thu, 04 Jun 2026 13:33:51 +1000 Message-ID: In-Reply-To: <20260602-dw-hdmi-qp-scramb-v7-25-445eb54ee1ed@collabora.com> References: <20260602-dw-hdmi-qp-scramb-v7-0-445eb54ee1ed@collabora.com> <20260602-dw-hdmi-qp-scramb-v7-25-445eb54ee1ed@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 a substantial rework that removes ~230 lines and adds ~35. Key obse= rvations: 1. **`vc4_hdmi_scrambler_enable/disable()` no longer call `drm_dev_enter()`= :** The old code guarded hardware access with `drm_dev_enter()`. The new ca= llbacks rely on the caller (`vc4_hdmi_encoder_post_crtc_enable/disable()`) = already being within a `drm_dev_enter()` section. This is correct for the m= odeset path, but these callbacks can also be invoked from the SCDC monitor = work item (`drm_scdc_monitor_scrambler` =E2=86=92 `drm_scdc_try_scrambling_= setup` =E2=86=92 `scrambler_enable`). In that path, there's no `drm_dev_ent= er()` protection. If the device is unplugged while the monitor work is runn= ing, the register access could be unsafe. Consider adding `drm_dev_enter()`= guards to the callbacks themselves. 2. **`connector->hdmi.scrambler_enabled =3D connector->hdmi.scrambler_suppo= rted`** in `connector_init()` seeds the state to ensure `drm_scdc_stop_scra= mbling()` runs at boot. This is well-commented and correct. ### PATCHES 26-28: KUnit tests Well-structured tests covering the validation matrix (patch 26) and scrambl= er_needed flag behavior (patch 28). The test infrastructure properly infers= `scrambler_supported` from the presence of callbacks, mirroring real drive= r behavior. ### PATCHES 29-30: EDID conformity fixes Thorough fixes with full `edid-decode` output documenting the before/after = state. No functional change to existing test behavior =E2=80=94 only confor= mity issues are addressed. --- **Summary of items to consider:** 1. `INIT_DELAYED_WORK` for scdc_work runs for all connectors, not just HDMI= (patch 2) 2. `curr_conn` pointer in dw-hdmi-qp has no explicit locking documentation = (patch 14) 3. Errors from SCDC sync during detect are silently swallowed (patch 10) = =E2=80=94 consider a debug log 4. vc4 scrambler callbacks lack `drm_dev_enter()` protection when called fr= om the SCDC monitor work (patch 25) 5. `enable_hpd(NULL, hdmi)` passes NULL for the unused first parameter (pat= ch 21) --- Generated by Claude Code Patch Reviewer