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

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]> wro=
te...
>> >>>> On Thu Jul 23, 2026 at 8:30 AM CEST, Alistair Popple wrote:
>> > ...
>> >>>> For instance, what's valid for userspace to extract from this? Give=
n chipid is a
>> >>>> composed value, is userspace e.g. allowed to make assumptions on ex=
tracting the
>> >>>> architecture? Or is this something we want to expose separately and=
 tell
>> >>>> userspace that the chipid is an opaque value identifying a specific=
 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 feedback o=
n as well.
>> >>> Reading the arch would then be a separate parameter.
>> >>=20
>> >> I'd prefer exposing it as an opaque value and provide other informati=
on
>> >> separately; the kernel already does the work anyway, so why duplicate=
 it in
>> >> userspace. It would also remain much cleaner if encoding details ever=
 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 mean =
using
>> > something that just counts up from, say, 1, instead of using real boot=
42
>> > values below, right?
>
> No. We'd just document these as a unique magic value that identifies the =
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 an=
ything
>> > particularly good coming from providing unnecessary details to user sp=
ace
>> > here. :)
>>=20
>> I'd probably not be too worried about keeping it as is, but just documen=
t 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 ju=
st
> makes life hard when a new chip is added thats only difference from the n=
ova-drm
> perspective is a new chipid. You don't want to have to worry about backpo=
rting
> patches to distros and kernels just to add the identifier for say TU117 e=
ven
> though it's functionally the same as TU116 from a Nova perspective.
>
> Obviously chipid would still get added to the header and eventually backp=
orted,
> but having been there and done that it's not a good user story to require=
 it.
>
>> If we expose the encoded information separately I don't see a huge incen=
tive 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 having =
to deal
>> with mapping values back and forth. I kind of like that nova-core uses t=
he
>> values from the uAPI header to define the Chipset struct, passes it to n=
ova-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 wa=
nt to go
>> with the key-value pair GETPARAM approach, which can become a bit tediou=
s.
>>=20
>> I think for the basic GPU information we could have a struct drm_nova_gp=
u_info,
>> which contains all the relevant information obtained at probe time, such=
 as
>> chipid, architecture, VRAM size, etc.
>>=20
>> If everything is available in a single struct there should be no reason =
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 stage =
approach
>> to obtain the information from userspace; probe size first, then allocat=
e with
>> the returned size:
>>=20
>> We can have a DRM_NOVA_INFO ioctl, which takes a struct containing the i=
nfo
>> identifier (e.g. DRM_NOVA_GPU_INFO), a size and a pointer to the actual =
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 fi=
lls 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 th=
e kernel
>> and call the same ioctl with the info pointer set to the allocated memor=
y.
>>=20
>> This way existing info structs remain extensible (as long as the layout =
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 memor=
y
> allocation dance doesn't seem any less tedious.

Well, it is two ioctls for a structured set of properties, whereas GETPARAM=
 is N
ioctls with no structure and slightly less type information.

> Also my experience is these structs tend to just grow over time and becom=
e
> unwieldy as new bits of info get added

Note that there's no need to grow existing info groups endlessly; if someth=
ing
truly becomes a separate concern we can just add a new group, e.g.
DRM_NOVA_MEMORY_INFO.

> and old ones deprecated forcing userspace to allocate memory for things i=
t
> might no longer care about.

This is equally true of GETPARAM; we accumulate keys that nobody uses anymo=
re.

Allocating for one or two deprecated fields for a single query doesn't seem=
 like
a big deal; plus we can always create a new info group and leave the old on=
e
alone, just like a deprecated GETPARAM key.

> Ultimately though I'm thinking (hoping?) there will not be a huge number =
of
> these - a lot of this info should probably get exposed via sysfs anyway.

I don't think sysfs is a good fit; it seems tedious for a UMD to do a bunch=
 of
sysfs file reads, each of them requiring at least three syscalls. Plus the
effort to parse the value from the string sysfs provides, which is more err=
or
prone than reading a typed struct.