Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] The GPU interrupt handler acknowledges hardware interrupts after signaling the waitqueue completion, creating a race window where a subsequent hardware operation's interrupt is erroneously cleared. --
commit a4bf01da864f9ea533a5e37d387ba8e2c63a2a99 Author: Adrián Larumbe <[email protected]> drm/panfrost: Avoid cache flush after perfcnt sample in fully coherent systems This commit optimizes the performance counter sample path by skipping the GPU cache flush on fully coherent (ACE) systems. It directly signals completion instead of issuing a cache clean command. > diff --git a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c > b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c > index ffc77121070e2..b3f71d7fd82a9 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c > +++ b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c [ ... ] > @@ -52,7 +52,10 @@ void panfrost_perfcnt_clean_cache_done(struct > panfrost_device *pfdev) > > void panfrost_perfcnt_sample_done(struct panfrost_device *pfdev) > { > - gpu_write(pfdev, GPU_CMD, GPU_CMD_CLEAN_CACHES); > + if (pfdev->features.selected_coherency != COHERENCY_ACE) > + gpu_write(pfdev, GPU_CMD, GPU_CMD_CLEAN_CACHES); > + else > + complete(&pfdev->perfcnt->dump_comp); [Severity: Medium] Does bypassing the cache clean step and calling complete() directly here expose a race condition with the GPU interrupt handler? In the baseline, the final completion was triggered by a different bit (GPU_IRQ_CLEAN_CACHES_COMPLETED). With this patch, the GPU_IRQ_PERFCNT_SAMPLE_COMPLETED handler path now calls complete() directly on ACE coherent systems. If the awoken user thread immediately issues a new dump, and the GPU completes it before the original IRQ handler in panfrost_gpu_irq_handler() acknowledges the hardware interrupt: panfrost_gpu.c:panfrost_gpu_irq_handler() ... // Software completion happens here (which now wakes the user thread) panfrost_perfcnt_sample_done(pfdev); ... // Meanwhile, a new dump is requested and finishes, raising a new interrupt ... // The original handler then clears the interrupt using its stale state gpu_write(pfdev, GPU_INT_CLEAR, state); Could the original handler inadvertently clear the new interrupt because its local state variable still holds the GPU_IRQ_PERFCNT_SAMPLE_COMPLETED bit? This could cause back-to-back performance counter dumps (e.g., if a local user calls the perfcnt dump ioctl in a tight loop) to sporadically fail with an ioctl timeout when their completion interrupt is lost. > } > > static int panfrost_perfcnt_dump_locked(struct panfrost_device *pfdev) -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=14
