Re: [PATCH] drm/xe: Add debugfs knob to disable Mid-Thread Preemption per GT

[Gustavo Sousa <gustavo.sousa@intel.com>]

Varun Gupta <varun.gupta@intel.com> writes:

> Add a per-GT debugfs entry 'disable_mtp' that forces thread-group
> preemption granularity on Xe2+ RCS/CCS engines by writing CS_CHICKEN1
> via MI_LRI into the WA BB of each newly created LRC.

Hm.  I think it would probably be better to make this an entry to select
the preemption level instead of simply making it disabled mid-thread
preemption.  So, we could call is "preemption_level" and then allow the
following values:

  - "default": Use the platform's default.
  - "mid-thread": For mid-thread preemption level.
  - "thread-group": For thread group preemption level.
  - "command": For command preemption level.

Furthermore, should we consider tainting the kernel if ever get a write
on that entry for any the last 3 values, since it will deviate from the
default and could be incompatible with some workloads?

>
> Signed-off-by: Varun Gupta <varun.gupta@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_gt_debugfs.c | 32 ++++++++++++++++++++++++++++++
>  drivers/gpu/drm/xe/xe_gt_types.h   |  8 ++++++++
>  drivers/gpu/drm/xe/xe_lrc.c        | 24 ++++++++++++++++++++++
>  3 files changed, 64 insertions(+)
>
> diff --git a/drivers/gpu/drm/xe/xe_gt_debugfs.c b/drivers/gpu/drm/xe/xe_gt_debugfs.c
> index f45306308cd6..e8f7ba6fc3c1 100644
> --- a/drivers/gpu/drm/xe/xe_gt_debugfs.c
> +++ b/drivers/gpu/drm/xe/xe_gt_debugfs.c
> @@ -334,6 +334,37 @@ static int force_reset_sync_show(struct seq_file *s, void *unused)
>  }
>  DEFINE_SHOW_STORE_ATTRIBUTE(force_reset_sync);
>  
> +static int disable_mtp_get(void *data, u64 *val)
> +{
> +	struct xe_gt *gt = data;
> +
> +	*val = gt->disable_mtp;
> +
> +	return 0;
> +}
> +
> +static int disable_mtp_set(void *data, u64 val)
> +{
> +	struct xe_gt *gt = data;
> +	struct xe_device *xe = gt_to_xe(gt);
> +
> +	if (GRAPHICS_VER(xe) < 20)
> +		return -EOPNOTSUPP;
> +
> +	if (!(gt->info.engine_mask & (XE_HW_ENGINE_RCS_MASK | XE_HW_ENGINE_CCS_MASK)))
> +		return -EOPNOTSUPP;
> +
> +	gt->disable_mtp = !!val;
> +	drm_dbg(&gt_to_xe(gt)->drm, "GT%u: disable_mtp=%u: new LRCs will %s MTP\n",
> +		gt->info.id, !!val, !!val ? "disable" : "enable");
> +
> +	return 0;
> +}
> +DEFINE_DEBUGFS_ATTRIBUTE(disable_mtp_fops,
> +			 disable_mtp_get,
> +			 disable_mtp_set,
> +			 "%llu\n");
> +
>  void xe_gt_debugfs_register(struct xe_gt *gt)
>  {
>  	struct xe_device *xe = gt_to_xe(gt);
> @@ -366,6 +397,7 @@ void xe_gt_debugfs_register(struct xe_gt *gt)
>  	debugfs_create_file("stats", 0600, root, gt, &stats_fops);
>  	debugfs_create_file("force_reset", 0600, root, gt, &force_reset_fops);
>  	debugfs_create_file("force_reset_sync", 0600, root, gt, &force_reset_sync_fops);
> +	debugfs_create_file("disable_mtp", 0600, root, gt, &disable_mtp_fops);

Wouldn't it be better if we added the entry as part of either
vf_safe_debugfs_list or pf_only_debugfs_list?

I'm just not sure about which of the two it should belong to.

>  
>  	drm_debugfs_create_files(vf_safe_debugfs_list,
>  				 ARRAY_SIZE(vf_safe_debugfs_list),
> diff --git a/drivers/gpu/drm/xe/xe_gt_types.h b/drivers/gpu/drm/xe/xe_gt_types.h
> index e5588c88800a..4f6d63b614f9 100644
> --- a/drivers/gpu/drm/xe/xe_gt_types.h
> +++ b/drivers/gpu/drm/xe/xe_gt_types.h
> @@ -219,6 +219,14 @@ struct xe_gt {
>  	 */
>  	u32 ccs_mode;
>  
> +	/**
> +	 * @disable_mtp: Force thread-group preemption granularity by disabling
> +	 * Mid-Thread Preemption on newly created LRCs.
> +	 *
> +	 * Xe2+ RCS/CCS only, controlled via debugfs.
> +	 */
> +	bool disable_mtp;
> +
>  	/** @usm: unified shared memory state */
>  	struct {
>  		/**
> diff --git a/drivers/gpu/drm/xe/xe_lrc.c b/drivers/gpu/drm/xe/xe_lrc.c
> index 3e7c995085d0..893456dad6a5 100644
> --- a/drivers/gpu/drm/xe/xe_lrc.c
> +++ b/drivers/gpu/drm/xe/xe_lrc.c
> @@ -1262,6 +1262,29 @@ static ssize_t setup_invalidate_auxccs_wa(struct xe_lrc *lrc,
>  	return emit(gt, batch) - batch;
>  }
>  
> +static ssize_t setup_disable_mtp_wa(struct xe_lrc *lrc,
> +				    struct xe_hw_engine *hwe,
> +				    u32 *batch, size_t max_len)
> +{
> +	struct xe_gt *gt = lrc->gt;
> +	struct xe_device *xe = gt_to_xe(gt);
> +	u32 *cmd = batch;
> +
> +	if (!gt->disable_mtp || GRAPHICS_VER(xe) < 20 ||
> +	    (hwe->class != XE_ENGINE_CLASS_RENDER &&
> +	     hwe->class != XE_ENGINE_CLASS_COMPUTE))
> +		return 0;
> +
> +	if (xe_gt_WARN_ON(gt, max_len < 3))
> +		return -ENOSPC;
> +
> +	*cmd++ = MI_LOAD_REGISTER_IMM | MI_LRI_LRM_CS_MMIO | MI_LRI_NUM_REGS(1);
> +	*cmd++ = CS_CHICKEN1(0).addr;
> +	*cmd++ = REG_MASKED_FIELD(PREEMPT_GPGPU_LEVEL_MASK, PREEMPT_GPGPU_THREAD_GROUP_LEVEL);

Is this enough?  Reading the Bspec page for CS_CHICKEN1, it appears that
the settings for PREEMPT_GPGPU_LEVEL_MASK depend on the value of
FF_SLICE_CS_CHICKEN1's field "Per Context Preemption Granularity
Control".

> +
> +	return cmd - batch;
> +}
> +
>  struct bo_setup {
>  	ssize_t (*setup)(struct xe_lrc *lrc, struct xe_hw_engine *hwe,
>  			 u32 *batch, size_t max_size);
> @@ -1342,6 +1365,7 @@ int xe_lrc_setup_wa_bb_with_scratch(struct xe_lrc *lrc, struct xe_hw_engine *hwe
>  		{ .setup = setup_timestamp_wa },
>  		{ .setup = setup_invalidate_state_cache_wa },
>  		{ .setup = setup_utilization_wa },
> +		{ .setup = setup_disable_mtp_wa },

This is not really a workaround in the official sense of the term. I
would drop the "_wa" from the function name.

--
Gustavo Sousa

>  		{ .setup = setup_configfs_post_ctx_restore_bb },
>  	};
>  	struct bo_setup_state state = {
> -- 
> 2.43.0