[PATCH v4 1/2] tests/kms_vrr: Bucketize refresh rate tolerance

[Nautiyal, Ankit K]

On 4/17/2025 2:45 PM, Mitul Golani wrote:
> Reduce false failures while preserving timing accuracy. Introduce
> a small tolerance buffer based on refresh rate which accounts for
> HW/SW latency without compromising validation on HRR panel.
> Although an imperical number but already IGT is living with that.

s/imperical/emperical


> This also ensures that asked refresh rate is not too off and always
> catch the real HW/software issues.
>
> --v2:
> Refresh rate criteria changes.
>
> --v3:
> Comment changes (Uma).
>
> --v4:
> Use pre-table for refresh rate checks (Ankit).
> Commit message changes (Ankit).
>
> Signed-off-by: Mitul Golani <mitulkumar.ajitkumar.golani at intel.com>
> ---
>   tests/kms_vrr.c | 82 +++++++++++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 79 insertions(+), 3 deletions(-)
>
> diff --git a/tests/kms_vrr.c b/tests/kms_vrr.c
> index c4bb30f6a..c9059c897 100644
> --- a/tests/kms_vrr.c
> +++ b/tests/kms_vrr.c
> @@ -124,6 +124,24 @@ typedef struct vtest_ns {
>   	uint64_t max;
>   } vtest_ns_t;
>   
> +typedef struct {
> +	int min_rate;	/* Min refresh rate in Hz */
> +	int max_rate;	/* Max refresh rate in Hz */
> +	int tolerance;	/* Tolerance in Hz */
> +} tolerance_config;
> +
> +/*
> + * Tolerance values are determined based on predefined ranges
> + * ≤ 120 Hz: ±1
> + * 121-240 Hz: ±10
> + * > 240 Hz: ±20

Just a suggestion: use <= instead of ≤ and +/- instead of ±.

Some machines might not have encoding for to show these characters.

> + */
> +tolerance_config tolerance_table[] = {
> +	{0, 120, 1},
> +	{121, 240, 10},
> +	{241, INT_MAX, 20}
> +};
> +
>   typedef struct data {
>   	igt_display_t display;
>   	int drm_fd;
> @@ -410,6 +428,57 @@ do_flip(data_t *data, igt_fb_t *fb)
>   	igt_reset_timeout();
>   }
>   
> +static void
> +calculate_tolerance(uint64_t *threshold_hi, uint64_t *threshold_lo, uint64_t rates_ns)
> +{
> +	uint32_t refresh_rate = NSECS_PER_SEC / rates_ns;
> +
> +	if (refresh_rate < 0)
> +		return;
> +
> +	/*
> +	 * Current IGT implementation follows this sequence:
> +	 * 1. Perform a page flip (`do_flip`).
> +	 * 2. Wait for the flip completion event.
> +	 * 3. Compare the timestamp of the flip completion event with the previous frame’s
> +	 *    completion timestamp.
> +	 * 4. Adjust CPU cycle burning based on the relative frame time.
> +	 * 5. Tolerance Check: Determine if the flip completion time falls within
> +	 *    the acceptable range.
> +	 *
> +	 * We set a tolerance value as the acceptable range of time within which a
> +	 * flip completion event should occur. If a flip completes too early or too
> +	 * late, it is marked as out of tolerance.
> +	 * As a result, additional CPU cycles are burned to match the `target_ns`.
> +	 * Even if the next frame is on time, the total frame time now includes:
> +	 * Burned CPU cycle time (from the previous frame) + Flip completion event time.
> +	 * This leads to miscalculation, causing **false out-of-range detections**.
> +	 * The impact is more significant on High Refresh Rate (HRR) panels, where:
> +	 * The allowed tolerance window is smaller and more correction time is required.
> +	 * i.e. for 210hz (4.762ms), allowed range is 209hz(4.784ms) to 211hz(4.739ms).
> +	 * This comes just 23 microsecond tolerance, which is much lesser
> +	 * for accounting HW/SW latency, CPU burn cycle latency and correction logic
> +	 * applied in igt for validation.
> +	 *
> +	 * To address this implement a Bucketing Strategy:
> +	 * Provide a small tolerance buffer to allow IGT tests to account for correction.
> +	 * Based on range of asked refresh rate. This prevents excessive failures due to minor
> +	 * timing adjustments.
> +	 */
> +

Can remove the extra line here.


> +	for(int i = 0; i < ARRAY_SIZE(tolerance_table); i++) {
> +		if (refresh_rate >= tolerance_table[i].min_rate &&
> +				refresh_rate <= tolerance_table[i].max_rate) {
> +			*threshold_hi =
> +				NSECS_PER_SEC / (((float)NSECS_PER_SEC / rates_ns) + tolerance_table[i].tolerance);
> +			*threshold_lo =
> +				NSECS_PER_SEC / (((float)NSECS_PER_SEC / rates_ns) - tolerance_table[i].tolerance);
> +
> +			return;
> +		}
> +	}
> +}
> +
>   /*
>    * Flips at the given rate and measures against the expected value.
>    * Returns the pass rate as a percentage from 0 - 100.
> @@ -439,9 +508,7 @@ flip_and_measure(data_t *data, igt_output_t *output, enum pipe pipe,
>   		else
>   			exp_rate_ns = vtest_ns.max;
>   
> -		/* Allow ~1 Hz deviation for different reasons causing delay. */
> -		threshold_hi[i] = NSECS_PER_SEC / (((float)NSECS_PER_SEC / exp_rate_ns) + 1);
> -		threshold_lo[i] = NSECS_PER_SEC / (((float)NSECS_PER_SEC / exp_rate_ns) - 1);
> +		calculate_tolerance(&threshold_hi[i], &threshold_lo[i], exp_rate_ns);
>   
>   		igt_info("Requested rate[%d]: %"PRIu64" ns, Expected rate between: %"PRIu64" ns to %"PRIu64" ns\n",
>   				i, rates_ns[i], threshold_hi[i], threshold_lo[i]);
> @@ -497,6 +564,15 @@ flip_and_measure(data_t *data, igt_output_t *output, enum pipe pipe,
>   		wait_ns -= diff_ns;
>   		target_ns = event_ns + wait_ns;
>   
> +		/*
> +		 * FIXME: This logic makes next immediate frame time calculation
> +		 * in inconsistent state, even if next flip comes on correct time,
> +		 * it will be marked as fail due to time difference from previous

Nit pick: May be you can break this long sentence into smaller

With above minor changes:

Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal at intel.com>


> +		 * flip. Needs to reset at every cycle for correct measurement.
> +		 * Once this is corrected, igt can ask for more stricter pass
> +		 * criteria.
> +		 */
> +
>   		while (get_time_ns() < target_ns - 10);
>   	}
>