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

[Steven Price]

On 05/10/2026 16:37, Boris Brezillon wrote:
> On Mon, 5 Oct 2026 16:06:02 +0100
> Steven Price <steven.price@xxxxxxx> wrote:
> 
>> On 05/10/2026 09:38, Boris Brezillon wrote:
>>> On Fri, 2 Oct 2026 16:14:38 +0100
>>> Steven Price <steven.price@xxxxxxx> 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 situation isn't actually a dirty cache line at the start, but a
>> stale one. We start off with the memory matching a clean line in the
>> GPU's cache. But because we don't have coherency the clean line can stay
>> even if it's inconsistent with everything else.
>>
>> CPU		|	GPU		| Memory
>> ----------------+-----------------------+--------------------
>>                 | clean line            | matches GPU cache
>> ----------------+-----------------------+--------------------
>>    CPU allocates new buffer and writes zeros
>> ----------------+-----------------------+--------------------
>> Dirty cache line| stale clean line      | unknown (cache line
>>                 |                       | might be evicted)
>> ----------------+-----------------------+--------------------
>>    CPU cleans its own cache to memory
>> ----------------+-----------------------+--------------------
>> Potential clean | stale clean line      | Matches CPU
>> cache line      |                       |
>> ----------------+-----------------------+--------------------
>>    Start dump without invaliding GPU
>> ----------------+-----------------------+--------------------
>> Potential clean | GPU writes data, and  | Unknown
>> cache line      | hits in the clean line|
>>                 | even though it's stale|
>> ----------------+-----------------------+--------------------
>>    CPU flushes the GPU's cache and invalidates it's own
>> ----------------+-----------------------+--------------------
>> No-cache line   | writes out clean line | Matches GPU
>>
>>
>> Of course for the GPU to have ended up with that stale clean cache line
>> means that the physical memory was previously used for something else on
>> the GPU, so the newly allocated BO has to reuse memory from a previous
>> BO that the GPU has accessed. And it's all "unlikely" due to the small
>> size of the GPU's cache.
> 
> Hm, I'd say it's actually impossible because every unmap operation is
> followed by a flush+inval of the GPU L2 and LSC, so for this stale
> clean line to exist on the GPU side when a physical page is GPU-mapped
> again, it would take a bug in the unmap logic or in the MMU HW, I
> think. Am I missing something?

Ah, yes that's true :) Although there's no need for us to do an
invalidate on the unmap path...

>>
>>
>>>>
>>>> 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).  
>>
>> So I agree this Should Be Safe™ because of how the driver is currently
>> using the performance counters. If we really want to drop the invalidate
>> then I think we need a comment explaining the logic. My worry is that
>> someone extends this in the future (e.g. allow selecting which counters
>> to enable) and breaks assumptions without them being documented.
>>
>> We normally do perform an invalidate when the GPU first touches a buffer
>> (e.g. for a BO) - it's just normally more implicit because it's done as
>> part of the job manager(/command stream).
>>
>> Also "caches known to be empty" is a dangerous thing to assume on
>> anything that could involve speculation - AFAIK Mali doesn't really
>> perform any form of speculation, but I'm not 100% sure on that.
>> Certainly with CPUs you don't get such a luxury of knowing what it might
>> have populated in the caches.
> 
> I suppose speculation pre-populating the caches with stale data would be
> covered by the flush+inval we do after a map operation (this is
> currently done in the unlock path regardless of the VM op, so both map
> and unmap get it). I don't know, maybe the goal is to relax the flushing
> policy around VM modifications in the future, but if things stay as
> they are now, we can assume that a fresh BO being GPU-mapped guarantees
> that no cacheline points to it until the first GPU access. But maybe
> I'm missing something else...

I have to admit I'm coming from experience on CPUs - there speculation
means a CPU can load a cache line at basically any point. So the
argument that a line cannot be in a cache almost never holds. GPUs (at
least Mali GPUs) haven't quite got to the stage of CPUs.

Thanks,
Steve