Message-ID: <oi5mksfhvuumriuqtld4hhd46gcghawtemaxnv4qs7cx76ecio@6rp3taivotiw>
Date: Thu, 9 Oct 2025 12:14:55 +1100
From: Alistair Popple <apopple@...dia.com>
To: Danilo Krummrich <dakr@...nel.org>
Cc: rust-for-linux@...r.kernel.org, dri-devel@...ts.freedesktop.org, 
	acourbot@...dia.com, 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 v4 02/13] gpu: nova-core: Create initial Gsp

On 2025-10-09 at 03:01 +1100, Danilo Krummrich <dakr@...nel.org> wrote...
> On Wed Oct 8, 2025 at 2:12 AM CEST, Alistair Popple wrote:
> > diff --git a/drivers/gpu/nova-core/gsp.rs b/drivers/gpu/nova-core/gsp.rs
> > index 221281da1a45..63099df77348 100644
> > --- a/drivers/gpu/nova-core/gsp.rs
> > +++ b/drivers/gpu/nova-core/gsp.rs
> > @@ -2,25 +2,94 @@
> >  
> >  mod boot;
> >  
> > +use kernel::device;
> > +use kernel::dma::CoherentAllocation;
> > +use kernel::dma::DmaAddress;
> > +use kernel::dma_write;
> > +use kernel::pci;
> >  use kernel::prelude::*;
> >  use kernel::ptr::Alignment;
> > +use kernel::transmute::AsBytes;
> >  
> >  pub(crate) use fw::{GspFwWprMeta, LibosParams};
> >  
> >  mod fw;
> >  
> > +use fw::LibosMemoryRegionInitArgument;
> > +
> >  pub(crate) const GSP_PAGE_SHIFT: usize = 12;
> >  pub(crate) const GSP_PAGE_SIZE: usize = 1 << GSP_PAGE_SHIFT;
> >  pub(crate) const GSP_HEAP_ALIGNMENT: Alignment = Alignment::new::<{ 1 << 20 }>();
> 
> This looks like it could depend on the firmware version in the future, hence it
> should probably defined somewhere in fw/ with a corresponding comment. The
> actual version switch is fine to omit for now of course (we agreed to add the
> infrastructure for the version switch subsequently).

Ok.
 
> > +/// Number of GSP pages to use in a RM log buffer.
> > +const RM_LOG_BUFFER_NUM_PAGES: usize = 0x10;
> 
> Why 0x10? Is there a specific reason?

No real reason other than 64K seems appropriate. It also happens to match what
Nouveau does.
 
> > +
> >  /// GSP runtime data.
> > -///
> > -/// This is an empty pinned placeholder for now.
> >  #[pin_data]
> > -pub(crate) struct Gsp {}
> > +pub(crate) struct Gsp {
> > +    libos: CoherentAllocation<LibosMemoryRegionInitArgument>,
> > +    pub loginit: CoherentAllocation<u8>,
> > +    pub logintr: CoherentAllocation<u8>,
> > +    pub logrm: CoherentAllocation<u8>,
> 
> This creates warnings for older compiler version, please use pub(crate) instead.

Ok. In fact I'm not even sure these need to public (for this series at least).

> > +}
> > +
> > +#[repr(C)]
> > +struct PteArray<const NUM_ENTRIES: usize>([u64; NUM_ENTRIES]);
> > +/// SAFETY: arrays of `u64` implement `AsBytes` and we are but a wrapper around it.
> > +unsafe impl<const NUM_ENTRIES: usize> AsBytes for PteArray<NUM_ENTRIES> {}
> 
> Please separate struct definitions and impl blocks with an empty line.

Done.

> > +impl<const NUM_PAGES: usize> PteArray<NUM_PAGES> {
> > +    fn new(handle: DmaAddress) -> Self {
> 
> No check that NUM_PAGES actually fits the size of the DMA buffer handle passed
> in? What happens if they do not match?

If it's bigger there's no issue, we just initialise a few more PTEs than
necessary that don't get copied. If it's smaller there will be uninitialised
PTEs. For v5 I've added another const to keep these the same.

> > +        let mut ptes = [0u64; NUM_PAGES];
> > +        for (i, pte) in ptes.iter_mut().enumerate() {
> > +            *pte = handle + ((i as u64) << GSP_PAGE_SHIFT);
> 
> I think this should be handle.checked_add(). Additionally we should add the
> following compile time check to make sure that the shift can never overflow:

Sure.

> 	const _MAX_OFFSET: usize = NUM_PAGES << GSP_PAGE_SHIFT;

Hmm. My Rust wasn't good enough to make this work:

impl<const NUM_PAGES: usize> PteArray<NUM_PAGES> {
    fn new(handle: DmaAddress) -> Result<Self> {
        // Compile time check to ensure the shift below never overflows.
        const _MAX_OFFSET: usize = NUM_PAGES << GSP_PAGE_SHIFT;

Results in:

error[E0401]: can't use generic parameters from outer item

Maybe I'm missing something (the documentation for E0401 didn't really enlighten
me I'm afraid).

> > +        }
> > +
> > +        Self(ptes)
> > +    }
> > +}
> > +
> > +/// Creates a new `CoherentAllocation<A>` with `name` of `size` elements, and
> > +/// register it into the `libos` object at argument position `libos_arg_nr`.
> > +fn create_logbuffer_dma_object(
> > +    dev: &device::Device<device::Bound>,
> > +) -> Result<CoherentAllocation<u8>> {
> > +    let mut obj = CoherentAllocation::<u8>::alloc_coherent(
> > +        dev,
> > +        RM_LOG_BUFFER_NUM_PAGES * GSP_PAGE_SIZE,
> > +        GFP_KERNEL | __GFP_ZERO,
> > +    )?;
> > +    let ptes = PteArray::<RM_LOG_BUFFER_NUM_PAGES>::new(obj.dma_handle());
> > +
> > +    // SAFETY: `obj` has just been created and we are its sole user.
> > +    unsafe {
> > +        // Copy the self-mapping PTE at the expected location.
> > +        obj.as_slice_mut(size_of::<u64>(), size_of_val(&ptes))?
> > +            .copy_from_slice(ptes.as_bytes())
> > +    };
> > +
> > +    Ok(obj)
> > +}
> 
> I think we should just create a new gsp::Logbuffer type for this rather than
> have a function as object constructor.

Have done for v5.

> >  
> >  impl Gsp {
> > -    pub(crate) fn new() -> impl PinInit<Self> {
> > -        pin_init!(Self {})
> > +    pub(crate) fn new(pdev: &pci::Device<device::Bound>) -> Result<impl PinInit<Self, Error>> {
> > +        let dev = pdev.as_ref();
> > +        let libos = CoherentAllocation::<LibosMemoryRegionInitArgument>::alloc_coherent(
> > +            dev,
> > +            GSP_PAGE_SIZE / size_of::<LibosMemoryRegionInitArgument>(),
> > +            GFP_KERNEL | __GFP_ZERO,
> > +        )?;
> > +        let loginit = create_logbuffer_dma_object(dev)?;
> > +        dma_write!(libos[0] = LibosMemoryRegionInitArgument::new("LOGINIT", &loginit))?;
> > +        let logintr = create_logbuffer_dma_object(dev)?;
> > +        dma_write!(libos[1] = LibosMemoryRegionInitArgument::new("LOGINTR", &logintr))?;
> > +        let logrm = create_logbuffer_dma_object(dev)?;
> > +        dma_write!(libos[2] = LibosMemoryRegionInitArgument::new("LOGRM", &logrm))?;
> > +
> > +        Ok(try_pin_init!(Self {
> > +            libos,
> > +            loginit,
> > +            logintr,
> > +            logrm,
> > +        }))
> >      }
> >  }
> > diff --git a/drivers/gpu/nova-core/gsp/fw.rs b/drivers/gpu/nova-core/gsp/fw.rs
> > index 181baa401770..dd1e7fc85d85 100644
> > --- a/drivers/gpu/nova-core/gsp/fw.rs
> > +++ b/drivers/gpu/nova-core/gsp/fw.rs
> > @@ -7,8 +7,10 @@
> >  
> >  use core::ops::Range;
> >  
> > +use kernel::dma::CoherentAllocation;
> >  use kernel::ptr::Alignable;
> >  use kernel::sizes::SZ_1M;
> > +use kernel::transmute::{AsBytes, FromBytes};
> >  
> >  use crate::gpu::Chipset;
> >  use crate::gsp;
> > @@ -99,3 +101,40 @@ pub(crate) fn wpr_heap_size(&self, chipset: Chipset, fb_size: u64) -> u64 {
> >  /// addresses of the GSP bootloader and firmware.
> >  #[repr(transparent)]
> >  pub(crate) struct GspFwWprMeta(bindings::GspFwWprMeta);
> > +
> > +#[repr(transparent)]
> > +pub(crate) struct LibosMemoryRegionInitArgument(bindings::LibosMemoryRegionInitArgument);
> 
> Please add some documentation for the type.

Ok, I have shamelessly stolen some from Nouveau for v5.

> > +
> > +// SAFETY: Padding is explicit and will not contain uninitialized data.
> > +unsafe impl AsBytes for LibosMemoryRegionInitArgument {}
> > +
> > +// SAFETY: This struct only contains integer types for which all bit patterns
> > +// are valid.
> > +unsafe impl FromBytes for LibosMemoryRegionInitArgument {}
> > +
> > +impl LibosMemoryRegionInitArgument {
> > +    pub(crate) fn new<A: AsBytes + FromBytes>(
> > +        name: &'static str,
> > +        obj: &CoherentAllocation<A>,
> > +    ) -> Self {
> > +        /// Generates the `ID8` identifier required for some GSP objects.
> > +        fn id8(name: &str) -> u64 {
> > +            let mut bytes = [0u8; core::mem::size_of::<u64>()];
> > +
> > +            for (c, b) in name.bytes().rev().zip(&mut bytes) {
> > +                *b = c;
> > +            }
> > +
> > +            u64::from_ne_bytes(bytes)
> > +        }
> > +
> > +        Self(bindings::LibosMemoryRegionInitArgument {
> > +            id8: id8(name),
> > +            pa: obj.dma_handle(),
> > +            size: obj.size() as u64,
> > +            kind: bindings::LibosMemoryRegionKind_LIBOS_MEMORY_REGION_CONTIGUOUS as u8,
> > +            loc: bindings::LibosMemoryRegionLoc_LIBOS_MEMORY_REGION_LOC_SYSMEM as u8,
> 
> Please prefer into() if possible.

In this case it won't work because obviously From<u32> isn't implemented
for u8. It's kind of redundant, because hopefully our bindings are sane, but
I've changed this to try_into() and made LibosMemoryRegionInitArgument::new()
failable for v5.

Powered by blists - more mailing lists