Re: [PATCH v5] drm: Add UAPI for the Asahi driver

[Janne Grunau <j@jannau.net>]

On Wed, Mar 26, 2025 at 02:16:38PM -0400, Alyssa Rosenzweig wrote:
> This adds the UAPI for the Asahi driver targeting the GPU in the Apple
> M1 and M2 series systems on chip. The UAPI design is based on other
> modern Vulkan-capable drivers, including Xe and Panthor. Memory
> management is based on explicit VM management. Synchronization is
> exclusively explicit sync.
> 
> This UAPI is validated against our open source Mesa stack, which is
> fully conformant to the OpenGL 4.6, OpenGL ES 3.2, OpenCL 3.0, and
> Vulkan 1.4 standards. The Vulkan driver supports sparse, exercising the
> VM_BIND mechanism.
> 
> This patch adds the standalone UAPI header. It is implemented by an open
> source DRM driver written in Rust. We fully intend to upstream this
> driver when possible. However, as a production graphics driver, it
> depends on a significant number of Rust abstractions that will take a
> long time to upstream. In the mean time, our userspace is upstream in
> Mesa but is not allowed to probe with upstream Mesa as the UAPI is not
> yet reviewed and merged in the upstream kernel. Although we ship a
> patched Mesa in Fedora Asahi Remix, any containers shipping upstream
> Mesa builds are broken for our users, including upstream Flatpak and
> Waydroid runtimes. Additionally, it forces us to maintain forks of Mesa
> and virglrenderer, which complicates bisects.
> 
> The intention in sending out this patch is for this UAPI to be
> thoroughly reviewed. Once we as the DRM community are satisfied with the
> UAPI, this header lands signifying that the UAPI is stable and must only
> be evolved in backwards-compatible ways; it will be the UAPI implemented
> in the DRM driver that eventually lands upstream. That promise lets us
> enable upstream Mesa, solving all these issues while the upstream Rust
> abstractions are developed.
> 
> https://github.com/alyssarosenzweig/linux/commits/agx-uapi-v5 contains
> the DRM driver implementing this proposed UAPI.
> 
> https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/33984 contains
> the Mesa patches to implement this proposed UAPI.
> 
> That Linux and Mesa branch together give a complete graphics/compute
> stack on top of this UAPI.
> 
> Co-developed-by: Asahi Lina <lina@asahilina.net>
> Signed-off-by: Asahi Lina <lina@asahilina.net>
> Acked-by: Simona Vetter <simona.vetter@ffwll.ch>
> Signed-off-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
> ---
> Changes in v5:
> - Rename GEM_BIND to VM_BIND and make it take an array of bind ops. This
>   significantly decreases the # of kernel<-->user roundtrips with Vulkan
>   sparse binding. The uAPI here is lifted directly from Xe.
> - Merge in_syncs and out_syncs arrays, but leave
>   in_sync_count/out_sync_count alone, requiring waits to precede
>   signals. This simplifies both kernel & userspace, compared to either
>   fully merged or fully separate arrays, so it seems like a Good idea.
> - Drop queue caps, make all caps render + compute. Even GLES2 uses
>   compute to accelerate blits, and even compute workloads use render for
>   a few fallback blits. This lets us drop a bunch of crud in both kernel
>   & userspace and should slightly improve submit overhead.
> - Reorder ioctl IDs to group a little more logically (bikeshed...).
> - Improve some comments.
> - Link to v4: https://lore.kernel.org/r/20250323-agx-uapi-v4-1-12ed2db96737@rosenzweig.io

...

> diff --git a/include/uapi/drm/asahi_drm.h b/include/uapi/drm/asahi_drm.h
> new file mode 100644
> index 0000000000000000000000000000000000000000..a9465cb89ebde6f6768fbd5ba0fa4d753e2a7e32
> --- /dev/null
> +++ b/include/uapi/drm/asahi_drm.h
> @@ -0,0 +1,1211 @@

...

> +/**
> + * struct drm_asahi_params_global - Global parameters.
> + *
> + * This struct may be queried by drm_asahi_get_params.
> + */
> +struct drm_asahi_params_global {
> +	/** @features: Feature bits from drm_asahi_feature */
> +	__u64 features;
> +
> +	/** @gpu_generation: GPU generation, e.g. 13 for G13G */
> +	__u32 gpu_generation;
> +
> +	/** @gpu_variant: GPU variant as a character, e.g. 'G' for G13G */
> +	__u32 gpu_variant;

nit: the example can avoid the duplication of 'G' with "e.g. 'C' for
G13C"

...

> +/**
> + * struct drm_asahi_get_params - Arguments passed to DRM_IOCTL_ASAHI_GET_PARAMS
> + */
> +struct drm_asahi_get_params {
> +	/** @param_group: Parameter group to fetch (MBZ) */
> +	__u32 param_group;
> +
> +	/** @pad: MBZ */
> +	__u32 pad;
> +
> +	/** @pointer: User pointer to write parameter struct */
> +	__u64 pointer;
> +
> +	/** @size: Size of user buffer, max size supported on return */
> +	__u64 size;

The comment is misleading in the case of newer / extended kernel which
supports a larger size than supplied. You could change it to "size
written on return" or clarify that the value on return will not exceed
the input value.

> +};
> +
> +/**
> + * struct drm_asahi_vm_create - Arguments passed to DRM_IOCTL_ASAHI_VM_CREATE
> + */
> +struct drm_asahi_vm_create {
> +	/**
> +	 * @kernel_start: Start of the kernel-reserved address range. See
> +	 * drm_asahi_params_global::vm_kernel_min_size.
> +	 *
> +	 * Both @kernel_start and @kernel_end must be within the range of
> +	 * valid VAs given by drm_asahi_params_global::vm_user_start and
> +	 * drm_asahi_params_global::vm_user_end. The size of the kernel range

This reads a little strange. Would it make sense to rename drm_asahi_params_global's
vm_user_start and vm_user_end to vm_start/vm_end?

> +	 * (@kernel_end - @kernel_start) must be at least
> +	 * drm_asahi_params_global::vm_kernel_min_size.
> +	 *
> +	 * Userspace must not bind any memory on this VM into this reserved
> +	 * range, it is for kernel use only.
> +	 */
> +	__u64 kernel_start;
> +
> +	/**
> +	 * @kernel_end: End of the kernel-reserved address range. See
> +	 * @kernel_start.
> +	 */
> +	__u64 kernel_end;

...

> +/**
> + * struct drm_asahi_vm_bind - Arguments passed to
> + * DRM_IOCTL_ASAHI_VM_BIND
> + */
> +struct drm_asahi_vm_bind {
> +	/** @vm_id: The ID of the VM to bind to */
> +	__u32 vm_id;
> +
> +	/** @num_binds: number of binds in this IOCTL. Must be non-zero. */
> +	__u32 num_binds;
> +
> +	/**
> +	 * @stride: If num_binds > 1, stride in bytes between consecutive binds.
> +	 * This allows extensibility of drm_asahi_gem_bind_op.
> +	 *
> +	 * If num_binds == 1, MBZ. Extensibility in that case is handled at the
> +	 * ioctl level instead.
> +	 */
> +	__u32 stride;
> +
> +	/** @pad: MBZ */
> +	__u32 pad;
> +
> +	/**
> +	 * @bind: Union holding the bind request.
> +	 *
> +	 * This union is named to make the Rust bindings nicer to work with.
> +	 */

This comment could use a short justification why this union does not
defeat extensibility after the initial statement that "structures should
not contain unions"

> +	union {
> +		/** @bind.b: If num_binds == 1, the bind */
> +		struct drm_asahi_gem_bind_op b;
> +
> +		/**
> +		 * @bind.userptr: If num_binds > 1, user pointer to an array of
> +		 * @num_binds structures of type @drm_asahi_gem_bind_op and size
> +		 * @stride bytes.
> +		 */
> +		__u64 userptr;
> +	} bind;
> +};

...

> +/**
> + * struct drm_asahi_submit - Arguments passed to DRM_IOCTL_ASAHI_SUBMIT
> + */
> +struct drm_asahi_submit {
> +	/**
> +	 * @syncs: An optional array of drm_asahi_sync. First @in_sync_count
> +	 * in-syncs then @out_sync_count out-syncs.
> +	 */
> +     __u64 syncs;

Would it make sense to explictly state that this is a pointer?

Reviewed-by: Janne Grunau <j@jannau.net>

ciao
Janne