Re: [PATCH v2 4/5] gpu: nova-core: gsp: cmdq: compute the queue regions from a count and a slot

[Eliot Courtney]

On Mon Nov 2, 2026 at 9:09 PM JST, Alexandre Courbot wrote:
> From: John Hubbard <jhubbard@xxxxxxxxxx>
>
> `driver_write_area` and `driver_read_area` both branch on the relative
> position of the read and write pointers to decide where the two slices
> they return end. This works for msgq v0, where the pointers are indices
> into the ring, but msgq v2 (used by the r000 firmware) keeps them in
> BAR0 registers as free-running counters, so the branches and their
> SAFETY arguments would have to be rewritten for it.
>
> Instead, compute the number of slots available to the driver and the
> slot they start at, and derive the slices from that. The slot counts
> come from one helper per queue, which are the only two places msgq v2
> will need to touch.
>
> Use I/O projections to limit the amount of unsafe code to the bare
> minimum.
>
> No functional change intended.
>
> Assisted-by: LLM
> Signed-off-by: John Hubbard <jhubbard@xxxxxxxxxx>
> Co-developed-by: Alexandre Courbot <acourbot@xxxxxxxxxx>
> Signed-off-by: Alexandre Courbot <acourbot@xxxxxxxxxx>
> ---
>  drivers/gpu/nova-core/gsp/cmdq.rs | 130 +++++++++++++++++---------------------
>  1 file changed, 58 insertions(+), 72 deletions(-)
>
> diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
> index 90e26c4dcc28..33cb9174c1e6 100644
> --- a/drivers/gpu/nova-core/gsp/cmdq.rs
> +++ b/drivers/gpu/nova-core/gsp/cmdq.rs
> @@ -19,8 +19,8 @@
>          Io, //
>      },
>      new_mutex,
> +    num::casts::FromSafeCast,
>      prelude::*,
> -    ptr,
>      sync::{
>          barrier::{
>              dma_mb,
> @@ -186,60 +186,58 @@ fn new(dev: &'a device::Device<device::Bound>, bar: Bar0<'a>) -> Result<Self> {
>      /// As the message queue is a circular buffer, the region may be discontiguous in memory. In
>      /// that case the second slice will have a non-zero length.
>      fn driver_write_area(&mut self) -> (&mut [[u8; GSP_PAGE_SIZE]], &mut [[u8; GSP_PAGE_SIZE]]) {
> -        let tx = self.cpu_write_ptr();
> -        let rx = self.gsp_read_ptr();
> +        let avail = usize::from_safe_cast(self.cpuq_free_slots());
> +        let w_slot = usize::from_safe_cast(self.cpu_write_ptr());
>  
> -        // Pointer to the first entry of the CPU message queue.
> -        let data = ptr::project!(mut self.mem.as_mut_ptr(), .cpuq.msgq.data[build: 0]);
> +        // `w_slot` is below `MSGQ_NUM_PAGES` per the invariant of `cpu_write_ptr`, so the
> +        // subtraction cannot underflow.
> +        let before_wrap = avail.min(cv!(MSGQ_NUM_PAGES => usize) - w_slot);
> +        // Per the line above, `before_wrap` is less or equal to `avail` so the subtraction cannot
> +        // underflow.
> +        let after_wrap = avail - before_wrap;
>  
> -        let (tail_end, wrap_end) = if rx == 0 {
> -            // The write area is non-wrapping, and stops at the second-to-last entry of the command
> -            // queue (to leave the last one empty).
> -            (MSGQ_NUM_PAGES - 1, 0)
> -        } else if rx <= tx {
> -            // The write area wraps and continues until `rx - 1`.
> -            (MSGQ_NUM_PAGES, rx - 1)
> -        } else {
> -            // The write area doesn't wrap and stops at `rx - 1`.
> -            (rx - 1, 0)
> -        };
> +        // PANIC: `w_slot + before_wrap` is at most `MSGQ_NUM_PAGES` by construction of
> +        // `before_wrap`.
> +        let after = io_project!(self.mem, .cpuq.msgq.data[panic: w_slot..w_slot + before_wrap]);
> +        // PANIC: `after_wrap` is at most `avail`, which is below `MSGQ_NUM_PAGES`.
> +        let before = io_project!(self.mem, .cpuq.msgq.data[panic: ..after_wrap]);

It looks like compared to v1, this kept the `after` and `before` names I
suggested but changed the indices (e.g. in_after => before_wrap). It
means that "before" and "after" are being used in two different ways in
this function ("after wrapping" and "after write pointer"). What about
picking one usage for the entire function? Either is fine imo. Same for
the other spot (read side).

With that changed:

Reviewed-by: Eliot Courtney <ecourtney@xxxxxxxxxx>