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

Tvrtko Ursulin
2019-03-07T16:28:37Z

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