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

["Alexandre Courbot" <acourbot@nvidia.com>]

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()`.

>
> 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@kernel.org>
> Cc: Gary Guo <gary@garyguo.net>
> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
> ---
>  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.