* [PATCH 1/7] gpu: nova-core: gsp: sort MsgFunction variants alphabetically
2026-02-12 6:28 [PATCH 0/7] gpu: nova-core: gsp: add continuation record support Eliot Courtney
@ 2026-02-12 6:28 ` Eliot Courtney
2026-02-12 6:28 ` [PATCH 2/7] gpu: nova-core: gsp: add mechanism to wait for space on command queue Eliot Courtney
` (6 subsequent siblings)
7 siblings, 0 replies; 18+ messages in thread
From: Eliot Courtney @ 2026-02-12 6:28 UTC (permalink / raw)
To: Danilo Krummrich, Alice Ryhl, Alexandre Courbot, David Airlie,
Simona Vetter
Cc: nouveau, dri-devel, linux-kernel, Eliot Courtney
There is no particular order required here and keeping them alphabetical
will help preventing future mistakes.
Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
---
drivers/gpu/nova-core/gsp/fw.rs | 97 +++++++++++++++++++++--------------------
1 file changed, 50 insertions(+), 47 deletions(-)
diff --git a/drivers/gpu/nova-core/gsp/fw.rs b/drivers/gpu/nova-core/gsp/fw.rs
index 83ff91614e36..5b96f5e622a4 100644
--- a/drivers/gpu/nova-core/gsp/fw.rs
+++ b/drivers/gpu/nova-core/gsp/fw.rs
@@ -192,34 +192,34 @@ pub(crate) fn new(gsp_firmware: &GspFirmware, fb_layout: &FbLayout) -> Self {
#[repr(u32)]
pub(crate) enum MsgFunction {
// Common function codes
- Nop = bindings::NV_VGPU_MSG_FUNCTION_NOP,
- SetGuestSystemInfo = bindings::NV_VGPU_MSG_FUNCTION_SET_GUEST_SYSTEM_INFO,
- AllocRoot = bindings::NV_VGPU_MSG_FUNCTION_ALLOC_ROOT,
+ AllocChannelDma = bindings::NV_VGPU_MSG_FUNCTION_ALLOC_CHANNEL_DMA,
+ AllocCtxDma = bindings::NV_VGPU_MSG_FUNCTION_ALLOC_CTX_DMA,
AllocDevice = bindings::NV_VGPU_MSG_FUNCTION_ALLOC_DEVICE,
AllocMemory = bindings::NV_VGPU_MSG_FUNCTION_ALLOC_MEMORY,
- AllocCtxDma = bindings::NV_VGPU_MSG_FUNCTION_ALLOC_CTX_DMA,
- AllocChannelDma = bindings::NV_VGPU_MSG_FUNCTION_ALLOC_CHANNEL_DMA,
- MapMemory = bindings::NV_VGPU_MSG_FUNCTION_MAP_MEMORY,
- BindCtxDma = bindings::NV_VGPU_MSG_FUNCTION_BIND_CTX_DMA,
AllocObject = bindings::NV_VGPU_MSG_FUNCTION_ALLOC_OBJECT,
+ AllocRoot = bindings::NV_VGPU_MSG_FUNCTION_ALLOC_ROOT,
+ BindCtxDma = bindings::NV_VGPU_MSG_FUNCTION_BIND_CTX_DMA,
Free = bindings::NV_VGPU_MSG_FUNCTION_FREE,
- Log = bindings::NV_VGPU_MSG_FUNCTION_LOG,
GetGspStaticInfo = bindings::NV_VGPU_MSG_FUNCTION_GET_GSP_STATIC_INFO,
- SetRegistry = bindings::NV_VGPU_MSG_FUNCTION_SET_REGISTRY,
- GspSetSystemInfo = bindings::NV_VGPU_MSG_FUNCTION_GSP_SET_SYSTEM_INFO,
+ GetStaticInfo = bindings::NV_VGPU_MSG_FUNCTION_GET_STATIC_INFO,
GspInitPostObjGpu = bindings::NV_VGPU_MSG_FUNCTION_GSP_INIT_POST_OBJGPU,
GspRmControl = bindings::NV_VGPU_MSG_FUNCTION_GSP_RM_CONTROL,
- GetStaticInfo = bindings::NV_VGPU_MSG_FUNCTION_GET_STATIC_INFO,
+ GspSetSystemInfo = bindings::NV_VGPU_MSG_FUNCTION_GSP_SET_SYSTEM_INFO,
+ Log = bindings::NV_VGPU_MSG_FUNCTION_LOG,
+ MapMemory = bindings::NV_VGPU_MSG_FUNCTION_MAP_MEMORY,
+ Nop = bindings::NV_VGPU_MSG_FUNCTION_NOP,
+ SetGuestSystemInfo = bindings::NV_VGPU_MSG_FUNCTION_SET_GUEST_SYSTEM_INFO,
+ SetRegistry = bindings::NV_VGPU_MSG_FUNCTION_SET_REGISTRY,
// Event codes
GspInitDone = bindings::NV_VGPU_MSG_EVENT_GSP_INIT_DONE,
+ GspLockdownNotice = bindings::NV_VGPU_MSG_EVENT_GSP_LOCKDOWN_NOTICE,
+ GspPostNoCat = bindings::NV_VGPU_MSG_EVENT_GSP_POST_NOCAT_RECORD,
GspRunCpuSequencer = bindings::NV_VGPU_MSG_EVENT_GSP_RUN_CPU_SEQUENCER,
- PostEvent = bindings::NV_VGPU_MSG_EVENT_POST_EVENT,
- RcTriggered = bindings::NV_VGPU_MSG_EVENT_RC_TRIGGERED,
MmuFaultQueued = bindings::NV_VGPU_MSG_EVENT_MMU_FAULT_QUEUED,
OsErrorLog = bindings::NV_VGPU_MSG_EVENT_OS_ERROR_LOG,
- GspPostNoCat = bindings::NV_VGPU_MSG_EVENT_GSP_POST_NOCAT_RECORD,
- GspLockdownNotice = bindings::NV_VGPU_MSG_EVENT_GSP_LOCKDOWN_NOTICE,
+ PostEvent = bindings::NV_VGPU_MSG_EVENT_POST_EVENT,
+ RcTriggered = bindings::NV_VGPU_MSG_EVENT_RC_TRIGGERED,
UcodeLibOsPrint = bindings::NV_VGPU_MSG_EVENT_UCODE_LIBOS_PRINT,
}
@@ -227,34 +227,34 @@ impl fmt::Display for MsgFunction {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self {
// Common function codes
- MsgFunction::Nop => write!(f, "NOP"),
- MsgFunction::SetGuestSystemInfo => write!(f, "SET_GUEST_SYSTEM_INFO"),
- MsgFunction::AllocRoot => write!(f, "ALLOC_ROOT"),
+ MsgFunction::AllocChannelDma => write!(f, "ALLOC_CHANNEL_DMA"),
+ MsgFunction::AllocCtxDma => write!(f, "ALLOC_CTX_DMA"),
MsgFunction::AllocDevice => write!(f, "ALLOC_DEVICE"),
MsgFunction::AllocMemory => write!(f, "ALLOC_MEMORY"),
- MsgFunction::AllocCtxDma => write!(f, "ALLOC_CTX_DMA"),
- MsgFunction::AllocChannelDma => write!(f, "ALLOC_CHANNEL_DMA"),
- MsgFunction::MapMemory => write!(f, "MAP_MEMORY"),
- MsgFunction::BindCtxDma => write!(f, "BIND_CTX_DMA"),
MsgFunction::AllocObject => write!(f, "ALLOC_OBJECT"),
+ MsgFunction::AllocRoot => write!(f, "ALLOC_ROOT"),
+ MsgFunction::BindCtxDma => write!(f, "BIND_CTX_DMA"),
MsgFunction::Free => write!(f, "FREE"),
- MsgFunction::Log => write!(f, "LOG"),
MsgFunction::GetGspStaticInfo => write!(f, "GET_GSP_STATIC_INFO"),
- MsgFunction::SetRegistry => write!(f, "SET_REGISTRY"),
- MsgFunction::GspSetSystemInfo => write!(f, "GSP_SET_SYSTEM_INFO"),
+ MsgFunction::GetStaticInfo => write!(f, "GET_STATIC_INFO"),
MsgFunction::GspInitPostObjGpu => write!(f, "GSP_INIT_POST_OBJGPU"),
MsgFunction::GspRmControl => write!(f, "GSP_RM_CONTROL"),
- MsgFunction::GetStaticInfo => write!(f, "GET_STATIC_INFO"),
+ MsgFunction::GspSetSystemInfo => write!(f, "GSP_SET_SYSTEM_INFO"),
+ MsgFunction::Log => write!(f, "LOG"),
+ MsgFunction::MapMemory => write!(f, "MAP_MEMORY"),
+ MsgFunction::Nop => write!(f, "NOP"),
+ MsgFunction::SetGuestSystemInfo => write!(f, "SET_GUEST_SYSTEM_INFO"),
+ MsgFunction::SetRegistry => write!(f, "SET_REGISTRY"),
// Event codes
MsgFunction::GspInitDone => write!(f, "INIT_DONE"),
+ MsgFunction::GspLockdownNotice => write!(f, "LOCKDOWN_NOTICE"),
+ MsgFunction::GspPostNoCat => write!(f, "NOCAT"),
MsgFunction::GspRunCpuSequencer => write!(f, "RUN_CPU_SEQUENCER"),
- MsgFunction::PostEvent => write!(f, "POST_EVENT"),
- MsgFunction::RcTriggered => write!(f, "RC_TRIGGERED"),
MsgFunction::MmuFaultQueued => write!(f, "MMU_FAULT_QUEUED"),
MsgFunction::OsErrorLog => write!(f, "OS_ERROR_LOG"),
- MsgFunction::GspPostNoCat => write!(f, "NOCAT"),
- MsgFunction::GspLockdownNotice => write!(f, "LOCKDOWN_NOTICE"),
+ MsgFunction::PostEvent => write!(f, "POST_EVENT"),
+ MsgFunction::RcTriggered => write!(f, "RC_TRIGGERED"),
MsgFunction::UcodeLibOsPrint => write!(f, "LIBOS_PRINT"),
}
}
@@ -265,38 +265,41 @@ impl TryFrom<u32> for MsgFunction {
fn try_from(value: u32) -> Result<MsgFunction> {
match value {
- bindings::NV_VGPU_MSG_FUNCTION_NOP => Ok(MsgFunction::Nop),
- bindings::NV_VGPU_MSG_FUNCTION_SET_GUEST_SYSTEM_INFO => {
- Ok(MsgFunction::SetGuestSystemInfo)
- }
- bindings::NV_VGPU_MSG_FUNCTION_ALLOC_ROOT => Ok(MsgFunction::AllocRoot),
+ // Common function codes
+ bindings::NV_VGPU_MSG_FUNCTION_ALLOC_CHANNEL_DMA => Ok(MsgFunction::AllocChannelDma),
+ bindings::NV_VGPU_MSG_FUNCTION_ALLOC_CTX_DMA => Ok(MsgFunction::AllocCtxDma),
bindings::NV_VGPU_MSG_FUNCTION_ALLOC_DEVICE => Ok(MsgFunction::AllocDevice),
bindings::NV_VGPU_MSG_FUNCTION_ALLOC_MEMORY => Ok(MsgFunction::AllocMemory),
- bindings::NV_VGPU_MSG_FUNCTION_ALLOC_CTX_DMA => Ok(MsgFunction::AllocCtxDma),
- bindings::NV_VGPU_MSG_FUNCTION_ALLOC_CHANNEL_DMA => Ok(MsgFunction::AllocChannelDma),
- bindings::NV_VGPU_MSG_FUNCTION_MAP_MEMORY => Ok(MsgFunction::MapMemory),
- bindings::NV_VGPU_MSG_FUNCTION_BIND_CTX_DMA => Ok(MsgFunction::BindCtxDma),
bindings::NV_VGPU_MSG_FUNCTION_ALLOC_OBJECT => Ok(MsgFunction::AllocObject),
+ bindings::NV_VGPU_MSG_FUNCTION_ALLOC_ROOT => Ok(MsgFunction::AllocRoot),
+ bindings::NV_VGPU_MSG_FUNCTION_BIND_CTX_DMA => Ok(MsgFunction::BindCtxDma),
bindings::NV_VGPU_MSG_FUNCTION_FREE => Ok(MsgFunction::Free),
- bindings::NV_VGPU_MSG_FUNCTION_LOG => Ok(MsgFunction::Log),
bindings::NV_VGPU_MSG_FUNCTION_GET_GSP_STATIC_INFO => Ok(MsgFunction::GetGspStaticInfo),
- bindings::NV_VGPU_MSG_FUNCTION_SET_REGISTRY => Ok(MsgFunction::SetRegistry),
- bindings::NV_VGPU_MSG_FUNCTION_GSP_SET_SYSTEM_INFO => Ok(MsgFunction::GspSetSystemInfo),
+ bindings::NV_VGPU_MSG_FUNCTION_GET_STATIC_INFO => Ok(MsgFunction::GetStaticInfo),
bindings::NV_VGPU_MSG_FUNCTION_GSP_INIT_POST_OBJGPU => {
Ok(MsgFunction::GspInitPostObjGpu)
}
bindings::NV_VGPU_MSG_FUNCTION_GSP_RM_CONTROL => Ok(MsgFunction::GspRmControl),
- bindings::NV_VGPU_MSG_FUNCTION_GET_STATIC_INFO => Ok(MsgFunction::GetStaticInfo),
+ bindings::NV_VGPU_MSG_FUNCTION_GSP_SET_SYSTEM_INFO => Ok(MsgFunction::GspSetSystemInfo),
+ bindings::NV_VGPU_MSG_FUNCTION_LOG => Ok(MsgFunction::Log),
+ bindings::NV_VGPU_MSG_FUNCTION_MAP_MEMORY => Ok(MsgFunction::MapMemory),
+ bindings::NV_VGPU_MSG_FUNCTION_NOP => Ok(MsgFunction::Nop),
+ bindings::NV_VGPU_MSG_FUNCTION_SET_GUEST_SYSTEM_INFO => {
+ Ok(MsgFunction::SetGuestSystemInfo)
+ }
+ bindings::NV_VGPU_MSG_FUNCTION_SET_REGISTRY => Ok(MsgFunction::SetRegistry),
+
+ // Event codes
bindings::NV_VGPU_MSG_EVENT_GSP_INIT_DONE => Ok(MsgFunction::GspInitDone),
+ bindings::NV_VGPU_MSG_EVENT_GSP_LOCKDOWN_NOTICE => Ok(MsgFunction::GspLockdownNotice),
+ bindings::NV_VGPU_MSG_EVENT_GSP_POST_NOCAT_RECORD => Ok(MsgFunction::GspPostNoCat),
bindings::NV_VGPU_MSG_EVENT_GSP_RUN_CPU_SEQUENCER => {
Ok(MsgFunction::GspRunCpuSequencer)
}
- bindings::NV_VGPU_MSG_EVENT_POST_EVENT => Ok(MsgFunction::PostEvent),
- bindings::NV_VGPU_MSG_EVENT_RC_TRIGGERED => Ok(MsgFunction::RcTriggered),
bindings::NV_VGPU_MSG_EVENT_MMU_FAULT_QUEUED => Ok(MsgFunction::MmuFaultQueued),
bindings::NV_VGPU_MSG_EVENT_OS_ERROR_LOG => Ok(MsgFunction::OsErrorLog),
- bindings::NV_VGPU_MSG_EVENT_GSP_POST_NOCAT_RECORD => Ok(MsgFunction::GspPostNoCat),
- bindings::NV_VGPU_MSG_EVENT_GSP_LOCKDOWN_NOTICE => Ok(MsgFunction::GspLockdownNotice),
+ bindings::NV_VGPU_MSG_EVENT_POST_EVENT => Ok(MsgFunction::PostEvent),
+ bindings::NV_VGPU_MSG_EVENT_RC_TRIGGERED => Ok(MsgFunction::RcTriggered),
bindings::NV_VGPU_MSG_EVENT_UCODE_LIBOS_PRINT => Ok(MsgFunction::UcodeLibOsPrint),
_ => Err(EINVAL),
}
--
2.53.0
^ permalink raw reply [flat|nested] 18+ messages in thread* [PATCH 2/7] gpu: nova-core: gsp: add mechanism to wait for space on command queue
2026-02-12 6:28 [PATCH 0/7] gpu: nova-core: gsp: add continuation record support Eliot Courtney
2026-02-12 6:28 ` [PATCH 1/7] gpu: nova-core: gsp: sort MsgFunction variants alphabetically Eliot Courtney
@ 2026-02-12 6:28 ` Eliot Courtney
2026-02-18 3:13 ` Alexandre Courbot
2026-02-12 6:28 ` [PATCH 3/7] gpu: nova-core: gsp: add checking oversized commands Eliot Courtney
` (5 subsequent siblings)
7 siblings, 1 reply; 18+ messages in thread
From: Eliot Courtney @ 2026-02-12 6:28 UTC (permalink / raw)
To: Danilo Krummrich, Alice Ryhl, Alexandre Courbot, David Airlie,
Simona Vetter
Cc: nouveau, dri-devel, linux-kernel, Eliot Courtney
Add `allocate_command_with_timeout` which waits for space on the GSP
command queue. It uses a similar timeout to nouveau.
Let `send_command` wait for space to free up in the command queue by
calling `allocate_command_with_timeout`. This is required to
support continuation records which can fill up the queue.
Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
---
drivers/gpu/nova-core/gsp/cmdq.rs | 42 ++++++++++++++++++++++++++++++++++++---
1 file changed, 39 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
index 46819a82a51a..baae06de0e09 100644
--- a/drivers/gpu/nova-core/gsp/cmdq.rs
+++ b/drivers/gpu/nova-core/gsp/cmdq.rs
@@ -243,6 +243,16 @@ fn new(dev: &device::Device<device::Bound>) -> Result<Self> {
}
}
+ fn driver_bytes_available_to_write(&self) -> usize {
+ let tx = self.cpu_write_ptr();
+ let rx = self.gsp_read_ptr();
+ // `rx` and `tx` are both in `0..MSGQ_NUM_PAGES` per the invariants of `gsp_read_ptr` and
+ // `cpu_write_ptr`. The minimum value case is where `rx == 0` and `tx == MSGQ_NUM_PAGES -
+ // 1`, which gives `0 + MSGQ_NUM_PAGES - (MSGQ_NUM_PAGES - 1) - 1 == 0`.
+ let slots = (rx + MSGQ_NUM_PAGES - tx - 1) % MSGQ_NUM_PAGES;
+ num::u32_as_usize(slots) * GSP_PAGE_SIZE
+ }
+
/// Returns the region of the GSP message queue that the driver is currently allowed to read
/// from.
///
@@ -311,6 +321,25 @@ fn allocate_command(&mut self, size: usize) -> Result<GspCommand<'_>> {
})
}
+ /// Allocates a region on the command queue that is large enough to send a command of `size`
+ /// bytes, waiting for space to become available.
+ ///
+ /// This returns a [`GspCommand`] ready to be written to by the caller.
+ ///
+ /// # Errors
+ ///
+ /// - `ETIMEDOUT` if space does not become available within the timeout.
+ /// - `EIO` if the command header is not properly aligned.
+ fn allocate_command_with_timeout(&mut self, size: usize) -> Result<GspCommand<'_>> {
+ read_poll_timeout(
+ || Ok(self.driver_bytes_available_to_write()),
+ |available_bytes| *available_bytes >= size_of::<GspMsgElement>() + size,
+ Delta::ZERO,
+ Delta::from_secs(1),
+ )?;
+ self.allocate_command(size)
+ }
+
// Returns the index of the memory page the GSP will write the next message to.
//
// # Invariants
@@ -480,11 +509,18 @@ fn notify_gsp(bar: &Bar0) {
.write(bar);
}
+ fn command_size<M>(command: &M) -> usize
+ where
+ M: CommandToGsp,
+ {
+ size_of::<M::Command>() + command.variable_payload_len()
+ }
+
/// Sends `command` to the GSP.
///
/// # Errors
///
- /// - `EAGAIN` if there was not enough space in the command queue to send the command.
+ /// - `ETIMEDOUT` if space does not become available within the timeout.
/// - `EIO` if the variable payload requested by the command has not been entirely
/// written to by its [`CommandToGsp::init_variable_payload`] method.
///
@@ -495,8 +531,8 @@ pub(crate) fn send_command<M>(&mut self, bar: &Bar0, command: M) -> Result
// This allows all error types, including `Infallible`, to be used for `M::InitError`.
Error: From<M::InitError>,
{
- let command_size = size_of::<M::Command>() + command.variable_payload_len();
- let dst = self.gsp_mem.allocate_command(command_size)?;
+ let command_size = Self::command_size(&command);
+ let dst = self.gsp_mem.allocate_command_with_timeout(command_size)?;
// Extract area for the command itself.
let (cmd, payload_1) = M::Command::from_bytes_mut_prefix(dst.contents.0).ok_or(EIO)?;
--
2.53.0
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 2/7] gpu: nova-core: gsp: add mechanism to wait for space on command queue
2026-02-12 6:28 ` [PATCH 2/7] gpu: nova-core: gsp: add mechanism to wait for space on command queue Eliot Courtney
@ 2026-02-18 3:13 ` Alexandre Courbot
2026-02-18 4:05 ` Eliot Courtney
0 siblings, 1 reply; 18+ messages in thread
From: Alexandre Courbot @ 2026-02-18 3:13 UTC (permalink / raw)
To: Eliot Courtney
Cc: Danilo Krummrich, Alice Ryhl, David Airlie, Simona Vetter,
nouveau, dri-devel, linux-kernel
On Thu Feb 12, 2026 at 3:28 PM JST, Eliot Courtney wrote:
> Add `allocate_command_with_timeout` which waits for space on the GSP
> command queue. It uses a similar timeout to nouveau.
>
> Let `send_command` wait for space to free up in the command queue by
> calling `allocate_command_with_timeout`. This is required to
I'd name it just `allocate_command_timeout`, to follow the pattern of
the existing `read_poll_timeout` - but actually we might not even need
a new method (see below).
> support continuation records which can fill up the queue.
>
> Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
> ---
> drivers/gpu/nova-core/gsp/cmdq.rs | 42 ++++++++++++++++++++++++++++++++++++---
> 1 file changed, 39 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
> index 46819a82a51a..baae06de0e09 100644
> --- a/drivers/gpu/nova-core/gsp/cmdq.rs
> +++ b/drivers/gpu/nova-core/gsp/cmdq.rs
> @@ -243,6 +243,16 @@ fn new(dev: &device::Device<device::Bound>) -> Result<Self> {
> }
> }
>
> + fn driver_bytes_available_to_write(&self) -> usize {
For consistency with `driver_write_area`, shall we name this
`driver_write_area_size`? And add a doccomment mentioning the returned
value is in bytes.
> + let tx = self.cpu_write_ptr();
> + let rx = self.gsp_read_ptr();
> + // `rx` and `tx` are both in `0..MSGQ_NUM_PAGES` per the invariants of `gsp_read_ptr` and
Nit: missing empty line.
> + // `cpu_write_ptr`. The minimum value case is where `rx == 0` and `tx == MSGQ_NUM_PAGES -
> + // 1`, which gives `0 + MSGQ_NUM_PAGES - (MSGQ_NUM_PAGES - 1) - 1 == 0`.
> + let slots = (rx + MSGQ_NUM_PAGES - tx - 1) % MSGQ_NUM_PAGES;
> + num::u32_as_usize(slots) * GSP_PAGE_SIZE
> + }
> +
> /// Returns the region of the GSP message queue that the driver is currently allowed to read
> /// from.
> ///
> @@ -311,6 +321,25 @@ fn allocate_command(&mut self, size: usize) -> Result<GspCommand<'_>> {
> })
> }
>
> + /// Allocates a region on the command queue that is large enough to send a command of `size`
> + /// bytes, waiting for space to become available.
> + ///
> + /// This returns a [`GspCommand`] ready to be written to by the caller.
> + ///
> + /// # Errors
> + ///
> + /// - `ETIMEDOUT` if space does not become available within the timeout.
> + /// - `EIO` if the command header is not properly aligned.
> + fn allocate_command_with_timeout(&mut self, size: usize) -> Result<GspCommand<'_>> {
Should the timeout be an argument? That way we can simply add it to
`allocate_command`, and invoke it with `Delta::ZERO` whenever we don't
want to wait. This is more explicit at the call site, removes the
need to have two methods, and removes the redundant size check from
`allocate_command` which is now done by this `read_poll_timeout`.
> + read_poll_timeout(
> + || Ok(self.driver_bytes_available_to_write()),
> + |available_bytes| *available_bytes >= size_of::<GspMsgElement>() + size,
> + Delta::ZERO,
> + Delta::from_secs(1),
> + )?;
> + self.allocate_command(size)
> + }
> +
> // Returns the index of the memory page the GSP will write the next message to.
> //
> // # Invariants
> @@ -480,11 +509,18 @@ fn notify_gsp(bar: &Bar0) {
> .write(bar);
> }
>
> + fn command_size<M>(command: &M) -> usize
Shouldn't this be a member function of `CommandToGsp`? Please add some
basic documentation for it as well. As a general rule, all methods, even
basic ones, should have at least one line of doccomment.
> + where
> + M: CommandToGsp,
> + {
> + size_of::<M::Command>() + command.variable_payload_len()
> + }
> +
> /// Sends `command` to the GSP.
> ///
> /// # Errors
> ///
> - /// - `EAGAIN` if there was not enough space in the command queue to send the command.
> + /// - `ETIMEDOUT` if space does not become available within the timeout.
> /// - `EIO` if the variable payload requested by the command has not been entirely
> /// written to by its [`CommandToGsp::init_variable_payload`] method.
> ///
> @@ -495,8 +531,8 @@ pub(crate) fn send_command<M>(&mut self, bar: &Bar0, command: M) -> Result
> // This allows all error types, including `Infallible`, to be used for `M::InitError`.
> Error: From<M::InitError>,
> {
> - let command_size = size_of::<M::Command>() + command.variable_payload_len();
> - let dst = self.gsp_mem.allocate_command(command_size)?;
> + let command_size = Self::command_size(&command);
The addition of `command_size` looks like an unrelated change - it is
not really leveraged until patch 6 (although it is still valuable on its
own). Can you move it to its own patch for clarity?
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 2/7] gpu: nova-core: gsp: add mechanism to wait for space on command queue
2026-02-18 3:13 ` Alexandre Courbot
@ 2026-02-18 4:05 ` Eliot Courtney
2026-02-18 7:20 ` Alexandre Courbot
0 siblings, 1 reply; 18+ messages in thread
From: Eliot Courtney @ 2026-02-18 4:05 UTC (permalink / raw)
To: Alexandre Courbot, Eliot Courtney
Cc: Danilo Krummrich, Alice Ryhl, Simona Vetter, nouveau, dri-devel,
linux-kernel
On Wed Feb 18, 2026 at 12:13 PM JST, Alexandre Courbot wrote:
>> + /// Allocates a region on the command queue that is large enough to send a command of `size`
>> + /// bytes, waiting for space to become available.
>> + ///
>> + /// This returns a [`GspCommand`] ready to be written to by the caller.
>> + ///
>> + /// # Errors
>> + ///
>> + /// - `ETIMEDOUT` if space does not become available within the timeout.
>> + /// - `EIO` if the command header is not properly aligned.
>> + fn allocate_command_with_timeout(&mut self, size: usize) -> Result<GspCommand<'_>> {
>
> Should the timeout be an argument? That way we can simply add it to
> `allocate_command`, and invoke it with `Delta::ZERO` whenever we don't
> want to wait. This is more explicit at the call site, removes the
> need to have two methods, and removes the redundant size check from
> `allocate_command` which is now done by this `read_poll_timeout`.
Good idea, thanks.
>> + fn command_size<M>(command: &M) -> usize
>
> Shouldn't this be a member function of `CommandToGsp`? Please add some
> basic documentation for it as well. As a general rule, all methods, even
> basic ones, should have at least one line of doccomment.
I thought about this, but adding a function to CommandToGsp with
a default implementation seems odd to me, because implementors of that
trait could override it, which does not really make sense. We have
command size defined as the size of the struct plus the variable payload
size. Adding a function to CommandToGsp would give two methods to
calculate the command size which could differ. So, seems weird to me.
An alternative would be to make it a free standing function. This would
let it be used by WrappingCommand later as well. WDYT?
Will make sure all functions have doccomment from now on, thanks.
>
>> + where
>> + M: CommandToGsp,
>> + {
>> + size_of::<M::Command>() + command.variable_payload_len()
>> + }
>> +
>> /// Sends `command` to the GSP.
>> ///
>> /// # Errors
>> ///
>> - /// - `EAGAIN` if there was not enough space in the command queue to send the command.
>> + /// - `ETIMEDOUT` if space does not become available within the timeout.
>> /// - `EIO` if the variable payload requested by the command has not been entirely
>> /// written to by its [`CommandToGsp::init_variable_payload`] method.
>> ///
>> @@ -495,8 +531,8 @@ pub(crate) fn send_command<M>(&mut self, bar: &Bar0, command: M) -> Result
>> // This allows all error types, including `Infallible`, to be used for `M::InitError`.
>> Error: From<M::InitError>,
>> {
>> - let command_size = size_of::<M::Command>() + command.variable_payload_len();
>> - let dst = self.gsp_mem.allocate_command(command_size)?;
>> + let command_size = Self::command_size(&command);
>
> The addition of `command_size` looks like an unrelated change - it is
> not really leveraged until patch 6 (although it is still valuable on its
> own). Can you move it to its own patch for clarity?
Will do.
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 2/7] gpu: nova-core: gsp: add mechanism to wait for space on command queue
2026-02-18 4:05 ` Eliot Courtney
@ 2026-02-18 7:20 ` Alexandre Courbot
0 siblings, 0 replies; 18+ messages in thread
From: Alexandre Courbot @ 2026-02-18 7:20 UTC (permalink / raw)
To: Eliot Courtney
Cc: Danilo Krummrich, Alice Ryhl, Simona Vetter, nouveau, dri-devel,
linux-kernel
On Wed Feb 18, 2026 at 1:05 PM JST, Eliot Courtney wrote:
>>> + fn command_size<M>(command: &M) -> usize
>>
>> Shouldn't this be a member function of `CommandToGsp`? Please add some
>> basic documentation for it as well. As a general rule, all methods, even
>> basic ones, should have at least one line of doccomment.
>
> I thought about this, but adding a function to CommandToGsp with
> a default implementation seems odd to me, because implementors of that
> trait could override it, which does not really make sense. We have
> command size defined as the size of the struct plus the variable payload
> size. Adding a function to CommandToGsp would give two methods to
> calculate the command size which could differ. So, seems weird to me.
>
> An alternative would be to make it a free standing function. This would
> let it be used by WrappingCommand later as well. WDYT?
Using default trait methods even when they should not be overriden is
common in the kernel, so I am not worried about that. But you are right
that the use of this size is local to the command queue so maybe it's
better to keep it as a (documented :)) method of it.
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 3/7] gpu: nova-core: gsp: add checking oversized commands
2026-02-12 6:28 [PATCH 0/7] gpu: nova-core: gsp: add continuation record support Eliot Courtney
2026-02-12 6:28 ` [PATCH 1/7] gpu: nova-core: gsp: sort MsgFunction variants alphabetically Eliot Courtney
2026-02-12 6:28 ` [PATCH 2/7] gpu: nova-core: gsp: add mechanism to wait for space on command queue Eliot Courtney
@ 2026-02-12 6:28 ` Eliot Courtney
2026-02-18 3:23 ` Alexandre Courbot
2026-02-12 6:28 ` [PATCH 4/7] gpu: nova-core: gsp: clarify invariant on command queue Eliot Courtney
` (4 subsequent siblings)
7 siblings, 1 reply; 18+ messages in thread
From: Eliot Courtney @ 2026-02-12 6:28 UTC (permalink / raw)
To: Danilo Krummrich, Alice Ryhl, Alexandre Courbot, David Airlie,
Simona Vetter
Cc: nouveau, dri-devel, linux-kernel, Eliot Courtney
The limit is 16 pages for a single command sent to the GSP. Return an
error if `allocate_command` is called with a too large size.
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 baae06de0e09..dbc9e95f1b17 100644
--- a/drivers/gpu/nova-core/gsp/cmdq.rs
+++ b/drivers/gpu/nova-core/gsp/cmdq.rs
@@ -122,6 +122,11 @@ fn read(
/// Number of GSP pages making the [`Msgq`].
pub(crate) const MSGQ_NUM_PAGES: u32 = 0x3f;
+/// Maximum size of a single GSP command in bytes.
+///
+/// A single command can span up to 16 GSP pages.
+pub(crate) const MSGQ_MSG_SIZE_MAX: usize = GSP_PAGE_SIZE * 16;
+
/// Circular buffer of a [`Msgq`].
///
/// This area of memory is to be shared between the driver and the GSP to exchange commands or
@@ -329,8 +334,11 @@ fn allocate_command(&mut self, size: usize) -> Result<GspCommand<'_>> {
/// # Errors
///
/// - `ETIMEDOUT` if space does not become available within the timeout.
- /// - `EIO` if the command header is not properly aligned.
+ /// - `EIO` if the command header is not properly aligned or sizing is impossible.
fn allocate_command_with_timeout(&mut self, size: usize) -> Result<GspCommand<'_>> {
+ if size_of::<GspMsgElement>() + size > MSGQ_MSG_SIZE_MAX {
+ return Err(EIO);
+ }
read_poll_timeout(
|| Ok(self.driver_bytes_available_to_write()),
|available_bytes| *available_bytes >= size_of::<GspMsgElement>() + size,
--
2.53.0
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 3/7] gpu: nova-core: gsp: add checking oversized commands
2026-02-12 6:28 ` [PATCH 3/7] gpu: nova-core: gsp: add checking oversized commands Eliot Courtney
@ 2026-02-18 3:23 ` Alexandre Courbot
2026-02-18 6:50 ` Eliot Courtney
0 siblings, 1 reply; 18+ messages in thread
From: Alexandre Courbot @ 2026-02-18 3:23 UTC (permalink / raw)
To: Eliot Courtney
Cc: Danilo Krummrich, Alice Ryhl, David Airlie, Simona Vetter,
nouveau, dri-devel, linux-kernel
On Thu Feb 12, 2026 at 3:28 PM JST, Eliot Courtney wrote:
> The limit is 16 pages for a single command sent to the GSP. Return an
> error if `allocate_command` is called with a too large size.
>
> 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 baae06de0e09..dbc9e95f1b17 100644
> --- a/drivers/gpu/nova-core/gsp/cmdq.rs
> +++ b/drivers/gpu/nova-core/gsp/cmdq.rs
> @@ -122,6 +122,11 @@ fn read(
> /// Number of GSP pages making the [`Msgq`].
> pub(crate) const MSGQ_NUM_PAGES: u32 = 0x3f;
>
> +/// Maximum size of a single GSP command in bytes.
> +///
> +/// A single command can span up to 16 GSP pages.
> +pub(crate) const MSGQ_MSG_SIZE_MAX: usize = GSP_PAGE_SIZE * 16;
Where does this limitation come from? Is there a value in OpenRM that we
can add to the bindings as the source of truth?
> +
> /// Circular buffer of a [`Msgq`].
> ///
> /// This area of memory is to be shared between the driver and the GSP to exchange commands or
> @@ -329,8 +334,11 @@ fn allocate_command(&mut self, size: usize) -> Result<GspCommand<'_>> {
> /// # Errors
> ///
> /// - `ETIMEDOUT` if space does not become available within the timeout.
> - /// - `EIO` if the command header is not properly aligned.
> + /// - `EIO` if the command header is not properly aligned or sizing is impossible.
`sizing is impossible` should be its own error - probably `EMSGSIZE`
(which would need to be added to `kernel/error.rs` in its own patch).
Also the wording sounds a bit confusing to me, why not say "if the
command is larger than `MSGQ_MSG_SIZE_MAX`"?
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 3/7] gpu: nova-core: gsp: add checking oversized commands
2026-02-18 3:23 ` Alexandre Courbot
@ 2026-02-18 6:50 ` Eliot Courtney
2026-02-18 7:25 ` Alexandre Courbot
0 siblings, 1 reply; 18+ messages in thread
From: Eliot Courtney @ 2026-02-18 6:50 UTC (permalink / raw)
To: Alexandre Courbot, Eliot Courtney
Cc: Danilo Krummrich, Alice Ryhl, David Airlie, Simona Vetter,
nouveau, dri-devel, linux-kernel, dri-devel
On Wed Feb 18, 2026 at 12:23 PM JST, Alexandre Courbot wrote:
>> +/// Maximum size of a single GSP command in bytes.
>> +///
>> +/// A single command can span up to 16 GSP pages.
>> +pub(crate) const MSGQ_MSG_SIZE_MAX: usize = GSP_PAGE_SIZE * 16;
>
> Where does this limitation come from? Is there a value in OpenRM that we
> can add to the bindings as the source of truth?
This is a GSP protocol level limitation. And yes, there's a constant
so I'll add that to the bindings. Thanks!
>
>> +
>> /// Circular buffer of a [`Msgq`].
>> ///
>> /// This area of memory is to be shared between the driver and the GSP to exchange commands or
>> @@ -329,8 +334,11 @@ fn allocate_command(&mut self, size: usize) -> Result<GspCommand<'_>> {
>> /// # Errors
>> ///
>> /// - `ETIMEDOUT` if space does not become available within the timeout.
>> - /// - `EIO` if the command header is not properly aligned.
>> + /// - `EIO` if the command header is not properly aligned or sizing is impossible.
>
> `sizing is impossible` should be its own error - probably `EMSGSIZE`
> (which would need to be added to `kernel/error.rs` in its own patch).
> Also the wording sounds a bit confusing to me, why not say "if the
> command is larger than `MSGQ_MSG_SIZE_MAX`"?
Do you mean in its own patch in this series, or sent as a separate patch
to a different tree (e.g. rfl)?
Tangential but I feel like some of the errors in this file are a bit
odd, like the existing EIO if the command doesn't fit (which it always
should). WDYT?
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 3/7] gpu: nova-core: gsp: add checking oversized commands
2026-02-18 6:50 ` Eliot Courtney
@ 2026-02-18 7:25 ` Alexandre Courbot
0 siblings, 0 replies; 18+ messages in thread
From: Alexandre Courbot @ 2026-02-18 7:25 UTC (permalink / raw)
To: Eliot Courtney
Cc: Danilo Krummrich, Alice Ryhl, Simona Vetter, nouveau, dri-devel,
linux-kernel, dri-devel
On Wed Feb 18, 2026 at 3:50 PM JST, Eliot Courtney wrote:
> On Wed Feb 18, 2026 at 12:23 PM JST, Alexandre Courbot wrote:
>>> +/// Maximum size of a single GSP command in bytes.
>>> +///
>>> +/// A single command can span up to 16 GSP pages.
>>> +pub(crate) const MSGQ_MSG_SIZE_MAX: usize = GSP_PAGE_SIZE * 16;
>>
>> Where does this limitation come from? Is there a value in OpenRM that we
>> can add to the bindings as the source of truth?
>
> This is a GSP protocol level limitation. And yes, there's a constant
> so I'll add that to the bindings. Thanks!
>
>>
>>> +
>>> /// Circular buffer of a [`Msgq`].
>>> ///
>>> /// This area of memory is to be shared between the driver and the GSP to exchange commands or
>>> @@ -329,8 +334,11 @@ fn allocate_command(&mut self, size: usize) -> Result<GspCommand<'_>> {
>>> /// # Errors
>>> ///
>>> /// - `ETIMEDOUT` if space does not become available within the timeout.
>>> - /// - `EIO` if the command header is not properly aligned.
>>> + /// - `EIO` if the command header is not properly aligned or sizing is impossible.
>>
>> `sizing is impossible` should be its own error - probably `EMSGSIZE`
>> (which would need to be added to `kernel/error.rs` in its own patch).
>> Also the wording sounds a bit confusing to me, why not say "if the
>> command is larger than `MSGQ_MSG_SIZE_MAX`"?
>
> Do you mean in its own patch in this series, or sent as a separate patch
> to a different tree (e.g. rfl)?
Please send it with this series, it is small enough that we can probably
get an Acked-by and merge it through the DRM tree.
>
> Tangential but I feel like some of the errors in this file are a bit
> odd, like the existing EIO if the command doesn't fit (which it always
> should). WDYT?
There is certainly room for improvement - if you see room for improving
(or better, removing!) errors, please don't hesitate!
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 4/7] gpu: nova-core: gsp: clarify invariant on command queue
2026-02-12 6:28 [PATCH 0/7] gpu: nova-core: gsp: add continuation record support Eliot Courtney
` (2 preceding siblings ...)
2026-02-12 6:28 ` [PATCH 3/7] gpu: nova-core: gsp: add checking oversized commands Eliot Courtney
@ 2026-02-12 6:28 ` Eliot Courtney
2026-02-12 6:28 ` [PATCH 5/7] gpu: nova-core: gsp: unconditionally call variable payload handling Eliot Courtney
` (3 subsequent siblings)
7 siblings, 0 replies; 18+ messages in thread
From: Eliot Courtney @ 2026-02-12 6:28 UTC (permalink / raw)
To: Danilo Krummrich, Alice Ryhl, Alexandre Courbot, David Airlie,
Simona Vetter
Cc: nouveau, dri-devel, linux-kernel, Eliot Courtney
Clarify why using only the first returned slice from allocate_command
for the message headers is okay.
Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
---
drivers/gpu/nova-core/gsp/cmdq.rs | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
index dbc9e95f1b17..78f0b1a0e30c 100644
--- a/drivers/gpu/nova-core/gsp/cmdq.rs
+++ b/drivers/gpu/nova-core/gsp/cmdq.rs
@@ -542,7 +542,9 @@ pub(crate) fn send_command<M>(&mut self, bar: &Bar0, command: M) -> Result
let command_size = Self::command_size(&command);
let dst = self.gsp_mem.allocate_command_with_timeout(command_size)?;
- // Extract area for the command itself.
+ // Extract area for the command itself. The GSP message header and the command header
+ // together are guaranteed to fit entirely into a single page, so it's ok to only look
+ // at `dst.contents.0` here.
let (cmd, payload_1) = M::Command::from_bytes_mut_prefix(dst.contents.0).ok_or(EIO)?;
// Fill the header and command in-place.
--
2.53.0
^ permalink raw reply [flat|nested] 18+ messages in thread* [PATCH 5/7] gpu: nova-core: gsp: unconditionally call variable payload handling
2026-02-12 6:28 [PATCH 0/7] gpu: nova-core: gsp: add continuation record support Eliot Courtney
` (3 preceding siblings ...)
2026-02-12 6:28 ` [PATCH 4/7] gpu: nova-core: gsp: clarify invariant on command queue Eliot Courtney
@ 2026-02-12 6:28 ` Eliot Courtney
2026-02-12 6:28 ` [PATCH 6/7] gpu: nova-core: gsp: support large RPCs via continuation record Eliot Courtney
` (2 subsequent siblings)
7 siblings, 0 replies; 18+ messages in thread
From: Eliot Courtney @ 2026-02-12 6:28 UTC (permalink / raw)
To: Danilo Krummrich, Alice Ryhl, Alexandre Courbot, David Airlie,
Simona Vetter
Cc: nouveau, dri-devel, linux-kernel, Eliot Courtney
Unconditionally call the variable length payload code, which is a no-op
if there is no such payload but could defensively catch some coding
errors by e.g. checking that the allocated size is completely filled.
Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
---
drivers/gpu/nova-core/gsp/cmdq.rs | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
index 78f0b1a0e30c..3e9f88eec7cc 100644
--- a/drivers/gpu/nova-core/gsp/cmdq.rs
+++ b/drivers/gpu/nova-core/gsp/cmdq.rs
@@ -556,16 +556,14 @@ pub(crate) fn send_command<M>(&mut self, bar: &Bar0, command: M) -> Result
command.init().__init(core::ptr::from_mut(cmd))?;
}
- // Fill the variable-length payload.
- if command_size > size_of::<M::Command>() {
- let mut sbuffer =
- SBufferIter::new_writer([&mut payload_1[..], &mut dst.contents.1[..]]);
- command.init_variable_payload(&mut sbuffer)?;
-
- if !sbuffer.is_empty() {
- return Err(EIO);
- }
+ // Fill the variable-length payload, which may be empty.
+ let mut sbuffer = SBufferIter::new_writer([&mut payload_1[..], &mut dst.contents.1[..]]);
+ command.init_variable_payload(&mut sbuffer)?;
+
+ if !sbuffer.is_empty() {
+ return Err(EIO);
}
+ drop(sbuffer);
// Compute checksum now that the whole message is ready.
dst.header
--
2.53.0
^ permalink raw reply [flat|nested] 18+ messages in thread* [PATCH 6/7] gpu: nova-core: gsp: support large RPCs via continuation record
2026-02-12 6:28 [PATCH 0/7] gpu: nova-core: gsp: add continuation record support Eliot Courtney
` (4 preceding siblings ...)
2026-02-12 6:28 ` [PATCH 5/7] gpu: nova-core: gsp: unconditionally call variable payload handling Eliot Courtney
@ 2026-02-12 6:28 ` Eliot Courtney
2026-02-18 7:16 ` Alexandre Courbot
2026-02-12 6:28 ` [PATCH 7/7] gpu: nova-core: gsp: add tests for WrappingCommand Eliot Courtney
2026-02-17 18:18 ` [PATCH 0/7] gpu: nova-core: gsp: add continuation record support John Hubbard
7 siblings, 1 reply; 18+ messages in thread
From: Eliot Courtney @ 2026-02-12 6:28 UTC (permalink / raw)
To: Danilo Krummrich, Alice Ryhl, Alexandre Courbot, David Airlie,
Simona Vetter
Cc: nouveau, dri-devel, linux-kernel, Eliot Courtney
Splits large RPCs if necessary and sends the remaining parts using
continuation records. RPCs that do not need continuation records
continue to write directly into the command buffer. Ones that do write
into a staging buffer first, so there is one copy.
Continuation record for receive is not necessary to support at the
moment because those replies do not need to be read and are currently
drained by retrying `receive_msg` on ERANGE.
Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
---
drivers/gpu/nova-core/gsp/cmdq.rs | 47 ++++++++++++-
drivers/gpu/nova-core/gsp/commands.rs | 124 ++++++++++++++++++++++++++++++++++
drivers/gpu/nova-core/gsp/fw.rs | 5 ++
3 files changed, 173 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
index 3e9f88eec7cc..c24d813fc587 100644
--- a/drivers/gpu/nova-core/gsp/cmdq.rs
+++ b/drivers/gpu/nova-core/gsp/cmdq.rs
@@ -29,6 +29,10 @@
use crate::{
driver::Bar0,
gsp::{
+ commands::{
+ ContinuationRecord,
+ WrappingCommand, //
+ },
fw::{
GspMsgElement,
MsgFunction,
@@ -524,7 +528,7 @@ fn command_size<M>(command: &M) -> usize
size_of::<M::Command>() + command.variable_payload_len()
}
- /// Sends `command` to the GSP.
+ /// Sends `command` to the GSP, without splitting it.
///
/// # Errors
///
@@ -533,13 +537,13 @@ fn command_size<M>(command: &M) -> usize
/// written to by its [`CommandToGsp::init_variable_payload`] method.
///
/// Error codes returned by the command initializers are propagated as-is.
- pub(crate) fn send_command<M>(&mut self, bar: &Bar0, command: M) -> Result
+ fn send_single_command<M>(&mut self, bar: &Bar0, command: &M) -> Result
where
M: CommandToGsp,
// This allows all error types, including `Infallible`, to be used for `M::InitError`.
Error: From<M::InitError>,
{
- let command_size = Self::command_size(&command);
+ let command_size = Self::command_size(command);
let dst = self.gsp_mem.allocate_command_with_timeout(command_size)?;
// Extract area for the command itself. The GSP message header and the command header
@@ -590,6 +594,43 @@ pub(crate) fn send_command<M>(&mut self, bar: &Bar0, command: M) -> Result
Ok(())
}
+ fn send_continuation_record(&mut self, bar: &Bar0, cont: &ContinuationRecord<'_>) -> Result {
+ self.send_single_command(bar, cont)
+ }
+
+ /// Sends `command` to the GSP.
+ ///
+ /// The command may be split into multiple messages if it is large.
+ ///
+ /// # Errors
+ ///
+ /// - `ETIMEDOUT` if space does not become available within the timeout.
+ /// - `EIO` if the variable payload requested by the command has not been entirely
+ /// written to by its [`CommandToGsp::init_variable_payload`] method.
+ ///
+ /// Error codes returned by the command initializers are propagated as-is.
+ pub(crate) fn send_command<M>(&mut self, bar: &Bar0, command: M) -> Result
+ where
+ M: CommandToGsp,
+ Error: From<M::InitError>,
+ {
+ let msg_max_size = MSGQ_MSG_SIZE_MAX - size_of::<GspMsgElement>();
+ let mut wrapped = WrappingCommand::new(command, msg_max_size)?;
+
+ self.send_single_command(bar, &wrapped)?;
+
+ while let Some(continuation) = wrapped.next_continuation_record() {
+ dev_dbg!(
+ &self.dev,
+ "GSP RPC: send continuation: size=0x{:x}\n",
+ Self::command_size(&continuation),
+ );
+ self.send_continuation_record(bar, &continuation)?;
+ }
+
+ Ok(())
+ }
+
/// Wait for a message to become available on the message queue.
///
/// This works purely at the transport layer and does not interpret or validate the message
diff --git a/drivers/gpu/nova-core/gsp/commands.rs b/drivers/gpu/nova-core/gsp/commands.rs
index c8430a076269..99603880d56f 100644
--- a/drivers/gpu/nova-core/gsp/commands.rs
+++ b/drivers/gpu/nova-core/gsp/commands.rs
@@ -242,3 +242,127 @@ pub(crate) fn get_gsp_info(cmdq: &mut Cmdq, bar: &Bar0) -> Result<GetGspStaticIn
}
}
}
+
+#[derive(Zeroable)]
+pub(crate) struct Empty {}
+
+// SAFETY: `Empty` is a zero-sized type with no bytes, therefore it trivially has no uninitialized
+// bytes.
+unsafe impl AsBytes for Empty {}
+
+// SAFETY: `Empty` is a zero-sized type with no bytes, therefore it trivially has no uninitialized
+// bytes.
+unsafe impl FromBytes for Empty {}
+
+/// The `ContinuationRecord` command.
+pub(crate) struct ContinuationRecord<'a> {
+ data: &'a [u8],
+}
+
+impl<'a> ContinuationRecord<'a> {
+ /// Creates a new `ContinuationRecord` command with the given data.
+ pub(crate) fn new(data: &'a [u8]) -> Self {
+ Self { data }
+ }
+}
+
+impl<'a> CommandToGsp for ContinuationRecord<'a> {
+ const FUNCTION: MsgFunction = MsgFunction::ContinuationRecord;
+ type Command = Empty;
+ type InitError = Infallible;
+
+ fn init(&self) -> impl Init<Self::Command, Self::InitError> {
+ Empty::init_zeroed()
+ }
+
+ fn variable_payload_len(&self) -> usize {
+ self.data.len()
+ }
+
+ fn init_variable_payload(
+ &self,
+ dst: &mut SBufferIter<core::array::IntoIter<&mut [u8], 2>>,
+ ) -> Result {
+ dst.write_all(self.data)
+ }
+}
+
+/// Wrapper that splits a command across continuation records if needed.
+pub(crate) struct WrappingCommand<C: CommandToGsp> {
+ inner: C,
+ offset: usize,
+ max_size: usize,
+ staging: KVVec<u8>,
+}
+
+impl<C: CommandToGsp> WrappingCommand<C>
+where
+ Error: From<C::InitError>,
+{
+ /// Creates a new `WrappingCommand` that wraps `inner`, splitting it into
+ /// multiple messages if its size exceeds `max_size`.
+ pub(crate) fn new(inner: C, max_size: usize) -> Result<Self> {
+ let payload_len = inner.variable_payload_len();
+ let command_size = size_of::<C::Command>() + payload_len;
+ let (offset, staging) = if command_size > max_size {
+ let mut staging = KVVec::<u8>::from_elem(0u8, payload_len, GFP_KERNEL)?;
+ let mut sbuffer = SBufferIter::new_writer([staging.as_mut_slice(), &mut []]);
+ inner.init_variable_payload(&mut sbuffer)?;
+ if !sbuffer.is_empty() {
+ return Err(EIO);
+ }
+ drop(sbuffer);
+
+ (max_size - size_of::<C::Command>(), staging)
+ } else {
+ (0, KVVec::new())
+ };
+ Ok(Self {
+ inner,
+ offset,
+ max_size,
+ staging,
+ })
+ }
+
+ pub(crate) fn next_continuation_record(&mut self) -> Option<ContinuationRecord<'_>> {
+ let remaining = self.staging.len() - self.offset;
+ if remaining > 0 {
+ let chunk_size = remaining.min(self.max_size);
+ let record = ContinuationRecord::new(
+ &self.staging.as_slice()[self.offset..(self.offset + chunk_size)],
+ );
+ self.offset += chunk_size;
+ Some(record)
+ } else {
+ None
+ }
+ }
+}
+
+impl<C: CommandToGsp> CommandToGsp for WrappingCommand<C> {
+ const FUNCTION: MsgFunction = C::FUNCTION;
+ type Command = C::Command;
+ type InitError = C::InitError;
+
+ fn init(&self) -> impl Init<Self::Command, Self::InitError> {
+ self.inner.init()
+ }
+
+ fn variable_payload_len(&self) -> usize {
+ self.inner
+ .variable_payload_len()
+ .min(self.max_size - size_of::<C::Command>())
+ }
+
+ fn init_variable_payload(
+ &self,
+ dst: &mut SBufferIter<core::array::IntoIter<&mut [u8], 2>>,
+ ) -> Result {
+ if self.staging.is_empty() {
+ self.inner.init_variable_payload(dst)
+ } else {
+ dst.write_all(&self.staging.as_slice()[..self.variable_payload_len()])
+ }
+ }
+}
diff --git a/drivers/gpu/nova-core/gsp/fw.rs b/drivers/gpu/nova-core/gsp/fw.rs
index 5b96f5e622a4..5ceb070aa0d7 100644
--- a/drivers/gpu/nova-core/gsp/fw.rs
+++ b/drivers/gpu/nova-core/gsp/fw.rs
@@ -199,6 +199,7 @@ pub(crate) enum MsgFunction {
AllocObject = bindings::NV_VGPU_MSG_FUNCTION_ALLOC_OBJECT,
AllocRoot = bindings::NV_VGPU_MSG_FUNCTION_ALLOC_ROOT,
BindCtxDma = bindings::NV_VGPU_MSG_FUNCTION_BIND_CTX_DMA,
+ ContinuationRecord = bindings::NV_VGPU_MSG_FUNCTION_CONTINUATION_RECORD,
Free = bindings::NV_VGPU_MSG_FUNCTION_FREE,
GetGspStaticInfo = bindings::NV_VGPU_MSG_FUNCTION_GET_GSP_STATIC_INFO,
GetStaticInfo = bindings::NV_VGPU_MSG_FUNCTION_GET_STATIC_INFO,
@@ -234,6 +235,7 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
MsgFunction::AllocObject => write!(f, "ALLOC_OBJECT"),
MsgFunction::AllocRoot => write!(f, "ALLOC_ROOT"),
MsgFunction::BindCtxDma => write!(f, "BIND_CTX_DMA"),
+ MsgFunction::ContinuationRecord => write!(f, "CONTINUATION_RECORD"),
MsgFunction::Free => write!(f, "FREE"),
MsgFunction::GetGspStaticInfo => write!(f, "GET_GSP_STATIC_INFO"),
MsgFunction::GetStaticInfo => write!(f, "GET_STATIC_INFO"),
@@ -273,6 +275,9 @@ fn try_from(value: u32) -> Result<MsgFunction> {
bindings::NV_VGPU_MSG_FUNCTION_ALLOC_OBJECT => Ok(MsgFunction::AllocObject),
bindings::NV_VGPU_MSG_FUNCTION_ALLOC_ROOT => Ok(MsgFunction::AllocRoot),
bindings::NV_VGPU_MSG_FUNCTION_BIND_CTX_DMA => Ok(MsgFunction::BindCtxDma),
+ bindings::NV_VGPU_MSG_FUNCTION_CONTINUATION_RECORD => {
+ Ok(MsgFunction::ContinuationRecord)
+ }
bindings::NV_VGPU_MSG_FUNCTION_FREE => Ok(MsgFunction::Free),
bindings::NV_VGPU_MSG_FUNCTION_GET_GSP_STATIC_INFO => Ok(MsgFunction::GetGspStaticInfo),
bindings::NV_VGPU_MSG_FUNCTION_GET_STATIC_INFO => Ok(MsgFunction::GetStaticInfo),
--
2.53.0
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 6/7] gpu: nova-core: gsp: support large RPCs via continuation record
2026-02-12 6:28 ` [PATCH 6/7] gpu: nova-core: gsp: support large RPCs via continuation record Eliot Courtney
@ 2026-02-18 7:16 ` Alexandre Courbot
2026-02-18 9:00 ` Eliot Courtney
0 siblings, 1 reply; 18+ messages in thread
From: Alexandre Courbot @ 2026-02-18 7:16 UTC (permalink / raw)
To: Eliot Courtney
Cc: Danilo Krummrich, Alice Ryhl, David Airlie, Simona Vetter,
nouveau, dri-devel, linux-kernel
On Thu Feb 12, 2026 at 3:28 PM JST, Eliot Courtney wrote:
> Splits large RPCs if necessary and sends the remaining parts using
> continuation records. RPCs that do not need continuation records
> continue to write directly into the command buffer. Ones that do write
> into a staging buffer first, so there is one copy.
>
> Continuation record for receive is not necessary to support at the
> moment because those replies do not need to be read and are currently
> drained by retrying `receive_msg` on ERANGE.
Do they not need to be read at the moment, or is this a permanent thing?
I hope it's the latter. ^_^;
>
> Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
> ---
> drivers/gpu/nova-core/gsp/cmdq.rs | 47 ++++++++++++-
> drivers/gpu/nova-core/gsp/commands.rs | 124 ++++++++++++++++++++++++++++++++++
> drivers/gpu/nova-core/gsp/fw.rs | 5 ++
> 3 files changed, 173 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
> index 3e9f88eec7cc..c24d813fc587 100644
> --- a/drivers/gpu/nova-core/gsp/cmdq.rs
> +++ b/drivers/gpu/nova-core/gsp/cmdq.rs
> @@ -29,6 +29,10 @@
> use crate::{
> driver::Bar0,
> gsp::{
> + commands::{
> + ContinuationRecord,
> + WrappingCommand, //
> + },
> fw::{
> GspMsgElement,
> MsgFunction,
> @@ -524,7 +528,7 @@ fn command_size<M>(command: &M) -> usize
> size_of::<M::Command>() + command.variable_payload_len()
> }
>
> - /// Sends `command` to the GSP.
> + /// Sends `command` to the GSP, without splitting it.
> ///
> /// # Errors
> ///
> @@ -533,13 +537,13 @@ fn command_size<M>(command: &M) -> usize
> /// written to by its [`CommandToGsp::init_variable_payload`] method.
> ///
> /// Error codes returned by the command initializers are propagated as-is.
> - pub(crate) fn send_command<M>(&mut self, bar: &Bar0, command: M) -> Result
> + fn send_single_command<M>(&mut self, bar: &Bar0, command: &M) -> Result
> where
> M: CommandToGsp,
> // This allows all error types, including `Infallible`, to be used for `M::InitError`.
> Error: From<M::InitError>,
> {
> - let command_size = Self::command_size(&command);
> + let command_size = Self::command_size(command);
> let dst = self.gsp_mem.allocate_command_with_timeout(command_size)?;
>
> // Extract area for the command itself. The GSP message header and the command header
> @@ -590,6 +594,43 @@ pub(crate) fn send_command<M>(&mut self, bar: &Bar0, command: M) -> Result
> Ok(())
> }
>
> + fn send_continuation_record(&mut self, bar: &Bar0, cont: &ContinuationRecord<'_>) -> Result {
> + self.send_single_command(bar, cont)
> + }
This one-liner method out of the blue is a bit surprising, and the first
reflex is to try to inline it. Its reason to exist should be documented
(you mention it in the cover letter, but that's not visible after this
patch gets merged), but I'd lean towards removing it and just qualifying
its unique caller:
// Turbofish needed because the compiler cannot infer M here.
self.send_single_command::<ContinuationRecord<'_>>(bar, &continuation)?;
Fewer moving parts, and it makes it clear on the caller's side that a
continuation record is just another command, like the truncated one sent
before it.
> +
> + /// Sends `command` to the GSP.
> + ///
> + /// The command may be split into multiple messages if it is large.
> + ///
> + /// # Errors
> + ///
> + /// - `ETIMEDOUT` if space does not become available within the timeout.
> + /// - `EIO` if the variable payload requested by the command has not been entirely
> + /// written to by its [`CommandToGsp::init_variable_payload`] method.
> + ///
> + /// Error codes returned by the command initializers are propagated as-is.
> + pub(crate) fn send_command<M>(&mut self, bar: &Bar0, command: M) -> Result
> + where
> + M: CommandToGsp,
> + Error: From<M::InitError>,
> + {
> + let msg_max_size = MSGQ_MSG_SIZE_MAX - size_of::<GspMsgElement>();
This could be a const.
> + let mut wrapped = WrappingCommand::new(command, msg_max_size)?;
The `max_size` argument is a bit unfortunate as it is a constant for
non-testing purposes. Is there a way we can make testing work with the
same limit as real-world use? That would also make our testing closer to
reality, on top of making this argument unneeded.
> +
> + self.send_single_command(bar, &wrapped)?;
> +
> + while let Some(continuation) = wrapped.next_continuation_record() {
> + dev_dbg!(
> + &self.dev,
> + "GSP RPC: send continuation: size=0x{:x}\n",
> + Self::command_size(&continuation),
> + );
> + self.send_continuation_record(bar, &continuation)?;
> + }
Btw, can we recover if a split message fails between two continuation
records? I suspect the GSP will notice that the next message is not the
expected continuation record and recover from there?
> +
> + Ok(())
> + }
> +
> /// Wait for a message to become available on the message queue.
> ///
> /// This works purely at the transport layer and does not interpret or validate the message
> diff --git a/drivers/gpu/nova-core/gsp/commands.rs b/drivers/gpu/nova-core/gsp/commands.rs
> index c8430a076269..99603880d56f 100644
> --- a/drivers/gpu/nova-core/gsp/commands.rs
> +++ b/drivers/gpu/nova-core/gsp/commands.rs
> @@ -242,3 +242,127 @@ pub(crate) fn get_gsp_info(cmdq: &mut Cmdq, bar: &Bar0) -> Result<GetGspStaticIn
> }
> }
> }
> +
> +#[derive(Zeroable)]
> +pub(crate) struct Empty {}
> +
> +// SAFETY: `Empty` is a zero-sized type with no bytes, therefore it trivially has no uninitialized
> +// bytes.
> +unsafe impl AsBytes for Empty {}
> +
> +// SAFETY: `Empty` is a zero-sized type with no bytes, therefore it trivially has no uninitialized
> +// bytes.
> +unsafe impl FromBytes for Empty {}
Since commit 209c70953aa (on master, but not drm-rust-next yet) you can
just use `()` as the `Command` type - that will let you remove `Empty`
entirely. You'll then need to invoke `pin_init::init_zeroed()` in
`init`.
> +
> +/// The `ContinuationRecord` command.
> +pub(crate) struct ContinuationRecord<'a> {
> + data: &'a [u8],
> +}
> +
> +impl<'a> ContinuationRecord<'a> {
> + /// Creates a new `ContinuationRecord` command with the given data.
> + pub(crate) fn new(data: &'a [u8]) -> Self {
> + Self { data }
> + }
> +}
> +
> +impl<'a> CommandToGsp for ContinuationRecord<'a> {
> + const FUNCTION: MsgFunction = MsgFunction::ContinuationRecord;
> + type Command = Empty;
> + type InitError = Infallible;
> +
> + fn init(&self) -> impl Init<Self::Command, Self::InitError> {
> + Empty::init_zeroed()
> + }
> +
> + fn variable_payload_len(&self) -> usize {
> + self.data.len()
> + }
> +
> + fn init_variable_payload(
> + &self,
> + dst: &mut SBufferIter<core::array::IntoIter<&mut [u8], 2>>,
> + ) -> Result {
> + dst.write_all(self.data)
> + }
> +}
> +
> +/// Wrapper that splits a command across continuation records if needed.
> +pub(crate) struct WrappingCommand<C: CommandToGsp> {
> + inner: C,
> + offset: usize,
> + max_size: usize,
> + staging: KVVec<u8>,
Since it is conditionally-used, `staging` should be an `Option` instead
of assuming an empty state means it is unused. But hold on, I think we
can do without any sort of conditional here.
> +}
This deserves more doccomments, including on its members. But I would
also like to entertain a slightly different design.
In this patch, `WrappingCommand` is always used, including for messages
that don't need to be truncated. While the overhead is arguably
negligible, this makes split messages pass as the norm, while they are a
very rare exception.
Also `WrappingCommand` now becomes both a command and a provider of
other commands, which I find hard to wrap (haha) my head around, and
forces you to pass the command by reference in `send_single_command`
because you need to use it again afterwards. The name is also not very
descriptive (why does it wrap?). How about this instead:
`send_command` is the central path that all commands take, so it is the
right place to check whether we need to use continuation records. If we
don't, then we just send the command as-is, as we do today.
If we need to split, we do it through a private method of `Cmdq` that
consumes the command and returns a tuple `(SplitCommand<M>,
ContinuationRecords)`
`SplitCommand<M>` can be a wrapper around the original command, but that
initializes the truncated part of its variable payload.
`ContinuationRecords` implements `Iterator<Item = ContinuationRecord>`.
`ContinuationRecord` works mostly like your current version, except it
owns its data (yes, more allocations, but they're smaller so the
allocator might actually appreciate).
Since `SplitCommand` is only used when there is actually a split needed,
`staging` does not need an empty state anymore, and the code of each
type becomes simpler.
By doing so, you also don't need to pass the command by reference in
`send_single_command` anymore.
> +
> +impl<C: CommandToGsp> WrappingCommand<C>
> +where
> + Error: From<C::InitError>,
This `where` is unneeded.
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 6/7] gpu: nova-core: gsp: support large RPCs via continuation record
2026-02-18 7:16 ` Alexandre Courbot
@ 2026-02-18 9:00 ` Eliot Courtney
2026-02-18 11:49 ` Alexandre Courbot
0 siblings, 1 reply; 18+ messages in thread
From: Eliot Courtney @ 2026-02-18 9:00 UTC (permalink / raw)
To: Alexandre Courbot, Eliot Courtney
Cc: Danilo Krummrich, Alice Ryhl, Simona Vetter, nouveau, dri-devel,
linux-kernel
On Wed Feb 18, 2026 at 4:16 PM JST, Alexandre Courbot wrote:
>> + /// Sends `command` to the GSP.
>> + ///
>> + /// The command may be split into multiple messages if it is large.
>> + ///
>> + /// # Errors
>> + ///
>> + /// - `ETIMEDOUT` if space does not become available within the timeout.
>> + /// - `EIO` if the variable payload requested by the command has not been entirely
>> + /// written to by its [`CommandToGsp::init_variable_payload`] method.
>> + ///
>> + /// Error codes returned by the command initializers are propagated as-is.
>> + pub(crate) fn send_command<M>(&mut self, bar: &Bar0, command: M) -> Result
>> + where
>> + M: CommandToGsp,
>> + Error: From<M::InitError>,
>> + {
>> + let msg_max_size = MSGQ_MSG_SIZE_MAX - size_of::<GspMsgElement>();
>
> This could be a const.
>
>> + let mut wrapped = WrappingCommand::new(command, msg_max_size)?;
>
> The `max_size` argument is a bit unfortunate as it is a constant for
> non-testing purposes. Is there a way we can make testing work with the
> same limit as real-world use? That would also make our testing closer to
> reality, on top of making this argument unneeded.
Yeah good point, will do.
>
>> +
>> + self.send_single_command(bar, &wrapped)?;
>> +
>> + while let Some(continuation) = wrapped.next_continuation_record() {
>> + dev_dbg!(
>> + &self.dev,
>> + "GSP RPC: send continuation: size=0x{:x}\n",
>> + Self::command_size(&continuation),
>> + );
>> + self.send_continuation_record(bar, &continuation)?;
>> + }
>
> Btw, can we recover if a split message fails between two continuation
> records? I suspect the GSP will notice that the next message is not the
> expected continuation record and recover from there?
IIUC neither openrm or nouveau can recover from a failure during sending
continuation records. What failure mode do you see happening that we
could recover from?
>> +#[derive(Zeroable)]
>> +pub(crate) struct Empty {}
>> +
>> +// SAFETY: `Empty` is a zero-sized type with no bytes, therefore it trivially has no uninitialized
>> +// bytes.
>> +unsafe impl AsBytes for Empty {}
>> +
>> +// SAFETY: `Empty` is a zero-sized type with no bytes, therefore it trivially has no uninitialized
>> +// bytes.
>> +unsafe impl FromBytes for Empty {}
>
> Since commit 209c70953aa (on master, but not drm-rust-next yet) you can
> just use `()` as the `Command` type - that will let you remove `Empty`
> entirely. You'll then need to invoke `pin_init::init_zeroed()` in
> `init`.
Great!
>> +
>> +/// The `ContinuationRecord` command.
>> +pub(crate) struct ContinuationRecord<'a> {
>> + data: &'a [u8],
>> +}
>> +
>> +impl<'a> ContinuationRecord<'a> {
>> + /// Creates a new `ContinuationRecord` command with the given data.
>> + pub(crate) fn new(data: &'a [u8]) -> Self {
>> + Self { data }
>> + }
>> +}
>> +
>> +impl<'a> CommandToGsp for ContinuationRecord<'a> {
>> + const FUNCTION: MsgFunction = MsgFunction::ContinuationRecord;
>> + type Command = Empty;
>> + type InitError = Infallible;
>> +
>> + fn init(&self) -> impl Init<Self::Command, Self::InitError> {
>> + Empty::init_zeroed()
>> + }
>> +
>> + fn variable_payload_len(&self) -> usize {
>> + self.data.len()
>> + }
>> +
>> + fn init_variable_payload(
>> + &self,
>> + dst: &mut SBufferIter<core::array::IntoIter<&mut [u8], 2>>,
>> + ) -> Result {
>> + dst.write_all(self.data)
>> + }
>> +}
>> +
>> +/// Wrapper that splits a command across continuation records if needed.
>> +pub(crate) struct WrappingCommand<C: CommandToGsp> {
>> + inner: C,
>> + offset: usize,
>> + max_size: usize,
>> + staging: KVVec<u8>,
>
> Since it is conditionally-used, `staging` should be an `Option` instead
> of assuming an empty state means it is unused. But hold on, I think we
> can do without any sort of conditional here.
Yeah you are right. I had used empty here since it makes
`next_continuation_record` here more consistent in that it doesn't need
to check for None then for length anyway, but it's special casing in
`init_variable_payload` in a very optiony way regardless.
I reckon using Option we could put `offset` in there as well in a tuple,
since if we want to use Option to avoid the special case of empty
`staging`, we might as well use it to avoid having a conceptually
useless `offset` field too.
>
>> +}
>
> This deserves more doccomments, including on its members. But I would
> also like to entertain a slightly different design.
>
> In this patch, `WrappingCommand` is always used, including for messages
> that don't need to be truncated. While the overhead is arguably
> negligible, this makes split messages pass as the norm, while they are a
> very rare exception.
>
> Also `WrappingCommand` now becomes both a command and a provider of
> other commands, which I find hard to wrap (haha) my head around, and
> forces you to pass the command by reference in `send_single_command`
> because you need to use it again afterwards. The name is also not very
> descriptive (why does it wrap?). How about this instead:
>
> `send_command` is the central path that all commands take, so it is the
> right place to check whether we need to use continuation records. If we
> don't, then we just send the command as-is, as we do today.
>
> If we need to split, we do it through a private method of `Cmdq` that
> consumes the command and returns a tuple `(SplitCommand<M>,
> ContinuationRecords)`
>
> `SplitCommand<M>` can be a wrapper around the original command, but that
> initializes the truncated part of its variable payload.
>
> `ContinuationRecords` implements `Iterator<Item = ContinuationRecord>`.
> `ContinuationRecord` works mostly like your current version, except it
> owns its data (yes, more allocations, but they're smaller so the
> allocator might actually appreciate).
Is it really the case that more, smaller allocations is better if we are
using KVVec rather than KVec? It feels like it would have
overhead/fragmentation to me. Eager to learn if this is wrong though
>
> Since `SplitCommand` is only used when there is actually a split needed,
> `staging` does not need an empty state anymore, and the code of each
> type becomes simpler.
>
> By doing so, you also don't need to pass the command by reference in
> `send_single_command` anymore.
I considered putting the continuation record logic inside cmdq, but I
feel it's nicer to keep it out of the core command queue logic as much
as possible, because it feels a bit noisy to have in the cmdq which
hopefully can just concentrate on sending messages, not how those
messages get made or how/when they get split.
I agree that it is a bit odd to have a command that also provides extra
commands. By the way I considered doing an Iterator thing for
WrappingCommand, but felt like it would introduce more complexity
than it was worth and make it more confusing on the command that also
produces commands front.
Another benefit to pulling the logic out of command queue is that it
makes it a lot easier to test AFAICT. If we want to test the splitting,
if the logic is a private method in the command queue that feels like it
might end up more as an integration test.
But if we just want to avoid having a command which also produces
commands, what if we created a separate type to hold the split state /
decision and it would return a SplitCommand plus either an iterator or
the iterator-like `next_continuation_record`, while being testable
separately. WDYT?
>
>> +
>> +impl<C: CommandToGsp> WrappingCommand<C>
>> +where
>> + Error: From<C::InitError>,
>
> This `where` is unneeded.
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 6/7] gpu: nova-core: gsp: support large RPCs via continuation record
2026-02-18 9:00 ` Eliot Courtney
@ 2026-02-18 11:49 ` Alexandre Courbot
0 siblings, 0 replies; 18+ messages in thread
From: Alexandre Courbot @ 2026-02-18 11:49 UTC (permalink / raw)
To: Eliot Courtney
Cc: Danilo Krummrich, Alice Ryhl, Simona Vetter, nouveau, dri-devel,
linux-kernel
On Wed Feb 18, 2026 at 6:00 PM JST, Eliot Courtney wrote:
<snip>
>>> +
>>> + self.send_single_command(bar, &wrapped)?;
>>> +
>>> + while let Some(continuation) = wrapped.next_continuation_record() {
>>> + dev_dbg!(
>>> + &self.dev,
>>> + "GSP RPC: send continuation: size=0x{:x}\n",
>>> + Self::command_size(&continuation),
>>> + );
>>> + self.send_continuation_record(bar, &continuation)?;
>>> + }
>>
>> Btw, can we recover if a split message fails between two continuation
>> records? I suspect the GSP will notice that the next message is not the
>> expected continuation record and recover from there?
>
> IIUC neither openrm or nouveau can recover from a failure during sending
> continuation records. What failure mode do you see happening that we
> could recover from?
None in particular, I just want to confirm we have thought about it. If
OpenRM considers this a catastrophic failure, doing the same in Nova is
fair enough.
<snip>
>>> +
>>> +/// The `ContinuationRecord` command.
>>> +pub(crate) struct ContinuationRecord<'a> {
>>> + data: &'a [u8],
>>> +}
>>> +
>>> +impl<'a> ContinuationRecord<'a> {
>>> + /// Creates a new `ContinuationRecord` command with the given data.
>>> + pub(crate) fn new(data: &'a [u8]) -> Self {
>>> + Self { data }
>>> + }
>>> +}
>>> +
>>> +impl<'a> CommandToGsp for ContinuationRecord<'a> {
>>> + const FUNCTION: MsgFunction = MsgFunction::ContinuationRecord;
>>> + type Command = Empty;
>>> + type InitError = Infallible;
>>> +
>>> + fn init(&self) -> impl Init<Self::Command, Self::InitError> {
>>> + Empty::init_zeroed()
>>> + }
>>> +
>>> + fn variable_payload_len(&self) -> usize {
>>> + self.data.len()
>>> + }
>>> +
>>> + fn init_variable_payload(
>>> + &self,
>>> + dst: &mut SBufferIter<core::array::IntoIter<&mut [u8], 2>>,
>>> + ) -> Result {
>>> + dst.write_all(self.data)
>>> + }
>>> +}
>>> +
>>> +/// Wrapper that splits a command across continuation records if needed.
>>> +pub(crate) struct WrappingCommand<C: CommandToGsp> {
>>> + inner: C,
>>> + offset: usize,
>>> + max_size: usize,
>>> + staging: KVVec<u8>,
>>
>> Since it is conditionally-used, `staging` should be an `Option` instead
>> of assuming an empty state means it is unused. But hold on, I think we
>> can do without any sort of conditional here.
>
> Yeah you are right. I had used empty here since it makes
> `next_continuation_record` here more consistent in that it doesn't need
> to check for None then for length anyway, but it's special casing in
> `init_variable_payload` in a very optiony way regardless.
>
> I reckon using Option we could put `offset` in there as well in a tuple,
> since if we want to use Option to avoid the special case of empty
> `staging`, we might as well use it to avoid having a conceptually
> useless `offset` field too.
All the more reason not to handle the non-continuation case in this
type. Let's keep it straightforward and responsible for only doing the
splitting of messages - if we indeed need another type to be able to
handle both cases, we can then compose one using an enum.
>
>>
>>> +}
>>
>> This deserves more doccomments, including on its members. But I would
>> also like to entertain a slightly different design.
>>
>> In this patch, `WrappingCommand` is always used, including for messages
>> that don't need to be truncated. While the overhead is arguably
>> negligible, this makes split messages pass as the norm, while they are a
>> very rare exception.
>>
>> Also `WrappingCommand` now becomes both a command and a provider of
>> other commands, which I find hard to wrap (haha) my head around, and
>> forces you to pass the command by reference in `send_single_command`
>> because you need to use it again afterwards. The name is also not very
>> descriptive (why does it wrap?). How about this instead:
>>
>> `send_command` is the central path that all commands take, so it is the
>> right place to check whether we need to use continuation records. If we
>> don't, then we just send the command as-is, as we do today.
>>
>> If we need to split, we do it through a private method of `Cmdq` that
>> consumes the command and returns a tuple `(SplitCommand<M>,
>> ContinuationRecords)`
>>
>> `SplitCommand<M>` can be a wrapper around the original command, but that
>> initializes the truncated part of its variable payload.
>>
>> `ContinuationRecords` implements `Iterator<Item = ContinuationRecord>`.
>> `ContinuationRecord` works mostly like your current version, except it
>> owns its data (yes, more allocations, but they're smaller so the
>> allocator might actually appreciate).
>
> Is it really the case that more, smaller allocations is better if we are
> using KVVec rather than KVec? It feels like it would have
> overhead/fragmentation to me. Eager to learn if this is wrong though
Ah, for some reason I read KVec here. KVVec is fine, and thinking about
it, we don't even need one allocation per continuation record: just one
for the main command, and one for the continuation records iterator
type, which the individual `ContinuationRecord`s can refer to using a
slice (basically, your current design).
>
>>
>> Since `SplitCommand` is only used when there is actually a split needed,
>> `staging` does not need an empty state anymore, and the code of each
>> type becomes simpler.
>>
>> By doing so, you also don't need to pass the command by reference in
>> `send_single_command` anymore.
>
> I considered putting the continuation record logic inside cmdq, but I
> feel it's nicer to keep it out of the core command queue logic as much
> as possible, because it feels a bit noisy to have in the cmdq which
> hopefully can just concentrate on sending messages, not how those
> messages get made or how/when they get split.
Fair enough. Are you suggesting that we use a new type instead of a
`Cmdq` function? If so, that looks good to me, it's basically turning
the tuple return value I suggested into its own type with a constructor.
That's arguably cleaner actually.
>
> I agree that it is a bit odd to have a command that also provides extra
> commands. By the way I considered doing an Iterator thing for
> WrappingCommand, but felt like it would introduce more complexity
> than it was worth and make it more confusing on the command that also
> produces commands front.
Ah, with the `ContinuationRecord`s borrowing from `self` it is difficult
to implement `Iterator` on `WrappingCommand` indeed. The current method
is fine, it's preferable to doing another copy and thankfully we don't
need the full power of iterators here.
>
> Another benefit to pulling the logic out of command queue is that it
> makes it a lot easier to test AFAICT. If we want to test the splitting,
> if the logic is a private method in the command queue that feels like it
> might end up more as an integration test.
>
> But if we just want to avoid having a command which also produces
> commands, what if we created a separate type to hold the split state /
> decision and it would return a SplitCommand plus either an iterator or
> the iterator-like `next_continuation_record`, while being testable
> separately. WDYT?
So basically, the simplified `SplitCommand` and its continuation
records, which we wrap into an enum holding the decision of whether the
command is split or not? That sounds fine.
I'd like to also keep the continuation records outside of
`SplitCommand`, as this will let us pass the commands by value to
`send_command`, which is a detail I like as it ensures we cannot send
the same command twice without explicitly cloning it before.
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 7/7] gpu: nova-core: gsp: add tests for WrappingCommand
2026-02-12 6:28 [PATCH 0/7] gpu: nova-core: gsp: add continuation record support Eliot Courtney
` (5 preceding siblings ...)
2026-02-12 6:28 ` [PATCH 6/7] gpu: nova-core: gsp: support large RPCs via continuation record Eliot Courtney
@ 2026-02-12 6:28 ` Eliot Courtney
2026-02-17 18:18 ` [PATCH 0/7] gpu: nova-core: gsp: add continuation record support John Hubbard
7 siblings, 0 replies; 18+ messages in thread
From: Eliot Courtney @ 2026-02-12 6:28 UTC (permalink / raw)
To: Danilo Krummrich, Alice Ryhl, Alexandre Courbot, David Airlie,
Simona Vetter
Cc: nouveau, dri-devel, linux-kernel, Eliot Courtney
Add tests for WrappingCommand. They cover boundary conditions at the
split points to make sure the right number of continuation records are
made. They also check that the data concatenated is correct.
Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
---
drivers/gpu/nova-core/gsp/commands.rs | 123 ++++++++++++++++++++++++++++++++++
1 file changed, 123 insertions(+)
diff --git a/drivers/gpu/nova-core/gsp/commands.rs b/drivers/gpu/nova-core/gsp/commands.rs
index 99603880d56f..9a41c4a9d90d 100644
--- a/drivers/gpu/nova-core/gsp/commands.rs
+++ b/drivers/gpu/nova-core/gsp/commands.rs
@@ -366,3 +366,126 @@ fn init_variable_payload(
}
}
}
+
+#[kunit_tests(nova_core_gsp_commands)]
+mod tests {
+ use super::*;
+
+ struct TestPayload {
+ data: KVVec<u8>,
+ }
+
+ impl TestPayload {
+ fn generate_pattern(len: usize) -> Result<KVVec<u8>> {
+ let mut data = KVVec::with_capacity(len, GFP_KERNEL)?;
+ for i in 0..len {
+ data.push(i as u8, GFP_KERNEL)?;
+ }
+ Ok(data)
+ }
+
+ fn new(len: usize) -> Result<Self> {
+ Ok(Self {
+ data: Self::generate_pattern(len)?,
+ })
+ }
+ }
+
+ impl CommandToGsp for TestPayload {
+ const FUNCTION: MsgFunction = MsgFunction::Nop;
+ type Command = Empty;
+ type InitError = Infallible;
+
+ fn init(&self) -> impl Init<Self::Command, Self::InitError> {
+ Empty::init_zeroed()
+ }
+
+ fn variable_payload_len(&self) -> usize {
+ self.data.len()
+ }
+
+ fn init_variable_payload(
+ &self,
+ dst: &mut SBufferIter<core::array::IntoIter<&mut [u8], 2>>,
+ ) -> Result {
+ dst.write_all(self.data.as_slice())
+ }
+ }
+
+ fn read_payload(cmd: &impl CommandToGsp) -> Result<KVVec<u8>> {
+ let len = cmd.variable_payload_len();
+ let mut buf = KVVec::from_elem(0u8, len, GFP_KERNEL)?;
+ let mut sbuf = SBufferIter::new_writer([buf.as_mut_slice(), &mut []]);
+ cmd.init_variable_payload(&mut sbuf)?;
+ drop(sbuf);
+ Ok(buf)
+ }
+
+ struct WrappingCommandTest {
+ payload_size: usize,
+ max_size: usize,
+ num_continuations: usize,
+ }
+
+ fn check_wrapping(t: WrappingCommandTest) -> Result {
+ let mut wrapped = WrappingCommand::new(TestPayload::new(t.payload_size)?, t.max_size)?;
+
+ let mut buf = read_payload(&wrapped)?;
+ assert!(buf.len() <= t.max_size);
+
+ let mut num_continuations = 0;
+ while let Some(cont) = wrapped.next_continuation_record() {
+ let payload = read_payload(&cont)?;
+ assert!(payload.len() <= t.max_size);
+ buf.extend_from_slice(&payload, GFP_KERNEL)?;
+ num_continuations += 1;
+ }
+
+ assert_eq!(num_continuations, t.num_continuations);
+ assert_eq!(
+ buf.as_slice(),
+ TestPayload::generate_pattern(t.payload_size)?.as_slice()
+ );
+ Ok(())
+ }
+
+ #[test]
+ fn wrapping_command() -> Result {
+ check_wrapping(WrappingCommandTest {
+ payload_size: 0,
+ max_size: 50,
+ num_continuations: 0,
+ })?;
+ check_wrapping(WrappingCommandTest {
+ payload_size: 50,
+ max_size: 50,
+ num_continuations: 0,
+ })?;
+ check_wrapping(WrappingCommandTest {
+ payload_size: 100,
+ max_size: 50,
+ num_continuations: 1,
+ })?;
+ check_wrapping(WrappingCommandTest {
+ payload_size: 101,
+ max_size: 100,
+ num_continuations: 1,
+ })?;
+ check_wrapping(WrappingCommandTest {
+ payload_size: 150,
+ max_size: 100,
+ num_continuations: 1,
+ })?;
+ check_wrapping(WrappingCommandTest {
+ payload_size: 300,
+ max_size: 100,
+ num_continuations: 2,
+ })?;
+ check_wrapping(WrappingCommandTest {
+ payload_size: 350,
+ max_size: 100,
+ num_continuations: 3,
+ })?;
+ Ok(())
+ }
+}
--
2.53.0
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 0/7] gpu: nova-core: gsp: add continuation record support
2026-02-12 6:28 [PATCH 0/7] gpu: nova-core: gsp: add continuation record support Eliot Courtney
` (6 preceding siblings ...)
2026-02-12 6:28 ` [PATCH 7/7] gpu: nova-core: gsp: add tests for WrappingCommand Eliot Courtney
@ 2026-02-17 18:18 ` John Hubbard
7 siblings, 0 replies; 18+ messages in thread
From: John Hubbard @ 2026-02-17 18:18 UTC (permalink / raw)
To: Eliot Courtney, Danilo Krummrich, Alice Ryhl, Alexandre Courbot,
David Airlie, Simona Vetter
Cc: nouveau, dri-devel, linux-kernel
On 2/11/26 10:28 PM, Eliot Courtney wrote:
> GSP commands over 16 pages need to be sent using "continuation records"
> which essentially means splitting the payload over multiple commands.
Let's please include rust-for-linux on Cc for Nova patches, for a while
longer at least.
thanks,
John Hubbard
>
> This series adds a command type `ContinuationRecord` which just writes
> its header and whatever payload it is given. It also adds a type
> `WrappingCommand` which supports splitting a large RPC into smaller
> ones.
>
> The send pathway uses `WrappingCommand` to send all commands, but if
> the command fits into 16 pages, it still writes directly into the
> command queue. If it is larger than 16 pages and needs continuation
> records, it writes into a staging buffer, so there is one copy.
>
> This patch series uses EIO for mis-sized commands to be consistent with
> the rest of the code.
>
> Patch 6 introduces a single helper send_continuation_record. This is
> because the compiler can't properly infer the types without this.
>
> Signed-off-by: Eliot Courtney <ecourtney@nvidia.com>
> ---
> Eliot Courtney (7):
> gpu: nova-core: gsp: sort MsgFunction variants alphabetically
> gpu: nova-core: gsp: add mechanism to wait for space on command queue
> gpu: nova-core: gsp: add checking oversized commands
> gpu: nova-core: gsp: clarify invariant on command queue
> gpu: nova-core: gsp: unconditionally call variable payload handling
> gpu: nova-core: gsp: support large RPCs via continuation record
> gpu: nova-core: gsp: add tests for WrappingCommand
>
> drivers/gpu/nova-core/gsp/cmdq.rs | 113 ++++++++++++++--
> drivers/gpu/nova-core/gsp/commands.rs | 247 ++++++++++++++++++++++++++++++++++
> drivers/gpu/nova-core/gsp/fw.rs | 102 +++++++-------
> 3 files changed, 401 insertions(+), 61 deletions(-)
> ---
> base-commit: cea7b66a80412e2a5b74627b89ae25f1d0110a4b
> change-id: 20260203-cmdq-continuation-b99f3d5966c3
>
> Best regards,
^ permalink raw reply [flat|nested] 18+ messages in thread