[PATCH 0/2] gpu: nova-core: gsp: fix misc cmdq issues

[PATCH 0/2] gpu: nova-core: gsp: fix misc cmdq issues

[Eliot Courtney]

Fix two cmdq issues.

This is based on drm-rust-next.

Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
---
Eliot Courtney (2):
      gpu: nova-core: gsp: cmdq: wrap the sequence number on overflow
      gpu: nova-core: gsp: cmdq: increase MSGQ_NUM_PAGES to 64

 drivers/gpu/nova-core/gsp/cmdq.rs | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)
---
base-commit: 7ecd9ee29f7bfbe9a293952fbc83c3f8bd7205dd
change-id: 20261109-msgq-64-pages-bab32c03d627

Best regards,
--  
Eliot Courtney <ecourtney@nvidia.com>

[PATCH 1/2] gpu: nova-core: gsp: cmdq: wrap the sequence number on overflow

[Eliot Courtney]

The sequence number can (eventually) overflow, in which case we should
just wrap it around.

Fixes: 75f6b1de8133 ("gpu: nova-core: gsp: Add GSP command queue bindings and handling")
Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
---
 drivers/gpu/nova-core/gsp/cmdq.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
index 5f3de456042e..33074bb53e8c 100644
--- a/drivers/gpu/nova-core/gsp/cmdq.rs
+++ b/drivers/gpu/nova-core/gsp/cmdq.rs
@@ -578,7 +578,7 @@ fn send_command_element<E: CommandElement>(&mut self, command: E) -> Result<u32>
 
         // All set - update the write pointer and inform the GSP of the new command.
         let elem_count = dst.header.element_count();
-        self.seq += 1;
+        self.seq = self.seq.wrapping_add(1);
         self.gsp_mem.advance_cpu_write_ptr(elem_count);
 
         Ok(seq)

-- 
2.56.0

[PATCH 2/2] gpu: nova-core: gsp: cmdq: increase MSGQ_NUM_PAGES to 64

[Eliot Courtney]

In the msgq v2 protocol, which the r000 firmware uses, the queue
pointers are kept in u32 registers and increase monotonically until they
wrap at 2^32.

`MSGQ_NUM_PAGES` is currently 63 and does not divide 2^32. This means
that on msgq v2, the slot can jump backwards when the counters wrap.
For example:

    (2^32 - 2) % 63 == 2
    (2^32 - 1) % 63 == 3
             0 % 63 == 0    (slot 0 instead of slot 4)

The writer can then overwrite slots before the reader has read them.

Increase `MSGQ_NUM_PAGES` to 64 and check at compile time that
`MSGQ_NUM_PAGES` divides 2^32.

Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
---
 drivers/gpu/nova-core/gsp/cmdq.rs | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
index 33074bb53e8c..240ab42db557 100644
--- a/drivers/gpu/nova-core/gsp/cmdq.rs
+++ b/drivers/gpu/nova-core/gsp/cmdq.rs
@@ -84,7 +84,15 @@ trait CommandElement {
 }
 
 /// Number of GSP pages making the [`Msgq`].
-pub(crate) const MSGQ_NUM_PAGES: u32 = 0x3f;
+pub(crate) const MSGQ_NUM_PAGES: u32 = 64;
+
+// From version 2 of the msgq protocol, the queue pointers increase
+// monotonically in `u32` counters until they wrap. If `MSGQ_NUM_PAGES` does not
+// divide `1 << 32`, then the slot, `counter % MSGQ_NUM_PAGES`, can jump
+// backwards when wrapping, which could overwrite slots that still need to be
+// read. So, require that the last counter value, `u32::MAX`, maps to the last
+// slot.
+static_assert!(u32::MAX % MSGQ_NUM_PAGES == MSGQ_NUM_PAGES - 1);
 
 /// Circular buffer of a [`Msgq`].
 ///

-- 
2.56.0

Re: [PATCH 1/2] gpu: nova-core: gsp: cmdq: wrap the sequence number on overflow

[Alexandre Courbot]

On Wed Nov 11, 2026 at 10:37 AM JST, Eliot Courtney wrote:
> The sequence number can (eventually) overflow, in which case we should
> just wrap it around.
>
> Fixes: 75f6b1de8133 ("gpu: nova-core: gsp: Add GSP command queue bindings and handling")
> Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>

Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>

Re: [PATCH 2/2] gpu: nova-core: gsp: cmdq: increase MSGQ_NUM_PAGES to 64

[Alexandre Courbot]

On Wed Nov 11, 2026 at 10:37 AM JST, Eliot Courtney wrote:
> In the msgq v2 protocol, which the r000 firmware uses, the queue
> pointers are kept in u32 registers and increase monotonically until they
> wrap at 2^32.
>
> `MSGQ_NUM_PAGES` is currently 63 and does not divide 2^32. This means
> that on msgq v2, the slot can jump backwards when the counters wrap.
> For example:
>
>     (2^32 - 2) % 63 == 2
>     (2^32 - 1) % 63 == 3
>              0 % 63 == 0    (slot 0 instead of slot 4)
>
> The writer can then overwrite slots before the reader has read them.
>
> Increase `MSGQ_NUM_PAGES` to 64 and check at compile time that
> `MSGQ_NUM_PAGES` divides 2^32.

Have we tested that this was also working on msgq v0?

>
> Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
> ---
>  drivers/gpu/nova-core/gsp/cmdq.rs | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
> index 33074bb53e8c..240ab42db557 100644
> --- a/drivers/gpu/nova-core/gsp/cmdq.rs
> +++ b/drivers/gpu/nova-core/gsp/cmdq.rs
> @@ -84,7 +84,15 @@ trait CommandElement {
>  }
>  
>  /// Number of GSP pages making the [`Msgq`].
> -pub(crate) const MSGQ_NUM_PAGES: u32 = 0x3f;
> +pub(crate) const MSGQ_NUM_PAGES: u32 = 64;
> +
> +// From version 2 of the msgq protocol, the queue pointers increase
> +// monotonically in `u32` counters until they wrap. If `MSGQ_NUM_PAGES` does not
> +// divide `1 << 32`, then the slot, `counter % MSGQ_NUM_PAGES`, can jump
> +// backwards when wrapping, which could overwrite slots that still need to be
> +// read. So, require that the last counter value, `u32::MAX`, maps to the last
> +// slot.

That's a pretty convoluted way to say the number of slots must be a
power of 2 representable as a u32. :)

> +static_assert!(u32::MAX % MSGQ_NUM_PAGES == MSGQ_NUM_PAGES - 1);

Same here, can't we use `MSGQ_NUM_PAGES.is_power_of_two()`?

Re: [PATCH 2/2] gpu: nova-core: gsp: cmdq: increase MSGQ_NUM_PAGES to 64

[Eliot Courtney]

On Wed Nov 11, 2026 at 11:49 PM JST, Alexandre Courbot wrote:
> On Wed Nov 11, 2026 at 10:37 AM JST, Eliot Courtney wrote:
>> In the msgq v2 protocol, which the r000 firmware uses, the queue
>> pointers are kept in u32 registers and increase monotonically until they
>> wrap at 2^32.
>>
>> `MSGQ_NUM_PAGES` is currently 63 and does not divide 2^32. This means
>> that on msgq v2, the slot can jump backwards when the counters wrap.
>> For example:
>>
>>     (2^32 - 2) % 63 == 2
>>     (2^32 - 1) % 63 == 3
>>              0 % 63 == 0    (slot 0 instead of slot 4)
>>
>> The writer can then overwrite slots before the reader has read them.
>>
>> Increase `MSGQ_NUM_PAGES` to 64 and check at compile time that
>> `MSGQ_NUM_PAGES` divides 2^32.
>
> Have we tested that this was also working on msgq v0?

Yerp I tested boot on turing, ampere, ada, blackwell.

>
>>
>> Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
>> ---
>>  drivers/gpu/nova-core/gsp/cmdq.rs | 10 +++++++++-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
>> index 33074bb53e8c..240ab42db557 100644
>> --- a/drivers/gpu/nova-core/gsp/cmdq.rs
>> +++ b/drivers/gpu/nova-core/gsp/cmdq.rs
>> @@ -84,7 +84,15 @@ trait CommandElement {
>>  }
>>  
>>  /// Number of GSP pages making the [`Msgq`].
>> -pub(crate) const MSGQ_NUM_PAGES: u32 = 0x3f;
>> +pub(crate) const MSGQ_NUM_PAGES: u32 = 64;
>> +
>> +// From version 2 of the msgq protocol, the queue pointers increase
>> +// monotonically in `u32` counters until they wrap. If `MSGQ_NUM_PAGES` does not
>> +// divide `1 << 32`, then the slot, `counter % MSGQ_NUM_PAGES`, can jump
>> +// backwards when wrapping, which could overwrite slots that still need to be
>> +// read. So, require that the last counter value, `u32::MAX`, maps to the last
>> +// slot.
>
> That's a pretty convoluted way to say the number of slots must be a
> power of 2 representable as a u32. :)
>
>> +static_assert!(u32::MAX % MSGQ_NUM_PAGES == MSGQ_NUM_PAGES - 1);
>
> Same here, can't we use `MSGQ_NUM_PAGES.is_power_of_two()`?

I actually had this originally but I think that this form expresses the
condition more directly. The condition/issue is what happens when you
wrap the counter - it should go to the next slot (which necessarily
needs to be zero since a wrapped counter is 0). So you want
MSGQ_NUM_PAGES to divide 2^32. So if you write
MSGQ_NUM_PAGES.is_power_of_two() you are implicitly relying on all
divisors being a power of two, which is an extra logical step.

But in the end I don't really mind, so lmk wdyt.

Re: [PATCH 2/2] gpu: nova-core: gsp: cmdq: increase MSGQ_NUM_PAGES to 64

[Alexandre Courbot]

On Thu Nov 12, 2026 at 10:43 AM JST, Eliot Courtney wrote:
> On Wed Nov 11, 2026 at 11:49 PM JST, Alexandre Courbot wrote:
>> On Wed Nov 11, 2026 at 10:37 AM JST, Eliot Courtney wrote:
>>> In the msgq v2 protocol, which the r000 firmware uses, the queue
>>> pointers are kept in u32 registers and increase monotonically until they
>>> wrap at 2^32.
>>>
>>> `MSGQ_NUM_PAGES` is currently 63 and does not divide 2^32. This means
>>> that on msgq v2, the slot can jump backwards when the counters wrap.
>>> For example:
>>>
>>>     (2^32 - 2) % 63 == 2
>>>     (2^32 - 1) % 63 == 3
>>>              0 % 63 == 0    (slot 0 instead of slot 4)
>>>
>>> The writer can then overwrite slots before the reader has read them.
>>>
>>> Increase `MSGQ_NUM_PAGES` to 64 and check at compile time that
>>> `MSGQ_NUM_PAGES` divides 2^32.
>>
>> Have we tested that this was also working on msgq v0?
>
> Yerp I tested boot on turing, ampere, ada, blackwell.
>
>>
>>>
>>> Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
>>> ---
>>>  drivers/gpu/nova-core/gsp/cmdq.rs | 10 +++++++++-
>>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
>>> index 33074bb53e8c..240ab42db557 100644
>>> --- a/drivers/gpu/nova-core/gsp/cmdq.rs
>>> +++ b/drivers/gpu/nova-core/gsp/cmdq.rs
>>> @@ -84,7 +84,15 @@ trait CommandElement {
>>>  }
>>>  
>>>  /// Number of GSP pages making the [`Msgq`].
>>> -pub(crate) const MSGQ_NUM_PAGES: u32 = 0x3f;
>>> +pub(crate) const MSGQ_NUM_PAGES: u32 = 64;
>>> +
>>> +// From version 2 of the msgq protocol, the queue pointers increase
>>> +// monotonically in `u32` counters until they wrap. If `MSGQ_NUM_PAGES` does not
>>> +// divide `1 << 32`, then the slot, `counter % MSGQ_NUM_PAGES`, can jump
>>> +// backwards when wrapping, which could overwrite slots that still need to be
>>> +// read. So, require that the last counter value, `u32::MAX`, maps to the last
>>> +// slot.
>>
>> That's a pretty convoluted way to say the number of slots must be a
>> power of 2 representable as a u32. :)
>>
>>> +static_assert!(u32::MAX % MSGQ_NUM_PAGES == MSGQ_NUM_PAGES - 1);
>>
>> Same here, can't we use `MSGQ_NUM_PAGES.is_power_of_two()`?
>
> I actually had this originally but I think that this form expresses the
> condition more directly. The condition/issue is what happens when you
> wrap the counter - it should go to the next slot (which necessarily
> needs to be zero since a wrapped counter is 0). So you want
> MSGQ_NUM_PAGES to divide 2^32. So if you write
> MSGQ_NUM_PAGES.is_power_of_two() you are implicitly relying on all
> divisors being a power of two, which is an extra logical step.
>
> But in the end I don't really mind, so lmk wdyt.

The current condition also involves an extra logical step. What you
ideally would have tested is

  (1 << 32).is_multiple_of(MSGQ_NUM_PAGES)

... but as `u32::MAX + 1` is not representable using a u32, the one you
chose is the closest one. It does takes some extra labor to understand
what it intends to convey though.

Trying to put myself into the shoes of someone looking at this code for
the first time, this comment and the condition look a bit hard to chew.
How about using a u64 to test the condition we actually want:

  // The queue pointers are wrapping `u32` counters, and the slot is
  // `counter % MSGQ_NUM_PAGES`. For slots to remain contiguous when a counter
  // wraps, `MSGQ_NUM_PAGES` must divide `1 << 32`.
  static_assert!((1u64 << u32::BITS).is_multiple_of(cv!(MSGQ_NUM_PAGES)));

(I have removed the mention the the msgq v2 detail - it is useful in the
commit log, but becomes unneeded information in the code as it
eventually will only support v2 anyway)

WDYT?

Re: [PATCH 2/2] gpu: nova-core: gsp: cmdq: increase MSGQ_NUM_PAGES to 64

[Eliot Courtney]

On Thu Nov 12, 2026 at 2:19 PM JST, Alexandre Courbot wrote:
> On Thu Nov 12, 2026 at 10:43 AM JST, Eliot Courtney wrote:
>> On Wed Nov 11, 2026 at 11:49 PM JST, Alexandre Courbot wrote:
>>> On Wed Nov 11, 2026 at 10:37 AM JST, Eliot Courtney wrote:
>>>> In the msgq v2 protocol, which the r000 firmware uses, the queue
>>>> pointers are kept in u32 registers and increase monotonically until they
>>>> wrap at 2^32.
>>>>
>>>> `MSGQ_NUM_PAGES` is currently 63 and does not divide 2^32. This means
>>>> that on msgq v2, the slot can jump backwards when the counters wrap.
>>>> For example:
>>>>
>>>>     (2^32 - 2) % 63 == 2
>>>>     (2^32 - 1) % 63 == 3
>>>>              0 % 63 == 0    (slot 0 instead of slot 4)
>>>>
>>>> The writer can then overwrite slots before the reader has read them.
>>>>
>>>> Increase `MSGQ_NUM_PAGES` to 64 and check at compile time that
>>>> `MSGQ_NUM_PAGES` divides 2^32.
>>>
>>> Have we tested that this was also working on msgq v0?
>>
>> Yerp I tested boot on turing, ampere, ada, blackwell.
>>
>>>
>>>>
>>>> Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
>>>> ---
>>>>  drivers/gpu/nova-core/gsp/cmdq.rs | 10 +++++++++-
>>>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
>>>> index 33074bb53e8c..240ab42db557 100644
>>>> --- a/drivers/gpu/nova-core/gsp/cmdq.rs
>>>> +++ b/drivers/gpu/nova-core/gsp/cmdq.rs
>>>> @@ -84,7 +84,15 @@ trait CommandElement {
>>>>  }
>>>>  
>>>>  /// Number of GSP pages making the [`Msgq`].
>>>> -pub(crate) const MSGQ_NUM_PAGES: u32 = 0x3f;
>>>> +pub(crate) const MSGQ_NUM_PAGES: u32 = 64;
>>>> +
>>>> +// From version 2 of the msgq protocol, the queue pointers increase
>>>> +// monotonically in `u32` counters until they wrap. If `MSGQ_NUM_PAGES` does not
>>>> +// divide `1 << 32`, then the slot, `counter % MSGQ_NUM_PAGES`, can jump
>>>> +// backwards when wrapping, which could overwrite slots that still need to be
>>>> +// read. So, require that the last counter value, `u32::MAX`, maps to the last
>>>> +// slot.
>>>
>>> That's a pretty convoluted way to say the number of slots must be a
>>> power of 2 representable as a u32. :)
>>>
>>>> +static_assert!(u32::MAX % MSGQ_NUM_PAGES == MSGQ_NUM_PAGES - 1);
>>>
>>> Same here, can't we use `MSGQ_NUM_PAGES.is_power_of_two()`?
>>
>> I actually had this originally but I think that this form expresses the
>> condition more directly. The condition/issue is what happens when you
>> wrap the counter - it should go to the next slot (which necessarily
>> needs to be zero since a wrapped counter is 0). So you want
>> MSGQ_NUM_PAGES to divide 2^32. So if you write
>> MSGQ_NUM_PAGES.is_power_of_two() you are implicitly relying on all
>> divisors being a power of two, which is an extra logical step.
>>
>> But in the end I don't really mind, so lmk wdyt.
>
> The current condition also involves an extra logical step. What you
> ideally would have tested is
>
>   (1 << 32).is_multiple_of(MSGQ_NUM_PAGES)
>
> ... but as `u32::MAX + 1` is not representable using a u32, the one you
> chose is the closest one. It does takes some extra labor to understand
> what it intends to convey though.
>
> Trying to put myself into the shoes of someone looking at this code for
> the first time, this comment and the condition look a bit hard to chew.
> How about using a u64 to test the condition we actually want:
>
>   // The queue pointers are wrapping `u32` counters, and the slot is
>   // `counter % MSGQ_NUM_PAGES`. For slots to remain contiguous when a counter
>   // wraps, `MSGQ_NUM_PAGES` must divide `1 << 32`.
>   static_assert!((1u64 << u32::BITS).is_multiple_of(cv!(MSGQ_NUM_PAGES)));
>
> (I have removed the mention the the msgq v2 detail - it is useful in the
> commit log, but becomes unneeded information in the code as it
> eventually will only support v2 anyway)
>
> WDYT?

Yeah I originally also tried this but I didn't find as nice a form
(w.r.t. the conversions to u64) as you did, so I will go with your
version. Thanks!