Re: [PATCH v8 06/25] gpu: nova-core: mm: Add support to use PRAMIN windows to write to VRAM

["Alexandre Courbot" <acourbot@nvidia.com>]

Hi Joel,

On Wed Feb 25, 2026 at 7:53 AM JST, Joel Fernandes wrote:
> PRAMIN apertures are a crucial mechanism to direct read/write to VRAM.
> Add support for the same.
>
> Cc: Nikola Djukic <ndjukic@nvidia.com>
> Signed-off-by: Joel Fernandes <joelagnelf@nvidia.com>

I have two sets of comments for this patch - one that is immediately
actionable, the other that depends on the availability of the new `Io`
interface. Let's start with the actionable items, I'll do the discussion
about `Io` in another email.

> ---
>  drivers/gpu/nova-core/mm.rs        |   5 +
>  drivers/gpu/nova-core/mm/pramin.rs | 292 +++++++++++++++++++++++++++++
>  drivers/gpu/nova-core/nova_core.rs |   1 +
>  drivers/gpu/nova-core/regs.rs      |   5 +
>  4 files changed, 303 insertions(+)
>  create mode 100644 drivers/gpu/nova-core/mm.rs
>  create mode 100644 drivers/gpu/nova-core/mm/pramin.rs
>
> diff --git a/drivers/gpu/nova-core/mm.rs b/drivers/gpu/nova-core/mm.rs
> new file mode 100644
> index 000000000000..7a5dd4220c67
> --- /dev/null
> +++ b/drivers/gpu/nova-core/mm.rs
> @@ -0,0 +1,5 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Memory management subsystems for nova-core.
> +
> +pub(crate) mod pramin;
> diff --git a/drivers/gpu/nova-core/mm/pramin.rs b/drivers/gpu/nova-core/mm/pramin.rs
> new file mode 100644
> index 000000000000..04b652d3ee4f
> --- /dev/null
> +++ b/drivers/gpu/nova-core/mm/pramin.rs
> @@ -0,0 +1,292 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +//! Direct VRAM access through the PRAMIN aperture.
> +//!
> +//! PRAMIN provides a 1MB sliding window into VRAM through BAR0, allowing the CPU to access
> +//! video memory directly. Access is managed through a two-level API:
> +//!
> +//! - [`Pramin`]: The parent object that owns the BAR0 reference and synchronization lock.
> +//! - [`PraminWindow`]: A guard object that holds exclusive PRAMIN access for its lifetime.
> +//!
> +//! The PRAMIN aperture is a 1MB region at BAR0 + 0x700000 for all GPUs. The window base is
> +//! controlled by the `NV_PBUS_BAR0_WINDOW` register and must be 64KB aligned.

s/must be/is - it's not like that hardware is giving us a choice anyway
since we cannot even express a non-aligned value in the window register.

> +//!
> +//! # Examples
> +//!
> +//! ## Basic read/write
> +//!
> +//! ```no_run
> +//! use crate::driver::Bar0;
> +//! use crate::mm::pramin;
> +//! use kernel::devres::Devres;
> +//! use kernel::prelude::*;
> +//! use kernel::sync::Arc;
> +//!
> +//! fn example(devres_bar: Arc<Devres<Bar0>>) -> Result<()> {
> +//!     let pramin = Arc::pin_init(pramin::Pramin::new(devres_bar)?, GFP_KERNEL)?;
> +//!     let mut window = pramin.window()?;
> +//!
> +//!     // Write and read back.
> +//!     window.try_write32(0x100, 0xDEADBEEF)?;
> +//!     let val = window.try_read32(0x100)?;
> +//!     assert_eq!(val, 0xDEADBEEF);
> +//!
> +//!     Ok(())
> +//!     // Original window position restored on drop.
> +//! }
> +//! ```
> +//!
> +//! ## Auto-repositioning across VRAM regions
> +//!
> +//! ```no_run
> +//! use crate::driver::Bar0;
> +//! use crate::mm::pramin;
> +//! use kernel::devres::Devres;
> +//! use kernel::prelude::*;
> +//! use kernel::sync::Arc;
> +//!
> +//! fn example(devres_bar: Arc<Devres<Bar0>>) -> Result<()> {
> +//!     let pramin = Arc::pin_init(pramin::Pramin::new(devres_bar)?, GFP_KERNEL)?;
> +//!     let mut window = pramin.window()?;
> +//!
> +//!     // Access first 1MB region.
> +//!     window.try_write32(0x100, 0x11111111)?;
> +//!
> +//!     // Access at 2MB - window auto-repositions.
> +//!     window.try_write32(0x200000, 0x22222222)?;
> +//!
> +//!     // Back to first region - window repositions again.
> +//!     let val = window.try_read32(0x100)?;
> +//!     assert_eq!(val, 0x11111111);
> +//!
> +//!     Ok(())
> +//! }
> +//! ```
> +
> +#![expect(unused)]
> +
> +use crate::{
> +    driver::Bar0,
> +    num::u64_as_usize,
> +    regs, //
> +};
> +
> +use kernel::bits::genmask_u64;
> +use kernel::devres::Devres;
> +use kernel::io::Io;
> +use kernel::new_mutex;
> +use kernel::prelude::*;
> +use kernel::ptr::{
> +    Alignable,
> +    Alignment, //
> +};
> +use kernel::sizes::{
> +    SZ_1M,
> +    SZ_64K, //
> +};
> +use kernel::sync::{
> +    lock::mutex::MutexGuard,
> +    Arc,
> +    Mutex, //
> +};
> +
> +/// PRAMIN aperture base offset in BAR0.
> +const PRAMIN_BASE: usize = 0x700000;
> +
> +/// PRAMIN aperture size (1MB).
> +const PRAMIN_SIZE: usize = SZ_1M;
> +
> +/// 64KB alignment for window base.
> +const WINDOW_ALIGN: Alignment = Alignment::new::<SZ_64K>();
> +
> +/// Maximum addressable VRAM offset (40-bit address space).
> +///
> +/// The `NV_PBUS_BAR0_WINDOW` register has a 24-bit `window_base` field (bits 23:0) that stores
> +/// bits [39:16] of the target VRAM address. This limits the addressable space to 2^40 bytes.
> +const MAX_VRAM_OFFSET: usize = u64_as_usize(genmask_u64(0..=39));
> +
> +/// Generate a PRAMIN read accessor.
> +macro_rules! define_pramin_read {
> +    ($name:ident, $ty:ty) => {
> +        #[doc = concat!("Read a `", stringify!($ty), "` from VRAM at the given offset.")]
> +        pub(crate) fn $name(&mut self, vram_offset: usize) -> Result<$ty> {
> +            // Compute window parameters without bar reference.
> +            let (bar_offset, new_base) =
> +                self.compute_window(vram_offset, ::core::mem::size_of::<$ty>())?;
> +
> +            // Update window base if needed and perform read.
> +            let bar = self.bar.try_access().ok_or(ENODEV)?;

Ouch, we are calling `try_access` for every single read or write
operation. Thankfully we can do without this - see my comments on
`PraminWindow` a bit later.

> +            if let Some(base) = new_base {
> +                Self::write_window_base(&bar, base);
> +                self.state.current_base = base;
> +            }
> +            bar.$name(bar_offset)
> +        }
> +    };
> +}
> +
> +/// Generate a PRAMIN write accessor.
> +macro_rules! define_pramin_write {
> +    ($name:ident, $ty:ty) => {
> +        #[doc = concat!("Write a `", stringify!($ty), "` to VRAM at the given offset.")]
> +        pub(crate) fn $name(&mut self, vram_offset: usize, value: $ty) -> Result {
> +            // Compute window parameters without bar reference.
> +            let (bar_offset, new_base) =
> +                self.compute_window(vram_offset, ::core::mem::size_of::<$ty>())?;
> +
> +            // Update window base if needed and perform write.
> +            let bar = self.bar.try_access().ok_or(ENODEV)?;
> +            if let Some(base) = new_base {
> +                Self::write_window_base(&bar, base);
> +                self.state.current_base = base;
> +            }
> +            bar.$name(value, bar_offset)
> +        }
> +    };
> +}
> +
> +/// PRAMIN state protected by mutex.
> +struct PraminState {
> +    current_base: usize,
> +}

This has only one member and no impl block of its own - shall we inline
it?

> +
> +/// PRAMIN aperture manager.
> +///
> +/// Call [`Pramin::window()`] to acquire exclusive PRAMIN access.
> +#[pin_data]
> +pub(crate) struct Pramin {
> +    bar: Arc<Devres<Bar0>>,
> +    /// PRAMIN aperture state, protected by a mutex.
> +    ///
> +    /// # Safety
> +    ///
> +    /// This lock is acquired during the DMA fence signalling critical path.

nit: s/signalling/signaling

> +    /// It must NEVER be held across any reclaimable CPU memory / allocations
> +    /// (`GFP_KERNEL`), because the memory reclaim path can call
> +    /// `dma_fence_wait()`, which would deadlock with this lock held.
> +    #[pin]
> +    state: Mutex<PraminState>,
> +}
> +
> +impl Pramin {
> +    /// Create a pin-initializer for PRAMIN.
> +    pub(crate) fn new(bar: Arc<Devres<Bar0>>) -> Result<impl PinInit<Self>> {
> +        let bar_access = bar.try_access().ok_or(ENODEV)?;
> +        let current_base = Self::try_read_window_base(&bar_access)?;
> +
> +        Ok(pin_init!(Self {
> +            bar,
> +            state <- new_mutex!(PraminState { current_base }, "pramin_state"),
> +        }))
> +    }
> +
> +    /// Acquire exclusive PRAMIN access.
> +    ///
> +    /// Returns a [`PraminWindow`] guard that provides VRAM read/write accessors.
> +    /// The [`PraminWindow`] is exclusive and only one can exist at a time.
> +    pub(crate) fn window(&self) -> Result<PraminWindow<'_>> {

The name of this method is strange - we don't pass the base of any
window area. It looks more like it is actually acquiring the `Pramin`
for exclusive access.

Also fun question: what happens if we try to access an area above (or
below) the available VRAM?

> +        let state = self.state.lock();
> +        let saved_base = state.current_base;
> +        Ok(PraminWindow {
> +            bar: self.bar.clone(),
> +            state,
> +            saved_base,
> +        })
> +    }
> +
> +    /// Read the current window base from the BAR0_WINDOW register.
> +    fn try_read_window_base(bar: &Bar0) -> Result<usize> {
> +        let reg = regs::NV_PBUS_BAR0_WINDOW::read(bar);
> +        let base = u64::from(reg.window_base());
> +        let shifted = base.checked_shl(16).ok_or(EOVERFLOW)?;
> +        shifted.try_into().map_err(|_| EOVERFLOW)

This function is actually infallible.

The `checked_shl` cannot fail because we are shifting a `u32` by 16,
which will always fit into a `u64`. There is some `Bounded` code
incoming that will allow us to prove that safely, but it is not
available in `drm-rust-next` yet. For now can you use a regular shift
operation with a TODO to convert to `Bounded`?

The last line could use `num::u64_as_size` to do the conversion
infallibly. But actually the window base is native to the GPU, not the
host CPU architecture - so it should really be a u64 anyway.

> +    }
> +}
> +
> +/// PRAMIN window guard for direct VRAM access.
> +///
> +/// This guard holds exclusive access to the PRAMIN aperture. The window auto-repositions
> +/// when accessing VRAM offsets outside the current 1MB range. Original window position
> +/// is saved on creation and restored on drop.
> +///
> +/// Only one [`PraminWindow`] can exist at a time per [`Pramin`] instance (enforced by the
> +/// internal `MutexGuard`).
> +pub(crate) struct PraminWindow<'a> {
> +    bar: Arc<Devres<Bar0>>,

Cloning the `Arc` is what forces us to do a `try_access` on every
read/write operation. Needless to say this is overkill and not
efficient.

The `PraminWindow` already holds a reference to its `Pramin`, so you can
just call `try_access` in `window` and store that. This turns `bar` into
this:

    bar: RevocableGuard<'a, Bar0>,

And now you can perform operations on `bar` directly without needing to
acquire it every time.

> +    state: MutexGuard<'a, PraminState>,

I'm wondering whether we should remove the `Mutex` from `Pramin` and
make its methods take `&mut self` (and this would thus become a `&mut
PraminState`).

The rationale for this is that `Pramin` will be owned by `GpuMm`, which
can implement its own locking policy (including across several of the
items it owns) as it needs. That way `Pramin` also stays "pure" in that
it only needs to care about abstracting the hardware.

> +    saved_base: usize,

What is the point of saving and restoring the original position?
`Pramin` gets exclusive access to the PRAMIN area, and whenever we make
an access through the window the base address can be reprogrammed. So
it's not as if the original value has some particular meaning or
constitutes state that deserves to be preserved. Getting rid of this
behavior also lets us remove the `Drop` implementation.

> +}
> +
> +impl PraminWindow<'_> {
> +    /// Write a new window base to the BAR0_WINDOW register.
> +    fn write_window_base(bar: &Bar0, base: usize) {

`base` should be the native type of the GPU, i.e. `u64`.

> +        // CAST:
> +        // - We have guaranteed that the base is within the addressable range (40-bits).
> +        // - After >> 16, a 40-bit aligned base becomes 24 bits, which fits in u32.

This function does not guarantee anything of the sort - even the caller
doesn't, this actually relies on the behavior of `compute_window`,
another method.

You can enforce this limit by using a `Bounded<u64, 40>` as the type for
base addresses. There is even staging code (not yet in drm-rust-next
unfortunately) that will lets you turn it into a `Bounded<u32, 32>`
safely to be written into the register.

> +        regs::NV_PBUS_BAR0_WINDOW::default()
> +            .set_window_base((base >> 16) as u32)
> +            .write(bar);
> +    }
> +
> +    /// Compute window parameters for a VRAM access.
> +    ///
> +    /// Returns (`bar_offset`, `new_base`) where:
> +    /// - `bar_offset`: The BAR0 offset to use for the access.
> +    /// - `new_base`: `Some(base)` if window needs repositioning, `None` otherwise.
> +    fn compute_window(
> +        &self,
> +        vram_offset: usize,
> +        access_size: usize,
> +    ) -> Result<(usize, Option<usize>)> {
> +        // Validate VRAM offset is within addressable range (40-bit address space).
> +        let end_offset = vram_offset.checked_add(access_size).ok_or(EINVAL)?;
> +        if end_offset > MAX_VRAM_OFFSET + 1 {

If we are going to use `MAX_VRAM_OFFSET` like this, let's define it as
`1 << 40` and turn this into `if end_offset > MAX_VRAM_OFFSET`.

> +            return Err(EINVAL);
> +        }
> +
> +        // Calculate which 64KB-aligned base we need.
> +        let needed_base = vram_offset.align_down(WINDOW_ALIGN);
> +
> +        // Calculate offset within the window.
> +        let offset_in_window = vram_offset - needed_base;
> +
> +        // Check if access fits in 1MB window from this base.
> +        if offset_in_window + access_size > PRAMIN_SIZE {
> +            return Err(EINVAL);
> +        }

Here `offset_in_window` cannot be larger than 64K because of the line
before - this check is effectively dead code.

> +
> +        // Return bar offset and whether window needs repositioning.
> +        let new_base = if self.state.current_base != needed_base {
> +            Some(needed_base)
> +        } else {
> +            None
> +        };

So maybe I am missing something, but aren't we writing a new base if the
requested offset doesn't fit within [current_base..current_base+64K],
despite the fact that the PRAMIN window is 1MB? This looks like a bug.

Also what happens if we access PRAMIN with decreasing addresses? Won't
we be doing more window resets than we need?

> +
> +        Ok((PRAMIN_BASE + offset_in_window, new_base))
> +    }
> +
> +    define_pramin_read!(try_read8, u8);
> +    define_pramin_read!(try_read16, u16);
> +    define_pramin_read!(try_read32, u32);
> +    define_pramin_read!(try_read64, u64);
> +
> +    define_pramin_write!(try_write8, u8);
> +    define_pramin_write!(try_write16, u16);
> +    define_pramin_write!(try_write32, u32);
> +    define_pramin_write!(try_write64, u64);
> +}
> +
> +impl Drop for PraminWindow<'_> {
> +    fn drop(&mut self) {
> +        // Restore the original window base if it changed.
> +        if self.state.current_base != self.saved_base {
> +            if let Some(bar) = self.bar.try_access() {
> +                Self::write_window_base(&bar, self.saved_base);
> +
> +                // Update state to reflect the restored base.
> +                self.state.current_base = self.saved_base;
> +            }
> +        }
> +        // MutexGuard drops automatically, releasing the lock.
> +    }
> +}

Let's drop this alongside the original window base restoration, which
doesn't serve any purpose.

> diff --git a/drivers/gpu/nova-core/nova_core.rs b/drivers/gpu/nova-core/nova_core.rs
> index c1121e7c64c5..3de00db3279e 100644
> --- a/drivers/gpu/nova-core/nova_core.rs
> +++ b/drivers/gpu/nova-core/nova_core.rs
> @@ -13,6 +13,7 @@
>  mod gfw;
>  mod gpu;
>  mod gsp;
> +mod mm;
>  mod num;
>  mod regs;
>  mod sbuffer;
> diff --git a/drivers/gpu/nova-core/regs.rs b/drivers/gpu/nova-core/regs.rs
> index ea0d32f5396c..d0982e346f74 100644
> --- a/drivers/gpu/nova-core/regs.rs
> +++ b/drivers/gpu/nova-core/regs.rs
> @@ -102,6 +102,11 @@ fn fmt(&self, f: &mut kernel::fmt::Formatter<'_>) -> kernel::fmt::Result {
>      31:16   frts_err_code as u16;
>  });
>  
> +register!(NV_PBUS_BAR0_WINDOW @ 0x00001700, "BAR0 window control for PRAMIN access" {
> +    25:24   target as u8, "Target memory (0=VRAM, 1=SYS_MEM_COH, 2=SYS_MEM_NONCOH)";

This should be converted to an enum. I understand that we only ever want
to use `Vram` as Hopper+ don't support the other types of memories - a
single-variant enum will document that fact and force us to set the
correct value.