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.
