[Piglit] [PATCH v2 02/12] egl_android_native_fence_sync: Add sw_sync lib.

[Eric Anholt]

Rafael Antognolli <rafael.antognolli at intel.com> writes:

> Add a small library that helps manipulating software fences. They are
> useful for testing EGL Android fences, since the latter can be created
> out of them.
>
> Signed-off-by: Rafael Antognolli <rafael.antognolli at intel.com>
> ---
>  .../spec/egl_android_native_fence_sync/sw_sync.c   | 211 +++++++++++++++++++++
>  .../spec/egl_android_native_fence_sync/sw_sync.h   |  50 +++++
>  2 files changed, 261 insertions(+)
>  create mode 100644 tests/egl/spec/egl_android_native_fence_sync/sw_sync.c
>  create mode 100644 tests/egl/spec/egl_android_native_fence_sync/sw_sync.h
>
> diff --git a/tests/egl/spec/egl_android_native_fence_sync/sw_sync.c b/tests/egl/spec/egl_android_native_fence_sync/sw_sync.c
> new file mode 100644
> index 0000000..4e8117f
> --- /dev/null
> +++ b/tests/egl/spec/egl_android_native_fence_sync/sw_sync.c
> @@ -0,0 +1,211 @@
> +/*
> + * 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 at collabora.com>
> + */
> +
> +#include <fcntl.h>
> +#include <poll.h>
> +#include <stdint.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include <linux/sync_file.h>
> +#include <sys/ioctl.h>
> +
> +#include "sw_sync.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"
> +
> +bool sw_sync_is_supported(void)
> +{
> +	if(access(DEVFS_SW_SYNC, R_OK | W_OK) != -1) {
> +		return true;

space after "if"

> +	} else if (access(DEBUGFS_SW_SYNC, R_OK | W_OK) != -1 ) {
> +		return true;
> +	}
> +
> +	return false;
> +}
> +
> +int sw_sync_fd_is_valid(int fd)
> +{
> +	int status;
> +
> +	if (fd < 0)
> +		return 0;
> +
> +	status = fcntl(fd, F_GETFD, 0);
> +	return status >= 0;
> +}

Do we need to F_GETFD?  Couldn't all callers of this just "if (fd < 0)"?

> +
> +static
> +void sw_sync_fd_close(int fd)
> +{
> +	if (!sw_sync_fd_is_valid(fd))
> +		return;
> +
> +	close(fd);

Is there a reason to call is_valid() before close()?  I think you could
drop this entire function and just close(fd) from
sw_sync_timeline_destroy() and fence_destroy().

> +}
> +
> +int sw_sync_timeline_create(void)
> +{
> +	int fd = open(DEVFS_SW_SYNC, O_RDWR);
> +
> +	if (!sw_sync_fd_is_valid(fd))
> +		fd = open(DEBUGFS_SW_SYNC, O_RDWR);
> +
> +	return fd;
> +}
> +
> +void sw_sync_timeline_destroy(int fd)
> +{
> +	return sw_sync_fd_close(fd);
> +}
> +
> +void sw_sync_fence_destroy(int fd)
> +{
> +	return sw_sync_fd_close(fd);
> +}
> +
> +int sw_sync_fence_create(int fd, int32_t seqno)
> +{
> +	struct sw_sync_create_fence_data data = {};
> +	data.value = seqno;
> +
> +	if (fd >= 0) {
> +		ioctl(fd, SW_SYNC_IOC_CREATE_FENCE, &data);
> +		return data.fence;
> +	} else {
> +		ioctl(fd, SW_SYNC_IOC_CREATE_FENCE, &data);
> +		return -1;
> +	}

Calling an ioctl on a negative fd?  Shouldn't you just early return -1
with no ioctl call?

> +static struct sync_file_info *sync_file_info(int fd)
> +{
> +	struct sync_file_info *info;
> +	struct sync_fence_info *fence_info;
> +	int err, num_fences;
> +
> +	info = calloc(1, sizeof(*info));
> +	if (info == NULL)
> +		return NULL;
> +
> +	err = ioctl(fd, SYNC_IOC_FILE_INFO, info);
> +	if (err < 0) {
> +		free(info);
> +		return NULL;
> +	}
> +
> +	num_fences = info->num_fences;
> +
> +	if (num_fences) {
> +		info->flags = 0;
> +		info->num_fences = num_fences;
> +
> +		fence_info = calloc(num_fences, sizeof(*fence_info));
> +		if (!fence_info) {
> +			free(info);
> +			return NULL;
> +		}
> +
> +		info->sync_fence_info = (uint64_t)(unsigned long) (fence_info);
> +
> +		err = ioctl(fd, SYNC_IOC_FILE_INFO, info);
> +		if (err < 0) {
> +			free(fence_info);
> +			free(info);
> +			return NULL;
> +		}
> +	}
> +
> +	return info;
> +}
> +
> +static void sync_file_info_free(struct sync_file_info *info)
> +{
> +	free((void *)(uintptr_t)info->sync_fence_info);
> +	free(info);
> +}
> +
> +int sw_sync_fence_size(int fd)
> +{
> +	int count;
> +	struct sync_file_info *info = sync_file_info(fd);
> +
> +	if (!info)
> +		return 0;
> +
> +	count = info->num_fences;
> +
> +	sync_file_info_free(info);
> +
> +	return count;
> +}
> +
> +int sw_sync_fence_count_status(int fd, int status)
> +{
> +	int i, count = 0;
> +	struct sync_fence_info *fence_info = NULL;
> +	struct sync_file_info *info = sync_file_info(fd);
> +
> +	if (!info)
> +		return -1;
> +
> +	fence_info = (struct sync_fence_info *)(uintptr_t)info->sync_fence_info;
> +	for (i = 0 ; i < info->num_fences ; i++) {
> +		if (fence_info[i].status == status)
> +			count++;
> +	}
> +
> +	sync_file_info_free(info);
> +
> +	return count;
> +}

These functions seem to be unused.  Do we expect them to get used, or
should we drop them?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/piglit/attachments/20170616/b5195f82/attachment.sig>