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

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

On Mon Sep 28, 2026 at 3:40 PM JST, Eliot Courtney wrote:
> On Mon Sep 28, 2026 at 3:19 PM JST, Alexandre Courbot wrote:
>> 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@nvidia.com>
>>>> ---
>>>>  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.
>
> Yerp I think it can be. I also don't think it's bad to keep the trait
> around even after we remove r570. If we are passing in closures anyway
> to cmdq to do message layer stuff, that's kinda like an unnamed trait
> anyway. Thanks!

So it came to a surprise to me, but RPC will stay even after r570 is
removed (although it will only be used with vGPU IIUC) - that's actually
the main motivation for this design. So the trait will have more than
one implementor even after the switch to r000.