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

["Gary Guo" <gary@garyguo.net>]

On Mon Apr 20, 2026 at 6:51 PM BST, Timur Tabi wrote:
> On Mon, 2026-04-20 at 21:50 +0900, Alexandre Courbot wrote:
>> > 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 first, this code in Nouveau is actually wrong, and I have a patchset pending upstream that fixes
> it.  This is the new version:

Ah, I see, so this is what the "supports_display" refers to in the commit
message?

It's not instinctively the same concept in my mind, but it makes sense now you
explained it.

>
> 	gsp->fb.wpr2.frts.size = device->chipset == 0x170 ? 0 : 0x100000;
>
> So this really needs to be a HAL-specific implementation.  For better or worse, we're actively
> trying to avoid code like this in Nova:
>
> 	if chipset is X or Y
> 	{
> 		// do this
> 	} else {
> 		// do that
> 	}
>
>> > 
>> > 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.
>
> I think there is something that everyone needs to understand.  99% of this stuff is completely
> arbitrary.  Someone, somewhere decided that FRTS is normally 1MB, but for some reason on GA100 it's
> 0.  It's not because GA100 doesn't have display hardware, because there are other GPUs that also
> don't have it but the FRTS is still 1MB.  
>
> So there's no reason to believe that in some future version of hardware and/or software, there might
> be a bigger or smaller FRTS.  If it really were an either-or thing, then it would have been defined
> as a boolean in our firmware data structures.
>
> The other thing to keep in mind is that a lot of this code is based on OpenRM.  One thing you need
> to understand is that RM is GPU HAL-happy.  We have this weird in-house object-oriented C variant
> that makes it trivial to HAL-ify any code.  So as soon as there is even the slightest difference
> between any two GPUs -- boom! make it a HAL!  There are a bunch of places in RM that are implemented
> as HALs that could be implemented more abstractly, but just aren't.  So if that's how RM does it,
> then that's how Nova is going to do it.
>
>> > (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. :)
>
> The FRTS region *does* exist, but its size is zero.  This is because the start address is still
> needed.  
>
> Some parts of GSP-RM will look at frts_size, see that it's zero, and then bail early.  For example,
> here's one code snippet from GSP-RM:
>
>     if (size == 0)
>     {
>         return NV_TRUE;
>     }
>
> But elsewhere in the same file is this:
>
>     rmLibosConfigInfo.gfwSrMem.pa = wprApertureStart + pWprMeta->frtsOffset + pWprMeta->frtsSize;
>
> So I guess what I'm really trying to say is that I've done a lot of research into how this code
> works, and it's written the way I wrote it for a reason.
>
> As for using a Trait to define a default value that is just overridden by GA100, that would be okay,
> but it wouldn't be any clearer or more accurate.
>
> I personally don't like `super::tu102::frts_size_tu102()` and I think both tu102::frts_size() and
> ga102::frts_size() should just return u64::SZ_1M, but I don't care enough to make a stink about it.

Thanks, that's a very good explanation how the reason the code is written as is.
With the information above, I'm happy with current approach.

Acked-by: Gary Guo <gary@garyguo.net>

I think it's important to note that not all of us have knowledge (or even
access) to some of the context that you have, so something that may seem obvious
to you could require some additional background or reasons to be added in commit
message or comment.

Also, in the commit message you mentioned

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

and this probably should be corrected.

Thanks,
Gary