Re: [PATCH v4 16/17] gpu: nova-core: add KUnit tests for the interrupt tree and HALs

[Alexandre Courbot]

On Sat Sep 12, 2026 at 5:43 AM BST, John Hubbard wrote:
<...>
> diff --git a/drivers/gpu/nova-core/falcon/hal.rs b/drivers/gpu/nova-core/falcon/hal.rs
> index 3f1f509eccbd..70bddcaf4266 100644
> --- a/drivers/gpu/nova-core/falcon/hal.rs
> +++ b/drivers/gpu/nova-core/falcon/hal.rs
> @@ -171,3 +171,51 @@ pub(super) fn falcon_hal<E: FalconEngine + 'static>(
>  
>      Ok(hal)
>  }
> +
> +#[kunit_tests(nova_core_falcon_hal)]
> +mod tests {
> +    use super::*;
> +
> +    /// Turing falcons have no retrigger register. GA100 and every later chipset have it.
> +    #[test]
> +    fn intr_retrigger_gate_per_arch() {
> +        for chipset in [Chipset::TU102, Chipset::TU116] {
> +            assert!(!falcon_intr_hal(chipset).has_intr_retrigger());
> +        }
> +
> +        for chipset in [
> +            Chipset::GA100,
> +            Chipset::GA102,
> +            Chipset::AD102,
> +            Chipset::GH100,
> +            Chipset::GB100,
> +            Chipset::GB202,
> +        ] {
> +            assert!(falcon_intr_hal(chipset).has_intr_retrigger());
> +        }
> +    }
> +
> +    /// The RISC-V routing offsets change at GA102, so GA100 still uses the Turing ones.
> +    #[test]
> +    fn riscv_routing_offsets_split_at_ga102() {
> +        for chipset in [Chipset::TU102, Chipset::TU116, Chipset::GA100] {
> +            assert_eq!(
> +                falcon_intr_hal(chipset).riscv_routing(),
> +                RiscvRouting::Tu102
> +            );
> +        }
> +
> +        for chipset in [
> +            Chipset::GA102,
> +            Chipset::AD102,
> +            Chipset::GH100,
> +            Chipset::GB100,
> +            Chipset::GB202,
> +        ] {
> +            assert_eq!(
> +                falcon_intr_hal(chipset).riscv_routing(),
> +                RiscvRouting::Ga102
> +            );
> +        }
> +    }

Unfortunately these tests will happily keep passing it we add an
architecture and forget to update the test. And anyway I think we want
to remove `RiscvRouting`, so that leaves us with nothing to test
against.

Which is honestly not much of a problem, since all these tests do it
checking that we did not make a mistake when assigning the HAL of each
chipset. Unit tests are more to ensure that some algorithm performs as
expected, here it's basically a low-value typo check. I think we can do
without such tests.

> +}
> diff --git a/drivers/gpu/nova-core/irq/hal.rs b/drivers/gpu/nova-core/irq/hal.rs
> index ede9a10ccda6..03852918013d 100644
> --- a/drivers/gpu/nova-core/irq/hal.rs
> +++ b/drivers/gpu/nova-core/irq/hal.rs
> @@ -88,3 +88,67 @@ pub(super) fn cpu_interrupt_hal(chipset: Chipset) -> &'static dyn CpuInterruptHa
>          }
>      }
>  }
> +
> +#[kunit_tests(nova_core_gin_hal)]
> +mod tests {
> +    use super::*;
> +
> +    use crate::gpu::Chipset;
> +
> +    /// Turing through Ada implement an 8-leaf tree.
> +    #[test]
> +    fn pre_hopper_tree_size() {
> +        for chipset in [Chipset::TU102, Chipset::GA102, Chipset::AD102] {
> +            assert_eq!(cpu_interrupt_hal(chipset).leaf_count(), LeafCount::Eight);
> +        }
> +    }
> +
> +    /// Hopper and later implement a 16-leaf tree.
> +    #[test]
> +    fn hopper_plus_tree_size() {
> +        for chipset in [Chipset::GH100, Chipset::GB100, Chipset::GB202] {
> +            assert_eq!(cpu_interrupt_hal(chipset).leaf_count(), LeafCount::Sixteen);
> +        }
> +    }

Same here, these two tests should be merged into one so we can catch the
case where we add a new architecture and overlook updating the test. But
here again I question their usefulness since they only restate something
that is already clearly laid out in the code.

> +
> +    /// MSI rearms through the configuration-space mirror only before Hopper. Hopper and later
> +    /// cycle the `TOP` enables of every serviced subtree.
> +    #[test]
> +    fn msi_rearm_method_per_arch() {
> +        for chipset in [Chipset::TU102, Chipset::GA102, Chipset::AD102] {
> +            let hal = cpu_interrupt_hal(chipset);
> +            assert_eq!(
> +                hal.pci_irq_rearm_method(MsiType::Msi),
> +                PciIrqRearmMethod::ConfigMirrorEoi
> +            );
> +        }
> +
> +        for chipset in [Chipset::GH100, Chipset::GB100, Chipset::GB202] {
> +            let hal = cpu_interrupt_hal(chipset);
> +            assert_eq!(
> +                hal.pci_irq_rearm_method(MsiType::Msi),
> +                PciIrqRearmMethod::TopEnableCycleServiced
> +            );
> +        }
> +    }
> +
> +    /// MSI-X rearms one subtree on every architecture, since each subtree has its own table
> +    /// entry.
> +    #[test]
> +    fn msix_rearms_one_subtree_on_every_arch() {
> +        for chipset in [
> +            Chipset::TU102,
> +            Chipset::GA102,
> +            Chipset::AD102,
> +            Chipset::GH100,
> +            Chipset::GB100,
> +            Chipset::GB202,
> +        ] {
> +            let hal = cpu_interrupt_hal(chipset);
> +            assert_eq!(
> +                hal.pci_irq_rearm_method(MsiType::MsiX),
> +                PciIrqRearmMethod::TopEnableCycleSubtree
> +            );
> +        }
> +    }

Same issue here. The rest of the tests look like they are actually
testing functionality, so they are ok to keep.