Re: [PATCH v2 05/10] gpu: nova-core: split FbLayout into FSP and non-FSP versions

On Fri Jul 3, 2026 at 7:22 PM JST, Eliot Courtney wrote:
> `FbLayout` is currently used for both pre and post FSP architectures. It
> contains ranges for each region of framebuffer, but on post FSP
> architectures, only the size is actually used by GSP. The offsets are
> not decided by the driver. So, for post FSP architectures `FbLayout`
> contains essentially guesses for the offsets. Instead, make separate
> types so that we only store the information that's actually needed,
> rather than keeping around offsets that may not be correct.
>
> Signed-off-by: Eliot Courtney <[email protected]>

These patches (5-8) are the only one remaining from the series, which is
not entirely a coincidence since they are kind of a different series by
themselves. :)

The patch's premise looks correct to me; although I wonder if we
couldn't avoid the enum by making the FB layout information more local.
Its use in `boot.rs` is what makes it difficult.

Ordering nit: this patch introduces an architectural change, following
by smaller fixes (at least for patches 7-8). If the fixes had come
first, they could have been merged first and the larger change would
operate on a better base. This is not a request to reorder if doing so
is not easy; just a note for future series.

Some more comments inline.

> ---
>  drivers/gpu/nova-core/fb.rs            | 70 ++++++++++++++++++++++---
>  drivers/gpu/nova-core/fsp.rs           | 15 +++---
>  drivers/gpu/nova-core/gsp/boot.rs      | 26 +++++-----
>  drivers/gpu/nova-core/gsp/fw.rs        | 95 ++++++++++++++++++++++++++--------
>  drivers/gpu/nova-core/gsp/hal.rs       |  4 +-
>  drivers/gpu/nova-core/gsp/hal/gh100.rs | 10 ++--
>  drivers/gpu/nova-core/gsp/hal/tu102.rs | 24 +++++----
>  7 files changed, 178 insertions(+), 66 deletions(-)
>
> diff --git a/drivers/gpu/nova-core/fb.rs b/drivers/gpu/nova-core/fb.rs
> index 273cff752fae..fd60f93258a9 100644
> --- a/drivers/gpu/nova-core/fb.rs
> +++ b/drivers/gpu/nova-core/fb.rs
> @@ -144,11 +144,30 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
>      }
>  }
>  
> -/// Layout of the GPU framebuffer memory.
> -///
> -/// Contains ranges of GPU memory reserved for a given purpose during the GSP boot process.
> +/// Framebuffer information required for GSP boot.
>  #[derive(Debug)]
> -pub(crate) struct FbLayout {
> +pub(crate) enum GspFbInfo {
> +    /// Concrete framebuffer ranges for host computed framebuffer layout.
> +    Ranges(FbRanges),
> +    /// Sizes of framebuffer ranges for GSP-FMC computed ranges.
> +    Sizes(FbSizes),
> +}
> +
> +impl GspFbInfo {
> +    /// Computes the framebuffer region information required for boot.
> +    pub(crate) fn new(chipset: Chipset, bar: Bar0<'_>, gsp_fw: &GspFirmware) -> Result<Self> {
> +        match chipset.gsp_boot_method() {
> +            gsp::GspBootMethod::Fsp => FbSizes::new(chipset, bar).map(Self::Sizes),
> +            gsp::GspBootMethod::Sec2 { .. } => {
> +                FbRanges::new(chipset, bar, gsp_fw).map(Self::Ranges)
> +            }
> +        }
> +    }
> +}
> +
> +/// Framebuffer ranges needed for GSP boot process.
> +#[derive(Debug)]
> +pub(crate) struct FbRanges {
>      /// Range of the framebuffer. Starts at `0`.
>      pub(crate) fb: FbRange,
>      /// VGA workspace, small area of reserved memory at the end of the framebuffer.
> @@ -163,15 +182,17 @@ pub(crate) struct FbLayout {
>      pub(crate) wpr2_heap: FbRange,
>      /// WPR2 region range, starting with an instance of `GspFwWprMeta`.
>      pub(crate) wpr2: FbRange,
> +    /// Non-WPR heap, located just below WPR2.
>      pub(crate) heap: FbRange,
> +    /// Number of VF partitions.
>      pub(crate) vf_partition_count: u8,
>      /// PMU reserved memory size, in bytes.
>      pub(crate) pmu_reserved_size: u32,
>  }
>  
> -impl FbLayout {
> -    /// Computes the FB layout for `chipset` required to run the `gsp_fw` GSP firmware.
> -    pub(crate) fn new(chipset: Chipset, bar: Bar0<'_>, gsp_fw: &GspFirmware) -> Result<Self> {
> +impl FbRanges {
> +    /// Computes concrete framebuffer ranges required on non-FSP booting architectures.
> +    fn new(chipset: Chipset, bar: Bar0<'_>, gsp_fw: &GspFirmware) -> Result<Self> {
>          let hal = hal::fb_hal(chipset);
>  
>          let fb = {
> @@ -270,3 +291,38 @@ pub(crate) fn new(chipset: Chipset, bar: Bar0<'_>, gsp_fw: &GspFirmware) -> Resu
>          })
>      }
>  }
> +
> +/// Framebuffer region sizes needed for GSP-FMC boot.
> +#[derive(Debug)]
> +pub(crate) struct FbSizes {
> +    /// VGA workspace size, in bytes.
> +    pub(crate) vga_workspace_size: u64,
> +    /// FRTS size, in bytes.
> +    pub(crate) frts_size: u64,
> +    /// WPR2 heap size, in bytes.
> +    pub(crate) wpr2_heap_size: u64,
> +    /// Non-WPR heap size, in bytes.
> +    pub(crate) heap_size: u64,
> +    /// PMU reserved memory size, in bytes.
> +    pub(crate) pmu_reserved_size: u32,
> +    /// Number of VF partitions.
> +    pub(crate) vf_partition_count: u8,
> +}
> +
> +impl FbSizes {
> +    /// Computes the framebuffer region sizes for GSP-FMC boot.
> +    fn new(chipset: Chipset, bar: Bar0<'_>) -> Result<Self> {
> +        let hal = hal::fb_hal(chipset);
> +        let fb_size = hal.vidmem_size(bar);
> +
> +        Ok(Self {
> +            vga_workspace_size: u64::SZ_128K,

If this is a const, do we need to store it here? Can't we define it and
use it where needed?

> +            frts_size: hal.frts_size(),
> +            wpr2_heap_size: gsp::LibosParams::from_chipset(chipset)
> +                .wpr_heap_size(chipset, fb_size)?,
> +            heap_size: u64::from(hal.non_wpr_heap_size()),
> +            pmu_reserved_size: hal.pmu_reserved_size(),
> +            vf_partition_count: 0,
> +        })
> +    }
> +}
> diff --git a/drivers/gpu/nova-core/fsp.rs b/drivers/gpu/nova-core/fsp.rs
> index 5b782aa2e3fd..533fb95573ab 100644
> --- a/drivers/gpu/nova-core/fsp.rs
> +++ b/drivers/gpu/nova-core/fsp.rs
> @@ -31,7 +31,7 @@
>          fsp::Fsp as FspEngine,
>          Falcon, //
>      },
> -    fb::FbLayout,
> +    fb::FbSizes,
>      firmware::{
>          fsp::{
>              FmcSignatures,
> @@ -136,14 +136,14 @@ struct FspCotMessage {
>  impl FspCotMessage {
>      /// Returns an in-place initializer for [`FspCotMessage`].
>      fn new<'a>(
> -        fb_layout: &FbLayout,
> +        fb_info: &FbSizes,
>          fsp_fw: &'a FspFirmware,
>          args: &'a FmcBootArgs<'_>,
>      ) -> Result<impl Init<Self> + 'a> {
>          // frts_vidmem_offset is measured from the end of FB, so FRTS sits at
>          // (end of FB) - frts_vidmem_offset.
>          let frts_vidmem_offset = if !args.resume {
> -            let frts_reserved_size = fb_layout.heap.len() + u64::from(fb_layout.pmu_reserved_size);
> +            let frts_reserved_size = fb_info.heap_size + u64::from(fb_info.pmu_reserved_size);
>  
>              frts_reserved_size
>                  .align_up(Alignment::new::<SZ_2M>())
> @@ -153,7 +153,7 @@ fn new<'a>(
>          };
>  
>          let frts_size: u32 = if !args.resume {
> -            fb_layout.frts.len().try_into()?
> +            fb_info.frts_size.try_into()?
>          } else {
>              0
>          };
> @@ -339,15 +339,12 @@ fn send_sync_fsp<M>(&mut self, dev: &device::Device, msg: &M) -> Result<KVec<u8>
>      pub(crate) fn boot_fmc(
>          &mut self,
>          dev: &device::Device<device::Bound>,
> -        fb_layout: &FbLayout,
> +        fb_info: &FbSizes,
>          args: &FmcBootArgs<'_>,
>      ) -> Result {
>          dev_dbg!(dev, "Starting FSP boot sequence for {}\n", args.chipset);
>  
> -        let msg = KBox::init(
> -            FspCotMessage::new(fb_layout, &self.fsp_fw, args)?,
> -            GFP_KERNEL,
> -        )?;
> +        let msg = KBox::init(FspCotMessage::new(fb_info, &self.fsp_fw, args)?, GFP_KERNEL)?;
>  
>          let _response_buf = self.send_sync_fsp(dev, &*msg)?;
>  
> diff --git a/drivers/gpu/nova-core/gsp/boot.rs b/drivers/gpu/nova-core/gsp/boot.rs
> index c347558aa8e5..14fd96084746 100644
> --- a/drivers/gpu/nova-core/gsp/boot.rs
> +++ b/drivers/gpu/nova-core/gsp/boot.rs
> @@ -16,7 +16,7 @@
>          gsp::Gsp,
>          Falcon, //
>      },
> -    fb::FbLayout,
> +    fb::GspFbInfo,
>      firmware::{
>          gsp::GspFirmware,
>          FIRMWARE_VERSION, //
> @@ -50,23 +50,21 @@ pub(crate) fn boot(
>  
>          let gsp_fw = KBox::pin_init(GspFirmware::new(dev, chipset, FIRMWARE_VERSION), GFP_KERNEL)?;
>  
> -        let fb_layout = FbLayout::new(chipset, bar, &gsp_fw)?;
> -        dev_dbg!(dev, "{:#x?}\n", fb_layout);
> +        let fb_info = GspFbInfo::new(chipset, bar, &gsp_fw)?;
> +        dev_dbg!(dev, "{:#x?}\n", fb_info);
>  
> -        let wpr_meta = Coherent::init(dev, GFP_KERNEL, GspFwWprMeta::new(&gsp_fw, &fb_layout))?;
> +        let wpr_meta = Coherent::init(dev, GFP_KERNEL, GspFwWprMeta::new(&gsp_fw, &fb_info))?;
>  
>          // Perform the chipset-specific boot sequence, and retrieve the unload bundle.
> -        let unload_bundle = hal
> -            .boot(&self, &mut ctx, &fb_layout, &wpr_meta)?
> -            .or_else(|| {
> -                dev_warn!(dev, "The GSP won't be able to unload properly on unbind.\n");
> -                dev_warn!(
> -                    dev,
> -                    "The GPU will need to be reset before the driver can bind again.\n"
> -                );
> +        let unload_bundle = hal.boot(&self, &mut ctx, &fb_info, &wpr_meta)?.or_else(|| {
> +            dev_warn!(dev, "The GSP won't be able to unload properly on unbind.\n");
> +            dev_warn!(
> +                dev,
> +                "The GPU will need to be reset before the driver can bind again.\n"
> +            );
>  
> -                None
> -            });
> +            None
> +        });
>  
>          let mut unload_guard =
>              ScopeGuard::new_with_data((ctx, unload_bundle), |(ctx, unload_bundle)| {
> diff --git a/drivers/gpu/nova-core/gsp/fw.rs b/drivers/gpu/nova-core/gsp/fw.rs
> index 2590931262af..3b148147cb18 100644
> --- a/drivers/gpu/nova-core/gsp/fw.rs
> +++ b/drivers/gpu/nova-core/gsp/fw.rs
> @@ -29,7 +29,7 @@
>  };
>  
>  use crate::{
> -    fb::FbLayout,
> +    fb::GspFbInfo,
>      firmware::gsp::GspFirmware,
>      gpu::{
>          Architecture,
> @@ -215,11 +215,65 @@ unsafe impl FromBytes for GspFwWprMeta {}
>  
>  impl GspFwWprMeta {
>      /// Returns an initializer for a `GspFwWprMeta` suitable for booting `gsp_firmware` using the
> -    /// `fb_layout` layout.
> +    /// framebuffer information.
>      pub(crate) fn new<'a>(
>          gsp_firmware: &'a GspFirmware,
> -        fb_layout: &'a FbLayout,
> +        fb_info: &'a GspFbInfo,
>      ) -> impl Init<Self> + 'a {
> +        #[derive(Default)]
> +        struct WprMetaFields {
> +            gsp_fw_rsvd_start: u64,
> +            non_wpr_heap_offset: u64,
> +            non_wpr_heap_size: u64,
> +            gsp_fw_wpr_start: u64,
> +            gsp_fw_heap_offset: u64,
> +            gsp_fw_heap_size: u64,
> +            gsp_fw_offset: u64,
> +            boot_bin_offset: u64,
> +            frts_offset: u64,
> +            frts_size: u64,
> +            gsp_fw_wpr_end: u64,
> +            gsp_fw_heap_vf_partition_count: u8,
> +            fb_size: u64,
> +            vga_workspace_offset: u64,
> +            vga_workspace_size: u64,
> +            pmu_reserved_size: u32,
> +        }
> +
> +        let fields = match fb_info {
> +            GspFbInfo::Ranges(ranges) => WprMetaFields {
> +                gsp_fw_rsvd_start: ranges.heap.start,
> +                non_wpr_heap_offset: ranges.heap.start,
> +                non_wpr_heap_size: ranges.heap.len(),
> +                gsp_fw_wpr_start: ranges.wpr2.start,
> +                gsp_fw_heap_offset: ranges.wpr2_heap.start,
> +                gsp_fw_heap_size: ranges.wpr2_heap.len(),
> +                gsp_fw_offset: ranges.elf.start,
> +                boot_bin_offset: ranges.boot.start,
> +                frts_offset: ranges.frts.start,
> +                frts_size: ranges.frts.len(),
> +                gsp_fw_wpr_end: ranges
> +                    .vga_workspace
> +                    .start
> +                    .align_down(Alignment::new::<SZ_128K>()),
> +                gsp_fw_heap_vf_partition_count: ranges.vf_partition_count,
> +                fb_size: ranges.fb.len(),
> +                vga_workspace_offset: ranges.vga_workspace.start,
> +                vga_workspace_size: ranges.vga_workspace.len(),
> +                pmu_reserved_size: ranges.pmu_reserved_size,
> +            },
> +            GspFbInfo::Sizes(sizes) => WprMetaFields {
> +                non_wpr_heap_size: sizes.heap_size,
> +                gsp_fw_heap_size: sizes.wpr2_heap_size,
> +                frts_size: sizes.frts_size,
> +                gsp_fw_heap_vf_partition_count: sizes.vf_partition_count,
> +                vga_workspace_size: sizes.vga_workspace_size,
> +                pmu_reserved_size: sizes.pmu_reserved_size,
> +                // When only sizes are supplied, offsets and several other parameters are not used.
> +                ..Default::default()
> +            },
> +        };
> +
>          let init_inner = init!(bindings::GspFwWprMeta {
>              // CAST: we want to store the bits of `GSP_FW_WPR_META_MAGIC` unmodified.
>              magic: bindings::GSP_FW_WPR_META_MAGIC as u64,
> @@ -237,25 +291,22 @@ pub(crate) fn new<'a>(
>                      sizeOfSignature: u64::from_safe_cast(gsp_firmware.signatures.size()),
>                  },
>              },
> -            gspFwRsvdStart: fb_layout.heap.start,
> -            nonWprHeapOffset: fb_layout.heap.start,
> -            nonWprHeapSize: fb_layout.heap.end - fb_layout.heap.start,
> -            gspFwWprStart: fb_layout.wpr2.start,
> -            gspFwHeapOffset: fb_layout.wpr2_heap.start,
> -            gspFwHeapSize: fb_layout.wpr2_heap.end - fb_layout.wpr2_heap.start,
> -            gspFwOffset: fb_layout.elf.start,
> -            bootBinOffset: fb_layout.boot.start,
> -            frtsOffset: fb_layout.frts.start,
> -            frtsSize: fb_layout.frts.end - fb_layout.frts.start,
> -            gspFwWprEnd: fb_layout
> -                .vga_workspace
> -                .start
> -                .align_down(Alignment::new::<SZ_128K>()),
> -            gspFwHeapVfPartitionCount: fb_layout.vf_partition_count,
> -            fbSize: fb_layout.fb.end - fb_layout.fb.start,
> -            vgaWorkspaceOffset: fb_layout.vga_workspace.start,
> -            vgaWorkspaceSize: fb_layout.vga_workspace.end - fb_layout.vga_workspace.start,
> -            pmuReservedSize: fb_layout.pmu_reserved_size,
> +            gspFwRsvdStart: fields.gsp_fw_rsvd_start,
> +            nonWprHeapOffset: fields.non_wpr_heap_offset,
> +            nonWprHeapSize: fields.non_wpr_heap_size,
> +            gspFwWprStart: fields.gsp_fw_wpr_start,
> +            gspFwHeapOffset: fields.gsp_fw_heap_offset,
> +            gspFwHeapSize: fields.gsp_fw_heap_size,
> +            gspFwOffset: fields.gsp_fw_offset,
> +            bootBinOffset: fields.boot_bin_offset,
> +            frtsOffset: fields.frts_offset,
> +            frtsSize: fields.frts_size,
> +            gspFwWprEnd: fields.gsp_fw_wpr_end,
> +            gspFwHeapVfPartitionCount: fields.gsp_fw_heap_vf_partition_count,
> +            fbSize: fields.fb_size,
> +            vgaWorkspaceOffset: fields.vga_workspace_offset,
> +            vgaWorkspaceSize: fields.vga_workspace_size,
> +            pmuReservedSize: fields.pmu_reserved_size,
>              ..Zeroable::init_zeroed()
>          });
>  
> diff --git a/drivers/gpu/nova-core/gsp/hal.rs b/drivers/gpu/nova-core/gsp/hal.rs
> index 46428c623087..ddd356fafc1e 100644
> --- a/drivers/gpu/nova-core/gsp/hal.rs
> +++ b/drivers/gpu/nova-core/gsp/hal.rs
> @@ -11,7 +11,7 @@
>  };
>  
>  use crate::{
> -    fb::FbLayout,
> +    fb::GspFbInfo,
>      firmware::gsp::GspFirmware,
>      gpu::Chipset,
>      gsp::{
> @@ -42,7 +42,7 @@ fn boot(
>          &self,
>          gsp: &Gsp,
>          ctx: &mut GspBootContext<'_, '_>,
> -        fb_layout: &FbLayout,
> +        fb_info: &GspFbInfo,
>          wpr_meta: &Coherent<GspFwWprMeta>,
>      ) -> Result<Option<crate::gsp::UnloadBundle>>;
>  
> diff --git a/drivers/gpu/nova-core/gsp/hal/gh100.rs b/drivers/gpu/nova-core/gsp/hal/gh100.rs
> index 270703d0f5c6..6fc6d487e4c8 100644
> --- a/drivers/gpu/nova-core/gsp/hal/gh100.rs
> +++ b/drivers/gpu/nova-core/gsp/hal/gh100.rs
> @@ -15,7 +15,7 @@
>          gsp::Gsp as GspEngine,
>          Falcon, //
>      },
> -    fb::FbLayout,
> +    fb::GspFbInfo,
>      fsp::FmcBootArgs,
>      gsp::{
>          hal::{
> @@ -136,13 +136,17 @@ fn boot(
>          &self,
>          gsp: &Gsp,
>          ctx: &mut GspBootContext<'_, '_>,
> -        fb_layout: &FbLayout,
> +        fb_info: &GspFbInfo,
>          wpr_meta: &Coherent<GspFwWprMeta>,
>      ) -> Result<Option<crate::gsp::UnloadBundle>> {
>          let dev = ctx.dev();
>          let chipset = ctx.chipset;
>          let gsp_falcon = ctx.gsp_falcon;
>  
> +        let GspFbInfo::Sizes(fb_sizes) = fb_info else {
> +            return Err(EINVAL);
> +        };

Mmm I wish we would avoid that, this is another example of a runtime
check that should not need to be performed.

In this case I think we can, as we also have access to the
`GspFwWprMeta` which contains the FRTS size information that we are
using. We would just need to construct the range from it, pass it to
`run_fwsec_frts`, and we remove visibility from a lot of information
that this method didn't need in the first place. This could be a
standalone cleanup patch that comes before this one.

All the same (and again IIUC), the GH100's GSP HAL `boot` method should
be able to work entirely with `GspFwWprMeta`.

With these two out of the way, the only place where the FB layout
information is required becomes the construction of `GspFwWprMeta`;
which should give us more opportunities to make things more local and
remove `GspFbInfo` altogether, maybe by making the construction of
`GspFwWprMeta` a HAL method of `Fb` so we can hide `FbRanges`/`FbSizes`
there?

It's still not completely clear to me, so let's first see if my
suggestion can be applied and what the resulting code looks like once it
is done. But I sense room for simplification. :)