Re: [PATCH v4 06/13] gpu: nova-core: gsp: fold TU102 unload bundle construction into HAL method

On Mon Jun 29, 2026 at 11:09 PM JST, Alexandre Courbot wrote:
> The construction of the unload bundle is currently a bit convoluted and
> could be done in one function instead of two.
>
> Additionally, turn that function into a method of `Tu102`. A following
> patch will turn the "use FWSEC bootloader" property into a flag of the
> TU102 HAL itself, and making this a method will allow the code to access
> it instead of querying `Chipset`.
>
> Signed-off-by: Alexandre Courbot <[email protected]>
> ---
>  drivers/gpu/nova-core/gsp/hal/tu102.rs | 87 ++++++++++++++++------------------
>  1 file changed, 41 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/gpu/nova-core/gsp/hal/tu102.rs b/drivers/gpu/nova-core/gsp/hal/tu102.rs
> index ef465b99af05..0505a5beee6e 100644
> --- a/drivers/gpu/nova-core/gsp/hal/tu102.rs
> +++ b/drivers/gpu/nova-core/gsp/hal/tu102.rs
> @@ -56,22 +56,6 @@ enum FwsecUnloadFirmware {
>  }
>  
>  impl FwsecUnloadFirmware {
> -    /// Loads the FWSEC SB firmware, as well as its bootloader if `chipset` requires it.
> -    fn new(
> -        dev: &device::Device<device::Bound>,
> -        chipset: Chipset,
> -        bios: &Vbios,
> -        gsp_falcon: &Falcon<'_, GspEngine>,
> -    ) -> Result<Self> {
> -        let fwsec_sb = FwsecFirmware::new(dev, gsp_falcon, bios, FwsecCommand::Sb)?;
> -
> -        Ok(if chipset.needs_fwsec_bootloader() {
> -            Self::WithBl(FwsecFirmwareWithBl::new(fwsec_sb, dev, chipset)?)
> -        } else {
> -            Self::WithoutBl(fwsec_sb)
> -        })
> -    }
> -
>      /// Runs the FWSEC SB firmware.
>      fn run(
>          &self,
> @@ -93,33 +77,6 @@ struct Sec2UnloadBundle {
>      booter_unloader: BooterFirmware,
>  }
>  
> -impl Sec2UnloadBundle {
> -    /// Load and prepare the resources required to properly reset the GSP after it has been stopped.
> -    fn build(
> -        dev: &device::Device<device::Bound>,
> -        chipset: Chipset,
> -        bios: &Vbios,
> -        gsp_falcon: &Falcon<'_, GspEngine>,
> -        sec2_falcon: &Falcon<'_, Sec2>,
> -    ) -> Result<KBox<dyn UnloadBundle>> {
> -        KBox::new(
> -            Self {
> -                fwsec_sb: FwsecUnloadFirmware::new(dev, chipset, bios, gsp_falcon)?,
> -                booter_unloader: BooterFirmware::new(
> -                    dev,
> -                    BooterKind::Unloader,
> -                    chipset,
> -                    FIRMWARE_VERSION,
> -                    sec2_falcon,
> -                )?,
> -            },
> -            GFP_KERNEL,
> -        )
> -        .map(|b| b as KBox<dyn UnloadBundle>)
> -        .map_err(Into::into)
> -    }
> -}
> -
>  impl UnloadBundle for Sec2UnloadBundle {
>      fn run(&self, ctx: &GspBootContext<'_>) -> Result {
>          let dev = ctx.dev();
> @@ -257,6 +214,44 @@ fn run_fwsec_frts(
>  
>  struct Tu102;
>  
> +impl Tu102 {
> +    /// Load and prepare the resources required to properly reset the GSP after it has been stopped.
> +    fn build_unload_bundle(
> +        &self,
> +        dev: &device::Device<device::Bound>,
> +        chipset: Chipset,
> +        bios: &Vbios,
> +        gsp_falcon: &Falcon<'_, GspEngine>,
> +        sec2_falcon: &Falcon<'_, Sec2>,
> +    ) -> Result<crate::gsp::UnloadBundle> {
> +        // Load the FWSEC SB firmware, as well as its bootloader if required.
> +        let fwsec_sb =
> +            FwsecFirmware::new(dev, gsp_falcon, bios, FwsecCommand::Sb).and_then(|fwsec_sb| {
> +                Ok(if chipset.needs_fwsec_bootloader() {
> +                    FwsecUnloadFirmware::WithBl(FwsecFirmwareWithBl::new(fwsec_sb, dev, chipset)?)
> +                } else {
> +                    FwsecUnloadFirmware::WithoutBl(fwsec_sb)
> +                })
> +            })?;

nit: Maybe nicer to write this like:
```
// Load the FWSEC SB firmware, as well as its bootloader if required.
let fwsec_sb = FwsecFirmware::new(dev, gsp_falcon, bios, FwsecCommand::Sb)?;
let fwsec_sb = if self.needs_fwsec_bootloader {
    FwsecUnloadFirmware::WithBl(FwsecFirmwareWithBl::new(fwsec_sb, dev, chipset)?)
} else {
    FwsecUnloadFirmware::WithoutBl(fwsec_sb)
};
```

With that,

Reviewed-by: Eliot Courtney <[email protected]>

> +
> +        KBox::new(
> +            Sec2UnloadBundle {
> +                fwsec_sb,
> +                booter_unloader: BooterFirmware::new(
> +                    dev,
> +                    BooterKind::Unloader,
> +                    chipset,
> +                    FIRMWARE_VERSION,
> +                    sec2_falcon,
> +                )?,
> +            },
> +            GFP_KERNEL,
> +        )
> +        .map(|b| crate::gsp::UnloadBundle(b))
> +        .map_err(Into::into)
> +    }
> +}
> +
>  impl GspHal for Tu102 {
>      fn boot(
>          &self,
> @@ -277,10 +272,10 @@ fn boot(
>          //
>          // If the unload bundle creation fails, the GPU will need to be reset before the driver can
>          // be probed again.
> -        let unload_bundle = Sec2UnloadBundle::build(dev, chipset, &bios, gsp_falcon, sec2_falcon)
> +        let unload_bundle = self
> +            .build_unload_bundle(dev, chipset, &bios, gsp_falcon, sec2_falcon)
>              .inspect_err(|e| dev_warn!(dev, "Failed to prepare unload firmware: {:?}\n", e))
> -            .ok()
> -            .map(crate::gsp::UnloadBundle);
> +            .ok();
>  
>          // Run the unload bundle to try and recover the GSP if an error occurs.
>          let unload_guard = ScopeGuard::new_with_data(unload_bundle, |unload_bundle| {