| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <da776218-e930-5cbf-b2ab-8e6c39b900cd@redhat.com>
Date: Tue, 25 Jul 2023 04:03:55 +0200
From: Danilo Krummrich <dakr@...hat.com>
To: Faith Ekstrand <faith@...strand.net>, airlied@...il.com
Cc: daniel@...ll.ch, tzimmermann@...e.de, mripard@...nel.org,
corbet@....net, christian.koenig@....com, bskeggs@...hat.com,
Liam.Howlett@...cle.com, matthew.brost@...el.com,
boris.brezillon@...labora.com, alexdeucher@...il.com,
ogabbay@...nel.org, bagasdotme@...il.com, willy@...radead.org,
jason@...kstrand.net, donald.robson@...tec.com,
dri-devel@...ts.freedesktop.org, nouveau@...ts.freedesktop.org,
linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
Dave Airlie <airlied@...hat.com>
Subject: Re: [PATCH drm-misc-next v8 03/12] drm/nouveau: new VM_BIND uapi
interfaces
On 7/22/23 00:58, Faith Ekstrand wrote:
>
> On Wed, Jul 19, 2023 at 7:15 PM Danilo Krummrich <dakr@...hat.com
> <mailto:dakr@...hat.com>> wrote:
>
> This commit provides the interfaces for the new UAPI motivated by the
> Vulkan API. It allows user mode drivers (UMDs) to:
>
> 1) Initialize a GPU virtual address (VA) space via the new
> DRM_IOCTL_NOUVEAU_VM_INIT ioctl. UMDs can provide a kernel reserved
> VA area.
>
> 2) Bind and unbind GPU VA space mappings via the new
> DRM_IOCTL_NOUVEAU_VM_BIND ioctl.
>
> 3) Execute push buffers with the new DRM_IOCTL_NOUVEAU_EXEC ioctl.
>
> Both, DRM_IOCTL_NOUVEAU_VM_BIND and DRM_IOCTL_NOUVEAU_EXEC support
> asynchronous processing with DRM syncobjs as synchronization mechanism.
>
> The default DRM_IOCTL_NOUVEAU_VM_BIND is synchronous processing,
> DRM_IOCTL_NOUVEAU_EXEC supports asynchronous processing only.
>
> Co-authored-by: Dave Airlie <airlied@...hat.com
> <mailto:airlied@...hat.com>>
> Signed-off-by: Danilo Krummrich <dakr@...hat.com
> <mailto:dakr@...hat.com>>
> ---
> Documentation/gpu/driver-uapi.rst | 8 ++
> include/uapi/drm/nouveau_drm.h | 209 ++++++++++++++++++++++++++++++
> 2 files changed, 217 insertions(+)
>
> diff --git a/Documentation/gpu/driver-uapi.rst
> b/Documentation/gpu/driver-uapi.rst
> index 4411e6919a3d..9c7ca6e33a68 100644
> --- a/Documentation/gpu/driver-uapi.rst
> +++ b/Documentation/gpu/driver-uapi.rst
> @@ -6,3 +6,11 @@ drm/i915 uAPI
> =============
>
> .. kernel-doc:: include/uapi/drm/i915_drm.h
> +
> +drm/nouveau uAPI
> +================
> +
> +VM_BIND / EXEC uAPI
> +-------------------
> +
> +.. kernel-doc:: include/uapi/drm/nouveau_drm.h
> diff --git a/include/uapi/drm/nouveau_drm.h
> b/include/uapi/drm/nouveau_drm.h
> index 853a327433d3..4d3a70529637 100644
> --- a/include/uapi/drm/nouveau_drm.h
> +++ b/include/uapi/drm/nouveau_drm.h
> @@ -126,6 +126,209 @@ struct drm_nouveau_gem_cpu_fini {
> __u32 handle;
> };
>
> +/**
> + * struct drm_nouveau_sync - sync object
> + *
> + * This structure serves as synchronization mechanism for (potentially)
> + * asynchronous operations such as EXEC or VM_BIND.
> + */
> +struct drm_nouveau_sync {
> + /**
> + * @flags: the flags for a sync object
> + *
> + * The first 8 bits are used to determine the type of the
> sync object.
> + */
> + __u32 flags;
> +#define DRM_NOUVEAU_SYNC_SYNCOBJ 0x0
> +#define DRM_NOUVEAU_SYNC_TIMELINE_SYNCOBJ 0x1
> +#define DRM_NOUVEAU_SYNC_TYPE_MASK 0xf
> + /**
> + * @handle: the handle of the sync object
> + */
> + __u32 handle;
> + /**
> + * @timeline_value:
> + *
> + * The timeline point of the sync object in case the syncobj
> is of
> + * type DRM_NOUVEAU_SYNC_TIMELINE_SYNCOBJ.
> + */
> + __u64 timeline_value;
> +};
> +
> +/**
> + * struct drm_nouveau_vm_init - GPU VA space init structure
> + *
> + * Used to initialize the GPU's VA space for a user client, telling
> the kernel
> + * which portion of the VA space is managed by the UMD and kernel
> respectively.
>
>
> I assume this has to be called quite early. Like maybe before any BOs or
> channels are created? In any case, it'd be nice to have that documented.
Exactly, doing any of those will disable the new uAPI entirely if it
wasn't yet initialized. I will add some documentation for this.
>
> + */
> +struct drm_nouveau_vm_init {
> + /**
> + * @unmanaged_addr: start address of the kernel managed VA
> space region
> + */
> + __u64 unmanaged_addr;
> + /**
> + * @unmanaged_size: size of the kernel managed VA space
> region in bytes
> + */
> + __u64 unmanaged_size;
>
>
> Over-all, I think this is the right API. My only concern is with the
> word "unmanaged". None of the VA space is unmanaged. Some is
> userspace-managed and some is kernel-managed. I guess "unmanaged" kinda
> makes sense because this is coming from userspace and it's saying which
> bits it manages and which bits it doesn't. Still seems clunky to me.
> Maybe kernel_managed? IDK, that feels weird too. Since we're already
> using UMD in this file, we could call it kmd_managed. IDK. 🤷🏻♀️
kernel_managed / kmd_managed both sounds fine to me. I'm good with
either one.
>
> Yeah, I know this is a total bikeshed color thing and I'm not going to
> block anything based on it. 😅 Just wanted to see if we can come up with
> anything better. It's documented and that's the important thing.
>
> +};
> +
> +/**
> + * struct drm_nouveau_vm_bind_op - VM_BIND operation
> + *
> + * This structure represents a single VM_BIND operation. UMDs
> should pass
> + * an array of this structure via struct drm_nouveau_vm_bind's
> &op_ptr field.
> + */
> +struct drm_nouveau_vm_bind_op {
> + /**
> + * @op: the operation type
> + */
> + __u32 op;
> +/**
> + * @DRM_NOUVEAU_VM_BIND_OP_MAP:
> + *
> + * Map a GEM object to the GPU's VA space. Optionally, the
> + * &DRM_NOUVEAU_VM_BIND_SPARSE flag can be passed to instruct the
> kernel to
> + * create sparse mappings for the given range.
> + */
> +#define DRM_NOUVEAU_VM_BIND_OP_MAP 0x0
> +/**
> + * @DRM_NOUVEAU_VM_BIND_OP_UNMAP:
> + *
> + * Unmap an existing mapping in the GPU's VA space. If the region
> the mapping
> + * is located in is a sparse region, new sparse mappings are
> created where the
> + * unmapped (memory backed) mapping was mapped previously. To
> remove a sparse
> + * region the &DRM_NOUVEAU_VM_BIND_SPARSE must be set.
> + */
> +#define DRM_NOUVEAU_VM_BIND_OP_UNMAP 0x1
> + /**
> + * @flags: the flags for a &drm_nouveau_vm_bind_op
> + */
> + __u32 flags;
> +/**
> + * @DRM_NOUVEAU_VM_BIND_SPARSE:
> + *
> + * Indicates that an allocated VA space region should be sparse.
> + */
> +#define DRM_NOUVEAU_VM_BIND_SPARSE (1 << 8)
> + /**
> + * @handle: the handle of the DRM GEM object to map
> + */
> + __u32 handle;
> + /**
> + * @pad: 32 bit padding, should be 0
> + */
> + __u32 pad;
> + /**
> + * @addr:
> + *
> + * the address the VA space region or (memory backed)
> mapping should be mapped to
> + */
> + __u64 addr;
> + /**
> + * @bo_offset: the offset within the BO backing the mapping
> + */
> + __u64 bo_offset;
> + /**
> + * @range: the size of the requested mapping in bytes
> + */
> + __u64 range;
> +};
> +
> +/**
> + * struct drm_nouveau_vm_bind - structure for DRM_IOCTL_NOUVEAU_VM_BIND
> + */
> +struct drm_nouveau_vm_bind {
> + /**
> + * @op_count: the number of &drm_nouveau_vm_bind_op
> + */
> + __u32 op_count;
>
>
> I've chatted a bit with Dave on IRC about this but both VM_BIND and EXEC
> should support `op_count == 0` and do exactly the same thing that they
> would do if there were real ops. In the case of vm_bind, that just means
> wait on the waits and then signal the signals. In particular, it should
> NOT just return success and do nothing. Dave has a patch for this for
> EXEC but IDK if VM_BIND needs any attention. Of course, if it's not
> ASYNC, then quickly doing nothing after validating inputs is acceptable.
What will this be used for? I guess it would not be important to be
executed in order with "regular" (non-noop) jobs? Because the only thing
this would tell you is that e.g. for VM_BIND all previous binds
completed, which is what we have syncobjs for.
- Danilo
>
> + /**
> + * @flags: the flags for a &drm_nouveau_vm_bind ioctl
> + */
> + __u32 flags;
> +/**
> + * @DRM_NOUVEAU_VM_BIND_RUN_ASYNC:
> + *
> + * Indicates that the given VM_BIND operation should be executed
> asynchronously
> + * by the kernel.
> + *
> + * If this flag is not supplied the kernel executes the associated
> operations
> + * synchronously and doesn't accept any &drm_nouveau_sync objects.
> + */
> +#define DRM_NOUVEAU_VM_BIND_RUN_ASYNC 0x1
> + /**
> + * @wait_count: the number of wait &drm_nouveau_syncs
> + */
> + __u32 wait_count;
> + /**
> + * @sig_count: the number of &drm_nouveau_syncs to signal
> when finished
> + */
> + __u32 sig_count;
> + /**
> + * @wait_ptr: pointer to &drm_nouveau_syncs to wait for
> + */
> + __u64 wait_ptr;
> + /**
> + * @sig_ptr: pointer to &drm_nouveau_syncs to signal when
> finished
> + */
> + __u64 sig_ptr;
> + /**
> + * @op_ptr: pointer to the &drm_nouveau_vm_bind_ops to execute
> + */
> + __u64 op_ptr;
> +};
> +
> +/**
> + * struct drm_nouveau_exec_push - EXEC push operation
> + *
> + * This structure represents a single EXEC push operation. UMDs
> should pass an
> + * array of this structure via struct drm_nouveau_exec's &push_ptr
> field.
> + */
> +struct drm_nouveau_exec_push {
> + /**
> + * @va: the virtual address of the push buffer mapping
> + */
> + __u64 va;
> + /**
> + * @va_len: the length of the push buffer mapping
> + */
> + __u64 va_len;
> +};
> +
> +/**
> + * struct drm_nouveau_exec - structure for DRM_IOCTL_NOUVEAU_EXEC
> + */
> +struct drm_nouveau_exec {
> + /**
> + * @channel: the channel to execute the push buffer in
> + */
> + __u32 channel;
> + /**
> + * @push_count: the number of &drm_nouveau_exec_push ops
> + */
> + __u32 push_count;
>
>
> Same comment as above. We want `push_count == 0` to behave the same as
> any other EXEC just without anything new. In particular, it needs to
> wait on all the waits as well as the previous EXECs on that channel and
> then signal the sigs. I know Dave has a patch for this and it's working
> quite well in my testing.
>
> Other than that, everything looks good. I'm still re-reading all the
> NVK patches but they've been working quite well in my testing this week
> apart from a perf issue I need to dig into. I'll give a real RB once
> we're sure we all agree on the semantics of _count.
>
> ~Faith
>
> + /**
> + * @wait_count: the number of wait &drm_nouveau_syncs
> + */
> + __u32 wait_count;
> + /**
> + * @sig_count: the number of &drm_nouveau_syncs to signal
> when finished
> + */
> + __u32 sig_count;
> + /**
> + * @wait_ptr: pointer to &drm_nouveau_syncs to wait for
> + */
> + __u64 wait_ptr;
> + /**
> + * @sig_ptr: pointer to &drm_nouveau_syncs to signal when
> finished
> + */
> + __u64 sig_ptr;
> + /**
> + * @push_ptr: pointer to &drm_nouveau_exec_push ops
> + */
> + __u64 push_ptr;
> +};
> +
> #define DRM_NOUVEAU_GETPARAM 0x00 /* deprecated */
> #define DRM_NOUVEAU_SETPARAM 0x01 /* deprecated */
> #define DRM_NOUVEAU_CHANNEL_ALLOC 0x02 /* deprecated */
> @@ -136,6 +339,9 @@ struct drm_nouveau_gem_cpu_fini {
> #define DRM_NOUVEAU_NVIF 0x07
> #define DRM_NOUVEAU_SVM_INIT 0x08
> #define DRM_NOUVEAU_SVM_BIND 0x09
> +#define DRM_NOUVEAU_VM_INIT 0x10
> +#define DRM_NOUVEAU_VM_BIND 0x11
> +#define DRM_NOUVEAU_EXEC 0x12
> #define DRM_NOUVEAU_GEM_NEW 0x40
> #define DRM_NOUVEAU_GEM_PUSHBUF 0x41
> #define DRM_NOUVEAU_GEM_CPU_PREP 0x42
> @@ -197,6 +403,9 @@ struct drm_nouveau_svm_bind {
> #define DRM_IOCTL_NOUVEAU_GEM_CPU_FINI DRM_IOW
> (DRM_COMMAND_BASE + DRM_NOUVEAU_GEM_CPU_FINI, struct
> drm_nouveau_gem_cpu_fini)
> #define DRM_IOCTL_NOUVEAU_GEM_INFO
> DRM_IOWR(DRM_COMMAND_BASE + DRM_NOUVEAU_GEM_INFO, struct
> drm_nouveau_gem_info)
>
> +#define DRM_IOCTL_NOUVEAU_VM_INIT
> DRM_IOWR(DRM_COMMAND_BASE + DRM_NOUVEAU_VM_INIT, struct
> drm_nouveau_vm_init)
> +#define DRM_IOCTL_NOUVEAU_VM_BIND
> DRM_IOWR(DRM_COMMAND_BASE + DRM_NOUVEAU_VM_BIND, struct
> drm_nouveau_vm_bind)
> +#define DRM_IOCTL_NOUVEAU_EXEC
> DRM_IOWR(DRM_COMMAND_BASE + DRM_NOUVEAU_EXEC, struct drm_nouveau_exec)
> #if defined(__cplusplus)
> }
> #endif
> --
> 2.41.0
>
Powered by blists - more mailing lists