Re: [PATCH] gpu: nova-core: make pramin/buddy optional

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

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.