Re: [PATCH i-g-t 1/2] lib/xe: Add sync tag to xe_force_gt_reset

[Matthew Brost <matthew.brost@intel.com>]

On Wed, Jun 05, 2024 at 08:12:47AM -0700, Jonathan Cavitt wrote:
> Add a tag to xe_force_gt_reset that allows the user to decide whether or
> not they want the reset to be synchronous or not.  This is done by
> having the sys call target a new synchronous reset function when the tag
> is set to true.
> 
> For now, default the tag to false for all current use cases of
> xe_force_gt_reset.
> 
> Suggested-by: Matthew Brost <matthew.brost@intel.com>
> Signed-off-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
> CC: John Harrison <john.c.harrison@intel.com>
> CC: Stuart Summers <stuart.summers@intel.com>
> ---
>  lib/xe/xe_gt.c              | 2 +-
>  lib/xe/xe_ioctl.c           | 6 +++---
>  lib/xe/xe_ioctl.h           | 2 +-
>  tests/intel/xe_exec_reset.c | 8 ++++----
>  tests/intel/xe_gt_freq.c    | 2 +-
>  tests/intel/xe_wedged.c     | 2 +-
>  6 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/lib/xe/xe_gt.c b/lib/xe/xe_gt.c
> index 743d7a26ec..36beb596b5 100644
> --- a/lib/xe/xe_gt.c
> +++ b/lib/xe/xe_gt.c
> @@ -69,7 +69,7 @@ void xe_force_gt_reset_all(int xe_fd)
>  	int gt;
>  
>  	xe_for_each_gt(xe_fd, gt)
> -		xe_force_gt_reset(xe_fd, gt);
> +		xe_force_gt_reset(xe_fd, gt, false);

Kinda a nit but I think I'd rather see two functions:

xe_force_gt_reset_async()
xe_force_gt_reset_sync()

Which call the internal function xe_force_gt_reset with correct true /
false sync argument.

This just self documentations IGT the code better. i.e. It is not
obvious for look at 'xe_force_gt_reset(..., false)' is an async reset
while 'xe_force_gt_reset_async' it is.

Another option would be make the last argument a 'int flags' and define
RESET_ASYNC / RESET_SYNC flags. Again this better at self documenting
compared a true / false argument which doesn't immediately mean anything
(i.e. requires some to go look at 'xe_force_gt_reset' to figure out what
that argument means).

Matt

>  }
>  
>  /**
> diff --git a/lib/xe/xe_ioctl.c b/lib/xe/xe_ioctl.c
> index 934c877ebc..a44b65e4ff 100644
> --- a/lib/xe/xe_ioctl.c
> +++ b/lib/xe/xe_ioctl.c
> @@ -529,7 +529,7 @@ int64_t xe_wait_ufence(int fd, uint64_t *addr, uint64_t value,
>  	return timeout;
>  }
>  
> -void xe_force_gt_reset(int fd, int gt)
> +void xe_force_gt_reset(int fd, int gt, bool sync)
>  {
>  	char reset_string[128];
>  	struct stat st;
> @@ -537,7 +537,7 @@ void xe_force_gt_reset(int fd, int gt)
>  	igt_assert_eq(fstat(fd, &st), 0);
>  
>  	snprintf(reset_string, sizeof(reset_string),
> -		 "cat /sys/kernel/debug/dri/%d/gt%d/force_reset",
> -		 minor(st.st_rdev), gt);
> +		 "cat /sys/kernel/debug/dri/%d/gt%d/force_reset%s",
> +		 minor(st.st_rdev), gt, sync ? "_sync" : "");
>  	system(reset_string);
>  }
> diff --git a/lib/xe/xe_ioctl.h b/lib/xe/xe_ioctl.h
> index 4d08402e0b..ff5b5d3ed0 100644
> --- a/lib/xe/xe_ioctl.h
> +++ b/lib/xe/xe_ioctl.h
> @@ -91,6 +91,6 @@ int __xe_wait_ufence(int fd, uint64_t *addr, uint64_t value,
>  		     uint32_t exec_queue, int64_t *timeout);
>  int64_t xe_wait_ufence(int fd, uint64_t *addr, uint64_t value,
>  		       uint32_t exec_queue, int64_t timeout);
> -void xe_force_gt_reset(int fd, int gt);
> +void xe_force_gt_reset(int fd, int gt, bool sync);
>  
>  #endif /* XE_IOCTL_H */
> diff --git a/tests/intel/xe_exec_reset.c b/tests/intel/xe_exec_reset.c
> index e47c3730dd..ed67284800 100644
> --- a/tests/intel/xe_exec_reset.c
> +++ b/tests/intel/xe_exec_reset.c
> @@ -239,7 +239,7 @@ test_balancer(int fd, int gt, int class, int n_exec_queues, int n_execs,
>  	}
>  
>  	if (flags & GT_RESET)
> -		xe_force_gt_reset(fd, gt);
> +		xe_force_gt_reset(fd, gt, false);
>  
>  	if (flags & CLOSE_FD) {
>  		if (flags & CLOSE_EXEC_QUEUES) {
> @@ -383,7 +383,7 @@ test_legacy_mode(int fd, struct drm_xe_engine_class_instance *eci,
>  	}
>  
>  	if (flags & GT_RESET)
> -		xe_force_gt_reset(fd, eci->gt_id);
> +		xe_force_gt_reset(fd, eci->gt_id, false);
>  
>  	if (flags & CLOSE_FD) {
>  		if (flags & CLOSE_EXEC_QUEUES) {
> @@ -530,7 +530,7 @@ test_compute_mode(int fd, struct drm_xe_engine_class_instance *eci,
>  	}
>  
>  	if (flags & GT_RESET)
> -		xe_force_gt_reset(fd, eci->gt_id);
> +		xe_force_gt_reset(fd, eci->gt_id, false);
>  
>  	if (flags & CLOSE_FD) {
>  		if (flags & CLOSE_EXEC_QUEUES) {
> @@ -590,7 +590,7 @@ static void do_resets(struct gt_thread_data *t)
>  	while (!*(t->exit)) {
>  		usleep(250000);	/* 250 ms */
>  		(*t->num_reset)++;
> -		xe_force_gt_reset(t->fd, t->gt);
> +		xe_force_gt_reset(t->fd, t->gt, false);
>  	}
>  }
>  
> diff --git a/tests/intel/xe_gt_freq.c b/tests/intel/xe_gt_freq.c
> index ff99b46a08..1cfc248947 100644
> --- a/tests/intel/xe_gt_freq.c
> +++ b/tests/intel/xe_gt_freq.c
> @@ -324,7 +324,7 @@ static void test_reset(int fd, int gt_id, int cycles)
>  		igt_assert_f(get_freq(fd, gt_id, "cur") == rpn,
>  			     "Failed after %d good cycles\n", i);
>  
> -		xe_force_gt_reset(fd, gt_id);
> +		xe_force_gt_reset(fd, gt_id, false);
>  
>  		igt_assert_f(get_freq(fd, gt_id, "min") == rpn,
>  			     "Failed after %d good cycles\n", i);
> diff --git a/tests/intel/xe_wedged.c b/tests/intel/xe_wedged.c
> index aa4a452bfc..4f772a0e10 100644
> --- a/tests/intel/xe_wedged.c
> +++ b/tests/intel/xe_wedged.c
> @@ -31,7 +31,7 @@ static void force_wedged(int fd)
>  	igt_debugfs_write(fd, "fail_gt_reset/probability", "100");
>  	igt_debugfs_write(fd, "fail_gt_reset/times", "2");
>  
> -	xe_force_gt_reset(fd, 0);
> +	xe_force_gt_reset(fd, 0, false);
>  	sleep(1);
>  }
>  
> -- 
> 2.25.1
>