Re: [PATCH v2 10/12] gpu: nova-core: falcon: allow DMA transfers to read from a framebuffer address

["Eliot Courtney" <ecourtney@nvidia.com>]

On Sat Nov 7, 2026 at 6:48 PM JST, Alexandre Courbot wrote:
> The r000 boot protocol has a load-and-execute event in which GSP-RM asks
> the driver to load a Heavy-Secured binary from framebuffer memory into
> the GSP falcon's memory by DMA in order to run it.
>
> Nova-core's falcon DMA loading method currently only supports loading
> from a DMA object that the driver has allocated, so there is no way to
> copy an image from framebuffer memory that an event supplied.
>
> Address this by making the source of a falcon DMA transfer an enum,
> which can be either a DMA object, or a GPU framebuffer address.
>
> The framebuffer address source has no user yet, but will be used by the
> r000 boot protocol.
>
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
>  drivers/gpu/nova-core/falcon.rs | 65 ++++++++++++++++++++++++++---------------
>  1 file changed, 42 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/nova-core/falcon.rs b/drivers/gpu/nova-core/falcon.rs
> index 00da664887fc..b63265abd263 100644
> --- a/drivers/gpu/nova-core/falcon.rs
> +++ b/drivers/gpu/nova-core/falcon.rs
> @@ -140,6 +140,15 @@ pub(crate) enum FalconMem {
>      Dmem { tag: bool },
>  }
>  
> +/// Where a falcon DMA transfer reads from.
> +pub(crate) enum FalconDmaSource<'a> {
> +    /// A DMA object in system memory.
> +    Sysmem(&'a Coherent<'a, [u8]>),
> +    /// GPU framebuffer memory at physical address `base`.
> +    #[expect(dead_code)]
> +    Framebuffer { base: u64 },
> +}
> +
>  bounded_enum! {
>      /// Defines the Framebuffer Interface (FBIF) aperture type.
>      /// This determines the memory type for external memory access during a DMA transfer, which is
> @@ -544,13 +553,10 @@ pub(crate) fn with_fbif_transcfg<R>(
>          Ok(result)
>      }
>  
> -    /// Perform a DMA write according to `load_offsets` from `dma_obj` into the falcon's
> -    /// `target_mem`.
> -    ///
> -    /// `sec` is set if the loaded firmware is expected to run in secure mode.
> +    /// Perform a DMA write according to `load_offsets` from `src` into the falcon's `target_mem`.
>      fn dma_wr(
>          &self,
> -        dma_obj: &Coherent<'_, [u8]>,
> +        src: FalconDmaSource<'_>,
>          target_mem: FalconMem,
>          load_offsets: FalconDmaLoadTarget,
>      ) -> Result {
> @@ -563,23 +569,32 @@ fn dma_wr(
>          // For IMEM and tagged DMEM, we want to use the start offset as a virtual address tag for
>          // each page, since addresses in the firmware (and the boot vector) are virtual.
>          //
> -        // For untagged DMEM, the start offset is folded into the DMA base.
> -        let (fb_offset, base_offset) = match target_mem {
> -            FalconMem::ImemSecure | FalconMem::ImemNonSecure | FalconMem::Dmem { tag: true } => {
> -                (load_offsets.src_start, 0)
> +        // For untagged DMEM in a DMA object, the start offset is folded into the DMA base. For a
> +        // framebuffer address, the DMA engine adds the start offset to the address.
> +        let (fb_offset, dma_base) = match src {
> +            FalconDmaSource::Sysmem(dma_obj) => {
> +                let (fb_offset, base_offset) = match target_mem {
> +                    FalconMem::ImemSecure
> +                    | FalconMem::ImemNonSecure
> +                    | FalconMem::Dmem { tag: true } => (load_offsets.src_start, 0),
> +                    FalconMem::Dmem { tag: false } => {
> +                        (0, usize::from_safe_cast(load_offsets.src_start))
> +                    }
> +                };
> +
> +                // Upper limit of transfer is `(num_transfers * DMA_LEN) + load_offsets.src_start`.
> +                let dma_end = num_transfers
> +                    .checked_mul(DMA_LEN)
> +                    .and_then(|size| size.checked_add(load_offsets.src_start))
> +                    .map(usize::from_safe_cast)
> +                    .ok_or(EOVERFLOW)?;
> +
> +                (
> +                    fb_offset,
> +                    io_project!(dma_obj, [try: base_offset..dma_end]).dma_address(),
> +                )
>              }
> -            FalconMem::Dmem { tag: false } => (0, usize::from_safe_cast(load_offsets.src_start)),
> -        };
> -
> -        let dma_base = {
> -            // Upper limit of transfer is `(num_transfers * DMA_LEN) + load_offsets.src_start`.
> -            let dma_end = num_transfers
> -                .checked_mul(DMA_LEN)
> -                .and_then(|size| size.checked_add(load_offsets.src_start))
> -                .map(usize::from_safe_cast)
> -                .ok_or(EOVERFLOW)?;
> -
> -            io_project!(dma_obj, [try: base_offset..dma_end]).dma_address()
> +            FalconDmaSource::Framebuffer { base } => (load_offsets.src_start, base),
>          };

I think this code is simplifiable. The `Sysmem` branch computes some offsets
based on `target_mem`. But I don't see why this has to be specific to that
branch. What we could do (and what openrm does afaict) is just split that out,
for example:

```
let (fb_offset, base_offset) = match target_mem {
    FalconMem::ImemSecure | FalconMem::ImemNonSecure | FalconMem::Dmem { tag: true } => {
        (load_offsets.src_start, 0)
    }
    FalconMem::Dmem { tag: false } => (0, usize::from_safe_cast(load_offsets.src_start)),
};

let dma_base = match src {
    FalconDmaSource::Sysmem(dma_obj) => {
        let dma_end = num_transfers
            .checked_mul(DMA_LEN)
            .and_then(|size| size.checked_add(load_offsets.src_start))
            .map(usize::from_safe_cast)
            .ok_or(EOVERFLOW)?;

        io_project!(dma_obj, [try: base_offset..dma_end]).dma_address()
    }
    FalconDmaSource::Framebuffer { base } => base
        .checked_add(u64::from_safe_cast(base_offset))
        .ok_or(EOVERFLOW)?,
};
```

Alternatively I think the following is simpler where you just always pass the
base of the DMA region and use the offset param
(NV_PFALCON_FALCON_DMATRFFBOFFS), but it's not what openrm does. That is,
tag==false case it doesn't matter (?) how the offset is split between
`fb_offset` and `base_offset` is so might as well make it match the other cases:

```
let dma_base = match src {
    FalconDmaSource::Sysmem(dma_obj) => {
        let dma_end = num_transfers
            .checked_mul(DMA_LEN)
            .and_then(|size| size.checked_add(load_offsets.src_start))
            .map(usize::from_safe_cast)
            .ok_or(EOVERFLOW)?;

        io_project!(dma_obj, [try: 0..dma_end]).dma_address()
    }
    FalconDmaSource::Framebuffer { base } => base,
};
let dma_offset = load_offsets.src_start;
```