On 06/10/2026 18:18, Adrián Larumbe wrote: > 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?
Without coherency between the CPU and GPU then when sharing a buffer with the GPU there needs to be cache maintenance (otherwise the GPU might see stale data which is still in RAM, which would be a security hole). This is because a previous user could have mapped the data cached. I believe this is done internally within the DMA framework. For reading the data written by the GPU, again there needs to be cache maintenance - a flush on the GPU and an invalidate on the CPU. However I think we map the data on the CPU as write-combining (i.e. uncached) so there's no need for an explicit invalidate on the CPU - the reads will bypass the cache. >> ----------------+-----------------------+-------------------- >> 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? Sorry, I shouldn't have mentioned command streams - that's a Panthor thing. For Panfrost the flush/invalidate is done as part of the job submission (FLUSH_PT). With the Panthor and the MCU we then have a FLUSH_CACHE2 instruction that can be part of the command stream - so yes you can trigger (some forms of) a cache flush during the middle of a command stream without CPU intervention. Thanks, Steve >> >> 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 > >
