[PATCH i-g-t] tests/intel/xe_pmu: Add function engine activity test
Riana Tauro
riana.tauro at intel.com
Mon Mar 10 11:34:55 UTC 2025
Hi Umesh
On 3/8/2025 4:02 AM, Umesh Nerlige Ramappa wrote:
> Hi Riana,
>
> I am numbering the comments so that I can let you know if any are optional.
>
> On Tue, Mar 04, 2025 at 03:40:19PM +0530, Riana Tauro wrote:
>> Add a test to validate per-function engine activity by
>> running cork on all functions simulatenously
>>
>> Signed-off-by: Riana Tauro <riana.tauro at intel.com>
>> ---
>> tests/intel/xe_pmu.c | 89 ++++++++++++++++++++++++++++++++++++++++++--
>> 1 file changed, 85 insertions(+), 4 deletions(-)
>>
>> diff --git a/tests/intel/xe_pmu.c b/tests/intel/xe_pmu.c
>> index 4584fc7d3..5a93d581a 100644
>> --- a/tests/intel/xe_pmu.c
>> +++ b/tests/intel/xe_pmu.c
>> @@ -14,6 +14,7 @@
>>
>> #include "igt.h"
>> #include "igt_perf.h"
>> +#include "igt_sriov_device.h"
>> #include "igt_sysfs.h"
>>
>> #include "xe/xe_gt.h"
>> @@ -106,7 +107,7 @@ static uint64_t add_format_config(const char
>> *xe_device, const char *format, uin
>> }
>>
>> static uint64_t get_event_config(int xe, unsigned int gt, struct
>> drm_xe_engine_class_instance *eci,
>> - const char *event)
>> + int function, const char *event)
>> {
>> char xe_device[100];
>> uint64_t pmu_config = 0;
>> @@ -122,6 +123,9 @@ static uint64_t get_event_config(int xe, unsigned
>> int gt, struct drm_xe_engine_c
>> pmu_config |= add_format_config(xe_device, "engine_instance",
>> eci->engine_instance);
>> }
>>
>> + if (function != -1)
>> + pmu_config |= add_format_config(xe_device, "function",
>> function);
>
> (1)
> How about just have a new helper that takes in the function parameter:
>
> - Make char xe_device[100] global to this file and initialize it in fixture
> - define new helper, something like:
>
> static uint64_t get_event_config_fn(int xe, unsigned int gt,
> struct drm_xe_engine_class_instance
> *eci,
> uint32_t function,
> const char *event)
> {
> return get_event_config(xe, gt, eci, event) |
> add_format_config(xe_device, "function", function);
> }
>
> For any counters that are not function specific (rc6 etc.), they can use
> the old helper.
yeah this can be done. Will add this
>
>> +
>> return pmu_config;
>> }
>>
>> @@ -143,10 +147,10 @@ static void engine_activity(int fd, struct
>> drm_xe_engine_class_instance *eci, un
>> uint32_t vm;
>> int pmu_fd[2];
>>
>> - config = get_event_config(fd, eci->gt_id, eci, "engine-active-
>> ticks");
>> + config = get_event_config(fd, eci->gt_id, eci, -1, "engine-
>> active-ticks");
>> pmu_fd[0] = open_group(fd, config, -1);
>>
>> - config = get_event_config(fd, eci->gt_id, eci, "engine-total-
>> ticks");
>> + config = get_event_config(fd, eci->gt_id, eci, -1, "engine-total-
>> ticks");
>> pmu_fd[1] = open_group(fd, config, pmu_fd[0]);
>>
>> vm = xe_vm_create(fd, 0, 0);
>> @@ -187,6 +191,75 @@ static void engine_activity(int fd, struct
>> drm_xe_engine_class_instance *eci, un
>> igt_assert(!engine_active_ticks);
>> }
>>
>> +/**
>> + * SUBTEST: fn-engine-activity-load
>> + * Description: Test to validate engine activity by running load on
>> all functions simultaneously
>> + */
>> +static void engine_activity_fn(int fd, struct
>> drm_xe_engine_class_instance *eci, int num_fns)
>> +{
>> + uint64_t config, engine_active_ticks, engine_total_ticks;
>> + int num = num_fns + 1;
>
> (2)
> s/num_fns/num_vfs/ everywhere since it truly is number of vfs.
>
> OR
>
> don't rename, but add 1 to num_fns when you call
> igt_sriov_get_enabled_vfs().
>
sure will follow the second suggestion
>> + uint64_t after[2 * num], before[2 * num], pmu_fd[2 * num];
>> + struct pmu_function {
>> + struct xe_cork *cork;
>> + uint32_t vm;
>> + int fd;
>
> (3)
> why not just include after[2], before[2], pmu_fd[2] inside this struct?
i tried this, before and after cannot be placed in struct because of
pmu_read. i think pmu_fd can be placed. will move that to struct
>
>> + } fn[num];
>> + int i, idx = 0;
>
> If you include everything within the struct, then you can drop idx. Just
> use 0 and 1 index.
>> +
>> + pmu_fd[0] = -1;
>> + for (i = 0; i < num; i++) {
>> + config = get_event_config(fd, eci->gt_id, eci, i, "engine-
>> active-ticks");
>> + pmu_fd[idx] = open_group(fd, config, pmu_fd[0]);
>> +
>> + config = get_event_config(fd, eci->gt_id, eci, i, "engine-
>> total-ticks");
>> + pmu_fd[idx + 1] = open_group(fd, config, pmu_fd[0]);
>> +
>> + if (i > 0)
>> + fn[i].fd = igt_sriov_open_vf_drm_device(fd, i);
>> + else
>> + fn[i].fd = fd;
>> +
>> + fn[i].vm = xe_vm_create(fn[i].fd, 0, 0);
>> + fn[i].cork = xe_cork_create_opts(fn[i].fd, eci, fn[i].vm, 1, 1);
>> + xe_cork_sync_start(fn[i].fd, fn[i].cork);
>> +
>> + idx += 2;
>> + }
>> +
>> + pmu_read_multi(pmu_fd[0], 2 * num, before);
>> + usleep(SLEEP_DURATION * USEC_PER_SEC);
>> + pmu_read_multi(pmu_fd[0], 2 * num, after);
>
> ok, I see why you haven't included before/after/pmu_fd in the struct.
> Maybe you
> can still include pmu_fd within the struct since you are only passing
> pmu_fd[0] to all the calls (which would be fn[0].pmu_fd[0]. That should
> still
> help get rid of idx in the above loop.
yeah this can be done
>
>> + idx = 0;
>> + for (i = 0; i < num; i++) {
>
> For this loop you can use a local idx = num * 2;
>
>> + xe_cork_sync_end(fn[i].fd, fn[i].cork);
>> +
>> + engine_active_ticks = after[idx] - before[idx];
>> + engine_total_ticks = after[idx + 1] - before[idx + 1];
>> +
>> + igt_debug("Engine active ticks: after %ld, before %ld delta
>> %ld\n", after[idx],
>> + before[idx], engine_active_ticks);
>> + igt_debug("Engine total ticks: after %ld, before %ld delta
>> %ld\n", after[idx + 1],
>> + before[idx + 1], engine_total_ticks);
>
> (4) Maybe also print the function num.
>
>> +
>> + if (fn[i].cork)
>> + xe_cork_destroy(fn[i].fd, fn[i].cork);
>> +
>> + xe_vm_destroy(fn[i].fd, fn[i].vm);
>> +
>> + close(pmu_fd[idx]);
>> + close(pmu_fd[idx + 1]);
>> +
>> + if (i > 0)
>> + close(fn[i].fd);
>> +
>> + assert_within_epsilon(engine_active_ticks,
>> engine_total_ticks, tolerance);
>> +
>> + idx += 2;
>> + }
>> +}
>> +
>> /**
>> * SUBTEST: gt-c6-idle
>> * Description: Basic residency test to validate idle residency
>> @@ -201,7 +274,7 @@ static void test_gt_c6_idle(int xe, unsigned int gt)
>> uint64_t val;
>>
>> /* Get the PMU config for the gt-c6 event */
>> - pmu_config = get_event_config(xe, gt, NULL, "gt-c6-residency");
>> + pmu_config = get_event_config(xe, gt, NULL, -1, "gt-c6-residency");
>>
>> pmu_fd = open_pmu(xe, pmu_config);
>>
>> @@ -253,6 +326,14 @@ igt_main
>> test_each_engine("engine-activity-load", fd, eci)
>> engine_activity(fd, eci, TEST_LOAD);
>>
>> + igt_describe("Validate per-function engine activity");
>> + test_each_engine("fn-engine-activity-load", fd, eci) {
>> + int num_fns = igt_sriov_get_enabled_vfs(fd);
>> +
This has to be done after pf check
>> + igt_require(igt_sriov_is_pf(fd) && num_fns);
>> + engine_activity_fn(fd, eci, num_fns);
>> + }
>> +
>> igt_fixture {
>> close(fd);
>> }
>
> From my end, (3) is optional.
>
> Are you able to configure sched_if_idle?
I tried by provisioning the VF's with execution quanta
If the results are for a
> specific sched
> policy, then I would add a igt_require in the test.
Yeah was checking CI, everything failed or skipped.
don't think CI will run this test except when vms are enabled. Will
check if there is any sriov igt lib function to enable the vfs and set
execution quantum or the sched policy.
Thanks
Riana
>
> Thanks,
> Umesh
>
>
>> --
>> 2.47.1
>>
More information about the igt-dev
mailing list