Re: [PATCH v4 4/7] drm: nova: Add a GPU info ioctl

On Fri Aug 21, 2026 at 7:10 AM CEST, Alistair Popple wrote:
> On 2026-08-18 at 06:18 +1000, Danilo Krummrich <[email protected]> wrote...
>> On Tue Aug 11, 2026 at 7:06 AM CEST, Alistair Popple wrote:
>> > diff --git a/include/uapi/drm/nova_drm.h b/include/uapi/drm/nova_drm.h
>> > index ea7665383644..2604e4d2698b 100644
>> > --- a/include/uapi/drm/nova_drm.h
>> > +++ b/include/uapi/drm/nova_drm.h
>> > @@ -118,9 +118,34 @@ struct drm_nova_gem_info {
>> >  	__u64 size;
>> >  };
>> >  
>> > +/**
>> > + * struct drm_nova_gpu_info - query DRM GPU info.
>> > + */
>> > +struct drm_nova_gpu_info {
>> > +	/**
>> > +	 * @size: The amount of space allocated by userspace for this structure.
>> > +	 * The kernel will return the amount of data it did/could actually write.
>> > +	 * User space can use this to determine how much of the struct is valid
>> > +	 * when running against an older kernel.
>> > +	 */
>> > +	__u64 size;
>> > +
>> > +	/**
>> > +	 * @chipid: GPU chip identifier. See &enum drm_nova_chipid for currently
>> > +	 * known chip identifiers.
>> > +	 */
>> > +	__u32 chipid;

I think we now also want to add a field for the architecture now that chipid is
considered opaque.

>> > +
>> > +	/**
>> > +	 * @pad: 32 bit padding, must be 0.
>> > +	 */
>> > +	__u32 pad;
>> > +};
>> 
>> I think we should add the indirection we discussed in [1], i.e. have an
>> indirection via
>> 
>> 	struct drm_nova_info {
>> 		__u32 id;
>> 		__u32 size;
>> 		__u64 info;
>> 		/* Revserved fields, just in case? */
>> 	};
>> 
>> so we can easily add new info structures, or extend an existing one with a v2
>> without having to create new ioctls for this purpose.
>
> Sorry, I should have called this difference out more explicitly.
>
> Basically I ended up doing it this way because it didn't make much sense to me
> putting an ioctl interface within an ioctl interface when DRM ioctl handling
> can already deal with matching numbers and truncating/extending the struct as
> required. It just leads to more code comparing ID's, etc and I'm not really sure
> what the advantage is. Are we concerned about running out of ioctls if we have
> to add other types of info struct?
>
> Doing this as top-level ioctl makes the strace decoders simpler and means we can
> just rely on the existing DRM ioctl handling to get everything right rather than
> duplicating that in nova-drm. Or is there some other advantage to [1] that I've
> missed that isn't solved here?

I don't think we are really concerned about running out of ioctls, but it seems
cleaner and more self-contained than having N ioctls for different info structs
and in the worst case having v2...vN info ioctls.

It also allows us to define a new info type struct whenever we think something
is a new logical info group. Making it per ioctl will always raise the question
of "do we really need a new ioctl for this, can't we just fit it in X", which
over time tends to get messy.

I think eventually we will have a bunch of different info categories. OpenRM
seems to have quite some as well (not too many categories, but with lots of
fields), Xe and amdgpu have even more categories.

> Thanks for looking.
>
>  - Alistair
>
>> 
>> [1] https://lore.kernel.org/nova-gpu/[email protected]/
>> 
>> >  #define DRM_NOVA_GETPARAM		0x00
>> >  #define DRM_NOVA_GEM_CREATE		0x01
>> >  #define DRM_NOVA_GEM_INFO		0x02
>> > +#define DRM_NOVA_GPU_INFO		0x03
>> >  
>> >  /* Note: this is an enum so that it can be resolved by Rust bindgen. */
>> >  enum {
>> > @@ -130,6 +155,8 @@ enum {
>> >  						   struct drm_nova_gem_create),
>> >  	DRM_IOCTL_NOVA_GEM_INFO		= DRM_IOWR(DRM_COMMAND_BASE + DRM_NOVA_GEM_INFO,
>> >  						   struct drm_nova_gem_info),
>> > +	DRM_IOCTL_NOVA_GPU_INFO		= DRM_IOWR(DRM_COMMAND_BASE + DRM_NOVA_GPU_INFO,
>> > +						   struct drm_nova_gpu_info),
>> >  };
>> >  
>> >  #if defined(__cplusplus)
>> > -- 
>> > 2.54.0
>>