panfrost: Fix reference counting with batch->resources

Refactor accesses to batch->resources to happen through safe helpers
that update the appropriate bookkeeping. This makes it obvious that (in
particular) reference counts are updated when they should be.

The functional change is that we are now correctly unreferencing
resources during shadowing, fixing a leak of shadowed resources.

Closes: #7362
Fixes: 2d8f28df731 ("panfrost: Replace resource shadowing flush")
Signed-off-by: Alyssa Rosenzweig <alyssa@collabora.com>
Reported-by: Mastodon, apparently
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/19753>
(cherry picked from commit 42212a9bfdab8381beb9206b5d2551344c71d584)

3 files changed, 75 insertions, 45 deletions

diff --git a/.pick_status.json b/.pick_status.json
index b873cc20510..61e4b000f8e 100644
--- a/.pick_status.json
+++ b/.pick_status.json
@@ -1057,7 +1057,7 @@
         "description": "panfrost: Fix reference counting with batch->resources",
         "nominated": true,
         "nomination_type": 1,
-        "resolution": 0,
+        "resolution": 1,
         "main_sha": null,
         "because_sha": "2d8f28df731638e1270b0ae273c7bfd2b29b7993"
     },
diff --git a/src/gallium/drivers/panfrost/pan_job.c b/src/gallium/drivers/panfrost/pan_job.c
index 6ef813c2ecc..54e294f6c48 100644
--- a/src/gallium/drivers/panfrost/pan_job.c
+++ b/src/gallium/drivers/panfrost/pan_job.c
@@ -99,6 +99,74 @@ panfrost_batch_init(struct panfrost_context *ctx,
         screen->vtbl.init_batch(batch);
 }
 
+/*
+ * Safe helpers for manipulating batch->resources follow. In addition to
+ * wrapping the underlying set operations, these update the required
+ * bookkeeping for resource tracking and reference counting.
+ */
+static bool
+panfrost_batch_uses_resource(struct panfrost_batch *batch,
+                             struct panfrost_resource *rsrc)
+{
+        return _mesa_set_search(batch->resources, rsrc) != NULL;
+}
+
+static void
+panfrost_batch_add_resource(struct panfrost_batch *batch,
+                            struct panfrost_resource *rsrc)
+{
+        bool found = false;
+        _mesa_set_search_or_add(batch->resources, rsrc, &found);
+
+        if (!found) {
+                /* Cache number of batches accessing a resource */
+                rsrc->track.nr_users++;
+
+                /* Reference the resource on the batch */
+                pipe_reference(NULL, &rsrc->base.reference);
+        }
+}
+
+static void
+panfrost_batch_remove_resource_internal(struct panfrost_context *ctx,
+                                        struct panfrost_resource *rsrc)
+{
+        struct hash_entry *writer = _mesa_hash_table_search(ctx->writers, rsrc);
+        if (writer) {
+                _mesa_hash_table_remove(ctx->writers, writer);
+                rsrc->track.nr_writers--;
+        }
+
+        rsrc->track.nr_users--;
+        pipe_resource_reference((struct pipe_resource **) &rsrc, NULL);
+}
+
+static void
+panfrost_batch_remove_resource_if_present(struct panfrost_context *ctx,
+                                          struct panfrost_batch *batch,
+                                          struct panfrost_resource *rsrc)
+{
+        struct set_entry *ent = _mesa_set_search(batch->resources, rsrc);
+
+        if (ent != NULL) {
+                panfrost_batch_remove_resource_internal(ctx, rsrc);
+                _mesa_set_remove(batch->resources, ent);
+        }
+}
+
+static void
+panfrost_batch_destroy_resources(struct panfrost_context *ctx,
+                                 struct panfrost_batch *batch)
+{
+        set_foreach(batch->resources, entry) {
+                struct panfrost_resource *rsrc = (void *) entry->key;
+
+                panfrost_batch_remove_resource_internal(ctx, rsrc);
+        }
+
+        _mesa_set_destroy(batch->resources, NULL);
+}
+
 static void
 panfrost_batch_cleanup(struct panfrost_context *ctx, struct panfrost_batch *batch)
 {
@@ -122,20 +190,7 @@ panfrost_batch_cleanup(struct panfrost_context *ctx, struct panfrost_batch *batc
                 panfrost_bo_unreference(bo);
         }
 
-        set_foreach(batch->resources, entry) {
-                struct panfrost_resource *rsrc = (void *) entry->key;
-
-                if (_mesa_hash_table_search(ctx->writers, rsrc)) {
-                        _mesa_hash_table_remove_key(ctx->writers, rsrc);
-                        rsrc->track.nr_writers--;
-                }
-
-                rsrc->track.nr_users--;
-
-                pipe_resource_reference((struct pipe_resource **) &rsrc, NULL);
-        }
-
-        _mesa_set_destroy(batch->resources, NULL);
+        panfrost_batch_destroy_resources(ctx, batch);
         panfrost_pool_cleanup(&batch->pool);
         panfrost_pool_cleanup(&batch->invisible_pool);
 
@@ -239,17 +294,8 @@ panfrost_batch_update_access(struct panfrost_batch *batch,
         uint32_t batch_idx = panfrost_batch_idx(batch);
         struct hash_entry *entry = _mesa_hash_table_search(ctx->writers, rsrc);
         struct panfrost_batch *writer = entry ? entry->data : NULL;
-        bool found = false;
 
-        _mesa_set_search_or_add(batch->resources, rsrc, &found);
-
-        if (!found) {
-                /* Cache number of batches accessing a resource */
-                rsrc->track.nr_users++;
-
-                /* Reference the resource on the batch */
-                pipe_reference(NULL, &rsrc->base.reference);
-        }
+        panfrost_batch_add_resource(batch, rsrc);
 
         /* Flush users if required */
         if (writes || ((writer != NULL) && (writer != batch))) {
@@ -262,7 +308,7 @@ panfrost_batch_update_access(struct panfrost_batch *batch,
                                 continue;
 
                         /* Submit if it's a user */
-                        if (_mesa_set_search(batch->resources, rsrc))
+                        if (panfrost_batch_uses_resource(batch, rsrc))
                                 panfrost_batch_submit(ctx, batch);
                 }
         }
@@ -369,30 +415,14 @@ panfrost_resource_swap_bo(struct panfrost_context *ctx,
                           struct panfrost_resource *rsrc,
                           struct panfrost_bo *newbo)
 {
-        /* Any batch writing this resource is writing to the old BO, not the
-         * new BO. After swapping the resource's backing BO, there will be no
-         * writers of the updated resource. Existing writers still hold a
-         * reference to the old BO for reference counting.
-         */
-        struct hash_entry *writer = _mesa_hash_table_search(ctx->writers, rsrc);
-        if (writer) {
-                _mesa_hash_table_remove(ctx->writers, writer);
-                rsrc->track.nr_writers--;
-        }
-
         /* Likewise, any batch reading this resource is reading the old BO, and
          * after swapping will not be reading this resource.
          */
         unsigned i;
         foreach_batch(ctx, i) {
                 struct panfrost_batch *batch = &ctx->batches.slots[i];
-                struct set_entry *ent = _mesa_set_search(batch->resources, rsrc);
-
-                if (!ent)
-                        continue;
 
-                _mesa_set_remove(batch->resources, ent);
-                rsrc->track.nr_users--;
+                panfrost_batch_remove_resource_if_present(ctx, batch, rsrc);
         }
 
         /* Swap the pointers, dropping a reference to the old BO which is no
@@ -897,7 +927,7 @@ panfrost_flush_batches_accessing_rsrc(struct panfrost_context *ctx,
         foreach_batch(ctx, i) {
                 struct panfrost_batch *batch = &ctx->batches.slots[i];
 
-                if (!_mesa_set_search(batch->resources, rsrc))
+                if (!panfrost_batch_uses_resource(batch, rsrc))
                         continue;
 
                 perf_debug_ctx(ctx, "Flushing user due to: %s", reason);
diff --git a/src/gallium/drivers/panfrost/pan_job.h b/src/gallium/drivers/panfrost/pan_job.h
index fb62529a130..8840ff7487b 100644
--- a/src/gallium/drivers/panfrost/pan_job.h
+++ b/src/gallium/drivers/panfrost/pan_job.h
@@ -192,7 +192,7 @@ struct panfrost_batch {
         struct pan_tristate sprite_coord_origin;
         struct pan_tristate first_provoking_vertex;
 
-        /* Referenced resources */
+        /* Referenced resources, holds a pipe_reference. */
         struct set *resources;
 };