| Message ID | 20161122132839.26250-2-robert.foss@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Robert, looks pretty good to me, have just found a few nits. With those addressed: Reviewed-by: Tomeu Vizoso <tomeu.vizoso@collabora.com> Regards, Tomeu On 22 November 2016 at 14:28, <robert.foss@collabora.com> wrote: > From: Robert Foss <robert.foss@collabora.com> > > Base functions to help testing the Sync File Framework (explicit fencing > mechanism ported from Android). > These functions allow you to create, use and destroy timelines and fences. > > Signed-off-by: Robert Foss <robert.foss@collabora.com> > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > Reviewed-by: Eric Engestrom <eric@engestrom.ch> > --- > lib/Makefile.sources | 2 + > lib/sw_sync.c | 199 +++++++++++++++++++++++++++++++++++++++++++++++++++ > lib/sw_sync.h | 48 +++++++++++++ > 3 files changed, 249 insertions(+) > create mode 100644 lib/sw_sync.c > create mode 100644 lib/sw_sync.h > > diff --git a/lib/Makefile.sources b/lib/Makefile.sources > index e8e277b..7aaf42f 100644 > --- a/lib/Makefile.sources > +++ b/lib/Makefile.sources > @@ -63,6 +63,8 @@ lib_source_list = \ > rendercopy_gen8.c \ > rendercopy_gen9.c \ > rendercopy.h \ > + sw_sync.c \ > + sw_sync.h \ > intel_reg_map.c \ > intel_iosf.c \ > igt_kms.c \ > diff --git a/lib/sw_sync.c b/lib/sw_sync.c > new file mode 100644 > index 0000000..862f8c4 > --- /dev/null > +++ b/lib/sw_sync.c > @@ -0,0 +1,199 @@ > +/* > + * Copyright 2012 Google, Inc What's the reason for this? I only see simple wrappers for the uapi in the IGT style, so I wonder how it can be based in some work that carries Google's copyright from 2012. > + * Copyright © 2016 Collabora, Ltd. > + * > + * Based on the implementation from the Android Open Source Project Ditto. > + * 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 (including the next > + * paragraph) 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. > + * > + * Authors: > + * Robert Foss <robert.foss@collabora.com> > + */ > + > +#ifndef ANDROID > +#define _GNU_SOURCE > +#else > +#include <libgen.h> > +#endif > +#include <fcntl.h> > +#include <poll.h> > +#include <stdint.h> > +#include <linux/sync_file.h> > +#include <sys/ioctl.h> > + > +#include "sw_sync.h" > +#include "drmtest.h" > +#include "ioctl_wrappers.h" > + > +#ifndef SW_SYNC_IOC_INC > > +struct sw_sync_create_fence_data { > + __u32 value; > + char name[32]; > + __s32 fence; > +}; > + > +#define SW_SYNC_IOC_MAGIC 'W' > +#define SW_SYNC_IOC_CREATE_FENCE _IOWR(SW_SYNC_IOC_MAGIC, 0,\ > + struct sw_sync_create_fence_data) > +#define SW_SYNC_IOC_INC _IOW(SW_SYNC_IOC_MAGIC, 1, __u32) > +#endif What about using the LOCAL_ scheme as in ioctl_wrappers.h so we have a bigger chance of remembering to remove the local definitions once we stop testing kernels without this API definition? > + > +#define DEVFS_SW_SYNC "/dev/sw_sync" What's the reason for this? May be worth adding a comment if we really need to keep this. > +#define DEBUGFS_SW_SYNC "/sys/kernel/debug/sync/sw_sync" > + > + > +int sw_sync_fd_is_valid(int fd) bool? > +{ > + int status; > + > + if (fd < 0) > + return 0; > + > + status = fcntl(fd, F_GETFD, 0); > + return status >= 0; > +} > + > +int sw_sync_timeline_create(void) > +{ > + int fd = open(DEVFS_SW_SYNC, O_RDWR); > + > + if (fd < 0) > + fd = open(DEBUGFS_SW_SYNC, O_RDWR); > + > + igt_assert(sw_sync_fd_is_valid(fd)); > + > + return fd; > +} > + > +int __sw_sync_fence_create(int fd, uint32_t seqno) > +{ > + > + struct sw_sync_create_fence_data data; > + > + memset(&data, 0, sizeof(data)); Just initialize to zero? > + data.value = seqno; > + > + if (igt_ioctl(fd, SW_SYNC_IOC_CREATE_FENCE, &data)) > + return -errno; > + > + return data.fence; > +} > + > +int sw_sync_fence_create(int fd, uint32_t seqno) > +{ > + int fence = __sw_sync_fence_create(fd, seqno); > + igt_assert(fence >= 0); > + return fence; > +} > + > +void sw_sync_timeline_inc(int fd, uint32_t count) > +{ > + uint32_t arg = count; > + > + do_ioctl(fd, SW_SYNC_IOC_INC, &arg); > +} > + > +int sync_merge(int fd1, int fd2) > +{ > + struct sync_merge_data data = {}; > + int err; > + > + data.fd2 = fd2; > + > + err = ioctl(fd1, SYNC_IOC_MERGE, &data); > + if (err < 0) > + return -errno; > + > + sw_sync_fd_is_valid(data.fence); Was the idea to assert on this? > + > + return data.fence; > +} > + > +int sync_wait(int fd, int timeout) > +{ > + struct pollfd fds = {0}; > + 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; > +} > + > +int sync_fence_count(int fd) > +{ > + struct sync_file_info info; > + > + memset(&info, 0, sizeof(info)); Just initialize to zero? > + if (ioctl(fd, SYNC_IOC_FILE_INFO, &info)) > + return -errno; > + > + return info.num_fences; > +} > + > +static int __sync_fence_count_status(int fd, int status) > +{ > + struct sync_file_info info; > + struct sync_fence_info *fence_info; > + int count; > + int i; > + > + memset(&info, 0, sizeof(info)); Just initialize to zero? > + if (ioctl(fd, SYNC_IOC_FILE_INFO, &info)) > + return -errno; > + > + fence_info = calloc(info.num_fences, sizeof(*fence_info)); > + if (!fence_info) > + return -ENOMEM; > + > + info.sync_fence_info = (uintptr_t)fence_info; > + if (ioctl(fd, SYNC_IOC_FILE_INFO, &info)) { > + count = -errno; > + } else { > + count = 0; > + for (i = 0 ; i < info.num_fences ; i++) > + count += fence_info[i].status == status; I think I would prefer if the code was longer by one line, and avoided doing arithmetic with booleans :) > + } > + > + free(fence_info); > + > + return count; > +} > + > +int sync_fence_count_status(int fd, int status) > +{ > + int count = __sync_fence_count_status(fd, status); > + igt_assert(count >= 0); > + return count; > +} > diff --git a/lib/sw_sync.h b/lib/sw_sync.h > new file mode 100644 > index 0000000..1f47b29 > --- /dev/null > +++ b/lib/sw_sync.h > @@ -0,0 +1,48 @@ > +/* > + * Copyright 2012 Google, Inc Same as above. > + * Copyright © 2016 Collabora, Ltd. > + * > + * Based on the implementation from the Android Open Source Project > + * > + * 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 (including the next > + * paragraph) 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. > + * > + * Authors: > + * Robert Foss <robert.foss@collabora.com> > + */ > + > +#ifndef SW_SYNC_H > +#define SW_SYNC_H > + > +#define SW_SYNC_FENCE_STATUS_ERROR (-1) > +#define SW_SYNC_FENCE_STATUS_ACTIVE (0) > +#define SW_SYNC_FENCE_STATUS_SIGNALED (1) > + > +int sw_sync_fd_is_valid(int fd); This one doesn't seem to be used outside sw_sync.c, so maybe remove? Haven't checked the ones below. > +int sw_sync_timeline_create(void); > +int __sw_sync_fence_create(int fd, uint32_t seqno); > +int sw_sync_fence_create(int fd, uint32_t seqno); > +void sw_sync_timeline_inc(int fd, uint32_t count); > +int sync_merge(int fd1, int fd2); > +int sync_wait(int fence, int timeout); > +int sync_fence_count(int fd); > +int sync_fence_count_status(int fd, int status); > + > +#endif > + > -- > 2.10.2 >
diff --git a/lib/Makefile.sources b/lib/Makefile.sources index e8e277b..7aaf42f 100644 --- a/lib/Makefile.sources +++ b/lib/Makefile.sources @@ -63,6 +63,8 @@ lib_source_list = \ rendercopy_gen8.c \ rendercopy_gen9.c \ rendercopy.h \ + sw_sync.c \ + sw_sync.h \ intel_reg_map.c \ intel_iosf.c \ igt_kms.c \ diff --git a/lib/sw_sync.c b/lib/sw_sync.c new file mode 100644 index 0000000..862f8c4 --- /dev/null +++ b/lib/sw_sync.c @@ -0,0 +1,199 @@ +/* + * Copyright 2012 Google, Inc + * Copyright © 2016 Collabora, Ltd. + * + * Based on the implementation from the Android Open Source Project + * + * 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 (including the next + * paragraph) 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. + * + * Authors: + * Robert Foss <robert.foss@collabora.com> + */ + +#ifndef ANDROID +#define _GNU_SOURCE +#else +#include <libgen.h> +#endif +#include <fcntl.h> +#include <poll.h> +#include <stdint.h> +#include <linux/sync_file.h> +#include <sys/ioctl.h> + +#include "sw_sync.h" +#include "drmtest.h" +#include "ioctl_wrappers.h" + +#ifndef SW_SYNC_IOC_INC +struct sw_sync_create_fence_data { + __u32 value; + char name[32]; + __s32 fence; +}; + +#define SW_SYNC_IOC_MAGIC 'W' +#define SW_SYNC_IOC_CREATE_FENCE _IOWR(SW_SYNC_IOC_MAGIC, 0,\ + struct sw_sync_create_fence_data) +#define SW_SYNC_IOC_INC _IOW(SW_SYNC_IOC_MAGIC, 1, __u32) +#endif + +#define DEVFS_SW_SYNC "/dev/sw_sync" +#define DEBUGFS_SW_SYNC "/sys/kernel/debug/sync/sw_sync" + + +int sw_sync_fd_is_valid(int fd) +{ + int status; + + if (fd < 0) + return 0; + + status = fcntl(fd, F_GETFD, 0); + return status >= 0; +} + +int sw_sync_timeline_create(void) +{ + int fd = open(DEVFS_SW_SYNC, O_RDWR); + + if (fd < 0) + fd = open(DEBUGFS_SW_SYNC, O_RDWR); + + igt_assert(sw_sync_fd_is_valid(fd)); + + return fd; +} + +int __sw_sync_fence_create(int fd, uint32_t seqno) +{ + + struct sw_sync_create_fence_data data; + + memset(&data, 0, sizeof(data)); + data.value = seqno; + + if (igt_ioctl(fd, SW_SYNC_IOC_CREATE_FENCE, &data)) + return -errno; + + return data.fence; +} + +int sw_sync_fence_create(int fd, uint32_t seqno) +{ + int fence = __sw_sync_fence_create(fd, seqno); + igt_assert(fence >= 0); + return fence; +} + +void sw_sync_timeline_inc(int fd, uint32_t count) +{ + uint32_t arg = count; + + do_ioctl(fd, SW_SYNC_IOC_INC, &arg); +} + +int sync_merge(int fd1, int fd2) +{ + struct sync_merge_data data = {}; + int err; + + data.fd2 = fd2; + + err = ioctl(fd1, SYNC_IOC_MERGE, &data); + if (err < 0) + return -errno; + + sw_sync_fd_is_valid(data.fence); + + return data.fence; +} + +int sync_wait(int fd, int timeout) +{ + struct pollfd fds = {0}; + 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; +} + +int sync_fence_count(int fd) +{ + struct sync_file_info info; + + memset(&info, 0, sizeof(info)); + if (ioctl(fd, SYNC_IOC_FILE_INFO, &info)) + return -errno; + + return info.num_fences; +} + +static int __sync_fence_count_status(int fd, int status) +{ + struct sync_file_info info; + struct sync_fence_info *fence_info; + int count; + int i; + + memset(&info, 0, sizeof(info)); + if (ioctl(fd, SYNC_IOC_FILE_INFO, &info)) + return -errno; + + fence_info = calloc(info.num_fences, sizeof(*fence_info)); + if (!fence_info) + return -ENOMEM; + + info.sync_fence_info = (uintptr_t)fence_info; + if (ioctl(fd, SYNC_IOC_FILE_INFO, &info)) { + count = -errno; + } else { + count = 0; + for (i = 0 ; i < info.num_fences ; i++) + count += fence_info[i].status == status; + } + + free(fence_info); + + return count; +} + +int sync_fence_count_status(int fd, int status) +{ + int count = __sync_fence_count_status(fd, status); + igt_assert(count >= 0); + return count; +} diff --git a/lib/sw_sync.h b/lib/sw_sync.h new file mode 100644 index 0000000..1f47b29 --- /dev/null +++ b/lib/sw_sync.h @@ -0,0 +1,48 @@ +/* + * Copyright 2012 Google, Inc + * Copyright © 2016 Collabora, Ltd. + * + * Based on the implementation from the Android Open Source Project + * + * 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 (including the next + * paragraph) 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. + * + * Authors: + * Robert Foss <robert.foss@collabora.com> + */ + +#ifndef SW_SYNC_H +#define SW_SYNC_H + +#define SW_SYNC_FENCE_STATUS_ERROR (-1) +#define SW_SYNC_FENCE_STATUS_ACTIVE (0) +#define SW_SYNC_FENCE_STATUS_SIGNALED (1) + +int sw_sync_fd_is_valid(int fd); +int sw_sync_timeline_create(void); +int __sw_sync_fence_create(int fd, uint32_t seqno); +int sw_sync_fence_create(int fd, uint32_t seqno); +void sw_sync_timeline_inc(int fd, uint32_t count); +int sync_merge(int fd1, int fd2); +int sync_wait(int fence, int timeout); +int sync_fence_count(int fd); +int sync_fence_count_status(int fd, int status); + +#endif +