On Thu Feb 26, 2026 at 2:50 PM GMT, Eliot Courtney wrote:
> Wrap `Cmdq`'s mutable state in a new struct `CmdqInner` and wrap that in
> a Mutex. This lets `Cmdq` methods take &self instead of &mut self, which
> lets required commands be sent e.g. while unloading the driver.
>
> The mutex is held over both send and receive in `send_sync_command` to
> make sure that it doesn't get the reply of some other command that could
> have been sent just beforehand.
>
> Reviewed-by: Zhi Wang <[email protected]>
> Signed-off-by: Eliot Courtney <[email protected]>
> ---
> drivers/gpu/nova-core/gsp/boot.rs | 8 +-
> drivers/gpu/nova-core/gsp/cmdq.rs | 266
> ++++++++++++++++++---------------
> drivers/gpu/nova-core/gsp/commands.rs | 4 +-
> drivers/gpu/nova-core/gsp/sequencer.rs | 2 +-
> 4 files changed, 153 insertions(+), 127 deletions(-)
>
> diff --git a/drivers/gpu/nova-core/gsp/boot.rs
> b/drivers/gpu/nova-core/gsp/boot.rs
> index 55899eba75db..d12ad1bd2cd8 100644
> --- a/drivers/gpu/nova-core/gsp/boot.rs
> +++ b/drivers/gpu/nova-core/gsp/boot.rs
> @@ -128,7 +128,7 @@ fn run_fwsec_frts(
> ///
> /// Upon return, the GSP is up and running, and its runtime object given
> as return value.
> pub(crate) fn boot(
> - mut self: Pin<&mut Self>,
> + self: Pin<&mut Self>,
> pdev: &pci::Device<device::Bound>,
> bar: &Bar0,
> chipset: Chipset,
> @@ -214,13 +214,13 @@ pub(crate) fn boot(
> dev: pdev.as_ref().into(),
> bar,
> };
> - GspSequencer::run(&mut self.cmdq, seq_params)?;
> + GspSequencer::run(&self.cmdq, seq_params)?;
>
> // Wait until GSP is fully initialized.
> - commands::wait_gsp_init_done(&mut self.cmdq)?;
> + commands::wait_gsp_init_done(&self.cmdq)?;
>
> // Obtain and display basic GPU information.
> - let info = commands::get_gsp_info(&mut self.cmdq, bar)?;
> + let info = commands::get_gsp_info(&self.cmdq, bar)?;
> match info.gpu_name() {
> Ok(name) => dev_info!(pdev, "GPU name: {}\n", name),
> Err(e) => dev_warn!(pdev, "GPU name unavailable: {:?}\n", e),
> diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs
> b/drivers/gpu/nova-core/gsp/cmdq.rs
> index 6bb1decd2af5..5010587c96f9 100644
> --- a/drivers/gpu/nova-core/gsp/cmdq.rs
> +++ b/drivers/gpu/nova-core/gsp/cmdq.rs
> @@ -16,8 +16,12 @@
> },
> dma_write,
> io::poll::read_poll_timeout,
> + new_mutex,
> prelude::*,
> - sync::aref::ARef,
> + sync::{
> + aref::ARef,
> + Mutex, //
> + },
> time::Delta,
> transmute::{
> AsBytes,
> @@ -54,8 +58,8 @@
>
> /// 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
> -/// needs to send a given command.
> +/// The main purpose of this trait is to provide [`Cmdq`] with the
> information it needs to send
> +/// a given command.
> ///
> /// [`CommandToGsp::init`] in particular is responsible for initializing the
> command directly
> /// into the space reserved for it in the command queue buffer.
> @@ -470,66 +474,15 @@ pub(crate) fn command_size<M>(command: &M) -> usize
> size_of::<M::Command>() + command.variable_payload_len()
> }
>
> -/// GSP command queue.
> -///
> -/// Provides the ability to send commands and receive messages from the GSP
> using a shared memory
> -/// area.
> -#[pin_data]
> -pub(crate) struct Cmdq {
> - /// Device this command queue belongs to.
> - dev: ARef<device::Device>,
> +/// Inner mutex protected state of [`Cmdq`].
> +struct CmdqInner {
> /// Current command sequence number.
> seq: u32,
> /// Memory area shared with the GSP for communicating commands and
> messages.
> gsp_mem: DmaGspMem,
> }
>
> -impl Cmdq {
> - /// Offset of the data after the PTEs.
> - const POST_PTE_OFFSET: usize = core::mem::offset_of!(GspMem, cpuq);
> -
> - /// Offset of command queue ring buffer.
> - pub(crate) const CMDQ_OFFSET: usize = core::mem::offset_of!(GspMem, cpuq)
> - + core::mem::offset_of!(Msgq, msgq)
> - - Self::POST_PTE_OFFSET;
> -
> - /// Offset of message queue ring buffer.
> - pub(crate) const STATQ_OFFSET: usize = core::mem::offset_of!(GspMem,
> gspq)
> - + core::mem::offset_of!(Msgq, msgq)
> - - Self::POST_PTE_OFFSET;
> -
> - /// Number of page table entries for the GSP shared region.
> - pub(crate) const NUM_PTES: usize = size_of::<GspMem>() >> GSP_PAGE_SHIFT;
> -
> - /// Creates a new command queue for `dev`.
> - pub(crate) fn new(dev: &device::Device<device::Bound>) -> impl
> PinInit<Self, Error> + '_ {
> - try_pin_init!(Self {
> - gsp_mem: DmaGspMem::new(dev)?,
> - dev: dev.into(),
> - seq: 0,
> - })
> - }
> -
> - /// Computes the checksum for the message pointed to by `it`.
> - ///
> - /// A message is made of several parts, so `it` is an iterator over byte
> slices representing
> - /// these parts.
> - fn calculate_checksum<T: Iterator<Item = u8>>(it: T) -> u32 {
> - let sum64 = it
> - .enumerate()
> - .map(|(idx, byte)| (((idx % 8) * 8) as u32, byte))
> - .fold(0, |acc, (rol, byte)| acc ^
> u64::from(byte).rotate_left(rol));
> -
> - ((sum64 >> 32) as u32) ^ (sum64 as u32)
> - }
> -
> - /// Notifies the GSP that we have updated the command queue pointers.
> - fn notify_gsp(bar: &Bar0) {
> - regs::NV_PGSP_QUEUE_HEAD::default()
> - .set_address(0)
> - .write(bar);
> - }
> -
> +impl CmdqInner {
> /// Sends `command` to the GSP, without splitting it.
> ///
> /// # Errors
> @@ -540,7 +493,7 @@ fn notify_gsp(bar: &Bar0) {
> /// written to by its [`CommandToGsp::init_variable_payload`] method.
> ///
> /// Error codes returned by the command initializers are propagated
> as-is.
> - fn send_single_command<M>(&mut self, bar: &Bar0, command: M) -> Result
> + fn send_single_command<M>(&mut self, dev: &device::Device, bar: &Bar0,
> command: M) -> Result
Any reason that the `dev` is passed in everything instead of just have it be
part of `CmdqInner`?
It appears that the `Cmdq` methods don't actually use it apart from passing it
to `CmdqInner`.
Best,
Gary
> where
> M: CommandToGsp,
> // This allows all error types, including `Infallible`, to be used
> for `M::InitError`.
> @@ -583,7 +536,7 @@ fn send_single_command<M>(&mut self, bar: &Bar0, command:
> M) -> Result
> ])));
>
> dev_dbg!(
> - &self.dev,
> + dev,
> "GSP RPC: send: seq# {}, function={:?}, length=0x{:x}\n",
> self.seq,
> M::FUNCTION,
