Re: [PATCH v4 1/5] gpu: nova-core: correct FRTS vidmem offset calculation

I'm basically ready to apply this, but a couple questions inline for
potential push-time fixes:

On Tue Aug 4, 2026 at 2:41 PM JST, Eliot Courtney wrote:
> Currently, the frts vidmem offset is calculated based on the non-wpr
> heap size and pmu reservation size, but this is not right. The layout
> actually looks like this:
>
> | non-wpr heap | WPR2 .. FRTS | PMU reserved | ... | VGA workspace |
>
> It's just by coincidence + generous alignment that the values happened
> to match. Instead, define a per-architecture reserved size at the end of
> the framebuffer and use this plus the PMU reserved size to calculate the
> frts vidmem offset.
>
> Fixes: d317e4585fa3 ("gpu: nova-core: Hopper/Blackwell: add FSP Chain of Trust boot")
> Signed-off-by: Eliot Courtney <[email protected]>
> ---
>  drivers/gpu/nova-core/fb/hal/gb100.rs  |  1 +
>  drivers/gpu/nova-core/fb/hal/gb202.rs  |  1 +
>  drivers/gpu/nova-core/fsp.rs           | 27 +++++++++++++++++++--------
>  drivers/gpu/nova-core/fsp/hal.rs       |  4 ++++
>  drivers/gpu/nova-core/fsp/hal/gb100.rs |  6 ++++++
>  drivers/gpu/nova-core/fsp/hal/gb202.rs |  9 ++++++++-
>  drivers/gpu/nova-core/fsp/hal/gh100.rs |  9 ++++++++-
>  7 files changed, 47 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/nova-core/fb/hal/gb100.rs b/drivers/gpu/nova-core/fb/hal/gb100.rs
> index ec55ec3fc7e1..7e5b0e3ffc67 100644
> --- a/drivers/gpu/nova-core/fb/hal/gb100.rs
> +++ b/drivers/gpu/nova-core/fb/hal/gb100.rs
> @@ -80,6 +80,7 @@ fn write_sysmem_flush_page_gb100(bar: Bar0<'_>, addr: Bounded<u64, 52>) {
>      );
>  }
>  
> +// This PMU reservation size is r570-specific.
>  pub(super) const fn pmu_reserved_size_gb100() -> u32 {
>      usize_into_u32::<{ const_align_up(SZ_8M + SZ_16M + SZ_4K, Alignment::new::<SZ_128K>()).unwrap() }>(
>      )
> diff --git a/drivers/gpu/nova-core/fb/hal/gb202.rs b/drivers/gpu/nova-core/fb/hal/gb202.rs
> index 69ba35d2ea08..c590e5b1269c 100644
> --- a/drivers/gpu/nova-core/fb/hal/gb202.rs
> +++ b/drivers/gpu/nova-core/fb/hal/gb202.rs
> @@ -73,6 +73,7 @@ fn pmu_reserved_size(&self) -> u32 {
>  
>      fn non_wpr_heap_size(&self) -> u32 {
>          // Non-WPR heap for GB20x (see Open RM: kgspGetNonWprHeapSize, GB202+).
> +        // This size is r570-specific.
>          u32::SZ_2M + u32::SZ_128K
>      }
>  
> diff --git a/drivers/gpu/nova-core/fsp.rs b/drivers/gpu/nova-core/fsp.rs
> index ba4544210e40..a81b51028282 100644
> --- a/drivers/gpu/nova-core/fsp.rs
> +++ b/drivers/gpu/nova-core/fsp.rs
> @@ -251,20 +251,31 @@ struct FspCotMessage {
>  }
>  
>  impl FspCotMessage {
> +    /// Computes the FRTS vidmem offset for the Chain-of-Trust message. It is measured backwards
> +    /// from the end of the framebuffer.
> +    fn frts_vidmem_offset(hal: &dyn hal::FspHal, fb_layout: &FbLayout) -> Result<u64> {
> +        let mut offset = u64::from(hal.fb_end_reserved_size());
> +
> +        if fb_layout.pmu_reserved_size != 0 {
> +            offset = (offset + u64::from(fb_layout.pmu_reserved_size))
> +                // The 2 MiB alignment is r570-specific.
> +                .align_up(Alignment::new::<SZ_2M>())
> +                .ok_or(EINVAL)?;
> +        }

Sashiko pointed it out, and I was on the fence about commenting on that
in the last revision, but I find it strange that we only align on
`pmu_reserved_size != 0`. The only chipset for which this doesn't run is
GH100, which is already 2MB-aligned, so this would be a no-op for it
anyway. And worst FRTS would sit slightly lower in memory, wasting a bit
of space above it.

WDYT about applying the alignment unconditionally?

I'm also fine with keeping it the current way, but the condition
deserves a comment at the very least, say:

    // As per OpenRM's `kfspPrepareBootCommands_GH100`.

As that's where it comes from, but the OpenRM code does not give a much
better explanation.

> +
> +        Ok(offset)
> +    }
> +
>      /// Returns an in-place initializer for [`FspCotMessage`].
>      fn new<'a>(
>          fb_layout: &FbLayout,
>          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 hal = hal::fsp_hal(args.chipset).ok_or(ENOTSUPP)?;
>  
> -            frts_reserved_size
> -                .align_up(Alignment::new::<SZ_2M>())
> -                .ok_or(EINVAL)?
> +        let frts_vidmem_offset = if !args.resume {
> +            Self::frts_vidmem_offset(hal, fb_layout)?
>          } else {
>              0
>          };
> @@ -275,7 +286,7 @@ fn new<'a>(
>              0
>          };
>  
> -        let version = hal::fsp_hal(args.chipset).ok_or(ENOTSUPP)?.cot_version();
> +        let version = hal.cot_version();
>          let size = num::usize_into_u16::<{ core::mem::size_of::<NvdmPayloadCot>() }>();
>  
>          Ok(init!(Self {
> diff --git a/drivers/gpu/nova-core/fsp/hal.rs b/drivers/gpu/nova-core/fsp/hal.rs
> index b6f2624bb13d..aa2f8bda59d2 100644
> --- a/drivers/gpu/nova-core/fsp/hal.rs
> +++ b/drivers/gpu/nova-core/fsp/hal.rs
> @@ -19,6 +19,10 @@ pub(super) trait FspHal {
>  
>      /// Returns the FSP Chain of Trust protocol version this chipset advertises.
>      fn cot_version(&self) -> u16;
> +
> +    // TODO: consider moving this into the TLV firmware metadata when ready
> +    /// Returns the size reserved at the end of the framebuffer, in bytes.
> +    fn fb_end_reserved_size(&self) -> u32;

Only noticed now, but since this is immediately converted to a `u64`,
and in the same spirit as patch 3, how about returning a `u64` directly
here? I can also do it on apply.