Re: [PATCH v12 12/15] drm/panfrost: Skip cache flush/invalidate when enabling perfcnt

[Boris Brezillon <boris.brezillon@collabora.com>]

On Fri, 2 Oct 2026 16:14:38 +0100
Steven Price <steven.price@arm.com> wrote:

> On 29/09/2026 04:44, Adrián Larumbe wrote:
> > The GPU cache flush/invalidate operation is unnecessary. First off, the
> > GPU doesn't read off the perfcnt sample buffer, only writes into it, so  
> 
> I don't think this is entirely true. The GPU performance counter unit
> only writes the counters that are enabled, counters that share a cache
> line but are not enabled are not written by the performance counter
> unit, but if the L2 contains that cache line then the write can hit in
> the L2 and dirty the entire line including stale data where the
> unwritten cache line is.

I don't think this can happen though, because _enable_locked() is
creating a BO (and its GPU mapping) just before enabling the perfcnt
block, meaning the buffer is known to have no dirty cacheline pointing
to it until the first dump happens. And we do flush and invalidate GPU
caches after each dump, so again, we should be covered.

> 
> The upshot is that if the CPU has cleared a block of memory which the
> GPU happens to have cached, then the "unused" counters may end up
> showing the old data before the CPU cleared it (if they share a cache
> line with an active counter).

I agree, but that's not a case we can hit in the enable path. I think I
mentioned the commit message was misleading, and that we should instead
talk about the fact the GPU is not supposed to have cached anything up
until the first SAMPLE following a the ENABLE step.

> 
> I have to admit it's probably somewhat academic given that Panfrost
> doesn't expose the ability to control which counters are enabled...
> 
> Is there a good reason for this patch (i.e. have you seen a performance
> problem with doing the invalidate)? Otherwise I'd prefer we keep to the
> safe route rather than trying to over optimise cache maintenance.

I think I was the one suggesting dropping this flush so that
panfrost_perfcnt_hw_enable() (in the last patch) has one less fallible
operation. Besides, I find it confusing to have a cache flush+inval in
a path where the GPU is not supposed to have accessed the buffer yet
(or later on, when we re-enable after a RESET, in a path where the GPU
has been reset and the caches are known to be empty).

> 
> Obviously in the fully coherent case the invalidate could be skipped (as
> in the next patch).
> 
> Thanks,
> Steve
> 
> > an invalidate doesn't make a difference. Then flushing GPU caches after
> > each sample has been written is enough for the CPU to see updated values.
> > 
> > Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
> > Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> > ---
> >  drivers/gpu/drm/panfrost/panfrost_perfcnt.c | 15 ++-------------
> >  1 file changed, 2 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
> > index f71534e741b6..ffc77121070e 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
> > +++ b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
> > @@ -124,21 +124,10 @@ static int panfrost_perfcnt_enable_locked(struct panfrost_device *pfdev,
> >  	panfrost_gem_internal_set_label(&bo->base, "Perfcnt sample buffer");
> >  
> >  	/*
> > -	 * Invalidate the cache and clear the counters to start from a fresh
> > -	 * state.
> > +	 * Clear the counters to start from a fresh state.
> >  	 */
> > -	reinit_completion(&pfdev->perfcnt->dump_comp);
> > -	gpu_write(pfdev, GPU_INT_CLEAR,
> > -		  GPU_IRQ_CLEAN_CACHES_COMPLETED |
> > -		  GPU_IRQ_PERFCNT_SAMPLE_COMPLETED);
> > +	gpu_write(pfdev, GPU_INT_CLEAR, GPU_IRQ_PERFCNT_SAMPLE_COMPLETED);
> >  	gpu_write(pfdev, GPU_CMD, GPU_CMD_PERFCNT_CLEAR);
> > -	gpu_write(pfdev, GPU_CMD, GPU_CMD_CLEAN_INV_CACHES);
> > -	ret = wait_for_completion_timeout(&pfdev->perfcnt->dump_comp,
> > -					  msecs_to_jiffies(1000));
> > -	if (!ret) {
> > -		ret = -ETIMEDOUT;
> > -		goto err_vunmap;
> > -	}
> >  
> >  	ret = panfrost_mmu_as_get(pfdev, perfcnt->mapping->mmu);
> >  	if (ret < 0)
> >   
>