Claude review: drm/atomic: track individual colorop updates

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

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/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: 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/atomic: track individual colorop updates

[Claude Code Review Bot]

Overall Series Review

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

---

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

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

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

---

---
Generated by Claude Code Patch Reviewer

Claude review: drm/atomic: track individual colorop updates

[Claude Code Review Bot]

Patch Review

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

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

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

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

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

---

---
Generated by Claude Code Patch Reviewer

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: 5
Reviewed: 2026-05-26T07:18:46.332271

---

This is a well-structured v7 series that solves a real bug: colorop property changes (e.g., shaper/3D-LUT updates in gamescope night mode) were not being tracked, so the AMD display driver never reprogrammed its color blocks. The fix is clean and follows the existing pattern used for CRTC `color_mgmt_changed`.

The series logically progresses: patches 1-2 make lut interpolation properties mutable (moving them from `drm_colorop` to `drm_colorop_state`), patch 3 wires up change tracking in the DRM core, and patch 4 consumes the new flag in the AMD driver. The approach is correct and the code is well-reviewed (Harry Wentland, Chaitanya Kumar Borah R-b tags).

No correctness bugs found. One minor observation and one minor nit below.

---

---
Generated by Claude Code Patch Reviewer

Claude review: drm/atomic: track individual colorop updates

[Claude Code Review Bot]

Patch Review

This is the core patch. It sets `plane_state->color_mgmt_changed` whenever a colorop property value actually changes.

**Change to `drm_atomic_set_colorop_for_plane`**: Now returns `bool` indicating whether the color pipeline pointer actually changed. The early-out when `plane_state->color_pipeline == colorop` avoids unnecessary state dirtying when userspace re-sets the same pipeline. The only call site at line 615 uses the return value correctly:

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

The function is `EXPORT_SYMBOL` so theoretically out-of-tree callers exist, but since the return value is ignorable (void→bool), this is ABI-safe.

**Change to `drm_atomic_colorop_set_property`**: Each property branch now does a compare-before-assign and sets `*replaced = true` only on actual value change. This is correct for all scalar properties (bypass, lut1d_interpolation, curve_1d_type, multiplier, lut3d_interpolation). The `data_property` case passes `replaced` through to `drm_atomic_color_set_data_property` which delegates to `drm_property_replace_blob_from_id` — that function already computes `replaced` correctly for blobs.

**Change to `drm_atomic_set_property` (COLOROP case)**: After detecting a change, it pulls in the plane state and sets `color_mgmt_changed`:

```c
ret = drm_atomic_colorop_set_property(colorop, colorop_state,
                                      file_priv, prop, prop_value,
                                      &replaced);
if (ret || !replaced)
    break;

plane_state = drm_atomic_get_plane_state(state, colorop->plane);
...
plane_state->color_mgmt_changed |= replaced;
```

**Minor observation**: At line 1325, `plane_state->color_mgmt_changed |= replaced;` is guarded by `if (ret || !replaced)` above (line 1317), so `replaced` is always `true` when line 1325 is reached. The `|=` is harmless (and consistent with the style used elsewhere), but `= true` would be equivalent. This is not a bug — the existing code is fine for consistency and future-proofing.

**`#include <linux/types.h>`** added to `drm_atomic_uapi.h` for the `bool` return type — correct.

No issues.

---

---
Generated by Claude Code Patch Reviewer

[PATCH v8 0/4] drm/atomic: track individual colorop updates

[Melissa Wen]

This is a partial of [1], only with patches related to individual
colorop update tracking. I.e., I'm detaching from here fixes regarding
attempts of changing colorops that are not part of an active color
pipeline, or in the transition between active and inactive color
pipelines.

This series focus on tracking updates for each individual color
operation, allowing the driver to react accordingly. This new version
just adds r-b and Fixes tag accordingly. During the Display Next
Hackfest we also agree that it should be applied to drm-misc-fixes and
it can be backported by AMD after.

- Patches 1 and 2 make lut1d_interpolation and lut3d_interpolation
  colorops correctly behave as mutable, handling their changes via
  drm_colorop_state.

- Patches 3 and 4 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, but doesn't consider as a change
  when the same color pipeline value is set to the plane COLOR_PIPELINE
  prop. That way, the driver can react accordingly and update their
  color blocks. As interpolation properties become mutable, they are
  also tracked here.

It also fixes shaper/3D LUT updates when changing night mode settings on
gamescope with a custom branch that supports `COLOR_PIPELINE`:
- https://github.com/ValveSoftware/gamescope/pull/2113

v1: https://lore.kernel.org/dri-devel/20260318162348.299807-1-mwen@igalia.com/
Changes:
- include linux types for function's bool return type (kernel bot on MSM
  driver)
- add Harry's r-b tags

v2: https://lore.kernel.org/dri-devel/20260323131942.494217-1-mwen@igalia.com/
Changes:
- [NEW] two patches to only consider colorop updates from active color
  pipelines (Chaitanya)
- [NEW] make lut interpolation properties mutable + Alex H patch for
  kernel docs
- track lut(1/3)d_interpolation updates (Chaitanya)
- rebase changes according to new patches

v3: https://lore.kernel.org/dri-devel/20260403135909.214378-1-mwen@igalia.com/
Changes: rebase on drm-misc-next

v4: https://lore.kernel.org/dri-devel/20260501132527.522320-1-mwen@igalia.com/
Changes: fix kernel doc (kernel bot)

v5: https://lore.kernel.org/dri-devel/20260506192633.16066-1-mwen@igalia.com/
Changes:
- rebase on drm-misc-next
- fix kernel-doc and correctly reword (atomic) state to plane_state (Chaitanya)
- reject inactive colorop updates in atomic check time, instead of
  during property's setup, to avoid ordering dependency as pointed out by Chaitanya
- use `|= replaced` for consistency (Chaitanya)
- add Chaitanya's r-b tags to patches 1,3-5

[1] v6: https://lore.kernel.org/dri-devel/20260519211111.228303-1-mwen@igalia.com/
Changes:
- detach patches that implement individual tracking from those related
  to inactive colorop updates.

v7: https://lore.kernel.org/dri-devel/20260525100524.304263-1-mwen@igalia.com/
Changes:
- add Fixes and r-b tags

Alex Hung (1):
  drm/colorop: Remove read-only comments from interpolation fields

Melissa Wen (3):
  drm/colorop: make lut(1/3)d_interpolation props correctly behave as
    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                  |  4 +-
 drivers/gpu/drm/drm_atomic_uapi.c             | 68 +++++++++++++++----
 drivers/gpu/drm/drm_colorop.c                 | 16 ++++-
 include/drm/drm_atomic_uapi.h                 |  4 +-
 include/drm/drm_colorop.h                     | 34 +++++-----
 6 files changed, 93 insertions(+), 39 deletions(-)

-- 
2.53.0

[PATCH v8 1/4] 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().

Fixes: 7fa3ee8c0a79 ("drm/colorop: Define LUT_1D interpolation")
Fixes: db971856bbe0 ("drm/colorop: Add 3D LUT support to color pipeline")
Reviewed-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
Signed-off-by: Alex Hung <alex.hung@amd.com>
Signed-off-by: Melissa Wen <mwen@igalia.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 b4b9e4f558ab..5cea4d7c4efa 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 v8 2/4] drm/colorop: make lut(1/3)d_interpolation props correctly behave as mutable

[Melissa Wen]

As interpolation props are actually mutable props, any changes should be
handled by drm_colorop_state. Move their enum and make it correctly
behaves as mutable.

Fixes: 7fa3ee8c0a79 ("drm/colorop: Define LUT_1D interpolation")
Fixes: db971856bbe0 ("drm/colorop: Add 3D LUT support to color pipeline")
Reviewed-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
Reviewed-by: Alex Hung <alex.hung@amd.com>
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 3af1b9cc9a06..735ab7badc2e 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -910,7 +910,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:
@@ -922,7 +922,7 @@ static void drm_atomic_colorop_print_state(struct drm_printer *p,
 	case DRM_COLOROP_3D_LUT:
 		drm_printf_indent(p, 1, "size=%d\n", colorop->size);
 		drm_printf_indent(p, 1, "interpolation=%s\n",
-				  drm_get_colorop_lut3d_interpolation_name(colorop->lut3d_interpolation));
+				  drm_get_colorop_lut3d_interpolation_name(state->lut3d_interpolation));
 		drm_printf_indent(p, 1, "data blob id=%d\n", state->data ? state->data->base.id : 0);
 		break;
 	default:
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 6441b55cc274..78423905051e 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -751,13 +751,13 @@ static int drm_atomic_colorop_set_property(struct drm_colorop *colorop,
 	if (property == colorop->bypass_property) {
 		state->bypass = val;
 	} else if (property == colorop->lut1d_interpolation_property) {
-		colorop->lut1d_interpolation = val;
+		state->lut1d_interpolation = val;
 	} else if (property == colorop->curve_1d_type_property) {
 		state->curve_1d_type = val;
 	} else if (property == colorop->multiplier_property) {
 		state->multiplier = val;
 	} else if (property == colorop->lut3d_interpolation_property) {
-		colorop->lut3d_interpolation = val;
+		state->lut3d_interpolation = val;
 	} else if (property == colorop->data_property) {
 		return drm_atomic_color_set_data_property(colorop, state,
 							  property, val);
@@ -782,7 +782,7 @@ drm_atomic_colorop_get_property(struct drm_colorop *colorop,
 	else if (property == colorop->bypass_property)
 		*val = state->bypass;
 	else if (property == colorop->lut1d_interpolation_property)
-		*val = colorop->lut1d_interpolation;
+		*val = state->lut1d_interpolation;
 	else if (property == colorop->curve_1d_type_property)
 		*val = state->curve_1d_type;
 	else if (property == colorop->multiplier_property)
@@ -790,7 +790,7 @@ drm_atomic_colorop_get_property(struct drm_colorop *colorop,
 	else if (property == colorop->size_property)
 		*val = colorop->size;
 	else if (property == colorop->lut3d_interpolation_property)
-		*val = colorop->lut3d_interpolation;
+		*val = state->lut3d_interpolation;
 	else if (property == colorop->data_property)
 		*val = (state->data) ? state->data->base.id : 0;
 	else
diff --git a/drivers/gpu/drm/drm_colorop.c b/drivers/gpu/drm/drm_colorop.c
index c0eecde8c176..682fcc651525 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_init(struct drm_colorop_state *colorop_state,
 							   &val))
 			colorop_state->curve_1d_type = val;
 	}
+
+	if (colorop->lut1d_interpolation_property) {
+		if (!drm_object_property_get_default_value(&colorop->base,
+							   colorop->lut1d_interpolation_property,
+							   &val))
+			colorop_state->lut1d_interpolation = val;
+	}
+
+	if (colorop->lut3d_interpolation_property) {
+		if (!drm_object_property_get_default_value(&colorop->base,
+							   colorop->lut3d_interpolation_property,
+							   &val))
+			colorop_state->lut3d_interpolation = val;
+	}
 }
 
 /**
diff --git a/include/drm/drm_colorop.h b/include/drm/drm_colorop.h
index 5cea4d7c4efa..224fae40ed2b 100644
--- a/include/drm/drm_colorop.h
+++ b/include/drm/drm_colorop.h
@@ -183,6 +183,20 @@ struct drm_colorop_state {
 	 */
 	struct drm_property_blob *data;
 
+	/**
+	 * @lut1d_interpolation:
+	 *
+	 * Interpolation for DRM_COLOROP_1D_LUT
+	 */
+	enum drm_colorop_lut1d_interpolation_type lut1d_interpolation;
+
+	/**
+	 * @lut3d_interpolation:
+	 *
+	 * Interpolation for DRM_COLOROP_3D_LUT
+	 */
+	enum drm_colorop_lut3d_interpolation_type lut3d_interpolation;
+
 	/** @state: backpointer to global drm_atomic_commit */
 	struct drm_atomic_commit *state;
 };
@@ -306,20 +320,6 @@ struct drm_colorop {
 	 */
 	uint32_t size;
 
-	/**
-	 * @lut1d_interpolation:
-	 *
-	 * Interpolation for DRM_COLOROP_1D_LUT
-	 */
-	enum drm_colorop_lut1d_interpolation_type lut1d_interpolation;
-
-	/**
-	 * @lut3d_interpolation:
-	 *
-	 * Interpolation for DRM_COLOROP_3D_LUT
-	 */
-	enum drm_colorop_lut3d_interpolation_type lut3d_interpolation;
-
 	/**
 	 * @lut1d_interpolation_property:
 	 *
-- 
2.53.0

[PATCH v8 3/4] 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. Since we're here, only announce and
track changes to plane COLOR_PIPELINE prop if its value is actually
changing.

Fixes: 8c5ea1745f4c ("drm/colorop: Add BYPASS property")
Fixes: 7fa3ee8c0a79 ("drm/colorop: Define LUT_1D interpolation")
Fixes: 41651f9d42eb ("drm/colorop: Add 1D Curve subtype")
Fixes: 3410108037d5 ("drm/colorop: Add multiplier type")
Fixes: db971856bbe0 ("drm/colorop: Add 3D LUT support to color pipeline")
Fixes: e5719e7f1900 ("drm/colorop: Add 3x4 CTM type")
Fixes: 99a4e4f08abe ("drm/colorop: Add 1D Curve Custom LUT type")
Fixes: 2afc3184f3b3 ("drm/plane: Add COLOR PIPELINE property")
Reviewed-by: Harry Wentland <harry.wentland@amd.com> #v1
Reviewed-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
Reviewed-by: Alex Hung <alex.hung@amd.com>
Signed-off-by: Melissa Wen <mwen@igalia.com>
---
 drivers/gpu/drm/drm_atomic_uapi.c | 64 ++++++++++++++++++++++++-------
 include/drm/drm_atomic_uapi.h     |  4 +-
 2 files changed, 54 insertions(+), 14 deletions(-)

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

Fixes: 9ba25915efba ("drm/amd/display: Add support for sRGB EOTF in DEGAM block")
Reviewed-by: Harry Wentland <harry.wentland@amd.com>
Reviewed-by: Alex Hung <alex.hung@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 ba7f98a87808..ca97f1da8c0e 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -10215,7 +10215,7 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_commit *state,
 			continue;
 
 		bundle->surface_updates[planes_count].surface = dc_plane;
-		if (new_pcrtc_state->color_mgmt_changed) {
+		if (new_pcrtc_state->color_mgmt_changed || new_plane_state->color_mgmt_changed) {
 			bundle->surface_updates[planes_count].gamma = &dc_plane->gamma_correction;
 			bundle->surface_updates[planes_count].in_transfer_func = &dc_plane->in_transfer_func;
 			bundle->surface_updates[planes_count].gamut_remap_matrix = &dc_plane->gamut_remap_matrix;
@@ -12057,6 +12057,10 @@ static bool should_reset_plane(struct drm_atomic_commit *state,
 	if (new_crtc_state->color_mgmt_changed)
 		return true;
 
+	/* Plane color pipeline or its colorop changes. */
+	if (new_plane_state->color_mgmt_changed)
+		return true;
+
 	/*
 	 * On zpos change, planes need to be reordered by removing and re-adding
 	 * them one by one to the dc state, in order of descending zpos.
-- 
2.53.0

Claude review: drm/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: 5
Reviewed: 2026-06-04T12:16:43.925332

---

This is a clean, well-structured 4-patch bug-fix series at v8. It addresses two related issues in the DRM color pipeline (colorop) infrastructure:

1. **lut1d/lut3d interpolation properties were incorrectly treated as immutable** — stored on `drm_colorop` instead of `drm_colorop_state`, meaning changes from userspace went to the wrong place and weren't tracked per-atomic-state.

2. **Individual colorop property changes were not tracked** — changing a colorop property (bypass, curve type, LUT data, multiplier, interpolation) didn't set `plane_state->color_mgmt_changed`, so drivers had no way to know color blocks needed updating. This broke gamescope night-mode (shaper/3D-LUT updates).

The series is logically ordered: first fix docs (patch 1), then fix the data model (patch 2), then add change tracking in the core (patch 3), then wire it up in AMD's driver (patch 4). Each patch is independently correct, has appropriate Fixes tags, and carries reviewed-by tags from multiple reviewers (Intel, AMD). The code is straightforward, follows existing patterns, and I found no correctness bugs.

**Recommendation: This series looks ready to merge.**

---

---
Generated by Claude Code Patch Reviewer

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

[Claude Code Review Bot]

Patch Review

**Author:** Alex Hung

Trivial documentation fix. Removes "Read-only" from kernel-doc comments on `lut1d_interpolation`, `lut3d_interpolation`, `lut1d_interpolation_property`, and `lut3d_interpolation_property` fields in `struct drm_colorop`. The comments were wrong because `drm_atomic_colorop_set_property()` already allowed userspace to write these values.

**Verdict:** Correct and straightforward. No issues.

---

---
Generated by Claude Code Patch Reviewer

Claude review: drm/colorop: make lut(1/3)d_interpolation props correctly behave as mutable

[Claude Code Review Bot]

Patch Review

**Author:** Melissa Wen

This is the key data-model fix. It moves `lut1d_interpolation` and `lut3d_interpolation` from `struct drm_colorop` (the immutable base object) to `struct drm_colorop_state` (the per-atomic-commit mutable state).

**Changes reviewed:**

- **`include/drm/drm_colorop.h`**: Fields move from `drm_colorop` to `drm_colorop_state`. Clean move, correct types preserved.

- **`drm_atomic_uapi.c`**: `drm_atomic_colorop_set_property()` and `drm_atomic_colorop_get_property()` now read/write `state->lut1d_interpolation` / `state->lut3d_interpolation` instead of `colorop->lut1d_interpolation` / `colorop->lut3d_interpolation`. Correct.

- **`drm_atomic.c` (`drm_atomic_colorop_print_state`)**: Debug printing now reads from `state->` instead of `colorop->`. Correct.

- **`drm_colorop.c`**: The `__drm_colorop_state_reset()` function (called `__drm_colorop_state_init` in the mbox diff context lines — presumably renamed in the base tree) now initializes the interpolation fields from the property defaults via `drm_object_property_get_default_value()`. This follows the existing pattern for `curve_1d_type`. The `colorop->lut1d_interpolation = interpolation` assignments in `drm_plane_colorop_curve_1d_lut_init()` and `drm_plane_colorop_3dlut_init()` are correctly removed since the field no longer exists on `drm_colorop`.

**Potential concern:** The interpolation fields are unconditionally present on every `drm_colorop_state`, even for colorop types that don't use them (e.g., CTM_3X4, MULTIPLIER). This wastes a few bytes per state object but is harmless and consistent with how other fields like `data` are handled.

**Verdict:** Correct. No issues.

---

---
Generated by Claude Code Patch Reviewer

Claude review: drm/atomic: track individual colorop updates

[Claude Code Review Bot]

Patch Review

**Author:** Melissa Wen

This is the largest patch and the core behavioral change. It ensures `plane_state->color_mgmt_changed` is set whenever any colorop property in the plane's color pipeline actually changes value.

**Changes reviewed:**

- **`drm_atomic_set_colorop_for_plane()`** now returns `bool` instead of `void`. It returns `false` (no change) when the same colorop pointer is set again, `true` when it actually changes. The early-return for no-change avoids unnecessary debug logging and change tracking. Only one caller exists in-tree, at line 615, which uses `|=` to combine the result into `color_mgmt_changed`. Correct.

- **`drm_atomic_colorop_set_property()`** gains a `bool *replaced` output parameter. Each property branch now compares old vs new value before writing, and only sets `*replaced = true` when the value actually changed:

  ```c
  if (state->bypass != val) {
      state->bypass = val;
      *replaced = true;
  }
  ```

  This pattern is applied to `bypass`, `lut1d_interpolation`, `curve_1d_type`, `multiplier`, and `lut3d_interpolation`. The `data` property delegates to `drm_atomic_color_set_data_property()` which passes the pointer through to `drm_property_replace_blob_from_id()` — that function already has a `bool *replaced` parameter with the correct semantics.

- **`drm_atomic_set_property()` (DRM_MODE_OBJECT_COLOROP case)**: After calling `drm_atomic_colorop_set_property()`, if `ret || !replaced` it breaks early (no plane state needed). Otherwise it fetches the plane state and sets `plane_state->color_mgmt_changed |= replaced`. The `drm_atomic_get_plane_state()` call may fail (returns ERR_PTR), which is correctly handled.

- **`include/drm/drm_atomic_uapi.h`**: Adds `#include <linux/types.h>` for the `bool` return type, and updates the declaration. Clean.

**One minor observation:** At line 1325, `plane_state->color_mgmt_changed |= replaced;` — at this point `replaced` is guaranteed `true` (the `if (ret || !replaced) break;` on line 1317 already filtered the false case), so the `|=` is equivalent to `= true`. This is fine — using `|=` is defensive and matches the convention used elsewhere (e.g., line 615).

**Verdict:** Correct and well-implemented. No issues.

---

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

**Author:** Melissa Wen

Wires up the new `plane_state->color_mgmt_changed` flag in AMD's display driver, at two critical decision points:

1. **`amdgpu_dm_commit_planes()`** (line 10218): The condition for sending color surface updates to DC now checks `new_plane_state->color_mgmt_changed` in addition to `new_pcrtc_state->color_mgmt_changed`:

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

   This ensures that when a colorop property changes (e.g., shaper/3D-LUT update for gamescope night mode), the driver actually sends the updated gamma, transfer functions, gamut remap, HDR mult, shaper, 3D LUT, and blend TF to DC.

2. **`should_reset_plane()`** (line 12060–12062): Returns `true` when `new_plane_state->color_mgmt_changed` is set, causing the plane to be removed from and re-added to the DC state. This is the same behavior already triggered by `new_crtc_state->color_mgmt_changed`.

**Verdict:** Correct, minimal, and directly fixes the reported gamescope issue. No issues.

---
Generated by Claude Code Patch Reviewer