[igt-dev] [PATCH i-g-t 2/2] lib/igt_sysfs: add asserting helpers for read/write operations

Kamil Konieczny kamil.konieczny at linux.intel.com
Wed Jul 12 13:05:04 UTC 2023


Hi Lukasz,
On 2023-07-12 at 14:33:13 +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
> igt_aux_enable_pm_suspend_dbg() function, because of which tests started
> asserting after introduced changes.
> 
> Signed-off-by: Lukasz Laguna <lukasz.laguna at intel.com>

Reviewed-by: Kamil Konieczny <kamil.konieczny at linux.intel.com>

> ---
>  lib/i915/gem_submission.c      |   4 +-
>  lib/igt_aux.c                  |   2 +-
>  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, 260 insertions(+), 123 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 c5fb10185..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 {
> 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
> 


More information about the igt-dev mailing list