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