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

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

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