Message-ID: <a8eeccb7-9586-440f-a12a-e877a9197652@nvidia.com>
Date: Wed, 22 Oct 2025 13:48:43 -0400
From: Joel Fernandes <joelagnelf@...dia.com>
To: John Hubbard <jhubbard@...dia.com>, linux-kernel@...r.kernel.org,
 rust-for-linux@...r.kernel.org, dri-devel@...ts.freedesktop.org,
 dakr@...nel.org, acourbot@...dia.com
Cc: Alistair Popple <apopple@...dia.com>, Miguel Ojeda <ojeda@...nel.org>,
 Alex Gaynor <alex.gaynor@...il.com>, Boqun Feng <boqun.feng@...il.com>,
 Gary Guo <gary@...yguo.net>, bjorn3_gh@...tonmail.com,
 Benno Lossin <lossin@...nel.org>, Andreas Hindborg <a.hindborg@...nel.org>,
 Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
 David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
 Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
 Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
 Timur Tabi <ttabi@...dia.com>, joel@...lfernandes.org,
 Elle Rhumsaa <elle@...thered-steel.dev>,
 Daniel Almeida <daniel.almeida@...labora.com>, nouveau@...ts.freedesktop.org
Subject: Re: [PATCH 6/7] nova-core: mm: Add support to use PRAMIN windows to
 write to VRAM

Hi John,

On 10/21/2025 10:18 PM, John Hubbard wrote:
[...]
> First of all, I'd like very much for this patch to be at the beginning
> of a new patchset, that includes calling code that actually uses this
> pramin functionality.

There's no code in this patchset that uses pramin functionality though, so not
sure what you mean? This is a prerequisite patch for mm as mentioned in the
cover letter.

>> diff --git a/drivers/gpu/nova-core/mm/mod.rs b/drivers/gpu/nova-core/mm/mod.rs
>> new file mode 100644
>> index 000000000000..54c7cd9416a9
>> --- /dev/null
>> +++ b/drivers/gpu/nova-core/mm/mod.rs
>> @@ -0,0 +1,3 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +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..4f4e1b8c0b9b
>> --- /dev/null
>> +++ b/drivers/gpu/nova-core/mm/pramin.rs
>> @@ -0,0 +1,241 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +//! Direct VRAM access through PRAMIN window before page tables are set up.
> 
> Let's omit "before page tables are set up", because that's an assumption
> about the caller. In fact, this can be used at any time. (As an aside,
> various diag tools use this HW feature to peek/poke at vidmem, as you
> might expect.)

Ok.

> 
>> +//! PRAMIN can also write to system memory, however for simplicty we only
>> +//! support VRAM access.
> 
> Actually, I'd like to say something a bit different:
> 
> For Hopper/Blackwell and later GPUs, PRAMIN can only access VRAM.
> For earlier GPUs, sysmem was technically supported, but in practice
> never used in production drivers.

Ah, will clarify.

> 
> This is why Nova only provides VRAM access: Nova supports only a few
> of those older GPU generations (Turing, Ampere, and Ada), and there is
> no use case even for those few GPUs, for attempting sysmem access
> via PRAMIN.

Makes sense. Plus we can also access sysmem from BAR1 if needed.

>> +//!
>> +//! # Examples
>> +//!
>> +//! ## Writing u32 data to VRAM
>> +//!
>> +//! ```no_run
>> +//! use crate::driver::Bar0;
>> +//! use crate::mm::pramin::PraminVram;
>> +//!
>> +//! fn write_data_to_vram(bar: &Bar0) -> Result {
>> +//!     let pramin = PraminVram::new(bar);
>> +//!     // Write 4 32-bit words to VRAM at offset 0x10000
>> +//!     let data: [u32; 4] = [0xDEADBEEF, 0xCAFEBABE, 0x12345678, 0x87654321];
>> +//!     pramin.write::<u32>(0x10000, &data)?;
>> +//!     Ok(())
>> +//! }
>> +//! ```
>> +//!
>> +//! ## Reading bytes from VRAM
>> +//!
>> +//! ```no_run
>> +//! use crate::driver::Bar0;
>> +//! use crate::mm::pramin::PraminVram;
>> +//!
>> +//! fn read_data_from_vram(bar: &Bar0, buffer: &mut KVec<u8>) -> Result {
>> +//!     let pramin = PraminVram::new(bar);
>> +//!     // Read a u8 from VRAM starting at offset 0x20000
>> +//!     pramin.read::<u8>(0x20000, buffer)?;
>> +//!     Ok(())
>> +//! }
>> +//! ```
>> +
>> +#![expect(dead_code)]
>> +
>> +use crate::driver::Bar0;
>> +use crate::regs;
>> +use core::mem;
>> +use kernel::prelude::*;
>> +
>> +/// PRAMIN is a window into the VRAM (not a hardware block) that is used to access
>> +/// the VRAM directly. These addresses are consistent across all GPUs.
>> +const PRAMIN_BASE: usize = 0x700000; // PRAMIN is always at BAR0 + 0x700000
>> +const PRAMIN_SIZE: usize = 0x100000; // 1MB aperture - max access per window position
>> +
>> +/// Trait for types that can be read/written through PRAMIN.
>> +pub(crate) trait PraminNum: Copy + Default + Sized {
>> +    fn read_from_bar(bar: &Bar0, offset: usize) -> Result<Self>;
>> +
>> +    fn write_to_bar(self, bar: &Bar0, offset: usize) -> Result;
>> +
>> +    fn size_bytes() -> usize {
>> +        mem::size_of::<Self>()
>> +    }
>> +
>> +    fn alignment() -> usize {
>> +        Self::size_bytes()
>> +    }
>> +}
>> +
>> +/// Macro to implement PraminNum trait for unsigned integer types.
>> +macro_rules! impl_pramin_unsigned_num {
>> +    ($bits:literal) => {
>> +        ::kernel::macros::paste! {
>> +            impl PraminNum for [<u $bits>] {
>> +                fn read_from_bar(bar: &Bar0, offset: usize) -> Result<Self> {
>> +                    bar.[<try_read $bits>](offset)
>> +                }
>> +
>> +                fn write_to_bar(self, bar: &Bar0, offset: usize) -> Result {
>> +                    bar.[<try_write $bits>](self, offset)
>> +                }
>> +            }
>> +        }
>> +    };
>> +}
>> +
>> +impl_pramin_unsigned_num!(8);
>> +impl_pramin_unsigned_num!(16);
>> +impl_pramin_unsigned_num!(32);
>> +impl_pramin_unsigned_num!(64);
>> +
>> +/// Direct VRAM access through PRAMIN window before page tables are set up.
>> +pub(crate) struct PraminVram<'a> {
>> +    bar: &'a Bar0,
>> +    saved_window_addr: usize,
>> +}
>> +
>> +impl<'a> PraminVram<'a> {
>> +    /// Create a new PRAMIN VRAM accessor, saving current window state,
>> +    /// the state is restored when the accessor is dropped.
>> +    ///
>> +    /// The BAR0 window base must be 64KB aligned but provides 1MB of VRAM access.
>> +    /// Window is repositioned automatically when accessing data beyond 1MB boundaries.
>> +    pub(crate) fn new(bar: &'a Bar0) -> Self {
>> +        let saved_window_addr = Self::get_window_addr(bar);
>> +        Self {
>> +            bar,
>> +            saved_window_addr,
>> +        }
>> +    }
>> +
>> +    /// Set BAR0 window to point to specific FB region.
>> +    ///
>> +    /// # Arguments
>> +    ///
>> +    /// * `fb_offset` - VRAM byte offset where the window should be positioned.
>> +    ///                 Must be 64KB aligned (lower 16 bits zero).
>> +    fn set_window_addr(&self, fb_offset: usize) -> Result {
>> +        // FB offset must be 64KB aligned (hardware requirement for window_base field)
>> +        // Once positioned, the window provides access to 1MB of VRAM through PRAMIN aperture
>> +        if fb_offset & 0xFFFF != 0 {
>> +            return Err(EINVAL);
>> +        }
>> +
>> +        let window_reg = regs::NV_PBUS_BAR0_WINDOW::default().set_window_addr(fb_offset);
>> +        window_reg.write(self.bar);
>> +
>> +        // Read back to ensure it took effect
>> +        let readback = regs::NV_PBUS_BAR0_WINDOW::read(self.bar);
>> +        if readback.window_base() != window_reg.window_base() {
>> +            return Err(EIO);
>> +        }
> 
> Let's not "read it back to ensure it took effect". This is not required
> by the hardware.

Ok, it was more for my ascertaining that the write to effect, but you're right
its not necessary so I'll remove it.

>> +        Ok(())
>> +    }
>> +
>> +    /// Get current BAR0 window offset.
>> +    ///
>> +    /// # Returns
>> +    ///
>> +    /// The byte offset in VRAM where the PRAMIN window is currently positioned.
>> +    /// This offset is always 64KB aligned.
>> +    fn get_window_addr(bar: &Bar0) -> usize {
>> +        let window_reg = regs::NV_PBUS_BAR0_WINDOW::read(bar);
>> +        window_reg.get_window_addr()
>> +    }
>> +
>> +    /// Common logic for accessing VRAM data through PRAMIN with windowing.
>> +    ///
>> +    /// # Arguments
>> +    ///
>> +    /// * `fb_offset` - Starting byte offset in VRAM (framebuffer) where access begins.
>> +    ///                 Must be aligned to `T::alignment()`.
>> +    /// * `num_items` - Number of items of type `T` to process.
>> +    /// * `operation` - Closure called for each item to perform the actual read/write.
>> +    ///                 Takes two parameters:
>> +    ///                 - `data_idx`: Index of the item in the data array (0..num_items)
>> +    ///                 - `pramin_offset`: BAR0 offset in the PRAMIN aperture to access
>> +    ///
>> +    /// The function automatically handles PRAMIN window repositioning when accessing
>> +    /// data that spans multiple 1MB windows.
>> +    fn access_vram<T: PraminNum, F>(
>> +        &self,
>> +        fb_offset: usize,
>> +        num_items: usize,
>> +        mut operation: F,
>> +    ) -> Result
> 
> This is far too much functionality, and the code can be made much smaller
> and simpler.
> and still get what we need. Open RM only supplies small accessors
> (8 thru 64 bits wide), and no "bulk access". The calling code can loop if 
> necessary.

The code uses a sliding window approach to reposition the moving window,
abstracting away the details of the moving window from the caller. That
simplifies the callers a lot as they don't need to "loop" and know when to move
the window when they hit limits. They can also write to greater than 1MB. The
bulk of the logic is in this function and the surrounding code is mostly
wrappers, which part is complicated or that you did not understand?

Just to note also, the PRAMIN moving window functionality in this patch allows
us to not need BAR2 to access VRAM for instance memory. That is a code
simplification then as we do not need code for BAR2 (the tradeoff being slightly
slower instance memory access). I confirmed with the team that this is also an
option. Abstracting the sliding window functionality becomes important then, so
I'd not vote for removing this functionality for that reason. And if we ever use
BAR2, having it is still useful because it allows us to have a fallback too for
comparison/reference.

> 
> We should do likewise, and avoid this.
> 
> Then we can just create things such as write_u32() or write<u32>(), etc.
> 
> And do we even *need* read?? I'm not sure we do.

We do need reads as we walk through page tables structures. Note that the page
tables are partially allocated by the GSP.

> 
> This is hopefully showing the value of including the calling code, as
> a follow-on patch in the series.

Unfortunately, there are too many dependencies as I mentioned in the cover
letter, so I would like to get functionality merged in stages. That's the
best way to make good progress IMO for nova-core. Of course, we have to careful
about design etc and I kept it as simple as possible out of that intention. My
pramin patch was written 3-4 months ago now, so I'd like to not keep it too
sitting comfortably in my tree. :). And this patch is critical for mm.

That said, I'll go look at the usecases and APIs again and see if I can simplify
anything more.

thanks,

 - Joel

Powered by blists - more mailing lists