[Intel-xe] [PATCH] drm/xe: Extend drm_xe_vm_bind_op

[Vivi, Rodrigo]

On Fri, 2023-09-08 at 11:17 +0300, Joonas Lahtinen wrote:
> (+ Thomas who this was discussed with and Faith for original
> reasoning for op[])
> 
> Top-posting to discuss the alternative of having a single VM_BIND per
> operation instead of array of operations.
> 
> Thomas is working on the feature of returning an error from the
> middle
> of the op[] array to the userspace and unrolling the operations plus
> -EINTR
> handling became under discussion.
> 
> With the addition of debug metadata per op (aka. per vma) the
> unrolling and
> -EINTR handling will become further complex.
> 
> Would there be a measurable performance issue if we would simplify
> and
> just to do VM_BIND(op)[] instead of VM_BIND(op[])?
> 
> Unrolling would be much easier and there would be no need for complex
> error
> returning scheme. The GPUVA operation may still split into maximum 3
> separate
> ops but the rest of the unrolling like metadata would need to be only
> dealt with once.

Cc: Paulo and Jose

> 
> Regards, Joonas
> 
> Quoting Rodrigo Vivi (2023-09-08 00:45:53)
> > On Thu, Sep 07, 2023 at 04:51:21PM +0300, Mika Kuoppala wrote:
> > > Rodrigo Vivi <rodrigo.vivi at intel.com> writes:
> > > 
> > > > On Mon, Sep 04, 2023 at 05:46:44PM +0300, Mika Kuoppala wrote:
> > > > > The bind api is extensible but for a single bind op, there
> > > > > is not a mechanism to extend. Add extensions field to
> > > > > struct drm_xe_vm_bind_op.
> > > > 
> > > > But why would you want to extend the operation?
> > > > Except for the destroy ones, every ioctl itself is extensible.
> > > > 
> > > > So, DRM_IOCTL_XE_VM_BIND is extensible. Why would we need to
> > > > get
> > > > prepared to extend the operations themselves? And if we extend
> > > > the operation, what to do with the extension at the ioctl
> > > > level?
> > > > which one has precedence? how to organize that?
> > > > 
> > > 
> > > The intent is to pass debugger metadata as part of particular
> > > vm bind operation. For example on MAP, we could associate
> > > ELF/ISA (relevant parts) as metadata for this bind range.
> > > 
> > > So in vector of binds, we want to tag specific one map (in
> > > between)
> > > with debugger metadata.
> > > 
> > > With extending the XE_VM_BIND itself, this could be possible too
> > > but would then need to deliver index into the vector instead
> > > of carrying the metadata as part of per operation.
> > > As atleast in this example, the extension is heavily tied
> > > into particular OP (map).
> > > 
> > > I take that you mean precedence of VM_BIND vs bind op?
> > > Excellent question and I dont know all the use cases the
> > > vm_bind have to cater. So I can only refer to example above,
> > > VM_BIND extensions would be only in scope to all operations
> > > and vm_bind_op extensions would be tightly coupled to per
> > > operation only.
> > 
> > So, maybe we should do the same union that we do with the
> > ops themselves and make the extension also an array of num_binds ?
> > 
> > > 
> > > Thanks for feedback!
> > > -Mika
> > > 
> > > > > 
> > > > > Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > > > > Cc: Matthew Brost <matthew.brost at intel.com>
> > > > > Cc: Lucas De Marchi <lucas.demarchi at intel.com>
> > > > > Cc: Francois Dugast <francois.dugast at intel.com>
> > > > > Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> > > > > Cc: Dominik Grzegorzek <dominik.grzegorzek at intel.com>
> > > > > Signed-off-by: Mika Kuoppala <mika.kuoppala at linux.intel.com>
> > > > > ---
> > > > >  include/uapi/drm/xe_drm.h | 3 +++
> > > > >  1 file changed, 3 insertions(+)
> > > > > 
> > > > > diff --git a/include/uapi/drm/xe_drm.h
> > > > > b/include/uapi/drm/xe_drm.h
> > > > > index 86f16d50e9cc..5c6c86f5e5fc 100644
> > > > > --- a/include/uapi/drm/xe_drm.h
> > > > > +++ b/include/uapi/drm/xe_drm.h
> > > > > @@ -552,6 +552,9 @@ struct drm_xe_vm_destroy {
> > > > >  };
> > > > >  
> > > > >  struct drm_xe_vm_bind_op {
> > > > > +  /** @extensions: Pointer to the first extension struct, if
> > > > > any */
> > > > > +  __u64 extensions;
> > > > > +
> > > > >    /**
> > > > >     * @obj: GEM object to operate on, MBZ for MAP_USERPTR,
> > > > > MBZ for UNMAP
> > > > >     */
> > > > > -- 
> > > > > 2.34.1
> > > > >