Re: [PATCH i-g-t 1/2] tests/intel/xe_pm: Test to validate vm bind functionality with suspend and resume

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Fri, Mar 15, 2024 at 10:35:35AM +0530, sai.gowtham.ch@intel.com wrote:
> From: Sai Gowtham Ch <sai.gowtham.ch@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@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.

> +
> +	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?

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