Re: [PATCH v2 4/9] gpu: nova-core: gsp: cmdq: split the transport part of the send path

[Alexandre Courbot]

On Mon Sep 28, 2026 at 2:16 PM JST, Eliot Courtney wrote:
> On Sun Sep 27, 2026 at 10:46 PM JST, Alexandre Courbot wrote:
>> Move the transport part of `send_single_command` into
>> `send_command_element`, which allocates the queue slots, writes the
>> element header, calls a closure to fill the remainder of the command,
>> then computes the checksum, advances the write pointer and rings the
>> doorbell.
>>
>> The RPC part of `send_single_command` (writing the RPC header and the
>> command payload) is passed as a closure, unchanged apart from its
>> indentation. This sets things up for moving the RPC code into its own
>> sub-module, leaving the transport agnostic of the message type.
>>
>> No functional change intended.
>>
>> Signed-off-by: Alexandre Courbot <acourbot@xxxxxxxxxx>
>> ---
>>  drivers/gpu/nova-core/gsp/cmdq.rs | 118 ++++++++++++++++++++++++--------------
>>  1 file changed, 74 insertions(+), 44 deletions(-)
>>
>> diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
>> index 07e8e32c3d57..b6d50b0bd039 100644
>> --- a/drivers/gpu/nova-core/gsp/cmdq.rs
>> +++ b/drivers/gpu/nova-core/gsp/cmdq.rs
>> @@ -638,65 +638,35 @@ impl CmdqInner<'_> {
>>      /// Timeout for waiting for space on the command queue.
>>      const ALLOCATE_TIMEOUT: Delta = Delta::from_secs(1);
>>  
>> -    /// Sends `command` to the GSP, without splitting it.
>> +    /// Allocates enough send slots to store a command of `sizes_in_bytes` length, initialize them
>> +    /// using `command_init`, and send the command to the GSP.
>>      ///
>>      /// # Errors
>>      ///
>>      /// - `EMSGSIZE` if the command exceeds the maximum queue element size.
>>      /// - `ETIMEDOUT` if space does not become available within the timeout.
>> -    /// - `EIO` if the variable payload requested by the command has not been entirely
>> -    ///   written to by its [`CommandToGsp::init_variable_payload`] method.
>>      ///
>> -    /// Error codes returned by the command initializers are propagated as-is.
>> -    fn send_single_command<M>(&mut self, command: M) -> Result
>> -    where
>> -        M: CommandToGsp,
>> -        // This allows all error types, including `Infallible`, to be used for `M::InitError`.
>> -        Error: From<M::InitError>,
>> -    {
>> -        let size_in_bytes = command.size();
>> -        let dst = self
>> +    /// Error codes returned by `command_init` are returned as-is.
>> +    fn send_command_element(
>> +        &mut self,
>> +        size_in_bytes: usize,
>> +        command_init: impl FnOnce(&mut GspCommand<'_>) -> Result,
>> +    ) -> Result {
>> +        let mut dst = self
>>              .gsp_mem
>>              .allocate_command(size_in_bytes, Self::ALLOCATE_TIMEOUT)?;
>>  
>> +        let seq = self.seq;
>
> nit: this local reads noisily to me
>
>> +
>>          // Fill the header.
>> -        let msg_element_init = GspMsgElement::init(self.seq, size_in_bytes);
>> +        let msg_element_init = GspMsgElement::init(seq, size_in_bytes);
>>          // SAFETY: `msg_header` is a valid reference, and not touched if the initializer fails.
>>          unsafe {
>>              pin_init::raw_try_init(core::ptr::from_mut(dst.header), msg_element_init)?;
>>          }
>>  
>> -        // Extract area for the command itself. The GSP message header and the command header
>> -        // together are guaranteed to fit entirely into a single page, so it's ok to only look
>> -        // at `dst.contents.0` here.
>> -        let (cmd, payload_1) = M::Command::from_bytes_mut_prefix(dst.contents.0).ok_or(EIO)?;
>> -        let rpc_header_init = RpcMessageHeader::init(size_in_bytes, M::FUNCTION);
>> -        // SAFETY: `rpc_header_mut()` and `cmd` are valid references, and not touched if the
>> -        // initializer fails.
>> -        unsafe {
>> -            pin_init::raw_try_init(
>> -                core::ptr::from_mut(dst.header.rpc_header_mut()),
>> -                rpc_header_init,
>> -            )?;
>> -            pin_init::raw_try_init(core::ptr::from_mut(cmd), command.init())?;
>> -        }
>> -
>> -        // Fill the variable-length payload, which may be empty.
>> -        let mut sbuffer = SBufferIter::new_writer([&mut payload_1[..], &mut dst.contents.1[..]]);
>> -        command.init_variable_payload(&mut sbuffer)?;
>> -
>> -        if !sbuffer.is_empty() {
>> -            return Err(EIO);
>> -        }
>> -        drop(sbuffer);
>> -
>> -        dev_dbg!(
>> -            &self.dev,
>> -            "GSP RPC: send: seq# {}, function={:?}, length=0x{:x}\n",
>> -            self.seq,
>> -            M::FUNCTION,
>> -            size_in_bytes,
>> -        );
>> +        // Initialize the message payload.
>> +        command_init(&mut dst)?;
>>  
>>          // Compute checksum now that the whole message is ready.
>>          dst.header
>> @@ -715,6 +685,66 @@ fn send_single_command<M>(&mut self, command: M) -> Result
>>          Ok(())
>>      }
>>  
>> +    /// Sends `command` to the GSP, without splitting it.
>> +    ///
>> +    /// # Errors
>> +    ///
>> +    /// - `EMSGSIZE` if the command exceeds the maximum queue element size.
>> +    /// - `ETIMEDOUT` if space does not become available within the timeout.
>> +    /// - `EIO` if the variable payload requested by the command has not been entirely
>> +    ///   written to by its [`CommandToGsp::init_variable_payload`] method.
>> +    ///
>> +    /// Error codes returned by the command initializers are propagated as-is.
>> +    fn send_single_command<M>(&mut self, command: M) -> Result
>> +    where
>> +        M: CommandToGsp,
>> +        // This allows all error types, including `Infallible`, to be used for `M::InitError`.
>> +        Error: From<M::InitError>,
>> +    {
>> +        let dev = self.dev;
>> +        let seq = self.seq;
>> +        let size_in_bytes = command.size();
>
> I suspect if we pass dev and seq into the closure below, we can
> further split the RPC layer from the transport layer. That means the
> patches after this won't have to impl on CmdqInner for e.g.
> `send_single_command` and instead we can just define a trait that writes
> in the message layer data (so send_single_command could e.g. instead
> take an impl RpcCommandWriter or whatever, and we can impl
> RpcCommandWriter for anything that impls CommandToGsp, inserting the
> message layer protocol stuff in there).

That sounds like a much better design indeed - I was contemplating
introducing traits and thought it might be better to keep things simple
and revisit until r000 is completed, but it probably won't be limiting
us in any way, and removes the unneeded mirror methods in `CmdqInner`.

The same pattern can probably also be applied on the receiving side, so
I'll try and do it there as well.