Skip to contentSkip to conversation

gpu: nova: adopt driver lifetime infrastructure

PATCH Applied Danilo Krummrich 3 participants 13 messages May 25 – May 29

Subsystems (5)
lore
Share & tools
lore

Danilo Krummrich

author maintainer
#0 lore raw

cover gpu: nova: adopt driver lifetime infrastructure

Adopt the driver core lifetime infrastructure for nova.

Use the lifetime-bound pci::Bar directly in NovaCore, eliminating the Arc<Devres<Bar0>> indirection. This lets SysmemFlush borrow the Bar and implement Drop for automatic cleanup.

Replace ARef<Device> with plain borrows in SysmemFlush and the GSP sequencer, where the structs are already lifetime-parameterized.

Separate the driver type from the driver data to allow the private data to be lifetime-parameterized via the Data GAT.

This patch series is based on [1] and drm-rust-next.

[1] https://lore.kernel.org/driver-core/[email protected]/

Danilo Krummrich (5):
gpu: nova-core: use lifetime for Bar
gpu: nova-core: unregister sysmem flush page from Drop
gpu: nova-core: replace ARef<Device> with &'bound Device in
SysmemFlush
gpu: nova-core: gsp: replace ARef<Device> with &'a Device in sequencer
gpu: nova: separate driver type from driver data

 drivers/gpu/drm/nova/driver.rs         | 12 ++++----
 drivers/gpu/nova-core/driver.rs        | 32 +++++++++-------------
 drivers/gpu/nova-core/fb.rs            | 31 ++++++++++-----------
 drivers/gpu/nova-core/gpu.rs           | 38 ++++++++------------------
 drivers/gpu/nova-core/gsp/boot.rs      |  2 +-
 drivers/gpu/nova-core/gsp/sequencer.rs | 11 ++++----
 6 files changed, 52 insertions(+), 74 deletions(-)
Signature · 1 line
2.54.0
author maintainer
#1 lore raw

PATCH 1/5 gpu: nova-core: use lifetime for Bar

Take advantage of the lifetime-parameterized pci::Bar<'bound> to hold the BAR mapping directly in NovaCore<'bound>, and pass a borrowed reference to Gpu<'bound>.

This eliminates the Arc<Devres<Bar0>> indirection, removes runtime revocation checks for BAR access, and simplifies Gpu::unbind().

---
 drivers/gpu/nova-core/driver.rs | 32 +++++++++++++++-----------------
 drivers/gpu/nova-core/gpu.rs    | 33 +++++++++++++--------------------
 2 files changed, 28 insertions(+), 37 deletions(-)
drivers/gpu/nova-core/driver.rs +15 -17
index d3f2245ba2e0..d4cf4379ee87 100644
@@ -13,12 +13,9 @@
     },
     prelude::*,
     sizes::SZ_16M,
-    sync::{
-        atomic::{
-            Atomic,
-            Relaxed, //
-        },
-        Arc,
+    sync::atomic::{
+        Atomic,
+        Relaxed, //
     },
     types::ForLt,
 };
@@ -31,7 +28,8 @@
 #[pin_data]
 pub(crate) struct NovaCore<'bound> {
     #[pin]
-    pub(crate) gpu: Gpu,
+    pub(crate) gpu: Gpu<'bound>,
+    bar: pci::Bar<'bound, BAR0_SIZE>,
     #[allow(clippy::type_complexity)]
     _reg: auxiliary::Registration<'bound, ForLt!(())>,
 }
@@ -48,7 +46,7 @@ pub(crate) struct NovaCore<'bound> {
 // DMA addresses. These systems should be quite rare.
 const GPU_DMA_BITS: u32 = 47;
 
-pub(crate) type Bar0 = pci::Bar<'static, BAR0_SIZE>;
+pub(crate) type Bar0 = kernel::io::Mmio<BAR0_SIZE>;
 
 kernel::pci_device_table!(
     PCI_TABLE,
@@ -95,14 +93,14 @@ fn probe<'bound>(
             // other threads of execution.
             unsafe { pdev.dma_set_mask_and_coherent(DmaMask::new::<GPU_DMA_BITS>())? };
 
-            let bar = Arc::new(
-                pdev.iomap_region_sized::<BAR0_SIZE>(0, c"nova-core/bar0")?
-                    .into_devres()?,
-                GFP_KERNEL,
-            )?;
-
             Ok(try_pin_init!(NovaCore {
-                gpu <- Gpu::new(pdev, bar.clone(), bar.access(pdev.as_ref())?),
+                bar: pdev.iomap_region_sized::<BAR0_SIZE>(0, c"nova-core/bar0")?,
+                // TODO: Use `&bar` self-referential pin-init syntax once available.
+                //
+                // SAFETY: `bar` is initialized before this expression is evaluated
+                // (`try_pin_init!()` initializes fields in declaration order), lives at a pinned
+                // stable address, and is dropped after `gpu` (struct field drop order).
+                gpu <- Gpu::new(pdev, unsafe { &*core::ptr::from_ref(bar) }),
                 _reg: auxiliary::Registration::new(
                     pdev.as_ref(),
                     c"nova-drm",
@@ -116,7 +114,7 @@ fn probe<'bound>(
         })
     }
 
-    fn unbind<'bound>(pdev: &'bound pci::Device<Core<'_>>, this: Pin<&Self::Data<'bound>>) {
-        this.gpu.unbind(pdev.as_ref());
+    fn unbind<'bound>(_pdev: &'bound pci::Device<Core<'_>>, this: Pin<&Self::Data<'bound>>) {
+        this.gpu.unbind();
     }
 }
drivers/gpu/nova-core/gpu.rs +13 -20
index 78c4195e6132..108dd094e354 100644
@@ -2,13 +2,11 @@
 
 use kernel::{
     device,
-    devres::Devres,
     fmt,
     io::Io,
     num::Bounded,
     pci,
-    prelude::*,
-    sync::Arc, //
+    prelude::*, //
 };
 
 use crate::{
@@ -246,10 +244,10 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
 
 /// Structure holding the resources required to operate the GPU.
 #[pin_data]
-pub(crate) struct Gpu {
+pub(crate) struct Gpu<'gpu> {
     spec: Spec,
-    /// MMIO mapping of PCI BAR 0
-    bar: Arc<Devres<Bar0>>,
+    /// MMIO mapping of PCI BAR 0.
+    bar: &'gpu Bar0,
     /// System memory page required for flushing all pending GPU-side memory writes done through
     /// PCIE into system memory, via sysmembar (A GPU-initiated HW memory-barrier operation).
     sysmem_flush: SysmemFlush,
@@ -262,12 +260,11 @@ pub(crate) struct Gpu {
     gsp: Gsp,
 }
 
-impl Gpu {
-    pub(crate) fn new<'a>(
-        pdev: &'a pci::Device<device::Bound>,
-        devres_bar: Arc<Devres<Bar0>>,
-        bar: &'a Bar0,
-    ) -> impl PinInit<Self, Error> + 'a {
+impl<'gpu> Gpu<'gpu> {
+    pub(crate) fn new(
+        pdev: &'gpu pci::Device<device::Bound>,
+        bar: &'gpu Bar0,
+    ) -> impl PinInit<Self, Error> + 'gpu {
         try_pin_init!(Self {
             spec: Spec::new(pdev.as_ref(), bar).inspect(|spec| {
                 dev_info!(pdev,"NVIDIA ({})\n", spec);
@@ -279,6 +276,8 @@ pub(crate) fn new<'a>(
                     .inspect_err(|_| dev_err!(pdev, "GFW boot did not complete\n"))?;
             },
 
+            bar,
+
             sysmem_flush: SysmemFlush::register(pdev.as_ref(), bar, spec.chipset)?,
 
             gsp_falcon: Falcon::new(
@@ -292,19 +291,13 @@ pub(crate) fn new<'a>(
             gsp <- Gsp::new(pdev),
 
             _: { gsp.boot(pdev, bar, spec.chipset, gsp_falcon, sec2_falcon)? },
-
-            bar: devres_bar,
         })
     }
 
     /// Called when the corresponding [`Device`](device::Device) is unbound.
     ///
     /// Note: This method must only be called from `Driver::unbind`.
-    pub(crate) fn unbind(&self, dev: &device::Device<device::Core<'_>>) {
-        kernel::warn_on!(self
-            .bar
-            .access(dev)
-            .inspect(|bar| self.sysmem_flush.unregister(bar))
-            .is_err());
+    pub(crate) fn unbind(&self) {
+        self.sysmem_flush.unregister(self.bar);
     }
 }
Signature · 1 line
2.54.0
Reviewed-by on v1 1/5
#2 lore raw

Re: [PATCH 1/5] gpu: nova-core: use lifetime for Bar

Quoted 9 lines · Danilo Krummrich

Take advantage of the lifetime-parameterized pci::Bar<'bound> to hold the BAR mapping directly in NovaCore<'bound>, and pass a borrowed reference to Gpu<'bound>.

This eliminates the Arc<Devres<Bar0>> indirection, removes runtime revocation checks for BAR access, and simplifies Gpu::unbind().

---

author maintainer
#3 lore raw

PATCH 2/5 gpu: nova-core: unregister sysmem flush page from Drop

In reply to Danilo Krummrich

Now that SysmemFlush can borrow the Bar via HRT lifetime, store a &'bound Bar0 reference and implement Drop to automatically unregister the sysmem flush page. This removes the need for manual unregister() calls and the Gpu::unbind() method.

Fixes 6554ad65b589 ("gpu: nova-core: register sysmem flush page")
---
 drivers/gpu/nova-core/driver.rs |  4 ----
 drivers/gpu/nova-core/fb.rs     | 22 ++++++++++------------
 drivers/gpu/nova-core/gpu.rs    |  9 +--------
 3 files changed, 11 insertions(+), 24 deletions(-)
drivers/gpu/nova-core/driver.rs +0 -4
index d4cf4379ee87..cff5034c2dcd 100644
@@ -113,8 +113,4 @@ fn probe<'bound>(
             }))
         })
     }
-
-    fn unbind<'bound>(_pdev: &'bound pci::Device<Core<'_>>, this: Pin<&Self::Data<'bound>>) {
-        this.gpu.unbind();
-    }
 }
drivers/gpu/nova-core/fb.rs +10 -12
index 6ee87050ce69..3b3271790cc9 100644
@@ -43,21 +43,20 @@
 /// Because of this, the sysmem flush memory page must be registered as early as possible during
 /// driver initialization, and before any falcon is reset.
 ///
-/// Users are responsible for manually calling [`Self::unregister`] before dropping this object,
-/// otherwise the GPU might still use it even after it has been freed.
-pub(crate) struct SysmemFlush {
+pub(crate) struct SysmemFlush<'sys> {
     /// Chipset we are operating on.
     chipset: Chipset,
     device: ARef<device::Device>,
+    bar: &'sys Bar0,
     /// Keep the page alive as long as we need it.
     page: CoherentHandle,
 }
 
-impl SysmemFlush {
+impl<'sys> SysmemFlush<'sys> {
     /// Allocate a memory page and register it as the sysmem flush page.
     pub(crate) fn register(
         dev: &device::Device<device::Bound>,
-        bar: &Bar0,
+        bar: &'sys Bar0,
         chipset: Chipset,
     ) -> Result<Self> {
         let page = CoherentHandle::alloc(dev, kernel::page::PAGE_SIZE, GFP_KERNEL)?;
@@ -67,19 +66,18 @@ pub(crate) fn register(
         Ok(Self {
             chipset,
             device: dev.into(),
+            bar,
             page,
         })
     }
+}
 
-    /// Unregister the managed sysmem flush page.
-    ///
-    /// In order to gracefully tear down the GPU, users must make sure to call this method before
-    /// dropping the object.
-    pub(crate) fn unregister(&self, bar: &Bar0) {
+impl Drop for SysmemFlush<'_> {
+    fn drop(&mut self) {
         let hal = hal::fb_hal(self.chipset);
 
-        if hal.read_sysmem_flush_page(bar) == self.page.dma_handle() {
-            let _ = hal.write_sysmem_flush_page(bar, 0).inspect_err(|e| {
+        if hal.read_sysmem_flush_page(self.bar) == self.page.dma_handle() {
+            let _ = hal.write_sysmem_flush_page(self.bar, 0).inspect_err(|e| {
                 dev_warn!(
                     &self.device,
                     "failed to unregister sysmem flush page: {:?}\n",
drivers/gpu/nova-core/gpu.rs +1 -8
index 108dd094e354..cf134cab49cd 100644
@@ -250,7 +250,7 @@ pub(crate) struct Gpu<'gpu> {
     bar: &'gpu Bar0,
     /// System memory page required for flushing all pending GPU-side memory writes done through
     /// PCIE into system memory, via sysmembar (A GPU-initiated HW memory-barrier operation).
-    sysmem_flush: SysmemFlush,
+    sysmem_flush: SysmemFlush<'gpu>,
     /// GSP falcon instance, used for GSP boot up and cleanup.
     gsp_falcon: Falcon<GspFalcon>,
     /// SEC2 falcon instance, used for GSP boot up and cleanup.
@@ -293,11 +293,4 @@ pub(crate) fn new(
             _: { gsp.boot(pdev, bar, spec.chipset, gsp_falcon, sec2_falcon)? },
         })
     }
-
-    /// Called when the corresponding [`Device`](device::Device) is unbound.
-    ///
-    /// Note: This method must only be called from `Driver::unbind`.
-    pub(crate) fn unbind(&self) {
-        self.sysmem_flush.unregister(self.bar);
-    }
 }
Signature · 1 line
2.54.0
Reviewed-by on v1 2/5
#4 lore raw

Re: [PATCH 2/5] gpu: nova-core: unregister sysmem flush page from Drop

Quoted 10 lines · Danilo Krummrich

Now that SysmemFlush can borrow the Bar via HRT lifetime, store a &'bound Bar0 reference and implement Drop to automatically unregister the sysmem flush page. This removes the need for manual unregister() calls and the Gpu::unbind() method.

Fixes 6554ad65b589 ("gpu: nova-core: register sysmem flush page")

---

author maintainer
#5 lore raw

PATCH 3/5 gpu: nova-core: replace ARef<Device> with &'bound Device in SysmemFlush

In reply to Danilo Krummrich

Now that SysmemFlush is lifetime-parameterized, the ARef<Device> is unnecessary -- a plain &'bound Device reference suffices.

---
 drivers/gpu/nova-core/fb.rs | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)
drivers/gpu/nova-core/fb.rs +4 -5
index 3b3271790cc9..1fb65d4eb290 100644
@@ -15,8 +15,7 @@
         Alignable,
         Alignment, //
     },
-    sizes::*,
-    sync::aref::ARef, //
+    sizes::*, //
 };
 
 use crate::{
@@ -46,7 +45,7 @@
 pub(crate) struct SysmemFlush<'sys> {
     /// Chipset we are operating on.
     chipset: Chipset,
-    device: ARef<device::Device>,
+    device: &'sys device::Device,
     bar: &'sys Bar0,
     /// Keep the page alive as long as we need it.
     page: CoherentHandle,
@@ -55,7 +54,7 @@ pub(crate) struct SysmemFlush<'sys> {
 impl<'sys> SysmemFlush<'sys> {
     /// Allocate a memory page and register it as the sysmem flush page.
     pub(crate) fn register(
-        dev: &device::Device<device::Bound>,
+        dev: &'sys device::Device<device::Bound>,
         bar: &'sys Bar0,
         chipset: Chipset,
     ) -> Result<Self> {
@@ -65,7 +64,7 @@ pub(crate) fn register(
 
         Ok(Self {
             chipset,
-            device: dev.into(),
+            device: dev,
             bar,
             page,
         })
Signature · 1 line
2.54.0
author maintainer
#7 lore raw

PATCH 4/5 gpu: nova-core: gsp: replace ARef<Device> with &'a Device in sequencer

In reply to Danilo Krummrich

GspSequencer, GspSeqIter, and GspSequencerParams are already lifetime-parameterized; the ARef<Device> is unnecessary -- a plain &'a Device reference suffices.

---
 drivers/gpu/nova-core/gsp/boot.rs      |  2 +-
 drivers/gpu/nova-core/gsp/sequencer.rs | 11 +++++------
 2 files changed, 6 insertions(+), 7 deletions(-)
drivers/gpu/nova-core/gsp/boot.rs +1 -1
index df105ef4b371..3e8f9611d2b3 100644
@@ -234,7 +234,7 @@ pub(crate) fn boot(
             libos_dma_handle: libos_handle,
             gsp_falcon,
             sec2_falcon,
-            dev: pdev.as_ref().into(),
+            dev,
             bar,
         };
         GspSequencer::run(&self.cmdq, seq_params)?;
drivers/gpu/nova-core/gsp/sequencer.rs +5 -6
index 474e4c8021db..b3015483ed17 100644
@@ -11,7 +11,6 @@
         Io, //
     },
     prelude::*,
-    sync::aref::ARef,
     time::{
         delay::fsleep,
         Delta, //
@@ -142,7 +141,7 @@ pub(crate) struct GspSequencer<'a> {
     /// Bootloader application version.
     bootloader_app_version: u32,
     /// Device for logging.
-    dev: ARef<device::Device>,
+    dev: &'a device::Device,
 }
 
 impl fw::RegWritePayload {
@@ -281,7 +280,7 @@ pub(crate) struct GspSeqIter<'a> {
     /// Number of commands processed so far.
     cmds_processed: u32,
     /// Device for logging.
-    dev: ARef<device::Device>,
+    dev: &'a device::Device,
 }
 
 impl<'a> Iterator for GspSeqIter<'a> {
@@ -309,7 +308,7 @@ fn next(&mut self) -> Option<Self::Item> {
             self.cmd_data.len() - offset
         };
         buffer[..copy_len].copy_from_slice(&self.cmd_data[offset..offset + copy_len]);
-        let cmd_result = GspSeqCmd::new(&buffer, &self.dev);
+        let cmd_result = GspSeqCmd::new(&buffer, self.dev);
 
         cmd_result.map_or_else(
             |_err| {
@@ -334,7 +333,7 @@ fn iter(&self) -> GspSeqIter<'_> {
             current_offset: 0,
             total_cmds: self.seq_info.cmd_index,
             cmds_processed: 0,
-            dev: self.dev.clone(),
+            dev: self.dev,
         }
     }
 }
@@ -350,7 +349,7 @@ pub(crate) struct GspSequencerParams<'a> {
     /// SEC2 falcon for core operations.
     pub(crate) sec2_falcon: &'a Falcon<Sec2>,
     /// Device for logging.
-    pub(crate) dev: ARef<device::Device>,
+    pub(crate) dev: &'a device::Device,
     /// BAR0 for register access.
     pub(crate) bar: &'a Bar0,
 }
Signature · 1 line
2.54.0
author maintainer
#9 lore raw

PATCH 5/5 gpu: nova: separate driver type from driver data

In reply to Danilo Krummrich

Split NovaDriver into a unit struct for trait implementations and a separate Nova struct for the private driver data.

---
 drivers/gpu/drm/nova/driver.rs | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)
drivers/gpu/drm/nova/driver.rs +7 -5
index e4765bc5b3ec..4289df7de01c 100644
@@ -15,9 +15,11 @@
 use crate::file::File;
 use crate::gem::NovaObject;
 
-pub(crate) struct NovaDriver {
+pub(crate) struct NovaDriver;
+
+pub(crate) struct Nova {
     #[expect(unused)]
-    drm: ARef<drm::Device<Self>>,
+    drm: ARef<drm::Device<NovaDriver>>,
 }
 
 /// Convienence type alias for the DRM device type for this driver
@@ -51,19 +53,19 @@ pub(crate) struct NovaData {
 
 impl auxiliary::Driver for NovaDriver {
     type IdInfo = ();
-    type Data<'bound> = Self;
+    type Data<'bound> = Nova;
     const ID_TABLE: auxiliary::IdTable<Self::IdInfo> = &AUX_TABLE;
 
     fn probe<'bound>(
         adev: &'bound auxiliary::Device<Core<'_>>,
         _info: &'bound Self::IdInfo,
-    ) -> impl PinInit<Self, Error> + 'bound {
+    ) -> impl PinInit<Self::Data<'bound>, Error> + 'bound {
         let data = try_pin_init!(NovaData { adev: adev.into() });
 
         let drm = drm::Device::<Self>::new(adev.as_ref(), data)?;
         drm::Registration::new_foreign_owned(&drm, adev.as_ref(), 0)?;
 
-        Ok(Self { drm })
+        Ok(Nova { drm })
     }
 }
 
Signature · 1 line
2.54.0
maintainer Reviewed-by Tested-by on the v1 series
#11 lore raw
In reply to Danilo Krummrich
Quoted 9 lines · Danilo Krummrich

Adopt the driver core lifetime infrastructure for nova.

Use the lifetime-bound pci::Bar directly in NovaCore, eliminating the Arc<Devres<Bar0>> indirection. This lets SysmemFlush borrow the Bar and implement Drop for automatic cleanup.

Replace ARef<Device> with plain borrows in SysmemFlush and the GSP sequencer, where the structs are already lifetime-parameterized.

Quoted 6 lines

Separate the driver type from the driver data to allow the private data to be lifetime-parameterized via the Data GAT.

This patch series is based on [1] and drm-rust-next.

[1] https://lore.kernel.org/driver-core/[email protected]/

The series,

author maintainer Applied
#12 lore raw
In reply to Danilo Krummrich
Quoted 1 line · Danilo Krummrich

[PATCH 0/5] gpu: nova: adopt driver lifetime infrastructure

Applied, thanks!

Branch: drm-rust-next
Tree: https://gitlab.freedesktop.org/drm/rust/kernel.git

[1/5] gpu: nova-core: use lifetime for Bar

      commit: 88b70f5a05af
[2/5] gpu: nova-core: unregister sysmem flush page from Drop
      commit: d8143c9e988b
[3/5] gpu: nova-core: replace ARef<Device> with &'bound Device in SysmemFlush
      commit: 17b6544ec141
[4/5] gpu: nova-core: gsp: replace ARef<Device> with &'a Device in sequencer
      commit: 859805f070bb
[5/5] gpu: nova: separate driver type from driver data
      commit: dc395c2831b5

The patches will appear in the next linux-next integration (typically within 24 hours on weekdays).

The patches are queued up for the upcoming merge window for the next major kernel release.