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 <[email protected]> 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
