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

[Alexandre Courbot]

On Wed Sep 23, 2026 at 8:30 PM JST, Gary Guo wrote:
> 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 :)

We're lucky that one highlight of this revision was to remove AI comment
churn. :)

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

Oh yes, I forgot about this patch. Do you mean using `ptr::project` to
create the final sub-slices, or am I missing something else?