From 739da17f6efa371d46499d999d141849a121e128 Mon Sep 17 00:00:00 2001 From: "Eric R. Smith" Date: Tue, 11 Mar 2025 18:56:34 -0300 Subject: [PATCH] panfrost,lima: use index size in panfrost minmax_cache Bifrost keeps a cache of information about buffers being used as indices. Unfortunately, it was not keeping information about the size of the indices (probably because this rarely changes). If a program deliberately re-interprets the indices as a different type (e.g. UNSIGNED_INT instead of UNSIGNED_SHORT) then we will use incorrect values from the cache. This actually showed up in a test program we were running. Fix by saving the index size in the cache key. Cc: mesa-stable Reviewed-by: Erik Faye-Lund Part-of: --- src/gallium/drivers/lima/lima_draw.c | 8 ++- src/gallium/drivers/lima/lima_resource.c | 8 ++- src/gallium/drivers/panfrost/pan_helpers.c | 6 +- src/gallium/drivers/panfrost/pan_resource.c | 8 ++- src/panfrost/shared/pan_minmax_cache.c | 79 +++++++++++++++++---- src/panfrost/shared/pan_minmax_cache.h | 3 + 6 files changed, 90 insertions(+), 22 deletions(-) diff --git a/src/gallium/drivers/lima/lima_draw.c b/src/gallium/drivers/lima/lima_draw.c index d95bdd3ba5e..8687edaed25 100644 --- a/src/gallium/drivers/lima/lima_draw.c +++ b/src/gallium/drivers/lima/lima_draw.c @@ -1091,14 +1091,16 @@ lima_draw_vbo_indexed(struct pipe_context *pctx, else { ctx->index_res = lima_resource(info->index.resource); ctx->index_offset = 0; - needs_indices = !panfrost_minmax_cache_get(ctx->index_res->index_cache, draw->start, - draw->count, &ctx->min_index, &ctx->max_index); + needs_indices = !panfrost_minmax_cache_get(ctx->index_res->index_cache, info->index_size, + draw->start, draw->count, + &ctx->min_index, &ctx->max_index); } if (needs_indices) { u_vbuf_get_minmax_index(pctx, info, draw, &ctx->min_index, &ctx->max_index); if (!info->has_user_indices) - panfrost_minmax_cache_add(ctx->index_res->index_cache, draw->start, draw->count, + panfrost_minmax_cache_add(ctx->index_res->index_cache, info->index_size, + draw->start, draw->count, ctx->min_index, ctx->max_index); } diff --git a/src/gallium/drivers/lima/lima_resource.c b/src/gallium/drivers/lima/lima_resource.c index 247a6895bd9..210c24a6fbd 100644 --- a/src/gallium/drivers/lima/lima_resource.c +++ b/src/gallium/drivers/lima/lima_resource.c @@ -708,7 +708,9 @@ lima_transfer_map(struct pipe_context *pctx, ptrans->layer_stride = res->levels[level].layer_stride; if ((usage & PIPE_MAP_WRITE) && (usage & PIPE_MAP_DIRECTLY)) - panfrost_minmax_cache_invalidate(res->index_cache, ptrans->box.x, ptrans->box.width); + panfrost_minmax_cache_invalidate(res->index_cache, + util_format_get_blocksize(pres->format), + ptrans->box.x, ptrans->box.width); return bo->map + res->levels[level].offset + box->z * res->levels[level].layer_stride + @@ -817,7 +819,9 @@ lima_transfer_unmap(struct pipe_context *pctx, if (trans->staging) free(trans->staging); if (ptrans->usage & PIPE_MAP_WRITE) { - panfrost_minmax_cache_invalidate(res->index_cache, ptrans->box.x, ptrans->box.width); + panfrost_minmax_cache_invalidate(res->index_cache, + util_format_get_blocksize(res->base.format), + ptrans->box.x, ptrans->box.width); } pipe_resource_reference(&ptrans->resource, NULL); diff --git a/src/gallium/drivers/panfrost/pan_helpers.c b/src/gallium/drivers/panfrost/pan_helpers.c index f94fd868f7b..1d80ef4067c 100644 --- a/src/gallium/drivers/panfrost/pan_helpers.c +++ b/src/gallium/drivers/panfrost/pan_helpers.c @@ -141,7 +141,8 @@ panfrost_get_index_buffer_bounded(struct panfrost_batch *batch, } else if (!info->has_user_indices) { /* Check the cache */ needs_indices = !panfrost_minmax_cache_get( - rsrc->index_cache, draw->start, draw->count, min_index, max_index); + rsrc->index_cache, info->index_size, draw->start, draw->count, + min_index, max_index); } if (needs_indices) { @@ -149,7 +150,8 @@ panfrost_get_index_buffer_bounded(struct panfrost_batch *batch, u_vbuf_get_minmax_index(&ctx->base, info, draw, min_index, max_index); if (!info->has_user_indices) - panfrost_minmax_cache_add(rsrc->index_cache, draw->start, draw->count, + panfrost_minmax_cache_add(rsrc->index_cache, info->index_size, + draw->start, draw->count, *min_index, *max_index); } diff --git a/src/gallium/drivers/panfrost/pan_resource.c b/src/gallium/drivers/panfrost/pan_resource.c index e4af8248acb..249442c0504 100644 --- a/src/gallium/drivers/panfrost/pan_resource.c +++ b/src/gallium/drivers/panfrost/pan_resource.c @@ -1431,7 +1431,9 @@ panfrost_ptr_map(struct pipe_context *pctx, struct pipe_resource *resource, if (usage & PIPE_MAP_WRITE) { BITSET_SET(rsrc->valid.data, level); panfrost_minmax_cache_invalidate( - rsrc->index_cache, transfer->base.box.x, transfer->base.box.width); + rsrc->index_cache, + util_format_get_blocksize(rsrc->base.format), + transfer->base.box.x, transfer->base.box.width); } return bo->ptr.cpu + rsrc->image.layout.slices[level].offset + @@ -1892,7 +1894,9 @@ panfrost_ptr_unmap(struct pipe_context *pctx, struct pipe_transfer *transfer) transfer->box.x + transfer->box.width); if (transfer->usage & PIPE_MAP_WRITE) { - panfrost_minmax_cache_invalidate(prsrc->index_cache, transfer->box.x, + panfrost_minmax_cache_invalidate(prsrc->index_cache, + util_format_get_blocksize(prsrc->base.format), + transfer->box.x, transfer->box.width); } diff --git a/src/panfrost/shared/pan_minmax_cache.c b/src/panfrost/shared/pan_minmax_cache.c index 11f46973405..78eedffe4c4 100644 --- a/src/panfrost/shared/pan_minmax_cache.c +++ b/src/panfrost/shared/pan_minmax_cache.c @@ -41,17 +41,59 @@ #include "pan_minmax_cache.h" #include "util/macros.h" -bool -panfrost_minmax_cache_get(struct panfrost_minmax_cache *cache, unsigned start, - unsigned count, unsigned *min_index, - unsigned *max_index) +/* + * note: a count of 0 would be an empty range, which we don't have to + * cache; so returning a 0 to indicate "do not cache" is sensible + * otherwise create a key that encodes the start, count, and index size + */ +static uint64_t +panfrost_calc_cache_key(struct panfrost_minmax_cache *cache, unsigned index_size, + unsigned start, unsigned count) { - uint64_t ht_key = (((uint64_t)count) << 32) | start; - bool found = false; + uint64_t ht_key; if (!cache) + return 0; /* do not cache if no cache! */ + + /* we're going to store the item size in the upper bits of the count; + * if the count is too big to do this safely, bail and do not use + * the cache (this case is going to be horrible no matter what we do, + * and is highly unlikely) + */ + if (count > 0x3FFFFFFF) + return 0; /* do not cache */ + + /* find log2(index_size) or die tryin' */ + switch (index_size) { + case 1: + index_size = 0; + break; + case 2: + index_size = 1; + break; + case 4: + index_size = 2; + break; + default: + unreachable("unknown index size"); + } + count = count | (index_size << 30); + ht_key = ((uint64_t)count << 32) | start; + return ht_key; +} + +bool +panfrost_minmax_cache_get(struct panfrost_minmax_cache *cache, unsigned index_size, + unsigned start, unsigned count, + unsigned *min_index, unsigned *max_index) +{ + uint64_t ht_key = panfrost_calc_cache_key(cache, index_size, start, count); + bool found = false; + + if (!ht_key) return false; + for (unsigned i = 0; i < cache->size; ++i) { if (cache->keys[i] == ht_key) { uint64_t hit = cache->values[i]; @@ -67,15 +109,15 @@ panfrost_minmax_cache_get(struct panfrost_minmax_cache *cache, unsigned start, } void -panfrost_minmax_cache_add(struct panfrost_minmax_cache *cache, unsigned start, - unsigned count, unsigned min_index, - unsigned max_index) +panfrost_minmax_cache_add(struct panfrost_minmax_cache *cache, unsigned index_size, + unsigned start, unsigned count, + unsigned min_index, unsigned max_index) { - uint64_t ht_key = (((uint64_t)count) << 32) | start; + uint64_t ht_key = panfrost_calc_cache_key(cache, index_size, start, count); uint64_t value = min_index | (((uint64_t)max_index) << 32); unsigned index = 0; - if (!cache) + if (!ht_key) return; if (cache->size == PANFROST_MINMAX_SIZE) { @@ -95,19 +137,30 @@ panfrost_minmax_cache_add(struct panfrost_minmax_cache *cache, unsigned start, void panfrost_minmax_cache_invalidate(struct panfrost_minmax_cache *cache, + unsigned index_size, size_t offset, size_t size) { /* Ensure there is a cache to invalidate and a write */ if (!cache) return; + /* convert offset and size to bytes, so that if we + update a region using a different item size we + still invalidate it */ + offset *= index_size; + size *= index_size; unsigned valid_count = 0; for (unsigned i = 0; i < cache->size; ++i) { uint64_t key = cache->keys[i]; - uint32_t start = key & 0xffffffff; - uint32_t count = key >> 32; + /* the item size is in the upper 2 bits of the key + * as above, convert size and count to bytes to make + * region comparison agnostic to item size + */ + uint32_t key_index_size = (key >> 62); + size_t count = ((key >> 32) & 0x3fffffff) << key_index_size; + size_t start = (key & 0xffffffff) << key_index_size; /* 1D range intersection */ bool invalid = MAX2(offset, start) < diff --git a/src/panfrost/shared/pan_minmax_cache.h b/src/panfrost/shared/pan_minmax_cache.h index 417745e5064..f1bf9890c1d 100644 --- a/src/panfrost/shared/pan_minmax_cache.h +++ b/src/panfrost/shared/pan_minmax_cache.h @@ -41,14 +41,17 @@ struct panfrost_minmax_cache { }; bool panfrost_minmax_cache_get(struct panfrost_minmax_cache *cache, + unsigned index_size, unsigned start, unsigned count, unsigned *min_index, unsigned *max_index); void panfrost_minmax_cache_add(struct panfrost_minmax_cache *cache, + unsigned index_size, unsigned start, unsigned count, unsigned min_index, unsigned max_index); void panfrost_minmax_cache_invalidate(struct panfrost_minmax_cache *cache, + unsigned index_size, size_t offset, size_t size); #endif