Re: [PATCH v4 20/20] gpu: nova-core: load and run FWSEC-FRTS

["Alexandre Courbot" <acourbot@nvidia.com>]

On Wed Jun 4, 2025 at 6:45 AM JST, Lyude Paul wrote:
> On Wed, 2025-05-21 at 15:45 +0900, Alexandre Courbot wrote:
>> With all the required pieces in place, load FWSEC-FRTS onto the GSP
>> falcon, run it, and check that it successfully carved out the WPR2
>> region out of framebuffer memory.
>> 
>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
>> ---
>>  drivers/gpu/nova-core/falcon.rs |  3 ---
>>  drivers/gpu/nova-core/gpu.rs    | 57 ++++++++++++++++++++++++++++++++++++++++-
>>  drivers/gpu/nova-core/regs.rs   | 15 +++++++++++
>>  3 files changed, 71 insertions(+), 4 deletions(-)
>> 
>> diff --git a/drivers/gpu/nova-core/falcon.rs b/drivers/gpu/nova-core/falcon.rs
>> index f224ca881b72954d17fee87278ecc7a0ffac5322..91f0451a04e7b4d0631fbcf9b1e76e59d5dfb7e8 100644
>> --- a/drivers/gpu/nova-core/falcon.rs
>> +++ b/drivers/gpu/nova-core/falcon.rs
>> @@ -2,9 +2,6 @@
>>  
>>  //! Falcon microprocessor base support
>>  
>> -// To be removed when all code is used.
>> -#![expect(dead_code)]
>> -
>>  use core::ops::Deref;
>>  use core::time::Duration;
>>  use hal::FalconHal;
>> diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
>> index 5a4c23a7a6c22abc1f6e72a307fa3336d731a396..280929203189fba6ad8e37709927597bb9c7d545 100644
>> --- a/drivers/gpu/nova-core/gpu.rs
>> +++ b/drivers/gpu/nova-core/gpu.rs
>> @@ -246,7 +246,7 @@ pub(crate) fn new(
>>  
>>          let bios = Vbios::new(pdev, bar)?;
>>  
>> -        let _fwsec_frts = FwsecFirmware::new(
>> +        let fwsec_frts = FwsecFirmware::new(
>>              &gsp_falcon,
>>              pdev.as_ref(),
>>              bar,
>> @@ -257,6 +257,61 @@ 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)?;
>> +        gsp_falcon.dma_load(bar, &fwsec_frts)?;
>> +        let (mbox0, _) = gsp_falcon.boot(bar, Some(0), None)?;
>> +        if mbox0 != 0 {
>> +            dev_err!(pdev.as_ref(), "FWSEC firmware returned error {}\n", mbox0);
>> +            return Err(EINVAL);
>> +        }
>> +
>> +        // 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(EINVAL);
>> +        }
>> +
>> +        // 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(ENOTTY);
>
> ENOTTY? Is this correct?

Probably not - I guess `EIO` would be better to express a firmware
failure? (and for the errors around this one as well).

>
>> +        } else if wpr2_lo != fb_layout.frts.start {
>> +            dev_err!(
>> +                pdev.as_ref(),
>> +                "WPR2 region created at unexpected address {:#x} ; expected {:#x}\n",
>
> Extra space (but if that's intentional, feel free to leave it)

Oops, French typography habits. ;)

>
> Besides those two nits: Reviewed-by: Lyude Paul <lyude@redhat.com>

Thanks!