Claude review: don't allow changes to inactive colorops

Claude review: don't allow changes to inactive colorops

[Claude Code Review Bot]

Overall Series Review

Subject: don't allow changes to inactive colorops
Author: Melissa Wen <mwen@igalia.com>
Patches: 6
Reviewed: 2026-05-27T14:45:24.998890

---

This 3-patch series implements a policy for DRM color pipeline colorops: userspace can only modify colorops belonging to an active (or transitioning) pipeline, and colorop state should persist across pipeline deactivation/reactivation cycles. The motivation is clear and well-documented in the cover letter, following discussion on dri-devel and #wayland IRC.

The series is logically structured: Patch 1 narrows which colorops get added to the atomic state during `drm_atomic_add_affected_planes`, Patch 2 ensures `drm_atomic_helper_duplicate_state` snapshots all colorops (for persistence), and Patch 3 adds the validation check in `drm_atomic_check_only`.

The design is generally sound but there are a few issues worth addressing, particularly around accessing `plane->state` directly in Patch 3 (potential race/unsafe access), and the interaction between Patches 1 and 2 creating a subtle inconsistency in how "all colorops" vs "pipeline colorops" are treated.

---

---
Generated by Claude Code Patch Reviewer

[PATCH v2 0/3] don't allow changes to inactive colorops

[Melissa Wen]

This series is a follow-up of what was discussed in [1] and on #wayland
IRC channel regarding policy and userspace expectations on changes in
colorop properties and the current status of the color pipeline in which
the colorop is part of. In short, we agreed that userspace can change
properties of colorops that are currently part of an active color
pipeline or when the pipeline is switching status in the same commit.
However, userspace cannot change colorop properties of inactive color
pipeline in the expactation that it will be activated at some point in
the future.

Userspace also expects persistence of color pipeline already set, even
if it becomes inactive for a while, when activated, colorop settings
previouly set should be preserved.

In addition, I found some bugs on IGT tests when this policy is applied.
So I sent bug fixes to kms_colorop and kms_properties to behave
according to this contract [2]. The rest of the series in [1] was
detached in [3] since there is no dependecy between them.

[1] https://lore.kernel.org/dri-devel/20260519211111.228303-1-mwen@igalia.com/
[2] https://lore.kernel.org/igt-dev/20260602211259.898147-1-mwen@igalia.com
[3] https://lore.kernel.org/igt-dev/20260526140752.503380-1-mwen@igalia.com/


[v1] https://lore.kernel.org/dri-devel/20260526142940.504911-1-mwen@igalia.com/
Changes:
- define a macro to walk in the color pipeline (Alex H.)
- fix checkpatch warning (Alex H.)

Melissa

Melissa Wen (3):
  drm/atomic: only add states of active or transient active colorops
  drm/atomic: duplicate state of all colorops
  drm/atomic: reject colorop update from inactive color pipeline

 drivers/gpu/drm/drm_atomic.c        | 125 ++++++++++++++++++++++++++--
 drivers/gpu/drm/drm_atomic_helper.c |   9 +-
 include/drm/drm_colorop.h           |   3 +
 3 files changed, 122 insertions(+), 15 deletions(-)

-- 
2.53.0

[PATCH v2 1/3] drm/atomic: only add states of active or transient active colorops

[Melissa Wen]

Only consider affected colorop states those that are part of an active
color pipeline or a pipeline that is about to be activated or
deactivated in the same atomic commit, i.e., colorop is in the chain of
old/new plane color pipeline property. To cover color_pipeline
deactivation, remove the condition for plane_state->color_pipeline.

Signed-off-by: Melissa Wen <mwen@igalia.com>
---

v2: define a macro to walk in the color pipeline (Alex H.)
---
 drivers/gpu/drm/drm_atomic.c | 65 +++++++++++++++++++++++++++++++-----
 include/drm/drm_colorop.h    |  3 ++
 2 files changed, 59 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 3af1b9cc9a06..464562861408 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -892,6 +892,57 @@ static int drm_atomic_plane_check(const struct drm_plane_state *old_plane_state,
 	return 0;
 }
 
+/*
+ * This function walks old and new plane state color pipelines and adds all
+ * colorops in use by @plane to the atomic configuration @state. This is useful
+ * when an atomic commit needs to check all currently enabled or about to be
+ * enabled colorop on @plane, e.g. when changing the mode. This also avoids
+ * including colorop states that are not part of the atomic state.
+ *
+ * Returns:
+ * 0 on success or can fail with -EDEADLK or -ENOMEM. When the error is EDEADLK
+ * then the w/w mutex code has detected a deadlock and the entire atomic
+ * sequence must be restarted. All other errors are fatal.
+ */
+static int
+drm_atomic_add_pipeline_colorops(struct drm_atomic_commit *state,
+				 struct drm_plane *plane)
+{
+	struct drm_colorop *colorop;
+	struct drm_colorop_state *colorop_state;
+	struct drm_plane_state *new_plane_state, *old_plane_state;
+
+	new_plane_state = drm_atomic_get_new_plane_state(state, plane);
+	old_plane_state = drm_atomic_get_old_plane_state(state, plane);
+
+	if (WARN_ON(!new_plane_state || !old_plane_state))
+		return -EINVAL;
+
+	drm_dbg_atomic(plane->dev,
+		       "Adding old+new pipeline colorops for [PLANE:%d:%s]\n",
+		       plane->base.id, plane->name);
+
+	drm_for_each_colorop_in_pipeline(colorop,
+					 new_plane_state->color_pipeline) {
+		colorop_state = drm_atomic_get_colorop_state(state, colorop);
+		if (IS_ERR(colorop_state))
+			return PTR_ERR(colorop_state);
+	}
+
+	/* Same color pipeline as new; no point walking old. */
+	if (new_plane_state->color_pipeline == old_plane_state->color_pipeline)
+		return 0;
+
+	drm_for_each_colorop_in_pipeline(colorop,
+					 old_plane_state->color_pipeline) {
+		colorop_state = drm_atomic_get_colorop_state(state, colorop);
+		if (IS_ERR(colorop_state))
+			return PTR_ERR(colorop_state);
+	}
+
+	return 0;
+}
+
 static void drm_atomic_colorop_print_state(struct drm_printer *p,
 					   const struct drm_colorop_state *state)
 {
@@ -1671,11 +1722,9 @@ drm_atomic_add_affected_planes(struct drm_atomic_commit *state,
 		if (IS_ERR(plane_state))
 			return PTR_ERR(plane_state);
 
-		if (plane_state->color_pipeline) {
-			ret = drm_atomic_add_affected_colorops(state, plane);
-			if (ret)
-				return ret;
-		}
+		ret = drm_atomic_add_pipeline_colorops(state, plane);
+		if (ret)
+			return ret;
 	}
 	return 0;
 }
@@ -1687,10 +1736,8 @@ EXPORT_SYMBOL(drm_atomic_add_affected_planes);
  * @plane: DRM plane
  *
  * This function walks the current configuration and adds all colorops
- * currently used by @plane to the atomic configuration @state. This is useful
- * when an atomic commit also needs to check all currently enabled colorop on
- * @plane, e.g. when changing the mode. It's also useful when re-enabling a plane
- * to avoid special code to force-enable all colorops.
+ * currently used by @plane to the atomic configuration @state. It's useful
+ * when re-enabling a plane to avoid special code to force-enable all colorops.
  *
  * Since acquiring a colorop state will always also acquire the w/w mutex of the
  * current plane for that colorop (if there is any) adding all the colorop states for
diff --git a/include/drm/drm_colorop.h b/include/drm/drm_colorop.h
index b4b9e4f558ab..006278e21d49 100644
--- a/include/drm/drm_colorop.h
+++ b/include/drm/drm_colorop.h
@@ -459,6 +459,9 @@ static inline unsigned int drm_colorop_index(const struct drm_colorop *colorop)
 #define drm_for_each_colorop(colorop, dev) \
 	list_for_each_entry(colorop, &(dev)->mode_config.colorop_list, head)
 
+#define drm_for_each_colorop_in_pipeline(colorop, pipeline) \
+	for ((colorop) = (pipeline); (colorop); (colorop) = (colorop)->next)
+
 /**
  * drm_get_colorop_type_name - return a string for colorop type
  * @type: colorop type to compute name of
-- 
2.53.0

[PATCH v2 2/3] drm/atomic: duplicate state of all colorops

[Melissa Wen]

Userspace expects that colorop settings of an inactive color pipeline
persist, so that, when the color pipeline is activated again, colorops
previously set preserves their values when it was deactivated. Colorop
setup is expected to persist even during a suspend/resume. To snapshot
colorop settings correctly, duplicate state of all colorops in a given
plane, independent if color pipeline is active or not.

Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 51f39edc31ed..ea15e58518fd 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -3752,12 +3752,9 @@ drm_atomic_helper_duplicate_state(struct drm_device *dev,
 			goto free;
 		}
 
-		if (plane_state->color_pipeline) {
-			err = drm_atomic_add_affected_colorops(state, plane);
-			if (err)
-				goto free;
-		}
-
+		err = drm_atomic_add_affected_colorops(state, plane);
+		if (err)
+			goto free;
 	}
 
 	drm_connector_list_iter_begin(dev, &conn_iter);
-- 
2.53.0

[PATCH v2 3/3] drm/atomic: reject colorop update from inactive color pipeline

[Melissa Wen]

Only allow updates on colorops that are part of an active pipeline, i.e.
check if a colorop belongs to the color pipeline of a plane in its
current, new or old state. If not, reject the state change of this
inactive colorop. Performing this check later in drm_atomic_check_only()
to remove the ordering dependency that would exist if done at the time
of colorop property setting. Userspace is allowed to change colorops of
an active color pipeline, or when activating or deactivating its
pipeline in the same commit. However, changes in inactive color pipeline
is not allowed.

Suggested-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 drivers/gpu/drm/drm_atomic.c | 60 ++++++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 464562861408..960b52624deb 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -943,6 +943,55 @@ drm_atomic_add_pipeline_colorops(struct drm_atomic_commit *state,
 	return 0;
 }
 
+/**
+ * drm_atomic_colorop_check - check new colorop state
+ * @new_colorop_state: new colorop state to check
+ *
+ * Ensure that the colorop in @new_colorop_state belongs to an active color
+ * pipeline, i.e. it's in the chain of colorops set to the color_pipeline
+ * property of current, old or new plane state.
+ *
+ * Returns: 0 on success, -EINVAL otherwise.
+ */
+static int drm_atomic_colorop_check(const struct drm_colorop_state *new_colorop_state)
+{
+	struct drm_atomic_commit *state = new_colorop_state->state;
+	struct drm_plane *plane = new_colorop_state->colorop->plane;
+	struct drm_plane_state *new_plane_state, *old_plane_state;
+	struct drm_colorop *colorop;
+
+	new_plane_state = drm_atomic_get_new_plane_state(state, plane);
+	old_plane_state = drm_atomic_get_old_plane_state(state, plane);
+
+	/* No changes in the plane state. Check current-committed plane state */
+	if (!new_plane_state) {
+		for (colorop = plane->state->color_pipeline; colorop; colorop = colorop->next)
+			if (colorop == new_colorop_state->colorop)
+				return 0;
+		return -EINVAL;
+	}
+
+	if (WARN_ON(!old_plane_state))
+		return -EINVAL;
+
+	/* Check if the colorop is active in the new plane state */
+	for (colorop = new_plane_state->color_pipeline; colorop; colorop = colorop->next)
+		if (colorop == new_colorop_state->colorop)
+			return 0;
+
+	/* Same color pipeline as new; no point walking old. Colorop isn't active */
+	if (new_plane_state->color_pipeline == old_plane_state->color_pipeline)
+		return -EINVAL;
+
+	/* Check if the colorop was active in the old plane state */
+	for (colorop = old_plane_state->color_pipeline; colorop; colorop = colorop->next)
+		if (colorop == new_colorop_state->colorop)
+			return 0;
+
+	/* Colorop is not part of an active color pipeline. */
+	return -EINVAL;
+}
+
 static void drm_atomic_colorop_print_state(struct drm_printer *p,
 					   const struct drm_colorop_state *state)
 {
@@ -1792,6 +1841,8 @@ int drm_atomic_check_only(struct drm_atomic_commit *state)
 	struct drm_plane *plane;
 	struct drm_plane_state *old_plane_state;
 	struct drm_plane_state *new_plane_state;
+	struct drm_colorop *colorop;
+	struct drm_colorop_state *new_colorop_state;
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *old_crtc_state;
 	struct drm_crtc_state *new_crtc_state;
@@ -1808,6 +1859,15 @@ int drm_atomic_check_only(struct drm_atomic_commit *state)
 			requested_crtc |= drm_crtc_mask(crtc);
 	}
 
+	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;
+		}
+	}
+
 	for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
 		ret = drm_atomic_plane_check(old_plane_state, new_plane_state);
 		if (ret) {
-- 
2.53.0

Re: [PATCH v2 1/3] drm/atomic: only add states of active or transient active colorops

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Adding old pipeline colorops to the atomic state on deactivation causes the AMD driver to incorrectly detect a deactivated pipeline as active.

Pre-existing issues:
- [High] Modifying a COLOROP property via the ATOMIC IOCTL fails to pull the associated plane into the atomic state, causing the update to be silently ignored by the driver.
- [High] Changing the COLOR_PIPELINE plane property does not pull the affected colorops into the atomic state unless a CRTC modeset is concurrently triggered.
--

commit c677eb54425763294f2391cd91e9253b70b8e880
Author: Melissa Wen <mwen@igalia.com>

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

Only consider affected colorop states those that are part of an active
color pipeline or a pipeline that is about to be activated or
deactivated in the same atomic commit, i.e., colorop is in the chain of
old/new plane color pipeline property. To cover color_pipeline
deactivation, remove the condition for plane_state->color_pipeline.

> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 170de30c28ae4..0d36a955c4d43 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c

[ ... ]

> @@ -812,6 +812,57 @@ static int drm_atomic_plane_check(const struct drm_plane_state *old_plane_state,
>  	return 0;
>  }
>  
> +/*
> + * This function walks old and new plane state color pipelines and adds all
> + * colorops in use by @plane to the atomic configuration @state. This is useful
> + * when an atomic commit needs to check all currently enabled or about to be
> + * enabled colorop on @plane, e.g. when changing the mode. This also avoids
> + * including colorop states that are not part of the atomic state.
> + *
> + * Returns:
> + * 0 on success or can fail with -EDEADLK or -ENOMEM. When the error is EDEADLK
> + * then the w/w mutex code has detected a deadlock and the entire atomic
> + * sequence must be restarted. All other errors are fatal.
> + */
> +static int
> +drm_atomic_add_pipeline_colorops(struct drm_atomic_commit *state,
> +				 struct drm_plane *plane)
> +{
> +	struct drm_colorop *colorop;
> +	struct drm_colorop_state *colorop_state;
> +	struct drm_plane_state *new_plane_state, *old_plane_state;
> +
> +	new_plane_state = drm_atomic_get_new_plane_state(state, plane);
> +	old_plane_state = drm_atomic_get_old_plane_state(state, plane);
> +
> +	if (WARN_ON(!new_plane_state || !old_plane_state))
> +		return -EINVAL;
> +
> +	drm_dbg_atomic(plane->dev,
> +		       "Adding old+new pipeline colorops for [PLANE:%d:%s]\n",
> +		       plane->base.id, plane->name);
> +
> +	drm_for_each_colorop_in_pipeline(colorop,
> +					 new_plane_state->color_pipeline) {
> +		colorop_state = drm_atomic_get_colorop_state(state, colorop);

[Severity: High]
This is a pre-existing issue, but does modifying a colorop property via the
atomic ioctl fail to pull the associated plane into the atomic state?

The call to drm_atomic_get_colorop_state() successfully fetches and adds the
colorop to the state:

drivers/gpu/drm/drm_atomic.c:drm_atomic_get_colorop_state() {
    ...
    ret = drm_modeset_lock(&colorop->plane->mutex, state->acquire_ctx);
    colorop_state = drm_atomic_helper_colorop_duplicate_state(colorop);
    ...
}

However, it does not call drm_atomic_get_plane_state() to add the parent plane.
When the standard driver commit path executes, it iterates only over planes
present in the state:

drivers/gpu/drm/drm_atomic_helper.c:drm_atomic_helper_commit_planes() {
    ...
    for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
        ...
        funcs->atomic_update(plane, state);
    ...
}

Does this mean the driver's atomic_update hook is skipped, and the hardware is
not programmed with the new colorop parameters unless userspace also modifies a
plane property in the same commit?

> +		if (IS_ERR(colorop_state))
> +			return PTR_ERR(colorop_state);
> +	}
> +
> +	/* Same color pipeline as new; no point walking old. */
> +	if (new_plane_state->color_pipeline == old_plane_state->color_pipeline)
> +		return 0;
> +
> +	drm_for_each_colorop_in_pipeline(colorop,
> +					 old_plane_state->color_pipeline) {
> +		colorop_state = drm_atomic_get_colorop_state(state, colorop);

[Severity: High]
Does adding the old pipeline's colorops to the atomic state during deactivation
cause issues with active pipeline detection?

When the pipeline is being deactivated, the old colorops' bypass flags are
untouched and remain false.

In the AMD driver, dm_plane_color_pipeline_active() checks all colorops added
to the state:

drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c:dm_plane_color_pipeline_active() {
    ...
    for_each_oldnew_colorop_in_state(state, colorop, old_colorop_state, new_colorop_state, i) {
        ...
        if (!cstate->bypass)
            return true;
    }
}

Will this cause the driver to see the old colorops, incorrectly evaluate the
deactivated pipeline as active, and attempt to program disabled hardware?

> +		if (IS_ERR(colorop_state))
> +			return PTR_ERR(colorop_state);
> +	}
> +
> +	return 0;
> +}

[ ... ]

> @@ -1591,11 +1642,9 @@ drm_atomic_add_affected_planes(struct drm_atomic_commit *state,
>  		if (IS_ERR(plane_state))
>  			return PTR_ERR(plane_state);
>  
> -		if (plane_state->color_pipeline) {
> -			ret = drm_atomic_add_affected_colorops(state, plane);
> -			if (ret)
> -				return ret;
> -		}
> +		ret = drm_atomic_add_pipeline_colorops(state, plane);

[Severity: High]
This is a pre-existing issue, but does changing the color pipeline plane
property fail to pull the affected colorops into the atomic state if a CRTC
modeset isn't triggered?

The drm_atomic_add_pipeline_colorops() function is invoked here from
drm_atomic_add_affected_planes(). However, this is only called when a modeset
is occurring:

drivers/gpu/drm/drm_atomic_helper.c:drm_atomic_helper_check_modeset() {
    ...
    if (!drm_atomic_crtc_needs_modeset(new_crtc_state))
        continue;
    ...
    ret = drm_atomic_add_affected_planes(state, crtc);
}

If userspace dynamically changes the color pipeline property on a plane as a
standalone update without triggering a full CRTC modeset, will this bypass
drm_atomic_add_affected_planes() entirely?

Could this result in the colorops for the newly activated or deactivated
pipeline never being added to the atomic commit state?

> +		if (ret)
> +			return ret;
>  	}
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260604180457.1110110-1-mwen@igalia.com?part=1

Claude review: don't allow changes to inactive colorops

[Claude Code Review Bot]

Overall Series Review

Subject: don't allow changes to inactive colorops
Author: Melissa Wen <mwen@igalia.com>
Patches: 5
Reviewed: 2026-06-05T06:13:38.976779

---

This is a well-structured 3-patch series from Melissa Wen that establishes a UAPI contract for DRM color pipeline management: userspace may only modify colorop properties when the color pipeline is active (or transitioning active/inactive in the same commit), and colorop settings must persist across deactivation/reactivation cycles. The series is clearly motivated by real userspace expectations discussed with compositors on the Wayland IRC channel and on dri-devel.

The approach is sound:
1. Patch 1 narrows which colorops get added to atomic state (only those in the old/new pipeline) when checking affected planes.
2. Patch 2 ensures state duplication (suspend/resume) captures all colorops for persistence.
3. Patch 3 adds the enforcement check in `drm_atomic_check_only()` that rejects colorop modifications on inactive pipelines.

The series is generally clean and well-commented, with good V2 refinements (the `drm_for_each_colorop_in_pipeline` macro). There are a few issues worth discussing, detailed below.

---

---
Generated by Claude Code Patch Reviewer

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

[Claude Code Review Bot]

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

Claude review: drm/atomic: duplicate state of all colorops

[Claude Code Review Bot]

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

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

[Claude Code Review Bot]

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