Re: [PATCH i-g-t v2] tests/kms_color: Always use atomic_commit for setting color properties

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Nov 24, 2025 at 06:47:27AM +0000, Jason-JH Lin (林睿祥) wrote:
> On Fri, 2025-11-21 at 12:38 +0200, Ville Syrjälä wrote:
> > 
> > External email : Please do not click links or open attachments until
> > you have verified the sender or the content.
> > 
> > 
> > On Fri, Nov 21, 2025 at 09:47:52AM +0000, Jason-JH Lin (林睿祥) wrote:
> > > Hi Ville,
> > > 
> > > Thanks for your rapid reply.
> > > 
> > > On Fri, 2025-11-21 at 10:56 +0200, Ville Syrjälä wrote:
> > > > 
> > > > External email : Please do not click links or open attachments
> > > > until
> > > > you have verified the sender or the content.
> > > > 
> > > > 
> > > > On Fri, Nov 21, 2025 at 03:53:54PM +0800, Jason-JH Lin wrote:
> > > > > When a driver makes use of gamma_lut, degamma_lut, or ctm
> > > > > properties
> > > > > from crtc state, these properties are applied only via atomic
> > > > > commit.
> > > > > This change ensures all relevant properties are properly
> > > > > synchronized
> > > > > to crtc state during testing.
> > > > > 
> > > > > All upstream drivers that support color management properties
> > > > > also
> > > > > support atomic commits, so legacy commit handling in kms_color
> > > > > is
> > > > > no
> > > > > longer needed.
> > > > 
> > > > Incorrect.
> > > 
> > > The conclusion is refer from:
> > > https://patchwork.freedesktop.org/patch/254752/?series=50539&rev=1
> > > 
> > > I'll remove it because I'm not sure for this.
> > > 
> > > > 
> > > > But rather than spreading the current mess even further, I think
> > > > what we want is a igt_display_commit() variant that takes care of
> > > > the is_atomic check. So add that, blast it over all the existing
> > > > places that do the check by hand (with cocci/sed/etc), and
> > > > and then look at using it for new stuff like this where it
> > > > probably makes sense to use atomic when available.
> > > > 
> > > 
> > > Sorry for missing you in the cc loop.
> > > Does my previous version of this patch satisfy this?
> > > https://patchwork.freedesktop.org/patch/687863
> > 
> > I meant that you should add some kind of
> >  igt_display_commit_something()
> >  {
> >         igt_display_commit2(display, display->is_atomic ?
> > COMMIT_ATOMIC : COMMIT_LEGACY);
> >  }
> > wrapper thingy, and deploy it everwhere that currently
> > has that is_atomic check open coded. I guess the real problem
> > is coming up with a name... igt_display_commit_auto() maybe?
> > 
> 
> Got it, I'll use a wrapper API "igt_disaply_commit_auto()" in the next
> version.
> 
> > > 
> > > > > 
> > > > > Update the test logic to always use igt_display_commit_atomic
> > > > > with
> > > > > DRM_MODE_ATOMIC_ALLOW_MODESET for all color management
> > > > > operations.
> > > > > This simplifies code and guarantees correct validation of
> > > > > gamma,
> > > > > degamma, and ctm properties.
> > > > 
> > > > What do you mean with "guarantees correct validation"? The
> > > > current
> > > > code works just fine AFAIK.
> > > 
> > > I have encountered that GAMM_LUT drm property is not updated into
> > > the
> > > gamma_lut struct in drm_crtc_state via igt_disaply_commit().
> > > 
> > > I found igt_disaply_commit() will finally call
> > > drmModeObjectSetProperty() and igt_plane_commit() in
> > > igt_pipe_commit().
> > > These functions will set GAMM_LUT property to drm driver but they
> > > won't
> > > update the gamma_lut struct in crtc state. So our GAMMA driver
> > > won't do
> > > anything when running these IGT KmsColorTest.
> > 
> > Whether the commit comes via that atomic ioctl or the setproperty
> > ioctl
> > the same exact code gets used to update the blobs in the crtc state,
> > and then the driver's atomic hooks get called in exactly the same
> > way.
> > 
> > But with the non-atomic igt commit you obviously will see multiple
> > commits coming into the kernel, and you won't see the full state in
> > the kernel until all the commits are done.
> > 
> > So if you're saying that the crtc state gamma blob didn't have the
> > expected new stuff in it, then I think you were either looking at
> > the state before all the commits had arrived, or you have some kind
> > of driver bug where the state gets corrupted.
> > 
> 
> Looking into igt_pipe_commit(), it used drmModeObjectSetProperty() to
> set property to DRM and it used igt_plane_commit() to synchronize all
> plane states and only primary plane will call drmModeSetCrtc().
> 
> It think the reason we didn't get the new gamma_lut in crtc state is
> taht set_gamma() will only call igt_pipe_obj_prop_changed() to update
> pipe_obj->changed flag, but not update plane->changed flag.
> In igt_primary_plane_commit_legacy(), it'll return if primary->changed
> flag is not set.

The LUT properties are supposed to be on the CRTC. So the plane
stuff should not matter, unless you driver is doing something
completely non-stanadard.

-- 
Ville Syrjälä
Intel