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

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

On Thu Nov 5, 2026 at 12:07 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 as two variants, gh100 and gb100.
> 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 we avoid sending it to registers
> other we write the offset to registers.
> The gb100 path just avoids the war.
> The value is also stored to send to GSP.
>
> Signed-off-by: Dave Airlie <airlied@redhat.com>

There are a couple of Clippy issues with this patch, please make sure
to build with `CLIPPY=1`. `make rustfmt` also reformatted a few lines.

<...>
> diff --git a/drivers/gpu/nova-core/fsp/regs.rs b/drivers/gpu/nova-core/fsp/regs.rs
> new file mode 100644
> index 000000000000..d4b8482a6854
> --- /dev/null
> +++ b/drivers/gpu/nova-core/fsp/regs.rs
> @@ -0,0 +1,30 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +pub(crate) mod gh100 {

FSP only makes sense for GH100 and more recent, so I guess we can do
without this sub-module?

> +    use kernel::io::register;
> +
> +    use crate::{
> +        driver::NovaRegisters,
> +    };
> +
> +    // FSP PRI space; the timer is its only consumer so far.

I guess we don't need to mention who the consumer is here; especially
since the list might increase in the future.

> +    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(crate) NV_GFW_FSP_UCODE_VERSION(u32) @ 0x008f0334 {

We are trying to keep registers (at least those defined within
sub-modules) local to their owning module, so ideally the visibility
should be `pub(super)`. And instead of accessing the register directly
in `timer`, a `Fsp::is_bug3957833_affected()` method would expose the
functionality we need.

> +            11:0 full;
> +        }
> +    }
> +
> +    impl NV_GFW_FSP_UCODE_VERSION {
> +        /// Check if the hopper only workaround is required
> +        pub(crate) fn is_bug3957833_affected(self) -> bool {

So as mentioned above, this method would be moved to a method of `Fsp`
that `timer` would call, keeping registers local to their module.

We will need to store the hal and `Chipset` into the `Fsp` struct for
that, but that's standard practice.

> +            /// FSP ucode version from which on the fix for bug 3957833 is included.
> +            const FSP_BUG_3957833_FIX_VERSION: u32 = 0x44c;
> +
> +            self.full().get() < FSP_BUG_3957833_FIX_VERSION

You can omit the `.get()` here, there are comparison operators
implementations between `Bounded` and regular integer types.

<...>
> diff --git a/drivers/gpu/nova-core/timer.rs b/drivers/gpu/nova-core/timer.rs
> new file mode 100644
> index 000000000000..e7ad37806e5b
> --- /dev/null
> +++ b/drivers/gpu/nova-core/timer.rs
> @@ -0,0 +1,33 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +mod hal;
> +mod regs;
> +
> +use kernel::{
> +    prelude::*, //
> +};
> +
> +use crate::{
> +    driver::Bar0,
> +    gpu::Chipset, //
> +};
> +
> +pub(crate) struct Timer;
> +
> +// struct that just holds the sys timer offset
> +// after initialising the GPU timer block.

Can this be a regular Rust doccomment? If possible describing its
purpose, not its contents.

> +pub(crate) struct TimerState {
> +    /// This value is to be passed to GspInit SYS_TIMER_OFFSET_NS
> +    /// It is considered an opaque value, passed from timer hal to GSP.
> +    #[allow(dead_code)]
> +    sys_timer_offset_ns: u64,

I assume a future patch will need to read this value, so we can
preemptively make it `pub(crate)`.

> +}
> +
> +impl Timer {
> +    /// Initialize the GPU timer.
> +    pub(crate) fn init(bar: Bar0<'_>, chipset: Chipset) -> Result<TimerState> {
> +        Ok(TimerState {
> +            sys_timer_offset_ns: hal::timer_hal(chipset).init(bar)?,
> +        })
> +    }
> +}
> diff --git a/drivers/gpu/nova-core/timer/hal.rs b/drivers/gpu/nova-core/timer/hal.rs
> new file mode 100644
> index 000000000000..b78066bf9a0d
> --- /dev/null
> +++ b/drivers/gpu/nova-core/timer/hal.rs
> @@ -0,0 +1,27 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +use kernel::prelude::*;
> +
> +use crate::{
> +    driver::Bar0,
> +    gpu::{
> +        Architecture,
> +        Chipset, //
> +    },
> +};
> +
> +mod gh100;
> +mod tu102;
> +
> +pub(super) trait TimerHal {
> +    /// return system timer offset ns

nit: capitalization.

Maybe also explain that this can program some timer-related registers,
otherwise the name `init` isn't justified for someone who believes that
this just returns the time without any side-effect.

> +    fn init(&self, bar: Bar0<'_>) -> Result<u64>;
> +}
> +
> +pub(super) fn timer_hal(chipset: Chipset) -> &'static dyn TimerHal {
> +    match chipset.arch() {
> +        Architecture::Turing | Architecture::Ampere | Architecture::Ada => tu102::TU102_HAL,
> +        Architecture::Hopper => gh100::GH100_HAL,
> +        Architecture::BlackwellGB10x | Architecture::BlackwellGB20x => gh100::GB100_HAL,
> +    }
> +}
> 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..0486df588964
> --- /dev/null
> +++ b/drivers/gpu/nova-core/timer/hal/gh100.rs
> @@ -0,0 +1,79 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +use super::TimerHal;
> +
> +use kernel::{
> +    io::Io,
> +    prelude::*,
> +    time::*, //
> +};
> +
> +use crate::{
> +    driver::Bar0,
> +    fsp::regs as fspregs,
> +    timer::regs, //
> +};
> +
> +/// Timer HAL for Hopper and Blackwell.
> +struct Gh100 {
> +    /// `true` on Hopper, FSP ucode on newer GPUs always has the fix.

This comment doesn't parse for me - can we simplify it?

> +    fsp_bug3957833_war: bool,
> +}
> +
> +impl Gh100 {
> +    /// kfspRequiresBug3957833WAR_HAL: returns `true` if the FSP ucode predates the fix for

If we name OpenRM methods, let's state that they come from OpenRM as that
name might be confusing to an unfamiliar reader.

> +    /// bug 3957833 and hence the sys timer offset must not be programmed. Can only be true
> +    /// on GH100 with older FSP.
> +    fn requires_bug3957833_war(&self, bar: Bar0<'_>) -> bool {
> +        self.fsp_bug3957833_war
> +            && bar.read(fspregs::gh100::NV_GFW_FSP_UCODE_VERSION).is_bug3957833_affected()

With the change suggested above we would call something like
`Fsp::is_bug3957833_affected()`.

> +    }
> +
> +    fn timer_time(&self, bar: Bar0<'_>, time_ns: u64) -> Result<u64> {
> +        let mut sec_hi2 = u32::from(bar.read(regs::gh100::NV_PGC6_SCI_SEC_TIMER_TIME_1));
> +        let sec_timer_ns: u64;
> +        loop {
> +            let sec_lo = u32::from(bar.read(regs::gh100::NV_PGC6_SCI_SEC_TIMER_TIME_0));
> +            let sec_hi = u32::from(bar.read(regs::gh100::NV_PGC6_SCI_SEC_TIMER_TIME_1));
> +            if sec_hi == sec_hi2 {
> +                sec_timer_ns = u64::from(sec_hi) << 32 | u64::from(sec_lo);
> +                break;
> +            }
> +            sec_hi2 = u32::from(bar.read(regs::gh100::NV_PGC6_SCI_SEC_TIMER_TIME_1));
> +        }

Looks like this mirrors the `timer_read` function of `tu102`, can we
move it into its own method for symmetry?

Note that you don't need to wrap everything in `u32::from` - register
values can be compared directly. You'll just need to access `.nsec()`
when building `sec_timer_ns`, and shift `sec_lo.nsec()` by `5` (or
better, `NV_PGC6_SCI_SEC_TIMER_TIME_0::NSEC_SHIFT`) to get
the actual value. Usually when we have a value that needs to be shifted
we add a convenience method to the register type directly, that probably
applies here as well.

Then if you bulk-import `regs::gh100::*`, you can also shorten every
line that accesses a register.

> +
> +        // The system and the GPU timer are both free-running with unrelated epochs, so the
> +        // difference can be negative. Both operands are < 2^62, hence this cannot overflow.

This comment doesn't look right, both operands are `u64` and IIUC this
*will* underflow if `sec_timer_ns > time_ns`.

I'd ignore Sashiko's suggestion as it looks like it is trying to
reconcile the code against the comment instead of the other way around.

> +        let sys_timer_offset_ns = time_ns.checked_sub(sec_timer_ns).ok_or(EINVAL)?;
> +
> +        if !self.requires_bug3957833_war(bar) {
> +            bar.write_reg(
> +                regs::gh100::NV_PGC6_SCI_SYS_TIMER_OFFSET_1::try_from_nsec(sys_timer_offset_ns)?
> +            );
> +            bar.write_reg(
> +                regs::gh100::NV_PGC6_SCI_SYS_TIMER_OFFSET_0::try_from_nsec(sys_timer_offset_ns)?
> +                    .with_update(true),
> +            );
> +        }
> +        Ok(sys_timer_offset_ns)

Once we have moved the timer read loop into its own method, I guess the
remainder can be nicely folded into `init` directly, and we don't need
`timer_time` anymore?

> +    }
> +}
> +
> +impl TimerHal for Gh100 {
> +    fn init(&self, bar: Bar0<'_>) -> Result<u64> {
> +        // The open drivers uses wall clock time here not monotonic
> +        let sys_time_ns = Instant::<RealTime>::now().as_nanos();
> +
> +        self.timer_time(bar, sys_time_ns as u64)
> +    }
> +}
> +
> +const GH100: Gh100 = Gh100 {
> +    fsp_bug3957833_war: true,
> +};
> +const GB100: Gh100 = Gh100 {
> +    fsp_bug3957833_war: false,
> +};
> +
> +pub(super) const GH100_HAL: &dyn TimerHal = &GH100;
> +pub(super) const GB100_HAL: &dyn TimerHal = &GB100;

We prefer to keep each HAL in a module bearing the same name, so `GB100`
should be in `gb100.rs`.

But if we move `is_bug3957833_affected` into Fsp, then both the
`fsp_bug3957833_war` flag and `GB100` HAL can go away. `Timer::init`
will just need to be passed an `Option<&Fsp>`, which it passes down to
the HAL method so it can query it (similarly to what `VgpuManager::new`
does - only with `as_ref()` instead of `as_mut()`).

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

Here as well you can remove the `u32::from` easily.

> +
> +fn timer_time(bar: Bar0<'_>, time_ns: u64) -> Result {
> +    if bar.read(regs::NV_PTIMER_TIME_PRIV_LEVEL_MASK).level0() {
> +        bar.write_reg(regs::NV_PTIMER_TIME_1::try_from_nsec(time_ns)?);
> +        bar.write_reg(regs::NV_PTIMER_TIME_0::try_from_nsec(time_ns)?);
> +        Ok(())
> +    } else {
> +        Err(EINVAL)
> +    }
> +}
> +
> +impl TimerHal for Tu102 {
> +    fn init(&self, bar: Bar0<'_>) -> 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.
> +        let sys_time_ns = Instant::<RealTime>::now().as_nanos();
> +
> +        timer_time(bar, sys_time_ns as u64)?;
> +        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..3de7174f20db
> --- /dev/null
> +++ b/drivers/gpu/nova-core/timer/regs.rs
> @@ -0,0 +1,82 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +use kernel::io::register;
> +use kernel::prelude::*;
> +
> +use crate::{
> +    driver::NovaRegisters,
> +};
> +use pin_init::Zeroable;
> +
> +// PTIMER
> +
> +register! {
> +    base: NovaRegisters;
> +
> +    pub(crate) NV_PTIMER_TIME_PRIV_LEVEL_MASK(u32) @ 0x00009430 {

All the definitions of this module can be `pub(super)`

> +        4:4 level0 => bool;
> +    }
> +
> +    pub(crate) NV_PTIMER_TIME_0(u32) @ 0x00009400 {
> +        31:5 nsec;
> +    }
> +
> +    pub(crate) NV_PTIMER_TIME_1(u32) @ 0x00009410 {
> +        28:0 nsec;
> +    }
> +}
> +
> +impl NV_PTIMER_TIME_0 {
> +    pub(crate) fn try_from_nsec(nsec: u64) -> Result<Self> {
> +        Ok(Self::zeroed().try_with_nsec(((nsec >> 5) & 0x7FF_FFFF) as u32)?)
> +    }

Clippy pointed out that you can get rid of the `Ok()` because
`try_with_nsec` already returns the expected type.

There are 2 operations that silently truncate the passed value - the
right shift (which we cannot avoid), and the mask. We should document
these, or maybe take a `u32` to leave that responsibility to the caller
so we only need to mention the truncated 5 LSBs.

With that this method can actually become infallible, because `nsec`
covers the full u32 range. We just need to construct the expected
`Bounded` value, which looks something like this:

    pub(super) fn from_nsec(nsec: u32) -> Self {
        let nsec = Bounded::<u32, 32>::from(nsec).shr::<{ Self::NSEC_SHIFT }, _>();

        Self::zeroed().with_nsec(nsec)
    }

The `shr` method on `Bounded` reduces its number of valid bits, turning
it into exactly the type expected for `nsec`, so we don't need to use
the fallible `try_with_nsec` method. Also note that I have used the
constant shift that is automatically defined for each bitfield instead
of the immediate value `5`.

It's a bit of a mouthful but it is demonstrably correct and doesn't rely
on any truncating operation (notably `as` that we try to avoid whenever
possible).

> +}
> +
> +impl NV_PTIMER_TIME_1 {
> +    pub(crate) fn try_from_nsec(nsec: u64) -> Result<Self> {

Here as well we should document that this works with only the 32 MSBs
(or, again, take a `u32` and leave that to the caller).

> +        Ok(Self::zeroed().try_with_nsec(((nsec >> 32) & 0x1FFF_FFFF) as u32)?)

You can just do

    Self::zeroed().try_with_nsec(nsec >> 32)

`try_with_nsec` is happy to take a u64, and here I expect that we want
to fail if the value is larger than what the registers pair can
represent? Otherwise this method should also be infallible, as the call
to `try_with_nsec` cannot technically fail with the mask you applied
anyway.

> +    }
> +}
> +
> +pub(crate) mod gh100 {
> +    use kernel::io::register;
> +    use kernel::prelude::*;
> +
> +    use crate::driver::NovaRegisters;
> +    use pin_init::Zeroable;
> +
> +    // PGC6/TIMER
> +    register! {
> +        base: NovaRegisters;
> +
> +        pub(crate) NV_PGC6_SCI_SEC_TIMER_TIME_0(u32) @ 0x00118f54 {

Since we are two modules down, instead of `pub(super)` this will need to
be `pub(in crate::timer)`.

> +            31:5 nsec;
> +        }
> +
> +        pub(crate) NV_PGC6_SCI_SEC_TIMER_TIME_1(u32) @ 0x00118f58 {
> +            28:0 nsec;
> +        }
> +
> +        pub(crate) NV_PGC6_SCI_SYS_TIMER_OFFSET_0(u32) @ 0x00118df4 {
> +            0:0 update;
> +            31:5 nsec;
> +        }
> +
> +        pub(crate) NV_PGC6_SCI_SYS_TIMER_OFFSET_1(u32) @ 0x00118df8 {
> +            28:0 nsec;
> +        }
> +
> +    }
> +
> +    impl NV_PGC6_SCI_SYS_TIMER_OFFSET_0 {
> +        pub(crate) fn try_from_nsec(nsec: u64) -> Result<Self> {

As for `NV_PTIMER_TIME_0` this method can be infallible. Actually it's
an exact mirror...

> +            Ok(Self::zeroed().try_with_nsec(((nsec >> 5) & 0x7FF_FFFF) as u32)?)

... so we can leverage its sibling like this:

    Self::zeroed().with_nsec(NV_PTIMER_TIME_0::from_nsec(nsec).nsec())

(this just leverages the NV_PTIMER_TIME_0 method to build a value,
extract the field, and write it as-is.)

> +        }
> +    }
> +
> +    impl NV_PGC6_SCI_SYS_TIMER_OFFSET_1 {
> +        pub(crate) fn try_from_nsec(nsec: u64) -> Result<Self> {
> +            Ok(Self::zeroed().try_with_nsec(((nsec >> 32) & 0x1FFF_FFFF) as u32)?)

and here as well:

    NV_PTIMER_TIME_1::try_from_nsec(nsec).map(|v| Self::zeroed().with_nsec(v.nsec()))