[Intel-gfx] [PATCH v3 3/3] drm/doc/rfc: VM_BIND uapi definition
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Thu Jun 23 08:27:22 UTC 2022
On 22/06/2022 17:44, Niranjana Vishwanathapura wrote:
> On Wed, Jun 22, 2022 at 04:57:17PM +0100, Tvrtko Ursulin wrote:
>>
>> On 22/06/2022 16:12, Niranjana Vishwanathapura wrote:
>>> On Wed, Jun 22, 2022 at 09:10:07AM +0100, Tvrtko Ursulin wrote:
>>>>
>>>> On 22/06/2022 04:56, 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.
>>>>>
>>>>> Signed-off-by: Niranjana Vishwanathapura
>>>>> <niranjana.vishwanathapura at intel.com>
>>>>> ---
>>>>> Documentation/gpu/rfc/i915_vm_bind.h | 243
>>>>> +++++++++++++++++++++++++++
>>>>> 1 file changed, 243 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..fa23b2d7ec6f
>>>>> --- /dev/null
>>>>> +++ b/Documentation/gpu/rfc/i915_vm_bind.h
>>>>> @@ -0,0 +1,243 @@
>>>>> +/* SPDX-License-Identifier: MIT */
>>>>> +/*
>>>>> + * Copyright © 2022 Intel Corporation
>>>>> + */
>>>>> +
>>>>> +/**
>>>>> + * DOC: I915_PARAM_HAS_VM_BIND
>>>>> + *
>>>>> + * VM_BIND feature availability.
>>>>> + * See typedef drm_i915_getparam_t param.
>>>>> + */
>>>>> +#define I915_PARAM_HAS_VM_BIND 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_vm_bind_fence - Bind/unbind completion
>>>>> notification.
>>>>> + *
>>>>> + * A timeline out fence for vm_bind/unbind completion notification.
>>>>> + */
>>>>> +struct drm_i915_gem_vm_bind_fence {
>>>>> + /** @handle: User's handle for a drm_syncobj to signal. */
>>>>> + __u32 handle;
>>>>> +
>>>>> + /** @rsvd: Reserved, MBZ */
>>>>> + __u32 rsvd;
>>>>> +
>>>>> + /**
>>>>> + * @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 should 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 should be 2M aligned, @offset and @length should be 64K
>>>>> aligned.
>>>>
>>>> Should some error codes be documented and has the ability to
>>>> programmatically probe the alignment restrictions been considered?
>>>>
>>>
>>> Currently what we have internally is that -EINVAL is returned if the
>>> sart, offset
>>> and length are not aligned. If the specified mapping already exits,
>>> we return
>>> -EEXIST. If there are conflicts in the VA range and VA range can't be
>>> reserved,
>>> then -ENOSPC is returned. I can add this documentation here. But I am
>>> worried
>>> that there will be more suggestions/feedback about error codes while
>>> reviewing
>>> the code patch series, and we have to revisit it again.
>>
>> I'd still suggest documenting those three. It makes sense to explain
>> to userspace what behaviour they will see if they get it wrong.
>>
>
> Ok.
>
>>>>> + * Also, on those platforms, it is not allowed to bind an device
>>>>> local-memory
>>>>> + * object and a system memory object in a single 2M section of VA
>>>>> range.
>>>>
>>>> Text should be clear whether "not allowed" means there will be an
>>>> error returned, or it will appear to work but bad things will happen.
>>>>
>>>
>>> Yah, error returned, will fix.
>>>
>>>>> + */
>>>>> +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_FENCE_VALID:
>>>>> + * @fence is valid, needs bind completion notification.
>>>>> + *
>>>>> + * I915_GEM_VM_BIND_READONLY:
>>>>> + * Mapping is read-only.
>>>>> + *
>>>>> + * I915_GEM_VM_BIND_CAPTURE:
>>>>> + * Capture this mapping in the dump upon GPU error.
>>>>> + *
>>>>> + * I915_GEM_VM_BIND_TLB_FLUSH:
>>>>> + * Flush the TLB for the specified range after bind completion.
>>>>> + */
>>>>> + __u64 flags;
>>>>> +#define I915_GEM_VM_BIND_FENCE_VALID (1 << 0)
>>>>> +#define I915_GEM_VM_BIND_READONLY (1 << 1)
>>>>> +#define I915_GEM_VM_BIND_CAPTURE (1 << 2)
>>>>> +#define I915_GEM_VM_BIND_TLB_FLUSH (1 << 2)
>>>>
>>>> What is the use case for allowing any random user to play with
>>>> (global) TLB flushing?
>>>>
>>>
>>> I heard it from Daniel on intel-gfx, apparently it is a Mesa
>>> requirement.
>>
>> Okay I think that one needs clarifying.
>>
>
> After chatting with Jason, I think we can remove it for now and
> we can revisit it later if Mesa thinks it is required.
IRC or some other thread?
>>>>> +
>>>>> + /** @fence: Timeline fence for bind completion signaling */
>>>>> + struct drm_i915_gem_vm_bind_fence fence;
>>>>
>>>> As agreed the other day - please document in the main kerneldoc
>>>> section that all (un)binds are executed asynchronously and out of
>>>> order.
>>>>
>>>
>>> I have added it in the latest revision of .rst file.
>>
>> Right, but I'd say to mention it in the uapi docs.
>>
>
> Ok
>
>>>>> +
>>>>> + /** @extensions: 0-terminated chain of extensions */
>>>>> + __u64 extensions;
>>>>> +};
>>>>> +
>>>>> +/**
>>>>> + * struct drm_i915_gem_vm_unbind - VA to object mapping to unbind.
>>>>> + *
>>>>> + * This structure is passed to VM_UNBIND ioctl and specifies the
>>>>> GPU virtual
>>>>> + * address (VA) range that should be unbound from the device page
>>>>> table of the
>>>>> + * specified address space (VM). The specified VA range must match
>>>>> one of the
>>>>> + * mappings created with the VM_BIND ioctl. TLB is flushed upon
>>>>> unbind
>>>>> + * completion. The unbind operation will force unbind the specified
>>>>
>>>> Do we want to provide TLB flushing guarantees here and why? (As
>>>> opposed to leaving them for implementation details.) If there is no
>>>> implied order in either binds/unbinds, or between the two
>>>> intermixed, then what is the point of guaranteeing a TLB flush on
>>>> unbind completion?
>>>>
>>>
>>> I think we ensure that tlb is flushed before signaling the out fence
>>> of vm_unbind call, then user ensure corretness by staging submissions
>>> or vm_bind calls after vm_unbind out fence signaling.
>>
>> I don't see why is this required. Driver does not need to flush
>> immediately on unbind for correctness/security and neither for the
>> uapi contract. If there is no subsequent usage/bind then the flush is
>> pointless. And if the user re-binds to same VA range, against an
>> active VM, then perhaps the expectations need to be defined. Is this
>> supported or user error or what.
>>
>
> After a vm_unbind, UMD can re-bind to same VA range against an active VM.
> Though I am not sue with Mesa usecase if that new mapping is required for
> running GPU job or it will be for the next submission. But ensuring the
> tlb flush upon unbind, KMD can ensure correctness.
Isn't that their problem? If they re-bind for submitting _new_ work then
they get the flush as part of batch buffer pre-amble.
> Note that on platforms with selective TLB invalidation, it is not
> as expensive as flushing the whole TLB. On platforms without selective
> tlb invalidation, we can put some optimization later as mentioned
> in the .rst file.
>
> Also note that UMDs can vm_unbind a mapping while VM is active.
> By flushing the tlb, we ensure there is no inadvertent access to
> mapping that no longer exists. I can add this to documentation.
This one would surely be their problem. Kernel only needs to flush when
it decides to re-use the backing store.
To be clear, overall I have reservations about offering strong
guarantees about the TLB flushing behaviour at the level of these two
ioctls. If we don't need to offer them it would be good to not do it,
otherwise we limit ourselves on the implementation side and more
importantly add a global performance hit where majority of userspace do
not need this guarantee to start with.
I only don't fully remember how is that compute use case supposed to
work where new work keeps getting submitted against a running batch. Am
I missing something there?
>>>> range from
>>>>> + * device page table without waiting for any GPU job to complete.
>>>>> It is UMDs
>>>>> + * responsibility to ensure the mapping is no longer in use before
>>>>> calling
>>>>> + * VM_UNBIND.
>>>>> + *
>>>>> + * The @start and @length musy specify a unique mapping bound with
>>>>> VM_BIND
>>>>> + * ioctl.
>>>>> + */
>>>>> +struct drm_i915_gem_vm_unbind {
>>>>> + /** @vm_id: VM (address space) id to bind */
>>>>> + __u32 vm_id;
>>>>> +
>>>>> + /** @rsvd: Reserved, MBZ */
>>>>> + __u32 rsvd;
>>>>> +
>>>>> + /** @start: Virtual Address start to unbind */
>>>>> + __u64 start;
>>>>> +
>>>>> + /** @length: Length of mapping to unbind */
>>>>> + __u64 length;
>>>>> +
>>>>> + /**
>>>>> + * @flags: Supported flags are:
>>>>> + *
>>>>> + * I915_GEM_VM_UNBIND_FENCE_VALID:
>>>>> + * @fence is valid, needs unbind completion notification.
>>>>> + */
>>>>> + __u64 flags;
>>>>> +#define I915_GEM_VM_UNBIND_FENCE_VALID (1 << 0)
>>>>> +
>>>>> + /** @fence: Timeline fence for unbind completion signaling */
>>>>> + struct drm_i915_gem_vm_bind_fence fence;
>>>>
>>>> I am not sure the simplified ioctl story is super coherent. If
>>>> everything is now fully async and out of order, but the input fence
>>>> has been dropped, then how is userspace supposed to handle the
>>>> address space? It will have to wait (in userspace) for unbinds to
>>>> complete before submitting subsequent binds which use the same VA
>>>> range.
>>>>
>>>
>>> Yah and Mesa appararently will be having the support to handle it.
>>>
>>>> Maybe that's passable, but then the fact execbuf3 has no input fence
>>>> suggests a userspace wait between it and binds. And I am pretty sure
>>>> historically those were always quite bad for performance.
>>>>
>>>
>>> execbuf3 has the input fence through timline fence array support.
>>
>> I think I confused the field in execbuf3 for for the output fence.. So
>> that part is fine, async binds chained with input fence to execbuf3.
>> Fire and forget for userspace.
>>
>> Although I then don't understand why execbuf3 wouldn't support an
>> output fence? What mechanism is userspace supposed to use for that?
>> Export a fence from batch buffer BO? That would be an extra ioctl so
>> if we can avoid it why not?
>>
>
> execbuf3 supports out fence as well through timeline fence array.
Ah okay, I am uninformed in this topic, sorry.
Regards,
Tvrtko
More information about the Intel-gfx
mailing list