All of lore.kernel.org
 help / color / mirror / Atom feed
* [igt-dev] [PATCH i-g-t 0/8] Use device dependant module parameters
@ 2020-04-19 15:17 Juha-Pekka Heikkila
  2020-04-19 15:17 ` [igt-dev] [PATCH i-g-t 1/8] lib/params: add igt_params.c for module parameter access Juha-Pekka Heikkila
                   ` (9 more replies)
  0 siblings, 10 replies; 19+ messages in thread
From: Juha-Pekka Heikkila @ 2020-04-19 15:17 UTC (permalink / raw)
  To: igt-dev

Rebased and fixed Jani's work. Some patches changed in ways I put my
own S-b in place. These changes match to my kernel where there is path
/sys/kernel/debug/dri/<device>/i915_params/


Jani Nikula (8):
  lib/params: add igt_params.c for module parameter access
  lib/params: start renaming functions igt_params_*
  lib/params: overhaul param saving
  params open with path return
  igt/params: add generic saving module parameter set
  igt/params: use igt_params_set_save for igt_set_module_param*
  BROKEN lib/debugfs: use regular module param functions for
    prefault_disable
  tests/gem_eio: switch to using igt_params_set()

 benchmarks/gem_exec_reloc.c           |   2 +-
 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                      | 302 ++++++++++++++++++++++++++
 lib/igt_params.h                      |  47 ++++
 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 +-
 22 files changed, 401 insertions(+), 271 deletions(-)
 create mode 100644 lib/igt_params.c
 create mode 100644 lib/igt_params.h

-- 
2.17.1

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [igt-dev] [PATCH i-g-t 1/8] lib/params: add igt_params.c for module parameter access
  2020-04-19 15:17 [igt-dev] [PATCH i-g-t 0/8] Use device dependant module parameters Juha-Pekka Heikkila
@ 2020-04-19 15:17 ` Juha-Pekka Heikkila
  2020-04-19 15:17 ` [igt-dev] [PATCH i-g-t 2/8] lib/params: start renaming functions igt_params_* Juha-Pekka Heikkila
                   ` (8 subsequent siblings)
  9 siblings, 0 replies; 19+ messages in thread
From: Juha-Pekka Heikkila @ 2020-04-19 15:17 UTC (permalink / raw)
  To: igt-dev; +Cc: Jani Nikula

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>
---
 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..c9af0403 100644
--- a/lib/meson.build
+++ b/lib/meson.build
@@ -17,6 +17,7 @@ lib_sources = [
 	'igt_halffloat.c',
 	'igt_matrix.c',
 	'igt_perf.c',
+	'igt_params.c',
 	'igt_primes.c',
 	'igt_rand.c',
 	'igt_rapl.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.17.1

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

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [igt-dev] [PATCH i-g-t 2/8] lib/params: start renaming functions igt_params_*
  2020-04-19 15:17 [igt-dev] [PATCH i-g-t 0/8] Use device dependant module parameters Juha-Pekka Heikkila
  2020-04-19 15:17 ` [igt-dev] [PATCH i-g-t 1/8] lib/params: add igt_params.c for module parameter access Juha-Pekka Heikkila
@ 2020-04-19 15:17 ` Juha-Pekka Heikkila
  2020-04-20  9:33   ` Petri Latvala
  2020-04-19 15:17 ` [igt-dev] [PATCH i-g-t 3/8] lib/params: overhaul param saving Juha-Pekka Heikkila
                   ` (7 subsequent siblings)
  9 siblings, 1 reply; 19+ messages in thread
From: Juha-Pekka Heikkila @ 2020-04-19 15:17 UTC (permalink / raw)
  To: igt-dev; +Cc: Jani Nikula

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

based on patch from Jani Nikula.

Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.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.17.1

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

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [igt-dev] [PATCH i-g-t 3/8] lib/params: overhaul param saving
  2020-04-19 15:17 [igt-dev] [PATCH i-g-t 0/8] Use device dependant module parameters Juha-Pekka Heikkila
  2020-04-19 15:17 ` [igt-dev] [PATCH i-g-t 1/8] lib/params: add igt_params.c for module parameter access Juha-Pekka Heikkila
  2020-04-19 15:17 ` [igt-dev] [PATCH i-g-t 2/8] lib/params: start renaming functions igt_params_* Juha-Pekka Heikkila
@ 2020-04-19 15:17 ` Juha-Pekka Heikkila
  2020-04-19 15:17 ` [igt-dev] [PATCH i-g-t 4/8] params open with path return Juha-Pekka Heikkila
                   ` (6 subsequent siblings)
  9 siblings, 0 replies; 19+ messages in thread
From: Juha-Pekka Heikkila @ 2020-04-19 15:17 UTC (permalink / raw)
  To: igt-dev; +Cc: Jani Nikula

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

More generic, use existing functions.

Signed-off-by: Jani Nikula <jani.nikula@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.17.1

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

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [igt-dev] [PATCH i-g-t 4/8] params open with path return
  2020-04-19 15:17 [igt-dev] [PATCH i-g-t 0/8] Use device dependant module parameters Juha-Pekka Heikkila
                   ` (2 preceding siblings ...)
  2020-04-19 15:17 ` [igt-dev] [PATCH i-g-t 3/8] lib/params: overhaul param saving Juha-Pekka Heikkila
@ 2020-04-19 15:17 ` Juha-Pekka Heikkila
  2020-04-19 15:17 ` [igt-dev] [PATCH i-g-t 5/8] igt/params: add generic saving module parameter set Juha-Pekka Heikkila
                   ` (5 subsequent siblings)
  9 siblings, 0 replies; 19+ messages in thread
From: Juha-Pekka Heikkila @ 2020-04-19 15:17 UTC (permalink / raw)
  To: igt-dev; +Cc: Jani Nikula

From: Jani Nikula <jani.nikula@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.17.1

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

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [igt-dev] [PATCH i-g-t 5/8] igt/params: add generic saving module parameter set
  2020-04-19 15:17 [igt-dev] [PATCH i-g-t 0/8] Use device dependant module parameters Juha-Pekka Heikkila
                   ` (3 preceding siblings ...)
  2020-04-19 15:17 ` [igt-dev] [PATCH i-g-t 4/8] params open with path return Juha-Pekka Heikkila
@ 2020-04-19 15:17 ` Juha-Pekka Heikkila
  2020-04-19 15:17 ` [igt-dev] [PATCH i-g-t 6/8] igt/params: use igt_params_set_save for igt_set_module_param* Juha-Pekka Heikkila
                   ` (4 subsequent siblings)
  9 siblings, 0 replies; 19+ messages in thread
From: Juha-Pekka Heikkila @ 2020-04-19 15:17 UTC (permalink / raw)
  To: igt-dev; +Cc: Jani Nikula

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

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 lib/igt_params.c | 56 ++++++++++++++++++++++++++++++++++++++++--------
 lib/igt_params.h |  3 +++
 2 files changed, 50 insertions(+), 9 deletions(-)

diff --git a/lib/igt_params.c b/lib/igt_params.c
index fe4b1df3..8de47b02 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,35 @@ 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 Intel)
+ * @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.17.1

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

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [igt-dev] [PATCH i-g-t 6/8] igt/params: use igt_params_set_save for igt_set_module_param*
  2020-04-19 15:17 [igt-dev] [PATCH i-g-t 0/8] Use device dependant module parameters Juha-Pekka Heikkila
                   ` (4 preceding siblings ...)
  2020-04-19 15:17 ` [igt-dev] [PATCH i-g-t 5/8] igt/params: add generic saving module parameter set Juha-Pekka Heikkila
@ 2020-04-19 15:17 ` Juha-Pekka Heikkila
  2020-04-19 15:17 ` [igt-dev] [PATCH i-g-t 7/8] BROKEN lib/debugfs: use regular module param functions for prefault_disable Juha-Pekka Heikkila
                   ` (3 subsequent siblings)
  9 siblings, 0 replies; 19+ messages in thread
From: Juha-Pekka Heikkila @ 2020-04-19 15:17 UTC (permalink / raw)
  To: igt-dev; +Cc: Jani Nikula

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

Unify parameter access. Based on original patch from Jani Nikula.

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

diff --git a/lib/igt_params.c b/lib/igt_params.c
index 8de47b02..0a1474c3 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,42 @@ 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;
 
-	dir = igt_sysfs_open(device);
+	dir = igt_debugfs_dir(device);
 	if (dir >= 0) {
 		params = openat(dir,
-				"device/driver/module/parameters",
+				"i915_params",
 				O_RDONLY);
+		if (params >= 0 ) {
+			char* debugfspath = malloc(PATH_MAX);
+			igt_debugfs_path(device, debugfspath, PATH_MAX);
+			if (param != NULL) {
+				struct stat buffer;
+				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);
 	}
 
@@ -150,7 +179,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 +190,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;
 
@@ -232,21 +261,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
@@ -257,12 +274,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.17.1

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

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [igt-dev] [PATCH i-g-t 7/8] BROKEN lib/debugfs: use regular module param functions for prefault_disable
  2020-04-19 15:17 [igt-dev] [PATCH i-g-t 0/8] Use device dependant module parameters Juha-Pekka Heikkila
                   ` (5 preceding siblings ...)
  2020-04-19 15:17 ` [igt-dev] [PATCH i-g-t 6/8] igt/params: use igt_params_set_save for igt_set_module_param* Juha-Pekka Heikkila
@ 2020-04-19 15:17 ` Juha-Pekka Heikkila
  2020-04-19 15:17 ` [igt-dev] [PATCH i-g-t 8/8] tests/gem_eio: switch to using igt_params_set() Juha-Pekka Heikkila
                   ` (2 subsequent siblings)
  9 siblings, 0 replies; 19+ messages in thread
From: Juha-Pekka Heikkila @ 2020-04-19 15:17 UTC (permalink / raw)
  To: igt-dev; +Cc: Jani Nikula

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

Functional change switches from igt_require to igt_assert on errors.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 benchmarks/gem_exec_reloc.c |  2 +-
 lib/igt_params.c            | 24 ++++++++++++++++++++++++
 lib/igt_params.h            |  7 +++++++
 3 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/benchmarks/gem_exec_reloc.c b/benchmarks/gem_exec_reloc.c
index 0a2454cb..d7841b58 100644
--- a/benchmarks/gem_exec_reloc.c
+++ b/benchmarks/gem_exec_reloc.c
@@ -38,7 +38,7 @@
 #include "drm.h"
 #include "intel_reg.h"
 #include "ioctl_wrappers.h"
-#include "igt_debugfs.h"
+#include "igt_params.h"
 #include "drmtest.h"
 #include "i915/gem_mman.h"
 
diff --git a/lib/igt_params.c b/lib/igt_params.c
index 0a1474c3..80ec586c 100644
--- a/lib/igt_params.c
+++ b/lib/igt_params.c
@@ -276,3 +276,27 @@ void igt_set_module_param_int(const char *name, int val)
 {
 	igt_assert(igt_params_set_save(-1, name, "%d", val));
 }
+
+/**
+ * igt_disable_prefault:
+ *
+ * Disable prefaulting in certain gem ioctls through the module param. As usual
+ * this installs an exit handler to clean up and re-enable prefaulting even when
+ * the test exited abnormally.
+ *
+ * igt_enable_prefault() will enable normale operation again.
+ */
+void igt_disable_prefault(void)
+{
+	igt_require(igt_params_set_save(-1, "prefault_disable", "Y"));
+}
+
+/**
+ * igt_enable_prefault:
+ *
+ * Enable prefault (again) through the module param.
+ */
+void igt_enable_prefault(void)
+{
+	igt_require(igt_params_set_save(-1, "prefault_disable", "N"));
+}
diff --git a/lib/igt_params.h b/lib/igt_params.h
index 1b750d23..f18ea436 100644
--- a/lib/igt_params.h
+++ b/lib/igt_params.h
@@ -37,4 +37,11 @@ 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);
 
+/*
+ * Prefault control
+ */
+
+void igt_disable_prefault(void);
+void igt_enable_prefault(void);
+
 #endif /* __IGT_PARAMS_H__ */
-- 
2.17.1

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

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [igt-dev] [PATCH i-g-t 8/8] tests/gem_eio: switch to using igt_params_set()
  2020-04-19 15:17 [igt-dev] [PATCH i-g-t 0/8] Use device dependant module parameters Juha-Pekka Heikkila
                   ` (6 preceding siblings ...)
  2020-04-19 15:17 ` [igt-dev] [PATCH i-g-t 7/8] BROKEN lib/debugfs: use regular module param functions for prefault_disable Juha-Pekka Heikkila
@ 2020-04-19 15:17 ` Juha-Pekka Heikkila
  2020-04-19 15:31 ` [igt-dev] ✗ GitLab.Pipeline: warning for Use device dependant module parameters Patchwork
  2020-04-19 15:47 ` [igt-dev] ✗ Fi.CI.BAT: failure " Patchwork
  9 siblings, 0 replies; 19+ messages in thread
From: Juha-Pekka Heikkila @ 2020-04-19 15:17 UTC (permalink / raw)
  To: igt-dev; +Cc: Jani Nikula

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

Based on patch from Jani Nikula.

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

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

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [igt-dev] ✗ GitLab.Pipeline: warning for Use device dependant module parameters
  2020-04-19 15:17 [igt-dev] [PATCH i-g-t 0/8] Use device dependant module parameters Juha-Pekka Heikkila
                   ` (7 preceding siblings ...)
  2020-04-19 15:17 ` [igt-dev] [PATCH i-g-t 8/8] tests/gem_eio: switch to using igt_params_set() Juha-Pekka Heikkila
@ 2020-04-19 15:31 ` Patchwork
  2020-04-19 15:47 ` [igt-dev] ✗ Fi.CI.BAT: failure " Patchwork
  9 siblings, 0 replies; 19+ messages in thread
From: Patchwork @ 2020-04-19 15:31 UTC (permalink / raw)
  To: Juha-Pekka Heikkila; +Cc: igt-dev

== Series Details ==

Series: Use device dependant module parameters
URL   : https://patchwork.freedesktop.org/series/76163/
State : warning

== Summary ==

Did not get list of undocumented tests for this run, something is wrong!

Other than that, pipeline status: FAILED.

see https://gitlab.freedesktop.org/gfx-ci/igt-ci-tags/pipelines/134147 for the overview.

containers:igt has failed (https://gitlab.freedesktop.org/gfx-ci/igt-ci-tags/-/jobs/2338956):
  
  Skipping Git submodules setup
  section_end:1587310256:get_sources
  section_start:1587310256:restore_cache
  section_end:1587310258:restore_cache
  section_start:1587310258:download_artifacts
  Downloading artifacts for build:tests-fedora (2338938)...
  Downloading artifacts from coordinator... ok        id=2338938 responseStatus=200 OK token=8Yzfbjnn
  section_end:1587310263:download_artifacts
  section_start:1587310263:build_script
  Authenticating with credentials from job payload (GitLab Registry)
  $ podman login -u gitlab-ci-token -p $CI_JOB_TOKEN $CI_REGISTRY
  Error: error authenticating creds for "registry.freedesktop.org": unexpected http code: 403 (Forbidden), URL: https://gitlab.freedesktop.org/jwt/auth?account=gitlab-ci-token&service=container_registry
  section_end:1587310264:build_script
  section_start:1587310264:after_script
  section_end:1587310266:after_script
  section_start:1587310266:upload_artifacts_on_failure
  section_end:1587310268:upload_artifacts_on_failure
  ERROR: Job failed: exit code 1

== Logs ==

For more details see: https://gitlab.freedesktop.org/gfx-ci/igt-ci-tags/pipelines/134147
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [igt-dev] ✗ Fi.CI.BAT: failure for Use device dependant module parameters
  2020-04-19 15:17 [igt-dev] [PATCH i-g-t 0/8] Use device dependant module parameters Juha-Pekka Heikkila
                   ` (8 preceding siblings ...)
  2020-04-19 15:31 ` [igt-dev] ✗ GitLab.Pipeline: warning for Use device dependant module parameters Patchwork
@ 2020-04-19 15:47 ` Patchwork
  9 siblings, 0 replies; 19+ messages in thread
From: Patchwork @ 2020-04-19 15:47 UTC (permalink / raw)
  To: Juha-Pekka Heikkila; +Cc: igt-dev

== Series Details ==

Series: Use device dependant module parameters
URL   : https://patchwork.freedesktop.org/series/76163/
State : failure

== Summary ==

CI Bug Log - changes from IGT_5601 -> IGTPW_4488
====================================================

Summary
-------

  **FAILURE**

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

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

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

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

### IGT changes ###

#### Possible regressions ####

  * igt@kms_force_connector_basic@force-load-detect:
    - fi-hsw-4770:        [PASS][1] -> [FAIL][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5601/fi-hsw-4770/igt@kms_force_connector_basic@force-load-detect.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4488/fi-hsw-4770/igt@kms_force_connector_basic@force-load-detect.html
    - fi-elk-e7500:       [PASS][3] -> [FAIL][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5601/fi-elk-e7500/igt@kms_force_connector_basic@force-load-detect.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4488/fi-elk-e7500/igt@kms_force_connector_basic@force-load-detect.html
    - fi-ivb-3770:        [PASS][5] -> [FAIL][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5601/fi-ivb-3770/igt@kms_force_connector_basic@force-load-detect.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4488/fi-ivb-3770/igt@kms_force_connector_basic@force-load-detect.html
    - fi-byt-j1900:       [PASS][7] -> [FAIL][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5601/fi-byt-j1900/igt@kms_force_connector_basic@force-load-detect.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4488/fi-byt-j1900/igt@kms_force_connector_basic@force-load-detect.html
    - fi-blb-e6850:       [PASS][9] -> [FAIL][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5601/fi-blb-e6850/igt@kms_force_connector_basic@force-load-detect.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4488/fi-blb-e6850/igt@kms_force_connector_basic@force-load-detect.html
    - fi-pnv-d510:        [PASS][11] -> [FAIL][12]
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5601/fi-pnv-d510/igt@kms_force_connector_basic@force-load-detect.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4488/fi-pnv-d510/igt@kms_force_connector_basic@force-load-detect.html
    - fi-snb-2520m:       [PASS][13] -> [FAIL][14]
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5601/fi-snb-2520m/igt@kms_force_connector_basic@force-load-detect.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4488/fi-snb-2520m/igt@kms_force_connector_basic@force-load-detect.html
    - fi-gdg-551:         [PASS][15] -> [FAIL][16]
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5601/fi-gdg-551/igt@kms_force_connector_basic@force-load-detect.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4488/fi-gdg-551/igt@kms_force_connector_basic@force-load-detect.html
    - fi-bwr-2160:        [PASS][17] -> [FAIL][18]
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5601/fi-bwr-2160/igt@kms_force_connector_basic@force-load-detect.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4488/fi-bwr-2160/igt@kms_force_connector_basic@force-load-detect.html
    - fi-snb-2600:        [PASS][19] -> [FAIL][20]
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5601/fi-snb-2600/igt@kms_force_connector_basic@force-load-detect.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4488/fi-snb-2600/igt@kms_force_connector_basic@force-load-detect.html

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

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

### IGT changes ###

#### Issues hit ####

  * igt@i915_module_load@reload:
    - fi-skl-6770hq:      [PASS][21] -> [DMESG-WARN][22] ([i915#203])
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5601/fi-skl-6770hq/igt@i915_module_load@reload.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4488/fi-skl-6770hq/igt@i915_module_load@reload.html

  * igt@i915_pm_rpm@basic-pci-d3-state:
    - fi-skl-6770hq:      [PASS][23] -> [SKIP][24] ([fdo#109271]) +2 similar issues
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5601/fi-skl-6770hq/igt@i915_pm_rpm@basic-pci-d3-state.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4488/fi-skl-6770hq/igt@i915_pm_rpm@basic-pci-d3-state.html

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
    - fi-skl-6770hq:      [PASS][25] -> [DMESG-FAIL][26] ([i915#165])
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5601/fi-skl-6770hq/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4488/fi-skl-6770hq/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a.html

  
#### Warnings ####

  * igt@i915_pm_rpm@module-reload:
    - fi-skl-6770hq:      [FAIL][27] ([i915#178]) -> [DMESG-WARN][28] ([i915#203])
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_5601/fi-skl-6770hq/igt@i915_pm_rpm@module-reload.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4488/fi-skl-6770hq/igt@i915_pm_rpm@module-reload.html

  
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [i915#165]: https://gitlab.freedesktop.org/drm/intel/issues/165
  [i915#178]: https://gitlab.freedesktop.org/drm/intel/issues/178
  [i915#203]: https://gitlab.freedesktop.org/drm/intel/issues/203


Participating hosts (50 -> 45)
------------------------------

  Additional (1): fi-kbl-7560u 
  Missing    (6): fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-byt-clapper fi-bdw-samus 


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

  * CI: CI-20190529 -> None
  * IGT: IGT_5601 -> IGTPW_4488

  CI-20190529: 20190529
  CI_DRM_8324: 279672c3e0717ef7047b97b49d98636ef2242a91 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_4488: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4488/index.html
  IGT_5601: d3422eb9bfbebab8a97687bf29552a471473963b @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [igt-dev] [PATCH i-g-t 2/8] lib/params: start renaming functions igt_params_*
  2020-04-19 15:17 ` [igt-dev] [PATCH i-g-t 2/8] lib/params: start renaming functions igt_params_* Juha-Pekka Heikkila
@ 2020-04-20  9:33   ` Petri Latvala
  2020-04-21  7:38     ` Juha-Pekka Heikkila
  0 siblings, 1 reply; 19+ messages in thread
From: Petri Latvala @ 2020-04-20  9:33 UTC (permalink / raw)
  To: Juha-Pekka Heikkila; +Cc: igt-dev, Jani Nikula

On Sun, Apr 19, 2020 at 06:17:44PM +0300, Juha-Pekka Heikkila wrote:
> From: Jani Nikula <jani.nikula@intel.com>
> 
> based on patch from Jani Nikula.


From Jani, and then based on Jani.

Please keep the original S-o-b as well as your own.


-- 
Petri Latvala



> 
> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.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.17.1
> 
> _______________________________________________
> 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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [igt-dev] [PATCH i-g-t 2/8] lib/params: start renaming functions igt_params_*
  2020-04-20  9:33   ` Petri Latvala
@ 2020-04-21  7:38     ` Juha-Pekka Heikkila
  2020-04-21  8:07       ` Jani Nikula
  0 siblings, 1 reply; 19+ messages in thread
From: Juha-Pekka Heikkila @ 2020-04-21  7:38 UTC (permalink / raw)
  To: Petri Latvala; +Cc: igt-dev, Jani Nikula

On 20.4.2020 12.33, Petri Latvala wrote:
> On Sun, Apr 19, 2020 at 06:17:44PM +0300, Juha-Pekka Heikkila wrote:
>> From: Jani Nikula <jani.nikula@intel.com>
>>
>> based on patch from Jani Nikula.
> 
> 
>  From Jani, and then based on Jani.
> 
> Please keep the original S-o-b as well as your own.
> 
> 

Yea, that look like a mess now. I'm thinking I should take authorship on 
changed patches. I don't think I can keep Jani's S-o-b for changed 
patches. I'll anyway need to make changes yet so this it's not final 
revision, currently I have i915 hardcoded there for parameter path.

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [igt-dev] [PATCH i-g-t 2/8] lib/params: start renaming functions igt_params_*
  2020-04-21  7:38     ` Juha-Pekka Heikkila
@ 2020-04-21  8:07       ` Jani Nikula
  0 siblings, 0 replies; 19+ messages in thread
From: Jani Nikula @ 2020-04-21  8:07 UTC (permalink / raw)
  To: juhapekka.heikkila, Petri Latvala; +Cc: igt-dev

On Tue, 21 Apr 2020, Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com> wrote:
> On 20.4.2020 12.33, Petri Latvala wrote:
>> On Sun, Apr 19, 2020 at 06:17:44PM +0300, Juha-Pekka Heikkila wrote:
>>> From: Jani Nikula <jani.nikula@intel.com>
>>>
>>> based on patch from Jani Nikula.
>> 
>> 
>>  From Jani, and then based on Jani.
>> 
>> Please keep the original S-o-b as well as your own.
>> 
>> 
>
> Yea, that look like a mess now. I'm thinking I should take authorship on 
> changed patches. I don't think I can keep Jani's S-o-b for changed 
> patches. I'll anyway need to make changes yet so this it's not final 
> revision, currently I have i915 hardcoded there for parameter path.

Like I've said way back when, it's fine with me for you to take
authorship when you change the patches. The starting point was a hacky
pile of patches that I threw over the fence, and I think it would be
silly for me to insist on retaining the authorship.

That said, Signed-off-by is a legal thing, and you *should* retain
that. See https://developercertificate.org/ for what it signifies.

BR,
Jani.

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [igt-dev] [PATCH i-g-t 1/8] lib/params: add igt_params.c for module parameter access
  2020-04-22  8:13     ` Juha-Pekka Heikkila
@ 2020-04-22  8:19       ` Petri Latvala
  0 siblings, 0 replies; 19+ messages in thread
From: Petri Latvala @ 2020-04-22  8:19 UTC (permalink / raw)
  To: Juha-Pekka Heikkila; +Cc: igt-dev, Jani Nikula

On Wed, Apr 22, 2020 at 11:13:43AM +0300, Juha-Pekka Heikkila wrote:
> On 22.4.2020 11.02, Petri Latvala wrote:
> > On Tue, Apr 21, 2020 at 07:17:18PM +0300, Juha-Pekka Heikkila wrote:
> > > 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>
> > 
> > 
> > This patch is just moving code around without functional changes,
> > correct?
> 
> Yes, it's correct. It's just buildup for those changes that follow in those
> other patches.

Alright. Code motion patches are impossible to determine to be just
code motion without magic eyes. :P

> 
> > 
> > 
> > > diff --git a/lib/meson.build b/lib/meson.build
> > > index e2060430..c9af0403 100644
> > > --- a/lib/meson.build
> > > +++ b/lib/meson.build
> > > @@ -17,6 +17,7 @@ lib_sources = [
> > >   	'igt_halffloat.c',
> > >   	'igt_matrix.c',
> > >   	'igt_perf.c',
> > > +	'igt_params.c',
> > >   	'igt_primes.c',
> > >   	'igt_rand.c',
> > >   	'igt_rapl.c',
> > 
> > Alphabetical ordering is off-by-one here.
> 
> I'll rearrange.


With that done, slap in a

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [igt-dev] [PATCH i-g-t 1/8] lib/params: add igt_params.c for module parameter access
  2020-04-22  8:02   ` Petri Latvala
@ 2020-04-22  8:13     ` Juha-Pekka Heikkila
  2020-04-22  8:19       ` Petri Latvala
  0 siblings, 1 reply; 19+ messages in thread
From: Juha-Pekka Heikkila @ 2020-04-22  8:13 UTC (permalink / raw)
  To: Petri Latvala; +Cc: igt-dev, Jani Nikula

On 22.4.2020 11.02, Petri Latvala wrote:
> On Tue, Apr 21, 2020 at 07:17:18PM +0300, Juha-Pekka Heikkila wrote:
>> 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>
> 
> 
> This patch is just moving code around without functional changes,
> correct?

Yes, it's correct. It's just buildup for those changes that follow in 
those other patches.

> 
> 
>> diff --git a/lib/meson.build b/lib/meson.build
>> index e2060430..c9af0403 100644
>> --- a/lib/meson.build
>> +++ b/lib/meson.build
>> @@ -17,6 +17,7 @@ lib_sources = [
>>   	'igt_halffloat.c',
>>   	'igt_matrix.c',
>>   	'igt_perf.c',
>> +	'igt_params.c',
>>   	'igt_primes.c',
>>   	'igt_rand.c',
>>   	'igt_rapl.c',
> 
> Alphabetical ordering is off-by-one here.

I'll rearrange.

> 
> 
> 

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [igt-dev] [PATCH i-g-t 1/8] lib/params: add igt_params.c for module parameter access
  2020-04-21 16:17 ` [igt-dev] [PATCH i-g-t 1/8] lib/params: add igt_params.c for module parameter access Juha-Pekka Heikkila
@ 2020-04-22  8:02   ` Petri Latvala
  2020-04-22  8:13     ` Juha-Pekka Heikkila
  0 siblings, 1 reply; 19+ messages in thread
From: Petri Latvala @ 2020-04-22  8:02 UTC (permalink / raw)
  To: Juha-Pekka Heikkila; +Cc: igt-dev, Jani Nikula

On Tue, Apr 21, 2020 at 07:17:18PM +0300, Juha-Pekka Heikkila wrote:
> 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>


This patch is just moving code around without functional changes,
correct?


> diff --git a/lib/meson.build b/lib/meson.build
> index e2060430..c9af0403 100644
> --- a/lib/meson.build
> +++ b/lib/meson.build
> @@ -17,6 +17,7 @@ lib_sources = [
>  	'igt_halffloat.c',
>  	'igt_matrix.c',
>  	'igt_perf.c',
> +	'igt_params.c',
>  	'igt_primes.c',
>  	'igt_rand.c',
>  	'igt_rapl.c',

Alphabetical ordering is off-by-one here.



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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [igt-dev] [PATCH i-g-t 1/8] lib/params: add igt_params.c for module parameter access
  2020-04-21 16:17 [igt-dev] [PATCH i-g-t 0/8] Use device dependant module parameters Juha-Pekka Heikkila
@ 2020-04-21 16:17 ` Juha-Pekka Heikkila
  2020-04-22  8:02   ` Petri Latvala
  0 siblings, 1 reply; 19+ messages in thread
From: Juha-Pekka Heikkila @ 2020-04-21 16:17 UTC (permalink / raw)
  To: igt-dev; +Cc: Jani Nikula

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>
---
 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..c9af0403 100644
--- a/lib/meson.build
+++ b/lib/meson.build
@@ -17,6 +17,7 @@ lib_sources = [
 	'igt_halffloat.c',
 	'igt_matrix.c',
 	'igt_perf.c',
+	'igt_params.c',
 	'igt_primes.c',
 	'igt_rand.c',
 	'igt_rapl.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.17.1

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

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [igt-dev] [PATCH i-g-t 1/8] lib/params: add igt_params.c for module parameter access
  2020-04-20 12:17 [igt-dev] [PATCH i-g-t 0/8] " Juha-Pekka Heikkila
@ 2020-04-20 12:17 ` Juha-Pekka Heikkila
  0 siblings, 0 replies; 19+ messages in thread
From: Juha-Pekka Heikkila @ 2020-04-20 12:17 UTC (permalink / raw)
  To: igt-dev; +Cc: Jani Nikula

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>
---
 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..c9af0403 100644
--- a/lib/meson.build
+++ b/lib/meson.build
@@ -17,6 +17,7 @@ lib_sources = [
 	'igt_halffloat.c',
 	'igt_matrix.c',
 	'igt_perf.c',
+	'igt_params.c',
 	'igt_primes.c',
 	'igt_rand.c',
 	'igt_rapl.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

^ permalink raw reply related	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2020-04-22  8:20 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-19 15:17 [igt-dev] [PATCH i-g-t 0/8] Use device dependant module parameters Juha-Pekka Heikkila
2020-04-19 15:17 ` [igt-dev] [PATCH i-g-t 1/8] lib/params: add igt_params.c for module parameter access Juha-Pekka Heikkila
2020-04-19 15:17 ` [igt-dev] [PATCH i-g-t 2/8] lib/params: start renaming functions igt_params_* Juha-Pekka Heikkila
2020-04-20  9:33   ` Petri Latvala
2020-04-21  7:38     ` Juha-Pekka Heikkila
2020-04-21  8:07       ` Jani Nikula
2020-04-19 15:17 ` [igt-dev] [PATCH i-g-t 3/8] lib/params: overhaul param saving Juha-Pekka Heikkila
2020-04-19 15:17 ` [igt-dev] [PATCH i-g-t 4/8] params open with path return Juha-Pekka Heikkila
2020-04-19 15:17 ` [igt-dev] [PATCH i-g-t 5/8] igt/params: add generic saving module parameter set Juha-Pekka Heikkila
2020-04-19 15:17 ` [igt-dev] [PATCH i-g-t 6/8] igt/params: use igt_params_set_save for igt_set_module_param* Juha-Pekka Heikkila
2020-04-19 15:17 ` [igt-dev] [PATCH i-g-t 7/8] BROKEN lib/debugfs: use regular module param functions for prefault_disable Juha-Pekka Heikkila
2020-04-19 15:17 ` [igt-dev] [PATCH i-g-t 8/8] tests/gem_eio: switch to using igt_params_set() Juha-Pekka Heikkila
2020-04-19 15:31 ` [igt-dev] ✗ GitLab.Pipeline: warning for Use device dependant module parameters Patchwork
2020-04-19 15:47 ` [igt-dev] ✗ Fi.CI.BAT: failure " Patchwork
2020-04-20 12:17 [igt-dev] [PATCH i-g-t 0/8] " Juha-Pekka Heikkila
2020-04-20 12:17 ` [igt-dev] [PATCH i-g-t 1/8] lib/params: add igt_params.c for module parameter access Juha-Pekka Heikkila
2020-04-21 16:17 [igt-dev] [PATCH i-g-t 0/8] Use device dependant module parameters Juha-Pekka Heikkila
2020-04-21 16:17 ` [igt-dev] [PATCH i-g-t 1/8] lib/params: add igt_params.c for module parameter access Juha-Pekka Heikkila
2020-04-22  8:02   ` Petri Latvala
2020-04-22  8:13     ` Juha-Pekka Heikkila
2020-04-22  8:19       ` Petri Latvala

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.