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 <acour...@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 <ly...@redhat.com>
Thanks!
