Re: [PATCH v12 04/22] gpu: nova-core: mm: Add support to use PRAMIN windows to write to VRAM

[Joel Fernandes <joelagnelf@nvidia.com>]

Hi Alex,

On Sun, 03 May 2026, Alexandre Courbot wrote:
> `Pramin::new` takes different arguments (also check the other examples).

Fixed.

> The HRT series [1] will allow you to greatly simplify all this by
> storing a `&'a Bar0` directly in this structure. It will most likely
> land this cycle, so I think it's a good idea to proactively depend on
> them. It should apply cleanly (modulo the Tyr patches IIRC, but you can
> skip them if you don't build the driver) on top of `drm-rust-next` - on
> top of which this should also be rebased anyway.
>
> [1] https://lore.kernel.org/all/20260427221155.2144848-1-dakr@kernel.org/

Actually, I was going in the direction of making Bar0, GpuMm and Bar1
use Arc instead of lifetimes initially. The reason is, some of these are
long lived references. Directly using `&'a Bar0` means the lifetimes
also gets threaded to Pramin and then threads to GpuMm which is long
lived in some of my later patches. Some of the cases where we will have
long lived mappings are also vGPU and channels. I spoke to Danilo about
this, and we discussed that an initial approach could just use Arc since
refcounts are cheap and it should be just as performant, then once we
have the design ironed out, we can migrate to using lifetimes in this
code where appropriate. Even before HRT, I already used lifetimes and
analyzed the tradeoffs.

> I don't think that checking the range in this type is particularly
> useful, because this abstraction cannot validate VRAM ownership
> completely: VRAM can be sparse, reserved, or protected, and with the
> current bound access to these invalid areas is not prevented.

I think there's value in keeping this. The range check is similar in
spirit to what `Bar0` accessors do -- bounds are checked, but a passing
check doesn't guarantee the IO will succeed at the hardware level (an
address in a valid range could still cause issues). That's not a reason
to remove the guard; it still catches obvious bugs early. Having a sanity
check that says "this address is at least within the declared VRAM
region" is more useful than having no check at all, even if ownership
semantics aren't enforced at this layer.

>
> PRAMIN should only model the hardware windowing mechanism; address
> validity belongs to the higher layers that allocate or obtain those
> addresses. IIUC nothing will explode if we try to read or write into
> areas that are not VRAM.
> The problem with this lock is that it is buried, alongside its usage
> guide, deep into this type, ensuring that users won't ever see it. It
> will create deadlocks rather than preventing them.

Having an internal lock isn't inherently problematic -- the GPU buddy
allocator bindings follow the same pattern, for example. Let us go by usecase,
lets not complicate design based on a hypothetical. I wouldn't move the mutex
out of Pramin just based on guessing and complicate/thread the mutable reference
to layers that don't need to handle that complexity.

Further, what concrete design or use case requires mutex to be external? I
wouldn't add that complexity based on hypotheticals -- we go by actual use
cases, and there are actual users of this code in the patches that follow. For
what it's worth, I did try the outer-mutex approach myself before you mentioned
it and decided against it for this and the buddy code based on the reasons
mentioned.

> And when you look closer at it, you realize it is actually acquired for
> any use of `Pramin`, since the only method it exposes is `get_window`,
> which acquires the lock, and returns a `PraminWindow` which has exactly
> the same layout as `Pramin`, except that the lock is acquired.
>
> So what's the point? Let's just make `get_window` require a `&mut self`
> and let the owner of `Pramin` decide how to manage concurrent accesses.
> At least the doccomment for the lock will be visible from a higher
> layer.

The point is to keep it simple. What you're suggesting requires the
caller to do: lock + get_window + access window. My design is get_window
+ access window -- you need the latter two anyway, so the locking is
absorbed without any extra burden. Propagating lock outward adds
complexity at every layer that calls into `Pramin`; handling the lock
internally, as done elsewhere in the codebase, avoids that.

> Currently `Pramin` mixes elements of hardware access, memory management,
> and synchronization into the same type, squashing what should be
> different layers into a single one. Now that the `Io` trait is merged
> and available in `drm-rust-next`, it is a good time to implement the
> feedback I gave on v8. Roughly:
>
> - `Pramin` should just be the owner and arbiter of the
>   `NV_PBUS_BAR0_WINDOW` register. Its `get_window(&mut self,
>   window_base: Bounded<u64, 24>)` method simply returns a window to the
>   1MB area starting from `base << 16`. It's the simplest possible
>   abstraction.
> - The returned window implements `Io` and `IoKnownSize`. This will allow
>   users to do all the fancy stuff that comes with `Io`, including
>   projections.
> - Random accesses to VRAM, which are needed for walking the page tables
>   and updating them, can be built on top of this simple API. Either by
>   adding read/write ops directly to `Pramin`, or by defining another
>   type that owns a `&mut Pramin` and moves the window automatically if
>   the next access is out of bounds. This should result in window
>   management code that is simpler than the current `compute_window`.
>
> That's really all we need. Then `GpuMm` can manage concurrency similarly
> to the current model by wrapping `Pramin` inside a `Mutex`, and is also
> free to experiment with different locking strategies, something the
> current design doesn't allow.

I am looking into implementing `Io`/`IoKnownSize` on `PraminWindow` --
that part makes sense independently. The `Io` impl would operate on the
current fixed 1MB window view; the auto-repositioning methods stay as a
there is a usecase (more later). None of this requires the locking changes.

I'm familiar with the v8 feedback, but I disagree with parts of it. The
current design isn't complex -- it's the external locking + split into
multiple types approach that I think would make it more complex. I'm
adopting the parts of your suggestions that I agreed with.

The auto-repositioning is actively used and not hypothetical.
`install_mappings` (mm/pagetable/map.rs) holds a single `PraminWindow`
while writing PDEs at scattered buddy-allocated VRAM addresses and then
walking up to 4 PDE levels per VFN -- each level a separately-allocated
4KB page that can be many MB apart. `invalidate_ptes` similarly holds one
window across all VFNs: for a range mapping more than ~256 PT pages the
PTE region alone spans > 1MB. Removing auto-repositioning would push that
window arithmetic onto every caller.

> > +        // TODO: Convert to Bounded<u64, 40> when available.
>
> It's available. :)

Changed to use Bounded, thanks for that primitive.

> I think this method (and `write_window_base`) should be methods of the
> `NV_PBUS_BAR0_WINDOW` register (`window_base` and `set_window_base`)

Agreed, I will move it to the registers.

> > +        vram_offset: usize,
>
> This should be a `u64`, since it is a VRAM address.
>
> Actually, I noticed that you introduced a `VramAddress` type in a latter
> patch. It might be worth introducing it earlier and using it everywhere
> a VRAM address is involved, that way we cannot make any mistake - not
> critical for now, but feel free to do it if you think it helps.

Cool, I will look into it. Sounds reasonable to do.

thanks,
-- 
Joel Fernandes