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/amd/display: Replace use of system_wq with system_percpu_wq Date: Sat, 14 Mar 2026 06:50:05 +1000 Message-ID: In-Reply-To: <20260313144715.252069-1-marco.crivellari@suse.com> References: <20260313144715.252069-1-marco.crivellari@suse.com> <20260313144715.252069-1-marco.crivellari@suse.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 **Correctness concern =E2=80=94 per-CPU workqueue may not be appropriate he= re:** Both work items modified in this patch do not have per-CPU affinity require= ments: 1. **`schedule_dc_vmin_vmax`** (line 575): The work handler `dm_handle_vmin= _vmax_update` immediately takes `adev->dm.dc_lock` (a mutex) and calls `dc_= stream_adjust_vmin_vmax`. There is no reason this work needs to run on the = CPU that queued it =E2=80=94 it operates on device-global state protected b= y a mutex. 2. **`handle_hpd_irq_helper`** (line 4145): The delayed work for HDMI HPD d= ebouncing performs connector hotplug detection. Again, there is no CPU-affi= nity requirement =E2=80=94 this is device-level connector handling. `system_percpu_wq` is a per-CPU workqueue (`WQ_PERCPU`), meaning work items= will execute on the same CPU that queued them. For IRQ handler offloading = (the vmin/vmax case is called from `dm_vupdate_high_irq`), pinning work to = the interrupt-servicing CPU might cause unnecessary load imbalance, especia= lly since the work immediately contends on a global mutex anyway. **The more appropriate replacement** for these use cases would likely be `s= ystem_dfl_wq` (the replacement for `system_unbound_wq`), which allows the s= cheduler to place the work optimally, or simply staying with the current `s= ystem_wq` until the migration plan for non-CPU-affine users is clarified. **Suggestion:** Confirm with the workqueue maintainers (Tejun Heo) whether = `system_percpu_wq` is indeed the correct target for work items that have no= per-CPU affinity requirement. The referenced refactoring thread suggests `= system_wq -> system_percpu_wq` as a blanket mapping, but for work items tha= t take global mutexes and operate on shared device state, `system_dfl_wq` m= ay be more appropriate. **Code changes themselves are mechanically correct** =E2=80=94 the API sign= atures for `queue_work()` and `mod_delayed_work()` accept any `struct workq= ueue_struct *`, so the substitution compiles and functions correctly regard= less of which workqueue is chosen. --- Generated by Claude Code Patch Reviewer