Re: [PATCH 08/10] gpu: nova-core: use projection for PFALCON and PFALCON2 registers

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

On Wed Jul 22, 2026 at 1:54 AM JST, Gary Guo wrote:
> Add fixed size region types for these and add projection methods that
> project from `Bar0` into these. Update these registers to be registers on
> `PFalconRegisters` and `PFalcon2Registers` and not relative registers on
> `NovaRegisters`.
>
> The use sites are updated mechanically; calls to the projection methods are
> not extracted in this commit.
>
> Signed-off-by: Gary Guo <gary@garyguo.net>
> ---
>  drivers/gpu/nova-core/falcon.rs                    | 158 +++++++++------------
>  drivers/gpu/nova-core/falcon/fsp.rs                |  53 +++----
>  drivers/gpu/nova-core/falcon/gsp.rs                |  43 +++---
>  drivers/gpu/nova-core/falcon/hal/ga102.rs          |  39 ++---
>  drivers/gpu/nova-core/falcon/hal/tu102.rs          |   8 +-
>  drivers/gpu/nova-core/falcon/sec2.rs               |  32 +++--
>  drivers/gpu/nova-core/firmware/fwsec/bootloader.rs |  11 +-
>  drivers/gpu/nova-core/regs.rs                      |  81 ++++++-----
>  8 files changed, 193 insertions(+), 232 deletions(-)
>
> diff --git a/drivers/gpu/nova-core/falcon.rs b/drivers/gpu/nova-core/falcon.rs
> index 78948cc8bff3..3cd065019a66 100644
> --- a/drivers/gpu/nova-core/falcon.rs
> +++ b/drivers/gpu/nova-core/falcon.rs
> @@ -14,13 +14,12 @@
>      },
>      io::{
>          poll::read_poll_timeout,
> -        register::{
> -            RegisterBase,
> -            WithBase, //
> -        },
> +        register::Array,
>          Io,
> +        Mmio, //
>      },
>      prelude::*,
> +    sizes::SZ_4K,
>      time::Delta,
>  };
>  
> @@ -165,18 +164,22 @@ pub(crate) enum FalconFbifMemType with From<Bounded<u32, 1>> {
>      }
>  }
>  
> -/// Type used to represent the `PFALCON` registers address base for a given falcon engine.
> -pub(crate) struct PFalconBase(());
> +/// Type used to represent the `PFALCON` registers.
> +#[repr(align(4))]
> +#[derive(FromBytes, IntoBytes)]
> +pub(crate) struct PFalconRegisters([u8; SZ_4K]);

nit: `SZ_4K` looks correct for both PFALCON and PFALCON2, but the size
should be defined at a const of its own.

>  
> -/// Type used to represent the `PFALCON2` registers address base for a given falcon engine.
> -pub(crate) struct PFalcon2Base(());
> +/// Type used to represent the `PFALCON2` registers.
> +#[repr(align(4))]
> +#[derive(FromBytes, IntoBytes)]
> +pub(crate) struct PFalcon2Registers([u8; SZ_4K]);
>  
>  /// Trait defining the parameters of a given Falcon engine.
>  ///
>  /// Each engine provides one base for `PFALCON` and `PFALCON2` registers.
> -pub(crate) trait FalconEngine:
> -    Send + Sync + RegisterBase<PFalconBase> + RegisterBase<PFalcon2Base> + Sized
> -{
> +pub(crate) trait FalconEngine: Send + Sync + Sized {
> +    fn pfalcon(io: Bar0<'_>) -> Mmio<'_, PFalconRegisters>;
> +    fn pfalcon2(io: Bar0<'_>) -> Mmio<'_, PFalcon2Registers>;

We are calling these methods quite a bit throughout the code. I
understand they are supposed to be optimized away, but that's still a
lot of repetition.

Since we now have HRTB and `Falcon` is already referencing the `Bar0`,
how about storing the projected `Mmio` inside the `Falcon` instance?

    pub(crate) struct Falcon<'a, E: FalconEngine> {
        ...
        pfalcon: Mmio<'a, PFalconRegisters>,
        pfalcon2: Mmio<'a, PFalcon2Registers>,
    }

    pub(crate) fn new(...) -> Result<Self> {
        Ok(Self {
            ...
            pfalcon: E::pfalcon(bar),
            pfalcon2: E::pfalcon2(bar),
        })
    }

That way all the

    E::pfalcon(self.bar)...

Can become simply

    self.pfalcon

And I suspect that once this is generalized, `Falcon` won't even need to
store a reference to the `Bar0` (and potentially poke the I/O of other
engines) anymore.

Actually I would like to push that even further and replace the
`pfalcon()` and `pfalcon2()` trait methods by associated constants used
to construct the projected view in `Falcon::new`, since the projections
are all constructed the same way, but doing so requires
`generic_const_exprs`. :/

We could make it work by moving the `OFFSET` generic argument of
`subregion` into a regular argument and enforcing its invariants using
`build_assert!`, but that would require `subregion` to be
`#[inline(always)]`. I don't know if there is another trick we can use,
if not otherwise I guess the trait methods are ok, especially if they
are only called once in the constructor.