[PATCH i-g-t v1 1/2] tests/kms_vrr: Improve VRR test robustness with cleanup before assertions
Sharma, Swati2
swati2.sharma at intel.com
Mon Jul 21 12:46:42 UTC 2025
Hi Ramanaidu,
On 09-07-2025 03:35 am, Naladala Ramanaidu wrote:
> 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);
Add a new line here.
> + 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);
Instead of igt_assert_f() we can use igt_fail() it will avoid the
confusing |assert_f(result > 75)| right after explicitly checking
|result < 75).|
> + }
> }
>
> 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);
> + }
Same here.
> }
>
> 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);
Same here.
> + }
> }
>
> 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);
Same here.
> + }
> }
>
> /*
> @@ -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);
Same here.
> + }
> }
>
> 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);
Same here.
> + }
> }
>
> 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);
Same here.
> + }
>
> /* 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);
Same here.
> + }
>
> 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);
Same here.
> + }
> }
>
> 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);
Same here.
> + }
>
> /*
> * 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);
Same here.
> + }
>
> 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);
Same here.
> + }
> }
> }
>
> @@ -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);
Same here.
> + }
> }
> }
>
> -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;
Why are we removing break? Do we want to run test on each pipe?
> }
> + igt_output_set_pipe(output, PIPE_NONE);
> + igt_output_override_mode(output, NULL);
Is this redundant?
> }
> }
>
More information about the igt-dev
mailing list