Message-ID: <4553a31a-fd13-41c4-8bcb-3b830cd7b661@nvidia.com>
Date: Mon, 20 Oct 2025 15:35:03 -0700
From: John Hubbard <jhubbard@...dia.com>
To: Joel Fernandes <joelagnelf@...dia.com>, linux-kernel@...r.kernel.org,
 rust-for-linux@...r.kernel.org, dri-devel@...ts.freedesktop.org,
 dakr@...nel.org, acourbot@...dia.com
Cc: Alistair Popple <apopple@...dia.com>, Miguel Ojeda <ojeda@...nel.org>,
 Alex Gaynor <alex.gaynor@...il.com>, Boqun Feng <boqun.feng@...il.com>,
 Gary Guo <gary@...yguo.net>, bjorn3_gh@...tonmail.com,
 Benno Lossin <lossin@...nel.org>, Andreas Hindborg <a.hindborg@...nel.org>,
 Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
 David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
 Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
 Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
 Timur Tabi <ttabi@...dia.com>, joel@...lfernandes.org,
 Elle Rhumsaa <elle@...thered-steel.dev>,
 Daniel Almeida <daniel.almeida@...labora.com>, nouveau@...ts.freedesktop.org
Subject: Re: [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@...dia.com>

> 
> Signed-off-by: Joel Fernandes <joelagnelf@...dia.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

Powered by blists - more mailing lists