Re: [PATCH v3 3/7] drm: nova: Add chipid enum to nova-drm UAPI

On Thu Jul 30, 2026 at 1:46 PM CEST, Alistair Popple wrote:
> On 2026-07-29 at 21:22 +1000, Danilo Krummrich <[email protected]> wrote...
>> On Wed Jul 29, 2026 at 10:24 AM CEST, Alistair Popple wrote:
>> > On 2026-07-29 at 08:57 +1000, Danilo Krummrich <[email protected]> wrote=
...
>> >> On Tue Jul 28, 2026 at 11:49 PM CEST, John Hubbard wrote:
>> >> > On 7/28/26 2:29 PM, Danilo Krummrich wrote:
>> >> >> On Tue Jul 28, 2026 at 10:00 AM CEST, Alistair Popple wrote:
>> >> >>> On 2026-07-26 at 01:23 +1000, Danilo Krummrich <[email protected]> =
wrote...
>> >> >>>> On Thu Jul 23, 2026 at 8:30 AM CEST, Alistair Popple wrote:
>> >> > ...
>> >> >>>> For instance, what's valid for userspace to extract from this? G=
iven chipid is a
>> >> >>>> composed value, is userspace e.g. allowed to make assumptions on=
 extracting the
>> >> >>>> architecture? Or is this something we want to expose separately =
and tell
>> >> >>>> userspace that the chipid is an opaque value identifying a speci=
fic chip only?
>> >> >>>
>> >> >>> I _think_ it makes sense for this to just be an opaque value, but=
 it's a good
>> >> >>> question that I should probably try and get some internal feedbac=
k on as well.
>> >> >>> Reading the arch would then be a separate parameter.
>> >> >>=20
>> >> >> I'd prefer exposing it as an opaque value and provide other inform=
ation
>> >> >> separately; the kernel already does the work anyway, so why duplic=
ate it in
>> >> >> userspace. It would also remain much cleaner if encoding details e=
ver change.
>> >
>> > Yep, I agree. Just want to make sure we get everyone in agreement.
>> >
>> >> >
>> >> > Just to orient myself, I'm assuming that by "opaque values", you me=
an using
>> >> > something that just counts up from, say, 1, instead of using real b=
oot42
>> >> > values below, right?
>> >
>> > No. We'd just document these as a unique magic value that identifies t=
he GPU
>> > chip rather than something that could be decoded into anything else.
>> >
>> >> > That seems like a good approach to me, too. Because I can't imagine=
 anything
>> >> > particularly good coming from providing unnecessary details to user=
 space
>> >> > here. :)
>> >>=20
>> >> I'd probably not be too worried about keeping it as is, but just docu=
ment it as
>> >> being an opaque value that shouldn't be messed with.
>> >
>> > Yes, agree with this. I don't think we want to mess with the value. It=
 just
>> > makes life hard when a new chip is added thats only difference from th=
e nova-drm
>> > perspective is a new chipid. You don't want to have to worry about bac=
kporting
>> > patches to distros and kernels just to add the identifier for say TU11=
7 even
>> > though it's functionally the same as TU116 from a Nova perspective.
>> >
>> > Obviously chipid would still get added to the header and eventually ba=
ckported,
>> > but having been there and done that it's not a good user story to requ=
ire it.
>> >
>> >> If we expose the encoded information separately I don't see a huge in=
centive for
>> >> userspace to redo the work and mess with it anyway.
>> >>=20
>> >> The advantage of keeping the values as they are is that we avoid havi=
ng to deal
>> >> with mapping values back and forth. I kind of like that nova-core use=
s the
>> >> values from the uAPI header to define the Chipset struct, passes it t=
o nova-drm
>> >> and nova-drm does use it to fill in the userspace exposed data.
>> >
>> > So do I.
>> >
>> >> Something else worth to consider in this context is whether we really=
 want to go
>> >> with the key-value pair GETPARAM approach, which can become a bit ted=
ious.
>> >>=20
>> >> I think for the basic GPU information we could have a struct drm_nova=
_gpu_info,
>> >> which contains all the relevant information obtained at probe time, s=
uch as
>> >> chipid, architecture, VRAM size, etc.
>> >>=20
>> >> If everything is available in a single struct there should be no reas=
on for
>> >> userspace to mess with the chipid value. However, I'd also not object=
 to make it
>> >> a "real" opaque value.
>> >>=20
>> >> In order to deal with additions to the struct we could take a two sta=
ge approach
>> >> to obtain the information from userspace; probe size first, then allo=
cate with
>> >> the returned size:
>> >>=20
>> >> We can have a DRM_NOVA_INFO ioctl, which takes a struct containing th=
e info
>> >> identifier (e.g. DRM_NOVA_GPU_INFO), a size and a pointer to the actu=
al info
>> >> structure.
>> >>=20
>> >> 	struct drm_nova_info {
>> >> 		__u32 id;
>> >> 		__u32 size;
>> >> 		__u64 info;
>> >> 	};
>> >>=20
>> >> Userspace can call the DRM_NOVA_INFO with the identifier (e.g.
>> >> DRM_NOVA_GPU_INFO), size and info pointer set to zero, and the kernel=
 fills in
>> >> the size of the info struct (e.g. struct drm_nova_gpu_info).
>> >>=20
>> >> Subsequently, userspace can allocate memory with the size reported by=
 the kernel
>> >> and call the same ioctl with the info pointer set to the allocated me=
mory.
>> >>=20
>> >> This way existing info structs remain extensible (as long as the layo=
ut isn't
>> >> changed of course) and new ones can be added at any time.
>> >
>> > I did consider an approach like this but having to do the two ioctl me=
mory
>> > allocation dance doesn't seem any less tedious.
>>=20
>> Well, it is two ioctls for a structured set of properties, whereas GETPA=
RAM is N
>> ioctls with no structure and slightly less type information.
>
> I guess I could be convinced of the struct approach, but there's no need =
to
> do the whole two ioctl dance. Fields can only ever get appended so just d=
o it
> based on size - an old user-space passing a too small struct doesn't get =
the new
> fields and a new user-space passing a too big struct can get an error cod=
e or we
> allow it and return the actual size written.

I thought about this too, and I agree we should support this.

The advantage is that the userspace side can remain much simpler (especiall=
y in
memory safe languages), since it can just use the size of its struct versio=
n.

IOW, the "buffer is smaller than struct" case goes into the kernel, where w=
e
could leverage a UserSliceWriter::write_truncated() helper [1].

When userspace passes a bigger buffer than needed by the kernel we should n=
ot
return an error code; a newer userspace can work perfectly fine with an old=
er
kernel.

We could still keep the probe semantics for size =3D=3D 0 though. It should=
n't cost
us anything, as it should be trivial. And it could be useful for diagnostic
purposes / tooling to dump the full kernel struct regardless of version (e.=
g.
for bug reports).

[1] UserSliceWriter::write_truncated()

diff --git a/rust/kernel/uaccess.rs b/rust/kernel/uaccess.rs
index 5f6c4d7a1a51..c63b91942631 100644
--- a/rust/kernel/uaccess.rs
+++ b/rust/kernel/uaccess.rs
@@ -624,6 +624,20 @@ pub fn write<T: AsBytes>(&mut self, value: &T) -> Resu=
lt {
         self.length -=3D len;
         Ok(())
     }
+
+    /// Writes as much of the provided value as fits in the remaining buff=
er.
+    ///
+    /// Copies `min(size_of::<T>(), self.len())` bytes to userspace. Retur=
ns the number of bytes
+    /// actually written. This is useful for versioned structs where an ol=
der userspace may provide
+    /// a smaller buffer than the current kernel struct.
+    ///
+    /// Fails with [`EFAULT`] if the write happens on a bad address. This =
call may modify the
+    /// associated userspace slice even if it returns an error.
+    pub fn write_truncated<T: AsBytes>(&mut self, value: &T) -> Result<usi=
ze> {
+        let len =3D self.length.min(size_of::<T>());
+        self.write_slice(&value.as_bytes()[..len])?;
+        Ok(len)
+    }
 }

 /// Reads a nul-terminated string into `dst` and returns the length.

>> > Also my experience is these structs tend to just grow over time and be=
come
>> > unwieldy as new bits of info get added
>>=20
>> Note that there's no need to grow existing info groups endlessly; if som=
ething
>> truly becomes a separate concern we can just add a new group, e.g.
>> DRM_NOVA_MEMORY_INFO.
>
> Fair enough, although you likely end up with duplicated information as it=
ems
> from one group get copied into another which would be a bit ugly, having =
two
> ways to read the same thing. My main motivation here was really just that
> GETPARAM is very easy to extend as we discover we need other parameters g=
iven
> that we don't know what they all are yet.
>
> I was also thinking there wasn't much of a downside, but you make a good =
point
> on slightly less type information.
>
>> > and old ones deprecated forcing userspace to allocate memory for thing=
s it
>> > might no longer care about.
>>=20
>> This is equally true of GETPARAM; we accumulate keys that nobody uses an=
ymore.
>>=20
>> Allocating for one or two deprecated fields for a single query doesn't s=
eem like
>> a big deal; plus we can always create a new info group and leave the old=
 one
>> alone, just like a deprecated GETPARAM key.
>>=20
>> > Ultimately though I'm thinking (hoping?) there will not be a huge numb=
er of
>> > these - a lot of this info should probably get exposed via sysfs anywa=
y.
>>=20
>> I don't think sysfs is a good fit; it seems tedious for a UMD to do a bu=
nch of
>> sysfs file reads, each of them requiring at least three syscalls. Plus t=
he
>> effort to parse the value from the string sysfs provides, which is more =
error
>> prone than reading a typed struct.
>
> Yeah, that was an idea I'd heard floating around and I thought it may hav=
e come
> from upstream but I'm not entirely convinced of it for the reasons you st=
ate.
>
>  - Alistair