Re: [PATCH i-g-t v4 2/2] tests/kms_vrr: add CMRR fixed and video mode subtests

On 8/12/2026 6:37 PM, Naladala Ramanaidu wrote:
> Add test coverage to validate Content Match Refresh Rate (CMRR)
> behavior across both fixed and video timing display modes.
> 
> This introduces a shared helper library to support refresh-rate
> mode selection and parsing,

Does it?

  along with two new test cases covering
> fixed-mode and video-mode scenarios.
> 

Let's call it desktop mode (here and everywhere else). Refresh rates are 
fixed here anyway.

> The new tests measure actual display refresh timing during CMRR
> operation and compare it against the expected target rate to
> verify correctness. Video mode coverage validates behavior across
> standard video timing rates, while fixed mode coverage targets
> display modes that fall outside the standard video timing set.
> 
> Each test cycles through the relevant refresh configurations,
> applying and resetting the appropriate settings between
> measurement passes to ensure consistent and isolated test results.
> 
> v2: Fix test issue.
> v3: Address review comments. (Mitul)
> v4: Fix test issue.
> 
> Signed-off-by: Naladala Ramanaidu <[email protected]>
> ---
>   tests/kms_vrr.c | 204 ++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 204 insertions(+)
> 
> diff --git a/tests/kms_vrr.c b/tests/kms_vrr.c
> index 27f18e8d0..ab6bc66d9 100644
> --- a/tests/kms_vrr.c
> +++ b/tests/kms_vrr.c
> @@ -32,6 +32,7 @@
>   #include "igt_pm.h"
>   #include "igt_psr.h"
>   #include "i915/intel_drrs.h"
> +#include "igt_vrr.h"
>   #include "sw_sync.h"
>   #include <fcntl.h>
>   #include <signal.h>
> @@ -80,6 +81,14 @@
>    *
>    * SUBTEST: lobf-dc3co
>    * Description: Test DC3CO entry during LOBF.
> + *
> + * SUBTEST: cmrr-fixed-mode
> + * Description: Test to set a fixed CMRR target refresh rate and verify it
> + *              is correctly applied.
> + *
> + * SUBTEST: cmrr-video-mode
> + * Description: Test to set standard video timing refresh rates via CMRR
> + *              and verify each target rate is correctly applied.
>    */
>   
>   #define NSECS_PER_SEC (1000000000ull)
> @@ -103,6 +112,8 @@ enum {
>   	TEST_LINK_OFF = 1 << 10,
>   	TEST_NEGATIVE = 1 << 11,
>   	TEST_FORCE_RR = 1 << 12,
> +	TEST_CMRR_FIXED_MODE = 1 << 13,
> +	TEST_CMRR_VIDEO_MODE = 1 << 14,
>   };
>   
>   enum {
> @@ -580,6 +591,184 @@ flip_and_measure(data_t *data, igt_output_t *output,
>   	return 0;
>   }
>   
> +static
> +uint64_t wait_next_vblank_ts_ns(data_t *data, igt_crtc_t *crtc)
> +{
> +	union drm_wait_vblank vbl = {};
> +
> +	vbl.request.type = DRM_VBLANK_RELATIVE | igt_crtc_get_vbl_flag(crtc);
> +	vbl.request.sequence = 1;
> +	do_or_die(igt_ioctl(data->drm_fd, DRM_IOCTL_WAIT_VBLANK, &vbl));
> +
> +	return vbl.reply.tval_sec * NSECS_PER_SEC + vbl.reply.tval_usec * 1000ull;
> +}
> +

Time to create a helper for this. See wait_for_vblank() in kms_async_flips.c

> +static void
> +flip_and_measure_target_rr(data_t *data, igt_crtc_t *crtc,
> +			   double vrefresh, uint32_t cmrr_mode)
> +{
> +	uint32_t i;
> +	bool front = false;
> +	uint64_t last_vblank_ns, vblank_ns;
> +	uint32_t err_frames = 0;
> +	uint64_t frame_times_ns[TARGET_RR_SAMP_COUNT];
> +	uint64_t exp_time_ns;
> +	uint64_t total_frame_time_ns = 0;
> +	uint64_t avg_frame_time_ns;
> +	double  avg_refresh_rate;
> +	double  expected_rr;
> +	uint32_t valid_frames = 0;
> +
> +	exp_time_ns = igt_kms_frame_time_from_vrefresh(vrefresh);
> +
> +	do_flip(data, &data->fb[0]);
> +	(void)get_kernel_event_ns(data, DRM_EVENT_FLIP_COMPLETE);
> +	last_vblank_ns = wait_next_vblank_ts_ns(data, crtc);
> +
> +	for (i = 0; i < TARGET_RR_SAMP_COUNT; i++) {
> +		front = !front;
> +
> +		do_flip(data, front ? &data->fb[1] : &data->fb[0]);
> +		vblank_ns = wait_next_vblank_ts_ns(data, crtc);
> +		(void)get_kernel_event_ns(data, DRM_EVENT_FLIP_COMPLETE);
> +
> +		frame_times_ns[i] = vblank_ns - last_vblank_ns;
> +
> +		if (frame_times_ns[i] > (exp_time_ns + (exp_time_ns / 2))) {
> +			err_frames++;
> +			last_vblank_ns = vblank_ns;
> +			continue;
> +		}
> +
> +		last_vblank_ns = vblank_ns;
> +		total_frame_time_ns += frame_times_ns[i];
> +	}
> +
> +	valid_frames = TARGET_RR_SAMP_COUNT - err_frames;
> +	igt_assert_f(valid_frames > 0,
> +		     "No valid frame samples collected\n");

We should hold valid frames to a higher standard than > 0

> +
> +	avg_frame_time_ns = total_frame_time_ns / valid_frames;
> +	avg_refresh_rate = (double)NSECS_PER_SEC / (double)avg_frame_time_ns;
> +
> +	if (cmrr_mode == CMRR_VIDEO_MODE) {
> +		expected_rr = (double)(vrefresh * CMRR_NUMERATOR) /
> +			      (double)CMRR_VIDEO_MODE_DENOMINATOR;
> +		igt_assert_f(fabs(avg_refresh_rate - expected_rr) <= 0.02,
> +			     "CMRR refresh rate mismatch: "
> +			     "measured avg_rr = %.3f Hz, "
> +			     "expected_rr = %.3f Hz\n",
> +			     avg_refresh_rate, expected_rr);
> +	}
> +
> +	if (cmrr_mode == CMRR_NON_VIDEO_MODE) {
> +		expected_rr = (double)(vrefresh * CMRR_NUMERATOR) /
> +			      (double)CMRR_DENOMINATOR;
> +		igt_assert_f(fabs(avg_refresh_rate - expected_rr) <= 0.02,
> +			     "CMRR refresh rate mismatch: "
> +			     "measured avg_rr = %.3f Hz, "
> +			     "expected_rr = %.3f Hz\n",
> +			     avg_refresh_rate, expected_rr);
> +	}
> +
> +	if (cmrr_mode == CMRR_DISABLE) {
> +		expected_rr = vrefresh;
> +		igt_assert_f(fabs(avg_refresh_rate - expected_rr) <= 0.02,
> +			     "CMRR refresh rate mismatch: "
> +			     "measured avg_rr = %.3f Hz, "
> +			     "expected_rr = %.3f Hz\n",
> +			     avg_refresh_rate, expected_rr);
> +	}
> +
> +	igt_info("Average RR (Hz): %.2f , Expected RR (Hz): %.2f, error frames = %d\n",
> +		 avg_refresh_rate, expected_rr, err_frames);
> +}
> +
> +static
> +void test_cmrr(data_t *data, igt_crtc_t *crtc,
> +	       igt_output_t *output, uint32_t flags)
> +{
> +	uint32_t found;
> +	double rr_from_mode;
> +	drmModeModeInfo mode;
> +	drmModeConnectorPtr connector;
> +	int j;
> +
> +	igt_require_f(cmrr_supported(data->drm_fd, crtc->crtc_index),
> +		      "CMRR not supported\n");
> +	prepare_test(data, output, crtc);
> +	set_vrr_on_crtc(data, crtc, true, false);
> +
> +	if (flags & TEST_CMRR_VIDEO_MODE) {
> +		for (j = 0; j < igt_vrr_standard_video_timing_fps_count; j++) {
> +			found = igt_vrr_get_mode_with_video_timing(output,
> +							igt_vrr_standard_video_timing_fps[j],
> +							&mode);
> +			if (found) {
> +				rr_from_mode = igt_vrr_mode_line_refresh_hz(&mode);
> +				igt_output_override_mode(output, &mode);
> +				igt_info("Override mode:");
> +				kmstest_dump_mode(&mode);
> +				igt_display_commit2(&data->display, COMMIT_ATOMIC);
> +				igt_target_rr_debugfs_write(data->drm_fd,
> +							    crtc->crtc_index,
> +							    mode.vrefresh,
> +							    CMRR_NUMERATOR,
> +							    CMRR_VIDEO_MODE_DENOMINATOR);
> +
> +				flip_and_measure_target_rr(data, crtc,
> +							   mode.vrefresh,
> +							   CMRR_VIDEO_MODE);
> +
> +				igt_target_rr_debugfs_write(data->drm_fd,
> +							    crtc->crtc_index,
> +							    mode.vrefresh,
> +							    0, 0);
> +
> +				flip_and_measure_target_rr(data, crtc,
> +							   rr_from_mode,
> +							   CMRR_DISABLE);
> +			}
> +		}
> +	}
> +
> +	if (flags & TEST_CMRR_FIXED_MODE) {
> +		found = 0;
> +		connector = output->config.connector;
> +		for (j = 0; j < connector->count_modes; j++) {
> +			mode = connector->modes[j];
> +			rr_from_mode = igt_vrr_mode_line_refresh_hz(&mode);
> +
> +			if (rr_from_mode - mode.vrefresh > 0.04) {
> +				found = 1;
> +				igt_output_override_mode(output, &mode);
> +				igt_info("Override mode:");
> +				kmstest_dump_mode(&mode);
> +				igt_display_commit2(&data->display, COMMIT_ATOMIC);
> +				igt_target_rr_debugfs_write(data->drm_fd,
> +							    crtc->crtc_index,
> +							    mode.vrefresh,
> +							    CMRR_NUMERATOR,
> +							    CMRR_DENOMINATOR);
> +
> +				flip_and_measure_target_rr(data, crtc,
> +							   mode.vrefresh,
> +							   CMRR_NON_VIDEO_MODE);
> +
> +				igt_target_rr_debugfs_write(data->drm_fd,
> +							    crtc->crtc_index,
> +							    mode.vrefresh,
> +							    0, 0);
> +
> +				flip_and_measure_target_rr(data, crtc,
> +							   rr_from_mode,
> +							   CMRR_DISABLE);
> +			}
> +		}
> +		igt_require_f(found, "No non-video-timing mode found.\n");

Should this be a skip?

> +	}
> +}
> +
>   /* Basic VRR flip functionality test - enable, measure, disable, measure */
>   static void
>   test_basic(data_t *data, igt_crtc_t *crtc, igt_output_t *output,
> @@ -591,6 +780,7 @@ test_basic(data_t *data, igt_crtc_t *crtc, igt_output_t *output,
>   	uint64_t rate[] = {0};
>   
>   	prepare_test(data, output, crtc);
> +
>   	range = data->range;
>   	vtest_ns = data->vtest_ns;
>   	rate[0] = vtest_ns.rate_ns;
> @@ -1147,6 +1337,20 @@ int igt_main_args("drs:", long_opts, help_str, opt_handler, &data)
>   		}
>   	}
>   
> +	igt_subtest_group() {
> +		igt_fixture()
> +			igt_require_intel(data.drm_fd);
> +

There was some recent discussion regarding intel specific tests in tests 
folder [1].

But this change is just following a pre-existing pattern so this should 
not be a blocker but may be this warrants a broader discussion.

==
Chaitanya

[1] 
https://lore.kernel.org/igt-dev/[email protected]/

> +		igt_describe("Test to validate CMRR in fixed mode.");
> +		igt_subtest_with_dynamic("cmrr-fixed-mode") {
> +			run_vrr_test(&data, test_cmrr, TEST_CMRR_FIXED_MODE);
> +		}
> +
> +		igt_describe("Test to validate CMRR in video mode.");
> +		igt_subtest_with_dynamic("cmrr-video-mode") {
> +			run_vrr_test(&data, test_cmrr, TEST_CMRR_VIDEO_MODE);
> +		}
> +	}
>   	igt_fixture() {
>   		close(data.debugfs_fd);
>   		igt_display_fini(&data.display);