Re: [PATCH v4 17/20] gpu: nova-core: compute layout of the FRTS region

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

On Wed Jun 4, 2025 at 6:14 AM JST, Lyude Paul wrote:
> On Wed, 2025-05-21 at 15:45 +0900, Alexandre Courbot wrote:
>> FWSEC-FRTS is run with the desired address of the FRTS region as
>> parameter, which we need to compute depending on some hardware
>> parameters.
>> 
>> Do this in a `FbLayout` structure, that will be later extended to
>> describe more memory regions used to boot the GSP.
>> 
>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
>> ---
>>  drivers/gpu/nova-core/gpu.rs              |  4 ++
>>  drivers/gpu/nova-core/gsp.rs              |  3 ++
>>  drivers/gpu/nova-core/gsp/fb.rs           | 77 +++++++++++++++++++++++++++++++
>>  drivers/gpu/nova-core/gsp/fb/hal.rs       | 30 ++++++++++++
>>  drivers/gpu/nova-core/gsp/fb/hal/ga100.rs | 24 ++++++++++
>>  drivers/gpu/nova-core/gsp/fb/hal/ga102.rs | 24 ++++++++++
>>  drivers/gpu/nova-core/gsp/fb/hal/tu102.rs | 28 +++++++++++
>>  drivers/gpu/nova-core/nova_core.rs        |  1 +
>>  drivers/gpu/nova-core/regs.rs             | 76 ++++++++++++++++++++++++++++++
>>  9 files changed, 267 insertions(+)
>> 
>> diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
>> index 39b1cd3eaf8dcf95900eb93d43cfb4f085c897f0..7e03a5696011d12814995928b2984cceae6b6756 100644
>> --- a/drivers/gpu/nova-core/gpu.rs
>> +++ b/drivers/gpu/nova-core/gpu.rs
>> @@ -7,6 +7,7 @@
>>  use crate::falcon::{gsp::Gsp, sec2::Sec2, Falcon};
>>  use crate::firmware::{Firmware, FIRMWARE_VERSION};
>>  use crate::gfw;
>> +use crate::gsp::fb::FbLayout;
>>  use crate::regs;
>>  use crate::util;
>>  use crate::vbios::Vbios;
>> @@ -239,6 +240,9 @@ pub(crate) fn new(
>>  
>>          let _sec2_falcon = Falcon::<Sec2>::new(pdev.as_ref(), spec.chipset, bar, true)?;
>>  
>> +        let fb_layout = FbLayout::new(spec.chipset, bar)?;
>> +        dev_dbg!(pdev.as_ref(), "{:#x?}\n", fb_layout);
>> +
>>          // Will be used in a later patch when fwsec firmware is needed.
>>          let _bios = Vbios::new(pdev, bar)?;
>>  
>> diff --git a/drivers/gpu/nova-core/gsp.rs b/drivers/gpu/nova-core/gsp.rs
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..27616a9d2b7069b18661fc97811fa1cac285b8f8
>> --- /dev/null
>> +++ b/drivers/gpu/nova-core/gsp.rs
>> @@ -0,0 +1,3 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +pub(crate) mod fb;
>> diff --git a/drivers/gpu/nova-core/gsp/fb.rs b/drivers/gpu/nova-core/gsp/fb.rs
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..e65f2619b4c03c4fa51bb24f3d60e8e7008e6ca5
>> --- /dev/null
>> +++ b/drivers/gpu/nova-core/gsp/fb.rs
>> @@ -0,0 +1,77 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +use core::ops::Range;
>> +
>> +use kernel::num::NumExt;
>> +use kernel::prelude::*;
>> +
>> +use crate::driver::Bar0;
>> +use crate::gpu::Chipset;
>> +use crate::regs;
>> +
>> +mod hal;
>> +
>> +/// Layout of the GPU framebuffer memory.
>> +///
>> +/// Contains ranges of GPU memory reserved for a given purpose during the GSP bootup process.
>> +#[derive(Debug)]
>> +#[expect(dead_code)]
>> +pub(crate) struct FbLayout {
>> +    pub fb: Range<u64>,
>> +    pub vga_workspace: Range<u64>,
>> +    pub frts: Range<u64>,
>> +}
>> +
>> +impl FbLayout {
>> +    /// Computes the FB layout.
>> +    pub(crate) fn new(chipset: Chipset, bar: &Bar0) -> Result<Self> {
>> +        let hal = chipset.get_fb_fal();
>> +
>> +        let fb = {
>> +            let fb_size = hal.vidmem_size(bar);
>> +
>> +            0..fb_size
>> +        };
>> +
>> +        let vga_workspace = {
>> +            let vga_base = {
>> +                const NV_PRAMIN_SIZE: u64 = 0x100000;
>
> Don't leave those size constants out, they're getting lonely :C

Not quite sure where I should put these; they are not used (for now)
anywhere else, so the relevant scope is not obvious to me. Any
suggestion?

>
>> +                let base = fb.end - NV_PRAMIN_SIZE;
>> +
>> +                if hal.supports_display(bar) {
>> +                    match regs::NV_PDISP_VGA_WORKSPACE_BASE::read(bar).vga_workspace_addr() {
>
> Considering how long register names are by default, I wonder if we should just
> be doing:
>
> `use crate::regs::*`
>
> Instead, since the NV_* makes it pretty unambiguous already.

We could - I'm just a bit wary of introducing lots of (unrelated)
register names into the file's namespace... Maybe we should split
`regs.rs` into smaller sub-modules, e.g. `pdisp`, `pfb`, `pfalcon`, etc?

>
>> +                        Some(addr) => {
>> +                            if addr < base {
>> +                                const VBIOS_WORKSPACE_SIZE: u64 = 0x20000;
>> +
>> +                                // Point workspace address to end of framebuffer.
>> +                                fb.end - VBIOS_WORKSPACE_SIZE
>> +                            } else {
>> +                                addr
>> +                            }
>> +                        }
>> +                        None => base,
>> +                    }
>> +                } else {
>> +                    base
>> +                }
>> +            };
>> +
>> +            vga_base..fb.end
>> +        };
>> +
>> +        let frts = {
>> +            const FRTS_DOWN_ALIGN: u64 = 0x20000;
>> +            const FRTS_SIZE: u64 = 0x100000;
>> +            let frts_base = vga_workspace.start.align_down(FRTS_DOWN_ALIGN) - FRTS_SIZE;
>> +
>> +            frts_base..frts_base + FRTS_SIZE
>> +        };
>> +
>> +        Ok(Self {
>> +            fb,
>> +            vga_workspace,
>> +            frts,
>> +        })
>> +    }
>> +}
>> diff --git a/drivers/gpu/nova-core/gsp/fb/hal.rs b/drivers/gpu/nova-core/gsp/fb/hal.rs
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..9f8e777e90527026a39061166c6af6257a066aca
>> --- /dev/null
>> +++ b/drivers/gpu/nova-core/gsp/fb/hal.rs
>> @@ -0,0 +1,30 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +use crate::driver::Bar0;
>> +use crate::gpu::Chipset;
>> +
>> +mod ga100;
>> +mod ga102;
>> +mod tu102;
>> +
>> +pub(crate) trait FbHal {
>> +    /// Returns `true` is display is supported.
>> +    fn supports_display(&self, bar: &Bar0) -> bool;
>> +    /// Returns the VRAM size, in bytes.
>> +    fn vidmem_size(&self, bar: &Bar0) -> u64;
>> +}
>> +
>> +impl Chipset {
>> +    /// Returns the HAL corresponding to this chipset.
>> +    pub(super) fn get_fb_fal(self) -> &'static dyn FbHal {
>> +        use Chipset::*;
>> +
>> +        match self {
>> +            TU102 | TU104 | TU106 | TU117 | TU116 => tu102::TU102_HAL,
>> +            GA100 => ga100::GA100_HAL,
>> +            GA102 | GA103 | GA104 | GA106 | GA107 | AD102 | AD103 | AD104 | AD106 | AD107 => {
>
> Hopefully I'm not hallucinating us adding #[derive(Ordering)] or whatever it's
> called now that I'm 17 patches deep but, couldn't we use ranges here w/r/t to
> the model numbers?

I wish we could, but Rust doesn't allow this yet:

    error[E0029]: only `char` and numeric types are allowed in range patterns
      --> drivers/gpu/nova-core/gsp/fb/hal.rs:23:13
      |
    23 |             TU102..TU116 => tu102::TU102_HAL,
      |             -----^^-----
      |             |      |
      |             |      this is of type `Chipset` but it should be `char` or numeric
      |             this is of type `Chipset` but it should be `char` or numeric

Applying `#[repr(u32)]` on `Chipset` does not enable ranges unfortunately.

>
> Otherwise:
>
> Reviewed-by: Lyude Paul <lyude@redhat.com>

Thank you!