Nouveau - Sep 2025 - [PATCH v5 02/12] gpu: nova-core: move GSP boot code to a dedicated method

On Thu Sep 11, 2025 at 9:46 PM JST, Danilo Krummrich
wrote:
> On 9/11/25 2:17 PM, Alexandre Courbot wrote:
>> On Thu Sep 11, 2025 at 8:22 PM JST, Danilo Krummrich wrote:
>>> On 9/11/25 1:04 PM, Alexandre Courbot wrote:
>>>> +    /// Attempt to start the GSP.
>>>> +    ///
>>>> +    /// This is a GPU-dependent and complex procedure that
involves loading firmware files from
>>>> +    /// user-space, patching them with signatures, and
building firmware-specific intricate data
>>>> +    /// structures that the GSP will use at runtime.
>>>> +    ///
>>>> +    /// Upon return, the GSP is up and running, and its
runtime object given as return value.
>>>> +    pub(crate) fn start_gsp(
>>>> +        pdev: &pci::Device<device::Bound>,
>>>> +        bar: &Bar0,
>>>> +        chipset: Chipset,
>>>> +        gsp_falcon: &Falcon<Gsp>,
>>>> +        _sec2_falcon: &Falcon<Sec2>,
>>>> +    ) -> Result<()> {> +        let dev =
pdev.as_ref();
>>>> +
>>>> +        let bios = Vbios::new(dev, bar)?;
>>>> +
>>>> +        let fb_layout = FbLayout::new(chipset, bar)?;
>>>> +        dev_dbg!(dev, "{:#x?}\n", fb_layout);
>>>> +
>>>> +        Self::run_fwsec_frts(dev, gsp_falcon, bar, &bios,
&fb_layout)?;
>>>> +
>>>> +        // Return an empty placeholder for now, to be replaced
with the GSP runtime data.
>>>> +        Ok(())
>>>> +    }
>>>
>>> I'd rather create the Gsp structure already, move the code to
Gsp::new() and
>>> return an impl PinInit<Self, Error>. If you don't want to
store any of the
>>> object instances you create above yet, you can just stuff all the
code into an
>>> initializer code block, as you do in the next patch with
>>> gfw::wait_gfw_boot_completion().
>> 
>> I don't think that would work, or be any better even if it did. The
full
>> GSP initialization is pretty complex and all we need to return is one
>> object created at the beginning that doesn't need to be pinned.
>> Moreover, the process is also dependent on the GPU family and
completely
>> different on Hopper/Blackwell.
>
> Why would it not work? There is no difference between the code above being
> executed from an initializer block or directly in Gsp::new().
Yeah, that's pretty much my point. :) Why run it in an initializer if
the result doesn't need to be initialized in-place anyway?
>> You can see the whole process on [1]. `libos` is the object that is
>> returned (although its name and type will change). All the rest it
>> loading, preparing and running firmware, and that is done on the GPU. I
>> think it would be very out of place in the GSP module.
>> 
>> It is also very step-by-step: run this firmware, wait for it to
>> complete, run another one, wait for a specific message from the GSP,
run
>> the sequencer, etc. And most of this stuff is thrown away once the GSP
>> is running. That's where the limits of what we can do with
`pin_init!`
>> are reached, and the GSP object doesn't need to be pinned anyway.
>
> I don't see that, in the code you linked you have a bunch of calls that
don't
> return anything that needs to survive, this can be in an initializer block.
>
> And then you have
>
> let mut libos = gsp::GspMemObjects::new(pdev, bar)?;
>
> which only needs the device reference and the bar reference.
>
> So you can easily write this as:
>
> try_pin_init!(Self {
>    _: {
>       // all the throw-away stuff from above
>    },
>    libos <- gsp::GspMemObjects::new(pdev, bar),
>    _: {
>       libos.do_some_stuff_mutable()?;
>    }
> })
Can the second initializer block access variables created in the first?
I suspect we can also initialize `libos` first, and move everything in a
block, but then my question would be why do we need to jump through that
hoop.
>> By keeping the initialization in the GPU, we can keep the GSP object
>> architecture-independent, and I think it makes sense from a design
point
>> of view. That's not to say this code should be in `gpu.rs`, maybe
we
>> want to move it to a GPU HAL, or if we really want this as part of the
>> GSP a `gsp/boot` module supporting all the different archs. But I'd
>> prefer to think about this when we start supporting several
>> architectures.
>
> Didn't we talk about a struct Gsp that will eventually be returned by
> Self::start_gsp(), or did I make this up in my head?
>
> The way I think about this is that we'll have a struct Gsp that
represents the
> entry point in the driver to mess with the GSP command queue.
>
> But either way, this throws up two questions, if Self::start_gsp() return a
> struct GspMemObjects instead (which is probably the same thing with a
different
> name), then:
>
> Are we sure this won't need any locks? If it will need locking (which I
expect)
> then it needs pin-init.
Sorry, I have been imprecise: I should I said: "it can be moved"
rather
than "it doesn't need to be pinned". In that case I don't
think
`Gsp::new` needs to return an `impl PinInit`, right?
>
> If it never needs pinning why did you write it as
>
> gsp <- Self::start_gsp(pdev, bar, spec.chipset, gsp_falcon,
sec2_falcon)?,
>
> in a patch 3?
>> [1]
https://github.com/Gnurou/linux/blob/gsp_init_rebase/drivers/gpu/nova-core/gpu.rs#L305
Ah, I blindly copied that part from your initial suggestion [1] and
forgot to double check that part. We can use `:` here for `gsp`, as the
returned value of `start_gsp` can be moved without any issue. So if we
put it behind a lock at the `Gpu` level, the current pattern should not
be a problem as it can be moved where needed by the `Gpu` initializer.

Now I don't have a precise idea of how we are going to do locking, and
you seem to have given it more thought than I have, so please let me
know if I am still missing something.

[1] https://lore.kernel.org/rust-for-linux/DCOCL398HXDH.3QH9U6UGGIUP1 at
kernel.org/

On Thu Sep 11, 2025 at 3:26 PM CEST, Alexandre Courbot
wrote:
> On Thu Sep 11, 2025 at 9:46 PM JST, Danilo Krummrich wrote:
>> On 9/11/25 2:17 PM, Alexandre Courbot wrote:
>>> You can see the whole process on [1]. `libos` is the object that is
>>> returned (although its name and type will change). All the rest it
>>> loading, preparing and running firmware, and that is done on the
GPU. I
>>> think it would be very out of place in the GSP module.
>>> 
>>> It is also very step-by-step: run this firmware, wait for it to
>>> complete, run another one, wait for a specific message from the
GSP, run
>>> the sequencer, etc. And most of this stuff is thrown away once the
GSP
>>> is running. That's where the limits of what we can do with
`pin_init!`
>>> are reached, and the GSP object doesn't need to be pinned
anyway.
>>
>> I don't see that, in the code you linked you have a bunch of calls
that don't
>> return anything that needs to survive, this can be in an initializer
block.
>>
>> And then you have
>>
>> let mut libos = gsp::GspMemObjects::new(pdev, bar)?;
>>
>> which only needs the device reference and the bar reference.
>>
>> So you can easily write this as:
>>
>> try_pin_init!(Self {
>>    _: {
>>       // all the throw-away stuff from above
>>    },
>>    libos <- gsp::GspMemObjects::new(pdev, bar),
>>    _: {
>>       libos.do_some_stuff_mutable()?;
>>    }
>> })
>
> Can the second initializer block access variables created in the first?
No, that's not yet possible :( but I'll make it work next cycle.

---
Cheers,
Benno

On Thu, Sep 11, 2025 at 10:26:08PM +0900, Alexandre Courbot
wrote:
> On Thu Sep 11, 2025 at 9:46 PM JST, Danilo Krummrich wrote:
[..] 
> >> By keeping the initialization in the GPU, we can keep the GSP
object
> >> architecture-independent, and I think it makes sense from a design
point
> >> of view. That's not to say this code should be in `gpu.rs`,
maybe we
> >> want to move it to a GPU HAL, or if we really want this as part of
the
> >> GSP a `gsp/boot` module supporting all the different archs. But
I'd
> >> prefer to think about this when we start supporting several
> >> architectures.
> >
> > Didn't we talk about a struct Gsp that will eventually be returned
by
> > Self::start_gsp(), or did I make this up in my head?
> >
> > The way I think about this is that we'll have a struct Gsp that
represents the
> > entry point in the driver to mess with the GSP command queue.
> >
> > But either way, this throws up two questions, if Self::start_gsp()
return a
> > struct GspMemObjects instead (which is probably the same thing with a
different
> > name), then:
> >
> > Are we sure this won't need any locks? If it will need locking
(which I expect)
> > then it needs pin-init.
> 
> Sorry, I have been imprecise: I should I said: "it can be moved"
rather
> than "it doesn't need to be pinned". In that case I don't
think
> `Gsp::new` needs to return an `impl PinInit`, right?
If you don't mind clarifying for me, what is the difference between "it
doesn't need to be pinned" and "it can be moved"? AFAICS,
they mean the same
thing. If you don't want move semantics on something, the only way to
achieve
that is pinning no?. If it can be moved, and it contains locks, then that will
break unless pinned AFAICS. So if need locking in Gsp, which I think we'll
need (to support sychrnoized command queue accesses), then I think pinning is
unavoidable.

On the other hand, if just the firmware loading part is kept separate,
then perhaps that part can remain unpinned?

Any chance we can initialize the locks later? We don't need locking until
after the boot process is completed, and if there's a way we can dynamically
"pin", where we hypothetically pin after the boot process completed,
that
might also work. Though I am not sure if that's something possible in
Rust/rust4linux or if it makes sense.

Other thoughts?

thanks,

 - Joel

On Sat Sep 13, 2025 at 3:02 AM CEST, Joel Fernandes
wrote:
> Any chance we can initialize the locks later? We don't need locking
until
> after the boot process is completed, and if there's a way we can
dynamically
> "pin", where we hypothetically pin after the boot process
completed, that
> might also work. Though I am not sure if that's something possible in
> Rust/rust4linux or if it makes sense.
We can't partially initialize structures and then rely on accessing
initialized
data only. This is one of the sources for memory bugs that Rust tries to solve.

You can wrap fields into Option types and initialize them later, which would
defer pin-init calls for the price of having Option fields around.

However, we should never do such things. If there's the necessity to do
something like that, it indicates a design issue.

In this case, there's no problem, we can use pin-init without any issues
right
away, and should do so.

pin-init is going to be an essential part of *every* Rust driver given that a
lot of the C infrastruture that we abstract requires pinned initialization, such
as locks and other synchronization primitives.