Re: [PATCH v5 09/11] drm: nova: Report GPU name in GPU info

[Alistair Popple]

On 2026-09-01 at 00:33 +1000, Danilo Krummrich <dakr@xxxxxxxxxx> wrote...
> On Fri Aug 28, 2026 at 5:35 AM CEST, Alistair Popple wrote:
> >  impl NovaCoreApi<'_> {
> > +    /// Returns the NUL-terminated full GPU name supplied by GSP-RM.
> > +    pub fn gpu_name(&self) -> [u8; 64] {
> 
> I don't really like that we have two separate accessors for this, can't we just
> use the one we already have, which also does all the validation already?

Yeah, I don't know how I forgot we already had an accessor for that. I've been
carrying this patch on my own tree for quite a while, so guess just enough code
moved around change that I missed it.

Anyway I agree, a new acessor is totally unnecessary.

> The constructor of GpuInfo could look like this:
> 
> 	fn new(reg_data: &DrmRegData<'_>) -> Result<Self> {
> 	    let mut info = uapi::drm_nova_gpu_info {
> 	        architecture: reg_data.api.architecture(),
> 	        implementation: reg_data.api.implementation(),
> 	        ..Default::default()
> 	    };
> 	    let bytes = reg_data.api.gpu_name()?.as_bytes();
> 	    info.gpu_name[..bytes.len()].copy_from_slice(bytes);
> 	    Ok(Self(info))
> 	}
> 
> Could also be infallible if we want to go with a fallback name, given that we
> consider the firmware not providing something useful as non-fatal so far.

Yeah, I think keeping this infallible makes sense. 

> > +        *self.gpu.gsp_static_info.gpu_name_bytes()
> > +    }
> > +
> >      /// Obtain a [`NovaCoreApi`] handle from an auxiliary device registered
> >      /// by nova-core.
> >      pub fn of(adev: &auxiliary::Device<Bound>) -> Result<Pin<&NovaCoreApi<'_>>> {
> > diff --git a/drivers/gpu/nova-core/gsp/commands.rs b/drivers/gpu/nova-core/gsp/commands.rs
> > index 6453184af55b..b8dc64808620 100644
> > --- a/drivers/gpu/nova-core/gsp/commands.rs
> > +++ b/drivers/gpu/nova-core/gsp/commands.rs
> > @@ -251,6 +251,11 @@ pub(crate) enum GpuNameError {
> >  }
> >  
> >  impl GetGspStaticInfoReply {
> > +    /// Returns the full GPU name as a NUL-terminated byte string.
> > +    pub(crate) fn gpu_name_bytes(&self) -> &[u8; 64] {
> > +        &self.gpu_name
> > +    }
> 
> AFAICS there's nothing ensuring that this is actually NULL terminated? The
> existing gpu_name() method already does this.

Yep, relied on FW which is sub-optimal. The existing accessors obviously fix
that.

 - Alistair