Re: [PATCH 10/16] gpu: nova-core: add basic timer device

[Danilo Krummrich <dakr@kernel.org>]

On Sun, Apr 20, 2025 at 09:19:42PM +0900, Alexandre Courbot wrote:
> Add a timer that works with GPU time and provides the ability to wait on
> a condition with a specific timeout.

What can this timer do for us, what and HrTimer can't do for us?

> 
> The `Duration` Rust type is used to keep track is differences between
> timestamps ; this will be replaced by the equivalent kernel type once it
> lands.

Fine for me -- can you please add a corresponding TODO and add it to your list
of follow-up patches?

> diff --git a/drivers/gpu/nova-core/timer.rs b/drivers/gpu/nova-core/timer.rs
> new file mode 100644
> index 0000000000000000000000000000000000000000..8987352f4192bc9b4b2fc0fb5f2e8e62ff27be68
> --- /dev/null
> +++ b/drivers/gpu/nova-core/timer.rs
> @@ -0,0 +1,133 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Nova Core Timer subdevice
> +
> +// To be removed when all code is used.
> +#![allow(dead_code)]

Please prefer 'expect'.

> +
> +use core::fmt::Display;
> +use core::ops::{Add, Sub};
> +use core::time::Duration;
> +
> +use kernel::devres::Devres;
> +use kernel::num::U64Ext;
> +use kernel::prelude::*;
> +
> +use crate::driver::Bar0;
> +use crate::regs;
> +
> +/// A timestamp with nanosecond granularity obtained from the GPU timer.
> +///
> +/// A timestamp can also be substracted to another in order to obtain a [`Duration`].
> +#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord)]
> +pub(crate) struct Timestamp(u64);
> +
> +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()));
> +            }

So, if hi did not change since we've read both hi and lo, we can trust both
values. Probably worth to add a brief comment.

Additionally, we may want to add that if we get unlucky, it takes around 4s to
get unlucky again, even though that's rather obvious.

> +        }
> +    }
> +
> +    #[allow(dead_code)]
> +    pub(crate) fn time(bar: &Bar0, time: u64) {
> +        regs::PtimerTime1::default()
> +            .set_hi(time.upper_32_bits())
> +            .write(bar);
> +        regs::PtimerTime0::default()
> +            .set_lo(time.lower_32_bits())
> +            .write(bar);
> +    }
> +
> +    /// Wait until `cond` is true or `timeout` elapsed, based on GPU time.
> +    ///
> +    /// When `cond` evaluates to `Some`, its return value is returned.
> +    ///
> +    /// `Err(ETIMEDOUT)` is returned if `timeout` has been reached without `cond` evaluating to
> +    /// `Some`, or if the timer device is stuck for some reason.
> +    pub(crate) fn wait_on<R, F: Fn() -> Option<R>>(
> +        &self,
> +        bar: &Devres<Bar0>,
> +        timeout: Duration,
> +        cond: F,
> +    ) -> Result<R> {
> +        // Number of consecutive time reads after which we consider the timer frozen if it hasn't
> +        // moved forward.
> +        const MAX_STALLED_READS: usize = 16;

Huh! Can't we trust the timer hardware? Probably one reason more to use HrTimer?

> +
> +        let (mut cur_time, mut prev_time, deadline) = {
> +            let cur_time = with_bar!(bar, |b| self.read(b))?;
> +            let deadline = cur_time + timeout;
> +
> +            (cur_time, cur_time, deadline)
> +        };
> +        let mut num_reads = 0;
> +
> +        loop {
> +            if let Some(ret) = cond() {
> +                return Ok(ret);
> +            }
> +
> +            (|| {
> +                cur_time = with_bar!(bar, |b| self.read(b))?;
> +
> +                /* Check if the timer is frozen for some reason. */
> +                if cur_time == prev_time {
> +                    if num_reads >= MAX_STALLED_READS {
> +                        return Err(ETIMEDOUT);
> +                    }
> +                    num_reads += 1;
> +                } else {
> +                    if cur_time >= deadline {
> +                        return Err(ETIMEDOUT);
> +                    }
> +
> +                    num_reads = 0;
> +                    prev_time = cur_time;
> +                }
> +
> +                Ok(())
> +            })()?;
> +        }
> +    }
> +}
> 
> -- 
> 2.49.0
>