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

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

[Claude Code Review Bot]

Patch Review

**Overall:** Straightforward driver-side consumption of the new flag. Very small and focused.

**Observations:**

1. **`amdgpu_dm_commit_planes` change** (line 456):
   ```c
   if (new_pcrtc_state->color_mgmt_changed || new_plane_state->color_mgmt_changed) {
   ```
   This correctly triggers color block updates when either CRTC or plane colorop state changes. This is the actual bug fix for the gamescope night mode issue.

2. **`should_reset_plane` change** (lines 464-466):
   ```c
   if (new_plane_state->color_mgmt_changed)
       return true;
   ```
   This ensures plane reset (remove + re-add to DC state) happens on colorop changes. This parallels the existing `new_crtc_state->color_mgmt_changed` check just above it. The comment is clear and appropriate.

3. **Potential concern - plane reset cost**: Returning `true` from `should_reset_plane` triggers a full plane remove/re-add cycle in DC. For frequent colorop updates (e.g., animated night mode transitions), this could be heavyweight. However, this matches the existing behavior for CRTC color mgmt changes, so it's consistent with current expectations. If performance becomes an issue, it could be optimized later to only do a surface update without full reset, but that's out of scope here.

4. **No changes needed for `amdgpu_dm_atomic_check`**: The existing check path already handles plane state validation, so no additional check-phase changes are needed. This is correct since the core DRM atomic code already sets up the plane state properly in patch 1.

---
Generated by Claude Code Patch Reviewer

[PATCH v4 0/6] drm/atomic: track colorop changes of a given plane

[Melissa Wen]

This series aims to monitor 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.

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

This series fix shaper/3D LUT updates when changing night mode settings
on gamescope with a custom branch that supports `COLOR_PIPELINE`[1].

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

[1] https://github.com/ValveSoftware/gamescope/pull/2113

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: don't set colorop properties of inactive color pipelines
  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                  | 43 +++++----
 drivers/gpu/drm/drm_atomic_helper.c           |  9 +-
 drivers/gpu/drm/drm_atomic_uapi.c             | 93 +++++++++++++++----
 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, 136 insertions(+), 71 deletions(-)

-- 
2.53.0

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

Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 drivers/gpu/drm/drm_atomic.c        | 39 ++++++++++++++---------------
 drivers/gpu/drm/drm_atomic_helper.c |  9 +++----
 include/drm/drm_atomic.h            |  2 +-
 3 files changed, 23 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 54bab7e9f935..825522df9c12 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1591,26 +1591,25 @@ drm_atomic_add_affected_planes(struct drm_atomic_state *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
+ * @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 an active color pipeline set for a @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.
  *
  * 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 +1621,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_state *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 a768398a1884..c8dadbf5c319 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 8883290cd014..8916923f32b8 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_state *state,
 			       struct drm_crtc *crtc);
 int __must_check
-drm_atomic_add_affected_colorops(struct drm_atomic_state *state,
+drm_atomic_add_affected_colorops(struct drm_plane_state *state,
 				 struct drm_plane *plane);
 
 int __must_check drm_atomic_check_only(struct drm_atomic_state *state);
-- 
2.53.0

[PATCH v4 2/6] drm/atomic: don't set colorop properties of inactive color pipelines

[Melissa Wen]

Reject attempts to change property values of a colorop that is not
part of an active plane color pipeline.

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

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 5bd5bf6661df..bff8d58f8f12 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -1275,15 +1275,38 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
 		break;
 	}
 	case DRM_MODE_OBJECT_COLOROP: {
-		struct drm_colorop *colorop = obj_to_colorop(obj);
-		struct drm_colorop_state *colorop_state;
+		struct drm_colorop *active_colorop, *colorop = obj_to_colorop(obj);
+		struct drm_colorop_state *colorop_state = NULL;
+		struct drm_plane_state *plane_state;
 
-		colorop_state = drm_atomic_get_colorop_state(state, colorop);
-		if (IS_ERR(colorop_state)) {
-			ret = PTR_ERR(colorop_state);
+		plane_state = drm_atomic_get_plane_state(state, colorop->plane);
+		if (IS_ERR(plane_state)) {
+			ret = PTR_ERR(plane_state);
 			break;
 		}
 
+		/* Check if the colorop obj is part of an active color pipeline */
+		for (active_colorop = plane_state->color_pipeline;
+		     active_colorop;
+		     active_colorop = active_colorop->next) {
+			if (active_colorop == colorop) {
+				colorop_state = drm_atomic_get_colorop_state(state, colorop);
+				if (IS_ERR(colorop_state)) {
+					ret = PTR_ERR(colorop_state);
+					goto err;
+				}
+				break;
+			}
+		}
+
+		if (!colorop_state) {
+			drm_dbg_atomic(prop->dev,
+				       "[COLOROP:%d:%d] not part of the active pipeline\n",
+					obj->id, colorop->type);
+			ret = -EINVAL;
+			goto err;
+		}
+
 		ret = drm_atomic_colorop_set_property(colorop, colorop_state,
 						      file_priv, prop, prop_value);
 		break;
@@ -1294,6 +1317,7 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
 		break;
 	}
 
+err:
 	drm_property_change_valid_put(prop, ref);
 	return ret;
 }
-- 
2.53.0

[PATCH v4 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().

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 bd082854ca74..61cc8206b4c4 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

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

Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 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 825522df9c12..cc9c5db8908b 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -830,7 +830,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:
@@ -842,7 +842,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 bff8d58f8f12..25fe94410af7 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..b6930ef278c3 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) {
+		drm_object_property_get_default_value(&colorop->base,
+						      colorop->lut1d_interpolation_property,
+						      &val);
+		colorop_state->lut1d_interpolation = val;
+	}
+
+	if (colorop->lut3d_interpolation_property) {
+		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 61cc8206b4c4..d5b45339333f 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_state */
 	struct drm_atomic_state *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

[PATCH v4 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
Signed-off-by: Melissa Wen <mwen@igalia.com>
--

v3:
- track lut1d/3d_interpolation changes (Chaitanya)

v2: add linux types to provide bool for MSM driver (kernel bot)
---
 drivers/gpu/drm/drm_atomic_uapi.c | 55 +++++++++++++++++++++++--------
 include/drm/drm_atomic_uapi.h     |  4 ++-
 2 files changed, 45 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 25fe94410af7..2bac16c9855a 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",
@@ -1278,6 +1303,7 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
 		struct drm_colorop *active_colorop, *colorop = obj_to_colorop(obj);
 		struct drm_colorop_state *colorop_state = NULL;
 		struct drm_plane_state *plane_state;
+		bool replaced = false;
 
 		plane_state = drm_atomic_get_plane_state(state, colorop->plane);
 		if (IS_ERR(plane_state)) {
@@ -1308,7 +1334,10 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
 		}
 
 		ret = drm_atomic_colorop_set_property(colorop, colorop_state,
-						      file_priv, prop, prop_value);
+						      file_priv, prop, prop_value,
+						      &replaced);
+		if (!ret && replaced)
+			plane_state->color_mgmt_changed = true;
 		break;
 	}
 	default:
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

[PATCH v4 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 e96a12ff2d31..d3237f61246c 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -10067,7 +10067,7 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *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;
@@ -11808,6 +11808,10 @@ static bool should_reset_plane(struct drm_atomic_state *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

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

[kernel test robot]

Hi Melissa,

kernel test robot noticed the following build warnings:

[auto build test WARNING on drm-misc/drm-misc-next]
[also build test WARNING on next-20260430]
[cannot apply to linus/master v6.16-rc1]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Melissa-Wen/drm-atomic-only-add-colorop-state-from-active-color-pipeline/20260504-102820
base:   https://gitlab.freedesktop.org/drm/misc/kernel.git drm-misc-next
patch link:    https://lore.kernel.org/r/20260501132527.522320-7-mwen%40igalia.com
patch subject: [PATCH v4 6/6] drm/amd/display: use plane color_mgmt_changed to track colorop changes
config: x86_64-rhel-9.4 (https://download.01.org/0day-ci/archive/20260504/202605042355.hTox5m9T-lkp@intel.com/config)
compiler: gcc-14 (Debian 14.2.0-19) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260504/202605042355.hTox5m9T-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202605042355.hTox5m9T-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> Warning: drivers/gpu/drm/drm_atomic.c:1625 function parameter 'plane_state' not described in 'drm_atomic_add_affected_colorops'
>> Warning: drivers/gpu/drm/drm_atomic.c:1625 function parameter 'plane_state' not described in 'drm_atomic_add_affected_colorops'

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Claude review: drm/atomic: track colorop changes of a given plane

[Claude Code Review Bot]

Overall Series Review

Subject: drm/atomic: track colorop changes of a given plane
Author: Melissa Wen <mwen@igalia.com>
Patches: 8
Reviewed: 2026-05-05T09:26:00.753936

---

This is a well-structured 6-patch series that addresses a real functional bug: colorop property changes (e.g., shaper/3D LUT updates for gamescope night mode) were not being detected by the driver because there was no change-tracking mechanism for individual colorop properties on a plane's color pipeline.

The series is logically ordered:
1. First, restrict colorop state management to active pipelines only (patches 1-2)
2. Then, fix the mutable nature of interpolation properties (patches 3-4)
3. Finally, add change tracking via `color_mgmt_changed` (patches 5-6)

**Overall assessment:** The approach is sound and follows existing DRM patterns (mirroring CRTC `color_mgmt_changed` semantics for planes). The code is clean and well-decomposed. I have a few concerns noted per-patch below, mostly around edge cases.

---
Generated by Claude Code Patch Reviewer

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

[Claude Code Review Bot]

Patch Review

**Purpose:** Changes `drm_atomic_add_affected_colorops()` to walk only the active pipeline's linked list (`colorop->next`) instead of iterating all colorops on the device and filtering by plane.

**Review:**

The core logic change is good — walking the linked list via `plane_state->color_pipeline` / `colorop->next` is both more efficient and semantically correct:

```c
for (colorop = plane_state->color_pipeline;
     colorop;
     colorop = colorop->next) {
    colorop_state = drm_atomic_get_colorop_state(plane_state->state, colorop);
```

However, the function signature change is a notable API change:

```c
-drm_atomic_add_affected_colorops(struct drm_atomic_state *state,
+drm_atomic_add_affected_colorops(struct drm_plane_state *plane_state,
                                  struct drm_plane *plane)
```

**Concern:** The `plane` parameter is now redundant since it can be derived from `plane_state->plane`. Keeping both parameters is mildly inconsistent — consider whether `plane` should be dropped from the signature entirely to avoid callers passing a mismatched plane. That said, it mirrors the pattern of other DRM helpers that take both state and object, so this is acceptable as-is.

The null-guard at the top is a good addition:
```c
if (!plane_state || !plane_state->color_pipeline)
    return 0;
```

The removal of the `WARN_ON` for missing plane state is correct since the plane_state is now passed directly.

The callers in `drm_atomic_add_affected_planes` and `drm_atomic_helper_duplicate_state` are updated cleanly — the redundant `if (plane_state->color_pipeline)` checks are moved inside the function itself.

**Minor nit:** The `@state` parameter documentation says "DRM plane state" but the parameter is named `plane_state` — this is fine but slightly unusual for kerneldoc since `@state` doesn't match the parameter name `plane_state`. Looking at the header, the declaration parameter name is `state`, so the mismatch is between the .c and .h files. The declaration in `drm_atomic.h` uses `state` as the parameter name:

```c
drm_atomic_add_affected_colorops(struct drm_plane_state *state,
                                 struct drm_plane *plane);
```

This is acceptable, but ideally the parameter name in the header and implementation should match (`plane_state` in both places would be clearer).

---
Generated by Claude Code Patch Reviewer

Claude review: drm/atomic: don't set colorop properties of inactive color pipelines

[Claude Code Review Bot]

Patch Review

**Purpose:** Rejects userspace attempts to set properties on colorops that are not part of the currently active pipeline.

**Review:**

The validation logic is correct — walk the active pipeline to verify the target colorop belongs to it:

```c
for (active_colorop = plane_state->color_pipeline;
     active_colorop;
     active_colorop = active_colorop->next) {
    if (active_colorop == colorop) {
        colorop_state = drm_atomic_get_colorop_state(state, colorop);
```

**Concern — UAPI behavior change:** This introduces a new `-EINVAL` error for a scenario that previously succeeded silently. If any existing userspace was setting properties on colorops not in the active pipeline (even if it was a no-op), this will now break it. The commit message should ideally note this is an intentional UAPI tightening and explain why it's safe (e.g., no known userspace does this, or the old behavior was always a bug).

**Concern — `goto err` label placement:** The new `err:` label sits just before `drm_property_change_valid_put`:

```c
+err:
 	drm_property_change_valid_put(prop, ref);
 	return ret;
```

This means when `drm_atomic_get_colorop_state` fails (returns IS_ERR), the code does `ret = PTR_ERR(colorop_state); goto err;` which correctly calls `drm_property_change_valid_put`. For the "not part of active pipeline" case, `ret = -EINVAL; goto err;` also correctly calls the put function. The error handling is correct.

**Minor observation:** Getting the plane state (`drm_atomic_get_plane_state`) for every colorop property set has a cost, but it's necessary for the pipeline walk and is already cheap if the state is already in the atomic state (common case).

---
Generated by Claude Code Patch Reviewer

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

[Claude Code Review Bot]

Patch Review

**Purpose:** Documentation-only change removing "Read-only" comments from lut1d/3d_interpolation fields in preparation for making them mutable.

**Review:** Straightforward and correct. This is properly sequenced before patch 4 which actually makes the fields mutable. The authored-by is correctly attributed to Alex Hung.

No issues.

---
Generated by Claude Code Patch Reviewer

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

[Claude Code Review Bot]

Patch Review

**Purpose:** Moves `lut1d_interpolation` and `lut3d_interpolation` from `struct drm_colorop` (immutable HW object) to `struct drm_colorop_state` (per-atomic-state), making them properly mutable.

**Review:**

The state reset initialization is correctly done through `drm_object_property_get_default_value`:

```c
if (colorop->lut1d_interpolation_property) {
    drm_object_property_get_default_value(&colorop->base,
                                          colorop->lut1d_interpolation_property,
                                          &val);
    colorop_state->lut1d_interpolation = val;
}
```

**Minor concern:** The return value of `drm_object_property_get_default_value` is not checked here. Looking at the existing `curve_1d_type` pattern just above:

```c
if (colorop->curve_1d_type_property) {
    drm_object_property_get_default_value(&colorop->base,
                                          colorop->curve_1d_type_property,
                                          &val))
```

Wait — in the existing code, the return value IS checked as part of an `if` condition. But in the new code for interpolation properties, it is NOT:

```c
if (colorop->lut1d_interpolation_property) {
    drm_object_property_get_default_value(&colorop->base,
                                          colorop->lut1d_interpolation_property,
                                          &val);
    colorop_state->lut1d_interpolation = val;
}
```

Looking at the existing code for `curve_1d_type`, the pattern is:
```c
if (colorop->curve_1d_type_property) {
    if (!drm_object_property_get_default_value(&colorop->base,
                                               colorop->curve_1d_type_property,
                                               &val))
        colorop_state->curve_1d_type = val;
}
```

**The new code doesn't guard the assignment with the return value check.** If `drm_object_property_get_default_value` fails, `val` would be stale from a previous call (the `curve_1d_type` lookup). This is probably benign since the property is guaranteed to exist when the property pointer is non-NULL, but it's inconsistent with the existing pattern. This should match the existing defensive style.

The removal of `colorop->lut1d_interpolation = interpolation` from the init functions is correct since the value now lives in state.

The print_state and get/set property changes are mechanical and correct.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/atomic: track individual colorop updates

[Claude Code Review Bot]

Patch Review

**Purpose:** Uses the plane's `color_mgmt_changed` flag to track when any colorop property in the active pipeline changes value.

**Review:**

The approach of threading a `bool *replaced` through the set_property path is functional but adds some complexity. The pattern is:

```c
static int drm_atomic_colorop_set_property(..., bool *replaced)
{
    if (property == colorop->bypass_property) {
        if (state->bypass != val) {
            state->bypass = val;
            *replaced = true;
        }
    } ...
}
```

Then at the call site:
```c
bool replaced = false;
...
ret = drm_atomic_colorop_set_property(colorop, colorop_state,
                                      file_priv, prop, prop_value,
                                      &replaced);
if (!ret && replaced)
    plane_state->color_mgmt_changed = true;
```

**This is correct and well-designed.** It avoids setting `color_mgmt_changed` when the same value is written (no-change commit), which prevents unnecessary driver work. The `|=` for pipeline changes in `drm_atomic_set_colorop_for_plane` is also correct:

```c
state->color_mgmt_changed |= drm_atomic_set_colorop_for_plane(state, colorop);
```

**Design note on `replaced` naming:** The name `replaced` is borrowed from the blob property API (`drm_property_replace_blob`), but for scalar properties it reads slightly oddly. Something like `changed` might be clearer. This is a minor style preference.

**`drm_atomic_set_colorop_for_plane` return value change:** The function now returns `bool` instead of `void` and adds an early-return optimization:

```c
if (plane_state->color_pipeline == colorop)
    return false;
```

This is a good optimization — avoids logging and state changes when re-setting the same pipeline. The `#include <linux/types.h>` addition to `drm_atomic_uapi.h` for the `bool` return type is correct.

---
Generated by Claude Code Patch Reviewer

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

[Claude Code Review Bot]

Patch Review

**Purpose:** Makes the AMD display driver react to plane-level `color_mgmt_changed`, completing the end-to-end fix.

**Review:**

The `amdgpu_dm_commit_planes` change is straightforward:

```c
-if (new_pcrtc_state->color_mgmt_changed) {
+if (new_pcrtc_state->color_mgmt_changed || new_plane_state->color_mgmt_changed) {
```

This correctly extends the existing CRTC-level check to also cover plane-level colorop changes.

The `should_reset_plane` addition:

```c
+if (new_plane_state->color_mgmt_changed)
+    return true;
```

This is placed right after the CRTC `color_mgmt_changed` check, which is logically consistent. However, **returning `true` from `should_reset_plane` causes plane recreation** (remove + re-add to DC state), which is a heavier operation than just updating surface properties. Is a full plane reset always necessary for colorop changes, or would just the surface update in `amdgpu_dm_commit_planes` suffice?

Looking at the existing code, the CRTC `color_mgmt_changed` also triggers a plane reset, so this follows the established pattern. The comment says "Plane color pipeline or its colorop changes" which is clear.

**Overall this patch is correct** and represents the minimal driver-side change to consume the new tracking mechanism.

---

**Summary:** The series is well-crafted and addresses a real bug. The main substantive issue is the missing return value check for `drm_object_property_get_default_value` in patch 4's `__drm_colorop_state_reset` additions. The UAPI tightening in patch 2 should be called out more explicitly. Otherwise, this looks good to merge.

---
Generated by Claude Code Patch Reviewer