[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