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

[Gary Guo]

On Wed Sep 23, 2026 at 5:55 AM BST, Alexandre Courbot wrote:
> On Fri Sep 18, 2026 at 10:06 AM JST, John Hubbard wrote:
>> 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, and
>> split the ring at the start slot. 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.
>>
>> No functional changes.
>>
>> Assisted-by: LLM
>> Signed-off-by: John Hubbard <jhubbard@xxxxxxxxxx>
>> ---
>>  drivers/gpu/nova-core/gsp/cmdq.rs | 120 ++++++++++--------------------
>>  1 file changed, 41 insertions(+), 79 deletions(-)
>
> This looks like an improvement regardless of the r000 switch!
>
>>
>> diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
>> index 80e6e79c5f3c..a1c9b7cce255 100644
>> --- a/drivers/gpu/nova-core/gsp/cmdq.rs
>> +++ b/drivers/gpu/nova-core/gsp/cmdq.rs
>> @@ -262,107 +262,69 @@ fn new(dev: &'a device::Device<device::Bound>, bar: Bar0<'a>) -> Result<Self> {
>>          Ok(Self { mem: gsp_mem, bar })
>>      }
>>  
>> -    /// Returns the region of the CPU message queue that the driver is currently allowed to write
>> -    /// to.
>> +    /// Returns the region of the CPU message queue that the driver may write to.
>>      ///
>> -    /// 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.
>> +    /// The ring wraps, so the region comes as two slices, and the second is empty unless the
>> +    /// region crosses the end of the ring.
>
> There is a recurring pattern in this series to drive-by rewrite comments
> when there is no real need to do so. The new comment is not even
> marginally better as we lose the temporal nature ("currently") of the
> borrow. This creates churn restating the same thing using different
> words and disrupts the diff, so can we avoid doing that unless the patch
> actually changes what the comment describes?

That's a pretty common thing for LLM to do :)

>
>>      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 = num::u32_as_usize(self.free_slots());
>> +        let w_slot = num::u32_as_usize(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)
>> -        };
>> -
>>          // 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.
>> -        // - 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.
>> -        unsafe {
>> -            (
>> -                core::slice::from_raw_parts_mut(
>> -                    data.add(num::u32_as_usize(tx)),
>> -                    num::u32_as_usize(tail_end - tx),
>> -                ),
>> -                core::slice::from_raw_parts_mut(data, num::u32_as_usize(wrap_end)),
>> -            )
>> -        }
>> +        // - `data` points to the `MSGQ_NUM_PAGES` initialized entries of the CPU message queue.
>> +        // - The returned slices cover the `avail` free slots from the write pointer on, which the
>> +        //   GSP does not read until `advance_cpu_write_ptr` publishes them.
>> +        // - `split_at_mut` gives two non-overlapping halves, and the `&mut self` borrow lasts as
>> +        //   long as the returned slices, so that no other call hands out the same region while
>> +        //   they live.
>> +        let data =
>> +            unsafe { core::slice::from_raw_parts_mut(data, num::u32_as_usize(MSGQ_NUM_PAGES)) };
>> +        let (before_w, after_w) = data.split_at_mut(w_slot);
>
> This creates a reference over the whole ring, including the parts owned
> by the GSP, which breaks the `Coherent` safety contract that the device
> must not be able to read or write to a live slice. So we'll need to call
> `from_raw_parts_mut` twice, with the correct sizes, instead of
> splitting.
>
> (also `split_at_mut` is panicking and should have a `PANIC:` comment
> justifying why it cannot, but once the point above is addressed that
> call will go away).
>
> I wanted to try it locally and ended up with something that seems to
> work, so let me share it to save some time:
>
>     fn driver_write_area(&mut self) -> (&mut [[u8; GSP_PAGE_SIZE]], &mut [[u8; GSP_PAGE_SIZE]]) {
>         let avail = self.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 in_after = avail.min(MSGQ_NUM_PAGES - w_slot);
>         let in_before = avail - in_after;
>
>         // SAFETY:
>         // - `data` was created from a valid pointer of `MSGQ_NUM_PAGES` entries.
>         // - The `in_after` entries after `w_slot` belong to the `avail` entries that the driver is
>         //   currently allowed to write.
>         // - The `in_before` first entries belong to the `avail` entries that the driver is
>         //   currently allowed to write.
>         // - The slices do not overlap.
>         unsafe {
>             (
>                 core::slice::from_raw_parts_mut(
>                     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(in_before)),
>             )
>         }
>     }
>
> It has turned out quite short, which I like! I also opted to work with
> the original `u32` until the very end, as it results in less conversions
> overall.

Possibly take some thing from the old projection syntax rework series?

https://lore.kernel.org/rust-for-linux/20260415-projection-syntax-rework-v1-4-450723cb3727@xxxxxxxxxxx/

Best,
Gary