On Wed Feb 18, 2026 at 12:13 PM JST, Alexandre Courbot wrote:
>> + /// Allocates a region on the command queue that is large enough to
>> send a command of `size`
>> + /// bytes, waiting for space to become available.
>> + ///
>> + /// This returns a [`GspCommand`] ready to be written to by the caller.
>> + ///
>> + /// # Errors
>> + ///
>> + /// - `ETIMEDOUT` if space does not become available within the timeout.
>> + /// - `EIO` if the command header is not properly aligned.
>> + fn allocate_command_with_timeout(&mut self, size: usize) ->
>> Result<GspCommand<'_>> {
>
> Should the timeout be an argument? That way we can simply add it to
> `allocate_command`, and invoke it with `Delta::ZERO` whenever we don't
> want to wait. This is more explicit at the call site, removes the
> need to have two methods, and removes the redundant size check from
> `allocate_command` which is now done by this `read_poll_timeout`.
Good idea, thanks.
>> + fn command_size<M>(command: &M) -> usize
>
> Shouldn't this be a member function of `CommandToGsp`? Please add some
> basic documentation for it as well. As a general rule, all methods, even
> basic ones, should have at least one line of doccomment.
I thought about this, but adding a function to CommandToGsp with
a default implementation seems odd to me, because implementors of that
trait could override it, which does not really make sense. We have
command size defined as the size of the struct plus the variable payload
size. Adding a function to CommandToGsp would give two methods to
calculate the command size which could differ. So, seems weird to me.
An alternative would be to make it a free standing function. This would
let it be used by WrappingCommand later as well. WDYT?
Will make sure all functions have doccomment from now on, thanks.
>
>> + where
>> + M: CommandToGsp,
>> + {
>> + size_of::<M::Command>() + command.variable_payload_len()
>> + }
>> +
>> /// Sends `command` to the GSP.
>> ///
>> /// # Errors
>> ///
>> - /// - `EAGAIN` if there was not enough space in the command queue to
>> send the command.
>> + /// - `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.
>> ///
>> @@ -495,8 +531,8 @@ pub(crate) fn send_command<M>(&mut self, bar: &Bar0,
>> command: M) -> Result
>> // This allows all error types, including `Infallible`, to be used
>> for `M::InitError`.
>> Error: From<M::InitError>,
>> {
>> - let command_size = size_of::<M::Command>() +
>> command.variable_payload_len();
>> - let dst = self.gsp_mem.allocate_command(command_size)?;
>> + let command_size = Self::command_size(&command);
>
> The addition of `command_size` looks like an unrelated change - it is
> not really leveraged until patch 6 (although it is still valuable on its
> own). Can you move it to its own patch for clarity?
Will do.
