[PATCH i-g-t 1/2] tests/intel/xe_pm: Test to validate vm bind functionality with suspend and resume
Ch, Sai Gowtham
sai.gowtham.ch at intel.com
Fri Mar 22 17:12:53 UTC 2024
>-----Original Message-----
>From: Vivi, Rodrigo <rodrigo.vivi at intel.com>
>Sent: Friday, March 22, 2024 2:08 AM
>To: Ch, Sai Gowtham <sai.gowtham.ch at intel.com>
>Cc: igt-dev at lists.freedesktop.org
>Subject: Re: [PATCH i-g-t 1/2] tests/intel/xe_pm: Test to validate vm bind
>functionality with suspend and resume
>
>On Fri, Mar 15, 2024 at 10:35:35AM +0530, sai.gowtham.ch at intel.com wrote:
>> From: Sai Gowtham Ch <sai.gowtham.ch at intel.com>
>>
>> Test validates vm bind functionality, by suspend and resuming the
>> device after binding VM to a VA.
>>
>> Signed-off-by: Sai Gowtham Ch <sai.gowtham.ch at intel.com>
>> ---
>> tests/intel/xe_pm.c | 103
>> ++++++++++++++++++++++++++++++++++++--------
>> 1 file changed, 84 insertions(+), 19 deletions(-)
>>
>> diff --git a/tests/intel/xe_pm.c b/tests/intel/xe_pm.c index
>> fcbed6249..ac402e757 100644
>> --- a/tests/intel/xe_pm.c
>> +++ b/tests/intel/xe_pm.c
>> @@ -33,6 +33,9 @@
>> #define MAGIC_1 0xc0ffee
>> #define MAGIC_2 0xdeadbeef
>>
>> +#define USERPTR (0x1 << 0)
>> +#define PREFETCH (0x1 << 5)
>
>why 0 and 5?
>
>> +
>> typedef struct {
>> int fd_xe;
>> struct pci_device *pci_xe;
>> @@ -273,10 +276,28 @@ static void close_fw_handle(int sig)
>> * @d3cold: d3cold
>> */
>>
>> +/**
>> + * SUBTEST: %s-vm-bind-%s
>> + * DESCRIPTION: Test to check suspend/autoresume on %arg[1] state
>> + * with vm bind %arg[2] combination
>> + * Functionality: pm - %arg[1]
>> + *
>> + * arg[1]:
>> + *
>> + * @s2idle: s2idle
>> + * @s3: s3
>> + * @s4: s4
>> + *
>> + * arg[2]:
>> + *
>> + * @basic: basic
>> + * @usrptr: usrptr
>> + * @prefetch: prefetch
>> + */
>> static void
>> test_exec(device_t device, struct drm_xe_engine_class_instance *eci,
>> int n_exec_queues, int n_execs, enum igt_suspend_state s_state,
>> - enum igt_acpi_d_state d_state)
>> + enum igt_acpi_d_state d_state, unsigned int flags, bool test_vm)
>> {
>> uint32_t vm;
>> uint64_t addr = 0x1a0000;
>> @@ -308,7 +329,6 @@ test_exec(device_t device, struct
>> drm_xe_engine_class_instance *eci,
>>
>> if (check_rpm)
>> igt_assert(in_d3(device, d_state));
>> -
>> vm = xe_vm_create(device.fd_xe, 0, 0);
>>
>> if (check_rpm)
>> @@ -320,21 +340,45 @@ test_exec(device_t device, struct
>drm_xe_engine_class_instance *eci,
>> if (check_rpm && runtime_usage_available(device.pci_xe))
>> rpm_usage = igt_pm_get_runtime_usage(device.pci_xe);
>>
>> - bo = xe_bo_create(device.fd_xe, vm, bo_size,
>> - vram_if_possible(device.fd_xe, eci->gt_id),
>> - DRM_XE_GEM_CREATE_FLAG_NEEDS_VISIBLE_VRAM);
>> - data = xe_bo_map(device.fd_xe, bo, bo_size);
>> + if (flags & USERPTR)
>> + data = aligned_alloc(xe_get_default_alignment(device.fd_xe),
>bo_size);
>> + else {
>> + if (flags & PREFETCH)
>> + bo = xe_bo_create(device.fd_xe, 0, bo_size,
>> + all_memory_regions(device.fd_xe) |
>> + vram_if_possible(device.fd_xe, 0),
>> +
>DRM_XE_GEM_CREATE_FLAG_NEEDS_VISIBLE_VRAM);
>> + else
>> + bo = xe_bo_create(device.fd_xe, 0, bo_size,
>> + vram_if_possible(device.fd_xe, eci-
>>gt_id),
>> +
>DRM_XE_GEM_CREATE_FLAG_NEEDS_VISIBLE_VRAM);
>> + data = xe_bo_map(device.fd_xe, bo, bo_size);
>> + }
>> + memset(data, 0, bo_size);
>>
>> for (i = 0; i < n_exec_queues; i++) {
>> exec_queues[i] = xe_exec_queue_create(device.fd_xe, vm, eci, 0);
>> bind_exec_queues[i] = 0;
>> syncobjs[i] = syncobj_create(device.fd_xe, 0);
>> - };
>> + }
>>
>> sync[0].handle = syncobj_create(device.fd_xe, 0);
>>
>> - xe_vm_bind_async(device.fd_xe, vm, bind_exec_queues[0], bo, 0, addr,
>> - bo_size, sync, 1);
>> + if (bo) {
>> + xe_vm_bind_async(device.fd_xe, vm, bind_exec_queues[0], bo, 0,
>addr,
>> + bo_size, sync, 1);
>> + } else {
>> + xe_vm_bind_userptr_async(device.fd_xe, vm,
>bind_exec_queues[0],
>> + to_user_pointer(data), addr,
>> + bo_size, sync, 1);
>> + }
>> +
>> + if (flags & PREFETCH)
>> + xe_vm_prefetch_async(device.fd_xe, vm, bind_exec_queues[0], 0,
>addr,
>> + bo_size, sync, 1,
>0);
>
>This patch is changing too much at once without explanation.
>Please split the series. All this userptr and prefetch should be both in separate
>patches.
>
>In the current way it is hard to follow on why you need all the changes above.
>
Will split the changes in to different patches in my next version.
>> +
>> + if (test_vm)
>> + igt_system_suspend_autoresume(s_state,
>SUSPEND_TEST_NONE);
>
>these 2 lines seems to be the only part that is in the commit message itself, and,
>thinking about this while looking the code it looks like this line is not even needed
>because we would already taking care of this need with the
>system_suspend_autoresume call below right along with the exec?
>
>or what is this bringing?
Intension was to do suspend and resume right after vm_binds and after submitting exec, However
to validate vm-bind combination I feel it should be fine to do S&R once should be fine, which test_exec is
already doing, will be removing this line.
Thanks,
Gowtham
>
>>
>> if (check_rpm && runtime_usage_available(device.pci_xe))
>> igt_assert(igt_pm_get_runtime_usage(device.pci_xe) >
>rpm_usage); @@
>> -398,9 +442,10 @@ NULL));
>> xe_exec_queue_destroy(device.fd_xe,
>bind_exec_queues[i]);
>> }
>>
>> - munmap(data, bo_size);
>> -
>> - gem_close(device.fd_xe, bo);
>> + if (bo) {
>> + munmap(data, bo_size);
>> + gem_close(device.fd_xe, bo);
>> + }
>>
>> if (check_rpm && runtime_usage_available(device.pci_xe))
>> igt_assert(igt_pm_get_runtime_usage(device.pci_xe) <
>rpm_usage); @@
>> -585,6 +630,16 @@ igt_main
>> { NULL },
>> };
>>
>> + const struct vm_op {
>> + const char *name;
>> + unsigned int flags;;
>> + } vm_op[] = {
>> + { "basic", 0 },
>
>perhaps you need a proper for the ops,
>adding the 0 there instead of hardcoding it here?
>
>> + { "usrptr", USERPTR },
>> + { "prefetch", PREFETCH },
>> + { NULL },
>> + };
>> +
>> igt_fixture {
>> memset(&device, 0, sizeof(device));
>> device.fd_xe = drm_open_driver(DRIVER_XE); @@ -594,7 +649,7
>@@
>> igt_main
>>
>> /* Always perform initial once-basic exec checking for health */
>> xe_for_each_engine(device.fd_xe, hwe)
>> - test_exec(device, hwe, 1, 1, NO_SUSPEND, NO_RPM);
>> + test_exec(device, hwe, 1, 1, NO_SUSPEND, NO_RPM, 0,
>0);
>>
>> igt_pm_get_d3cold_allowed(device.pci_slot_name,
>&d3cold_allowed);
>> igt_assert(igt_setup_runtime_pm(device.fd_xe));
>> @@ -611,7 +666,7 @@ igt_main
>> igt_subtest_f("%s-basic-exec", s->name) {
>> xe_for_each_engine(device.fd_xe, hwe)
>> test_exec(device, hwe, 1, 2, s->state,
>> - NO_RPM);
>> + NO_RPM, 0, 0);
>> }
>>
>> igt_subtest_f("%s-exec-after", s->name) { @@ -619,13 +674,13
>@@
>> igt_main
>> SUSPEND_TEST_NONE);
>> xe_for_each_engine(device.fd_xe, hwe)
>> test_exec(device, hwe, 1, 2, NO_SUSPEND,
>> - NO_RPM);
>> + NO_RPM, 0, 0);
>> }
>>
>> igt_subtest_f("%s-multiple-execs", s->name) {
>> xe_for_each_engine(device.fd_xe, hwe)
>> test_exec(device, hwe, 16, 32, s->state,
>> - NO_RPM);
>> + NO_RPM, 0, 0);
>> }
>>
>> for (const struct d_state *d = d_states; d->name; d++) { @@ -
>633,10
>> +688,20 @@ igt_main
>> igt_assert(setup_d3(device, d->state));
>> xe_for_each_engine(device.fd_xe, hwe)
>> test_exec(device, hwe, 1, 2, s->state,
>> - NO_RPM);
>> + NO_RPM, 0, 0);
>> cleanup_d3(device);
>> }
>> }
>> +
>> + for (const struct vm_op *op = vm_op; op->name; op++) {
>> + igt_subtest_f("%s-vm-bind-%s", s->name, op->name) {
>> + xe_for_each_engine(device.fd_xe, hwe)
>> + test_exec(device, hwe, 16, 32, s->state,
>> + NO_RPM, op->flags, 1);
>> + }
>> + }
>> +
>> +
>> }
>>
>> for (const struct d_state *d = d_states; d->name; d++) { @@ -650,7
>> +715,7 @@ igt_main
>> igt_assert(setup_d3(device, d->state));
>> xe_for_each_engine(device.fd_xe, hwe)
>> test_exec(device, hwe, 1, 1,
>> - NO_SUSPEND, d->state);
>> + NO_SUSPEND, d->state, 0, 0);
>> cleanup_d3(device);
>> }
>>
>> @@ -658,7 +723,7 @@ igt_main
>> igt_assert(setup_d3(device, d->state));
>> xe_for_each_engine(device.fd_xe, hwe)
>> test_exec(device, hwe, 16, 32,
>> - NO_SUSPEND, d->state);
>> + NO_SUSPEND, d->state, 0, 0);
>> cleanup_d3(device);
>> }
>> }
>> --
>> 2.39.1
>>
More information about the igt-dev
mailing list