Re: [PATCH v5 4/6] gpu: nova-core: add FbHal::frts_size() for GA100 support

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

On Mon Apr 20, 2026 at 7:23 PM JST, Gary Guo wrote:
> On Mon Apr 20, 2026 at 2:28 AM BST, Alexandre Courbot wrote:
>> On Mon Apr 20, 2026 at 4:47 AM JST, Gary Guo wrote:
>>> On Fri Apr 17, 2026 at 8:13 PM BST, Timur Tabi wrote:
>>>> Introduce FbHal method frts_size() to return the size of the FRTS
>>>> window.  GA100 is a special case in that there is no FRTS, and so
>>>> the size must be set to 0.
>>>>
>>>> Note that we cannot use supports_display() to determine the FRTS
>>>> size because there are other GPUs (e.g. GA102GL) that have display
>>>> disabled (and so supports_display() returns False), but the FRTS
>>>> window size still needs to be 1MB.
>>>>
>>>> Signed-off-by: Timur Tabi <ttabi@nvidia.com>
>>>> Reviewed-by: Eliot Courtney <ecourtney@nvidia.com>
>>>> ---
>>>>  drivers/gpu/nova-core/fb.rs           |  6 +++---
>>>>  drivers/gpu/nova-core/fb/hal.rs       |  3 +++
>>>>  drivers/gpu/nova-core/fb/hal/ga100.rs |  6 ++++++
>>>>  drivers/gpu/nova-core/fb/hal/ga102.rs |  4 ++++
>>>>  drivers/gpu/nova-core/fb/hal/tu102.rs | 11 ++++++++++-
>>>>  5 files changed, 26 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/nova-core/fb.rs b/drivers/gpu/nova-core/fb.rs
>>>> index f357fb28b22c..a305a6dac758 100644
>>>> --- a/drivers/gpu/nova-core/fb.rs
>>>> +++ b/drivers/gpu/nova-core/fb.rs
>>>> @@ -216,10 +216,10 @@ pub(crate) fn new(chipset: Chipset, bar: &Bar0, gsp_fw: &GspFirmware) -> Result<
>>>>  
>>>>          let frts = {
>>>>              const FRTS_DOWN_ALIGN: Alignment = Alignment::new::<SZ_128K>();
>>>> -            const FRTS_SIZE: u64 = usize_as_u64(SZ_1M);
>>>> -            let frts_base = vga_workspace.start.align_down(FRTS_DOWN_ALIGN) - FRTS_SIZE;
>>>> +            let frts_size: u64 = hal.frts_size();
>>>> +            let frts_base = vga_workspace.start.align_down(FRTS_DOWN_ALIGN) - frts_size;
>>>>  
>>>> -            FbRange(frts_base..frts_base + FRTS_SIZE)
>>>> +            FbRange(frts_base..frts_base + frts_size)
>>>>          };
>>>>  
>>>>          let boot = {
>>>> diff --git a/drivers/gpu/nova-core/fb/hal.rs b/drivers/gpu/nova-core/fb/hal.rs
>>>> index aba0abd8ee00..1c01a6cbed65 100644
>>>> --- a/drivers/gpu/nova-core/fb/hal.rs
>>>> +++ b/drivers/gpu/nova-core/fb/hal.rs
>>>> @@ -25,6 +25,9 @@ pub(crate) trait FbHal {
>>>>  
>>>>      /// Returns the VRAM size, in bytes.
>>>>      fn vidmem_size(&self, bar: &Bar0) -> u64;
>>>> +
>>>> +    /// Returns the FRTS size, in bytes.
>>>> +    fn frts_size(&self) -> u64;
>>>>  }
>>>>  
>>>>  /// Returns the HAL corresponding to `chipset`.
>>>> diff --git a/drivers/gpu/nova-core/fb/hal/ga100.rs b/drivers/gpu/nova-core/fb/hal/ga100.rs
>>>> index 1c03783cddef..2f5871d915c3 100644
>>>> --- a/drivers/gpu/nova-core/fb/hal/ga100.rs
>>>> +++ b/drivers/gpu/nova-core/fb/hal/ga100.rs
>>>> @@ -66,6 +66,12 @@ fn supports_display(&self, bar: &Bar0) -> bool {
>>>>      fn vidmem_size(&self, bar: &Bar0) -> u64 {
>>>>          super::tu102::vidmem_size_gp102(bar)
>>>>      }
>>>> +
>>>> +    // GA100 is a special case where its FRTS region exists, but is empty.  We
>>>> +    // return a size of 0 because we still need to record where the region starts.
>>>> +    fn frts_size(&self) -> u64 {
>>>> +        0
>>>> +    }
>>>>  }
>>>>  
>>>>  const GA100: Ga100 = Ga100;
>>>> diff --git a/drivers/gpu/nova-core/fb/hal/ga102.rs b/drivers/gpu/nova-core/fb/hal/ga102.rs
>>>> index 4b9f0f74d0e7..3bb66f64bef7 100644
>>>> --- a/drivers/gpu/nova-core/fb/hal/ga102.rs
>>>> +++ b/drivers/gpu/nova-core/fb/hal/ga102.rs
>>>> @@ -35,6 +35,10 @@ fn supports_display(&self, bar: &Bar0) -> bool {
>>>>      fn vidmem_size(&self, bar: &Bar0) -> u64 {
>>>>          vidmem_size_ga102(bar)
>>>>      }
>>>> +
>>>> +    fn frts_size(&self) -> u64 {
>>>> +        super::tu102::frts_size_tu102()
>>>
>>> I checked the patch history and it looks like this changes from v1 to v2. If the
>>> argument that "both have the same size" then I think this just makes thing more
>>> confusing then the purpose of deduplicating.
>>>
>>> All the methods above are either delegating to GA100 code or custom, and now
>>> this delegates to TU102 without any explanation.
>>
>> The reason for this is that all chips have the same FRTS region size,
>> *except* GA100 which is 0. GA100 is the exception (it is also documented
>> as you can see in its snippet above) and should be treated as such. Thus
>> I'd say the current approach is correct.
>
> Then it should be:
>
>     trait FbHal {
>         fn frts_size(&self) -> u64 {
>             u64::SZ_1M
>         }
>     }
>
> and only have it overriden on GA100.

Mmm we can discuss the design of the HAL, but this is beyond the scope
of this series which just follows what other modules do. 

>
> Or better, use a boolean, I find Nouveau's C code clearer in this regard:
>
>     /*
> 	 * Calculate FB layout. FRTS is a memory region created by the FWSEC-FRTS firmware.
> 	 * FWSEC comes from VBIOS.  So on systems with no VBIOS (e.g. GA100), the FRTS does
> 	 * not exist.  Therefore, use the existence of VBIOS to determine whether to reserve
> 	 * an FRTS region.
> 	 */
> 	gsp->fb.wpr2.frts.size = device->bios ? 0x100000 : 0;
>
> so we could simiarly do
>
>     fn has_frts(&self) -> bool;
>
> Or
>
>     fn has_vbios(&self) -> bool;
>

The boolean-based approach might make more sense if there is really no
other possible size for the FRTS (which the Nouveau code seems to
suggest). I'll let Timur answer that one.

> (BTW, I suppose the FRTS region really does not exist, but the start address
> matters because firmware does some checks on the start address to ensure it's
> within a specific region regardless whether the size is 0?)

And that one as well. :)