+Rob On Tue, 17 Sep 2019 17:13:04 +0200 Boris Brezillon <boris.brezil...@collabora.com> wrote:
> On Tue, 17 Sep 2019 08:36:56 -0400 > Alyssa Rosenzweig <aly...@rosenzweig.io> wrote: > > > "Can't use pipe_framebuffer_state as a hash key" Is this still relevant? > > I thought we did this. > > I did this yes. I thought it was only a problem for the wallpaper > draw, but it's actually wrong for any kind of blit where src and > dst point to the same resource, you're right. I guess the solution > would be to not use util_framebuffer_state_equal() but compare the > resources pointed by surfaces contained in the fb state, and not hash > the fb state directly, but hash its resource pointers instead (pretty > much what was done before my patch :-/). I tried implementing my own hash/compare funcs for the FBO key (instead of using util_framebuffer_state_equal() and hashing the whole struct) based on the freedreno logic, and I added an assert to see if there was cases where util_framebuffer_state_equal() was returning something different (see the below diff). There are indeed 2 deqp-gles2 tests that trigger this assert. So, now the question is, is it worth fixing, or can we live with the extra batch addition because we can't figure out that 2 FBOs are identical in rare occasions. There's still something I don't understand in the freedreno logic: they say they don't want to retain refs to the surface pointed by the fb state, because refcounting gets messy then, but I don't get why, especially since they retain refs to the resources pointed by those surfaces. Anyway, I guess all of this can be sorted out after this series has landed. --->8--- diff --git a/src/gallium/drivers/panfrost/pan_job.c b/src/gallium/drivers/panfrost/pan_job.c index 04257727ee0a..a90c427f6042 100644 --- a/src/gallium/drivers/panfrost/pan_job.c +++ b/src/gallium/drivers/panfrost/pan_job.c @@ -100,6 +100,74 @@ panfrost_batch_fence_reference(struct panfrost_batch_fence *fence) pipe_reference(NULL, &fence->reference); } +static bool +panfrost_batch_surf_compare(const struct pipe_surface *a, + const struct pipe_surface *b) +{ + if (!a || !b) + return a == b; + + if (a->texture != b->texture || + memcmp(&a->u, &b->u, sizeof(a->u)) || + a->nr_samples != b->nr_samples || + a->format != b->format) + return false; + + return true; +} + +static bool +panfrost_batch_compare(const void *a, const void *b) +{ + const struct pipe_framebuffer_state *fba = a, *fbb = b; + unsigned i; + + if (fba->width != fbb->width || fba->height != fbb->height || + fba->layers != fbb->layers || fba->samples != fbb->samples || + fba->nr_cbufs != fbb->nr_cbufs) + return false; + + for (i = 0; i < fba->nr_cbufs; i++) { + if (!panfrost_batch_surf_compare(fba->cbufs[i], fbb->cbufs[i])) + return false; + } + + return panfrost_batch_surf_compare(fba->zsbuf, fbb->zsbuf); +} + +static uint32_t +panfrost_batch_surf_hash(uint32_t hash, const struct pipe_surface *surf) +{ + uint32_t fmt_samples = surf ? (surf->format | (surf->nr_samples << 16)) : 0; + struct pipe_resource *rsrc = surf ? surf->texture : NULL; + union pipe_surface_desc dummy_desc = { }; + + hash = _mesa_fnv32_1a_accumulate_block(hash, &rsrc, sizeof(rsrc)); + hash = _mesa_fnv32_1a_accumulate_block(hash, + surf ? &surf->u : &dummy_desc, + sizeof(surf->u)); + return _mesa_fnv32_1a_accumulate_block(hash, &fmt_samples, + sizeof(fmt_samples)); +} + +static uint32_t +panfrost_batch_hash(const void *key) +{ + const struct pipe_framebuffer_state *fb = key; + uint64_t header = fb->width | fb->height << 16 | + (uint64_t)fb->layers << 32 | + (uint64_t)fb->samples << 48 | + (uint64_t)fb->nr_cbufs << 56; + uint32_t hash = _mesa_fnv32_1a_offset_bias; + unsigned i; + + hash = _mesa_fnv32_1a_accumulate_block(hash, &header, sizeof(header)); + for (i = 0; i < ARRAY_SIZE(fb->cbufs); i++) + hash = panfrost_batch_surf_hash(hash, fb->cbufs[i]); + + return panfrost_batch_surf_hash(hash, fb->zsbuf); +} + static struct panfrost_batch * panfrost_create_batch(struct panfrost_context *ctx, const struct pipe_framebuffer_state *key) @@ -252,8 +320,10 @@ panfrost_get_batch(struct panfrost_context *ctx, struct hash_entry *entry = _mesa_hash_table_search(ctx->fbo_to_batch, key); - if (entry) + if (entry) { + assert(util_framebuffer_state_equal(entry->key, key)); return entry->data; + } /* Otherwise, let's create a job */ @@ -1115,18 +1185,6 @@ panfrost_batch_clear(struct panfrost_batch *batch, ctx->pipe_framebuffer.height); } -static bool -panfrost_batch_compare(const void *a, const void *b) -{ - return util_framebuffer_state_equal(a, b); -} - -static uint32_t -panfrost_batch_hash(const void *key) -{ - return _mesa_hash_data(key, sizeof(struct pipe_framebuffer_state)); -} - /* Given a new bounding rectangle (scissor), let the job cover the union of the * new and old bounding rectangles */ _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev