From aba3aed96e4394a213e188f2f71ef045803a27c5 Mon Sep 17 00:00:00 2001 From: Lionel Landwerlin Date: Sat, 2 May 2020 16:46:47 +0300 Subject: [PATCH] iris: fix export of GEM handles MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit We reuse DRM file descriptors internally. Therefore when we export a GEM handle we must do so in the file descriptor used externally. This change also fixes a file descriptor leak of the FD given at screen creation. v2: Don't bother checking fd equals, they're always different Fix dmabuf leak Fix GEM handle leaks by tracking exported handles v3: Check os_same_file_description error (Michel) Don't create multiple exports for a given GEM table v4: Add WARN_ONCE (Ken) Rename external_fd to winsys_fd v5: Remove export lock in favor of bufmgr's Signed-off-by: Lionel Landwerlin Closes: https://gitlab.freedesktop.org/mesa/mesa/-/issues/2882 Fixes: 7557f1605968 ("iris: share buffer managers accross screens") Tested-by: Eric Engestrom Tested-by: Tapani Pälli Reviewed-by: Kenneth Graunke Part-of: --- src/gallium/drivers/iris/iris_bufmgr.c | 106 ++++++++++++++++++++++- src/gallium/drivers/iris/iris_bufmgr.h | 16 ++++ src/gallium/drivers/iris/iris_resource.c | 33 +++++-- src/gallium/drivers/iris/iris_screen.c | 2 + src/gallium/drivers/iris/iris_screen.h | 8 +- 5 files changed, 155 insertions(+), 10 deletions(-) diff --git a/src/gallium/drivers/iris/iris_bufmgr.c b/src/gallium/drivers/iris/iris_bufmgr.c index f2cd53b4495..bc9ad13c171 100644 --- a/src/gallium/drivers/iris/iris_bufmgr.c +++ b/src/gallium/drivers/iris/iris_bufmgr.c @@ -63,6 +63,7 @@ #include "util/macros.h" #include "util/hash_table.h" #include "util/list.h" +#include "util/os_file.h" #include "util/u_dynarray.h" #include "util/vma.h" #include "iris_bufmgr.h" @@ -91,6 +92,17 @@ #define PAGE_SIZE 4096 +#define WARN_ONCE(cond, fmt...) do { \ + if (unlikely(cond)) { \ + static bool _warned = false; \ + if (!_warned) { \ + fprintf(stderr, "WARNING: "); \ + fprintf(stderr, fmt); \ + _warned = true; \ + } \ + } \ +} while (0) + #define FILE_DEBUG_FLAG DEBUG_BUFMGR static inline int @@ -126,6 +138,16 @@ struct bo_cache_bucket { uint64_t size; }; +struct bo_export { + /** File descriptor associated with a handle export. */ + int drm_fd; + + /** GEM handle in drm_fd */ + uint32_t gem_handle; + + struct list_head link; +}; + struct iris_bufmgr { /** * List into the list of bufmgr. @@ -349,9 +371,13 @@ static struct iris_bo * bo_calloc(void) { struct iris_bo *bo = calloc(1, sizeof(*bo)); - if (bo) { - bo->hash = _mesa_hash_pointer(bo); - } + if (!bo) + return NULL; + + list_inithead(&bo->exports); + + bo->hash = _mesa_hash_pointer(bo); + return bo; } @@ -705,6 +731,7 @@ iris_bo_gem_create_from_name(struct iris_bufmgr *bufmgr, goto err_unref; bo->tiling_mode = get_tiling.tiling_mode; + /* XXX stride is unknown */ DBG("bo_create_from_handle: %d (%s)\n", handle, bo->name); @@ -733,6 +760,16 @@ bo_close(struct iris_bo *bo) entry = _mesa_hash_table_search(bufmgr->handle_table, &bo->gem_handle); _mesa_hash_table_remove(bufmgr->handle_table, entry); + + list_for_each_entry_safe(struct bo_export, export, &bo->exports, link) { + struct drm_gem_close close = { .handle = export->gem_handle }; + gen_ioctl(export->drm_fd, DRM_IOCTL_GEM_CLOSE, &close); + + list_del(&export->link); + free(export); + } + } else { + assert(list_is_empty(&bo->exports)); } /* Close this object */ @@ -1477,6 +1514,69 @@ iris_bo_flink(struct iris_bo *bo, uint32_t *name) return 0; } +int +iris_bo_export_gem_handle_for_device(struct iris_bo *bo, int drm_fd, + uint32_t *out_handle) +{ + /* Only add the new GEM handle to the list of export if it belongs to a + * different GEM device. Otherwise we might close the same buffer multiple + * times. + */ + struct iris_bufmgr *bufmgr = bo->bufmgr; + int ret = os_same_file_description(drm_fd, bufmgr->fd); + WARN_ONCE(ret < 0, + "Kernel has no file descriptor comparison support: %s\n", + strerror(errno)); + if (ret == 0) { + *out_handle = iris_bo_export_gem_handle(bo); + return 0; + } + + struct bo_export *export = calloc(1, sizeof(*export)); + if (!export) + return -ENOMEM; + + export->drm_fd = drm_fd; + + int dmabuf_fd = -1; + int err = iris_bo_export_dmabuf(bo, &dmabuf_fd); + if (err) { + free(export); + return err; + } + + mtx_lock(&bufmgr->lock); + err = drmPrimeFDToHandle(drm_fd, dmabuf_fd, &export->gem_handle); + close(dmabuf_fd); + if (err) { + mtx_unlock(&bufmgr->lock); + free(export); + return err; + } + + bool found = false; + list_for_each_entry(struct bo_export, iter, &bo->exports, link) { + if (iter->drm_fd != drm_fd) + continue; + /* Here we assume that for a given DRM fd, we'll always get back the + * same GEM handle for a given buffer. + */ + assert(iter->gem_handle == export->gem_handle); + free(export); + export = iter; + found = true; + break; + } + if (!found) + list_addtail(&export->link, &bo->exports); + + mtx_unlock(&bufmgr->lock); + + *out_handle = export->gem_handle; + + return 0; +} + static void add_bucket(struct iris_bufmgr *bufmgr, int size) { diff --git a/src/gallium/drivers/iris/iris_bufmgr.h b/src/gallium/drivers/iris/iris_bufmgr.h index 2966d993ff6..7c4a88703f8 100644 --- a/src/gallium/drivers/iris/iris_bufmgr.h +++ b/src/gallium/drivers/iris/iris_bufmgr.h @@ -28,6 +28,7 @@ #include #include #include +#include "c11/threads.h" #include "util/macros.h" #include "util/u_atomic.h" #include "util/list.h" @@ -193,6 +194,9 @@ struct iris_bo { /** BO cache list */ struct list_head head; + /** List of GEM handle exports of this buffer (bo_export) */ + struct list_head exports; + /** * Synchronization sequence number of most recent access of this BO from * each caching domain. @@ -391,6 +395,18 @@ int iris_bo_export_dmabuf(struct iris_bo *bo, int *prime_fd); struct iris_bo *iris_bo_import_dmabuf(struct iris_bufmgr *bufmgr, int prime_fd, uint32_t tiling, uint32_t stride); +/** + * Exports a bo as a GEM handle into a given DRM file descriptor + * \param bo Buffer to export + * \param drm_fd File descriptor where the new handle is created + * \param out_handle Pointer to store the new handle + * + * Returns 0 if the buffer was successfully exported, a non zero error code + * otherwise. + */ +int iris_bo_export_gem_handle_for_device(struct iris_bo *bo, int drm_fd, + uint32_t *out_handle); + uint32_t iris_bo_export_gem_handle(struct iris_bo *bo); int iris_reg_read(struct iris_bufmgr *bufmgr, uint32_t offset, uint64_t *out); diff --git a/src/gallium/drivers/iris/iris_resource.c b/src/gallium/drivers/iris/iris_resource.c index 48ca9faf15b..513105d5c2c 100644 --- a/src/gallium/drivers/iris/iris_resource.c +++ b/src/gallium/drivers/iris/iris_resource.c @@ -1138,7 +1138,7 @@ iris_resource_disable_aux_on_first_query(struct pipe_resource *resource, } static bool -iris_resource_get_param(struct pipe_screen *screen, +iris_resource_get_param(struct pipe_screen *pscreen, struct pipe_context *context, struct pipe_resource *resource, unsigned plane, @@ -1147,6 +1147,7 @@ iris_resource_get_param(struct pipe_screen *screen, unsigned handle_usage, uint64_t *value) { + struct iris_screen *screen = (struct iris_screen *)pscreen; struct iris_resource *res = (struct iris_resource *)resource; bool mod_with_aux = res->mod_info && res->mod_info->aux_usage != ISL_AUX_USAGE_NONE; @@ -1155,7 +1156,7 @@ iris_resource_get_param(struct pipe_screen *screen, unsigned handle; if (iris_resource_unfinished_aux_import(res)) - iris_resource_finish_aux_import(screen, res); + iris_resource_finish_aux_import(pscreen, res); struct iris_bo *bo = wants_aux ? res->aux.bo : res->bo; @@ -1187,9 +1188,19 @@ iris_resource_get_param(struct pipe_screen *screen, if (result) *value = handle; return result; - case PIPE_RESOURCE_PARAM_HANDLE_TYPE_KMS: - *value = iris_bo_export_gem_handle(bo); + case PIPE_RESOURCE_PARAM_HANDLE_TYPE_KMS: { + /* Because we share the same drm file across multiple iris_screen, when + * we export a GEM handle we must make sure it is valid in the DRM file + * descriptor the caller is using (this is the FD given at screen + * creation). + */ + uint32_t handle; + if (iris_bo_export_gem_handle_for_device(bo, screen->winsys_fd, &handle)) + return false; + *value = handle; return true; + } + case PIPE_RESOURCE_PARAM_HANDLE_TYPE_FD: result = iris_bo_export_dmabuf(bo, (int *) &handle) == 0; if (result) @@ -1207,6 +1218,7 @@ iris_resource_get_handle(struct pipe_screen *pscreen, struct winsys_handle *whandle, unsigned usage) { + struct iris_screen *screen = (struct iris_screen *) pscreen; struct iris_resource *res = (struct iris_resource *)resource; bool mod_with_aux = res->mod_info && res->mod_info->aux_usage != ISL_AUX_USAGE_NONE; @@ -1244,9 +1256,18 @@ iris_resource_get_handle(struct pipe_screen *pscreen, switch (whandle->type) { case WINSYS_HANDLE_TYPE_SHARED: return iris_bo_flink(bo, &whandle->handle) == 0; - case WINSYS_HANDLE_TYPE_KMS: - whandle->handle = iris_bo_export_gem_handle(bo); + case WINSYS_HANDLE_TYPE_KMS: { + /* Because we share the same drm file across multiple iris_screen, when + * we export a GEM handle we must make sure it is valid in the DRM file + * descriptor the caller is using (this is the FD given at screen + * creation). + */ + uint32_t handle; + if (iris_bo_export_gem_handle_for_device(bo, screen->winsys_fd, &handle)) + return false; + whandle->handle = handle; return true; + } case WINSYS_HANDLE_TYPE_FD: return iris_bo_export_dmabuf(bo, (int *) &whandle->handle) == 0; } diff --git a/src/gallium/drivers/iris/iris_screen.c b/src/gallium/drivers/iris/iris_screen.c index 73ce8288724..374713cdcb2 100644 --- a/src/gallium/drivers/iris/iris_screen.c +++ b/src/gallium/drivers/iris/iris_screen.c @@ -527,6 +527,7 @@ iris_screen_destroy(struct iris_screen *screen) u_transfer_helper_destroy(screen->base.transfer_helper); iris_bufmgr_unref(screen->bufmgr); disk_cache_destroy(screen->disk_cache); + close(screen->winsys_fd); ralloc_free(screen); } @@ -706,6 +707,7 @@ iris_screen_create(int fd, const struct pipe_screen_config *config) return NULL; screen->fd = iris_bufmgr_get_fd(screen->bufmgr); + screen->winsys_fd = fd; if (getenv("INTEL_NO_HW") != NULL) screen->no_hw = true; diff --git a/src/gallium/drivers/iris/iris_screen.h b/src/gallium/drivers/iris/iris_screen.h index b11178bc5be..836954fc1d6 100644 --- a/src/gallium/drivers/iris/iris_screen.h +++ b/src/gallium/drivers/iris/iris_screen.h @@ -150,9 +150,15 @@ struct iris_screen { /** Global slab allocator for iris_transfer_map objects */ struct slab_parent_pool transfer_pool; - /** drm device file descriptor, on shared with bufmgr, do not close. */ + /** drm device file descriptor, shared with bufmgr, do not close. */ int fd; + /** + * drm device file descriptor to used for window system integration, owned + * by iris_screen, can be a different DRM instance than fd. + */ + int winsys_fd; + /** PCI ID for our GPU device */ int pci_id; -- 2.30.2