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, > })) > } > } >