Re: [PATCH v2 10/10] gpu: nova-core: wait for RISC-V HALTED on FSP unload

On Thu Jul 23, 2026 at 12:59 PM JST, Eliot Courtney wrote:
> On Thu Jul 23, 2026 at 12:28 PM JST, Alexandre Courbot wrote:
>> On Fri Jul 3, 2026 at 3:22 AM PDT, Eliot Courtney wrote:
>>> Currently the code waits for "not active" but this is not the same as
>>> halted as there are more than two states. Match openrm here and wait for
>>> halted instead.
>>>
>>> Fixes: c7fea1f70944 ("gpu: nova-core: add non-sec2 unload path")
>>> Signed-off-by: Eliot Courtney <[email protected]>
>>> ---
>>>  drivers/gpu/nova-core/falcon.rs           | 11 +++++++++++
>>>  drivers/gpu/nova-core/falcon/hal.rs       |  5 +++++
>>>  drivers/gpu/nova-core/falcon/hal/ga102.rs |  7 +++++++
>>>  drivers/gpu/nova-core/falcon/hal/tu102.rs |  4 ++++
>>>  drivers/gpu/nova-core/gsp/hal/gh100.rs    | 11 +++++++++--
>>>  5 files changed, 36 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/nova-core/falcon.rs b/drivers/gpu/nova-core/falcon.rs
>>> index 78948cc8bff3..ec286017535f 100644
>>> --- a/drivers/gpu/nova-core/falcon.rs
>>> +++ b/drivers/gpu/nova-core/falcon.rs
>>> @@ -749,11 +749,22 @@ pub(crate) fn signature_reg_fuse_version(
>>>  
>>>      /// Check if the RISC-V core is active.
>>>      ///
>>> +    /// Note that this does not imply that the RISC-V core is halted if it returns `false`.
>>
>> I would say "guarantee" instead of "imply" here, wdyt?
>
> Yes I think "guarantee" is better here since otherwise you need the
> mathematical reading of "imply" to not think it's an if-and-only-if.
>
>>
>>> +    ///
>>>      /// Returns `true` if the RISC-V core is active, `false` otherwise.
>>>      pub(crate) fn is_riscv_active(&self) -> bool {
>>>          self.hal.is_riscv_active(self)
>>>      }
>>>  
>>> +    /// Checks whether the RISC-V core is halted.
>>> +    ///
>>> +    /// Note that this does not imply that the RISC-V core is active if it returns `false`.
>>
>> Same here.
>>
>>> +    ///
>>> +    /// Returns [`ENOTSUPP`] if the status is not available.
>>> +    pub(crate) fn is_riscv_halted(&self) -> Result<bool> {
>>> +        self.hal.is_riscv_halted(self)
>>> +    }
>>> +
>>>      /// Load a firmware image into Falcon memory, using the preferred method for the current
>>>      /// chipset.
>>>      pub(crate) fn load<F: FalconFirmware<Target = E> + FalconDmaLoadable>(&self, fw: &F) -> Result {
>>> diff --git a/drivers/gpu/nova-core/falcon/hal.rs b/drivers/gpu/nova-core/falcon/hal.rs
>>> index ee4a017f3a4c..7e532889a1f4 100644
>>> --- a/drivers/gpu/nova-core/falcon/hal.rs
>>> +++ b/drivers/gpu/nova-core/falcon/hal.rs
>>> @@ -53,6 +53,11 @@ fn signature_reg_fuse_version(
>>>      /// Returns `true` if the RISC-V core is active, `false` otherwise.
>>>      fn is_riscv_active(&self, falcon: &Falcon<'_, E>) -> bool;
>>>  
>>> +    /// Checks whether the RISC-V core is halted.
>>> +    ///
>>> +    /// Returns [`ENOTSUPP`] if the chipset does not expose RISC-V halt status.
>>> +    fn is_riscv_halted(&self, falcon: &Falcon<'_, E>) -> Result<bool>;
>>
>> It's a bit unfortunate that we add a runtime error check for something
>> that is essentially known to never fail (the GSP's GH100 HAL being the
>> sole caller of this method). I cannot think of a better design at
>> the moment, but this hints that our HAL could be improved as I think we
>> already have several instances of the same pattern elsewhere.
>>
>> Not a big deal anyway - if you can confirm my wording nit above, this
>> one should be good to merge as well.
>
> Yeah, I couldn't think of a good way to do this either and I was unhappy
> about it. Wording nit sgtm. Thanks!

Pushed to drm-rust-next! Thank you.