Message-Id: <DCJZ9RJMI55S.38IB570PFGM7V@nvidia.com>
Date: Thu, 04 Sep 2025 20:26:34 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "Alistair Popple" <apopple@...dia.com>
Cc: <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>, "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>, "Nouveau"
 <nouveau-bounces@...ts.freedesktop.org>
Subject: Re: [PATCH 05/10] gpu: nova-core: gsp: Add GSP command queue
 handling

On Thu Sep 4, 2025 at 3:57 PM JST, Alistair Popple wrote:
<snip>
>> > +}
>> > +
>> > +// This next section contains constants and structures hand-coded from the GSP
>> > +// headers We could replace these with bindgen versions, but that's a bit of a
>> > +// pain because they basically end up pulling in the world (ie. definitions for
>> > +// every rpc method). So for now the hand-coded ones are fine. They are just
>> > +// structs so we can easily move to bindgen generated ones if/when we want to.
>> > +
>> > +// A GSP RPC header
>> > +#[repr(C)]
>> > +#[derive(Debug, Clone)]
>> > +struct GspRpcHeader {
>> > +    header_version: u32,
>> > +    signature: u32,
>> > +    length: u32,
>> > +    function: u32,
>> > +    rpc_result: u32,
>> > +    rpc_result_private: u32,
>> > +    sequence: u32,
>> > +    cpu_rm_gfid: u32,
>> > +}
>> 
>> This is the equivalent of `rpc_message_header_v03_00` in OpenRM. The
>> fact it is versioned makes me a bit nervous. :) If the layout change
>> somehow, we are in for a fun night of debugging. This is where having an
>> opaque abstraction built on top of a bindgen-generated type would be
>> handy: if the layout changes in an incompatible way, when the
>> abstraction would break at compile-time.
>
> Argh! I guess I wrote that before we were generating the headers and I never
> thought to go back and change that. Will fix these to use the generated binding.
>
> I will sync up with you offline but I'm not really understanding the point here.
> If a bindgen generated type changes in some incompatible way wouldn't we already
> get a compile-time error? And if the bindgen generated type changes, what's to
> say the rest of the logic hasn't also changed?
>
> Whilst I'm not totally opposed to something like this it just seems premature
> - the ABI is supposed to be stabalising and in practice none of the structures
> we care about appear to have changed in the 3 years since 525.53 was released.
> So IHMO it would be better to just deal with these changes if (not when) they
> happen rather than try and create an abstraction now, especially as this is only
> supposed to become more stable.

While I also hope we will achieve some level of ABI stability, I want to
provision a bit just in case this goal turns out to be too idealistic.

At the moment we are touching bindings internals a bit everywhere in the
`gsp` module. As the driver matures, that trend can only continue - if
one day we realize that we need a firmware version abstraction after
all, it will get increasingly difficult (and time-consuming) to shoehorn
back as time goes. It is much easier to do this from the start.

Also, having a proper abstraction objectively results in better code. I
will share the bits I have written with you for testing purposes, but I
think you will agree that this makes the GSP module much cleaner,
focused on the higher-level operations, while the lower-level code is
divided into easy-to-understand methods, right next to the type they
manipulate instead of being inlined as part of the sub-logic of another
function. Even without the prospect of multiple firmware versions, it is
worth doing.

<snip>
>> Doing so is valuable for clarity as well as future compatibility, as it
>> clearly shows in a single page of code how the header is used. Here is
>> all the code operating on it, in a single block instead of being spread
>> through this file:
>> 
>> 	impl MsgqTxHeader {
>> 			pub(crate) fn new(msgq_size: u32, msg_count: u32, rx_hdr_offset: u32) -> Self {
>> 					Self(bindings::msgqTxHeader {
>> 							version: 0,
>> 							size: msgq_size,
>> 							msgSize: GSP_PAGE_SIZE as u32,
>> 							msgCount: msg_count as u32,
>> 							writePtr: 0,
>> 							flags: 1,
>> 							rxHdrOff: rx_hdr_offset,
>> 							entryOff: GSP_PAGE_SIZE as u32,
>> 					})
>> 			}
>> 
>> 			pub(crate) fn write_ptr(&self) -> u32 {
>> 					let ptr = (&self.0.writePtr) as *const u32;
>> 
>> 					unsafe { ptr.read_volatile() }
>> 			}
>> 
>> 			pub(crate) fn set_write_ptr(&mut self, val: u32) {
>> 					let ptr = (&mut self.0.writePtr) as *mut u32;
>> 
>> 					unsafe { ptr.write_volatile(val) }
>> 			}
>> 	}
>
> Yes, this makes a lot of sense although I'm still not seeing the value of the
> [repr(transparent)] representation. Hopefully you can explain during out sync
> up ;)

This type is shared with the GSP, so we must ensure that its layout is
exactly the same as the C structure it wraps.

Powered by blists - more mailing lists