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

["Nilawar, Badal" <badal.nilawar@intel.com>]

On 27-12-2023 14:02, 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:
> - Changed wait for completion to interruptible (Anshuman).
> - Moved timeout to xe_gt.h (Anshuman).
> - Created a debugfs for updating timeout (Rodrigo).
> 
> 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_debugfs.c | 12 ++++++++++++
>   drivers/gpu/drm/xe/xe_gt_types.h   |  6 ++++++
>   3 files changed, 20 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_debugfs.c b/drivers/gpu/drm/xe/xe_gt_debugfs.c
> index c4b67cf09f8f..fbda886c8a95 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,
> +						msecs_to_jiffies(gt->reset_timeout_ms));
For now this is ok but in future if there is need for synchronous reset 
in other places then we should use wait_event here. 	> +	if (ret <= 0) {
> +		drm_err(&xe->drm, "gt reset timed out/interrputed, ret %ld\n", ret);
> +		return -ETIMEDOUT;
> +	}
>   
>   	return 0;
>   }
> @@ -225,6 +233,10 @@ void xe_gt_debugfs_register(struct xe_gt *gt)
>   		return;
>   	}
>   
> +	/* set a default timeout */
> +	gt->reset_timeout_ms = 1000;
> +	debugfs_create_u32("gt_reset_timeout_ms", 0600, root,
> +					&gt->reset_timeout_ms);
May be move this in the end i.e. before calling xe_uc_debugfs_register.
>   	/*
>   	 * Allocate local copy as we need to pass in the GT to the debugfs
>   	 * entry and drm_debugfs_create_files just references the drm_info_list
> diff --git a/drivers/gpu/drm/xe/xe_gt_types.h b/drivers/gpu/drm/xe/xe_gt_types.h
> index f74684660475..824cefde20d2 100644
> --- a/drivers/gpu/drm/xe/xe_gt_types.h
> +++ b/drivers/gpu/drm/xe/xe_gt_types.h
> @@ -358,6 +358,12 @@ struct xe_gt {
>   		/** @oob: bitmap with active OOB workaroudns */
>   		unsigned long *oob;
>   	} wa_active;
> +
> +	/** @reset_done : completion for GT reset */
> +	struct completion reset_done;
> +
> +	/** @gt_reset_timeout_ms : gt reset timeout in ms */
> +	u32 reset_timeout_ms;
Wrap this inside structure.
	struct {
		struct completion done;
		u32 timeout_ms;
	} reset;

Regards,
Badal
>   };
>   
>   #endif