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

Timur Tabi
2026-04-20T17:51:16Z

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:

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.