Skip to contentSkip to conversation

tests/i915/gem_sync.c :Added __for_each_physical_engine to utilize all available

PATCH Superseded Arjun Melkaveri 3 participants 5 messages Apr 14, 2020 – Apr 22, 2020

v7 of 8 Superseded Latest is v8 →

lore
Share & tools
lore

Arjun Melkaveri

author
#0 lore raw

PATCH v7 1/1 tests/i915/gem_sync.c :Added __for_each_physical_engine to utilize all available

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 <[email protected]>
Cc Ursulin Tvrtko <[email protected]>
Signed-off-by sai gowtham <[email protected]>
Signed-off-by Arjun Melkaveri <[email protected]>
---
 tests/i915/gem_sync.c | 310 ++++++++++++++++++++++++++----------------
 1 file changed, 195 insertions(+), 115 deletions(-)
tests/i915/gem_sync.c +195 -115
index 2ef55ecc..f1a87f59 100644
@@ -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);
+			}
 		}
 	}
Signature · 6 lines
2.25.1

_______________________________________________
igt-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/igt-dev
CI
#1 lore raw

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

== Series Details ==

== 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
[email protected]
https://lists.freedesktop.org/mailman/listinfo/igt-dev

CI
#2 lore raw

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

In reply to Arjun Melkaveri
== Series Details ==

== 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
[email protected]
https://lists.freedesktop.org/mailman/listinfo/igt-dev

#3 lore raw
In reply to Arjun Melkaveri
Quoted 7 lines · 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.

Quoted 8 lines

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.

Quoted 24 lines
Cc Dec Katarzyna <[email protected]>
Cc Ursulin Tvrtko <[email protected]>
Signed-off-by sai gowtham <[email protected]>
Signed-off-by Arjun Melkaveri <[email protected]>
---
  tests/i915/gem_sync.c | 310 ++++++++++++++++++++++++++----------------
  1 file changed, 195 insertions(+), 115 deletions(-)
tests/i915/gem_sync.c +3 -0
index 2ef55ecc..f1a87f59 100644
@@ -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"

tests/i915/gem_sync.c line 44

Quoted 5 lines
tests/i915/gem_sync.c +2 -0
@@ -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.

tests/i915/gem_sync.c line 45

Quoted 1 line
tests/i915/gem_sync.c +1 -0
+#define NSEC_PER_MSEC (1000 * 1000ull)

Unused.

Quoted 66 lines
tests/i915/gem_sync.c +22 -9
+#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;

tests/i915/gem_sync.c idle_ring(int fd, unsigned ring, int timeout) · line 211

Quoted 10 lines
tests/i915/gem_sync.c +3 -4
  	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.

Quoted 25 lines
tests/i915/gem_sync.c +6 -4
+			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;

tests/i915/gem_sync.c wakeup_ring(int fd, unsigned ring, int timeout, int wlen) · line 324

Quoted 10 lines
tests/i915/gem_sync.c +3 -4
  	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.

Quoted 21 lines
tests/i915/gem_sync.c +4 -3
+			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;

tests/i915/gem_sync.c static void active_ring(int fd, unsigned ring, int timeout) · line 391

Quoted 10 lines
tests/i915/gem_sync.c +3 -4
  	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.

Quoted 22 lines
tests/i915/gem_sync.c +4 -3
+			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;

tests/i915/gem_sync.c active_wakeup_ring(int fd, unsigned ring, int timeout, int wlen) · line 526

Quoted 10 lines
tests/i915/gem_sync.c +3 -4
  	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.

Quoted 25 lines
tests/i915/gem_sync.c +4 -3
+			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);

tests/i915/gem_sync.c store_ring(int fd, unsigned ring, int num_children, int timeout) · line 643

Quoted 10 lines
tests/i915/gem_sync.c +3 -4
  	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.

Quoted 34 lines
tests/i915/gem_sync.c +6 -5
+			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;

tests/i915/gem_sync.c store_many(int fd, unsigned ring, int timeout) · line 976

Quoted 10 lines
tests/i915/gem_sync.c +2 -2
 
  			igt_fork(child, 1)
  				__store_many(fd,
-					     eb_ring(e),
+					     e2->flags,
  					     timeout,
  					     &shared[n]);
 
-			names[n++] = e->name;
+			names[n++] = e2->name;

Ditto.

Quoted 36 lines
tests/i915/gem_sync.c +7 -7
  		}
  		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];

tests/i915/gem_sync.c store_all(int fd, int num_children, int timeout) · line 1163

Quoted 10 lines
tests/i915/gem_sync.c +3 -3
+	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.

Quoted 24 lines
tests/i915/gem_sync.c +10 -1
+			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);

tests/i915/gem_sync.c preempt(int fd, unsigned ring, int num_children, int timeout) · line 1248

Quoted 10 lines
tests/i915/gem_sync.c +10 -0
+
+	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.

Quoted 40 lines
tests/i915/gem_sync.c +34 -1
+}
+
  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},

tests/i915/gem_sync.c preempt(int fd, unsigned ring, int num_children, int timeout) · line 1221

Quoted 10 lines
tests/i915/gem_sync.c +4 -1
+		{ 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.

Quoted 46 lines
tests/i915/gem_sync.c +1 -41
  		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")

tests/i915/gem_sync.c preempt(int fd, unsigned ring, int num_children, int timeout) · line 1304

Quoted 10 lines
tests/i915/gem_sync.c +3 -7
-		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?

Quoted 9 lines
tests/i915/gem_sync.c +9 -0
+						   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);
+					}

tests/i915/gem_sync.c preempt(int fd, unsigned ring, int num_children, int timeout) · line 1279

Quoted 10 lines
tests/i915/gem_sync.c +4 -1
+				}
+			}
+		}
+	}
 
  	igt_subtest("basic-all")
  		sync_all(fd, 1, 2);
  	igt_subtest("basic-store-all")
  		store_all(fd, 1, 2);
-

Noise as well.

Quoted 34 lines
tests/i915/gem_sync.c +26 -0
  	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);
+					}

tests/i915/gem_sync.c igt_main · line 1368

Quoted 10 lines
tests/i915/gem_sync.c +9 -0
+				}
+			}
+		}
+	}
+
+
+	igt_subtest_group {
+		const struct intel_execution_engine2 *e;
+
  		igt_fixture {

Regards,

Tvrtko

Quoted 10 lines
tests/i915/gem_sync.c +0 -3
  			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)

tests/i915/gem_sync.c igt_main · line 1384

Quoted 10 lines
tests/i915/gem_sync.c +6 -1
-				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
[email protected]
https://lists.freedesktop.org/mailman/listinfo/igt-dev

author
#4 lore raw
Quoted 41 lines · Tvrtko Ursulin
Quoted 7 lines · 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.

Quoted 8 lines

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.

Quoted 21 lines
Cc Dec Katarzyna <[email protected]>
Cc Ursulin Tvrtko <[email protected]>
Signed-off-by sai gowtham <[email protected]>
Signed-off-by Arjun Melkaveri <[email protected]>
---
  tests/i915/gem_sync.c | 310 ++++++++++++++++++++++++++----------------
  1 file changed, 195 insertions(+), 115 deletions(-)
tests/i915/gem_sync.c +3 -0
index 2ef55ecc..f1a87f59 100644
@@ -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"
Quoted 9 lines

tests/i915/gem_sync.c line 43

Quoted 5 lines
tests/i915/gem_sync.c +2 -0
@@ -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

Quoted 4 lines

tests/i915/gem_sync.c line 45

Quoted 1 line
tests/i915/gem_sync.c +1 -0
+#define NSEC_PER_MSEC (1000 * 1000ull)

Unused.

Will remove this

Quoted 62 lines
Quoted 62 lines
tests/i915/gem_sync.c +22 -11
+#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)))
Quoted 9 lines

tests/i915/gem_sync.c idle_ring(int fd, unsigned ring, int timeout) · line 211

Quoted 6 lines
tests/i915/gem_sync.c +3 -2
+		__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.

Quoted 250 lines
Quoted 22 lines
tests/i915/gem_sync.c +6 -4
+			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];

tests/i915/gem_sync.c wakeup_ring(int fd, unsigned ring, int timeout, int wlen) · line 321

Quoted 10 lines
tests/i915/gem_sync.c +3 -4
  	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.

Quoted 19 lines
tests/i915/gem_sync.c +4 -3
+			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];

tests/i915/gem_sync.c static void active_ring(int fd, unsigned ring, int timeout) · line 389

Quoted 10 lines
tests/i915/gem_sync.c +3 -4
  	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.

Quoted 20 lines
tests/i915/gem_sync.c +4 -3
+			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];

tests/i915/gem_sync.c active_wakeup_ring(int fd, unsigned ring, int timeout, int wlen) · line 524

Quoted 10 lines
tests/i915/gem_sync.c +3 -4
  	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.

Quoted 21 lines
tests/i915/gem_sync.c +4 -3
+			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;

tests/i915/gem_sync.c store_ring(int fd, unsigned ring, int num_children, int timeout) · line 640

Quoted 10 lines
tests/i915/gem_sync.c +3 -4
  	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.

Quoted 28 lines
tests/i915/gem_sync.c +5 -5
+			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) {

tests/i915/gem_sync.c store_many(int fd, unsigned ring, int timeout) · line 973

Quoted 10 lines
tests/i915/gem_sync.c +3 -2
+			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.

Quoted 33 lines
tests/i915/gem_sync.c +6 -7
  		}
  		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;

tests/i915/gem_sync.c store_all(int fd, int num_children, int timeout) · line 1162

Quoted 10 lines
tests/i915/gem_sync.c +4 -3
+	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.

Quoted 29 lines
tests/i915/gem_sync.c +17 -1
+			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;
+	}
Quoted 9 lines

tests/i915/gem_sync.c preempt(int fd, unsigned ring, int num_children, int timeout) · line 1248

Quoted 3 lines
tests/i915/gem_sync.c +3 -0
+
+	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.

Quoted 100 lines
Quoted 39 lines
tests/i915/gem_sync.c +34 -1
+}
+
  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},

tests/i915/gem_sync.c preempt(int fd, unsigned ring, int num_children, int timeout) · line 1221

Quoted 10 lines
tests/i915/gem_sync.c +4 -1
+		{ 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.

Quoted 48 lines
tests/i915/gem_sync.c +1 -45
  		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")
Quoted 9 lines

tests/i915/gem_sync.c preempt(int fd, unsigned ring, int num_children, int timeout) · line 1304

Quoted 6 lines
tests/i915/gem_sync.c +3 -3
-		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 .

Quoted 79 lines
Quoted 8 lines
tests/i915/gem_sync.c +8 -0
+						   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);

tests/i915/gem_sync.c preempt(int fd, unsigned ring, int num_children, int timeout) · line 1279

Quoted 10 lines
tests/i915/gem_sync.c +5 -1
+					}
+				}
+			}
+		}
+	}
  	igt_subtest("basic-all")
  		sync_all(fd, 1, 2);
  	igt_subtest("basic-store-all")
  		store_all(fd, 1, 2);
-

Noise as well.

Quoted 33 lines
tests/i915/gem_sync.c +26 -0
  	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);
+					}

tests/i915/gem_sync.c igt_main · line 1367

Quoted 10 lines
tests/i915/gem_sync.c +9 -0
+				}
+			}
+		}
+	}
+
+
+	igt_subtest_group {
+		const struct intel_execution_engine2 *e;
+
  		igt_fixture {

Regards,

Tvrtko

Quoted 10 lines
tests/i915/gem_sync.c +1 -4
  			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") {
Quoted 8 lines

tests/i915/gem_sync.c igt_main · line 1384

Quoted 8 lines
tests/i915/gem_sync.c +5 -0
+			__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
[email protected]
https://lists.freedesktop.org/mailman/listinfo/igt-dev