Hi Dave,
On Mon Oct 26, 2026 at 1:24 PM JST, Dave Airlie via B4 Relay wrote:
> From: Dave Airlie <[email protected]>
>
> These aren't used on systems with no framebuffer memory.
>
> Signed-off-by: Dave Airlie <[email protected]>
> ---
> 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.
