[igt-dev] [PATCH i-g-t v4 0/2] kms_atomic_transition improvements

[igt-dev] [PATCH i-g-t v4 0/2] kms_atomic_transition improvements

[Stanislav Lisovskiy]

Fixed run_transition_test issue, which might happen when no changes
are done and wait_transition is called and made setup_parms function
tolerate if kernel can't afford having all sprite planes enabled
depending on current mode. Remove redundant condition nesting and code.

Stanislav Lisovskiy (2):
  igt/tests/kms_atomic_transition: Skip transition, if no changes done
  igt/tests/kms_atomic_transition: Tolerate if can't have all planes

 tests/kms_atomic_transition.c | 130 +++++++++++++++++++---------------
 1 file changed, 72 insertions(+), 58 deletions(-)

-- 
2.17.1

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

[igt-dev] [PATCH i-g-t v4 1/2] igt/tests/kms_atomic_transition: Skip transition, if no changes done

[Stanislav Lisovskiy]

While fixing used amount of planes, discovered that if
wm_setup_plane is called with 0 planes(might happen during
main testing cycle, as parms[i].mask can be 0 due to randomization)
then subsequent wait_transition fails in assertion on fd_completed.
So added return value to wm_setup_plane, which would allow to
determine, if we need to skip this step.

Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
---
 tests/kms_atomic_transition.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/tests/kms_atomic_transition.c b/tests/kms_atomic_transition.c
index 18f73317..638fe17e 100644
--- a/tests/kms_atomic_transition.c
+++ b/tests/kms_atomic_transition.c
@@ -118,11 +118,12 @@ static void configure_fencing(igt_plane_t *plane)
 	igt_assert_eq(ret, 0);
 }
 
-static void
+static int
 wm_setup_plane(igt_display_t *display, enum pipe pipe,
 	       uint32_t mask, struct plane_parms *parms, bool fencing)
 {
 	igt_plane_t *plane;
+	int planes_set_up = 0;
 
 	/*
 	* Make sure these buffers are suited for display use
@@ -133,8 +134,10 @@ wm_setup_plane(igt_display_t *display, enum pipe pipe,
 		int i = plane->index;
 
 		if (!mask || !(parms[i].mask & mask)) {
-			if (plane->values[IGT_PLANE_FB_ID])
+			if (plane->values[IGT_PLANE_FB_ID]) {
 				igt_plane_set_fb(plane, NULL);
+				planes_set_up++;
+			}
 			continue;
 		}
 
@@ -144,7 +147,10 @@ wm_setup_plane(igt_display_t *display, enum pipe pipe,
 		igt_plane_set_fb(plane, parms[i].fb);
 		igt_fb_set_size(parms[i].fb, plane, parms[i].width, parms[i].height);
 		igt_plane_set_size(plane, parms[i].width, parms[i].height);
+
+		planes_set_up++;
 	}
+	return planes_set_up;
 }
 
 static void ev_page_flip(int fd, unsigned seq, unsigned tv_sec, unsigned tv_usec, void *user_data)
@@ -544,7 +550,8 @@ run_transition_test(igt_display_t *display, enum pipe pipe, igt_output_t *output
 
 		igt_output_set_pipe(output, pipe);
 
-		wm_setup_plane(display, pipe, i, parms, fencing);
+		if (!wm_setup_plane(display, pipe, i, parms, fencing))
+			continue;
 
 		atomic_commit(display, pipe, flags, (void *)(unsigned long)i, fencing);
 		wait_for_transition(display, pipe, nonblocking, fencing);
@@ -552,7 +559,8 @@ run_transition_test(igt_display_t *display, enum pipe pipe, igt_output_t *output
 		if (type == TRANSITION_MODESET_DISABLE) {
 			igt_output_set_pipe(output, PIPE_NONE);
 
-			wm_setup_plane(display, pipe, 0, parms, fencing);
+			if (!wm_setup_plane(display, pipe, 0, parms, fencing))
+				continue;
 
 			atomic_commit(display, pipe, flags, (void *) 0UL, fencing);
 			wait_for_transition(display, pipe, nonblocking, fencing);
@@ -568,7 +576,8 @@ run_transition_test(igt_display_t *display, enum pipe pipe, igt_output_t *output
 				    n_enable_planes < pipe_obj->n_planes)
 					continue;
 
-				wm_setup_plane(display, pipe, j, parms, fencing);
+				if (!wm_setup_plane(display, pipe, j, parms, fencing))
+					continue;
 
 				if (type >= TRANSITION_MODESET)
 					igt_output_override_mode(output, &override_mode);
@@ -576,7 +585,9 @@ run_transition_test(igt_display_t *display, enum pipe pipe, igt_output_t *output
 				atomic_commit(display, pipe, flags, (void *)(unsigned long) j, fencing);
 				wait_for_transition(display, pipe, nonblocking, fencing);
 
-				wm_setup_plane(display, pipe, i, parms, fencing);
+				if (!wm_setup_plane(display, pipe, i, parms, fencing))
+					continue;
+
 				if (type >= TRANSITION_MODESET)
 					igt_output_override_mode(output, NULL);
 
-- 
2.17.1

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

[igt-dev] [PATCH i-g-t v4 2/2] igt/tests/kms_atomic_transition: Tolerate if can't have all planes

[Stanislav Lisovskiy]

With some upcoming changes i915 might not allow
all sprite planes enabled, depending on available
bandwidth limitation. Thus the test need to decrement
amount of planes and try again, instead of panicking.

v2: Removed excessive nested conditions, making code a bit
    more readable(hopefully).

v3: Stopped using global n_planes variable as it might cause
    resources being unreleased.
    Using now parms[i].mask as a way to determine if plane
    has to be included into commit.

v4: Removed unneeded n_overlays initialization.

Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
---
 tests/kms_atomic_transition.c | 107 +++++++++++++++++-----------------
 1 file changed, 55 insertions(+), 52 deletions(-)

diff --git a/tests/kms_atomic_transition.c b/tests/kms_atomic_transition.c
index 638fe17e..a04220f3 100644
--- a/tests/kms_atomic_transition.c
+++ b/tests/kms_atomic_transition.c
@@ -212,9 +212,11 @@ static void setup_parms(igt_display_t *display, enum pipe pipe,
 	unsigned sprite_width, sprite_height, prev_w, prev_h;
 	bool max_sprite_width, max_sprite_height, alpha = true;
 	uint32_t n_planes = display->pipes[pipe].n_planes;
-	uint32_t n_overlays = 0, overlays[n_planes];
+	uint32_t n_overlays, overlays[n_planes];
 	igt_plane_t *plane;
-	uint32_t iter_mask = 3;
+	uint32_t iter_mask;
+	int retries = n_planes - 1;
+	int ret = 0;
 
 	do_or_die(drmGetCap(display->drm_fd, DRM_CAP_CURSOR_WIDTH, &cursor_width));
 	if (cursor_width >= mode->hdisplay)
@@ -224,6 +226,10 @@ static void setup_parms(igt_display_t *display, enum pipe pipe,
 	if (cursor_height >= mode->vdisplay)
 		cursor_height = mode->vdisplay;
 
+retry:
+	n_overlays = 0;
+	iter_mask = 3;
+
 	for_each_plane_on_pipe(display, pipe, plane) {
 		int i = plane->index;
 
@@ -238,12 +244,16 @@ static void setup_parms(igt_display_t *display, enum pipe pipe,
 			parms[i].height = cursor_height;
 			parms[i].mask = 1 << 1;
 		} else {
-			parms[i].fb = sprite_fb;
-			parms[i].mask = 1 << 2;
-
-			iter_mask |= 1 << 2;
-
-			overlays[n_overlays++] = i;
+			if (n_overlays < (n_planes - 2)) {
+				parms[i].fb = sprite_fb;
+				parms[i].mask = 1 << 2;
+				iter_mask |= 1 << 2;
+				overlays[n_overlays++] = i;
+			}
+			else {
+				parms[i].fb = NULL;
+				parms[i].mask = 0;
+			}
 		}
 	}
 
@@ -278,16 +288,13 @@ static void setup_parms(igt_display_t *display, enum pipe pipe,
 	 * Pre gen9 not all sizes are supported, find the biggest possible
 	 * size that can be enabled on all sprite planes.
 	 */
-retry:
 	prev_w = sprite_width = cursor_width;
 	prev_h = sprite_height = cursor_height;
 
 	max_sprite_width = (sprite_width == mode->hdisplay);
 	max_sprite_height = (sprite_height == mode->vdisplay);
 
-	while (1) {
-		int ret;
-
+	while (!max_sprite_width && !max_sprite_height) {
 		set_sprite_wh(display, pipe, parms, sprite_fb,
 			      alpha, sprite_width, sprite_height);
 
@@ -295,54 +302,49 @@ retry:
 		ret = igt_display_try_commit_atomic(display, DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
 		igt_assert(!is_atomic_check_failure_errno(ret));
 
-		if (is_atomic_check_plane_size_errno(ret)) {
-			if (cursor_width == sprite_width &&
-			    cursor_height == sprite_height) {
-				igt_assert_f(alpha,
-					      "Cannot configure the test with all sprite planes enabled\n");
-
-				/* retry once with XRGB format. */
-				alpha = false;
-				goto retry;
-			}
-
-			sprite_width = prev_w;
-			sprite_height = prev_h;
-
-			if (max_sprite_width && max_sprite_height) {
-				set_sprite_wh(display, pipe, parms, sprite_fb,
-					      alpha, sprite_width, sprite_height);
-				break;
-			}
-
-			if (!max_sprite_width)
-				max_sprite_width = true;
-			else
-				max_sprite_height = true;
-		} else {
+		if (!is_atomic_check_plane_size_errno(ret)) {
 			prev_w = sprite_width;
 			prev_h = sprite_height;
-		}
-
-		if (!max_sprite_width) {
-			sprite_width *= 2;
-
+    
+			sprite_width *= max_sprite_width ? 1 : 2;
 			if (sprite_width >= mode->hdisplay) {
 				max_sprite_width = true;
-
 				sprite_width = mode->hdisplay;
 			}
-		} else if (!max_sprite_height) {
-			sprite_height *= 2;
 
+			sprite_height *= max_sprite_height ? 1 : 2;
 			if (sprite_height >= mode->vdisplay) {
 				max_sprite_height = true;
-
 				sprite_height = mode->vdisplay;
 			}
-		} else
-			/* Max sized sprites for all! */
-			break;
+			continue;
+		}
+
+		if (cursor_width == sprite_width &&
+		    cursor_height == sprite_height) {
+			igt_assert_f(retries > 0,
+				      "Cannot configure the test with all sprite planes enabled\n");
+			--retries;
+			/* retry once with XRGB format. */
+			if (alpha) {
+				alpha = false;
+				igt_warn("Removed alpha\n");
+			}
+			else {
+				igt_assert_f(n_planes > 1, "No planes left to proceed with!");
+				n_planes--;
+				igt_warn("Reduced available planes to %d\n", n_planes);
+			}
+			goto retry;
+		}
+
+		sprite_width = prev_w;
+		sprite_height = prev_h;
+
+		if (!max_sprite_width)
+			max_sprite_width = true;
+		else
+			max_sprite_height = true;
 	}
 
 	igt_info("Running test on pipe %s with resolution %dx%d and sprite size %dx%d alpha %i\n",
@@ -463,7 +465,6 @@ run_transition_test(igt_display_t *display, enum pipe pipe, igt_output_t *output
 
 	if (flags & DRM_MODE_ATOMIC_ALLOW_MODESET) {
 		igt_output_set_pipe(output, PIPE_NONE);
-
 		igt_display_commit2(display, COMMIT_ATOMIC);
 
 		igt_output_set_pipe(output, pipe);
@@ -525,8 +526,10 @@ run_transition_test(igt_display_t *display, enum pipe pipe, igt_output_t *output
 		}
 
 		/* force planes to be part of commit */
-		for_each_plane_on_pipe(display, pipe, plane)
-			igt_plane_set_position(plane, 0, 0);
+		for_each_plane_on_pipe(display, pipe, plane) {
+			if (parms[plane->index].mask)
+				igt_plane_set_position(plane, 0, 0);
+		}
 
 		igt_display_commit2(display, COMMIT_ATOMIC);
 
-- 
2.17.1

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

[igt-dev] ✗ Fi.CI.BAT: failure for kms_atomic_transition improvements (rev7)

[Patchwork]

== Series Details ==

Series: kms_atomic_transition improvements (rev7)
URL   : https://patchwork.freedesktop.org/series/58728/
State : failure

== Summary ==

CI Bug Log - changes from IGT_4924 -> IGTPW_2773
====================================================

Summary
-------

  **FAILURE**

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

  External URL: https://patchwork.freedesktop.org/api/1.0/series/58728/revisions/7/mbox/

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

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

### IGT changes ###

#### Possible regressions ####

  * igt@gem_tiled_blits@basic:
    - fi-icl-y:           PASS -> FAIL

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

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

### IGT changes ###

#### Issues hit ####

  * igt@amdgpu/amd_cs_nop@sync-fork-compute0:
    - fi-icl-u3:          NOTRUN -> SKIP [fdo#109315] +17

  * igt@gem_exec_basic@gtt-bsd1:
    - fi-icl-u3:          NOTRUN -> SKIP [fdo#109276] +7

  * igt@gem_exec_parse@basic-rejected:
    - fi-icl-u3:          NOTRUN -> SKIP [fdo#109289] +1

  * igt@i915_selftest@live_contexts:
    - fi-icl-u3:          NOTRUN -> DMESG-FAIL [fdo#108569]

  * igt@kms_busy@basic-flip-c:
    - fi-blb-e6850:       NOTRUN -> SKIP [fdo#109271] / [fdo#109278]

  * igt@kms_chamelium@hdmi-edid-read:
    - fi-icl-u3:          NOTRUN -> SKIP [fdo#109284] +8

  * igt@kms_force_connector_basic@prune-stale-modes:
    - fi-icl-u3:          NOTRUN -> SKIP [fdo#109285] +3

  * igt@kms_frontbuffer_tracking@basic:
    - fi-icl-u3:          NOTRUN -> FAIL [fdo#103167]

  * igt@kms_pipe_crc_basic@hang-read-crc-pipe-c:
    - fi-blb-e6850:       NOTRUN -> SKIP [fdo#109271] +48

  * igt@kms_pipe_crc_basic@read-crc-pipe-a-frame-sequence:
    - fi-byt-clapper:     PASS -> FAIL [fdo#103191] / [fdo#107362]

  
#### Possible fixes ####

  * igt@gem_exec_suspend@basic-s3:
    - fi-blb-e6850:       INCOMPLETE [fdo#107718] -> PASS

  * igt@gem_exec_suspend@basic-s4-devices:
    - fi-icl-y:           DMESG-WARN [fdo#109638] -> PASS

  * igt@i915_pm_rpm@module-reload:
    - fi-skl-6770hq:      FAIL [fdo#108511] -> PASS

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
    - fi-byt-clapper:     FAIL [fdo#103191] / [fdo#107362] -> PASS +1

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
  [fdo#107362]: https://bugs.freedesktop.org/show_bug.cgi?id=107362
  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#108511]: https://bugs.freedesktop.org/show_bug.cgi?id=108511
  [fdo#108569]: https://bugs.freedesktop.org/show_bug.cgi?id=108569
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109276]: https://bugs.freedesktop.org/show_bug.cgi?id=109276
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#109284]: https://bugs.freedesktop.org/show_bug.cgi?id=109284
  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [fdo#109289]: https://bugs.freedesktop.org/show_bug.cgi?id=109289
  [fdo#109315]: https://bugs.freedesktop.org/show_bug.cgi?id=109315
  [fdo#109593]: https://bugs.freedesktop.org/show_bug.cgi?id=109593
  [fdo#109638]: https://bugs.freedesktop.org/show_bug.cgi?id=109638


Participating hosts (45 -> 39)
------------------------------

  Additional (2): fi-icl-dsi fi-icl-u3 
  Missing    (8): fi-kbl-soraka fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-whl-u fi-gdg-551 fi-bdw-samus fi-skl-6600u 


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

    * IGT: IGT_4924 -> IGTPW_2773

  CI_DRM_5862: c97de10a1caf7b856efe273c901fceea3fc78367 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_2773: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2773/
  IGT_4924: bcf2f21996b2ee5c6177f5412046690ff8017772 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2773/
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t v4 2/2] igt/tests/kms_atomic_transition: Tolerate if can't have all planes

[Daniel Vetter]

On Wed, Apr 03, 2019 at 03:54:50PM +0300, Stanislav Lisovskiy wrote:
> With some upcoming changes i915 might not allow
> all sprite planes enabled, depending on available
> bandwidth limitation. Thus the test need to decrement
> amount of planes and try again, instead of panicking.
> 
> v2: Removed excessive nested conditions, making code a bit
>     more readable(hopefully).
> 
> v3: Stopped using global n_planes variable as it might cause
>     resources being unreleased.
>     Using now parms[i].mask as a way to determine if plane
>     has to be included into commit.
> 
> v4: Removed unneeded n_overlays initialization.
> 
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> ---
>  tests/kms_atomic_transition.c | 107 +++++++++++++++++-----------------
>  1 file changed, 55 insertions(+), 52 deletions(-)
> 
> diff --git a/tests/kms_atomic_transition.c b/tests/kms_atomic_transition.c
> index 638fe17e..a04220f3 100644
> --- a/tests/kms_atomic_transition.c
> +++ b/tests/kms_atomic_transition.c
> @@ -212,9 +212,11 @@ static void setup_parms(igt_display_t *display, enum pipe pipe,
>  	unsigned sprite_width, sprite_height, prev_w, prev_h;
>  	bool max_sprite_width, max_sprite_height, alpha = true;
>  	uint32_t n_planes = display->pipes[pipe].n_planes;
> -	uint32_t n_overlays = 0, overlays[n_planes];
> +	uint32_t n_overlays, overlays[n_planes];
>  	igt_plane_t *plane;
> -	uint32_t iter_mask = 3;
> +	uint32_t iter_mask;
> +	int retries = n_planes - 1;
> +	int ret = 0;
>  
>  	do_or_die(drmGetCap(display->drm_fd, DRM_CAP_CURSOR_WIDTH, &cursor_width));
>  	if (cursor_width >= mode->hdisplay)
> @@ -224,6 +226,10 @@ static void setup_parms(igt_display_t *display, enum pipe pipe,
>  	if (cursor_height >= mode->vdisplay)
>  		cursor_height = mode->vdisplay;
>  
> +retry:
> +	n_overlays = 0;
> +	iter_mask = 3;
> +
>  	for_each_plane_on_pipe(display, pipe, plane) {
>  		int i = plane->index;
>  
> @@ -238,12 +244,16 @@ static void setup_parms(igt_display_t *display, enum pipe pipe,
>  			parms[i].height = cursor_height;
>  			parms[i].mask = 1 << 1;
>  		} else {
> -			parms[i].fb = sprite_fb;
> -			parms[i].mask = 1 << 2;
> -
> -			iter_mask |= 1 << 2;
> -
> -			overlays[n_overlays++] = i;
> +			if (n_overlays < (n_planes - 2)) {
> +				parms[i].fb = sprite_fb;
> +				parms[i].mask = 1 << 2;
> +				iter_mask |= 1 << 2;
> +				overlays[n_overlays++] = i;
> +			}
> +			else {
> +				parms[i].fb = NULL;
> +				parms[i].mask = 0;
> +			}
>  		}
>  	}
>  
> @@ -278,16 +288,13 @@ static void setup_parms(igt_display_t *display, enum pipe pipe,
>  	 * Pre gen9 not all sizes are supported, find the biggest possible
>  	 * size that can be enabled on all sprite planes.
>  	 */
> -retry:
>  	prev_w = sprite_width = cursor_width;
>  	prev_h = sprite_height = cursor_height;
>  
>  	max_sprite_width = (sprite_width == mode->hdisplay);
>  	max_sprite_height = (sprite_height == mode->vdisplay);
>  
> -	while (1) {
> -		int ret;
> -
> +	while (!max_sprite_width && !max_sprite_height) {
>  		set_sprite_wh(display, pipe, parms, sprite_fb,
>  			      alpha, sprite_width, sprite_height);
>  
> @@ -295,54 +302,49 @@ retry:
>  		ret = igt_display_try_commit_atomic(display, DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
>  		igt_assert(!is_atomic_check_failure_errno(ret));
>  
> -		if (is_atomic_check_plane_size_errno(ret)) {
> -			if (cursor_width == sprite_width &&
> -			    cursor_height == sprite_height) {
> -				igt_assert_f(alpha,
> -					      "Cannot configure the test with all sprite planes enabled\n");
> -
> -				/* retry once with XRGB format. */
> -				alpha = false;
> -				goto retry;
> -			}
> -
> -			sprite_width = prev_w;
> -			sprite_height = prev_h;
> -
> -			if (max_sprite_width && max_sprite_height) {
> -				set_sprite_wh(display, pipe, parms, sprite_fb,
> -					      alpha, sprite_width, sprite_height);
> -				break;
> -			}
> -
> -			if (!max_sprite_width)
> -				max_sprite_width = true;
> -			else
> -				max_sprite_height = true;
> -		} else {
> +		if (!is_atomic_check_plane_size_errno(ret)) {
>  			prev_w = sprite_width;
>  			prev_h = sprite_height;
> -		}
> -
> -		if (!max_sprite_width) {
> -			sprite_width *= 2;
> -
> +    
> +			sprite_width *= max_sprite_width ? 1 : 2;
>  			if (sprite_width >= mode->hdisplay) {
>  				max_sprite_width = true;
> -
>  				sprite_width = mode->hdisplay;
>  			}
> -		} else if (!max_sprite_height) {
> -			sprite_height *= 2;
>  
> +			sprite_height *= max_sprite_height ? 1 : 2;
>  			if (sprite_height >= mode->vdisplay) {
>  				max_sprite_height = true;
> -
>  				sprite_height = mode->vdisplay;
>  			}
> -		} else
> -			/* Max sized sprites for all! */
> -			break;
> +			continue;
> +		}
> +
> +		if (cursor_width == sprite_width &&
> +		    cursor_height == sprite_height) {
> +			igt_assert_f(retries > 0,
> +				      "Cannot configure the test with all sprite planes enabled\n");
> +			--retries;
> +			/* retry once with XRGB format. */
> +			if (alpha) {
> +				alpha = false;
> +				igt_warn("Removed alpha\n");
> +			}
> +			else {
> +				igt_assert_f(n_planes > 1, "No planes left to proceed with!");
> +				n_planes--;

Same comment as with kms_concurrent and all the other plane tests: Just
dropping the last plane reduces our test coverage, we need to randomize
which plane we drop.

The other bit: We need to recompute how many planes we can enable for
every transition. Right now that's not the case with Ville's current patch
series, but that's also just an interim solution. Actual bw consumption
very much depends upon where all the planes are place, how big they are
and all that stuff. So we need to recompute/recheck that every transition
works, for each specific transition. Because the exact transition might
also matter.

Or maybe I'm not entirely understanding how this test works here.
-Daniel

> +				igt_warn("Reduced available planes to %d\n", n_planes);
> +			}
> +			goto retry;
> +		}
> +
> +		sprite_width = prev_w;
> +		sprite_height = prev_h;
> +
> +		if (!max_sprite_width)
> +			max_sprite_width = true;
> +		else
> +			max_sprite_height = true;
>  	}
>  
>  	igt_info("Running test on pipe %s with resolution %dx%d and sprite size %dx%d alpha %i\n",
> @@ -463,7 +465,6 @@ run_transition_test(igt_display_t *display, enum pipe pipe, igt_output_t *output
>  
>  	if (flags & DRM_MODE_ATOMIC_ALLOW_MODESET) {
>  		igt_output_set_pipe(output, PIPE_NONE);
> -
>  		igt_display_commit2(display, COMMIT_ATOMIC);
>  
>  		igt_output_set_pipe(output, pipe);
> @@ -525,8 +526,10 @@ run_transition_test(igt_display_t *display, enum pipe pipe, igt_output_t *output
>  		}
>  
>  		/* force planes to be part of commit */
> -		for_each_plane_on_pipe(display, pipe, plane)
> -			igt_plane_set_position(plane, 0, 0);
> +		for_each_plane_on_pipe(display, pipe, plane) {
> +			if (parms[plane->index].mask)
> +				igt_plane_set_position(plane, 0, 0);
> +		}
>  
>  		igt_display_commit2(display, COMMIT_ATOMIC);
>  
> -- 
> 2.17.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] ✓ Fi.CI.BAT: success for kms_atomic_transition improvements (rev8)

[Patchwork]

== Series Details ==

Series: kms_atomic_transition improvements (rev8)
URL   : https://patchwork.freedesktop.org/series/58728/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5865 -> IGTPW_2779
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/58728/revisions/8/mbox/

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

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

### IGT changes ###

#### Issues hit ####

  * igt@amdgpu/amd_basic@cs-compute:
    - fi-kbl-8809g:       NOTRUN -> FAIL [fdo#108094]

  * igt@gem_exec_suspend@basic-s3:
    - fi-blb-e6850:       PASS -> INCOMPLETE [fdo#107718]

  * igt@i915_selftest@live_evict:
    - fi-bsw-kefka:       PASS -> DMESG-WARN [fdo#107709]

  * igt@kms_busy@basic-flip-a:
    - fi-bsw-n3050:       NOTRUN -> SKIP [fdo#109271] / [fdo#109278] +1

  * igt@kms_chamelium@hdmi-crc-fast:
    - fi-bsw-n3050:       NOTRUN -> SKIP [fdo#109271] +62

  * igt@kms_pipe_crc_basic@nonblocking-crc-pipe-b:
    - fi-byt-clapper:     PASS -> FAIL [fdo#107362]

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
    - fi-byt-clapper:     PASS -> FAIL [fdo#103191] / [fdo#107362] +2

  * igt@prime_vgem@basic-fence-flip:
    - fi-ilk-650:         PASS -> FAIL [fdo#104008]

  * igt@runner@aborted:
    - fi-bsw-kefka:       NOTRUN -> FAIL [fdo#107709]

  
#### Possible fixes ####

  * igt@amdgpu/amd_basic@userptr:
    - fi-kbl-8809g:       DMESG-WARN [fdo#108965] -> PASS

  * igt@i915_selftest@live_uncore:
    - fi-skl-gvtdvm:      DMESG-FAIL [fdo#110210] -> PASS
    - fi-ivb-3770:        DMESG-FAIL [fdo#110210] -> PASS

  * igt@kms_pipe_crc_basic@read-crc-pipe-a:
    - fi-byt-clapper:     FAIL [fdo#107362] -> PASS

  
#### Warnings ####

  * igt@i915_selftest@live_contexts:
    - fi-icl-y:           INCOMPLETE [fdo#108569] -> DMESG-FAIL [fdo#108569]

  
  [fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
  [fdo#104008]: https://bugs.freedesktop.org/show_bug.cgi?id=104008
  [fdo#107362]: https://bugs.freedesktop.org/show_bug.cgi?id=107362
  [fdo#107709]: https://bugs.freedesktop.org/show_bug.cgi?id=107709
  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#108094]: https://bugs.freedesktop.org/show_bug.cgi?id=108094
  [fdo#108569]: https://bugs.freedesktop.org/show_bug.cgi?id=108569
  [fdo#108965]: https://bugs.freedesktop.org/show_bug.cgi?id=108965
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#110210]: https://bugs.freedesktop.org/show_bug.cgi?id=110210


Participating hosts (46 -> 40)
------------------------------

  Additional (1): fi-bsw-n3050 
  Missing    (7): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-bdw-samus 


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

    * IGT: IGT_4925 -> IGTPW_2779

  CI_DRM_5865: 0dafc5b0971e3e7622864fe0987d6676f0d2c7a6 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_2779: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2779/
  IGT_4925: ca623acb8b2b6f0a4cdb01946dc9002e11d62574 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2779/
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t v4 2/2] igt/tests/kms_atomic_transition: Tolerate if can't have all planes

[Lisovskiy, Stanislav]

On Wed, 2019-04-03 at 16:11 +0200, Daniel Vetter wrote:
> On Wed, Apr 03, 2019 at 03:54:50PM +0300, Stanislav Lisovskiy wrote:
> > With some upcoming changes i915 might not allow
> > all sprite planes enabled, depending on available
> > bandwidth limitation. Thus the test need to decrement
> > amount of planes and try again, instead of panicking.
> > 
> > v2: Removed excessive nested conditions, making code a bit
> >     more readable(hopefully).
> > 
> > v3: Stopped using global n_planes variable as it might cause
> >     resources being unreleased.
> >     Using now parms[i].mask as a way to determine if plane
> >     has to be included into commit.
> > 
> > v4: Removed unneeded n_overlays initialization.
> > 
> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > ---
> >  tests/kms_atomic_transition.c | 107 +++++++++++++++++-------------
> > ----
> >  1 file changed, 55 insertions(+), 52 deletions(-)
> > 
> > diff --git a/tests/kms_atomic_transition.c
> > b/tests/kms_atomic_transition.c
> > index 638fe17e..a04220f3 100644
> > --- a/tests/kms_atomic_transition.c
> > +++ b/tests/kms_atomic_transition.c
> > @@ -212,9 +212,11 @@ static void setup_parms(igt_display_t
> > *display, enum pipe pipe,
> >  	unsigned sprite_width, sprite_height, prev_w, prev_h;
> >  	bool max_sprite_width, max_sprite_height, alpha = true;
> >  	uint32_t n_planes = display->pipes[pipe].n_planes;
> > -	uint32_t n_overlays = 0, overlays[n_planes];
> > +	uint32_t n_overlays, overlays[n_planes];
> >  	igt_plane_t *plane;
> > -	uint32_t iter_mask = 3;
> > +	uint32_t iter_mask;
> > +	int retries = n_planes - 1;
> > +	int ret = 0;
> >  
> >  	do_or_die(drmGetCap(display->drm_fd, DRM_CAP_CURSOR_WIDTH,
> > &cursor_width));
> >  	if (cursor_width >= mode->hdisplay)
> > @@ -224,6 +226,10 @@ static void setup_parms(igt_display_t
> > *display, enum pipe pipe,
> >  	if (cursor_height >= mode->vdisplay)
> >  		cursor_height = mode->vdisplay;
> >  
> > +retry:
> > +	n_overlays = 0;
> > +	iter_mask = 3;
> > +
> >  	for_each_plane_on_pipe(display, pipe, plane) {
> >  		int i = plane->index;
> >  
> > @@ -238,12 +244,16 @@ static void setup_parms(igt_display_t
> > *display, enum pipe pipe,
> >  			parms[i].height = cursor_height;
> >  			parms[i].mask = 1 << 1;
> >  		} else {
> > -			parms[i].fb = sprite_fb;
> > -			parms[i].mask = 1 << 2;
> > -
> > -			iter_mask |= 1 << 2;
> > -
> > -			overlays[n_overlays++] = i;
> > +			if (n_overlays < (n_planes - 2)) {
> > +				parms[i].fb = sprite_fb;
> > +				parms[i].mask = 1 << 2;
> > +				iter_mask |= 1 << 2;
> > +				overlays[n_overlays++] = i;
> > +			}
> > +			else {
> > +				parms[i].fb = NULL;
> > +				parms[i].mask = 0;
> > +			}
> >  		}
> >  	}
> >  
> > @@ -278,16 +288,13 @@ static void setup_parms(igt_display_t
> > *display, enum pipe pipe,
> >  	 * Pre gen9 not all sizes are supported, find the biggest
> > possible
> >  	 * size that can be enabled on all sprite planes.
> >  	 */
> > -retry:
> >  	prev_w = sprite_width = cursor_width;
> >  	prev_h = sprite_height = cursor_height;
> >  
> >  	max_sprite_width = (sprite_width == mode->hdisplay);
> >  	max_sprite_height = (sprite_height == mode->vdisplay);
> >  
> > -	while (1) {
> > -		int ret;
> > -
> > +	while (!max_sprite_width && !max_sprite_height) {
> >  		set_sprite_wh(display, pipe, parms, sprite_fb,
> >  			      alpha, sprite_width, sprite_height);
> >  
> > @@ -295,54 +302,49 @@ retry:
> >  		ret = igt_display_try_commit_atomic(display,
> > DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
> >  		igt_assert(!is_atomic_check_failure_errno(ret));
> >  
> > -		if (is_atomic_check_plane_size_errno(ret)) {
> > -			if (cursor_width == sprite_width &&
> > -			    cursor_height == sprite_height) {
> > -				igt_assert_f(alpha,
> > -					      "Cannot configure the
> > test with all sprite planes enabled\n");
> > -
> > -				/* retry once with XRGB format. */
> > -				alpha = false;
> > -				goto retry;
> > -			}
> > -
> > -			sprite_width = prev_w;
> > -			sprite_height = prev_h;
> > -
> > -			if (max_sprite_width && max_sprite_height) {
> > -				set_sprite_wh(display, pipe, parms,
> > sprite_fb,
> > -					      alpha, sprite_width,
> > sprite_height);
> > -				break;
> > -			}
> > -
> > -			if (!max_sprite_width)
> > -				max_sprite_width = true;
> > -			else
> > -				max_sprite_height = true;
> > -		} else {
> > +		if (!is_atomic_check_plane_size_errno(ret)) {
> >  			prev_w = sprite_width;
> >  			prev_h = sprite_height;
> > -		}
> > -
> > -		if (!max_sprite_width) {
> > -			sprite_width *= 2;
> > -
> > +    
> > +			sprite_width *= max_sprite_width ? 1 : 2;
> >  			if (sprite_width >= mode->hdisplay) {
> >  				max_sprite_width = true;
> > -
> >  				sprite_width = mode->hdisplay;
> >  			}
> > -		} else if (!max_sprite_height) {
> > -			sprite_height *= 2;
> >  
> > +			sprite_height *= max_sprite_height ? 1 : 2;
> >  			if (sprite_height >= mode->vdisplay) {
> >  				max_sprite_height = true;
> > -
> >  				sprite_height = mode->vdisplay;
> >  			}
> > -		} else
> > -			/* Max sized sprites for all! */
> > -			break;
> > +			continue;
> > +		}
> > +
> > +		if (cursor_width == sprite_width &&
> > +		    cursor_height == sprite_height) {
> > +			igt_assert_f(retries > 0,
> > +				      "Cannot configure the test with
> > all sprite planes enabled\n");
> > +			--retries;
> > +			/* retry once with XRGB format. */
> > +			if (alpha) {
> > +				alpha = false;
> > +				igt_warn("Removed alpha\n");
> > +			}
> > +			else {
> > +				igt_assert_f(n_planes > 1, "No planes
> > left to proceed with!");
> > +				n_planes--;
> 
> Same comment as with kms_concurrent and all the other plane tests:
> Just
> dropping the last plane reduces our test coverage, we need to
> randomize
> which plane we drop.

I think that is a good idea, I'm now testing a patch which randomizes
which one of available planes is going to be removed.

For the latter change I would leave for further patches as currently 
it doesn't matter anyway, but we need the issues related to new bw
requirements to be fixed.
> 
> The other bit: We need to recompute how many planes we can enable for
> every transition. Right now that's not the case with Ville's current
> patch
> series, but that's also just an interim solution. Actual bw
> consumption
> very much depends upon where all the planes are place, how big they
> are
> and all that stuff. So we need to recompute/recheck that every
> transition
> works, for each specific transition. Because the exact transition
> might
> also matter.
> 
> Or maybe I'm not entirely understanding how this test works here.
> -Daniel
> 
> > +				igt_warn("Reduced available planes to
> > %d\n", n_planes);
> > +			}
> > +			goto retry;
> > +		}
> > +
> > +		sprite_width = prev_w;
> > +		sprite_height = prev_h;
> > +
> > +		if (!max_sprite_width)
> > +			max_sprite_width = true;
> > +		else
> > +			max_sprite_height = true;
> >  	}
> >  
> >  	igt_info("Running test on pipe %s with resolution %dx%d and
> > sprite size %dx%d alpha %i\n",
> > @@ -463,7 +465,6 @@ run_transition_test(igt_display_t *display,
> > enum pipe pipe, igt_output_t *output
> >  
> >  	if (flags & DRM_MODE_ATOMIC_ALLOW_MODESET) {
> >  		igt_output_set_pipe(output, PIPE_NONE);
> > -
> >  		igt_display_commit2(display, COMMIT_ATOMIC);
> >  
> >  		igt_output_set_pipe(output, pipe);
> > @@ -525,8 +526,10 @@ run_transition_test(igt_display_t *display,
> > enum pipe pipe, igt_output_t *output
> >  		}
> >  
> >  		/* force planes to be part of commit */
> > -		for_each_plane_on_pipe(display, pipe, plane)
> > -			igt_plane_set_position(plane, 0, 0);
> > +		for_each_plane_on_pipe(display, pipe, plane) {
> > +			if (parms[plane->index].mask)
> > +				igt_plane_set_position(plane, 0, 0);
> > +		}
> >  
> >  		igt_display_commit2(display, COMMIT_ATOMIC);
> >  
> > -- 
> > 2.17.1
> > 
> 
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t v4 2/2] igt/tests/kms_atomic_transition: Tolerate if can't have all planes

[Daniel Vetter]

On Thu, Apr 04, 2019 at 11:45:03AM +0000, Lisovskiy, Stanislav wrote:
> On Wed, 2019-04-03 at 16:11 +0200, Daniel Vetter wrote:
> > On Wed, Apr 03, 2019 at 03:54:50PM +0300, Stanislav Lisovskiy wrote:
> > > With some upcoming changes i915 might not allow
> > > all sprite planes enabled, depending on available
> > > bandwidth limitation. Thus the test need to decrement
> > > amount of planes and try again, instead of panicking.
> > > 
> > > v2: Removed excessive nested conditions, making code a bit
> > >     more readable(hopefully).
> > > 
> > > v3: Stopped using global n_planes variable as it might cause
> > >     resources being unreleased.
> > >     Using now parms[i].mask as a way to determine if plane
> > >     has to be included into commit.
> > > 
> > > v4: Removed unneeded n_overlays initialization.
> > > 
> > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > > ---
> > >  tests/kms_atomic_transition.c | 107 +++++++++++++++++-------------
> > > ----
> > >  1 file changed, 55 insertions(+), 52 deletions(-)
> > > 
> > > diff --git a/tests/kms_atomic_transition.c
> > > b/tests/kms_atomic_transition.c
> > > index 638fe17e..a04220f3 100644
> > > --- a/tests/kms_atomic_transition.c
> > > +++ b/tests/kms_atomic_transition.c
> > > @@ -212,9 +212,11 @@ static void setup_parms(igt_display_t
> > > *display, enum pipe pipe,
> > >  	unsigned sprite_width, sprite_height, prev_w, prev_h;
> > >  	bool max_sprite_width, max_sprite_height, alpha = true;
> > >  	uint32_t n_planes = display->pipes[pipe].n_planes;
> > > -	uint32_t n_overlays = 0, overlays[n_planes];
> > > +	uint32_t n_overlays, overlays[n_planes];
> > >  	igt_plane_t *plane;
> > > -	uint32_t iter_mask = 3;
> > > +	uint32_t iter_mask;
> > > +	int retries = n_planes - 1;
> > > +	int ret = 0;
> > >  
> > >  	do_or_die(drmGetCap(display->drm_fd, DRM_CAP_CURSOR_WIDTH,
> > > &cursor_width));
> > >  	if (cursor_width >= mode->hdisplay)
> > > @@ -224,6 +226,10 @@ static void setup_parms(igt_display_t
> > > *display, enum pipe pipe,
> > >  	if (cursor_height >= mode->vdisplay)
> > >  		cursor_height = mode->vdisplay;
> > >  
> > > +retry:
> > > +	n_overlays = 0;
> > > +	iter_mask = 3;
> > > +
> > >  	for_each_plane_on_pipe(display, pipe, plane) {
> > >  		int i = plane->index;
> > >  
> > > @@ -238,12 +244,16 @@ static void setup_parms(igt_display_t
> > > *display, enum pipe pipe,
> > >  			parms[i].height = cursor_height;
> > >  			parms[i].mask = 1 << 1;
> > >  		} else {
> > > -			parms[i].fb = sprite_fb;
> > > -			parms[i].mask = 1 << 2;
> > > -
> > > -			iter_mask |= 1 << 2;
> > > -
> > > -			overlays[n_overlays++] = i;
> > > +			if (n_overlays < (n_planes - 2)) {
> > > +				parms[i].fb = sprite_fb;
> > > +				parms[i].mask = 1 << 2;
> > > +				iter_mask |= 1 << 2;
> > > +				overlays[n_overlays++] = i;
> > > +			}
> > > +			else {
> > > +				parms[i].fb = NULL;
> > > +				parms[i].mask = 0;
> > > +			}
> > >  		}
> > >  	}
> > >  
> > > @@ -278,16 +288,13 @@ static void setup_parms(igt_display_t
> > > *display, enum pipe pipe,
> > >  	 * Pre gen9 not all sizes are supported, find the biggest
> > > possible
> > >  	 * size that can be enabled on all sprite planes.
> > >  	 */
> > > -retry:
> > >  	prev_w = sprite_width = cursor_width;
> > >  	prev_h = sprite_height = cursor_height;
> > >  
> > >  	max_sprite_width = (sprite_width == mode->hdisplay);
> > >  	max_sprite_height = (sprite_height == mode->vdisplay);
> > >  
> > > -	while (1) {
> > > -		int ret;
> > > -
> > > +	while (!max_sprite_width && !max_sprite_height) {
> > >  		set_sprite_wh(display, pipe, parms, sprite_fb,
> > >  			      alpha, sprite_width, sprite_height);
> > >  
> > > @@ -295,54 +302,49 @@ retry:
> > >  		ret = igt_display_try_commit_atomic(display,
> > > DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
> > >  		igt_assert(!is_atomic_check_failure_errno(ret));
> > >  
> > > -		if (is_atomic_check_plane_size_errno(ret)) {
> > > -			if (cursor_width == sprite_width &&
> > > -			    cursor_height == sprite_height) {
> > > -				igt_assert_f(alpha,
> > > -					      "Cannot configure the
> > > test with all sprite planes enabled\n");
> > > -
> > > -				/* retry once with XRGB format. */
> > > -				alpha = false;
> > > -				goto retry;
> > > -			}
> > > -
> > > -			sprite_width = prev_w;
> > > -			sprite_height = prev_h;
> > > -
> > > -			if (max_sprite_width && max_sprite_height) {
> > > -				set_sprite_wh(display, pipe, parms,
> > > sprite_fb,
> > > -					      alpha, sprite_width,
> > > sprite_height);
> > > -				break;
> > > -			}
> > > -
> > > -			if (!max_sprite_width)
> > > -				max_sprite_width = true;
> > > -			else
> > > -				max_sprite_height = true;
> > > -		} else {
> > > +		if (!is_atomic_check_plane_size_errno(ret)) {
> > >  			prev_w = sprite_width;
> > >  			prev_h = sprite_height;
> > > -		}
> > > -
> > > -		if (!max_sprite_width) {
> > > -			sprite_width *= 2;
> > > -
> > > +    
> > > +			sprite_width *= max_sprite_width ? 1 : 2;
> > >  			if (sprite_width >= mode->hdisplay) {
> > >  				max_sprite_width = true;
> > > -
> > >  				sprite_width = mode->hdisplay;
> > >  			}
> > > -		} else if (!max_sprite_height) {
> > > -			sprite_height *= 2;
> > >  
> > > +			sprite_height *= max_sprite_height ? 1 : 2;
> > >  			if (sprite_height >= mode->vdisplay) {
> > >  				max_sprite_height = true;
> > > -
> > >  				sprite_height = mode->vdisplay;
> > >  			}
> > > -		} else
> > > -			/* Max sized sprites for all! */
> > > -			break;
> > > +			continue;
> > > +		}
> > > +
> > > +		if (cursor_width == sprite_width &&
> > > +		    cursor_height == sprite_height) {
> > > +			igt_assert_f(retries > 0,
> > > +				      "Cannot configure the test with
> > > all sprite planes enabled\n");
> > > +			--retries;
> > > +			/* retry once with XRGB format. */
> > > +			if (alpha) {
> > > +				alpha = false;
> > > +				igt_warn("Removed alpha\n");
> > > +			}
> > > +			else {
> > > +				igt_assert_f(n_planes > 1, "No planes
> > > left to proceed with!");
> > > +				n_planes--;
> > 
> > Same comment as with kms_concurrent and all the other plane tests:
> > Just
> > dropping the last plane reduces our test coverage, we need to
> > randomize
> > which plane we drop.
> 
> I think that is a good idea, I'm now testing a patch which randomizes
> which one of available planes is going to be removed.
> 
> For the latter change I would leave for further patches as currently 
> it doesn't matter anyway, but we need the issues related to new bw
> requirements to be fixed.

Just doing some bug scrub and we have some EINVAL failures like this all
over the place. So I guess there's some other cases where we reject
configs in the kernel and then the test falls over. We'll need this (it's
how atomic is meant to work really).

Also, the bw band-aid from Ville is a really bad solution, we need
something better - no point in having 7 planes if you can't even use them.
Adding more try_commit in the wrong place feels like a detour.
-Daniel

> > 
> > The other bit: We need to recompute how many planes we can enable for
> > every transition. Right now that's not the case with Ville's current
> > patch
> > series, but that's also just an interim solution. Actual bw
> > consumption
> > very much depends upon where all the planes are place, how big they
> > are
> > and all that stuff. So we need to recompute/recheck that every
> > transition
> > works, for each specific transition. Because the exact transition
> > might
> > also matter.
> > 
> > Or maybe I'm not entirely understanding how this test works here.
> > -Daniel
> > 
> > > +				igt_warn("Reduced available planes to
> > > %d\n", n_planes);
> > > +			}
> > > +			goto retry;
> > > +		}
> > > +
> > > +		sprite_width = prev_w;
> > > +		sprite_height = prev_h;
> > > +
> > > +		if (!max_sprite_width)
> > > +			max_sprite_width = true;
> > > +		else
> > > +			max_sprite_height = true;
> > >  	}
> > >  
> > >  	igt_info("Running test on pipe %s with resolution %dx%d and
> > > sprite size %dx%d alpha %i\n",
> > > @@ -463,7 +465,6 @@ run_transition_test(igt_display_t *display,
> > > enum pipe pipe, igt_output_t *output
> > >  
> > >  	if (flags & DRM_MODE_ATOMIC_ALLOW_MODESET) {
> > >  		igt_output_set_pipe(output, PIPE_NONE);
> > > -
> > >  		igt_display_commit2(display, COMMIT_ATOMIC);
> > >  
> > >  		igt_output_set_pipe(output, pipe);
> > > @@ -525,8 +526,10 @@ run_transition_test(igt_display_t *display,
> > > enum pipe pipe, igt_output_t *output
> > >  		}
> > >  
> > >  		/* force planes to be part of commit */
> > > -		for_each_plane_on_pipe(display, pipe, plane)
> > > -			igt_plane_set_position(plane, 0, 0);
> > > +		for_each_plane_on_pipe(display, pipe, plane) {
> > > +			if (parms[plane->index].mask)
> > > +				igt_plane_set_position(plane, 0, 0);
> > > +		}
> > >  
> > >  		igt_display_commit2(display, COMMIT_ATOMIC);
> > >  
> > > -- 
> > > 2.17.1
> > > 
> > 
> > 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t v4 2/2] igt/tests/kms_atomic_transition: Tolerate if can't have all planes

[Lisovskiy, Stanislav]

On Thu, 2019-04-04 at 14:56 +0200, Daniel Vetter wrote:
> On Thu, Apr 04, 2019 at 11:45:03AM +0000, Lisovskiy, Stanislav wrote:
> > On Wed, 2019-04-03 at 16:11 +0200, Daniel Vetter wrote:
> > > On Wed, Apr 03, 2019 at 03:54:50PM +0300, Stanislav Lisovskiy
> > > wrote:
> > > > With some upcoming changes i915 might not allow
> > > > all sprite planes enabled, depending on available
> > > > bandwidth limitation. Thus the test need to decrement
> > > > amount of planes and try again, instead of panicking.
> > > > 
> > > > v2: Removed excessive nested conditions, making code a bit
> > > >     more readable(hopefully).
> > > > 
> > > > v3: Stopped using global n_planes variable as it might cause
> > > >     resources being unreleased.
> > > >     Using now parms[i].mask as a way to determine if plane
> > > >     has to be included into commit.
> > > > 
> > > > v4: Removed unneeded n_overlays initialization.
> > > > 
> > > > Signed-off-by: Stanislav Lisovskiy <
> > > > stanislav.lisovskiy@intel.com>
> > > > ---
> > > >  tests/kms_atomic_transition.c | 107 +++++++++++++++++---------
> > > > ----
> > > > ----
> > > >  1 file changed, 55 insertions(+), 52 deletions(-)
> > > > 
> > > > diff --git a/tests/kms_atomic_transition.c
> > > > b/tests/kms_atomic_transition.c
> > > > index 638fe17e..a04220f3 100644
> > > > --- a/tests/kms_atomic_transition.c
> > > > +++ b/tests/kms_atomic_transition.c
> > > > @@ -212,9 +212,11 @@ static void setup_parms(igt_display_t
> > > > *display, enum pipe pipe,
> > > >  	unsigned sprite_width, sprite_height, prev_w, prev_h;
> > > >  	bool max_sprite_width, max_sprite_height, alpha = true;
> > > >  	uint32_t n_planes = display->pipes[pipe].n_planes;
> > > > -	uint32_t n_overlays = 0, overlays[n_planes];
> > > > +	uint32_t n_overlays, overlays[n_planes];
> > > >  	igt_plane_t *plane;
> > > > -	uint32_t iter_mask = 3;
> > > > +	uint32_t iter_mask;
> > > > +	int retries = n_planes - 1;
> > > > +	int ret = 0;
> > > >  
> > > >  	do_or_die(drmGetCap(display->drm_fd,
> > > > DRM_CAP_CURSOR_WIDTH,
> > > > &cursor_width));
> > > >  	if (cursor_width >= mode->hdisplay)
> > > > @@ -224,6 +226,10 @@ static void setup_parms(igt_display_t
> > > > *display, enum pipe pipe,
> > > >  	if (cursor_height >= mode->vdisplay)
> > > >  		cursor_height = mode->vdisplay;
> > > >  
> > > > +retry:
> > > > +	n_overlays = 0;
> > > > +	iter_mask = 3;
> > > > +
> > > >  	for_each_plane_on_pipe(display, pipe, plane) {
> > > >  		int i = plane->index;
> > > >  
> > > > @@ -238,12 +244,16 @@ static void setup_parms(igt_display_t
> > > > *display, enum pipe pipe,
> > > >  			parms[i].height = cursor_height;
> > > >  			parms[i].mask = 1 << 1;
> > > >  		} else {
> > > > -			parms[i].fb = sprite_fb;
> > > > -			parms[i].mask = 1 << 2;
> > > > -
> > > > -			iter_mask |= 1 << 2;
> > > > -
> > > > -			overlays[n_overlays++] = i;
> > > > +			if (n_overlays < (n_planes - 2)) {
> > > > +				parms[i].fb = sprite_fb;
> > > > +				parms[i].mask = 1 << 2;
> > > > +				iter_mask |= 1 << 2;
> > > > +				overlays[n_overlays++] = i;
> > > > +			}
> > > > +			else {
> > > > +				parms[i].fb = NULL;
> > > > +				parms[i].mask = 0;
> > > > +			}
> > > >  		}
> > > >  	}
> > > >  
> > > > @@ -278,16 +288,13 @@ static void setup_parms(igt_display_t
> > > > *display, enum pipe pipe,
> > > >  	 * Pre gen9 not all sizes are supported, find the
> > > > biggest
> > > > possible
> > > >  	 * size that can be enabled on all sprite planes.
> > > >  	 */
> > > > -retry:
> > > >  	prev_w = sprite_width = cursor_width;
> > > >  	prev_h = sprite_height = cursor_height;
> > > >  
> > > >  	max_sprite_width = (sprite_width == mode->hdisplay);
> > > >  	max_sprite_height = (sprite_height == mode->vdisplay);
> > > >  
> > > > -	while (1) {
> > > > -		int ret;
> > > > -
> > > > +	while (!max_sprite_width && !max_sprite_height) {
> > > >  		set_sprite_wh(display, pipe, parms, sprite_fb,
> > > >  			      alpha, sprite_width,
> > > > sprite_height);
> > > >  
> > > > @@ -295,54 +302,49 @@ retry:
> > > >  		ret = igt_display_try_commit_atomic(display,
> > > > DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_ATOMIC_ALLOW_MODESET,
> > > > NULL);
> > > >  		igt_assert(!is_atomic_check_failure_errno(ret))
> > > > ;
> > > >  
> > > > -		if (is_atomic_check_plane_size_errno(ret)) {
> > > > -			if (cursor_width == sprite_width &&
> > > > -			    cursor_height == sprite_height) {
> > > > -				igt_assert_f(alpha,
> > > > -					      "Cannot configure
> > > > the
> > > > test with all sprite planes enabled\n");
> > > > -
> > > > -				/* retry once with XRGB format.
> > > > */
> > > > -				alpha = false;
> > > > -				goto retry;
> > > > -			}
> > > > -
> > > > -			sprite_width = prev_w;
> > > > -			sprite_height = prev_h;
> > > > -
> > > > -			if (max_sprite_width &&
> > > > max_sprite_height) {
> > > > -				set_sprite_wh(display, pipe,
> > > > parms,
> > > > sprite_fb,
> > > > -					      alpha,
> > > > sprite_width,
> > > > sprite_height);
> > > > -				break;
> > > > -			}
> > > > -
> > > > -			if (!max_sprite_width)
> > > > -				max_sprite_width = true;
> > > > -			else
> > > > -				max_sprite_height = true;
> > > > -		} else {
> > > > +		if (!is_atomic_check_plane_size_errno(ret)) {
> > > >  			prev_w = sprite_width;
> > > >  			prev_h = sprite_height;
> > > > -		}
> > > > -
> > > > -		if (!max_sprite_width) {
> > > > -			sprite_width *= 2;
> > > > -
> > > > +    
> > > > +			sprite_width *= max_sprite_width ? 1 :
> > > > 2;
> > > >  			if (sprite_width >= mode->hdisplay) {
> > > >  				max_sprite_width = true;
> > > > -
> > > >  				sprite_width = mode->hdisplay;
> > > >  			}
> > > > -		} else if (!max_sprite_height) {
> > > > -			sprite_height *= 2;
> > > >  
> > > > +			sprite_height *= max_sprite_height ? 1
> > > > : 2;
> > > >  			if (sprite_height >= mode->vdisplay) {
> > > >  				max_sprite_height = true;
> > > > -
> > > >  				sprite_height = mode->vdisplay;
> > > >  			}
> > > > -		} else
> > > > -			/* Max sized sprites for all! */
> > > > -			break;
> > > > +			continue;
> > > > +		}
> > > > +
> > > > +		if (cursor_width == sprite_width &&
> > > > +		    cursor_height == sprite_height) {
> > > > +			igt_assert_f(retries > 0,
> > > > +				      "Cannot configure the
> > > > test with
> > > > all sprite planes enabled\n");
> > > > +			--retries;
> > > > +			/* retry once with XRGB format. */
> > > > +			if (alpha) {
> > > > +				alpha = false;
> > > > +				igt_warn("Removed alpha\n");
> > > > +			}
> > > > +			else {
> > > > +				igt_assert_f(n_planes > 1, "No
> > > > planes
> > > > left to proceed with!");
> > > > +				n_planes--;
> > > 
> > > Same comment as with kms_concurrent and all the other plane
> > > tests:
> > > Just
> > > dropping the last plane reduces our test coverage, we need to
> > > randomize
> > > which plane we drop.
> > 
> > I think that is a good idea, I'm now testing a patch which
> > randomizes
> > which one of available planes is going to be removed.
> > 
> > For the latter change I would leave for further patches as
> > currently 
> > it doesn't matter anyway, but we need the issues related to new bw
> > requirements to be fixed.
> 
> Just doing some bug scrub and we have some EINVAL failures like this
> all
> over the place. So I guess there's some other cases where we reject
> configs in the kernel and then the test falls over. We'll need this
> (it's
> how atomic is meant to work really).
> 
> Also, the bw band-aid from Ville is a really bad solution, we need
> something better - no point in having 7 planes if you can't even use
> them.
> Adding more try_commit in the wrong place feels like a detour.
> -Daniel

That's a topic for much larger discussion, I have been hearing around
for almost a month :) 
I have been talking with Ville and Martin a lot about this.
And yeah to me it is also controversial, that you have planes, but they
are not always usable, however...
I completely understand all the pros and cons, you 
are describing here. But that really goes far beyond that IGT patch.
I'm just dealing with reality what we have currently here and trying to
make it not to fail miserably at least. Once we will have "something
better" that you describe, I would be happy to implement a
correspondent changes. 

> 
> > > 
> > > The other bit: We need to recompute how many planes we can enable
> > > for
> > > every transition. Right now that's not the case with Ville's
> > > current
> > > patch
> > > series, but that's also just an interim solution. Actual bw
> > > consumption
> > > very much depends upon where all the planes are place, how big
> > > they
> > > are
> > > and all that stuff. So we need to recompute/recheck that every
> > > transition
> > > works, for each specific transition. Because the exact transition
> > > might
> > > also matter.
> > > 
> > > Or maybe I'm not entirely understanding how this test works here.
> > > -Daniel
> > > 
> > > > +				igt_warn("Reduced available
> > > > planes to
> > > > %d\n", n_planes);
> > > > +			}
> > > > +			goto retry;
> > > > +		}
> > > > +
> > > > +		sprite_width = prev_w;
> > > > +		sprite_height = prev_h;
> > > > +
> > > > +		if (!max_sprite_width)
> > > > +			max_sprite_width = true;
> > > > +		else
> > > > +			max_sprite_height = true;
> > > >  	}
> > > >  
> > > >  	igt_info("Running test on pipe %s with resolution %dx%d
> > > > and
> > > > sprite size %dx%d alpha %i\n",
> > > > @@ -463,7 +465,6 @@ run_transition_test(igt_display_t *display,
> > > > enum pipe pipe, igt_output_t *output
> > > >  
> > > >  	if (flags & DRM_MODE_ATOMIC_ALLOW_MODESET) {
> > > >  		igt_output_set_pipe(output, PIPE_NONE);
> > > > -
> > > >  		igt_display_commit2(display, COMMIT_ATOMIC);
> > > >  
> > > >  		igt_output_set_pipe(output, pipe);
> > > > @@ -525,8 +526,10 @@ run_transition_test(igt_display_t
> > > > *display,
> > > > enum pipe pipe, igt_output_t *output
> > > >  		}
> > > >  
> > > >  		/* force planes to be part of commit */
> > > > -		for_each_plane_on_pipe(display, pipe, plane)
> > > > -			igt_plane_set_position(plane, 0, 0);
> > > > +		for_each_plane_on_pipe(display, pipe, plane) {
> > > > +			if (parms[plane->index].mask)
> > > > +				igt_plane_set_position(plane,
> > > > 0, 0);
> > > > +		}
> > > >  
> > > >  		igt_display_commit2(display, COMMIT_ATOMIC);
> > > >  
> > > > -- 
> > > > 2.17.1
> > > > 
> > > 
> > > 
> 
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t v4 2/2] igt/tests/kms_atomic_transition: Tolerate if can't have all planes

[Daniel Vetter]

On Fri, Apr 05, 2019 at 07:20:15AM +0000, Lisovskiy, Stanislav wrote:
> On Thu, 2019-04-04 at 14:56 +0200, Daniel Vetter wrote:
> > On Thu, Apr 04, 2019 at 11:45:03AM +0000, Lisovskiy, Stanislav wrote:
> > > On Wed, 2019-04-03 at 16:11 +0200, Daniel Vetter wrote:
> > > > On Wed, Apr 03, 2019 at 03:54:50PM +0300, Stanislav Lisovskiy
> > > > wrote:
> > > > > With some upcoming changes i915 might not allow
> > > > > all sprite planes enabled, depending on available
> > > > > bandwidth limitation. Thus the test need to decrement
> > > > > amount of planes and try again, instead of panicking.
> > > > > 
> > > > > v2: Removed excessive nested conditions, making code a bit
> > > > >     more readable(hopefully).
> > > > > 
> > > > > v3: Stopped using global n_planes variable as it might cause
> > > > >     resources being unreleased.
> > > > >     Using now parms[i].mask as a way to determine if plane
> > > > >     has to be included into commit.
> > > > > 
> > > > > v4: Removed unneeded n_overlays initialization.
> > > > > 
> > > > > Signed-off-by: Stanislav Lisovskiy <
> > > > > stanislav.lisovskiy@intel.com>
> > > > > ---
> > > > >  tests/kms_atomic_transition.c | 107 +++++++++++++++++---------
> > > > > ----
> > > > > ----
> > > > >  1 file changed, 55 insertions(+), 52 deletions(-)
> > > > > 
> > > > > diff --git a/tests/kms_atomic_transition.c
> > > > > b/tests/kms_atomic_transition.c
> > > > > index 638fe17e..a04220f3 100644
> > > > > --- a/tests/kms_atomic_transition.c
> > > > > +++ b/tests/kms_atomic_transition.c
> > > > > @@ -212,9 +212,11 @@ static void setup_parms(igt_display_t
> > > > > *display, enum pipe pipe,
> > > > >  	unsigned sprite_width, sprite_height, prev_w, prev_h;
> > > > >  	bool max_sprite_width, max_sprite_height, alpha = true;
> > > > >  	uint32_t n_planes = display->pipes[pipe].n_planes;
> > > > > -	uint32_t n_overlays = 0, overlays[n_planes];
> > > > > +	uint32_t n_overlays, overlays[n_planes];
> > > > >  	igt_plane_t *plane;
> > > > > -	uint32_t iter_mask = 3;
> > > > > +	uint32_t iter_mask;
> > > > > +	int retries = n_planes - 1;
> > > > > +	int ret = 0;
> > > > >  
> > > > >  	do_or_die(drmGetCap(display->drm_fd,
> > > > > DRM_CAP_CURSOR_WIDTH,
> > > > > &cursor_width));
> > > > >  	if (cursor_width >= mode->hdisplay)
> > > > > @@ -224,6 +226,10 @@ static void setup_parms(igt_display_t
> > > > > *display, enum pipe pipe,
> > > > >  	if (cursor_height >= mode->vdisplay)
> > > > >  		cursor_height = mode->vdisplay;
> > > > >  
> > > > > +retry:
> > > > > +	n_overlays = 0;
> > > > > +	iter_mask = 3;
> > > > > +
> > > > >  	for_each_plane_on_pipe(display, pipe, plane) {
> > > > >  		int i = plane->index;
> > > > >  
> > > > > @@ -238,12 +244,16 @@ static void setup_parms(igt_display_t
> > > > > *display, enum pipe pipe,
> > > > >  			parms[i].height = cursor_height;
> > > > >  			parms[i].mask = 1 << 1;
> > > > >  		} else {
> > > > > -			parms[i].fb = sprite_fb;
> > > > > -			parms[i].mask = 1 << 2;
> > > > > -
> > > > > -			iter_mask |= 1 << 2;
> > > > > -
> > > > > -			overlays[n_overlays++] = i;
> > > > > +			if (n_overlays < (n_planes - 2)) {
> > > > > +				parms[i].fb = sprite_fb;
> > > > > +				parms[i].mask = 1 << 2;
> > > > > +				iter_mask |= 1 << 2;
> > > > > +				overlays[n_overlays++] = i;
> > > > > +			}
> > > > > +			else {
> > > > > +				parms[i].fb = NULL;
> > > > > +				parms[i].mask = 0;
> > > > > +			}
> > > > >  		}
> > > > >  	}
> > > > >  
> > > > > @@ -278,16 +288,13 @@ static void setup_parms(igt_display_t
> > > > > *display, enum pipe pipe,
> > > > >  	 * Pre gen9 not all sizes are supported, find the
> > > > > biggest
> > > > > possible
> > > > >  	 * size that can be enabled on all sprite planes.
> > > > >  	 */
> > > > > -retry:
> > > > >  	prev_w = sprite_width = cursor_width;
> > > > >  	prev_h = sprite_height = cursor_height;
> > > > >  
> > > > >  	max_sprite_width = (sprite_width == mode->hdisplay);
> > > > >  	max_sprite_height = (sprite_height == mode->vdisplay);
> > > > >  
> > > > > -	while (1) {
> > > > > -		int ret;
> > > > > -
> > > > > +	while (!max_sprite_width && !max_sprite_height) {
> > > > >  		set_sprite_wh(display, pipe, parms, sprite_fb,
> > > > >  			      alpha, sprite_width,
> > > > > sprite_height);
> > > > >  
> > > > > @@ -295,54 +302,49 @@ retry:
> > > > >  		ret = igt_display_try_commit_atomic(display,
> > > > > DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_ATOMIC_ALLOW_MODESET,
> > > > > NULL);
> > > > >  		igt_assert(!is_atomic_check_failure_errno(ret))
> > > > > ;
> > > > >  
> > > > > -		if (is_atomic_check_plane_size_errno(ret)) {
> > > > > -			if (cursor_width == sprite_width &&
> > > > > -			    cursor_height == sprite_height) {
> > > > > -				igt_assert_f(alpha,
> > > > > -					      "Cannot configure
> > > > > the
> > > > > test with all sprite planes enabled\n");
> > > > > -
> > > > > -				/* retry once with XRGB format.
> > > > > */
> > > > > -				alpha = false;
> > > > > -				goto retry;
> > > > > -			}
> > > > > -
> > > > > -			sprite_width = prev_w;
> > > > > -			sprite_height = prev_h;
> > > > > -
> > > > > -			if (max_sprite_width &&
> > > > > max_sprite_height) {
> > > > > -				set_sprite_wh(display, pipe,
> > > > > parms,
> > > > > sprite_fb,
> > > > > -					      alpha,
> > > > > sprite_width,
> > > > > sprite_height);
> > > > > -				break;
> > > > > -			}
> > > > > -
> > > > > -			if (!max_sprite_width)
> > > > > -				max_sprite_width = true;
> > > > > -			else
> > > > > -				max_sprite_height = true;
> > > > > -		} else {
> > > > > +		if (!is_atomic_check_plane_size_errno(ret)) {
> > > > >  			prev_w = sprite_width;
> > > > >  			prev_h = sprite_height;
> > > > > -		}
> > > > > -
> > > > > -		if (!max_sprite_width) {
> > > > > -			sprite_width *= 2;
> > > > > -
> > > > > +    
> > > > > +			sprite_width *= max_sprite_width ? 1 :
> > > > > 2;
> > > > >  			if (sprite_width >= mode->hdisplay) {
> > > > >  				max_sprite_width = true;
> > > > > -
> > > > >  				sprite_width = mode->hdisplay;
> > > > >  			}
> > > > > -		} else if (!max_sprite_height) {
> > > > > -			sprite_height *= 2;
> > > > >  
> > > > > +			sprite_height *= max_sprite_height ? 1
> > > > > : 2;
> > > > >  			if (sprite_height >= mode->vdisplay) {
> > > > >  				max_sprite_height = true;
> > > > > -
> > > > >  				sprite_height = mode->vdisplay;
> > > > >  			}
> > > > > -		} else
> > > > > -			/* Max sized sprites for all! */
> > > > > -			break;
> > > > > +			continue;
> > > > > +		}
> > > > > +
> > > > > +		if (cursor_width == sprite_width &&
> > > > > +		    cursor_height == sprite_height) {
> > > > > +			igt_assert_f(retries > 0,
> > > > > +				      "Cannot configure the
> > > > > test with
> > > > > all sprite planes enabled\n");
> > > > > +			--retries;
> > > > > +			/* retry once with XRGB format. */
> > > > > +			if (alpha) {
> > > > > +				alpha = false;
> > > > > +				igt_warn("Removed alpha\n");
> > > > > +			}
> > > > > +			else {
> > > > > +				igt_assert_f(n_planes > 1, "No
> > > > > planes
> > > > > left to proceed with!");
> > > > > +				n_planes--;
> > > > 
> > > > Same comment as with kms_concurrent and all the other plane
> > > > tests:
> > > > Just
> > > > dropping the last plane reduces our test coverage, we need to
> > > > randomize
> > > > which plane we drop.
> > > 
> > > I think that is a good idea, I'm now testing a patch which
> > > randomizes
> > > which one of available planes is going to be removed.
> > > 
> > > For the latter change I would leave for further patches as
> > > currently 
> > > it doesn't matter anyway, but we need the issues related to new bw
> > > requirements to be fixed.
> > 
> > Just doing some bug scrub and we have some EINVAL failures like this
> > all
> > over the place. So I guess there's some other cases where we reject
> > configs in the kernel and then the test falls over. We'll need this
> > (it's
> > how atomic is meant to work really).
> > 
> > Also, the bw band-aid from Ville is a really bad solution, we need
> > something better - no point in having 7 planes if you can't even use
> > them.
> > Adding more try_commit in the wrong place feels like a detour.
> > -Daniel
> 
> That's a topic for much larger discussion, I have been hearing around
> for almost a month :) 
> I have been talking with Ville and Martin a lot about this.
> And yeah to me it is also controversial, that you have planes, but they
> are not always usable, however...
> I completely understand all the pros and cons, you 
> are describing here. But that really goes far beyond that IGT patch.
> I'm just dealing with reality what we have currently here and trying to
> make it not to fail miserably at least. Once we will have "something
> better" that you describe, I would be happy to implement a
> correspondent changes. 

We _do_ have this "something better", since day 1 of atomic. It's called
try_commit.

And yes "some planes sometimes don't work for reasons only clear to the
kernel driver" has also been part of atomic since day one.

Please don't merge hacks to handle management pressure, let's do the right
thing. I'm happy to handle managers for you who don't get this. Cutting
corners here doesn't help anyone (and yes this should have been caught
when the test was originally written, but well it wasnt).
-Daniel

> 
> > 
> > > > 
> > > > The other bit: We need to recompute how many planes we can enable
> > > > for
> > > > every transition. Right now that's not the case with Ville's
> > > > current
> > > > patch
> > > > series, but that's also just an interim solution. Actual bw
> > > > consumption
> > > > very much depends upon where all the planes are place, how big
> > > > they
> > > > are
> > > > and all that stuff. So we need to recompute/recheck that every
> > > > transition
> > > > works, for each specific transition. Because the exact transition
> > > > might
> > > > also matter.
> > > > 
> > > > Or maybe I'm not entirely understanding how this test works here.
> > > > -Daniel
> > > > 
> > > > > +				igt_warn("Reduced available
> > > > > planes to
> > > > > %d\n", n_planes);
> > > > > +			}
> > > > > +			goto retry;
> > > > > +		}
> > > > > +
> > > > > +		sprite_width = prev_w;
> > > > > +		sprite_height = prev_h;
> > > > > +
> > > > > +		if (!max_sprite_width)
> > > > > +			max_sprite_width = true;
> > > > > +		else
> > > > > +			max_sprite_height = true;
> > > > >  	}
> > > > >  
> > > > >  	igt_info("Running test on pipe %s with resolution %dx%d
> > > > > and
> > > > > sprite size %dx%d alpha %i\n",
> > > > > @@ -463,7 +465,6 @@ run_transition_test(igt_display_t *display,
> > > > > enum pipe pipe, igt_output_t *output
> > > > >  
> > > > >  	if (flags & DRM_MODE_ATOMIC_ALLOW_MODESET) {
> > > > >  		igt_output_set_pipe(output, PIPE_NONE);
> > > > > -
> > > > >  		igt_display_commit2(display, COMMIT_ATOMIC);
> > > > >  
> > > > >  		igt_output_set_pipe(output, pipe);
> > > > > @@ -525,8 +526,10 @@ run_transition_test(igt_display_t
> > > > > *display,
> > > > > enum pipe pipe, igt_output_t *output
> > > > >  		}
> > > > >  
> > > > >  		/* force planes to be part of commit */
> > > > > -		for_each_plane_on_pipe(display, pipe, plane)
> > > > > -			igt_plane_set_position(plane, 0, 0);
> > > > > +		for_each_plane_on_pipe(display, pipe, plane) {
> > > > > +			if (parms[plane->index].mask)
> > > > > +				igt_plane_set_position(plane,
> > > > > 0, 0);
> > > > > +		}
> > > > >  
> > > > >  		igt_display_commit2(display, COMMIT_ATOMIC);
> > > > >  
> > > > > -- 
> > > > > 2.17.1
> > > > > 
> > > > 
> > > > 
> > 
> > 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t v4 2/2] igt/tests/kms_atomic_transition: Tolerate if can't have all planes

[Lisovskiy, Stanislav]

On Tue, 2019-04-16 at 10:52 +0200, Daniel Vetter wrote:
> On Fri, Apr 05, 2019 at 07:20:15AM +0000, Lisovskiy, Stanislav wrote:
> > On Thu, 2019-04-04 at 14:56 +0200, Daniel Vetter wrote:
> > > On Thu, Apr 04, 2019 at 11:45:03AM +0000, Lisovskiy, Stanislav
> > > wrote:
> > > > On Wed, 2019-04-03 at 16:11 +0200, Daniel Vetter wrote:
> > > > > On Wed, Apr 03, 2019 at 03:54:50PM +0300, Stanislav Lisovskiy
> > > > > wrote:
> > > > > > With some upcoming changes i915 might not allow
> > > > > > all sprite planes enabled, depending on available
> > > > > > bandwidth limitation. Thus the test need to decrement
> > > > > > amount of planes and try again, instead of panicking.
> > > > > > 
> > > > > > v2: Removed excessive nested conditions, making code a bit
> > > > > >     more readable(hopefully).
> > > > > > 
> > > > > > v3: Stopped using global n_planes variable as it might
> > > > > > cause
> > > > > >     resources being unreleased.
> > > > > >     Using now parms[i].mask as a way to determine if plane
> > > > > >     has to be included into commit.
> > > > > > 
> > > > > > v4: Removed unneeded n_overlays initialization.
> > > > > > 
> > > > > > Signed-off-by: Stanislav Lisovskiy <
> > > > > > stanislav.lisovskiy@intel.com>
> > > > > > ---
> > > > > >  tests/kms_atomic_transition.c | 107 +++++++++++++++++-----
> > > > > > ----
> > > > > > ----
> > > > > > ----
> > > > > >  1 file changed, 55 insertions(+), 52 deletions(-)
> > > > > > 
> > > > > > diff --git a/tests/kms_atomic_transition.c
> > > > > > b/tests/kms_atomic_transition.c
> > > > > > index 638fe17e..a04220f3 100644
> > > > > > --- a/tests/kms_atomic_transition.c
> > > > > > +++ b/tests/kms_atomic_transition.c
> > > > > > @@ -212,9 +212,11 @@ static void setup_parms(igt_display_t
> > > > > > *display, enum pipe pipe,
> > > > > >  	unsigned sprite_width, sprite_height, prev_w, prev_h;
> > > > > >  	bool max_sprite_width, max_sprite_height, alpha = true;
> > > > > >  	uint32_t n_planes = display->pipes[pipe].n_planes;
> > > > > > -	uint32_t n_overlays = 0, overlays[n_planes];
> > > > > > +	uint32_t n_overlays, overlays[n_planes];
> > > > > >  	igt_plane_t *plane;
> > > > > > -	uint32_t iter_mask = 3;
> > > > > > +	uint32_t iter_mask;
> > > > > > +	int retries = n_planes - 1;
> > > > > > +	int ret = 0;
> > > > > >  
> > > > > >  	do_or_die(drmGetCap(display->drm_fd,
> > > > > > DRM_CAP_CURSOR_WIDTH,
> > > > > > &cursor_width));
> > > > > >  	if (cursor_width >= mode->hdisplay)
> > > > > > @@ -224,6 +226,10 @@ static void setup_parms(igt_display_t
> > > > > > *display, enum pipe pipe,
> > > > > >  	if (cursor_height >= mode->vdisplay)
> > > > > >  		cursor_height = mode->vdisplay;
> > > > > >  
> > > > > > +retry:
> > > > > > +	n_overlays = 0;
> > > > > > +	iter_mask = 3;
> > > > > > +
> > > > > >  	for_each_plane_on_pipe(display, pipe, plane) {
> > > > > >  		int i = plane->index;
> > > > > >  
> > > > > > @@ -238,12 +244,16 @@ static void setup_parms(igt_display_t
> > > > > > *display, enum pipe pipe,
> > > > > >  			parms[i].height = cursor_height;
> > > > > >  			parms[i].mask = 1 << 1;
> > > > > >  		} else {
> > > > > > -			parms[i].fb = sprite_fb;
> > > > > > -			parms[i].mask = 1 << 2;
> > > > > > -
> > > > > > -			iter_mask |= 1 << 2;
> > > > > > -
> > > > > > -			overlays[n_overlays++] = i;
> > > > > > +			if (n_overlays < (n_planes - 2)) {
> > > > > > +				parms[i].fb = sprite_fb;
> > > > > > +				parms[i].mask = 1 << 2;
> > > > > > +				iter_mask |= 1 << 2;
> > > > > > +				overlays[n_overlays++] = i;
> > > > > > +			}
> > > > > > +			else {
> > > > > > +				parms[i].fb = NULL;
> > > > > > +				parms[i].mask = 0;
> > > > > > +			}
> > > > > >  		}
> > > > > >  	}
> > > > > >  
> > > > > > @@ -278,16 +288,13 @@ static void setup_parms(igt_display_t
> > > > > > *display, enum pipe pipe,
> > > > > >  	 * Pre gen9 not all sizes are supported, find the
> > > > > > biggest
> > > > > > possible
> > > > > >  	 * size that can be enabled on all sprite planes.
> > > > > >  	 */
> > > > > > -retry:
> > > > > >  	prev_w = sprite_width = cursor_width;
> > > > > >  	prev_h = sprite_height = cursor_height;
> > > > > >  
> > > > > >  	max_sprite_width = (sprite_width == mode->hdisplay);
> > > > > >  	max_sprite_height = (sprite_height == mode->vdisplay);
> > > > > >  
> > > > > > -	while (1) {
> > > > > > -		int ret;
> > > > > > -
> > > > > > +	while (!max_sprite_width && !max_sprite_height) {
> > > > > >  		set_sprite_wh(display, pipe, parms, sprite_fb,
> > > > > >  			      alpha, sprite_width,
> > > > > > sprite_height);
> > > > > >  
> > > > > > @@ -295,54 +302,49 @@ retry:
> > > > > >  		ret = igt_display_try_commit_atomic(display,
> > > > > > DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_ATOMIC_ALLOW_MODESET,
> > > > > > NULL);
> > > > > >  		igt_assert(!is_atomic_check_failure_errno(ret))
> > > > > > ;
> > > > > >  
> > > > > > -		if (is_atomic_check_plane_size_errno(ret)) {
> > > > > > -			if (cursor_width == sprite_width &&
> > > > > > -			    cursor_height == sprite_height) {
> > > > > > -				igt_assert_f(alpha,
> > > > > > -					      "Cannot configure
> > > > > > the
> > > > > > test with all sprite planes enabled\n");
> > > > > > -
> > > > > > -				/* retry once with XRGB format.
> > > > > > */
> > > > > > -				alpha = false;
> > > > > > -				goto retry;
> > > > > > -			}
> > > > > > -
> > > > > > -			sprite_width = prev_w;
> > > > > > -			sprite_height = prev_h;
> > > > > > -
> > > > > > -			if (max_sprite_width &&
> > > > > > max_sprite_height) {
> > > > > > -				set_sprite_wh(display, pipe,
> > > > > > parms,
> > > > > > sprite_fb,
> > > > > > -					      alpha,
> > > > > > sprite_width,
> > > > > > sprite_height);
> > > > > > -				break;
> > > > > > -			}
> > > > > > -
> > > > > > -			if (!max_sprite_width)
> > > > > > -				max_sprite_width = true;
> > > > > > -			else
> > > > > > -				max_sprite_height = true;
> > > > > > -		} else {
> > > > > > +		if (!is_atomic_check_plane_size_errno(ret)) {
> > > > > >  			prev_w = sprite_width;
> > > > > >  			prev_h = sprite_height;
> > > > > > -		}
> > > > > > -
> > > > > > -		if (!max_sprite_width) {
> > > > > > -			sprite_width *= 2;
> > > > > > -
> > > > > > +    
> > > > > > +			sprite_width *= max_sprite_width ? 1 :
> > > > > > 2;
> > > > > >  			if (sprite_width >= mode->hdisplay) {
> > > > > >  				max_sprite_width = true;
> > > > > > -
> > > > > >  				sprite_width = mode->hdisplay;
> > > > > >  			}
> > > > > > -		} else if (!max_sprite_height) {
> > > > > > -			sprite_height *= 2;
> > > > > >  
> > > > > > +			sprite_height *= max_sprite_height ? 1
> > > > > > : 2;
> > > > > >  			if (sprite_height >= mode->vdisplay) {
> > > > > >  				max_sprite_height = true;
> > > > > > -
> > > > > >  				sprite_height = mode->vdisplay;
> > > > > >  			}
> > > > > > -		} else
> > > > > > -			/* Max sized sprites for all! */
> > > > > > -			break;
> > > > > > +			continue;
> > > > > > +		}
> > > > > > +
> > > > > > +		if (cursor_width == sprite_width &&
> > > > > > +		    cursor_height == sprite_height) {
> > > > > > +			igt_assert_f(retries > 0,
> > > > > > +				      "Cannot configure the
> > > > > > test with
> > > > > > all sprite planes enabled\n");
> > > > > > +			--retries;
> > > > > > +			/* retry once with XRGB format. */
> > > > > > +			if (alpha) {
> > > > > > +				alpha = false;
> > > > > > +				igt_warn("Removed alpha\n");
> > > > > > +			}
> > > > > > +			else {
> > > > > > +				igt_assert_f(n_planes > 1, "No
> > > > > > planes
> > > > > > left to proceed with!");
> > > > > > +				n_planes--;
> > > > > 
> > > > > Same comment as with kms_concurrent and all the other plane
> > > > > tests:
> > > > > Just
> > > > > dropping the last plane reduces our test coverage, we need to
> > > > > randomize
> > > > > which plane we drop.
> > > > 
> > > > I think that is a good idea, I'm now testing a patch which
> > > > randomizes
> > > > which one of available planes is going to be removed.
> > > > 
> > > > For the latter change I would leave for further patches as
> > > > currently 
> > > > it doesn't matter anyway, but we need the issues related to new
> > > > bw
> > > > requirements to be fixed.
> > > 
> > > Just doing some bug scrub and we have some EINVAL failures like
> > > this
> > > all
> > > over the place. So I guess there's some other cases where we
> > > reject
> > > configs in the kernel and then the test falls over. We'll need
> > > this
> > > (it's
> > > how atomic is meant to work really).
> > > 
> > > Also, the bw band-aid from Ville is a really bad solution, we
> > > need
> > > something better - no point in having 7 planes if you can't even
> > > use
> > > them.
> > > Adding more try_commit in the wrong place feels like a detour.
> > > -Daniel
> > 
> > That's a topic for much larger discussion, I have been hearing
> > around
> > for almost a month :) 
> > I have been talking with Ville and Martin a lot about this.
> > And yeah to me it is also controversial, that you have planes, but
> > they
> > are not always usable, however...
> > I completely understand all the pros and cons, you 
> > are describing here. But that really goes far beyond that IGT
> > patch.
> > I'm just dealing with reality what we have currently here and
> > trying to
> > make it not to fail miserably at least. Once we will have
> > "something
> > better" that you describe, I would be happy to implement a
> > correspondent changes. 
> 
> We _do_ have this "something better", since day 1 of atomic. It's
> called
> try_commit.
> 
> And yes "some planes sometimes don't work for reasons only clear to
> the
> kernel driver" has also been part of atomic since day one.

Well, thats is what most of those fixes are about - we just change test
case to query first, if that config is going to work, before actually
committing it. If it doesn't work we try to reduce planes number or
their size. So what do you exactly mean or don't like here? 

> 
> Please don't merge hacks to handle management pressure, let's do the
> right
> thing. I'm happy to handle managers for you who don't get this.
> Cutting
> corners here doesn't help anyone (and yes this should have been
> caught
> when the test was originally written, but well it wasnt).

Well, I can't literally "merge" anything. What I'm doing is changing 
patches to match new bw paradigm. Have you seen bw patches?
Due those changes we now have -EINVAL returned, whenever we don't have
resources(kms_concurrent, kms_atomic_transition, kms_plane_multiple..).
So as you have said we need to use try_commit to understand
first, if configuration is going to work or not. I'm doing that.

As I understand also BW is not something, we have any other solutions
for currently.

If you are not happy with it, I understand that - but as you
understand, I'm definitely not the first guy whom you should refer to
here.

> -Daniel
> 
> > 
> > > 
> > > > > 
> > > > > The other bit: We need to recompute how many planes we can
> > > > > enable
> > > > > for
> > > > > every transition. Right now that's not the case with Ville's
> > > > > current
> > > > > patch
> > > > > series, but that's also just an interim solution. Actual bw
> > > > > consumption
> > > > > very much depends upon where all the planes are place, how
> > > > > big
> > > > > they
> > > > > are
> > > > > and all that stuff. So we need to recompute/recheck that
> > > > > every
> > > > > transition
> > > > > works, for each specific transition. Because the exact
> > > > > transition
> > > > > might
> > > > > also matter.
> > > > > 
> > > > > Or maybe I'm not entirely understanding how this test works
> > > > > here.
> > > > > -Daniel
> > > > > 
> > > > > > +				igt_warn("Reduced available
> > > > > > planes to
> > > > > > %d\n", n_planes);
> > > > > > +			}
> > > > > > +			goto retry;
> > > > > > +		}
> > > > > > +
> > > > > > +		sprite_width = prev_w;
> > > > > > +		sprite_height = prev_h;
> > > > > > +
> > > > > > +		if (!max_sprite_width)
> > > > > > +			max_sprite_width = true;
> > > > > > +		else
> > > > > > +			max_sprite_height = true;
> > > > > >  	}
> > > > > >  
> > > > > >  	igt_info("Running test on pipe %s with resolution %dx%d
> > > > > > and
> > > > > > sprite size %dx%d alpha %i\n",
> > > > > > @@ -463,7 +465,6 @@ run_transition_test(igt_display_t
> > > > > > *display,
> > > > > > enum pipe pipe, igt_output_t *output
> > > > > >  
> > > > > >  	if (flags & DRM_MODE_ATOMIC_ALLOW_MODESET) {
> > > > > >  		igt_output_set_pipe(output, PIPE_NONE);
> > > > > > -
> > > > > >  		igt_display_commit2(display, COMMIT_ATOMIC);
> > > > > >  
> > > > > >  		igt_output_set_pipe(output, pipe);
> > > > > > @@ -525,8 +526,10 @@ run_transition_test(igt_display_t
> > > > > > *display,
> > > > > > enum pipe pipe, igt_output_t *output
> > > > > >  		}
> > > > > >  
> > > > > >  		/* force planes to be part of commit */
> > > > > > -		for_each_plane_on_pipe(display, pipe, plane)
> > > > > > -			igt_plane_set_position(plane, 0, 0);
> > > > > > +		for_each_plane_on_pipe(display, pipe, plane) {
> > > > > > +			if (parms[plane->index].mask)
> > > > > > +				igt_plane_set_position(plane,
> > > > > > 0, 0);
> > > > > > +		}
> > > > > >  
> > > > > >  		igt_display_commit2(display, COMMIT_ATOMIC);
> > > > > >  
> > > > > > -- 
> > > > > > 2.17.1
> > > > > > 
> > > > > 
> > > > > 
> > > 
> > > 
> 
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t v4 2/2] igt/tests/kms_atomic_transition: Tolerate if can't have all planes

[Daniel Vetter]

On Tue, Apr 16, 2019 at 09:07:52AM +0000, Lisovskiy, Stanislav wrote:
> On Tue, 2019-04-16 at 10:52 +0200, Daniel Vetter wrote:
> > On Fri, Apr 05, 2019 at 07:20:15AM +0000, Lisovskiy, Stanislav wrote:
> > > On Thu, 2019-04-04 at 14:56 +0200, Daniel Vetter wrote:
> > > > On Thu, Apr 04, 2019 at 11:45:03AM +0000, Lisovskiy, Stanislav
> > > > wrote:
> > > > > On Wed, 2019-04-03 at 16:11 +0200, Daniel Vetter wrote:
> > > > > > On Wed, Apr 03, 2019 at 03:54:50PM +0300, Stanislav Lisovskiy
> > > > > > wrote:
> > > > > > > With some upcoming changes i915 might not allow
> > > > > > > all sprite planes enabled, depending on available
> > > > > > > bandwidth limitation. Thus the test need to decrement
> > > > > > > amount of planes and try again, instead of panicking.
> > > > > > > 
> > > > > > > v2: Removed excessive nested conditions, making code a bit
> > > > > > >     more readable(hopefully).
> > > > > > > 
> > > > > > > v3: Stopped using global n_planes variable as it might
> > > > > > > cause
> > > > > > >     resources being unreleased.
> > > > > > >     Using now parms[i].mask as a way to determine if plane
> > > > > > >     has to be included into commit.
> > > > > > > 
> > > > > > > v4: Removed unneeded n_overlays initialization.
> > > > > > > 
> > > > > > > Signed-off-by: Stanislav Lisovskiy <
> > > > > > > stanislav.lisovskiy@intel.com>
> > > > > > > ---
> > > > > > >  tests/kms_atomic_transition.c | 107 +++++++++++++++++-----
> > > > > > > ----
> > > > > > > ----
> > > > > > > ----
> > > > > > >  1 file changed, 55 insertions(+), 52 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/tests/kms_atomic_transition.c
> > > > > > > b/tests/kms_atomic_transition.c
> > > > > > > index 638fe17e..a04220f3 100644
> > > > > > > --- a/tests/kms_atomic_transition.c
> > > > > > > +++ b/tests/kms_atomic_transition.c
> > > > > > > @@ -212,9 +212,11 @@ static void setup_parms(igt_display_t
> > > > > > > *display, enum pipe pipe,
> > > > > > >  	unsigned sprite_width, sprite_height, prev_w, prev_h;
> > > > > > >  	bool max_sprite_width, max_sprite_height, alpha = true;
> > > > > > >  	uint32_t n_planes = display->pipes[pipe].n_planes;
> > > > > > > -	uint32_t n_overlays = 0, overlays[n_planes];
> > > > > > > +	uint32_t n_overlays, overlays[n_planes];
> > > > > > >  	igt_plane_t *plane;
> > > > > > > -	uint32_t iter_mask = 3;
> > > > > > > +	uint32_t iter_mask;
> > > > > > > +	int retries = n_planes - 1;
> > > > > > > +	int ret = 0;
> > > > > > >  
> > > > > > >  	do_or_die(drmGetCap(display->drm_fd,
> > > > > > > DRM_CAP_CURSOR_WIDTH,
> > > > > > > &cursor_width));
> > > > > > >  	if (cursor_width >= mode->hdisplay)
> > > > > > > @@ -224,6 +226,10 @@ static void setup_parms(igt_display_t
> > > > > > > *display, enum pipe pipe,
> > > > > > >  	if (cursor_height >= mode->vdisplay)
> > > > > > >  		cursor_height = mode->vdisplay;
> > > > > > >  
> > > > > > > +retry:
> > > > > > > +	n_overlays = 0;
> > > > > > > +	iter_mask = 3;
> > > > > > > +
> > > > > > >  	for_each_plane_on_pipe(display, pipe, plane) {
> > > > > > >  		int i = plane->index;
> > > > > > >  
> > > > > > > @@ -238,12 +244,16 @@ static void setup_parms(igt_display_t
> > > > > > > *display, enum pipe pipe,
> > > > > > >  			parms[i].height = cursor_height;
> > > > > > >  			parms[i].mask = 1 << 1;
> > > > > > >  		} else {
> > > > > > > -			parms[i].fb = sprite_fb;
> > > > > > > -			parms[i].mask = 1 << 2;
> > > > > > > -
> > > > > > > -			iter_mask |= 1 << 2;
> > > > > > > -
> > > > > > > -			overlays[n_overlays++] = i;
> > > > > > > +			if (n_overlays < (n_planes - 2)) {
> > > > > > > +				parms[i].fb = sprite_fb;
> > > > > > > +				parms[i].mask = 1 << 2;
> > > > > > > +				iter_mask |= 1 << 2;
> > > > > > > +				overlays[n_overlays++] = i;
> > > > > > > +			}
> > > > > > > +			else {
> > > > > > > +				parms[i].fb = NULL;
> > > > > > > +				parms[i].mask = 0;
> > > > > > > +			}
> > > > > > >  		}
> > > > > > >  	}
> > > > > > >  
> > > > > > > @@ -278,16 +288,13 @@ static void setup_parms(igt_display_t
> > > > > > > *display, enum pipe pipe,
> > > > > > >  	 * Pre gen9 not all sizes are supported, find the
> > > > > > > biggest
> > > > > > > possible
> > > > > > >  	 * size that can be enabled on all sprite planes.
> > > > > > >  	 */
> > > > > > > -retry:
> > > > > > >  	prev_w = sprite_width = cursor_width;
> > > > > > >  	prev_h = sprite_height = cursor_height;
> > > > > > >  
> > > > > > >  	max_sprite_width = (sprite_width == mode->hdisplay);
> > > > > > >  	max_sprite_height = (sprite_height == mode->vdisplay);
> > > > > > >  
> > > > > > > -	while (1) {
> > > > > > > -		int ret;
> > > > > > > -
> > > > > > > +	while (!max_sprite_width && !max_sprite_height) {
> > > > > > >  		set_sprite_wh(display, pipe, parms, sprite_fb,
> > > > > > >  			      alpha, sprite_width,
> > > > > > > sprite_height);
> > > > > > >  
> > > > > > > @@ -295,54 +302,49 @@ retry:
> > > > > > >  		ret = igt_display_try_commit_atomic(display,
> > > > > > > DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_ATOMIC_ALLOW_MODESET,
> > > > > > > NULL);
> > > > > > >  		igt_assert(!is_atomic_check_failure_errno(ret))
> > > > > > > ;
> > > > > > >  
> > > > > > > -		if (is_atomic_check_plane_size_errno(ret)) {
> > > > > > > -			if (cursor_width == sprite_width &&
> > > > > > > -			    cursor_height == sprite_height) {
> > > > > > > -				igt_assert_f(alpha,
> > > > > > > -					      "Cannot configure
> > > > > > > the
> > > > > > > test with all sprite planes enabled\n");
> > > > > > > -
> > > > > > > -				/* retry once with XRGB format.
> > > > > > > */
> > > > > > > -				alpha = false;
> > > > > > > -				goto retry;
> > > > > > > -			}
> > > > > > > -
> > > > > > > -			sprite_width = prev_w;
> > > > > > > -			sprite_height = prev_h;
> > > > > > > -
> > > > > > > -			if (max_sprite_width &&
> > > > > > > max_sprite_height) {
> > > > > > > -				set_sprite_wh(display, pipe,
> > > > > > > parms,
> > > > > > > sprite_fb,
> > > > > > > -					      alpha,
> > > > > > > sprite_width,
> > > > > > > sprite_height);
> > > > > > > -				break;
> > > > > > > -			}
> > > > > > > -
> > > > > > > -			if (!max_sprite_width)
> > > > > > > -				max_sprite_width = true;
> > > > > > > -			else
> > > > > > > -				max_sprite_height = true;
> > > > > > > -		} else {
> > > > > > > +		if (!is_atomic_check_plane_size_errno(ret)) {
> > > > > > >  			prev_w = sprite_width;
> > > > > > >  			prev_h = sprite_height;
> > > > > > > -		}
> > > > > > > -
> > > > > > > -		if (!max_sprite_width) {
> > > > > > > -			sprite_width *= 2;
> > > > > > > -
> > > > > > > +    
> > > > > > > +			sprite_width *= max_sprite_width ? 1 :
> > > > > > > 2;
> > > > > > >  			if (sprite_width >= mode->hdisplay) {
> > > > > > >  				max_sprite_width = true;
> > > > > > > -
> > > > > > >  				sprite_width = mode->hdisplay;
> > > > > > >  			}
> > > > > > > -		} else if (!max_sprite_height) {
> > > > > > > -			sprite_height *= 2;
> > > > > > >  
> > > > > > > +			sprite_height *= max_sprite_height ? 1
> > > > > > > : 2;
> > > > > > >  			if (sprite_height >= mode->vdisplay) {
> > > > > > >  				max_sprite_height = true;
> > > > > > > -
> > > > > > >  				sprite_height = mode->vdisplay;
> > > > > > >  			}
> > > > > > > -		} else
> > > > > > > -			/* Max sized sprites for all! */
> > > > > > > -			break;
> > > > > > > +			continue;
> > > > > > > +		}
> > > > > > > +
> > > > > > > +		if (cursor_width == sprite_width &&
> > > > > > > +		    cursor_height == sprite_height) {
> > > > > > > +			igt_assert_f(retries > 0,
> > > > > > > +				      "Cannot configure the
> > > > > > > test with
> > > > > > > all sprite planes enabled\n");
> > > > > > > +			--retries;
> > > > > > > +			/* retry once with XRGB format. */
> > > > > > > +			if (alpha) {
> > > > > > > +				alpha = false;
> > > > > > > +				igt_warn("Removed alpha\n");
> > > > > > > +			}
> > > > > > > +			else {
> > > > > > > +				igt_assert_f(n_planes > 1, "No
> > > > > > > planes
> > > > > > > left to proceed with!");
> > > > > > > +				n_planes--;
> > > > > > 
> > > > > > Same comment as with kms_concurrent and all the other plane
> > > > > > tests:
> > > > > > Just
> > > > > > dropping the last plane reduces our test coverage, we need to
> > > > > > randomize
> > > > > > which plane we drop.
> > > > > 
> > > > > I think that is a good idea, I'm now testing a patch which
> > > > > randomizes
> > > > > which one of available planes is going to be removed.
> > > > > 
> > > > > For the latter change I would leave for further patches as
> > > > > currently 
> > > > > it doesn't matter anyway, but we need the issues related to new
> > > > > bw
> > > > > requirements to be fixed.
> > > > 
> > > > Just doing some bug scrub and we have some EINVAL failures like
> > > > this
> > > > all
> > > > over the place. So I guess there's some other cases where we
> > > > reject
> > > > configs in the kernel and then the test falls over. We'll need
> > > > this
> > > > (it's
> > > > how atomic is meant to work really).
> > > > 
> > > > Also, the bw band-aid from Ville is a really bad solution, we
> > > > need
> > > > something better - no point in having 7 planes if you can't even
> > > > use
> > > > them.
> > > > Adding more try_commit in the wrong place feels like a detour.
> > > > -Daniel
> > > 
> > > That's a topic for much larger discussion, I have been hearing
> > > around
> > > for almost a month :) 
> > > I have been talking with Ville and Martin a lot about this.
> > > And yeah to me it is also controversial, that you have planes, but
> > > they
> > > are not always usable, however...
> > > I completely understand all the pros and cons, you 
> > > are describing here. But that really goes far beyond that IGT
> > > patch.
> > > I'm just dealing with reality what we have currently here and
> > > trying to
> > > make it not to fail miserably at least. Once we will have
> > > "something
> > > better" that you describe, I would be happy to implement a
> > > correspondent changes. 
> > 
> > We _do_ have this "something better", since day 1 of atomic. It's
> > called
> > try_commit.
> > 
> > And yes "some planes sometimes don't work for reasons only clear to
> > the
> > kernel driver" has also been part of atomic since day one.
> 
> Well, thats is what most of those fixes are about - we just change test
> case to query first, if that config is going to work, before actually
> committing it. If it doesn't work we try to reduce planes number or
> their size. So what do you exactly mean or don't like here? 
> 
> > 
> > Please don't merge hacks to handle management pressure, let's do the
> > right
> > thing. I'm happy to handle managers for you who don't get this.
> > Cutting
> > corners here doesn't help anyone (and yes this should have been
> > caught
> > when the test was originally written, but well it wasnt).
> 
> Well, I can't literally "merge" anything. What I'm doing is changing 
> patches to match new bw paradigm. Have you seen bw patches?
> Due those changes we now have -EINVAL returned, whenever we don't have
> resources(kms_concurrent, kms_atomic_transition, kms_plane_multiple..).
> So as you have said we need to use try_commit to understand
> first, if configuration is going to work or not. I'm doing that.
> 
> As I understand also BW is not something, we have any other solutions
> for currently.
> 
> If you are not happy with it, I understand that - but as you
> understand, I'm definitely not the first guy whom you should refer to
> here.

Maybe there's a misunderstanding. What this patch does here is figure out
how many planes can be used at most using try-commit. But ones it does
that, it also assumes that any combination of these planes (no matter
which dest rectangles we end up picking for them) will work out.

This again encodes an assumption about how i915.ko works, like the
previous assumption that we can use all planes.

What I'd like to see, and how atomic is specified, is that _any_ change in
the configuration (dst/src, pixel format, fb stride, size, tiling, really
anything) could flip the configuration from "works" to "doesn't work".
Worse, the actual transition (i.e. the previous state) also matters. Hence
the usual way an atomic commit should work is:

while {useful_config_left} {
	build_config();
	ret = try_commit();

	if (ret == -EINVAL || ret == -ERANGE) {
		reduce configs
	} else {
		break;
	}
}

render_stuff();
commit();

Iow _every_ commit needs to first figure out what's possible. And this is
what we need to do for the more randomized coverage tests that try to make
sure corner cases work. More feature specific (and hence limited) tests
can make more assumption.

Hopefully that explains better what I mean here.

This is relevant for BW calculation since on most hw it matters whether
planes are close together (as measured in horizontal scan lines) or not.
I'm assuming that intel hw is similar here, and Ville already said he'll
try to figure out what's possible and where the real limits are. E.g. if
you use all planes as horizontal bars, we should be able to use all 7. But
we can't use all 7 if you split the screen into vertical bars.
-Daniel

> 
> > -Daniel
> > 
> > > 
> > > > 
> > > > > > 
> > > > > > The other bit: We need to recompute how many planes we can
> > > > > > enable
> > > > > > for
> > > > > > every transition. Right now that's not the case with Ville's
> > > > > > current
> > > > > > patch
> > > > > > series, but that's also just an interim solution. Actual bw
> > > > > > consumption
> > > > > > very much depends upon where all the planes are place, how
> > > > > > big
> > > > > > they
> > > > > > are
> > > > > > and all that stuff. So we need to recompute/recheck that
> > > > > > every
> > > > > > transition
> > > > > > works, for each specific transition. Because the exact
> > > > > > transition
> > > > > > might
> > > > > > also matter.
> > > > > > 
> > > > > > Or maybe I'm not entirely understanding how this test works
> > > > > > here.
> > > > > > -Daniel
> > > > > > 
> > > > > > > +				igt_warn("Reduced available
> > > > > > > planes to
> > > > > > > %d\n", n_planes);
> > > > > > > +			}
> > > > > > > +			goto retry;
> > > > > > > +		}
> > > > > > > +
> > > > > > > +		sprite_width = prev_w;
> > > > > > > +		sprite_height = prev_h;
> > > > > > > +
> > > > > > > +		if (!max_sprite_width)
> > > > > > > +			max_sprite_width = true;
> > > > > > > +		else
> > > > > > > +			max_sprite_height = true;
> > > > > > >  	}
> > > > > > >  
> > > > > > >  	igt_info("Running test on pipe %s with resolution %dx%d
> > > > > > > and
> > > > > > > sprite size %dx%d alpha %i\n",
> > > > > > > @@ -463,7 +465,6 @@ run_transition_test(igt_display_t
> > > > > > > *display,
> > > > > > > enum pipe pipe, igt_output_t *output
> > > > > > >  
> > > > > > >  	if (flags & DRM_MODE_ATOMIC_ALLOW_MODESET) {
> > > > > > >  		igt_output_set_pipe(output, PIPE_NONE);
> > > > > > > -
> > > > > > >  		igt_display_commit2(display, COMMIT_ATOMIC);
> > > > > > >  
> > > > > > >  		igt_output_set_pipe(output, pipe);
> > > > > > > @@ -525,8 +526,10 @@ run_transition_test(igt_display_t
> > > > > > > *display,
> > > > > > > enum pipe pipe, igt_output_t *output
> > > > > > >  		}
> > > > > > >  
> > > > > > >  		/* force planes to be part of commit */
> > > > > > > -		for_each_plane_on_pipe(display, pipe, plane)
> > > > > > > -			igt_plane_set_position(plane, 0, 0);
> > > > > > > +		for_each_plane_on_pipe(display, pipe, plane) {
> > > > > > > +			if (parms[plane->index].mask)
> > > > > > > +				igt_plane_set_position(plane,
> > > > > > > 0, 0);
> > > > > > > +		}
> > > > > > >  
> > > > > > >  		igt_display_commit2(display, COMMIT_ATOMIC);
> > > > > > >  
> > > > > > > -- 
> > > > > > > 2.17.1
> > > > > > > 
> > > > > > 
> > > > > > 
> > > > 
> > > > 
> > 
> > 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t v4 2/2] igt/tests/kms_atomic_transition: Tolerate if can't have all planes

[Lisovskiy, Stanislav]

On Tue, 2019-04-16 at 14:27 +0200, Daniel Vetter wrote:
> On Tue, Apr 16, 2019 at 09:07:52AM +0000, Lisovskiy, Stanislav wrote:
> > On Tue, 2019-04-16 at 10:52 +0200, Daniel Vetter wrote:
> > > On Fri, Apr 05, 2019 at 07:20:15AM +0000, Lisovskiy, Stanislav
> > > wrote:
> > > > On Thu, 2019-04-04 at 14:56 +0200, Daniel Vetter wrote:
> > > > > On Thu, Apr 04, 2019 at 11:45:03AM +0000, Lisovskiy,
> > > > > Stanislav
> > > > > wrote:
> > > > > > On Wed, 2019-04-03 at 16:11 +0200, Daniel Vetter wrote:
> > > > > > > On Wed, Apr 03, 2019 at 03:54:50PM +0300, Stanislav
> > > > > > > Lisovskiy
> > > > > > > wrote:
> > > > > > > > With some upcoming changes i915 might not allow
> > > > > > > > all sprite planes enabled, depending on available
> > > > > > > > bandwidth limitation. Thus the test need to decrement
> > > > > > > > amount of planes and try again, instead of panicking.
> > > > > > > > 
> > > > > > > > v2: Removed excessive nested conditions, making code a
> > > > > > > > bit
> > > > > > > >     more readable(hopefully).
> > > > > > > > 
> > > > > > > > v3: Stopped using global n_planes variable as it might
> > > > > > > > cause
> > > > > > > >     resources being unreleased.
> > > > > > > >     Using now parms[i].mask as a way to determine if
> > > > > > > > plane
> > > > > > > >     has to be included into commit.
> > > > > > > > 
> > > > > > > > v4: Removed unneeded n_overlays initialization.
> > > > > > > > 
> > > > > > > > Signed-off-by: Stanislav Lisovskiy <
> > > > > > > > stanislav.lisovskiy@intel.com>
> > > > > > > > ---
> > > > > > > >  tests/kms_atomic_transition.c | 107 +++++++++++++++++-
> > > > > > > > ----
> > > > > > > > ----
> > > > > > > > ----
> > > > > > > > ----
> > > > > > > >  1 file changed, 55 insertions(+), 52 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/tests/kms_atomic_transition.c
> > > > > > > > b/tests/kms_atomic_transition.c
> > > > > > > > index 638fe17e..a04220f3 100644
> > > > > > > > --- a/tests/kms_atomic_transition.c
> > > > > > > > +++ b/tests/kms_atomic_transition.c
> > > > > > > > @@ -212,9 +212,11 @@ static void
> > > > > > > > setup_parms(igt_display_t
> > > > > > > > *display, enum pipe pipe,
> > > > > > > > 
> 
> Maybe there's a misunderstanding. What this patch does here is figure
> out
> how many planes can be used at most using try-commit. But ones it
> does
> that, it also assumes that any combination of these planes (no matter
> which dest rectangles we end up picking for them) will work out.
> 
> This again encodes an assumption about how i915.ko works, like the
> previous assumption that we can use all planes.
> 
> What I'd like to see, and how atomic is specified, is that _any_
> change in
> the configuration (dst/src, pixel format, fb stride, size, tiling,
> really
> anything) could flip the configuration from "works" to "doesn't
> work".
> Worse, the actual transition (i.e. the previous state) also matters.
> Hence
> the usual way an atomic commit should work is:
> 
> while {useful_config_left} {
> 	build_config();
> 	ret = try_commit();
> 
> 	if (ret == -EINVAL || ret == -ERANGE) {
> 		reduce configs
> 	} else {
> 		break;
> 	}
> }
> 
> render_stuff();
> commit();
> 
> Iow _every_ commit needs to first figure out what's possible. And
> this is
> what we need to do for the more randomized coverage tests that try to
> make
> sure corner cases work. More feature specific (and hence limited)
> tests
> can make more assumption.
> 
> Hopefully that explains better what I mean here.

I agree that we need such test, kms_atomic_transition now at least
tries to randomize which plane will be thrown away, once we run
into bw limits.
I'm also writing now a stress test case, which attempts to use allpipes/planes simultaneously with gpu/cpu load. 
However it doesn't have a randomization, but only tries to enable
everything it can simultaneously at least up to the limits when
try_commit fails. 

So do you think we should add this format/tiling randomized
configuration testing to kms_atomic_transition or have some other test
case for that?

> 
> This is relevant for BW calculation since on most hw it matters
> whether
> planes are close together (as measured in horizontal scan lines) or
> not.
> I'm assuming that intel hw is similar here, and Ville already said
> he'll
> try to figure out what's possible and where the real limits are. E.g.
> if
> you use all planes as horizontal bars, we should be able to use all
> 7. But
> we can't use all 7 if you split the screen into vertical bars.
> -Daniel
> 
> > 
> > > -Daniel
> > > 
> > > > 
> > > > > 
> > > > > > > 
> > > > > > > The other bit: We need to recompute how many planes we
> > > > > > > can
> > > > > > > enable
> > > > > > > for
> > > > > > > every transition. Right now that's not the case with
> > > > > > > Ville's
> > > > > > > current
> > > > > > > patch
> > > > > > > series, but that's also just an interim solution. Actual
> > > > > > > bw
> > > > > > > consumption
> > > > > > > very much depends upon where all the planes are place,
> > > > > > > how
> > > > > > > big
> > > > > > > they
> > > > > > > are
> > > > > > > and all that stuff. So we need to recompute/recheck that
> > > > > > > every
> > > > > > > transition
> > > > > > > works, for each specific transition. Because the exact
> > > > > > > transition
> > > > > > > might
> > > > > > > also matter.
> > > > > > > 
> > > > > > > Or maybe I'm not entirely understanding how this test
> > > > > > > works
> > > > > > > here.
> > > > > > > -Daniel
> > > > > > > 
> > > > > > > > +				igt_warn("Reduced
> > > > > > > > available
> > > > > > > > planes to
> > > > > > > > %d\n", n_planes);
> > > > > > > > +			}
> > > > > > > > +			goto retry;
> > > > > > > > +		}
> > > > > > > > +
> > > > > > > > +		sprite_width = prev_w;
> > > > > > > > +		sprite_height = prev_h;
> > > > > > > > +
> > > > > > > > +		if (!max_sprite_width)
> > > > > > > > +			max_sprite_width = true;
> > > > > > > > +		else
> > > > > > > > +			max_sprite_height = true;
> > > > > > > >  	}
> > > > > > > >  
> > > > > > > >  	igt_info("Running test on pipe %s with
> > > > > > > > resolution %dx%d
> > > > > > > > and
> > > > > > > > sprite size %dx%d alpha %i\n",
> > > > > > > > @@ -463,7 +465,6 @@ run_transition_test(igt_display_t
> > > > > > > > *display,
> > > > > > > > enum pipe pipe, igt_output_t *output
> > > > > > > >  
> > > > > > > >  	if (flags & DRM_MODE_ATOMIC_ALLOW_MODESET) {
> > > > > > > >  		igt_output_set_pipe(output, PIPE_NONE);
> > > > > > > > -
> > > > > > > >  		igt_display_commit2(display,
> > > > > > > > COMMIT_ATOMIC);
> > > > > > > >  
> > > > > > > >  		igt_output_set_pipe(output, pipe);
> > > > > > > > @@ -525,8 +526,10 @@ run_transition_test(igt_display_t
> > > > > > > > *display,
> > > > > > > > enum pipe pipe, igt_output_t *output
> > > > > > > >  		}
> > > > > > > >  
> > > > > > > >  		/* force planes to be part of commit */
> > > > > > > > -		for_each_plane_on_pipe(display, pipe,
> > > > > > > > plane)
> > > > > > > > -			igt_plane_set_position(plane,
> > > > > > > > 0, 0);
> > > > > > > > +		for_each_plane_on_pipe(display, pipe,
> > > > > > > > plane) {
> > > > > > > > +			if (parms[plane->index].mask)
> > > > > > > > +				igt_plane_set_position(
> > > > > > > > plane,
> > > > > > > > 0, 0);
> > > > > > > > +		}
> > > > > > > >  
> > > > > > > >  		igt_display_commit2(display,
> > > > > > > > COMMIT_ATOMIC);
> > > > > > > >  
> > > > > > > > -- 
> > > > > > > > 2.17.1
> > > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > 
> > > > > 
> > > 
> > > 
> 
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t v4 2/2] igt/tests/kms_atomic_transition: Tolerate if can't have all planes

[Daniel Vetter]

On Tue, Apr 16, 2019 at 12:43:46PM +0000, Lisovskiy, Stanislav wrote:
> On Tue, 2019-04-16 at 14:27 +0200, Daniel Vetter wrote:
> > On Tue, Apr 16, 2019 at 09:07:52AM +0000, Lisovskiy, Stanislav wrote:
> > > On Tue, 2019-04-16 at 10:52 +0200, Daniel Vetter wrote:
> > > > On Fri, Apr 05, 2019 at 07:20:15AM +0000, Lisovskiy, Stanislav
> > > > wrote:
> > > > > On Thu, 2019-04-04 at 14:56 +0200, Daniel Vetter wrote:
> > > > > > On Thu, Apr 04, 2019 at 11:45:03AM +0000, Lisovskiy,
> > > > > > Stanislav
> > > > > > wrote:
> > > > > > > On Wed, 2019-04-03 at 16:11 +0200, Daniel Vetter wrote:
> > > > > > > > On Wed, Apr 03, 2019 at 03:54:50PM +0300, Stanislav
> > > > > > > > Lisovskiy
> > > > > > > > wrote:
> > > > > > > > > With some upcoming changes i915 might not allow
> > > > > > > > > all sprite planes enabled, depending on available
> > > > > > > > > bandwidth limitation. Thus the test need to decrement
> > > > > > > > > amount of planes and try again, instead of panicking.
> > > > > > > > > 
> > > > > > > > > v2: Removed excessive nested conditions, making code a
> > > > > > > > > bit
> > > > > > > > >     more readable(hopefully).
> > > > > > > > > 
> > > > > > > > > v3: Stopped using global n_planes variable as it might
> > > > > > > > > cause
> > > > > > > > >     resources being unreleased.
> > > > > > > > >     Using now parms[i].mask as a way to determine if
> > > > > > > > > plane
> > > > > > > > >     has to be included into commit.
> > > > > > > > > 
> > > > > > > > > v4: Removed unneeded n_overlays initialization.
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Stanislav Lisovskiy <
> > > > > > > > > stanislav.lisovskiy@intel.com>
> > > > > > > > > ---
> > > > > > > > >  tests/kms_atomic_transition.c | 107 +++++++++++++++++-
> > > > > > > > > ----
> > > > > > > > > ----
> > > > > > > > > ----
> > > > > > > > > ----
> > > > > > > > >  1 file changed, 55 insertions(+), 52 deletions(-)
> > > > > > > > > 
> > > > > > > > > diff --git a/tests/kms_atomic_transition.c
> > > > > > > > > b/tests/kms_atomic_transition.c
> > > > > > > > > index 638fe17e..a04220f3 100644
> > > > > > > > > --- a/tests/kms_atomic_transition.c
> > > > > > > > > +++ b/tests/kms_atomic_transition.c
> > > > > > > > > @@ -212,9 +212,11 @@ static void
> > > > > > > > > setup_parms(igt_display_t
> > > > > > > > > *display, enum pipe pipe,
> > > > > > > > > 
> > 
> > Maybe there's a misunderstanding. What this patch does here is figure
> > out
> > how many planes can be used at most using try-commit. But ones it
> > does
> > that, it also assumes that any combination of these planes (no matter
> > which dest rectangles we end up picking for them) will work out.
> > 
> > This again encodes an assumption about how i915.ko works, like the
> > previous assumption that we can use all planes.
> > 
> > What I'd like to see, and how atomic is specified, is that _any_
> > change in
> > the configuration (dst/src, pixel format, fb stride, size, tiling,
> > really
> > anything) could flip the configuration from "works" to "doesn't
> > work".
> > Worse, the actual transition (i.e. the previous state) also matters.
> > Hence
> > the usual way an atomic commit should work is:
> > 
> > while {useful_config_left} {
> > 	build_config();
> > 	ret = try_commit();
> > 
> > 	if (ret == -EINVAL || ret == -ERANGE) {
> > 		reduce configs
> > 	} else {
> > 		break;
> > 	}
> > }
> > 
> > render_stuff();
> > commit();
> > 
> > Iow _every_ commit needs to first figure out what's possible. And
> > this is
> > what we need to do for the more randomized coverage tests that try to
> > make
> > sure corner cases work. More feature specific (and hence limited)
> > tests
> > can make more assumption.
> > 
> > Hopefully that explains better what I mean here.
> 
> I agree that we need such test, kms_atomic_transition now at least
> tries to randomize which plane will be thrown away, once we run
> into bw limits.
> I'm also writing now a stress test case, which attempts to use allpipes/planes simultaneously with gpu/cpu load. 
> However it doesn't have a randomization, but only tries to enable
> everything it can simultaneously at least up to the limits when
> try_commit fails. 
> 
> So do you think we should add this format/tiling randomized
> configuration testing to kms_atomic_transition or have some other test
> case for that?

We need to adjust all the randomized testcases to follow this pattern. Not
new testcases - this really is a testcase bug, since the testcase makes
assumption about hw limitations that aren't holding up in all cases.

I'm also not sure we need even more randomized testcases, we seem to have
quite a few already. They're quite expensive in CI machine time, so adding
more (instead of improving the ones we have) feels like the wrong
direction.

And yes reworking the existing tests to follow the above pattern will be a
lot of work. That's why I'm not happy with pushing the quick workaround
and then forgetting about the real problem.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] [PATCH i-g-t v4 2/2] igt/tests/kms_atomic_transition: Tolerate if can't have all planes

[Stanislav Lisovskiy]

With some upcoming changes i915 might not allow
all sprite planes enabled, depending on available
bandwidth limitation. Thus the test need to decrement
amount of planes and try again, instead of panicking.

v2: Removed excessive nested conditions, making code a bit
    more readable(hopefully). Stopped using global n_planes
    variable as it might cause resources being unreleased.
    Using now parms[i].mask as a way to determine if plane
    has to be included into commit.

Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
---
 tests/kms_atomic_transition.c | 105 +++++++++++++++++-----------------
 1 file changed, 54 insertions(+), 51 deletions(-)

diff --git a/tests/kms_atomic_transition.c b/tests/kms_atomic_transition.c
index 638fe17e..ee07880e 100644
--- a/tests/kms_atomic_transition.c
+++ b/tests/kms_atomic_transition.c
@@ -214,7 +214,9 @@ static void setup_parms(igt_display_t *display, enum pipe pipe,
 	uint32_t n_planes = display->pipes[pipe].n_planes;
 	uint32_t n_overlays = 0, overlays[n_planes];
 	igt_plane_t *plane;
-	uint32_t iter_mask = 3;
+	uint32_t iter_mask;
+	int retries = n_planes - 1;
+	int ret = 0;
 
 	do_or_die(drmGetCap(display->drm_fd, DRM_CAP_CURSOR_WIDTH, &cursor_width));
 	if (cursor_width >= mode->hdisplay)
@@ -224,6 +226,10 @@ static void setup_parms(igt_display_t *display, enum pipe pipe,
 	if (cursor_height >= mode->vdisplay)
 		cursor_height = mode->vdisplay;
 
+retry:
+	n_overlays = 0;
+	iter_mask = 3;
+
 	for_each_plane_on_pipe(display, pipe, plane) {
 		int i = plane->index;
 
@@ -238,12 +244,16 @@ static void setup_parms(igt_display_t *display, enum pipe pipe,
 			parms[i].height = cursor_height;
 			parms[i].mask = 1 << 1;
 		} else {
-			parms[i].fb = sprite_fb;
-			parms[i].mask = 1 << 2;
-
-			iter_mask |= 1 << 2;
-
-			overlays[n_overlays++] = i;
+			if (n_overlays < (n_planes - 2)) {
+				parms[i].fb = sprite_fb;
+				parms[i].mask = 1 << 2;
+				iter_mask |= 1 << 2;
+				overlays[n_overlays++] = i;
+			}
+			else {
+				parms[i].fb = NULL;
+				parms[i].mask = 0;
+			}
 		}
 	}
 
@@ -278,16 +288,13 @@ static void setup_parms(igt_display_t *display, enum pipe pipe,
 	 * Pre gen9 not all sizes are supported, find the biggest possible
 	 * size that can be enabled on all sprite planes.
 	 */
-retry:
 	prev_w = sprite_width = cursor_width;
 	prev_h = sprite_height = cursor_height;
 
 	max_sprite_width = (sprite_width == mode->hdisplay);
 	max_sprite_height = (sprite_height == mode->vdisplay);
 
-	while (1) {
-		int ret;
-
+	while (!max_sprite_width && !max_sprite_height) {
 		set_sprite_wh(display, pipe, parms, sprite_fb,
 			      alpha, sprite_width, sprite_height);
 
@@ -295,54 +302,49 @@ retry:
 		ret = igt_display_try_commit_atomic(display, DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
 		igt_assert(!is_atomic_check_failure_errno(ret));
 
-		if (is_atomic_check_plane_size_errno(ret)) {
-			if (cursor_width == sprite_width &&
-			    cursor_height == sprite_height) {
-				igt_assert_f(alpha,
-					      "Cannot configure the test with all sprite planes enabled\n");
-
-				/* retry once with XRGB format. */
-				alpha = false;
-				goto retry;
-			}
-
-			sprite_width = prev_w;
-			sprite_height = prev_h;
-
-			if (max_sprite_width && max_sprite_height) {
-				set_sprite_wh(display, pipe, parms, sprite_fb,
-					      alpha, sprite_width, sprite_height);
-				break;
-			}
-
-			if (!max_sprite_width)
-				max_sprite_width = true;
-			else
-				max_sprite_height = true;
-		} else {
+		if (!is_atomic_check_plane_size_errno(ret)) {
 			prev_w = sprite_width;
 			prev_h = sprite_height;
-		}
-
-		if (!max_sprite_width) {
-			sprite_width *= 2;
-
+    
+			sprite_width *= max_sprite_width ? 1 : 2;
 			if (sprite_width >= mode->hdisplay) {
 				max_sprite_width = true;
-
 				sprite_width = mode->hdisplay;
 			}
-		} else if (!max_sprite_height) {
-			sprite_height *= 2;
 
+			sprite_height *= max_sprite_height ? 1 : 2;
 			if (sprite_height >= mode->vdisplay) {
 				max_sprite_height = true;
-
 				sprite_height = mode->vdisplay;
 			}
-		} else
-			/* Max sized sprites for all! */
-			break;
+			continue;
+		}
+
+		if (cursor_width == sprite_width &&
+		    cursor_height == sprite_height) {
+			igt_assert_f(retries > 0,
+				      "Cannot configure the test with all sprite planes enabled\n");
+			--retries;
+			/* retry once with XRGB format. */
+			if (alpha) {
+				alpha = false;
+				igt_warn("Removed alpha\n");
+			}
+			else {
+				igt_assert_f(n_planes > 1, "No planes left to proceed with!");
+				n_planes--;
+				igt_warn("Reduced available planes to %d\n", n_planes);
+			}
+			goto retry;
+		}
+
+		sprite_width = prev_w;
+		sprite_height = prev_h;
+
+		if (!max_sprite_width)
+			max_sprite_width = true;
+		else
+			max_sprite_height = true;
 	}
 
 	igt_info("Running test on pipe %s with resolution %dx%d and sprite size %dx%d alpha %i\n",
@@ -463,7 +465,6 @@ run_transition_test(igt_display_t *display, enum pipe pipe, igt_output_t *output
 
 	if (flags & DRM_MODE_ATOMIC_ALLOW_MODESET) {
 		igt_output_set_pipe(output, PIPE_NONE);
-
 		igt_display_commit2(display, COMMIT_ATOMIC);
 
 		igt_output_set_pipe(output, pipe);
@@ -525,8 +526,10 @@ run_transition_test(igt_display_t *display, enum pipe pipe, igt_output_t *output
 		}
 
 		/* force planes to be part of commit */
-		for_each_plane_on_pipe(display, pipe, plane)
-			igt_plane_set_position(plane, 0, 0);
+		for_each_plane_on_pipe(display, pipe, plane) {
+			if (parms[plane->index].mask)
+				igt_plane_set_position(plane, 0, 0);
+		}
 
 		igt_display_commit2(display, COMMIT_ATOMIC);
 
-- 
2.17.1

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