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/atomic: track individual colorop updates Date: Tue, 26 May 2026 07:18:47 +1000 Message-ID: In-Reply-To: <20260525100524.304263-4-mwen@igalia.com> References: <20260525100524.304263-1-mwen@igalia.com> <20260525100524.304263-4-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 This is the core patch. It sets `plane_state->color_mgmt_changed` whenever = a colorop property value actually changes. **Change to `drm_atomic_set_colorop_for_plane`**: Now returns `bool` indica= ting whether the color pipeline pointer actually changed. The early-out whe= n `plane_state->color_pipeline =3D=3D colorop` avoids unnecessary state dir= tying when userspace re-sets the same pipeline. The only call site at line = 615 uses the return value correctly: ```c state->color_mgmt_changed |=3D drm_atomic_set_colorop_for_plane(state, colo= rop); ``` The function is `EXPORT_SYMBOL` so theoretically out-of-tree callers exist,= but since the return value is ignorable (void=E2=86=92bool), this is ABI-s= afe. **Change to `drm_atomic_colorop_set_property`**: Each property branch now d= oes a compare-before-assign and sets `*replaced =3D true` only on actual va= lue change. This is correct for all scalar properties (bypass, lut1d_interp= olation, curve_1d_type, multiplier, lut3d_interpolation). The `data_propert= y` case passes `replaced` through to `drm_atomic_color_set_data_property` w= hich delegates to `drm_property_replace_blob_from_id` =E2=80=94 that functi= on already computes `replaced` correctly for blobs. **Change to `drm_atomic_set_property` (COLOROP case)**: After detecting a c= hange, it pulls in the plane state and sets `color_mgmt_changed`: ```c ret =3D drm_atomic_colorop_set_property(colorop, colorop_state, file_priv, prop, prop_value, &replaced); if (ret || !replaced) break; plane_state =3D drm_atomic_get_plane_state(state, colorop->plane); ... plane_state->color_mgmt_changed |=3D replaced; ``` **Minor observation**: At line 1325, `plane_state->color_mgmt_changed |=3D = replaced;` is guarded by `if (ret || !replaced)` above (line 1317), so `rep= laced` is always `true` when line 1325 is reached. The `|=3D` is harmless (= and consistent with the style used elsewhere), but `=3D true` would be equi= valent. This is not a bug =E2=80=94 the existing code is fine for consisten= cy and future-proofing. **`#include `** added to `drm_atomic_uapi.h` for the `bool` = return type =E2=80=94 correct. No issues. --- --- Generated by Claude Code Patch Reviewer