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: Mon, 25 May 2026 22:24:20 +1000 Message-ID: In-Reply-To: <20260519211111.228303-6-mwen@igalia.com> References: <20260519211111.228303-1-mwen@igalia.com> <20260519211111.228303-6-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 **Status: Good logic, but needs the drm_colorop.c fixes moved to Patch 4** This is the core of the series. It: 1. Changes `drm_atomic_set_colorop_for_plane()` to return `bool` indicating= whether the pipeline changed. 2. Adds change tracking through `replaced` out-parameter in `drm_atomic_col= orop_set_property()` and `drm_atomic_color_set_data_property()`. 3. In `drm_atomic_set_property()` for `DRM_MODE_OBJECT_COLOROP`, acquires t= he plane state and sets `color_mgmt_changed` when a colorop property actual= ly changed value. The change-detection pattern is clean =E2=80=94 each property setter only s= ets `*replaced =3D true` when the new value differs: ```c if (state->bypass !=3D val) { state->bypass =3D val; *replaced =3D true; } ``` One observation in `drm_atomic_set_property`: ```c if (ret || !replaced) break; ... plane_state->color_mgmt_changed |=3D replaced; ``` At this point `replaced` is guaranteed `true`, so `|=3D replaced` is equiva= lent to `=3D true`. The `|=3D` is harmless and follows the pattern for not = clobbering, but it's slightly misleading since `replaced` can't be `false` = here. Very minor, and Chaitanya already approved this for consistency. The `drm_colorop.c` hunk fixes the syntax bugs from Patch 4. **This fix mus= t be squashed into Patch 4** to maintain bisectability. --- --- Generated by Claude Code Patch Reviewer