Message-Id: <DCJ6G4DJ1JSY.1U6II6SNMZAZQ@nvidia.com>
Date: Wed, 03 Sep 2025 21:51:20 +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 03/10] gpu: nova-core: gsp: Create wpr metadata

On Wed Sep 3, 2025 at 5:57 PM JST, Alistair Popple wrote:
<snip>
>> I've discussed the bindings abstractions with Danilo last week. We
>> agreed that no layout information should ever escape the `nvfw` module.
>> I.e. the fields of `GspFwWprMeta` should not even be visible here.
>> 
>> Instead, `GspFwWprMeta` should be wrapped privately into another
>> structure inside `nvfw`:
>> 
>>   /// Structure passed to the GSP bootloader, containing the framebuffer layout as well as the DMA
>>   /// addresses of the GSP bootloader and firmware.
>>   #[repr(transparent)]
>>   pub(crate) struct GspFwWprMeta(r570_144::GspFwWprMeta);
>
> I'm a little bit unsure what the advantage of this is? Admittedly I'm not sure
> I've seen the discussion from last week so I may have missed something but it's
> not obvious how creating another layer of abstraction is better. How would it
> help contain any layout changes to nvfw? Supporting any new struct fields for
> example would almost certainly still require code changes outside nvfw.

It is not as much creating a new abstraction layer as it is controlling
where it resides - nicely contained in `nvfw` or all over the place.
This is particularly relevant if we consider that binding abstractions
are more likely to require `unsafe` code, that we will then be able to
confine to the `nvfw` module. As I got reminded in my own series, we
don't want `unsafe` code in regular driver modules.

Even if a new field is added to `GspFwWprMeta`, there is a good chance
that the parameters of its current constructor will cover what we need
to initialize it, so the calling code outside of `nvfw` won't need to
change. Of course we cannot guarantee this will be true all the time,
but it still covers us better than the alternative.

And then there is the question of if/when we need to support several
firmware versions. If we start having code in `gsp` that is specific to
a given firmware version, this is already a non-starter. Whereas having
all the abstractions in a single module leaves us in a better position
to use trait objects and virtual calls, or apply proc-macro magic.

>
> My thinking here was that the bindings (at least for GSP) probably want to live
> in the Gsp crate/module, and the rest of the driver would be isolated from Gsp
> changes by the public API provided by the Gsp crate/module rather than trying to
> do that at the binding level. For example the get_gsp_info() command implemented
> in [1] provides a separate public struct representing what the rest of the
> driver needs, thus ensuring the implementation specific details of Gsp (such as
> struct layout) do not leak into the wider nova-core driver.
>
>> All its implementations should also be there:
>> 
>>   // SAFETY: Padding is explicit and will not contain uninitialized data.
>>   unsafe impl AsBytes for GspFwWprMeta {}
>> 
>>   // SAFETY: This struct only contains integer types for which all bit patterns
>>   // are valid.
>>   unsafe impl FromBytes for GspFwWprMeta {}
>
> Makes sense.
>
>> And lastly, this `new` method can also be moved into `nvfw`, as an impl
>> block for the wrapping `GspFwWprMeta` type. That way no layout detail
>> escapes that module, and it will be easier to adapt the code to
>> potential layout chances with new firmware versions.
>> 
>> (note that my series is the one carelessly re-exporting `GspFwWprMeta`
>> as-is - I'll fix that too in v3)
>> 
>> The same applies to `LibosMemoryRegionInitArgument` of the previous
>> patch, and other types introduced in subsequent patches. Usually there
>> is little more work to do than moving the implentations into `nvfw` as
>> everything is already abstracted correctly - just not where we
>> eventually want it.
>
> This is where I get a little bit uncomfortable - this doesn't feel right to me.
> It seems to me moving all these implementations to the bindings would just end
> up with a significant amount of Gsp code in nvfw.rs rather than in the places
> that actually use it, making nvfw.rs large and unwieldy and the code more
> distributed and harder to follow.

If we want to split things more logically, I think it's perfectly fine
to have e.g. a `nvfw/gsp` module that contains all the GSP abstractions,
another one for the sequencer, etc. As long as all the version-specific
bits are contained below `nvfw`.

>
> And it's all tightly coupled anyway - for example the Gsp boot arguments require some
> command queue offsets which are all pretty specific to the Gsp implementation.
> Ie. we can't define some nice public API in the Gsp crate for "getting arguments
> required for booting Gsp" without that just being "here is a struct containing
> all the fields that must be packed into the Gsp arguments for this version",
> which at that point may as well just be the actual struct itself right?

Which particular structure are you refering to?

Powered by blists - more mailing lists