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

[Andi Shyti <andi.shyti@intel.com>]

Hi Tvrtko,

> +const struct intel_execution_engine2 *
> +gem_eb_flags_to_engine(unsigned int flags)
> +{
> +	const struct intel_execution_engine2 *e2;
> +
> +	__for_each_static_engine(e2) {
> +		if (e2->flags == flags)
> +			return e2;
> +	}
> +
> +	return NULL;
> +}

the amount of "helpers" is getting almost unbearable for a simple
mind like mine.

This means that we can get rid of

 gem_execbuf_flags_to_engine_class
 gem_ring_is_physical_engine
 gem_ring_has_physical_engine

in igt_gt.c, right?

> +bool gem_context_has_engine_map(int fd, uint32_t ctx)
> +{
> +	struct drm_i915_gem_context_param param = {
> +		.param = I915_CONTEXT_PARAM_ENGINES,
> +		.ctx_id = ctx
> +	};
> +	int ret;
> +
> +	ret = __gem_context_get_param(fd, &param);
> +	igt_assert_eq(ret, 0);
> +
> +	return param.size;

a small nitpick: bool to me means '0' or '1'.

So, if you do 'return param.size', I would call the function
gem_context_get_param_size, otherwise I would return
'!!param.size' and keep it bool.

(We are also somehow abusing on the size definition of bool in
C99/C17 or previous.)

I'm not asking you to change it, but it would make me happier :)

> +}
> diff --git a/lib/i915/gem_engine_topology.h b/lib/i915/gem_engine_topology.h
> index 2415fd1e379b..b175483fac1c 100644
> --- a/lib/i915/gem_engine_topology.h
> +++ b/lib/i915/gem_engine_topology.h
> @@ -53,6 +53,11 @@ int gem_context_lookup_engine(int fd, uint64_t engine, uint32_t ctx_id,
>  
>  void gem_context_set_all_engines(int fd, uint32_t ctx);
>  
> +bool gem_context_has_engine_map(int fd, uint32_t ctx);
> +
> +const struct intel_execution_engine2 *
> +gem_eb_flags_to_engine(unsigned int flags);
> +
>  #define __for_each_static_engine(e__) \
>  	for ((e__) = intel_execution_engines2; (e__)->name; (e__)++)
>  
> diff --git a/tests/i915/gem_ctx_switch.c b/tests/i915/gem_ctx_switch.c
> index 647911d4c42e..407905de2d34 100644
> --- a/tests/i915/gem_ctx_switch.c
> +++ b/tests/i915/gem_ctx_switch.c
> @@ -55,7 +55,7 @@ static double elapsed(const struct timespec *start, const struct timespec *end)
>  
>  static int measure_qlen(int fd,
>  			struct drm_i915_gem_execbuffer2 *execbuf,
> -			unsigned int *engine, unsigned int nengine,
> +			const struct intel_engine_data *engines,
>  			int timeout)
>  {
>  	const struct drm_i915_gem_exec_object2 * const obj =
> @@ -63,15 +63,17 @@ static int measure_qlen(int fd,
>  	uint32_t ctx[64];
>  	int min = INT_MAX, max = 0;
>  
> -	for (int i = 0; i < ARRAY_SIZE(ctx); i++)
> +	for (int i = 0; i < ARRAY_SIZE(ctx); i++) {
>  		ctx[i] = gem_context_create(fd);
> +		gem_context_set_all_engines(fd, ctx[i]);
> +	}
>  
> -	for (unsigned int n = 0; n < nengine; n++) {
> +	for (unsigned int n = 0; n < engines->nengines; n++) {
>  		uint64_t saved = execbuf->flags;
>  		struct timespec tv = {};
>  		int q;
>  
> -		execbuf->flags |= engine[n];
> +		execbuf->flags |= engines->engines[n].flags;
>  
>  		for (int i = 0; i < ARRAY_SIZE(ctx); i++) {
>  			execbuf->rsvd1 = ctx[i];
> @@ -90,7 +92,8 @@ static int measure_qlen(int fd,
>  		 * Be conservative and aim not to overshoot timeout, so scale
>  		 * down by 8 for hopefully a max of 12.5% error.
>  		 */
> -		q = ARRAY_SIZE(ctx) * timeout * 1e9 / igt_nsec_elapsed(&tv) / 8 + 1;
> +		q = ARRAY_SIZE(ctx) * timeout * 1e9 / igt_nsec_elapsed(&tv) /
> +		    8 + 1;

I don't know whether it's me who is paranoic, but the change
above doesn't match the commit log.

>  		if (q < min)
>  			min = q;
>  		if (q > max)
> @@ -107,7 +110,7 @@ static int measure_qlen(int fd,
>  }
>  
>  static void single(int fd, uint32_t handle,
> -		   const struct intel_execution_engine *e,
> +		   const struct intel_execution_engine2 *e2,
>  		   unsigned flags,
>  		   const int ncpus,
>  		   int timeout)
> @@ -125,13 +128,14 @@ static void single(int fd, uint32_t handle,
>  	shared = mmap(NULL, 4096, PROT_WRITE, MAP_SHARED | MAP_ANON, -1, 0);
>  	igt_assert(shared != MAP_FAILED);
>  
> -	gem_require_ring(fd, e->exec_id | e->flags);
> -
>  	for (n = 0; n < 64; n++) {
>  		if (flags & QUEUE)
>  			contexts[n] = gem_queue_create(fd);
>  		else
>  			contexts[n] = gem_context_create(fd);
> +
> +		if (gem_context_has_engine_map(fd, 0))
> +			gem_context_set_all_engines(fd, contexts[n]);
>  	}
>  
>  	memset(&obj, 0, sizeof(obj));
> @@ -152,12 +156,12 @@ static void single(int fd, uint32_t handle,
>  	execbuf.buffers_ptr = to_user_pointer(&obj);
>  	execbuf.buffer_count = 1;
>  	execbuf.rsvd1 = contexts[0];
> -	execbuf.flags = e->exec_id | e->flags;
> +	execbuf.flags = e2->flags;
>  	execbuf.flags |= LOCAL_I915_EXEC_HANDLE_LUT;
>  	execbuf.flags |= LOCAL_I915_EXEC_NO_RELOC;
>  	igt_require(__gem_execbuf(fd, &execbuf) == 0);
>  	if (__gem_execbuf(fd, &execbuf)) {
> -		execbuf.flags = e->exec_id | e->flags;
> +		execbuf.flags = e2->flags;
>  		reloc.target_handle = obj.handle;
>  		gem_execbuf(fd, &execbuf);
>  	}
> @@ -190,7 +194,8 @@ static void single(int fd, uint32_t handle,
>  		clock_gettime(CLOCK_MONOTONIC, &now);
>  
>  		igt_info("[%d] %s: %'u cycles: %.3fus%s\n",
> -			 child, e->name, count, elapsed(&start, &now)*1e6 / count,
> +			 child, e2->name, count,
> +			 elapsed(&start, &now) * 1e6 / count,
>  			 flags & INTERRUPTIBLE ? " (interruptible)" : "");
>  
>  		shared[child].elapsed = elapsed(&start, &now);
> @@ -209,7 +214,7 @@ static void single(int fd, uint32_t handle,
>  		}
>  
>  		igt_info("Total %s: %'lu cycles: %.3fus%s\n",
> -			 e->name, total, max*1e6 / total,
> +			 e2->name, total, max*1e6 / total,
>  			 flags & INTERRUPTIBLE ? " (interruptible)" : "");
>  	}
>  
> @@ -223,25 +228,20 @@ static void all(int fd, uint32_t handle, unsigned flags, int timeout)
>  {
>  	struct drm_i915_gem_execbuffer2 execbuf;
>  	struct drm_i915_gem_exec_object2 obj[2];
> -	unsigned int engine[16], e;
> -	const char *name[16];
> +	struct intel_engine_data engines = { };
>  	uint32_t contexts[65];
> -	unsigned int nengine;
>  	int n, qlen;
>  
> -	nengine = 0;
> -	for_each_physical_engine(fd, e) {
> -		engine[nengine] = e;
> -		name[nengine] = e__->name;
> -		nengine++;
> -	}
> -	igt_require(nengine);
> +	engines = intel_init_engine_list(fd, 0);
> +	igt_require(engines.nengines);

Off-topic:
This I guess can be the "flags" mapping that Chris was suggesting
once, I guess we can achieve that by just doing the above without
adding helpers (which would drive crazy people like me).

The rest of the patch I trust you it works :)
(however the devotion to whatever is legacy doesn't leave me with
a good taste after all the work done)

You can add my Reviewed-by: Andi Shyti <andi.shyti@intel.com>

Thanks, this patch clarifies a few more things as well,
Andi
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev