Re: [PATCH 1/4] gpu: nova-core: move GSP unload state to a pinned Gpu subobject

[Alexandre Courbot]

On Wed Jun 10, 2026 at 7:14 PM JST, Gary Guo wrote:
> On Tue Jun 9, 2026 at 9:03 AM BST, Alexandre Courbot wrote:
>> `Gpu` currently owns the state needed to unload the GSP directly. This
>> means that `unload_bundle` has to be the last initialized field: once GSP
>> boot succeeds, any later initialization failure would leave `Gpu`
>> partially initialized, and its `PinnedDrop` implementation would not run.
>>
>> This prevents adding fallible `Gpu` fields that need to query the GSP
>> after it has booted.
>>
>> Move the GSP state and unload bundle into a dedicated pinned
>> `GspResources` object. Once that subobject has been initialized, its
>> `PinnedDrop` implementation will run even if initialization of a later
>> `Gpu` field fails, ensuring that the GSP unload sequence is executed.
>>
>> Signed-off-by: Alexandre Courbot <acourbot@xxxxxxxxxx>
>> ---
>>  drivers/gpu/nova-core/gpu.rs | 86 +++++++++++++++++++++++++-------------------
>>  1 file changed, 49 insertions(+), 37 deletions(-)
>>
>> diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
>> index b3c91731db45..6b3e02c71dee 100644
>> --- a/drivers/gpu/nova-core/gpu.rs
>> +++ b/drivers/gpu/nova-core/gpu.rs
>> @@ -262,35 +262,59 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
>>      }
>>  }
>>  
>> -/// Structure holding the resources required to operate the GPU.
>> +/// Self-contained resources to operate and drop the GSP.
>>  #[pin_data(PinnedDrop)]
>> -pub(crate) struct Gpu<'gpu> {
>> +struct GspResources<'gpu> {
>>      /// Device owning the GPU.
>>      device: &'gpu device::Device<device::Bound>,
>> -    spec: Spec,
>>      /// MMIO mapping of PCI BAR 0.
>>      bar: Bar0<'gpu>,
>> -    /// System memory page required for flushing all pending GPU-side memory writes done through
>> -    /// PCIE into system memory, via sysmembar (A GPU-initiated HW memory-barrier operation).
>> -    sysmem_flush: SysmemFlush<'gpu>,
>>      /// GSP falcon instance, used for GSP boot up and cleanup.
>>      gsp_falcon: Falcon<GspFalcon>,
>>      /// SEC2 falcon instance, used for GSP boot up and cleanup.
>>      sec2_falcon: Falcon<Sec2Falcon>,
>> -    /// GSP runtime data. Temporarily an empty placeholder.
>> +    /// GSP runtime data.
>>      #[pin]
>>      gsp: Gsp,
>>      /// GSP unload firmware bundle, if any.
>>      unload_bundle: Option<gsp::UnloadBundle>,
>
> I suppose this field is for unloading only? If so, you could just create a new
> type that stores `device`, `bar`, `bundle` only, which works as `device` and
> `bar` are Copy.
>
> struct UnloadGuard<'gpu> {
>     deivce: &'gpu device::Device<device::Bound>,
>     bar: Bar0<'gpu>,
>     unload_bundle: Option<gsp::UnloadBundle>,
> }
>
> impl Drop for UnloadGuard {
>     ...
> }
>
> This does mean that the `device` and `bar` are stored
> repeatedly, but this way you get a very clean way of representing this: once
> `UnloadGuard` is created unload will happen.

For `UnloadGuard` to be able to run the unload sequence, it would also
need a reference to the `Gsp` itself, and to the GSP and SEC2 falcons -
which would basically turn it into `GspResources` unless I misunderstood
your suggestion.

(the falcons are owned by `GspResources` because I want to make their
`load` and `run` methods take `&mut self` soon)

`GspResources` also already stores `device` and `bar` in the fashion you
suggested, for the same reason.