John Hubbard
2025-Nov-14 02:41 UTC
[PATCH v8 2/6] gpu: nova-core: prepare Spec and Revision types for boot0/boot42
1) Decouple Revision from boot0.
2) Enhance Revision, which in turn simplifies Spec::new().
3) Also, slightly enhance the comment about Spec, to be more precise.
Cc: Alexandre Courbot <acourbot at nvidia.com>
Cc: Danilo Krummrich <dakr at kernel.org>
Cc: Timur Tabi <ttabi at nvidia.com>
Reviewed-by: Joel Fernandes <joelagnelf at nvidia.com>
Signed-off-by: John Hubbard <jhubbard at nvidia.com>
---
drivers/gpu/nova-core/gpu.rs | 26 ++++++++++++--------------
drivers/gpu/nova-core/regs.rs | 11 ++++++++++-
2 files changed, 22 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
index 7fd9e91771a6..8f438188fc03 100644
--- a/drivers/gpu/nova-core/gpu.rs
+++ b/drivers/gpu/nova-core/gpu.rs
@@ -143,17 +143,8 @@ fn try_from(value: u8) -> Result<Self> {
}
pub(crate) struct Revision {
- major: u8,
- minor: u8,
-}
-
-impl Revision {
- fn from_boot0(boot0: regs::NV_PMC_BOOT_0) -> Self {
- Self {
- major: boot0.major_revision(),
- minor: boot0.minor_revision(),
- }
- }
+ pub(crate) major: u8,
+ pub(crate) minor: u8,
}
impl fmt::Display for Revision {
@@ -162,10 +153,9 @@ 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.
revision: Revision,
}
@@ -173,9 +163,17 @@ impl Spec {
fn new(bar: &Bar0) -> Result<Spec> {
let boot0 = regs::NV_PMC_BOOT_0::read(bar);
+ Spec::try_from(boot0)
+ }
+}
+
+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: Revision::from_boot0(boot0),
+ revision: boot0.revision(),
})
}
}
diff --git a/drivers/gpu/nova-core/regs.rs b/drivers/gpu/nova-core/regs.rs
index 934003cab8a8..8c9af3c59708 100644
--- a/drivers/gpu/nova-core/regs.rs
+++ b/drivers/gpu/nova-core/regs.rs
@@ -24,7 +24,8 @@
},
gpu::{
Architecture,
- Chipset, //
+ Chipset,
+ Revision, //
},
num::FromSafeCast,
};
@@ -56,6 +57,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) -> Revision {
+ Revision {
+ major: self.major_revision(),
+ minor: self.minor_revision(),
+ }
+ }
}
// PBUS
--
2.51.2
Alexandre Courbot
2025-Nov-14 14:37 UTC
[PATCH v8 2/6] gpu: nova-core: prepare Spec and Revision types for boot0/boot42
On Fri Nov 14, 2025 at 11:41 AM JST, John Hubbard wrote:> 1) Decouple Revision from boot0. > > 2) Enhance Revision, which in turn simplifies Spec::new(). > > 3) Also, slightly enhance the comment about Spec, to be more precise. > > Cc: Alexandre Courbot <acourbot at nvidia.com> > Cc: Danilo Krummrich <dakr at kernel.org> > Cc: Timur Tabi <ttabi at nvidia.com> > Reviewed-by: Joel Fernandes <joelagnelf at nvidia.com> > Signed-off-by: John Hubbard <jhubbard at nvidia.com> > --- > drivers/gpu/nova-core/gpu.rs | 26 ++++++++++++-------------- > drivers/gpu/nova-core/regs.rs | 11 ++++++++++- > 2 files changed, 22 insertions(+), 15 deletions(-) > > diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs > index 7fd9e91771a6..8f438188fc03 100644 > --- a/drivers/gpu/nova-core/gpu.rs > +++ b/drivers/gpu/nova-core/gpu.rs > @@ -143,17 +143,8 @@ fn try_from(value: u8) -> Result<Self> { > } > > pub(crate) struct Revision { > - major: u8, > - minor: u8, > -} > - > -impl Revision { > - fn from_boot0(boot0: regs::NV_PMC_BOOT_0) -> Self { > - Self { > - major: boot0.major_revision(), > - minor: boot0.minor_revision(), > - } > - } > + pub(crate) major: u8, > + pub(crate) minor: u8,Something felt a bit off with this diff, and I only realized why now. We are moving, for no good reason, the creation of `Revision` into the boot0 (and later boot42) register, which forces us to increase the visibility of its fields. And while `Revision` is now created by a method of the register it originates from, `Spec` for some reason isn't, and we even add a `TryFrom` implementation for it here. This creates an asymmetry that has no justification afaict. Instead, what if we replaced this `from_boot0` method by a `From<BOOT0>` implementation? That way, we have consistency in how we derive our chip information structures, and this patch can be reduced to this (with the other changes below): diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs index dfeba9d5d8f6..57c20d1e7274 100644 --- a/drivers/gpu/nova-core/gpu.rs +++ b/drivers/gpu/nova-core/gpu.rs @@ -147,8 +147,8 @@ pub(crate) struct Revision { minor: u8, } -impl Revision { - fn from_boot0(boot0: regs::NV_PMC_BOOT_0) -> Self { +impl From<regs::NV_PMC_BOOT_0> for Revision { + fn from(boot0: regs::NV_PMC_BOOT_0) -> Self { Self { major: boot0.major_revision(), minor: boot0.minor_revision(), @@ -162,10 +162,9 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { } } -/// Structure holding the metadata of the GPU. +/// Structure holding a basic description of the GPU: `Chipset` and `Revision`. pub(crate) struct Spec { chipset: Chipset, - /// The revision of the chipset. revision: Revision, } @@ -173,9 +172,17 @@ impl Spec { fn new(bar: &Bar0) -> Result<Spec> { let boot0 = regs::NV_PMC_BOOT_0::read(bar); + Spec::try_from(boot0) + } +} + +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: Revision::from_boot0(boot0), + revision: boot0.into(), }) } } ... and the subsequent patches also get some simplification.> } > > impl fmt::Display for Revision { > @@ -162,10 +153,9 @@ 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`.There is no `Architecture` in this structure though?> pub(crate) struct Spec { > chipset: Chipset, > - /// The revision of the chipset. > revision: Revision, > } > > @@ -173,9 +163,17 @@ impl Spec { > fn new(bar: &Bar0) -> Result<Spec> { > let boot0 = regs::NV_PMC_BOOT_0::read(bar); > > + Spec::try_from(boot0) > + } > +} > + > +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: Revision::from_boot0(boot0), > + revision: boot0.revision(), > } > }) > } > diff --git a/drivers/gpu/nova-core/regs.rs b/drivers/gpu/nova-core/regs.rs > index 934003cab8a8..8c9af3c59708 100644 > --- a/drivers/gpu/nova-core/regs.rs > +++ b/drivers/gpu/nova-core/regs.rs > @@ -24,7 +24,8 @@ > }, > gpu::{ > Architecture, > - Chipset, // > + Chipset, > + Revision, // > }, > num::FromSafeCast, > }; > @@ -56,6 +57,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) -> Revision { > + Revision { > + major: self.major_revision(), > + minor: self.minor_revision(), > + } > + }With the `From<BOOT0> for Revision` implementation we can also drop this method.