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

[Tvrtko Ursulin]

On 02/07/2019 10:00, 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.
> 
> Signed-off-by: Ramalingam C <ramalingam.c at intel.com>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>
> ---
>   tests/i915/gem_spin_batch.c | 71 +++++++++++++++++++++++++------------
>   1 file changed, 48 insertions(+), 23 deletions(-)
> 
> diff --git a/tests/i915/gem_spin_batch.c b/tests/i915/gem_spin_batch.c
> index 3b4f90731539..6281fcbb374c 100644
> --- a/tests/i915/gem_spin_batch.c
> +++ b/tests/i915/gem_spin_batch.c
> @@ -32,7 +32,8 @@
>   		     "'%s' != '%s' (%lld not within %d%% tolerance of %lld)\n",\
>   		     #x, #ref, (long long)x, tolerance, (long long)ref)
>   
> -static void spin(int fd, unsigned int engine, unsigned int timeout_sec)
> +static void spin(int fd, const struct intel_execution_engine2 *e2,
> +		 unsigned int timeout_sec)
>   {
>   	const uint64_t timeout_100ms = 100000000LL;
>   	unsigned long loops = 0;
> @@ -41,9 +42,9 @@ static void spin(int fd, unsigned int engine, unsigned int timeout_sec)
>   	struct timespec itv = { };
>   	uint64_t elapsed;
>   
> -	spin = __igt_spin_new(fd, .engine = engine);
> +	spin = __igt_spin_new(fd, .engine = e2->flags);
>   	while ((elapsed = igt_nsec_elapsed(&tv)) >> 30 < timeout_sec) {
> -		igt_spin_t *next = __igt_spin_new(fd, .engine = engine);
> +		igt_spin_t *next = __igt_spin_new(fd, .engine = e2->flags);
>   
>   		igt_spin_set_timeout(spin,
>   				     timeout_100ms - igt_nsec_elapsed(&itv));
> @@ -69,13 +70,14 @@ static void spin(int fd, unsigned int engine, unsigned int timeout_sec)
>   #define RESUBMIT_NEW_CTX     (1 << 0)
>   #define RESUBMIT_ALL_ENGINES (1 << 1)
>   
> -static void spin_resubmit(int fd, unsigned int engine, unsigned int flags)
> +static void spin_resubmit(int fd, const struct intel_execution_engine2 *e2,
> +			  unsigned int flags)
>   {
>   	const uint32_t ctx0 = gem_context_create(fd);
>   	const uint32_t ctx1 = (flags & RESUBMIT_NEW_CTX) ?
>   		gem_context_create(fd) : ctx0;
> -	igt_spin_t *spin = __igt_spin_new(fd, .ctx = ctx0, .engine = engine);
> -	unsigned int other;
> +	igt_spin_t *spin = __igt_spin_new(fd, .ctx = ctx0, .engine = e2->flags);
> +	const struct intel_execution_engine2 *other;
>   
>   	struct drm_i915_gem_execbuffer2 eb = {
>   		.buffer_count = 1,
> @@ -83,16 +85,23 @@ static void spin_resubmit(int fd, unsigned int engine, unsigned int flags)
>   		.rsvd1 = ctx1,
>   	};
>   
> +	igt_assert(gem_context_has_engine_map(fd, 0) ||
> +		   !(flags & RESUBMIT_ALL_ENGINES));
> +
>   	if (flags & RESUBMIT_ALL_ENGINES) {
> -		for_each_physical_engine(fd, other) {
> -			if  (other == engine)
> +		gem_context_set_all_engines(fd, ctx0);
> +		if (ctx0 != ctx1)
> +			gem_context_set_all_engines(fd, ctx1);
> +
> +		for_each_context_engine(fd, ctx1, other) {
> +			if  (other->flags == e2->flags)

Make this comparison based on class:instance please. If there are enough 
call sites consider a helper like gem_is_engine_equal?

>   				continue;
>   
> -			eb.flags = other;
> +			eb.flags = other->flags;
>   			gem_execbuf(fd, &eb);
>   		}
>   	} else {
> -		eb.flags = engine;
> +		eb.flags = e2->flags;
>   		gem_execbuf(fd, &eb);
>   	}
>   
> @@ -115,12 +124,12 @@ static void spin_exit_handler(int sig)
>   
>   static void spin_on_all_engines(int fd, unsigned int timeout_sec)
>   {
> -	unsigned engine;
> +	const struct intel_execution_engine2 *e2;
>   
> -	for_each_physical_engine(fd, engine) {
> +	__for_each_physical_engine(fd, e2) {
>   		igt_fork(child, 1) {
>   			igt_install_exit_handler(spin_exit_handler);
> -			spin(fd, engine, timeout_sec);
> +			spin(fd, e2, timeout_sec);
>   		}
>   	}
>   
> @@ -129,6 +138,7 @@ static void spin_on_all_engines(int fd, unsigned int timeout_sec)
>   
>   igt_main
>   {
> +	const struct intel_execution_engine2 *e2;
>   	const struct intel_execution_engine *e;
>   	int fd = -1;
>   
> @@ -141,20 +151,35 @@ igt_main
>   	}
>   
>   	for (e = intel_execution_engines; e->name; e++) {
> -		igt_subtest_f("basic-%s", e->name)
> -			spin(fd, e->exec_id, 3);
> +		e2 = gem_eb_flags_to_engine(e->exec_id | e->flags);
> +		if (!e2)
> +			continue;

And rebase to catch up with my fix which changes this helper to return 
the object, not the pointer. Skip condition is then e2.flags == -1.

> +
> +		igt_subtest_f("legacy-%s", e->name)
> +			spin(fd, e2, 3);
> +
> +		igt_subtest_f("legacy-resubmit-%s", e->name)
> +			spin_resubmit(fd, e2, 0);
> +
> +		igt_subtest_f("legacy-resubmit-new-%s", e->name)
> +			spin_resubmit(fd, e2, RESUBMIT_NEW_CTX);
> +	}
> +
> +	__for_each_physical_engine(fd, e2) {
> +		igt_subtest_f("%s", e2->name)
> +			spin(fd, e2, 3);
>   
> -		igt_subtest_f("resubmit-%s", e->name)
> -			spin_resubmit(fd, e->exec_id, 0);
> +		igt_subtest_f("resubmit-%s", e2->name)
> +			spin_resubmit(fd, e2, 0);
>   
> -		igt_subtest_f("resubmit-new-%s", e->name)
> -			spin_resubmit(fd, e->exec_id, RESUBMIT_NEW_CTX);
> +		igt_subtest_f("resubmit-new-%s", e2->name)
> +			spin_resubmit(fd, e2, RESUBMIT_NEW_CTX);
>   
> -		igt_subtest_f("resubmit-all-%s", e->name)
> -			spin_resubmit(fd, e->exec_id, RESUBMIT_ALL_ENGINES);
> +		igt_subtest_f("resubmit-all-%s", e2->name)
> +			spin_resubmit(fd, e2, RESUBMIT_ALL_ENGINES);
>   
> -		igt_subtest_f("resubmit-new-all-%s", e->name)
> -			spin_resubmit(fd, e->exec_id,
> +		igt_subtest_f("resubmit-new-all-%s", e2->name)
> +			spin_resubmit(fd, e2,
>   				      RESUBMIT_NEW_CTX |
>   				      RESUBMIT_ALL_ENGINES);
>   	}
> 

The rest is I think okay.

Regards,

Tvrtko