From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: drm/panthor: Prepare the scheduler logic for FW events in IRQ context
Date: Sat, 16 May 2026 13:32:45 +1000
Message-ID: <review-patch6-20260512-panthor-signal-from-irq-v2-6-95c614a739cb@collabora.com>
In-Reply-To: <20260512-panthor-signal-from-irq-v2-6-95c614a739cb@collabora.com>
References: <20260512-panthor-signal-from-irq-v2-0-95c614a739cb@collabora.com> <20260512-panthor-signal-from-irq-v2-6-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

This is the most substantial and critical patch in the series. Key changes:

**1. New `events_lock` spinlock:** Replaces the `sched->lock` mutex for the=
 event processing path. This is necessary because mutex can't be held in IR=
Q context.

**2. Event processing inlined into `panthor_sched_report_fw_events()`:** Th=
e old `fw_events_work` work item is eliminated. Events are now processed di=
rectly under `events_lock`:
```c
guard(spinlock_irqsave)(&ptdev->scheduler->events_lock);
// process events inline
```

**3. Fence signalling moved inline:** `group_sync_upd_work` (work item) rep=
laced by `group_check_job_completion()` called directly from `csg_slot_sync=
_update_locked()`. This is the key change enabling fence signalling from IR=
Q context. The `dma_fence_signal_locked()` call is correctly wrapped with `=
dma_fence_begin_signalling()` / `dma_fence_end_signalling()`.

**4. Lock upgrades:** Multiple `spin_lock()` converted to `spin_lock_irqsav=
e()` variants for `fence_ctx.lock` and `fdinfo.lock`, since these are now t=
aken under `events_lock` (IRQ context).

**Concern =E2=80=94 `events_lock` scope in `tick_ctx_apply` and `panthor_sc=
hed_suspend`:**

In `tick_ctx_apply`, the patch uses `guard(spinlock_irqsave)` inside an `if=
` block:
```c
if (group->csg_id >=3D 0) {
    guard(spinlock_irqsave)(&sched->events_lock);
    sched_process_csg_irq_locked(ptdev, group->csg_id);
}

group_unbind_locked(group);
```

The `guard()` acquires at declaration and releases at scope exit. Since it'=
s declared inside the `if` block, the lock is released when the `if` block =
exits, **before** `group_unbind_locked()` is called. However, `group_unbind=
_locked()` itself takes `events_lock` internally (for the slot reassignment=
). So the lock is released between `sched_process_csg_irq_locked()` and the=
 unbind's `events_lock` section. There's a theoretical window where a new I=
RQ could arrive and process events for this CSG slot between the two. This =
is addressed in patch 7 by moving the `sched_process_csg_irq_locked()` call=
 into `group_unbind_locked()` itself, making it atomic. So while this patch=
 has a subtle window, patch 7 fixes it. This is acceptable as a series but =
worth noting the ordering dependency.

**Concern =E2=80=94 `sched_queue_delayed_work` from IRQ context:**

`sched_queue_delayed_work` calls `mod_delayed_work()`, which is documented =
as IRQ-safe, so this is fine.

**Concern =E2=80=94 `cancel_delayed_work` from IRQ context:**

`queue_suspend_timeout_locked()` calls `cancel_delayed_work()` (not `_sync`=
), which is IRQ-safe. Good.

**Concern =E2=80=94 double `dma_fence_begin_signalling`:**

In `queue_check_job_completion()`:
```c
cookie =3D dma_fence_begin_signalling();
scoped_guard(spinlock_irqsave, &queue->fence_ctx.lock) { ... }
dma_fence_end_signalling(cookie);
```

And the caller `group_check_job_completion()` also does:
```c
cookie =3D dma_fence_begin_signalling();
for (...) queue_check_job_completion(queue);
dma_fence_end_signalling(cookie);
```

This results in nested `dma_fence_begin_signalling()` calls. The kernel's f=
ence signalling annotation framework supports nesting (it's a counter), so =
this is correct but redundant. The inner `dma_fence_begin_signalling` in `q=
ueue_check_job_completion` could be removed since the caller already establ=
ishes the signalling context, but it's not a bug.

Overall this patch is the riskiest in the series but the approach is sound.

---

---
Generated by Claude Code Patch Reviewer