On Wed Feb 18, 2026 at 4:16 PM JST, Alexandre Courbot wrote:
>> + /// 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.
Yeah good point, will do.
>
>> +
>> + 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?
IIUC neither openrm or nouveau can recover from a failure during sending
continuation records. What failure mode do you see happening that we
could recover from?
>> +#[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`.
Great!
>> +
>> +/// 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.
Yeah you are right. I had used empty here since it makes
`next_continuation_record` here more consistent in that it doesn't need
to check for None then for length anyway, but it's special casing in
`init_variable_payload` in a very optiony way regardless.
I reckon using Option we could put `offset` in there as well in a tuple,
since if we want to use Option to avoid the special case of empty
`staging`, we might as well use it to avoid having a conceptually
useless `offset` field too.
>
>> +}
>
> 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).
Is it really the case that more, smaller allocations is better if we are
using KVVec rather than KVec? It feels like it would have
overhead/fragmentation to me. Eager to learn if this is wrong though
>
> 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.
I considered putting the continuation record logic inside cmdq, but I
feel it's nicer to keep it out of the core command queue logic as much
as possible, because it feels a bit noisy to have in the cmdq which
hopefully can just concentrate on sending messages, not how those
messages get made or how/when they get split.
I agree that it is a bit odd to have a command that also provides extra
commands. By the way I considered doing an Iterator thing for
WrappingCommand, but felt like it would introduce more complexity
than it was worth and make it more confusing on the command that also
produces commands front.
Another benefit to pulling the logic out of command queue is that it
makes it a lot easier to test AFAICT. If we want to test the splitting,
if the logic is a private method in the command queue that feels like it
might end up more as an integration test.
But if we just want to avoid having a command which also produces
commands, what if we created a separate type to hold the split state /
decision and it would return a SplitCommand plus either an iterator or
the iterator-like `next_continuation_record`, while being testable
separately. WDYT?
>
>> +
>> +impl<C: CommandToGsp> WrappingCommand<C>
>> +where
>> + Error: From<C::InitError>,
>
> This `where` is unneeded.
