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

[John Hubbard <jhubbard@nvidia.com>]

On 11/5/25 11:45 PM, Alexandre Courbot wrote:
> 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. :)

Yes. That is an eternal truth, which I foolishly ignored here. haha

> 
> 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.
> 

Will do.

>>
>> 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`?

Yes.

> 
>>      }
>>  }
>>  
>> @@ -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.

Yes.

thanks,
-- 
John Hubbard