Message-Id: <DD2JYDPBOKA8.2QCK0P7CR1T3V@nvidia.com>
Date: Fri, 26 Sep 2025 16:27:04 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "Alistair Popple" <apopple@...dia.com>,
 <rust-for-linux@...r.kernel.org>, <dri-devel@...ts.freedesktop.org>,
 <dakr@...nel.org>, <acourbot@...dia.com>
Cc: "Miguel Ojeda" <ojeda@...nel.org>, "Alex Gaynor"
 <alex.gaynor@...il.com>, "Boqun Feng" <boqun.feng@...il.com>, "Gary Guo"
 <gary@...yguo.net>, Björn Roy Baron
 <bjorn3_gh@...tonmail.com>, "Benno Lossin" <lossin@...nel.org>, "Andreas
 Hindborg" <a.hindborg@...nel.org>, "Alice Ryhl" <aliceryhl@...gle.com>,
 "Trevor Gross" <tmgross@...ch.edu>, "David Airlie" <airlied@...il.com>,
 "Simona Vetter" <simona@...ll.ch>, "Maarten Lankhorst"
 <maarten.lankhorst@...ux.intel.com>, "Maxime Ripard" <mripard@...nel.org>,
 "Thomas Zimmermann" <tzimmermann@...e.de>, "John Hubbard"
 <jhubbard@...dia.com>, "Joel Fernandes" <joelagnelf@...dia.com>, "Timur
 Tabi" <ttabi@...dia.com>, <linux-kernel@...r.kernel.org>,
 <nouveau@...ts.freedesktop.org>
Subject: Re: [PATCH v2 06/10] gpu: nova-core: gsp: Create rmargs

On Mon Sep 22, 2025 at 8:30 PM JST, Alistair Popple wrote:
> Initialise the GSP resource manager arguments (rmargs) which provide
> initialisation parameters to the GSP firmware during boot. The rmargs
> structure contains arguments to configure the GSP message/command queue
> location.
>
> These are mapped for coherent DMA and added to the libos data structure
> for access when booting GSP.
>
> Signed-off-by: Alistair Popple <apopple@...dia.com>
>
> ---
>
> Changes for v2:
>  - Rebased on Alex's latest series
> ---
>  drivers/gpu/nova-core/gsp.rs                  | 29 +++++++++++++++-
>  drivers/gpu/nova-core/gsp/cmdq.rs             | 14 ++++++--
>  drivers/gpu/nova-core/gsp/fw.rs               | 19 +++++++++++
>  .../gpu/nova-core/gsp/fw/r570_144/bindings.rs | 33 +++++++++++++++++++
>  4 files changed, 91 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/nova-core/gsp.rs b/drivers/gpu/nova-core/gsp.rs
> index 3d4028d67d2e..bb08bd537ec4 100644
> --- a/drivers/gpu/nova-core/gsp.rs
> +++ b/drivers/gpu/nova-core/gsp.rs
> @@ -17,7 +17,10 @@
>  use crate::fb::FbLayout;
>  use crate::gsp::cmdq::GspCmdq;
>  
> -use fw::LibosMemoryRegionInitArgument;
> +use fw::{
> +    LibosMemoryRegionInitArgument, GSP_ARGUMENTS_CACHED, GSP_SR_INIT_ARGUMENTS,
> +    MESSAGE_QUEUE_INIT_ARGUMENTS,
> +};
>  
>  pub(crate) mod cmdq;
>  
> @@ -33,6 +36,7 @@ pub(crate) struct Gsp {
>      pub logintr: CoherentAllocation<u8>,
>      pub logrm: CoherentAllocation<u8>,
>      pub cmdq: GspCmdq,
> +    rmargs: CoherentAllocation<GSP_ARGUMENTS_CACHED>,
>  }
>  
>  /// Creates a self-mapping page table for `obj` at its beginning.
> @@ -90,12 +94,35 @@ pub(crate) fn new(pdev: &pci::Device<device::Bound>) -> Result<impl PinInit<Self
>  
>          // Creates its own PTE array
>          let cmdq = GspCmdq::new(dev)?;
> +        let rmargs =
> +            create_coherent_dma_object::<GSP_ARGUMENTS_CACHED>(dev, "RMARGS", 1, &mut libos, 3)?;
> +        let (shared_mem_phys_addr, cmd_queue_offset, stat_queue_offset) = cmdq.get_cmdq_offsets();
> +
> +        dma_write!(
> +            rmargs[0].messageQueueInitArguments = MESSAGE_QUEUE_INIT_ARGUMENTS {
> +                sharedMemPhysAddr: shared_mem_phys_addr,
> +                pageTableEntryCount: cmdq.nr_ptes,
> +                cmdQueueOffset: cmd_queue_offset,
> +                statQueueOffset: stat_queue_offset,
> +                ..Default::default()
> +            }
> +        )?;
> +        dma_write!(
> +            rmargs[0].srInitArguments = GSP_SR_INIT_ARGUMENTS {
> +                oldLevel: 0,
> +                flags: 0,
> +                bInPMTransition: 0,
> +                ..Default::default()
> +            }
> +        )?;
> +        dma_write!(rmargs[0].bDmemStack = 1)?;

Wrapping our bindings is going to help clean up this code as well.

First, types named in CAPITALS_SNAKE_CASE are not idiomatic Rust and
look like constants. And it's not even like the bindings types are
consistently named that way, since we also have e.g. `GspFwWprMeta` - so
let's give them a proper public name and bring some consistency at the
same time.

This will make all the fields from `GSP_ARGUMENTS_CACHED` invisible to
this module as they should be, so the wrapping `GspArgumentsCached` type
should then have a constructor that receives a referene to the command
queue and takes the information is needs from it, similarly to
`GspFwWprMeta`. This will reduce the 3 `dma_write!` into a single one.

Then we should remove `get_cmdq_offsets`, which is super confusing. I am
also not fond of `cmdq.nr_ptes`. More on them below.

>  
>          Ok(try_pin_init!(Self {
>              libos,
>              loginit,
>              logintr,
>              logrm,
> +            rmargs,
>              cmdq,
>          }))
>      }
> diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
> index a9ba1a4c73d8..9170ccf4a064 100644
> --- a/drivers/gpu/nova-core/gsp/cmdq.rs
> +++ b/drivers/gpu/nova-core/gsp/cmdq.rs
> @@ -99,7 +99,6 @@ fn new(dev: &device::Device<device::Bound>) -> Result<Self> {
>          Ok(Self(gsp_mem))
>      }
>  
> -    #[expect(unused)]
>      fn dma_handle(&self) -> DmaAddress {
>          self.0.dma_handle()
>      }
> @@ -218,7 +217,7 @@ pub(crate) struct GspCmdq {
>      dev: ARef<device::Device>,
>      seq: u32,
>      gsp_mem: DmaGspMem,
> -    pub _nr_ptes: u32,
> +    pub nr_ptes: u32,
>  }
>  
>  impl GspCmdq {
> @@ -231,7 +230,7 @@ pub(crate) fn new(dev: &device::Device<device::Bound>) -> Result<GspCmdq> {
>              dev: dev.into(),
>              seq: 0,
>              gsp_mem,
> -            _nr_ptes: nr_ptes as u32,
> +            nr_ptes: nr_ptes as u32,
>          })
>      }
>  
> @@ -382,6 +381,15 @@ pub(crate) fn receive_msg_from_gsp<M: GspMessageFromGsp, R>(
>              .advance_cpu_read_ptr(msg_header.rpc.length.div_ceil(GSP_PAGE_SIZE as u32));
>          result
>      }
> +
> +    pub(crate) fn get_cmdq_offsets(&self) -> (u64, u64, u64) {
> +        (
> +            self.gsp_mem.dma_handle(),
> +            core::mem::offset_of!(Msgq, msgq) as u64,
> +            (core::mem::offset_of!(GspMem, gspq) - core::mem::offset_of!(GspMem, cpuq)
> +                + core::mem::offset_of!(Msgq, msgq)) as u64,
> +        )
> +    }

So this thing returns 3 u64s, one of which is actually a DMA handle,
while the two others are technically constants. The only thing that
needs to be inferred at runtime is the DMA handle - all the rest is
static.

So we can make the two last returned values associated constants of
`GspCmdq`:

  impl GspCmdq {
      /// Offset of the data after the PTEs.
      const POST_PTE_OFFSET: usize = core::mem::offset_of!(GspMem, cpuq);

      /// Offset of command queue ring buffer.
      pub(crate) const CMDQ_OFFSET: usize = core::mem::offset_of!(GspMem, cpuq)
          + core::mem::offset_of!(Msgq, msgq)
          - Self::POST_PTE_OFFSET;

      /// Offset of message queue ring buffer.
      pub(crate) const STATQ_OFFSET: usize = core::mem::offset_of!(GspMem, gspq)
          + core::mem::offset_of!(Msgq, msgq)
          - Self::POST_PTE_OFFSET;

`GspArgumentsCached::new` can then import `GspCmdq` and use these to
initialize its corresponding members.

Remains `nr_ptes`. It was introduced in the previous patch as follows:

    let nr_ptes = size_of::<GspMem>() >> GSP_PAGE_SHIFT;

Which turns out to also be a constant! So let's add it next to the others:

impl GspCmdq {
    ...
    /// Number of page table entries for the GSP shared region.
    pub(crate) const NUM_PTES: usize = size_of::<GspMem>() >> GSP_PAGE_SHIFT;

And you can remove `GspCmdq::nr_ptes` altogether.

With this, `GspArgumentsCached::new` can take a reference to the
`GspCmdq` to initialize from, grab its DMA handle, and initialize
everything else using the constants we defined above. We remove a bunch
of inconsistently-named imports from `gsp.rs`, and replace
firmware-dependent incantations to initialize our GSP arguments with a
single constructor call that tells exactly what it does in a single
line.

Powered by blists - more mailing lists