Re: [PATCH i-g-t 3/3] tests/intel/xe_pm: Tests vm-unbind all flag functionality with s&R

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

On Tue, Mar 26, 2024 at 01:40:45AM +0530, sai.gowtham.ch@intel.com wrote:
> From: Sai Gowtham Ch <sai.gowtham.ch@intel.com>
> 
> Test validates vm unbind all flag functionality with suspend and resume
> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Sai Gowtham Ch <sai.gowtham.ch@intel.com>
> ---
>  tests/intel/xe_pm.c | 61 ++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 46 insertions(+), 15 deletions(-)
> 
> diff --git a/tests/intel/xe_pm.c b/tests/intel/xe_pm.c
> index e016f2bca..7a9816623 100644
> --- a/tests/intel/xe_pm.c
> +++ b/tests/intel/xe_pm.c
> @@ -292,10 +292,23 @@ static void close_fw_handle(int sig)
>   * @usrptr:     usrptr
>   * @prefetch:  prefetch
>   */
> +
> +/**
> + * SUBTEST: %s-vm-unbind-all
> + * Description: Validate vm unbind all flag functionality with suspend and resume
> + *
> + * Functionality: pm - %arg[1]
> + *
> + * arg[1]:
> + *
> + * @s2idle:     s2idle
> + * @s3:         s3
> + * @s4:         s4
> + */
>  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, unsigned int flags)
> +	  enum igt_acpi_d_state d_state, unsigned int flags, int n_vmas)

instead passing another argument for a value that is either 0 (1?) or 2,
create a new operation flag and pass in the existing 'flags' input.
then create a
#define MAX_VMAS 2

then, inside the function:

int n_vmas = flags & UNBIND_ALL ? 2 : 1;

>  {
>  	uint32_t vm;
>  	uint64_t addr = 0x1a0000;
> @@ -364,12 +377,19 @@ test_exec(device_t device, struct drm_xe_engine_class_instance *eci,
>  
>  	sync[0].handle = syncobj_create(device.fd_xe, 0);
>  
> -	if (bo)
> -		xe_vm_bind_async(device.fd_xe, vm, bind_exec_queues[0], bo, 0, addr,
> -				 bo_size, sync, 1);
> -	else
> +	if (bo) {
> +		if (n_vmas) {

please don't do that.
use n_vmas 1 or 2 and then make the for to run only once for 1 and twice for 2:

> +			for (i = 0; i < n_vmas; i++)
> +				xe_vm_bind_async(device.fd_xe, vm, bind_exec_queues[0], bo, 0, addr,
> +						 bo_size, sync, 1);

I'm afraid you have forgotten to use the 'i' in here?
s/addr/addr + i * bo_size/ perhaps?

> +		} else {
> +			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,
> @@ -421,8 +441,13 @@ test_exec(device_t device, struct drm_xe_engine_class_instance *eci,
>  		rpm_usage = igt_pm_get_runtime_usage(device.pci_xe);
>  
>  	sync[0].flags |= DRM_XE_SYNC_FLAG_SIGNAL;
> -	xe_vm_unbind_async(device.fd_xe, vm, bind_exec_queues[0], 0, addr,
> -			   bo_size, sync, 1);
> +
> +	if (n_vmas)
> +		xe_vm_unbind_all_async(device.fd_xe, vm, 0, bo, sync, 1);
> +	else
> +		xe_vm_unbind_async(device.fd_xe, vm, bind_exec_queues[0], 0, addr,
> +				   bo_size, sync, 1);
> +
>  	igt_assert(syncobj_wait(device.fd_xe, &sync[0].handle, 1, INT64_MAX, 0,
>  NULL));
>  
> @@ -641,7 +666,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, 0);
> +			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));
> @@ -658,7 +683,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, 0);
> +					  NO_RPM, 0, 0);
>  		}
>  
>  		igt_subtest_f("%s-exec-after", s->name) {
> @@ -666,29 +691,35 @@ igt_main
>  						      SUSPEND_TEST_NONE);
>  			xe_for_each_engine(device.fd_xe, hwe)
>  				test_exec(device, hwe, 1, 2, NO_SUSPEND,
> -					  NO_RPM, 0);
> +					  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, 0);
> +					  NO_RPM, 0, 0);
>  		}
>  
>  		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);
> +						  NO_RPM, op->flags, 0);
>  			}
>  		}
>  
> +		igt_subtest_f("%s-vm-unbind-all", s->name) {
> +			xe_for_each_engine(device.fd_xe, hwe)
> +				test_exec(device, hwe, 16, 32, s->state, NO_RPM,
> +					  0, 2);
> +		}
> +
>  		for (const struct d_state *d = d_states; d->name; d++) {
>  			igt_subtest_f("%s-%s-basic-exec", s->name, d->name) {
>  				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, 0);
> +						  NO_RPM, 0, 0);
>  				cleanup_d3(device);
>  			}
>  		}
> @@ -705,7 +736,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, 0);
> +					  NO_SUSPEND, d->state, 0, 0);
>  			cleanup_d3(device);
>  		}
>  
> @@ -713,7 +744,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, 0);
> +					  NO_SUSPEND, d->state, 0, 0);
>  			cleanup_d3(device);
>  		}
>  	}
> -- 
> 2.39.1
>