Re: [PATCH v3 08/33] gpu: nova-core: gsp: compute the queue regions from a count and a slot
From: John Hubbard
Date: Wed Sep 23 2026 - 16:36:37 EST
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@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. :)
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@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?