Hi Robert,
looks pretty good to me, have just found a few nits.
With those addressed:
Reviewed-by: Tomeu Vizoso <tomeu.vizoso@xxxxxxxxxxxxx>
Regards,
Tomeu
On 22 November 2016 at 14:28, <robert.foss@xxxxxxxxxxxxx> wrote:
> From: Robert Foss <robert.foss@xxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxx>
> Signed-off-by: Gustavo Padovan <gustavo.padovan@xxxxxxxxxxxxxxx>
> Reviewed-by: Eric Engestrom <eric@xxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxx>
> + */
> +
> +#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@xxxxxxxxxxxxx>
> + */
> +
> +#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
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
