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@xxxxxxxxx>
> > > ---
> > > 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.
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.
/Thomas
