From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by gabe.freedesktop.org (Postfix) with ESMTPS id 2A03410E696 for ; Fri, 12 May 2023 12:14:41 +0000 (UTC) Message-ID: <4e5b3e07-8c09-4532-a07c-0818e3650e5d@linux.intel.com> Date: Fri, 12 May 2023 13:14:37 +0100 MIME-Version: 1.0 Content-Language: en-US To: "Dixit, Ashutosh" , Umesh Nerlige Ramappa References: <20230506005528.1890922-1-umesh.nerlige.ramappa@intel.com> <20230506005528.1890922-2-umesh.nerlige.ramappa@intel.com> <87mt2az3j7.wl-ashutosh.dixit@intel.com> From: Tvrtko Ursulin In-Reply-To: <87mt2az3j7.wl-ashutosh.dixit@intel.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit 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 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. > >> >> #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. >> + >> 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. >> 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. 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 >>