From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 535F0243367; Thu, 11 Sep 2025 12:46:56 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1757594817; cv=none; b=Inw/djCo98URhdN9WKD2pxVSzefAKXrmKemz7GiTEwpO2PFl9vy/io4h1+/N6RhC4wevVnADNoUMQvWVgMrhS7xbb9G5ve6LglLwUFZL9nDNm8Ej5FnMYMHJTEcoWQD4IWBVMKTeB8Hb5HMnExsYbtp5BJKR7DukRcO/pqrFvZc= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1757594817; c=relaxed/simple; bh=ZMKRsM2d2FmjPym3iMEMNiiUE1WxtkOsfG1kTOUZ+po=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=CRzUdM5A3aU9Vv0jArtMtxwfRoazMe7P872DMOqntxUPtO1Ahw2RvF3akR53HZLN1IWmWCzg5/8K1jkrojmGEQOLxYDi13beQe4YlSmWqDVJjipsnykPlbW5vUZM5i/L6mla73njPrtbs9faRqkAHO5v2T/iRajaRvV6N5scF20= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=b2Jgjxe0; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="b2Jgjxe0" Received: by smtp.kernel.org (Postfix) with ESMTPSA id B1842C4CEF1; Thu, 11 Sep 2025 12:46:52 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1757594816; bh=ZMKRsM2d2FmjPym3iMEMNiiUE1WxtkOsfG1kTOUZ+po=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=b2Jgjxe0smjNxwt90PXOQmhfvrW+ekazryG1m+dsEte/g78R+1fK3qaDluF7kr6qw DOUSJCOcA1AxyvONbB/ozhz0+y2dBOVqOu1Npn4WNDyBu0uKNYhPZcI5gSvrxShHsA JahfpeIWpvJth+407eK40K+s2A6BHrfSYHtL/DPAdToRxSNbDeLCoCxQn86SIUuIGk YWGOFAF4lrSbKcYkJPXMYY0KzQQ2SNnLs89YCEMmlxTVk8emI6d5xVNPdLO3qjITm7 qSlhqA5q/vBTAeTXwmixuXgvjIqOB/4KixRAVS5iQNIruTnDfHHodcyWPNoY4n/ayS Y/RuMddb9f1Hw== Message-ID: Date: Thu, 11 Sep 2025 14:46:51 +0200 Precedence: bulk X-Mailing-List: rust-for-linux@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v5 02/12] gpu: nova-core: move GSP boot code to a dedicated method To: Alexandre Courbot 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 , rust-for-linux@vger.kernel.org, linux-kernel@vger.kernel.org, nouveau@lists.freedesktop.org, dri-devel@lists.freedesktop.org References: <20250911-nova_firmware-v5-0-5a8a33bddca1@nvidia.com> <20250911-nova_firmware-v5-2-5a8a33bddca1@nvidia.com> From: Danilo Krummrich Content-Language: en-US In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit On 9/11/25 2:17 PM, Alexandre Courbot wrote: > On Thu Sep 11, 2025 at 8:22 PM JST, Danilo Krummrich wrote: >> On 9/11/25 1:04 PM, Alexandre Courbot wrote: >>> + /// Attempt to start the GSP. >>> + /// >>> + /// This is a GPU-dependent and complex procedure that involves loading firmware files from >>> + /// user-space, patching them with signatures, and building firmware-specific intricate data >>> + /// structures that the GSP will use at runtime. >>> + /// >>> + /// Upon return, the GSP is up and running, and its runtime object given as return value. >>> + pub(crate) fn start_gsp( >>> + pdev: &pci::Device, >>> + bar: &Bar0, >>> + chipset: Chipset, >>> + gsp_falcon: &Falcon, >>> + _sec2_falcon: &Falcon, >>> + ) -> Result<()> {> + let dev = pdev.as_ref(); >>> + >>> + let bios = Vbios::new(dev, bar)?; >>> + >>> + let fb_layout = FbLayout::new(chipset, bar)?; >>> + dev_dbg!(dev, "{:#x?}\n", fb_layout); >>> + >>> + Self::run_fwsec_frts(dev, gsp_falcon, bar, &bios, &fb_layout)?; >>> + >>> + // Return an empty placeholder for now, to be replaced with the GSP runtime data. >>> + Ok(()) >>> + } >> >> I'd rather create the Gsp structure already, move the code to Gsp::new() and >> return an impl PinInit. If you don't want to store any of the >> object instances you create above yet, you can just stuff all the code into an >> initializer code block, as you do in the next patch with >> gfw::wait_gfw_boot_completion(). > > I don't think that would work, or be any better even if it did. The full > GSP initialization is pretty complex and all we need to return is one > object created at the beginning that doesn't need to be pinned. > Moreover, the process is also dependent on the GPU family and completely > different on Hopper/Blackwell. Why would it not work? There is no difference between the code above being executed from an initializer block or directly in Gsp::new(). > You can see the whole process on [1]. `libos` is the object that is > returned (although its name and type will change). All the rest it > loading, preparing and running firmware, and that is done on the GPU. I > think it would be very out of place in the GSP module. > > It is also very step-by-step: run this firmware, wait for it to > complete, run another one, wait for a specific message from the GSP, run > the sequencer, etc. And most of this stuff is thrown away once the GSP > is running. That's where the limits of what we can do with `pin_init!` > are reached, and the GSP object doesn't need to be pinned anyway. I don't see that, in the code you linked you have a bunch of calls that don't return anything that needs to survive, this can be in an initializer block. And then you have let mut libos = gsp::GspMemObjects::new(pdev, bar)?; which only needs the device reference and the bar reference. So you can easily write this as: try_pin_init!(Self { _: { // all the throw-away stuff from above }, libos <- gsp::GspMemObjects::new(pdev, bar), _: { libos.do_some_stuff_mutable()?; } }) > By keeping the initialization in the GPU, we can keep the GSP object > architecture-independent, and I think it makes sense from a design point > of view. That's not to say this code should be in `gpu.rs`, maybe we > want to move it to a GPU HAL, or if we really want this as part of the > GSP a `gsp/boot` module supporting all the different archs. But I'd > prefer to think about this when we start supporting several > architectures. Didn't we talk about a struct Gsp that will eventually be returned by Self::start_gsp(), or did I make this up in my head? The way I think about this is that we'll have a struct Gsp that represents the entry point in the driver to mess with the GSP command queue. But either way, this throws up two questions, if Self::start_gsp() return a struct GspMemObjects instead (which is probably the same thing with a different name), then: Are we sure this won't need any locks? If it will need locking (which I expect) then it needs pin-init. If it never needs pinning why did you write it as gsp <- Self::start_gsp(pdev, bar, spec.chipset, gsp_falcon, sec2_falcon)?, in a patch 3? > [1] https://github.com/Gnurou/linux/blob/gsp_init_rebase/drivers/gpu/nova-core/gpu.rs#L305