Message-ID: <Z-BkP2Kt0NYKJwfC@blossom>
Date: Sun, 23 Mar 2025 15:42:55 -0400
From: Alyssa Rosenzweig <alyssa@...enzweig.io>
To: Faith Ekstrand <faith.ekstrand@...labora.com>
Cc: David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
	Maxime Ripard <mripard@...nel.org>,
	Thomas Zimmermann <tzimmermann@...e.de>,
	Miguel Ojeda <ojeda@...nel.org>,
	Alex Gaynor <alex.gaynor@...il.com>,
	Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
	"Björn Roy Baron" <bjorn3_gh@...tonmail.com>,
	Benno Lossin <benno.lossin@...ton.me>,
	Andreas Hindborg <a.hindborg@...nel.org>,
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
	Janne Grunau <j@...nau.net>, Sven Peter <sven@...npeter.dev>,
	Jonathan Corbet <corbet@....net>,
	Sergio Lopez Pascual <slp@...rega.org>,
	Ryan Houdek <sonicadvance1@...il.com>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	dri-devel <dri-devel@...ts.freedesktop.org>,
	rust-for-linux <rust-for-linux@...r.kernel.org>,
	asahi <asahi@...ts.linux.dev>,
	linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
	linux-doc <linux-doc@...r.kernel.org>,
	Asahi Lina <lina@...hilina.net>
Subject: Re: [PATCH v3] drm: Add UAPI for the Asahi driver

> I'm good with this. There's a slim possibility that upstream may
> evolve in ways that make the current UAPI tricky to implement.
> However, given that it's based on prior art from the nouveau, Intel,
> and panfrost teams and that you've been shipping it in production for
> a while, I think that possibility is pretty remote.

Yeah, I'm not too worried about that... the uAPI isn't being designed
around the kernel driver, so unless we're deprecating GEM or something
we should be good!

>  > +    /** @DRM_ASAHI_GEM_BIND: Bind/unbind memory to a VM. */ 
>  > +    DRM_ASAHI_GEM_BIND, 
> 
> I was about to complain about the GEM_BIND name but I actually prefer
> it. Given that it binds a single GEM object to a given range in a
> given VM, I think it makes sense to have it be an option on the GEM
> object. If and when you implement a bind queue, you can use VM_BIND
> for the new multi-bind ioctl and that will be an operation on the VM
> that says "bind all this stuff, here's some fences to know when."

Sounds good.

>  > +    /** @vm_user_start: VM user range start VMA */ 
>  > +    __u64 vm_user_start; 
>  > + 
>  > +    /** @vm_user_end: VM user range end VMA */ 
>  > +    __u64 vm_user_end; 
> 
> Does this have to be chosen by the kernel? Are there fixed addresses chosen by the firmware which need to be respected? Or is this just the range of valid GPU addresses? I also see kernel start/end being passed in at VM creation. I'm confused. At the very least, this needs a much better comment than the one above.

Added a bunch of comments in v4.

>  > +    /** 
>  > +     * @vm_kernel_min_size: Minimum kernel VMA window size within user 
>  > +     * range 
>  > +     */ 
>  > +    __u64 vm_kernel_min_size; 

>  > +    /** 
>  > +     * @max_commands_per_submission: Maximum number of supported commands 
>  > +     * per submission 
>  > +     */ 
>  > +    __u32 max_commands_per_submission; 
> 
> Pain. But we have this in nouveau as well, so...

This mirrors firmware limits. Either we have to split in userspace or
kernelspace. And at least if we split in userspace, there are no
surprises about where oversynchronization happens.

>  > +    /** 
>  > +     * @firmware_version: GPU firmware version, as 4 integers 
>  > +     */ 
>  > +    __u32 firmware_version[4]; 
> 
> There's a part of me that's like "This should never matter. You shouldn't expose that detail to userspace!" but let's be real...

TBH, I agree. If we need it for something later we can revisit but by
design this should never matter and current Mesa doesn't do anything
with it anyway.

Dropped in v4.

>  > + 
>  > +    /** 
>  > +     * @user_timestamp_frequency_hz: Timebase frequency for user timestamps 
>  > +     */ 
>  > +    __u64 user_timestamp_frequency_hz; 
> 
> Why is this different? What are user timestamps and how are they different from GPU timestamps?

I've added comments in v4 to clarify.

This does raise the question of, maybe drm_asahi_get_time should be
returning time in nanoseconds instead? I'm tempted to make that change
in v4. That would let us get rid of one of the parameters.

In practice the firmware-written "user" timestamps are themselves in
nanoseconds but we don't want to make that uAPI. We can't scale those in
the kernel (what if we write timestamps and then immediately
vkCmdCopyQuery them? We don't want to force a CPU roundtrip/stall just
for that.)

The kernel-read timestamps on current systems are from a 24MHz SoC-wide
reference clock, which both the GPU and CPU share. We can scale these in
the kernel to nanos, which is what userspace does itself currently.
Kind of bikeshed territory but meh?

>  > +    /** 
>  > +     * @DRM_ASAHI_FEATURE_SOFT_FAULTS: GPU has "soft fault" enabled. Shader 
>  > +     * loads of unmapped memory will return zero. Shader stores to unmapped 
>  > +     * memory will be silently discarded. Note that only shader load/store 
>  > +     * is affected. Other hardware units are not affected, notably including 
>  > +     * texture sampling. 
>  > +     */ 
>  > +    DRM_ASAHI_FEATURE_SOFT_FAULTS = (1UL) << 0, 
>  > +}; 
> 
> This makes me a little nervous. Why isn't this a bit you can set on VM creation? If it's something that's chosen by the kernel and which userspace can query but not change, that seems problematic from a backwards compatiblity PoV.

As far as we know, the fault control register must be set during GPU
initialization and cannot be changed once we're booted. So, we can
override it only via a kernel command line parameter.

macOS enables soft fault in all production builds. We do too these days,
but we have a kernel cmdline flag to disable soft fault / enable hard
faults to aid debugging.

Mesa queries this parameter to determine whether it can speculate
aggressively loads out of control (CAN_SPECULATE) -- it can with
production Asahi systems but not when I'm running CTS on my dev box.

Maybe more problematically, Mesa also currently gates sparseBuffer on
this flag, although I'm probably going to rewrite the sparse buffer
implementation in the coming months to avoid that dependence for various
reasons.

Should I avoid releasing an upstream Mesa with sparseBuffer advertised
until the dependence is dropped, to avoid a theoretical uAPI break if we
wanted to change this policy later? (where the regression is "old Mesas
in containers lose sparseBuffer/FL12").

Powered by blists - more mailing lists