Re: [PATCH RFC v3 6/7] gpu: nova-core: add basic timer device

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

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@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@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@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@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!