Message-ID: <pv7bhr5tsbszgql2zoisz4bwanzs75y4wu4lorc3bzgwahhbzk@f22lcgcnqbdj>
Date: Fri, 3 Oct 2025 09:38:05 +1000
From: Alistair Popple <apopple@...dia.com>
To: Alexandre Courbot <acourbot@...dia.com>
Cc: Benno Lossin <lossin@...nel.org>, 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>, 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 v3 08/13] gpu: nova-core: Add bindings and accessors for
 GspSystemInfo

On 2025-10-02 at 23:49 +1000, Alexandre Courbot <acourbot@...dia.com> wrote...
> Hi Alistair, (+Benno as this concerns the `init!` macros)
> 
> On Tue Sep 30, 2025 at 10:16 PM JST, Alistair Popple wrote:
> > Adds bindings and an in-place initialiser for the GspSystemInfo struct.
> >
> > Signed-off-by: Alistair Popple <apopple@...dia.com>
> >
> > ---
> >
> > It would be good to move to using the `init!` macros at some point, but
> > I couldn't figure out how to make that work to initialise an enum rather
> > than a struct as is required for the transparent representation.
> 
> Indeed we have to jump through a few (minor) hoops.
> 
> First the `init!` macros do not seem to support tuple structs. They
> match a `{` after the type name, which is not present in
> `GspSystemInfo`. By turning it into a regular struct with a single
> field, we can overcome this, and it doesn't affect the layout the
> `#[repr(transparent)]` can still be used.

I was thinking we should fix the `init!` macro to support tuple structs. Is
there some fundamental reason `init!` couldn't be modified to support tuple
structs? It seems like it would be nicer to fix that limitation rather than work
around it here.

> Then, due to a limitation with declarative macros, `init!` interprets
> `::` as a separator for generic arguments, so `bindings::GspSystemInfo`
> also doesn't parse. Here the trick is to use a local type alias.
> 
> After overcoming these two, I have been able to make
> `GspSystemInfo::init` return an in-place initializer. It is then a
> matter of changing `send_gsp_command` to accept it - please apply the
> following patch on top of your series for an illustration of how it can
> be done.
> 
> Note that I have added a new generic argument for the error returned by
> the `Init` - this is to let us also use infallible initializers
> transparently. The cool thing is that callers don't need to specify any
> generic argument since they can be inferred automatically.
> 
> diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs b/drivers/gpu/nova-core/gsp/cmdq.rs
> index 5580eaf52f7b..0709153f9dc9 100644
> --- a/drivers/gpu/nova-core/gsp/cmdq.rs
> +++ b/drivers/gpu/nova-core/gsp/cmdq.rs
> @@ -247,12 +247,20 @@ fn notify_gsp(bar: &Bar0) {
>          NV_PGSP_QUEUE_HEAD::default().set_address(0).write(bar);
>      }
> 
> -    pub(crate) fn send_gsp_command<M: CommandToGsp>(
> +    pub(crate) fn send_gsp_command<M, E>(
>          &mut self,
>          bar: &Bar0,
>          payload_size: usize,
> -        init: impl FnOnce(&mut M, SBuffer<core::array::IntoIter<&mut [u8], 2>>) -> Result,
> -    ) -> Result {
> +        init: impl Init<M, E>,
> +        init_sbuffer: impl FnOnce(SBuffer<core::array::IntoIter<&mut [u8], 2>>) -> Result,
> +    ) -> Result
> +    where
> +        M: CommandToGsp,
> +        // This allows all error types, including `Infallible`, to be used with `init`. Without
> +        // this we cannot use regular stack objects as `init` since their `Init` implementation
> +        // does not return any error.
> +        Error: From<E>,
> +    {
>          // TODO: a method that extracts the regions for a given command?
>          // ... and another that reduces the region to a given number of bytes!
>          let driver_area = self.gsp_mem.driver_write_area();
> @@ -264,7 +272,7 @@ pub(crate) fn send_gsp_command<M: CommandToGsp>(
>              return Err(EAGAIN);
>          }
> 
> -        let (msg_header, cmd, payload_1, payload_2) = {
> +        let (msg_header, cmd_ptr, payload_1, payload_2) = {
>              #[allow(clippy::incompatible_msrv)]
>              let (msg_header_slice, slice_1) = driver_area
>                  .0
> @@ -272,7 +280,6 @@ pub(crate) fn send_gsp_command<M: CommandToGsp>(
>                  .split_at_mut(size_of::<GspMsgElement>());
>              let msg_header = GspMsgElement::from_bytes_mut(msg_header_slice).ok_or(EINVAL)?;
>              let (cmd_slice, payload_1) = slice_1.split_at_mut(size_of::<M>());
> -            let cmd = M::from_bytes_mut(cmd_slice).ok_or(EINVAL)?;
>              #[allow(clippy::incompatible_msrv)]
>              let payload_2 = driver_area.1.as_flattened_mut();
>              // TODO: Replace this workaround to cut the payload size.
> @@ -283,11 +290,22 @@ pub(crate) fn send_gsp_command<M: CommandToGsp>(
>                  None => (&mut payload_1[..payload_size], payload_2),
>              };
> 
> -            (msg_header, cmd, payload_1, payload_2)
> +            (
> +                msg_header,
> +                cmd_slice.as_mut_ptr().cast(),
> +                payload_1,
> +                payload_2,
> +            )
> +        };
> +
> +        let cmd = unsafe {
> +            init.__init(cmd_ptr)?;
> +            // Convert the pointer backto a reference for checksum.
> +            &mut *cmd_ptr
>          };
> 
>          let sbuffer = SBuffer::new_writer([&mut payload_1[..], &mut payload_2[..]]);
> -        init(cmd, sbuffer)?;
> +        init_sbuffer(sbuffer)?;
> 
>          *msg_header =
>              GspMsgElement::new(self.seq, size_of::<M>() + payload_size, M::FUNCTION as u32);
> diff --git a/drivers/gpu/nova-core/gsp/commands.rs b/drivers/gpu/nova-core/gsp/commands.rs
> index 69df8d4be353..6f1be9078853 100644
> --- a/drivers/gpu/nova-core/gsp/commands.rs
> +++ b/drivers/gpu/nova-core/gsp/commands.rs
> @@ -79,10 +79,12 @@ pub(crate) fn build_registry(cmdq: &mut Cmdq, bar: &Bar0) -> Result {
>          ],
>      };
> 
> -    cmdq.send_gsp_command::<PackedRegistryTable>(bar, registry.size(), |table, sbuffer| {
> -        *table = PackedRegistryTable::new(GSP_REGISTRY_NUM_ENTRIES as u32, registry.size() as u32);
> -        registry.write_payload(sbuffer)
> -    })
> +    cmdq.send_gsp_command(
> +        bar,
> +        registry.size(),
> +        PackedRegistryTable::new(GSP_REGISTRY_NUM_ENTRIES as u32, registry.size() as u32),
> +        |sbuffer| registry.write_payload(sbuffer),
> +    )
>  }
> 
>  impl CommandToGsp for GspSystemInfo {
> @@ -95,7 +97,7 @@ pub(crate) fn set_system_info(
>      bar: &Bar0,
>  ) -> Result {
>      build_assert!(size_of::<GspSystemInfo>() < GSP_PAGE_SIZE);
> -    cmdq.send_gsp_command::<GspSystemInfo>(bar, 0, |info, _| GspSystemInfo::init(info, dev))?;
> +    cmdq.send_gsp_command(bar, 0, GspSystemInfo::init(dev), |_| Ok(()))?;
> 
>      Ok(())
>  }
> diff --git a/drivers/gpu/nova-core/gsp/fw/commands.rs b/drivers/gpu/nova-core/gsp/fw/commands.rs
> index 83c2b017c4cb..e69be2f422f2 100644
> --- a/drivers/gpu/nova-core/gsp/fw/commands.rs
> +++ b/drivers/gpu/nova-core/gsp/fw/commands.rs
> @@ -4,31 +4,50 @@
>  use kernel::transmute::{AsBytes, FromBytes};
>  use kernel::{device, pci};
> 
> +// Ideally we would derive this for all our bindings, using the same technique as
> +// https://lore.kernel.org/rust-for-linux/20250814093046.2071971-3-lossin@kernel.org/
> +unsafe impl Zeroable for bindings::GspSystemInfo {}
> +
>  #[repr(transparent)]
> -pub(crate) struct GspSystemInfo(bindings::GspSystemInfo);
> +pub(crate) struct GspSystemInfo {
> +    // `try_init!` doesn't seem to work with tuple structs. Work around this by declaring a regular
> +    // field, which comes down to exactly the same.
> +    inner: bindings::GspSystemInfo,
> +}
> 
>  impl GspSystemInfo {
> -    pub(crate) fn init(&mut self, dev: &pci::Device<device::Bound>) -> Result {
> -        self.0.gpuPhysAddr = dev.resource_start(0)?;
> -        self.0.gpuPhysFbAddr = dev.resource_start(1)?;
> -        self.0.gpuPhysInstAddr = dev.resource_start(3)?;
> -        self.0.nvDomainBusDeviceFunc = u64::from(dev.dev_id());
> +    #[allow(non_snake_case)]
> +    pub(crate) fn init<'a>(dev: &'a pci::Device<device::Bound>) -> impl Init<Self, Error> + 'a {
> +        // `try_init!` interprets `::` as a separator for generics, use a type alias to remove
> +        // them.
> +        type InnerGspSystemInfo = bindings::GspSystemInfo;
> 
> -        // Using TASK_SIZE in r535_gsp_rpc_set_system_info() seems wrong because
> -        // TASK_SIZE is per-task. That's probably a design issue in GSP-RM though.
> -        self.0.maxUserVa = (1 << 47) - 4096;
> -        self.0.pciConfigMirrorBase = 0x088000;
> -        self.0.pciConfigMirrorSize = 0x001000;
> +        // Initializer for the bindings type.
> +        let init_inner = try_init!(InnerGspSystemInfo {
> +            gpuPhysAddr: dev.resource_start(0)?,
> +            gpuPhysFbAddr: dev.resource_start(1)?,
> +            gpuPhysInstAddr: dev.resource_start(3)?,
> +            nvDomainBusDeviceFunc: u64::from(dev.dev_id()),
> 
> -        self.0.PCIDeviceID =
> -            (u32::from(dev.device_id()) << 16) | u32::from(dev.vendor_id().as_raw());
> -        self.0.PCISubDeviceID =
> -            (u32::from(dev.subsystem_device_id()) << 16) | u32::from(dev.subsystem_vendor_id());
> -        self.0.PCIRevisionID = u32::from(dev.revision_id());
> -        self.0.bIsPrimary = 0;
> -        self.0.bPreserveVideoMemoryAllocations = 0;
> +            // Using TASK_SIZE in r535_gsp_rpc_set_system_info() seems wrong because
> +            // TASK_SIZE is per-task. That's probably a design issue in GSP-RM though.
> +            maxUserVa: (1 << 47) - 4096,
> +            pciConfigMirrorBase: 0x088000,
> +            pciConfigMirrorSize: 0x001000,
> 
> -        Ok(())
> +            PCIDeviceID: (u32::from(dev.device_id()) << 16) | u32::from(dev.vendor_id().as_raw()),
> +            PCISubDeviceID: (u32::from(dev.subsystem_device_id()) << 16)
> +                | u32::from(dev.subsystem_vendor_id()),
> +            PCIRevisionID: u32::from(dev.revision_id()),
> +            bIsPrimary: 0,
> +            bPreserveVideoMemoryAllocations: 0,
> +            ..Zeroable::init_zeroed()
> +        });
> +
> +        // Final initializer for our type.
> +        try_init!(GspSystemInfo {
> +            inner <- init_inner,
> +        })
>      }
>  }

Powered by blists - more mailing lists