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

[Alexandre Courbot]

On Mon Nov 2, 2026 at 2:46 PM JST, Eliot Courtney wrote:
> On Fri Oct 30, 2026 at 4:41 PM JST, Alexandre Courbot wrote:
>> From: John Hubbard <jhubbard@xxxxxxxxxx>
>>
>> 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@xxxxxxxxxx>
>> Co-developed-by: Alexandre Courbot <acourbot@xxxxxxxxxx>
>> Signed-off-by: Alexandre Courbot <acourbot@xxxxxxxxxx>
>> ---
>
> Do you reckon you could tighten up/rewrite this commit message a bit?
> For me it's a bit hard to follow.

Absolutely. And I'll also simplify a couple of others as well.

>
>>  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@xxxxxxxxxxx/

Yup, I had forgotten about that. Done, thanks for the reminder.

>
>>              )
>>          }
>>      }
>>  
>> -    /// 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.

Do you mean by merging e.g. `cpu_write_ptr` and `cpuq_free_slots` into a
single method? These two are called in separate contexts as well, so in
these contexts we would have to ignore half of the returned value (that
we might have performed I/O to obtain). So I'm not sure that it would
really help make the code more readable here.

> Reviewed-by: Eliot Courtney <ecourtney@xxxxxxxxxx>

I won't take it this round as the next revision will change
substantively, but thanks for the review!