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

[Joel Fernandes <joelagnelf@nvidia.com>]

Hi Danilo,

On Wed, Sep 03, 2025 at 04:53:57PM +0200, Danilo Krummrich wrote:
> On Wed Sep 3, 2025 at 2:29 PM CEST, Alexandre Courbot wrote:
> > To be honest I am not completely sure about the best layout yet and will
> > need more visibility to understand whether this is optimal. But
> > considering that we want to run the GSP boot process over a built `Gpu`
> > instance, we cannot store the result of said process inside `Gpu` unless
> > we put it inside e.g. an `Option`. But then the variant will always be
> > `Some` after `probe` returns, and yet we will have to perform a match
> > every time we want to access it.
> >
> > The current separation sounds reasonable to me for the time being, with
> > `Gpu` containing purely hardware resources obtained without help from
> > user-space, while `Gsp` is the result of running a bunch of firmwares.
> > An alternative design would be to store `Gpu` inside `Gsp`, but `Gsp`
> > inside `Gpu` is trickier due to the build order. No matter what we do,
> > switching the layout later should be trivial if we don't choose the
> > best one now.
> 
> Gsp should be part of the Gpu object.

Just checking, if Gsp is a part of Gpu as you mentioned, and start_gsp() is
called from within Gpu::new(), does that not avoid the problem of referring
to fields of pin-initialized structs entirely, at least for this usecase for
nova?  Or is there any nova-related usecase where this is problematic?

thanks,

 - Joel



[...]

> The Gpu object represents the entire
> instance of the Gpu, including hardware ressources, firmware runtime state, etc.
> 
> The initialization of the Gsp structure doesn't really need a Gpu structure to
> be constructed, it needs certain members of the Gpu structure, i.e. order of
> initialization of the members does matter.
> 
> If it makes things more obvious we can always create new types and increase the
> hierarchy within the Gpu struct itself.
> 
> The technical limitation you're facing is always the same, no matter the layout
> we choose: we need pin-init to provide us references to already initialized
> members.
> 
> I will check with Benno in today's Rust call what's the best way to address
> this.
> 
> > There is also an easy workaround to the sibling initialization issue,
> > which is to store `Gpu` and `Gsp` behind `Pin<KBox>` - that way we can
> > initialize both outside `try_pin_init!`, at the cost of two more heap
> > allocations over the whole lifetime of the device. If we don't have a
> > proper solution to the problem now, this might be better than using
> > `unsafe` as a temporary solution.
> 
> Yeah, this workaround is much easier to implement when they're siblings (less
> allocations temporarily), but let's not design things this way because of that.
> 
> As mentioned above, I will check with Benno today.
> 
> > The same workaround could also be used for to `GspFirmware` and its page
> > tables - since `GspFirmware` is temporary and can apparently be
> > discarded after the GSP is booted, this shouldn't be a big issue. This
> > will allow the driver to probe, and we can add TODO items to fix that
> > later if a solution is in sight.
> >
> >>
> >> I thought the intent was to keep temporary values local to start_gsp() and not
> >> store them next to Gpu in the same allocation?
> >
> > It is not visible in the current patchset, but `start_gsp` will
> > eventually return the runtime data of the GSP - notably its log buffers
> > and command queue, which are needed to operate it. All the rest (notably
> > the loaded firmwares) will be local to `start_gsp` and discarded upon
> > its return.
> 
> Ok, that makes sense, but it should really be part of the Gpu structure.