Hi Matt,
On 2022-12-05 at 17:11:44 +0000, Matthew Auld wrote:
> 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@xxxxxxxxx>
> > > Cc: Kamil Konieczny <kamil.konieczny@xxxxxxxxxxxxxxx>
> > > Cc: Petri Latvala <petri.latvala@xxxxxxxxx>
> > > Cc: Andrzej Hajda <andrzej.hajda@xxxxxxxxx>
> > > Cc: Nirmoy Das <nirmoy.das@xxxxxxxxx>
> > > ---
> > > .../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?
You are right, sorry, I readed it as igt_ioctl so
please do not follow my comment here.
Regards,
Kamil
>
> >
> > > +{
> > > + 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
> > >
