On Wed, Jun 29, 2022 at 11:39:52PM -0700, Zanoni, Paulo R wrote:
> On Wed, 2022-06-29 at 23:08 -0700, Niranjana Vishwanathapura wrote:
> > On Wed, Jun 29, 2022 at 05:33:49PM -0700, Zanoni, Paulo R wrote:
> > > On Sat, 2022-06-25 at 18:49 -0700, Niranjana Vishwanathapura wrote:
> > > > VM_BIND and related uapi definitions
> > > >
> > > > v2: Reduce the scope to simple Mesa use case.
> > > > v3: Expand VM_UNBIND documentation and add
> > > > I915_GEM_VM_BIND/UNBIND_FENCE_VALID
> > > > and I915_GEM_VM_BIND_TLB_FLUSH flags.
> > > > v4: Remove I915_GEM_VM_BIND_TLB_FLUSH flag and add additional
> > > > documentation for vm_bind/unbind.
> > > > v5: Remove TLB flush requirement on VM_UNBIND.
> > > > Add version support to stage implementation.
> > > > v6: Define and use drm_i915_gem_timeline_fence structure for
> > > > all timeline fences.
> > > > v7: Rename I915_PARAM_HAS_VM_BIND to I915_PARAM_VM_BIND_VERSION.
> > > > Update documentation on async vm_bind/unbind and versioning.
> > > > Remove redundant vm_bind/unbind FENCE_VALID flag, execbuf3
> > > > batch_count field and I915_EXEC3_SECURE flag.
> > > >
> > > > Signed-off-by: Niranjana Vishwanathapura <niranjana.vishwanathapura@xxxxxxxxx>
> > > > Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> > > > ---
> > > > Documentation/gpu/rfc/i915_vm_bind.h | 280 +++++++++++++++++++++++++++
> > > > 1 file changed, 280 insertions(+)
> > > > create mode 100644 Documentation/gpu/rfc/i915_vm_bind.h
> > > >
> > > > diff --git a/Documentation/gpu/rfc/i915_vm_bind.h b/Documentation/gpu/rfc/i915_vm_bind.h
> > > > new file mode 100644
> > > > index 000000000000..a93e08bceee6
> > > > --- /dev/null
> > > > +++ b/Documentation/gpu/rfc/i915_vm_bind.h
> > > > @@ -0,0 +1,280 @@
> > > > +/* SPDX-License-Identifier: MIT */
> > > > +/*
> > > > + * Copyright © 2022 Intel Corporation
> > > > + */
> > > > +
> > > > +/**
> > > > + * DOC: I915_PARAM_VM_BIND_VERSION
> > > > + *
> > > > + * VM_BIND feature version supported.
> > > > + * See typedef drm_i915_getparam_t param.
> > > > + *
> > > > + * Specifies the VM_BIND feature version supported.
> > > > + * The following versions of VM_BIND have been defined:
> > > > + *
> > > > + * 0: No VM_BIND support.
> > > > + *
> > > > + * 1: In VM_UNBIND calls, the UMD must specify the exact mappings created
> > > > + * previously with VM_BIND, the ioctl will not support unbinding multiple
> > > > + * mappings or splitting them. Similarly, VM_BIND calls will not replace
> > > > + * any existing mappings.
> > > > + *
> > > > + * 2: The restrictions on unbinding partial or multiple mappings is
> > > > + * lifted, Similarly, binding will replace any mappings in the given range.
> > > > + *
> > > > + * See struct drm_i915_gem_vm_bind and struct drm_i915_gem_vm_unbind.
> > > > + */
> > > > +#define I915_PARAM_VM_BIND_VERSION 57
> > > > +
> > > > +/**
> > > > + * DOC: I915_VM_CREATE_FLAGS_USE_VM_BIND
> > > > + *
> > > > + * Flag to opt-in for VM_BIND mode of binding during VM creation.
> > > > + * See struct drm_i915_gem_vm_control flags.
> > > > + *
> > > > + * The older execbuf2 ioctl will not support VM_BIND mode of operation.
> > > > + * For VM_BIND mode, we have new execbuf3 ioctl which will not accept any
> > > > + * execlist (See struct drm_i915_gem_execbuffer3 for more details).
> > > > + */
> > > > +#define I915_VM_CREATE_FLAGS_USE_VM_BIND (1 << 0)
> > > > +
> > > > +/* VM_BIND related ioctls */
> > > > +#define DRM_I915_GEM_VM_BIND 0x3d
> > > > +#define DRM_I915_GEM_VM_UNBIND 0x3e
> > > > +#define DRM_I915_GEM_EXECBUFFER3 0x3f
> > > > +
> > > > +#define DRM_IOCTL_I915_GEM_VM_BIND DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_GEM_VM_BIND, struct drm_i915_gem_vm_bind)
> > > > +#define DRM_IOCTL_I915_GEM_VM_UNBIND DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_GEM_VM_UNBIND, struct drm_i915_gem_vm_bind)
> > > > +#define DRM_IOCTL_I915_GEM_EXECBUFFER3 DRM_IOWR(DRM_COMMAND_BASE + DRM_I915_GEM_EXECBUFFER3, struct drm_i915_gem_execbuffer3)
> > > > +
> > > > +/**
> > > > + * struct drm_i915_gem_timeline_fence - An input or output timeline fence.
> > > > + *
> > > > + * The operation will wait for input fence to signal.
> > > > + *
> > > > + * The returned output fence will be signaled after the completion of the
> > > > + * operation.
> > > > + */
> > > > +struct drm_i915_gem_timeline_fence {
> > > > + /** @handle: User's handle for a drm_syncobj to wait on or signal. */
> > > > + __u32 handle;
> > > > +
> > > > + /**
> > > > + * @flags: Supported flags are:
> > > > + *
> > > > + * I915_TIMELINE_FENCE_WAIT:
> > > > + * Wait for the input fence before the operation.
> > > > + *
> > > > + * I915_TIMELINE_FENCE_SIGNAL:
> > > > + * Return operation completion fence as output.
> > > > + */
> > > > + __u32 flags;
> > > > +#define I915_TIMELINE_FENCE_WAIT (1 << 0)
> > > > +#define I915_TIMELINE_FENCE_SIGNAL (1 << 1)
> > > > +#define __I915_TIMELINE_FENCE_UNKNOWN_FLAGS (-(I915_TIMELINE_FENCE_SIGNAL << 1))
> > > > +
> > > > + /**
> > > > + * @value: A point in the timeline.
> > > > + * Value must be 0 for a binary drm_syncobj. A Value of 0 for a
> > > > + * timeline drm_syncobj is invalid as it turns a drm_syncobj into a
> > > > + * binary one.
> > > > + */
> > > > + __u64 value;
> > > > +};
> > > > +
> > > > +/**
> > > > + * struct drm_i915_gem_vm_bind - VA to object mapping to bind.
> > > > + *
> > > > + * This structure is passed to VM_BIND ioctl and specifies the mapping of GPU
> > > > + * virtual address (VA) range to the section of an object that should be bound
> > > > + * in the device page table of the specified address space (VM).
> > > > + * The VA range specified must be unique (ie., not currently bound) and can
> > > > + * be mapped to whole object or a section of the object (partial binding).
> > > > + * Multiple VA mappings can be created to the same section of the object
> > > > + * (aliasing).
> > > > + *
> > > > + * The @start, @offset and @length must be 4K page aligned. However the DG2
> > > > + * and XEHPSDV has 64K page size for device local-memory and has compact page
> > > > + * table. On those platforms, for binding device local-memory objects, the
> > > > + * @start must be 2M aligned, @offset and @length must be 64K aligned.
> > > > + * Also, for such mappings, i915 will reserve the whole 2M range for it so as
> > > > + * to not allow multiple mappings in that 2M range (Compact page tables do not
> > > > + * allow 64K page and 4K page bindings in the same 2M range).
> > > > + *
> > > > + * Error code -EINVAL will be returned if @start, @offset and @length are not
> > > > + * properly aligned. In version 1 (See I915_PARAM_VM_BIND_VERSION), error code
> > > > + * -ENOSPC will be returned if the VA range specified can't be reserved.
> > > > + *
> > > > + * VM_BIND/UNBIND ioctl calls executed on different CPU threads concurrently
> > > > + * are not ordered. Furthermore, parts of the VM_BIND operation can be done
> > > > + * asynchronously, if valid @fence is specified.
> > >
> > > Does that mean that if I don't provide @fence, then this ioctl will be
> > > synchronous (i.e., when it returns, the memory will be guaranteed to be
> > > bound)? The text is kinda implying that, but from one of your earlier
> > > replies to Tvrtko, that doesn't seem to be the case. I guess we could
> > > change the text to make this more explicit.
> > >
> >
> > Yes, I thought, if user doesn't specify the out fence, KMD better make
> > the ioctl synchronous by waiting until the binding finishes before
> > returning. Otherwise, UMD has no way to ensure binding is complete and
> > UMD must pass in out fence for VM_BIND calls.
> >
> > But latest comment form Daniel on other thread might suggest something else.
> > Daniel, can you comment?
>
> Whatever we decide, let's make sure it's documented.
>
> >
> > > In addition, previously we had the guarantee that an execbuf ioctl
> > > would wait for all the pending vm_bind operations to finish before
> > > doing anything. Do we still have this guarantee or do we have to make
> > > use of the fences now?
> > >
> >
> > No, we don't have that anymore (execbuf is decoupled from VM_BIND).
> > Execbuf3 submission will not wait for any previous VM_BIND to finish.
> > UMD must pass in VM_BIND out fence as in fence for execbuf3 to ensure
> > that.
>
> Got it, thanks.
>
> >
> > > > + */
> > > > +struct drm_i915_gem_vm_bind {
> > > > + /** @vm_id: VM (address space) id to bind */
> > > > + __u32 vm_id;
> > > > +
> > > > + /** @handle: Object handle */
> > > > + __u32 handle;
> > > > +
> > > > + /** @start: Virtual Address start to bind */
> > > > + __u64 start;
> > > > +
> > > > + /** @offset: Offset in object to bind */
> > > > + __u64 offset;
> > > > +
> > > > + /** @length: Length of mapping to bind */
> > > > + __u64 length;
> > > > +
> > > > + /**
> > > > + * @flags: Supported flags are:
> > > > + *
> > > > + * I915_GEM_VM_BIND_READONLY:
> > > > + * Mapping is read-only.
> > >
> > > Can you please explain what happens when we try to write to a range
> > > that's bound as read-only?
> > >
> >
> > It will be mapped as read-only in device page table. Hence any
> > write access will fail. I would expect a CAT error reported.
>
> What's a CAT error? Does this lead to machine freeze or a GPU hang?
> Let's make sure we document this.
>
Catastrophic error.
> >
> > I am seeing that currently the page table R/W setting is based
> > on whether BO is readonly or not (UMDs can request a userptr
> > BO to readonly). We can make this READONLY here as a subset.
> > ie., if BO is readonly, the mappings must be readonly. If BO
> > is not readonly, then the mapping can be either readonly or
> > not.
> >
> > But if Mesa doesn't have a use for this, then we can remove
> > this flag for now.
> >
>
> I was considering using it for Vulkan's Sparse
> residencyNonResidentStrict, so we map all unbound pages to a read-only
> page. But for that to work, the required behavior would have to be:
> reads all return zero, writes are ignored without any sort of error.
>
> But maybe our hardware provides other ways to implement this, I haven't
> checked yet.
>
I am not sure what the behavior is. Probably writes are not simply ignored,
will check.
Looks like we can remove this flag for now. We can always add it back
later if we need it. Is that Ok with you?
