[igt-dev] [PATCH i-g-t 0/7] Use device dependant module parameters

[igt-dev] [PATCH i-g-t 0/7] Use device dependant module parameters

[Juha-Pekka Heikkila]

Rebased and fixed Jani's work. Patches which didn't change I did maintain
Jani's authorship. These changes match to my kernel where there is path
/sys/kernel/debug/dri/<device>/i915_params/

Special part is the search of 'default device' in patch
'use igt_params_set_save for igt_set_module_param*'. Don't know if it is 
the most obvious way nor do I know will this work on non-Intel driver.

Patch #4 Jani had not added S-o-b, will be added if Jani comment.

Fixed issues pointed by Petri. Reading comments I wasn't certain on
concensus about the final naming so I didn't go change that for now.

/Juha-Pekka

Jani Nikula (4):
  lib/params: add igt_params.c for module parameter access
  lib/params: overhaul param saving
  lib/params: add igt_params_open() which will return path
  igt/params: add generic saving module parameter set

Juha-Pekka Heikkila (3):
  lib/params: start renaming functions igt_params_*
  igt/params: use igt_params_set_save for igt_set_module_param*
  tests/gem_eio: switch to using igt_params_set()

 lib/Makefile.sources                  |   2 +
 lib/drmtest.c                         |   2 +-
 lib/i915/gem_submission.c             |   3 +-
 lib/igt.h                             |   1 +
 lib/igt_aux.c                         | 146 +----------
 lib/igt_aux.h                         |   3 -
 lib/igt_gt.c                          |   3 +-
 lib/igt_params.c                      | 335 ++++++++++++++++++++++++++
 lib/igt_params.h                      |  40 +++
 lib/igt_psr.c                         |   1 +
 lib/igt_sysfs.c                       |  68 ------
 lib/igt_sysfs.h                       |   5 -
 lib/meson.build                       |   1 +
 tests/i915/gem_ctx_exec.c             |   2 +-
 tests/i915/gem_ctx_persistence.c      |  10 +-
 tests/i915/gem_eio.c                  |  57 ++---
 tests/i915/gem_mmap_gtt.c             |   2 +-
 tests/i915/gem_reset_stats.c          |   6 +-
 tests/i915/sysfs_heartbeat_interval.c |   3 +-
 tests/i915/sysfs_preempt_timeout.c    |   3 +-
 tests/i915/sysfs_timeslice_duration.c |   3 +-
 21 files changed, 426 insertions(+), 270 deletions(-)
 create mode 100644 lib/igt_params.c
 create mode 100644 lib/igt_params.h

-- 
2.26.0

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

[igt-dev] [PATCH i-g-t 1/7] lib/params: add igt_params.c for module parameter access

[Juha-Pekka Heikkila]

From: Jani Nikula <jani.nikula@intel.com>

We have generic helpers for sysfs access in igt_sysfs.c, but we also
have a number of module parameter access specific helpers scattered here
and there. Start gathering the latter into a file of its own.

For i915, the long-term goal is to migrate from module parameters to
device specific debugfs parameters. With all igt module param access
centralized in one place, we can make the transition much easier.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
Reviewed-by: Petri Latvala <petri.latvala@intel.com>
---
 lib/Makefile.sources             |   2 +
 lib/i915/gem_submission.c        |   1 +
 lib/igt.h                        |   1 +
 lib/igt_aux.c                    | 143 +-----------------
 lib/igt_aux.h                    |   3 -
 lib/igt_gt.c                     |   1 +
 lib/igt_params.c                 | 244 +++++++++++++++++++++++++++++++
 lib/igt_params.h                 |  38 +++++
 lib/igt_psr.c                    |   1 +
 lib/igt_sysfs.c                  |  68 ---------
 lib/igt_sysfs.h                  |   5 -
 lib/meson.build                  |   1 +
 tests/i915/gem_ctx_persistence.c |   1 +
 13 files changed, 291 insertions(+), 218 deletions(-)
 create mode 100644 lib/igt_params.c
 create mode 100644 lib/igt_params.h

diff --git a/lib/Makefile.sources b/lib/Makefile.sources
index 1e2c88ae..2ead9d02 100644
--- a/lib/Makefile.sources
+++ b/lib/Makefile.sources
@@ -47,6 +47,8 @@ lib_source_list =	 	\
 	igt_list.h		\
 	igt_matrix.c		\
 	igt_matrix.h		\
+	igt_params.c		\
+	igt_params.h		\
 	igt_primes.c		\
 	igt_primes.h		\
 	igt_rand.c		\
diff --git a/lib/i915/gem_submission.c b/lib/i915/gem_submission.c
index 72de0c22..bb58a207 100644
--- a/lib/i915/gem_submission.c
+++ b/lib/i915/gem_submission.c
@@ -32,6 +32,7 @@
 
 #include "igt_core.h"
 #include "igt_gt.h"
+#include "igt_params.h"
 #include "igt_sysfs.h"
 #include "intel_chipset.h"
 #include "intel_reg.h"
diff --git a/lib/igt.h b/lib/igt.h
index a6c4e44d..0bf98aa5 100644
--- a/lib/igt.h
+++ b/lib/igt.h
@@ -37,6 +37,7 @@
 #include "igt_frame.h"
 #include "igt_gt.h"
 #include "igt_kms.h"
+#include "igt_params.h"
 #include "igt_pm.h"
 #include "igt_stats.h"
 #ifdef HAVE_CHAMELIUM
diff --git a/lib/igt_aux.c b/lib/igt_aux.c
index ecab5d99..c55b2916 100644
--- a/lib/igt_aux.c
+++ b/lib/igt_aux.c
@@ -60,6 +60,7 @@
 #include "igt_aux.h"
 #include "igt_debugfs.h"
 #include "igt_gt.h"
+#include "igt_params.h"
 #include "igt_rand.h"
 #include "igt_sysfs.h"
 #include "config.h"
@@ -1116,148 +1117,6 @@ void igt_unlock_mem(void)
 	locked_mem = NULL;
 }
 
-
-#define MODULE_PARAM_DIR "/sys/module/i915/parameters/"
-#define PARAM_NAME_MAX_SZ 32
-#define PARAM_VALUE_MAX_SZ 16
-#define PARAM_FILE_PATH_MAX_SZ (strlen(MODULE_PARAM_DIR) + PARAM_NAME_MAX_SZ)
-
-struct module_param_data {
-	char name[PARAM_NAME_MAX_SZ];
-	char original_value[PARAM_VALUE_MAX_SZ];
-
-	struct module_param_data *next;
-};
-struct module_param_data *module_params = NULL;
-
-static void igt_module_param_exit_handler(int sig)
-{
-	const size_t dir_len = strlen(MODULE_PARAM_DIR);
-	char file_path[PARAM_FILE_PATH_MAX_SZ];
-	struct module_param_data *data;
-	int fd;
-
-	/* We don't need to assert string sizes on this function since they were
-	 * already checked before being stored on the lists. Besides,
-	 * igt_assert() is not AS-Safe. */
-	strcpy(file_path, MODULE_PARAM_DIR);
-
-	for (data = module_params; data != NULL; data = data->next) {
-		strcpy(file_path + dir_len, data->name);
-
-		fd = open(file_path, O_RDWR);
-		if (fd >= 0) {
-			int size = strlen (data->original_value);
-
-			if (size != write(fd, data->original_value, size)) {
-				const char msg[] = "WARNING: Module parameters "
-					"may not have been reset to their "
-					"original values\n";
-				assert(write(STDERR_FILENO, msg, sizeof(msg))
-				       == sizeof(msg));
-			}
-
-			close(fd);
-		}
-	}
-	/* free() is not AS-Safe, so we can't call it here. */
-}
-
-/**
- * igt_save_module_param:
- * @name: name of the i915.ko module parameter
- * @file_path: full sysfs file path for the parameter
- *
- * Reads the current value of an i915.ko module parameter, saves it on an array,
- * then installs an exit handler to restore it when the program exits.
- *
- * It is safe to call this function multiple times for the same parameter.
- *
- * Notice that this function is called by igt_set_module_param(), so that one -
- * or one of its wrappers - is the only function the test programs need to call.
- */
-static void igt_save_module_param(const char *name, const char *file_path)
-{
-	struct module_param_data *data;
-	size_t n;
-	int fd;
-
-	/* Check if this parameter is already saved. */
-	for (data = module_params; data != NULL; data = data->next)
-		if (strncmp(data->name, name, PARAM_NAME_MAX_SZ) == 0)
-			return;
-
-	if (!module_params)
-		igt_install_exit_handler(igt_module_param_exit_handler);
-
-	data = calloc(1, sizeof (*data));
-	igt_assert(data);
-
-	strncpy(data->name, name, PARAM_NAME_MAX_SZ - 1);
-
-	fd = open(file_path, O_RDONLY);
-	igt_assert(fd >= 0);
-
-	n = read(fd, data->original_value, PARAM_VALUE_MAX_SZ);
-	igt_assert_f(n > 0 && n < PARAM_VALUE_MAX_SZ,
-		     "Need to increase PARAM_VALUE_MAX_SZ\n");
-
-	igt_assert(close(fd) == 0);
-
-	data->next = module_params;
-	module_params = data;
-}
-
-/**
- * igt_set_module_param:
- * @name: i915.ko parameter name
- * @val: i915.ko parameter value
- *
- * This function sets the desired value for the given i915.ko parameter. It also
- * takes care of saving and restoring the values that were already set before
- * the test was run.
- *
- * Please consider using igt_set_module_param_int() for the integer and bool
- * parameters.
- */
-void igt_set_module_param(const char *name, const char *val)
-{
-	char file_path[PARAM_FILE_PATH_MAX_SZ];
-	size_t len = strlen(val);
-	int fd;
-
-	igt_assert_f(strlen(name) < PARAM_NAME_MAX_SZ,
-		     "Need to increase PARAM_NAME_MAX_SZ\n");
-	strcpy(file_path, MODULE_PARAM_DIR);
-	strcpy(file_path + strlen(MODULE_PARAM_DIR), name);
-
-	igt_save_module_param(name, file_path);
-
-	fd = open(file_path, O_RDWR);
-	igt_assert(write(fd, val, len) == len);
-	igt_assert(close(fd) == 0);
-}
-
-/**
- * igt_set_module_param_int:
- * @name: i915.ko parameter name
- * @val: i915.ko parameter value
- *
- * This is a wrapper for igt_set_module_param() that takes an integer instead of
- * a string. Please see igt_set_module_param().
- */
-void igt_set_module_param_int(const char *name, int val)
-{
-	char str[PARAM_VALUE_MAX_SZ];
-	int n;
-
-	n = snprintf(str, PARAM_VALUE_MAX_SZ, "%d\n", val);
-	igt_assert_f(n < PARAM_VALUE_MAX_SZ,
-		     "Need to increase PARAM_VALUE_MAX_SZ\n");
-
-	igt_set_module_param(name, str);
-}
-
 /**
  * igt_is_process_running:
  * @comm: Name of process in the form found in /proc/pid/comm (limited to 15
diff --git a/lib/igt_aux.h b/lib/igt_aux.h
index 04d22904..bf57ccf5 100644
--- a/lib/igt_aux.h
+++ b/lib/igt_aux.h
@@ -285,9 +285,6 @@ double igt_stop_siglatency(struct igt_mean *result);
 
 bool igt_allow_unlimited_files(void);
 
-void igt_set_module_param(const char *name, const char *val);
-void igt_set_module_param_int(const char *name, int val);
-
 int igt_is_process_running(const char *comm);
 int igt_terminate_process(int sig, const char *comm);
 void igt_lsof(const char *dpath);
diff --git a/lib/igt_gt.c b/lib/igt_gt.c
index 256c7cbc..22340a3d 100644
--- a/lib/igt_gt.c
+++ b/lib/igt_gt.c
@@ -35,6 +35,7 @@
 #include "igt_aux.h"
 #include "igt_core.h"
 #include "igt_gt.h"
+#include "igt_params.h"
 #include "igt_sysfs.h"
 #include "igt_debugfs.h"
 #include "ioctl_wrappers.h"
diff --git a/lib/igt_params.c b/lib/igt_params.c
new file mode 100644
index 00000000..4bd2b1f2
--- /dev/null
+++ b/lib/igt_params.c
@@ -0,0 +1,244 @@
+/*
+ * Copyright © 2019 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ */
+
+#include <assert.h>
+#include <fcntl.h>
+#include <limits.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <sys/ioctl.h>
+
+#include <i915_drm.h>
+
+#include "igt_core.h"
+#include "igt_params.h"
+#include "igt_sysfs.h"
+
+#define MODULE_PARAM_DIR "/sys/module/i915/parameters/"
+#define PARAM_NAME_MAX_SZ 32
+#define PARAM_VALUE_MAX_SZ 16
+#define PARAM_FILE_PATH_MAX_SZ (strlen(MODULE_PARAM_DIR) + PARAM_NAME_MAX_SZ)
+
+struct module_param_data {
+	char name[PARAM_NAME_MAX_SZ];
+	char original_value[PARAM_VALUE_MAX_SZ];
+
+	struct module_param_data *next;
+};
+struct module_param_data *module_params = NULL;
+
+static void igt_module_param_exit_handler(int sig)
+{
+	const size_t dir_len = strlen(MODULE_PARAM_DIR);
+	char file_path[PARAM_FILE_PATH_MAX_SZ];
+	struct module_param_data *data;
+	int fd;
+
+	/* We don't need to assert string sizes on this function since they were
+	 * already checked before being stored on the lists. Besides,
+	 * igt_assert() is not AS-Safe. */
+	strcpy(file_path, MODULE_PARAM_DIR);
+
+	for (data = module_params; data != NULL; data = data->next) {
+		strcpy(file_path + dir_len, data->name);
+
+		fd = open(file_path, O_RDWR);
+		if (fd >= 0) {
+			int size = strlen (data->original_value);
+
+			if (size != write(fd, data->original_value, size)) {
+				const char msg[] = "WARNING: Module parameters "
+					"may not have been reset to their "
+					"original values\n";
+				assert(write(STDERR_FILENO, msg, sizeof(msg))
+				       == sizeof(msg));
+			}
+
+			close(fd);
+		}
+	}
+	/* free() is not AS-Safe, so we can't call it here. */
+}
+
+/**
+ * igt_save_module_param:
+ * @name: name of the i915.ko module parameter
+ * @file_path: full sysfs file path for the parameter
+ *
+ * Reads the current value of an i915.ko module parameter, saves it on an array,
+ * then installs an exit handler to restore it when the program exits.
+ *
+ * It is safe to call this function multiple times for the same parameter.
+ *
+ * Notice that this function is called by igt_set_module_param(), so that one -
+ * or one of its wrappers - is the only function the test programs need to call.
+ */
+static void igt_save_module_param(const char *name, const char *file_path)
+{
+	struct module_param_data *data;
+	size_t n;
+	int fd;
+
+	/* Check if this parameter is already saved. */
+	for (data = module_params; data != NULL; data = data->next)
+		if (strncmp(data->name, name, PARAM_NAME_MAX_SZ) == 0)
+			return;
+
+	if (!module_params)
+		igt_install_exit_handler(igt_module_param_exit_handler);
+
+	data = calloc(1, sizeof (*data));
+	igt_assert(data);
+
+	strncpy(data->name, name, PARAM_NAME_MAX_SZ - 1);
+
+	fd = open(file_path, O_RDONLY);
+	igt_assert(fd >= 0);
+
+	n = read(fd, data->original_value, PARAM_VALUE_MAX_SZ);
+	igt_assert_f(n > 0 && n < PARAM_VALUE_MAX_SZ,
+		     "Need to increase PARAM_VALUE_MAX_SZ\n");
+
+	igt_assert(close(fd) == 0);
+
+	data->next = module_params;
+	module_params = data;
+}
+
+/**
+ * igt_sysfs_open_parameters:
+ * @device: fd of the device
+ *
+ * This opens the module parameters directory (under sysfs) corresponding
+ * to the device for use with igt_sysfs_set() and igt_sysfs_get().
+ *
+ * Returns:
+ * The directory fd, or -1 on failure.
+ */
+int igt_sysfs_open_parameters(int device)
+{
+	int dir, params = -1;
+
+	dir = igt_sysfs_open(device);
+	if (dir >= 0) {
+		params = openat(dir,
+				"device/driver/module/parameters",
+				O_RDONLY);
+		close(dir);
+	}
+
+	if (params < 0) { /* builtin? */
+		drm_version_t version;
+		char name[32] = "";
+		char path[PATH_MAX];
+
+		memset(&version, 0, sizeof(version));
+		version.name_len = sizeof(name);
+		version.name = name;
+		ioctl(device, DRM_IOCTL_VERSION, &version);
+
+		sprintf(path, "/sys/module/%s/parameters", name);
+		params = open(path, O_RDONLY);
+	}
+
+	return params;
+}
+
+/**
+ * igt_sysfs_set_parameters:
+ * @device: fd of the device
+ * @parameter: the name of the parameter to set
+ * @fmt: printf-esque format string
+ *
+ * Returns true on success
+ */
+bool igt_sysfs_set_parameter(int device,
+			     const char *parameter,
+			     const char *fmt, ...)
+{
+	va_list ap;
+	int dir;
+	int ret;
+
+	dir = igt_sysfs_open_parameters(device);
+	if (dir < 0)
+		return false;
+
+	va_start(ap, fmt);
+	ret = igt_sysfs_vprintf(dir, parameter, fmt, ap);
+	va_end(ap);
+
+	close(dir);
+
+	return ret > 0;
+}
+
+/**
+ * igt_set_module_param:
+ * @name: i915.ko parameter name
+ * @val: i915.ko parameter value
+ *
+ * This function sets the desired value for the given i915.ko parameter. It also
+ * takes care of saving and restoring the values that were already set before
+ * the test was run.
+ *
+ * Please consider using igt_set_module_param_int() for the integer and bool
+ * parameters.
+ */
+void igt_set_module_param(const char *name, const char *val)
+{
+	char file_path[PARAM_FILE_PATH_MAX_SZ];
+	size_t len = strlen(val);
+	int fd;
+
+	igt_assert_f(strlen(name) < PARAM_NAME_MAX_SZ,
+		     "Need to increase PARAM_NAME_MAX_SZ\n");
+	strcpy(file_path, MODULE_PARAM_DIR);
+	strcpy(file_path + strlen(MODULE_PARAM_DIR), name);
+
+	igt_save_module_param(name, file_path);
+
+	fd = open(file_path, O_RDWR);
+	igt_assert(write(fd, val, len) == len);
+	igt_assert(close(fd) == 0);
+}
+
+/**
+ * igt_set_module_param_int:
+ * @name: i915.ko parameter name
+ * @val: i915.ko parameter value
+ *
+ * This is a wrapper for igt_set_module_param() that takes an integer instead of
+ * a string. Please see igt_set_module_param().
+ */
+void igt_set_module_param_int(const char *name, int val)
+{
+	char str[PARAM_VALUE_MAX_SZ];
+	int n;
+
+	n = snprintf(str, PARAM_VALUE_MAX_SZ, "%d\n", val);
+	igt_assert_f(n < PARAM_VALUE_MAX_SZ,
+		     "Need to increase PARAM_VALUE_MAX_SZ\n");
+
+	igt_set_module_param(name, str);
+}
diff --git a/lib/igt_params.h b/lib/igt_params.h
new file mode 100644
index 00000000..cf0fd18f
--- /dev/null
+++ b/lib/igt_params.h
@@ -0,0 +1,38 @@
+/*
+ * Copyright © 2019 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ */
+
+#ifndef __IGT_PARAMS_H__
+#define __IGT_PARAMS_H__
+
+#include <stdbool.h>
+
+int igt_sysfs_open_parameters(int device);
+
+__attribute__((format(printf, 3, 4)))
+bool igt_sysfs_set_parameter(int device, const char *parameter,
+			     const char *fmt, ...);
+
+void igt_set_module_param(const char *name, const char *val);
+void igt_set_module_param_int(const char *name, int val);
+
+#endif /* __IGT_PARAMS_H__ */
diff --git a/lib/igt_psr.c b/lib/igt_psr.c
index 83ccacdd..2bcf14ea 100644
--- a/lib/igt_psr.c
+++ b/lib/igt_psr.c
@@ -21,6 +21,7 @@
  * IN THE SOFTWARE.
  */
 
+#include "igt_params.h"
 #include "igt_psr.h"
 #include "igt_sysfs.h"
 #include <errno.h>
diff --git a/lib/igt_sysfs.c b/lib/igt_sysfs.c
index 7528c3bd..6aafe534 100644
--- a/lib/igt_sysfs.c
+++ b/lib/igt_sysfs.c
@@ -135,74 +135,6 @@ int igt_sysfs_open(int device)
 	return open(path, O_RDONLY);
 }
 
-/**
- * igt_sysfs_set_parameters:
- * @device: fd of the device
- * @parameter: the name of the parameter to set
- * @fmt: printf-esque format string
- *
- * Returns true on success
- */
-bool igt_sysfs_set_parameter(int device,
-			     const char *parameter,
-			     const char *fmt, ...)
-{
-	va_list ap;
-	int dir;
-	int ret;
-
-	dir = igt_sysfs_open_parameters(device);
-	if (dir < 0)
-		return false;
-
-	va_start(ap, fmt);
-	ret = igt_sysfs_vprintf(dir, parameter, fmt, ap);
-	va_end(ap);
-
-	close(dir);
-
-	return ret > 0;
-}
-
-/**
- * igt_sysfs_open_parameters:
- * @device: fd of the device
- *
- * This opens the module parameters directory (under sysfs) corresponding
- * to the device for use with igt_sysfs_set() and igt_sysfs_get().
- *
- * Returns:
- * The directory fd, or -1 on failure.
- */
-int igt_sysfs_open_parameters(int device)
-{
-	int dir, params = -1;
-
-	dir = igt_sysfs_open(device);
-	if (dir >= 0) {
-		params = openat(dir,
-				"device/driver/module/parameters",
-				O_RDONLY);
-		close(dir);
-	}
-
-	if (params < 0) { /* builtin? */
-		drm_version_t version;
-		char name[32] = "";
-		char path[PATH_MAX];
-
-		memset(&version, 0, sizeof(version));
-		version.name_len = sizeof(name);
-		version.name = name;
-		ioctl(device, DRM_IOCTL_VERSION, &version);
-
-		sprintf(path, "/sys/module/%s/parameters", name);
-		params = open(path, O_RDONLY);
-	}
-
-	return params;
-}
-
 /**
  * igt_sysfs_write:
  * @dir: directory for the device from igt_sysfs_open()
diff --git a/lib/igt_sysfs.h b/lib/igt_sysfs.h
index b646df30..64935a5c 100644
--- a/lib/igt_sysfs.h
+++ b/lib/igt_sysfs.h
@@ -30,11 +30,6 @@
 
 char *igt_sysfs_path(int device, char *path, int pathlen);
 int igt_sysfs_open(int device);
-int igt_sysfs_open_parameters(int device);
-bool igt_sysfs_set_parameter(int device,
-			     const char *parameter,
-			     const char *fmt, ...)
-	__attribute__((format(printf,3,4)));
 
 int igt_sysfs_read(int dir, const char *attr, void *data, int len);
 int igt_sysfs_write(int dir, const char *attr, const void *data, int len);
diff --git a/lib/meson.build b/lib/meson.build
index e2060430..9ffb533c 100644
--- a/lib/meson.build
+++ b/lib/meson.build
@@ -16,6 +16,7 @@ lib_sources = [
 	'igt_gt.c',
 	'igt_halffloat.c',
 	'igt_matrix.c',
+	'igt_params.c',
 	'igt_perf.c',
 	'igt_primes.c',
 	'igt_rand.c',
diff --git a/tests/i915/gem_ctx_persistence.c b/tests/i915/gem_ctx_persistence.c
index 3d52987d..965b788b 100644
--- a/tests/i915/gem_ctx_persistence.c
+++ b/tests/i915/gem_ctx_persistence.c
@@ -38,6 +38,7 @@
 #include "igt_dummyload.h"
 #include "igt_gt.h"
 #include "igt_sysfs.h"
+#include "igt_params.h"
 #include "ioctl_wrappers.h" /* gem_wait()! */
 #include "sw_sync.h"
 
-- 
2.26.0

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

[igt-dev] [PATCH i-g-t 2/7] lib/params: start renaming functions igt_params_*

[Juha-Pekka Heikkila]

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
Reviewed-by: Petri Latvala <petri.latvala@intel.com>
---
 lib/drmtest.c                         |  2 +-
 lib/i915/gem_submission.c             |  2 +-
 lib/igt_aux.c                         |  3 +--
 lib/igt_gt.c                          |  2 +-
 lib/igt_params.c                      | 12 +++++-------
 lib/igt_params.h                      |  5 ++---
 tests/i915/gem_ctx_exec.c             |  2 +-
 tests/i915/gem_ctx_persistence.c      |  9 ++++-----
 tests/i915/gem_mmap_gtt.c             |  2 +-
 tests/i915/gem_reset_stats.c          |  6 ++----
 tests/i915/sysfs_heartbeat_interval.c |  3 ++-
 tests/i915/sysfs_preempt_timeout.c    |  3 ++-
 tests/i915/sysfs_timeslice_duration.c |  3 ++-
 13 files changed, 25 insertions(+), 29 deletions(-)

diff --git a/lib/drmtest.c b/lib/drmtest.c
index 1fc39925..17067843 100644
--- a/lib/drmtest.c
+++ b/lib/drmtest.c
@@ -393,7 +393,7 @@ static void __cancel_work_at_exit(int fd)
 {
 	igt_terminate_spins(); /* for older kernels */
 
-	igt_sysfs_set_parameter(fd, "reset", "%x", -1u /* any method */);
+	igt_params_set(fd, "reset", "%x", -1u /* any method */);
 	igt_drop_caches_set(fd,
 			    /* cancel everything */
 			    DROP_RESET_ACTIVE | DROP_RESET_SEQNO |
diff --git a/lib/i915/gem_submission.c b/lib/i915/gem_submission.c
index bb58a207..4d80eb6a 100644
--- a/lib/i915/gem_submission.c
+++ b/lib/i915/gem_submission.c
@@ -78,7 +78,7 @@ unsigned gem_submission_method(int fd)
 
 	int dir;
 
-	dir = igt_sysfs_open_parameters(fd);
+	dir = igt_params_open(fd);
 	if (dir < 0)
 		return 0;
 
diff --git a/lib/igt_aux.c b/lib/igt_aux.c
index c55b2916..fba34933 100644
--- a/lib/igt_aux.c
+++ b/lib/igt_aux.c
@@ -514,8 +514,7 @@ void igt_fork_hang_detector(int fd)
 	 * they are a test failure!) and so the loss of per-engine reset
 	 * functionality is not an issue.
 	 */
-	igt_assert(igt_sysfs_set_parameter
-		   (fd, "reset", "%d", 1 /* only global reset */));
+	igt_assert(igt_params_set(fd, "reset", "%d", 1 /* only global reset */));
 
 	signal(SIGIO, sig_abort);
 	igt_fork_helper(&hang_detector)
diff --git a/lib/igt_gt.c b/lib/igt_gt.c
index 22340a3d..cedb142d 100644
--- a/lib/igt_gt.c
+++ b/lib/igt_gt.c
@@ -188,7 +188,7 @@ igt_hang_t igt_allow_hang(int fd, unsigned ctx, unsigned flags)
 		__gem_context_set_param(fd, &param);
 		allow_reset = INT_MAX; /* any reset method */
 	}
-	igt_require(igt_sysfs_set_parameter(fd, "reset", "%d", allow_reset));
+	igt_require(igt_params_set(fd, "reset", "%d", allow_reset));
 
 	if (!igt_check_boolean_env_var("IGT_HANG_WITHOUT_RESET", false))
 		igt_require(has_gpu_reset(fd));
diff --git a/lib/igt_params.c b/lib/igt_params.c
index 4bd2b1f2..f220e73b 100644
--- a/lib/igt_params.c
+++ b/lib/igt_params.c
@@ -126,7 +126,7 @@ static void igt_save_module_param(const char *name, const char *file_path)
 }
 
 /**
- * igt_sysfs_open_parameters:
+ * igt_params_open:
  * @device: fd of the device
  *
  * This opens the module parameters directory (under sysfs) corresponding
@@ -135,7 +135,7 @@ static void igt_save_module_param(const char *name, const char *file_path)
  * Returns:
  * The directory fd, or -1 on failure.
  */
-int igt_sysfs_open_parameters(int device)
+int igt_params_open(int device)
 {
 	int dir, params = -1;
 
@@ -165,22 +165,20 @@ int igt_sysfs_open_parameters(int device)
 }
 
 /**
- * igt_sysfs_set_parameters:
+ * igt_params_set:
  * @device: fd of the device
  * @parameter: the name of the parameter to set
  * @fmt: printf-esque format string
  *
  * Returns true on success
  */
-bool igt_sysfs_set_parameter(int device,
-			     const char *parameter,
-			     const char *fmt, ...)
+bool igt_params_set(int device, const char *parameter, const char *fmt, ...)
 {
 	va_list ap;
 	int dir;
 	int ret;
 
-	dir = igt_sysfs_open_parameters(device);
+	dir = igt_params_open(device);
 	if (dir < 0)
 		return false;
 
diff --git a/lib/igt_params.h b/lib/igt_params.h
index cf0fd18f..52eed77f 100644
--- a/lib/igt_params.h
+++ b/lib/igt_params.h
@@ -26,11 +26,10 @@
 
 #include <stdbool.h>
 
-int igt_sysfs_open_parameters(int device);
+int igt_params_open(int device);
 
 __attribute__((format(printf, 3, 4)))
-bool igt_sysfs_set_parameter(int device, const char *parameter,
-			     const char *fmt, ...);
+bool igt_params_set(int device, const char *parameter, const char *fmt, ...);
 
 void igt_set_module_param(const char *name, const char *val);
 void igt_set_module_param_int(const char *name, int val);
diff --git a/tests/i915/gem_ctx_exec.c b/tests/i915/gem_ctx_exec.c
index ad2f9e54..bc2a352c 100644
--- a/tests/i915/gem_ctx_exec.c
+++ b/tests/i915/gem_ctx_exec.c
@@ -276,7 +276,7 @@ static void nohangcheck_hostile(int i915)
 
 	i915 = gem_reopen_driver(i915);
 
-	dir = igt_sysfs_open_parameters(i915);
+	dir = igt_params_open(i915);
 	igt_require(dir != -1);
 
 	ctx = gem_context_create(i915);
diff --git a/tests/i915/gem_ctx_persistence.c b/tests/i915/gem_ctx_persistence.c
index 965b788b..cdaa87aa 100644
--- a/tests/i915/gem_ctx_persistence.c
+++ b/tests/i915/gem_ctx_persistence.c
@@ -95,7 +95,7 @@ static void enable_hangcheck(int i915)
 {
 	int dir;
 
-	dir = igt_sysfs_open_parameters(i915);
+	dir = igt_params_open(i915);
 	if (dir < 0) /* no parameters, must be default! */
 		return;
 
@@ -361,7 +361,7 @@ static void test_nohangcheck_hostile(int i915)
 	 * we forcibly terminate that context.
 	 */
 
-	dir = igt_sysfs_open_parameters(i915);
+	dir = igt_params_open(i915);
 	igt_require(dir != -1);
 
 	igt_require(__enable_hangcheck(dir, false));
@@ -398,7 +398,7 @@ static void test_nohangcheck_hang(int i915)
 
 	igt_require(!gem_has_cmdparser(i915, ALL_ENGINES));
 
-	dir = igt_sysfs_open_parameters(i915);
+	dir = igt_params_open(i915);
 	igt_require(dir != -1);
 
 	igt_require(__enable_hangcheck(dir, false));
@@ -1090,8 +1090,7 @@ igt_main
 		igt_require_gem(i915);
 
 		/* Restore the reset modparam if left clobbered */
-		igt_assert(igt_sysfs_set_parameter
-			   (i915, "reset", "%d", -1 /* any [default] reset */));
+		igt_assert(igt_params_set(i915, "reset", "%d", -1));
 
 		enable_hangcheck(i915);
 		igt_install_exit_handler(exit_handler);
diff --git a/tests/i915/gem_mmap_gtt.c b/tests/i915/gem_mmap_gtt.c
index 1f4655af..32c92287 100644
--- a/tests/i915/gem_mmap_gtt.c
+++ b/tests/i915/gem_mmap_gtt.c
@@ -580,7 +580,7 @@ test_hang(int fd)
 	int dir;
 
 	hang = igt_allow_hang(fd, 0, 0);
-	igt_require(igt_sysfs_set_parameter(fd, "reset", "1")); /* global */
+	igt_require(igt_params_set(fd, "reset", "1")); /* global */
 
 	control = mmap(NULL, 4096, PROT_WRITE, MAP_SHARED | MAP_ANON, -1, 0);
 	igt_assert(control != MAP_FAILED);
diff --git a/tests/i915/gem_reset_stats.c b/tests/i915/gem_reset_stats.c
index 9c7b6645..8ea090d8 100644
--- a/tests/i915/gem_reset_stats.c
+++ b/tests/i915/gem_reset_stats.c
@@ -784,8 +784,7 @@ igt_main
 
 		has_reset_stats = gem_has_reset_stats(fd);
 
-		igt_assert(igt_sysfs_set_parameter
-			   (fd, "reset", "%d", 1 /* only global reset */));
+		igt_assert(igt_params_set(fd, "reset", "%d", 1 /* only global reset */));
 
 		using_full_reset = !gem_engine_reset_enabled(fd) &&
 				   gem_gpu_reset_enabled(fd);
@@ -846,8 +845,7 @@ igt_main
 		int fd;
 
 		fd = drm_open_driver(DRIVER_INTEL);
-		igt_assert(igt_sysfs_set_parameter
-			   (fd, "reset", "%d", INT_MAX /* any reset method */));
+		igt_assert(igt_params_set(fd, "reset", "%d", INT_MAX /* any reset method */));
 		close(fd);
 	}
 }
diff --git a/tests/i915/sysfs_heartbeat_interval.c b/tests/i915/sysfs_heartbeat_interval.c
index 662f4865..d1924f95 100644
--- a/tests/i915/sysfs_heartbeat_interval.c
+++ b/tests/i915/sysfs_heartbeat_interval.c
@@ -31,6 +31,7 @@
 #include <sys/wait.h>
 #include <unistd.h>
 
+#include "igt_params.h"
 #include "drmtest.h" /* gem_quiescent_gpu()! */
 #include "i915/gem_engine_topology.h"
 #include "igt_dummyload.h"
@@ -52,7 +53,7 @@ static void enable_hangcheck(int i915, bool state)
 {
 	int dir;
 
-	dir = igt_sysfs_open_parameters(i915);
+	dir = igt_params_open(i915);
 	if (dir < 0) /* no parameters, must be default! */
 		return;
 
diff --git a/tests/i915/sysfs_preempt_timeout.c b/tests/i915/sysfs_preempt_timeout.c
index a7e93a4d..8a52dfb9 100644
--- a/tests/i915/sysfs_preempt_timeout.c
+++ b/tests/i915/sysfs_preempt_timeout.c
@@ -29,6 +29,7 @@
 #include <sys/types.h>
 #include <unistd.h>
 
+#include "igt_params.h"
 #include "drmtest.h" /* gem_quiescent_gpu()! */
 #include "i915/gem_engine_topology.h"
 #include "igt_dummyload.h"
@@ -51,7 +52,7 @@ static bool enable_hangcheck(int i915, bool state)
 	bool success;
 	int dir;
 
-	dir = igt_sysfs_open_parameters(i915);
+	dir = igt_params_open(i915);
 	if (dir < 0) /* no parameters, must be default! */
 		return false;
 
diff --git a/tests/i915/sysfs_timeslice_duration.c b/tests/i915/sysfs_timeslice_duration.c
index b983df43..123b9e19 100644
--- a/tests/i915/sysfs_timeslice_duration.c
+++ b/tests/i915/sysfs_timeslice_duration.c
@@ -29,6 +29,7 @@
 #include <sys/types.h>
 #include <unistd.h>
 
+#include "igt_params.h"
 #include "drmtest.h" /* gem_quiescent_gpu()! */
 #include "i915/gem_engine_topology.h"
 #include "i915/gem_mman.h"
@@ -61,7 +62,7 @@ static bool enable_hangcheck(int i915, bool state)
 	bool success;
 	int dir;
 
-	dir = igt_sysfs_open_parameters(i915);
+	dir = igt_params_open(i915);
 	if (dir < 0) /* no parameters, must be default! */
 		return false;
 
-- 
2.26.0

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

[igt-dev] [PATCH i-g-t 3/7] lib/params: overhaul param saving

[Juha-Pekka Heikkila]

From: Jani Nikula <jani.nikula@intel.com>

More generic, use existing functions.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
---
 lib/igt_params.c | 101 +++++++++++++++++++----------------------------
 1 file changed, 41 insertions(+), 60 deletions(-)

diff --git a/lib/igt_params.c b/lib/igt_params.c
index f220e73b..b5ac1266 100644
--- a/lib/igt_params.c
+++ b/lib/igt_params.c
@@ -34,92 +34,74 @@
 #include "igt_params.h"
 #include "igt_sysfs.h"
 
-#define MODULE_PARAM_DIR "/sys/module/i915/parameters/"
-#define PARAM_NAME_MAX_SZ 32
-#define PARAM_VALUE_MAX_SZ 16
-#define PARAM_FILE_PATH_MAX_SZ (strlen(MODULE_PARAM_DIR) + PARAM_NAME_MAX_SZ)
-
 struct module_param_data {
-	char name[PARAM_NAME_MAX_SZ];
-	char original_value[PARAM_VALUE_MAX_SZ];
+	char *path;
+	char *name;
+	char *original_value;
 
 	struct module_param_data *next;
 };
 struct module_param_data *module_params = NULL;
 
-static void igt_module_param_exit_handler(int sig)
+static void igt_params_exit_handler(int sig)
 {
-	const size_t dir_len = strlen(MODULE_PARAM_DIR);
-	char file_path[PARAM_FILE_PATH_MAX_SZ];
 	struct module_param_data *data;
-	int fd;
-
-	/* We don't need to assert string sizes on this function since they were
-	 * already checked before being stored on the lists. Besides,
-	 * igt_assert() is not AS-Safe. */
-	strcpy(file_path, MODULE_PARAM_DIR);
+	int dir;
 
 	for (data = module_params; data != NULL; data = data->next) {
-		strcpy(file_path + dir_len, data->name);
-
-		fd = open(file_path, O_RDWR);
-		if (fd >= 0) {
-			int size = strlen (data->original_value);
-
-			if (size != write(fd, data->original_value, size)) {
-				const char msg[] = "WARNING: Module parameters "
-					"may not have been reset to their "
-					"original values\n";
-				assert(write(STDERR_FILENO, msg, sizeof(msg))
-				       == sizeof(msg));
-			}
-
-			close(fd);
+		dir = open(data->path, O_RDONLY);
+
+		if (!igt_sysfs_set(dir, data->name, data->original_value)) {
+			const char msg[] = "WARNING: Module parameters "
+				"may not have been reset to their "
+				"original values\n";
+			assert(write(STDERR_FILENO, msg, sizeof(msg))
+			       == sizeof(msg));
 		}
+
+		close(dir);
 	}
 	/* free() is not AS-Safe, so we can't call it here. */
 }
 
 /**
- * igt_save_module_param:
- * @name: name of the i915.ko module parameter
- * @file_path: full sysfs file path for the parameter
+ * igt_params_save:
+ * @dir: file handle for path
+ * @path: full path to the sysfs directory
+ * @name: name of the sysfs attribute
  *
- * Reads the current value of an i915.ko module parameter, saves it on an array,
- * then installs an exit handler to restore it when the program exits.
+ * Reads the current value of a sysfs attribute, saves it on an array, then
+ * installs an exit handler to restore it when the program exits.
  *
  * It is safe to call this function multiple times for the same parameter.
  *
  * Notice that this function is called by igt_set_module_param(), so that one -
  * or one of its wrappers - is the only function the test programs need to call.
  */
-static void igt_save_module_param(const char *name, const char *file_path)
+static void igt_params_save(int dir, const char *path, const char *name)
 {
 	struct module_param_data *data;
-	size_t n;
-	int fd;
 
 	/* Check if this parameter is already saved. */
 	for (data = module_params; data != NULL; data = data->next)
-		if (strncmp(data->name, name, PARAM_NAME_MAX_SZ) == 0)
+		if (strcmp(data->path, path) == 0 &&
+		    strcmp(data->name, name) == 0)
 			return;
 
 	if (!module_params)
-		igt_install_exit_handler(igt_module_param_exit_handler);
+		igt_install_exit_handler(igt_params_exit_handler);
 
 	data = calloc(1, sizeof (*data));
 	igt_assert(data);
 
-	strncpy(data->name, name, PARAM_NAME_MAX_SZ - 1);
-
-	fd = open(file_path, O_RDONLY);
-	igt_assert(fd >= 0);
+	data->path = strdup(path);
+	igt_assert(data->path);
 
-	n = read(fd, data->original_value, PARAM_VALUE_MAX_SZ);
-	igt_assert_f(n > 0 && n < PARAM_VALUE_MAX_SZ,
-		     "Need to increase PARAM_VALUE_MAX_SZ\n");
+	data->name = strdup(name);
+	igt_assert(data->name);
 
-	igt_assert(close(fd) == 0);
+	data->original_value = igt_sysfs_get(dir, name);
+	igt_assert(data->original_value);
 
 	data->next = module_params;
 	module_params = data;
@@ -205,22 +187,21 @@ bool igt_params_set(int device, const char *parameter, const char *fmt, ...)
  */
 void igt_set_module_param(const char *name, const char *val)
 {
-	char file_path[PARAM_FILE_PATH_MAX_SZ];
-	size_t len = strlen(val);
-	int fd;
+	const char *path = "/sys/module/i915/parameters";
+	int dir;
 
-	igt_assert_f(strlen(name) < PARAM_NAME_MAX_SZ,
-		     "Need to increase PARAM_NAME_MAX_SZ\n");
-	strcpy(file_path, MODULE_PARAM_DIR);
-	strcpy(file_path + strlen(MODULE_PARAM_DIR), name);
+	dir = open(path, O_RDONLY);
+	igt_assert(dir >= 0);
 
-	igt_save_module_param(name, file_path);
+	igt_params_save(dir, path, name);
 
-	fd = open(file_path, O_RDWR);
-	igt_assert(write(fd, val, len) == len);
-	igt_assert(close(fd) == 0);
+	igt_assert(igt_sysfs_set(dir, name, val));
+
+	igt_assert(close(dir) == 0);
 }
 
+#define PARAM_VALUE_MAX_SZ 16
+
 /**
  * igt_set_module_param_int:
  * @name: i915.ko parameter name
-- 
2.26.0

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

[igt-dev] [PATCH i-g-t 4/7] lib/params: add igt_params_open() which will return path

[Juha-Pekka Heikkila]

From: Jani Nikula <jani.nikula@intel.com>

Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
---
 lib/igt_params.c | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/lib/igt_params.c b/lib/igt_params.c
index b5ac1266..fe4b1df3 100644
--- a/lib/igt_params.c
+++ b/lib/igt_params.c
@@ -107,17 +107,7 @@ static void igt_params_save(int dir, const char *path, const char *name)
 	module_params = data;
 }
 
-/**
- * igt_params_open:
- * @device: fd of the device
- *
- * This opens the module parameters directory (under sysfs) corresponding
- * to the device for use with igt_sysfs_set() and igt_sysfs_get().
- *
- * Returns:
- * The directory fd, or -1 on failure.
- */
-int igt_params_open(int device)
+static int __igt_params_open(int device, char **outpath)
 {
 	int dir, params = -1;
 
@@ -141,11 +131,28 @@ int igt_params_open(int device)
 
 		sprintf(path, "/sys/module/%s/parameters", name);
 		params = open(path, O_RDONLY);
+		if (params >= 0 && outpath)
+			*outpath = strdup(path);
 	}
 
 	return params;
 }
 
+/**
+ * igt_params_open:
+ * @device: fd of the device
+ *
+ * This opens the module parameters directory (under sysfs) corresponding
+ * to the device for use with igt_sysfs_set() and igt_sysfs_get().
+ *
+ * Returns:
+ * The directory fd, or -1 on failure.
+ */
+int igt_params_open(int device)
+{
+	return __igt_params_open(device, NULL);
+}
+
 /**
  * igt_params_set:
  * @device: fd of the device
-- 
2.26.0

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

[igt-dev] [PATCH i-g-t 5/7] igt/params: add generic saving module parameter set

[Juha-Pekka Heikkila]

From: Jani Nikula <jani.nikula@intel.com>

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
---
 lib/igt_params.c | 57 ++++++++++++++++++++++++++++++++++++++++--------
 lib/igt_params.h |  3 +++
 2 files changed, 51 insertions(+), 9 deletions(-)

diff --git a/lib/igt_params.c b/lib/igt_params.c
index fe4b1df3..d9cf986c 100644
--- a/lib/igt_params.c
+++ b/lib/igt_params.c
@@ -153,6 +153,29 @@ int igt_params_open(int device)
 	return __igt_params_open(device, NULL);
 }
 
+__attribute__((format(printf, 3, 0)))
+static bool __igt_params_set(int device, const char *parameter,
+			     const char *fmt, va_list ap, bool save)
+{
+	char *path = NULL;
+	int dir;
+	int ret;
+
+	dir = __igt_params_open(device, save ? &path : NULL);
+	if (dir < 0)
+		return false;
+
+	if (save)
+		igt_params_save(dir, path, parameter);
+
+	ret = igt_sysfs_vprintf(dir, parameter, fmt, ap);
+
+	close(dir);
+	free(path);
+
+	return ret > 0;
+}
+
 /**
  * igt_params_set:
  * @device: fd of the device
@@ -164,20 +187,36 @@ int igt_params_open(int device)
 bool igt_params_set(int device, const char *parameter, const char *fmt, ...)
 {
 	va_list ap;
-	int dir;
-	int ret;
-
-	dir = igt_params_open(device);
-	if (dir < 0)
-		return false;
+	bool ret;
 
 	va_start(ap, fmt);
-	ret = igt_sysfs_vprintf(dir, parameter, fmt, ap);
+	ret = __igt_params_set(device, parameter, fmt, ap, false);
 	va_end(ap);
 
-	close(dir);
+	return ret;
+}
 
-	return ret > 0;
+/**
+ * igt_params_set_save:
+ * @device: fd of the device or -1 to default to first matching starting from
+ *	    zero. See __igt_params_open()
+ * @parameter: the name of the parameter to set
+ * @fmt: printf-esque format string
+ *
+ * Save the original value to be restored.
+ *
+ * Returns true on success
+ */
+bool igt_params_set_save(int device, const char *parameter, const char *fmt, ...)
+{
+	va_list ap;
+	bool ret;
+
+	va_start(ap, fmt);
+	ret = __igt_params_set(device, parameter, fmt, ap, true);
+	va_end(ap);
+
+	return ret;
 }
 
 /**
diff --git a/lib/igt_params.h b/lib/igt_params.h
index 52eed77f..1b750d23 100644
--- a/lib/igt_params.h
+++ b/lib/igt_params.h
@@ -31,6 +31,9 @@ int igt_params_open(int device);
 __attribute__((format(printf, 3, 4)))
 bool igt_params_set(int device, const char *parameter, const char *fmt, ...);
 
+__attribute__((format(printf, 3, 4)))
+bool igt_params_set_save(int device, const char *parameter, const char *fmt, ...);
+
 void igt_set_module_param(const char *name, const char *val);
 void igt_set_module_param_int(const char *name, int val);
 
-- 
2.26.0

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

[igt-dev] [PATCH i-g-t 6/7] igt/params: use igt_params_set_save for igt_set_module_param*

[Juha-Pekka Heikkila]

Unify parameter access. Here is also decided 'default' device
if -1 is specified as device. Derault being that device
which is first to match requirements, starting search
from zero upwards.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
---
 lib/igt_params.c | 134 +++++++++++++++++++++++++++++++++++------------
 1 file changed, 100 insertions(+), 34 deletions(-)

diff --git a/lib/igt_params.c b/lib/igt_params.c
index d9cf986c..dc35f233 100644
--- a/lib/igt_params.c
+++ b/lib/igt_params.c
@@ -27,12 +27,14 @@
 #include <stdbool.h>
 #include <stdio.h>
 #include <sys/ioctl.h>
+#include <sys/stat.h>
 
 #include <i915_drm.h>
 
 #include "igt_core.h"
 #include "igt_params.h"
 #include "igt_sysfs.h"
+#include "igt_debugfs.h"
 
 struct module_param_data {
 	char *path;
@@ -107,15 +109,59 @@ static void igt_params_save(int dir, const char *path, const char *name)
 	module_params = data;
 }
 
-static int __igt_params_open(int device, char **outpath)
+static int __igt_params_open(int device, char **outpath, const char *param)
 {
 	int dir, params = -1;
+	struct stat buffer;
+	char searchname[64];
+	char searchpath[PATH_MAX];
+	char *foundname, *ctx;
 
-	dir = igt_sysfs_open(device);
+	dir = igt_debugfs_dir(device);
 	if (dir >= 0) {
-		params = openat(dir,
-				"device/driver/module/parameters",
-				O_RDONLY);
+		int devname;
+
+		devname = openat(dir, "name", O_RDONLY);
+		igt_assert(devname >= 0);
+
+		read(devname, searchname, sizeof(searchname));
+		close(devname);
+
+		foundname = strtok_r(searchname, " ", &ctx);
+		igt_assert(foundname);
+
+		snprintf(searchpath, PATH_MAX, "%s_params", foundname);
+		params = openat(dir, searchpath, O_RDONLY);
+
+		if (params >= 0) {
+			char *debugfspath = malloc(PATH_MAX);
+
+			igt_debugfs_path(device, debugfspath, PATH_MAX);
+			if (param != NULL) {
+				char filepath[PATH_MAX];
+
+				snprintf(filepath, PATH_MAX, "%s/%s",
+					 debugfspath, param);
+
+				if (stat(filepath, &buffer) == 0) {
+					if (outpath != NULL)
+						*outpath = debugfspath;
+					else
+						free(debugfspath);
+				} else {
+					free(debugfspath);
+					close(params);
+					params = -1;
+				}
+			} else if (outpath != NULL) {
+				/*
+				 * Caller is responsible to free this.
+				 */
+				*outpath = debugfspath;
+			} else {
+				free(debugfspath);
+			}
+		}
 		close(dir);
 	}
 
@@ -124,12 +170,51 @@ static int __igt_params_open(int device, char **outpath)
 		char name[32] = "";
 		char path[PATH_MAX];
 
-		memset(&version, 0, sizeof(version));
-		version.name_len = sizeof(name);
-		version.name = name;
-		ioctl(device, DRM_IOCTL_VERSION, &version);
-
-		sprintf(path, "/sys/module/%s/parameters", name);
+		if (device == -1) {
+			/*
+			 * find default device
+			 */
+			int file, i;
+			const char *debugfs_root = igt_debugfs_mount();
+
+			igt_assert(debugfs_root);
+
+			for (i = 0; i < 63; i++) {
+				char testpath[PATH_MAX];
+
+				snprintf(searchpath, PATH_MAX,
+					 "%s/dri/%d/name", debugfs_root, i);
+
+				file = open(searchpath, O_RDONLY);
+
+				if (file < 0)
+					continue;
+
+				read(file, searchname, sizeof(searchname));
+				close(file);
+
+				foundname = strtok_r(searchname, " ", &ctx);
+				if (!foundname)
+					continue;
+
+				snprintf(testpath, PATH_MAX,
+					 "/sys/module/%s/parameters",
+					 foundname);
+
+				if (stat(testpath, &buffer) == 0 &&
+				    S_ISDIR(buffer.st_mode)) {
+					snprintf(name, sizeof(name), "%s",
+						 foundname);
+					break;
+				}
+			}
+		} else {
+			memset(&version, 0, sizeof(version));
+			version.name_len = sizeof(name);
+			version.name = name;
+			ioctl(device, DRM_IOCTL_VERSION, &version);
+		}
+		snprintf(path, sizeof(path), "/sys/module/%s/parameters", name);
 		params = open(path, O_RDONLY);
 		if (params >= 0 && outpath)
 			*outpath = strdup(path);
@@ -150,7 +235,7 @@ static int __igt_params_open(int device, char **outpath)
  */
 int igt_params_open(int device)
 {
-	return __igt_params_open(device, NULL);
+	return __igt_params_open(device, NULL, NULL);
 }
 
 __attribute__((format(printf, 3, 0)))
@@ -161,7 +246,7 @@ static bool __igt_params_set(int device, const char *parameter,
 	int dir;
 	int ret;
 
-	dir = __igt_params_open(device, save ? &path : NULL);
+	dir = __igt_params_open(device, save ? &path : NULL, parameter);
 	if (dir < 0)
 		return false;
 
@@ -233,21 +318,9 @@ bool igt_params_set_save(int device, const char *parameter, const char *fmt, ...
  */
 void igt_set_module_param(const char *name, const char *val)
 {
-	const char *path = "/sys/module/i915/parameters";
-	int dir;
-
-	dir = open(path, O_RDONLY);
-	igt_assert(dir >= 0);
-
-	igt_params_save(dir, path, name);
-
-	igt_assert(igt_sysfs_set(dir, name, val));
-
-	igt_assert(close(dir) == 0);
+	igt_assert(igt_params_set_save(-1, name, "%s", val));
 }
 
-#define PARAM_VALUE_MAX_SZ 16
-
 /**
  * igt_set_module_param_int:
  * @name: i915.ko parameter name
@@ -258,12 +331,5 @@ void igt_set_module_param(const char *name, const char *val)
  */
 void igt_set_module_param_int(const char *name, int val)
 {
-	char str[PARAM_VALUE_MAX_SZ];
-	int n;
-
-	n = snprintf(str, PARAM_VALUE_MAX_SZ, "%d\n", val);
-	igt_assert_f(n < PARAM_VALUE_MAX_SZ,
-		     "Need to increase PARAM_VALUE_MAX_SZ\n");
-
-	igt_set_module_param(name, str);
+	igt_assert(igt_params_set_save(-1, name, "%d", val));
 }
-- 
2.26.0

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

[igt-dev] [PATCH i-g-t 7/7] tests/gem_eio: switch to using igt_params_set()

[Juha-Pekka Heikkila]

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
---
 tests/i915/gem_eio.c | 57 +++++++++++++++++++-------------------------
 1 file changed, 24 insertions(+), 33 deletions(-)

diff --git a/tests/i915/gem_eio.c b/tests/i915/gem_eio.c
index 1ec60941..ad59f83a 100644
--- a/tests/i915/gem_eio.c
+++ b/tests/i915/gem_eio.c
@@ -52,20 +52,11 @@
 
 IGT_TEST_DESCRIPTION("Test that specific ioctls report a wedged GPU (EIO).");
 
-static bool i915_reset_control(bool enable)
+static bool i915_reset_control(int fd, bool enable)
 {
-	const char *path = "/sys/module/i915/parameters/reset";
-	int fd, ret;
-
 	igt_debug("%s GPU reset\n", enable ? "Enabling" : "Disabling");
 
-	fd = open(path, O_RDWR);
-	igt_require(fd >= 0);
-
-	ret = write(fd, &"01"[enable], 1) == 1;
-	close(fd);
-
-	return ret;
+	return igt_params_set(fd, "reset", "%d", enable);
 }
 
 static void trigger_reset(int fd)
@@ -106,9 +97,9 @@ static void wedge_gpu(int fd)
 	/* First idle the GPU then disable GPU resets before injecting a hang */
 	gem_quiescent_gpu(fd);
 
-	igt_require(i915_reset_control(false));
+	igt_require(i915_reset_control(fd, false));
 	manual_hang(fd);
-	igt_assert(i915_reset_control(true));
+	igt_assert(i915_reset_control(fd, true));
 }
 
 static int __gem_throttle(int fd)
@@ -346,7 +337,7 @@ static void __test_banned(int fd)
 	gem_write(fd, obj.handle, 0, &bbe, sizeof(bbe));
 
 	gem_quiescent_gpu(fd);
-	igt_require(i915_reset_control(true));
+	igt_require(i915_reset_control(fd, true));
 
 	igt_until_timeout(5) {
 		igt_spin_t *hang;
@@ -404,9 +395,9 @@ static void test_wait(int fd, unsigned int flags, unsigned int wait)
 	 */
 
 	if (flags & TEST_WEDGE)
-		igt_require(i915_reset_control(false));
+		igt_require(i915_reset_control(fd, false));
 	else
-		igt_require(i915_reset_control(true));
+		igt_require(i915_reset_control(fd, true));
 
 	hang = spin_sync(fd, 0, I915_EXEC_DEFAULT);
 
@@ -415,7 +406,7 @@ static void test_wait(int fd, unsigned int flags, unsigned int wait)
 
 	igt_spin_free(fd, hang);
 
-	igt_require(i915_reset_control(true));
+	igt_require(i915_reset_control(fd, true));
 
 	trigger_reset(fd);
 	close(fd);
@@ -430,12 +421,12 @@ static void test_suspend(int fd, int state)
 	igt_system_suspend_autoresume(state, SUSPEND_TEST_DEVICES);
 
 	/* Check we can suspend when the driver is already wedged */
-	igt_require(i915_reset_control(false));
+	igt_require(i915_reset_control(fd, false));
 	manual_hang(fd);
 
 	igt_system_suspend_autoresume(state, SUSPEND_TEST_DEVICES);
 
-	igt_require(i915_reset_control(true));
+	igt_require(i915_reset_control(fd, true));
 	trigger_reset(fd);
 	close(fd);
 }
@@ -470,7 +461,7 @@ static void test_inflight(int fd, unsigned int wait)
 
 		gem_quiescent_gpu(fd);
 		igt_debug("Starting %s on engine '%s'\n", __func__, e->name);
-		igt_require(i915_reset_control(false));
+		igt_require(i915_reset_control(fd, false));
 
 		hang = spin_sync(fd, 0, eb_ring(e));
 		obj[0].handle = hang->handle;
@@ -495,7 +486,7 @@ static void test_inflight(int fd, unsigned int wait)
 		}
 		igt_spin_free(fd, hang);
 
-		igt_assert(i915_reset_control(true));
+		igt_assert(i915_reset_control(fd, true));
 		trigger_reset(fd);
 
 		gem_close(fd, obj[1].handle);
@@ -520,7 +511,7 @@ static void test_inflight_suspend(int fd)
 	fd = gem_reopen_driver(fd);
 	igt_require_gem(fd);
 	igt_require(gem_has_exec_fence(fd));
-	igt_require(i915_reset_control(false));
+	igt_require(i915_reset_control(fd, false));
 
 	memset(obj, 0, sizeof(obj));
 	obj[0].flags = EXEC_OBJECT_WRITE;
@@ -553,7 +544,7 @@ static void test_inflight_suspend(int fd)
 	}
 	igt_spin_free(fd, hang);
 
-	igt_assert(i915_reset_control(true));
+	igt_assert(i915_reset_control(fd, true));
 	trigger_reset(fd);
 	close(fd);
 }
@@ -601,7 +592,7 @@ static void test_inflight_contexts(int fd, unsigned int wait)
 		gem_quiescent_gpu(fd);
 
 		igt_debug("Starting %s on engine '%s'\n", __func__, e->name);
-		igt_require(i915_reset_control(false));
+		igt_require(i915_reset_control(fd, false));
 
 		memset(obj, 0, sizeof(obj));
 		obj[0].flags = EXEC_OBJECT_WRITE;
@@ -636,7 +627,7 @@ static void test_inflight_contexts(int fd, unsigned int wait)
 		igt_spin_free(fd, hang);
 		gem_close(fd, obj[1].handle);
 
-		igt_assert(i915_reset_control(true));
+		igt_assert(i915_reset_control(fd, true));
 		trigger_reset(fd);
 
 		for (unsigned int n = 0; n < ARRAY_SIZE(ctx); n++)
@@ -663,7 +654,7 @@ static void test_inflight_external(int fd)
 
 	fence = igt_cork_plug(&cork, fd);
 
-	igt_require(i915_reset_control(false));
+	igt_require(i915_reset_control(fd, false));
 	hang = __spin_poll(fd, 0, 0);
 
 	memset(&obj, 0, sizeof(obj));
@@ -695,7 +686,7 @@ static void test_inflight_external(int fd)
 	close(fence);
 
 	igt_spin_free(fd, hang);
-	igt_assert(i915_reset_control(true));
+	igt_assert(i915_reset_control(fd, true));
 	trigger_reset(fd);
 	close(fd);
 }
@@ -714,7 +705,7 @@ static void test_inflight_internal(int fd, unsigned int wait)
 	fd = gem_reopen_driver(fd);
 	igt_require_gem(fd);
 
-	igt_require(i915_reset_control(false));
+	igt_require(i915_reset_control(fd, false));
 	hang = spin_sync(fd, 0, 0);
 
 	memset(obj, 0, sizeof(obj));
@@ -745,7 +736,7 @@ static void test_inflight_internal(int fd, unsigned int wait)
 	}
 	igt_spin_free(fd, hang);
 
-	igt_assert(i915_reset_control(true));
+	igt_assert(i915_reset_control(fd, true));
 	trigger_reset(fd);
 	close(fd);
 }
@@ -781,7 +772,7 @@ static void reset_stress(int fd, uint32_t ctx0,
 
 		gem_quiescent_gpu(fd);
 
-		igt_require(i915_reset_control(flags & TEST_WEDGE ?
+		igt_require(i915_reset_control(fd, flags & TEST_WEDGE ?
 					       false : true));
 
 		/*
@@ -803,7 +794,7 @@ static void reset_stress(int fd, uint32_t ctx0,
 		igt_assert_eq(sync_fence_status(hang->out_fence), -EIO);
 
 		/* Unwedge by forcing a reset. */
-		igt_assert(i915_reset_control(true));
+		igt_assert(i915_reset_control(fd, true));
 		trigger_reset(fd);
 
 		gem_quiescent_gpu(fd);
@@ -918,7 +909,7 @@ static int fd = -1;
 static void
 exit_handler(int sig)
 {
-	i915_reset_control(true);
+	i915_reset_control(fd, true);
 	igt_force_gpu_reset(fd);
 }
 
@@ -933,7 +924,7 @@ igt_main
 
 		igt_allow_hang(fd, 0, 0);
 
-		igt_require(i915_reset_control(true));
+		igt_require(i915_reset_control(fd, true));
 		igt_force_gpu_reset(fd);
 		igt_install_exit_handler(exit_handler);
 	}
-- 
2.26.0

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

[igt-dev] ✓ Fi.CI.BAT: success for Use device dependant module parameters (rev5)

[Patchwork]

== Series Details ==

Series: Use device dependant module parameters (rev5)
URL   : https://patchwork.freedesktop.org/series/76163/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_8387 -> IGTPW_4513
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

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

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

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

### IGT changes ###

#### Possible fixes ####

  * igt@i915_selftest@live@execlists:
    - {fi-tgl-dsi}:       [DMESG-FAIL][1] ([i915#656]) -> [PASS][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8387/fi-tgl-dsi/igt@i915_selftest@live@execlists.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4513/fi-tgl-dsi/igt@i915_selftest@live@execlists.html

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

  [i915#656]: https://gitlab.freedesktop.org/drm/intel/issues/656


Participating hosts (50 -> 43)
------------------------------

  Missing    (7): fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-kbl-7500u fi-ctg-p8600 fi-byt-clapper fi-bdw-samus 


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

  * CI: CI-20190529 -> None
  * IGT: IGT_5614 -> IGTPW_4513

  CI-20190529: 20190529
  CI_DRM_8387: 2f06b2b1d33997d1fb16564959f0ebee8a26b632 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_4513: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4513/index.html
  IGT_5614: d095827add11d4e8158b87683971ee659749d9a4 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4513/index.html
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] ✓ Fi.CI.IGT: success for Use device dependant module parameters (rev5)

[Patchwork]

== Series Details ==

Series: Use device dependant module parameters (rev5)
URL   : https://patchwork.freedesktop.org/series/76163/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_8387_full -> IGTPW_4513_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

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

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

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

### IGT changes ###

#### Issues hit ####

  * igt@i915_suspend@forcewake:
    - shard-kbl:          [PASS][1] -> [DMESG-WARN][2] ([i915#180])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8387/shard-kbl6/igt@i915_suspend@forcewake.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4513/shard-kbl1/igt@i915_suspend@forcewake.html

  * igt@kms_cursor_crc@pipe-a-cursor-64x64-onscreen:
    - shard-kbl:          [PASS][3] -> [FAIL][4] ([i915#54] / [i915#93] / [i915#95])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8387/shard-kbl7/igt@kms_cursor_crc@pipe-a-cursor-64x64-onscreen.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4513/shard-kbl7/igt@kms_cursor_crc@pipe-a-cursor-64x64-onscreen.html

  * igt@kms_cursor_crc@pipe-c-cursor-suspend:
    - shard-apl:          [PASS][5] -> [DMESG-WARN][6] ([i915#180]) +4 similar issues
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8387/shard-apl8/igt@kms_cursor_crc@pipe-c-cursor-suspend.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4513/shard-apl6/igt@kms_cursor_crc@pipe-c-cursor-suspend.html

  * igt@kms_cursor_legacy@2x-long-flip-vs-cursor-legacy:
    - shard-glk:          [PASS][7] -> [FAIL][8] ([i915#72])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8387/shard-glk8/igt@kms_cursor_legacy@2x-long-flip-vs-cursor-legacy.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4513/shard-glk1/igt@kms_cursor_legacy@2x-long-flip-vs-cursor-legacy.html

  * igt@kms_draw_crc@draw-method-rgb565-blt-ytiled:
    - shard-glk:          [PASS][9] -> [FAIL][10] ([i915#52] / [i915#54]) +1 similar issue
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8387/shard-glk5/igt@kms_draw_crc@draw-method-rgb565-blt-ytiled.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4513/shard-glk7/igt@kms_draw_crc@draw-method-rgb565-blt-ytiled.html

  * igt@kms_flip_tiling@flip-changes-tiling-yf:
    - shard-kbl:          [PASS][11] -> [FAIL][12] ([i915#699] / [i915#93] / [i915#95])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8387/shard-kbl1/igt@kms_flip_tiling@flip-changes-tiling-yf.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4513/shard-kbl2/igt@kms_flip_tiling@flip-changes-tiling-yf.html
    - shard-apl:          [PASS][13] -> [FAIL][14] ([i915#95])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8387/shard-apl6/igt@kms_flip_tiling@flip-changes-tiling-yf.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4513/shard-apl4/igt@kms_flip_tiling@flip-changes-tiling-yf.html

  * igt@kms_mmap_write_crc@main:
    - shard-kbl:          [PASS][15] -> [FAIL][16] ([i915#93] / [i915#95])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8387/shard-kbl1/igt@kms_mmap_write_crc@main.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4513/shard-kbl6/igt@kms_mmap_write_crc@main.html

  * igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min:
    - shard-kbl:          [PASS][17] -> [FAIL][18] ([fdo#108145] / [i915#265] / [i915#93] / [i915#95])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8387/shard-kbl4/igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4513/shard-kbl4/igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min.html
    - shard-apl:          [PASS][19] -> [FAIL][20] ([fdo#108145] / [i915#265] / [i915#95])
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8387/shard-apl6/igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4513/shard-apl6/igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min.html

  * igt@kms_psr@psr2_primary_blt:
    - shard-iclb:         [PASS][21] -> [SKIP][22] ([fdo#109441])
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8387/shard-iclb2/igt@kms_psr@psr2_primary_blt.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4513/shard-iclb6/igt@kms_psr@psr2_primary_blt.html

  
#### Possible fixes ####

  * igt@gem_ctx_persistence@legacy-engines-mixed-process@vebox:
    - shard-apl:          [FAIL][23] ([i915#1528]) -> [PASS][24]
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8387/shard-apl3/igt@gem_ctx_persistence@legacy-engines-mixed-process@vebox.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4513/shard-apl1/igt@gem_ctx_persistence@legacy-engines-mixed-process@vebox.html

  * igt@gem_exec_whisper@basic-queues-forked:
    - shard-kbl:          [FAIL][25] ([i915#1479] / [i915#1772]) -> [PASS][26]
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8387/shard-kbl2/igt@gem_exec_whisper@basic-queues-forked.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4513/shard-kbl1/igt@gem_exec_whisper@basic-queues-forked.html

  * igt@kms_cursor_crc@pipe-a-cursor-128x128-offscreen:
    - shard-kbl:          [FAIL][27] ([i915#54] / [i915#93] / [i915#95]) -> [PASS][28] +2 similar issues
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8387/shard-kbl3/igt@kms_cursor_crc@pipe-a-cursor-128x128-offscreen.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4513/shard-kbl1/igt@kms_cursor_crc@pipe-a-cursor-128x128-offscreen.html

  * igt@kms_cursor_legacy@flip-vs-cursor-crc-atomic:
    - shard-kbl:          [FAIL][29] ([i915#1566] / [i915#93] / [i915#95]) -> [PASS][30]
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8387/shard-kbl4/igt@kms_cursor_legacy@flip-vs-cursor-crc-atomic.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4513/shard-kbl6/igt@kms_cursor_legacy@flip-vs-cursor-crc-atomic.html

  * igt@kms_cursor_legacy@pipe-b-torture-move:
    - shard-iclb:         [DMESG-WARN][31] ([i915#128]) -> [PASS][32]
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8387/shard-iclb8/igt@kms_cursor_legacy@pipe-b-torture-move.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4513/shard-iclb1/igt@kms_cursor_legacy@pipe-b-torture-move.html

  * igt@kms_draw_crc@draw-method-rgb565-pwrite-ytiled:
    - shard-glk:          [FAIL][33] ([i915#52] / [i915#54]) -> [PASS][34] +3 similar issues
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8387/shard-glk4/igt@kms_draw_crc@draw-method-rgb565-pwrite-ytiled.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4513/shard-glk4/igt@kms_draw_crc@draw-method-rgb565-pwrite-ytiled.html

  * igt@kms_draw_crc@draw-method-xrgb8888-mmap-cpu-untiled:
    - shard-kbl:          [FAIL][35] ([i915#177] / [i915#52] / [i915#54] / [i915#93] / [i915#95]) -> [PASS][36]
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8387/shard-kbl2/igt@kms_draw_crc@draw-method-xrgb8888-mmap-cpu-untiled.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4513/shard-kbl6/igt@kms_draw_crc@draw-method-xrgb8888-mmap-cpu-untiled.html
    - shard-apl:          [FAIL][37] ([i915#52] / [i915#54] / [i915#95]) -> [PASS][38]
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8387/shard-apl3/igt@kms_draw_crc@draw-method-xrgb8888-mmap-cpu-untiled.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4513/shard-apl2/igt@kms_draw_crc@draw-method-xrgb8888-mmap-cpu-untiled.html

  * igt@kms_draw_crc@draw-method-xrgb8888-mmap-wc-untiled:
    - shard-kbl:          [FAIL][39] ([fdo#108145] / [i915#177] / [i915#52] / [i915#54] / [i915#93] / [i915#95]) -> [PASS][40]
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8387/shard-kbl1/igt@kms_draw_crc@draw-method-xrgb8888-mmap-wc-untiled.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4513/shard-kbl7/igt@kms_draw_crc@draw-method-xrgb8888-mmap-wc-untiled.html
    - shard-apl:          [FAIL][41] ([fdo#108145] / [i915#52] / [i915#54] / [i915#95]) -> [PASS][42]
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8387/shard-apl6/igt@kms_draw_crc@draw-method-xrgb8888-mmap-wc-untiled.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4513/shard-apl3/igt@kms_draw_crc@draw-method-xrgb8888-mmap-wc-untiled.html

  * {igt@kms_flip@flip-vs-expired-vblank@a-hdmi-a1}:
    - shard-glk:          [FAIL][43] ([i915#79]) -> [PASS][44]
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8387/shard-glk4/igt@kms_flip@flip-vs-expired-vblank@a-hdmi-a1.html
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4513/shard-glk2/igt@kms_flip@flip-vs-expired-vblank@a-hdmi-a1.html

  * {igt@kms_flip@flip-vs-suspend-interruptible@a-dp1}:
    - shard-kbl:          [DMESG-WARN][45] ([i915#180]) -> [PASS][46] +2 similar issues
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8387/shard-kbl1/igt@kms_flip@flip-vs-suspend-interruptible@a-dp1.html
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4513/shard-kbl4/igt@kms_flip@flip-vs-suspend-interruptible@a-dp1.html

  * igt@kms_flip_tiling@flip-changes-tiling-y:
    - shard-kbl:          [FAIL][47] ([i915#699] / [i915#93] / [i915#95]) -> [PASS][48]
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8387/shard-kbl2/igt@kms_flip_tiling@flip-changes-tiling-y.html
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4513/shard-kbl4/igt@kms_flip_tiling@flip-changes-tiling-y.html

  * igt@kms_frontbuffer_tracking@fbc-suspend:
    - shard-apl:          [DMESG-WARN][49] ([i915#180] / [i915#95]) -> [PASS][50]
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8387/shard-apl2/igt@kms_frontbuffer_tracking@fbc-suspend.html
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4513/shard-apl4/igt@kms_frontbuffer_tracking@fbc-suspend.html

  * igt@kms_plane_cursor@pipe-a-viewport-size-64:
    - shard-kbl:          [FAIL][51] ([i915#1559] / [i915#93] / [i915#95]) -> [PASS][52]
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8387/shard-kbl7/igt@kms_plane_cursor@pipe-a-viewport-size-64.html
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4513/shard-kbl6/igt@kms_plane_cursor@pipe-a-viewport-size-64.html
    - shard-apl:          [FAIL][53] ([i915#1559] / [i915#95]) -> [PASS][54]
   [53]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8387/shard-apl2/igt@kms_plane_cursor@pipe-a-viewport-size-64.html
   [54]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4513/shard-apl6/igt@kms_plane_cursor@pipe-a-viewport-size-64.html

  * igt@kms_psr2_su@frontbuffer:
    - shard-iclb:         [SKIP][55] ([fdo#109642] / [fdo#111068]) -> [PASS][56]
   [55]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8387/shard-iclb6/igt@kms_psr2_su@frontbuffer.html
   [56]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4513/shard-iclb2/igt@kms_psr2_su@frontbuffer.html

  * igt@kms_psr@psr2_dpms:
    - shard-iclb:         [SKIP][57] ([fdo#109441]) -> [PASS][58] +1 similar issue
   [57]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8387/shard-iclb7/igt@kms_psr@psr2_dpms.html
   [58]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4513/shard-iclb2/igt@kms_psr@psr2_dpms.html

  * igt@kms_vblank@pipe-a-ts-continuation-suspend:
    - shard-apl:          [DMESG-WARN][59] ([i915#180]) -> [PASS][60] +5 similar issues
   [59]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8387/shard-apl3/igt@kms_vblank@pipe-a-ts-continuation-suspend.html
   [60]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4513/shard-apl6/igt@kms_vblank@pipe-a-ts-continuation-suspend.html

  * {igt@perf@blocking-parameterized}:
    - shard-kbl:          [FAIL][61] ([i915#1542]) -> [PASS][62]
   [61]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8387/shard-kbl7/igt@perf@blocking-parameterized.html
   [62]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4513/shard-kbl2/igt@perf@blocking-parameterized.html

  * igt@perf@polling:
    - shard-glk:          [SKIP][63] ([fdo#109271]) -> [PASS][64]
   [63]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8387/shard-glk7/igt@perf@polling.html
   [64]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4513/shard-glk4/igt@perf@polling.html
    - shard-tglb:         [SKIP][65] ([i915#405]) -> [PASS][66]
   [65]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8387/shard-tglb8/igt@perf@polling.html
   [66]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4513/shard-tglb7/igt@perf@polling.html
    - shard-apl:          [SKIP][67] ([fdo#109271]) -> [PASS][68]
   [67]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8387/shard-apl7/igt@perf@polling.html
   [68]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4513/shard-apl6/igt@perf@polling.html
    - shard-kbl:          [SKIP][69] ([fdo#109271]) -> [PASS][70]
   [69]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8387/shard-kbl3/igt@perf@polling.html
   [70]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4513/shard-kbl4/igt@perf@polling.html
    - shard-hsw:          [SKIP][71] ([fdo#109271]) -> [PASS][72]
   [71]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8387/shard-hsw7/igt@perf@polling.html
   [72]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4513/shard-hsw4/igt@perf@polling.html
    - shard-iclb:         [SKIP][73] ([i915#405]) -> [PASS][74]
   [73]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8387/shard-iclb1/igt@perf@polling.html
   [74]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4513/shard-iclb6/igt@perf@polling.html

  * {igt@perf@polling-parameterized}:
    - shard-hsw:          [FAIL][75] ([i915#1542]) -> [PASS][76]
   [75]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8387/shard-hsw1/igt@perf@polling-parameterized.html
   [76]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4513/shard-hsw2/igt@perf@polling-parameterized.html

  
#### Warnings ####

  * igt@kms_fbcon_fbt@fbc-suspend:
    - shard-kbl:          [FAIL][77] ([i915#93] / [i915#95]) -> [DMESG-FAIL][78] ([i915#180] / [i915#95])
   [77]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8387/shard-kbl1/igt@kms_fbcon_fbt@fbc-suspend.html
   [78]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4513/shard-kbl1/igt@kms_fbcon_fbt@fbc-suspend.html

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

  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
  [fdo#109642]: https://bugs.freedesktop.org/show_bug.cgi?id=109642
  [fdo#111068]: https://bugs.freedesktop.org/show_bug.cgi?id=111068
  [i915#128]: https://gitlab.freedesktop.org/drm/intel/issues/128
  [i915#1479]: https://gitlab.freedesktop.org/drm/intel/issues/1479
  [i915#1528]: https://gitlab.freedesktop.org/drm/intel/issues/1528
  [i915#1542]: https://gitlab.freedesktop.org/drm/intel/issues/1542
  [i915#1559]: https://gitlab.freedesktop.org/drm/intel/issues/1559
  [i915#1566]: https://gitlab.freedesktop.org/drm/intel/issues/1566
  [i915#177]: https://gitlab.freedesktop.org/drm/intel/issues/177
  [i915#1772]: https://gitlab.freedesktop.org/drm/intel/issues/1772
  [i915#180]: https://gitlab.freedesktop.org/drm/intel/issues/180
  [i915#265]: https://gitlab.freedesktop.org/drm/intel/issues/265
  [i915#405]: https://gitlab.freedesktop.org/drm/intel/issues/405
  [i915#52]: https://gitlab.freedesktop.org/drm/intel/issues/52
  [i915#54]: https://gitlab.freedesktop.org/drm/intel/issues/54
  [i915#699]: https://gitlab.freedesktop.org/drm/intel/issues/699
  [i915#72]: https://gitlab.freedesktop.org/drm/intel/issues/72
  [i915#79]: https://gitlab.freedesktop.org/drm/intel/issues/79
  [i915#93]: https://gitlab.freedesktop.org/drm/intel/issues/93
  [i915#95]: https://gitlab.freedesktop.org/drm/intel/issues/95


Participating hosts (10 -> 8)
------------------------------

  Missing    (2): pig-skl-6260u pig-glk-j5005 


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

  * CI: CI-20190529 -> None
  * IGT: IGT_5614 -> IGTPW_4513
  * Piglit: piglit_4509 -> None

  CI-20190529: 20190529
  CI_DRM_8387: 2f06b2b1d33997d1fb16564959f0ebee8a26b632 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_4513: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4513/index.html
  IGT_5614: d095827add11d4e8158b87683971ee659749d9a4 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4513/index.html
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 3/7] lib/params: overhaul param saving

[Petri Latvala]

On Tue, Apr 28, 2020 at 11:22:51PM +0300, Juha-Pekka Heikkila wrote:
> From: Jani Nikula <jani.nikula@intel.com>
> 
> More generic, use existing functions.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> ---
>  lib/igt_params.c | 101 +++++++++++++++++++----------------------------
>  1 file changed, 41 insertions(+), 60 deletions(-)
> 
> diff --git a/lib/igt_params.c b/lib/igt_params.c
> index f220e73b..b5ac1266 100644
> --- a/lib/igt_params.c
> +++ b/lib/igt_params.c
> @@ -34,92 +34,74 @@
>  #include "igt_params.h"
>  #include "igt_sysfs.h"
>  
> -#define MODULE_PARAM_DIR "/sys/module/i915/parameters/"
> -#define PARAM_NAME_MAX_SZ 32
> -#define PARAM_VALUE_MAX_SZ 16
> -#define PARAM_FILE_PATH_MAX_SZ (strlen(MODULE_PARAM_DIR) + PARAM_NAME_MAX_SZ)
> -
>  struct module_param_data {
> -	char name[PARAM_NAME_MAX_SZ];
> -	char original_value[PARAM_VALUE_MAX_SZ];
> +	char *path;
> +	char *name;
> +	char *original_value;
>  
>  	struct module_param_data *next;
>  };
>  struct module_param_data *module_params = NULL;
>  
> -static void igt_module_param_exit_handler(int sig)
> +static void igt_params_exit_handler(int sig)
>  {
> -	const size_t dir_len = strlen(MODULE_PARAM_DIR);
> -	char file_path[PARAM_FILE_PATH_MAX_SZ];
>  	struct module_param_data *data;
> -	int fd;
> -
> -	/* We don't need to assert string sizes on this function since they were
> -	 * already checked before being stored on the lists. Besides,
> -	 * igt_assert() is not AS-Safe. */
> -	strcpy(file_path, MODULE_PARAM_DIR);
> +	int dir;
>  
>  	for (data = module_params; data != NULL; data = data->next) {
> -		strcpy(file_path + dir_len, data->name);
> -
> -		fd = open(file_path, O_RDWR);
> -		if (fd >= 0) {
> -			int size = strlen (data->original_value);
> -
> -			if (size != write(fd, data->original_value, size)) {
> -				const char msg[] = "WARNING: Module parameters "
> -					"may not have been reset to their "
> -					"original values\n";
> -				assert(write(STDERR_FILENO, msg, sizeof(msg))
> -				       == sizeof(msg));
> -			}
> -
> -			close(fd);
> +		dir = open(data->path, O_RDONLY);
> +
> +		if (!igt_sysfs_set(dir, data->name, data->original_value)) {
> +			const char msg[] = "WARNING: Module parameters "
> +				"may not have been reset to their "
> +				"original values\n";
> +			assert(write(STDERR_FILENO, msg, sizeof(msg))
> +			       == sizeof(msg));
>  		}
> +
> +		close(dir);
>  	}
>  	/* free() is not AS-Safe, so we can't call it here. */
>  }
>  
>  /**
> - * igt_save_module_param:
> - * @name: name of the i915.ko module parameter
> - * @file_path: full sysfs file path for the parameter
> + * igt_params_save:
> + * @dir: file handle for path
> + * @path: full path to the sysfs directory
> + * @name: name of the sysfs attribute
>   *
> - * Reads the current value of an i915.ko module parameter, saves it on an array,
> - * then installs an exit handler to restore it when the program exits.
> + * Reads the current value of a sysfs attribute, saves it on an array, then
> + * installs an exit handler to restore it when the program exits.
>   *
>   * It is safe to call this function multiple times for the same parameter.
>   *
>   * Notice that this function is called by igt_set_module_param(), so that one -
>   * or one of its wrappers - is the only function the test programs need to call.
>   */
> -static void igt_save_module_param(const char *name, const char *file_path)
> +static void igt_params_save(int dir, const char *path, const char *name)
>  {
>  	struct module_param_data *data;
> -	size_t n;
> -	int fd;
>  
>  	/* Check if this parameter is already saved. */
>  	for (data = module_params; data != NULL; data = data->next)
> -		if (strncmp(data->name, name, PARAM_NAME_MAX_SZ) == 0)
> +		if (strcmp(data->path, path) == 0 &&
> +		    strcmp(data->name, name) == 0)
>  			return;
>  
>  	if (!module_params)
> -		igt_install_exit_handler(igt_module_param_exit_handler);
> +		igt_install_exit_handler(igt_params_exit_handler);
>  
>  	data = calloc(1, sizeof (*data));
>  	igt_assert(data);
>  
> -	strncpy(data->name, name, PARAM_NAME_MAX_SZ - 1);
> -
> -	fd = open(file_path, O_RDONLY);
> -	igt_assert(fd >= 0);
> +	data->path = strdup(path);
> +	igt_assert(data->path);
>  
> -	n = read(fd, data->original_value, PARAM_VALUE_MAX_SZ);
> -	igt_assert_f(n > 0 && n < PARAM_VALUE_MAX_SZ,
> -		     "Need to increase PARAM_VALUE_MAX_SZ\n");
> +	data->name = strdup(name);
> +	igt_assert(data->name);
>  
> -	igt_assert(close(fd) == 0);
> +	data->original_value = igt_sysfs_get(dir, name);
> +	igt_assert(data->original_value);
>  
>  	data->next = module_params;
>  	module_params = data;
> @@ -205,22 +187,21 @@ bool igt_params_set(int device, const char *parameter, const char *fmt, ...)
>   */
>  void igt_set_module_param(const char *name, const char *val)
>  {
> -	char file_path[PARAM_FILE_PATH_MAX_SZ];
> -	size_t len = strlen(val);
> -	int fd;
> +	const char *path = "/sys/module/i915/parameters";
> +	int dir;
>  
> -	igt_assert_f(strlen(name) < PARAM_NAME_MAX_SZ,
> -		     "Need to increase PARAM_NAME_MAX_SZ\n");
> -	strcpy(file_path, MODULE_PARAM_DIR);
> -	strcpy(file_path + strlen(MODULE_PARAM_DIR), name);
> +	dir = open(path, O_RDONLY);
> +	igt_assert(dir >= 0);
>  
> -	igt_save_module_param(name, file_path);
> +	igt_params_save(dir, path, name);
>  
> -	fd = open(file_path, O_RDWR);
> -	igt_assert(write(fd, val, len) == len);
> -	igt_assert(close(fd) == 0);
> +	igt_assert(igt_sysfs_set(dir, name, val));
> +
> +	igt_assert(close(dir) == 0);
>  }
>  
> +#define PARAM_VALUE_MAX_SZ 16
> +

For the earlier comment on this define: My confusion was from it not
being used at all after the full series, but hey, it gets completely
removed by a later patch.


Reviewed-by: Petri Latvala <petri.latvala@intel.com>
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t 4/7] lib/params: add igt_params_open() which will return path

[Petri Latvala]

On Tue, Apr 28, 2020 at 11:22:52PM +0300, Juha-Pekka Heikkila wrote:
> From: Jani Nikula <jani.nikula@intel.com>
> 
> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>

Restore author's S-o-b and
Reviewed-by: Petri Latvala <petri.latvala@intel.com>


> ---
>  lib/igt_params.c | 29 ++++++++++++++++++-----------
>  1 file changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/lib/igt_params.c b/lib/igt_params.c
> index b5ac1266..fe4b1df3 100644
> --- a/lib/igt_params.c
> +++ b/lib/igt_params.c
> @@ -107,17 +107,7 @@ static void igt_params_save(int dir, const char *path, const char *name)
>  	module_params = data;
>  }
>  
> -/**
> - * igt_params_open:
> - * @device: fd of the device
> - *
> - * This opens the module parameters directory (under sysfs) corresponding
> - * to the device for use with igt_sysfs_set() and igt_sysfs_get().
> - *
> - * Returns:
> - * The directory fd, or -1 on failure.
> - */
> -int igt_params_open(int device)
> +static int __igt_params_open(int device, char **outpath)
>  {
>  	int dir, params = -1;
>  
> @@ -141,11 +131,28 @@ int igt_params_open(int device)
>  
>  		sprintf(path, "/sys/module/%s/parameters", name);
>  		params = open(path, O_RDONLY);
> +		if (params >= 0 && outpath)
> +			*outpath = strdup(path);
>  	}
>  
>  	return params;
>  }
>  
> +/**
> + * igt_params_open:
> + * @device: fd of the device
> + *
> + * This opens the module parameters directory (under sysfs) corresponding
> + * to the device for use with igt_sysfs_set() and igt_sysfs_get().
> + *
> + * Returns:
> + * The directory fd, or -1 on failure.
> + */
> +int igt_params_open(int device)
> +{
> +	return __igt_params_open(device, NULL);
> +}
> +
>  /**
>   * igt_params_set:
>   * @device: fd of the device
> -- 
> 2.26.0
> 
> _______________________________________________
> 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

Re: [igt-dev] [PATCH i-g-t 5/7] igt/params: add generic saving module parameter set

[Petri Latvala]

On Tue, Apr 28, 2020 at 11:22:53PM +0300, Juha-Pekka Heikkila wrote:
> From: Jani Nikula <jani.nikula@intel.com>
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> ---
>  lib/igt_params.c | 57 ++++++++++++++++++++++++++++++++++++++++--------
>  lib/igt_params.h |  3 +++
>  2 files changed, 51 insertions(+), 9 deletions(-)
> 
> diff --git a/lib/igt_params.c b/lib/igt_params.c
> index fe4b1df3..d9cf986c 100644
> --- a/lib/igt_params.c
> +++ b/lib/igt_params.c
> @@ -153,6 +153,29 @@ int igt_params_open(int device)
>  	return __igt_params_open(device, NULL);
>  }
>  
> +__attribute__((format(printf, 3, 0)))
> +static bool __igt_params_set(int device, const char *parameter,
> +			     const char *fmt, va_list ap, bool save)
> +{
> +	char *path = NULL;
> +	int dir;
> +	int ret;
> +
> +	dir = __igt_params_open(device, save ? &path : NULL);
> +	if (dir < 0)
> +		return false;
> +
> +	if (save)
> +		igt_params_save(dir, path, parameter);
> +
> +	ret = igt_sysfs_vprintf(dir, parameter, fmt, ap);
> +
> +	close(dir);
> +	free(path);
> +
> +	return ret > 0;
> +}
> +
>  /**
>   * igt_params_set:
>   * @device: fd of the device
> @@ -164,20 +187,36 @@ int igt_params_open(int device)
>  bool igt_params_set(int device, const char *parameter, const char *fmt, ...)
>  {
>  	va_list ap;
> -	int dir;
> -	int ret;
> -
> -	dir = igt_params_open(device);
> -	if (dir < 0)
> -		return false;
> +	bool ret;
>  
>  	va_start(ap, fmt);
> -	ret = igt_sysfs_vprintf(dir, parameter, fmt, ap);
> +	ret = __igt_params_set(device, parameter, fmt, ap, false);
>  	va_end(ap);
>  
> -	close(dir);
> +	return ret;
> +}
>  
> -	return ret > 0;
> +/**
> + * igt_params_set_save:
> + * @device: fd of the device or -1 to default to first matching starting from
> + *	    zero. See __igt_params_open()


This documentation is still only understandable if you already know what it does.

"-- to default to the first /dev/dri/cardX device, starting from zero" ?

"See <a function without documentation>" in generated documentation is
less than useful as well... Maybe just drop that sentence altogether
from docs, point to the function in a normal comment? Or link to
igt_debugfs_path() that will do the actual -1 handling...

With better docs,
Reviewed-by: Petri Latvala <petri.latvala@intel.com>



> + * @parameter: the name of the parameter to set
> + * @fmt: printf-esque format string
> + *
> + * Save the original value to be restored.
> + *
> + * Returns true on success
> + */
> +bool igt_params_set_save(int device, const char *parameter, const char *fmt, ...)
> +{
> +	va_list ap;
> +	bool ret;
> +
> +	va_start(ap, fmt);
> +	ret = __igt_params_set(device, parameter, fmt, ap, true);
> +	va_end(ap);
> +
> +	return ret;
>  }
>  
>  /**
> diff --git a/lib/igt_params.h b/lib/igt_params.h
> index 52eed77f..1b750d23 100644
> --- a/lib/igt_params.h
> +++ b/lib/igt_params.h
> @@ -31,6 +31,9 @@ int igt_params_open(int device);
>  __attribute__((format(printf, 3, 4)))
>  bool igt_params_set(int device, const char *parameter, const char *fmt, ...);
>  
> +__attribute__((format(printf, 3, 4)))
> +bool igt_params_set_save(int device, const char *parameter, const char *fmt, ...);
> +
>  void igt_set_module_param(const char *name, const char *val);
>  void igt_set_module_param_int(const char *name, int val);
>  
> -- 
> 2.26.0
> 
> _______________________________________________
> 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

Re: [igt-dev] [PATCH i-g-t 6/7] igt/params: use igt_params_set_save for igt_set_module_param*

[Petri Latvala]

On Tue, Apr 28, 2020 at 11:22:54PM +0300, Juha-Pekka Heikkila wrote:
> Unify parameter access. Here is also decided 'default' device
> if -1 is specified as device. Derault being that device

s/Derault/Default/

> which is first to match requirements, starting search
> from zero upwards.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>


This patch also changes module parameters to use debugfs files instead
of sysfs's /sys/module/<m>/parameters! Could be worth a mention in a
commit message maybe?!


-- 
Petri Latvala



> ---
>  lib/igt_params.c | 134 +++++++++++++++++++++++++++++++++++------------
>  1 file changed, 100 insertions(+), 34 deletions(-)
> 
> diff --git a/lib/igt_params.c b/lib/igt_params.c
> index d9cf986c..dc35f233 100644
> --- a/lib/igt_params.c
> +++ b/lib/igt_params.c
> @@ -27,12 +27,14 @@
>  #include <stdbool.h>
>  #include <stdio.h>
>  #include <sys/ioctl.h>
> +#include <sys/stat.h>
>  
>  #include <i915_drm.h>
>  
>  #include "igt_core.h"
>  #include "igt_params.h"
>  #include "igt_sysfs.h"
> +#include "igt_debugfs.h"
>  
>  struct module_param_data {
>  	char *path;
> @@ -107,15 +109,59 @@ static void igt_params_save(int dir, const char *path, const char *name)
>  	module_params = data;
>  }
>  
> -static int __igt_params_open(int device, char **outpath)
> +static int __igt_params_open(int device, char **outpath, const char *param)
>  {
>  	int dir, params = -1;
> +	struct stat buffer;
> +	char searchname[64];
> +	char searchpath[PATH_MAX];
> +	char *foundname, *ctx;
>  
> -	dir = igt_sysfs_open(device);
> +	dir = igt_debugfs_dir(device);
>  	if (dir >= 0) {
> -		params = openat(dir,
> -				"device/driver/module/parameters",
> -				O_RDONLY);
> +		int devname;
> +
> +		devname = openat(dir, "name", O_RDONLY);
> +		igt_assert(devname >= 0);
> +
> +		read(devname, searchname, sizeof(searchname));
> +		close(devname);
> +
> +		foundname = strtok_r(searchname, " ", &ctx);
> +		igt_assert(foundname);
> +
> +		snprintf(searchpath, PATH_MAX, "%s_params", foundname);
> +		params = openat(dir, searchpath, O_RDONLY);
> +
> +		if (params >= 0) {
> +			char *debugfspath = malloc(PATH_MAX);
> +
> +			igt_debugfs_path(device, debugfspath, PATH_MAX);
> +			if (param != NULL) {
> +				char filepath[PATH_MAX];
> +
> +				snprintf(filepath, PATH_MAX, "%s/%s",
> +					 debugfspath, param);
> +
> +				if (stat(filepath, &buffer) == 0) {
> +					if (outpath != NULL)
> +						*outpath = debugfspath;
> +					else
> +						free(debugfspath);
> +				} else {
> +					free(debugfspath);
> +					close(params);
> +					params = -1;
> +				}
> +			} else if (outpath != NULL) {
> +				/*
> +				 * Caller is responsible to free this.
> +				 */
> +				*outpath = debugfspath;
> +			} else {
> +				free(debugfspath);
> +			}
> +		}
>  		close(dir);
>  	}
>  
> @@ -124,12 +170,51 @@ static int __igt_params_open(int device, char **outpath)
>  		char name[32] = "";
>  		char path[PATH_MAX];
>  
> -		memset(&version, 0, sizeof(version));
> -		version.name_len = sizeof(name);
> -		version.name = name;
> -		ioctl(device, DRM_IOCTL_VERSION, &version);
> -
> -		sprintf(path, "/sys/module/%s/parameters", name);
> +		if (device == -1) {
> +			/*
> +			 * find default device
> +			 */
> +			int file, i;
> +			const char *debugfs_root = igt_debugfs_mount();
> +
> +			igt_assert(debugfs_root);
> +
> +			for (i = 0; i < 63; i++) {
> +				char testpath[PATH_MAX];
> +
> +				snprintf(searchpath, PATH_MAX,
> +					 "%s/dri/%d/name", debugfs_root, i);
> +
> +				file = open(searchpath, O_RDONLY);
> +
> +				if (file < 0)
> +					continue;
> +
> +				read(file, searchname, sizeof(searchname));
> +				close(file);
> +
> +				foundname = strtok_r(searchname, " ", &ctx);
> +				if (!foundname)
> +					continue;
> +
> +				snprintf(testpath, PATH_MAX,
> +					 "/sys/module/%s/parameters",
> +					 foundname);
> +
> +				if (stat(testpath, &buffer) == 0 &&
> +				    S_ISDIR(buffer.st_mode)) {
> +					snprintf(name, sizeof(name), "%s",
> +						 foundname);
> +					break;
> +				}
> +			}
> +		} else {
> +			memset(&version, 0, sizeof(version));
> +			version.name_len = sizeof(name);
> +			version.name = name;
> +			ioctl(device, DRM_IOCTL_VERSION, &version);
> +		}
> +		snprintf(path, sizeof(path), "/sys/module/%s/parameters", name);
>  		params = open(path, O_RDONLY);
>  		if (params >= 0 && outpath)
>  			*outpath = strdup(path);
> @@ -150,7 +235,7 @@ static int __igt_params_open(int device, char **outpath)
>   */
>  int igt_params_open(int device)
>  {
> -	return __igt_params_open(device, NULL);
> +	return __igt_params_open(device, NULL, NULL);
>  }
>  
>  __attribute__((format(printf, 3, 0)))
> @@ -161,7 +246,7 @@ static bool __igt_params_set(int device, const char *parameter,
>  	int dir;
>  	int ret;
>  
> -	dir = __igt_params_open(device, save ? &path : NULL);
> +	dir = __igt_params_open(device, save ? &path : NULL, parameter);
>  	if (dir < 0)
>  		return false;
>  
> @@ -233,21 +318,9 @@ bool igt_params_set_save(int device, const char *parameter, const char *fmt, ...
>   */
>  void igt_set_module_param(const char *name, const char *val)
>  {
> -	const char *path = "/sys/module/i915/parameters";
> -	int dir;
> -
> -	dir = open(path, O_RDONLY);
> -	igt_assert(dir >= 0);
> -
> -	igt_params_save(dir, path, name);
> -
> -	igt_assert(igt_sysfs_set(dir, name, val));
> -
> -	igt_assert(close(dir) == 0);
> +	igt_assert(igt_params_set_save(-1, name, "%s", val));
>  }
>  
> -#define PARAM_VALUE_MAX_SZ 16
> -
>  /**
>   * igt_set_module_param_int:
>   * @name: i915.ko parameter name
> @@ -258,12 +331,5 @@ void igt_set_module_param(const char *name, const char *val)
>   */
>  void igt_set_module_param_int(const char *name, int val)
>  {
> -	char str[PARAM_VALUE_MAX_SZ];
> -	int n;
> -
> -	n = snprintf(str, PARAM_VALUE_MAX_SZ, "%d\n", val);
> -	igt_assert_f(n < PARAM_VALUE_MAX_SZ,
> -		     "Need to increase PARAM_VALUE_MAX_SZ\n");
> -
> -	igt_set_module_param(name, str);
> +	igt_assert(igt_params_set_save(-1, name, "%d", val));
>  }
> -- 
> 2.26.0
> 
> _______________________________________________
> 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

Re: [igt-dev] [PATCH i-g-t 7/7] tests/gem_eio: switch to using igt_params_set()

[Petri Latvala]

On Tue, Apr 28, 2020 at 11:22:55PM +0300, Juha-Pekka Heikkila wrote:
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>

Reviewed-by: Petri Latvala <petri.latvala@intel.com>


> ---
>  tests/i915/gem_eio.c | 57 +++++++++++++++++++-------------------------
>  1 file changed, 24 insertions(+), 33 deletions(-)
> 
> diff --git a/tests/i915/gem_eio.c b/tests/i915/gem_eio.c
> index 1ec60941..ad59f83a 100644
> --- a/tests/i915/gem_eio.c
> +++ b/tests/i915/gem_eio.c
> @@ -52,20 +52,11 @@
>  
>  IGT_TEST_DESCRIPTION("Test that specific ioctls report a wedged GPU (EIO).");
>  
> -static bool i915_reset_control(bool enable)
> +static bool i915_reset_control(int fd, bool enable)
>  {
> -	const char *path = "/sys/module/i915/parameters/reset";
> -	int fd, ret;
> -
>  	igt_debug("%s GPU reset\n", enable ? "Enabling" : "Disabling");
>  
> -	fd = open(path, O_RDWR);
> -	igt_require(fd >= 0);
> -
> -	ret = write(fd, &"01"[enable], 1) == 1;
> -	close(fd);
> -
> -	return ret;
> +	return igt_params_set(fd, "reset", "%d", enable);
>  }
>  
>  static void trigger_reset(int fd)
> @@ -106,9 +97,9 @@ static void wedge_gpu(int fd)
>  	/* First idle the GPU then disable GPU resets before injecting a hang */
>  	gem_quiescent_gpu(fd);
>  
> -	igt_require(i915_reset_control(false));
> +	igt_require(i915_reset_control(fd, false));
>  	manual_hang(fd);
> -	igt_assert(i915_reset_control(true));
> +	igt_assert(i915_reset_control(fd, true));
>  }
>  
>  static int __gem_throttle(int fd)
> @@ -346,7 +337,7 @@ static void __test_banned(int fd)
>  	gem_write(fd, obj.handle, 0, &bbe, sizeof(bbe));
>  
>  	gem_quiescent_gpu(fd);
> -	igt_require(i915_reset_control(true));
> +	igt_require(i915_reset_control(fd, true));
>  
>  	igt_until_timeout(5) {
>  		igt_spin_t *hang;
> @@ -404,9 +395,9 @@ static void test_wait(int fd, unsigned int flags, unsigned int wait)
>  	 */
>  
>  	if (flags & TEST_WEDGE)
> -		igt_require(i915_reset_control(false));
> +		igt_require(i915_reset_control(fd, false));
>  	else
> -		igt_require(i915_reset_control(true));
> +		igt_require(i915_reset_control(fd, true));
>  
>  	hang = spin_sync(fd, 0, I915_EXEC_DEFAULT);
>  
> @@ -415,7 +406,7 @@ static void test_wait(int fd, unsigned int flags, unsigned int wait)
>  
>  	igt_spin_free(fd, hang);
>  
> -	igt_require(i915_reset_control(true));
> +	igt_require(i915_reset_control(fd, true));
>  
>  	trigger_reset(fd);
>  	close(fd);
> @@ -430,12 +421,12 @@ static void test_suspend(int fd, int state)
>  	igt_system_suspend_autoresume(state, SUSPEND_TEST_DEVICES);
>  
>  	/* Check we can suspend when the driver is already wedged */
> -	igt_require(i915_reset_control(false));
> +	igt_require(i915_reset_control(fd, false));
>  	manual_hang(fd);
>  
>  	igt_system_suspend_autoresume(state, SUSPEND_TEST_DEVICES);
>  
> -	igt_require(i915_reset_control(true));
> +	igt_require(i915_reset_control(fd, true));
>  	trigger_reset(fd);
>  	close(fd);
>  }
> @@ -470,7 +461,7 @@ static void test_inflight(int fd, unsigned int wait)
>  
>  		gem_quiescent_gpu(fd);
>  		igt_debug("Starting %s on engine '%s'\n", __func__, e->name);
> -		igt_require(i915_reset_control(false));
> +		igt_require(i915_reset_control(fd, false));
>  
>  		hang = spin_sync(fd, 0, eb_ring(e));
>  		obj[0].handle = hang->handle;
> @@ -495,7 +486,7 @@ static void test_inflight(int fd, unsigned int wait)
>  		}
>  		igt_spin_free(fd, hang);
>  
> -		igt_assert(i915_reset_control(true));
> +		igt_assert(i915_reset_control(fd, true));
>  		trigger_reset(fd);
>  
>  		gem_close(fd, obj[1].handle);
> @@ -520,7 +511,7 @@ static void test_inflight_suspend(int fd)
>  	fd = gem_reopen_driver(fd);
>  	igt_require_gem(fd);
>  	igt_require(gem_has_exec_fence(fd));
> -	igt_require(i915_reset_control(false));
> +	igt_require(i915_reset_control(fd, false));
>  
>  	memset(obj, 0, sizeof(obj));
>  	obj[0].flags = EXEC_OBJECT_WRITE;
> @@ -553,7 +544,7 @@ static void test_inflight_suspend(int fd)
>  	}
>  	igt_spin_free(fd, hang);
>  
> -	igt_assert(i915_reset_control(true));
> +	igt_assert(i915_reset_control(fd, true));
>  	trigger_reset(fd);
>  	close(fd);
>  }
> @@ -601,7 +592,7 @@ static void test_inflight_contexts(int fd, unsigned int wait)
>  		gem_quiescent_gpu(fd);
>  
>  		igt_debug("Starting %s on engine '%s'\n", __func__, e->name);
> -		igt_require(i915_reset_control(false));
> +		igt_require(i915_reset_control(fd, false));
>  
>  		memset(obj, 0, sizeof(obj));
>  		obj[0].flags = EXEC_OBJECT_WRITE;
> @@ -636,7 +627,7 @@ static void test_inflight_contexts(int fd, unsigned int wait)
>  		igt_spin_free(fd, hang);
>  		gem_close(fd, obj[1].handle);
>  
> -		igt_assert(i915_reset_control(true));
> +		igt_assert(i915_reset_control(fd, true));
>  		trigger_reset(fd);
>  
>  		for (unsigned int n = 0; n < ARRAY_SIZE(ctx); n++)
> @@ -663,7 +654,7 @@ static void test_inflight_external(int fd)
>  
>  	fence = igt_cork_plug(&cork, fd);
>  
> -	igt_require(i915_reset_control(false));
> +	igt_require(i915_reset_control(fd, false));
>  	hang = __spin_poll(fd, 0, 0);
>  
>  	memset(&obj, 0, sizeof(obj));
> @@ -695,7 +686,7 @@ static void test_inflight_external(int fd)
>  	close(fence);
>  
>  	igt_spin_free(fd, hang);
> -	igt_assert(i915_reset_control(true));
> +	igt_assert(i915_reset_control(fd, true));
>  	trigger_reset(fd);
>  	close(fd);
>  }
> @@ -714,7 +705,7 @@ static void test_inflight_internal(int fd, unsigned int wait)
>  	fd = gem_reopen_driver(fd);
>  	igt_require_gem(fd);
>  
> -	igt_require(i915_reset_control(false));
> +	igt_require(i915_reset_control(fd, false));
>  	hang = spin_sync(fd, 0, 0);
>  
>  	memset(obj, 0, sizeof(obj));
> @@ -745,7 +736,7 @@ static void test_inflight_internal(int fd, unsigned int wait)
>  	}
>  	igt_spin_free(fd, hang);
>  
> -	igt_assert(i915_reset_control(true));
> +	igt_assert(i915_reset_control(fd, true));
>  	trigger_reset(fd);
>  	close(fd);
>  }
> @@ -781,7 +772,7 @@ static void reset_stress(int fd, uint32_t ctx0,
>  
>  		gem_quiescent_gpu(fd);
>  
> -		igt_require(i915_reset_control(flags & TEST_WEDGE ?
> +		igt_require(i915_reset_control(fd, flags & TEST_WEDGE ?
>  					       false : true));
>  
>  		/*
> @@ -803,7 +794,7 @@ static void reset_stress(int fd, uint32_t ctx0,
>  		igt_assert_eq(sync_fence_status(hang->out_fence), -EIO);
>  
>  		/* Unwedge by forcing a reset. */
> -		igt_assert(i915_reset_control(true));
> +		igt_assert(i915_reset_control(fd, true));
>  		trigger_reset(fd);
>  
>  		gem_quiescent_gpu(fd);
> @@ -918,7 +909,7 @@ static int fd = -1;
>  static void
>  exit_handler(int sig)
>  {
> -	i915_reset_control(true);
> +	i915_reset_control(fd, true);
>  	igt_force_gpu_reset(fd);
>  }
>  
> @@ -933,7 +924,7 @@ igt_main
>  
>  		igt_allow_hang(fd, 0, 0);
>  
> -		igt_require(i915_reset_control(true));
> +		igt_require(i915_reset_control(fd, true));
>  		igt_force_gpu_reset(fd);
>  		igt_install_exit_handler(exit_handler);
>  	}
> -- 
> 2.26.0
> 
> _______________________________________________
> 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

Re: [igt-dev] [PATCH i-g-t 2/7] lib/params: start renaming functions igt_params_*

[Arkadiusz Hiler]

On Tue, Apr 28, 2020 at 11:22:50PM +0300, Juha-Pekka Heikkila wrote:
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> Reviewed-by: Petri Latvala <petri.latvala@intel.com>
> ---
>  lib/drmtest.c                         |  2 +-
>  lib/i915/gem_submission.c             |  2 +-
>  lib/igt_aux.c                         |  3 +--
>  lib/igt_gt.c                          |  2 +-
>  lib/igt_params.c                      | 12 +++++-------
>  lib/igt_params.h                      |  5 ++---
>  tests/i915/gem_ctx_exec.c             |  2 +-
>  tests/i915/gem_ctx_persistence.c      |  9 ++++-----
>  tests/i915/gem_mmap_gtt.c             |  2 +-
>  tests/i915/gem_reset_stats.c          |  6 ++----
>  tests/i915/sysfs_heartbeat_interval.c |  3 ++-
>  tests/i915/sysfs_preempt_timeout.c    |  3 ++-
>  tests/i915/sysfs_timeslice_duration.c |  3 ++-
>  13 files changed, 25 insertions(+), 29 deletions(-)
> 
> diff --git a/lib/drmtest.c b/lib/drmtest.c
> index 1fc39925..17067843 100644
> --- a/lib/drmtest.c
> +++ b/lib/drmtest.c
> @@ -393,7 +393,7 @@ static void __cancel_work_at_exit(int fd)
>  {
>  	igt_terminate_spins(); /* for older kernels */
>  
> -	igt_sysfs_set_parameter(fd, "reset", "%x", -1u /* any method */);
> +	igt_params_set(fd, "reset", "%x", -1u /* any method */);

With #define MODULE_PARAM_DIR "/sys/module/i915/parameters/"
and the debugfs interface everything here is i915-specific.

i915_param_ prefix would be better as it both suggests that we are
dealing with i915 module parameters instead of some elusive IGT
framework params and doesn't create illusion that this is
driver-agnostic.

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

Re: [igt-dev] [PATCH i-g-t 5/7] igt/params: add generic saving module parameter set

[Arkadiusz Hiler]

On Tue, Apr 28, 2020 at 11:22:53PM +0300, Juha-Pekka Heikkila wrote:
> From: Jani Nikula <jani.nikula@intel.com>
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> ---
>  lib/igt_params.c | 57 ++++++++++++++++++++++++++++++++++++++++--------
>  lib/igt_params.h |  3 +++
>  2 files changed, 51 insertions(+), 9 deletions(-)
> 
> diff --git a/lib/igt_params.c b/lib/igt_params.c
> index fe4b1df3..d9cf986c 100644
> --- a/lib/igt_params.c
> +++ b/lib/igt_params.c
> @@ -153,6 +153,29 @@ int igt_params_open(int device)
>  	return __igt_params_open(device, NULL);
>  }
>  
> +__attribute__((format(printf, 3, 0)))

this is using a va_list, not "...", so this format attirbue should not
be here

> +static bool __igt_params_set(int device, const char *parameter,
> +			     const char *fmt, va_list ap, bool save)
> +{
> +	char *path = NULL;
> +	int dir;
> +	int ret;
> +
> +	dir = __igt_params_open(device, save ? &path : NULL);
> +	if (dir < 0)
> +		return false;
> +
> +	if (save)
> +		igt_params_save(dir, path, parameter);
> +
> +	ret = igt_sysfs_vprintf(dir, parameter, fmt, ap);
> +
> +	close(dir);
> +	free(path);
> +
> +	return ret > 0;
> +}
> +
>  /**
>   * igt_params_set:
>   * @device: fd of the device
> @@ -164,20 +187,36 @@ int igt_params_open(int device)
>  bool igt_params_set(int device, const char *parameter, const char *fmt, ...)
>  {
>  	va_list ap;
> -	int dir;
> -	int ret;
> -
> -	dir = igt_params_open(device);
> -	if (dir < 0)
> -		return false;
> +	bool ret;
>  
>  	va_start(ap, fmt);
> -	ret = igt_sysfs_vprintf(dir, parameter, fmt, ap);
> +	ret = __igt_params_set(device, parameter, fmt, ap, false);
>  	va_end(ap);
>  
> -	close(dir);
> +	return ret;
> +}
>  
> -	return ret > 0;
> +/**
> + * igt_params_set_save:
> + * @device: fd of the device or -1 to default to first matching starting from
> + *	    zero. See __igt_params_open()
> + * @parameter: the name of the parameter to set
> + * @fmt: printf-esque format string
> + *
> + * Save the original value to be restored.

this needs a bit more explanation

1. mention that it's basically _params_save followed by _params_set
2. mention that the value is resoted by atexit handler, but refer to the
   _params_save for details


> + *
> + * Returns true on success
> + */
> +bool igt_params_set_save(int device, const char *parameter, const char *fmt, ...)

_save_and_set

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

Re: [igt-dev] [PATCH i-g-t 6/7] igt/params: use igt_params_set_save for igt_set_module_param*

[Arkadiusz Hiler]

On Tue, Apr 28, 2020 at 11:22:54PM +0300, Juha-Pekka Heikkila wrote:
> Unify parameter access. Here is also decided 'default' device
> if -1 is specified as device. Derault being that device
> which is first to match requirements, starting search
> from zero upwards.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> ---
>  lib/igt_params.c | 134 +++++++++++++++++++++++++++++++++++------------
>  1 file changed, 100 insertions(+), 34 deletions(-)
> 
> diff --git a/lib/igt_params.c b/lib/igt_params.c
> index d9cf986c..dc35f233 100644
> --- a/lib/igt_params.c
> +++ b/lib/igt_params.c
> @@ -27,12 +27,14 @@
>  #include <stdbool.h>
>  #include <stdio.h>
>  #include <sys/ioctl.h>
> +#include <sys/stat.h>
>  
>  #include <i915_drm.h>
>  
>  #include "igt_core.h"
>  #include "igt_params.h"
>  #include "igt_sysfs.h"
> +#include "igt_debugfs.h"
>  
>  struct module_param_data {
>  	char *path;
> @@ -107,15 +109,59 @@ static void igt_params_save(int dir, const char *path, const char *name)
>  	module_params = data;
>  }
>  
> -static int __igt_params_open(int device, char **outpath)
> +static int __igt_params_open(int device, char **outpath, const char *param)

Please add a function documentation block on top of this. Deciphering
the role of the new "param" argument in the context of this function
from the code alone is really hard. You should also mention how it being
set vs it being NULL affect the outcome.

I would appreciate a bit more contet on this change in the commit
message, especially about the sysfs -> debugfs switch.

If I got your intention right, the code LGTM.

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

Re: [igt-dev] [PATCH i-g-t 2/7] lib/params: start renaming functions igt_params_*

[Juha-Pekka Heikkila]

On 5.5.2020 17.04, Arkadiusz Hiler wrote:
> On Tue, Apr 28, 2020 at 11:22:50PM +0300, Juha-Pekka Heikkila wrote:
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
>> Reviewed-by: Petri Latvala <petri.latvala@intel.com>
>> ---
>>   lib/drmtest.c                         |  2 +-
>>   lib/i915/gem_submission.c             |  2 +-
>>   lib/igt_aux.c                         |  3 +--
>>   lib/igt_gt.c                          |  2 +-
>>   lib/igt_params.c                      | 12 +++++-------
>>   lib/igt_params.h                      |  5 ++---
>>   tests/i915/gem_ctx_exec.c             |  2 +-
>>   tests/i915/gem_ctx_persistence.c      |  9 ++++-----
>>   tests/i915/gem_mmap_gtt.c             |  2 +-
>>   tests/i915/gem_reset_stats.c          |  6 ++----
>>   tests/i915/sysfs_heartbeat_interval.c |  3 ++-
>>   tests/i915/sysfs_preempt_timeout.c    |  3 ++-
>>   tests/i915/sysfs_timeslice_duration.c |  3 ++-
>>   13 files changed, 25 insertions(+), 29 deletions(-)
>>
>> diff --git a/lib/drmtest.c b/lib/drmtest.c
>> index 1fc39925..17067843 100644
>> --- a/lib/drmtest.c
>> +++ b/lib/drmtest.c
>> @@ -393,7 +393,7 @@ static void __cancel_work_at_exit(int fd)
>>   {
>>   	igt_terminate_spins(); /* for older kernels */
>>   
>> -	igt_sysfs_set_parameter(fd, "reset", "%x", -1u /* any method */);
>> +	igt_params_set(fd, "reset", "%x", -1u /* any method */);
> 
> With #define MODULE_PARAM_DIR "/sys/module/i915/parameters/"
> and the debugfs interface everything here is i915-specific.
> 
> i915_param_ prefix would be better as it both suggests that we are
> dealing with i915 module parameters instead of some elusive IGT
> framework params and doesn't create illusion that this is
> driver-agnostic.

This is intermediate step, admittedly could be squashed but I didn't see 
reason for hiding building of parameters. Anyway on final setup there's 
not supposing to be anything limiting igt_params to i915. That said I 
have not had possibility to try this on amdgpu or something but still..

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

Re: [igt-dev] [PATCH i-g-t 4/7] lib/params: add igt_params_open() which will return path

[Juha-Pekka Heikkila]

On 5.5.2020 10.07, Petri Latvala wrote:
> On Tue, Apr 28, 2020 at 11:22:52PM +0300, Juha-Pekka Heikkila wrote:
>> From: Jani Nikula <jani.nikula@intel.com>
>>
>> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> 
> Restore author's S-o-b and

I've been trying to advertise on cover letter about this but maybe Jani 
didn't see it there. Original patch doesn't have Jani's S-o-b. If Jani 
comment about it I could add it here.

> Reviewed-by: Petri Latvala <petri.latvala@intel.com>
> 
> 
>> ---
>>   lib/igt_params.c | 29 ++++++++++++++++++-----------
>>   1 file changed, 18 insertions(+), 11 deletions(-)
>>
>> diff --git a/lib/igt_params.c b/lib/igt_params.c
>> index b5ac1266..fe4b1df3 100644
>> --- a/lib/igt_params.c
>> +++ b/lib/igt_params.c
>> @@ -107,17 +107,7 @@ static void igt_params_save(int dir, const char *path, const char *name)
>>   	module_params = data;
>>   }
>>   
>> -/**
>> - * igt_params_open:
>> - * @device: fd of the device
>> - *
>> - * This opens the module parameters directory (under sysfs) corresponding
>> - * to the device for use with igt_sysfs_set() and igt_sysfs_get().
>> - *
>> - * Returns:
>> - * The directory fd, or -1 on failure.
>> - */
>> -int igt_params_open(int device)
>> +static int __igt_params_open(int device, char **outpath)
>>   {
>>   	int dir, params = -1;
>>   
>> @@ -141,11 +131,28 @@ int igt_params_open(int device)
>>   
>>   		sprintf(path, "/sys/module/%s/parameters", name);
>>   		params = open(path, O_RDONLY);
>> +		if (params >= 0 && outpath)
>> +			*outpath = strdup(path);
>>   	}
>>   
>>   	return params;
>>   }
>>   
>> +/**
>> + * igt_params_open:
>> + * @device: fd of the device
>> + *
>> + * This opens the module parameters directory (under sysfs) corresponding
>> + * to the device for use with igt_sysfs_set() and igt_sysfs_get().
>> + *
>> + * Returns:
>> + * The directory fd, or -1 on failure.
>> + */
>> +int igt_params_open(int device)
>> +{
>> +	return __igt_params_open(device, NULL);
>> +}
>> +
>>   /**
>>    * igt_params_set:
>>    * @device: fd of the device
>> -- 
>> 2.26.0
>>
>> _______________________________________________
>> 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

Re: [igt-dev] [PATCH i-g-t 5/7] igt/params: add generic saving module parameter set

[Juha-Pekka Heikkila]

On 5.5.2020 10.16, Petri Latvala wrote:
> On Tue, Apr 28, 2020 at 11:22:53PM +0300, Juha-Pekka Heikkila wrote:
>> From: Jani Nikula <jani.nikula@intel.com>
>>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
>> ---
>>   lib/igt_params.c | 57 ++++++++++++++++++++++++++++++++++++++++--------
>>   lib/igt_params.h |  3 +++
>>   2 files changed, 51 insertions(+), 9 deletions(-)
>>
>> diff --git a/lib/igt_params.c b/lib/igt_params.c
>> index fe4b1df3..d9cf986c 100644
>> --- a/lib/igt_params.c
>> +++ b/lib/igt_params.c
>> @@ -153,6 +153,29 @@ int igt_params_open(int device)
>>   	return __igt_params_open(device, NULL);
>>   }
>>   
>> +__attribute__((format(printf, 3, 0)))
>> +static bool __igt_params_set(int device, const char *parameter,
>> +			     const char *fmt, va_list ap, bool save)
>> +{
>> +	char *path = NULL;
>> +	int dir;
>> +	int ret;
>> +
>> +	dir = __igt_params_open(device, save ? &path : NULL);
>> +	if (dir < 0)
>> +		return false;
>> +
>> +	if (save)
>> +		igt_params_save(dir, path, parameter);
>> +
>> +	ret = igt_sysfs_vprintf(dir, parameter, fmt, ap);
>> +
>> +	close(dir);
>> +	free(path);
>> +
>> +	return ret > 0;
>> +}
>> +
>>   /**
>>    * igt_params_set:
>>    * @device: fd of the device
>> @@ -164,20 +187,36 @@ int igt_params_open(int device)
>>   bool igt_params_set(int device, const char *parameter, const char *fmt, ...)
>>   {
>>   	va_list ap;
>> -	int dir;
>> -	int ret;
>> -
>> -	dir = igt_params_open(device);
>> -	if (dir < 0)
>> -		return false;
>> +	bool ret;
>>   
>>   	va_start(ap, fmt);
>> -	ret = igt_sysfs_vprintf(dir, parameter, fmt, ap);
>> +	ret = __igt_params_set(device, parameter, fmt, ap, false);
>>   	va_end(ap);
>>   
>> -	close(dir);
>> +	return ret;
>> +}
>>   
>> -	return ret > 0;
>> +/**
>> + * igt_params_set_save:
>> + * @device: fd of the device or -1 to default to first matching starting from
>> + *	    zero. See __igt_params_open()
> 
> 
> This documentation is still only understandable if you already know what it does.
> 
> "-- to default to the first /dev/dri/cardX device, starting from zero" ?
> 
> "See <a function without documentation>" in generated documentation is
> less than useful as well... Maybe just drop that sentence altogether
> from docs, point to the function in a normal comment? Or link to
> igt_debugfs_path() that will do the actual -1 handling...
> 
> With better docs,
> Reviewed-by: Petri Latvala <petri.latvala@intel.com>

These were generally in line with how sysfs was documented but I agree 
it's more fun to read code where there are more hints on what's going 
on. I'll checkout those comment blocks what's all the stuff that could 
be said there.

> 
> 
> 
>> + * @parameter: the name of the parameter to set
>> + * @fmt: printf-esque format string
>> + *
>> + * Save the original value to be restored.
>> + *
>> + * Returns true on success
>> + */
>> +bool igt_params_set_save(int device, const char *parameter, const char *fmt, ...)
>> +{
>> +	va_list ap;
>> +	bool ret;
>> +
>> +	va_start(ap, fmt);
>> +	ret = __igt_params_set(device, parameter, fmt, ap, true);
>> +	va_end(ap);
>> +
>> +	return ret;
>>   }
>>   
>>   /**
>> diff --git a/lib/igt_params.h b/lib/igt_params.h
>> index 52eed77f..1b750d23 100644
>> --- a/lib/igt_params.h
>> +++ b/lib/igt_params.h
>> @@ -31,6 +31,9 @@ int igt_params_open(int device);
>>   __attribute__((format(printf, 3, 4)))
>>   bool igt_params_set(int device, const char *parameter, const char *fmt, ...);
>>   
>> +__attribute__((format(printf, 3, 4)))
>> +bool igt_params_set_save(int device, const char *parameter, const char *fmt, ...);
>> +
>>   void igt_set_module_param(const char *name, const char *val);
>>   void igt_set_module_param_int(const char *name, int val);
>>   
>> -- 
>> 2.26.0
>>
>> _______________________________________________
>> 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

Re: [igt-dev] [PATCH i-g-t 5/7] igt/params: add generic saving module parameter set

[Juha-Pekka Heikkila]

On 5.5.2020 17.09, Arkadiusz Hiler wrote:
> On Tue, Apr 28, 2020 at 11:22:53PM +0300, Juha-Pekka Heikkila wrote:
>> From: Jani Nikula <jani.nikula@intel.com>
>>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
>> ---
>>   lib/igt_params.c | 57 ++++++++++++++++++++++++++++++++++++++++--------
>>   lib/igt_params.h |  3 +++
>>   2 files changed, 51 insertions(+), 9 deletions(-)
>>
>> diff --git a/lib/igt_params.c b/lib/igt_params.c
>> index fe4b1df3..d9cf986c 100644
>> --- a/lib/igt_params.c
>> +++ b/lib/igt_params.c
>> @@ -153,6 +153,29 @@ int igt_params_open(int device)
>>   	return __igt_params_open(device, NULL);
>>   }
>>   
>> +__attribute__((format(printf, 3, 0)))
> 
> this is using a va_list, not "...", so this format attirbue should not
> be here
> 

Good catch

>> +static bool __igt_params_set(int device, const char *parameter,
>> +			     const char *fmt, va_list ap, bool save)
>> +{
>> +	char *path = NULL;
>> +	int dir;
>> +	int ret;
>> +
>> +	dir = __igt_params_open(device, save ? &path : NULL);
>> +	if (dir < 0)
>> +		return false;
>> +
>> +	if (save)
>> +		igt_params_save(dir, path, parameter);
>> +
>> +	ret = igt_sysfs_vprintf(dir, parameter, fmt, ap);
>> +
>> +	close(dir);
>> +	free(path);
>> +
>> +	return ret > 0;
>> +}
>> +
>>   /**
>>    * igt_params_set:
>>    * @device: fd of the device
>> @@ -164,20 +187,36 @@ int igt_params_open(int device)
>>   bool igt_params_set(int device, const char *parameter, const char *fmt, ...)
>>   {
>>   	va_list ap;
>> -	int dir;
>> -	int ret;
>> -
>> -	dir = igt_params_open(device);
>> -	if (dir < 0)
>> -		return false;
>> +	bool ret;
>>   
>>   	va_start(ap, fmt);
>> -	ret = igt_sysfs_vprintf(dir, parameter, fmt, ap);
>> +	ret = __igt_params_set(device, parameter, fmt, ap, false);
>>   	va_end(ap);
>>   
>> -	close(dir);
>> +	return ret;
>> +}
>>   
>> -	return ret > 0;
>> +/**
>> + * igt_params_set_save:
>> + * @device: fd of the device or -1 to default to first matching starting from
>> + *	    zero. See __igt_params_open()
>> + * @parameter: the name of the parameter to set
>> + * @fmt: printf-esque format string
>> + *
>> + * Save the original value to be restored.
> 
> this needs a bit more explanation
> 
> 1. mention that it's basically _params_save followed by _params_set
> 2. mention that the value is resoted by atexit handler, but refer to the
>     _params_save for details
> 

As I replied to Petri on same patch, this is generally in line how sysfs 
is documented but totally agree it's more fun to read code with more 
hints what's going on. It makes it easier to see if code and comments 
disagree -- usually that's a sign both are wrong. I'll see what I can 
come up with my inner poet.

Thanks for the comments guys, I'll make some changes and send new set.

> 
>> + *
>> + * Returns true on success
>> + */
>> +bool igt_params_set_save(int device, const char *parameter, const char *fmt, ...)
> 
> _save_and_set
> 

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

Re: [igt-dev] [PATCH i-g-t 5/7] igt/params: add generic saving module parameter set

[Juha-Pekka Heikkila]

On 5.5.2020 21.46, Juha-Pekka Heikkila wrote:
> On 5.5.2020 17.09, Arkadiusz Hiler wrote:
>> On Tue, Apr 28, 2020 at 11:22:53PM +0300, Juha-Pekka Heikkila wrote:
>>> From: Jani Nikula <jani.nikula@intel.com>
>>>
>>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>>> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
>>> ---
>>>   lib/igt_params.c | 57 ++++++++++++++++++++++++++++++++++++++++--------
>>>   lib/igt_params.h |  3 +++
>>>   2 files changed, 51 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/lib/igt_params.c b/lib/igt_params.c
>>> index fe4b1df3..d9cf986c 100644
>>> --- a/lib/igt_params.c
>>> +++ b/lib/igt_params.c
>>> @@ -153,6 +153,29 @@ int igt_params_open(int device)
>>>       return __igt_params_open(device, NULL);
>>>   }
>>> +__attribute__((format(printf, 3, 0)))
>>
>> this is using a va_list, not "...", so this format attirbue should not
>> be here
>>
> 
> Good catch
> 

Looking it with bit more thought, what's wrong with it? I think it is 
required there.

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

Re: [igt-dev] [PATCH i-g-t 6/7] igt/params: use igt_params_set_save for igt_set_module_param*

[Petri Latvala]

On Tue, Apr 28, 2020 at 11:22:54PM +0300, Juha-Pekka Heikkila wrote:
> Unify parameter access. Here is also decided 'default' device
> if -1 is specified as device. Derault being that device
> which is first to match requirements, starting search
> from zero upwards.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> ---
>  lib/igt_params.c | 134 +++++++++++++++++++++++++++++++++++------------
>  1 file changed, 100 insertions(+), 34 deletions(-)
> 
> diff --git a/lib/igt_params.c b/lib/igt_params.c
> index d9cf986c..dc35f233 100644
> --- a/lib/igt_params.c
> +++ b/lib/igt_params.c
> @@ -27,12 +27,14 @@
>  #include <stdbool.h>
>  #include <stdio.h>
>  #include <sys/ioctl.h>
> +#include <sys/stat.h>
>  
>  #include <i915_drm.h>
>  
>  #include "igt_core.h"
>  #include "igt_params.h"
>  #include "igt_sysfs.h"
> +#include "igt_debugfs.h"
>  
>  struct module_param_data {
>  	char *path;
> @@ -107,15 +109,59 @@ static void igt_params_save(int dir, const char *path, const char *name)
>  	module_params = data;
>  }
>  
> -static int __igt_params_open(int device, char **outpath)
> +static int __igt_params_open(int device, char **outpath, const char *param)
>  {
>  	int dir, params = -1;
> +	struct stat buffer;
> +	char searchname[64];
> +	char searchpath[PATH_MAX];
> +	char *foundname, *ctx;
>  
> -	dir = igt_sysfs_open(device);
> +	dir = igt_debugfs_dir(device);
>  	if (dir >= 0) {
> -		params = openat(dir,
> -				"device/driver/module/parameters",
> -				O_RDONLY);
> +		int devname;
> +
> +		devname = openat(dir, "name", O_RDONLY);
> +		igt_assert(devname >= 0);
> +
> +		read(devname, searchname, sizeof(searchname));
> +		close(devname);
> +
> +		foundname = strtok_r(searchname, " ", &ctx);
> +		igt_assert(foundname);

This assert and the one above, I wonder if it makes more sense to
igt_require instead. Do we want to claim it's a kernel bug if the
driver doesn't want to reveal its name?


Either way, the code looks funky with its control flow, but
Reviewed-by: Petri Latvala <petri.latvala@intel.com>


> +
> +		snprintf(searchpath, PATH_MAX, "%s_params", foundname);
> +		params = openat(dir, searchpath, O_RDONLY);
> +
> +		if (params >= 0) {
> +			char *debugfspath = malloc(PATH_MAX);
> +
> +			igt_debugfs_path(device, debugfspath, PATH_MAX);
> +			if (param != NULL) {
> +				char filepath[PATH_MAX];
> +
> +				snprintf(filepath, PATH_MAX, "%s/%s",
> +					 debugfspath, param);
> +
> +				if (stat(filepath, &buffer) == 0) {
> +					if (outpath != NULL)
> +						*outpath = debugfspath;
> +					else
> +						free(debugfspath);
> +				} else {
> +					free(debugfspath);
> +					close(params);
> +					params = -1;
> +				}
> +			} else if (outpath != NULL) {
> +				/*
> +				 * Caller is responsible to free this.
> +				 */
> +				*outpath = debugfspath;
> +			} else {
> +				free(debugfspath);
> +			}
> +		}
>  		close(dir);
>  	}
>  
> @@ -124,12 +170,51 @@ static int __igt_params_open(int device, char **outpath)
>  		char name[32] = "";
>  		char path[PATH_MAX];
>  
> -		memset(&version, 0, sizeof(version));
> -		version.name_len = sizeof(name);
> -		version.name = name;
> -		ioctl(device, DRM_IOCTL_VERSION, &version);
> -
> -		sprintf(path, "/sys/module/%s/parameters", name);
> +		if (device == -1) {
> +			/*
> +			 * find default device
> +			 */
> +			int file, i;
> +			const char *debugfs_root = igt_debugfs_mount();
> +
> +			igt_assert(debugfs_root);
> +
> +			for (i = 0; i < 63; i++) {
> +				char testpath[PATH_MAX];
> +
> +				snprintf(searchpath, PATH_MAX,
> +					 "%s/dri/%d/name", debugfs_root, i);
> +
> +				file = open(searchpath, O_RDONLY);
> +
> +				if (file < 0)
> +					continue;
> +
> +				read(file, searchname, sizeof(searchname));
> +				close(file);
> +
> +				foundname = strtok_r(searchname, " ", &ctx);
> +				if (!foundname)
> +					continue;
> +
> +				snprintf(testpath, PATH_MAX,
> +					 "/sys/module/%s/parameters",
> +					 foundname);
> +
> +				if (stat(testpath, &buffer) == 0 &&
> +				    S_ISDIR(buffer.st_mode)) {
> +					snprintf(name, sizeof(name), "%s",
> +						 foundname);
> +					break;
> +				}
> +			}
> +		} else {
> +			memset(&version, 0, sizeof(version));
> +			version.name_len = sizeof(name);
> +			version.name = name;
> +			ioctl(device, DRM_IOCTL_VERSION, &version);
> +		}
> +		snprintf(path, sizeof(path), "/sys/module/%s/parameters", name);
>  		params = open(path, O_RDONLY);
>  		if (params >= 0 && outpath)
>  			*outpath = strdup(path);
> @@ -150,7 +235,7 @@ static int __igt_params_open(int device, char **outpath)
>   */
>  int igt_params_open(int device)
>  {
> -	return __igt_params_open(device, NULL);
> +	return __igt_params_open(device, NULL, NULL);
>  }
>  
>  __attribute__((format(printf, 3, 0)))
> @@ -161,7 +246,7 @@ static bool __igt_params_set(int device, const char *parameter,
>  	int dir;
>  	int ret;
>  
> -	dir = __igt_params_open(device, save ? &path : NULL);
> +	dir = __igt_params_open(device, save ? &path : NULL, parameter);
>  	if (dir < 0)
>  		return false;
>  
> @@ -233,21 +318,9 @@ bool igt_params_set_save(int device, const char *parameter, const char *fmt, ...
>   */
>  void igt_set_module_param(const char *name, const char *val)
>  {
> -	const char *path = "/sys/module/i915/parameters";
> -	int dir;
> -
> -	dir = open(path, O_RDONLY);
> -	igt_assert(dir >= 0);
> -
> -	igt_params_save(dir, path, name);
> -
> -	igt_assert(igt_sysfs_set(dir, name, val));
> -
> -	igt_assert(close(dir) == 0);
> +	igt_assert(igt_params_set_save(-1, name, "%s", val));
>  }
>  
> -#define PARAM_VALUE_MAX_SZ 16
> -
>  /**
>   * igt_set_module_param_int:
>   * @name: i915.ko parameter name
> @@ -258,12 +331,5 @@ void igt_set_module_param(const char *name, const char *val)
>   */
>  void igt_set_module_param_int(const char *name, int val)
>  {
> -	char str[PARAM_VALUE_MAX_SZ];
> -	int n;
> -
> -	n = snprintf(str, PARAM_VALUE_MAX_SZ, "%d\n", val);
> -	igt_assert_f(n < PARAM_VALUE_MAX_SZ,
> -		     "Need to increase PARAM_VALUE_MAX_SZ\n");
> -
> -	igt_set_module_param(name, str);
> +	igt_assert(igt_params_set_save(-1, name, "%d", val));
>  }
> -- 
> 2.26.0
> 
> _______________________________________________
> 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

Re: [igt-dev] [PATCH i-g-t 5/7] igt/params: add generic saving module parameter set

[Arkadiusz Hiler]

On Tue, May 05, 2020 at 11:00:03PM +0300, Juha-Pekka Heikkila wrote:
> On 5.5.2020 21.46, Juha-Pekka Heikkila wrote:
> > On 5.5.2020 17.09, Arkadiusz Hiler wrote:
> > > On Tue, Apr 28, 2020 at 11:22:53PM +0300, Juha-Pekka Heikkila wrote:
> > > > From: Jani Nikula <jani.nikula@intel.com>
> > > > 
> > > > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> > > > Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> > > > ---
> > > >   lib/igt_params.c | 57 ++++++++++++++++++++++++++++++++++++++++--------
> > > >   lib/igt_params.h |  3 +++
> > > >   2 files changed, 51 insertions(+), 9 deletions(-)
> > > > 
> > > > diff --git a/lib/igt_params.c b/lib/igt_params.c
> > > > index fe4b1df3..d9cf986c 100644
> > > > --- a/lib/igt_params.c
> > > > +++ b/lib/igt_params.c
> > > > @@ -153,6 +153,29 @@ int igt_params_open(int device)
> > > >       return __igt_params_open(device, NULL);
> > > >   }
> > > > +__attribute__((format(printf, 3, 0)))
> > > 
> > > this is using a va_list, not "...", so this format attirbue should not
> > > be here
> > > 
> > 
> > Good catch
> > 
> 
> Looking it with bit more thought, what's wrong with it? I think it is
> required there.

I found it a bit useless that we do printf format checking without
having any arugments to match types for as they are wrapped in va_list,
but if compilers are screeming that it's necessary so be it.

Not sure what benefit does it provide though.

-- 
Cheers,
Arek

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

Re: [igt-dev] [PATCH i-g-t 5/7] igt/params: add generic saving module parameter set

[Juha-Pekka Heikkila]

On 6.5.2020 12.05, Arkadiusz Hiler wrote:
> On Tue, May 05, 2020 at 11:00:03PM +0300, Juha-Pekka Heikkila wrote:
>> On 5.5.2020 21.46, Juha-Pekka Heikkila wrote:
>>> On 5.5.2020 17.09, Arkadiusz Hiler wrote:
>>>> On Tue, Apr 28, 2020 at 11:22:53PM +0300, Juha-Pekka Heikkila wrote:
>>>>> From: Jani Nikula <jani.nikula@intel.com>
>>>>>
>>>>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>>>>> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
>>>>> ---
>>>>>    lib/igt_params.c | 57 ++++++++++++++++++++++++++++++++++++++++--------
>>>>>    lib/igt_params.h |  3 +++
>>>>>    2 files changed, 51 insertions(+), 9 deletions(-)
>>>>>
>>>>> diff --git a/lib/igt_params.c b/lib/igt_params.c
>>>>> index fe4b1df3..d9cf986c 100644
>>>>> --- a/lib/igt_params.c
>>>>> +++ b/lib/igt_params.c
>>>>> @@ -153,6 +153,29 @@ int igt_params_open(int device)
>>>>>        return __igt_params_open(device, NULL);
>>>>>    }
>>>>> +__attribute__((format(printf, 3, 0)))
>>>>
>>>> this is using a va_list, not "...", so this format attirbue should not
>>>> be here
>>>>
>>>
>>> Good catch
>>>
>>
>> Looking it with bit more thought, what's wrong with it? I think it is
>> required there.
> 
> I found it a bit useless that we do printf format checking without
> having any arugments to match types for as they are wrapped in va_list,
> but if compilers are screeming that it's necessary so be it.
> 
> Not sure what benefit does it provide though.

Benefit is not to get compiler warning which would be caused from later 
usage of vsnprintf. Other way to avoid it is to take out 
"missing-format-attribute" warnings.

va_list doesn't do anything here, parameter 3 is "const char *fmt". 
Va_list will not be touched.

/Juha-Pekka

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

Re: [igt-dev] [PATCH i-g-t 5/7] igt/params: add generic saving module parameter set

[Petri Latvala]

On Wed, May 06, 2020 at 12:54:37PM +0300, Juha-Pekka Heikkila wrote:
> On 6.5.2020 12.05, Arkadiusz Hiler wrote:
> > On Tue, May 05, 2020 at 11:00:03PM +0300, Juha-Pekka Heikkila wrote:
> > > On 5.5.2020 21.46, Juha-Pekka Heikkila wrote:
> > > > On 5.5.2020 17.09, Arkadiusz Hiler wrote:
> > > > > On Tue, Apr 28, 2020 at 11:22:53PM +0300, Juha-Pekka Heikkila wrote:
> > > > > > From: Jani Nikula <jani.nikula@intel.com>
> > > > > > 
> > > > > > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> > > > > > Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> > > > > > ---
> > > > > >    lib/igt_params.c | 57 ++++++++++++++++++++++++++++++++++++++++--------
> > > > > >    lib/igt_params.h |  3 +++
> > > > > >    2 files changed, 51 insertions(+), 9 deletions(-)
> > > > > > 
> > > > > > diff --git a/lib/igt_params.c b/lib/igt_params.c
> > > > > > index fe4b1df3..d9cf986c 100644
> > > > > > --- a/lib/igt_params.c
> > > > > > +++ b/lib/igt_params.c
> > > > > > @@ -153,6 +153,29 @@ int igt_params_open(int device)
> > > > > >        return __igt_params_open(device, NULL);
> > > > > >    }
> > > > > > +__attribute__((format(printf, 3, 0)))
> > > > > 
> > > > > this is using a va_list, not "...", so this format attirbue should not
> > > > > be here
> > > > > 
> > > > 
> > > > Good catch
> > > > 
> > > 
> > > Looking it with bit more thought, what's wrong with it? I think it is
> > > required there.
> > 
> > I found it a bit useless that we do printf format checking without
> > having any arugments to match types for as they are wrapped in va_list,
> > but if compilers are screeming that it's necessary so be it.
> > 
> > Not sure what benefit does it provide though.
> 
> Benefit is not to get compiler warning which would be caused from later
> usage of vsnprintf. Other way to avoid it is to take out
> "missing-format-attribute" warnings.

First I thought format_arg(3) is proper here but after checking the
docs, it is as J-P says:

"For functions where the arguments are not available to be checked
(such as vprintf), specify the third parameter as zero. In this case
the compiler only checks the format string for consistency."


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

[igt-dev] [PATCH i-g-t 3/7] lib/params: overhaul param saving

[Juha-Pekka Heikkila]

From: Jani Nikula <jani.nikula@intel.com>

More generic, use existing functions.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
Reviewed-by: Petri Latvala <petri.latvala@intel.com>
---
 lib/igt_params.c | 101 +++++++++++++++++++----------------------------
 1 file changed, 41 insertions(+), 60 deletions(-)

diff --git a/lib/igt_params.c b/lib/igt_params.c
index f220e73b..b5ac1266 100644
--- a/lib/igt_params.c
+++ b/lib/igt_params.c
@@ -34,92 +34,74 @@
 #include "igt_params.h"
 #include "igt_sysfs.h"
 
-#define MODULE_PARAM_DIR "/sys/module/i915/parameters/"
-#define PARAM_NAME_MAX_SZ 32
-#define PARAM_VALUE_MAX_SZ 16
-#define PARAM_FILE_PATH_MAX_SZ (strlen(MODULE_PARAM_DIR) + PARAM_NAME_MAX_SZ)
-
 struct module_param_data {
-	char name[PARAM_NAME_MAX_SZ];
-	char original_value[PARAM_VALUE_MAX_SZ];
+	char *path;
+	char *name;
+	char *original_value;
 
 	struct module_param_data *next;
 };
 struct module_param_data *module_params = NULL;
 
-static void igt_module_param_exit_handler(int sig)
+static void igt_params_exit_handler(int sig)
 {
-	const size_t dir_len = strlen(MODULE_PARAM_DIR);
-	char file_path[PARAM_FILE_PATH_MAX_SZ];
 	struct module_param_data *data;
-	int fd;
-
-	/* We don't need to assert string sizes on this function since they were
-	 * already checked before being stored on the lists. Besides,
-	 * igt_assert() is not AS-Safe. */
-	strcpy(file_path, MODULE_PARAM_DIR);
+	int dir;
 
 	for (data = module_params; data != NULL; data = data->next) {
-		strcpy(file_path + dir_len, data->name);
-
-		fd = open(file_path, O_RDWR);
-		if (fd >= 0) {
-			int size = strlen (data->original_value);
-
-			if (size != write(fd, data->original_value, size)) {
-				const char msg[] = "WARNING: Module parameters "
-					"may not have been reset to their "
-					"original values\n";
-				assert(write(STDERR_FILENO, msg, sizeof(msg))
-				       == sizeof(msg));
-			}
-
-			close(fd);
+		dir = open(data->path, O_RDONLY);
+
+		if (!igt_sysfs_set(dir, data->name, data->original_value)) {
+			const char msg[] = "WARNING: Module parameters "
+				"may not have been reset to their "
+				"original values\n";
+			assert(write(STDERR_FILENO, msg, sizeof(msg))
+			       == sizeof(msg));
 		}
+
+		close(dir);
 	}
 	/* free() is not AS-Safe, so we can't call it here. */
 }
 
 /**
- * igt_save_module_param:
- * @name: name of the i915.ko module parameter
- * @file_path: full sysfs file path for the parameter
+ * igt_params_save:
+ * @dir: file handle for path
+ * @path: full path to the sysfs directory
+ * @name: name of the sysfs attribute
  *
- * Reads the current value of an i915.ko module parameter, saves it on an array,
- * then installs an exit handler to restore it when the program exits.
+ * Reads the current value of a sysfs attribute, saves it on an array, then
+ * installs an exit handler to restore it when the program exits.
  *
  * It is safe to call this function multiple times for the same parameter.
  *
  * Notice that this function is called by igt_set_module_param(), so that one -
  * or one of its wrappers - is the only function the test programs need to call.
  */
-static void igt_save_module_param(const char *name, const char *file_path)
+static void igt_params_save(int dir, const char *path, const char *name)
 {
 	struct module_param_data *data;
-	size_t n;
-	int fd;
 
 	/* Check if this parameter is already saved. */
 	for (data = module_params; data != NULL; data = data->next)
-		if (strncmp(data->name, name, PARAM_NAME_MAX_SZ) == 0)
+		if (strcmp(data->path, path) == 0 &&
+		    strcmp(data->name, name) == 0)
 			return;
 
 	if (!module_params)
-		igt_install_exit_handler(igt_module_param_exit_handler);
+		igt_install_exit_handler(igt_params_exit_handler);
 
 	data = calloc(1, sizeof (*data));
 	igt_assert(data);
 
-	strncpy(data->name, name, PARAM_NAME_MAX_SZ - 1);
-
-	fd = open(file_path, O_RDONLY);
-	igt_assert(fd >= 0);
+	data->path = strdup(path);
+	igt_assert(data->path);
 
-	n = read(fd, data->original_value, PARAM_VALUE_MAX_SZ);
-	igt_assert_f(n > 0 && n < PARAM_VALUE_MAX_SZ,
-		     "Need to increase PARAM_VALUE_MAX_SZ\n");
+	data->name = strdup(name);
+	igt_assert(data->name);
 
-	igt_assert(close(fd) == 0);
+	data->original_value = igt_sysfs_get(dir, name);
+	igt_assert(data->original_value);
 
 	data->next = module_params;
 	module_params = data;
@@ -205,22 +187,21 @@ bool igt_params_set(int device, const char *parameter, const char *fmt, ...)
  */
 void igt_set_module_param(const char *name, const char *val)
 {
-	char file_path[PARAM_FILE_PATH_MAX_SZ];
-	size_t len = strlen(val);
-	int fd;
+	const char *path = "/sys/module/i915/parameters";
+	int dir;
 
-	igt_assert_f(strlen(name) < PARAM_NAME_MAX_SZ,
-		     "Need to increase PARAM_NAME_MAX_SZ\n");
-	strcpy(file_path, MODULE_PARAM_DIR);
-	strcpy(file_path + strlen(MODULE_PARAM_DIR), name);
+	dir = open(path, O_RDONLY);
+	igt_assert(dir >= 0);
 
-	igt_save_module_param(name, file_path);
+	igt_params_save(dir, path, name);
 
-	fd = open(file_path, O_RDWR);
-	igt_assert(write(fd, val, len) == len);
-	igt_assert(close(fd) == 0);
+	igt_assert(igt_sysfs_set(dir, name, val));
+
+	igt_assert(close(dir) == 0);
 }
 
+#define PARAM_VALUE_MAX_SZ 16
+
 /**
  * igt_set_module_param_int:
  * @name: i915.ko parameter name
-- 
2.26.0

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