iris: Fix bad external BO hash table and zombie list interactions

A while ago, we started deferring GEM object closure and VMA release
until buffers were idle.  This had some unforeseen interactions with
external buffers.

We keep imported buffers in hash tables, so if we have repeated imports
of the same GEM object, we map those to the same iris_bo structure.
This is critical for several reasons.  Unfortunately, we broke this
assumption.  When freeing a non-idle external buffer, we would drop it
from the hash tables, then move it to the zombie list.  If someone
reimported the same GEM object, we would not find it in the hash tables,
and go ahead and make a second iris_bo for that GEM object.  But the old
iris_bo would still be in the zombie list, and so we would eventually
call GEM_CLOSE on it - closing a BO that should have still been live.

To work around this, we defer removing a BO from the hash tables until
it's actually fully closed.  This has the strange effect that an
external BO may be on the zombie list, and yet be resurrected before
it can be properly cleaned up.  In this case, we remove it from the
list so it won't be freed.

Fixes severe instability in Weston, which was hitting EINVALs and
ENOENTs from execbuf2, due to batches referring to a GEM object that
had been closed, or at least had its VMA torched.

Fixes: 457a55716ea ("iris: Defer closing and freeing VMA until buffers are idle.")

diff --git a/src/gallium/drivers/iris/iris_bufmgr.c b/src/gallium/drivers/iris/iris_bufmgr.c
index 0986e2eab8596068923a3e46a467b8af6d9544fb..d5e7a1e85ba4efb72035ac052fb3f3ea75ccc908 100644 (file)
--- a/src/gallium/drivers/iris/iris_bufmgr.c
+++ b/src/gallium/drivers/iris/iris_bufmgr.c
@@ -180,6 +180,15 @@ find_and_ref_external_bo(struct hash_table *ht, unsigned int key)
 
    if (bo) {
       assert(bo->external);
+      assert(!bo->reusable);
+
+      /* Being non-reusable, the BO cannot be in the cache lists, but it
+       * may be in the zombie list if it had reached zero references, but
+       * we hadn't yet closed it...and then reimported the same BO.  If it
+       * is, then remove it since it's now been resurrected.
+       */
+      if (bo->head.prev || bo->head.next)
+         list_del(&bo->head);
 
       iris_bo_reference(bo);
    }
@@ -701,6 +710,18 @@ bo_close(struct iris_bo *bo)
 {
    struct iris_bufmgr *bufmgr = bo->bufmgr;
 
+   if (bo->external) {
+      struct hash_entry *entry;
+
+      if (bo->global_name) {
+         entry = _mesa_hash_table_search(bufmgr->name_table, &bo->global_name);
+         _mesa_hash_table_remove(bufmgr->name_table, entry);
+      }
+
+      entry = _mesa_hash_table_search(bufmgr->handle_table, &bo->gem_handle);
+      _mesa_hash_table_remove(bufmgr->handle_table, entry);
+   }
+
    /* Close this object */
    struct drm_gem_close close = { .handle = bo->gem_handle };
    int ret = gen_ioctl(bufmgr->fd, DRM_IOCTL_GEM_CLOSE, &close);
@@ -733,18 +754,6 @@ bo_free(struct iris_bo *bo)
       munmap(bo->map_gtt, bo->size);
    }
 
-   if (bo->external) {
-      struct hash_entry *entry;
-
-      if (bo->global_name) {
-         entry = _mesa_hash_table_search(bufmgr->name_table, &bo->global_name);
-         _mesa_hash_table_remove(bufmgr->name_table, entry);
-      }
-
-      entry = _mesa_hash_table_search(bufmgr->handle_table, &bo->gem_handle);
-      _mesa_hash_table_remove(bufmgr->handle_table, entry);
-   }
-
    if (bo->idle) {
       bo_close(bo);
    } else {