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

["Borah, Chaitanya Kumar" <chaitanya.kumar.borah@intel.com>]

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