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 <[email protected]> > --- > 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?
