Skip to contentSkip to conversation

gpu: nova-core: expose the logging buffers via debugfs

PATCH Superseded Timur Tabi 3 participants 11 messages Mar 17 – Mar 19

Subsystems (6)

v10 of 11 Superseded Latest is v11 →

lore
Share & tools
lore

Timur Tabi

author
#0 lore raw

v10 cover gpu: nova-core: expose the logging buffers via debugfs

GSP-RM writes its printf messages to "logging buffers", which are blocks of memory allocated by the driver. The messages are encoded, so exposing the buffers as debugfs entries allows the buffers to be extracted and decoded by a special application.

When the driver loads, a /sys/kernel/debug/nova_core root entry is created. To do this, the normal module_pci_driver! macro call is replaced with an explicit initialization function, as this allows that debugfs entry to be created once for all GPUs.

Then in each GPU's initialization, a subdirectory based on the PCI BDF name is created, and the logging buffer entries are created under that.

This series depends on Eliot Courtney's Cmdq patch set [1].

Note: the debugfs entry has a file size of 0, because debugfs defaults a 0 size and the Rust abstractions do not adjust it for the same of the object. Nouveau makes this adjustment manually in the driver.

[1] https://lore.kernel.org/all/[email protected]/

Changes since v9:
1. Rebased onto drm-rust-next and resolved merge conflicts
2. Replaced c_str! macro with 'c' prefix
3. Improved safety and function comments in uaccess.rs
4. Replaced self.count() with more appropos self.size()
5. Added example to write_dma()

Timur Tabi (6):
rust: device: add device name method
rust: uaccess: add write_dma() for copying from DMA buffers to
userspace
rust: dma: implement BinaryWriter for CoherentAllocation<u8>
gpu: nova-core: Replace module_pci_driver! with explicit module init
gpu: nova-core: create debugfs root in module init
gpu: nova-core: create GSP-RM logging buffers debugfs entries

 drivers/gpu/nova-core/gsp.rs       | 48 +++++++++++++---
 drivers/gpu/nova-core/nova_core.rs | 50 +++++++++++++++-
 rust/helpers/device.c              |  5 ++
 rust/kernel/device.rs              | 11 ++++
 rust/kernel/dma.rs                 | 35 +++++++++++-
 rust/kernel/uaccess.rs             | 92 ++++++++++++++++++++++++++----
 6 files changed, 220 insertions(+), 21 deletions(-)

base-commit: 76bce7ac51673640a4a46236ea723cf5543268d7
prerequisite-patch-id: fefd403caf8af386276351dd12397dda8ae8553f
prerequisite-patch-id: 3e02192944c4dde97e6895a28371479aa49ddc96
prerequisite-patch-id: f5f44c25a817e1e539a8418c88fe1e99d06b2c34
prerequisite-patch-id: 45041e4ac74e3ea76ea8dd0632e7746418bdf822
prerequisite-patch-id: d4f34f7eda8f5dcad2464f1bf3789e78556eb51c

Signature · 1 line
2.53.0
author
#1 lore raw

PATCH v10 1/6 rust: device: add device name method

Add a name() method to the Device type, which returns a CStr that contains the device name.

---
 rust/helpers/device.c |  5 +++++
 rust/kernel/device.rs | 11 +++++++++++
 2 files changed, 16 insertions(+)
rust/helpers/device.c +5 -0
index a8ab931a9bd1..3be4ee590784 100644
@@ -25,3 +25,8 @@ __rust_helper void rust_helper_dev_set_drvdata(struct device *dev, void *data)
 {
 	dev_set_drvdata(dev, data);
 }
+
+__rust_helper const char *rust_helper_dev_name(const struct device *dev)
+{
+	return dev_name(dev);
+}
rust/kernel/device.rs +11 -0
index 94e0548e7687..8f6ad4f53101 100644
@@ -489,6 +489,17 @@ pub fn fwnode(&self) -> Option<&property::FwNode> {
         // defined as a `#[repr(transparent)]` wrapper around `fwnode_handle`.
         Some(unsafe { &*fwnode_handle.cast() })
     }
+
+    /// Returns the name of the device.
+    ///
+    /// This is the kobject name of the device, or its initial name if the kobject is not yet
+    /// available.
+    #[inline]
+    pub fn name(&self) -> &CStr {
+        // SAFETY: By its type invariant `self.as_raw()` is a valid pointer to a `struct device`.
+        // The returned string is valid for the lifetime of the device.
+        unsafe { CStr::from_char_ptr(bindings::dev_name(self.as_raw())) }
+    }
 }
 
 // SAFETY: `Device` is a transparent wrapper of a type that doesn't depend on `Device`'s generic
Signature · 1 line
2.53.0
author
#2 lore raw

PATCH v10 2/6 rust: uaccess: add write_dma() for copying from DMA buffers to userspace

In reply to Timur Tabi

Add UserSliceWriter::write_dma() to copy data from a CoherentAllocation<u8> to userspace. This provides a safe interface for copying DMA buffer contents to userspace without requiring callers to work with raw pointers.

Because write_dma() and write_slice() have common code, factor that code out into a helper function, write_raw().

The method handles bounds checking and offset calculation internally, wrapping the unsafe copy_to_user() call.

---
 rust/kernel/uaccess.rs | 92 +++++++++++++++++++++++++++++++++++++-----
 1 file changed, 82 insertions(+), 10 deletions(-)
rust/kernel/uaccess.rs +82 -10
index f989539a31b4..c9004167c08d 100644
@@ -7,6 +7,7 @@
 use crate::{
     alloc::{Allocator, Flags},
     bindings,
+    dma::CoherentAllocation,
     error::Result,
     ffi::{c_char, c_void},
     fs::file,
@@ -459,20 +460,20 @@ pub fn is_empty(&self) -> bool {
         self.length == 0
     }
 
-    /// Writes raw data to this user pointer from a kernel buffer.
+    /// Low-level write from a raw pointer.
     ///
-    /// Fails with [`EFAULT`] if the write happens on a bad address, or if the write goes out of
-    /// bounds of this [`UserSliceWriter`]. This call may modify the associated userspace slice even
-    /// if it returns an error.
-    pub fn write_slice(&mut self, data: &[u8]) -> Result {
-        let len = data.len();
-        let data_ptr = data.as_ptr().cast::<c_void>();
+    /// # Safety
+    ///
+    /// The caller must ensure that `from` is valid for reads of `len` bytes.
+    unsafe fn write_raw(&mut self, from: *const u8, len: usize) -> Result {
         if len > self.length {
             return Err(EFAULT);
         }
-        // SAFETY: `data_ptr` points into an immutable slice of length `len`, so we may read
-        // that many bytes from it.
-        let res = unsafe { bindings::copy_to_user(self.ptr.as_mut_ptr(), data_ptr, len) };
+
+        // SAFETY: Caller guarantees `from` is valid for `len` bytes (see this function's
+        // safety contract).
+        let res =
+            unsafe { bindings::copy_to_user(self.ptr.as_mut_ptr(), from.cast::<c_void>(), len) };
         if res != 0 {
             return Err(EFAULT);
         }
@@ -481,6 +482,77 @@ pub fn write_slice(&mut self, data: &[u8]) -> Result {
         Ok(())
     }
 
+    /// Writes raw data to this user pointer from a kernel buffer.
+    ///
+    /// Fails with [`EFAULT`] if the write happens on a bad address, or if the write goes out of
+    /// bounds of this [`UserSliceWriter`]. This call may modify the associated userspace slice even
+    /// if it returns an error.
+    pub fn write_slice(&mut self, data: &[u8]) -> Result {
+        // SAFETY: `data` is a valid slice, so `data.as_ptr()` is valid for
+        // reading `data.len()` bytes.
+        unsafe { self.write_raw(data.as_ptr(), data.len()) }
+    }
+
+    /// Writes raw data to this user pointer from a DMA coherent allocation.
+    ///
+    /// Copies `count` bytes from `alloc` starting from `offset` into this userspace slice.
+    ///
+    /// # Errors
+    ///
+    /// - [`EOVERFLOW`]: `offset + count` overflows.
+    /// - [`ERANGE`]: `offset + count` exceeds the size of `alloc`, or `count` exceeds the
+    ///   size of the user-space buffer.
+    /// - [`EFAULT`]: the write hits a bad address or goes out of bounds of this
+    ///   [`UserSliceWriter`].
+    ///
+    /// This call may modify the associated userspace slice even if it returns an error.
+    ///
+    /// Note: The memory may be concurrently modified by hardware (e.g., DMA). In such cases,
+    /// the copied data may be inconsistent, but this does not cause undefined behavior.
+    ///
+    /// # Example
+    ///
+    /// Copy the first 256 bytes of a DMA coherent allocation into a userspace buffer:
+    ///
+    /// ```no_run
+    /// use kernel::uaccess::UserSliceWriter;
+    /// use kernel::dma::CoherentAllocation;
+    ///
+    /// fn copy_dma_to_user(
+    ///     mut writer: UserSliceWriter,
+    ///     alloc: &CoherentAllocation<u8>,
+    /// ) -> Result {
+    ///     writer.write_dma(alloc, 0, 256)
+    /// }
+    /// ```
+    pub fn write_dma(
+        &mut self,
+        alloc: &CoherentAllocation<u8>,
+        offset: usize,
+        count: usize,
+    ) -> Result {
+        let len = alloc.size();
+        if offset.checked_add(count).ok_or(EOVERFLOW)? > len {
+            return Err(ERANGE);
+        }
+
+        if count > self.len() {
+            return Err(ERANGE);
+        }
+
+        // SAFETY: `start_ptr()` returns a valid pointer to a memory region of `count()` bytes,
+        // as guaranteed by the `CoherentAllocation` invariants. The check above ensures
+        // `offset + count <= len`.
+        let src_ptr = unsafe { alloc.start_ptr().add(offset) };
+
+        // Note: Use `write_raw` instead of `write_slice` because the allocation is coherent
+        // memory that hardware may modify (e.g., DMA); we cannot form a `&[u8]` slice over
+        // such volatile memory.
+        //
+        // SAFETY: `src_ptr` points into the allocation and is valid for `count` bytes (see above).
+        unsafe { self.write_raw(src_ptr, count) }
+    }
+
     /// Writes raw data to this user pointer from a kernel buffer partially.
     ///
     /// This is the same as [`Self::write_slice`] but considers the given `offset` into `data` and
Signature · 1 line
2.53.0
author
#3 lore raw

PATCH v10 3/6 rust: dma: implement BinaryWriter for CoherentAllocation<u8>

In reply to Timur Tabi

Implement the BinaryWriter trait for CoherentAllocation<u8>, enabling DMA coherent allocations to be exposed as readable binary files. The implementation handles offset tracking and bounds checking, copying data from the coherent allocation to userspace via write_dma().

---
 rust/kernel/dma.rs | 35 ++++++++++++++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)
rust/kernel/dma.rs +34 -1
index a396f8435739..453fc547a0fc 100644
@@ -5,12 +5,14 @@
 //! C header: [`include/linux/dma-mapping.h`](srctree/include/linux/dma-mapping.h)
 
 use crate::{
-    bindings, build_assert, device,
+    bindings, build_assert, debugfs, device,
     device::{Bound, Core},
     error::{to_result, Result},
+    fs::file,
     prelude::*,
     sync::aref::ARef,
     transmute::{AsBytes, FromBytes},
+    uaccess::UserSliceWriter,
 };
 use core::ptr::NonNull;
 
@@ -664,6 +666,37 @@ fn drop(&mut self) {
 // can be sent to another thread.
 unsafe impl<T: AsBytes + FromBytes + Send> Send for CoherentAllocation<T> {}
 
+// SAFETY: Sharing `&CoherentAllocation` across threads is safe if `T` is `Sync`, because all
+// methods that access the buffer contents (`field_read`, `field_write`, `as_slice`,
+// `as_slice_mut`) are `unsafe`, and callers are responsible for ensuring no data races occur.
+// The safe methods only return metadata or raw pointers whose use requires `unsafe`.
+unsafe impl<T: AsBytes + FromBytes + Sync> Sync for CoherentAllocation<T> {}
+
+impl debugfs::BinaryWriter for CoherentAllocation<u8> {
+    fn write_to_slice(
+        &self,
+        writer: &mut UserSliceWriter,
+        offset: &mut file::Offset,
+    ) -> Result<usize> {
+        if offset.is_negative() {
+            return Err(EINVAL);
+        }
+
+        // If the offset is too large for a usize (e.g. on 32-bit platforms),
+        // then consider that as past EOF and just return 0 bytes.
+        let Ok(offset_val) = usize::try_from(*offset) else {
+            return Ok(0);
+        };
+
+        let count = self.size().saturating_sub(offset_val).min(writer.len());
+
+        writer.write_dma(self, offset_val, count)?;
+
+        *offset += count as i64;
+        Ok(count)
+    }
+}
+
 /// Reads a field of an item from an allocated region of structs.
 ///
 /// The syntax is of the form `kernel::dma_read!(dma, proj)` where `dma` is an expression evaluating
Signature · 1 line
2.53.0
author
#4 lore raw

PATCH v10 4/6 gpu: nova-core: Replace module_pci_driver! with explicit module init

In reply to Timur Tabi

Replace the module_pci_driver! macro with an explicit module initialization using the standard module! macro and InPlaceModule trait implementation. No functional change intended, with the exception that the driver now prints a message when loaded.

This change is necessary so that we can create a top-level "nova_core" debugfs entry when the driver is loaded.

---
 drivers/gpu/nova-core/nova_core.rs | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)
drivers/gpu/nova-core/nova_core.rs +23 -2
index b5caf1044697..0114a59825aa 100644
@@ -2,6 +2,13 @@
 
 //! Nova Core GPU Driver
 
+use kernel::{
+    driver::Registration,
+    pci,
+    prelude::*,
+    InPlaceModule, //
+};
+
 #[macro_use]
 mod bitfield;
 
@@ -20,8 +27,22 @@
 
 pub(crate) const MODULE_NAME: &core::ffi::CStr = <LocalModule as kernel::ModuleMetadata>::NAME;
 
-kernel::module_pci_driver! {
-    type: driver::NovaCore,
+#[pin_data]
+struct NovaCoreModule {
+    #[pin]
+    _driver: Registration<pci::Adapter<driver::NovaCore>>,
+}
+
+impl InPlaceModule for NovaCoreModule {
+    fn init(module: &'static kernel::ThisModule) -> impl PinInit<Self, Error> {
+        try_pin_init!(Self {
+            _driver <- Registration::new(MODULE_NAME, module),
+        })
+    }
+}
+
+module! {
+    type: NovaCoreModule,
     name: "NovaCore",
     authors: ["Danilo Krummrich"],
     description: "Nova Core GPU driver",
Signature · 1 line
2.53.0
maintainer Reviewed-by on v10 4/6
#5 lore raw

Re: [PATCH v10 4/6] gpu: nova-core: Replace module_pci_driver! with explicit module init

Quoted 5 lines · Timur Tabi

Replace the module_pci_driver! macro with an explicit module initialization using the standard module! macro and InPlaceModule trait implementation. No functional change intended, with the exception that the driver now prints a message when loaded.

Quoted 7 lines

This change is necessary so that we can create a top-level "nova_core" debugfs entry when the driver is loaded.

author
#6 lore raw

PATCH v10 5/6 gpu: nova-core: create debugfs root in module init

In reply to Timur Tabi

Create the 'nova_core' root debugfs entry when the driver loads.

Normally, non-const global variables need to be protected by a mutex. Instead, we use unsafe code, as we know the entry is never modified after the driver is loaded. This solves the lifetime issue of the mutex guard, which would otherwise have required the use of pin_init_scope.

---
 drivers/gpu/nova-core/nova_core.rs | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)
drivers/gpu/nova-core/nova_core.rs +25 -0
index 0114a59825aa..b7e6c6274aa8 100644
@@ -3,6 +3,7 @@
 //! Nova Core GPU Driver
 
 use kernel::{
+    debugfs,
     driver::Registration,
     pci,
     prelude::*,
@@ -27,16 +28,40 @@
 
 pub(crate) const MODULE_NAME: &core::ffi::CStr = <LocalModule as kernel::ModuleMetadata>::NAME;
 
+// FIXME: Move this into per-module data once that exists
+static mut DEBUGFS_ROOT: Option<debugfs::Dir> = None;
+
+/// Guard that clears DEBUGFS_ROOT when dropped.
+struct DebugfsRootGuard;
+
+impl Drop for DebugfsRootGuard {
+    fn drop(&mut self) {
+        // SAFETY: This guard is dropped after _driver (due to field order),
+        // so the driver is unregistered and no probe() can be running.
+        unsafe { DEBUGFS_ROOT = None };
+    }
+}
+
 #[pin_data]
 struct NovaCoreModule {
+    // Fields are dropped in declaration order, so _driver is dropped first,
+    // then _debugfs_guard clears DEBUGFS_ROOT.
     #[pin]
     _driver: Registration<pci::Adapter<driver::NovaCore>>,
+    _debugfs_guard: DebugfsRootGuard,
 }
 
 impl InPlaceModule for NovaCoreModule {
     fn init(module: &'static kernel::ThisModule) -> impl PinInit<Self, Error> {
+        let dir = debugfs::Dir::new(kernel::c_str!("nova_core"));
+
+        // SAFETY: We are the only driver code running during init, so there
+        // cannot be any concurrent access to `DEBUGFS_ROOT`.
+        unsafe { DEBUGFS_ROOT = Some(dir) };
+
         try_pin_init!(Self {
             _driver <- Registration::new(MODULE_NAME, module),
+            _debugfs_guard: DebugfsRootGuard,
         })
     }
 }
Signature · 1 line
2.53.0
maintainer Reviewed-by on v10 5/6
#7 lore raw

Re: [PATCH v10 5/6] gpu: nova-core: create debugfs root in module init

Quoted 8 lines · Timur Tabi

Create the 'nova_core' root debugfs entry when the driver loads.

Normally, non-const global variables need to be protected by a mutex. Instead, we use unsafe code, as we know the entry is never modified after the driver is loaded. This solves the lifetime issue of the mutex guard, which would otherwise have required the use of pin_init_scope.

Quoted 5 lines

---

With a few nitty nits below.

Quoted 16 lines
 drivers/gpu/nova-core/nova_core.rs | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)
drivers/gpu/nova-core/nova_core.rs +1 -0
index 0114a59825aa..b7e6c6274aa8 100644
@@ -3,6 +3,7 @@
 //! Nova Core GPU Driver
 
 use kernel::{
+    debugfs,
     driver::Registration,
     pci,
     prelude::*,

drivers/gpu/nova-core/nova_core.rs line 31

Quoted 4 lines
drivers/gpu/nova-core/nova_core.rs +1 -0
@@ -27,16 +28,40 @@
 
 pub(crate) const MODULE_NAME: &core::ffi::CStr = <LocalModule as kernel::ModuleMetadata>::NAME;
 
+// FIXME: Move this into per-module data once that exists

I'd change this to do TODO: for easier discovery.

drivers/gpu/nova-core/nova_core.rs line 34

Quoted 3 lines
drivers/gpu/nova-core/nova_core.rs +3 -0
+static mut DEBUGFS_ROOT: Option<debugfs::Dir> = None;
+
+/// Guard that clears DEBUGFS_ROOT when dropped.

Item names need `` quotes.

drivers/gpu/nova-core/nova_core.rs line 40

Quoted 6 lines
drivers/gpu/nova-core/nova_core.rs +6 -0
+struct DebugfsRootGuard;
+
+impl Drop for DebugfsRootGuard {
+    fn drop(&mut self) {
+        // SAFETY: This guard is dropped after _driver (due to field order),
+        // so the driver is unregistered and no probe() can be running.

Here as well (even if it's not a doccomment).

drivers/gpu/nova-core/nova_core.rs line 48

Quoted 8 lines
drivers/gpu/nova-core/nova_core.rs +6 -0
+        unsafe { DEBUGFS_ROOT = None };
+    }
+}
+
 #[pin_data]
 struct NovaCoreModule {
+    // Fields are dropped in declaration order, so _driver is dropped first,
+    // then _debugfs_guard clears DEBUGFS_ROOT.

And here.

author
#8 lore raw

PATCH v10 6/6 gpu: nova-core: create GSP-RM logging buffers debugfs entries

In reply to Timur Tabi

Create read-only debugfs entries for LOGINIT, LOGRM, and LOGINTR, which are the three primary printf logging buffers from GSP-RM. LOGPMU will be added at a later date, as it requires it support for its RPC message first.

This patch uses the pin_init_scope feature to create the entries. pin_init_scope solves the lifetime issue over the DEBUGFS_ROOT reference by delaying its acquisition until the time the entry is actually initialized.

---
 drivers/gpu/nova-core/gsp.rs | 48 ++++++++++++++++++++++++++++++------
 1 file changed, 40 insertions(+), 8 deletions(-)
drivers/gpu/nova-core/gsp.rs +40 -8
index 72f173726f87..9399783ccb8d 100644
@@ -3,6 +3,7 @@
 mod boot;
 
 use kernel::{
+    debugfs,
     device,
     dma::{
         CoherentAllocation,
@@ -102,17 +103,24 @@ fn new(dev: &device::Device<device::Bound>) -> Result<Self> {
     }
 }
 
-/// GSP runtime data.
-#[pin_data]
-pub(crate) struct Gsp {
-    /// Libos arguments.
-    pub(crate) libos: CoherentAllocation<LibosMemoryRegionInitArgument>,
+/// Log buffers used by GSP-RM for debug logging.
+struct LogBuffers {
     /// Init log buffer.
     loginit: LogBuffer,
     /// Interrupts log buffer.
     logintr: LogBuffer,
     /// RM log buffer.
     logrm: LogBuffer,
+}
+
+/// GSP runtime data.
+#[pin_data]
+pub(crate) struct Gsp {
+    /// Libos arguments.
+    pub(crate) libos: CoherentAllocation<LibosMemoryRegionInitArgument>,
+    /// Log buffers, optionally exposed via debugfs.
+    #[pin]
+    logs: debugfs::Scope<LogBuffers>,
     /// Command queue.
     #[pin]
     pub(crate) cmdq: Cmdq,
@@ -126,15 +134,17 @@ pub(crate) fn new(pdev: &pci::Device<device::Bound>) -> impl PinInit<Self, Error
         pin_init::pin_init_scope(move || {
             let dev = pdev.as_ref();
 
+            // Create log buffers before try_pin_init! so they're accessible throughout
+            let loginit = LogBuffer::new(dev)?;
+            let logintr = LogBuffer::new(dev)?;
+            let logrm = LogBuffer::new(dev)?;
+
             Ok(try_pin_init!(Self {
                 libos: CoherentAllocation::<LibosMemoryRegionInitArgument>::alloc_coherent(
                     dev,
                     GSP_PAGE_SIZE / size_of::<LibosMemoryRegionInitArgument>(),
                     GFP_KERNEL | __GFP_ZERO,
                 )?,
-                loginit: LogBuffer::new(dev)?,
-                logintr: LogBuffer::new(dev)?,
-                logrm: LogBuffer::new(dev)?,
                 cmdq <- Cmdq::new(dev),
                 rmargs: CoherentAllocation::<GspArgumentsPadded>::alloc_coherent(
                     dev,
@@ -155,6 +165,28 @@ pub(crate) fn new(pdev: &pci::Device<device::Bound>) -> impl PinInit<Self, Error
                     dma_write!(rmargs, [0]?.inner, fw::GspArgumentsCached::new(&cmdq));
                     dma_write!(libos, [3]?, LibosMemoryRegionInitArgument::new("RMARGS", rmargs));
                 },
+                logs <- {
+                    let log_buffers = LogBuffers {
+                        loginit,
+                        logintr,
+                        logrm,
+                    };
+
+                    #[allow(static_mut_refs)]
+                    // SAFETY: `DEBUGFS_ROOT` is created before driver registration and cleared
+                    // after driver unregistration, so no probe() can race with its modification.
+                    // PANIC: `DEBUGFS_ROOT` cannot be `None` here.  It is set before driver
+                    // registration and cleared after driver unregistration, so it is always
+                    // `Some` for the entire lifetime that probe() can be called.
+                    let log_parent: &debugfs::Dir = unsafe { crate::DEBUGFS_ROOT.as_ref() }
+                        .expect("DEBUGFS_ROOT not initialized");
+
+                    log_parent.scope(log_buffers, dev.name(), |logs, dir| {
+                        dir.read_binary_file(c"loginit", &logs.loginit.0);
+                        dir.read_binary_file(c"logintr", &logs.logintr.0);
+                        dir.read_binary_file(c"logrm", &logs.logrm.0);
+                    })
+                },
             }))
         })
     }
Signature · 1 line
2.53.0
Tested-by on v10 6/6
#9 lore raw

Re: [PATCH v10 6/6] gpu: nova-core: create GSP-RM logging buffers debugfs entries

Quoted 5 lines · Timur Tabi

Create read-only debugfs entries for LOGINIT, LOGRM, and LOGINTR, which are the three primary printf logging buffers from GSP-RM. LOGPMU will be added at a later date, as it requires it support for its RPC message first.

Quoted 8 lines

This patch uses the pin_init_scope feature to create the entries. pin_init_scope solves the lifetime issue over the DEBUGFS_ROOT reference by delaying its acquisition until the time the entry is actually initialized.

(This patch lacked that tag, so I took a moment to retest the v10 series overall, while I'm here.)

I had to resolve a bunch of conflicts, because Eliot's patches still are against an older branch, while this branch has moved ahead.

But anyway, it passes: decodes a Turing log file with this v10 applied.

thanks,

Signature · 1 line
John Hubbard
Quoted 96 lines
---
 drivers/gpu/nova-core/gsp.rs | 48 ++++++++++++++++++++++++++++++------
 1 file changed, 40 insertions(+), 8 deletions(-)
drivers/gpu/nova-core/gsp.rs +40 -8
index 72f173726f87..9399783ccb8d 100644
@@ -3,6 +3,7 @@
 mod boot;
 
 use kernel::{
+    debugfs,
     device,
     dma::{
         CoherentAllocation,
@@ -102,17 +103,24 @@ fn new(dev: &device::Device<device::Bound>) -> Result<Self> {
     }
 }
 
-/// GSP runtime data.
-#[pin_data]
-pub(crate) struct Gsp {
-    /// Libos arguments.
-    pub(crate) libos: CoherentAllocation<LibosMemoryRegionInitArgument>,
+/// Log buffers used by GSP-RM for debug logging.
+struct LogBuffers {
     /// Init log buffer.
     loginit: LogBuffer,
     /// Interrupts log buffer.
     logintr: LogBuffer,
     /// RM log buffer.
     logrm: LogBuffer,
+}
+
+/// GSP runtime data.
+#[pin_data]
+pub(crate) struct Gsp {
+    /// Libos arguments.
+    pub(crate) libos: CoherentAllocation<LibosMemoryRegionInitArgument>,
+    /// Log buffers, optionally exposed via debugfs.
+    #[pin]
+    logs: debugfs::Scope<LogBuffers>,
     /// Command queue.
     #[pin]
     pub(crate) cmdq: Cmdq,
@@ -126,15 +134,17 @@ pub(crate) fn new(pdev: &pci::Device<device::Bound>) -> impl PinInit<Self, Error
         pin_init::pin_init_scope(move || {
             let dev = pdev.as_ref();
 
+            // Create log buffers before try_pin_init! so they're accessible throughout
+            let loginit = LogBuffer::new(dev)?;
+            let logintr = LogBuffer::new(dev)?;
+            let logrm = LogBuffer::new(dev)?;
+
             Ok(try_pin_init!(Self {
                 libos: CoherentAllocation::<LibosMemoryRegionInitArgument>::alloc_coherent(
                     dev,
                     GSP_PAGE_SIZE / size_of::<LibosMemoryRegionInitArgument>(),
                     GFP_KERNEL | __GFP_ZERO,
                 )?,
-                loginit: LogBuffer::new(dev)?,
-                logintr: LogBuffer::new(dev)?,
-                logrm: LogBuffer::new(dev)?,
                 cmdq <- Cmdq::new(dev),
                 rmargs: CoherentAllocation::<GspArgumentsPadded>::alloc_coherent(
                     dev,
@@ -155,6 +165,28 @@ pub(crate) fn new(pdev: &pci::Device<device::Bound>) -> impl PinInit<Self, Error
                     dma_write!(rmargs, [0]?.inner, fw::GspArgumentsCached::new(&cmdq));
                     dma_write!(libos, [3]?, LibosMemoryRegionInitArgument::new("RMARGS", rmargs));
                 },
+                logs <- {
+                    let log_buffers = LogBuffers {
+                        loginit,
+                        logintr,
+                        logrm,
+                    };
+
+                    #[allow(static_mut_refs)]
+                    // SAFETY: `DEBUGFS_ROOT` is created before driver registration and cleared
+                    // after driver unregistration, so no probe() can race with its modification.
+                    // PANIC: `DEBUGFS_ROOT` cannot be `None` here.  It is set before driver
+                    // registration and cleared after driver unregistration, so it is always
+                    // `Some` for the entire lifetime that probe() can be called.
+                    let log_parent: &debugfs::Dir = unsafe { crate::DEBUGFS_ROOT.as_ref() }
+                        .expect("DEBUGFS_ROOT not initialized");
+
+                    log_parent.scope(log_buffers, dev.name(), |logs, dir| {
+                        dir.read_binary_file(c"loginit", &logs.loginit.0);
+                        dir.read_binary_file(c"logintr", &logs.logintr.0);
+                        dir.read_binary_file(c"logrm", &logs.logrm.0);
+                    })
+                },
             }))
         })
     }
#10 lore raw
In reply to Timur Tabi
Quoted 10 lines · Timur Tabi

GSP-RM writes its printf messages to "logging buffers", which are blocks of memory allocated by the driver. The messages are encoded, so exposing the buffers as debugfs entries allows the buffers to be extracted and decoded by a special application.

When the driver loads, a /sys/kernel/debug/nova_core root entry is created. To do this, the normal module_pci_driver! macro call is replaced with an explicit initialization function, as this allows that debugfs entry to be created once for all GPUs.

Quoted 5 lines

Then in each GPU's initialization, a subdirectory based on the PCI BDF name is created, and the logging buffer entries are created under that.

This series depends on Eliot Courtney's Cmdq patch set [1].

The series now applies nicely by itself on drm-rust-next! I'll let Danilo coordinate its merging with the rest of the I/O related stuff (hence my Reviewed-bys on the nova-core patches).

I think it looks great now, thanks!