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

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

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  | 5 +++++
 drivers/gpu/drm/drm_colorop.c | 2 --
 2 files changed, 5 insertions(+), 2 deletions(-)

-- 
2.25.1

[PATCH 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+
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 aa19de769eb2..5037efcc3497 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 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.

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

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index e3029c8f02e5..8bcd76aaeb6a 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1588,6 +1588,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));
 
@@ -1601,6 +1602,10 @@ drm_atomic_add_affected_planes(struct drm_atomic_state *state,
 
 		if (IS_ERR(plane_state))
 			return PTR_ERR(plane_state);
+
+		ret = drm_atomic_add_affected_colorops(state, plane);
+		if (ret)
+			return ret;
 	}
 	return 0;
 }
-- 
2.25.1

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

[Shankar, Uma]

> -----Original Message-----
> From: Borah, Chaitanya Kumar <chaitanya.kumar.borah@intel.com>
> Sent: Wednesday, February 18, 2026 12:27 PM
> To: dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; intel-
> xe@lists.freedesktop.org
> Cc: contact@emersion.fr; alex.hung@amd.com; harry.wentland@amd.com;
> daniels@collabora.com; mwen@igalia.com; sebastian.wick@redhat.com;
> Shankar, Uma <uma.shankar@intel.com>; ville.syrjala@linux.intel.com;
> maarten.lankhorst@linux.intel.com; Nikula, Jani <jani.nikula@intel.com>;
> louis.chauvet@bootlin.com; stable@vger.kernel.org; Borah, Chaitanya Kumar
> <chaitanya.kumar.borah@intel.com>
> Subject: [PATCH 1/2] drm/colorop: Preserve bypass value in duplicate_state()
> 
> __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.

Looks Good to me.
Reviewed-by: Uma Shankar <uma.shankar@intel.com>

> Fixes: 8c5ea1745f4c ("drm/colorop: Add BYPASS property")
> Cc: <stable@vger.kernel.org> #v6.19+
> 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
> aa19de769eb2..5037efcc3497 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

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

[Shankar, Uma]

> -----Original Message-----
> From: Borah, Chaitanya Kumar <chaitanya.kumar.borah@intel.com>
> Sent: Wednesday, February 18, 2026 12:27 PM
> To: dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; intel-
> xe@lists.freedesktop.org
> Cc: contact@emersion.fr; alex.hung@amd.com; harry.wentland@amd.com;
> daniels@collabora.com; mwen@igalia.com; sebastian.wick@redhat.com;
> Shankar, Uma <uma.shankar@intel.com>; ville.syrjala@linux.intel.com;
> maarten.lankhorst@linux.intel.com; Nikula, Jani <jani.nikula@intel.com>;
> louis.chauvet@bootlin.com; stable@vger.kernel.org; Borah, Chaitanya Kumar
> <chaitanya.kumar.borah@intel.com>
> Subject: [PATCH 2/2] drm/atomic: Add affected colorops with affected planes
> 
> 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.

Even though colorop is an object in itself but practically it doesn't have any existence without
the plane. So to add to state along with plane seems logical. Also its good to handle this in
drm core than individual drivers.

The change looks good to me.
Reviewed-by: Uma Shankar <uma.shankar@intel.com>

> Fixes: 2afc3184f3b3 ("drm/plane: Add COLOR PIPELINE property")
> Cc: <stable@vger.kernel.org> #v6.19+
> Signed-off-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
> ---
>  drivers/gpu/drm/drm_atomic.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index
> e3029c8f02e5..8bcd76aaeb6a 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1588,6 +1588,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));
> 
> @@ -1601,6 +1602,10 @@ drm_atomic_add_affected_planes(struct
> drm_atomic_state *state,
> 
>  		if (IS_ERR(plane_state))
>  			return PTR_ERR(plane_state);
> +
> +		ret = drm_atomic_add_affected_colorops(state, plane);
> +		if (ret)
> +			return ret;
>  	}
>  	return 0;
>  }
> --
> 2.25.1

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

[Harry Wentland]

On 2026-02-18 01:57, Chaitanya Kumar Borah wrote:
> 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 tends to reflect my thinking when I wrote the colorop stuff.

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

Could this lead drivers to reprogram possibly expensive colorops
when they didn't change? It won't be an issue for amdgpu since we
have another level of state tracking, but for drivers that strictly
follow the atomic model it might lead to issues.

On the other hand it makes colorop handling less error-prone in amdgpu,
and possibly fixes a bug I've come across where we get confused if an
active colorop isn't part of the state.

Harry

> 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
> 
> 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  | 5 +++++
>  drivers/gpu/drm/drm_colorop.c | 2 --
>  2 files changed, 5 insertions(+), 2 deletions(-)
> 

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

[Shankar, Uma]

> -----Original Message-----
> From: Harry Wentland <harry.wentland@amd.com>
> Sent: Tuesday, February 24, 2026 2:44 AM
> To: Borah, Chaitanya Kumar <chaitanya.kumar.borah@intel.com>; dri-
> devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; intel-
> xe@lists.freedesktop.org
> Cc: contact@emersion.fr; alex.hung@amd.com; daniels@collabora.com;
> mwen@igalia.com; sebastian.wick@redhat.com; Shankar, Uma
> <uma.shankar@intel.com>; ville.syrjala@linux.intel.com;
> maarten.lankhorst@linux.intel.com; Nikula, Jani <jani.nikula@intel.com>;
> louis.chauvet@bootlin.com; stable@vger.kernel.org
> Subject: Re: [PATCH 0/2] drm/colorop: Keep colorop state consistent across
> atomic commits
> 
> On 2026-02-18 01:57, Chaitanya Kumar Borah wrote:
> > 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 tends to reflect my thinking when I wrote the colorop stuff.
> 
> > 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.
> >
> 
> Could this lead drivers to reprogram possibly expensive colorops when they didn't
> change? It won't be an issue for amdgpu since we have another level of state
> tracking, but for drivers that strictly follow the atomic model it might lead to
> issues.

I think this will be modeset path where impact or latency will be less as compared to
a flip operation.  However, individual drivers can check respective state and skip update.

Regards,
Uma Shankar

> On the other hand it makes colorop handling less error-prone in amdgpu, and
> possibly fixes a bug I've come across where we get confused if an active colorop
> isn't part of the state.
> 
> Harry
> 
> > 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
> >
> > 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  | 5 +++++
> > drivers/gpu/drm/drm_colorop.c | 2 --
> >  2 files changed, 5 insertions(+), 2 deletions(-)
> >

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

[Borah, Chaitanya Kumar]

Thank you Harry for looking into it.

On 2/24/2026 2:44 AM, Harry Wentland wrote:
> On 2026-02-18 01:57, Chaitanya Kumar Borah wrote:
>> 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 tends to reflect my thinking when I wrote the colorop stuff.
> 
>> 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.
>>
> 
> Could this lead drivers to reprogram possibly expensive colorops
> when they didn't change? It won't be an issue for amdgpu since we
> have another level of state tracking, but for drivers that strictly
> follow the atomic model it might lead to issues.
> 

For xe/i915 too, this should be something that we can handle in 
atomic_check().

I guess the real question is whether this violates the atomic design 
contract. As far as I understand, when we pull in affected planes for a 
CRTC, we don’t actually verify whether any plane state has changed.
So by that analogy, should this be acceptable as well?

> On the other hand it makes colorop handling less error-prone in amdgpu,
> and possibly fixes a bug I've come across where we get confused if an
> active colorop isn't part of the state.
> 
> Harry
> 
>> 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
>>
>> 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  | 5 +++++
>>   drivers/gpu/drm/drm_colorop.c | 2 --
>>   2 files changed, 5 insertions(+), 2 deletions(-)
>>
> 

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

[Borah, Chaitanya Kumar]

On 2/26/2026 11:29 AM, Borah, Chaitanya Kumar wrote:
> Thank you Harry for looking into it.
> 
> On 2/24/2026 2:44 AM, Harry Wentland wrote:
>> On 2026-02-18 01:57, Chaitanya Kumar Borah wrote:
>>> 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 tends to reflect my thinking when I wrote the colorop stuff.
>>
>>> 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.
>>>
>>
>> Could this lead drivers to reprogram possibly expensive colorops
>> when they didn't change? It won't be an issue for amdgpu since we
>> have another level of state tracking, but for drivers that strictly
>> follow the atomic model it might lead to issues.
>>
> 
> For xe/i915 too, this should be something that we can handle in 
> atomic_check().
> 
> I guess the real question is whether this violates the atomic design 
> contract. As far as I understand, when we pull in affected planes for a 
> CRTC, we don’t actually verify whether any plane state has changed.
> So by that analogy, should this be acceptable as well?
> 

One downside of adding colorops indiscriminately is lot of logs.

[drm:drm_atomic_get_colorop_state] Added [COLOROP:288:1] 
ffff88810db07240 state to ffff88810e040800

So I created a version where we add only if a color pipeline is selected.

https://lore.kernel.org/intel-gfx/20260310113238.3495981-3-chaitanya.kumar.borah@intel.com/T/#u

Harry, please let me know if this approach still work for you.

It would be good to hear others’ opinions on this as well.

>> On the other hand it makes colorop handling less error-prone in amdgpu,
>> and possibly fixes a bug I've come across where we get confused if an
>> active colorop isn't part of the state.
>>
>> Harry
>>
>>> 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
>>>
>>> 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  | 5 +++++
>>>   drivers/gpu/drm/drm_colorop.c | 2 --
>>>   2 files changed, 5 insertions(+), 2 deletions(-)
>>>
>>
> 

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

[Borah, Chaitanya Kumar]

On 2/24/2026 2:07 AM, Shankar, Uma wrote:
>> Subject: [PATCH 2/2] drm/atomic: Add affected colorops with affected planes
>>
>> 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.
> Even though colorop is an object in itself but practically it doesn't have any existence without
> the plane. So to add to state along with plane seems logical. Also its good to handle this in
> drm core than individual drivers.
> 
> The change looks good to me.
> Reviewed-by: Uma Shankar<uma.shankar@intel.com>

Thank you, Uma, for the review.

I have sent another version of the patch where the colorops are added 
only when pipeline is enabled. This avoids flooding of following logs 
when pipeline is disabled.


  [drm:drm_atomic_get_colorop_state] Added [COLOROP:288:1] 
ffff88810db07240 state to ffff88810e040800

Let me know if this looks good for you.

==
Chaitanya