[PATCH] drm/tyr: move probe resources into registration data

[PATCH] drm/tyr: move probe resources into registration data

[Deborah Brouwer]

Introduce TyrDrmRegistrationData to hold resources that need to stay alive
while the DRM device is registered. This moves the parent platform device,
clocks, regulators, MMIO mapping, firmware state, and GPU info into data
owned by the DRM registration.

The registration data is tied to the platform device binding lifetime, so
use Registration::new_with_lt() and store the returned Registration in
the platform driver data.

Add initial firmware and MMU stubs so the registration data can own the
probe-time resources that firmware loading and GPU VM setup will need.

Signed-off-by: Deborah Brouwer <deborah.brouwer@collabora.com>
---
This patch applies on top of the drm lifetime series [1] and the
full branch [2]

[1] https://lore.kernel.org/rust-for-linux/20260603011711.2077361-1-dakr@kernel.org/
[2] https://git.kernel.org/pub/scm/linux/kernel/git/dakr/linux.git/log/?h=drm-lifetime
---
 drivers/gpu/drm/tyr/driver.rs | 51 +++++++++++++++++++++++++++-----
 drivers/gpu/drm/tyr/file.rs   | 11 +++----
 drivers/gpu/drm/tyr/fw.rs     | 69 +++++++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/tyr/mmu.rs    | 43 +++++++++++++++++++++++++++
 drivers/gpu/drm/tyr/tyr.rs    |  2 ++
 5 files changed, 164 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/tyr/driver.rs b/drivers/gpu/drm/tyr/driver.rs
index 819f64a7573d..acf5080a5467 100644
--- a/drivers/gpu/drm/tyr/driver.rs
+++ b/drivers/gpu/drm/tyr/driver.rs
@@ -29,6 +29,7 @@
     sizes::SZ_2M,
     sync::{
         aref::ARef,
+        Arc,
         Mutex, //
     },
     time,
@@ -37,9 +38,11 @@
 
 use crate::{
     file::TyrDrmFileData,
+    fw::Firmware,
     gem::BoData,
     gpu,
     gpu::GpuInfo,
+    mmu::Mmu,
     regs::gpu_control::*, //
 };
 
@@ -49,7 +52,6 @@
 
 /// Convenience type alias for the DRM device type for this driver.
 pub(crate) type TyrDrmDevice<Ctx = drm::Registered> = drm::Device<TyrDrmDriver, Ctx>;
-
 pub(crate) struct TyrPlatformDriver;
 
 #[pin_data(PinnedDrop)]
@@ -59,21 +61,36 @@ pub(crate) struct TyrPlatformDriverData<'bound> {
 }
 
 #[pin_data]
-pub(crate) struct TyrDrmDeviceData {
+pub(crate) struct TyrDrmDeviceData;
+
+/// Resources kept alive by the DRM registration.
+#[pin_data]
+pub(crate) struct TyrDrmRegistrationData<'bound> {
+    /// Parent platform device.
     pub(crate) pdev: ARef<platform::Device>,
 
+    /// Firmware sections.
+    pub(crate) fw: Arc<Firmware<'bound>>,
+
     #[pin]
     clks: Mutex<Clocks>,
 
     #[pin]
     regulators: Mutex<Regulators>,
 
+    /// GPU MMIO register mapping.
+    pub(crate) iomem: Arc<IoMem<'bound>>,
+
     /// Some information on the GPU.
     ///
     /// This is mainly queried by userspace, i.e.: Mesa.
     pub(crate) gpu_info: GpuInfo,
 }
 
+impl ForLt for TyrDrmRegistrationData<'static> {
+    type Of<'bound> = TyrDrmRegistrationData<'bound>;
+}
+
 fn issue_soft_reset(dev: &Device, iomem: &IoMem<'_>) -> Result {
     iomem.write_reg(GPU_COMMAND::reset(ResetMode::SoftReset));
 
@@ -119,7 +136,7 @@ fn probe<'bound>(
         let sram_regulator = Regulator::<regulator::Enabled>::get(pdev.as_ref(), c"sram")?;
 
         let request = pdev.io_request_by_index(0).ok_or(ENODEV)?;
-        let iomem = request.iomap_sized::<SZ_2M>()?;
+        let iomem = Arc::new(request.iomap_sized::<SZ_2M>()?, GFP_KERNEL)?;
 
         issue_soft_reset(pdev.as_ref(), &iomem)?;
         gpu::l2_power_on(pdev.as_ref(), &iomem)?;
@@ -137,8 +154,23 @@ fn probe<'bound>(
 
         let platform: ARef<platform::Device> = pdev.into();
 
-        let data = try_pin_init!(TyrDrmDeviceData {
+        let dev_data = try_pin_init!(TyrDrmDeviceData {});
+
+        let unreg_dev = drm::UnregisteredDevice::<TyrDrmDriver>::new(pdev, dev_data)?;
+
+        let mmu = Mmu::new(iomem.as_arc_borrow(), &gpu_info)?;
+
+        let firmware = Firmware::new(
+            pdev,
+            iomem.clone(),
+            &unreg_dev,
+            mmu.as_arc_borrow(),
+            &gpu_info,
+        )?;
+
+        let reg_data = try_pin_init!(TyrDrmRegistrationData {
                 pdev: platform.clone(),
+                fw: firmware,
                 clks <- new_mutex!(Clocks {
                     core: core_clk,
                     stacks: stacks_clk,
@@ -148,11 +180,16 @@ fn probe<'bound>(
                     _mali: mali_regulator,
                     _sram: sram_regulator,
                 }),
+                iomem,
                 gpu_info,
         });
 
-        let tdev = drm::UnregisteredDevice::<TyrDrmDriver>::new(pdev, data)?;
-        let reg = drm::Registration::new(pdev.as_ref(), tdev, (), 0)?;
+        // SAFETY: `reg` is stored in the platform driver data and is not leaked or
+        // forgotten, so it is dropped before the `'bound` registration data can become
+        // invalid.
+        let reg = unsafe {
+            drm::driver::Registration::new_with_lt(pdev.as_ref(), unreg_dev, reg_data, 0)?
+        };
 
         let driver = TyrPlatformDriverData {
             _device: reg.device().into(),
@@ -184,7 +221,7 @@ fn drop(self: Pin<&mut Self>) {}
 #[vtable]
 impl drm::Driver for TyrDrmDriver {
     type Data = TyrDrmDeviceData;
-    type RegistrationData = ForLt!(());
+    type RegistrationData = TyrDrmRegistrationData<'static>;
     type File = TyrDrmFileData;
     type Object<R: drm::DeviceContext> = drm::gem::shmem::Object<BoData, R>;
     type ParentDevice<Ctx: DeviceContext> = platform::Device<Ctx>;
diff --git a/drivers/gpu/drm/tyr/file.rs b/drivers/gpu/drm/tyr/file.rs
index 9f53da7633ab..9baec67febc9 100644
--- a/drivers/gpu/drm/tyr/file.rs
+++ b/drivers/gpu/drm/tyr/file.rs
@@ -11,7 +11,8 @@
 
 use crate::driver::{
     TyrDrmDevice,
-    TyrDrmDriver, //
+    TyrDrmDriver,
+    TyrDrmRegistrationData, //
 };
 
 #[pin_data]
@@ -30,16 +31,16 @@ fn open(_dev: &drm::Device<Self::Driver>) -> Result<Pin<KBox<Self>>> {
 
 impl TyrDrmFileData {
     pub(crate) fn dev_query(
-        ddev: &TyrDrmDevice,
+        _ddev: &TyrDrmDevice,
         _pdev: &platform::Device<device::Bound>,
-        _reg_data: &(),
+        reg_data: &TyrDrmRegistrationData<'_>,
         devquery: &mut uapi::drm_panthor_dev_query,
         _file: &TyrDrmFile,
     ) -> Result<u32> {
         if devquery.pointer == 0 {
             match devquery.type_ {
                 uapi::drm_panthor_dev_query_type_DRM_PANTHOR_DEV_QUERY_GPU_INFO => {
-                    devquery.size = core::mem::size_of_val(&ddev.gpu_info) as u32;
+                    devquery.size = core::mem::size_of_val(&reg_data.gpu_info) as u32;
                     Ok(0)
                 }
                 _ => Err(EINVAL),
@@ -53,7 +54,7 @@ pub(crate) fn dev_query(
                     )
                     .writer();
 
-                    writer.write(&ddev.gpu_info)?;
+                    writer.write(&reg_data.gpu_info)?;
 
                     Ok(0)
                 }
diff --git a/drivers/gpu/drm/tyr/fw.rs b/drivers/gpu/drm/tyr/fw.rs
new file mode 100644
index 000000000000..4872e1a07c05
--- /dev/null
+++ b/drivers/gpu/drm/tyr/fw.rs
@@ -0,0 +1,69 @@
+// SPDX-License-Identifier: GPL-2.0 or MIT
+
+//! Firmware loading and management for Mali CSF GPUs.
+//!
+//! This is currently only a lifetime/resource stub. The actual firmware parser,
+//! section loading, MCU VM mapping, and boot sequence will be added later.
+#![allow(dead_code)]
+
+use kernel::{
+    device::Bound,
+    drm::Uninit,
+    platform,
+    prelude::*,
+    sync::{
+        aref::ARef,
+        Arc,
+        ArcBorrow, //
+    }, //
+};
+
+use crate::{
+    driver::{
+        IoMem,
+        TyrDrmDevice, //
+    },
+    gpu::GpuInfo,
+    mmu::Mmu, //
+};
+
+/// Firmware state for a bound Tyr device.
+///
+/// For now this only keeps the device resources alive. Later this will own the
+/// loaded firmware sections, MCU VM mappings, and boot state.
+pub(crate) struct Firmware<'bound> {
+    /// Parent platform device.
+    _pdev: ARef<platform::Device>,
+
+    /// MMIO mapping used for firmware/MCU register access.
+    _iomem: Arc<IoMem<'bound>>,
+}
+
+impl<'bound> Firmware<'bound> {
+    /// Create firmware state for this device.
+    ///
+    /// This stub only records the resources that future firmware loading code
+    /// will need.
+    pub(crate) fn new(
+        pdev: &'bound platform::Device<Bound>,
+        iomem: Arc<IoMem<'bound>>,
+        _ddev: &TyrDrmDevice<Uninit>,
+        _mmu: ArcBorrow<'_, Mmu<'bound>>,
+        _gpu_info: &GpuInfo,
+    ) -> Result<Arc<Firmware<'bound>>> {
+        Ok(Arc::new(
+            Self {
+                _pdev: pdev.into(),
+                _iomem: iomem,
+            },
+            GFP_KERNEL,
+        )?)
+    }
+
+    /// Boot the firmware.
+    ///
+    /// This is a placeholder until the MCU boot sequence is implemented.
+    pub(crate) fn boot(&self) -> Result {
+        todo!()
+    }
+}
diff --git a/drivers/gpu/drm/tyr/mmu.rs b/drivers/gpu/drm/tyr/mmu.rs
new file mode 100644
index 000000000000..7cbfbe39a290
--- /dev/null
+++ b/drivers/gpu/drm/tyr/mmu.rs
@@ -0,0 +1,43 @@
+// SPDX-License-Identifier: GPL-2.0 or MIT
+
+//! Memory Management Unit (MMU) driver for the Tyr GPU.
+//!
+//! This is currently only a lifetime/resource stub.
+//! The actual MMU initialization, page table management, and GPU VM mapping will be added later.
+
+use core::marker::PhantomData;
+
+use kernel::{
+    prelude::*,
+    sync::{
+        Arc, //
+        ArcBorrow,
+    },
+};
+
+use crate::{
+    driver::IoMem,
+    gpu::GpuInfo,
+    regs::gpu_control::AS_PRESENT, //
+};
+
+pub(super) struct Mmu<'bound> {
+    _bound: PhantomData<&'bound ()>,
+}
+
+impl<'bound> Mmu<'bound> {
+    pub(super) fn new(
+        _iomem: ArcBorrow<'_, IoMem<'bound>>,
+        gpu_info: &GpuInfo,
+    ) -> Result<Arc<Mmu<'bound>>> {
+        let present = AS_PRESENT::from_raw(gpu_info.as_present).present().get();
+        let slot_count: usize = present.count_ones().try_into()?;
+        pr_info!("MMU: {} address space slots present", slot_count);
+        Ok(Arc::new(
+            Mmu {
+                _bound: PhantomData,
+            },
+            GFP_KERNEL,
+        )?)
+    }
+}
diff --git a/drivers/gpu/drm/tyr/tyr.rs b/drivers/gpu/drm/tyr/tyr.rs
index 95cda7b0962f..d4c2a474b712 100644
--- a/drivers/gpu/drm/tyr/tyr.rs
+++ b/drivers/gpu/drm/tyr/tyr.rs
@@ -9,8 +9,10 @@
 
 mod driver;
 mod file;
+mod fw;
 mod gem;
 mod gpu;
+mod mmu;
 mod regs;
 
 kernel::module_platform_driver! {

---
base-commit: 0fb675787f89bb4a9c820fc8e9ad5ab13ea3adb7
change-id: 20260603-use_tyr_reg_data-32720e66b2dd

Best regards,
-- 
Deborah Brouwer <deborah.brouwer@collabora.com>

Claude review: drm/tyr: move probe resources into registration data

[Claude Code Review Bot]

Patch Review

**driver.rs — TyrDrmDeviceData emptied and TyrDrmRegistrationData introduced**

The core restructuring is clean. `TyrDrmDeviceData` becomes a unit struct and all resources move to the new `TyrDrmRegistrationData<'bound>`:

```rust
#[pin_data]
pub(crate) struct TyrDrmDeviceData;

#[pin_data]
pub(crate) struct TyrDrmRegistrationData<'bound> {
    pub(crate) pdev: ARef<platform::Device>,
    pub(crate) fw: Arc<Firmware<'bound>>,
    #[pin]
    clks: Mutex<Clocks>,
    #[pin]
    regulators: Mutex<Regulators>,
    pub(crate) iomem: Arc<IoMem<'bound>>,
    pub(crate) gpu_info: GpuInfo,
}
```

The `ForLt` implementation is the standard pattern for the Rust DRM lifetime framework:

```rust
impl ForLt for TyrDrmRegistrationData<'static> {
    type Of<'bound> = TyrDrmRegistrationData<'bound>;
}
```

**driver.rs — probe() restructuring**

The probe sequence now creates the DRM device first (unregistered), then creates auxiliary objects, then builds registration data and registers:

```rust
let dev_data = try_pin_init!(TyrDrmDeviceData {});
let unreg_dev = drm::UnregisteredDevice::<TyrDrmDriver>::new(pdev, dev_data)?;

let mmu = Mmu::new(iomem.as_arc_borrow(), &gpu_info)?;
let firmware = Firmware::new(pdev, iomem.clone(), &unreg_dev, mmu.as_arc_borrow(), &gpu_info)?;
```

**Concern: `Mmu` created but never stored.** The `Mmu` is constructed (which logs the address space slot count) and passed as `ArcBorrow` to `Firmware::new()`, but `Firmware` ignores it (`_mmu` parameter). The `Arc<Mmu>` is then dropped when `mmu` goes out of scope. This means the MMU object exists only transiently for its logging side effect. Consider either:
- Not creating it here at all (defer to when it's actually needed), or
- Storing it in `TyrDrmRegistrationData` if it needs to outlive probe

The current approach creates an object just to drop it, which is confusing even for a stub.

**driver.rs — Safety comment on `new_with_lt`**

```rust
// SAFETY: `reg` is stored in the platform driver data and is not leaked or
// forgotten, so it is dropped before the `'bound` registration data can become
// invalid.
```

The safety argument is correct in substance — the `Registration` is stored in `TyrPlatformDriverData` which is the platform driver data, dropped when the device unbinds. However, it would be slightly more precise to state that the `Registration` is dropped before `'bound` ends because the platform framework guarantees `TyrPlatformDriverData` is dropped during unbind, which happens while the device is still bound.

**driver.rs — IoMem wrapping change**

```rust
-        let iomem = request.iomap_sized::<SZ_2M>()?;
+        let iomem = Arc::new(request.iomap_sized::<SZ_2M>()?, GFP_KERNEL)?;
```

This changes from a direct value (or `Arc::pin_init` in the current tree) to `Arc::new`. Since `IoMem` is now shared between `TyrDrmRegistrationData` and `Firmware`, wrapping in `Arc` is correct. Verify that `IoMem` doesn't require pinning — if the base series' `IoMem<'bound>` is `Unpin`, `Arc::new` is fine; if it needs pinning, this should be `Arc::pin_init`.

**driver.rs — Cosmetic blank line removal**

```rust
 pub(crate) type TyrDrmDevice<Ctx = drm::Registered> = drm::Device<TyrDrmDriver, Ctx>;
-
 pub(crate) struct TyrPlatformDriver;
```

Nit: the blank line between unrelated declarations is conventional and aids readability. Consider keeping it.

**file.rs — dev_query uses registration data**

The changes here are a straightforward consequence of moving `gpu_info` to registration data:

```rust
-        ddev: &TyrDrmDevice,
+        _ddev: &TyrDrmDevice,
         _pdev: &platform::Device<device::Bound>,
-        _reg_data: &(),
+        reg_data: &TyrDrmRegistrationData<'_>,
```

and:

```rust
-                    devquery.size = core::mem::size_of_val(&ddev.gpu_info) as u32;
+                    devquery.size = core::mem::size_of_val(&reg_data.gpu_info) as u32;
```

This is correct. The `gpu_info` accessor properly follows the data to its new home.

**fw.rs — Firmware stub**

The stub is clean and clearly documented. A few observations:

```rust
pub(crate) struct Firmware<'bound> {
    _pdev: ARef<platform::Device>,
    _iomem: Arc<IoMem<'bound>>,
}
```

**Minor: redundant `_pdev` reference.** The `pdev` is already held by `TyrDrmRegistrationData`, and `Firmware` is owned by the registration data via `Arc<Firmware<'bound>>`. The firmware doesn't need its own `ARef<platform::Device>` to keep the device alive — the registration data already does that. Unless future firmware code needs to call device APIs directly (likely), in which case keeping it is forward-looking and fine.

```rust
pub(crate) fn boot(&self) -> Result {
    todo!()
}
```

This will panic at runtime if called. Fine for a stub, but ensure no code path can reach it before the implementation is filled in.

**mmu.rs — MMU stub**

```rust
pub(super) struct Mmu<'bound> {
    _bound: PhantomData<&'bound ()>,
}
```

The struct carries the `'bound` lifetime via `PhantomData` even though it currently holds no `'bound`-lifetime resources. This is correct forward-looking design for when MMU will hold `IoMem<'bound>` or similar.

```rust
let present = AS_PRESENT::from_raw(gpu_info.as_present).present().get();
let slot_count: usize = present.count_ones().try_into()?;
pr_info!("MMU: {} address space slots present", slot_count);
```

**Nit: `pr_info!` vs `dev_info!`.** Kernel driver messages should use `dev_info!` to include the device name for multi-device systems. The `Mmu::new()` would need to accept a device reference to do this. Consider adding `pdev: &platform::Device` or `dev: &Device` as a parameter.

**Nit: `try_into()` is infallible here.** `u32::count_ones()` returns `u32`, and `u32` → `usize` never fails on any Linux-supported architecture (usize ≥ 32 bits). Using `as usize` would be simpler and avoids an unnecessary error path, though `try_into()` is the safer Rust idiom.

**tyr.rs — module declarations**

```rust
+mod fw;
 mod gem;
 mod gpu;
+mod mmu;
```

Straightforward module additions, properly sorted alphabetically.

**Overall: the patch is well-structured and correct for its stated purpose of establishing lifetime scaffolding.** The main actionable feedback is the `Mmu` create-and-drop pattern and the `pr_info!` vs `dev_info!` issue.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/tyr: move probe resources into registration data

[Claude Code Review Bot]

Overall Series Review

Subject: drm/tyr: move probe resources into registration data
Author: Deborah Brouwer <deborah.brouwer@collabora.com>
Patches: 1
Reviewed: 2026-06-04T11:21:43.367133

---

This is a single well-structured patch that moves probe-time resources (platform device, clocks, regulators, MMIO mapping, firmware state, GPU info) out of `TyrDrmDeviceData` and into a new `TyrDrmRegistrationData<'bound>` struct tied to the DRM registration lifetime. It builds on top of Danilo Krummrich's drm-lifetime series, which introduces `Registration::new_with_lt()` and the `RegistrationData` associated type on `drm::Driver`.

The patch correctly establishes lifetime and ownership patterns for resources that firmware loading and GPU VM setup will need. The `Firmware` and `Mmu` stubs are minimal and appropriate for scaffolding future work.

**Strengths:**
- Clean separation between DRM device data (now empty) and registration-scoped resources
- Correct use of the `ForLt` higher-kinded type pattern for lifetime-parameterized registration data
- Reasonable safety argument for `new_with_lt`
- Stub modules are clearly documented as placeholders

**Concerns (minor to moderate):**
- The `Mmu` object is created, logs information, and is immediately dropped — it's not stored anywhere
- `pr_info!` in `mmu.rs` should use `dev_info!` with a device reference
- Missing `PinnedDrop` for `TyrDrmRegistrationData` to handle clock cleanup (depends on whether the base series changed `Clk` to auto-disable on drop)

---

---
Generated by Claude Code Patch Reviewer