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: Reduce dma_fence signalling latency Date: Tue, 05 May 2026 11:44:52 +1000 Message-ID: In-Reply-To: <20260429-panthor-signal-from-irq-v1-0-4b92ae4142d2@collabora.com> References: <20260429-panthor-signal-from-irq-v1-0-4b92ae4142d2@collabora.com> X-Mailer: Claude Code Patch Reviewer Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit MIME-Version: 1.0 Overall Series Review Subject: drm/panthor: Reduce dma_fence signalling latency Author: Boris Brezillon Patches: 33 Reviewed: 2026-05-05T11:44:52.297248 --- This is a well-structured 10-patch series from Boris Brezillon to move panthor's job IRQ processing from a threaded IRQ handler (and work items) into the raw/hard IRQ handler, reducing dma_fence signalling latency. The series progressively refactors the IRQ infrastructure, converts locking to be IRQ-safe, and culminates in processing FW events directly in hard IRQ context with optional interrupt coalescing. The overall design is sound -- most GPU drivers already signal fences from hard IRQ context, and the incremental approach makes the transition reviewable. The prerequisite series (from Karunika Choo) adds the `iomem` field to `panthor_irq` that multiple patches depend on. **Key concerns:** 1. **Bug in patch 9:** `drm_WARN_ON_ONCE()` is called with a string literal instead of a boolean condition expression -- this will always trigger the warning since a string literal is truthy. 2. **Locking concern in patch 6:** `update_fdinfo_stats()` uses `scoped_guard(spinlock, ...)` (non-irqsave) but is now called from `queue_check_job_completion()` which runs in hard IRQ context. This will deadlock if a regular context holds `group->fdinfo.lock` and gets interrupted. 3. **Double `dma_fence_begin_signalling` in patch 6:** `group_check_job_completion()` calls `dma_fence_begin_signalling()` and then calls `queue_check_job_completion()` which also calls `dma_fence_begin_signalling()`, resulting in nested signalling annotations. 4. **Patch 10 (coalescing):** The debugfs interface uses `container_of(file->private_data, struct panthor_device, base)` which relies on `minor->dev` being the private_data -- this is fragile and should be verified. The coalescing stats fields (`coalesced_cnt`, `inbounds_cnt`) are accessed without any synchronization between hard IRQ context and debugfs reads. --- --- Generated by Claude Code Patch Reviewer