All of lore.kernel.org
 help / color / mirror / Atom feed
* [igt-dev] [PATCH i-g-t 1/3] lib/igt_sysfs: Add helpers to iterate over GTs
@ 2022-04-20  6:12 Ashutosh Dixit
  2022-04-20  6:12 ` [igt-dev] [PATCH i-g-t 2/3] lib/igt_sysfs: Add RPS sysfs helpers Ashutosh Dixit
                   ` (3 more replies)
  0 siblings, 4 replies; 12+ messages in thread
From: Ashutosh Dixit @ 2022-04-20  6:12 UTC (permalink / raw)
  To: igt-dev

From: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>

Provide iterators to:
- construct the subdirectory string for a gt
- obtain fd for the subdirectory of the interface

v2: Separated out RPS functionality into seaparate patch (Ashutosh)

Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
Cc: Andi Shyti <andi.shyti@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
---
 lib/igt_sysfs.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++
 lib/igt_sysfs.h | 13 +++++++++
 2 files changed, 84 insertions(+)

diff --git a/lib/igt_sysfs.c b/lib/igt_sysfs.c
index f8ef23e2c8e2..b167c0507039 100644
--- a/lib/igt_sysfs.c
+++ b/lib/igt_sysfs.c
@@ -54,6 +54,21 @@
  * provides basic support for like igt_sysfs_open().
  */
 
+/**
+ * igt_sysfs_has_attr:
+ * @dir: sysfs directory fd
+ * @attr: attr inside sysfs dir that needs to be checked for existence
+ *
+ * This checks if specified attr exists in device sysfs directory.
+ *
+ * Returns:
+ * true if attr exists in sysfs, false otherwise.
+ */
+bool igt_sysfs_has_attr(int dir, const char *attr)
+{
+	return !faccessat(dir, attr, F_OK, 0);
+}
+
 /**
  * igt_sysfs_path:
  * @device: fd of the device
@@ -104,6 +119,62 @@ int igt_sysfs_open(int device)
 	return open(path, O_RDONLY);
 }
 
+/**
+ * igt_sysfs_gt_path:
+ * @device: fd of the device
+ * @gt: gt number
+ * @path: buffer to fill with the sysfs gt path to the device
+ * @pathlen: length of @path buffer
+ *
+ * This finds the sysfs directory corresponding to @device and @gt. If the gt
+ * specific directory is not available and gt is 0, path is filled with sysfs
+ * base directory.
+ *
+ * Returns:
+ * The directory path, or NULL on failure.
+ */
+char *igt_sysfs_gt_path(int device, int gt, char *path, int pathlen)
+{
+	struct stat st;
+
+	if (device < 0)
+		return NULL;
+
+	if (igt_debug_on(fstat(device, &st)) || igt_debug_on(!S_ISCHR(st.st_mode)))
+		return NULL;
+
+	snprintf(path, pathlen, "/sys/dev/char/%d:%d/gt/gt%d",
+		 major(st.st_rdev), minor(st.st_rdev), gt);
+
+	if (!igt_debug_on(access(path, F_OK)))
+		return path;
+	else if (!igt_debug_on(gt != 0))
+		return igt_sysfs_path(device, path, pathlen);
+
+	return NULL;
+}
+
+/**
+ * igt_sysfs_gt_open:
+ * @device: fd of the device
+ * @gt: gt number
+ *
+ * This opens the sysfs gt directory corresponding to device and gt for use
+ * with igt_sysfs_set() and igt_sysfs_get().
+ *
+ * Returns:
+ * The directory fd, or -1 on failure.
+ */
+int igt_sysfs_gt_open(int device, int gt)
+{
+	char path[96];
+
+	if (igt_debug_on(!igt_sysfs_gt_path(device, gt, path, sizeof(path))))
+		return -1;
+
+	return open(path, O_RDONLY);
+}
+
 /**
  * igt_sysfs_write:
  * @dir: directory for the device from igt_sysfs_open()
diff --git a/lib/igt_sysfs.h b/lib/igt_sysfs.h
index 56741a0a37e3..33317a969619 100644
--- a/lib/igt_sysfs.h
+++ b/lib/igt_sysfs.h
@@ -28,8 +28,21 @@
 #include <stdbool.h>
 #include <stdarg.h>
 
+#define for_each_sysfs_gt_path(i915__, path__, pathlen__) \
+	for (int gt__ = 0; \
+	     igt_sysfs_gt_path(i915__, gt__, path__, pathlen__) != NULL; \
+	     gt__++)
+
+#define for_each_sysfs_gt_dirfd(i915__, dirfd__, gt__) \
+	for (gt__ = 0; \
+	     (dirfd__ = igt_sysfs_gt_open(i915__, gt__)) != -1; \
+	     close(dirfd__), gt__++)
+
 char *igt_sysfs_path(int device, char *path, int pathlen);
 int igt_sysfs_open(int device);
+char *igt_sysfs_gt_path(int device, int gt, char *path, int pathlen);
+int igt_sysfs_gt_open(int device, int gt);
+bool igt_sysfs_has_attr(int dir, const char *attr);
 
 int igt_sysfs_read(int dir, const char *attr, void *data, int len);
 int igt_sysfs_write(int dir, const char *attr, const void *data, int len);
-- 
2.34.1

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

* [igt-dev] [PATCH i-g-t 2/3] lib/igt_sysfs: Add RPS sysfs helpers
  2022-04-20  6:12 [igt-dev] [PATCH i-g-t 1/3] lib/igt_sysfs: Add helpers to iterate over GTs Ashutosh Dixit
@ 2022-04-20  6:12 ` Ashutosh Dixit
  2022-04-21 15:49   ` Kamil Konieczny
  2022-04-20  6:12 ` [igt-dev] [PATCH i-g-t 3/3] tests/i915_pm_disag_freq: New test for media freq factor Ashutosh Dixit
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 12+ messages in thread
From: Ashutosh Dixit @ 2022-04-20  6:12 UTC (permalink / raw)
  To: igt-dev

From: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>

RPS sysfs files exposed by the kernel can either be in per-gt sysfs or in
the per-device legacy sysfs. Add helpers to read/write these files in
either of the two sets of locations.

v2: Added function descriptions (Kamil)
    Separated patch from "lib/igt_sysfs: Add helpers to iterate over GTs"

Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
Cc: Andi Shyti <andi.shyti@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
---
 lib/igt_sysfs.c | 90 +++++++++++++++++++++++++++++++++++++++++++++++++
 lib/igt_sysfs.h | 54 +++++++++++++++++++++++++++++
 2 files changed, 144 insertions(+)

diff --git a/lib/igt_sysfs.c b/lib/igt_sysfs.c
index b167c0507039..0ac5d2a3d6e5 100644
--- a/lib/igt_sysfs.c
+++ b/lib/igt_sysfs.c
@@ -54,6 +54,96 @@
  * provides basic support for like igt_sysfs_open().
  */
 
+enum {
+	GT,
+	RPS,
+
+	SYSFS_NUM_TYPES,
+};
+
+static const char *i915_attr_name[SYSFS_NUM_TYPES][SYSFS_NUM_ATTR] = {
+	{
+		"gt_act_freq_mhz",
+		"gt_cur_freq_mhz",
+		"gt_min_freq_mhz",
+		"gt_max_freq_mhz",
+		"gt_RP0_freq_mhz",
+		"gt_RP1_freq_mhz",
+		"gt_RPn_freq_mhz",
+		"gt_idle_freq_mhz",
+		"gt_boost_freq_mhz",
+		"power/rc6_enable",
+		"power/rc6_residency_ms",
+		"power/rc6p_residency_ms",
+		"power/rc6pp_residency_ms",
+		"power/media_rc6_residency_ms",
+	},
+	{
+		"rps_act_freq_mhz",
+		"rps_cur_freq_mhz",
+		"rps_min_freq_mhz",
+		"rps_max_freq_mhz",
+		"rps_RP0_freq_mhz",
+		"rps_RP1_freq_mhz",
+		"rps_RPn_freq_mhz",
+		"rps_idle_freq_mhz",
+		"rps_boost_freq_mhz",
+		"rc6_enable",
+		"rc6_residency_ms",
+		"rc6p_residency_ms",
+		"rc6pp_residency_ms",
+		"media_rc6_residency_ms",
+	},
+};
+
+/**
+ * igt_sysfs_dir_id_to_name:
+ * @dir: sysfs directory fd
+ * @id: sysfs attribute id
+ *
+ * Returns attribute name corresponding to attribute id in either the
+ * per-gt or legacy per-device sysfs
+ *
+ * Returns:
+ * Attribute name in sysfs
+ */
+const char *igt_sysfs_dir_id_to_name(int dir, enum i915_attr_id id)
+{
+	igt_assert(id < SYSFS_NUM_ATTR);
+
+	if (igt_sysfs_has_attr(dir, i915_attr_name[GT][id]))
+		return i915_attr_name[GT][id];
+	else if (igt_sysfs_has_attr(dir, i915_attr_name[RPS][id]))
+		return i915_attr_name[RPS][id];
+	else
+		igt_assert_f(0, "Invalid sysfs dir\n");
+}
+
+/**
+ * igt_sysfs_path_id_to_name:
+ * @path: sysfs directory path
+ * @id: sysfs attribute id
+ *
+ * Returns attribute name corresponding to attribute id in either the
+ * per-gt or legacy per-device sysfs
+ *
+ * Returns:
+ * Attribute name in sysfs
+ */
+const char *igt_sysfs_path_id_to_name(const char *path, enum i915_attr_id id)
+{
+	int dir;
+	const char *name;
+
+	dir = open(path, O_RDONLY);
+	igt_assert(dir);
+
+	name = igt_sysfs_dir_id_to_name(dir, id);
+	close(dir);
+
+	return name;
+}
+
 /**
  * igt_sysfs_has_attr:
  * @dir: sysfs directory fd
diff --git a/lib/igt_sysfs.h b/lib/igt_sysfs.h
index 33317a969619..8e39b8fa9890 100644
--- a/lib/igt_sysfs.h
+++ b/lib/igt_sysfs.h
@@ -38,11 +38,65 @@
 	     (dirfd__ = igt_sysfs_gt_open(i915__, gt__)) != -1; \
 	     close(dirfd__), gt__++)
 
+#define igt_sysfs_rps_write(dir, id, data, len) \
+	igt_sysfs_write(dir, igt_sysfs_dir_id_to_name(dir, id), data, len)
+
+#define igt_sysfs_rps_read(dir, id, data, len) \
+	igt_sysfs_read(dir, igt_sysfs_dir_id_to_name(dir, id), data, len)
+
+#define igt_sysfs_rps_set(dir, id, value) \
+	igt_sysfs_set(dir, igt_sysfs_dir_id_to_name(dir, id), value)
+
+#define igt_sysfs_rps_get(dir, id) \
+	igt_sysfs_get(dir, igt_sysfs_dir_id_to_name(dir, id))
+
+#define igt_sysfs_rps_scanf(dir, id, fmt, ...) \
+	igt_sysfs_scanf(dir, igt_sysfs_dir_id_to_name(dir, id), fmt, ##__VA_ARGS__)
+
+#define igt_sysfs_rps_vprintf(dir, id, fmt, ap) \
+	igt_sysfs_vprintf(dir, igt_sysfs_dir_id_to_name(id), fmt, ap)
+
+#define igt_sysfs_rps_printf(dir, id, fmt, ...) \
+	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))
+
+#define igt_sysfs_rps_set_u32(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))
+
+#define igt_sysfs_rps_set_boolean(dir, id, value) \
+	igt_sysfs_set_boolean(dir, igt_sysfs_dir_id_to_name(dir, id), value)
+
+enum i915_attr_id {
+	RPS_ACT_FREQ_MHZ,
+	RPS_CUR_FREQ_MHZ,
+	RPS_MIN_FREQ_MHZ,
+	RPS_MAX_FREQ_MHZ,
+	RPS_RP0_FREQ_MHZ,
+	RPS_RP1_FREQ_MHZ,
+	RPS_RPn_FREQ_MHZ,
+	RPS_IDLE_FREQ_MHZ,
+	RPS_BOOST_FREQ_MHZ,
+	RC6_ENABLE,
+	RC6_RESIDENCY_MS,
+	RC6P_RESIDENCY_MS,
+	RC6PP_RESIDENCY_MS,
+	MEDIA_RC6_RESIDENCY_MS,
+
+	SYSFS_NUM_ATTR,
+};
+
 char *igt_sysfs_path(int device, char *path, int pathlen);
 int igt_sysfs_open(int device);
 char *igt_sysfs_gt_path(int device, int gt, char *path, int pathlen);
 int igt_sysfs_gt_open(int device, int gt);
 bool igt_sysfs_has_attr(int dir, const char *attr);
+const char *igt_sysfs_dir_id_to_name(int dir, enum i915_attr_id id);
+const char *igt_sysfs_path_id_to_name(const char *path, enum i915_attr_id id);
 
 int igt_sysfs_read(int dir, const char *attr, void *data, int len);
 int igt_sysfs_write(int dir, const char *attr, const void *data, int len);
-- 
2.34.1

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

* [igt-dev] [PATCH i-g-t 3/3] tests/i915_pm_disag_freq: New test for media freq factor
  2022-04-20  6:12 [igt-dev] [PATCH i-g-t 1/3] lib/igt_sysfs: Add helpers to iterate over GTs Ashutosh Dixit
  2022-04-20  6:12 ` [igt-dev] [PATCH i-g-t 2/3] lib/igt_sysfs: Add RPS sysfs helpers Ashutosh Dixit
@ 2022-04-20  6:12 ` Ashutosh Dixit
  2022-04-21 17:00   ` Kamil Konieczny
  2022-04-22 16:15   ` Kamil Konieczny
  2022-04-20  7:09 ` [igt-dev] ✗ Fi.CI.BAT: failure for series starting with [i-g-t,1/3] lib/igt_sysfs: Add helpers to iterate over GTs Patchwork
  2022-04-21 11:05 ` [igt-dev] [PATCH i-g-t 1/3] " Dandamudi, Priyanka
  3 siblings, 2 replies; 12+ messages in thread
From: Ashutosh Dixit @ 2022-04-20  6:12 UTC (permalink / raw)
  To: igt-dev

XEHPSDV and DG2/ATS-M allow media IP blocks to run at frequencies different
from the GT frequency. i915 exposes sysfs controls for this frequency
"disaggregation". IGT's introduced in this patch exercise and verify these
per-gt (gt/gtN) sysfs attributes.

Further, RPS defaults exposed in gt/gtN/.defaults sysfs directory are used
in the test to start and complete in the known default state.

v2: Added igt_describe's and s/igt_info/igt_debug/  (Kamil)

Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
Cc: Anshuman Gupta <anshuman.gupta@intel.com>
Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
---
 tests/i915/i915_pm_disag_freq.c | 186 ++++++++++++++++++++++++++++++++
 tests/meson.build               |   8 ++
 2 files changed, 194 insertions(+)
 create mode 100644 tests/i915/i915_pm_disag_freq.c

diff --git a/tests/i915/i915_pm_disag_freq.c b/tests/i915/i915_pm_disag_freq.c
new file mode 100644
index 000000000000..af216eb98815
--- /dev/null
+++ b/tests/i915/i915_pm_disag_freq.c
@@ -0,0 +1,186 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2022 Intel Corporation
+ */
+
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+
+#include "i915/gem.h"
+#include "igt.h"
+#include "igt_sysfs.h"
+
+IGT_TEST_DESCRIPTION(
+	"Tests for sysfs controls for \"disaggregated\" IP blocks, viz. IP "
+	"blocks which run at frequencies different from the main GT frequency."
+);
+
+#define FREQ_SCALE_FACTOR	0.00390625f	/* 1.0f / 256 */
+
+/*
+ * Firmware interfaces are not completely synchronous, a delay is needed
+ * before the requested freq is actually set.
+ * Media ratio read back after set will mismatch if this value is too small
+ */
+#define wait_freq_set()	usleep(100000)
+
+static int i915 = -1;
+const intel_ctx_t *ctx;
+uint64_t ahnd;
+
+static void spin_all(void)
+{
+	igt_spin_t *spin = igt_spin_new(i915, .ahnd = ahnd, .ctx = ctx, .engine = ALL_ENGINES,
+					.flags = IGT_SPIN_POLL_RUN);
+
+	/* Wait till at least one spinner starts */
+	igt_spin_busywait_until_started(spin);
+}
+
+static void restore_rps_defaults(int dir)
+{
+	int def, min, max, media;
+
+	/* 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);
+
+	min = igt_sysfs_get_u32(def, "rps_min_freq_mhz");
+	igt_sysfs_set_u32(dir, "rps_min_freq_mhz", min);
+
+	if (igt_sysfs_has_attr(dir, "media_freq_factor")) {
+		media = igt_sysfs_get_u32(def, "media_freq_factor");
+		igt_sysfs_set_u32(dir, "media_freq_factor", media);
+	}
+
+	close(def);
+}
+
+static void __restore_rps_defaults(int sig)
+{
+	int dir, gt;
+
+	for_each_sysfs_gt_dirfd(i915, dir, gt)
+		restore_rps_defaults(dir);
+}
+
+static void setup_freq(int gt, int dir)
+{
+	int rp0, rp1, rpn, min, max, act, media;
+
+	ctx = intel_ctx_create_all_physical(i915);
+	ahnd = get_reloc_ahnd(i915, ctx->id);
+
+	/* Reset to known state */
+	restore_rps_defaults(dir);
+
+	/* Spin on all engines to jack freq up to max */
+	spin_all();
+	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");
+
+	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");
+		igt_debug("media ratio: %.2f\n", media * FREQ_SCALE_FACTOR);
+	}
+}
+
+static void cleanup(int dir)
+{
+	igt_free_spins(i915);
+	put_ahnd(ahnd);
+	intel_ctx_destroy(i915, ctx);
+	restore_rps_defaults(dir);
+	gem_quiescent_gpu(i915);
+}
+
+static void media_freq(int gt, int dir)
+{
+	float scale;
+
+	igt_require(igt_sysfs_has_attr(dir, "media_freq_factor"));
+
+	igt_sysfs_scanf(dir, "media_freq_factor.scale", "%g", &scale);
+	igt_assert_eq(scale, FREQ_SCALE_FACTOR);
+
+	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_debug("media ratio value 0.0 represents dynamic mode\n");
+
+	/*
+	 * Media freq ratio modes supported are: dynamic (0), 1:2 (128) and
+	 * 1:1 (256). Setting dynamic (0) can return any of the three
+	 * modes. Fixed ratio modes should return the same value.
+	 */
+	for (int v = 256; v >= 0; v -= 64) {
+		int getv, ret;
+
+		/*
+		 * Check that we can set the mode. Ratios other than 1:2
+		 * and 1:1 are not supported.
+		 */
+		ret = igt_sysfs_printf(dir, "media_freq_factor", "%u", v);
+		if (ret <= 0) {
+			igt_debug("Media ratio %.2f is not supported\n", v * scale);
+			continue;
+		}
+
+		wait_freq_set();
+
+		getv = igt_sysfs_get_u32(dir, "media_freq_factor");
+
+		igt_debug("media ratio set: %.2f, media ratio get: %.2f\n",
+			  v * scale, getv * scale);
+
+		/*
+		 * Skip validation in dynamic mode since the returned media
+		 * ratio and freq are platform dependent and not clearly defined
+		 */
+		if (v)
+			igt_assert_eq(getv, v);
+	}
+
+	cleanup(dir);
+}
+
+igt_main
+{
+	int dir, gt;
+
+	igt_fixture {
+		i915 = drm_open_driver(DRIVER_INTEL);
+
+		/* Disag multipliers (aka "frequency factors") are not simulated. */
+		igt_require(!igt_run_in_simulation());
+		igt_install_exit_handler(__restore_rps_defaults);
+	}
+
+	igt_describe("Tests for \"disaggregated\" media freq sysfs");
+	igt_subtest_with_dynamic("media-freq") {
+		for_each_sysfs_gt_dirfd(i915, dir, gt) {
+			igt_dynamic_f("gt%d", gt)
+				media_freq(gt, dir);
+		}
+	}
+
+	igt_fixture {
+		close(i915);
+	}
+}
diff --git a/tests/meson.build b/tests/meson.build
index 7261e9aa2950..cd89defbb418 100644
--- a/tests/meson.build
+++ b/tests/meson.build
@@ -356,6 +356,14 @@ test_executables += executable('gem_mmap_offset',
 	   install : true)
 test_list += 'gem_mmap_offset'
 
+test_executables += executable('i915_pm_disag_freq',
+	   join_paths('i915', 'i915_pm_disag_freq.c'),
+	   dependencies : test_deps + [ lib_igt_perf ],
+	   install_dir : libexecdir,
+	   install_rpath : libexecdir_rpathdir,
+	   install : true)
+test_list += 'i915_pm_disag_freq'
+
 test_executables += executable('i915_pm_rc6_residency',
 	   join_paths('i915', 'i915_pm_rc6_residency.c'),
 	   dependencies : test_deps + [ lib_igt_perf ],
-- 
2.34.1

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

* [igt-dev] ✗ Fi.CI.BAT: failure for series starting with [i-g-t,1/3] lib/igt_sysfs: Add helpers to iterate over GTs
  2022-04-20  6:12 [igt-dev] [PATCH i-g-t 1/3] lib/igt_sysfs: Add helpers to iterate over GTs Ashutosh Dixit
  2022-04-20  6:12 ` [igt-dev] [PATCH i-g-t 2/3] lib/igt_sysfs: Add RPS sysfs helpers Ashutosh Dixit
  2022-04-20  6:12 ` [igt-dev] [PATCH i-g-t 3/3] tests/i915_pm_disag_freq: New test for media freq factor Ashutosh Dixit
@ 2022-04-20  7:09 ` Patchwork
  2022-04-21 11:05 ` [igt-dev] [PATCH i-g-t 1/3] " Dandamudi, Priyanka
  3 siblings, 0 replies; 12+ messages in thread
From: Patchwork @ 2022-04-20  7:09 UTC (permalink / raw)
  To: Ashutosh Dixit; +Cc: igt-dev

[-- Attachment #1: Type: text/plain, Size: 5673 bytes --]

== Series Details ==

Series: series starting with [i-g-t,1/3] lib/igt_sysfs: Add helpers to iterate over GTs
URL   : https://patchwork.freedesktop.org/series/102856/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_11523 -> IGTPW_6952
====================================================

Summary
-------

  **FAILURE**

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

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

Participating hosts (47 -> 43)
------------------------------

  Additional (2): fi-hsw-4770 fi-rkl-11600 
  Missing    (6): fi-cml-u2 fi-bdw-5557u fi-hsw-4200u fi-bsw-cyan fi-ctg-p8600 bat-rpls-2 

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

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

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live@slpc:
    - bat-dg1-5:          NOTRUN -> [INCOMPLETE][1]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6952/bat-dg1-5/igt@i915_selftest@live@slpc.html

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_suspend@basic-s3@smem:
    - fi-rkl-11600:       NOTRUN -> [INCOMPLETE][2] ([i915#5127])
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6952/fi-rkl-11600/igt@gem_exec_suspend@basic-s3@smem.html

  * igt@gem_huc_copy@huc-copy:
    - fi-hsw-4770:        NOTRUN -> [SKIP][3] ([fdo#109271]) +9 similar issues
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6952/fi-hsw-4770/igt@gem_huc_copy@huc-copy.html

  * igt@i915_pm_backlight@basic-brightness:
    - fi-hsw-4770:        NOTRUN -> [SKIP][4] ([fdo#109271] / [i915#3012])
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6952/fi-hsw-4770/igt@i915_pm_backlight@basic-brightness.html

  * igt@kms_busy@basic@modeset:
    - fi-tgl-u2:          [PASS][5] -> [DMESG-WARN][6] ([i915#402]) +1 similar issue
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11523/fi-tgl-u2/igt@kms_busy@basic@modeset.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6952/fi-tgl-u2/igt@kms_busy@basic@modeset.html

  * igt@kms_chamelium@common-hpd-after-suspend:
    - fi-hsw-4770:        NOTRUN -> [SKIP][7] ([fdo#109271] / [fdo#111827]) +8 similar issues
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6952/fi-hsw-4770/igt@kms_chamelium@common-hpd-after-suspend.html

  * igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-d:
    - fi-hsw-4770:        NOTRUN -> [SKIP][8] ([fdo#109271] / [i915#533])
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6952/fi-hsw-4770/igt@kms_pipe_crc_basic@compare-crc-sanitycheck-pipe-d.html

  * igt@kms_psr@primary_mmap_gtt:
    - fi-hsw-4770:        NOTRUN -> [SKIP][9] ([fdo#109271] / [i915#1072]) +3 similar issues
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6952/fi-hsw-4770/igt@kms_psr@primary_mmap_gtt.html

  
#### Possible fixes ####

  * igt@i915_selftest@live@hangcheck:
    - fi-adl-ddr5:        [DMESG-WARN][10] -> [PASS][11]
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11523/fi-adl-ddr5/igt@i915_selftest@live@hangcheck.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6952/fi-adl-ddr5/igt@i915_selftest@live@hangcheck.html

  * igt@kms_flip@basic-flip-vs-dpms@a-edp1:
    - fi-tgl-u2:          [DMESG-WARN][12] ([i915#402]) -> [PASS][13] +1 similar issue
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11523/fi-tgl-u2/igt@kms_flip@basic-flip-vs-dpms@a-edp1.html
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6952/fi-tgl-u2/igt@kms_flip@basic-flip-vs-dpms@a-edp1.html

  
#### Warnings ####

  * igt@i915_selftest@live@hangcheck:
    - bat-dg1-5:          [INCOMPLETE][14] -> [DMESG-FAIL][15] ([i915#4494] / [i915#4957])
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_11523/bat-dg1-5/igt@i915_selftest@live@hangcheck.html
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6952/bat-dg1-5/igt@i915_selftest@live@hangcheck.html

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

  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
  [i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072
  [i915#3012]: https://gitlab.freedesktop.org/drm/intel/issues/3012
  [i915#3576]: https://gitlab.freedesktop.org/drm/intel/issues/3576
  [i915#402]: https://gitlab.freedesktop.org/drm/intel/issues/402
  [i915#4494]: https://gitlab.freedesktop.org/drm/intel/issues/4494
  [i915#4957]: https://gitlab.freedesktop.org/drm/intel/issues/4957
  [i915#5127]: https://gitlab.freedesktop.org/drm/intel/issues/5127
  [i915#533]: https://gitlab.freedesktop.org/drm/intel/issues/533


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

  * CI: CI-20190529 -> None
  * IGT: IGT_6440 -> IGTPW_6952

  CI-20190529: 20190529
  CI_DRM_11523: 6b6803c3f43f5d0f960246b4b52f956f1a579833 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_6952: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6952/index.html
  IGT_6440: 04262fc75ff3ec42f4db0c929d46b7cd5083911f @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git



== Testlist changes ==

+igt@i915_pm_disag_freq@media-freq

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6952/index.html

[-- Attachment #2: Type: text/html, Size: 6835 bytes --]

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

* Re: [igt-dev] [PATCH i-g-t 1/3] lib/igt_sysfs: Add helpers to iterate over GTs
  2022-04-20  6:12 [igt-dev] [PATCH i-g-t 1/3] lib/igt_sysfs: Add helpers to iterate over GTs Ashutosh Dixit
                   ` (2 preceding siblings ...)
  2022-04-20  7:09 ` [igt-dev] ✗ Fi.CI.BAT: failure for series starting with [i-g-t,1/3] lib/igt_sysfs: Add helpers to iterate over GTs Patchwork
@ 2022-04-21 11:05 ` Dandamudi, Priyanka
  3 siblings, 0 replies; 12+ messages in thread
From: Dandamudi, Priyanka @ 2022-04-21 11:05 UTC (permalink / raw)
  To: Dixit, Ashutosh, igt-dev



> -----Original Message-----
> From: igt-dev <igt-dev-bounces@lists.freedesktop.org> On Behalf Of
> Ashutosh Dixit
> Sent: 20 April 2022 11:43 AM
> To: igt-dev@lists.freedesktop.org
> Subject: [igt-dev] [PATCH i-g-t 1/3] lib/igt_sysfs: Add helpers to iterate over
> GTs
> 
> From: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> 
> Provide iterators to:
> - construct the subdirectory string for a gt
> - obtain fd for the subdirectory of the interface
> 
> v2: Separated out RPS functionality into seaparate patch (Ashutosh)
> 
> Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
> Cc: Andi Shyti <andi.shyti@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
> Signed-off-by: Umesh Nerlige Ramappa
> <umesh.nerlige.ramappa@intel.com>
> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> ---
>  lib/igt_sysfs.c | 71
> +++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/igt_sysfs.h | 13 +++++++++
>  2 files changed, 84 insertions(+)
> 
> diff --git a/lib/igt_sysfs.c b/lib/igt_sysfs.c index f8ef23e2c8e2..b167c0507039
> 100644
> --- a/lib/igt_sysfs.c
> +++ b/lib/igt_sysfs.c
> @@ -54,6 +54,21 @@
>   * provides basic support for like igt_sysfs_open().
>   */
> 
> +/**
> + * igt_sysfs_has_attr:
> + * @dir: sysfs directory fd
> + * @attr: attr inside sysfs dir that needs to be checked for existence
> + *
> + * This checks if specified attr exists in device sysfs directory.
> + *
> + * Returns:
> + * true if attr exists in sysfs, false otherwise.
> + */
> +bool igt_sysfs_has_attr(int dir, const char *attr) {
> +	return !faccessat(dir, attr, F_OK, 0); }
> +
>  /**
>   * igt_sysfs_path:
>   * @device: fd of the device
> @@ -104,6 +119,62 @@ int igt_sysfs_open(int device)
>  	return open(path, O_RDONLY);
>  }
> 
> +/**
> + * igt_sysfs_gt_path:
> + * @device: fd of the device
> + * @gt: gt number
> + * @path: buffer to fill with the sysfs gt path to the device
> + * @pathlen: length of @path buffer
> + *
> + * This finds the sysfs directory corresponding to @device and @gt. If
> +the gt
> + * specific directory is not available and gt is 0, path is filled with
> +sysfs
> + * base directory.
> + *
> + * Returns:
> + * The directory path, or NULL on failure.
> + */
> +char *igt_sysfs_gt_path(int device, int gt, char *path, int pathlen) {
> +	struct stat st;
> +
> +	if (device < 0)
> +		return NULL;
> +
> +	if (igt_debug_on(fstat(device, &st)) ||
> igt_debug_on(!S_ISCHR(st.st_mode)))
> +		return NULL;
> +
> +	snprintf(path, pathlen, "/sys/dev/char/%d:%d/gt/gt%d",
> +		 major(st.st_rdev), minor(st.st_rdev), gt);
> +
> +	if (!igt_debug_on(access(path, F_OK)))
> +		return path;
> +	else if (!igt_debug_on(gt != 0))
> +		return igt_sysfs_path(device, path, pathlen);
> +
> +	return NULL;
> +}
> +
> +/**
> + * igt_sysfs_gt_open:
> + * @device: fd of the device
> + * @gt: gt number
> + *
> + * This opens the sysfs gt directory corresponding to device and gt for
> +use
> + * with igt_sysfs_set() and igt_sysfs_get().
> + *
> + * Returns:
> + * The directory fd, or -1 on failure.
> + */
> +int igt_sysfs_gt_open(int device, int gt) {
> +	char path[96];
> +
> +	if (igt_debug_on(!igt_sysfs_gt_path(device, gt, path, sizeof(path))))
> +		return -1;
> +
> +	return open(path, O_RDONLY);
> +}
> +
>  /**
>   * igt_sysfs_write:
>   * @dir: directory for the device from igt_sysfs_open() diff --git
> a/lib/igt_sysfs.h b/lib/igt_sysfs.h index 56741a0a37e3..33317a969619 100644
> --- a/lib/igt_sysfs.h
> +++ b/lib/igt_sysfs.h
> @@ -28,8 +28,21 @@
>  #include <stdbool.h>
>  #include <stdarg.h>
> 
> +#define for_each_sysfs_gt_path(i915__, path__, pathlen__) \
> +	for (int gt__ = 0; \
> +	     igt_sysfs_gt_path(i915__, gt__, path__, pathlen__) != NULL; \
> +	     gt__++)
> +
> +#define for_each_sysfs_gt_dirfd(i915__, dirfd__, gt__) \
> +	for (gt__ = 0; \
> +	     (dirfd__ = igt_sysfs_gt_open(i915__, gt__)) != -1; \
> +	     close(dirfd__), gt__++)
> +
>  char *igt_sysfs_path(int device, char *path, int pathlen);  int
> igt_sysfs_open(int device);
> +char *igt_sysfs_gt_path(int device, int gt, char *path, int pathlen);
> +int igt_sysfs_gt_open(int device, int gt); bool igt_sysfs_has_attr(int
> +dir, const char *attr);
> 
>  int igt_sysfs_read(int dir, const char *attr, void *data, int len);  int
> igt_sysfs_write(int dir, const char *attr, const void *data, int len);
> --
> 2.34.1
[Dandamudi, Priyanka] 
LGTM:
Reviewed-by: Priyanka Dandamudi <Priyanka.dandamudi@intel.com>

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

* Re: [igt-dev] [PATCH i-g-t 2/3] lib/igt_sysfs: Add RPS sysfs helpers
  2022-04-20  6:12 ` [igt-dev] [PATCH i-g-t 2/3] lib/igt_sysfs: Add RPS sysfs helpers Ashutosh Dixit
@ 2022-04-21 15:49   ` Kamil Konieczny
  2022-04-25 23:15     ` Dixit, Ashutosh
  0 siblings, 1 reply; 12+ messages in thread
From: Kamil Konieczny @ 2022-04-21 15:49 UTC (permalink / raw)
  To: igt-dev; +Cc: Tvrtko Ursulin

Hi Ashutosh,

On 2022-04-19 at 23:12:50 -0700, Ashutosh Dixit wrote:
> From: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> 
> RPS sysfs files exposed by the kernel can either be in per-gt sysfs or in
> the per-device legacy sysfs. Add helpers to read/write these files in
> either of the two sets of locations.
> 
> v2: Added function descriptions (Kamil)
>     Separated patch from "lib/igt_sysfs: Add helpers to iterate over GTs"
> 
> Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
> Cc: Andi Shyti <andi.shyti@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> ---
>  lib/igt_sysfs.c | 90 +++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/igt_sysfs.h | 54 +++++++++++++++++++++++++++++
>  2 files changed, 144 insertions(+)
> 
> diff --git a/lib/igt_sysfs.c b/lib/igt_sysfs.c
> index b167c0507039..0ac5d2a3d6e5 100644
> --- a/lib/igt_sysfs.c
> +++ b/lib/igt_sysfs.c
> @@ -54,6 +54,96 @@
>   * provides basic support for like igt_sysfs_open().
>   */
>  
> +enum {
> +	GT,
> +	RPS,
> +
> +	SYSFS_NUM_TYPES,
> +};
> +
> +static const char *i915_attr_name[SYSFS_NUM_TYPES][SYSFS_NUM_ATTR] = {
> +	{
> +		"gt_act_freq_mhz",
> +		"gt_cur_freq_mhz",
> +		"gt_min_freq_mhz",
> +		"gt_max_freq_mhz",
> +		"gt_RP0_freq_mhz",
> +		"gt_RP1_freq_mhz",
> +		"gt_RPn_freq_mhz",
> +		"gt_idle_freq_mhz",
> +		"gt_boost_freq_mhz",
> +		"power/rc6_enable",
> +		"power/rc6_residency_ms",
> +		"power/rc6p_residency_ms",
> +		"power/rc6pp_residency_ms",
> +		"power/media_rc6_residency_ms",
> +	},
> +	{
> +		"rps_act_freq_mhz",
> +		"rps_cur_freq_mhz",
> +		"rps_min_freq_mhz",
> +		"rps_max_freq_mhz",
> +		"rps_RP0_freq_mhz",
> +		"rps_RP1_freq_mhz",
> +		"rps_RPn_freq_mhz",
> +		"rps_idle_freq_mhz",
> +		"rps_boost_freq_mhz",
> +		"rc6_enable",
> +		"rc6_residency_ms",
> +		"rc6p_residency_ms",
> +		"rc6pp_residency_ms",
> +		"media_rc6_residency_ms",
> +	},
> +};
> +
> +/**
> + * igt_sysfs_dir_id_to_name:
> + * @dir: sysfs directory fd
> + * @id: sysfs attribute id
> + *
> + * Returns attribute name corresponding to attribute id in either the
> + * per-gt or legacy per-device sysfs
> + *
> + * Returns:
> + * Attribute name in sysfs
> + */
> +const char *igt_sysfs_dir_id_to_name(int dir, enum i915_attr_id id)
> +{
> +	igt_assert(id < SYSFS_NUM_ATTR);

What about id < 0 ?

> +
> +	if (igt_sysfs_has_attr(dir, i915_attr_name[GT][id]))
> +		return i915_attr_name[GT][id];
> +	else if (igt_sysfs_has_attr(dir, i915_attr_name[RPS][id]))
------- ^
s/else //

> +		return i915_attr_name[RPS][id];
> +	else
------- ^
drop this else here, checkpatch warn:
WARNING: else is not generally useful after a break or return
#102: FILE: lib/igt_sysfs.c:118:
+               return i915_attr_name[RPS][id];
+       else

> +		igt_assert_f(0, "Invalid sysfs dir\n");

and align this assert.

Rest looks good, with that fixed you can add my r-b tag.

Regards,
Kamil

> +}
> +
> +/**
> + * igt_sysfs_path_id_to_name:
> + * @path: sysfs directory path
> + * @id: sysfs attribute id
> + *
> + * Returns attribute name corresponding to attribute id in either the
> + * per-gt or legacy per-device sysfs
> + *
> + * Returns:
> + * Attribute name in sysfs
> + */
> +const char *igt_sysfs_path_id_to_name(const char *path, enum i915_attr_id id)
> +{
> +	int dir;
> +	const char *name;
> +
> +	dir = open(path, O_RDONLY);
> +	igt_assert(dir);
> +
> +	name = igt_sysfs_dir_id_to_name(dir, id);
> +	close(dir);
> +
> +	return name;
> +}
> +
>  /**
>   * igt_sysfs_has_attr:
>   * @dir: sysfs directory fd
> diff --git a/lib/igt_sysfs.h b/lib/igt_sysfs.h
> index 33317a969619..8e39b8fa9890 100644
> --- a/lib/igt_sysfs.h
> +++ b/lib/igt_sysfs.h
> @@ -38,11 +38,65 @@
>  	     (dirfd__ = igt_sysfs_gt_open(i915__, gt__)) != -1; \
>  	     close(dirfd__), gt__++)
>  
> +#define igt_sysfs_rps_write(dir, id, data, len) \
> +	igt_sysfs_write(dir, igt_sysfs_dir_id_to_name(dir, id), data, len)
> +
> +#define igt_sysfs_rps_read(dir, id, data, len) \
> +	igt_sysfs_read(dir, igt_sysfs_dir_id_to_name(dir, id), data, len)
> +
> +#define igt_sysfs_rps_set(dir, id, value) \
> +	igt_sysfs_set(dir, igt_sysfs_dir_id_to_name(dir, id), value)
> +
> +#define igt_sysfs_rps_get(dir, id) \
> +	igt_sysfs_get(dir, igt_sysfs_dir_id_to_name(dir, id))
> +
> +#define igt_sysfs_rps_scanf(dir, id, fmt, ...) \
> +	igt_sysfs_scanf(dir, igt_sysfs_dir_id_to_name(dir, id), fmt, ##__VA_ARGS__)
> +
> +#define igt_sysfs_rps_vprintf(dir, id, fmt, ap) \
> +	igt_sysfs_vprintf(dir, igt_sysfs_dir_id_to_name(id), fmt, ap)
> +
> +#define igt_sysfs_rps_printf(dir, id, fmt, ...) \
> +	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))
> +
> +#define igt_sysfs_rps_set_u32(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))
> +
> +#define igt_sysfs_rps_set_boolean(dir, id, value) \
> +	igt_sysfs_set_boolean(dir, igt_sysfs_dir_id_to_name(dir, id), value)
> +
> +enum i915_attr_id {
> +	RPS_ACT_FREQ_MHZ,
> +	RPS_CUR_FREQ_MHZ,
> +	RPS_MIN_FREQ_MHZ,
> +	RPS_MAX_FREQ_MHZ,
> +	RPS_RP0_FREQ_MHZ,
> +	RPS_RP1_FREQ_MHZ,
> +	RPS_RPn_FREQ_MHZ,
> +	RPS_IDLE_FREQ_MHZ,
> +	RPS_BOOST_FREQ_MHZ,
> +	RC6_ENABLE,
> +	RC6_RESIDENCY_MS,
> +	RC6P_RESIDENCY_MS,
> +	RC6PP_RESIDENCY_MS,
> +	MEDIA_RC6_RESIDENCY_MS,
> +
> +	SYSFS_NUM_ATTR,
> +};
> +
>  char *igt_sysfs_path(int device, char *path, int pathlen);
>  int igt_sysfs_open(int device);
>  char *igt_sysfs_gt_path(int device, int gt, char *path, int pathlen);
>  int igt_sysfs_gt_open(int device, int gt);
>  bool igt_sysfs_has_attr(int dir, const char *attr);
> +const char *igt_sysfs_dir_id_to_name(int dir, enum i915_attr_id id);
> +const char *igt_sysfs_path_id_to_name(const char *path, enum i915_attr_id id);
>  
>  int igt_sysfs_read(int dir, const char *attr, void *data, int len);
>  int igt_sysfs_write(int dir, const char *attr, const void *data, int len);
> -- 
> 2.34.1
> 

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

* Re: [igt-dev] [PATCH i-g-t 3/3] tests/i915_pm_disag_freq: New test for media freq factor
  2022-04-20  6:12 ` [igt-dev] [PATCH i-g-t 3/3] tests/i915_pm_disag_freq: New test for media freq factor Ashutosh Dixit
@ 2022-04-21 17:00   ` Kamil Konieczny
  2022-04-26 19:45     ` Dixit, Ashutosh
  2022-04-22 16:15   ` Kamil Konieczny
  1 sibling, 1 reply; 12+ messages in thread
From: Kamil Konieczny @ 2022-04-21 17:00 UTC (permalink / raw)
  To: igt-dev

Hi Ahutosh,

On 2022-04-19 at 23:12:51 -0700, Ashutosh Dixit wrote:
> XEHPSDV and DG2/ATS-M allow media IP blocks to run at frequencies different
> from the GT frequency. i915 exposes sysfs controls for this frequency
> "disaggregation". IGT's introduced in this patch exercise and verify these
-- ^
I am not happy with that name here and with use of it in test
name, there are many SoC with different clock dividers and/or
multipliers and none of them is "disaggregated", they are just
different IP blocks inside SoC which run with different
frequncies. Maybe just drop this and do s/disag_// and
s/"disaggregation"// and s/"disaggregated"// or use "media"
instead (without colons).
If you insist you can keep this and better describe this term.

> per-gt (gt/gtN) sysfs attributes.
> 
> Further, RPS defaults exposed in gt/gtN/.defaults sysfs directory are used
> in the test to start and complete in the known default state.
> 
> v2: Added igt_describe's and s/igt_info/igt_debug/  (Kamil)
> 
> Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
> Cc: Anshuman Gupta <anshuman.gupta@intel.com>
> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> ---
>  tests/i915/i915_pm_disag_freq.c | 186 ++++++++++++++++++++++++++++++++
>  tests/meson.build               |   8 ++
>  2 files changed, 194 insertions(+)
>  create mode 100644 tests/i915/i915_pm_disag_freq.c
> 
> diff --git a/tests/i915/i915_pm_disag_freq.c b/tests/i915/i915_pm_disag_freq.c
> new file mode 100644
> index 000000000000..af216eb98815
> --- /dev/null
> +++ b/tests/i915/i915_pm_disag_freq.c
> @@ -0,0 +1,186 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2022 Intel Corporation
> + */
> +
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <fcntl.h>
> +
> +#include "i915/gem.h"
> +#include "igt.h"
> +#include "igt_sysfs.h"
> +
> +IGT_TEST_DESCRIPTION(
> +	"Tests for sysfs controls for \"disaggregated\" IP blocks, viz. IP "
> +	"blocks which run at frequencies different from the main GT frequency."

Imho this should be rewritten, those "disaggregated" should be
dropped from description, so it will be like

	"Tests for sysfs controls for IP blocks which run at frequencies "
	"different from the main GT frequency."

> +);
> +
> +#define FREQ_SCALE_FACTOR	0.00390625f	/* 1.0f / 256 */
> +
> +/*
> + * Firmware interfaces are not completely synchronous, a delay is needed
> + * before the requested freq is actually set.
> + * Media ratio read back after set will mismatch if this value is too small

Please add note that this is currently set to 0.1s.

> + */
> +#define wait_freq_set()	usleep(100000)
> +
> +static int i915 = -1;
> +const intel_ctx_t *ctx;
> +uint64_t ahnd;
> +
> +static void spin_all(void)
> +{
> +	igt_spin_t *spin = igt_spin_new(i915, .ahnd = ahnd, .ctx = ctx, .engine = ALL_ENGINES,

What about only media engines here ? Or conversly, test without
media engines ? This can go into separate test.

> +					.flags = IGT_SPIN_POLL_RUN);
> +
> +	/* Wait till at least one spinner starts */
> +	igt_spin_busywait_until_started(spin);
> +}
> +
> +static void restore_rps_defaults(int dir)
> +{
> +	int def, min, max, media;
> +
> +	/* 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);
> +
> +	min = igt_sysfs_get_u32(def, "rps_min_freq_mhz");
> +	igt_sysfs_set_u32(dir, "rps_min_freq_mhz", min);
> +
> +	if (igt_sysfs_has_attr(dir, "media_freq_factor")) {
> +		media = igt_sysfs_get_u32(def, "media_freq_factor");
> +		igt_sysfs_set_u32(dir, "media_freq_factor", media);
> +	}
> +
> +	close(def);
> +}
> +
> +static void __restore_rps_defaults(int sig)
> +{
> +	int dir, gt;
> +
> +	for_each_sysfs_gt_dirfd(i915, dir, gt)
> +		restore_rps_defaults(dir);
> +}
> +
> +static void setup_freq(int gt, int dir)
> +{
> +	int rp0, rp1, rpn, min, max, act, media;
> +
> +	ctx = intel_ctx_create_all_physical(i915);
> +	ahnd = get_reloc_ahnd(i915, ctx->id);
> +
> +	/* Reset to known state */
> +	restore_rps_defaults(dir);
> +
> +	/* Spin on all engines to jack freq up to max */
> +	spin_all();
> +	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");
> +
> +	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");
> +		igt_debug("media ratio: %.2f\n", media * FREQ_SCALE_FACTOR);
> +	}
> +}
> +
> +static void cleanup(int dir)
> +{
> +	igt_free_spins(i915);
> +	put_ahnd(ahnd);
> +	intel_ctx_destroy(i915, ctx);
> +	restore_rps_defaults(dir);
> +	gem_quiescent_gpu(i915);
> +}
> +
> +static void media_freq(int gt, int dir)
> +{
> +	float scale;
> +
> +	igt_require(igt_sysfs_has_attr(dir, "media_freq_factor"));
> +
> +	igt_sysfs_scanf(dir, "media_freq_factor.scale", "%g", &scale);
> +	igt_assert_eq(scale, FREQ_SCALE_FACTOR);
> +
> +	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_debug("media ratio value 0.0 represents dynamic mode\n");
> +
> +	/*
> +	 * Media freq ratio modes supported are: dynamic (0), 1:2 (128) and
> +	 * 1:1 (256). Setting dynamic (0) can return any of the three
> +	 * modes. Fixed ratio modes should return the same value.
> +	 */
> +	for (int v = 256; v >= 0; v -= 64) {
> +		int getv, ret;
> +
> +		/*
> +		 * Check that we can set the mode. Ratios other than 1:2
> +		 * and 1:1 are not supported.
> +		 */
> +		ret = igt_sysfs_printf(dir, "media_freq_factor", "%u", v);
> +		if (ret <= 0) {
> +			igt_debug("Media ratio %.2f is not supported\n", v * scale);
> +			continue;
> +		}
> +
> +		wait_freq_set();
> +
> +		getv = igt_sysfs_get_u32(dir, "media_freq_factor");
> +
> +		igt_debug("media ratio set: %.2f, media ratio get: %.2f\n",
> +			  v * scale, getv * scale);
> +
> +		/*
> +		 * Skip validation in dynamic mode since the returned media
> +		 * ratio and freq are platform dependent and not clearly defined
> +		 */
> +		if (v)
> +			igt_assert_eq(getv, v);
> +	}
> +
> +	cleanup(dir);
> +}
> +
> +igt_main
> +{
> +	int dir, gt;
> +
> +	igt_fixture {
> +		i915 = drm_open_driver(DRIVER_INTEL);
> +
> +		/* Disag multipliers (aka "frequency factors") are not simulated. */
> +		igt_require(!igt_run_in_simulation());
> +		igt_install_exit_handler(__restore_rps_defaults);
> +	}
> +
> +	igt_describe("Tests for \"disaggregated\" media freq sysfs");
-------------------------------- ^ -------------------- ^
imho this "dis" word should be dropped and please
s/freq/frequency/

> +	igt_subtest_with_dynamic("media-freq") {
> +		for_each_sysfs_gt_dirfd(i915, dir, gt) {
> +			igt_dynamic_f("gt%d", gt)
> +				media_freq(gt, dir);
> +		}
> +	}
> +
> +	igt_fixture {
> +		close(i915);
> +	}
> +}
> diff --git a/tests/meson.build b/tests/meson.build
> index 7261e9aa2950..cd89defbb418 100644
> --- a/tests/meson.build
> +++ b/tests/meson.build
> @@ -356,6 +356,14 @@ test_executables += executable('gem_mmap_offset',
>  	   install : true)
>  test_list += 'gem_mmap_offset'
>  
> +test_executables += executable('i915_pm_disag_freq',
> +	   join_paths('i915', 'i915_pm_disag_freq.c'),

What about i915_pm_media_freq.c or i915_pm_freq.c or even
i915_pm_ip_freq.c ? Looks better imho.

> +	   dependencies : test_deps + [ lib_igt_perf ],
> +	   install_dir : libexecdir,
> +	   install_rpath : libexecdir_rpathdir,
> +	   install : true)
> +test_list += 'i915_pm_disag_freq'
> +
>  test_executables += executable('i915_pm_rc6_residency',
>  	   join_paths('i915', 'i915_pm_rc6_residency.c'),
>  	   dependencies : test_deps + [ lib_igt_perf ],
> -- 
> 2.34.1
> 
Regards,
Kamil

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

* Re: [igt-dev] [PATCH i-g-t 3/3] tests/i915_pm_disag_freq: New test for media freq factor
  2022-04-20  6:12 ` [igt-dev] [PATCH i-g-t 3/3] tests/i915_pm_disag_freq: New test for media freq factor Ashutosh Dixit
  2022-04-21 17:00   ` Kamil Konieczny
@ 2022-04-22 16:15   ` Kamil Konieczny
  2022-04-26 19:46     ` Dixit, Ashutosh
  1 sibling, 1 reply; 12+ messages in thread
From: Kamil Konieczny @ 2022-04-22 16:15 UTC (permalink / raw)
  To: igt-dev

Hi Ashutosh,

few more nits, see below.

On 2022-04-19 at 23:12:51 -0700, Ashutosh Dixit wrote:
> XEHPSDV and DG2/ATS-M allow media IP blocks to run at frequencies different
> from the GT frequency. i915 exposes sysfs controls for this frequency
> "disaggregation". IGT's introduced in this patch exercise and verify these
> per-gt (gt/gtN) sysfs attributes.
> 
> Further, RPS defaults exposed in gt/gtN/.defaults sysfs directory are used
> in the test to start and complete in the known default state.
> 
> v2: Added igt_describe's and s/igt_info/igt_debug/  (Kamil)
> 
> Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
> Cc: Anshuman Gupta <anshuman.gupta@intel.com>
> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> ---
>  tests/i915/i915_pm_disag_freq.c | 186 ++++++++++++++++++++++++++++++++
>  tests/meson.build               |   8 ++
>  2 files changed, 194 insertions(+)
>  create mode 100644 tests/i915/i915_pm_disag_freq.c
> 
> diff --git a/tests/i915/i915_pm_disag_freq.c b/tests/i915/i915_pm_disag_freq.c
> new file mode 100644
> index 000000000000..af216eb98815
> --- /dev/null
> +++ b/tests/i915/i915_pm_disag_freq.c
> @@ -0,0 +1,186 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2022 Intel Corporation
> + */
> +
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <fcntl.h>
> +
> +#include "i915/gem.h"
> +#include "igt.h"
> +#include "igt_sysfs.h"
> +
> +IGT_TEST_DESCRIPTION(
> +	"Tests for sysfs controls for \"disaggregated\" IP blocks, viz. IP "
> +	"blocks which run at frequencies different from the main GT frequency."
> +);
> +
> +#define FREQ_SCALE_FACTOR	0.00390625f	/* 1.0f / 256 */
> +
> +/*
> + * Firmware interfaces are not completely synchronous, a delay is needed
> + * before the requested freq is actually set.
> + * Media ratio read back after set will mismatch if this value is too small
> + */
> +#define wait_freq_set()	usleep(100000)
> +
> +static int i915 = -1;
> +const intel_ctx_t *ctx;
> +uint64_t ahnd;
> +
> +static void spin_all(void)
> +{
> +	igt_spin_t *spin = igt_spin_new(i915, .ahnd = ahnd, .ctx = ctx, .engine = ALL_ENGINES,
> +					.flags = IGT_SPIN_POLL_RUN);
> +
> +	/* Wait till at least one spinner starts */
> +	igt_spin_busywait_until_started(spin);
> +}
> +
> +static void restore_rps_defaults(int dir)
> +{
> +	int def, min, max, media;
> +
> +	/* 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);
> +
> +	min = igt_sysfs_get_u32(def, "rps_min_freq_mhz");
> +	igt_sysfs_set_u32(dir, "rps_min_freq_mhz", min);
> +
> +	if (igt_sysfs_has_attr(dir, "media_freq_factor")) {
> +		media = igt_sysfs_get_u32(def, "media_freq_factor");
> +		igt_sysfs_set_u32(dir, "media_freq_factor", media);
> +	}
> +
> +	close(def);
> +}
> +
> +static void __restore_rps_defaults(int sig)
> +{
> +	int dir, gt;
> +
> +	for_each_sysfs_gt_dirfd(i915, dir, gt)
> +		restore_rps_defaults(dir);
> +}
> +
> +static void setup_freq(int gt, int dir)
> +{
> +	int rp0, rp1, rpn, min, max, act, media;
> +
> +	ctx = intel_ctx_create_all_physical(i915);
> +	ahnd = get_reloc_ahnd(i915, ctx->id);
> +
> +	/* Reset to known state */
> +	restore_rps_defaults(dir);
> +
> +	/* Spin on all engines to jack freq up to max */
> +	spin_all();
> +	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");
> +
> +	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);
---------------------- ^

When printing please use proper names, so s/mhz/MHz/g

> +
> +	if (igt_sysfs_has_attr(dir, "media_freq_factor")) {
> +		media = igt_sysfs_get_u32(dir, "media_freq_factor");
> +		igt_debug("media ratio: %.2f\n", media * FREQ_SCALE_FACTOR);
> +	}
> +}
> +
> +static void cleanup(int dir)
> +{
> +	igt_free_spins(i915);
> +	put_ahnd(ahnd);
> +	intel_ctx_destroy(i915, ctx);
> +	restore_rps_defaults(dir);
> +	gem_quiescent_gpu(i915);
> +}
> +
> +static void media_freq(int gt, int dir)
> +{
> +	float scale;
> +
> +	igt_require(igt_sysfs_has_attr(dir, "media_freq_factor"));
> +
> +	igt_sysfs_scanf(dir, "media_freq_factor.scale", "%g", &scale);
> +	igt_assert_eq(scale, FREQ_SCALE_FACTOR);
> +
> +	setup_freq(gt, dir);
> +
> +	igt_debug("media RP0 mhz: %d, media RPn mhz: %d\n",
---------------------------- ^ ---------------- ^
Same here.

> +		  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");
> +
> +	/*
> +	 * Media freq ratio modes supported are: dynamic (0), 1:2 (128) and
> +	 * 1:1 (256). Setting dynamic (0) can return any of the three
> +	 * modes. Fixed ratio modes should return the same value.
> +	 */
> +	for (int v = 256; v >= 0; v -= 64) {
> +		int getv, ret;
> +
> +		/*
> +		 * Check that we can set the mode. Ratios other than 1:2
> +		 * and 1:1 are not supported.
> +		 */
> +		ret = igt_sysfs_printf(dir, "media_freq_factor", "%u", v);
> +		if (ret <= 0) {
> +			igt_debug("Media ratio %.2f is not supported\n", v * scale);
> +			continue;
> +		}
> +
> +		wait_freq_set();
> +
> +		getv = igt_sysfs_get_u32(dir, "media_freq_factor");
> +
> +		igt_debug("media ratio set: %.2f, media ratio get: %.2f\n",
> +			  v * scale, getv * scale);

Maybe it is worth to print RPx and actual frequncies here ?

Regards,
Kamil

> +
> +		/*
> +		 * Skip validation in dynamic mode since the returned media
> +		 * ratio and freq are platform dependent and not clearly defined
> +		 */
> +		if (v)
> +			igt_assert_eq(getv, v);
> +	}
> +
> +	cleanup(dir);
> +}
> +
> +igt_main
> +{
> +	int dir, gt;
> +
> +	igt_fixture {
> +		i915 = drm_open_driver(DRIVER_INTEL);
> +
> +		/* Disag multipliers (aka "frequency factors") are not simulated. */
> +		igt_require(!igt_run_in_simulation());
> +		igt_install_exit_handler(__restore_rps_defaults);
> +	}
> +
> +	igt_describe("Tests for \"disaggregated\" media freq sysfs");
> +	igt_subtest_with_dynamic("media-freq") {
> +		for_each_sysfs_gt_dirfd(i915, dir, gt) {
> +			igt_dynamic_f("gt%d", gt)
> +				media_freq(gt, dir);
> +		}
> +	}
> +
> +	igt_fixture {
> +		close(i915);
> +	}
> +}
> diff --git a/tests/meson.build b/tests/meson.build
> index 7261e9aa2950..cd89defbb418 100644
> --- a/tests/meson.build
> +++ b/tests/meson.build
> @@ -356,6 +356,14 @@ test_executables += executable('gem_mmap_offset',
>  	   install : true)
>  test_list += 'gem_mmap_offset'
>  
> +test_executables += executable('i915_pm_disag_freq',
> +	   join_paths('i915', 'i915_pm_disag_freq.c'),
> +	   dependencies : test_deps + [ lib_igt_perf ],
> +	   install_dir : libexecdir,
> +	   install_rpath : libexecdir_rpathdir,
> +	   install : true)
> +test_list += 'i915_pm_disag_freq'
> +
>  test_executables += executable('i915_pm_rc6_residency',
>  	   join_paths('i915', 'i915_pm_rc6_residency.c'),
>  	   dependencies : test_deps + [ lib_igt_perf ],
> -- 
> 2.34.1
> 

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

* Re: [igt-dev] [PATCH i-g-t 2/3] lib/igt_sysfs: Add RPS sysfs helpers
  2022-04-21 15:49   ` Kamil Konieczny
@ 2022-04-25 23:15     ` Dixit, Ashutosh
  0 siblings, 0 replies; 12+ messages in thread
From: Dixit, Ashutosh @ 2022-04-25 23:15 UTC (permalink / raw)
  To: Kamil Konieczny, igt-dev, Ashutosh Dixit, Andi Shyti,
	Tvrtko Ursulin, Sujaritha Sundaresan, Umesh Nerlige Ramappa

On Thu, 21 Apr 2022 08:49:37 -0700, Kamil Konieczny wrote:
>
> Hi Ashutosh,

Hi Kamil,

> > +/**
> > + * igt_sysfs_dir_id_to_name:
> > + * @dir: sysfs directory fd
> > + * @id: sysfs attribute id
> > + *
> > + * Returns attribute name corresponding to attribute id in either the
> > + * per-gt or legacy per-device sysfs
> > + *
> > + * Returns:
> > + * Attribute name in sysfs
> > + */
> > +const char *igt_sysfs_dir_id_to_name(int dir, enum i915_attr_id id)
> > +{
> > +	igt_assert(id < SYSFS_NUM_ATTR);
>
> What about id < 0 ?

Changed to "igt_assert((uint32_t) id < SYSFS_NUM_ATTR);

> > +
> > +	if (igt_sysfs_has_attr(dir, i915_attr_name[GT][id]))
> > +		return i915_attr_name[GT][id];
> > +	else if (igt_sysfs_has_attr(dir, i915_attr_name[RPS][id]))
> ------- ^
> s/else //

Done.

> > +		return i915_attr_name[RPS][id];
> > +	else
> ------- ^
> drop this else here, checkpatch warn:
> WARNING: else is not generally useful after a break or return
> #102: FILE: lib/igt_sysfs.c:118:
> +               return i915_attr_name[RPS][id];
> +       else
>
> > +		igt_assert_f(0, "Invalid sysfs dir\n");
>
> and align this assert.

Done.

>
> Rest looks good, with that fixed you can add my r-b tag.

Thanks, merged with the above changes.

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

* Re: [igt-dev] [PATCH i-g-t 3/3] tests/i915_pm_disag_freq: New test for media freq factor
  2022-04-21 17:00   ` Kamil Konieczny
@ 2022-04-26 19:45     ` Dixit, Ashutosh
  0 siblings, 0 replies; 12+ messages in thread
From: Dixit, Ashutosh @ 2022-04-26 19:45 UTC (permalink / raw)
  To: Kamil Konieczny, igt-dev, Ashutosh Dixit, Anshuman Gupta

On Thu, 21 Apr 2022 10:00:51 -0700, Kamil Konieczny wrote:
>
> Hi Ashutosh,

Hi Kamil,

I have made some changes to this test so that it can get merged in
increments as the kernel patches are reviewed and merged. The new patch is
posted here:

https://patchwork.freedesktop.org/series/103175/

Some sysfs entries which are still under review are removed till they are
approved.

> On 2022-04-19 at 23:12:51 -0700, Ashutosh Dixit wrote:
> > XEHPSDV and DG2/ATS-M allow media IP blocks to run at frequencies different
> > from the GT frequency. i915 exposes sysfs controls for this frequency
> > "disaggregation". IGT's introduced in this patch exercise and verify these
> -- ^
> I am not happy with that name here and with use of it in test
> name, there are many SoC with different clock dividers and/or
> multipliers and none of them is "disaggregated", they are just
> different IP blocks inside SoC which run with different
> frequncies. Maybe just drop this and do s/disag_// and
> s/"disaggregation"// and s/"disaggregated"// or use "media"
> instead (without colons).
> If you insist you can keep this and better describe this term.

Agreed, I have change the name of the test to "i915_pm_freq_mult" (in the
sense of frequency multiplier or factor) and discontinued using the
"disaggregated" term anywhere.

> > +IGT_TEST_DESCRIPTION(
> > +	"Tests for sysfs controls for \"disaggregated\" IP blocks, viz. IP "
> > +	"blocks which run at frequencies different from the main GT frequency."
>
> Imho this should be rewritten, those "disaggregated" should be
> dropped from description, so it will be like
>
>	"Tests for sysfs controls for IP blocks which run at frequencies "
>	"different from the main GT frequency."

Done, please take a look at the new text.

> > +#define FREQ_SCALE_FACTOR	0.00390625f	/* 1.0f / 256 */
> > +
> > +/*
> > + * Firmware interfaces are not completely synchronous, a delay is needed
> > + * before the requested freq is actually set.
> > + * Media ratio read back after set will mismatch if this value is too small
>
> Please add note that this is currently set to 0.1s.
>
> > + */
> > +#define wait_freq_set()	usleep(100000)

I'm skipping this since it is obvious that 0.1 seconds == 100000 us,
don't want to over-comment.

> > +
> > +static int i915 = -1;
> > +const intel_ctx_t *ctx;
> > +uint64_t ahnd;
> > +
> > +static void spin_all(void)
> > +{
> > +	igt_spin_t *spin = igt_spin_new(i915, .ahnd = ahnd, .ctx = ctx, .engine = ALL_ENGINES,
>
> What about only media engines here ? Or conversly, test without
> media engines ? This can go into separate test.

I have mentioned in reply to your other mail that at present the actual
media freq (after setting these factors) is not available on current
platforms but may be available on future platforms. We can make these
changes you suggest or add new tests later if needed after media freq is
available. At present just running the spinner to make sure GT is running
at max freq.

> > +igt_main
> > +{
> > +	int dir, gt;
> > +
> > +	igt_fixture {
> > +		i915 = drm_open_driver(DRIVER_INTEL);
> > +
> > +		/* Disag multipliers (aka "frequency factors") are not simulated. */
> > +		igt_require(!igt_run_in_simulation());
> > +		igt_install_exit_handler(__restore_rps_defaults);
> > +	}
> > +
> > +	igt_describe("Tests for \"disaggregated\" media freq sysfs");
> -------------------------------- ^ -------------------- ^
> imho this "dis" word should be dropped and please
> s/freq/frequency/

Done.

> > diff --git a/tests/meson.build b/tests/meson.build
> > index 7261e9aa2950..cd89defbb418 100644
> > --- a/tests/meson.build
> > +++ b/tests/meson.build
> > @@ -356,6 +356,14 @@ test_executables += executable('gem_mmap_offset',
> >	   install : true)
> >  test_list += 'gem_mmap_offset'
> >
> > +test_executables += executable('i915_pm_disag_freq',
> > +	   join_paths('i915', 'i915_pm_disag_freq.c'),
>
> What about i915_pm_media_freq.c or i915_pm_freq.c or even
> i915_pm_ip_freq.c ? Looks better imho.

Changed to i915_pm_freq_mult.c, let me know if you're ok with this name.

Thanks.
--
Ashutosh

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

* Re: [igt-dev] [PATCH i-g-t 3/3] tests/i915_pm_disag_freq: New test for media freq factor
  2022-04-22 16:15   ` Kamil Konieczny
@ 2022-04-26 19:46     ` Dixit, Ashutosh
  0 siblings, 0 replies; 12+ messages in thread
From: Dixit, Ashutosh @ 2022-04-26 19:46 UTC (permalink / raw)
  To: Kamil Konieczny, igt-dev, Anshuman Gupta, Ashutosh Dixit

On Fri, 22 Apr 2022 09:15:05 -0700, Kamil Konieczny wrote:
>
> > +	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);
> ---------------------- ^
>
> When printing please use proper names, so s/mhz/MHz/g

Done.

> > +	igt_debug("media RP0 mhz: %d, media RPn mhz: %d\n",
> ---------------------------- ^ ---------------- ^
> Same here.

This line is dropped for now but will fix when it's added back.

> > +	for (int v = 256; v >= 0; v -= 64) {
> > +		int getv, ret;
> > +
> > +		/*
> > +		 * Check that we can set the mode. Ratios other than 1:2
> > +		 * and 1:1 are not supported.
> > +		 */
> > +		ret = igt_sysfs_printf(dir, "media_freq_factor", "%u", v);
> > +		if (ret <= 0) {
> > +			igt_debug("Media ratio %.2f is not supported\n", v * scale);
> > +			continue;
> > +		}
> > +
> > +		wait_freq_set();
> > +
> > +		getv = igt_sysfs_get_u32(dir, "media_freq_factor");
> > +
> > +		igt_debug("media ratio set: %.2f, media ratio get: %.2f\n",
> > +			  v * scale, getv * scale);
>
> Maybe it is worth to print RPx and actual frequncies here ?

The problem is the actual media freq after setting the freq factor is not
available on current platforms but may be available on some future
platforms. RPx freq's don't change and they are being printed at the
beginning (now if you run the test with --debug).

Thanks.
--
Ashutosh

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

* [igt-dev] [PATCH i-g-t 3/3] tests/i915_pm_disag_freq: New test for media freq factor
  2022-04-13 17:27 [igt-dev] [PATCH i-g-t 1/2] lib/igt_sysfs: Add helpers to iterate over gts Ashutosh Dixit
@ 2022-04-20  5:02 ` Ashutosh Dixit
  0 siblings, 0 replies; 12+ messages in thread
From: Ashutosh Dixit @ 2022-04-20  5:02 UTC (permalink / raw)
  To: igt-dev

XEHPSDV and DG2/ATS-M allow media IP blocks to run at frequencies different
from the GT frequency. i915 exposes sysfs controls for this frequency
"disaggregation". IGT's introduced in this patch exercise and verify these
per-gt (gt/gtN) sysfs attributes.

Further, RPS defaults exposed in gt/gtN/.defaults sysfs directory are used
in the test to start and complete in the known default state.

v2: Added igt_describe's and s/igt_info/igt_debug/  (Kamil)

Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
Cc: Anshuman Gupta <anshuman.gupta@intel.com>
Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
---
 tests/i915/i915_pm_disag_freq.c | 186 ++++++++++++++++++++++++++++++++
 tests/meson.build               |   8 ++
 2 files changed, 194 insertions(+)
 create mode 100644 tests/i915/i915_pm_disag_freq.c

diff --git a/tests/i915/i915_pm_disag_freq.c b/tests/i915/i915_pm_disag_freq.c
new file mode 100644
index 000000000000..af216eb98815
--- /dev/null
+++ b/tests/i915/i915_pm_disag_freq.c
@@ -0,0 +1,186 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2022 Intel Corporation
+ */
+
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+
+#include "i915/gem.h"
+#include "igt.h"
+#include "igt_sysfs.h"
+
+IGT_TEST_DESCRIPTION(
+	"Tests for sysfs controls for \"disaggregated\" IP blocks, viz. IP "
+	"blocks which run at frequencies different from the main GT frequency."
+);
+
+#define FREQ_SCALE_FACTOR	0.00390625f	/* 1.0f / 256 */
+
+/*
+ * Firmware interfaces are not completely synchronous, a delay is needed
+ * before the requested freq is actually set.
+ * Media ratio read back after set will mismatch if this value is too small
+ */
+#define wait_freq_set()	usleep(100000)
+
+static int i915 = -1;
+const intel_ctx_t *ctx;
+uint64_t ahnd;
+
+static void spin_all(void)
+{
+	igt_spin_t *spin = igt_spin_new(i915, .ahnd = ahnd, .ctx = ctx, .engine = ALL_ENGINES,
+					.flags = IGT_SPIN_POLL_RUN);
+
+	/* Wait till at least one spinner starts */
+	igt_spin_busywait_until_started(spin);
+}
+
+static void restore_rps_defaults(int dir)
+{
+	int def, min, max, media;
+
+	/* 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);
+
+	min = igt_sysfs_get_u32(def, "rps_min_freq_mhz");
+	igt_sysfs_set_u32(dir, "rps_min_freq_mhz", min);
+
+	if (igt_sysfs_has_attr(dir, "media_freq_factor")) {
+		media = igt_sysfs_get_u32(def, "media_freq_factor");
+		igt_sysfs_set_u32(dir, "media_freq_factor", media);
+	}
+
+	close(def);
+}
+
+static void __restore_rps_defaults(int sig)
+{
+	int dir, gt;
+
+	for_each_sysfs_gt_dirfd(i915, dir, gt)
+		restore_rps_defaults(dir);
+}
+
+static void setup_freq(int gt, int dir)
+{
+	int rp0, rp1, rpn, min, max, act, media;
+
+	ctx = intel_ctx_create_all_physical(i915);
+	ahnd = get_reloc_ahnd(i915, ctx->id);
+
+	/* Reset to known state */
+	restore_rps_defaults(dir);
+
+	/* Spin on all engines to jack freq up to max */
+	spin_all();
+	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");
+
+	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");
+		igt_debug("media ratio: %.2f\n", media * FREQ_SCALE_FACTOR);
+	}
+}
+
+static void cleanup(int dir)
+{
+	igt_free_spins(i915);
+	put_ahnd(ahnd);
+	intel_ctx_destroy(i915, ctx);
+	restore_rps_defaults(dir);
+	gem_quiescent_gpu(i915);
+}
+
+static void media_freq(int gt, int dir)
+{
+	float scale;
+
+	igt_require(igt_sysfs_has_attr(dir, "media_freq_factor"));
+
+	igt_sysfs_scanf(dir, "media_freq_factor.scale", "%g", &scale);
+	igt_assert_eq(scale, FREQ_SCALE_FACTOR);
+
+	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_debug("media ratio value 0.0 represents dynamic mode\n");
+
+	/*
+	 * Media freq ratio modes supported are: dynamic (0), 1:2 (128) and
+	 * 1:1 (256). Setting dynamic (0) can return any of the three
+	 * modes. Fixed ratio modes should return the same value.
+	 */
+	for (int v = 256; v >= 0; v -= 64) {
+		int getv, ret;
+
+		/*
+		 * Check that we can set the mode. Ratios other than 1:2
+		 * and 1:1 are not supported.
+		 */
+		ret = igt_sysfs_printf(dir, "media_freq_factor", "%u", v);
+		if (ret <= 0) {
+			igt_debug("Media ratio %.2f is not supported\n", v * scale);
+			continue;
+		}
+
+		wait_freq_set();
+
+		getv = igt_sysfs_get_u32(dir, "media_freq_factor");
+
+		igt_debug("media ratio set: %.2f, media ratio get: %.2f\n",
+			  v * scale, getv * scale);
+
+		/*
+		 * Skip validation in dynamic mode since the returned media
+		 * ratio and freq are platform dependent and not clearly defined
+		 */
+		if (v)
+			igt_assert_eq(getv, v);
+	}
+
+	cleanup(dir);
+}
+
+igt_main
+{
+	int dir, gt;
+
+	igt_fixture {
+		i915 = drm_open_driver(DRIVER_INTEL);
+
+		/* Disag multipliers (aka "frequency factors") are not simulated. */
+		igt_require(!igt_run_in_simulation());
+		igt_install_exit_handler(__restore_rps_defaults);
+	}
+
+	igt_describe("Tests for \"disaggregated\" media freq sysfs");
+	igt_subtest_with_dynamic("media-freq") {
+		for_each_sysfs_gt_dirfd(i915, dir, gt) {
+			igt_dynamic_f("gt%d", gt)
+				media_freq(gt, dir);
+		}
+	}
+
+	igt_fixture {
+		close(i915);
+	}
+}
diff --git a/tests/meson.build b/tests/meson.build
index 7261e9aa2950..cd89defbb418 100644
--- a/tests/meson.build
+++ b/tests/meson.build
@@ -356,6 +356,14 @@ test_executables += executable('gem_mmap_offset',
 	   install : true)
 test_list += 'gem_mmap_offset'
 
+test_executables += executable('i915_pm_disag_freq',
+	   join_paths('i915', 'i915_pm_disag_freq.c'),
+	   dependencies : test_deps + [ lib_igt_perf ],
+	   install_dir : libexecdir,
+	   install_rpath : libexecdir_rpathdir,
+	   install : true)
+test_list += 'i915_pm_disag_freq'
+
 test_executables += executable('i915_pm_rc6_residency',
 	   join_paths('i915', 'i915_pm_rc6_residency.c'),
 	   dependencies : test_deps + [ lib_igt_perf ],
-- 
2.34.1

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

end of thread, other threads:[~2022-04-26 19:46 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-20  6:12 [igt-dev] [PATCH i-g-t 1/3] lib/igt_sysfs: Add helpers to iterate over GTs Ashutosh Dixit
2022-04-20  6:12 ` [igt-dev] [PATCH i-g-t 2/3] lib/igt_sysfs: Add RPS sysfs helpers Ashutosh Dixit
2022-04-21 15:49   ` Kamil Konieczny
2022-04-25 23:15     ` Dixit, Ashutosh
2022-04-20  6:12 ` [igt-dev] [PATCH i-g-t 3/3] tests/i915_pm_disag_freq: New test for media freq factor Ashutosh Dixit
2022-04-21 17:00   ` Kamil Konieczny
2022-04-26 19:45     ` Dixit, Ashutosh
2022-04-22 16:15   ` Kamil Konieczny
2022-04-26 19:46     ` Dixit, Ashutosh
2022-04-20  7:09 ` [igt-dev] ✗ Fi.CI.BAT: failure for series starting with [i-g-t,1/3] lib/igt_sysfs: Add helpers to iterate over GTs Patchwork
2022-04-21 11:05 ` [igt-dev] [PATCH i-g-t 1/3] " Dandamudi, Priyanka
  -- strict thread matches above, loose matches on Subject: below --
2022-04-13 17:27 [igt-dev] [PATCH i-g-t 1/2] lib/igt_sysfs: Add helpers to iterate over gts Ashutosh Dixit
2022-04-20  5:02 ` [igt-dev] [PATCH i-g-t 3/3] tests/i915_pm_disag_freq: New test for media freq factor Ashutosh Dixit

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.