Re: [PATCH 5/7] gpu: nova-core: falcon: allow DMEM DMA transfers to request tagging

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] gpu: nova-core: falcon: dma_wr() tags DMEM from 0 instead of src_start
--

commit 56cacdd1fe2703c8ab2ff5d3ae42bbd82138503b
Author: Alexandre Courbot <acourbot@nvidia.com>

gpu: nova-core: falcon: allow DMEM DMA transfers to request tagging

This commit adds a tag flag to FalconMem::Dmem and uses it to program the
DMTAG bit in NV_PFALCON_FALCON_DMATRFCMD during Falcon DMA transfers.

> diff --git a/drivers/gpu/nova-core/falcon.rs b/drivers/gpu/nova-core/falcon.rs
> index 1bceb38c33c66..f48f2eabab0f4 100644
> --- a/drivers/gpu/nova-core/falcon.rs
> +++ b/drivers/gpu/nova-core/falcon.rs
> @@ -131,8 +131,9 @@ pub(crate) enum FalconMem {
>      /// Non-Secure Instruction Memory.
>      #[expect(unused)]
>      ImemNonSecure,
> -    /// Data Memory.
> -    Dmem,
> +    /// Data Memory. If `tag` is set, a transfer into DMEM also tags each block with its source
> +    /// offset, which is then the DMEM virtual address of the image.
> +    Dmem { tag: bool },
>  }
>  
>  bounded_enum! {
> @@ -529,9 +530,9 @@ fn dma_wr(
>          // code addresses in the firmware (and the boot vector) are virtual.
>          //
>          // For DMEM, the start offset is folded into the DMA address.
> -        let (src_start, dma_start) = match target_mem {
> -            FalconMem::ImemSecure | FalconMem::ImemNonSecure => (load_offsets.src_start, 0),
> -            FalconMem::Dmem => (0, usize::from_safe_cast(load_offsets.src_start)),
> +        let (src_start, dma_start, tag_dmem) = match target_mem {
> +            FalconMem::ImemSecure | FalconMem::ImemNonSecure => (load_offsets.src_start, 0, false),
> +            FalconMem::Dmem { tag } => (0, usize::from_safe_cast(load_offsets.src_start), tag),
>          };

[Severity: Medium]
When Falcon::dma_wr() in drivers/gpu/nova-core/falcon.rs is called with
FalconMem::Dmem { tag: true } and a non-zero load_offsets.src_start, does
this end up tagging the DMEM blocks starting at 0 instead of
load_offsets.src_start?

The comment on FalconMem::Dmem states that if tag is set, each block is
tagged with its source offset as the DMEM virtual address. However,
Falcon::dma_wr() unconditionally sets src_start to 0 and folds
load_offsets.src_start into dma_start for FalconMem::Dmem { tag }, and then
programs NV_PFALCON_FALCON_DMATRFFBOFFS in the transfer loop with
src_start + pos:

    for pos in (0..num_transfers).map(|i| i * DMA_LEN) {
        self.pfalcon.write_reg(
            regs::NV_PFALCON_FALCON_DMATRFMOFFS::zeroed()
                .try_with_offs(load_offsets.dst_start + pos)?,
        );
        self.pfalcon.write_reg(
            regs::NV_PFALCON_FALCON_DMATRFFBOFFS::zeroed().with_offs(src_start + pos),
        );

Because the Falcon DMA engine uses NV_PFALCON_FALCON_DMATRFFBOFFS as the
block tag when set_dmtag is enabled, setting src_start to 0 tags blocks
with pos rather than load_offsets.src_start + pos. Note that follow-up
commit 5a1fab864a35 updates the VRAM path to pass src_start, but still
leaves src_start as 0 in the FalconDmaSource::Sysmem arm when tag is true.

Should FalconMem::Dmem { tag: true } keep src_start = load_offsets.src_start
and dma_start = 0, matching the IMEM path?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103-r000-fw-v1-0-e3d38fc130e3@nvidia.com?part=5