All of lore.kernel.org
 help / color / mirror / Atom feed
* [igt-dev] ✗ Fi.CI.BAT: failure for tests/kms_vrr: Update condition checks for flipline test (rev2)
  2021-01-19 14:17 [igt-dev] [PATCH i-g-t] tests/kms_vrr: Update condition checks for flipline test Bhanuprakash Modem
@ 2021-01-19  7:15 ` Patchwork
  2021-01-19 19:15 ` [igt-dev] [PATCH i-g-t] tests/kms_vrr: Update condition checks for flipline test Ville Syrjälä
  1 sibling, 0 replies; 6+ messages in thread
From: Patchwork @ 2021-01-19  7:15 UTC (permalink / raw)
  To: Bhanuprakash Modem; +Cc: igt-dev


[-- Attachment #1.1: Type: text/plain, Size: 3587 bytes --]

== Series Details ==

Series: tests/kms_vrr: Update condition checks for flipline test (rev2)
URL   : https://patchwork.freedesktop.org/series/84932/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_9636 -> IGTPW_5400
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with IGTPW_5400 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in IGTPW_5400, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5400/index.html

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in IGTPW_5400:

### IGT changes ###

#### Possible regressions ####

  * igt@gem_render_linear_blits@basic:
    - fi-byt-j1900:       [PASS][1] -> [FAIL][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9636/fi-byt-j1900/igt@gem_render_linear_blits@basic.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5400/fi-byt-j1900/igt@gem_render_linear_blits@basic.html

  
Known issues
------------

  Here are the changes found in IGTPW_5400 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@amdgpu/amd_basic@cs-compute:
    - fi-tgl-y:           NOTRUN -> [SKIP][3] ([fdo#109315] / [i915#2575]) +5 similar issues
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5400/fi-tgl-y/igt@amdgpu/amd_basic@cs-compute.html

  * igt@prime_vgem@basic-fence-flip:
    - fi-tgl-y:           [PASS][4] -> [DMESG-WARN][5] ([i915#402]) +1 similar issue
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9636/fi-tgl-y/igt@prime_vgem@basic-fence-flip.html
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5400/fi-tgl-y/igt@prime_vgem@basic-fence-flip.html

  
#### Possible fixes ####

  * igt@kms_chamelium@hdmi-crc-fast:
    - fi-kbl-7500u:       [DMESG-WARN][6] ([i915#2868]) -> [PASS][7]
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9636/fi-kbl-7500u/igt@kms_chamelium@hdmi-crc-fast.html
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5400/fi-kbl-7500u/igt@kms_chamelium@hdmi-crc-fast.html

  * igt@prime_self_import@basic-with_two_bos:
    - fi-tgl-y:           [DMESG-WARN][8] ([i915#402]) -> [PASS][9] +1 similar issue
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9636/fi-tgl-y/igt@prime_self_import@basic-with_two_bos.html
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5400/fi-tgl-y/igt@prime_self_import@basic-with_two_bos.html

  
  [fdo#109315]: https://bugs.freedesktop.org/show_bug.cgi?id=109315
  [i915#2575]: https://gitlab.freedesktop.org/drm/intel/issues/2575
  [i915#2868]: https://gitlab.freedesktop.org/drm/intel/issues/2868
  [i915#402]: https://gitlab.freedesktop.org/drm/intel/issues/402


Participating hosts (44 -> 37)
------------------------------

  Missing    (7): fi-ilk-m540 fi-hsw-4200u fi-bsw-cyan fi-ctg-p8600 fi-dg1-1 fi-cml-drallion fi-bdw-samus 


Build changes
-------------

  * CI: CI-20190529 -> None
  * IGT: IGT_5960 -> IGTPW_5400

  CI-20190529: 20190529
  CI_DRM_9636: f560ac388c527f2f166897c9091f7b9ad652050f @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_5400: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5400/index.html
  IGT_5960: ace82fcd5f3623f8dde7c220a825873dc53dfae4 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_5400/index.html

[-- Attachment #1.2: Type: text/html, Size: 4377 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [igt-dev] [PATCH i-g-t] tests/kms_vrr: Update condition checks for flipline test
@ 2021-01-19 14:17 Bhanuprakash Modem
  2021-01-19  7:15 ` [igt-dev] ✗ Fi.CI.BAT: failure for tests/kms_vrr: Update condition checks for flipline test (rev2) Patchwork
  2021-01-19 19:15 ` [igt-dev] [PATCH i-g-t] tests/kms_vrr: Update condition checks for flipline test Ville Syrjälä
  0 siblings, 2 replies; 6+ messages in thread
From: Bhanuprakash Modem @ 2021-01-19 14:17 UTC (permalink / raw)
  To: igt-dev

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

V2:
* Rebase

Cc: Manasi Navare <manasi.d.navare@intel.com>
Cc: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
Signed-off-by: Bhanuprakash Modem <bhanuprakash.modem@intel.com>
---
 tests/kms_vrr.c | 51 ++++++++++++++++++++-----------------------------
 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 pipe, bool enabled)
 {
 	igt_pipe_set_prop_value(&data->display, pipe, IGT_CRTC_VRR_ENABLED,
 				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 *output, enum pipe pipe)
 	 */
 	igt_pipe_set_prop_value(&data->display, pipe, IGT_CRTC_VRR_ENABLED, 0);

-	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. */
@@ -291,14 +290,10 @@ flip_and_measure(data_t *data, igt_output_t *output, enum pipe pipe,
 		 * difference between 144Hz and 143Hz which should give this
 		 * 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))
 			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, enum pipe pipe,
 		while (get_time_ns() < target_ns);
 	}

-	igt_info("Completed %u flips, %u were in threshold for %"PRIu64"ns.\n",
-		 total_flip, total_pass, rate_ns);
+	igt_info("Completed %u flips, %u were in threshold for (%llu Hz) %"PRIu64"ns.\n",
+		 total_flip, total_pass, (NSECS_PER_SEC/rate_ns), rate_ns);

 	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)
 	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) Hz\n",
+		 output->name, kmstest_pipe_name(pipe), range.min, range.max);

 	prepare_test(data, output, pipe);

@@ -370,46 +365,42 @@ test_basic(data_t *data, enum pipe pipe, igt_output_t *output, uint32_t flags)
 	 * 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
 	 *      would be 60Hz.
 	 * if refresh_rate is 50Hz:
 	 *      Flip will happen right away so returned refresh rate is 50Hz.
-	 * if refresh_rate < 40Hz:
-	 *      Flip should happen at the vmax so the returned refresh rate
-	 *      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);
 		igt_assert_f(result > 75,
-			     "Refresh rate %"PRIu64"ns: Target VRR on threshold not reached, result was %u%%\n",
-			     rate, result);
+			     "Refresh rate (%u Hz) %"PRIu64"ns: Target VRR on threshold not reached, result was %u%%\n",
+			     (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);
-		igt_assert_f(result > 75,
-			     "Refresh rate %"PRIu64"ns: Target VRR on threshold not reached, result was %u%%\n",
-			     rate, result);
+		igt_assert_f(result > 35,
+			     "Refresh rate (%u Hz) %"PRIu64"ns: Target VRR on threshold not reached, result was %u%%\n",
+			     (range.min - 5), rate, result);
 	}

 	rate = vtest_ns.mid;
 	result = flip_and_measure(data, output, pipe, rate, TEST_DURATION_NS);
 	igt_assert_f(result > 75,
-		     "Refresh rate %"PRIu64"ns: Target VRR on threshold not reached, result was %u%%\n",
-		     rate, result);
+		     "Refresh rate (%u Hz) %"PRIu64"ns: Target VRR on threshold not reached, result was %u%%\n",
+		     ((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);
 	igt_assert_f(result < 10,
-		     "Refresh rate %"PRIu64"ns: Target VRR off threshold exceeded, result was %u%%\n",
-		     rate, result);
+		     "Refresh rate (%u Hz) %"PRIu64"ns: Target VRR off threshold exceeded, result was %u%%\n",
+		     ((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

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [igt-dev] [PATCH i-g-t] tests/kms_vrr: Update condition checks for flipline test
  2021-01-19 14:17 [igt-dev] [PATCH i-g-t] tests/kms_vrr: Update condition checks for flipline test Bhanuprakash Modem
  2021-01-19  7:15 ` [igt-dev] ✗ Fi.CI.BAT: failure for tests/kms_vrr: Update condition checks for flipline test (rev2) Patchwork
@ 2021-01-19 19:15 ` Ville Syrjälä
  2021-01-20  5:36   ` Modem, Bhanuprakash
  1 sibling, 1 reply; 6+ messages in thread
From: Ville Syrjälä @ 2021-01-19 19:15 UTC (permalink / raw)
  To: Bhanuprakash Modem; +Cc: igt-dev

On Tue, Jan 19, 2021 at 07:47:12PM +0530, Bhanuprakash Modem wrote:
> 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
- otherwise confirm the timestamps are sensible. Not quite sure what the
  best way would be. One idea was to just make sure the timestamp points
  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
  for event, schedule next flip for event timestamps + whatever time we
  have left to reach the exected refresh rate). This latter idea I think
  would catch bugs where the interval between timestamps is correct but
  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.

> 
> V2:
> * Rebase
> 
> Cc: Manasi Navare <manasi.d.navare@intel.com>
> Cc: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
> Signed-off-by: Bhanuprakash Modem <bhanuprakash.modem@intel.com>
> ---
>  tests/kms_vrr.c | 51 ++++++++++++++++++++-----------------------------
>  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 pipe, bool enabled)
>  {
>  	igt_pipe_set_prop_value(&data->display, pipe, IGT_CRTC_VRR_ENABLED,
>  				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 *output, enum pipe pipe)
>  	 */
>  	igt_pipe_set_prop_value(&data->display, pipe, IGT_CRTC_VRR_ENABLED, 0);
> 
> -	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. */
> @@ -291,14 +290,10 @@ flip_and_measure(data_t *data, igt_output_t *output, enum pipe pipe,
>  		 * difference between 144Hz and 143Hz which should give this
>  		 * 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))
>  			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, enum pipe pipe,
>  		while (get_time_ns() < target_ns);
>  	}
> 
> -	igt_info("Completed %u flips, %u were in threshold for %"PRIu64"ns.\n",
> -		 total_flip, total_pass, rate_ns);
> +	igt_info("Completed %u flips, %u were in threshold for (%llu Hz) %"PRIu64"ns.\n",
> +		 total_flip, total_pass, (NSECS_PER_SEC/rate_ns), rate_ns);
> 
>  	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)
>  	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) Hz\n",
> +		 output->name, kmstest_pipe_name(pipe), range.min, range.max);
> 
>  	prepare_test(data, output, pipe);
> 
> @@ -370,46 +365,42 @@ test_basic(data_t *data, enum pipe pipe, igt_output_t *output, uint32_t flags)
>  	 * 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
>  	 *      would be 60Hz.
>  	 * if refresh_rate is 50Hz:
>  	 *      Flip will happen right away so returned refresh rate is 50Hz.
> -	 * if refresh_rate < 40Hz:
> -	 *      Flip should happen at the vmax so the returned refresh rate
> -	 *      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);
>  		igt_assert_f(result > 75,
> -			     "Refresh rate %"PRIu64"ns: Target VRR on threshold not reached, result was %u%%\n",
> -			     rate, result);
> +			     "Refresh rate (%u Hz) %"PRIu64"ns: Target VRR on threshold not reached, result was %u%%\n",
> +			     (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);
> -		igt_assert_f(result > 75,
> -			     "Refresh rate %"PRIu64"ns: Target VRR on threshold not reached, result was %u%%\n",
> -			     rate, result);
> +		igt_assert_f(result > 35,
> +			     "Refresh rate (%u Hz) %"PRIu64"ns: Target VRR on threshold not reached, result was %u%%\n",
> +			     (range.min - 5), rate, result);
>  	}
> 
>  	rate = vtest_ns.mid;
>  	result = flip_and_measure(data, output, pipe, rate, TEST_DURATION_NS);
>  	igt_assert_f(result > 75,
> -		     "Refresh rate %"PRIu64"ns: Target VRR on threshold not reached, result was %u%%\n",
> -		     rate, result);
> +		     "Refresh rate (%u Hz) %"PRIu64"ns: Target VRR on threshold not reached, result was %u%%\n",
> +		     ((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);
>  	igt_assert_f(result < 10,
> -		     "Refresh rate %"PRIu64"ns: Target VRR off threshold exceeded, result was %u%%\n",
> -		     rate, result);
> +		     "Refresh rate (%u Hz) %"PRIu64"ns: Target VRR off threshold exceeded, result was %u%%\n",
> +		     ((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
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [igt-dev] [PATCH i-g-t] tests/kms_vrr: Update condition checks for flipline test
  2021-01-19 19:15 ` [igt-dev] [PATCH i-g-t] tests/kms_vrr: Update condition checks for flipline test Ville Syrjälä
@ 2021-01-20  5:36   ` Modem, Bhanuprakash
  2021-01-20 10:53     ` Ville Syrjälä
  0 siblings, 1 reply; 6+ messages in thread
From: Modem, Bhanuprakash @ 2021-01-20  5:36 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: igt-dev

> -----Original Message-----
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Sent: Wednesday, January 20, 2021 12:46 AM
> To: Modem, Bhanuprakash <bhanuprakash.modem@intel.com>
> 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:
> > 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

> - otherwise confirm the timestamps are sensible. Not quite sure what the
>   best way would be. One idea was to just make sure the timestamp points
>   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
>   for event, schedule next flip for event timestamps + whatever time we
>   have left to reach the exected refresh rate). This latter idea I think
>   would catch bugs where the interval between timestamps is correct but
>   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
* Repeat

Am I missing something here?

> 
> >
> > V2:
> > * Rebase
> >
> > Cc: Manasi Navare <manasi.d.navare@intel.com>
> > Cc: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
> > Signed-off-by: Bhanuprakash Modem <bhanuprakash.modem@intel.com>
> > ---
> >  tests/kms_vrr.c | 51 ++++++++++++++++++++-----------------------------
> >  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
> pipe, bool enabled)
> >  {
> >  	igt_pipe_set_prop_value(&data->display, pipe, IGT_CRTC_VRR_ENABLED,
> >  				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
> *output, enum pipe pipe)
> >  	 */
> >  	igt_pipe_set_prop_value(&data->display, pipe, IGT_CRTC_VRR_ENABLED,
> 0);
> >
> > -	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.
> */
> > @@ -291,14 +290,10 @@ flip_and_measure(data_t *data, igt_output_t
> *output, enum pipe pipe,
> >  		 * difference between 144Hz and 143Hz which should give this
> >  		 * 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))
> >  			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,
> enum pipe pipe,
> >  		while (get_time_ns() < target_ns);
> >  	}
> >
> > -	igt_info("Completed %u flips, %u were in threshold for
> %"PRIu64"ns.\n",
> > -		 total_flip, total_pass, rate_ns);
> > +	igt_info("Completed %u flips, %u were in threshold for (%llu Hz)
> %"PRIu64"ns.\n",
> > +		 total_flip, total_pass, (NSECS_PER_SEC/rate_ns), rate_ns);
> >
> >  	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)
> >  	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)
> Hz\n",
> > +		 output->name, kmstest_pipe_name(pipe), range.min, range.max);
> >
> >  	prepare_test(data, output, pipe);
> >
> > @@ -370,46 +365,42 @@ test_basic(data_t *data, enum pipe pipe,
> igt_output_t *output, uint32_t flags)
> >  	 * 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
> >  	 *      would be 60Hz.
> >  	 * if refresh_rate is 50Hz:
> >  	 *      Flip will happen right away so returned refresh rate is
> 50Hz.
> > -	 * if refresh_rate < 40Hz:
> > -	 *      Flip should happen at the vmax so the returned refresh rate
> > -	 *      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);
> >  		igt_assert_f(result > 75,
> > -			     "Refresh rate %"PRIu64"ns: Target VRR on threshold
> not reached, result was %u%%\n",
> > -			     rate, result);
> > +			     "Refresh rate (%u Hz) %"PRIu64"ns: Target VRR on
> threshold not reached, result was %u%%\n",
> > +			     (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);
> > -		igt_assert_f(result > 75,
> > -			     "Refresh rate %"PRIu64"ns: Target VRR on threshold
> not reached, result was %u%%\n",
> > -			     rate, result);
> > +		igt_assert_f(result > 35,
> > +			     "Refresh rate (%u Hz) %"PRIu64"ns: Target VRR on
> threshold not reached, result was %u%%\n",
> > +			     (range.min - 5), rate, result);
> >  	}
> >
> >  	rate = vtest_ns.mid;
> >  	result = flip_and_measure(data, output, pipe, rate,
> TEST_DURATION_NS);
> >  	igt_assert_f(result > 75,
> > -		     "Refresh rate %"PRIu64"ns: Target VRR on threshold not
> reached, result was %u%%\n",
> > -		     rate, result);
> > +		     "Refresh rate (%u Hz) %"PRIu64"ns: Target VRR on threshold
> not reached, result was %u%%\n",
> > +		     ((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);
> >  	igt_assert_f(result < 10,
> > -		     "Refresh rate %"PRIu64"ns: Target VRR off threshold
> exceeded, result was %u%%\n",
> > -		     rate, result);
> > +		     "Refresh rate (%u Hz) %"PRIu64"ns: Target VRR off
> threshold exceeded, result was %u%%\n",
> > +		     ((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
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [igt-dev] [PATCH i-g-t] tests/kms_vrr: Update condition checks for flipline test
  2021-01-20  5:36   ` Modem, Bhanuprakash
@ 2021-01-20 10:53     ` Ville Syrjälä
  2021-01-22 11:52       ` Modem, Bhanuprakash
  0 siblings, 1 reply; 6+ messages in thread
From: Ville Syrjälä @ 2021-01-20 10:53 UTC (permalink / raw)
  To: Modem, Bhanuprakash; +Cc: igt-dev

On Wed, Jan 20, 2021 at 05:36:59AM +0000, Modem, Bhanuprakash wrote:
> > -----Original Message-----
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Sent: Wednesday, January 20, 2021 12:46 AM
> > To: Modem, Bhanuprakash <bhanuprakash.modem@intel.com>
> > 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:
> > > 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
> 
> > - otherwise confirm the timestamps are sensible. Not quite sure what the
> >   best way would be. One idea was to just make sure the timestamp points
> >   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
> >   for event, schedule next flip for event timestamps + whatever time we
> >   have left to reach the exected refresh rate). This latter idea I think
> >   would catch bugs where the interval between timestamps is correct but
> >   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.

> * Repeat
> 
> Am I missing something here?
> 
> > 
> > >
> > > V2:
> > > * Rebase
> > >
> > > Cc: Manasi Navare <manasi.d.navare@intel.com>
> > > Cc: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
> > > Signed-off-by: Bhanuprakash Modem <bhanuprakash.modem@intel.com>
> > > ---
> > >  tests/kms_vrr.c | 51 ++++++++++++++++++++-----------------------------
> > >  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
> > pipe, bool enabled)
> > >  {
> > >  	igt_pipe_set_prop_value(&data->display, pipe, IGT_CRTC_VRR_ENABLED,
> > >  				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
> > *output, enum pipe pipe)
> > >  	 */
> > >  	igt_pipe_set_prop_value(&data->display, pipe, IGT_CRTC_VRR_ENABLED,
> > 0);
> > >
> > > -	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.
> > */
> > > @@ -291,14 +290,10 @@ flip_and_measure(data_t *data, igt_output_t
> > *output, enum pipe pipe,
> > >  		 * difference between 144Hz and 143Hz which should give this
> > >  		 * 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))
> > >  			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,
> > enum pipe pipe,
> > >  		while (get_time_ns() < target_ns);
> > >  	}
> > >
> > > -	igt_info("Completed %u flips, %u were in threshold for
> > %"PRIu64"ns.\n",
> > > -		 total_flip, total_pass, rate_ns);
> > > +	igt_info("Completed %u flips, %u were in threshold for (%llu Hz)
> > %"PRIu64"ns.\n",
> > > +		 total_flip, total_pass, (NSECS_PER_SEC/rate_ns), rate_ns);
> > >
> > >  	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)
> > >  	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)
> > Hz\n",
> > > +		 output->name, kmstest_pipe_name(pipe), range.min, range.max);
> > >
> > >  	prepare_test(data, output, pipe);
> > >
> > > @@ -370,46 +365,42 @@ test_basic(data_t *data, enum pipe pipe,
> > igt_output_t *output, uint32_t flags)
> > >  	 * 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
> > >  	 *      would be 60Hz.
> > >  	 * if refresh_rate is 50Hz:
> > >  	 *      Flip will happen right away so returned refresh rate is
> > 50Hz.
> > > -	 * if refresh_rate < 40Hz:
> > > -	 *      Flip should happen at the vmax so the returned refresh rate
> > > -	 *      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);
> > >  		igt_assert_f(result > 75,
> > > -			     "Refresh rate %"PRIu64"ns: Target VRR on threshold
> > not reached, result was %u%%\n",
> > > -			     rate, result);
> > > +			     "Refresh rate (%u Hz) %"PRIu64"ns: Target VRR on
> > threshold not reached, result was %u%%\n",
> > > +			     (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);
> > > -		igt_assert_f(result > 75,
> > > -			     "Refresh rate %"PRIu64"ns: Target VRR on threshold
> > not reached, result was %u%%\n",
> > > -			     rate, result);
> > > +		igt_assert_f(result > 35,
> > > +			     "Refresh rate (%u Hz) %"PRIu64"ns: Target VRR on
> > threshold not reached, result was %u%%\n",
> > > +			     (range.min - 5), rate, result);
> > >  	}
> > >
> > >  	rate = vtest_ns.mid;
> > >  	result = flip_and_measure(data, output, pipe, rate,
> > TEST_DURATION_NS);
> > >  	igt_assert_f(result > 75,
> > > -		     "Refresh rate %"PRIu64"ns: Target VRR on threshold not
> > reached, result was %u%%\n",
> > > -		     rate, result);
> > > +		     "Refresh rate (%u Hz) %"PRIu64"ns: Target VRR on threshold
> > not reached, result was %u%%\n",
> > > +		     ((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);
> > >  	igt_assert_f(result < 10,
> > > -		     "Refresh rate %"PRIu64"ns: Target VRR off threshold
> > exceeded, result was %u%%\n",
> > > -		     rate, result);
> > > +		     "Refresh rate (%u Hz) %"PRIu64"ns: Target VRR off
> > threshold exceeded, result was %u%%\n",
> > > +		     ((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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [igt-dev] [PATCH i-g-t] tests/kms_vrr: Update condition checks for flipline test
  2021-01-20 10:53     ` Ville Syrjälä
@ 2021-01-22 11:52       ` Modem, Bhanuprakash
  0 siblings, 0 replies; 6+ messages in thread
From: Modem, Bhanuprakash @ 2021-01-22 11:52 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: igt-dev

> -----Original Message-----
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Sent: Wednesday, January 20, 2021 4:24 PM
> To: Modem, Bhanuprakash <bhanuprakash.modem@intel.com>
> Cc: igt-dev@lists.freedesktop.org; Navare, Manasi D
> <manasi.d.navare@intel.com>
> 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:
> > > -----Original Message-----
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Sent: Wednesday, January 20, 2021 12:46 AM
> > > To: Modem, Bhanuprakash <bhanuprakash.modem@intel.com>
> > > 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:
> > > > 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
> >
> > > - otherwise confirm the timestamps are sensible. Not quite sure what
> the
> > >   best way would be. One idea was to just make sure the timestamp
> points
> > >   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
> > >   for event, schedule next flip for event timestamps + whatever time
> we
> > >   have left to reach the exected refresh rate). This latter idea I
> think
> > >   would catch bugs where the interval between timestamps is correct
> but
> > >   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);

> 
> > * Repeat
> >
> > Am I missing something here?
> >
> > >
> > > >
> > > > V2:
> > > > * Rebase
> > > >
> > > > Cc: Manasi Navare <manasi.d.navare@intel.com>
> > > > Cc: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
> > > > Signed-off-by: Bhanuprakash Modem <bhanuprakash.modem@intel.com>
> > > > ---
> > > >  tests/kms_vrr.c | 51 ++++++++++++++++++++--------------------------
> ---
> > > >  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
> > > pipe, bool enabled)
> > > >  {
> > > >  	igt_pipe_set_prop_value(&data->display, pipe,
> IGT_CRTC_VRR_ENABLED,
> > > >  				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
> > > *output, enum pipe pipe)
> > > >  	 */
> > > >  	igt_pipe_set_prop_value(&data->display, pipe,
> IGT_CRTC_VRR_ENABLED,
> > > 0);
> > > >
> > > > -	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.
> > > */
> > > > @@ -291,14 +290,10 @@ flip_and_measure(data_t *data, igt_output_t
> > > *output, enum pipe pipe,
> > > >  		 * difference between 144Hz and 143Hz which should give
> this
> > > >  		 * 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))
> > > >  			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,
> > > enum pipe pipe,
> > > >  		while (get_time_ns() < target_ns);
> > > >  	}
> > > >
> > > > -	igt_info("Completed %u flips, %u were in threshold for
> > > %"PRIu64"ns.\n",
> > > > -		 total_flip, total_pass, rate_ns);
> > > > +	igt_info("Completed %u flips, %u were in threshold for (%llu
> Hz)
> > > %"PRIu64"ns.\n",
> > > > +		 total_flip, total_pass, (NSECS_PER_SEC/rate_ns),
> rate_ns);
> > > >
> > > >  	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)
> > > >  	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)
> > > Hz\n",
> > > > +		 output->name, kmstest_pipe_name(pipe), range.min,
> range.max);
> > > >
> > > >  	prepare_test(data, output, pipe);
> > > >
> > > > @@ -370,46 +365,42 @@ test_basic(data_t *data, enum pipe pipe,
> > > igt_output_t *output, uint32_t flags)
> > > >  	 * 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
> > > >  	 *      would be 60Hz.
> > > >  	 * if refresh_rate is 50Hz:
> > > >  	 *      Flip will happen right away so returned refresh rate is
> > > 50Hz.
> > > > -	 * if refresh_rate < 40Hz:
> > > > -	 *      Flip should happen at the vmax so the returned refresh
> rate
> > > > -	 *      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);
> > > >  		igt_assert_f(result > 75,
> > > > -			     "Refresh rate %"PRIu64"ns: Target VRR on
> threshold
> > > not reached, result was %u%%\n",
> > > > -			     rate, result);
> > > > +			     "Refresh rate (%u Hz) %"PRIu64"ns: Target VRR
> on
> > > threshold not reached, result was %u%%\n",
> > > > +			     (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);
> > > > -		igt_assert_f(result > 75,
> > > > -			     "Refresh rate %"PRIu64"ns: Target VRR on
> threshold
> > > not reached, result was %u%%\n",
> > > > -			     rate, result);
> > > > +		igt_assert_f(result > 35,
> > > > +			     "Refresh rate (%u Hz) %"PRIu64"ns: Target VRR
> on
> > > threshold not reached, result was %u%%\n",
> > > > +			     (range.min - 5), rate, result);
> > > >  	}
> > > >
> > > >  	rate = vtest_ns.mid;
> > > >  	result = flip_and_measure(data, output, pipe, rate,
> > > TEST_DURATION_NS);
> > > >  	igt_assert_f(result > 75,
> > > > -		     "Refresh rate %"PRIu64"ns: Target VRR on threshold
> not
> > > reached, result was %u%%\n",
> > > > -		     rate, result);
> > > > +		     "Refresh rate (%u Hz) %"PRIu64"ns: Target VRR on
> threshold
> > > not reached, result was %u%%\n",
> > > > +		     ((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);
> > > >  	igt_assert_f(result < 10,
> > > > -		     "Refresh rate %"PRIu64"ns: Target VRR off threshold
> > > exceeded, result was %u%%\n",
> > > > -		     rate, result);
> > > > +		     "Refresh rate (%u Hz) %"PRIu64"ns: Target VRR off
> > > threshold exceeded, result was %u%%\n",
> > > > +		     ((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

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-01-22 11:52 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-19 14:17 [igt-dev] [PATCH i-g-t] tests/kms_vrr: Update condition checks for flipline test Bhanuprakash Modem
2021-01-19  7:15 ` [igt-dev] ✗ Fi.CI.BAT: failure for tests/kms_vrr: Update condition checks for flipline test (rev2) Patchwork
2021-01-19 19:15 ` [igt-dev] [PATCH i-g-t] tests/kms_vrr: Update condition checks for flipline test Ville Syrjälä
2021-01-20  5:36   ` Modem, Bhanuprakash
2021-01-20 10:53     ` Ville Syrjälä
2021-01-22 11:52       ` Modem, Bhanuprakash

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.