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 colorop state from active color pipeline
Date: Thu, 07 May 2026 13:05:37 +1000
Message-ID: <review-patch1-20260506192633.16066-2-mwen@igalia.com>
In-Reply-To: <20260506192633.16066-2-mwen@igalia.com>
References: <20260506192633.16066-1-mwen@igalia.com> <20260506192633.16066-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

**Assessment: Good, minor nit**

This patch changes `drm_atomic_add_affected_colorops()` to take a `drm_plan=
e_state *` instead of `drm_atomic_state *`, and walks only the active `colo=
r_pipeline` linked list instead of iterating all colorops on the device and=
 filtering by plane.

The key behavioral change is sound =E2=80=94 previously `drm_for_each_color=
op(colorop, plane->dev)` iterated *all* colorops across *all* planes on the=
 device, filtering by `colorop->plane !=3D plane`. Now it walks only the ac=
tive pipeline chain:

```c
for (colorop =3D plane_state->color_pipeline;
     colorop;
     colorop =3D colorop->next) {
```

This is both more efficient and more correct (no states get created for col=
orops from inactive pipelines).

The NULL guard at the top is clean:
```c
if (!plane_state || !plane_state->color_pipeline)
    return 0;
```

The callers in `drm_atomic_add_affected_planes()` and `drm_atomic_helper_du=
plicate_state()` are updated to remove the now-redundant `if (plane_state->=
color_pipeline)` check and pass `plane_state` instead of `state`.

**Nit**: The kernel-doc parameter name changed from `@state` to `@plane_sta=
te`, but the declaration in `drm_atomic.h` still uses the name `state`:
```c
drm_atomic_add_affected_colorops(struct drm_plane_state *state,
```
This is technically valid C, but calling a `drm_plane_state *` parameter "`=
state`" is confusing since "state" in the DRM atomic world typically refers=
 to `drm_atomic_state`. Consider naming it `plane_state` in the header decl=
aration to match the implementation.

The removal of `WARN_ON(!drm_atomic_get_new_plane_state(state, plane))` mak=
es sense since the function now receives the plane_state directly.

---
Generated by Claude Code Patch Reviewer