thr3ads.net - Nouveau - [PATCH v2 08/12] nova-core: sequencer: Add register opcodes [Nov 2025]

If this information is useful, please help other people find it:
Share via:

Joel Fernandes

2025-Nov-02 23:59 UTC

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

These opcodes are used for register write, modify, poll and store (save)
sequencer operations.

Signed-off-by: Joel Fernandes <joelagnelf at 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 }))
+            }
+            _ => 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
+        );
+        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 };
+
+        // 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),
+        }
+    }
+}
+
 pub(crate) struct GspSeqIter<'a> {
     cmd_data: &'a [u8],
     current_offset: usize, // Tracking the current position.
-- 
2.34.1

John Hubbard

2025-Nov-05 02:50 UTC

head link

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

On 11/2/25 3:59 PM, Joel Fernandes wrote:> These opcodes are used for register write, modify, poll and store (save)
> sequencer operations.
> 
> Signed-off-by: Joel Fernandes <joelagnelf at nvidia.com>
> ---
>  drivers/gpu/nova-core/gsp/sequencer.rs | 138 +++++++++++++++++++++++--
>  1 file changed, 131 insertions(+), 7 deletions(-)
...> @@ -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",
Hi Joel,

The RegRead, RegWrite, RegPoll prints generate over 400 lines
per GPU, into the logs. This is too much, especially now that
it's been working for a while.

I'm thinking let's delete these entirely. If we somehow get
into debugging this aspect of the sequencer, we can temporarily
add whatever printing we need, but I think it's one notch too
far for the final product, now that you have it working.

thanks,
-- 
John Hubbard

Alexandre Courbot

2025-Nov-10 13:50 UTC

head link

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

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 at 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?

Nouveau - Nov 2025 - [PATCH v2 08/12] nova-core: sequencer: Add register opcodes

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

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

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