Re: [PATCH v5 08/13] gpu: nova-core: sequencer: Add register opcodes

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

Hi Lyude,

On Sat Nov 15, 2025 at 6:55 AM JST, Lyude Paul wrote:
> We're very close! I see a few things we still need to fix though
>
> On Fri, 2025-11-14 at 14:55 -0500, Joel Fernandes wrote:
>> ...
>> -impl GspSeqCmdRunner for GspSeqCmd {
>> -    fn run(&self, _seq: &GspSequencer<'_>) -> Result {
>> +impl GspSeqCmdRunner for fw::RegWritePayload {
>> +    fn run(&self, sequencer: &GspSequencer<'_>) -> Result {
>> +        let addr = self.addr() as usize;
>> +        let val = self.val();
>> +        let _ = sequencer.bar.try_write32(val, addr);
>
> We're still not handling the possible error from try_write32() here
> Also - addr/val seem a bit superfluous

I wonder whether this could be on purpose? As in, the register sequence
might try to write to non-existing registers on some models but this is
not necessarily a fatal error. Still, I can complete initialization even
when handling the error, so let's err on the side or correctness unless
Joel signals that this is indeed what we want (in which case a comment
would ensure there cannot be any confusion as to the purpose).

For now, changed this to:

    let addr = usize::from_safe_cast(self.addr());

    sequencer.bar.try_write32(self.val(), addr)
>
>>          Ok(())
>>      }
>>  }
>>  
>> +impl GspSeqCmdRunner for fw::RegModifyPayload {
>> +    fn run(&self, sequencer: &GspSequencer<'_>) -> Result {
>> +        let addr = self.addr() as usize;
>> +        if let Ok(temp) = sequencer.bar.try_read32(addr) {
>> +            let _ = sequencer
>> +                .bar
>> +                .try_write32((temp & !self.mask()) | self.val(), addr);
>
> Same here

This is now:

    let addr = usize::from_safe_cast(self.addr());

    sequencer.bar.try_read32(addr).and_then(|val| {
        sequencer
            .bar
            .try_write32((val & !self.mask()) | self.val(), addr)
    })

>
>> +        }
>> +        Ok(())
>> +    }
>> +}
>> +
>> +impl GspSeqCmdRunner for fw::RegPollPayload {
>> +    fn run(&self, sequencer: &GspSequencer<'_>) -> Result {
>> +        let addr = self.addr() as usize;
>> +
>> +        // Default timeout to 4 seconds.
>> +        let timeout_us = if self.timeout() == 0 {
>> +            4_000_000
>> +        } else {
>> +            i64::from(self.timeout())
>> +        };
>> +
>> +        // First read.
>> +        sequencer.bar.try_read32(addr)?;
>> +
>> +        // Poll the requested register with requested timeout.
>> +        read_poll_timeout(
>> +            || sequencer.bar.try_read32(addr),
>> +            |current| (current & self.mask()) == self.val(),
>> +            Delta::ZERO,
>> +            Delta::from_micros(timeout_us),
>> +        )
>> +        .map(|_| ())
>> +    }
>> +}
>> +
>> +impl GspSeqCmdRunner for fw::RegStorePayload {
>> +    fn run(&self, sequencer: &GspSequencer<'_>) -> Result {
>> +        let addr = self.addr() as usize;
>> +        let _index = self.index();
>> +
>> +        let _val = sequencer.bar.try_read32(addr)?;
>> +
>> +        Ok(())
>
> These variables still seem superfluous - we don't use _index at all.
>
> This function should just be rewritten as:
>
>     fn run(&self, sequencer: &GspSequencer<'_>) -> Result {
>         sequencer.bar.try_read32(self.addr() as usize)?;

Actually I thought this is also a good opportunity to replace the `as`
with the safer `FromSafeCast` interface we introduced recently precisely
for that (you probably have already noticed from the previous fixes :)).
Only this would look a bit heavy if done inline, so for readability I'll
keep the temporary variable and change this to:

    let addr = usize::from_safe_cast(self.addr());

    sequencer.bar.try_read32(addr).map(|_| ())

This makes me wonder about the `index` member of this op - it is not
used at all! Joel, do you know what it is for?

Thanks for spotting these!