On 06/10/2026 17:45, Adrián Larumbe wrote: > On 2026-10-02 16:14:38+01:00, Steven Price 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. > > From the perspective of UM, is it bad if offsets into the perfcnt buffer > that stand for counters they did not enable contain undefined values?
There's two things to consider here: * Does the UM care? The UM could write canary values to check that the GPU has actually filled in the counters. Replacing them with "undefined" values would break this behaviour. I'm not sure whether this is done - but it can be used to check that the hardware is actually writing the counter as expected. * Is it a security hole? If the GPU cache was from when this BO was used by another context then this is obviously bad. As Boris pointed out this shouldn't be possible in the current driver if we're invalidating on unmap. >> 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 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. > > Like Boris said in a later reply, it was mostly about simplifying error > handling > in the perfcnt enable path. We saw this initial clean being done and thought > it > wasn't necessary. I think we need at least a comment explaining why this is safe. It does seem that as things currently stand we don't actually need it though. Thanks, Steve >> Obviously in the fully coherent case the invalidate could be skipped (as >> in the next patch). >> >> Thanks, >> Steve > >
