Re: [PATCH] drm/xe: Make sysfs gt force reset synchronous

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

On Thu, Dec 21, 2023 at 10:46:41PM +0530, Karthik Poosa wrote:
> Wait for gt reset to complete before returning from force_reset
> sysfs call. Without this igt test freq_reset_multiple fails
> sporadically in case xe_guc_pc is not started.
> 
> v2:
> - Rebase.
> - Changed wait for completion to interruptible (Anshuman).
> - Moved timeout to xe_gt.h (Anshuman).
> 

The patch itself makes sense but small issue / NIT with variable
placement / name. Details below.

> Testcase: igt@xe_guc_pc@freq_reset_multiple
> Signed-off-by: Karthik Poosa <karthik.poosa@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_gt.c         | 2 ++
>  drivers/gpu/drm/xe/xe_gt.h         | 1 +
>  drivers/gpu/drm/xe/xe_gt_debugfs.c | 8 ++++++++
>  drivers/gpu/drm/xe/xe_gt_types.h   | 3 +++
>  4 files changed, 14 insertions(+)
> 
> diff --git a/drivers/gpu/drm/xe/xe_gt.c b/drivers/gpu/drm/xe/xe_gt.c
> index 3af2adec1295..47abb9336c58 100644
> --- a/drivers/gpu/drm/xe/xe_gt.c
> +++ b/drivers/gpu/drm/xe/xe_gt.c
> @@ -65,6 +65,7 @@ struct xe_gt *xe_gt_alloc(struct xe_tile *tile)
>  
>  	gt->tile = tile;
>  	gt->ordered_wq = alloc_ordered_workqueue("gt-ordered-wq", 0);
> +	init_completion(&gt->reset_done);
>  
>  	return gt;
>  }
> @@ -633,6 +634,7 @@ static int gt_reset(struct xe_gt *gt)
>  	xe_device_mem_access_put(gt_to_xe(gt));
>  	XE_WARN_ON(err);
>  
> +	complete(&gt->reset_done);
>  	xe_gt_info(gt, "reset done\n");
>  
>  	return 0;
> diff --git a/drivers/gpu/drm/xe/xe_gt.h b/drivers/gpu/drm/xe/xe_gt.h
> index 4486e083f5ef..591f5271c07d 100644
> --- a/drivers/gpu/drm/xe/xe_gt.h
> +++ b/drivers/gpu/drm/xe/xe_gt.h
> @@ -17,6 +17,7 @@
>  			  xe_hw_engine_is_valid((hwe__)))
>  
>  #define CCS_MASK(gt) (((gt)->info.engine_mask & XE_HW_ENGINE_CCS_MASK) >> XE_HW_ENGINE_CCS0)
> +#define XE_GT_RESET_TIMEOUT_MS (msecs_to_jiffies(5 * 1000))
>  
>  #ifdef CONFIG_FAULT_INJECTION
>  #include <linux/fault-inject.h> /* XXX: fault-inject.h is broken */
> diff --git a/drivers/gpu/drm/xe/xe_gt_debugfs.c b/drivers/gpu/drm/xe/xe_gt_debugfs.c
> index c4b67cf09f8f..543515e91c3d 100644
> --- a/drivers/gpu/drm/xe/xe_gt_debugfs.c
> +++ b/drivers/gpu/drm/xe/xe_gt_debugfs.c
> @@ -58,8 +58,16 @@ static int hw_engines(struct seq_file *m, void *data)
>  static int force_reset(struct seq_file *m, void *data)
>  {
>  	struct xe_gt *gt = node_to_gt(m->private);
> +	struct xe_device *xe = gt_to_xe(gt);
> +	long ret;
>  
>  	xe_gt_reset_async(gt);
> +	ret = wait_for_completion_interruptible_timeout(&gt->reset_done,
> +							XE_GT_RESET_TIMEOUT_MS);
> +	if (ret <= 0) {
> +		drm_err(&xe->drm, "gt reset timed out/interrputed, ret %ld\n", ret);
> +		return -ETIMEDOUT;
> +	}
>  
>  	return 0;
>  }
> diff --git a/drivers/gpu/drm/xe/xe_gt_types.h b/drivers/gpu/drm/xe/xe_gt_types.h
> index f74684660475..58396561fd6c 100644
> --- a/drivers/gpu/drm/xe/xe_gt_types.h
> +++ b/drivers/gpu/drm/xe/xe_gt_types.h
> @@ -358,6 +358,9 @@ struct xe_gt {
>  		/** @oob: bitmap with active OOB workaroudns */
>  		unsigned long *oob;
>  	} wa_active;
> +
> +	/** @reset_done : completion for GT reset */
> +	struct completion reset_done;

This should be reset sub-structure:

144         /** @reset: state for GT resets */
145         struct {
146                 /**
147                  * @worker: work so GT resets can done async allowing to reset
148                  * code to safely flush all code paths
149                  */
150                 struct work_struct worker;
151         } reset;


I'd move here and rename variable to 'done' so in the code it is
'reset.done'.

Matt

>  };
>  
>  #endif
> -- 
> 2.25.1
>