-----Original Message-----
From: Ville Syrjälä <redacted>
Sent: Wednesday, January 20, 2021 4:24 PM
To: Modem, Bhanuprakash <redacted>
Cc: igt-dev@lists.freedesktop.org; Navare, Manasi D
[off-list ref]
Subject: Re: [igt-dev] [PATCH i-g-t] tests/kms_vrr: Update condition
checks for flipline test

On Wed, Jan 20, 2021 at 05:36:59AM +0000, Modem, Bhanuprakash wrote:

quoted

quoted

-----Original Message-----
From: Ville Syrjälä <redacted>
Sent: Wednesday, January 20, 2021 12:46 AM
To: Modem, Bhanuprakash <redacted>
Cc: igt-dev@lists.freedesktop.org
Subject: Re: [igt-dev] [PATCH i-g-t] tests/kms_vrr: Update condition
checks for flipline test

On Tue, Jan 19, 2021 at 07:47:12PM +0530, Bhanuprakash Modem wrote:

quoted

This patch includes below updates
* For Flipline test: if refresh_rate <= Vrr_min then
	- Expected returned refresh rate would be vrr_max
	- At least 35% of the flips should be in threshold
* Update "igt_display_commit_atomic" with "igt_display_commit2"
* Add few debug prints

Some ideas I had for a vrr test:
- test flipping at a few different rates
- make sure the interval between flip timestamps
  matches the expected rate

Actually, we are doing the same in our flipline[*] test.

Example: if we have a monitor with vrr range of 40 - 60Hz
We'll try to flip with 35, 50 and 65 Hz, and the difference between the
two flip timestamps should be within the required threshold from the
expected rate. A ~50us threshold is an arbitrary.

[*] https://patchwork.freedesktop.org/patch/392037

quoted

- otherwise confirm the timestamps are sensible. Not quite sure what

the

quoted

quoted

  best way would be. One idea was to just make sure the timestamp

points

quoted

quoted

  to more or less the correct point in time. Another idea was to drive
  the whole loop baed on the timestamps + target refresh rate (ie.

wait

quoted

quoted

  for event, schedule next flip for event timestamps + whatever time

we

quoted

quoted

  have left to reach the exected refresh rate). This latter idea I

think

quoted

quoted

  would catch bugs where the interval between timestamps is correct

but

quoted

quoted

  the absolute times are not correct (eg. if the timestamps are
  miscorrected to point fr into the future based on the max vblank
  length, or too close into the future based on min vblank length).
- the busy loop thing I don't think should be needed. Sure, there is
  some wakeup latency due to C-states and whatnot, but it should be
  possible to compensate to make sure we reach the target refresh rate
  w/o spinning.

This part is not clear to me.

Ex. To generate the flips with refresh rate 50Hz (ie. 20000000ns), below
is the expected sequence:

* Request a flip & wait for flip completion event
* Capture the event timestamp to compare
* Wait 20000000ns for next flip

That would anyway need a bit of correction to account for event
delivery latency + whatever overhead we have in submitting the
flip.

But in addition I was suggesting that we calculate how long to
wait based on the flip event timestamp + whatever extra is needed
on top to reach the target frame rate. Thus if the timestamp is
garbage the test should fail since we'll end up flipping at the
wrong rate.

Does this make sense?

-       diff_ns = now_ns - start_ns;
+       diff_ns = last_event_ns - start_ns;
        wait_ns = ((diff_ns + rate_ns - 1) / rate_ns) * rate_ns;
+       wait_ns += rate_ns - (last_event_ns - event_ns);

quoted

* Repeat

Am I missing something here?

quoted

quoted

V2:
* Rebase

Cc: Manasi Navare <redacted>
Cc: Nicholas Kazlauskas <redacted>
Signed-off-by: Bhanuprakash Modem <redacted>
---
 tests/kms_vrr.c | 51 ++++++++++++++++++++--------------------------

---

quoted

quoted

quoted

 1 file changed, 21 insertions(+), 30 deletions(-)

diff --git a/tests/kms_vrr.c b/tests/kms_vrr.c
index beb06854f..e6edd131f 100644
--- a/tests/kms_vrr.c
+++ b/tests/kms_vrr.c

@@ -170,7 +170,7 @@ static void set_vrr_on_pipe(data_t *data, enum

pipe

quoted

quoted

pipe, bool enabled)

quoted

 {
 	igt_pipe_set_prop_value(&data->display, pipe,

IGT_CRTC_VRR_ENABLED,

quoted

quoted

quoted

 				enabled);
-	igt_display_commit_atomic(&data->display, 0, NULL);
+	igt_display_commit2(&data->display, COMMIT_ATOMIC);
 }

 /* Prepare the display for testing on the given pipe. */

@@ -208,8 +208,7 @@ static void prepare_test(data_t *data,

igt_output_t

quoted

quoted

*output, enum pipe pipe)

quoted

 	 */
 	igt_pipe_set_prop_value(&data->display, pipe,

IGT_CRTC_VRR_ENABLED,

quoted

quoted

0);

quoted

-	igt_display_commit_atomic(&data->display,
-				  DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
+	igt_display_commit2(&data->display, COMMIT_ATOMIC);
 }

 /* Waits for the vblank interval. Returns the vblank timestamp in

ns.

quoted

quoted

*/

quoted

@@ -291,14 +290,10 @@ flip_and_measure(data_t *data, igt_output_t

*output, enum pipe pipe,

quoted

 		 * difference between 144Hz and 143Hz which should give

this

quoted

quoted

quoted

 		 * enough accuracy for most use cases.
 		 */
-		if (rate_ns <= vtest_ns.min && rate_ns >= vtest_ns.max)
+		if ((rate_ns < vtest_ns.min) && (rate_ns >=

vtest_ns.max))

quoted

quoted

quoted

 			diff_ns = rate_ns;
-		else if (rate_ns > vtest_ns.min)
-			diff_ns = vtest_ns.min;
-		else if (rate_ns < vtest_ns.max)
-			diff_ns = vtest_ns.max;
 		else
-			diff_ns = rate_ns;
+			diff_ns = vtest_ns.max;
 		diff_ns -= event_ns - last_event_ns;

 		if (llabs(diff_ns) < 50000ll)

@@ -323,8 +318,8 @@ flip_and_measure(data_t *data, igt_output_t

*output,

quoted

quoted

enum pipe pipe,

quoted

 		while (get_time_ns() < target_ns);
 	}

-	igt_info("Completed %u flips, %u were in threshold for

%"PRIu64"ns.\n",

quoted

-		 total_flip, total_pass, rate_ns);
+	igt_info("Completed %u flips, %u were in threshold for (%llu

Hz)

quoted

quoted

%"PRIu64"ns.\n",

quoted

+		 total_flip, total_pass, (NSECS_PER_SEC/rate_ns),

rate_ns);

quoted

quoted

quoted

 	return total_flip ? ((total_pass * 100) / total_flip) : 0;
 }

@@ -338,8 +333,8 @@ test_basic(data_t *data, enum pipe pipe,

igt_output_t *output, uint32_t flags)

quoted

 	range_t range = get_vrr_range(data, output);
 	uint64_t rate = vtest_ns.mid;

-	igt_info("VRR Test execution on %s, PIPE_%s\n",
-		 output->name, kmstest_pipe_name(pipe));
+	igt_info("VRR Test execution on %s, PIPE_%s with VRR range:

(%u-%u)

quoted

quoted

Hz\n",

quoted

+		 output->name, kmstest_pipe_name(pipe), range.min,

range.max);

quoted

quoted

quoted

 	prepare_test(data, output, pipe);

@@ -370,46 +365,42 @@ test_basic(data_t *data, enum pipe pipe,

igt_output_t *output, uint32_t flags)

quoted

 	 * decision boundary.
 	 *
 	 * Example: if range is 40 - 60Hz and
-	 * if refresh_rate > 60Hz:
+	 * if refresh_rate > 60Hz or <= 40Hz:
 	 *      Flip should happen at the flipline boundary & returned

refresh rate

quoted

 	 *      would be 60Hz.
 	 * if refresh_rate is 50Hz:
 	 *      Flip will happen right away so returned refresh rate is

50Hz.

quoted

-	 * if refresh_rate < 40Hz:
-	 *      Flip should happen at the vmax so the returned refresh

rate

quoted

quoted

quoted

-	 *      would be 40Hz.
 	 */
 	if (flags & TEST_FLIPLINE) {
-		rate = rate_from_refresh(range.min - 5);
+		rate = rate_from_refresh(range.max + 5);
 		result = flip_and_measure(data, output, pipe, rate,

TEST_DURATION_NS);

quoted

 		igt_assert_f(result > 75,
-			     "Refresh rate %"PRIu64"ns: Target VRR on

threshold

quoted

quoted

not reached, result was %u%%\n",

quoted

-			     rate, result);
+			     "Refresh rate (%u Hz) %"PRIu64"ns: Target VRR

on

quoted

quoted

threshold not reached, result was %u%%\n",

quoted

+			     (range.max + 5), rate, result);

-		rate = rate_from_refresh(range.max + 5);
+		rate = rate_from_refresh(range.min - 5);
 		result = flip_and_measure(data, output, pipe, rate,

TEST_DURATION_NS);

quoted

-		igt_assert_f(result > 75,
-			     "Refresh rate %"PRIu64"ns: Target VRR on

threshold

quoted

quoted

not reached, result was %u%%\n",

quoted

-			     rate, result);
+		igt_assert_f(result > 35,
+			     "Refresh rate (%u Hz) %"PRIu64"ns: Target VRR

on

quoted

quoted

threshold not reached, result was %u%%\n",

quoted

+			     (range.min - 5), rate, result);
 	}

 	rate = vtest_ns.mid;
 	result = flip_and_measure(data, output, pipe, rate,

TEST_DURATION_NS);

quoted

 	igt_assert_f(result > 75,
-		     "Refresh rate %"PRIu64"ns: Target VRR on threshold

not

quoted

quoted

reached, result was %u%%\n",

quoted

-		     rate, result);
+		     "Refresh rate (%u Hz) %"PRIu64"ns: Target VRR on

threshold

quoted

quoted

not reached, result was %u%%\n",

quoted

+		     ((range.max + range.min) / 2), rate, result);

 	set_vrr_on_pipe(data, pipe, 0);
 	result = flip_and_measure(data, output, pipe, rate,

TEST_DURATION_NS);

quoted

 	igt_assert_f(result < 10,
-		     "Refresh rate %"PRIu64"ns: Target VRR off threshold

exceeded, result was %u%%\n",

quoted

-		     rate, result);
+		     "Refresh rate (%u Hz) %"PRIu64"ns: Target VRR off

threshold exceeded, result was %u%%\n",

quoted

+		     ((range.max + range.min) / 2), rate, result);

 	/* Clean-up */
 	igt_plane_set_fb(data->primary, NULL);
 	igt_output_set_pipe(output, PIPE_NONE);
-	igt_display_commit_atomic(&data->display,
-				  DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
+	igt_display_commit2(&data->display, COMMIT_ATOMIC);

 	igt_remove_fb(data->drm_fd, &data->fb1);
 	igt_remove_fb(data->drm_fd, &data->fb0);
--
2.20.1

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

--
Ville Syrjälä
Intel

--
Ville Syrjälä
Intel

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev