[Intel-gfx] [PATCH i-g-t v2 01/13] lib/sw_sync: Add helper functions for managing synchronization primitives

[Chris Wilson]

On Mon, Sep 12, 2016 at 06:08:30PM -0400, robert.foss at collabora.com wrote:
> From: Robert Foss <robert.foss at 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 at collabora.com>
> Signed-off-by: Gustavo Padovan <gustavo.padovan at collabora.co.uk>
> Reviewed-by: Eric Engestrom <eric at engestrom.ch>
> ---
>  lib/Makefile.sources |   2 +
>  lib/sw_sync.c        | 237 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/sw_sync.h        |  49 +++++++++++
>  3 files changed, 288 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 bac9a7f..3dc7c3c 100644
> --- a/lib/Makefile.sources
> +++ b/lib/Makefile.sources
> @@ -61,6 +61,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..179a473
> --- /dev/null
> +++ b/lib/sw_sync.c
> @@ -0,0 +1,237 @@
> +/*
> + * 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>
> + */
> +
> +#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;
> +}
> +
> +static
> +void sw_sync_fd_close(int fd)
> +{
> +	if (!sw_sync_fd_is_valid(fd))
> +		return;
> +
> +	close(fd);

What bug are you trying to prevent? This is the same as just calling
close(fd) and seeing EINVAL.

> +}
> +
> +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_require(fd >= 0);

> +
> +		fd = open(DEBUGFS_SW_SYNC, O_RDWR);
> +
> +	igt_assert(sw_sync_fd_is_valid(fd));
> +
> +	return fd;
> +}
> +
> +void sw_sync_timeline_destroy(int fd)
> +{
> +	return sw_sync_fd_close(fd);

Now just close(fd); and might as well not obfuscate that these are plain
fd.

> +int sw_sync_fence_create(int fd, int32_t seqno)
> +{
> +	struct sw_sync_create_fence_data data = {};
> +	data.value = seqno;
> +
> +	if (fd >= 0) {
> +		do_ioctl(fd, SW_SYNC_IOC_CREATE_FENCE, &data);
> +		return data.fence;
> +	} else {
> +		do_ioctl_err(fd, SW_SYNC_IOC_CREATE_FENCE, &data, EBADF);
> +		return -1;

Try:

int __sw_sync_fence_create(int fd, int32_t seqno) /* int32_t not unsigned ? */
{

	struct sw_sync_create_fence_data data;

	memset(&data, 0, sizeof(data));
	data.value = seqno;

	if (igt_ioctl(fd, SW_SYNC_IOCT_CREATE_FENCE, &data))
		return -errno;
	
	return data.fence;
}

int sw_sync_fence_create(int fd, int32_t seqno)
{
	int fence = __sw_sync_fence_create(fd, seqno);
	igt_assert(fence >= 0);
	return fence;
}

Then only in the test code do you send garbage and check for the
expected errno.



> +	}
> +}
> +
> +void sw_sync_timeline_inc(int fd, uint32_t count)
> +{
> +	uint32_t arg = count;
> +
> +	if (fd == 0 || fd == -1)
> +		return;

What's wrong with fd == 0?

See above. By default assert that we don't fail, then have a function
for negative tests.

> +
> +	do_ioctl(fd, SW_SYNC_IOC_INC, &arg);

Try not to use do_ioctl(), it generates horrible messages from the macro
expansions of the ioctl numbers.

> +}
> +
> +int sw_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 err;

Our convention is to return -errno.

> +
> +	sw_sync_fd_is_valid(data.fence);
> +
> +	return data.fence;
> +}
> +
> +int sw_sync_wait(int fence, int timeout)
> +{
> +	struct pollfd fds;
> +	int ret;
> +
> +	fds.fd = fence;
> +	fds.events = POLLIN | POLLERR;
> +
> +	ret = poll(&fds, 1, timeout);
> +
> +	return ret;
> +}
> +
> +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);

Querying all those fences seems like a waste of time now.

sw_sync_fence_count not size.

	struct sync_file_info info;

	memset(&info, 0, sizeof(info));
	if (ioctl(fd, SYNC_IOC_FILE_INFO, &info))
		return -errno;
	
	return info.num_fences;

> +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;
> +}

int __sw_sync_fence_count_status(int fd, int status)
{
	struct sync_file_info info;
	struct sync_fence_info *fence_info;
	int count;

	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.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 sw_sync_fence_count_status(int fd, int status)
{
	int count = __sw_sync_fence_count_status(fd, status);
	igt_assert(count >= 0);
	return count;
}

So that regular callers don't have to worry about errors.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre