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

["Alexandre Courbot" <acourbot@nvidia.com>]

On Mon Nov 2, 2026 at 10:27 PM JST, Eliot Courtney wrote:
> On Mon Nov 2, 2026 at 9:09 PM JST, Alexandre Courbot wrote:
>> From: John Hubbard <jhubbard@nvidia.com>
>>
>> `driver_write_area` and `driver_read_area` both branch on the relative
>> position of the read and write pointers to decide where the two slices
>> they return end. This works for msgq v0, where the pointers are indices
>> into the ring, but msgq v2 (used by the r000 firmware) keeps them in
>> BAR0 registers as free-running counters, so the branches and their
>> SAFETY arguments would have to be rewritten for it.
>>
>> Instead, compute the number of slots available to the driver and the
>> slot they start at, and derive the slices from that. The slot counts
>> come from one helper per queue, which are the only two places msgq v2
>> will need to touch.
>>
>> Use I/O projections to limit the amount of unsafe code to the bare
>> minimum.
>>
>> No functional change intended.
>>
>> Assisted-by: LLM
>> Signed-off-by: John Hubbard <jhubbard@nvidia.com>
>> Co-developed-by: Alexandre Courbot <acourbot@nvidia.com>
>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
>> ---
>>  drivers/gpu/nova-core/gsp/cmdq.rs | 130 +++++++++++++++++---------------------
>>  1 file changed, 58 insertions(+), 72 deletions(-)
>>
>> diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
>> index 90e26c4dcc28..33cb9174c1e6 100644
>> --- a/drivers/gpu/nova-core/gsp/cmdq.rs
>> +++ b/drivers/gpu/nova-core/gsp/cmdq.rs
>> @@ -19,8 +19,8 @@
>>          Io, //
>>      },
>>      new_mutex,
>> +    num::casts::FromSafeCast,
>>      prelude::*,
>> -    ptr,
>>      sync::{
>>          barrier::{
>>              dma_mb,
>> @@ -186,60 +186,58 @@ 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 = usize::from_safe_cast(self.cpuq_free_slots());
>> +        let w_slot = usize::from_safe_cast(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]);
>> +        // `w_slot` is below `MSGQ_NUM_PAGES` per the invariant of `cpu_write_ptr`, so the
>> +        // subtraction cannot underflow.
>> +        let before_wrap = avail.min(cv!(MSGQ_NUM_PAGES => usize) - w_slot);
>> +        // Per the line above, `before_wrap` is less or equal to `avail` so the subtraction cannot
>> +        // underflow.
>> +        let after_wrap = avail - before_wrap;
>>  
>> -        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)
>> -        };
>> +        // PANIC: `w_slot + before_wrap` is at most `MSGQ_NUM_PAGES` by construction of
>> +        // `before_wrap`.
>> +        let after = io_project!(self.mem, .cpuq.msgq.data[panic: w_slot..w_slot + before_wrap]);
>> +        // PANIC: `after_wrap` is at most `avail`, which is below `MSGQ_NUM_PAGES`.
>> +        let before = io_project!(self.mem, .cpuq.msgq.data[panic: ..after_wrap]);
>
> It looks like compared to v1, this kept the `after` and `before` names I
> suggested but changed the indices (e.g. in_after => before_wrap). It
> means that "before" and "after" are being used in two different ways in
> this function ("after wrapping" and "after write pointer"). What about
> picking one usage for the entire function? Either is fine imo. Same for
> the other spot (read side).

Indeed, this was a bit confusing. I have used `after_len/before_len` for
the length, and kept `after/before` for the slices, so things should be
more consistent now.