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

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

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.

> +        }
> +    }
>  }
> diff --git a/drivers/gpu/nova-core/fsp/hal.rs b/drivers/gpu/nova-core/fsp/hal.rs
> index eaf5837ac5a8..8bcc8f71cf8f 100644
> --- a/drivers/gpu/nova-core/fsp/hal.rs
> +++ b/drivers/gpu/nova-core/fsp/hal.rs
> @@ -23,6 +23,11 @@ pub(super) trait FspHal {
>      // TODO: consider moving this into the TLV firmware metadata when ready
>      /// Returns the size reserved at the end of the framebuffer, in bytes.
>      fn fb_end_reserved_size(&self) -> u64;
> +
> +    /// Return if the bug 3957833 is affected
> +    fn is_bug_3957833_affected(&self, _bar: Bar0<'_>) -> bool {
> +        false
> +    }
>  }
>  
>  /// Returns the FSP HAL, or `None` if the architecture doesn't support FSP.
> diff --git a/drivers/gpu/nova-core/fsp/hal/gh100.rs b/drivers/gpu/nova-core/fsp/hal/gh100.rs
> index 9a8563799da8..538870566b8c 100644
> --- a/drivers/gpu/nova-core/fsp/hal/gh100.rs
> +++ b/drivers/gpu/nova-core/fsp/hal/gh100.rs
> @@ -9,6 +9,7 @@
>  use crate::{
>      driver::Bar0,
>      fsp::hal::FspHal,
> +    fsp::regs as fspregs,

Since this is the fsp-local registers module, let's just import
`fsp::regs::*` and use the registers without any module prefix.

>      regs, //
>  };
>  
> @@ -33,6 +34,11 @@ fn cot_version(&self) -> u16 {
>      fn fb_end_reserved_size(&self) -> u64 {
>          u64::SZ_2M
>      }
> +
> +    fn is_bug_3957833_affected(&self, bar: Bar0<'_>) -> bool {
> +        bar.read(fspregs::NV_GFW_FSP_UCODE_VERSION)
> +            .is_bug3957833_affected()

s/is_bug39/is_bug_39

> +    }
>  }
>  
>  const GH100: Gh100 = Gh100;
> diff --git a/drivers/gpu/nova-core/fsp/regs.rs b/drivers/gpu/nova-core/fsp/regs.rs
> new file mode 100644
> index 000000000000..de69d5bed427
> --- /dev/null
> +++ b/drivers/gpu/nova-core/fsp/regs.rs
> @@ -0,0 +1,28 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +use kernel::io::register;
> +
> +use crate::driver::NovaRegisters;
> +
> +register! {
> +    base: NovaRegisters;
> +
> +    /// FSP ucode version, encoded as `major.minor` in bits 11:8 and 7:0, written by the FSP
> +    /// during boot. (`NV_PFSP_FALCON_COMMON_SCRATCH_GROUP_3(1)` in the hardware headers.)
> +    pub(super) NV_GFW_FSP_UCODE_VERSION(u32) @ 0x008f0334 {
> +        11:0 full;
> +    }

I'd like our register definitions to reflect the aliasing of the comment
(which is also represented by OpenRM). So we would start by defining the
actual `NV_PFSP_FALCON_COMMON_SCRATCH_GROUP_3` (which is an array of 4
registers per OpenRM):

    pub(super) NV_PFSP_FALCON_COMMON_SCRATCH_GROUP_3(u32)[4] @ 0x008f0330 {
        31:0 val;
    }

And then define `NV_GFW_FSP_UCODE_VERSION` as an alias to the register
at index 1.

    /// FSP ucode version, written by the FSP during boot.
    pub(super) NV_GFW_FSP_UCODE_VERSION(u32) => NV_PFSP_FALCON_COMMON_SCRATCH_GROUP_3[1] {
        11:0 full;
    }

That way we can reduce the comment to the information that is not
obvious from the declaration. I'd also drop the references to the
non-defined fields, unless you want to add them - it is possible to
define overlapping fields, but since the code doesn't use them we can do
without for now.

<...>
> diff --git a/drivers/gpu/nova-core/timer/hal/gh100.rs b/drivers/gpu/nova-core/timer/hal/gh100.rs
> new file mode 100644
> index 000000000000..49e348c89e52
> --- /dev/null
> +++ b/drivers/gpu/nova-core/timer/hal/gh100.rs
> @@ -0,0 +1,62 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +use super::TimerHal;
> +
> +use kernel::{
> +    io::Io,
> +    prelude::*,
> +    time::*, //
> +};
> +
> +use crate::{
> +    driver::Bar0,
> +    fsp::Fsp,
> +    timer::regs, //

Here as well, let's bring the whole `timer::regs::gh100::*` since we are
in the GH100 HAL, this will make the code more concise and easier to read.

> +};
> +
> +/// Timer HAL for Hopper and Blackwell.
> +struct Gh100;
> +
> +fn sec_timer_read(bar: Bar0<'_>) -> u64 {
> +    let mut hi2 = bar.read(regs::gh100::NV_PGC6_SCI_SEC_TIMER_TIME_1);
> +    loop {
> +        let lo = bar.read(regs::gh100::NV_PGC6_SCI_SEC_TIMER_TIME_0);
> +        let hi = bar.read(regs::gh100::NV_PGC6_SCI_SEC_TIMER_TIME_1);
> +        if hi == hi2 {
> +            return u64::from(u32::from(hi)) << 32 | u64::from(u32::from(lo));
> +        }
> +        hi2 = bar.read(regs::gh100::NV_PGC6_SCI_SEC_TIMER_TIME_1);
> +    }
> +}
> +
> +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?

Your argument was that resume might need to do the same thing, but
it seems to me that resume would just call `setup` again anyway.

> +
> +impl TimerHal for Gh100 {
> +    fn setup(&self, bar: Bar0<'_>, fsp: Option<&Fsp<'_>>) -> Result<u64> {
> +        // The open drivers uses wall clock time here not monotonic

This module calls it "open drivers", tu102 says "The RM"... Let's call
it "OpenRM" in both. :)

> +        let sys_time_ns = Instant::<RealTime>::now().as_nanos();
> +
> +        // fsp is required on gh100 - return error if not available.
> +        let fsp = fsp.ok_or(EINVAL)?;
> +
> +        sys_timer_offset(bar, fsp, sys_time_ns as u64)

`as` statements require a `CAST:` comment that explains why they are safe.

In this particular case I am not sure what we are supposed to expect, as
an `Instant<RealTime>` is not supposed to be negative anyway. So I
suppose that could be the `CAST:` comment.

> +    }
> +}
> +
> +const GH100: Gh100 = Gh100;
> +pub(super) const GH100_HAL: &dyn TimerHal = &GH100;
> diff --git a/drivers/gpu/nova-core/timer/hal/tu102.rs b/drivers/gpu/nova-core/timer/hal/tu102.rs
> new file mode 100644
> index 000000000000..629d744a4b12
> --- /dev/null
> +++ b/drivers/gpu/nova-core/timer/hal/tu102.rs
> @@ -0,0 +1,53 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +use super::TimerHal;
> +
> +use kernel::{
> +    io::Io,
> +    prelude::*,
> +    time::*, //
> +};
> +
> +use crate::{
> +    driver::Bar0,
> +    fsp::Fsp,
> +    timer::regs, //
> +};
> +struct Tu102;

For symmetry, let's grant this a comment like its gh100 counterpart.

> +
> +// this code will be used in the gb10b hal eventually
> +#[expect(dead_code)]
> +fn timer_read(bar: Bar0<'_>) -> u64 {
> +    let mut hi2 = bar.read(regs::NV_PTIMER_TIME_1);
> +    loop {
> +        let lo = bar.read(regs::NV_PTIMER_TIME_0);
> +        let hi = bar.read(regs::NV_PTIMER_TIME_1);
> +        if hi == hi2 {
> +            return u64::from(u32::from(hi)) << 32 | u64::from(u32::from(lo));
> +        }
> +        hi2 = bar.read(regs::NV_PTIMER_TIME_1);
> +    }
> +}

Usually we try to not introduce dead code proactively, especially if the
user is not in sight. Can we introduce that alongside the gb10b code
that makes use of it?

> +
> +fn set_time(bar: Bar0<'_>, time_ns: u64) -> Result {
> +    if !bar.read(regs::NV_PTIMER_TIME_PRIV_LEVEL_MASK).level0() {
> +        return Err(EINVAL);
> +    }
> +    bar.write_reg(regs::NV_PTIMER_TIME_1::from_nsec(time_ns));
> +    bar.write_reg(regs::NV_PTIMER_TIME_0::from_nsec(time_ns));
> +    Ok(())
> +}
> +
> +impl TimerHal for Tu102 {
> +    fn setup(&self, bar: Bar0<'_>, _fsp: Option<&Fsp<'_>>) -> Result<u64> {
> +        // The RM uses wall clock time here (osGetSystemTime -> ktime_get_real), so the GPU time
> +        // matches the host's real time, e.g. for timestamp correlation in userspace.

nit: s/The RM/OpenRM.

> +        let sys_time_ns = Instant::<RealTime>::now().as_nanos();
> +
> +        set_time(bar, sys_time_ns as u64)?;

Same here, this needs a `CAST:` comment.

> +        Ok(0_u64)
> +    }
> +}
> +
> +const TU102: Tu102 = Tu102;
> +pub(super) const TU102_HAL: &dyn TimerHal = &TU102;
> diff --git a/drivers/gpu/nova-core/timer/regs.rs b/drivers/gpu/nova-core/timer/regs.rs
> new file mode 100644
> index 000000000000..89dd478a37be
> --- /dev/null
> +++ b/drivers/gpu/nova-core/timer/regs.rs
> @@ -0,0 +1,98 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +use kernel::io::register;
> +use kernel::num::Bounded;
> +
> +use crate::driver::NovaRegisters;
> +use pin_init::Zeroable;
> +
> +/// Field value for the NSEC field (31:5) of the `_0` registers: `nsec[31:5]`.
> +///
> +/// The GPU timer counts nanoseconds with 32 ns granularity, so `nsec[4:0]` is dropped.
> +fn nsec_lo(nsec: u64) -> Bounded<u32, 27> {
> +    Bounded::<u32, 32>::from(nsec as u32).shr::<5, _>()
> +}
> +
> +/// Field value for the NSEC field (28:0) of the `_1` registers: `nsec[60:32]`.
> +///
> +/// The GPU timer is 61 bits wide, so `nsec[63:61]` is not representable and is
> +/// truncated — the same bits the hardware discards on a raw register write.

Non-ascii character (em dash) in the comment.

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.