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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox