On Thu Feb 12, 2026 at 3:28 PM JST, Eliot Courtney wrote: > Splits large RPCs if necessary and sends the remaining parts using > continuation records. RPCs that do not need continuation records > continue to write directly into the command buffer. Ones that do write > into a staging buffer first, so there is one copy. > > Continuation record for receive is not necessary to support at the > moment because those replies do not need to be read and are currently > drained by retrying `receive_msg` on ERANGE.
Do they not need to be read at the moment, or is this a permanent thing? I hope it's the latter. ^_^; > > Signed-off-by: Eliot Courtney <[email protected]> > --- > drivers/gpu/nova-core/gsp/cmdq.rs | 47 ++++++++++++- > drivers/gpu/nova-core/gsp/commands.rs | 124 > ++++++++++++++++++++++++++++++++++ > drivers/gpu/nova-core/gsp/fw.rs | 5 ++ > 3 files changed, 173 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs > b/drivers/gpu/nova-core/gsp/cmdq.rs > index 3e9f88eec7cc..c24d813fc587 100644 > --- a/drivers/gpu/nova-core/gsp/cmdq.rs > +++ b/drivers/gpu/nova-core/gsp/cmdq.rs > @@ -29,6 +29,10 @@ > use crate::{ > driver::Bar0, > gsp::{ > + commands::{ > + ContinuationRecord, > + WrappingCommand, // > + }, > fw::{ > GspMsgElement, > MsgFunction, > @@ -524,7 +528,7 @@ fn command_size<M>(command: &M) -> usize > size_of::<M::Command>() + command.variable_payload_len() > } > > - /// Sends `command` to the GSP. > + /// Sends `command` to the GSP, without splitting it. > /// > /// # Errors > /// > @@ -533,13 +537,13 @@ fn command_size<M>(command: &M) -> usize > /// written to by its [`CommandToGsp::init_variable_payload`] method. > /// > /// Error codes returned by the command initializers are propagated > as-is. > - pub(crate) fn send_command<M>(&mut self, bar: &Bar0, command: M) -> > Result > + fn send_single_command<M>(&mut self, bar: &Bar0, command: &M) -> Result > where > M: CommandToGsp, > // This allows all error types, including `Infallible`, to be used > for `M::InitError`. > Error: From<M::InitError>, > { > - let command_size = Self::command_size(&command); > + let command_size = Self::command_size(command); > let dst = self.gsp_mem.allocate_command_with_timeout(command_size)?; > > // Extract area for the command itself. The GSP message header and > the command header > @@ -590,6 +594,43 @@ pub(crate) fn send_command<M>(&mut self, bar: &Bar0, > command: M) -> Result > Ok(()) > } > > + fn send_continuation_record(&mut self, bar: &Bar0, cont: > &ContinuationRecord<'_>) -> Result { > + self.send_single_command(bar, cont) > + } This one-liner method out of the blue is a bit surprising, and the first reflex is to try to inline it. Its reason to exist should be documented (you mention it in the cover letter, but that's not visible after this patch gets merged), but I'd lean towards removing it and just qualifying its unique caller: // Turbofish needed because the compiler cannot infer M here. self.send_single_command::<ContinuationRecord<'_>>(bar, &continuation)?; Fewer moving parts, and it makes it clear on the caller's side that a continuation record is just another command, like the truncated one sent before it. > + > + /// Sends `command` to the GSP. > + /// > + /// The command may be split into multiple messages if it is large. > + /// > + /// # Errors > + /// > + /// - `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. > + pub(crate) fn send_command<M>(&mut self, bar: &Bar0, command: M) -> > Result > + where > + M: CommandToGsp, > + Error: From<M::InitError>, > + { > + let msg_max_size = MSGQ_MSG_SIZE_MAX - size_of::<GspMsgElement>(); This could be a const. > + let mut wrapped = WrappingCommand::new(command, msg_max_size)?; The `max_size` argument is a bit unfortunate as it is a constant for non-testing purposes. Is there a way we can make testing work with the same limit as real-world use? That would also make our testing closer to reality, on top of making this argument unneeded. > + > + self.send_single_command(bar, &wrapped)?; > + > + while let Some(continuation) = wrapped.next_continuation_record() { > + dev_dbg!( > + &self.dev, > + "GSP RPC: send continuation: size=0x{:x}\n", > + Self::command_size(&continuation), > + ); > + self.send_continuation_record(bar, &continuation)?; > + } Btw, can we recover if a split message fails between two continuation records? I suspect the GSP will notice that the next message is not the expected continuation record and recover from there? > + > + Ok(()) > + } > + > /// Wait for a message to become available on the message queue. > /// > /// This works purely at the transport layer and does not interpret or > validate the message > diff --git a/drivers/gpu/nova-core/gsp/commands.rs > b/drivers/gpu/nova-core/gsp/commands.rs > index c8430a076269..99603880d56f 100644 > --- a/drivers/gpu/nova-core/gsp/commands.rs > +++ b/drivers/gpu/nova-core/gsp/commands.rs > @@ -242,3 +242,127 @@ pub(crate) fn get_gsp_info(cmdq: &mut Cmdq, bar: &Bar0) > -> Result<GetGspStaticIn > } > } > } > + > +#[derive(Zeroable)] > +pub(crate) struct Empty {} > + > +// SAFETY: `Empty` is a zero-sized type with no bytes, therefore it > trivially has no uninitialized > +// bytes. > +unsafe impl AsBytes for Empty {} > + > +// SAFETY: `Empty` is a zero-sized type with no bytes, therefore it > trivially has no uninitialized > +// bytes. > +unsafe impl FromBytes for Empty {} Since commit 209c70953aa (on master, but not drm-rust-next yet) you can just use `()` as the `Command` type - that will let you remove `Empty` entirely. You'll then need to invoke `pin_init::init_zeroed()` in `init`. > + > +/// The `ContinuationRecord` command. > +pub(crate) struct ContinuationRecord<'a> { > + data: &'a [u8], > +} > + > +impl<'a> ContinuationRecord<'a> { > + /// Creates a new `ContinuationRecord` command with the given data. > + pub(crate) fn new(data: &'a [u8]) -> Self { > + Self { data } > + } > +} > + > +impl<'a> CommandToGsp for ContinuationRecord<'a> { > + const FUNCTION: MsgFunction = MsgFunction::ContinuationRecord; > + type Command = Empty; > + type InitError = Infallible; > + > + fn init(&self) -> impl Init<Self::Command, Self::InitError> { > + Empty::init_zeroed() > + } > + > + fn variable_payload_len(&self) -> usize { > + self.data.len() > + } > + > + fn init_variable_payload( > + &self, > + dst: &mut SBufferIter<core::array::IntoIter<&mut [u8], 2>>, > + ) -> Result { > + dst.write_all(self.data) > + } > +} > + > +/// Wrapper that splits a command across continuation records if needed. > +pub(crate) struct WrappingCommand<C: CommandToGsp> { > + inner: C, > + offset: usize, > + max_size: usize, > + staging: KVVec<u8>, Since it is conditionally-used, `staging` should be an `Option` instead of assuming an empty state means it is unused. But hold on, I think we can do without any sort of conditional here. > +} This deserves more doccomments, including on its members. But I would also like to entertain a slightly different design. In this patch, `WrappingCommand` is always used, including for messages that don't need to be truncated. While the overhead is arguably negligible, this makes split messages pass as the norm, while they are a very rare exception. Also `WrappingCommand` now becomes both a command and a provider of other commands, which I find hard to wrap (haha) my head around, and forces you to pass the command by reference in `send_single_command` because you need to use it again afterwards. The name is also not very descriptive (why does it wrap?). How about this instead: `send_command` is the central path that all commands take, so it is the right place to check whether we need to use continuation records. If we don't, then we just send the command as-is, as we do today. If we need to split, we do it through a private method of `Cmdq` that consumes the command and returns a tuple `(SplitCommand<M>, ContinuationRecords)` `SplitCommand<M>` can be a wrapper around the original command, but that initializes the truncated part of its variable payload. `ContinuationRecords` implements `Iterator<Item = ContinuationRecord>`. `ContinuationRecord` works mostly like your current version, except it owns its data (yes, more allocations, but they're smaller so the allocator might actually appreciate). Since `SplitCommand` is only used when there is actually a split needed, `staging` does not need an empty state anymore, and the code of each type becomes simpler. By doing so, you also don't need to pass the command by reference in `send_single_command` anymore. > + > +impl<C: CommandToGsp> WrappingCommand<C> > +where > + Error: From<C::InitError>, This `where` is unneeded.
