Re: [PATCH v2 13/13] [RFC] gpu: nova: implement trait object FalconUCodeDescriptor

[John Hubbard <jhubbard@nvidia.com>]

On 12/1/25 3:39 PM, Timur Tabi wrote:
> Implement the trait object FalconUCodeDescriptor to handle the two
> versions of the Falcon microcode descriptor.
> 
> Introduce the FalconUCodeDescriptor trait to provide a unified interface
> for accessing fields in both V2 and V3 Falcon microcode descriptor formats.
> This replaces repetitive match statements in each accessor method with a
> single as_descriptor() method that returns a trait object, reducing boilerplate
> and making it easier to add new accessors in the future.
> 
> However, not all match states can be eliminated.  The FalconLoadParams
> implementation still needs to match on the version because different fields
> of the descriptor are used depending on the version.
> 
> Signed-off-by: Timur Tabi <ttabi@nvidia.com>
> ---
>  drivers/gpu/nova-core/firmware.rs | 91 ++++++++++++++++++-------------
>  1 file changed, 54 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/gpu/nova-core/firmware.rs b/drivers/gpu/nova-core/firmware.rs
> index 3008d18f9313..2ad56a387a79 100644
> --- a/drivers/gpu/nova-core/firmware.rs
> +++ b/drivers/gpu/nova-core/firmware.rs
> @@ -125,13 +125,55 @@ pub(crate) enum FalconUCodeDesc {
>      V3(FalconUCodeDescV3),
>  }
>  
> +// First define trait
> +pub(crate) trait FalconUCodeDescriptor {
> +    fn hdr(&self) -> u32;
> +    fn imem_load_size(&self) -> u32;
> +    fn interface_offset(&self) -> u32;
> +    fn dmem_load_size(&self) -> u32;
> +    fn pkc_data_offset(&self) -> u32;
> +    fn engine_id_mask(&self) -> u16;
> +    fn ucode_id(&self) -> u8;
> +    fn signature_count(&self) -> u8;
> +    fn signature_versions(&self) -> u16;
> +}
> +
> +impl FalconUCodeDescriptor for FalconUCodeDescV2 {
> +    fn hdr(&self) -> u32  { self.hdr }
> +    fn imem_load_size(&self) -> u32  { self.imem_load_size }
> +    fn interface_offset(&self) -> u32  { self.interface_offset }
> +    fn dmem_load_size(&self) -> u32 { self.dmem_load_size }
> +    fn pkc_data_offset(&self) -> u32 { 0 }
> +    fn engine_id_mask(&self) -> u16 { 0 }
> +    fn ucode_id(&self) -> u8 { 0 }
> +    fn signature_count(&self) -> u8 { 0 }
> +    fn signature_versions(&self) -> u16 { 0 }
> +}
> +
> +impl FalconUCodeDescriptor for FalconUCodeDescV3 {
> +    fn hdr(&self) -> u32  { self.hdr }
> +    fn imem_load_size(&self) -> u32  { self.imem_load_size }
> +    fn interface_offset(&self) -> u32  { self.interface_offset }
> +    fn dmem_load_size(&self) -> u32 { self.dmem_load_size }
> +    fn pkc_data_offset(&self) -> u32 { self.pkc_data_offset }
> +    fn engine_id_mask(&self) -> u16 { self.engine_id_mask }
> +    fn ucode_id(&self) -> u8 { self.ucode_id }
> +    fn signature_count(&self) -> u8 { self.signature_count }
> +    fn signature_versions(&self) -> u16 { self.signature_versions }
> +}

Being able to see the differences between v2 and v3 in just 20 lines
of code is surprisingly helpful. I like this much more than I expected
to like it. :)

> +
>  impl FalconUCodeDesc {
> +    // Return trait object, the only match needed.
> +    pub(crate) fn as_descriptor(&self) -> &dyn FalconUCodeDescriptor {
> +        match self {
> +            FalconUCodeDesc::V2(v2) => v2,
> +            FalconUCodeDesc::V3(v3) => v3,
> +        }
> +    }
> +
>      /// Returns the size in bytes of the header.
>      pub(crate) fn size(&self) -> usize {
> -        let hdr = match self {
> -            FalconUCodeDesc::V2(v2) => v2.hdr,
> -            FalconUCodeDesc::V3(v3) => v3.hdr,
> -        };
> +        let hdr = self.as_descriptor().hdr();
>  
>          const HDR_SIZE_SHIFT: u32 = 16;
>          const HDR_SIZE_MASK: u32 = 0xffff0000;
> @@ -139,60 +181,35 @@ pub(crate) fn size(&self) -> usize {
>      }
>  
>      pub(crate) fn imem_load_size(&self) -> u32 {
> -        match self {
> -            FalconUCodeDesc::V2(v2) => v2.imem_load_size,
> -            FalconUCodeDesc::V3(v3) => v3.imem_load_size,
> -        }
> +        self.as_descriptor().imem_load_size()
>      }
>  
>      pub(crate) fn interface_offset(&self) -> u32 {
> -        match self {
> -            FalconUCodeDesc::V2(v2) => v2.interface_offset,
> -            FalconUCodeDesc::V3(v3) => v3.interface_offset,
> -        }
> +        self.as_descriptor().interface_offset()
>      }
>  
> -
>      pub(crate) fn dmem_load_size(&self) -> u32 {
> -        match self {
> -            FalconUCodeDesc::V2(v2) => v2.dmem_load_size,
> -            FalconUCodeDesc::V3(v3) => v3.dmem_load_size,
> -        }
> +        self.as_descriptor().dmem_load_size()
>      }
>  
>      pub(crate) fn pkc_data_offset(&self) -> u32 {
> -        match self {
> -            FalconUCodeDesc::V2(_v2) => 0,
> -            FalconUCodeDesc::V3(v3) => v3.pkc_data_offset,
> -        }
> +        self.as_descriptor().pkc_data_offset()
>      }
>  
>      pub(crate) fn engine_id_mask(&self) -> u16 {
> -        match self {
> -            FalconUCodeDesc::V2(_v2) => 0,
> -            FalconUCodeDesc::V3(v3) => v3.engine_id_mask,
> -        }
> +        self.as_descriptor().engine_id_mask()
>      }
>  
>      pub(crate) fn ucode_id(&self) -> u8 {
> -        match self {
> -            FalconUCodeDesc::V2(_v2) => 0,
> -            FalconUCodeDesc::V3(v3) => v3.ucode_id,
> -        }
> +        self.as_descriptor().ucode_id()
>      }
>  
>      pub(crate) fn signature_count(&self) -> u8 {
> -        match self {
> -            FalconUCodeDesc::V2(_v2) => 0,
> -            FalconUCodeDesc::V3(v3) => v3.signature_count,
> -        }
> +        self.as_descriptor().signature_count()
>      }
>  
>      pub(crate) fn signature_versions(&self) -> u16 {
> -        match self {
> -            FalconUCodeDesc::V2(_v2) => 0,
> -            FalconUCodeDesc::V3(v3) => v3.signature_versions,
> -        }
> +        self.as_descriptor().signature_versions()
>      }
>  }
>  

Overall, I'm really preferring this end result. The differences
between v2 and v3 are concisely encapsulated, and the remaining
code is clear and less error prone.


thanks,
-- 
John Hubbard