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: Thu, 07 May 2026 13:05:39 +1000 Message-ID: In-Reply-To: <20260506192633.16066-7-mwen@igalia.com> References: <20260506192633.16066-1-mwen@igalia.com> <20260506192633.16066-7-mwen@igalia.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 **Assessment: Good, straightforward driver-side consumer** This wires the new `color_mgmt_changed` flag into the AMD display driver at= two critical points: 1. **`amdgpu_dm_commit_planes()`**: The color surface updates (gamma, trans= fer func, gamut remap, HDR mult, shaper, 3D LUT, blend TF) now also trigger= when `new_plane_state->color_mgmt_changed` is set: ```c if (new_pcrtc_state->color_mgmt_changed || new_plane_state->color_mgmt_chan= ged) { ``` This is the actual bug fix =E2=80=94 without this, changing colorop propert= ies (like shaper/3D LUT for night mode) wouldn't cause DC to reprogram the = color blocks. 2. **`should_reset_plane()`**: Returns true when `new_plane_state->color_mg= mt_changed` is set, causing plane reset during atomic check. This ensures t= he DC plane state is fully recreated when colorop properties change. Both additions are consistent with the existing CRTC `color_mgmt_changed` p= attern. The comment `/* Plane color pipeline or its colorop changes. */` is= appropriate. Harry's R-b tag from v1 carried forward is noted. **One question for the maintainer**: `should_reset_plane()` returning true = can trigger full plane recreation which is relatively expensive. For the ga= mescope night mode use case (shaper/3D LUT data changes), this should be fi= ne since it happens on user-initiated actions, not per-frame. But if a comp= ositor were to update colorop properties every frame, this could cause perf= ormance issues. The existing CRTC `color_mgmt_changed` path has the same ch= aracteristic, so this is consistent, but worth keeping in mind. --- Generated by Claude Code Patch Reviewer