From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Jason-JH Lin <jason-jh.lin@mediatek.com>
Cc: igt-dev@lists.freedesktop.org,
Karthik B S <karthik.b.s@intel.com>,
Swati Sharma <swati2.sharma@intel.com>,
Uma <uma.shankar@intel.com>,
Chaitanya Kumar <chaitanya.kumar.borah@intel.com>,
Kamil Konieczny <kamil.konieczny@linux.intel.com>,
Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>,
Bhanuprakash Modem <bhanuprakash.modem@gmail.com>,
Fei Shao <fshao@chromium.org>, Jani <jani.nikula@intel.com>,
Paul-PL Chen <paul-pl.chen@mediatek.com>,
Nancy Lin <nancy.lin@mediatek.com>,
Singo Chang <singo.chang@mediatek.com>,
Gil Dekel <gildekel@google.com>, Yacoub <markyacoub@chromium.org>,
Project_Global_Chrome_Upstream_Group@mediatek.com
Subject: Re: [PATCH i-g-t v2] tests/kms_color: Always use atomic_commit for setting color properties
Date: Fri, 21 Nov 2025 10:56:22 +0200 [thread overview]
Message-ID: <aSApNt1PwpgrX5E9@intel.com> (raw)
In-Reply-To: <20251121075406.2006325-1-jason-jh.lin@mediatek.com>
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.
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.
>
> 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.
>
> 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,
> display->pipes[primary->pipe->pipe].crtc_offset);
Hmm, I wonder if we could nuke these extra vblank waits. I suppose
there might be a slight issue with the prefill happening before the
new LUT is loaded (on the platforms with a single buffered LUT).
Someone should perhaps spend a few brain cells on this and at least
test how much the test could be sped up by eliminating the extra
vblank waits...
> igt_pipe_crc_collect_crc(data->pipe_crc, &crc_fullcolors);
> @@ -142,7 +142,7 @@ static bool test_pipe_degamma(data_t *data,
> paint_gradient_rectangles(data, mode, red_green_blue, &fb);
> igt_plane_set_fb(primary, &fb);
> set_degamma(data, primary->pipe, degamma_full);
> - igt_display_commit(&data->display);
> + igt_display_commit_atomic(display, DRM_MODE_ATOMIC_ALLOW_MODESET, 0);
> igt_wait_for_vblank(data->drm_fd,
> display->pipes[primary->pipe->pipe].crtc_offset);
> igt_pipe_crc_collect_crc(data->pipe_crc, &crc_fullgamma);
> @@ -156,7 +156,7 @@ static bool test_pipe_degamma(data_t *data,
> disable_degamma(primary->pipe);
> igt_plane_set_fb(primary, NULL);
> igt_output_set_pipe(output, PIPE_NONE);
> - igt_display_commit(&data->display);
> + igt_display_commit_atomic(display, DRM_MODE_ATOMIC_ALLOW_MODESET, 0);
> igt_remove_fb(data->drm_fd, &fb);
> igt_remove_fb(data->drm_fd, &fb_modeset);
>
> @@ -215,12 +215,12 @@ static bool test_pipe_gamma(data_t *data,
> disable_ctm(primary->pipe);
> disable_degamma(primary->pipe);
> set_gamma(data, primary->pipe, gamma_full);
> - igt_display_commit(&data->display);
> + igt_display_commit_atomic(display, DRM_MODE_ATOMIC_ALLOW_MODESET, 0);
>
> /* Draw solid colors with no gamma 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,
> display->pipes[primary->pipe->pipe].crtc_offset);
> igt_pipe_crc_collect_crc(data->pipe_crc, &crc_fullcolors);
> @@ -231,7 +231,7 @@ static bool test_pipe_gamma(data_t *data,
> */
> paint_gradient_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,
> display->pipes[primary->pipe->pipe].crtc_offset);
> igt_pipe_crc_collect_crc(data->pipe_crc, &crc_fullgamma);
> @@ -245,7 +245,7 @@ static bool test_pipe_gamma(data_t *data,
> disable_gamma(primary->pipe);
> igt_plane_set_fb(primary, NULL);
> igt_output_set_pipe(output, PIPE_NONE);
> - igt_display_commit(&data->display);
> + igt_display_commit_atomic(display, DRM_MODE_ATOMIC_ALLOW_MODESET, 0);
> igt_remove_fb(data->drm_fd, &fb);
> igt_remove_fb(data->drm_fd, &fb_modeset);
>
> @@ -312,12 +312,12 @@ static bool test_pipe_legacy_gamma(data_t *data,
> disable_degamma(primary->pipe);
> disable_gamma(primary->pipe);
> disable_ctm(primary->pipe);
> - igt_display_commit(&data->display);
> + igt_display_commit_atomic(display, DRM_MODE_ATOMIC_ALLOW_MODESET, 0);
>
> /* Draw solid colors with no gamma 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,
> display->pipes[primary->pipe->pipe].crtc_offset);
> igt_pipe_crc_collect_crc(data->pipe_crc, &crc_fullcolors);
> @@ -334,7 +334,7 @@ static bool test_pipe_legacy_gamma(data_t *data,
> red_lut[i] = green_lut[i] = blue_lut[i] = 0xffff;
> igt_assert_eq(drmModeCrtcSetGamma(data->drm_fd, primary->pipe->crtc_id,
> legacy_lut_size, red_lut, green_lut, blue_lut), 0);
> - igt_display_commit(&data->display);
> + igt_display_commit_atomic(display, DRM_MODE_ATOMIC_ALLOW_MODESET, 0);
> igt_wait_for_vblank(data->drm_fd,
> display->pipes[primary->pipe->pipe].crtc_offset);
> igt_pipe_crc_collect_crc(data->pipe_crc, &crc_fullgamma);
> @@ -351,11 +351,11 @@ static bool test_pipe_legacy_gamma(data_t *data,
>
> igt_assert_eq(drmModeCrtcSetGamma(data->drm_fd, primary->pipe->crtc_id,
> legacy_lut_size, red_lut, green_lut, blue_lut), 0);
> - igt_display_commit(&data->display);
> + igt_display_commit_atomic(display, DRM_MODE_ATOMIC_ALLOW_MODESET, 0);
>
> igt_plane_set_fb(primary, NULL);
> igt_output_set_pipe(output, PIPE_NONE);
> - igt_display_commit(&data->display);
> + igt_display_commit_atomic(display, DRM_MODE_ATOMIC_ALLOW_MODESET, 0);
> igt_remove_fb(data->drm_fd, &fb);
> igt_remove_fb(data->drm_fd, &fb_modeset);
>
> @@ -384,6 +384,7 @@ static bool test_pipe_legacy_gamma_reset(data_t *data,
> uint16_t *red_lut, *green_lut, *blue_lut;
> struct drm_color_lut *lut;
> drmModePropertyBlobPtr blob;
> + igt_display_t *display = &data->display;
> igt_output_t *output = data->output;
> bool ret = true;
>
> @@ -399,7 +400,7 @@ static bool test_pipe_legacy_gamma_reset(data_t *data,
> disable_degamma(primary->pipe);
> disable_ctm(primary->pipe);
> disable_gamma(primary->pipe);
> - igt_display_commit(&data->display);
> + igt_display_commit_atomic(display, DRM_MODE_ATOMIC_ALLOW_MODESET, 0);
>
> /*
> * Set a degama & gamma LUT and a CTM using the
> @@ -411,7 +412,7 @@ static bool test_pipe_legacy_gamma_reset(data_t *data,
> if (igt_pipe_obj_has_prop(primary->pipe, IGT_CRTC_CTM))
> set_ctm(primary->pipe, ctm_identity);
> set_gamma(data, primary->pipe, gamma_zero);
> - igt_display_commit(&data->display);
> + igt_display_commit_atomic(display, DRM_MODE_ATOMIC_ALLOW_MODESET, 0);
>
> if (igt_pipe_obj_has_prop(primary->pipe, IGT_CRTC_DEGAMMA_LUT)) {
> blob = get_blob(data, primary->pipe, IGT_CRTC_DEGAMMA_LUT);
> @@ -466,7 +467,7 @@ static bool test_pipe_legacy_gamma_reset(data_t *data,
> primary->pipe->crtc_id,
> legacy_lut_size,
> red_lut, green_lut, blue_lut), 0);
> - igt_display_commit(&data->display);
> + igt_display_commit_atomic(display, DRM_MODE_ATOMIC_ALLOW_MODESET, 0);
>
> if (igt_pipe_obj_has_prop(primary->pipe, IGT_CRTC_DEGAMMA_LUT))
> igt_assert(get_blob(data, primary->pipe,
> @@ -493,7 +494,7 @@ static bool test_pipe_legacy_gamma_reset(data_t *data,
> end:
> igt_plane_set_fb(primary, NULL);
> igt_output_set_pipe(output, PIPE_NONE);
> - igt_display_commit(&data->display);
> + igt_display_commit_atomic(display, DRM_MODE_ATOMIC_ALLOW_MODESET, 0);
>
> free_lut(degamma_linear);
> free_lut(gamma_zero);
> @@ -572,12 +573,12 @@ static bool test_pipe_ctm(data_t *data,
> igt_debug("color after[2] %f,%f,%f\n", after[2].r, after[2].g, after[2].b);
>
> disable_ctm(primary->pipe);
> - igt_display_commit(&data->display);
> + igt_display_commit_atomic(display, DRM_MODE_ATOMIC_ALLOW_MODESET, 0);
>
> paint_rectangles(data, mode, after, &fb);
> igt_plane_set_fb(primary, &fb);
> set_ctm(primary->pipe, ctm_identity);
> - igt_display_commit(&data->display);
> + igt_display_commit_atomic(display, DRM_MODE_ATOMIC_ALLOW_MODESET, 0);
> igt_wait_for_vblank(data->drm_fd,
> display->pipes[primary->pipe->pipe].crtc_offset);
> igt_pipe_crc_collect_crc(data->pipe_crc, &crc_software);
> @@ -586,7 +587,7 @@ static bool test_pipe_ctm(data_t *data,
> paint_rectangles(data, mode, before, &fb);
> igt_plane_set_fb(primary, &fb);
> set_ctm(primary->pipe, ctm_matrix);
> - igt_display_commit(&data->display);
> + igt_display_commit_atomic(display, DRM_MODE_ATOMIC_ALLOW_MODESET, 0);
> igt_wait_for_vblank(data->drm_fd,
> display->pipes[primary->pipe->pipe].crtc_offset);
> igt_pipe_crc_collect_crc(data->pipe_crc, &crc_hardware);
> @@ -600,7 +601,7 @@ static bool test_pipe_ctm(data_t *data,
> disable_ctm(primary->pipe);
> igt_plane_set_fb(primary, NULL);
> igt_output_set_pipe(output, PIPE_NONE);
> - igt_display_commit(&data->display);
> + igt_display_commit_atomic(display, DRM_MODE_ATOMIC_ALLOW_MODESET, 0);
> igt_remove_fb(data->drm_fd, &fb);
> igt_remove_fb(data->drm_fd, &fb_modeset);
>
> @@ -689,7 +690,7 @@ static void test_pipe_limited_range_ctm(data_t *data,
> igt_output_set_prop_value(output, IGT_CONNECTOR_BROADCAST_RGB, BROADCAST_RGB_FULL);
> paint_rectangles(data, mode, red_green_blue_limited, &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,
> display->pipes[primary->pipe->pipe].crtc_offset);
> igt_pipe_crc_collect_crc(data->pipe_crc, &crc_full);
> @@ -698,7 +699,7 @@ static void test_pipe_limited_range_ctm(data_t *data,
> igt_output_set_prop_value(output, IGT_CONNECTOR_BROADCAST_RGB, BROADCAST_RGB_16_235);
> paint_rectangles(data, mode, red_green_blue_full, &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,
> display->pipes[primary->pipe->pipe].crtc_offset);
> igt_pipe_crc_collect_crc(data->pipe_crc, &crc_limited);
> --
> 2.43.0
--
Ville Syrjälä
Intel
next prev parent reply other threads:[~2025-11-21 8:56 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-11-21 7:53 [PATCH i-g-t v2] tests/kms_color: Always use atomic_commit for setting color properties Jason-JH Lin
2025-11-21 8:56 ` Ville Syrjälä [this message]
2025-11-21 9:47 ` Jason-JH Lin (林睿祥)
2025-11-21 10:38 ` Ville Syrjälä
2025-11-24 6:47 ` Jason-JH Lin (林睿祥)
2025-11-24 21:00 ` Ville Syrjälä
2025-11-25 2:19 ` Jason-JH Lin (林睿祥)
2025-11-25 17:06 ` Ville Syrjälä
2025-11-26 14:40 ` Jason-JH Lin (林睿祥)
2026-02-11 11:24 ` Jason-JH Lin (林睿祥)
2026-02-11 13:31 ` Ville Syrjälä
2026-02-12 10:15 ` Jason-JH Lin (林睿祥)
2026-02-12 11:57 ` Ville Syrjälä
2026-02-12 15:02 ` Jason-JH Lin (林睿祥)
2025-11-25 2:50 ` ✓ Xe.CI.BAT: success for " Patchwork
2025-11-25 3:03 ` ✓ i915.CI.BAT: " Patchwork
2025-11-25 4:43 ` ✗ Xe.CI.Full: failure " Patchwork
2025-11-25 8:52 ` ✗ i915.CI.Full: " Patchwork
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=aSApNt1PwpgrX5E9@intel.com \
--to=ville.syrjala@linux.intel.com \
--cc=Project_Global_Chrome_Upstream_Group@mediatek.com \
--cc=bhanuprakash.modem@gmail.com \
--cc=chaitanya.kumar.borah@intel.com \
--cc=fshao@chromium.org \
--cc=gildekel@google.com \
--cc=igt-dev@lists.freedesktop.org \
--cc=jani.nikula@intel.com \
--cc=jason-jh.lin@mediatek.com \
--cc=juhapekka.heikkila@gmail.com \
--cc=kamil.konieczny@linux.intel.com \
--cc=karthik.b.s@intel.com \
--cc=markyacoub@chromium.org \
--cc=nancy.lin@mediatek.com \
--cc=paul-pl.chen@mediatek.com \
--cc=singo.chang@mediatek.com \
--cc=swati2.sharma@intel.com \
--cc=uma.shankar@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.