Re: [PATCH v2 2/4] gpu: nova-core: gsp: add sync and async command queue API to `Cmdq`

[John Hubbard <jhubbard@nvidia.com>]

On 2/26/26 7:50 AM, Eliot Courtney wrote:
> Add sync and async command queue API and the type infrastructure to know
> what reply is expected from each `CommandToGsp`.
> 
> Use a marker type `NoReply` which does not implement `MessageFromGsp` to
> mark async commands which don't expect a response.
> 
...
> +    /// Type of the reply expected from the GSP, or [`NoReply`] for async commands.

Hi Eliot,

The following does not need to hold up your patchset, but I want
to bring it up somewhere just to work through it.

The sync/async naming that GSP RM uses is a little bit "off". I
spent some time discussing it with them, and the problem is that
sync/async is a concept that is somewhat independent of whether
a reply is expected. Usually, sync means a blocking wait for a
response, which is not necessarily required in all case with
GSP RM calls.

The naming would be better here if it reflected simply that
a response is expected, or not. I don't have great names for
that, but "fire and forget" works well for what we have so
far called "async". So we could do create a convention in which
no annotation means that the API has a response that will come
back, and some abbreviated for of "fire and forget" or "one way"
added to the function name would mean that no response is
expected.

Again, I don't think this has to happen here, because we can
go through and rename later, no problem there. But when I saw
the sync/asynch and remembered the very recent discussion, I
figured I'd better post something about it.

And yes, I started us off in the wrong direction with the
IS_ASYNCH thing! haha

thanks,
-- 
John Hubbard

> +    type Reply;
> +
>       /// Error type returned by [`CommandToGsp::init`].
>       type InitError;
>   
> @@ -604,7 +611,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<M>(&mut self, bar: &Bar0, command: M) -> Result
>       where
>           M: CommandToGsp,
>           Error: From<M::InitError>,
> @@ -626,6 +633,51 @@ pub(crate) fn send_command<M>(&mut self, bar: &Bar0, command: M) -> Result
>           Ok(())
>       }
>   
> +    /// 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_sync_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(bar, command)?;
> +
> +        loop {
> +            match self.receive_msg::<M::Reply>(Delta::from_secs(10)) {
> +                Ok(reply) => break Ok(reply),
> +                Err(ERANGE) => continue,
> +                Err(e) => break Err(e),
> +            }
> +        }
> +    }
> +
> +    /// 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_async_command<M>(&mut self, bar: &Bar0, command: M) -> Result
> +    where
> +        M: CommandToGsp<Reply = NoReply>,
> +        Error: From<M::InitError>,
> +    {
> +        self.send_command(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/commands.rs b/drivers/gpu/nova-core/gsp/commands.rs
> index 74f875755e53..47ca31611927 100644
> --- a/drivers/gpu/nova-core/gsp/commands.rs
> +++ b/drivers/gpu/nova-core/gsp/commands.rs
> @@ -26,7 +26,8 @@
>               command_size,
>               Cmdq,
>               CommandToGsp,
> -            MessageFromGsp, //
> +            MessageFromGsp,
> +            NoReply, //
>           },
>           fw::{
>               commands::*,
> @@ -53,6 +54,7 @@ pub(crate) fn new(pdev: &'a pci::Device<device::Bound>) -> Self {
>   impl<'a> CommandToGsp for SetSystemInfo<'a> {
>       const FUNCTION: MsgFunction = MsgFunction::GspSetSystemInfo;
>       type Command = GspSetSystemInfo;
> +    type Reply = NoReply;
>       type InitError = Error;
>   
>       fn init(&self) -> impl Init<Self::Command, Self::InitError> {
> @@ -104,6 +106,7 @@ pub(crate) fn new() -> Self {
>   impl CommandToGsp for SetRegistry {
>       const FUNCTION: MsgFunction = MsgFunction::SetRegistry;
>       type Command = PackedRegistryTable;
> +    type Reply = NoReply;
>       type InitError = Infallible;
>   
>       fn init(&self) -> impl Init<Self::Command, Self::InitError> {
> @@ -183,6 +186,7 @@ pub(crate) fn wait_gsp_init_done(cmdq: &mut Cmdq) -> Result {
>   impl CommandToGsp for GetGspStaticInfo {
>       const FUNCTION: MsgFunction = MsgFunction::GetGspStaticInfo;
>       type Command = GspStaticConfigInfo;
> +    type Reply = GetGspStaticInfoReply;
>       type InitError = Infallible;
>   
>       fn init(&self) -> impl Init<Self::Command, Self::InitError> {
> @@ -236,15 +240,7 @@ pub(crate) fn gpu_name(&self) -> core::result::Result<&str, GpuNameError> {
>   
>   /// Send the [`GetGspInfo`] command and awaits for its reply.
>   pub(crate) fn get_gsp_info(cmdq: &mut Cmdq, bar: &Bar0) -> Result<GetGspStaticInfoReply> {
> -    cmdq.send_command(bar, GetGspStaticInfo)?;
> -
> -    loop {
> -        match cmdq.receive_msg::<GetGspStaticInfoReply>(Delta::from_secs(5)) {
> -            Ok(info) => return Ok(info),
> -            Err(ERANGE) => continue,
> -            Err(e) => return Err(e),
> -        }
> -    }
> +    cmdq.send_sync_command(bar, GetGspStaticInfo)
>   }
>   
>   /// The `ContinuationRecord` command.
> @@ -262,6 +258,7 @@ pub(crate) fn new(data: &'a [u8]) -> Self {
>   impl<'a> CommandToGsp for ContinuationRecord<'a> {
>       const FUNCTION: MsgFunction = MsgFunction::ContinuationRecord;
>       type Command = ();
> +    type Reply = NoReply;
>       type InitError = Infallible;
>   
>       fn init(&self) -> impl Init<Self::Command, Self::InitError> {
> @@ -356,6 +353,7 @@ pub(crate) enum SplitCommand<'a, C: CommandToGsp> {
>   impl<'a, C: CommandToGsp> CommandToGsp for SplitCommand<'a, C> {
>       const FUNCTION: MsgFunction = C::FUNCTION;
>       type Command = C::Command;
> +    type Reply = C::Reply;
>       type InitError = C::InitError;
>   
>       fn init(&self) -> impl Init<Self::Command, Self::InitError> {
> @@ -412,6 +410,7 @@ fn new(len: usize) -> Result<Self> {
>       impl CommandToGsp for TestPayload {
>           const FUNCTION: MsgFunction = MsgFunction::Nop;
>           type Command = ();
> +        type Reply = NoReply;
>           type InitError = Infallible;
>   
>           fn init(&self) -> impl Init<Self::Command, Self::InitError> {
>