Re: [PATCH v2 12/15] gpu: nova-core: drive GSP events with the SWGEN0 interrupt

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

Hi John,

I am still looking at this one in depth, but wanted to post what I've
found so far before going to sleep.

On Sat Aug 29, 2026 at 10:33 AM JST, John Hubbard wrote:
> The GSP posts events, logs and error records to the GSP-to-CPU queue and
> raises the falcon SWGEN0 output. GSP boot polls for its own
> notifications, which leaves the latch set and pending bits in the tree.
>
> nova-core drained the queue only while polling for a command reply, so
> an event sat unread until the next command was sent.
>
> Service the queue from a threaded handler on the GSP notification
> vector. The top half runs in hard interrupt context and touches only
> registers: it clears the GIN leaf, takes the falcon's SWGEN0 latch and
> rearms PCI delivery. Draining the queue takes the command-queue mutex,
> which can sleep, so the top half wakes the IRQ thread to do it.
>
> Quiesce the tree, clear the latch and rearm PCI delivery before
> registering the handler, so none of that boot state reaches it.
> Pre-Hopper MSI rearms through a configuration-space write that the tree
> drain does not perform, and an interrupt delivered before probe leaves
> delivery un-armed.
>
> Move the vector allocation out of the self-test and into probe, because
> the vectors are allocated once for the whole PCI device rather than per
> handler. The self-test and the GSP handler each take the vector for the
> subtree they service.

Could the vector allocation have been put at its final place since patch
7?

<...>
> diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
> index 589b4b210a22..932e39e012f0 100644
> --- a/drivers/gpu/nova-core/gpu.rs
> +++ b/drivers/gpu/nova-core/gpu.rs
> @@ -10,7 +10,8 @@
>      num::Bounded,
>      pci,
>      prelude::*,
> -    sizes::SizeConstants, //
> +    sizes::SizeConstants,
> +    sync::Arc, //
>  };
>  
>  use crate::{
> @@ -25,10 +26,12 @@
>      fsp::Fsp,
>      gsp::{
>          self,
> +        cmdq::Cmdq,
>          commands::GetGspStaticInfoReply,
>          Gsp,
>          GspBootContext, //
>      },
> +    irq::SubtreeVectors,
>      vgpu::VgpuManager, //
>  };
>  
> @@ -323,12 +326,27 @@ fn drop(self: Pin<&mut Self>) {
>  }
>  
>  impl<'gpu> Gpu<'gpu> {
> +    /// Returns the chipset this GPU was identified as.
> +    pub(crate) fn chipset(&self) -> Chipset {
> +        self.spec.chipset
> +    }
> +
> +    /// Returns a shared handle to the GSP command queue.
> +    pub(crate) fn cmdq(&self) -> Arc<Cmdq> {
> +        self.gsp_resources.gsp.cmdq()
> +    }

I would expect that the interrupt rework series by Danilo would allow us
to borrow a reference to the `Cmdq` for the interrupt handler. Is there
a hard reason we cannot do it and need an `Arc`? `GspInterrupt` already
borrows `Bar0`, so this should not be different.

If the `Arc` is really needed (which I doubt, but still going through
the code), the `Arc` refactor should be in its own patch to make this
one a bit lighter and easier to review.

<...>
> diff --git a/drivers/gpu/nova-core/irq/doorbell_test.rs b/drivers/gpu/nova-core/irq/doorbell_test.rs
> index 3fd8b26e135e..c9712fa1bd18 100644
> --- a/drivers/gpu/nova-core/irq/doorbell_test.rs
> +++ b/drivers/gpu/nova-core/irq/doorbell_test.rs

I would not expect `doorbell_test.rs` to be affected by this patch -
please see if this can be squashed into patch 7.

<...>
> diff --git a/drivers/gpu/nova-core/irq/gsp.rs b/drivers/gpu/nova-core/irq/gsp.rs
> new file mode 100644
> index 000000000000..6366380eef98
> --- /dev/null
> +++ b/drivers/gpu/nova-core/irq/gsp.rs
> @@ -0,0 +1,215 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
> +
> +//! GSP event (SWGEN0) interrupt handling.
> +//!
> +//! The GSP firmware raises SWGEN0 when it has posted messages in the GSP-to-CPU queue. That
> +//! signal reaches the CPU as a PCI interrupt through the GIN tree. This module provides the
> +//! threaded IRQ handler for it. The top half services the GIN leaf and the falcon SWGEN0 latch,
> +//! and the IRQ thread drains the message queue.
> +//!
> +//! See `Documentation/gpu/nova/core/interrupts.rst`.
> +
> +use kernel::{
> +    device, irq, pci,
> +    prelude::*,
> +    sync::{
> +        aref::ARef,
> +        Arc, //
> +    },
> +};
> +
> +use super::{
> +    interrupt_tree::{
> +        GinVector,
> +        LeafEnableGuard,
> +        Subtree,
> +        Tree, //
> +    },
> +    SubtreeVectors, //
> +};
> +use crate::{
> +    driver::Bar0,
> +    falcon::gsp::Gsp as GspFalcon,
> +    gpu::Chipset,
> +    gsp::cmdq::Cmdq, //
> +};
> +
> +/// Fixed GSP notification vector.
> +///
> +/// The resource manager pins the GSP SWGEN0 notification to this vector on every supported chip,

nit: "GSP-RM pins the ..." for alignment with the rest of the docs.

> +/// so nova-core uses the constant directly instead of discovering it at runtime. The leaf and bit
> +/// serviced by the handler are derived from it.
> +const GSP_INTR_0_VECTOR: GinVector = GinVector::new::<155>();
> +
> +/// Subtree carrying the GSP notification vector, and the only subtree nova-core services.
> +///
> +/// Probe allocates PCI vectors for this subtree, and the GSP handler names it as the subtree it
> +/// serves, both when it takes its vector and when it rearms.
> +pub(crate) const GSP_SUBTREE: Subtree = GSP_INTR_0_VECTOR.subtree();
> +
> +/// 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.
> +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();
> +    // 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.
> +    tree.rearm_pci_irq(GSP_SUBTREE);
> +}
> +
> +/// Threaded IRQ handler for the GSP SWGEN0 event.
> +///
> +/// The top half clears the GIN leaf and reads the falcon SWGEN0 latch. The IRQ thread drains the
> +/// GSP-to-CPU message queue, which takes the command-queue lock.
> +#[pin_data]
> +pub(crate) struct GspInterrupt<'a> {
> +    /// Borrowed BAR0, for falcon register access from interrupt context.
> +    bar: Bar0<'a>,
> +    /// The GSP command queue, drained by the IRQ thread.
> +    cmdq: Arc<Cmdq>,

As mentioned above, I strongly suspect this can be a `&'a Cmdq`. Which
would be great as it would make the whole `Arc` refactoring unnecessary.

> +    /// The GIN interrupt tree for this chipset.
> +    tree: Tree<'a>,
> +    /// Device, for logging from interrupt context without taking the command-queue lock.
> +    dev: ARef<device::Device>,

This can be a `&'a device::Device` (tested locally).

Will post some more tomorrow but this is what I have so far. :)