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().
> 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()?;
}
})
> 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.
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
