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

[John Hubbard <jhubbard@nvidia.com>]

On 9/23/26 6:20 AM, Alexandre Courbot wrote:
> 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@nvidia.com>
>>>> ---
>>>>   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. :)

So true! :)

I'll take action to avoid this sort of thing in the future, sorry
about that.


thanks,
-- 
John Hubbard

> 
>>
>>>
>>>>       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@garyguo.net/
> 
> 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?