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

[John Hubbard <jhubbard@nvidia.com>]

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