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

[Alexandre Courbot]

On Tue Jul 28, 2026 at 7:24 PM JST, Gary Guo wrote:
> On Tue Jul 28, 2026 at 7:45 AM BST, Alexandre Courbot wrote:
>> 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@xxxxxxxxxxx>
>>> ---
>>>  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(-)
>>>
>> 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
>
> Right, this is the direction that I do think we should head to. As described in
> commit message, this commit is intended as a mechanical conversion and refactors
> are left to the future.

Great, keeping this commit purely mechanical sounds good to me if you
prefer it that way (although one can argue that the refactor itself is
also mostly mechanical as the end result shouldn't be more complex).

>
>>
>> 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.
>
> This was definitely one option that I have considered, however as `NEW_SIZE`
> still needs to be generic so we're not entirely turbofish-free. But if making
> `offset` become an effective const parameter helps nova impl, it might make
> sense to use `build_assert!` for this.

Maybe we can have another variant of `subregion` that operates that way,
but your call. I don't see any obvious flaw with the current
implementation, it's just that the `FalconEngine` methods should not be
useful outside of the constructor eventually, so it seems a bit
superfluous to have them visible to the whole crate.