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/amd/display: use plane color_mgmt_changed to track colorop changes Date: Tue, 05 May 2026 09:26:02 +1000 Message-ID: In-Reply-To: <20260501132527.522320-7-mwen@igalia.com> References: <20260501132527.522320-1-mwen@igalia.com> <20260501132527.522320-7-mwen@igalia.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit MIME-Version: 1.0 Patch Review **Purpose:** Makes the AMD display driver react to plane-level `color_mgmt_changed`, completing the end-to-end fix. **Review:** The `amdgpu_dm_commit_planes` change is straightforward: ```c -if (new_pcrtc_state->color_mgmt_changed) { +if (new_pcrtc_state->color_mgmt_changed || new_plane_state->color_mgmt_changed) { ``` This correctly extends the existing CRTC-level check to also cover plane-level colorop changes. The `should_reset_plane` addition: ```c +if (new_plane_state->color_mgmt_changed) + return true; ``` This is placed right after the CRTC `color_mgmt_changed` check, which is logically consistent. However, **returning `true` from `should_reset_plane` causes plane recreation** (remove + re-add to DC state), which is a heavier operation than just updating surface properties. Is a full plane reset always necessary for colorop changes, or would just the surface update in `amdgpu_dm_commit_planes` suffice? Looking at the existing code, the CRTC `color_mgmt_changed` also triggers a plane reset, so this follows the established pattern. The comment says "Plane color pipeline or its colorop changes" which is clear. **Overall this patch is correct** and represents the minimal driver-side change to consume the new tracking mechanism. --- **Summary:** The series is well-crafted and addresses a real bug. The main substantive issue is the missing return value check for `drm_object_property_get_default_value` in patch 4's `__drm_colorop_state_reset` additions. The UAPI tightening in patch 2 should be called out more explicitly. Otherwise, this looks good to merge. --- Generated by Claude Code Patch Reviewer