Danilo Krummrich
2025-Jun-18 20:24 UTC
[PATCH v5 23/23] gpu: nova-core: load and run FWSEC-FRTS
On Wed, Jun 18, 2025 at 10:23:15PM +0200, Danilo Krummrich wrote:> On Thu, Jun 12, 2025 at 11:01:51PM +0900, Alexandre Courbot wrote: > > @@ -237,6 +237,67 @@ pub(crate) fn new( > > }, > > )?; > > > > + // Check that the WPR2 region does not already exists - if it does, the GPU needs to be > > + // reset. > > + if regs::NV_PFB_PRI_MMU_WPR2_ADDR_HI::read(bar).hi_val() != 0 { > > + dev_err!( > > + pdev.as_ref(), > > + "WPR2 region already exists - GPU needs to be reset to proceed\n" > > + ); > > + return Err(EBUSY); > > + } > > + > > + // Reset falcon, load FWSEC-FRTS, and run it. > > + gsp_falcon > > + .reset(bar) > > + .inspect_err(|e| dev_err!(pdev.as_ref(), "Failed to reset GSP falcon: {:?}\n", e))?; > > + gsp_falcon > > + .dma_load(bar, &fwsec_frts) > > + .inspect_err(|e| dev_err!(pdev.as_ref(), "Failed to load FWSEC-FRTS: {:?}\n", e))?; > > + let (mbox0, _) = gsp_falcon > > + .boot(bar, Some(0), None) > > + .inspect_err(|e| dev_err!(pdev.as_ref(), "Failed to boot FWSEC-FRTS: {:?}\n", e))?; > > + if mbox0 != 0 { > > + dev_err!(pdev.as_ref(), "FWSEC firmware returned error {}\n", mbox0); > > + return Err(EIO); > > + } > > + > > + // SCRATCH_E contains FWSEC-FRTS' error code, if any. > > + let frts_status = regs::NV_PBUS_SW_SCRATCH_0E::read(bar).frts_err_code(); > > + if frts_status != 0 { > > + dev_err!( > > + pdev.as_ref(), > > + "FWSEC-FRTS returned with error code {:#x}", > > + frts_status > > + ); > > + return Err(EIO); > > + } > > + > > + // Check the WPR2 has been created as we requested. > > + let (wpr2_lo, wpr2_hi) = ( > > + (regs::NV_PFB_PRI_MMU_WPR2_ADDR_LO::read(bar).lo_val() as u64) << 12, > > + (regs::NV_PFB_PRI_MMU_WPR2_ADDR_HI::read(bar).hi_val() as u64) << 12, > > + ); > > + if wpr2_hi == 0 { > > + dev_err!( > > + pdev.as_ref(), > > + "WPR2 region not created after running FWSEC-FRTS\n" > > + ); > > + > > + return Err(EIO); > > + } else if wpr2_lo != fb_layout.frts.start { > > + dev_err!( > > + pdev.as_ref(), > > + "WPR2 region created at unexpected address {:#x}; expected {:#x}\n", > > + wpr2_lo, > > + fb_layout.frts.start, > > + ); > > + return Err(EIO); > > + } > > + > > + dev_dbg!(pdev.as_ref(), "WPR2: {:#x}-{:#x}\n", wpr2_lo, wpr2_hi); > > + dev_dbg!(pdev.as_ref(), "GPU instance built\n"); > > + > > This makes Gpu::new() quite messy, can we move this to a separate function > please?Actually, can't this just be a method of FwsecFirmware?
Alexandre Courbot
2025-Jun-19 12:35 UTC
[PATCH v5 23/23] gpu: nova-core: load and run FWSEC-FRTS
On Thu Jun 19, 2025 at 5:24 AM JST, Danilo Krummrich wrote:> On Wed, Jun 18, 2025 at 10:23:15PM +0200, Danilo Krummrich wrote: >> On Thu, Jun 12, 2025 at 11:01:51PM +0900, Alexandre Courbot wrote: >> > @@ -237,6 +237,67 @@ pub(crate) fn new( >> > }, >> > )?; >> > >> > + // Check that the WPR2 region does not already exists - if it does, the GPU needs to be >> > + // reset. >> > + if regs::NV_PFB_PRI_MMU_WPR2_ADDR_HI::read(bar).hi_val() != 0 { >> > + dev_err!( >> > + pdev.as_ref(), >> > + "WPR2 region already exists - GPU needs to be reset to proceed\n" >> > + ); >> > + return Err(EBUSY); >> > + } >> > + >> > + // Reset falcon, load FWSEC-FRTS, and run it. >> > + gsp_falcon >> > + .reset(bar) >> > + .inspect_err(|e| dev_err!(pdev.as_ref(), "Failed to reset GSP falcon: {:?}\n", e))?; >> > + gsp_falcon >> > + .dma_load(bar, &fwsec_frts) >> > + .inspect_err(|e| dev_err!(pdev.as_ref(), "Failed to load FWSEC-FRTS: {:?}\n", e))?; >> > + let (mbox0, _) = gsp_falcon >> > + .boot(bar, Some(0), None) >> > + .inspect_err(|e| dev_err!(pdev.as_ref(), "Failed to boot FWSEC-FRTS: {:?}\n", e))?; >> > + if mbox0 != 0 { >> > + dev_err!(pdev.as_ref(), "FWSEC firmware returned error {}\n", mbox0); >> > + return Err(EIO); >> > + } >> > + >> > + // SCRATCH_E contains FWSEC-FRTS' error code, if any. >> > + let frts_status = regs::NV_PBUS_SW_SCRATCH_0E::read(bar).frts_err_code(); >> > + if frts_status != 0 { >> > + dev_err!( >> > + pdev.as_ref(), >> > + "FWSEC-FRTS returned with error code {:#x}", >> > + frts_status >> > + ); >> > + return Err(EIO); >> > + } >> > + >> > + // Check the WPR2 has been created as we requested. >> > + let (wpr2_lo, wpr2_hi) = ( >> > + (regs::NV_PFB_PRI_MMU_WPR2_ADDR_LO::read(bar).lo_val() as u64) << 12, >> > + (regs::NV_PFB_PRI_MMU_WPR2_ADDR_HI::read(bar).hi_val() as u64) << 12, >> > + ); >> > + if wpr2_hi == 0 { >> > + dev_err!( >> > + pdev.as_ref(), >> > + "WPR2 region not created after running FWSEC-FRTS\n" >> > + ); >> > + >> > + return Err(EIO); >> > + } else if wpr2_lo != fb_layout.frts.start { >> > + dev_err!( >> > + pdev.as_ref(), >> > + "WPR2 region created at unexpected address {:#x}; expected {:#x}\n", >> > + wpr2_lo, >> > + fb_layout.frts.start, >> > + ); >> > + return Err(EIO); >> > + } >> > + >> > + dev_dbg!(pdev.as_ref(), "WPR2: {:#x}-{:#x}\n", wpr2_lo, wpr2_hi); >> > + dev_dbg!(pdev.as_ref(), "GPU instance built\n"); >> > + >> >> This makes Gpu::new() quite messy, can we move this to a separate function >> please? > > Actually, can't this just be a method of FwsecFirmware?Yes and no. :) FWSEC can run two commands, `Frts` and `Sb`, and some of the code here is specific to `Frts`. The code that is not specific to it (loading the firmware into the falcon, booting and checking MBOX) can be moved into a method of `FwsecFirmware`, and it makes sense to do so actually. All of this code is going to be moved out of `Gpu::new()` eventually (i.e. the follow-up patchset), but we are still figuring out where it will eventually land. We will need some other entity to manage the GSP boot (GspBooter?), and I am still learning which parts are common to all GPU families and which ones should be a HAL. So for now I'd rather keep it here, modulo the part that can be moved into `FwsecFirmware`.