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

[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 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>
---
 drivers/gpu/drm/drm_atomic.c | 67 +++++++++++++++++++++++++++++++-----
 1 file changed, 58 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 170de30c28ae..4fb3a23e862a 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -812,6 +812,59 @@ 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);
+
+	for (colorop = new_plane_state->color_pipeline;
+	     colorop;
+	     colorop = colorop->next) {
+		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;
+
+	for (colorop = old_plane_state->color_pipeline;
+	     colorop;
+	     colorop = colorop->next) {
+		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)
 {
@@ -1591,11 +1644,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;
 }
@@ -1607,10 +1658,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
-- 
2.53.0

[PATCH 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 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

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

[Alex Hung]

On 5/26/26 08:17, Melissa Wen wrote:
> 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>
> ---
>   drivers/gpu/drm/drm_atomic.c | 67 +++++++++++++++++++++++++++++++-----
>   1 file changed, 58 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 170de30c28ae..4fb3a23e862a 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -812,6 +812,59 @@ 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);
> +
> +	for (colorop = new_plane_state->color_pipeline;
> +	     colorop;
> +	     colorop = colorop->next) {

This for-loop is used 5 times in this patchset. How about a macro in 
drm_colorop.h?

#define drm_for_each_colorop_in_pipeline(colorop, pipeline) \
     for ((colorop) = (pipeline); (colorop); (colorop) = (colorop)->next)

> +		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;
> +
> +	for (colorop = old_plane_state->color_pipeline;
> +	     colorop;
> +	     colorop = colorop->next) {
> +		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)
>   {
> @@ -1591,11 +1644,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;
>   }
> @@ -1607,10 +1658,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

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

[Alex Hung]

On 5/26/26 08:17, Melissa Wen wrote:
> 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;

return should be in a new line.

> +
> +	/* 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) {

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

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

[Claude Code Review Bot]

Patch Review

**Purpose:** Replaces the call to `drm_atomic_add_affected_colorops()` in `drm_atomic_add_affected_planes()` with a new `drm_atomic_add_pipeline_colorops()` that only walks the old and new pipeline chains rather than iterating over every colorop on the plane.

**Code quality:** The new function is well-structured with clear early-exit optimization when old and new pipelines are the same.

**Issue 1 — WARN_ON condition may be too strict:**

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

In `drm_atomic_add_affected_planes()`, `drm_atomic_get_plane_state()` was just called, which creates both old and new plane state entries. So `new_plane_state` being NULL here should indeed be impossible. However, `old_plane_state` is also guaranteed since `drm_atomic_get_plane_state()` always sets both old and new. The WARN_ON is defensive and fine, just noting it's arguably unnecessary.

**Issue 2 — docstring update in `drm_atomic_add_affected_colorops` is incomplete:**

The patch updates the doc comment for `drm_atomic_add_affected_colorops()`:

```c
- * 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
+ * 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.
```

But the function still adds **all** colorops for the plane (via `drm_for_each_colorop`), not just pipeline-active ones. This is correct for its remaining caller (`drm_atomic_helper_duplicate_state` in Patch 2), but the doc should clarify that this adds *all* colorops, not just those in the active pipeline. This would help future readers understand the distinction from `drm_atomic_add_pipeline_colorops`.

**Otherwise looks correct.** The implementation properly handles the pipeline transition case (old pipeline != new pipeline) by walking both chains.

---

---
Generated by Claude Code Patch Reviewer

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

[Claude Code Review Bot]

Patch Review

**Purpose:** In `drm_atomic_helper_duplicate_state()`, remove the `if (plane_state->color_pipeline)` guard so that **all** colorops are snapshotted regardless of whether a pipeline is currently active. This ensures colorop settings persist across deactivation/reactivation.

**The change is straightforward and correct:**

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

**Minor concern — interaction with Patch 1:**  Patch 1 introduces `drm_atomic_add_pipeline_colorops()` for use in `drm_atomic_add_affected_planes()` (which only adds active/transitioning colorops), while this patch keeps `drm_atomic_add_affected_colorops()` here (which adds all colorops on the plane). The two functions serve legitimately different purposes, but having two similarly-named functions with different semantics could be confusing. A comment in the code or in the commit message acknowledging this intentional distinction would help.

**No bugs found.** The change correctly ensures persistence of colorop state across pipeline deactivation.

---

---
Generated by Claude Code Patch Reviewer

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