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

[Alexandre Courbot]

On Mon Nov 9, 2026 at 4:23 PM JST, Eliot Courtney wrote:
> 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@xxxxxxxxxx>
>> ---
>>  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;
> ```

I like your first option because it is basically a no-op with sysmem
(and also the diff looks nicer).

Option 2 makes the code considerably smaller and easier to understand.
The offset we store in `NV_PFALCON_FALCON_DMATRFFBOFFS` becomes bigger,
but that should still be ok as it is 32 bits. We are computing
`end_offset` as a `u32` anyway, so we would have overflow if this was a
problem.

I've tried it with both r570 and r000 and both passed, so since it is
shorter I will add a patch to switch to it (having it in a separate
commit makes it easier to revert if need be).