From 986a2d9f66db5df6661b57290ec31927c6dadcf8 Mon Sep 17 00:00:00 2001 From: Simon Ser Date: Wed, 4 Oct 2023 12:40:34 +0200 Subject: [PATCH] render: replace wlr_renderer_get_drm_fd() with drm_dev_id wlr_renderer_get_drm_fd() forces the renderer to open the DRM node and keep the FD around for no good reason. Instead, use dev_t to refer to a DRM device. This also removes footguns related to FD ownership. --- include/render/egl.h | 5 ++- include/render/gles2.h | 2 +- include/render/vulkan.h | 5 +-- include/wlr/render/interface.h | 1 - include/wlr/render/wlr_renderer.h | 9 +--- render/allocator/allocator.c | 35 +++++++++++++-- render/egl.c | 74 +++++++++++++------------------ render/gles2/renderer.c | 22 ++------- render/vulkan/renderer.c | 19 ++------ render/vulkan/vulkan.c | 38 +++------------- render/wlr_renderer.c | 9 +--- types/wlr_drm.c | 9 ++-- types/wlr_linux_dmabuf_v1.c | 9 +--- 13 files changed, 91 insertions(+), 146 deletions(-) diff --git a/include/render/egl.h b/include/render/egl.h index 0765cce74..6a9b81dbf 100644 --- a/include/render/egl.h +++ b/include/render/egl.h @@ -88,7 +88,10 @@ const struct wlr_drm_format_set *wlr_egl_get_dmabuf_render_formats( */ bool wlr_egl_destroy_image(struct wlr_egl *egl, EGLImageKHR image); -int wlr_egl_dup_drm_fd(struct wlr_egl *egl); +/** + * Get the DRM device ID used by EGL. + */ +bool wlr_egl_get_drm_dev_id(struct wlr_egl *egl, dev_t *devid); /** * Restore EGL context that was previously saved using wlr_egl_save_current(). diff --git a/include/render/gles2.h b/include/render/gles2.h index a472ee9c6..b39128591 100644 --- a/include/render/gles2.h +++ b/include/render/gles2.h @@ -41,8 +41,8 @@ struct wlr_gles2_tex_shader { struct wlr_gles2_renderer { struct wlr_renderer wlr_renderer; + dev_t drm_dev_id; struct wlr_egl *egl; - int drm_fd; struct wlr_drm_format_set shm_texture_formats; diff --git a/include/render/vulkan.h b/include/render/vulkan.h index 3f703b5c4..f7d44e7cd 100644 --- a/include/render/vulkan.h +++ b/include/render/vulkan.h @@ -38,8 +38,6 @@ struct wlr_vk_device { VkPhysicalDevice phdev; VkDevice dev; - int drm_fd; - bool implicit_sync_interop; bool sampler_ycbcr_conversion; @@ -66,7 +64,7 @@ struct wlr_vk_device { // Tries to find the VkPhysicalDevice for the given drm fd. // Might find none and return VK_NULL_HANDLE. VkPhysicalDevice vulkan_find_drm_phdev(struct wlr_vk_instance *ini, int drm_fd); -int vulkan_open_phdev_drm_fd(VkPhysicalDevice phdev); +bool vulkan_get_phdev_drm_dev_id(VkPhysicalDevice phdev, dev_t *dev_id); // Creates a device for the given instance and physical device. struct wlr_vk_device *vulkan_device_create(struct wlr_vk_instance *ini, @@ -231,6 +229,7 @@ struct wlr_vk_renderer { struct wlr_renderer wlr_renderer; struct wlr_backend *backend; struct wlr_vk_device *dev; + dev_t drm_dev_id; VkCommandPool command_pool; diff --git a/include/wlr/render/interface.h b/include/wlr/render/interface.h index 89f6de970..d46caa80f 100644 --- a/include/wlr/render/interface.h +++ b/include/wlr/render/interface.h @@ -25,7 +25,6 @@ struct wlr_renderer_impl { const struct wlr_drm_format_set *(*get_render_formats)( struct wlr_renderer *renderer); void (*destroy)(struct wlr_renderer *renderer); - int (*get_drm_fd)(struct wlr_renderer *renderer); struct wlr_texture *(*texture_from_buffer)(struct wlr_renderer *renderer, struct wlr_buffer *buffer); struct wlr_render_pass *(*begin_buffer_pass)(struct wlr_renderer *renderer, diff --git a/include/wlr/render/wlr_renderer.h b/include/wlr/render/wlr_renderer.h index 62da07610..2b35f9617 100644 --- a/include/wlr/render/wlr_renderer.h +++ b/include/wlr/render/wlr_renderer.h @@ -29,6 +29,8 @@ struct wlr_renderer { // Capabilities required for the buffer used as a render target (bitmask of // enum wlr_buffer_cap) uint32_t render_buffer_caps; + // DRM device used for rendering, if any + const dev_t *drm_dev_id; struct { struct wl_signal destroy; @@ -78,13 +80,6 @@ bool wlr_renderer_init_wl_display(struct wlr_renderer *r, bool wlr_renderer_init_wl_shm(struct wlr_renderer *r, struct wl_display *wl_display); -/** - * Obtains the FD of the DRM device used for rendering, or -1 if unavailable. - * - * The caller doesn't have ownership of the FD, it must not close it. - */ -int wlr_renderer_get_drm_fd(struct wlr_renderer *r); - /** * Destroys the renderer. * diff --git a/render/allocator/allocator.c b/render/allocator/allocator.c index 27b08fc82..ff8aefda2 100644 --- a/render/allocator/allocator.c +++ b/render/allocator/allocator.c @@ -150,11 +150,40 @@ struct wlr_allocator *wlr_allocator_autocreate(struct wlr_backend *backend, uint32_t backend_caps = backend_get_buffer_caps(backend); // Note, drm_fd may be negative if unavailable int drm_fd = wlr_backend_get_drm_fd(backend); - if (drm_fd < 0) { - drm_fd = wlr_renderer_get_drm_fd(renderer); + + int render_drm_fd = -1; + if (drm_fd < 0 && renderer->drm_dev_id != NULL) { + drmDevice *dev = NULL; + if (drmGetDeviceFromDevId(*renderer->drm_dev_id, 0, &dev) != 0) { + wlr_log(WLR_ERROR, "drmGetDeviceFromDevId failed"); + return NULL; + } + + const char *node_name = NULL; + if (dev->available_nodes & (1 << DRM_NODE_RENDER)) { + node_name = dev->nodes[DRM_NODE_RENDER]; + } else { + assert(dev->available_nodes & (1 << DRM_NODE_PRIMARY)); + wlr_log(WLR_DEBUG, "No DRM render node available, " + "falling back to primary node '%s'", dev->nodes[DRM_NODE_PRIMARY]); + node_name = dev->nodes[DRM_NODE_PRIMARY]; + } + render_drm_fd = open(node_name, O_RDWR | O_NONBLOCK | O_CLOEXEC); + drmFreeDevice(&dev); + if (render_drm_fd < 0) { + wlr_log_errno(WLR_ERROR, "Failed to open DRM node %s", node_name); + return NULL; + } + + drm_fd = render_drm_fd; } - return allocator_autocreate_with_drm_fd(backend_caps, renderer, drm_fd); + struct wlr_allocator *alloc = + allocator_autocreate_with_drm_fd(backend_caps, renderer, drm_fd); + if (render_drm_fd >= 0) { + close(render_drm_fd); + } + return alloc; } void wlr_allocator_destroy(struct wlr_allocator *alloc) { diff --git a/render/egl.c b/render/egl.c index 19868ca84..9e93b5837 100644 --- a/render/egl.c +++ b/render/egl.c @@ -5,6 +5,7 @@ #include #include #include +#include #include #include #include @@ -949,68 +950,53 @@ static char *get_render_name(const char *name) { return render_name; } -static int dup_egl_device_drm_fd(struct wlr_egl *egl) { +static char *get_drm_node_path(struct wlr_egl *egl) { if (egl->device == EGL_NO_DEVICE_EXT || (!egl->exts.EXT_device_drm && !egl->exts.EXT_device_drm_render_node)) { - return -1; + return NULL; } - char *render_name = NULL; #ifdef EGL_EXT_device_drm_render_node if (egl->exts.EXT_device_drm_render_node) { const char *name = egl->procs.eglQueryDeviceStringEXT(egl->device, EGL_DRM_RENDER_NODE_FILE_EXT); if (name == NULL) { wlr_log(WLR_DEBUG, "EGL device has no render node"); - return -1; + return false; } - render_name = strdup(name); + return strdup(name); } #endif - if (render_name == NULL) { - const char *primary_name = egl->procs.eglQueryDeviceStringEXT(egl->device, - EGL_DRM_DEVICE_FILE_EXT); - if (primary_name == NULL) { - wlr_log(WLR_ERROR, - "eglQueryDeviceStringEXT(EGL_DRM_DEVICE_FILE_EXT) failed"); - return -1; - } - - render_name = get_render_name(primary_name); - if (render_name == NULL) { - wlr_log(WLR_ERROR, "Can't find render node name for device %s", - primary_name); - return -1; - } + const char *primary_name = egl->procs.eglQueryDeviceStringEXT(egl->device, + EGL_DRM_DEVICE_FILE_EXT); + if (primary_name == NULL) { + wlr_log(WLR_ERROR, + "eglQueryDeviceStringEXT(EGL_DRM_DEVICE_FILE_EXT) failed"); + return false; } - int render_fd = open(render_name, O_RDWR | O_NONBLOCK | O_CLOEXEC); - if (render_fd < 0) { - wlr_log_errno(WLR_ERROR, "Failed to open DRM render node %s", - render_name); + return get_render_name(primary_name); +} + +bool wlr_egl_get_drm_dev_id(struct wlr_egl *egl, dev_t *dev_id) { + char *render_name = get_drm_node_path(egl); + // Fallback to GBM's FD if we can't use EGLDevice + if (render_name == NULL && egl->gbm_device != NULL) { + render_name = drmGetDeviceNameFromFd2(gbm_device_get_fd(egl->gbm_device)); + } + if (render_name == NULL) { + return false; + } + + struct stat render_stat = {0}; + if (stat(render_name, &render_stat) != 0) { + wlr_log_errno(WLR_ERROR, "stat failed"); free(render_name); - return -1; + return false; } free(render_name); - return render_fd; -} - -int wlr_egl_dup_drm_fd(struct wlr_egl *egl) { - int fd = dup_egl_device_drm_fd(egl); - if (fd >= 0) { - return fd; - } - - // Fallback to GBM's FD if we can't use EGLDevice - if (egl->gbm_device == NULL) { - return -1; - } - - fd = fcntl(gbm_device_get_fd(egl->gbm_device), F_DUPFD_CLOEXEC, 0); - if (fd < 0) { - wlr_log_errno(WLR_ERROR, "Failed to dup GBM FD"); - } - return fd; + *dev_id = render_stat.st_rdev; + return true; } diff --git a/render/gles2/renderer.c b/render/gles2/renderer.c index 7bb9cf346..e3b83f625 100644 --- a/render/gles2/renderer.c +++ b/render/gles2/renderer.c @@ -183,17 +183,6 @@ static const struct wlr_drm_format_set *gles2_get_render_formats( return wlr_egl_get_dmabuf_render_formats(renderer->egl); } -static int gles2_get_drm_fd(struct wlr_renderer *wlr_renderer) { - struct wlr_gles2_renderer *renderer = - gles2_get_renderer(wlr_renderer); - - if (renderer->drm_fd < 0) { - renderer->drm_fd = wlr_egl_dup_drm_fd(renderer->egl); - } - - return renderer->drm_fd; -} - struct wlr_egl *wlr_gles2_renderer_get_egl(struct wlr_renderer *wlr_renderer) { struct wlr_gles2_renderer *renderer = gles2_get_renderer(wlr_renderer); @@ -231,11 +220,6 @@ static void gles2_destroy(struct wlr_renderer *wlr_renderer) { wlr_egl_destroy(renderer->egl); wlr_drm_format_set_finish(&renderer->shm_texture_formats); - - if (renderer->drm_fd >= 0) { - close(renderer->drm_fd); - } - free(renderer); } @@ -356,7 +340,6 @@ static const struct wlr_renderer_impl renderer_impl = { .destroy = gles2_destroy, .get_texture_formats = gles2_get_texture_formats, .get_render_formats = gles2_get_render_formats, - .get_drm_fd = gles2_get_drm_fd, .texture_from_buffer = gles2_texture_from_buffer, .begin_buffer_pass = gles2_begin_buffer_pass, .render_timer_create = gles2_render_timer_create, @@ -532,7 +515,6 @@ struct wlr_renderer *wlr_gles2_renderer_create(struct wlr_egl *egl) { renderer->egl = egl; renderer->exts_str = exts_str; - renderer->drm_fd = -1; wlr_log(WLR_INFO, "Creating GLES2 renderer"); wlr_log(WLR_INFO, "Using %s", glGetString(GL_VERSION)); @@ -629,6 +611,10 @@ struct wlr_renderer *wlr_gles2_renderer_create(struct wlr_egl *egl) { GL_DEBUG_TYPE_PUSH_GROUP_KHR, GL_DONT_CARE, 0, NULL, GL_FALSE); } + if (wlr_egl_get_drm_dev_id(renderer->egl, &renderer->drm_dev_id)) { + renderer->wlr_renderer.drm_dev_id = &renderer->drm_dev_id; + } + push_gles2_debug(renderer); GLuint prog; diff --git a/render/vulkan/renderer.c b/render/vulkan/renderer.c index ffb685f62..79babdf9d 100644 --- a/render/vulkan/renderer.c +++ b/render/vulkan/renderer.c @@ -1294,11 +1294,6 @@ destroy_image: return false; } -static int vulkan_get_drm_fd(struct wlr_renderer *wlr_renderer) { - struct wlr_vk_renderer *renderer = vulkan_get_renderer(wlr_renderer); - return renderer->dev->drm_fd; -} - static struct wlr_render_pass *vulkan_begin_buffer_pass(struct wlr_renderer *wlr_renderer, struct wlr_buffer *buffer, const struct wlr_buffer_pass_options *options) { struct wlr_vk_renderer *renderer = vulkan_get_renderer(wlr_renderer); @@ -1322,7 +1317,6 @@ static const struct wlr_renderer_impl renderer_impl = { .get_texture_formats = vulkan_get_texture_formats, .get_render_formats = vulkan_get_render_formats, .destroy = vulkan_destroy, - .get_drm_fd = vulkan_get_drm_fd, .texture_from_buffer = vulkan_texture_from_buffer, .begin_buffer_pass = vulkan_begin_buffer_pass, }; @@ -2156,6 +2150,10 @@ struct wlr_renderer *vulkan_renderer_create_for_device(struct wlr_vk_device *dev wl_list_init(&renderer->render_buffers); wl_list_init(&renderer->pipeline_layouts); + if (vulkan_get_phdev_drm_dev_id(dev->phdev, &renderer->drm_dev_id)) { + renderer->wlr_renderer.drm_dev_id = &renderer->drm_dev_id; + } + if (!init_static_render_data(renderer)) { goto error; } @@ -2221,15 +2219,6 @@ struct wlr_renderer *wlr_vk_renderer_create_with_drm_fd(int drm_fd) { return NULL; } - // Do not use the drm_fd that was passed in: we should prefer the render - // node even if a primary node was provided - dev->drm_fd = vulkan_open_phdev_drm_fd(phdev); - if (dev->drm_fd < 0) { - vulkan_device_destroy(dev); - vulkan_instance_destroy(ini); - return NULL; - } - return vulkan_renderer_create_for_device(dev); } diff --git a/render/vulkan/vulkan.c b/render/vulkan/vulkan.c index 7cdc44a02..1aad40853 100644 --- a/render/vulkan/vulkan.c +++ b/render/vulkan/vulkan.c @@ -2,7 +2,6 @@ #undef _POSIX_C_SOURCE #endif #include -#include #include #include #include @@ -364,7 +363,7 @@ VkPhysicalDevice vulkan_find_drm_phdev(struct wlr_vk_instance *ini, int drm_fd) return VK_NULL_HANDLE; } -int vulkan_open_phdev_drm_fd(VkPhysicalDevice phdev) { +bool vulkan_get_phdev_drm_dev_id(VkPhysicalDevice phdev, dev_t *dev_id) { // vulkan_find_drm_phdev() already checks that VK_EXT_physical_device_drm // is supported VkPhysicalDeviceDrmPropertiesEXT drm_props = { @@ -376,38 +375,16 @@ int vulkan_open_phdev_drm_fd(VkPhysicalDevice phdev) { }; vkGetPhysicalDeviceProperties2(phdev, &props); - dev_t devid; if (drm_props.hasRender) { - devid = makedev(drm_props.renderMajor, drm_props.renderMinor); + *dev_id = makedev(drm_props.renderMajor, drm_props.renderMinor); } else if (drm_props.hasPrimary) { - devid = makedev(drm_props.primaryMajor, drm_props.primaryMinor); + *dev_id = makedev(drm_props.primaryMajor, drm_props.primaryMinor); } else { wlr_log(WLR_ERROR, "Physical device is missing both render and primary nodes"); - return -1; + return false; } - drmDevice *device = NULL; - if (drmGetDeviceFromDevId(devid, 0, &device) != 0) { - wlr_log_errno(WLR_ERROR, "drmGetDeviceFromDevId failed"); - return -1; - } - - const char *name = NULL; - if (device->available_nodes & (1 << DRM_NODE_RENDER)) { - name = device->nodes[DRM_NODE_RENDER]; - } else { - assert(device->available_nodes & (1 << DRM_NODE_PRIMARY)); - name = device->nodes[DRM_NODE_PRIMARY]; - wlr_log(WLR_DEBUG, "DRM device %s has no render node, " - "falling back to primary node", name); - } - - int drm_fd = open(name, O_RDWR | O_NONBLOCK | O_CLOEXEC); - if (drm_fd < 0) { - wlr_log_errno(WLR_ERROR, "Failed to open DRM node %s", name); - } - drmFreeDevice(&device); - return drm_fd; + return true; } static void load_device_proc(struct wlr_vk_device *dev, const char *name, @@ -452,7 +429,6 @@ struct wlr_vk_device *vulkan_device_create(struct wlr_vk_instance *ini, dev->phdev = phdev; dev->instance = ini; - dev->drm_fd = -1; // For dmabuf import we require at least the external_memory_fd, // external_memory_dma_buf, queue_family_foreign, @@ -650,10 +626,6 @@ void vulkan_device_destroy(struct wlr_vk_device *dev) { vkDestroyDevice(dev->dev, NULL); } - if (dev->drm_fd > 0) { - close(dev->drm_fd); - } - wlr_drm_format_set_finish(&dev->dmabuf_render_formats); wlr_drm_format_set_finish(&dev->dmabuf_texture_formats); wlr_drm_format_set_finish(&dev->shm_texture_formats); diff --git a/render/wlr_renderer.c b/render/wlr_renderer.c index 590372ebe..62a1b326c 100644 --- a/render/wlr_renderer.c +++ b/render/wlr_renderer.c @@ -83,7 +83,7 @@ bool wlr_renderer_init_wl_display(struct wlr_renderer *r, } if (wlr_renderer_get_texture_formats(r, WLR_BUFFER_CAP_DMABUF) != NULL && - wlr_renderer_get_drm_fd(r) >= 0 && + r->drm_dev_id != NULL && wlr_linux_dmabuf_v1_create_with_renderer(wl_display, 4, r) == NULL) { return false; } @@ -287,13 +287,6 @@ struct wlr_renderer *wlr_renderer_autocreate(struct wlr_backend *backend) { return renderer_autocreate(backend, -1); } -int wlr_renderer_get_drm_fd(struct wlr_renderer *r) { - if (!r->impl->get_drm_fd) { - return -1; - } - return r->impl->get_drm_fd(r); -} - struct wlr_render_pass *wlr_renderer_begin_buffer_pass(struct wlr_renderer *renderer, struct wlr_buffer *buffer, const struct wlr_buffer_pass_options *options) { struct wlr_buffer_pass_options default_options = {0}; diff --git a/types/wlr_drm.c b/types/wlr_drm.c index c58cdb4ec..290b9b20f 100644 --- a/types/wlr_drm.c +++ b/types/wlr_drm.c @@ -201,15 +201,14 @@ static void handle_display_destroy(struct wl_listener *listener, void *data) { struct wlr_drm *wlr_drm_create(struct wl_display *display, struct wlr_renderer *renderer) { - int drm_fd = wlr_renderer_get_drm_fd(renderer); - if (drm_fd < 0) { - wlr_log(WLR_ERROR, "Failed to get DRM FD from renderer"); + if (renderer->drm_dev_id == NULL) { + wlr_log(WLR_ERROR, "Failed to get DRM device from renderer"); return NULL; } drmDevice *dev = NULL; - if (drmGetDevice2(drm_fd, 0, &dev) != 0) { - wlr_log(WLR_ERROR, "drmGetDevice2 failed"); + if (drmGetDeviceFromDevId(*renderer->drm_dev_id, 0, &dev) != 0) { + wlr_log(WLR_ERROR, "drmGetDeviceFromDevId failed"); return NULL; } diff --git a/types/wlr_linux_dmabuf_v1.c b/types/wlr_linux_dmabuf_v1.c index bfd97637a..007a896fe 100644 --- a/types/wlr_linux_dmabuf_v1.c +++ b/types/wlr_linux_dmabuf_v1.c @@ -1087,16 +1087,11 @@ bool wlr_linux_dmabuf_feedback_v1_init_with_options(struct wlr_linux_dmabuf_feed *feedback = (struct wlr_linux_dmabuf_feedback_v1){0}; - int renderer_drm_fd = wlr_renderer_get_drm_fd(options->main_renderer); - if (renderer_drm_fd < 0) { - wlr_log(WLR_ERROR, "Failed to get renderer DRM FD"); - goto error; - } - dev_t renderer_dev; - if (!devid_from_fd(renderer_drm_fd, &renderer_dev)) { + if (options->main_renderer->drm_dev_id == NULL) { goto error; } + dev_t renderer_dev = *options->main_renderer->drm_dev_id; feedback->main_device = renderer_dev; const struct wlr_drm_format_set *renderer_formats =