Nouveau - Nov 2025 - [PATCH v6 1/4] gpu: nova-core: implement Display for Spec

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 at nvidia.com>
>> Cc: Danilo Krummrich <dakr at kernel.org>
>> Cc: Timur Tabi <ttabi at nvidia.com>
>> Signed-off-by: John Hubbard <jhubbard at 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 at nvidia.com>
>>> Cc: Danilo Krummrich <dakr at kernel.org>
>>> Cc: Timur Tabi <ttabi at nvidia.com>
>>> Signed-off-by: John Hubbard <jhubbard at 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.