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

["Danilo Krummrich" <dakr@kernel.org>]

On Wed Jul 29, 2026 at 10:24 AM CEST, Alistair Popple wrote:
> On 2026-07-29 at 08:57 +1000, Danilo Krummrich <dakr@kernel.org> 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 <dakr@kernel.org> 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? Given 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 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 on as well.
>> >>> Reading the arch would then be a separate parameter.
>> >> 
>> >> I'd prefer exposing it as an opaque value and provide other information
>> >> 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 boot42
>> > 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 anything
>> > particularly good coming from providing unnecessary details to user space
>> > here. :)
>> 
>> I'd probably not be too worried about keeping it as is, but just document 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 the nova-drm
> perspective is a new chipid. You don't want to have to worry about backporting
> patches to distros and kernels just to add the identifier for say TU117 even
> though it's functionally the same as TU116 from a Nova perspective.
>
> Obviously chipid would still get added to the header and eventually backported,
> 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 incentive for
>> userspace to redo the work and mess with it anyway.
>> 
>> 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 the
>> values from the uAPI header to define the Chipset struct, passes it to 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 tedious.
>> 
>> 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, such as
>> chipid, architecture, VRAM size, etc.
>> 
>> 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.
>> 
>> 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 allocate with
>> the returned size:
>> 
>> We can have a DRM_NOVA_INFO ioctl, which takes a struct containing the info
>> identifier (e.g. DRM_NOVA_GPU_INFO), a size and a pointer to the actual info
>> structure.
>> 
>> 	struct drm_nova_info {
>> 		__u32 id;
>> 		__u32 size;
>> 		__u64 info;
>> 	};
>> 
>> 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).
>> 
>> 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 memory.
>> 
>> 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 memory
> 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 become
> unwieldy as new bits of info get added

Note that there's no need to grow existing info groups endlessly; if something
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 it
> might no longer care about.

This is equally true of GETPARAM; we accumulate keys that nobody uses anymore.

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 one
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 error
prone than reading a typed struct.