Claude review: drm/atomic: only add states of active or transient active colorops

[Claude Code Review Bot <claude-review@example.com>]

Patch Review

**Purpose:** Replace the broad `drm_atomic_add_affected_colorops()` call (which 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) = (pipeline); (colorop); (colorop) = (colorop)->next)
```

This is clean and straightforward. It safely handles `pipeline == 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` — this is fine if the codebase has already renamed the struct (I can see the existing code also uses `drm_atomic_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 always exist when this is called. The WARN_ON is appropriate to flag programming errors.

**Optimization with pointer comparison:**

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

This is correct — if both old and new point to the same pipeline head, walking old would add the exact same colorop states that were already added 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 = drm_atomic_add_affected_colorops(state, plane);
```

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

The old code called `drm_atomic_add_affected_colorops` which adds ALL colorops for the plane (not just those in the pipeline). The new code only adds pipeline-linked colorops. This is a behavioral narrowing — if a plane has colorops that aren't currently part of any pipeline chain, they'll no longer be pulled into the atomic state here. This seems intentional and correct for the stated goal, but it's worth confirming: are there any drivers that rely on non-pipeline colorops being pulled in through `drm_atomic_add_affected_planes()`?

**Doc update for `drm_atomic_add_affected_colorops`:**

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

**Minor nit:** The function comment block for `drm_atomic_add_pipeline_colorops` is quite verbose for a static helper. Not a problem, just an observation — the detail is welcome for this kind of subtle semantic change.

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

---

---
Generated by Claude Code Patch Reviewer