From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: drm/atomic: only add states of active or transient active colorops
Date: Fri, 05 Jun 2026 06:13:39 +1000
Message-ID: <review-patch1-20260604180457.1110110-2-mwen@igalia.com>
In-Reply-To: <20260604180457.1110110-2-mwen@igalia.com>
References: <20260604180457.1110110-1-mwen@igalia.com> <20260604180457.1110110-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:** Replace the broad `drm_atomic_add_affected_colorops()` call (w=
hich walks ALL colorops for a plane) with the new `drm_atomic_add_pipeline_=
colorops()` that only adds colorops from the old and new color pipelines.

**New macro (`drm_for_each_colorop_in_pipeline`):**

```c
#define drm_for_each_colorop_in_pipeline(colorop, pipeline) \
	for ((colorop) =3D (pipeline); (colorop); (colorop) =3D (colorop)->next)
```

This is clean and straightforward. It safely handles `pipeline =3D=3D NULL`=
 (the loop body simply never executes). Good addition from the v2 feedback.

**`drm_atomic_add_pipeline_colorops()` function:**

```c
static int
drm_atomic_add_pipeline_colorops(struct drm_atomic_commit *state,
				 struct drm_plane *plane)
```

The function signature uses `struct drm_atomic_commit *state` rather than `=
struct drm_atomic_state *state` =E2=80=94 this is fine if the codebase has =
already renamed the struct (I can see the existing code also uses `drm_atom=
ic_commit`), so this is consistent.

The WARN_ON check:

```c
if (WARN_ON(!new_plane_state || !old_plane_state))
    return -EINVAL;
```

This is reasonable defensive coding. Since `drm_atomic_add_affected_planes(=
)` always calls `drm_atomic_get_plane_state()` first, both states should al=
ways exist when this is called. The WARN_ON is appropriate to flag programm=
ing errors.

**Optimization with pointer comparison:**

```c
if (new_plane_state->color_pipeline =3D=3D old_plane_state->color_pipeline)
    return 0;
```

This is correct =E2=80=94 if both old and new point to the same pipeline he=
ad, walking old would add the exact same colorop states that were already a=
dded from the new walk (since `drm_atomic_get_colorop_state` is idempotent =
for already-tracked colorops). Good optimization.

**Concern: `drm_atomic_add_affected_planes` behavior change:**

The existing code:
```c
if (plane_state->color_pipeline) {
    ret =3D drm_atomic_add_affected_colorops(state, plane);
```

is replaced with:
```c
ret =3D drm_atomic_add_pipeline_colorops(state, plane);
```

The old code called `drm_atomic_add_affected_colorops` which adds ALL color=
ops for the plane (not just those in the pipeline). The new code only adds =
pipeline-linked colorops. This is a behavioral narrowing =E2=80=94 if a pla=
ne has colorops that aren't currently part of any pipeline chain, they'll n=
o longer be pulled into the atomic state here. This seems intentional and c=
orrect for the stated goal, but it's worth confirming: are there any driver=
s that rely on non-pipeline colorops being pulled in through `drm_atomic_ad=
d_affected_planes()`?

**Doc update for `drm_atomic_add_affected_colorops`:**

The patch updates the doc comment to narrow the stated purpose to "re-enabl=
ing a plane to avoid special code to force-enable all colorops." This is ap=
propriate since `drm_atomic_add_affected_planes()` no longer calls it.

**Minor nit:** The function comment block for `drm_atomic_add_pipeline_colo=
rops` is quite verbose for a static helper. Not a problem, just an observat=
ion =E2=80=94 the detail is welcome for this kind of subtle semantic change.

Overall: **Looks good.** Clean implementation.

---

---
Generated by Claude Code Patch Reviewer