On Mon Sep 29, 2025 at 9:29 AM JST, Alistair Popple wrote: > On 2025-09-26 at 11:24 +1000, Alexandre Courbot <[email protected]> wrote... >> On Thu Sep 25, 2025 at 5:24 AM JST, Lyude Paul wrote: >> > On Mon, 2025-09-22 at 21:30 +1000, Alistair Popple wrote: >> >> The GSP requires some pieces of metadata to boot. These are passed in a >> >> struct which the GSP transfers via DMA. Create this struct and get a >> >> handle to it for future use when booting the GSP. >> >> >> >> Signed-off-by: Alistair Popple <[email protected]> >> >> >> >> --- >> >> >> >> Changes for v2: >> >> - Rebased on Alex's latest version >> >> --- >> >> drivers/gpu/nova-core/fb.rs | 1 - >> >> drivers/gpu/nova-core/firmware/gsp.rs | 3 +- >> >> drivers/gpu/nova-core/firmware/riscv.rs | 6 +- >> >> drivers/gpu/nova-core/gsp.rs | 1 + >> >> drivers/gpu/nova-core/gsp/boot.rs | 7 +++ >> >> drivers/gpu/nova-core/gsp/fw.rs | 63 ++++++++++++++++++- >> >> .../gpu/nova-core/gsp/fw/r570_144/bindings.rs | 2 + >> >> 7 files changed, 75 insertions(+), 8 deletions(-) >> >> >> >> diff --git a/drivers/gpu/nova-core/fb.rs b/drivers/gpu/nova-core/fb.rs >> >> index 4d6a1f452183..5580498ba2fb 100644 >> >> --- a/drivers/gpu/nova-core/fb.rs >> >> +++ b/drivers/gpu/nova-core/fb.rs >> >> @@ -87,7 +87,6 @@ pub(crate) fn unregister(&self, bar: &Bar0) { >> >> /// >> >> /// Contains ranges of GPU memory reserved for a given purpose during >> >> the GSP boot process. >> >> #[derive(Debug)] >> >> -#[expect(dead_code)] >> >> pub(crate) struct FbLayout { >> >> /// Range of the framebuffer. Starts at `0`. >> >> pub(crate) fb: Range<u64>, >> >> diff --git a/drivers/gpu/nova-core/firmware/gsp.rs >> >> b/drivers/gpu/nova-core/firmware/gsp.rs >> >> index 9654810834d9..67b85e1db27d 100644 >> >> --- a/drivers/gpu/nova-core/firmware/gsp.rs >> >> +++ b/drivers/gpu/nova-core/firmware/gsp.rs >> >> @@ -127,7 +127,7 @@ pub(crate) struct GspFirmware { >> >> /// Size in bytes of the firmware contained in [`Self::fw`]. >> >> pub size: usize, >> >> /// Device-mapped GSP signatures matching the GPU's [`Chipset`]. >> >> - signatures: DmaObject, >> >> + pub signatures: DmaObject, >> >> /// GSP bootloader, verifies the GSP firmware before loading and >> >> running it. >> >> pub bootloader: RiscvFirmware, >> >> } >> >> @@ -212,7 +212,6 @@ pub(crate) fn new<'a, 'b>( >> >> })) >> >> } >> >> >> >> - #[expect(unused)] >> >> /// Returns the DMA handle of the radix3 level 0 page table. >> >> pub(crate) fn radix3_dma_handle(&self) -> DmaAddress { >> >> self.level0.dma_handle() >> >> diff --git a/drivers/gpu/nova-core/firmware/riscv.rs >> >> b/drivers/gpu/nova-core/firmware/riscv.rs >> >> index b90acfc81e78..dec33d2b631a 100644 >> >> --- a/drivers/gpu/nova-core/firmware/riscv.rs >> >> +++ b/drivers/gpu/nova-core/firmware/riscv.rs >> >> @@ -53,11 +53,11 @@ fn new(bin_fw: &BinFirmware<'_>) -> Result<Self> { >> >> #[expect(unused)] >> >> pub(crate) struct RiscvFirmware { >> >> /// Offset at which the code starts in the firmware image. >> >> - code_offset: u32, >> >> + pub code_offset: u32, >> >> /// Offset at which the data starts in the firmware image. >> >> - data_offset: u32, >> >> + pub data_offset: u32, >> >> /// Offset at which the manifest starts in the firmware image. >> >> - manifest_offset: u32, >> >> + pub manifest_offset: u32, >> >> /// Application version. >> >> app_version: u32, >> >> /// Device-mapped firmware image. >> >> diff --git a/drivers/gpu/nova-core/gsp.rs b/drivers/gpu/nova-core/gsp.rs >> >> index 0185f66971ff..2daa46f2a514 100644 >> >> --- a/drivers/gpu/nova-core/gsp.rs >> >> +++ b/drivers/gpu/nova-core/gsp.rs >> >> @@ -13,6 +13,7 @@ >> >> use kernel::ptr::Alignment; >> >> use kernel::transmute::{AsBytes, FromBytes}; >> >> >> >> +use crate::fb::FbLayout; >> >> use fw::LibosMemoryRegionInitArgument; >> >> >> >> pub(crate) const GSP_PAGE_SHIFT: usize = 12; >> >> diff --git a/drivers/gpu/nova-core/gsp/boot.rs >> >> b/drivers/gpu/nova-core/gsp/boot.rs >> >> index fb22508128c4..1d2448331d7a 100644 >> >> --- a/drivers/gpu/nova-core/gsp/boot.rs >> >> +++ b/drivers/gpu/nova-core/gsp/boot.rs >> >> @@ -1,6 +1,8 @@ >> >> // SPDX-License-Identifier: GPL-2.0 >> >> >> >> use kernel::device; >> >> +use kernel::dma::CoherentAllocation; >> >> +use kernel::dma_write; >> >> use kernel::pci; >> >> use kernel::prelude::*; >> >> >> >> @@ -14,6 +16,7 @@ >> >> FIRMWARE_VERSION, >> >> }; >> >> use crate::gpu::Chipset; >> >> +use crate::gsp::GspFwWprMeta; >> >> use crate::regs; >> >> use crate::vbios::Vbios; >> >> >> >> @@ -132,6 +135,10 @@ pub(crate) fn boot( >> >> bar, >> >> )?; >> >> >> >> + let wpr_meta = >> >> + CoherentAllocation::<GspFwWprMeta>::alloc_coherent(dev, 1, >> >> GFP_KERNEL | __GFP_ZERO)?; >> >> + dma_write!(wpr_meta[0] = GspFwWprMeta::new(&gsp_fw, >> >> &fb_layout))?; >> > >> > Not something I think we need to block this series on, but this line does >> > make >> > me wonder if we should have a variant of dma_write!() that uses >> > CoherentAllocation::write(), since I think that would actually be faster >> > then >> > calling dma_write!() here. >> >> Can you elaborate a bit on this idea? Would it be faster because it uses >> a non-volatile write in this case? >> >> On a related note, I wish we could make all these accesses to >> single-instance coherent allocations non-fallible, as this is a pattern >> we use often in Nova and the only thing that can fail is >> `item_from_index`, which we know at build-time is valid as we are >> accessing the first element. >> >> So if we enforced a rule that `count` must be >= 0 in >> `CoherentAllocation::alloc_attrs` (which is not currently enforced but >> would make sense imho), we could maybe add a new variant to >> `dma_read/write` that matches a non-indexed expression, and makes a >> non-fallible access to the first element of the allocation? How does >> that sound? > > Would this have to be limited to the first element though? I assume we could > make a CoherentAllocation variant where the number of elements is a compile > time > constant and therefore dma_read/write on those would be infallible except at > build time.
Oh yeah, we would have to try and write such a variant but AFAICT this should work nicely.
