[PATCH libdrm] add libsync.h helper

[Rob Clark]

On Mon, Oct 31, 2016 at 9:55 AM, Chris Wilson <chris at chris-wilson.co.uk> wrote:
> On Mon, Oct 31, 2016 at 09:44:07AM -0400, Rob Clark wrote:
>> From: Rob Clark <robclark at freedesktop.org>
>>
>> Rather than cut/pasting these couple ioctl wrappers everywhere, just
>> stuff them as static-inline into a header.
>>
>> Signed-off-by: Rob Clark <robclark at freedesktop.org>
>> ---
>> This is probably mostly used from mesa, but some drivers, test apps, etc
>> may also want to use it from libdrm.
>>
>>  Makefile.sources |  1 +
>>  libsync.h        | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 75 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..fc23b7f
>> --- /dev/null
>> +++ b/libsync.h
>> @@ -0,0 +1,74 @@
>> +/*
>> + *  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 <stdint.h>
>> +#include <string.h>
>> +#include <sys/ioctl.h>
>> +#include <sys/poll.h>
>> +
>> +// todo prob should just #include <linux/sync_file.h> ?
>> +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;
>> +
>> +     fds.fd = fd;
>> +     fds.events = POLLIN | POLLERR;
>
> POLLERR is implied and ignored in fds.events.
>
>> +     return poll(&fds, 1, timeout);
>
> Returns: -1 on error, 0 on timeout, 1 if signaled.
>
> This function is horrible wrt to -EINTR for example :) Hmm, mixing
> poll() and looping over signals until timeout doesn't look as
> straightforward.

hmm, this was just basically cut/paste from android libsync (and iirc
removing the legacy ioctls)..  although I did just realize there was a
newer version which turned this into:

   ret = poll(..);
   if (ret > 0)
      return 0;
   return ret;

perhaps not super-awesome for -EINTR handling.. and tbh I'm 100% of
the behavior when this used to be simply "return ioctl(fd,
SYNC_IOC_WAIT, &to);"..

>> +}
>> +
>> +static inline int sync_merge(const char *name, int fd1, int fd2)
>> +{
>> +     struct sync_merge_data data = {};
>> +     int err;
>
> What I liked was doing
>
> if (fd2 < 0)
>         return dup(fd1);
>
> if (fd1 < 0)
>         return dup(fd2);
>
> That makes accumulating the fences in the caller much easier (i.e. they
> start with
>         batch.fence_in = -1;
> then
>         batch.fence_in = sync_merge(batch.fence_in, fence);
> finished by
>         execbuf(&batch);
>         close(batch.fence_in);
>         batch.fence_in = -1;

I guess that would end up ignoring the fence name.. although not sure
that I care.  But probably we should either make the android version
behave the same, or pick new names for these fxns.  I think having
same names but slightly different behaviour would be confusing.

Oh, and now that I'm actually looking at that code, the extra null
terminator bit after the strncpy() is kinda unneeded..  should just
do:

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

(without the -1) instead

BR,
-R

> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre