Message-Id: <20250619-nova-frts-v6-24-ecf41ef99252@nvidia.com>
Date: Thu, 19 Jun 2025 22:24:08 +0900
From: Alexandre Courbot <acourbot@...dia.com>
To: 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>, 
 Andreas Hindborg <a.hindborg@...nel.org>, Alice Ryhl <aliceryhl@...gle.com>, 
 Trevor Gross <tmgross@...ch.edu>, Danilo Krummrich <dakr@...nel.org>, 
 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>, 
 Benno Lossin <lossin@...nel.org>
Cc: John Hubbard <jhubbard@...dia.com>, Ben Skeggs <bskeggs@...dia.com>, 
 Joel Fernandes <joelagnelf@...dia.com>, Timur Tabi <ttabi@...dia.com>, 
 Alistair Popple <apopple@...dia.com>, linux-kernel@...r.kernel.org, 
 rust-for-linux@...r.kernel.org, nouveau@...ts.freedesktop.org, 
 dri-devel@...ts.freedesktop.org, Alexandre Courbot <acourbot@...dia.com>
Subject: [PATCH v6 24/24] gpu: nova-core: update and annotate TODO list

A few new dependencies are required to remove some of the TODO items:

- A way to safely convert from byte slices to types implementing
  `FromBytes`,
- A way to obtain slices and write into a `CoherentAllocation`,
- Several improvements to the `register!()` macro,
- Alignment operations to powers of two, and an equivalent to the C
  `fls`,
- Support for `xa_alloc` in the XAlloc bindings.

Some items have also become obsolete:

- The auxiliary bus abstractions have been implemented and are in use,
- The ELF utilities are not considered for being part of the core kernel
  bindings anymore.
- VBIOS, falcon and GPU timer have been completed.

We now have quite a few TODO entries in the code, so annotate them with
a 4 letter code representing the corresponding task in `todo.rst`. This
allows to easily find which part of the code corresponds to a given
entry (and conversely).

Signed-off-by: Alexandre Courbot <acourbot@...dia.com>
---
 Documentation/gpu/nova/core/todo.rst      | 107 ++++++++++++++++--------------
 drivers/gpu/nova-core/dma.rs              |   2 +-
 drivers/gpu/nova-core/driver.rs           |   2 +-
 drivers/gpu/nova-core/falcon.rs           |   8 ++-
 drivers/gpu/nova-core/falcon/hal/ga102.rs |  10 +--
 drivers/gpu/nova-core/fb.rs               |   2 +-
 drivers/gpu/nova-core/firmware/fwsec.rs   |   6 +-
 drivers/gpu/nova-core/gfw.rs              |   2 +-
 drivers/gpu/nova-core/gpu.rs              |   2 +-
 drivers/gpu/nova-core/regs.rs             |   8 +--
 drivers/gpu/nova-core/regs/macros.rs      |   2 +-
 drivers/gpu/nova-core/util.rs             |   2 +-
 drivers/gpu/nova-core/vbios.rs            |   2 +-
 13 files changed, 84 insertions(+), 71 deletions(-)

diff --git a/Documentation/gpu/nova/core/todo.rst b/Documentation/gpu/nova/core/todo.rst
index 8a459fc088121f770bfcda5dfb4ef51c712793ce..894a1e9c3741a43ad4eb76d24a9486862999874e 100644
--- a/Documentation/gpu/nova/core/todo.rst
+++ b/Documentation/gpu/nova/core/todo.rst
@@ -14,14 +14,17 @@ Tasks may have the following fields:
 - ``Contact``: The person that can be contacted for further information about
   the task.
 
+A task might have `[ABCD]` code after its name. This code can be used to grep
+into the code for `TODO` entries related to it.
+
 Enablement (Rust)
 =================
 
 Tasks that are not directly related to nova-core, but are preconditions in terms
 of required APIs.
 
-FromPrimitive API
------------------
+FromPrimitive API [FPRI]
+------------------------
 
 Sometimes the need arises to convert a number to a value of an enum or a
 structure.
@@ -41,8 +44,27 @@ automatically generates the corresponding mappings between a value and a number.
 | Complexity: Beginner
 | Link: https://docs.rs/num/latest/num/trait.FromPrimitive.html
 
-Generic register abstraction
-----------------------------
+Conversion from byte slices for types implementing FromBytes [TRSM]
+-------------------------------------------------------------------
+
+We retrieve several structures from byte streams coming from the BIOS or loaded
+firmware. At the moment converting the bytes slice into the proper type require
+an inelegant `unsafe` operation; this will go away once `FromBytes` implements
+a proper `from_bytes` method.
+
+| Complexity: Beginner
+
+CoherentAllocation improvements [COHA]
+--------------------------------------
+
+`CoherentAllocation` needs a safe way to write into the allocation, and to
+obtain slices within the allocation.
+
+| Complexity: Beginner
+| Contact: Abdiel Janulgue
+
+Generic register abstraction [REGA]
+-----------------------------------
 
 Work out how register constants and structures can be automatically generated
 through generalized macros.
@@ -102,16 +124,40 @@ Usage:
 	let boot0 = Boot0::read(&bar);
 	pr_info!("Revision: {}\n", boot0.revision());
 
-Note: a work-in-progress implementation currently resides in
+A work-in-progress implementation currently resides in
 `drivers/gpu/nova-core/regs/macros.rs` and is used in nova-core. It would be
 nice to improve it (possibly using proc macros) and move it to the `kernel`
 crate so it can be used by other components as well.
 
+Features desired before this happens:
+
+* Relative register with build-time base address validation,
+* Arrays of registers with build-time index validation,
+* Make I/O optional I/O (for field values that are not registers),
+* Support other sizes than `u32`,
+* Allow visibility control for registers and individual fields,
+* Use Rust slice syntax to express fields ranges.
+
 | Complexity: Advanced
 | Contact: Alexandre Courbot
 
-Delay / Sleep abstractions
---------------------------
+Numerical operations [NUMM]
+---------------------------
+
+Nova uses integer operations that are not part of the standard library (or not
+implemented in an optimized way for the kernel). These include:
+
+- Aligning up and down to a power of two,
+- The "Find Last Set Bit" (`fls` function of the C part of the kernel)
+  operation.
+
+A `num` core kernel module is being designed to provide these operations.
+
+| Complexity: Intermediate
+| Contact: Alexandre Courbot
+
+Delay / Sleep abstractions [DLAY]
+---------------------------------
 
 Rust abstractions for the kernel's delay() and sleep() functions.
 
@@ -159,18 +205,6 @@ mailing list yet.
 | Complexity: Intermediate
 | Contact: Abdiel Janulgue
 
-ELF utils
----------
-
-Rust implementation of ELF header representation to retrieve section header
-tables, names, and data from an ELF-formatted images.
-
-There is preceding work from Abdiel Janulgue, which hasn't made it to the
-mailing list yet.
-
-| Complexity: Beginner
-| Contact: Abdiel Janulgue
-
 PCI MISC APIs
 -------------
 
@@ -179,12 +213,11 @@ capability, MSI API abstractions.
 
 | Complexity: Beginner
 
-Auxiliary bus abstractions
---------------------------
+XArray bindings [XARR]
+----------------------
 
-Rust abstraction for the auxiliary bus APIs.
-
-This is needed to connect nova-core to the nova-drm driver.
+We need bindings for `xa_alloc`/`xa_alloc_cyclic` in order to generate the
+auxiliary device IDs.
 
 | Complexity: Intermediate
 
@@ -216,15 +249,6 @@ Build the radix3 page table to map the firmware.
 | Complexity: Intermediate
 | Contact: Abdiel Janulgue
 
-vBIOS support
--------------
-
-Parse the vBIOS and probe the structures required for driver initialization.
-
-| Contact: Dave Airlie
-| Reference: Vec extensions
-| Complexity: Intermediate
-
 Initial Devinit support
 -----------------------
 
@@ -234,23 +258,6 @@ configuration.
 | Contact: Dave Airlie
 | Complexity: Beginner
 
-Boot Falcon controller
-----------------------
-
-Infrastructure to load and execute falcon (sec2) firmware images; handle the
-GSP falcon processor and fwsec loading.
-
-| Complexity: Advanced
-| Contact: Dave Airlie
-
-GPU Timer support
------------------
-
-Support for the GPU's internal timer peripheral.
-
-| Complexity: Beginner
-| Contact: Dave Airlie
-
 MMU / PT management
 -------------------
 
diff --git a/drivers/gpu/nova-core/dma.rs b/drivers/gpu/nova-core/dma.rs
index 1f1f8c378d8e2cf51edc772e7afe392e9c9c8831..94f44bcfd748d18ea42c520e36a618bde9635e55 100644
--- a/drivers/gpu/nova-core/dma.rs
+++ b/drivers/gpu/nova-core/dma.rs
@@ -26,7 +26,7 @@ pub(crate) fn new(dev: &device::Device<device::Bound>, len: usize) -> Result<Sel
 
     pub(crate) fn from_data(dev: &device::Device<device::Bound>, data: &[u8]) -> Result<Self> {
         Self::new(dev, data.len()).map(|mut dma_obj| {
-            // TODO: replace with `CoherentAllocation::write()` once available.
+            // TODO[COHA]: replace with `CoherentAllocation::write()` once available.
             // SAFETY:
             // - `dma_obj`'s size is at least `data.len()`.
             // - We have just created this object and there is no other user at this stage.
diff --git a/drivers/gpu/nova-core/driver.rs b/drivers/gpu/nova-core/driver.rs
index ffe25c7a2fdad289549460f7fd87d6e09299a35c..518ef8739550fd0b63b5a4aa98cd1fd814770725 100644
--- a/drivers/gpu/nova-core/driver.rs
+++ b/drivers/gpu/nova-core/driver.rs
@@ -42,7 +42,7 @@ fn probe(pdev: &pci::Device<Core>, _info: &Self::IdInfo) -> Result<Pin<KBox<Self
                 _reg: auxiliary::Registration::new(
                     pdev.as_ref(),
                     c_str!("nova-drm"),
-                    0, // TODO: Once it lands, use XArray; for now we don't use the ID.
+                    0, // TODO[XARR]: Once it lands, use XArray; for now we don't use the ID.
                     crate::MODULE_NAME
                 )?,
             }),
diff --git a/drivers/gpu/nova-core/falcon.rs b/drivers/gpu/nova-core/falcon.rs
index fe4d3d458a6b105bfdd6257111d3eed8ed8aba7c..07be1c30668c4bef9e073fe6ad49234aceb7fb81 100644
--- a/drivers/gpu/nova-core/falcon.rs
+++ b/drivers/gpu/nova-core/falcon.rs
@@ -35,6 +35,7 @@ pub(crate) enum FalconCoreRev {
     Rev7 = 7,
 }
 
+// TODO[FPRI]: replace with `FromPrimitive`.
 impl TryFrom<u8> for FalconCoreRev {
     type Error = Error;
 
@@ -68,6 +69,7 @@ pub(crate) enum FalconCoreRevSubversion {
     Subversion3 = 3,
 }
 
+// TODO[FPRI]: replace with `FromPrimitive`.
 impl TryFrom<u8> for FalconCoreRevSubversion {
     type Error = Error;
 
@@ -101,6 +103,7 @@ pub(crate) enum FalconSecurityModel {
     Heavy = 3,
 }
 
+// TODO[FPRI]: replace with `FromPrimitive`.
 impl TryFrom<u8> for FalconSecurityModel {
     type Error = Error;
 
@@ -128,6 +131,7 @@ pub(crate) enum FalconModSelAlgo {
     Rsa3k = 1,
 }
 
+// TODO[FPRI]: replace with `FromPrimitive`.
 impl TryFrom<u8> for FalconModSelAlgo {
     type Error = Error;
 
@@ -148,6 +152,7 @@ pub(crate) enum DmaTrfCmdSize {
     Size256B = 0x6,
 }
 
+// TODO[FPRI]: replace with `FromPrimitive`.
 impl TryFrom<u8> for DmaTrfCmdSize {
     type Error = Error;
 
@@ -199,6 +204,7 @@ pub(crate) enum FalconFbifTarget {
     NoncoherentSysmem = 2,
 }
 
+// TODO[FPRI]: replace with `FromPrimitive`.
 impl TryFrom<u8> for FalconFbifTarget {
     type Error = Error;
 
@@ -354,7 +360,7 @@ fn reset_eng(&self, bar: &Bar0) -> Result {
 
         regs::NV_PFALCON_FALCON_ENGINE::alter(bar, E::BASE, |v| v.set_reset(true));
 
-        // TODO: replace with udelay() or equivalent once available.
+        // TODO[DLAY]: replace with udelay() or equivalent once available.
         // TIMEOUT: falcon engine should not take more than 10us to reset.
         let _: Result = util::wait_on(Duration::from_micros(10), || None);
 
diff --git a/drivers/gpu/nova-core/falcon/hal/ga102.rs b/drivers/gpu/nova-core/falcon/hal/ga102.rs
index 0a4e5e7adf8cbcec9f67bb09ba758a9cb2887bae..664327f75cf4199cca37d22ca18b2b9abac781f8 100644
--- a/drivers/gpu/nova-core/falcon/hal/ga102.rs
+++ b/drivers/gpu/nova-core/falcon/hal/ga102.rs
@@ -42,10 +42,10 @@ fn signature_reg_fuse_version_ga102(
     engine_id_mask: u16,
     ucode_id: u8,
 ) -> Result<u32> {
-    // TODO: The ucode fuse versions are contained in the FUSE_OPT_FPF_<ENGINE>_UCODE<X>_VERSION
-    // registers, which are an array. Our register definition macros do not allow us to manage them
-    // properly, so we need to hardcode their addresses for now. Clean this up once we support
-    // register arrays.
+    // TODO[REGA]: The ucode fuse versions are contained in the
+    // FUSE_OPT_FPF_<ENGINE>_UCODE<X>_VERSION registers, which are an array. Our register
+    // definition macros do not allow us to manage them properly, so we need to hardcode their
+    // addresses for now. Clean this up once we support register arrays.
 
     // Each engine has 16 ucode version registers numbered from 1 to 16.
     if ucode_id == 0 || ucode_id > 16 {
@@ -69,7 +69,7 @@ fn signature_reg_fuse_version_ga102(
     let reg_fuse_version =
         bar.read32(reg_fuse_base + ((ucode_id - 1) as usize * core::mem::size_of::<u32>()));
 
-    // TODO: replace with `last_set_bit` once it lands.
+    // TODO[NUMM]: replace with `last_set_bit` once it lands.
     Ok(u32::BITS - reg_fuse_version.leading_zeros())
 }
 
diff --git a/drivers/gpu/nova-core/fb.rs b/drivers/gpu/nova-core/fb.rs
index dc009a3ed44c2de7ffeb8cc0be06a72cf2ca5309..48003527a2472a4a8b784af0d481a441c8d2426e 100644
--- a/drivers/gpu/nova-core/fb.rs
+++ b/drivers/gpu/nova-core/fb.rs
@@ -121,7 +121,7 @@ pub(crate) fn new(chipset: Chipset, bar: &Bar0) -> Result<Self> {
         let frts = {
             const FRTS_DOWN_ALIGN: u64 = SZ_128K as u64;
             const FRTS_SIZE: u64 = SZ_1M as u64;
-            // TODO: replace with `align_down` once it lands.
+            // TODO[NUMM]: replace with `align_down` once it lands.
             let frts_base = (vga_workspace.start & !(FRTS_DOWN_ALIGN - 1)) - FRTS_SIZE;
 
             frts_base..frts_base + FRTS_SIZE
diff --git a/drivers/gpu/nova-core/firmware/fwsec.rs b/drivers/gpu/nova-core/firmware/fwsec.rs
index 6058598ce76e25484cc4ebebd1be80b9dd1b469c..047aab76470ecb0a0486f6917f6fda69b5381391 100644
--- a/drivers/gpu/nova-core/firmware/fwsec.rs
+++ b/drivers/gpu/nova-core/firmware/fwsec.rs
@@ -150,8 +150,8 @@ impl FirmwareSignature<FwsecFirmware> for Bcrt30Rsa3kSignature {}
 /// Callers must ensure that the region of memory returned is not written for as long as the
 /// returned reference is alive.
 ///
-/// TODO: Remove this and `transmute_mut` once `CoherentAllocation::as_slice` is available and we
-/// have a way to transmute objects implementing FromBytes, e.g.:
+/// TODO[TRSM][COHA]: Remove this and `transmute_mut` once `CoherentAllocation::as_slice` is
+/// available and we have a way to transmute objects implementing FromBytes, e.g.:
 /// https://lore.kernel.org/lkml/20250330234039.29814-1-christiansantoslima21@gmail.com/
 unsafe fn transmute<'a, 'b, T: Sized + FromBytes>(
     fw: &'a DmaObject,
@@ -218,7 +218,7 @@ fn dmem_load_params(&self) -> FalconLoadTarget {
         FalconLoadTarget {
             src_start: self.desc.imem_load_size,
             dst_start: self.desc.dmem_phys_base,
-            // TODO: replace with `align_up` once it lands.
+            // TODO[NUMM]: replace with `align_up` once it lands.
             len: self
                 .desc
                 .dmem_load_size
diff --git a/drivers/gpu/nova-core/gfw.rs b/drivers/gpu/nova-core/gfw.rs
index fa3f642bc814c7eea1ce1f2c2e24e684d1ae5fda..97fdd311a1f842c1c4e5c28d10bb26066a7aa586 100644
--- a/drivers/gpu/nova-core/gfw.rs
+++ b/drivers/gpu/nova-core/gfw.rs
@@ -31,7 +31,7 @@ pub(crate) fn wait_gfw_boot_completion(bar: &Bar0) -> Result {
         } else {
             // Avoid busy-looping.
             // SAFETY: msleep should be safe to call with any parameter.
-            // TODO: replace with [1] once it merges.
+            // TODO[DLAY]: replace with [1] once it merges.
             // [1] https://lore.kernel.org/rust-for-linux/20250423192857.199712-6-fujita.tomonori@gmail.com/
             unsafe { bindings::msleep(1) };
 
diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
index ae454c0e2fb4d485e99fbf9cd80c2ebb89884887..7a6ff6d89cb2a5e3176489e54552d04633861ad6 100644
--- a/drivers/gpu/nova-core/gpu.rs
+++ b/drivers/gpu/nova-core/gpu.rs
@@ -37,7 +37,7 @@ impl Chipset {
             ];
         }
 
-        // TODO replace with something like derive(FromPrimitive)
+        // TODO[FPRI]: replace with something like derive(FromPrimitive)
         impl TryFrom<u32> for Chipset {
             type Error = kernel::error::Error;
 
diff --git a/drivers/gpu/nova-core/regs.rs b/drivers/gpu/nova-core/regs.rs
index ccfaeed55cff90e66ac0acf37dcbd0eb344994c5..707f87d6828df54c959af87fd13bbdd3a25aa020 100644
--- a/drivers/gpu/nova-core/regs.rs
+++ b/drivers/gpu/nova-core/regs.rs
@@ -44,7 +44,7 @@ pub(crate) fn chipset(self) -> Result<Chipset> {
 
 /* PBUS */
 
-// TODO: this is an array of registers.
+// TODO[REGA]: this is an array of registers.
 register!(NV_PBUS_SW_SCRATCH_0E@...0001438  {
     31:16   frts_err_code as u16;
 });
@@ -110,7 +110,7 @@ pub(crate) fn higher_bound(self) -> u64 {
     0:0     read_protection_level0 as bool, "Set after FWSEC lowers its protection level";
 });
 
-// TODO: This is an array of registers.
+// TODO[REGA]: This is an array of registers.
 register!(NV_PGC6_AON_SECURE_SCRATCH_GROUP_05 @ 0x00118234 {
     31:0    value as u32;
 });
@@ -272,7 +272,7 @@ pub(crate) fn mem_scrubbing_done(self) -> bool {
     0:0     reset as bool;
 });
 
-// TODO: this is an array of registers.
+// TODO[REGA]: this is an array of registers.
 register!(NV_PFALCON_FBIF_TRANSCFG @ +0x00000600 {
     1:0     target as u8 ?=> FalconFbifTarget;
     2:2     mem_type as bool => FalconFbifMemType;
@@ -294,7 +294,7 @@ pub(crate) fn mem_scrubbing_done(self) -> bool {
     31:0    value as u32;
 });
 
-// TODO: this is an array of registers.
+// TODO[REGA]: this is an array of registers.
 register!(NV_PFALCON2_FALCON_BROM_PARAADDR @ +0x00001210 {
     31:0    value as u32;
 });
diff --git a/drivers/gpu/nova-core/regs/macros.rs b/drivers/gpu/nova-core/regs/macros.rs
index e0e6fef3796f9dd2ce4e0223444a05bcc53075a6..cdf668073480ed703c89ffa8628f5c9de6494687 100644
--- a/drivers/gpu/nova-core/regs/macros.rs
+++ b/drivers/gpu/nova-core/regs/macros.rs
@@ -147,7 +147,7 @@ impl $name {
             pub(crate) const OFFSET: usize = $offset;
         }
 
-        // TODO: display the raw hex value, then the value of all the fields. This requires
+        // TODO[REGA]: display the raw hex value, then the value of all the fields. This requires
         // matching the fields, which will complexify the syntax considerably...
         impl ::core::fmt::Debug for $name {
             fn fmt(&self, f: &mut ::core::fmt::Formatter<'_>) -> ::core::fmt::Result {
diff --git a/drivers/gpu/nova-core/util.rs b/drivers/gpu/nova-core/util.rs
index 69f29238b25ed949b00def1b748df3ff7567d83c..5cafe0797cd6f9567afb7e1e9af23b961a8a87f6 100644
--- a/drivers/gpu/nova-core/util.rs
+++ b/drivers/gpu/nova-core/util.rs
@@ -32,7 +32,7 @@ pub(crate) const fn const_bytes_to_str(bytes: &[u8]) -> &str {
 /// `Err(ETIMEDOUT)` is returned if `timeout` has been reached without `cond` evaluating to
 /// `Some`.
 ///
-/// TODO: replace with `read_poll_timeout` once it is available.
+/// TODO[DLAY]: replace with `read_poll_timeout` once it is available.
 /// (https://lore.kernel.org/lkml/20250220070611.214262-8-fujita.tomonori@gmail.com/)
 pub(crate) fn wait_on<R, F: Fn() -> Option<R>>(timeout: Duration, cond: F) -> Result<R> {
     let start_time = Instant::now();
diff --git a/drivers/gpu/nova-core/vbios.rs b/drivers/gpu/nova-core/vbios.rs
index c029c600b9b3081ad1e1dd4112acd4ed914e9d8d..a5889eb149a16beabc0ddbdc87666520114c8aec 100644
--- a/drivers/gpu/nova-core/vbios.rs
+++ b/drivers/gpu/nova-core/vbios.rs
@@ -175,7 +175,7 @@ fn next(&mut self) -> Option<Self::Item> {
 
         // Advance to next image (aligned to 512 bytes)
         self.current_offset += image_size;
-        // TODO: replace with `align_up` once it lands.
+        // TODO[NUMM]: replace with `align_up` once it lands.
         self.current_offset = self.current_offset.next_multiple_of(512);
 
         Some(Ok(full_image))

-- 
2.49.0

Powered by blists - more mailing lists