From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTPS id 7D78910E6F0 for ; Sat, 13 May 2023 00:08:31 +0000 (UTC) Date: Fri, 12 May 2023 17:08:24 -0700 From: Umesh Nerlige Ramappa To: Tvrtko Ursulin Message-ID: References: <20230506005528.1890922-1-umesh.nerlige.ramappa@intel.com> <20230506005528.1890922-2-umesh.nerlige.ramappa@intel.com> <87mt2az3j7.wl-ashutosh.dixit@intel.com> <4e5b3e07-8c09-4532-a07c-0818e3650e5d@linux.intel.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Disposition: inline In-Reply-To: <4e5b3e07-8c09-4532-a07c-0818e3650e5d@linux.intel.com> MIME-Version: 1.0 Subject: Re: [igt-dev] [PATCH i-g-t 01/15] perf_pmu: Support multi-tile in rc6 subtest List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: igt-dev@lists.freedesktop.org Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" List-ID: 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 >>> >>>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 >>>Signed-off-by: Umesh Nerlige Ramappa >>>--- >>> 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 >>>