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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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.
> 
>>
>>   #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
>>