From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 07E1FCA0FED for ; Wed, 10 Sep 2025 08:01:30 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id B4A5210E891; Wed, 10 Sep 2025 08:01:29 +0000 (UTC) Authentication-Results: gabe.freedesktop.org; dkim=pass (2048-bit key; unprotected) header.d=kernel.org header.i=@kernel.org header.b="O9hu+wut"; dkim-atps=neutral Received: from sea.source.kernel.org (sea.source.kernel.org [172.234.252.31]) by gabe.freedesktop.org (Postfix) with ESMTPS id 6209F10E891; Wed, 10 Sep 2025 08:01:28 +0000 (UTC) Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by sea.source.kernel.org (Postfix) with ESMTP id 1AA05447F4; Wed, 10 Sep 2025 08:01:28 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id B6E1DC4CEF5; Wed, 10 Sep 2025 08:01:23 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1757491288; bh=W9AyICeigCjB4ZfW6Mn13qQroeZohBNvhnwa50EVj9s=; h=Date:Subject:Cc:To:From:References:In-Reply-To:From; b=O9hu+wutcuu/Q7972UJxuki8Y3HF4ragOWc7XGwFauq0XvJt5T/41JaU7f3ND83lO EVRVi9iOwSjLuwb6kX7NPM10qUQFLq0OnnSD35bMP+nXottJf4RDXgzxBESv7bUZgT AlwjvU4+dydveIghjt8smeGX8GqNRzYAUopY6BXbQhfZayr5d3hdU3NAFcOYyTvWXN xgIA1u/iIR0GClqtjHMuVTVU/y7N3th6G6/NoazYd+uoYfiOvKtQYu0xxxtCqQKtUP UoLc3sdaX9535hMlddg5lLAjLv8Z2XnGYx2dM73rzI8s74z9PjMVhTevClGKq8Lgcq VZ5K6xEMCyIrg== Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Wed, 10 Sep 2025 10:01:22 +0200 Message-Id: Subject: Re: [PATCH v3 02/11] gpu: nova-core: move GSP boot code out of `Gpu` constructor Cc: "Miguel Ojeda" , "Alex Gaynor" , "Boqun Feng" , "Gary Guo" , =?utf-8?q?Bj=C3=B6rn_Roy_Baron?= , "Benno Lossin" , "Andreas Hindborg" , "Alice Ryhl" , "Trevor Gross" , "David Airlie" , "Simona Vetter" , "Maarten Lankhorst" , "Maxime Ripard" , "Thomas Zimmermann" , "John Hubbard" , "Alistair Popple" , "Joel Fernandes" , "Timur Tabi" , , , , To: "Alexandre Courbot" From: "Danilo Krummrich" References: <20250902-nova_firmware-v3-0-56854d9c5398@nvidia.com> <20250902-nova_firmware-v3-2-56854d9c5398@nvidia.com> <843554b1-f4c5-43f5-a23b-583339708bea@kernel.org> In-Reply-To: X-BeenThere: nouveau@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Nouveau development list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: nouveau-bounces@lists.freedesktop.org Sender: "Nouveau" 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, >> bar: &'a Bar0 >> devres_bar: Arc>, >> ) -> impl PinInit + 'a { >> try_pin_init(Self { >> bar: devres_bar, >> spec: Spec::new(bar)?, >> gsp_falcon: Falcon::::new(dev, spec.chipset)?, >> sec2_falcon: Falcon::::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 th= is will be the only occurance of this we'll ever have.) > here is what it looks like when I got it to compile: This looks great! > pub(crate) fn new<'a>( > pdev: &'a pci::Device, > devres_bar: Arc>, > bar: &'a Bar0, > ) -> impl PinInit + '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 di= d not complete"))?; + }, > > sysmem_flush: SysmemFlush::register(pdev.as_ref(), bar, spec.= chipset)?, > > gsp_falcon: Falcon::::new( > pdev.as_ref(), > spec.chipset, > bar, > spec.chipset > Chipset::GA100, > ) > .inspect(|falcon| falcon.clear_swgen0_intr(bar))?, > > sec2_falcon: Falcon::::new(pdev.as_ref(), spec.chipset,= bar, true)?, > - gsp: Self::start_gsp(pdev, bar, spec.chipset, gsp_falcon, sec= 2_falcon)?, + gsp <- Self::start_gsp(pdev, bar, spec.chipset, gsp_falcon, s= ec2_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. > 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 prop= erly work. Otherwise, I can't see how this is a downgrade. It represents the initializ= ation 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= >, 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 w= e'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 initial= ize 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.