Nouveau - Oct 2025 - [PATCH 5/7] gpu: nova-core: Add support for managing GSP falcon interrupts

On 10/20/25 11:55 AM, Joel Fernandes wrote:
> Add support for managing GSP falcon interrupts. These are required for
> GSP message queue interrupt handling.
> 
> Also rename clear_swgen0_intr() to enable_msq_interrupt() for
> readability.
Hi Joel,

I have a few comments below, including one that doesn't apply to you,
but to Alex Courbot.

Also, other than some trivia below, I can't find any problems with this
patch, other than possibly the above commit message wording, so
regardless of what we do with the .alter() method, please feel free to
add:

Reviewed-by: John Hubbard <jhubbard at nvidia.com>
> 
> Signed-off-by: Joel Fernandes <joelagnelf at nvidia.com>
> ---
>  drivers/gpu/nova-core/falcon/gsp.rs | 26 +++++++++++++++++++++++---
>  drivers/gpu/nova-core/gpu.rs        |  2 +-
>  drivers/gpu/nova-core/regs.rs       | 10 ++++++++++
>  3 files changed, 34 insertions(+), 4 deletions(-)

> 
> diff --git a/drivers/gpu/nova-core/falcon/gsp.rs
b/drivers/gpu/nova-core/falcon/gsp.rs
> index f17599cb49fa..6da63823996b 100644
> --- a/drivers/gpu/nova-core/falcon/gsp.rs
> +++ b/drivers/gpu/nova-core/falcon/gsp.rs
> @@ -22,11 +22,31 @@ impl FalconEngine for Gsp {
>  }
>  
>  impl Falcon<Gsp> {
> -    /// Clears the SWGEN0 bit in the Falcon's IRQ status clear
register to
> -    /// allow GSP to signal CPU for processing new messages in message
queue.
> -    pub(crate) fn clear_swgen0_intr(&self, bar: &Bar0) {
> +    /// Enable the GSP Falcon message queue interrupt (SWGEN0 interrupt).
> +    #[expect(dead_code)]
> +    pub(crate) fn enable_msgq_interrupt(&self, bar: &Bar0) {
> +        regs::NV_PFALCON_FALCON_IRQMASK::alter(bar, &Gsp::ID, |r|
r.set_swgen0(true));
> +    }
Alex, this ".alter" method is misnamed, IMHO. Because for registers,
The One True Way (or so I claim, haha) is to have the following methods:

    .read
    .modify, also known as RMW (read-modify-write)
    .write

"alter" never shows up in this naming scheme. I'm going to claim
that
this is a bit jarring for old hardware/kernel programmers.

But it's not too late: these are only used in a very few places, and
entirely
within nova-core, too.

Can I *please* send a patch to rename "alter" to "modify",
perhaps?

> +
> +    /// Check if the message queue interrupt is pending.
> +    #[expect(dead_code)]
> +    pub(crate) fn has_msgq_interrupt(&self, bar: &Bar0) -> bool
{
> +        regs::NV_PFALCON_FALCON_IRQSTAT::read(bar, &Gsp::ID).swgen0()
> +    }
Joel:

I am guessing that there is never a situation in which we would *disable*
these interrupts, right? Just thought I'd ask.
> +
> +    /// Clears the message queue interrupt to allow GSP to signal CPU
> +    /// for processing new messages.
> +    pub(crate) fn clear_msgq_interrupt(&self, bar: &Bar0) {
>          regs::NV_PFALCON_FALCON_IRQSCLR::default()
>              .set_swgen0(true)
>              .write(bar, &Gsp::ID);
>      }
> +
> +    /// Acknowledge all pending GSP interrupts.
> +    #[expect(dead_code)]
> +    pub(crate) fn ack_all_interrupts(&self, bar: &Bar0) {
> +        // Read status and write the raw value to IRQSCLR to clear all
pending interrupts.
> +        let status = regs::NV_PFALCON_FALCON_IRQSTAT::read(bar,
&Gsp::ID);
> +       
regs::NV_PFALCON_FALCON_IRQSCLR::from(u32::from(status)).write(bar,
&Gsp::ID);
> +    }
>  }
> diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
> index af20e2daea24..fb120cf7b15d 100644
> --- a/drivers/gpu/nova-core/gpu.rs
> +++ b/drivers/gpu/nova-core/gpu.rs
> @@ -216,7 +216,7 @@ pub(crate) fn new<'a>(
>                  bar,
>                  spec.chipset > Chipset::GA100,
>              )
> -            .inspect(|falcon| falcon.clear_swgen0_intr(bar))?,
> +            .inspect(|falcon| falcon.clear_msgq_interrupt(bar))?,
>  
>              sec2_falcon: Falcon::new(pdev.as_ref(), spec.chipset, bar,
true)?,
>  
> diff --git a/drivers/gpu/nova-core/regs.rs b/drivers/gpu/nova-core/regs.rs
> index 206dab2e1335..a3836a01996b 100644
> --- a/drivers/gpu/nova-core/regs.rs
> +++ b/drivers/gpu/nova-core/regs.rs
> @@ -198,6 +198,16 @@ pub(crate) fn vga_workspace_addr(self) ->
Option<u64> {
>  
>  // PFALCON
>  
> +register!(NV_PFALCON_FALCON_IRQMASK @ PFalconBase[0x00000014] {
> +    4:4     halt as bool;
> +    6:6     swgen0 as bool;
> +});
> +
> +register!(NV_PFALCON_FALCON_IRQSTAT @ PFalconBase[0x00000008] {
> +    4:4     halt as bool;
> +    6:6     swgen0 as bool;
> +});
> +
>  register!(NV_PFALCON_FALCON_IRQSCLR @ PFalconBase[0x00000004] {
>      4:4     halt as bool;
>      6:6     swgen0 as bool;
thanks,
-- 
John Hubbard

Hi John,

On 10/20/2025 6:35 PM, John Hubbard wrote:
[...]
> Joel:
> 
> I am guessing that there is never a situation in which we would *disable*
> these interrupts, right? Just thought I'd ask.
Thanks for double checking. At the moment, we don't have usecase to disable
swgen0. I checked my prototype code and Nouveau as well and we never have to do
this.

thanks,

 - Joel

On Tue Oct 21, 2025 at 7:35 AM JST, John Hubbard wrote:
> Alex, this ".alter" method is misnamed, IMHO. Because for
registers,
> The One True Way (or so I claim, haha) is to have the following methods:
>
>     .read
>     .modify, also known as RMW (read-modify-write)
>     .write
>
> "alter" never shows up in this naming scheme. I'm going to
claim that
> this is a bit jarring for old hardware/kernel programmers.
>
> But it's not too late: these are only used in a very few places, and
entirely
> within nova-core, too.
>
> Can I *please* send a patch to rename "alter" to
"modify", perhaps?
Oh yes, although I was just thinking that this should be renamed to
`update` for consistency with regmap.