Re: [igt-dev] [PATCH i-g-t v2 1/2] lib/dmabuf_sync_file: move common stuff into lib

[Matthew Auld <matthew.auld@intel.com>]

On 05/12/2022 16:31, Kamil Konieczny wrote:
> Hi Matt,
> 
> after re-reading I have few more nits.
> 
> On 2022-12-02 at 12:02:41 +0000, Matthew Auld wrote:
>> So we can use this across different tests.
>>
>> v2
>>    - Add docs for everything (Petri)
>>    - Add missing copyright and fix headers slightly (Kamil)
>>
>> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
>> Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
>> Cc: Petri Latvala <petri.latvala@intel.com>
>> Cc: Andrzej Hajda <andrzej.hajda@intel.com>
>> Cc: Nirmoy Das <nirmoy.das@intel.com>
>> ---
>>   .../igt-gpu-tools/igt-gpu-tools-docs.xml      |   1 +
>>   lib/dmabuf_sync_file.c                        | 211 ++++++++++++++++++
>>   lib/dmabuf_sync_file.h                        |  26 +++
>>   lib/meson.build                               |   1 +
>>   tests/dmabuf_sync_file.c                      | 133 +----------
>>   5 files changed, 243 insertions(+), 129 deletions(-)
>>   create mode 100644 lib/dmabuf_sync_file.c
>>   create mode 100644 lib/dmabuf_sync_file.h
>>
>> diff --git a/docs/reference/igt-gpu-tools/igt-gpu-tools-docs.xml b/docs/reference/igt-gpu-tools/igt-gpu-tools-docs.xml
>> index 1ce842f4..102c8a89 100644
>> --- a/docs/reference/igt-gpu-tools/igt-gpu-tools-docs.xml
>> +++ b/docs/reference/igt-gpu-tools/igt-gpu-tools-docs.xml
>> @@ -15,6 +15,7 @@
>>   
>>     <chapter>
>>       <title>API Reference</title>
>> +    <xi:include href="xml/dmabuf_sync_file.xml"/>
>>       <xi:include href="xml/drmtest.xml"/>
>>       <xi:include href="xml/igt_alsa.xml"/>
>>       <xi:include href="xml/igt_audio.xml"/>
>> diff --git a/lib/dmabuf_sync_file.c b/lib/dmabuf_sync_file.c
>> new file mode 100644
>> index 00000000..bcce2486
>> --- /dev/null
>> +++ b/lib/dmabuf_sync_file.c
>> @@ -0,0 +1,211 @@
>> +// SPDX-License-Identifier: MIT
>> +/*
>> + * Copyright © 2022 Intel Corporation
>> + */
>> +
>> +#include "igt.h"
>> +#include "igt_vgem.h"
>> +#include "sw_sync.h"
>> +
>> +#include "dmabuf_sync_file.h"
>> +
>> +/**
>> + * SECTION: dmabuf_sync_file
>> + * @short_description: DMABUF importing/exporting fencing support library
>> + * @title: DMABUF Sync File
>> + * @include: dmabuf_sync_file.h
>> + */
>> +
>> +struct igt_dma_buf_sync_file {
>> +	__u32 flags;
>> +	__s32 fd;
>> +};
>> +
>> +#define IGT_DMA_BUF_IOCTL_EXPORT_SYNC_FILE _IOWR(DMA_BUF_BASE, 2, struct igt_dma_buf_sync_file)
>> +#define IGT_DMA_BUF_IOCTL_IMPORT_SYNC_FILE _IOW(DMA_BUF_BASE, 3, struct igt_dma_buf_sync_file)
>> +
>> +/**
>> + * has_dmabuf_export_sync_file:
>> + * @fd: The open drm fd
>> + *
>> + * Check if the kernel supports exporting a sync file from dmabuf.
>> + */
>> +bool has_dmabuf_export_sync_file(int fd)
>> +{
>> +	struct vgem_bo bo;
>> +	int dmabuf, ret;
>> +	struct igt_dma_buf_sync_file arg;
>> +
>> +	bo.width = 1;
>> +	bo.height = 1;
>> +	bo.bpp = 32;
>> +	vgem_create(fd, &bo);
>> +
>> +	dmabuf = prime_handle_to_fd(fd, bo.handle);
>> +	gem_close(fd, bo.handle);
>> +
>> +	arg.flags = DMA_BUF_SYNC_WRITE;
>> +	arg.fd = -1;
>> +
>> +	ret = igt_ioctl(dmabuf, IGT_DMA_BUF_IOCTL_EXPORT_SYNC_FILE, &arg);
>> +	close(dmabuf);
>> +	igt_assert(ret == 0 || errno == ENOTTY);
> 
> imho we should not explode here, it was ok in test but in lib
> we should just return false in case of unexpected error, you can
> add igt_debug if you think it will help.
> This may lead to change in place where you use it, like
> 	igt_require(has_dmabuf_export_sync_file(fd));

Yup, makes sense. Will fix.

> 
>> +
>> +	return ret == 0;
>> +}
>> +
>> +/**
>> + * dmabuf_export_sync_file:
>> + * @dmabuf: The dmabuf fd
>> + * @flags: The flags to control the behaviour
>> + *
>> + * Take a snapshot of the current dma-resv fences in the dmabuf, and export as a
>> + * syncfile. The @flags should at least specify either DMA_BUF_SYNC_WRITE or
>> + * DMA_BUF_SYNC_READ, depending on if we care about the read or write fences.
>> + */
>> +int dmabuf_export_sync_file(int dmabuf, uint32_t flags)
> 
> As you do not check for errors so this should be
> int __dmabuf_export_sync_file(int dmabuf, uint32_t flags)

do_ioctl() is doing an igt_assert_eq(ioctl(...), 0). Or what do you mean 
by not checking for errors?

> 
>> +{
>> +	struct igt_dma_buf_sync_file arg;
>> +
>> +	arg.flags = flags;
>> +	arg.fd = -1;
>> +	do_ioctl(dmabuf, IGT_DMA_BUF_IOCTL_EXPORT_SYNC_FILE, &arg);
>> +
>> +	return arg.fd;
>> +}
>> +
>> +/**
>> + * has_dmabuf_import_sync_file:
>> + * @fd: The open drm fd
>> + *
>> + * Check if the kernel supports importing a sync file into a dmabuf.
>> + */
>> +bool has_dmabuf_import_sync_file(int fd)
>> +{
>> +	struct vgem_bo bo;
>> +	int dmabuf, timeline, fence, ret;
>> +	struct igt_dma_buf_sync_file arg;
>> +
>> +	bo.width = 1;
>> +	bo.height = 1;
>> +	bo.bpp = 32;
>> +	vgem_create(fd, &bo);
>> +
>> +	dmabuf = prime_handle_to_fd(fd, bo.handle);
>> +	gem_close(fd, bo.handle);
>> +
>> +	timeline = sw_sync_timeline_create();
>> +	fence = sw_sync_timeline_create_fence(timeline, 1);
>> +	sw_sync_timeline_inc(timeline, 1);
>> +
>> +	arg.flags = DMA_BUF_SYNC_RW;
>> +	arg.fd = fence;
>> +
>> +	ret = igt_ioctl(dmabuf, IGT_DMA_BUF_IOCTL_IMPORT_SYNC_FILE, &arg);
>> +	close(dmabuf);
>> +	close(fence);
>> +	igt_assert(ret == 0 || errno == ENOTTY);
> 
> Same here, return false instead of assert.
> 
>> +
>> +	return ret == 0;
>> +}
>> +
>> +/**
>> + * dmabuf_import_sync_file:
>> + * @dmabuf: The dmabuf fd
>> + * @flags: The flags to control the behaviour
>> + * @sync_fd: The sync file (i.e our fence) to import
>> + *
>> + * Import the sync file @sync_fd, into the dmabuf. The @flags should at least
>> + * specify DMA_BUF_SYNC_WRITE or DMA_BUF_SYNC_READ, depending on if we are
>> + * importing the @sync_fd as a read or write fence.
>> + */
>> +void dmabuf_import_sync_file(int dmabuf, uint32_t flags, int sync_fd)
> 
> Same here, no error checks so use __ like:
> void __dmabuf_import_sync_file(int dmabuf, uint32_t flags, int sync_fd)
> 
>> +{
>> +	struct igt_dma_buf_sync_file arg;
>> +
>> +	arg.flags = flags;
>> +	arg.fd = sync_fd;
>> +	do_ioctl(dmabuf, IGT_DMA_BUF_IOCTL_IMPORT_SYNC_FILE, &arg);
>> +}
>> +
>> +/**
>> + * dmabuf_import_timeline_fence:
>> + * @dmabuf: The dmabuf fd
>> + * @flags: The flags to control the behaviour
>> + * @timeline: The sync file timeline where the new fence is created
>> + * @seqno: The sequence number to use for the fence
>> + *
>> + * Create a new fence as part of @timeline, and import as a sync file into the
>> + * dmabuf.  The @flags should at least specify DMA_BUF_SYNC_WRITE or
>> + * DMA_BUF_SYNC_READ, depending on if we are importing the new fence as a read
>> + * or write fence.
>> + */
>> +void dmabuf_import_timeline_fence(int dmabuf, uint32_t flags,
> 
> Same here, no error checks so use __ before function name.
> 
>> +				  int timeline, uint32_t seqno)
>> +{
>> +	int fence;
>> +
>> +	fence = sw_sync_timeline_create_fence(timeline, seqno);
>> +	dmabuf_import_sync_file(dmabuf, flags, fence);
>> +	close(fence);
>> +}
>> +
>> +/**
>> + * dmabuf_busy:
>> + * @dmabuf: The dmabuf fd
>> + * @flags: The flags to control the behaviour
>> + *
>> + * Check if the fences in the dmabuf are still busy. The @flags should at least
>> + * specify DMA_BUF_SYNC_WRITE or DMA_BUF_SYNC_READ, depending on if we are
>> + * checking if the read or read fences have all signalled. Or DMA_BUF_SYNC_RW if
>> + * we care about both.
>> + */
>> +bool dmabuf_busy(int dmabuf, uint32_t flags)
>> +{
>> +	struct pollfd pfd = { .fd = dmabuf };
>> +
>> +	/* If DMA_BUF_SYNC_WRITE is set, we don't want to set POLLIN or
>> +	 * else poll() may return a non-zero value if there are only read
>> +	 * fences because POLLIN is ready even if POLLOUT isn't.
>> +	 */
>> +	if (flags & DMA_BUF_SYNC_WRITE)
>> +		pfd.events |= POLLOUT;
>> +	else if (flags & DMA_BUF_SYNC_READ)
>> +		pfd.events |= POLLIN;
>> +
>> +	return poll(&pfd, 1, 0) == 0;
>> +}
>> +
>> +/**
>> + * sync_file_busy:
>> + * @sync_file: The sync file to check
>> + *
>> + * Check if the @sync_file is still busy or not.
>> + */
>> +bool sync_file_busy(int sync_file)
>> +{
>> +	struct pollfd pfd = { .fd = sync_file, .events = POLLIN };
>> +	return poll(&pfd, 1, 0) == 0;
>> +}
>> +
>> +/**
>> + * dmabuf_sync_file_busy:
>> + * @dmabuf: The dmabuf fd
>> + * @flags: The flags to control the behaviour
>> + *
>> + * Export the current fences in @dmabuf as a sync file and check if still busy.
>> + * The @flags should at least contain DMA_BUF_SYNC_WRITE or DMA_BUF_SYNC_READ,
>> + * to specify which fences are to be exported from the @dmabuf and checked if
>> + * busy. Or DMA_BUF_SYNC_RW if we care about both.
>> + */
>> +bool dmabuf_sync_file_busy(int dmabuf, uint32_t flags)
>> +{
>> +	int sync_file;
>> +	bool busy;
>> +
>> +	sync_file = dmabuf_export_sync_file(dmabuf, flags);
> 
> Check for error here.
> 
> Regards,
> Kamil
> 
>> +	busy = sync_file_busy(sync_file);
>> +	close(sync_file);
>> +
>> +	return busy;
>> +}
>> diff --git a/lib/dmabuf_sync_file.h b/lib/dmabuf_sync_file.h
>> new file mode 100644
>> index 00000000..d642ff30
>> --- /dev/null
>> +++ b/lib/dmabuf_sync_file.h
>> @@ -0,0 +1,26 @@
>> +/* SPDX-License-Identifier: MIT */
>> +/*
>> + * Copyright © 2022 Intel Corporation
>> + */
>> +
>> +#ifndef DMABUF_SYNC_FILE_H
>> +#define DMABUF_SYNC_FILE_H
>> +
>> +#ifdef __linux__
>> +#include <linux/dma-buf.h>
>> +#endif
>> +#include <sys/poll.h>
>> +#include <stdbool.h>
>> +#include <stdint.h>
>> +
>> +bool has_dmabuf_export_sync_file(int fd);
>> +bool has_dmabuf_import_sync_file(int fd);
>> +int dmabuf_export_sync_file(int dmabuf, uint32_t flags);
>> +void dmabuf_import_sync_file(int dmabuf, uint32_t flags, int sync_fd);
>> +void dmabuf_import_timeline_fence(int dmabuf, uint32_t flags,
>> +			     int timeline, uint32_t seqno);
>> +bool dmabuf_busy(int dmabuf, uint32_t flags);
>> +bool sync_file_busy(int sync_file);
>> +bool dmabuf_sync_file_busy(int dmabuf, uint32_t flags);
>> +
>> +#endif
>> diff --git a/lib/meson.build b/lib/meson.build
>> index cef2d0ff..896d5733 100644
>> --- a/lib/meson.build
>> +++ b/lib/meson.build
>> @@ -1,5 +1,6 @@
>>   lib_sources = [
>>   	'drmtest.c',
>> +	'dmabuf_sync_file.c',
>>   	'huc_copy.c',
>>   	'i915/gem.c',
>>   	'i915/gem_context.c',
>> diff --git a/tests/dmabuf_sync_file.c b/tests/dmabuf_sync_file.c
>> index 2179a76d..25bb6ad7 100644
>> --- a/tests/dmabuf_sync_file.c
>> +++ b/tests/dmabuf_sync_file.c
>> @@ -1,140 +1,15 @@
>>   // SPDX-License-Identifier: MIT
>> +/*
>> + * Copyright © 2022 Intel Corporation
>> + */
>>   
>>   #include "igt.h"
>>   #include "igt_vgem.h"
>>   #include "sw_sync.h"
>> -
>> -#include <linux/dma-buf.h>
>> -#include <sys/poll.h>
>> +#include "dmabuf_sync_file.h"
>>   
>>   IGT_TEST_DESCRIPTION("Tests for sync_file support in dma-buf");
>>   
>> -struct igt_dma_buf_sync_file {
>> -	__u32 flags;
>> -	__s32 fd;
>> -};
>> -
>> -#define IGT_DMA_BUF_IOCTL_EXPORT_SYNC_FILE _IOWR(DMA_BUF_BASE, 2, struct igt_dma_buf_sync_file)
>> -#define IGT_DMA_BUF_IOCTL_IMPORT_SYNC_FILE _IOW(DMA_BUF_BASE, 3, struct igt_dma_buf_sync_file)
>> -
>> -static bool has_dmabuf_export_sync_file(int fd)
>> -{
>> -	struct vgem_bo bo;
>> -	int dmabuf, ret;
>> -	struct igt_dma_buf_sync_file arg;
>> -
>> -	bo.width = 1;
>> -	bo.height = 1;
>> -	bo.bpp = 32;
>> -	vgem_create(fd, &bo);
>> -
>> -	dmabuf = prime_handle_to_fd(fd, bo.handle);
>> -	gem_close(fd, bo.handle);
>> -
>> -	arg.flags = DMA_BUF_SYNC_WRITE;
>> -	arg.fd = -1;
>> -
>> -	ret = igt_ioctl(dmabuf, IGT_DMA_BUF_IOCTL_EXPORT_SYNC_FILE, &arg);
>> -	close(dmabuf);
>> -	igt_assert(ret == 0 || errno == ENOTTY);
>> -
>> -	return ret == 0;
>> -}
>> -
>> -static int dmabuf_export_sync_file(int dmabuf, uint32_t flags)
>> -{
>> -	struct igt_dma_buf_sync_file arg;
>> -
>> -	arg.flags = flags;
>> -	arg.fd = -1;
>> -	do_ioctl(dmabuf, IGT_DMA_BUF_IOCTL_EXPORT_SYNC_FILE, &arg);
>> -
>> -	return arg.fd;
>> -}
>> -
>> -static bool has_dmabuf_import_sync_file(int fd)
>> -{
>> -	struct vgem_bo bo;
>> -	int dmabuf, timeline, fence, ret;
>> -	struct igt_dma_buf_sync_file arg;
>> -
>> -	bo.width = 1;
>> -	bo.height = 1;
>> -	bo.bpp = 32;
>> -	vgem_create(fd, &bo);
>> -
>> -	dmabuf = prime_handle_to_fd(fd, bo.handle);
>> -	gem_close(fd, bo.handle);
>> -
>> -	timeline = sw_sync_timeline_create();
>> -	fence = sw_sync_timeline_create_fence(timeline, 1);
>> -	sw_sync_timeline_inc(timeline, 1);
>> -
>> -	arg.flags = DMA_BUF_SYNC_RW;
>> -	arg.fd = fence;
>> -
>> -	ret = igt_ioctl(dmabuf, IGT_DMA_BUF_IOCTL_IMPORT_SYNC_FILE, &arg);
>> -	close(dmabuf);
>> -	close(fence);
>> -	igt_assert(ret == 0 || errno == ENOTTY);
>> -
>> -	return ret == 0;
>> -}
>> -
>> -static void dmabuf_import_sync_file(int dmabuf, uint32_t flags, int sync_fd)
>> -{
>> -	struct igt_dma_buf_sync_file arg;
>> -
>> -	arg.flags = flags;
>> -	arg.fd = sync_fd;
>> -	do_ioctl(dmabuf, IGT_DMA_BUF_IOCTL_IMPORT_SYNC_FILE, &arg);
>> -}
>> -
>> -static void
>> -dmabuf_import_timeline_fence(int dmabuf, uint32_t flags,
>> -			     int timeline, uint32_t seqno)
>> -{
>> -	int fence;
>> -
>> -	fence = sw_sync_timeline_create_fence(timeline, seqno);
>> -	dmabuf_import_sync_file(dmabuf, flags, fence);
>> -	close(fence);
>> -}
>> -
>> -static bool dmabuf_busy(int dmabuf, uint32_t flags)
>> -{
>> -	struct pollfd pfd = { .fd = dmabuf };
>> -
>> -	/* If DMA_BUF_SYNC_WRITE is set, we don't want to set POLLIN or
>> -	 * else poll() may return a non-zero value if there are only read
>> -	 * fences because POLLIN is ready even if POLLOUT isn't.
>> -	 */
>> -	if (flags & DMA_BUF_SYNC_WRITE)
>> -		pfd.events |= POLLOUT;
>> -	else if (flags & DMA_BUF_SYNC_READ)
>> -		pfd.events |= POLLIN;
>> -
>> -	return poll(&pfd, 1, 0) == 0;
>> -}
>> -
>> -static bool sync_file_busy(int sync_file)
>> -{
>> -	struct pollfd pfd = { .fd = sync_file, .events = POLLIN };
>> -	return poll(&pfd, 1, 0) == 0;
>> -}
>> -
>> -static bool dmabuf_sync_file_busy(int dmabuf, uint32_t flags)
>> -{
>> -	int sync_file;
>> -	bool busy;
>> -
>> -	sync_file = dmabuf_export_sync_file(dmabuf, flags);
>> -	busy = sync_file_busy(sync_file);
>> -	close(sync_file);
>> -
>> -	return busy;
>> -}
>> -
>>   static void test_export_basic(int fd)
>>   {
>>   	struct vgem_bo bo;
>> -- 
>> 2.38.1
>>