From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.remote-ip=100.103.45.18 ARC-Seal: i=1; d=subspace.kernel.org; s=arc20261015; a=rsa-sha256; cv=none; t=1793749947; b=dI77SoSUmTWXiW9sLZzv7dRwTSXSKcqjHBUcK3w0SeWN7oRkF3kaMRkDruYitw8l5k1T WkUyS5C9fYX+hdaM57JICMMQH9KWGqBf6e4eczp7poZcUQpag+Sg91GHRGGX2meDGX4uv GI4uEhQlzCObVyif4xKRemmeoW4o7x6w8bfPuXtCawi9E0w8mH2H5nUoPoBEG9tw+1ZWD I5/7FZ37Mq3ZzleCYszAuFqUK8vybX3dLjXQIa19DGhfKngSUmvqgarTBYZPz3FEvtfwY KODWZm61eJL6L7VQChywg3P9zt91CGHzcDieH1M54e4794i6R3OzoEfwLF0ZvD8DC9Q== ARC-Message-Signature: i=1; d=subspace.kernel.org; s=arc20261015; a=rsa-sha256; c=relaxed/relaxed; t=1793749947; h=Received:DKIM-Signature:Mime-Version:Content-Transfer-Encoding: Content-Type:Date:Message-Id:Subject:Cc:To:From:References: In-Reply-To; bh=OnPyf4DsfdGQnmVoQBAPDlL7/w93YlPd+SQ2gkHHEGc=; b=TBYixEv+IxVjYjj5zQFkM744nUt1+CANHmzbvnvXyDWli9M+G+MYpoPvHkv12Yf65f9c 3jP+9DM1nDZTTkRmaAkF0L1yORvm80MPvKk8IW54ojY20Ue4+W13Iy/H202EF7rHXoBig +iTKJLV4Gt2Q0amzNF6Ves2qxbwgSfSS8iuIqL6btvy5Z/f+4oCapiZsBDjQ110zWxeTL xwwE89BXyzuBW904Yv7WLKrYJezmZ0ydBzirNn1je3T7oVQiFiPmzRWo9/Q9RDrnlv9Rz ApTXDBwbkegH1M+7EK7xmi1DvIIopoNjbfl27ffu/egOmglhtI/SB9zIg+o7uIgXX1g== ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dkim=pass header.d=kernel.org header.i=@kernel.org header.a=rsa-sha256 header.s=k20260515 header.b=OcdIlHEJ; arc=none smtp.remote-ip=100.103.45.18 Received: from smtp.kernel.org (quasi.space.kernel.org [100.103.45.18]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (prime256v1)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 1183B606A32F for ; Tue, 3 Nov 2026 23:52:26 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key, unprotected) header.d=kernel.org header.i=@kernel.org header.a=rsa-sha256 header.s=k20260515 header.b=OcdIlHEJ Received: by smtp.kernel.org (Postfix) with ESMTPSA id 90E6D1F000C1; Tue, 3 Nov 2026 23:52:24 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1793749946; bh=OnPyf4DsfdGQnmVoQBAPDlL7/w93YlPd+SQ2gkHHEGc=; h=Date:Subject:Cc:To:From:References:In-Reply-To; b=OcdIlHEJPkEBA0v1RhFTf+JxevlaJmzv8EdsdkhJd0dPlhMl82to2kYCx4jSsGGFT oJDxWNLlCnQ+hdK5/8TmTT3AWXBdZ0dtQw+YcEZiy6qtiRZEgW/Wsu4AIah6rKPs5E Q9UNhsmIXOwkWSQOGdLYkNDsrSlUbXEhgQIsui/IUBZHmEbcSrelhfA/C9vyTyeciF /CsQaqEwWgJQlULUwaYR3XZ8JmzPs1XsP/CZWkcX8H5zBTBmZEHSLA0JBpd5UnwgE1 Ea4K7/WeLYi1o8QljfqDp1Pzi+vttB1vODrxZU7XFKCBSVJBLxoQaiYWOqGiBM9GaA yHLPdab51YduA== X-Mailing-List: nova-gpu@lists.linux.dev List-Id: List-Post: List-Help: List-Subscribe: List-Unsubscribe: List-Owner: Precedence: list Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Wed, 04 Nov 2026 00:52:22 +0100 Message-Id: Subject: Re: [PATCH v3 2/2] gpu: nova-core: add support for GPU timer block. (v3) Cc: , "Andreas Hindborg" , "FUJITA Tomonori" , "Gary Guo" , "Alice Ryhl" , "Daniel Almeida" , "Alexandre Courbot" , , To: "Dave Airlie via B4 Relay" From: "Danilo Krummrich" References: <20261103-nova-timer-v3-0-0cf9bbae8fd9@redhat.com> <20261103-nova-timer-v3-2-0cf9bbae8fd9@redhat.com> In-Reply-To: <20261103-nova-timer-v3-2-0cf9bbae8fd9@redhat.com> On Tue Nov 3, 2026 at 1:33 AM CET, Dave Airlie via B4 Relay wrote: > +// struct that just holds the sys timer offset > +// after initialising the GPU timer block. > +pub(crate) struct Timer { > + sys_timer_offset_ns: i64, > +} I feel like this should actually be TimerState, as it is not really a timer (instance) representation in the sense of carrying the HAL and providing me= thods to interact with the hardware (which we do not intend). I'd suggest making Timer a unit struct and giving it an init() function, i.= e. something like this: struct Timer; struct TimerState { sys_timer_offset_ns: i64, } impl Timer { /// Initialize the GPU's system timer. pub(crate) fn init(bar: Bar0<'_>, chipset: Chipset) -> Result; } > + > +impl Timer { > + pub(crate) fn new(bar: Bar0<'_>, chipset: Chipset) -> Result { > + Ok(Self { > + sys_timer_offset_ns: hal::timer_hal(chipset).init(bar)?, > + }) > + } > + > + // this value will be passed to GspInitRequest SYS_TIMER_OFFSET_NS NIT: This should describe what the returned value represents, although in t= his specific case it may also still make sense to give a hint how it is used. Please use intra-doc links and markdown where it makes sense. > + #[expect(dead_code)] > + pub(crate) fn sys_timer_offset_ns(&self) -> i64 { > + self.sys_timer_offset_ns > + } > +} > diff --git a/drivers/gpu/nova-core/timer/hal.rs b/drivers/gpu/nova-core/t= imer/hal.rs > new file mode 100644 > index 000000000000..fd702867b470 > --- /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: /// Initialize the GPU's system timer. /// /// Returns the system timer offset in nanoseconds. > + fn init(&self, bar: Bar0<'_>) -> Result; > +} > + > +pub(super) fn timer_hal(chipset: Chipset) -> &'static dyn TimerHal { > + match chipset.arch() { > + Architecture::Turing | Architecture::Ampere | Architecture::Ada = =3D> tu102::TU102_HAL, > + Architecture::Hopper =3D> gh100::GH100_HAL, > + Architecture::BlackwellGB10x | Architecture::BlackwellGB20x =3D>= 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..a9d46bdd2527 > --- /dev/null > +++ b/drivers/gpu/nova-core/timer/hal/gh100.rs > @@ -0,0 +1,94 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +use super::TimerHal; > + > +use kernel::{ > + io::Io, > + prelude::*, > + time::*, // > +}; > + > +use crate::{ > + driver::Bar0, > + timer::regs, // > +}; > + > +/// Timer HAL for Hopper and Blackwell. > +struct Gh100 { > + /// `true` on Hopper, the only architecture where the FSP ucode can = predate the fix for > + /// bug 3957833 and hence the sys timer offset must not be programme= d. Blackwell FSP > + /// always includes the fix. > + fsp_bug3957833_war: bool, > +} > + > +/// FSP ucode version from which on the fix for bug 3957833 is included. > +const FSP_BUG_3957833_FIX_VERSION: u32 =3D 0x44c; > + > +impl Gh100 { > + /// kfspRequiresBug3957833WAR_HAL: returns `true` if the FSP ucode p= redates the fix for > + /// bug 3957833 and hence the sys timer offset must not be programme= d. The RM implements > + /// the version check for GH100 only; for all later chips it default= s to `false`. > + fn requires_bug3957833_war(&self, bar: Bar0<'_>) -> bool { > + self.fsp_bug3957833_war > + && bar.read(regs::gh100::NV_GFW_FSP_UCODE_VERSION).full().ge= t() > + < FSP_BUG_3957833_FIX_VERSION I'd move this into regs.rs and make it a method of NV_GFW_FSP_UCODE_VERSION= , e.g. impl NV_GFW_FSP_UCODE_VERSION { pub(crate) fn is_bug_3957833_affected(self) -> bool { const FSP_BUG_3957833_FIX_VERSION: u32 =3D 0x44c; =09 self.full().get() < FSP_BUG_3957833_FIX_VERSION } } so you can just call bar.read(regs::gh100::NV_GFW_FSP_UCODE_VERSION).is_bug_3957833_affected() =09 This also keeps FSP_BUG_3957833_FIX_VERSION local to regs.rs and gets us ri= d of this HAL function. > + } > + > + fn timer_time(&self, bar: Bar0<'_>, time_ns: i64) -> Result { Can't we just use unsigned for the argument and return type? None of those = are ever expected to be negative, no? The invariant for the argument to be non-negative comes from Instant, so I'= d convert before calling into timer_time(). This should avoid the corresponding as ca= sts below. > + let mut sec_hi2 =3D u32::from(bar.read(regs::gh100::NV_PGC6_SCI_= SEC_TIMER_TIME_1)); > + let sec_timer_ns: u64; > + loop { > + let sec_lo =3D u32::from(bar.read(regs::gh100::NV_PGC6_SCI_S= EC_TIMER_TIME_0)); > + let sec_hi =3D u32::from(bar.read(regs::gh100::NV_PGC6_SCI_S= EC_TIMER_TIME_1)); > + if sec_hi =3D=3D sec_hi2 { > + sec_timer_ns =3D u64::from(sec_hi) << 32 | u64::from(sec= _lo); > + break; > + } > + sec_hi2 =3D u32::from(bar.read(regs::gh100::NV_PGC6_SCI_SEC_= TIMER_TIME_1)); > + } > + > + // The system and the GPU timer are both free-running with unrel= ated epochs, so the > + // difference can be negative. Both operands are < 2^62, hence t= his cannot overflow. I think we accumulated a bunch of those comments in nova-core, I think it w= ould be nice to consider whether we want a prefix for this, similar to TIMEOUT o= r CALC. Maybe OVERFLOW? But this can be a follow-up of course. On the other hand, this is a fallible method already, so I'd just go with checked_sub(). > + let sys_timer_offset_ns =3D time_ns - sec_timer_ns as i64; > + > + // The offset is passed to the GSP as an unsigned value and, lik= e in the RM's > + // tmrSetCurrentTime_GH100, is required to be non-negative. With= wall clock time this > + // can only fail if the system clock is set behind the GPU power= -on time. > + if sys_timer_offset_ns < 0 { > + return Err(EINVAL); > + } > + > + if !self.requires_bug3957833_war(bar) { > + let offset_ns =3D sys_timer_offset_ns as u64; > + bar.write_reg( > + regs::gh100::NV_PGC6_SCI_SYS_TIMER_OFFSET_1::zeroed() > + .try_with_nsec(((offset_ns >> 32) & 0x1fff_ffff) as = u32)?, > + ); > + bar.write_reg( > + regs::gh100::NV_PGC6_SCI_SYS_TIMER_OFFSET_0::zeroed() > + .try_with_nsec(((offset_ns >> 5) & 0x7ff_ffff) as u3= 2)? > + .with_update(true), > + ); This looks similar to NV_TLB_FLUSH_PDB_LO::from_pdb_addr(). I think we shou= ld do this in the register impl: impl NV_PGC6_SCI_SYS_TIMER_OFFSET_1 { /// Creates a register value from bits 60:32 of the offset. pub(crate) fn from_offset_ns(offset_ns: u64) -> Self { let nsec =3D Bounded::::from_expr( ((offset_ns >> 32) & 0x1fff_ffff) as u32, ); =09 Self::zeroed().with_nsec(nsec) } } =09 > + // 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; > + } > + } > +} Hm...I feel like it should go into fsp/regs.rs regardless.