Re: [PATCH v7 14/17] drm/panfrost: Avoid cache flush after perfcnt sample in fully coherent systems

[sashiko-bot@kernel.org]

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 <adrian.larumbe@collabora.com>

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/20260828-claude-fixes-v7-0-72a13b2c125d@collabora.com?part=14