[Mesa-dev] [PATCH 2/2] pipe-loader: move dup(fd) within pipe_loader_drm_probe_fd
Emil Velikov
emil.l.velikov at gmail.com
Wed Aug 29 17:13:14 UTC 2018
From: Emil Velikov <emil.velikov at collabora.com>
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 <leo.liu at amd.com>
Cc: Thomas Hellstrom <thellstrom at vmware.com>
Cc: Axel Davy <davyaxel0 at gmail.com>
Cc: Patrick Rudolph <siro at das-labor.org>
Signed-off-by: Emil Velikov <emil.velikov at collabora.com>
---
I'm 99% sure that the VAAPI/XA notes are correct, but I left it as CHECK
since it's been a while since I useed those ;-)
Leo, Thomas, can you please confirm the respective CHECK notes?
Axel, Patrick, I've split out the double-close in a prep patch for stable.
An ack/test would be greatly appreciated.
Thanks
---
.../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 <string.h>
#include <xf86drm.h>
#include <unistd.h>
+#include <fcntl.h>
#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 bb1ff504886..ebbaa30b6ec 100644
--- a/src/gallium/auxiliary/vl/vl_winsys_dri.c
+++ b/src/gallium/auxiliary/vl/vl_winsys_dri.c
@@ -29,7 +29,6 @@
#include <sys/types.h>
#include <sys/stat.h>
-#include <fcntl.h>
#include <X11/Xlib-xcb.h>
#include <X11/extensions/dri2tokens.h>
@@ -421,6 +420,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);
@@ -429,15 +430,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:
@@ -465,5 +463,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 <assert.h>
-#include <fcntl.h>
#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 b21e6815796..08af6b71550 100644
--- a/src/gallium/state_trackers/dri/dri2.c
+++ b/src/gallium/state_trackers/dri/dri2.c
@@ -29,7 +29,6 @@
*/
#include <xf86drm.h>
-#include <fcntl.h>
#include "GL/mesa_glinterop.h"
#include "util/u_memory.h"
#include "util/u_inlines.h"
@@ -2097,7 +2096,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)
@@ -2109,11 +2107,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);
@@ -2174,10 +2168,7 @@ destroy_screen:
release_pipe:
if (screen->dev)
pipe_loader_release(&screen->dev, 1);
- else
- close(fd);
-free_screen:
FREE(screen);
return NULL;
}
@@ -2206,10 +2197,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);
}
@@ -2249,10 +2237,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 935f9e55923..8abfd3c7af4 100644
--- a/src/gallium/state_trackers/dri/dri_screen.c
+++ b/src/gallium/state_trackers/dri/dri_screen.c
@@ -485,6 +485,7 @@ dri_destroy_screen(__DRIscreen * sPriv)
if (screen->dev)
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 <unistd.h>
-#include <fcntl.h>
#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.18.0
More information about the mesa-dev
mailing list