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

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

On Wed May 14, 2025 at 1:41 AM JST, Danilo Krummrich wrote:
<snip>
>> 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"?

Yes, let me try to consolidate things around the `num` module. Not sure
why it didn't occur to me to add that one there.

>
>> +
>> +/// 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;

It is the same indeed, and fb_size and fb_len are semantically the same
thing so no need to have both.

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

The bios field does not seem to be used at the moment anyway, so let me
remove it.

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

Do we want a module here? I'm fine with it, but these methods are
already private anyway and putting them under a module would require
them to have `pub(super)` visibility.

... or maybe we should have an actual HAL here with dynamic dispatch,
similar to what we have in the falcon module. That's what OpenRM does
actually. Let me look into that.

>
>> +/// 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?

After double-checking with OpenRM GA102 is indeed correct.

>
>> +        (regs::NV_PGC6_AON_SECURE_SCRATCH_GROUP_42::read(bar).value() as u64) << 20
>
> Why isn't this shift part of the register abstraction?

This value came from a scratch register, which interpretation depends on
context so I did not abstract the values. But OpenRM does, so let me add
a way to create alias registers so we can get adequate definitions here
as well.

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

Ack.

>
>> +
>> +        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?

Looked it up in OpenRM, it is the size of PRAMIN. I'll add a constant.

>
>> +    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?

Yes. Also added documentation in the field to explain that the field's
lowest 16 bits are truncated.

>
>> +    if addr < base {
>> +        fb_size - 0x20000
>
> What's this offset? Can this ever underflow?

This one is also defined as a constant in OpenRM. Let me replicate it
here.