Re: [PATCH v5 2/2] gpu: nova-core: add support for GPU timer block. (v5)

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

On Mon Nov 9, 2026 at 10:52 AM JST, David Airlie wrote:
> On Sun, Nov 8, 2026 at 11:52 PM Alexandre Courbot <acourbot@nvidia.com> wrote:
>>
>> On Fri Nov 6, 2026 at 2:34 PM JST, Dave Airlie via B4 Relay wrote:
>> > From: Dave Airlie <airlied@redhat.com>
>> >
>> > This is needed to calculate the system timer offset that is passed
>> > to GSP init.
>> >
>> > 2 HALs exist:
>> >
>> > a) tu102 is the gv100 path from open-gpu, the priv level is checked
>> > and the cpu time is written to the gpu timer. no offset is used.
>> >
>> > b) gh100 is for hopper+
>> > This is the gh100 path from open-gpu, where the secure system
>> > timer is read, the offset is calculated.
>> > On gh100 which need a work around to avoid sending it to registers
>>
>> nit: "On gh100 a work around is needed to avoid..." maybe?
>>
>> > if the FSP firmware is too old.
>> > Otherwise we write the offset to the offset registers.
>> >
>> > Signed-off-by: Dave Airlie <airlied@redhat.com>
>> > ---
>> >  drivers/gpu/nova-core/fsp.rs             | 19 ++++++-
>> >  drivers/gpu/nova-core/fsp/hal.rs         |  5 ++
>> >  drivers/gpu/nova-core/fsp/hal/gh100.rs   |  6 ++
>> >  drivers/gpu/nova-core/fsp/regs.rs        | 28 +++++++++
>> >  drivers/gpu/nova-core/gpu.rs             |  8 +++
>> >  drivers/gpu/nova-core/nova_core.rs       |  1 +
>> >  drivers/gpu/nova-core/timer.rs           | 38 +++++++++++++
>> >  drivers/gpu/nova-core/timer/hal.rs       | 31 ++++++++++
>> >  drivers/gpu/nova-core/timer/hal/gh100.rs | 62 ++++++++++++++++++++
>> >  drivers/gpu/nova-core/timer/hal/tu102.rs | 53 +++++++++++++++++
>> >  drivers/gpu/nova-core/timer/regs.rs      | 98 ++++++++++++++++++++++++++++++++
>> >  11 files changed, 346 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/drivers/gpu/nova-core/fsp.rs b/drivers/gpu/nova-core/fsp.rs
>> > index 3a4a10487f33..a2ea50465a55 100644
>> > --- a/drivers/gpu/nova-core/fsp.rs
>> > +++ b/drivers/gpu/nova-core/fsp.rs
>> > @@ -50,10 +50,11 @@
>> >          NvdmHeader,
>> >          NvdmType, //
>> >      },
>> > -    regs, //
>> > +    regs as bootregs, //
>> >  };
>> >
>> >  mod hal;
>> > +mod regs;
>> >
>> >  /// PRC message sub-command.
>> >  #[derive(Debug, Clone, Copy, PartialEq, Eq)]
>> > @@ -389,6 +390,7 @@ pub(crate) fn boot_params(&self) -> &Coherent<'_, GspFmcBootParams> {
>> >  pub(crate) struct Fsp<'a> {
>> >      falcon: Falcon<'a, FspEngine>,
>> >      fmc_fw: GspFmcFirmware<'a>,
>> > +    chipset: Chipset,
>> >  }
>> >
>> >  impl<'a> Fsp<'a> {
>> > @@ -428,7 +430,7 @@ fn wait_secure_boot(
>> >
>> >          read_poll_timeout(
>> >              || Ok(hal.fsp_boot_status(bar)),
>> > -            |&status| status == regs::NV_THERM_I2CS_SCRATCH_FSP_BOOT_COMPLETE_STATUS_SUCCESS,
>> > +            |&status| status == bootregs::NV_THERM_I2CS_SCRATCH_FSP_BOOT_COMPLETE_STATUS_SUCCESS,
>> >              Delta::from_millis(10),
>> >              Delta::from_millis(FSP_SECURE_BOOT_TIMEOUT_MS),
>> >          )
>> > @@ -436,7 +438,11 @@ fn wait_secure_boot(
>> >              dev_err!(dev, "FSP secure boot completion error: {:?}\n", e);
>> >          })?;
>> >
>> > -        Ok(Fsp { falcon, fmc_fw })
>> > +        Ok(Fsp {
>> > +            falcon,
>> > +            fmc_fw,
>> > +            chipset,
>> > +        })
>> >      }
>> >
>> >      /// Sends a message to FSP and waits for the response.
>> > @@ -549,4 +555,11 @@ pub(crate) fn boot_fmc(
>> >          dev_dbg!(dev, "FSP Chain of Trust completed successfully\n");
>> >          Ok(())
>> >      }
>> > +
>> > +    pub(crate) fn is_bug_3957833_affected(&self, bar: Bar0<'_>) -> bool {
>> > +        match hal::fsp_hal(self.chipset) {
>> > +            Some(hal) => hal.is_bug_3957833_affected(bar),
>> > +            None => false,
>>
>> As I mentioned in my other comment, storing the HAL instead of the
>> chipset is technically better, but I'm ok if this is not done just for
>> this single-use method.
>
> Okay I've done it for v6, there are places we don't do this already,
> but I'll do it for fsp.
>
>> > +fn sys_timer_offset(bar: Bar0<'_>, fsp: &Fsp<'_>, time_ns: u64) -> Result<u64> {
>> > +    let sec_timer_ns = sec_timer_read(bar);
>> > +
>> > +    let sys_timer_offset_ns = time_ns.wrapping_sub(sec_timer_ns);
>> > +
>> > +    if !fsp.is_bug_3957833_affected(bar) {
>> > +        bar.write_reg(regs::gh100::NV_PGC6_SCI_SYS_TIMER_OFFSET_1::from_nsec(
>> > +            sys_timer_offset_ns,
>> > +        ));
>> > +        bar.write_reg(
>> > +            regs::gh100::NV_PGC6_SCI_SYS_TIMER_OFFSET_0::from_nsec(sys_timer_offset_ns)
>> > +                .with_update(true),
>> > +        );
>> > +    }
>> > +    Ok(sys_timer_offset_ns)
>> > +}
>>
>> So I know we discussed that on v4, but this function is named like it
>> just returns a value, whereas it also might program some registers. Can
>> we mirror what `tu102` does and extract the `if fsp.is_bug...` block
>> into another function that `setup` calls?
>
> No, this gives the workaround too much recognition. I've renamed this
> function to set_sys_timer_offset,
> That is the point of this function, to calculate and set the system
> timer offset in hardware. Broken hardware
> should not direct the function's focus, working hardware calculates
> and programs the registers.
>
>> Regarding the truncated MSBs in `nsec_hi`, I *think* it is ok to
>> truncate them indeed, as the value written would still be a valid anchor
>> value for the GPU. It also shouldn't manifest itself until 2043 or so
>> for chips < GH100, unless someone decides to mess with their system
>> clock.
>>
>> I guess this issue is the reason why GH100+ are storing a delta instead
>> of an absolute value.
>
> For v6, I've done everything else asked,
>
> For how the timer hw works, I unfortunately can't know this, because
> it isn't documented in public docs, if you guys want to pursue this
> internally, I can only do what OpenRM does.
>
> Also are we okay using OpenRM? like I know there was some issues in
> the past, maybe we need to have a proper way to reference the NVIDIA
> open-gpu-kernel-drivers.

I do recognize I have sometimes ambivalent feelings on the topic of
whether we should follow OpenRM or not. :)

When in doubt as we currently are, I think the case for following OpenRM
is that doing so at least guarantees we won't do worse than it does, and
it has the benefit of being battle-tested.

I've tried to find out more about how the timer registers work. It
appears that the GH100 SEC_TIMER starts at 0 from power-on, so unless
people mess with their system clock it should always be smaller than it,
hence subtracting it should not wrap.

Now if it *does* wrap for some unfortunate reason, we would still get a
valid timer value by adding `SEC_TIMER + OFFSET` modulo 61 bits, which
supports the case for wrapping - it just needs to be documented as
otherwise it can be pretty confusing.

Then there is the year 2043 issue because the timer values are 61 bits.
It affects both the TU102 and GH100 HALs. I guess software relying on
timestamps will have to adjust for that, but from the perspective of
programming the registers I don't think we can do much better than
masking.