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

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

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

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

---
v5: fix kernel-doc for plane_state (kernel bot)
---
 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..8eb519673fc5 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
+ * @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 v5 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 v5 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 v5 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 8eb519673fc5..1386172af92b 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 v5 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 v5 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

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: 7
Reviewed: 2026-05-07T13:05:37.464407

---

This is a well-structured v5 series that addresses a real functional bug: colorop property updates (e.g., shaper/3D LUT changes for gamescope night mode) were not triggering driver-level color block reprogramming. The series is logically decomposed into three pairs:

1. **Patches 1-2**: Scope colorop state tracking to only the *active* color pipeline (optimization + correctness)
2. **Patches 3-4**: Make `lut1d_interpolation` and `lut3d_interpolation` properly mutable by moving them from `drm_colorop` (immutable object) to `drm_colorop_state` (per-commit state)
3. **Patches 5-6**: Add `color_mgmt_changed` tracking for plane colorop changes and wire it into the AMD display driver

The series is clean, the ordering is correct (each patch builds on the prior), and the approach of reusing the existing `color_mgmt_changed` pattern from CRTC state is sound. One notable concern: the `goto err` label introduced in patch 2 is shared with other cases in the switch, which could have unintended side effects if future cases are added between the COLOROP case and the label. Otherwise, the series looks good.

---

---
Generated by Claude Code Patch Reviewer

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

[Claude Code Review Bot]

Patch Review

**Assessment: Good, minor nit**

This patch changes `drm_atomic_add_affected_colorops()` to take a `drm_plane_state *` instead of `drm_atomic_state *`, and walks only the active `color_pipeline` linked list instead of iterating all colorops on the device and filtering by plane.

The key behavioral change is sound — previously `drm_for_each_colorop(colorop, plane->dev)` iterated *all* colorops across *all* planes on the device, filtering by `colorop->plane != plane`. Now it walks only the active pipeline chain:

```c
for (colorop = plane_state->color_pipeline;
     colorop;
     colorop = colorop->next) {
```

This is both more efficient and more correct (no states get created for colorops from inactive pipelines).

The NULL guard at the top is clean:
```c
if (!plane_state || !plane_state->color_pipeline)
    return 0;
```

The callers in `drm_atomic_add_affected_planes()` and `drm_atomic_helper_duplicate_state()` are updated to remove the now-redundant `if (plane_state->color_pipeline)` check and pass `plane_state` instead of `state`.

**Nit**: The kernel-doc parameter name changed from `@state` to `@plane_state`, but the declaration in `drm_atomic.h` still uses the name `state`:
```c
drm_atomic_add_affected_colorops(struct drm_plane_state *state,
```
This is technically valid C, but calling a `drm_plane_state *` parameter "`state`" is confusing since "state" in the DRM atomic world typically refers to `drm_atomic_state`. Consider naming it `plane_state` in the header declaration to match the implementation.

The removal of `WARN_ON(!drm_atomic_get_new_plane_state(state, plane))` makes sense since the function now receives the plane_state directly.

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

**Assessment: Good, one concern about goto label scope**

This patch adds validation in `drm_atomic_set_property()` to reject attempts to set properties on colorops that are not part of the currently active color pipeline. The logic is straightforward:

```c
plane_state = drm_atomic_get_plane_state(state, colorop->plane);
...
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);
        ...
        break;
    }
}

if (!colorop_state) {
    ...
    ret = -EINVAL;
    goto err;
}
```

**Concern**: The `err:` label is added *after* the closing brace of the `default:` case and before the common cleanup code `drm_property_change_valid_put(prop, ref)`. This means that `goto err` from the COLOROP case will still correctly call `drm_property_change_valid_put()`, which is the intended behavior. However, the label is structurally placed outside the switch statement, which means any future code adding error paths in *other* cases might accidentally use this label. This isn't a bug today, but the asymmetry (only the COLOROP case uses `goto err`, all other cases use `break`) is worth noting. An alternative would be to set `ret = -EINVAL` and `break` instead of `goto err`, since both paths end up at `drm_property_change_valid_put()` anyway.

**Observation**: The call to `drm_atomic_get_plane_state()` here means that any userspace attempt to set a colorop property will also pull in the plane state, even if the set ultimately fails. This is a new side effect but probably fine since plane state is cheap and necessary for the validation.

---
Generated by Claude Code Patch Reviewer

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

[Claude Code Review Bot]

Patch Review

**Assessment: Straightforward documentation fix, looks good**

This is a simple kernel-doc cleanup by Alex Hung that removes stale "Read-only" comments from `lut1d_interpolation`, `lut3d_interpolation`, and their associated property fields. The comments were inaccurate since userspace can already set these via `drm_atomic_colorop_set_property()`. This correctly prepares for patch 4 which makes them truly mutable.

No concerns.

---
Generated by Claude Code Patch Reviewer

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

[Claude Code Review Bot]

Patch Review

**Assessment: Good, well-structured**

This patch moves `lut1d_interpolation` and `lut3d_interpolation` from `struct drm_colorop` (the immutable hardware object) to `struct drm_colorop_state` (the per-commit mutable state). This is the right pattern — properties that userspace can change per atomic commit must live in the state struct.

The changes are consistent:
- **drm_colorop.h**: Fields moved from `drm_colorop` to `drm_colorop_state`
- **drm_atomic.c**: Print state reads from `state->lut1d_interpolation` / `state->lut3d_interpolation` instead of `colorop->...`
- **drm_atomic_uapi.c**: Set/get property uses `state->...` instead of `colorop->...`
- **drm_colorop.c**: Default values initialized in `__drm_colorop_state_reset()` via `drm_object_property_get_default_value()`, and the assignment `colorop->lut1d_interpolation = interpolation` removed from init functions

The initialization in `__drm_colorop_state_reset()` correctly mirrors the pattern already used for `curve_1d_type`:

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

Since `__drm_atomic_helper_colorop_duplicate_state()` does a `memcpy` of the entire state, the new fields will be correctly preserved during state duplication without additional code.

**Note**: The return value of `drm_object_property_get_default_value()` is not checked for errors (unlike the existing `curve_1d_type` case which uses `if (!drm_object_property_get_default_value(...))`). The existing code for `curve_1d_type` wraps the call in an `if (!drm_object_property_get_default_value(...))` condition. The new code for `lut1d_interpolation` and `lut3d_interpolation` does not check the return value — the value will be used even if the call fails. This is inconsistent with the existing pattern but practically harmless since the property existence check (`if (colorop->lut1d_interpolation_property)`) ensures the property exists and the default value lookup should succeed.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/atomic: track individual colorop updates

[Claude Code Review Bot]

Patch Review

**Assessment: Good, one design observation**

This is the core patch that wires up `color_mgmt_changed` tracking. The approach is clean:

1. `drm_atomic_set_colorop_for_plane()` now returns `bool` (true if pipeline changed) and the caller uses `|=` to accumulate into `plane_state->color_mgmt_changed`.

2. `drm_atomic_colorop_set_property()` and `drm_atomic_color_set_data_property()` gain a `bool *replaced` output parameter to signal when a property value actually changed.

3. In `drm_atomic_set_property()`, after setting a colorop property:
```c
if (!ret && replaced)
    plane_state->color_mgmt_changed = true;
```

The change detection in `drm_atomic_colorop_set_property()` correctly checks each property individually:
```c
if (state->bypass != val) {
    state->bypass = val;
    *replaced = true;
}
```

**Design observation**: The `replaced` variable is initialized to `false` but only set to `true` — it's never checked before being returned. For `drm_atomic_color_set_data_property()`, the `replaced` pointer is forwarded directly to `drm_property_replace_blob_from_id()`, which handles blob comparison internally. This is correct because `drm_property_replace_blob_from_id()` already sets `*replaced` to indicate whether the blob pointer changed.

**Observation about `drm_atomic_set_colorop_for_plane()`**: The early return `if (plane_state->color_pipeline == colorop) return false` is a nice optimization — it avoids the debug prints for no-op pipeline selections. However, this changes behavior slightly: previously, the debug message would fire even for redundant sets. This is fine since it's debug-only.

The `#include <linux/types.h>` in `drm_atomic_uapi.h` for `bool` is correct and matches the v2 changelog note about fixing the MSM driver build.

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

**Assessment: Good, straightforward driver-side consumer**

This wires the new `color_mgmt_changed` flag into the AMD display driver at two critical points:

1. **`amdgpu_dm_commit_planes()`**: The color surface updates (gamma, transfer func, gamut remap, HDR mult, shaper, 3D LUT, blend TF) now also trigger when `new_plane_state->color_mgmt_changed` is set:
```c
if (new_pcrtc_state->color_mgmt_changed || new_plane_state->color_mgmt_changed) {
```
This is the actual bug fix — without this, changing colorop properties (like shaper/3D LUT for night mode) wouldn't cause DC to reprogram the color blocks.

2. **`should_reset_plane()`**: Returns true when `new_plane_state->color_mgmt_changed` is set, causing plane reset during atomic check. This ensures the DC plane state is fully recreated when colorop properties change.

Both additions are consistent with the existing CRTC `color_mgmt_changed` pattern. The comment `/* Plane color pipeline or its colorop changes. */` is appropriate.

Harry's R-b tag from v1 carried forward is noted.

**One question for the maintainer**: `should_reset_plane()` returning true can trigger full plane recreation which is relatively expensive. For the gamescope night mode use case (shaper/3D LUT data changes), this should be fine since it happens on user-initiated actions, not per-frame. But if a compositor were to update colorop properties every frame, this could cause performance issues. The existing CRTC `color_mgmt_changed` path has the same characteristic, so this is consistent, but worth keeping in mind.

---
Generated by Claude Code Patch Reviewer