[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