* [PATCH] gpu: nova-core: make pramin/buddy optional
@ 2026-10-26 4:24 ` Dave Airlie
0 siblings, 0 replies; 10+ messages in thread
From: Dave Airlie @ 2026-10-26 4:24 UTC (permalink / raw)
To: Danilo Krummrich, Alexandre Courbot, David Airlie
Cc: nova-gpu, dri-devel, Dave Airlie
These aren't used on systems with no framebuffer memory.
Signed-off-by: Dave Airlie <airlied@redhat.com>
---
drivers/gpu/nova-core/gpu.rs | 15 +++++++------
drivers/gpu/nova-core/mm.rs | 34 +++++++++++++++---------------
drivers/gpu/nova-core/mm/bar_user.rs | 14 ++++++------
drivers/gpu/nova-core/mm/pagetable/map.rs | 10 ++++-----
drivers/gpu/nova-core/mm/pagetable/walk.rs | 2 +-
5 files changed, 38 insertions(+), 37 deletions(-)
diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
index 4e59072cab50..5d53fde2cdc1 100644
--- a/drivers/gpu/nova-core/gpu.rs
+++ b/drivers/gpu/nova-core/gpu.rs
@@ -564,18 +564,19 @@ pub(crate) fn new<'a>(
// Create GPU memory manager owning memory management resources.
mm: {
- let usable_vram = gsp_static_info.usable_fb_regions.first().ok_or(ENODEV)?;
- let buddy_params = GpuBuddyParams {
- base_offset: usable_vram.start,
- size: usable_vram.end - usable_vram.start,
- chunk_size: Alignment::new::<SZ_4K>(),
- };
+ let buddy_params = gsp_static_info.usable_fb_regions.first().map(|usable_vram| {
+ GpuBuddyParams {
+ base_offset: usable_vram.start,
+ size: usable_vram.end - usable_vram.start,
+ chunk_size: Alignment::new::<SZ_4K>(),
+ }
+ });
GpuMm::new(
bar,
gsp_resources.spec.chipset,
buddy_params,
- VramAddress::from_raw(gsp_static_info.total_fb_end),
+ Some(VramAddress::from_raw(gsp_static_info.total_fb_end)),
)?
},
diff --git a/drivers/gpu/nova-core/mm.rs b/drivers/gpu/nova-core/mm.rs
index 8a934c55169f..9a2c73c978be 100644
--- a/drivers/gpu/nova-core/mm.rs
+++ b/drivers/gpu/nova-core/mm.rs
@@ -71,12 +71,12 @@ macro_rules! impl_pfn_bounded {
/// GPU Memory Manager - owns all core MM components.
///
/// Provides centralized ownership of memory management resources:
-/// - [`GpuBuddy`] allocator for VRAM page table allocation.
-/// - [`pramin::Pramin`] for direct VRAM access.
+/// - Optional [`GpuBuddy`] allocator for VRAM page table allocation.
+/// - Optional [`pramin::Pramin`] for direct VRAM access.
/// - [`Tlb`] manager for translation buffer flush operations.
pub(crate) struct GpuMm<'gpu> {
- buddy: GpuBuddy,
- pramin: pramin::Pramin<'gpu>,
+ buddy: Option<GpuBuddy>,
+ pramin: Option<pramin::Pramin<'gpu>>,
tlb: Pin<KBox<Tlb<'gpu>>>,
}
@@ -85,28 +85,28 @@ impl<'gpu> GpuMm<'gpu> {
pub(crate) fn new(
bar: Bar0<'gpu>,
chipset: Chipset,
- buddy_params: GpuBuddyParams,
- total_fb_end: VramAddress,
+ buddy_params: Option<GpuBuddyParams>,
+ total_fb_end: Option<VramAddress>,
) -> Result<Self> {
- // PRAMIN covers all physical VRAM (including GSP-reserved areas
- // above the usable region, e.g. the BAR1 page directory).
- let vram_region = VramAddress::ZERO..total_fb_end;
-
Ok(Self {
- buddy: GpuBuddy::new(buddy_params)?,
- pramin: pramin::Pramin::new(bar, chipset, vram_region)?,
+ buddy: buddy_params.map(GpuBuddy::new).transpose()?,
+ // PRAMIN covers all physical VRAM, including GSP-reserved areas above
+ // the usable region, such as the BAR1 page directory.
+ pramin: total_fb_end
+ .map(|end| pramin::Pramin::new(bar, chipset, VramAddress::ZERO..end))
+ .transpose()?,
tlb: KBox::pin_init(Tlb::new(bar), GFP_KERNEL)?,
})
}
/// Access the [`GpuBuddy`] allocator.
- pub(crate) fn buddy(&self) -> &GpuBuddy {
- &self.buddy
+ pub(crate) fn buddy(&self) -> Result<&GpuBuddy> {
+ self.buddy.as_ref().ok_or(ENODEV)
}
/// Access the [`pramin::Pramin`].
- fn pramin_mut(&mut self) -> &mut pramin::Pramin<'gpu> {
- &mut self.pramin
+ fn pramin_mut(&mut self) -> Result<&mut pramin::Pramin<'gpu>> {
+ self.pramin.as_mut().ok_or(ENODEV)
}
/// Access the [`Tlb`] manager.
@@ -329,7 +329,7 @@ pub(crate) fn run(
return Ok(());
};
- pramin::selftest::run(dev, mm.pramin_mut(), VramAddress::from_raw(base))?;
+ pramin::selftest::run(dev, mm.pramin_mut()?, VramAddress::from_raw(base))?;
bar_user::run_self_test(dev, mm, bar_user, bar1_pdb, chipset)
}
}
diff --git a/drivers/gpu/nova-core/mm/bar_user.rs b/drivers/gpu/nova-core/mm/bar_user.rs
index 6e9384f06f43..baf9450b93a6 100644
--- a/drivers/gpu/nova-core/mm/bar_user.rs
+++ b/drivers/gpu/nova-core/mm/bar_user.rs
@@ -212,14 +212,14 @@ pub(crate) fn run_self_test(
let pdb_addr = VramAddress::from_raw(bar1_pdb);
// Check if initial page tables are in VRAM.
- if crate::mm::pagetable::check_pdb_valid(mm.pramin_mut(), pdb_addr, chipset).is_err() {
+ if crate::mm::pagetable::check_pdb_valid(mm.pramin_mut()?, pdb_addr, chipset).is_err() {
dev_info!(dev, "MM: Self-test SKIPPED - no valid VRAM page tables\n");
return Ok(());
}
// Set up a test page from the buddy allocator.
let test_page_blocks = KBox::pin_init(
- mm.buddy().alloc_blocks(
+ mm.buddy()?.alloc_blocks(
GpuBuddyAllocMode::Simple,
SZ_4K.into_safe_cast(),
Alignment::new::<SZ_4K>(),
@@ -245,7 +245,7 @@ pub(crate) fn run_self_test(
let vram_read_addr = test_vram + 0x100;
// Test 1: Write via PRAMIN, read via BAR1.
- mm.pramin_mut()
+ mm.pramin_mut()?
.window_at::<u32>(vram_read_addr)?
.view()
.write_val(PATTERN_PRAMIN);
@@ -296,7 +296,7 @@ pub(crate) fn run_self_test(
// Punch a 4K hole at base+16K so the subsequent 32K allocation must split.
let _hole = KBox::pin_init(
- mm.buddy().alloc_blocks(
+ mm.buddy()?.alloc_blocks(
GpuBuddyAllocMode::Range(range_base + sz_16k..range_base + sz_16k + sz_4k),
SZ_4K.into_safe_cast(),
Alignment::new::<SZ_4K>(),
@@ -308,7 +308,7 @@ pub(crate) fn run_self_test(
// Allocate 32K within [base, base+36K). The hole forces the allocator to return
// split blocks whose sizes are determined by buddy alignment.
let blocks = KBox::pin_init(
- mm.buddy().alloc_blocks(
+ mm.buddy()?.alloc_blocks(
GpuBuddyAllocMode::Range(range_base..range_base + sz_32k_4k),
SZ_32K.into_safe_cast(),
Alignment::new::<SZ_4K>(),
@@ -353,7 +353,7 @@ pub(crate) fn run_self_test(
bar1.try_write32(PATTERN_BAR1, page_bar1_off)?;
let pramin_val = mm
- .pramin_mut()
+ .pramin_mut()?
.window_at::<u32>(VramAddress::from_raw(page_phys))?
.view()
.read_val();
@@ -391,7 +391,7 @@ pub(crate) fn run_self_test(
let access = bar_user.map(mm, &[test_pfn], true)?;
// Write pattern via PRAMIN, read via BarUserAccess.
- mm.pramin_mut()
+ mm.pramin_mut()?
.window_at::<u32>(test_vram)?
.view()
.write_val(PATTERN_BAR1);
diff --git a/drivers/gpu/nova-core/mm/pagetable/map.rs b/drivers/gpu/nova-core/mm/pagetable/map.rs
index 77431c509a89..fd404b0734bb 100644
--- a/drivers/gpu/nova-core/mm/pagetable/map.rs
+++ b/drivers/gpu/nova-core/mm/pagetable/map.rs
@@ -81,7 +81,7 @@ pub(super) fn new(pdb_addr: VramAddress) -> Self {
/// Allocate and zero a physical page table page.
fn alloc_and_zero_page(mm: &mut GpuMm<'_>, level: PageTableLevel) -> Result<PreparedPtPage> {
let blocks = KBox::pin_init(
- mm.buddy().alloc_blocks(
+ mm.buddy()?.alloc_blocks(
GpuBuddyAllocMode::Simple,
SZ_4K.into_safe_cast(),
Alignment::new::<SZ_4K>(),
@@ -94,7 +94,7 @@ fn alloc_and_zero_page(mm: &mut GpuMm<'_>, level: PageTableLevel) -> Result<Prep
// Zero via PRAMIN.
let window = mm
- .pramin_mut()
+ .pramin_mut()?
.window_at::<[u64; PAGE_SIZE / 8]>(page_vram)?;
for i in 0..PAGE_SIZE / 8 {
io_write!(window.view(), [build: i], 0);
@@ -120,7 +120,7 @@ fn ensure_single_pte_path(
for _ in 0..max_iter {
let result = self
.walker
- .walk_pde_levels(mm.pramin_mut(), vfn, |install_addr| {
+ .walk_pde_levels(mm.pramin_mut()?, vfn, |install_addr| {
pt_pages.get(&install_addr).and_then(|p| {
p.alloc
.iter()
@@ -197,7 +197,7 @@ pub(super) fn install_mappings(
writable: bool,
) -> Result {
{
- let pramin = mm.pramin_mut();
+ let pramin = mm.pramin_mut()?;
// Drain prepared PT pages, install all pending PDEs.
let mut cursor = pt_pages.cursor_front_mut();
@@ -255,7 +255,7 @@ pub(super) fn invalidate_ptes(
let invalid_pte = M::Pte::invalid();
{
- let pramin = mm.pramin_mut();
+ let pramin = mm.pramin_mut()?;
for i in 0..num_pages {
let i_u64: u64 = i.into_safe_cast();
let vfn = Vfn::new(vfn_start.raw() + i_u64);
diff --git a/drivers/gpu/nova-core/mm/pagetable/walk.rs b/drivers/gpu/nova-core/mm/pagetable/walk.rs
index 76c1729971f5..19aae75c2c76 100644
--- a/drivers/gpu/nova-core/mm/pagetable/walk.rs
+++ b/drivers/gpu/nova-core/mm/pagetable/walk.rs
@@ -172,7 +172,7 @@ pub(super) fn walk_pde_levels(
///
/// Returns [`WalkResult::PageTableMissing`] if intermediate tables don't exist.
pub(super) fn walk_to_pte_lookup(&self, mm: &mut GpuMm<'_>, vfn: Vfn) -> Result<WalkResult> {
- self.walk_to_pte_lookup_with_window(mm.pramin_mut(), vfn)
+ self.walk_to_pte_lookup_with_window(mm.pramin_mut()?, vfn)
}
/// Walk to PTE using a caller-provided PRAMIN manager (lookup only).
---
base-commit: 3497a403264b094ebd61b8b755165d56e7d9dc37
change-id: 20261026-nova-pramin-option-50cdf1102281
Best regards,
--
Dave Airlie <airlied@redhat.com>
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH] gpu: nova-core: make pramin/buddy optional
2026-10-26 4:24 ` Dave Airlie
(?)
@ 2026-10-26 6:58 ` sashiko-bot
-1 siblings, 0 replies; 10+ messages in thread
From: sashiko-bot @ 2026-10-26 6:58 UTC (permalink / raw)
To: Dave Airlie, Danilo Krummrich, Alexandre Courbot, David Airlie
Cc: nova-gpu, dri-devel
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] gpu: nova-core: Gpu::new() always passes Some for total_fb_end to GpuMm::new()
--
commit c48efe432798819fed4e7632bc17f4fc720dd7e4
Author: Dave Airlie <airlied@redhat.com>
gpu: nova-core: make pramin/buddy optional
This commit updates GpuMm so that the GpuBuddy allocator and Pramin manager
are optional to support GPU systems with no framebuffer memory.
> diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
> index 4e59072cab507..5d53fde2cdc1e 100644
> --- a/drivers/gpu/nova-core/gpu.rs
> +++ b/drivers/gpu/nova-core/gpu.rs
> @@ -564,18 +564,19 @@ pub(crate) fn new<'a>(
>
> // Create GPU memory manager owning memory management resources.
> mm: {
> - let usable_vram = gsp_static_info.usable_fb_regions.first().ok_or(ENODEV)?;
> - let buddy_params = GpuBuddyParams {
> - base_offset: usable_vram.start,
> - size: usable_vram.end - usable_vram.start,
> - chunk_size: Alignment::new::<SZ_4K>(),
> - };
> + let buddy_params = gsp_static_info.usable_fb_regions.first().map(|usable_vram| {
> + GpuBuddyParams {
> + base_offset: usable_vram.start,
> + size: usable_vram.end - usable_vram.start,
> + chunk_size: Alignment::new::<SZ_4K>(),
> + }
> + });
>
> GpuMm::new(
> bar,
> gsp_resources.spec.chipset,
> buddy_params,
> - VramAddress::from_raw(gsp_static_info.total_fb_end),
> + Some(VramAddress::from_raw(gsp_static_info.total_fb_end)),
> )?
> },
[Severity: Medium]
In Gpu::new(), the total_fb_end argument to GpuMm::new() is unconditionally
wrapped in Some(...). Since Gpu::new() is the only caller of GpuMm::new(),
won't GpuMm::new() always receive Some(...) for total_fb_end and
unconditionally call pramin::Pramin::new(), which writes the hardware BAR0
PRAMIN window base register via hal.write_pramin_window_base(bar, base) and
leaves GpuMm::pramin populated even on systems without framebuffer memory?
Also, in drivers/gpu/nova-core/gsp/commands.rs, GetGspStaticInfoReply::read()
still requires msg.total_fb_end() to be present when called from
Gpu::new() -> Gsp::get_static_info():
drivers/gpu/nova-core/gsp/commands.rs:GetGspStaticInfoReply::read() {
...
let total_fb_end = msg.total_fb_end().ok_or(EINVAL)?;
...
}
When GSP reports no framebuffer regions, msg.total_fb_end() returns None,
causing GetGspStaticInfoReply::read() to fail probe with EINVAL before
GpuMm::new() is reached.
Should GetGspStaticInfoReply::total_fb_end be changed to Option<u64> (or
Option<VramAddress>) and passed through to GpuMm::new() without wrapping it
unconditionally in Some(...)?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261026-nova-pramin-option-v1-1-d073b3f047fc@redhat.com?part=1
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] gpu: nova-core: make pramin/buddy optional
2026-10-26 4:24 ` Dave Airlie
(?)
(?)
@ 2026-10-26 18:50 ` Danilo Krummrich
-1 siblings, 0 replies; 10+ messages in thread
From: Danilo Krummrich @ 2026-10-26 18:50 UTC (permalink / raw)
To: Dave Airlie via B4 Relay
Cc: airlied, Alexandre Courbot, David Airlie, nova-gpu, dri-devel
On Mon Oct 26, 2026 at 5:24 AM CET, Dave Airlie via B4 Relay wrote:
> These aren't used on systems with no framebuffer memory.
Looks good, but I think it would be nice to have some more context in the commit
message.
Thanks,
Danilo
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] gpu: nova-core: make pramin/buddy optional
2026-10-26 4:24 ` Dave Airlie
` (2 preceding siblings ...)
(?)
@ 2026-10-30 3:51 ` Alexandre Courbot
2026-10-30 6:18 ` David Airlie
2026-10-30 7:54 ` Eliot Courtney
-1 siblings, 2 replies; 10+ messages in thread
From: Alexandre Courbot @ 2026-10-30 3:51 UTC (permalink / raw)
To: Dave Airlie via B4 Relay
Cc: airlied, Danilo Krummrich, David Airlie, nova-gpu, dri-devel
Hi Dave,
On Mon Oct 26, 2026 at 1:24 PM JST, Dave Airlie via B4 Relay wrote:
> From: Dave Airlie <airlied@redhat.com>
>
> These aren't used on systems with no framebuffer memory.
>
> Signed-off-by: Dave Airlie <airlied@redhat.com>
> ---
> drivers/gpu/nova-core/gpu.rs | 15 +++++++------
> drivers/gpu/nova-core/mm.rs | 34 +++++++++++++++---------------
> drivers/gpu/nova-core/mm/bar_user.rs | 14 ++++++------
> drivers/gpu/nova-core/mm/pagetable/map.rs | 10 ++++-----
> drivers/gpu/nova-core/mm/pagetable/walk.rs | 2 +-
> 5 files changed, 38 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
> index 4e59072cab50..5d53fde2cdc1 100644
> --- a/drivers/gpu/nova-core/gpu.rs
> +++ b/drivers/gpu/nova-core/gpu.rs
> @@ -564,18 +564,19 @@ pub(crate) fn new<'a>(
>
> // Create GPU memory manager owning memory management resources.
> mm: {
> - let usable_vram = gsp_static_info.usable_fb_regions.first().ok_or(ENODEV)?;
> - let buddy_params = GpuBuddyParams {
> - base_offset: usable_vram.start,
> - size: usable_vram.end - usable_vram.start,
> - chunk_size: Alignment::new::<SZ_4K>(),
> - };
> + let buddy_params = gsp_static_info.usable_fb_regions.first().map(|usable_vram| {
> + GpuBuddyParams {
> + base_offset: usable_vram.start,
> + size: usable_vram.end - usable_vram.start,
> + chunk_size: Alignment::new::<SZ_4K>(),
> + }
> + });
>
> GpuMm::new(
> bar,
> gsp_resources.spec.chipset,
> buddy_params,
> - VramAddress::from_raw(gsp_static_info.total_fb_end),
> + Some(VramAddress::from_raw(gsp_static_info.total_fb_end)),
> )?
> },
>
> diff --git a/drivers/gpu/nova-core/mm.rs b/drivers/gpu/nova-core/mm.rs
> index 8a934c55169f..9a2c73c978be 100644
> --- a/drivers/gpu/nova-core/mm.rs
> +++ b/drivers/gpu/nova-core/mm.rs
> @@ -71,12 +71,12 @@ macro_rules! impl_pfn_bounded {
> /// GPU Memory Manager - owns all core MM components.
> ///
> /// Provides centralized ownership of memory management resources:
> -/// - [`GpuBuddy`] allocator for VRAM page table allocation.
> -/// - [`pramin::Pramin`] for direct VRAM access.
> +/// - Optional [`GpuBuddy`] allocator for VRAM page table allocation.
> +/// - Optional [`pramin::Pramin`] for direct VRAM access.
> /// - [`Tlb`] manager for translation buffer flush operations.
> pub(crate) struct GpuMm<'gpu> {
> - buddy: GpuBuddy,
> - pramin: pramin::Pramin<'gpu>,
> + buddy: Option<GpuBuddy>,
> + pramin: Option<pramin::Pramin<'gpu>>,
> tlb: Pin<KBox<Tlb<'gpu>>>,
> }
>
> @@ -85,28 +85,28 @@ impl<'gpu> GpuMm<'gpu> {
> pub(crate) fn new(
> bar: Bar0<'gpu>,
> chipset: Chipset,
> - buddy_params: GpuBuddyParams,
> - total_fb_end: VramAddress,
> + buddy_params: Option<GpuBuddyParams>,
> + total_fb_end: Option<VramAddress>,
This is related to the comment made by Sashiko, but in which cases are
we expecting `total_fb_end` to be `None` here? Is this the same set of
systems for which `buddy_params` will be `None`? If so both arguments
should be merged into a new type to remove the ability to represent
invalid state where one is `Some` and the other is `None`.
> ) -> Result<Self> {
> - // PRAMIN covers all physical VRAM (including GSP-reserved areas
> - // above the usable region, e.g. the BAR1 page directory).
> - let vram_region = VramAddress::ZERO..total_fb_end;
> -
> Ok(Self {
> - buddy: GpuBuddy::new(buddy_params)?,
> - pramin: pramin::Pramin::new(bar, chipset, vram_region)?,
> + buddy: buddy_params.map(GpuBuddy::new).transpose()?,
> + // PRAMIN covers all physical VRAM, including GSP-reserved areas above
> + // the usable region, such as the BAR1 page directory.
> + pramin: total_fb_end
> + .map(|end| pramin::Pramin::new(bar, chipset, VramAddress::ZERO..end))
> + .transpose()?,
> tlb: KBox::pin_init(Tlb::new(bar), GFP_KERNEL)?,
> })
> }
>
> /// Access the [`GpuBuddy`] allocator.
> - pub(crate) fn buddy(&self) -> &GpuBuddy {
> - &self.buddy
> + pub(crate) fn buddy(&self) -> Result<&GpuBuddy> {
> + self.buddy.as_ref().ok_or(ENODEV)
> }
For this kind of accessor to an `Option` it is considered more idiomatic
to just return an `Option` and let the caller decide how to handle the
error, but in this case it just seems to be a private convenience method
used only by submodules, so doing that would defeat its purpose.
Instead, can we make it private?
<...>
> diff --git a/drivers/gpu/nova-core/mm/pagetable/map.rs b/drivers/gpu/nova-core/mm/pagetable/map.rs
> index 77431c509a89..fd404b0734bb 100644
> --- a/drivers/gpu/nova-core/mm/pagetable/map.rs
> +++ b/drivers/gpu/nova-core/mm/pagetable/map.rs
> @@ -81,7 +81,7 @@ pub(super) fn new(pdb_addr: VramAddress) -> Self {
> /// Allocate and zero a physical page table page.
> fn alloc_and_zero_page(mm: &mut GpuMm<'_>, level: PageTableLevel) -> Result<PreparedPtPage> {
> let blocks = KBox::pin_init(
> - mm.buddy().alloc_blocks(
> + mm.buddy()?.alloc_blocks(
> GpuBuddyAllocMode::Simple,
> SZ_4K.into_safe_cast(),
> Alignment::new::<SZ_4K>(),
> @@ -94,7 +94,7 @@ fn alloc_and_zero_page(mm: &mut GpuMm<'_>, level: PageTableLevel) -> Result<Prep
>
> // Zero via PRAMIN.
> let window = mm
> - .pramin_mut()
> + .pramin_mut()?
> .window_at::<[u64; PAGE_SIZE / 8]>(page_vram)?;
> for i in 0..PAGE_SIZE / 8 {
> io_write!(window.view(), [build: i], 0);
> @@ -120,7 +120,7 @@ fn ensure_single_pte_path(
> for _ in 0..max_iter {
> let result = self
> .walker
> - .walk_pde_levels(mm.pramin_mut(), vfn, |install_addr| {
> + .walk_pde_levels(mm.pramin_mut()?, vfn, |install_addr| {
> pt_pages.get(&install_addr).and_then(|p| {
> p.alloc
> .iter()
> @@ -197,7 +197,7 @@ pub(super) fn install_mappings(
> writable: bool,
> ) -> Result {
> {
> - let pramin = mm.pramin_mut();
> + let pramin = mm.pramin_mut()?;
>
> // Drain prepared PT pages, install all pending PDEs.
> let mut cursor = pt_pages.cursor_front_mut();
> @@ -255,7 +255,7 @@ pub(super) fn invalidate_ptes(
> let invalid_pte = M::Pte::invalid();
>
> {
> - let pramin = mm.pramin_mut();
> + let pramin = mm.pramin_mut()?;
I'll let Eliot comment on this in more depth as I am not very familiar
with this module, but I'd like to avoid these runtime checks if
possible. On systems without a framebuffer, we should not even be able
to call these methods, so I'd like to enforce it at that level.
Ideally, we would consolidate all the state requiring a framebuffer into
a new type (i.e. my first comment, but pushed further), and put it
behind a single `Option` that solely tells us whether the system has a
framebuffer or not. Then methods requiring a framebuffer can be passed a
reference to that type and work without runtime checking its presence.
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] gpu: nova-core: make pramin/buddy optional
2026-10-30 3:51 ` Alexandre Courbot
@ 2026-10-30 6:18 ` David Airlie
2026-10-30 11:28 ` Alexandre Courbot
2026-10-30 7:54 ` Eliot Courtney
1 sibling, 1 reply; 10+ messages in thread
From: David Airlie @ 2026-10-30 6:18 UTC (permalink / raw)
To: Alexandre Courbot
Cc: Dave Airlie via B4 Relay, Danilo Krummrich, David Airlie,
nova-gpu, dri-devel
> This is related to the comment made by Sashiko, but in which cases are
> we expecting `total_fb_end` to be `None` here? Is this the same set of
> systems for which `buddy_params` will be `None`? If so both arguments
> should be merged into a new type to remove the ability to represent
> invalid state where one is `Some` and the other is `None`.
Indeed I've been working on a patchset to better define the concept
for total_fb_end, so this patchset can work on top in a proper manner.
The question of whether total_fb_end == Some(0) or total_fb_end ==
None is a sufficient indicator for no framebuffer, and I suppose we
have to make a choice before we can land this patch, anyone got an opinion?
> >
> > /// Access the [`GpuBuddy`] allocator.
> > - pub(crate) fn buddy(&self) -> &GpuBuddy {
> > - &self.buddy
> > + pub(crate) fn buddy(&self) -> Result<&GpuBuddy> {
> > + self.buddy.as_ref().ok_or(ENODEV)
> > }
>
> For this kind of accessor to an `Option` it is considered more idiomatic
> to just return an `Option` and let the caller decide how to handle the
> error, but in this case it just seems to be a private convenience method
> used only by submodules, so doing that would defeat its purpose.
> Instead, can we make it private?
possibly, I'll look before resubmitting anything.
>
> <...>
> > diff --git a/drivers/gpu/nova-core/mm/pagetable/map.rs b/drivers/gpu/nova-core/mm/pagetable/map.rs
> > index 77431c509a89..fd404b0734bb 100644
> > --- a/drivers/gpu/nova-core/mm/pagetable/map.rs
> > +++ b/drivers/gpu/nova-core/mm/pagetable/map.rs
> > @@ -81,7 +81,7 @@ pub(super) fn new(pdb_addr: VramAddress) -> Self {
> > /// Allocate and zero a physical page table page.
> > fn alloc_and_zero_page(mm: &mut GpuMm<'_>, level: PageTableLevel) -> Result<PreparedPtPage> {
> > let blocks = KBox::pin_init(
> > - mm.buddy().alloc_blocks(
> > + mm.buddy()?.alloc_blocks(
> > GpuBuddyAllocMode::Simple,
> > SZ_4K.into_safe_cast(),
> > Alignment::new::<SZ_4K>(),
> > @@ -94,7 +94,7 @@ fn alloc_and_zero_page(mm: &mut GpuMm<'_>, level: PageTableLevel) -> Result<Prep
> >
> > // Zero via PRAMIN.
> > let window = mm
> > - .pramin_mut()
> > + .pramin_mut()?
> > .window_at::<[u64; PAGE_SIZE / 8]>(page_vram)?;
> > for i in 0..PAGE_SIZE / 8 {
> > io_write!(window.view(), [build: i], 0);
> > @@ -120,7 +120,7 @@ fn ensure_single_pte_path(
> > for _ in 0..max_iter {
> > let result = self
> > .walker
> > - .walk_pde_levels(mm.pramin_mut(), vfn, |install_addr| {
> > + .walk_pde_levels(mm.pramin_mut()?, vfn, |install_addr| {
> > pt_pages.get(&install_addr).and_then(|p| {
> > p.alloc
> > .iter()
> > @@ -197,7 +197,7 @@ pub(super) fn install_mappings(
> > writable: bool,
> > ) -> Result {
> > {
> > - let pramin = mm.pramin_mut();
> > + let pramin = mm.pramin_mut()?;
> >
> > // Drain prepared PT pages, install all pending PDEs.
> > let mut cursor = pt_pages.cursor_front_mut();
> > @@ -255,7 +255,7 @@ pub(super) fn invalidate_ptes(
> > let invalid_pte = M::Pte::invalid();
> >
> > {
> > - let pramin = mm.pramin_mut();
> > + let pramin = mm.pramin_mut()?;
>
> I'll let Eliot comment on this in more depth as I am not very familiar
> with this module, but I'd like to avoid these runtime checks if
> possible. On systems without a framebuffer, we should not even be able
> to call these methods, so I'd like to enforce it at that level.
>
> Ideally, we would consolidate all the state requiring a framebuffer into
> a new type (i.e. my first comment, but pushed further), and put it
> behind a single `Option` that solely tells us whether the system has a
> framebuffer or not. Then methods requiring a framebuffer can be passed a
> reference to that type and work without runtime checking its presence.
I think we have to get to handling instmem maps in bar2 properly
before we can completely encapsulate everything.
Ideally instmem would have one API for allocation and it would be
backed on systems with VRAM with vram allocations, and with dma
allocations on 0-fb systems.
Then we would have to store the instmem backing in some sort of
encapsulation that hides the pramin/vram buddy/bar2 handling and
device handle for dma alloc handling
Does that make sense?
Dave.
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] gpu: nova-core: make pramin/buddy optional
2026-10-30 6:18 ` David Airlie
@ 2026-10-30 11:28 ` Alexandre Courbot
0 siblings, 0 replies; 10+ messages in thread
From: Alexandre Courbot @ 2026-10-30 11:28 UTC (permalink / raw)
To: David Airlie
Cc: Dave Airlie via B4 Relay, Danilo Krummrich, David Airlie,
nova-gpu, dri-devel
On Fri Oct 30, 2026 at 3:18 PM JST, David Airlie wrote:
>> This is related to the comment made by Sashiko, but in which cases are
>> we expecting `total_fb_end` to be `None` here? Is this the same set of
>> systems for which `buddy_params` will be `None`? If so both arguments
>> should be merged into a new type to remove the ability to represent
>> invalid state where one is `Some` and the other is `None`.
>
> Indeed I've been working on a patchset to better define the concept
> for total_fb_end, so this patchset can work on top in a proper manner.
>
> The question of whether total_fb_end == Some(0) or total_fb_end ==
> None is a sufficient indicator for no framebuffer, and I suppose we
> have to make a choice before we can land this patch, anyone got an opinion?
I don't have a definitive answer to that, only a similar question [1]
for a hypothetical FB HAL method that might also cover this?
[1] https://lore.kernel.org/all/DMI4UK02HZB8.1KEPETWUOZ7E5@nvidia.com/
<...>
>> > @@ -120,7 +120,7 @@ fn ensure_single_pte_path(
>> > for _ in 0..max_iter {
>> > let result = self
>> > .walker
>> > - .walk_pde_levels(mm.pramin_mut(), vfn, |install_addr| {
>> > + .walk_pde_levels(mm.pramin_mut()?, vfn, |install_addr| {
>> > pt_pages.get(&install_addr).and_then(|p| {
>> > p.alloc
>> > .iter()
>> > @@ -197,7 +197,7 @@ pub(super) fn install_mappings(
>> > writable: bool,
>> > ) -> Result {
>> > {
>> > - let pramin = mm.pramin_mut();
>> > + let pramin = mm.pramin_mut()?;
>> >
>> > // Drain prepared PT pages, install all pending PDEs.
>> > let mut cursor = pt_pages.cursor_front_mut();
>> > @@ -255,7 +255,7 @@ pub(super) fn invalidate_ptes(
>> > let invalid_pte = M::Pte::invalid();
>> >
>> > {
>> > - let pramin = mm.pramin_mut();
>> > + let pramin = mm.pramin_mut()?;
>>
>> I'll let Eliot comment on this in more depth as I am not very familiar
>> with this module, but I'd like to avoid these runtime checks if
>> possible. On systems without a framebuffer, we should not even be able
>> to call these methods, so I'd like to enforce it at that level.
>>
>> Ideally, we would consolidate all the state requiring a framebuffer into
>> a new type (i.e. my first comment, but pushed further), and put it
>> behind a single `Option` that solely tells us whether the system has a
>> framebuffer or not. Then methods requiring a framebuffer can be passed a
>> reference to that type and work without runtime checking its presence.
>
> I think we have to get to handling instmem maps in bar2 properly
> before we can completely encapsulate everything.
>
> Ideally instmem would have one API for allocation and it would be
> backed on systems with VRAM with vram allocations, and with dma
> allocations on 0-fb systems.
>
> Then we would have to store the instmem backing in some sort of
> encapsulation that hides the pramin/vram buddy/bar2 handling and
> device handle for dma alloc handling
>
> Does that make sense?
Yes, and especially after Eliot's comment my suggestion seems to be moot
anyway. As long as we have an idea of where we want to take this to
minimize unneeded runtime checks, I think these kind of temporary
workarounds are ok while the correct design comes together.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] gpu: nova-core: make pramin/buddy optional
2026-10-30 3:51 ` Alexandre Courbot
2026-10-30 6:18 ` David Airlie
@ 2026-10-30 7:54 ` Eliot Courtney
1 sibling, 0 replies; 10+ messages in thread
From: Eliot Courtney @ 2026-10-30 7:54 UTC (permalink / raw)
To: Alexandre Courbot, Dave Airlie via B4 Relay
Cc: airlied, Danilo Krummrich, David Airlie, nova-gpu, dri-devel,
dri-devel
On Fri Oct 30, 2026 at 12:51 PM JST, Alexandre Courbot wrote:
[...]
>> @@ -85,28 +85,28 @@ impl<'gpu> GpuMm<'gpu> {
>> pub(crate) fn new(
>> bar: Bar0<'gpu>,
>> chipset: Chipset,
>> - buddy_params: GpuBuddyParams,
>> - total_fb_end: VramAddress,
>> + buddy_params: Option<GpuBuddyParams>,
>> + total_fb_end: Option<VramAddress>,
>
> This is related to the comment made by Sashiko, but in which cases are
> we expecting `total_fb_end` to be `None` here? Is this the same set of
> systems for which `buddy_params` will be `None`? If so both arguments
> should be merged into a new type to remove the ability to represent
> invalid state where one is `Some` and the other is `None`.
Agree with this here - these both rely on vram existing so nice to group
them.
[...]
>> ) -> Result {
>> {
>> - let pramin = mm.pramin_mut();
>> + let pramin = mm.pramin_mut()?;
>>
>> // Drain prepared PT pages, install all pending PDEs.
>> let mut cursor = pt_pages.cursor_front_mut();
>> @@ -255,7 +255,7 @@ pub(super) fn invalidate_ptes(
>> let invalid_pte = M::Pte::invalid();
>>
>> {
>> - let pramin = mm.pramin_mut();
>> + let pramin = mm.pramin_mut()?;
>
> I'll let Eliot comment on this in more depth as I am not very familiar
> with this module, but I'd like to avoid these runtime checks if
> possible. On systems without a framebuffer, we should not even be able
> to call these methods, so I'd like to enforce it at that level.
>
> Ideally, we would consolidate all the state requiring a framebuffer into
> a new type (i.e. my first comment, but pushed further), and put it
> behind a single `Option` that solely tells us whether the system has a
> framebuffer or not. Then methods requiring a framebuffer can be passed a
> reference to that type and work without runtime checking its presence.
I think that we can't just put both pramin and buddy/etc into a single
struct for vram-less systems. For example, for vGPU guest, pramin isn't
available (but we still have physical vram to allocate). We have some
matrix more like:
VRAM: exists(need physical alloc)/doesn't exist(use sysmem)
Pramin: exists(bootstrapping ptes)/exists(not using it)/doesn't exist
CPU->VRAM access: pramin/bar1+bar2/coherent link/no vram
So these things kinda end up having to be handled separately, afaict. We
will still have to manipulate GPU page tables even on systems that have
no vram/framebuffer.
The current method of always using pramin to write page tables needs to
be reworked and abstracted (I'm looking into it), since pramin may not
be available, and the method to write the page tables could be one of a
few things (pramin, bar2, sysmem, copy engine).
Given that this needs to be reworked regardless (e.g. map.rs should not
be using pramin directly) I think this is a reasonable short term
workaround tbh.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] gpu: nova-core: make pramin/buddy optional
2026-10-26 4:24 ` Dave Airlie
` (3 preceding siblings ...)
(?)
@ 2026-10-30 7:57 ` Eliot Courtney
2026-10-30 11:23 ` Alexandre Courbot
-1 siblings, 1 reply; 10+ messages in thread
From: Eliot Courtney @ 2026-10-30 7:57 UTC (permalink / raw)
To: airlied, Danilo Krummrich, Alexandre Courbot, David Airlie
Cc: nova-gpu, dri-devel, dri-devel
On Mon Oct 26, 2026 at 1:24 PM JST, Dave Airlie via B4 Relay wrote:
> From: Dave Airlie <airlied@redhat.com>
>
> These aren't used on systems with no framebuffer memory.
>
> Signed-off-by: Dave Airlie <airlied@redhat.com>
> ---
> drivers/gpu/nova-core/gpu.rs | 15 +++++++------
> drivers/gpu/nova-core/mm.rs | 34 +++++++++++++++---------------
> drivers/gpu/nova-core/mm/bar_user.rs | 14 ++++++------
> drivers/gpu/nova-core/mm/pagetable/map.rs | 10 ++++-----
> drivers/gpu/nova-core/mm/pagetable/walk.rs | 2 +-
> 5 files changed, 38 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
> index 4e59072cab50..5d53fde2cdc1 100644
> --- a/drivers/gpu/nova-core/gpu.rs
> +++ b/drivers/gpu/nova-core/gpu.rs
> @@ -564,18 +564,19 @@ pub(crate) fn new<'a>(
>
> // Create GPU memory manager owning memory management resources.
> mm: {
> - let usable_vram = gsp_static_info.usable_fb_regions.first().ok_or(ENODEV)?;
> - let buddy_params = GpuBuddyParams {
> - base_offset: usable_vram.start,
> - size: usable_vram.end - usable_vram.start,
> - chunk_size: Alignment::new::<SZ_4K>(),
> - };
> + let buddy_params = gsp_static_info.usable_fb_regions.first().map(|usable_vram| {
> + GpuBuddyParams {
> + base_offset: usable_vram.start,
> + size: usable_vram.end - usable_vram.start,
> + chunk_size: Alignment::new::<SZ_4K>(),
> + }
> + });
We should probably add a helper method on e.g. Chipset to say which
chips we expect FB on vs not, instead of proceeding with no fb regions
here for ones where we expect there to be one.
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] gpu: nova-core: make pramin/buddy optional
2026-10-30 7:57 ` Eliot Courtney
@ 2026-10-30 11:23 ` Alexandre Courbot
0 siblings, 0 replies; 10+ messages in thread
From: Alexandre Courbot @ 2026-10-30 11:23 UTC (permalink / raw)
To: Eliot Courtney
Cc: airlied, Danilo Krummrich, David Airlie, nova-gpu, dri-devel,
dri-devel
On Fri Oct 30, 2026 at 4:57 PM JST, Eliot Courtney wrote:
> On Mon Oct 26, 2026 at 1:24 PM JST, Dave Airlie via B4 Relay wrote:
>> From: Dave Airlie <airlied@redhat.com>
>>
>> These aren't used on systems with no framebuffer memory.
>>
>> Signed-off-by: Dave Airlie <airlied@redhat.com>
>> ---
>> drivers/gpu/nova-core/gpu.rs | 15 +++++++------
>> drivers/gpu/nova-core/mm.rs | 34 +++++++++++++++---------------
>> drivers/gpu/nova-core/mm/bar_user.rs | 14 ++++++------
>> drivers/gpu/nova-core/mm/pagetable/map.rs | 10 ++++-----
>> drivers/gpu/nova-core/mm/pagetable/walk.rs | 2 +-
>> 5 files changed, 38 insertions(+), 37 deletions(-)
>>
>> diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
>> index 4e59072cab50..5d53fde2cdc1 100644
>> --- a/drivers/gpu/nova-core/gpu.rs
>> +++ b/drivers/gpu/nova-core/gpu.rs
>> @@ -564,18 +564,19 @@ pub(crate) fn new<'a>(
>>
>> // Create GPU memory manager owning memory management resources.
>> mm: {
>> - let usable_vram = gsp_static_info.usable_fb_regions.first().ok_or(ENODEV)?;
>> - let buddy_params = GpuBuddyParams {
>> - base_offset: usable_vram.start,
>> - size: usable_vram.end - usable_vram.start,
>> - chunk_size: Alignment::new::<SZ_4K>(),
>> - };
>> + let buddy_params = gsp_static_info.usable_fb_regions.first().map(|usable_vram| {
>> + GpuBuddyParams {
>> + base_offset: usable_vram.start,
>> + size: usable_vram.end - usable_vram.start,
>> + chunk_size: Alignment::new::<SZ_4K>(),
>> + }
>> + });
>
> We should probably add a helper method on e.g. Chipset to say which
> chips we expect FB on vs not, instead of proceeding with no fb regions
> here for ones where we expect there to be one.
Typically "a helper method on Chipset" means that we want a HAL method.
It just so happens that we already have a FB HAL, and a `vidmem_size`
method on it. I don't know if a vidmem size of `0` can always be
interpreted as "no framebuffer", but if not maybe we can rework it to
add that information, or just add a new method.
^ permalink raw reply [flat|nested] 10+ messages in thread