Re: [PATCH v3 02/11] gpu: nova-core: move GSP boot code out of `Gpu` constructor

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

On Tue Sep 9, 2025 at 11:43 PM JST, Danilo Krummrich wrote:
> On Tue Sep 9, 2025 at 4:11 PM CEST, Alexandre Courbot wrote:
>> On Wed Sep 3, 2025 at 5:27 PM JST, Danilo Krummrich wrote:
>>> On Wed Sep 3, 2025 at 9:10 AM CEST, Alexandre Courbot wrote:
>>>> On Wed Sep 3, 2025 at 8:12 AM JST, Danilo Krummrich wrote:
>>>>> On 9/2/25 4:31 PM, Alexandre Courbot wrote:
>>>>>>       pub(crate) fn new(
>>>>>>           pdev: &pci::Device<device::Bound>,
>>>>>>           devres_bar: Arc<Devres<Bar0>>,
>>>>>
>>>>> The diff is hiding it, but with this patch we should also make sure that this 
>>>>> returns impl PinInit<Self, Error> rather than Result<impl PinInit<Self>.
>>>>>
>>>>> I think this should be possible now.
>>>>
>>>> There is still code that can return errors (falcon creation, etc) - do
>>>> you mean that we should move it into the pin initializer and turn it
>>>> into a `try_pin_init`?
>>>
>>> Yeah, that would be better practice, if it doesn't work out for a good reason
>>> we can also fall back to Result<impl PinInit<Self, Error>, but we should at
>>> least try to avoid it.
>>
>> I tried but could not do it in a way that is satisfying. The problem is
>> that `Gpu::new` receives a `Arc<Devres<Bar0>>`, which we need to
>> `access` in order to do anything useful with it. If we first store it
>> into the `Gpu` structure, then every subsequent member needs to `access`
>> it in its own code block in order to perform their own initialization.
>> This is quite cumbersome.
>>
>> If there is a way to obtain the `Bar0` once after the `bar` member of
>> `Gpu` is initialized, and then use that instance with each remaining
>> member, then that problem would go away but I am not aware of such a
>> thing.
>
> What about this?
>
> 	impl Gpu {
> 	    pub(crate) fn new<'a>(
> 	        dev: &'a Device<Bound>,
> 	        bar: &'a Bar0
> 	        devres_bar: Arc<Devres<Bar0>>,
> 	    ) -> impl PinInit<Self, Error> + 'a {
> 	        try_pin_init(Self {
> 	            bar: devres_bar,
> 	            spec: Spec::new(bar)?,
> 	            gsp_falcon: Falcon::<Gsp>::new(dev, spec.chipset)?,
> 	            sec2_falcon: Falcon::<Sec2>::new(dev, spec.chipset)?,
> 	            sysmem_flush: SysmemFlush::register(dev, bar, spec.chipset)?
> 	            gsp <- Gsp::new(gsp_falcon, sec2_falcon, sysmem_flush)?,
> 	        })
> 	    }
> 	}

It does work. The bizareness of passing the `bar` twice aside, here is
what it looks like when I got it to compile:

    pub(crate) fn new<'a>(
        pdev: &'a pci::Device<device::Bound>,
        devres_bar: Arc<Devres<Bar0>>,
        bar: &'a Bar0,
    ) -> 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
                );
            })?,

            sysmem_flush: SysmemFlush::register(pdev.as_ref(), bar, spec.chipset)?,

            gsp_falcon: Falcon::<Gsp>::new(
                pdev.as_ref(),
                spec.chipset,
                bar,
                spec.chipset > Chipset::GA100,
            )
            .inspect(|falcon| falcon.clear_swgen0_intr(bar))?,

            sec2_falcon: Falcon::<Sec2>::new(pdev.as_ref(), spec.chipset, bar, true)?,

            gsp: Self::start_gsp(pdev, bar, spec.chipset, gsp_falcon, sec2_falcon)?,

            bar: devres_bar,
        })
    }

The wait for GFW initialization had to be moved to `probe`, but that's
fine IMO. I do however find the code less readable in this form, less
editable as well. And LSP seems lost, so I don't get any syntax
highlighting in the `try_pin_init` block.

Fundamentally, this changes the method from a fallible method returning
a non-fallible initializer into a non-fallible method returning a
fallible initializer. I'm ok with that, and maybe this new form will
encourage us to keep this method short, which is what we want, but other
than that what benefit are we getting from this change?