On Wed, May 07, 2025 at 10:52:44PM +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 <acour...@nvidia.com>
> ---
> drivers/gpu/nova-core/gpu.rs | 4 ++
> drivers/gpu/nova-core/gsp.rs | 3 ++
> drivers/gpu/nova-core/gsp/fb.rs | 108
> +++++++++++++++++++++++++++++++++++++
> drivers/gpu/nova-core/nova_core.rs | 1 +
> drivers/gpu/nova-core/regs.rs | 27 ++++++++++
> 5 files changed, 143 insertions(+)
>
> diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
> index
> 4bf7f72247e5320935a517270b5a0e1ec2becfec..a3d96639706e808305cce66416778d2bf6e7e683
> 100644
> --- a/drivers/gpu/nova-core/gpu.rs
> +++ b/drivers/gpu/nova-core/gpu.rs
> @@ -7,6 +7,7 @@
> use crate::driver::Bar0;
> use crate::falcon::{gsp::Gsp, sec2::Sec2, Falcon};
> use crate::firmware::Firmware;
> +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);
> +
> let _bios = Vbios::new(pdev, bar)?;
>
> Ok(pin_init!(Self {
> 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..f28ded59469d52daf39e5d19c09efd7bf08fee92
> --- /dev/null
> +++ b/drivers/gpu/nova-core/gsp/fb.rs
> @@ -0,0 +1,108 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +use core::ops::Range;
> +
> +use kernel::prelude::*;
> +
> +use crate::driver::Bar0;
> +use crate::gpu::Chipset;
> +use crate::regs;
> +
> +fn align_down(value: u64, align: u64) -> u64 {
> + value & !(align - 1)
> +}
Can this go in the previous patch, i.e. "rust: num: Add an upward alignment
helper for usize"?
> +
> +/// 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 bios: Range<u64>,
> +
> + pub frts: Range<u64>,
Please remove the empty lines.
> +}
> +
> +impl FbLayout {
> + pub(crate) fn new(chipset: Chipset, bar: &Bar0) -> Result<Self> {
> + let fb = {
> + let fb_size = vidmem_size(bar, chipset);
> +
> + 0..fb_size
> + };
> + let fb_len = fb.end - fb.start;
Isn't this the same as fb_size? Why not just write it as
let fb_size = vidmem_size(bar, chipset);
let fb = 0..fb_size;
> +
> + let vga_workspace = {
> + let vga_base = vga_workspace_addr(bar, fb_len, chipset);
> +
> + vga_base..fb.end
> + };
> +
> + let bios = vga_workspace.clone();
Why? And why store the same thing twice in FbLayout? If it's really needed,
clone it in the constructor below and add a comment why it's the same.
> +
> + let frts = {
> + const FRTS_DOWN_ALIGN: u64 = 0x20000;
> + const FRTS_SIZE: u64 = 0x100000;
> + let frts_base = align_down(vga_workspace.start, FRTS_DOWN_ALIGN)
> - FRTS_SIZE;
> +
> + frts_base..frts_base + FRTS_SIZE
> + };
> +
> + Ok(Self {
> + fb,
> + vga_workspace,
> + bios,
> + frts,
> + })
> + }
> +}
I'd probably wrap those helpers below in
mod hal { ... }
or even a new file fb/hal.rs to make their purpose obvious.
> +/// Returns `true` if the display is disabled.
> +fn display_disabled(bar: &Bar0, chipset: Chipset) -> bool {
> + if chipset >= Chipset::GA100 {
> +
> regs::NV_FUSE_STATUS_OPT_DISPLAY_MAXWELL::read(bar).display_disabled()
> + } else {
> + regs::NV_FUSE_STATUS_OPT_DISPLAY_AMPERE::read(bar).display_disabled()
> + }
> +}
> +
> +/// Returns the video memory size in bytes.
> +fn vidmem_size(bar: &Bar0, chipset: Chipset) -> u64 {
> + if chipset >= Chipset::GA102 {
Is GA102 intentional or should this also be GA100?
> + (regs::NV_PGC6_AON_SECURE_SCRATCH_GROUP_42::read(bar).value() as
> u64) << 20
Why isn't this shift part of the register abstraction?
> + } else {
> + let local_mem_range =
> regs::NV_PFB_PRI_MMU_LOCAL_MEMORY_RANGE::read(bar);
> + let size =
> + (local_mem_range.lower_mag() as u64) <<
> ((local_mem_range.lower_scale() as u64) + 20);
Same here. Please make this part of the register abstraction as it is done in
previous patches.
> +
> + if local_mem_range.ecc_mode_enabled() {
> + size / 16 * 15
> + } else {
> + size
> + }
> + }
> +}
> +
> +/// Returns the vga workspace address.
> +fn vga_workspace_addr(bar: &Bar0, fb_size: u64, chipset: Chipset) -> u64 {
> + let base = fb_size - 0x100000;
What's this offset? How do you guarantee that this never underflows?
> + let vga_workspace_base = if display_disabled(bar, chipset) {
> + regs::NV_PDISP_VGA_WORKSPACE_BASE::read(bar)
> + } else {
> + return base;
> + };
> +
> + if !vga_workspace_base.status_valid() {
> + return base;
> + }
> +
> + let addr = (vga_workspace_base.addr() as u64) << 16;
Where does this shift come from? Shouldn't this be part of the register
abstraction?
> + if addr < base {
> + fb_size - 0x20000
What's this offset? Can this ever underflow?
> + } else {
> + addr
> + }
> +}
> diff --git a/drivers/gpu/nova-core/nova_core.rs
> b/drivers/gpu/nova-core/nova_core.rs
> index
> ff6d0b40c18f36af4c7e2d5c839fdf77dba23321..202e978e56f024de3ae8b178e65b63c2cea244e1
> 100644
> --- a/drivers/gpu/nova-core/nova_core.rs
> +++ b/drivers/gpu/nova-core/nova_core.rs
> @@ -8,6 +8,7 @@
> mod falcon;
> mod firmware;
> mod gpu;
> +mod gsp;
> mod regs;
> mod util;
> mod vbios;
> diff --git a/drivers/gpu/nova-core/regs.rs b/drivers/gpu/nova-core/regs.rs
> index
> b5c6eeb6ed873a06b4aefcb375f4944eb0b20597..15ec9b7e69694ff198b5353d562fc1aff5eefd3f
> 100644
> --- a/drivers/gpu/nova-core/regs.rs
> +++ b/drivers/gpu/nova-core/regs.rs
> @@ -53,6 +53,12 @@ pub(crate) fn chipset(self) -> Result<Chipset, Error> {
> 23:0 adr_63_40 as u32;
> });
>
> +register!(NV_PFB_PRI_MMU_LOCAL_MEMORY_RANGE @ 0x00100ce0 {
> + 3:0 lower_scale as u8;
> + 9:4 lower_mag as u8;
> + 30:30 ecc_mode_enabled as bool;
> +});
> +
> /* PGC6 */
>
> register!(NV_PGC6_AON_SECURE_SCRATCH_GROUP_05_PRIV_LEVEL_MASK @ 0x00118128 {
> @@ -64,6 +70,27 @@ pub(crate) fn chipset(self) -> Result<Chipset, Error> {
> 31:0 value as u32;
> });
>
> +register!(NV_PGC6_AON_SECURE_SCRATCH_GROUP_42 @ 0x001183a4 {
> + 31:0 value as u32;
> +});
> +
> +/* PDISP */
> +
> +register!(NV_PDISP_VGA_WORKSPACE_BASE @ 0x00625f04 {
> + 3:3 status_valid as bool;
> + 31:8 addr as u32;
> +});
> +
> +/* FUSE */
> +
> +register!(NV_FUSE_STATUS_OPT_DISPLAY_MAXWELL @ 0x00021c04 {
> + 0:0 display_disabled as bool;
> +});
> +
> +register!(NV_FUSE_STATUS_OPT_DISPLAY_AMPERE @ 0x00820c04 {
> + 0:0 display_disabled as bool;
> +});
> +
> /* PFALCON */
>
> register!(NV_PFALCON_FALCON_IRQSCLR @ +0x00000004 {
>
> --
> 2.49.0
>
