[igt-dev] [PATCH] [PATCH i-g-t][V7]tests/i915/gem_sync.c :Added __for_each_physical_engine to utilize all available

[igt-dev] [PATCH] [PATCH i-g-t][V7]tests/i915/gem_sync.c :Added __for_each_physical_engine to utilize all available

[Arjun Melkaveri]

Replaced the legacy for_each_engine* defines with the ones
implemented in the gem_engine_topology library.

Used  gem_context_clone_with_engines
to make sure that engine index was potentially created
based on a  default context with engine map configured.

Added Legacy engine coverage for sync_ring and sync_all.

Divided tests into static for ALL_Engine test cases
and dynamic for multiple engine .

V7:
Initializing engine and engine name with
maximum supported engines value.

Cc: Dec Katarzyna <katarzyna.dec@intel.com>
Cc: Ursulin Tvrtko <tvrtko.ursulin@intel.com>
Signed-off-by: sai gowtham <sai.gowtham.ch@intel.com>
Signed-off-by: Arjun Melkaveri <arjun.melkaveri@intel.com>
---
 tests/i915/gem_sync.c | 310 ++++++++++++++++++++++++++----------------
 1 file changed, 195 insertions(+), 115 deletions(-)

diff --git a/tests/i915/gem_sync.c b/tests/i915/gem_sync.c
index 2ef55ecc..f1a87f59 100644
--- a/tests/i915/gem_sync.c
+++ b/tests/i915/gem_sync.c
@@ -24,6 +24,9 @@
 #include <time.h>
 #include <pthread.h>
 
+#include "igt_debugfs.h"
+#include "igt_dummyload.h"
+#include "igt_gt.h"
 #include "igt.h"
 #include "igt_sysfs.h"
 
@@ -37,6 +40,10 @@
 #define MIN_PRIO LOCAL_I915_CONTEXT_MIN_USER_PRIORITY
 
 #define ENGINE_MASK  (I915_EXEC_RING_MASK | LOCAL_I915_EXEC_BSD_MASK)
+#define RESET_TIMEOUT_MS (2 * MSEC_PER_SEC)
+static unsigned long reset_timeout_ms = RESET_TIMEOUT_MS;
+#define NSEC_PER_MSEC (1000 * 1000ull)
+#define EXECBUF_MAX_ENGINES (I915_EXEC_RING_MASK + 1)
 
 IGT_TEST_DESCRIPTION("Basic check of ring<->ring write synchronisation.");
 
@@ -78,24 +85,35 @@ out:
 	return ts.tv_sec + 1e-9*ts.tv_nsec;
 }
 
+static void cleanup(int i915)
+{
+	igt_drop_caches_set(i915,
+			    /* cancel everything */
+			    DROP_RESET_ACTIVE | DROP_RESET_SEQNO |
+			    /* cleanup */
+			    DROP_ACTIVE | DROP_RETIRE | DROP_IDLE | DROP_FREED);
+	igt_require_gem(i915);
+}
+
+
 static void
 sync_ring(int fd, unsigned ring, int num_children, int timeout)
 {
-	unsigned engines[16];
-	const char *names[16];
+	const struct intel_execution_engine2 *e2;
+	unsigned int engines[EXECBUF_MAX_ENGINES];
+	const char *names[EXECBUF_MAX_ENGINES];
 	int num_engines = 0;
 
 	if (ring == ALL_ENGINES) {
-		for_each_physical_engine(e, fd) {
-			names[num_engines] = e->name;
-			engines[num_engines++] = eb_ring(e);
+		__for_each_physical_engine(fd, e2) {
+			names[num_engines] = strdup(e2->name);
+			engines[num_engines++] = e2->flags;
 			if (num_engines == ARRAY_SIZE(engines))
 				break;
 		}
 
 		num_children *= num_engines;
 	} else {
-		gem_require_ring(fd, ring);
 		names[num_engines] = NULL;
 		engines[num_engines++] = ring;
 	}
@@ -139,7 +157,7 @@ sync_ring(int fd, unsigned ring, int num_children, int timeout)
 }
 
 static void
-idle_ring(int fd, unsigned ring, int timeout)
+idle_ring(int fd, unsigned int ring, int num_children, int timeout)
 {
 	const uint32_t bbe = MI_BATCH_BUFFER_END;
 	struct drm_i915_gem_exec_object2 object;
@@ -180,23 +198,23 @@ idle_ring(int fd, unsigned ring, int timeout)
 static void
 wakeup_ring(int fd, unsigned ring, int timeout, int wlen)
 {
-	unsigned engines[16];
-	const char *names[16];
+	const struct intel_execution_engine2 *e2;
+	unsigned int engines[EXECBUF_MAX_ENGINES];
+	const char *names[EXECBUF_MAX_ENGINES];
 	int num_engines = 0;
 
 	if (ring == ALL_ENGINES) {
-		for_each_physical_engine(e, fd) {
-			if (!gem_can_store_dword(fd, eb_ring(e)))
+		__for_each_physical_engine(fd, e2) {
+			if (!gem_class_can_store_dword(fd, e2->class))
 				continue;
 
-			names[num_engines] = e->name;
-			engines[num_engines++] = eb_ring(e);
+			names[num_engines] = e2->name;
+			engines[num_engines++] = e2->flags;
 			if (num_engines == ARRAY_SIZE(engines))
 				break;
 		}
 		igt_require(num_engines);
 	} else {
-		gem_require_ring(fd, ring);
 		igt_require(gem_can_store_dword(fd, ring));
 		names[num_engines] = NULL;
 		engines[num_engines++] = ring;
@@ -290,25 +308,26 @@ wakeup_ring(int fd, unsigned ring, int timeout, int wlen)
 	igt_assert_eq(intel_detect_and_clear_missed_interrupts(fd), 0);
 }
 
-static void active_ring(int fd, unsigned ring, int timeout)
+static void active_ring(int fd, unsigned int ring,
+			int num_children, int timeout)
 {
-	unsigned engines[16];
-	const char *names[16];
+	const struct intel_execution_engine2 *e2;
+	unsigned int engines[EXECBUF_MAX_ENGINES];
+	const char *names[EXECBUF_MAX_ENGINES];
 	int num_engines = 0;
 
 	if (ring == ALL_ENGINES) {
-		for_each_physical_engine(e, fd) {
-			if (!gem_can_store_dword(fd, eb_ring(e)))
+		__for_each_physical_engine(fd, e2) {
+			if (!gem_class_can_store_dword(fd, e2->class))
 				continue;
 
-			names[num_engines] = e->name;
-			engines[num_engines++] = eb_ring(e);
+			names[num_engines] = e2->name;
+			engines[num_engines++] = e2->flags;
 			if (num_engines == ARRAY_SIZE(engines))
 				break;
 		}
 		igt_require(num_engines);
 	} else {
-		gem_require_ring(fd, ring);
 		igt_require(gem_can_store_dword(fd, ring));
 		names[num_engines] = NULL;
 		engines[num_engines++] = ring;
@@ -359,23 +378,23 @@ static void active_ring(int fd, unsigned ring, int timeout)
 static void
 active_wakeup_ring(int fd, unsigned ring, int timeout, int wlen)
 {
-	unsigned engines[16];
-	const char *names[16];
+	const struct intel_execution_engine2 *e2;
+	unsigned int engines[EXECBUF_MAX_ENGINES];
+	const char *names[EXECBUF_MAX_ENGINES];
 	int num_engines = 0;
 
 	if (ring == ALL_ENGINES) {
-		for_each_physical_engine(e, fd) {
-			if (!gem_can_store_dword(fd, eb_ring(e)))
+		__for_each_physical_engine(fd, e2) {
+			if (!gem_class_can_store_dword(fd, e2->class))
 				continue;
 
-			names[num_engines] = e->name;
-			engines[num_engines++] = eb_ring(e);
+			names[num_engines] = e2->name;
+			engines[num_engines++] = e2->flags;
 			if (num_engines == ARRAY_SIZE(engines))
 				break;
 		}
 		igt_require(num_engines);
 	} else {
-		gem_require_ring(fd, ring);
 		igt_require(gem_can_store_dword(fd, ring));
 		names[num_engines] = NULL;
 		engines[num_engines++] = ring;
@@ -493,25 +512,25 @@ active_wakeup_ring(int fd, unsigned ring, int timeout, int wlen)
 static void
 store_ring(int fd, unsigned ring, int num_children, int timeout)
 {
+	const struct intel_execution_engine2 *e2;
 	const int gen = intel_gen(intel_get_drm_devid(fd));
-	unsigned engines[16];
-	const char *names[16];
+	unsigned int engines[EXECBUF_MAX_ENGINES];
+	const char *names[EXECBUF_MAX_ENGINES];
 	int num_engines = 0;
 
 	if (ring == ALL_ENGINES) {
-		for_each_physical_engine(e, fd) {
-			if (!gem_can_store_dword(fd, eb_ring(e)))
+		__for_each_physical_engine(fd, e2) {
+			if (!gem_class_can_store_dword(fd, e2->class))
 				continue;
 
-			names[num_engines] = e->name;
-			engines[num_engines++] = eb_ring(e);
+			names[num_engines] = e2->name;
+			engines[num_engines++] = e2->flags;
 			if (num_engines == ARRAY_SIZE(engines))
 				break;
 		}
 
 		num_children *= num_engines;
 	} else {
-		gem_require_ring(fd, ring);
 		igt_require(gem_can_store_dword(fd, ring));
 		names[num_engines] = NULL;
 		engines[num_engines++] = ring;
@@ -608,27 +627,27 @@ store_ring(int fd, unsigned ring, int num_children, int timeout)
 static void
 switch_ring(int fd, unsigned ring, int num_children, int timeout)
 {
+	const struct intel_execution_engine2 *e2;
 	const int gen = intel_gen(intel_get_drm_devid(fd));
-	unsigned engines[16];
-	const char *names[16];
+	unsigned int engines[EXECBUF_MAX_ENGINES];
+	const char *names[EXECBUF_MAX_ENGINES];
 	int num_engines = 0;
 
 	gem_require_contexts(fd);
 
 	if (ring == ALL_ENGINES) {
-		for_each_physical_engine(e, fd) {
-			if (!gem_can_store_dword(fd, eb_ring(e)))
+		__for_each_physical_engine(fd, e2) {
+			if (!gem_class_can_store_dword(fd, e2->class))
 				continue;
 
-			names[num_engines] = e->name;
-			engines[num_engines++] = eb_ring(e);
+			names[num_engines] = e2->name;
+			engines[num_engines++] = e2->flags;
 			if (num_engines == ARRAY_SIZE(engines))
 				break;
 		}
 
 		num_children *= num_engines;
 	} else {
-		gem_require_ring(fd, ring);
 		igt_require(gem_can_store_dword(fd, ring));
 		names[num_engines] = NULL;
 		engines[num_engines++] = ring;
@@ -931,10 +950,11 @@ __store_many(int fd, unsigned ring, int timeout, unsigned long *cycles)
 }
 
 static void
-store_many(int fd, unsigned ring, int timeout)
+store_many(int fd, unsigned int ring, int num_children, int timeout)
 {
+	const struct intel_execution_engine2 *e2;
 	unsigned long *shared;
-	const char *names[16];
+	const char *names[EXECBUF_MAX_ENGINES];
 	int n = 0;
 
 	shared = mmap(NULL, 4096, PROT_WRITE, MAP_SHARED | MAP_ANON, -1, 0);
@@ -943,21 +963,20 @@ store_many(int fd, unsigned ring, int timeout)
 	intel_detect_and_clear_missed_interrupts(fd);
 
 	if (ring == ALL_ENGINES) {
-		for_each_physical_engine(e, fd) {
-			if (!gem_can_store_dword(fd, eb_ring(e)))
+		__for_each_physical_engine(fd, e2) {
+			if (!gem_class_can_store_dword(fd, e2->class))
 				continue;
 
 			igt_fork(child, 1)
 				__store_many(fd,
-					     eb_ring(e),
+					     e2->flags,
 					     timeout,
 					     &shared[n]);
 
-			names[n++] = e->name;
+			names[n++] = e2->name;
 		}
 		igt_waitchildren();
 	} else {
-		gem_require_ring(fd, ring);
 		igt_require(gem_can_store_dword(fd, ring));
 		__store_many(fd, ring, timeout, &shared[n]);
 		names[n++] = NULL;
@@ -1025,15 +1044,16 @@ sync_all(int fd, int num_children, int timeout)
 static void
 store_all(int fd, int num_children, int timeout)
 {
+	const struct intel_execution_engine2 *e;
 	const int gen = intel_gen(intel_get_drm_devid(fd));
-	unsigned engines[16];
+	unsigned int engines[EXECBUF_MAX_ENGINES];
 	int num_engines = 0;
 
-	for_each_physical_engine(e, fd) {
-		if (!gem_can_store_dword(fd, eb_ring(e)))
+	__for_each_physical_engine(fd, e) {
+		if (!gem_class_can_store_dword(fd, e->class))
 			continue;
 
-		engines[num_engines++] = eb_ring(e);
+		engines[num_engines++] = e->flags;
 		if (num_engines == ARRAY_SIZE(engines))
 			break;
 	}
@@ -1132,22 +1152,22 @@ store_all(int fd, int num_children, int timeout)
 static void
 preempt(int fd, unsigned ring, int num_children, int timeout)
 {
-	unsigned engines[16];
-	const char *names[16];
+	const struct intel_execution_engine2 *e2;
+	unsigned int engines[EXECBUF_MAX_ENGINES];
+	const char *names[EXECBUF_MAX_ENGINES];
 	int num_engines = 0;
 	uint32_t ctx[2];
 
 	if (ring == ALL_ENGINES) {
-		for_each_physical_engine(e, fd) {
-			names[num_engines] = e->name;
-			engines[num_engines++] = eb_ring(e);
+		__for_each_physical_engine(fd, e2) {
+			names[num_engines] = e2->name;
+			engines[num_engines++] = e2->flags;
 			if (num_engines == ARRAY_SIZE(engines))
 				break;
 		}
 
 		num_children *= num_engines;
 	} else {
-		gem_require_ring(fd, ring);
 		names[num_engines] = NULL;
 		engines[num_engines++] = ring;
 	}
@@ -1207,76 +1227,99 @@ preempt(int fd, unsigned ring, int num_children, int timeout)
 	gem_context_destroy(fd, ctx[0]);
 }
 
+static void do_test(void (*test)(int i915, unsigned int engine,
+		    int num_children, int test_timeout),
+		    int i915, unsigned int engine, int num_children,
+		    int test_timeout, const char *name)
+{
+#define ATTR "preempt_timeout_ms"
+	int timeout = -1;
+
+	cleanup(i915);
+
+	gem_engine_property_scanf(i915, name, ATTR, "%d", &timeout);
+	if (timeout != -1) {
+		igt_require(gem_engine_property_printf(i915, name,
+						       ATTR, "%d", 50) > 0);
+		reset_timeout_ms = 200;
+	}
+
+	test(i915, engine, num_children, test_timeout);
+	gem_quiescent_gpu(i915);
+}
+
 igt_main
 {
-	const struct intel_execution_engine *e;
 	const int ncpus = sysconf(_SC_NPROCESSORS_ONLN);
 	int fd = -1;
 
+	struct {
+		const char *name;
+		void (*func)(int fd, unsigned int engine,
+			     int num_children, int timeout);
+		int num_children;
+		int timeout;
+	} *test, allengine[] = {
+		{ "basic-each", sync_ring, 1, 2},
+		{ "basic-store-each", store_ring, 1, 2},
+		{ "basic-many-each", store_many, 0, 2},
+		{ "switch-each", switch_ring, 1, 20},
+		{ "forked-switch-each", switch_ring, ncpus, 20},
+		{ "forked-each", sync_ring, ncpus, 20},
+		{ "forked-store-each", store_ring, ncpus, 20},
+		{ "active-each", active_ring, 0, 20},
+		{ "wakeup-each", wakeup_ring, 20, 1},
+		{ "active-wakeup-each", active_wakeup_ring, 20, 1},
+		{ "double-wakeup-each", wakeup_ring, 20, 2},
+		{ NULL, NULL },
+	}, tests[] = {
+		{ "default", sync_ring, 1, 20},
+		{ "idle", idle_ring, 0, 20},
+		{ "active", active_ring, 0, 20},
+		{ "wakeup", wakeup_ring, 20, 1},
+		{ "active-wakeup", active_wakeup_ring, 20, 1},
+		{ "double-wakeup", wakeup_ring, 20, 2},
+		{ "store", store_ring, 1, 20},
+		{ "switch", switch_ring, 1, 20},
+		{ "forked-switch", switch_ring, ncpus, 20},
+		{ "many", store_many, 0, 20},
+		{ "forked", sync_ring, ncpus, 20},
+		{ "forked-store", store_ring, ncpus, 20},
+		{ NULL, NULL },
+	};
+
+
 	igt_fixture {
 		fd = drm_open_driver(DRIVER_INTEL);
 		igt_require_gem(fd);
 		gem_submission_print_method(fd);
 		gem_scheduler_print_capability(fd);
-
 		igt_fork_hang_detector(fd);
 	}
 
-	for (e = intel_execution_engines; e->name; e++) {
-		igt_subtest_f("%s", e->name)
-			sync_ring(fd, eb_ring(e), 1, 20);
-		igt_subtest_f("idle-%s", e->name)
-			idle_ring(fd, eb_ring(e), 20);
-		igt_subtest_f("active-%s", e->name)
-			active_ring(fd, eb_ring(e), 20);
-		igt_subtest_f("wakeup-%s", e->name)
-			wakeup_ring(fd, eb_ring(e), 20, 1);
-		igt_subtest_f("active-wakeup-%s", e->name)
-			active_wakeup_ring(fd, eb_ring(e), 20, 1);
-		igt_subtest_f("double-wakeup-%s", e->name)
-			wakeup_ring(fd, eb_ring(e), 20, 2);
-		igt_subtest_f("store-%s", e->name)
-			store_ring(fd, eb_ring(e), 1, 20);
-		igt_subtest_f("switch-%s", e->name)
-			switch_ring(fd, eb_ring(e), 1, 20);
-		igt_subtest_f("forked-switch-%s", e->name)
-			switch_ring(fd, eb_ring(e), ncpus, 20);
-		igt_subtest_f("many-%s", e->name)
-			store_many(fd, eb_ring(e), 20);
-		igt_subtest_f("forked-%s", e->name)
-			sync_ring(fd, eb_ring(e), ncpus, 20);
-		igt_subtest_f("forked-store-%s", e->name)
-			store_ring(fd, eb_ring(e), ncpus, 20);
-	}
+	/* legacy of selecting engines. */
 
-	igt_subtest("basic-each")
-		sync_ring(fd, ALL_ENGINES, 1, 2);
-	igt_subtest("basic-store-each")
-		store_ring(fd, ALL_ENGINES, 1, 2);
-	igt_subtest("basic-many-each")
-		store_many(fd, ALL_ENGINES, 2);
-	igt_subtest("switch-each")
-		switch_ring(fd, ALL_ENGINES, 1, 20);
-	igt_subtest("forked-switch-each")
-		switch_ring(fd, ALL_ENGINES, ncpus, 20);
-	igt_subtest("forked-each")
-		sync_ring(fd, ALL_ENGINES, ncpus, 20);
-	igt_subtest("forked-store-each")
-		store_ring(fd, ALL_ENGINES, ncpus, 20);
-	igt_subtest("active-each")
-		active_ring(fd, ALL_ENGINES, 20);
-	igt_subtest("wakeup-each")
-		wakeup_ring(fd, ALL_ENGINES, 20, 1);
-	igt_subtest("active-wakeup-each")
-		active_wakeup_ring(fd, ALL_ENGINES, 20, 1);
-	igt_subtest("double-wakeup-each")
-		wakeup_ring(fd, ALL_ENGINES, 20, 2);
+	igt_subtest_group {
+		for (test = tests; test->name; test++) {
+			igt_subtest_with_dynamic_f("legacy-engines-%s",
+						   test->name) {
+				for_each_physical_engine(e, fd) {
+					igt_dynamic_f("%s", e->name) {
+						do_test(test->func,
+							fd, eb_ring(e),
+							test->num_children,
+							test->timeout,
+							e->full_name);
+					}
+				}
+			}
+		}
+	}
 
 	igt_subtest("basic-all")
 		sync_all(fd, 1, 2);
 	igt_subtest("basic-store-all")
 		store_all(fd, 1, 2);
-
 	igt_subtest("all")
 		sync_all(fd, 1, 20);
 	igt_subtest("store-all")
@@ -1286,7 +1329,42 @@ igt_main
 	igt_subtest("forked-store-all")
 		store_all(fd, ncpus, 20);
 
+	/* All Engines */
 	igt_subtest_group {
+		for (test = allengine; test->name; test++) {
+			igt_subtest_f("%s", test->name) {
+				do_test(test->func,
+					fd, ALL_ENGINES,
+					test->num_children,
+					test->timeout,
+					test->name);
+			}
+		}
+	}
+
+	/* New way of selecting engines. */
+	igt_subtest_group {
+		const struct intel_execution_engine2 *e;
+
+		for (test = tests; test->name; test++) {
+			igt_subtest_with_dynamic_f("%s", test->name) {
+				__for_each_physical_engine(fd, e) {
+					igt_dynamic_f("%s", e->name) {
+						do_test(test->func,
+							fd, e->flags,
+							test->num_children,
+							test->timeout,
+							e->name);
+					}
+				}
+			}
+		}
+	}
+
+
+	igt_subtest_group {
+		const struct intel_execution_engine2 *e;
+
 		igt_fixture {
 			gem_require_contexts(fd);
 			igt_require(gem_scheduler_has_ctx_priority(fd));
@@ -1295,10 +1373,12 @@ igt_main
 
 		igt_subtest("preempt-all")
 			preempt(fd, ALL_ENGINES, 1, 20);
-
-		for (e = intel_execution_engines; e->name; e++) {
-			igt_subtest_f("preempt-%s", e->name)
-				preempt(fd, eb_ring(e), ncpus, 20);
+		igt_subtest_with_dynamic("preempt") {
+			__for_each_physical_engine(fd, e) {
+				/* Requires master for STORE_DWORD on gen4/5 */
+				igt_dynamic_f("%s", e->name)
+					preempt(fd, e->flags, ncpus, 20);
+			}
 		}
 	}
 
-- 
2.25.1

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

[igt-dev] ✗ GitLab.Pipeline: failure for tests/i915/gem_sync.c :Added __for_each_physical_engine to utilize all available (rev8)

[Patchwork]

== Series Details ==

Series: tests/i915/gem_sync.c :Added __for_each_physical_engine to utilize all available (rev8)
URL   : https://patchwork.freedesktop.org/series/75536/
State : failure

== Summary ==

ERROR! This series introduces new undocumented tests:

gem_sync@active
gem_sync@active-wakeup
gem_sync@double-wakeup
gem_sync@forked
gem_sync@forked-store
gem_sync@forked-switch
gem_sync@idle
gem_sync@legacy-engines-active
gem_sync@legacy-engines-active-wakeup
gem_sync@legacy-engines-default
gem_sync@legacy-engines-double-wakeup
gem_sync@legacy-engines-forked
gem_sync@legacy-engines-forked-store
gem_sync@legacy-engines-forked-switch
gem_sync@legacy-engines-idle
gem_sync@legacy-engines-many
gem_sync@legacy-engines-store
gem_sync@legacy-engines-switch
gem_sync@legacy-engines-wakeup
gem_sync@many
gem_sync@preempt
gem_sync@store
gem_sync@switch
gem_sync@wakeup

Can you document them as per the requirement in the [CONTRIBUTING.md]?

[Documentation] has more details on how to do this.

Here are few examples:
https://gitlab.freedesktop.org/drm/igt-gpu-tools/commit/0316695d03aa46108296b27f3982ec93200c7a6e
https://gitlab.freedesktop.org/drm/igt-gpu-tools/commit/443cc658e1e6b492ee17bf4f4d891029eb7a205d

Thanks in advance!

[CONTRIBUTING.md]: https://gitlab.freedesktop.org/drm/igt-gpu-tools/blob/master/CONTRIBUTING.md#L19
[Documentation]: https://drm.pages.freedesktop.org/igt-gpu-tools/igt-gpu-tools-Core.html#igt-describe

Other than that, pipeline status: SUCCESS.

see https://gitlab.freedesktop.org/gfx-ci/igt-ci-tags/pipelines/132128 for the overview.

== Logs ==

For more details see: https://gitlab.freedesktop.org/gfx-ci/igt-ci-tags/pipelines/132128
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] ✗ Fi.CI.BAT: failure for tests/i915/gem_sync.c :Added __for_each_physical_engine to utilize all available (rev8)

[Patchwork]

== Series Details ==

Series: tests/i915/gem_sync.c :Added __for_each_physical_engine to utilize all available (rev8)
URL   : https://patchwork.freedesktop.org/series/75536/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_8298 -> IGTPW_4461
====================================================

Summary
-------

  **FAILURE**

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

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

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

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

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live@gt_timelines:
    - fi-bwr-2160:        [PASS][1] -> [INCOMPLETE][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8298/fi-bwr-2160/igt@i915_selftest@live@gt_timelines.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4461/fi-bwr-2160/igt@i915_selftest@live@gt_timelines.html

  


Participating hosts (48 -> 44)
------------------------------

  Missing    (4): fi-byt-clapper fi-byt-squawks fi-bsw-cyan fi-hsw-4200u 


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

  * CI: CI-20190529 -> None
  * IGT: IGT_5589 -> IGTPW_4461

  CI-20190529: 20190529
  CI_DRM_8298: 17f82f0c2857d0b442adbdb62eb44b61d0f5b775 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_4461: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4461/index.html
  IGT_5589: 31962324ac86f029e2841e56e97c42cf9d572956 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools



== Testlist changes ==

+++ 24 lines
--- 90 lines

== Logs ==

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

Re: [igt-dev] [PATCH] [PATCH i-g-t][V7]tests/i915/gem_sync.c :Added __for_each_physical_engine to utilize all available

[Tvrtko Ursulin]

On 14/04/2020 18:51, Arjun Melkaveri wrote:
> Replaced the legacy for_each_engine* defines with the ones
> implemented in the gem_engine_topology library.
> 
> Used  gem_context_clone_with_engines
> to make sure that engine index was potentially created
> based on a  default context with engine map configured.
> 
> Added Legacy engine coverage for sync_ring and sync_all.
> 
> Divided tests into static for ALL_Engine test cases
> and dynamic for multiple engine .
> 
> V7:
> Initializing engine and engine name with
> maximum supported engines value.

You lost the rest of the change log, not terribly important though.

> 
> Cc: Dec Katarzyna <katarzyna.dec@intel.com>
> Cc: Ursulin Tvrtko <tvrtko.ursulin@intel.com>
> Signed-off-by: sai gowtham <sai.gowtham.ch@intel.com>
> Signed-off-by: Arjun Melkaveri <arjun.melkaveri@intel.com>
> ---
>   tests/i915/gem_sync.c | 310 ++++++++++++++++++++++++++----------------
>   1 file changed, 195 insertions(+), 115 deletions(-)
> 
> diff --git a/tests/i915/gem_sync.c b/tests/i915/gem_sync.c
> index 2ef55ecc..f1a87f59 100644
> --- a/tests/i915/gem_sync.c
> +++ b/tests/i915/gem_sync.c
> @@ -24,6 +24,9 @@
>   #include <time.h>
>   #include <pthread.h>
>   
> +#include "igt_debugfs.h"
> +#include "igt_dummyload.h"
> +#include "igt_gt.h"
>   #include "igt.h"
>   #include "igt_sysfs.h"
>   
> @@ -37,6 +40,10 @@
>   #define MIN_PRIO LOCAL_I915_CONTEXT_MIN_USER_PRIORITY
>   
>   #define ENGINE_MASK  (I915_EXEC_RING_MASK | LOCAL_I915_EXEC_BSD_MASK)
> +#define RESET_TIMEOUT_MS (2 * MSEC_PER_SEC)
> +static unsigned long reset_timeout_ms = RESET_TIMEOUT_MS;

I've asked in one of the previous rounds what's this for? It's only set 
in do_test and never acted upon.

> +#define NSEC_PER_MSEC (1000 * 1000ull)

Unused.

> +#define EXECBUF_MAX_ENGINES (I915_EXEC_RING_MASK + 1)
>   
>   IGT_TEST_DESCRIPTION("Basic check of ring<->ring write synchronisation.");
>   
> @@ -78,24 +85,35 @@ out:
>   	return ts.tv_sec + 1e-9*ts.tv_nsec;
>   }
>   
> +static void cleanup(int i915)
> +{
> +	igt_drop_caches_set(i915,
> +			    /* cancel everything */
> +			    DROP_RESET_ACTIVE | DROP_RESET_SEQNO |
> +			    /* cleanup */
> +			    DROP_ACTIVE | DROP_RETIRE | DROP_IDLE | DROP_FREED);
> +	igt_require_gem(i915);
> +}
> +
> +
>   static void
>   sync_ring(int fd, unsigned ring, int num_children, int timeout)
>   {
> -	unsigned engines[16];
> -	const char *names[16];
> +	const struct intel_execution_engine2 *e2;
> +	unsigned int engines[EXECBUF_MAX_ENGINES];
> +	const char *names[EXECBUF_MAX_ENGINES];
>   	int num_engines = 0;
>   
>   	if (ring == ALL_ENGINES) {
> -		for_each_physical_engine(e, fd) {
> -			names[num_engines] = e->name;
> -			engines[num_engines++] = eb_ring(e);
> +		__for_each_physical_engine(fd, e2) {
> +			names[num_engines] = strdup(e2->name);
> +			engines[num_engines++] = e2->flags;
>   			if (num_engines == ARRAY_SIZE(engines))
>   				break;
>   		}
>   
>   		num_children *= num_engines;
>   	} else {
> -		gem_require_ring(fd, ring);
>   		names[num_engines] = NULL;
>   		engines[num_engines++] = ring;
>   	}
> @@ -139,7 +157,7 @@ sync_ring(int fd, unsigned ring, int num_children, int timeout)
>   }
>   
>   static void
> -idle_ring(int fd, unsigned ring, int timeout)
> +idle_ring(int fd, unsigned int ring, int num_children, int timeout)
>   {
>   	const uint32_t bbe = MI_BATCH_BUFFER_END;
>   	struct drm_i915_gem_exec_object2 object;
> @@ -180,23 +198,23 @@ idle_ring(int fd, unsigned ring, int timeout)
>   static void
>   wakeup_ring(int fd, unsigned ring, int timeout, int wlen)
>   {
> -	unsigned engines[16];
> -	const char *names[16];
> +	const struct intel_execution_engine2 *e2;
> +	unsigned int engines[EXECBUF_MAX_ENGINES];
> +	const char *names[EXECBUF_MAX_ENGINES];
>   	int num_engines = 0;
>   
>   	if (ring == ALL_ENGINES) {
> -		for_each_physical_engine(e, fd) {
> -			if (!gem_can_store_dword(fd, eb_ring(e)))
> +		__for_each_physical_engine(fd, e2) {
> +			if (!gem_class_can_store_dword(fd, e2->class))
>   				continue;
>   
> -			names[num_engines] = e->name;
> -			engines[num_engines++] = eb_ring(e);
> +			names[num_engines] = e2->name;

Looks like you need strdup here as well.

> +			engines[num_engines++] = e2->flags;
>   			if (num_engines == ARRAY_SIZE(engines))
>   				break;
>   		}
>   		igt_require(num_engines);
>   	} else {
> -		gem_require_ring(fd, ring);
>   		igt_require(gem_can_store_dword(fd, ring));
>   		names[num_engines] = NULL;
>   		engines[num_engines++] = ring;
> @@ -290,25 +308,26 @@ wakeup_ring(int fd, unsigned ring, int timeout, int wlen)
>   	igt_assert_eq(intel_detect_and_clear_missed_interrupts(fd), 0);
>   }
>   
> -static void active_ring(int fd, unsigned ring, int timeout)
> +static void active_ring(int fd, unsigned int ring,
> +			int num_children, int timeout)
>   {
> -	unsigned engines[16];
> -	const char *names[16];
> +	const struct intel_execution_engine2 *e2;
> +	unsigned int engines[EXECBUF_MAX_ENGINES];
> +	const char *names[EXECBUF_MAX_ENGINES];
>   	int num_engines = 0;
>   
>   	if (ring == ALL_ENGINES) {
> -		for_each_physical_engine(e, fd) {
> -			if (!gem_can_store_dword(fd, eb_ring(e)))
> +		__for_each_physical_engine(fd, e2) {
> +			if (!gem_class_can_store_dword(fd, e2->class))
>   				continue;
>   
> -			names[num_engines] = e->name;
> -			engines[num_engines++] = eb_ring(e);
> +			names[num_engines] = e2->name;

And here.

> +			engines[num_engines++] = e2->flags;
>   			if (num_engines == ARRAY_SIZE(engines))
>   				break;
>   		}
>   		igt_require(num_engines);
>   	} else {
> -		gem_require_ring(fd, ring);
>   		igt_require(gem_can_store_dword(fd, ring));
>   		names[num_engines] = NULL;
>   		engines[num_engines++] = ring;
> @@ -359,23 +378,23 @@ static void active_ring(int fd, unsigned ring, int timeout)
>   static void
>   active_wakeup_ring(int fd, unsigned ring, int timeout, int wlen)
>   {
> -	unsigned engines[16];
> -	const char *names[16];
> +	const struct intel_execution_engine2 *e2;
> +	unsigned int engines[EXECBUF_MAX_ENGINES];
> +	const char *names[EXECBUF_MAX_ENGINES];
>   	int num_engines = 0;
>   
>   	if (ring == ALL_ENGINES) {
> -		for_each_physical_engine(e, fd) {
> -			if (!gem_can_store_dword(fd, eb_ring(e)))
> +		__for_each_physical_engine(fd, e2) {
> +			if (!gem_class_can_store_dword(fd, e2->class))
>   				continue;
>   
> -			names[num_engines] = e->name;
> -			engines[num_engines++] = eb_ring(e);
> +			names[num_engines] = e2->name;

And here.

> +			engines[num_engines++] = e2->flags;
>   			if (num_engines == ARRAY_SIZE(engines))
>   				break;
>   		}
>   		igt_require(num_engines);
>   	} else {
> -		gem_require_ring(fd, ring);
>   		igt_require(gem_can_store_dword(fd, ring));
>   		names[num_engines] = NULL;
>   		engines[num_engines++] = ring;
> @@ -493,25 +512,25 @@ active_wakeup_ring(int fd, unsigned ring, int timeout, int wlen)
>   static void
>   store_ring(int fd, unsigned ring, int num_children, int timeout)
>   {
> +	const struct intel_execution_engine2 *e2;
>   	const int gen = intel_gen(intel_get_drm_devid(fd));
> -	unsigned engines[16];
> -	const char *names[16];
> +	unsigned int engines[EXECBUF_MAX_ENGINES];
> +	const char *names[EXECBUF_MAX_ENGINES];
>   	int num_engines = 0;
>   
>   	if (ring == ALL_ENGINES) {
> -		for_each_physical_engine(e, fd) {
> -			if (!gem_can_store_dword(fd, eb_ring(e)))
> +		__for_each_physical_engine(fd, e2) {
> +			if (!gem_class_can_store_dword(fd, e2->class))
>   				continue;
>   
> -			names[num_engines] = e->name;
> -			engines[num_engines++] = eb_ring(e);
> +			names[num_engines] = e2->name;

And here.

> +			engines[num_engines++] = e2->flags;
>   			if (num_engines == ARRAY_SIZE(engines))
>   				break;
>   		}
>   
>   		num_children *= num_engines;
>   	} else {
> -		gem_require_ring(fd, ring);
>   		igt_require(gem_can_store_dword(fd, ring));
>   		names[num_engines] = NULL;
>   		engines[num_engines++] = ring;
> @@ -608,27 +627,27 @@ store_ring(int fd, unsigned ring, int num_children, int timeout)
>   static void
>   switch_ring(int fd, unsigned ring, int num_children, int timeout)
>   {
> +	const struct intel_execution_engine2 *e2;
>   	const int gen = intel_gen(intel_get_drm_devid(fd));
> -	unsigned engines[16];
> -	const char *names[16];
> +	unsigned int engines[EXECBUF_MAX_ENGINES];
> +	const char *names[EXECBUF_MAX_ENGINES];
>   	int num_engines = 0;
>   
>   	gem_require_contexts(fd);
>   
>   	if (ring == ALL_ENGINES) {
> -		for_each_physical_engine(e, fd) {
> -			if (!gem_can_store_dword(fd, eb_ring(e)))
> +		__for_each_physical_engine(fd, e2) {
> +			if (!gem_class_can_store_dword(fd, e2->class))
>   				continue;
>   
> -			names[num_engines] = e->name;
> -			engines[num_engines++] = eb_ring(e);
> +			names[num_engines] = e2->name;

Ditto.

> +			engines[num_engines++] = e2->flags;
>   			if (num_engines == ARRAY_SIZE(engines))
>   				break;
>   		}
>   
>   		num_children *= num_engines;
>   	} else {
> -		gem_require_ring(fd, ring);
>   		igt_require(gem_can_store_dword(fd, ring));
>   		names[num_engines] = NULL;
>   		engines[num_engines++] = ring;
> @@ -931,10 +950,11 @@ __store_many(int fd, unsigned ring, int timeout, unsigned long *cycles)
>   }
>   
>   static void
> -store_many(int fd, unsigned ring, int timeout)
> +store_many(int fd, unsigned int ring, int num_children, int timeout)
>   {
> +	const struct intel_execution_engine2 *e2;
>   	unsigned long *shared;
> -	const char *names[16];
> +	const char *names[EXECBUF_MAX_ENGINES];
>   	int n = 0;
>   
>   	shared = mmap(NULL, 4096, PROT_WRITE, MAP_SHARED | MAP_ANON, -1, 0);
> @@ -943,21 +963,20 @@ store_many(int fd, unsigned ring, int timeout)
>   	intel_detect_and_clear_missed_interrupts(fd);
>   
>   	if (ring == ALL_ENGINES) {
> -		for_each_physical_engine(e, fd) {
> -			if (!gem_can_store_dword(fd, eb_ring(e)))
> +		__for_each_physical_engine(fd, e2) {
> +			if (!gem_class_can_store_dword(fd, e2->class))
>   				continue;
>   
>   			igt_fork(child, 1)
>   				__store_many(fd,
> -					     eb_ring(e),
> +					     e2->flags,
>   					     timeout,
>   					     &shared[n]);
>   
> -			names[n++] = e->name;
> +			names[n++] = e2->name;

Ditto.

>   		}
>   		igt_waitchildren();
>   	} else {
> -		gem_require_ring(fd, ring);
>   		igt_require(gem_can_store_dword(fd, ring));
>   		__store_many(fd, ring, timeout, &shared[n]);
>   		names[n++] = NULL;
> @@ -1025,15 +1044,16 @@ sync_all(int fd, int num_children, int timeout)
>   static void
>   store_all(int fd, int num_children, int timeout)
>   {
> +	const struct intel_execution_engine2 *e;
>   	const int gen = intel_gen(intel_get_drm_devid(fd));
> -	unsigned engines[16];
> +	unsigned int engines[EXECBUF_MAX_ENGINES];
>   	int num_engines = 0;
>   
> -	for_each_physical_engine(e, fd) {
> -		if (!gem_can_store_dword(fd, eb_ring(e)))
> +	__for_each_physical_engine(fd, e) {
> +		if (!gem_class_can_store_dword(fd, e->class))
>   			continue;
>   
> -		engines[num_engines++] = eb_ring(e);
> +		engines[num_engines++] = e->flags;
>   		if (num_engines == ARRAY_SIZE(engines))
>   			break;
>   	}
> @@ -1132,22 +1152,22 @@ store_all(int fd, int num_children, int timeout)
>   static void
>   preempt(int fd, unsigned ring, int num_children, int timeout)
>   {
> -	unsigned engines[16];
> -	const char *names[16];
> +	const struct intel_execution_engine2 *e2;
> +	unsigned int engines[EXECBUF_MAX_ENGINES];
> +	const char *names[EXECBUF_MAX_ENGINES];
>   	int num_engines = 0;
>   	uint32_t ctx[2];
>   
>   	if (ring == ALL_ENGINES) {
> -		for_each_physical_engine(e, fd) {
> -			names[num_engines] = e->name;
> -			engines[num_engines++] = eb_ring(e);
> +		__for_each_physical_engine(fd, e2) {
> +			names[num_engines] = e2->name;

Last one.

> +			engines[num_engines++] = e2->flags;
>   			if (num_engines == ARRAY_SIZE(engines))
>   				break;
>   		}
>   
>   		num_children *= num_engines;
>   	} else {
> -		gem_require_ring(fd, ring);
>   		names[num_engines] = NULL;
>   		engines[num_engines++] = ring;
>   	}
> @@ -1207,76 +1227,99 @@ preempt(int fd, unsigned ring, int num_children, int timeout)
>   	gem_context_destroy(fd, ctx[0]);
>   }
>   
> +static void do_test(void (*test)(int i915, unsigned int engine,
> +		    int num_children, int test_timeout),
> +		    int i915, unsigned int engine, int num_children,
> +		    int test_timeout, const char *name)
> +{
> +#define ATTR "preempt_timeout_ms"
> +	int timeout = -1;
> +
> +	cleanup(i915);
> +
> +	gem_engine_property_scanf(i915, name, ATTR, "%d", &timeout);
> +	if (timeout != -1) {
> +		igt_require(gem_engine_property_printf(i915, name,
> +						       ATTR, "%d", 50) > 0);
> +		reset_timeout_ms = 200;
> +	}
> +
> +	test(i915, engine, num_children, test_timeout);
> +	gem_quiescent_gpu(i915);

For stuff done in this wrapper - I don't really like stuffing in changes 
unrelated to patch title. If it is converting engine iteration it should 
stick to that. If you want to make some other improvements it should be 
a separate patch.

> +}
> +
>   igt_main
>   {
> -	const struct intel_execution_engine *e;
>   	const int ncpus = sysconf(_SC_NPROCESSORS_ONLN);
>   	int fd = -1;
>   
> +	struct {
> +		const char *name;
> +		void (*func)(int fd, unsigned int engine,
> +			     int num_children, int timeout);
> +		int num_children;
> +		int timeout;
> +	} *test, allengine[] = {
> +		{ "basic-each", sync_ring, 1, 2},
> +		{ "basic-store-each", store_ring, 1, 2},
> +		{ "basic-many-each", store_many, 0, 2},
> +		{ "switch-each", switch_ring, 1, 20},
> +		{ "forked-switch-each", switch_ring, ncpus, 20},
> +		{ "forked-each", sync_ring, ncpus, 20},
> +		{ "forked-store-each", store_ring, ncpus, 20},
> +		{ "active-each", active_ring, 0, 20},
> +		{ "wakeup-each", wakeup_ring, 20, 1},
> +		{ "active-wakeup-each", active_wakeup_ring, 20, 1},
> +		{ "double-wakeup-each", wakeup_ring, 20, 2},
> +		{ NULL, NULL },
> +	}, tests[] = {
> +		{ "default", sync_ring, 1, 20},
> +		{ "idle", idle_ring, 0, 20},
> +		{ "active", active_ring, 0, 20},
> +		{ "wakeup", wakeup_ring, 20, 1},
> +		{ "active-wakeup", active_wakeup_ring, 20, 1},
> +		{ "double-wakeup", wakeup_ring, 20, 2},
> +		{ "store", store_ring, 1, 20},
> +		{ "switch", switch_ring, 1, 20},
> +		{ "forked-switch", switch_ring, ncpus, 20},
> +		{ "many", store_many, 0, 20},
> +		{ "forked", sync_ring, ncpus, 20},
> +		{ "forked-store", store_ring, ncpus, 20},
> +		{ NULL, NULL },
> +	};
> +
> +
>   	igt_fixture {
>   		fd = drm_open_driver(DRIVER_INTEL);
>   		igt_require_gem(fd);
>   		gem_submission_print_method(fd);
>   		gem_scheduler_print_capability(fd);
> -

Best to avoid noise.

>   		igt_fork_hang_detector(fd);
>   	}
>   
> -	for (e = intel_execution_engines; e->name; e++) {
> -		igt_subtest_f("%s", e->name)
> -			sync_ring(fd, eb_ring(e), 1, 20);
> -		igt_subtest_f("idle-%s", e->name)
> -			idle_ring(fd, eb_ring(e), 20);
> -		igt_subtest_f("active-%s", e->name)
> -			active_ring(fd, eb_ring(e), 20);
> -		igt_subtest_f("wakeup-%s", e->name)
> -			wakeup_ring(fd, eb_ring(e), 20, 1);
> -		igt_subtest_f("active-wakeup-%s", e->name)
> -			active_wakeup_ring(fd, eb_ring(e), 20, 1);
> -		igt_subtest_f("double-wakeup-%s", e->name)
> -			wakeup_ring(fd, eb_ring(e), 20, 2);
> -		igt_subtest_f("store-%s", e->name)
> -			store_ring(fd, eb_ring(e), 1, 20);
> -		igt_subtest_f("switch-%s", e->name)
> -			switch_ring(fd, eb_ring(e), 1, 20);
> -		igt_subtest_f("forked-switch-%s", e->name)
> -			switch_ring(fd, eb_ring(e), ncpus, 20);
> -		igt_subtest_f("many-%s", e->name)
> -			store_many(fd, eb_ring(e), 20);
> -		igt_subtest_f("forked-%s", e->name)
> -			sync_ring(fd, eb_ring(e), ncpus, 20);
> -		igt_subtest_f("forked-store-%s", e->name)
> -			store_ring(fd, eb_ring(e), ncpus, 20);
> -	}
> +	/* legacy of selecting engines. */
>   
> -	igt_subtest("basic-each")
> -		sync_ring(fd, ALL_ENGINES, 1, 2);
> -	igt_subtest("basic-store-each")
> -		store_ring(fd, ALL_ENGINES, 1, 2);
> -	igt_subtest("basic-many-each")
> -		store_many(fd, ALL_ENGINES, 2);
> -	igt_subtest("switch-each")
> -		switch_ring(fd, ALL_ENGINES, 1, 20);
> -	igt_subtest("forked-switch-each")
> -		switch_ring(fd, ALL_ENGINES, ncpus, 20);
> -	igt_subtest("forked-each")
> -		sync_ring(fd, ALL_ENGINES, ncpus, 20);
> -	igt_subtest("forked-store-each")
> -		store_ring(fd, ALL_ENGINES, ncpus, 20);
> -	igt_subtest("active-each")
> -		active_ring(fd, ALL_ENGINES, 20);
> -	igt_subtest("wakeup-each")
> -		wakeup_ring(fd, ALL_ENGINES, 20, 1);
> -	igt_subtest("active-wakeup-each")
> -		active_wakeup_ring(fd, ALL_ENGINES, 20, 1);
> -	igt_subtest("double-wakeup-each")
> -		wakeup_ring(fd, ALL_ENGINES, 20, 2);
> +	igt_subtest_group {
> +		for (test = tests; test->name; test++) {
> +			igt_subtest_with_dynamic_f("legacy-engines-%s",

Have we not agreed to not add "engines" to test name?

> +						   test->name) {
> +				for_each_physical_engine(e, fd) {
> +					igt_dynamic_f("%s", e->name) {
> +						do_test(test->func,
> +							fd, eb_ring(e),
> +							test->num_children,
> +							test->timeout,
> +							e->full_name);
> +					}
> +				}
> +			}
> +		}
> +	}
>   
>   	igt_subtest("basic-all")
>   		sync_all(fd, 1, 2);
>   	igt_subtest("basic-store-all")
>   		store_all(fd, 1, 2);
> -

Noise as well.

>   	igt_subtest("all")
>   		sync_all(fd, 1, 20);
>   	igt_subtest("store-all")
> @@ -1286,7 +1329,42 @@ igt_main
>   	igt_subtest("forked-store-all")
>   		store_all(fd, ncpus, 20);
>   
> +	/* All Engines */
>   	igt_subtest_group {
> +		for (test = allengine; test->name; test++) {
> +			igt_subtest_f("%s", test->name) {
> +				do_test(test->func,
> +					fd, ALL_ENGINES,
> +					test->num_children,
> +					test->timeout,
> +					test->name);
> +			}
> +		}
> +	}
> +
> +	/* New way of selecting engines. */
> +	igt_subtest_group {
> +		const struct intel_execution_engine2 *e;
> +
> +		for (test = tests; test->name; test++) {
> +			igt_subtest_with_dynamic_f("%s", test->name) {
> +				__for_each_physical_engine(fd, e) {
> +					igt_dynamic_f("%s", e->name) {
> +						do_test(test->func,
> +							fd, e->flags,
> +							test->num_children,
> +							test->timeout,
> +							e->name);
> +					}
> +				}
> +			}
> +		}
> +	}
> +
> +
> +	igt_subtest_group {
> +		const struct intel_execution_engine2 *e;
> +
>   		igt_fixture {

Regards,

Tvrtko

>   			gem_require_contexts(fd);
>   			igt_require(gem_scheduler_has_ctx_priority(fd));
> @@ -1295,10 +1373,12 @@ igt_main
>   
>   		igt_subtest("preempt-all")
>   			preempt(fd, ALL_ENGINES, 1, 20);
> -
> -		for (e = intel_execution_engines; e->name; e++) {
> -			igt_subtest_f("preempt-%s", e->name)
> -				preempt(fd, eb_ring(e), ncpus, 20);
> +		igt_subtest_with_dynamic("preempt") {
> +			__for_each_physical_engine(fd, e) {
> +				/* Requires master for STORE_DWORD on gen4/5 */
> +				igt_dynamic_f("%s", e->name)
> +					preempt(fd, e->flags, ncpus, 20);
> +			}
>   		}
>   	}
>   
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH] [PATCH i-g-t][V7]tests/i915/gem_sync.c :Added __for_each_physical_engine to utilize all available

[Melkaveri, Arjun]

On Wed, Apr 22, 2020 at 02:13:44PM +0100, Tvrtko Ursulin wrote:
> 
> On 14/04/2020 18:51, Arjun Melkaveri wrote:
> > Replaced the legacy for_each_engine* defines with the ones
> > implemented in the gem_engine_topology library.
> > 
> > Used  gem_context_clone_with_engines
> > to make sure that engine index was potentially created
> > based on a  default context with engine map configured.
> > 
> > Added Legacy engine coverage for sync_ring and sync_all.
> > 
> > Divided tests into static for ALL_Engine test cases
> > and dynamic for multiple engine .
> > 
> > V7:
> > Initializing engine and engine name with
> > maximum supported engines value.
> 
> You lost the rest of the change log, not terribly important though.
> 
> > 
> > Cc: Dec Katarzyna <katarzyna.dec@intel.com>
> > Cc: Ursulin Tvrtko <tvrtko.ursulin@intel.com>
> > Signed-off-by: sai gowtham <sai.gowtham.ch@intel.com>
> > Signed-off-by: Arjun Melkaveri <arjun.melkaveri@intel.com>
> > ---
> >   tests/i915/gem_sync.c | 310 ++++++++++++++++++++++++++----------------
> >   1 file changed, 195 insertions(+), 115 deletions(-)
> > 
> > diff --git a/tests/i915/gem_sync.c b/tests/i915/gem_sync.c
> > index 2ef55ecc..f1a87f59 100644
> > --- a/tests/i915/gem_sync.c
> > +++ b/tests/i915/gem_sync.c
> > @@ -24,6 +24,9 @@
> >   #include <time.h>
> >   #include <pthread.h>
> > +#include "igt_debugfs.h"
> > +#include "igt_dummyload.h"
> > +#include "igt_gt.h"
> >   #include "igt.h"
> >   #include "igt_sysfs.h"
> > @@ -37,6 +40,10 @@
> >   #define MIN_PRIO LOCAL_I915_CONTEXT_MIN_USER_PRIORITY
> >   #define ENGINE_MASK  (I915_EXEC_RING_MASK | LOCAL_I915_EXEC_BSD_MASK)
> > +#define RESET_TIMEOUT_MS (2 * MSEC_PER_SEC)
> > +static unsigned long reset_timeout_ms = RESET_TIMEOUT_MS;
> 
> I've asked in one of the previous rounds what's this for? It's only set in
> do_test and never acted upon.
> 
Will modify this in next patch
> > +#define NSEC_PER_MSEC (1000 * 1000ull)
> 
> Unused.
>
Will remove this 
> > +#define EXECBUF_MAX_ENGINES (I915_EXEC_RING_MASK + 1)
> >   IGT_TEST_DESCRIPTION("Basic check of ring<->ring write synchronisation.");
> > @@ -78,24 +85,35 @@ out:
> >   	return ts.tv_sec + 1e-9*ts.tv_nsec;
> >   }
> > +static void cleanup(int i915)
> > +{
> > +	igt_drop_caches_set(i915,
> > +			    /* cancel everything */
> > +			    DROP_RESET_ACTIVE | DROP_RESET_SEQNO |
> > +			    /* cleanup */
> > +			    DROP_ACTIVE | DROP_RETIRE | DROP_IDLE | DROP_FREED);
> > +	igt_require_gem(i915);
> > +}
> > +
> > +
> >   static void
> >   sync_ring(int fd, unsigned ring, int num_children, int timeout)
> >   {
> > -	unsigned engines[16];
> > -	const char *names[16];
> > +	const struct intel_execution_engine2 *e2;
> > +	unsigned int engines[EXECBUF_MAX_ENGINES];
> > +	const char *names[EXECBUF_MAX_ENGINES];
> >   	int num_engines = 0;
> >   	if (ring == ALL_ENGINES) {
> > -		for_each_physical_engine(e, fd) {
> > -			names[num_engines] = e->name;
> > -			engines[num_engines++] = eb_ring(e);
> > +		__for_each_physical_engine(fd, e2) {
> > +			names[num_engines] = strdup(e2->name);
> > +			engines[num_engines++] = e2->flags;
> >   			if (num_engines == ARRAY_SIZE(engines))
> >   				break;
> >   		}
> >   		num_children *= num_engines;
> >   	} else {
> > -		gem_require_ring(fd, ring);
> >   		names[num_engines] = NULL;
> >   		engines[num_engines++] = ring;
> >   	}
> > @@ -139,7 +157,7 @@ sync_ring(int fd, unsigned ring, int num_children, int timeout)
> >   }
> >   static void
> > -idle_ring(int fd, unsigned ring, int timeout)
> > +idle_ring(int fd, unsigned int ring, int num_children, int timeout)
> >   {
> >   	const uint32_t bbe = MI_BATCH_BUFFER_END;
> >   	struct drm_i915_gem_exec_object2 object;
> > @@ -180,23 +198,23 @@ idle_ring(int fd, unsigned ring, int timeout)
> >   static void
> >   wakeup_ring(int fd, unsigned ring, int timeout, int wlen)
> >   {
> > -	unsigned engines[16];
> > -	const char *names[16];
> > +	const struct intel_execution_engine2 *e2;
> > +	unsigned int engines[EXECBUF_MAX_ENGINES];
> > +	const char *names[EXECBUF_MAX_ENGINES];
> >   	int num_engines = 0;
> >   	if (ring == ALL_ENGINES) {
> > -		for_each_physical_engine(e, fd) {
> > -			if (!gem_can_store_dword(fd, eb_ring(e)))
> > +		__for_each_physical_engine(fd, e2) {
> > +			if (!gem_class_can_store_dword(fd, e2->class))
> >   				continue;
> > -			names[num_engines] = e->name;
> > -			engines[num_engines++] = eb_ring(e);
> > +			names[num_engines] = e2->name;
> 
> Looks like you need strdup here as well.
> 
I am changing this to use intel_engine_data, to remove usage of local
variable. 
> > +			engines[num_engines++] = e2->flags;
> >   			if (num_engines == ARRAY_SIZE(engines))
> >   				break;
> >   		}
> >   		igt_require(num_engines);
> >   	} else {
> > -		gem_require_ring(fd, ring);
> >   		igt_require(gem_can_store_dword(fd, ring));
> >   		names[num_engines] = NULL;
> >   		engines[num_engines++] = ring;
> > @@ -290,25 +308,26 @@ wakeup_ring(int fd, unsigned ring, int timeout, int wlen)
> >   	igt_assert_eq(intel_detect_and_clear_missed_interrupts(fd), 0);
> >   }
> > -static void active_ring(int fd, unsigned ring, int timeout)
> > +static void active_ring(int fd, unsigned int ring,
> > +			int num_children, int timeout)
> >   {
> > -	unsigned engines[16];
> > -	const char *names[16];
> > +	const struct intel_execution_engine2 *e2;
> > +	unsigned int engines[EXECBUF_MAX_ENGINES];
> > +	const char *names[EXECBUF_MAX_ENGINES];
> >   	int num_engines = 0;
> >   	if (ring == ALL_ENGINES) {
> > -		for_each_physical_engine(e, fd) {
> > -			if (!gem_can_store_dword(fd, eb_ring(e)))
> > +		__for_each_physical_engine(fd, e2) {
> > +			if (!gem_class_can_store_dword(fd, e2->class))
> >   				continue;
> > -			names[num_engines] = e->name;
> > -			engines[num_engines++] = eb_ring(e);
> > +			names[num_engines] = e2->name;
> 
> And here.
> 
> > +			engines[num_engines++] = e2->flags;
> >   			if (num_engines == ARRAY_SIZE(engines))
> >   				break;
> >   		}
> >   		igt_require(num_engines);
> >   	} else {
> > -		gem_require_ring(fd, ring);
> >   		igt_require(gem_can_store_dword(fd, ring));
> >   		names[num_engines] = NULL;
> >   		engines[num_engines++] = ring;
> > @@ -359,23 +378,23 @@ static void active_ring(int fd, unsigned ring, int timeout)
> >   static void
> >   active_wakeup_ring(int fd, unsigned ring, int timeout, int wlen)
> >   {
> > -	unsigned engines[16];
> > -	const char *names[16];
> > +	const struct intel_execution_engine2 *e2;
> > +	unsigned int engines[EXECBUF_MAX_ENGINES];
> > +	const char *names[EXECBUF_MAX_ENGINES];
> >   	int num_engines = 0;
> >   	if (ring == ALL_ENGINES) {
> > -		for_each_physical_engine(e, fd) {
> > -			if (!gem_can_store_dword(fd, eb_ring(e)))
> > +		__for_each_physical_engine(fd, e2) {
> > +			if (!gem_class_can_store_dword(fd, e2->class))
> >   				continue;
> > -			names[num_engines] = e->name;
> > -			engines[num_engines++] = eb_ring(e);
> > +			names[num_engines] = e2->name;
> 
> And here.
> 
> > +			engines[num_engines++] = e2->flags;
> >   			if (num_engines == ARRAY_SIZE(engines))
> >   				break;
> >   		}
> >   		igt_require(num_engines);
> >   	} else {
> > -		gem_require_ring(fd, ring);
> >   		igt_require(gem_can_store_dword(fd, ring));
> >   		names[num_engines] = NULL;
> >   		engines[num_engines++] = ring;
> > @@ -493,25 +512,25 @@ active_wakeup_ring(int fd, unsigned ring, int timeout, int wlen)
> >   static void
> >   store_ring(int fd, unsigned ring, int num_children, int timeout)
> >   {
> > +	const struct intel_execution_engine2 *e2;
> >   	const int gen = intel_gen(intel_get_drm_devid(fd));
> > -	unsigned engines[16];
> > -	const char *names[16];
> > +	unsigned int engines[EXECBUF_MAX_ENGINES];
> > +	const char *names[EXECBUF_MAX_ENGINES];
> >   	int num_engines = 0;
> >   	if (ring == ALL_ENGINES) {
> > -		for_each_physical_engine(e, fd) {
> > -			if (!gem_can_store_dword(fd, eb_ring(e)))
> > +		__for_each_physical_engine(fd, e2) {
> > +			if (!gem_class_can_store_dword(fd, e2->class))
> >   				continue;
> > -			names[num_engines] = e->name;
> > -			engines[num_engines++] = eb_ring(e);
> > +			names[num_engines] = e2->name;
> 
> And here.
> 
> > +			engines[num_engines++] = e2->flags;
> >   			if (num_engines == ARRAY_SIZE(engines))
> >   				break;
> >   		}
> >   		num_children *= num_engines;
> >   	} else {
> > -		gem_require_ring(fd, ring);
> >   		igt_require(gem_can_store_dword(fd, ring));
> >   		names[num_engines] = NULL;
> >   		engines[num_engines++] = ring;
> > @@ -608,27 +627,27 @@ store_ring(int fd, unsigned ring, int num_children, int timeout)
> >   static void
> >   switch_ring(int fd, unsigned ring, int num_children, int timeout)
> >   {
> > +	const struct intel_execution_engine2 *e2;
> >   	const int gen = intel_gen(intel_get_drm_devid(fd));
> > -	unsigned engines[16];
> > -	const char *names[16];
> > +	unsigned int engines[EXECBUF_MAX_ENGINES];
> > +	const char *names[EXECBUF_MAX_ENGINES];
> >   	int num_engines = 0;
> >   	gem_require_contexts(fd);
> >   	if (ring == ALL_ENGINES) {
> > -		for_each_physical_engine(e, fd) {
> > -			if (!gem_can_store_dword(fd, eb_ring(e)))
> > +		__for_each_physical_engine(fd, e2) {
> > +			if (!gem_class_can_store_dword(fd, e2->class))
> >   				continue;
> > -			names[num_engines] = e->name;
> > -			engines[num_engines++] = eb_ring(e);
> > +			names[num_engines] = e2->name;
> 
> Ditto.
> 
> > +			engines[num_engines++] = e2->flags;
> >   			if (num_engines == ARRAY_SIZE(engines))
> >   				break;
> >   		}
> >   		num_children *= num_engines;
> >   	} else {
> > -		gem_require_ring(fd, ring);
> >   		igt_require(gem_can_store_dword(fd, ring));
> >   		names[num_engines] = NULL;
> >   		engines[num_engines++] = ring;
> > @@ -931,10 +950,11 @@ __store_many(int fd, unsigned ring, int timeout, unsigned long *cycles)
> >   }
> >   static void
> > -store_many(int fd, unsigned ring, int timeout)
> > +store_many(int fd, unsigned int ring, int num_children, int timeout)
> >   {
> > +	const struct intel_execution_engine2 *e2;
> >   	unsigned long *shared;
> > -	const char *names[16];
> > +	const char *names[EXECBUF_MAX_ENGINES];
> >   	int n = 0;
> >   	shared = mmap(NULL, 4096, PROT_WRITE, MAP_SHARED | MAP_ANON, -1, 0);
> > @@ -943,21 +963,20 @@ store_many(int fd, unsigned ring, int timeout)
> >   	intel_detect_and_clear_missed_interrupts(fd);
> >   	if (ring == ALL_ENGINES) {
> > -		for_each_physical_engine(e, fd) {
> > -			if (!gem_can_store_dword(fd, eb_ring(e)))
> > +		__for_each_physical_engine(fd, e2) {
> > +			if (!gem_class_can_store_dword(fd, e2->class))
> >   				continue;
> >   			igt_fork(child, 1)
> >   				__store_many(fd,
> > -					     eb_ring(e),
> > +					     e2->flags,
> >   					     timeout,
> >   					     &shared[n]);
> > -			names[n++] = e->name;
> > +			names[n++] = e2->name;
> 
> Ditto.
> 
> >   		}
> >   		igt_waitchildren();
> >   	} else {
> > -		gem_require_ring(fd, ring);
> >   		igt_require(gem_can_store_dword(fd, ring));
> >   		__store_many(fd, ring, timeout, &shared[n]);
> >   		names[n++] = NULL;
> > @@ -1025,15 +1044,16 @@ sync_all(int fd, int num_children, int timeout)
> >   static void
> >   store_all(int fd, int num_children, int timeout)
> >   {
> > +	const struct intel_execution_engine2 *e;
> >   	const int gen = intel_gen(intel_get_drm_devid(fd));
> > -	unsigned engines[16];
> > +	unsigned int engines[EXECBUF_MAX_ENGINES];
> >   	int num_engines = 0;
> > -	for_each_physical_engine(e, fd) {
> > -		if (!gem_can_store_dword(fd, eb_ring(e)))
> > +	__for_each_physical_engine(fd, e) {
> > +		if (!gem_class_can_store_dword(fd, e->class))
> >   			continue;
> > -		engines[num_engines++] = eb_ring(e);
> > +		engines[num_engines++] = e->flags;
> >   		if (num_engines == ARRAY_SIZE(engines))
> >   			break;
> >   	}
> > @@ -1132,22 +1152,22 @@ store_all(int fd, int num_children, int timeout)
> >   static void
> >   preempt(int fd, unsigned ring, int num_children, int timeout)
> >   {
> > -	unsigned engines[16];
> > -	const char *names[16];
> > +	const struct intel_execution_engine2 *e2;
> > +	unsigned int engines[EXECBUF_MAX_ENGINES];
> > +	const char *names[EXECBUF_MAX_ENGINES];
> >   	int num_engines = 0;
> >   	uint32_t ctx[2];
> >   	if (ring == ALL_ENGINES) {
> > -		for_each_physical_engine(e, fd) {
> > -			names[num_engines] = e->name;
> > -			engines[num_engines++] = eb_ring(e);
> > +		__for_each_physical_engine(fd, e2) {
> > +			names[num_engines] = e2->name;
> 
> Last one.
> 
> > +			engines[num_engines++] = e2->flags;
> >   			if (num_engines == ARRAY_SIZE(engines))
> >   				break;
> >   		}
> >   		num_children *= num_engines;
> >   	} else {
> > -		gem_require_ring(fd, ring);
> >   		names[num_engines] = NULL;
> >   		engines[num_engines++] = ring;
> >   	}
> > @@ -1207,76 +1227,99 @@ preempt(int fd, unsigned ring, int num_children, int timeout)
> >   	gem_context_destroy(fd, ctx[0]);
> >   }
> > +static void do_test(void (*test)(int i915, unsigned int engine,
> > +		    int num_children, int test_timeout),
> > +		    int i915, unsigned int engine, int num_children,
> > +		    int test_timeout, const char *name)
> > +{
> > +#define ATTR "preempt_timeout_ms"
> > +	int timeout = -1;
> > +
> > +	cleanup(i915);
> > +
> > +	gem_engine_property_scanf(i915, name, ATTR, "%d", &timeout);
> > +	if (timeout != -1) {
> > +		igt_require(gem_engine_property_printf(i915, name,
> > +						       ATTR, "%d", 50) > 0);
> > +		reset_timeout_ms = 200;
> > +	}
> > +
> > +	test(i915, engine, num_children, test_timeout);
> > +	gem_quiescent_gpu(i915);
> 
> For stuff done in this wrapper - I don't really like stuffing in changes
> unrelated to patch title. If it is converting engine iteration it should
> stick to that. If you want to make some other improvements it should be a
> separate patch.
> 
I was acting on the reviews comments . by following 28e25ad1f1f987450f017d7f99548d2d7727d388
commit . Will clean up do_test ,as needed for this test.
> > +}
> > +
> >   igt_main
> >   {
> > -	const struct intel_execution_engine *e;
> >   	const int ncpus = sysconf(_SC_NPROCESSORS_ONLN);
> >   	int fd = -1;
> > +	struct {
> > +		const char *name;
> > +		void (*func)(int fd, unsigned int engine,
> > +			     int num_children, int timeout);
> > +		int num_children;
> > +		int timeout;
> > +	} *test, allengine[] = {
> > +		{ "basic-each", sync_ring, 1, 2},
> > +		{ "basic-store-each", store_ring, 1, 2},
> > +		{ "basic-many-each", store_many, 0, 2},
> > +		{ "switch-each", switch_ring, 1, 20},
> > +		{ "forked-switch-each", switch_ring, ncpus, 20},
> > +		{ "forked-each", sync_ring, ncpus, 20},
> > +		{ "forked-store-each", store_ring, ncpus, 20},
> > +		{ "active-each", active_ring, 0, 20},
> > +		{ "wakeup-each", wakeup_ring, 20, 1},
> > +		{ "active-wakeup-each", active_wakeup_ring, 20, 1},
> > +		{ "double-wakeup-each", wakeup_ring, 20, 2},
> > +		{ NULL, NULL },
> > +	}, tests[] = {
> > +		{ "default", sync_ring, 1, 20},
> > +		{ "idle", idle_ring, 0, 20},
> > +		{ "active", active_ring, 0, 20},
> > +		{ "wakeup", wakeup_ring, 20, 1},
> > +		{ "active-wakeup", active_wakeup_ring, 20, 1},
> > +		{ "double-wakeup", wakeup_ring, 20, 2},
> > +		{ "store", store_ring, 1, 20},
> > +		{ "switch", switch_ring, 1, 20},
> > +		{ "forked-switch", switch_ring, ncpus, 20},
> > +		{ "many", store_many, 0, 20},
> > +		{ "forked", sync_ring, ncpus, 20},
> > +		{ "forked-store", store_ring, ncpus, 20},
> > +		{ NULL, NULL },
> > +	};
> > +
> > +
> >   	igt_fixture {
> >   		fd = drm_open_driver(DRIVER_INTEL);
> >   		igt_require_gem(fd);
> >   		gem_submission_print_method(fd);
> >   		gem_scheduler_print_capability(fd);
> > -
> 
> Best to avoid noise.
> 
> >   		igt_fork_hang_detector(fd);
> >   	}
> > -	for (e = intel_execution_engines; e->name; e++) {
> > -		igt_subtest_f("%s", e->name)
> > -			sync_ring(fd, eb_ring(e), 1, 20);
> > -		igt_subtest_f("idle-%s", e->name)
> > -			idle_ring(fd, eb_ring(e), 20);
> > -		igt_subtest_f("active-%s", e->name)
> > -			active_ring(fd, eb_ring(e), 20);
> > -		igt_subtest_f("wakeup-%s", e->name)
> > -			wakeup_ring(fd, eb_ring(e), 20, 1);
> > -		igt_subtest_f("active-wakeup-%s", e->name)
> > -			active_wakeup_ring(fd, eb_ring(e), 20, 1);
> > -		igt_subtest_f("double-wakeup-%s", e->name)
> > -			wakeup_ring(fd, eb_ring(e), 20, 2);
> > -		igt_subtest_f("store-%s", e->name)
> > -			store_ring(fd, eb_ring(e), 1, 20);
> > -		igt_subtest_f("switch-%s", e->name)
> > -			switch_ring(fd, eb_ring(e), 1, 20);
> > -		igt_subtest_f("forked-switch-%s", e->name)
> > -			switch_ring(fd, eb_ring(e), ncpus, 20);
> > -		igt_subtest_f("many-%s", e->name)
> > -			store_many(fd, eb_ring(e), 20);
> > -		igt_subtest_f("forked-%s", e->name)
> > -			sync_ring(fd, eb_ring(e), ncpus, 20);
> > -		igt_subtest_f("forked-store-%s", e->name)
> > -			store_ring(fd, eb_ring(e), ncpus, 20);
> > -	}
> > +	/* legacy of selecting engines. */
> > -	igt_subtest("basic-each")
> > -		sync_ring(fd, ALL_ENGINES, 1, 2);
> > -	igt_subtest("basic-store-each")
> > -		store_ring(fd, ALL_ENGINES, 1, 2);
> > -	igt_subtest("basic-many-each")
> > -		store_many(fd, ALL_ENGINES, 2);
> > -	igt_subtest("switch-each")
> > -		switch_ring(fd, ALL_ENGINES, 1, 20);
> > -	igt_subtest("forked-switch-each")
> > -		switch_ring(fd, ALL_ENGINES, ncpus, 20);
> > -	igt_subtest("forked-each")
> > -		sync_ring(fd, ALL_ENGINES, ncpus, 20);
> > -	igt_subtest("forked-store-each")
> > -		store_ring(fd, ALL_ENGINES, ncpus, 20);
> > -	igt_subtest("active-each")
> > -		active_ring(fd, ALL_ENGINES, 20);
> > -	igt_subtest("wakeup-each")
> > -		wakeup_ring(fd, ALL_ENGINES, 20, 1);
> > -	igt_subtest("active-wakeup-each")
> > -		active_wakeup_ring(fd, ALL_ENGINES, 20, 1);
> > -	igt_subtest("double-wakeup-each")
> > -		wakeup_ring(fd, ALL_ENGINES, 20, 2);
> > +	igt_subtest_group {
> > +		for (test = tests; test->name; test++) {
> > +			igt_subtest_with_dynamic_f("legacy-engines-%s",
> 
> Have we not agreed to not add "engines" to test name?
> 
I removed it for other test group. will modify this too .
> > +						   test->name) {
> > +				for_each_physical_engine(e, fd) {
> > +					igt_dynamic_f("%s", e->name) {
> > +						do_test(test->func,
> > +							fd, eb_ring(e),
> > +							test->num_children,
> > +							test->timeout,
> > +							e->full_name);
> > +					}
> > +				}
> > +			}
> > +		}
> > +	}
> >   	igt_subtest("basic-all")
> >   		sync_all(fd, 1, 2);
> >   	igt_subtest("basic-store-all")
> >   		store_all(fd, 1, 2);
> > -
> 
> Noise as well.
> 
> >   	igt_subtest("all")
> >   		sync_all(fd, 1, 20);
> >   	igt_subtest("store-all")
> > @@ -1286,7 +1329,42 @@ igt_main
> >   	igt_subtest("forked-store-all")
> >   		store_all(fd, ncpus, 20);
> > +	/* All Engines */
> >   	igt_subtest_group {
> > +		for (test = allengine; test->name; test++) {
> > +			igt_subtest_f("%s", test->name) {
> > +				do_test(test->func,
> > +					fd, ALL_ENGINES,
> > +					test->num_children,
> > +					test->timeout,
> > +					test->name);
> > +			}
> > +		}
> > +	}
> > +
> > +	/* New way of selecting engines. */
> > +	igt_subtest_group {
> > +		const struct intel_execution_engine2 *e;
> > +
> > +		for (test = tests; test->name; test++) {
> > +			igt_subtest_with_dynamic_f("%s", test->name) {
> > +				__for_each_physical_engine(fd, e) {
> > +					igt_dynamic_f("%s", e->name) {
> > +						do_test(test->func,
> > +							fd, e->flags,
> > +							test->num_children,
> > +							test->timeout,
> > +							e->name);
> > +					}
> > +				}
> > +			}
> > +		}
> > +	}
> > +
> > +
> > +	igt_subtest_group {
> > +		const struct intel_execution_engine2 *e;
> > +
> >   		igt_fixture {
> 
> Regards,
> 
> Tvrtko
> 
> >   			gem_require_contexts(fd);
> >   			igt_require(gem_scheduler_has_ctx_priority(fd));
> > @@ -1295,10 +1373,12 @@ igt_main
> >   		igt_subtest("preempt-all")
> >   			preempt(fd, ALL_ENGINES, 1, 20);
> > -
> > -		for (e = intel_execution_engines; e->name; e++) {
> > -			igt_subtest_f("preempt-%s", e->name)
> > -				preempt(fd, eb_ring(e), ncpus, 20);
> > +		igt_subtest_with_dynamic("preempt") {
> > +			__for_each_physical_engine(fd, e) {
> > +				/* Requires master for STORE_DWORD on gen4/5 */
> > +				igt_dynamic_f("%s", e->name)
> > +					preempt(fd, e->flags, ncpus, 20);
> > +			}
> >   		}
> >   	}
> > 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev