Re: [PATCH] add libsync.h helper

On Mon, Oct 31, 2016 at 1:23 PM, Chris Wilson chris@chris-wilson.co.uk wrote:

On Mon, Oct 31, 2016 at 12:43:09PM -0400, Rob Clark wrote:

...

From: Rob Clark robclark@freedesktop.org

Rather than cut/pasting these couple ioctl wrappers everywhere, just stuff them as static-inline into a header.

This is probably mostly used from mesa, but some drivers, test apps, etc may also want to use it from libdrm.

v2: handle EINTR, add sync_accumulate() based on #dri-devel discussion, etc

Signed-off-by: Rob Clark robclark@freedesktop.org

Makefile.sources | 1 + libsync.h | 145 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 146 insertions(+) create mode 100644 libsync.h

diff --git a/Makefile.sources b/Makefile.sources index a57036a..10aa1d0 100644 --- a/Makefile.sources +++ b/Makefile.sources @@ -13,6 +13,7 @@ LIBDRM_FILES := \ util_math.h

LIBDRM_H_FILES := \

• libsync.h \
  xf86drm.h \
  xf86drmMode.h
  
  

diff --git a/libsync.h b/libsync.h new file mode 100644 index 0000000..2f9aafb --- /dev/null +++ b/libsync.h @@ -0,0 +1,145 @@ +/*

• • sync abstraction
• • Copyright 2015-2016 Collabora Ltd.
• • Based on the implementation from the Android Open Source Project,
• • Copyright 2012 Google, Inc
• • Permission is hereby granted, free of charge, to any person obtaining a
• • copy of this software and associated documentation files (the "Software"),
• • to deal in the Software without restriction, including without limitation
• • the rights to use, copy, modify, merge, publish, distribute, sublicense,
• • and/or sell copies of the Software, and to permit persons to whom the
• • Software is furnished to do so, subject to the following conditions:
• • The above copyright notice and this permission notice shall be included in
• • all copies or substantial portions of the Software.
• • THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
• • IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
• • FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
• • THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
• • OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
• • ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
• • OTHER DEALINGS IN THE SOFTWARE.
• */

+#ifndef _LIBSYNC_H +#define _LIBSYNC_H

+#include <assert.h> +#include <errno.h> +#include <stdint.h> +#include <string.h> +#include <sys/ioctl.h> +#include <sys/poll.h> +#include <unistd.h>

+#if defined(__cplusplus) +extern "C" { +#endif

+/* duplicated from linux/sync_file.h to avoid build-time dependency

• • on new headers
• */

+struct sync_merge_data {

• char    name[32];
  

• int32_t fd2;
  

• int32_t fence;
  

• uint32_t        flags;
  

• uint32_t        pad;
  

+}; +#define SYNC_IOC_MAGIC '>' +#define SYNC_IOC_MERGE _IOWR(SYNC_IOC_MAGIC, 3, struct sync_merge_data)

+static inline int sync_wait(int fd, int timeout) +{

• struct pollfd fds = {0};
  

Would have thought you would have gone with fds = { fd, POLLIN }; :)

I'm a bit shakey on what compilers other than gcc would complain about, so I was actually thinking about going the memset() route to be super-pedantic..

I do prefer designated initializers but I seem to remember c++ being unhappy about that. Not super huge big fan of undesignated (anonymous?) initializers, but I guess it is probably syscall abi and not the sort of thing where struct element order could change.. meh?

...

• int ret;
  

• fds.fd = fd;
  

• fds.events = POLLIN;
  

• do {
  

•         ret = poll(&fds, 1, timeout);
  

•         if (ret > 0) {
  

•                 if (fds.revents & (POLLERR | POLLNVAL)) {
  

•                         errno = EINVAL;
  

•                         return -1;
  

•                 }
  

•                 return 0;
  

•         } else if (ret == 0) {
  

•                 errno = ETIME;
  

•                 return -1;
  

•         }
  

• } while (ret == -1 && (errno == EINTR || errno == EAGAIN));
  

• return ret;
  

+}

+static inline int sync_merge(const char *name, int fd1, int fd2) +{

• struct sync_merge_data data = {0};
  

• int ret;
  

• data.fd2 = fd2;
  

• strncpy(data.name, name, sizeof(data.name));
  

• do {
  

•         ret = ioctl(fd1, SYNC_IOC_MERGE, &data);
  

• } while (ret == -1 && (errno == EINTR || errno == EAGAIN));
  

• if (ret < 0)
  

•         return ret;
  

• return data.fence;
  

+}

+/* accumulate fd2 into fd1. If *fd1 is not a valid fd then dup fd2,

• • otherwise sync_merge() and close the old *fd1. This can be used
• • to implement the pattern:
• • init()
• • {

• •   batch.fence_fd = 1;
    

-1

oh whoops, thx, fixed up locally

BR, -R

...

• • }
• • // does *NOT* take ownership of fd
• • server_sync(int fd)
• • {

• •   if (sync_accumulate("foo", &batch.fence_fd, fd)) {
    

• •      ... error ...
    

• •   }
    

• • }
• */

+static inline int sync_accumulate(const char *name, int *fd1, int fd2) +{

• int ret;
  

• assert(fd2 >= 0);
  

• if (*fd1 < 0) {
  

•         *fd1 = dup(fd2);
  

•         return 0;
  

• }
  

• ret = sync_merge(name, *fd1, fd2);
  

• if (ret < 0) {
  

•         /* leave *fd1 as it is */
  

•         return ret;
  

• }
  

• close(*fd1);
  

• *fd1 = ret;
  

• return 0;
  

+}

Lgtm, matches how we want to use it. Reviewed-by: Chris Wilson chris@chris-wilson.co.uk -Chris

-- Chris Wilson, Intel Open Source Technology Centre