Mesa (main): freedreno: Fix batch flush race condition

[GitLab Mirror]

Module: Mesa
Branch: main
Commit: 507f701d9e89b891fdfa03ad23dc73acf3c5c6b4
URL:    http://cgit.freedesktop.org/mesa/mesa/commit/?id=507f701d9e89b891fdfa03ad23dc73acf3c5c6b4

Author: Rob Clark <robdclark at chromium.org>
Date:   Sat Jun  5 12:13:24 2021 -0700

freedreno: Fix batch flush race condition

We need to remove the batch cache entry before marking as flushed.

Note that we are already holding the batch lock, to prevent flushing
something that another context is emitting cmdstream to, but there is
a window between batch cache lookup (under screen lock) and acquiring
the batch lock that could previously result in batch cache lookup
returning a flushed batch.

Signed-off-by: Rob Clark <robdclark at chromium.org>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/11200>

---

 src/gallium/drivers/freedreno/freedreno_batch.c    | 37 ++++++++++------------
 src/gallium/drivers/freedreno/freedreno_batch.h    |  2 +-
 .../drivers/freedreno/freedreno_batch_cache.c      | 11 ++-----
 3 files changed, 21 insertions(+), 29 deletions(-)

diff --git a/src/gallium/drivers/freedreno/freedreno_batch.c b/src/gallium/drivers/freedreno/freedreno_batch.c
index 0f4e91a90cf..e1704d09723 100644
--- a/src/gallium/drivers/freedreno/freedreno_batch.c
+++ b/src/gallium/drivers/freedreno/freedreno_batch.c
@@ -260,7 +260,7 @@ batch_reset_dependencies(struct fd_batch *batch)
 }
 
 static void
-batch_reset_resources_locked(struct fd_batch *batch)
+batch_reset_resources(struct fd_batch *batch)
 {
    fd_screen_assert_locked(batch->ctx->screen);
 
@@ -274,21 +274,16 @@ batch_reset_resources_locked(struct fd_batch *batch)
    }
 }
 
-static void
-batch_reset_resources(struct fd_batch *batch) assert_dt
-{
-   fd_screen_lock(batch->ctx->screen);
-   batch_reset_resources_locked(batch);
-   fd_screen_unlock(batch->ctx->screen);
-}
-
 static void
 batch_reset(struct fd_batch *batch) assert_dt
 {
    DBG("%p", batch);
 
    batch_reset_dependencies(batch);
+
+   fd_screen_lock(batch->ctx->screen);
    batch_reset_resources(batch);
+   fd_screen_unlock(batch->ctx->screen);
 
    batch_fini(batch);
    batch_init(batch);
@@ -316,7 +311,7 @@ __fd_batch_destroy(struct fd_batch *batch)
 
    fd_bc_invalidate_batch(batch, true);
 
-   batch_reset_resources_locked(batch);
+   batch_reset_resources(batch);
    debug_assert(batch->resources->entries == 0);
    _mesa_set_destroy(batch->resources, NULL);
 
@@ -366,26 +361,28 @@ batch_flush(struct fd_batch *batch) assert_dt
 
    batch_flush_dependencies(batch);
 
+   fd_screen_lock(batch->ctx->screen);
+   batch_reset_resources(batch);
+   /* NOTE: remove=false removes the batch from the hashtable, so future
+    * lookups won't cache-hit a flushed batch, but leaves the weak reference
+    * to the batch to avoid having multiple batches with same batch->idx, as
+    * that causes all sorts of hilarity.
+    */
+   fd_bc_invalidate_batch(batch, false);
    batch->flushed = true;
+
    if (batch == batch->ctx->batch)
-      fd_batch_reference(&batch->ctx->batch, NULL);
+      fd_batch_reference_locked(&batch->ctx->batch, NULL);
+
+   fd_screen_unlock(batch->ctx->screen);
 
    if (batch->fence)
       fd_fence_ref(&batch->ctx->last_fence, batch->fence);
 
    fd_gmem_render_tiles(batch);
-   batch_reset_resources(batch);
 
    debug_assert(batch->reference.count > 0);
 
-   fd_screen_lock(batch->ctx->screen);
-   /* NOTE: remove=false removes the batch from the hashtable, so future
-    * lookups won't cache-hit a flushed batch, but leaves the weak reference
-    * to the batch to avoid having multiple batches with same batch->idx, as
-    * that causes all sorts of hilarity.
-    */
-   fd_bc_invalidate_batch(batch, false);
-   fd_screen_unlock(batch->ctx->screen);
    cleanup_submit(batch);
    fd_batch_unlock_submit(batch);
 }
diff --git a/src/gallium/drivers/freedreno/freedreno_batch.h b/src/gallium/drivers/freedreno/freedreno_batch.h
index dc3a3ff8527..535d0deb797 100644
--- a/src/gallium/drivers/freedreno/freedreno_batch.h
+++ b/src/gallium/drivers/freedreno/freedreno_batch.h
@@ -351,7 +351,7 @@ fd_batch_unlock_submit(struct fd_batch *batch)
 }
 
 /**
- * Returns true if emit-lock was aquired, false if failed to aquire lock,
+ * Returns true if emit-lock was acquired, false if failed to acquire lock,
  * ie. batch already flushed.
  */
 static inline bool MUST_CHECK
diff --git a/src/gallium/drivers/freedreno/freedreno_batch_cache.c b/src/gallium/drivers/freedreno/freedreno_batch_cache.c
index 78fc81691f4..751c34ada98 100644
--- a/src/gallium/drivers/freedreno/freedreno_batch_cache.c
+++ b/src/gallium/drivers/freedreno/freedreno_batch_cache.c
@@ -436,15 +436,10 @@ batch_from_key(struct fd_batch_cache *cache, struct fd_batch_key *key,
       _mesa_hash_table_search_pre_hashed(cache->ht, hash, key);
 
    if (entry) {
+      free(key);
       fd_batch_reference_locked(&batch, (struct fd_batch *)entry->data);
-
-      if (batch->flushed) {
-         fd_bc_invalidate_batch(batch, false);
-         fd_batch_reference_locked(&batch, NULL);
-      } else {
-         free(key);
-         return batch;
-      }
+      assert(!batch->flushed);
+      return batch;
    }
 
    batch = alloc_batch_locked(cache, ctx, false);