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 029E4C369D5 for ; Thu, 24 Apr 2025 20:09:00 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id B054510E08F; Thu, 24 Apr 2025 20:09:00 +0000 (UTC) Authentication-Results: gabe.freedesktop.org; dkim=pass (2048-bit key; unprotected) header.d=kernel.org header.i=@kernel.org header.b="ISS9edEF"; dkim-atps=neutral Received: from tor.source.kernel.org (tor.source.kernel.org [172.105.4.254]) by gabe.freedesktop.org (Postfix) with ESMTPS id CF88310E08F; Thu, 24 Apr 2025 20:08:59 +0000 (UTC) Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by tor.source.kernel.org (Postfix) with ESMTP id 18E1361137; Thu, 24 Apr 2025 20:08:37 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 20AA7C4CEE3; Thu, 24 Apr 2025 20:08:53 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1745525338; bh=r/J+71axIcbcBnTDMTPbPhfWreusoyG9PFW3KMwW3d0=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=ISS9edEF3u5Xby+MOrLIfGta/xXgvj8DjuWHfzBXrFZ9ZBVbkAwIkoDv9x14/vlmd hzy7BUxXKcII6wkglUSqd21SGKks8ao2Mb/0m5uGpwG+WWvYJL4XwU43191f7cCpJX zy0BhwhgLd7MBL5szOCwB0f9KYZB+tcTH1/At/LklVOkX6MC3pKizBD69oeN3VTsMR ku0O6Rzq6WoT1dSlZeSQRjA10kpghiyV2qSGBzVH87+Dy5b1c41PQgsNkCSjCQUTeA j1gGwCwlYLhhucBavtvJ+AQVSrgVaTuEEAoJBlBgoftI+s7Reib0R0jbd5rHQcx3BT 9fCHoitRYzsyA== Date: Thu, 24 Apr 2025 22:08:51 +0200 From: Danilo Krummrich To: Joel Fernandes Cc: Alexandre Courbot , Miguel Ojeda , Alex Gaynor , Boqun Feng , Gary Guo , =?iso-8859-1?Q?Bj=F6rn?= Roy Baron , Benno Lossin , Andreas Hindborg , Alice Ryhl , Trevor Gross , David Airlie , Simona Vetter , Maarten Lankhorst , Maxime Ripard , Thomas Zimmermann , Jonathan Corbet , John Hubbard , Ben Skeggs , Timur Tabi , Alistair Popple , linux-kernel@vger.kernel.org, rust-for-linux@vger.kernel.org, nouveau@lists.freedesktop.org, dri-devel@lists.freedesktop.org Subject: Re: [PATCH 13/16] gpu: nova-core: Add support for VBIOS ucode extraction for boot Message-ID: References: <20250420-nova-frts-v1-0-ecd1cca23963@nvidia.com> <20250420-nova-frts-v1-13-ecd1cca23963@nvidia.com> <7f3aa4b3-a24a-41c6-b75e-61e0e6e11ee3@nvidia.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <7f3aa4b3-a24a-41c6-b75e-61e0e6e11ee3@nvidia.com> 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 Thu, Apr 24, 2025 at 02:54:42PM -0400, Joel Fernandes wrote: > > > On 4/23/2025 10:06 AM, Danilo Krummrich wrote: > [...] > >> + > >> + /// Probe for VBIOS extraction > >> + /// Once the VBIOS object is built, bar0 is not read for vbios purposes anymore. > >> + pub(crate) fn probe(bar0: &Devres) -> Result { > > > > Let's not call it probe(), what about VBios::parse(), or simply VBios::new()? > > > > Yes, new() is better. I changed it. > > >> + // VBIOS data vector: As BIOS images are scanned, they are added to this vector > >> + // for reference or copying into other data structures. It is the entire > >> + // scanned contents of the VBIOS which progressively extends. It is used > >> + // so that we do not re-read any contents that are already read as we use > >> + // the cumulative length read so far, and re-read any gaps as we extend > >> + // the length > >> + let mut data = KVec::new(); > >> + > >> + // Loop through all the BiosImage and extract relevant ones and relevant data from them > >> + let mut cur_offset = 0; > > > > I suggest to create a new type that contains data and offset and implement > > read_bios_image_at_offset() and friends as methods of this type. I think this > > would turn out much cleaner. > I moved it into struct Vbios {} itself instead of introducing a new type. Is > that Ok? > > I agree it is cleaner. Please see below link for this particular refactor > (moving data) and let me know if it looks Ok to you: http://bit.ly/4lHfDKZ I still think a new type would be better, the Option> that is only used for the construction of the actual type instance is a bit weird. It's basically two types in one, which is also why you need two options -- better separate them.