[PATCH v4 0/2] Bucketize refresh rate tolerance

[PATCH v4 0/2] Bucketize refresh rate tolerance

[Mitul Golani]

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.

Mitul Golani (2):
  tests/kms_vrr: Bucketize refresh rate tolerance
  tests/kms_vrr: Increase readability of kms_vrr

 tests/kms_vrr.c | 93 +++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 86 insertions(+), 7 deletions(-)

-- 
2.48.1

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

[Mitul Golani]

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.
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@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
+ */
+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.
+	 */
+
+	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
+		 * 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);
 	}
 
-- 
2.48.1

[PATCH v4 2/2] tests/kms_vrr: Increase readability of kms_vrr

[Mitul Golani]

Add converted refresh rate value apart from nanosecond time prints
to increase readability of debug logs.

Signed-off-by: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com>
---
 tests/kms_vrr.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/tests/kms_vrr.c b/tests/kms_vrr.c
index c9059c897..91e3c12a6 100644
--- a/tests/kms_vrr.c
+++ b/tests/kms_vrr.c
@@ -510,8 +510,10 @@ flip_and_measure(data_t *data, igt_output_t *output, enum pipe pipe,
 
 		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]);
+		igt_info("Requested rate[%d]: %" PRIu64 " ns (%.2f Hz), Expected rate between: %" PRIu64 " ns (%.2f Hz) to %" PRIu64 " ns (%.2f Hz)\n",
+			 i, rates_ns[i], (float)NSECS_PER_SEC / rates_ns[i], threshold_hi[i],
+			 (float)NSECS_PER_SEC / threshold_hi[i], threshold_lo[i],
+			 (float)NSECS_PER_SEC / threshold_lo[i]);
 	}
 
 	/* Align with the flip completion event to speed up convergence. */
@@ -536,8 +538,9 @@ flip_and_measure(data_t *data, igt_output_t *output, enum pipe pipe,
 		 */
 		event_ns = get_kernel_event_ns(data, DRM_EVENT_FLIP_COMPLETE);
 
-		igt_debug("event_ns - last_event_ns: %"PRIu64"\n",
-						(event_ns - last_event_ns));
+		igt_debug("event_ns - last_event_ns: %" PRIu64 " (%.2f Hz)\n",
+			  event_ns - last_event_ns,
+			  (float)NSECS_PER_SEC / (event_ns - last_event_ns));
 
 		/*
 		 * Check if the difference between the two flip timestamps
-- 
2.48.1

Re: [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@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@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);
>   	}
>