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 CB8B13112C4; Wed, 3 Sep 2025 14:54:03 +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=1756911243; cv=none; b=TUSwO+vShVI3MVO79TFH2Rab87YaZPBLuP4uG+iOk7kXXq7vGFzAXC1J2RAsEo1XkgWTkAJ2rCiaIhqPZYflzeh4kqs30Sq4QGxyXdctD8Xkkn2CIB4RkAB7qx04yUN5+VELEhZypC48JgyPqW0EtDl73wYHur/qCcbsOpqDer8= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1756911243; c=relaxed/simple; bh=7dxGcjkVQEaBQwL4M7LziNmEIKiJ5GLHnlBKDFlV1x4=; h=Mime-Version:Content-Type:Date:Message-Id:Cc:To:From:Subject: References:In-Reply-To; b=bAn+53cwaWYOJ3eLdzlkvl7XvlZc9yOgMzPcbi2UUzHdhioETuJbxGw7hq3zxS2PvoLeM945p3NZIfc2XRxBue112R7y5srSNgFb0BPSFPkIUfwqrg+A1m7pNDCMHkkm3RonrPLt0K9Mwq8BaP2L6jjEdU7jI5WrGDjwE8Vf0cc= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=cOYnk5FL; 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="cOYnk5FL" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 5DED0C4CEF0; Wed, 3 Sep 2025 14:53:59 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1756911243; bh=7dxGcjkVQEaBQwL4M7LziNmEIKiJ5GLHnlBKDFlV1x4=; h=Date:Cc:To:From:Subject:References:In-Reply-To:From; b=cOYnk5FLKNlwh9PbZjvvg9kqFbsgBfKkUoUjIFl87XE4B3zWld6UdRWlywX2SnV1/ j4Quq4c2Mjixm6/oxIaFuMMm5KNp5Y+a3EtVEA3maIcpQtxmpo3IgDffU9l96ieClU s4nQW7A9gQjfBCdMV9gqLVd4igDUp0zIh9TCDdX1YfKj3qp7IQmUEGgbzNl2DkAE5Z 8QNJ2AJuKi4js9nSBkrFMuenQ/ktrly/VMSdIs8bVVBqOhKJdgx9Lj0e6DOdLPnV2G PyvhG/4RqwBYiPOVP/pKhI+aYCGQMDwUIXsLjaDBfwBZoVvVOtPs95IeTTAhbqqQoW kaf5jPmxdl4ag== Precedence: bulk X-Mailing-List: rust-for-linux@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Wed, 03 Sep 2025 16:53:57 +0200 Message-Id: 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" Subject: Re: [PATCH v3 02/11] gpu: nova-core: move GSP boot code out of `Gpu` constructor References: <20250902-nova_firmware-v3-0-56854d9c5398@nvidia.com> <20250902-nova_firmware-v3-2-56854d9c5398@nvidia.com> In-Reply-To: On Wed Sep 3, 2025 at 2:29 PM CEST, Alexandre Courbot wrote: > To be honest I am not completely sure about the best layout yet and will > need more visibility to understand whether this is optimal. But > considering that we want to run the GSP boot process over a built `Gpu` > instance, we cannot store the result of said process inside `Gpu` unless > we put it inside e.g. an `Option`. But then the variant will always be > `Some` after `probe` returns, and yet we will have to perform a match > every time we want to access it. > > The current separation sounds reasonable to me for the time being, with > `Gpu` containing purely hardware resources obtained without help from > user-space, while `Gsp` is the result of running a bunch of firmwares. > An alternative design would be to store `Gpu` inside `Gsp`, but `Gsp` > inside `Gpu` is trickier due to the build order. No matter what we do, > switching the layout later should be trivial if we don't choose the > best one now. Gsp should be part of the Gpu object. The Gpu object represents the entire instance of the Gpu, including hardware ressources, firmware runtime state,= etc. The initialization of the Gsp structure doesn't really need a Gpu structure= to be constructed, it needs certain members of the Gpu structure, i.e. order o= f initialization of the members does matter. If it makes things more obvious we can always create new types and increase= the hierarchy within the Gpu struct itself. The technical limitation you're facing is always the same, no matter the la= yout we choose: we need pin-init to provide us references to already initialized members. I will check with Benno in today's Rust call what's the best way to address this. > There is also an easy workaround to the sibling initialization issue, > which is to store `Gpu` and `Gsp` behind `Pin` - that way we can > initialize both outside `try_pin_init!`, at the cost of two more heap > allocations over the whole lifetime of the device. If we don't have a > proper solution to the problem now, this might be better than using > `unsafe` as a temporary solution. Yeah, this workaround is much easier to implement when they're siblings (le= ss allocations temporarily), but let's not design things this way because of t= hat. As mentioned above, I will check with Benno today. > The same workaround could also be used for to `GspFirmware` and its page > tables - since `GspFirmware` is temporary and can apparently be > discarded after the GSP is booted, this shouldn't be a big issue. This > will allow the driver to probe, and we can add TODO items to fix that > later if a solution is in sight. > >> >> I thought the intent was to keep temporary values local to start_gsp() a= nd not >> store them next to Gpu in the same allocation? > > It is not visible in the current patchset, but `start_gsp` will > eventually return the runtime data of the GSP - notably its log buffers > and command queue, which are needed to operate it. All the rest (notably > the loaded firmwares) will be local to `start_gsp` and discarded upon > its return. Ok, that makes sense, but it should really be part of the Gpu structure.