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

[Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>]

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@xxxxxxxxxxxxxxxxxx>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> ---
>   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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx