Message-ID: <q2ehvle73bvop6muga44cebwzgpm2g5tghf2txq2orvgsaryh2@hfmxjcymhsrl>
Date: Mon, 29 Sep 2025 16:36:55 +1000
From: Alistair Popple <apopple@...dia.com>
To: Alexandre Courbot <acourbot@...dia.com>
Cc: rust-for-linux@...r.kernel.org, dri-devel@...ts.freedesktop.org, 
	dakr@...nel.org, 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 2025-09-26 at 17:27 +1000, Alexandre Courbot <acourbot@...dia.com> wrote...
> 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.

I think there are two aspects to the bindings - one which was addressed in
the comments for patch 5 is how to abstract them. The other is that the way we
currently generate them results in some  ugly name.

Given we want to generate these from our internal IDL eventually I would favour
fixing this naming ugliness by touching up the currently generated bindings. So
maybe I will do that for the next revision.

> 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.

I will admit that was a bit of a hack.

> >  
> >          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.

Thanks! That is a useful observation for cleaning these up.

> 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.

So this would also live in `fw.rs`? What I'm really concerned about is that if
we're not allowed access the C bindings outside of `fw.rs` then everything ends
up in `fw.rs`, and worse still `fw.rs` basically ends up importing everything as
well, tightly coupling everything into one big blob.

So ugly naming aside, I really can't see the advantage of this. It would seem
much cleaner to have to have gsp.rs import the C bindings it needs and tie that
togeather with the other higher level Gsp abstractions.

Although I agree with the overall sentiment - the code could be better here -
we could still create GspArgumentsCached::new() as you suggest just there's no
reason for it to be in `fw.rs`. The type aliases I had were just to hide the
ugly names really.

Powered by blists - more mailing lists