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

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

[Matthew Auld]

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);
+
+	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)
+{
+	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);
+
+	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)
+{
+	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,
+				  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);
+	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

[igt-dev] [PATCH i-g-t v2 2/2] tests/i915/gem_exec_balancer: exercise dmabuf import

[Matthew Auld]

With parallel submission it should be easy to get a fence array as the
output fence. Try importing this into dma-buf reservation object, to see
if anything explodes.

v2: (Kamil)
 - Use ifdef __linux__ for linux headers
 - Add igt_describe() for new test

References: https://gitlab.freedesktop.org/drm/intel/-/issues/7532
Signed-off-by: Matthew Auld <matthew.auld@intel.com>
Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
Cc: Andrzej Hajda <andrzej.hajda@intel.com>
Cc: Nirmoy Das <nirmoy.das@intel.com>
Reviewed-by: Andrzej Hajda <andrzej.hajda@intel.com>
---
 tests/i915/gem_exec_balancer.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/tests/i915/gem_exec_balancer.c b/tests/i915/gem_exec_balancer.c
index 4300dbd1..d7acdca1 100644
--- a/tests/i915/gem_exec_balancer.c
+++ b/tests/i915/gem_exec_balancer.c
@@ -27,6 +27,7 @@
 #include <sys/signal.h>
 #include <poll.h>
 
+#include "dmabuf_sync_file.h"
 #include "i915/gem.h"
 #include "i915/gem_engine_topology.h"
 #include "i915/gem_create.h"
@@ -2856,6 +2857,7 @@ static void logical_sort_siblings(int i915,
 #define PARALLEL_SUBMIT_FENCE		(0x1 << 3)
 #define PARALLEL_CONTEXTS		(0x1 << 4)
 #define PARALLEL_VIRTUAL		(0x1 << 5)
+#define PARALLEL_OUT_FENCE_DMABUF	(0x1 << 6)
 
 static void parallel_thread(int i915, unsigned int flags,
 			    struct i915_engine_class_instance *siblings,
@@ -2871,6 +2873,8 @@ static void parallel_thread(int i915, unsigned int flags,
 	uint32_t target_bo_idx = 0;
 	uint32_t first_bb_idx = 1;
 	intel_ctx_cfg_t cfg;
+	uint32_t dmabuf_handle;
+	int dmabuf;
 
 	igt_assert(bb_per_execbuf < 32);
 
@@ -2924,11 +2928,20 @@ static void parallel_thread(int i915, unsigned int flags,
 	execbuf.buffers_ptr = to_user_pointer(obj);
 	execbuf.rsvd1 = ctx->id;
 
+	if (flags & PARALLEL_OUT_FENCE_DMABUF) {
+		dmabuf_handle = gem_create(i915, 4096);
+		dmabuf = prime_handle_to_fd(i915, dmabuf_handle);
+	}
+
 	for (n = 0; n < PARALLEL_BB_LOOP_COUNT; ++n) {
 		execbuf.flags &= ~0x3full;
 		gem_execbuf_wr(i915, &execbuf);
 
 		if (flags & PARALLEL_OUT_FENCE) {
+			if (flags & PARALLEL_OUT_FENCE_DMABUF)
+				dmabuf_import_sync_file(dmabuf, DMA_BUF_SYNC_WRITE,
+							execbuf.rsvd2 >> 32);
+
 			igt_assert_eq(sync_fence_wait(execbuf.rsvd2 >> 32,
 						      1000), 0);
 			igt_assert_eq(sync_fence_status(execbuf.rsvd2 >> 32), 1);
@@ -2959,6 +2972,11 @@ static void parallel_thread(int i915, unsigned int flags,
 	if (fence)
 		close(fence);
 
+	if (flags & PARALLEL_OUT_FENCE_DMABUF) {
+		gem_close(i915, dmabuf_handle);
+		close(dmabuf);
+	}
+
 	check_bo(i915, obj[target_bo_idx].handle,
 		 bb_per_execbuf * PARALLEL_BB_LOOP_COUNT, true);
 
@@ -3420,6 +3438,11 @@ igt_main
 		igt_subtest("parallel-out-fence")
 			parallel(i915, PARALLEL_OUT_FENCE);
 
+		igt_describe("Regression test to check that dmabuf imported sync file can handle fence array");
+		igt_subtest("parallel-dmabuf-import-out-fence")
+			parallel(i915, PARALLEL_OUT_FENCE |
+				 PARALLEL_OUT_FENCE_DMABUF);
+
 		igt_subtest("parallel-keep-in-fence")
 			parallel(i915, PARALLEL_OUT_FENCE | PARALLEL_IN_FENCE);
 
-- 
2.38.1

[igt-dev] ✗ Fi.CI.BAT: failure for series starting with [i-g-t,v2,1/2] lib/dmabuf_sync_file: move common stuff into lib

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 5407 bytes --]

== Series Details ==

Series: series starting with [i-g-t,v2,1/2] lib/dmabuf_sync_file: move common stuff into lib
URL   : https://patchwork.freedesktop.org/series/111582/
State : failure

== Summary ==

CI Bug Log - changes from IGT_7079 -> IGTPW_8187
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with IGTPW_8187 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in IGTPW_8187, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8187/index.html

Participating hosts (42 -> 40)
------------------------------

  Additional (1): bat-atsm-1 
  Missing    (3): fi-ilk-m540 fi-tgl-dsi bat-dg1-6 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in IGTPW_8187:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live@execlists:
    - fi-apl-guc:         [PASS][1] -> [INCOMPLETE][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_7079/fi-apl-guc/igt@i915_selftest@live@execlists.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8187/fi-apl-guc/igt@i915_selftest@live@execlists.html

  
Known issues
------------

  Here are the changes found in IGTPW_8187 that come from known issues:

### IGT changes ###

#### Possible fixes ####

  * igt@i915_pm_rpm@module-reload:
    - {bat-rpls-2}:       [WARN][3] ([i915#7346]) -> [PASS][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_7079/bat-rpls-2/igt@i915_pm_rpm@module-reload.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8187/bat-rpls-2/igt@i915_pm_rpm@module-reload.html

  * igt@i915_selftest@live@gt_heartbeat:
    - fi-cfl-guc:         [DMESG-FAIL][5] ([i915#5334]) -> [PASS][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_7079/fi-cfl-guc/igt@i915_selftest@live@gt_heartbeat.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8187/fi-cfl-guc/igt@i915_selftest@live@gt_heartbeat.html
    - fi-apl-guc:         [DMESG-FAIL][7] ([i915#5334]) -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_7079/fi-apl-guc/igt@i915_selftest@live@gt_heartbeat.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8187/fi-apl-guc/igt@i915_selftest@live@gt_heartbeat.html

  * igt@i915_selftest@live@migrate:
    - {bat-dg2-11}:       [DMESG-WARN][9] ([i915#7359]) -> [PASS][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_7079/bat-dg2-11/igt@i915_selftest@live@migrate.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8187/bat-dg2-11/igt@i915_selftest@live@migrate.html

  * igt@i915_selftest@live@requests:
    - fi-kbl-soraka:      [INCOMPLETE][11] ([i915#7640]) -> [PASS][12]
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_7079/fi-kbl-soraka/igt@i915_selftest@live@requests.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8187/fi-kbl-soraka/igt@i915_selftest@live@requests.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#103375]: https://bugs.freedesktop.org/show_bug.cgi?id=103375
  [fdo#109295]: https://bugs.freedesktop.org/show_bug.cgi?id=109295
  [i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072
  [i915#1836]: https://gitlab.freedesktop.org/drm/intel/issues/1836
  [i915#2582]: https://gitlab.freedesktop.org/drm/intel/issues/2582
  [i915#4077]: https://gitlab.freedesktop.org/drm/intel/issues/4077
  [i915#4079]: https://gitlab.freedesktop.org/drm/intel/issues/4079
  [i915#4083]: https://gitlab.freedesktop.org/drm/intel/issues/4083
  [i915#5334]: https://gitlab.freedesktop.org/drm/intel/issues/5334
  [i915#6077]: https://gitlab.freedesktop.org/drm/intel/issues/6077
  [i915#6078]: https://gitlab.freedesktop.org/drm/intel/issues/6078
  [i915#6093]: https://gitlab.freedesktop.org/drm/intel/issues/6093
  [i915#6094]: https://gitlab.freedesktop.org/drm/intel/issues/6094
  [i915#6166]: https://gitlab.freedesktop.org/drm/intel/issues/6166
  [i915#6311]: https://gitlab.freedesktop.org/drm/intel/issues/6311
  [i915#6434]: https://gitlab.freedesktop.org/drm/intel/issues/6434
  [i915#6621]: https://gitlab.freedesktop.org/drm/intel/issues/6621
  [i915#6645]: https://gitlab.freedesktop.org/drm/intel/issues/6645
  [i915#6997]: https://gitlab.freedesktop.org/drm/intel/issues/6997
  [i915#7346]: https://gitlab.freedesktop.org/drm/intel/issues/7346
  [i915#7357]: https://gitlab.freedesktop.org/drm/intel/issues/7357
  [i915#7359]: https://gitlab.freedesktop.org/drm/intel/issues/7359
  [i915#7640]: https://gitlab.freedesktop.org/drm/intel/issues/7640


Build changes
-------------

  * CI: CI-20190529 -> None
  * IGT: IGT_7079 -> IGTPW_8187

  CI-20190529: 20190529
  CI_DRM_12462: 00b10bdfd8b9edc9b2c681d806fbb6ae2e5f31a3 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_8187: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8187/index.html
  IGT_7079: 272354f8e1bbeb56df1663c42a9958f2ff9b8f54 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git


Testlist changes
----------------

+igt@gem_exec_balancer@parallel-dmabuf-import-out-fence

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8187/index.html

[-- Attachment #2: Type: text/html, Size: 4935 bytes --]

Re: [igt-dev] ✗ Fi.CI.BAT: failure for series starting with [i-g-t,v2,1/2] lib/dmabuf_sync_file: move common stuff into lib

[Kamil Konieczny]

Hi Lakshmi,

these are unrelated to the change.

--
Kamil

On 2022-12-02 at 13:27:47 -0000, Patchwork wrote:
> == Series Details ==
> 
> Series: series starting with [i-g-t,v2,1/2] lib/dmabuf_sync_file: move common stuff into lib
> URL   : https://patchwork.freedesktop.org/series/111582/
> State : failure
> 
> == Summary ==
> 
> CI Bug Log - changes from IGT_7079 -> IGTPW_8187
> ====================================================
> 
> Summary
> -------
> 
>   **FAILURE**
> 
>   Serious unknown changes coming with IGTPW_8187 absolutely need to be
>   verified manually.
>   
>   If you think the reported changes have nothing to do with the changes
>   introduced in IGTPW_8187, please notify your bug team to allow them
>   to document this new failure mode, which will reduce false positives in CI.
> 
>   External URL: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8187/index.html
> 
> Participating hosts (42 -> 40)
> ------------------------------
> 
>   Additional (1): bat-atsm-1 
>   Missing    (3): fi-ilk-m540 fi-tgl-dsi bat-dg1-6 
> 
> Possible new issues
> -------------------
> 
>   Here are the unknown changes that may have been introduced in IGTPW_8187:
> 
> ### IGT changes ###
> 
> #### Possible regressions ####
> 
>   * igt@i915_selftest@live@execlists:
>     - fi-apl-guc:         [PASS][1] -> [INCOMPLETE][2]
>    [1]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_7079/fi-apl-guc/igt@i915_selftest@live@execlists.html
>    [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8187/fi-apl-guc/igt@i915_selftest@live@execlists.html
> 
>   
> Known issues
> ------------
> 
>   Here are the changes found in IGTPW_8187 that come from known issues:
> 
> ### IGT changes ###
> 
> #### Possible fixes ####
> 
>   * igt@i915_pm_rpm@module-reload:
>     - {bat-rpls-2}:       [WARN][3] ([i915#7346]) -> [PASS][4]
>    [3]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_7079/bat-rpls-2/igt@i915_pm_rpm@module-reload.html
>    [4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8187/bat-rpls-2/igt@i915_pm_rpm@module-reload.html
> 
>   * igt@i915_selftest@live@gt_heartbeat:
>     - fi-cfl-guc:         [DMESG-FAIL][5] ([i915#5334]) -> [PASS][6]
>    [5]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_7079/fi-cfl-guc/igt@i915_selftest@live@gt_heartbeat.html
>    [6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8187/fi-cfl-guc/igt@i915_selftest@live@gt_heartbeat.html
>     - fi-apl-guc:         [DMESG-FAIL][7] ([i915#5334]) -> [PASS][8]
>    [7]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_7079/fi-apl-guc/igt@i915_selftest@live@gt_heartbeat.html
>    [8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8187/fi-apl-guc/igt@i915_selftest@live@gt_heartbeat.html
> 
>   * igt@i915_selftest@live@migrate:
>     - {bat-dg2-11}:       [DMESG-WARN][9] ([i915#7359]) -> [PASS][10]
>    [9]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_7079/bat-dg2-11/igt@i915_selftest@live@migrate.html
>    [10]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8187/bat-dg2-11/igt@i915_selftest@live@migrate.html
> 
>   * igt@i915_selftest@live@requests:
>     - fi-kbl-soraka:      [INCOMPLETE][11] ([i915#7640]) -> [PASS][12]
>    [11]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_7079/fi-kbl-soraka/igt@i915_selftest@live@requests.html
>    [12]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8187/fi-kbl-soraka/igt@i915_selftest@live@requests.html
> 
>   
>   {name}: This element is suppressed. This means it is ignored when computing
>           the status of the difference (SUCCESS, WARNING, or FAILURE).
> 
>   [fdo#103375]: https://bugs.freedesktop.org/show_bug.cgi?id=103375
>   [fdo#109295]: https://bugs.freedesktop.org/show_bug.cgi?id=109295
>   [i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072
>   [i915#1836]: https://gitlab.freedesktop.org/drm/intel/issues/1836
>   [i915#2582]: https://gitlab.freedesktop.org/drm/intel/issues/2582
>   [i915#4077]: https://gitlab.freedesktop.org/drm/intel/issues/4077
>   [i915#4079]: https://gitlab.freedesktop.org/drm/intel/issues/4079
>   [i915#4083]: https://gitlab.freedesktop.org/drm/intel/issues/4083
>   [i915#5334]: https://gitlab.freedesktop.org/drm/intel/issues/5334
>   [i915#6077]: https://gitlab.freedesktop.org/drm/intel/issues/6077
>   [i915#6078]: https://gitlab.freedesktop.org/drm/intel/issues/6078
>   [i915#6093]: https://gitlab.freedesktop.org/drm/intel/issues/6093
>   [i915#6094]: https://gitlab.freedesktop.org/drm/intel/issues/6094
>   [i915#6166]: https://gitlab.freedesktop.org/drm/intel/issues/6166
>   [i915#6311]: https://gitlab.freedesktop.org/drm/intel/issues/6311
>   [i915#6434]: https://gitlab.freedesktop.org/drm/intel/issues/6434
>   [i915#6621]: https://gitlab.freedesktop.org/drm/intel/issues/6621
>   [i915#6645]: https://gitlab.freedesktop.org/drm/intel/issues/6645
>   [i915#6997]: https://gitlab.freedesktop.org/drm/intel/issues/6997
>   [i915#7346]: https://gitlab.freedesktop.org/drm/intel/issues/7346
>   [i915#7357]: https://gitlab.freedesktop.org/drm/intel/issues/7357
>   [i915#7359]: https://gitlab.freedesktop.org/drm/intel/issues/7359
>   [i915#7640]: https://gitlab.freedesktop.org/drm/intel/issues/7640
> 
> 
> Build changes
> -------------
> 
>   * CI: CI-20190529 -> None
>   * IGT: IGT_7079 -> IGTPW_8187
> 
>   CI-20190529: 20190529
>   CI_DRM_12462: 00b10bdfd8b9edc9b2c681d806fbb6ae2e5f31a3 @ git://anongit.freedesktop.org/gfx-ci/linux
>   IGTPW_8187: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8187/index.html
>   IGT_7079: 272354f8e1bbeb56df1663c42a9958f2ff9b8f54 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
> 
> 
> Testlist changes
> ----------------
> 
> +igt@gem_exec_balancer@parallel-dmabuf-import-out-fence
> 
> == Logs ==
> 
> For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8187/index.html

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

[Kamil Konieczny]

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

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

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

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

[Matthew Auld]

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

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

[Kamil Konieczny]

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@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?

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