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

[John Hubbard]

On 9/6/26 11:59 PM, Alexandre Courbot wrote:
> 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.

Yes, sorry about that, I have been doing that for v4 actually and
it should be much better there.

> <...>
> > 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.

Yes, will do.

thanks,
--
John Hubbard