All of lore.kernel.org
 help / color / mirror / Atom feed
* [igt-dev] [PATCH i-g-t 1/2] HAX: tests/kms_atomic_transition: Don't test more planes than memory bandwidth can support
@ 2019-04-05 23:31 José Roberto de Souza
  2019-04-05 23:31 ` [igt-dev] [PATCH i-g-t 2/2] tests/kms_atomic_transition: Remove useless code José Roberto de Souza
                   ` (5 more replies)
  0 siblings, 6 replies; 10+ messages in thread
From: José Roberto de Souza @ 2019-04-05 23:31 UTC (permalink / raw)
  To: igt-dev; +Cc: Stanislav Lisovskiy

Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 tests/kms_atomic_transition.c | 217 ++++++++++++++++++++--------------
 1 file changed, 130 insertions(+), 87 deletions(-)

diff --git a/tests/kms_atomic_transition.c b/tests/kms_atomic_transition.c
index 18f73317..9988f303 100644
--- a/tests/kms_atomic_transition.c
+++ b/tests/kms_atomic_transition.c
@@ -40,9 +40,18 @@
 #define DRM_CAP_CURSOR_HEIGHT 0x9
 #endif
 
+enum group_type {
+	GROUP_TYPE_NONE = 0,
+	GROUP_TYPE_PRIMARY = 1 << 0,
+	GROUP_TYPE_CURSOR = 1 << 1,
+	GROUP_TYPE_OVERLAY = 1 << 2,
+	GROUP_TYPE_OVERLAY2 = 1 << 3
+};
+
 struct plane_parms {
 	struct igt_fb *fb;
-	uint32_t width, height, mask;
+	uint32_t width, height;
+	enum group_type mask;
 };
 
 /* globals for fence support */
@@ -203,12 +212,11 @@ static void setup_parms(igt_display_t *display, enum pipe pipe,
 			unsigned *iter_max)
 {
 	uint64_t cursor_width, cursor_height;
-	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 sprite_width, sprite_height, prev_w, prev_h;
 	igt_plane_t *plane;
-	uint32_t iter_mask = 3;
+	uint32_t iter_mask = 0, max_overlays, n_overlays = 0;
+	bool alpha = true;
+	int ret;
 
 	do_or_die(drmGetCap(display->drm_fd, DRM_CAP_CURSOR_WIDTH, &cursor_width));
 	if (cursor_width >= mode->hdisplay)
@@ -225,123 +233,156 @@ static void setup_parms(igt_display_t *display, enum pipe pipe,
 			parms[i].fb = primary_fb;
 			parms[i].width = mode->hdisplay;
 			parms[i].height = mode->vdisplay;
-			parms[i].mask = 1 << 0;
+			parms[i].mask = GROUP_TYPE_PRIMARY;
+			iter_mask |= GROUP_TYPE_PRIMARY;
 		} else if (plane->type == DRM_PLANE_TYPE_CURSOR) {
 			parms[i].fb = argb_fb;
 			parms[i].width = cursor_width;
 			parms[i].height = cursor_height;
-			parms[i].mask = 1 << 1;
+			parms[i].mask = GROUP_TYPE_CURSOR;
+			iter_mask |= GROUP_TYPE_CURSOR;
 		} else {
 			parms[i].fb = sprite_fb;
-			parms[i].mask = 1 << 2;
-
-			iter_mask |= 1 << 2;
-
-			overlays[n_overlays++] = i;
+			parms[i].mask = GROUP_TYPE_OVERLAY;
+			iter_mask |= GROUP_TYPE_OVERLAY;
+			n_overlays++;
+			if (alpha)
+				alpha = igt_plane_has_format_mod(plane,
+								 DRM_FORMAT_ARGB8888,
+								 LOCAL_DRM_FORMAT_MOD_NONE);
 		}
 	}
 
-	if (n_overlays >= 2) {
-		uint32_t i;
+	prev_w = sprite_width = cursor_width;
+	prev_h = sprite_height = cursor_height;
+
+	igt_create_fb(display->drm_fd, cursor_width, cursor_height,
+		      DRM_FORMAT_ARGB8888, LOCAL_DRM_FORMAT_MOD_NONE, argb_fb);
+
+	igt_create_fb(display->drm_fd, sprite_width, sprite_height,
+		      alpha ? DRM_FORMAT_ARGB8888 : DRM_FORMAT_XRGB8888,
+		      LOCAL_DRM_FORMAT_MOD_NONE, sprite_fb);
+
+	max_overlays = n_overlays;
+retry_bw:
+	/* Limit the number of planes */
+	while (max_overlays < n_overlays) {
+		int i = hars_petruska_f54_1_random_unsafe_max(display->pipes[pipe].n_planes);
 
 		/*
-		 * Create 2 groups for overlays, make sure 1 plane is put
-		 * in each then spread the rest out.
+		 * Always keep primary and cursor and discard already
+		 * removed planes
 		 */
-		iter_mask |= 1 << 3;
-		parms[overlays[n_overlays - 1]].mask = 1 << 3;
+		if (parms[i].mask != GROUP_TYPE_OVERLAY)
+			continue;
 
-		for (i = 1; i < n_overlays - 1; i++) {
-			int val = hars_petruska_f54_1_random_unsafe_max(2);
+		parms[i].mask = GROUP_TYPE_NONE;
+		n_overlays--;
+	}
 
-			parms[overlays[i]].mask = 1 << (2 + val);
-		}
+	/*
+	 * Check if there is enough bandwidth for the current number of planes.
+	 * As the plane size and position is not taken into account we can do
+	 * this earlier.
+	 */
+	set_sprite_wh(display, pipe, parms, sprite_fb, alpha, sprite_width,
+		      sprite_height);
+	wm_setup_plane(display, pipe, iter_mask, parms, false);
+
+	/* It should be able to handle at least primary and cursor */
+	if (!max_overlays) {
+		iter_mask &= ~GROUP_TYPE_OVERLAY;
+		*iter_max = iter_mask + 1;
+		return;
 	}
 
-	igt_create_fb(display->drm_fd, cursor_width, cursor_height,
-		      DRM_FORMAT_ARGB8888, LOCAL_DRM_FORMAT_MOD_NONE, argb_fb);
+	ret = igt_display_try_commit_atomic(display, DRM_MODE_ATOMIC_TEST_ONLY |
+					    DRM_MODE_ATOMIC_ALLOW_MODESET,
+					    NULL);
+	/*
+	 * Could mean other errors but this is also the error returned when
+	 * there is not enough bandwidth for all the planes
+	 */
+	if (ret == -EINVAL) {
+		max_overlays--;
+		goto retry_bw;
+	}
 
-	igt_create_fb(display->drm_fd, cursor_width, cursor_height,
-		      DRM_FORMAT_ARGB8888, LOCAL_DRM_FORMAT_MOD_NONE, sprite_fb);
+	igt_assert_f(!ret, "Error %i not expected by try_commit()\n", ret);
 
-	*iter_max = iter_mask + 1;
-	if (!n_overlays)
-		return;
+	/* So it have enough bandwidth for n_overlays planes */
 
 	/*
-	 * Pre gen9 not all sizes are supported, find the biggest possible
-	 * size that can be enabled on all sprite planes.
+	 * Create 2 groups for overlays, make sure 1 plane is put in each then
+	 * spread the rest out.
 	 */
-retry:
-	prev_w = sprite_width = cursor_width;
-	prev_h = sprite_height = cursor_height;
+	iter_mask &= ~GROUP_TYPE_OVERLAY;
+	for_each_plane_on_pipe(display, pipe, plane) {
+		int i = plane->index;
 
-	max_sprite_width = (sprite_width == mode->hdisplay);
-	max_sprite_height = (sprite_height == mode->vdisplay);
+		if (parms[i].mask != GROUP_TYPE_OVERLAY)
+			continue;
 
-	while (1) {
-		int ret;
+		/* First overlay plane will be overlay group 1 */
+		if (!(iter_mask & GROUP_TYPE_OVERLAY)) {
+			iter_mask |= GROUP_TYPE_OVERLAY;
+			continue;
+		}
 
-		set_sprite_wh(display, pipe, parms, sprite_fb,
-			      alpha, sprite_width, sprite_height);
+		/* Second overlay plane will be overlay group 1 */
+		if (!(iter_mask & GROUP_TYPE_OVERLAY2)) {
+			iter_mask |= GROUP_TYPE_OVERLAY2;
+			parms[i].mask = GROUP_TYPE_OVERLAY2;
+			continue;
+		}
 
-		wm_setup_plane(display, pipe, (1 << n_planes) - 1, parms, false);
-		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));
+		/* Sort the group of the rest of overlay planes */
+		if (hars_petruska_f54_1_random_unsafe_max(2))
+			parms[i].mask = GROUP_TYPE_OVERLAY2;
+	}
 
-		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;
-			}
+	*iter_max = iter_mask + 1;
 
+	/*
+	 * Pre gen9 not all sizes are supported, find the biggest possible
+	 * size that can be enabled on all sprite planes.
+	 */
+	while (1) {
+		/* Size limit reached */
+		if (is_atomic_check_plane_size_errno(ret)) {
 			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 {
-			prev_w = sprite_width;
-			prev_h = sprite_height;
+			break;
 		}
 
-		if (!max_sprite_width) {
-			sprite_width *= 2;
+		/* Commit is valid and it reached max size, use this size */
+		if (sprite_width == mode->hdisplay ||
+		    sprite_height == mode->vdisplay)
+			break;
 
-			if (sprite_width >= mode->hdisplay) {
-				max_sprite_width = true;
+		prev_w = sprite_width;
+		prev_h = sprite_height;
 
-				sprite_width = mode->hdisplay;
-			}
-		} else if (!max_sprite_height) {
-			sprite_height *= 2;
+		sprite_width *= 2;
+		if (sprite_width >= mode->hdisplay)
+			sprite_width = mode->hdisplay;
 
-			if (sprite_height >= mode->vdisplay) {
-				max_sprite_height = true;
+		sprite_height *= 2;
+		if (sprite_height >= mode->vdisplay)
+			sprite_height = mode->vdisplay;
 
-				sprite_height = mode->vdisplay;
-			}
-		} else
-			/* Max sized sprites for all! */
-			break;
+		set_sprite_wh(display, pipe, parms, sprite_fb, sprite_width,
+			      sprite_height, alpha);
+		wm_setup_plane(display, pipe, iter_mask, parms, false);
+		ret = igt_display_try_commit_atomic(display,
+						    DRM_MODE_ATOMIC_TEST_ONLY |
+						    DRM_MODE_ATOMIC_ALLOW_MODESET,
+						    NULL);
 	}
 
-	igt_info("Running test on pipe %s with resolution %dx%d and sprite size %dx%d alpha %i\n",
+	igt_info("Running test on pipe %s with resolution %dx%d, sprite size %dx%d, alpha %i and %i overlay planes\n",
 		 kmstest_pipe_name(pipe), mode->hdisplay, mode->vdisplay,
-		 sprite_width, sprite_height, alpha);
+		 sprite_width, sprite_height, alpha, n_overlays);
 }
 
 static void prepare_fencing(igt_display_t *display, enum pipe pipe)
@@ -519,8 +560,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 != GROUP_TYPE_NONE)
+				igt_plane_set_position(plane, 0, 0);
+		}
 
 		igt_display_commit2(display, COMMIT_ATOMIC);
 
-- 
2.21.0

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

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

* [igt-dev] [PATCH i-g-t 2/2] tests/kms_atomic_transition: Remove useless code
  2019-04-05 23:31 [igt-dev] [PATCH i-g-t 1/2] HAX: tests/kms_atomic_transition: Don't test more planes than memory bandwidth can support José Roberto de Souza
@ 2019-04-05 23:31 ` José Roberto de Souza
  2019-04-06  0:23 ` [igt-dev] ✗ Fi.CI.BAT: failure for series starting with [i-g-t,1/2] HAX: tests/kms_atomic_transition: Don't test more planes than memory bandwidth can support Patchwork
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 10+ messages in thread
From: José Roberto de Souza @ 2019-04-05 23:31 UTC (permalink / raw)
  To: igt-dev

setup_parms() already makes sure that overlay planes size is supported
by driver and GPU, no need to re-run it.

Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 tests/kms_atomic_transition.c | 39 -----------------------------------
 1 file changed, 39 deletions(-)

diff --git a/tests/kms_atomic_transition.c b/tests/kms_atomic_transition.c
index 9988f303..1cee2b6e 100644
--- a/tests/kms_atomic_transition.c
+++ b/tests/kms_atomic_transition.c
@@ -471,7 +471,6 @@ run_transition_test(igt_display_t *display, enum pipe pipe, igt_output_t *output
 	uint32_t iter_max, i;
 	struct plane_parms parms[pipe_obj->n_planes];
 	unsigned flags = 0;
-	int ret;
 
 	if (fencing)
 		prepare_fencing(display, pipe);
@@ -508,44 +507,6 @@ run_transition_test(igt_display_t *display, enum pipe pipe, igt_output_t *output
 
 	setup_parms(display, pipe, mode, &fb, &argb_fb, &sprite_fb, parms, &iter_max);
 
-	/*
-	 * In some configurations the tests may not run to completion with all
-	 * sprite planes lit up at 4k resolution, try decreasing width/size of secondary
-	 * planes to fix this
-	 */
-	while (1) {
-		wm_setup_plane(display, pipe, iter_max - 1, parms, false);
-
-		if (fencing)
-			igt_pipe_request_out_fence(pipe_obj);
-
-		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) || pipe_obj->n_planes < 3)
-			break;
-
-		ret = 0;
-		for_each_plane_on_pipe(display, pipe, plane) {
-			i = plane->index;
-
-			if (plane->type == DRM_PLANE_TYPE_PRIMARY ||
-			    plane->type == DRM_PLANE_TYPE_CURSOR)
-				continue;
-
-			if (parms[i].width <= 512)
-				continue;
-
-			parms[i].width /= 2;
-			ret = 1;
-			igt_info("Reducing sprite %i to %ux%u\n", i - 1, parms[i].width, parms[i].height);
-			break;
-		}
-
-		if (!ret)
-			igt_skip("Cannot run tests without proper size sprite planes\n");
-	}
-
 	igt_display_commit2(display, COMMIT_ATOMIC);
 
 	if (type == TRANSITION_AFTER_FREE) {
-- 
2.21.0

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

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

* [igt-dev] ✗ Fi.CI.BAT: failure for series starting with [i-g-t,1/2] HAX: tests/kms_atomic_transition: Don't test more planes than memory bandwidth can support
  2019-04-05 23:31 [igt-dev] [PATCH i-g-t 1/2] HAX: tests/kms_atomic_transition: Don't test more planes than memory bandwidth can support José Roberto de Souza
  2019-04-05 23:31 ` [igt-dev] [PATCH i-g-t 2/2] tests/kms_atomic_transition: Remove useless code José Roberto de Souza
@ 2019-04-06  0:23 ` Patchwork
  2019-04-06  1:27 ` [igt-dev] ✓ Fi.CI.BAT: success for series starting with [i-g-t,1/2] HAX: tests/kms_atomic_transition: Don't test more planes than memory bandwidth can support (rev2) Patchwork
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 10+ messages in thread
From: Patchwork @ 2019-04-06  0:23 UTC (permalink / raw)
  To: Souza, Jose; +Cc: igt-dev

== Series Details ==

Series: series starting with [i-g-t,1/2] HAX: tests/kms_atomic_transition: Don't test more planes than memory bandwidth can support
URL   : https://patchwork.freedesktop.org/series/59086/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_5882 -> IGTPW_2806
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with IGTPW_2806 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in IGTPW_2806, 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/59086/revisions/1/mbox/

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

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

### IGT changes ###

#### Possible regressions ####

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

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

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

### IGT changes ###

#### Issues hit ####

  * igt@i915_module_load@reload-with-fault-injection:
    - fi-icl-y:           PASS -> INCOMPLETE [fdo#107713]

  * igt@i915_selftest@live_execlists:
    - fi-apl-guc:         PASS -> INCOMPLETE [fdo#103927] / [fdo#109720]

  * igt@kms_frontbuffer_tracking@basic:
    - fi-byt-clapper:     PASS -> FAIL [fdo#103167]

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
    - fi-blb-e6850:       PASS -> INCOMPLETE [fdo#107718]

  
#### Possible fixes ####

  * igt@i915_pm_rpm@module-reload:
    - fi-skl-6770hq:      DMESG-WARN [fdo#105541] -> PASS
    - {fi-icl-u3}:        DMESG-WARN [fdo#107724] -> PASS

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

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

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

  
  {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#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [fdo#105541]: https://bugs.freedesktop.org/show_bug.cgi?id=105541
  [fdo#107362]: https://bugs.freedesktop.org/show_bug.cgi?id=107362
  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
  [fdo#109720]: https://bugs.freedesktop.org/show_bug.cgi?id=109720


Participating hosts (50 -> 44)
------------------------------

  Missing    (6): fi-kbl-soraka fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-bdw-samus fi-skl-6600u 


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

    * IGT: IGT_4932 -> IGTPW_2806

  CI_DRM_5882: 012535789c9c890854cd1e0fb926f44931a82a63 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_2806: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2806/
  IGT_4932: 08cf63a8fac11e3594b57580331fb319241a0d69 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

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

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

* [igt-dev] ✓ Fi.CI.BAT: success for series starting with [i-g-t,1/2] HAX: tests/kms_atomic_transition: Don't test more planes than memory bandwidth can support (rev2)
  2019-04-05 23:31 [igt-dev] [PATCH i-g-t 1/2] HAX: tests/kms_atomic_transition: Don't test more planes than memory bandwidth can support José Roberto de Souza
  2019-04-05 23:31 ` [igt-dev] [PATCH i-g-t 2/2] tests/kms_atomic_transition: Remove useless code José Roberto de Souza
  2019-04-06  0:23 ` [igt-dev] ✗ Fi.CI.BAT: failure for series starting with [i-g-t,1/2] HAX: tests/kms_atomic_transition: Don't test more planes than memory bandwidth can support Patchwork
@ 2019-04-06  1:27 ` Patchwork
  2019-04-07  2:02 ` [igt-dev] ✗ Fi.CI.IGT: failure " Patchwork
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 10+ messages in thread
From: Patchwork @ 2019-04-06  1:27 UTC (permalink / raw)
  To: Souza, Jose; +Cc: igt-dev

== Series Details ==

Series: series starting with [i-g-t,1/2] HAX: tests/kms_atomic_transition: Don't test more planes than memory bandwidth can support (rev2)
URL   : https://patchwork.freedesktop.org/series/59086/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5882 -> IGTPW_2807
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/59086/revisions/2/mbox/

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

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

### IGT changes ###

#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * igt@gem_ctx_exec@basic:
    - {fi-icl-u3}:        PASS -> INCOMPLETE

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

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

### IGT changes ###

#### Issues hit ####

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

  * igt@i915_selftest@live_execlists:
    - fi-apl-guc:         PASS -> INCOMPLETE [fdo#103927] / [fdo#109720]

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

  * igt@kms_flip@basic-flip-vs-wf_vblank:
    - fi-bwr-2160:        PASS -> FAIL [fdo#100368]

  * igt@kms_frontbuffer_tracking@basic:
    - fi-byt-clapper:     PASS -> FAIL [fdo#103167]

  * igt@runner@aborted:
    - fi-apl-guc:         NOTRUN -> FAIL [fdo#108622] / [fdo#109720]

  
#### Possible fixes ####

  * igt@i915_pm_rpm@module-reload:
    - fi-skl-6770hq:      DMESG-WARN [fdo#105541] -> PASS

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

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

  [fdo#100368]: https://bugs.freedesktop.org/show_bug.cgi?id=100368
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
  [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [fdo#105541]: https://bugs.freedesktop.org/show_bug.cgi?id=105541
  [fdo#107362]: https://bugs.freedesktop.org/show_bug.cgi?id=107362
  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#108622]: https://bugs.freedesktop.org/show_bug.cgi?id=108622
  [fdo#109720]: https://bugs.freedesktop.org/show_bug.cgi?id=109720
  [fdo#110210]: https://bugs.freedesktop.org/show_bug.cgi?id=110210


Participating hosts (50 -> 41)
------------------------------

  Missing    (9): fi-kbl-soraka fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-snb-2520m fi-hsw-4770 fi-icl-y fi-bdw-samus fi-skl-6600u 


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

    * IGT: IGT_4932 -> IGTPW_2807

  CI_DRM_5882: 012535789c9c890854cd1e0fb926f44931a82a63 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_2807: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2807/
  IGT_4932: 08cf63a8fac11e3594b57580331fb319241a0d69 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

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

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

* [igt-dev] ✗ Fi.CI.IGT: failure for series starting with [i-g-t,1/2] HAX: tests/kms_atomic_transition: Don't test more planes than memory bandwidth can support (rev2)
  2019-04-05 23:31 [igt-dev] [PATCH i-g-t 1/2] HAX: tests/kms_atomic_transition: Don't test more planes than memory bandwidth can support José Roberto de Souza
                   ` (2 preceding siblings ...)
  2019-04-06  1:27 ` [igt-dev] ✓ Fi.CI.BAT: success for series starting with [i-g-t,1/2] HAX: tests/kms_atomic_transition: Don't test more planes than memory bandwidth can support (rev2) Patchwork
@ 2019-04-07  2:02 ` Patchwork
  2019-04-08  8:38 ` [igt-dev] [PATCH i-g-t 1/2] HAX: tests/kms_atomic_transition: Don't test more planes than memory bandwidth can support Lisovskiy, Stanislav
  2019-04-08  9:47 ` Lisovskiy, Stanislav
  5 siblings, 0 replies; 10+ messages in thread
From: Patchwork @ 2019-04-07  2:02 UTC (permalink / raw)
  To: Souza, Jose; +Cc: igt-dev

== Series Details ==

Series: series starting with [i-g-t,1/2] HAX: tests/kms_atomic_transition: Don't test more planes than memory bandwidth can support (rev2)
URL   : https://patchwork.freedesktop.org/series/59086/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_5882_full -> IGTPW_2807_full
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with IGTPW_2807_full absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in IGTPW_2807_full, 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/59086/revisions/2/mbox/

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

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

### IGT changes ###

#### Possible regressions ####

  * igt@kms_atomic_transition@plane-all-transition:
    - shard-snb:          PASS -> FAIL +8

  * igt@kms_atomic_transition@plane-use-after-nonblocking-unbind:
    - shard-hsw:          PASS -> FAIL +8

  * igt@kms_plane_multiple@atomic-pipe-b-tiling-x:
    - shard-apl:          PASS -> DMESG-WARN

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_create@create-clear:
    - shard-snb:          PASS -> INCOMPLETE [fdo#105411]

  * igt@kms_atomic_transition@1x-modeset-transitions-nonblocking-fencing:
    - shard-apl:          PASS -> FAIL [fdo#109660]

  * igt@kms_atomic_transition@3x-modeset-transitions:
    - shard-snb:          NOTRUN -> SKIP [fdo#109271] / [fdo#109278] +12

  * igt@kms_atomic_transition@3x-modeset-transitions-fencing:
    - shard-kbl:          NOTRUN -> SKIP [fdo#109271] / [fdo#109278]

  * igt@kms_atomic_transition@plane-all-modeset-transition:
    - shard-apl:          PASS -> INCOMPLETE [fdo#103927]

  * igt@kms_atomic_transition@plane-all-modeset-transition-internal-panels:
    - shard-kbl:          NOTRUN -> SKIP [fdo#109271] +23

  * igt@kms_busy@extended-modeset-hang-newfb-render-a:
    - shard-hsw:          PASS -> DMESG-WARN [fdo#110222]
    - shard-kbl:          PASS -> DMESG-WARN [fdo#110222]
    - shard-snb:          PASS -> DMESG-WARN [fdo#110222]

  * igt@kms_frontbuffer_tracking@fbcpsr-shrfb-scaledprimary:
    - shard-apl:          NOTRUN -> SKIP [fdo#109271] +11

  * igt@kms_frontbuffer_tracking@psr-1p-primscrn-shrfb-pgflip-blt:
    - shard-snb:          NOTRUN -> SKIP [fdo#109271] +77

  * igt@kms_plane@plane-panning-bottom-right-suspend-pipe-c-planes:
    - shard-kbl:          PASS -> INCOMPLETE [fdo#103665]

  * igt@kms_plane_alpha_blend@pipe-b-alpha-basic:
    - shard-kbl:          NOTRUN -> FAIL [fdo#108145] / [fdo#108590]

  * igt@kms_plane_scaling@pipe-c-scaler-with-pixel-format:
    - shard-glk:          PASS -> SKIP [fdo#109271] / [fdo#109278]

  * igt@kms_vblank@pipe-b-ts-continuation-dpms-rpm:
    - shard-apl:          PASS -> FAIL [fdo#104894] +1

  * igt@perf_pmu@rc6-runtime-pm:
    - shard-apl:          PASS -> FAIL [fdo#105010]

  * igt@prime_nv_api@i915_self_import:
    - shard-glk:          NOTRUN -> SKIP [fdo#109271] +7

  
#### Possible fixes ####

  * igt@i915_suspend@sysfs-reader:
    - shard-kbl:          INCOMPLETE [fdo#103665] -> PASS

  * igt@kms_busy@extended-modeset-hang-newfb-render-b:
    - shard-hsw:          DMESG-WARN [fdo#110222] -> PASS +3

  * igt@kms_busy@extended-modeset-hang-newfb-with-reset-render-b:
    - shard-kbl:          DMESG-WARN [fdo#110222] -> PASS +3

  * igt@kms_color@pipe-a-ctm-max:
    - shard-kbl:          FAIL [fdo#108147] -> PASS
    - shard-apl:          FAIL [fdo#108147] -> PASS

  * igt@kms_cursor_crc@cursor-64x64-sliding:
    - shard-apl:          FAIL [fdo#103232] -> PASS
    - shard-kbl:          FAIL [fdo#103232] -> PASS

  * igt@kms_cursor_legacy@2x-long-nonblocking-modeset-vs-cursor-atomic:
    - shard-glk:          FAIL [fdo#106509] / [fdo#107409] -> PASS

  * igt@kms_frontbuffer_tracking@fbc-2p-scndscrn-shrfb-plflip-blt:
    - shard-glk:          FAIL [fdo#103167] -> PASS

  * igt@kms_plane@pixel-format-pipe-a-planes-source-clamping:
    - shard-glk:          SKIP [fdo#109271] -> PASS

  * igt@kms_setmode@basic:
    - shard-apl:          FAIL [fdo#99912] -> PASS

  * igt@kms_vblank@pipe-b-ts-continuation-modeset-rpm:
    - shard-apl:          FAIL [fdo#104894] -> PASS
    - shard-kbl:          FAIL [fdo#104894] -> PASS

  
#### Warnings ####

  * igt@kms_vblank@pipe-a-ts-continuation-dpms-suspend:
    - shard-kbl:          FAIL [fdo#104894] -> INCOMPLETE [fdo#103665]

  
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
  [fdo#103665]: https://bugs.freedesktop.org/show_bug.cgi?id=103665
  [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [fdo#104894]: https://bugs.freedesktop.org/show_bug.cgi?id=104894
  [fdo#105010]: https://bugs.freedesktop.org/show_bug.cgi?id=105010
  [fdo#105411]: https://bugs.freedesktop.org/show_bug.cgi?id=105411
  [fdo#106509]: https://bugs.freedesktop.org/show_bug.cgi?id=106509
  [fdo#107409]: https://bugs.freedesktop.org/show_bug.cgi?id=107409
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#108147]: https://bugs.freedesktop.org/show_bug.cgi?id=108147
  [fdo#108590]: https://bugs.freedesktop.org/show_bug.cgi?id=108590
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
  [fdo#109660]: https://bugs.freedesktop.org/show_bug.cgi?id=109660
  [fdo#110222]: https://bugs.freedesktop.org/show_bug.cgi?id=110222
  [fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912


Participating hosts (10 -> 5)
------------------------------

  Missing    (5): shard-skl pig-hsw-4770r pig-glk-j5005 shard-iclb pig-skl-6260u 


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

    * IGT: IGT_4932 -> IGTPW_2807
    * Piglit: piglit_4509 -> None

  CI_DRM_5882: 012535789c9c890854cd1e0fb926f44931a82a63 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_2807: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2807/
  IGT_4932: 08cf63a8fac11e3594b57580331fb319241a0d69 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

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

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

* Re: [igt-dev] [PATCH i-g-t 1/2] HAX: tests/kms_atomic_transition: Don't test more planes than memory bandwidth can support
  2019-04-05 23:31 [igt-dev] [PATCH i-g-t 1/2] HAX: tests/kms_atomic_transition: Don't test more planes than memory bandwidth can support José Roberto de Souza
                   ` (3 preceding siblings ...)
  2019-04-07  2:02 ` [igt-dev] ✗ Fi.CI.IGT: failure " Patchwork
@ 2019-04-08  8:38 ` Lisovskiy, Stanislav
  2019-04-09  1:41   ` Souza, Jose
  2019-04-08  9:47 ` Lisovskiy, Stanislav
  5 siblings, 1 reply; 10+ messages in thread
From: Lisovskiy, Stanislav @ 2019-04-08  8:38 UTC (permalink / raw)
  To: igt-dev, Souza, Jose

On Fri, 2019-04-05 at 16:31 -0700, José Roberto de Souza wrote:
> Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  tests/kms_atomic_transition.c | 217 ++++++++++++++++++++----------
> ----
>  1 file changed, 130 insertions(+), 87 deletions(-)
> 
> diff --git a/tests/kms_atomic_transition.c
> b/tests/kms_atomic_transition.c
> index 18f73317..9988f303 100644
> --- a/tests/kms_atomic_transition.c
> +++ b/tests/kms_atomic_transition.c
> @@ -40,9 +40,18 @@
>  #define DRM_CAP_CURSOR_HEIGHT 0x9
>  #endif
>  
> +enum group_type {
> +	GROUP_TYPE_NONE = 0,
> +	GROUP_TYPE_PRIMARY = 1 << 0,
> +	GROUP_TYPE_CURSOR = 1 << 1,
> +	GROUP_TYPE_OVERLAY = 1 << 2,
> +	GROUP_TYPE_OVERLAY2 = 1 << 3
> +};
> +
>  struct plane_parms {
>  	struct igt_fb *fb;
> -	uint32_t width, height, mask;
> +	uint32_t width, height;
> +	enum group_type mask;
>  };
>  
>  /* globals for fence support */
> @@ -203,12 +212,11 @@ static void setup_parms(igt_display_t *display,
> enum pipe pipe,
>  			unsigned *iter_max)
>  {
>  	uint64_t cursor_width, cursor_height;
> -	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 sprite_width, sprite_height, prev_w, prev_h;
>  	igt_plane_t *plane;
> -	uint32_t iter_mask = 3;
> +	uint32_t iter_mask = 0, max_overlays, n_overlays = 0;
> +	bool alpha = true;
> +	int ret;
>  
>  	do_or_die(drmGetCap(display->drm_fd, DRM_CAP_CURSOR_WIDTH,
> &cursor_width));
>  	if (cursor_width >= mode->hdisplay)
> @@ -225,123 +233,156 @@ static void setup_parms(igt_display_t
> *display, enum pipe pipe,
>  			parms[i].fb = primary_fb;
>  			parms[i].width = mode->hdisplay;
>  			parms[i].height = mode->vdisplay;
> -			parms[i].mask = 1 << 0;
> +			parms[i].mask = GROUP_TYPE_PRIMARY;
> +			iter_mask |= GROUP_TYPE_PRIMARY;
>  		} else if (plane->type == DRM_PLANE_TYPE_CURSOR) {
>  			parms[i].fb = argb_fb;
>  			parms[i].width = cursor_width;
>  			parms[i].height = cursor_height;
> -			parms[i].mask = 1 << 1;
> +			parms[i].mask = GROUP_TYPE_CURSOR;
> +			iter_mask |= GROUP_TYPE_CURSOR;
>  		} else {
>  			parms[i].fb = sprite_fb;
> -			parms[i].mask = 1 << 2;
> -
> -			iter_mask |= 1 << 2;
> -
> -			overlays[n_overlays++] = i;
> +			parms[i].mask = GROUP_TYPE_OVERLAY;
> +			iter_mask |= GROUP_TYPE_OVERLAY;
> +			n_overlays++;
> +			if (alpha)
> +				alpha = igt_plane_has_format_mod(plane,
> +								 DRM_FO
> RMAT_ARGB8888,
> +								 LOCAL_
> DRM_FORMAT_MOD_NONE);
>  		}
>  	}
>  
> -	if (n_overlays >= 2) {
> -		uint32_t i;
> +	prev_w = sprite_width = cursor_width;
> +	prev_h = sprite_height = cursor_height;
> +
> +	igt_create_fb(display->drm_fd, cursor_width, cursor_height,
> +		      DRM_FORMAT_ARGB8888, LOCAL_DRM_FORMAT_MOD_NONE,
> argb_fb);
> +
> +	igt_create_fb(display->drm_fd, sprite_width, sprite_height,
> +		      alpha ? DRM_FORMAT_ARGB8888 :
> DRM_FORMAT_XRGB8888,
> +		      LOCAL_DRM_FORMAT_MOD_NONE, sprite_fb);
> +
> +	max_overlays = n_overlays;
> +retry_bw:
> +	/* Limit the number of planes */
> +	while (max_overlays < n_overlays) {
> +		int i = hars_petruska_f54_1_random_unsafe_max(display-
> >pipes[pipe].n_planes);
>  
>  		/*
> -		 * Create 2 groups for overlays, make sure 1 plane is
> put
> -		 * in each then spread the rest out.
> +		 * Always keep primary and cursor and discard already
> +		 * removed planes
>  		 */
> -		iter_mask |= 1 << 3;
> -		parms[overlays[n_overlays - 1]].mask = 1 << 3;
> +		if (parms[i].mask != GROUP_TYPE_OVERLAY)
> +			continue;
>  
> -		for (i = 1; i < n_overlays - 1; i++) {
> -			int val =
> hars_petruska_f54_1_random_unsafe_max(2);
> +		parms[i].mask = GROUP_TYPE_NONE;

You were arguing that parms[i].mask can't be 0. Now what if you call
now wm_setup_plane with it as a parameter? :)

> +		n_overlays--;
> +	}
>  
> -			parms[overlays[i]].mask = 1 << (2 + val);
> -		}

> +	/*
> +	 * Check if there is enough bandwidth for the current number of
> planes.
> +	 * As the plane size and position is not taken into account we
> can do
> +	 * this earlier.
> +	 */
> +	set_sprite_wh(display, pipe, parms, sprite_fb, alpha,
> sprite_width,
> +		      sprite_height);
> +	wm_setup_plane(display, pipe, iter_mask, parms, false);
> +
> +	/* It should be able to handle at least primary and cursor */
> +	if (!max_overlays) {
> +		iter_mask &= ~GROUP_TYPE_OVERLAY;
> +		*iter_max = iter_mask + 1;
> +		return;
>  	}

You could just take care about this organizing cycle above properly.
Moreover you could bail out, checking that much earlier, thus reducing
the amount of code executed.

>  
> -	igt_create_fb(display->drm_fd, cursor_width, cursor_height,
> -		      DRM_FORMAT_ARGB8888, LOCAL_DRM_FORMAT_MOD_NONE,
> argb_fb);
> +	ret = igt_display_try_commit_atomic(display,
> DRM_MODE_ATOMIC_TEST_ONLY |
> +					    DRM_MODE_ATOMIC_ALLOW_MODES
> ET,
> +					    NULL);
> +	/*
> +	 * Could mean other errors but this is also the error returned
> when
> +	 * there is not enough bandwidth for all the planes
> +	 */
> +	if (ret == -EINVAL) {
> +		max_overlays--;
> +		goto retry_bw;
> +	}

There was a macro for that, in fact I have agreed with Daniel, so 
that we'll have a macro here to see if there is a "correct" return
value or if we need to fail. What if ret is ENOSPACE? Or something
else? We had a bug about this. Now you are going to make it fail
somewhere else again.

Also 

>  
> -	igt_create_fb(display->drm_fd, cursor_width, cursor_height,
> -		      DRM_FORMAT_ARGB8888, LOCAL_DRM_FORMAT_MOD_NONE,
> sprite_fb);
> +	igt_assert_f(!ret, "Error %i not expected by try_commit()\n",
> ret);
>  
> -	*iter_max = iter_mask + 1;
> -	if (!n_overlays)
> -		return;
> +	/* So it have enough bandwidth for n_overlays planes */
>  
>  	/*
> -	 * Pre gen9 not all sizes are supported, find the biggest
> possible
> -	 * size that can be enabled on all sprite planes.
> +	 * Create 2 groups for overlays, make sure 1 plane is put in
> each then
> +	 * spread the rest out.
>  	 */
> -retry:
> -	prev_w = sprite_width = cursor_width;
> -	prev_h = sprite_height = cursor_height;
> +	iter_mask &= ~GROUP_TYPE_OVERLAY;
> +	for_each_plane_on_pipe(display, pipe, plane) {
> +		int i = plane->index;
>  
> -	max_sprite_width = (sprite_width == mode->hdisplay);
> -	max_sprite_height = (sprite_height == mode->vdisplay);
> +		if (parms[i].mask != GROUP_TYPE_OVERLAY)
> +			continue;
>  
> -	while (1) {
> -		int ret;
> +		/* First overlay plane will be overlay group 1 */
> +		if (!(iter_mask & GROUP_TYPE_OVERLAY)) {
> +			iter_mask |= GROUP_TYPE_OVERLAY;
> +			continue;
> +		}
>  
> -		set_sprite_wh(display, pipe, parms, sprite_fb,
> -			      alpha, sprite_width, sprite_height);
> +		/* Second overlay plane will be overlay group 1 */
> +		if (!(iter_mask & GROUP_TYPE_OVERLAY2)) {
> +			iter_mask |= GROUP_TYPE_OVERLAY2;
> +			parms[i].mask = GROUP_TYPE_OVERLAY2;
> +			continue;
> +		}
>  

Why it is so complex? Previously we were setting it only once.
Now you have two more conditions, checking and setting it second time,
introducing more entropy.

Moreover this is very unreadable and hard to check which errors
it can bring. 

> -		wm_setup_plane(display, pipe, (1 << n_planes) - 1,
> parms, false);
> -		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));
> +		/* Sort the group of the rest of overlay planes */
> +		if (hars_petruska_f54_1_random_unsafe_max(2))
> +			parms[i].mask = GROUP_TYPE_OVERLAY2;
> +	}
>  
> -		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;
> -			}
> +	*iter_max = iter_mask + 1;
>  
> +	/*
> +	 * Pre gen9 not all sizes are supported, find the biggest
> possible
> +	 * size that can be enabled on all sprite planes.
> +	 */
> +	while (1) {
> +		/* Size limit reached */
> +		if (is_atomic_check_plane_size_errno(ret)) {
>  			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 {
> -			prev_w = sprite_width;
> -			prev_h = sprite_height;
> +			break;
>  		}
>  
> -		if (!max_sprite_width) {
> -			sprite_width *= 2;
> +		/* Commit is valid and it reached max size, use this
> size */
> +		if (sprite_width == mode->hdisplay ||
> +		    sprite_height == mode->vdisplay)
> +			break;
>  
> -			if (sprite_width >= mode->hdisplay) {
> -				max_sprite_width = true;
> +		prev_w = sprite_width;
> +		prev_h = sprite_height;
>  
> -				sprite_width = mode->hdisplay;
> -			}
> -		} else if (!max_sprite_height) {
> -			sprite_height *= 2;
> +		sprite_width *= 2;
> +		if (sprite_width >= mode->hdisplay)
> +			sprite_width = mode->hdisplay;
>  
> -			if (sprite_height >= mode->vdisplay) {
> -				max_sprite_height = true;
> +		sprite_height *= 2;
> +		if (sprite_height >= mode->vdisplay)
> +			sprite_height = mode->vdisplay;
>  
> -				sprite_height = mode->vdisplay;
> -			}
> -		} else
> -			/* Max sized sprites for all! */
> -			break;
> +		set_sprite_wh(display, pipe, parms, sprite_fb,
> sprite_width,
> +			      sprite_height, alpha);
> +		wm_setup_plane(display, pipe, iter_mask, parms, false);
> +		ret = igt_display_try_commit_atomic(display,
> +						    DRM_MODE_ATOMIC_TES
> T_ONLY |
> +						    DRM_MODE_ATOMIC_ALL
> OW_MODESET,
> +						    NULL);
>  	}
>  
> -	igt_info("Running test on pipe %s with resolution %dx%d and
> sprite size %dx%d alpha %i\n",
> +	igt_info("Running test on pipe %s with resolution %dx%d, sprite
> size %dx%d, alpha %i and %i overlay planes\n",
>  		 kmstest_pipe_name(pipe), mode->hdisplay, mode-
> >vdisplay,
> -		 sprite_width, sprite_height, alpha);
> +		 sprite_width, sprite_height, alpha, n_overlays);
>  }
>  
>  static void prepare_fencing(igt_display_t *display, enum pipe pipe)
> @@ -519,8 +560,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 !=
> GROUP_TYPE_NONE)
> +				igt_plane_set_position(plane, 0, 0);
> +		}
>  
>  		igt_display_commit2(display, COMMIT_ATOMIC);
>  
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

* Re: [igt-dev] [PATCH i-g-t 1/2] HAX: tests/kms_atomic_transition: Don't test more planes than memory bandwidth can support
  2019-04-05 23:31 [igt-dev] [PATCH i-g-t 1/2] HAX: tests/kms_atomic_transition: Don't test more planes than memory bandwidth can support José Roberto de Souza
                   ` (4 preceding siblings ...)
  2019-04-08  8:38 ` [igt-dev] [PATCH i-g-t 1/2] HAX: tests/kms_atomic_transition: Don't test more planes than memory bandwidth can support Lisovskiy, Stanislav
@ 2019-04-08  9:47 ` Lisovskiy, Stanislav
  2019-04-09  1:51   ` Souza, Jose
  5 siblings, 1 reply; 10+ messages in thread
From: Lisovskiy, Stanislav @ 2019-04-08  9:47 UTC (permalink / raw)
  To: igt-dev, Souza, Jose

On Fri, 2019-04-05 at 16:31 -0700, José Roberto de Souza wrote:
> Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  tests/kms_atomic_transition.c | 217 ++++++++++++++++++++----------
> ----
>  1 file changed, 130 insertions(+), 87 deletions(-)
> 
> diff --git a/tests/kms_atomic_transition.c
> b/tests/kms_atomic_transition.c
> index 18f73317..9988f303 100644
> --- a/tests/kms_atomic_transition.c
> +++ b/tests/kms_atomic_transition.c
> @@ -40,9 +40,18 @@
>  #define DRM_CAP_CURSOR_HEIGHT 0x9
>  #endif
>  

Also take a look here: 
https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2807/

This code has just broken all kms_atomic_transition test
cases for some platforms. 
You told that my patch has flaws, didn't explain which and then
sent a patch which has more code, conditions, cycles and introducing
problems... So why is it better?

> +enum group_type {
> +	GROUP_TYPE_NONE = 0,
> +	GROUP_TYPE_PRIMARY = 1 << 0,
> +	GROUP_TYPE_CURSOR = 1 << 1,
> +	GROUP_TYPE_OVERLAY = 1 << 2,
> +	GROUP_TYPE_OVERLAY2 = 1 << 3
> +};
> +
>  struct plane_parms {
>  	struct igt_fb *fb;
> -	uint32_t width, height, mask;
> +	uint32_t width, height;
> +	enum group_type mask;
>  };
>  
>  /* globals for fence support */
> @@ -203,12 +212,11 @@ static void setup_parms(igt_display_t *display,
> enum pipe pipe,
>  			unsigned *iter_max)
>  {
>  	uint64_t cursor_width, cursor_height;
> -	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 sprite_width, sprite_height, prev_w, prev_h;
>  	igt_plane_t *plane;
> -	uint32_t iter_mask = 3;
> +	uint32_t iter_mask = 0, max_overlays, n_overlays = 0;
> +	bool alpha = true;
> +	int ret;
>  
>  	do_or_die(drmGetCap(display->drm_fd, DRM_CAP_CURSOR_WIDTH,
> &cursor_width));
>  	if (cursor_width >= mode->hdisplay)
> @@ -225,123 +233,156 @@ static void setup_parms(igt_display_t
> *display, enum pipe pipe,
>  			parms[i].fb = primary_fb;
>  			parms[i].width = mode->hdisplay;
>  			parms[i].height = mode->vdisplay;
> -			parms[i].mask = 1 << 0;
> +			parms[i].mask = GROUP_TYPE_PRIMARY;
> +			iter_mask |= GROUP_TYPE_PRIMARY;
>  		} else if (plane->type == DRM_PLANE_TYPE_CURSOR) {
>  			parms[i].fb = argb_fb;
>  			parms[i].width = cursor_width;
>  			parms[i].height = cursor_height;
> -			parms[i].mask = 1 << 1;
> +			parms[i].mask = GROUP_TYPE_CURSOR;
> +			iter_mask |= GROUP_TYPE_CURSOR;
>  		} else {
>  			parms[i].fb = sprite_fb;
> -			parms[i].mask = 1 << 2;
> -
> -			iter_mask |= 1 << 2;
> -
> -			overlays[n_overlays++] = i;
> +			parms[i].mask = GROUP_TYPE_OVERLAY;
> +			iter_mask |= GROUP_TYPE_OVERLAY;
> +			n_overlays++;
> +			if (alpha)
> +				alpha = igt_plane_has_format_mod(plane,
> +								 DRM_FO
> RMAT_ARGB8888,
> +								 LOCAL_
> DRM_FORMAT_MOD_NONE);
>  		}
>  	}
>  
> -	if (n_overlays >= 2) {
> -		uint32_t i;
> +	prev_w = sprite_width = cursor_width;
> +	prev_h = sprite_height = cursor_height;
> +
> +	igt_create_fb(display->drm_fd, cursor_width, cursor_height,
> +		      DRM_FORMAT_ARGB8888, LOCAL_DRM_FORMAT_MOD_NONE,
> argb_fb);
> +
> +	igt_create_fb(display->drm_fd, sprite_width, sprite_height,
> +		      alpha ? DRM_FORMAT_ARGB8888 :
> DRM_FORMAT_XRGB8888,
> +		      LOCAL_DRM_FORMAT_MOD_NONE, sprite_fb);
> +
> +	max_overlays = n_overlays;
> +retry_bw:
> +	/* Limit the number of planes */
> +	while (max_overlays < n_overlays) {
> +		int i = hars_petruska_f54_1_random_unsafe_max(display-
> >pipes[pipe].n_planes);
>  
>  		/*
> -		 * Create 2 groups for overlays, make sure 1 plane is
> put
> -		 * in each then spread the rest out.
> +		 * Always keep primary and cursor and discard already
> +		 * removed planes
>  		 */
> -		iter_mask |= 1 << 3;
> -		parms[overlays[n_overlays - 1]].mask = 1 << 3;
> +		if (parms[i].mask != GROUP_TYPE_OVERLAY)
> +			continue;
>  
> -		for (i = 1; i < n_overlays - 1; i++) {
> -			int val =
> hars_petruska_f54_1_random_unsafe_max(2);
> +		parms[i].mask = GROUP_TYPE_NONE;
> +		n_overlays--;
> +	}
>  
> -			parms[overlays[i]].mask = 1 << (2 + val);
> -		}
> +	/*
> +	 * Check if there is enough bandwidth for the current number of
> planes.
> +	 * As the plane size and position is not taken into account we
> can do
> +	 * this earlier.
> +	 */
> +	set_sprite_wh(display, pipe, parms, sprite_fb, alpha,
> sprite_width,
> +		      sprite_height);
> +	wm_setup_plane(display, pipe, iter_mask, parms, false);
> +
> +	/* It should be able to handle at least primary and cursor */
> +	if (!max_overlays) {
> +		iter_mask &= ~GROUP_TYPE_OVERLAY;
> +		*iter_max = iter_mask + 1;
> +		return;
>  	}
>  
> -	igt_create_fb(display->drm_fd, cursor_width, cursor_height,
> -		      DRM_FORMAT_ARGB8888, LOCAL_DRM_FORMAT_MOD_NONE,
> argb_fb);
> +	ret = igt_display_try_commit_atomic(display,
> DRM_MODE_ATOMIC_TEST_ONLY |
> +					    DRM_MODE_ATOMIC_ALLOW_MODES
> ET,
> +					    NULL);
> +	/*
> +	 * Could mean other errors but this is also the error returned
> when
> +	 * there is not enough bandwidth for all the planes
> +	 */
> +	if (ret == -EINVAL) {
> +		max_overlays--;
> +		goto retry_bw;
> +	}
>  
> -	igt_create_fb(display->drm_fd, cursor_width, cursor_height,
> -		      DRM_FORMAT_ARGB8888, LOCAL_DRM_FORMAT_MOD_NONE,
> sprite_fb);
> +	igt_assert_f(!ret, "Error %i not expected by try_commit()\n",
> ret);
>  
> -	*iter_max = iter_mask + 1;
> -	if (!n_overlays)
> -		return;
> +	/* So it have enough bandwidth for n_overlays planes */
>  
>  	/*
> -	 * Pre gen9 not all sizes are supported, find the biggest
> possible
> -	 * size that can be enabled on all sprite planes.
> +	 * Create 2 groups for overlays, make sure 1 plane is put in
> each then
> +	 * spread the rest out.
>  	 */
> -retry:
> -	prev_w = sprite_width = cursor_width;
> -	prev_h = sprite_height = cursor_height;
> +	iter_mask &= ~GROUP_TYPE_OVERLAY;
> +	for_each_plane_on_pipe(display, pipe, plane) {
> +		int i = plane->index;
>  
> -	max_sprite_width = (sprite_width == mode->hdisplay);
> -	max_sprite_height = (sprite_height == mode->vdisplay);
> +		if (parms[i].mask != GROUP_TYPE_OVERLAY)
> +			continue;
>  
> -	while (1) {
> -		int ret;
> +		/* First overlay plane will be overlay group 1 */
> +		if (!(iter_mask & GROUP_TYPE_OVERLAY)) {
> +			iter_mask |= GROUP_TYPE_OVERLAY;
> +			continue;
> +		}
>  
> -		set_sprite_wh(display, pipe, parms, sprite_fb,
> -			      alpha, sprite_width, sprite_height);
> +		/* Second overlay plane will be overlay group 1 */
> +		if (!(iter_mask & GROUP_TYPE_OVERLAY2)) {
> +			iter_mask |= GROUP_TYPE_OVERLAY2;
> +			parms[i].mask = GROUP_TYPE_OVERLAY2;
> +			continue;
> +		}
>  
> -		wm_setup_plane(display, pipe, (1 << n_planes) - 1,
> parms, false);
> -		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));
> +		/* Sort the group of the rest of overlay planes */
> +		if (hars_petruska_f54_1_random_unsafe_max(2))
> +			parms[i].mask = GROUP_TYPE_OVERLAY2;
> +	}
>  
> -		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;
> -			}
> +	*iter_max = iter_mask + 1;
>  
> +	/*
> +	 * Pre gen9 not all sizes are supported, find the biggest
> possible
> +	 * size that can be enabled on all sprite planes.
> +	 */
> +	while (1) {
> +		/* Size limit reached */
> +		if (is_atomic_check_plane_size_errno(ret)) {
>  			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 {
> -			prev_w = sprite_width;
> -			prev_h = sprite_height;
> +			break;
>  		}
>  
> -		if (!max_sprite_width) {
> -			sprite_width *= 2;
> +		/* Commit is valid and it reached max size, use this
> size */
> +		if (sprite_width == mode->hdisplay ||
> +		    sprite_height == mode->vdisplay)
> +			break;
>  
> -			if (sprite_width >= mode->hdisplay) {
> -				max_sprite_width = true;
> +		prev_w = sprite_width;
> +		prev_h = sprite_height;
>  
> -				sprite_width = mode->hdisplay;
> -			}
> -		} else if (!max_sprite_height) {
> -			sprite_height *= 2;
> +		sprite_width *= 2;
> +		if (sprite_width >= mode->hdisplay)
> +			sprite_width = mode->hdisplay;
>  
> -			if (sprite_height >= mode->vdisplay) {
> -				max_sprite_height = true;
> +		sprite_height *= 2;
> +		if (sprite_height >= mode->vdisplay)
> +			sprite_height = mode->vdisplay;
>  
> -				sprite_height = mode->vdisplay;
> -			}
> -		} else
> -			/* Max sized sprites for all! */
> -			break;
> +		set_sprite_wh(display, pipe, parms, sprite_fb,
> sprite_width,
> +			      sprite_height, alpha);
> +		wm_setup_plane(display, pipe, iter_mask, parms, false);
> +		ret = igt_display_try_commit_atomic(display,
> +						    DRM_MODE_ATOMIC_TES
> T_ONLY |
> +						    DRM_MODE_ATOMIC_ALL
> OW_MODESET,
> +						    NULL);
>  	}
>  
> -	igt_info("Running test on pipe %s with resolution %dx%d and
> sprite size %dx%d alpha %i\n",
> +	igt_info("Running test on pipe %s with resolution %dx%d, sprite
> size %dx%d, alpha %i and %i overlay planes\n",
>  		 kmstest_pipe_name(pipe), mode->hdisplay, mode-
> >vdisplay,
> -		 sprite_width, sprite_height, alpha);
> +		 sprite_width, sprite_height, alpha, n_overlays);
>  }
>  
>  static void prepare_fencing(igt_display_t *display, enum pipe pipe)
> @@ -519,8 +560,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 !=
> GROUP_TYPE_NONE)
> +				igt_plane_set_position(plane, 0, 0);
> +		}
>  
>  		igt_display_commit2(display, COMMIT_ATOMIC);
>  
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

* Re: [igt-dev] [PATCH i-g-t 1/2] HAX: tests/kms_atomic_transition: Don't test more planes than memory bandwidth can support
  2019-04-08  8:38 ` [igt-dev] [PATCH i-g-t 1/2] HAX: tests/kms_atomic_transition: Don't test more planes than memory bandwidth can support Lisovskiy, Stanislav
@ 2019-04-09  1:41   ` Souza, Jose
  0 siblings, 0 replies; 10+ messages in thread
From: Souza, Jose @ 2019-04-09  1:41 UTC (permalink / raw)
  To: igt-dev, Lisovskiy, Stanislav


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

On Mon, 2019-04-08 at 09:38 +0100, Lisovskiy, Stanislav wrote:
> On Fri, 2019-04-05 at 16:31 -0700, José Roberto de Souza wrote:
> > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  tests/kms_atomic_transition.c | 217 ++++++++++++++++++++----------
> > ----
> >  1 file changed, 130 insertions(+), 87 deletions(-)
> > 
> > diff --git a/tests/kms_atomic_transition.c
> > b/tests/kms_atomic_transition.c
> > index 18f73317..9988f303 100644
> > --- a/tests/kms_atomic_transition.c
> > +++ b/tests/kms_atomic_transition.c
> > @@ -40,9 +40,18 @@
> >  #define DRM_CAP_CURSOR_HEIGHT 0x9
> >  #endif
> >  
> > +enum group_type {
> > +	GROUP_TYPE_NONE = 0,
> > +	GROUP_TYPE_PRIMARY = 1 << 0,
> > +	GROUP_TYPE_CURSOR = 1 << 1,
> > +	GROUP_TYPE_OVERLAY = 1 << 2,
> > +	GROUP_TYPE_OVERLAY2 = 1 << 3
> > +};
> > +
> >  struct plane_parms {
> >  	struct igt_fb *fb;
> > -	uint32_t width, height, mask;
> > +	uint32_t width, height;
> > +	enum group_type mask;
> >  };
> >  
> >  /* globals for fence support */
> > @@ -203,12 +212,11 @@ static void setup_parms(igt_display_t
> > *display,
> > enum pipe pipe,
> >  			unsigned *iter_max)
> >  {
> >  	uint64_t cursor_width, cursor_height;
> > -	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 sprite_width, sprite_height, prev_w, prev_h;
> >  	igt_plane_t *plane;
> > -	uint32_t iter_mask = 3;
> > +	uint32_t iter_mask = 0, max_overlays, n_overlays = 0;
> > +	bool alpha = true;
> > +	int ret;
> >  
> >  	do_or_die(drmGetCap(display->drm_fd, DRM_CAP_CURSOR_WIDTH,
> > &cursor_width));
> >  	if (cursor_width >= mode->hdisplay)
> > @@ -225,123 +233,156 @@ static void setup_parms(igt_display_t
> > *display, enum pipe pipe,
> >  			parms[i].fb = primary_fb;
> >  			parms[i].width = mode->hdisplay;
> >  			parms[i].height = mode->vdisplay;
> > -			parms[i].mask = 1 << 0;
> > +			parms[i].mask = GROUP_TYPE_PRIMARY;
> > +			iter_mask |= GROUP_TYPE_PRIMARY;
> >  		} else if (plane->type == DRM_PLANE_TYPE_CURSOR) {
> >  			parms[i].fb = argb_fb;
> >  			parms[i].width = cursor_width;
> >  			parms[i].height = cursor_height;
> > -			parms[i].mask = 1 << 1;
> > +			parms[i].mask = GROUP_TYPE_CURSOR;
> > +			iter_mask |= GROUP_TYPE_CURSOR;
> >  		} else {
> >  			parms[i].fb = sprite_fb;
> > -			parms[i].mask = 1 << 2;
> > -
> > -			iter_mask |= 1 << 2;
> > -
> > -			overlays[n_overlays++] = i;
> > +			parms[i].mask = GROUP_TYPE_OVERLAY;
> > +			iter_mask |= GROUP_TYPE_OVERLAY;
> > +			n_overlays++;
> > +			if (alpha)
> > +				alpha = igt_plane_has_format_mod(plane,
> > +								 DRM_FO
> > RMAT_ARGB8888,
> > +								 LOCAL_
> > DRM_FORMAT_MOD_NONE);
> >  		}
> >  	}
> >  
> > -	if (n_overlays >= 2) {
> > -		uint32_t i;
> > +	prev_w = sprite_width = cursor_width;
> > +	prev_h = sprite_height = cursor_height;
> > +
> > +	igt_create_fb(display->drm_fd, cursor_width, cursor_height,
> > +		      DRM_FORMAT_ARGB8888, LOCAL_DRM_FORMAT_MOD_NONE,
> > argb_fb);
> > +
> > +	igt_create_fb(display->drm_fd, sprite_width, sprite_height,
> > +		      alpha ? DRM_FORMAT_ARGB8888 :
> > DRM_FORMAT_XRGB8888,
> > +		      LOCAL_DRM_FORMAT_MOD_NONE, sprite_fb);
> > +
> > +	max_overlays = n_overlays;
> > +retry_bw:
> > +	/* Limit the number of planes */
> > +	while (max_overlays < n_overlays) {
> > +		int i = hars_petruska_f54_1_random_unsafe_max(display-
> > > pipes[pipe].n_planes);
> >  
> >  		/*
> > -		 * Create 2 groups for overlays, make sure 1 plane is
> > put
> > -		 * in each then spread the rest out.
> > +		 * Always keep primary and cursor and discard already
> > +		 * removed planes
> >  		 */
> > -		iter_mask |= 1 << 3;
> > -		parms[overlays[n_overlays - 1]].mask = 1 << 3;
> > +		if (parms[i].mask != GROUP_TYPE_OVERLAY)
> > +			continue;
> >  
> > -		for (i = 1; i < n_overlays - 1; i++) {
> > -			int val =
> > hars_petruska_f54_1_random_unsafe_max(2);
> > +		parms[i].mask = GROUP_TYPE_NONE;
> 
> You were arguing that parms[i].mask can't be 0. Now what if you call
> now wm_setup_plane with it as a parameter? :)

There is no problem in have mask = 0, the problem is say that is mask =
0 in the commit message but that don't happen.

> 
> > +		n_overlays--;
> > +	}
> >  
> > -			parms[overlays[i]].mask = 1 << (2 + val);
> > -		}
> > +	/*
> > +	 * Check if there is enough bandwidth for the current number of
> > planes.
> > +	 * As the plane size and position is not taken into account we
> > can do
> > +	 * this earlier.
> > +	 */
> > +	set_sprite_wh(display, pipe, parms, sprite_fb, alpha,
> > sprite_width,
> > +		      sprite_height);
> > +	wm_setup_plane(display, pipe, iter_mask, parms, false);
> > +
> > +	/* It should be able to handle at least primary and cursor */
> > +	if (!max_overlays) {
> > +		iter_mask &= ~GROUP_TYPE_OVERLAY;
> > +		*iter_max = iter_mask + 1;
> > +		return;
> >  	}
> 
> You could just take care about this organizing cycle above properly.
> Moreover you could bail out, checking that much earlier, thus
> reducing
> the amount of code executed.

The calls to set_sprite_wh() and wm_setup_plane() in !max_overlays
conditions takes care of free the framebuffers.

> 
> >  
> > -	igt_create_fb(display->drm_fd, cursor_width, cursor_height,
> > -		      DRM_FORMAT_ARGB8888, LOCAL_DRM_FORMAT_MOD_NONE,
> > argb_fb);
> > +	ret = igt_display_try_commit_atomic(display,
> > DRM_MODE_ATOMIC_TEST_ONLY |
> > +					    DRM_MODE_ATOMIC_ALLOW_MODES
> > ET,
> > +					    NULL);
> > +	/*
> > +	 * Could mean other errors but this is also the error returned
> > when
> > +	 * there is not enough bandwidth for all the planes
> > +	 */
> > +	if (ret == -EINVAL) {
> > +		max_overlays--;
> > +		goto retry_bw;
> > +	}
> 
> There was a macro for that, in fact I have agreed with Daniel, so 
> that we'll have a macro here to see if there is a "correct" return
> value or if we need to fail. What if ret is ENOSPACE? Or something
> else? We had a bug about this. Now you are going to make it fail
> somewhere else again.	

Yeah it should return a error for anything != 0 after check for
-EINVAL.

> 
> Also 
> 
> >  
> > -	igt_create_fb(display->drm_fd, cursor_width, cursor_height,
> > -		      DRM_FORMAT_ARGB8888, LOCAL_DRM_FORMAT_MOD_NONE,
> > sprite_fb);
> > +	igt_assert_f(!ret, "Error %i not expected by try_commit()\n",
> > ret);
> >  
> > -	*iter_max = iter_mask + 1;
> > -	if (!n_overlays)
> > -		return;
> > +	/* So it have enough bandwidth for n_overlays planes */
> >  
> >  	/*
> > -	 * Pre gen9 not all sizes are supported, find the biggest
> > possible
> > -	 * size that can be enabled on all sprite planes.
> > +	 * Create 2 groups for overlays, make sure 1 plane is put in
> > each then
> > +	 * spread the rest out.
> >  	 */
> > -retry:
> > -	prev_w = sprite_width = cursor_width;
> > -	prev_h = sprite_height = cursor_height;
> > +	iter_mask &= ~GROUP_TYPE_OVERLAY;
> > +	for_each_plane_on_pipe(display, pipe, plane) {
> > +		int i = plane->index;
> >  
> > -	max_sprite_width = (sprite_width == mode->hdisplay);
> > -	max_sprite_height = (sprite_height == mode->vdisplay);
> > +		if (parms[i].mask != GROUP_TYPE_OVERLAY)
> > +			continue;
> >  
> > -	while (1) {
> > -		int ret;
> > +		/* First overlay plane will be overlay group 1 */
> > +		if (!(iter_mask & GROUP_TYPE_OVERLAY)) {
> > +			iter_mask |= GROUP_TYPE_OVERLAY;
> > +			continue;
> > +		}
> >  
> > -		set_sprite_wh(display, pipe, parms, sprite_fb,
> > -			      alpha, sprite_width, sprite_height);
> > +		/* Second overlay plane will be overlay group 1 */
> > +		if (!(iter_mask & GROUP_TYPE_OVERLAY2)) {
> > +			iter_mask |= GROUP_TYPE_OVERLAY2;
> > +			parms[i].mask = GROUP_TYPE_OVERLAY2;
> > +			continue;
> > +		}
> >  
> 
> Why it is so complex? Previously we were setting it only once.
> Now you have two more conditions, checking and setting it second
> time,
> introducing more entropy.

Here it is only randomizing the groups once, after the number of planes
that fits in BW is founds.

> 
> Moreover this is very unreadable and hard to check which errors
> it can bring. 
> 
> > -		wm_setup_plane(display, pipe, (1 << n_planes) - 1,
> > parms, false);
> > -		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));
> > +		/* Sort the group of the rest of overlay planes */
> > +		if (hars_petruska_f54_1_random_unsafe_max(2))
> > +			parms[i].mask = GROUP_TYPE_OVERLAY2;
> > +	}
> >  
> > -		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;
> > -			}
> > +	*iter_max = iter_mask + 1;
> >  
> > +	/*
> > +	 * Pre gen9 not all sizes are supported, find the biggest
> > possible
> > +	 * size that can be enabled on all sprite planes.
> > +	 */
> > +	while (1) {
> > +		/* Size limit reached */
> > +		if (is_atomic_check_plane_size_errno(ret)) {
> >  			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 {
> > -			prev_w = sprite_width;
> > -			prev_h = sprite_height;
> > +			break;
> >  		}
> >  
> > -		if (!max_sprite_width) {
> > -			sprite_width *= 2;
> > +		/* Commit is valid and it reached max size, use this
> > size */
> > +		if (sprite_width == mode->hdisplay ||
> > +		    sprite_height == mode->vdisplay)
> > +			break;
> >  
> > -			if (sprite_width >= mode->hdisplay) {
> > -				max_sprite_width = true;
> > +		prev_w = sprite_width;
> > +		prev_h = sprite_height;
> >  
> > -				sprite_width = mode->hdisplay;
> > -			}
> > -		} else if (!max_sprite_height) {
> > -			sprite_height *= 2;
> > +		sprite_width *= 2;
> > +		if (sprite_width >= mode->hdisplay)
> > +			sprite_width = mode->hdisplay;
> >  
> > -			if (sprite_height >= mode->vdisplay) {
> > -				max_sprite_height = true;
> > +		sprite_height *= 2;
> > +		if (sprite_height >= mode->vdisplay)
> > +			sprite_height = mode->vdisplay;
> >  
> > -				sprite_height = mode->vdisplay;
> > -			}
> > -		} else
> > -			/* Max sized sprites for all! */
> > -			break;
> > +		set_sprite_wh(display, pipe, parms, sprite_fb,
> > sprite_width,
> > +			      sprite_height, alpha);
> > +		wm_setup_plane(display, pipe, iter_mask, parms, false);
> > +		ret = igt_display_try_commit_atomic(display,
> > +						    DRM_MODE_ATOMIC_TES
> > T_ONLY |
> > +						    DRM_MODE_ATOMIC_ALL
> > OW_MODESET,
> > +						    NULL);
> >  	}
> >  
> > -	igt_info("Running test on pipe %s with resolution %dx%d and
> > sprite size %dx%d alpha %i\n",
> > +	igt_info("Running test on pipe %s with resolution %dx%d, sprite
> > size %dx%d, alpha %i and %i overlay planes\n",
> >  		 kmstest_pipe_name(pipe), mode->hdisplay, mode-
> > > vdisplay,
> > -		 sprite_width, sprite_height, alpha);
> > +		 sprite_width, sprite_height, alpha, n_overlays);
> >  }
> >  
> >  static void prepare_fencing(igt_display_t *display, enum pipe
> > pipe)
> > @@ -519,8 +560,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 !=
> > GROUP_TYPE_NONE)
> > +				igt_plane_set_position(plane, 0, 0);
> > +		}
> >  
> >  		igt_display_commit2(display, COMMIT_ATOMIC);
> >  

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

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

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

* Re: [igt-dev] [PATCH i-g-t 1/2] HAX: tests/kms_atomic_transition: Don't test more planes than memory bandwidth can support
  2019-04-08  9:47 ` Lisovskiy, Stanislav
@ 2019-04-09  1:51   ` Souza, Jose
  2019-04-09  7:27     ` Lisovskiy, Stanislav
  0 siblings, 1 reply; 10+ messages in thread
From: Souza, Jose @ 2019-04-09  1:51 UTC (permalink / raw)
  To: igt-dev, Lisovskiy, Stanislav


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

On Mon, 2019-04-08 at 10:47 +0100, Lisovskiy, Stanislav wrote:
> On Fri, 2019-04-05 at 16:31 -0700, José Roberto de Souza wrote:
> > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  tests/kms_atomic_transition.c | 217 ++++++++++++++++++++----------
> > ----
> >  1 file changed, 130 insertions(+), 87 deletions(-)
> > 
> > diff --git a/tests/kms_atomic_transition.c
> > b/tests/kms_atomic_transition.c
> > index 18f73317..9988f303 100644
> > --- a/tests/kms_atomic_transition.c
> > +++ b/tests/kms_atomic_transition.c
> > @@ -40,9 +40,18 @@
> >  #define DRM_CAP_CURSOR_HEIGHT 0x9
> >  #endif
> >  
> 
> Also take a look here: 
> https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2807/
> 
> This code has just broken all kms_atomic_transition test
> cases for some platforms. 
> You told that my patch has flaws, didn't explain which and then
> sent a patch which has more code, conditions, cycles and introducing
> problems... So why is it better?

I did not properly tested this it was more like a proof of concept,
that is why the "HAX" in the commit message.

It was more to show that we don't that first patch from your series at
all, we should not just skip the whole test iteration because because
wm_setup_plane() == 0.

Fell free to take some ideas from this patch or not.

Anyways I found the problem and now it is passing: 
https://patchwork.freedesktop.org/series/59191/

> 
> > +enum group_type {
> > +	GROUP_TYPE_NONE = 0,
> > +	GROUP_TYPE_PRIMARY = 1 << 0,
> > +	GROUP_TYPE_CURSOR = 1 << 1,
> > +	GROUP_TYPE_OVERLAY = 1 << 2,
> > +	GROUP_TYPE_OVERLAY2 = 1 << 3
> > +};
> > +
> >  struct plane_parms {
> >  	struct igt_fb *fb;
> > -	uint32_t width, height, mask;
> > +	uint32_t width, height;
> > +	enum group_type mask;
> >  };
> >  
> >  /* globals for fence support */
> > @@ -203,12 +212,11 @@ static void setup_parms(igt_display_t
> > *display,
> > enum pipe pipe,
> >  			unsigned *iter_max)
> >  {
> >  	uint64_t cursor_width, cursor_height;
> > -	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 sprite_width, sprite_height, prev_w, prev_h;
> >  	igt_plane_t *plane;
> > -	uint32_t iter_mask = 3;
> > +	uint32_t iter_mask = 0, max_overlays, n_overlays = 0;
> > +	bool alpha = true;
> > +	int ret;
> >  
> >  	do_or_die(drmGetCap(display->drm_fd, DRM_CAP_CURSOR_WIDTH,
> > &cursor_width));
> >  	if (cursor_width >= mode->hdisplay)
> > @@ -225,123 +233,156 @@ static void setup_parms(igt_display_t
> > *display, enum pipe pipe,
> >  			parms[i].fb = primary_fb;
> >  			parms[i].width = mode->hdisplay;
> >  			parms[i].height = mode->vdisplay;
> > -			parms[i].mask = 1 << 0;
> > +			parms[i].mask = GROUP_TYPE_PRIMARY;
> > +			iter_mask |= GROUP_TYPE_PRIMARY;
> >  		} else if (plane->type == DRM_PLANE_TYPE_CURSOR) {
> >  			parms[i].fb = argb_fb;
> >  			parms[i].width = cursor_width;
> >  			parms[i].height = cursor_height;
> > -			parms[i].mask = 1 << 1;
> > +			parms[i].mask = GROUP_TYPE_CURSOR;
> > +			iter_mask |= GROUP_TYPE_CURSOR;
> >  		} else {
> >  			parms[i].fb = sprite_fb;
> > -			parms[i].mask = 1 << 2;
> > -
> > -			iter_mask |= 1 << 2;
> > -
> > -			overlays[n_overlays++] = i;
> > +			parms[i].mask = GROUP_TYPE_OVERLAY;
> > +			iter_mask |= GROUP_TYPE_OVERLAY;
> > +			n_overlays++;
> > +			if (alpha)
> > +				alpha = igt_plane_has_format_mod(plane,
> > +								 DRM_FO
> > RMAT_ARGB8888,
> > +								 LOCAL_
> > DRM_FORMAT_MOD_NONE);
> >  		}
> >  	}
> >  
> > -	if (n_overlays >= 2) {
> > -		uint32_t i;
> > +	prev_w = sprite_width = cursor_width;
> > +	prev_h = sprite_height = cursor_height;
> > +
> > +	igt_create_fb(display->drm_fd, cursor_width, cursor_height,
> > +		      DRM_FORMAT_ARGB8888, LOCAL_DRM_FORMAT_MOD_NONE,
> > argb_fb);
> > +
> > +	igt_create_fb(display->drm_fd, sprite_width, sprite_height,
> > +		      alpha ? DRM_FORMAT_ARGB8888 :
> > DRM_FORMAT_XRGB8888,
> > +		      LOCAL_DRM_FORMAT_MOD_NONE, sprite_fb);
> > +
> > +	max_overlays = n_overlays;
> > +retry_bw:
> > +	/* Limit the number of planes */
> > +	while (max_overlays < n_overlays) {
> > +		int i = hars_petruska_f54_1_random_unsafe_max(display-
> > > pipes[pipe].n_planes);
> >  
> >  		/*
> > -		 * Create 2 groups for overlays, make sure 1 plane is
> > put
> > -		 * in each then spread the rest out.
> > +		 * Always keep primary and cursor and discard already
> > +		 * removed planes
> >  		 */
> > -		iter_mask |= 1 << 3;
> > -		parms[overlays[n_overlays - 1]].mask = 1 << 3;
> > +		if (parms[i].mask != GROUP_TYPE_OVERLAY)
> > +			continue;
> >  
> > -		for (i = 1; i < n_overlays - 1; i++) {
> > -			int val =
> > hars_petruska_f54_1_random_unsafe_max(2);
> > +		parms[i].mask = GROUP_TYPE_NONE;
> > +		n_overlays--;
> > +	}
> >  
> > -			parms[overlays[i]].mask = 1 << (2 + val);
> > -		}
> > +	/*
> > +	 * Check if there is enough bandwidth for the current number of
> > planes.
> > +	 * As the plane size and position is not taken into account we
> > can do
> > +	 * this earlier.
> > +	 */
> > +	set_sprite_wh(display, pipe, parms, sprite_fb, alpha,
> > sprite_width,
> > +		      sprite_height);
> > +	wm_setup_plane(display, pipe, iter_mask, parms, false);
> > +
> > +	/* It should be able to handle at least primary and cursor */
> > +	if (!max_overlays) {
> > +		iter_mask &= ~GROUP_TYPE_OVERLAY;
> > +		*iter_max = iter_mask + 1;
> > +		return;
> >  	}
> >  
> > -	igt_create_fb(display->drm_fd, cursor_width, cursor_height,
> > -		      DRM_FORMAT_ARGB8888, LOCAL_DRM_FORMAT_MOD_NONE,
> > argb_fb);
> > +	ret = igt_display_try_commit_atomic(display,
> > DRM_MODE_ATOMIC_TEST_ONLY |
> > +					    DRM_MODE_ATOMIC_ALLOW_MODES
> > ET,
> > +					    NULL);
> > +	/*
> > +	 * Could mean other errors but this is also the error returned
> > when
> > +	 * there is not enough bandwidth for all the planes
> > +	 */
> > +	if (ret == -EINVAL) {
> > +		max_overlays--;
> > +		goto retry_bw;
> > +	}
> >  
> > -	igt_create_fb(display->drm_fd, cursor_width, cursor_height,
> > -		      DRM_FORMAT_ARGB8888, LOCAL_DRM_FORMAT_MOD_NONE,
> > sprite_fb);
> > +	igt_assert_f(!ret, "Error %i not expected by try_commit()\n",
> > ret);
> >  
> > -	*iter_max = iter_mask + 1;
> > -	if (!n_overlays)
> > -		return;
> > +	/* So it have enough bandwidth for n_overlays planes */
> >  
> >  	/*
> > -	 * Pre gen9 not all sizes are supported, find the biggest
> > possible
> > -	 * size that can be enabled on all sprite planes.
> > +	 * Create 2 groups for overlays, make sure 1 plane is put in
> > each then
> > +	 * spread the rest out.
> >  	 */
> > -retry:
> > -	prev_w = sprite_width = cursor_width;
> > -	prev_h = sprite_height = cursor_height;
> > +	iter_mask &= ~GROUP_TYPE_OVERLAY;
> > +	for_each_plane_on_pipe(display, pipe, plane) {
> > +		int i = plane->index;
> >  
> > -	max_sprite_width = (sprite_width == mode->hdisplay);
> > -	max_sprite_height = (sprite_height == mode->vdisplay);
> > +		if (parms[i].mask != GROUP_TYPE_OVERLAY)
> > +			continue;
> >  
> > -	while (1) {
> > -		int ret;
> > +		/* First overlay plane will be overlay group 1 */
> > +		if (!(iter_mask & GROUP_TYPE_OVERLAY)) {
> > +			iter_mask |= GROUP_TYPE_OVERLAY;
> > +			continue;
> > +		}
> >  
> > -		set_sprite_wh(display, pipe, parms, sprite_fb,
> > -			      alpha, sprite_width, sprite_height);
> > +		/* Second overlay plane will be overlay group 1 */
> > +		if (!(iter_mask & GROUP_TYPE_OVERLAY2)) {
> > +			iter_mask |= GROUP_TYPE_OVERLAY2;
> > +			parms[i].mask = GROUP_TYPE_OVERLAY2;
> > +			continue;
> > +		}
> >  
> > -		wm_setup_plane(display, pipe, (1 << n_planes) - 1,
> > parms, false);
> > -		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));
> > +		/* Sort the group of the rest of overlay planes */
> > +		if (hars_petruska_f54_1_random_unsafe_max(2))
> > +			parms[i].mask = GROUP_TYPE_OVERLAY2;
> > +	}
> >  
> > -		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;
> > -			}
> > +	*iter_max = iter_mask + 1;
> >  
> > +	/*
> > +	 * Pre gen9 not all sizes are supported, find the biggest
> > possible
> > +	 * size that can be enabled on all sprite planes.
> > +	 */
> > +	while (1) {
> > +		/* Size limit reached */
> > +		if (is_atomic_check_plane_size_errno(ret)) {
> >  			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 {
> > -			prev_w = sprite_width;
> > -			prev_h = sprite_height;
> > +			break;
> >  		}
> >  
> > -		if (!max_sprite_width) {
> > -			sprite_width *= 2;
> > +		/* Commit is valid and it reached max size, use this
> > size */
> > +		if (sprite_width == mode->hdisplay ||
> > +		    sprite_height == mode->vdisplay)
> > +			break;
> >  
> > -			if (sprite_width >= mode->hdisplay) {
> > -				max_sprite_width = true;
> > +		prev_w = sprite_width;
> > +		prev_h = sprite_height;
> >  
> > -				sprite_width = mode->hdisplay;
> > -			}
> > -		} else if (!max_sprite_height) {
> > -			sprite_height *= 2;
> > +		sprite_width *= 2;
> > +		if (sprite_width >= mode->hdisplay)
> > +			sprite_width = mode->hdisplay;
> >  
> > -			if (sprite_height >= mode->vdisplay) {
> > -				max_sprite_height = true;
> > +		sprite_height *= 2;
> > +		if (sprite_height >= mode->vdisplay)
> > +			sprite_height = mode->vdisplay;
> >  
> > -				sprite_height = mode->vdisplay;
> > -			}
> > -		} else
> > -			/* Max sized sprites for all! */
> > -			break;
> > +		set_sprite_wh(display, pipe, parms, sprite_fb,
> > sprite_width,
> > +			      sprite_height, alpha);
> > +		wm_setup_plane(display, pipe, iter_mask, parms, false);
> > +		ret = igt_display_try_commit_atomic(display,
> > +						    DRM_MODE_ATOMIC_TES
> > T_ONLY |
> > +						    DRM_MODE_ATOMIC_ALL
> > OW_MODESET,
> > +						    NULL);
> >  	}
> >  
> > -	igt_info("Running test on pipe %s with resolution %dx%d and
> > sprite size %dx%d alpha %i\n",
> > +	igt_info("Running test on pipe %s with resolution %dx%d, sprite
> > size %dx%d, alpha %i and %i overlay planes\n",
> >  		 kmstest_pipe_name(pipe), mode->hdisplay, mode-
> > > vdisplay,
> > -		 sprite_width, sprite_height, alpha);
> > +		 sprite_width, sprite_height, alpha, n_overlays);
> >  }
> >  
> >  static void prepare_fencing(igt_display_t *display, enum pipe
> > pipe)
> > @@ -519,8 +560,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 !=
> > GROUP_TYPE_NONE)
> > +				igt_plane_set_position(plane, 0, 0);
> > +		}
> >  
> >  		igt_display_commit2(display, COMMIT_ATOMIC);
> >  

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

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

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

* Re: [igt-dev] [PATCH i-g-t 1/2] HAX: tests/kms_atomic_transition: Don't test more planes than memory bandwidth can support
  2019-04-09  1:51   ` Souza, Jose
@ 2019-04-09  7:27     ` Lisovskiy, Stanislav
  0 siblings, 0 replies; 10+ messages in thread
From: Lisovskiy, Stanislav @ 2019-04-09  7:27 UTC (permalink / raw)
  To: igt-dev, Souza, Jose; +Cc: Syrjala, Ville, Peres, Martin

On Mon, 2019-04-08 at 18:51 -0700, Souza, Jose wrote:
> On Mon, 2019-04-08 at 10:47 +0100, Lisovskiy, Stanislav wrote:
> > On Fri, 2019-04-05 at 16:31 -0700, José Roberto de Souza wrote:
> > > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > >  tests/kms_atomic_transition.c | 217 ++++++++++++++++++++------
> > > ----
> > > ----
> > >  1 file changed, 130 insertions(+), 87 deletions(-)
> > > 
> > > diff --git a/tests/kms_atomic_transition.c
> > > b/tests/kms_atomic_transition.c
> > > index 18f73317..9988f303 100644
> > > --- a/tests/kms_atomic_transition.c
> > > +++ b/tests/kms_atomic_transition.c
> > > @@ -40,9 +40,18 @@
> > >  #define DRM_CAP_CURSOR_HEIGHT 0x9
> > >  #endif
> > >  
> > 
> > Also take a look here: 
> > https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2807/
> > 
> > This code has just broken all kms_atomic_transition test
> > cases for some platforms. 
> > You told that my patch has flaws, didn't explain which and then
> > sent a patch which has more code, conditions, cycles and
> > introducing
> > problems... So why is it better?
> 
> I did not properly tested this it was more like a proof of concept,
> that is why the "HAX" in the commit message.

Ok,

> 
> It was more to show that we don't that first patch from your series
> at
> all, we should not just skip the whole test iteration because because
> wm_setup_plane() == 0.
> 
> Fell free to take some ideas from this patch or not.
> 
> Anyways I found the problem and now it is passing: 
> https://patchwork.freedesktop.org/series/59191/


Well if you, or anyone is in doubt, just copy-paste this code to
run_transition test in kms_atomic_transition.c:

parms[0].mask = 0;  /* GROUP_TYPE_NONE in your patch */

wm_setup_plane(display, pipe, 0, parms, false);

atomic_commit(display, pipe, flags, (void *)(unsigned long)0, false);
wait_for_transition(display, pipe, false, false);

wm_setup_plane(display, pipe, 1, parms, false);

atomic_commit(display, pipe, flags, (void *)(unsigned long)1, false);
wait_for_transition(display, pipe, false, false);

Here we could have i = 0, j = 1 in run_transition.

Reproduces the below issue with 100% reproduction rate:

(kms_atomic_transition:3227) CRITICAL: Test assertion failure function
wait_for_transition, file ../tests/kms_atomic_transition.c:431:
(kms_atomic_transition:3227) CRITICAL: Failed assertion:
fd_completed(display->drm_fd)
Stack trace:
  #0 ../lib/igt_core.c:1474 __igt_fail_assert()
  #1 ../tests/kms_atomic_transition.c:433 wait_for_transition()
  #2 ../tests/kms_atomic_transition.c:540 run_transition_test()
  #3 ../tests/kms_atomic_transition.c:966 __real_main933()
  #4 ../tests/kms_atomic_transition.c:933 main()
  #5 ../csu/libc-start.c:344 __libc_start_main()
  #6 [_start+0x2a]
Subtest plane-all-transition-nonblocking failed.

Or this one(i=1, j=3):

parms[0].mask = 1<<2;
parms[1].mask = 0;  /* (GROUP_TYPE_NONE in your patch) */

wm_setup_plane(display, pipe, 1, parms, false);

atomic_commit(display, pipe, flags, (void *)(unsigned long)1, false);
wait_for_transition(display, pipe, false, false);

wm_setup_plane(display, pipe, 3, parms, false);

atomic_commit(display, pipe, flags, (void *)(unsigned long)3, false);
wait_for_transition(display, pipe, false, false);

Again gives:

(kms_atomic_transition:3227) CRITICAL: Test assertion failure function
wait_for_transition, file ../tests/kms_atomic_transition.c:431:
(kms_atomic_transition:3227) CRITICAL: Failed assertion:
fd_completed(display->drm_fd)
Stack trace:
  #0 ../lib/igt_core.c:1474 __igt_fail_assert()
  #1 ../tests/kms_atomic_transition.c:433 wait_for_transition()
  #2 ../tests/kms_atomic_transition.c:540 run_transition_test()
  #3 ../tests/kms_atomic_transition.c:966 __real_main933()
  #4 ../tests/kms_atomic_transition.c:933 main()
  #5 ../csu/libc-start.c:344 __libc_start_main()
  #6 [_start+0x2a]
Subtest plane-all-transition-nonblocking failed.

Note, this might not be visible in tests or visible from time to time,
depending which planes got removed, for which i were parms[i].mask set,
i.e just a matter of luck or until someone changes the transition
algorithm. 

So should we really wait until shit hits the fan?

> 
> > 
> > > +enum group_type {
> > > +	GROUP_TYPE_NONE = 0,
> > > +	GROUP_TYPE_PRIMARY = 1 << 0,
> > > +	GROUP_TYPE_CURSOR = 1 << 1,
> > > +	GROUP_TYPE_OVERLAY = 1 << 2,
> > > +	GROUP_TYPE_OVERLAY2 = 1 << 3
> > > +};
> > > +
> > >  struct plane_parms {
> > >  	struct igt_fb *fb;
> > > -	uint32_t width, height, mask;
> > > +	uint32_t width, height;
> > > +	enum group_type mask;
> > >  };
> > >  
> > >  /* globals for fence support */
> > > @@ -203,12 +212,11 @@ static void setup_parms(igt_display_t
> > > *display,
> > > enum pipe pipe,
> > >  			unsigned *iter_max)
> > >  {
> > >  	uint64_t cursor_width, cursor_height;
> > > -	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 sprite_width, sprite_height, prev_w, prev_h;
> > >  	igt_plane_t *plane;
> > > -	uint32_t iter_mask = 3;
> > > +	uint32_t iter_mask = 0, max_overlays, n_overlays = 0;
> > > +	bool alpha = true;
> > > +	int ret;
> > >  
> > >  	do_or_die(drmGetCap(display->drm_fd, DRM_CAP_CURSOR_WIDTH,
> > > &cursor_width));
> > >  	if (cursor_width >= mode->hdisplay)
> > > @@ -225,123 +233,156 @@ static void setup_parms(igt_display_t
> > > *display, enum pipe pipe,
> > >  			parms[i].fb = primary_fb;
> > >  			parms[i].width = mode->hdisplay;
> > >  			parms[i].height = mode->vdisplay;
> > > -			parms[i].mask = 1 << 0;
> > > +			parms[i].mask = GROUP_TYPE_PRIMARY;
> > > +			iter_mask |= GROUP_TYPE_PRIMARY;
> > >  		} else if (plane->type == DRM_PLANE_TYPE_CURSOR) {
> > >  			parms[i].fb = argb_fb;
> > >  			parms[i].width = cursor_width;
> > >  			parms[i].height = cursor_height;
> > > -			parms[i].mask = 1 << 1;
> > > +			parms[i].mask = GROUP_TYPE_CURSOR;
> > > +			iter_mask |= GROUP_TYPE_CURSOR;
> > >  		} else {
> > >  			parms[i].fb = sprite_fb;
> > > -			parms[i].mask = 1 << 2;
> > > -
> > > -			iter_mask |= 1 << 2;
> > > -
> > > -			overlays[n_overlays++] = i;
> > > +			parms[i].mask = GROUP_TYPE_OVERLAY;
> > > +			iter_mask |= GROUP_TYPE_OVERLAY;
> > > +			n_overlays++;
> > > +			if (alpha)
> > > +				alpha = igt_plane_has_format_mod(plane,
> > > +								 DRM_FO
> > > RMAT_ARGB8888,
> > > +								 LOCAL_
> > > DRM_FORMAT_MOD_NONE);
> > >  		}
> > >  	}
> > >  
> > > -	if (n_overlays >= 2) {
> > > -		uint32_t i;
> > > +	prev_w = sprite_width = cursor_width;
> > > +	prev_h = sprite_height = cursor_height;
> > > +
> > > +	igt_create_fb(display->drm_fd, cursor_width, cursor_height,
> > > +		      DRM_FORMAT_ARGB8888, LOCAL_DRM_FORMAT_MOD_NONE,
> > > argb_fb);
> > > +
> > > +	igt_create_fb(display->drm_fd, sprite_width, sprite_height,
> > > +		      alpha ? DRM_FORMAT_ARGB8888 :
> > > DRM_FORMAT_XRGB8888,
> > > +		      LOCAL_DRM_FORMAT_MOD_NONE, sprite_fb);
> > > +
> > > +	max_overlays = n_overlays;
> > > +retry_bw:
> > > +	/* Limit the number of planes */
> > > +	while (max_overlays < n_overlays) {
> > > +		int i = hars_petruska_f54_1_random_unsafe_max(display-
> > > > pipes[pipe].n_planes);
> > > 
> > >  
> > >  		/*
> > > -		 * Create 2 groups for overlays, make sure 1 plane is
> > > put
> > > -		 * in each then spread the rest out.
> > > +		 * Always keep primary and cursor and discard already
> > > +		 * removed planes
> > >  		 */
> > > -		iter_mask |= 1 << 3;
> > > -		parms[overlays[n_overlays - 1]].mask = 1 << 3;
> > > +		if (parms[i].mask != GROUP_TYPE_OVERLAY)
> > > +			continue;
> > >  
> > > -		for (i = 1; i < n_overlays - 1; i++) {
> > > -			int val =
> > > hars_petruska_f54_1_random_unsafe_max(2);
> > > +		parms[i].mask = GROUP_TYPE_NONE;
> > > +		n_overlays--;
> > > +	}
> > >  
> > > -			parms[overlays[i]].mask = 1 << (2 + val);
> > > -		}
> > > +	/*
> > > +	 * Check if there is enough bandwidth for the current number of
> > > planes.
> > > +	 * As the plane size and position is not taken into account we
> > > can do
> > > +	 * this earlier.
> > > +	 */
> > > +	set_sprite_wh(display, pipe, parms, sprite_fb, alpha,
> > > sprite_width,
> > > +		      sprite_height);
> > > +	wm_setup_plane(display, pipe, iter_mask, parms, false);
> > > +
> > > +	/* It should be able to handle at least primary and cursor */
> > > +	if (!max_overlays) {
> > > +		iter_mask &= ~GROUP_TYPE_OVERLAY;
> > > +		*iter_max = iter_mask + 1;
> > > +		return;
> > >  	}
> > >  
> > > -	igt_create_fb(display->drm_fd, cursor_width, cursor_height,
> > > -		      DRM_FORMAT_ARGB8888, LOCAL_DRM_FORMAT_MOD_NONE,
> > > argb_fb);
> > > +	ret = igt_display_try_commit_atomic(display,
> > > DRM_MODE_ATOMIC_TEST_ONLY |
> > > +					    DRM_MODE_ATOMIC_ALLOW_MODES
> > > ET,
> > > +					    NULL);
> > > +	/*
> > > +	 * Could mean other errors but this is also the error returned
> > > when
> > > +	 * there is not enough bandwidth for all the planes
> > > +	 */
> > > +	if (ret == -EINVAL) {
> > > +		max_overlays--;
> > > +		goto retry_bw;
> > > +	}
> > >  
> > > -	igt_create_fb(display->drm_fd, cursor_width, cursor_height,
> > > -		      DRM_FORMAT_ARGB8888, LOCAL_DRM_FORMAT_MOD_NONE,
> > > sprite_fb);
> > > +	igt_assert_f(!ret, "Error %i not expected by try_commit()\n",
> > > ret);
> > >  
> > > -	*iter_max = iter_mask + 1;
> > > -	if (!n_overlays)
> > > -		return;
> > > +	/* So it have enough bandwidth for n_overlays planes */
> > >  
> > >  	/*
> > > -	 * Pre gen9 not all sizes are supported, find the biggest
> > > possible
> > > -	 * size that can be enabled on all sprite planes.
> > > +	 * Create 2 groups for overlays, make sure 1 plane is put in
> > > each then
> > > +	 * spread the rest out.
> > >  	 */
> > > -retry:
> > > -	prev_w = sprite_width = cursor_width;
> > > -	prev_h = sprite_height = cursor_height;
> > > +	iter_mask &= ~GROUP_TYPE_OVERLAY;
> > > +	for_each_plane_on_pipe(display, pipe, plane) {
> > > +		int i = plane->index;
> > >  
> > > -	max_sprite_width = (sprite_width == mode->hdisplay);
> > > -	max_sprite_height = (sprite_height == mode->vdisplay);
> > > +		if (parms[i].mask != GROUP_TYPE_OVERLAY)
> > > +			continue;
> > >  
> > > -	while (1) {
> > > -		int ret;
> > > +		/* First overlay plane will be overlay group 1 */
> > > +		if (!(iter_mask & GROUP_TYPE_OVERLAY)) {
> > > +			iter_mask |= GROUP_TYPE_OVERLAY;
> > > +			continue;
> > > +		}
> > >  
> > > -		set_sprite_wh(display, pipe, parms, sprite_fb,
> > > -			      alpha, sprite_width, sprite_height);
> > > +		/* Second overlay plane will be overlay group 1 */
> > > +		if (!(iter_mask & GROUP_TYPE_OVERLAY2)) {
> > > +			iter_mask |= GROUP_TYPE_OVERLAY2;
> > > +			parms[i].mask = GROUP_TYPE_OVERLAY2;
> > > +			continue;
> > > +		}
> > >  
> > > -		wm_setup_plane(display, pipe, (1 << n_planes) - 1,
> > > parms, false);
> > > -		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));
> > > +		/* Sort the group of the rest of overlay planes */
> > > +		if (hars_petruska_f54_1_random_unsafe_max(2))
> > > +			parms[i].mask = GROUP_TYPE_OVERLAY2;
> > > +	}
> > >  
> > > -		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;
> > > -			}
> > > +	*iter_max = iter_mask + 1;
> > >  
> > > +	/*
> > > +	 * Pre gen9 not all sizes are supported, find the biggest
> > > possible
> > > +	 * size that can be enabled on all sprite planes.
> > > +	 */
> > > +	while (1) {
> > > +		/* Size limit reached */
> > > +		if (is_atomic_check_plane_size_errno(ret)) {
> > >  			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 {
> > > -			prev_w = sprite_width;
> > > -			prev_h = sprite_height;
> > > +			break;
> > >  		}
> > >  
> > > -		if (!max_sprite_width) {
> > > -			sprite_width *= 2;
> > > +		/* Commit is valid and it reached max size, use this
> > > size */
> > > +		if (sprite_width == mode->hdisplay ||
> > > +		    sprite_height == mode->vdisplay)
> > > +			break;
> > >  
> > > -			if (sprite_width >= mode->hdisplay) {
> > > -				max_sprite_width = true;
> > > +		prev_w = sprite_width;
> > > +		prev_h = sprite_height;
> > >  
> > > -				sprite_width = mode->hdisplay;
> > > -			}
> > > -		} else if (!max_sprite_height) {
> > > -			sprite_height *= 2;
> > > +		sprite_width *= 2;
> > > +		if (sprite_width >= mode->hdisplay)
> > > +			sprite_width = mode->hdisplay;
> > >  
> > > -			if (sprite_height >= mode->vdisplay) {
> > > -				max_sprite_height = true;
> > > +		sprite_height *= 2;
> > > +		if (sprite_height >= mode->vdisplay)
> > > +			sprite_height = mode->vdisplay;
> > >  
> > > -				sprite_height = mode->vdisplay;
> > > -			}
> > > -		} else
> > > -			/* Max sized sprites for all! */
> > > -			break;
> > > +		set_sprite_wh(display, pipe, parms, sprite_fb,
> > > sprite_width,
> > > +			      sprite_height, alpha);
> > > +		wm_setup_plane(display, pipe, iter_mask, parms, false);
> > > +		ret = igt_display_try_commit_atomic(display,
> > > +						    DRM_MODE_ATOMIC_TES
> > > T_ONLY |
> > > +						    DRM_MODE_ATOMIC_ALL
> > > OW_MODESET,
> > > +						    NULL);
> > >  	}
> > >  
> > > -	igt_info("Running test on pipe %s with resolution %dx%d and
> > > sprite size %dx%d alpha %i\n",
> > > +	igt_info("Running test on pipe %s with resolution %dx%d, sprite
> > > size %dx%d, alpha %i and %i overlay planes\n",
> > >  		 kmstest_pipe_name(pipe), mode->hdisplay, mode-
> > > > vdisplay,
> > > 
> > > -		 sprite_width, sprite_height, alpha);
> > > +		 sprite_width, sprite_height, alpha, n_overlays);
> > >  }
> > >  
> > >  static void prepare_fencing(igt_display_t *display, enum pipe
> > > pipe)
> > > @@ -519,8 +560,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 !=
> > > GROUP_TYPE_NONE)
> > > +				igt_plane_set_position(plane, 0, 0);
> > > +		}
> > >  
> > >  		igt_display_commit2(display, COMMIT_ATOMIC);
> > >  
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

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

end of thread, other threads:[~2019-04-09  7:27 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-05 23:31 [igt-dev] [PATCH i-g-t 1/2] HAX: tests/kms_atomic_transition: Don't test more planes than memory bandwidth can support José Roberto de Souza
2019-04-05 23:31 ` [igt-dev] [PATCH i-g-t 2/2] tests/kms_atomic_transition: Remove useless code José Roberto de Souza
2019-04-06  0:23 ` [igt-dev] ✗ Fi.CI.BAT: failure for series starting with [i-g-t,1/2] HAX: tests/kms_atomic_transition: Don't test more planes than memory bandwidth can support Patchwork
2019-04-06  1:27 ` [igt-dev] ✓ Fi.CI.BAT: success for series starting with [i-g-t,1/2] HAX: tests/kms_atomic_transition: Don't test more planes than memory bandwidth can support (rev2) Patchwork
2019-04-07  2:02 ` [igt-dev] ✗ Fi.CI.IGT: failure " Patchwork
2019-04-08  8:38 ` [igt-dev] [PATCH i-g-t 1/2] HAX: tests/kms_atomic_transition: Don't test more planes than memory bandwidth can support Lisovskiy, Stanislav
2019-04-09  1:41   ` Souza, Jose
2019-04-08  9:47 ` Lisovskiy, Stanislav
2019-04-09  1:51   ` Souza, Jose
2019-04-09  7:27     ` Lisovskiy, Stanislav

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