Alexandre Courbot
2025-Mar-21 05:41 UTC
[PATCH RFC v3 6/7] gpu: nova-core: add basic timer device
Hi Boqun, On Fri Mar 21, 2025 at 3:17 AM JST, Boqun Feng wrote:> Hi Alexandre, > > On Thu, Mar 20, 2025 at 10:39:14PM +0900, Alexandre Courbot wrote: >> Add a basic timer device and exercise it during device probing. This >> first draft is probably very questionable. >> >> One point in particular which should IMHO receive attention: the generic >> wait_on() method aims at providing similar functionality to Nouveau's >> nvkm_[num]sec() macros. Since this method will be heavily used with >> different conditions to test, I'd like to avoid monomorphizing it >> entirely with each instance ; that's something that is achieved in >> nvkm_xsec() using functions that the macros invoke. >> >> I have tried achieving the same result in Rust using closures (kept >> as-is in the current code), but they seem to be monomorphized as well. >> Calling extra functions could work better, but looks also less elegant >> to me, so I am really open to suggestions here. >> >> Signed-off-by: Alexandre Courbot <acourbot at nvidia.com> >> --- >> drivers/gpu/nova-core/driver.rs | 4 +- >> drivers/gpu/nova-core/gpu.rs | 55 +++++++++++++++- >> drivers/gpu/nova-core/nova_core.rs | 1 + >> drivers/gpu/nova-core/regs.rs | 11 ++++ >> drivers/gpu/nova-core/timer.rs | 132 +++++++++++++++++++++++++++++++++++++ >> 5 files changed, 201 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/nova-core/driver.rs b/drivers/gpu/nova-core/driver.rs >> index 63c19f140fbdd65d8fccf81669ac590807cc120f..0cd23aa306e4082405f480afc0530a41131485e7 100644 >> --- a/drivers/gpu/nova-core/driver.rs >> +++ b/drivers/gpu/nova-core/driver.rs >> @@ -10,7 +10,7 @@ pub(crate) struct NovaCore { >> pub(crate) gpu: Gpu, >> } >> >> -const BAR0_SIZE: usize = 8; >> +const BAR0_SIZE: usize = 0x9500; >> pub(crate) type Bar0 = pci::Bar<BAR0_SIZE>; >> >> kernel::pci_device_table!( >> @@ -42,6 +42,8 @@ fn probe(pdev: &mut pci::Device, _info: &Self::IdInfo) -> Result<Pin<KBox<Self>> >> GFP_KERNEL, >> )?; >> >> + let _ = this.gpu.test_timer(); >> + >> Ok(this) >> } >> } >> diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs >> index d96901e5c8eace1e7c57c77da7def209e8149cd3..f010d3152530b1cec032ca620e59de51a2fc1a13 100644 >> --- a/drivers/gpu/nova-core/gpu.rs >> +++ b/drivers/gpu/nova-core/gpu.rs >> @@ -6,8 +6,10 @@ >> >> use crate::driver::Bar0; >> use crate::regs; >> +use crate::timer::Timer; >> use crate::util; >> use core::fmt; >> +use core::time::Duration; >> > > Since there is already a Delta type proposed to represent the timestamp > difference in kernel: > > https://lore.kernel.org/rust-for-linux/20250220070611.214262-4-fujita.tomonori at gmail.com/ > > , could you please make your work based on that and avoid using > core::time::Duration. Thanks! > >> macro_rules! define_chipset { >> ({ $($variant:ident = $value:expr),* $(,)* }) => >> @@ -179,6 +181,7 @@ pub(crate) struct Gpu { >> /// MMIO mapping of PCI BAR 0 >> bar: Devres<Bar0>, >> fw: Firmware, >> + timer: Timer, >> } >> > [...] >> + >> +/// A timestamp with nanosecond granularity obtained from the GPU timer. >> +/// >> +/// A timestamp can also be substracted to another in order to obtain a [`Duration`]. >> +/// >> +/// TODO: add Kunit tests! >> +#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord)] >> +pub(crate) struct Timestamp(u64); >> + > > Also an Instant type has been proposed and reviewed for a while: > > https://lore.kernel.org/rust-for-linux/20250220070611.214262-5-fujita.tomonori at gmail.com/ > > we should use that type instead of re-inventing the wheel here. Of > course, it's currently not quite working because Instant is only for > CLOCK_MONOTONIC. But there was a proposal to make `Instant` generic over > clock: > > https://lore.kernel.org/rust-for-linux/20230714-rust-time-v2-1-f5aed84218c4 at asahilina.net/ > > if you follow that design, you can implement a `Instant<NovaGpu>`, where > > ipml Now for NovaGpu { > fn now() -> Instant<Self> { > // your Timer::read() implementation. > } > }Ah, thanks for pointing this out. I'll keep track of these patches, hopefully they get merged soon!
Daniel Brooks
2025-Mar-21 16:20 UTC
[PATCH RFC v3 6/7] gpu: nova-core: add basic timer device
"Alexandre Courbot" <acourbot at nvidia.com> writes:> Hi Boqun, > > On Fri Mar 21, 2025 at 3:17 AM JST, Boqun Feng wrote: >> Also an Instant type has been proposed and reviewed for a while: >> >> https://lore.kernel.org/rust-for-linux/20250220070611.214262-5-fujita.tomonori at gmail.com/ >> >> we should use that type instead of re-inventing the wheel here. Of >> course, it's currently not quite working because Instant is only for >> CLOCK_MONOTONIC. But there was a proposal to make `Instant` generic over >> clock: >> >> https://lore.kernel.org/rust-for-linux/20230714-rust-time-v2-1-f5aed84218c4 at asahilina.net/ >> >> if you follow that design, you can implement a `Instant<NovaGpu>`, where >> >> ipml Now for NovaGpu { >> fn now() -> Instant<Self> { >> // your Timer::read() implementation. >> } >> } > > Ah, thanks for pointing this out. I'll keep track of these patches, > hopefully they get merged soon!Would that actually work though? Instant is a ktime_t, which is a signed i64 rather than a u64. When I read your patch I assumed that you had to add your own Timestamp because the value had to be whatever was read from the GPU, independent of the clock value kept by the system or other hardware. db48x
Alexandre Courbot
2025-Mar-24 01:03 UTC
[PATCH RFC v3 6/7] gpu: nova-core: add basic timer device
On Sat Mar 22, 2025 at 1:20 AM JST, Daniel Brooks wrote:> "Alexandre Courbot" <acourbot at nvidia.com> writes: > >> Hi Boqun, >> >> On Fri Mar 21, 2025 at 3:17 AM JST, Boqun Feng wrote: >>> Also an Instant type has been proposed and reviewed for a while: >>> >>> https://lore.kernel.org/rust-for-linux/20250220070611.214262-5-fujita.tomonori at gmail.com/ >>> >>> we should use that type instead of re-inventing the wheel here. Of >>> course, it's currently not quite working because Instant is only for >>> CLOCK_MONOTONIC. But there was a proposal to make `Instant` generic over >>> clock: >>> >>> https://lore.kernel.org/rust-for-linux/20230714-rust-time-v2-1-f5aed84218c4 at asahilina.net/ >>> >>> if you follow that design, you can implement a `Instant<NovaGpu>`, where >>> >>> ipml Now for NovaGpu { >>> fn now() -> Instant<Self> { >>> // your Timer::read() implementation. >>> } >>> } >> >> Ah, thanks for pointing this out. I'll keep track of these patches, >> hopefully they get merged soon! > > Would that actually work though? Instant is a ktime_t, which is a signed > i64 rather than a u64. When I read your patch I assumed that you had to > add your own Timestamp because the value had to be whatever was read > from the GPU, independent of the clock value kept by the system or other > hardware.That's correct, and honestly there would be little benefit in using an already existing type outside of not re-writing very similar code as GPU time stamps are only used locally anyway. I'll keep an eye on these patches though in case we can harmonize things a bit.