* [igt-dev] [PATCH i-g-t] lib/igt_sysfs: add asserting helpers for read/write operations
@ 2023-07-12 11:45 Lukasz Laguna
2023-07-12 12:32 ` Kamil Konieczny
0 siblings, 1 reply; 7+ messages in thread
From: Lukasz Laguna @ 2023-07-12 11:45 UTC (permalink / raw)
To: igt-dev; +Cc: kamil.konieczny
Prefix names of existing, non-asserting helpers with "__":
- igt_sysfs_get_u32 -> __igt_sysfs_get_u32
- igt_sysfs_set_u32 -> __igt_sysfs_set_u32
- igt_sysfs_get_u64 -> __igt_sysfs_get_u64
- igt_sysfs_set_u64 -> __igt_sysfs_set_u64
- igt_sysfs_get_boolean -> __igt_sysfs_get_boolean
- igt_sysfs_set_boolean -> __igt_sysfs_set_boolean
Replace calls to don't introduce any functional changes in the
existing code.
Additionally, reimplement non-asserting get helpers to return boolean
result of the read operation and store the read value via pointer
passed as function parameter. In previous implementation, it wasn't
possible to distinguish if returned zero was a read value or failure on
a read attempt.
On the occasion, fix a typo in modified debug message and fix a bug in
igt_aux_enable_pm_suspend_dbg() function, because of which tests started
asserting after introduced changes.
Signed-off-by: Lukasz Laguna <lukasz.laguna@intel.com>
---
lib/i915/gem_submission.c | 4 +-
lib/igt_aux.c | 4 +-
lib/igt_pm.c | 2 +-
lib/igt_sysfs.c | 220 +++++++++++++++++++++++++--------
lib/igt_sysfs.h | 12 +-
tests/i915/i915_pm_dc.c | 6 +-
tests/i915/i915_pm_freq_mult.c | 42 ++++---
tests/i915/i915_pm_rps.c | 32 ++---
tests/i915/i915_power.c | 9 +-
tests/i915/perf_pmu.c | 45 ++++---
tests/kms_prime.c | 2 +-
tests/nouveau_crc.c | 7 +-
12 files changed, 261 insertions(+), 124 deletions(-)
diff --git a/lib/i915/gem_submission.c b/lib/i915/gem_submission.c
index adf5eb394..7d1c3970f 100644
--- a/lib/i915/gem_submission.c
+++ b/lib/i915/gem_submission.c
@@ -65,12 +65,14 @@ unsigned gem_submission_method(int fd)
const int gen = intel_gen(intel_get_drm_devid(fd));
unsigned method = GEM_SUBMISSION_RINGBUF;
int dir;
+ uint32_t value = 0;
dir = igt_params_open(fd);
if (dir < 0)
return 0;
- if (igt_sysfs_get_u32(dir, "enable_guc") & 1) {
+ __igt_sysfs_get_u32(dir, "enable_guc", &value);
+ if (value & 1) {
method = GEM_SUBMISSION_GUC;
} else if (gen >= 8) {
method = GEM_SUBMISSION_EXECLISTS;
diff --git a/lib/igt_aux.c b/lib/igt_aux.c
index fd5103043..1821ace67 100644
--- a/lib/igt_aux.c
+++ b/lib/igt_aux.c
@@ -953,7 +953,7 @@ static void igt_aux_enable_pm_suspend_dbg(int power_dir)
if (sysfs_fd > 0) {
__console_suspend_saved_state = igt_sysfs_get_boolean(sysfs_fd, "console_suspend");
- if (!igt_sysfs_set_boolean(sysfs_fd, "console_suspend", CONSOLE_SUSPEND_DISABLE))
+ if (!__igt_sysfs_set_boolean(sysfs_fd, "console_suspend", CONSOLE_SUSPEND_DISABLE))
igt_warn("Unable to disable console suspend\n");
} else {
@@ -962,7 +962,7 @@ static void igt_aux_enable_pm_suspend_dbg(int power_dir)
/* pm_debug_messages depends on CONFIG_PM_SLEEP_DEBUG */
if (!faccessat(power_dir, "pm_debug_messages", R_OK | W_OK, 0)) {
- __pm_debug_messages_state = igt_sysfs_get_boolean(sysfs_fd, "pm_debug_messages");
+ __pm_debug_messages_state = igt_sysfs_get_boolean(power_dir, "pm_debug_messages");
igt_sysfs_set_boolean(power_dir, "pm_debug_messages", true);
}
diff --git a/lib/igt_pm.c b/lib/igt_pm.c
index 18c84bf3a..ba591f0f8 100644
--- a/lib/igt_pm.c
+++ b/lib/igt_pm.c
@@ -1415,5 +1415,5 @@ void igt_pm_ignore_slpc_efficient_freq(int i915, int gtfd, bool val)
return;
igt_require(igt_sysfs_has_attr(gtfd, "slpc_ignore_eff_freq"));
- igt_assert(igt_sysfs_set_u32(gtfd, "slpc_ignore_eff_freq", val));
+ igt_sysfs_set_u32(gtfd, "slpc_ignore_eff_freq", val);
}
diff --git a/lib/igt_sysfs.c b/lib/igt_sysfs.c
index 412edfc29..83182020b 100644
--- a/lib/igt_sysfs.c
+++ b/lib/igt_sysfs.c
@@ -565,122 +565,236 @@ int igt_sysfs_printf(int dir, const char *attr, const char *fmt, ...)
return ret;
}
+/**
+ * __igt_sysfs_get_u32:
+ * @dir: directory corresponding to attribute
+ * @attr: name of the sysfs node to read
+ * @value: pointer for storing read value
+ *
+ * Convenience wrapper to read a unsigned 32bit integer from a sysfs file.
+ *
+ * Returns:
+ * True if value successfully read, false otherwise.
+ */
+bool __igt_sysfs_get_u32(int dir, const char *attr, uint32_t *value)
+{
+ if (igt_debug_on(igt_sysfs_scanf(dir, attr, "%u", value) != 1))
+ return false;
+
+ return true;
+}
+
/**
* igt_sysfs_get_u32:
- * @dir: directory for the device from igt_sysfs_open()
- * @attr: name of the sysfs node to open
+ * @dir: directory corresponding to attribute
+ * @attr: name of the sysfs node to read
*
* Convenience wrapper to read a unsigned 32bit integer from a sysfs file.
+ * It asserts on failure.
*
* Returns:
- * The value read.
+ * Read value.
*/
uint32_t igt_sysfs_get_u32(int dir, const char *attr)
{
- uint32_t result;
+ uint32_t value;
+
+ igt_assert_f(__igt_sysfs_get_u32(dir, attr, &value),
+ "Failed to read %s attribute (%s)\n", attr, strerror(errno));
+
+ return value;
+}
+
+/**
+ * __igt_sysfs_set_u32:
+ * @dir: directory corresponding to attribute
+ * @attr: name of the sysfs node to write
+ * @value: value to set
+ *
+ * Convenience wrapper to write a unsigned 32bit integer to a sysfs file.
+ *
+ * Returns:
+ * True if successfully written, false otherwise.
+ */
+bool __igt_sysfs_set_u32(int dir, const char *attr, uint32_t value)
+{
+ return igt_sysfs_printf(dir, attr, "%u", value) > 0;
+}
+
+/**
+ * igt_sysfs_set_u32:
+ * @dir: directory corresponding to attribute
+ * @attr: name of the sysfs node to write
+ * @value: value to set
+ *
+ * Convenience wrapper to write a unsigned 32bit integer to a sysfs file.
+ * It asserts on failure.
+ */
+void igt_sysfs_set_u32(int dir, const char *attr, uint32_t value)
+{
+ igt_assert_f(__igt_sysfs_set_u32(dir, attr, value),
+ "Failed to write %u to %s attribute (%s)\n", value, attr, strerror(errno));
+}
- if (igt_debug_on(igt_sysfs_scanf(dir, attr, "%u", &result) != 1))
- return 0;
+/**
+ * __igt_sysfs_get_u64:
+ * @dir: directory corresponding to attribute
+ * @attr: name of the sysfs node to read
+ * @value: pointer for storing read value
+ *
+ * Convenience wrapper to read a unsigned 64bit integer from a sysfs file.
+ *
+ * Returns:
+ * True if value successfully read, false otherwise.
+ */
+bool __igt_sysfs_get_u64(int dir, const char *attr, uint64_t *value)
+{
+ if (igt_debug_on(igt_sysfs_scanf(dir, attr, "%"PRIu64, value) != 1))
+ return false;
- return result;
+ return true;
}
/**
* igt_sysfs_get_u64:
- * @dir: directory for the device from igt_sysfs_open()
- * @attr: name of the sysfs node to open
+ * @dir: directory corresponding to attribute
+ * @attr: name of the sysfs node to read
*
* Convenience wrapper to read a unsigned 64bit integer from a sysfs file.
+ * It asserts on failure.
*
* Returns:
- * The value read.
+ * Read value.
*/
uint64_t igt_sysfs_get_u64(int dir, const char *attr)
{
- uint64_t result;
+ uint64_t value;
- if (igt_debug_on(igt_sysfs_scanf(dir, attr, "%"PRIu64, &result) != 1))
- return 0;
+ igt_assert_f(__igt_sysfs_get_u64(dir, attr, &value),
+ "Failed to read %s attribute (%s)\n", attr, strerror(errno));
- return result;
+ return value;
}
/**
- * igt_sysfs_set_u64:
- * @dir: directory for the device from igt_sysfs_open()
- * @attr: name of the sysfs node to open
+ * __igt_sysfs_set_u64:
+ * @dir: directory corresponding to attribute
+ * @attr: name of the sysfs node to write
* @value: value to set
*
* Convenience wrapper to write a unsigned 64bit integer to a sysfs file.
*
* Returns:
- * True if successfully written
+ * True if successfully written, false otherwise.
*/
-bool igt_sysfs_set_u64(int dir, const char *attr, uint64_t value)
+bool __igt_sysfs_set_u64(int dir, const char *attr, uint64_t value)
{
return igt_sysfs_printf(dir, attr, "%"PRIu64, value) > 0;
}
/**
- * igt_sysfs_set_u32:
- * @dir: directory for the device from igt_sysfs_open()
- * @attr: name of the sysfs node to open
+ * igt_sysfs_set_u64:
+ * @dir: directory corresponding to attribute
+ * @attr: name of the sysfs node to write
* @value: value to set
*
- * Convenience wrapper to write a unsigned 32bit integer to a sysfs file.
- *
- * Returns:
- * True if successfully written
+ * Convenience wrapper to write a unsigned 64bit integer to a sysfs file.
+ * It asserts on failure.
*/
-bool igt_sysfs_set_u32(int dir, const char *attr, uint32_t value)
+void igt_sysfs_set_u64(int dir, const char *attr, uint64_t value)
{
- return igt_sysfs_printf(dir, attr, "%u", value) > 0;
+ igt_assert_f(__igt_sysfs_set_u64(dir, attr, value),
+ "Failed to write %"PRIu64" to %s attribute (%s)\n",
+ value, attr, strerror(errno));
}
/**
- * igt_sysfs_get_boolean:
- * @dir: directory for the device from igt_sysfs_open()
- * @attr: name of the sysfs node to open
+ * __igt_sysfs_get_boolean:
+ * @dir: directory corresponding to attribute
+ * @attr: name of the sysfs node to read
+ * @value: pointer for storing read value
*
* Convenience wrapper to read a boolean sysfs file.
- *
+ *
* Returns:
- * The value read.
+ * True if value successfully read, false otherwise.
*/
-bool igt_sysfs_get_boolean(int dir, const char *attr)
+bool __igt_sysfs_get_boolean(int dir, const char *attr, bool *value)
{
- int result;
char *buf;
+ int ret, read_value;
buf = igt_sysfs_get(dir, attr);
- if (igt_debug_on(!buf))
+ if (igt_debug_on_f(!buf, "Failed to read %s attribute (%s)\n", attr, strerror(errno)))
return false;
- if (sscanf(buf, "%d", &result) != 1) {
- /* kernel's param_get_bool() returns "Y"/"N" */
- result = !strcasecmp(buf, "Y");
+ ret = sscanf(buf, "%d", &read_value);
+ if (((ret == 1) && (read_value == 1)) || ((ret == 0) && !strcasecmp(buf, "Y"))) {
+ *value = true;
+ } else if (((ret == 1) && (read_value == 0)) || ((ret == 0) && !strcasecmp(buf, "N"))) {
+ *value = false;
+ } else {
+ igt_debug("Value read from %s attribute (%s) is not as expected (0|1|N|Y|n|y)\n",
+ attr, buf);
+ free(buf);
+ return false;
}
free(buf);
- return result;
+ return true;
}
/**
- * igt_sysfs_set_boolean:
- * @dir: directory for the device from igt_sysfs_open()
- * @attr: name of the sysfs node to open
+ * igt_sysfs_get_boolean:
+ * @dir: directory corresponding to attribute
+ * @attr: name of the sysfs node to read
+ *
+ * Convenience wrapper to read a boolean sysfs file.
+ * It asserts on failure.
+ *
+ * Returns:
+ * Read value.
+ */
+bool igt_sysfs_get_boolean(int dir, const char *attr)
+{
+ bool value;
+
+ igt_assert(__igt_sysfs_get_boolean(dir, attr, &value));
+
+ return value;
+}
+
+/**
+ * __igt_sysfs_set_boolean:
+ * @dir: directory corresponding to attribute
+ * @attr: name of the sysfs node to write
* @value: value to set
*
* Convenience wrapper to write a boolean sysfs file.
- *
+ *
* Returns:
- * The value read.
+ * True if successfully written, false otherwise.
*/
-bool igt_sysfs_set_boolean(int dir, const char *attr, bool value)
+bool __igt_sysfs_set_boolean(int dir, const char *attr, bool value)
{
return igt_sysfs_printf(dir, attr, "%d", value) == 1;
}
+/**
+ * igt_sysfs_set_boolean:
+ * @dir: directory corresponding to attribute
+ * @attr: name of the sysfs node to write
+ * @value: value to set
+ *
+ * Convenience wrapper to write a boolean sysfs file.
+ * It asserts on failure.
+ */
+void igt_sysfs_set_boolean(int dir, const char *attr, bool value)
+{
+ igt_assert_f(__igt_sysfs_set_boolean(dir, attr, value),
+ "Failed to write %u to %s attribute (%s)\n", value, attr, strerror(errno));
+}
+
static void bind_con(const char *name, bool enable)
{
const char *path = "/sys/class/vtconsole";
@@ -845,14 +959,14 @@ static bool rw_attr_equal_within_epsilon(uint64_t x, uint64_t ref, double tol)
/* Sweep the range of values for an attribute to identify matching reads/writes */
static int rw_attr_sweep(igt_sysfs_rw_attr_t *rw)
{
- uint64_t get, set = rw->start;
+ uint64_t get = 0, set = rw->start;
int num_points = 0;
bool ret;
igt_debug("'%s': sweeping range of values\n", rw->attr);
while (set < UINT64_MAX / 2) {
- ret = igt_sysfs_set_u64(rw->dir, rw->attr, set);
- get = igt_sysfs_get_u64(rw->dir, rw->attr);
+ ret = __igt_sysfs_set_u64(rw->dir, rw->attr, set);
+ __igt_sysfs_get_u64(rw->dir, rw->attr, &get);
igt_debug("'%s': ret %d set %lu get %lu\n", rw->attr, ret, set, get);
if (ret && rw_attr_equal_within_epsilon(get, set, rw->tol)) {
igt_debug("'%s': matches\n", rw->attr);
@@ -888,7 +1002,7 @@ static int rw_attr_sweep(igt_sysfs_rw_attr_t *rw)
*/
void igt_sysfs_rw_attr_verify(igt_sysfs_rw_attr_t *rw)
{
- uint64_t prev, get;
+ uint64_t prev = 0, get = 0;
struct stat st;
int ret;
@@ -896,7 +1010,7 @@ void igt_sysfs_rw_attr_verify(igt_sysfs_rw_attr_t *rw)
igt_assert(st.st_mode & 0222); /* writable */
igt_assert(rw->start); /* cannot be 0 */
- prev = igt_sysfs_get_u64(rw->dir, rw->attr);
+ __igt_sysfs_get_u64(rw->dir, rw->attr, &prev);
igt_debug("'%s': prev %lu\n", rw->attr, prev);
ret = rw_attr_sweep(rw);
@@ -905,8 +1019,8 @@ void igt_sysfs_rw_attr_verify(igt_sysfs_rw_attr_t *rw)
* Restore previous value: we don't assert before this point so
* that we can restore the attr before asserting
*/
- igt_assert_eq(1, igt_sysfs_set_u64(rw->dir, rw->attr, prev));
- get = igt_sysfs_get_u64(rw->dir, rw->attr);
+ igt_sysfs_set_u64(rw->dir, rw->attr, prev);
+ __igt_sysfs_get_u64(rw->dir, rw->attr, &get);
igt_assert_eq(get, prev);
igt_assert(!ret);
}
diff --git a/lib/igt_sysfs.h b/lib/igt_sysfs.h
index d507fde8b..0087c03b7 100644
--- a/lib/igt_sysfs.h
+++ b/lib/igt_sysfs.h
@@ -119,14 +119,20 @@ int igt_sysfs_vprintf(int dir, const char *attr, const char *fmt, va_list ap)
int igt_sysfs_printf(int dir, const char *attr, const char *fmt, ...)
__attribute__((format(printf,3,4)));
+bool __igt_sysfs_get_u32(int dir, const char *attr, uint32_t *value);
uint32_t igt_sysfs_get_u32(int dir, const char *attr);
-bool igt_sysfs_set_u32(int dir, const char *attr, uint32_t value);
+bool __igt_sysfs_set_u32(int dir, const char *attr, uint32_t value);
+void igt_sysfs_set_u32(int dir, const char *attr, uint32_t value);
+bool __igt_sysfs_get_u64(int dir, const char *attr, uint64_t *value);
uint64_t igt_sysfs_get_u64(int dir, const char *attr);
-bool igt_sysfs_set_u64(int dir, const char *attr, uint64_t value);
+bool __igt_sysfs_set_u64(int dir, const char *attr, uint64_t value);
+void igt_sysfs_set_u64(int dir, const char *attr, uint64_t value);
+bool __igt_sysfs_get_boolean(int dir, const char *attr, bool *value);
bool igt_sysfs_get_boolean(int dir, const char *attr);
-bool igt_sysfs_set_boolean(int dir, const char *attr, bool value);
+bool __igt_sysfs_set_boolean(int dir, const char *attr, bool value);
+void igt_sysfs_set_boolean(int dir, const char *attr, bool value);
void bind_fbcon(bool enable);
void kick_snd_hda_intel(void);
diff --git a/tests/i915/i915_pm_dc.c b/tests/i915/i915_pm_dc.c
index 5069ddcc3..90fe847fc 100644
--- a/tests/i915/i915_pm_dc.c
+++ b/tests/i915/i915_pm_dc.c
@@ -533,8 +533,8 @@ static void setup_dc9_dpms(data_t *data, int dc_target)
int prev_dc = 0, prev_rpm, sysfs_fd;
igt_require((sysfs_fd = open(KMS_HELPER, O_RDONLY)) >= 0);
- kms_poll_saved_state = igt_sysfs_get_boolean(sysfs_fd, "poll");
- igt_sysfs_set_boolean(sysfs_fd, "poll", KMS_POLL_DISABLE);
+ __igt_sysfs_get_boolean(sysfs_fd, "poll", &kms_poll_saved_state);
+ __igt_sysfs_set_boolean(sysfs_fd, "poll", KMS_POLL_DISABLE);
close(sysfs_fd);
if (DC9_RESETS_DC_COUNTERS(data->devid)) {
prev_dc = read_dc_counter(data->debugfs_fd, dc_target);
@@ -629,7 +629,7 @@ static void kms_poll_state_restore(int sig)
sysfs_fd = open(KMS_HELPER, O_RDONLY);
if (sysfs_fd >= 0) {
- igt_sysfs_set_boolean(sysfs_fd, "poll", kms_poll_saved_state);
+ __igt_sysfs_set_boolean(sysfs_fd, "poll", kms_poll_saved_state);
close(sysfs_fd);
}
}
diff --git a/tests/i915/i915_pm_freq_mult.c b/tests/i915/i915_pm_freq_mult.c
index af62bbc2c..49f6722b1 100644
--- a/tests/i915/i915_pm_freq_mult.c
+++ b/tests/i915/i915_pm_freq_mult.c
@@ -50,25 +50,26 @@ static void spin_all(void)
static void restore_rps_defaults(int dir)
{
- int def, min, max;
+ int def;
+ uint32_t min = 0, max = 0;
/* Read from gt/gtN/.defaults/ write to gt/gtN/ */
def = openat(dir, ".defaults", O_RDONLY);
if (def < 0)
return;
- max = igt_sysfs_get_u32(def, "rps_max_freq_mhz");
- igt_sysfs_set_u32(dir, "rps_max_freq_mhz", max);
+ __igt_sysfs_get_u32(def, "rps_max_freq_mhz", &max);
+ __igt_sysfs_set_u32(dir, "rps_max_freq_mhz", max);
- min = igt_sysfs_get_u32(def, "rps_min_freq_mhz");
- igt_sysfs_set_u32(dir, "rps_min_freq_mhz", min);
+ __igt_sysfs_get_u32(def, "rps_min_freq_mhz", &min);
+ __igt_sysfs_set_u32(dir, "rps_min_freq_mhz", min);
close(def);
}
static void setup_freq(int gt, int dir)
{
- int rp0, rp1, rpn, min, max, act, media;
+ uint32_t rp0 = 0, rp1 = 0, rpn = 0, min = 0, max = 0, act = 0, media = 0;
ctx = intel_ctx_create_all_physical(i915);
ahnd = get_reloc_ahnd(i915, ctx->id);
@@ -81,17 +82,18 @@ static void setup_freq(int gt, int dir)
wait_freq_set();
/* Print some debug information */
- rp0 = igt_sysfs_get_u32(dir, "rps_RP0_freq_mhz");
- rp1 = igt_sysfs_get_u32(dir, "rps_RP1_freq_mhz");
- rpn = igt_sysfs_get_u32(dir, "rps_RPn_freq_mhz");
- min = igt_sysfs_get_u32(dir, "rps_min_freq_mhz");
- max = igt_sysfs_get_u32(dir, "rps_max_freq_mhz");
- act = igt_sysfs_get_u32(dir, "rps_act_freq_mhz");
+ rp0 = __igt_sysfs_get_u32(dir, "rps_RP0_freq_mhz", &rp0);
+ rp1 = __igt_sysfs_get_u32(dir, "rps_RP1_freq_mhz", &rp1);
+ rpn = __igt_sysfs_get_u32(dir, "rps_RPn_freq_mhz", &rpn);
+ min = __igt_sysfs_get_u32(dir, "rps_min_freq_mhz", &min);
+ max = __igt_sysfs_get_u32(dir, "rps_max_freq_mhz", &max);
+ act = __igt_sysfs_get_u32(dir, "rps_act_freq_mhz", &act);
- igt_debug("RP0 MHz: %d, RP1 MHz: %d, RPn MHz: %d, min MHz: %d, max MHz: %d, act MHz: %d\n", rp0, rp1, rpn, min, max, act);
+ igt_debug("RP0 MHz: %u, RP1 MHz: %u, RPn MHz: %u, min MHz: %u, max MHz: %u, act MHz: %u\n",
+ rp0, rp1, rpn, min, max, act);
if (igt_sysfs_has_attr(dir, "media_freq_factor")) {
- media = igt_sysfs_get_u32(dir, "media_freq_factor");
+ __igt_sysfs_get_u32(dir, "media_freq_factor", &media);
igt_debug("media ratio: %.2f\n", media * FREQ_SCALE_FACTOR);
}
}
@@ -108,6 +110,7 @@ static void cleanup(int dir)
static void media_freq(int gt, int dir)
{
float scale;
+ uint32_t rp0 = 0, rpn = 0;
igt_require(igt_sysfs_has_attr(dir, "media_freq_factor"));
@@ -116,9 +119,9 @@ static void media_freq(int gt, int dir)
setup_freq(gt, dir);
- igt_debug("media RP0 mhz: %d, media RPn mhz: %d\n",
- igt_sysfs_get_u32(dir, "media_RP0_freq_mhz"),
- igt_sysfs_get_u32(dir, "media_RPn_freq_mhz"));
+ __igt_sysfs_get_u32(dir, "media_RP0_freq_mhz", &rp0);
+ __igt_sysfs_get_u32(dir, "media_RPn_freq_mhz", &rpn);
+ igt_debug("media RP0 MHz: %u, media RPn MHz: %u\n", rp0, rpn);
igt_debug("media ratio value 0.0 represents dynamic mode\n");
/*
@@ -127,7 +130,8 @@ static void media_freq(int gt, int dir)
* modes. Fixed ratio modes should return the same value.
*/
for (int v = 256; v >= 0; v -= 64) {
- int getv, ret;
+ int ret;
+ uint32_t getv = 0;
/*
* Check that we can set the mode. Ratios other than 1:2
@@ -141,7 +145,7 @@ static void media_freq(int gt, int dir)
wait_freq_set();
- getv = igt_sysfs_get_u32(dir, "media_freq_factor");
+ __igt_sysfs_get_u32(dir, "media_freq_factor", &getv);
igt_debug("media ratio set: %.2f, media ratio get: %.2f\n",
v * scale, getv * scale);
diff --git a/tests/i915/i915_pm_rps.c b/tests/i915/i915_pm_rps.c
index 7044fcd81..d9d43e568 100644
--- a/tests/i915/i915_pm_rps.c
+++ b/tests/i915/i915_pm_rps.c
@@ -720,7 +720,7 @@ static void fence_order(int i915)
.buffer_count = ARRAY_SIZE(obj),
};
uint64_t wr, rw;
- int min, max;
+ uint32_t min = 0, max = 0;
double freq;
int sysfs;
@@ -742,14 +742,14 @@ static void fence_order(int i915)
*/
sysfs = igt_sysfs_open(i915);
- min = igt_sysfs_get_u32(sysfs, "gt_RPn_freq_mhz");
- max = igt_sysfs_get_u32(sysfs, "gt_RP0_freq_mhz");
+ __igt_sysfs_get_u32(sysfs, "gt_RPn_freq_mhz", &min);
+ __igt_sysfs_get_u32(sysfs, "gt_RP0_freq_mhz", &max);
igt_require(max > min);
/* Only allow ourselves to upclock via waitboosting */
- igt_sysfs_printf(sysfs, "gt_min_freq_mhz", "%d", min);
- igt_sysfs_printf(sysfs, "gt_max_freq_mhz", "%d", min);
- igt_sysfs_printf(sysfs, "gt_boost_freq_mhz", "%d", max);
+ igt_sysfs_printf(sysfs, "gt_min_freq_mhz", "%u", min);
+ igt_sysfs_printf(sysfs, "gt_max_freq_mhz", "%u", min);
+ igt_sysfs_printf(sysfs, "gt_boost_freq_mhz", "%u", max);
/* Warm up to bind the vma */
__fence_order(i915, &obj[0], &execbuf, 0, 0, &freq);
@@ -763,8 +763,8 @@ static void fence_order(int i915)
gem_close(i915, obj[0].handle);
gem_close(i915, obj[1].handle);
- igt_sysfs_printf(sysfs, "gt_min_freq_mhz", "%d", min);
- igt_sysfs_printf(sysfs, "gt_max_freq_mhz", "%d", max);
+ igt_sysfs_printf(sysfs, "gt_min_freq_mhz", "%u", min);
+ igt_sysfs_printf(sysfs, "gt_max_freq_mhz", "%u", max);
close(sysfs);
@@ -830,7 +830,7 @@ static void engine_order(int i915)
uint64_t forward, backward, both;
unsigned int num_engines;
const intel_ctx_t *ctx;
- int min, max;
+ uint32_t min = 0, max = 0;
double freq;
int sysfs;
@@ -862,14 +862,14 @@ static void engine_order(int i915)
execbuf.rsvd1 = ctx->id;
sysfs = igt_sysfs_open(i915);
- min = igt_sysfs_get_u32(sysfs, "gt_RPn_freq_mhz");
- max = igt_sysfs_get_u32(sysfs, "gt_RP0_freq_mhz");
+ __igt_sysfs_get_u32(sysfs, "gt_RPn_freq_mhz", &min);
+ __igt_sysfs_get_u32(sysfs, "gt_RP0_freq_mhz", &max);
igt_require(max > min);
/* Only allow ourselves to upclock via waitboosting */
- igt_sysfs_printf(sysfs, "gt_min_freq_mhz", "%d", min);
- igt_sysfs_printf(sysfs, "gt_max_freq_mhz", "%d", min);
- igt_sysfs_printf(sysfs, "gt_boost_freq_mhz", "%d", max);
+ igt_sysfs_printf(sysfs, "gt_min_freq_mhz", "%u", min);
+ igt_sysfs_printf(sysfs, "gt_max_freq_mhz", "%u", min);
+ igt_sysfs_printf(sysfs, "gt_boost_freq_mhz", "%u", max);
/* Warm up to bind the vma */
gem_execbuf(i915, &execbuf);
@@ -893,8 +893,8 @@ static void engine_order(int i915)
gem_close(i915, obj[1].handle);
intel_ctx_destroy(i915, ctx);
- igt_sysfs_printf(sysfs, "gt_min_freq_mhz", "%d", min);
- igt_sysfs_printf(sysfs, "gt_max_freq_mhz", "%d", max);
+ igt_sysfs_printf(sysfs, "gt_min_freq_mhz", "%u", min);
+ igt_sysfs_printf(sysfs, "gt_max_freq_mhz", "%u", max);
close(sysfs);
diff --git a/tests/i915/i915_power.c b/tests/i915/i915_power.c
index ed7bef495..ee00cbcd8 100644
--- a/tests/i915/i915_power.c
+++ b/tests/i915/i915_power.c
@@ -38,7 +38,8 @@ static void sanity(int i915)
double idle, busy;
igt_spin_t *spin;
uint64_t ahnd;
- int dir, gt, req, act;
+ int dir, gt;
+ uint32_t req = 0, act = 0;
#define DURATION_SEC 2
@@ -58,9 +59,9 @@ static void sanity(int i915)
igt_spin_busywait_until_started(spin);
busy = measure_power(&pwr, DURATION_SEC);
i915_for_each_gt(i915, dir, gt) {
- req = igt_sysfs_get_u32(dir, "rps_cur_freq_mhz");
- act = igt_sysfs_get_u32(dir, "rps_act_freq_mhz");
- igt_info("gt %d: req MHz: %d, act MHz: %d\n", gt, req, act);
+ __igt_sysfs_get_u32(dir, "rps_cur_freq_mhz", &req);
+ __igt_sysfs_get_u32(dir, "rps_act_freq_mhz", &act);
+ igt_info("gt %d: req MHz: %u, act MHz: %u\n", gt, req, act);
}
igt_free_spins(i915);
put_ahnd(ahnd);
diff --git a/tests/i915/perf_pmu.c b/tests/i915/perf_pmu.c
index 0806a8e54..441ec6f57 100644
--- a/tests/i915/perf_pmu.c
+++ b/tests/i915/perf_pmu.c
@@ -1782,7 +1782,7 @@ static igt_spin_t *spin_sync_gt(int i915, uint64_t ahnd, unsigned int gt,
static void
test_frequency(int gem_fd, unsigned int gt)
{
- uint32_t min_freq, max_freq, boost_freq;
+ uint32_t min_freq = 0, max_freq = 0, boost_freq = 0, read_value = 0;
uint64_t val[2], start[2], slept;
double min[2], max[2];
igt_spin_t *spin;
@@ -1793,9 +1793,9 @@ test_frequency(int gem_fd, unsigned int gt)
sysfs = igt_sysfs_gt_open(gem_fd, gt);
igt_require(sysfs >= 0);
- min_freq = igt_sysfs_get_u32(sysfs, "rps_RPn_freq_mhz");
- max_freq = igt_sysfs_get_u32(sysfs, "rps_RP0_freq_mhz");
- boost_freq = igt_sysfs_get_u32(sysfs, "rps_boost_freq_mhz");
+ __igt_sysfs_get_u32(sysfs, "rps_RPn_freq_mhz", &min_freq);
+ __igt_sysfs_get_u32(sysfs, "rps_RP0_freq_mhz", &max_freq);
+ __igt_sysfs_get_u32(sysfs, "rps_boost_freq_mhz", &boost_freq);
igt_info("Frequency: min=%u, max=%u, boost=%u MHz\n",
min_freq, max_freq, boost_freq);
igt_require(min_freq > 0 && max_freq > 0 && boost_freq > 0);
@@ -1808,12 +1808,15 @@ test_frequency(int gem_fd, unsigned int gt)
/*
* Set GPU to min frequency and read PMU counters.
*/
- igt_require(igt_sysfs_set_u32(sysfs, "rps_min_freq_mhz", min_freq));
- igt_require(igt_sysfs_get_u32(sysfs, "rps_min_freq_mhz") == min_freq);
- igt_require(igt_sysfs_set_u32(sysfs, "rps_max_freq_mhz", min_freq));
- igt_require(igt_sysfs_get_u32(sysfs, "rps_max_freq_mhz") == min_freq);
- igt_require(igt_sysfs_set_u32(sysfs, "rps_boost_freq_mhz", min_freq));
- igt_require(igt_sysfs_get_u32(sysfs, "rps_boost_freq_mhz") == min_freq);
+ igt_require(__igt_sysfs_set_u32(sysfs, "rps_min_freq_mhz", min_freq));
+ igt_require(__igt_sysfs_get_u32(sysfs, "rps_min_freq_mhz", &read_value));
+ igt_require(read_value == min_freq);
+ igt_require(__igt_sysfs_set_u32(sysfs, "rps_max_freq_mhz", min_freq));
+ igt_require(__igt_sysfs_get_u32(sysfs, "rps_max_freq_mhz", &read_value));
+ igt_require(read_value == min_freq);
+ igt_require(__igt_sysfs_set_u32(sysfs, "rps_boost_freq_mhz", min_freq));
+ igt_require(__igt_sysfs_get_u32(sysfs, "rps_boost_freq_mhz", &read_value));
+ igt_require(read_value == min_freq);
gem_quiescent_gpu(gem_fd); /* Idle to be sure the change takes effect */
spin = spin_sync_gt(gem_fd, ahnd, gt, &ctx);
@@ -1834,13 +1837,16 @@ test_frequency(int gem_fd, unsigned int gt)
/*
* Set GPU to max frequency and read PMU counters.
*/
- igt_require(igt_sysfs_set_u32(sysfs, "rps_max_freq_mhz", max_freq));
- igt_require(igt_sysfs_get_u32(sysfs, "rps_max_freq_mhz") == max_freq);
- igt_require(igt_sysfs_set_u32(sysfs, "rps_boost_freq_mhz", boost_freq));
- igt_require(igt_sysfs_get_u32(sysfs, "rps_boost_freq_mhz") == boost_freq);
+ igt_require(__igt_sysfs_set_u32(sysfs, "rps_max_freq_mhz", max_freq));
+ igt_require(__igt_sysfs_get_u32(sysfs, "rps_max_freq_mhz", &read_value));
+ igt_require(read_value == max_freq);
+ igt_require(__igt_sysfs_set_u32(sysfs, "rps_boost_freq_mhz", boost_freq));
+ igt_require(__igt_sysfs_get_u32(sysfs, "rps_boost_freq_mhz", &read_value));
+ igt_require(read_value == boost_freq);
- igt_require(igt_sysfs_set_u32(sysfs, "rps_min_freq_mhz", max_freq));
- igt_require(igt_sysfs_get_u32(sysfs, "rps_min_freq_mhz") == max_freq);
+ igt_require(__igt_sysfs_set_u32(sysfs, "rps_min_freq_mhz", max_freq));
+ igt_require(__igt_sysfs_get_u32(sysfs, "rps_min_freq_mhz", &read_value));
+ igt_require(read_value == max_freq);
gem_quiescent_gpu(gem_fd);
spin = spin_sync_gt(gem_fd, ahnd, gt, &ctx);
@@ -1859,10 +1865,11 @@ test_frequency(int gem_fd, unsigned int gt)
/*
* Restore min/max.
*/
- igt_sysfs_set_u32(sysfs, "rps_min_freq_mhz", min_freq);
- if (igt_sysfs_get_u32(sysfs, "rps_min_freq_mhz") != min_freq)
+ __igt_sysfs_set_u32(sysfs, "rps_min_freq_mhz", min_freq);
+ __igt_sysfs_get_u32(sysfs, "rps_min_freq_mhz", &read_value);
+ if (read_value != min_freq)
igt_warn("Unable to restore min frequency to saved value [%u MHz], now %u MHz\n",
- min_freq, igt_sysfs_get_u32(sysfs, "rps_min_freq_mhz"));
+ min_freq, read_value);
close(fd[0]);
close(fd[1]);
put_ahnd(ahnd);
diff --git a/tests/kms_prime.c b/tests/kms_prime.c
index 52f587961..dedbf6233 100644
--- a/tests/kms_prime.c
+++ b/tests/kms_prime.c
@@ -341,7 +341,7 @@ static void kms_poll_state_restore(void)
int sysfs_fd;
igt_assert((sysfs_fd = open(KMS_HELPER, O_RDONLY)) >= 0);
- igt_sysfs_set_boolean(sysfs_fd, "poll", kms_poll_saved_state);
+ __igt_sysfs_set_boolean(sysfs_fd, "poll", kms_poll_saved_state);
close(sysfs_fd);
}
diff --git a/tests/nouveau_crc.c b/tests/nouveau_crc.c
index d5aa0e650..c94972318 100644
--- a/tests/nouveau_crc.c
+++ b/tests/nouveau_crc.c
@@ -264,15 +264,18 @@ static void test_ctx_flip_threshold_reset_after_capture(data_t *data)
{
igt_pipe_crc_t *pipe_crc;
const int fd = data->drm_fd;
+ uint32_t value = 0;
pipe_crc = igt_pipe_crc_new(fd, data->pipe, IGT_PIPE_CRC_SOURCE_AUTO);
set_crc_flip_threshold(data, 5);
igt_pipe_crc_start(pipe_crc);
- igt_assert_eq(igt_sysfs_get_u32(data->nv_crc_dir, "flip_threshold"), 5);
+ __igt_sysfs_get_u32(data->nv_crc_dir, "flip_threshold", &value);
+ igt_assert_eq(value, 5);
igt_pipe_crc_stop(pipe_crc);
- igt_assert_neq(igt_sysfs_get_u32(data->nv_crc_dir, "flip_threshold"), 5);
+ __igt_sysfs_get_u32(data->nv_crc_dir, "flip_threshold", &value);
+ igt_assert_neq(value, 5);
igt_pipe_crc_free(pipe_crc);
}
--
2.40.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [igt-dev] [PATCH i-g-t] lib/igt_sysfs: add asserting helpers for read/write operations
2023-07-12 11:45 [igt-dev] [PATCH i-g-t] lib/igt_sysfs: add asserting helpers for read/write operations Lukasz Laguna
@ 2023-07-12 12:32 ` Kamil Konieczny
0 siblings, 0 replies; 7+ messages in thread
From: Kamil Konieczny @ 2023-07-12 12:32 UTC (permalink / raw)
To: igt-dev, kamil.konieczny
Hi Lukasz,
On 2023-07-12 at 13:45:43 +0200, Lukasz Laguna wrote:
> Prefix names of existing, non-asserting helpers with "__":
>
> - igt_sysfs_get_u32 -> __igt_sysfs_get_u32
> - igt_sysfs_set_u32 -> __igt_sysfs_set_u32
> - igt_sysfs_get_u64 -> __igt_sysfs_get_u64
> - igt_sysfs_set_u64 -> __igt_sysfs_set_u64
> - igt_sysfs_get_boolean -> __igt_sysfs_get_boolean
> - igt_sysfs_set_boolean -> __igt_sysfs_set_boolean
>
> Replace calls to don't introduce any functional changes in the
> existing code.
>
> Additionally, reimplement non-asserting get helpers to return boolean
> result of the read operation and store the read value via pointer
> passed as function parameter. In previous implementation, it wasn't
> possible to distinguish if returned zero was a read value or failure on
> a read attempt.
>
> On the occasion, fix a typo in modified debug message and fix a bug in
----------------------------------------------------------- ^^^^^^^^^
Please send it in separate (first in series) patch.
Regards,
Kamil
> igt_aux_enable_pm_suspend_dbg() function, because of which tests started
> asserting after introduced changes.
>
> Signed-off-by: Lukasz Laguna <lukasz.laguna@intel.com>
> ---
> lib/i915/gem_submission.c | 4 +-
> lib/igt_aux.c | 4 +-
> lib/igt_pm.c | 2 +-
> lib/igt_sysfs.c | 220 +++++++++++++++++++++++++--------
> lib/igt_sysfs.h | 12 +-
> tests/i915/i915_pm_dc.c | 6 +-
> tests/i915/i915_pm_freq_mult.c | 42 ++++---
> tests/i915/i915_pm_rps.c | 32 ++---
> tests/i915/i915_power.c | 9 +-
> tests/i915/perf_pmu.c | 45 ++++---
> tests/kms_prime.c | 2 +-
> tests/nouveau_crc.c | 7 +-
> 12 files changed, 261 insertions(+), 124 deletions(-)
>
> diff --git a/lib/i915/gem_submission.c b/lib/i915/gem_submission.c
> index adf5eb394..7d1c3970f 100644
> --- a/lib/i915/gem_submission.c
> +++ b/lib/i915/gem_submission.c
> @@ -65,12 +65,14 @@ unsigned gem_submission_method(int fd)
> const int gen = intel_gen(intel_get_drm_devid(fd));
> unsigned method = GEM_SUBMISSION_RINGBUF;
> int dir;
> + uint32_t value = 0;
>
> dir = igt_params_open(fd);
> if (dir < 0)
> return 0;
>
> - if (igt_sysfs_get_u32(dir, "enable_guc") & 1) {
> + __igt_sysfs_get_u32(dir, "enable_guc", &value);
> + if (value & 1) {
> method = GEM_SUBMISSION_GUC;
> } else if (gen >= 8) {
> method = GEM_SUBMISSION_EXECLISTS;
> diff --git a/lib/igt_aux.c b/lib/igt_aux.c
> index fd5103043..1821ace67 100644
> --- a/lib/igt_aux.c
> +++ b/lib/igt_aux.c
> @@ -953,7 +953,7 @@ static void igt_aux_enable_pm_suspend_dbg(int power_dir)
> if (sysfs_fd > 0) {
> __console_suspend_saved_state = igt_sysfs_get_boolean(sysfs_fd, "console_suspend");
>
> - if (!igt_sysfs_set_boolean(sysfs_fd, "console_suspend", CONSOLE_SUSPEND_DISABLE))
> + if (!__igt_sysfs_set_boolean(sysfs_fd, "console_suspend", CONSOLE_SUSPEND_DISABLE))
> igt_warn("Unable to disable console suspend\n");
>
> } else {
> @@ -962,7 +962,7 @@ static void igt_aux_enable_pm_suspend_dbg(int power_dir)
>
> /* pm_debug_messages depends on CONFIG_PM_SLEEP_DEBUG */
> if (!faccessat(power_dir, "pm_debug_messages", R_OK | W_OK, 0)) {
> - __pm_debug_messages_state = igt_sysfs_get_boolean(sysfs_fd, "pm_debug_messages");
> + __pm_debug_messages_state = igt_sysfs_get_boolean(power_dir, "pm_debug_messages");
> igt_sysfs_set_boolean(power_dir, "pm_debug_messages", true);
> }
>
> diff --git a/lib/igt_pm.c b/lib/igt_pm.c
> index 18c84bf3a..ba591f0f8 100644
> --- a/lib/igt_pm.c
> +++ b/lib/igt_pm.c
> @@ -1415,5 +1415,5 @@ void igt_pm_ignore_slpc_efficient_freq(int i915, int gtfd, bool val)
> return;
>
> igt_require(igt_sysfs_has_attr(gtfd, "slpc_ignore_eff_freq"));
> - igt_assert(igt_sysfs_set_u32(gtfd, "slpc_ignore_eff_freq", val));
> + igt_sysfs_set_u32(gtfd, "slpc_ignore_eff_freq", val);
> }
> diff --git a/lib/igt_sysfs.c b/lib/igt_sysfs.c
> index 412edfc29..83182020b 100644
> --- a/lib/igt_sysfs.c
> +++ b/lib/igt_sysfs.c
> @@ -565,122 +565,236 @@ int igt_sysfs_printf(int dir, const char *attr, const char *fmt, ...)
> return ret;
> }
>
> +/**
> + * __igt_sysfs_get_u32:
> + * @dir: directory corresponding to attribute
> + * @attr: name of the sysfs node to read
> + * @value: pointer for storing read value
> + *
> + * Convenience wrapper to read a unsigned 32bit integer from a sysfs file.
> + *
> + * Returns:
> + * True if value successfully read, false otherwise.
> + */
> +bool __igt_sysfs_get_u32(int dir, const char *attr, uint32_t *value)
> +{
> + if (igt_debug_on(igt_sysfs_scanf(dir, attr, "%u", value) != 1))
> + return false;
> +
> + return true;
> +}
> +
> /**
> * igt_sysfs_get_u32:
> - * @dir: directory for the device from igt_sysfs_open()
> - * @attr: name of the sysfs node to open
> + * @dir: directory corresponding to attribute
> + * @attr: name of the sysfs node to read
> *
> * Convenience wrapper to read a unsigned 32bit integer from a sysfs file.
> + * It asserts on failure.
> *
> * Returns:
> - * The value read.
> + * Read value.
> */
> uint32_t igt_sysfs_get_u32(int dir, const char *attr)
> {
> - uint32_t result;
> + uint32_t value;
> +
> + igt_assert_f(__igt_sysfs_get_u32(dir, attr, &value),
> + "Failed to read %s attribute (%s)\n", attr, strerror(errno));
> +
> + return value;
> +}
> +
> +/**
> + * __igt_sysfs_set_u32:
> + * @dir: directory corresponding to attribute
> + * @attr: name of the sysfs node to write
> + * @value: value to set
> + *
> + * Convenience wrapper to write a unsigned 32bit integer to a sysfs file.
> + *
> + * Returns:
> + * True if successfully written, false otherwise.
> + */
> +bool __igt_sysfs_set_u32(int dir, const char *attr, uint32_t value)
> +{
> + return igt_sysfs_printf(dir, attr, "%u", value) > 0;
> +}
> +
> +/**
> + * igt_sysfs_set_u32:
> + * @dir: directory corresponding to attribute
> + * @attr: name of the sysfs node to write
> + * @value: value to set
> + *
> + * Convenience wrapper to write a unsigned 32bit integer to a sysfs file.
> + * It asserts on failure.
> + */
> +void igt_sysfs_set_u32(int dir, const char *attr, uint32_t value)
> +{
> + igt_assert_f(__igt_sysfs_set_u32(dir, attr, value),
> + "Failed to write %u to %s attribute (%s)\n", value, attr, strerror(errno));
> +}
>
> - if (igt_debug_on(igt_sysfs_scanf(dir, attr, "%u", &result) != 1))
> - return 0;
> +/**
> + * __igt_sysfs_get_u64:
> + * @dir: directory corresponding to attribute
> + * @attr: name of the sysfs node to read
> + * @value: pointer for storing read value
> + *
> + * Convenience wrapper to read a unsigned 64bit integer from a sysfs file.
> + *
> + * Returns:
> + * True if value successfully read, false otherwise.
> + */
> +bool __igt_sysfs_get_u64(int dir, const char *attr, uint64_t *value)
> +{
> + if (igt_debug_on(igt_sysfs_scanf(dir, attr, "%"PRIu64, value) != 1))
> + return false;
>
> - return result;
> + return true;
> }
>
> /**
> * igt_sysfs_get_u64:
> - * @dir: directory for the device from igt_sysfs_open()
> - * @attr: name of the sysfs node to open
> + * @dir: directory corresponding to attribute
> + * @attr: name of the sysfs node to read
> *
> * Convenience wrapper to read a unsigned 64bit integer from a sysfs file.
> + * It asserts on failure.
> *
> * Returns:
> - * The value read.
> + * Read value.
> */
> uint64_t igt_sysfs_get_u64(int dir, const char *attr)
> {
> - uint64_t result;
> + uint64_t value;
>
> - if (igt_debug_on(igt_sysfs_scanf(dir, attr, "%"PRIu64, &result) != 1))
> - return 0;
> + igt_assert_f(__igt_sysfs_get_u64(dir, attr, &value),
> + "Failed to read %s attribute (%s)\n", attr, strerror(errno));
>
> - return result;
> + return value;
> }
>
> /**
> - * igt_sysfs_set_u64:
> - * @dir: directory for the device from igt_sysfs_open()
> - * @attr: name of the sysfs node to open
> + * __igt_sysfs_set_u64:
> + * @dir: directory corresponding to attribute
> + * @attr: name of the sysfs node to write
> * @value: value to set
> *
> * Convenience wrapper to write a unsigned 64bit integer to a sysfs file.
> *
> * Returns:
> - * True if successfully written
> + * True if successfully written, false otherwise.
> */
> -bool igt_sysfs_set_u64(int dir, const char *attr, uint64_t value)
> +bool __igt_sysfs_set_u64(int dir, const char *attr, uint64_t value)
> {
> return igt_sysfs_printf(dir, attr, "%"PRIu64, value) > 0;
> }
>
> /**
> - * igt_sysfs_set_u32:
> - * @dir: directory for the device from igt_sysfs_open()
> - * @attr: name of the sysfs node to open
> + * igt_sysfs_set_u64:
> + * @dir: directory corresponding to attribute
> + * @attr: name of the sysfs node to write
> * @value: value to set
> *
> - * Convenience wrapper to write a unsigned 32bit integer to a sysfs file.
> - *
> - * Returns:
> - * True if successfully written
> + * Convenience wrapper to write a unsigned 64bit integer to a sysfs file.
> + * It asserts on failure.
> */
> -bool igt_sysfs_set_u32(int dir, const char *attr, uint32_t value)
> +void igt_sysfs_set_u64(int dir, const char *attr, uint64_t value)
> {
> - return igt_sysfs_printf(dir, attr, "%u", value) > 0;
> + igt_assert_f(__igt_sysfs_set_u64(dir, attr, value),
> + "Failed to write %"PRIu64" to %s attribute (%s)\n",
> + value, attr, strerror(errno));
> }
>
> /**
> - * igt_sysfs_get_boolean:
> - * @dir: directory for the device from igt_sysfs_open()
> - * @attr: name of the sysfs node to open
> + * __igt_sysfs_get_boolean:
> + * @dir: directory corresponding to attribute
> + * @attr: name of the sysfs node to read
> + * @value: pointer for storing read value
> *
> * Convenience wrapper to read a boolean sysfs file.
> - *
> + *
> * Returns:
> - * The value read.
> + * True if value successfully read, false otherwise.
> */
> -bool igt_sysfs_get_boolean(int dir, const char *attr)
> +bool __igt_sysfs_get_boolean(int dir, const char *attr, bool *value)
> {
> - int result;
> char *buf;
> + int ret, read_value;
>
> buf = igt_sysfs_get(dir, attr);
> - if (igt_debug_on(!buf))
> + if (igt_debug_on_f(!buf, "Failed to read %s attribute (%s)\n", attr, strerror(errno)))
> return false;
>
> - if (sscanf(buf, "%d", &result) != 1) {
> - /* kernel's param_get_bool() returns "Y"/"N" */
> - result = !strcasecmp(buf, "Y");
> + ret = sscanf(buf, "%d", &read_value);
> + if (((ret == 1) && (read_value == 1)) || ((ret == 0) && !strcasecmp(buf, "Y"))) {
> + *value = true;
> + } else if (((ret == 1) && (read_value == 0)) || ((ret == 0) && !strcasecmp(buf, "N"))) {
> + *value = false;
> + } else {
> + igt_debug("Value read from %s attribute (%s) is not as expected (0|1|N|Y|n|y)\n",
> + attr, buf);
> + free(buf);
> + return false;
> }
>
> free(buf);
> - return result;
> + return true;
> }
>
> /**
> - * igt_sysfs_set_boolean:
> - * @dir: directory for the device from igt_sysfs_open()
> - * @attr: name of the sysfs node to open
> + * igt_sysfs_get_boolean:
> + * @dir: directory corresponding to attribute
> + * @attr: name of the sysfs node to read
> + *
> + * Convenience wrapper to read a boolean sysfs file.
> + * It asserts on failure.
> + *
> + * Returns:
> + * Read value.
> + */
> +bool igt_sysfs_get_boolean(int dir, const char *attr)
> +{
> + bool value;
> +
> + igt_assert(__igt_sysfs_get_boolean(dir, attr, &value));
> +
> + return value;
> +}
> +
> +/**
> + * __igt_sysfs_set_boolean:
> + * @dir: directory corresponding to attribute
> + * @attr: name of the sysfs node to write
> * @value: value to set
> *
> * Convenience wrapper to write a boolean sysfs file.
> - *
> + *
> * Returns:
> - * The value read.
> + * True if successfully written, false otherwise.
> */
> -bool igt_sysfs_set_boolean(int dir, const char *attr, bool value)
> +bool __igt_sysfs_set_boolean(int dir, const char *attr, bool value)
> {
> return igt_sysfs_printf(dir, attr, "%d", value) == 1;
> }
>
> +/**
> + * igt_sysfs_set_boolean:
> + * @dir: directory corresponding to attribute
> + * @attr: name of the sysfs node to write
> + * @value: value to set
> + *
> + * Convenience wrapper to write a boolean sysfs file.
> + * It asserts on failure.
> + */
> +void igt_sysfs_set_boolean(int dir, const char *attr, bool value)
> +{
> + igt_assert_f(__igt_sysfs_set_boolean(dir, attr, value),
> + "Failed to write %u to %s attribute (%s)\n", value, attr, strerror(errno));
> +}
> +
> static void bind_con(const char *name, bool enable)
> {
> const char *path = "/sys/class/vtconsole";
> @@ -845,14 +959,14 @@ static bool rw_attr_equal_within_epsilon(uint64_t x, uint64_t ref, double tol)
> /* Sweep the range of values for an attribute to identify matching reads/writes */
> static int rw_attr_sweep(igt_sysfs_rw_attr_t *rw)
> {
> - uint64_t get, set = rw->start;
> + uint64_t get = 0, set = rw->start;
> int num_points = 0;
> bool ret;
>
> igt_debug("'%s': sweeping range of values\n", rw->attr);
> while (set < UINT64_MAX / 2) {
> - ret = igt_sysfs_set_u64(rw->dir, rw->attr, set);
> - get = igt_sysfs_get_u64(rw->dir, rw->attr);
> + ret = __igt_sysfs_set_u64(rw->dir, rw->attr, set);
> + __igt_sysfs_get_u64(rw->dir, rw->attr, &get);
> igt_debug("'%s': ret %d set %lu get %lu\n", rw->attr, ret, set, get);
> if (ret && rw_attr_equal_within_epsilon(get, set, rw->tol)) {
> igt_debug("'%s': matches\n", rw->attr);
> @@ -888,7 +1002,7 @@ static int rw_attr_sweep(igt_sysfs_rw_attr_t *rw)
> */
> void igt_sysfs_rw_attr_verify(igt_sysfs_rw_attr_t *rw)
> {
> - uint64_t prev, get;
> + uint64_t prev = 0, get = 0;
> struct stat st;
> int ret;
>
> @@ -896,7 +1010,7 @@ void igt_sysfs_rw_attr_verify(igt_sysfs_rw_attr_t *rw)
> igt_assert(st.st_mode & 0222); /* writable */
> igt_assert(rw->start); /* cannot be 0 */
>
> - prev = igt_sysfs_get_u64(rw->dir, rw->attr);
> + __igt_sysfs_get_u64(rw->dir, rw->attr, &prev);
> igt_debug("'%s': prev %lu\n", rw->attr, prev);
>
> ret = rw_attr_sweep(rw);
> @@ -905,8 +1019,8 @@ void igt_sysfs_rw_attr_verify(igt_sysfs_rw_attr_t *rw)
> * Restore previous value: we don't assert before this point so
> * that we can restore the attr before asserting
> */
> - igt_assert_eq(1, igt_sysfs_set_u64(rw->dir, rw->attr, prev));
> - get = igt_sysfs_get_u64(rw->dir, rw->attr);
> + igt_sysfs_set_u64(rw->dir, rw->attr, prev);
> + __igt_sysfs_get_u64(rw->dir, rw->attr, &get);
> igt_assert_eq(get, prev);
> igt_assert(!ret);
> }
> diff --git a/lib/igt_sysfs.h b/lib/igt_sysfs.h
> index d507fde8b..0087c03b7 100644
> --- a/lib/igt_sysfs.h
> +++ b/lib/igt_sysfs.h
> @@ -119,14 +119,20 @@ int igt_sysfs_vprintf(int dir, const char *attr, const char *fmt, va_list ap)
> int igt_sysfs_printf(int dir, const char *attr, const char *fmt, ...)
> __attribute__((format(printf,3,4)));
>
> +bool __igt_sysfs_get_u32(int dir, const char *attr, uint32_t *value);
> uint32_t igt_sysfs_get_u32(int dir, const char *attr);
> -bool igt_sysfs_set_u32(int dir, const char *attr, uint32_t value);
> +bool __igt_sysfs_set_u32(int dir, const char *attr, uint32_t value);
> +void igt_sysfs_set_u32(int dir, const char *attr, uint32_t value);
>
> +bool __igt_sysfs_get_u64(int dir, const char *attr, uint64_t *value);
> uint64_t igt_sysfs_get_u64(int dir, const char *attr);
> -bool igt_sysfs_set_u64(int dir, const char *attr, uint64_t value);
> +bool __igt_sysfs_set_u64(int dir, const char *attr, uint64_t value);
> +void igt_sysfs_set_u64(int dir, const char *attr, uint64_t value);
>
> +bool __igt_sysfs_get_boolean(int dir, const char *attr, bool *value);
> bool igt_sysfs_get_boolean(int dir, const char *attr);
> -bool igt_sysfs_set_boolean(int dir, const char *attr, bool value);
> +bool __igt_sysfs_set_boolean(int dir, const char *attr, bool value);
> +void igt_sysfs_set_boolean(int dir, const char *attr, bool value);
>
> void bind_fbcon(bool enable);
> void kick_snd_hda_intel(void);
> diff --git a/tests/i915/i915_pm_dc.c b/tests/i915/i915_pm_dc.c
> index 5069ddcc3..90fe847fc 100644
> --- a/tests/i915/i915_pm_dc.c
> +++ b/tests/i915/i915_pm_dc.c
> @@ -533,8 +533,8 @@ static void setup_dc9_dpms(data_t *data, int dc_target)
> int prev_dc = 0, prev_rpm, sysfs_fd;
>
> igt_require((sysfs_fd = open(KMS_HELPER, O_RDONLY)) >= 0);
> - kms_poll_saved_state = igt_sysfs_get_boolean(sysfs_fd, "poll");
> - igt_sysfs_set_boolean(sysfs_fd, "poll", KMS_POLL_DISABLE);
> + __igt_sysfs_get_boolean(sysfs_fd, "poll", &kms_poll_saved_state);
> + __igt_sysfs_set_boolean(sysfs_fd, "poll", KMS_POLL_DISABLE);
> close(sysfs_fd);
> if (DC9_RESETS_DC_COUNTERS(data->devid)) {
> prev_dc = read_dc_counter(data->debugfs_fd, dc_target);
> @@ -629,7 +629,7 @@ static void kms_poll_state_restore(int sig)
>
> sysfs_fd = open(KMS_HELPER, O_RDONLY);
> if (sysfs_fd >= 0) {
> - igt_sysfs_set_boolean(sysfs_fd, "poll", kms_poll_saved_state);
> + __igt_sysfs_set_boolean(sysfs_fd, "poll", kms_poll_saved_state);
> close(sysfs_fd);
> }
> }
> diff --git a/tests/i915/i915_pm_freq_mult.c b/tests/i915/i915_pm_freq_mult.c
> index af62bbc2c..49f6722b1 100644
> --- a/tests/i915/i915_pm_freq_mult.c
> +++ b/tests/i915/i915_pm_freq_mult.c
> @@ -50,25 +50,26 @@ static void spin_all(void)
>
> static void restore_rps_defaults(int dir)
> {
> - int def, min, max;
> + int def;
> + uint32_t min = 0, max = 0;
>
> /* Read from gt/gtN/.defaults/ write to gt/gtN/ */
> def = openat(dir, ".defaults", O_RDONLY);
> if (def < 0)
> return;
>
> - max = igt_sysfs_get_u32(def, "rps_max_freq_mhz");
> - igt_sysfs_set_u32(dir, "rps_max_freq_mhz", max);
> + __igt_sysfs_get_u32(def, "rps_max_freq_mhz", &max);
> + __igt_sysfs_set_u32(dir, "rps_max_freq_mhz", max);
>
> - min = igt_sysfs_get_u32(def, "rps_min_freq_mhz");
> - igt_sysfs_set_u32(dir, "rps_min_freq_mhz", min);
> + __igt_sysfs_get_u32(def, "rps_min_freq_mhz", &min);
> + __igt_sysfs_set_u32(dir, "rps_min_freq_mhz", min);
>
> close(def);
> }
>
> static void setup_freq(int gt, int dir)
> {
> - int rp0, rp1, rpn, min, max, act, media;
> + uint32_t rp0 = 0, rp1 = 0, rpn = 0, min = 0, max = 0, act = 0, media = 0;
>
> ctx = intel_ctx_create_all_physical(i915);
> ahnd = get_reloc_ahnd(i915, ctx->id);
> @@ -81,17 +82,18 @@ static void setup_freq(int gt, int dir)
> wait_freq_set();
>
> /* Print some debug information */
> - rp0 = igt_sysfs_get_u32(dir, "rps_RP0_freq_mhz");
> - rp1 = igt_sysfs_get_u32(dir, "rps_RP1_freq_mhz");
> - rpn = igt_sysfs_get_u32(dir, "rps_RPn_freq_mhz");
> - min = igt_sysfs_get_u32(dir, "rps_min_freq_mhz");
> - max = igt_sysfs_get_u32(dir, "rps_max_freq_mhz");
> - act = igt_sysfs_get_u32(dir, "rps_act_freq_mhz");
> + rp0 = __igt_sysfs_get_u32(dir, "rps_RP0_freq_mhz", &rp0);
> + rp1 = __igt_sysfs_get_u32(dir, "rps_RP1_freq_mhz", &rp1);
> + rpn = __igt_sysfs_get_u32(dir, "rps_RPn_freq_mhz", &rpn);
> + min = __igt_sysfs_get_u32(dir, "rps_min_freq_mhz", &min);
> + max = __igt_sysfs_get_u32(dir, "rps_max_freq_mhz", &max);
> + act = __igt_sysfs_get_u32(dir, "rps_act_freq_mhz", &act);
>
> - igt_debug("RP0 MHz: %d, RP1 MHz: %d, RPn MHz: %d, min MHz: %d, max MHz: %d, act MHz: %d\n", rp0, rp1, rpn, min, max, act);
> + igt_debug("RP0 MHz: %u, RP1 MHz: %u, RPn MHz: %u, min MHz: %u, max MHz: %u, act MHz: %u\n",
> + rp0, rp1, rpn, min, max, act);
>
> if (igt_sysfs_has_attr(dir, "media_freq_factor")) {
> - media = igt_sysfs_get_u32(dir, "media_freq_factor");
> + __igt_sysfs_get_u32(dir, "media_freq_factor", &media);
> igt_debug("media ratio: %.2f\n", media * FREQ_SCALE_FACTOR);
> }
> }
> @@ -108,6 +110,7 @@ static void cleanup(int dir)
> static void media_freq(int gt, int dir)
> {
> float scale;
> + uint32_t rp0 = 0, rpn = 0;
>
> igt_require(igt_sysfs_has_attr(dir, "media_freq_factor"));
>
> @@ -116,9 +119,9 @@ static void media_freq(int gt, int dir)
>
> setup_freq(gt, dir);
>
> - igt_debug("media RP0 mhz: %d, media RPn mhz: %d\n",
> - igt_sysfs_get_u32(dir, "media_RP0_freq_mhz"),
> - igt_sysfs_get_u32(dir, "media_RPn_freq_mhz"));
> + __igt_sysfs_get_u32(dir, "media_RP0_freq_mhz", &rp0);
> + __igt_sysfs_get_u32(dir, "media_RPn_freq_mhz", &rpn);
> + igt_debug("media RP0 MHz: %u, media RPn MHz: %u\n", rp0, rpn);
> igt_debug("media ratio value 0.0 represents dynamic mode\n");
>
> /*
> @@ -127,7 +130,8 @@ static void media_freq(int gt, int dir)
> * modes. Fixed ratio modes should return the same value.
> */
> for (int v = 256; v >= 0; v -= 64) {
> - int getv, ret;
> + int ret;
> + uint32_t getv = 0;
>
> /*
> * Check that we can set the mode. Ratios other than 1:2
> @@ -141,7 +145,7 @@ static void media_freq(int gt, int dir)
>
> wait_freq_set();
>
> - getv = igt_sysfs_get_u32(dir, "media_freq_factor");
> + __igt_sysfs_get_u32(dir, "media_freq_factor", &getv);
>
> igt_debug("media ratio set: %.2f, media ratio get: %.2f\n",
> v * scale, getv * scale);
> diff --git a/tests/i915/i915_pm_rps.c b/tests/i915/i915_pm_rps.c
> index 7044fcd81..d9d43e568 100644
> --- a/tests/i915/i915_pm_rps.c
> +++ b/tests/i915/i915_pm_rps.c
> @@ -720,7 +720,7 @@ static void fence_order(int i915)
> .buffer_count = ARRAY_SIZE(obj),
> };
> uint64_t wr, rw;
> - int min, max;
> + uint32_t min = 0, max = 0;
> double freq;
> int sysfs;
>
> @@ -742,14 +742,14 @@ static void fence_order(int i915)
> */
>
> sysfs = igt_sysfs_open(i915);
> - min = igt_sysfs_get_u32(sysfs, "gt_RPn_freq_mhz");
> - max = igt_sysfs_get_u32(sysfs, "gt_RP0_freq_mhz");
> + __igt_sysfs_get_u32(sysfs, "gt_RPn_freq_mhz", &min);
> + __igt_sysfs_get_u32(sysfs, "gt_RP0_freq_mhz", &max);
> igt_require(max > min);
>
> /* Only allow ourselves to upclock via waitboosting */
> - igt_sysfs_printf(sysfs, "gt_min_freq_mhz", "%d", min);
> - igt_sysfs_printf(sysfs, "gt_max_freq_mhz", "%d", min);
> - igt_sysfs_printf(sysfs, "gt_boost_freq_mhz", "%d", max);
> + igt_sysfs_printf(sysfs, "gt_min_freq_mhz", "%u", min);
> + igt_sysfs_printf(sysfs, "gt_max_freq_mhz", "%u", min);
> + igt_sysfs_printf(sysfs, "gt_boost_freq_mhz", "%u", max);
>
> /* Warm up to bind the vma */
> __fence_order(i915, &obj[0], &execbuf, 0, 0, &freq);
> @@ -763,8 +763,8 @@ static void fence_order(int i915)
> gem_close(i915, obj[0].handle);
> gem_close(i915, obj[1].handle);
>
> - igt_sysfs_printf(sysfs, "gt_min_freq_mhz", "%d", min);
> - igt_sysfs_printf(sysfs, "gt_max_freq_mhz", "%d", max);
> + igt_sysfs_printf(sysfs, "gt_min_freq_mhz", "%u", min);
> + igt_sysfs_printf(sysfs, "gt_max_freq_mhz", "%u", max);
>
> close(sysfs);
>
> @@ -830,7 +830,7 @@ static void engine_order(int i915)
> uint64_t forward, backward, both;
> unsigned int num_engines;
> const intel_ctx_t *ctx;
> - int min, max;
> + uint32_t min = 0, max = 0;
> double freq;
> int sysfs;
>
> @@ -862,14 +862,14 @@ static void engine_order(int i915)
> execbuf.rsvd1 = ctx->id;
>
> sysfs = igt_sysfs_open(i915);
> - min = igt_sysfs_get_u32(sysfs, "gt_RPn_freq_mhz");
> - max = igt_sysfs_get_u32(sysfs, "gt_RP0_freq_mhz");
> + __igt_sysfs_get_u32(sysfs, "gt_RPn_freq_mhz", &min);
> + __igt_sysfs_get_u32(sysfs, "gt_RP0_freq_mhz", &max);
> igt_require(max > min);
>
> /* Only allow ourselves to upclock via waitboosting */
> - igt_sysfs_printf(sysfs, "gt_min_freq_mhz", "%d", min);
> - igt_sysfs_printf(sysfs, "gt_max_freq_mhz", "%d", min);
> - igt_sysfs_printf(sysfs, "gt_boost_freq_mhz", "%d", max);
> + igt_sysfs_printf(sysfs, "gt_min_freq_mhz", "%u", min);
> + igt_sysfs_printf(sysfs, "gt_max_freq_mhz", "%u", min);
> + igt_sysfs_printf(sysfs, "gt_boost_freq_mhz", "%u", max);
>
> /* Warm up to bind the vma */
> gem_execbuf(i915, &execbuf);
> @@ -893,8 +893,8 @@ static void engine_order(int i915)
> gem_close(i915, obj[1].handle);
> intel_ctx_destroy(i915, ctx);
>
> - igt_sysfs_printf(sysfs, "gt_min_freq_mhz", "%d", min);
> - igt_sysfs_printf(sysfs, "gt_max_freq_mhz", "%d", max);
> + igt_sysfs_printf(sysfs, "gt_min_freq_mhz", "%u", min);
> + igt_sysfs_printf(sysfs, "gt_max_freq_mhz", "%u", max);
>
> close(sysfs);
>
> diff --git a/tests/i915/i915_power.c b/tests/i915/i915_power.c
> index ed7bef495..ee00cbcd8 100644
> --- a/tests/i915/i915_power.c
> +++ b/tests/i915/i915_power.c
> @@ -38,7 +38,8 @@ static void sanity(int i915)
> double idle, busy;
> igt_spin_t *spin;
> uint64_t ahnd;
> - int dir, gt, req, act;
> + int dir, gt;
> + uint32_t req = 0, act = 0;
>
> #define DURATION_SEC 2
>
> @@ -58,9 +59,9 @@ static void sanity(int i915)
> igt_spin_busywait_until_started(spin);
> busy = measure_power(&pwr, DURATION_SEC);
> i915_for_each_gt(i915, dir, gt) {
> - req = igt_sysfs_get_u32(dir, "rps_cur_freq_mhz");
> - act = igt_sysfs_get_u32(dir, "rps_act_freq_mhz");
> - igt_info("gt %d: req MHz: %d, act MHz: %d\n", gt, req, act);
> + __igt_sysfs_get_u32(dir, "rps_cur_freq_mhz", &req);
> + __igt_sysfs_get_u32(dir, "rps_act_freq_mhz", &act);
> + igt_info("gt %d: req MHz: %u, act MHz: %u\n", gt, req, act);
> }
> igt_free_spins(i915);
> put_ahnd(ahnd);
> diff --git a/tests/i915/perf_pmu.c b/tests/i915/perf_pmu.c
> index 0806a8e54..441ec6f57 100644
> --- a/tests/i915/perf_pmu.c
> +++ b/tests/i915/perf_pmu.c
> @@ -1782,7 +1782,7 @@ static igt_spin_t *spin_sync_gt(int i915, uint64_t ahnd, unsigned int gt,
> static void
> test_frequency(int gem_fd, unsigned int gt)
> {
> - uint32_t min_freq, max_freq, boost_freq;
> + uint32_t min_freq = 0, max_freq = 0, boost_freq = 0, read_value = 0;
> uint64_t val[2], start[2], slept;
> double min[2], max[2];
> igt_spin_t *spin;
> @@ -1793,9 +1793,9 @@ test_frequency(int gem_fd, unsigned int gt)
> sysfs = igt_sysfs_gt_open(gem_fd, gt);
> igt_require(sysfs >= 0);
>
> - min_freq = igt_sysfs_get_u32(sysfs, "rps_RPn_freq_mhz");
> - max_freq = igt_sysfs_get_u32(sysfs, "rps_RP0_freq_mhz");
> - boost_freq = igt_sysfs_get_u32(sysfs, "rps_boost_freq_mhz");
> + __igt_sysfs_get_u32(sysfs, "rps_RPn_freq_mhz", &min_freq);
> + __igt_sysfs_get_u32(sysfs, "rps_RP0_freq_mhz", &max_freq);
> + __igt_sysfs_get_u32(sysfs, "rps_boost_freq_mhz", &boost_freq);
> igt_info("Frequency: min=%u, max=%u, boost=%u MHz\n",
> min_freq, max_freq, boost_freq);
> igt_require(min_freq > 0 && max_freq > 0 && boost_freq > 0);
> @@ -1808,12 +1808,15 @@ test_frequency(int gem_fd, unsigned int gt)
> /*
> * Set GPU to min frequency and read PMU counters.
> */
> - igt_require(igt_sysfs_set_u32(sysfs, "rps_min_freq_mhz", min_freq));
> - igt_require(igt_sysfs_get_u32(sysfs, "rps_min_freq_mhz") == min_freq);
> - igt_require(igt_sysfs_set_u32(sysfs, "rps_max_freq_mhz", min_freq));
> - igt_require(igt_sysfs_get_u32(sysfs, "rps_max_freq_mhz") == min_freq);
> - igt_require(igt_sysfs_set_u32(sysfs, "rps_boost_freq_mhz", min_freq));
> - igt_require(igt_sysfs_get_u32(sysfs, "rps_boost_freq_mhz") == min_freq);
> + igt_require(__igt_sysfs_set_u32(sysfs, "rps_min_freq_mhz", min_freq));
> + igt_require(__igt_sysfs_get_u32(sysfs, "rps_min_freq_mhz", &read_value));
> + igt_require(read_value == min_freq);
> + igt_require(__igt_sysfs_set_u32(sysfs, "rps_max_freq_mhz", min_freq));
> + igt_require(__igt_sysfs_get_u32(sysfs, "rps_max_freq_mhz", &read_value));
> + igt_require(read_value == min_freq);
> + igt_require(__igt_sysfs_set_u32(sysfs, "rps_boost_freq_mhz", min_freq));
> + igt_require(__igt_sysfs_get_u32(sysfs, "rps_boost_freq_mhz", &read_value));
> + igt_require(read_value == min_freq);
>
> gem_quiescent_gpu(gem_fd); /* Idle to be sure the change takes effect */
> spin = spin_sync_gt(gem_fd, ahnd, gt, &ctx);
> @@ -1834,13 +1837,16 @@ test_frequency(int gem_fd, unsigned int gt)
> /*
> * Set GPU to max frequency and read PMU counters.
> */
> - igt_require(igt_sysfs_set_u32(sysfs, "rps_max_freq_mhz", max_freq));
> - igt_require(igt_sysfs_get_u32(sysfs, "rps_max_freq_mhz") == max_freq);
> - igt_require(igt_sysfs_set_u32(sysfs, "rps_boost_freq_mhz", boost_freq));
> - igt_require(igt_sysfs_get_u32(sysfs, "rps_boost_freq_mhz") == boost_freq);
> + igt_require(__igt_sysfs_set_u32(sysfs, "rps_max_freq_mhz", max_freq));
> + igt_require(__igt_sysfs_get_u32(sysfs, "rps_max_freq_mhz", &read_value));
> + igt_require(read_value == max_freq);
> + igt_require(__igt_sysfs_set_u32(sysfs, "rps_boost_freq_mhz", boost_freq));
> + igt_require(__igt_sysfs_get_u32(sysfs, "rps_boost_freq_mhz", &read_value));
> + igt_require(read_value == boost_freq);
>
> - igt_require(igt_sysfs_set_u32(sysfs, "rps_min_freq_mhz", max_freq));
> - igt_require(igt_sysfs_get_u32(sysfs, "rps_min_freq_mhz") == max_freq);
> + igt_require(__igt_sysfs_set_u32(sysfs, "rps_min_freq_mhz", max_freq));
> + igt_require(__igt_sysfs_get_u32(sysfs, "rps_min_freq_mhz", &read_value));
> + igt_require(read_value == max_freq);
>
> gem_quiescent_gpu(gem_fd);
> spin = spin_sync_gt(gem_fd, ahnd, gt, &ctx);
> @@ -1859,10 +1865,11 @@ test_frequency(int gem_fd, unsigned int gt)
> /*
> * Restore min/max.
> */
> - igt_sysfs_set_u32(sysfs, "rps_min_freq_mhz", min_freq);
> - if (igt_sysfs_get_u32(sysfs, "rps_min_freq_mhz") != min_freq)
> + __igt_sysfs_set_u32(sysfs, "rps_min_freq_mhz", min_freq);
> + __igt_sysfs_get_u32(sysfs, "rps_min_freq_mhz", &read_value);
> + if (read_value != min_freq)
> igt_warn("Unable to restore min frequency to saved value [%u MHz], now %u MHz\n",
> - min_freq, igt_sysfs_get_u32(sysfs, "rps_min_freq_mhz"));
> + min_freq, read_value);
> close(fd[0]);
> close(fd[1]);
> put_ahnd(ahnd);
> diff --git a/tests/kms_prime.c b/tests/kms_prime.c
> index 52f587961..dedbf6233 100644
> --- a/tests/kms_prime.c
> +++ b/tests/kms_prime.c
> @@ -341,7 +341,7 @@ static void kms_poll_state_restore(void)
> int sysfs_fd;
>
> igt_assert((sysfs_fd = open(KMS_HELPER, O_RDONLY)) >= 0);
> - igt_sysfs_set_boolean(sysfs_fd, "poll", kms_poll_saved_state);
> + __igt_sysfs_set_boolean(sysfs_fd, "poll", kms_poll_saved_state);
> close(sysfs_fd);
>
> }
> diff --git a/tests/nouveau_crc.c b/tests/nouveau_crc.c
> index d5aa0e650..c94972318 100644
> --- a/tests/nouveau_crc.c
> +++ b/tests/nouveau_crc.c
> @@ -264,15 +264,18 @@ static void test_ctx_flip_threshold_reset_after_capture(data_t *data)
> {
> igt_pipe_crc_t *pipe_crc;
> const int fd = data->drm_fd;
> + uint32_t value = 0;
>
> pipe_crc = igt_pipe_crc_new(fd, data->pipe, IGT_PIPE_CRC_SOURCE_AUTO);
>
> set_crc_flip_threshold(data, 5);
> igt_pipe_crc_start(pipe_crc);
> - igt_assert_eq(igt_sysfs_get_u32(data->nv_crc_dir, "flip_threshold"), 5);
> + __igt_sysfs_get_u32(data->nv_crc_dir, "flip_threshold", &value);
> + igt_assert_eq(value, 5);
> igt_pipe_crc_stop(pipe_crc);
>
> - igt_assert_neq(igt_sysfs_get_u32(data->nv_crc_dir, "flip_threshold"), 5);
> + __igt_sysfs_get_u32(data->nv_crc_dir, "flip_threshold", &value);
> + igt_assert_neq(value, 5);
> igt_pipe_crc_free(pipe_crc);
> }
>
> --
> 2.40.0
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [igt-dev] [PATCH i-g-t] lib/igt_sysfs: add asserting helpers for read/write operations
2023-06-19 14:08 ` Laguna, Lukasz
@ 2023-06-21 7:14 ` Laguna, Lukasz
0 siblings, 0 replies; 7+ messages in thread
From: Laguna, Lukasz @ 2023-06-21 7:14 UTC (permalink / raw)
To: Kamil Konieczny, igt-dev, kamil.konieczny
On 6/19/2023 16:08, Laguna, Lukasz wrote:
> On 6/19/2023 14:05, Kamil Konieczny wrote:
>> Hi Łukasz,
>>
>> On 2023-06-19 at 08:26:52 +0200, Łukasz Łaguna wrote:
>>> Prefix names of existing, non-asserting helpers with "__". Replace all
>>> calls to non-asserting ones in order to don't introduce any functional
>> --------- ^
>> Please write the names of functions you will be
>> enhancing/changing.
>>
>>> changes in the existing code.
>>>
>>> Signed-off-by: Lukasz Laguna <lukasz.laguna@intel.com>
>>> ---
>>> lib/i915/gem_submission.c | 2 +-
>>> lib/igt_pm.c | 2 +-
>>> lib/igt_power.c | 2 +-
>>> lib/igt_sysfs.c | 210
>>> +++++++++++++++++++++++++--------
>>> lib/igt_sysfs.h | 16 ++-
>>> tests/i915/i915_pm_dc.c | 4 +-
>>> tests/i915/i915_pm_freq_mult.c | 28 ++---
>>> tests/i915/i915_pm_rps.c | 8 +-
>>> tests/i915/i915_power.c | 4 +-
>>> tests/i915/perf_pmu.c | 38 +++---
>>> tests/kms_prime.c | 4 +-
>>> tests/nouveau_crc.c | 4 +-
>>> 12 files changed, 220 insertions(+), 102 deletions(-)
>>>
>>> diff --git a/lib/i915/gem_submission.c b/lib/i915/gem_submission.c
>>> index adf5eb394..76286129a 100644
>>> --- a/lib/i915/gem_submission.c
>>> +++ b/lib/i915/gem_submission.c
>>> @@ -70,7 +70,7 @@ unsigned gem_submission_method(int fd)
>>> if (dir < 0)
>>> return 0;
>>> - if (igt_sysfs_get_u32(dir, "enable_guc") & 1) {
>>> + if (__igt_sysfs_get_u32(dir, "enable_guc") & 1) {
>> This example show that __igt_sysfs_get_u32 should return 0
>> in case of error or here we should also check for existance
>> of this sysfs param.
>>
>>> method = GEM_SUBMISSION_GUC;
>>> } else if (gen >= 8) {
>>> method = GEM_SUBMISSION_EXECLISTS;
>>> diff --git a/lib/igt_pm.c b/lib/igt_pm.c
>>> index 18c84bf3a..40a245e83 100644
>>> --- a/lib/igt_pm.c
>>> +++ b/lib/igt_pm.c
>>> @@ -1415,5 +1415,5 @@ void igt_pm_ignore_slpc_efficient_freq(int
>>> i915, int gtfd, bool val)
>>> return;
>>> igt_require(igt_sysfs_has_attr(gtfd, "slpc_ignore_eff_freq"));
>>> - igt_assert(igt_sysfs_set_u32(gtfd, "slpc_ignore_eff_freq", val));
>>> + igt_assert(__igt_sysfs_set_u32(gtfd, "slpc_ignore_eff_freq",
>>> val));
>>> }
>>> diff --git a/lib/igt_power.c b/lib/igt_power.c
>>> index 3b34be406..b94a9e09d 100644
>>> --- a/lib/igt_power.c
>>> +++ b/lib/igt_power.c
>>> @@ -137,7 +137,7 @@ void igt_power_get_energy(struct igt_power
>>> *power, struct power_sample *s)
>>> if (power->hwmon_fd >= 0) {
>>> if (igt_sysfs_has_attr(power->hwmon_fd, "energy1_input"))
>>> - s->energy = igt_sysfs_get_u64(power->hwmon_fd,
>>> "energy1_input");
>>> + s->energy = __igt_sysfs_get_u64(power->hwmon_fd,
>>> "energy1_input");
>>> } else if (power->rapl.fd >= 0) {
>>> rapl_read(&power->rapl, s);
>>> }
>>> diff --git a/lib/igt_sysfs.c b/lib/igt_sysfs.c
>>> index 35a4faa9a..c4d98cd53 100644
>>> --- a/lib/igt_sysfs.c
>>> +++ b/lib/igt_sysfs.c
>>> @@ -511,94 +511,163 @@ int igt_sysfs_printf(int dir, const char
>>> *attr, const char *fmt, ...)
>>> return ret;
>>> }
>>> +static uint32_t ___igt_sysfs_get_u32(int dir, const char *attr,
>>> bool assert)
>> ------------------------------------------------------------------
>> ^^^^^^^^^^^
>> Do not write such functions, rather write two variants of
>> get_u32, for example:
>>
>> no asserts here:
>> uint32_t __igt_sysfs_get_u32(int dir, const char *attr)
>>
>> checks present here:
>> uint32_t igt_sysfs_get_u32(int dir, const char *attr)
>
> Hi Kamil, thanks for review.
>
> I implemented such variants below. ___igt_sysfs_get_u32() was used
> only to avoid code duplication (and it's static).
> Do you prefer me to duplicate code in
> __igt_sysfs_get_u32() and igt_sysfs_get_u32() and remove
> ___igt_sysfs_get_u32()?
>
>> or alternativly, you could add
>>
>> bool __igt_sysfs_get_u32(int dir, const char *attr, uint32_t *val)
>
> The main problem with the original implementation of
> igt_sysfs_get_u32() is that we are not able to distinguish if it
> returns 0 because of an error or because it's a read value.
> So I even prefer bool __igt_sysfs_get_u32() , but on the other hand
> helper returning read value is more convenient and I guess that's why
> it was implemented in a such way, so I didn't want to change that. But
> maybe I should? What's your opinion?
>
Hi,
I decided to use bool __igt_sysfs_get_u32() approach and sent a v2 to ML.
Regards,
Lukasz
> Regards,
> Lukasz
>
>> Same note goes for u64 variant,
>>
>> Regrads,
>> Kamil
>>
>>> +{
>>> + uint32_t result;
>>> + int ret;
>>> +
>>> + ret = igt_sysfs_scanf(dir, attr, "%u", &result);
>>> + if (assert)
>>> + igt_assert(ret == 1);
>>> +
>>> + if (igt_debug_on(ret != 1))
>>> + return 0;
>>> +
>>> + return result;
>>> +}
>>> +
>>> /**
>>> - * igt_sysfs_get_u32:
>>> - * @dir: directory for the device from igt_sysfs_open()
>>> - * @attr: name of the sysfs node to open
>>> + * __igt_sysfs_get_u32:
>>> + * @dir: directory corresponding to attribute
>>> + * @attr: name of the sysfs node to read
>>> *
>>> * Convenience wrapper to read a unsigned 32bit integer from a
>>> sysfs file.
>>> *
>>> * Returns:
>>> * The value read.
>>> */
>>> +uint32_t __igt_sysfs_get_u32(int dir, const char *attr)
>>> +{
>>> + return ___igt_sysfs_get_u32(dir, attr, false);
>>> +}
>>> +
>>> +/**
>>> + * igt_sysfs_get_u32:
>>> + * @dir: directory corresponding to attribute
>>> + * @attr: name of the sysfs node to read
>>> + *
>>> + * Asserting equivalent of __igt_sysfs_get_u32.
>>> + *
>>> + * Returns:
>>> + * The value read.
>>> + */
>>> uint32_t igt_sysfs_get_u32(int dir, const char *attr)
>>> {
>>> - uint32_t result;
>>> + return ___igt_sysfs_get_u32(dir, attr, true);
>>> +}
>>> +
>>> +/**
>>> + * __igt_sysfs_set_u32:
>>> + * @dir: directory corresponding to attribute
>>> + * @attr: name of the sysfs node to write
>>> + * @value: value to set
>>> + *
>>> + * Convenience wrapper to write a unsigned 32bit integer to a sysfs
>>> file.
>>> + *
>>> + * Returns:
>>> + * True if successfully written, false otherwise.
>>> + */
>>> +bool __igt_sysfs_set_u32(int dir, const char *attr, uint32_t value)
>>> +{
>>> + return igt_sysfs_printf(dir, attr, "%u", value) > 0;
>>> +}
>>> +
>>> +/**
>>> + * igt_sysfs_set_u32:
>>> + * @dir: directory corresponding to attribute
>>> + * @attr: name of the sysfs node to write
>>> + * @value: value to set
>>> + *
>>> + * Asserting equivalent of __igt_sysfs_set_u32.
>>> + */
>>> +void igt_sysfs_set_u32(int dir, const char *attr, uint32_t value)
>>> +{
>>> + igt_assert(__igt_sysfs_set_u32(dir, attr, value));
>>> +}
>>> - if (igt_debug_on(igt_sysfs_scanf(dir, attr, "%u", &result) !=
>>> 1))
>>> +static uint64_t ___igt_sysfs_get_u64(int dir, const char *attr,
>>> bool assert)
>>> +{
>>> + uint64_t result;
>>> + int ret;
>>> +
>>> + ret = igt_sysfs_scanf(dir, attr, "%"PRIu64, &result);
>>> + if (assert)
>>> + igt_assert(ret == 1);
>>> +
>>> + if (igt_debug_on(ret != 1))
>>> return 0;
>>> return result;
>>> }
>>> /**
>>> - * igt_sysfs_get_u64:
>>> - * @dir: directory for the device from igt_sysfs_open()
>>> - * @attr: name of the sysfs node to open
>>> + * __igt_sysfs_get_u64:
>>> + * @dir: directory corresponding to attribute
>>> + * @attr: name of the sysfs node to read
>>> *
>>> * Convenience wrapper to read a unsigned 64bit integer from a
>>> sysfs file.
>>> *
>>> * Returns:
>>> * The value read.
>>> */
>>> -uint64_t igt_sysfs_get_u64(int dir, const char *attr)
>>> +uint64_t __igt_sysfs_get_u64(int dir, const char *attr)
>>> {
>>> - uint64_t result;
>>> -
>>> - if (igt_debug_on(igt_sysfs_scanf(dir, attr, "%"PRIu64, &result)
>>> != 1))
>>> - return 0;
>>> -
>>> - return result;
>>> + return ___igt_sysfs_get_u64(dir, attr, false);
>>> }
>>> /**
>>> - * igt_sysfs_set_u64:
>>> - * @dir: directory for the device from igt_sysfs_open()
>>> - * @attr: name of the sysfs node to open
>>> - * @value: value to set
>>> + * igt_sysfs_get_u64:
>>> + * @dir: directory corresponding to attribute
>>> + * @attr: name of the sysfs node to read
>>> *
>>> - * Convenience wrapper to write a unsigned 64bit integer to a sysfs
>>> file.
>>> + * Asserting equivalent of __igt_sysfs_get_u64.
>>> *
>>> * Returns:
>>> - * True if successfully written
>>> + * The value read.
>>> */
>>> -bool igt_sysfs_set_u64(int dir, const char *attr, uint64_t value)
>>> +uint64_t igt_sysfs_get_u64(int dir, const char *attr)
>>> {
>>> - return igt_sysfs_printf(dir, attr, "%"PRIu64, value) > 0;
>>> + return ___igt_sysfs_get_u64(dir, attr, true);
>>> }
>>> /**
>>> - * igt_sysfs_set_u32:
>>> - * @dir: directory for the device from igt_sysfs_open()
>>> - * @attr: name of the sysfs node to open
>>> + * __igt_sysfs_set_u64:
>>> + * @dir: directory corresponding to attribute
>>> + * @attr: name of the sysfs node to write
>>> * @value: value to set
>>> *
>>> - * Convenience wrapper to write a unsigned 32bit integer to a sysfs
>>> file.
>>> + * Convenience wrapper to write a unsigned 64bit integer to a sysfs
>>> file.
>>> *
>>> * Returns:
>>> - * True if successfully written
>>> + * True if successfully written, false otherwise.
>>> */
>>> -bool igt_sysfs_set_u32(int dir, const char *attr, uint32_t value)
>>> +bool __igt_sysfs_set_u64(int dir, const char *attr, uint64_t value)
>>> {
>>> - return igt_sysfs_printf(dir, attr, "%u", value) > 0;
>>> + return igt_sysfs_printf(dir, attr, "%"PRIu64, value) > 0;
>>> }
>>> /**
>>> - * igt_sysfs_get_boolean:
>>> - * @dir: directory for the device from igt_sysfs_open()
>>> - * @attr: name of the sysfs node to open
>>> + * igt_sysfs_set_u64:
>>> + * @dir: directory corresponding to attribute
>>> + * @attr: name of the sysfs node to write
>>> + * @value: value to set
>>> *
>>> - * Convenience wrapper to read a boolean sysfs file.
>>> - *
>>> - * Returns:
>>> - * The value read.
>>> + * Asserting equivalent of __igt_sysfs_set_u64.
>>> */
>>> -bool igt_sysfs_get_boolean(int dir, const char *attr)
>>> +void igt_sysfs_set_u64(int dir, const char *attr, uint64_t value)
>>> +{
>>> + igt_assert(__igt_sysfs_set_u64(dir, attr, value));
>>> +}
>>> +
>>> +static bool ___igt_sysfs_get_boolean(int dir, const char *attr,
>>> bool assert)
>>> {
>>> int result;
>>> char *buf;
>>> buf = igt_sysfs_get(dir, attr);
>>> + if (assert)
>>> + igt_assert(buf);
>>> +
>>> if (igt_debug_on(!buf))
>>> return false;
>>> @@ -612,21 +681,64 @@ bool igt_sysfs_get_boolean(int dir, const
>>> char *attr)
>>> }
>>> /**
>>> - * igt_sysfs_set_boolean:
>>> - * @dir: directory for the device from igt_sysfs_open()
>>> - * @attr: name of the sysfs node to open
>>> + * __igt_sysfs_get_boolean:
>>> + * @dir: directory corresponding to attribute
>>> + * @attr: name of the sysfs node to read
>>> + *
>>> + * Convenience wrapper to read a boolean sysfs file.
>>> + *
>>> + * Returns:
>>> + * The value read.
>>> + */
>>> +bool __igt_sysfs_get_boolean(int dir, const char *attr)
>>> +{
>>> + return ___igt_sysfs_get_boolean(dir, attr, false);
>>> +}
>>> +
>>> +/**
>>> + * igt_sysfs_get_boolean:
>>> + * @dir: directory corresponding to attribute
>>> + * @attr: name of the sysfs node to read
>>> + *
>>> + * Asserting equivalent of __igt_sysfs_get_boolean.
>>> + *
>>> + * Returns:
>>> + * The value read.
>>> + */
>>> +bool igt_sysfs_get_boolean(int dir, const char *attr)
>>> +{
>>> + return ___igt_sysfs_get_boolean(dir, attr, true);
>>> +}
>>> +
>>> +/**
>>> + * __igt_sysfs_set_boolean:
>>> + * @dir: directory corresponding to attribute
>>> + * @attr: name of the sysfs node to write
>>> * @value: value to set
>>> *
>>> * Convenience wrapper to write a boolean sysfs file.
>>> - *
>>> + *
>>> * Returns:
>>> - * The value read.
>>> + * True if successfully written, false otherwise.
>>> */
>>> -bool igt_sysfs_set_boolean(int dir, const char *attr, bool value)
>>> +bool __igt_sysfs_set_boolean(int dir, const char *attr, bool value)
>>> {
>>> return igt_sysfs_printf(dir, attr, "%d", value) == 1;
>>> }
>>> +/**
>>> + * igt_sysfs_set_boolean:
>>> + * @dir: directory corresponding to attribute
>>> + * @attr: name of the sysfs node to write
>>> + * @value: value to set
>>> + *
>>> + * Asserting equivalent of __igt_sysfs_set_boolean.
>>> + */
>>> +void igt_sysfs_set_boolean(int dir, const char *attr, bool value)
>>> +{
>>> + igt_assert(__igt_sysfs_set_boolean(dir, attr, value));
>>> +}
>>> +
>>> static void bind_con(const char *name, bool enable)
>>> {
>>> const char *path = "/sys/class/vtconsole";
>>> @@ -797,8 +909,8 @@ static int rw_attr_sweep(igt_sysfs_rw_attr_t *rw)
>>> igt_debug("'%s': sweeping range of values\n", rw->attr);
>>> while (set < UINT64_MAX / 2) {
>>> - ret = igt_sysfs_set_u64(rw->dir, rw->attr, set);
>>> - get = igt_sysfs_get_u64(rw->dir, rw->attr);
>>> + ret = __igt_sysfs_set_u64(rw->dir, rw->attr, set);
>>> + get = __igt_sysfs_get_u64(rw->dir, rw->attr);
>>> igt_debug("'%s': ret %d set %lu get %lu\n", rw->attr, ret,
>>> set, get);
>>> if (ret && rw_attr_equal_within_epsilon(get, set, rw->tol)) {
>>> igt_debug("'%s': matches\n", rw->attr);
>>> @@ -842,7 +954,7 @@ void
>>> igt_sysfs_rw_attr_verify(igt_sysfs_rw_attr_t *rw)
>>> igt_assert(st.st_mode & 0222); /* writable */
>>> igt_assert(rw->start); /* cannot be 0 */
>>> - prev = igt_sysfs_get_u64(rw->dir, rw->attr);
>>> + prev = __igt_sysfs_get_u64(rw->dir, rw->attr);
>>> igt_debug("'%s': prev %lu\n", rw->attr, prev);
>>> ret = rw_attr_sweep(rw);
>>> @@ -851,8 +963,8 @@ void
>>> igt_sysfs_rw_attr_verify(igt_sysfs_rw_attr_t *rw)
>>> * Restore previous value: we don't assert before this point so
>>> * that we can restore the attr before asserting
>>> */
>>> - igt_assert_eq(1, igt_sysfs_set_u64(rw->dir, rw->attr, prev));
>>> - get = igt_sysfs_get_u64(rw->dir, rw->attr);
>>> + igt_assert_eq(1, __igt_sysfs_set_u64(rw->dir, rw->attr, prev));
>>> + get = __igt_sysfs_get_u64(rw->dir, rw->attr);
>>> igt_assert_eq(get, prev);
>>> igt_assert(!ret);
>>> }
>>> diff --git a/lib/igt_sysfs.h b/lib/igt_sysfs.h
>>> index 978b6906e..118137d5e 100644
>>> --- a/lib/igt_sysfs.h
>>> +++ b/lib/igt_sysfs.h
>>> @@ -62,10 +62,10 @@
>>> igt_sysfs_printf(dir, igt_sysfs_dir_id_to_name(dir, id), fmt,
>>> ##__VA_ARGS__)
>>> #define igt_sysfs_rps_get_u32(dir, id) \
>>> - igt_sysfs_get_u32(dir, igt_sysfs_dir_id_to_name(dir, id))
>>> + __igt_sysfs_get_u32(dir, igt_sysfs_dir_id_to_name(dir, id))
>>> #define igt_sysfs_rps_set_u32(dir, id, value) \
>>> - igt_sysfs_set_u32(dir, igt_sysfs_dir_id_to_name(dir, id), value)
>>> + __igt_sysfs_set_u32(dir, igt_sysfs_dir_id_to_name(dir, id), value)
>>> #define igt_sysfs_rps_get_boolean(dir, id) \
>>> igt_sysfs_get_boolean(dir, igt_sysfs_dir_id_to_name(dir, id))
>>> @@ -114,14 +114,20 @@ int igt_sysfs_vprintf(int dir, const char
>>> *attr, const char *fmt, va_list ap)
>>> int igt_sysfs_printf(int dir, const char *attr, const char *fmt, ...)
>>> __attribute__((format(printf,3,4)));
>>> +uint32_t __igt_sysfs_get_u32(int dir, const char *attr);
>>> uint32_t igt_sysfs_get_u32(int dir, const char *attr);
>>> -bool igt_sysfs_set_u32(int dir, const char *attr, uint32_t value);
>>> +bool __igt_sysfs_set_u32(int dir, const char *attr, uint32_t value);
>>> +void igt_sysfs_set_u32(int dir, const char *attr, uint32_t value);
>>> +uint64_t __igt_sysfs_get_u64(int dir, const char *attr);
>>> uint64_t igt_sysfs_get_u64(int dir, const char *attr);
>>> -bool igt_sysfs_set_u64(int dir, const char *attr, uint64_t value);
>>> +bool __igt_sysfs_set_u64(int dir, const char *attr, uint64_t value);
>>> +void igt_sysfs_set_u64(int dir, const char *attr, uint64_t value);
>>> +bool __igt_sysfs_get_boolean(int dir, const char *attr);
>>> bool igt_sysfs_get_boolean(int dir, const char *attr);
>>> -bool igt_sysfs_set_boolean(int dir, const char *attr, bool value);
>>> +bool __igt_sysfs_set_boolean(int dir, const char *attr, bool value);
>>> +void igt_sysfs_set_boolean(int dir, const char *attr, bool value);
>>> void bind_fbcon(bool enable);
>>> void kick_snd_hda_intel(void);
>>> diff --git a/tests/i915/i915_pm_dc.c b/tests/i915/i915_pm_dc.c
>>> index 2bb07ba8b..734bcfa8b 100644
>>> --- a/tests/i915/i915_pm_dc.c
>>> +++ b/tests/i915/i915_pm_dc.c
>>> @@ -534,7 +534,7 @@ static void setup_dc9_dpms(data_t *data, int
>>> dc_target)
>>> igt_require((sysfs_fd = open(KMS_HELPER, O_RDONLY)) >= 0);
>>> kms_poll_saved_state = igt_sysfs_get_boolean(sysfs_fd, "poll");
>>> - igt_sysfs_set_boolean(sysfs_fd, "poll", KMS_POLL_DISABLE);
>>> + __igt_sysfs_set_boolean(sysfs_fd, "poll", KMS_POLL_DISABLE);
>>> close(sysfs_fd);
>>> if (DC9_RESETS_DC_COUNTERS(data->devid)) {
>>> prev_dc = read_dc_counter(data->debugfs_fd, dc_target);
>>> @@ -629,7 +629,7 @@ static void kms_poll_state_restore(int sig)
>>> sysfs_fd = open(KMS_HELPER, O_RDONLY);
>>> if (sysfs_fd >= 0) {
>>> - igt_sysfs_set_boolean(sysfs_fd, "poll", kms_poll_saved_state);
>>> + __igt_sysfs_set_boolean(sysfs_fd, "poll",
>>> kms_poll_saved_state);
>>> close(sysfs_fd);
>>> }
>>> }
>>> diff --git a/tests/i915/i915_pm_freq_mult.c
>>> b/tests/i915/i915_pm_freq_mult.c
>>> index d75ec3f9e..82a578f8e 100644
>>> --- a/tests/i915/i915_pm_freq_mult.c
>>> +++ b/tests/i915/i915_pm_freq_mult.c
>>> @@ -57,11 +57,11 @@ static void restore_rps_defaults(int dir)
>>> if (def < 0)
>>> return;
>>> - max = igt_sysfs_get_u32(def, "rps_max_freq_mhz");
>>> - igt_sysfs_set_u32(dir, "rps_max_freq_mhz", max);
>>> + max = __igt_sysfs_get_u32(def, "rps_max_freq_mhz");
>>> + __igt_sysfs_set_u32(dir, "rps_max_freq_mhz", max);
>>> - min = igt_sysfs_get_u32(def, "rps_min_freq_mhz");
>>> - igt_sysfs_set_u32(dir, "rps_min_freq_mhz", min);
>>> + min = __igt_sysfs_get_u32(def, "rps_min_freq_mhz");
>>> + __igt_sysfs_set_u32(dir, "rps_min_freq_mhz", min);
>>> close(def);
>>> }
>>> @@ -81,17 +81,17 @@ static void setup_freq(int gt, int dir)
>>> wait_freq_set();
>>> /* Print some debug information */
>>> - rp0 = igt_sysfs_get_u32(dir, "rps_RP0_freq_mhz");
>>> - rp1 = igt_sysfs_get_u32(dir, "rps_RP1_freq_mhz");
>>> - rpn = igt_sysfs_get_u32(dir, "rps_RPn_freq_mhz");
>>> - min = igt_sysfs_get_u32(dir, "rps_min_freq_mhz");
>>> - max = igt_sysfs_get_u32(dir, "rps_max_freq_mhz");
>>> - act = igt_sysfs_get_u32(dir, "rps_act_freq_mhz");
>>> + rp0 = __igt_sysfs_get_u32(dir, "rps_RP0_freq_mhz");
>>> + rp1 = __igt_sysfs_get_u32(dir, "rps_RP1_freq_mhz");
>>> + rpn = __igt_sysfs_get_u32(dir, "rps_RPn_freq_mhz");
>>> + min = __igt_sysfs_get_u32(dir, "rps_min_freq_mhz");
>>> + max = __igt_sysfs_get_u32(dir, "rps_max_freq_mhz");
>>> + act = __igt_sysfs_get_u32(dir, "rps_act_freq_mhz");
>>> igt_debug("RP0 MHz: %d, RP1 MHz: %d, RPn MHz: %d, min MHz:
>>> %d, max MHz: %d, act MHz: %d\n", rp0, rp1, rpn, min, max, act);
>>> if (igt_sysfs_has_attr(dir, "media_freq_factor")) {
>>> - media = igt_sysfs_get_u32(dir, "media_freq_factor");
>>> + media = __igt_sysfs_get_u32(dir, "media_freq_factor");
>>> igt_debug("media ratio: %.2f\n", media * FREQ_SCALE_FACTOR);
>>> }
>>> }
>>> @@ -117,8 +117,8 @@ static void media_freq(int gt, int dir)
>>> setup_freq(gt, dir);
>>> igt_debug("media RP0 mhz: %d, media RPn mhz: %d\n",
>>> - igt_sysfs_get_u32(dir, "media_RP0_freq_mhz"),
>>> - igt_sysfs_get_u32(dir, "media_RPn_freq_mhz"));
>>> + __igt_sysfs_get_u32(dir, "media_RP0_freq_mhz"),
>>> + __igt_sysfs_get_u32(dir, "media_RPn_freq_mhz"));
>>> igt_debug("media ratio value 0.0 represents dynamic mode\n");
>>> /*
>>> @@ -141,7 +141,7 @@ static void media_freq(int gt, int dir)
>>> wait_freq_set();
>>> - getv = igt_sysfs_get_u32(dir, "media_freq_factor");
>>> + getv = __igt_sysfs_get_u32(dir, "media_freq_factor");
>>> igt_debug("media ratio set: %.2f, media ratio get: %.2f\n",
>>> v * scale, getv * scale);
>>> diff --git a/tests/i915/i915_pm_rps.c b/tests/i915/i915_pm_rps.c
>>> index eaacc7c90..5ae91ffd2 100644
>>> --- a/tests/i915/i915_pm_rps.c
>>> +++ b/tests/i915/i915_pm_rps.c
>>> @@ -742,8 +742,8 @@ static void fence_order(int i915)
>>> */
>>> sysfs = igt_sysfs_open(i915);
>>> - min = igt_sysfs_get_u32(sysfs, "gt_RPn_freq_mhz");
>>> - max = igt_sysfs_get_u32(sysfs, "gt_RP0_freq_mhz");
>>> + min = __igt_sysfs_get_u32(sysfs, "gt_RPn_freq_mhz");
>>> + max = __igt_sysfs_get_u32(sysfs, "gt_RP0_freq_mhz");
>>> igt_require(max > min);
>>> /* Only allow ourselves to upclock via waitboosting */
>>> @@ -862,8 +862,8 @@ static void engine_order(int i915)
>>> execbuf.rsvd1 = ctx->id;
>>> sysfs = igt_sysfs_open(i915);
>>> - min = igt_sysfs_get_u32(sysfs, "gt_RPn_freq_mhz");
>>> - max = igt_sysfs_get_u32(sysfs, "gt_RP0_freq_mhz");
>>> + min = __igt_sysfs_get_u32(sysfs, "gt_RPn_freq_mhz");
>>> + max = __igt_sysfs_get_u32(sysfs, "gt_RP0_freq_mhz");
>>> igt_require(max > min);
>>> /* Only allow ourselves to upclock via waitboosting */
>>> diff --git a/tests/i915/i915_power.c b/tests/i915/i915_power.c
>>> index 3675b9d6d..86b0fd08c 100644
>>> --- a/tests/i915/i915_power.c
>>> +++ b/tests/i915/i915_power.c
>>> @@ -58,8 +58,8 @@ static void sanity(int i915)
>>> igt_spin_busywait_until_started(spin);
>>> busy = measure_power(&pwr, DURATION_SEC);
>>> i915_for_each_gt(i915, dir, gt) {
>>> - req = igt_sysfs_get_u32(dir, "rps_cur_freq_mhz");
>>> - act = igt_sysfs_get_u32(dir, "rps_act_freq_mhz");
>>> + req = __igt_sysfs_get_u32(dir, "rps_cur_freq_mhz");
>>> + act = __igt_sysfs_get_u32(dir, "rps_act_freq_mhz");
>>> igt_info("gt %d: req MHz: %d, act MHz: %d\n", gt, req, act);
>>> }
>>> igt_free_spins(i915);
>>> diff --git a/tests/i915/perf_pmu.c b/tests/i915/perf_pmu.c
>>> index 8b31df7b2..1a5595d67 100644
>>> --- a/tests/i915/perf_pmu.c
>>> +++ b/tests/i915/perf_pmu.c
>>> @@ -1793,9 +1793,9 @@ test_frequency(int gem_fd, unsigned int gt)
>>> sysfs = igt_sysfs_gt_open(gem_fd, gt);
>>> igt_require(sysfs >= 0);
>>> - min_freq = igt_sysfs_get_u32(sysfs, "rps_RPn_freq_mhz");
>>> - max_freq = igt_sysfs_get_u32(sysfs, "rps_RP0_freq_mhz");
>>> - boost_freq = igt_sysfs_get_u32(sysfs, "rps_boost_freq_mhz");
>>> + min_freq = __igt_sysfs_get_u32(sysfs, "rps_RPn_freq_mhz");
>>> + max_freq = __igt_sysfs_get_u32(sysfs, "rps_RP0_freq_mhz");
>>> + boost_freq = __igt_sysfs_get_u32(sysfs, "rps_boost_freq_mhz");
>>> igt_info("Frequency: min=%u, max=%u, boost=%u MHz\n",
>>> min_freq, max_freq, boost_freq);
>>> igt_require(min_freq > 0 && max_freq > 0 && boost_freq > 0);
>>> @@ -1808,12 +1808,12 @@ test_frequency(int gem_fd, unsigned int gt)
>>> /*
>>> * Set GPU to min frequency and read PMU counters.
>>> */
>>> - igt_require(igt_sysfs_set_u32(sysfs, "rps_min_freq_mhz",
>>> min_freq));
>>> - igt_require(igt_sysfs_get_u32(sysfs, "rps_min_freq_mhz") ==
>>> min_freq);
>>> - igt_require(igt_sysfs_set_u32(sysfs, "rps_max_freq_mhz",
>>> min_freq));
>>> - igt_require(igt_sysfs_get_u32(sysfs, "rps_max_freq_mhz") ==
>>> min_freq);
>>> - igt_require(igt_sysfs_set_u32(sysfs, "rps_boost_freq_mhz",
>>> min_freq));
>>> - igt_require(igt_sysfs_get_u32(sysfs, "rps_boost_freq_mhz") ==
>>> min_freq);
>>> + igt_require(__igt_sysfs_set_u32(sysfs, "rps_min_freq_mhz",
>>> min_freq));
>>> + igt_require(__igt_sysfs_get_u32(sysfs, "rps_min_freq_mhz") ==
>>> min_freq);
>>> + igt_require(__igt_sysfs_set_u32(sysfs, "rps_max_freq_mhz",
>>> min_freq));
>>> + igt_require(__igt_sysfs_get_u32(sysfs, "rps_max_freq_mhz") ==
>>> min_freq);
>>> + igt_require(__igt_sysfs_set_u32(sysfs, "rps_boost_freq_mhz",
>>> min_freq));
>>> + igt_require(__igt_sysfs_get_u32(sysfs, "rps_boost_freq_mhz") ==
>>> min_freq);
>>> gem_quiescent_gpu(gem_fd); /* Idle to be sure the change
>>> takes effect */
>>> spin = spin_sync_gt(gem_fd, ahnd, gt, &ctx);
>>> @@ -1834,13 +1834,13 @@ test_frequency(int gem_fd, unsigned int gt)
>>> /*
>>> * Set GPU to max frequency and read PMU counters.
>>> */
>>> - igt_require(igt_sysfs_set_u32(sysfs, "rps_max_freq_mhz",
>>> max_freq));
>>> - igt_require(igt_sysfs_get_u32(sysfs, "rps_max_freq_mhz") ==
>>> max_freq);
>>> - igt_require(igt_sysfs_set_u32(sysfs, "rps_boost_freq_mhz",
>>> boost_freq));
>>> - igt_require(igt_sysfs_get_u32(sysfs, "rps_boost_freq_mhz") ==
>>> boost_freq);
>>> + igt_require(__igt_sysfs_set_u32(sysfs, "rps_max_freq_mhz",
>>> max_freq));
>>> + igt_require(__igt_sysfs_get_u32(sysfs, "rps_max_freq_mhz") ==
>>> max_freq);
>>> + igt_require(__igt_sysfs_set_u32(sysfs, "rps_boost_freq_mhz",
>>> boost_freq));
>>> + igt_require(__igt_sysfs_get_u32(sysfs, "rps_boost_freq_mhz") ==
>>> boost_freq);
>>> - igt_require(igt_sysfs_set_u32(sysfs, "rps_min_freq_mhz",
>>> max_freq));
>>> - igt_require(igt_sysfs_get_u32(sysfs, "rps_min_freq_mhz") ==
>>> max_freq);
>>> + igt_require(__igt_sysfs_set_u32(sysfs, "rps_min_freq_mhz",
>>> max_freq));
>>> + igt_require(__igt_sysfs_get_u32(sysfs, "rps_min_freq_mhz") ==
>>> max_freq);
>>> gem_quiescent_gpu(gem_fd);
>>> spin = spin_sync_gt(gem_fd, ahnd, gt, &ctx);
>>> @@ -1859,10 +1859,10 @@ test_frequency(int gem_fd, unsigned int gt)
>>> /*
>>> * Restore min/max.
>>> */
>>> - igt_sysfs_set_u32(sysfs, "rps_min_freq_mhz", min_freq);
>>> - if (igt_sysfs_get_u32(sysfs, "rps_min_freq_mhz") != min_freq)
>>> + __igt_sysfs_set_u32(sysfs, "rps_min_freq_mhz", min_freq);
>>> + if (__igt_sysfs_get_u32(sysfs, "rps_min_freq_mhz") != min_freq)
>>> igt_warn("Unable to restore min frequency to saved value
>>> [%u MHz], now %u MHz\n",
>>> - min_freq, igt_sysfs_get_u32(sysfs, "rps_min_freq_mhz"));
>>> + min_freq, __igt_sysfs_get_u32(sysfs,
>>> "rps_min_freq_mhz"));
>>> close(fd[0]);
>>> close(fd[1]);
>>> put_ahnd(ahnd);
>>> @@ -1891,7 +1891,7 @@ test_frequency_idle(int gem_fd, unsigned int gt)
>>> sysfs = igt_sysfs_gt_open(gem_fd, gt);
>>> igt_require(sysfs >= 0);
>>> - min_freq = igt_sysfs_get_u32(sysfs, "rps_RPn_freq_mhz");
>>> + min_freq = __igt_sysfs_get_u32(sysfs, "rps_RPn_freq_mhz");
>>> close(sysfs);
>>> /* While parked, our convention is to report the GPU at 0Hz */
>>> diff --git a/tests/kms_prime.c b/tests/kms_prime.c
>>> index dd5ab993e..98ed30f12 100644
>>> --- a/tests/kms_prime.c
>>> +++ b/tests/kms_prime.c
>>> @@ -341,7 +341,7 @@ static void kms_poll_state_restore(void)
>>> int sysfs_fd;
>>> igt_assert((sysfs_fd = open(KMS_HELPER, O_RDONLY)) >= 0);
>>> - igt_sysfs_set_boolean(sysfs_fd, "poll", kms_poll_saved_state);
>>> + __igt_sysfs_set_boolean(sysfs_fd, "poll", kms_poll_saved_state);
>>> close(sysfs_fd);
>>> }
>>> @@ -352,7 +352,7 @@ static void kms_poll_disable(void)
>>> igt_require((sysfs_fd = open(KMS_HELPER, O_RDONLY)) >= 0);
>>> kms_poll_saved_state = igt_sysfs_get_boolean(sysfs_fd, "poll");
>>> - igt_sysfs_set_boolean(sysfs_fd, "poll", KMS_POLL_DISABLE);
>>> + __igt_sysfs_set_boolean(sysfs_fd, "poll", KMS_POLL_DISABLE);
>>> kms_poll_disabled = true;
>>> close(sysfs_fd);
>>> }
>>> diff --git a/tests/nouveau_crc.c b/tests/nouveau_crc.c
>>> index 785d39bde..b78a53653 100644
>>> --- a/tests/nouveau_crc.c
>>> +++ b/tests/nouveau_crc.c
>>> @@ -269,10 +269,10 @@ static void
>>> test_ctx_flip_threshold_reset_after_capture(data_t *data)
>>> set_crc_flip_threshold(data, 5);
>>> igt_pipe_crc_start(pipe_crc);
>>> - igt_assert_eq(igt_sysfs_get_u32(data->nv_crc_dir,
>>> "flip_threshold"), 5);
>>> + igt_assert_eq(__igt_sysfs_get_u32(data->nv_crc_dir,
>>> "flip_threshold"), 5);
>>> igt_pipe_crc_stop(pipe_crc);
>>> - igt_assert_neq(igt_sysfs_get_u32(data->nv_crc_dir,
>>> "flip_threshold"), 5);
>>> + igt_assert_neq(__igt_sysfs_get_u32(data->nv_crc_dir,
>>> "flip_threshold"), 5);
>>> igt_pipe_crc_free(pipe_crc);
>>> }
>>> --
>>> 2.40.0
>>>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [igt-dev] [PATCH i-g-t] lib/igt_sysfs: add asserting helpers for read/write operations
2023-06-19 12:05 ` Kamil Konieczny
2023-06-19 12:51 ` Kamil Konieczny
@ 2023-06-19 14:08 ` Laguna, Lukasz
2023-06-21 7:14 ` Laguna, Lukasz
1 sibling, 1 reply; 7+ messages in thread
From: Laguna, Lukasz @ 2023-06-19 14:08 UTC (permalink / raw)
To: Kamil Konieczny, igt-dev, kamil.konieczny
On 6/19/2023 14:05, Kamil Konieczny wrote:
> Hi Łukasz,
>
> On 2023-06-19 at 08:26:52 +0200, Łukasz Łaguna wrote:
>> Prefix names of existing, non-asserting helpers with "__". Replace all
>> calls to non-asserting ones in order to don't introduce any functional
> --------- ^
> Please write the names of functions you will be
> enhancing/changing.
>
>> changes in the existing code.
>>
>> Signed-off-by: Lukasz Laguna <lukasz.laguna@intel.com>
>> ---
>> lib/i915/gem_submission.c | 2 +-
>> lib/igt_pm.c | 2 +-
>> lib/igt_power.c | 2 +-
>> lib/igt_sysfs.c | 210 +++++++++++++++++++++++++--------
>> lib/igt_sysfs.h | 16 ++-
>> tests/i915/i915_pm_dc.c | 4 +-
>> tests/i915/i915_pm_freq_mult.c | 28 ++---
>> tests/i915/i915_pm_rps.c | 8 +-
>> tests/i915/i915_power.c | 4 +-
>> tests/i915/perf_pmu.c | 38 +++---
>> tests/kms_prime.c | 4 +-
>> tests/nouveau_crc.c | 4 +-
>> 12 files changed, 220 insertions(+), 102 deletions(-)
>>
>> diff --git a/lib/i915/gem_submission.c b/lib/i915/gem_submission.c
>> index adf5eb394..76286129a 100644
>> --- a/lib/i915/gem_submission.c
>> +++ b/lib/i915/gem_submission.c
>> @@ -70,7 +70,7 @@ unsigned gem_submission_method(int fd)
>> if (dir < 0)
>> return 0;
>>
>> - if (igt_sysfs_get_u32(dir, "enable_guc") & 1) {
>> + if (__igt_sysfs_get_u32(dir, "enable_guc") & 1) {
> This example show that __igt_sysfs_get_u32 should return 0
> in case of error or here we should also check for existance
> of this sysfs param.
>
>> method = GEM_SUBMISSION_GUC;
>> } else if (gen >= 8) {
>> method = GEM_SUBMISSION_EXECLISTS;
>> diff --git a/lib/igt_pm.c b/lib/igt_pm.c
>> index 18c84bf3a..40a245e83 100644
>> --- a/lib/igt_pm.c
>> +++ b/lib/igt_pm.c
>> @@ -1415,5 +1415,5 @@ void igt_pm_ignore_slpc_efficient_freq(int i915, int gtfd, bool val)
>> return;
>>
>> igt_require(igt_sysfs_has_attr(gtfd, "slpc_ignore_eff_freq"));
>> - igt_assert(igt_sysfs_set_u32(gtfd, "slpc_ignore_eff_freq", val));
>> + igt_assert(__igt_sysfs_set_u32(gtfd, "slpc_ignore_eff_freq", val));
>> }
>> diff --git a/lib/igt_power.c b/lib/igt_power.c
>> index 3b34be406..b94a9e09d 100644
>> --- a/lib/igt_power.c
>> +++ b/lib/igt_power.c
>> @@ -137,7 +137,7 @@ void igt_power_get_energy(struct igt_power *power, struct power_sample *s)
>>
>> if (power->hwmon_fd >= 0) {
>> if (igt_sysfs_has_attr(power->hwmon_fd, "energy1_input"))
>> - s->energy = igt_sysfs_get_u64(power->hwmon_fd, "energy1_input");
>> + s->energy = __igt_sysfs_get_u64(power->hwmon_fd, "energy1_input");
>> } else if (power->rapl.fd >= 0) {
>> rapl_read(&power->rapl, s);
>> }
>> diff --git a/lib/igt_sysfs.c b/lib/igt_sysfs.c
>> index 35a4faa9a..c4d98cd53 100644
>> --- a/lib/igt_sysfs.c
>> +++ b/lib/igt_sysfs.c
>> @@ -511,94 +511,163 @@ int igt_sysfs_printf(int dir, const char *attr, const char *fmt, ...)
>> return ret;
>> }
>>
>> +static uint32_t ___igt_sysfs_get_u32(int dir, const char *attr, bool assert)
> ------------------------------------------------------------------ ^^^^^^^^^^^
> Do not write such functions, rather write two variants of
> get_u32, for example:
>
> no asserts here:
> uint32_t __igt_sysfs_get_u32(int dir, const char *attr)
>
> checks present here:
> uint32_t igt_sysfs_get_u32(int dir, const char *attr)
Hi Kamil, thanks for review.
I implemented such variants below. ___igt_sysfs_get_u32() was used only
to avoid code duplication (and it's static).
Do you prefer me to duplicate code in
__igt_sysfs_get_u32() and igt_sysfs_get_u32() and remove
___igt_sysfs_get_u32()?
> or alternativly, you could add
>
> bool __igt_sysfs_get_u32(int dir, const char *attr, uint32_t *val)
The main problem with the original implementation of igt_sysfs_get_u32()
is that we are not able to distinguish if it returns 0 because of an
error or because it's a read value.
So I even prefer bool __igt_sysfs_get_u32() , but on the other hand
helper returning read value is more convenient and I guess that's why it
was implemented in a such way, so I didn't want to change that. But
maybe I should? What's your opinion?
Regards,
Lukasz
> Same note goes for u64 variant,
>
> Regrads,
> Kamil
>
>> +{
>> + uint32_t result;
>> + int ret;
>> +
>> + ret = igt_sysfs_scanf(dir, attr, "%u", &result);
>> + if (assert)
>> + igt_assert(ret == 1);
>> +
>> + if (igt_debug_on(ret != 1))
>> + return 0;
>> +
>> + return result;
>> +}
>> +
>> /**
>> - * igt_sysfs_get_u32:
>> - * @dir: directory for the device from igt_sysfs_open()
>> - * @attr: name of the sysfs node to open
>> + * __igt_sysfs_get_u32:
>> + * @dir: directory corresponding to attribute
>> + * @attr: name of the sysfs node to read
>> *
>> * Convenience wrapper to read a unsigned 32bit integer from a sysfs file.
>> *
>> * Returns:
>> * The value read.
>> */
>> +uint32_t __igt_sysfs_get_u32(int dir, const char *attr)
>> +{
>> + return ___igt_sysfs_get_u32(dir, attr, false);
>> +}
>> +
>> +/**
>> + * igt_sysfs_get_u32:
>> + * @dir: directory corresponding to attribute
>> + * @attr: name of the sysfs node to read
>> + *
>> + * Asserting equivalent of __igt_sysfs_get_u32.
>> + *
>> + * Returns:
>> + * The value read.
>> + */
>> uint32_t igt_sysfs_get_u32(int dir, const char *attr)
>> {
>> - uint32_t result;
>> + return ___igt_sysfs_get_u32(dir, attr, true);
>> +}
>> +
>> +/**
>> + * __igt_sysfs_set_u32:
>> + * @dir: directory corresponding to attribute
>> + * @attr: name of the sysfs node to write
>> + * @value: value to set
>> + *
>> + * Convenience wrapper to write a unsigned 32bit integer to a sysfs file.
>> + *
>> + * Returns:
>> + * True if successfully written, false otherwise.
>> + */
>> +bool __igt_sysfs_set_u32(int dir, const char *attr, uint32_t value)
>> +{
>> + return igt_sysfs_printf(dir, attr, "%u", value) > 0;
>> +}
>> +
>> +/**
>> + * igt_sysfs_set_u32:
>> + * @dir: directory corresponding to attribute
>> + * @attr: name of the sysfs node to write
>> + * @value: value to set
>> + *
>> + * Asserting equivalent of __igt_sysfs_set_u32.
>> + */
>> +void igt_sysfs_set_u32(int dir, const char *attr, uint32_t value)
>> +{
>> + igt_assert(__igt_sysfs_set_u32(dir, attr, value));
>> +}
>>
>> - if (igt_debug_on(igt_sysfs_scanf(dir, attr, "%u", &result) != 1))
>> +static uint64_t ___igt_sysfs_get_u64(int dir, const char *attr, bool assert)
>> +{
>> + uint64_t result;
>> + int ret;
>> +
>> + ret = igt_sysfs_scanf(dir, attr, "%"PRIu64, &result);
>> + if (assert)
>> + igt_assert(ret == 1);
>> +
>> + if (igt_debug_on(ret != 1))
>> return 0;
>>
>> return result;
>> }
>>
>> /**
>> - * igt_sysfs_get_u64:
>> - * @dir: directory for the device from igt_sysfs_open()
>> - * @attr: name of the sysfs node to open
>> + * __igt_sysfs_get_u64:
>> + * @dir: directory corresponding to attribute
>> + * @attr: name of the sysfs node to read
>> *
>> * Convenience wrapper to read a unsigned 64bit integer from a sysfs file.
>> *
>> * Returns:
>> * The value read.
>> */
>> -uint64_t igt_sysfs_get_u64(int dir, const char *attr)
>> +uint64_t __igt_sysfs_get_u64(int dir, const char *attr)
>> {
>> - uint64_t result;
>> -
>> - if (igt_debug_on(igt_sysfs_scanf(dir, attr, "%"PRIu64, &result) != 1))
>> - return 0;
>> -
>> - return result;
>> + return ___igt_sysfs_get_u64(dir, attr, false);
>> }
>>
>> /**
>> - * igt_sysfs_set_u64:
>> - * @dir: directory for the device from igt_sysfs_open()
>> - * @attr: name of the sysfs node to open
>> - * @value: value to set
>> + * igt_sysfs_get_u64:
>> + * @dir: directory corresponding to attribute
>> + * @attr: name of the sysfs node to read
>> *
>> - * Convenience wrapper to write a unsigned 64bit integer to a sysfs file.
>> + * Asserting equivalent of __igt_sysfs_get_u64.
>> *
>> * Returns:
>> - * True if successfully written
>> + * The value read.
>> */
>> -bool igt_sysfs_set_u64(int dir, const char *attr, uint64_t value)
>> +uint64_t igt_sysfs_get_u64(int dir, const char *attr)
>> {
>> - return igt_sysfs_printf(dir, attr, "%"PRIu64, value) > 0;
>> + return ___igt_sysfs_get_u64(dir, attr, true);
>> }
>>
>> /**
>> - * igt_sysfs_set_u32:
>> - * @dir: directory for the device from igt_sysfs_open()
>> - * @attr: name of the sysfs node to open
>> + * __igt_sysfs_set_u64:
>> + * @dir: directory corresponding to attribute
>> + * @attr: name of the sysfs node to write
>> * @value: value to set
>> *
>> - * Convenience wrapper to write a unsigned 32bit integer to a sysfs file.
>> + * Convenience wrapper to write a unsigned 64bit integer to a sysfs file.
>> *
>> * Returns:
>> - * True if successfully written
>> + * True if successfully written, false otherwise.
>> */
>> -bool igt_sysfs_set_u32(int dir, const char *attr, uint32_t value)
>> +bool __igt_sysfs_set_u64(int dir, const char *attr, uint64_t value)
>> {
>> - return igt_sysfs_printf(dir, attr, "%u", value) > 0;
>> + return igt_sysfs_printf(dir, attr, "%"PRIu64, value) > 0;
>> }
>>
>> /**
>> - * igt_sysfs_get_boolean:
>> - * @dir: directory for the device from igt_sysfs_open()
>> - * @attr: name of the sysfs node to open
>> + * igt_sysfs_set_u64:
>> + * @dir: directory corresponding to attribute
>> + * @attr: name of the sysfs node to write
>> + * @value: value to set
>> *
>> - * Convenience wrapper to read a boolean sysfs file.
>> - *
>> - * Returns:
>> - * The value read.
>> + * Asserting equivalent of __igt_sysfs_set_u64.
>> */
>> -bool igt_sysfs_get_boolean(int dir, const char *attr)
>> +void igt_sysfs_set_u64(int dir, const char *attr, uint64_t value)
>> +{
>> + igt_assert(__igt_sysfs_set_u64(dir, attr, value));
>> +}
>> +
>> +static bool ___igt_sysfs_get_boolean(int dir, const char *attr, bool assert)
>> {
>> int result;
>> char *buf;
>>
>> buf = igt_sysfs_get(dir, attr);
>> + if (assert)
>> + igt_assert(buf);
>> +
>> if (igt_debug_on(!buf))
>> return false;
>>
>> @@ -612,21 +681,64 @@ bool igt_sysfs_get_boolean(int dir, const char *attr)
>> }
>>
>> /**
>> - * igt_sysfs_set_boolean:
>> - * @dir: directory for the device from igt_sysfs_open()
>> - * @attr: name of the sysfs node to open
>> + * __igt_sysfs_get_boolean:
>> + * @dir: directory corresponding to attribute
>> + * @attr: name of the sysfs node to read
>> + *
>> + * Convenience wrapper to read a boolean sysfs file.
>> + *
>> + * Returns:
>> + * The value read.
>> + */
>> +bool __igt_sysfs_get_boolean(int dir, const char *attr)
>> +{
>> + return ___igt_sysfs_get_boolean(dir, attr, false);
>> +}
>> +
>> +/**
>> + * igt_sysfs_get_boolean:
>> + * @dir: directory corresponding to attribute
>> + * @attr: name of the sysfs node to read
>> + *
>> + * Asserting equivalent of __igt_sysfs_get_boolean.
>> + *
>> + * Returns:
>> + * The value read.
>> + */
>> +bool igt_sysfs_get_boolean(int dir, const char *attr)
>> +{
>> + return ___igt_sysfs_get_boolean(dir, attr, true);
>> +}
>> +
>> +/**
>> + * __igt_sysfs_set_boolean:
>> + * @dir: directory corresponding to attribute
>> + * @attr: name of the sysfs node to write
>> * @value: value to set
>> *
>> * Convenience wrapper to write a boolean sysfs file.
>> - *
>> + *
>> * Returns:
>> - * The value read.
>> + * True if successfully written, false otherwise.
>> */
>> -bool igt_sysfs_set_boolean(int dir, const char *attr, bool value)
>> +bool __igt_sysfs_set_boolean(int dir, const char *attr, bool value)
>> {
>> return igt_sysfs_printf(dir, attr, "%d", value) == 1;
>> }
>>
>> +/**
>> + * igt_sysfs_set_boolean:
>> + * @dir: directory corresponding to attribute
>> + * @attr: name of the sysfs node to write
>> + * @value: value to set
>> + *
>> + * Asserting equivalent of __igt_sysfs_set_boolean.
>> + */
>> +void igt_sysfs_set_boolean(int dir, const char *attr, bool value)
>> +{
>> + igt_assert(__igt_sysfs_set_boolean(dir, attr, value));
>> +}
>> +
>> static void bind_con(const char *name, bool enable)
>> {
>> const char *path = "/sys/class/vtconsole";
>> @@ -797,8 +909,8 @@ static int rw_attr_sweep(igt_sysfs_rw_attr_t *rw)
>>
>> igt_debug("'%s': sweeping range of values\n", rw->attr);
>> while (set < UINT64_MAX / 2) {
>> - ret = igt_sysfs_set_u64(rw->dir, rw->attr, set);
>> - get = igt_sysfs_get_u64(rw->dir, rw->attr);
>> + ret = __igt_sysfs_set_u64(rw->dir, rw->attr, set);
>> + get = __igt_sysfs_get_u64(rw->dir, rw->attr);
>> igt_debug("'%s': ret %d set %lu get %lu\n", rw->attr, ret, set, get);
>> if (ret && rw_attr_equal_within_epsilon(get, set, rw->tol)) {
>> igt_debug("'%s': matches\n", rw->attr);
>> @@ -842,7 +954,7 @@ void igt_sysfs_rw_attr_verify(igt_sysfs_rw_attr_t *rw)
>> igt_assert(st.st_mode & 0222); /* writable */
>> igt_assert(rw->start); /* cannot be 0 */
>>
>> - prev = igt_sysfs_get_u64(rw->dir, rw->attr);
>> + prev = __igt_sysfs_get_u64(rw->dir, rw->attr);
>> igt_debug("'%s': prev %lu\n", rw->attr, prev);
>>
>> ret = rw_attr_sweep(rw);
>> @@ -851,8 +963,8 @@ void igt_sysfs_rw_attr_verify(igt_sysfs_rw_attr_t *rw)
>> * Restore previous value: we don't assert before this point so
>> * that we can restore the attr before asserting
>> */
>> - igt_assert_eq(1, igt_sysfs_set_u64(rw->dir, rw->attr, prev));
>> - get = igt_sysfs_get_u64(rw->dir, rw->attr);
>> + igt_assert_eq(1, __igt_sysfs_set_u64(rw->dir, rw->attr, prev));
>> + get = __igt_sysfs_get_u64(rw->dir, rw->attr);
>> igt_assert_eq(get, prev);
>> igt_assert(!ret);
>> }
>> diff --git a/lib/igt_sysfs.h b/lib/igt_sysfs.h
>> index 978b6906e..118137d5e 100644
>> --- a/lib/igt_sysfs.h
>> +++ b/lib/igt_sysfs.h
>> @@ -62,10 +62,10 @@
>> igt_sysfs_printf(dir, igt_sysfs_dir_id_to_name(dir, id), fmt, ##__VA_ARGS__)
>>
>> #define igt_sysfs_rps_get_u32(dir, id) \
>> - igt_sysfs_get_u32(dir, igt_sysfs_dir_id_to_name(dir, id))
>> + __igt_sysfs_get_u32(dir, igt_sysfs_dir_id_to_name(dir, id))
>>
>> #define igt_sysfs_rps_set_u32(dir, id, value) \
>> - igt_sysfs_set_u32(dir, igt_sysfs_dir_id_to_name(dir, id), value)
>> + __igt_sysfs_set_u32(dir, igt_sysfs_dir_id_to_name(dir, id), value)
>>
>> #define igt_sysfs_rps_get_boolean(dir, id) \
>> igt_sysfs_get_boolean(dir, igt_sysfs_dir_id_to_name(dir, id))
>> @@ -114,14 +114,20 @@ int igt_sysfs_vprintf(int dir, const char *attr, const char *fmt, va_list ap)
>> int igt_sysfs_printf(int dir, const char *attr, const char *fmt, ...)
>> __attribute__((format(printf,3,4)));
>>
>> +uint32_t __igt_sysfs_get_u32(int dir, const char *attr);
>> uint32_t igt_sysfs_get_u32(int dir, const char *attr);
>> -bool igt_sysfs_set_u32(int dir, const char *attr, uint32_t value);
>> +bool __igt_sysfs_set_u32(int dir, const char *attr, uint32_t value);
>> +void igt_sysfs_set_u32(int dir, const char *attr, uint32_t value);
>>
>> +uint64_t __igt_sysfs_get_u64(int dir, const char *attr);
>> uint64_t igt_sysfs_get_u64(int dir, const char *attr);
>> -bool igt_sysfs_set_u64(int dir, const char *attr, uint64_t value);
>> +bool __igt_sysfs_set_u64(int dir, const char *attr, uint64_t value);
>> +void igt_sysfs_set_u64(int dir, const char *attr, uint64_t value);
>>
>> +bool __igt_sysfs_get_boolean(int dir, const char *attr);
>> bool igt_sysfs_get_boolean(int dir, const char *attr);
>> -bool igt_sysfs_set_boolean(int dir, const char *attr, bool value);
>> +bool __igt_sysfs_set_boolean(int dir, const char *attr, bool value);
>> +void igt_sysfs_set_boolean(int dir, const char *attr, bool value);
>>
>> void bind_fbcon(bool enable);
>> void kick_snd_hda_intel(void);
>> diff --git a/tests/i915/i915_pm_dc.c b/tests/i915/i915_pm_dc.c
>> index 2bb07ba8b..734bcfa8b 100644
>> --- a/tests/i915/i915_pm_dc.c
>> +++ b/tests/i915/i915_pm_dc.c
>> @@ -534,7 +534,7 @@ static void setup_dc9_dpms(data_t *data, int dc_target)
>>
>> igt_require((sysfs_fd = open(KMS_HELPER, O_RDONLY)) >= 0);
>> kms_poll_saved_state = igt_sysfs_get_boolean(sysfs_fd, "poll");
>> - igt_sysfs_set_boolean(sysfs_fd, "poll", KMS_POLL_DISABLE);
>> + __igt_sysfs_set_boolean(sysfs_fd, "poll", KMS_POLL_DISABLE);
>> close(sysfs_fd);
>> if (DC9_RESETS_DC_COUNTERS(data->devid)) {
>> prev_dc = read_dc_counter(data->debugfs_fd, dc_target);
>> @@ -629,7 +629,7 @@ static void kms_poll_state_restore(int sig)
>>
>> sysfs_fd = open(KMS_HELPER, O_RDONLY);
>> if (sysfs_fd >= 0) {
>> - igt_sysfs_set_boolean(sysfs_fd, "poll", kms_poll_saved_state);
>> + __igt_sysfs_set_boolean(sysfs_fd, "poll", kms_poll_saved_state);
>> close(sysfs_fd);
>> }
>> }
>> diff --git a/tests/i915/i915_pm_freq_mult.c b/tests/i915/i915_pm_freq_mult.c
>> index d75ec3f9e..82a578f8e 100644
>> --- a/tests/i915/i915_pm_freq_mult.c
>> +++ b/tests/i915/i915_pm_freq_mult.c
>> @@ -57,11 +57,11 @@ static void restore_rps_defaults(int dir)
>> if (def < 0)
>> return;
>>
>> - max = igt_sysfs_get_u32(def, "rps_max_freq_mhz");
>> - igt_sysfs_set_u32(dir, "rps_max_freq_mhz", max);
>> + max = __igt_sysfs_get_u32(def, "rps_max_freq_mhz");
>> + __igt_sysfs_set_u32(dir, "rps_max_freq_mhz", max);
>>
>> - min = igt_sysfs_get_u32(def, "rps_min_freq_mhz");
>> - igt_sysfs_set_u32(dir, "rps_min_freq_mhz", min);
>> + min = __igt_sysfs_get_u32(def, "rps_min_freq_mhz");
>> + __igt_sysfs_set_u32(dir, "rps_min_freq_mhz", min);
>>
>> close(def);
>> }
>> @@ -81,17 +81,17 @@ static void setup_freq(int gt, int dir)
>> wait_freq_set();
>>
>> /* Print some debug information */
>> - rp0 = igt_sysfs_get_u32(dir, "rps_RP0_freq_mhz");
>> - rp1 = igt_sysfs_get_u32(dir, "rps_RP1_freq_mhz");
>> - rpn = igt_sysfs_get_u32(dir, "rps_RPn_freq_mhz");
>> - min = igt_sysfs_get_u32(dir, "rps_min_freq_mhz");
>> - max = igt_sysfs_get_u32(dir, "rps_max_freq_mhz");
>> - act = igt_sysfs_get_u32(dir, "rps_act_freq_mhz");
>> + rp0 = __igt_sysfs_get_u32(dir, "rps_RP0_freq_mhz");
>> + rp1 = __igt_sysfs_get_u32(dir, "rps_RP1_freq_mhz");
>> + rpn = __igt_sysfs_get_u32(dir, "rps_RPn_freq_mhz");
>> + min = __igt_sysfs_get_u32(dir, "rps_min_freq_mhz");
>> + max = __igt_sysfs_get_u32(dir, "rps_max_freq_mhz");
>> + act = __igt_sysfs_get_u32(dir, "rps_act_freq_mhz");
>>
>> igt_debug("RP0 MHz: %d, RP1 MHz: %d, RPn MHz: %d, min MHz: %d, max MHz: %d, act MHz: %d\n", rp0, rp1, rpn, min, max, act);
>>
>> if (igt_sysfs_has_attr(dir, "media_freq_factor")) {
>> - media = igt_sysfs_get_u32(dir, "media_freq_factor");
>> + media = __igt_sysfs_get_u32(dir, "media_freq_factor");
>> igt_debug("media ratio: %.2f\n", media * FREQ_SCALE_FACTOR);
>> }
>> }
>> @@ -117,8 +117,8 @@ static void media_freq(int gt, int dir)
>> setup_freq(gt, dir);
>>
>> igt_debug("media RP0 mhz: %d, media RPn mhz: %d\n",
>> - igt_sysfs_get_u32(dir, "media_RP0_freq_mhz"),
>> - igt_sysfs_get_u32(dir, "media_RPn_freq_mhz"));
>> + __igt_sysfs_get_u32(dir, "media_RP0_freq_mhz"),
>> + __igt_sysfs_get_u32(dir, "media_RPn_freq_mhz"));
>> igt_debug("media ratio value 0.0 represents dynamic mode\n");
>>
>> /*
>> @@ -141,7 +141,7 @@ static void media_freq(int gt, int dir)
>>
>> wait_freq_set();
>>
>> - getv = igt_sysfs_get_u32(dir, "media_freq_factor");
>> + getv = __igt_sysfs_get_u32(dir, "media_freq_factor");
>>
>> igt_debug("media ratio set: %.2f, media ratio get: %.2f\n",
>> v * scale, getv * scale);
>> diff --git a/tests/i915/i915_pm_rps.c b/tests/i915/i915_pm_rps.c
>> index eaacc7c90..5ae91ffd2 100644
>> --- a/tests/i915/i915_pm_rps.c
>> +++ b/tests/i915/i915_pm_rps.c
>> @@ -742,8 +742,8 @@ static void fence_order(int i915)
>> */
>>
>> sysfs = igt_sysfs_open(i915);
>> - min = igt_sysfs_get_u32(sysfs, "gt_RPn_freq_mhz");
>> - max = igt_sysfs_get_u32(sysfs, "gt_RP0_freq_mhz");
>> + min = __igt_sysfs_get_u32(sysfs, "gt_RPn_freq_mhz");
>> + max = __igt_sysfs_get_u32(sysfs, "gt_RP0_freq_mhz");
>> igt_require(max > min);
>>
>> /* Only allow ourselves to upclock via waitboosting */
>> @@ -862,8 +862,8 @@ static void engine_order(int i915)
>> execbuf.rsvd1 = ctx->id;
>>
>> sysfs = igt_sysfs_open(i915);
>> - min = igt_sysfs_get_u32(sysfs, "gt_RPn_freq_mhz");
>> - max = igt_sysfs_get_u32(sysfs, "gt_RP0_freq_mhz");
>> + min = __igt_sysfs_get_u32(sysfs, "gt_RPn_freq_mhz");
>> + max = __igt_sysfs_get_u32(sysfs, "gt_RP0_freq_mhz");
>> igt_require(max > min);
>>
>> /* Only allow ourselves to upclock via waitboosting */
>> diff --git a/tests/i915/i915_power.c b/tests/i915/i915_power.c
>> index 3675b9d6d..86b0fd08c 100644
>> --- a/tests/i915/i915_power.c
>> +++ b/tests/i915/i915_power.c
>> @@ -58,8 +58,8 @@ static void sanity(int i915)
>> igt_spin_busywait_until_started(spin);
>> busy = measure_power(&pwr, DURATION_SEC);
>> i915_for_each_gt(i915, dir, gt) {
>> - req = igt_sysfs_get_u32(dir, "rps_cur_freq_mhz");
>> - act = igt_sysfs_get_u32(dir, "rps_act_freq_mhz");
>> + req = __igt_sysfs_get_u32(dir, "rps_cur_freq_mhz");
>> + act = __igt_sysfs_get_u32(dir, "rps_act_freq_mhz");
>> igt_info("gt %d: req MHz: %d, act MHz: %d\n", gt, req, act);
>> }
>> igt_free_spins(i915);
>> diff --git a/tests/i915/perf_pmu.c b/tests/i915/perf_pmu.c
>> index 8b31df7b2..1a5595d67 100644
>> --- a/tests/i915/perf_pmu.c
>> +++ b/tests/i915/perf_pmu.c
>> @@ -1793,9 +1793,9 @@ test_frequency(int gem_fd, unsigned int gt)
>> sysfs = igt_sysfs_gt_open(gem_fd, gt);
>> igt_require(sysfs >= 0);
>>
>> - min_freq = igt_sysfs_get_u32(sysfs, "rps_RPn_freq_mhz");
>> - max_freq = igt_sysfs_get_u32(sysfs, "rps_RP0_freq_mhz");
>> - boost_freq = igt_sysfs_get_u32(sysfs, "rps_boost_freq_mhz");
>> + min_freq = __igt_sysfs_get_u32(sysfs, "rps_RPn_freq_mhz");
>> + max_freq = __igt_sysfs_get_u32(sysfs, "rps_RP0_freq_mhz");
>> + boost_freq = __igt_sysfs_get_u32(sysfs, "rps_boost_freq_mhz");
>> igt_info("Frequency: min=%u, max=%u, boost=%u MHz\n",
>> min_freq, max_freq, boost_freq);
>> igt_require(min_freq > 0 && max_freq > 0 && boost_freq > 0);
>> @@ -1808,12 +1808,12 @@ test_frequency(int gem_fd, unsigned int gt)
>> /*
>> * Set GPU to min frequency and read PMU counters.
>> */
>> - igt_require(igt_sysfs_set_u32(sysfs, "rps_min_freq_mhz", min_freq));
>> - igt_require(igt_sysfs_get_u32(sysfs, "rps_min_freq_mhz") == min_freq);
>> - igt_require(igt_sysfs_set_u32(sysfs, "rps_max_freq_mhz", min_freq));
>> - igt_require(igt_sysfs_get_u32(sysfs, "rps_max_freq_mhz") == min_freq);
>> - igt_require(igt_sysfs_set_u32(sysfs, "rps_boost_freq_mhz", min_freq));
>> - igt_require(igt_sysfs_get_u32(sysfs, "rps_boost_freq_mhz") == min_freq);
>> + igt_require(__igt_sysfs_set_u32(sysfs, "rps_min_freq_mhz", min_freq));
>> + igt_require(__igt_sysfs_get_u32(sysfs, "rps_min_freq_mhz") == min_freq);
>> + igt_require(__igt_sysfs_set_u32(sysfs, "rps_max_freq_mhz", min_freq));
>> + igt_require(__igt_sysfs_get_u32(sysfs, "rps_max_freq_mhz") == min_freq);
>> + igt_require(__igt_sysfs_set_u32(sysfs, "rps_boost_freq_mhz", min_freq));
>> + igt_require(__igt_sysfs_get_u32(sysfs, "rps_boost_freq_mhz") == min_freq);
>>
>> gem_quiescent_gpu(gem_fd); /* Idle to be sure the change takes effect */
>> spin = spin_sync_gt(gem_fd, ahnd, gt, &ctx);
>> @@ -1834,13 +1834,13 @@ test_frequency(int gem_fd, unsigned int gt)
>> /*
>> * Set GPU to max frequency and read PMU counters.
>> */
>> - igt_require(igt_sysfs_set_u32(sysfs, "rps_max_freq_mhz", max_freq));
>> - igt_require(igt_sysfs_get_u32(sysfs, "rps_max_freq_mhz") == max_freq);
>> - igt_require(igt_sysfs_set_u32(sysfs, "rps_boost_freq_mhz", boost_freq));
>> - igt_require(igt_sysfs_get_u32(sysfs, "rps_boost_freq_mhz") == boost_freq);
>> + igt_require(__igt_sysfs_set_u32(sysfs, "rps_max_freq_mhz", max_freq));
>> + igt_require(__igt_sysfs_get_u32(sysfs, "rps_max_freq_mhz") == max_freq);
>> + igt_require(__igt_sysfs_set_u32(sysfs, "rps_boost_freq_mhz", boost_freq));
>> + igt_require(__igt_sysfs_get_u32(sysfs, "rps_boost_freq_mhz") == boost_freq);
>>
>> - igt_require(igt_sysfs_set_u32(sysfs, "rps_min_freq_mhz", max_freq));
>> - igt_require(igt_sysfs_get_u32(sysfs, "rps_min_freq_mhz") == max_freq);
>> + igt_require(__igt_sysfs_set_u32(sysfs, "rps_min_freq_mhz", max_freq));
>> + igt_require(__igt_sysfs_get_u32(sysfs, "rps_min_freq_mhz") == max_freq);
>>
>> gem_quiescent_gpu(gem_fd);
>> spin = spin_sync_gt(gem_fd, ahnd, gt, &ctx);
>> @@ -1859,10 +1859,10 @@ test_frequency(int gem_fd, unsigned int gt)
>> /*
>> * Restore min/max.
>> */
>> - igt_sysfs_set_u32(sysfs, "rps_min_freq_mhz", min_freq);
>> - if (igt_sysfs_get_u32(sysfs, "rps_min_freq_mhz") != min_freq)
>> + __igt_sysfs_set_u32(sysfs, "rps_min_freq_mhz", min_freq);
>> + if (__igt_sysfs_get_u32(sysfs, "rps_min_freq_mhz") != min_freq)
>> igt_warn("Unable to restore min frequency to saved value [%u MHz], now %u MHz\n",
>> - min_freq, igt_sysfs_get_u32(sysfs, "rps_min_freq_mhz"));
>> + min_freq, __igt_sysfs_get_u32(sysfs, "rps_min_freq_mhz"));
>> close(fd[0]);
>> close(fd[1]);
>> put_ahnd(ahnd);
>> @@ -1891,7 +1891,7 @@ test_frequency_idle(int gem_fd, unsigned int gt)
>> sysfs = igt_sysfs_gt_open(gem_fd, gt);
>> igt_require(sysfs >= 0);
>>
>> - min_freq = igt_sysfs_get_u32(sysfs, "rps_RPn_freq_mhz");
>> + min_freq = __igt_sysfs_get_u32(sysfs, "rps_RPn_freq_mhz");
>> close(sysfs);
>>
>> /* While parked, our convention is to report the GPU at 0Hz */
>> diff --git a/tests/kms_prime.c b/tests/kms_prime.c
>> index dd5ab993e..98ed30f12 100644
>> --- a/tests/kms_prime.c
>> +++ b/tests/kms_prime.c
>> @@ -341,7 +341,7 @@ static void kms_poll_state_restore(void)
>> int sysfs_fd;
>>
>> igt_assert((sysfs_fd = open(KMS_HELPER, O_RDONLY)) >= 0);
>> - igt_sysfs_set_boolean(sysfs_fd, "poll", kms_poll_saved_state);
>> + __igt_sysfs_set_boolean(sysfs_fd, "poll", kms_poll_saved_state);
>> close(sysfs_fd);
>>
>> }
>> @@ -352,7 +352,7 @@ static void kms_poll_disable(void)
>>
>> igt_require((sysfs_fd = open(KMS_HELPER, O_RDONLY)) >= 0);
>> kms_poll_saved_state = igt_sysfs_get_boolean(sysfs_fd, "poll");
>> - igt_sysfs_set_boolean(sysfs_fd, "poll", KMS_POLL_DISABLE);
>> + __igt_sysfs_set_boolean(sysfs_fd, "poll", KMS_POLL_DISABLE);
>> kms_poll_disabled = true;
>> close(sysfs_fd);
>> }
>> diff --git a/tests/nouveau_crc.c b/tests/nouveau_crc.c
>> index 785d39bde..b78a53653 100644
>> --- a/tests/nouveau_crc.c
>> +++ b/tests/nouveau_crc.c
>> @@ -269,10 +269,10 @@ static void test_ctx_flip_threshold_reset_after_capture(data_t *data)
>>
>> set_crc_flip_threshold(data, 5);
>> igt_pipe_crc_start(pipe_crc);
>> - igt_assert_eq(igt_sysfs_get_u32(data->nv_crc_dir, "flip_threshold"), 5);
>> + igt_assert_eq(__igt_sysfs_get_u32(data->nv_crc_dir, "flip_threshold"), 5);
>> igt_pipe_crc_stop(pipe_crc);
>>
>> - igt_assert_neq(igt_sysfs_get_u32(data->nv_crc_dir, "flip_threshold"), 5);
>> + igt_assert_neq(__igt_sysfs_get_u32(data->nv_crc_dir, "flip_threshold"), 5);
>> igt_pipe_crc_free(pipe_crc);
>> }
>>
>> --
>> 2.40.0
>>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [igt-dev] [PATCH i-g-t] lib/igt_sysfs: add asserting helpers for read/write operations
2023-06-19 12:05 ` Kamil Konieczny
@ 2023-06-19 12:51 ` Kamil Konieczny
2023-06-19 14:08 ` Laguna, Lukasz
1 sibling, 0 replies; 7+ messages in thread
From: Kamil Konieczny @ 2023-06-19 12:51 UTC (permalink / raw)
To: igt-dev; +Cc: kamil.konieczny
Hi,
On 2023-06-19 at 14:05:29 +0200, Kamil Konieczny wrote:
> Hi Łukasz,
>
> On 2023-06-19 at 08:26:52 +0200, Łukasz Łaguna wrote:
> > Prefix names of existing, non-asserting helpers with "__". Replace all
> > calls to non-asserting ones in order to don't introduce any functional
> --------- ^
> Please write the names of functions you will be
> enhancing/changing.
>
> > changes in the existing code.
> >
> > Signed-off-by: Lukasz Laguna <lukasz.laguna@intel.com>
> > ---
> > lib/i915/gem_submission.c | 2 +-
> > lib/igt_pm.c | 2 +-
> > lib/igt_power.c | 2 +-
> > lib/igt_sysfs.c | 210 +++++++++++++++++++++++++--------
> > lib/igt_sysfs.h | 16 ++-
> > tests/i915/i915_pm_dc.c | 4 +-
> > tests/i915/i915_pm_freq_mult.c | 28 ++---
> > tests/i915/i915_pm_rps.c | 8 +-
> > tests/i915/i915_power.c | 4 +-
> > tests/i915/perf_pmu.c | 38 +++---
> > tests/kms_prime.c | 4 +-
> > tests/nouveau_crc.c | 4 +-
> > 12 files changed, 220 insertions(+), 102 deletions(-)
> >
> > diff --git a/lib/i915/gem_submission.c b/lib/i915/gem_submission.c
> > index adf5eb394..76286129a 100644
> > --- a/lib/i915/gem_submission.c
> > +++ b/lib/i915/gem_submission.c
> > @@ -70,7 +70,7 @@ unsigned gem_submission_method(int fd)
> > if (dir < 0)
> > return 0;
> >
> > - if (igt_sysfs_get_u32(dir, "enable_guc") & 1) {
> > + if (__igt_sysfs_get_u32(dir, "enable_guc") & 1) {
>
> This example show that __igt_sysfs_get_u32 should return 0
> in case of error or here we should also check for existance
> of this sysfs param.
>
> > method = GEM_SUBMISSION_GUC;
> > } else if (gen >= 8) {
> > method = GEM_SUBMISSION_EXECLISTS;
> > diff --git a/lib/igt_pm.c b/lib/igt_pm.c
> > index 18c84bf3a..40a245e83 100644
> > --- a/lib/igt_pm.c
> > +++ b/lib/igt_pm.c
> > @@ -1415,5 +1415,5 @@ void igt_pm_ignore_slpc_efficient_freq(int i915, int gtfd, bool val)
> > return;
> >
> > igt_require(igt_sysfs_has_attr(gtfd, "slpc_ignore_eff_freq"));
> > - igt_assert(igt_sysfs_set_u32(gtfd, "slpc_ignore_eff_freq", val));
> > + igt_assert(__igt_sysfs_set_u32(gtfd, "slpc_ignore_eff_freq", val));
> > }
> > diff --git a/lib/igt_power.c b/lib/igt_power.c
> > index 3b34be406..b94a9e09d 100644
> > --- a/lib/igt_power.c
> > +++ b/lib/igt_power.c
> > @@ -137,7 +137,7 @@ void igt_power_get_energy(struct igt_power *power, struct power_sample *s)
> >
> > if (power->hwmon_fd >= 0) {
> > if (igt_sysfs_has_attr(power->hwmon_fd, "energy1_input"))
> > - s->energy = igt_sysfs_get_u64(power->hwmon_fd, "energy1_input");
> > + s->energy = __igt_sysfs_get_u64(power->hwmon_fd, "energy1_input");
> > } else if (power->rapl.fd >= 0) {
> > rapl_read(&power->rapl, s);
> > }
> > diff --git a/lib/igt_sysfs.c b/lib/igt_sysfs.c
> > index 35a4faa9a..c4d98cd53 100644
> > --- a/lib/igt_sysfs.c
> > +++ b/lib/igt_sysfs.c
> > @@ -511,94 +511,163 @@ int igt_sysfs_printf(int dir, const char *attr, const char *fmt, ...)
> > return ret;
> > }
> >
> > +static uint32_t ___igt_sysfs_get_u32(int dir, const char *attr, bool assert)
> ------------------------------------------------------------------ ^^^^^^^^^^^
> Do not write such functions, rather write two variants of
> get_u32, for example:
>
> no asserts here:
> uint32_t __igt_sysfs_get_u32(int dir, const char *attr)
>
> checks present here:
> uint32_t __igt_sysfs_get_u32(int dir, const char *attr)
This should be:
uint32_t igt_sysfs_get_u32(int dir, const char *attr)
Regards,
Kamil
>
> or alternativly, you could add
>
> bool __igt_sysfs_get_u32(int dir, const char *attr, uint32_t *val)
>
> Same note goes for u64 variant,
>
> Regrads,
> Kamil
>
> > +{
> > + uint32_t result;
> > + int ret;
> > +
> > + ret = igt_sysfs_scanf(dir, attr, "%u", &result);
> > + if (assert)
> > + igt_assert(ret == 1);
> > +
> > + if (igt_debug_on(ret != 1))
> > + return 0;
> > +
> > + return result;
> > +}
> > +
> > /**
> > - * igt_sysfs_get_u32:
> > - * @dir: directory for the device from igt_sysfs_open()
> > - * @attr: name of the sysfs node to open
> > + * __igt_sysfs_get_u32:
> > + * @dir: directory corresponding to attribute
> > + * @attr: name of the sysfs node to read
> > *
> > * Convenience wrapper to read a unsigned 32bit integer from a sysfs file.
> > *
> > * Returns:
> > * The value read.
> > */
> > +uint32_t __igt_sysfs_get_u32(int dir, const char *attr)
> > +{
> > + return ___igt_sysfs_get_u32(dir, attr, false);
> > +}
> > +
> > +/**
> > + * igt_sysfs_get_u32:
> > + * @dir: directory corresponding to attribute
> > + * @attr: name of the sysfs node to read
> > + *
> > + * Asserting equivalent of __igt_sysfs_get_u32.
> > + *
> > + * Returns:
> > + * The value read.
> > + */
> > uint32_t igt_sysfs_get_u32(int dir, const char *attr)
> > {
> > - uint32_t result;
> > + return ___igt_sysfs_get_u32(dir, attr, true);
> > +}
> > +
> > +/**
> > + * __igt_sysfs_set_u32:
> > + * @dir: directory corresponding to attribute
> > + * @attr: name of the sysfs node to write
> > + * @value: value to set
> > + *
> > + * Convenience wrapper to write a unsigned 32bit integer to a sysfs file.
> > + *
> > + * Returns:
> > + * True if successfully written, false otherwise.
> > + */
> > +bool __igt_sysfs_set_u32(int dir, const char *attr, uint32_t value)
> > +{
> > + return igt_sysfs_printf(dir, attr, "%u", value) > 0;
> > +}
> > +
> > +/**
> > + * igt_sysfs_set_u32:
> > + * @dir: directory corresponding to attribute
> > + * @attr: name of the sysfs node to write
> > + * @value: value to set
> > + *
> > + * Asserting equivalent of __igt_sysfs_set_u32.
> > + */
> > +void igt_sysfs_set_u32(int dir, const char *attr, uint32_t value)
> > +{
> > + igt_assert(__igt_sysfs_set_u32(dir, attr, value));
> > +}
> >
> > - if (igt_debug_on(igt_sysfs_scanf(dir, attr, "%u", &result) != 1))
> > +static uint64_t ___igt_sysfs_get_u64(int dir, const char *attr, bool assert)
> > +{
> > + uint64_t result;
> > + int ret;
> > +
> > + ret = igt_sysfs_scanf(dir, attr, "%"PRIu64, &result);
> > + if (assert)
> > + igt_assert(ret == 1);
> > +
> > + if (igt_debug_on(ret != 1))
> > return 0;
> >
> > return result;
> > }
> >
> > /**
> > - * igt_sysfs_get_u64:
> > - * @dir: directory for the device from igt_sysfs_open()
> > - * @attr: name of the sysfs node to open
> > + * __igt_sysfs_get_u64:
> > + * @dir: directory corresponding to attribute
> > + * @attr: name of the sysfs node to read
> > *
> > * Convenience wrapper to read a unsigned 64bit integer from a sysfs file.
> > *
> > * Returns:
> > * The value read.
> > */
> > -uint64_t igt_sysfs_get_u64(int dir, const char *attr)
> > +uint64_t __igt_sysfs_get_u64(int dir, const char *attr)
> > {
> > - uint64_t result;
> > -
> > - if (igt_debug_on(igt_sysfs_scanf(dir, attr, "%"PRIu64, &result) != 1))
> > - return 0;
> > -
> > - return result;
> > + return ___igt_sysfs_get_u64(dir, attr, false);
> > }
> >
> > /**
> > - * igt_sysfs_set_u64:
> > - * @dir: directory for the device from igt_sysfs_open()
> > - * @attr: name of the sysfs node to open
> > - * @value: value to set
> > + * igt_sysfs_get_u64:
> > + * @dir: directory corresponding to attribute
> > + * @attr: name of the sysfs node to read
> > *
> > - * Convenience wrapper to write a unsigned 64bit integer to a sysfs file.
> > + * Asserting equivalent of __igt_sysfs_get_u64.
> > *
> > * Returns:
> > - * True if successfully written
> > + * The value read.
> > */
> > -bool igt_sysfs_set_u64(int dir, const char *attr, uint64_t value)
> > +uint64_t igt_sysfs_get_u64(int dir, const char *attr)
> > {
> > - return igt_sysfs_printf(dir, attr, "%"PRIu64, value) > 0;
> > + return ___igt_sysfs_get_u64(dir, attr, true);
> > }
> >
> > /**
> > - * igt_sysfs_set_u32:
> > - * @dir: directory for the device from igt_sysfs_open()
> > - * @attr: name of the sysfs node to open
> > + * __igt_sysfs_set_u64:
> > + * @dir: directory corresponding to attribute
> > + * @attr: name of the sysfs node to write
> > * @value: value to set
> > *
> > - * Convenience wrapper to write a unsigned 32bit integer to a sysfs file.
> > + * Convenience wrapper to write a unsigned 64bit integer to a sysfs file.
> > *
> > * Returns:
> > - * True if successfully written
> > + * True if successfully written, false otherwise.
> > */
> > -bool igt_sysfs_set_u32(int dir, const char *attr, uint32_t value)
> > +bool __igt_sysfs_set_u64(int dir, const char *attr, uint64_t value)
> > {
> > - return igt_sysfs_printf(dir, attr, "%u", value) > 0;
> > + return igt_sysfs_printf(dir, attr, "%"PRIu64, value) > 0;
> > }
> >
> > /**
> > - * igt_sysfs_get_boolean:
> > - * @dir: directory for the device from igt_sysfs_open()
> > - * @attr: name of the sysfs node to open
> > + * igt_sysfs_set_u64:
> > + * @dir: directory corresponding to attribute
> > + * @attr: name of the sysfs node to write
> > + * @value: value to set
> > *
> > - * Convenience wrapper to read a boolean sysfs file.
> > - *
> > - * Returns:
> > - * The value read.
> > + * Asserting equivalent of __igt_sysfs_set_u64.
> > */
> > -bool igt_sysfs_get_boolean(int dir, const char *attr)
> > +void igt_sysfs_set_u64(int dir, const char *attr, uint64_t value)
> > +{
> > + igt_assert(__igt_sysfs_set_u64(dir, attr, value));
> > +}
> > +
> > +static bool ___igt_sysfs_get_boolean(int dir, const char *attr, bool assert)
> > {
> > int result;
> > char *buf;
> >
> > buf = igt_sysfs_get(dir, attr);
> > + if (assert)
> > + igt_assert(buf);
> > +
> > if (igt_debug_on(!buf))
> > return false;
> >
> > @@ -612,21 +681,64 @@ bool igt_sysfs_get_boolean(int dir, const char *attr)
> > }
> >
> > /**
> > - * igt_sysfs_set_boolean:
> > - * @dir: directory for the device from igt_sysfs_open()
> > - * @attr: name of the sysfs node to open
> > + * __igt_sysfs_get_boolean:
> > + * @dir: directory corresponding to attribute
> > + * @attr: name of the sysfs node to read
> > + *
> > + * Convenience wrapper to read a boolean sysfs file.
> > + *
> > + * Returns:
> > + * The value read.
> > + */
> > +bool __igt_sysfs_get_boolean(int dir, const char *attr)
> > +{
> > + return ___igt_sysfs_get_boolean(dir, attr, false);
> > +}
> > +
> > +/**
> > + * igt_sysfs_get_boolean:
> > + * @dir: directory corresponding to attribute
> > + * @attr: name of the sysfs node to read
> > + *
> > + * Asserting equivalent of __igt_sysfs_get_boolean.
> > + *
> > + * Returns:
> > + * The value read.
> > + */
> > +bool igt_sysfs_get_boolean(int dir, const char *attr)
> > +{
> > + return ___igt_sysfs_get_boolean(dir, attr, true);
> > +}
> > +
> > +/**
> > + * __igt_sysfs_set_boolean:
> > + * @dir: directory corresponding to attribute
> > + * @attr: name of the sysfs node to write
> > * @value: value to set
> > *
> > * Convenience wrapper to write a boolean sysfs file.
> > - *
> > + *
> > * Returns:
> > - * The value read.
> > + * True if successfully written, false otherwise.
> > */
> > -bool igt_sysfs_set_boolean(int dir, const char *attr, bool value)
> > +bool __igt_sysfs_set_boolean(int dir, const char *attr, bool value)
> > {
> > return igt_sysfs_printf(dir, attr, "%d", value) == 1;
> > }
> >
> > +/**
> > + * igt_sysfs_set_boolean:
> > + * @dir: directory corresponding to attribute
> > + * @attr: name of the sysfs node to write
> > + * @value: value to set
> > + *
> > + * Asserting equivalent of __igt_sysfs_set_boolean.
> > + */
> > +void igt_sysfs_set_boolean(int dir, const char *attr, bool value)
> > +{
> > + igt_assert(__igt_sysfs_set_boolean(dir, attr, value));
> > +}
> > +
> > static void bind_con(const char *name, bool enable)
> > {
> > const char *path = "/sys/class/vtconsole";
> > @@ -797,8 +909,8 @@ static int rw_attr_sweep(igt_sysfs_rw_attr_t *rw)
> >
> > igt_debug("'%s': sweeping range of values\n", rw->attr);
> > while (set < UINT64_MAX / 2) {
> > - ret = igt_sysfs_set_u64(rw->dir, rw->attr, set);
> > - get = igt_sysfs_get_u64(rw->dir, rw->attr);
> > + ret = __igt_sysfs_set_u64(rw->dir, rw->attr, set);
> > + get = __igt_sysfs_get_u64(rw->dir, rw->attr);
> > igt_debug("'%s': ret %d set %lu get %lu\n", rw->attr, ret, set, get);
> > if (ret && rw_attr_equal_within_epsilon(get, set, rw->tol)) {
> > igt_debug("'%s': matches\n", rw->attr);
> > @@ -842,7 +954,7 @@ void igt_sysfs_rw_attr_verify(igt_sysfs_rw_attr_t *rw)
> > igt_assert(st.st_mode & 0222); /* writable */
> > igt_assert(rw->start); /* cannot be 0 */
> >
> > - prev = igt_sysfs_get_u64(rw->dir, rw->attr);
> > + prev = __igt_sysfs_get_u64(rw->dir, rw->attr);
> > igt_debug("'%s': prev %lu\n", rw->attr, prev);
> >
> > ret = rw_attr_sweep(rw);
> > @@ -851,8 +963,8 @@ void igt_sysfs_rw_attr_verify(igt_sysfs_rw_attr_t *rw)
> > * Restore previous value: we don't assert before this point so
> > * that we can restore the attr before asserting
> > */
> > - igt_assert_eq(1, igt_sysfs_set_u64(rw->dir, rw->attr, prev));
> > - get = igt_sysfs_get_u64(rw->dir, rw->attr);
> > + igt_assert_eq(1, __igt_sysfs_set_u64(rw->dir, rw->attr, prev));
> > + get = __igt_sysfs_get_u64(rw->dir, rw->attr);
> > igt_assert_eq(get, prev);
> > igt_assert(!ret);
> > }
> > diff --git a/lib/igt_sysfs.h b/lib/igt_sysfs.h
> > index 978b6906e..118137d5e 100644
> > --- a/lib/igt_sysfs.h
> > +++ b/lib/igt_sysfs.h
> > @@ -62,10 +62,10 @@
> > igt_sysfs_printf(dir, igt_sysfs_dir_id_to_name(dir, id), fmt, ##__VA_ARGS__)
> >
> > #define igt_sysfs_rps_get_u32(dir, id) \
> > - igt_sysfs_get_u32(dir, igt_sysfs_dir_id_to_name(dir, id))
> > + __igt_sysfs_get_u32(dir, igt_sysfs_dir_id_to_name(dir, id))
> >
> > #define igt_sysfs_rps_set_u32(dir, id, value) \
> > - igt_sysfs_set_u32(dir, igt_sysfs_dir_id_to_name(dir, id), value)
> > + __igt_sysfs_set_u32(dir, igt_sysfs_dir_id_to_name(dir, id), value)
> >
> > #define igt_sysfs_rps_get_boolean(dir, id) \
> > igt_sysfs_get_boolean(dir, igt_sysfs_dir_id_to_name(dir, id))
> > @@ -114,14 +114,20 @@ int igt_sysfs_vprintf(int dir, const char *attr, const char *fmt, va_list ap)
> > int igt_sysfs_printf(int dir, const char *attr, const char *fmt, ...)
> > __attribute__((format(printf,3,4)));
> >
> > +uint32_t __igt_sysfs_get_u32(int dir, const char *attr);
> > uint32_t igt_sysfs_get_u32(int dir, const char *attr);
> > -bool igt_sysfs_set_u32(int dir, const char *attr, uint32_t value);
> > +bool __igt_sysfs_set_u32(int dir, const char *attr, uint32_t value);
> > +void igt_sysfs_set_u32(int dir, const char *attr, uint32_t value);
> >
> > +uint64_t __igt_sysfs_get_u64(int dir, const char *attr);
> > uint64_t igt_sysfs_get_u64(int dir, const char *attr);
> > -bool igt_sysfs_set_u64(int dir, const char *attr, uint64_t value);
> > +bool __igt_sysfs_set_u64(int dir, const char *attr, uint64_t value);
> > +void igt_sysfs_set_u64(int dir, const char *attr, uint64_t value);
> >
> > +bool __igt_sysfs_get_boolean(int dir, const char *attr);
> > bool igt_sysfs_get_boolean(int dir, const char *attr);
> > -bool igt_sysfs_set_boolean(int dir, const char *attr, bool value);
> > +bool __igt_sysfs_set_boolean(int dir, const char *attr, bool value);
> > +void igt_sysfs_set_boolean(int dir, const char *attr, bool value);
> >
> > void bind_fbcon(bool enable);
> > void kick_snd_hda_intel(void);
> > diff --git a/tests/i915/i915_pm_dc.c b/tests/i915/i915_pm_dc.c
> > index 2bb07ba8b..734bcfa8b 100644
> > --- a/tests/i915/i915_pm_dc.c
> > +++ b/tests/i915/i915_pm_dc.c
> > @@ -534,7 +534,7 @@ static void setup_dc9_dpms(data_t *data, int dc_target)
> >
> > igt_require((sysfs_fd = open(KMS_HELPER, O_RDONLY)) >= 0);
> > kms_poll_saved_state = igt_sysfs_get_boolean(sysfs_fd, "poll");
> > - igt_sysfs_set_boolean(sysfs_fd, "poll", KMS_POLL_DISABLE);
> > + __igt_sysfs_set_boolean(sysfs_fd, "poll", KMS_POLL_DISABLE);
> > close(sysfs_fd);
> > if (DC9_RESETS_DC_COUNTERS(data->devid)) {
> > prev_dc = read_dc_counter(data->debugfs_fd, dc_target);
> > @@ -629,7 +629,7 @@ static void kms_poll_state_restore(int sig)
> >
> > sysfs_fd = open(KMS_HELPER, O_RDONLY);
> > if (sysfs_fd >= 0) {
> > - igt_sysfs_set_boolean(sysfs_fd, "poll", kms_poll_saved_state);
> > + __igt_sysfs_set_boolean(sysfs_fd, "poll", kms_poll_saved_state);
> > close(sysfs_fd);
> > }
> > }
> > diff --git a/tests/i915/i915_pm_freq_mult.c b/tests/i915/i915_pm_freq_mult.c
> > index d75ec3f9e..82a578f8e 100644
> > --- a/tests/i915/i915_pm_freq_mult.c
> > +++ b/tests/i915/i915_pm_freq_mult.c
> > @@ -57,11 +57,11 @@ static void restore_rps_defaults(int dir)
> > if (def < 0)
> > return;
> >
> > - max = igt_sysfs_get_u32(def, "rps_max_freq_mhz");
> > - igt_sysfs_set_u32(dir, "rps_max_freq_mhz", max);
> > + max = __igt_sysfs_get_u32(def, "rps_max_freq_mhz");
> > + __igt_sysfs_set_u32(dir, "rps_max_freq_mhz", max);
> >
> > - min = igt_sysfs_get_u32(def, "rps_min_freq_mhz");
> > - igt_sysfs_set_u32(dir, "rps_min_freq_mhz", min);
> > + min = __igt_sysfs_get_u32(def, "rps_min_freq_mhz");
> > + __igt_sysfs_set_u32(dir, "rps_min_freq_mhz", min);
> >
> > close(def);
> > }
> > @@ -81,17 +81,17 @@ static void setup_freq(int gt, int dir)
> > wait_freq_set();
> >
> > /* Print some debug information */
> > - rp0 = igt_sysfs_get_u32(dir, "rps_RP0_freq_mhz");
> > - rp1 = igt_sysfs_get_u32(dir, "rps_RP1_freq_mhz");
> > - rpn = igt_sysfs_get_u32(dir, "rps_RPn_freq_mhz");
> > - min = igt_sysfs_get_u32(dir, "rps_min_freq_mhz");
> > - max = igt_sysfs_get_u32(dir, "rps_max_freq_mhz");
> > - act = igt_sysfs_get_u32(dir, "rps_act_freq_mhz");
> > + rp0 = __igt_sysfs_get_u32(dir, "rps_RP0_freq_mhz");
> > + rp1 = __igt_sysfs_get_u32(dir, "rps_RP1_freq_mhz");
> > + rpn = __igt_sysfs_get_u32(dir, "rps_RPn_freq_mhz");
> > + min = __igt_sysfs_get_u32(dir, "rps_min_freq_mhz");
> > + max = __igt_sysfs_get_u32(dir, "rps_max_freq_mhz");
> > + act = __igt_sysfs_get_u32(dir, "rps_act_freq_mhz");
> >
> > igt_debug("RP0 MHz: %d, RP1 MHz: %d, RPn MHz: %d, min MHz: %d, max MHz: %d, act MHz: %d\n", rp0, rp1, rpn, min, max, act);
> >
> > if (igt_sysfs_has_attr(dir, "media_freq_factor")) {
> > - media = igt_sysfs_get_u32(dir, "media_freq_factor");
> > + media = __igt_sysfs_get_u32(dir, "media_freq_factor");
> > igt_debug("media ratio: %.2f\n", media * FREQ_SCALE_FACTOR);
> > }
> > }
> > @@ -117,8 +117,8 @@ static void media_freq(int gt, int dir)
> > setup_freq(gt, dir);
> >
> > igt_debug("media RP0 mhz: %d, media RPn mhz: %d\n",
> > - igt_sysfs_get_u32(dir, "media_RP0_freq_mhz"),
> > - igt_sysfs_get_u32(dir, "media_RPn_freq_mhz"));
> > + __igt_sysfs_get_u32(dir, "media_RP0_freq_mhz"),
> > + __igt_sysfs_get_u32(dir, "media_RPn_freq_mhz"));
> > igt_debug("media ratio value 0.0 represents dynamic mode\n");
> >
> > /*
> > @@ -141,7 +141,7 @@ static void media_freq(int gt, int dir)
> >
> > wait_freq_set();
> >
> > - getv = igt_sysfs_get_u32(dir, "media_freq_factor");
> > + getv = __igt_sysfs_get_u32(dir, "media_freq_factor");
> >
> > igt_debug("media ratio set: %.2f, media ratio get: %.2f\n",
> > v * scale, getv * scale);
> > diff --git a/tests/i915/i915_pm_rps.c b/tests/i915/i915_pm_rps.c
> > index eaacc7c90..5ae91ffd2 100644
> > --- a/tests/i915/i915_pm_rps.c
> > +++ b/tests/i915/i915_pm_rps.c
> > @@ -742,8 +742,8 @@ static void fence_order(int i915)
> > */
> >
> > sysfs = igt_sysfs_open(i915);
> > - min = igt_sysfs_get_u32(sysfs, "gt_RPn_freq_mhz");
> > - max = igt_sysfs_get_u32(sysfs, "gt_RP0_freq_mhz");
> > + min = __igt_sysfs_get_u32(sysfs, "gt_RPn_freq_mhz");
> > + max = __igt_sysfs_get_u32(sysfs, "gt_RP0_freq_mhz");
> > igt_require(max > min);
> >
> > /* Only allow ourselves to upclock via waitboosting */
> > @@ -862,8 +862,8 @@ static void engine_order(int i915)
> > execbuf.rsvd1 = ctx->id;
> >
> > sysfs = igt_sysfs_open(i915);
> > - min = igt_sysfs_get_u32(sysfs, "gt_RPn_freq_mhz");
> > - max = igt_sysfs_get_u32(sysfs, "gt_RP0_freq_mhz");
> > + min = __igt_sysfs_get_u32(sysfs, "gt_RPn_freq_mhz");
> > + max = __igt_sysfs_get_u32(sysfs, "gt_RP0_freq_mhz");
> > igt_require(max > min);
> >
> > /* Only allow ourselves to upclock via waitboosting */
> > diff --git a/tests/i915/i915_power.c b/tests/i915/i915_power.c
> > index 3675b9d6d..86b0fd08c 100644
> > --- a/tests/i915/i915_power.c
> > +++ b/tests/i915/i915_power.c
> > @@ -58,8 +58,8 @@ static void sanity(int i915)
> > igt_spin_busywait_until_started(spin);
> > busy = measure_power(&pwr, DURATION_SEC);
> > i915_for_each_gt(i915, dir, gt) {
> > - req = igt_sysfs_get_u32(dir, "rps_cur_freq_mhz");
> > - act = igt_sysfs_get_u32(dir, "rps_act_freq_mhz");
> > + req = __igt_sysfs_get_u32(dir, "rps_cur_freq_mhz");
> > + act = __igt_sysfs_get_u32(dir, "rps_act_freq_mhz");
> > igt_info("gt %d: req MHz: %d, act MHz: %d\n", gt, req, act);
> > }
> > igt_free_spins(i915);
> > diff --git a/tests/i915/perf_pmu.c b/tests/i915/perf_pmu.c
> > index 8b31df7b2..1a5595d67 100644
> > --- a/tests/i915/perf_pmu.c
> > +++ b/tests/i915/perf_pmu.c
> > @@ -1793,9 +1793,9 @@ test_frequency(int gem_fd, unsigned int gt)
> > sysfs = igt_sysfs_gt_open(gem_fd, gt);
> > igt_require(sysfs >= 0);
> >
> > - min_freq = igt_sysfs_get_u32(sysfs, "rps_RPn_freq_mhz");
> > - max_freq = igt_sysfs_get_u32(sysfs, "rps_RP0_freq_mhz");
> > - boost_freq = igt_sysfs_get_u32(sysfs, "rps_boost_freq_mhz");
> > + min_freq = __igt_sysfs_get_u32(sysfs, "rps_RPn_freq_mhz");
> > + max_freq = __igt_sysfs_get_u32(sysfs, "rps_RP0_freq_mhz");
> > + boost_freq = __igt_sysfs_get_u32(sysfs, "rps_boost_freq_mhz");
> > igt_info("Frequency: min=%u, max=%u, boost=%u MHz\n",
> > min_freq, max_freq, boost_freq);
> > igt_require(min_freq > 0 && max_freq > 0 && boost_freq > 0);
> > @@ -1808,12 +1808,12 @@ test_frequency(int gem_fd, unsigned int gt)
> > /*
> > * Set GPU to min frequency and read PMU counters.
> > */
> > - igt_require(igt_sysfs_set_u32(sysfs, "rps_min_freq_mhz", min_freq));
> > - igt_require(igt_sysfs_get_u32(sysfs, "rps_min_freq_mhz") == min_freq);
> > - igt_require(igt_sysfs_set_u32(sysfs, "rps_max_freq_mhz", min_freq));
> > - igt_require(igt_sysfs_get_u32(sysfs, "rps_max_freq_mhz") == min_freq);
> > - igt_require(igt_sysfs_set_u32(sysfs, "rps_boost_freq_mhz", min_freq));
> > - igt_require(igt_sysfs_get_u32(sysfs, "rps_boost_freq_mhz") == min_freq);
> > + igt_require(__igt_sysfs_set_u32(sysfs, "rps_min_freq_mhz", min_freq));
> > + igt_require(__igt_sysfs_get_u32(sysfs, "rps_min_freq_mhz") == min_freq);
> > + igt_require(__igt_sysfs_set_u32(sysfs, "rps_max_freq_mhz", min_freq));
> > + igt_require(__igt_sysfs_get_u32(sysfs, "rps_max_freq_mhz") == min_freq);
> > + igt_require(__igt_sysfs_set_u32(sysfs, "rps_boost_freq_mhz", min_freq));
> > + igt_require(__igt_sysfs_get_u32(sysfs, "rps_boost_freq_mhz") == min_freq);
> >
> > gem_quiescent_gpu(gem_fd); /* Idle to be sure the change takes effect */
> > spin = spin_sync_gt(gem_fd, ahnd, gt, &ctx);
> > @@ -1834,13 +1834,13 @@ test_frequency(int gem_fd, unsigned int gt)
> > /*
> > * Set GPU to max frequency and read PMU counters.
> > */
> > - igt_require(igt_sysfs_set_u32(sysfs, "rps_max_freq_mhz", max_freq));
> > - igt_require(igt_sysfs_get_u32(sysfs, "rps_max_freq_mhz") == max_freq);
> > - igt_require(igt_sysfs_set_u32(sysfs, "rps_boost_freq_mhz", boost_freq));
> > - igt_require(igt_sysfs_get_u32(sysfs, "rps_boost_freq_mhz") == boost_freq);
> > + igt_require(__igt_sysfs_set_u32(sysfs, "rps_max_freq_mhz", max_freq));
> > + igt_require(__igt_sysfs_get_u32(sysfs, "rps_max_freq_mhz") == max_freq);
> > + igt_require(__igt_sysfs_set_u32(sysfs, "rps_boost_freq_mhz", boost_freq));
> > + igt_require(__igt_sysfs_get_u32(sysfs, "rps_boost_freq_mhz") == boost_freq);
> >
> > - igt_require(igt_sysfs_set_u32(sysfs, "rps_min_freq_mhz", max_freq));
> > - igt_require(igt_sysfs_get_u32(sysfs, "rps_min_freq_mhz") == max_freq);
> > + igt_require(__igt_sysfs_set_u32(sysfs, "rps_min_freq_mhz", max_freq));
> > + igt_require(__igt_sysfs_get_u32(sysfs, "rps_min_freq_mhz") == max_freq);
> >
> > gem_quiescent_gpu(gem_fd);
> > spin = spin_sync_gt(gem_fd, ahnd, gt, &ctx);
> > @@ -1859,10 +1859,10 @@ test_frequency(int gem_fd, unsigned int gt)
> > /*
> > * Restore min/max.
> > */
> > - igt_sysfs_set_u32(sysfs, "rps_min_freq_mhz", min_freq);
> > - if (igt_sysfs_get_u32(sysfs, "rps_min_freq_mhz") != min_freq)
> > + __igt_sysfs_set_u32(sysfs, "rps_min_freq_mhz", min_freq);
> > + if (__igt_sysfs_get_u32(sysfs, "rps_min_freq_mhz") != min_freq)
> > igt_warn("Unable to restore min frequency to saved value [%u MHz], now %u MHz\n",
> > - min_freq, igt_sysfs_get_u32(sysfs, "rps_min_freq_mhz"));
> > + min_freq, __igt_sysfs_get_u32(sysfs, "rps_min_freq_mhz"));
> > close(fd[0]);
> > close(fd[1]);
> > put_ahnd(ahnd);
> > @@ -1891,7 +1891,7 @@ test_frequency_idle(int gem_fd, unsigned int gt)
> > sysfs = igt_sysfs_gt_open(gem_fd, gt);
> > igt_require(sysfs >= 0);
> >
> > - min_freq = igt_sysfs_get_u32(sysfs, "rps_RPn_freq_mhz");
> > + min_freq = __igt_sysfs_get_u32(sysfs, "rps_RPn_freq_mhz");
> > close(sysfs);
> >
> > /* While parked, our convention is to report the GPU at 0Hz */
> > diff --git a/tests/kms_prime.c b/tests/kms_prime.c
> > index dd5ab993e..98ed30f12 100644
> > --- a/tests/kms_prime.c
> > +++ b/tests/kms_prime.c
> > @@ -341,7 +341,7 @@ static void kms_poll_state_restore(void)
> > int sysfs_fd;
> >
> > igt_assert((sysfs_fd = open(KMS_HELPER, O_RDONLY)) >= 0);
> > - igt_sysfs_set_boolean(sysfs_fd, "poll", kms_poll_saved_state);
> > + __igt_sysfs_set_boolean(sysfs_fd, "poll", kms_poll_saved_state);
> > close(sysfs_fd);
> >
> > }
> > @@ -352,7 +352,7 @@ static void kms_poll_disable(void)
> >
> > igt_require((sysfs_fd = open(KMS_HELPER, O_RDONLY)) >= 0);
> > kms_poll_saved_state = igt_sysfs_get_boolean(sysfs_fd, "poll");
> > - igt_sysfs_set_boolean(sysfs_fd, "poll", KMS_POLL_DISABLE);
> > + __igt_sysfs_set_boolean(sysfs_fd, "poll", KMS_POLL_DISABLE);
> > kms_poll_disabled = true;
> > close(sysfs_fd);
> > }
> > diff --git a/tests/nouveau_crc.c b/tests/nouveau_crc.c
> > index 785d39bde..b78a53653 100644
> > --- a/tests/nouveau_crc.c
> > +++ b/tests/nouveau_crc.c
> > @@ -269,10 +269,10 @@ static void test_ctx_flip_threshold_reset_after_capture(data_t *data)
> >
> > set_crc_flip_threshold(data, 5);
> > igt_pipe_crc_start(pipe_crc);
> > - igt_assert_eq(igt_sysfs_get_u32(data->nv_crc_dir, "flip_threshold"), 5);
> > + igt_assert_eq(__igt_sysfs_get_u32(data->nv_crc_dir, "flip_threshold"), 5);
> > igt_pipe_crc_stop(pipe_crc);
> >
> > - igt_assert_neq(igt_sysfs_get_u32(data->nv_crc_dir, "flip_threshold"), 5);
> > + igt_assert_neq(__igt_sysfs_get_u32(data->nv_crc_dir, "flip_threshold"), 5);
> > igt_pipe_crc_free(pipe_crc);
> > }
> >
> > --
> > 2.40.0
> >
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [igt-dev] [PATCH i-g-t] lib/igt_sysfs: add asserting helpers for read/write operations
2023-06-19 6:26 Łukasz Łaguna
@ 2023-06-19 12:05 ` Kamil Konieczny
2023-06-19 12:51 ` Kamil Konieczny
2023-06-19 14:08 ` Laguna, Lukasz
0 siblings, 2 replies; 7+ messages in thread
From: Kamil Konieczny @ 2023-06-19 12:05 UTC (permalink / raw)
To: igt-dev; +Cc: kamil.konieczny
Hi Łukasz,
On 2023-06-19 at 08:26:52 +0200, Łukasz Łaguna wrote:
> Prefix names of existing, non-asserting helpers with "__". Replace all
> calls to non-asserting ones in order to don't introduce any functional
--------- ^
Please write the names of functions you will be
enhancing/changing.
> changes in the existing code.
>
> Signed-off-by: Lukasz Laguna <lukasz.laguna@intel.com>
> ---
> lib/i915/gem_submission.c | 2 +-
> lib/igt_pm.c | 2 +-
> lib/igt_power.c | 2 +-
> lib/igt_sysfs.c | 210 +++++++++++++++++++++++++--------
> lib/igt_sysfs.h | 16 ++-
> tests/i915/i915_pm_dc.c | 4 +-
> tests/i915/i915_pm_freq_mult.c | 28 ++---
> tests/i915/i915_pm_rps.c | 8 +-
> tests/i915/i915_power.c | 4 +-
> tests/i915/perf_pmu.c | 38 +++---
> tests/kms_prime.c | 4 +-
> tests/nouveau_crc.c | 4 +-
> 12 files changed, 220 insertions(+), 102 deletions(-)
>
> diff --git a/lib/i915/gem_submission.c b/lib/i915/gem_submission.c
> index adf5eb394..76286129a 100644
> --- a/lib/i915/gem_submission.c
> +++ b/lib/i915/gem_submission.c
> @@ -70,7 +70,7 @@ unsigned gem_submission_method(int fd)
> if (dir < 0)
> return 0;
>
> - if (igt_sysfs_get_u32(dir, "enable_guc") & 1) {
> + if (__igt_sysfs_get_u32(dir, "enable_guc") & 1) {
This example show that __igt_sysfs_get_u32 should return 0
in case of error or here we should also check for existance
of this sysfs param.
> method = GEM_SUBMISSION_GUC;
> } else if (gen >= 8) {
> method = GEM_SUBMISSION_EXECLISTS;
> diff --git a/lib/igt_pm.c b/lib/igt_pm.c
> index 18c84bf3a..40a245e83 100644
> --- a/lib/igt_pm.c
> +++ b/lib/igt_pm.c
> @@ -1415,5 +1415,5 @@ void igt_pm_ignore_slpc_efficient_freq(int i915, int gtfd, bool val)
> return;
>
> igt_require(igt_sysfs_has_attr(gtfd, "slpc_ignore_eff_freq"));
> - igt_assert(igt_sysfs_set_u32(gtfd, "slpc_ignore_eff_freq", val));
> + igt_assert(__igt_sysfs_set_u32(gtfd, "slpc_ignore_eff_freq", val));
> }
> diff --git a/lib/igt_power.c b/lib/igt_power.c
> index 3b34be406..b94a9e09d 100644
> --- a/lib/igt_power.c
> +++ b/lib/igt_power.c
> @@ -137,7 +137,7 @@ void igt_power_get_energy(struct igt_power *power, struct power_sample *s)
>
> if (power->hwmon_fd >= 0) {
> if (igt_sysfs_has_attr(power->hwmon_fd, "energy1_input"))
> - s->energy = igt_sysfs_get_u64(power->hwmon_fd, "energy1_input");
> + s->energy = __igt_sysfs_get_u64(power->hwmon_fd, "energy1_input");
> } else if (power->rapl.fd >= 0) {
> rapl_read(&power->rapl, s);
> }
> diff --git a/lib/igt_sysfs.c b/lib/igt_sysfs.c
> index 35a4faa9a..c4d98cd53 100644
> --- a/lib/igt_sysfs.c
> +++ b/lib/igt_sysfs.c
> @@ -511,94 +511,163 @@ int igt_sysfs_printf(int dir, const char *attr, const char *fmt, ...)
> return ret;
> }
>
> +static uint32_t ___igt_sysfs_get_u32(int dir, const char *attr, bool assert)
------------------------------------------------------------------ ^^^^^^^^^^^
Do not write such functions, rather write two variants of
get_u32, for example:
no asserts here:
uint32_t __igt_sysfs_get_u32(int dir, const char *attr)
checks present here:
uint32_t __igt_sysfs_get_u32(int dir, const char *attr)
or alternativly, you could add
bool __igt_sysfs_get_u32(int dir, const char *attr, uint32_t *val)
Same note goes for u64 variant,
Regrads,
Kamil
> +{
> + uint32_t result;
> + int ret;
> +
> + ret = igt_sysfs_scanf(dir, attr, "%u", &result);
> + if (assert)
> + igt_assert(ret == 1);
> +
> + if (igt_debug_on(ret != 1))
> + return 0;
> +
> + return result;
> +}
> +
> /**
> - * igt_sysfs_get_u32:
> - * @dir: directory for the device from igt_sysfs_open()
> - * @attr: name of the sysfs node to open
> + * __igt_sysfs_get_u32:
> + * @dir: directory corresponding to attribute
> + * @attr: name of the sysfs node to read
> *
> * Convenience wrapper to read a unsigned 32bit integer from a sysfs file.
> *
> * Returns:
> * The value read.
> */
> +uint32_t __igt_sysfs_get_u32(int dir, const char *attr)
> +{
> + return ___igt_sysfs_get_u32(dir, attr, false);
> +}
> +
> +/**
> + * igt_sysfs_get_u32:
> + * @dir: directory corresponding to attribute
> + * @attr: name of the sysfs node to read
> + *
> + * Asserting equivalent of __igt_sysfs_get_u32.
> + *
> + * Returns:
> + * The value read.
> + */
> uint32_t igt_sysfs_get_u32(int dir, const char *attr)
> {
> - uint32_t result;
> + return ___igt_sysfs_get_u32(dir, attr, true);
> +}
> +
> +/**
> + * __igt_sysfs_set_u32:
> + * @dir: directory corresponding to attribute
> + * @attr: name of the sysfs node to write
> + * @value: value to set
> + *
> + * Convenience wrapper to write a unsigned 32bit integer to a sysfs file.
> + *
> + * Returns:
> + * True if successfully written, false otherwise.
> + */
> +bool __igt_sysfs_set_u32(int dir, const char *attr, uint32_t value)
> +{
> + return igt_sysfs_printf(dir, attr, "%u", value) > 0;
> +}
> +
> +/**
> + * igt_sysfs_set_u32:
> + * @dir: directory corresponding to attribute
> + * @attr: name of the sysfs node to write
> + * @value: value to set
> + *
> + * Asserting equivalent of __igt_sysfs_set_u32.
> + */
> +void igt_sysfs_set_u32(int dir, const char *attr, uint32_t value)
> +{
> + igt_assert(__igt_sysfs_set_u32(dir, attr, value));
> +}
>
> - if (igt_debug_on(igt_sysfs_scanf(dir, attr, "%u", &result) != 1))
> +static uint64_t ___igt_sysfs_get_u64(int dir, const char *attr, bool assert)
> +{
> + uint64_t result;
> + int ret;
> +
> + ret = igt_sysfs_scanf(dir, attr, "%"PRIu64, &result);
> + if (assert)
> + igt_assert(ret == 1);
> +
> + if (igt_debug_on(ret != 1))
> return 0;
>
> return result;
> }
>
> /**
> - * igt_sysfs_get_u64:
> - * @dir: directory for the device from igt_sysfs_open()
> - * @attr: name of the sysfs node to open
> + * __igt_sysfs_get_u64:
> + * @dir: directory corresponding to attribute
> + * @attr: name of the sysfs node to read
> *
> * Convenience wrapper to read a unsigned 64bit integer from a sysfs file.
> *
> * Returns:
> * The value read.
> */
> -uint64_t igt_sysfs_get_u64(int dir, const char *attr)
> +uint64_t __igt_sysfs_get_u64(int dir, const char *attr)
> {
> - uint64_t result;
> -
> - if (igt_debug_on(igt_sysfs_scanf(dir, attr, "%"PRIu64, &result) != 1))
> - return 0;
> -
> - return result;
> + return ___igt_sysfs_get_u64(dir, attr, false);
> }
>
> /**
> - * igt_sysfs_set_u64:
> - * @dir: directory for the device from igt_sysfs_open()
> - * @attr: name of the sysfs node to open
> - * @value: value to set
> + * igt_sysfs_get_u64:
> + * @dir: directory corresponding to attribute
> + * @attr: name of the sysfs node to read
> *
> - * Convenience wrapper to write a unsigned 64bit integer to a sysfs file.
> + * Asserting equivalent of __igt_sysfs_get_u64.
> *
> * Returns:
> - * True if successfully written
> + * The value read.
> */
> -bool igt_sysfs_set_u64(int dir, const char *attr, uint64_t value)
> +uint64_t igt_sysfs_get_u64(int dir, const char *attr)
> {
> - return igt_sysfs_printf(dir, attr, "%"PRIu64, value) > 0;
> + return ___igt_sysfs_get_u64(dir, attr, true);
> }
>
> /**
> - * igt_sysfs_set_u32:
> - * @dir: directory for the device from igt_sysfs_open()
> - * @attr: name of the sysfs node to open
> + * __igt_sysfs_set_u64:
> + * @dir: directory corresponding to attribute
> + * @attr: name of the sysfs node to write
> * @value: value to set
> *
> - * Convenience wrapper to write a unsigned 32bit integer to a sysfs file.
> + * Convenience wrapper to write a unsigned 64bit integer to a sysfs file.
> *
> * Returns:
> - * True if successfully written
> + * True if successfully written, false otherwise.
> */
> -bool igt_sysfs_set_u32(int dir, const char *attr, uint32_t value)
> +bool __igt_sysfs_set_u64(int dir, const char *attr, uint64_t value)
> {
> - return igt_sysfs_printf(dir, attr, "%u", value) > 0;
> + return igt_sysfs_printf(dir, attr, "%"PRIu64, value) > 0;
> }
>
> /**
> - * igt_sysfs_get_boolean:
> - * @dir: directory for the device from igt_sysfs_open()
> - * @attr: name of the sysfs node to open
> + * igt_sysfs_set_u64:
> + * @dir: directory corresponding to attribute
> + * @attr: name of the sysfs node to write
> + * @value: value to set
> *
> - * Convenience wrapper to read a boolean sysfs file.
> - *
> - * Returns:
> - * The value read.
> + * Asserting equivalent of __igt_sysfs_set_u64.
> */
> -bool igt_sysfs_get_boolean(int dir, const char *attr)
> +void igt_sysfs_set_u64(int dir, const char *attr, uint64_t value)
> +{
> + igt_assert(__igt_sysfs_set_u64(dir, attr, value));
> +}
> +
> +static bool ___igt_sysfs_get_boolean(int dir, const char *attr, bool assert)
> {
> int result;
> char *buf;
>
> buf = igt_sysfs_get(dir, attr);
> + if (assert)
> + igt_assert(buf);
> +
> if (igt_debug_on(!buf))
> return false;
>
> @@ -612,21 +681,64 @@ bool igt_sysfs_get_boolean(int dir, const char *attr)
> }
>
> /**
> - * igt_sysfs_set_boolean:
> - * @dir: directory for the device from igt_sysfs_open()
> - * @attr: name of the sysfs node to open
> + * __igt_sysfs_get_boolean:
> + * @dir: directory corresponding to attribute
> + * @attr: name of the sysfs node to read
> + *
> + * Convenience wrapper to read a boolean sysfs file.
> + *
> + * Returns:
> + * The value read.
> + */
> +bool __igt_sysfs_get_boolean(int dir, const char *attr)
> +{
> + return ___igt_sysfs_get_boolean(dir, attr, false);
> +}
> +
> +/**
> + * igt_sysfs_get_boolean:
> + * @dir: directory corresponding to attribute
> + * @attr: name of the sysfs node to read
> + *
> + * Asserting equivalent of __igt_sysfs_get_boolean.
> + *
> + * Returns:
> + * The value read.
> + */
> +bool igt_sysfs_get_boolean(int dir, const char *attr)
> +{
> + return ___igt_sysfs_get_boolean(dir, attr, true);
> +}
> +
> +/**
> + * __igt_sysfs_set_boolean:
> + * @dir: directory corresponding to attribute
> + * @attr: name of the sysfs node to write
> * @value: value to set
> *
> * Convenience wrapper to write a boolean sysfs file.
> - *
> + *
> * Returns:
> - * The value read.
> + * True if successfully written, false otherwise.
> */
> -bool igt_sysfs_set_boolean(int dir, const char *attr, bool value)
> +bool __igt_sysfs_set_boolean(int dir, const char *attr, bool value)
> {
> return igt_sysfs_printf(dir, attr, "%d", value) == 1;
> }
>
> +/**
> + * igt_sysfs_set_boolean:
> + * @dir: directory corresponding to attribute
> + * @attr: name of the sysfs node to write
> + * @value: value to set
> + *
> + * Asserting equivalent of __igt_sysfs_set_boolean.
> + */
> +void igt_sysfs_set_boolean(int dir, const char *attr, bool value)
> +{
> + igt_assert(__igt_sysfs_set_boolean(dir, attr, value));
> +}
> +
> static void bind_con(const char *name, bool enable)
> {
> const char *path = "/sys/class/vtconsole";
> @@ -797,8 +909,8 @@ static int rw_attr_sweep(igt_sysfs_rw_attr_t *rw)
>
> igt_debug("'%s': sweeping range of values\n", rw->attr);
> while (set < UINT64_MAX / 2) {
> - ret = igt_sysfs_set_u64(rw->dir, rw->attr, set);
> - get = igt_sysfs_get_u64(rw->dir, rw->attr);
> + ret = __igt_sysfs_set_u64(rw->dir, rw->attr, set);
> + get = __igt_sysfs_get_u64(rw->dir, rw->attr);
> igt_debug("'%s': ret %d set %lu get %lu\n", rw->attr, ret, set, get);
> if (ret && rw_attr_equal_within_epsilon(get, set, rw->tol)) {
> igt_debug("'%s': matches\n", rw->attr);
> @@ -842,7 +954,7 @@ void igt_sysfs_rw_attr_verify(igt_sysfs_rw_attr_t *rw)
> igt_assert(st.st_mode & 0222); /* writable */
> igt_assert(rw->start); /* cannot be 0 */
>
> - prev = igt_sysfs_get_u64(rw->dir, rw->attr);
> + prev = __igt_sysfs_get_u64(rw->dir, rw->attr);
> igt_debug("'%s': prev %lu\n", rw->attr, prev);
>
> ret = rw_attr_sweep(rw);
> @@ -851,8 +963,8 @@ void igt_sysfs_rw_attr_verify(igt_sysfs_rw_attr_t *rw)
> * Restore previous value: we don't assert before this point so
> * that we can restore the attr before asserting
> */
> - igt_assert_eq(1, igt_sysfs_set_u64(rw->dir, rw->attr, prev));
> - get = igt_sysfs_get_u64(rw->dir, rw->attr);
> + igt_assert_eq(1, __igt_sysfs_set_u64(rw->dir, rw->attr, prev));
> + get = __igt_sysfs_get_u64(rw->dir, rw->attr);
> igt_assert_eq(get, prev);
> igt_assert(!ret);
> }
> diff --git a/lib/igt_sysfs.h b/lib/igt_sysfs.h
> index 978b6906e..118137d5e 100644
> --- a/lib/igt_sysfs.h
> +++ b/lib/igt_sysfs.h
> @@ -62,10 +62,10 @@
> igt_sysfs_printf(dir, igt_sysfs_dir_id_to_name(dir, id), fmt, ##__VA_ARGS__)
>
> #define igt_sysfs_rps_get_u32(dir, id) \
> - igt_sysfs_get_u32(dir, igt_sysfs_dir_id_to_name(dir, id))
> + __igt_sysfs_get_u32(dir, igt_sysfs_dir_id_to_name(dir, id))
>
> #define igt_sysfs_rps_set_u32(dir, id, value) \
> - igt_sysfs_set_u32(dir, igt_sysfs_dir_id_to_name(dir, id), value)
> + __igt_sysfs_set_u32(dir, igt_sysfs_dir_id_to_name(dir, id), value)
>
> #define igt_sysfs_rps_get_boolean(dir, id) \
> igt_sysfs_get_boolean(dir, igt_sysfs_dir_id_to_name(dir, id))
> @@ -114,14 +114,20 @@ int igt_sysfs_vprintf(int dir, const char *attr, const char *fmt, va_list ap)
> int igt_sysfs_printf(int dir, const char *attr, const char *fmt, ...)
> __attribute__((format(printf,3,4)));
>
> +uint32_t __igt_sysfs_get_u32(int dir, const char *attr);
> uint32_t igt_sysfs_get_u32(int dir, const char *attr);
> -bool igt_sysfs_set_u32(int dir, const char *attr, uint32_t value);
> +bool __igt_sysfs_set_u32(int dir, const char *attr, uint32_t value);
> +void igt_sysfs_set_u32(int dir, const char *attr, uint32_t value);
>
> +uint64_t __igt_sysfs_get_u64(int dir, const char *attr);
> uint64_t igt_sysfs_get_u64(int dir, const char *attr);
> -bool igt_sysfs_set_u64(int dir, const char *attr, uint64_t value);
> +bool __igt_sysfs_set_u64(int dir, const char *attr, uint64_t value);
> +void igt_sysfs_set_u64(int dir, const char *attr, uint64_t value);
>
> +bool __igt_sysfs_get_boolean(int dir, const char *attr);
> bool igt_sysfs_get_boolean(int dir, const char *attr);
> -bool igt_sysfs_set_boolean(int dir, const char *attr, bool value);
> +bool __igt_sysfs_set_boolean(int dir, const char *attr, bool value);
> +void igt_sysfs_set_boolean(int dir, const char *attr, bool value);
>
> void bind_fbcon(bool enable);
> void kick_snd_hda_intel(void);
> diff --git a/tests/i915/i915_pm_dc.c b/tests/i915/i915_pm_dc.c
> index 2bb07ba8b..734bcfa8b 100644
> --- a/tests/i915/i915_pm_dc.c
> +++ b/tests/i915/i915_pm_dc.c
> @@ -534,7 +534,7 @@ static void setup_dc9_dpms(data_t *data, int dc_target)
>
> igt_require((sysfs_fd = open(KMS_HELPER, O_RDONLY)) >= 0);
> kms_poll_saved_state = igt_sysfs_get_boolean(sysfs_fd, "poll");
> - igt_sysfs_set_boolean(sysfs_fd, "poll", KMS_POLL_DISABLE);
> + __igt_sysfs_set_boolean(sysfs_fd, "poll", KMS_POLL_DISABLE);
> close(sysfs_fd);
> if (DC9_RESETS_DC_COUNTERS(data->devid)) {
> prev_dc = read_dc_counter(data->debugfs_fd, dc_target);
> @@ -629,7 +629,7 @@ static void kms_poll_state_restore(int sig)
>
> sysfs_fd = open(KMS_HELPER, O_RDONLY);
> if (sysfs_fd >= 0) {
> - igt_sysfs_set_boolean(sysfs_fd, "poll", kms_poll_saved_state);
> + __igt_sysfs_set_boolean(sysfs_fd, "poll", kms_poll_saved_state);
> close(sysfs_fd);
> }
> }
> diff --git a/tests/i915/i915_pm_freq_mult.c b/tests/i915/i915_pm_freq_mult.c
> index d75ec3f9e..82a578f8e 100644
> --- a/tests/i915/i915_pm_freq_mult.c
> +++ b/tests/i915/i915_pm_freq_mult.c
> @@ -57,11 +57,11 @@ static void restore_rps_defaults(int dir)
> if (def < 0)
> return;
>
> - max = igt_sysfs_get_u32(def, "rps_max_freq_mhz");
> - igt_sysfs_set_u32(dir, "rps_max_freq_mhz", max);
> + max = __igt_sysfs_get_u32(def, "rps_max_freq_mhz");
> + __igt_sysfs_set_u32(dir, "rps_max_freq_mhz", max);
>
> - min = igt_sysfs_get_u32(def, "rps_min_freq_mhz");
> - igt_sysfs_set_u32(dir, "rps_min_freq_mhz", min);
> + min = __igt_sysfs_get_u32(def, "rps_min_freq_mhz");
> + __igt_sysfs_set_u32(dir, "rps_min_freq_mhz", min);
>
> close(def);
> }
> @@ -81,17 +81,17 @@ static void setup_freq(int gt, int dir)
> wait_freq_set();
>
> /* Print some debug information */
> - rp0 = igt_sysfs_get_u32(dir, "rps_RP0_freq_mhz");
> - rp1 = igt_sysfs_get_u32(dir, "rps_RP1_freq_mhz");
> - rpn = igt_sysfs_get_u32(dir, "rps_RPn_freq_mhz");
> - min = igt_sysfs_get_u32(dir, "rps_min_freq_mhz");
> - max = igt_sysfs_get_u32(dir, "rps_max_freq_mhz");
> - act = igt_sysfs_get_u32(dir, "rps_act_freq_mhz");
> + rp0 = __igt_sysfs_get_u32(dir, "rps_RP0_freq_mhz");
> + rp1 = __igt_sysfs_get_u32(dir, "rps_RP1_freq_mhz");
> + rpn = __igt_sysfs_get_u32(dir, "rps_RPn_freq_mhz");
> + min = __igt_sysfs_get_u32(dir, "rps_min_freq_mhz");
> + max = __igt_sysfs_get_u32(dir, "rps_max_freq_mhz");
> + act = __igt_sysfs_get_u32(dir, "rps_act_freq_mhz");
>
> igt_debug("RP0 MHz: %d, RP1 MHz: %d, RPn MHz: %d, min MHz: %d, max MHz: %d, act MHz: %d\n", rp0, rp1, rpn, min, max, act);
>
> if (igt_sysfs_has_attr(dir, "media_freq_factor")) {
> - media = igt_sysfs_get_u32(dir, "media_freq_factor");
> + media = __igt_sysfs_get_u32(dir, "media_freq_factor");
> igt_debug("media ratio: %.2f\n", media * FREQ_SCALE_FACTOR);
> }
> }
> @@ -117,8 +117,8 @@ static void media_freq(int gt, int dir)
> setup_freq(gt, dir);
>
> igt_debug("media RP0 mhz: %d, media RPn mhz: %d\n",
> - igt_sysfs_get_u32(dir, "media_RP0_freq_mhz"),
> - igt_sysfs_get_u32(dir, "media_RPn_freq_mhz"));
> + __igt_sysfs_get_u32(dir, "media_RP0_freq_mhz"),
> + __igt_sysfs_get_u32(dir, "media_RPn_freq_mhz"));
> igt_debug("media ratio value 0.0 represents dynamic mode\n");
>
> /*
> @@ -141,7 +141,7 @@ static void media_freq(int gt, int dir)
>
> wait_freq_set();
>
> - getv = igt_sysfs_get_u32(dir, "media_freq_factor");
> + getv = __igt_sysfs_get_u32(dir, "media_freq_factor");
>
> igt_debug("media ratio set: %.2f, media ratio get: %.2f\n",
> v * scale, getv * scale);
> diff --git a/tests/i915/i915_pm_rps.c b/tests/i915/i915_pm_rps.c
> index eaacc7c90..5ae91ffd2 100644
> --- a/tests/i915/i915_pm_rps.c
> +++ b/tests/i915/i915_pm_rps.c
> @@ -742,8 +742,8 @@ static void fence_order(int i915)
> */
>
> sysfs = igt_sysfs_open(i915);
> - min = igt_sysfs_get_u32(sysfs, "gt_RPn_freq_mhz");
> - max = igt_sysfs_get_u32(sysfs, "gt_RP0_freq_mhz");
> + min = __igt_sysfs_get_u32(sysfs, "gt_RPn_freq_mhz");
> + max = __igt_sysfs_get_u32(sysfs, "gt_RP0_freq_mhz");
> igt_require(max > min);
>
> /* Only allow ourselves to upclock via waitboosting */
> @@ -862,8 +862,8 @@ static void engine_order(int i915)
> execbuf.rsvd1 = ctx->id;
>
> sysfs = igt_sysfs_open(i915);
> - min = igt_sysfs_get_u32(sysfs, "gt_RPn_freq_mhz");
> - max = igt_sysfs_get_u32(sysfs, "gt_RP0_freq_mhz");
> + min = __igt_sysfs_get_u32(sysfs, "gt_RPn_freq_mhz");
> + max = __igt_sysfs_get_u32(sysfs, "gt_RP0_freq_mhz");
> igt_require(max > min);
>
> /* Only allow ourselves to upclock via waitboosting */
> diff --git a/tests/i915/i915_power.c b/tests/i915/i915_power.c
> index 3675b9d6d..86b0fd08c 100644
> --- a/tests/i915/i915_power.c
> +++ b/tests/i915/i915_power.c
> @@ -58,8 +58,8 @@ static void sanity(int i915)
> igt_spin_busywait_until_started(spin);
> busy = measure_power(&pwr, DURATION_SEC);
> i915_for_each_gt(i915, dir, gt) {
> - req = igt_sysfs_get_u32(dir, "rps_cur_freq_mhz");
> - act = igt_sysfs_get_u32(dir, "rps_act_freq_mhz");
> + req = __igt_sysfs_get_u32(dir, "rps_cur_freq_mhz");
> + act = __igt_sysfs_get_u32(dir, "rps_act_freq_mhz");
> igt_info("gt %d: req MHz: %d, act MHz: %d\n", gt, req, act);
> }
> igt_free_spins(i915);
> diff --git a/tests/i915/perf_pmu.c b/tests/i915/perf_pmu.c
> index 8b31df7b2..1a5595d67 100644
> --- a/tests/i915/perf_pmu.c
> +++ b/tests/i915/perf_pmu.c
> @@ -1793,9 +1793,9 @@ test_frequency(int gem_fd, unsigned int gt)
> sysfs = igt_sysfs_gt_open(gem_fd, gt);
> igt_require(sysfs >= 0);
>
> - min_freq = igt_sysfs_get_u32(sysfs, "rps_RPn_freq_mhz");
> - max_freq = igt_sysfs_get_u32(sysfs, "rps_RP0_freq_mhz");
> - boost_freq = igt_sysfs_get_u32(sysfs, "rps_boost_freq_mhz");
> + min_freq = __igt_sysfs_get_u32(sysfs, "rps_RPn_freq_mhz");
> + max_freq = __igt_sysfs_get_u32(sysfs, "rps_RP0_freq_mhz");
> + boost_freq = __igt_sysfs_get_u32(sysfs, "rps_boost_freq_mhz");
> igt_info("Frequency: min=%u, max=%u, boost=%u MHz\n",
> min_freq, max_freq, boost_freq);
> igt_require(min_freq > 0 && max_freq > 0 && boost_freq > 0);
> @@ -1808,12 +1808,12 @@ test_frequency(int gem_fd, unsigned int gt)
> /*
> * Set GPU to min frequency and read PMU counters.
> */
> - igt_require(igt_sysfs_set_u32(sysfs, "rps_min_freq_mhz", min_freq));
> - igt_require(igt_sysfs_get_u32(sysfs, "rps_min_freq_mhz") == min_freq);
> - igt_require(igt_sysfs_set_u32(sysfs, "rps_max_freq_mhz", min_freq));
> - igt_require(igt_sysfs_get_u32(sysfs, "rps_max_freq_mhz") == min_freq);
> - igt_require(igt_sysfs_set_u32(sysfs, "rps_boost_freq_mhz", min_freq));
> - igt_require(igt_sysfs_get_u32(sysfs, "rps_boost_freq_mhz") == min_freq);
> + igt_require(__igt_sysfs_set_u32(sysfs, "rps_min_freq_mhz", min_freq));
> + igt_require(__igt_sysfs_get_u32(sysfs, "rps_min_freq_mhz") == min_freq);
> + igt_require(__igt_sysfs_set_u32(sysfs, "rps_max_freq_mhz", min_freq));
> + igt_require(__igt_sysfs_get_u32(sysfs, "rps_max_freq_mhz") == min_freq);
> + igt_require(__igt_sysfs_set_u32(sysfs, "rps_boost_freq_mhz", min_freq));
> + igt_require(__igt_sysfs_get_u32(sysfs, "rps_boost_freq_mhz") == min_freq);
>
> gem_quiescent_gpu(gem_fd); /* Idle to be sure the change takes effect */
> spin = spin_sync_gt(gem_fd, ahnd, gt, &ctx);
> @@ -1834,13 +1834,13 @@ test_frequency(int gem_fd, unsigned int gt)
> /*
> * Set GPU to max frequency and read PMU counters.
> */
> - igt_require(igt_sysfs_set_u32(sysfs, "rps_max_freq_mhz", max_freq));
> - igt_require(igt_sysfs_get_u32(sysfs, "rps_max_freq_mhz") == max_freq);
> - igt_require(igt_sysfs_set_u32(sysfs, "rps_boost_freq_mhz", boost_freq));
> - igt_require(igt_sysfs_get_u32(sysfs, "rps_boost_freq_mhz") == boost_freq);
> + igt_require(__igt_sysfs_set_u32(sysfs, "rps_max_freq_mhz", max_freq));
> + igt_require(__igt_sysfs_get_u32(sysfs, "rps_max_freq_mhz") == max_freq);
> + igt_require(__igt_sysfs_set_u32(sysfs, "rps_boost_freq_mhz", boost_freq));
> + igt_require(__igt_sysfs_get_u32(sysfs, "rps_boost_freq_mhz") == boost_freq);
>
> - igt_require(igt_sysfs_set_u32(sysfs, "rps_min_freq_mhz", max_freq));
> - igt_require(igt_sysfs_get_u32(sysfs, "rps_min_freq_mhz") == max_freq);
> + igt_require(__igt_sysfs_set_u32(sysfs, "rps_min_freq_mhz", max_freq));
> + igt_require(__igt_sysfs_get_u32(sysfs, "rps_min_freq_mhz") == max_freq);
>
> gem_quiescent_gpu(gem_fd);
> spin = spin_sync_gt(gem_fd, ahnd, gt, &ctx);
> @@ -1859,10 +1859,10 @@ test_frequency(int gem_fd, unsigned int gt)
> /*
> * Restore min/max.
> */
> - igt_sysfs_set_u32(sysfs, "rps_min_freq_mhz", min_freq);
> - if (igt_sysfs_get_u32(sysfs, "rps_min_freq_mhz") != min_freq)
> + __igt_sysfs_set_u32(sysfs, "rps_min_freq_mhz", min_freq);
> + if (__igt_sysfs_get_u32(sysfs, "rps_min_freq_mhz") != min_freq)
> igt_warn("Unable to restore min frequency to saved value [%u MHz], now %u MHz\n",
> - min_freq, igt_sysfs_get_u32(sysfs, "rps_min_freq_mhz"));
> + min_freq, __igt_sysfs_get_u32(sysfs, "rps_min_freq_mhz"));
> close(fd[0]);
> close(fd[1]);
> put_ahnd(ahnd);
> @@ -1891,7 +1891,7 @@ test_frequency_idle(int gem_fd, unsigned int gt)
> sysfs = igt_sysfs_gt_open(gem_fd, gt);
> igt_require(sysfs >= 0);
>
> - min_freq = igt_sysfs_get_u32(sysfs, "rps_RPn_freq_mhz");
> + min_freq = __igt_sysfs_get_u32(sysfs, "rps_RPn_freq_mhz");
> close(sysfs);
>
> /* While parked, our convention is to report the GPU at 0Hz */
> diff --git a/tests/kms_prime.c b/tests/kms_prime.c
> index dd5ab993e..98ed30f12 100644
> --- a/tests/kms_prime.c
> +++ b/tests/kms_prime.c
> @@ -341,7 +341,7 @@ static void kms_poll_state_restore(void)
> int sysfs_fd;
>
> igt_assert((sysfs_fd = open(KMS_HELPER, O_RDONLY)) >= 0);
> - igt_sysfs_set_boolean(sysfs_fd, "poll", kms_poll_saved_state);
> + __igt_sysfs_set_boolean(sysfs_fd, "poll", kms_poll_saved_state);
> close(sysfs_fd);
>
> }
> @@ -352,7 +352,7 @@ static void kms_poll_disable(void)
>
> igt_require((sysfs_fd = open(KMS_HELPER, O_RDONLY)) >= 0);
> kms_poll_saved_state = igt_sysfs_get_boolean(sysfs_fd, "poll");
> - igt_sysfs_set_boolean(sysfs_fd, "poll", KMS_POLL_DISABLE);
> + __igt_sysfs_set_boolean(sysfs_fd, "poll", KMS_POLL_DISABLE);
> kms_poll_disabled = true;
> close(sysfs_fd);
> }
> diff --git a/tests/nouveau_crc.c b/tests/nouveau_crc.c
> index 785d39bde..b78a53653 100644
> --- a/tests/nouveau_crc.c
> +++ b/tests/nouveau_crc.c
> @@ -269,10 +269,10 @@ static void test_ctx_flip_threshold_reset_after_capture(data_t *data)
>
> set_crc_flip_threshold(data, 5);
> igt_pipe_crc_start(pipe_crc);
> - igt_assert_eq(igt_sysfs_get_u32(data->nv_crc_dir, "flip_threshold"), 5);
> + igt_assert_eq(__igt_sysfs_get_u32(data->nv_crc_dir, "flip_threshold"), 5);
> igt_pipe_crc_stop(pipe_crc);
>
> - igt_assert_neq(igt_sysfs_get_u32(data->nv_crc_dir, "flip_threshold"), 5);
> + igt_assert_neq(__igt_sysfs_get_u32(data->nv_crc_dir, "flip_threshold"), 5);
> igt_pipe_crc_free(pipe_crc);
> }
>
> --
> 2.40.0
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* [igt-dev] [PATCH i-g-t] lib/igt_sysfs: add asserting helpers for read/write operations
@ 2023-06-19 6:26 Łukasz Łaguna
2023-06-19 12:05 ` Kamil Konieczny
0 siblings, 1 reply; 7+ messages in thread
From: Łukasz Łaguna @ 2023-06-19 6:26 UTC (permalink / raw)
To: igt-dev; +Cc: kamil.konieczny
Prefix names of existing, non-asserting helpers with "__". Replace all
calls to non-asserting ones in order to don't introduce any functional
changes in the existing code.
Signed-off-by: Lukasz Laguna <lukasz.laguna@intel.com>
---
lib/i915/gem_submission.c | 2 +-
lib/igt_pm.c | 2 +-
lib/igt_power.c | 2 +-
lib/igt_sysfs.c | 210 +++++++++++++++++++++++++--------
lib/igt_sysfs.h | 16 ++-
tests/i915/i915_pm_dc.c | 4 +-
tests/i915/i915_pm_freq_mult.c | 28 ++---
tests/i915/i915_pm_rps.c | 8 +-
tests/i915/i915_power.c | 4 +-
tests/i915/perf_pmu.c | 38 +++---
tests/kms_prime.c | 4 +-
tests/nouveau_crc.c | 4 +-
12 files changed, 220 insertions(+), 102 deletions(-)
diff --git a/lib/i915/gem_submission.c b/lib/i915/gem_submission.c
index adf5eb394..76286129a 100644
--- a/lib/i915/gem_submission.c
+++ b/lib/i915/gem_submission.c
@@ -70,7 +70,7 @@ unsigned gem_submission_method(int fd)
if (dir < 0)
return 0;
- if (igt_sysfs_get_u32(dir, "enable_guc") & 1) {
+ if (__igt_sysfs_get_u32(dir, "enable_guc") & 1) {
method = GEM_SUBMISSION_GUC;
} else if (gen >= 8) {
method = GEM_SUBMISSION_EXECLISTS;
diff --git a/lib/igt_pm.c b/lib/igt_pm.c
index 18c84bf3a..40a245e83 100644
--- a/lib/igt_pm.c
+++ b/lib/igt_pm.c
@@ -1415,5 +1415,5 @@ void igt_pm_ignore_slpc_efficient_freq(int i915, int gtfd, bool val)
return;
igt_require(igt_sysfs_has_attr(gtfd, "slpc_ignore_eff_freq"));
- igt_assert(igt_sysfs_set_u32(gtfd, "slpc_ignore_eff_freq", val));
+ igt_assert(__igt_sysfs_set_u32(gtfd, "slpc_ignore_eff_freq", val));
}
diff --git a/lib/igt_power.c b/lib/igt_power.c
index 3b34be406..b94a9e09d 100644
--- a/lib/igt_power.c
+++ b/lib/igt_power.c
@@ -137,7 +137,7 @@ void igt_power_get_energy(struct igt_power *power, struct power_sample *s)
if (power->hwmon_fd >= 0) {
if (igt_sysfs_has_attr(power->hwmon_fd, "energy1_input"))
- s->energy = igt_sysfs_get_u64(power->hwmon_fd, "energy1_input");
+ s->energy = __igt_sysfs_get_u64(power->hwmon_fd, "energy1_input");
} else if (power->rapl.fd >= 0) {
rapl_read(&power->rapl, s);
}
diff --git a/lib/igt_sysfs.c b/lib/igt_sysfs.c
index 35a4faa9a..c4d98cd53 100644
--- a/lib/igt_sysfs.c
+++ b/lib/igt_sysfs.c
@@ -511,94 +511,163 @@ int igt_sysfs_printf(int dir, const char *attr, const char *fmt, ...)
return ret;
}
+static uint32_t ___igt_sysfs_get_u32(int dir, const char *attr, bool assert)
+{
+ uint32_t result;
+ int ret;
+
+ ret = igt_sysfs_scanf(dir, attr, "%u", &result);
+ if (assert)
+ igt_assert(ret == 1);
+
+ if (igt_debug_on(ret != 1))
+ return 0;
+
+ return result;
+}
+
/**
- * igt_sysfs_get_u32:
- * @dir: directory for the device from igt_sysfs_open()
- * @attr: name of the sysfs node to open
+ * __igt_sysfs_get_u32:
+ * @dir: directory corresponding to attribute
+ * @attr: name of the sysfs node to read
*
* Convenience wrapper to read a unsigned 32bit integer from a sysfs file.
*
* Returns:
* The value read.
*/
+uint32_t __igt_sysfs_get_u32(int dir, const char *attr)
+{
+ return ___igt_sysfs_get_u32(dir, attr, false);
+}
+
+/**
+ * igt_sysfs_get_u32:
+ * @dir: directory corresponding to attribute
+ * @attr: name of the sysfs node to read
+ *
+ * Asserting equivalent of __igt_sysfs_get_u32.
+ *
+ * Returns:
+ * The value read.
+ */
uint32_t igt_sysfs_get_u32(int dir, const char *attr)
{
- uint32_t result;
+ return ___igt_sysfs_get_u32(dir, attr, true);
+}
+
+/**
+ * __igt_sysfs_set_u32:
+ * @dir: directory corresponding to attribute
+ * @attr: name of the sysfs node to write
+ * @value: value to set
+ *
+ * Convenience wrapper to write a unsigned 32bit integer to a sysfs file.
+ *
+ * Returns:
+ * True if successfully written, false otherwise.
+ */
+bool __igt_sysfs_set_u32(int dir, const char *attr, uint32_t value)
+{
+ return igt_sysfs_printf(dir, attr, "%u", value) > 0;
+}
+
+/**
+ * igt_sysfs_set_u32:
+ * @dir: directory corresponding to attribute
+ * @attr: name of the sysfs node to write
+ * @value: value to set
+ *
+ * Asserting equivalent of __igt_sysfs_set_u32.
+ */
+void igt_sysfs_set_u32(int dir, const char *attr, uint32_t value)
+{
+ igt_assert(__igt_sysfs_set_u32(dir, attr, value));
+}
- if (igt_debug_on(igt_sysfs_scanf(dir, attr, "%u", &result) != 1))
+static uint64_t ___igt_sysfs_get_u64(int dir, const char *attr, bool assert)
+{
+ uint64_t result;
+ int ret;
+
+ ret = igt_sysfs_scanf(dir, attr, "%"PRIu64, &result);
+ if (assert)
+ igt_assert(ret == 1);
+
+ if (igt_debug_on(ret != 1))
return 0;
return result;
}
/**
- * igt_sysfs_get_u64:
- * @dir: directory for the device from igt_sysfs_open()
- * @attr: name of the sysfs node to open
+ * __igt_sysfs_get_u64:
+ * @dir: directory corresponding to attribute
+ * @attr: name of the sysfs node to read
*
* Convenience wrapper to read a unsigned 64bit integer from a sysfs file.
*
* Returns:
* The value read.
*/
-uint64_t igt_sysfs_get_u64(int dir, const char *attr)
+uint64_t __igt_sysfs_get_u64(int dir, const char *attr)
{
- uint64_t result;
-
- if (igt_debug_on(igt_sysfs_scanf(dir, attr, "%"PRIu64, &result) != 1))
- return 0;
-
- return result;
+ return ___igt_sysfs_get_u64(dir, attr, false);
}
/**
- * igt_sysfs_set_u64:
- * @dir: directory for the device from igt_sysfs_open()
- * @attr: name of the sysfs node to open
- * @value: value to set
+ * igt_sysfs_get_u64:
+ * @dir: directory corresponding to attribute
+ * @attr: name of the sysfs node to read
*
- * Convenience wrapper to write a unsigned 64bit integer to a sysfs file.
+ * Asserting equivalent of __igt_sysfs_get_u64.
*
* Returns:
- * True if successfully written
+ * The value read.
*/
-bool igt_sysfs_set_u64(int dir, const char *attr, uint64_t value)
+uint64_t igt_sysfs_get_u64(int dir, const char *attr)
{
- return igt_sysfs_printf(dir, attr, "%"PRIu64, value) > 0;
+ return ___igt_sysfs_get_u64(dir, attr, true);
}
/**
- * igt_sysfs_set_u32:
- * @dir: directory for the device from igt_sysfs_open()
- * @attr: name of the sysfs node to open
+ * __igt_sysfs_set_u64:
+ * @dir: directory corresponding to attribute
+ * @attr: name of the sysfs node to write
* @value: value to set
*
- * Convenience wrapper to write a unsigned 32bit integer to a sysfs file.
+ * Convenience wrapper to write a unsigned 64bit integer to a sysfs file.
*
* Returns:
- * True if successfully written
+ * True if successfully written, false otherwise.
*/
-bool igt_sysfs_set_u32(int dir, const char *attr, uint32_t value)
+bool __igt_sysfs_set_u64(int dir, const char *attr, uint64_t value)
{
- return igt_sysfs_printf(dir, attr, "%u", value) > 0;
+ return igt_sysfs_printf(dir, attr, "%"PRIu64, value) > 0;
}
/**
- * igt_sysfs_get_boolean:
- * @dir: directory for the device from igt_sysfs_open()
- * @attr: name of the sysfs node to open
+ * igt_sysfs_set_u64:
+ * @dir: directory corresponding to attribute
+ * @attr: name of the sysfs node to write
+ * @value: value to set
*
- * Convenience wrapper to read a boolean sysfs file.
- *
- * Returns:
- * The value read.
+ * Asserting equivalent of __igt_sysfs_set_u64.
*/
-bool igt_sysfs_get_boolean(int dir, const char *attr)
+void igt_sysfs_set_u64(int dir, const char *attr, uint64_t value)
+{
+ igt_assert(__igt_sysfs_set_u64(dir, attr, value));
+}
+
+static bool ___igt_sysfs_get_boolean(int dir, const char *attr, bool assert)
{
int result;
char *buf;
buf = igt_sysfs_get(dir, attr);
+ if (assert)
+ igt_assert(buf);
+
if (igt_debug_on(!buf))
return false;
@@ -612,21 +681,64 @@ bool igt_sysfs_get_boolean(int dir, const char *attr)
}
/**
- * igt_sysfs_set_boolean:
- * @dir: directory for the device from igt_sysfs_open()
- * @attr: name of the sysfs node to open
+ * __igt_sysfs_get_boolean:
+ * @dir: directory corresponding to attribute
+ * @attr: name of the sysfs node to read
+ *
+ * Convenience wrapper to read a boolean sysfs file.
+ *
+ * Returns:
+ * The value read.
+ */
+bool __igt_sysfs_get_boolean(int dir, const char *attr)
+{
+ return ___igt_sysfs_get_boolean(dir, attr, false);
+}
+
+/**
+ * igt_sysfs_get_boolean:
+ * @dir: directory corresponding to attribute
+ * @attr: name of the sysfs node to read
+ *
+ * Asserting equivalent of __igt_sysfs_get_boolean.
+ *
+ * Returns:
+ * The value read.
+ */
+bool igt_sysfs_get_boolean(int dir, const char *attr)
+{
+ return ___igt_sysfs_get_boolean(dir, attr, true);
+}
+
+/**
+ * __igt_sysfs_set_boolean:
+ * @dir: directory corresponding to attribute
+ * @attr: name of the sysfs node to write
* @value: value to set
*
* Convenience wrapper to write a boolean sysfs file.
- *
+ *
* Returns:
- * The value read.
+ * True if successfully written, false otherwise.
*/
-bool igt_sysfs_set_boolean(int dir, const char *attr, bool value)
+bool __igt_sysfs_set_boolean(int dir, const char *attr, bool value)
{
return igt_sysfs_printf(dir, attr, "%d", value) == 1;
}
+/**
+ * igt_sysfs_set_boolean:
+ * @dir: directory corresponding to attribute
+ * @attr: name of the sysfs node to write
+ * @value: value to set
+ *
+ * Asserting equivalent of __igt_sysfs_set_boolean.
+ */
+void igt_sysfs_set_boolean(int dir, const char *attr, bool value)
+{
+ igt_assert(__igt_sysfs_set_boolean(dir, attr, value));
+}
+
static void bind_con(const char *name, bool enable)
{
const char *path = "/sys/class/vtconsole";
@@ -797,8 +909,8 @@ static int rw_attr_sweep(igt_sysfs_rw_attr_t *rw)
igt_debug("'%s': sweeping range of values\n", rw->attr);
while (set < UINT64_MAX / 2) {
- ret = igt_sysfs_set_u64(rw->dir, rw->attr, set);
- get = igt_sysfs_get_u64(rw->dir, rw->attr);
+ ret = __igt_sysfs_set_u64(rw->dir, rw->attr, set);
+ get = __igt_sysfs_get_u64(rw->dir, rw->attr);
igt_debug("'%s': ret %d set %lu get %lu\n", rw->attr, ret, set, get);
if (ret && rw_attr_equal_within_epsilon(get, set, rw->tol)) {
igt_debug("'%s': matches\n", rw->attr);
@@ -842,7 +954,7 @@ void igt_sysfs_rw_attr_verify(igt_sysfs_rw_attr_t *rw)
igt_assert(st.st_mode & 0222); /* writable */
igt_assert(rw->start); /* cannot be 0 */
- prev = igt_sysfs_get_u64(rw->dir, rw->attr);
+ prev = __igt_sysfs_get_u64(rw->dir, rw->attr);
igt_debug("'%s': prev %lu\n", rw->attr, prev);
ret = rw_attr_sweep(rw);
@@ -851,8 +963,8 @@ void igt_sysfs_rw_attr_verify(igt_sysfs_rw_attr_t *rw)
* Restore previous value: we don't assert before this point so
* that we can restore the attr before asserting
*/
- igt_assert_eq(1, igt_sysfs_set_u64(rw->dir, rw->attr, prev));
- get = igt_sysfs_get_u64(rw->dir, rw->attr);
+ igt_assert_eq(1, __igt_sysfs_set_u64(rw->dir, rw->attr, prev));
+ get = __igt_sysfs_get_u64(rw->dir, rw->attr);
igt_assert_eq(get, prev);
igt_assert(!ret);
}
diff --git a/lib/igt_sysfs.h b/lib/igt_sysfs.h
index 978b6906e..118137d5e 100644
--- a/lib/igt_sysfs.h
+++ b/lib/igt_sysfs.h
@@ -62,10 +62,10 @@
igt_sysfs_printf(dir, igt_sysfs_dir_id_to_name(dir, id), fmt, ##__VA_ARGS__)
#define igt_sysfs_rps_get_u32(dir, id) \
- igt_sysfs_get_u32(dir, igt_sysfs_dir_id_to_name(dir, id))
+ __igt_sysfs_get_u32(dir, igt_sysfs_dir_id_to_name(dir, id))
#define igt_sysfs_rps_set_u32(dir, id, value) \
- igt_sysfs_set_u32(dir, igt_sysfs_dir_id_to_name(dir, id), value)
+ __igt_sysfs_set_u32(dir, igt_sysfs_dir_id_to_name(dir, id), value)
#define igt_sysfs_rps_get_boolean(dir, id) \
igt_sysfs_get_boolean(dir, igt_sysfs_dir_id_to_name(dir, id))
@@ -114,14 +114,20 @@ int igt_sysfs_vprintf(int dir, const char *attr, const char *fmt, va_list ap)
int igt_sysfs_printf(int dir, const char *attr, const char *fmt, ...)
__attribute__((format(printf,3,4)));
+uint32_t __igt_sysfs_get_u32(int dir, const char *attr);
uint32_t igt_sysfs_get_u32(int dir, const char *attr);
-bool igt_sysfs_set_u32(int dir, const char *attr, uint32_t value);
+bool __igt_sysfs_set_u32(int dir, const char *attr, uint32_t value);
+void igt_sysfs_set_u32(int dir, const char *attr, uint32_t value);
+uint64_t __igt_sysfs_get_u64(int dir, const char *attr);
uint64_t igt_sysfs_get_u64(int dir, const char *attr);
-bool igt_sysfs_set_u64(int dir, const char *attr, uint64_t value);
+bool __igt_sysfs_set_u64(int dir, const char *attr, uint64_t value);
+void igt_sysfs_set_u64(int dir, const char *attr, uint64_t value);
+bool __igt_sysfs_get_boolean(int dir, const char *attr);
bool igt_sysfs_get_boolean(int dir, const char *attr);
-bool igt_sysfs_set_boolean(int dir, const char *attr, bool value);
+bool __igt_sysfs_set_boolean(int dir, const char *attr, bool value);
+void igt_sysfs_set_boolean(int dir, const char *attr, bool value);
void bind_fbcon(bool enable);
void kick_snd_hda_intel(void);
diff --git a/tests/i915/i915_pm_dc.c b/tests/i915/i915_pm_dc.c
index 2bb07ba8b..734bcfa8b 100644
--- a/tests/i915/i915_pm_dc.c
+++ b/tests/i915/i915_pm_dc.c
@@ -534,7 +534,7 @@ static void setup_dc9_dpms(data_t *data, int dc_target)
igt_require((sysfs_fd = open(KMS_HELPER, O_RDONLY)) >= 0);
kms_poll_saved_state = igt_sysfs_get_boolean(sysfs_fd, "poll");
- igt_sysfs_set_boolean(sysfs_fd, "poll", KMS_POLL_DISABLE);
+ __igt_sysfs_set_boolean(sysfs_fd, "poll", KMS_POLL_DISABLE);
close(sysfs_fd);
if (DC9_RESETS_DC_COUNTERS(data->devid)) {
prev_dc = read_dc_counter(data->debugfs_fd, dc_target);
@@ -629,7 +629,7 @@ static void kms_poll_state_restore(int sig)
sysfs_fd = open(KMS_HELPER, O_RDONLY);
if (sysfs_fd >= 0) {
- igt_sysfs_set_boolean(sysfs_fd, "poll", kms_poll_saved_state);
+ __igt_sysfs_set_boolean(sysfs_fd, "poll", kms_poll_saved_state);
close(sysfs_fd);
}
}
diff --git a/tests/i915/i915_pm_freq_mult.c b/tests/i915/i915_pm_freq_mult.c
index d75ec3f9e..82a578f8e 100644
--- a/tests/i915/i915_pm_freq_mult.c
+++ b/tests/i915/i915_pm_freq_mult.c
@@ -57,11 +57,11 @@ static void restore_rps_defaults(int dir)
if (def < 0)
return;
- max = igt_sysfs_get_u32(def, "rps_max_freq_mhz");
- igt_sysfs_set_u32(dir, "rps_max_freq_mhz", max);
+ max = __igt_sysfs_get_u32(def, "rps_max_freq_mhz");
+ __igt_sysfs_set_u32(dir, "rps_max_freq_mhz", max);
- min = igt_sysfs_get_u32(def, "rps_min_freq_mhz");
- igt_sysfs_set_u32(dir, "rps_min_freq_mhz", min);
+ min = __igt_sysfs_get_u32(def, "rps_min_freq_mhz");
+ __igt_sysfs_set_u32(dir, "rps_min_freq_mhz", min);
close(def);
}
@@ -81,17 +81,17 @@ static void setup_freq(int gt, int dir)
wait_freq_set();
/* Print some debug information */
- rp0 = igt_sysfs_get_u32(dir, "rps_RP0_freq_mhz");
- rp1 = igt_sysfs_get_u32(dir, "rps_RP1_freq_mhz");
- rpn = igt_sysfs_get_u32(dir, "rps_RPn_freq_mhz");
- min = igt_sysfs_get_u32(dir, "rps_min_freq_mhz");
- max = igt_sysfs_get_u32(dir, "rps_max_freq_mhz");
- act = igt_sysfs_get_u32(dir, "rps_act_freq_mhz");
+ rp0 = __igt_sysfs_get_u32(dir, "rps_RP0_freq_mhz");
+ rp1 = __igt_sysfs_get_u32(dir, "rps_RP1_freq_mhz");
+ rpn = __igt_sysfs_get_u32(dir, "rps_RPn_freq_mhz");
+ min = __igt_sysfs_get_u32(dir, "rps_min_freq_mhz");
+ max = __igt_sysfs_get_u32(dir, "rps_max_freq_mhz");
+ act = __igt_sysfs_get_u32(dir, "rps_act_freq_mhz");
igt_debug("RP0 MHz: %d, RP1 MHz: %d, RPn MHz: %d, min MHz: %d, max MHz: %d, act MHz: %d\n", rp0, rp1, rpn, min, max, act);
if (igt_sysfs_has_attr(dir, "media_freq_factor")) {
- media = igt_sysfs_get_u32(dir, "media_freq_factor");
+ media = __igt_sysfs_get_u32(dir, "media_freq_factor");
igt_debug("media ratio: %.2f\n", media * FREQ_SCALE_FACTOR);
}
}
@@ -117,8 +117,8 @@ static void media_freq(int gt, int dir)
setup_freq(gt, dir);
igt_debug("media RP0 mhz: %d, media RPn mhz: %d\n",
- igt_sysfs_get_u32(dir, "media_RP0_freq_mhz"),
- igt_sysfs_get_u32(dir, "media_RPn_freq_mhz"));
+ __igt_sysfs_get_u32(dir, "media_RP0_freq_mhz"),
+ __igt_sysfs_get_u32(dir, "media_RPn_freq_mhz"));
igt_debug("media ratio value 0.0 represents dynamic mode\n");
/*
@@ -141,7 +141,7 @@ static void media_freq(int gt, int dir)
wait_freq_set();
- getv = igt_sysfs_get_u32(dir, "media_freq_factor");
+ getv = __igt_sysfs_get_u32(dir, "media_freq_factor");
igt_debug("media ratio set: %.2f, media ratio get: %.2f\n",
v * scale, getv * scale);
diff --git a/tests/i915/i915_pm_rps.c b/tests/i915/i915_pm_rps.c
index eaacc7c90..5ae91ffd2 100644
--- a/tests/i915/i915_pm_rps.c
+++ b/tests/i915/i915_pm_rps.c
@@ -742,8 +742,8 @@ static void fence_order(int i915)
*/
sysfs = igt_sysfs_open(i915);
- min = igt_sysfs_get_u32(sysfs, "gt_RPn_freq_mhz");
- max = igt_sysfs_get_u32(sysfs, "gt_RP0_freq_mhz");
+ min = __igt_sysfs_get_u32(sysfs, "gt_RPn_freq_mhz");
+ max = __igt_sysfs_get_u32(sysfs, "gt_RP0_freq_mhz");
igt_require(max > min);
/* Only allow ourselves to upclock via waitboosting */
@@ -862,8 +862,8 @@ static void engine_order(int i915)
execbuf.rsvd1 = ctx->id;
sysfs = igt_sysfs_open(i915);
- min = igt_sysfs_get_u32(sysfs, "gt_RPn_freq_mhz");
- max = igt_sysfs_get_u32(sysfs, "gt_RP0_freq_mhz");
+ min = __igt_sysfs_get_u32(sysfs, "gt_RPn_freq_mhz");
+ max = __igt_sysfs_get_u32(sysfs, "gt_RP0_freq_mhz");
igt_require(max > min);
/* Only allow ourselves to upclock via waitboosting */
diff --git a/tests/i915/i915_power.c b/tests/i915/i915_power.c
index 3675b9d6d..86b0fd08c 100644
--- a/tests/i915/i915_power.c
+++ b/tests/i915/i915_power.c
@@ -58,8 +58,8 @@ static void sanity(int i915)
igt_spin_busywait_until_started(spin);
busy = measure_power(&pwr, DURATION_SEC);
i915_for_each_gt(i915, dir, gt) {
- req = igt_sysfs_get_u32(dir, "rps_cur_freq_mhz");
- act = igt_sysfs_get_u32(dir, "rps_act_freq_mhz");
+ req = __igt_sysfs_get_u32(dir, "rps_cur_freq_mhz");
+ act = __igt_sysfs_get_u32(dir, "rps_act_freq_mhz");
igt_info("gt %d: req MHz: %d, act MHz: %d\n", gt, req, act);
}
igt_free_spins(i915);
diff --git a/tests/i915/perf_pmu.c b/tests/i915/perf_pmu.c
index 8b31df7b2..1a5595d67 100644
--- a/tests/i915/perf_pmu.c
+++ b/tests/i915/perf_pmu.c
@@ -1793,9 +1793,9 @@ test_frequency(int gem_fd, unsigned int gt)
sysfs = igt_sysfs_gt_open(gem_fd, gt);
igt_require(sysfs >= 0);
- min_freq = igt_sysfs_get_u32(sysfs, "rps_RPn_freq_mhz");
- max_freq = igt_sysfs_get_u32(sysfs, "rps_RP0_freq_mhz");
- boost_freq = igt_sysfs_get_u32(sysfs, "rps_boost_freq_mhz");
+ min_freq = __igt_sysfs_get_u32(sysfs, "rps_RPn_freq_mhz");
+ max_freq = __igt_sysfs_get_u32(sysfs, "rps_RP0_freq_mhz");
+ boost_freq = __igt_sysfs_get_u32(sysfs, "rps_boost_freq_mhz");
igt_info("Frequency: min=%u, max=%u, boost=%u MHz\n",
min_freq, max_freq, boost_freq);
igt_require(min_freq > 0 && max_freq > 0 && boost_freq > 0);
@@ -1808,12 +1808,12 @@ test_frequency(int gem_fd, unsigned int gt)
/*
* Set GPU to min frequency and read PMU counters.
*/
- igt_require(igt_sysfs_set_u32(sysfs, "rps_min_freq_mhz", min_freq));
- igt_require(igt_sysfs_get_u32(sysfs, "rps_min_freq_mhz") == min_freq);
- igt_require(igt_sysfs_set_u32(sysfs, "rps_max_freq_mhz", min_freq));
- igt_require(igt_sysfs_get_u32(sysfs, "rps_max_freq_mhz") == min_freq);
- igt_require(igt_sysfs_set_u32(sysfs, "rps_boost_freq_mhz", min_freq));
- igt_require(igt_sysfs_get_u32(sysfs, "rps_boost_freq_mhz") == min_freq);
+ igt_require(__igt_sysfs_set_u32(sysfs, "rps_min_freq_mhz", min_freq));
+ igt_require(__igt_sysfs_get_u32(sysfs, "rps_min_freq_mhz") == min_freq);
+ igt_require(__igt_sysfs_set_u32(sysfs, "rps_max_freq_mhz", min_freq));
+ igt_require(__igt_sysfs_get_u32(sysfs, "rps_max_freq_mhz") == min_freq);
+ igt_require(__igt_sysfs_set_u32(sysfs, "rps_boost_freq_mhz", min_freq));
+ igt_require(__igt_sysfs_get_u32(sysfs, "rps_boost_freq_mhz") == min_freq);
gem_quiescent_gpu(gem_fd); /* Idle to be sure the change takes effect */
spin = spin_sync_gt(gem_fd, ahnd, gt, &ctx);
@@ -1834,13 +1834,13 @@ test_frequency(int gem_fd, unsigned int gt)
/*
* Set GPU to max frequency and read PMU counters.
*/
- igt_require(igt_sysfs_set_u32(sysfs, "rps_max_freq_mhz", max_freq));
- igt_require(igt_sysfs_get_u32(sysfs, "rps_max_freq_mhz") == max_freq);
- igt_require(igt_sysfs_set_u32(sysfs, "rps_boost_freq_mhz", boost_freq));
- igt_require(igt_sysfs_get_u32(sysfs, "rps_boost_freq_mhz") == boost_freq);
+ igt_require(__igt_sysfs_set_u32(sysfs, "rps_max_freq_mhz", max_freq));
+ igt_require(__igt_sysfs_get_u32(sysfs, "rps_max_freq_mhz") == max_freq);
+ igt_require(__igt_sysfs_set_u32(sysfs, "rps_boost_freq_mhz", boost_freq));
+ igt_require(__igt_sysfs_get_u32(sysfs, "rps_boost_freq_mhz") == boost_freq);
- igt_require(igt_sysfs_set_u32(sysfs, "rps_min_freq_mhz", max_freq));
- igt_require(igt_sysfs_get_u32(sysfs, "rps_min_freq_mhz") == max_freq);
+ igt_require(__igt_sysfs_set_u32(sysfs, "rps_min_freq_mhz", max_freq));
+ igt_require(__igt_sysfs_get_u32(sysfs, "rps_min_freq_mhz") == max_freq);
gem_quiescent_gpu(gem_fd);
spin = spin_sync_gt(gem_fd, ahnd, gt, &ctx);
@@ -1859,10 +1859,10 @@ test_frequency(int gem_fd, unsigned int gt)
/*
* Restore min/max.
*/
- igt_sysfs_set_u32(sysfs, "rps_min_freq_mhz", min_freq);
- if (igt_sysfs_get_u32(sysfs, "rps_min_freq_mhz") != min_freq)
+ __igt_sysfs_set_u32(sysfs, "rps_min_freq_mhz", min_freq);
+ if (__igt_sysfs_get_u32(sysfs, "rps_min_freq_mhz") != min_freq)
igt_warn("Unable to restore min frequency to saved value [%u MHz], now %u MHz\n",
- min_freq, igt_sysfs_get_u32(sysfs, "rps_min_freq_mhz"));
+ min_freq, __igt_sysfs_get_u32(sysfs, "rps_min_freq_mhz"));
close(fd[0]);
close(fd[1]);
put_ahnd(ahnd);
@@ -1891,7 +1891,7 @@ test_frequency_idle(int gem_fd, unsigned int gt)
sysfs = igt_sysfs_gt_open(gem_fd, gt);
igt_require(sysfs >= 0);
- min_freq = igt_sysfs_get_u32(sysfs, "rps_RPn_freq_mhz");
+ min_freq = __igt_sysfs_get_u32(sysfs, "rps_RPn_freq_mhz");
close(sysfs);
/* While parked, our convention is to report the GPU at 0Hz */
diff --git a/tests/kms_prime.c b/tests/kms_prime.c
index dd5ab993e..98ed30f12 100644
--- a/tests/kms_prime.c
+++ b/tests/kms_prime.c
@@ -341,7 +341,7 @@ static void kms_poll_state_restore(void)
int sysfs_fd;
igt_assert((sysfs_fd = open(KMS_HELPER, O_RDONLY)) >= 0);
- igt_sysfs_set_boolean(sysfs_fd, "poll", kms_poll_saved_state);
+ __igt_sysfs_set_boolean(sysfs_fd, "poll", kms_poll_saved_state);
close(sysfs_fd);
}
@@ -352,7 +352,7 @@ static void kms_poll_disable(void)
igt_require((sysfs_fd = open(KMS_HELPER, O_RDONLY)) >= 0);
kms_poll_saved_state = igt_sysfs_get_boolean(sysfs_fd, "poll");
- igt_sysfs_set_boolean(sysfs_fd, "poll", KMS_POLL_DISABLE);
+ __igt_sysfs_set_boolean(sysfs_fd, "poll", KMS_POLL_DISABLE);
kms_poll_disabled = true;
close(sysfs_fd);
}
diff --git a/tests/nouveau_crc.c b/tests/nouveau_crc.c
index 785d39bde..b78a53653 100644
--- a/tests/nouveau_crc.c
+++ b/tests/nouveau_crc.c
@@ -269,10 +269,10 @@ static void test_ctx_flip_threshold_reset_after_capture(data_t *data)
set_crc_flip_threshold(data, 5);
igt_pipe_crc_start(pipe_crc);
- igt_assert_eq(igt_sysfs_get_u32(data->nv_crc_dir, "flip_threshold"), 5);
+ igt_assert_eq(__igt_sysfs_get_u32(data->nv_crc_dir, "flip_threshold"), 5);
igt_pipe_crc_stop(pipe_crc);
- igt_assert_neq(igt_sysfs_get_u32(data->nv_crc_dir, "flip_threshold"), 5);
+ igt_assert_neq(__igt_sysfs_get_u32(data->nv_crc_dir, "flip_threshold"), 5);
igt_pipe_crc_free(pipe_crc);
}
--
2.40.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-07-12 12:32 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-07-12 11:45 [igt-dev] [PATCH i-g-t] lib/igt_sysfs: add asserting helpers for read/write operations Lukasz Laguna
2023-07-12 12:32 ` Kamil Konieczny
-- strict thread matches above, loose matches on Subject: below --
2023-06-19 6:26 Łukasz Łaguna
2023-06-19 12:05 ` Kamil Konieczny
2023-06-19 12:51 ` Kamil Konieczny
2023-06-19 14:08 ` Laguna, Lukasz
2023-06-21 7:14 ` Laguna, Lukasz
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.