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 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?

> 
> > > 
> > > 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.

> 
> Then I found igt_atomic_commit() will sync these properties to drm_crtc
> _state and make our HW driver work successfully. So I send the patch to
> ask for review and guidance.
> 
> Or do you know what I may miss in the original igt_disaply_commit()?

IIRC the one thing that is busted in the legacy commit path is
plane properties for primary/cursor planes. Pretty sure I sent
a patch to fix that long ago, but it might have had some kind of
performance problem due to some unintended property value
ping-pong. I suppose I should take another look at it again...

Anyways, that's only about plane properties so should have nothing
to do with the crtc gamma stuff.

> 
> > 
> > > 
> > > Signed-off-by: Jason-JH Lin <jason-jh.lin@mediatek.com>
> > > ---
> > >  tests/kms_color.c | 47 ++++++++++++++++++++++++-------------------
> > > ----
> > >  1 file changed, 24 insertions(+), 23 deletions(-)
> > > 
> > > diff --git a/tests/kms_color.c b/tests/kms_color.c
> > > index c424083b7fd4..0d2baf8f8aad 100644
> > > --- a/tests/kms_color.c
> > > +++ b/tests/kms_color.c
> > > @@ -125,12 +125,12 @@ static bool test_pipe_degamma(data_t *data,
> > >       disable_ctm(primary->pipe);
> > >       disable_gamma(primary->pipe);
> > >       set_degamma(data, primary->pipe, degamma_linear);
> > > -     igt_display_commit(&data->display);
> > > +     igt_display_commit_atomic(display,
> > > DRM_MODE_ATOMIC_ALLOW_MODESET, 0);
> > > 
> > >       /* Draw solid colors with linear degamma transformation. */
> > >       paint_rectangles(data, mode, red_green_blue, &fb);
> > >       igt_plane_set_fb(primary, &fb);
> > > -     igt_display_commit(&data->display);
> > > +     igt_display_commit_atomic(display,
> > > DRM_MODE_ATOMIC_ALLOW_MODESET, 0);
> > >       igt_wait_for_vblank(data->drm_fd,
> > 
> 
> [snip]
> 
> > --
> > Ville Syrjälä
> > Intel
> 
> Regards,
> Jason-JH Lin

-- 
Ville Syrjälä
Intel