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

[Tvrtko Ursulin]

On 27/06/2019 21:15, Andi Shyti wrote:
> 
> 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?

If/when there are no callers left we can like everything. I dont' know 
if that is the case right now.

>> +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.

Some people would then complain !! was not needed. ~o~

And actually I think I want to remove the distinction of no map and map 
with no engines for the callers of this helpers. So returning size would 
not work for that.

> (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 :)

I don't understand the issue with size definition. Size is u32 - will 
not implicit conversion to bool just work?

> 
>> +}
>> 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.

What do you mean:

"""
Also beware of drive-by formatting changes.
"""

;)

File to many lines falling of 80 char so I tidied it alongside.

> 
>>   		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).

I don't remember what was this flags mapping. Something to return a list 
of eb->flags for all physical engines? Not sure I like that since flags 
carry is very limited information. Having a list of engine objects 
sounds much better.

> 
> 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 at intel.com>
> 
> Thanks, this patch clarifies a few more things as well,

Thanks!

Regards,

Tvrtko