From 64b73b770b71c15b5628545fed132f81a51bd555 Mon Sep 17 00:00:00 2001 From: Kenneth Graunke Date: Sat, 3 Aug 2019 03:13:55 -0700 Subject: [PATCH] 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.") --- src/gallium/drivers/iris/iris_bufmgr.c | 33 ++++++++++++++++---------- 1 file changed, 21 insertions(+), 12 deletions(-) diff --git a/src/gallium/drivers/iris/iris_bufmgr.c b/src/gallium/drivers/iris/iris_bufmgr.c index 0986e2eab85..d5e7a1e85ba 100644 --- 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 { -- 2.30.2