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

[Alistair Popple]

On 2026-08-26 at 08:37 +1000, Danilo Krummrich <dakr@xxxxxxxxxx> wrote...
> On Tue Aug 25, 2026 at 9:32 AM CEST, Alistair Popple wrote:
> > On 2026-08-25 at 05:10 +1000, Danilo Krummrich <dakr@xxxxxxxxxx> wrote...
> >> 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.
> 
> To be clear, my main point is that having a single info ioctl with different
> info types is more self-contained and provides more flexibility to introduce new
> info types whenever we think it is warranted. Long term I expect it to be the
> cleaner API.
> 
> >> 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 the code would be rather trivial:

Yeah, I agree it's nothing too complicated. It is still more code though, and
therefore more code for people to grok and more code for every userspace user
to implement (not always in Rust!) rather than relying on existing code. And it
does add another layer of semantics to understand, even if they are similar, so
in that sense I regard it as more complex.

But it seems unlikely further discussion will change much here and I'd like to
get this merged soon. Obviously you consider this worthwhile so I'll just go
ahead with this approach for v5.

> 
> 	fn write_info<T: AsBytes>(info: &mut uapi::drm_nova_info, value: &T) -> Result {
> 	    let len = size_of_val(value).min(info.size);
> 	    let uptr = UserPtr::from_addr(info.data);
> 	    let mut writer = UserSlice::new(uptr, len).writer();
> 
> 	    // Note: I made this up, as I think we want to add this method to
> 	    // `UserSliceWriter`, to avoid having to call `as_bytes()`.
> 	    writer.write_truncated(&value)?;
> 	    info.size = len;
> 
> 	    Ok(())
> 	}
> 
> 	match info.id {
> 	    uapi::DRM_NOVA_INFO_GPU => write_info(info, &uapi::drm_nova_gpu_info { ... })?,
> 	    uapi::DRM_NOVA_INFO_MEM => write_info(info, &uapi::drm_nova_mem_info { ... })?,
> 	    _ => return Err(EINVAL),
> 	}
> 
> Honestly, I think this is even less complicated that adding a new ioctl for a
> new info struct.
> 
> And on the userspace side:
> 
> 	fn query_info<T: Default>(fd: &DrmDevice, id: DrmNovaInfoId) -> Result<T> {
> 	    let mut value = T::default();
> 	    let mut info = drm_nova_info {
> 	        id: id.as_raw(),
> 	        size: size_of::<T>(),
> 	        data: ptr::from_mut(&mut value) as u64,
> 	    };
> 
> 	    fd.ioctl(DRM_IOCTL_NOVA_INFO, &mut info)?;
> 	    Ok(value)
> 	}
> 
> 	let gpu_info: drm_nova_gpu_info = query_info(&dev, DRM_NOVA_INFO_GPU)?;
> 	let mem_info: drm_nova_mem_info = query_info(&dev, DRM_NOVA_INFO_MEM)?;
> 
> (I pushed a few cleanups to drm-test, so this code should work.)

Thanks. I've already taken a look at drm-test, seems fairly easy to follow.

 - Alistair