Re: [PATCH v5 05/11] drm: nova: Add an info ioctl

[Alistair Popple]

On 2026-09-01 at 00:23 +1000, Danilo Krummrich <dakr@xxxxxxxxxx> wrote...
> On Fri Aug 28, 2026 at 5:35 AM CEST, Alistair Popple wrote:
> > +/// GPU information returned to userspace.
> > +///
> > +/// # Invariants
> > +///
> > +/// - The layout of this type is identical to `struct drm_nova_gpu_info`.
> > +/// - All bytes in the value are initialized.
> 
> I don't think we need those invariants. The first one is covered by
> #[repr(transparent)] and the second invariant is trivally satisfied by the fact
> that we create a value of that type.

Ok.

> Maybe you meant to say that we explicitly initialized everything despite
> uapi::drm_nova_gpu_info being FromBytes (i.e. no "random" values)? But I think
> even that wouldn't need an invariant.

Yep, basically trying to justify why As/FromBytes is safe but if the safety
comments for these implementations on their own are adequate I will just use
those and remove this.

> > +#[repr(transparent)]
> > +struct GpuInfo(uapi::drm_nova_gpu_info);
> > +
> > +impl GpuInfo {
> > +    fn new(reg_data: &DrmRegData<'_>) -> Self {
> > +        Self(uapi::drm_nova_gpu_info {
> > +            architecture: reg_data.api.architecture(),
> > +            implementation: reg_data.api.implementation(),
> > +        })
> > +    }
> > +}
> > +
> > +// SAFETY: `GpuInfo` has no implicit padding, kernel pointers, or interior
> > +// mutability, and all of its fields are initialized before it is written to
> > +// userspace.
> > +unsafe impl AsBytes for GpuInfo {}
> > +
> > +fn write_info<T: AsBytes>(info: &mut uapi::drm_nova_info, value: &T) -> Result {
> 
> We could take T by value I guess? We don't need it anymore after it has been
> written to the user buffer.

Makes sense.

> > +    let mut writer =
> > +        UserSlice::new(UserPtr::from_addr(info.data as usize), info.size as usize).writer();
> > +
> > +    info.size = writer.write_truncated(value)? as u32;
> > +
> > +    Ok(())
> > +}
> > +
> >  impl drm::file::DriverFile for File {
> >      type Driver = NovaDriver;
> >  
> > @@ -78,4 +112,27 @@ pub(crate) fn gem_info(
> >  
> >          Ok(0)
> >      }
> > +
> > +    /// IOCTL: info: Query device information.
> > +    pub(crate) fn info(
> > +        _dev: &NovaDevice<Registered>,
> > +        reg_data: &DrmRegData<'_>,
> > +        info: &mut uapi::drm_nova_info,
> > +        _file: &drm::File<File>,
> > +    ) -> Result<u32> {
> > +        if info.data == 0 {
> > +            info.size = match info.id {
> > +                uapi::DRM_NOVA_INFO_GPU => size_of::<GpuInfo>() as u32,
> > +                _ => return Err(EINVAL),
> > +            };
> > +            return Ok(0);
> > +        }
> 
> I think this check can go into write_info(), so we don't have to repeat this for
> every info. I.e. we can just add
> 
> 	if info.data == 0 {
> 	    info.size = size_of::<T>() as u32;
> 	    return Ok(());
> 	}
> 
> at the beginning of write_info(). It may construct the value even if
> info.data == 0, but I don't think we care. :)

Heh. That was why I did it this way, in case obtaining the info was expensive
for some value of "expensive". But info should mostly be cached (ie. cheap) and
I don't mind keeping things simple :)

> 
> > +
> > +        match info.id {
> > +            uapi::DRM_NOVA_INFO_GPU => write_info(info, &GpuInfo::new(reg_data))?,
> > +            _ => return Err(EINVAL),
> > +        }
> > +
> > +        Ok(0)
> > +    }
> >  }
> > diff --git a/drivers/gpu/nova-core/api.rs b/drivers/gpu/nova-core/api.rs
> > index 610cfc01111e..cff730a38c1d 100644
> > --- a/drivers/gpu/nova-core/api.rs
> > +++ b/drivers/gpu/nova-core/api.rs
> > @@ -12,11 +12,12 @@
> >      types::CovariantForLt, //
> >  };
> >  
> > -use crate::gpu::Gpu;
> > +use crate::gpu::{
> > +    Gpu, //
> > +};
> >  
> >  /// API handle for the auxiliary bus child drivers to interact with nova-core.
> >  pub struct NovaCoreApi<'bound> {
> > -    #[expect(unused)]
> >      pub(crate) gpu: Pin<&'bound Gpu<'bound>>,
> >  }
> >  
> > @@ -26,4 +27,14 @@ impl NovaCoreApi<'_> {
> >      pub fn of(adev: &auxiliary::Device<Bound>) -> Result<Pin<&NovaCoreApi<'_>>> {
> >          adev.registration_data::<CovariantForLt!(NovaCoreApi<'_>)>()
> >      }
> > +
> > +    /// Returns the architecture identifier of this GPU.
> > +    pub fn architecture(&self) -> u32 {
> > +        self.gpu.spec.chipset.arch() as u32
> > +    }
> > +
> > +    /// Returns the implementation identifier of this GPU.
> > +    pub fn implementation(&self) -> u32 {
> > +        self.gpu.spec.chipset.implementation()
> > +    }
> 
> We should make the NovaCoreApi just provide an accessor for &Spec and make every
> subsequent method we need public. Otherwise we end up with endless forwarding
> methods. We can also add as_raw() methods to the specific types as needed.

Ok. This is where I don't have a good instinct for what we think should be an
accessor/forwarding method vs. where we should just expose the underlying data
structure and required methods to API users.

In the past it seems there's been some resistance to exposing nova-core or gsp
data structures like this which is why I added the forwarding methods. In future
we're going to have other data-structures that NovaCoreApi will need to access
so it would be good to understand what we should do here so we can keep things
somewhat consistent.

It seems pretty arbitrary to say expose self.gpu.spec externally
because of endless forwarding methods but to then require them for say
self.gpu.gsp_static_info. So maybe we can just have a NovaCoreApi method that
returns self.gpu directly instead of writing forwarding methods for each field
of self.gpu that we need to access? Access to genuinely internal nova-core
fields/methods can always be controlled through visibility modifiers.

 - Alistair