Re: [PATCH v2 08/12] nova-core: sequencer: Add register opcodes

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

On Mon Nov 3, 2025 at 8:59 AM JST, Joel Fernandes wrote:
> These opcodes are used for register write, modify, poll and store (save)
> sequencer operations.
>
> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>
> ---
>  drivers/gpu/nova-core/gsp/sequencer.rs | 138 +++++++++++++++++++++++--
>  1 file changed, 131 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/nova-core/gsp/sequencer.rs b/drivers/gpu/nova-core/gsp/sequencer.rs
> index 48c40140876b..f429fee01f86 100644
> --- a/drivers/gpu/nova-core/gsp/sequencer.rs
> +++ b/drivers/gpu/nova-core/gsp/sequencer.rs
> @@ -5,6 +5,7 @@
>  use core::mem::size_of;
>  use kernel::alloc::flags::GFP_KERNEL;
>  use kernel::device;
> +use kernel::io::poll::read_poll_timeout;
>  use kernel::prelude::*;
>  use kernel::time::Delta;
>  use kernel::transmute::FromBytes;
> @@ -40,13 +41,36 @@ struct GspSequencerInfo<'a> {
>  
>  /// GSP Sequencer Command types with payload data.
>  /// Commands have an opcode and a opcode-dependent struct.
> -#[allow(dead_code)]
> -pub(crate) enum GspSeqCmd {}
> +#[allow(clippy::enum_variant_names)]
> +pub(crate) enum GspSeqCmd {
> +    RegWrite(fw::GSP_SEQ_BUF_PAYLOAD_REG_WRITE),
> +    RegModify(fw::GSP_SEQ_BUF_PAYLOAD_REG_MODIFY),
> +    RegPoll(fw::GSP_SEQ_BUF_PAYLOAD_REG_POLL),
> +    RegStore(fw::GSP_SEQ_BUF_PAYLOAD_REG_STORE),
> +}
>  
>  impl GspSeqCmd {
>      /// Creates a new GspSeqCmd from a firmware GSP_SEQUENCER_BUFFER_CMD.
> -    pub(crate) fn from_fw_cmd(_cmd: &fw::GSP_SEQUENCER_BUFFER_CMD) -> Result<Self> {
> -        Err(EINVAL)
> +    pub(crate) fn from_fw_cmd(cmd: &fw::GSP_SEQUENCER_BUFFER_CMD) -> Result<Self> {
> +        match cmd.opCode {
> +            fw::GSP_SEQ_BUF_OPCODE_GSP_SEQ_BUF_OPCODE_REG_WRITE => {
> +                // SAFETY: We're using the union field that corresponds to the opCode.
> +                Ok(GspSeqCmd::RegWrite(unsafe { cmd.payload.regWrite }))
> +            }
> +            fw::GSP_SEQ_BUF_OPCODE_GSP_SEQ_BUF_OPCODE_REG_MODIFY => {
> +                // SAFETY: We're using the union field that corresponds to the opCode.
> +                Ok(GspSeqCmd::RegModify(unsafe { cmd.payload.regModify }))
> +            }
> +            fw::GSP_SEQ_BUF_OPCODE_GSP_SEQ_BUF_OPCODE_REG_POLL => {
> +                // SAFETY: We're using the union field that corresponds to the opCode.
> +                Ok(GspSeqCmd::RegPoll(unsafe { cmd.payload.regPoll }))
> +            }
> +            fw::GSP_SEQ_BUF_OPCODE_GSP_SEQ_BUF_OPCODE_REG_STORE => {
> +                // SAFETY: We're using the union field that corresponds to the opCode.
> +                Ok(GspSeqCmd::RegStore(unsafe { cmd.payload.regStore }))
> +            }

I'd rather have these `unsafe` statements in the `fw` module - guess
that's all he more reason to add an abstraction layer there.

> +            _ => Err(EINVAL),
> +        }
>      }
>  
>      pub(crate) fn new(data: &[u8], dev: &device::Device<device::Bound>) -> Result<Self> {
> @@ -64,7 +88,16 @@ pub(crate) fn new(data: &[u8], dev: &device::Device<device::Bound>) -> Result<Se
>      /// Get the size of this command in bytes, the command consists of
>      /// a 4-byte opcode, and a variable-sized payload.
>      pub(crate) fn size_bytes(&self) -> usize {
> -        0
> +        let opcode_size = size_of::<fw::GSP_SEQ_BUF_OPCODE>();
> +        match self {
> +            // For commands with payloads, add the payload size in bytes.
> +            GspSeqCmd::RegWrite(_) => opcode_size + size_of::<fw::GSP_SEQ_BUF_PAYLOAD_REG_WRITE>(),
> +            GspSeqCmd::RegModify(_) => {
> +                opcode_size + size_of::<fw::GSP_SEQ_BUF_PAYLOAD_REG_MODIFY>()
> +            }
> +            GspSeqCmd::RegPoll(_) => opcode_size + size_of::<fw::GSP_SEQ_BUF_PAYLOAD_REG_POLL>(),
> +            GspSeqCmd::RegStore(_) => opcode_size + size_of::<fw::GSP_SEQ_BUF_PAYLOAD_REG_STORE>(),
> +        }
>      }
>  }
>  
> @@ -83,12 +116,103 @@ pub(crate) trait GspSeqCmdRunner {
>      fn run(&self, sequencer: &GspSequencer<'_>) -> Result;
>  }
>  
> -impl GspSeqCmdRunner for GspSeqCmd {
> -    fn run(&self, _seq: &GspSequencer<'_>) -> Result {
> +impl GspSeqCmdRunner for fw::GSP_SEQ_BUF_PAYLOAD_REG_WRITE {
> +    fn run(&self, sequencer: &GspSequencer<'_>) -> Result {
> +        dev_dbg!(
> +            sequencer.dev,
> +            "RegWrite: addr=0x{:x}, val=0x{:x}\n",
> +            self.addr,
> +            self.val
> +        );

Per the other feedback I believe you are going to remove these `dev_dbg`
anyway, but since the opcodes derive a `Debug` implementation, you could
have just printed that from the caller for a similar result and no extra
code.

> +        let addr = self.addr as usize;
> +        let val = self.val;
> +        let _ = sequencer.bar.try_write32(val, addr);
> +        Ok(())
> +    }
> +}
> +
> +impl GspSeqCmdRunner for fw::GSP_SEQ_BUF_PAYLOAD_REG_MODIFY {
> +    fn run(&self, sequencer: &GspSequencer<'_>) -> Result {
> +        dev_dbg!(
> +            sequencer.dev,
> +            "RegModify: addr=0x{:x}, mask=0x{:x}, val=0x{:x}\n",
> +            self.addr,
> +            self.mask,
> +            self.val
> +        );
> +
> +        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);
> +        }
>          Ok(())
>      }
>  }
>  
> +impl GspSeqCmdRunner for fw::GSP_SEQ_BUF_PAYLOAD_REG_POLL {
> +    fn run(&self, sequencer: &GspSequencer<'_>) -> Result {
> +        dev_dbg!(
> +            sequencer.dev,
> +            "RegPoll: addr=0x{:x}, mask=0x{:x}, val=0x{:x}, timeout=0x{:x}, error=0x{:x}\n",
> +            self.addr,
> +            self.mask,
> +            self.val,
> +            self.timeout,
> +            self.error
> +        );
> +
> +        let addr = self.addr as usize;
> +        let mut timeout_us = i64::from(self.timeout);
> +
> +        // Default timeout to 4 seconds.
> +        timeout_us = if timeout_us == 0 { 4000000 } else { timeout_us };

`let timeout_us = if self.timeout == 0 { 4_000_000 } else {
i64::from(self.timeout)`

(removes the `mut` on `timeout_us`)

> +
> +        // 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::GSP_SEQ_BUF_PAYLOAD_REG_STORE {
> +    fn run(&self, sequencer: &GspSequencer<'_>) -> Result {
> +        let addr = self.addr as usize;
> +        let _index = self.index;
> +
> +        let val = sequencer.bar.try_read32(addr)?;
> +
> +        dev_dbg!(
> +            sequencer.dev,
> +            "RegStore: addr=0x{:x}, index=0x{:x}, value={:?}\n",
> +            self.addr,
> +            self.index,
> +            val
> +        );
> +
> +        Ok(())
> +    }
> +}
> +
> +impl GspSeqCmdRunner for GspSeqCmd {
> +    fn run(&self, seq: &GspSequencer<'_>) -> Result {
> +        match self {
> +            GspSeqCmd::RegWrite(cmd) => cmd.run(seq),
> +            GspSeqCmd::RegModify(cmd) => cmd.run(seq),
> +            GspSeqCmd::RegPoll(cmd) => cmd.run(seq),
> +            GspSeqCmd::RegStore(cmd) => cmd.run(seq),
> +        }
> +    }
> +}

This makes me wonder: do we need to store the deserialized version of
these operands, and make a second `match` on them? How about passing the
`bar` to the deserialization command and have it run the operand
immediately?