Re: [igt-dev] [PATCH i-g-t 01/15] perf_pmu: Support multi-tile in rc6 subtest

[Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>]

On Fri, May 12, 2023 at 01:14:37PM +0100, Tvrtko Ursulin wrote:
>
>On 12/05/2023 03:28, Dixit, Ashutosh wrote:
>>On Fri, 05 May 2023 17:55:14 -0700, Umesh Nerlige Ramappa wrote:
>>>
>>
>>Hi Umesh/Tvrtko,
>>
>>>From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>
>>>Teach test how to wake up a particular tile and make it iterate all of
>>>them using dynamic subtests.
>>>
>>>v2: Finalize SHIFT to 60. Drop FIXME from i915_drm.h
>>>
>>>Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
>>>---
>>>  include/drm-uapi/i915_drm.h | 17 ++++++++++++++-
>>>  tests/i915/perf_pmu.c       | 41 ++++++++++++++++++++++++++-----------
>>>  2 files changed, 45 insertions(+), 13 deletions(-)
>>>
>>>diff --git a/include/drm-uapi/i915_drm.h b/include/drm-uapi/i915_drm.h
>>>index a0876ee41..e164ad014 100644
>>>--- a/include/drm-uapi/i915_drm.h
>>>+++ b/include/drm-uapi/i915_drm.h
>>>@@ -280,7 +280,16 @@ enum drm_i915_pmu_engine_sample {
>>>  #define I915_PMU_ENGINE_SEMA(class, instance) \
>>>	__I915_PMU_ENGINE(class, instance, I915_SAMPLE_SEMA)
>>>
>>>-#define __I915_PMU_OTHER(x) (__I915_PMU_ENGINE(0xff, 0xff, 0xf) + 1 + (x))
>>>+/*
>>>+ * Top 4 bits of every non-engine counter are GT id.
>>>+ */
>>>+#define __I915_PMU_GT_SHIFT (60)
>>>+
>>>+#define ___I915_PMU_OTHER(gt, x) \
>>>+	(((__u64)__I915_PMU_ENGINE(0xff, 0xff, 0xf) + 1 + (x)) | \
>>>+	((__u64)(gt) << __I915_PMU_GT_SHIFT))
>>>+
>>>+#define __I915_PMU_OTHER(x) ___I915_PMU_OTHER(0, x)
>>
>>Typically we don't modify include/drm-uapi/i915_drm.h directly, it is
>>sync'd with the kernel.
>>
>>So maybe let's add the above to lib/i915/i915_drm_local.h.

sure

>>
>>>
>>>  #define I915_PMU_ACTUAL_FREQUENCY	__I915_PMU_OTHER(0)
>>>  #define I915_PMU_REQUESTED_FREQUENCY	__I915_PMU_OTHER(1)
>>>@@ -290,6 +299,12 @@ enum drm_i915_pmu_engine_sample {
>>>
>>>  #define I915_PMU_LAST /* Deprecated - do not use */ I915_PMU_RC6_RESIDENCY
>>>
>>>+#define __I915_PMU_ACTUAL_FREQUENCY(gt)		___I915_PMU_OTHER(gt, 0)
>>>+#define __I915_PMU_REQUESTED_FREQUENCY(gt)	___I915_PMU_OTHER(gt, 1)
>>>+#define __I915_PMU_INTERRUPTS(gt)		___I915_PMU_OTHER(gt, 2)
>>>+#define __I915_PMU_RC6_RESIDENCY(gt)		___I915_PMU_OTHER(gt, 3)
>>>+#define __I915_PMU_SOFTWARE_GT_AWAKE_TIME(gt)	___I915_PMU_OTHER(gt, 4)
>>>+
>>>  /* Each region is a minimum of 16k, and there are at most 255 of them.
>>>   */
>>>  #define I915_NR_TEX_REGIONS 255	/* table size 2k - maximum due to use
>>>diff --git a/tests/i915/perf_pmu.c b/tests/i915/perf_pmu.c
>>>index c5f083bbd..97ad09d76 100644
>>>--- a/tests/i915/perf_pmu.c
>>>+++ b/tests/i915/perf_pmu.c
>>>@@ -1707,8 +1707,16 @@ static bool wait_for_suspended(int gem_fd)
>>>	return suspended;
>>>  }
>>>
>>>+static int open_forcewake_handle(int fd, unsigned int gt)
>>>+{
>>>+	if (getenv("IGT_NO_FORCEWAKE"))
>>>+		return -1;
>>>+
>>>+	return igt_debugfs_gt_open(fd, gt, "forcewake_user", O_WRONLY);
>>>+}
>>
>>Let's create a new function igt_open_forcewake_gt_handle() below
>>igt_open_forcewake_handle() in lib/igt_gt.c and add this code there so the
>>code can be shared.
>
>Typically we'd move to lib/ only when there are 2-3 callers and so it 
>is clear helper is useful. Don't know, I am okay either way.
>

leaving as is

>>>+
>>>  static void
>>>-test_rc6(int gem_fd, unsigned int flags)
>>>+test_rc6(int gem_fd, unsigned int gt, unsigned int flags)
>>>  {
>>>	int64_t duration_ns = 2e9;
>>>	uint64_t idle, busy, prev, ts[2];
>>>@@ -1717,7 +1725,7 @@ test_rc6(int gem_fd, unsigned int flags)
>>>
>>>	gem_quiescent_gpu(gem_fd);
>>>
>>>-	fd = open_pmu(gem_fd, I915_PMU_RC6_RESIDENCY);
>>>+	fd = open_pmu(gem_fd, __I915_PMU_RC6_RESIDENCY(gt));
>>>
>>>	if (flags & TEST_RUNTIME_PM) {
>>>		drmModeRes *res;
>>>@@ -1784,8 +1792,8 @@ test_rc6(int gem_fd, unsigned int flags)
>>>	assert_within_epsilon(idle - prev, ts[1] - ts[0], tolerance);
>>>
>>>	/* Wake up device and check no RC6. */
>>>-	fw = igt_open_forcewake_handle(gem_fd);
>>>-	igt_assert(fw >= 0);
>>>+	fw = open_forcewake_handle(gem_fd, gt);
>>>+	igt_require(fw >= 0);
>>
>>Why not igt_assert?
>
>It probably was to support running the test on old kernels. Although I 
>am not sure if recently we have been disciplined enough with this 
>requirement.

If it asserted before, then maybe should assert now too. Changing to 
assert.

>
>>>	usleep(1e3); /* wait for the rc6 cycle counter to stop ticking */
>>>
>>>	prev = pmu_read_single(fd);
>>>@@ -2174,12 +2182,17 @@ static void pmu_read(int i915)
>>>		for_each_if((e)->class == I915_ENGINE_CLASS_RENDER) \
>>>			igt_dynamic_f("%s", e->name)
>>>
>>>+#define for_each_gt(i915, gtid, tmp) \
>>>+	for ((gtid) = 0; \
>>>+	     ((tmp) = igt_sysfs_gt_open((i915), (gtid))) != -1; \
>>>+	     close(tmp), (gtid)++)
>>
>>Use i915_for_each_gt from lib/ here.
>
>Yeah I guess these patches predate much of the IGT code added for 
>supporting multi-tile in the upstream since. Shrug.

changing this.

Thanks,
Umesh
>
>Regards,
>
>Tvrtko
>
>>
>>Thanks.
>>--
>>Ashutosh
>>
>>>+
>>>  igt_main
>>>  {
>>>	const struct intel_execution_engine2 *e;
>>>	unsigned int num_engines = 0;
>>>	const intel_ctx_t *ctx = NULL;
>>>-	int fd = -1;
>>>+	int gt, tmp, fd = -1;
>>>
>>>	/**
>>>	 * All PMU should be accompanied by a test.
>>>@@ -2396,17 +2409,21 @@ igt_main
>>>	/**
>>>	 * Test RC6 residency reporting.
>>>	 */
>>>-	igt_subtest("rc6")
>>>-		test_rc6(fd, 0);
>>>+	igt_subtest_with_dynamic("rc6") {
>>>+		for_each_gt(fd, gt, tmp) {
>>>+			igt_dynamic_f("gt%u", gt)
>>>+				test_rc6(fd, gt, 0);
>>>
>>>-	igt_subtest("rc6-runtime-pm")
>>>-		test_rc6(fd, TEST_RUNTIME_PM);
>>>+			igt_dynamic_f("runtime-pm-gt%u", gt)
>>>+				test_rc6(fd, gt, TEST_RUNTIME_PM);
>>>
>>>-	igt_subtest("rc6-runtime-pm-long")
>>>-		test_rc6(fd, TEST_RUNTIME_PM | FLAG_LONG);
>>>+			igt_dynamic_f("runtime-pm-long-gt%u", gt)
>>>+				test_rc6(fd, gt, TEST_RUNTIME_PM | FLAG_LONG);
>>>+		}
>>>+	}
>>>
>>>	igt_subtest("rc6-suspend")
>>>-		test_rc6(fd, TEST_S3);
>>>+		test_rc6(fd, 0, TEST_S3);
>>>
>>>	/**
>>>	 * Test GT wakeref tracking (similar to RC0, opposite of RC6)
>>>--
>>>2.34.1
>>>