[PATCH] tests/intel/xe_create: Remove the elapsed time validation

[Zbigniew Kempczyński]

On Fri, Aug 02, 2024 at 02:43:09PM +0530, Vivekanandan, Balasubramani wrote:
> On 02.08.2024 07:12, Zbigniew Kempczyński wrote:
> > On Thu, Aug 01, 2024 at 06:03:12PM +0530, Balasubramani Vivekanandan wrote:
> > > Time check used is incorrect. Time limit used for validation is
> > > arbitrarily chosen as 5 seconds with no real resoning behind it.
> > > With different execution environments taking different execution times,
> > > limit can't be fixed and leads to unnecessary hacks.
> > > So remove this time check completely.
> > 
> > MAXTIME picked in the past was chosen as a fraction of created/destroyed
> > exec queues in some (safe) time period. Up to relatively small number
> > of exec queues (up to 512) there was no issue observed. But with larger
> > number of exec queues we noticed kernel lockup (iirc guc related).
> 
> Time limit of 5 seconds is not fitting all platforms and execution
> environments. I would like to avoid hacks to workaround those.
> We can validate the test case by ensuring there were no GPU hangs or wait
> timeouts instead of checking the time taken.
> Measuring the time is more a performance validation rather than
> functionality validation. Better to isolate performance tests from
> functionality tests.
> 
> > 
> > I think keeping some time regime for those operations is good idea, we
> > may notice performance / stability drop if something unexpected will
> > happen in kmd/guc. MAXTIME/MAXEXECQUEUES were chosen as safe values
> > which should be easily handled. If you're removing this check you may
> > delete subtest as well.
> 
> Keeping subtest without time check will ensure the use case works fine
> without causing GPU hangs, kernel lockups or even dmesg warnings.

Agree, you've conviced me. Pick my r-b:

Reviewed-by: Zbigniew Kempczyński <zbigniew.kempczynski at intel.com>

--
Zbigniew

> 
> Regards,
> Bala
> 
> > 
> > --
> > Zbigniew
> > 
> > > 
> > > Signed-off-by: Balasubramani Vivekanandan <balasubramani.vivekanandan at intel.com>
> > > ---
> > >  tests/intel/xe_create.c | 17 +----------------
> > >  1 file changed, 1 insertion(+), 16 deletions(-)
> > > 
> > > diff --git a/tests/intel/xe_create.c b/tests/intel/xe_create.c
> > > index 76ffdcef2..80de07789 100644
> > > --- a/tests/intel/xe_create.c
> > > +++ b/tests/intel/xe_create.c
> > > @@ -157,7 +157,6 @@ enum vm_count {
> > >  };
> > >  
> > >  #define MAXEXECQUEUES 2048
> > > -#define MAXTIME 5
> > >  
> > >  /**
> > >   * SUBTEST: create-execqueues-%s
> > > @@ -175,10 +174,8 @@ enum vm_count {
> > >  static void create_execqueues(int fd, enum exec_queue_destroy ed,
> > >  			      enum vm_count vc)
> > >  {
> > > -	struct timespec tv = { };
> > >  	uint32_t num_engines, exec_queues_per_process, vm;
> > > -	int nproc = sysconf(_SC_NPROCESSORS_ONLN), seconds;
> > > -	int real_timeout = MAXTIME * (vc == SHARED ? 4 : 1);
> > > +	int nproc = sysconf(_SC_NPROCESSORS_ONLN);
> > >  
> > >  	if (vc == SHARED) {
> > >  		fd = drm_reopen_driver(fd);
> > > @@ -189,8 +186,6 @@ static void create_execqueues(int fd, enum exec_queue_destroy ed,
> > >  	exec_queues_per_process = max_t(uint32_t, 1, MAXEXECQUEUES / nproc);
> > >  	igt_debug("nproc: %u, exec_queues per process: %u\n", nproc, exec_queues_per_process);
> > >  
> > > -	igt_nsec_elapsed(&tv);
> > > -
> > >  	igt_fork(n, nproc) {
> > >  		struct drm_xe_engine *engine;
> > >  		uint32_t exec_queue, exec_queues[exec_queues_per_process];
> > > @@ -236,16 +231,6 @@ static void create_execqueues(int fd, enum exec_queue_destroy ed,
> > >  		xe_vm_destroy(fd, vm);
> > >  		drm_close_driver(fd);
> > >  	}
> > > -
> > > -	seconds = igt_seconds_elapsed(&tv);
> > > -	if (seconds > real_timeout) {
> > > -		if (igt_run_in_simulation())
> > > -			igt_info("Creating %d exec_queues took too long: %d [limit: %d] seconds\n",
> > > -				 MAXEXECQUEUES, seconds, real_timeout);
> > > -		else
> > > -			igt_assert_f(false, "Creating %d exec_queues took too long: %d [limit: %d] seconds\n",
> > > -				     MAXEXECQUEUES, seconds, real_timeout);
> > > -	}
> > >  }
> > >  
> > >  /**
> > > -- 
> > > 2.34.1
> > >