* [PATCH v4 2/2] gpu: nova-core: add support for GPU timer block. (v4)
@ 2026-11-05 3:07 ` Dave Airlie via B4 Relay
0 siblings, 0 replies; 11+ messages in thread
From: Dave Airlie via B4 Relay @ 2026-11-05 3:07 UTC (permalink / raw)
To: Andreas Hindborg, FUJITA Tomonori, Gary Guo, Alice Ryhl,
Danilo Krummrich, Daniel Almeida, Alexandre Courbot
Cc: nova-gpu, dri-devel, Dave Airlie
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>
---
drivers/gpu/nova-core/fsp.rs | 5 +-
drivers/gpu/nova-core/fsp/regs.rs | 30 ++++++++++++
drivers/gpu/nova-core/gpu.rs | 8 ++++
drivers/gpu/nova-core/nova_core.rs | 1 +
drivers/gpu/nova-core/timer.rs | 33 +++++++++++++
drivers/gpu/nova-core/timer/hal.rs | 27 +++++++++++
drivers/gpu/nova-core/timer/hal/gh100.rs | 79 ++++++++++++++++++++++++++++++
drivers/gpu/nova-core/timer/hal/tu102.rs | 53 +++++++++++++++++++++
drivers/gpu/nova-core/timer/regs.rs | 82 ++++++++++++++++++++++++++++++++
9 files changed, 316 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/nova-core/fsp.rs b/drivers/gpu/nova-core/fsp.rs
index b738dcabcdef..222b3dbaca47 100644
--- a/drivers/gpu/nova-core/fsp.rs
+++ b/drivers/gpu/nova-core/fsp.rs
@@ -51,10 +51,11 @@
NvdmType, //
},
num,
- regs, //
+ regs as bootregs, //
};
mod hal;
+pub(crate) mod regs;
/// PRC message sub-command.
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
@@ -429,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),
)
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 {
+ use kernel::io::register;
+
+ use crate::{
+ driver::NovaRegisters,
+ };
+
+ // FSP PRI space; the timer is its only consumer so far.
+ 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 {
+ 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 {
+ /// 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
+ }
+ }
+}
diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
index 4e59072cab50..99b6e33aa243 100644
--- a/drivers/gpu/nova-core/gpu.rs
+++ b/drivers/gpu/nova-core/gpu.rs
@@ -53,6 +53,10 @@
GpuMm,
VramAddress, //
},
+ timer::{
+ Timer,
+ TimerState, //
+ },
vgpu::VgpuManager, //
};
@@ -331,6 +335,8 @@ struct GspResources<'gpu> {
// TODO: use different resource types for each boot method, and make the relevant Gsp methods
// generic against them.
fsp: Option<Fsp<'gpu>>,
+ /// Timer state - contains system timer offset
+ timer: TimerState,
/// vGPU state detected before GSP boot.
vgpu: VgpuManager,
/// GSP runtime data.
@@ -497,6 +503,8 @@ pub(crate) fn new<'a>(
fsp: Fsp::try_new(dev, bar, spec.chipset)?,
+ timer: Timer::init(bar, spec.chipset)?,
+
vgpu: VgpuManager::new(pdev, spec.chipset, fsp.as_mut()),
gsp <- Gsp::new(pdev, bar),
diff --git a/drivers/gpu/nova-core/nova_core.rs b/drivers/gpu/nova-core/nova_core.rs
index a8f6dc22057b..ee59d81e5105 100644
--- a/drivers/gpu/nova-core/nova_core.rs
+++ b/drivers/gpu/nova-core/nova_core.rs
@@ -27,6 +27,7 @@
mod sbuffer;
#[cfg(CONFIG_NOVA_CORE_SELFTESTS)]
mod selftest;
+mod timer;
mod vbios;
mod vgpu;
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.
+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,
+}
+
+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
+ 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.
+ fsp_bug3957833_war: bool,
+}
+
+impl Gh100 {
+ /// kfspRequiresBug3957833WAR_HAL: returns `true` if the FSP ucode predates the fix for
+ /// 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()
+ }
+
+ 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));
+ }
+
+ // 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.
+ 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)
+ }
+}
+
+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;
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));
+ }
+}
+
+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 {
+ 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)?)
+ }
+}
+
+impl NV_PTIMER_TIME_1 {
+ pub(crate) fn try_from_nsec(nsec: u64) -> Result<Self> {
+ Ok(Self::zeroed().try_with_nsec(((nsec >> 32) & 0x1FFF_FFFF) as u32)?)
+ }
+}
+
+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 {
+ 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> {
+ Ok(Self::zeroed().try_with_nsec(((nsec >> 5) & 0x7FF_FFFF) as u32)?)
+ }
+ }
+
+ 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)?)
+ }
+ }
+}
--
2.56.0
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v4 2/2] gpu: nova-core: add support for GPU timer block. (v4)
2026-11-05 3:07 ` Dave Airlie via B4 Relay
(?)
@ 2026-11-05 3:21 ` sashiko-bot
-1 siblings, 0 replies; 11+ messages in thread
From: sashiko-bot @ 2026-11-05 3:21 UTC (permalink / raw)
To: Dave Airlie, Andreas Hindborg, FUJITA Tomonori, Gary Guo,
Alice Ryhl, Danilo Krummrich, Daniel Almeida, Alexandre Courbot
Cc: nova-gpu, dri-devel
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] gpu: nova-core: u64::checked_sub() fails on negative offset in timer_time()
--
commit 8e6e7110b2947fca24843940e52d712c197357df
Author: Dave Airlie <airlied@redhat.com>
gpu: nova-core: add support for GPU timer block. (v4)
This commit adds GPU timer block initialization support for nova-core across
Turing/Ampere/Ada (tu102 HAL) and Hopper/Blackwell (gh100/gb100 HALs) to
compute the system timer offset passed to GSP initialization.
> 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 0000000000000..0486df5889647
> --- /dev/null
> +++ b/drivers/gpu/nova-core/timer/hal/gh100.rs
> @@ -0,0 +1,79 @@
[ ... ]
> + 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));
> + }
> +
> + // 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.
> + let sys_timer_offset_ns = time_ns.checked_sub(sec_timer_ns).ok_or(EINVAL)?;
[Severity: High]
In Gh100::timer_time(), both time_ns and sec_timer_ns are u64 values. When
the difference is negative (time_ns < sec_timer_ns), u64::checked_sub()
returns None, causing Gh100::timer_time() to return Err(EINVAL) and abort
Gpu::new() -> Timer::init() -> Gh100::init() during probe.
Since the comment notes that the difference can be negative and both operands
are less than 2^62, should this use wrapping_sub() (or signed subtraction)
instead of checked_sub()?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261105-nova-timer-v4-0-45a7c121d4de@redhat.com?part=2
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v4 2/2] gpu: nova-core: add support for GPU timer block. (v4)
2026-11-05 3:07 ` Dave Airlie via B4 Relay
(?)
(?)
@ 2026-11-05 14:43 ` Alexandre Courbot
2026-11-06 5:31 ` David Airlie
-1 siblings, 1 reply; 11+ messages in thread
From: Alexandre Courbot @ 2026-11-05 14:43 UTC (permalink / raw)
To: Dave Airlie via B4 Relay
Cc: airlied, Andreas Hindborg, FUJITA Tomonori, Gary Guo, Alice Ryhl,
Danilo Krummrich, Daniel Almeida, nova-gpu, dri-devel
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()))
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v4 2/2] gpu: nova-core: add support for GPU timer block. (v4)
2026-11-05 14:43 ` Alexandre Courbot
@ 2026-11-06 5:31 ` David Airlie
2026-11-06 14:41 ` Alexandre Courbot
0 siblings, 1 reply; 11+ messages in thread
From: David Airlie @ 2026-11-06 5:31 UTC (permalink / raw)
To: Alexandre Courbot
Cc: Dave Airlie via B4 Relay, Andreas Hindborg, FUJITA Tomonori,
Gary Guo, Alice Ryhl, Danilo Krummrich, Daniel Almeida, nova-gpu,
dri-devel
On Fri, Nov 6, 2026 at 12:43 AM Alexandre Courbot <acourbot@nvidia.com> wrote:
>
> 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:
> 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.
I've done nearly everything you requested for v5.
I've just stored Chipset in there, is it worth storing hal? It might
happen later I suppose if needed, but for this I just rederive hal for
the one entrypoint.
> 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.
I used setup, I don't have a good name.
The main thing is I didn't use the NSEC_SHIFT thing, but just write
nsec_hi and lo wrappers that hardcode the values since to me it looks
cleaner than trying to infer that from the register definitions,
especially since I don't see a good way of doing it for nsec_hi. I
remove all falling from these paths and just went with truncation
because that is what actually makes sense.
nsec_lo/hi are now used in the register from_nsec for both tu102 and
gh100 registers.
> 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.
I just fixed things to truncate and used wrapping_sub, to be honest
being careful here is probably the wrong answer, my local review bot
pointed out some wierdness can occur in 2043 with the current scheme,
so let's just keep it simple, wrapping subtract, and feed the result
to the hardware which is what the other driver does and what nouveau
did.
>
> 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?
I didn't do this, I think resume support might need to program this
also, and it didn't look very clean.
Dave.
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v4 2/2] gpu: nova-core: add support for GPU timer block. (v4)
2026-11-06 5:31 ` David Airlie
@ 2026-11-06 14:41 ` Alexandre Courbot
0 siblings, 0 replies; 11+ messages in thread
From: Alexandre Courbot @ 2026-11-06 14:41 UTC (permalink / raw)
To: David Airlie
Cc: Dave Airlie via B4 Relay, Andreas Hindborg, FUJITA Tomonori,
Gary Guo, Alice Ryhl, Danilo Krummrich, Daniel Almeida, nova-gpu,
dri-devel
On Fri Nov 6, 2026 at 2:31 PM JST, David Airlie wrote:
<...>
> I've done nearly everything you requested for v5.
Thanks, took a quick look at it. It's getting late so I'll do a more
in-depth review tomorrow.
>
> I've just stored Chipset in there, is it worth storing hal? It might
> happen later I suppose if needed, but for this I just rederive hal for
> the one entrypoint.
Storing the hal is technically better, because in
`is_bug_3957833_affected` you need to handle the case where `fsp_hal`
returns `None`, even though we have already assessed that the HAL
existed in `Fsp::new`. So it's just a dead branch that can be avoided by
storing a reference to the HAL instead of the Chipset.
If you do that, `FspHal` will need to require `Send + Sync`, but that
won't create any issue.
Now in practice, `is_bug_3957833_affected` is only ever called once, so
it makes little difference. But I expect the FSP HAL will expand
further, so storing it will definitely become the long-term design.
>
>> 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.
>
> I used setup, I don't have a good name.
>
> The main thing is I didn't use the NSEC_SHIFT thing, but just write
> nsec_hi and lo wrappers that hardcode the values since to me it looks
> cleaner than trying to infer that from the register definitions,
> especially since I don't see a good way of doing it for nsec_hi. I
Ah yeah, I saw that you used a helper function that is used by both sets
of timer registers. In this case it is not obvious which one should
provide the constant.
In this context I guess we can go with it because this is only used
locally, and `Bounded` will throw a build error if the number of bits
don't match, but generally speaking I would like to avoid repeating
Nouveau's pattern of producing literal values out of a hat and use the
defined constants whenever possible, as they act as documentation on top
of preventing mistakes.
> remove all falling from these paths and just went with truncation
> because that is what actually makes sense.
Do we understand what happens if we truncate though?
>
> nsec_lo/hi are now used in the register from_nsec for both tu102 and
> gh100 registers.
>
>> 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.
>
> I just fixed things to truncate and used wrapping_sub, to be honest
> being careful here is probably the wrong answer, my local review bot
> pointed out some wierdness can occur in 2043 with the current scheme,
> so let's just keep it simple, wrapping subtract, and feed the result
> to the hardware which is what the other driver does and what nouveau
> did.
The other drivers do what they do, but here again do we understand what
happens when we write a wrapped value into the timer registers?
>
>>
>> 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?
>
> I didn't do this, I think resume support might need to program this
> also, and it didn't look very clean.
No problem, especially if this is expected to be used later.
^ permalink raw reply [flat|nested] 11+ messages in thread