Message-Id: <DD8TZ3TU57L3.2958OTC9UP4VF@kernel.org>
Date: Fri, 03 Oct 2025 18:34:12 +0200
From: "Benno Lossin" <lossin@...nel.org>
To: "Alexandre Courbot" <acourbot@...dia.com>, "Alistair Popple"
 <apopple@...dia.com>, <rust-for-linux@...r.kernel.org>,
 <dri-devel@...ts.freedesktop.org>, <dakr@...nel.org>
Cc: "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 Thu Oct 2, 2025 at 3:49 PM CEST, Alexandre Courbot 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.

Yeah that's the correct workaround at the moment. I'm tracking support
for tuple structs in [1]. Essentially the problem is that it requires
lots of effort to parse tuple structs using declarative macros. We will
get `syn` this cycle, which will enable me to support several things,
including tuple structs.

[1]: https://github.com/Rust-for-Linux/pin-init/issues/85

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

This one will also be solved when we switch to syn.

> 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)?;

This is missing a safety comment. I haven't looked at this locally, so I
don't know what is happening in the 10-20 lines that aren't shown, so I
don't know if this is correct (if you're only assuming its initialized
after this line completes then it's fine). The rest of the patch looks
normal.

Hope this helps!

---
Cheers,
Benno

> +            // Convert the pointer backto a reference for checksum.
> +            &mut *cmd_ptr
>          };

Powered by blists - more mailing lists