[PATCH i-g-t v1 1/2] tests/kms_vrr: Improve VRR test robustness with cleanup before assertions
Golani, Mitulkumar Ajitkumar
mitulkumar.ajitkumar.golani at intel.com
Thu Jul 31 08:44:02 UTC 2025
> -----Original Message-----
> From: Naladala, Ramanaidu <ramanaidu.naladala at intel.com>
> Sent: 09 July 2025 03:35
> To: igt-dev at lists.freedesktop.org
> Cc: Nautiyal, Ankit K <ankit.k.nautiyal at intel.com>; Golani, Mitulkumar
> Ajitkumar <mitulkumar.ajitkumar.golani at intel.com>; B S, Karthik
> <karthik.b.s at intel.com>; Naladala, Ramanaidu
> <ramanaidu.naladala at intel.com>
> Subject: [PATCH i-g-t v1 1/2] tests/kms_vrr: Improve VRR test robustness with
> cleanup before assertions
>
> The robustness and maintainability of the VRR (Variable Refresh Rate) by
> ensuring proper cleanup is performed before any assertion failure is triggered.
>
> Previously, when a test failed due to not meeting the expected VRR threshold,
> the test environment was not always cleaned up properly.
> This could lead to resource leaks or inconsistent states that might affect
> subsequent tests, making debugging more difficult.
>
> Signed-off-by: Naladala Ramanaidu <ramanaidu.naladala at intel.com>
> ---
> tests/kms_vrr.c | 161 +++++++++++++++++++++++++++++++-----------------
> 1 file changed, 105 insertions(+), 56 deletions(-)
>
> diff --git a/tests/kms_vrr.c b/tests/kms_vrr.c index 9c2c4ac92..a24f413de
> 100644
> --- a/tests/kms_vrr.c
> +++ b/tests/kms_vrr.c
> @@ -241,6 +241,18 @@ low_rr_mode_with_same_res(igt_output_t *output,
> unsigned int vrr_min)
> return mode;
> }
>
> +static void test_cleanup(data_t *data, enum pipe pipe, igt_output_t
> +*output) {
> + igt_pipe_set_prop_value(&data->display, pipe,
> IGT_CRTC_VRR_ENABLED,
> +false);
> +
> + igt_plane_set_fb(data->primary, NULL);
> + igt_output_set_pipe(output, PIPE_NONE);
> + igt_output_override_mode(output, NULL);
> + igt_display_commit2(&data->display, COMMIT_ATOMIC);
> + igt_remove_fb(data->drm_fd, &data->fb[1]);
> + igt_remove_fb(data->drm_fd, &data->fb[0]); }
> +
> static void
> virtual_rr_vrr_range_mode(drmModeModeInfo *mode, float
> virtual_refresh_rate) { @@ -701,25 +713,34 @@ test_basic(data_t *data,
> enum pipe pipe, igt_output_t *output, uint32_t flags)
> if (flags & TEST_FLIPLINE) {
> rate[0] = igt_kms_frame_time_from_vrefresh(range.max + 5);
> result = flip_and_measure(data, output, pipe, rate, 1, data-
> >duration_ns);
> - igt_assert_f(result > 75,
> - "Refresh rate (%u Hz) %"PRIu64"ns: Target VRR on
> threshold not reached, result was %u%%\n",
> - (range.max + 5), rate[0], result);
> + if (result < 75) {
> + test_cleanup(data, pipe, output);
> + igt_assert_f(result > 75,
> + "Refresh rate (%u Hz) %"PRIu64"ns: Target
> VRR on threshold not reached, result was %u%%\n",
> + (range.max + 5), rate[0], result);
> + }
> }
>
> if (flags & ~(TEST_NEGATIVE | TEST_MAXMIN)) {
> rate[0] = vtest_ns.rate_ns;
> result = flip_and_measure(data, output, pipe, rate, 1, data-
> >duration_ns);
> - igt_assert_f(result > 75,
> - "Refresh rate (%u Hz) %"PRIu64"ns: Target VRR on
> threshold not reached, result was %u%%\n",
> - ((range.max + range.min) / 2), rate[0], result);
> + if (result < 75) {
> + test_cleanup(data, pipe, output);
> + igt_assert_f(result > 75,
> + "Refresh rate (%u Hz) %"PRIu64"ns: Target
> VRR on threshold not reached, result was %u%%\n",
> + ((range.max + range.min) / 2), rate[0],
> result);
> + }
> }
>
> if (flags & TEST_FLIPLINE) {
> rate[0] = igt_kms_frame_time_from_vrefresh(range.min - 10);
> result = flip_and_measure(data, output, pipe, rate, 1, data-
> >duration_ns);
> - igt_assert_f(result < 50,
> - "Refresh rate (%u Hz) %"PRIu64"ns: Target VRR on
> threshold exceeded, result was %u%%\n",
> - (range.min - 10), rate[0], result);
> + if (result > 50) {
> + test_cleanup(data, pipe, output);
> + igt_assert_f(result < 50,
> + "Refresh rate (%u Hz) %"PRIu64"ns: Target
> VRR on threshold exceeded, result was %u%%\n",
> + (range.min - 10), rate[0], result);
> + }
> }
>
> if (flags & TEST_MAXMIN) {
> @@ -732,10 +753,12 @@ test_basic(data_t *data, enum pipe pipe,
> igt_output_t *output, uint32_t flags)
> };
>
> result = flip_and_measure(data, output, pipe, maxmin_rates, 2,
> data->duration_ns);
> - igt_assert_f(result > 75,
> - "Refresh rates (%u/%u Hz)
> %"PRIu64"ns/%"PRIu64"ns: Target VRR on threshold not reached, result was
> %u%%\n",
> - range.max, range_min, maxmin_rates[0],
> maxmin_rates[1], result);
> - return;
> + if (result < 75) {
> + test_cleanup(data, pipe, output);
> + igt_assert_f(result > 75,
> + "Refresh rates (%u/%u Hz)
> %"PRIu64"ns/%"PRIu64"ns: Target VRR on threshold not reached, result was
> %u%%\n",
> + range.max, range_min, maxmin_rates[0],
> maxmin_rates[1], result);
> + }
> }
>
> /*
> @@ -746,9 +769,12 @@ test_basic(data_t *data, enum pipe pipe, igt_output_t
> *output, uint32_t flags)
> set_vrr_on_pipe(data, pipe, !(flags & TEST_FASTSET), (flags &
> TEST_NEGATIVE) ? true : false);
> rate[0] = vtest_ns.rate_ns;
> result = flip_and_measure(data, output, pipe, rate, 1, data-
> >duration_ns);
> - igt_assert_f(result < 10,
> - "Refresh rate (%u Hz) %"PRIu64"ns: Target VRR %s threshold
> exceeded, result was %u%%\n",
> - ((range.max + range.min) / 2), rate[0], (flags &
> TEST_NEGATIVE)? "on" : "off", result);
> + if (result > 10) {
> + test_cleanup(data, pipe, output);
> + igt_assert_f(result < 10,
> + "Refresh rate (%u Hz) %"PRIu64"ns: Target VRR %s
> threshold exceeded, result was %u%%\n",
> + ((range.max + range.min) / 2), rate[0], (flags &
> TEST_NEGATIVE) ? "on" : "off", result);
> + }
> }
>
> static void
> @@ -758,6 +784,7 @@ test_seamless_rr_basic(data_t *data, enum pipe pipe,
> igt_output_t *output, uint3
> vtest_ns_t vtest_ns;
> uint64_t rate[] = {0};
> bool vrr = !!(flags & TEST_SEAMLESS_VRR);
> + uint32_t ret;
>
> igt_info("Use HIGH_RR Mode as default (VRR: %s): ", vrr ? "ON" :
> "OFF");
> kmstest_dump_mode(&data->switch_modes[HIGH_RR_MODE]);
> @@ -773,14 +800,21 @@ test_seamless_rr_basic(data_t *data, enum pipe
> pipe, igt_output_t *output, uint3
> * so switch to High clock mode as test preparation
> */
> igt_output_override_mode(output, &data-
> >switch_modes[HIGH_RR_MODE]);
> - igt_assert(igt_display_try_commit_atomic(&data->display,
> DRM_MODE_PAGE_FLIP_EVENT, NULL) == 0);
> + ret = igt_display_try_commit_atomic(&data->display,
> DRM_MODE_PAGE_FLIP_EVENT, NULL);
> + if (ret != 0) {
> + test_cleanup(data, pipe, output);
> + igt_assert(!ret);
> + }
> }
>
> rate[0] = vtest_ns.max;
> result = flip_and_measure(data, output, pipe, rate, 1, data-
> >duration_ns);
> - igt_assert_f(result > 75,
> - "Refresh rate (%u Hz) %"PRIu64"ns: Target VRR %s threshold
> not reached, result was %u%%\n",
> - data->range.max, rate[0], vrr ? "on" : "off", result);
> + if (result < 75) {
> + test_cleanup(data, pipe, output);
> + igt_assert_f(result > 75,
> + "Refresh rate (%u Hz) %"PRIu64"ns: Target VRR %s
> threshold not reached, result was %u%%\n",
> + data->range.max, rate[0], vrr ? "on" : "off", result);
> + }
>
> /* Switch to low rr mode without modeset. */
> igt_info("Switch to LOW_RR Mode (VRR: %s): ", vrr ? "ON" : "OFF"); @@
> -790,22 +824,32 @@ test_seamless_rr_basic(data_t *data, enum pipe pipe,
> igt_output_t *output, uint3
>
> rate[0] = vtest_ns.min;
> result = flip_and_measure(data, output, pipe, rate, 1, data-
> >duration_ns);
> - igt_assert_f(result > 75,
> - "Refresh rate (%u Hz) %"PRIu64"ns: Target VRR %s threshold
> not reached, result was %u%%\n",
> - data->range.min, rate[0], vrr ? "on" : "off", result);
> + if (result < 75) {
> + test_cleanup(data, pipe, output);
> + igt_assert_f(result > 75,
> + "Refresh rate (%u Hz) %"PRIu64"ns: Target VRR %s
> threshold not reached, result was %u%%\n",
> + data->range.min, rate[0], vrr ? "on" : "off", result);
> + }
>
> /* Switch back to high rr mode without modeset. */
> igt_info("Switch back to HIGH_RR Mode (VRR: %s): ", vrr ? "ON" :
> "OFF");
> kmstest_dump_mode(&data->switch_modes[HIGH_RR_MODE]);
> igt_output_override_mode(output, &data-
> >switch_modes[HIGH_RR_MODE]);
> - igt_assert(igt_display_try_commit_atomic(&data->display, 0, NULL) ==
> 0);
> + ret = igt_display_try_commit_atomic(&data->display, 0, NULL);
> + if (ret != 0) {
> + test_cleanup(data, pipe, output);
> + igt_assert(!ret);
> + }
>
> rate[0] = vtest_ns.rate_ns;
> result = flip_and_measure(data, output, pipe, rate, 1, data-
> >duration_ns);
> - igt_assert_f(vrr ? (result > 75) : (result < 10),
> - "Refresh rate (%u Hz) %"PRIu64"ns: Target VRR %s threshold
> %s, result was %u%%\n",
> - ((data->range.max + data->range.min) / 2), rate[0],
> - vrr ? "on" : "off", vrr ? "not reached" : "exceeded", result);
> + if (vrr ? (result < 75) : (result > 10)) {
> + test_cleanup(data, pipe, output);
> + igt_assert_f(vrr ? (result > 75) : (result < 10),
> + "Refresh rate (%u Hz) %"PRIu64"ns: Target VRR %s
> threshold %s, result was %u%%\n",
> + ((data->range.max + data->range.min) / 2), rate[0],
> + vrr ? "on" : "off", vrr ? "not reached" : "exceeded",
> result);
> + }
> }
>
> static void
> @@ -815,6 +859,7 @@ test_seamless_virtual_rr_basic(data_t *data, enum
> pipe pipe, igt_output_t *outpu
> unsigned int vrefresh;
> uint64_t rate[] = {0};
> uint32_t step_size;
> + uint32_t ret;
> drmModeModeInfo virtual_mode;
>
> igt_info("Use HIGH_RR Mode as default\n"); @@ -828,12 +873,19 @@
> test_seamless_virtual_rr_basic(data_t *data, enum pipe pipe, igt_output_t
> *outpu
> * switch to highest refresh rate mode.
> */
> igt_output_override_mode(output, &data-
> >switch_modes[HIGH_RR_MODE]);
> - igt_assert(igt_display_try_commit_atomic(&data->display,
> DRM_MODE_PAGE_FLIP_EVENT, NULL) == 0);
> + ret = igt_display_try_commit_atomic(&data->display,
> DRM_MODE_PAGE_FLIP_EVENT, NULL);
> + if (ret != 0) {
> + test_cleanup(data, pipe, output);
> + igt_assert(!ret);
> + }
>
> result = flip_and_measure(data, output, pipe, rate, 1,
> TEST_DURATION_NS);
> - igt_assert_f(result > 75,
> - "Refresh rate (%u Hz) %"PRIu64"ns: Target threshold not
> reached, result was %u%%\n",
> - data->switch_modes[HIGH_RR_MODE].vrefresh, rate[0],
> result);
> + if (result < 75) {
> + test_cleanup(data, pipe, output);
> + igt_assert_f(result > 75,
> + "Refresh rate (%u Hz) %"PRIu64"ns: Target threshold
> not reached, result was %u%%\n",
> + data->switch_modes[HIGH_RR_MODE].vrefresh,
> rate[0], result);
> + }
>
> /*
> * Calculate step size by considering the no. of steps required to @@ -
> 851,13 +903,21 @@ test_seamless_virtual_rr_basic(data_t *data, enum pipe
> pipe, igt_output_t *outpu
> kmstest_dump_mode(&virtual_mode);
>
> igt_output_override_mode(output, &virtual_mode);
> - igt_assert(igt_display_try_commit_atomic(&data->display, 0,
> NULL) == 0);
> +
> + ret = igt_display_try_commit_atomic(&data->display, 0, NULL);
> + if (ret != 0) {
> + test_cleanup(data, pipe, output);
> + igt_assert(!ret);
> + }
>
> rate[0] = igt_kms_frame_time_from_vrefresh(vrefresh);
> result = flip_and_measure(data, output, pipe, rate, 1,
> TEST_DURATION_NS);
> - igt_assert_f(result > 75,
> - "Refresh rate (%u Hz) %"PRIu64"ns: Target threshold
> not reached, result was %u%%\n",
> - vrefresh, rate[0], result);
> + if (result < 75) {
> + test_cleanup(data, pipe, output);
> + igt_assert_f(result > 75,
> + "Refresh rate (%u Hz) %"PRIu64"ns: Target
> threshold not reached, result was %u%%\n",
> + vrefresh, rate[0], result);
> + }
> }
> }
>
> @@ -924,26 +984,16 @@ test_cmrr(data_t *data, enum pipe pipe,
> igt_output_t *output, uint32_t flags)
> if (!igt_display_try_commit2(&data->display, COMMIT_ATOMIC)) {
> prepare_test(data, output, pipe);
> result = flip_and_measure_cmrr(data, output, pipe,
> TEST_DURATION_NS * 2);
> - igt_assert_f(result > 75,
> - "Refresh rate (%u Hz) %"PRIu64"ns: Target CMRR on
> threshold not reached, result was %u%%\n",
> - mode.vrefresh,
> igt_kms_frame_time_from_vrefresh(mode.vrefresh),
> - result);
> + if (result < 75) {
> + test_cleanup(data, pipe, output);
> + igt_assert_f(result > 75,
> + "Refresh rate (%u Hz) %"PRIu64"ns: Target
> CMRR on threshold not reached, result was %u%%\n",
> + mode.vrefresh,
> igt_kms_frame_time_from_vrefresh(mode.vrefresh),
> + result);
> + }
> }
> }
>
> -static void test_cleanup(data_t *data, enum pipe pipe, igt_output_t *output) -{
> - igt_pipe_set_prop_value(&data->display, pipe,
> IGT_CRTC_VRR_ENABLED, false);
> -
> - igt_plane_set_fb(data->primary, NULL);
> - igt_output_set_pipe(output, PIPE_NONE);
> - igt_output_override_mode(output, NULL);
> - igt_display_commit2(&data->display, COMMIT_ATOMIC);
> -
> - igt_remove_fb(data->drm_fd, &data->fb[1]);
> - igt_remove_fb(data->drm_fd, &data->fb[0]);
> -}
> -
> static bool output_constraint(data_t *data, igt_output_t *output, uint32_t flags)
> {
> data->debugfs_fd = igt_debugfs_dir(data->drm_fd); @@ -1060,10
> +1110,9 @@ run_vrr_test(data_t *data, test_t test, uint32_t flags)
> kmstest_pipe_name(pipe), output->name)
> test(data, pipe, output, flags);
>
> - test_cleanup(data, pipe, output);
> -
> - break;
> }
> + igt_output_set_pipe(output, PIPE_NONE);
> + igt_output_override_mode(output, NULL);
> }
> }
Please break your changes to separate modules as a step by step incremental changes, adding all changes to a single patch makes it difficult to read and review.
Also add a comment why you need separate cleanups when it was already present before.
Regards,
Mitul
>
> --
> 2.43.0
More information about the igt-dev
mailing list