Re: [PATCH v7 04/31] gpu: nova-core: move GPU init into Gpu::new()

[John Hubbard]

On 3/23/26 5:45 AM, Alexandre Courbot wrote:
> On Wed Mar 18, 2026 at 7:53 AM JST, John Hubbard wrote:
>> Move Spec creation and the dev_info log from the driver's probe() into
>> Gpu::new(), so that GPU-specific identification lives in the Gpu
>> constructor.
> 
> That's not what this patch does - Spec is already created in `Gpu::new()`.

Fixed in v8, along with all of the other comments below.

thanks,
-- 
John Hubbard
> 
>>
>> Restructure Gpu::new() to use pin_init_scope wrapping try_pin_init!,
>> which allows running fallible setup code (Spec::new) before the
>> pin-initializer. Add Spec::chipset() accessor for use by later patches.
> 
> What is missing is why we are doing this. It looks completely unneeded
> even when looking at the following patches.
> 
>>
>> The DMA mask setup stays in probe() where the safety argument for
>> dma_set_mask_and_coherent is straightforward.
> 
> Knowing the history of the series I understand why this sentence is
> here, but as of this revision it is not relevant.
> 
>>
>> Cc: Danilo Krummrich <dakr@xxxxxxxxxx>
>> Cc: Gary Guo <gary@xxxxxxxxxxx>
>> Signed-off-by: John Hubbard <jhubbard@xxxxxxxxxx>
>> ---
>>  drivers/gpu/nova-core/gpu.rs | 49 +++++++++++++++++++++---------------
>>  1 file changed, 29 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
>> index 3b4ccc3d18b9..8f317d213908 100644
>> --- a/drivers/gpu/nova-core/gpu.rs
>> +++ b/drivers/gpu/nova-core/gpu.rs
>> @@ -102,7 +102,7 @@ fn try_from(value: u32) -> Result<Self, Self::Error> {
>>  });
>>  
>>  impl Chipset {
>> -    pub(crate) const fn arch(self) -> Architecture {
>> +    pub(crate) const fn arch(&self) -> Architecture {
> 
> Why are we taking `self` by reference now? `Chipset` implements `Copy`
> so this should not be needed.
> 
>>          match self {
>>              Self::TU102 | Self::TU104 | Self::TU106 | Self::TU117 | Self::TU116 => {
>>                  Architecture::Turing
>> @@ -241,6 +241,10 @@ fn new(dev: &device::Device, bar: &Bar0) -> Result<Spec> {
>>              dev_err!(dev, "Unsupported chipset: {}\n", boot42);
>>          })
>>      }
>> +
>> +    pub(crate) fn chipset(&self) -> Chipset {
>> +        self.chipset
>> +    }
> 
> Short doccomment please, even if it is obvious by the method name what
> it does.
> 
>>  }
>>  
>>  impl TryFrom<regs::NV_PMC_BOOT_42> for Spec {
>> @@ -289,32 +293,37 @@ pub(crate) fn new<'a>(
>>          devres_bar: Arc<Devres<Bar0>>,
>>          bar: &'a Bar0,
>>      ) -> impl PinInit<Self, Error> + 'a {
>> -        try_pin_init!(Self {
>> -            spec: Spec::new(pdev.as_ref(), bar).inspect(|spec| {
>> -                dev_info!(pdev,"NVIDIA ({})\n", spec);
>> -            })?,
>> +        pin_init::pin_init_scope(move || {
>> +            let spec = Spec::new(pdev.as_ref(), bar)?;
>> +            dev_info!(pdev, "NVIDIA ({})\n", spec);
>> +
>> +            let chipset = spec.chipset();
>>  
>> -            // We must wait for GFW_BOOT completion before doing any significant setup on the GPU.
>> -            _: {
>> -                gfw::wait_gfw_boot_completion(bar)
>> -                    .inspect_err(|_| dev_err!(pdev, "GFW boot did not complete\n"))?;
>> -            },
>> +            Ok(try_pin_init!(Self {
>> +                // We must wait for GFW_BOOT completion before doing any significant setup
>> +                // on the GPU.
>> +                _: {
>> +                    gfw::wait_gfw_boot_completion(bar)
>> +                        .inspect_err(|_| dev_err!(pdev, "GFW boot did not complete\n"))?;
>> +                },
>>  
>> -            sysmem_flush: SysmemFlush::register(pdev.as_ref(), bar, spec.chipset)?,
>> +                sysmem_flush: SysmemFlush::register(pdev.as_ref(), bar, chipset)?,
>>  
>> -            gsp_falcon: Falcon::new(
>> -                pdev.as_ref(),
>> -                spec.chipset,
>> -            )
>> -            .inspect(|falcon| falcon.clear_swgen0_intr(bar))?,
>> +                gsp_falcon: Falcon::new(
>> +                    pdev.as_ref(),
>> +                    chipset,
>> +                )
>> +                .inspect(|falcon| falcon.clear_swgen0_intr(bar))?,
>>  
>> -            sec2_falcon: Falcon::new(pdev.as_ref(), spec.chipset)?,
>> +                sec2_falcon: Falcon::new(pdev.as_ref(), chipset)?,
>>  
>> -            gsp <- Gsp::new(pdev),
>> +                gsp <- Gsp::new(pdev),
>>  
>> -            _: { gsp.boot(pdev, bar, spec.chipset, gsp_falcon, sec2_falcon)? },
>> +                _: { gsp.boot(pdev, bar, chipset, gsp_falcon, sec2_falcon)? },
>>  
>> -            bar: devres_bar,
>> +                bar: devres_bar,
>> +                spec,
>> +            }))
> 
> This diff is basically a no-op? The commit message should help me make
> sense of it but since it is incorrect, I can only guess - and I suspect
> you are doing this dance because `Revision` and `Spec` do not implement
> `Copy`.
> 
> `Spec` is 4 bytes, both it and `Revision` are prime candidates for a
> `#[derive(Clone, Copy)]` (and `fmt::Debug` while we are at it).
> 
> I expect that just doing that will simplify the following patches as well.