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

[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 1/6] drm/atomic: only add colorop state from active color pipeline

[Melissa Wen]

Instead of adding colorop state of all colorops of a given plane, only
get those from an active color pipeline of this plane.

Reviewed-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
Signed-off-by: Melissa Wen <mwen@igalia.com>

---
v5: fix kernel-doc for plane_state (kernel bot)
v6: correctly reword state to plane_state (Chaitanya)
---
 drivers/gpu/drm/drm_atomic.c        | 40 ++++++++++++++---------------
 drivers/gpu/drm/drm_atomic_helper.c |  9 +++----
 include/drm/drm_atomic.h            |  2 +-
 3 files changed, 24 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 170de30c28ae..28831a548b0c 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1591,26 +1591,26 @@ 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_affected_colorops(plane_state, plane);
+		if (ret)
+			return ret;
 	}
 	return 0;
 }
 EXPORT_SYMBOL(drm_atomic_add_affected_planes);
 
 /**
- * drm_atomic_add_affected_colorops - add colorops for plane
- * @state: atomic state
+ * drm_atomic_add_affected_colorops - add active colorops for plane
+ * @plane_state: DRM plane state
  * @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 a @plane to the atomic plane configuration @plane_state.
+ * It only adds colorops that belong to an active color pipeline, i.e.,
+ * colorops that are in the chain set to the plane's color_pipeline property.
+ * This function is useful when an atomic commit needs to check all currently
+ * enabled colorop on @plane, e.g. when changing the mode; and 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
@@ -1622,23 +1622,23 @@ EXPORT_SYMBOL(drm_atomic_add_affected_planes);
  * sequence must be restarted. All other errors are fatal.
  */
 int
-drm_atomic_add_affected_colorops(struct drm_atomic_commit *state,
+drm_atomic_add_affected_colorops(struct drm_plane_state *plane_state,
 				 struct drm_plane *plane)
 {
 	struct drm_colorop *colorop;
 	struct drm_colorop_state *colorop_state;
 
-	WARN_ON(!drm_atomic_get_new_plane_state(state, plane));
+	if (!plane_state || !plane_state->color_pipeline)
+		return 0;
 
 	drm_dbg_atomic(plane->dev,
-		       "Adding all current colorops for [PLANE:%d:%s] to %p\n",
-		       plane->base.id, plane->name, state);
+		       "Adding all current active colorops for [PLANE:%d:%s] to %p\n",
+		       plane->base.id, plane->name, plane_state->state);
 
-	drm_for_each_colorop(colorop, plane->dev) {
-		if (colorop->plane != plane)
-			continue;
-
-		colorop_state = drm_atomic_get_colorop_state(state, colorop);
+	for (colorop = plane_state->color_pipeline;
+	     colorop;
+	     colorop = colorop->next) {
+		colorop_state = drm_atomic_get_colorop_state(plane_state->state, colorop);
 		if (IS_ERR(colorop_state))
 			return PTR_ERR(colorop_state);
 	}
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 51f39edc31ed..f1638087cdec 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(plane_state, plane);
+		if (err)
+			goto free;
 	}
 
 	drm_connector_list_iter_begin(dev, &conn_iter);
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index 1a80a8cdf269..7bba193cd973 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -919,7 +919,7 @@ int __must_check
 drm_atomic_add_affected_planes(struct drm_atomic_commit *state,
 			       struct drm_crtc *crtc);
 int __must_check
-drm_atomic_add_affected_colorops(struct drm_atomic_commit *state,
+drm_atomic_add_affected_colorops(struct drm_plane_state *plane_state,
 				 struct drm_plane *plane);
 
 int __must_check drm_atomic_check_only(struct drm_atomic_commit *state);
-- 
2.53.0

Claude review: drm/atomic: only add colorop state from active color pipeline

[Claude Code Review Bot]

Patch Review

**Status: Looks good**

Changes `drm_atomic_add_affected_colorops()` to take `struct drm_plane_state *` instead of `struct drm_atomic_commit *`, and walks only the active color pipeline chain (`plane_state->color_pipeline -> next -> next ...`) rather than iterating all colorops globally.

The early-out is clean:
```c
if (!plane_state || !plane_state->color_pipeline)
    return 0;
```

The callers in `drm_atomic.c` and `drm_atomic_helper.c` are updated consistently — the check for `plane_state->color_pipeline` is moved into the callee, simplifying call sites. The extra blank line removal in `drm_atomic_helper.c` (line 188) is cosmetic but harmless.

No issues.

---

---
Generated by Claude Code Patch Reviewer

[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

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

[Borah, Chaitanya Kumar]

On 5/20/2026 2:39 AM, Melissa Wen wrote:
> 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;
> +}
> +

This causes regression in our CI[1].

I looked into it and looks like the following sequence in 
igt@kms_color_pipeline causes the error

         set_color_pipeline_bypass(plane);
         reset_colorops(colorops);
         igt_plane_set_fb(plane, NULL);
         igt_display_commit_atomic(&data->display, 0, NULL);

So this change restricts bypassing/disabling both the pipeline and a 
colorop within it in a single commit.

Also Sashiko had the following to say

"Furthermore, does this unnecessarily restrict UAPI by preventing userspace
from configuring inactive pipelines before enabling them, or from resetting
properties on a pipeline in the same commit that switches away from it?"

So this will also fail a commit which tries to change a pipeline and 
disable the colorops in an old pipeline.

That got me thinking whether the first patch[3] in the series is also 
correct, since it is quite similar to the change[4] I added, where 
colorops are only added to the state when a pipeline is active. In both 
cases, we could end up ignoring colorops that are not part of the 
currently selected pipeline.

[1] 
https://intel-gfx-ci.01.org/tree/intel-xe/xe-pw-166922v1/shard-lnl-5/igt@kms_color_pipeline@plane-ctm3x4@pipe-a-plane-2.html
[2] 
https://sashiko.dev/#/patchset/20260520073827.3395745-3-chaitanya.kumar.borah%40intel.com
[3] 
https://lore.kernel.org/dri-devel/20260519211111.228303-2-mwen@igalia.com/
[4] 
https://lore.kernel.org/dri-devel/148df44d-2456-40e3-8be6-f98b89b7ee4d@amd.com/

P.S. Can you please send the next version to intel-gfx and intel-xe too?

==
Chaitanya
>   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) {

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

[Melissa Wen]

On 21/05/2026 13:00, Borah, Chaitanya Kumar wrote:
>
>
> On 5/20/2026 2:39 AM, Melissa Wen wrote:
>> 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;
>> +}
>> +
>
> This causes regression in our CI[1].
>
> I looked into it and looks like the following sequence in 
> igt@kms_color_pipeline causes the error
>
>         set_color_pipeline_bypass(plane);
>         reset_colorops(colorops);
>         igt_plane_set_fb(plane, NULL);
>         igt_display_commit_atomic(&data->display, 0, NULL);
>
> So this change restricts bypassing/disabling both the pipeline and a 
> colorop within it in a single commit.
>
> Also Sashiko had the following to say
>
> "Furthermore, does this unnecessarily restrict UAPI by preventing 
> userspace
> from configuring inactive pipelines before enabling them, or from 
> resetting
> properties on a pipeline in the same commit that switches away from it?"
>
> So this will also fail a commit which tries to change a pipeline and 
> disable the colorops in an old pipeline.

I wonder if userspace resetting colorops to disable a pipeline or 
configuring colorops before enabling the color pipeline is an expected 
behavior.

For resetting properties, I think I can solve it by taking into account 
old and new state to collect the active colorops, not only the new state.
But if configuring colorops before activate a color pipeline is 
expected, there is no need to have patches 1 and 2, since setting an 
inactive colorop have to be allowed. In that case, the solution is just 
drop both patches from the series.

Melissa

>
> That got me thinking whether the first patch[3] in the series is also 
> correct, since it is quite similar to the change[4] I added, where 
> colorops are only added to the state when a pipeline is active. In 
> both cases, we could end up ignoring colorops that are not part of the 
> currently selected pipeline.
>
> [1] 
> https://intel-gfx-ci.01.org/tree/intel-xe/xe-pw-166922v1/shard-lnl-5/igt@kms_color_pipeline@plane-ctm3x4@pipe-a-plane-2.html
> [2] 
> https://sashiko.dev/#/patchset/20260520073827.3395745-3-chaitanya.kumar.borah%40intel.com
> [3] 
> https://lore.kernel.org/dri-devel/20260519211111.228303-2-mwen@igalia.com/
> [4] 
> https://lore.kernel.org/dri-devel/148df44d-2456-40e3-8be6-f98b89b7ee4d@amd.com/
>
> P.S. Can you please send the next version to intel-gfx and intel-xe too?
>
> ==
> Chaitanya
>>   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) {
>

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

[Melissa Wen]

On 21/05/2026 13:00, Borah, Chaitanya Kumar wrote:
>
>
> On 5/20/2026 2:39 AM, Melissa Wen wrote:
>> 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;
>> +}
>> +
>
> This causes regression in our CI[1].
>
> I looked into it and looks like the following sequence in 
> igt@kms_color_pipeline causes the error
>
>         set_color_pipeline_bypass(plane);
>         reset_colorops(colorops);
>         igt_plane_set_fb(plane, NULL);
>         igt_display_commit_atomic(&data->display, 0, NULL);
>
> So this change restricts bypassing/disabling both the pipeline and a 
> colorop within it in a single commit.

Oops, cc'ing everyone.

"
I wonder if userspace resetting colorops to disable a pipeline or 
configuring colorops before enabling the color pipeline is an expected 
behavior.

For resetting properties, I think I can solve it by taking into account 
old and new state to collect the active colorops, not only the new state.
But if configuring colorops before activate a color pipeline is 
expected, there is no need to have patches 1 and 2, since setting an 
inactive colorop have to be allowed. In that case, the solution is just 
drop both patches from the series.
"

Melissa

>
> Also Sashiko had the following to say
>
> "Furthermore, does this unnecessarily restrict UAPI by preventing 
> userspace
> from configuring inactive pipelines before enabling them, or from 
> resetting
> properties on a pipeline in the same commit that switches away from it?"
>
> So this will also fail a commit which tries to change a pipeline and 
> disable the colorops in an old pipeline.
>
> That got me thinking whether the first patch[3] in the series is also 
> correct, since it is quite similar to the change[4] I added, where 
> colorops are only added to the state when a pipeline is active. In 
> both cases, we could end up ignoring colorops that are not part of the 
> currently selected pipeline.
>
> [1] 
> https://intel-gfx-ci.01.org/tree/intel-xe/xe-pw-166922v1/shard-lnl-5/igt@kms_color_pipeline@plane-ctm3x4@pipe-a-plane-2.html
> [2] 
> https://sashiko.dev/#/patchset/20260520073827.3395745-3-chaitanya.kumar.borah%40intel.com
> [3] 
> https://lore.kernel.org/dri-devel/20260519211111.228303-2-mwen@igalia.com/
> [4] 
> https://lore.kernel.org/dri-devel/148df44d-2456-40e3-8be6-f98b89b7ee4d@amd.com/
>
> P.S. Can you please send the next version to intel-gfx and intel-xe too?
>
> ==
> Chaitanya
>>   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) {
>

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

[PATCH v6 3/6] drm/colorop: Remove read-only comments from interpolation fields

[Melissa Wen]

From: Alex Hung <alex.hung@amd.com>

The lut1d_interpolation and lut3d_interpolation fields and their
associated properties were marked as read-only, but userspace
can set them via drm_atomic_colorop_set_property().

Reviewed-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
Signed-off-by: Alex Hung <alex.hung@amd.com>
---
 include/drm/drm_colorop.h | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/include/drm/drm_colorop.h b/include/drm/drm_colorop.h
index c873199c60da..53a2148082d5 100644
--- a/include/drm/drm_colorop.h
+++ b/include/drm/drm_colorop.h
@@ -309,7 +309,6 @@ struct drm_colorop {
 	/**
 	 * @lut1d_interpolation:
 	 *
-	 * Read-only
 	 * Interpolation for DRM_COLOROP_1D_LUT
 	 */
 	enum drm_colorop_lut1d_interpolation_type lut1d_interpolation;
@@ -317,7 +316,6 @@ struct drm_colorop {
 	/**
 	 * @lut3d_interpolation:
 	 *
-	 * Read-only
 	 * Interpolation for DRM_COLOROP_3D_LUT
 	 */
 	enum drm_colorop_lut3d_interpolation_type lut3d_interpolation;
@@ -325,7 +323,7 @@ struct drm_colorop {
 	/**
 	 * @lut1d_interpolation_property:
 	 *
-	 * Read-only property for DRM_COLOROP_1D_LUT interpolation
+	 * Property for DRM_COLOROP_1D_LUT interpolation
 	 */
 	struct drm_property *lut1d_interpolation_property;
 
@@ -353,7 +351,7 @@ struct drm_colorop {
 	/**
 	 * @lut3d_interpolation_property:
 	 *
-	 * Read-only property for DRM_COLOROP_3D_LUT interpolation
+	 * Property for DRM_COLOROP_3D_LUT interpolation
 	 */
 	struct drm_property *lut3d_interpolation_property;
 
-- 
2.53.0

Claude review: drm/colorop: Remove read-only comments from interpolation fields

[Claude Code Review Bot]

Patch Review

**Status: Looks good**

Straightforward documentation fix by Alex Hung. Removes "Read-only" from kernel-doc comments for `lut1d_interpolation`, `lut3d_interpolation`, `lut1d_interpolation_property`, and `lut3d_interpolation_property` since these are now settable by userspace.

No issues.

---

---
Generated by Claude Code Patch Reviewer

[PATCH v6 4/6] drm/colorop: make lut(1/3)d_interpolation mutable

[Melissa Wen]

As it's not immutable anymore, any changes should be handled by
drm_colorop_state. Move their enum and make it correctly behaves as
mutable.

Reviewed-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
Signed-off-by: Melissa Wen <mwen@igalia.com>

---

v6:
- check drm_object_property_get_default_value() before set interp props
---
 drivers/gpu/drm/drm_atomic.c      |  4 ++--
 drivers/gpu/drm/drm_atomic_uapi.c |  8 ++++----
 drivers/gpu/drm/drm_colorop.c     | 16 ++++++++++++++--
 include/drm/drm_colorop.h         | 28 ++++++++++++++--------------
 4 files changed, 34 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 659cf56150e5..b26212e719b2 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -857,7 +857,7 @@ static void drm_atomic_colorop_print_state(struct drm_printer *p,
 	case DRM_COLOROP_1D_LUT:
 		drm_printf_indent(p, 1, "size=%d\n", colorop->size);
 		drm_printf_indent(p, 1, "interpolation=%s\n",
-				  drm_get_colorop_lut1d_interpolation_name(colorop->lut1d_interpolation));
+				  drm_get_colorop_lut1d_interpolation_name(state->lut1d_interpolation));
 		drm_printf_indent(p, 1, "data blob id=%d\n", state->data ? state->data->base.id : 0);
 		break;
 	case DRM_COLOROP_CTM_3X4:
@@ -869,7 +869,7 @@ static void drm_atomic_colorop_print_state(struct drm_printer *p,
 	case DRM_COLOROP_3D_LUT:
 		drm_printf_indent(p, 1, "size=%d\n", colorop->size);
 		drm_printf_indent(p, 1, "interpolation=%s\n",
-				  drm_get_colorop_lut3d_interpolation_name(colorop->lut3d_interpolation));
+				  drm_get_colorop_lut3d_interpolation_name(state->lut3d_interpolation));
 		drm_printf_indent(p, 1, "data blob id=%d\n", state->data ? state->data->base.id : 0);
 		break;
 	default:
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 6441b55cc274..78423905051e 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -751,13 +751,13 @@ static int drm_atomic_colorop_set_property(struct drm_colorop *colorop,
 	if (property == colorop->bypass_property) {
 		state->bypass = val;
 	} else if (property == colorop->lut1d_interpolation_property) {
-		colorop->lut1d_interpolation = val;
+		state->lut1d_interpolation = val;
 	} else if (property == colorop->curve_1d_type_property) {
 		state->curve_1d_type = val;
 	} else if (property == colorop->multiplier_property) {
 		state->multiplier = val;
 	} else if (property == colorop->lut3d_interpolation_property) {
-		colorop->lut3d_interpolation = val;
+		state->lut3d_interpolation = val;
 	} else if (property == colorop->data_property) {
 		return drm_atomic_color_set_data_property(colorop, state,
 							  property, val);
@@ -782,7 +782,7 @@ drm_atomic_colorop_get_property(struct drm_colorop *colorop,
 	else if (property == colorop->bypass_property)
 		*val = state->bypass;
 	else if (property == colorop->lut1d_interpolation_property)
-		*val = colorop->lut1d_interpolation;
+		*val = state->lut1d_interpolation;
 	else if (property == colorop->curve_1d_type_property)
 		*val = state->curve_1d_type;
 	else if (property == colorop->multiplier_property)
@@ -790,7 +790,7 @@ drm_atomic_colorop_get_property(struct drm_colorop *colorop,
 	else if (property == colorop->size_property)
 		*val = colorop->size;
 	else if (property == colorop->lut3d_interpolation_property)
-		*val = colorop->lut3d_interpolation;
+		*val = state->lut3d_interpolation;
 	else if (property == colorop->data_property)
 		*val = (state->data) ? state->data->base.id : 0;
 	else
diff --git a/drivers/gpu/drm/drm_colorop.c b/drivers/gpu/drm/drm_colorop.c
index 764d12060666..b0a9a8094dfe 100644
--- a/drivers/gpu/drm/drm_colorop.c
+++ b/drivers/gpu/drm/drm_colorop.c
@@ -342,7 +342,6 @@ int drm_plane_colorop_curve_1d_lut_init(struct drm_device *dev, struct drm_color
 
 	colorop->lut1d_interpolation_property = prop;
 	drm_object_attach_property(&colorop->base, prop, interpolation);
-	colorop->lut1d_interpolation = interpolation;
 
 	/* data */
 	ret = drm_colorop_create_data_prop(dev, colorop);
@@ -442,7 +441,6 @@ int drm_plane_colorop_3dlut_init(struct drm_device *dev, struct drm_colorop *col
 
 	colorop->lut3d_interpolation_property = prop;
 	drm_object_attach_property(&colorop->base, prop, interpolation);
-	colorop->lut3d_interpolation = interpolation;
 
 	/* data */
 	ret = drm_colorop_create_data_prop(dev, colorop);
@@ -521,6 +519,20 @@ static void __drm_colorop_state_reset(struct drm_colorop_state *colorop_state,
 							   &val))
 			colorop_state->curve_1d_type = val;
 	}
+
+	if (colorop->lut1d_interpolation_property) {
+		if(!drm_object_property_get_default_value(&colorop->base,
+							  colorop->lut1d_interpolation_property,
+							  &val));
+			colorop_state->lut1d_interpolation = val;
+	}
+
+	if (colorop->lut3d_interpolation_property) {
+		if(!drm_object_property_get_default_value(&colorop->base,
+							  colorop->lut3d_interpolation_property,
+							  &val);
+			colorop_state->lut3d_interpolation = val;
+	}
 }
 
 /**
diff --git a/include/drm/drm_colorop.h b/include/drm/drm_colorop.h
index 53a2148082d5..d08a6a8a8392 100644
--- a/include/drm/drm_colorop.h
+++ b/include/drm/drm_colorop.h
@@ -183,6 +183,20 @@ struct drm_colorop_state {
 	 */
 	struct drm_property_blob *data;
 
+	/**
+	 * @lut1d_interpolation:
+	 *
+	 * Interpolation for DRM_COLOROP_1D_LUT
+	 */
+	enum drm_colorop_lut1d_interpolation_type lut1d_interpolation;
+
+	/**
+	 * @lut3d_interpolation:
+	 *
+	 * Interpolation for DRM_COLOROP_3D_LUT
+	 */
+	enum drm_colorop_lut3d_interpolation_type lut3d_interpolation;
+
 	/** @state: backpointer to global drm_atomic_commit */
 	struct drm_atomic_commit *state;
 };
@@ -306,20 +320,6 @@ struct drm_colorop {
 	 */
 	uint32_t size;
 
-	/**
-	 * @lut1d_interpolation:
-	 *
-	 * Interpolation for DRM_COLOROP_1D_LUT
-	 */
-	enum drm_colorop_lut1d_interpolation_type lut1d_interpolation;
-
-	/**
-	 * @lut3d_interpolation:
-	 *
-	 * Interpolation for DRM_COLOROP_3D_LUT
-	 */
-	enum drm_colorop_lut3d_interpolation_type lut3d_interpolation;
-
 	/**
 	 * @lut1d_interpolation_property:
 	 *
-- 
2.53.0

Re: [PATCH v6 4/6] drm/colorop: make lut(1/3)d_interpolation mutable

[Borah, Chaitanya Kumar]

On 5/20/2026 2:39 AM, Melissa Wen wrote:
> As it's not immutable anymore, any changes should be handled by
> drm_colorop_state. Move their enum and make it correctly behaves as
> mutable.
> 
> Reviewed-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
> Signed-off-by: Melissa Wen <mwen@igalia.com>
> 
> ---
> 
> v6:
> - check drm_object_property_get_default_value() before set interp props
> ---
>   drivers/gpu/drm/drm_atomic.c      |  4 ++--
>   drivers/gpu/drm/drm_atomic_uapi.c |  8 ++++----
>   drivers/gpu/drm/drm_colorop.c     | 16 ++++++++++++++--
>   include/drm/drm_colorop.h         | 28 ++++++++++++++--------------
>   4 files changed, 34 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 659cf56150e5..b26212e719b2 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -857,7 +857,7 @@ static void drm_atomic_colorop_print_state(struct drm_printer *p,
>   	case DRM_COLOROP_1D_LUT:
>   		drm_printf_indent(p, 1, "size=%d\n", colorop->size);
>   		drm_printf_indent(p, 1, "interpolation=%s\n",
> -				  drm_get_colorop_lut1d_interpolation_name(colorop->lut1d_interpolation));
> +				  drm_get_colorop_lut1d_interpolation_name(state->lut1d_interpolation));
>   		drm_printf_indent(p, 1, "data blob id=%d\n", state->data ? state->data->base.id : 0);
>   		break;
>   	case DRM_COLOROP_CTM_3X4:
> @@ -869,7 +869,7 @@ static void drm_atomic_colorop_print_state(struct drm_printer *p,
>   	case DRM_COLOROP_3D_LUT:
>   		drm_printf_indent(p, 1, "size=%d\n", colorop->size);
>   		drm_printf_indent(p, 1, "interpolation=%s\n",
> -				  drm_get_colorop_lut3d_interpolation_name(colorop->lut3d_interpolation));
> +				  drm_get_colorop_lut3d_interpolation_name(state->lut3d_interpolation));
>   		drm_printf_indent(p, 1, "data blob id=%d\n", state->data ? state->data->base.id : 0);
>   		break;
>   	default:
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index 6441b55cc274..78423905051e 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -751,13 +751,13 @@ static int drm_atomic_colorop_set_property(struct drm_colorop *colorop,
>   	if (property == colorop->bypass_property) {
>   		state->bypass = val;
>   	} else if (property == colorop->lut1d_interpolation_property) {
> -		colorop->lut1d_interpolation = val;
> +		state->lut1d_interpolation = val;
>   	} else if (property == colorop->curve_1d_type_property) {
>   		state->curve_1d_type = val;
>   	} else if (property == colorop->multiplier_property) {
>   		state->multiplier = val;
>   	} else if (property == colorop->lut3d_interpolation_property) {
> -		colorop->lut3d_interpolation = val;
> +		state->lut3d_interpolation = val;
>   	} else if (property == colorop->data_property) {
>   		return drm_atomic_color_set_data_property(colorop, state,
>   							  property, val);
> @@ -782,7 +782,7 @@ drm_atomic_colorop_get_property(struct drm_colorop *colorop,
>   	else if (property == colorop->bypass_property)
>   		*val = state->bypass;
>   	else if (property == colorop->lut1d_interpolation_property)
> -		*val = colorop->lut1d_interpolation;
> +		*val = state->lut1d_interpolation;
>   	else if (property == colorop->curve_1d_type_property)
>   		*val = state->curve_1d_type;
>   	else if (property == colorop->multiplier_property)
> @@ -790,7 +790,7 @@ drm_atomic_colorop_get_property(struct drm_colorop *colorop,
>   	else if (property == colorop->size_property)
>   		*val = colorop->size;
>   	else if (property == colorop->lut3d_interpolation_property)
> -		*val = colorop->lut3d_interpolation;
> +		*val = state->lut3d_interpolation;
>   	else if (property == colorop->data_property)
>   		*val = (state->data) ? state->data->base.id : 0;
>   	else
> diff --git a/drivers/gpu/drm/drm_colorop.c b/drivers/gpu/drm/drm_colorop.c
> index 764d12060666..b0a9a8094dfe 100644
> --- a/drivers/gpu/drm/drm_colorop.c
> +++ b/drivers/gpu/drm/drm_colorop.c
> @@ -342,7 +342,6 @@ int drm_plane_colorop_curve_1d_lut_init(struct drm_device *dev, struct drm_color
>   
>   	colorop->lut1d_interpolation_property = prop;
>   	drm_object_attach_property(&colorop->base, prop, interpolation);
> -	colorop->lut1d_interpolation = interpolation;
>   
>   	/* data */
>   	ret = drm_colorop_create_data_prop(dev, colorop);
> @@ -442,7 +441,6 @@ int drm_plane_colorop_3dlut_init(struct drm_device *dev, struct drm_colorop *col
>   
>   	colorop->lut3d_interpolation_property = prop;
>   	drm_object_attach_property(&colorop->base, prop, interpolation);
> -	colorop->lut3d_interpolation = interpolation;
>   
>   	/* data */
>   	ret = drm_colorop_create_data_prop(dev, colorop);
> @@ -521,6 +519,20 @@ static void __drm_colorop_state_reset(struct drm_colorop_state *colorop_state,
>   							   &val))
>   			colorop_state->curve_1d_type = val;
>   	}
> +
> +	if (colorop->lut1d_interpolation_property) {
> +		if(!drm_object_property_get_default_value(&colorop->base,
> +							  colorop->lut1d_interpolation_property,
> +							  &val));
> +			colorop_state->lut1d_interpolation = val;
> +	}
> +
> +	if (colorop->lut3d_interpolation_property) {
> +		if(!drm_object_property_get_default_value(&colorop->base,
> +							  colorop->lut3d_interpolation_property,
> +							  &val);
> +			colorop_state->lut3d_interpolation = val;
> +	}

I see you fixed the ; in the next patch, better to fix it within this 
patch. Also needs space between if and (.

>   }
>   
>   /**
> diff --git a/include/drm/drm_colorop.h b/include/drm/drm_colorop.h
> index 53a2148082d5..d08a6a8a8392 100644
> --- a/include/drm/drm_colorop.h
> +++ b/include/drm/drm_colorop.h
> @@ -183,6 +183,20 @@ struct drm_colorop_state {
>   	 */
>   	struct drm_property_blob *data;
>   
> +	/**
> +	 * @lut1d_interpolation:
> +	 *
> +	 * Interpolation for DRM_COLOROP_1D_LUT
> +	 */
> +	enum drm_colorop_lut1d_interpolation_type lut1d_interpolation;
> +
> +	/**
> +	 * @lut3d_interpolation:
> +	 *
> +	 * Interpolation for DRM_COLOROP_3D_LUT
> +	 */
> +	enum drm_colorop_lut3d_interpolation_type lut3d_interpolation;
> +
>   	/** @state: backpointer to global drm_atomic_commit */
>   	struct drm_atomic_commit *state;
>   };
> @@ -306,20 +320,6 @@ struct drm_colorop {
>   	 */
>   	uint32_t size;
>   
> -	/**
> -	 * @lut1d_interpolation:
> -	 *
> -	 * Interpolation for DRM_COLOROP_1D_LUT
> -	 */
> -	enum drm_colorop_lut1d_interpolation_type lut1d_interpolation;
> -
> -	/**
> -	 * @lut3d_interpolation:
> -	 *
> -	 * Interpolation for DRM_COLOROP_3D_LUT
> -	 */
> -	enum drm_colorop_lut3d_interpolation_type lut3d_interpolation;
> -
>   	/**
>   	 * @lut1d_interpolation_property:
>   	 *

Re: [PATCH v6 4/6] drm/colorop: make lut(1/3)d_interpolation mutable

[Melissa Wen]

On 21/05/2026 13:17, Borah, Chaitanya Kumar wrote:
>
>
> On 5/20/2026 2:39 AM, Melissa Wen wrote:
>> As it's not immutable anymore, any changes should be handled by
>> drm_colorop_state. Move their enum and make it correctly behaves as
>> mutable.
>>
>> Reviewed-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
>> Signed-off-by: Melissa Wen <mwen@igalia.com>
>>
>> ---
>>
>> v6:
>> - check drm_object_property_get_default_value() before set interp props
>> ---
>>   drivers/gpu/drm/drm_atomic.c      |  4 ++--
>>   drivers/gpu/drm/drm_atomic_uapi.c |  8 ++++----
>>   drivers/gpu/drm/drm_colorop.c     | 16 ++++++++++++++--
>>   include/drm/drm_colorop.h         | 28 ++++++++++++++--------------
>>   4 files changed, 34 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> index 659cf56150e5..b26212e719b2 100644
>> --- a/drivers/gpu/drm/drm_atomic.c
>> +++ b/drivers/gpu/drm/drm_atomic.c
>> @@ -857,7 +857,7 @@ static void drm_atomic_colorop_print_state(struct 
>> drm_printer *p,
>>       case DRM_COLOROP_1D_LUT:
>>           drm_printf_indent(p, 1, "size=%d\n", colorop->size);
>>           drm_printf_indent(p, 1, "interpolation=%s\n",
>> - 
>> drm_get_colorop_lut1d_interpolation_name(colorop->lut1d_interpolation));
>> + drm_get_colorop_lut1d_interpolation_name(state->lut1d_interpolation));
>>           drm_printf_indent(p, 1, "data blob id=%d\n", state->data ? 
>> state->data->base.id : 0);
>>           break;
>>       case DRM_COLOROP_CTM_3X4:
>> @@ -869,7 +869,7 @@ static void drm_atomic_colorop_print_state(struct 
>> drm_printer *p,
>>       case DRM_COLOROP_3D_LUT:
>>           drm_printf_indent(p, 1, "size=%d\n", colorop->size);
>>           drm_printf_indent(p, 1, "interpolation=%s\n",
>> - 
>> drm_get_colorop_lut3d_interpolation_name(colorop->lut3d_interpolation));
>> + drm_get_colorop_lut3d_interpolation_name(state->lut3d_interpolation));
>>           drm_printf_indent(p, 1, "data blob id=%d\n", state->data ? 
>> state->data->base.id : 0);
>>           break;
>>       default:
>> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c 
>> b/drivers/gpu/drm/drm_atomic_uapi.c
>> index 6441b55cc274..78423905051e 100644
>> --- a/drivers/gpu/drm/drm_atomic_uapi.c
>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
>> @@ -751,13 +751,13 @@ static int 
>> drm_atomic_colorop_set_property(struct drm_colorop *colorop,
>>       if (property == colorop->bypass_property) {
>>           state->bypass = val;
>>       } else if (property == colorop->lut1d_interpolation_property) {
>> -        colorop->lut1d_interpolation = val;
>> +        state->lut1d_interpolation = val;
>>       } else if (property == colorop->curve_1d_type_property) {
>>           state->curve_1d_type = val;
>>       } else if (property == colorop->multiplier_property) {
>>           state->multiplier = val;
>>       } else if (property == colorop->lut3d_interpolation_property) {
>> -        colorop->lut3d_interpolation = val;
>> +        state->lut3d_interpolation = val;
>>       } else if (property == colorop->data_property) {
>>           return drm_atomic_color_set_data_property(colorop, state,
>>                                 property, val);
>> @@ -782,7 +782,7 @@ drm_atomic_colorop_get_property(struct 
>> drm_colorop *colorop,
>>       else if (property == colorop->bypass_property)
>>           *val = state->bypass;
>>       else if (property == colorop->lut1d_interpolation_property)
>> -        *val = colorop->lut1d_interpolation;
>> +        *val = state->lut1d_interpolation;
>>       else if (property == colorop->curve_1d_type_property)
>>           *val = state->curve_1d_type;
>>       else if (property == colorop->multiplier_property)
>> @@ -790,7 +790,7 @@ drm_atomic_colorop_get_property(struct 
>> drm_colorop *colorop,
>>       else if (property == colorop->size_property)
>>           *val = colorop->size;
>>       else if (property == colorop->lut3d_interpolation_property)
>> -        *val = colorop->lut3d_interpolation;
>> +        *val = state->lut3d_interpolation;
>>       else if (property == colorop->data_property)
>>           *val = (state->data) ? state->data->base.id : 0;
>>       else
>> diff --git a/drivers/gpu/drm/drm_colorop.c 
>> b/drivers/gpu/drm/drm_colorop.c
>> index 764d12060666..b0a9a8094dfe 100644
>> --- a/drivers/gpu/drm/drm_colorop.c
>> +++ b/drivers/gpu/drm/drm_colorop.c
>> @@ -342,7 +342,6 @@ int drm_plane_colorop_curve_1d_lut_init(struct 
>> drm_device *dev, struct drm_color
>>         colorop->lut1d_interpolation_property = prop;
>>       drm_object_attach_property(&colorop->base, prop, interpolation);
>> -    colorop->lut1d_interpolation = interpolation;
>>         /* data */
>>       ret = drm_colorop_create_data_prop(dev, colorop);
>> @@ -442,7 +441,6 @@ int drm_plane_colorop_3dlut_init(struct 
>> drm_device *dev, struct drm_colorop *col
>>         colorop->lut3d_interpolation_property = prop;
>>       drm_object_attach_property(&colorop->base, prop, interpolation);
>> -    colorop->lut3d_interpolation = interpolation;
>>         /* data */
>>       ret = drm_colorop_create_data_prop(dev, colorop);
>> @@ -521,6 +519,20 @@ static void __drm_colorop_state_reset(struct 
>> drm_colorop_state *colorop_state,
>>                                  &val))
>>               colorop_state->curve_1d_type = val;
>>       }
>> +
>> +    if (colorop->lut1d_interpolation_property) {
>> + if(!drm_object_property_get_default_value(&colorop->base,
>> + colorop->lut1d_interpolation_property,
>> +                              &val));
>> +            colorop_state->lut1d_interpolation = val;
>> +    }
>> +
>> +    if (colorop->lut3d_interpolation_property) {
>> + if(!drm_object_property_get_default_value(&colorop->base,
>> + colorop->lut3d_interpolation_property,
>> +                              &val);
>> +            colorop_state->lut3d_interpolation = val;
>> +    }
>
> I see you fixed the ; in the next patch, better to fix it within this 
> patch. Also needs space between if and (.

Ouch, thanks for pointing it out.

>
>>   }
>>     /**
>> diff --git a/include/drm/drm_colorop.h b/include/drm/drm_colorop.h
>> index 53a2148082d5..d08a6a8a8392 100644
>> --- a/include/drm/drm_colorop.h
>> +++ b/include/drm/drm_colorop.h
>> @@ -183,6 +183,20 @@ struct drm_colorop_state {
>>        */
>>       struct drm_property_blob *data;
>>   +    /**
>> +     * @lut1d_interpolation:
>> +     *
>> +     * Interpolation for DRM_COLOROP_1D_LUT
>> +     */
>> +    enum drm_colorop_lut1d_interpolation_type lut1d_interpolation;
>> +
>> +    /**
>> +     * @lut3d_interpolation:
>> +     *
>> +     * Interpolation for DRM_COLOROP_3D_LUT
>> +     */
>> +    enum drm_colorop_lut3d_interpolation_type lut3d_interpolation;
>> +
>>       /** @state: backpointer to global drm_atomic_commit */
>>       struct drm_atomic_commit *state;
>>   };
>> @@ -306,20 +320,6 @@ struct drm_colorop {
>>        */
>>       uint32_t size;
>>   -    /**
>> -     * @lut1d_interpolation:
>> -     *
>> -     * Interpolation for DRM_COLOROP_1D_LUT
>> -     */
>> -    enum drm_colorop_lut1d_interpolation_type lut1d_interpolation;
>> -
>> -    /**
>> -     * @lut3d_interpolation:
>> -     *
>> -     * Interpolation for DRM_COLOROP_3D_LUT
>> -     */
>> -    enum drm_colorop_lut3d_interpolation_type lut3d_interpolation;
>> -
>>       /**
>>        * @lut1d_interpolation_property:
>>        *
>

Claude review: drm/colorop: make lut(1/3)d_interpolation mutable

[Claude Code Review Bot]

Patch Review

**Status: Has critical bugs — breaks build**

The design is correct: move `lut1d_interpolation` and `lut3d_interpolation` from `struct drm_colorop` to `struct drm_colorop_state`, update getters/setters/print to use `state->` instead of `colorop->`, and initialize defaults in `__drm_colorop_state_reset`. The header and `drm_atomic.c`/`drm_atomic_uapi.c` changes are all correct.

**Bug 1 — spurious semicolon (lut1d):**
```c
if(!drm_object_property_get_default_value(&colorop->base,
                      colorop->lut1d_interpolation_property,
                      &val));
    colorop_state->lut1d_interpolation = val;
```
The trailing `;` after the `)` makes the `if` body empty. The assignment executes unconditionally, using whatever `val` contained. When `drm_object_property_get_default_value` fails, `val` is uninitialized garbage. In practice, this function probably always succeeds here (property was just attached), so it's accidentally correct — but it's still wrong code and will trigger compiler warnings.

**Bug 2 — syntax error (lut3d):**
```c
if(!drm_object_property_get_default_value(&colorop->base,
                      colorop->lut3d_interpolation_property,
                      &val);
    colorop_state->lut3d_interpolation = val;
```
This has `&val);` — the `)` closes the function call, `;` appears inside the `if(...)` condition before the closing `)` of the `if`. **This is a compile error** — the `if` parenthesis is never properly closed.

Both are fixed by Patch 5, but Patch 4 alone doesn't compile, **breaking git bisect**. The fix from Patch 5 should be folded into this patch.

Also, minor style: `if(!` should have a space: `if (!` per kernel coding style.

---

---
Generated by Claude Code Patch Reviewer

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

[Melissa Wen]

As we do for CRTC color mgmt properties, use color_mgmt_changed flag to
track any value changes in the color pipeline of a given plane, so that
drivers can update color blocks as soon as plane color pipeline or
individual colorop values change.

Reviewed-by: Harry Wentland <harry.wentland@amd.com> #v1
Reviewed-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
Signed-off-by: Melissa Wen <mwen@igalia.com>
---

v2: add linux types to provide bool for MSM driver (kernel bot)
v3: track lut1d/3d_interpolation changes (Chaitanya)
v6: use `|= replaced` for consistency (Chaitanya)
---
 drivers/gpu/drm/drm_atomic_uapi.c | 64 ++++++++++++++++++++++++-------
 drivers/gpu/drm/drm_colorop.c     |  4 +-
 include/drm/drm_atomic_uapi.h     |  4 +-
 3 files changed, 56 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 78423905051e..e997917819e8 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -265,13 +265,19 @@ EXPORT_SYMBOL(drm_atomic_set_fb_for_plane);
  *
  * Helper function to select the color pipeline on a plane by setting
  * it to the first drm_colorop element of the pipeline.
+ *
+ * Return: true if plane color pipeline value changed, false otherwise.
  */
-void
+bool
 drm_atomic_set_colorop_for_plane(struct drm_plane_state *plane_state,
 				 struct drm_colorop *colorop)
 {
 	struct drm_plane *plane = plane_state->plane;
 
+	/* Color pipeline didn't change */
+	if (plane_state->color_pipeline == colorop)
+		return false;
+
 	if (colorop)
 		drm_dbg_atomic(plane->dev,
 			       "Set [COLOROP:%d] for [PLANE:%d:%s] state %p\n",
@@ -283,6 +289,8 @@ drm_atomic_set_colorop_for_plane(struct drm_plane_state *plane_state,
 			       plane->base.id, plane->name, plane_state);
 
 	plane_state->color_pipeline = colorop;
+
+	return true;
 }
 EXPORT_SYMBOL(drm_atomic_set_colorop_for_plane);
 
@@ -604,7 +612,7 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
 		if (val && !colorop)
 			return -EACCES;
 
-		drm_atomic_set_colorop_for_plane(state, colorop);
+		state->color_mgmt_changed |= drm_atomic_set_colorop_for_plane(state, colorop);
 	} else if (property == config->prop_fb_damage_clips) {
 		ret = drm_property_replace_blob_from_id(dev,
 					&state->fb_damage_clips,
@@ -713,11 +721,11 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
 static int drm_atomic_color_set_data_property(struct drm_colorop *colorop,
 					      struct drm_colorop_state *state,
 					      struct drm_property *property,
-					      uint64_t val)
+					      uint64_t val,
+					      bool *replaced)
 {
 	ssize_t elem_size = -1;
 	ssize_t size = -1;
-	bool replaced = false;
 
 	switch (colorop->type) {
 	case DRM_COLOROP_1D_LUT:
@@ -739,28 +747,45 @@ static int drm_atomic_color_set_data_property(struct drm_colorop *colorop,
 						 &state->data,
 						 val,
 						 -1, size, elem_size,
-						 &replaced);
+						 replaced);
 }
 
 static int drm_atomic_colorop_set_property(struct drm_colorop *colorop,
 					   struct drm_colorop_state *state,
 					   struct drm_file *file_priv,
 					   struct drm_property *property,
-					   uint64_t val)
+					   uint64_t val,
+					   bool *replaced)
 {
 	if (property == colorop->bypass_property) {
-		state->bypass = val;
+		if (state->bypass != val) {
+			state->bypass = val;
+			*replaced = true;
+		}
 	} else if (property == colorop->lut1d_interpolation_property) {
-		state->lut1d_interpolation = val;
+		if (state->lut1d_interpolation != val) {
+			state->lut1d_interpolation = val;
+			*replaced = true;
+		}
 	} else if (property == colorop->curve_1d_type_property) {
-		state->curve_1d_type = val;
+		if (state->curve_1d_type != val) {
+			state->curve_1d_type = val;
+			*replaced = true;
+		}
 	} else if (property == colorop->multiplier_property) {
-		state->multiplier = val;
+		if (state->multiplier != val) {
+			state->multiplier = val;
+			*replaced = true;
+		}
 	} else if (property == colorop->lut3d_interpolation_property) {
-		state->lut3d_interpolation = val;
+		if (state->lut3d_interpolation != val) {
+			state->lut3d_interpolation = val;
+			*replaced = true;
+		}
 	} else if (property == colorop->data_property) {
 		return drm_atomic_color_set_data_property(colorop, state,
-							  property, val);
+							  property, val,
+							  replaced);
 	} else {
 		drm_dbg_atomic(colorop->dev,
 			       "[COLOROP:%d:%d] unknown property [PROP:%d:%s]\n",
@@ -1275,8 +1300,10 @@ int drm_atomic_set_property(struct drm_atomic_commit *state,
 		break;
 	}
 	case DRM_MODE_OBJECT_COLOROP: {
+		struct drm_plane_state *plane_state;
 		struct drm_colorop *colorop = obj_to_colorop(obj);
 		struct drm_colorop_state *colorop_state;
+		bool replaced = false;
 
 		colorop_state = drm_atomic_get_colorop_state(state, colorop);
 		if (IS_ERR(colorop_state)) {
@@ -1285,7 +1312,18 @@ int drm_atomic_set_property(struct drm_atomic_commit *state,
 		}
 
 		ret = drm_atomic_colorop_set_property(colorop, colorop_state,
-						      file_priv, prop, prop_value);
+						      file_priv, prop, prop_value,
+						      &replaced);
+		if (ret || !replaced)
+			break;
+
+		plane_state = drm_atomic_get_plane_state(state, colorop->plane);
+		if (IS_ERR(plane_state)) {
+			ret = PTR_ERR(plane_state);
+			break;
+		}
+		plane_state->color_mgmt_changed |= replaced;
+
 		break;
 	}
 	default:
diff --git a/drivers/gpu/drm/drm_colorop.c b/drivers/gpu/drm/drm_colorop.c
index b0a9a8094dfe..27972a59839e 100644
--- a/drivers/gpu/drm/drm_colorop.c
+++ b/drivers/gpu/drm/drm_colorop.c
@@ -523,14 +523,14 @@ static void __drm_colorop_state_reset(struct drm_colorop_state *colorop_state,
 	if (colorop->lut1d_interpolation_property) {
 		if(!drm_object_property_get_default_value(&colorop->base,
 							  colorop->lut1d_interpolation_property,
-							  &val));
+							  &val))
 			colorop_state->lut1d_interpolation = val;
 	}
 
 	if (colorop->lut3d_interpolation_property) {
 		if(!drm_object_property_get_default_value(&colorop->base,
 							  colorop->lut3d_interpolation_property,
-							  &val);
+							  &val))
 			colorop_state->lut3d_interpolation = val;
 	}
 }
diff --git a/include/drm/drm_atomic_uapi.h b/include/drm/drm_atomic_uapi.h
index 436315523326..4e7e78f711e2 100644
--- a/include/drm/drm_atomic_uapi.h
+++ b/include/drm/drm_atomic_uapi.h
@@ -29,6 +29,8 @@
 #ifndef DRM_ATOMIC_UAPI_H_
 #define DRM_ATOMIC_UAPI_H_
 
+#include <linux/types.h>
+
 struct drm_crtc_state;
 struct drm_display_mode;
 struct drm_property_blob;
@@ -50,7 +52,7 @@ drm_atomic_set_crtc_for_plane(struct drm_plane_state *plane_state,
 			      struct drm_crtc *crtc);
 void drm_atomic_set_fb_for_plane(struct drm_plane_state *plane_state,
 				 struct drm_framebuffer *fb);
-void drm_atomic_set_colorop_for_plane(struct drm_plane_state *plane_state,
+bool drm_atomic_set_colorop_for_plane(struct drm_plane_state *plane_state,
 				      struct drm_colorop *colorop);
 int __must_check
 drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
-- 
2.53.0

Claude review: drm/atomic: track individual colorop updates

[Claude Code Review Bot]

Patch Review

**Status: Good logic, but needs the drm_colorop.c fixes moved to Patch 4**

This is the core of the series. It:
1. Changes `drm_atomic_set_colorop_for_plane()` to return `bool` indicating whether the pipeline changed.
2. Adds change tracking through `replaced` out-parameter in `drm_atomic_colorop_set_property()` and `drm_atomic_color_set_data_property()`.
3. In `drm_atomic_set_property()` for `DRM_MODE_OBJECT_COLOROP`, acquires the plane state and sets `color_mgmt_changed` when a colorop property actually changed value.

The change-detection pattern is clean — each property setter only sets `*replaced = true` when the new value differs:
```c
if (state->bypass != val) {
    state->bypass = val;
    *replaced = true;
}
```

One observation in `drm_atomic_set_property`:
```c
if (ret || !replaced)
    break;
...
plane_state->color_mgmt_changed |= replaced;
```
At this point `replaced` is guaranteed `true`, so `|= replaced` is equivalent to `= true`. The `|=` is harmless and follows the pattern for not clobbering, but it's slightly misleading since `replaced` can't be `false` here. Very minor, and Chaitanya already approved this for consistency.

The `drm_colorop.c` hunk fixes the syntax bugs from Patch 4. **This fix must be squashed into Patch 4** to maintain bisectability.

---

---
Generated by Claude Code Patch Reviewer

[PATCH v6 6/6] drm/amd/display: use plane color_mgmt_changed to track colorop changes

[Melissa Wen]

Ensure the driver tracks changes in any colorop property of a plane
color pipeline by using the same mechanism of CRTC color management and
update plane color blocks when any colorop property changes. It fixes an
issue observed on gamescope settings for night mode which is done via
shaper/3D-LUT updates.

Reviewed-by: Harry Wentland <harry.wentland@amd.com>
Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index d590f0df6abd..36425d9c2a67 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -10198,7 +10198,7 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_commit *state,
 			continue;
 
 		bundle->surface_updates[planes_count].surface = dc_plane;
-		if (new_pcrtc_state->color_mgmt_changed) {
+		if (new_pcrtc_state->color_mgmt_changed || new_plane_state->color_mgmt_changed) {
 			bundle->surface_updates[planes_count].gamma = &dc_plane->gamma_correction;
 			bundle->surface_updates[planes_count].in_transfer_func = &dc_plane->in_transfer_func;
 			bundle->surface_updates[planes_count].gamut_remap_matrix = &dc_plane->gamut_remap_matrix;
@@ -12024,6 +12024,10 @@ static bool should_reset_plane(struct drm_atomic_commit *state,
 	if (new_crtc_state->color_mgmt_changed)
 		return true;
 
+	/* Plane color pipeline or its colorop changes. */
+	if (new_plane_state->color_mgmt_changed)
+		return true;
+
 	/*
 	 * On zpos change, planes need to be reordered by removing and re-adding
 	 * them one by one to the dc state, in order of descending zpos.
-- 
2.53.0

Claude review: drm/amd/display: use plane color_mgmt_changed to track colorop changes

[Claude Code Review Bot]

Patch Review

**Status: Looks good**

Driver-side consumption of the new `plane_state->color_mgmt_changed` flag in two places in `amdgpu_dm.c`:

1. `amdgpu_dm_commit_planes`: Extends the existing CRTC `color_mgmt_changed` check to also trigger on plane-level changes:
```c
if (new_pcrtc_state->color_mgmt_changed || new_plane_state->color_mgmt_changed) {
```

2. `should_reset_plane`: Adds a new check that forces plane reset when color management changed:
```c
if (new_plane_state->color_mgmt_changed)
    return true;
```

Both are straightforward and correct. The comment "Plane color pipeline or its colorop changes." is acceptable. This patch fixes the actual user-visible bug (gamescope night mode shaper/3D LUT updates not taking effect).

---

**Summary of required changes:**
- **Must fix**: Squash the `drm_colorop.c` syntax fixes from Patch 5 into Patch 4 to fix the build breakage and maintain bisectability. The two-character changes (`));` → `))` and `&val);` → `&val))`) belong where the code was introduced.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/atomic: track individual colorop updates

[Claude Code Review Bot]

Overall Series Review

Subject: drm/atomic: track individual colorop updates
Author: Melissa Wen <mwen@igalia.com>
Patches: 12
Reviewed: 2026-05-25T22:24:19.776114

---

This is a well-structured v6 series from Melissa Wen (Igalia) that improves DRM color pipeline tracking by: (1) scoping colorop state to only active pipelines, (2) rejecting updates to inactive colorops, (3) making LUT interpolation mutable, and (4) propagating colorop changes via `plane_state->color_mgmt_changed` so drivers (AMDGPU demonstrated) can react.

The design is sound and the series has good review coverage (Reviewed-by from Harry Wentland and Chaitanya Kumar Borah on several patches). However, **Patch 4 has critical compilation errors** that break bisectability — two syntax bugs in `__drm_colorop_state_reset` that Patch 5 quietly fixes. These must be squashed into Patch 4 or the fix moved there.

Otherwise the logic across the series is correct and the API changes are clean.

---

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