[Intel-gfx] [PATCH i-g-t v5] tests/i915/gem_exec_async: Update with engine discovery

[Tvrtko Ursulin]

On 30/07/2019 04:58, 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]
> v4:
>    In legacy uAPI test path, iterate through for_each_engine [Tvrtko]
> v5:
>    Function for exec_flag comparison [Tvrtko]
> 
> Signed-off-by: Ramalingam C <ramalingam.c at intel.com>
> ---
>   lib/igt_gt.c                | 25 ++++++++++++++++++
>   lib/igt_gt.h                |  1 +
>   tests/i915/gem_exec_async.c | 52 +++++++++++++++++++++++++++----------
>   3 files changed, 64 insertions(+), 14 deletions(-)
> 
> diff --git a/lib/igt_gt.c b/lib/igt_gt.c
> index 78e3cd08925b..f594a46282bc 100644
> --- a/lib/igt_gt.c
> +++ b/lib/igt_gt.c
> @@ -633,3 +633,28 @@ bool gem_ring_has_physical_engine(int fd, unsigned ring)
>   
>   	return gem_has_ring(fd, ring);
>   }
> +
> +bool gem_eb_flags_are_different_engines(unsigned eb_flag1, unsigned eb_flag2)

I think we try to avoid implied int but not sure in this case whether to suggest unsigned int, long or uint64_t. If we are suggesting in the function name that any flags can be passed in perhaps it should be uint64_t and then we filter on the engine bits (flags.. &= I915_EXEC_RING_MASK | (3 << 13)) before checking. Yeah, I think that would be more robust for a generic helper.

And add a doc blurb for this helper since it is non-obvious why we went for different and not same. My thinking was the name different would be clearer to express kind of tri-state nature of this check. (Flags may be different, but engines are not guaranteed to be different.) Have I over-complicated it? Do we need to make it clearer by naming it gem_eb_flags_are_guaranteed_different_engines? :)

> +{
> +	if (eb_flag1 == eb_flag2)
> +		return false;
> +
> +	/* DEFAULT stands for RENDER in legacy uAPI*/
> +	if ((eb_flag1 == I915_EXEC_DEFAULT && eb_flag2 == I915_EXEC_RENDER) ||
> +	     (eb_flag1 == I915_EXEC_RENDER && eb_flag2 == I915_EXEC_DEFAULT))
> +		return false;
> +
> +	/*
> +	 * BSD could be executed on BSD1/BSD2. So BSD and BSD-n could be
> +	 * same engine.
> +	 */
> +	if ((eb_flag1 == I915_EXEC_BSD) &&
> +	    (eb_flag2 & ~I915_EXEC_BSD_MASK) == I915_EXEC_BSD)
> +			return false;
> +
> +	if ((eb_flag2 == I915_EXEC_BSD) &&
> +	    (eb_flag1 & ~I915_EXEC_BSD_MASK) == I915_EXEC_BSD)
> +			return false;

I think this works.

I've also come up with something to merge the two checks, not 100% it's correct or more readable:

if (((flag1 | flag2) & I915_EXEC_RING_MASK)) == I915_EXEC_BSD && // at least one is BSD
    !((flag1 ^ flag2) & I915_EXEC_RING_MASK) && // both are BSD
    (((flag1 | flag2) & (3 << 13))) != 3) // not guaranteed different
	return false;

Would need feeding in some values and checking it works as expected. Probably not worth it since I doubt it is more readable.

> +
> +	return true;
> +}
> diff --git a/lib/igt_gt.h b/lib/igt_gt.h
> index 73b5002a04bd..760b8baefc48 100644
> --- a/lib/igt_gt.h
> +++ b/lib/igt_gt.h
> @@ -101,5 +101,6 @@ extern const struct intel_execution_engine2 {
>   } intel_execution_engines2[];
>   
>   int gem_execbuf_flags_to_engine_class(unsigned int flags);
> +bool gem_eb_flags_are_different_engines(unsigned eb_flag1, unsigned eb_flag2);
>   
>   #endif /* IGT_GT_H */
> diff --git a/tests/i915/gem_exec_async.c b/tests/i915/gem_exec_async.c
> index 9a06af7e29cb..5fdce5ac5b77 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,33 @@ 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_eb_flags_are_different_engines(e2->flags, other))

if (!different)

Regards,

Tvrtko

> +				continue;
>   
> -		if (!gem_can_store_dword(fd, 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 +199,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 +216,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);
>   		}
>   	}
>   
>