Claude review: drm/atomic: reject colorop update from inactive color pipeline

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

Patch Review

**Purpose:** Add enforcement in `drm_atomic_check_only()` that rejects attempts to modify colorops that don't belong to an active (old or new) color pipeline.

**`drm_atomic_colorop_check()` function:**

```c
static int drm_atomic_colorop_check(const struct drm_colorop_state *new_colorop_state)
```

The function checks three paths:
1. No plane state in the atomic commit → check the current committed `plane->state->color_pipeline`
2. Colorop in new plane state's pipeline → allowed
3. Colorop in old plane state's pipeline (if different from new) → allowed (covers deactivation case)

**Potential issue — accessing `plane->state` without the atomic state lock:**

```c
if (!new_plane_state) {
    for (colorop = plane->state->color_pipeline; colorop; colorop = colorop->next)
        if (colorop == new_colorop_state->colorop)
            return 0;
    return -EINVAL;
}
```

When `new_plane_state` is NULL (meaning the plane wasn't part of this atomic commit), the code reads `plane->state->color_pipeline` — the current committed state. This is the standard pattern in DRM atomic for reading committed state when a plane hasn't been touched by the current commit. However, the colorop state IS in the commit (that's how we got here via `for_each_new_colorop_in_state`), so the colorop's plane mutex should already be held (since `drm_atomic_get_colorop_state()` acquires `colorop->plane->mutex`). This means `plane->state` is safe to read. **This looks correct.**

**Question about the `!new_plane_state` path semantics:**

If userspace modifies a colorop without also touching the plane's color_pipeline property in the same commit, does the colorop end up in the atomic state? The answer is yes — if userspace sets a colorop property, `drm_atomic_get_colorop_state()` is called during property setting, which adds it to the state. The plane state might not be in the commit though (if only the colorop property was touched, not the plane's color_pipeline). In that case this function checks the committed `plane->state->color_pipeline`. If the colorop is part of the currently-committed active pipeline, the change is allowed. This is the correct behavior.

**Not using `drm_for_each_colorop_in_pipeline` macro in this patch:**

Patch 3 does manual `for` loops:
```c
for (colorop = plane->state->color_pipeline; colorop; colorop = colorop->next)
```

rather than using the `drm_for_each_colorop_in_pipeline` macro introduced in Patch 1. This is inconsistent. While the macro is functionally identical, using it would be cleaner and more consistent within the series:

```c
drm_for_each_colorop_in_pipeline(colorop, plane->state->color_pipeline) {
    if (colorop == new_colorop_state->colorop)
        return 0;
}
```

**Placement in `drm_atomic_check_only()`:**

```c
for_each_new_colorop_in_state(state, colorop, new_colorop_state, i) {
    ret = drm_atomic_colorop_check(new_colorop_state);
    if (ret) {
        drm_dbg_atomic(dev, "[COLOROP:%d:%d] isn't in an active color pipeline.\n",
                       colorop->base.id, colorop->type);
        return ret;
    }
}
```

This is placed BEFORE the plane checks and BEFORE `config->funcs->atomic_check()`. The commit message explicitly mentions doing this check here "to remove the ordering dependency that would exist if done at the time of colorop property setting." This is a good design choice — it ensures userspace can set colorop properties and the color_pipeline property in any order within a single atomic commit, and the validation only happens at check time.

**Debug message format:**

```c
drm_dbg_atomic(dev, "[COLOROP:%d:%d] isn't in an active color pipeline.\n",
               colorop->base.id, colorop->type);
```

The second `%d` prints `colorop->type` as a raw integer. This is mildly less readable than using a type name string, but it's consistent with how other DRM debug messages work and the type enum values are well-defined. Fine as-is.

**Question: What happens when `for_each_new_colorop_in_state` iterates colorops that were added by `drm_atomic_add_pipeline_colorops` (from Patch 1)?**

In the `drm_atomic_add_affected_planes` → `drm_atomic_add_pipeline_colorops` path, colorops from old and new pipelines are added to the state. These will then be iterated by `for_each_new_colorop_in_state` in `drm_atomic_check_only()`, and `drm_atomic_colorop_check()` will verify they're in an active pipeline — which they will be, since they were added precisely because they're in old/new pipelines. So there's no false-positive rejection. **Correct.**

**Edge case: A colorop that appears in BOTH old and new pipelines** (i.e., the pipeline didn't change, or the colorop is in a shared suffix). `drm_atomic_get_colorop_state` is idempotent — calling it twice for the same colorop just returns the existing state. The check will find it in the new pipeline and return 0 immediately. **Correct.**

Overall: **Looks good**, with one minor style suggestion to use the new `drm_for_each_colorop_in_pipeline` macro for consistency.

---

## Summary

The series is well-designed and implements the agreed-upon UAPI contract cleanly. The three patches have a logical progression: narrow what gets pulled in for checking (patch 1), ensure full persistence for state duplication (patch 2), and enforce the policy (patch 3).

**Issues to address:**
1. **Minor (Patch 3):** The manual `for` loops walking the pipeline should use the `drm_for_each_colorop_in_pipeline` macro introduced in Patch 1 for consistency within the series.

**Questions for the author:**
1. Are there any out-of-tree drivers or userspace clients that currently modify colorops on inactive pipelines? The `-EINVAL` return from Patch 3 could break them. The cover letter mentions IGT fixes were sent separately, which is good.
2. Has this been tested with the pipeline switching from one set of colorops to a different set in a single commit (i.e., both old and new pipelines are non-NULL but different)?

---
Generated by Claude Code Patch Reviewer