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@chromium.org>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/11200>

diff --git a/src/gallium/drivers/freedreno/freedreno_batch.c b/src/gallium/drivers/freedreno/freedreno_batch.c
index 0f4e91a..e1704d0 100644 (file)
--- 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);
 
@@ -275,20 +275,15 @@ 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 dc3a3ff..535d0de 100644 (file)
--- 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 78fc816..751c34a 100644 (file)
--- 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);