Would you like me to pull these into drm-misc-next? I'd love the changes as base for me next patchset.
Harry On 2026-03-10 07:32, 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 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@[email protected] > > v2: > - Add affected colorops only when a pipeline is enabled > > 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 | 7 +++++++ > drivers/gpu/drm/drm_colorop.c | 2 -- > 2 files changed, 7 insertions(+), 2 deletions(-) >
