Re: [igt-dev] [RFC PATCH v10 4/6] lib: ioctl_wrappers: reach engines by index as well

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 07/03/2019 17:17, Andi Shyti wrote:
>> I understand the loop itself works, but I wanted to find out how do I write
>> a test which uses it and actually submits work.
>>
>> for_each_engine2(fd, ctx) {
>> 	...
>> 	eb.flags = ???;
>> 	eb.rsvd1 = ctx;
>>
>> 	gem_execbuf(fd, &eb);
>> }
>>
>> What do I replace ??? with so the test works on old and new kernels?
> 
> I guess it would be:
> 
>    index_map = 0;
>    for_each_engine2(fd, ctx) {
>        ...
>        eb.flags = ++index_map;
>        eb.rsvd1 = ctx;
> 
>        gem_execbuf(fd, &eb);
>    }
> 
> for_each_engine2 is just responsible for iterating through
> engines. (*)
> 
> while for the current api it would be:
> 
>    for_each_engine2(fd, 0) {
>        eb.flags = gem_class_instance_to_eb_flags(fd, e2__->class, e2__->instance));
>        eb.rsvd1 = 0;
>    }

So as I was pointing out it is not possible to write a test which works 
with same code on both old and new kernels? Which makes me ask - what is 
the point of making the iterator backward compatible?

IMHO we either drop the backward compatible goal or make some additions 
to enable writing truly agnostic tests. Maybe along the lines of:

for_each_engine2(fd, ctx, i) {
	...
	gem_set_execbuf_engine(&eb, fd, ctx, e2, i);
	...

    	gem_execbuf(fd, &eb);
}

void gem_set_execbuf_engine(...)
{
	if (gem_has_discovered_engines() /* simple pointer check, no ioctl */) {
		eb->rsvd1 = ctx;
		eb->flags |= i;
	} else {
		eb->flags |= gem_class_instance_to_eb_flags(...);
	}
}

A bit longish parameter list for the helper but I don't have better 
ideas at the moment.

Regards,

Tvrtko

> There definitely are better way for doing the
> for_each_engine(...) but I think this is the most consistent to
> the way things are done now.
> 
> In my opinion, next step would be, indeed, to get rid of all
> legacy and have a single for_each_engine(...) that works with
> everything.
> 
> But for this, at my current understanding, we need some
> re-architecturing of igt (and possibly using dynamic lists of
> engines as Chris pointed out).
> 
> Once this patch gets in, I can start looking at the next steps,
> but in one go, we would get everything in only at "[PATCH v1174]" :)
> 
> Andi
> 
> (*) Some extra code would be required to increment index_map
> into the for_each_engine2(), Chris was suggesting, indeed, an
> iter structure (or we can use some other 'C' tricks that would
> make the code quite unreadable).
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev