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: duplicate state of all colorops Date: Wed, 27 May 2026 14:45:25 +1000 Message-ID: In-Reply-To: <20260526142940.504911-3-mwen@igalia.com> References: <20260526142940.504911-1-mwen@igalia.com> <20260526142940.504911-3-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 **Purpose:** In `drm_atomic_helper_duplicate_state()`, remove the `if (plan= e_state->color_pipeline)` guard so that **all** colorops are snapshotted re= gardless of whether a pipeline is currently active. This ensures colorop se= ttings persist across deactivation/reactivation. **The change is straightforward and correct:** ```c - if (plane_state->color_pipeline) { - err =3D drm_atomic_add_affected_colorops(state, plane); - if (err) - goto free; - } - + err =3D drm_atomic_add_affected_colorops(state, plane); + if (err) + goto free; ``` **Minor concern =E2=80=94 interaction with Patch 1:** Patch 1 introduces `= drm_atomic_add_pipeline_colorops()` for use in `drm_atomic_add_affected_pla= nes()` (which only adds active/transitioning colorops), while this patch ke= eps `drm_atomic_add_affected_colorops()` here (which adds all colorops on t= he plane). The two functions serve legitimately different purposes, but hav= ing two similarly-named functions with different semantics could be confusi= ng. A comment in the code or in the commit message acknowledging this inten= tional distinction would help. **No bugs found.** The change correctly ensures persistence of colorop stat= e across pipeline deactivation. --- --- Generated by Claude Code Patch Reviewer