* [PATCH v2 01/12] drm/tyr: select DRM abstractions in Kconfig
2026-03-02 23:24 [PATCH v2 0/12] drm/tyr: firmware loading and MCU boot support Deborah Brouwer
@ 2026-03-02 23:24 ` Deborah Brouwer
2026-03-03 2:48 ` Claude review: " Claude Code Review Bot
2026-03-02 23:24 ` [PATCH v2 02/12] drm/tyr: move clock cleanup into Clocks Drop impl Deborah Brouwer
` (11 subsequent siblings)
12 siblings, 1 reply; 26+ messages in thread
From: Deborah Brouwer @ 2026-03-02 23:24 UTC (permalink / raw)
To: dri-devel, rust-for-linux
Cc: daniel.almeida, aliceryhl, boris.brezillon, beata.michalska,
lyude, work, Deborah Brouwer
From: Boris Brezillon <boris.brezillon@collabora.com>
When Tyr uses GEM_SHMEM_HELPER and GPUVM, these helpers must be enabled
or the build will fail with undefined symbol errors like:
"ld.lld: error: undefined symbol: drm_gem_shmem_free"
Introduce DRM_TYR_STATIC_DEPS and have Tyr select the required
abstractions to ensure that they are enabled when Tyr is built.
Also add MMU and IOMMU dependencies that will be required to boot
the firmware.
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
Signed-off-by: Deborah Brouwer <deborah.brouwer@collabora.com>
---
No changes in v2.
drivers/gpu/drm/tyr/Kconfig | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/tyr/Kconfig b/drivers/gpu/drm/tyr/Kconfig
index e933e6478027..e5db8fa5dd57 100644
--- a/drivers/gpu/drm/tyr/Kconfig
+++ b/drivers/gpu/drm/tyr/Kconfig
@@ -1,5 +1,13 @@
# SPDX-License-Identifier: GPL-2.0 or MIT
+config DRM_TYR_STATIC_DEPS
+ bool
+ select DRM_GEM_SHMEM_HELPER
+ select DRM_GPUVM
+ help
+ Ensure required DRM infrastructure is built-in when enabling Tyr
+ even if Tyr is =m
+
config DRM_TYR
tristate "Tyr (Rust DRM support for ARM Mali CSF-based GPUs)"
depends on DRM=y
@@ -7,6 +15,10 @@ config DRM_TYR
depends on ARM || ARM64 || COMPILE_TEST
depends on !GENERIC_ATOMIC64 # for IOMMU_IO_PGTABLE_LPAE
depends on COMMON_CLK
+ depends on MMU
+ select DRM_TYR_STATIC_DEPS
+ select IOMMU_IO_PGTABLE_LPAE
+ depends on IOMMU_SUPPORT
default n
help
Rust DRM driver for ARM Mali CSF-based GPUs.
@@ -16,5 +28,5 @@ config DRM_TYR
Note that the Mali-G68 and Mali-G78, while Valhall architecture, will
be supported with the panfrost driver as they are not CSF GPUs.
- if M is selected, the module will be called tyr. This driver is work
+ If M is selected, the module will be called tyr. This driver is work
in progress and may not be functional.
--
2.52.0
^ permalink raw reply related [flat|nested] 26+ messages in thread* Claude review: drm/tyr: select DRM abstractions in Kconfig
2026-03-02 23:24 ` [PATCH v2 01/12] drm/tyr: select DRM abstractions in Kconfig Deborah Brouwer
@ 2026-03-03 2:48 ` Claude Code Review Bot
0 siblings, 0 replies; 26+ messages in thread
From: Claude Code Review Bot @ 2026-03-03 2:48 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
**Author:** Boris Brezillon
Introduces `DRM_TYR_STATIC_DEPS` to ensure `DRM_GEM_SHMEM_HELPER` and `DRM_GPUVM` are built-in even when Tyr is `=m`. Also adds `MMU`, `IOMMU_SUPPORT`, and `IOMMU_IO_PGTABLE_LPAE` dependencies.
The ordering of `depends` and `select` is slightly unusual — conventionally all `depends on` lines come before `select` lines:
```
+ depends on MMU
+ select DRM_TYR_STATIC_DEPS
+ select IOMMU_IO_PGTABLE_LPAE
+ depends on IOMMU_SUPPORT
```
Having `depends on IOMMU_SUPPORT` after the `select` lines is a style nit — it works correctly, but grouping all `depends` together improves readability. Not a blocker.
The fix for `if` -> `If` capitalization is a nice bonus cleanup.
No functional issues.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 02/12] drm/tyr: move clock cleanup into Clocks Drop impl
2026-03-02 23:24 [PATCH v2 0/12] drm/tyr: firmware loading and MCU boot support Deborah Brouwer
2026-03-02 23:24 ` [PATCH v2 01/12] drm/tyr: select DRM abstractions in Kconfig Deborah Brouwer
@ 2026-03-02 23:24 ` Deborah Brouwer
2026-03-03 2:48 ` Claude review: " Claude Code Review Bot
2026-03-02 23:24 ` [PATCH v2 03/12] drm/tyr: rename TyrObject to BoData Deborah Brouwer
` (10 subsequent siblings)
12 siblings, 1 reply; 26+ messages in thread
From: Deborah Brouwer @ 2026-03-02 23:24 UTC (permalink / raw)
To: dri-devel, rust-for-linux
Cc: daniel.almeida, aliceryhl, boris.brezillon, beata.michalska,
lyude, work, Deborah Brouwer
Currently Tyr disables its clocks from TyrDrmDeviceData::drop(), which
causes them to be shut down before any other fields in TyrDrmDeviceData
are dropped. This prevents us from using the clocks when dropping the
other fields in TyrDrmDeviceData.
In order to better control when the clocks are dropped, move this cleanup
logic into a Drop implementation on the Clocks struct itself.
Since it serves no further purpose, remove the PinnedDrop implementation
for TyrDrmDeviceData.
Also, while here, remove the #[pin_data] annotation from both the struct
Clocks and struct Regulators since neither of these structs need this
macro to create structurally pinned fields.
Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Deborah Brouwer <deborah.brouwer@collabora.com>
---
Changes in v2:
- Mention pinning changes in commit message.
- Pick up Reviewed-by tags.
drivers/gpu/drm/tyr/driver.rs | 23 +++++++++--------------
1 file changed, 9 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/tyr/driver.rs b/drivers/gpu/drm/tyr/driver.rs
index 38e5268e881a..4fff01478391 100644
--- a/drivers/gpu/drm/tyr/driver.rs
+++ b/drivers/gpu/drm/tyr/driver.rs
@@ -54,7 +54,7 @@ pub(crate) struct TyrPlatformDriverData {
_device: ARef<TyrDrmDevice>,
}
-#[pin_data(PinnedDrop)]
+#[pin_data]
pub(crate) struct TyrDrmDeviceData {
pub(crate) pdev: ARef<platform::Device>,
@@ -168,17 +168,6 @@ impl PinnedDrop for TyrPlatformDriverData {
fn drop(self: Pin<&mut Self>) {}
}
-#[pinned_drop]
-impl PinnedDrop for TyrDrmDeviceData {
- fn drop(self: Pin<&mut Self>) {
- // TODO: the type-state pattern for Clks will fix this.
- let clks = self.clks.lock();
- clks.core.disable_unprepare();
- clks.stacks.disable_unprepare();
- clks.coregroup.disable_unprepare();
- }
-}
-
// We need to retain the name "panthor" to achieve drop-in compatibility with
// the C driver in the userspace stack.
const INFO: drm::DriverInfo = drm::DriverInfo {
@@ -202,14 +191,20 @@ impl drm::Driver for TyrDrmDriver {
}
}
-#[pin_data]
struct Clocks {
core: Clk,
stacks: OptionalClk,
coregroup: OptionalClk,
}
-#[pin_data]
+impl Drop for Clocks {
+ fn drop(&mut self) {
+ self.core.disable_unprepare();
+ self.stacks.disable_unprepare();
+ self.coregroup.disable_unprepare();
+ }
+}
+
struct Regulators {
_mali: Regulator<regulator::Enabled>,
_sram: Regulator<regulator::Enabled>,
--
2.52.0
^ permalink raw reply related [flat|nested] 26+ messages in thread* Claude review: drm/tyr: move clock cleanup into Clocks Drop impl
2026-03-02 23:24 ` [PATCH v2 02/12] drm/tyr: move clock cleanup into Clocks Drop impl Deborah Brouwer
@ 2026-03-03 2:48 ` Claude Code Review Bot
0 siblings, 0 replies; 26+ messages in thread
From: Claude Code Review Bot @ 2026-03-03 2:48 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
**Author:** Deborah Brouwer
Moves clock disable/unprepare from `TyrDrmDeviceData::PinnedDrop` into a `Drop` impl on `Clocks` itself. This ensures clocks are disabled last (after other fields that may depend on them are dropped), following Rust's field drop order (fields drop in declaration order).
```rust
+impl Drop for Clocks {
+ fn drop(&mut self) {
+ self.core.disable_unprepare();
+ self.stacks.disable_unprepare();
+ self.coregroup.disable_unprepare();
+ }
+}
```
This is correct because `Clocks` is declared after other fields in `TyrDrmDeviceData`, so it drops last. Also removes `#[pin_data]` from `Clocks` and `Regulators` since they don't need structural pinning.
Clean and well-motivated change.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 03/12] drm/tyr: rename TyrObject to BoData
2026-03-02 23:24 [PATCH v2 0/12] drm/tyr: firmware loading and MCU boot support Deborah Brouwer
2026-03-02 23:24 ` [PATCH v2 01/12] drm/tyr: select DRM abstractions in Kconfig Deborah Brouwer
2026-03-02 23:24 ` [PATCH v2 02/12] drm/tyr: move clock cleanup into Clocks Drop impl Deborah Brouwer
@ 2026-03-02 23:24 ` Deborah Brouwer
2026-03-03 2:48 ` Claude review: " Claude Code Review Bot
2026-03-02 23:24 ` [PATCH v2 04/12] drm/tyr: set DMA mask using GPU physical address Deborah Brouwer
` (9 subsequent siblings)
12 siblings, 1 reply; 26+ messages in thread
From: Deborah Brouwer @ 2026-03-02 23:24 UTC (permalink / raw)
To: dri-devel, rust-for-linux
Cc: daniel.almeida, aliceryhl, boris.brezillon, beata.michalska,
lyude, work, Deborah Brouwer
From: Boris Brezillon <boris.brezillon@collabora.com>
Currently the GEM inner driver data object is called `TyrObject` which
is a fairly generic name. To make the code easier to understand,
rename `TyrObject` to `BoData` so that the name better reflects its
role.
No functional change is intended.
Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
Co-developed-by: Deborah Brouwer <deborah.brouwer@collabora.com>
Signed-off-by: Deborah Brouwer <deborah.brouwer@collabora.com>
---
Changes in v2:
- Pick up Reviewed-by tags.
drivers/gpu/drm/tyr/driver.rs | 4 ++--
drivers/gpu/drm/tyr/gem.rs | 6 +++---
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/tyr/driver.rs b/drivers/gpu/drm/tyr/driver.rs
index 4fff01478391..eccf36d601ac 100644
--- a/drivers/gpu/drm/tyr/driver.rs
+++ b/drivers/gpu/drm/tyr/driver.rs
@@ -36,7 +36,7 @@
use crate::{
file::TyrDrmFileData,
- gem::TyrObject,
+ gem::BoData,
gpu,
gpu::GpuInfo,
regs, //
@@ -182,7 +182,7 @@ fn drop(self: Pin<&mut Self>) {}
impl drm::Driver for TyrDrmDriver {
type Data = TyrDrmDeviceData;
type File = TyrDrmFileData;
- type Object<R: drm::DeviceContext> = drm::gem::Object<TyrObject, R>;
+ type Object<R: drm::DeviceContext> = drm::gem::Object<BoData, R>;
const INFO: drm::DriverInfo = INFO;
diff --git a/drivers/gpu/drm/tyr/gem.rs b/drivers/gpu/drm/tyr/gem.rs
index 0c488984d2ec..87f3890e4be6 100644
--- a/drivers/gpu/drm/tyr/gem.rs
+++ b/drivers/gpu/drm/tyr/gem.rs
@@ -15,9 +15,9 @@
/// GEM Object inner driver data
#[pin_data]
-pub(crate) struct TyrObject {}
+pub(crate) struct BoData {}
-impl gem::DriverObject for TyrObject {
+impl gem::DriverObject for BoData {
type Driver = TyrDrmDriver;
type Args = ();
@@ -26,6 +26,6 @@ fn new<Ctx: DeviceContext>(
_size: usize,
_args: (),
) -> impl PinInit<Self, Error> {
- try_pin_init!(TyrObject {})
+ try_pin_init!(BoData {})
}
}
--
2.52.0
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v2 04/12] drm/tyr: set DMA mask using GPU physical address
2026-03-02 23:24 [PATCH v2 0/12] drm/tyr: firmware loading and MCU boot support Deborah Brouwer
` (2 preceding siblings ...)
2026-03-02 23:24 ` [PATCH v2 03/12] drm/tyr: rename TyrObject to BoData Deborah Brouwer
@ 2026-03-02 23:24 ` Deborah Brouwer
2026-03-03 2:48 ` Claude review: " Claude Code Review Bot
2026-03-02 23:24 ` [PATCH v2 05/12] drm/tyr: add MMU address space registers Deborah Brouwer
` (8 subsequent siblings)
12 siblings, 1 reply; 26+ messages in thread
From: Deborah Brouwer @ 2026-03-02 23:24 UTC (permalink / raw)
To: dri-devel, rust-for-linux
Cc: daniel.almeida, aliceryhl, boris.brezillon, beata.michalska,
lyude, work, Deborah Brouwer
From: Beata Michalska <beata.michalska@arm.com>
Configure the device DMA mask during probe using the GPU's physical
address capability reported in GpuInfo. This ensures DMA allocations
use an appropriate address mask.
Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Beata Michalska <beata.michalska@arm.com>
Co-developed-by: Deborah Brouwer <deborah.brouwer@collabora.com>
Signed-off-by: Deborah Brouwer <deborah.brouwer@collabora.com>
---
Changes in v2:
- Move the semi-colon outside of unsafe block and reformat.
- Pick up Reviewed-by tags.
drivers/gpu/drm/tyr/driver.rs | 9 +++++++++
drivers/gpu/drm/tyr/gpu.rs | 1 -
2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/tyr/driver.rs b/drivers/gpu/drm/tyr/driver.rs
index eccf36d601ac..781502c6db36 100644
--- a/drivers/gpu/drm/tyr/driver.rs
+++ b/drivers/gpu/drm/tyr/driver.rs
@@ -11,6 +11,10 @@
Device, //
},
devres::Devres,
+ dma::{
+ Device as DmaDevice,
+ DmaMask, //
+ },
drm,
drm::{
driver::Registration,
@@ -134,6 +138,11 @@ fn probe(
let gpu_info = GpuInfo::new(pdev.as_ref(), &iomem)?;
gpu_info.log(pdev);
+ // SAFETY: No concurrent DMA allocations or mappings can be made because
+ // the device is still being probed and therefore isn't being used by
+ // other threads of execution.
+ unsafe { pdev.dma_set_mask_and_coherent(DmaMask::try_new(gpu_info.pa_bits())?)? };
+
let uninit_ddev = UnregisteredDevice::<TyrDrmDriver>::new(pdev.as_ref())?;
let platform: ARef<platform::Device> = pdev.into();
diff --git a/drivers/gpu/drm/tyr/gpu.rs b/drivers/gpu/drm/tyr/gpu.rs
index a88775160f98..5d9637899c7d 100644
--- a/drivers/gpu/drm/tyr/gpu.rs
+++ b/drivers/gpu/drm/tyr/gpu.rs
@@ -139,7 +139,6 @@ pub(crate) fn va_bits(&self) -> u32 {
}
/// Returns the number of physical address bits supported by the GPU.
- #[expect(dead_code)]
pub(crate) fn pa_bits(&self) -> u32 {
(self.mmu_features >> 8) & genmask_u32(0..=7)
}
--
2.52.0
^ permalink raw reply related [flat|nested] 26+ messages in thread* Claude review: drm/tyr: set DMA mask using GPU physical address
2026-03-02 23:24 ` [PATCH v2 04/12] drm/tyr: set DMA mask using GPU physical address Deborah Brouwer
@ 2026-03-03 2:48 ` Claude Code Review Bot
0 siblings, 0 replies; 26+ messages in thread
From: Claude Code Review Bot @ 2026-03-03 2:48 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
**Author:** Beata Michalska
Sets the DMA mask during probe based on `gpu_info.pa_bits()`.
```rust
+ // SAFETY: No concurrent DMA allocations or mappings can be made because
+ // the device is still being probed and therefore isn't being used by
+ // other threads of execution.
+ unsafe { pdev.dma_set_mask_and_coherent(DmaMask::try_new(gpu_info.pa_bits())?)? };
```
The safety comment is appropriate for the probe-time context. Also removes `#[expect(dead_code)]` from `pa_bits()` since it's now used.
No issues.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 05/12] drm/tyr: add MMU address space registers
2026-03-02 23:24 [PATCH v2 0/12] drm/tyr: firmware loading and MCU boot support Deborah Brouwer
` (3 preceding siblings ...)
2026-03-02 23:24 ` [PATCH v2 04/12] drm/tyr: set DMA mask using GPU physical address Deborah Brouwer
@ 2026-03-02 23:24 ` Deborah Brouwer
2026-03-03 2:48 ` Claude review: " Claude Code Review Bot
2026-03-02 23:24 ` [PATCH v2 06/12] drm/tyr: add shmem backing for GEM objects Deborah Brouwer
` (7 subsequent siblings)
12 siblings, 1 reply; 26+ messages in thread
From: Deborah Brouwer @ 2026-03-02 23:24 UTC (permalink / raw)
To: dri-devel, rust-for-linux
Cc: daniel.almeida, aliceryhl, boris.brezillon, beata.michalska,
lyude, work, Deborah Brouwer
From: Daniel Almeida <daniel.almeida@collabora.com>
Add register definitions and constants for managing MMU address space,
including:
- Address space translation configuration (page table format, attributes)
- Memory attributes (cacheability, shareability)
- Address space commands (update, lock, flush)
- AsRegister helper for per-AS register access
These will be used by the MMU/VM manager to configure page tables and
control address space operations.
Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
Co-developed-by: Deborah Brouwer <deborah.brouwer@collabora.com>
Signed-off-by: Deborah Brouwer <deborah.brouwer@collabora.com>
---
Changes in v2:
- Change the author from Boris Brezillon to Daniel Almeida.
- Reduce MAX_AS_REGISTERS from 32 to 16 as stated in specs.
drivers/gpu/drm/tyr/regs.rs | 101 +++++++++++++++++++++++++++++++++++-
1 file changed, 100 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/tyr/regs.rs b/drivers/gpu/drm/tyr/regs.rs
index 611870c2e6af..489895d47c42 100644
--- a/drivers/gpu/drm/tyr/regs.rs
+++ b/drivers/gpu/drm/tyr/regs.rs
@@ -8,7 +8,10 @@
#![allow(dead_code)]
use kernel::{
- bits::bit_u32,
+ bits::{
+ bit_u32,
+ bit_u64, //
+ },
device::{
Bound,
Device, //
@@ -111,3 +114,99 @@ pub(crate) fn write(&self, dev: &Device<Bound>, iomem: &Devres<IoMem>, value: u3
pub(crate) const MMU_IRQ_CLEAR: Register<0x2004> = Register;
pub(crate) const MMU_IRQ_MASK: Register<0x2008> = Register;
pub(crate) const MMU_IRQ_STAT: Register<0x200c> = Register;
+
+pub(crate) const AS_TRANSCFG_ADRMODE_UNMAPPED: u64 = bit_u64(0);
+pub(crate) const AS_TRANSCFG_ADRMODE_AARCH64_4K: u64 = bit_u64(2) | bit_u64(1);
+pub(crate) const AS_TRANSCFG_PTW_MEMATTR_WB: u64 = bit_u64(25);
+pub(crate) const AS_TRANSCFG_PTW_RA: u64 = bit_u64(30);
+
+pub(crate) const fn as_transcfg_ina_bits(x: u64) -> u64 {
+ x << 6
+}
+
+pub(crate) const AS_MEMATTR_AARCH64_SH_MIDGARD_INNER: u32 = 0 << 4;
+pub(crate) const AS_MEMATTR_AARCH64_INNER_OUTER_NC: u32 = 1 << 6;
+pub(crate) const AS_MEMATTR_AARCH64_INNER_OUTER_WB: u32 = 2 << 6;
+
+pub(crate) fn as_memattr_aarch64_inner_alloc_expl(w: bool, r: bool) -> u32 {
+ (3 << 2) | (u32::from(w)) | ((u32::from(r)) << 1)
+}
+
+pub(crate) const AS_COMMAND_UPDATE: u32 = 1;
+pub(crate) const AS_COMMAND_LOCK: u32 = 2;
+pub(crate) const AS_COMMAND_FLUSH_PT: u32 = 4;
+pub(crate) const AS_COMMAND_FLUSH_MEM: u32 = 5;
+
+pub(crate) const AS_STATUS_ACTIVE: u32 = bit_u32(0);
+
+pub(crate) const AS_LOCK_REGION_MIN_SIZE: u32 = bit_u32(15);
+
+/// Maximum number of hardware address space slots.
+/// The actual number of slots available is usually lower.
+pub(crate) const MAX_AS_REGISTERS: usize = 16;
+
+const MMU_BASE: usize = 0x2400;
+const MMU_AS_SHIFT: usize = 6;
+
+const fn mmu_as(as_nr: usize) -> usize {
+ MMU_BASE + (as_nr << MMU_AS_SHIFT)
+}
+
+pub(crate) struct AsRegister(usize);
+
+impl AsRegister {
+ fn new(as_nr: usize, offset: usize) -> Result<Self> {
+ Ok(AsRegister(mmu_as(as_nr) + offset))
+ }
+
+ #[inline]
+ pub(crate) fn read(&self, dev: &Device<Bound>, iomem: &Devres<IoMem>) -> Result<u32> {
+ let value = (*iomem).access(dev)?.try_read32(self.0)?;
+ Ok(value)
+ }
+
+ #[inline]
+ pub(crate) fn write(&self, dev: &Device<Bound>, iomem: &Devres<IoMem>, value: u32) -> Result {
+ (*iomem).access(dev)?.try_write32(value, self.0)?;
+ Ok(())
+ }
+}
+
+pub(crate) fn as_transtab_lo(as_nr: usize) -> Result<AsRegister> {
+ AsRegister::new(as_nr, 0x0)
+}
+
+pub(crate) fn as_transtab_hi(as_nr: usize) -> Result<AsRegister> {
+ AsRegister::new(as_nr, 0x4)
+}
+
+pub(crate) fn as_memattr_lo(as_nr: usize) -> Result<AsRegister> {
+ AsRegister::new(as_nr, 0x8)
+}
+
+pub(crate) fn as_memattr_hi(as_nr: usize) -> Result<AsRegister> {
+ AsRegister::new(as_nr, 0xc)
+}
+
+pub(crate) fn as_lockaddr_lo(as_nr: usize) -> Result<AsRegister> {
+ AsRegister::new(as_nr, 0x10)
+}
+
+pub(crate) fn as_lockaddr_hi(as_nr: usize) -> Result<AsRegister> {
+ AsRegister::new(as_nr, 0x14)
+}
+
+pub(crate) fn as_command(as_nr: usize) -> Result<AsRegister> {
+ AsRegister::new(as_nr, 0x18)
+}
+
+pub(crate) fn as_status(as_nr: usize) -> Result<AsRegister> {
+ AsRegister::new(as_nr, 0x28)
+}
+
+pub(crate) fn as_transcfg_lo(as_nr: usize) -> Result<AsRegister> {
+ AsRegister::new(as_nr, 0x30)
+}
+pub(crate) fn as_transcfg_hi(as_nr: usize) -> Result<AsRegister> {
+ AsRegister::new(as_nr, 0x34)
+}
--
2.52.0
^ permalink raw reply related [flat|nested] 26+ messages in thread* Claude review: drm/tyr: add MMU address space registers
2026-03-02 23:24 ` [PATCH v2 05/12] drm/tyr: add MMU address space registers Deborah Brouwer
@ 2026-03-03 2:48 ` Claude Code Review Bot
0 siblings, 0 replies; 26+ messages in thread
From: Claude Code Review Bot @ 2026-03-03 2:48 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
**Author:** Daniel Almeida
Adds register definitions and an `AsRegister` helper for per-address-space register access.
One concern: `AsRegister::new()` does not validate `as_nr`:
```rust
+impl AsRegister {
+ fn new(as_nr: usize, offset: usize) -> Result<Self> {
+ Ok(AsRegister(mmu_as(as_nr) + offset))
+ }
```
It returns `Result` but never returns an error. If `as_nr >= MAX_AS_REGISTERS`, the computed offset will be out of the expected MMIO range. While callers in the address_space module do call `validate_as_slot()` before using these registers, the validation is in `AsRegister::new` is misleading since it always succeeds. Consider either adding a bounds check here or changing the return type to plain `AsRegister` (not `Result`). Given the `try_read32`/`try_write32` calls in read/write will catch truly out-of-bounds accesses at the IoMem level, this is a defense-in-depth concern rather than a functional bug.
Minor nit: `AS_MEMATTR_AARCH64_SH_MIDGARD_INNER` is `0 << 4` which is just `0`. A comment noting this is intentional (the zero value selects "midgard inner" shareability) would improve readability.
The change from `MAX_AS_REGISTERS = 32` to `16` matches the Mali CSF spec — good fix.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 06/12] drm/tyr: add shmem backing for GEM objects
2026-03-02 23:24 [PATCH v2 0/12] drm/tyr: firmware loading and MCU boot support Deborah Brouwer
` (4 preceding siblings ...)
2026-03-02 23:24 ` [PATCH v2 05/12] drm/tyr: add MMU address space registers Deborah Brouwer
@ 2026-03-02 23:24 ` Deborah Brouwer
2026-03-03 2:48 ` Claude review: " Claude Code Review Bot
2026-03-02 23:24 ` [PATCH v2 07/12] drm/tyr: Add generic slot manager Deborah Brouwer
` (6 subsequent siblings)
12 siblings, 1 reply; 26+ messages in thread
From: Deborah Brouwer @ 2026-03-02 23:24 UTC (permalink / raw)
To: dri-devel, rust-for-linux
Cc: daniel.almeida, aliceryhl, boris.brezillon, beata.michalska,
lyude, work, Deborah Brouwer
Add support for GEM buffer objects backed by shared memory.
This introduces the BoCreateArgs structure for passing creation parameters
including flags, and adds a flags field to BoData.
Co-developed-by: Boris Brezillon <boris.brezillon@collabora.com>
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
Signed-off-by: Deborah Brouwer <deborah.brouwer@collabora.com>
---
Changes in v2:
- Remove new_dummy_object() to add it later in vm patch.
- Remove unrelated change that used device alias.
drivers/gpu/drm/tyr/gem.rs | 22 +++++++++++++++++-----
1 file changed, 17 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/tyr/gem.rs b/drivers/gpu/drm/tyr/gem.rs
index 87f3890e4be6..c6d4d6f9bae3 100644
--- a/drivers/gpu/drm/tyr/gem.rs
+++ b/drivers/gpu/drm/tyr/gem.rs
@@ -1,4 +1,8 @@
// SPDX-License-Identifier: GPL-2.0 or MIT
+//! GEM buffer object management for the Tyr driver.
+//!
+//! This module provides buffer object (BO) management functionality using
+//! DRM's GEM subsystem with shmem backing.
use kernel::{
drm::{
@@ -13,19 +17,27 @@
TyrDrmDriver, //
};
-/// GEM Object inner driver data
+/// Tyr's DriverObject type for GEM objects.
#[pin_data]
-pub(crate) struct BoData {}
+pub(crate) struct BoData {
+ flags: u32,
+}
+
+/// Provides a way to pass arguments when creating BoData
+/// as required by the gem::DriverObject trait.
+pub(crate) struct BoCreateArgs {
+ flags: u32,
+}
impl gem::DriverObject for BoData {
type Driver = TyrDrmDriver;
- type Args = ();
+ type Args = BoCreateArgs;
fn new<Ctx: DeviceContext>(
_dev: &TyrDrmDevice<Ctx>,
_size: usize,
- _args: (),
+ args: BoCreateArgs,
) -> impl PinInit<Self, Error> {
- try_pin_init!(BoData {})
+ try_pin_init!(Self { flags: args.flags })
}
}
--
2.52.0
^ permalink raw reply related [flat|nested] 26+ messages in thread* Claude review: drm/tyr: add shmem backing for GEM objects
2026-03-02 23:24 ` [PATCH v2 06/12] drm/tyr: add shmem backing for GEM objects Deborah Brouwer
@ 2026-03-03 2:48 ` Claude Code Review Bot
0 siblings, 0 replies; 26+ messages in thread
From: Claude Code Review Bot @ 2026-03-03 2:48 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
**Author:** Deborah Brouwer / Boris Brezillon
Adds `BoCreateArgs` with a `flags` field and updates `BoData` accordingly. Currently `flags` is stored but not used beyond creation.
```rust
+pub(crate) struct BoCreateArgs {
+ flags: u32,
+}
```
The `flags` field is `pub(crate)` on the struct but the field itself is private (no `pub`). This means only code within `gem.rs` can construct a `BoCreateArgs`. Since `KernelBo::new()` (added later in patch 10) constructs it with `flags: 0`, and the firmware loader also uses `flags: 0`, this is fine for now but will need adjustment when userspace BO creation is added. The `flags` field on `BoData` is also unused currently — acceptable for scaffolding.
No blocking issues.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 07/12] drm/tyr: Add generic slot manager
2026-03-02 23:24 [PATCH v2 0/12] drm/tyr: firmware loading and MCU boot support Deborah Brouwer
` (5 preceding siblings ...)
2026-03-02 23:24 ` [PATCH v2 06/12] drm/tyr: add shmem backing for GEM objects Deborah Brouwer
@ 2026-03-02 23:24 ` Deborah Brouwer
2026-03-03 2:48 ` Claude review: " Claude Code Review Bot
2026-03-02 23:24 ` [PATCH v2 08/12] drm/tyr: add MMU module Deborah Brouwer
` (5 subsequent siblings)
12 siblings, 1 reply; 26+ messages in thread
From: Deborah Brouwer @ 2026-03-02 23:24 UTC (permalink / raw)
To: dri-devel, rust-for-linux
Cc: daniel.almeida, aliceryhl, boris.brezillon, beata.michalska,
lyude, work, Deborah Brouwer
From: Boris Brezillon <boris.brezillon@collabora.com>
Introduce a generic slot manager to dynamically allocate limited hardware
slots to software "seats". It can be used for both address space (AS) and
command stream group (CSG) slots.
The slot manager initially assigns seats to its free slots. It then
continues to reuse the same slot for a seat, as long as another seat
did not start to use the slot in the interim.
When contention arises because all of the slots are allocated, the slot
manager will lazily evict and reuse slots that have become idle (if any).
The seat state is protected using the LockedBy pattern with the same lock
that guards the SlotManager. This ensures the seat state stays consistent
across slot operations.
Hardware specific behaviour can be customized using the slot manager's
`SlotOperations` trait.
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
Co-developed-by: Deborah Brouwer <deborah.brouwer@collabora.com>
Signed-off-by: Deborah Brouwer <deborah.brouwer@collabora.com>
---
Changes in v2:
- Add documentation.
- Remove impl Unpin for SlotManager.
- Simplify check_seat() to avoid manual copy.
drivers/gpu/drm/tyr/slot.rs | 437 ++++++++++++++++++++++++++++++++++++
drivers/gpu/drm/tyr/tyr.rs | 1 +
2 files changed, 438 insertions(+)
create mode 100644 drivers/gpu/drm/tyr/slot.rs
diff --git a/drivers/gpu/drm/tyr/slot.rs b/drivers/gpu/drm/tyr/slot.rs
new file mode 100644
index 000000000000..debba75f6204
--- /dev/null
+++ b/drivers/gpu/drm/tyr/slot.rs
@@ -0,0 +1,437 @@
+// SPDX-License-Identifier: GPL-2.0 or MIT
+
+//! Slot management abstraction for limited hardware resources.
+//!
+//! This module provides a generic [`SlotManager`] that assigns limited hardware
+//! slots to logical "seats". A seat represents an entity (such as a virtual memory
+//! (VM) address space) that needs access to a hardware slot.
+//!
+//! The [`SlotManager`] tracks slot allocation using sequence numbers (seqno) to detect
+//! when a seat's binding has been invalidated. When a seat requests activation,
+//! the manager will either reuse the seat's existing slot (if still valid),
+//! allocate a free slot (if any are available), or evict the oldest idle slot if any
+//! slots are idle.
+//!
+//! Hardware-specific behavior is customized by implementing the [`SlotOperations`]
+//! trait, which allows callbacks when slots are activated or evicted.
+//!
+//! This is currently used for managing address space slots in the GPU, and it will
+//! also be used to manage Command Stream Group (CSG) interface slots in the future.
+//!
+//! [SlotOperations]: crate::slot::SlotOperations
+//! [SlotManager]: crate::slot::SlotManager
+#![allow(dead_code)]
+
+use core::{
+ mem::take,
+ ops::{
+ Deref,
+ DerefMut, //
+ }, //
+};
+
+use kernel::{
+ prelude::*,
+ sync::LockedBy, //
+};
+
+/// Seat information.
+///
+/// This can't be accessed directly by the element embedding a `Seat`,
+/// but is used by the generic slot manager logic to control residency
+/// of a certain object on a hardware slot.
+pub(crate) struct SeatInfo {
+ /// Slot used by this seat.
+ ///
+ /// This index is only valid if the slot pointed to by this index
+ /// has its `SlotInfo::seqno` match `SeatInfo::seqno`. Otherwise,
+ /// it means the object has been evicted from the hardware slot,
+ /// and a new slot needs to be acquired to make this object
+ /// resident again.
+ slot: u8,
+
+ /// Sequence number encoding the last time this seat was active.
+ /// We also use it to check if a slot is still bound to a seat.
+ seqno: u64,
+}
+
+/// Seat state.
+///
+/// This is meant to be embedded in the object that wants to acquire
+/// hardware slots. It also starts in the `Seat::NoSeat` state, and
+/// the slot manager will change the object value when an active/evict
+/// request is issued.
+#[derive(Default)]
+pub(crate) enum Seat {
+ #[expect(clippy::enum_variant_names)]
+ /// Resource is not resident.
+ ///
+ /// All objects start with a seat in the `Seat::NoSeat` state. The seat also
+ /// gets back to that state if the user requests eviction. It
+ /// can also end up in that state next time an operation is done
+ /// on a `Seat::Idle` seat and the slot manager finds out this
+ /// object has been evicted from the slot.
+ #[default]
+ NoSeat,
+
+ /// Resource is actively used and resident.
+ ///
+ /// When a seat is in the `Seat::Active` state, it can't be evicted, and the
+ /// slot pointed to by `SeatInfo::slot` is guaranteed to be reserved
+ /// for this object as long as the seat stays active.
+ Active(SeatInfo),
+
+ /// Resource is idle and might or might not be resident.
+ ///
+ /// When a seat is in the`Seat::Idle` state, we can't know for sure if the
+ /// object is resident or evicted until the next request we issue
+ /// to the slot manager. This tells the slot manager it can
+ /// reclaim the underlying slot if needed.
+ /// In order for the hardware to use this object again, the seat
+ /// needs to be turned into an `Seat::Active` state again
+ /// with a `SlotManager::activate()` call.
+ Idle(SeatInfo),
+}
+
+impl Seat {
+ /// Get the slot index this seat is pointing to.
+ ///
+ /// If the seat is not `Seat::Active` we can't trust the
+ /// `SeatInfo`. In that case `None` is returned, otherwise
+ /// `Some(SeatInfo::slot)` is returned.
+ pub(super) fn slot(&self) -> Option<u8> {
+ match self {
+ Self::Active(info) => Some(info.slot),
+ _ => None,
+ }
+ }
+}
+
+/// Trait describing the slot-related operations.
+pub(crate) trait SlotOperations {
+ /// Implementation-specific data associated with each slot.
+ type SlotData;
+
+ /// Called when a slot is being activated for a seat.
+ ///
+ /// This callback allows hardware-specific actions to be performed when a slot
+ /// becomes active, such as updating hardware registers or invalidating caches.
+ fn activate(&mut self, _slot_idx: usize, _slot_data: &Self::SlotData) -> Result {
+ Ok(())
+ }
+
+ /// Called when a slot is being evicted and freed.
+ ///
+ /// This callback allows hardware-specific cleanup when a slot is being
+ /// completely freed, either explicitly or when an idle slot is being
+ /// reused for a different seat. Any hardware state should be invalidated.
+ fn evict(&mut self, _slot_idx: usize, _slot_data: &Self::SlotData) -> Result {
+ Ok(())
+ }
+}
+
+/// Data attached to a slot.
+///
+/// Contains data and the sequence number used to check
+/// whether a seat's binding to this slot is still valid.
+struct SlotInfo<T> {
+ /// Type specific data attached to a slot
+ slot_data: T,
+
+ /// Sequence number from when this slot was last activated
+ seqno: u64,
+}
+
+/// Slot state.
+///
+/// Tracks whether a hardware slot is free, actively in use, or idle and available
+/// for eviction.
+#[derive(Default)]
+enum Slot<T> {
+ /// Slot is free.
+ ///
+ /// All slots start in the `Slot::Free` state when the slot manager is created.
+ #[default]
+ Free,
+
+ /// Slot is active.
+ ///
+ /// When in the `Slot::Active` state, the slot is guaranteed to stay active
+ /// for as long as the resource bound to it has its seat in the
+ /// `Seat::Active` state. No new resource can be bound to it.
+ Active(SlotInfo<T>),
+
+ /// Slot is idle.
+ ///
+ /// Happens when the underlying resource has been flagged
+ /// `Seat::Idle`. When in the `Slot::Idle` state, the slot manager is allowed
+ /// to evict the resource and re-assign the slot to someone else.
+ /// This process involves updating the `SlotInfo::seqno` which
+ /// will be checked against the `SeatInfo::seqno` in case the idle
+ /// resource wants to become active again.
+ Idle(SlotInfo<T>),
+}
+
+/// Generic slot manager object.
+///
+/// It abstracts away all the churn around activeness/idleness tracking
+/// and lets the implementer of the SlotOperations trait focus on how to
+/// make a resource active or evict it.
+///
+/// This structure must be protected by a lock.
+/// Seats that want to use this manager must be wrapped with
+/// `LockedBy<Seat, SlotManager<T, MAX_SLOTS>>` to ensure they are protected by the same lock.
+/// All operations on seats and slots are synchronized through this shared lock.
+pub(crate) struct SlotManager<T: SlotOperations, const MAX_SLOTS: usize> {
+ /// Manager specific data
+ manager: T,
+
+ /// Number of slots actually available
+ slot_count: usize,
+
+ /// Slots
+ slots: [Slot<T::SlotData>; MAX_SLOTS],
+
+ /// Sequence number incremented each time a Seat is successfully activated
+ use_seqno: u64,
+}
+
+/// A `Seat` protected by the same lock that is used to wrap the `SlotManager`.
+type LockedSeat<T, const MAX_SLOTS: usize> = LockedBy<Seat, SlotManager<T, MAX_SLOTS>>;
+
+impl<T: SlotOperations, const MAX_SLOTS: usize> SlotManager<T, MAX_SLOTS> {
+ /// Creates a new slot manager.
+ ///
+ /// Returns [`EINVAL`] if the slot count is zero or exceeds the maximum number of slots.
+ pub(crate) fn new(manager: T, slot_count: usize) -> Result<Self> {
+ if slot_count == 0 {
+ return Err(EINVAL);
+ }
+ if slot_count > MAX_SLOTS {
+ return Err(EINVAL);
+ }
+ Ok(Self {
+ manager,
+ slot_count,
+ slots: [const { Slot::Free }; MAX_SLOTS],
+ use_seqno: 1,
+ })
+ }
+
+ /// Records a slot as active for the given seat.
+ ///
+ /// Updates both the seat state and the slot state to reflect the active binding,
+ /// using the current sequence number. Increments the sequence number for the next
+ /// activation.
+ fn record_active_slot(
+ &mut self,
+ slot_idx: usize,
+ locked_seat: &LockedSeat<T, MAX_SLOTS>,
+ slot_data: T::SlotData,
+ ) -> Result {
+ let cur_seqno = self.use_seqno;
+
+ *locked_seat.access_mut(self) = Seat::Active(SeatInfo {
+ slot: slot_idx as u8,
+ seqno: cur_seqno,
+ });
+
+ self.slots[slot_idx] = Slot::Active(SlotInfo {
+ slot_data,
+ seqno: cur_seqno,
+ });
+
+ self.use_seqno += 1;
+ Ok(())
+ }
+
+ /// Activates a slot for the given seat.
+ ///
+ /// Calls the activation callback and then records the slot as active.
+ fn activate_slot(
+ &mut self,
+ slot_idx: usize,
+ locked_seat: &LockedSeat<T, MAX_SLOTS>,
+ slot_data: T::SlotData,
+ ) -> Result {
+ self.manager.activate(slot_idx, &slot_data)?;
+ self.record_active_slot(slot_idx, locked_seat, slot_data)
+ }
+
+ /// Allocates a slot for the given seat.
+ ///
+ /// Searches for a free slot first. If none are available, finds the oldest idle
+ /// slot (by sequence number) and evicts it. Returns [`EBUSY`] if all slots are
+ /// active and none can be evicted.
+ fn allocate_slot(
+ &mut self,
+ locked_seat: &LockedSeat<T, MAX_SLOTS>,
+ slot_data: T::SlotData,
+ ) -> Result {
+ let slots = &self.slots[..self.slot_count];
+
+ let mut idle_slot_idx = None;
+ let mut idle_slot_seqno: u64 = 0;
+
+ for (slot_idx, slot) in slots.iter().enumerate() {
+ match slot {
+ Slot::Free => {
+ return self.activate_slot(slot_idx, locked_seat, slot_data);
+ }
+ Slot::Idle(slot_info) => {
+ if idle_slot_idx.is_none() || slot_info.seqno < idle_slot_seqno {
+ idle_slot_idx = Some(slot_idx);
+ idle_slot_seqno = slot_info.seqno;
+ }
+ }
+ Slot::Active(_) => (),
+ }
+ }
+
+ match idle_slot_idx {
+ Some(slot_idx) => {
+ // Lazily evict idle slot just before it is reused
+ if let Slot::Idle(slot_info) = &self.slots[slot_idx] {
+ self.manager.evict(slot_idx, &slot_info.slot_data)?;
+ }
+ self.activate_slot(slot_idx, locked_seat, slot_data)
+ }
+ None => {
+ pr_err!(
+ "Slot allocation failed: all {} slots in use\n",
+ self.slot_count
+ );
+ Err(EBUSY)
+ }
+ }
+ }
+
+ /// Transitions a slot from active to idle state.
+ ///
+ /// Updates both the slot and seat to idle state, making the slot eligible for
+ /// eviction if needed by another seat.
+ fn idle_slot(&mut self, slot_idx: usize, locked_seat: &LockedSeat<T, MAX_SLOTS>) -> Result {
+ let slot = take(&mut self.slots[slot_idx]);
+
+ if let Slot::Active(slot_info) = slot {
+ self.slots[slot_idx] = Slot::Idle(SlotInfo {
+ slot_data: slot_info.slot_data,
+ seqno: slot_info.seqno,
+ })
+ };
+
+ *locked_seat.access_mut(self) = match locked_seat.access(self) {
+ Seat::Active(seat_info) | Seat::Idle(seat_info) => Seat::Idle(SeatInfo {
+ slot: seat_info.slot,
+ seqno: seat_info.seqno,
+ }),
+ Seat::NoSeat => Seat::NoSeat,
+ };
+ Ok(())
+ }
+
+ /// Evicts a seat from its slot and marks the slot as free.
+ ///
+ /// Calls the eviction callback then frees the slot and resets the seat to `NoSeat`.
+ fn evict_slot(&mut self, slot_idx: usize, locked_seat: &LockedSeat<T, MAX_SLOTS>) -> Result {
+ match &self.slots[slot_idx] {
+ Slot::Active(slot_info) | Slot::Idle(slot_info) => {
+ self.manager.evict(slot_idx, &slot_info.slot_data)?;
+ take(&mut self.slots[slot_idx]);
+ }
+ _ => (),
+ }
+
+ *locked_seat.access_mut(self) = Seat::NoSeat;
+ Ok(())
+ }
+
+ /// Checks and updates the seat state based on the slot it points to.
+ ///
+ /// Validates that the seat's sequence number matches the slot's sequence number.
+ /// If they don't match, the seat has been evicted and is reset to `NoSeat`.
+ fn check_seat(&mut self, locked_seat: &LockedSeat<T, MAX_SLOTS>) {
+ let (slot_idx, seqno, is_active) = match locked_seat.access(self) {
+ Seat::Active(info) => (info.slot as usize, info.seqno, true),
+ Seat::Idle(info) => (info.slot as usize, info.seqno, false),
+ _ => return,
+ };
+
+ let valid = if is_active {
+ !kernel::warn_on!(!matches!(&self.slots[slot_idx], Slot::Active(s) if s.seqno == seqno))
+ } else {
+ matches!(&self.slots[slot_idx], Slot::Idle(s) if s.seqno == seqno)
+ };
+
+ if !valid {
+ *locked_seat.access_mut(self) = Seat::NoSeat;
+ }
+ }
+
+ /// Make a resource active on any available/reclaimable slot.
+ ///
+ /// Returns [`EBUSY`] if all slots are in use and none can be reclaimed
+ /// or the reclaim failed. May also return errors from the callbacks.
+ pub(crate) fn activate(
+ &mut self,
+ locked_seat: &LockedSeat<T, MAX_SLOTS>,
+ slot_data: T::SlotData,
+ ) -> Result {
+ self.check_seat(locked_seat);
+ match locked_seat.access(self) {
+ Seat::Active(seat_info) | Seat::Idle(seat_info) => {
+ // With lazy eviction, if seqno matches, the hardware state is still
+ // valid for both Active and Idle slots, so just update our records
+ self.record_active_slot(seat_info.slot as usize, locked_seat, slot_data)
+ }
+ _ => self.allocate_slot(locked_seat, slot_data),
+ }
+ }
+
+ /// Flag a resource idle.
+ ///
+ /// The slot manager can decide to reclaim the slot this resource
+ /// was bound to at any point after function returns.
+ // The idle() method will be used when we start adding support for user VMs.
+ #[expect(dead_code)]
+ pub(crate) fn idle(&mut self, locked_seat: &LockedSeat<T, MAX_SLOTS>) -> Result {
+ self.check_seat(locked_seat);
+ if let Seat::Active(seat_info) = locked_seat.access(self) {
+ self.idle_slot(seat_info.slot as usize, locked_seat)?;
+ }
+ Ok(())
+ }
+
+ /// Evict a resource from its slot, and make this slot free again
+ /// for other users.
+ ///
+ /// May return errors from the eviction callback.
+ pub(crate) fn evict(&mut self, locked_seat: &LockedSeat<T, MAX_SLOTS>) -> Result {
+ self.check_seat(locked_seat);
+
+ match locked_seat.access(self) {
+ Seat::Active(seat_info) | Seat::Idle(seat_info) => {
+ let slot_idx = seat_info.slot as usize;
+
+ self.evict_slot(slot_idx, locked_seat)?;
+ }
+ _ => (),
+ }
+
+ Ok(())
+ }
+}
+
+impl<T: SlotOperations, const MAX_SLOTS: usize> Deref for SlotManager<T, MAX_SLOTS> {
+ type Target = T;
+
+ fn deref(&self) -> &Self::Target {
+ &self.manager
+ }
+}
+
+impl<T: SlotOperations, const MAX_SLOTS: usize> DerefMut for SlotManager<T, MAX_SLOTS> {
+ fn deref_mut(&mut self) -> &mut Self::Target {
+ &mut self.manager
+ }
+}
diff --git a/drivers/gpu/drm/tyr/tyr.rs b/drivers/gpu/drm/tyr/tyr.rs
index 9432ddd6b5b8..20b38120e20e 100644
--- a/drivers/gpu/drm/tyr/tyr.rs
+++ b/drivers/gpu/drm/tyr/tyr.rs
@@ -12,6 +12,7 @@
mod gem;
mod gpu;
mod regs;
+mod slot;
kernel::module_platform_driver! {
type: TyrPlatformDriverData,
--
2.52.0
^ permalink raw reply related [flat|nested] 26+ messages in thread* Claude review: drm/tyr: Add generic slot manager
2026-03-02 23:24 ` [PATCH v2 07/12] drm/tyr: Add generic slot manager Deborah Brouwer
@ 2026-03-03 2:48 ` Claude Code Review Bot
0 siblings, 0 replies; 26+ messages in thread
From: Claude Code Review Bot @ 2026-03-03 2:48 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
**Author:** Boris Brezillon
This is the most architecturally significant patch. The `SlotManager` abstraction is clean and well-documented.
A few observations:
1. In `idle_slot()`, the seat is read and then immediately overwritten:
```rust
+ fn idle_slot(&mut self, slot_idx: usize, locked_seat: &LockedSeat<T, MAX_SLOTS>) -> Result {
+ let slot = take(&mut self.slots[slot_idx]);
+
+ if let Slot::Active(slot_info) = slot {
+ self.slots[slot_idx] = Slot::Idle(SlotInfo {
+ slot_data: slot_info.slot_data,
+ seqno: slot_info.seqno,
+ })
+ };
+
+ *locked_seat.access_mut(self) = match locked_seat.access(self) {
```
The `access()` followed immediately by `access_mut()` on the same `LockedBy` works because `access()` returns a reference that is dropped before `access_mut()` is called (due to the `match` expression borrowing temporarily). But this pattern is subtle. It effectively copies the `SeatInfo` fields. This is fine since `SeatInfo` is `Copy`-like (contains `u8` and `u64`), but making `SeatInfo` derive `Clone`/`Copy` and simplifying would be cleaner.
2. `use_seqno` starts at 1 and increments on each activation. With `u64`, overflow is practically impossible (would take ~584 years at 1 billion activations/second), so no concern there.
3. The `Deref`/`DerefMut` implementations on `SlotManager` exposing the inner `T` are convenient but somewhat unusual — they allow calling `SlotOperations` methods directly on the manager. This is intentional for the `AsSlotManager` use case where the manager needs to call `as_enable()` etc. directly.
4. `record_active_slot` returns `Result` but can never fail — it always returns `Ok(())`. Minor cleanup opportunity.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 08/12] drm/tyr: add MMU module
2026-03-02 23:24 [PATCH v2 0/12] drm/tyr: firmware loading and MCU boot support Deborah Brouwer
` (6 preceding siblings ...)
2026-03-02 23:24 ` [PATCH v2 07/12] drm/tyr: Add generic slot manager Deborah Brouwer
@ 2026-03-02 23:24 ` Deborah Brouwer
2026-03-03 2:48 ` Claude review: " Claude Code Review Bot
2026-03-02 23:24 ` [PATCH v2 09/12] drm/tyr: add GPU virtual memory module Deborah Brouwer
` (4 subsequent siblings)
12 siblings, 1 reply; 26+ messages in thread
From: Deborah Brouwer @ 2026-03-02 23:24 UTC (permalink / raw)
To: dri-devel, rust-for-linux
Cc: daniel.almeida, aliceryhl, boris.brezillon, beata.michalska,
lyude, work, Deborah Brouwer
From: Boris Brezillon <boris.brezillon@collabora.com>
Add a Memory Management Unit (MMU) driver for Tyr. The MMU wraps a
SlotManager for allocating hardware address space slots. The underlying
AddressSpaceManager performs MMU operations including enabling/disabling
address spaces, flushing page tables, and locking regions for page table
updates.
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
Co-developed-by: Deborah Brouwer <deborah.brouwer@collabora.com>
Signed-off-by: Deborah Brouwer <deborah.brouwer@collabora.com>
---
Changes in v2:
- Add documentation.
- Propagate errors returned by as_flush/disable().
- Refactor VmAsData to use in-place pinned initialization for the page table.
- Make struct AddressSpaceConfig private.
drivers/gpu/drm/tyr/driver.rs | 3 +
drivers/gpu/drm/tyr/mmu.rs | 123 ++++++
drivers/gpu/drm/tyr/mmu/address_space.rs | 493 +++++++++++++++++++++++
drivers/gpu/drm/tyr/tyr.rs | 1 +
4 files changed, 620 insertions(+)
create mode 100644 drivers/gpu/drm/tyr/mmu.rs
create mode 100644 drivers/gpu/drm/tyr/mmu/address_space.rs
diff --git a/drivers/gpu/drm/tyr/driver.rs b/drivers/gpu/drm/tyr/driver.rs
index 781502c6db36..7174ab2fd011 100644
--- a/drivers/gpu/drm/tyr/driver.rs
+++ b/drivers/gpu/drm/tyr/driver.rs
@@ -43,6 +43,7 @@
gem::BoData,
gpu,
gpu::GpuInfo,
+ mmu::Mmu,
regs, //
};
@@ -146,6 +147,8 @@ fn probe(
let uninit_ddev = UnregisteredDevice::<TyrDrmDriver>::new(pdev.as_ref())?;
let platform: ARef<platform::Device> = pdev.into();
+ let _mmu = Mmu::new(pdev, iomem.as_arc_borrow(), &gpu_info)?;
+
let data = try_pin_init!(TyrDrmDeviceData {
pdev: platform.clone(),
clks <- new_mutex!(Clocks {
diff --git a/drivers/gpu/drm/tyr/mmu.rs b/drivers/gpu/drm/tyr/mmu.rs
new file mode 100644
index 000000000000..2c7ac1fb1ac2
--- /dev/null
+++ b/drivers/gpu/drm/tyr/mmu.rs
@@ -0,0 +1,123 @@
+// SPDX-License-Identifier: GPL-2.0 or MIT
+
+//! Memory Management Unit (MMU) driver for the Tyr GPU.
+//!
+//! This module manages GPU address spaces and virtual memory (VM) operations through
+//! hardware MMU slots. It provides functionality for flushing page tables and
+//! managing VM updates for active address spaces.
+//!
+//! The MMU coordinates with the [`AddressSpaceManager`] to handle hardware
+//! address space allocation and page table operations, using [`SlotManager`]
+//! to track which address spaces are currently active in hardware slots.
+//!
+//! [`AddressSpaceManager`]: address_space::AddressSpaceManager
+//! [`SlotManager`]: crate::slot::SlotManager
+#![allow(dead_code)]
+
+use core::ops::Range;
+
+use kernel::{
+ devres::Devres,
+ new_mutex,
+ platform,
+ prelude::*,
+ sync::{
+ Arc,
+ ArcBorrow,
+ Mutex, //
+ }, //
+};
+
+use crate::{
+ driver::IoMem,
+ gpu::GpuInfo,
+ mmu::address_space::{
+ AddressSpaceManager,
+ VmAsData, //
+ },
+ regs::MAX_AS_REGISTERS,
+ slot::SlotManager, //
+};
+
+pub(crate) mod address_space;
+
+pub(crate) type AsSlotManager = SlotManager<AddressSpaceManager, MAX_AS_REGISTERS>;
+
+/// MMU component of the GPU.
+///
+/// This is used to bind VM objects to an AS (Address Space) slot
+/// and make the VM active on the GPU.
+///
+/// All operations acquire an internal lock, allowing concurrent access from multiple
+/// threads. Methods may block if another thread holds the lock.
+#[pin_data]
+pub(crate) struct Mmu {
+ /// Manages the allocation of hardware MMU slots to GPU address spaces.
+ ///
+ /// Tracks which address spaces are currently active in hardware slots and
+ /// coordinates address space operations like flushing and VM updates.
+ ///
+ /// This mutex also protects individual [`Seat`]s that are wrapped with
+ /// `LockedBy<Seat, SlotManager<...>>` to share the same lock protection.
+ ///
+ /// [`Seat`]: crate::slot::Seat
+ #[pin]
+ pub(crate) as_manager: Mutex<AsSlotManager>,
+}
+
+impl Mmu {
+ /// Create an MMU component for this device.
+ pub(crate) fn new(
+ pdev: &platform::Device,
+ iomem: ArcBorrow<'_, Devres<IoMem>>,
+ gpu_info: &GpuInfo,
+ ) -> Result<Arc<Mmu>> {
+ let slot_count = gpu_info.as_present.count_ones().try_into()?;
+ let as_manager = AddressSpaceManager::new(pdev, iomem, gpu_info.as_present)?;
+ let mmu_init = try_pin_init!(Self{
+ as_manager <- new_mutex!(SlotManager::new(as_manager, slot_count)?),
+ });
+ Arc::pin_init(mmu_init, GFP_KERNEL)
+ }
+
+ /// Make a VM active.
+ ///
+ /// This implies assigning the VM to an AS slot through the slot manager.
+ pub(crate) fn activate_vm(&self, vm: ArcBorrow<'_, VmAsData>) -> Result {
+ self.as_manager.lock().activate_vm(vm)
+ }
+
+ /// Make the VM inactive.
+ ///
+ /// Evicts the VM from its AS slot through the slot manager.
+ pub(crate) fn deactivate_vm(&self, vm: &VmAsData) -> Result {
+ self.as_manager.lock().deactivate_vm(vm)
+ }
+
+ /// Flush caches after a VM update.
+ ///
+ /// If the VM is no longer resident, this is a NOP, otherwise, the
+ /// AS manager will flush the GPU and MMU Translation Lookaside Buffer (TLB) caches.
+ pub(crate) fn flush_vm(&self, vm: &VmAsData) -> Result {
+ self.as_manager.lock().flush_vm(vm)
+ }
+
+ /// Flags the start of a VM update.
+ ///
+ /// If the VM is resident, any GPU access on the memory range being
+ /// updated will be blocked until `Mmu::end_vm_update()` is called.
+ /// This guarantees the atomicity of a VM update.
+ /// If the VM is not resident, this is a NOP.
+ pub(crate) fn start_vm_update(&self, vm: &VmAsData, region: &Range<u64>) -> Result {
+ self.as_manager.lock().start_vm_update(vm, region)
+ }
+
+ /// Flags the end of a VM update.
+ ///
+ /// If the VM is resident, this will let GPU accesses on the updated
+ /// range go through, in case any of them were blocked.
+ /// If the VM is not resident, this is a NOP.
+ pub(crate) fn end_vm_update(&self, vm: &VmAsData) -> Result {
+ self.as_manager.lock().end_vm_update(vm)
+ }
+}
diff --git a/drivers/gpu/drm/tyr/mmu/address_space.rs b/drivers/gpu/drm/tyr/mmu/address_space.rs
new file mode 100644
index 000000000000..05ada3274466
--- /dev/null
+++ b/drivers/gpu/drm/tyr/mmu/address_space.rs
@@ -0,0 +1,493 @@
+// SPDX-License-Identifier: GPL-2.0 or MIT
+
+//! GPU address space management and hardware operations.
+//!
+//! This module manages GPU hardware address spaces (AS), including configuration,
+//! command submission, and page table update regions. It handles the hardware
+//! interaction for MMU operations through MMIO register access.
+//!
+//! The [`AddressSpaceManager`] implements [`SlotOperations`] to integrate with
+//! the slot management system, enabling and configuring address spaces in the
+//! hardware slots as needed.
+//!
+//! [`SlotOperations`]: crate::slot::SlotOperations
+
+use core::ops::Range;
+
+use kernel::{
+ bits::*,
+ device::{
+ Bound,
+ Device, //
+ },
+ devres::Devres,
+ error::Result,
+ io,
+ iommu::pgtable::{
+ Config,
+ IoPageTable,
+ ARM64LPAES1, //
+ },
+ platform,
+ prelude::*,
+ sizes::{
+ SZ_2M,
+ SZ_4K, //
+ },
+ sync::{
+ aref::ARef,
+ Arc,
+ ArcBorrow,
+ LockedBy, //
+ },
+ time::Delta, //
+};
+
+use crate::{
+ driver::IoMem,
+ mmu::{
+ AsSlotManager,
+ Mmu, //
+ },
+ regs::*,
+ slot::{
+ Seat,
+ SlotOperations, //
+ }, //
+};
+
+/// Hardware address space configuration registers.
+///
+/// Contains register values for configuring a GPU MMU address space.
+#[derive(Clone, Copy)]
+struct AddressSpaceConfig {
+ /// Translation configuration.
+ ///
+ /// Controls address translation mode, address range restrictions, translation table
+ /// walk attributes, and access permission settings for this address space.
+ transcfg: u64,
+
+ /// Translation table base address.
+ ///
+ /// The address of the top level of a translation table structure.
+ transtab: u64,
+
+ /// Memory attributes.
+ ///
+ /// Defines memory attribute indirection entries that control cacheability
+ /// and other memory access properties for the address space.
+ memattr: u64,
+}
+
+/// Virtual memory (VM) address space data for GPU MMU operations.
+///
+/// Contains all resources and information needed by the [`AddressSpaceManager`]
+/// to activate a VM in a hardware address space slot.
+///
+/// On activation, we will pass an [`Arc`]<[`VmAsData`]> that will be stored in
+/// the slot to make sure the page table and the underlying resources
+/// (pages) used by the AS slot won't go away while the MMU points to
+/// those.
+///
+/// The `as_seat` field uses [`LockedBy`] to ensure safe concurrent access to
+/// the slot assignment state, protected by the [`AsSlotManager`] lock.
+#[pin_data]
+pub(crate) struct VmAsData {
+ /// Tracks this VM's binding to a hardware address space slot.
+ as_seat: LockedBy<Seat, AsSlotManager>,
+
+ /// Virtual address bits for this address space.
+ va_bits: u8,
+
+ /// Page table.
+ ///
+ /// Managed by devres to ensure proper cleanup. The page table maps
+ /// GPU virtual addresses to physical addresses for this VM.
+ #[pin]
+ pub(crate) page_table: Devres<IoPageTable<ARM64LPAES1>>,
+}
+
+impl VmAsData {
+ /// Creates a new VM address space data structure.
+ ///
+ /// Initializes the page table for the address space.
+ pub(crate) fn new<'a>(
+ mmu: &'a Mmu,
+ pdev: &'a platform::Device,
+ va_bits: u32,
+ pa_bits: u32,
+ ) -> impl pin_init::PinInit<VmAsData, Error> + 'a {
+ // SAFETY: pdev is a bound device.
+ let dev = unsafe { pdev.as_ref().as_bound() };
+
+ let pt_config = Config {
+ quirks: 0,
+ pgsize_bitmap: SZ_4K | SZ_2M,
+ ias: va_bits,
+ oas: pa_bits,
+ coherent_walk: false,
+ };
+
+ let page_table_init = IoPageTable::new(dev, pt_config);
+
+ try_pin_init!(Self {
+ as_seat: LockedBy::new(&mmu.as_manager, Seat::NoSeat),
+ va_bits: va_bits as u8,
+ page_table <- page_table_init,
+ }? Error)
+ }
+
+ /// Computes the hardware configuration for this address space.
+ ///
+ /// The caller must ensure that the address space is evicted and cleaned up
+ /// before the `VmAsData` is dropped.
+ fn as_config(&self, dev: &Device<Bound>) -> Result<AddressSpaceConfig> {
+ let pt = self.page_table.access(dev)?;
+
+ Ok(AddressSpaceConfig {
+ transcfg: AS_TRANSCFG_PTW_MEMATTR_WB
+ | AS_TRANSCFG_PTW_RA
+ | AS_TRANSCFG_ADRMODE_AARCH64_4K
+ | as_transcfg_ina_bits(u64::from(55 - self.va_bits)),
+ // SAFETY: Caller ensures proper cleanup.
+ transtab: unsafe { pt.ttbr() },
+ memattr: mair_to_memattr(pt.mair()),
+ })
+ }
+}
+
+/// Converts AArch64 MAIR register value to GPU memory attributes.
+fn mair_to_memattr(mair: u64) -> u64 {
+ let mut memattr: u64 = 0;
+
+ for i in 0..8 {
+ let in_attr = (mair >> (8 * i)) as u8;
+ let outer = in_attr >> 4;
+ let inner = in_attr & 0xf;
+
+ // For caching to be enabled, inner and outer caching policy
+ // have to be both write-back, if one of them is write-through
+ // or non-cacheable, we just choose non-cacheable. Device
+ // memory is also translated to non-cacheable.
+ let out_attr = if (outer & 3 == 0) || (outer & 4 == 0) || (inner & 4 == 0) {
+ AS_MEMATTR_AARCH64_INNER_OUTER_NC
+ | AS_MEMATTR_AARCH64_SH_MIDGARD_INNER
+ | as_memattr_aarch64_inner_alloc_expl(false, false)
+ } else {
+ // Use SH_CPU_INNER mode so SH_IS, which is used when
+ // IOMMU_CACHE is set, actually maps to the standard
+ // definition of inner-shareable and not Mali's
+ // internal-shareable mode.
+ //
+ // TODO: this assumes a non-coherent system.
+ AS_MEMATTR_AARCH64_INNER_OUTER_WB
+ | AS_MEMATTR_AARCH64_SH_MIDGARD_INNER
+ | as_memattr_aarch64_inner_alloc_expl(inner & 1 != 0, inner & 2 != 0)
+ };
+
+ memattr |= (u64::from(out_attr)) << (8 * i);
+ }
+
+ memattr
+}
+
+/// Manages GPU hardware address spaces via MMIO register operations.
+///
+/// Coordinates all hardware-level address space operations including enabling,
+/// disabling, flushing, and updating address spaces. Implements [`SlotOperations`]
+/// to integrate with the generic slot management system.
+///
+/// [`SlotOperations`]: crate::slot::SlotOperations
+pub(crate) struct AddressSpaceManager {
+ /// Platform device reference for DMA and device operations.
+ pdev: ARef<platform::Device>,
+
+ /// Memory-mapped I/O region for GPU register access.
+ iomem: Arc<Devres<IoMem>>,
+
+ /// Bitmask of available address space slots from GPU_AS_PRESENT register.
+ as_present: u32,
+}
+
+impl SlotOperations for AddressSpaceManager {
+ /// VM address space data stored in each hardware slot.
+ type SlotData = Arc<VmAsData>;
+
+ /// Activates an address space in a hardware slot.
+ fn activate(&mut self, slot_idx: usize, slot_data: &Self::SlotData) -> Result {
+ let as_config = slot_data.as_config(self.dev())?;
+ self.as_enable(slot_idx, &as_config)
+ }
+
+ /// Evicts an address space from a hardware slot.
+ fn evict(&mut self, slot_idx: usize, _slot_data: &Self::SlotData) -> Result {
+ if self.iomem.try_access().is_some() {
+ self.as_flush(slot_idx)?;
+ self.as_disable(slot_idx)?;
+ }
+ Ok(())
+ }
+}
+
+impl AddressSpaceManager {
+ /// Creates a new address space manager.
+ ///
+ /// Initializes the manager with references to the platform device and
+ /// I/O memory region, along with the bitmask of available AS slots.
+ pub(super) fn new(
+ pdev: &platform::Device,
+ iomem: ArcBorrow<'_, Devres<IoMem>>,
+ as_present: u32,
+ ) -> Result<AddressSpaceManager> {
+ Ok(Self {
+ pdev: pdev.into(),
+ iomem: iomem.into(),
+ as_present,
+ })
+ }
+
+ /// Returns a reference to the bound device.
+ fn dev(&self) -> &Device<Bound> {
+ // SAFETY: pdev is a bound device.
+ unsafe { self.pdev.as_ref().as_bound() }
+ }
+
+ /// Validates that an AS slot number is within range and present in hardware.
+ ///
+ /// Checks that the slot index is less than [`MAX_AS_REGISTERS`] and that
+ /// the corresponding bit is set in the `as_present` mask read from the GPU.
+ ///
+ /// Returns [`EINVAL`] if the slot is out of range or not present in hardware.
+ fn validate_as_slot(&self, as_nr: usize) -> Result {
+ if as_nr >= MAX_AS_REGISTERS {
+ pr_err!(
+ "AS slot {} out of valid range (max {})\n",
+ as_nr,
+ MAX_AS_REGISTERS
+ );
+ return Err(EINVAL);
+ }
+
+ if (self.as_present & (1 << as_nr)) == 0 {
+ pr_err!(
+ "AS slot {} not present in hardware (AS_PRESENT={:#x})\n",
+ as_nr,
+ self.as_present
+ );
+ return Err(EINVAL);
+ }
+
+ Ok(())
+ }
+
+ /// Waits for an AS slot to become ready (not active).
+ ///
+ /// Returns an error if polling times out after 10ms or if register access fails.
+ fn as_wait_ready(&self, as_nr: usize) -> Result {
+ let op = || as_status(as_nr)?.read(self.dev(), &self.iomem);
+ let cond = |status: &u32| -> bool { *status & AS_STATUS_ACTIVE == 0 };
+ let _ =
+ io::poll::read_poll_timeout(op, cond, Delta::from_millis(0), Delta::from_millis(10))?;
+
+ Ok(())
+ }
+
+ /// Sends a command to an AS slot.
+ ///
+ /// Returns an error if waiting for ready times out or if register write fails.
+ fn as_send_cmd(&mut self, as_nr: usize, cmd: u32) -> Result {
+ self.as_wait_ready(as_nr)?;
+ as_command(as_nr)?.write(self.dev(), &self.iomem, cmd)?;
+ Ok(())
+ }
+
+ /// Sends a command to an AS slot and waits for completion.
+ ///
+ /// Returns an error if sending the command fails or if waiting for completion times out.
+ fn as_send_cmd_and_wait(&mut self, as_nr: usize, cmd: u32) -> Result {
+ self.as_send_cmd(as_nr, cmd)?;
+ self.as_wait_ready(as_nr)?;
+ Ok(())
+ }
+
+ /// Enables an AS slot with the provided configuration.
+ ///
+ /// Returns an error if the slot is invalid or if register writes/commands fail.
+ fn as_enable(&mut self, as_nr: usize, as_config: &AddressSpaceConfig) -> Result {
+ self.validate_as_slot(as_nr)?;
+
+ let transtab = as_config.transtab;
+ let transcfg = as_config.transcfg;
+ let memattr = as_config.memattr;
+
+ let transtab_lo = (transtab & 0xffffffff) as u32;
+ let transtab_hi = (transtab >> 32) as u32;
+
+ let transcfg_lo = (transcfg & 0xffffffff) as u32;
+ let transcfg_hi = (transcfg >> 32) as u32;
+
+ let memattr_lo = (memattr & 0xffffffff) as u32;
+ let memattr_hi = (memattr >> 32) as u32;
+
+ let dev = self.dev();
+ as_transtab_lo(as_nr)?.write(dev, &self.iomem, transtab_lo)?;
+ as_transtab_hi(as_nr)?.write(dev, &self.iomem, transtab_hi)?;
+
+ as_transcfg_lo(as_nr)?.write(dev, &self.iomem, transcfg_lo)?;
+ as_transcfg_hi(as_nr)?.write(dev, &self.iomem, transcfg_hi)?;
+
+ as_memattr_lo(as_nr)?.write(dev, &self.iomem, memattr_lo)?;
+ as_memattr_hi(as_nr)?.write(dev, &self.iomem, memattr_hi)?;
+
+ self.as_send_cmd_and_wait(as_nr, AS_COMMAND_UPDATE)?;
+
+ Ok(())
+ }
+
+ /// Disables an AS slot and clears its configuration.
+ ///
+ /// Returns an error if the slot is invalid or if register writes/commands fail.
+ fn as_disable(&mut self, as_nr: usize) -> Result {
+ self.validate_as_slot(as_nr)?;
+
+ // Flush AS before disabling
+ self.as_send_cmd_and_wait(as_nr, AS_COMMAND_FLUSH_MEM)?;
+
+ let dev = self.dev();
+ as_transtab_lo(as_nr)?.write(dev, &self.iomem, 0)?;
+ as_transtab_hi(as_nr)?.write(dev, &self.iomem, 0)?;
+
+ as_memattr_lo(as_nr)?.write(dev, &self.iomem, 0)?;
+ as_memattr_hi(as_nr)?.write(dev, &self.iomem, 0)?;
+
+ as_transcfg_lo(as_nr)?.write(dev, &self.iomem, AS_TRANSCFG_ADRMODE_UNMAPPED as u32)?;
+ as_transcfg_hi(as_nr)?.write(dev, &self.iomem, 0)?;
+
+ self.as_send_cmd_and_wait(as_nr, AS_COMMAND_UPDATE)?;
+
+ Ok(())
+ }
+
+ /// Starts an atomic translation table update for a memory region.
+ ///
+ /// Returns an error if the slot is invalid or if register writes/commands fail.
+ fn as_start_update(&mut self, as_nr: usize, region: &Range<u64>) -> Result {
+ self.validate_as_slot(as_nr)?;
+
+ // The locked region is a naturally aligned power of 2 block encoded as
+ // log2 minus 1.
+ //
+ // Calculate the desired start/end and look for the highest bit which
+ // differs. The smallest naturally aligned block must include this bit
+ // change, the desired region starts with this bit (and subsequent bits)
+ // zeroed and ends with the bit (and subsequent bits) set to one.
+ let region_width = core::cmp::max(
+ 64 - (region.start ^ (region.end - 1)).leading_zeros() as u8,
+ AS_LOCK_REGION_MIN_SIZE.trailing_zeros() as u8,
+ ) - 1;
+
+ // Mask off the low bits of region.start, which would be ignored by the
+ // hardware anyways.
+ let region_start =
+ region.start & genmask_checked_u64(u32::from(region_width)..=63).ok_or(EINVAL)?;
+
+ let region = (u64::from(region_width)) | region_start;
+
+ let region_lo = (region & 0xffffffff) as u32;
+ let region_hi = (region >> 32) as u32;
+
+ // Lock the region that needs to be updated.
+ let dev = self.dev();
+ as_lockaddr_lo(as_nr)?.write(dev, &self.iomem, region_lo)?;
+ as_lockaddr_hi(as_nr)?.write(dev, &self.iomem, region_hi)?;
+
+ self.as_send_cmd(as_nr, AS_COMMAND_LOCK)
+ }
+
+ /// Completes an atomic translation table update.
+ ///
+ /// Returns an error if the slot is invalid or if the flush command fails.
+ fn as_end_update(&mut self, as_nr: usize) -> Result {
+ self.validate_as_slot(as_nr)?;
+ self.as_send_cmd_and_wait(as_nr, AS_COMMAND_FLUSH_PT)
+ }
+
+ /// Flushes the translation table cache for an AS slot.
+ ///
+ /// Returns an error if the slot is invalid or if the flush command fails.
+ fn as_flush(&mut self, as_nr: usize) -> Result {
+ self.validate_as_slot(as_nr)?;
+ self.as_send_cmd(as_nr, AS_COMMAND_FLUSH_PT)
+ }
+}
+
+impl AsSlotManager {
+ /// Locks a region for translation table updates if the VM has an active slot.
+ ///
+ /// If the VM is currently assigned to a hardware slot, locks the specified
+ /// memory region to make translation table updates atomic. GPU accesses to the
+ /// region will be blocked until [`end_vm_update`] is called.
+ ///
+ /// If the VM is not resident in a hardware slot, this is a no-op.
+ pub(super) fn start_vm_update(&mut self, vm: &VmAsData, region: &Range<u64>) -> Result {
+ let seat = vm.as_seat.access(self);
+ match seat.slot() {
+ Some(slot) => {
+ let as_nr = slot as usize;
+ self.as_start_update(as_nr, region)
+ }
+ _ => Ok(()),
+ }
+ }
+
+ /// Completes translation table updates and unlocks the region.
+ ///
+ /// If the VM is currently assigned to a hardware slot, flushes the translation
+ /// table cache and unlocks the region that was locked by [`start_vm_update`],
+ /// allowing GPU accesses to proceed with the updated translation tables.
+ ///
+ /// If the VM is not resident in a hardware slot, this is a no-op.
+ pub(super) fn end_vm_update(&mut self, vm: &VmAsData) -> Result {
+ let seat = vm.as_seat.access(self);
+ match seat.slot() {
+ Some(slot) => {
+ let as_nr = slot as usize;
+ self.as_end_update(as_nr)
+ }
+ _ => Ok(()),
+ }
+ }
+
+ /// Flushes translation table cache if the VM has an active slot.
+ ///
+ /// If the VM is currently assigned to a hardware slot, invalidates cached
+ /// translation table entries to ensure subsequent GPU accesses use updated translations.
+ ///
+ /// If the VM is not resident in a hardware slot, this is a no-op.
+ pub(super) fn flush_vm(&mut self, vm: &VmAsData) -> Result {
+ let seat = vm.as_seat.access(self);
+ match seat.slot() {
+ Some(slot) => {
+ let as_nr = slot as usize;
+ self.as_flush(as_nr)
+ }
+ _ => Ok(()),
+ }
+ }
+
+ /// Activates a VM by assigning it to a hardware slot.
+ ///
+ /// Allocates a hardware address space slot for the VM and configures
+ /// it with the VM's translation table and memory attributes.
+ pub(super) fn activate_vm(&mut self, vm: ArcBorrow<'_, VmAsData>) -> Result {
+ self.activate(&vm.as_seat, vm.into())
+ }
+
+ /// Deactivates a VM by evicting it from its hardware slot.
+ ///
+ /// Flushes any pending operations and clears the hardware slot's
+ /// configuration, freeing the slot for use by other VMs.
+ pub(super) fn deactivate_vm(&mut self, vm: &VmAsData) -> Result {
+ self.evict(&vm.as_seat)
+ }
+}
diff --git a/drivers/gpu/drm/tyr/tyr.rs b/drivers/gpu/drm/tyr/tyr.rs
index 20b38120e20e..9f9f31ea02e3 100644
--- a/drivers/gpu/drm/tyr/tyr.rs
+++ b/drivers/gpu/drm/tyr/tyr.rs
@@ -11,6 +11,7 @@
mod file;
mod gem;
mod gpu;
+mod mmu;
mod regs;
mod slot;
--
2.52.0
^ permalink raw reply related [flat|nested] 26+ messages in thread* Claude review: drm/tyr: add MMU module
2026-03-02 23:24 ` [PATCH v2 08/12] drm/tyr: add MMU module Deborah Brouwer
@ 2026-03-03 2:48 ` Claude Code Review Bot
0 siblings, 0 replies; 26+ messages in thread
From: Claude Code Review Bot @ 2026-03-03 2:48 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
**Author:** Boris Brezillon
The MMU module wraps the slot manager for address space management.
In `Mmu::new()`:
```rust
+ let slot_count = gpu_info.as_present.count_ones().try_into()?;
```
`count_ones()` returns `u32`, and `try_into()` converts to `usize`. This can't fail in practice on any supported platform, but the `?` is harmless.
In `driver.rs`, the MMU is created but the result is initially unused:
```rust
+ let _mmu = Mmu::new(pdev, iomem.as_arc_borrow(), &gpu_info)?;
```
This is later replaced in patch 12 with `let mmu = ...` when firmware loading is added. This is fine for the incremental build-up approach.
The `AddressSpaceManager`'s `dev()` method has a safety comment:
```rust
+ fn dev(&self) -> &Device<Bound> {
+ // SAFETY: pdev is a bound device.
+ unsafe { self.pdev.as_ref().as_bound() }
+ }
```
This is called from `evict()` which can happen during driver teardown. If the device has been unbound by the time eviction happens, this would be unsound. However, the `iomem.try_access().is_some()` check in `evict()` should gate this — if devres has been released, `try_access()` returns `None` and the device operations are skipped. This appears safe.
The `mair_to_memattr()` function translates AArch64 MAIR register values to GPU memory attributes. The logic matches the C panthor driver's `panthor_mmu_as_enable()`. The comment about non-coherent systems (`TODO: this assumes a non-coherent system`) is good to keep visible.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 09/12] drm/tyr: add GPU virtual memory module
2026-03-02 23:24 [PATCH v2 0/12] drm/tyr: firmware loading and MCU boot support Deborah Brouwer
` (7 preceding siblings ...)
2026-03-02 23:24 ` [PATCH v2 08/12] drm/tyr: add MMU module Deborah Brouwer
@ 2026-03-02 23:24 ` Deborah Brouwer
2026-03-03 2:48 ` Claude review: " Claude Code Review Bot
2026-03-02 23:24 ` [PATCH v2 10/12] drm/tyr: add a kernel buffer object Deborah Brouwer
` (3 subsequent siblings)
12 siblings, 1 reply; 26+ messages in thread
From: Deborah Brouwer @ 2026-03-02 23:24 UTC (permalink / raw)
To: dri-devel, rust-for-linux
Cc: daniel.almeida, aliceryhl, boris.brezillon, beata.michalska,
lyude, work, Deborah Brouwer
From: Boris Brezillon <boris.brezillon@collabora.com>
Add GPU virtual address space management using the DRM GPUVM framework.
Each virtual memory (VM) space is backed by ARM64 LPAE Stage 1 page tables
and can be mapped into hardware address space (AS) slots for GPU execution.
The implementation provides memory isolation and virtual address
allocation. VMs support mapping GEM buffer objects with configurable
protection flags (readonly, noexec, uncached) and handle both 4KB and 2MB
page sizes. A new_dummy_object() helper is provided to create a dummy GEM
object for use as a GPUVM root.
The vm module integrates with the MMU for address space activation and
provides map/unmap/remap operations with page table synchronization.
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
Co-developed-by: Daniel Almeida <daniel.almeida@collabora.com>
Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
Co-developed-by: Deborah Brouwer <deborah.brouwer@collabora.com>
Signed-off-by: Deborah Brouwer <deborah.brouwer@collabora.com>
---
Changes in v2:
- Add documentation.
- Add function new_dummy_object().
- Remove page_table initialization and associated support functions
from Vm creation.
drivers/gpu/drm/tyr/gem.rs | 22 +-
drivers/gpu/drm/tyr/gpu.rs | 1 -
drivers/gpu/drm/tyr/tyr.rs | 1 +
drivers/gpu/drm/tyr/vm.rs | 795 +++++++++++++++++++++++++++++++++++++
4 files changed, 817 insertions(+), 2 deletions(-)
create mode 100644 drivers/gpu/drm/tyr/vm.rs
diff --git a/drivers/gpu/drm/tyr/gem.rs b/drivers/gpu/drm/tyr/gem.rs
index c6d4d6f9bae3..111acf33993f 100644
--- a/drivers/gpu/drm/tyr/gem.rs
+++ b/drivers/gpu/drm/tyr/gem.rs
@@ -7,9 +7,11 @@
use kernel::{
drm::{
gem,
+ gem::shmem,
DeviceContext, //
},
- prelude::*, //
+ prelude::*,
+ sync::aref::ARef, //
};
use crate::driver::{
@@ -41,3 +43,21 @@ fn new<Ctx: DeviceContext>(
try_pin_init!(Self { flags: args.flags })
}
}
+
+/// Type alias for Tyr GEM buffer objects.
+pub(crate) type Bo = gem::shmem::Object<BoData>;
+
+/// Creates a dummy GEM object to serve as the root of a GPUVM.
+pub(crate) fn new_dummy_object<Ctx: DeviceContext>(ddev: &TyrDrmDevice<Ctx>) -> Result<ARef<Bo>> {
+ let bo = gem::shmem::Object::<BoData>::new(
+ ddev,
+ 4096,
+ shmem::ObjectConfig {
+ map_wc: true,
+ parent_resv_obj: None,
+ },
+ BoCreateArgs { flags: 0 },
+ )?;
+
+ Ok(bo)
+}
diff --git a/drivers/gpu/drm/tyr/gpu.rs b/drivers/gpu/drm/tyr/gpu.rs
index 5d9637899c7d..c4cac3418455 100644
--- a/drivers/gpu/drm/tyr/gpu.rs
+++ b/drivers/gpu/drm/tyr/gpu.rs
@@ -133,7 +133,6 @@ pub(crate) fn log(&self, pdev: &platform::Device) {
}
/// Returns the number of virtual address bits supported by the GPU.
- #[expect(dead_code)]
pub(crate) fn va_bits(&self) -> u32 {
self.mmu_features & genmask_u32(0..=7)
}
diff --git a/drivers/gpu/drm/tyr/tyr.rs b/drivers/gpu/drm/tyr/tyr.rs
index 9f9f31ea02e3..b3244670dd79 100644
--- a/drivers/gpu/drm/tyr/tyr.rs
+++ b/drivers/gpu/drm/tyr/tyr.rs
@@ -14,6 +14,7 @@
mod mmu;
mod regs;
mod slot;
+mod vm;
kernel::module_platform_driver! {
type: TyrPlatformDriverData,
diff --git a/drivers/gpu/drm/tyr/vm.rs b/drivers/gpu/drm/tyr/vm.rs
new file mode 100644
index 000000000000..f19acebdd66b
--- /dev/null
+++ b/drivers/gpu/drm/tyr/vm.rs
@@ -0,0 +1,795 @@
+// SPDX-License-Identifier: GPL-2.0 or MIT
+
+//! GPU virtual memory management using the DRM GPUVM framework.
+//!
+//! This module manages GPU virtual address spaces, providing memory isolation and
+//! the illusion of owning the entire virtual address (VA) range, similar to CPU virtual memory.
+//! Each virtual memory (VM) area is backed by ARM64 LPAE Stage 1 page tables and can be
+//! mapped into hardware address space (AS) slots for GPU execution.
+#![allow(dead_code)]
+
+use core::ops::Range;
+
+use kernel::{
+ c_str,
+ drm::{
+ gpuvm::{
+ DriverGpuVm,
+ GpuVaAlloc,
+ GpuVm,
+ GpuVmBoRegistered,
+ GpuVmCore,
+ OpMap,
+ OpMapRequest,
+ OpMapped,
+ OpRemap,
+ OpRemapped,
+ OpUnmap,
+ OpUnmapped, //
+ },
+ DeviceContext, //
+ },
+ impl_flags,
+ iommu::pgtable::{
+ prot,
+ IoPageTable,
+ ARM64LPAES1, //
+ },
+ new_mutex,
+ platform,
+ prelude::*,
+ sizes::{
+ SZ_1G,
+ SZ_2M,
+ SZ_4K, //
+ },
+ sync::{
+ aref::ARef,
+ Arc,
+ ArcBorrow,
+ Mutex, //
+ },
+ uapi, //
+};
+
+use crate::{
+ driver::{
+ TyrDrmDevice,
+ TyrDrmDriver, //
+ },
+ gem,
+ gem::Bo,
+ gpu::GpuInfo,
+ mmu::{
+ address_space::VmAsData,
+ Mmu, //
+ }, //
+};
+
+impl_flags!(
+ /// Flags controlling virtual memory mapping behavior.
+ ///
+ /// These flags control access permissions and caching behavior for GPU virtual
+ /// memory mappings.
+ #[derive(Debug, Clone, Default, Copy, PartialEq, Eq)]
+ pub(crate) struct VmMapFlags(u32);
+
+ /// Individual flags that can be combined in [`VmMapFlags`].
+ #[derive(Debug, Clone, Copy, PartialEq, Eq)]
+ pub(crate) enum VmFlag {
+ /// Map as read-only.
+ Readonly = uapi::drm_panthor_vm_bind_op_flags_DRM_PANTHOR_VM_BIND_OP_MAP_READONLY as u32,
+ /// Map as non-executable.
+ Noexec = uapi::drm_panthor_vm_bind_op_flags_DRM_PANTHOR_VM_BIND_OP_MAP_NOEXEC as u32,
+ /// Map as uncached.
+ Uncached = uapi::drm_panthor_vm_bind_op_flags_DRM_PANTHOR_VM_BIND_OP_MAP_UNCACHED as u32,
+ }
+);
+
+impl VmMapFlags {
+ /// Convert the flags to `pgtable::prot`.
+ fn to_prot(self) -> u32 {
+ let mut prot = 0;
+
+ if self.contains(VmFlag::Readonly) {
+ prot |= prot::READ;
+ } else {
+ prot |= prot::READ | prot::WRITE;
+ }
+
+ if self.contains(VmFlag::Noexec) {
+ prot |= prot::NOEXEC;
+ }
+
+ if !self.contains(VmFlag::Uncached) {
+ prot |= prot::CACHE;
+ }
+
+ prot
+ }
+}
+
+impl core::fmt::Display for VmMapFlags {
+ fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
+ let mut first = true;
+
+ if self.contains(VmFlag::Readonly) {
+ write!(f, "READONLY")?;
+ first = false;
+ }
+ if self.contains(VmFlag::Noexec) {
+ if !first {
+ write!(f, " | ")?;
+ }
+ write!(f, "NOEXEC")?;
+ first = false;
+ }
+
+ if self.contains(VmFlag::Uncached) {
+ if !first {
+ write!(f, " | ")?;
+ }
+ write!(f, "UNCACHED")?;
+ }
+
+ Ok(())
+ }
+}
+
+impl TryFrom<u32> for VmMapFlags {
+ type Error = Error;
+
+ fn try_from(value: u32) -> core::result::Result<Self, Self::Error> {
+ let valid = (kernel::uapi::drm_panthor_vm_bind_op_flags_DRM_PANTHOR_VM_BIND_OP_MAP_READONLY
+ | kernel::uapi::drm_panthor_vm_bind_op_flags_DRM_PANTHOR_VM_BIND_OP_MAP_NOEXEC
+ | kernel::uapi::drm_panthor_vm_bind_op_flags_DRM_PANTHOR_VM_BIND_OP_MAP_UNCACHED)
+ as u32;
+
+ if value & !valid != 0 {
+ pr_err!("Invalid VM map flags: {:#x}\n", value);
+ return Err(EINVAL);
+ }
+ Ok(Self(value))
+ }
+}
+
+/// Arguments for a virtual memory map operation.
+struct VmMapArgs {
+ /// Access permissions and caching behavior for the mapping.
+ flags: VmMapFlags,
+ /// GEM buffer object registered with the GPUVM framework.
+ vm_bo: GpuVmBoRegistered<GpuVmData>,
+ /// Offset in bytes from the start of the buffer object.
+ bo_offset: u64,
+}
+
+/// Type of virtual memory operation.
+enum VmOpType {
+ /// Map a GEM buffer object into the virtual address space.
+ Map(VmMapArgs),
+ /// Unmap a region from the virtual address space.
+ Unmap,
+}
+
+/// Preallocated resources needed to execute a VM operation.
+///
+/// VM operations may require allocating new GPUVA objects to track mappings.
+/// To avoid allocation failures during the operation, preallocate the
+/// maximum number of GPUVAs that might be needed.
+struct VmOpResources {
+ /// Preallocated GPUVA objects for remap operations.
+ ///
+ /// Partial unmap requests or map requests overlapping existing mappings
+ /// will trigger a remap call, which needs to register up to three VA
+ /// objects (one for the new mapping, and two for the previous and next
+ /// mappings).
+ preallocated_gpuvas: [Option<GpuVaAlloc<GpuVmData>>; 3],
+}
+
+/// Request to execute a virtual memory operation.
+struct VmOpRequest {
+ /// Request type.
+ op_type: VmOpType,
+
+ /// Region of the virtual address space covered by this request.
+ region: Range<u64>,
+}
+
+/// Arguments for a page table map operation.
+struct PtMapArgs {
+ /// Memory protection flags describing allowed accesses for this mapping.
+ ///
+ /// This is directly derived from [`VmMapFlags`] via [`VmMapFlags::to_prot`].
+ prot: u32,
+}
+
+/// Type of page table operation.
+enum PtOpType {
+ /// Map pages into the page table.
+ Map(PtMapArgs),
+ /// Unmap pages from the page table.
+ Unmap,
+}
+
+/// Context for updating the GPU page table.
+///
+/// This context is created when beginning a page table update operation and
+/// automatically flushes changes when dropped. It ensures that the
+/// Memory Management Unit (MMU) state is properly managed and Translation
+/// Lookaside Buffer (TLB) entries are flushed.
+pub(crate) struct PtUpdateContext<'ctx> {
+ /// Page table.
+ pt: &'ctx IoPageTable<ARM64LPAES1>,
+
+ /// MMU manager.
+ mmu: &'ctx Mmu,
+
+ /// Reference to the address space data to pass to the MMU functions.
+ as_data: &'ctx VmAsData,
+
+ /// Region of the virtual address space covered by this request.
+ region: Range<u64>,
+
+ /// Operation type.
+ op_type: PtOpType,
+
+ /// Preallocated resources that can be used when executing the request.
+ resources: &'ctx mut VmOpResources,
+}
+
+impl<'ctx> PtUpdateContext<'ctx> {
+ /// Creates a new page table update context.
+ ///
+ /// This prepares the MMU for a page table update.
+ /// The context will automatically flush the TLB and
+ /// complete the update when dropped.
+ fn new(
+ pt: &'ctx IoPageTable<ARM64LPAES1>,
+ mmu: &'ctx Mmu,
+ as_data: &'ctx VmAsData,
+ region: Range<u64>,
+ op_type: PtOpType,
+ resources: &'ctx mut VmOpResources,
+ ) -> Result<PtUpdateContext<'ctx>> {
+ mmu.start_vm_update(as_data, ®ion)?;
+
+ Ok(Self {
+ pt,
+ mmu,
+ as_data,
+ region,
+ op_type,
+ resources,
+ })
+ }
+
+ /// Finds one of our pre-allocated VAs.
+ ///
+ /// It is a logic error to call this more than three times for a given
+ /// PtUpdateContext.
+ fn preallocated_gpuva(&mut self) -> Result<GpuVaAlloc<GpuVmData>> {
+ self.resources
+ .preallocated_gpuvas
+ .iter_mut()
+ .find_map(|f| f.take())
+ .ok_or(EINVAL)
+ }
+}
+
+impl Drop for PtUpdateContext<'_> {
+ fn drop(&mut self) {
+ if let Err(e) = self.mmu.end_vm_update(self.as_data) {
+ pr_err!("Failed to end VM update {:?}\n", e);
+ }
+
+ if let Err(e) = self.mmu.flush_vm(self.as_data) {
+ pr_err!("Failed to flush VM {:?}\n", e);
+ }
+ }
+}
+
+/// Driver implementation for the GPUVM framework.
+///
+/// Implements [`DriverGpuVm`] to provide VM operation callbacks (map, unmap, remap)
+/// and associated types for buffer objects, virtual addresses, and contexts.
+pub(crate) struct GpuVmData {}
+
+/// GPU virtual address space.
+///
+/// Each VM can be mapped into a hardware address space slot.
+#[pin_data]
+pub(crate) struct Vm {
+ /// Data referenced by an AS when the VM is active
+ as_data: Arc<VmAsData>,
+ /// MMU manager.
+ mmu: Arc<Mmu>,
+ /// Platform device reference (needed to access the page table via devres).
+ pdev: ARef<platform::Device>,
+ /// DRM GPUVM core for managing virtual address space.
+ #[pin]
+ gpuvm_core: Mutex<GpuVmCore<GpuVmData>>,
+ /// Non-core part of the GPUVM. Can be used for stuff that doesn't modify the
+ /// internal mapping tree, like GpuVm::obtain()
+ gpuvm: ARef<GpuVm<GpuVmData>>,
+ /// VA range for this VM.
+ va_range: Range<u64>,
+}
+
+impl Vm {
+ /// Creates a new GPU virtual address space.
+ ///
+ /// The VM is initialized with a page table configured according to the GPU's
+ /// address translation capabilities and registered with the GPUVM framework.
+ pub(crate) fn new<Ctx: DeviceContext>(
+ pdev: &platform::Device,
+ ddev: &TyrDrmDevice<Ctx>,
+ mmu: ArcBorrow<'_, Mmu>,
+ gpu_info: &GpuInfo,
+ ) -> Result<Arc<Vm>> {
+ let va_bits = gpu_info.va_bits();
+ let pa_bits = gpu_info.pa_bits();
+
+ let range = 0..(1u64 << va_bits);
+ let reserve_range = 0..0u64;
+
+ // dummy_obj is used to initialize the GPUVM tree.
+ let dummy_obj = gem::new_dummy_object(ddev).inspect_err(|e| {
+ pr_err!("Failed to create dummy GEM object: {:?}\n", e);
+ })?;
+
+ let gpuvm_core = kernel::drm::gpuvm::GpuVm::new::<Error, _>(
+ c_str!("Tyr::GpuVm"),
+ ddev,
+ &*dummy_obj,
+ range.clone(),
+ reserve_range,
+ GpuVmData {},
+ )
+ .inspect_err(|e| {
+ pr_err!("Failed to create GpuVm: {:?}\n", e);
+ })?;
+ let gpuvm = ARef::from(&*gpuvm_core);
+
+ let as_data = Arc::pin_init(VmAsData::new(&mmu, pdev, va_bits, pa_bits), GFP_KERNEL)?;
+
+ let vm = Arc::pin_init(
+ pin_init!(Self{
+ as_data,
+ pdev: pdev.into(),
+ mmu: mmu.into(),
+ gpuvm,
+ gpuvm_core <- new_mutex!(gpuvm_core),
+ va_range: range,
+ }),
+ GFP_KERNEL,
+ )?;
+
+ Ok(vm)
+ }
+
+ /// Activate the VM in a hardware address space slot.
+ pub(crate) fn activate(&self) -> Result {
+ self.mmu
+ .activate_vm(self.as_data.as_arc_borrow())
+ .inspect_err(|e| {
+ pr_err!("Failed to activate VM: {:?}\n", e);
+ })
+ }
+
+ /// Deactivate the VM by evicting it from its address space slot.
+ fn deactivate(&self) -> Result {
+ self.mmu.deactivate_vm(&self.as_data).inspect_err(|e| {
+ pr_err!("Failed to deactivate VM: {:?}\n", e);
+ })
+ }
+
+ /// Kills the VM by deactivating it and unmapping all regions.
+ pub(crate) fn kill(&self) {
+ // TODO: Turn the VM into a state where it can't be used.
+ let _ = self.deactivate().inspect_err(|e| {
+ pr_err!("Failed to deactivate VM: {:?}\n", e);
+ });
+ let _ = self
+ .unmap_range(self.va_range.start, self.va_range.end - self.va_range.start)
+ .inspect_err(|e| {
+ pr_err!("Failed to unmap range during deactivate: {:?}\n", e);
+ });
+ }
+
+ /// Executes a virtual memory operation.
+ ///
+ /// This handles both map and unmap operations by coordinating between the
+ /// GPUVM framework and the hardware page table.
+ fn exec_op(
+ &self,
+ gpuvm_core: &mut GpuVmCore<GpuVmData>,
+ req: VmOpRequest,
+ resources: &mut VmOpResources,
+ ) -> Result {
+ let pt = self
+ .as_data
+ .page_table
+ // SAFETY: pdev is a bound device.
+ .access(unsafe { self.pdev.as_ref().as_bound() })
+ .inspect_err(|e| {
+ pr_err!("Failed to access page table while mapping pages: {:?}\n", e);
+ })?;
+
+ match req.op_type {
+ VmOpType::Map(args) => {
+ let mut pt_upd = PtUpdateContext::new(
+ pt,
+ &self.mmu,
+ &self.as_data,
+ req.region,
+ PtOpType::Map(PtMapArgs {
+ prot: args.flags.to_prot(),
+ }),
+ resources,
+ )?;
+
+ gpuvm_core.sm_map(OpMapRequest {
+ addr: pt_upd.region.start,
+ range: pt_upd.region.end - pt_upd.region.start,
+ gem_offset: args.bo_offset,
+ vm_bo: args.vm_bo,
+ context: &mut pt_upd,
+ })
+ //PtUpdateContext drops here flushing the page table
+ }
+ VmOpType::Unmap => {
+ let mut pt_upd = PtUpdateContext::new(
+ pt,
+ &self.mmu,
+ &self.as_data,
+ req.region,
+ PtOpType::Unmap,
+ resources,
+ )?;
+
+ gpuvm_core.sm_unmap(
+ pt_upd.region.start,
+ pt_upd.region.end - pt_upd.region.start,
+ &mut pt_upd,
+ )
+ //PtUpdateContext drops here flushing the page table
+ }
+ }
+ }
+
+ /// Maps a GEM buffer object range into the VM at the specified virtual address.
+ ///
+ /// This creates a mapping from GPU virtual address `va` to the physical pages
+ /// backing the GEM object, starting at `bo_offset` bytes into the object and
+ /// spanning `size` bytes. The mapping respects the access permissions and
+ /// caching behavior specified in `flags`.
+ pub(crate) fn map_bo_range(
+ &self,
+ bo: &Bo,
+ bo_offset: u64,
+ size: u64,
+ va: u64,
+ flags: VmMapFlags,
+ ) -> Result {
+ let req = VmOpRequest {
+ op_type: VmOpType::Map(VmMapArgs {
+ vm_bo: self.gpuvm.obtain(bo, ())?,
+ flags,
+ bo_offset,
+ }),
+ region: va..(va + size),
+ };
+ let mut resources = VmOpResources {
+ preallocated_gpuvas: [
+ Some(GpuVaAlloc::<GpuVmData>::new(GFP_KERNEL)?),
+ Some(GpuVaAlloc::<GpuVmData>::new(GFP_KERNEL)?),
+ Some(GpuVaAlloc::<GpuVmData>::new(GFP_KERNEL)?),
+ ],
+ };
+ let mut gpuvm_core = self.gpuvm_core.lock();
+
+ self.exec_op(gpuvm_core.as_mut().get_mut(), req, &mut resources)?;
+
+ // We flush the defer cleanup list now. Things will be different in
+ // the asynchronous VM_BIND path, where we want the cleanup to
+ // happen outside the DMA signalling path.
+ self.gpuvm.deferred_cleanup();
+ Ok(())
+ }
+
+ /// Unmaps a virtual address range from the VM.
+ ///
+ /// This removes any existing mappings in the specified range, freeing the
+ /// virtual address space for reuse.
+ pub(crate) fn unmap_range(&self, va: u64, size: u64) -> Result {
+ let req = VmOpRequest {
+ op_type: VmOpType::Unmap,
+ region: va..(va + size),
+ };
+ let mut resources = VmOpResources {
+ preallocated_gpuvas: [
+ Some(GpuVaAlloc::<GpuVmData>::new(GFP_KERNEL)?),
+ Some(GpuVaAlloc::<GpuVmData>::new(GFP_KERNEL)?),
+ None,
+ ],
+ };
+ let mut gpuvm_core = self.gpuvm_core.lock();
+
+ self.exec_op(gpuvm_core.as_mut().get_mut(), req, &mut resources)?;
+
+ // We flush the defer cleanup list now. Things will be different in
+ // the asynchronous VM_BIND path, where we want the cleanup to
+ // happen outside the DMA signalling path.
+ self.gpuvm.deferred_cleanup();
+ Ok(())
+ }
+}
+
+impl DriverGpuVm for GpuVmData {
+ type Driver = TyrDrmDriver;
+ type Object = Bo;
+ type VmBoData = ();
+ type VaData = ();
+ type SmContext<'ctx> = PtUpdateContext<'ctx>;
+
+ /// Indicates that a new mapping should be created.
+ fn sm_step_map<'op>(
+ &mut self,
+ op: OpMap<'op, Self>,
+ context: &mut Self::SmContext<'_>,
+ ) -> Result<OpMapped<'op, Self>, Error> {
+ let start_iova = op.addr();
+ let mut iova = start_iova;
+ let mut bytes_left_to_map = op.length();
+ let mut gem_offset = op.gem_offset();
+ let sgt = op.obj().sg_table().inspect_err(|e| {
+ pr_err!("Failed to get sg_table: {:?}\n", e);
+ })?;
+ let prot = match &context.op_type {
+ PtOpType::Map(args) => args.prot,
+ _ => {
+ return Err(EINVAL);
+ }
+ };
+
+ for sgt_entry in sgt.iter() {
+ let mut paddr = sgt_entry.dma_address();
+ let mut sgt_entry_length: u64 = sgt_entry.dma_len();
+
+ if bytes_left_to_map == 0 {
+ break;
+ }
+
+ if gem_offset > 0 {
+ // Skip the entire SGT entry if the gem_offset exceeds its length
+ let skip = sgt_entry_length.min(gem_offset);
+ paddr += skip;
+ sgt_entry_length -= skip;
+ gem_offset -= skip;
+ }
+
+ if sgt_entry_length == 0 {
+ continue;
+ }
+
+ if gem_offset != 0 {
+ pr_err!("Invalid gem_offset {} in page table mapping.\n", gem_offset);
+ return Err(EINVAL);
+ }
+ let len = sgt_entry_length.min(bytes_left_to_map);
+
+ let segment_mapped = match pt_map(context.pt, iova, paddr, len, prot) {
+ Ok(segment_mapped) => segment_mapped,
+ Err(e) => {
+ // clean up any successful mappings from previous SGT entries.
+ let total_mapped = iova - start_iova;
+ if total_mapped > 0 {
+ pt_unmap(context.pt, start_iova..(start_iova + total_mapped)).ok();
+ }
+ return Err(e);
+ }
+ };
+
+ // Since there could be a partial mapping, only advance by the actual amount mapped
+ bytes_left_to_map -= segment_mapped;
+ iova += segment_mapped;
+ }
+
+ let gpuva = context.preallocated_gpuva()?;
+ let op = op.insert(gpuva, pin_init::init_zeroed());
+
+ Ok(op)
+ }
+
+ /// Indicates that an existing mapping should be removed.
+ fn sm_step_unmap<'op>(
+ &mut self,
+ op: OpUnmap<'op, Self>,
+ context: &mut Self::SmContext<'_>,
+ ) -> Result<OpUnmapped<'op, Self>, Error> {
+ let start_iova = op.va().addr();
+ let length = op.va().length();
+
+ let region = start_iova..(start_iova + length);
+ pt_unmap(context.pt, region.clone()).inspect_err(|e| {
+ pr_err!(
+ "Failed to unmap region {:#x}..{:#x}: {:?}\n",
+ region.start,
+ region.end,
+ e
+ );
+ })?;
+
+ let (op_unmapped, _va_removed) = op.remove();
+
+ Ok(op_unmapped)
+ }
+
+ /// Indicates that an existing mapping should be split up.
+ fn sm_step_remap<'op>(
+ &mut self,
+ op: OpRemap<'op, Self>,
+ context: &mut Self::SmContext<'_>,
+ ) -> Result<OpRemapped<'op, Self>, Error> {
+ let unmap_start = if let Some(prev) = op.prev() {
+ prev.addr() + prev.length()
+ } else {
+ op.va_to_unmap().addr()
+ };
+
+ let unmap_end = if let Some(next) = op.next() {
+ next.addr()
+ } else {
+ op.va_to_unmap().addr() + op.va_to_unmap().length()
+ };
+
+ let unmap_length = unmap_end - unmap_start;
+
+ if unmap_length > 0 {
+ let region = unmap_start..(unmap_start + unmap_length);
+ pt_unmap(context.pt, region.clone()).inspect_err(|e| {
+ pr_err!(
+ "Failed to unmap remap region {:#x}..{:#x}: {:?}\n",
+ region.start,
+ region.end,
+ e
+ );
+ })?;
+ }
+
+ let prev_va = context.preallocated_gpuva()?;
+ let next_va = context.preallocated_gpuva()?;
+
+ let (op_remapped, _remap_ret) = op.remap(
+ [prev_va, next_va],
+ pin_init::init_zeroed(),
+ pin_init::init_zeroed(),
+ );
+
+ Ok(op_remapped)
+ }
+}
+
+/// This function selects the largest supported block size (currently 4KB or 2MB)
+/// that can be used for a mapping at the given address and size, respecting alignment constraints.
+///
+/// We can map multiple pages at once but we can't exceed the size of the
+// table entry itself. So, if mapping 4KB pages, figure out how many pages
+// can be mapped before we hit the 2MB boundary. Or, if mapping 2MB pages,
+// figure out how many pages can be mapped before hitting the 1GB boundary
+// Returns the page size (4KB or 2MB) and the number of pages that can be mapped at that size.
+fn get_pgsize(addr: u64, size: u64) -> (u64, u64) {
+ // Get the distance to the next boundary of 2MB block
+ let blk_offset_2m = addr.wrapping_neg() % (SZ_2M as u64);
+
+ // Use 4K blocks if the address is not 2MB aligned, or we have less than 2MB to map
+ if blk_offset_2m != 0 || size < SZ_2M as u64 {
+ let pgcount = if blk_offset_2m == 0 {
+ size / SZ_4K as u64
+ } else {
+ blk_offset_2m.min(size) / SZ_4K as u64
+ };
+ return (SZ_4K as u64, pgcount);
+ }
+
+ let blk_offset_1g = addr.wrapping_neg() % (SZ_1G as u64);
+ let blk_offset = if blk_offset_1g == 0 {
+ SZ_1G as u64
+ } else {
+ blk_offset_1g
+ };
+ let pgcount = blk_offset.min(size) / SZ_2M as u64;
+
+ (SZ_2M as u64, pgcount)
+}
+
+/// Maps a physical address range into the page table at the specified virtual address.
+///
+/// This function maps `len` bytes of physical memory starting at `paddr` to the
+/// virtual address `iova`, using the protection flags specified in `prot`. It
+/// automatically selects optimal page sizes to minimize page table overhead.
+///
+/// If the mapping fails partway through, all successfully mapped pages are
+/// unmapped before returning an error.
+///
+/// Returns the number of bytes successfully mapped.
+fn pt_map(
+ pt: &IoPageTable<ARM64LPAES1>,
+ iova: u64,
+ paddr: u64,
+ len: u64,
+ prot: u32,
+) -> Result<u64> {
+ let mut segment_mapped = 0u64;
+ while segment_mapped < len {
+ let remaining = len - segment_mapped;
+ let curr_iova = iova + segment_mapped;
+ let curr_paddr = paddr + segment_mapped;
+
+ let (pgsize, pgcount) = get_pgsize(curr_iova | curr_paddr, remaining);
+
+ // SAFETY: Exclusive access to the page table is ensured because
+ // the pt reference comes from PtUpdateContext, which is created
+ // during a VM update operation, ensuring the driver does not concurrently
+ // modify the page table.
+ let (mapped, result) = unsafe {
+ pt.map_pages(
+ curr_iova as usize,
+ (curr_paddr as usize).try_into().unwrap(),
+ pgsize as usize,
+ pgcount as usize,
+ prot,
+ GFP_KERNEL,
+ )
+ };
+
+ if let Err(e) = result {
+ pr_err!("pt.map_pages failed at iova {:#x}: {:?}\n", curr_iova, e);
+ if segment_mapped > 0 {
+ pt_unmap(pt, iova..(iova + segment_mapped)).ok();
+ }
+ return Err(e);
+ }
+
+ if mapped == 0 {
+ pr_err!("Failed to map any pages at iova {:#x}\n", curr_iova);
+ if segment_mapped > 0 {
+ pt_unmap(pt, iova..(iova + segment_mapped)).ok();
+ }
+ return Err(ENOMEM);
+ }
+
+ segment_mapped += mapped as u64;
+ }
+
+ Ok(segment_mapped)
+}
+
+/// Unmaps a virtual address range from the page table.
+///
+/// This function removes all page table entries in the specified range,
+/// automatically handling different page sizes that may be present.
+fn pt_unmap(pt: &IoPageTable<ARM64LPAES1>, range: Range<u64>) -> Result {
+ let mut iova = range.start;
+ let mut bytes_left_to_unmap = range.end - range.start;
+
+ while bytes_left_to_unmap > 0 {
+ let (pgsize, pgcount) = get_pgsize(iova, bytes_left_to_unmap);
+
+ // SAFETY: Exclusive access to the page table is ensured because
+ // the pt reference comes from PtUpdateContext, which was
+ // created while holding &mut Vm, preventing any other access to the
+ // page table for the duration of this operation.
+ let unmapped = unsafe { pt.unmap_pages(iova as usize, pgsize as usize, pgcount as usize) };
+
+ if unmapped == 0 {
+ pr_err!("Failed to unmap any bytes at iova {:#x}\n", iova);
+ return Err(EINVAL);
+ }
+
+ bytes_left_to_unmap -= unmapped as u64;
+ iova += unmapped as u64;
+ }
+
+ Ok(())
+}
--
2.52.0
^ permalink raw reply related [flat|nested] 26+ messages in thread* Claude review: drm/tyr: add GPU virtual memory module
2026-03-02 23:24 ` [PATCH v2 09/12] drm/tyr: add GPU virtual memory module Deborah Brouwer
@ 2026-03-03 2:48 ` Claude Code Review Bot
0 siblings, 0 replies; 26+ messages in thread
From: Claude Code Review Bot @ 2026-03-03 2:48 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
**Author:** Boris Brezillon / Daniel Almeida / Deborah Brouwer
This is the largest patch. The VM module integrates GPUVM with the hardware page table.
Observations:
1. `reserve_range` is set to `0..0u64` (empty range):
```rust
+ let reserve_range = 0..0u64;
```
This means no VA space is reserved for the kernel. For the MCU firmware VM this is fine since the driver controls all mappings, but for user VMs this will need to be revisited.
2. In `sm_step_map`, when a mapping fails partway through SGT entries, the cleanup unmaps from `start_iova`:
```rust
+ Err(e) => {
+ let total_mapped = iova - start_iova;
+ if total_mapped > 0 {
+ pt_unmap(context.pt, start_iova..(start_iova + total_mapped)).ok();
+ }
+ return Err(e);
+ }
```
This is correct — it rolls back successful mappings from previous SGT entries. However, the `.ok()` silently ignores unmap failures during error recovery. A `pr_warn!` or similar would help debug issues.
3. The `get_pgsize()` function handles 4K and 2M pages:
```rust
+fn get_pgsize(addr: u64, size: u64) -> (u64, u64) {
+ let blk_offset_2m = addr.wrapping_neg() % (SZ_2M as u64);
```
Using `wrapping_neg()` to compute distance to next boundary is idiomatic. The function also caps the number of 2M pages at the 1G boundary, which is correct for AArch64 LPAE page tables where the next level table covers 1G.
4. The `PtUpdateContext::drop()` calls both `end_vm_update()` and `flush_vm()`, logging errors but not propagating them. This is unavoidable in `Drop`, but means that if the flush fails, GPU state may be inconsistent.
5. In `pt_map()`:
```rust
+ let (mapped, result) = unsafe {
+ pt.map_pages(
+ curr_iova as usize,
+ (curr_paddr as usize).try_into().unwrap(),
```
The `unwrap()` on `try_into()` could panic if `curr_paddr` doesn't fit in the target type of `PhysAddr`. On 32-bit systems this could be an issue, but the driver requires ARM64, so this is safe in practice. Worth a comment.
6. The SAFETY comments for `pt.map_pages()` and `pt.unmap_pages()` reference "PtUpdateContext" ensuring exclusive access. This is sound because the MMU lock + start_vm_update() sequence prevents concurrent page table modifications.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 10/12] drm/tyr: add a kernel buffer object
2026-03-02 23:24 [PATCH v2 0/12] drm/tyr: firmware loading and MCU boot support Deborah Brouwer
` (8 preceding siblings ...)
2026-03-02 23:24 ` [PATCH v2 09/12] drm/tyr: add GPU virtual memory module Deborah Brouwer
@ 2026-03-02 23:24 ` Deborah Brouwer
2026-03-03 2:48 ` Claude review: " Claude Code Review Bot
2026-03-02 23:24 ` [PATCH v2 11/12] drm/tyr: add parser for firmware binary Deborah Brouwer
` (2 subsequent siblings)
12 siblings, 1 reply; 26+ messages in thread
From: Deborah Brouwer @ 2026-03-02 23:24 UTC (permalink / raw)
To: dri-devel, rust-for-linux
Cc: daniel.almeida, aliceryhl, boris.brezillon, beata.michalska,
lyude, work, Deborah Brouwer
Introduce a buffer object type (KernelBo) for internal driver allocations
that are managed by the kernel rather than userspace.
KernelBo wraps a GEM shmem object and automatically handles GPU virtual
address space mapping during creation and unmapping on drop. This provides
a safe and convenient way for the driver to both allocate and clean up
internal buffers for kernel-managed resources.
Co-developed-by: Boris Brezillon <boris.brezillon@collabora.com>
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
Signed-off-by: Deborah Brouwer <deborah.brouwer@collabora.com>
---
Changes in v2:
- Add documentation.
- Add allocation strategy option for kernel buffer objects.
drivers/gpu/drm/tyr/gem.rs | 119 ++++++++++++++++++++++++++++++++++---
1 file changed, 112 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/tyr/gem.rs b/drivers/gpu/drm/tyr/gem.rs
index 111acf33993f..f3bb58ef5eae 100644
--- a/drivers/gpu/drm/tyr/gem.rs
+++ b/drivers/gpu/drm/tyr/gem.rs
@@ -4,6 +4,8 @@
//! This module provides buffer object (BO) management functionality using
//! DRM's GEM subsystem with shmem backing.
+use core::ops::Range;
+
use kernel::{
drm::{
gem,
@@ -11,23 +13,41 @@
DeviceContext, //
},
prelude::*,
- sync::aref::ARef, //
+ sync::{
+ aref::ARef,
+ Arc,
+ ArcBorrow, //
+ },
};
-use crate::driver::{
- TyrDrmDevice,
- TyrDrmDriver, //
+use crate::{
+ driver::{
+ TyrDrmDevice,
+ TyrDrmDriver, //
+ },
+ vm::{
+ Vm,
+ VmMapFlags, //
+ },
};
-/// Tyr's DriverObject type for GEM objects.
+/// Driver-specific data for Tyr GEM buffer objects.
+///
+/// This structure contains Tyr-specific metadata associated with each GEM object.
+/// It implements [`gem::DriverObject`] to provide driver-specific behavior for
+/// buffer object creation and management.
#[pin_data]
pub(crate) struct BoData {
+ /// Buffer object creation flags (currently unused).
flags: u32,
}
-/// Provides a way to pass arguments when creating BoData
-/// as required by the gem::DriverObject trait.
+/// Arguments for creating a [`BoData`] instance.
+///
+/// This structure is used to pass creation parameters when instantiating
+/// a new buffer object, as required by the [`gem::DriverObject`] trait.
pub(crate) struct BoCreateArgs {
+ /// Buffer object creation flags (currently unused).
flags: u32,
}
@@ -35,6 +55,12 @@ impl gem::DriverObject for BoData {
type Driver = TyrDrmDriver;
type Args = BoCreateArgs;
+ /// Constructs a new [`BoData`] instance for a GEM object.
+ ///
+ /// This function is called by the GEM subsystem when creating a new buffer
+ /// object. It initializes the driver-specific data with the provided flags.
+ /// The device and size parameters are currently unused but required by the
+ /// [`gem::DriverObject`] trait.
fn new<Ctx: DeviceContext>(
_dev: &TyrDrmDevice<Ctx>,
_size: usize,
@@ -61,3 +87,82 @@ pub(crate) fn new_dummy_object<Ctx: DeviceContext>(ddev: &TyrDrmDevice<Ctx>) ->
Ok(bo)
}
+
+/// VA allocation strategy for kernel buffer objects.
+///
+/// Specifies how the GPU virtual address should be determined when creating
+/// a [`KernelBo`]. An automatic VA allocation strategy will be added in the future.
+pub(crate) enum KernelBoVaAlloc {
+ /// Explicit VA address specified by the caller.
+ #[expect(dead_code)]
+ Explicit(u64),
+}
+
+/// A kernel-owned buffer object with automatic GPU virtual address mapping.
+///
+/// This structure represents a buffer object that is created and managed entirely
+/// by the kernel driver, as opposed to userspace-created GEM objects. It combines
+/// a GEM object with automatic GPU virtual address (VA) space mapping and cleanup.
+///
+/// When dropped, the buffer is automatically unmapped from the GPU VA space.
+pub(crate) struct KernelBo {
+ /// The underlying GEM buffer object.
+ #[expect(dead_code)]
+ pub(crate) bo: ARef<Bo>,
+ /// The GPU VM this buffer is mapped into.
+ vm: Arc<Vm>,
+ /// The GPU VA range occupied by this buffer.
+ va_range: Range<u64>,
+}
+
+impl KernelBo {
+ /// Creates a new kernel-owned buffer object and maps it into GPU VA space.
+ ///
+ /// This function allocates a new shmem-backed GEM object and immediately maps
+ /// it into the specified GPU virtual memory space. The mapping is automatically
+ /// cleaned up when the [`KernelBo`] is dropped.
+ #[expect(dead_code)]
+ pub(crate) fn new<Ctx: DeviceContext>(
+ ddev: &TyrDrmDevice<Ctx>,
+ vm: ArcBorrow<'_, Vm>,
+ size: u64,
+ va_alloc: KernelBoVaAlloc,
+ flags: VmMapFlags,
+ ) -> Result<Self> {
+ let KernelBoVaAlloc::Explicit(va) = va_alloc;
+
+ let bo = gem::shmem::Object::<BoData>::new(
+ ddev,
+ size as usize,
+ shmem::ObjectConfig {
+ map_wc: true,
+ parent_resv_obj: None,
+ },
+ BoCreateArgs { flags: 0 },
+ )?;
+
+ vm.map_bo_range(&bo, 0, size, va, flags)?;
+
+ Ok(KernelBo {
+ bo,
+ vm: vm.into(),
+ va_range: va..(va + size),
+ })
+ }
+}
+
+impl Drop for KernelBo {
+ fn drop(&mut self) {
+ let va = self.va_range.start;
+ let size = self.va_range.end - self.va_range.start;
+
+ if let Err(e) = self.vm.unmap_range(va, size) {
+ pr_err!(
+ "Failed to unmap KernelBo range {:#x}..{:#x}: {:?}\n",
+ self.va_range.start,
+ self.va_range.end,
+ e
+ );
+ }
+ }
+}
--
2.52.0
^ permalink raw reply related [flat|nested] 26+ messages in thread* Claude review: drm/tyr: add a kernel buffer object
2026-03-02 23:24 ` [PATCH v2 10/12] drm/tyr: add a kernel buffer object Deborah Brouwer
@ 2026-03-03 2:48 ` Claude Code Review Bot
0 siblings, 0 replies; 26+ messages in thread
From: Claude Code Review Bot @ 2026-03-03 2:48 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
**Author:** Deborah Brouwer / Boris Brezillon
Introduces `KernelBo` wrapping a GEM shmem object with auto-map/unmap in GPU VA space.
```rust
+ pub(crate) fn new<Ctx: DeviceContext>(
+ ...
+ size: u64,
+ ...
+ ) -> Result<Self> {
+ ...
+ let bo = gem::shmem::Object::<BoData>::new(
+ ddev,
+ size as usize,
```
The `size as usize` cast is safe on 64-bit (where `usize` is 64-bit), which is the target platform. But on 32-bit `COMPILE_TEST` builds, this could silently truncate. Low priority since firmware sections are small.
The `Drop` impl for `KernelBo` properly unmaps the VA range and logs errors. Clean design.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 11/12] drm/tyr: add parser for firmware binary
2026-03-02 23:24 [PATCH v2 0/12] drm/tyr: firmware loading and MCU boot support Deborah Brouwer
` (9 preceding siblings ...)
2026-03-02 23:24 ` [PATCH v2 10/12] drm/tyr: add a kernel buffer object Deborah Brouwer
@ 2026-03-02 23:24 ` Deborah Brouwer
2026-03-03 2:48 ` Claude review: " Claude Code Review Bot
2026-03-02 23:25 ` [PATCH v2 12/12] drm/tyr: add firmware loading and MCU boot support Deborah Brouwer
2026-03-03 2:48 ` Claude review: drm/tyr: " Claude Code Review Bot
12 siblings, 1 reply; 26+ messages in thread
From: Deborah Brouwer @ 2026-03-02 23:24 UTC (permalink / raw)
To: dri-devel, rust-for-linux
Cc: daniel.almeida, aliceryhl, boris.brezillon, beata.michalska,
lyude, work, Deborah Brouwer
From: Daniel Almeida <daniel.almeida@collabora.com>
Add a parser for the Mali CSF GPU firmware binary format. The firmware
consists of a header followed by entries describing how to load firmware
sections into the MCU's memory.
The parser extracts section metadata including virtual address ranges,
data byte offsets within the binary, and section flags controlling
permissions and cache modes. It validates the basic firmware structure
and alignment and ignores protected-mode sections for now.
Signed-off-by: Daniel Almeida <daniel.almeida@collabora.com>
Co-developed-by: Beata Michalska <beata.michalska@arm.com>
Signed-off-by: Beata Michalska <beata.michalska@arm.com>
Co-developed-by: Boris Brezillon <boris.brezillon@collabora.com>
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
Co-developed-by: Deborah Brouwer <deborah.brouwer@collabora.com>
Signed-off-by: Deborah Brouwer <deborah.brouwer@collabora.com>
---
Changes in v2:
- Add documentation.
drivers/gpu/drm/tyr/fw/parser.rs | 519 +++++++++++++++++++++++++++++++
1 file changed, 519 insertions(+)
create mode 100644 drivers/gpu/drm/tyr/fw/parser.rs
diff --git a/drivers/gpu/drm/tyr/fw/parser.rs b/drivers/gpu/drm/tyr/fw/parser.rs
new file mode 100644
index 000000000000..638707430701
--- /dev/null
+++ b/drivers/gpu/drm/tyr/fw/parser.rs
@@ -0,0 +1,519 @@
+// SPDX-License-Identifier: GPL-2.0 or MIT
+
+//! Firmware binary parser for Mali CSF (Command Stream Frontend) GPU.
+//!
+//! This module implements a parser for the Mali GPU firmware binary format. The firmware
+//! file contains a header followed by a sequence of entries, each describing how to load
+//! firmware sections into the MCU (Microcontroller Unit) memory. The parser extracts section metadata including:
+//! - Virtual address ranges where sections should be mapped
+//! - Data ranges (byte offsets) within the firmware binary
+//! - Section flags (permissions, cache modes)
+
+use core::{
+ mem::size_of,
+ ops::Range, //
+};
+
+use kernel::{
+ bits::bit_u32,
+ prelude::*,
+ str::CString, //
+};
+
+use crate::{
+ fw::{
+ SectionFlag,
+ SectionFlags,
+ CSF_MCU_SHARED_REGION_START, //
+ },
+ vm::{
+ VmFlag,
+ VmMapFlags, //
+ }, //
+};
+
+/// A parsed firmware section ready for loading into MCU memory.
+///
+/// Represents a single firmware section extracted from the firmware binary, containing
+/// all information needed to map the section's data into the MCU's virtual address space.
+pub(super) struct ParsedSection {
+ /// Byte offset range within the firmware binary where this section's data resides.
+ pub(super) data_range: Range<u32>,
+ /// MCU virtual address range where this section should be mapped.
+ pub(super) va: Range<u32>,
+ /// Memory protection and caching flags for the mapping.
+ pub(super) vm_map_flags: VmMapFlags,
+}
+
+/// A bare-bones `std::io::Cursor<[u8]>` clone to keep track of the current position in the firmware binary.
+///
+/// Provides methods to sequentially read primitive types and byte arrays from the firmware
+/// binary while maintaining the current read position.
+struct Cursor<'a> {
+ data: &'a [u8],
+ pos: usize,
+}
+
+impl<'a> Cursor<'a> {
+ fn new(data: &'a [u8]) -> Self {
+ Self { data, pos: 0 }
+ }
+
+ fn len(&self) -> usize {
+ self.data.len()
+ }
+
+ fn pos(&self) -> usize {
+ self.pos
+ }
+
+ /// Returns a view into the cursor's data.
+ ///
+ /// This spawns a new cursor, leaving the current cursor unchanged.
+ fn view(&self, range: Range<usize>) -> Result<Cursor<'_>> {
+ if range.start < self.pos || range.end > self.data.len() {
+ pr_err!(
+ "Invalid cursor range {:?} for data of length {}",
+ range,
+ self.data.len()
+ );
+
+ Err(EINVAL)
+ } else {
+ Ok(Self {
+ data: &self.data[range],
+ pos: 0,
+ })
+ }
+ }
+
+ /// Reads a slice of bytes from the current position and advances the cursor.
+ ///
+ /// Returns an error if the read would exceed the data bounds.
+ fn read(&mut self, nbytes: usize) -> Result<&[u8]> {
+ let start = self.pos;
+ let end = start + nbytes;
+
+ if end > self.data.len() {
+ pr_err!(
+ "Invalid firmware file: read of size {} at position {} is out of bounds",
+ nbytes,
+ start,
+ );
+ return Err(EINVAL);
+ }
+
+ self.pos += nbytes;
+ Ok(&self.data[start..end])
+ }
+
+ /// Reads a little-endian `u8` from the current position and advances the cursor.
+ fn read_u8(&mut self) -> Result<u8> {
+ let bytes = self.read(size_of::<u8>())?;
+ Ok(bytes[0])
+ }
+
+ /// Reads a little-endian `u16` from the current position and advances the cursor.
+ fn read_u16(&mut self) -> Result<u16> {
+ let bytes = self.read(size_of::<u16>())?;
+ Ok(u16::from_le_bytes(bytes.try_into().unwrap()))
+ }
+
+ /// Reads a little-endian `u32` from the current position and advances the cursor.
+ fn read_u32(&mut self) -> Result<u32> {
+ let bytes = self.read(size_of::<u32>())?;
+ Ok(u32::from_le_bytes(bytes.try_into().unwrap()))
+ }
+
+ /// Advances the cursor position by the specified number of bytes.
+ ///
+ /// Returns an error if the advance would exceed the data bounds.
+ fn advance(&mut self, nbytes: usize) -> Result {
+ if self.pos + nbytes > self.data.len() {
+ pr_err!(
+ "Invalid firmware file: advance of size {} at position {} is out of bounds",
+ nbytes,
+ self.pos,
+ );
+ return Err(EINVAL);
+ }
+ self.pos += nbytes;
+ Ok(())
+ }
+}
+
+/// Parser for Mali CSF GPU firmware binaries.
+///
+/// Parses the firmware binary format, extracting section metadata including virtual
+/// address ranges, data offsets, and memory protection flags needed to load firmware
+/// into the MCU's memory.
+pub(super) struct FwParser<'a> {
+ cursor: Cursor<'a>,
+}
+
+impl<'a> FwParser<'a> {
+ /// Creates a new firmware parser for the given firmware binary data.
+ pub(super) fn new(data: &'a [u8]) -> Self {
+ Self {
+ cursor: Cursor::new(data),
+ }
+ }
+
+ /// Parses the firmware binary and returns a collection of parsed sections.
+ ///
+ /// This method validates the firmware header and iterates through all entries
+ /// in the binary, extracting section information needed for loading.
+ pub(super) fn parse(&mut self) -> Result<KVec<ParsedSection>> {
+ let fw_header = self.parse_fw_header()?;
+
+ let mut parsed_sections = KVec::new();
+ while (self.cursor.pos() as u32) < fw_header.size {
+ let entry_section = self.parse_entry()?;
+
+ if let Some(inner) = entry_section.inner {
+ parsed_sections.push(inner, GFP_KERNEL)?;
+ }
+ }
+
+ Ok(parsed_sections)
+ }
+
+ fn parse_fw_header(&mut self) -> Result<FirmwareHeader> {
+ let fw_header: FirmwareHeader = match FirmwareHeader::new(&mut self.cursor) {
+ Ok(fw_header) => fw_header,
+ Err(e) => {
+ pr_err!("Invalid firmware file: {}", e.to_errno());
+ return Err(e);
+ }
+ };
+
+ if fw_header.size > self.cursor.len() as u32 {
+ pr_err!("Firmware image is truncated");
+ return Err(EINVAL);
+ }
+ Ok(fw_header)
+ }
+
+ fn parse_entry(&mut self) -> Result<EntrySection> {
+ let entry_section = EntrySection {
+ entry_hdr: EntryHeader(self.cursor.read_u32()?),
+ inner: None,
+ };
+
+ if self.cursor.pos() % size_of::<u32>() != 0
+ || entry_section.entry_hdr.size() as usize % size_of::<u32>() != 0
+ {
+ pr_err!(
+ "Firmware entry isn't 32 bit aligned, offset={:#x} size={:#x}\n",
+ self.cursor.pos() - size_of::<u32>(),
+ entry_section.entry_hdr.size()
+ );
+ return Err(EINVAL);
+ }
+
+ let section_hdr_size = entry_section.entry_hdr.size() as usize - size_of::<EntryHeader>();
+
+ let entry_section = {
+ let mut entry_cursor = self
+ .cursor
+ .view(self.cursor.pos()..self.cursor.pos() + section_hdr_size)?;
+
+ match entry_section.entry_hdr.entry_type() {
+ Ok(EntryType::Iface) => Ok(EntrySection {
+ entry_hdr: entry_section.entry_hdr,
+ inner: Self::parse_section_entry(&mut entry_cursor)?,
+ }),
+ Ok(
+ EntryType::Config
+ | EntryType::FutfTest
+ | EntryType::TraceBuffer
+ | EntryType::TimelineMetadata
+ | EntryType::BuildInfoMetadata,
+ ) => Ok(entry_section),
+
+ entry_type => {
+ if entry_type.is_err() || !entry_section.entry_hdr.optional() {
+ if !entry_section.entry_hdr.optional() {
+ pr_err!(
+ "Failed to handle firmware entry type: {}\n",
+ entry_type
+ .map_or(entry_section.entry_hdr.entry_type_raw(), |e| e as u8)
+ );
+ Err(EINVAL)
+ } else {
+ Ok(entry_section)
+ }
+ } else {
+ Ok(entry_section)
+ }
+ }
+ }
+ };
+
+ if entry_section.is_ok() {
+ self.cursor.advance(section_hdr_size)?;
+ }
+
+ entry_section
+ }
+
+ fn parse_section_entry(entry_cursor: &mut Cursor<'_>) -> Result<Option<ParsedSection>> {
+ let section_hdr: SectionHeader = SectionHeader::new(entry_cursor)?;
+
+ if section_hdr.data.end < section_hdr.data.start {
+ pr_err!(
+ "Firmware corrupted, data.end < data.start (0x{:x} < 0x{:x})\n",
+ section_hdr.data.end,
+ section_hdr.data.start
+ );
+ return Err(EINVAL);
+ }
+
+ if section_hdr.va.end < section_hdr.va.start {
+ pr_err!(
+ "Firmware corrupted, section_hdr.va.end < section_hdr.va.start (0x{:x} < 0x{:x})\n",
+ section_hdr.va.end,
+ section_hdr.va.start
+ );
+ return Err(EINVAL);
+ }
+
+ if section_hdr.section_flags.contains(SectionFlag::Prot) {
+ pr_info!("Firmware protected mode entry not supported, ignoring");
+ return Ok(None);
+ }
+
+ if section_hdr.va.start == CSF_MCU_SHARED_REGION_START
+ && !section_hdr.section_flags.contains(SectionFlag::Shared)
+ {
+ pr_err!(
+ "Interface at 0x{:x} must be shared",
+ CSF_MCU_SHARED_REGION_START
+ );
+ return Err(EINVAL);
+ }
+
+ let name_len = entry_cursor.len() - entry_cursor.pos();
+ let name_bytes = entry_cursor.read(name_len)?;
+
+ let mut name = KVec::with_capacity(name_bytes.len() + 1, GFP_KERNEL)?;
+ name.extend_from_slice(name_bytes, GFP_KERNEL)?;
+ name.push(0, GFP_KERNEL)?;
+
+ let _name = CStr::from_bytes_with_nul(&name)
+ .ok()
+ .and_then(|name| CString::try_from(name).ok());
+
+ let cache_mode = section_hdr.section_flags.cache_mode();
+ let mut vm_map_flags = VmMapFlags::empty();
+
+ if !section_hdr.section_flags.contains(SectionFlag::Write) {
+ vm_map_flags |= VmFlag::Readonly;
+ }
+ if !section_hdr.section_flags.contains(SectionFlag::Exec) {
+ vm_map_flags |= VmFlag::Noexec;
+ }
+ if cache_mode != SectionFlag::CacheModeCached.into() {
+ vm_map_flags |= VmFlag::Uncached;
+ }
+
+ Ok(Some(ParsedSection {
+ data_range: section_hdr.data.clone(),
+ va: section_hdr.va,
+ vm_map_flags,
+ }))
+ }
+}
+
+/// Firmware binary header containing version and size information.
+///
+/// The header is located at the beginning of the firmware binary and contains
+/// a magic value for validation, version information, and the total size of
+/// all structured headers that follow.
+#[expect(dead_code)]
+struct FirmwareHeader {
+ /// Magic value to check binary validity.
+ magic: u32,
+
+ /// Minor firmware version.
+ minor: u8,
+
+ /// Major firmware version.
+ major: u8,
+
+ /// Padding. Must be set to zero.
+ _padding1: u16,
+
+ /// Firmware version hash.
+ version_hash: u32,
+
+ /// Padding. Must be set to zero.
+ _padding2: u32,
+
+ /// Total size of all the structured data headers at beginning of firmware binary.
+ size: u32,
+}
+
+impl FirmwareHeader {
+ const FW_BINARY_MAGIC: u32 = 0xc3f13a6e;
+ const FW_BINARY_MAJOR_MAX: u8 = 0;
+
+ /// Reads and validates a firmware header from the cursor.
+ ///
+ /// Verifies the magic value, version compatibility, and padding fields.
+ fn new(cursor: &mut Cursor<'_>) -> Result<Self> {
+ let magic = cursor.read_u32()?;
+ if magic != Self::FW_BINARY_MAGIC {
+ pr_err!("Invalid firmware magic");
+ return Err(EINVAL);
+ }
+
+ let minor = cursor.read_u8()?;
+ let major = cursor.read_u8()?;
+
+ if major > Self::FW_BINARY_MAJOR_MAX {
+ pr_err!(
+ "Unsupported firmware binary header version {}.{} (expected {}.x)\n",
+ major,
+ minor,
+ Self::FW_BINARY_MAJOR_MAX
+ );
+ return Err(EINVAL);
+ }
+
+ let padding1 = cursor.read_u16()?;
+ let version_hash = cursor.read_u32()?;
+ let padding2 = cursor.read_u32()?;
+ let size = cursor.read_u32()?;
+
+ if padding1 != 0 || padding2 != 0 {
+ pr_err!("Invalid firmware file: header padding is not zero");
+ return Err(EINVAL);
+ }
+
+ let fw_header = Self {
+ magic,
+ minor,
+ major,
+ _padding1: padding1,
+ version_hash,
+ _padding2: padding2,
+ size,
+ };
+
+ Ok(fw_header)
+ }
+}
+
+/// Firmware section header for loading binary sections into MCU memory.
+#[derive(Debug)]
+struct SectionHeader {
+ section_flags: SectionFlags,
+ /// MCU virtual range to map this binary section to.
+ va: Range<u32>,
+ /// References the data in the FW binary.
+ data: Range<u32>,
+}
+
+impl SectionHeader {
+ /// Reads and validates a section header from the cursor.
+ ///
+ /// Parses section flags, virtual address range, and data range from the firmware binary.
+ fn new(cursor: &mut Cursor<'_>) -> Result<Self> {
+ let section_flags = cursor.read_u32()?;
+ let section_flags = SectionFlags::try_from(section_flags)?;
+
+ let va_start = cursor.read_u32()?;
+ let va_end = cursor.read_u32()?;
+
+ let va = va_start..va_end;
+
+ if va.is_empty() {
+ pr_err!(
+ "Invalid firmware file: empty VA range at pos {}\n",
+ cursor.pos(),
+ );
+ return Err(EINVAL);
+ }
+
+ let data_start = cursor.read_u32()?;
+ let data_end = cursor.read_u32()?;
+ let data = data_start..data_end;
+
+ Ok(Self {
+ section_flags,
+ va,
+ data,
+ })
+ }
+}
+
+/// A firmware entry containing a header and optional parsed section data.
+///
+/// Represents a single entry in the firmware binary, which may contain loadable
+/// section data or metadata that doesn't require loading.
+struct EntrySection {
+ entry_hdr: EntryHeader,
+ inner: Option<ParsedSection>,
+}
+
+/// Header for a firmware entry, packed into a single u32.
+///
+/// The entry header encodes the entry type, size, and optional flag in a
+/// 32-bit value with the following layout:
+/// - Bits 0-7: Entry type
+/// - Bits 8-15: Size in bytes
+/// - Bit 31: Optional flag
+struct EntryHeader(u32);
+
+impl EntryHeader {
+ fn entry_type_raw(&self) -> u8 {
+ (self.0 & 0xff) as u8
+ }
+
+ fn entry_type(&self) -> Result<EntryType> {
+ let v = self.entry_type_raw();
+ EntryType::try_from(v)
+ }
+
+ fn optional(&self) -> bool {
+ self.0 & bit_u32(31) != 0
+ }
+
+ fn size(&self) -> u32 {
+ self.0 >> 8 & 0xff
+ }
+}
+
+#[derive(Clone, Copy, Debug)]
+#[repr(u8)]
+enum EntryType {
+ /// Host <-> FW interface.
+ Iface = 0,
+ /// FW config.
+ Config = 1,
+ /// Unit tests.
+ FutfTest = 2,
+ /// Trace buffer interface.
+ TraceBuffer = 3,
+ /// Timeline metadata interface.
+ TimelineMetadata = 4,
+ /// Metadata about how the FW binary was built.
+ BuildInfoMetadata = 6,
+}
+
+impl TryFrom<u8> for EntryType {
+ type Error = Error;
+
+ fn try_from(value: u8) -> Result<Self, Self::Error> {
+ match value {
+ 0 => Ok(EntryType::Iface),
+ 1 => Ok(EntryType::Config),
+ 2 => Ok(EntryType::FutfTest),
+ 3 => Ok(EntryType::TraceBuffer),
+ 4 => Ok(EntryType::TimelineMetadata),
+ 6 => Ok(EntryType::BuildInfoMetadata),
+ _ => Err(EINVAL),
+ }
+ }
+}
--
2.52.0
^ permalink raw reply related [flat|nested] 26+ messages in thread* Claude review: drm/tyr: add parser for firmware binary
2026-03-02 23:24 ` [PATCH v2 11/12] drm/tyr: add parser for firmware binary Deborah Brouwer
@ 2026-03-03 2:48 ` Claude Code Review Bot
0 siblings, 0 replies; 26+ messages in thread
From: Claude Code Review Bot @ 2026-03-03 2:48 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
**Author:** Daniel Almeida / Beata Michalska / Boris Brezillon / Deborah Brouwer
The firmware parser is well-structured with a Cursor abstraction for sequential reading.
1. `EntryHeader::size()` extracts bits 8-15:
```rust
+ fn size(&self) -> u32 {
+ self.0 >> 8 & 0xff
+ }
```
This means the maximum entry size is 255 bytes. This matches the C panthor driver's `CSF_FW_BINARY_ENTRY_SIZE` extraction. Worth a comment documenting this limit.
2. The `Cursor::view()` method checks `range.start < self.pos`:
```rust
+ fn view(&self, range: Range<usize>) -> Result<Cursor<'_>> {
+ if range.start < self.pos || range.end > self.data.len() {
```
This prevents viewing data before the current position. The intent is to prevent re-reading already-consumed data. However, the comparison should probably be `range.start < self.pos` should be `range.start < 0` or removed — since `view()` is used with `self.cursor.pos()..self.cursor.pos() + section_hdr_size`, `range.start` always equals `self.pos`, so the check `range.start < self.pos` will never trigger. This is dead validation code. Not a bug, just unnecessary.
3. The section name is parsed and converted but stored in `_name` (unused):
```rust
+ let _name = CStr::from_bytes_with_nul(&name)
+ .ok()
+ .and_then(|name| CString::try_from(name).ok());
```
This is presumably for future use (logging, debugging). Fine to keep.
4. In `parse_entry()`, the error handling for unknown entry types has slightly convoluted logic:
```rust
+ entry_type => {
+ if entry_type.is_err() || !entry_section.entry_hdr.optional() {
+ if !entry_section.entry_hdr.optional() {
+ ...
+ Err(EINVAL)
+ } else {
+ Ok(entry_section)
+ }
+ } else {
+ Ok(entry_section)
+ }
+ }
```
The outer `if` checks `is_err() || !optional()`, then the inner `if` checks `!optional()`. If `is_err()` is true AND `optional()` is true, the outer if is true but the inner if is false, falling through to `Ok(entry_section)`. This is logically correct (unknown-but-optional entries are silently skipped), but the nesting is harder to follow than it needs to be. Could be simplified to:
```rust
if !entry_section.entry_hdr.optional() {
pr_err!(...);
Err(EINVAL)
} else {
Ok(entry_section)
}
```
Since if it's optional, we skip it regardless of whether the type is recognized.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 12/12] drm/tyr: add firmware loading and MCU boot support
2026-03-02 23:24 [PATCH v2 0/12] drm/tyr: firmware loading and MCU boot support Deborah Brouwer
` (10 preceding siblings ...)
2026-03-02 23:24 ` [PATCH v2 11/12] drm/tyr: add parser for firmware binary Deborah Brouwer
@ 2026-03-02 23:25 ` Deborah Brouwer
2026-03-03 2:48 ` Claude review: " Claude Code Review Bot
2026-03-03 2:48 ` Claude review: drm/tyr: " Claude Code Review Bot
12 siblings, 1 reply; 26+ messages in thread
From: Deborah Brouwer @ 2026-03-02 23:25 UTC (permalink / raw)
To: dri-devel, rust-for-linux
Cc: daniel.almeida, aliceryhl, boris.brezillon, beata.michalska,
lyude, work, Deborah Brouwer
Add firmware loading and management for the Mali CSF GPU. This introduces
the fw module that loads the Mali GPU firmware binary, parses it into
sections, and maps those sections into the MCU VM at the required
virtual addresses.
On probe, the firmware is loaded, its sections are mapped and populated,
the MCU VM is activated, and the MCU is booted.
Co-developed-by: Boris Brezillon <boris.brezillon@collabora.com>
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
Signed-off-by: Deborah Brouwer <deborah.brouwer@collabora.com>
---
Changes in v2:
- Add missing RUST_FW_LOADER_ABSTRACTIONS.
- Set explicit allocation strategy option for kernel buffer objects.
- Stop using KBox for fw sections.
- Add bounds check for firmware copying.
drivers/gpu/drm/tyr/Kconfig | 1 +
drivers/gpu/drm/tyr/driver.rs | 16 +-
drivers/gpu/drm/tyr/fw.rs | 270 ++++++++++++++++++++++++++++++++++
drivers/gpu/drm/tyr/gem.rs | 3 -
drivers/gpu/drm/tyr/mmu.rs | 1 -
drivers/gpu/drm/tyr/slot.rs | 1 -
drivers/gpu/drm/tyr/tyr.rs | 1 +
drivers/gpu/drm/tyr/vm.rs | 1 -
8 files changed, 287 insertions(+), 7 deletions(-)
create mode 100644 drivers/gpu/drm/tyr/fw.rs
diff --git a/drivers/gpu/drm/tyr/Kconfig b/drivers/gpu/drm/tyr/Kconfig
index e5db8fa5dd57..14da410f0d84 100644
--- a/drivers/gpu/drm/tyr/Kconfig
+++ b/drivers/gpu/drm/tyr/Kconfig
@@ -18,6 +18,7 @@ config DRM_TYR
depends on MMU
select DRM_TYR_STATIC_DEPS
select IOMMU_IO_PGTABLE_LPAE
+ select RUST_FW_LOADER_ABSTRACTIONS
depends on IOMMU_SUPPORT
default n
help
diff --git a/drivers/gpu/drm/tyr/driver.rs b/drivers/gpu/drm/tyr/driver.rs
index 7174ab2fd011..7bbdeb75740c 100644
--- a/drivers/gpu/drm/tyr/driver.rs
+++ b/drivers/gpu/drm/tyr/driver.rs
@@ -40,6 +40,7 @@
use crate::{
file::TyrDrmFileData,
+ fw::Firmware,
gem::BoData,
gpu,
gpu::GpuInfo,
@@ -63,6 +64,8 @@ pub(crate) struct TyrPlatformDriverData {
pub(crate) struct TyrDrmDeviceData {
pub(crate) pdev: ARef<platform::Device>,
+ pub(crate) fw: Arc<Firmware>,
+
#[pin]
clks: Mutex<Clocks>,
@@ -147,10 +150,21 @@ fn probe(
let uninit_ddev = UnregisteredDevice::<TyrDrmDriver>::new(pdev.as_ref())?;
let platform: ARef<platform::Device> = pdev.into();
- let _mmu = Mmu::new(pdev, iomem.as_arc_borrow(), &gpu_info)?;
+ let mmu = Mmu::new(pdev, iomem.as_arc_borrow(), &gpu_info)?;
+
+ let firmware = Firmware::new(
+ pdev,
+ iomem.as_arc_borrow(),
+ &uninit_ddev,
+ mmu.as_arc_borrow(),
+ &gpu_info,
+ )?;
+
+ firmware.boot()?;
let data = try_pin_init!(TyrDrmDeviceData {
pdev: platform.clone(),
+ fw: firmware,
clks <- new_mutex!(Clocks {
core: core_clk,
stacks: stacks_clk,
diff --git a/drivers/gpu/drm/tyr/fw.rs b/drivers/gpu/drm/tyr/fw.rs
new file mode 100644
index 000000000000..b62e5ed69c4d
--- /dev/null
+++ b/drivers/gpu/drm/tyr/fw.rs
@@ -0,0 +1,270 @@
+// SPDX-License-Identifier: GPL-2.0 or MIT
+
+//! Firmware loading and management for Mali CSF GPU.
+//!
+//! This module handles loading the Mali GPU firmware binary, parsing it into sections,
+//! and mapping those sections into the MCU's virtual address space. Each firmware section
+//! has specific properties (read/write/execute permissions, cache modes) and must be loaded
+//! at specific virtual addresses expected by the MCU.
+//!
+//! See [`Firmware`] for the main firmware management interface and [`Section`] for
+//! individual firmware sections.
+//!
+//! [`Firmware`]: crate::fw::Firmware
+//! [`Section`]: crate::fw::Section
+
+use kernel::{
+ bits::genmask_u32,
+ devres::Devres,
+ drm::{
+ gem::BaseObject,
+ Uninit, //
+ },
+ impl_flags,
+ io::{
+ poll,
+ Io, //
+ },
+ platform,
+ prelude::*,
+ str::CString,
+ sync::{
+ Arc,
+ ArcBorrow, //
+ },
+ time,
+ types::ARef, //
+};
+
+use crate::{
+ driver::{
+ IoMem,
+ TyrDrmDevice, //
+ },
+ fw::parser::{
+ FwParser,
+ ParsedSection, //
+ },
+ gem,
+ gem::{
+ KernelBo,
+ KernelBoVaAlloc, //
+ },
+ gpu::{
+ GpuId,
+ GpuInfo, //
+ },
+ mmu::Mmu,
+ regs,
+ vm::Vm, //
+};
+
+mod parser;
+
+impl_flags!(
+ #[derive(Debug, Clone, Default, Copy, PartialEq, Eq)]
+ pub(super) struct SectionFlags(u32);
+
+ #[derive(Debug, Clone, Copy, PartialEq, Eq)]
+ pub(super) enum SectionFlag {
+ Read = 1 << 0,
+ Write = 1 << 1,
+ Exec = 1 << 2,
+ CacheModeNone = 0 << 3,
+ CacheModeCached = 1 << 3,
+ CacheModeUncachedCoherent = 2 << 3,
+ CacheModeCachedCoherent = 3 << 3,
+ Prot = 1 << 5,
+ Shared = 1 << 30,
+ Zero = 1 << 31,
+ }
+);
+
+pub(super) const CACHE_MODE_MASK: SectionFlags = SectionFlags(genmask_u32(3..=4));
+
+pub(super) const CSF_MCU_SHARED_REGION_START: u32 = 0x04000000;
+
+impl SectionFlags {
+ fn cache_mode(&self) -> SectionFlags {
+ *self & CACHE_MODE_MASK
+ }
+}
+
+impl TryFrom<u32> for SectionFlags {
+ type Error = Error;
+
+ fn try_from(value: u32) -> Result<Self, Self::Error> {
+ let valid_flags = SectionFlags::from(SectionFlag::Read)
+ | SectionFlags::from(SectionFlag::Write)
+ | SectionFlags::from(SectionFlag::Exec)
+ | CACHE_MODE_MASK
+ | SectionFlags::from(SectionFlag::Prot)
+ | SectionFlags::from(SectionFlag::Shared)
+ | SectionFlags::from(SectionFlag::Zero);
+
+ if value & valid_flags.0 != value {
+ Err(EINVAL)
+ } else {
+ Ok(Self(value))
+ }
+ }
+}
+
+/// A parsed section of the firmware binary.
+struct Section {
+ // Raw firmware section data for reset purposes
+ #[expect(dead_code)]
+ data: KVec<u8>,
+
+ // Keep the BO backing this firmware section so that both the
+ // GPU mapping and CPU mapping remain valid until the Section is dropped.
+ #[expect(dead_code)]
+ mem: gem::KernelBo,
+}
+
+/// Loaded firmware with sections mapped into MCU VM.
+pub(crate) struct Firmware {
+ /// Platform device reference (needed to access the MCU JOB_IRQ registers).
+ pdev: ARef<platform::Device>,
+
+ /// Iomem need to access registers.
+ iomem: Arc<Devres<IoMem>>,
+
+ /// MCU VM.
+ vm: Arc<Vm>,
+
+ /// List of firmware sections.
+ #[expect(dead_code)]
+ sections: KVec<Section>,
+}
+
+impl Drop for Firmware {
+ fn drop(&mut self) {
+ // AS slots retain a VM ref, we need to kill the circular ref manually.
+ self.vm.kill();
+ }
+}
+
+impl Firmware {
+ fn init_section_mem(mem: &mut KernelBo, data: &KVec<u8>) -> Result {
+ if data.is_empty() {
+ return Ok(());
+ }
+
+ let vmap = mem.bo.vmap::<0>()?;
+ let size = mem.bo.size();
+
+ if data.len() > size {
+ pr_err!("fw section {} bigger than BO {}\n", data.len(), size);
+ return Err(EINVAL);
+ }
+
+ for (i, &byte) in data.iter().enumerate() {
+ vmap.try_write8(byte, i)?;
+ }
+
+ Ok(())
+ }
+
+ fn request(
+ ddev: &TyrDrmDevice<Uninit>,
+ gpu_info: &GpuInfo,
+ ) -> Result<kernel::firmware::Firmware> {
+ let gpu_id = GpuId::from(gpu_info.gpu_id);
+
+ let path = CString::try_from_fmt(fmt!(
+ "arm/mali/arch{}.{}/mali_csffw.bin",
+ gpu_id.arch_major,
+ gpu_id.arch_minor
+ ))?;
+
+ kernel::firmware::Firmware::request(&path, ddev.as_ref())
+ }
+
+ fn load(
+ ddev: &TyrDrmDevice<Uninit>,
+ gpu_info: &GpuInfo,
+ ) -> Result<(kernel::firmware::Firmware, KVec<ParsedSection>)> {
+ let fw = Self::request(ddev, gpu_info)?;
+ let mut parser = FwParser::new(fw.data());
+
+ let parsed_sections = parser.parse()?;
+
+ Ok((fw, parsed_sections))
+ }
+
+ /// Load firmware and map sections into MCU VM.
+ pub(crate) fn new(
+ pdev: &platform::Device,
+ iomem: ArcBorrow<'_, Devres<IoMem>>,
+ ddev: &TyrDrmDevice<Uninit>,
+ mmu: ArcBorrow<'_, Mmu>,
+ gpu_info: &GpuInfo,
+ ) -> Result<Arc<Firmware>> {
+ let vm = Vm::new(pdev, ddev, mmu, gpu_info)?;
+
+ let (fw, parsed_sections) = Self::load(ddev, gpu_info)?;
+
+ vm.activate()?;
+
+ let mut sections = KVec::new();
+ for parsed in parsed_sections {
+ let size = (parsed.va.end - parsed.va.start) as usize;
+ let va = u64::from(parsed.va.start);
+
+ let mut mem = KernelBo::new(
+ ddev,
+ vm.as_arc_borrow(),
+ size.try_into().unwrap(),
+ KernelBoVaAlloc::Explicit(va),
+ parsed.vm_map_flags,
+ )?;
+
+ let section_start = parsed.data_range.start as usize;
+ let section_end = parsed.data_range.end as usize;
+ let mut data = KVec::new();
+
+ // Ensure that the firmware slice is not out of bounds.
+ let fw_data = fw.data();
+ let bytes = fw_data.get(section_start..section_end).ok_or(EINVAL)?;
+ data.extend_from_slice(bytes, GFP_KERNEL)?;
+
+ Self::init_section_mem(&mut mem, &data)?;
+
+ sections.push(Section { data, mem }, GFP_KERNEL)?;
+ }
+
+ let firmware = Arc::new(
+ Firmware {
+ pdev: pdev.into(),
+ iomem: iomem.into(),
+ vm,
+ sections,
+ },
+ GFP_KERNEL,
+ )?;
+
+ Ok(firmware)
+ }
+
+ pub(crate) fn boot(&self) -> Result {
+ // SAFETY: Boot is currently only called in the probe path, so we're sure we have a bound
+ // device.
+ let dev = unsafe { self.pdev.as_ref().as_bound() };
+
+ regs::MCU_CONTROL.write(dev, &self.iomem, regs::MCU_CONTROL_AUTO)?;
+
+ if let Err(e) = poll::read_poll_timeout(
+ || regs::MCU_STATUS.read(dev, &self.iomem),
+ |status| *status == regs::MCU_STATUS_ENABLED,
+ time::Delta::from_millis(1),
+ time::Delta::from_millis(100),
+ ) {
+ let status = regs::MCU_STATUS.read(dev, &self.iomem)?;
+ pr_err!("MCU failed to boot, status: {}", status);
+ return Err(e);
+ }
+
+ Ok(())
+ }
+}
diff --git a/drivers/gpu/drm/tyr/gem.rs b/drivers/gpu/drm/tyr/gem.rs
index f3bb58ef5eae..ed4eeb4e6d69 100644
--- a/drivers/gpu/drm/tyr/gem.rs
+++ b/drivers/gpu/drm/tyr/gem.rs
@@ -94,7 +94,6 @@ pub(crate) fn new_dummy_object<Ctx: DeviceContext>(ddev: &TyrDrmDevice<Ctx>) ->
/// a [`KernelBo`]. An automatic VA allocation strategy will be added in the future.
pub(crate) enum KernelBoVaAlloc {
/// Explicit VA address specified by the caller.
- #[expect(dead_code)]
Explicit(u64),
}
@@ -107,7 +106,6 @@ pub(crate) enum KernelBoVaAlloc {
/// When dropped, the buffer is automatically unmapped from the GPU VA space.
pub(crate) struct KernelBo {
/// The underlying GEM buffer object.
- #[expect(dead_code)]
pub(crate) bo: ARef<Bo>,
/// The GPU VM this buffer is mapped into.
vm: Arc<Vm>,
@@ -121,7 +119,6 @@ impl KernelBo {
/// This function allocates a new shmem-backed GEM object and immediately maps
/// it into the specified GPU virtual memory space. The mapping is automatically
/// cleaned up when the [`KernelBo`] is dropped.
- #[expect(dead_code)]
pub(crate) fn new<Ctx: DeviceContext>(
ddev: &TyrDrmDevice<Ctx>,
vm: ArcBorrow<'_, Vm>,
diff --git a/drivers/gpu/drm/tyr/mmu.rs b/drivers/gpu/drm/tyr/mmu.rs
index 2c7ac1fb1ac2..52a6bbbb179a 100644
--- a/drivers/gpu/drm/tyr/mmu.rs
+++ b/drivers/gpu/drm/tyr/mmu.rs
@@ -12,7 +12,6 @@
//!
//! [`AddressSpaceManager`]: address_space::AddressSpaceManager
//! [`SlotManager`]: crate::slot::SlotManager
-#![allow(dead_code)]
use core::ops::Range;
diff --git a/drivers/gpu/drm/tyr/slot.rs b/drivers/gpu/drm/tyr/slot.rs
index debba75f6204..53abb9eeb970 100644
--- a/drivers/gpu/drm/tyr/slot.rs
+++ b/drivers/gpu/drm/tyr/slot.rs
@@ -20,7 +20,6 @@
//!
//! [SlotOperations]: crate::slot::SlotOperations
//! [SlotManager]: crate::slot::SlotManager
-#![allow(dead_code)]
use core::{
mem::take,
diff --git a/drivers/gpu/drm/tyr/tyr.rs b/drivers/gpu/drm/tyr/tyr.rs
index b3244670dd79..18b0668bb217 100644
--- a/drivers/gpu/drm/tyr/tyr.rs
+++ b/drivers/gpu/drm/tyr/tyr.rs
@@ -9,6 +9,7 @@
mod driver;
mod file;
+mod fw;
mod gem;
mod gpu;
mod mmu;
diff --git a/drivers/gpu/drm/tyr/vm.rs b/drivers/gpu/drm/tyr/vm.rs
index f19acebdd66b..936c3049812b 100644
--- a/drivers/gpu/drm/tyr/vm.rs
+++ b/drivers/gpu/drm/tyr/vm.rs
@@ -6,7 +6,6 @@
//! the illusion of owning the entire virtual address (VA) range, similar to CPU virtual memory.
//! Each virtual memory (VM) area is backed by ARM64 LPAE Stage 1 page tables and can be
//! mapped into hardware address space (AS) slots for GPU execution.
-#![allow(dead_code)]
use core::ops::Range;
--
2.52.0
^ permalink raw reply related [flat|nested] 26+ messages in thread* Claude review: drm/tyr: add firmware loading and MCU boot support
2026-03-02 23:25 ` [PATCH v2 12/12] drm/tyr: add firmware loading and MCU boot support Deborah Brouwer
@ 2026-03-03 2:48 ` Claude Code Review Bot
0 siblings, 0 replies; 26+ messages in thread
From: Claude Code Review Bot @ 2026-03-03 2:48 UTC (permalink / raw)
To: dri-devel-reviews
Patch Review
**Author:** Deborah Brouwer / Boris Brezillon
This ties everything together: loads firmware, creates MCU VM, maps firmware sections, boots the MCU.
1. The `Firmware::Drop` breaks a circular reference:
```rust
+impl Drop for Firmware {
+ fn drop(&mut self) {
+ self.vm.kill();
+ }
+}
```
This is because the VM -> VmAsData -> Seat -> SlotManager -> SlotData (Arc<VmAsData>) creates a cycle. The `kill()` method deactivates the VM (evicting from slot manager, dropping the Arc<VmAsData>) and then unmaps all regions. This is a necessary but somewhat fragile pattern — if `kill()` fails, the Arc cycle leaks. The error logging in `kill()` mitigates debuggability.
2. `init_section_mem()` writes firmware data byte-by-byte:
```rust
+ for (i, &byte) in data.iter().enumerate() {
+ vmap.try_write8(byte, i)?;
+ }
```
This is functional but slow for large firmware sections. A `memcpy_to_vmap()` or similar bulk operation would be more efficient, but this is a correctness-first approach that can be optimized later.
3. The MCU boot polling has reasonable timeouts:
```rust
+ if let Err(e) = poll::read_poll_timeout(
+ || regs::MCU_STATUS.read(dev, &self.iomem),
+ |status| *status == regs::MCU_STATUS_ENABLED,
+ time::Delta::from_millis(1),
+ time::Delta::from_millis(100),
+ ) {
```
1ms polling interval, 100ms total timeout. This matches the C panthor driver's `panthor_fw_start()` timeouts.
4. The `SectionFlag::CacheModeNone = 0 << 3` is zero, which means `SectionFlags::empty()` and `SectionFlag::CacheModeNone` are indistinguishable. This is by design (the zero cache mode is "none"), but the `impl_flags!` macro may not handle zero-valued flags correctly in `contains()` checks. However, the actual usage only checks `cache_mode != SectionFlag::CacheModeCached.into()`, which works correctly since cached mode is non-zero.
5. The firmware path construction looks correct:
```rust
+ let path = CString::try_from_fmt(fmt!(
+ "arm/mali/arch{}.{}/mali_csffw.bin",
+ gpu_id.arch_major,
+ gpu_id.arch_minor
+ ))?;
```
This matches the C panthor driver's firmware path format.
6. In the probe path, `firmware.boot()` is called before the DRM device is registered (before `try_pin_init!(TyrDrmDeviceData {...})`). This means if boot fails, cleanup is straightforward (no registered device to unregister). Good ordering.
Overall, this is a solid v2 series. The most actionable items are the `AsRegister::new()` validation gap and the slightly convoluted error handling in the firmware parser's `parse_entry()`.
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 26+ messages in thread
* Claude review: drm/tyr: firmware loading and MCU boot support
2026-03-02 23:24 [PATCH v2 0/12] drm/tyr: firmware loading and MCU boot support Deborah Brouwer
` (11 preceding siblings ...)
2026-03-02 23:25 ` [PATCH v2 12/12] drm/tyr: add firmware loading and MCU boot support Deborah Brouwer
@ 2026-03-03 2:48 ` Claude Code Review Bot
12 siblings, 0 replies; 26+ messages in thread
From: Claude Code Review Bot @ 2026-03-03 2:48 UTC (permalink / raw)
To: dri-devel-reviews
Overall Series Review
Subject: drm/tyr: firmware loading and MCU boot support
Author: Deborah Brouwer <deborah.brouwer@collabora.com>
Patches: 13
Reviewed: 2026-03-03T12:48:17.996827
---
This is a well-structured v2 series that adds firmware loading and MCU boot support to the Tyr Rust DRM driver for ARM Mali CSF GPUs. The series progresses logically: Kconfig fixes, small cleanups/renames, register definitions, then the major new subsystems (slot manager, MMU, VM, GEM kernel BOs, firmware parser, firmware loading). The code quality is generally high with good documentation and idiomatic Rust patterns. The LockedBy pattern for slot/seat synchronization is well thought out.
Several patches already carry Reviewed-by tags from Alice Ryhl, Boris Brezillon, and Daniel Almeida, indicating significant prior review.
Key concerns:
1. The `AsRegister::new()` never validates `as_nr`, making it possible to compute an out-of-bounds MMIO offset. Validation only happens later in `validate_as_slot()`, but some call paths may skip it.
2. The `idle_slot()` method reads the seat via `access()` then immediately does `access_mut()` on the same `LockedBy`, which copies the seat info but in a somewhat roundabout way.
3. The firmware parser's `EntryHeader::size()` field extraction (bits 8-15) only captures an 8-bit size, limiting entries to 255 bytes. This matches the C panthor driver but deserves a comment.
4. The `Firmware::Drop` calling `self.vm.kill()` to break a circular Arc reference is a known pattern but worth flagging for potential future refactoring.
---
---
Generated by Claude Code Patch Reviewer
^ permalink raw reply [flat|nested] 26+ messages in thread