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 <acour...@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.tomon...@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.tomon...@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-f5aed8421...@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!
