> -----Original Message----- > From: Harry Wentland <[email protected]> > Sent: Tuesday, February 24, 2026 2:44 AM > To: Borah, Chaitanya Kumar <[email protected]>; dri- > [email protected]; [email protected]; intel- > [email protected] > Cc: [email protected]; [email protected]; [email protected]; > [email protected]; [email protected]; Shankar, Uma > <[email protected]>; [email protected]; > [email protected]; Nikula, Jani <[email protected]>; > [email protected]; [email protected] > 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 > > @[email protected] > > > > Cc: Simon Ser <[email protected]> > > Cc: Alex Hung <[email protected]> > > Cc: Harry Wentland <[email protected]> > > Cc: Daniel Stone <[email protected]> > > Cc: Melissa Wen <[email protected]> > > Cc: Sebastian Wick <[email protected]> > > Cc: Alex Hung <[email protected]> > > Cc: Uma Shankar <[email protected]> > > Cc: Ville Syrjälä <[email protected]> > > Cc: Maarten Lankhorst <[email protected]> > > Cc: Jani Nikula <[email protected]> > > Cc: Louis Chauvet <[email protected]> > > Cc: <[email protected]> #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(-) > >
