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

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

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.

Andi

> 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