backend/drm: get rid of BO handle table

The BO handle table exists to avoid double-closing a BO handle,
which aren't reference-counted by the kernel. But if we can
guarantee that there is only ever a single ref for each BO handle,
then we don't need the BO handle table anymore.

This is possible if we create the handle right before the ADDFB2
IOCTL, and close the handle right after. The handles are very
short-lived and we don't need to track their lifetime.

Because of multi-planar FBs, we need to be a bit careful: some
FB planes might share the same handle. But with a small check, it's
easy to avoid double-closing the same handle (which wouldn't be a
big deal anyways).

There's one gotcha though: drmModeSetCursor2 takes a BO handle as
input. Saving the handles until drmModeSetCursor2 time would require
us to track BO handle lifetimes, so we wouldn't be able to get rid
of the BO handle table. As a workaround, use drmModeGetFB to turn the
FB ID back to a BO handle, call drmModeSetCursor2 and then immediately
close the BO handle. The overhead should be minimal since these IOCTLs
are pretty cheap.

Closes: https://github.com/swaywm/wlroots/issues/3164

backend/drm/backend.c

@@ -52,8 +52,6 @@ static void backend_destroy(struct wlr_backend *backend) {
 	wl_list_remove(&drm->dev_change.link);
 	wl_list_remove(&drm->dev_remove.link);
 
-	drm_bo_handle_table_finish(&drm->bo_handles);
-
 	if (drm->parent) {
 		finish_drm_renderer(&drm->mgpu_renderer);
 	}

backend/drm/bo_handle_table.c

@@ -1,43 +0,0 @@
-#include <assert.h>
-#include <stdint.h>
-#include <stdlib.h>
-#include <wlr/util/log.h>
-#include "backend/drm/bo_handle_table.h"
-
-static size_t align(size_t val, size_t align) {
-	size_t mask = align - 1;
-	return (val + mask) & ~mask;
-}
-
-void drm_bo_handle_table_finish(struct wlr_drm_bo_handle_table *table) {
-	free(table->nrefs);
-}
-
-bool drm_bo_handle_table_ref(struct wlr_drm_bo_handle_table *table,
-		uint32_t handle) {
-	assert(handle != 0);
-
-	if (handle > table->len) {
-		// Grow linearly, because we don't expect the number of BOs to explode
-		size_t len = align(handle + 1, 512);
-		size_t *nrefs = realloc(table->nrefs, len * sizeof(size_t));
-		if (nrefs == NULL) {
-			wlr_log_errno(WLR_ERROR, "realloc failed");
-			return false;
-		}
-		memset(&nrefs[table->len], 0, (len - table->len) * sizeof(size_t));
-		table->len = len;
-		table->nrefs = nrefs;
-	}
-
-	table->nrefs[handle]++;
-	return true;
-}
-
-size_t drm_bo_handle_table_unref(struct wlr_drm_bo_handle_table *table,
-		uint32_t handle) {
-	assert(handle < table->len);
-	assert(table->nrefs[handle] > 0);
-	table->nrefs[handle]--;
-	return table->nrefs[handle];
-}

backend/drm/legacy.c

@@ -138,11 +138,21 @@ static bool legacy_crtc_commit(struct wlr_drm_connector *conn,
 			return false;
 		}
 
-		uint32_t cursor_handle = cursor_fb->handles[0];
+		drmModeFB *drm_fb = drmModeGetFB(drm->fd, cursor_fb->id);
-		uint32_t cursor_width = cursor_fb->wlr_buf->width;
+		if (drm_fb == NULL) {
-		uint32_t cursor_height = cursor_fb->wlr_buf->height;
+			wlr_drm_conn_log_errno(conn, WLR_DEBUG, "Failed to get cursor "
-		if (drmModeSetCursor(drm->fd, crtc->id, cursor_handle,
+				"BO handle: drmModeGetFB failed");
-				cursor_width, cursor_height)) {
+			return false;
+		}
+		uint32_t cursor_handle = drm_fb->handle;
+		uint32_t cursor_width = drm_fb->width;
+		uint32_t cursor_height = drm_fb->height;
+		drmModeFreeFB(drm_fb);
+
+		int ret = drmModeSetCursor(drm->fd, crtc->id, cursor_handle,
+			cursor_width, cursor_height);
+		close_bo_handle(drm->fd, cursor_handle);
+		if (ret != 0) {
 			wlr_drm_conn_log_errno(conn, WLR_DEBUG, "drmModeSetCursor failed");
 			return false;
 		}

backend/drm/meson.build

@@ -1,7 +1,6 @@
 wlr_files += files(
 	'atomic.c',
 	'backend.c',
-	'bo_handle_table.c',
 	'cvt.c',
 	'drm.c',
 	'legacy.c',

backend/drm/renderer.c

@@ -204,42 +204,6 @@ static const struct wlr_addon_interface fb_addon_impl = {
 	.destroy = drm_fb_handle_destroy,
 };
 
-static uint32_t get_bo_handle_for_fd(struct wlr_drm_backend *drm,
-		int dmabuf_fd) {
-	uint32_t handle = 0;
-	int ret = drmPrimeFDToHandle(drm->fd, dmabuf_fd, &handle);
-	if (ret != 0) {
-		wlr_log_errno(WLR_DEBUG, "drmPrimeFDToHandle failed");
-		return 0;
-	}
-
-	if (!drm_bo_handle_table_ref(&drm->bo_handles, handle)) {
-		// If that failed, the handle wasn't ref'ed in the table previously,
-		// so safe to delete
-		struct drm_gem_close args = { .handle = handle };
-		drmIoctl(drm->fd, DRM_IOCTL_GEM_CLOSE, &args);
-		return 0;
-	}
-
-	return handle;
-}
-
-static void close_bo_handle(struct wlr_drm_backend *drm, uint32_t handle) {
-	if (handle == 0) {
-		return;
-	}
-
-	size_t nrefs = drm_bo_handle_table_unref(&drm->bo_handles, handle);
-	if (nrefs > 0) {
-		return;
-	}
-
-	struct drm_gem_close args = { .handle = handle };
-	if (drmIoctl(drm->fd, DRM_IOCTL_GEM_CLOSE, &args) != 0) {
-		wlr_log_errno(WLR_ERROR, "drmIoctl(GEM_CLOSE) failed");
-	}
-}
-
 static uint32_t get_fb_for_bo(struct wlr_drm_backend *drm,
 		struct wlr_dmabuf_attributes *dmabuf, uint32_t handles[static 4]) {
 	uint64_t modifiers[4] = {0};
@@ -281,6 +245,29 @@ static uint32_t get_fb_for_bo(struct wlr_drm_backend *drm,
 	return id;
 }
 
+static void close_all_bo_handles(struct wlr_drm_backend *drm,
+		uint32_t handles[static 4]) {
+	for (int i = 0; i < 4; ++i) {
+		if (handles[i] == 0) {
+			continue;
+		}
+
+		// If multiple planes share the same BO handle, avoid double-closing it
+		bool already_closed = false;
+		for (int j = 0; j < i; ++j) {
+			if (handles[i] == handles[j]) {
+				already_closed = true;
+				break;
+			}
+		}
+		if (already_closed) {
+			continue;
+		}
+
+		close_bo_handle(drm->fd, handles[i]);
+	}
+}
+
 static struct wlr_drm_fb *drm_fb_create(struct wlr_drm_backend *drm,
 		struct wlr_buffer *buf, const struct wlr_drm_format_set *formats) {
 	struct wlr_drm_fb *fb = calloc(1, sizeof(*fb));
@@ -318,19 +305,23 @@ static struct wlr_drm_fb *drm_fb_create(struct wlr_drm_backend *drm,
 		}
 	}
 
+	uint32_t handles[4] = {0};
 	for (int i = 0; i < attribs.n_planes; ++i) {
-		fb->handles[i] = get_bo_handle_for_fd(drm, attribs.fd[i]);
+		int ret = drmPrimeFDToHandle(drm->fd, attribs.fd[i], &handles[i]);
-		if (fb->handles[i] == 0) {
+		if (ret != 0) {
+			wlr_log_errno(WLR_DEBUG, "drmPrimeFDToHandle failed");
 			goto error_bo_handle;
 		}
 	}
 
-	fb->id = get_fb_for_bo(drm, &attribs, fb->handles);
+	fb->id = get_fb_for_bo(drm, &attribs, handles);
 	if (!fb->id) {
 		wlr_log(WLR_DEBUG, "Failed to import BO in KMS");
 		goto error_bo_handle;
 	}
 
+	close_all_bo_handles(drm, handles);
+
 	fb->backend = drm;
 	fb->wlr_buf = buf;
 
@@ -340,9 +331,7 @@ static struct wlr_drm_fb *drm_fb_create(struct wlr_drm_backend *drm,
 	return fb;
 
 error_bo_handle:
-	for (int i = 0; i < attribs.n_planes; ++i) {
+	close_all_bo_handles(drm, handles);
-		close_bo_handle(drm, fb->handles[i]);
-	}
 error_get_dmabuf:
 	free(fb);
 	return NULL;
@@ -358,10 +347,6 @@ void drm_fb_destroy(struct wlr_drm_fb *fb) {
 		wlr_log(WLR_ERROR, "drmModeRmFB failed");
 	}
 
-	for (size_t i = 0; i < sizeof(fb->handles) / sizeof(fb->handles[0]); ++i) {
-		close_bo_handle(drm, fb->handles[i]);
-	}
-
 	free(fb);
 }
 

backend/drm/util.c

@@ -320,3 +320,14 @@ size_t match_obj(size_t num_objs, const uint32_t objs[static restrict num_objs],
 	match_obj_(&st, 0, 0, 0, 0);
 	return st.score;
 }
+
+void close_bo_handle(int drm_fd, uint32_t handle) {
+	if (handle == 0) {
+		return;
+	}
+
+	struct drm_gem_close args = { .handle = handle };
+	if (drmIoctl(drm_fd, DRM_IOCTL_GEM_CLOSE, &args) != 0) {
+		wlr_log_errno(WLR_ERROR, "drmIoctl(GEM_CLOSE) failed");
+	}
+}

include/backend/drm/bo_handle_table.h

@@ -1,24 +0,0 @@
-#ifndef BACKEND_DRM_BO_HANDLE_TABLE_H
-#define BACKEND_DRM_BO_HANDLE_TABLE_H
-
-/**
- * Table performing reference counting for buffer object handles.
- *
- * The BO handles are allocated incrementally and are recycled by the kernel,
- * so a simple array is used.
- *
- * This design is inspired from amdgpu's code in libdrm:
- * https://gitlab.freedesktop.org/mesa/drm/-/blob/1a4c0ec9aea13211997f982715fe5ffcf19dd067/amdgpu/handle_table.c
- */
-struct wlr_drm_bo_handle_table {
-	size_t *nrefs;
-	size_t len;
-};
-
-void drm_bo_handle_table_finish(struct wlr_drm_bo_handle_table *table);
-bool drm_bo_handle_table_ref(struct wlr_drm_bo_handle_table *table,
-	uint32_t handle);
-size_t drm_bo_handle_table_unref(struct wlr_drm_bo_handle_table *table,
-	uint32_t handle);
-
-#endif

include/backend/drm/drm.h

@@ -11,7 +11,6 @@
 #include <wlr/backend/session.h>
 #include <wlr/render/drm_format_set.h>
 #include <xf86drmMode.h>
-#include "backend/drm/bo_handle_table.h"
 #include "backend/drm/iface.h"
 #include "backend/drm/properties.h"
 #include "backend/drm/renderer.h"
@@ -63,7 +62,6 @@ struct wlr_drm_backend {
 	int fd;
 	char *name;
 	struct wlr_device *dev;
-	struct wlr_drm_bo_handle_table bo_handles;
 
 	size_t num_crtcs;
 	struct wlr_drm_crtc *crtcs;

include/backend/drm/renderer.h

@@ -32,7 +32,6 @@ struct wlr_drm_fb {
 	struct wlr_drm_backend *backend;
 	struct wl_list link; // wlr_drm_backend.fbs
 
-	uint32_t handles[WLR_DMABUF_MAX_PLANES];
 	uint32_t id;
 };
 

include/backend/drm/util.h

@@ -36,4 +36,11 @@ size_t match_obj(size_t num_objs, const uint32_t objs[static restrict num_objs],
 		size_t num_res, const uint32_t res[static restrict num_res],
 		uint32_t out[static restrict num_res]);
 
+/**
+ * Close a GEM buffer handle.
+ *
+ * TODO: replace with drmCloseBufferHandle.
+ */
+void close_bo_handle(int drm_fd, uint32_t handle);
+
 #endif