From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claude Code Review Bot To: dri-devel-reviews@example.com Subject: Claude review: drm/panthor: Process FW events in IRQ context Date: Sat, 16 May 2026 13:32:46 +1000 Message-ID: In-Reply-To: <20260512-panthor-signal-from-irq-v2-9-95c614a739cb@collabora.com> References: <20260512-panthor-signal-from-irq-v2-0-95c614a739cb@collabora.com> <20260512-panthor-signal-from-irq-v2-9-95c614a739cb@collabora.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Patch Review Replaces the threaded handler with a custom hard IRQ handler `panthor_job_i= rq_raw_handler()`: ```c static irqreturn_t panthor_job_irq_raw_handler(int irq, void *data) { struct panthor_irq *pirq =3D data; if (!gpu_read(pirq->iomem, INT_STAT)) return IRQ_NONE; scoped_guard(spinlock_irqsave, &pirq->mask_lock) { if (pirq->state !=3D PANTHOR_IRQ_STATE_ACTIVE) return IRQ_NONE; pirq->state =3D PANTHOR_IRQ_STATE_PROCESSING; } /* We can use INT_STAT here, because we didn't mask the IRQs. */ panthor_job_irq_handler(pirq, gpu_read(pirq->iomem, INT_STAT)); scoped_guard(spinlock_irqsave, &pirq->mask_lock) { if (pirq->state =3D=3D PANTHOR_IRQ_STATE_PROCESSING) pirq->state =3D PANTHOR_IRQ_STATE_ACTIVE; } return IRQ_HANDLED; } ``` **Key difference from the old threaded handler:** The old handler looped re= ading `INT_RAWSTAT & mask`, clearing and processing until no more events. T= his new handler reads `INT_STAT` once. The comment says "We can use INT_STA= T here, because we didn't mask the IRQs." This is correct =E2=80=94 since t= he mask stays enabled, `INT_STAT` reflects only newly-active, unmasked inte= rrupts. If another event arrives during processing, it will fire a new IRQ. **Concern =E2=80=94 single-shot processing:** The old threaded handler loop= ed until `INT_RAWSTAT & mask` was zero, ensuring all pending events were dr= ained. This new handler processes a single `INT_STAT` read. If new events a= rrive between the first `INT_STAT` read (the check at the top) and the seco= nd `INT_STAT` read (passed to the handler), the handler sees both. But if e= vents arrive *after* the second read, they'll trigger a new interrupt since= the mask is still active. This should be fine for correctness, but could r= esult in more IRQ entries under heavy load compared to the drain-loop appro= ach. Given the ~5% performance improvement measured, the reduced latency cl= early outweighs any overhead from additional IRQ entries. **Concern =E2=80=94 `INT_STAT` read without `INT_CLEAR`:** Looking at `pant= hor_job_irq_handler()`, the handler does call `gpu_write(pirq->iomem, INT_C= LEAR, status)` at the start. The `INT_STAT` register reflects `INT_RAWSTAT = & INT_MASK`. After clearing via `INT_CLEAR`, the `INT_RAWSTAT` bit drops, s= o `INT_STAT` for that bit also drops. New events will set new `INT_RAWSTAT`= bits, generating new `INT_STAT` bits and new IRQs. This is correct. **Minor concern =E2=80=94 two `INT_STAT` reads:** The initial `INT_STAT` ch= eck and the second `INT_STAT` read passed to the handler could see differen= t values if events arrive between them. The second read may include more ev= ents than the first, but that's fine =E2=80=94 more events processed is bet= ter. If the second read returns 0 (all events cleared between reads), the h= andler would call `panthor_job_irq_handler(pirq, 0)` which does `gpu_write(= ..., INT_CLEAR, 0)` (no-op) and `panthor_sched_report_fw_events(ptdev, 0)` = (which is benign =E2=80=94 no bits set means no processing). So this edge c= ase is harmless. No blocking concerns. --- --- Generated by Claude Code Patch Reviewer