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: ite-it66121: Select HDMI or DVI mode based on sink type Date: Sat, 16 May 2026 13:19:46 +1000 Message-ID: In-Reply-To: <20260512132232.333654-3-javierm@redhat.com> References: <20260512132232.333654-1-javierm@redhat.com> <20260512132232.333654-3-javierm@redhat.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 **Correctness: The core logic is correct** The approach of checking `connector->display_info.is_hdmi` and conditionall= y setting the mode/AVI infoframes is the right fix: ```c if (connector->display_info.is_hdmi) { mode =3D IT66121_HDMI_MODE_HDMI; avi_pkt =3D IT66121_AVI_INFO_PKT_ON | IT66121_AVI_INFO_PKT_RPT; } ``` For DVI sinks, `mode` stays `IT66121_HDMI_MODE_DVI` (0) and `avi_pkt` stays= 0, which correctly disables HDMI mode and AVI infoframes. **Issue: `connector->display_info.is_hdmi` accessed outside mutex** ```c struct drm_connector *connector =3D ctx->connector; if (connector->display_info.is_hdmi) { ... } mutex_lock(&ctx->lock); ``` The `display_info.is_hdmi` check happens before `ctx->lock` is taken. While= `display_info` is typically stable once the EDID has been read (it's set d= uring `drm_edid_connector_update()` which happens at detect time), and `ctx= ->connector` was just set in the calling function, this is not a serious co= ncern in practice =E2=80=94 the lock here is protecting the regmap writes, = not the connector state. But it's worth noting the code reads `ctx->connect= or` without the lock. **Issue: Duplicate writes in `.mode_set` still not addressed** As mentioned in patch 1, the `.mode_set` callback still unconditionally wri= tes `IT66121_HDMI_MODE_HDMI` and enables AVI infoframes. So the sequence du= ring a mode change is: 1. `.mode_set` writes HDMI mode + AVI infoframes ON (always) 2. `.atomic_enable` writes DVI mode + AVI infoframes OFF (for DVI sinks) While the end state is correct, the DVI monitor momentarily receives HDMI-m= ode signaling between steps 1 and 2. This could cause transient issues depe= nding on the monitor. Removing the writes from `.mode_set` would eliminate = this. **Good: `IT66121_HDMI_MODE_DVI` define** ```c #define IT66121_HDMI_MODE_DVI 0 ``` Adding a named constant for the DVI mode value (0) instead of using a bare = 0 is good practice and makes the code self-documenting. **Observation: AVI infoframe content still written in `.mode_set` for DVI** Even after this series, the `.mode_set` callback still writes AVI infoframe= *content* to the `IT66121_AVIINFO_DB1_REG` registers and the checksum regi= ster for DVI sinks. While the AVI infoframe *transmission* is disabled (the= PKT register is cleared in `.atomic_enable`), writing infoframe content to= hardware for a DVI connection is wasted work. This is not a bug =E2=80=94 = just unnecessary register writes. A cleaner approach would be to skip the A= VI infoframe preparation in `.mode_set` for DVI sinks, but that would requi= re `connector` to be available in `.mode_set`, which is the whole problem t= his series is solving. So this is acceptable as-is; the infoframe content s= imply goes unused. **Summary**: The series fixes a real bug correctly but needs the duplicate = register writes removed from `.mode_set` in patch 1 (or a clear explanation= in the commit message for why they're intentionally kept). The enable orde= ring (unmute before TX mode set) is also worth reconsidering. --- Generated by Claude Code Patch Reviewer