Re: [PATCH v6 11/11] gpu: nova-core: add PIO support for loading firmware images

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

On Sat Jan 17, 2026 at 6:05 AM JST, Danilo Krummrich wrote:
<snip>
>> +                    regs::NV_PFALCON_FALCON_IMEMT::default()
>> +                        .set_tag(tag)
>> +                        .write(bar, &E::ID, port);
>> +                    for word in block.chunks_exact(4) {
>> +                        let w = [word[0], word[1], word[2], word[3]];
>> +                        regs::NV_PFALCON_FALCON_IMEMD::default()
>> +                            .set_data(u32::from_le_bytes(w))
>> +                            .write(bar, &E::ID, port);
>> +                    }
>> +                    tag += 1;
>> +                }
>> +            }
>> +            FalconMem::Dmem => {
>> +                regs::NV_PFALCON_FALCON_DMEMC::default()
>> +                    .set_aincw(true)
>> +                    .set_offs(mem_base)
>> +                    .write(bar, &E::ID, port);
>> +
>> +                for block in img.chunks(256) {
>> +                    for word in block.chunks_exact(4) {
>> +                        let w = [word[0], word[1], word[2], word[3]];
>> +                        regs::NV_PFALCON_FALCON_DMEMD::default()
>> +                            .set_data(u32::from_le_bytes(w))
>> +                            .write(bar, &E::ID, port);
>> +                    }
>> +                }
>> +            }
>> +        }
>> +
>> +        Ok(())
>> +    }
>> +
>> +    fn pio_wr<F: FalconFirmware<Target = E>>(
>> +        &self,
>> +        bar: &Bar0,
>> +        fw: &F,
>> +        target_mem: FalconMem,
>> +        load_offsets: &FalconLoadTarget,
>> +        port: u8,
>> +        tag: u16,
>> +    ) -> Result {
>> +        let start = usize::from_safe_cast(load_offsets.src_start);
>> +        let len = usize::from_safe_cast(load_offsets.len);
>> +        let mem_base = u16::try_from(load_offsets.dst_start)?;
>> +
>> +        // SAFETY: we are the only user of the firmware image at this stage
>> +        let data = unsafe { fw.as_slice(start, len).map_err(|_| EINVAL)? };
>
> Why do we need the firmware image to be backed by a DMA object at this point
> when you load the firmware image through PIO anyways?

When we request the firmware, we don't know whether it is going to be
loaded via DMA or PIO. DMA does require a DMA object though, so the safe
route is to always create one and access its slice if it turns out we
will be doing PIO.

Another way would be to put the firmware into a vector and create a DMA
object on-demand in `dma_load`, but that's a more important refactoring
that we can probably keep for after this patchset should we want to do
it.

>> +}
>> +
>>  /// The FWSEC microcode, extracted from the BIOS and to be run on the GSP falcon.
>>  ///
>>  /// It is responsible for e.g. carving out the WPR2 region as the first step of the GSP bootflow.
>> @@ -221,6 +286,8 @@ pub(crate) struct FwsecFirmware {
>>      desc: FalconUCodeDesc,
>>      /// GPU-accessible DMA object containing the firmware.
>>      ucode: FirmwareDmaObject<Self, Signed>,
>> +    /// Generic bootloader
>> +    gen_bootloader: Option<GenericBootloader>,
>
> I'm not convinced this is a good idea. We probably want a HAL here and have
> different FwsecFirmware types:
>
> One with a DMA object and one with a system memory object when the architecture
> uses PIO. In the latter case the object can have a GenericBootloader field, i.e.
> this also gets us rid of the Option and all the subsequent 'if chipset <
> Chipset::GA102' checks and 'match gbl_fw' matches below.

Yeah that's basically what I have in mind as well - the FW type using
the generic bootloader could wrap the regular FwsecFirmware as its
purpose is simply to load it. I'm trying to put together some code to
that effect but it looks like it would work and would also remove the
special-case code in the PIO loader.