From 6ccc435e7ad92bb0ba77d3fdb48c7127ba71239e Mon Sep 17 00:00:00 2001 From: Emil Velikov Date: Wed, 29 Aug 2018 18:13:14 +0100 Subject: [PATCH] pipe-loader: move dup(fd) within pipe_loader_drm_probe_fd Currently pipe_loader_drm_probe_fd takes ownership of the fd given. To match that, pipe_loader_release closes it. Yet we have many instances which do not want the change of ownership, and thus duplicate the fd before passing it to the pipe-loader. Move the dup() within pipe-loader, explicitly document that and document all the cases through the codebase. A trivial git grep -2 pipe_loader_release makes things as obvious as it gets ;-) Cc: Leo Liu Cc: Thomas Hellstrom Cc: Axel Davy Cc: Patrick Rudolph Signed-off-by: Emil Velikov Reviewed-by: Thomas Hellstrom Reviewed-by: Axel Davy (for nine) --- .../auxiliary/pipe-loader/pipe_loader.h | 3 +++ .../auxiliary/pipe-loader/pipe_loader_drm.c | 23 ++++++++++++++++--- src/gallium/auxiliary/vl/vl_winsys_dri.c | 11 ++++----- src/gallium/auxiliary/vl/vl_winsys_drm.c | 10 ++------ src/gallium/state_trackers/dri/dri2.c | 19 ++------------- src/gallium/state_trackers/dri/dri_screen.c | 1 + src/gallium/state_trackers/xa/xa_tracker.c | 12 +++------- src/gallium/targets/d3dadapter9/drm.c | 2 +- 8 files changed, 37 insertions(+), 44 deletions(-) diff --git a/src/gallium/auxiliary/pipe-loader/pipe_loader.h b/src/gallium/auxiliary/pipe-loader/pipe_loader.h index b50114310b4..be7e25afb02 100644 --- a/src/gallium/auxiliary/pipe-loader/pipe_loader.h +++ b/src/gallium/auxiliary/pipe-loader/pipe_loader.h @@ -146,6 +146,9 @@ pipe_loader_sw_probe_dri(struct pipe_loader_device **devs, * * This function is platform-specific. * + * Function does not take ownership of the fd, but duplicates it locally. + * The local fd is closed during pipe_loader_release. + * * \sa pipe_loader_probe */ bool diff --git a/src/gallium/auxiliary/pipe-loader/pipe_loader_drm.c b/src/gallium/auxiliary/pipe-loader/pipe_loader_drm.c index 6d2ed6e76f8..5a88a2ac2f0 100644 --- a/src/gallium/auxiliary/pipe-loader/pipe_loader_drm.c +++ b/src/gallium/auxiliary/pipe-loader/pipe_loader_drm.c @@ -35,6 +35,7 @@ #include #include #include +#include #include "loader.h" #include "target-helpers/drm_helper_public.h" @@ -168,8 +169,8 @@ get_driver_descriptor(const char *driver_name, struct util_dl_library **plib) return NULL; } -bool -pipe_loader_drm_probe_fd(struct pipe_loader_device **dev, int fd) +static bool +pipe_loader_drm_probe_fd_nodup(struct pipe_loader_device **dev, int fd) { struct pipe_loader_drm_device *ddev = CALLOC_STRUCT(pipe_loader_drm_device); int vendor_id, chip_id; @@ -212,6 +213,22 @@ pipe_loader_drm_probe_fd(struct pipe_loader_device **dev, int fd) return false; } +bool +pipe_loader_drm_probe_fd(struct pipe_loader_device **dev, int fd) +{ + bool ret; + int new_fd; + + if (fd < 0 || (new_fd = fcntl(fd, F_DUPFD_CLOEXEC, 3)) < 0) + return false; + + ret = pipe_loader_drm_probe_fd_nodup(dev, new_fd); + if (!ret) + close(new_fd); + + return ret; +} + static int open_drm_render_node_minor(int minor) { @@ -234,7 +251,7 @@ pipe_loader_drm_probe(struct pipe_loader_device **devs, int ndev) if (fd < 0) continue; - if (!pipe_loader_drm_probe_fd(&dev, fd)) { + if (!pipe_loader_drm_probe_fd_nodup(&dev, fd)) { close(fd); continue; } diff --git a/src/gallium/auxiliary/vl/vl_winsys_dri.c b/src/gallium/auxiliary/vl/vl_winsys_dri.c index cb779010a9c..137885d9475 100644 --- a/src/gallium/auxiliary/vl/vl_winsys_dri.c +++ b/src/gallium/auxiliary/vl/vl_winsys_dri.c @@ -29,7 +29,6 @@ #include #include -#include #include #include @@ -471,6 +470,8 @@ vl_dri2_screen_create(Display *display, int screen) vl_compositor_reset_dirty_area(&scrn->dirty_areas[0]); vl_compositor_reset_dirty_area(&scrn->dirty_areas[1]); + /* The pipe loader duplicates the fd */ + close(fd); free(authenticate); free(connect); free(dri2_query); @@ -479,15 +480,12 @@ vl_dri2_screen_create(Display *display, int screen) return &scrn->base; release_pipe: - if (scrn->base.dev) { + if (scrn->base.dev) pipe_loader_release(&scrn->base.dev, 1); - fd = -1; - } free_authenticate: free(authenticate); close_fd: - if (fd != -1) - close(fd); + close(fd); free_connect: free(connect); free_query: @@ -515,5 +513,6 @@ vl_dri2_screen_destroy(struct vl_screen *vscreen) vl_dri2_destroy_drawable(scrn); scrn->base.pscreen->destroy(scrn->base.pscreen); pipe_loader_release(&scrn->base.dev, 1); + /* There is no user provided fd */ FREE(scrn); } diff --git a/src/gallium/auxiliary/vl/vl_winsys_drm.c b/src/gallium/auxiliary/vl/vl_winsys_drm.c index df8809c501c..94eb6d74ee7 100644 --- a/src/gallium/auxiliary/vl/vl_winsys_drm.c +++ b/src/gallium/auxiliary/vl/vl_winsys_drm.c @@ -26,7 +26,6 @@ **************************************************************************/ #include -#include #include "pipe/p_screen.h" #include "pipe-loader/pipe_loader.h" @@ -48,10 +47,7 @@ vl_drm_screen_create(int fd) if (!vscreen) return NULL; - if (fd < 0 || (new_fd = fcntl(fd, F_DUPFD_CLOEXEC, 3)) < 0) - goto free_screen; - - if (pipe_loader_drm_probe_fd(&vscreen->dev, new_fd)) + if (pipe_loader_drm_probe_fd(&vscreen->dev, fd)) vscreen->pscreen = pipe_loader_create_screen(vscreen->dev); if (!vscreen->pscreen) @@ -68,10 +64,7 @@ vl_drm_screen_create(int fd) release_pipe: if (vscreen->dev) pipe_loader_release(&vscreen->dev, 1); - else - close(new_fd); -free_screen: FREE(vscreen); return NULL; } @@ -83,5 +76,6 @@ vl_drm_screen_destroy(struct vl_screen *vscreen) vscreen->pscreen->destroy(vscreen->pscreen); pipe_loader_release(&vscreen->dev, 1); + /* CHECK: The VAAPI loader/user preserves ownership of the original fd */ FREE(vscreen); } diff --git a/src/gallium/state_trackers/dri/dri2.c b/src/gallium/state_trackers/dri/dri2.c index 20540e39881..b17c5e16ede 100644 --- a/src/gallium/state_trackers/dri/dri2.c +++ b/src/gallium/state_trackers/dri/dri2.c @@ -29,7 +29,6 @@ */ #include -#include #include "GL/mesa_glinterop.h" #include "util/u_memory.h" #include "util/u_inlines.h" @@ -2103,7 +2102,6 @@ dri2_init_screen(__DRIscreen * sPriv) struct pipe_screen *pscreen = NULL; const struct drm_conf_ret *throttle_ret; const struct drm_conf_ret *dmabuf_ret; - int fd; screen = CALLOC_STRUCT(dri_screen); if (!screen) @@ -2115,11 +2113,7 @@ dri2_init_screen(__DRIscreen * sPriv) sPriv->driverPrivate = (void *)screen; - if (screen->fd < 0 || (fd = fcntl(screen->fd, F_DUPFD_CLOEXEC, 3)) < 0) - goto free_screen; - - - if (pipe_loader_drm_probe_fd(&screen->dev, fd)) { + if (pipe_loader_drm_probe_fd(&screen->dev, screen->fd)) { dri_init_options(screen); pscreen = pipe_loader_create_screen(screen->dev); @@ -2180,10 +2174,7 @@ destroy_screen: release_pipe: if (screen->dev) pipe_loader_release(&screen->dev, 1); - else - close(fd); -free_screen: FREE(screen); return NULL; } @@ -2212,10 +2203,7 @@ dri_kms_init_screen(__DRIscreen * sPriv) sPriv->driverPrivate = (void *)screen; - if (screen->fd < 0 || (fd = fcntl(screen->fd, F_DUPFD_CLOEXEC, 3)) < 0) - goto free_screen; - - if (pipe_loader_sw_probe_kms(&screen->dev, fd)) { + if (pipe_loader_sw_probe_kms(&screen->dev, screen->fd)) { dri_init_options(screen); pscreen = pipe_loader_create_screen(screen->dev); } @@ -2257,10 +2245,7 @@ destroy_screen: release_pipe: if (screen->dev) pipe_loader_release(&screen->dev, 1); - else - close(fd); -free_screen: FREE(screen); #endif // GALLIUM_SOFTPIPE return NULL; diff --git a/src/gallium/state_trackers/dri/dri_screen.c b/src/gallium/state_trackers/dri/dri_screen.c index 308e23685e8..82a0988a634 100644 --- a/src/gallium/state_trackers/dri/dri_screen.c +++ b/src/gallium/state_trackers/dri/dri_screen.c @@ -488,6 +488,7 @@ dri_destroy_screen(__DRIscreen * sPriv) pipe_loader_release(&screen->dev, 1); + /* The caller in dri_util preserves the fd ownership */ free(screen); sPriv->driverPrivate = NULL; sPriv->extensions = NULL; diff --git a/src/gallium/state_trackers/xa/xa_tracker.c b/src/gallium/state_trackers/xa/xa_tracker.c index c046a3a7097..d07cd146af9 100644 --- a/src/gallium/state_trackers/xa/xa_tracker.c +++ b/src/gallium/state_trackers/xa/xa_tracker.c @@ -27,7 +27,6 @@ */ #include -#include #include "xa_tracker.h" #include "xa_priv.h" #include "pipe/p_state.h" @@ -153,15 +152,11 @@ xa_tracker_create(int drm_fd) struct xa_tracker *xa = calloc(1, sizeof(struct xa_tracker)); enum xa_surface_type stype; unsigned int num_formats; - int fd; if (!xa) return NULL; - if (drm_fd < 0 || (fd = fcntl(drm_fd, F_DUPFD_CLOEXEC, 3)) < 0) - goto out_no_fd; - - if (pipe_loader_drm_probe_fd(&xa->dev, fd)) + if (pipe_loader_drm_probe_fd(&xa->dev, drm_fd)) xa->screen = pipe_loader_create_screen(xa->dev); if (!xa->screen) @@ -213,9 +208,7 @@ xa_tracker_create(int drm_fd) out_no_screen: if (xa->dev) pipe_loader_release(&xa->dev, 1); - else - close(fd); - out_no_fd: + free(xa); return NULL; } @@ -227,6 +220,7 @@ xa_tracker_destroy(struct xa_tracker *xa) xa_context_destroy(xa->default_ctx); xa->screen->destroy(xa->screen); pipe_loader_release(&xa->dev, 1); + /* CHECK: The XA API user preserves ownership of the original fd */ free(xa); } diff --git a/src/gallium/targets/d3dadapter9/drm.c b/src/gallium/targets/d3dadapter9/drm.c index a2a36dbbda9..85b3e10633e 100644 --- a/src/gallium/targets/d3dadapter9/drm.c +++ b/src/gallium/targets/d3dadapter9/drm.c @@ -107,7 +107,7 @@ drm_destroy( struct d3dadapter9_context *ctx ) if (drm->dev) pipe_loader_release(&drm->dev, 1); - /* The pipe loader takes ownership of the fd */ + close(drm->fd); FREE(ctx); } -- 2.30.2