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

["Eliot Courtney" <ecourtney@nvidia.com>]

On Fri Oct 30, 2026 at 4:41 PM JST, Alexandre Courbot wrote:
> From: John Hubbard <jhubbard@nvidia.com>
>
> The r000 firmware uses msgq v2, the queue layout that keeps the queue
> pointers in BAR0 registers as counts that do not wrap at the ring size.
> The r570 firmware's layout keeps the pointers in shared memory as
> indices into the ring. The two layouts differ in where a pointer is
> read and in how the size of the region that the driver may write, and
> of the region that it may read, follows from a queue's write pointer
> and read pointer. Splitting a region across the end of the ring is the
> same in both, and whether a region wraps follows from the order of the
> two pointers.
>
> The functions for the writable region and for the readable region each
> branched on the order of the write pointer and the read pointer. Each
> branch chose where the two slices ended, and the function then built
> the slices with open-coded pointer arithmetic. The SAFETY comments
> argued the slice bounds branch by branch, so the switch to msgq v2
> would have had to rewrite the branches and the argument along with the
> pointer rules.
>
> Compute the number of slots in a region and its start slot once. The
> first slice ends at the end of the region or at the end of the ring,
> whichever comes first, and the second slice carries the rest.
>
> The number of slots comes from one helper per queue, which are the only
> two places the msgq v2 switch needs to change. The two slices are built
> with separate from_raw_parts calls over the slots the driver currently
> owns, rather than by splitting a slice over the whole ring: a reference
> spanning slots the GSP may be accessing would break the contract of
> Coherent.
>
> No functional changes.
>
> Assisted-by: LLM
> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
> Co-developed-by: Alexandre Courbot <acourbot@nvidia.com>
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---

Do you reckon you could tighten up/rewrite this commit message a bit?
For me it's a bit hard to follow. 

>  drivers/gpu/nova-core/gsp/cmdq.rs | 96 +++++++++++++++++++--------------------
>  1 file changed, 47 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
> index a271114fb54c..7350074a9f96 100644
> --- a/drivers/gpu/nova-core/gsp/cmdq.rs
> +++ b/drivers/gpu/nova-core/gsp/cmdq.rs
> @@ -186,60 +186,64 @@ 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 = self.cpuq_free_slots();
> +        let w_slot = 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]);
>  
> -        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)
> -        };
> +        // `w_slot` is below `MSGQ_NUM_PAGES` per the invariant of `cpu_write_ptr`, so the
> +        // subtraction cannot underflow.
> +        let in_after = avail.min(MSGQ_NUM_PAGES - w_slot);
> +        let in_before = avail - in_after;
>  
>          // SAFETY:
> -        // - `data` was created from a valid pointer, and `rx` and `tx` are in the
> -        //   `0..MSGQ_NUM_PAGES` range per the invariants of `cpu_write_ptr` and `gsp_read_ptr`,
> -        //   thus the created slices are valid.
> -        // - The area starting at `tx` and ending at `rx - 2` modulo `MSGQ_NUM_PAGES`,
> -        //   inclusive, belongs to the driver for writing and is not accessed concurrently by
> -        //   the GSP.
> +        // - `data` was created from a valid pointer to `MSGQ_NUM_PAGES` entries.
> +        // - The `in_after` entries starting at `w_slot` belong to the `avail` entries that the
> +        //   driver is currently allowed to write, and so do the `in_before` first entries.
>          // - The caller holds a reference to `self` for as long as the returned slices are live,
>          //   meaning the CPU write pointer cannot be advanced and thus that the returned area
>          //   remains exclusive to the CPU for the duration of the slices.
> -        // - The created slices point to non-overlapping sub-ranges of `data` in all
> -        //   branches (in the `rx <= tx` case, the second slice ends at `rx - 1` which is strictly
> -        //   less than `tx` where the first slice starts; in the other cases the second slice is
> -        //   empty), so creating two `&mut` references from them does not violate aliasing rules.
> +        // - The slices do not overlap: `avail` is at most `MSGQ_NUM_PAGES - 1`, so `in_before`
> +        //   is strictly less than `w_slot` whenever it is non-zero.
>          unsafe {
>              (
>                  core::slice::from_raw_parts_mut(
> -                    data.add(num::u32_as_usize(tx)),
> -                    num::u32_as_usize(tail_end - tx),
> +                    data.add(num::u32_as_usize(w_slot)),
> +                    num::u32_as_usize(in_after),
>                  ),
> -                core::slice::from_raw_parts_mut(data, num::u32_as_usize(wrap_end)),
> +                core::slice::from_raw_parts_mut(data, num::u32_as_usize(in_before)),

Can we use io_project here? then unsafe as_mut() because they are disjoint
e.g.
```
let after = io_project!(self.mem, .cpuq.msgq.data[panic: w_slot..w_slot + in_after]);
let before = io_project!(self.mem, .cpuq.msgq.data[panic: ..in_before]);
unsafe { (after.as_mut(), before.as_mut()) }
```

Same for the other spot. Gary also suggested this on v3 [1].

[1]: https://lore.kernel.org/all/DLMUKFL5CAZ5.2ICMKG94JXP52@garyguo.net/

>              )
>          }
>      }
>  
> -    /// Returns the size of the region of the CPU message queue that the driver is currently allowed
> -    /// to write to, in bytes.
> -    fn driver_write_area_size(&self) -> usize {
> +    /// Returns the number of slots of the CPU message queue that the driver is currently allowed
> +    /// to write to.
> +    fn cpuq_free_slots(&self) -> u32 {
>          let tx = self.cpu_write_ptr();
>          let rx = self.gsp_read_ptr();
>  
>          // `rx` and `tx` are both in `0..MSGQ_NUM_PAGES` per the invariants of `gsp_read_ptr` and
>          // `cpu_write_ptr`. The minimum value case is where `rx == 0` and `tx == MSGQ_NUM_PAGES -
>          // 1`, which gives `0 + MSGQ_NUM_PAGES - (MSGQ_NUM_PAGES - 1) - 1 == 0`.
> -        let slots = (rx + MSGQ_NUM_PAGES - tx - 1) % MSGQ_NUM_PAGES;
> -        num::u32_as_usize(slots) * GSP_PAGE_SIZE
> +        (rx + MSGQ_NUM_PAGES - tx - 1) % MSGQ_NUM_PAGES
> +    }

optional nit: it may be nicer (for r000 as well) to have the helper e.g.
cpuq_free_slots return (u32, u32) with the starting slots and the # of
slots.

Reviewed-by: Eliot Courtney <ecourtney@nvidia.com>