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

["Alexandre Courbot" <acourbot@nvidia.com>]

On Tue Apr 22, 2025 at 9:07 PM JST, Danilo Krummrich wrote:
> 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?

It is local to the GPU, and the source of truth for all GPU-related
operations. Some pushbuffer commands can return timestamps that will
come from this timer and the driver must thus use it as well in
driver-related operations to make sure both are on the same table.

>
>> 
>> 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?

Sure.

>
>> 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'.

Ack.

>
>> +
>> +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.

Added a comment. The odds of being unlucky are infinitesimal and the
consequences (an extra pass of this loop) inconsequential, thankfully.

>
>> +        }
>> +    }
>> +
>> +    #[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?

No, to be clear I don't expect this to ever happen in real life, but I
also don't want to leave a loop without an exit condition.

OpenRM and Nouveau are both using it so I believe it can be trusted. :)