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

[Eliot Courtney]

On Sat May 23, 2026 at 9:13 AM JST, John Hubbard wrote:
> 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.

Yeah, good idea - I am glad to hear tbh. I wanted to delete these when
working on this but I wasn't completely sure what people find useful
for debugging etc so I left it alone. Thanks!