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
