Message-Id: <DCP2UI9NGTQ6.3O0ARTPL4WCA2@nvidia.com>
Date: Wed, 10 Sep 2025 20:18:03 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "Danilo Krummrich" <dakr@...nel.org>, "Alexandre Courbot"
 <acourbot@...dia.com>
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>, "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>, "Alistair Popple" <apopple@...dia.com>, "Joel
 Fernandes" <joelagnelf@...dia.com>, "Timur Tabi" <ttabi@...dia.com>,
 <rust-for-linux@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
 <nouveau@...ts.freedesktop.org>, <dri-devel@...ts.freedesktop.org>
Subject: Re: [PATCH v3 02/11] gpu: nova-core: move GSP boot code out of
 `Gpu` constructor

On Wed Sep 10, 2025 at 5:01 PM JST, Danilo Krummrich wrote:
> On Wed Sep 10, 2025 at 6:48 AM CEST, Alexandre Courbot wrote:
>> On Tue Sep 9, 2025 at 11:43 PM JST, Danilo Krummrich wrote:
>>> 	impl Gpu {
>>> 	    pub(crate) fn new<'a>(
>>> 	        dev: &'a Device<Bound>,
>>> 	        bar: &'a Bar0
>>> 	        devres_bar: Arc<Devres<Bar0>>,
>>> 	    ) -> impl PinInit<Self, Error> + 'a {
>>> 	        try_pin_init(Self {
>>> 	            bar: devres_bar,
>>> 	            spec: Spec::new(bar)?,
>>> 	            gsp_falcon: Falcon::<Gsp>::new(dev, spec.chipset)?,
>>> 	            sec2_falcon: Falcon::<Sec2>::new(dev, spec.chipset)?,
>>> 	            sysmem_flush: SysmemFlush::register(dev, bar, spec.chipset)?
>>> 	            gsp <- Gsp::new(gsp_falcon, sec2_falcon, sysmem_flush)?,
>>> 	        })
>>> 	    }
>>> 	}
>>
>> It does work. The bizareness of passing the `bar` twice aside,
>
> Yeah, but it really seems like a minor inconvinience. (Especially, since this
> will be the only occurance of this we'll ever have.)

It's definitely not a big deal.

>
>> here is what it looks like when I got it to compile:
>
> This looks great!
>
>>     pub(crate) fn new<'a>(
>>         pdev: &'a pci::Device<device::Bound>,
>>         devres_bar: Arc<Devres<Bar0>>,
>>         bar: &'a Bar0,
>>     ) -> impl PinInit<Self, Error> + 'a {
>>         try_pin_init!(Self {
>>             spec: Spec::new(bar).inspect(|spec| {
>>                 dev_info!(
>>                     pdev.as_ref(),
>>                     "NVIDIA (Chipset: {}, Architecture: {:?}, Revision: {})\n",
>>                     spec.chipset,
>>                     spec.chipset.arch(),
>>                     spec.revision
>>                 );
>>             })?,
>
> +             _: {
> +                 gfw::wait_gfw_boot_completion(bar)
> +                     .inspect_err(|_| dev_err!(pdev.as_ref(), "GFW boot did not complete"))?;
> +             },
>>
>>             sysmem_flush: SysmemFlush::register(pdev.as_ref(), bar, spec.chipset)?,
>>
>>             gsp_falcon: Falcon::<Gsp>::new(
>>                 pdev.as_ref(),
>>                 spec.chipset,
>>                 bar,
>>                 spec.chipset > Chipset::GA100,
>>             )
>>             .inspect(|falcon| falcon.clear_swgen0_intr(bar))?,
>>
>>             sec2_falcon: Falcon::<Sec2>::new(pdev.as_ref(), spec.chipset, bar, true)?,
>>
> -             gsp: Self::start_gsp(pdev, bar, spec.chipset, gsp_falcon, sec2_falcon)?,
> +             gsp <- Self::start_gsp(pdev, bar, spec.chipset, gsp_falcon, sec2_falcon),
>>
>>             bar: devres_bar,
>>         })
>>     }
>>
>> The wait for GFW initialization had to be moved to `probe`, but that's
>> fine IMO.
>
> That's not necessary, you can keep it in Gpu::new() -- I don't see what's
> preventing us from that. I inserted it in the code above.

This one didn't work for me, but maybe you have a newer version of the
internal references patch:

error: no rules expected reserved identifier `_`
    --> drivers/gpu/nova-core/gpu.rs:323:13
     |
323  |             _: {
     |             ^ no rules expected this token in macro call
     |
note: while trying to match `)`

>
>> I do however find the code less readable in this form, less
>> editable as well. And LSP seems lost, so I don't get any syntax
>> highlighting in the `try_pin_init` block.
>
> Benno is working on a syntax update, so automatic formatting etc. will properly
> work.
>
> Otherwise, I can't see how this is a downgrade. It represents the initialization
> process in a much clearer way that the current implementation of Gsp::new(),
> which is rather messy.
>
>> Fundamentally, this changes the method from a fallible method returning
>> a non-fallible initializer into a non-fallible method returning a
>> fallible initializer.
>
> Yeah, that's the best case when working with pin-init.
>
>> I'm ok with that, and maybe this new form will
>> encourage us to keep this method short, which is what we want, but other
>> than that what benefit are we getting from this change?
>
> The immediate benefit is that we don't need an extra allocation for the Gsp
> structure.
>
> The general benefit is that once we need to add more fields to
> structures that require pinning (such as locks -- and we will need a lot of
> them) we're prepared for it.
>
> If we're not prepared for it, I'm pretty sure that everytime someone needs to
> add e.g. a new lock for something, it will just result in a new Pin<KBox<T>>,
> because the initial pin-init hierarchy just isn't there, and creating a new
> allocation is more convinient than fixing the existing code.
>
> This is exactly what pin-init was introduced for in the kernel, such that we're
> not doing worse than equivalent C code.
>
> 	struct Foo {
> 	   struct Bar bar;
> 	   void *data;
> 	   struct mutex data_lock;
> 	}
>
> 	struct Bar {
> 	   void *data;
> 	   struct mutex data_lock;
> 	}
>
> In C you can just kmalloc(sizeof(Foo), GFP_KERNEL) and subsequently initialize
> all fields.
>
> In Rust I don't want to fall back having to allocate for Bar *and* for Foo
> separately.
>
> If there are things to improve with pin-init, let's improve them, but let's not
> do worse than C code in this aspect.

Maybe I need to get used to this way of initializing, but in any case I
can definitely live with it. And it we split initialization into proper
functions the code should remain readable.

I'll try and slip this into the next revision of this patchset, or as a
follow-up if there isn't one.

Powered by blists - more mailing lists