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.
