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 Tue, Nov 25, 2025 at 02:19:54AM +0000, Jason-JH Lin (林睿祥) wrote:
> On Mon, 2025-11-24 at 23:00 +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 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.
> > 
> 
> You're right. Plane stuff should not matter to CRTC.
> 
> So I means drmModeSetCrtc() won't be called if plane properties is not
> set in igt_primary_plane_commit_legacy() currently.
> 
> I think this legacy flow logic:
> {
> 
> if (!igt_plane_is_prop_changed(primary, IGT_PLANE_FB_ID)) &&
>     !(primary->changed & IGT_PLANE_COORD_CHANGED_MASK) &&
>     !igt_pipe_obj_is_prop_changed(primary->pipe, IGT_CRTC_MODE_ID))
>         return 0;
> ...
> 
> drmModeSetCrtc()
> 
> ...
> 
> }
> may block LUT properties being synchronized into crtc state when we
> only called set_gamma() and not updated any plane properties.

That doesn't affect CRTC properties apart from the mode.
For all other CRTC properties drmModeObjectSetProperty()
will be called regardless by igt_pipe_commit().

> 
> But I'm not sure the legacy flow will update the primary plane
> properties when CRTC properties is changed in some where.
> 
> So I won't modify this legacy flow logic, but will only add a wrapper
> API that you mentioned to fix the atomic flow of our drivers.
> 
> > --
> > Ville Syrjälä
> > Intel
> 
> Regards,
> Jason-JH Lin
> 

-- 
Ville Syrjälä
Intel