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: only add states of active or transient active colorops Date: Wed, 27 May 2026 14:45:25 +1000 Message-ID: In-Reply-To: <20260526142940.504911-2-mwen@igalia.com> References: <20260526142940.504911-1-mwen@igalia.com> <20260526142940.504911-2-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:** Replaces the call to `drm_atomic_add_affected_colorops()` in `= drm_atomic_add_affected_planes()` with a new `drm_atomic_add_pipeline_color= ops()` that only walks the old and new pipeline chains rather than iteratin= g over every colorop on the plane. **Code quality:** The new function is well-structured with clear early-exit= optimization when old and new pipelines are the same. **Issue 1 =E2=80=94 WARN_ON condition may be too strict:** ```c if (WARN_ON(!new_plane_state || !old_plane_state)) return -EINVAL; ``` In `drm_atomic_add_affected_planes()`, `drm_atomic_get_plane_state()` was j= ust called, which creates both old and new plane state entries. So `new_pla= ne_state` being NULL here should indeed be impossible. However, `old_plane_= state` is also guaranteed since `drm_atomic_get_plane_state()` always sets = both old and new. The WARN_ON is defensive and fine, just noting it's argua= bly unnecessary. **Issue 2 =E2=80=94 docstring update in `drm_atomic_add_affected_colorops` = is incomplete:** The patch updates the doc comment for `drm_atomic_add_affected_colorops()`: ```c - * currently used by @plane to the atomic configuration @state. This is us= eful - * when an atomic commit also needs to check all currently enabled colorop= on - * @plane, e.g. when changing the mode. It's also useful when re-enabling = a plane + * currently used by @plane to the atomic configuration @state. It's useful + * when re-enabling a plane to avoid special code to force-enable all colo= rops. ``` But the function still adds **all** colorops for the plane (via `drm_for_ea= ch_colorop`), not just pipeline-active ones. This is correct for its remain= ing caller (`drm_atomic_helper_duplicate_state` in Patch 2), but the doc sh= ould clarify that this adds *all* colorops, not just those in the active pi= peline. This would help future readers understand the distinction from `drm= _atomic_add_pipeline_colorops`. **Otherwise looks correct.** The implementation properly handles the pipeli= ne transition case (old pipeline !=3D new pipeline) by walking both chains. --- --- Generated by Claude Code Patch Reviewer