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

On 2026-08-25 at 05:10 +1000, Danilo Krummrich <[email protected]> wrote...
> 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.

I don't think so - see my response to patch 3 but I think the architecture
decoding needs to stay in user-space.

> >> > +
> >> > +	/**
> >> > +	 * @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.

But isn't v2...vN info ioctls dealt with in the usual way by extending the
existing struct and bumping the size? That seems like a pretty clean and
self-contained API to me.

> 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.

Doesn't that question also apply to adding GETPARAM N+1 though? If we're not
worried about running out of top-level ioctls I don't understand why they
are considered special enough to warrant the extra complexity of creating and
decoding a hiearchy of sub-ioctls.

> 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.

Sure, we could just reserve a block of top-level ioctls for info though if we
really cared about them being grouped together numerically. I'm not sure why
keeping them group numerically really matters though, it's nice to have but I
don't think it warrants any extra code.

But perhaps seeing an actual user of this API would be helpful for this
discussion, I will aim to do a pull request to drm-test this week to
demonstrate.

 - Alistair

> > 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
> >> 
>