Re: [PATCH v3 17/33] gpu: nova-core: add the r000 load-and-execute bootloader handler

[John Hubbard]

On 9/17/26 8:32 PM, Timur Tabi wrote:
> On Thu, 2026-09-17 at 18:07 -0700, John Hubbard wrote:
>> @@ -133,6 +141,75 @@ fn core_resume(&self) -> Result {
>>          Ok(())
>>      }
>>  
>> +    /// Runs the generic bootloader on the GSP falcon, as a
>> `GMCAPI_CMD_EXEC_GENERIC_BOOTLOADER`
>> +    /// event requests, and then restarts GSP-RM.
>> +    ///
>> +    /// The descriptor that the event carries names the image that the bootloader loads.
>> +    ///
>> +    /// # Errors
>> +    ///
>> +    /// - `EINVAL` if this chipset boots without the generic bootloader, if the payload is
>> shorter
>> +    ///   than the parameter block, if the descriptor is not the size that this driver
>> defines for
>> +    ///   it, or if the event names a context DMA slot or an aperture that does not exist.
>> +    /// - `ETIMEDOUT` if the RISC-V core does not suspend within two seconds, or the GSP
>> falcon does
>> +    ///   not halt within two seconds of starting the image.
>> +    ///
>> +    /// Errors from [`Self::core_resume`] are propagated as-is.
>> +    #[expect(dead_code)]
>> +    fn handle_load_exec_bootloader(&self, payload_0: &[u8], payload_1: &[u8]) -> Result {
>> +        let Self {
>> +            gsp_falcon, dev, ..
>> +        } = *self;
> 
> Why do you create local variables of two member of LoadExecContext?  By doing this:
> 
>         let Self {
>             gsp_falcon, dev, ..
>         } = *self;
> 
> you force every member of LoadExecContext to implement Copy, even the ones you are not copying
> here.
> 

Oh, that's not true! The ".." skips the other items, so there is no such
copying going on. Only the two named items are copied.

However, this is also just a little too unusual (Nova doesn't do this
little trick anywhere else), so I'm taking your advice and just Not
Doing It anymore, good call. :)


>> +        if params.dmem_desc_size != BootloaderDmemDescV2::SIZE {
>> +            dev_err!(
>> +                dev,
>> +                "Load-exec descriptor is {} bytes, expected {}\n",
>> +                params.dmem_desc_size,
>> +                BootloaderDmemDescV2::SIZE
>> +            );
> 
> So here, why not just do self.dev?

Yes, I'll go back to doing that.

thanks,
-- 
John Hubbard