Re: [PATCH v5] drm/tyr: add Job IRQ handling

Hi Laura, thanks for working on this! :)

> On 10 Aug 2026, at 05:11, Laura Nao <[email protected]> wrote:
> 
> Add a threaded IRQ wrapper for Tyr interrupt sources and use it to
> handle the firmware Job IRQ.
> 
> The Job IRQ reports requests from the CSF firmware, including global
> interface requests and CSG attention bits. Only the GLB bit is currently
> handled, as it will be used to check firmware readiness. CSG bits
> handling will be added at a later stage. Add a Job IRQ handler that
> masks the interrupt in the primary IRQ handler, processes pending raw
> status in the threaded handler, clears the handled bit, and reenables
> the mask before returning.
> Add a wait queue and a bool flag so the handler can signal firmware
> readiness when the GLB bit is set.
> 
> Co-developed-by: Daniel Almeida <[email protected]>
> Signed-off-by: Daniel Almeida <[email protected]>
> Co-developed-by: Deborah Brouwer <[email protected]>
> Signed-off-by: Deborah Brouwer <[email protected]>
> Signed-off-by: Laura Nao <[email protected]>
> ---
> Changes in v5:
> - Implement PinnedDrop on TyrIrq to clear the interrupt mask on teardown 
> - Link to v4: https://lore.kernel.org/r/[email protected]
> 
> Changes in v4:
> - Replaced c_str! with C-String in irq.rs
> - Moved clear_status() before handle() in handle_threaded(), to avoid 
>  losing events if hw re-asserts the bit while handle() is running.
> - Only unmask interrupt sources in job_irq_init() when the handler is 
>  correctly registered.
> - Link to v3: https://lore.kernel.org/r/[email protected]
> 
> Changes in v3:
> - Propagated request_threaded_irq_by_name safety requirement up the
>  call chain, made TyrIrq::request() and job_irq_init() unsafe
> - Dropped CSG bits masking, added TODO comment to note this will be
>  handled at a later stage. Reworded commit message to reflect the
>  change.
> - Added documentation for TyrIrqTrait, TyrIrq, JobIrq and job_irq_init()
> - Link to v2: https://lore.kernel.org/r/[email protected]
> 
> Changes in v2:
> - Dropped Wait custom type in favor of WaitQueue
> - Renamed JobIrq lifetime to generic 'a

Why?

> 
> This patch follows up to [1], which adds support for firmware loading
> and MCU booting to the Tyr driver. The changes included here were
> originally introduced in its v4[2], then dropped to reduce the scope of
> the series, and have been adjusted to work with the HRT (Higher-Ranked
> Lifetime Types) driver architecture recently introduced.
> 
> The patch adds a threaded IRQ wrapper for the firmware Job interrupts,
> used to signal events from the global CSF (GLB) and Command Stream Group
> (CSG) interfaces.
> 
> These changes will be later used to wait for global CSF interface
> readiness after firmware boot, as part of the CSF firmware interfaces
> support that will be submitted as a separate series.
> 
> This patch is based on drm-rust-next and depends on:
> - [PATCH v2] rust: irq: make Registration compatible with lifetime-bound drivers[3]
> - [PATCH 0/5] rust: sync: add WaitQueue infrastructure[4]
> 
> Note: [4] doesn't apply cleanly on drm-rust-next at the moment, due to
> missing changes in rust/kernel/sync/lock/spinlock.rs[5]. I've applied
> all dependencies and fixed conflicts for the purpose of testing this
> patch on top of drm-rust-next, a branch with these changes is available
> here: https://gitlab.freedesktop.org/laura.nao/linux/-/commits/b4/tyr-irq-v5
> 
> [1] https://lore.kernel.org/all/[email protected]/ 
> [2] https://lore.kernel.org/rust-for-linux/[email protected]/
> [3] https://lore.kernel.org/rust-for-linux/[email protected]/
> [4] https://lore.kernel.org/rust-for-linux/[email protected]/
> [5] https://lore.kernel.org/all/[email protected]/
> ---
> drivers/gpu/drm/tyr/driver.rs | 108 ++++++++++++++++++++++++++++++++++++++++
> drivers/gpu/drm/tyr/fw.rs     |   1 +
> drivers/gpu/drm/tyr/fw/irq.rs | 112 ++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 221 insertions(+)
> 
> diff --git a/drivers/gpu/drm/tyr/driver.rs b/drivers/gpu/drm/tyr/driver.rs
> index d78ad9d292ff..495cc3baa98c 100644
> --- a/drivers/gpu/drm/tyr/driver.rs
> +++ b/drivers/gpu/drm/tyr/driver.rs
> @@ -1,5 +1,7 @@
> // SPDX-License-Identifier: GPL-2.0 or MIT
> 
> +use core::marker::PhantomPinned;
> +
> use kernel::{
>     clk::{
>         Clk,
> @@ -21,6 +23,13 @@
>         poll,
>         Io, //
>     },
> +    irq::{
> +        Flags,
> +        IrqReturn,
> +        ThreadedHandler,
> +        ThreadedIrqReturn,
> +        ThreadedRegistration, //
> +    },
>     new_mutex,
>     of,
>     platform,
> @@ -236,3 +245,102 @@ struct Regulators {
>     _mali: Regulator<regulator::Enabled>,
>     _sram: Regulator<regulator::Enabled>,
> }
> +
> +/// Trait describing the operations required to service a Tyr interrupt source.
> +pub(crate) trait TyrIrqTrait: Sync {

Can we move this and friends to its own irq.rs file? 

driver.rs is more of platform/device/initialization glue. I think it would be
more tidy if we split this. Additionally, I think TyrIrq, TyrIrqTrait and etc
can be their own patch, separate from the patch that adds support to FW IRQs
specifically. You can use #[expect(dead_code)] in patch 1.

> +    /// Returns the masked interrupt status.
> +    fn read_status(&self) -> u32;
> +
> +    /// Clears all bits in the mask register, disabling the interrupt sources.
> +    fn clear_mask(&self);
> +
> +    /// Sets the mask register to re-enable the interrupt sources.
> +    fn reenable_mask(&self);
> +
> +    /// Returns the raw interrupt status, regardless of the current mask.
> +    fn read_raw_status(&self) -> u32;
> +
> +    /// Clears the given bits in the interrupt status register.
> +    fn clear_status(&self, status: u32);
> +
> +    /// Returns the bitmask of interrupt sources handled by this instance.
> +    fn mask(&self) -> u32;
> +
> +    /// Handles the given pending, unmasked interrupt bits.
> +    fn handle(&self, status: u32);
> +}
> +
> +/// A threaded IRQ wrapper shared by all Tyr interrupt sources.
> +#[pin_data(PinnedDrop)]
> +pub(crate) struct TyrIrq<T: TyrIrqTrait> {
> +    /// The interrupt source.
> +    irq: T,
> +    #[pin]
> +    _pin: PhantomPinned,
> +}
> +
> +impl<T: TyrIrqTrait> TyrIrq<T> {
> +    /// Requests a threaded IRQ registration for `irq`, using `name` to look up the interrupt.
> +    ///
> +    /// # Safety
> +    ///
> +    /// Callers must not `mem::forget()` the resulting registration or otherwise prevent its
> +    /// [`Drop`] implementation from running.
> +    pub(crate) unsafe fn request<'a>(
> +        pdev: &'a platform::Device<Bound>,
> +        name: &'static CStr,
> +        irq: T,
> +    ) -> Result<impl PinInit<ThreadedRegistration<'a, Self>, Error> + 'a>
> +    where
> +        T: 'a,
> +    {

Instead of 'a, I think we should converge on the 'bound nomenclature. Is there
a reason why we are using 'a here instead? Same for JobIrq<‘a> and job_irq_init.


> +        let handler = try_pin_init!(Self {
> +            irq,
> +            _pin: PhantomPinned,
> +        });
> +
> +        // SAFETY: The caller guarantees the resulting registration will not be leaked.
> +        Ok(unsafe { pdev.request_threaded_irq_by_name(Flags::SHARED, name, name, handler) })
> +    }
> +
> +    /// Returns a reference to the wrapped interrupt source.
> +    pub(crate) fn irq(&self) -> &T {
> +        &self.irq
> +    }

The only caller seems to be pin_chain in job_irq_init()? I think we can remove
this accessor and just access the field directly in request().

> +}
> +
> +impl<T: TyrIrqTrait> ThreadedHandler for TyrIrq<T> {
> +    fn handle(&self) -> ThreadedIrqReturn {
> +        let masked_status = self.irq.read_status();
> +
> +        if masked_status == 0 {
> +            return ThreadedIrqReturn::None;
> +        }
> +        self.irq.clear_mask();
> +        ThreadedIrqReturn::WakeThread
> +    }
> +
> +    fn handle_threaded(&self) -> IrqReturn {
> +        let mut ret = IrqReturn::None;
> +
> +        loop {
> +            let raw_status = self.irq.read_raw_status() & self.irq.mask();
> +            if raw_status == 0 {
> +                break;
> +            }
> +            self.irq.clear_status(raw_status);
> +            self.irq.handle(raw_status);
> +            ret = IrqReturn::Handled;
> +        }
> +
> +        self.irq.reenable_mask();
> +        ret
> +    }
> +}
> +
> +#[pinned_drop]
> +impl<T: TyrIrqTrait> PinnedDrop for TyrIrq<T> {
> +    fn drop(self: Pin<&mut Self>) {
> +        self.irq.clear_mask();
> +    }

In Rust, the drop order is first a struct then its fields, and handler is a
field of ThreadedRegistration. By the time this drop runs, free_irq() has
already run. Ideally we'd write this in the opposite order.

Perhaps what would work is moving this drop into a separate type:

_job_irq_mask: JobIrqMaskGuard<'bound>,
job_irq: Pin<KBox<ThreadedRegistration<...>>>

Because within a struct, fields drop in their declaration order, so first all
firmware interrupts are masked, then free_irq() waits for the in-flight
interrupts before deregistering.

> +}
> diff --git a/drivers/gpu/drm/tyr/fw.rs b/drivers/gpu/drm/tyr/fw.rs
> index 47d25c901bd0..8162b9806c97 100644
> --- a/drivers/gpu/drm/tyr/fw.rs
> +++ b/drivers/gpu/drm/tyr/fw.rs
> @@ -69,6 +69,7 @@
>     vm::Vm, //
> };
> 
> +pub(crate) mod irq;
> mod parser;
> 
> pub(super) const CSF_MCU_SHARED_REGION_START: u32 = 0x04000000;
> diff --git a/drivers/gpu/drm/tyr/fw/irq.rs b/drivers/gpu/drm/tyr/fw/irq.rs
> new file mode 100644
> index 000000000000..fcdc0097f2dd
> --- /dev/null
> +++ b/drivers/gpu/drm/tyr/fw/irq.rs
> @@ -0,0 +1,112 @@
> +// SPDX-License-Identifier: GPL-2.0 or MIT
> +
> +//! IRQ handling for the Job IRQ.
> +//!
> +//! The Job IRQ signals events from the MCU, including global interface acknowledgements.
> +#![allow(dead_code)]
> +
> +use core::sync::atomic::{
> +    AtomicBool,

kernel::sync::atomic::Atomic<bool>

> +    Ordering, //
> +};
> +
> +use kernel::{
> +    device::Bound, //
> +    io::Io,
> +    irq::ThreadedRegistration,
> +    platform,
> +    prelude::*,
> +    sync::{
> +        Arc,
> +        WaitQueue, //
> +    },
> +};
> +
> +use crate::{
> +    driver::{
> +        IoMem,
> +        TyrIrq,
> +        TyrIrqTrait, //
> +    },
> +    regs::job_control::{
> +        JOB_IRQ_CLEAR,
> +        JOB_IRQ_MASK,
> +        JOB_IRQ_RAWSTAT,
> +        JOB_IRQ_STATUS, //
> +    }, //
> +};
> +
> +// The Job IRQ, signalling requests or notification from the MCU.
> +pub(crate) struct JobIrq<'a> {
> +    /// GPU MMIO register mapping.
> +    iomem: Arc<IoMem<'a>>,
> +    /// Flag to signal firmware readiness.
> +    fw_ready: Arc<AtomicBool>,

kernel::sync::atomic::Atomic<bool>

> +    /// Wait queue used to signal firmware events to waiters.
> +    job_irq_wait: Arc<WaitQueue>,
> +}


I think I'd rather have a single type and a single Arc here, e.g.:

    /// The firmware events published by the Job IRQ handler, and the queue used
    /// to announce them.
    #[pin_data]
    pub(crate) struct JobIrqEvents {
        #[pin]
        wait: WaitQueue,
        /// Set once the firmware has signalled that the global interface is ready.
        ready: Atomic<bool>
    }

    impl JobIrqEvents {
        pub(crate) fn new() -> Result<Arc<Self>> {
            Arc::pin_init(
                pin_init!(JobIrqEvents {
                    wait <- new_waitqueue!(),
                    ready: AtomicBool::new(false),
                }),
                GFP_KERNEL,
            )
        }

        pub(crate) fn clear_ready(&self) {
            self.ready.store(false, Ordering::Relaxed);
        }

        pub(crate) fn wait_ready(&self, timeout_ms: Msecs) -> Result {
            self.wait.wait_event_timeout(
                || self.ready.load(Ordering::Acquire),
                msecs_to_jiffies(timeout_ms),
            )?;

            Ok(())
        }

        fn signal(&self, status: u32) { ... }
    }

> +
> +/// Requests a threaded IRQ registration for the Job IRQ.
> +///
> +/// # Safety
> +///
> +/// Callers must not `mem::forget()` the resulting registration or otherwise prevent its
> +/// [`Drop`] implementation from running.
> +pub(crate) unsafe fn job_irq_init<'a>(
> +    pdev: &'a platform::Device<Bound>,
> +    iomem: Arc<IoMem<'a>>,
> +    fw_ready: Arc<AtomicBool>,
> +    job_irq_wait: Arc<WaitQueue>,
> +) -> Result<impl PinInit<ThreadedRegistration<'a, TyrIrq<JobIrq<'a>>>, Error> + 'a> {
> +    let job_irq = JobIrq {
> +        iomem,
> +        fw_ready,
> +        job_irq_wait,
> +    };
> +    // Ensure interrupt sources are disabled until the handler is registered.
> +    job_irq.clear_mask();

IIRC panthor also clears the status here?

> +
> +    // SAFETY: The caller guarantees the resulting registration will not be leaked.
> +    let registration = unsafe { TyrIrq::request(pdev, c"job", job_irq) };
> +
> +    // Unmask once the handler has been successfully registered.
> +    Ok(registration?.pin_chain(|reg| {
> +        reg.handler().irq().reenable_mask();
> +        Ok(())
> +    }))
> +}
> +
> +impl TyrIrqTrait for JobIrq<'_> {
> +    fn read_status(&self) -> u32 {
> +        self.iomem.read(JOB_IRQ_STATUS).into_raw()
> +    }
> +
> +    fn clear_mask(&self) {
> +        self.iomem.write_reg(JOB_IRQ_MASK::zeroed());
> +    }
> +
> +    fn reenable_mask(&self) {
> +        self.iomem.write_reg(JOB_IRQ_MASK::zeroed().with_glb(true));
> +    }

Can this use mask() internally, please? :)

> +
> +    fn read_raw_status(&self) -> u32 {
> +        self.iomem.read(JOB_IRQ_RAWSTAT).into_raw()
> +    }
> +
> +    fn clear_status(&self, status: u32) {
> +        self.iomem.write_reg(JOB_IRQ_CLEAR::from_raw(status));
> +    }
> +
> +    fn mask(&self) -> u32 {
> +        JOB_IRQ_MASK::zeroed().with_glb(true).into_raw()
> +    }
> +
> +    fn handle(&self, status: u32) {
> +        // TODO: handle other Job IRQ events (e.g. CSG attention bits) here once
> +        // support for them is added.
> +        if JOB_IRQ_RAWSTAT::from_raw(status).glb() {
> +            self.fw_ready.store(true, Ordering::Release);

We only need to store this once. In fact, the “firmware is now ready”
meaning only applies during power up from the hardware’s perspective.


> +            self.job_irq_wait.wake_up_all();

We need to wake up waiters unconditionally. Not a problem now,
but this will be a problem in the future.

> +        }
> +    }
> +}
> 
> ---
> base-commit: 0710742e26a06c2c3167e77a8ea82b59632d9f7a
> change-id: 20260728-tyr-irq-v2-0b3c5022be33
> 
> Best regards,
> -- 
> Laura Nao <[email protected]>
> 
> 

Currently, the firmware booting code will poll for “fw_ready”. Can you
replace that with the JobIrqEvents logic you are introducing above?

— Daniel