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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxx/
, 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@xxxxxxxxx/
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@xxxxxxxxxxxxx/
if you follow that design, you can implement a `Instant<NovaGpu>`, where
ipml Now for NovaGpu {
fn now() -> Instant<Self> {
// your Timer::read() implementation.
}
}
Thanks!
Regards,
Boqun
> +impl Display for Timestamp {
> + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
> + write!(f, "{}", self.0)
> + }
> +}
> +
> +impl Add<Duration> for Timestamp {
> + type Output = Self;
> +
> + fn add(mut self, rhs: Duration) -> Self::Output {
> + let mut nanos = rhs.as_nanos();
> + while nanos > u64::MAX as u128 {
> + self.0 = self.0.wrapping_add(nanos as u64);
> + nanos -= u64::MAX as u128;
> + }
> +
> + Timestamp(self.0.wrapping_add(nanos as u64))
> + }
> +}
> +
> +impl Sub for Timestamp {
> + type Output = Duration;
> +
> + fn sub(self, rhs: Self) -> Self::Output {
> + Duration::from_nanos(self.0.wrapping_sub(rhs.0))
> + }
> +}
> +
> +pub(crate) struct Timer {}
> +
> +impl Timer {
> + pub(crate) fn new() -> Self {
> + Self {}
> + }
> +
> + /// Read the current timer timestamp.
> + pub(crate) fn read(&self, bar: &Bar0) -> Timestamp {
> + loop {
> + let hi = regs::PtimerTime1::read(bar);
> + let lo = regs::PtimerTime0::read(bar);
> +
> + if hi.hi() == regs::PtimerTime1::read(bar).hi() {
> + return Timestamp(u64::from_u32s(hi.hi(), lo.lo()));
> + }
> + }
> + }
> +
[...]
