Re: [PATCH v4 14/20] gpu: nova-core: vbios: construct `FwSecBiosImage` directly from BIOS images

[John Hubbard]

On 5/18/26 7:55 PM, Eliot Courtney wrote:
> `FwSecBiosBuilder` now only contains `falcon_ucode_offset` which just
> gets passed directly into `FwSecBiosImage`. Remove `FwSecBiosBuilder`
> and construct `FwSecBiosImage` directly, as a simplification.
> 
> Reviewed-by: Joel Fernandes <joelagnelf@xxxxxxxxxx>
> Signed-off-by: Eliot Courtney <ecourtney@xxxxxxxxxx>
> ---
>  drivers/gpu/nova-core/vbios.rs | 98 +++++++++++++++++-------------------------
>  1 file changed, 39 insertions(+), 59 deletions(-)
...> @@ -353,15 +349,23 @@ pub(crate) fn new(dev: &device::Device, bar0: &Bar0) -> Result<Vbios> {
>          }
>  
>          // Using all the images, setup the falcon data pointer in Fwsec.
> -        if let (Some(mut second), Some(first), Some(pci_at)) =
> +        if let (Some(second), Some(first), Some(pci_at)) =
>              (second_fwsec_image, first_fwsec_image, pci_at_image)
>          {
> -            second
> -                .setup_falcon_data(&pci_at, &first)
> +            let fwsec_image = FwSecBiosImage::new(pci_at, first, second)
>                  .inspect_err(|e| dev_err!(dev, "Falcon data setup failed: {:?}\n", e))?;
> -            Ok(Vbios {
> -                fwsec_image: second.build()?,
> -            })
> +
> +            if cfg!(debug_assertions) {
> +                // Print the desc header for debugging

Both this patch, and patch 16/20 are doing a tiny bit of work to
preserve this printing. And that looks good.

However, after careful consideration over several months, I have
come to believe that this printing is no longer earning its keep,
even for debug-level printing.

A significant fraction of the dmesg debug level output is consumed
by this one print, for example:

nova-core 0000:01:00.0: PmuLookupTableEntry desc: V3(
    FalconUCodeDescV3 {
        hdr: 78381825,
        stored_size: 59904,
        pkc_data_offset: 1444,
        interface_offset: 28,
        imem_phys_base: 0,
        imem_load_size: 57856,
        imem_virt_base: 0,
        dmem_phys_base: 0,
        dmem_load_size: 2048,
        engine_id_mask: 1024,
        ucode_id: 9,
        signature_count: 3,
        signature_versions: 7,
        _reserved: 37449,
    },
)

...and yet it is exceedingly rare to make use of that particular
data, even when debugging.

Let's just delete it. As always, bringup people can add it back in
temporarily if they need it. But they likely never will, because new
hardware doesn't hit this path anyway.



> +                let desc = fwsec_image.header()?;
> +                dev_dbg!(
> +                    fwsec_image.base.dev,
> +                    "PmuLookupTableEntry desc: {:#?}\n",
> +                    desc
> +                );
> +            }
> +
thanks,
-- 
John Hubbard