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 <[email protected]> >>>> --- >>>> 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.
