Alexandre Courbot
2025-Sep-02 14:31 UTC
[PATCH v3 02/11] gpu: nova-core: move GSP boot code out of `Gpu` constructor
Right now the GSP boot code is very incomplete and limited to running FRTS, so having it in `Gpu::new` is not a big constraint. However, this will change as we add more steps of the GSP boot process, some of which are rather cumbersome to perform on a partially-constructed GPU instance. Relatedly, booting the GSP typically happens only once in the GPU reset cycle. Most of the data created to complete this step (notably firmware loaded from user-space) is needed only temporary and can be discarded once the GSP is booted; it then makes all the more sense to store these as local variables of a dedicated method, instead of inside the `Gpu` structure where they as kept as long as the GPU is bound, using dozens of megabytes of host memory. Thus, introduce a `start_gsp` method on the `Gpu` struct, which is called by `probe` after the GPU is instantiated and will return the running GSP instance, once the code completing its boot is integrated. Signed-off-by: Alexandre Courbot <acourbot at nvidia.com> --- drivers/gpu/nova-core/driver.rs | 10 +++++++++- drivers/gpu/nova-core/gpu.rs | 41 ++++++++++++++++++++++++++++++----------- 2 files changed, 39 insertions(+), 12 deletions(-) diff --git a/drivers/gpu/nova-core/driver.rs b/drivers/gpu/nova-core/driver.rs index 274989ea1fb4a5e3e6678a08920ddc76d2809ab2..1062014c0a488e959379f009c2e8029ffaa1e2f8 100644 --- a/drivers/gpu/nova-core/driver.rs +++ b/drivers/gpu/nova-core/driver.rs @@ -6,6 +6,8 @@ #[pin_data] pub(crate) struct NovaCore { + // Placeholder for the real `Gsp` object once it is built. + pub(crate) gsp: (), #[pin] pub(crate) gpu: Gpu, _reg: auxiliary::Registration, @@ -40,8 +42,14 @@ fn probe(pdev: &pci::Device<Core>, _info: &Self::IdInfo) -> Result<Pin<KBox<Self )?; let this = KBox::pin_init( - try_pin_init!(Self { + try_pin_init!(&this in Self { gpu <- Gpu::new(pdev, bar)?, + gsp <- { + // SAFETY: `this.gpu` is initialized to a valid value. + let gpu = unsafe { &(*this.as_ptr()).gpu }; + + gpu.start_gsp(pdev)? + }, _reg: auxiliary::Registration::new( pdev.as_ref(), c_str!("nova-drm"), diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs index 4d0f759610ec6eb8a716c039a2e67859410da17c..8343276e52e6a87a8ff1aff9fc484e00d4b57417 100644 --- a/drivers/gpu/nova-core/gpu.rs +++ b/drivers/gpu/nova-core/gpu.rs @@ -172,6 +172,8 @@ pub(crate) struct Gpu { /// System memory page required for flushing all pending GPU-side memory writes done through /// PCIE into system memory, via sysmembar (A GPU-initiated HW memory-barrier operation). sysmem_flush: SysmemFlush, + gsp_falcon: Falcon<Gsp>, + sec2_falcon: Falcon<Sec2>, } #[pinned_drop] @@ -190,8 +192,8 @@ impl Gpu { /// TODO: this needs to be moved into a larger type responsible for booting the whole GSP /// (`GspBooter`?). fn run_fwsec_frts( + &self, dev: &device::Device<device::Bound>, - falcon: &Falcon<Gsp>, bar: &Bar0, bios: &Vbios, fb_layout: &FbLayout, @@ -208,7 +210,7 @@ fn run_fwsec_frts( let fwsec_frts = FwsecFirmware::new( dev, - falcon, + &self.gsp_falcon, bar, bios, FwsecCommand::Frts { @@ -218,7 +220,7 @@ fn run_fwsec_frts( )?; // Run FWSEC-FRTS to create the WPR2 region. - fwsec_frts.run(dev, falcon, bar)?; + fwsec_frts.run(dev, &self.gsp_falcon, bar)?; // SCRATCH_E contains the error code for FWSEC-FRTS. let frts_status = regs::NV_PBUS_SW_SCRATCH_0E_FRTS_ERR::read(bar).frts_err_code(); @@ -263,6 +265,28 @@ fn run_fwsec_frts( } } + /// 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(&self, pdev: &pci::Device<device::Bound>) -> Result<()> { + let dev = pdev.as_ref(); + + let bar = self.bar.access(dev)?; + + let bios = Vbios::new(dev, bar)?; + + let fb_layout = FbLayout::new(self.spec.chipset, bar)?; + dev_dbg!(dev, "{:#x?}\n", fb_layout); + + self.run_fwsec_frts(dev, bar, &bios, &fb_layout)?; + + Ok(()) + } + pub(crate) fn new( pdev: &pci::Device<device::Bound>, devres_bar: Arc<Devres<Bar0>>, @@ -293,20 +317,15 @@ pub(crate) fn new( )?; gsp_falcon.clear_swgen0_intr(bar); - let _sec2_falcon = Falcon::<Sec2>::new(pdev.as_ref(), spec.chipset, bar, true)?; - - let fb_layout = FbLayout::new(spec.chipset, bar)?; - dev_dbg!(pdev.as_ref(), "{:#x?}\n", fb_layout); - - let bios = Vbios::new(pdev.as_ref(), bar)?; - - Self::run_fwsec_frts(pdev.as_ref(), &gsp_falcon, bar, &bios, &fb_layout)?; + let sec2_falcon = Falcon::<Sec2>::new(pdev.as_ref(), spec.chipset, bar, true)?; Ok(pin_init!(Self { spec, bar: devres_bar, fw, sysmem_flush, + gsp_falcon, + sec2_falcon, })) } } -- 2.51.0
Danilo Krummrich
2025-Sep-02 19:53 UTC
[PATCH v3 02/11] gpu: nova-core: move GSP boot code out of `Gpu` constructor
On Tue Sep 2, 2025 at 4:31 PM CEST, Alexandre Courbot wrote:> diff --git a/drivers/gpu/nova-core/driver.rs b/drivers/gpu/nova-core/driver.rs > index 274989ea1fb4a5e3e6678a08920ddc76d2809ab2..1062014c0a488e959379f009c2e8029ffaa1e2f8 100644 > --- a/drivers/gpu/nova-core/driver.rs > +++ b/drivers/gpu/nova-core/driver.rs > @@ -6,6 +6,8 @@ > > #[pin_data] > pub(crate) struct NovaCore { > + // Placeholder for the real `Gsp` object once it is built. > + pub(crate) gsp: (), > #[pin] > pub(crate) gpu: Gpu, > _reg: auxiliary::Registration, > @@ -40,8 +42,14 @@ fn probe(pdev: &pci::Device<Core>, _info: &Self::IdInfo) -> Result<Pin<KBox<Self > )?; > > let this = KBox::pin_init( > - try_pin_init!(Self { > + try_pin_init!(&this in Self { > gpu <- Gpu::new(pdev, bar)?, > + gsp <- { > + // SAFETY: `this.gpu` is initialized to a valid value. > + let gpu = unsafe { &(*this.as_ptr()).gpu }; > + > + gpu.start_gsp(pdev)? > + },Please use pin_chain() [1] for this. More in general, unsafe code should be the absolute last resort. If we add new unsafe code I'd love to see a comment justifying why there's no other way than using unsafe code for this, as we agreed in [2]. I did a quick grep on this series and I see 21 occurrences of "unsafe", if I substract the ones for annotations and for FromBytes impls, it's still 9 new ones. :( Do we really need all of them? Otherwise, I really like this, it's a great improvement over initializing everything into the Gpu struct -- thanks for the refactoring! [1] https://rust.docs.kernel.org/kernel/prelude/trait.PinInit.html#method.pin_chain [2] https://docs.kernel.org/gpu/nova/guidelines.html#language
Danilo Krummrich
2025-Sep-02 23:12 UTC
[PATCH v3 02/11] gpu: nova-core: move GSP boot code out of `Gpu` constructor
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.> @@ -293,20 +317,15 @@ pub(crate) fn new( > )?; > gsp_falcon.clear_swgen0_intr(bar); > > - let _sec2_falcon = Falcon::<Sec2>::new(pdev.as_ref(), spec.chipset, bar, true)?; > - > - let fb_layout = FbLayout::new(spec.chipset, bar)?; > - dev_dbg!(pdev.as_ref(), "{:#x?}\n", fb_layout); > - > - let bios = Vbios::new(pdev.as_ref(), bar)?; > - > - Self::run_fwsec_frts(pdev.as_ref(), &gsp_falcon, bar, &bios, &fb_layout)?; > + let sec2_falcon = Falcon::<Sec2>::new(pdev.as_ref(), spec.chipset, bar, true)?; > > Ok(pin_init!(Self { > spec, > bar: devres_bar, > fw, > sysmem_flush, > + gsp_falcon, > + sec2_falcon, > })) > } > } >