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

[Harry Wentland <harry.wentland@amd.com>]

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@kms_color_pipeline@plane-lut1d.html
> 
> v2:
>   - Add affected colorops only when a pipeline is enabled
> 
> 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  | 7 +++++++
>  drivers/gpu/drm/drm_colorop.c | 2 --
>  2 files changed, 7 insertions(+), 2 deletions(-)
>