Re: [PATCH v3 3/5] gpu: nova-core: gsp: add reply/no-reply info to `CommandToGsp`

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

On Wed Mar 4, 2026 at 11:46 AM JST, Eliot Courtney wrote:
> Add type infrastructure to know what reply is expected from each
> `CommandToGsp`. Uses a marker type `NoReply` which does not implement
> `MessageFromGsp` to mark commands which don't expect a response.
>
> Update `send_command` to wait for a reply and add `send_command_no_wait`
> which sends a command that has no reply, without blocking.
>
> This prepares for adding locking to the queue.
>
> Tested-by: Zhi Wang <zhiw@nvidia.com>
> Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
> ---
>  drivers/gpu/nova-core/gsp/boot.rs              |  5 ++-
>  drivers/gpu/nova-core/gsp/cmdq.rs              | 55 +++++++++++++++++++++++++-
>  drivers/gpu/nova-core/gsp/cmdq/continuation.rs |  5 ++-
>  drivers/gpu/nova-core/gsp/commands.rs          | 16 +++-----
>  4 files changed, 67 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/nova-core/gsp/boot.rs b/drivers/gpu/nova-core/gsp/boot.rs
> index c56029f444cb..991eb5957e3d 100644
> --- a/drivers/gpu/nova-core/gsp/boot.rs
> +++ b/drivers/gpu/nova-core/gsp/boot.rs
> @@ -160,8 +160,9 @@ pub(crate) fn boot(
>          dma_write!(wpr_meta[0] = GspFwWprMeta::new(&gsp_fw, &fb_layout))?;
>  
>          self.cmdq
> -            .send_command(bar, commands::SetSystemInfo::new(pdev))?;
> -        self.cmdq.send_command(bar, commands::SetRegistry::new())?;
> +            .send_command_no_wait(bar, commands::SetSystemInfo::new(pdev))?;
> +        self.cmdq
> +            .send_command_no_wait(bar, commands::SetRegistry::new())?;
>  
>          gsp_falcon.reset(bar)?;
>          let libos_handle = self.libos.dma_handle();
> diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
> index 0192c85ddd75..7750f5792b21 100644
> --- a/drivers/gpu/nova-core/gsp/cmdq.rs
> +++ b/drivers/gpu/nova-core/gsp/cmdq.rs
> @@ -51,6 +51,10 @@
>      sbuffer::SBufferIter, //
>  };
>  
> +/// Marker type representing the absence of a reply for a command. This does not implement
> +/// `MessageFromGsp`.

This is giving either too much or not enough implementation detail. :)

Without knowing why `MessageFromGsp` is not implemented, this can be
confusing to users who will wonder why we give them this information.
I'd remove that sentence and instead say something like "commands using
this as their reply type are sent using `send_command_no_wait`" or
something like that.

> +pub(crate) struct NoReply;
> +
>  /// Trait implemented by types representing a command to send to the GSP.
>  ///
>  /// The main purpose of this trait is to provide [`Cmdq::send_command`] with the information it
> @@ -69,6 +73,10 @@ pub(crate) trait CommandToGsp {
>      /// Type generated by [`CommandToGsp::init`], to be written into the command queue buffer.
>      type Command: FromBytes + AsBytes;
>  
> +    /// Type of the reply expected from the GSP, or [`NoReply`] for commands that don't
> +    /// have a reply.
> +    type Reply;
> +
>      /// Error type returned by [`CommandToGsp::init`].
>      type InitError;
>  
> @@ -610,7 +618,7 @@ fn send_single_command<M>(&mut self, bar: &Bar0, command: M) -> Result
>      ///   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_command_internal<M>(&mut self, bar: &Bar0, command: M) -> Result
>      where
>          M: CommandToGsp,
>          Error: From<M::InitError>,
> @@ -630,6 +638,51 @@ pub(crate) fn send_command<M>(&mut self, bar: &Bar0, command: M) -> Result
>          }
>      }
>  
> +    /// Sends `command` to the GSP and waits for the reply.
> +    ///
> +    /// # Errors
> +    ///
> +    /// - `ETIMEDOUT` if space does not become available to send the command, or if the reply is
> +    ///   not received 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 and reply initializers are propagated as-is.
> +    pub(crate) fn send_command<M>(&mut self, bar: &Bar0, command: M) -> Result<M::Reply>
> +    where
> +        M: CommandToGsp,
> +        M::Reply: MessageFromGsp,
> +        Error: From<M::InitError>,
> +        Error: From<<M::Reply as MessageFromGsp>::InitError>,
> +    {
> +        self.send_command_internal(bar, command)?;
> +
> +        loop {
> +            match self.receive_msg::<M::Reply>(Self::RECEIVE_TIMEOUT) {
> +                Ok(reply) => break Ok(reply),
> +                Err(ERANGE) => continue,
> +                Err(e) => break Err(e),
> +            }
> +        }

There is an opportunity for factorize some more code here.

Notice how the other callers of `receive_msg` (`wait_gsp_init_done` and
`GspSequencer::run`) both use the same kind of loop, down to the same
error handling. We could move that loop logic here and do it in a single
place.

In the future, we will probably want to add handlers for
unexpected messages from the GSP and it will be easier if we receive all
messages from a single place.

This can be a separate patch from this one, but I think it makes sense
to have that in this series.

I expect the last patch to change a bit as a consequence of that - maybe
we will need a `receive_msg_loop` or something in `CmdqInner`.

> +    }
> +
> +    /// Sends `command` to the GSP without waiting for a reply.
> +    ///
> +    /// # 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_no_wait<M>(&mut self, bar: &Bar0, command: M) -> Result
> +    where
> +        M: CommandToGsp<Reply = NoReply>,
> +        Error: From<M::InitError>,
> +    {
> +        self.send_command_internal(bar, command)
> +    }
> +
>      /// 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/cmdq/continuation.rs b/drivers/gpu/nova-core/gsp/cmdq/continuation.rs
> index 637942917237..8a6bb8fa7e60 100644
> --- a/drivers/gpu/nova-core/gsp/cmdq/continuation.rs
> +++ b/drivers/gpu/nova-core/gsp/cmdq/continuation.rs
> @@ -6,7 +6,7 @@
>  
>  use kernel::prelude::*;
>  
> -use super::CommandToGsp;
> +use super::{CommandToGsp, NoReply};

Nit: let's follow the formatting convention for imports.