Message-Id: <DDQIOKFEXK29.3FDIXOR8S1284@nvidia.com>
Date: Fri, 24 Oct 2025 20:31:13 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "Joel Fernandes" <joelagnelf@...dia.com>, "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

On Thu Oct 23, 2025 at 2:48 AM JST, Joel Fernandes wrote:
<snip>
>>> +        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.

Whether we want a sliding window mechanism or not, I think it is
valuable to expose the PRAMIN functionality the way the hardware
supports it (i.e. set base address and work with a fixed slice), and
then build QoL features like the sliding window on top of it, through
e.g. another type that wraps the basic PRAMIN one.

This would make the code easier to read, allow more flexibility for
users (although in the case of PRAMIN we might not really need it), and
matches what Rust does for e.g. `BufReader`, which consumes a basic reader
and provide buffering for it.

>
>> 
>> 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.

Although we have neglected it lately, we could use our
`nova-core-unstable` staging branch for that - IIRC the goal was also to
keep track of pending patches and make sure they don't bitrot until they
can be sent.

Powered by blists - more mailing lists