Benno Lossin
2025-Oct-03 16:34 UTC
[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 at nvidia.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 > };
Janne Grunau
2025-Oct-03 17:25 UTC
[PATCH v3 08/13] gpu: nova-core: Add bindings and accessors for GspSystemInfo
On Fri, Oct 03, 2025 at 06:34:12PM +0200, Benno Lossin wrote:> 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 at nvidia.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.I was planning to submit https://github.com/AsahiLinux/linux/commit/2d95fd3b6c359634a0976f27f7a3c667826256da https://github.com/AsahiLinux/linux/commit/515638cb47cf0ebdac378686fcbbdc6a8364096a from the asahi downstream tree after 6.18-rc1. Does that still make sense timing wise? Types with type paths are used extensively in the asahi driver but I can initially work around that. Janne
Benno Lossin
2025-Oct-03 17:59 UTC
[PATCH v3 08/13] gpu: nova-core: Add bindings and accessors for GspSystemInfo
On Fri Oct 3, 2025 at 7:25 PM CEST, Janne Grunau wrote:> On Fri, Oct 03, 2025 at 06:34:12PM +0200, Benno Lossin wrote: >> 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 at nvidia.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.Oh by the way, enums are not supported due to a language limitation, see: https://github.com/Rust-for-Linux/pin-init/issues/59>> > 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. > > I was planning to submit > https://github.com/AsahiLinux/linux/commit/2d95fd3b6c359634a0976f27f7a3c667826256da > https://github.com/AsahiLinux/linux/commit/515638cb47cf0ebdac378686fcbbdc6a8364096a > from the asahi downstream tree after 6.18-rc1. Does that still make > sense timing wise?Probably not, since I'll depend on the syn patches this cycle which will mean that pin-init supports tuples in 6.19.> Types with type paths are used extensively in the asahi driver but I can > initially work around that.Yeah they should be supported simply by moving to syn, hope it doesn't introduce too much pain in the next cycle. --- Cheers, Benno