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. :)
