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: duplicate state of all colorops
Date: Fri, 05 Jun 2026 06:13:39 +1000
Message-ID: <review-patch2-20260604180457.1110110-3-mwen@igalia.com>
In-Reply-To: <20260604180457.1110110-3-mwen@igalia.com>
References: <20260604180457.1110110-1-mwen@igalia.com> <20260604180457.1110110-3-mwen@igalia.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0

Patch Review

**Purpose:** Ensure `drm_atomic_helper_duplicate_state()` duplicates ALL colorops for a plane, not just those in the active pipeline, so that colorop settings persist across deactivation.

The change:
```c
// Before:
if (plane_state->color_pipeline) {
    err = drm_atomic_add_affected_colorops(state, plane);
    if (err)
        goto free;
}

// After:
err = drm_atomic_add_affected_colorops(state, plane);
if (err)
    goto free;
```

This unconditionally calls `drm_atomic_add_affected_colorops()`, which walks ALL colorops for the plane (the original function that iterates `drm_for_each_colorop` across the entire device, filtering by `colorop->plane`). This ensures that even colorops not in the current pipeline get their state duplicated.

**This is correct for the stated goal:** suspend/resume needs the full snapshot so that when a pipeline is re-activated, all the previously-configured colorop states are preserved.

**Interaction with Patch 1:** Note that Patch 1 changed `drm_atomic_add_affected_planes()` to use the new `drm_atomic_add_pipeline_colorops()` (narrower). This patch keeps using the broader `drm_atomic_add_affected_colorops()` in `drm_atomic_helper_duplicate_state()`. The distinction is intentional: the duplicate-state path needs everything; the affected-planes path only needs pipeline-active colorops. This is a clean separation.

**Possible concern:** The removed blank line between `goto free; }` and `}` (the closing brace of the `drm_for_each_plane` loop) results in slightly tighter formatting. This is fine.

Overall: **Looks good.** Simple, correct, well-motivated.

---

---
Generated by Claude Code Patch Reviewer