diff mbox

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

Message ID 20160912220842.18752-2-robert.foss@collabora.com (mailing list archive)
State New, archived
Headers show

Commit Message

Robert Foss Sept. 12, 2016, 10:08 p.m. UTC
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        | 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

Comments

Chris Wilson Sept. 13, 2016, 11:03 a.m. UTC | #1
On Mon, Sep 12, 2016 at 06:08:30PM -0400, 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        | 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@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
Robert Foss Sept. 13, 2016, 3:40 p.m. UTC | #2
On 2016-09-13 07:03 AM, Chris Wilson wrote:
> On Mon, Sep 12, 2016 at 06:08:30PM -0400, 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        | 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@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.

It's not really trying to prevent any bugs, but rather the idea is have
low cognitive load on these functions. If an fd is valid it can always 
be closed.

Or are you suggesting that the above code is equivalent to close()?
If so it should be replaced by close().

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

Replacing if (!sw_sync_fd_is_valid(fd)) with fd<0 is seems to me like a 
good change, in this scenario they should be equivalent.

Adding igt_require() is a bit inflexbile, especially when it comes to 
actually testing timeline creation.

>
>> +
>> +		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.

You're definitely, right. I'll remove this function from lib/sw_sync and 
tests/sw_sync in v3.

>
>> +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.
>

What would the corresponding negative test code look like?
A call to __sw_sync_fence_create? Then __sw_sync_fence_create would have 
to be made accessible outside of lib/sw_sync.

Or maybe creating a second user of __sw_sync_fence_create along the 
lines of sw_sync_fence_create_fail with an inverted igt_assert check
is what you're suggesting.

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

Nothing. I'll fix this in v3.

>
>> +
>> +	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.

Will be fixed in v3.

>
>> +
>> +	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.

I'm slightly confused by this comment.
sw_sync_fence_size seems to be making just one query, just the same 
number as the revised code attached below.

>
> sw_sync_fence_count not size.

Agreed, count is the right name, will fix in v3.

>
> 	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.

That's a good improvement, I'll include it in v3.

Thanks for all of the feedback Chris. I'll spin and test v3 now.


Rob.
Chris Wilson Sept. 14, 2016, 12:18 p.m. UTC | #3
On Tue, Sep 13, 2016 at 11:40:18AM -0400, Robert Foss wrote:
> 
> 
> On 2016-09-13 07:03 AM, Chris Wilson wrote:
> >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.
> >
> 
> What would the corresponding negative test code look like?
> A call to __sw_sync_fence_create? Then __sw_sync_fence_create would
> have to be made accessible outside of lib/sw_sync.
> 
> Or maybe creating a second user of __sw_sync_fence_create along the
> lines of sw_sync_fence_create_fail with an inverted igt_assert check
> is what you're suggesting.

Exactly. Make the raw unchecked version available to tests. We have been
using __gem_foo() and gem_foo() to identify the difference. __gem_foo()
reports the error to the caller (so that they can feed in different
values of garbage and check for different errno, or maybe used as a
probe to see if the kernel supports such a function) and gem_foo() for
everyone else where we want to just focus on writing a test and hide the
error handling clutter.

Try not to put the error handling tests in the library itself.
-Chris
Robert Foss Sept. 14, 2016, 3:01 p.m. UTC | #4
On 2016-09-14 08:18 AM, Chris Wilson wrote:
> On Tue, Sep 13, 2016 at 11:40:18AM -0400, Robert Foss wrote:
>>
>>
>> On 2016-09-13 07:03 AM, Chris Wilson wrote:
>>> 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.
>>>
>>
>> What would the corresponding negative test code look like?
>> A call to __sw_sync_fence_create? Then __sw_sync_fence_create would
>> have to be made accessible outside of lib/sw_sync.
>>
>> Or maybe creating a second user of __sw_sync_fence_create along the
>> lines of sw_sync_fence_create_fail with an inverted igt_assert check
>> is what you're suggesting.
>
> Exactly. Make the raw unchecked version available to tests. We have been
> using __gem_foo() and gem_foo() to identify the difference. __gem_foo()
> reports the error to the caller (so that they can feed in different
> values of garbage and check for different errno, or maybe used as a
> probe to see if the kernel supports such a function) and gem_foo() for
> everyone else where we want to just focus on writing a test and hide the
> error handling clutter.
>
> Try not to put the error handling tests in the library itself.

Thanks!
Coming up in v4.


Rob.
diff mbox

Patch

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@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);
+}
+
+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);
+
+	igt_assert(sw_sync_fd_is_valid(fd));
+
+	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) {
+		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;
+	}
+}
+
+void sw_sync_timeline_inc(int fd, uint32_t count)
+{
+	uint32_t arg = count;
+
+	if (fd == 0 || fd == -1)
+		return;
+
+	do_ioctl(fd, SW_SYNC_IOC_INC, &arg);
+}
+
+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;
+
+	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;
+}
+
+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;
+}
diff --git a/lib/sw_sync.h b/lib/sw_sync.h
new file mode 100644
index 0000000..1092608
--- /dev/null
+++ b/lib/sw_sync.h
@@ -0,0 +1,49 @@ 
+/*
+ * 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);
+void sw_sync_timeline_destroy(int timeline);
+void sw_sync_fence_destroy(int fence);
+int sw_sync_fence_create(int fd, int32_t seqno);
+void sw_sync_timeline_inc(int fd, uint32_t count);
+int sw_sync_merge(int fd1, int fd2);
+int sw_sync_wait(int fence, int timeout);
+int sw_sync_fence_size(int fd);
+int sw_sync_fence_count_status(int fd, int status);
+
+#endif
+