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

Gary Guo
2026-04-20T10:23:13Z

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.

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;

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

Best,
Gary