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.
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(),
+ })
}
}
@@ -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 {
+ major: self.major_revision(),
+ minor: self.minor_revision(),
+ }
+ }
}
// PBUS
--
2.51.2On 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.
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
© 2016 - 2025 Red Hat, Inc.