On 2025-09-26 at 17:27 +1000, Alexandre Courbot <acourbot at nvidia.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 at nvidia.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.
Danilo Krummrich
2025-Sep-29 07:18 UTC
[PATCH v2 06/10] gpu: nova-core: gsp: Create rmargs
On Mon Sep 29, 2025 at 8:36 AM CEST, Alistair Popple wrote:> On 2025-09-26 at 17:27 +1000, Alexandre Courbot <acourbot at nvidia.com> wrote... >> On Mon Sep 22, 2025 at 8:30 PM JST, Alistair Popple wrote: >> > @@ -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.It's not about fixing the name of the generated C bindings, it's about not leaking firmware specific structures into core code of the driver. Please hide it in an abstraction that can deal with differences between firmware version internally; see also [1]. [1] https://lore.kernel.org/all/DCUAYNNP97QI.1VOX5XUS9KP7K at kernel.org/>> 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.Please also make sure to use the DmaAddress type instead of a raw u64 for DMA addresses.>> 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.You can (and probably should) extend the module structure, i.e. add a sub-directory ./gsp/fw/ and structure things accordingly.