Implement Display for Spec. This simplifies the dev_info!() code for
printing banners such as:
NVIDIA (Chipset: GA104, Architecture: Ampere, Revision: a.1)
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 | 20 +++++++++++++-------
1 file changed, 13 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
index 802e71e4f97d..7fd9e91771a6 100644
--- a/drivers/gpu/nova-core/gpu.rs
+++ b/drivers/gpu/nova-core/gpu.rs
@@ -180,6 +180,18 @@ fn new(bar: &Bar0) -> Result<Spec> {
}
}
+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
+ )
+ }
+}
+
/// Structure holding the resources required to operate the GPU.
#[pin_data]
pub(crate) struct Gpu {
@@ -206,13 +218,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.
--
2.51.2On Fri, 2025-11-07 at 20:39 -0800, John Hubbard wrote:
> Implement Display for Spec. This simplifies the dev_info!() code for
> printing banners such as:
>
> NVIDIA (Chipset: GA104, Architecture: Ampere, Revision: a.1)
>
> 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>
I'm okay with the entire patch set, but I do have a few questions.
> +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
> + )
> + }
> +}
> +
> /// Structure holding the resources required to operate the GPU.
> #[pin_data]
> pub(crate) struct Gpu {
> @@ -206,13 +218,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);
I believe that this is the only place where a `Spec` is actually printed. Does it really make
sense to implement Display for a single usage? Do we generally want to implement Display for
new types?
On 11/7/25 9:02 PM, Timur Tabi wrote:
> On Fri, 2025-11-07 at 20:39 -0800, John Hubbard wrote:
>> Implement Display for Spec. This simplifies the dev_info!() code for
>> printing banners such as:
>>
>> NVIDIA (Chipset: GA104, Architecture: Ampere, Revision: a.1)
>>
>> 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>
>
> I'm okay with the entire patch set, but I do have a few questions.
Great!
>
>> +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
>> + )
>> + }
>> +}
>> +
>> /// Structure holding the resources required to operate the GPU.
>> #[pin_data]
>> pub(crate) struct Gpu {
>> @@ -206,13 +218,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);
>
> I believe that this is the only place where a `Spec` is actually printed. Does it really make
> sense to implement Display for a single usage? Do we generally want to implement Display for
> new types?
>
I agree that it looks a little excessive, but I defer to reviewers
who have a lot more Rust experience, and they requested this during
a review of an earlier version.
thanks,
--
John Hubbard
On Sat Nov 8, 2025 at 2:10 PM JST, John Hubbard wrote:
> On 11/7/25 9:02 PM, Timur Tabi wrote:
>> On Fri, 2025-11-07 at 20:39 -0800, John Hubbard wrote:
>>> Implement Display for Spec. This simplifies the dev_info!() code for
>>> printing banners such as:
>>>
>>> NVIDIA (Chipset: GA104, Architecture: Ampere, Revision: a.1)
>>>
>>> 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>
>>
>> I'm okay with the entire patch set, but I do have a few questions.
>
> Great!
>
>>
>>> +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
>>> + )
>>> + }
>>> +}
>>> +
>>> /// Structure holding the resources required to operate the GPU.
>>> #[pin_data]
>>> pub(crate) struct Gpu {
>>> @@ -206,13 +218,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);
>>
>> I believe that this is the only place where a `Spec` is actually printed. Does it really make
>> sense to implement Display for a single usage? Do we generally want to implement Display for
>> new types?
>>
>
> I agree that it looks a little excessive, but I defer to reviewers
> who have a lot more Rust experience, and they requested this during
> a review of an earlier version.
I think this is the correct way to do; `Spec` should be the one to
decide how it is displayed, and from a maintainability perspective this
ensures that other sites that will want to print a `Spec` in the future
will reuse this implementation, instead of either rewriting one
themselves or having to figure out that there was already an existing
site and factor it out.
Iow, this code is proactively doing the right thing.
On Sat, Nov 8, 2025 at 12:42 PM Alexandre Courbot <acourbot@nvidia.com> wrote: > > I think this is the correct way to do; `Spec` should be the one to > decide how it is displayed, and from a maintainability perspective this > ensures that other sites that will want to print a `Spec` in the future > will reuse this implementation, instead of either rewriting one > themselves or having to figure out that there was already an existing > site and factor it out. > > Iow, this code is proactively doing the right thing. Yes, please, avoid hardcoding inlined display code. Cheers, Miguel
© 2016 - 2025 Red Hat, Inc.