From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: Add "link bpc" DRM property Date: Thu, 12 Mar 2026 06:57:50 +1000 Message-ID: In-Reply-To: <20260311-link-bpc-v4-0-51775e964720@collabora.com> References: <20260311-link-bpc-v4-0-51775e964720@collabora.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit MIME-Version: 1.0 Overall Series Review Subject: Add "link bpc" DRM property Author: Nicolas Frattaroli Patches: 3 Reviewed: 2026-03-12T06:57:50.360536 --- This is a 2-patch series adding a new immutable "link bpc" DRM connector property that exposes the actual bits-per-component achieved on a display link, letting userspace detect when bit depth has been degraded (e.g. due to bandwidth constraints). Patch 1 adds the core DRM infrastructure and HDMI helper integration; patch 2 adds amdgpu support. The design is clean and the approach is reasonable. There are a few issues worth discussing: **Timing concern in `commit_tail`**: The link_bpc property update and uevent fire *before* `funcs->atomic_commit_tail()` runs. This means userspace is notified of a bpc change before the hardware has actually committed the new state. For amdgpu specifically (which has its own `atomic_commit_tail`), this means the uevent fires before the display hardware is programmed. Ideally this should happen *after* the hw commit completes. **Clamping lower bound of 8**: The property range and clamping both use 8 as the minimum, but some displays legitimately run at 6 bpc (e.g. certain eDP panels). If 6 bpc is "experimental internal" for amdgpu, that's fine for patch 2's local helper, but hard-coding 8 as the universal lower bound in the core DRM property (patch 1) seems potentially limiting. **Missing link_bpc reset on disconnect**: When a connector is disabled/disconnected, `link_bpc` is never explicitly reset. It will carry the stale value from the last active state via `memcpy` in `duplicate_state`. This means the property could report a stale bpc value on a disconnected connector. --- --- Generated by Claude Code Patch Reviewer