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/connector: Add a 'link bpc' property Date: Sun, 22 Mar 2026 04:19:40 +1000 Message-ID: In-Reply-To: <20260319-link-bpc-v5-1-5306cd04a708@collabora.com> References: <20260319-link-bpc-v5-0-5306cd04a708@collabora.com> <20260319-link-bpc-v5-1-5306cd04a708@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 **Timing issue in commit_tail (major concern):** The patch inserts the link_bpc property update and uevent between `drm_atom= ic_helper_wait_for_dependencies()` and the `atomic_commit_tail()` call: ```c drm_atomic_helper_wait_for_dependencies(state); for_each_oldnew_connector_in_state(state, connector, old_conn_state, new_conn_state, i) { if (old_conn_state->link_bpc !=3D new_conn_state->link_bpc) drm_connector_update_link_bpc_property(connector, new_conn_state); } /* ... then atomic_commit_tail() actually programs hardware */ ``` This fires the uevent *before* the hardware has been programmed. Userspace = reacting to the uevent could query the property and see the new value while= the hardware is still at the old state. The property update and uevent sho= uld be moved to *after* `atomic_commit_tail()` returns (but before `drm_ato= mic_helper_commit_cleanup_done()`). That would match the pattern used by `d= rm_hdcp_update_content_protection` where the property is updated after the = HW state is known. **clamp() type mismatch:** ```c u8 bpc =3D clamp(state->link_bpc, 8, state->max_bpc); ``` `state->link_bpc` is `u8`, `state->max_bpc` is `u8`, but the literal `8` is= `int`. The kernel's `clamp()` macro has strict type checking and this will= likely generate a compiler warning or error. Should be `clamp_t(u8, state-= >link_bpc, 8, state->max_bpc)` or use a `(u8)8` cast. **Unconditional uevent on every link_bpc change:** `drm_connector_update_link_bpc_property()` always fires a uevent via `drm_s= ysfs_connector_property_event()`. The caller in `commit_tail` already check= s `old_conn_state->link_bpc !=3D new_conn_state->link_bpc`, so the uevent i= s conditional on an actual change =E2=80=94 that's good. However, the funct= ion itself doesn't gate on the property having actually changed from the co= nnector object's perspective (via `drm_object_property_set_value`). If `drm= _connector_update_link_bpc_property` is ever called from another path where= the value hasn't changed, it would fire a spurious uevent. Consider adding= a check inside the function, or document that callers must check for chang= es first. **Missing `int i` declaration:** The patch adds `for_each_oldnew_connector_in_state(state, connector, old_co= nn_state, new_conn_state, i)` in `commit_tail()` but the loop variable `i` = is already declared on line 2042 as `unsigned int i` (shared with the self_= refresh loop). This works, but only because the existing `i` is `unsigned i= nt` which matches what the macro expects. **Return value of `drm_connector_attach_link_bpc_property` returns int:** This function returns `int` (error codes), while the existing `drm_connecto= r_attach_max_bpc_property()` also returns `int`. This is fine, but callers = need to check it =E2=80=94 see patch 3 comments. **Minor nit =E2=80=94 double space in doc:** ```c * @max_bpc: specify the upper limit, matching that of 'max bpc' property ``` Extra space before "that". --- Generated by Claude Code Patch Reviewer