Re: [PATCH 7/8] gpu: nova-core: convert Gsp::new() to use CoherentInit

["Gary Guo" <gary@garyguo.net>]

On Tue Mar 3, 2026 at 4:22 PM GMT, Danilo Krummrich wrote:
> Convert libos (LibosMemoryRegionInitArgument) and rmargs
> (GspArgumentsPadded) to use CoherentInit / Coherent::init() and simplify
> the initialization. This also avoids separate initialization on the
> stack.
>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
>  drivers/gpu/nova-core/gsp.rs    | 47 +++++++++++--------------
>  drivers/gpu/nova-core/gsp/fw.rs | 62 +++++++++++++++++++++++----------
>  2 files changed, 65 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/gpu/nova-core/gsp.rs b/drivers/gpu/nova-core/gsp.rs
> index 25cd48514c77..cb7f6b4dc0f8 100644
> --- a/drivers/gpu/nova-core/gsp.rs
> +++ b/drivers/gpu/nova-core/gsp.rs
> @@ -5,10 +5,11 @@
>  use kernel::{
>      device,
>      dma::{
> +        Coherent,
>          CoherentAllocation,
> +        CoherentInit,
>          DmaAddress, //
>      },
> -    dma_write,
>      pci,
>      prelude::*,
>      transmute::AsBytes, //
> @@ -104,7 +105,7 @@ fn new(dev: &device::Device<device::Bound>) -> Result<Self> {
>  #[pin_data]
>  pub(crate) struct Gsp {
>      /// Libos arguments.
> -    pub(crate) libos: CoherentAllocation<LibosMemoryRegionInitArgument>,
> +    pub(crate) libos: Coherent<[LibosMemoryRegionInitArgument]>,
>      /// Init log buffer.
>      loginit: LogBuffer,
>      /// Interrupts log buffer.
> @@ -114,7 +115,7 @@ pub(crate) struct Gsp {
>      /// Command queue.
>      pub(crate) cmdq: Cmdq,
>      /// RM arguments.
> -    rmargs: CoherentAllocation<GspArgumentsPadded>,
> +    rmargs: Coherent<GspArgumentsPadded>,
>  }
>  
>  impl Gsp {
> @@ -123,34 +124,28 @@ pub(crate) fn new(pdev: &pci::Device<device::Bound>) -> impl PinInit<Self, Error
>          pin_init::pin_init_scope(move || {
>              let dev = pdev.as_ref();
>  
> +            // Initialise the logging structures. The OpenRM equivalents are in:
> +            // _kgspInitLibosLoggingStructures (allocates memory for buffers)
> +            // kgspSetupLibosInitArgs_IMPL (creates pLibosInitArgs[] array)
>              Ok(try_pin_init!(Self {
> -                libos: CoherentAllocation::<LibosMemoryRegionInitArgument>::alloc_coherent(
> -                    dev,
> -                    GSP_PAGE_SIZE / size_of::<LibosMemoryRegionInitArgument>(),
> -                    GFP_KERNEL | __GFP_ZERO,
> -                )?,
>                  loginit: LogBuffer::new(dev)?,
>                  logintr: LogBuffer::new(dev)?,
>                  logrm: LogBuffer::new(dev)?,
>                  cmdq: Cmdq::new(dev)?,
> -                rmargs: CoherentAllocation::<GspArgumentsPadded>::alloc_coherent(
> -                    dev,
> -                    1,
> -                    GFP_KERNEL | __GFP_ZERO,
> -                )?,
> -                _: {
> -                    // Initialise the logging structures. The OpenRM equivalents are in:
> -                    // _kgspInitLibosLoggingStructures (allocates memory for buffers)
> -                    // kgspSetupLibosInitArgs_IMPL (creates pLibosInitArgs[] array)
> -                    dma_write!(
> -                        libos, [0]?, LibosMemoryRegionInitArgument::new("LOGINIT", &loginit.0)
> -                    );
> -                    dma_write!(
> -                        libos, [1]?, LibosMemoryRegionInitArgument::new("LOGINTR", &logintr.0)
> -                    );
> -                    dma_write!(libos, [2]?, LibosMemoryRegionInitArgument::new("LOGRM", &logrm.0));
> -                    dma_write!(rmargs, [0]?.inner, fw::GspArgumentsCached::new(cmdq));
> -                    dma_write!(libos, [3]?, LibosMemoryRegionInitArgument::new("RMARGS", rmargs));
> +                rmargs: Coherent::init(dev, GFP_KERNEL, GspArgumentsPadded::new(cmdq))?,
> +                libos: {
> +                    let mut libos = CoherentInit::zeroed_slice(
> +                        dev,
> +                        GSP_PAGE_SIZE / size_of::<LibosMemoryRegionInitArgument>(),
> +                        GFP_KERNEL,
> +                    )?;
> +
> +                    libos.init_at(0, LibosMemoryRegionInitArgument::new("LOGINIT", &loginit.0))?;
> +                    libos.init_at(1, LibosMemoryRegionInitArgument::new("LOGINTR", &logintr.0))?;
> +                    libos.init_at(2, LibosMemoryRegionInitArgument::new("LOGRM", &logrm.0))?;
> +                    libos.init_at(3, LibosMemoryRegionInitArgument::new("RMARGS", rmargs))?;
> +
> +                    libos.into()
>                  },
>              }))
>          })
> diff --git a/drivers/gpu/nova-core/gsp/fw.rs b/drivers/gpu/nova-core/gsp/fw.rs
> index 751d5447214d..59cb03a9b238 100644
> --- a/drivers/gpu/nova-core/gsp/fw.rs
> +++ b/drivers/gpu/nova-core/gsp/fw.rs
> @@ -9,11 +9,12 @@
>  use core::ops::Range;
>  
>  use kernel::{
> -    dma::CoherentAllocation,
> +    dma::Coherent,
>      prelude::*,
>      ptr::{
>          Alignable,
> -        Alignment, //
> +        Alignment,
> +        KnownSize, //
>      },
>      sizes::{
>          SZ_128K,
> @@ -568,7 +569,9 @@ unsafe impl AsBytes for RunCpuSequencer {}
>  /// The memory allocated for the arguments must remain until the GSP sends the
>  /// init_done RPC.
>  #[repr(transparent)]
> -pub(crate) struct LibosMemoryRegionInitArgument(bindings::LibosMemoryRegionInitArgument);
> +pub(crate) struct LibosMemoryRegionInitArgument {
> +    inner: bindings::LibosMemoryRegionInitArgument,
> +}

FYI tuple struct construction is being worked at now:
https://github.com/Rust-for-Linux/pin-init/pull/113

>  
>  // SAFETY: Padding is explicit and does not contain uninitialized data.
>  unsafe impl AsBytes for LibosMemoryRegionInitArgument {}
> @@ -578,10 +581,10 @@ unsafe impl AsBytes for LibosMemoryRegionInitArgument {}
>  unsafe impl FromBytes for LibosMemoryRegionInitArgument {}
>  
>  impl LibosMemoryRegionInitArgument {
> -    pub(crate) fn new<A: AsBytes + FromBytes>(
> +    pub(crate) fn new<'a, A: AsBytes + FromBytes + KnownSize + ?Sized>(
>          name: &'static str,
> -        obj: &CoherentAllocation<A>,
> -    ) -> Self {
> +        obj: &'a Coherent<A>,
> +    ) -> impl Init<Self> + 'a {
>          /// Generates the `ID8` identifier required for some GSP objects.
>          fn id8(name: &str) -> u64 {
>              let mut bytes = [0u8; core::mem::size_of::<u64>()];
> @@ -593,7 +596,8 @@ fn id8(name: &str) -> u64 {
>              u64::from_ne_bytes(bytes)
>          }
>  
> -        Self(bindings::LibosMemoryRegionInitArgument {
> +        #[allow(non_snake_case)]
> +        let init_inner = init!(bindings::LibosMemoryRegionInitArgument {
>              id8: id8(name),
>              pa: obj.dma_handle(),
>              size: num::usize_as_u64(obj.size()),
> @@ -603,7 +607,11 @@ fn id8(name: &str) -> u64 {
>              loc: num::u32_into_u8::<
>                  { bindings::LibosMemoryRegionLoc_LIBOS_MEMORY_REGION_LOC_SYSMEM },
>              >(),
> -            ..Default::default()
> +            ..Zeroable::init_zeroed()
> +        });
> +
> +        init!(LibosMemoryRegionInitArgument {
> +            inner <- init_inner,
>          })
>      }
>  }
> @@ -814,15 +822,23 @@ unsafe impl FromBytes for GspMsgElement {}
>  
>  /// Arguments for GSP startup.
>  #[repr(transparent)]
> -pub(crate) struct GspArgumentsCached(bindings::GSP_ARGUMENTS_CACHED);
> +#[derive(Zeroable)]
> +pub(crate) struct GspArgumentsCached {
> +    inner: bindings::GSP_ARGUMENTS_CACHED,
> +}
>  
>  impl GspArgumentsCached {
>      /// Creates the arguments for starting the GSP up using `cmdq` as its command queue.
> -    pub(crate) fn new(cmdq: &Cmdq) -> Self {
> -        Self(bindings::GSP_ARGUMENTS_CACHED {
> -            messageQueueInitArguments: MessageQueueInitArguments::new(cmdq).0,
> +    pub(crate) fn new(cmdq: &Cmdq) -> impl Init<Self> + '_ {

This struct is not that big, so I think we can just keep the by-value
constructor.

Best,
Gary

> +        #[allow(non_snake_case)]
> +        let init_inner = init!(bindings::GSP_ARGUMENTS_CACHED {
> +            messageQueueInitArguments <- MessageQueueInitArguments::new(cmdq),
>              bDmemStack: 1,
> -            ..Default::default()
> +            ..Zeroable::init_zeroed()
> +        });
> +
> +        init!(GspArgumentsCached {
> +            inner <- init_inner,
>          })
>      }
>  }
> @@ -834,11 +850,21 @@ unsafe impl AsBytes for GspArgumentsCached {}
>  /// must all be a multiple of GSP_PAGE_SIZE in size, so add padding to force it
>  /// to that size.
>  #[repr(C)]
> +#[derive(Zeroable)]
>  pub(crate) struct GspArgumentsPadded {
>      pub(crate) inner: GspArgumentsCached,
>      _padding: [u8; GSP_PAGE_SIZE - core::mem::size_of::<bindings::GSP_ARGUMENTS_CACHED>()],
>  }
>  
> +impl GspArgumentsPadded {
> +    pub(crate) fn new(cmdq: &Cmdq) -> impl Init<Self> + '_ {
> +        init!(GspArgumentsPadded {
> +            inner <- GspArgumentsCached::new(cmdq),
> +            ..Zeroable::init_zeroed()
> +        })
> +    }
> +}
> +
>  // SAFETY: Padding is explicit and will not contain uninitialized data.
>  unsafe impl AsBytes for GspArgumentsPadded {}
>  
> @@ -847,18 +873,18 @@ unsafe impl AsBytes for GspArgumentsPadded {}
>  unsafe impl FromBytes for GspArgumentsPadded {}
>  
>  /// Init arguments for the message queue.
> -#[repr(transparent)]
> -struct MessageQueueInitArguments(bindings::MESSAGE_QUEUE_INIT_ARGUMENTS);
> +type MessageQueueInitArguments = bindings::MESSAGE_QUEUE_INIT_ARGUMENTS;
>  
>  impl MessageQueueInitArguments {
>      /// Creates a new init arguments structure for `cmdq`.
> -    fn new(cmdq: &Cmdq) -> Self {
> -        Self(bindings::MESSAGE_QUEUE_INIT_ARGUMENTS {
> +    #[allow(non_snake_case)]
> +    fn new(cmdq: &Cmdq) -> impl Init<Self> + '_ {
> +        init!(MessageQueueInitArguments {
>              sharedMemPhysAddr: cmdq.dma_handle(),
>              pageTableEntryCount: num::usize_into_u32::<{ Cmdq::NUM_PTES }>(),
>              cmdQueueOffset: num::usize_as_u64(Cmdq::CMDQ_OFFSET),
>              statQueueOffset: num::usize_as_u64(Cmdq::STATQ_OFFSET),
> -            ..Default::default()
> +            ..Zeroable::init_zeroed()
>          })
>      }
>  }