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

[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

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: 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

[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

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

[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

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

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

[PATCH 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 some bug fixes [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/dri-devel/20260525100524.304263-1-mwen@igalia.com/
[3] https://lore.kernel.org/igt-dev/20260526140752.503380-1-mwen@igalia.com/

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        | 126 ++++++++++++++++++++++++++--
 drivers/gpu/drm/drm_atomic_helper.c |   9 +-
 2 files changed, 120 insertions(+), 15 deletions(-)

-- 
2.53.0

[PATCH 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 | 59 ++++++++++++++++++++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 4fb3a23e862a..a0549435954b 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -865,6 +865,54 @@ 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)
 {
@@ -1714,6 +1762,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;
@@ -1730,6 +1780,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] is not part of 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

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

[Claude Code Review Bot]

Patch Review

**Purpose:** Adds `drm_atomic_colorop_check()` to validate that any colorop being modified in an atomic commit belongs to an active (or transitioning) pipeline, called from `drm_atomic_check_only()`.

**Issue 1 (Significant) — Direct access to `plane->state->color_pipeline` without the atomic state:**

```c
/* 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;
}
```

Accessing `plane->state` directly (the currently committed hardware state) is problematic. In the DRM atomic framework, `plane->state` is the committed state pointer and is generally only safe to dereference when holding the appropriate locks. Within `drm_atomic_check_only()`, we're in an atomic check context and the plane's lock may not be held if the plane wasn't added to this atomic commit. The `drm_atomic_get_new_plane_state()` returning NULL means this plane wasn't part of the commit at all — but the colorop somehow is, which is an unusual state.

Consider whether this case can actually occur given the call flow. If userspace sets a colorop property, `drm_atomic_get_colorop_state()` should have been called, which calls `drm_atomic_get_plane_state()` on `colorop->plane` (looking at the standard implementation). If that's the case, the plane would always be in the atomic state when a colorop is, making this `!new_plane_state` branch dead code. If it can't be reached, a `WARN_ON` return would be more appropriate than silently walking `plane->state`.

If this branch *can* be reached, then `plane->state` access needs careful consideration regarding locking.

**Issue 2 — Coding style:**

```c
if (WARN_ON(!old_plane_state)) return -EINVAL;
```

The kernel coding style requires the return statement on a separate line:

```c
if (WARN_ON(!old_plane_state))
    return -EINVAL;
```

**Issue 3 — Placement of the colorop check 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] is not part of an active color pipeline.\n",
+				       colorop->base.id, colorop->type);
+			return ret;
+		}
+	}
```

The check is placed *before* the plane checks (`for_each_oldnew_plane_in_state`). This is intentional per the commit message ("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"). The placement before plane checks seems reasonable — it's a precondition check.

**Issue 4 — `drm_dbg_atomic` format string:**

```c
drm_dbg_atomic(dev, "[COLOROP:%d:%d] is not part of an active color pipeline.\n",
               colorop->base.id, colorop->type);
```

Logging `colorop->type` (an enum) as `%d` works but is unusual. Other DRM debug messages typically print `[OBJECT:id:name]`. Since colorops don't have a name field, this is acceptable, but using the type enum integer may not be very informative for debugging. Consider whether there's a type-to-string helper.

**Issue 5 — Should the old pipeline check allow modifications on deactivation?**

The check allows modifying colorops that belong to the old pipeline (the one being deactivated):

```c
/* 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;
```

This means during pipeline deactivation (old has pipeline, new is NULL), userspace can still modify colorops of the old pipeline in the same commit. The cover letter says this is intentional ("when activating or deactivating its pipeline in the same commit"). This seems correct — userspace might want to set up colorop values while deactivating, so they're ready for future reactivation (relying on persistence from Patch 2).

**Overall for Patch 3:** The main concern is the `plane->state` direct access and whether it can actually be reached. The coding style issue with the single-line `if (WARN_ON(...)) return` should be fixed.

---
Generated by Claude Code Patch Reviewer

[PATCH v6 0/6] drm/atomic: track individual colorop updates

[Melissa Wen]

This series aims to track updates for each individual color operation,
allowing the driver to react accordingly.

- Patches 1 and 2 make colorop update process more consistent and
  optimized by only keeping colorop states from active color pipelines.
  Due to ordering dependency, attempts to update inactive colorops are
  not rejected at property setting time, but only later during atomic
  check.

- Patches 3 and 4 make lut1d_interpolation and lut3d_interpolation
  colorops correctly behave as mutable, handling their changes via
  drm_colorop_state.

- Finally, patches 5 and 6 track colorop updates of a given plane color
  pipeline by setting plane `color_mgmt_changed` flag, similar to what
  is done for tracking CRTC color mgmt property changes with CRTC
  `color_mgmt_changed` flag. The flag also tracks when a different color
  pipeline is set to a given plane. That way, the driver can react
  accordingly and update their color blocks.

It also fixes shaper/3D LUT updates when changing night mode settings on
gamescope with a custom branch that supports `COLOR_PIPELINE`:
- https://github.com/ValveSoftware/gamescope/pull/2113

v1: https://lore.kernel.org/dri-devel/20260318162348.299807-1-mwen@igalia.com/
Changes:
- include linux types for function's bool return type (kernel bot on MSM
  driver)
- add Harry's r-b tags

v2: https://lore.kernel.org/dri-devel/20260323131942.494217-1-mwen@igalia.com/
Changes:
- [NEW] two patches to only consider colorop updates from active color
  pipelines (Chaitanya)
- [NEW] make lut interpolation properties mutable + Alex H patch for
  kernel docs
- track lut(1/3)d_interpolation updates (Chaitanya)
- rebase changes according to new patches

v3: https://lore.kernel.org/dri-devel/20260403135909.214378-1-mwen@igalia.com/
Changes: rebase on drm-misc-next

v4: https://lore.kernel.org/dri-devel/20260501132527.522320-1-mwen@igalia.com/
Changes: fix kernel doc (kernel bot)

v5: https://lore.kernel.org/dri-devel/20260506192633.16066-1-mwen@igalia.com/
Changes:
- rebase on drm-misc-next
- fix kernel-doc and correctly reword (atomic) state to plane_state (Chaitanya)
- reject inactive colorop updates in atomic check time, instead of
  during property's setup, to avoid ordering dependency as pointed out by Chaitanya
- use `|= replaced` for consistency (Chaitanya)
- add Chaitanya's r-b tags to patches 1,3-5

Melissa Wen

Alex Hung (1):
  drm/colorop: Remove read-only comments from interpolation fields

Melissa Wen (5):
  drm/atomic: only add colorop state from active color pipeline
  drm/atomic: reject colorop update from inactive color pipeline
  drm/colorop: make lut(1/3)d_interpolation mutable
  drm/atomic: track individual colorop updates
  drm/amd/display: use plane color_mgmt_changed to track colorop changes

 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  6 +-
 drivers/gpu/drm/drm_atomic.c                  | 82 ++++++++++++++-----
 drivers/gpu/drm/drm_atomic_helper.c           |  9 +-
 drivers/gpu/drm/drm_atomic_uapi.c             | 68 +++++++++++----
 drivers/gpu/drm/drm_colorop.c                 | 16 +++-
 include/drm/drm_atomic.h                      |  2 +-
 include/drm/drm_atomic_uapi.h                 |  4 +-
 include/drm/drm_colorop.h                     | 34 ++++----
 8 files changed, 155 insertions(+), 66 deletions(-)

-- 
2.53.0

[PATCH v6 2/6] drm/atomic: reject colorop update from inactive color pipeline

[Melissa Wen]

Only allow updates on colorops that are part of an active pipeline.
Check if a colorop in a new state belongs to a color pipeline which was
set as a plane color_pipeline property and therefore is an active color
pipeline. If not, reject the atomic state. 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.

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

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 28831a548b0c..659cf56150e5 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -812,6 +812,33 @@ static int drm_atomic_plane_check(const struct drm_plane_state *old_plane_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 a plane state.
+ *
+ * Returns: 0 on success, -EINVAL otherwise.
+ */
+static int drm_atomic_colorop_check(const struct drm_colorop_state *new_colorop_state)
+{
+	struct drm_colorop *colorop, *color_pipeline;
+	struct drm_plane_state *new_plane_state;
+
+	new_plane_state = drm_atomic_get_new_plane_state(new_colorop_state->state,
+							 new_colorop_state->colorop->plane);
+	color_pipeline = new_plane_state ? new_plane_state->color_pipeline :
+			 new_colorop_state->colorop->plane->state->color_pipeline;
+
+	for (colorop = color_pipeline; colorop; colorop = colorop->next)
+		if (colorop == new_colorop_state->colorop)
+			return 0;
+
+	return -EINVAL;
+}
+
 static void drm_atomic_colorop_print_state(struct drm_printer *p,
 					   const struct drm_colorop_state *state)
 {
@@ -1665,6 +1692,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;
@@ -1681,6 +1710,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] is not part of 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

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

[Claude Code Review Bot]

Patch Review

**Status: Minor observation, otherwise good**

Adds `drm_atomic_colorop_check()` that verifies every colorop in the new state belongs to the active color pipeline. The check is done in `drm_atomic_check_only()` to avoid ordering dependencies at property-set time.

The fallback logic is reasonable:
```c
color_pipeline = new_plane_state ? new_plane_state->color_pipeline :
                 new_colorop_state->colorop->plane->state->color_pipeline;
```

When the plane isn't part of this atomic commit, it reads from the current committed state. This is safe because `drm_atomic_check_only` runs with appropriate locking.

**Minor**: The function returns `-EINVAL` with only a `drm_dbg_atomic` message. Since this is a user-visible error path (userspace set a colorop property on a colorop not in the active pipeline), the error code seems appropriate. It might be helpful to also log which plane and pipeline were expected, but that's a polish item.

---

---
Generated by Claude Code Patch Reviewer