[igt-dev] [PATCH i-g-t] tests/i915/gem_exec_async: Update with engine discovery

[Tvrtko Ursulin]

On 24/07/2019 03:21, Ramalingam C wrote:
> Legacy execbuf abi tests are prefixed with legacy. New test are added to
> run on physical engines accessed through engine discovery.
> 
> So legacy tests run on the unconfigured (with engine map) context and
> use a new helper gem_eb_flags_to_engine to look up the engine from the
> intel_execution_engines2 static list. This is only to enable the core
> test code to be shared.
> 
> Places where new contexts are created had to be updated to either
> equally configure the contexts or not.
> 
> v2:
>    retained the test as it is for legacy uapi testing and duplciated for
> 	new engine discovery [Tvrtko]
> v3:
>    Few nits addressed [Tvrtko]
> 
> Signed-off-by: Ramalingam C <ramalingam.c at intel.com>
> ---
>   tests/i915/gem_exec_async.c | 55 +++++++++++++++++++++++++++----------
>   1 file changed, 41 insertions(+), 14 deletions(-)
> 
> diff --git a/tests/i915/gem_exec_async.c b/tests/i915/gem_exec_async.c
> index 9a06af7e29cb..de3652da452a 100644
> --- a/tests/i915/gem_exec_async.c
> +++ b/tests/i915/gem_exec_async.c
> @@ -80,9 +80,10 @@ static void store_dword(int fd, unsigned ring,
>   	gem_close(fd, obj[1].handle);
>   }
>   
> -static void one(int fd, unsigned ring, uint32_t flags)
> +static void one(int fd, const struct intel_execution_engine2 *e2, bool legacy)
>   {
>   	const int gen = intel_gen(intel_get_drm_devid(fd));
> +	const struct intel_execution_engine2 *other2;
>   	struct drm_i915_gem_exec_object2 obj[2];
>   #define SCRATCH 0
>   #define BATCH 1
> @@ -138,20 +139,36 @@ static void one(int fd, unsigned ring, uint32_t flags)
>   	memset(&execbuf, 0, sizeof(execbuf));
>   	execbuf.buffers_ptr = to_user_pointer(obj);
>   	execbuf.buffer_count = 2;
> -	execbuf.flags = ring | flags;
> +	execbuf.flags = e2->flags;
>   	igt_require(__gem_execbuf(fd, &execbuf) == 0);
>   	gem_close(fd, obj[BATCH].handle);
>   
>   	i = 0;
> -	for_each_physical_engine(fd, other) {
> -		if (other == ring)
> -			continue;
> +	if (legacy) {
> +		for_each_engine(fd, other) {
> +			if (!gem_ring_has_physical_engine(fd, other))
> +				continue;

I think we shouldn't mention physical on this branch at all. Idea was 
purely to exercise legacy eb flags. Maybe we would need some magic to 
avoid other being equal to engine even if the flags are different (think 
DEFAULT/RENDER and BSD/BSD1-2).

>   
> -		if (!gem_can_store_dword(fd, other))
> -			continue;
> +			if (e2->flags == other)
> +				continue;
> +
> +			if (!gem_can_store_dword(fd, other))
> +				continue;
> +
> +			store_dword(fd, other, obj[SCRATCH].handle, 4*i, i);
> +			i++;
> +		}
> +	} else {
> +		__for_each_physical_engine(fd, other2) {
> +			if (gem_engine_is_equal(e2, other2))
> +				continue;
>   
> -		store_dword(fd, other, obj[SCRATCH].handle, 4*i, i);
> -		i++;
> +			if (!gem_class_can_store_dword(fd, other2->class))
> +				continue;
> +
> +			store_dword(fd, other2->flags, obj[SCRATCH].handle, 4*i, i);
> +			i++;
> +		}
>   	}
>   
>   	*batch = MI_BATCH_BUFFER_END;
> @@ -185,7 +202,9 @@ static bool has_async_execbuf(int fd)
>   
>   igt_main
>   {
> +	const struct intel_execution_engine2 *e2;
>   	const struct intel_execution_engine *e;
> +	struct intel_execution_engine2 e2__;
>   	int fd = -1;
>   
>   	igt_skip_on_simulation();
> @@ -200,14 +219,22 @@ igt_main
>   	}
>   
>   	for (e = intel_execution_engines; e->name; e++) {
> -		/* default exec-id is purely symbolic */
> -		if (e->exec_id == 0)
> +		e2__ = gem_eb_flags_to_engine(e->exec_id | e->flags);
> +		if (e2__.flags == -1)
>   			continue;
> +		e2 = &e2__;
>   
> -		igt_subtest_f("concurrent-writes-%s", e->name) {
> +		igt_subtest_f("legacy-concurrent-writes-%s", e2->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);
> +			igt_require(gem_class_can_store_dword(fd, e2->class));
> +			one(fd, e2, true);
> +		}
> +	}
> +
> +	__for_each_physical_engine(fd, e2) {
> +		igt_subtest_f("concurrent-writes-%s", e2->name) {
> +			igt_require(gem_class_can_store_dword(fd, e2->class));
> +			one(fd, e2, false);
>   		}
>   	}
>   
> 

The rest looks good to me.

Regards,

Tvrtko