Re: [Intel-gfx] [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 
Cc: Tvrtko Ursulin 
---
  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, );

@@ -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 

[Intel-gfx] [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 
Cc: Tvrtko Ursulin 
---
 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, );
@@ -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
-