[igt-dev] [PATCH i-g-t v3 1/3] lib/ioctl_wrapper: Add i915_get_param helper

[igt-dev] [PATCH i-g-t v3 1/3] lib/ioctl_wrapper: Add i915_get_param helper

[Lukasz Kalamarz]

getparam is used in few places across IGT, but no helper function
is used to reduce code duplication.
v2: Added doc part and changed return value in case of error

Signed-off-by: Lukasz Kalamarz <lukasz.kalamarz@intel.com>

Cc: Michal Winiarski <michal.winiarski@intel.com>
Cc: Katarzyna Dec <katarzyna.dec@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
---
 lib/ioctl_wrappers.c | 22 ++++++++++++++++++++++
 lib/ioctl_wrappers.h |  1 +
 2 files changed, 23 insertions(+)

diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
index 9f255508..aec4f15e 100644
--- a/lib/ioctl_wrappers.c
+++ b/lib/ioctl_wrappers.c
@@ -468,6 +468,28 @@ void gem_sync(int fd, uint32_t handle)
 	errno = 0;
 }
 
+/**
+ * drm_get_param:
+ * @fd: open i915 drm file descriptor
+ * @param: drm parameter we want to read
+ *
+ * Helper function that execute GETPARAM ioctl for a given parameter.
+ *
+ * Return: Read value from GETPARAM
+ */
+int drm_get_param(int fd, uint32_t param)
+{
+	int value;
+	drm_i915_getparam_t gp = {
+		.param = param,
+		.value = &value
+	};
+
+	if (igt_ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp) == -1)
+		return -EINVAL;
+
+	return value;
+}
 
 bool gem_create__has_stolen_support(int fd)
 {
diff --git a/lib/ioctl_wrappers.h b/lib/ioctl_wrappers.h
index b22b36b0..90174d13 100644
--- a/lib/ioctl_wrappers.h
+++ b/lib/ioctl_wrappers.h
@@ -74,6 +74,7 @@ int __gem_set_domain(int fd, uint32_t handle, uint32_t read, uint32_t write);
 void gem_set_domain(int fd, uint32_t handle, uint32_t read, uint32_t write);
 int gem_wait(int fd, uint32_t handle, int64_t *timeout_ns);
 void gem_sync(int fd, uint32_t handle);
+int drm_get_param(int fd, uint32_t param);
 bool gem_create__has_stolen_support(int fd);
 uint32_t __gem_create_stolen(int fd, uint64_t size);
 uint32_t gem_create_stolen(int fd, uint64_t size);
-- 
2.17.2

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] [PATCH i-g-t v3 2/3] tests: Use drm_get_param where applicable

[Lukasz Kalamarz]

Across several tests we check values of a given parameters.
With implementation of drm_get_param we can drop duplicated
lines and use helper function instead.
v2: Fixed errors in argument (fd are named as i915)

Signed-off-by: Lukasz Kalamarz <lukasz.kalamarz@intel.com>

Cc: Michal Winiarski <michal.winiarski@intel.com>
Cc: Katarzyna Dec <katarzyna.dec@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
---
 tests/i915/gem_busy.c          |  8 ++------
 tests/i915/gem_cs_tlb.c        | 10 ++--------
 tests/i915/gem_ctx_isolation.c |  7 +------
 tests/i915/gem_exec_async.c    |  6 ++----
 tests/i915/gem_exec_capture.c  |  5 +----
 tests/i915/gem_exec_fence.c    | 14 ++------------
 tests/i915/gem_exec_flush.c    |  5 +----
 tests/i915/gem_exec_params.c   | 16 ++++++----------
 tests/i915/gem_exec_parse.c    |  7 ++-----
 tests/i915/gem_exec_suspend.c  |  7 +------
 tests/i915/gem_mmap_gtt.c      |  7 ++-----
 tests/i915/gem_mmap_wc.c       |  7 +------
 tests/i915/hangman.c           |  5 +----
 tests/prime_vgem.c             |  7 ++-----
 14 files changed, 26 insertions(+), 85 deletions(-)

diff --git a/tests/i915/gem_busy.c b/tests/i915/gem_busy.c
index 76b44a5d..6cc8e45b 100644
--- a/tests/i915/gem_busy.c
+++ b/tests/i915/gem_busy.c
@@ -28,6 +28,7 @@
 #include "igt.h"
 #include "igt_rand.h"
 #include "igt_vgem.h"
+#include "ioctl_wrappers.h"
 #include "i915/gem_ring.h"
 
 #define LOCAL_EXEC_NO_RELOC (1<<11)
@@ -401,14 +402,9 @@ static void close_race(int fd)
 
 static bool has_semaphores(int fd)
 {
-	struct drm_i915_getparam gp;
 	int val = -1;
 
-	memset(&gp, 0, sizeof(gp));
-	gp.param = I915_PARAM_HAS_SEMAPHORES;
-	gp.value = &val;
-
-	drmIoctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
+	val = drm_get_param(fd, I915_PARAM_HAS_SEMAPHORES);
 	errno = 0;
 
 	return val > 0;
diff --git a/tests/i915/gem_cs_tlb.c b/tests/i915/gem_cs_tlb.c
index 51e1c4e1..891cd551 100644
--- a/tests/i915/gem_cs_tlb.c
+++ b/tests/i915/gem_cs_tlb.c
@@ -58,17 +58,11 @@ IGT_TEST_DESCRIPTION("Check whether we correctly invalidate the cs tlb.");
 
 static bool has_softpin(int fd)
 {
-	struct drm_i915_getparam gp;
 	int val = 0;
 
-	memset(&gp, 0, sizeof(gp));
-	gp.param = 37; /* I915_PARAM_HAS_EXEC_SOFTPIN */
-	gp.value = &val;
-
-	if (drmIoctl(fd, DRM_IOCTL_I915_GETPARAM, &gp))
-		return 0;
-
+	val = drm_get_param(fd, 37); /* I915_PARAM_HAS_EXEC_SOFTPIN */
 	errno = 0;
+
 	return (val == 1);
 }
 
diff --git a/tests/i915/gem_ctx_isolation.c b/tests/i915/gem_ctx_isolation.c
index 058cf3ec..27c8429d 100644
--- a/tests/i915/gem_ctx_isolation.c
+++ b/tests/i915/gem_ctx_isolation.c
@@ -671,14 +671,9 @@ static void preservation(int fd,
 
 static unsigned int __has_context_isolation(int fd)
 {
-	struct drm_i915_getparam gp;
 	int value = 0;
 
-	memset(&gp, 0, sizeof(gp));
-	gp.param = 50; /* I915_PARAM_HAS_CONTEXT_ISOLATION */
-	gp.value = &value;
-
-	igt_ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
+	value = drm_get_param(fd, 50); /* I915_PARAM_HAS_CONTEXT_ISOLATION */
 	errno = 0;
 
 	return value;
diff --git a/tests/i915/gem_exec_async.c b/tests/i915/gem_exec_async.c
index 9a06af7e..6b5a5d25 100644
--- a/tests/i915/gem_exec_async.c
+++ b/tests/i915/gem_exec_async.c
@@ -22,6 +22,7 @@
  */
 
 #include "igt.h"
+#include "ioctl_wrappers.h"
 
 #define LOCAL_OBJECT_ASYNC (1 << 6)
 #define LOCAL_PARAM_HAS_EXEC_ASYNC 43
@@ -173,12 +174,9 @@ static void one(int fd, unsigned ring, uint32_t flags)
 
 static bool has_async_execbuf(int fd)
 {
-	drm_i915_getparam_t gp;
 	int async = -1;
 
-	gp.param = LOCAL_PARAM_HAS_EXEC_ASYNC;
-	gp.value = &async;
-	drmIoctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
+	async = drm_get_param(fd, LOCAL_PARAM_HAS_EXEC_ASYNC);
 
 	return async > 0;
 }
diff --git a/tests/i915/gem_exec_capture.c b/tests/i915/gem_exec_capture.c
index 3e4a4377..72cf8ef4 100644
--- a/tests/i915/gem_exec_capture.c
+++ b/tests/i915/gem_exec_capture.c
@@ -189,12 +189,9 @@ static void userptr(int fd, int dir)
 
 static bool has_capture(int fd)
 {
-	drm_i915_getparam_t gp;
 	int async = -1;
 
-	gp.param = LOCAL_PARAM_HAS_EXEC_CAPTURE;
-	gp.value = &async;
-	drmIoctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
+	async = drm_get_param(fd, LOCAL_PARAM_HAS_EXEC_CAPTURE);
 
 	return async > 0;
 }
diff --git a/tests/i915/gem_exec_fence.c b/tests/i915/gem_exec_fence.c
index ba46595d..ee143a5f 100644
--- a/tests/i915/gem_exec_fence.c
+++ b/tests/i915/gem_exec_fence.c
@@ -803,14 +803,9 @@ static void test_fence_flip(int i915)
 
 static bool has_submit_fence(int fd)
 {
-	struct drm_i915_getparam gp;
 	int value = 0;
 
-	memset(&gp, 0, sizeof(gp));
-	gp.param = 0xdeadbeef ^ 51; /* I915_PARAM_HAS_EXEC_SUBMIT_FENCE */
-	gp.value = &value;
-
-	ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp, sizeof(gp));
+	value = drm_get_param(fd, 0xdeadbeef ^ 51); /* I915_PARAM_HAS_EXEC_SUBMIT_FENCE */
 	errno = 0;
 
 	return value;
@@ -825,14 +820,9 @@ static bool has_syncobj(int fd)
 
 static bool exec_has_fence_array(int fd)
 {
-	struct drm_i915_getparam gp;
 	int value = 0;
 
-	memset(&gp, 0, sizeof(gp));
-	gp.param = 49; /* I915_PARAM_HAS_EXEC_FENCE_ARRAY */
-	gp.value = &value;
-
-	ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp, sizeof(gp));
+	value = drm_get_param(fd, 49); /* I915_PARAM_HAS_EXEC_FENCE_ARRAY */
 	errno = 0;
 
 	return value;
diff --git a/tests/i915/gem_exec_flush.c b/tests/i915/gem_exec_flush.c
index f820b2a8..bdc9257d 100644
--- a/tests/i915/gem_exec_flush.c
+++ b/tests/i915/gem_exec_flush.c
@@ -359,11 +359,8 @@ static void batch(int fd, unsigned ring, int nchild, int timeout,
 
 	if (flags & CMDPARSER) {
 		int cmdparser = -1;
-                drm_i915_getparam_t gp;
 
-		gp.param = I915_PARAM_CMD_PARSER_VERSION;
-		gp.value = &cmdparser;
-		drmIoctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
+		cmdparser = drm_get_param(fd, I915_PARAM_CMD_PARSER_VERSION);
 		igt_require(cmdparser > 0);
 	}
 
diff --git a/tests/i915/gem_exec_params.c b/tests/i915/gem_exec_params.c
index 49c56a8d..5f443961 100644
--- a/tests/i915/gem_exec_params.c
+++ b/tests/i915/gem_exec_params.c
@@ -79,22 +79,18 @@ static bool has_ring(int fd, unsigned ring_exec_flags)
 static bool has_exec_batch_first(int fd)
 {
 	int val = -1;
-	struct drm_i915_getparam gp = {
-		.param = 48,
-		.value = &val,
-	};
-	ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
+
+	val = drm_get_param(fd, 48);
+
 	return val > 0;
 }
 
 static bool has_resource_streamer(int fd)
 {
 	int val = -1;
-	struct drm_i915_getparam gp = {
-		.param = I915_PARAM_HAS_RESOURCE_STREAMER,
-		.value = &val,
-	};
-	ioctl(fd, DRM_IOCTL_I915_GETPARAM , &gp);
+
+	val = drm_get_param(fd, I915_PARAM_HAS_RESOURCE_STREAMER);
+
 	return val > 0;
 }
 
diff --git a/tests/i915/gem_exec_parse.c b/tests/i915/gem_exec_parse.c
index b653b1bd..15315438 100644
--- a/tests/i915/gem_exec_parse.c
+++ b/tests/i915/gem_exec_parse.c
@@ -61,12 +61,9 @@ static int parser_version;
 static int command_parser_version(int fd)
 {
 	int version = -1;
-	drm_i915_getparam_t gp;
 
-	gp.param = I915_PARAM_CMD_PARSER_VERSION;
-	gp.value = &version;
-
-	if (drmIoctl(fd, DRM_IOCTL_I915_GETPARAM, &gp) == 0)
+	version = drm_get_param(fd, I915_PARAM_CMD_PARSER_VERSION);
+	if (version >= 1)
 		return version;
 
 	return -1;
diff --git a/tests/i915/gem_exec_suspend.c b/tests/i915/gem_exec_suspend.c
index 43c52d10..b44af299 100644
--- a/tests/i915/gem_exec_suspend.c
+++ b/tests/i915/gem_exec_suspend.c
@@ -73,14 +73,9 @@ static void test_all(int fd, unsigned flags)
 
 static bool has_semaphores(int fd)
 {
-	struct drm_i915_getparam gp;
 	int val = -1;
 
-	memset(&gp, 0, sizeof(gp));
-	gp.param = I915_PARAM_HAS_SEMAPHORES;
-	gp.value = &val;
-
-	drmIoctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
+	val = drm_get_param(fd, I915_PARAM_HAS_SEMAPHORES);
 	errno = 0;
 
 	return val > 0;
diff --git a/tests/i915/gem_mmap_gtt.c b/tests/i915/gem_mmap_gtt.c
index f6353555..396694e6 100644
--- a/tests/i915/gem_mmap_gtt.c
+++ b/tests/i915/gem_mmap_gtt.c
@@ -312,12 +312,9 @@ test_write_gtt(int fd)
 static bool is_coherent(int i915)
 {
 	int val = 1; /* by default, we assume GTT is coherent, hence the test */
-	struct drm_i915_getparam gp = {
-		gp.param = 52, /* GTT_COHERENT */
-		gp.value = &val,
-	};
 
-	ioctl(i915, DRM_IOCTL_I915_GETPARAM, &gp);
+	val = drm_get_param(i915, 52); /* GTT_COHERENT */
+
 	return val;
 }
 
diff --git a/tests/i915/gem_mmap_wc.c b/tests/i915/gem_mmap_wc.c
index 110883eb..73b2166b 100644
--- a/tests/i915/gem_mmap_wc.c
+++ b/tests/i915/gem_mmap_wc.c
@@ -85,7 +85,6 @@ create_pointer(int fd)
 static void
 test_invalid_flags(int fd)
 {
-	struct drm_i915_getparam gp;
 	struct local_i915_gem_mmap_v2 arg;
 	uint64_t flag = I915_MMAP_WC;
 	int val = -1;
@@ -95,12 +94,8 @@ test_invalid_flags(int fd)
 	arg.offset = 0;
 	arg.size = 4096;
 
-	memset(&gp, 0, sizeof(gp));
-	gp.param = 30; /* MMAP_VERSION */
-	gp.value = &val;
-
 	/* Do we have the new mmap_ioctl? */
-	drmIoctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
+	val = drm_get_param(fd, 30); /* MMAP_VERSION */
 
 	if (val >= 1) {
 		/*
diff --git a/tests/i915/hangman.c b/tests/i915/hangman.c
index 6ddae491..09316226 100644
--- a/tests/i915/hangman.c
+++ b/tests/i915/hangman.c
@@ -112,11 +112,8 @@ static FILE *open_error(void)
 static bool uses_cmd_parser(void)
 {
 	int parser_version = 0;
-	drm_i915_getparam_t gp;
 
-	gp.param = I915_PARAM_CMD_PARSER_VERSION;
-	gp.value = &parser_version;
-	drmIoctl(device, DRM_IOCTL_I915_GETPARAM, &gp);
+	parser_version = drm_get_param(device, I915_PARAM_CMD_PARSER_VERSION);
 
 	return parser_version > 0;
 }
diff --git a/tests/prime_vgem.c b/tests/prime_vgem.c
index 60bb951c..759299da 100644
--- a/tests/prime_vgem.c
+++ b/tests/prime_vgem.c
@@ -266,12 +266,9 @@ static void test_shrink(int vgem, int i915)
 static bool is_coherent(int i915)
 {
 	int val = 1; /* by default, we assume GTT is coherent, hence the test */
-	struct drm_i915_getparam gp = {
-		gp.param = 52, /* GTT_COHERENT */
-		gp.value = &val,
-	};
 
-	ioctl(i915, DRM_IOCTL_I915_GETPARAM, &gp);
+	val = drm_get_param(i915, 52); /* GTT_COHERENT */
+
 	return val;
 }
 
-- 
2.17.2

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] [PATCH i-g-t v3 3/3] lib: Use drm_get_param where it is applicable

[Lukasz Kalamarz]

With usage of implemented drm_get_param helper function, we can
remove some duplicated code lines across few libs.

v2: Fix typos and missing include
v3: Drop unnecessary getparam structure in one function

Signed-off-by: Lukasz Kalamarz <lukasz.kalamarz@intel.com>

Cc: Michal Winiarski <michal.winiarski@intel.com>
Cc: Katarzyna Dec <katarzyna.dec@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
---
 lib/drmtest.c             |  7 ++---
 lib/i915/gem_scheduler.c  |  8 +----
 lib/i915/gem_submission.c | 10 +++---
 lib/igt_gt.c              |  7 ++---
 lib/intel_chipset.c       |  7 ++---
 lib/ioctl_wrappers.c      | 66 ++++++---------------------------------
 6 files changed, 22 insertions(+), 83 deletions(-)

diff --git a/lib/drmtest.c b/lib/drmtest.c
index fee9d33a..81383fd4 100644
--- a/lib/drmtest.c
+++ b/lib/drmtest.c
@@ -107,14 +107,11 @@ bool is_i915_device(int fd)
 
 static bool has_known_intel_chipset(int fd)
 {
-	struct drm_i915_getparam gp;
 	int devid = 0;
 
-	memset(&gp, 0, sizeof(gp));
-	gp.param = I915_PARAM_CHIPSET_ID;
-	gp.value = &devid;
+	devid = drm_get_param(fd, I915_PARAM_CHIPSET_ID);
 
-	if (ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp, sizeof(gp)))
+	if (devid <= 0)
 		return false;
 
 	if (!intel_gen(devid))
diff --git a/lib/i915/gem_scheduler.c b/lib/i915/gem_scheduler.c
index ad156306..079559a2 100644
--- a/lib/i915/gem_scheduler.c
+++ b/lib/i915/gem_scheduler.c
@@ -52,14 +52,8 @@ unsigned gem_scheduler_capability(int fd)
 	static int caps = -1;
 
 	if (caps < 0) {
-		struct drm_i915_getparam gp;
-
-		memset(&gp, 0, sizeof(gp));
-		gp.param = LOCAL_I915_PARAM_HAS_SCHEDULER;
-		gp.value = &caps;
-
 		caps = 0;
-		ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp, sizeof(gp));
+		caps = drm_get_param(fd, LOCAL_I915_PARAM_HAS_SCHEDULER);
 		errno = 0;
 	}
 
diff --git a/lib/i915/gem_submission.c b/lib/i915/gem_submission.c
index 2fd460d5..583bb133 100644
--- a/lib/i915/gem_submission.c
+++ b/lib/i915/gem_submission.c
@@ -53,12 +53,12 @@
 static bool has_semaphores(int fd, int dir)
 {
 	int val = 0;
-	struct drm_i915_getparam gp = {
-		gp.param = I915_PARAM_HAS_SEMAPHORES,
-		gp.value = &val,
-	};
-	if (ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp) < 0)
+
+	val = drm_get_param(fd, I915_PARAM_HAS_SEMAPHORES);
+
+	if (val < 0)
 		val = igt_sysfs_get_boolean(dir, "semaphores");
+
 	return val;
 }
 
diff --git a/lib/igt_gt.c b/lib/igt_gt.c
index a2061924..18cd922b 100644
--- a/lib/igt_gt.c
+++ b/lib/igt_gt.c
@@ -57,14 +57,11 @@ static bool has_gpu_reset(int fd)
 {
 	static int once = -1;
 	if (once < 0) {
-		struct drm_i915_getparam gp;
 		int val = 0;
 
-		memset(&gp, 0, sizeof(gp));
-		gp.param = 35; /* HAS_GPU_RESET */
-		gp.value = &val;
+		val = drm_get_param(fd, 35); /* HAS_GPU_RESET */
 
-		if (ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp))
+		if (val > 0)
 			once = intel_gen(intel_get_drm_devid(fd)) >= 5;
 		else
 			once = val > 0;
diff --git a/lib/intel_chipset.c b/lib/intel_chipset.c
index 4748a3fb..da354fb6 100644
--- a/lib/intel_chipset.c
+++ b/lib/intel_chipset.c
@@ -41,6 +41,7 @@
 #include "drmtest.h"
 #include "intel_chipset.h"
 #include "igt_core.h"
+#include "ioctl_wrappers.h"
 
 /**
  * SECTION:intel_chipset
@@ -125,7 +126,6 @@ intel_get_pci_device(void)
 uint32_t
 intel_get_drm_devid(int fd)
 {
-	struct drm_i915_getparam gp;
 	const char *override;
 	int devid = 0;
 
@@ -135,10 +135,7 @@ intel_get_drm_devid(int fd)
 	if (override)
 		return strtol(override, NULL, 0);
 
-	memset(&gp, 0, sizeof(gp));
-	gp.param = I915_PARAM_CHIPSET_ID;
-	gp.value = &devid;
-	ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp, sizeof(gp));
+	devid = drm_get_param(fd, I915_PARAM_CHIPSET_ID);
 
 	return devid;
 }
diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
index aec4f15e..d156103b 100644
--- a/lib/ioctl_wrappers.c
+++ b/lib/ioctl_wrappers.c
@@ -494,16 +494,12 @@ int drm_get_param(int fd, uint32_t param)
 bool gem_create__has_stolen_support(int fd)
 {
 	static int has_stolen_support = -1;
-	struct drm_i915_getparam gp;
 	int val = -1;
 
 	if (has_stolen_support < 0) {
-		memset(&gp, 0, sizeof(gp));
-		gp.param = 38; /* CREATE_VERSION */
-		gp.value = &val;
+		val = drm_get_param(fd, 38); /* CREATE_VERSION */
 
 		/* Do we have the extended gem_create_ioctl? */
-		ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
 		has_stolen_support = val >= 2;
 	}
 
@@ -723,21 +719,13 @@ bool gem_mmap__has_wc(int fd)
 	static int has_wc = -1;
 
 	if (has_wc == -1) {
-		struct drm_i915_getparam gp;
 		int mmap_version = -1;
 		int gtt_version = -1;
 
 		has_wc = 0;
 
-		memset(&gp, 0, sizeof(gp));
-		gp.param = 40; /* MMAP_GTT_VERSION */
-		gp.value = &gtt_version;
-		ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
-
-		memset(&gp, 0, sizeof(gp));
-		gp.param = 30; /* MMAP_VERSION */
-		gp.value = &mmap_version;
-		ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
+		gtt_version = drm_get_param(fd, 40); /* MMAP_GTT_VERSION */
+		mmap_version = drm_get_param(fd, 30); /* MMAP_VERSION */
 
 		/* Do we have the new mmap_ioctl with DOMAIN_WC? */
 		if (mmap_version >= 1 && gtt_version >= 2) {
@@ -982,17 +970,11 @@ bool gem_bo_busy(int fd, uint32_t handle)
  */
 static int gem_gtt_type(int fd)
 {
-	struct drm_i915_getparam gp;
 	int val = 0;
 
-	memset(&gp, 0, sizeof(gp));
-	gp.param = 18; /* HAS_ALIASING_PPGTT */
-	gp.value = &val;
-
-	if (ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp, sizeof(gp)))
-		return 0;
-
+	val = drm_get_param(fd, 18); /* HAS_ALIASING_PPGTT */
 	errno = 0;
+
 	return val;
 }
 
@@ -1036,13 +1018,9 @@ bool gem_uses_full_ppgtt(int fd)
  */
 int gem_gpu_reset_type(int fd)
 {
-	struct drm_i915_getparam gp;
 	int gpu_reset_type = -1;
 
-	memset(&gp, 0, sizeof(gp));
-	gp.param = I915_PARAM_HAS_GPU_RESET;
-	gp.value = &gpu_reset_type;
-	drmIoctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
+	gpu_reset_type = drm_get_param(fd, I915_PARAM_HAS_GPU_RESET);
 
 	return gpu_reset_type;
 }
@@ -1090,14 +1068,8 @@ int gem_available_fences(int fd)
 	static int num_fences = -1;
 
 	if (num_fences < 0) {
-		struct drm_i915_getparam gp;
-
-		memset(&gp, 0, sizeof(gp));
-		gp.param = I915_PARAM_NUM_FENCES_AVAIL;
-		gp.value = &num_fences;
-
 		num_fences = 0;
-		ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp, sizeof(gp));
+		num_fences = drm_get_param(fd, I915_PARAM_NUM_FENCES_AVAIL);
 		errno = 0;
 	}
 
@@ -1109,14 +1081,8 @@ bool gem_has_llc(int fd)
 	static int has_llc = -1;
 
 	if (has_llc < 0) {
-		struct drm_i915_getparam gp;
-
-		memset(&gp, 0, sizeof(gp));
-		gp.param = I915_PARAM_HAS_LLC;
-		gp.value = &has_llc;
-
 		has_llc = 0;
-		ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp, sizeof(gp));
+		has_llc = drm_get_param(fd, I915_PARAM_HAS_LLC);
 		errno = 0;
 	}
 
@@ -1366,14 +1332,8 @@ bool gem_has_softpin(int fd)
 	static int has_softpin = -1;
 
 	if (has_softpin < 0) {
-		struct drm_i915_getparam gp;
-
-		memset(&gp, 0, sizeof(gp));
-		gp.param = I915_PARAM_HAS_EXEC_SOFTPIN;
-		gp.value = &has_softpin;
-
 		has_softpin = 0;
-		ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp, sizeof(gp));
+		has_softpin = drm_get_param(fd, I915_PARAM_HAS_EXEC_SOFTPIN);
 		errno = 0;
 	}
 
@@ -1394,14 +1354,8 @@ bool gem_has_exec_fence(int fd)
 	static int has_exec_fence = -1;
 
 	if (has_exec_fence < 0) {
-		struct drm_i915_getparam gp;
-
-		memset(&gp, 0, sizeof(gp));
-		gp.param = I915_PARAM_HAS_EXEC_FENCE;
-		gp.value = &has_exec_fence;
-
 		has_exec_fence = 0;
-		ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp, sizeof(gp));
+		has_exec_fence = drm_get_param(fd, I915_PARAM_HAS_EXEC_FENCE);
 		errno = 0;
 	}
 
-- 
2.17.2

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] ✗ Fi.CI.BAT: failure for series starting with [i-g-t,v3,1/3] lib/ioctl_wrapper: Add i915_get_param helper

[Patchwork]

== Series Details ==

Series: series starting with [i-g-t,v3,1/3] lib/ioctl_wrapper: Add i915_get_param helper
URL   : https://patchwork.freedesktop.org/series/53019/
State : failure

== Summary ==

IGT patchset build failed on latest successful build
de775f3a15daebcf191562fa0984f49497b76fd0 tests: close(fd) without igt_fixture considered harmful

[13/750] Linking static target lib/libigt-igt_color_encoding_c.a.
[14/750] Compiling C object 'lib/igt-igt_debugfs_c@sta/igt_debugfs.c.o'.
[15/750] Linking static target lib/libigt-igt_debugfs_c.a.
[16/750] Compiling C object 'lib/igt-igt_device_c@sta/igt_device.c.o'.
[17/750] Linking static target lib/libigt-igt_device_c.a.
[18/750] Compiling C object 'lib/igt-igt_aux_c@sta/igt_aux.c.o'.
[19/750] Linking static target lib/libigt-igt_aux_c.a.
[20/750] Compiling C object 'lib/igt-igt_gt_c@sta/igt_gt.c.o'.
[21/750] Linking static target lib/libigt-igt_gt_c.a.
[22/750] Compiling C object 'lib/igt-igt_gvt_c@sta/igt_gvt.c.o'.
[23/750] Linking static target lib/libigt-igt_gvt_c.a.
[24/750] Compiling C object 'lib/igt-igt_matrix_c@sta/igt_matrix.c.o'.
[25/750] Linking static target lib/libigt-igt_matrix_c.a.
[26/750] Compiling C object 'lib/igt-igt_primes_c@sta/igt_primes.c.o'.
[27/750] Linking static target lib/libigt-igt_primes_c.a.
[28/750] Compiling C object 'lib/igt-igt_rand_c@sta/igt_rand.c.o'.
[29/750] Linking static target lib/libigt-igt_rand_c.a.
[30/750] Compiling C object 'lib/igt-igt_stats_c@sta/igt_stats.c.o'.
[31/750] Linking static target lib/libigt-igt_stats_c.a.
[32/750] Compiling C object 'lib/igt-igt_sysfs_c@sta/igt_sysfs.c.o'.
[33/750] Linking static target lib/libigt-igt_sysfs_c.a.
[34/750] Compiling C object 'lib/igt-igt_syncobj_c@sta/igt_syncobj.c.o'.
[35/750] Linking static target lib/libigt-igt_syncobj_c.a.
[36/750] Compiling C object 'lib/igt-igt_sysrq_c@sta/igt_sysrq.c.o'.
[37/750] Linking static target lib/libigt-igt_sysrq_c.a.
[38/750] Compiling C object 'lib/igt-igt_vgem_c@sta/igt_vgem.c.o'.
[39/750] Linking static target lib/libigt-igt_vgem_c.a.
[40/750] Compiling C object 'lib/igt-igt_x86_c@sta/igt_x86.c.o'.
[41/750] Linking static target lib/libigt-igt_x86_c.a.
[42/750] Compiling C object 'lib/igt-instdone_c@sta/instdone.c.o'.
[43/750] Linking static target lib/libigt-instdone_c.a.
[44/750] Compiling C object 'lib/igt-intel_batchbuffer_c@sta/intel_batchbuffer.c.o'.
[45/750] Linking static target lib/libigt-intel_batchbuffer_c.a.
[46/750] Compiling C object 'lib/igt-intel_os_c@sta/intel_os.c.o'.
[47/750] Linking static target lib/libigt-intel_os_c.a.
[48/750] Compiling C object 'lib/igt-intel_device_info_c@sta/intel_device_info.c.o'.
[49/750] Compiling C object 'lib/igt-intel_chipset_c@sta/intel_chipset.c.o'.
[50/750] Linking static target lib/libigt-intel_chipset_c.a.
[51/750] Linking static target lib/libigt-intel_device_info_c.a.
[52/750] Compiling C object 'lib/igt-intel_mmio_c@sta/intel_mmio.c.o'.
[53/750] Linking static target lib/libigt-intel_mmio_c.a.
[54/750] Compiling C object 'lib/igt_chipset@sta/intel_chipset.c.o'.
FAILED: lib/igt_chipset@sta/intel_chipset.c.o 
ccache cc  -Ilib/igt_chipset@sta -Ilib -I../lib -I. -I../ -I../lib/stubs/syscalls -I../include/drm-uapi -I/home/cidrm/kernel_headers/include -fdiagnostics-color=always -pipe -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wextra -std=gnu11 -O0 -g -D_GNU_SOURCE -include config.h -Wbad-function-cast -Wdeclaration-after-statement -Wformat=2 -Wimplicit-fallthrough=0 -Wlogical-op -Wmissing-declarations -Wmissing-format-attribute -Wmissing-noreturn -Wmissing-prototypes -Wnested-externs -Wold-style-definition -Wpointer-arith -Wredundant-decls -Wshadow -Wstrict-prototypes -Wuninitialized -Wunused -Wno-clobbered -Wno-maybe-uninitialized -Wno-missing-field-initializers -Wno-pointer-arith -Wno-sign-compare -Wno-type-limits -Wno-unused-parameter -Wno-unused-result -Werror=address -Werror=array-bounds -Werror=implicit -Werror=init-self -Werror=int-to-pointer-cast -Werror=main -Werror=missing-braces -Werror=nonnull -Werror=pointer-to-int-cast -Werror=return-type -Werror=sequence-point -Werror=trigraphs -Werror=write-strings -fPIC -MD -MQ 'lib/igt_chipset@sta/intel_chipset.c.o' -MF 'lib/igt_chipset@sta/intel_chipset.c.o.d' -o 'lib/igt_chipset@sta/intel_chipset.c.o' -c ../lib/intel_chipset.c
In file included from ../lib/intel_chipset.c:44:0:
../lib/ioctl_wrappers.h:36:10: fatal error: intel_bufmgr.h: No such file or directory
 #include <intel_bufmgr.h>
          ^~~~~~~~~~~~~~~~
compilation terminated.
ninja: build stopped: subcommand failed.

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t v3 1/3] lib/ioctl_wrapper: Add i915_get_param helper

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 1313 bytes --]

Lukasz Kalamarz <lukasz.kalamarz@intel.com> writes:

> getparam is used in few places across IGT, but no helper function
> is used to reduce code duplication.
> v2: Added doc part and changed return value in case of error
>
> Signed-off-by: Lukasz Kalamarz <lukasz.kalamarz@intel.com>
>
> Cc: Michal Winiarski <michal.winiarski@intel.com>
> Cc: Katarzyna Dec <katarzyna.dec@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> ---
>  lib/ioctl_wrappers.c | 22 ++++++++++++++++++++++
>  lib/ioctl_wrappers.h |  1 +
>  2 files changed, 23 insertions(+)
>
> diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
> index 9f255508..aec4f15e 100644
> --- a/lib/ioctl_wrappers.c
> +++ b/lib/ioctl_wrappers.c
> @@ -468,6 +468,28 @@ void gem_sync(int fd, uint32_t handle)
>  	errno = 0;
>  }
>  
> +/**
> + * drm_get_param:
> + * @fd: open i915 drm file descriptor
> + * @param: drm parameter we want to read

Could we please call this i915_get_param, to match the commit message,
instead of drm_get_param()?  I know igt used to call i915-only ioctls
drm_* or gem_*, but it would be nice to see new functions use a more
appropriate namespace.

(I wouldn't block this, but if people decide not to do that then the
commit message subject should at least change)

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t v3 2/3] tests: Use drm_get_param where applicable

[Antonio Argenziano]

On 26/11/18 07:36, Lukasz Kalamarz wrote:
> Across several tests we check values of a given parameters.
> With implementation of drm_get_param we can drop duplicated
> lines and use helper function instead.
> v2: Fixed errors in argument (fd are named as i915)
> 
> Signed-off-by: Lukasz Kalamarz <lukasz.kalamarz@intel.com>
> 
> Cc: Michal Winiarski <michal.winiarski@intel.com>
> Cc: Katarzyna Dec <katarzyna.dec@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> ---
>   tests/i915/gem_busy.c          |  8 ++------
>   tests/i915/gem_cs_tlb.c        | 10 ++--------
>   tests/i915/gem_ctx_isolation.c |  7 +------
>   tests/i915/gem_exec_async.c    |  6 ++----
>   tests/i915/gem_exec_capture.c  |  5 +----
>   tests/i915/gem_exec_fence.c    | 14 ++------------
>   tests/i915/gem_exec_flush.c    |  5 +----
>   tests/i915/gem_exec_params.c   | 16 ++++++----------
>   tests/i915/gem_exec_parse.c    |  7 ++-----
>   tests/i915/gem_exec_suspend.c  |  7 +------
>   tests/i915/gem_mmap_gtt.c      |  7 ++-----
>   tests/i915/gem_mmap_wc.c       |  7 +------
>   tests/i915/hangman.c           |  5 +----
>   tests/prime_vgem.c             |  7 ++-----
>   14 files changed, 26 insertions(+), 85 deletions(-)
> 
> diff --git a/tests/i915/gem_busy.c b/tests/i915/gem_busy.c
> index 76b44a5d..6cc8e45b 100644
> --- a/tests/i915/gem_busy.c
> +++ b/tests/i915/gem_busy.c
> @@ -28,6 +28,7 @@
>   #include "igt.h"
>   #include "igt_rand.h"
>   #include "igt_vgem.h"
> +#include "ioctl_wrappers.h"
>   #include "i915/gem_ring.h"
>   
>   #define LOCAL_EXEC_NO_RELOC (1<<11)
> @@ -401,14 +402,9 @@ static void close_race(int fd)
>   
>   static bool has_semaphores(int fd)
>   {
> -	struct drm_i915_getparam gp;
>   	int val = -1;
>   
> -	memset(&gp, 0, sizeof(gp));
> -	gp.param = I915_PARAM_HAS_SEMAPHORES;
> -	gp.value = &val;
> -
> -	drmIoctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
> +	val = drm_get_param(fd, I915_PARAM_HAS_SEMAPHORES);
>   	errno = 0;
>   
>   	return val > 0;
> diff --git a/tests/i915/gem_cs_tlb.c b/tests/i915/gem_cs_tlb.c
> index 51e1c4e1..891cd551 100644
> --- a/tests/i915/gem_cs_tlb.c
> +++ b/tests/i915/gem_cs_tlb.c
> @@ -58,17 +58,11 @@ IGT_TEST_DESCRIPTION("Check whether we correctly invalidate the cs tlb.");
>   
>   static bool has_softpin(int fd)
>   {
> -	struct drm_i915_getparam gp;
>   	int val = 0;
>   
> -	memset(&gp, 0, sizeof(gp));
> -	gp.param = 37; /* I915_PARAM_HAS_EXEC_SOFTPIN */
> -	gp.value = &val;
> -
> -	if (drmIoctl(fd, DRM_IOCTL_I915_GETPARAM, &gp))
> -		return 0;
> -
> +	val = drm_get_param(fd, 37); /* I915_PARAM_HAS_EXEC_SOFTPIN */

Since you are here, this param is now defined in i915_drm.h use the 
macro defined there. There are a few more cases below as well.

>   	errno = 0;
> +
>   	return (val == 1);
>   }
>   
> diff --git a/tests/i915/gem_ctx_isolation.c b/tests/i915/gem_ctx_isolation.c
> index 058cf3ec..27c8429d 100644
> --- a/tests/i915/gem_ctx_isolation.c
> +++ b/tests/i915/gem_ctx_isolation.c
> @@ -671,14 +671,9 @@ static void preservation(int fd,
>   
>   static unsigned int __has_context_isolation(int fd)
>   {
> -	struct drm_i915_getparam gp;
>   	int value = 0;
>   
> -	memset(&gp, 0, sizeof(gp));
> -	gp.param = 50; /* I915_PARAM_HAS_CONTEXT_ISOLATION */
> -	gp.value = &value;
> -
> -	igt_ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
> +	value = drm_get_param(fd, 50); /* I915_PARAM_HAS_CONTEXT_ISOLATION */

here^

>   	errno = 0;
>   
>   	return value;
> diff --git a/tests/i915/gem_exec_async.c b/tests/i915/gem_exec_async.c
> index 9a06af7e..6b5a5d25 100644
> --- a/tests/i915/gem_exec_async.c
> +++ b/tests/i915/gem_exec_async.c
> @@ -22,6 +22,7 @@
>    */
>   
>   #include "igt.h"
> +#include "ioctl_wrappers.h"
>   
>   #define LOCAL_OBJECT_ASYNC (1 << 6)
>   #define LOCAL_PARAM_HAS_EXEC_ASYNC 43
> @@ -173,12 +174,9 @@ static void one(int fd, unsigned ring, uint32_t flags)
>   
>   static bool has_async_execbuf(int fd)
>   {
> -	drm_i915_getparam_t gp;
>   	int async = -1;
>   
> -	gp.param = LOCAL_PARAM_HAS_EXEC_ASYNC;
> -	gp.value = &async;
> -	drmIoctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
> +	async = drm_get_param(fd, LOCAL_PARAM_HAS_EXEC_ASYNC);
>   
>   	return async > 0;
>   }
> diff --git a/tests/i915/gem_exec_capture.c b/tests/i915/gem_exec_capture.c
> index 3e4a4377..72cf8ef4 100644
> --- a/tests/i915/gem_exec_capture.c
> +++ b/tests/i915/gem_exec_capture.c
> @@ -189,12 +189,9 @@ static void userptr(int fd, int dir)
>   
>   static bool has_capture(int fd)
>   {
> -	drm_i915_getparam_t gp;
>   	int async = -1;
>   
> -	gp.param = LOCAL_PARAM_HAS_EXEC_CAPTURE;
> -	gp.value = &async;
> -	drmIoctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
> +	async = drm_get_param(fd, LOCAL_PARAM_HAS_EXEC_CAPTURE);
>   
>   	return async > 0;
>   }
> diff --git a/tests/i915/gem_exec_fence.c b/tests/i915/gem_exec_fence.c
> index ba46595d..ee143a5f 100644
> --- a/tests/i915/gem_exec_fence.c
> +++ b/tests/i915/gem_exec_fence.c
> @@ -803,14 +803,9 @@ static void test_fence_flip(int i915)
>   
>   static bool has_submit_fence(int fd)
>   {
> -	struct drm_i915_getparam gp;
>   	int value = 0;
>   
> -	memset(&gp, 0, sizeof(gp));
> -	gp.param = 0xdeadbeef ^ 51; /* I915_PARAM_HAS_EXEC_SUBMIT_FENCE */
> -	gp.value = &value;
> -
> -	ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp, sizeof(gp));
> +	value = drm_get_param(fd, 0xdeadbeef ^ 51); /* I915_PARAM_HAS_EXEC_SUBMIT_FENCE */
>   	errno = 0;
>   
>   	return value;
> @@ -825,14 +820,9 @@ static bool has_syncobj(int fd)
>   
>   static bool exec_has_fence_array(int fd)
>   {
> -	struct drm_i915_getparam gp;
>   	int value = 0;
>   
> -	memset(&gp, 0, sizeof(gp));
> -	gp.param = 49; /* I915_PARAM_HAS_EXEC_FENCE_ARRAY */
> -	gp.value = &value;
> -
> -	ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp, sizeof(gp));
> +	value = drm_get_param(fd, 49); /* I915_PARAM_HAS_EXEC_FENCE_ARRAY */

Here^

>   	errno = 0;
>   
>   	return value;
> diff --git a/tests/i915/gem_exec_flush.c b/tests/i915/gem_exec_flush.c
> index f820b2a8..bdc9257d 100644
> --- a/tests/i915/gem_exec_flush.c
> +++ b/tests/i915/gem_exec_flush.c
> @@ -359,11 +359,8 @@ static void batch(int fd, unsigned ring, int nchild, int timeout,
>   
>   	if (flags & CMDPARSER) {
>   		int cmdparser = -1;
> -                drm_i915_getparam_t gp;
>   
> -		gp.param = I915_PARAM_CMD_PARSER_VERSION;
> -		gp.value = &cmdparser;
> -		drmIoctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
> +		cmdparser = drm_get_param(fd, I915_PARAM_CMD_PARSER_VERSION);
>   		igt_require(cmdparser > 0);
>   	}
>   
> diff --git a/tests/i915/gem_exec_params.c b/tests/i915/gem_exec_params.c
> index 49c56a8d..5f443961 100644
> --- a/tests/i915/gem_exec_params.c
> +++ b/tests/i915/gem_exec_params.c
> @@ -79,22 +79,18 @@ static bool has_ring(int fd, unsigned ring_exec_flags)
>   static bool has_exec_batch_first(int fd)
>   {
>   	int val = -1;
> -	struct drm_i915_getparam gp = {
> -		.param = 48,
> -		.value = &val,
> -	};
> -	ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
> +
> +	val = drm_get_param(fd, 48);

Here^

> +
>   	return val > 0;
>   }
>   
>   static bool has_resource_streamer(int fd)
>   {
>   	int val = -1;
> -	struct drm_i915_getparam gp = {
> -		.param = I915_PARAM_HAS_RESOURCE_STREAMER,
> -		.value = &val,
> -	};
> -	ioctl(fd, DRM_IOCTL_I915_GETPARAM , &gp);
> +
> +	val = drm_get_param(fd, I915_PARAM_HAS_RESOURCE_STREAMER);
> +
>   	return val > 0;
>   }
>   
> diff --git a/tests/i915/gem_exec_parse.c b/tests/i915/gem_exec_parse.c
> index b653b1bd..15315438 100644
> --- a/tests/i915/gem_exec_parse.c
> +++ b/tests/i915/gem_exec_parse.c
> @@ -61,12 +61,9 @@ static int parser_version;
>   static int command_parser_version(int fd)
>   {
>   	int version = -1;
> -	drm_i915_getparam_t gp;
>   
> -	gp.param = I915_PARAM_CMD_PARSER_VERSION;
> -	gp.value = &version;
> -
> -	if (drmIoctl(fd, DRM_IOCTL_I915_GETPARAM, &gp) == 0)
> +	version = drm_get_param(fd, I915_PARAM_CMD_PARSER_VERSION);
> +	if (version >= 1)
>   		return version;
>   
>   	return -1;
> diff --git a/tests/i915/gem_exec_suspend.c b/tests/i915/gem_exec_suspend.c
> index 43c52d10..b44af299 100644
> --- a/tests/i915/gem_exec_suspend.c
> +++ b/tests/i915/gem_exec_suspend.c
> @@ -73,14 +73,9 @@ static void test_all(int fd, unsigned flags)
>   
>   static bool has_semaphores(int fd)
>   {
> -	struct drm_i915_getparam gp;
>   	int val = -1;
>   
> -	memset(&gp, 0, sizeof(gp));
> -	gp.param = I915_PARAM_HAS_SEMAPHORES;
> -	gp.value = &val;
> -
> -	drmIoctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
> +	val = drm_get_param(fd, I915_PARAM_HAS_SEMAPHORES);
>   	errno = 0;
>   
>   	return val > 0;
> diff --git a/tests/i915/gem_mmap_gtt.c b/tests/i915/gem_mmap_gtt.c
> index f6353555..396694e6 100644
> --- a/tests/i915/gem_mmap_gtt.c
> +++ b/tests/i915/gem_mmap_gtt.c
> @@ -312,12 +312,9 @@ test_write_gtt(int fd)
>   static bool is_coherent(int i915)
>   {
>   	int val = 1; /* by default, we assume GTT is coherent, hence the test */
> -	struct drm_i915_getparam gp = {
> -		gp.param = 52, /* GTT_COHERENT */
> -		gp.value = &val,
> -	};
>   
> -	ioctl(i915, DRM_IOCTL_I915_GETPARAM, &gp);
> +	val = drm_get_param(i915, 52); /* GTT_COHERENT */
> +
>   	return val;
>   }
>   
> diff --git a/tests/i915/gem_mmap_wc.c b/tests/i915/gem_mmap_wc.c
> index 110883eb..73b2166b 100644
> --- a/tests/i915/gem_mmap_wc.c
> +++ b/tests/i915/gem_mmap_wc.c
> @@ -85,7 +85,6 @@ create_pointer(int fd)
>   static void
>   test_invalid_flags(int fd)
>   {
> -	struct drm_i915_getparam gp;
>   	struct local_i915_gem_mmap_v2 arg;
>   	uint64_t flag = I915_MMAP_WC;
>   	int val = -1;
> @@ -95,12 +94,8 @@ test_invalid_flags(int fd)
>   	arg.offset = 0;
>   	arg.size = 4096;
>   
> -	memset(&gp, 0, sizeof(gp));
> -	gp.param = 30; /* MMAP_VERSION */
> -	gp.value = &val;
> -
>   	/* Do we have the new mmap_ioctl? */
> -	drmIoctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
> +	val = drm_get_param(fd, 30); /* MMAP_VERSION */

here^

I think I got all of those that are defined but you should double check :).

Antonio.

>   
>   	if (val >= 1) {
>   		/*
> diff --git a/tests/i915/hangman.c b/tests/i915/hangman.c
> index 6ddae491..09316226 100644
> --- a/tests/i915/hangman.c
> +++ b/tests/i915/hangman.c
> @@ -112,11 +112,8 @@ static FILE *open_error(void)
>   static bool uses_cmd_parser(void)
>   {
>   	int parser_version = 0;
> -	drm_i915_getparam_t gp;
>   
> -	gp.param = I915_PARAM_CMD_PARSER_VERSION;
> -	gp.value = &parser_version;
> -	drmIoctl(device, DRM_IOCTL_I915_GETPARAM, &gp);
> +	parser_version = drm_get_param(device, I915_PARAM_CMD_PARSER_VERSION);
>   
>   	return parser_version > 0;
>   }
> diff --git a/tests/prime_vgem.c b/tests/prime_vgem.c
> index 60bb951c..759299da 100644
> --- a/tests/prime_vgem.c
> +++ b/tests/prime_vgem.c
> @@ -266,12 +266,9 @@ static void test_shrink(int vgem, int i915)
>   static bool is_coherent(int i915)
>   {
>   	int val = 1; /* by default, we assume GTT is coherent, hence the test */
> -	struct drm_i915_getparam gp = {
> -		gp.param = 52, /* GTT_COHERENT */
> -		gp.value = &val,
> -	};
>   
> -	ioctl(i915, DRM_IOCTL_I915_GETPARAM, &gp);
> +	val = drm_get_param(i915, 52); /* GTT_COHERENT */
> +
>   	return val;
>   }
>   
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t v3 1/3] lib/ioctl_wrapper: Add i915_get_param helper

[Nautiyal, Ankit K]

-----Original Message-----
From: igt-dev [mailto:igt-dev-bounces@lists.freedesktop.org] On Behalf Of Lukasz Kalamarz
Sent: Monday, November 26, 2018 9:06 PM
To: igt-dev@lists.freedesktop.org
Subject: [igt-dev] [PATCH i-g-t v3 1/3] lib/ioctl_wrapper: Add i915_get_param helper

getparam is used in few places across IGT, but no helper function is used to reduce code duplication.
v2: Added doc part and changed return value in case of error

Signed-off-by: Lukasz Kalamarz <lukasz.kalamarz@intel.com>

Cc: Michal Winiarski <michal.winiarski@intel.com>
Cc: Katarzyna Dec <katarzyna.dec@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
---
 lib/ioctl_wrappers.c | 22 ++++++++++++++++++++++  lib/ioctl_wrappers.h |  1 +
 2 files changed, 23 insertions(+)

diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c index 9f255508..aec4f15e 100644
--- a/lib/ioctl_wrappers.c
+++ b/lib/ioctl_wrappers.c
@@ -468,6 +468,28 @@ void gem_sync(int fd, uint32_t handle)
 	errno = 0;
 }
 
+/**
+ * drm_get_param:
+ * @fd: open i915 drm file descriptor
+ * @param: drm parameter we want to read
+ *
+ * Helper function that execute GETPARAM ioctl for a given parameter.
+ *
+ * Return: Read value from GETPARAM
+ */
+int drm_get_param(int fd, uint32_t param) {
+	int value;
+	drm_i915_getparam_t gp = {
+		.param = param,
+		.value = &value
+	};
+
+	if (igt_ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp) == -1)
+		return -EINVAL;
+

I think the ioctl can return some error value other than -1.
Shouldn’t  < 0 a proper check?
I suggest store the return value of the ioctl, check for < 0 and return the ret value in case of ioctl failure.

-Ankit

+	return value;
+}
 
 bool gem_create__has_stolen_support(int fd)  { diff --git a/lib/ioctl_wrappers.h b/lib/ioctl_wrappers.h index b22b36b0..90174d13 100644
--- a/lib/ioctl_wrappers.h
+++ b/lib/ioctl_wrappers.h
@@ -74,6 +74,7 @@ int __gem_set_domain(int fd, uint32_t handle, uint32_t read, uint32_t write);  void gem_set_domain(int fd, uint32_t handle, uint32_t read, uint32_t write);  int gem_wait(int fd, uint32_t handle, int64_t *timeout_ns);  void gem_sync(int fd, uint32_t handle);
+int drm_get_param(int fd, uint32_t param);
 bool gem_create__has_stolen_support(int fd);  uint32_t __gem_create_stolen(int fd, uint64_t size);  uint32_t gem_create_stolen(int fd, uint64_t size);
--
2.17.2

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] ✗ Fi.CI.BAT: failure for series starting with [i-g-t,v3,1/3] lib/ioctl_wrapper: Add i915_get_param helper (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [i-g-t,v3,1/3] lib/ioctl_wrapper: Add i915_get_param helper (rev2)
URL   : https://patchwork.freedesktop.org/series/53019/
State : failure

== Summary ==

Applying: lib/ioctl_wrapper: Add i915_get_param helper
Patch failed at 0001 lib/ioctl_wrapper: Add i915_get_param helper
Use 'git am --show-current-patch' to see the failed patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t v3 1/3] lib/ioctl_wrapper: Add i915_get_param helper

[Daniele Ceraolo Spurio]

On 26/11/2018 07:36, Lukasz Kalamarz wrote:
> getparam is used in few places across IGT, but no helper function
> is used to reduce code duplication.
> v2: Added doc part and changed return value in case of error
> 
> Signed-off-by: Lukasz Kalamarz <lukasz.kalamarz@intel.com>
> 
> Cc: Michal Winiarski <michal.winiarski@intel.com>
> Cc: Katarzyna Dec <katarzyna.dec@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> ---
>   lib/ioctl_wrappers.c | 22 ++++++++++++++++++++++
>   lib/ioctl_wrappers.h |  1 +
>   2 files changed, 23 insertions(+)
> 
> diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
> index 9f255508..aec4f15e 100644
> --- a/lib/ioctl_wrappers.c
> +++ b/lib/ioctl_wrappers.c
> @@ -468,6 +468,28 @@ void gem_sync(int fd, uint32_t handle)
>   	errno = 0;
>   }
>   
> +/**
> + * drm_get_param:
> + * @fd: open i915 drm file descriptor
> + * @param: drm parameter we want to read
> + *
> + * Helper function that execute GETPARAM ioctl for a given parameter.
> + *
> + * Return: Read value from GETPARAM
> + */
> +int drm_get_param(int fd, uint32_t param)
> +{
> +	int value;
> +	drm_i915_getparam_t gp = {
> +		.param = param,
> +		.value = &value
> +	};
> +
> +	if (igt_ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp) == -1)
> +		return -EINVAL;

Returning the errno value would be more appropriate here IMO.

Daniele

> +
> +	return value;
> +}
>   
>   bool gem_create__has_stolen_support(int fd)
>   {
> diff --git a/lib/ioctl_wrappers.h b/lib/ioctl_wrappers.h
> index b22b36b0..90174d13 100644
> --- a/lib/ioctl_wrappers.h
> +++ b/lib/ioctl_wrappers.h
> @@ -74,6 +74,7 @@ int __gem_set_domain(int fd, uint32_t handle, uint32_t read, uint32_t write);
>   void gem_set_domain(int fd, uint32_t handle, uint32_t read, uint32_t write);
>   int gem_wait(int fd, uint32_t handle, int64_t *timeout_ns);
>   void gem_sync(int fd, uint32_t handle);
> +int drm_get_param(int fd, uint32_t param);
>   bool gem_create__has_stolen_support(int fd);
>   uint32_t __gem_create_stolen(int fd, uint64_t size);
>   uint32_t gem_create_stolen(int fd, uint64_t size);
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t v3 2/3] tests: Use drm_get_param where applicable

[Daniele Ceraolo Spurio]

On 26/11/2018 07:36, Lukasz Kalamarz wrote:
> Across several tests we check values of a given parameters.
> With implementation of drm_get_param we can drop duplicated
> lines and use helper function instead.
> v2: Fixed errors in argument (fd are named as i915)
> 
> Signed-off-by: Lukasz Kalamarz <lukasz.kalamarz@intel.com>
> 
> Cc: Michal Winiarski <michal.winiarski@intel.com>
> Cc: Katarzyna Dec <katarzyna.dec@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> ---
>   tests/i915/gem_busy.c          |  8 ++------
>   tests/i915/gem_cs_tlb.c        | 10 ++--------
>   tests/i915/gem_ctx_isolation.c |  7 +------
>   tests/i915/gem_exec_async.c    |  6 ++----
>   tests/i915/gem_exec_capture.c  |  5 +----
>   tests/i915/gem_exec_fence.c    | 14 ++------------
>   tests/i915/gem_exec_flush.c    |  5 +----
>   tests/i915/gem_exec_params.c   | 16 ++++++----------
>   tests/i915/gem_exec_parse.c    |  7 ++-----
>   tests/i915/gem_exec_suspend.c  |  7 +------
>   tests/i915/gem_mmap_gtt.c      |  7 ++-----
>   tests/i915/gem_mmap_wc.c       |  7 +------
>   tests/i915/hangman.c           |  5 +----
>   tests/prime_vgem.c             |  7 ++-----
>   14 files changed, 26 insertions(+), 85 deletions(-)
> 
> diff --git a/tests/i915/gem_busy.c b/tests/i915/gem_busy.c
> index 76b44a5d..6cc8e45b 100644
> --- a/tests/i915/gem_busy.c
> +++ b/tests/i915/gem_busy.c
> @@ -28,6 +28,7 @@
>   #include "igt.h"
>   #include "igt_rand.h"
>   #include "igt_vgem.h"
> +#include "ioctl_wrappers.h"
>   #include "i915/gem_ring.h"
>   
>   #define LOCAL_EXEC_NO_RELOC (1<<11)
> @@ -401,14 +402,9 @@ static void close_race(int fd)
>   
>   static bool has_semaphores(int fd)
>   {
> -	struct drm_i915_getparam gp;
>   	int val = -1;
>   
> -	memset(&gp, 0, sizeof(gp));
> -	gp.param = I915_PARAM_HAS_SEMAPHORES;
> -	gp.value = &val;
> -
> -	drmIoctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
> +	val = drm_get_param(fd, I915_PARAM_HAS_SEMAPHORES);
>   	errno = 0;
>   
>   	return val > 0;
> diff --git a/tests/i915/gem_cs_tlb.c b/tests/i915/gem_cs_tlb.c
> index 51e1c4e1..891cd551 100644
> --- a/tests/i915/gem_cs_tlb.c
> +++ b/tests/i915/gem_cs_tlb.c
> @@ -58,17 +58,11 @@ IGT_TEST_DESCRIPTION("Check whether we correctly invalidate the cs tlb.");
>   
>   static bool has_softpin(int fd)
>   {
> -	struct drm_i915_getparam gp;
>   	int val = 0;
>   
> -	memset(&gp, 0, sizeof(gp));
> -	gp.param = 37; /* I915_PARAM_HAS_EXEC_SOFTPIN */
> -	gp.value = &val;
> -
> -	if (drmIoctl(fd, DRM_IOCTL_I915_GETPARAM, &gp))
> -		return 0;
> -
> +	val = drm_get_param(fd, 37); /* I915_PARAM_HAS_EXEC_SOFTPIN */
>   	errno = 0;
> +
>   	return (val == 1);
>   }
>   
> diff --git a/tests/i915/gem_ctx_isolation.c b/tests/i915/gem_ctx_isolation.c
> index 058cf3ec..27c8429d 100644
> --- a/tests/i915/gem_ctx_isolation.c
> +++ b/tests/i915/gem_ctx_isolation.c
> @@ -671,14 +671,9 @@ static void preservation(int fd,
>   
>   static unsigned int __has_context_isolation(int fd)
>   {
> -	struct drm_i915_getparam gp;
>   	int value = 0;
>   
> -	memset(&gp, 0, sizeof(gp));
> -	gp.param = 50; /* I915_PARAM_HAS_CONTEXT_ISOLATION */
> -	gp.value = &value;
> -
> -	igt_ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
> +	value = drm_get_param(fd, 50); /* I915_PARAM_HAS_CONTEXT_ISOLATION */

The code that uses this function does an igt_require() on the its 
return, so it will now pass even if value < 0. value was not modified 
before for non existing params but it is now negative with your change.

>   	errno = 0;
>   
>   	return value;
> diff --git a/tests/i915/gem_exec_async.c b/tests/i915/gem_exec_async.c
> index 9a06af7e..6b5a5d25 100644
> --- a/tests/i915/gem_exec_async.c
> +++ b/tests/i915/gem_exec_async.c
> @@ -22,6 +22,7 @@
>    */
>   
>   #include "igt.h"
> +#include "ioctl_wrappers.h"
>   
>   #define LOCAL_OBJECT_ASYNC (1 << 6)
>   #define LOCAL_PARAM_HAS_EXEC_ASYNC 43
> @@ -173,12 +174,9 @@ static void one(int fd, unsigned ring, uint32_t flags)
>   
>   static bool has_async_execbuf(int fd)
>   {
> -	drm_i915_getparam_t gp;
>   	int async = -1;
>   
> -	gp.param = LOCAL_PARAM_HAS_EXEC_ASYNC;
> -	gp.value = &async;
> -	drmIoctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
> +	async = drm_get_param(fd, LOCAL_PARAM_HAS_EXEC_ASYNC);
>   
>   	return async > 0;
>   }
> diff --git a/tests/i915/gem_exec_capture.c b/tests/i915/gem_exec_capture.c
> index 3e4a4377..72cf8ef4 100644
> --- a/tests/i915/gem_exec_capture.c
> +++ b/tests/i915/gem_exec_capture.c
> @@ -189,12 +189,9 @@ static void userptr(int fd, int dir)
>   
>   static bool has_capture(int fd)
>   {
> -	drm_i915_getparam_t gp;
>   	int async = -1;
>   
> -	gp.param = LOCAL_PARAM_HAS_EXEC_CAPTURE;
> -	gp.value = &async;
> -	drmIoctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
> +	async = drm_get_param(fd, LOCAL_PARAM_HAS_EXEC_CAPTURE);
>   
>   	return async > 0;
>   }
> diff --git a/tests/i915/gem_exec_fence.c b/tests/i915/gem_exec_fence.c
> index ba46595d..ee143a5f 100644
> --- a/tests/i915/gem_exec_fence.c
> +++ b/tests/i915/gem_exec_fence.c
> @@ -803,14 +803,9 @@ static void test_fence_flip(int i915)
>   
>   static bool has_submit_fence(int fd)
>   {
> -	struct drm_i915_getparam gp;
>   	int value = 0;
>   
> -	memset(&gp, 0, sizeof(gp));
> -	gp.param = 0xdeadbeef ^ 51; /* I915_PARAM_HAS_EXEC_SUBMIT_FENCE */
> -	gp.value = &value;
> -
> -	ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp, sizeof(gp));
> +	value = drm_get_param(fd, 0xdeadbeef ^ 51); /* I915_PARAM_HAS_EXEC_SUBMIT_FENCE */
>   	errno = 0;
>   
>   	return value;

The return is converted to bool, so even if value is negative it'll 
become true. return value > 0?

> @@ -825,14 +820,9 @@ static bool has_syncobj(int fd)
>   
>   static bool exec_has_fence_array(int fd)
>   {
> -	struct drm_i915_getparam gp;
>   	int value = 0;
>   
> -	memset(&gp, 0, sizeof(gp));
> -	gp.param = 49; /* I915_PARAM_HAS_EXEC_FENCE_ARRAY */
> -	gp.value = &value;
> -
> -	ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp, sizeof(gp));
> +	value = drm_get_param(fd, 49); /* I915_PARAM_HAS_EXEC_FENCE_ARRAY */
>   	errno = 0;
>   
>   	return value;

Same as above

> diff --git a/tests/i915/gem_exec_flush.c b/tests/i915/gem_exec_flush.c
> index f820b2a8..bdc9257d 100644
> --- a/tests/i915/gem_exec_flush.c
> +++ b/tests/i915/gem_exec_flush.c
> @@ -359,11 +359,8 @@ static void batch(int fd, unsigned ring, int nchild, int timeout,
>   
>   	if (flags & CMDPARSER) {
>   		int cmdparser = -1;
> -                drm_i915_getparam_t gp;
>   
> -		gp.param = I915_PARAM_CMD_PARSER_VERSION;
> -		gp.value = &cmdparser;
> -		drmIoctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
> +		cmdparser = drm_get_param(fd, I915_PARAM_CMD_PARSER_VERSION);
>   		igt_require(cmdparser > 0);
>   	}
>   
> diff --git a/tests/i915/gem_exec_params.c b/tests/i915/gem_exec_params.c
> index 49c56a8d..5f443961 100644
> --- a/tests/i915/gem_exec_params.c
> +++ b/tests/i915/gem_exec_params.c
> @@ -79,22 +79,18 @@ static bool has_ring(int fd, unsigned ring_exec_flags)
>   static bool has_exec_batch_first(int fd)
>   {
>   	int val = -1;
> -	struct drm_i915_getparam gp = {
> -		.param = 48,
> -		.value = &val,
> -	};
> -	ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
> +
> +	val = drm_get_param(fd, 48);
> +
>   	return val > 0;
>   }
>   
>   static bool has_resource_streamer(int fd)
>   {
>   	int val = -1;
> -	struct drm_i915_getparam gp = {
> -		.param = I915_PARAM_HAS_RESOURCE_STREAMER,
> -		.value = &val,
> -	};
> -	ioctl(fd, DRM_IOCTL_I915_GETPARAM , &gp);
> +
> +	val = drm_get_param(fd, I915_PARAM_HAS_RESOURCE_STREAMER);
> +
>   	return val > 0;
>   }
>   
> diff --git a/tests/i915/gem_exec_parse.c b/tests/i915/gem_exec_parse.c
> index b653b1bd..15315438 100644
> --- a/tests/i915/gem_exec_parse.c
> +++ b/tests/i915/gem_exec_parse.c
> @@ -61,12 +61,9 @@ static int parser_version;
>   static int command_parser_version(int fd)
>   {
>   	int version = -1;
> -	drm_i915_getparam_t gp;
>   
> -	gp.param = I915_PARAM_CMD_PARSER_VERSION;
> -	gp.value = &version;
> -
> -	if (drmIoctl(fd, DRM_IOCTL_I915_GETPARAM, &gp) == 0)
> +	version = drm_get_param(fd, I915_PARAM_CMD_PARSER_VERSION);
> +	if (version >= 1)
>   		return version;
>   
>   	return -1;
> diff --git a/tests/i915/gem_exec_suspend.c b/tests/i915/gem_exec_suspend.c
> index 43c52d10..b44af299 100644
> --- a/tests/i915/gem_exec_suspend.c
> +++ b/tests/i915/gem_exec_suspend.c
> @@ -73,14 +73,9 @@ static void test_all(int fd, unsigned flags)
>   
>   static bool has_semaphores(int fd)
>   {
> -	struct drm_i915_getparam gp;
>   	int val = -1;
>   
> -	memset(&gp, 0, sizeof(gp));
> -	gp.param = I915_PARAM_HAS_SEMAPHORES;
> -	gp.value = &val;
> -
> -	drmIoctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
> +	val = drm_get_param(fd, I915_PARAM_HAS_SEMAPHORES);
>   	errno = 0;
>   
>   	return val > 0;
> diff --git a/tests/i915/gem_mmap_gtt.c b/tests/i915/gem_mmap_gtt.c
> index f6353555..396694e6 100644
> --- a/tests/i915/gem_mmap_gtt.c
> +++ b/tests/i915/gem_mmap_gtt.c
> @@ -312,12 +312,9 @@ test_write_gtt(int fd)
>   static bool is_coherent(int i915)
>   {
>   	int val = 1; /* by default, we assume GTT is coherent, hence the test */

you're breaking the assumption that val is not modified by the getparam

> -	struct drm_i915_getparam gp = {
> -		gp.param = 52, /* GTT_COHERENT */
> -		gp.value = &val,
> -	};
>   
> -	ioctl(i915, DRM_IOCTL_I915_GETPARAM, &gp);
> +	val = drm_get_param(i915, 52); /* GTT_COHERENT */
> +
>   	return val;

Again cast to bool

>   }
>   
> diff --git a/tests/i915/gem_mmap_wc.c b/tests/i915/gem_mmap_wc.c
> index 110883eb..73b2166b 100644
> --- a/tests/i915/gem_mmap_wc.c
> +++ b/tests/i915/gem_mmap_wc.c
> @@ -85,7 +85,6 @@ create_pointer(int fd)
>   static void
>   test_invalid_flags(int fd)
>   {
> -	struct drm_i915_getparam gp;
>   	struct local_i915_gem_mmap_v2 arg;
>   	uint64_t flag = I915_MMAP_WC;
>   	int val = -1;
> @@ -95,12 +94,8 @@ test_invalid_flags(int fd)
>   	arg.offset = 0;
>   	arg.size = 4096;
>   
> -	memset(&gp, 0, sizeof(gp));
> -	gp.param = 30; /* MMAP_VERSION */
> -	gp.value = &val;
> -
>   	/* Do we have the new mmap_ioctl? */
> -	drmIoctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
> +	val = drm_get_param(fd, 30); /* MMAP_VERSION */
>   
>   	if (val >= 1) {
>   		/*
> diff --git a/tests/i915/hangman.c b/tests/i915/hangman.c
> index 6ddae491..09316226 100644
> --- a/tests/i915/hangman.c
> +++ b/tests/i915/hangman.c
> @@ -112,11 +112,8 @@ static FILE *open_error(void)
>   static bool uses_cmd_parser(void)
>   {
>   	int parser_version = 0;
> -	drm_i915_getparam_t gp;
>   
> -	gp.param = I915_PARAM_CMD_PARSER_VERSION;
> -	gp.value = &parser_version;
> -	drmIoctl(device, DRM_IOCTL_I915_GETPARAM, &gp);
> +	parser_version = drm_get_param(device, I915_PARAM_CMD_PARSER_VERSION);
>   
>   	return parser_version > 0;
>   }
> diff --git a/tests/prime_vgem.c b/tests/prime_vgem.c
> index 60bb951c..759299da 100644
> --- a/tests/prime_vgem.c
> +++ b/tests/prime_vgem.c
> @@ -266,12 +266,9 @@ static void test_shrink(int vgem, int i915)
>   static bool is_coherent(int i915)
>   {

Same issues as the other is_coherent() function. BTW, maybe it'd be 
worth moving it to a common place.


>   	int val = 1; /* by default, we assume GTT is coherent, hence the test */
> -	struct drm_i915_getparam gp = {
> -		gp.param = 52, /* GTT_COHERENT */
> -		gp.value = &val,
> -	};
>   
> -	ioctl(i915, DRM_IOCTL_I915_GETPARAM, &gp);
> +	val = drm_get_param(i915, 52); /* GTT_COHERENT */
> +
>   	return val;
>   }
>   
> 

A lot of the params are booleans (i.e. the feature is either there or 
not, no version), so we can maybe have a second level wrapper. something 
like:

bool i915_has_feature(int fd, uint32_t param)
{
	return drm_get_param(fd, param) > 0;
}

Daniele
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t v3 3/3] lib: Use drm_get_param where it is applicable

[Daniele Ceraolo Spurio]

On 26/11/2018 07:36, Lukasz Kalamarz wrote:
> With usage of implemented drm_get_param helper function, we can
> remove some duplicated code lines across few libs.
> 
> v2: Fix typos and missing include
> v3: Drop unnecessary getparam structure in one function
> 
> Signed-off-by: Lukasz Kalamarz <lukasz.kalamarz@intel.com>
> 
> Cc: Michal Winiarski <michal.winiarski@intel.com>
> Cc: Katarzyna Dec <katarzyna.dec@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> ---
>   lib/drmtest.c             |  7 ++---
>   lib/i915/gem_scheduler.c  |  8 +----
>   lib/i915/gem_submission.c | 10 +++---
>   lib/igt_gt.c              |  7 ++---
>   lib/intel_chipset.c       |  7 ++---
>   lib/ioctl_wrappers.c      | 66 ++++++---------------------------------
>   6 files changed, 22 insertions(+), 83 deletions(-)
> 
> diff --git a/lib/drmtest.c b/lib/drmtest.c
> index fee9d33a..81383fd4 100644
> --- a/lib/drmtest.c
> +++ b/lib/drmtest.c
> @@ -107,14 +107,11 @@ bool is_i915_device(int fd)
>   
>   static bool has_known_intel_chipset(int fd)
>   {
> -	struct drm_i915_getparam gp;
>   	int devid = 0;
>   
> -	memset(&gp, 0, sizeof(gp));
> -	gp.param = I915_PARAM_CHIPSET_ID;
> -	gp.value = &devid;
> +	devid = drm_get_param(fd, I915_PARAM_CHIPSET_ID);
>   
> -	if (ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp, sizeof(gp)))
> +	if (devid <= 0)
>   		return false;
>   
>   	if (!intel_gen(devid))

You can now potentially simplify this function with something like:

static bool has_known_intel_chipset(int fd)
{
	int devid = 0;
	devid = drm_get_param(fd, I915_PARAM_CHIPSET_ID);
	return devid > 0 && intel_gen(devid);
}

> diff --git a/lib/i915/gem_scheduler.c b/lib/i915/gem_scheduler.c
> index ad156306..079559a2 100644
> --- a/lib/i915/gem_scheduler.c
> +++ b/lib/i915/gem_scheduler.c
> @@ -52,14 +52,8 @@ unsigned gem_scheduler_capability(int fd)
>   	static int caps = -1;
>   
>   	if (caps < 0) {
> -		struct drm_i915_getparam gp;
> -
> -		memset(&gp, 0, sizeof(gp));
> -		gp.param = LOCAL_I915_PARAM_HAS_SCHEDULER;
> -		gp.value = &caps;
> -
>   		caps = 0;
> -		ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp, sizeof(gp));
> +		caps = drm_get_param(fd, LOCAL_I915_PARAM_HAS_SCHEDULER);

Now caps can be < 0 on error, which will break the logic. I guess you want:

	caps = drm_get_param(fd, LOCAL_I915_PARAM_HAS_SCHEDULER);
	if (caps < 0)
		caps = 0;

>   		errno = 0;
>   	}
>   
> diff --git a/lib/i915/gem_submission.c b/lib/i915/gem_submission.c
> index 2fd460d5..583bb133 100644
> --- a/lib/i915/gem_submission.c
> +++ b/lib/i915/gem_submission.c
> @@ -53,12 +53,12 @@
>   static bool has_semaphores(int fd, int dir)
>   {
>   	int val = 0;
> -	struct drm_i915_getparam gp = {
> -		gp.param = I915_PARAM_HAS_SEMAPHORES,
> -		gp.value = &val,
> -	};
> -	if (ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp) < 0)
> +
> +	val = drm_get_param(fd, I915_PARAM_HAS_SEMAPHORES);
> +
> +	if (val < 0)
>   		val = igt_sysfs_get_boolean(dir, "semaphores");
> +
>   	return val;
>   }
>   
> diff --git a/lib/igt_gt.c b/lib/igt_gt.c
> index a2061924..18cd922b 100644
> --- a/lib/igt_gt.c
> +++ b/lib/igt_gt.c
> @@ -57,14 +57,11 @@ static bool has_gpu_reset(int fd)
>   {
>   	static int once = -1;
>   	if (once < 0) {
> -		struct drm_i915_getparam gp;
>   		int val = 0;
>   
> -		memset(&gp, 0, sizeof(gp));
> -		gp.param = 35; /* HAS_GPU_RESET */
> -		gp.value = &val;
> +		val = drm_get_param(fd, 35); /* HAS_GPU_RESET */
>   
> -		if (ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp))
> +		if (val > 0)

This check is the wrong way around, you want val < 0 here (it's the 
failure case)

>   			once = intel_gen(intel_get_drm_devid(fd)) >= 5;
>   		else
>   			once = val > 0;
> diff --git a/lib/intel_chipset.c b/lib/intel_chipset.c
> index 4748a3fb..da354fb6 100644
> --- a/lib/intel_chipset.c
> +++ b/lib/intel_chipset.c
> @@ -41,6 +41,7 @@
>   #include "drmtest.h"
>   #include "intel_chipset.h"
>   #include "igt_core.h"
> +#include "ioctl_wrappers.h"
>   
>   /**
>    * SECTION:intel_chipset
> @@ -125,7 +126,6 @@ intel_get_pci_device(void)
>   uint32_t
>   intel_get_drm_devid(int fd)
>   {
> -	struct drm_i915_getparam gp;
>   	const char *override;
>   	int devid = 0;
>   
> @@ -135,10 +135,7 @@ intel_get_drm_devid(int fd)
>   	if (override)
>   		return strtol(override, NULL, 0);
>   
> -	memset(&gp, 0, sizeof(gp));
> -	gp.param = I915_PARAM_CHIPSET_ID;
> -	gp.value = &devid;
> -	ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp, sizeof(gp));
> +	devid = drm_get_param(fd, I915_PARAM_CHIPSET_ID);
>   
>   	return devid;
>   }
> diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
> index aec4f15e..d156103b 100644
> --- a/lib/ioctl_wrappers.c
> +++ b/lib/ioctl_wrappers.c
> @@ -494,16 +494,12 @@ int drm_get_param(int fd, uint32_t param)
>   bool gem_create__has_stolen_support(int fd)
>   {
>   	static int has_stolen_support = -1;
> -	struct drm_i915_getparam gp;
>   	int val = -1;
>   
>   	if (has_stolen_support < 0) {
> -		memset(&gp, 0, sizeof(gp));
> -		gp.param = 38; /* CREATE_VERSION */
> -		gp.value = &val;
> +		val = drm_get_param(fd, 38); /* CREATE_VERSION */
>   
>   		/* Do we have the extended gem_create_ioctl? */
> -		ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
>   		has_stolen_support = val >= 2;
>   	}
>   
> @@ -723,21 +719,13 @@ bool gem_mmap__has_wc(int fd)
>   	static int has_wc = -1;
>   
>   	if (has_wc == -1) {
> -		struct drm_i915_getparam gp;
>   		int mmap_version = -1;
>   		int gtt_version = -1;
>   
>   		has_wc = 0;
>   
> -		memset(&gp, 0, sizeof(gp));
> -		gp.param = 40; /* MMAP_GTT_VERSION */
> -		gp.value = &gtt_version;
> -		ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
> -
> -		memset(&gp, 0, sizeof(gp));
> -		gp.param = 30; /* MMAP_VERSION */
> -		gp.value = &mmap_version;
> -		ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
> +		gtt_version = drm_get_param(fd, 40); /* MMAP_GTT_VERSION */
> +		mmap_version = drm_get_param(fd, 30); /* MMAP_VERSION */
>   
>   		/* Do we have the new mmap_ioctl with DOMAIN_WC? */
>   		if (mmap_version >= 1 && gtt_version >= 2) {
> @@ -982,17 +970,11 @@ bool gem_bo_busy(int fd, uint32_t handle)
>    */
>   static int gem_gtt_type(int fd)
>   {
> -	struct drm_i915_getparam gp;
>   	int val = 0;
>   
> -	memset(&gp, 0, sizeof(gp));
> -	gp.param = 18; /* HAS_ALIASING_PPGTT */
> -	gp.value = &val;
> -
> -	if (ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp, sizeof(gp)))
> -		return 0;
> -
> +	val = drm_get_param(fd, 18); /* HAS_ALIASING_PPGTT */

You've lost the error case

if (val < 0)
	return 0

Or update the docs to say we return a negative val on error, we always 
check the return of this func with a > so that'd work anyway

>   	errno = 0;
> +
>   	return val;
>   }
>   
> @@ -1036,13 +1018,9 @@ bool gem_uses_full_ppgtt(int fd)
>    */
>   int gem_gpu_reset_type(int fd)
>   {
> -	struct drm_i915_getparam gp;
>   	int gpu_reset_type = -1;
>   
> -	memset(&gp, 0, sizeof(gp));
> -	gp.param = I915_PARAM_HAS_GPU_RESET;
> -	gp.value = &gpu_reset_type;
> -	drmIoctl(fd, DRM_IOCTL_I915_GETPARAM, &gp);
> +	gpu_reset_type = drm_get_param(fd, I915_PARAM_HAS_GPU_RESET);
>   
>   	return gpu_reset_type;
>   }
> @@ -1090,14 +1068,8 @@ int gem_available_fences(int fd)
>   	static int num_fences = -1;
>   
>   	if (num_fences < 0) {
> -		struct drm_i915_getparam gp;
> -
> -		memset(&gp, 0, sizeof(gp));
> -		gp.param = I915_PARAM_NUM_FENCES_AVAIL;
> -		gp.value = &num_fences;
> -
>   		num_fences = 0;
> -		ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp, sizeof(gp));
> +		num_fences = drm_get_param(fd, I915_PARAM_NUM_FENCES_AVAIL);

You've lost the assumption that if the kernel doesn't know 
I915_PARAM_NUM_FENCES_AVAIL num_fences stays 0.

>   		errno = 0;
>   	}
>   
> @@ -1109,14 +1081,8 @@ bool gem_has_llc(int fd)
>   	static int has_llc = -1;
>   
>   	if (has_llc < 0) {
> -		struct drm_i915_getparam gp;
> -
> -		memset(&gp, 0, sizeof(gp));
> -		gp.param = I915_PARAM_HAS_LLC;
> -		gp.value = &has_llc;
> -
>   		has_llc = 0;
> -		ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp, sizeof(gp));
> +		has_llc = drm_get_param(fd, I915_PARAM_HAS_LLC);

same as above. Since this is a boolean, with the wrapper I've suggested 
in the other patch this could become:

	has_llc = i915_has_feature(fd, I915_PARAM_HAS_LLC);

Which should set has_llc to 0 on ioctl failure

>   		errno = 0;
>   	}
>   
> @@ -1366,14 +1332,8 @@ bool gem_has_softpin(int fd)
>   	static int has_softpin = -1;
>   
>   	if (has_softpin < 0) {
> -		struct drm_i915_getparam gp;
> -
> -		memset(&gp, 0, sizeof(gp));
> -		gp.param = I915_PARAM_HAS_EXEC_SOFTPIN;
> -		gp.value = &has_softpin;
> -
>   		has_softpin = 0;
> -		ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp, sizeof(gp));
> +		has_softpin = drm_get_param(fd, I915_PARAM_HAS_EXEC_SOFTPIN);

same as above

>   		errno = 0;
>   	}
>   
> @@ -1394,14 +1354,8 @@ bool gem_has_exec_fence(int fd)
>   	static int has_exec_fence = -1;
>   
>   	if (has_exec_fence < 0) {
> -		struct drm_i915_getparam gp;
> -
> -		memset(&gp, 0, sizeof(gp));
> -		gp.param = I915_PARAM_HAS_EXEC_FENCE;
> -		gp.value = &has_exec_fence;
> -
>   		has_exec_fence = 0;
> -		ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp, sizeof(gp));
> +		has_exec_fence = drm_get_param(fd, I915_PARAM_HAS_EXEC_FENCE);

Same again.

Daniele

>   		errno = 0;
>   	}
>   
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev