[Intel-gfx] [RFC 06/10] drm/i915/vm_bind: Add I915_GEM_EXECBUFFER3 ioctl

Sat Jul 9 20:23:56 UTC 2022

On Fri, Jul 08, 2022 at 07:37:30AM -0700, Hellstrom, Thomas wrote:
>Hi,
>
>On Fri, 2022-07-08 at 06:47 -0700, Niranjana Vishwanathapura wrote:
>> On Thu, Jul 07, 2022 at 07:41:54AM -0700, Hellstrom, Thomas wrote:
>> > On Fri, 2022-07-01 at 15:50 -0700, Niranjana Vishwanathapura wrote:
>> > > Add new execbuf3 ioctl (I915_GEM_EXECBUFFER3) which only
>> > > works in vm_bind mode. The vm_bind mode only works with
>> > > this new execbuf3 ioctl.
>> > >
>> > > The new execbuf3 ioctl will not have any execlist
>> >
>> > I understand this that you mean there is no list of objects to
>> > validate
>> > attached to the drm_i915_gem_execbuffer3 structure rather than that
>> > the
>> > execlists submission backend is never used. Could we clarify this
>> > to
>> > avoid confusion.
>>
>> Yah, side effect of overloading the word 'execlist' for multiple
>> things.
>> Yah, I meant, no list of objects to validate. I agree, we need to
>> clarify
>> that here.
>>
>> >
>> >
>> > >  support
>> > > and all the legacy support like relocations etc are removed.
>> > >
>> > > Signed-off-by: Niranjana Vishwanathapura
>> > > <niranjana.vishwanathapura at intel.com>
>> > > ---
>> > >  drivers/gpu/drm/i915/Makefile                 |    1 +
>> > >  .../gpu/drm/i915/gem/i915_gem_execbuffer.c    |    5 +
>> > >  .../gpu/drm/i915/gem/i915_gem_execbuffer3.c   | 1029
>> > > +++++++++++++++++
>> > >  drivers/gpu/drm/i915/gem/i915_gem_ioctls.h    |    2 +
>> > >  drivers/gpu/drm/i915/i915_driver.c            |    1 +
>> > >  include/uapi/drm/i915_drm.h                   |   67 +-
>> > >  6 files changed, 1104 insertions(+), 1 deletion(-)
>> > >  create mode 100644
>> > > drivers/gpu/drm/i915/gem/i915_gem_execbuffer3.c
>> > >
>> > > diff --git a/drivers/gpu/drm/i915/Makefile
>> > > b/drivers/gpu/drm/i915/Makefile
>> > > index 4e1627e96c6e..38cd1c5bc1a5 100644
>> > > --- a/drivers/gpu/drm/i915/Makefile
>> > > +++ b/drivers/gpu/drm/i915/Makefile
>> > > @@ -148,6 +148,7 @@ gem-y += \
>> > >         gem/i915_gem_dmabuf.o \
>> > >         gem/i915_gem_domain.o \
>> > >         gem/i915_gem_execbuffer.o \
>> > > +       gem/i915_gem_execbuffer3.o \
>> > >         gem/i915_gem_internal.o \
>> > >         gem/i915_gem_object.o \
>> > >         gem/i915_gem_lmem.o \
>> > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
>> > > b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
>> > > index b7b2c14fd9e1..37bb1383ab8f 100644
>> > > --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
>> > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
>> > > @@ -782,6 +782,11 @@ static int eb_select_context(struct
>> > > i915_execbuffer *eb)
>> > >         if (unlikely(IS_ERR(ctx)))
>> > >                 return PTR_ERR(ctx);
>> > >
>> > > +       if (ctx->vm->vm_bind_mode) {
>> > > +               i915_gem_context_put(ctx);
>> > > +               return -EOPNOTSUPP;
>> > > +       }
>> > > +
>> > >         eb->gem_context = ctx;
>> > >         if (i915_gem_context_has_full_ppgtt(ctx))
>> > >                 eb->invalid_flags |= EXEC_OBJECT_NEEDS_GTT;
>> > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer3.c
>> > > b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer3.c
>> > > new file mode 100644
>> > > index 000000000000..13121df72e3d
>> > > --- /dev/null
>> > > +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer3.c
>> > > @@ -0,0 +1,1029 @@
>> > > +// SPDX-License-Identifier: MIT
>> > > +/*
>> > > + * Copyright © 2022 Intel Corporation
>> > > + */
>> > > +
>> > > +#include <linux/dma-resv.h>
>> > > +#include <linux/sync_file.h>
>> > > +#include <linux/uaccess.h>
>> > > +
>> > > +#include <drm/drm_syncobj.h>
>> > > +
>> > > +#include "gt/intel_context.h"
>> > > +#include "gt/intel_gpu_commands.h"
>> > > +#include "gt/intel_gt.h"
>> > > +#include "gt/intel_gt_pm.h"
>> > > +#include "gt/intel_ring.h"
>> > > +
>> > > +#include "i915_drv.h"
>> > > +#include "i915_file_private.h"
>> > > +#include "i915_gem_context.h"
>> > > +#include "i915_gem_ioctls.h"
>> > > +#include "i915_gem_vm_bind.h"
>> > > +#include "i915_trace.h"
>> > > +
>> > > +#define __EXEC3_ENGINE_PINNED          BIT_ULL(32)
>> > > +#define __EXEC3_INTERNAL_FLAGS         (~0ull << 32)
>> > > +
>> > > +/* Catch emission of unexpected errors for CI! */
>> > > +#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)
>> > > +#undef EINVAL
>> > > +#define EINVAL ({ \
>> > > +       DRM_DEBUG_DRIVER("EINVAL at %s:%d\n", __func__,
>> > > __LINE__); \
>> > > +       22; \
>> > > +})
>> > > +#endif
>> > > +
>> > > +/**
>> > > + * DOC: User command execution with execbuf3 ioctl
>> > > + *
>> > > + * A VM in VM_BIND mode will not support older execbuf mode of
>> > > binding.
>> > > + * The execbuf ioctl handling in VM_BIND mode differs
>> > > significantly
>> > > from the
>> > > + * older execbuf2 ioctl (See struct drm_i915_gem_execbuffer2).
>> > > + * Hence, a new execbuf3 ioctl has been added to support VM_BIND
>> > > mode. (See
>> > > + * struct drm_i915_gem_execbuffer3). The execbuf3 ioctl will not
>> > > accept any
>> > > + * execlist. Hence, no support for implicit sync.
>> > > + *
>> > > + * The new execbuf3 ioctl only works in VM_BIND mode and the
>> > > VM_BIND
>> > > mode only
>> > > + * works with execbuf3 ioctl for submission.
>> > > + *
>> > > + * The execbuf3 ioctl directly specifies the batch addresses
>> > > instead
>> > > of as
>> > > + * object handles as in execbuf2 ioctl. The execbuf3 ioctl will
>> > > also
>> > > not
>> > > + * support many of the older features like in/out/submit fences,
>> > > fence array,
>> > > + * default gem context etc. (See struct
>> > > drm_i915_gem_execbuffer3).
>> > > + *
>> > > + * In VM_BIND mode, VA allocation is completely managed by the
>> > > user
>> > > instead of
>> > > + * the i915 driver. Hence all VA assignment, eviction are not
>> > > applicable in
>> > > + * VM_BIND mode. Also, for determining object activeness,
>> > > VM_BIND
>> > > mode will not
>> > > + * be using the i915_vma active reference tracking. It will
>> > > instead
>> > > check the
>> > > + * dma-resv object's fence list for that.
>> > > + *
>> > > + * So, a lot of code supporting execbuf2 ioctl, like
>> > > relocations, VA
>> > > evictions,
>> > > + * vma lookup table, implicit sync, vma active reference
>> > > tracking
>> > > etc., are not
>> > > + * applicable for execbuf3 ioctl.
>> > > + */
>> > > +
>> > > +struct eb_fence {
>> > > +       struct drm_syncobj *syncobj; /* Use with ptr_mask_bits()
>> > > */
>> > > +       struct dma_fence *dma_fence;
>> > > +       u64 value;
>> > > +       struct dma_fence_chain *chain_fence;
>> > > +};
>> > > +
>> > > +struct i915_execbuffer {
>> > > +       struct drm_i915_private *i915; /** i915 backpointer */
>> > > +       struct drm_file *file; /** per-file lookup tables and
>> > > limits
>> > > */
>> > > +       struct drm_i915_gem_execbuffer3 *args; /** ioctl
>> > > parameters
>> > > */
>> > > +
>> > > +       struct intel_gt *gt; /* gt for the execbuf */
>> > > +       struct intel_context *context; /* logical state for the
>> > > request */
>> > > +       struct i915_gem_context *gem_context; /** caller's
>> > > context */
>> > > +
>> > > +       /** our requests to build */
>> > > +       struct i915_request *requests[MAX_ENGINE_INSTANCE + 1];
>> > > +
>> > > +       /** used for excl fence in dma_resv objects when > 1 BB
>> > > submitted */
>> > > +       struct dma_fence *composite_fence;
>> > > +
>> > > +       struct i915_gem_ww_ctx ww;
>> > > +
>> > > +       /* number of batches in execbuf IOCTL */
>> > > +       unsigned int num_batches;
>> > > +
>> > > +       u64 batch_addresses[MAX_ENGINE_INSTANCE + 1];
>> > > +       /** identity of the batch obj/vma */
>> > > +       struct i915_vma *batches[MAX_ENGINE_INSTANCE + 1];
>> > > +
>> > > +       struct eb_fence *fences;
>> > > +       unsigned long num_fences;
>> > > +};
>> >
>> > Kerneldoc structures please.
>> >
>> > It seems we are duplicating a lot of code from i915_execbuffer.c.
>> > Did
>> > you consider
>> >
>> > struct i915_execbuffer3 {
>> > ...
>> > };
>> >
>> > struct i915_execbuffer2 {
>> >        struct i915_execbuffer3 eb3;
>> >        ...
>> >        [members that are not common]
>> > };
>> >
>> > Allowing execbuffer2 to share the execbuffer3 code to some extent.
>> > Not sure about the gain at this point though. My worry would be
>> > that fo
>> > r example fixes might be applied to one file and not the other.
>>
>> I have added a TODO in the cover letter of this patch series to share
>> the code between execbuf2 and execbuf3.
>> But, I am not sure to what extent. Execbuf3 is much leaner than
>> execbuf2
>> and we don't want to make it bad by forcing code sharing with legacy
>> path.
>> We can perhaps abstract out some functions which takes specific
>> arguments
>> (instead of 'eb'), that way we can keep these structures separate and
>> still
>> share some code.
>
>
>Fully agree we shouldn't make eb3 code more complicated because of eb2.
>My question was more of using i915_execbuffer3 and its functions as a
>"base class" and subclass it for eb2, eb2 adding and implementing
>additional functionality it needs.

Yah, this was Daniel's feedback before the new execbuf3 was proposed.
A base eb class derived to execlist mode and vm_bind mode eb classes.

I think that this will require lot of changes in the legacy execbuf2
code, which we are now not touching for VM_BIND mode. Hence, I am not
sure if it is worth it.

>
>But OTOH I just learned we've might have been asked not to share any
>code between those two from drm maintainers, so need to dig up that
>discussion.

Yah, that is what I understood from the VM_BIND design feedback.
Here I am only thinking about possiblity having some common helper
functions that both can make use of (like eb_pin/unpin_engine etc).

Niranjana

>
>/Thomas
>
>
>