[Mesa-dev] [PATCH v2 00/37] panfrost: Support batch pipelining

[Boris Brezillon]

+Rob

On Tue, 17 Sep 2019 17:13:04 +0200
Boris Brezillon <boris.brezillon at collabora.com> wrote:

> On Tue, 17 Sep 2019 08:36:56 -0400
> Alyssa Rosenzweig <alyssa at 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 */