Re: [PATCH v4 14/17] gpu: nova-core: add the falcon interrupt registers and their HAL

[Alexandre Courbot]

On Sat Sep 12, 2026 at 5:43 AM BST, John Hubbard wrote:
> A falcon has a set of interrupt causes, and it latches each one that is
> raised in its IRQSTAT register. On a RISC-V falcon, each cause is routed
> either to the host, meaning the CPU, or to the falcon's own RISC-V core,
> and IRQSTAT holds the causes of both. Two more registers say which is
> which: PRISCV_RISCV_IRQMASK holds the enabled causes, and
> PRISCV_RISCV_IRQDEST holds the causes routed to the host. Open RM

nit: OpenRM. Let's be consistent through the series, I suspect there
other such spellings.

> intersects the three to get the causes that the host has to service,
> and nova-core does the same.
>
> A falcon signals the interrupt tree only when its set of host-routed
> causes goes from empty to non-empty. A handler that clears the tree
> leaf while a cause is still latched in the falcon leaves that set
> non-empty, so no later cause produces a transition, and the falcon's
> interrupts stop arriving. INTR_RETRIGGER makes the falcon re-emit its
> host-routed causes into the tree, which supplies the missing
> transition. Turing falcons do not implement it.
>
> IRQSCLR clears a cause's latch, but it cannot end the source behind the
> cause. A cause driven from outside the falcon, such as a fault
> containment or ECC error on Blackwell, stays set through the write.
>
> Add the four registers: IRQSTAT, INTR_RETRIGGER, and the two routing
> registers. Record the IRQSCLR limit on its existing definition. The
> routing registers go in per-chip modules, because their offsets move at
> GA102 rather than at the Turing-to-Ampere boundary.
>
> Add a HAL for the two properties that follow, the retrigger register
> and the routing offsets, which split the chipsets three ways:
>
> * Turing falcons have no retrigger register.
>
> * GA100 has the retrigger register, and keeps the Turing routing
>   offsets.
>
> * GA102 and later have the retrigger register, and their routing
>   registers moved.
>
> Keep this HAL apart from the falcon boot HAL. The boot HAL is generic
> over the falcon's engine type, so obtaining one is a heap allocation,
> and the interrupt handler that needs these two properties runs in hard
> interrupt context, where it cannot allocate.

This paragraph is not really accurate. We create the HAL in the Falcon
constructor and store it inside the instance (because of course we don't
perform a heap allocation every time we need to invoke a HAL function),
so the HAL is already created when interrupts occur and no allocation
occurs. The HALs could perfectly be shared.

The actual reason why they are split is because the interrupt handler
has no access to the Falcon instance, so the HAL must be queried every
time. Which, in this case, would indeed trigger an allocation, but the
fact the HAL is heap-allocated is not the root cause for the split.

Actually it could make sense to have the handler hold a reference to the
Falcon from a design perspective (to prevent e.g. loading a new firmware
while we are listening to the current one); I am just not sure it is
practical in the current state of things so let's keep things split for
now.

>
> Assisted-by: LLM
> Signed-off-by: John Hubbard <jhubbard@xxxxxxxxxx>
> ---
>  drivers/gpu/nova-core/falcon/hal.rs       | 80 ++++++++++++++++++++++-
>  drivers/gpu/nova-core/falcon/hal/ga102.rs | 21 +++++-
>  drivers/gpu/nova-core/falcon/hal/tu102.rs | 36 +++++++++-

With the "split the chipsets three ways" bit I would have expected 3
HALs files to be touched?

>  drivers/gpu/nova-core/regs.rs             | 69 +++++++++++++++++++
>  4 files changed, 202 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/nova-core/falcon/hal.rs b/drivers/gpu/nova-core/falcon/hal.rs
> index 7e532889a1f4..052610c4a4da 100644
> --- a/drivers/gpu/nova-core/falcon/hal.rs
> +++ b/drivers/gpu/nova-core/falcon/hal.rs
> @@ -1,17 +1,25 @@
>  // SPDX-License-Identifier: GPL-2.0
>  
> -use kernel::prelude::*;
> +use kernel::{
> +    io::{
> +        Io,
> +        Mmio, //
> +    },
> +    prelude::*, //
> +};
>  
>  use crate::{
>      falcon::{
>          Falcon,
>          FalconBromParams,
> -        FalconEngine, //
> +        FalconEngine,
> +        PFalcon2Registers, //
>      },
>      gpu::{
>          Architecture,
>          Chipset, //
>      },
> +    regs,
>  };
>  
>  mod ga102;
> @@ -72,6 +80,74 @@ fn signature_reg_fuse_version(
>      fn load_method(&self) -> LoadMethod;
>  }
>  
> +/// Offsets of a falcon's RISC-V interrupt routing registers.
> +#[derive(Clone, Copy, Debug, Eq, PartialEq)]
> +#[expect(dead_code)]
> +pub(crate) enum RiscvRouting {
> +    /// The Turing offsets. GA100 uses them too.
> +    Tu102,
> +
> +    /// The offsets from GA102 on.
> +    Ga102,
> +}
> +
> +impl RiscvRouting {
> +    /// Returns the causes in `latched` that are routed to the host, meaning the CPU, rather than
> +    /// to the falcon's own RISC-V core.
> +    ///
> +    /// The causes routed to the core belong to the firmware running on it, and the host does not
> +    /// service them.
> +    #[expect(dead_code)]
> +    pub(crate) fn host_routed_causes(
> +        self,
> +        pfalcon2: Mmio<'_, PFalcon2Registers>,
> +        latched: regs::NV_PFALCON_FALCON_IRQSTAT,
> +    ) -> regs::NV_PFALCON_FALCON_IRQSTAT {
> +        let (mask, dest) = match self {
> +            Self::Tu102 => (
> +                pfalcon2.read(regs::tu102::NV_PRISCV_RISCV_IRQMASK).value(),
> +                pfalcon2.read(regs::tu102::NV_PRISCV_RISCV_IRQDEST).value(),
> +            ),
> +            Self::Ga102 => (
> +                pfalcon2.read(regs::ga102::NV_PRISCV_RISCV_IRQMASK).value(),
> +                pfalcon2.read(regs::ga102::NV_PRISCV_RISCV_IRQDEST).value(),
> +            ),
> +        };

There is no need for a new type and dedicated dispatch, that just
negates the purpose of the HAL, which exactly the issue I pointed in the
previous revision.

Also the only reason `RiscvRouting` is used for outside of the HAL it to
call `host_routed_causes`, so the HAL just needs a `host_routed_causes`
method, with two different implementations, and you get the end result
without any extra steps.

> +
> +        regs::NV_PFALCON_FALCON_IRQSTAT::from(latched.into_raw() & mask & dest)
> +    }
> +}
> +
> +/// Interrupt properties of a falcon that differ by GPU family.
> +///
> +/// Separate from [`FalconHal`] because the GSP event handler calls these from hard interrupt
> +/// context, where it cannot make the heap allocation that a `FalconHal` takes.
> +#[expect(dead_code)]
> +pub(crate) trait FalconIntrHal {
> +    /// Returns whether these falcons implement `NV_PFALCON_FALCON_INTR_RETRIGGER`.
> +    fn has_intr_retrigger(&self) -> bool;

Here also there is little value in asking whether the Falcon supports
retrigger, only to do it on the caller right after. Let's just have a
`retrigger` method which is a no-op on Turing and does something on
anything above.

> +
> +    /// Returns the offsets of `PRISCV_RISCV_IRQMASK` and `PRISCV_RISCV_IRQDEST`.
> +    fn riscv_routing(&self) -> RiscvRouting;

And as mentioned above, this should just implement `host_routed_causes`
without another type in the middle.

> +}
> +
> +/// Returns the [`FalconIntrHal`] for `chipset`.
> +///
> +/// GA100 has its own arm: it has the retrigger register, which Turing lacks, and the Turing
> +/// routing offsets, which GA102 moved.
> +#[expect(dead_code)]
> +pub(crate) fn falcon_intr_hal(chipset: Chipset) -> &'static dyn FalconIntrHal {
> +    match chipset.arch() {
> +        Architecture::Turing => tu102::TU102_INTR_HAL,
> +        Architecture::Ampere if chipset == Chipset::GA100 => tu102::GA100_INTR_HAL,
> +        Architecture::Ampere
> +        | Architecture::Ada
> +        | Architecture::Hopper
> +        | Architecture::BlackwellGB10x
> +        | Architecture::BlackwellGB20x => ga102::GA102_INTR_HAL,
> +    }
> +}
> +
>  /// Returns a boxed falcon HAL adequate for `chipset`.
>  ///
>  /// We use a heap-allocated trait object instead of a statically defined one because the
> diff --git a/drivers/gpu/nova-core/falcon/hal/ga102.rs b/drivers/gpu/nova-core/falcon/hal/ga102.rs
> index f9a8444cf840..ff97983f22fe 100644
> --- a/drivers/gpu/nova-core/falcon/hal/ga102.rs
> +++ b/drivers/gpu/nova-core/falcon/hal/ga102.rs
> @@ -28,7 +28,11 @@
>      regs,
>  };
>  
> -use super::FalconHal;
> +use super::{
> +    FalconHal,
> +    FalconIntrHal,
> +    RiscvRouting, //
> +};
>  
>  fn select_core_ga102(pfalcon2: Mmio<'_, PFalcon2Registers>) -> Result {
>      let bcr_ctrl = pfalcon2.read(regs::NV_PRISCV_RISCV_BCR_CTRL);
> @@ -170,3 +174,18 @@ fn load_method(&self) -> LoadMethod {
>          LoadMethod::Dma
>      }
>  }
> +
> +/// The falcon interrupt properties of GA102 and later.
> +struct Ga102Intr;
> +
> +impl FalconIntrHal for Ga102Intr {
> +    fn has_intr_retrigger(&self) -> bool {
> +        true
> +    }
> +
> +    fn riscv_routing(&self) -> RiscvRouting {
> +        RiscvRouting::Ga102
> +    }
> +}
> +
> +pub(super) const GA102_INTR_HAL: &dyn FalconIntrHal = &Ga102Intr;
> diff --git a/drivers/gpu/nova-core/falcon/hal/tu102.rs b/drivers/gpu/nova-core/falcon/hal/tu102.rs
> index 7fc6e83c2566..f79aa85e6a62 100644
> --- a/drivers/gpu/nova-core/falcon/hal/tu102.rs
> +++ b/drivers/gpu/nova-core/falcon/hal/tu102.rs
> @@ -21,7 +21,11 @@
>      regs, //
>  };
>  
> -use super::FalconHal;
> +use super::{
> +    FalconHal,
> +    FalconIntrHal,
> +    RiscvRouting, //
> +};
>  
>  pub(super) struct Tu102<E: FalconEngine>(PhantomData<E>);
>  
> @@ -80,3 +84,33 @@ fn load_method(&self) -> LoadMethod {
>          LoadMethod::Pio
>      }
>  }
> +
> +/// The falcon interrupt properties of Turing.
> +struct Tu102Intr;
> +
> +impl FalconIntrHal for Tu102Intr {
> +    fn has_intr_retrigger(&self) -> bool {
> +        false
> +    }
> +
> +    fn riscv_routing(&self) -> RiscvRouting {
> +        RiscvRouting::Tu102
> +    }
> +}
> +
> +pub(super) const TU102_INTR_HAL: &dyn FalconIntrHal = &Tu102Intr;
> +
> +/// GA100's falcon interrupt properties: the Turing routing offsets and the retrigger register.
> +struct Ga100Intr;
> +
> +impl FalconIntrHal for Ga100Intr {
> +    fn has_intr_retrigger(&self) -> bool {
> +        true
> +    }
> +
> +    fn riscv_routing(&self) -> RiscvRouting {
> +        RiscvRouting::Tu102
> +    }
> +}
> +
> +pub(super) const GA100_INTR_HAL: &dyn FalconIntrHal = &Ga100Intr;

Yup, that's what I expected: the LLM didn't want to go through the labor
of creating a new HAL file and somehow decided to put the Ga100 HAL into
the tu102 module. The kind of stuff a human would have gotten right from
the start.

So this HAL should be in a `ga100` module. Thankfully there is no need
to duplicate the `FalconHal` (a nice side-effect of the split).