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 <[email protected]> >> Cc: Danilo Krummrich <[email protected]> >> Cc: Timur Tabi <[email protected]> >> Signed-off-by: John Hubbard <[email protected]> >> --- >> 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
