[PATCH v2 0/2] drm/colorop: Keep colorop state consistent across atomic commits

[PATCH v2 0/2] drm/colorop: Keep colorop state consistent across atomic commits

[Chaitanya Kumar Borah]

This series aims to keep colorop state consistent across atomic
transactions by ensuring it accurately reflects committed hardware
state and remains part of the atomic update whenever its associated
plane is involved.

It contains two changes:
- Preserves the bypass value in duplicated colorop state.

_drm_atomic_helper_colorop_duplicate_state() unconditionally reset
bypass to true, which means the duplicated state no longer reflects the
committed hardware state. Since bypass directly controls whether the
colorop is active in hardware, this can lead to an unintended disable
during subsequent commits.

This could potentially be a problem also for colorops where bypass value
is immutably false.

Conceptually, I consider 'bypass' to behave similar to 'visible' in plane 
state - it represents current HW state and should therefore be preserved
across duplication.

- Add affected colorops with affected plane

Colorops are unique in the DRM model. While they are DRM objects with their
own states, they are logically attached to a plane and exposed through
a plane property. In some sense, they share the same hierarchy as CRTC and
planes while following a different 'ownership' model.

Given that enabling a CRTC pulls in all its affected planes into the atomic
state, it follows that when a plane is added, its associated colorops are
also included. Otherwise, during modesets or internal commits, colorop state
may be missing from the transaction, resulting in inconsistent or incomplete
state updates.

That said, I do have a concern about potentially inflating the atomic
state by automatically pulling in colorops from the core. It is not
entirely clear to me whether inclusion of affected colorops should be
handled in core, or left to individual drivers.

My understanding of the atomic framework is still evolving, so
I would appreciate feedback from those more familiar with the intended
design direction.

==
Chaitanya

P.S/Background/TL;DR:

I discovered inconsistency with the colorop state while analysing CRC mismatches
in kms_color_pipeline test cases[1]. Visual inspection reveals that while CRC is
being collected degamma block has been reset. This was traced back to the internal
commit that the driver does to disable PSR2 and selective fetch for CRC collection.

crtc_crc_open
    -> intel_crtc_set_crc_source
        -> intel_crtc_crc_setup_workarounds
            -> drm_atomic_commit

During this flow colorop states are never added to the atomic state which in turn
makes intel_plane_color_copy_uapi_to_hw_state() disable the colorops.

If we add the colorops, to the atomic state, the problem still persisted because
while duplicating the colorop state, 'bypass' was getting reset to true.

The two changes made in this series fixes the issue.

[1] https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_18001/shard-mtlp-6/igt@kms_color_pipeline@plane-lut1d.html

v2:
  - Add affected colorops only when a pipeline is enabled

Cc: Simon Ser <contact@emersion.fr>
Cc: Alex Hung <alex.hung@amd.com>
Cc: Harry Wentland <harry.wentland@amd.com>
Cc: Daniel Stone <daniels@collabora.com>
Cc: Melissa Wen <mwen@igalia.com>
Cc: Sebastian Wick <sebastian.wick@redhat.com>
Cc: Alex Hung <alex.hung@amd.com>
Cc: Uma Shankar <uma.shankar@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Louis Chauvet <louis.chauvet@bootlin.com>
Cc: <stable@vger.kernel.org> #v6.19+

Chaitanya Kumar Borah (2):
  drm/colorop: Preserve bypass value in duplicate_state()
  drm/atomic: Add affected colorops with affected planes

 drivers/gpu/drm/drm_atomic.c  | 7 +++++++
 drivers/gpu/drm/drm_colorop.c | 2 --
 2 files changed, 7 insertions(+), 2 deletions(-)

-- 
2.25.1

[PATCH v2 1/2] drm/colorop: Preserve bypass value in duplicate_state()

[Chaitanya Kumar Borah]

__drm_atomic_helper_colorop_duplicate_state() unconditionally
sets state->bypass = true after copying the existing state.

This override causes the new atomic state to no longer reflect
the currently committed hardware state. Since the bypass property
directly controls whether the colorop is active in hardware,
resetting it to true can inadvertently disable an active colorop
during a subsequent commit, particularly for internal driver commits
where userspace does not touch the property.

Drop the unconditional assignment and preserve the duplicated
bypass value.

Fixes: 8c5ea1745f4c ("drm/colorop: Add BYPASS property")
Cc: <stable@vger.kernel.org> #v6.19+
Reviewed-by: Uma Shankar <uma.shankar@intel.com>
Signed-off-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
---
 drivers/gpu/drm/drm_colorop.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_colorop.c b/drivers/gpu/drm/drm_colorop.c
index f421c623b3f0..e44a738c4c14 100644
--- a/drivers/gpu/drm/drm_colorop.c
+++ b/drivers/gpu/drm/drm_colorop.c
@@ -466,8 +466,6 @@ static void __drm_atomic_helper_colorop_duplicate_state(struct drm_colorop *colo
 
 	if (state->data)
 		drm_property_blob_get(state->data);
-
-	state->bypass = true;
 }
 
 struct drm_colorop_state *
-- 
2.25.1

[PATCH v2 2/2] drm/atomic: Add affected colorops with affected planes

[Chaitanya Kumar Borah]

When drm_atomic_add_affected_planes() adds a plane to the atomic
state, the associated colorops are not guaranteed to be included.
This can leave colorop state out of the transaction when planes
are pulled in implicitly (eg. during modeset or internal commits).

Also add affected colorops when adding affected planes to keep
plane and color pipeline state consistent within the atomic
transaction.

v2: Add affected colorops only when a pipeline is enabled

Fixes: 2afc3184f3b3 ("drm/plane: Add COLOR PIPELINE property")
Cc: <stable@vger.kernel.org> #v6.19+
Reviewed-by: Uma Shankar <uma.shankar@intel.com> #v1
Signed-off-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
---
 drivers/gpu/drm/drm_atomic.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 04925166df98..dd9f27cfe991 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1587,6 +1587,7 @@ drm_atomic_add_affected_planes(struct drm_atomic_state *state,
 	const struct drm_crtc_state *old_crtc_state =
 		drm_atomic_get_old_crtc_state(state, crtc);
 	struct drm_plane *plane;
+	int ret;
 
 	WARN_ON(!drm_atomic_get_new_crtc_state(state, crtc));
 
@@ -1600,6 +1601,12 @@ 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;
+		}
 	}
 	return 0;
 }
-- 
2.25.1

Claude review: drm/colorop: Keep colorop state consistent across atomic commits

[Claude Code Review Bot]

Overall Series Review

Subject: drm/colorop: Keep colorop state consistent across atomic commits
Author: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
Patches: 3
Reviewed: 2026-03-11T13:23:06.659608

---

This is a well-motivated 2-patch fix series addressing colorop state inconsistency during internal/driver-initiated atomic commits. The cover letter clearly explains the problem: when Intel's PSR2/CRC workaround triggers an internal commit via `drm_atomic_add_affected_planes()`, colorop states are either missing from the transaction or, when included, have their `bypass` property incorrectly reset to `true` during state duplication. The two patches work together to fix both sides of the bug.

The changes are small, targeted, and appropriate for stable backport. The fix is correct in my assessment, though patch 2 has a minor concern worth discussing.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/colorop: Preserve bypass value in duplicate_state()

[Claude Code Review Bot]

Patch Review

**Verdict: Looks good.**

The fix removes the unconditional `state->bypass = true` assignment after `memcpy()` in `__drm_atomic_helper_colorop_duplicate_state()`:

```c
 	if (state->data)
 		drm_property_blob_get(state->data);
-
-	state->bypass = true;
```

This is clearly correct. The `memcpy()` on line 465 already copies the entire state including `bypass`. Overwriting it to `true` afterwards defeats the purpose of duplicating the current hardware state — exactly the same pattern used for plane and CRTC state duplication, where no fields are unconditionally reset to non-hardware values after the copy.

As the cover letter notes, this is also a problem for colorops where bypass is immutably false — the forced `true` would be contradicting the hardware capabilities.

One question: was there an original reason this line existed? Looking at the commit message for `8c5ea1745f4c` ("drm/colorop: Add BYPASS property") might reveal if this was intentional to ensure bypass-by-default semantics for userspace-initiated commits. If so, the change is still correct (userspace should set bypass explicitly), but it's worth understanding the original intent to ensure no regressions.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/atomic: Add affected colorops with affected planes

[Claude Code Review Bot]

Patch Review

**Verdict: Mostly good, one concern.**

The change adds colorops to the atomic state when `drm_atomic_add_affected_planes()` pulls in a plane:

```c
+		if (plane_state->color_pipeline) {
+			ret = drm_atomic_add_affected_colorops(state, plane);
+			if (ret)
+				return ret;
+		}
```

The guard on `plane_state->color_pipeline` (added in v2) is a reasonable optimization to avoid pulling in colorop state when no pipeline is active, keeping the atomic state small for the common case.

**Concern about which state is checked:** `plane_state` here is the return value of `drm_atomic_get_plane_state()`, which returns the **new** (duplicated) plane state. Since `__drm_atomic_helper_plane_duplicate_state()` preserves `color_pipeline` via `memcpy()`, this correctly reflects the committed hardware state. This is fine.

**Minor concern — should this be unconditional?** The `color_pipeline` check means that if a plane's pipeline was *just disabled* in the same transaction (e.g., userspace set `COLOR_PIPELINE` to NULL in this commit), the colorops won't be pulled in. In theory this could matter if the driver needs to see the colorop state transition from active→bypass. However, since `drm_atomic_add_affected_planes()` operates on the *old* crtc state's plane mask (planes that were enabled before this commit), and the plane state here is the duplicated (pre-commit) state, `color_pipeline` should still reflect the old active pipeline. So this should be fine in practice.

**The `Reviewed-by: Uma Shankar` tag says `#v1`** — this was reviewed on v1 but v2 added the `color_pipeline` guard. The tag should ideally be re-confirmed for v2 since the logic changed (previously it would unconditionally add colorops, now it's conditional).

**Overall:** Both patches are correct and well-scoped fixes for a real bug. The series is suitable for merging and stable backport.

---
Generated by Claude Code Patch Reviewer