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

["Eliot Courtney" <ecourtney@nvidia.com>]

On Thu Jul 23, 2026 at 2:02 PM JST, Alexandre Courbot wrote:
> 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 <ecourtney@nvidia.com>
>
> 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.

The reason I used this order is that the next patch introduces
`fb_end_reserved_size` (which is needed to actually calculate the right
values) but it's only used on FbSizes (since it's FSP only). Doing the
reverse order would mean adding a new field that is only sometimes used
to the existing `FbLayout`. The other two patches after this could have
gone before this so that's fair enough.

>
> 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?

Yerp, I think so.

>
>> +            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. :)

Yeah it is unfortuante to have the runtime check but it's just another
consequence of pushing stuff across the dynamic dispatch boundary on the
HALs (see below for a method to avoid this). I am not sure about using
GspFwWprMeta as the source of truth though, for the following reasons:

1. The next patch adds `fb_end_reserved_size` which is used by the FSP
   code. It's not part of `GspFwWprMeta`. Meaning that GspFbInfo encodes
   more info than just the wire format bindings required by some of the
   hardware.
2. It feels a little gross to me to use such a bindings adjacent
   structure to be used upwardly instead of downwardly to the
   firmware/RPC boundary.
3. If fb.rs needs to depend on the hardware/firmware-specific
   GspFwWprMeta I feel it dilutes the meaning of having fb.rs be a
   separate module holding general info about the fb layout used by
   multiple modules. The dependency direction feels a little awkward to
   me.

Instead, I think it may be better to remove Coherent<GspFwWprMeta> from
the boot signature and create it inside the HAL. I think it's a bit
nicer to push that more HAL-y object downwards.

If we do this we can also push the creation of FbSizes/FbRanges into
each HAL implementation and get rid of the GspFbInfo enum and the
runtime check. WDYT?