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

[Adrián Larumbe]

On 2026-10-05 16:06:02+01:00, Steven Price 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:
> > 
> > 
> > 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

We're not doing this explicitly for BO memory from the CPU. AFAIK the only way to
do that manually is calling dma_sync_single_for_device() on physical addresses. I
suppose this means the reason why the CPU can see up-to-date dump data is because
a GPU cache flush also invalidates CPU caches for the same lines?

> ----------------+-----------------------+--------------------
> 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.
> 
> >>
> > 
> > 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 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 documentead.

Perfcnt ioctl's are currently marked as unstable, but even so, adding the
initial flush/invalidate back in case anyone ever wanted to redefine the
uAPI to add support for fine-grained selection of counters shouldn't be
much of an issue. It's just that, as things stand now, it simplifies error
handling quite a bit, and I guess it's a reasonable trade-off.

I'll add a comment explaining why it's safe not to do the initial
flush/invalidate in a new revision.

> 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).

Does this mean instructions that are part of the command stream passed to the
submit ioctl can trigger a flush/invalidate without CPU intervention?

> 
> 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.
> 
> Thanks,
> Steve