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

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

On Thu Sep 3, 2026 at 12:15 PM JST, John Hubbard wrote:
> The GSP posts events, logs and error records to the GSP-to-CPU queue and
> raises the falcon SWGEN0 output. A falcon signals the interrupt tree
> only on a transition of the causes it routes to the host, and IRQSTAT
> also reports the causes the falcon keeps for its own RISC-V core. GSP
> boot polls for its own notifications, so it leaves the SWGEN0 latch set
> and leaves pending bits behind 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 causes pending for the
> host, writes INTR_RETRIGGER so that a cause arriving while the top half
> runs still signals the tree, 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.
>
> Intersect IRQSTAT with the RISC-V routing registers the way Open RM
> does, so the firmware's own causes are left alone. Clear the latch of a
> host cause that is not a posted message, since nova-core has no recovery
> path for one and the retrigger would raise it again.
>
> Put the interrupt setup on the GPU rather than in the driver's probe.
> The handler is then torn down before the queue it drains is freed, and
> before the GSP is unloaded. Quiesce the tree and clear the latch before
> registering, so no boot state reaches the handler, and keep the subtree
> enabled at TOP for as long as the handler is registered. Quiescing
> disables the subtree, and under pre-Hopper MSI the rearm is a
> configuration-space write that never enables it again.

So this is a mishmash of many different things which makes it very
tedious to review. The falcon HAL stuff belongs in patch 11, the new
`SubtreeSet` method in patch 3, `irq.rs` changes where relevant, the
Cmdq::drain should be its own patch, and this patch should really just
add the handler and wire things together.

> Assisted-by: Cursor:claude-opus-5

On this revision the AI assistance showed mostly in the tedious comments
restating what the code does and the unneeded churn. These really take a
toll in terms of time and energy (and dare I say motivation). We need a
more thorough human pre-submit pass because otherwise the net effect is
a shift of labor onto reviewers, whose bandwidth is very limited.

<...>
> diff --git a/drivers/gpu/nova-core/falcon/hal.rs b/drivers/gpu/nova-core/falcon/hal.rs
> index 7e532889a1f4..5272b3b63ae4 100644
> --- a/drivers/gpu/nova-core/falcon/hal.rs
> +++ b/drivers/gpu/nova-core/falcon/hal.rs
> @@ -1,8 +1,15 @@
>  // SPDX-License-Identifier: GPL-2.0
>  
> -use kernel::prelude::*;
> +use kernel::{
> +    io::{
> +        register::WithBase,
> +        Io, //
> +    },
> +    prelude::*, //
> +};
>  
>  use crate::{
> +    driver::Bar0,
>      falcon::{
>          Falcon,
>          FalconBromParams,
> @@ -12,6 +19,7 @@
>          Architecture,
>          Chipset, //
>      },
> +    regs,
>  };
>  
>  mod ga102;
> @@ -72,6 +80,45 @@ fn signature_reg_fuse_version(
>      fn load_method(&self) -> LoadMethod;
>  }
>  
> +/// Returns whether `chipset`'s falcons implement `NV_PFALCON_FALCON_INTR_RETRIGGER`.
> +///
> +/// Turing falcons do not. Ampere and later do, including GA100, whose falcon otherwise uses the
> +/// Turing HAL, so this is keyed on the architecture rather than provided through [`FalconHal`].
> +pub(crate) fn has_intr_retrigger(chipset: Chipset) -> bool {
> +    !matches!(chipset.arch(), Architecture::Turing)
> +}
> +
> +/// Returns whether `chipset` carries the RISC-V interrupt routing registers at the Turing
> +/// offsets.
> +///
> +/// GA102 moved `NV_PRISCV_RISCV_IRQMASK` and `NV_PRISCV_RISCV_IRQDEST`, and GA100 kept the Turing
> +/// offsets, which is also why [`falcon_hal`] gives GA100 the Turing HAL.
> +fn has_turing_riscv_routing(chipset: Chipset) -> bool {
> +    matches!(chipset.arch(), Architecture::Turing) || chipset == Chipset::GA100
> +}
> +
> +/// Returns the interrupt causes a RISC-V falcon on `chipset` routes to the host, in the layout of
> +/// `NV_PFALCON_FALCON_IRQSTAT`.
> +///
> +/// A cause reaches the host only if the RISC-V core both enables it and directs it there, which
> +/// `NV_PRISCV_RISCV_IRQMASK` and `NV_PRISCV_RISCV_IRQDEST` say. Every other latched cause belongs
> +/// to the firmware running on the core.
> +pub(crate) fn host_intr_routing<E: FalconEngine>(bar: Bar0<'_>, chipset: Chipset) -> u32 {
> +    if has_turing_riscv_routing(chipset) {
> +        bar.read(regs::tu102::NV_PRISCV_RISCV_IRQMASK::of::<E>())
> +            .value()
> +            & bar
> +                .read(regs::tu102::NV_PRISCV_RISCV_IRQDEST::of::<E>())
> +                .value()
> +    } else {
> +        bar.read(regs::ga102::NV_PRISCV_RISCV_IRQMASK::of::<E>())
> +            .value()
> +            & bar
> +                .read(regs::ga102::NV_PRISCV_RISCV_IRQDEST::of::<E>())
> +                .value()
> +    }
> +}
> +

Why not use regular HAL methods here? This completely breaks the pattern
we introduced for HALs. If the current HALs don't fit the routing you
need, then we should introduce a new one.