[PATCH igt 1/2] Iterate over physical engines

[PATCH igt 1/2] Iterate over physical engines

[Chris Wilson]

We current have a single for_each_engine() iterator which we use to
generate both a set of uABI engines and a set of physical engines.
Determining what uABI ring-id corresponds to an actual HW engine is
tricky, so pull that out to a library function and introduce
for_each_physical_engine() for cases where we want to issue requests
once on each HW ring (avoiding aliasing issues).

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 lib/igt_gt.c               | 23 +++++++++++++
 lib/igt_gt.h               |  9 +++++
 tests/amdgpu/amd_prime.c   |  6 +---
 tests/gem_concurrent_all.c |  2 +-
 tests/gem_ctx_create.c     |  5 +--
 tests/gem_exec_await.c     | 17 +--------
 tests/gem_exec_create.c    | 17 +--------
 tests/gem_exec_fence.c     | 16 ++-------
 tests/gem_exec_gttfill.c   | 16 +--------
 tests/gem_exec_nop.c       | 32 +++--------------
 tests/gem_exec_parallel.c  | 15 ++------
 tests/gem_exec_reloc.c     |  2 +-
 tests/gem_exec_schedule.c  | 21 ++---------
 tests/gem_exec_store.c     |  2 +-
 tests/gem_exec_suspend.c   | 20 +++--------
 tests/gem_exec_whisper.c   | 15 ++------
 tests/gem_ring_sync_loop.c |  2 +-
 tests/gem_spin_batch.c     |  5 +--
 tests/gem_sync.c           | 86 +++++++---------------------------------------
 19 files changed, 72 insertions(+), 239 deletions(-)

diff --git a/lib/igt_gt.c b/lib/igt_gt.c
index f70fcb92..e630550b 100644
--- a/lib/igt_gt.c
+++ b/lib/igt_gt.c
@@ -660,3 +660,26 @@ bool gem_has_engine(int gem_fd,
 			    gem_class_instance_to_eb_flags(gem_fd, class,
 							   instance));
 }
+
+bool gem_ring_is_physical_engine(int fd, unsigned ring)
+{
+	if (ring == I915_EXEC_DEFAULT)
+		return false;
+
+	/* BSD uses an extra flag to chose between aliasing modes */
+	if ((ring & 63) == I915_EXEC_BSD) {
+		bool explicit_bsd = ring & (3 << 13);
+		bool has_bsd2 = gem_has_bsd2(fd);
+		return explicit_bsd ? has_bsd2 : !has_bsd2;
+	}
+
+	return true;
+}
+
+bool gem_ring_has_physical_engine(int fd, unsigned ring)
+{
+	if (!gem_ring_is_physical_engine(fd, ring))
+		return false;
+
+	return gem_has_ring(fd, ring);
+}
diff --git a/lib/igt_gt.h b/lib/igt_gt.h
index 68592410..4d9d1aa0 100644
--- a/lib/igt_gt.h
+++ b/lib/igt_gt.h
@@ -81,6 +81,15 @@ extern const struct intel_execution_engine {
 	     e__++) \
 		for_if (gem_has_ring(fd__, flags__ = e__->exec_id | e__->flags))
 
+#define for_each_physical_engine(fd__, flags__) \
+	for (const struct intel_execution_engine *e__ = intel_execution_engines;\
+	     e__->name; \
+	     e__++) \
+		for_if (gem_ring_has_physical_engine(fd__, flags__ = e__->exec_id | e__->flags))
+
+bool gem_ring_is_physical_engine(int fd, unsigned int ring);
+bool gem_ring_has_physical_engine(int fd, unsigned int ring);
+
 bool gem_can_store_dword(int fd, unsigned int engine);
 
 extern const struct intel_execution_engine2 {
diff --git a/tests/amdgpu/amd_prime.c b/tests/amdgpu/amd_prime.c
index b2f326b4..bb68ccf3 100644
--- a/tests/amdgpu/amd_prime.c
+++ b/tests/amdgpu/amd_prime.c
@@ -179,12 +179,8 @@ static void i915_to_amd(int i915, int amd, amdgpu_device_handle device)
 	struct cork c;
 
 	nengine = 0;
-	for_each_engine(i915, engine) {
-		if (engine == 0)
-			continue;
-
+	for_each_physical_engine(i915, engine)
 		engines[nengine++] = engine;
-	}
 	igt_require(nengine);
 
 	memset(obj, 0, sizeof(obj));
diff --git a/tests/gem_concurrent_all.c b/tests/gem_concurrent_all.c
index 201b491b..3a1097ba 100644
--- a/tests/gem_concurrent_all.c
+++ b/tests/gem_concurrent_all.c
@@ -960,7 +960,7 @@ static igt_hang_t all_hang(void)
 	execbuf.buffers_ptr = to_user_pointer(&obj);
 	execbuf.buffer_count = 1;
 
-	for_each_engine(fd, engine) {
+	for_each_physical_engine(fd, engine) {
 		hang = igt_hang_ring(fd, engine);
 
 		execbuf.flags = engine;
diff --git a/tests/gem_ctx_create.c b/tests/gem_ctx_create.c
index 058445b6..1b32d6c3 100644
--- a/tests/gem_ctx_create.c
+++ b/tests/gem_ctx_create.c
@@ -321,11 +321,8 @@ igt_main
 		igt_require_gem(fd);
 		gem_require_contexts(fd);
 
-		for_each_engine(fd, engine) {
-			if (engine == 0)
-				continue;
+		for_each_physical_engine(fd, engine)
 			all_engines[all_nengine++] = engine;
-		}
 		igt_require(all_nengine);
 
 		if (gem_uses_full_ppgtt(fd)) {
diff --git a/tests/gem_exec_await.c b/tests/gem_exec_await.c
index e19363c4..fccc24d9 100644
--- a/tests/gem_exec_await.c
+++ b/tests/gem_exec_await.c
@@ -44,17 +44,6 @@ static double elapsed(const struct timespec *start, const struct timespec *end)
 		(end->tv_nsec - start->tv_nsec)*1e-9);
 }
 
-static bool ignore_engine(int fd, unsigned engine)
-{
-	if (engine == 0)
-		return true;
-
-	if (gem_has_bsd2(fd) && engine == I915_EXEC_BSD)
-		return true;
-
-	return false;
-}
-
 static void xchg_obj(void *array, unsigned i, unsigned j)
 {
 	struct drm_i915_gem_exec_object2 *obj = array;
@@ -89,12 +78,8 @@ static void wide(int fd, int ring_size, int timeout, unsigned int flags)
 	double time;
 
 	nengine = 0;
-	for_each_engine(fd, engine) {
-		if (ignore_engine(fd, engine))
-			continue;
-
+	for_each_physical_engine(fd, engine)
 		engines[nengine++] = engine;
-	}
 	igt_require(nengine);
 
 	exec = calloc(nengine, sizeof(*exec));
diff --git a/tests/gem_exec_create.c b/tests/gem_exec_create.c
index 28479a9d..54a2429e 100644
--- a/tests/gem_exec_create.c
+++ b/tests/gem_exec_create.c
@@ -54,17 +54,6 @@ static double elapsed(const struct timespec *start, const struct timespec *end)
 		(end->tv_nsec - start->tv_nsec)*1e-9);
 }
 
-static bool ignore_engine(int fd, unsigned engine)
-{
-	if (engine == 0)
-		return true;
-
-	if (gem_has_bsd2(fd) && engine == I915_EXEC_BSD)
-		return true;
-
-	return false;
-}
-
 #define LEAK 0x1
 
 static void all(int fd, unsigned flags, int timeout, int ncpus)
@@ -77,12 +66,8 @@ static void all(int fd, unsigned flags, int timeout, int ncpus)
 	unsigned engine;
 
 	nengine = 0;
-	for_each_engine(fd, engine) {
-		if (ignore_engine(fd, engine))
-			continue;
-
+	for_each_physical_engine(fd, engine)
 		engines[nengine++] = engine;
-	}
 	igt_require(nengine);
 
 	memset(&obj, 0, sizeof(obj));
diff --git a/tests/gem_exec_fence.c b/tests/gem_exec_fence.c
index 312505d6..39ca17ee 100644
--- a/tests/gem_exec_fence.c
+++ b/tests/gem_exec_fence.c
@@ -273,7 +273,7 @@ static void test_fence_await(int fd, unsigned ring, unsigned flags)
 	igt_assert(fence != -1);
 
 	i = 0;
-	for_each_engine(fd, engine) {
+	for_each_physical_engine(fd, engine) {
 		if (!gem_can_store_dword(fd, engine))
 			continue;
 
@@ -521,10 +521,7 @@ static void test_parallel(int fd, unsigned int master)
 	obj[BATCH].relocation_count = 1;
 
 	/* Queue all secondaries */
-	for_each_engine(fd, engine) {
-		if (engine == 0 || engine == I915_EXEC_BSD)
-			continue;
-
+	for_each_physical_engine(fd, engine) {
 		if (engine == master)
 			continue;
 
@@ -699,15 +696,8 @@ static void test_long_history(int fd, long ring_size, unsigned flags)
 		limit = ring_size / 3;
 
 	nengine = 0;
-	for_each_engine(fd, engine) {
-		if (engine == 0)
-			continue;
-
-		if (engine == I915_EXEC_BSD)
-			continue;
-
+	for_each_physical_engine(fd, engine)
 		engines[nengine++] = engine;
-	}
 	igt_require(nengine);
 
 	gem_quiescent_gpu(fd);
diff --git a/tests/gem_exec_gttfill.c b/tests/gem_exec_gttfill.c
index 96ed832f..4097e407 100644
--- a/tests/gem_exec_gttfill.c
+++ b/tests/gem_exec_gttfill.c
@@ -28,17 +28,6 @@ IGT_TEST_DESCRIPTION("Fill the GTT with batches.");
 
 #define BATCH_SIZE (4096<<10)
 
-static bool ignore_engine(int fd, unsigned engine)
-{
-	if (engine == 0)
-		return true;
-
-	if (gem_has_bsd2(fd) && engine == I915_EXEC_BSD)
-		return true;
-
-	return false;
-}
-
 static void xchg_u32(void *array, unsigned i, unsigned j)
 {
 	uint32_t *u32 = array;
@@ -126,10 +115,7 @@ static void fillgtt(int fd, unsigned ring, int timeout)
 
 	nengine = 0;
 	if (ring == 0) {
-		for_each_engine(fd, engine) {
-			if (ignore_engine(fd, engine))
-				continue;
-
+		for_each_physical_engine(fd, engine) {
 			if (!gem_can_store_dword(fd, engine))
 				continue;
 
diff --git a/tests/gem_exec_nop.c b/tests/gem_exec_nop.c
index d3e9a3e0..d971ffcb 100644
--- a/tests/gem_exec_nop.c
+++ b/tests/gem_exec_nop.c
@@ -188,17 +188,6 @@ static void headless(int fd, uint32_t handle)
 	assert_within_epsilon(n_headless, n_display, 0.1f);
 }
 
-static bool ignore_engine(int fd, unsigned engine)
-{
-	if (engine == 0)
-		return true;
-
-	if (gem_has_bsd2(fd) && engine == I915_EXEC_BSD)
-		return true;
-
-	return false;
-}
-
 static void parallel(int fd, uint32_t handle, int timeout)
 {
 	struct drm_i915_gem_execbuffer2 execbuf;
@@ -212,10 +201,7 @@ static void parallel(int fd, uint32_t handle, int timeout)
 
 	sum = 0;
 	nengine = 0;
-	for_each_engine(fd, engine) {
-		if (ignore_engine(fd, engine))
-			continue;
-
+	for_each_physical_engine(fd, engine) {
 		engines[nengine] = engine;
 		names[nengine] = e__->name;
 		nengine++;
@@ -277,10 +263,7 @@ static void series(int fd, uint32_t handle, int timeout)
 	const char *name;
 
 	nengine = 0;
-	for_each_engine(fd, engine) {
-		if (ignore_engine(fd, engine))
-			continue;
-
+	for_each_physical_engine(fd, engine) {
 		time = nop_on_ring(fd, handle, engine, 1, &count) / count;
 		if (time > max) {
 			name = e__->name;
@@ -375,12 +358,9 @@ static void sequential(int fd, uint32_t handle, unsigned flags, int timeout)
 
 	nengine = 0;
 	sum = 0;
-	for_each_engine(fd, n) {
+	for_each_physical_engine(fd, n) {
 		unsigned long count;
 
-		if (ignore_engine(fd, n))
-			continue;
-
 		time = nop_on_ring(fd, handle, n, 1, &count) / count;
 		sum += time;
 		igt_debug("%s: %.3fus\n", e__->name, 1e6*time);
@@ -509,12 +489,8 @@ static void fence_signal(int fd, uint32_t handle,
 
 	nengine = 0;
 	if (ring_id == -1) {
-		for_each_engine(fd, n) {
-			if (ignore_engine(fd, n))
-				continue;
-
+		for_each_physical_engine(fd, n)
 			engines[nengine++] = n;
-		}
 	} else {
 		gem_require_ring(fd, ring_id);
 		engines[nengine++] = ring_id;
diff --git a/tests/gem_exec_parallel.c b/tests/gem_exec_parallel.c
index 11b6e775..fe5ffe8f 100644
--- a/tests/gem_exec_parallel.c
+++ b/tests/gem_exec_parallel.c
@@ -55,17 +55,6 @@ static void check_bo(int fd, uint32_t handle, int pass)
 	munmap(map, 4096);
 }
 
-static bool ignore_engine(int fd, unsigned engine)
-{
-	if (engine == 0)
-		return true;
-
-	if (!gem_can_store_dword(fd, engine))
-		return true;
-
-	return false;
-}
-
 #define CONTEXTS 0x1
 #define FDS 0x2
 
@@ -180,8 +169,8 @@ static void all(int fd, unsigned engine, unsigned flags)
 
 	nengine = 0;
 	if (engine == -1) {
-		for_each_engine(fd, engine) {
-			if (!ignore_engine(fd, engine))
+		for_each_physical_engine(fd, engine) {
+			if (gem_can_store_dword(fd, engine))
 				engines[nengine++] = engine;
 		}
 	} else {
diff --git a/tests/gem_exec_reloc.c b/tests/gem_exec_reloc.c
index 432a42a9..213de1d7 100644
--- a/tests/gem_exec_reloc.c
+++ b/tests/gem_exec_reloc.c
@@ -258,7 +258,7 @@ static void active(int fd, unsigned engine)
 
 	nengine = 0;
 	if (engine == -1) {
-		for_each_engine(fd, engine) {
+		for_each_physical_engine(fd, engine) {
 			if (gem_can_store_dword(fd, engine))
 				engines[nengine++] = engine;
 		}
diff --git a/tests/gem_exec_schedule.c b/tests/gem_exec_schedule.c
index 5f24df33..03d457e9 100644
--- a/tests/gem_exec_schedule.c
+++ b/tests/gem_exec_schedule.c
@@ -188,17 +188,6 @@ static void fifo(int fd, unsigned ring)
 	munmap(ptr, 4096);
 }
 
-static bool ignore_engine(int fd, unsigned engine)
-{
-	if (engine == 0)
-		return true;
-
-	if (gem_has_bsd2(fd) && engine == I915_EXEC_BSD)
-		return true;
-
-	return false;
-}
-
 static void smoketest(int fd, unsigned ring, unsigned timeout)
 {
 	const int ncpus = sysconf(_SC_NPROCESSORS_ONLN);
@@ -210,12 +199,8 @@ static void smoketest(int fd, unsigned ring, unsigned timeout)
 
 	nengine = 0;
 	if (ring == -1) {
-		for_each_engine(fd, engine) {
-			if (ignore_engine(fd, engine))
-				continue;
-
+		for_each_physical_engine(fd, engine)
 			engines[nengine++] = engine;
-		}
 	} else {
 		engines[nengine++] = ring;
 	}
@@ -440,7 +425,7 @@ static void preempt_other(int fd, unsigned ring)
 	gem_context_set_priority(fd, ctx[HI], MAX_PRIO);
 
 	n = 0;
-	for_each_engine(fd, other) {
+	for_each_physical_engine(fd, other) {
 		igt_assert(n < ARRAY_SIZE(spin));
 
 		spin[n] = __igt_spin_batch_new(fd, ctx[NOISE], other, 0);
@@ -496,7 +481,7 @@ static void preempt_self(int fd, unsigned ring)
 
 	n = 0;
 	gem_context_set_priority(fd, ctx[HI], MIN_PRIO);
-	for_each_engine(fd, other) {
+	for_each_physical_engine(fd, other) {
 		spin[n] = __igt_spin_batch_new(fd, ctx[NOISE], other, 0);
 		store_dword(fd, ctx[HI], other,
 			    result, (n + 1)*sizeof(uint32_t), n + 1,
diff --git a/tests/gem_exec_store.c b/tests/gem_exec_store.c
index 31a2c096..a7673489 100644
--- a/tests/gem_exec_store.c
+++ b/tests/gem_exec_store.c
@@ -220,7 +220,7 @@ static void store_all(int fd)
 
 	nengine = 0;
 	intel_detect_and_clear_missed_interrupts(fd);
-	for_each_engine(fd, engine) {
+	for_each_physical_engine(fd, engine) {
 		if (!gem_can_store_dword(fd, engine))
 			continue;
 
diff --git a/tests/gem_exec_suspend.c b/tests/gem_exec_suspend.c
index bbdc6e55..351347cb 100644
--- a/tests/gem_exec_suspend.c
+++ b/tests/gem_exec_suspend.c
@@ -62,25 +62,13 @@ static void check_bo(int fd, uint32_t handle)
 	munmap(map, 4096);
 }
 
-static bool ignore_engine(int fd, unsigned engine)
-{
-	if (engine == 0)
-		return true;
-
-	if (!gem_can_store_dword(fd, engine))
-		return true;
-
-	return false;
-}
-
 static void test_all(int fd, unsigned flags)
 {
 	unsigned engine;
 
-	for_each_engine(fd, engine) {
-		if (!ignore_engine(fd, engine))
+	for_each_physical_engine(fd, engine)
+		if (gem_can_store_dword(fd, engine))
 			run_test(fd, engine, flags & ~0xff);
-	}
 }
 
 static bool has_semaphores(int fd)
@@ -118,8 +106,8 @@ static void run_test(int fd, unsigned engine, unsigned flags)
 		 * GPU is then unlikely to be active!)
 		 */
 		if (has_semaphores(fd)) {
-			for_each_engine(fd, engine) {
-				if (!ignore_engine(fd, engine))
+			for_each_physical_engine(fd, engine) {
+				if (gem_can_store_dword(fd, engine))
 					engines[nengine++] = engine;
 			}
 		} else {
diff --git a/tests/gem_exec_whisper.c b/tests/gem_exec_whisper.c
index 5f9fedf5..1f4dad63 100644
--- a/tests/gem_exec_whisper.c
+++ b/tests/gem_exec_whisper.c
@@ -79,17 +79,6 @@ static void verify_reloc(int fd, uint32_t handle,
 	}
 }
 
-static bool ignore_engine(int fd, unsigned engine)
-{
-	if (engine == 0)
-		return true;
-
-	if (!gem_can_store_dword(fd, engine))
-		return true;
-
-	return false;
-}
-
 #define CONTEXTS 0x1
 #define FDS 0x2
 #define INTERRUPTIBLE 0x4
@@ -217,8 +206,8 @@ static void whisper(int fd, unsigned engine, unsigned flags)
 
 	nengine = 0;
 	if (engine == -1) {
-		for_each_engine(fd, engine) {
-			if (!ignore_engine(fd, engine))
+		for_each_physical_engine(fd, engine) {
+			if (gem_can_store_dword(fd, engine))
 				engines[nengine++] = engine;
 		}
 	} else {
diff --git a/tests/gem_ring_sync_loop.c b/tests/gem_ring_sync_loop.c
index ab5bedb3..118f3638 100644
--- a/tests/gem_ring_sync_loop.c
+++ b/tests/gem_ring_sync_loop.c
@@ -48,7 +48,7 @@ sync_loop(int fd)
 	int i;
 
 	nengine = 0;
-	for_each_engine(fd, engine)
+	for_each_physical_engine(fd, engine)
 		engines[nengine++] = engine;
 	igt_require(nengine);
 
diff --git a/tests/gem_spin_batch.c b/tests/gem_spin_batch.c
index 89631130..026f9830 100644
--- a/tests/gem_spin_batch.c
+++ b/tests/gem_spin_batch.c
@@ -76,10 +76,7 @@ static void spin_on_all_engines(int fd, unsigned int timeout_sec)
 {
 	unsigned engine;
 
-	for_each_engine(fd, engine) {
-		if (engine == 0)
-			continue;
-
+	for_each_physical_engine(fd, engine) {
 		igt_fork(child, 1) {
 			igt_install_exit_handler(spin_exit_handler);
 			spin(fd, engine, timeout_sec);
diff --git a/tests/gem_sync.c b/tests/gem_sync.c
index d70515ea..788fafc3 100644
--- a/tests/gem_sync.c
+++ b/tests/gem_sync.c
@@ -86,23 +86,9 @@ sync_ring(int fd, unsigned ring, int num_children, int timeout)
 	int num_engines = 0;
 
 	if (ring == ~0u) {
-		const struct intel_execution_engine *e;
-
-		for (e = intel_execution_engines; e->name; e++) {
-			if (e->exec_id == 0)
-				continue;
-
-			if (!gem_has_ring(fd, e->exec_id | e->flags))
-				continue;
-
-			if (e->exec_id == I915_EXEC_BSD) {
-				int is_bsd2 = e->flags != 0;
-				if (gem_has_bsd2(fd) != is_bsd2)
-					continue;
-			}
-
-			names[num_engines] = e->name;
-			engines[num_engines++] = e->exec_id | e->flags;
+		for_each_physical_engine(fd, ring) {
+			names[num_engines] = e__->name;
+			engines[num_engines++] = ring;
 			if (num_engines == ARRAY_SIZE(engines))
 				break;
 		}
@@ -200,26 +186,9 @@ store_ring(int fd, unsigned ring, int num_children, int timeout)
 	int num_engines = 0;
 
 	if (ring == ~0u) {
-		const struct intel_execution_engine *e;
-
-		for (e = intel_execution_engines; e->name; e++) {
-			if (e->exec_id == 0)
-				continue;
-
-			if (!gem_has_ring(fd, e->exec_id | e->flags))
-				continue;
-
-			if (!gem_can_store_dword(fd, e->exec_id | e->flags))
-				continue;
-
-			if (e->exec_id == I915_EXEC_BSD) {
-				int is_bsd2 = e->flags != 0;
-				if (gem_has_bsd2(fd) != is_bsd2)
-					continue;
-			}
-
-			names[num_engines] = e->name;
-			engines[num_engines++] = e->exec_id | e->flags;
+		for_each_physical_engine(fd, ring) {
+			names[num_engines] = e__->name;
+			engines[num_engines++] = ring;
 			if (num_engines == ARRAY_SIZE(engines))
 				break;
 		}
@@ -502,31 +471,14 @@ store_many(int fd, unsigned ring, int timeout)
 	intel_detect_and_clear_missed_interrupts(fd);
 
 	if (ring == ~0u) {
-		const struct intel_execution_engine *e;
-
-		for (e = intel_execution_engines; e->name; e++) {
-			if (e->exec_id == 0)
-				continue;
-
-			if (!gem_has_ring(fd, e->exec_id | e->flags))
-				continue;
-
-			if (!gem_can_store_dword(fd, e->exec_id | e->flags))
-				continue;
-
-			if (e->exec_id == I915_EXEC_BSD) {
-				int is_bsd2 = e->flags != 0;
-				if (gem_has_bsd2(fd) != is_bsd2)
-					continue;
-			}
-
+		for_each_physical_engine(fd, ring) {
 			igt_fork(child, 1)
 				__store_many(fd,
-					     e->exec_id | e->flags,
+					     ring,
 					     timeout,
 					     &shared[n]);
 
-			names[n++] = e->name;
+			names[n++] = e__->name;
 		}
 		igt_waitchildren();
 	} else {
@@ -737,23 +689,9 @@ preempt(int fd, unsigned ring, int num_children, int timeout)
 	uint32_t ctx[2];
 
 	if (ring == ~0u) {
-		const struct intel_execution_engine *e;
-
-		for (e = intel_execution_engines; e->name; e++) {
-			if (e->exec_id == 0)
-				continue;
-
-			if (!gem_has_ring(fd, e->exec_id | e->flags))
-				continue;
-
-			if (e->exec_id == I915_EXEC_BSD) {
-				int is_bsd2 = e->flags != 0;
-				if (gem_has_bsd2(fd) != is_bsd2)
-					continue;
-			}
-
-			names[num_engines] = e->name;
-			engines[num_engines++] = e->exec_id | e->flags;
+		for_each_physical_engine(fd, ring) {
+			names[num_engines] = e__->name;
+			engines[num_engines++] = ring;
 			if (num_engines == ARRAY_SIZE(engines))
 				break;
 		}
-- 
2.16.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[igt-dev] [PATCH igt 1/2] Iterate over physical engines

[Chris Wilson]

We current have a single for_each_engine() iterator which we use to
generate both a set of uABI engines and a set of physical engines.
Determining what uABI ring-id corresponds to an actual HW engine is
tricky, so pull that out to a library function and introduce
for_each_physical_engine() for cases where we want to issue requests
once on each HW ring (avoiding aliasing issues).

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 lib/igt_gt.c               | 23 +++++++++++++
 lib/igt_gt.h               |  9 +++++
 tests/amdgpu/amd_prime.c   |  6 +---
 tests/gem_concurrent_all.c |  2 +-
 tests/gem_ctx_create.c     |  5 +--
 tests/gem_exec_await.c     | 17 +--------
 tests/gem_exec_create.c    | 17 +--------
 tests/gem_exec_fence.c     | 16 ++-------
 tests/gem_exec_gttfill.c   | 16 +--------
 tests/gem_exec_nop.c       | 32 +++--------------
 tests/gem_exec_parallel.c  | 15 ++------
 tests/gem_exec_reloc.c     |  2 +-
 tests/gem_exec_schedule.c  | 21 ++---------
 tests/gem_exec_store.c     |  2 +-
 tests/gem_exec_suspend.c   | 20 +++--------
 tests/gem_exec_whisper.c   | 15 ++------
 tests/gem_ring_sync_loop.c |  2 +-
 tests/gem_spin_batch.c     |  5 +--
 tests/gem_sync.c           | 86 +++++++---------------------------------------
 19 files changed, 72 insertions(+), 239 deletions(-)

diff --git a/lib/igt_gt.c b/lib/igt_gt.c
index f70fcb92..e630550b 100644
--- a/lib/igt_gt.c
+++ b/lib/igt_gt.c
@@ -660,3 +660,26 @@ bool gem_has_engine(int gem_fd,
 			    gem_class_instance_to_eb_flags(gem_fd, class,
 							   instance));
 }
+
+bool gem_ring_is_physical_engine(int fd, unsigned ring)
+{
+	if (ring == I915_EXEC_DEFAULT)
+		return false;
+
+	/* BSD uses an extra flag to chose between aliasing modes */
+	if ((ring & 63) == I915_EXEC_BSD) {
+		bool explicit_bsd = ring & (3 << 13);
+		bool has_bsd2 = gem_has_bsd2(fd);
+		return explicit_bsd ? has_bsd2 : !has_bsd2;
+	}
+
+	return true;
+}
+
+bool gem_ring_has_physical_engine(int fd, unsigned ring)
+{
+	if (!gem_ring_is_physical_engine(fd, ring))
+		return false;
+
+	return gem_has_ring(fd, ring);
+}
diff --git a/lib/igt_gt.h b/lib/igt_gt.h
index 68592410..4d9d1aa0 100644
--- a/lib/igt_gt.h
+++ b/lib/igt_gt.h
@@ -81,6 +81,15 @@ extern const struct intel_execution_engine {
 	     e__++) \
 		for_if (gem_has_ring(fd__, flags__ = e__->exec_id | e__->flags))
 
+#define for_each_physical_engine(fd__, flags__) \
+	for (const struct intel_execution_engine *e__ = intel_execution_engines;\
+	     e__->name; \
+	     e__++) \
+		for_if (gem_ring_has_physical_engine(fd__, flags__ = e__->exec_id | e__->flags))
+
+bool gem_ring_is_physical_engine(int fd, unsigned int ring);
+bool gem_ring_has_physical_engine(int fd, unsigned int ring);
+
 bool gem_can_store_dword(int fd, unsigned int engine);
 
 extern const struct intel_execution_engine2 {
diff --git a/tests/amdgpu/amd_prime.c b/tests/amdgpu/amd_prime.c
index b2f326b4..bb68ccf3 100644
--- a/tests/amdgpu/amd_prime.c
+++ b/tests/amdgpu/amd_prime.c
@@ -179,12 +179,8 @@ static void i915_to_amd(int i915, int amd, amdgpu_device_handle device)
 	struct cork c;
 
 	nengine = 0;
-	for_each_engine(i915, engine) {
-		if (engine == 0)
-			continue;
-
+	for_each_physical_engine(i915, engine)
 		engines[nengine++] = engine;
-	}
 	igt_require(nengine);
 
 	memset(obj, 0, sizeof(obj));
diff --git a/tests/gem_concurrent_all.c b/tests/gem_concurrent_all.c
index 201b491b..3a1097ba 100644
--- a/tests/gem_concurrent_all.c
+++ b/tests/gem_concurrent_all.c
@@ -960,7 +960,7 @@ static igt_hang_t all_hang(void)
 	execbuf.buffers_ptr = to_user_pointer(&obj);
 	execbuf.buffer_count = 1;
 
-	for_each_engine(fd, engine) {
+	for_each_physical_engine(fd, engine) {
 		hang = igt_hang_ring(fd, engine);
 
 		execbuf.flags = engine;
diff --git a/tests/gem_ctx_create.c b/tests/gem_ctx_create.c
index 058445b6..1b32d6c3 100644
--- a/tests/gem_ctx_create.c
+++ b/tests/gem_ctx_create.c
@@ -321,11 +321,8 @@ igt_main
 		igt_require_gem(fd);
 		gem_require_contexts(fd);
 
-		for_each_engine(fd, engine) {
-			if (engine == 0)
-				continue;
+		for_each_physical_engine(fd, engine)
 			all_engines[all_nengine++] = engine;
-		}
 		igt_require(all_nengine);
 
 		if (gem_uses_full_ppgtt(fd)) {
diff --git a/tests/gem_exec_await.c b/tests/gem_exec_await.c
index e19363c4..fccc24d9 100644
--- a/tests/gem_exec_await.c
+++ b/tests/gem_exec_await.c
@@ -44,17 +44,6 @@ static double elapsed(const struct timespec *start, const struct timespec *end)
 		(end->tv_nsec - start->tv_nsec)*1e-9);
 }
 
-static bool ignore_engine(int fd, unsigned engine)
-{
-	if (engine == 0)
-		return true;
-
-	if (gem_has_bsd2(fd) && engine == I915_EXEC_BSD)
-		return true;
-
-	return false;
-}
-
 static void xchg_obj(void *array, unsigned i, unsigned j)
 {
 	struct drm_i915_gem_exec_object2 *obj = array;
@@ -89,12 +78,8 @@ static void wide(int fd, int ring_size, int timeout, unsigned int flags)
 	double time;
 
 	nengine = 0;
-	for_each_engine(fd, engine) {
-		if (ignore_engine(fd, engine))
-			continue;
-
+	for_each_physical_engine(fd, engine)
 		engines[nengine++] = engine;
-	}
 	igt_require(nengine);
 
 	exec = calloc(nengine, sizeof(*exec));
diff --git a/tests/gem_exec_create.c b/tests/gem_exec_create.c
index 28479a9d..54a2429e 100644
--- a/tests/gem_exec_create.c
+++ b/tests/gem_exec_create.c
@@ -54,17 +54,6 @@ static double elapsed(const struct timespec *start, const struct timespec *end)
 		(end->tv_nsec - start->tv_nsec)*1e-9);
 }
 
-static bool ignore_engine(int fd, unsigned engine)
-{
-	if (engine == 0)
-		return true;
-
-	if (gem_has_bsd2(fd) && engine == I915_EXEC_BSD)
-		return true;
-
-	return false;
-}
-
 #define LEAK 0x1
 
 static void all(int fd, unsigned flags, int timeout, int ncpus)
@@ -77,12 +66,8 @@ static void all(int fd, unsigned flags, int timeout, int ncpus)
 	unsigned engine;
 
 	nengine = 0;
-	for_each_engine(fd, engine) {
-		if (ignore_engine(fd, engine))
-			continue;
-
+	for_each_physical_engine(fd, engine)
 		engines[nengine++] = engine;
-	}
 	igt_require(nengine);
 
 	memset(&obj, 0, sizeof(obj));
diff --git a/tests/gem_exec_fence.c b/tests/gem_exec_fence.c
index 312505d6..39ca17ee 100644
--- a/tests/gem_exec_fence.c
+++ b/tests/gem_exec_fence.c
@@ -273,7 +273,7 @@ static void test_fence_await(int fd, unsigned ring, unsigned flags)
 	igt_assert(fence != -1);
 
 	i = 0;
-	for_each_engine(fd, engine) {
+	for_each_physical_engine(fd, engine) {
 		if (!gem_can_store_dword(fd, engine))
 			continue;
 
@@ -521,10 +521,7 @@ static void test_parallel(int fd, unsigned int master)
 	obj[BATCH].relocation_count = 1;
 
 	/* Queue all secondaries */
-	for_each_engine(fd, engine) {
-		if (engine == 0 || engine == I915_EXEC_BSD)
-			continue;
-
+	for_each_physical_engine(fd, engine) {
 		if (engine == master)
 			continue;
 
@@ -699,15 +696,8 @@ static void test_long_history(int fd, long ring_size, unsigned flags)
 		limit = ring_size / 3;
 
 	nengine = 0;
-	for_each_engine(fd, engine) {
-		if (engine == 0)
-			continue;
-
-		if (engine == I915_EXEC_BSD)
-			continue;
-
+	for_each_physical_engine(fd, engine)
 		engines[nengine++] = engine;
-	}
 	igt_require(nengine);
 
 	gem_quiescent_gpu(fd);
diff --git a/tests/gem_exec_gttfill.c b/tests/gem_exec_gttfill.c
index 96ed832f..4097e407 100644
--- a/tests/gem_exec_gttfill.c
+++ b/tests/gem_exec_gttfill.c
@@ -28,17 +28,6 @@ IGT_TEST_DESCRIPTION("Fill the GTT with batches.");
 
 #define BATCH_SIZE (4096<<10)
 
-static bool ignore_engine(int fd, unsigned engine)
-{
-	if (engine == 0)
-		return true;
-
-	if (gem_has_bsd2(fd) && engine == I915_EXEC_BSD)
-		return true;
-
-	return false;
-}
-
 static void xchg_u32(void *array, unsigned i, unsigned j)
 {
 	uint32_t *u32 = array;
@@ -126,10 +115,7 @@ static void fillgtt(int fd, unsigned ring, int timeout)
 
 	nengine = 0;
 	if (ring == 0) {
-		for_each_engine(fd, engine) {
-			if (ignore_engine(fd, engine))
-				continue;
-
+		for_each_physical_engine(fd, engine) {
 			if (!gem_can_store_dword(fd, engine))
 				continue;
 
diff --git a/tests/gem_exec_nop.c b/tests/gem_exec_nop.c
index d3e9a3e0..d971ffcb 100644
--- a/tests/gem_exec_nop.c
+++ b/tests/gem_exec_nop.c
@@ -188,17 +188,6 @@ static void headless(int fd, uint32_t handle)
 	assert_within_epsilon(n_headless, n_display, 0.1f);
 }
 
-static bool ignore_engine(int fd, unsigned engine)
-{
-	if (engine == 0)
-		return true;
-
-	if (gem_has_bsd2(fd) && engine == I915_EXEC_BSD)
-		return true;
-
-	return false;
-}
-
 static void parallel(int fd, uint32_t handle, int timeout)
 {
 	struct drm_i915_gem_execbuffer2 execbuf;
@@ -212,10 +201,7 @@ static void parallel(int fd, uint32_t handle, int timeout)
 
 	sum = 0;
 	nengine = 0;
-	for_each_engine(fd, engine) {
-		if (ignore_engine(fd, engine))
-			continue;
-
+	for_each_physical_engine(fd, engine) {
 		engines[nengine] = engine;
 		names[nengine] = e__->name;
 		nengine++;
@@ -277,10 +263,7 @@ static void series(int fd, uint32_t handle, int timeout)
 	const char *name;
 
 	nengine = 0;
-	for_each_engine(fd, engine) {
-		if (ignore_engine(fd, engine))
-			continue;
-
+	for_each_physical_engine(fd, engine) {
 		time = nop_on_ring(fd, handle, engine, 1, &count) / count;
 		if (time > max) {
 			name = e__->name;
@@ -375,12 +358,9 @@ static void sequential(int fd, uint32_t handle, unsigned flags, int timeout)
 
 	nengine = 0;
 	sum = 0;
-	for_each_engine(fd, n) {
+	for_each_physical_engine(fd, n) {
 		unsigned long count;
 
-		if (ignore_engine(fd, n))
-			continue;
-
 		time = nop_on_ring(fd, handle, n, 1, &count) / count;
 		sum += time;
 		igt_debug("%s: %.3fus\n", e__->name, 1e6*time);
@@ -509,12 +489,8 @@ static void fence_signal(int fd, uint32_t handle,
 
 	nengine = 0;
 	if (ring_id == -1) {
-		for_each_engine(fd, n) {
-			if (ignore_engine(fd, n))
-				continue;
-
+		for_each_physical_engine(fd, n)
 			engines[nengine++] = n;
-		}
 	} else {
 		gem_require_ring(fd, ring_id);
 		engines[nengine++] = ring_id;
diff --git a/tests/gem_exec_parallel.c b/tests/gem_exec_parallel.c
index 11b6e775..fe5ffe8f 100644
--- a/tests/gem_exec_parallel.c
+++ b/tests/gem_exec_parallel.c
@@ -55,17 +55,6 @@ static void check_bo(int fd, uint32_t handle, int pass)
 	munmap(map, 4096);
 }
 
-static bool ignore_engine(int fd, unsigned engine)
-{
-	if (engine == 0)
-		return true;
-
-	if (!gem_can_store_dword(fd, engine))
-		return true;
-
-	return false;
-}
-
 #define CONTEXTS 0x1
 #define FDS 0x2
 
@@ -180,8 +169,8 @@ static void all(int fd, unsigned engine, unsigned flags)
 
 	nengine = 0;
 	if (engine == -1) {
-		for_each_engine(fd, engine) {
-			if (!ignore_engine(fd, engine))
+		for_each_physical_engine(fd, engine) {
+			if (gem_can_store_dword(fd, engine))
 				engines[nengine++] = engine;
 		}
 	} else {
diff --git a/tests/gem_exec_reloc.c b/tests/gem_exec_reloc.c
index 432a42a9..213de1d7 100644
--- a/tests/gem_exec_reloc.c
+++ b/tests/gem_exec_reloc.c
@@ -258,7 +258,7 @@ static void active(int fd, unsigned engine)
 
 	nengine = 0;
 	if (engine == -1) {
-		for_each_engine(fd, engine) {
+		for_each_physical_engine(fd, engine) {
 			if (gem_can_store_dword(fd, engine))
 				engines[nengine++] = engine;
 		}
diff --git a/tests/gem_exec_schedule.c b/tests/gem_exec_schedule.c
index 5f24df33..03d457e9 100644
--- a/tests/gem_exec_schedule.c
+++ b/tests/gem_exec_schedule.c
@@ -188,17 +188,6 @@ static void fifo(int fd, unsigned ring)
 	munmap(ptr, 4096);
 }
 
-static bool ignore_engine(int fd, unsigned engine)
-{
-	if (engine == 0)
-		return true;
-
-	if (gem_has_bsd2(fd) && engine == I915_EXEC_BSD)
-		return true;
-
-	return false;
-}
-
 static void smoketest(int fd, unsigned ring, unsigned timeout)
 {
 	const int ncpus = sysconf(_SC_NPROCESSORS_ONLN);
@@ -210,12 +199,8 @@ static void smoketest(int fd, unsigned ring, unsigned timeout)
 
 	nengine = 0;
 	if (ring == -1) {
-		for_each_engine(fd, engine) {
-			if (ignore_engine(fd, engine))
-				continue;
-
+		for_each_physical_engine(fd, engine)
 			engines[nengine++] = engine;
-		}
 	} else {
 		engines[nengine++] = ring;
 	}
@@ -440,7 +425,7 @@ static void preempt_other(int fd, unsigned ring)
 	gem_context_set_priority(fd, ctx[HI], MAX_PRIO);
 
 	n = 0;
-	for_each_engine(fd, other) {
+	for_each_physical_engine(fd, other) {
 		igt_assert(n < ARRAY_SIZE(spin));
 
 		spin[n] = __igt_spin_batch_new(fd, ctx[NOISE], other, 0);
@@ -496,7 +481,7 @@ static void preempt_self(int fd, unsigned ring)
 
 	n = 0;
 	gem_context_set_priority(fd, ctx[HI], MIN_PRIO);
-	for_each_engine(fd, other) {
+	for_each_physical_engine(fd, other) {
 		spin[n] = __igt_spin_batch_new(fd, ctx[NOISE], other, 0);
 		store_dword(fd, ctx[HI], other,
 			    result, (n + 1)*sizeof(uint32_t), n + 1,
diff --git a/tests/gem_exec_store.c b/tests/gem_exec_store.c
index 31a2c096..a7673489 100644
--- a/tests/gem_exec_store.c
+++ b/tests/gem_exec_store.c
@@ -220,7 +220,7 @@ static void store_all(int fd)
 
 	nengine = 0;
 	intel_detect_and_clear_missed_interrupts(fd);
-	for_each_engine(fd, engine) {
+	for_each_physical_engine(fd, engine) {
 		if (!gem_can_store_dword(fd, engine))
 			continue;
 
diff --git a/tests/gem_exec_suspend.c b/tests/gem_exec_suspend.c
index bbdc6e55..351347cb 100644
--- a/tests/gem_exec_suspend.c
+++ b/tests/gem_exec_suspend.c
@@ -62,25 +62,13 @@ static void check_bo(int fd, uint32_t handle)
 	munmap(map, 4096);
 }
 
-static bool ignore_engine(int fd, unsigned engine)
-{
-	if (engine == 0)
-		return true;
-
-	if (!gem_can_store_dword(fd, engine))
-		return true;
-
-	return false;
-}
-
 static void test_all(int fd, unsigned flags)
 {
 	unsigned engine;
 
-	for_each_engine(fd, engine) {
-		if (!ignore_engine(fd, engine))
+	for_each_physical_engine(fd, engine)
+		if (gem_can_store_dword(fd, engine))
 			run_test(fd, engine, flags & ~0xff);
-	}
 }
 
 static bool has_semaphores(int fd)
@@ -118,8 +106,8 @@ static void run_test(int fd, unsigned engine, unsigned flags)
 		 * GPU is then unlikely to be active!)
 		 */
 		if (has_semaphores(fd)) {
-			for_each_engine(fd, engine) {
-				if (!ignore_engine(fd, engine))
+			for_each_physical_engine(fd, engine) {
+				if (gem_can_store_dword(fd, engine))
 					engines[nengine++] = engine;
 			}
 		} else {
diff --git a/tests/gem_exec_whisper.c b/tests/gem_exec_whisper.c
index 5f9fedf5..1f4dad63 100644
--- a/tests/gem_exec_whisper.c
+++ b/tests/gem_exec_whisper.c
@@ -79,17 +79,6 @@ static void verify_reloc(int fd, uint32_t handle,
 	}
 }
 
-static bool ignore_engine(int fd, unsigned engine)
-{
-	if (engine == 0)
-		return true;
-
-	if (!gem_can_store_dword(fd, engine))
-		return true;
-
-	return false;
-}
-
 #define CONTEXTS 0x1
 #define FDS 0x2
 #define INTERRUPTIBLE 0x4
@@ -217,8 +206,8 @@ static void whisper(int fd, unsigned engine, unsigned flags)
 
 	nengine = 0;
 	if (engine == -1) {
-		for_each_engine(fd, engine) {
-			if (!ignore_engine(fd, engine))
+		for_each_physical_engine(fd, engine) {
+			if (gem_can_store_dword(fd, engine))
 				engines[nengine++] = engine;
 		}
 	} else {
diff --git a/tests/gem_ring_sync_loop.c b/tests/gem_ring_sync_loop.c
index ab5bedb3..118f3638 100644
--- a/tests/gem_ring_sync_loop.c
+++ b/tests/gem_ring_sync_loop.c
@@ -48,7 +48,7 @@ sync_loop(int fd)
 	int i;
 
 	nengine = 0;
-	for_each_engine(fd, engine)
+	for_each_physical_engine(fd, engine)
 		engines[nengine++] = engine;
 	igt_require(nengine);
 
diff --git a/tests/gem_spin_batch.c b/tests/gem_spin_batch.c
index 89631130..026f9830 100644
--- a/tests/gem_spin_batch.c
+++ b/tests/gem_spin_batch.c
@@ -76,10 +76,7 @@ static void spin_on_all_engines(int fd, unsigned int timeout_sec)
 {
 	unsigned engine;
 
-	for_each_engine(fd, engine) {
-		if (engine == 0)
-			continue;
-
+	for_each_physical_engine(fd, engine) {
 		igt_fork(child, 1) {
 			igt_install_exit_handler(spin_exit_handler);
 			spin(fd, engine, timeout_sec);
diff --git a/tests/gem_sync.c b/tests/gem_sync.c
index d70515ea..788fafc3 100644
--- a/tests/gem_sync.c
+++ b/tests/gem_sync.c
@@ -86,23 +86,9 @@ sync_ring(int fd, unsigned ring, int num_children, int timeout)
 	int num_engines = 0;
 
 	if (ring == ~0u) {
-		const struct intel_execution_engine *e;
-
-		for (e = intel_execution_engines; e->name; e++) {
-			if (e->exec_id == 0)
-				continue;
-
-			if (!gem_has_ring(fd, e->exec_id | e->flags))
-				continue;
-
-			if (e->exec_id == I915_EXEC_BSD) {
-				int is_bsd2 = e->flags != 0;
-				if (gem_has_bsd2(fd) != is_bsd2)
-					continue;
-			}
-
-			names[num_engines] = e->name;
-			engines[num_engines++] = e->exec_id | e->flags;
+		for_each_physical_engine(fd, ring) {
+			names[num_engines] = e__->name;
+			engines[num_engines++] = ring;
 			if (num_engines == ARRAY_SIZE(engines))
 				break;
 		}
@@ -200,26 +186,9 @@ store_ring(int fd, unsigned ring, int num_children, int timeout)
 	int num_engines = 0;
 
 	if (ring == ~0u) {
-		const struct intel_execution_engine *e;
-
-		for (e = intel_execution_engines; e->name; e++) {
-			if (e->exec_id == 0)
-				continue;
-
-			if (!gem_has_ring(fd, e->exec_id | e->flags))
-				continue;
-
-			if (!gem_can_store_dword(fd, e->exec_id | e->flags))
-				continue;
-
-			if (e->exec_id == I915_EXEC_BSD) {
-				int is_bsd2 = e->flags != 0;
-				if (gem_has_bsd2(fd) != is_bsd2)
-					continue;
-			}
-
-			names[num_engines] = e->name;
-			engines[num_engines++] = e->exec_id | e->flags;
+		for_each_physical_engine(fd, ring) {
+			names[num_engines] = e__->name;
+			engines[num_engines++] = ring;
 			if (num_engines == ARRAY_SIZE(engines))
 				break;
 		}
@@ -502,31 +471,14 @@ store_many(int fd, unsigned ring, int timeout)
 	intel_detect_and_clear_missed_interrupts(fd);
 
 	if (ring == ~0u) {
-		const struct intel_execution_engine *e;
-
-		for (e = intel_execution_engines; e->name; e++) {
-			if (e->exec_id == 0)
-				continue;
-
-			if (!gem_has_ring(fd, e->exec_id | e->flags))
-				continue;
-
-			if (!gem_can_store_dword(fd, e->exec_id | e->flags))
-				continue;
-
-			if (e->exec_id == I915_EXEC_BSD) {
-				int is_bsd2 = e->flags != 0;
-				if (gem_has_bsd2(fd) != is_bsd2)
-					continue;
-			}
-
+		for_each_physical_engine(fd, ring) {
 			igt_fork(child, 1)
 				__store_many(fd,
-					     e->exec_id | e->flags,
+					     ring,
 					     timeout,
 					     &shared[n]);
 
-			names[n++] = e->name;
+			names[n++] = e__->name;
 		}
 		igt_waitchildren();
 	} else {
@@ -737,23 +689,9 @@ preempt(int fd, unsigned ring, int num_children, int timeout)
 	uint32_t ctx[2];
 
 	if (ring == ~0u) {
-		const struct intel_execution_engine *e;
-
-		for (e = intel_execution_engines; e->name; e++) {
-			if (e->exec_id == 0)
-				continue;
-
-			if (!gem_has_ring(fd, e->exec_id | e->flags))
-				continue;
-
-			if (e->exec_id == I915_EXEC_BSD) {
-				int is_bsd2 = e->flags != 0;
-				if (gem_has_bsd2(fd) != is_bsd2)
-					continue;
-			}
-
-			names[num_engines] = e->name;
-			engines[num_engines++] = e->exec_id | e->flags;
+		for_each_physical_engine(fd, ring) {
+			names[num_engines] = e__->name;
+			engines[num_engines++] = ring;
 			if (num_engines == ARRAY_SIZE(engines))
 				break;
 		}
-- 
2.16.1

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

[PATCH igt 2/2] igt/gem_spin_batch: Avoid waiting when running concurrently

[Chris Wilson]

If we do a global wait while trying to execute spinners in parallel,
it ends badly with a GPU hang.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104352
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 tests/gem_spin_batch.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/tests/gem_spin_batch.c b/tests/gem_spin_batch.c
index 026f9830..cffeb6d7 100644
--- a/tests/gem_spin_batch.c
+++ b/tests/gem_spin_batch.c
@@ -41,7 +41,7 @@ static void spin(int fd, unsigned int engine, unsigned int timeout_sec)
 	struct timespec itv = { };
 	uint64_t elapsed;
 
-	spin = igt_spin_batch_new(fd, 0, engine, 0);
+	spin = __igt_spin_batch_new(fd, 0, engine, 0);
 	while ((elapsed = igt_nsec_elapsed(&tv)) >> 30 < timeout_sec) {
 		igt_spin_t *next = __igt_spin_batch_new(fd, 0, engine, 0);
 
@@ -64,7 +64,6 @@ static void spin(int fd, unsigned int engine, unsigned int timeout_sec)
 		 loops, (long long)elapsed, (long)(elapsed / timeout_100ms));
 
 	assert_within_epsilon(timeout_100ms * loops, elapsed, MAX_ERROR);
-	igt_assert_eq(intel_detect_and_clear_missed_interrupts(fd), 0);
 }
 
 static void spin_exit_handler(int sig)
@@ -101,15 +100,18 @@ igt_main
 	}
 
 	for (e = intel_execution_engines; e->name; e++) {
-		if (e->exec_id == 0)
-			continue;
-
-		igt_subtest_f("basic-%s", e->name)
+		igt_subtest_f("basic-%s", e->name) {
+			intel_detect_and_clear_missed_interrupts(fd);
 			spin(fd, e->exec_id, 3);
+			igt_assert_eq(intel_detect_and_clear_missed_interrupts(fd), 0);
+		}
 	}
 
-	igt_subtest("spin-each")
+	igt_subtest("spin-each") {
+		intel_detect_and_clear_missed_interrupts(fd);
 		spin_on_all_engines(fd, 3);
+		igt_assert_eq(intel_detect_and_clear_missed_interrupts(fd), 0);
+	}
 
 	igt_fixture {
 		igt_stop_hang_detector();
-- 
2.16.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[igt-dev] [PATCH igt 2/2] igt/gem_spin_batch: Avoid waiting when running concurrently

[Chris Wilson]

If we do a global wait while trying to execute spinners in parallel,
it ends badly with a GPU hang.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104352
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 tests/gem_spin_batch.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/tests/gem_spin_batch.c b/tests/gem_spin_batch.c
index 026f9830..cffeb6d7 100644
--- a/tests/gem_spin_batch.c
+++ b/tests/gem_spin_batch.c
@@ -41,7 +41,7 @@ static void spin(int fd, unsigned int engine, unsigned int timeout_sec)
 	struct timespec itv = { };
 	uint64_t elapsed;
 
-	spin = igt_spin_batch_new(fd, 0, engine, 0);
+	spin = __igt_spin_batch_new(fd, 0, engine, 0);
 	while ((elapsed = igt_nsec_elapsed(&tv)) >> 30 < timeout_sec) {
 		igt_spin_t *next = __igt_spin_batch_new(fd, 0, engine, 0);
 
@@ -64,7 +64,6 @@ static void spin(int fd, unsigned int engine, unsigned int timeout_sec)
 		 loops, (long long)elapsed, (long)(elapsed / timeout_100ms));
 
 	assert_within_epsilon(timeout_100ms * loops, elapsed, MAX_ERROR);
-	igt_assert_eq(intel_detect_and_clear_missed_interrupts(fd), 0);
 }
 
 static void spin_exit_handler(int sig)
@@ -101,15 +100,18 @@ igt_main
 	}
 
 	for (e = intel_execution_engines; e->name; e++) {
-		if (e->exec_id == 0)
-			continue;
-
-		igt_subtest_f("basic-%s", e->name)
+		igt_subtest_f("basic-%s", e->name) {
+			intel_detect_and_clear_missed_interrupts(fd);
 			spin(fd, e->exec_id, 3);
+			igt_assert_eq(intel_detect_and_clear_missed_interrupts(fd), 0);
+		}
 	}
 
-	igt_subtest("spin-each")
+	igt_subtest("spin-each") {
+		intel_detect_and_clear_missed_interrupts(fd);
 		spin_on_all_engines(fd, 3);
+		igt_assert_eq(intel_detect_and_clear_missed_interrupts(fd), 0);
+	}
 
 	igt_fixture {
 		igt_stop_hang_detector();
-- 
2.16.1

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

Re: [PATCH igt 1/2] Iterate over physical engines

[Tvrtko Ursulin]

On 21/02/2018 14:45, Chris Wilson wrote:
> We current have a single for_each_engine() iterator which we use to
> generate both a set of uABI engines and a set of physical engines.
> Determining what uABI ring-id corresponds to an actual HW engine is
> tricky, so pull that out to a library function and introduce
> for_each_physical_engine() for cases where we want to issue requests
> once on each HW ring (avoiding aliasing issues).

As you know I tried to make for_each_engine actually behave like this 
(iterate physical engines). I still think it would be better / more 
intuitive, and leave the uABI for a specially named iterator instead. 
However, I am willing to accept this compromise in order to start moving 
towards the long overdue cleanup in this respect.

> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   lib/igt_gt.c               | 23 +++++++++++++
>   lib/igt_gt.h               |  9 +++++
>   tests/amdgpu/amd_prime.c   |  6 +---
>   tests/gem_concurrent_all.c |  2 +-
>   tests/gem_ctx_create.c     |  5 +--
>   tests/gem_exec_await.c     | 17 +--------
>   tests/gem_exec_create.c    | 17 +--------
>   tests/gem_exec_fence.c     | 16 ++-------
>   tests/gem_exec_gttfill.c   | 16 +--------
>   tests/gem_exec_nop.c       | 32 +++--------------
>   tests/gem_exec_parallel.c  | 15 ++------
>   tests/gem_exec_reloc.c     |  2 +-
>   tests/gem_exec_schedule.c  | 21 ++---------
>   tests/gem_exec_store.c     |  2 +-
>   tests/gem_exec_suspend.c   | 20 +++--------
>   tests/gem_exec_whisper.c   | 15 ++------
>   tests/gem_ring_sync_loop.c |  2 +-
>   tests/gem_spin_batch.c     |  5 +--
>   tests/gem_sync.c           | 86 +++++++---------------------------------------
>   19 files changed, 72 insertions(+), 239 deletions(-)
> 
> diff --git a/lib/igt_gt.c b/lib/igt_gt.c
> index f70fcb92..e630550b 100644
> --- a/lib/igt_gt.c
> +++ b/lib/igt_gt.c
> @@ -660,3 +660,26 @@ bool gem_has_engine(int gem_fd,
>   			    gem_class_instance_to_eb_flags(gem_fd, class,
>   							   instance));
>   }
> +
> +bool gem_ring_is_physical_engine(int fd, unsigned ring)
> +{
> +	if (ring == I915_EXEC_DEFAULT)
> +		return false;
> +
> +	/* BSD uses an extra flag to chose between aliasing modes */
> +	if ((ring & 63) == I915_EXEC_BSD) {

I can see you have a special fondness towards 63 :), but at least hex 
mask would look less out of place.

I do wonder how we ended up with I915_EXEC_RING_MASK so out of sync. Why 
shouldn't we just bump it in uapi headers?

> +		bool explicit_bsd = ring & (3 << 13);
> +		bool has_bsd2 = gem_has_bsd2(fd);
> +		return explicit_bsd ? has_bsd2 : !has_bsd2;
> +	}
> +
> +	return true;
> +}
> +
> +bool gem_ring_has_physical_engine(int fd, unsigned ring)
> +{
> +	if (!gem_ring_is_physical_engine(fd, ring))
> +		return false;
> +
> +	return gem_has_ring(fd, ring);
> +}
> diff --git a/lib/igt_gt.h b/lib/igt_gt.h
> index 68592410..4d9d1aa0 100644
> --- a/lib/igt_gt.h
> +++ b/lib/igt_gt.h
> @@ -81,6 +81,15 @@ extern const struct intel_execution_engine {
>   	     e__++) \
>   		for_if (gem_has_ring(fd__, flags__ = e__->exec_id | e__->flags))
>   
> +#define for_each_physical_engine(fd__, flags__) \
> +	for (const struct intel_execution_engine *e__ = intel_execution_engines;\
> +	     e__->name; \
> +	     e__++) \
> +		for_if (gem_ring_has_physical_engine(fd__, flags__ = e__->exec_id | e__->flags))
> +
> +bool gem_ring_is_physical_engine(int fd, unsigned int ring);
> +bool gem_ring_has_physical_engine(int fd, unsigned int ring);

gcc shouldn't ever compiled on unsigned vs unsigned int in declaration 
vs definition right?

> +
>   bool gem_can_store_dword(int fd, unsigned int engine);
>   
>   extern const struct intel_execution_engine2 {
> diff --git a/tests/amdgpu/amd_prime.c b/tests/amdgpu/amd_prime.c
> index b2f326b4..bb68ccf3 100644
> --- a/tests/amdgpu/amd_prime.c
> +++ b/tests/amdgpu/amd_prime.c
> @@ -179,12 +179,8 @@ static void i915_to_amd(int i915, int amd, amdgpu_device_handle device)
>   	struct cork c;
>   
>   	nengine = 0;
> -	for_each_engine(i915, engine) {
> -		if (engine == 0)
> -			continue;
> -
> +	for_each_physical_engine(i915, engine)
>   		engines[nengine++] = engine;
> -	}
>   	igt_require(nengine);
>   
>   	memset(obj, 0, sizeof(obj));
> diff --git a/tests/gem_concurrent_all.c b/tests/gem_concurrent_all.c
> index 201b491b..3a1097ba 100644
> --- a/tests/gem_concurrent_all.c
> +++ b/tests/gem_concurrent_all.c
> @@ -960,7 +960,7 @@ static igt_hang_t all_hang(void)
>   	execbuf.buffers_ptr = to_user_pointer(&obj);
>   	execbuf.buffer_count = 1;
>   
> -	for_each_engine(fd, engine) {
> +	for_each_physical_engine(fd, engine) {
>   		hang = igt_hang_ring(fd, engine);
>   
>   		execbuf.flags = engine;
> diff --git a/tests/gem_ctx_create.c b/tests/gem_ctx_create.c
> index 058445b6..1b32d6c3 100644
> --- a/tests/gem_ctx_create.c
> +++ b/tests/gem_ctx_create.c
> @@ -321,11 +321,8 @@ igt_main
>   		igt_require_gem(fd);
>   		gem_require_contexts(fd);
>   
> -		for_each_engine(fd, engine) {
> -			if (engine == 0)
> -				continue;
> +		for_each_physical_engine(fd, engine)
>   			all_engines[all_nengine++] = engine;
> -		}
>   		igt_require(all_nengine);
>   
>   		if (gem_uses_full_ppgtt(fd)) {
> diff --git a/tests/gem_exec_await.c b/tests/gem_exec_await.c
> index e19363c4..fccc24d9 100644
> --- a/tests/gem_exec_await.c
> +++ b/tests/gem_exec_await.c
> @@ -44,17 +44,6 @@ static double elapsed(const struct timespec *start, const struct timespec *end)
>   		(end->tv_nsec - start->tv_nsec)*1e-9);
>   }
>   
> -static bool ignore_engine(int fd, unsigned engine)
> -{
> -	if (engine == 0)
> -		return true;
> -
> -	if (gem_has_bsd2(fd) && engine == I915_EXEC_BSD)
> -		return true;
> -
> -	return false;
> -}
> -
>   static void xchg_obj(void *array, unsigned i, unsigned j)
>   {
>   	struct drm_i915_gem_exec_object2 *obj = array;
> @@ -89,12 +78,8 @@ static void wide(int fd, int ring_size, int timeout, unsigned int flags)
>   	double time;
>   
>   	nengine = 0;
> -	for_each_engine(fd, engine) {
> -		if (ignore_engine(fd, engine))
> -			continue;
> -
> +	for_each_physical_engine(fd, engine)
>   		engines[nengine++] = engine;
> -	}
>   	igt_require(nengine);
>   
>   	exec = calloc(nengine, sizeof(*exec));
> diff --git a/tests/gem_exec_create.c b/tests/gem_exec_create.c
> index 28479a9d..54a2429e 100644
> --- a/tests/gem_exec_create.c
> +++ b/tests/gem_exec_create.c
> @@ -54,17 +54,6 @@ static double elapsed(const struct timespec *start, const struct timespec *end)
>   		(end->tv_nsec - start->tv_nsec)*1e-9);
>   }
>   
> -static bool ignore_engine(int fd, unsigned engine)
> -{
> -	if (engine == 0)
> -		return true;
> -
> -	if (gem_has_bsd2(fd) && engine == I915_EXEC_BSD)
> -		return true;
> -
> -	return false;
> -}
> -
>   #define LEAK 0x1
>   
>   static void all(int fd, unsigned flags, int timeout, int ncpus)
> @@ -77,12 +66,8 @@ static void all(int fd, unsigned flags, int timeout, int ncpus)
>   	unsigned engine;
>   
>   	nengine = 0;
> -	for_each_engine(fd, engine) {
> -		if (ignore_engine(fd, engine))
> -			continue;
> -
> +	for_each_physical_engine(fd, engine)
>   		engines[nengine++] = engine;
> -	}
>   	igt_require(nengine);
>   
>   	memset(&obj, 0, sizeof(obj));
> diff --git a/tests/gem_exec_fence.c b/tests/gem_exec_fence.c
> index 312505d6..39ca17ee 100644
> --- a/tests/gem_exec_fence.c
> +++ b/tests/gem_exec_fence.c
> @@ -273,7 +273,7 @@ static void test_fence_await(int fd, unsigned ring, unsigned flags)
>   	igt_assert(fence != -1);
>   
>   	i = 0;
> -	for_each_engine(fd, engine) {
> +	for_each_physical_engine(fd, engine) {
>   		if (!gem_can_store_dword(fd, engine))
>   			continue;
>   
> @@ -521,10 +521,7 @@ static void test_parallel(int fd, unsigned int master)
>   	obj[BATCH].relocation_count = 1;
>   
>   	/* Queue all secondaries */
> -	for_each_engine(fd, engine) {
> -		if (engine == 0 || engine == I915_EXEC_BSD)
> -			continue;

Behaviour change - BSD(s) not skipped any more, relevant?.

> -
> +	for_each_physical_engine(fd, engine) {
>   		if (engine == master)
>   			continue;
>   
> @@ -699,15 +696,8 @@ static void test_long_history(int fd, long ring_size, unsigned flags)
>   		limit = ring_size / 3;
>   
>   	nengine = 0;
> -	for_each_engine(fd, engine) {
> -		if (engine == 0)
> -			continue;
> -
> -		if (engine == I915_EXEC_BSD)
> -			continue;

Same.

> -
> +	for_each_physical_engine(fd, engine)
>   		engines[nengine++] = engine;
> -	}
>   	igt_require(nengine);
>   
>   	gem_quiescent_gpu(fd);
> diff --git a/tests/gem_exec_gttfill.c b/tests/gem_exec_gttfill.c
> index 96ed832f..4097e407 100644
> --- a/tests/gem_exec_gttfill.c
> +++ b/tests/gem_exec_gttfill.c
> @@ -28,17 +28,6 @@ IGT_TEST_DESCRIPTION("Fill the GTT with batches.");
>   
>   #define BATCH_SIZE (4096<<10)
>   
> -static bool ignore_engine(int fd, unsigned engine)
> -{
> -	if (engine == 0)
> -		return true;
> -
> -	if (gem_has_bsd2(fd) && engine == I915_EXEC_BSD)
> -		return true;
> -
> -	return false;
> -}
> -
>   static void xchg_u32(void *array, unsigned i, unsigned j)
>   {
>   	uint32_t *u32 = array;
> @@ -126,10 +115,7 @@ static void fillgtt(int fd, unsigned ring, int timeout)
>   
>   	nengine = 0;
>   	if (ring == 0) {
> -		for_each_engine(fd, engine) {
> -			if (ignore_engine(fd, engine))
> -				continue;
> -
> +		for_each_physical_engine(fd, engine) {
>   			if (!gem_can_store_dword(fd, engine))
>   				continue;
>   
> diff --git a/tests/gem_exec_nop.c b/tests/gem_exec_nop.c
> index d3e9a3e0..d971ffcb 100644
> --- a/tests/gem_exec_nop.c
> +++ b/tests/gem_exec_nop.c
> @@ -188,17 +188,6 @@ static void headless(int fd, uint32_t handle)
>   	assert_within_epsilon(n_headless, n_display, 0.1f);
>   }
>   
> -static bool ignore_engine(int fd, unsigned engine)
> -{
> -	if (engine == 0)
> -		return true;
> -
> -	if (gem_has_bsd2(fd) && engine == I915_EXEC_BSD)
> -		return true;
> -
> -	return false;
> -}
> -
>   static void parallel(int fd, uint32_t handle, int timeout)
>   {
>   	struct drm_i915_gem_execbuffer2 execbuf;
> @@ -212,10 +201,7 @@ static void parallel(int fd, uint32_t handle, int timeout)
>   
>   	sum = 0;
>   	nengine = 0;
> -	for_each_engine(fd, engine) {
> -		if (ignore_engine(fd, engine))
> -			continue;
> -
> +	for_each_physical_engine(fd, engine) {
>   		engines[nengine] = engine;
>   		names[nengine] = e__->name;
>   		nengine++;
> @@ -277,10 +263,7 @@ static void series(int fd, uint32_t handle, int timeout)
>   	const char *name;
>   
>   	nengine = 0;
> -	for_each_engine(fd, engine) {
> -		if (ignore_engine(fd, engine))
> -			continue;
> -
> +	for_each_physical_engine(fd, engine) {
>   		time = nop_on_ring(fd, handle, engine, 1, &count) / count;
>   		if (time > max) {
>   			name = e__->name;
> @@ -375,12 +358,9 @@ static void sequential(int fd, uint32_t handle, unsigned flags, int timeout)
>   
>   	nengine = 0;
>   	sum = 0;
> -	for_each_engine(fd, n) {
> +	for_each_physical_engine(fd, n) {
>   		unsigned long count;
>   
> -		if (ignore_engine(fd, n))
> -			continue;
> -
>   		time = nop_on_ring(fd, handle, n, 1, &count) / count;
>   		sum += time;
>   		igt_debug("%s: %.3fus\n", e__->name, 1e6*time);
> @@ -509,12 +489,8 @@ static void fence_signal(int fd, uint32_t handle,
>   
>   	nengine = 0;
>   	if (ring_id == -1) {
> -		for_each_engine(fd, n) {
> -			if (ignore_engine(fd, n))
> -				continue;
> -
> +		for_each_physical_engine(fd, n)
>   			engines[nengine++] = n;
> -		}
>   	} else {
>   		gem_require_ring(fd, ring_id);
>   		engines[nengine++] = ring_id;
> diff --git a/tests/gem_exec_parallel.c b/tests/gem_exec_parallel.c
> index 11b6e775..fe5ffe8f 100644
> --- a/tests/gem_exec_parallel.c
> +++ b/tests/gem_exec_parallel.c
> @@ -55,17 +55,6 @@ static void check_bo(int fd, uint32_t handle, int pass)
>   	munmap(map, 4096);
>   }
>   
> -static bool ignore_engine(int fd, unsigned engine)
> -{
> -	if (engine == 0)
> -		return true;
> -
> -	if (!gem_can_store_dword(fd, engine))
> -		return true;
> -
> -	return false;
> -}
> -
>   #define CONTEXTS 0x1
>   #define FDS 0x2
>   
> @@ -180,8 +169,8 @@ static void all(int fd, unsigned engine, unsigned flags)
>   
>   	nengine = 0;
>   	if (engine == -1) {
> -		for_each_engine(fd, engine) {
> -			if (!ignore_engine(fd, engine))
> +		for_each_physical_engine(fd, engine) {
> +			if (gem_can_store_dword(fd, engine))
>   				engines[nengine++] = engine;
>   		}
>   	} else {
> diff --git a/tests/gem_exec_reloc.c b/tests/gem_exec_reloc.c
> index 432a42a9..213de1d7 100644
> --- a/tests/gem_exec_reloc.c
> +++ b/tests/gem_exec_reloc.c
> @@ -258,7 +258,7 @@ static void active(int fd, unsigned engine)
>   
>   	nengine = 0;
>   	if (engine == -1) {
> -		for_each_engine(fd, engine) {
> +		for_each_physical_engine(fd, engine) {
>   			if (gem_can_store_dword(fd, engine))
>   				engines[nengine++] = engine;
>   		}
> diff --git a/tests/gem_exec_schedule.c b/tests/gem_exec_schedule.c
> index 5f24df33..03d457e9 100644
> --- a/tests/gem_exec_schedule.c
> +++ b/tests/gem_exec_schedule.c
> @@ -188,17 +188,6 @@ static void fifo(int fd, unsigned ring)
>   	munmap(ptr, 4096);
>   }
>   
> -static bool ignore_engine(int fd, unsigned engine)
> -{
> -	if (engine == 0)
> -		return true;
> -
> -	if (gem_has_bsd2(fd) && engine == I915_EXEC_BSD)
> -		return true;
> -
> -	return false;
> -}
> -
>   static void smoketest(int fd, unsigned ring, unsigned timeout)
>   {
>   	const int ncpus = sysconf(_SC_NPROCESSORS_ONLN);
> @@ -210,12 +199,8 @@ static void smoketest(int fd, unsigned ring, unsigned timeout)
>   
>   	nengine = 0;
>   	if (ring == -1) {
> -		for_each_engine(fd, engine) {
> -			if (ignore_engine(fd, engine))
> -				continue;
> -
> +		for_each_physical_engine(fd, engine)
>   			engines[nengine++] = engine;
> -		}
>   	} else {
>   		engines[nengine++] = ring;
>   	}
> @@ -440,7 +425,7 @@ static void preempt_other(int fd, unsigned ring)
>   	gem_context_set_priority(fd, ctx[HI], MAX_PRIO);
>   
>   	n = 0;
> -	for_each_engine(fd, other) {
> +	for_each_physical_engine(fd, other) {
>   		igt_assert(n < ARRAY_SIZE(spin));
>   
>   		spin[n] = __igt_spin_batch_new(fd, ctx[NOISE], other, 0);
> @@ -496,7 +481,7 @@ static void preempt_self(int fd, unsigned ring)
>   
>   	n = 0;
>   	gem_context_set_priority(fd, ctx[HI], MIN_PRIO);
> -	for_each_engine(fd, other) {
> +	for_each_physical_engine(fd, other) {
>   		spin[n] = __igt_spin_batch_new(fd, ctx[NOISE], other, 0);
>   		store_dword(fd, ctx[HI], other,
>   			    result, (n + 1)*sizeof(uint32_t), n + 1,
> diff --git a/tests/gem_exec_store.c b/tests/gem_exec_store.c
> index 31a2c096..a7673489 100644
> --- a/tests/gem_exec_store.c
> +++ b/tests/gem_exec_store.c
> @@ -220,7 +220,7 @@ static void store_all(int fd)
>   
>   	nengine = 0;
>   	intel_detect_and_clear_missed_interrupts(fd);
> -	for_each_engine(fd, engine) {
> +	for_each_physical_engine(fd, engine) {
>   		if (!gem_can_store_dword(fd, engine))
>   			continue;
>   
> diff --git a/tests/gem_exec_suspend.c b/tests/gem_exec_suspend.c
> index bbdc6e55..351347cb 100644
> --- a/tests/gem_exec_suspend.c
> +++ b/tests/gem_exec_suspend.c
> @@ -62,25 +62,13 @@ static void check_bo(int fd, uint32_t handle)
>   	munmap(map, 4096);
>   }
>   
> -static bool ignore_engine(int fd, unsigned engine)
> -{
> -	if (engine == 0)
> -		return true;
> -
> -	if (!gem_can_store_dword(fd, engine))
> -		return true;
> -
> -	return false;
> -}
> -
>   static void test_all(int fd, unsigned flags)
>   {
>   	unsigned engine;
>   
> -	for_each_engine(fd, engine) {
> -		if (!ignore_engine(fd, engine))
> +	for_each_physical_engine(fd, engine)
> +		if (gem_can_store_dword(fd, engine))
>   			run_test(fd, engine, flags & ~0xff);
> -	}
>   }
>   
>   static bool has_semaphores(int fd)
> @@ -118,8 +106,8 @@ static void run_test(int fd, unsigned engine, unsigned flags)
>   		 * GPU is then unlikely to be active!)
>   		 */
>   		if (has_semaphores(fd)) {
> -			for_each_engine(fd, engine) {
> -				if (!ignore_engine(fd, engine))
> +			for_each_physical_engine(fd, engine) {
> +				if (gem_can_store_dword(fd, engine))
>   					engines[nengine++] = engine;
>   			}
>   		} else {
> diff --git a/tests/gem_exec_whisper.c b/tests/gem_exec_whisper.c
> index 5f9fedf5..1f4dad63 100644
> --- a/tests/gem_exec_whisper.c
> +++ b/tests/gem_exec_whisper.c
> @@ -79,17 +79,6 @@ static void verify_reloc(int fd, uint32_t handle,
>   	}
>   }
>   
> -static bool ignore_engine(int fd, unsigned engine)
> -{
> -	if (engine == 0)
> -		return true;
> -
> -	if (!gem_can_store_dword(fd, engine))
> -		return true;
> -
> -	return false;
> -}
> -
>   #define CONTEXTS 0x1
>   #define FDS 0x2
>   #define INTERRUPTIBLE 0x4
> @@ -217,8 +206,8 @@ static void whisper(int fd, unsigned engine, unsigned flags)
>   
>   	nengine = 0;
>   	if (engine == -1) {
> -		for_each_engine(fd, engine) {
> -			if (!ignore_engine(fd, engine))
> +		for_each_physical_engine(fd, engine) {
> +			if (gem_can_store_dword(fd, engine))
>   				engines[nengine++] = engine;
>   		}
>   	} else {
> diff --git a/tests/gem_ring_sync_loop.c b/tests/gem_ring_sync_loop.c
> index ab5bedb3..118f3638 100644
> --- a/tests/gem_ring_sync_loop.c
> +++ b/tests/gem_ring_sync_loop.c
> @@ -48,7 +48,7 @@ sync_loop(int fd)
>   	int i;
>   
>   	nengine = 0;
> -	for_each_engine(fd, engine)
> +	for_each_physical_engine(fd, engine)
>   		engines[nengine++] = engine;
>   	igt_require(nengine);
>   
> diff --git a/tests/gem_spin_batch.c b/tests/gem_spin_batch.c
> index 89631130..026f9830 100644
> --- a/tests/gem_spin_batch.c
> +++ b/tests/gem_spin_batch.c
> @@ -76,10 +76,7 @@ static void spin_on_all_engines(int fd, unsigned int timeout_sec)
>   {
>   	unsigned engine;
>   
> -	for_each_engine(fd, engine) {
> -		if (engine == 0)
> -			continue;
> -
> +	for_each_physical_engine(fd, engine) {
>   		igt_fork(child, 1) {
>   			igt_install_exit_handler(spin_exit_handler);
>   			spin(fd, engine, timeout_sec);
> diff --git a/tests/gem_sync.c b/tests/gem_sync.c
> index d70515ea..788fafc3 100644
> --- a/tests/gem_sync.c
> +++ b/tests/gem_sync.c
> @@ -86,23 +86,9 @@ sync_ring(int fd, unsigned ring, int num_children, int timeout)
>   	int num_engines = 0;
>   
>   	if (ring == ~0u) {
> -		const struct intel_execution_engine *e;
> -
> -		for (e = intel_execution_engines; e->name; e++) {
> -			if (e->exec_id == 0)
> -				continue;
> -
> -			if (!gem_has_ring(fd, e->exec_id | e->flags))
> -				continue;
> -
> -			if (e->exec_id == I915_EXEC_BSD) {
> -				int is_bsd2 = e->flags != 0;
> -				if (gem_has_bsd2(fd) != is_bsd2)
> -					continue;
> -			}
> -
> -			names[num_engines] = e->name;
> -			engines[num_engines++] = e->exec_id | e->flags;
> +		for_each_physical_engine(fd, ring) {
> +			names[num_engines] = e__->name;
> +			engines[num_engines++] = ring;
>   			if (num_engines == ARRAY_SIZE(engines))
>   				break;
>   		}
> @@ -200,26 +186,9 @@ store_ring(int fd, unsigned ring, int num_children, int timeout)
>   	int num_engines = 0;
>   
>   	if (ring == ~0u) {
> -		const struct intel_execution_engine *e;
> -
> -		for (e = intel_execution_engines; e->name; e++) {
> -			if (e->exec_id == 0)
> -				continue;
> -
> -			if (!gem_has_ring(fd, e->exec_id | e->flags))
> -				continue;
> -
> -			if (!gem_can_store_dword(fd, e->exec_id | e->flags))
> -				continue;
> -
> -			if (e->exec_id == I915_EXEC_BSD) {
> -				int is_bsd2 = e->flags != 0;
> -				if (gem_has_bsd2(fd) != is_bsd2)
> -					continue;
> -			}
> -
> -			names[num_engines] = e->name;
> -			engines[num_engines++] = e->exec_id | e->flags;
> +		for_each_physical_engine(fd, ring) {

gem_can_store_dword gone.

> +			names[num_engines] = e__->name;
> +			engines[num_engines++] = ring;
>   			if (num_engines == ARRAY_SIZE(engines))
>   				break;
>   		}
> @@ -502,31 +471,14 @@ store_many(int fd, unsigned ring, int timeout)
>   	intel_detect_and_clear_missed_interrupts(fd);
>   
>   	if (ring == ~0u) {
> -		const struct intel_execution_engine *e;
> -
> -		for (e = intel_execution_engines; e->name; e++) {
> -			if (e->exec_id == 0)
> -				continue;
> -
> -			if (!gem_has_ring(fd, e->exec_id | e->flags))
> -				continue;
> -
> -			if (!gem_can_store_dword(fd, e->exec_id | e->flags))
> -				continue;
> -
> -			if (e->exec_id == I915_EXEC_BSD) {
> -				int is_bsd2 = e->flags != 0;
> -				if (gem_has_bsd2(fd) != is_bsd2)
> -					continue;
> -			}
> -
> +		for_each_physical_engine(fd, ring) {

gem_can_store_dword again.

>   			igt_fork(child, 1)
>   				__store_many(fd,
> -					     e->exec_id | e->flags,
> +					     ring,
>   					     timeout,
>   					     &shared[n]);
>   
> -			names[n++] = e->name;
> +			names[n++] = e__->name;
>   		}
>   		igt_waitchildren();
>   	} else {
> @@ -737,23 +689,9 @@ preempt(int fd, unsigned ring, int num_children, int timeout)
>   	uint32_t ctx[2];
>   
>   	if (ring == ~0u) {
> -		const struct intel_execution_engine *e;
> -
> -		for (e = intel_execution_engines; e->name; e++) {
> -			if (e->exec_id == 0)
> -				continue;
> -
> -			if (!gem_has_ring(fd, e->exec_id | e->flags))
> -				continue;
> -
> -			if (e->exec_id == I915_EXEC_BSD) {
> -				int is_bsd2 = e->flags != 0;
> -				if (gem_has_bsd2(fd) != is_bsd2)
> -					continue;
> -			}
> -
> -			names[num_engines] = e->name;
> -			engines[num_engines++] = e->exec_id | e->flags;
> +		for_each_physical_engine(fd, ring) {
> +			names[num_engines] = e__->name;
> +			engines[num_engines++] = ring;
>   			if (num_engines == ARRAY_SIZE(engines))
>   				break;
>   		}
> 

No complaints.

Hm, what I did with intel_execution_engines2 to fake class/instance. 
Have to think if that could somehow be merged into one. Or perhaps the 
approach adopted and then when available just switch the implementation 
to real class/instance.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [igt-dev] [Intel-gfx] [PATCH igt 1/2] Iterate over physical engines

[Tvrtko Ursulin]

On 21/02/2018 14:45, Chris Wilson wrote:
> We current have a single for_each_engine() iterator which we use to
> generate both a set of uABI engines and a set of physical engines.
> Determining what uABI ring-id corresponds to an actual HW engine is
> tricky, so pull that out to a library function and introduce
> for_each_physical_engine() for cases where we want to issue requests
> once on each HW ring (avoiding aliasing issues).

As you know I tried to make for_each_engine actually behave like this 
(iterate physical engines). I still think it would be better / more 
intuitive, and leave the uABI for a specially named iterator instead. 
However, I am willing to accept this compromise in order to start moving 
towards the long overdue cleanup in this respect.

> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   lib/igt_gt.c               | 23 +++++++++++++
>   lib/igt_gt.h               |  9 +++++
>   tests/amdgpu/amd_prime.c   |  6 +---
>   tests/gem_concurrent_all.c |  2 +-
>   tests/gem_ctx_create.c     |  5 +--
>   tests/gem_exec_await.c     | 17 +--------
>   tests/gem_exec_create.c    | 17 +--------
>   tests/gem_exec_fence.c     | 16 ++-------
>   tests/gem_exec_gttfill.c   | 16 +--------
>   tests/gem_exec_nop.c       | 32 +++--------------
>   tests/gem_exec_parallel.c  | 15 ++------
>   tests/gem_exec_reloc.c     |  2 +-
>   tests/gem_exec_schedule.c  | 21 ++---------
>   tests/gem_exec_store.c     |  2 +-
>   tests/gem_exec_suspend.c   | 20 +++--------
>   tests/gem_exec_whisper.c   | 15 ++------
>   tests/gem_ring_sync_loop.c |  2 +-
>   tests/gem_spin_batch.c     |  5 +--
>   tests/gem_sync.c           | 86 +++++++---------------------------------------
>   19 files changed, 72 insertions(+), 239 deletions(-)
> 
> diff --git a/lib/igt_gt.c b/lib/igt_gt.c
> index f70fcb92..e630550b 100644
> --- a/lib/igt_gt.c
> +++ b/lib/igt_gt.c
> @@ -660,3 +660,26 @@ bool gem_has_engine(int gem_fd,
>   			    gem_class_instance_to_eb_flags(gem_fd, class,
>   							   instance));
>   }
> +
> +bool gem_ring_is_physical_engine(int fd, unsigned ring)
> +{
> +	if (ring == I915_EXEC_DEFAULT)
> +		return false;
> +
> +	/* BSD uses an extra flag to chose between aliasing modes */
> +	if ((ring & 63) == I915_EXEC_BSD) {

I can see you have a special fondness towards 63 :), but at least hex 
mask would look less out of place.

I do wonder how we ended up with I915_EXEC_RING_MASK so out of sync. Why 
shouldn't we just bump it in uapi headers?

> +		bool explicit_bsd = ring & (3 << 13);
> +		bool has_bsd2 = gem_has_bsd2(fd);
> +		return explicit_bsd ? has_bsd2 : !has_bsd2;
> +	}
> +
> +	return true;
> +}
> +
> +bool gem_ring_has_physical_engine(int fd, unsigned ring)
> +{
> +	if (!gem_ring_is_physical_engine(fd, ring))
> +		return false;
> +
> +	return gem_has_ring(fd, ring);
> +}
> diff --git a/lib/igt_gt.h b/lib/igt_gt.h
> index 68592410..4d9d1aa0 100644
> --- a/lib/igt_gt.h
> +++ b/lib/igt_gt.h
> @@ -81,6 +81,15 @@ extern const struct intel_execution_engine {
>   	     e__++) \
>   		for_if (gem_has_ring(fd__, flags__ = e__->exec_id | e__->flags))
>   
> +#define for_each_physical_engine(fd__, flags__) \
> +	for (const struct intel_execution_engine *e__ = intel_execution_engines;\
> +	     e__->name; \
> +	     e__++) \
> +		for_if (gem_ring_has_physical_engine(fd__, flags__ = e__->exec_id | e__->flags))
> +
> +bool gem_ring_is_physical_engine(int fd, unsigned int ring);
> +bool gem_ring_has_physical_engine(int fd, unsigned int ring);

gcc shouldn't ever compiled on unsigned vs unsigned int in declaration 
vs definition right?

> +
>   bool gem_can_store_dword(int fd, unsigned int engine);
>   
>   extern const struct intel_execution_engine2 {
> diff --git a/tests/amdgpu/amd_prime.c b/tests/amdgpu/amd_prime.c
> index b2f326b4..bb68ccf3 100644
> --- a/tests/amdgpu/amd_prime.c
> +++ b/tests/amdgpu/amd_prime.c
> @@ -179,12 +179,8 @@ static void i915_to_amd(int i915, int amd, amdgpu_device_handle device)
>   	struct cork c;
>   
>   	nengine = 0;
> -	for_each_engine(i915, engine) {
> -		if (engine == 0)
> -			continue;
> -
> +	for_each_physical_engine(i915, engine)
>   		engines[nengine++] = engine;
> -	}
>   	igt_require(nengine);
>   
>   	memset(obj, 0, sizeof(obj));
> diff --git a/tests/gem_concurrent_all.c b/tests/gem_concurrent_all.c
> index 201b491b..3a1097ba 100644
> --- a/tests/gem_concurrent_all.c
> +++ b/tests/gem_concurrent_all.c
> @@ -960,7 +960,7 @@ static igt_hang_t all_hang(void)
>   	execbuf.buffers_ptr = to_user_pointer(&obj);
>   	execbuf.buffer_count = 1;
>   
> -	for_each_engine(fd, engine) {
> +	for_each_physical_engine(fd, engine) {
>   		hang = igt_hang_ring(fd, engine);
>   
>   		execbuf.flags = engine;
> diff --git a/tests/gem_ctx_create.c b/tests/gem_ctx_create.c
> index 058445b6..1b32d6c3 100644
> --- a/tests/gem_ctx_create.c
> +++ b/tests/gem_ctx_create.c
> @@ -321,11 +321,8 @@ igt_main
>   		igt_require_gem(fd);
>   		gem_require_contexts(fd);
>   
> -		for_each_engine(fd, engine) {
> -			if (engine == 0)
> -				continue;
> +		for_each_physical_engine(fd, engine)
>   			all_engines[all_nengine++] = engine;
> -		}
>   		igt_require(all_nengine);
>   
>   		if (gem_uses_full_ppgtt(fd)) {
> diff --git a/tests/gem_exec_await.c b/tests/gem_exec_await.c
> index e19363c4..fccc24d9 100644
> --- a/tests/gem_exec_await.c
> +++ b/tests/gem_exec_await.c
> @@ -44,17 +44,6 @@ static double elapsed(const struct timespec *start, const struct timespec *end)
>   		(end->tv_nsec - start->tv_nsec)*1e-9);
>   }
>   
> -static bool ignore_engine(int fd, unsigned engine)
> -{
> -	if (engine == 0)
> -		return true;
> -
> -	if (gem_has_bsd2(fd) && engine == I915_EXEC_BSD)
> -		return true;
> -
> -	return false;
> -}
> -
>   static void xchg_obj(void *array, unsigned i, unsigned j)
>   {
>   	struct drm_i915_gem_exec_object2 *obj = array;
> @@ -89,12 +78,8 @@ static void wide(int fd, int ring_size, int timeout, unsigned int flags)
>   	double time;
>   
>   	nengine = 0;
> -	for_each_engine(fd, engine) {
> -		if (ignore_engine(fd, engine))
> -			continue;
> -
> +	for_each_physical_engine(fd, engine)
>   		engines[nengine++] = engine;
> -	}
>   	igt_require(nengine);
>   
>   	exec = calloc(nengine, sizeof(*exec));
> diff --git a/tests/gem_exec_create.c b/tests/gem_exec_create.c
> index 28479a9d..54a2429e 100644
> --- a/tests/gem_exec_create.c
> +++ b/tests/gem_exec_create.c
> @@ -54,17 +54,6 @@ static double elapsed(const struct timespec *start, const struct timespec *end)
>   		(end->tv_nsec - start->tv_nsec)*1e-9);
>   }
>   
> -static bool ignore_engine(int fd, unsigned engine)
> -{
> -	if (engine == 0)
> -		return true;
> -
> -	if (gem_has_bsd2(fd) && engine == I915_EXEC_BSD)
> -		return true;
> -
> -	return false;
> -}
> -
>   #define LEAK 0x1
>   
>   static void all(int fd, unsigned flags, int timeout, int ncpus)
> @@ -77,12 +66,8 @@ static void all(int fd, unsigned flags, int timeout, int ncpus)
>   	unsigned engine;
>   
>   	nengine = 0;
> -	for_each_engine(fd, engine) {
> -		if (ignore_engine(fd, engine))
> -			continue;
> -
> +	for_each_physical_engine(fd, engine)
>   		engines[nengine++] = engine;
> -	}
>   	igt_require(nengine);
>   
>   	memset(&obj, 0, sizeof(obj));
> diff --git a/tests/gem_exec_fence.c b/tests/gem_exec_fence.c
> index 312505d6..39ca17ee 100644
> --- a/tests/gem_exec_fence.c
> +++ b/tests/gem_exec_fence.c
> @@ -273,7 +273,7 @@ static void test_fence_await(int fd, unsigned ring, unsigned flags)
>   	igt_assert(fence != -1);
>   
>   	i = 0;
> -	for_each_engine(fd, engine) {
> +	for_each_physical_engine(fd, engine) {
>   		if (!gem_can_store_dword(fd, engine))
>   			continue;
>   
> @@ -521,10 +521,7 @@ static void test_parallel(int fd, unsigned int master)
>   	obj[BATCH].relocation_count = 1;
>   
>   	/* Queue all secondaries */
> -	for_each_engine(fd, engine) {
> -		if (engine == 0 || engine == I915_EXEC_BSD)
> -			continue;

Behaviour change - BSD(s) not skipped any more, relevant?.

> -
> +	for_each_physical_engine(fd, engine) {
>   		if (engine == master)
>   			continue;
>   
> @@ -699,15 +696,8 @@ static void test_long_history(int fd, long ring_size, unsigned flags)
>   		limit = ring_size / 3;
>   
>   	nengine = 0;
> -	for_each_engine(fd, engine) {
> -		if (engine == 0)
> -			continue;
> -
> -		if (engine == I915_EXEC_BSD)
> -			continue;

Same.

> -
> +	for_each_physical_engine(fd, engine)
>   		engines[nengine++] = engine;
> -	}
>   	igt_require(nengine);
>   
>   	gem_quiescent_gpu(fd);
> diff --git a/tests/gem_exec_gttfill.c b/tests/gem_exec_gttfill.c
> index 96ed832f..4097e407 100644
> --- a/tests/gem_exec_gttfill.c
> +++ b/tests/gem_exec_gttfill.c
> @@ -28,17 +28,6 @@ IGT_TEST_DESCRIPTION("Fill the GTT with batches.");
>   
>   #define BATCH_SIZE (4096<<10)
>   
> -static bool ignore_engine(int fd, unsigned engine)
> -{
> -	if (engine == 0)
> -		return true;
> -
> -	if (gem_has_bsd2(fd) && engine == I915_EXEC_BSD)
> -		return true;
> -
> -	return false;
> -}
> -
>   static void xchg_u32(void *array, unsigned i, unsigned j)
>   {
>   	uint32_t *u32 = array;
> @@ -126,10 +115,7 @@ static void fillgtt(int fd, unsigned ring, int timeout)
>   
>   	nengine = 0;
>   	if (ring == 0) {
> -		for_each_engine(fd, engine) {
> -			if (ignore_engine(fd, engine))
> -				continue;
> -
> +		for_each_physical_engine(fd, engine) {
>   			if (!gem_can_store_dword(fd, engine))
>   				continue;
>   
> diff --git a/tests/gem_exec_nop.c b/tests/gem_exec_nop.c
> index d3e9a3e0..d971ffcb 100644
> --- a/tests/gem_exec_nop.c
> +++ b/tests/gem_exec_nop.c
> @@ -188,17 +188,6 @@ static void headless(int fd, uint32_t handle)
>   	assert_within_epsilon(n_headless, n_display, 0.1f);
>   }
>   
> -static bool ignore_engine(int fd, unsigned engine)
> -{
> -	if (engine == 0)
> -		return true;
> -
> -	if (gem_has_bsd2(fd) && engine == I915_EXEC_BSD)
> -		return true;
> -
> -	return false;
> -}
> -
>   static void parallel(int fd, uint32_t handle, int timeout)
>   {
>   	struct drm_i915_gem_execbuffer2 execbuf;
> @@ -212,10 +201,7 @@ static void parallel(int fd, uint32_t handle, int timeout)
>   
>   	sum = 0;
>   	nengine = 0;
> -	for_each_engine(fd, engine) {
> -		if (ignore_engine(fd, engine))
> -			continue;
> -
> +	for_each_physical_engine(fd, engine) {
>   		engines[nengine] = engine;
>   		names[nengine] = e__->name;
>   		nengine++;
> @@ -277,10 +263,7 @@ static void series(int fd, uint32_t handle, int timeout)
>   	const char *name;
>   
>   	nengine = 0;
> -	for_each_engine(fd, engine) {
> -		if (ignore_engine(fd, engine))
> -			continue;
> -
> +	for_each_physical_engine(fd, engine) {
>   		time = nop_on_ring(fd, handle, engine, 1, &count) / count;
>   		if (time > max) {
>   			name = e__->name;
> @@ -375,12 +358,9 @@ static void sequential(int fd, uint32_t handle, unsigned flags, int timeout)
>   
>   	nengine = 0;
>   	sum = 0;
> -	for_each_engine(fd, n) {
> +	for_each_physical_engine(fd, n) {
>   		unsigned long count;
>   
> -		if (ignore_engine(fd, n))
> -			continue;
> -
>   		time = nop_on_ring(fd, handle, n, 1, &count) / count;
>   		sum += time;
>   		igt_debug("%s: %.3fus\n", e__->name, 1e6*time);
> @@ -509,12 +489,8 @@ static void fence_signal(int fd, uint32_t handle,
>   
>   	nengine = 0;
>   	if (ring_id == -1) {
> -		for_each_engine(fd, n) {
> -			if (ignore_engine(fd, n))
> -				continue;
> -
> +		for_each_physical_engine(fd, n)
>   			engines[nengine++] = n;
> -		}
>   	} else {
>   		gem_require_ring(fd, ring_id);
>   		engines[nengine++] = ring_id;
> diff --git a/tests/gem_exec_parallel.c b/tests/gem_exec_parallel.c
> index 11b6e775..fe5ffe8f 100644
> --- a/tests/gem_exec_parallel.c
> +++ b/tests/gem_exec_parallel.c
> @@ -55,17 +55,6 @@ static void check_bo(int fd, uint32_t handle, int pass)
>   	munmap(map, 4096);
>   }
>   
> -static bool ignore_engine(int fd, unsigned engine)
> -{
> -	if (engine == 0)
> -		return true;
> -
> -	if (!gem_can_store_dword(fd, engine))
> -		return true;
> -
> -	return false;
> -}
> -
>   #define CONTEXTS 0x1
>   #define FDS 0x2
>   
> @@ -180,8 +169,8 @@ static void all(int fd, unsigned engine, unsigned flags)
>   
>   	nengine = 0;
>   	if (engine == -1) {
> -		for_each_engine(fd, engine) {
> -			if (!ignore_engine(fd, engine))
> +		for_each_physical_engine(fd, engine) {
> +			if (gem_can_store_dword(fd, engine))
>   				engines[nengine++] = engine;
>   		}
>   	} else {
> diff --git a/tests/gem_exec_reloc.c b/tests/gem_exec_reloc.c
> index 432a42a9..213de1d7 100644
> --- a/tests/gem_exec_reloc.c
> +++ b/tests/gem_exec_reloc.c
> @@ -258,7 +258,7 @@ static void active(int fd, unsigned engine)
>   
>   	nengine = 0;
>   	if (engine == -1) {
> -		for_each_engine(fd, engine) {
> +		for_each_physical_engine(fd, engine) {
>   			if (gem_can_store_dword(fd, engine))
>   				engines[nengine++] = engine;
>   		}
> diff --git a/tests/gem_exec_schedule.c b/tests/gem_exec_schedule.c
> index 5f24df33..03d457e9 100644
> --- a/tests/gem_exec_schedule.c
> +++ b/tests/gem_exec_schedule.c
> @@ -188,17 +188,6 @@ static void fifo(int fd, unsigned ring)
>   	munmap(ptr, 4096);
>   }
>   
> -static bool ignore_engine(int fd, unsigned engine)
> -{
> -	if (engine == 0)
> -		return true;
> -
> -	if (gem_has_bsd2(fd) && engine == I915_EXEC_BSD)
> -		return true;
> -
> -	return false;
> -}
> -
>   static void smoketest(int fd, unsigned ring, unsigned timeout)
>   {
>   	const int ncpus = sysconf(_SC_NPROCESSORS_ONLN);
> @@ -210,12 +199,8 @@ static void smoketest(int fd, unsigned ring, unsigned timeout)
>   
>   	nengine = 0;
>   	if (ring == -1) {
> -		for_each_engine(fd, engine) {
> -			if (ignore_engine(fd, engine))
> -				continue;
> -
> +		for_each_physical_engine(fd, engine)
>   			engines[nengine++] = engine;
> -		}
>   	} else {
>   		engines[nengine++] = ring;
>   	}
> @@ -440,7 +425,7 @@ static void preempt_other(int fd, unsigned ring)
>   	gem_context_set_priority(fd, ctx[HI], MAX_PRIO);
>   
>   	n = 0;
> -	for_each_engine(fd, other) {
> +	for_each_physical_engine(fd, other) {
>   		igt_assert(n < ARRAY_SIZE(spin));
>   
>   		spin[n] = __igt_spin_batch_new(fd, ctx[NOISE], other, 0);
> @@ -496,7 +481,7 @@ static void preempt_self(int fd, unsigned ring)
>   
>   	n = 0;
>   	gem_context_set_priority(fd, ctx[HI], MIN_PRIO);
> -	for_each_engine(fd, other) {
> +	for_each_physical_engine(fd, other) {
>   		spin[n] = __igt_spin_batch_new(fd, ctx[NOISE], other, 0);
>   		store_dword(fd, ctx[HI], other,
>   			    result, (n + 1)*sizeof(uint32_t), n + 1,
> diff --git a/tests/gem_exec_store.c b/tests/gem_exec_store.c
> index 31a2c096..a7673489 100644
> --- a/tests/gem_exec_store.c
> +++ b/tests/gem_exec_store.c
> @@ -220,7 +220,7 @@ static void store_all(int fd)
>   
>   	nengine = 0;
>   	intel_detect_and_clear_missed_interrupts(fd);
> -	for_each_engine(fd, engine) {
> +	for_each_physical_engine(fd, engine) {
>   		if (!gem_can_store_dword(fd, engine))
>   			continue;
>   
> diff --git a/tests/gem_exec_suspend.c b/tests/gem_exec_suspend.c
> index bbdc6e55..351347cb 100644
> --- a/tests/gem_exec_suspend.c
> +++ b/tests/gem_exec_suspend.c
> @@ -62,25 +62,13 @@ static void check_bo(int fd, uint32_t handle)
>   	munmap(map, 4096);
>   }
>   
> -static bool ignore_engine(int fd, unsigned engine)
> -{
> -	if (engine == 0)
> -		return true;
> -
> -	if (!gem_can_store_dword(fd, engine))
> -		return true;
> -
> -	return false;
> -}
> -
>   static void test_all(int fd, unsigned flags)
>   {
>   	unsigned engine;
>   
> -	for_each_engine(fd, engine) {
> -		if (!ignore_engine(fd, engine))
> +	for_each_physical_engine(fd, engine)
> +		if (gem_can_store_dword(fd, engine))
>   			run_test(fd, engine, flags & ~0xff);
> -	}
>   }
>   
>   static bool has_semaphores(int fd)
> @@ -118,8 +106,8 @@ static void run_test(int fd, unsigned engine, unsigned flags)
>   		 * GPU is then unlikely to be active!)
>   		 */
>   		if (has_semaphores(fd)) {
> -			for_each_engine(fd, engine) {
> -				if (!ignore_engine(fd, engine))
> +			for_each_physical_engine(fd, engine) {
> +				if (gem_can_store_dword(fd, engine))
>   					engines[nengine++] = engine;
>   			}
>   		} else {
> diff --git a/tests/gem_exec_whisper.c b/tests/gem_exec_whisper.c
> index 5f9fedf5..1f4dad63 100644
> --- a/tests/gem_exec_whisper.c
> +++ b/tests/gem_exec_whisper.c
> @@ -79,17 +79,6 @@ static void verify_reloc(int fd, uint32_t handle,
>   	}
>   }
>   
> -static bool ignore_engine(int fd, unsigned engine)
> -{
> -	if (engine == 0)
> -		return true;
> -
> -	if (!gem_can_store_dword(fd, engine))
> -		return true;
> -
> -	return false;
> -}
> -
>   #define CONTEXTS 0x1
>   #define FDS 0x2
>   #define INTERRUPTIBLE 0x4
> @@ -217,8 +206,8 @@ static void whisper(int fd, unsigned engine, unsigned flags)
>   
>   	nengine = 0;
>   	if (engine == -1) {
> -		for_each_engine(fd, engine) {
> -			if (!ignore_engine(fd, engine))
> +		for_each_physical_engine(fd, engine) {
> +			if (gem_can_store_dword(fd, engine))
>   				engines[nengine++] = engine;
>   		}
>   	} else {
> diff --git a/tests/gem_ring_sync_loop.c b/tests/gem_ring_sync_loop.c
> index ab5bedb3..118f3638 100644
> --- a/tests/gem_ring_sync_loop.c
> +++ b/tests/gem_ring_sync_loop.c
> @@ -48,7 +48,7 @@ sync_loop(int fd)
>   	int i;
>   
>   	nengine = 0;
> -	for_each_engine(fd, engine)
> +	for_each_physical_engine(fd, engine)
>   		engines[nengine++] = engine;
>   	igt_require(nengine);
>   
> diff --git a/tests/gem_spin_batch.c b/tests/gem_spin_batch.c
> index 89631130..026f9830 100644
> --- a/tests/gem_spin_batch.c
> +++ b/tests/gem_spin_batch.c
> @@ -76,10 +76,7 @@ static void spin_on_all_engines(int fd, unsigned int timeout_sec)
>   {
>   	unsigned engine;
>   
> -	for_each_engine(fd, engine) {
> -		if (engine == 0)
> -			continue;
> -
> +	for_each_physical_engine(fd, engine) {
>   		igt_fork(child, 1) {
>   			igt_install_exit_handler(spin_exit_handler);
>   			spin(fd, engine, timeout_sec);
> diff --git a/tests/gem_sync.c b/tests/gem_sync.c
> index d70515ea..788fafc3 100644
> --- a/tests/gem_sync.c
> +++ b/tests/gem_sync.c
> @@ -86,23 +86,9 @@ sync_ring(int fd, unsigned ring, int num_children, int timeout)
>   	int num_engines = 0;
>   
>   	if (ring == ~0u) {
> -		const struct intel_execution_engine *e;
> -
> -		for (e = intel_execution_engines; e->name; e++) {
> -			if (e->exec_id == 0)
> -				continue;
> -
> -			if (!gem_has_ring(fd, e->exec_id | e->flags))
> -				continue;
> -
> -			if (e->exec_id == I915_EXEC_BSD) {
> -				int is_bsd2 = e->flags != 0;
> -				if (gem_has_bsd2(fd) != is_bsd2)
> -					continue;
> -			}
> -
> -			names[num_engines] = e->name;
> -			engines[num_engines++] = e->exec_id | e->flags;
> +		for_each_physical_engine(fd, ring) {
> +			names[num_engines] = e__->name;
> +			engines[num_engines++] = ring;
>   			if (num_engines == ARRAY_SIZE(engines))
>   				break;
>   		}
> @@ -200,26 +186,9 @@ store_ring(int fd, unsigned ring, int num_children, int timeout)
>   	int num_engines = 0;
>   
>   	if (ring == ~0u) {
> -		const struct intel_execution_engine *e;
> -
> -		for (e = intel_execution_engines; e->name; e++) {
> -			if (e->exec_id == 0)
> -				continue;
> -
> -			if (!gem_has_ring(fd, e->exec_id | e->flags))
> -				continue;
> -
> -			if (!gem_can_store_dword(fd, e->exec_id | e->flags))
> -				continue;
> -
> -			if (e->exec_id == I915_EXEC_BSD) {
> -				int is_bsd2 = e->flags != 0;
> -				if (gem_has_bsd2(fd) != is_bsd2)
> -					continue;
> -			}
> -
> -			names[num_engines] = e->name;
> -			engines[num_engines++] = e->exec_id | e->flags;
> +		for_each_physical_engine(fd, ring) {

gem_can_store_dword gone.

> +			names[num_engines] = e__->name;
> +			engines[num_engines++] = ring;
>   			if (num_engines == ARRAY_SIZE(engines))
>   				break;
>   		}
> @@ -502,31 +471,14 @@ store_many(int fd, unsigned ring, int timeout)
>   	intel_detect_and_clear_missed_interrupts(fd);
>   
>   	if (ring == ~0u) {
> -		const struct intel_execution_engine *e;
> -
> -		for (e = intel_execution_engines; e->name; e++) {
> -			if (e->exec_id == 0)
> -				continue;
> -
> -			if (!gem_has_ring(fd, e->exec_id | e->flags))
> -				continue;
> -
> -			if (!gem_can_store_dword(fd, e->exec_id | e->flags))
> -				continue;
> -
> -			if (e->exec_id == I915_EXEC_BSD) {
> -				int is_bsd2 = e->flags != 0;
> -				if (gem_has_bsd2(fd) != is_bsd2)
> -					continue;
> -			}
> -
> +		for_each_physical_engine(fd, ring) {

gem_can_store_dword again.

>   			igt_fork(child, 1)
>   				__store_many(fd,
> -					     e->exec_id | e->flags,
> +					     ring,
>   					     timeout,
>   					     &shared[n]);
>   
> -			names[n++] = e->name;
> +			names[n++] = e__->name;
>   		}
>   		igt_waitchildren();
>   	} else {
> @@ -737,23 +689,9 @@ preempt(int fd, unsigned ring, int num_children, int timeout)
>   	uint32_t ctx[2];
>   
>   	if (ring == ~0u) {
> -		const struct intel_execution_engine *e;
> -
> -		for (e = intel_execution_engines; e->name; e++) {
> -			if (e->exec_id == 0)
> -				continue;
> -
> -			if (!gem_has_ring(fd, e->exec_id | e->flags))
> -				continue;
> -
> -			if (e->exec_id == I915_EXEC_BSD) {
> -				int is_bsd2 = e->flags != 0;
> -				if (gem_has_bsd2(fd) != is_bsd2)
> -					continue;
> -			}
> -
> -			names[num_engines] = e->name;
> -			engines[num_engines++] = e->exec_id | e->flags;
> +		for_each_physical_engine(fd, ring) {
> +			names[num_engines] = e__->name;
> +			engines[num_engines++] = ring;
>   			if (num_engines == ARRAY_SIZE(engines))
>   				break;
>   		}
> 

No complaints.

Hm, what I did with intel_execution_engines2 to fake class/instance. 
Have to think if that could somehow be merged into one. Or perhaps the 
approach adopted and then when available just switch the implementation 
to real class/instance.

Regards,

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

[igt-dev] ✗ Fi.CI.BAT: failure for series starting with [1/2] Iterate over physical engines

[Patchwork]

== Series Details ==

Series: series starting with [1/2] Iterate over physical engines
URL   : https://patchwork.freedesktop.org/series/38704/
State : failure

== Summary ==

IGT patchset tested on top of latest successful build
960e55a87d7b7d7385063e37cc9f281df2be8037 igt/gem_ctx_isolation: Check isolation of registers between contexts

with latest DRM-Tip kernel build CI_DRM_3817
562a3886d1cd drm-tip: 2018y-02m-21d-18h-35m-44s UTC integration manifest

Testlist changes:
+igt@gem_spin_batch@basic-default

Test gem_sync:
        Subgroup basic-many-each:
                pass       -> INCOMPLETE (fi-snb-2520m)
                pass       -> INCOMPLETE (fi-snb-2600)

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:417s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:424s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:374s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:491s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:297s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:479s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:483s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:468s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:460s
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:563s
fi-elk-e7500     total:288  pass:229  dwarn:0   dfail:0   fail:0   skip:59  time:414s
fi-gdg-551       total:288  pass:179  dwarn:0   dfail:0   fail:1   skip:108 time:298s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:512s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:386s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:407s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:454s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:422s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:453s
fi-kbl-7560u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:487s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:446s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:493s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:595s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:430s
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:502s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:519s
fi-skl-6700k2    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:487s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:475s
fi-skl-guc       total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:407s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:432s
fi-snb-2520m     total:149  pass:132  dwarn:0   dfail:0   fail:0   skip:16 
fi-snb-2600      total:149  pass:132  dwarn:0   dfail:0   fail:0   skip:16 

== Logs ==

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

Re: [PATCH igt 1/2] Iterate over physical engines

[Chris Wilson]

Quoting Chris Wilson (2018-02-21 14:45:21)
> diff --git a/tests/gem_sync.c b/tests/gem_sync.c
> index d70515ea..788fafc3 100644
> --- a/tests/gem_sync.c
> +++ b/tests/gem_sync.c
> @@ -86,23 +86,9 @@ sync_ring(int fd, unsigned ring, int num_children, int timeout)
>         int num_engines = 0;
>  
>         if (ring == ~0u) {
> -               const struct intel_execution_engine *e;
> -
> -               for (e = intel_execution_engines; e->name; e++) {
> -                       if (e->exec_id == 0)
> -                               continue;
> -
> -                       if (!gem_has_ring(fd, e->exec_id | e->flags))
> -                               continue;
> -
> -                       if (e->exec_id == I915_EXEC_BSD) {
> -                               int is_bsd2 = e->flags != 0;
> -                               if (gem_has_bsd2(fd) != is_bsd2)
> -                                       continue;
> -                       }
> -
> -                       names[num_engines] = e->name;
> -                       engines[num_engines++] = e->exec_id | e->flags;
> +               for_each_physical_engine(fd, ring) {
> +                       names[num_engines] = e__->name;
> +                       engines[num_engines++] = ring;
>                         if (num_engines == ARRAY_SIZE(engines))
>                                 break;
>                 }
> @@ -200,26 +186,9 @@ store_ring(int fd, unsigned ring, int num_children, int timeout)
>         int num_engines = 0;
>  
>         if (ring == ~0u) {
> -               const struct intel_execution_engine *e;
> -
> -               for (e = intel_execution_engines; e->name; e++) {
> -                       if (e->exec_id == 0)
> -                               continue;
> -
> -                       if (!gem_has_ring(fd, e->exec_id | e->flags))
> -                               continue;
> -
> -                       if (!gem_can_store_dword(fd, e->exec_id | e->flags))
> -                               continue;
> -
> -                       if (e->exec_id == I915_EXEC_BSD) {
> -                               int is_bsd2 = e->flags != 0;
> -                               if (gem_has_bsd2(fd) != is_bsd2)
> -                                       continue;
> -                       }
> -
> -                       names[num_engines] = e->name;
> -                       engines[num_engines++] = e->exec_id | e->flags;
> +               for_each_physical_engine(fd, ring) {
> +                       names[num_engines] = e__->name;
> +                       engines[num_engines++] = ring;
>                         if (num_engines == ARRAY_SIZE(engines))
>                                 break;
>                 }

So I missed the stores inside gem_sync, which require
gem_can_store_dword() protection.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [igt-dev] [PATCH igt 1/2] Iterate over physical engines

[Chris Wilson]

Quoting Chris Wilson (2018-02-21 14:45:21)
> diff --git a/tests/gem_sync.c b/tests/gem_sync.c
> index d70515ea..788fafc3 100644
> --- a/tests/gem_sync.c
> +++ b/tests/gem_sync.c
> @@ -86,23 +86,9 @@ sync_ring(int fd, unsigned ring, int num_children, int timeout)
>         int num_engines = 0;
>  
>         if (ring == ~0u) {
> -               const struct intel_execution_engine *e;
> -
> -               for (e = intel_execution_engines; e->name; e++) {
> -                       if (e->exec_id == 0)
> -                               continue;
> -
> -                       if (!gem_has_ring(fd, e->exec_id | e->flags))
> -                               continue;
> -
> -                       if (e->exec_id == I915_EXEC_BSD) {
> -                               int is_bsd2 = e->flags != 0;
> -                               if (gem_has_bsd2(fd) != is_bsd2)
> -                                       continue;
> -                       }
> -
> -                       names[num_engines] = e->name;
> -                       engines[num_engines++] = e->exec_id | e->flags;
> +               for_each_physical_engine(fd, ring) {
> +                       names[num_engines] = e__->name;
> +                       engines[num_engines++] = ring;
>                         if (num_engines == ARRAY_SIZE(engines))
>                                 break;
>                 }
> @@ -200,26 +186,9 @@ store_ring(int fd, unsigned ring, int num_children, int timeout)
>         int num_engines = 0;
>  
>         if (ring == ~0u) {
> -               const struct intel_execution_engine *e;
> -
> -               for (e = intel_execution_engines; e->name; e++) {
> -                       if (e->exec_id == 0)
> -                               continue;
> -
> -                       if (!gem_has_ring(fd, e->exec_id | e->flags))
> -                               continue;
> -
> -                       if (!gem_can_store_dword(fd, e->exec_id | e->flags))
> -                               continue;
> -
> -                       if (e->exec_id == I915_EXEC_BSD) {
> -                               int is_bsd2 = e->flags != 0;
> -                               if (gem_has_bsd2(fd) != is_bsd2)
> -                                       continue;
> -                       }
> -
> -                       names[num_engines] = e->name;
> -                       engines[num_engines++] = e->exec_id | e->flags;
> +               for_each_physical_engine(fd, ring) {
> +                       names[num_engines] = e__->name;
> +                       engines[num_engines++] = ring;
>                         if (num_engines == ARRAY_SIZE(engines))
>                                 break;
>                 }

So I missed the stores inside gem_sync, which require
gem_can_store_dword() protection.
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [PATCH igt 1/2] Iterate over physical engines

[Chris Wilson]

Quoting Tvrtko Ursulin (2018-02-21 16:25:34)
> 
> On 21/02/2018 14:45, Chris Wilson wrote:
> > We current have a single for_each_engine() iterator which we use to
> > generate both a set of uABI engines and a set of physical engines.
> > Determining what uABI ring-id corresponds to an actual HW engine is
> > tricky, so pull that out to a library function and introduce
> > for_each_physical_engine() for cases where we want to issue requests
> > once on each HW ring (avoiding aliasing issues).
> 
> As you know I tried to make for_each_engine actually behave like this 
> (iterate physical engines). I still think it would be better / more 
> intuitive, and leave the uABI for a specially named iterator instead. 
> However, I am willing to accept this compromise in order to start moving 
> towards the long overdue cleanup in this respect.

Right, next step would be s/for_each_engine/for_each_uabi_ring/ to catch
the remaining cases, although most now in this case iterate over the
engine static array and igt_require(gem_has_ring()), so that the test
list is stable across machines.

[snip]

> Hm, what I did with intel_execution_engines2 to fake class/instance. 
> Have to think if that could somehow be merged into one. Or perhaps the 
> approach adopted and then when available just switch the implementation 
> to real class/instance.

Yeah, I think of this as an intermediate stepping stone so will be
refined as we refine the uABI. In the meantime, if there are trivial
refactors, no problem, but I wouldn't spend too much effort forcing it
as I think we will end up replacing it.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH igt 1/2] Iterate over physical engines

[Chris Wilson]

Quoting Tvrtko Ursulin (2018-02-21 16:25:34)
> 
> On 21/02/2018 14:45, Chris Wilson wrote:
> > We current have a single for_each_engine() iterator which we use to
> > generate both a set of uABI engines and a set of physical engines.
> > Determining what uABI ring-id corresponds to an actual HW engine is
> > tricky, so pull that out to a library function and introduce
> > for_each_physical_engine() for cases where we want to issue requests
> > once on each HW ring (avoiding aliasing issues).
> 
> As you know I tried to make for_each_engine actually behave like this 
> (iterate physical engines). I still think it would be better / more 
> intuitive, and leave the uABI for a specially named iterator instead. 
> However, I am willing to accept this compromise in order to start moving 
> towards the long overdue cleanup in this respect.

Right, next step would be s/for_each_engine/for_each_uabi_ring/ to catch
the remaining cases, although most now in this case iterate over the
engine static array and igt_require(gem_has_ring()), so that the test
list is stable across machines.

[snip]

> Hm, what I did with intel_execution_engines2 to fake class/instance. 
> Have to think if that could somehow be merged into one. Or perhaps the 
> approach adopted and then when available just switch the implementation 
> to real class/instance.

Yeah, I think of this as an intermediate stepping stone so will be
refined as we refine the uABI. In the meantime, if there are trivial
refactors, no problem, but I wouldn't spend too much effort forcing it
as I think we will end up replacing it.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH igt v2] Iterate over physical engines

[Chris Wilson]

We current have a single for_each_engine() iterator which we use to
generate both a set of uABI engines and a set of physical engines.
Determining what uABI ring-id corresponds to an actual HW engine is
tricky, so pull that out to a library function and introduce
for_each_physical_engine() for cases where we want to issue requests
once on each HW ring (avoiding aliasing issues).

v2: Remember can_store_dword for gem_sync

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 lib/igt_gt.c               |  23 ++++++++++
 lib/igt_gt.h               |   9 ++++
 tests/amdgpu/amd_prime.c   |   6 +--
 tests/gem_concurrent_all.c |   2 +-
 tests/gem_ctx_create.c     |   5 +--
 tests/gem_exec_await.c     |  17 +-------
 tests/gem_exec_create.c    |  17 +-------
 tests/gem_exec_fence.c     |  16 ++-----
 tests/gem_exec_gttfill.c   |  16 +------
 tests/gem_exec_nop.c       |  32 ++------------
 tests/gem_exec_parallel.c  |  15 +------
 tests/gem_exec_reloc.c     |   2 +-
 tests/gem_exec_schedule.c  |  21 ++-------
 tests/gem_exec_store.c     |   2 +-
 tests/gem_exec_suspend.c   |  20 ++-------
 tests/gem_exec_whisper.c   |  15 +------
 tests/gem_ring_sync_loop.c |   2 +-
 tests/gem_spin_batch.c     |   5 +--
 tests/gem_sync.c           | 104 ++++++++-------------------------------------
 19 files changed, 78 insertions(+), 251 deletions(-)

diff --git a/lib/igt_gt.c b/lib/igt_gt.c
index f70fcb92..e630550b 100644
--- a/lib/igt_gt.c
+++ b/lib/igt_gt.c
@@ -660,3 +660,26 @@ bool gem_has_engine(int gem_fd,
 			    gem_class_instance_to_eb_flags(gem_fd, class,
 							   instance));
 }
+
+bool gem_ring_is_physical_engine(int fd, unsigned ring)
+{
+	if (ring == I915_EXEC_DEFAULT)
+		return false;
+
+	/* BSD uses an extra flag to chose between aliasing modes */
+	if ((ring & 63) == I915_EXEC_BSD) {
+		bool explicit_bsd = ring & (3 << 13);
+		bool has_bsd2 = gem_has_bsd2(fd);
+		return explicit_bsd ? has_bsd2 : !has_bsd2;
+	}
+
+	return true;
+}
+
+bool gem_ring_has_physical_engine(int fd, unsigned ring)
+{
+	if (!gem_ring_is_physical_engine(fd, ring))
+		return false;
+
+	return gem_has_ring(fd, ring);
+}
diff --git a/lib/igt_gt.h b/lib/igt_gt.h
index 68592410..4d9d1aa0 100644
--- a/lib/igt_gt.h
+++ b/lib/igt_gt.h
@@ -81,6 +81,15 @@ extern const struct intel_execution_engine {
 	     e__++) \
 		for_if (gem_has_ring(fd__, flags__ = e__->exec_id | e__->flags))
 
+#define for_each_physical_engine(fd__, flags__) \
+	for (const struct intel_execution_engine *e__ = intel_execution_engines;\
+	     e__->name; \
+	     e__++) \
+		for_if (gem_ring_has_physical_engine(fd__, flags__ = e__->exec_id | e__->flags))
+
+bool gem_ring_is_physical_engine(int fd, unsigned int ring);
+bool gem_ring_has_physical_engine(int fd, unsigned int ring);
+
 bool gem_can_store_dword(int fd, unsigned int engine);
 
 extern const struct intel_execution_engine2 {
diff --git a/tests/amdgpu/amd_prime.c b/tests/amdgpu/amd_prime.c
index b2f326b4..bb68ccf3 100644
--- a/tests/amdgpu/amd_prime.c
+++ b/tests/amdgpu/amd_prime.c
@@ -179,12 +179,8 @@ static void i915_to_amd(int i915, int amd, amdgpu_device_handle device)
 	struct cork c;
 
 	nengine = 0;
-	for_each_engine(i915, engine) {
-		if (engine == 0)
-			continue;
-
+	for_each_physical_engine(i915, engine)
 		engines[nengine++] = engine;
-	}
 	igt_require(nengine);
 
 	memset(obj, 0, sizeof(obj));
diff --git a/tests/gem_concurrent_all.c b/tests/gem_concurrent_all.c
index 201b491b..3a1097ba 100644
--- a/tests/gem_concurrent_all.c
+++ b/tests/gem_concurrent_all.c
@@ -960,7 +960,7 @@ static igt_hang_t all_hang(void)
 	execbuf.buffers_ptr = to_user_pointer(&obj);
 	execbuf.buffer_count = 1;
 
-	for_each_engine(fd, engine) {
+	for_each_physical_engine(fd, engine) {
 		hang = igt_hang_ring(fd, engine);
 
 		execbuf.flags = engine;
diff --git a/tests/gem_ctx_create.c b/tests/gem_ctx_create.c
index 058445b6..1b32d6c3 100644
--- a/tests/gem_ctx_create.c
+++ b/tests/gem_ctx_create.c
@@ -321,11 +321,8 @@ igt_main
 		igt_require_gem(fd);
 		gem_require_contexts(fd);
 
-		for_each_engine(fd, engine) {
-			if (engine == 0)
-				continue;
+		for_each_physical_engine(fd, engine)
 			all_engines[all_nengine++] = engine;
-		}
 		igt_require(all_nengine);
 
 		if (gem_uses_full_ppgtt(fd)) {
diff --git a/tests/gem_exec_await.c b/tests/gem_exec_await.c
index e19363c4..fccc24d9 100644
--- a/tests/gem_exec_await.c
+++ b/tests/gem_exec_await.c
@@ -44,17 +44,6 @@ static double elapsed(const struct timespec *start, const struct timespec *end)
 		(end->tv_nsec - start->tv_nsec)*1e-9);
 }
 
-static bool ignore_engine(int fd, unsigned engine)
-{
-	if (engine == 0)
-		return true;
-
-	if (gem_has_bsd2(fd) && engine == I915_EXEC_BSD)
-		return true;
-
-	return false;
-}
-
 static void xchg_obj(void *array, unsigned i, unsigned j)
 {
 	struct drm_i915_gem_exec_object2 *obj = array;
@@ -89,12 +78,8 @@ static void wide(int fd, int ring_size, int timeout, unsigned int flags)
 	double time;
 
 	nengine = 0;
-	for_each_engine(fd, engine) {
-		if (ignore_engine(fd, engine))
-			continue;
-
+	for_each_physical_engine(fd, engine)
 		engines[nengine++] = engine;
-	}
 	igt_require(nengine);
 
 	exec = calloc(nengine, sizeof(*exec));
diff --git a/tests/gem_exec_create.c b/tests/gem_exec_create.c
index 28479a9d..54a2429e 100644
--- a/tests/gem_exec_create.c
+++ b/tests/gem_exec_create.c
@@ -54,17 +54,6 @@ static double elapsed(const struct timespec *start, const struct timespec *end)
 		(end->tv_nsec - start->tv_nsec)*1e-9);
 }
 
-static bool ignore_engine(int fd, unsigned engine)
-{
-	if (engine == 0)
-		return true;
-
-	if (gem_has_bsd2(fd) && engine == I915_EXEC_BSD)
-		return true;
-
-	return false;
-}
-
 #define LEAK 0x1
 
 static void all(int fd, unsigned flags, int timeout, int ncpus)
@@ -77,12 +66,8 @@ static void all(int fd, unsigned flags, int timeout, int ncpus)
 	unsigned engine;
 
 	nengine = 0;
-	for_each_engine(fd, engine) {
-		if (ignore_engine(fd, engine))
-			continue;
-
+	for_each_physical_engine(fd, engine)
 		engines[nengine++] = engine;
-	}
 	igt_require(nengine);
 
 	memset(&obj, 0, sizeof(obj));
diff --git a/tests/gem_exec_fence.c b/tests/gem_exec_fence.c
index 312505d6..39ca17ee 100644
--- a/tests/gem_exec_fence.c
+++ b/tests/gem_exec_fence.c
@@ -273,7 +273,7 @@ static void test_fence_await(int fd, unsigned ring, unsigned flags)
 	igt_assert(fence != -1);
 
 	i = 0;
-	for_each_engine(fd, engine) {
+	for_each_physical_engine(fd, engine) {
 		if (!gem_can_store_dword(fd, engine))
 			continue;
 
@@ -521,10 +521,7 @@ static void test_parallel(int fd, unsigned int master)
 	obj[BATCH].relocation_count = 1;
 
 	/* Queue all secondaries */
-	for_each_engine(fd, engine) {
-		if (engine == 0 || engine == I915_EXEC_BSD)
-			continue;
-
+	for_each_physical_engine(fd, engine) {
 		if (engine == master)
 			continue;
 
@@ -699,15 +696,8 @@ static void test_long_history(int fd, long ring_size, unsigned flags)
 		limit = ring_size / 3;
 
 	nengine = 0;
-	for_each_engine(fd, engine) {
-		if (engine == 0)
-			continue;
-
-		if (engine == I915_EXEC_BSD)
-			continue;
-
+	for_each_physical_engine(fd, engine)
 		engines[nengine++] = engine;
-	}
 	igt_require(nengine);
 
 	gem_quiescent_gpu(fd);
diff --git a/tests/gem_exec_gttfill.c b/tests/gem_exec_gttfill.c
index 96ed832f..4097e407 100644
--- a/tests/gem_exec_gttfill.c
+++ b/tests/gem_exec_gttfill.c
@@ -28,17 +28,6 @@ IGT_TEST_DESCRIPTION("Fill the GTT with batches.");
 
 #define BATCH_SIZE (4096<<10)
 
-static bool ignore_engine(int fd, unsigned engine)
-{
-	if (engine == 0)
-		return true;
-
-	if (gem_has_bsd2(fd) && engine == I915_EXEC_BSD)
-		return true;
-
-	return false;
-}
-
 static void xchg_u32(void *array, unsigned i, unsigned j)
 {
 	uint32_t *u32 = array;
@@ -126,10 +115,7 @@ static void fillgtt(int fd, unsigned ring, int timeout)
 
 	nengine = 0;
 	if (ring == 0) {
-		for_each_engine(fd, engine) {
-			if (ignore_engine(fd, engine))
-				continue;
-
+		for_each_physical_engine(fd, engine) {
 			if (!gem_can_store_dword(fd, engine))
 				continue;
 
diff --git a/tests/gem_exec_nop.c b/tests/gem_exec_nop.c
index d3e9a3e0..d971ffcb 100644
--- a/tests/gem_exec_nop.c
+++ b/tests/gem_exec_nop.c
@@ -188,17 +188,6 @@ static void headless(int fd, uint32_t handle)
 	assert_within_epsilon(n_headless, n_display, 0.1f);
 }
 
-static bool ignore_engine(int fd, unsigned engine)
-{
-	if (engine == 0)
-		return true;
-
-	if (gem_has_bsd2(fd) && engine == I915_EXEC_BSD)
-		return true;
-
-	return false;
-}
-
 static void parallel(int fd, uint32_t handle, int timeout)
 {
 	struct drm_i915_gem_execbuffer2 execbuf;
@@ -212,10 +201,7 @@ static void parallel(int fd, uint32_t handle, int timeout)
 
 	sum = 0;
 	nengine = 0;
-	for_each_engine(fd, engine) {
-		if (ignore_engine(fd, engine))
-			continue;
-
+	for_each_physical_engine(fd, engine) {
 		engines[nengine] = engine;
 		names[nengine] = e__->name;
 		nengine++;
@@ -277,10 +263,7 @@ static void series(int fd, uint32_t handle, int timeout)
 	const char *name;
 
 	nengine = 0;
-	for_each_engine(fd, engine) {
-		if (ignore_engine(fd, engine))
-			continue;
-
+	for_each_physical_engine(fd, engine) {
 		time = nop_on_ring(fd, handle, engine, 1, &count) / count;
 		if (time > max) {
 			name = e__->name;
@@ -375,12 +358,9 @@ static void sequential(int fd, uint32_t handle, unsigned flags, int timeout)
 
 	nengine = 0;
 	sum = 0;
-	for_each_engine(fd, n) {
+	for_each_physical_engine(fd, n) {
 		unsigned long count;
 
-		if (ignore_engine(fd, n))
-			continue;
-
 		time = nop_on_ring(fd, handle, n, 1, &count) / count;
 		sum += time;
 		igt_debug("%s: %.3fus\n", e__->name, 1e6*time);
@@ -509,12 +489,8 @@ static void fence_signal(int fd, uint32_t handle,
 
 	nengine = 0;
 	if (ring_id == -1) {
-		for_each_engine(fd, n) {
-			if (ignore_engine(fd, n))
-				continue;
-
+		for_each_physical_engine(fd, n)
 			engines[nengine++] = n;
-		}
 	} else {
 		gem_require_ring(fd, ring_id);
 		engines[nengine++] = ring_id;
diff --git a/tests/gem_exec_parallel.c b/tests/gem_exec_parallel.c
index 11b6e775..fe5ffe8f 100644
--- a/tests/gem_exec_parallel.c
+++ b/tests/gem_exec_parallel.c
@@ -55,17 +55,6 @@ static void check_bo(int fd, uint32_t handle, int pass)
 	munmap(map, 4096);
 }
 
-static bool ignore_engine(int fd, unsigned engine)
-{
-	if (engine == 0)
-		return true;
-
-	if (!gem_can_store_dword(fd, engine))
-		return true;
-
-	return false;
-}
-
 #define CONTEXTS 0x1
 #define FDS 0x2
 
@@ -180,8 +169,8 @@ static void all(int fd, unsigned engine, unsigned flags)
 
 	nengine = 0;
 	if (engine == -1) {
-		for_each_engine(fd, engine) {
-			if (!ignore_engine(fd, engine))
+		for_each_physical_engine(fd, engine) {
+			if (gem_can_store_dword(fd, engine))
 				engines[nengine++] = engine;
 		}
 	} else {
diff --git a/tests/gem_exec_reloc.c b/tests/gem_exec_reloc.c
index 432a42a9..213de1d7 100644
--- a/tests/gem_exec_reloc.c
+++ b/tests/gem_exec_reloc.c
@@ -258,7 +258,7 @@ static void active(int fd, unsigned engine)
 
 	nengine = 0;
 	if (engine == -1) {
-		for_each_engine(fd, engine) {
+		for_each_physical_engine(fd, engine) {
 			if (gem_can_store_dword(fd, engine))
 				engines[nengine++] = engine;
 		}
diff --git a/tests/gem_exec_schedule.c b/tests/gem_exec_schedule.c
index 5f24df33..03d457e9 100644
--- a/tests/gem_exec_schedule.c
+++ b/tests/gem_exec_schedule.c
@@ -188,17 +188,6 @@ static void fifo(int fd, unsigned ring)
 	munmap(ptr, 4096);
 }
 
-static bool ignore_engine(int fd, unsigned engine)
-{
-	if (engine == 0)
-		return true;
-
-	if (gem_has_bsd2(fd) && engine == I915_EXEC_BSD)
-		return true;
-
-	return false;
-}
-
 static void smoketest(int fd, unsigned ring, unsigned timeout)
 {
 	const int ncpus = sysconf(_SC_NPROCESSORS_ONLN);
@@ -210,12 +199,8 @@ static void smoketest(int fd, unsigned ring, unsigned timeout)
 
 	nengine = 0;
 	if (ring == -1) {
-		for_each_engine(fd, engine) {
-			if (ignore_engine(fd, engine))
-				continue;
-
+		for_each_physical_engine(fd, engine)
 			engines[nengine++] = engine;
-		}
 	} else {
 		engines[nengine++] = ring;
 	}
@@ -440,7 +425,7 @@ static void preempt_other(int fd, unsigned ring)
 	gem_context_set_priority(fd, ctx[HI], MAX_PRIO);
 
 	n = 0;
-	for_each_engine(fd, other) {
+	for_each_physical_engine(fd, other) {
 		igt_assert(n < ARRAY_SIZE(spin));
 
 		spin[n] = __igt_spin_batch_new(fd, ctx[NOISE], other, 0);
@@ -496,7 +481,7 @@ static void preempt_self(int fd, unsigned ring)
 
 	n = 0;
 	gem_context_set_priority(fd, ctx[HI], MIN_PRIO);
-	for_each_engine(fd, other) {
+	for_each_physical_engine(fd, other) {
 		spin[n] = __igt_spin_batch_new(fd, ctx[NOISE], other, 0);
 		store_dword(fd, ctx[HI], other,
 			    result, (n + 1)*sizeof(uint32_t), n + 1,
diff --git a/tests/gem_exec_store.c b/tests/gem_exec_store.c
index 31a2c096..a7673489 100644
--- a/tests/gem_exec_store.c
+++ b/tests/gem_exec_store.c
@@ -220,7 +220,7 @@ static void store_all(int fd)
 
 	nengine = 0;
 	intel_detect_and_clear_missed_interrupts(fd);
-	for_each_engine(fd, engine) {
+	for_each_physical_engine(fd, engine) {
 		if (!gem_can_store_dword(fd, engine))
 			continue;
 
diff --git a/tests/gem_exec_suspend.c b/tests/gem_exec_suspend.c
index bbdc6e55..351347cb 100644
--- a/tests/gem_exec_suspend.c
+++ b/tests/gem_exec_suspend.c
@@ -62,25 +62,13 @@ static void check_bo(int fd, uint32_t handle)
 	munmap(map, 4096);
 }
 
-static bool ignore_engine(int fd, unsigned engine)
-{
-	if (engine == 0)
-		return true;
-
-	if (!gem_can_store_dword(fd, engine))
-		return true;
-
-	return false;
-}
-
 static void test_all(int fd, unsigned flags)
 {
 	unsigned engine;
 
-	for_each_engine(fd, engine) {
-		if (!ignore_engine(fd, engine))
+	for_each_physical_engine(fd, engine)
+		if (gem_can_store_dword(fd, engine))
 			run_test(fd, engine, flags & ~0xff);
-	}
 }
 
 static bool has_semaphores(int fd)
@@ -118,8 +106,8 @@ static void run_test(int fd, unsigned engine, unsigned flags)
 		 * GPU is then unlikely to be active!)
 		 */
 		if (has_semaphores(fd)) {
-			for_each_engine(fd, engine) {
-				if (!ignore_engine(fd, engine))
+			for_each_physical_engine(fd, engine) {
+				if (gem_can_store_dword(fd, engine))
 					engines[nengine++] = engine;
 			}
 		} else {
diff --git a/tests/gem_exec_whisper.c b/tests/gem_exec_whisper.c
index 5f9fedf5..1f4dad63 100644
--- a/tests/gem_exec_whisper.c
+++ b/tests/gem_exec_whisper.c
@@ -79,17 +79,6 @@ static void verify_reloc(int fd, uint32_t handle,
 	}
 }
 
-static bool ignore_engine(int fd, unsigned engine)
-{
-	if (engine == 0)
-		return true;
-
-	if (!gem_can_store_dword(fd, engine))
-		return true;
-
-	return false;
-}
-
 #define CONTEXTS 0x1
 #define FDS 0x2
 #define INTERRUPTIBLE 0x4
@@ -217,8 +206,8 @@ static void whisper(int fd, unsigned engine, unsigned flags)
 
 	nengine = 0;
 	if (engine == -1) {
-		for_each_engine(fd, engine) {
-			if (!ignore_engine(fd, engine))
+		for_each_physical_engine(fd, engine) {
+			if (gem_can_store_dword(fd, engine))
 				engines[nengine++] = engine;
 		}
 	} else {
diff --git a/tests/gem_ring_sync_loop.c b/tests/gem_ring_sync_loop.c
index ab5bedb3..118f3638 100644
--- a/tests/gem_ring_sync_loop.c
+++ b/tests/gem_ring_sync_loop.c
@@ -48,7 +48,7 @@ sync_loop(int fd)
 	int i;
 
 	nengine = 0;
-	for_each_engine(fd, engine)
+	for_each_physical_engine(fd, engine)
 		engines[nengine++] = engine;
 	igt_require(nengine);
 
diff --git a/tests/gem_spin_batch.c b/tests/gem_spin_batch.c
index 89631130..026f9830 100644
--- a/tests/gem_spin_batch.c
+++ b/tests/gem_spin_batch.c
@@ -76,10 +76,7 @@ static void spin_on_all_engines(int fd, unsigned int timeout_sec)
 {
 	unsigned engine;
 
-	for_each_engine(fd, engine) {
-		if (engine == 0)
-			continue;
-
+	for_each_physical_engine(fd, engine) {
 		igt_fork(child, 1) {
 			igt_install_exit_handler(spin_exit_handler);
 			spin(fd, engine, timeout_sec);
diff --git a/tests/gem_sync.c b/tests/gem_sync.c
index d70515ea..175b8993 100644
--- a/tests/gem_sync.c
+++ b/tests/gem_sync.c
@@ -86,23 +86,9 @@ sync_ring(int fd, unsigned ring, int num_children, int timeout)
 	int num_engines = 0;
 
 	if (ring == ~0u) {
-		const struct intel_execution_engine *e;
-
-		for (e = intel_execution_engines; e->name; e++) {
-			if (e->exec_id == 0)
-				continue;
-
-			if (!gem_has_ring(fd, e->exec_id | e->flags))
-				continue;
-
-			if (e->exec_id == I915_EXEC_BSD) {
-				int is_bsd2 = e->flags != 0;
-				if (gem_has_bsd2(fd) != is_bsd2)
-					continue;
-			}
-
-			names[num_engines] = e->name;
-			engines[num_engines++] = e->exec_id | e->flags;
+		for_each_physical_engine(fd, ring) {
+			names[num_engines] = e__->name;
+			engines[num_engines++] = ring;
 			if (num_engines == ARRAY_SIZE(engines))
 				break;
 		}
@@ -200,26 +186,12 @@ store_ring(int fd, unsigned ring, int num_children, int timeout)
 	int num_engines = 0;
 
 	if (ring == ~0u) {
-		const struct intel_execution_engine *e;
-
-		for (e = intel_execution_engines; e->name; e++) {
-			if (e->exec_id == 0)
-				continue;
-
-			if (!gem_has_ring(fd, e->exec_id | e->flags))
+		for_each_physical_engine(fd, ring) {
+			if (!gem_can_store_dword(fd, ring))
 				continue;
 
-			if (!gem_can_store_dword(fd, e->exec_id | e->flags))
-				continue;
-
-			if (e->exec_id == I915_EXEC_BSD) {
-				int is_bsd2 = e->flags != 0;
-				if (gem_has_bsd2(fd) != is_bsd2)
-					continue;
-			}
-
-			names[num_engines] = e->name;
-			engines[num_engines++] = e->exec_id | e->flags;
+			names[num_engines] = e__->name;
+			engines[num_engines++] = ring;
 			if (num_engines == ARRAY_SIZE(engines))
 				break;
 		}
@@ -502,31 +474,17 @@ store_many(int fd, unsigned ring, int timeout)
 	intel_detect_and_clear_missed_interrupts(fd);
 
 	if (ring == ~0u) {
-		const struct intel_execution_engine *e;
-
-		for (e = intel_execution_engines; e->name; e++) {
-			if (e->exec_id == 0)
+		for_each_physical_engine(fd, ring) {
+			if (!gem_can_store_dword(fd, ring))
 				continue;
 
-			if (!gem_has_ring(fd, e->exec_id | e->flags))
-				continue;
-
-			if (!gem_can_store_dword(fd, e->exec_id | e->flags))
-				continue;
-
-			if (e->exec_id == I915_EXEC_BSD) {
-				int is_bsd2 = e->flags != 0;
-				if (gem_has_bsd2(fd) != is_bsd2)
-					continue;
-			}
-
 			igt_fork(child, 1)
 				__store_many(fd,
-					     e->exec_id | e->flags,
+					     ring,
 					     timeout,
 					     &shared[n]);
 
-			names[n++] = e->name;
+			names[n++] = e__->name;
 		}
 		igt_waitchildren();
 	} else {
@@ -612,27 +570,15 @@ static void
 store_all(int fd, int num_children, int timeout)
 {
 	const int gen = intel_gen(intel_get_drm_devid(fd));
-	const struct intel_execution_engine *e;
 	unsigned engines[16];
 	int num_engines = 0;
+	unsigned int ring;
 
-	for (e = intel_execution_engines; e->name; e++) {
-		if (e->exec_id == 0)
-			continue;
-
-		if (!gem_has_ring(fd, e->exec_id | e->flags))
-			continue;
-
-		if (!gem_can_store_dword(fd, e->exec_id))
+	for_each_physical_engine(fd, ring) {
+		if (!gem_can_store_dword(fd, ring))
 			continue;
 
-		if (e->exec_id == I915_EXEC_BSD) {
-			int is_bsd2 = e->flags != 0;
-			if (gem_has_bsd2(fd) != is_bsd2)
-				continue;
-		}
-
-		engines[num_engines++] = e->exec_id | e->flags;
+		engines[num_engines++] = ring;
 		if (num_engines == ARRAY_SIZE(engines))
 			break;
 	}
@@ -737,23 +683,9 @@ preempt(int fd, unsigned ring, int num_children, int timeout)
 	uint32_t ctx[2];
 
 	if (ring == ~0u) {
-		const struct intel_execution_engine *e;
-
-		for (e = intel_execution_engines; e->name; e++) {
-			if (e->exec_id == 0)
-				continue;
-
-			if (!gem_has_ring(fd, e->exec_id | e->flags))
-				continue;
-
-			if (e->exec_id == I915_EXEC_BSD) {
-				int is_bsd2 = e->flags != 0;
-				if (gem_has_bsd2(fd) != is_bsd2)
-					continue;
-			}
-
-			names[num_engines] = e->name;
-			engines[num_engines++] = e->exec_id | e->flags;
+		for_each_physical_engine(fd, ring) {
+			names[num_engines] = e__->name;
+			engines[num_engines++] = ring;
 			if (num_engines == ARRAY_SIZE(engines))
 				break;
 		}
-- 
2.16.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[igt-dev] [PATCH igt v2] Iterate over physical engines

[Chris Wilson]

We current have a single for_each_engine() iterator which we use to
generate both a set of uABI engines and a set of physical engines.
Determining what uABI ring-id corresponds to an actual HW engine is
tricky, so pull that out to a library function and introduce
for_each_physical_engine() for cases where we want to issue requests
once on each HW ring (avoiding aliasing issues).

v2: Remember can_store_dword for gem_sync

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 lib/igt_gt.c               |  23 ++++++++++
 lib/igt_gt.h               |   9 ++++
 tests/amdgpu/amd_prime.c   |   6 +--
 tests/gem_concurrent_all.c |   2 +-
 tests/gem_ctx_create.c     |   5 +--
 tests/gem_exec_await.c     |  17 +-------
 tests/gem_exec_create.c    |  17 +-------
 tests/gem_exec_fence.c     |  16 ++-----
 tests/gem_exec_gttfill.c   |  16 +------
 tests/gem_exec_nop.c       |  32 ++------------
 tests/gem_exec_parallel.c  |  15 +------
 tests/gem_exec_reloc.c     |   2 +-
 tests/gem_exec_schedule.c  |  21 ++-------
 tests/gem_exec_store.c     |   2 +-
 tests/gem_exec_suspend.c   |  20 ++-------
 tests/gem_exec_whisper.c   |  15 +------
 tests/gem_ring_sync_loop.c |   2 +-
 tests/gem_spin_batch.c     |   5 +--
 tests/gem_sync.c           | 104 ++++++++-------------------------------------
 19 files changed, 78 insertions(+), 251 deletions(-)

diff --git a/lib/igt_gt.c b/lib/igt_gt.c
index f70fcb92..e630550b 100644
--- a/lib/igt_gt.c
+++ b/lib/igt_gt.c
@@ -660,3 +660,26 @@ bool gem_has_engine(int gem_fd,
 			    gem_class_instance_to_eb_flags(gem_fd, class,
 							   instance));
 }
+
+bool gem_ring_is_physical_engine(int fd, unsigned ring)
+{
+	if (ring == I915_EXEC_DEFAULT)
+		return false;
+
+	/* BSD uses an extra flag to chose between aliasing modes */
+	if ((ring & 63) == I915_EXEC_BSD) {
+		bool explicit_bsd = ring & (3 << 13);
+		bool has_bsd2 = gem_has_bsd2(fd);
+		return explicit_bsd ? has_bsd2 : !has_bsd2;
+	}
+
+	return true;
+}
+
+bool gem_ring_has_physical_engine(int fd, unsigned ring)
+{
+	if (!gem_ring_is_physical_engine(fd, ring))
+		return false;
+
+	return gem_has_ring(fd, ring);
+}
diff --git a/lib/igt_gt.h b/lib/igt_gt.h
index 68592410..4d9d1aa0 100644
--- a/lib/igt_gt.h
+++ b/lib/igt_gt.h
@@ -81,6 +81,15 @@ extern const struct intel_execution_engine {
 	     e__++) \
 		for_if (gem_has_ring(fd__, flags__ = e__->exec_id | e__->flags))
 
+#define for_each_physical_engine(fd__, flags__) \
+	for (const struct intel_execution_engine *e__ = intel_execution_engines;\
+	     e__->name; \
+	     e__++) \
+		for_if (gem_ring_has_physical_engine(fd__, flags__ = e__->exec_id | e__->flags))
+
+bool gem_ring_is_physical_engine(int fd, unsigned int ring);
+bool gem_ring_has_physical_engine(int fd, unsigned int ring);
+
 bool gem_can_store_dword(int fd, unsigned int engine);
 
 extern const struct intel_execution_engine2 {
diff --git a/tests/amdgpu/amd_prime.c b/tests/amdgpu/amd_prime.c
index b2f326b4..bb68ccf3 100644
--- a/tests/amdgpu/amd_prime.c
+++ b/tests/amdgpu/amd_prime.c
@@ -179,12 +179,8 @@ static void i915_to_amd(int i915, int amd, amdgpu_device_handle device)
 	struct cork c;
 
 	nengine = 0;
-	for_each_engine(i915, engine) {
-		if (engine == 0)
-			continue;
-
+	for_each_physical_engine(i915, engine)
 		engines[nengine++] = engine;
-	}
 	igt_require(nengine);
 
 	memset(obj, 0, sizeof(obj));
diff --git a/tests/gem_concurrent_all.c b/tests/gem_concurrent_all.c
index 201b491b..3a1097ba 100644
--- a/tests/gem_concurrent_all.c
+++ b/tests/gem_concurrent_all.c
@@ -960,7 +960,7 @@ static igt_hang_t all_hang(void)
 	execbuf.buffers_ptr = to_user_pointer(&obj);
 	execbuf.buffer_count = 1;
 
-	for_each_engine(fd, engine) {
+	for_each_physical_engine(fd, engine) {
 		hang = igt_hang_ring(fd, engine);
 
 		execbuf.flags = engine;
diff --git a/tests/gem_ctx_create.c b/tests/gem_ctx_create.c
index 058445b6..1b32d6c3 100644
--- a/tests/gem_ctx_create.c
+++ b/tests/gem_ctx_create.c
@@ -321,11 +321,8 @@ igt_main
 		igt_require_gem(fd);
 		gem_require_contexts(fd);
 
-		for_each_engine(fd, engine) {
-			if (engine == 0)
-				continue;
+		for_each_physical_engine(fd, engine)
 			all_engines[all_nengine++] = engine;
-		}
 		igt_require(all_nengine);
 
 		if (gem_uses_full_ppgtt(fd)) {
diff --git a/tests/gem_exec_await.c b/tests/gem_exec_await.c
index e19363c4..fccc24d9 100644
--- a/tests/gem_exec_await.c
+++ b/tests/gem_exec_await.c
@@ -44,17 +44,6 @@ static double elapsed(const struct timespec *start, const struct timespec *end)
 		(end->tv_nsec - start->tv_nsec)*1e-9);
 }
 
-static bool ignore_engine(int fd, unsigned engine)
-{
-	if (engine == 0)
-		return true;
-
-	if (gem_has_bsd2(fd) && engine == I915_EXEC_BSD)
-		return true;
-
-	return false;
-}
-
 static void xchg_obj(void *array, unsigned i, unsigned j)
 {
 	struct drm_i915_gem_exec_object2 *obj = array;
@@ -89,12 +78,8 @@ static void wide(int fd, int ring_size, int timeout, unsigned int flags)
 	double time;
 
 	nengine = 0;
-	for_each_engine(fd, engine) {
-		if (ignore_engine(fd, engine))
-			continue;
-
+	for_each_physical_engine(fd, engine)
 		engines[nengine++] = engine;
-	}
 	igt_require(nengine);
 
 	exec = calloc(nengine, sizeof(*exec));
diff --git a/tests/gem_exec_create.c b/tests/gem_exec_create.c
index 28479a9d..54a2429e 100644
--- a/tests/gem_exec_create.c
+++ b/tests/gem_exec_create.c
@@ -54,17 +54,6 @@ static double elapsed(const struct timespec *start, const struct timespec *end)
 		(end->tv_nsec - start->tv_nsec)*1e-9);
 }
 
-static bool ignore_engine(int fd, unsigned engine)
-{
-	if (engine == 0)
-		return true;
-
-	if (gem_has_bsd2(fd) && engine == I915_EXEC_BSD)
-		return true;
-
-	return false;
-}
-
 #define LEAK 0x1
 
 static void all(int fd, unsigned flags, int timeout, int ncpus)
@@ -77,12 +66,8 @@ static void all(int fd, unsigned flags, int timeout, int ncpus)
 	unsigned engine;
 
 	nengine = 0;
-	for_each_engine(fd, engine) {
-		if (ignore_engine(fd, engine))
-			continue;
-
+	for_each_physical_engine(fd, engine)
 		engines[nengine++] = engine;
-	}
 	igt_require(nengine);
 
 	memset(&obj, 0, sizeof(obj));
diff --git a/tests/gem_exec_fence.c b/tests/gem_exec_fence.c
index 312505d6..39ca17ee 100644
--- a/tests/gem_exec_fence.c
+++ b/tests/gem_exec_fence.c
@@ -273,7 +273,7 @@ static void test_fence_await(int fd, unsigned ring, unsigned flags)
 	igt_assert(fence != -1);
 
 	i = 0;
-	for_each_engine(fd, engine) {
+	for_each_physical_engine(fd, engine) {
 		if (!gem_can_store_dword(fd, engine))
 			continue;
 
@@ -521,10 +521,7 @@ static void test_parallel(int fd, unsigned int master)
 	obj[BATCH].relocation_count = 1;
 
 	/* Queue all secondaries */
-	for_each_engine(fd, engine) {
-		if (engine == 0 || engine == I915_EXEC_BSD)
-			continue;
-
+	for_each_physical_engine(fd, engine) {
 		if (engine == master)
 			continue;
 
@@ -699,15 +696,8 @@ static void test_long_history(int fd, long ring_size, unsigned flags)
 		limit = ring_size / 3;
 
 	nengine = 0;
-	for_each_engine(fd, engine) {
-		if (engine == 0)
-			continue;
-
-		if (engine == I915_EXEC_BSD)
-			continue;
-
+	for_each_physical_engine(fd, engine)
 		engines[nengine++] = engine;
-	}
 	igt_require(nengine);
 
 	gem_quiescent_gpu(fd);
diff --git a/tests/gem_exec_gttfill.c b/tests/gem_exec_gttfill.c
index 96ed832f..4097e407 100644
--- a/tests/gem_exec_gttfill.c
+++ b/tests/gem_exec_gttfill.c
@@ -28,17 +28,6 @@ IGT_TEST_DESCRIPTION("Fill the GTT with batches.");
 
 #define BATCH_SIZE (4096<<10)
 
-static bool ignore_engine(int fd, unsigned engine)
-{
-	if (engine == 0)
-		return true;
-
-	if (gem_has_bsd2(fd) && engine == I915_EXEC_BSD)
-		return true;
-
-	return false;
-}
-
 static void xchg_u32(void *array, unsigned i, unsigned j)
 {
 	uint32_t *u32 = array;
@@ -126,10 +115,7 @@ static void fillgtt(int fd, unsigned ring, int timeout)
 
 	nengine = 0;
 	if (ring == 0) {
-		for_each_engine(fd, engine) {
-			if (ignore_engine(fd, engine))
-				continue;
-
+		for_each_physical_engine(fd, engine) {
 			if (!gem_can_store_dword(fd, engine))
 				continue;
 
diff --git a/tests/gem_exec_nop.c b/tests/gem_exec_nop.c
index d3e9a3e0..d971ffcb 100644
--- a/tests/gem_exec_nop.c
+++ b/tests/gem_exec_nop.c
@@ -188,17 +188,6 @@ static void headless(int fd, uint32_t handle)
 	assert_within_epsilon(n_headless, n_display, 0.1f);
 }
 
-static bool ignore_engine(int fd, unsigned engine)
-{
-	if (engine == 0)
-		return true;
-
-	if (gem_has_bsd2(fd) && engine == I915_EXEC_BSD)
-		return true;
-
-	return false;
-}
-
 static void parallel(int fd, uint32_t handle, int timeout)
 {
 	struct drm_i915_gem_execbuffer2 execbuf;
@@ -212,10 +201,7 @@ static void parallel(int fd, uint32_t handle, int timeout)
 
 	sum = 0;
 	nengine = 0;
-	for_each_engine(fd, engine) {
-		if (ignore_engine(fd, engine))
-			continue;
-
+	for_each_physical_engine(fd, engine) {
 		engines[nengine] = engine;
 		names[nengine] = e__->name;
 		nengine++;
@@ -277,10 +263,7 @@ static void series(int fd, uint32_t handle, int timeout)
 	const char *name;
 
 	nengine = 0;
-	for_each_engine(fd, engine) {
-		if (ignore_engine(fd, engine))
-			continue;
-
+	for_each_physical_engine(fd, engine) {
 		time = nop_on_ring(fd, handle, engine, 1, &count) / count;
 		if (time > max) {
 			name = e__->name;
@@ -375,12 +358,9 @@ static void sequential(int fd, uint32_t handle, unsigned flags, int timeout)
 
 	nengine = 0;
 	sum = 0;
-	for_each_engine(fd, n) {
+	for_each_physical_engine(fd, n) {
 		unsigned long count;
 
-		if (ignore_engine(fd, n))
-			continue;
-
 		time = nop_on_ring(fd, handle, n, 1, &count) / count;
 		sum += time;
 		igt_debug("%s: %.3fus\n", e__->name, 1e6*time);
@@ -509,12 +489,8 @@ static void fence_signal(int fd, uint32_t handle,
 
 	nengine = 0;
 	if (ring_id == -1) {
-		for_each_engine(fd, n) {
-			if (ignore_engine(fd, n))
-				continue;
-
+		for_each_physical_engine(fd, n)
 			engines[nengine++] = n;
-		}
 	} else {
 		gem_require_ring(fd, ring_id);
 		engines[nengine++] = ring_id;
diff --git a/tests/gem_exec_parallel.c b/tests/gem_exec_parallel.c
index 11b6e775..fe5ffe8f 100644
--- a/tests/gem_exec_parallel.c
+++ b/tests/gem_exec_parallel.c
@@ -55,17 +55,6 @@ static void check_bo(int fd, uint32_t handle, int pass)
 	munmap(map, 4096);
 }
 
-static bool ignore_engine(int fd, unsigned engine)
-{
-	if (engine == 0)
-		return true;
-
-	if (!gem_can_store_dword(fd, engine))
-		return true;
-
-	return false;
-}
-
 #define CONTEXTS 0x1
 #define FDS 0x2
 
@@ -180,8 +169,8 @@ static void all(int fd, unsigned engine, unsigned flags)
 
 	nengine = 0;
 	if (engine == -1) {
-		for_each_engine(fd, engine) {
-			if (!ignore_engine(fd, engine))
+		for_each_physical_engine(fd, engine) {
+			if (gem_can_store_dword(fd, engine))
 				engines[nengine++] = engine;
 		}
 	} else {
diff --git a/tests/gem_exec_reloc.c b/tests/gem_exec_reloc.c
index 432a42a9..213de1d7 100644
--- a/tests/gem_exec_reloc.c
+++ b/tests/gem_exec_reloc.c
@@ -258,7 +258,7 @@ static void active(int fd, unsigned engine)
 
 	nengine = 0;
 	if (engine == -1) {
-		for_each_engine(fd, engine) {
+		for_each_physical_engine(fd, engine) {
 			if (gem_can_store_dword(fd, engine))
 				engines[nengine++] = engine;
 		}
diff --git a/tests/gem_exec_schedule.c b/tests/gem_exec_schedule.c
index 5f24df33..03d457e9 100644
--- a/tests/gem_exec_schedule.c
+++ b/tests/gem_exec_schedule.c
@@ -188,17 +188,6 @@ static void fifo(int fd, unsigned ring)
 	munmap(ptr, 4096);
 }
 
-static bool ignore_engine(int fd, unsigned engine)
-{
-	if (engine == 0)
-		return true;
-
-	if (gem_has_bsd2(fd) && engine == I915_EXEC_BSD)
-		return true;
-
-	return false;
-}
-
 static void smoketest(int fd, unsigned ring, unsigned timeout)
 {
 	const int ncpus = sysconf(_SC_NPROCESSORS_ONLN);
@@ -210,12 +199,8 @@ static void smoketest(int fd, unsigned ring, unsigned timeout)
 
 	nengine = 0;
 	if (ring == -1) {
-		for_each_engine(fd, engine) {
-			if (ignore_engine(fd, engine))
-				continue;
-
+		for_each_physical_engine(fd, engine)
 			engines[nengine++] = engine;
-		}
 	} else {
 		engines[nengine++] = ring;
 	}
@@ -440,7 +425,7 @@ static void preempt_other(int fd, unsigned ring)
 	gem_context_set_priority(fd, ctx[HI], MAX_PRIO);
 
 	n = 0;
-	for_each_engine(fd, other) {
+	for_each_physical_engine(fd, other) {
 		igt_assert(n < ARRAY_SIZE(spin));
 
 		spin[n] = __igt_spin_batch_new(fd, ctx[NOISE], other, 0);
@@ -496,7 +481,7 @@ static void preempt_self(int fd, unsigned ring)
 
 	n = 0;
 	gem_context_set_priority(fd, ctx[HI], MIN_PRIO);
-	for_each_engine(fd, other) {
+	for_each_physical_engine(fd, other) {
 		spin[n] = __igt_spin_batch_new(fd, ctx[NOISE], other, 0);
 		store_dword(fd, ctx[HI], other,
 			    result, (n + 1)*sizeof(uint32_t), n + 1,
diff --git a/tests/gem_exec_store.c b/tests/gem_exec_store.c
index 31a2c096..a7673489 100644
--- a/tests/gem_exec_store.c
+++ b/tests/gem_exec_store.c
@@ -220,7 +220,7 @@ static void store_all(int fd)
 
 	nengine = 0;
 	intel_detect_and_clear_missed_interrupts(fd);
-	for_each_engine(fd, engine) {
+	for_each_physical_engine(fd, engine) {
 		if (!gem_can_store_dword(fd, engine))
 			continue;
 
diff --git a/tests/gem_exec_suspend.c b/tests/gem_exec_suspend.c
index bbdc6e55..351347cb 100644
--- a/tests/gem_exec_suspend.c
+++ b/tests/gem_exec_suspend.c
@@ -62,25 +62,13 @@ static void check_bo(int fd, uint32_t handle)
 	munmap(map, 4096);
 }
 
-static bool ignore_engine(int fd, unsigned engine)
-{
-	if (engine == 0)
-		return true;
-
-	if (!gem_can_store_dword(fd, engine))
-		return true;
-
-	return false;
-}
-
 static void test_all(int fd, unsigned flags)
 {
 	unsigned engine;
 
-	for_each_engine(fd, engine) {
-		if (!ignore_engine(fd, engine))
+	for_each_physical_engine(fd, engine)
+		if (gem_can_store_dword(fd, engine))
 			run_test(fd, engine, flags & ~0xff);
-	}
 }
 
 static bool has_semaphores(int fd)
@@ -118,8 +106,8 @@ static void run_test(int fd, unsigned engine, unsigned flags)
 		 * GPU is then unlikely to be active!)
 		 */
 		if (has_semaphores(fd)) {
-			for_each_engine(fd, engine) {
-				if (!ignore_engine(fd, engine))
+			for_each_physical_engine(fd, engine) {
+				if (gem_can_store_dword(fd, engine))
 					engines[nengine++] = engine;
 			}
 		} else {
diff --git a/tests/gem_exec_whisper.c b/tests/gem_exec_whisper.c
index 5f9fedf5..1f4dad63 100644
--- a/tests/gem_exec_whisper.c
+++ b/tests/gem_exec_whisper.c
@@ -79,17 +79,6 @@ static void verify_reloc(int fd, uint32_t handle,
 	}
 }
 
-static bool ignore_engine(int fd, unsigned engine)
-{
-	if (engine == 0)
-		return true;
-
-	if (!gem_can_store_dword(fd, engine))
-		return true;
-
-	return false;
-}
-
 #define CONTEXTS 0x1
 #define FDS 0x2
 #define INTERRUPTIBLE 0x4
@@ -217,8 +206,8 @@ static void whisper(int fd, unsigned engine, unsigned flags)
 
 	nengine = 0;
 	if (engine == -1) {
-		for_each_engine(fd, engine) {
-			if (!ignore_engine(fd, engine))
+		for_each_physical_engine(fd, engine) {
+			if (gem_can_store_dword(fd, engine))
 				engines[nengine++] = engine;
 		}
 	} else {
diff --git a/tests/gem_ring_sync_loop.c b/tests/gem_ring_sync_loop.c
index ab5bedb3..118f3638 100644
--- a/tests/gem_ring_sync_loop.c
+++ b/tests/gem_ring_sync_loop.c
@@ -48,7 +48,7 @@ sync_loop(int fd)
 	int i;
 
 	nengine = 0;
-	for_each_engine(fd, engine)
+	for_each_physical_engine(fd, engine)
 		engines[nengine++] = engine;
 	igt_require(nengine);
 
diff --git a/tests/gem_spin_batch.c b/tests/gem_spin_batch.c
index 89631130..026f9830 100644
--- a/tests/gem_spin_batch.c
+++ b/tests/gem_spin_batch.c
@@ -76,10 +76,7 @@ static void spin_on_all_engines(int fd, unsigned int timeout_sec)
 {
 	unsigned engine;
 
-	for_each_engine(fd, engine) {
-		if (engine == 0)
-			continue;
-
+	for_each_physical_engine(fd, engine) {
 		igt_fork(child, 1) {
 			igt_install_exit_handler(spin_exit_handler);
 			spin(fd, engine, timeout_sec);
diff --git a/tests/gem_sync.c b/tests/gem_sync.c
index d70515ea..175b8993 100644
--- a/tests/gem_sync.c
+++ b/tests/gem_sync.c
@@ -86,23 +86,9 @@ sync_ring(int fd, unsigned ring, int num_children, int timeout)
 	int num_engines = 0;
 
 	if (ring == ~0u) {
-		const struct intel_execution_engine *e;
-
-		for (e = intel_execution_engines; e->name; e++) {
-			if (e->exec_id == 0)
-				continue;
-
-			if (!gem_has_ring(fd, e->exec_id | e->flags))
-				continue;
-
-			if (e->exec_id == I915_EXEC_BSD) {
-				int is_bsd2 = e->flags != 0;
-				if (gem_has_bsd2(fd) != is_bsd2)
-					continue;
-			}
-
-			names[num_engines] = e->name;
-			engines[num_engines++] = e->exec_id | e->flags;
+		for_each_physical_engine(fd, ring) {
+			names[num_engines] = e__->name;
+			engines[num_engines++] = ring;
 			if (num_engines == ARRAY_SIZE(engines))
 				break;
 		}
@@ -200,26 +186,12 @@ store_ring(int fd, unsigned ring, int num_children, int timeout)
 	int num_engines = 0;
 
 	if (ring == ~0u) {
-		const struct intel_execution_engine *e;
-
-		for (e = intel_execution_engines; e->name; e++) {
-			if (e->exec_id == 0)
-				continue;
-
-			if (!gem_has_ring(fd, e->exec_id | e->flags))
+		for_each_physical_engine(fd, ring) {
+			if (!gem_can_store_dword(fd, ring))
 				continue;
 
-			if (!gem_can_store_dword(fd, e->exec_id | e->flags))
-				continue;
-
-			if (e->exec_id == I915_EXEC_BSD) {
-				int is_bsd2 = e->flags != 0;
-				if (gem_has_bsd2(fd) != is_bsd2)
-					continue;
-			}
-
-			names[num_engines] = e->name;
-			engines[num_engines++] = e->exec_id | e->flags;
+			names[num_engines] = e__->name;
+			engines[num_engines++] = ring;
 			if (num_engines == ARRAY_SIZE(engines))
 				break;
 		}
@@ -502,31 +474,17 @@ store_many(int fd, unsigned ring, int timeout)
 	intel_detect_and_clear_missed_interrupts(fd);
 
 	if (ring == ~0u) {
-		const struct intel_execution_engine *e;
-
-		for (e = intel_execution_engines; e->name; e++) {
-			if (e->exec_id == 0)
+		for_each_physical_engine(fd, ring) {
+			if (!gem_can_store_dword(fd, ring))
 				continue;
 
-			if (!gem_has_ring(fd, e->exec_id | e->flags))
-				continue;
-
-			if (!gem_can_store_dword(fd, e->exec_id | e->flags))
-				continue;
-
-			if (e->exec_id == I915_EXEC_BSD) {
-				int is_bsd2 = e->flags != 0;
-				if (gem_has_bsd2(fd) != is_bsd2)
-					continue;
-			}
-
 			igt_fork(child, 1)
 				__store_many(fd,
-					     e->exec_id | e->flags,
+					     ring,
 					     timeout,
 					     &shared[n]);
 
-			names[n++] = e->name;
+			names[n++] = e__->name;
 		}
 		igt_waitchildren();
 	} else {
@@ -612,27 +570,15 @@ static void
 store_all(int fd, int num_children, int timeout)
 {
 	const int gen = intel_gen(intel_get_drm_devid(fd));
-	const struct intel_execution_engine *e;
 	unsigned engines[16];
 	int num_engines = 0;
+	unsigned int ring;
 
-	for (e = intel_execution_engines; e->name; e++) {
-		if (e->exec_id == 0)
-			continue;
-
-		if (!gem_has_ring(fd, e->exec_id | e->flags))
-			continue;
-
-		if (!gem_can_store_dword(fd, e->exec_id))
+	for_each_physical_engine(fd, ring) {
+		if (!gem_can_store_dword(fd, ring))
 			continue;
 
-		if (e->exec_id == I915_EXEC_BSD) {
-			int is_bsd2 = e->flags != 0;
-			if (gem_has_bsd2(fd) != is_bsd2)
-				continue;
-		}
-
-		engines[num_engines++] = e->exec_id | e->flags;
+		engines[num_engines++] = ring;
 		if (num_engines == ARRAY_SIZE(engines))
 			break;
 	}
@@ -737,23 +683,9 @@ preempt(int fd, unsigned ring, int num_children, int timeout)
 	uint32_t ctx[2];
 
 	if (ring == ~0u) {
-		const struct intel_execution_engine *e;
-
-		for (e = intel_execution_engines; e->name; e++) {
-			if (e->exec_id == 0)
-				continue;
-
-			if (!gem_has_ring(fd, e->exec_id | e->flags))
-				continue;
-
-			if (e->exec_id == I915_EXEC_BSD) {
-				int is_bsd2 = e->flags != 0;
-				if (gem_has_bsd2(fd) != is_bsd2)
-					continue;
-			}
-
-			names[num_engines] = e->name;
-			engines[num_engines++] = e->exec_id | e->flags;
+		for_each_physical_engine(fd, ring) {
+			names[num_engines] = e__->name;
+			engines[num_engines++] = ring;
 			if (num_engines == ARRAY_SIZE(engines))
 				break;
 		}
-- 
2.16.1

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

[igt-dev] ✓ Fi.CI.BAT: success for series starting with [v2] Iterate over physical engines (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [v2] Iterate over physical engines (rev2)
URL   : https://patchwork.freedesktop.org/series/38704/
State : success

== Summary ==

IGT patchset tested on top of latest successful build
960e55a87d7b7d7385063e37cc9f281df2be8037 igt/gem_ctx_isolation: Check isolation of registers between contexts

with latest DRM-Tip kernel build CI_DRM_3819
42016703e66b drm-tip: 2018y-02m-21d-21h-26m-53s UTC integration manifest

Testlist changes:
+igt@gem_spin_batch@basic-default

Test debugfs_test:
        Subgroup read_all_entries:
                pass       -> INCOMPLETE (fi-snb-2520m) fdo#103713
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-c:
                incomplete -> PASS       (fi-bxt-dsi) fdo#103927

fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:417s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:426s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:375s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:490s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:287s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:480s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:485s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:470s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:456s
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:562s
fi-elk-e7500     total:288  pass:229  dwarn:0   dfail:0   fail:0   skip:59  time:411s
fi-gdg-551       total:288  pass:179  dwarn:0   dfail:0   fail:1   skip:108 time:284s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:510s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:390s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:409s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:455s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:411s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:453s
fi-kbl-7560u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:494s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:450s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:494s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:589s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:425s
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:500s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:517s
fi-skl-6700k2    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:487s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:475s
fi-skl-guc       total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:405s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:430s
fi-snb-2520m     total:3    pass:2    dwarn:0   dfail:0   fail:0   skip:0  
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:394s

== Logs ==

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

[igt-dev] ✗ Fi.CI.IGT: failure for series starting with [v2] Iterate over physical engines (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [v2] Iterate over physical engines (rev2)
URL   : https://patchwork.freedesktop.org/series/38704/
State : failure

== Summary ==

Test kms_flip:
        Subgroup modeset-vs-vblank-race:
                fail       -> PASS       (shard-hsw) fdo#103060 +1
Test gem_exec_schedule:
        Subgroup preempt-self-vebox:
                pass       -> FAIL       (shard-apl)
        Subgroup preempt-other-blt:
                pass       -> FAIL       (shard-apl)
        Subgroup preempt-self-blt:
                pass       -> FAIL       (shard-apl)
        Subgroup preempt-other-render:
                pass       -> FAIL       (shard-apl)
        Subgroup preempt-self-bsd:
                pass       -> FAIL       (shard-apl)
        Subgroup preempt-other-vebox:
                pass       -> FAIL       (shard-apl)
        Subgroup preempt-other-bsd:
                pass       -> FAIL       (shard-apl)
        Subgroup preempt-self-render:
                pass       -> FAIL       (shard-apl)
Test kms_chv_cursor_fail:
        Subgroup pipe-b-128x128-left-edge:
                pass       -> DMESG-WARN (shard-snb)
        Subgroup pipe-b-128x128-top-edge:
                dmesg-warn -> PASS       (shard-snb)
Test kms_sysfs_edid_timing:
                pass       -> WARN       (shard-apl) fdo#100047
Test kms_rotation_crc:
        Subgroup sprite-rotation-180:
                fail       -> PASS       (shard-snb) fdo#103925

fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060
fdo#100047 https://bugs.freedesktop.org/show_bug.cgi?id=100047
fdo#103925 https://bugs.freedesktop.org/show_bug.cgi?id=103925

shard-apl        total:3326 pass:1745 dwarn:1   dfail:0   fail:20  skip:1557 time:12069s
shard-hsw        total:3465 pass:1768 dwarn:1   dfail:0   fail:2   skip:1693 time:11692s
shard-snb        total:3465 pass:1357 dwarn:2   dfail:0   fail:2   skip:2104 time:6728s
Blacklisted hosts:
shard-kbl        total:3444 pass:1937 dwarn:1   dfail:0   fail:26  skip:1479 time:9377s

== Logs ==

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

[PATCH igt v3] Iterate over physical engines

[Chris Wilson]

We current have a single for_each_engine() iterator which we use to
generate both a set of uABI engines and a set of physical engines.
Determining what uABI ring-id corresponds to an actual HW engine is
tricky, so pull that out to a library function and introduce
for_each_physical_engine() for cases where we want to issue requests
once on each HW ring (avoiding aliasing issues).

v2: Remember can_store_dword for gem_sync
v3: Find more open-coded for_each_physical

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 lib/igt_gt.c               |  23 +++++++++
 lib/igt_gt.h               |   9 ++++
 tests/amdgpu/amd_prime.c   |   6 +--
 tests/drv_hangman.c        |  20 ++------
 tests/gem_busy.c           |  31 ++++++------
 tests/gem_concurrent_all.c |   2 +-
 tests/gem_ctx_create.c     |   5 +-
 tests/gem_ctx_thrash.c     |  37 +++-----------
 tests/gem_exec_async.c     |  15 +++---
 tests/gem_exec_await.c     |  17 +------
 tests/gem_exec_capture.c   |   2 +-
 tests/gem_exec_create.c    |  17 +------
 tests/gem_exec_fence.c     |  16 ++----
 tests/gem_exec_gttfill.c   |  16 +-----
 tests/gem_exec_latency.c   |  19 +++----
 tests/gem_exec_nop.c       |  32 ++----------
 tests/gem_exec_parallel.c  |  15 +-----
 tests/gem_exec_reloc.c     |   2 +-
 tests/gem_exec_schedule.c  |  31 ++++--------
 tests/gem_exec_store.c     |   2 +-
 tests/gem_exec_suspend.c   |  20 ++------
 tests/gem_exec_whisper.c   |  15 +-----
 tests/gem_ring_sync_loop.c |   2 +-
 tests/gem_spin_batch.c     |   5 +-
 tests/gem_sync.c           | 123 ++++++++-------------------------------------
 25 files changed, 124 insertions(+), 358 deletions(-)

diff --git a/lib/igt_gt.c b/lib/igt_gt.c
index f70fcb92..e630550b 100644
--- a/lib/igt_gt.c
+++ b/lib/igt_gt.c
@@ -660,3 +660,26 @@ bool gem_has_engine(int gem_fd,
 			    gem_class_instance_to_eb_flags(gem_fd, class,
 							   instance));
 }
+
+bool gem_ring_is_physical_engine(int fd, unsigned ring)
+{
+	if (ring == I915_EXEC_DEFAULT)
+		return false;
+
+	/* BSD uses an extra flag to chose between aliasing modes */
+	if ((ring & 63) == I915_EXEC_BSD) {
+		bool explicit_bsd = ring & (3 << 13);
+		bool has_bsd2 = gem_has_bsd2(fd);
+		return explicit_bsd ? has_bsd2 : !has_bsd2;
+	}
+
+	return true;
+}
+
+bool gem_ring_has_physical_engine(int fd, unsigned ring)
+{
+	if (!gem_ring_is_physical_engine(fd, ring))
+		return false;
+
+	return gem_has_ring(fd, ring);
+}
diff --git a/lib/igt_gt.h b/lib/igt_gt.h
index 68592410..4d9d1aa0 100644
--- a/lib/igt_gt.h
+++ b/lib/igt_gt.h
@@ -81,6 +81,15 @@ extern const struct intel_execution_engine {
 	     e__++) \
 		for_if (gem_has_ring(fd__, flags__ = e__->exec_id | e__->flags))
 
+#define for_each_physical_engine(fd__, flags__) \
+	for (const struct intel_execution_engine *e__ = intel_execution_engines;\
+	     e__->name; \
+	     e__++) \
+		for_if (gem_ring_has_physical_engine(fd__, flags__ = e__->exec_id | e__->flags))
+
+bool gem_ring_is_physical_engine(int fd, unsigned int ring);
+bool gem_ring_has_physical_engine(int fd, unsigned int ring);
+
 bool gem_can_store_dword(int fd, unsigned int engine);
 
 extern const struct intel_execution_engine2 {
diff --git a/tests/amdgpu/amd_prime.c b/tests/amdgpu/amd_prime.c
index b2f326b4..bb68ccf3 100644
--- a/tests/amdgpu/amd_prime.c
+++ b/tests/amdgpu/amd_prime.c
@@ -179,12 +179,8 @@ static void i915_to_amd(int i915, int amd, amdgpu_device_handle device)
 	struct cork c;
 
 	nengine = 0;
-	for_each_engine(i915, engine) {
-		if (engine == 0)
-			continue;
-
+	for_each_physical_engine(i915, engine)
 		engines[nengine++] = engine;
-	}
 	igt_require(nengine);
 
 	memset(obj, 0, sizeof(obj));
diff --git a/tests/drv_hangman.c b/tests/drv_hangman.c
index 40c82257..38cb20c3 100644
--- a/tests/drv_hangman.c
+++ b/tests/drv_hangman.c
@@ -183,8 +183,6 @@ static void test_error_state_capture(unsigned ring_id,
 	igt_hang_t hang;
 	uint64_t offset;
 
-	igt_require(gem_has_ring(device, ring_id));
-
 	clear_error_state();
 
 	hang = igt_hang_ctx(device, 0, ring_id, HANG_ALLOW_CAPTURE, &offset);
@@ -255,23 +253,11 @@ igt_main
 		if (e->exec_id == 0)
 			continue;
 
-		/*
-		 * If the device has 2 BSD rings then due to obtuse aliasing
-		 * in the API, we can not determine which ring I915_EXEC_BSD
-		 * will map to, and so must skip the test; as the matching name
-		 * may be either bsd or bsd2 depending on the kernel/test
-		 * ordering.
-		 *
-		 * Here we are not checking that executing on every ABI engine
-		 * results in a detectable hang, but that a hang generated
-		 * from a specific HW engine gives an indentifiable result.
-		 */
-		if (e->exec_id == I915_EXEC_BSD && e->flags == 0)
-			continue;
-
-		igt_subtest_f("error-state-capture-%s", e->name)
+		igt_subtest_f("error-state-capture-%s", e->name) {
+			igt_require(gem_ring_has_physical_engine(device, e->exec_id | e->flags));
 			test_error_state_capture(e->exec_id | e->flags,
 						 e->full_name);
+		}
 	}
 
 	igt_subtest("hangcheck-unterminated")
diff --git a/tests/gem_busy.c b/tests/gem_busy.c
index c349c291..49cb1c95 100644
--- a/tests/gem_busy.c
+++ b/tests/gem_busy.c
@@ -156,7 +156,7 @@ static void semaphore(int fd, unsigned ring, uint32_t flags)
 
 #define PARALLEL 1
 #define HANG 2
-static void one(int fd, unsigned ring, uint32_t flags, unsigned test_flags)
+static void one(int fd, unsigned ring, unsigned test_flags)
 {
 	const int gen = intel_gen(intel_get_drm_devid(fd));
 	struct drm_i915_gem_exec_object2 obj[2];
@@ -173,7 +173,7 @@ static void one(int fd, unsigned ring, uint32_t flags, unsigned test_flags)
 	memset(&execbuf, 0, sizeof(execbuf));
 	execbuf.buffers_ptr = to_user_pointer(obj);
 	execbuf.buffer_count = 2;
-	execbuf.flags = ring | flags;
+	execbuf.flags = ring;
 	if (gen < 6)
 		execbuf.flags |= I915_EXEC_SECURE;
 
@@ -245,20 +245,17 @@ static void one(int fd, unsigned ring, uint32_t flags, unsigned test_flags)
 	__gem_busy(fd, obj[BATCH].handle, &read[BATCH], &write[BATCH]);
 
 	if (test_flags & PARALLEL) {
-		const struct intel_execution_engine *e;
-
-		for (e = intel_execution_engines; e->name; e++) {
-			if (e->exec_id == 0 || e->exec_id == ring)
-				continue;
+		unsigned other;
 
-			if (!gem_has_ring(fd, e->exec_id | e->flags))
+		for_each_physical_engine(fd, other) {
+			if (other == ring)
 				continue;
 
-			if (!gem_can_store_dword(fd, e->exec_id | e->flags))
+			if (!gem_can_store_dword(fd, other))
 				continue;
 
-			igt_debug("Testing %s in parallel\n", e->name);
-			one(fd, e->exec_id, e->flags, 0);
+			igt_debug("Testing %s in parallel\n", e__->name);
+			one(fd, other, 0);
 		}
 	}
 
@@ -591,10 +588,10 @@ igt_main
 					continue;
 
 				igt_subtest_f("extended-%s", e->name) {
-					gem_require_ring(fd, e->exec_id | e->flags);
+					igt_require(gem_ring_has_physical_engine(fd, e->exec_id | e->flags));
 					igt_require(gem_can_store_dword(fd, e->exec_id | e->flags));
 					gem_quiescent_gpu(fd);
-					one(fd, e->exec_id, e->flags, 0);
+					one(fd, e->exec_id | e->flags, 0);
 					gem_quiescent_gpu(fd);
 				}
 			}
@@ -605,11 +602,11 @@ igt_main
 					continue;
 
 				igt_subtest_f("extended-parallel-%s", e->name) {
-					gem_require_ring(fd, e->exec_id | e->flags);
+					igt_require(gem_ring_has_physical_engine(fd, e->exec_id | e->flags));
 					igt_require(gem_can_store_dword(fd, e->exec_id | e->flags));
 
 					gem_quiescent_gpu(fd);
-					one(fd, e->exec_id, e->flags, PARALLEL);
+					one(fd, e->exec_id | e->flags, PARALLEL);
 					gem_quiescent_gpu(fd);
 				}
 			}
@@ -670,11 +667,11 @@ igt_main
 
 				igt_subtest_f("extended-hang-%s", e->name) {
 					igt_skip_on_simulation();
-					gem_require_ring(fd, e->exec_id | e->flags);
+					igt_require(gem_ring_has_physical_engine(fd, e->exec_id | e->flags));
 					igt_require(gem_can_store_dword(fd, e->exec_id | e->flags));
 
 					gem_quiescent_gpu(fd);
-					one(fd, e->exec_id, e->flags, HANG);
+					one(fd, e->exec_id | e->flags, HANG);
 					gem_quiescent_gpu(fd);
 				}
 			}
diff --git a/tests/gem_concurrent_all.c b/tests/gem_concurrent_all.c
index 201b491b..3a1097ba 100644
--- a/tests/gem_concurrent_all.c
+++ b/tests/gem_concurrent_all.c
@@ -960,7 +960,7 @@ static igt_hang_t all_hang(void)
 	execbuf.buffers_ptr = to_user_pointer(&obj);
 	execbuf.buffer_count = 1;
 
-	for_each_engine(fd, engine) {
+	for_each_physical_engine(fd, engine) {
 		hang = igt_hang_ring(fd, engine);
 
 		execbuf.flags = engine;
diff --git a/tests/gem_ctx_create.c b/tests/gem_ctx_create.c
index 058445b6..1b32d6c3 100644
--- a/tests/gem_ctx_create.c
+++ b/tests/gem_ctx_create.c
@@ -321,11 +321,8 @@ igt_main
 		igt_require_gem(fd);
 		gem_require_contexts(fd);
 
-		for_each_engine(fd, engine) {
-			if (engine == 0)
-				continue;
+		for_each_physical_engine(fd, engine)
 			all_engines[all_nengine++] = engine;
-		}
 		igt_require(all_nengine);
 
 		if (gem_uses_full_ppgtt(fd)) {
diff --git a/tests/gem_ctx_thrash.c b/tests/gem_ctx_thrash.c
index b5d334f4..23203158 100644
--- a/tests/gem_ctx_thrash.c
+++ b/tests/gem_ctx_thrash.c
@@ -130,22 +130,9 @@ static void single(const char *name, bool all_engines)
 
 	num_engines = 0;
 	if (all_engines) {
-		for (e = intel_execution_engines; e->name; e++) {
-			if (e->exec_id == 0)
-				continue;
-
-			if (has_engine(fd, e, 0))
-				continue;
-
-			if (e->exec_id == I915_EXEC_BSD) {
-				int is_bsd2 = e->flags != 0;
-				if (gem_has_bsd2(fd) != is_bsd2)
-					continue;
-			}
-
-			igt_require(has_engine(fd, e, 1) == -ENOENT);
-
-			engines[num_engines++] = e->exec_id | e->flags;
+		unsigned engine;
+		for_each_physical_engine(fd, engine) {
+			engines[num_engines++] = engine;
 			if (num_engines == ARRAY_SIZE(engines))
 				break;
 		}
@@ -243,7 +230,7 @@ static void single(const char *name, bool all_engines)
 static void processes(void)
 {
 	const struct intel_execution_engine *e;
-	unsigned engines[16];
+	unsigned engines[16], engine;
 	int num_engines;
 	struct rlimit rlim;
 	unsigned num_ctx;
@@ -253,20 +240,8 @@ static void processes(void)
 	fd = drm_open_driver(DRIVER_INTEL);
 
 	num_engines = 0;
-	for (e = intel_execution_engines; e->name; e++) {
-		if (e->exec_id == 0)
-			continue;
-
-		if (has_engine(fd, e, 0))
-			continue;
-
-		if (e->exec_id == I915_EXEC_BSD) {
-			int is_bsd2 = e->flags != 0;
-			if (gem_has_bsd2(fd) != is_bsd2)
-				continue;
-		}
-
-		engines[num_engines++] = e->exec_id | e->flags;
+	for_each_physical_engine(fd, engine) {
+		engines[num_engines++] = engine;
 		if (num_engines == ARRAY_SIZE(engines))
 			break;
 	}
diff --git a/tests/gem_exec_async.c b/tests/gem_exec_async.c
index 30e9452f..9a06af7e 100644
--- a/tests/gem_exec_async.c
+++ b/tests/gem_exec_async.c
@@ -88,6 +88,7 @@ static void one(int fd, unsigned ring, uint32_t flags)
 #define BATCH 1
 	struct drm_i915_gem_relocation_entry reloc;
 	struct drm_i915_gem_execbuffer2 execbuf;
+	unsigned int other;
 	uint32_t *batch;
 	int i;
 
@@ -142,19 +143,14 @@ static void one(int fd, unsigned ring, uint32_t flags)
 	gem_close(fd, obj[BATCH].handle);
 
 	i = 0;
-	for (const struct intel_execution_engine *e = intel_execution_engines;
-	     e->name; e++) {
-		if (e->exec_id == 0 || e->exec_id == ring)
+	for_each_physical_engine(fd, other) {
+		if (other == ring)
 			continue;
 
-		if (!gem_has_ring(fd, e->exec_id | e->flags))
+		if (!gem_can_store_dword(fd, other))
 			continue;
 
-		if (!gem_can_store_dword(fd, e->exec_id | e->flags))
-			continue;
-
-		store_dword(fd, e->exec_id | e->flags,
-			    obj[SCRATCH].handle, 4*i, i);
+		store_dword(fd, other, obj[SCRATCH].handle, 4*i, i);
 		i++;
 	}
 
@@ -209,6 +205,7 @@ igt_main
 			continue;
 
 		igt_subtest_f("concurrent-writes-%s", e->name) {
+			igt_require(gem_ring_has_physical_engine(fd, e->exec_id | e->flags));
 			igt_require(gem_can_store_dword(fd, e->exec_id | e->flags));
 			one(fd, e->exec_id, e->flags);
 		}
diff --git a/tests/gem_exec_await.c b/tests/gem_exec_await.c
index e19363c4..fccc24d9 100644
--- a/tests/gem_exec_await.c
+++ b/tests/gem_exec_await.c
@@ -44,17 +44,6 @@ static double elapsed(const struct timespec *start, const struct timespec *end)
 		(end->tv_nsec - start->tv_nsec)*1e-9);
 }
 
-static bool ignore_engine(int fd, unsigned engine)
-{
-	if (engine == 0)
-		return true;
-
-	if (gem_has_bsd2(fd) && engine == I915_EXEC_BSD)
-		return true;
-
-	return false;
-}
-
 static void xchg_obj(void *array, unsigned i, unsigned j)
 {
 	struct drm_i915_gem_exec_object2 *obj = array;
@@ -89,12 +78,8 @@ static void wide(int fd, int ring_size, int timeout, unsigned int flags)
 	double time;
 
 	nengine = 0;
-	for_each_engine(fd, engine) {
-		if (ignore_engine(fd, engine))
-			continue;
-
+	for_each_physical_engine(fd, engine)
 		engines[nengine++] = engine;
-	}
 	igt_require(nengine);
 
 	exec = calloc(nengine, sizeof(*exec));
diff --git a/tests/gem_exec_capture.c b/tests/gem_exec_capture.c
index aa80d59d..43c443be 100644
--- a/tests/gem_exec_capture.c
+++ b/tests/gem_exec_capture.c
@@ -208,7 +208,7 @@ igt_main
 			continue;
 
 		igt_subtest_f("capture-%s", e->name) {
-			gem_require_ring(fd, e->exec_id | e->flags);
+			igt_require(gem_ring_has_physical_engine(fd, e->exec_id | e->flags));
 			igt_require(gem_can_store_dword(fd, e->exec_id | e->flags));
 			capture(fd, dir, e->exec_id | e->flags);
 		}
diff --git a/tests/gem_exec_create.c b/tests/gem_exec_create.c
index 28479a9d..54a2429e 100644
--- a/tests/gem_exec_create.c
+++ b/tests/gem_exec_create.c
@@ -54,17 +54,6 @@ static double elapsed(const struct timespec *start, const struct timespec *end)
 		(end->tv_nsec - start->tv_nsec)*1e-9);
 }
 
-static bool ignore_engine(int fd, unsigned engine)
-{
-	if (engine == 0)
-		return true;
-
-	if (gem_has_bsd2(fd) && engine == I915_EXEC_BSD)
-		return true;
-
-	return false;
-}
-
 #define LEAK 0x1
 
 static void all(int fd, unsigned flags, int timeout, int ncpus)
@@ -77,12 +66,8 @@ static void all(int fd, unsigned flags, int timeout, int ncpus)
 	unsigned engine;
 
 	nengine = 0;
-	for_each_engine(fd, engine) {
-		if (ignore_engine(fd, engine))
-			continue;
-
+	for_each_physical_engine(fd, engine)
 		engines[nengine++] = engine;
-	}
 	igt_require(nengine);
 
 	memset(&obj, 0, sizeof(obj));
diff --git a/tests/gem_exec_fence.c b/tests/gem_exec_fence.c
index 312505d6..39ca17ee 100644
--- a/tests/gem_exec_fence.c
+++ b/tests/gem_exec_fence.c
@@ -273,7 +273,7 @@ static void test_fence_await(int fd, unsigned ring, unsigned flags)
 	igt_assert(fence != -1);
 
 	i = 0;
-	for_each_engine(fd, engine) {
+	for_each_physical_engine(fd, engine) {
 		if (!gem_can_store_dword(fd, engine))
 			continue;
 
@@ -521,10 +521,7 @@ static void test_parallel(int fd, unsigned int master)
 	obj[BATCH].relocation_count = 1;
 
 	/* Queue all secondaries */
-	for_each_engine(fd, engine) {
-		if (engine == 0 || engine == I915_EXEC_BSD)
-			continue;
-
+	for_each_physical_engine(fd, engine) {
 		if (engine == master)
 			continue;
 
@@ -699,15 +696,8 @@ static void test_long_history(int fd, long ring_size, unsigned flags)
 		limit = ring_size / 3;
 
 	nengine = 0;
-	for_each_engine(fd, engine) {
-		if (engine == 0)
-			continue;
-
-		if (engine == I915_EXEC_BSD)
-			continue;
-
+	for_each_physical_engine(fd, engine)
 		engines[nengine++] = engine;
-	}
 	igt_require(nengine);
 
 	gem_quiescent_gpu(fd);
diff --git a/tests/gem_exec_gttfill.c b/tests/gem_exec_gttfill.c
index 96ed832f..4097e407 100644
--- a/tests/gem_exec_gttfill.c
+++ b/tests/gem_exec_gttfill.c
@@ -28,17 +28,6 @@ IGT_TEST_DESCRIPTION("Fill the GTT with batches.");
 
 #define BATCH_SIZE (4096<<10)
 
-static bool ignore_engine(int fd, unsigned engine)
-{
-	if (engine == 0)
-		return true;
-
-	if (gem_has_bsd2(fd) && engine == I915_EXEC_BSD)
-		return true;
-
-	return false;
-}
-
 static void xchg_u32(void *array, unsigned i, unsigned j)
 {
 	uint32_t *u32 = array;
@@ -126,10 +115,7 @@ static void fillgtt(int fd, unsigned ring, int timeout)
 
 	nengine = 0;
 	if (ring == 0) {
-		for_each_engine(fd, engine) {
-			if (ignore_engine(fd, engine))
-				continue;
-
+		for_each_physical_engine(fd, engine) {
 			if (!gem_can_store_dword(fd, engine))
 				continue;
 
diff --git a/tests/gem_exec_latency.c b/tests/gem_exec_latency.c
index 74044bf4..17ab6e33 100644
--- a/tests/gem_exec_latency.c
+++ b/tests/gem_exec_latency.c
@@ -277,13 +277,13 @@ static void latency_from_ring(int fd,
 			      unsigned ring, const char *name,
 			      unsigned flags)
 {
-	const struct intel_execution_engine *e;
 	const int gen = intel_gen(intel_get_drm_devid(fd));
 	const int has_64bit_reloc = gen >= 8;
 	struct drm_i915_gem_exec_object2 obj[3];
 	struct drm_i915_gem_relocation_entry reloc;
 	struct drm_i915_gem_execbuffer2 execbuf;
 	const unsigned int repeats = ring_size / 2;
+	unsigned int other;
 	uint32_t *map, *results;
 	uint32_t ctx[2] = {};
 	int i, j;
@@ -329,22 +329,16 @@ static void latency_from_ring(int fd,
 	reloc.presumed_offset = obj[1].offset;
 	reloc.target_handle = flags & CORK ? 1 : 0;
 
-	for (e = intel_execution_engines; e->name; e++) {
+	for_each_physical_engine(fd, other) {
 		igt_spin_t *spin = NULL;
 		struct cork c;
 
-		if (e->exec_id == 0)
-			continue;
-
-		if (!gem_has_ring(fd, e->exec_id | e->flags))
-			continue;
-
 		gem_set_domain(fd, obj[2].handle,
 			       I915_GEM_DOMAIN_GTT,
 			       I915_GEM_DOMAIN_GTT);
 
 		if (flags & PREEMPT)
-			spin = igt_spin_batch_new(fd, ctx[0], ring, 0);
+			spin = __igt_spin_batch_new(fd, ctx[0], ring, 0);
 
 		if (flags & CORK) {
 			plug(fd, &c);
@@ -382,7 +376,7 @@ static void latency_from_ring(int fd,
 			gem_execbuf(fd, &execbuf);
 
 			execbuf.flags &= ~ENGINE_FLAGS;
-			execbuf.flags |= e->exec_id | e->flags;
+			execbuf.flags |= other;
 
 			execbuf.batch_start_offset = 64 * (j + repeats);
 			reloc.offset =
@@ -415,7 +409,8 @@ static void latency_from_ring(int fd,
 		igt_spin_batch_free(fd, spin);
 
 		igt_info("%s-%s delay: %.2f\n",
-			 name, e->name, (results[2*repeats-1] - results[0]) / (double)repeats);
+			 name, e__->name,
+			 (results[2*repeats-1] - results[0]) / (double)repeats);
 	}
 
 	munmap(map, 64*1024);
@@ -461,7 +456,7 @@ igt_main
 
 			igt_subtest_group {
 				igt_fixture {
-					gem_require_ring(device, e->exec_id | e->flags);
+					igt_require(gem_ring_has_physical_engine(device, e->exec_id | e->flags));
 				}
 
 				igt_subtest_f("%s-dispatch", e->name)
diff --git a/tests/gem_exec_nop.c b/tests/gem_exec_nop.c
index d3e9a3e0..d971ffcb 100644
--- a/tests/gem_exec_nop.c
+++ b/tests/gem_exec_nop.c
@@ -188,17 +188,6 @@ static void headless(int fd, uint32_t handle)
 	assert_within_epsilon(n_headless, n_display, 0.1f);
 }
 
-static bool ignore_engine(int fd, unsigned engine)
-{
-	if (engine == 0)
-		return true;
-
-	if (gem_has_bsd2(fd) && engine == I915_EXEC_BSD)
-		return true;
-
-	return false;
-}
-
 static void parallel(int fd, uint32_t handle, int timeout)
 {
 	struct drm_i915_gem_execbuffer2 execbuf;
@@ -212,10 +201,7 @@ static void parallel(int fd, uint32_t handle, int timeout)
 
 	sum = 0;
 	nengine = 0;
-	for_each_engine(fd, engine) {
-		if (ignore_engine(fd, engine))
-			continue;
-
+	for_each_physical_engine(fd, engine) {
 		engines[nengine] = engine;
 		names[nengine] = e__->name;
 		nengine++;
@@ -277,10 +263,7 @@ static void series(int fd, uint32_t handle, int timeout)
 	const char *name;
 
 	nengine = 0;
-	for_each_engine(fd, engine) {
-		if (ignore_engine(fd, engine))
-			continue;
-
+	for_each_physical_engine(fd, engine) {
 		time = nop_on_ring(fd, handle, engine, 1, &count) / count;
 		if (time > max) {
 			name = e__->name;
@@ -375,12 +358,9 @@ static void sequential(int fd, uint32_t handle, unsigned flags, int timeout)
 
 	nengine = 0;
 	sum = 0;
-	for_each_engine(fd, n) {
+	for_each_physical_engine(fd, n) {
 		unsigned long count;
 
-		if (ignore_engine(fd, n))
-			continue;
-
 		time = nop_on_ring(fd, handle, n, 1, &count) / count;
 		sum += time;
 		igt_debug("%s: %.3fus\n", e__->name, 1e6*time);
@@ -509,12 +489,8 @@ static void fence_signal(int fd, uint32_t handle,
 
 	nengine = 0;
 	if (ring_id == -1) {
-		for_each_engine(fd, n) {
-			if (ignore_engine(fd, n))
-				continue;
-
+		for_each_physical_engine(fd, n)
 			engines[nengine++] = n;
-		}
 	} else {
 		gem_require_ring(fd, ring_id);
 		engines[nengine++] = ring_id;
diff --git a/tests/gem_exec_parallel.c b/tests/gem_exec_parallel.c
index 11b6e775..fe5ffe8f 100644
--- a/tests/gem_exec_parallel.c
+++ b/tests/gem_exec_parallel.c
@@ -55,17 +55,6 @@ static void check_bo(int fd, uint32_t handle, int pass)
 	munmap(map, 4096);
 }
 
-static bool ignore_engine(int fd, unsigned engine)
-{
-	if (engine == 0)
-		return true;
-
-	if (!gem_can_store_dword(fd, engine))
-		return true;
-
-	return false;
-}
-
 #define CONTEXTS 0x1
 #define FDS 0x2
 
@@ -180,8 +169,8 @@ static void all(int fd, unsigned engine, unsigned flags)
 
 	nengine = 0;
 	if (engine == -1) {
-		for_each_engine(fd, engine) {
-			if (!ignore_engine(fd, engine))
+		for_each_physical_engine(fd, engine) {
+			if (gem_can_store_dword(fd, engine))
 				engines[nengine++] = engine;
 		}
 	} else {
diff --git a/tests/gem_exec_reloc.c b/tests/gem_exec_reloc.c
index 432a42a9..213de1d7 100644
--- a/tests/gem_exec_reloc.c
+++ b/tests/gem_exec_reloc.c
@@ -258,7 +258,7 @@ static void active(int fd, unsigned engine)
 
 	nengine = 0;
 	if (engine == -1) {
-		for_each_engine(fd, engine) {
+		for_each_physical_engine(fd, engine) {
 			if (gem_can_store_dword(fd, engine))
 				engines[nengine++] = engine;
 		}
diff --git a/tests/gem_exec_schedule.c b/tests/gem_exec_schedule.c
index 5f24df33..dff5abb9 100644
--- a/tests/gem_exec_schedule.c
+++ b/tests/gem_exec_schedule.c
@@ -188,17 +188,6 @@ static void fifo(int fd, unsigned ring)
 	munmap(ptr, 4096);
 }
 
-static bool ignore_engine(int fd, unsigned engine)
-{
-	if (engine == 0)
-		return true;
-
-	if (gem_has_bsd2(fd) && engine == I915_EXEC_BSD)
-		return true;
-
-	return false;
-}
-
 static void smoketest(int fd, unsigned ring, unsigned timeout)
 {
 	const int ncpus = sysconf(_SC_NPROCESSORS_ONLN);
@@ -210,12 +199,8 @@ static void smoketest(int fd, unsigned ring, unsigned timeout)
 
 	nengine = 0;
 	if (ring == -1) {
-		for_each_engine(fd, engine) {
-			if (ignore_engine(fd, engine))
-				continue;
-
+		for_each_physical_engine(fd, engine)
 			engines[nengine++] = engine;
-		}
 	} else {
 		engines[nengine++] = ring;
 	}
@@ -440,7 +425,7 @@ static void preempt_other(int fd, unsigned ring)
 	gem_context_set_priority(fd, ctx[HI], MAX_PRIO);
 
 	n = 0;
-	for_each_engine(fd, other) {
+	for_each_physical_engine(fd, other) {
 		igt_assert(n < ARRAY_SIZE(spin));
 
 		spin[n] = __igt_spin_batch_new(fd, ctx[NOISE], other, 0);
@@ -496,7 +481,7 @@ static void preempt_self(int fd, unsigned ring)
 
 	n = 0;
 	gem_context_set_priority(fd, ctx[HI], MIN_PRIO);
-	for_each_engine(fd, other) {
+	for_each_physical_engine(fd, other) {
 		spin[n] = __igt_spin_batch_new(fd, ctx[NOISE], other, 0);
 		store_dword(fd, ctx[HI], other,
 			    result, (n + 1)*sizeof(uint32_t), n + 1,
@@ -1030,7 +1015,7 @@ igt_main
 				continue;
 
 			igt_subtest_f("fifo-%s", e->name) {
-				gem_require_ring(fd, e->exec_id | e->flags);
+				igt_require(gem_ring_has_physical_engine(fd, e->exec_id | e->flags));
 				igt_require(gem_can_store_dword(fd, e->exec_id) | e->flags);
 				fifo(fd, e->exec_id | e->flags);
 			}
@@ -1047,13 +1032,12 @@ igt_main
 			smoketest(fd, -1, 30);
 
 		for (e = intel_execution_engines; e->name; e++) {
-			/* default exec-id is purely symbolic */
 			if (e->exec_id == 0)
 				continue;
 
 			igt_subtest_group {
 				igt_fixture {
-					gem_require_ring(fd, e->exec_id | e->flags);
+					igt_require(gem_ring_has_physical_engine(fd, e->exec_id | e->flags));
 					igt_require(gem_can_store_dword(fd, e->exec_id) | e->flags);
 				}
 
@@ -1130,9 +1114,12 @@ igt_main
 		}
 
 		for (e = intel_execution_engines; e->name; e++) {
+			if (e->exec_id == 0)
+				continue;
+
 			igt_subtest_group {
 				igt_fixture {
-					gem_require_ring(fd, e->exec_id | e->flags);
+					igt_require(gem_ring_has_physical_engine(fd, e->exec_id | e->flags));
 					igt_require(gem_scheduler_has_preemption(fd));
 				}
 
diff --git a/tests/gem_exec_store.c b/tests/gem_exec_store.c
index 31a2c096..a7673489 100644
--- a/tests/gem_exec_store.c
+++ b/tests/gem_exec_store.c
@@ -220,7 +220,7 @@ static void store_all(int fd)
 
 	nengine = 0;
 	intel_detect_and_clear_missed_interrupts(fd);
-	for_each_engine(fd, engine) {
+	for_each_physical_engine(fd, engine) {
 		if (!gem_can_store_dword(fd, engine))
 			continue;
 
diff --git a/tests/gem_exec_suspend.c b/tests/gem_exec_suspend.c
index bbdc6e55..351347cb 100644
--- a/tests/gem_exec_suspend.c
+++ b/tests/gem_exec_suspend.c
@@ -62,25 +62,13 @@ static void check_bo(int fd, uint32_t handle)
 	munmap(map, 4096);
 }
 
-static bool ignore_engine(int fd, unsigned engine)
-{
-	if (engine == 0)
-		return true;
-
-	if (!gem_can_store_dword(fd, engine))
-		return true;
-
-	return false;
-}
-
 static void test_all(int fd, unsigned flags)
 {
 	unsigned engine;
 
-	for_each_engine(fd, engine) {
-		if (!ignore_engine(fd, engine))
+	for_each_physical_engine(fd, engine)
+		if (gem_can_store_dword(fd, engine))
 			run_test(fd, engine, flags & ~0xff);
-	}
 }
 
 static bool has_semaphores(int fd)
@@ -118,8 +106,8 @@ static void run_test(int fd, unsigned engine, unsigned flags)
 		 * GPU is then unlikely to be active!)
 		 */
 		if (has_semaphores(fd)) {
-			for_each_engine(fd, engine) {
-				if (!ignore_engine(fd, engine))
+			for_each_physical_engine(fd, engine) {
+				if (gem_can_store_dword(fd, engine))
 					engines[nengine++] = engine;
 			}
 		} else {
diff --git a/tests/gem_exec_whisper.c b/tests/gem_exec_whisper.c
index 5f9fedf5..1f4dad63 100644
--- a/tests/gem_exec_whisper.c
+++ b/tests/gem_exec_whisper.c
@@ -79,17 +79,6 @@ static void verify_reloc(int fd, uint32_t handle,
 	}
 }
 
-static bool ignore_engine(int fd, unsigned engine)
-{
-	if (engine == 0)
-		return true;
-
-	if (!gem_can_store_dword(fd, engine))
-		return true;
-
-	return false;
-}
-
 #define CONTEXTS 0x1
 #define FDS 0x2
 #define INTERRUPTIBLE 0x4
@@ -217,8 +206,8 @@ static void whisper(int fd, unsigned engine, unsigned flags)
 
 	nengine = 0;
 	if (engine == -1) {
-		for_each_engine(fd, engine) {
-			if (!ignore_engine(fd, engine))
+		for_each_physical_engine(fd, engine) {
+			if (gem_can_store_dword(fd, engine))
 				engines[nengine++] = engine;
 		}
 	} else {
diff --git a/tests/gem_ring_sync_loop.c b/tests/gem_ring_sync_loop.c
index ab5bedb3..118f3638 100644
--- a/tests/gem_ring_sync_loop.c
+++ b/tests/gem_ring_sync_loop.c
@@ -48,7 +48,7 @@ sync_loop(int fd)
 	int i;
 
 	nengine = 0;
-	for_each_engine(fd, engine)
+	for_each_physical_engine(fd, engine)
 		engines[nengine++] = engine;
 	igt_require(nengine);
 
diff --git a/tests/gem_spin_batch.c b/tests/gem_spin_batch.c
index 89631130..026f9830 100644
--- a/tests/gem_spin_batch.c
+++ b/tests/gem_spin_batch.c
@@ -76,10 +76,7 @@ static void spin_on_all_engines(int fd, unsigned int timeout_sec)
 {
 	unsigned engine;
 
-	for_each_engine(fd, engine) {
-		if (engine == 0)
-			continue;
-
+	for_each_physical_engine(fd, engine) {
 		igt_fork(child, 1) {
 			igt_install_exit_handler(spin_exit_handler);
 			spin(fd, engine, timeout_sec);
diff --git a/tests/gem_sync.c b/tests/gem_sync.c
index d70515ea..f451287a 100644
--- a/tests/gem_sync.c
+++ b/tests/gem_sync.c
@@ -86,23 +86,9 @@ sync_ring(int fd, unsigned ring, int num_children, int timeout)
 	int num_engines = 0;
 
 	if (ring == ~0u) {
-		const struct intel_execution_engine *e;
-
-		for (e = intel_execution_engines; e->name; e++) {
-			if (e->exec_id == 0)
-				continue;
-
-			if (!gem_has_ring(fd, e->exec_id | e->flags))
-				continue;
-
-			if (e->exec_id == I915_EXEC_BSD) {
-				int is_bsd2 = e->flags != 0;
-				if (gem_has_bsd2(fd) != is_bsd2)
-					continue;
-			}
-
-			names[num_engines] = e->name;
-			engines[num_engines++] = e->exec_id | e->flags;
+		for_each_physical_engine(fd, ring) {
+			names[num_engines] = e__->name;
+			engines[num_engines++] = ring;
 			if (num_engines == ARRAY_SIZE(engines))
 				break;
 		}
@@ -200,26 +186,12 @@ store_ring(int fd, unsigned ring, int num_children, int timeout)
 	int num_engines = 0;
 
 	if (ring == ~0u) {
-		const struct intel_execution_engine *e;
-
-		for (e = intel_execution_engines; e->name; e++) {
-			if (e->exec_id == 0)
-				continue;
-
-			if (!gem_has_ring(fd, e->exec_id | e->flags))
-				continue;
-
-			if (!gem_can_store_dword(fd, e->exec_id | e->flags))
+		for_each_physical_engine(fd, ring) {
+			if (!gem_can_store_dword(fd, ring))
 				continue;
 
-			if (e->exec_id == I915_EXEC_BSD) {
-				int is_bsd2 = e->flags != 0;
-				if (gem_has_bsd2(fd) != is_bsd2)
-					continue;
-			}
-
-			names[num_engines] = e->name;
-			engines[num_engines++] = e->exec_id | e->flags;
+			names[num_engines] = e__->name;
+			engines[num_engines++] = ring;
 			if (num_engines == ARRAY_SIZE(engines))
 				break;
 		}
@@ -502,31 +474,17 @@ store_many(int fd, unsigned ring, int timeout)
 	intel_detect_and_clear_missed_interrupts(fd);
 
 	if (ring == ~0u) {
-		const struct intel_execution_engine *e;
-
-		for (e = intel_execution_engines; e->name; e++) {
-			if (e->exec_id == 0)
-				continue;
-
-			if (!gem_has_ring(fd, e->exec_id | e->flags))
+		for_each_physical_engine(fd, ring) {
+			if (!gem_can_store_dword(fd, ring))
 				continue;
 
-			if (!gem_can_store_dword(fd, e->exec_id | e->flags))
-				continue;
-
-			if (e->exec_id == I915_EXEC_BSD) {
-				int is_bsd2 = e->flags != 0;
-				if (gem_has_bsd2(fd) != is_bsd2)
-					continue;
-			}
-
 			igt_fork(child, 1)
 				__store_many(fd,
-					     e->exec_id | e->flags,
+					     ring,
 					     timeout,
 					     &shared[n]);
 
-			names[n++] = e->name;
+			names[n++] = e__->name;
 		}
 		igt_waitchildren();
 	} else {
@@ -547,24 +505,11 @@ store_many(int fd, unsigned ring, int timeout)
 static void
 sync_all(int fd, int num_children, int timeout)
 {
-	const struct intel_execution_engine *e;
-	unsigned engines[16];
+	unsigned engines[16], engine;
 	int num_engines = 0;
 
-	for (e = intel_execution_engines; e->name; e++) {
-		if (e->exec_id == 0)
-			continue;
-
-		if (!gem_has_ring(fd, e->exec_id | e->flags))
-			continue;
-
-		if (e->exec_id == I915_EXEC_BSD) {
-			int is_bsd2 = e->flags != 0;
-			if (gem_has_bsd2(fd) != is_bsd2)
-				continue;
-		}
-
-		engines[num_engines++] = e->exec_id | e->flags;
+	for_each_physical_engine(fd, engine) {
+		engines[num_engines++] = engine;
 		if (num_engines == ARRAY_SIZE(engines))
 			break;
 	}
@@ -612,27 +557,15 @@ static void
 store_all(int fd, int num_children, int timeout)
 {
 	const int gen = intel_gen(intel_get_drm_devid(fd));
-	const struct intel_execution_engine *e;
 	unsigned engines[16];
 	int num_engines = 0;
+	unsigned int ring;
 
-	for (e = intel_execution_engines; e->name; e++) {
-		if (e->exec_id == 0)
-			continue;
-
-		if (!gem_has_ring(fd, e->exec_id | e->flags))
-			continue;
-
-		if (!gem_can_store_dword(fd, e->exec_id))
+	for_each_physical_engine(fd, ring) {
+		if (!gem_can_store_dword(fd, ring))
 			continue;
 
-		if (e->exec_id == I915_EXEC_BSD) {
-			int is_bsd2 = e->flags != 0;
-			if (gem_has_bsd2(fd) != is_bsd2)
-				continue;
-		}
-
-		engines[num_engines++] = e->exec_id | e->flags;
+		engines[num_engines++] = ring;
 		if (num_engines == ARRAY_SIZE(engines))
 			break;
 	}
@@ -737,23 +670,9 @@ preempt(int fd, unsigned ring, int num_children, int timeout)
 	uint32_t ctx[2];
 
 	if (ring == ~0u) {
-		const struct intel_execution_engine *e;
-
-		for (e = intel_execution_engines; e->name; e++) {
-			if (e->exec_id == 0)
-				continue;
-
-			if (!gem_has_ring(fd, e->exec_id | e->flags))
-				continue;
-
-			if (e->exec_id == I915_EXEC_BSD) {
-				int is_bsd2 = e->flags != 0;
-				if (gem_has_bsd2(fd) != is_bsd2)
-					continue;
-			}
-
-			names[num_engines] = e->name;
-			engines[num_engines++] = e->exec_id | e->flags;
+		for_each_physical_engine(fd, ring) {
+			names[num_engines] = e__->name;
+			engines[num_engines++] = ring;
 			if (num_engines == ARRAY_SIZE(engines))
 				break;
 		}
-- 
2.16.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for Iterate over physical engines

[Patchwork]

== Series Details ==

Series: Iterate over physical engines
URL   : https://patchwork.freedesktop.org/series/38746/
State : success

== Summary ==

IGT patchset tested on top of latest successful build
af65798b6674e28816bd13c096963c2d7fcdd856 lib/igt_perf: Find active perf CPU

with latest DRM-Tip kernel build CI_DRM_3819
42016703e66b drm-tip: 2018y-02m-21d-21h-26m-53s UTC integration manifest

Testlist changes:
+igt@drv_hangman@error-state-capture-bsd
-igt@gem_exec_schedule@pi-ringfull-default

Test gem_ringfill:
        Subgroup basic-default-hang:
                dmesg-warn -> INCOMPLETE (fi-pnv-d510) fdo#101600

fdo#101600 https://bugs.freedesktop.org/show_bug.cgi?id=101600

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:413s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:422s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:374s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:482s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:286s
fi-bxt-dsi       total:246  pass:219  dwarn:0   dfail:0   fail:0   skip:26 
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:483s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:469s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:456s
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:564s
fi-elk-e7500     total:288  pass:229  dwarn:0   dfail:0   fail:0   skip:59  time:414s
fi-gdg-551       total:288  pass:179  dwarn:0   dfail:0   fail:1   skip:108 time:285s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:506s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:386s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:413s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:450s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:411s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:451s
fi-kbl-7560u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:493s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:454s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:498s
fi-pnv-d510      total:146  pass:113  dwarn:0   dfail:0   fail:0   skip:32 
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:423s
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:505s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:521s
fi-skl-6700k2    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:486s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:473s
fi-skl-guc       total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:407s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:433s
fi-snb-2520m     total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:525s
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:390s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_984/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH igt v3] Iterate over physical engines

[Tvrtko Ursulin]

On 22/02/2018 07:52, Chris Wilson wrote:
> We current have a single for_each_engine() iterator which we use to
> generate both a set of uABI engines and a set of physical engines.
> Determining what uABI ring-id corresponds to an actual HW engine is
> tricky, so pull that out to a library function and introduce
> for_each_physical_engine() for cases where we want to issue requests
> once on each HW ring (avoiding aliasing issues).
> 
> v2: Remember can_store_dword for gem_sync
> v3: Find more open-coded for_each_physical
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   lib/igt_gt.c               |  23 +++++++++
>   lib/igt_gt.h               |   9 ++++
>   tests/amdgpu/amd_prime.c   |   6 +--
>   tests/drv_hangman.c        |  20 ++------
>   tests/gem_busy.c           |  31 ++++++------
>   tests/gem_concurrent_all.c |   2 +-
>   tests/gem_ctx_create.c     |   5 +-
>   tests/gem_ctx_thrash.c     |  37 +++-----------
>   tests/gem_exec_async.c     |  15 +++---
>   tests/gem_exec_await.c     |  17 +------
>   tests/gem_exec_capture.c   |   2 +-
>   tests/gem_exec_create.c    |  17 +------
>   tests/gem_exec_fence.c     |  16 ++----
>   tests/gem_exec_gttfill.c   |  16 +-----
>   tests/gem_exec_latency.c   |  19 +++----
>   tests/gem_exec_nop.c       |  32 ++----------
>   tests/gem_exec_parallel.c  |  15 +-----
>   tests/gem_exec_reloc.c     |   2 +-
>   tests/gem_exec_schedule.c  |  31 ++++--------
>   tests/gem_exec_store.c     |   2 +-
>   tests/gem_exec_suspend.c   |  20 ++------
>   tests/gem_exec_whisper.c   |  15 +-----
>   tests/gem_ring_sync_loop.c |   2 +-
>   tests/gem_spin_batch.c     |   5 +-
>   tests/gem_sync.c           | 123 ++++++++-------------------------------------
>   25 files changed, 124 insertions(+), 358 deletions(-)
> 
> diff --git a/lib/igt_gt.c b/lib/igt_gt.c
> index f70fcb92..e630550b 100644
> --- a/lib/igt_gt.c
> +++ b/lib/igt_gt.c
> @@ -660,3 +660,26 @@ bool gem_has_engine(int gem_fd,
>   			    gem_class_instance_to_eb_flags(gem_fd, class,
>   							   instance));
>   }
> +
> +bool gem_ring_is_physical_engine(int fd, unsigned ring)
> +{
> +	if (ring == I915_EXEC_DEFAULT)
> +		return false;
> +
> +	/* BSD uses an extra flag to chose between aliasing modes */
> +	if ((ring & 63) == I915_EXEC_BSD) {
> +		bool explicit_bsd = ring & (3 << 13);
> +		bool has_bsd2 = gem_has_bsd2(fd);
> +		return explicit_bsd ? has_bsd2 : !has_bsd2;
> +	}
> +
> +	return true;
> +}
> +
> +bool gem_ring_has_physical_engine(int fd, unsigned ring)
> +{
> +	if (!gem_ring_is_physical_engine(fd, ring))
> +		return false;
> +
> +	return gem_has_ring(fd, ring);
> +}
> diff --git a/lib/igt_gt.h b/lib/igt_gt.h
> index 68592410..4d9d1aa0 100644
> --- a/lib/igt_gt.h
> +++ b/lib/igt_gt.h
> @@ -81,6 +81,15 @@ extern const struct intel_execution_engine {
>   	     e__++) \
>   		for_if (gem_has_ring(fd__, flags__ = e__->exec_id | e__->flags))
>   
> +#define for_each_physical_engine(fd__, flags__) \
> +	for (const struct intel_execution_engine *e__ = intel_execution_engines;\
> +	     e__->name; \
> +	     e__++) \
> +		for_if (gem_ring_has_physical_engine(fd__, flags__ = e__->exec_id | e__->flags))
> +
> +bool gem_ring_is_physical_engine(int fd, unsigned int ring);
> +bool gem_ring_has_physical_engine(int fd, unsigned int ring);
> +
>   bool gem_can_store_dword(int fd, unsigned int engine);
>   
>   extern const struct intel_execution_engine2 {
> diff --git a/tests/amdgpu/amd_prime.c b/tests/amdgpu/amd_prime.c
> index b2f326b4..bb68ccf3 100644
> --- a/tests/amdgpu/amd_prime.c
> +++ b/tests/amdgpu/amd_prime.c
> @@ -179,12 +179,8 @@ static void i915_to_amd(int i915, int amd, amdgpu_device_handle device)
>   	struct cork c;
>   
>   	nengine = 0;
> -	for_each_engine(i915, engine) {
> -		if (engine == 0)
> -			continue;
> -
> +	for_each_physical_engine(i915, engine)
>   		engines[nengine++] = engine;
> -	}
>   	igt_require(nengine);
>   
>   	memset(obj, 0, sizeof(obj));
> diff --git a/tests/drv_hangman.c b/tests/drv_hangman.c
> index 40c82257..38cb20c3 100644
> --- a/tests/drv_hangman.c
> +++ b/tests/drv_hangman.c
> @@ -183,8 +183,6 @@ static void test_error_state_capture(unsigned ring_id,
>   	igt_hang_t hang;
>   	uint64_t offset;
>   
> -	igt_require(gem_has_ring(device, ring_id));
> -
>   	clear_error_state();
>   
>   	hang = igt_hang_ctx(device, 0, ring_id, HANG_ALLOW_CAPTURE, &offset);
> @@ -255,23 +253,11 @@ igt_main
>   		if (e->exec_id == 0)
>   			continue;
>   
> -		/*
> -		 * If the device has 2 BSD rings then due to obtuse aliasing
> -		 * in the API, we can not determine which ring I915_EXEC_BSD
> -		 * will map to, and so must skip the test; as the matching name
> -		 * may be either bsd or bsd2 depending on the kernel/test
> -		 * ordering.
> -		 *
> -		 * Here we are not checking that executing on every ABI engine
> -		 * results in a detectable hang, but that a hang generated
> -		 * from a specific HW engine gives an indentifiable result.
> -		 */
> -		if (e->exec_id == I915_EXEC_BSD && e->flags == 0)
> -			continue;
> -
> -		igt_subtest_f("error-state-capture-%s", e->name)
> +		igt_subtest_f("error-state-capture-%s", e->name) {
> +			igt_require(gem_ring_has_physical_engine(device, e->exec_id | e->flags));
>   			test_error_state_capture(e->exec_id | e->flags,
>   						 e->full_name);
> +		}
>   	}
>   
>   	igt_subtest("hangcheck-unterminated")
> diff --git a/tests/gem_busy.c b/tests/gem_busy.c
> index c349c291..49cb1c95 100644
> --- a/tests/gem_busy.c
> +++ b/tests/gem_busy.c
> @@ -156,7 +156,7 @@ static void semaphore(int fd, unsigned ring, uint32_t flags)
>   
>   #define PARALLEL 1
>   #define HANG 2
> -static void one(int fd, unsigned ring, uint32_t flags, unsigned test_flags)
> +static void one(int fd, unsigned ring, unsigned test_flags)
>   {
>   	const int gen = intel_gen(intel_get_drm_devid(fd));
>   	struct drm_i915_gem_exec_object2 obj[2];
> @@ -173,7 +173,7 @@ static void one(int fd, unsigned ring, uint32_t flags, unsigned test_flags)
>   	memset(&execbuf, 0, sizeof(execbuf));
>   	execbuf.buffers_ptr = to_user_pointer(obj);
>   	execbuf.buffer_count = 2;
> -	execbuf.flags = ring | flags;
> +	execbuf.flags = ring;
>   	if (gen < 6)
>   		execbuf.flags |= I915_EXEC_SECURE;
>   
> @@ -245,20 +245,17 @@ static void one(int fd, unsigned ring, uint32_t flags, unsigned test_flags)
>   	__gem_busy(fd, obj[BATCH].handle, &read[BATCH], &write[BATCH]);
>   
>   	if (test_flags & PARALLEL) {
> -		const struct intel_execution_engine *e;
> -
> -		for (e = intel_execution_engines; e->name; e++) {
> -			if (e->exec_id == 0 || e->exec_id == ring)
> -				continue;
> +		unsigned other;
>   
> -			if (!gem_has_ring(fd, e->exec_id | e->flags))
> +		for_each_physical_engine(fd, other) {
> +			if (other == ring)
>   				continue;
>   
> -			if (!gem_can_store_dword(fd, e->exec_id | e->flags))
> +			if (!gem_can_store_dword(fd, other))
>   				continue;
>   
> -			igt_debug("Testing %s in parallel\n", e->name);
> -			one(fd, e->exec_id, e->flags, 0);
> +			igt_debug("Testing %s in parallel\n", e__->name);
> +			one(fd, other, 0);
>   		}
>   	}
>   
> @@ -591,10 +588,10 @@ igt_main
>   					continue;
>   
>   				igt_subtest_f("extended-%s", e->name) {
> -					gem_require_ring(fd, e->exec_id | e->flags);
> +					igt_require(gem_ring_has_physical_engine(fd, e->exec_id | e->flags));
>   					igt_require(gem_can_store_dword(fd, e->exec_id | e->flags));
>   					gem_quiescent_gpu(fd);
> -					one(fd, e->exec_id, e->flags, 0);
> +					one(fd, e->exec_id | e->flags, 0);
>   					gem_quiescent_gpu(fd);
>   				}
>   			}
> @@ -605,11 +602,11 @@ igt_main
>   					continue;
>   
>   				igt_subtest_f("extended-parallel-%s", e->name) {
> -					gem_require_ring(fd, e->exec_id | e->flags);
> +					igt_require(gem_ring_has_physical_engine(fd, e->exec_id | e->flags));
>   					igt_require(gem_can_store_dword(fd, e->exec_id | e->flags));
>   
>   					gem_quiescent_gpu(fd);
> -					one(fd, e->exec_id, e->flags, PARALLEL);
> +					one(fd, e->exec_id | e->flags, PARALLEL);
>   					gem_quiescent_gpu(fd);
>   				}
>   			}
> @@ -670,11 +667,11 @@ igt_main
>   
>   				igt_subtest_f("extended-hang-%s", e->name) {
>   					igt_skip_on_simulation();
> -					gem_require_ring(fd, e->exec_id | e->flags);
> +					igt_require(gem_ring_has_physical_engine(fd, e->exec_id | e->flags));
>   					igt_require(gem_can_store_dword(fd, e->exec_id | e->flags));
>   
>   					gem_quiescent_gpu(fd);
> -					one(fd, e->exec_id, e->flags, HANG);
> +					one(fd, e->exec_id | e->flags, HANG);
>   					gem_quiescent_gpu(fd);
>   				}
>   			}
> diff --git a/tests/gem_concurrent_all.c b/tests/gem_concurrent_all.c
> index 201b491b..3a1097ba 100644
> --- a/tests/gem_concurrent_all.c
> +++ b/tests/gem_concurrent_all.c
> @@ -960,7 +960,7 @@ static igt_hang_t all_hang(void)
>   	execbuf.buffers_ptr = to_user_pointer(&obj);
>   	execbuf.buffer_count = 1;
>   
> -	for_each_engine(fd, engine) {
> +	for_each_physical_engine(fd, engine) {
>   		hang = igt_hang_ring(fd, engine);
>   
>   		execbuf.flags = engine;
> diff --git a/tests/gem_ctx_create.c b/tests/gem_ctx_create.c
> index 058445b6..1b32d6c3 100644
> --- a/tests/gem_ctx_create.c
> +++ b/tests/gem_ctx_create.c
> @@ -321,11 +321,8 @@ igt_main
>   		igt_require_gem(fd);
>   		gem_require_contexts(fd);
>   
> -		for_each_engine(fd, engine) {
> -			if (engine == 0)
> -				continue;
> +		for_each_physical_engine(fd, engine)
>   			all_engines[all_nengine++] = engine;
> -		}
>   		igt_require(all_nengine);
>   
>   		if (gem_uses_full_ppgtt(fd)) {
> diff --git a/tests/gem_ctx_thrash.c b/tests/gem_ctx_thrash.c
> index b5d334f4..23203158 100644
> --- a/tests/gem_ctx_thrash.c
> +++ b/tests/gem_ctx_thrash.c
> @@ -130,22 +130,9 @@ static void single(const char *name, bool all_engines)
>   
>   	num_engines = 0;
>   	if (all_engines) {
> -		for (e = intel_execution_engines; e->name; e++) {
> -			if (e->exec_id == 0)
> -				continue;
> -
> -			if (has_engine(fd, e, 0))
> -				continue;
> -
> -			if (e->exec_id == I915_EXEC_BSD) {
> -				int is_bsd2 = e->flags != 0;
> -				if (gem_has_bsd2(fd) != is_bsd2)
> -					continue;
> -			}
> -
> -			igt_require(has_engine(fd, e, 1) == -ENOENT);
> -
> -			engines[num_engines++] = e->exec_id | e->flags;
> +		unsigned engine;
> +		for_each_physical_engine(fd, engine) {
> +			engines[num_engines++] = engine;
>   			if (num_engines == ARRAY_SIZE(engines))
>   				break;
>   		}
> @@ -243,7 +230,7 @@ static void single(const char *name, bool all_engines)
>   static void processes(void)
>   {
>   	const struct intel_execution_engine *e;
> -	unsigned engines[16];
> +	unsigned engines[16], engine;
>   	int num_engines;
>   	struct rlimit rlim;
>   	unsigned num_ctx;
> @@ -253,20 +240,8 @@ static void processes(void)
>   	fd = drm_open_driver(DRIVER_INTEL);
>   
>   	num_engines = 0;
> -	for (e = intel_execution_engines; e->name; e++) {
> -		if (e->exec_id == 0)
> -			continue;
> -
> -		if (has_engine(fd, e, 0))
> -			continue;
> -
> -		if (e->exec_id == I915_EXEC_BSD) {
> -			int is_bsd2 = e->flags != 0;
> -			if (gem_has_bsd2(fd) != is_bsd2)
> -				continue;
> -		}
> -
> -		engines[num_engines++] = e->exec_id | e->flags;
> +	for_each_physical_engine(fd, engine) {
> +		engines[num_engines++] = engine;
>   		if (num_engines == ARRAY_SIZE(engines))
>   			break;
>   	}
> diff --git a/tests/gem_exec_async.c b/tests/gem_exec_async.c
> index 30e9452f..9a06af7e 100644
> --- a/tests/gem_exec_async.c
> +++ b/tests/gem_exec_async.c
> @@ -88,6 +88,7 @@ static void one(int fd, unsigned ring, uint32_t flags)
>   #define BATCH 1
>   	struct drm_i915_gem_relocation_entry reloc;
>   	struct drm_i915_gem_execbuffer2 execbuf;
> +	unsigned int other;
>   	uint32_t *batch;
>   	int i;
>   
> @@ -142,19 +143,14 @@ static void one(int fd, unsigned ring, uint32_t flags)
>   	gem_close(fd, obj[BATCH].handle);
>   
>   	i = 0;
> -	for (const struct intel_execution_engine *e = intel_execution_engines;
> -	     e->name; e++) {
> -		if (e->exec_id == 0 || e->exec_id == ring)
> +	for_each_physical_engine(fd, other) {
> +		if (other == ring)
>   			continue;
>   
> -		if (!gem_has_ring(fd, e->exec_id | e->flags))
> +		if (!gem_can_store_dword(fd, other))
>   			continue;
>   
> -		if (!gem_can_store_dword(fd, e->exec_id | e->flags))
> -			continue;
> -
> -		store_dword(fd, e->exec_id | e->flags,
> -			    obj[SCRATCH].handle, 4*i, i);
> +		store_dword(fd, other, obj[SCRATCH].handle, 4*i, i);
>   		i++;
>   	}
>   
> @@ -209,6 +205,7 @@ igt_main
>   			continue;
>   
>   		igt_subtest_f("concurrent-writes-%s", e->name) {
> +			igt_require(gem_ring_has_physical_engine(fd, e->exec_id | e->flags));
>   			igt_require(gem_can_store_dword(fd, e->exec_id | e->flags));
>   			one(fd, e->exec_id, e->flags);
>   		}
> diff --git a/tests/gem_exec_await.c b/tests/gem_exec_await.c
> index e19363c4..fccc24d9 100644
> --- a/tests/gem_exec_await.c
> +++ b/tests/gem_exec_await.c
> @@ -44,17 +44,6 @@ static double elapsed(const struct timespec *start, const struct timespec *end)
>   		(end->tv_nsec - start->tv_nsec)*1e-9);
>   }
>   
> -static bool ignore_engine(int fd, unsigned engine)
> -{
> -	if (engine == 0)
> -		return true;
> -
> -	if (gem_has_bsd2(fd) && engine == I915_EXEC_BSD)
> -		return true;
> -
> -	return false;
> -}
> -
>   static void xchg_obj(void *array, unsigned i, unsigned j)
>   {
>   	struct drm_i915_gem_exec_object2 *obj = array;
> @@ -89,12 +78,8 @@ static void wide(int fd, int ring_size, int timeout, unsigned int flags)
>   	double time;
>   
>   	nengine = 0;
> -	for_each_engine(fd, engine) {
> -		if (ignore_engine(fd, engine))
> -			continue;
> -
> +	for_each_physical_engine(fd, engine)
>   		engines[nengine++] = engine;
> -	}
>   	igt_require(nengine);
>   
>   	exec = calloc(nengine, sizeof(*exec));
> diff --git a/tests/gem_exec_capture.c b/tests/gem_exec_capture.c
> index aa80d59d..43c443be 100644
> --- a/tests/gem_exec_capture.c
> +++ b/tests/gem_exec_capture.c
> @@ -208,7 +208,7 @@ igt_main
>   			continue;
>   
>   		igt_subtest_f("capture-%s", e->name) {
> -			gem_require_ring(fd, e->exec_id | e->flags);
> +			igt_require(gem_ring_has_physical_engine(fd, e->exec_id | e->flags));
>   			igt_require(gem_can_store_dword(fd, e->exec_id | e->flags));
>   			capture(fd, dir, e->exec_id | e->flags);
>   		}
> diff --git a/tests/gem_exec_create.c b/tests/gem_exec_create.c
> index 28479a9d..54a2429e 100644
> --- a/tests/gem_exec_create.c
> +++ b/tests/gem_exec_create.c
> @@ -54,17 +54,6 @@ static double elapsed(const struct timespec *start, const struct timespec *end)
>   		(end->tv_nsec - start->tv_nsec)*1e-9);
>   }
>   
> -static bool ignore_engine(int fd, unsigned engine)
> -{
> -	if (engine == 0)
> -		return true;
> -
> -	if (gem_has_bsd2(fd) && engine == I915_EXEC_BSD)
> -		return true;
> -
> -	return false;
> -}
> -
>   #define LEAK 0x1
>   
>   static void all(int fd, unsigned flags, int timeout, int ncpus)
> @@ -77,12 +66,8 @@ static void all(int fd, unsigned flags, int timeout, int ncpus)
>   	unsigned engine;
>   
>   	nengine = 0;
> -	for_each_engine(fd, engine) {
> -		if (ignore_engine(fd, engine))
> -			continue;
> -
> +	for_each_physical_engine(fd, engine)
>   		engines[nengine++] = engine;
> -	}
>   	igt_require(nengine);
>   
>   	memset(&obj, 0, sizeof(obj));
> diff --git a/tests/gem_exec_fence.c b/tests/gem_exec_fence.c
> index 312505d6..39ca17ee 100644
> --- a/tests/gem_exec_fence.c
> +++ b/tests/gem_exec_fence.c
> @@ -273,7 +273,7 @@ static void test_fence_await(int fd, unsigned ring, unsigned flags)
>   	igt_assert(fence != -1);
>   
>   	i = 0;
> -	for_each_engine(fd, engine) {
> +	for_each_physical_engine(fd, engine) {
>   		if (!gem_can_store_dword(fd, engine))
>   			continue;
>   
> @@ -521,10 +521,7 @@ static void test_parallel(int fd, unsigned int master)
>   	obj[BATCH].relocation_count = 1;
>   
>   	/* Queue all secondaries */
> -	for_each_engine(fd, engine) {
> -		if (engine == 0 || engine == I915_EXEC_BSD)
> -			continue;
> -
> +	for_each_physical_engine(fd, engine) {
>   		if (engine == master)
>   			continue;
>   
> @@ -699,15 +696,8 @@ static void test_long_history(int fd, long ring_size, unsigned flags)
>   		limit = ring_size / 3;
>   
>   	nengine = 0;
> -	for_each_engine(fd, engine) {
> -		if (engine == 0)
> -			continue;
> -
> -		if (engine == I915_EXEC_BSD)
> -			continue;
> -
> +	for_each_physical_engine(fd, engine)
>   		engines[nengine++] = engine;
> -	}
>   	igt_require(nengine);
>   
>   	gem_quiescent_gpu(fd);
> diff --git a/tests/gem_exec_gttfill.c b/tests/gem_exec_gttfill.c
> index 96ed832f..4097e407 100644
> --- a/tests/gem_exec_gttfill.c
> +++ b/tests/gem_exec_gttfill.c
> @@ -28,17 +28,6 @@ IGT_TEST_DESCRIPTION("Fill the GTT with batches.");
>   
>   #define BATCH_SIZE (4096<<10)
>   
> -static bool ignore_engine(int fd, unsigned engine)
> -{
> -	if (engine == 0)
> -		return true;
> -
> -	if (gem_has_bsd2(fd) && engine == I915_EXEC_BSD)
> -		return true;
> -
> -	return false;
> -}
> -
>   static void xchg_u32(void *array, unsigned i, unsigned j)
>   {
>   	uint32_t *u32 = array;
> @@ -126,10 +115,7 @@ static void fillgtt(int fd, unsigned ring, int timeout)
>   
>   	nengine = 0;
>   	if (ring == 0) {
> -		for_each_engine(fd, engine) {
> -			if (ignore_engine(fd, engine))
> -				continue;
> -
> +		for_each_physical_engine(fd, engine) {
>   			if (!gem_can_store_dword(fd, engine))
>   				continue;
>   
> diff --git a/tests/gem_exec_latency.c b/tests/gem_exec_latency.c
> index 74044bf4..17ab6e33 100644
> --- a/tests/gem_exec_latency.c
> +++ b/tests/gem_exec_latency.c
> @@ -277,13 +277,13 @@ static void latency_from_ring(int fd,
>   			      unsigned ring, const char *name,
>   			      unsigned flags)
>   {
> -	const struct intel_execution_engine *e;
>   	const int gen = intel_gen(intel_get_drm_devid(fd));
>   	const int has_64bit_reloc = gen >= 8;
>   	struct drm_i915_gem_exec_object2 obj[3];
>   	struct drm_i915_gem_relocation_entry reloc;
>   	struct drm_i915_gem_execbuffer2 execbuf;
>   	const unsigned int repeats = ring_size / 2;
> +	unsigned int other;
>   	uint32_t *map, *results;
>   	uint32_t ctx[2] = {};
>   	int i, j;
> @@ -329,22 +329,16 @@ static void latency_from_ring(int fd,
>   	reloc.presumed_offset = obj[1].offset;
>   	reloc.target_handle = flags & CORK ? 1 : 0;
>   
> -	for (e = intel_execution_engines; e->name; e++) {
> +	for_each_physical_engine(fd, other) {
>   		igt_spin_t *spin = NULL;
>   		struct cork c;
>   
> -		if (e->exec_id == 0)
> -			continue;
> -
> -		if (!gem_has_ring(fd, e->exec_id | e->flags))
> -			continue;
> -
>   		gem_set_domain(fd, obj[2].handle,
>   			       I915_GEM_DOMAIN_GTT,
>   			       I915_GEM_DOMAIN_GTT);
>   
>   		if (flags & PREEMPT)
> -			spin = igt_spin_batch_new(fd, ctx[0], ring, 0);
> +			spin = __igt_spin_batch_new(fd, ctx[0], ring, 0);
>   
>   		if (flags & CORK) {
>   			plug(fd, &c);
> @@ -382,7 +376,7 @@ static void latency_from_ring(int fd,
>   			gem_execbuf(fd, &execbuf);
>   
>   			execbuf.flags &= ~ENGINE_FLAGS;
> -			execbuf.flags |= e->exec_id | e->flags;
> +			execbuf.flags |= other;
>   
>   			execbuf.batch_start_offset = 64 * (j + repeats);
>   			reloc.offset =
> @@ -415,7 +409,8 @@ static void latency_from_ring(int fd,
>   		igt_spin_batch_free(fd, spin);
>   
>   		igt_info("%s-%s delay: %.2f\n",
> -			 name, e->name, (results[2*repeats-1] - results[0]) / (double)repeats);
> +			 name, e__->name,
> +			 (results[2*repeats-1] - results[0]) / (double)repeats);
>   	}
>   
>   	munmap(map, 64*1024);
> @@ -461,7 +456,7 @@ igt_main
>   
>   			igt_subtest_group {
>   				igt_fixture {
> -					gem_require_ring(device, e->exec_id | e->flags);
> +					igt_require(gem_ring_has_physical_engine(device, e->exec_id | e->flags));
>   				}
>   
>   				igt_subtest_f("%s-dispatch", e->name)
> diff --git a/tests/gem_exec_nop.c b/tests/gem_exec_nop.c
> index d3e9a3e0..d971ffcb 100644
> --- a/tests/gem_exec_nop.c
> +++ b/tests/gem_exec_nop.c
> @@ -188,17 +188,6 @@ static void headless(int fd, uint32_t handle)
>   	assert_within_epsilon(n_headless, n_display, 0.1f);
>   }
>   
> -static bool ignore_engine(int fd, unsigned engine)
> -{
> -	if (engine == 0)
> -		return true;
> -
> -	if (gem_has_bsd2(fd) && engine == I915_EXEC_BSD)
> -		return true;
> -
> -	return false;
> -}
> -
>   static void parallel(int fd, uint32_t handle, int timeout)
>   {
>   	struct drm_i915_gem_execbuffer2 execbuf;
> @@ -212,10 +201,7 @@ static void parallel(int fd, uint32_t handle, int timeout)
>   
>   	sum = 0;
>   	nengine = 0;
> -	for_each_engine(fd, engine) {
> -		if (ignore_engine(fd, engine))
> -			continue;
> -
> +	for_each_physical_engine(fd, engine) {
>   		engines[nengine] = engine;
>   		names[nengine] = e__->name;
>   		nengine++;
> @@ -277,10 +263,7 @@ static void series(int fd, uint32_t handle, int timeout)
>   	const char *name;
>   
>   	nengine = 0;
> -	for_each_engine(fd, engine) {
> -		if (ignore_engine(fd, engine))
> -			continue;
> -
> +	for_each_physical_engine(fd, engine) {
>   		time = nop_on_ring(fd, handle, engine, 1, &count) / count;
>   		if (time > max) {
>   			name = e__->name;
> @@ -375,12 +358,9 @@ static void sequential(int fd, uint32_t handle, unsigned flags, int timeout)
>   
>   	nengine = 0;
>   	sum = 0;
> -	for_each_engine(fd, n) {
> +	for_each_physical_engine(fd, n) {
>   		unsigned long count;
>   
> -		if (ignore_engine(fd, n))
> -			continue;
> -
>   		time = nop_on_ring(fd, handle, n, 1, &count) / count;
>   		sum += time;
>   		igt_debug("%s: %.3fus\n", e__->name, 1e6*time);
> @@ -509,12 +489,8 @@ static void fence_signal(int fd, uint32_t handle,
>   
>   	nengine = 0;
>   	if (ring_id == -1) {
> -		for_each_engine(fd, n) {
> -			if (ignore_engine(fd, n))
> -				continue;
> -
> +		for_each_physical_engine(fd, n)
>   			engines[nengine++] = n;
> -		}
>   	} else {
>   		gem_require_ring(fd, ring_id);
>   		engines[nengine++] = ring_id;
> diff --git a/tests/gem_exec_parallel.c b/tests/gem_exec_parallel.c
> index 11b6e775..fe5ffe8f 100644
> --- a/tests/gem_exec_parallel.c
> +++ b/tests/gem_exec_parallel.c
> @@ -55,17 +55,6 @@ static void check_bo(int fd, uint32_t handle, int pass)
>   	munmap(map, 4096);
>   }
>   
> -static bool ignore_engine(int fd, unsigned engine)
> -{
> -	if (engine == 0)
> -		return true;
> -
> -	if (!gem_can_store_dword(fd, engine))
> -		return true;
> -
> -	return false;
> -}
> -
>   #define CONTEXTS 0x1
>   #define FDS 0x2
>   
> @@ -180,8 +169,8 @@ static void all(int fd, unsigned engine, unsigned flags)
>   
>   	nengine = 0;
>   	if (engine == -1) {
> -		for_each_engine(fd, engine) {
> -			if (!ignore_engine(fd, engine))
> +		for_each_physical_engine(fd, engine) {
> +			if (gem_can_store_dword(fd, engine))
>   				engines[nengine++] = engine;
>   		}
>   	} else {
> diff --git a/tests/gem_exec_reloc.c b/tests/gem_exec_reloc.c
> index 432a42a9..213de1d7 100644
> --- a/tests/gem_exec_reloc.c
> +++ b/tests/gem_exec_reloc.c
> @@ -258,7 +258,7 @@ static void active(int fd, unsigned engine)
>   
>   	nengine = 0;
>   	if (engine == -1) {
> -		for_each_engine(fd, engine) {
> +		for_each_physical_engine(fd, engine) {
>   			if (gem_can_store_dword(fd, engine))
>   				engines[nengine++] = engine;
>   		}
> diff --git a/tests/gem_exec_schedule.c b/tests/gem_exec_schedule.c
> index 5f24df33..dff5abb9 100644
> --- a/tests/gem_exec_schedule.c
> +++ b/tests/gem_exec_schedule.c
> @@ -188,17 +188,6 @@ static void fifo(int fd, unsigned ring)
>   	munmap(ptr, 4096);
>   }
>   
> -static bool ignore_engine(int fd, unsigned engine)
> -{
> -	if (engine == 0)
> -		return true;
> -
> -	if (gem_has_bsd2(fd) && engine == I915_EXEC_BSD)
> -		return true;
> -
> -	return false;
> -}
> -
>   static void smoketest(int fd, unsigned ring, unsigned timeout)
>   {
>   	const int ncpus = sysconf(_SC_NPROCESSORS_ONLN);
> @@ -210,12 +199,8 @@ static void smoketest(int fd, unsigned ring, unsigned timeout)
>   
>   	nengine = 0;
>   	if (ring == -1) {
> -		for_each_engine(fd, engine) {
> -			if (ignore_engine(fd, engine))
> -				continue;
> -
> +		for_each_physical_engine(fd, engine)
>   			engines[nengine++] = engine;
> -		}
>   	} else {
>   		engines[nengine++] = ring;
>   	}
> @@ -440,7 +425,7 @@ static void preempt_other(int fd, unsigned ring)
>   	gem_context_set_priority(fd, ctx[HI], MAX_PRIO);
>   
>   	n = 0;
> -	for_each_engine(fd, other) {
> +	for_each_physical_engine(fd, other) {
>   		igt_assert(n < ARRAY_SIZE(spin));
>   
>   		spin[n] = __igt_spin_batch_new(fd, ctx[NOISE], other, 0);
> @@ -496,7 +481,7 @@ static void preempt_self(int fd, unsigned ring)
>   
>   	n = 0;
>   	gem_context_set_priority(fd, ctx[HI], MIN_PRIO);
> -	for_each_engine(fd, other) {
> +	for_each_physical_engine(fd, other) {
>   		spin[n] = __igt_spin_batch_new(fd, ctx[NOISE], other, 0);
>   		store_dword(fd, ctx[HI], other,
>   			    result, (n + 1)*sizeof(uint32_t), n + 1,
> @@ -1030,7 +1015,7 @@ igt_main
>   				continue;
>   
>   			igt_subtest_f("fifo-%s", e->name) {
> -				gem_require_ring(fd, e->exec_id | e->flags);
> +				igt_require(gem_ring_has_physical_engine(fd, e->exec_id | e->flags));
>   				igt_require(gem_can_store_dword(fd, e->exec_id) | e->flags);
>   				fifo(fd, e->exec_id | e->flags);
>   			}
> @@ -1047,13 +1032,12 @@ igt_main
>   			smoketest(fd, -1, 30);
>   
>   		for (e = intel_execution_engines; e->name; e++) {
> -			/* default exec-id is purely symbolic */
>   			if (e->exec_id == 0)
>   				continue;
>   
>   			igt_subtest_group {
>   				igt_fixture {
> -					gem_require_ring(fd, e->exec_id | e->flags);
> +					igt_require(gem_ring_has_physical_engine(fd, e->exec_id | e->flags));
>   					igt_require(gem_can_store_dword(fd, e->exec_id) | e->flags);
>   				}
>   
> @@ -1130,9 +1114,12 @@ igt_main
>   		}
>   
>   		for (e = intel_execution_engines; e->name; e++) {
> +			if (e->exec_id == 0)
> +				continue;
> +
>   			igt_subtest_group {
>   				igt_fixture {
> -					gem_require_ring(fd, e->exec_id | e->flags);
> +					igt_require(gem_ring_has_physical_engine(fd, e->exec_id | e->flags));
>   					igt_require(gem_scheduler_has_preemption(fd));
>   				}
>   
> diff --git a/tests/gem_exec_store.c b/tests/gem_exec_store.c
> index 31a2c096..a7673489 100644
> --- a/tests/gem_exec_store.c
> +++ b/tests/gem_exec_store.c
> @@ -220,7 +220,7 @@ static void store_all(int fd)
>   
>   	nengine = 0;
>   	intel_detect_and_clear_missed_interrupts(fd);
> -	for_each_engine(fd, engine) {
> +	for_each_physical_engine(fd, engine) {
>   		if (!gem_can_store_dword(fd, engine))
>   			continue;
>   
> diff --git a/tests/gem_exec_suspend.c b/tests/gem_exec_suspend.c
> index bbdc6e55..351347cb 100644
> --- a/tests/gem_exec_suspend.c
> +++ b/tests/gem_exec_suspend.c
> @@ -62,25 +62,13 @@ static void check_bo(int fd, uint32_t handle)
>   	munmap(map, 4096);
>   }
>   
> -static bool ignore_engine(int fd, unsigned engine)
> -{
> -	if (engine == 0)
> -		return true;
> -
> -	if (!gem_can_store_dword(fd, engine))
> -		return true;
> -
> -	return false;
> -}
> -
>   static void test_all(int fd, unsigned flags)
>   {
>   	unsigned engine;
>   
> -	for_each_engine(fd, engine) {
> -		if (!ignore_engine(fd, engine))
> +	for_each_physical_engine(fd, engine)
> +		if (gem_can_store_dword(fd, engine))
>   			run_test(fd, engine, flags & ~0xff);
> -	}
>   }
>   
>   static bool has_semaphores(int fd)
> @@ -118,8 +106,8 @@ static void run_test(int fd, unsigned engine, unsigned flags)
>   		 * GPU is then unlikely to be active!)
>   		 */
>   		if (has_semaphores(fd)) {
> -			for_each_engine(fd, engine) {
> -				if (!ignore_engine(fd, engine))
> +			for_each_physical_engine(fd, engine) {
> +				if (gem_can_store_dword(fd, engine))
>   					engines[nengine++] = engine;
>   			}
>   		} else {
> diff --git a/tests/gem_exec_whisper.c b/tests/gem_exec_whisper.c
> index 5f9fedf5..1f4dad63 100644
> --- a/tests/gem_exec_whisper.c
> +++ b/tests/gem_exec_whisper.c
> @@ -79,17 +79,6 @@ static void verify_reloc(int fd, uint32_t handle,
>   	}
>   }
>   
> -static bool ignore_engine(int fd, unsigned engine)
> -{
> -	if (engine == 0)
> -		return true;
> -
> -	if (!gem_can_store_dword(fd, engine))
> -		return true;
> -
> -	return false;
> -}
> -
>   #define CONTEXTS 0x1
>   #define FDS 0x2
>   #define INTERRUPTIBLE 0x4
> @@ -217,8 +206,8 @@ static void whisper(int fd, unsigned engine, unsigned flags)
>   
>   	nengine = 0;
>   	if (engine == -1) {
> -		for_each_engine(fd, engine) {
> -			if (!ignore_engine(fd, engine))
> +		for_each_physical_engine(fd, engine) {
> +			if (gem_can_store_dword(fd, engine))
>   				engines[nengine++] = engine;
>   		}
>   	} else {
> diff --git a/tests/gem_ring_sync_loop.c b/tests/gem_ring_sync_loop.c
> index ab5bedb3..118f3638 100644
> --- a/tests/gem_ring_sync_loop.c
> +++ b/tests/gem_ring_sync_loop.c
> @@ -48,7 +48,7 @@ sync_loop(int fd)
>   	int i;
>   
>   	nengine = 0;
> -	for_each_engine(fd, engine)
> +	for_each_physical_engine(fd, engine)
>   		engines[nengine++] = engine;
>   	igt_require(nengine);
>   
> diff --git a/tests/gem_spin_batch.c b/tests/gem_spin_batch.c
> index 89631130..026f9830 100644
> --- a/tests/gem_spin_batch.c
> +++ b/tests/gem_spin_batch.c
> @@ -76,10 +76,7 @@ static void spin_on_all_engines(int fd, unsigned int timeout_sec)
>   {
>   	unsigned engine;
>   
> -	for_each_engine(fd, engine) {
> -		if (engine == 0)
> -			continue;
> -
> +	for_each_physical_engine(fd, engine) {
>   		igt_fork(child, 1) {
>   			igt_install_exit_handler(spin_exit_handler);
>   			spin(fd, engine, timeout_sec);
> diff --git a/tests/gem_sync.c b/tests/gem_sync.c
> index d70515ea..f451287a 100644
> --- a/tests/gem_sync.c
> +++ b/tests/gem_sync.c
> @@ -86,23 +86,9 @@ sync_ring(int fd, unsigned ring, int num_children, int timeout)
>   	int num_engines = 0;
>   
>   	if (ring == ~0u) {
> -		const struct intel_execution_engine *e;
> -
> -		for (e = intel_execution_engines; e->name; e++) {
> -			if (e->exec_id == 0)
> -				continue;
> -
> -			if (!gem_has_ring(fd, e->exec_id | e->flags))
> -				continue;
> -
> -			if (e->exec_id == I915_EXEC_BSD) {
> -				int is_bsd2 = e->flags != 0;
> -				if (gem_has_bsd2(fd) != is_bsd2)
> -					continue;
> -			}
> -
> -			names[num_engines] = e->name;
> -			engines[num_engines++] = e->exec_id | e->flags;
> +		for_each_physical_engine(fd, ring) {
> +			names[num_engines] = e__->name;
> +			engines[num_engines++] = ring;
>   			if (num_engines == ARRAY_SIZE(engines))
>   				break;
>   		}
> @@ -200,26 +186,12 @@ store_ring(int fd, unsigned ring, int num_children, int timeout)
>   	int num_engines = 0;
>   
>   	if (ring == ~0u) {
> -		const struct intel_execution_engine *e;
> -
> -		for (e = intel_execution_engines; e->name; e++) {
> -			if (e->exec_id == 0)
> -				continue;
> -
> -			if (!gem_has_ring(fd, e->exec_id | e->flags))
> -				continue;
> -
> -			if (!gem_can_store_dword(fd, e->exec_id | e->flags))
> +		for_each_physical_engine(fd, ring) {
> +			if (!gem_can_store_dword(fd, ring))
>   				continue;
>   
> -			if (e->exec_id == I915_EXEC_BSD) {
> -				int is_bsd2 = e->flags != 0;
> -				if (gem_has_bsd2(fd) != is_bsd2)
> -					continue;
> -			}
> -
> -			names[num_engines] = e->name;
> -			engines[num_engines++] = e->exec_id | e->flags;
> +			names[num_engines] = e__->name;
> +			engines[num_engines++] = ring;
>   			if (num_engines == ARRAY_SIZE(engines))
>   				break;
>   		}
> @@ -502,31 +474,17 @@ store_many(int fd, unsigned ring, int timeout)
>   	intel_detect_and_clear_missed_interrupts(fd);
>   
>   	if (ring == ~0u) {
> -		const struct intel_execution_engine *e;
> -
> -		for (e = intel_execution_engines; e->name; e++) {
> -			if (e->exec_id == 0)
> -				continue;
> -
> -			if (!gem_has_ring(fd, e->exec_id | e->flags))
> +		for_each_physical_engine(fd, ring) {
> +			if (!gem_can_store_dword(fd, ring))
>   				continue;
>   
> -			if (!gem_can_store_dword(fd, e->exec_id | e->flags))
> -				continue;
> -
> -			if (e->exec_id == I915_EXEC_BSD) {
> -				int is_bsd2 = e->flags != 0;
> -				if (gem_has_bsd2(fd) != is_bsd2)
> -					continue;
> -			}
> -
>   			igt_fork(child, 1)
>   				__store_many(fd,
> -					     e->exec_id | e->flags,
> +					     ring,
>   					     timeout,
>   					     &shared[n]);
>   
> -			names[n++] = e->name;
> +			names[n++] = e__->name;
>   		}
>   		igt_waitchildren();
>   	} else {
> @@ -547,24 +505,11 @@ store_many(int fd, unsigned ring, int timeout)
>   static void
>   sync_all(int fd, int num_children, int timeout)
>   {
> -	const struct intel_execution_engine *e;
> -	unsigned engines[16];
> +	unsigned engines[16], engine;
>   	int num_engines = 0;
>   
> -	for (e = intel_execution_engines; e->name; e++) {
> -		if (e->exec_id == 0)
> -			continue;
> -
> -		if (!gem_has_ring(fd, e->exec_id | e->flags))
> -			continue;
> -
> -		if (e->exec_id == I915_EXEC_BSD) {
> -			int is_bsd2 = e->flags != 0;
> -			if (gem_has_bsd2(fd) != is_bsd2)
> -				continue;
> -		}
> -
> -		engines[num_engines++] = e->exec_id | e->flags;
> +	for_each_physical_engine(fd, engine) {
> +		engines[num_engines++] = engine;
>   		if (num_engines == ARRAY_SIZE(engines))
>   			break;
>   	}
> @@ -612,27 +557,15 @@ static void
>   store_all(int fd, int num_children, int timeout)
>   {
>   	const int gen = intel_gen(intel_get_drm_devid(fd));
> -	const struct intel_execution_engine *e;
>   	unsigned engines[16];
>   	int num_engines = 0;
> +	unsigned int ring;
>   
> -	for (e = intel_execution_engines; e->name; e++) {
> -		if (e->exec_id == 0)
> -			continue;
> -
> -		if (!gem_has_ring(fd, e->exec_id | e->flags))
> -			continue;
> -
> -		if (!gem_can_store_dword(fd, e->exec_id))
> +	for_each_physical_engine(fd, ring) {
> +		if (!gem_can_store_dword(fd, ring))
>   			continue;
>   
> -		if (e->exec_id == I915_EXEC_BSD) {
> -			int is_bsd2 = e->flags != 0;
> -			if (gem_has_bsd2(fd) != is_bsd2)
> -				continue;
> -		}
> -
> -		engines[num_engines++] = e->exec_id | e->flags;
> +		engines[num_engines++] = ring;
>   		if (num_engines == ARRAY_SIZE(engines))
>   			break;
>   	}
> @@ -737,23 +670,9 @@ preempt(int fd, unsigned ring, int num_children, int timeout)
>   	uint32_t ctx[2];
>   
>   	if (ring == ~0u) {
> -		const struct intel_execution_engine *e;
> -
> -		for (e = intel_execution_engines; e->name; e++) {
> -			if (e->exec_id == 0)
> -				continue;
> -
> -			if (!gem_has_ring(fd, e->exec_id | e->flags))
> -				continue;
> -
> -			if (e->exec_id == I915_EXEC_BSD) {
> -				int is_bsd2 = e->flags != 0;
> -				if (gem_has_bsd2(fd) != is_bsd2)
> -					continue;
> -			}
> -
> -			names[num_engines] = e->name;
> -			engines[num_engines++] = e->exec_id | e->flags;
> +		for_each_physical_engine(fd, ring) {
> +			names[num_engines] = e__->name;
> +			engines[num_engines++] = ring;
>   			if (num_engines == ARRAY_SIZE(engines))
>   				break;
>   		}
> 

I would prefer if for_each_physical engine took an engine parameter but 
in the interest of progress:

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.IGT: failure for Iterate over physical engines

[Patchwork]

== Series Details ==

Series: Iterate over physical engines
URL   : https://patchwork.freedesktop.org/series/38746/
State : failure

== Summary ==

Test kms_chv_cursor_fail:
        Subgroup pipe-b-128x128-top-edge:
                dmesg-warn -> PASS       (shard-snb) fdo#105185
Test gem_exec_schedule:
        Subgroup preempt-self-vebox:
                pass       -> FAIL       (shard-apl)
        Subgroup preempt-other-bsd:
                pass       -> FAIL       (shard-apl)
        Subgroup preempt-other-render:
                pass       -> FAIL       (shard-apl)
        Subgroup preempt-self-render:
                pass       -> FAIL       (shard-apl)
        Subgroup preempt-other-blt:
                pass       -> FAIL       (shard-apl)
        Subgroup preempt-other-vebox:
                pass       -> FAIL       (shard-apl)
        Subgroup preempt-self-bsd:
                pass       -> FAIL       (shard-apl)
        Subgroup preempt-self-blt:
                pass       -> FAIL       (shard-apl)
Test gem_exec_async:
        Subgroup concurrent-writes-bsd2:
                pass       -> SKIP       (shard-hsw)
                pass       -> SKIP       (shard-apl)
        Subgroup concurrent-writes-bsd1:
                pass       -> SKIP       (shard-hsw)
                pass       -> SKIP       (shard-apl)
Test perf:
        Subgroup oa-exponents:
                incomplete -> PASS       (shard-apl) fdo#102254
Test perf_pmu:
        Subgroup cpu-hotplug:
                incomplete -> SKIP       (shard-apl) fdo#104965
Test kms_rotation_crc:
        Subgroup sprite-rotation-180:
                fail       -> PASS       (shard-snb) fdo#103925 +1
Test kms_flip:
        Subgroup 2x-plain-flip-ts-check:
                pass       -> FAIL       (shard-hsw) fdo#100368 +1
        Subgroup modeset-vs-vblank-race:
                fail       -> PASS       (shard-hsw) fdo#103060 +1

fdo#105185 https://bugs.freedesktop.org/show_bug.cgi?id=105185
fdo#102254 https://bugs.freedesktop.org/show_bug.cgi?id=102254
fdo#104965 https://bugs.freedesktop.org/show_bug.cgi?id=104965
fdo#103925 https://bugs.freedesktop.org/show_bug.cgi?id=103925
fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060

shard-apl        total:3465 pass:1812 dwarn:1   dfail:0   fail:19  skip:1633 time:12492s
shard-hsw        total:3465 pass:1765 dwarn:1   dfail:0   fail:4   skip:1694 time:11710s
shard-snb        total:3465 pass:1358 dwarn:1   dfail:0   fail:3   skip:2103 time:6706s
Blacklisted hosts:
shard-kbl        total:3447 pass:1892 dwarn:12  dfail:2   fail:29  skip:1511 time:9460s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_984/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx