Re: [PATCH v2 12/16] gpu: nova-core: use projection for PFALCON and PFALCON2 registers

On Wed Aug 12, 2026 at 3:48 PM BST, Alexandre Courbot wrote:
> On Thu Aug 6, 2026 at 1:35 AM JST, Gary Guo wrote:
>> -/// 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]);
>>  
>> -/// 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>;
>
> Remember on v1 when we contemplated using associated consts? Turns out
> we can with this version:
>
>     // Need a better name, but you get the idea.
>     pub(crate) type PFalconRegs = OffsetLoc<NovaRegisters, PFalconRegisters>;
>     pub(crate) type PFalcon2Regs = OffsetLoc<NovaRegisters, PFalcon2Registers>;
>
>     pub(crate) trait FalconEngine: Send + Sync + Sized {
>         const PFALCON: PFalconRegs;
>         const PFALCON2: PFalcon2Regs;
>     }
>
> ... and make `Falcon::new` call `io_project` directly, and it works! At
> the cost of importing `OffsetLoc` in `falcon.rs`, but that removes ~30
> LoCs in total, and I'm not sure `OffsetLoc` should be hidden anyway.

Originally I want the type of consts created by `register!()` to be considered
as implementation detail and thus ideally not to be named by the user. To user I
want them to treat it as just `impl IoLoc<>`.

However, thinking about this again, I think we might be able to have a single
type for all registers after all the cleanups, so it'd make sense to make this
public API. However, I do want a better name than `OffsetLoc` if we're going to
make this use case supported, though.

Best,
Gary

>
>>  }
>>  
>>  /// Represents a portion of the firmware to be loaded into a particular memory (e.g. IMEM or DMEM)
>> @@ -358,6 +361,8 @@ pub(crate) struct Falcon<'a, E: FalconEngine> {
>>      hal: KBox<dyn FalconHal<E>>,
>>      dev: &'a device::Device<device::Bound>,
>>      bar: Bar0<'a>,
>> +    pub(crate) pfalcon: Mmio<'a, PFalconRegisters>,
>> +    pfalcon2: Mmio<'a, PFalcon2Registers>,
>>  }
>>