[PATCH v2 0/2] drm/atomic: track colorop changes of a given plane

[PATCH v2 0/2] drm/atomic: track colorop changes of a given plane

[Melissa Wen]

Similar to what is done for tracking CRTC color mgmt property changes
with CRTC `color_mgmt_changed` flag, track colorop updates of a given
plane color pipeline by setting plane `color_mgmt_changed` flag. Also
true if setting a different color pipeline to a given plane. That way,
the driver can react accordingly and update their color blocks.

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

This series doesn't cover 1D/3D LUT interpolation, since it's documented
as read-only properties.

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

Let me know your thoughts!

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

Melissa Wen

Melissa Wen (2):
  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_uapi.c             | 53 +++++++++++++++----
 include/drm/drm_atomic_uapi.h                 |  4 +-
 3 files changed, 50 insertions(+), 13 deletions(-)

-- 
2.53.0

[PATCH v2 1/2] 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>
---

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

diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 87de41fb4459..713fa9e81732 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);
 
@@ -600,7 +608,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,
@@ -709,11 +717,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:
@@ -735,28 +743,39 @@ 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) {
 		colorop->lut1d_interpolation = val;
 	} 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) {
 		colorop->lut3d_interpolation = val;
 	} 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",
@@ -1273,6 +1292,8 @@ int drm_atomic_set_property(struct drm_atomic_state *state,
 	case DRM_MODE_OBJECT_COLOROP: {
 		struct drm_colorop *colorop = obj_to_colorop(obj);
 		struct drm_colorop_state *colorop_state;
+		struct drm_plane_state *plane_state;
+		bool replaced = false;
 
 		colorop_state = drm_atomic_get_colorop_state(state, colorop);
 		if (IS_ERR(colorop_state)) {
@@ -1281,7 +1302,17 @@ 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)
+			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 = 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

Claude review: drm/atomic: track individual colorop updates

[Claude Code Review Bot]

Patch Review

**Overall:** Solid patch. The approach of threading a `bool *replaced` through the colorop property-set path and using it to conditionally set `plane_state->color_mgmt_changed` is clean and consistent.

**Observations:**

1. **`drm_atomic_set_colorop_for_plane` return value change** (lines 213-233): Changing from `void` to `bool` and adding the early-return short-circuit is good. The usage at line 241:
   ```c
   state->color_mgmt_changed |= drm_atomic_set_colorop_for_plane(state, colorop);
   ```
   Correctly uses `|=` to avoid clearing a previously-set flag. Good.

2. **Interpolation properties intentionally not tracked** (lines 282, 295): The `lut1d_interpolation` and `lut3d_interpolation` properties are set directly on the `colorop` object (not `state`) and don't set `*replaced`. The cover letter mentions these are read-only properties and intentionally excluded. This is correct - they describe hardware capabilities rather than userspace-settable state.

3. **`replaced` naming convention**: The `bool *replaced` out-parameter is functional but the name is slightly misleading for scalar properties like `bypass` or `multiplier` where nothing is being "replaced" (that term fits blob properties better). Something like `changed` would be more natural. Minor style nit, not worth a respin.

4. **Plane state fetch on colorop change** (lines 324-329): In `drm_atomic_set_property`, when a colorop value changes, the code fetches the plane state via `drm_atomic_get_plane_state()`. This is correct - it ensures the plane is part of the atomic commit and its state is properly tracked. The error handling with `PTR_ERR` is also correct.

5. **`#include <linux/types.h>`** (line 341): Added to provide `bool` for the header. This is the right fix for the build issue the kernel bot reported on MSM.

6. **Missing tracking for `drm_atomic_colorop_set_property` unknown property path** (lines 302-306): If the property is unknown, the function returns `-EINVAL` and `*replaced` remains `false`. The caller at line 321 checks `if (ret || !replaced)` and breaks. This correctly avoids setting `color_mgmt_changed` on error. Good.

7. **Thread safety consideration**: The `replaced` variable is stack-local in `drm_atomic_set_property` (line 310), so no concurrency concerns. Fine.

---
Generated by Claude Code Patch Reviewer

[PATCH v2 2/2] 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 dfe95c9b8746..dc3f284d0834 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -9955,7 +9955,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;
@@ -11695,6 +11695,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/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

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: 3
Reviewed: 2026-03-25T07:51:14.429216

---

This is a clean, well-structured 2-patch series that adds change tracking for colorop properties in the DRM atomic API's color pipeline. The approach mirrors the existing `color_mgmt_changed` pattern already used for CRTC color management, which is good for consistency. The series solves a real bug (gamescope night mode shaper/3D LUT updates not taking effect) with minimal, focused changes.

The overall design is sound: patch 1 adds the core infrastructure in the DRM atomic helpers, and patch 2 consumes it in the amdgpu driver. The change-detection logic (only marking `color_mgmt_changed` when values actually differ) avoids unnecessary driver work on redundant sets, which is a nice touch.

**Series verdict: Looks good.** A few minor observations below, but nothing blocking.

---

---
Generated by Claude Code Patch Reviewer