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 15:46, Andi Shyti wrote:
> Hi Tvrtko,
> 
> yes, this patchset has grown and changed many times over the
> review iterations and unfortunately, it's not very obvius.
> 
>>>>> With the new engine query method engines are reachable through
>>>>> an index and context they are combined with.
>>>>>
>>>>> The 'gem_has_ring()' becomes 'gem_context_has_engine()' that
>>>>> requires the index that the engine is mapped within the driver.
>>>>>
>>>>> The previous 'gem_has_ring()' function becomes a wrapper to the new
>>>>> 'gem_context_has_engine()'.
>>>>>
>>>>> Signed-off-by: Andi Shyti <andi.shyti@intel.com>
>>>>> ---
>>>>>     lib/ioctl_wrappers.c | 4 +++-
>>>>>     lib/ioctl_wrappers.h | 4 +++-
>>>>>     2 files changed, 6 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
>>>>> index 39920f8707d2..a2597e282704 100644
>>>>> --- a/lib/ioctl_wrappers.c
>>>>> +++ b/lib/ioctl_wrappers.c
>>>>> @@ -1252,7 +1252,7 @@ void igt_require_gem(int fd)
>>>>>     	igt_require_f(err == 0, "Unresponsive i915/GEM device\n");
>>>>>     }
>>>>> -bool gem_has_ring(int fd, unsigned ring)
>>>>> +bool gem_context_has_engine(int fd, unsigned ring, unsigned ctx)
>>>>>     {
>>>>>     	struct drm_i915_gem_execbuffer2 execbuf;
>>>>>     	struct drm_i915_gem_exec_object2 exec;
>>>>> @@ -1268,6 +1268,8 @@ bool gem_has_ring(int fd, unsigned ring)
>>>>>     	execbuf.buffers_ptr = to_user_pointer(&exec);
>>>>>     	execbuf.buffer_count = 1;
>>>>>     	execbuf.flags = ring;
>>>>> +	execbuf.rsvd1 = ctx;
>>>>> +
>>>>>     	return __gem_execbuf(fd, &execbuf) == -ENOENT;
>>>>>     }
>>>>> diff --git a/lib/ioctl_wrappers.h b/lib/ioctl_wrappers.h
>>>>> index f0be26080da6..446e973b7449 100644
>>>>> --- a/lib/ioctl_wrappers.h
>>>>> +++ b/lib/ioctl_wrappers.h
>>>>> @@ -142,11 +142,13 @@ bool gem_has_exec_fence(int fd);
>>>>>     /* check functions which auto-skip tests by calling igt_skip() */
>>>>>     void gem_require_caching(int fd);
>>>>> -bool gem_has_ring(int fd, unsigned ring);
>>>>> +bool gem_context_has_engine(int fd, unsigned ring, unsigned ctx);
>>>>>     void gem_require_ring(int fd, unsigned ring);
>>>>>     bool gem_has_mocs_registers(int fd);
>>>>>     void gem_require_mocs_registers(int fd);
>>>>> +#define gem_has_ring(fd, ring) gem_context_has_engine(fd, ring, 0)
>>>>> +
>>>>>     /* prime */
>>>>>     struct local_dma_buf_sync {
>>>>>     	uint64_t flags;
>>>>>
>>>>
>>>> I don't understand why this. All current callers of gem_has_ring pass in eb
>>>> flags and not an index so how it can work?
>>>
>>> This is because of patch 3/6 this makes the for_each_engine2()
>>> able to work with new and old api.
>>
>> How does it do that? Maybe I am extra slow today..
> 
> We assume we have the old api (i.e. we use the current
> intel_execution_engines2[] array):
> 
> #define for_each_engine2(fd, ctx) \
>              ...
>                     |---- the for_if() inverts the logic ----------|
>                     V                                              V
>                   for_if (gem_has_engine_topology() || \    <--- false
>                           gem_has_engine(fd, e2__->class, e2__->instance)) <--- true
> 
> gem_has_engine() is the bit that will be called, which translates
> class/instance to eb flag and calls gem_has_ring(fd, flags), which now is
> 
> #define gem_has_ring(fd, ring) gem_context_has_engine(fd, ring, 0) (*)
> 
> gem_context_has_engine() would work exactly as before and assign
> ring to execbuf.flags and '0' to execbuf.rsvd1, nothing changes,
> although the logic is a bit twisted (we still have discussion on
> names with Chris :) ).
> 
> At the same time the "gem_set_context_get_engines()" (which means
> set context and get engines) has returned
> intel_execution_engines2[] and we iterate through the
> preallocated engines.
> 
> It works exactly like for_each_engine(...), but using the new
> "struct intel_execution_engine2" instead of the old "struct
> intel_execution_engine".
> 
> If we have the new uapi, then we don't care, because
> gem_has_engine_topology is true and we move forward:
> 
> #define for_each_engine2(fd, ctx) \
>              ...
>                   for_if (gem_has_engine_topology() || \    <--- true
>                           gem_has_engine(fd, e2__->class, e2__->instance)) <--- does not matter
> 
> "gem_set_context_get_engines()" has returned the
> "intel_active_engines2[]" array that we created by querying the
> driver.
> 
> On the other hand, if you see the subtest "exec-ctx" (patch 6/6),
> we call exactly the same function without goint through the
> definition(*) and gem_context_has_engine(...) would work by
> using the new api:
> 
>     gem_context_has_engine(fd, ++index_map, ctx_id));
> 
> index_map is assigned to execbuf.flags, while ctx_id is assigned
> to execbuf.rsvd1.
> 
> This definition reduces quite some code, because I can use
> gem_context_has_engine(...) for both indexed engines and not.
> 
> I don't know if I made myself clear, but if you want we can also
> take this offline.

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?

Regards,

Tvrtko

>> How is the for_each_engine2 without a helper to get engine flags, or the
>> index variable, supposed to be used if one wants to submit a batch to all
>> engines:
>>
>> for_each_engine2(...)
>> 	eb(engine=??)
>>
>> Both in legacy and engine discovery mode.
>>
>> Regards,
>>
>> Tvrtko
>>
>>> Have I messed up the patch order?
>>>
>>> Andi
>>>
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev