Re: [PATCH v4 2/2] gpu: nova-core: add support for GPU timer block. (v4)

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

On Fri Nov 6, 2026 at 2:31 PM JST, David Airlie wrote:
<...>
> I've done nearly everything you requested for v5.

Thanks, took a quick look at it. It's getting late so I'll do a more
in-depth review tomorrow.

>
> I've just stored Chipset in there, is it worth storing hal? It might
> happen later I suppose if needed, but for this I just rederive hal for
> the one entrypoint.

Storing the hal is technically better, because in
`is_bug_3957833_affected` you need to handle the case where `fsp_hal`
returns `None`, even though we have already assessed that the HAL
existed in `Fsp::new`. So it's just a dead branch that can be avoided by
storing a reference to the HAL instead of the Chipset.

If you do that, `FspHal` will need to require `Send + Sync`, but that
won't create any issue.

Now in practice, `is_bug_3957833_affected` is only ever called once, so
it makes little difference. But I expect the FSP HAL will expand
further, so storing it will definitely become the long-term design.

>
>> Maybe also explain that this can program some timer-related registers,
>> otherwise the name `init` isn't justified for someone who believes that
>> this just returns the time without any side-effect.
>
> I used setup, I don't have a good name.
>
> The main thing is I didn't use the NSEC_SHIFT thing, but just write
> nsec_hi and lo wrappers that hardcode the values since to me it looks
> cleaner than trying to infer that from the register definitions,
> especially since I don't see a good way of doing it for nsec_hi. I

Ah yeah, I saw that you used a helper function that is used by both sets
of timer registers. In this case it is not obvious which one should
provide the constant.

In this context I guess we can go with it because this is only used
locally, and `Bounded` will throw a build error if the number of bits
don't match, but generally speaking I would like to avoid repeating
Nouveau's pattern of producing literal values out of a hat and use the
defined constants whenever possible, as they act as documentation on top
of preventing mistakes.

> remove all falling from these paths and just went with truncation
> because that is what actually makes sense.

Do we understand what happens if we truncate though?

>
> nsec_lo/hi are now used in the register from_nsec for both tu102 and
> gh100 registers.
>
>> I'd ignore Sashiko's suggestion as it looks like it is trying to
>> reconcile the code against the comment instead of the other way around.
>
> I just fixed things to truncate and used wrapping_sub, to be honest
> being careful here is probably the wrong answer, my local review bot
> pointed out some wierdness can occur in 2043 with the current scheme,
> so let's just keep it simple, wrapping subtract, and feed the result
> to the hardware which is what the other driver does and what nouveau
> did.

The other drivers do what they do, but here again do we understand what
happens when we write a wrapped value into the timer registers?

>
>>
>> Once we have moved the timer read loop into its own method, I guess the
>> remainder can be nicely folded into `init` directly, and we don't need
>> `timer_time` anymore?
>
> I didn't do this, I think resume support might need to program this
> also, and it didn't look very clean.

No problem, especially if this is expected to be used later.