Re: [PATCH v5 1/3] gpu: nova-core: prepare Spec and Revision types for boot0/boot42

["Alexandre Courbot" <acourbot@nvidia.com>]

On Thu Nov 6, 2025 at 10:27 AM JST, John Hubbard wrote:
> 1) Implement Display for Spec. This simplifies the dev_info!() code for
>    printing banners such as:
>
>     NVIDIA (Chipset: GA104, Architecture: Ampere, Revision: a.1)
>
> 2) Decouple Revision from boot0.
>
> 3) Enhance Revision, which in turn simplifies Spec::new().
>
> 4) Also, slightly enhance the comment about Spec, to be more precise.

A bullet-list in a patch description is a sure sign you will be asked to
split things up. :)

And in this case I think it makes all the more sense, since all these
things taken separately ought to be simple, but having them in the same
diff makes it confusing to review.

Although it's mostly the `Display` implementation that at the very least
should be its own patch, the rest can probably be kept together as it is
related, and adding an intermediate patch would require temporary code
to build Revision. The diff becomes much clearer once the impl blocks
are moved where they should be (please see below).

The comment update can be squashed together with the Revision/Spec
patch.

>
> Cc: Alexandre Courbot <acourbot@nvidia.com>
> Cc: Danilo Krummrich <dakr@kernel.org>
> Cc: Timur Tabi <ttabi@nvidia.com>
> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
> ---
>  drivers/gpu/nova-core/gpu.rs  | 45 +++++++++++++++++++----------------
>  drivers/gpu/nova-core/regs.rs |  8 +++++++
>  2 files changed, 33 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
> index 9d182bffe8b4..8173cdcd8378 100644
> --- a/drivers/gpu/nova-core/gpu.rs
> +++ b/drivers/gpu/nova-core/gpu.rs
> @@ -130,16 +130,18 @@ fn try_from(value: u8) -> Result<Self> {
>  }
>  
>  pub(crate) struct Revision {
> -    major: u8,
> -    minor: u8,
> +    pub(crate) major: u8,
> +    pub(crate) minor: u8,
>  }
>  
> -impl Revision {
> -    fn from_boot0(boot0: regs::NV_PMC_BOOT_0) -> Self {
> -        Self {
> -            major: boot0.major_revision(),
> -            minor: boot0.minor_revision(),
> -        }
> +impl TryFrom<regs::NV_PMC_BOOT_0> for Spec {
> +    type Error = Error;
> +
> +    fn try_from(boot0: regs::NV_PMC_BOOT_0) -> Result<Self> {
> +        Ok(Self {
> +            chipset: boot0.chipset()?,
> +            revision: boot0.revision(),
> +        })

This impl block for `Revision` gets replaced by a block for `Spec`,
which is only declared later. Can you move it (and the one for BOOT_42
in the third patch) to the right place, next to the other impl blocks
for `Spec`?

>      }
>  }
>  
> @@ -149,7 +151,7 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
>      }
>  }
>  
> -/// Structure holding the metadata of the GPU.
> +/// Structure holding a basic description of the GPU: Architecture, Chipset and Revision.
>  pub(crate) struct Spec {
>      chipset: Chipset,
>      /// The revision of the chipset.
> @@ -160,10 +162,19 @@ impl Spec {
>      fn new(bar: &Bar0) -> Result<Spec> {
>          let boot0 = regs::NV_PMC_BOOT_0::read(bar);
>  
> -        Ok(Self {
> -            chipset: boot0.chipset()?,
> -            revision: Revision::from_boot0(boot0),
> -        })
> +        Spec::try_from(boot0)
> +    }
> +}
> +
> +impl fmt::Display for Spec {
> +    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
> +        write!(
> +            f,
> +            "Chipset: {}, Architecture: {:?}, Revision: {}",
> +            self.chipset,
> +            self.chipset.arch(),
> +            self.revision
> +        )
>      }
>  }
>  
> @@ -193,13 +204,7 @@ pub(crate) fn new<'a>(
>      ) -> impl PinInit<Self, Error> + 'a {
>          try_pin_init!(Self {
>              spec: Spec::new(bar).inspect(|spec| {
> -                dev_info!(
> -                    pdev.as_ref(),
> -                    "NVIDIA (Chipset: {}, Architecture: {:?}, Revision: {})\n",
> -                    spec.chipset,
> -                    spec.chipset.arch(),
> -                    spec.revision
> -                );
> +                dev_info!(pdev.as_ref(),"NVIDIA ({})\n", spec);
>              })?,
>  
>              // We must wait for GFW_BOOT completion before doing any significant setup on the GPU.
> diff --git a/drivers/gpu/nova-core/regs.rs b/drivers/gpu/nova-core/regs.rs
> index 206dab2e1335..207b865335af 100644
> --- a/drivers/gpu/nova-core/regs.rs
> +++ b/drivers/gpu/nova-core/regs.rs
> @@ -41,6 +41,14 @@ pub(crate) fn chipset(self) -> Result<Chipset> {
>              })
>              .and_then(Chipset::try_from)
>      }
> +
> +    /// Returns the revision information of the chip.
> +    pub(crate) fn revision(self) -> crate::gpu::Revision {
> +        crate::gpu::Revision {

nit: let's import `Revision`, or at least `gpu`, to align with what we
do with the other structures.