Re: [PATCH v2 13/15] gpu: nova-core: retrigger the GSP falcon and clear every latched cause

["Alexandre Courbot" <acourbot@nvidia.com>]

On Sat Aug 29, 2026 at 10:33 AM JST, John Hubbard wrote:
<...>
> diff --git a/drivers/gpu/nova-core/irq/gsp.rs b/drivers/gpu/nova-core/irq/gsp.rs
> index 6366380eef98..a1030d66cc70 100644
> --- a/drivers/gpu/nova-core/irq/gsp.rs
> +++ b/drivers/gpu/nova-core/irq/gsp.rs
> @@ -50,18 +50,17 @@
>  
>  /// Clears the interrupt state that GSP boot left behind.
>  ///
> -/// Disables every vector in every implemented leaf, clears the falcon's SWGEN0 latch, clears the
> -/// tree's pending bits, and rearms PCI interrupt delivery. On return no vector is enabled, so the
> -/// tree delivers nothing.
> +/// Disables every vector in every implemented leaf, clears the tree's pending bits, clears the
> +/// falcon's SWGEN0 latch, and rearms PCI interrupt delivery. On return no vector is enabled, so
> +/// the tree delivers nothing.
>  pub(crate) fn quiesce(bar: Bar0<'_>, chipset: Chipset, irq_type: pci::IrqType) {
>      let tree = Tree::new(bar, chipset, irq_type, GSP_SUBTREE.into());
>      tree.disable_all_leaves();
> -    // GSP boot consumes its notifications by polling the queue, which leaves SWGEN0 latched.
> -    // Clear it before the tree drain below, so the drain clears the tree state the clear sets.
> -    // Messages already posted raise no interrupt of their own, and the caller's queue drain
> -    // covers them.
> -    GspFalcon::clear_swgen0_intr(bar);
>      tree.drain();
> +    // GSP boot consumes its notifications by polling the queue, which leaves SWGEN0 latched, and
> +    // the GSP drives no new signal while it is set. Clear it after the tree drain, which erases
> +    // every leaf bit and would erase the one a message posted since the clear had set.
> +    GspFalcon::clear_swgen0_intr(bar);

This code was introduced in the previous patch and is now immediately
moved around, and the comments for the two versions contradict each
other. This patch's order seems to be the correct one, so let's use it
from the get-go.

I guess this stems from the general ordering problem with this patch: if
its new falcon registers were introduced before patch 12, then patch 12
could be written in its final form instead of having its code amended
right away. It would be nice if you could reorder things this way for
v3.

>      // The `TOP_EN` cycle in `drain` is the rearm for the two enable-cycle methods, but pre-Hopper
>      // MSI rearms through a configuration-space write instead. An interrupt delivered before probe
>      // leaves delivery un-armed on that path, with no handler to have rearmed it.
> @@ -80,6 +79,8 @@ pub(crate) struct GspInterrupt<'a> {
>      cmdq: Arc<Cmdq>,
>      /// The GIN interrupt tree for this chipset.
>      tree: Tree<'a>,
> +    /// Chipset, for the falcon retrigger, which Turing does not implement.
> +    chipset: Chipset,
>      /// Device, for logging from interrupt context without taking the command-queue lock.
>      dev: ARef<device::Device>,
>  }
> @@ -98,15 +99,18 @@ pub(crate) fn new(
>              bar,
>              cmdq,
>              tree: Tree::new(bar, chipset, irq_type, GSP_SUBTREE.into()),
> +            chipset,
>              dev,
>          }? Error)
>      }
>  }
>  
>  impl irq::ThreadedHandler for GspInterrupt<'_> {
> -    /// Top half: clears the GIN leaf, takes the falcon SWGEN0 latch, and rearms PCI interrupt
> -    /// delivery.
> +    /// Top half: clears the GIN leaf, takes every cause the falcon reports, and rearms PCI
> +    /// interrupt delivery.
>      fn handle(&self) -> irq::ThreadedIrqReturn {
> +        let bar = self.bar;
> +
>          // Only service our own vector: require the GSP bit in the leaf and clear just that bit, so
>          // a co-pending vector in the same leaf stays pending for whoever services it. The subtree
>          // stays enabled, so there is no whole-tree disable and enable.
> @@ -119,27 +123,40 @@ fn handle(&self) -> irq::ThreadedIrqReturn {
>          }
>          leaf.clear_vectors(GSP_INTR_0_VECTOR.leaf_mask());
>  
> -        // SWGEN0 is the message-queue notification, so wake the IRQ thread to drain it.
> -        let status = GspFalcon::take_swgen0_intr(self.bar);
> -        let ret = if status.swgen0() {
> -            irq::ThreadedIrqReturn::WakeThread
> -        } else {
> -            // The tree routes every falcon cause to this vector, so something other than a posted
> -            // message fired it, for example a HALT from a GSP crash. There is no recovery path for
> -            // those causes, so report the status rather than discarding it.
> +        let status = GspFalcon::take_swgen0_intr(bar);
> +
> +        // Every cause the falcon reports leaves the falcon's enabled set on this invocation. A
> +        // cause left latched holds that set non-empty, and the falcon signals the tree only on a
> +        // transition of the set, so no later SWGEN0 would signal at all.
> +        let unserviceable = status.with_swgen0(false);
> +        if unserviceable.into_raw() != 0 {
> +            // The tree routes every falcon cause to this vector, so a cause other than a posted
> +            // message also arrives here, for example a HALT from a GSP crash. nova-core has no
> +            // recovery path for those, so report the status rather than discarding it, then mask
> +            // the cause.
>              dev_err!(
>                  &self.dev,
> -                "GSP interrupt with no SWGEN0, falcon IRQSTAT {:#x}\n",
> +                "unserviceable GSP falcon interrupt, IRQSTAT {:#x}\n",
>                  status.into_raw()
>              );
> -            irq::ThreadedIrqReturn::Handled
> -        };
> +            GspFalcon::mask_and_clear_intr(bar, unserviceable);

Is this correct? My Clanker told me (and looking closer it seems to be
correct) that `IRQSTAT` also reports causes that are masked, or routed
to the RISC-V core itself rather than to the host (i.e. GSP-RM's own
interrupts), and that OpenRM filters these out: `kgspService_TU102()`
gets its status from `kflcnGetPendingHostInterrupts()`, which for a
RISC-V falcon is `FALCON_IRQSTAT & PRISCV_RISCV_IRQMASK &
PRISCV_RISCV_IRQDEST`. Without that filter, any GSP-internal cause that
happens to be latched when SWGEN0 fires gets reported here, then cleared
and masked out from under the firmware.

Also in RISC-V mode the mask gating host routing appears to be
`PRISCV_RISCV_IRQMASK`, not `FALCON_IRQMASK` - OpenRM never writes the
latter for the GSP. So the comment about the tree routing every falcon
cause here also seems inaccurate.