Re: [PATCH v2 03/10] gpu: nova-core: gsp: Create wpr metadata

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

On Mon Sep 29, 2025 at 9:29 AM JST, Alistair Popple wrote:
> On 2025-09-26 at 11:24 +1000, Alexandre Courbot <acourbot@nvidia.com> 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 <apopple@nvidia.com>
>> >> 
>> >> ---
>> >> 
>> >> 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.