Re: [PATCH v8] drm/xe: Add debugfs knob to control GPGPU preemption granularity

Varun Gupta <[email protected]> writes:

> Introduce a per-GT debugfs knob, 'gpgpu_preemption_level', to allow
> overriding the GPGPU preemption level on a per-context basis for newly
> created LRCs.
> Add an RTP rule to enable per-context control via FF_SLICE_CS_CHICKEN1,
> allowing the preemption level to be programmed directly into the LRC
> image at CTX_CS_CHICKEN1 during context initialization.
>
> v8:
>   - Encapsulate LRC preemption programming into
>     xe_lrc_set_gpgpu_preemption_level() (Gustavo)
>   - Add xe_gt_WARN and early return for unexpected preemption level values
>     to prevent invalid CS_CHICKEN1 programming. (Gustavo)
>   - Change -EPERM to -EINVAL for MTP fused off check (Gustavo)
>   - Combine FF_SLICE_CS_CHICKEN1 RTP rule using OR macro (Gustavo)
>
> v7:
>   - Restrict XEHP_FUSE4 read to Xe2+ primary GTs to prevent invalid
>     Media GT accesses and SR-IOV warnings. (Sashiko)
>
> v6:
>   - Add missing xe_gt_printk.h include to fix compilation failure.
>
> v5:
>   - Use correct offset for CTX_CS_CHICKEN1. (Matt)
>   - Restrict to the RCS engine. (Matt)
>   - Drop LRC layout table modifications; dummy layouts do not need late
>     context registers. (Matt)
>   - Stash WMTP fuse state at boot to remove pm/forcewake from debugfs.
>     (Matt)
>   - Rename debugfs knob to 'gpgpu_preemption_level'. (Matt)
>   - Use simple_write_to_buffer() and remove unnecessary READ_ONCE/
>     WRITE_ONCE macros. (Matt)
>   - Do not restrict debugfs visibility for SR-IOV VFs. (Matt)
>
> v4
>   - Fix incorrect NOP padding in the RCS context layout. (Sashiko)
>
> v3:
>   - Wrapped XEHP_FUSE4 forcewake read with xe_pm_runtime_get/put to
>     prevent PCIe aborts/timeouts when the GPU is in D3hot/D3cold. (sashiko)
>   - Fixed NOP macro truncation by splitting padding offsets larger than
>     0x7f into multiple NOPs, ensuring correct context layout. (sashiko)
>   - Converted CTX_CS_CHICKEN1 initialization to a read-modify-write
>     sequence to avoid overwriting golden context mask bits. (sashiko)
>   - Removed the FF_SLICE_CS_CHICKEN1 workaround for CCS engines, as
>     compute engines lack this 3D fixed-function register, which was
>     causing GuC "illegal register" panics on initialization.
>
> v2:
>   - Dropped the WA BB/MI_LRI path; per Bspec, CS_CHICKEN1 is context
>     save/restore state at DW 0x00E2, so we map CTX_CS_CHICKEN1 and program
>     it directly in LRC init via xe_lrc_write_ctx_reg(). (Matt)
>   - Split Xe2 CCS context offsets into a dedicated xe2_ccs_offsets array
>     to map CS_CHICKEN1 without polluting other XCS engines.
>   - Converted the debugfs interface from a binary boolean to a
>     multi-option string knob ("default", "mid-thread", "thread-group",
>     "command"). (Gustavo)
>   - Restricted file creation to the Physical Function
>     (!IS_SRIOV_VF). (Gustavo)
>   - Added kernel tainting (TAINT_USER) when deviating from
>     defaults. (Gustavo)
>   - Added power-safe hardware fuse check (FUSE4 register 0x9114[20],
>     CFEG_WMTP_DISABLE) before allowing MTP selection. (Matt)
>
> Signed-off-by: Varun Gupta <[email protected]>
> ---
>  drivers/gpu/drm/xe/regs/xe_lrc_layout.h |  2 +
>  drivers/gpu/drm/xe/xe_gt.c              |  6 +++
>  drivers/gpu/drm/xe/xe_gt_debugfs.c      | 66 +++++++++++++++++++++++++
>  drivers/gpu/drm/xe/xe_gt_types.h        | 34 +++++++++++++
>  drivers/gpu/drm/xe/xe_lrc.c             | 33 +++++++++++++
>  drivers/gpu/drm/xe/xe_wa.c              |  4 +-
>  6 files changed, 144 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/xe/regs/xe_lrc_layout.h b/drivers/gpu/drm/xe/regs/xe_lrc_layout.h
> index 4ab86fc369fd..6f7abc0181b5 100644
> --- a/drivers/gpu/drm/xe/regs/xe_lrc_layout.h
> +++ b/drivers/gpu/drm/xe/regs/xe_lrc_layout.h
> @@ -37,6 +37,8 @@
>  #define CTX_QUEUE_TIMESTAMP		(0xd0 + 1)
>  #define CTX_QUEUE_TIMESTAMP_UDW		(0xd2 + 1)
>  
> +#define CTX_CS_CHICKEN1			(0x12e + 1)
> +
>  #define INDIRECT_CTX_RING_HEAD		(0x02 + 1)
>  #define INDIRECT_CTX_RING_TAIL		(0x04 + 1)
>  #define INDIRECT_CTX_RING_START		(0x06 + 1)
> diff --git a/drivers/gpu/drm/xe/xe_gt.c b/drivers/gpu/drm/xe/xe_gt.c
> index 783eb6d631b5..64532bf1f801 100644
> --- a/drivers/gpu/drm/xe/xe_gt.c
> +++ b/drivers/gpu/drm/xe/xe_gt.c
> @@ -608,6 +608,12 @@ static int gt_init_with_gt_forcewake(struct xe_gt *gt)
>  	 */
>  	gt->info.gmdid = xe_mmio_read32(&gt->mmio, GMD_ID);
>  
> +	if (GRAPHICS_VER(gt_to_xe(gt)) >= 20 && xe_gt_is_main_type(gt))
> +		gt->info.has_wmtp_disabled = !!(xe_mmio_read32(&gt->mmio, XEHP_FUSE4) &
> +			CFEG_WMTP_DISABLE);
> +	else
> +		gt->info.has_wmtp_disabled = 0;
> +
>  	/*
>  	 * Wa_14026539277 can't be implemented as a regular GT workaround (i.e.
>  	 * as an entry in gt_was[]) for two reasons: it is actually a device
> diff --git a/drivers/gpu/drm/xe/xe_gt_debugfs.c b/drivers/gpu/drm/xe/xe_gt_debugfs.c
> index c38bcacb27e4..6211c0b6d00b 100644
> --- a/drivers/gpu/drm/xe/xe_gt_debugfs.c
> +++ b/drivers/gpu/drm/xe/xe_gt_debugfs.c
> @@ -6,6 +6,8 @@
>  #include "xe_gt_debugfs.h"
>  
>  #include <linux/debugfs.h>
> +#include <linux/panic.h>
> +#include <linux/string.h>
>  
>  #include <drm/drm_debugfs.h>
>  #include <drm/drm_managed.h>
> @@ -15,6 +17,7 @@
>  #include "xe_gt.h"
>  #include "xe_gt_mcr.h"
>  #include "xe_gt_idle.h"
> +#include "xe_gt_printk.h"
>  #include "xe_gt_sriov_pf_debugfs.h"
>  #include "xe_gt_sriov_vf_debugfs.h"
>  #include "xe_gt_stats.h"
> @@ -336,6 +339,65 @@ static int force_reset_sync_show(struct seq_file *s, void *unused)
>  }
>  DEFINE_SHOW_STORE_ATTRIBUTE(force_reset_sync);
>  
> +static const char * const gpgpu_preemption_level_names[] = {
> +	[XE_GPGPU_PREEMPT_DEFAULT]      = "default",
> +	[XE_GPGPU_PREEMPT_MID_THREAD]   = "mid-thread",
> +	[XE_GPGPU_PREEMPT_THREAD_GROUP] = "thread-group",
> +	[XE_GPGPU_PREEMPT_COMMAND]      = "command",
> +};
> +
> +static int gpgpu_preemption_level_show(struct seq_file *m, void *unused)
> +{
> +	struct xe_gt *gt = m->private;
> +
> +	seq_printf(m, "%s\n", gpgpu_preemption_level_names[gt->gpgpu_preemption_level]);
> +
> +	return 0;
> +}
> +
> +static ssize_t gpgpu_preemption_level_write(struct file *file,
> +					    const char __user *ubuf,
> +					    size_t len, loff_t *offp)
> +{
> +	struct seq_file *m = file->private_data;
> +	struct xe_gt *gt = m->private;
> +	enum xe_gpgpu_preempt_level new_level;
> +	char buf[16];
> +	ssize_t copied;
> +	int idx;
> +
> +	if (*offp)
> +		return -EINVAL;
> +
> +	copied = simple_write_to_buffer(buf, sizeof(buf) - 1, offp, ubuf, len);
> +	if (copied < 0)
> +		return copied;
> +
> +	buf[copied] = '\0';
> +	idx = sysfs_match_string(gpgpu_preemption_level_names, strim(buf));
> +	if (idx < 0)
> +		return idx;
> +
> +	new_level = (enum xe_gpgpu_preempt_level)idx;
> +
> +	if (new_level == XE_GPGPU_PREEMPT_MID_THREAD && gt->info.has_wmtp_disabled) {
> +		xe_gt_warn(gt, "MTP fused off in hardware, cannot select mid-thread\n");
> +		return -EINVAL;
> +	}
> +
> +	if (new_level != XE_GPGPU_PREEMPT_DEFAULT) {
> +		add_taint(TAINT_USER, LOCKDEP_STILL_OK);
> +		xe_gt_notice(gt,
> +			     "GPGPU preemption overridden to '%s' (applies to new LRCs only)\n",
> +			     gpgpu_preemption_level_names[new_level]);
> +	}
> +
> +	gt->gpgpu_preemption_level = new_level;
> +
> +	return copied;
> +}
> +DEFINE_SHOW_STORE_ATTRIBUTE(gpgpu_preemption_level);
> +
>  void xe_gt_debugfs_register(struct xe_gt *gt)
>  {
>  	struct xe_device *xe = gt_to_xe(gt);
> @@ -369,6 +431,10 @@ void xe_gt_debugfs_register(struct xe_gt *gt)
>  	debugfs_create_file("force_reset", 0600, root, gt, &force_reset_fops);
>  	debugfs_create_file("force_reset_sync", 0600, root, gt, &force_reset_sync_fops);
>  
> +	if (GRAPHICS_VER(xe) >= 20 && (gt->info.engine_mask & XE_HW_ENGINE_RCS_MASK))
> +		debugfs_create_file("gpgpu_preemption_level", 0600, root,
> +				    gt, &gpgpu_preemption_level_fops);
> +
>  	drm_debugfs_create_files(vf_safe_debugfs_list,
>  				 ARRAY_SIZE(vf_safe_debugfs_list),
>  				 root, minor);
> diff --git a/drivers/gpu/drm/xe/xe_gt_types.h b/drivers/gpu/drm/xe/xe_gt_types.h
> index e5588c88800a..97bd943309b3 100644
> --- a/drivers/gpu/drm/xe/xe_gt_types.h
> +++ b/drivers/gpu/drm/xe/xe_gt_types.h
> @@ -35,6 +35,33 @@ enum xe_gt_eu_type {
>  	XE_GT_EU_TYPE_SIMD16,
>  };
>  
> +/**
> + * enum xe_gpgpu_preempt_level - Per-context GPGPU preemption override mode
> + *
> + * Selects the preemption granularity programmed into CS_CHICKEN1[2:1] for
> + * newly created Xe2+ RCS LRCs.
> + *
> + * The per-context override is effective only when
> + * FF_SLICE_CS_CHICKEN1[FFSC_PERCTX_PREEMPT_CTRL] is enabled via RTP.
> + *
> + * This setting is GT-scoped and affects only LRCs created after the value is
> + * changed; existing contexts keep their previously programmed value.

Nitpick: I think this paragraph doesn't really apply to the description
of the type.  It would probably better if it was moved to the kerneldoc
for the gpgpu_preemption_level member (where the "GT-scoped" part would
be already implicit).

> + *
> + * @XE_GPGPU_PREEMPT_DEFAULT: Keep platform default preemption granularity.
> + * @XE_GPGPU_PREEMPT_MID_THREAD: Force mid-thread preemption level.
> + * @XE_GPGPU_PREEMPT_THREAD_GROUP: Force thread-group preemption level.
> + * @XE_GPGPU_PREEMPT_COMMAND: Force command-level preemption.
> + *
> + * Zero-initialized via kzalloc, so XE_GPGPU_PREEMPT_DEFAULT is the safe
> + * default (no override from platform policy).

Nitpick: Similarly, this "zero-initialized via kzalloc" is more a
description of the instances of xe_gt than to the enum type itself.  I
think we could just drop this paragraph.

> + */
> +enum xe_gpgpu_preempt_level {
> +	XE_GPGPU_PREEMPT_DEFAULT = 0,
> +	XE_GPGPU_PREEMPT_MID_THREAD,
> +	XE_GPGPU_PREEMPT_THREAD_GROUP,
> +	XE_GPGPU_PREEMPT_COMMAND,
> +};
> +
>  #define XE_MAX_DSS_FUSE_REGS		4
>  #define XE_MAX_DSS_FUSE_BITS		(32 * XE_MAX_DSS_FUSE_REGS)
>  #define XE_MAX_EU_FUSE_REGS		1
> @@ -151,6 +178,8 @@ struct xe_gt {
>  		 * feature.
>  		 */
>  		u8 has_xe2_blt_instructions:1;
> +		/** @info.has_wmtp_disabled: hardware fuse indicates WMTP is disabled */
> +		u8 has_wmtp_disabled:1;

I guess now we could have a follow-up patch to make
xe_rtp_cfeg_wmtp_disabled() use this field.

With the nitpicks from above,

Reviewed-by: Gustavo Sousa <[email protected]>

That said, I think we should also wait for Matt Roper's feedback in case
I missed some important detail.

--
Gustavo Sousa

>  		/**
>  		 * @info.num_geometry_xecore_fuse_regs: Number of 32b-bit fuse
>  		 * registers the geometry XeCore mask spans.
> @@ -219,6 +248,11 @@ struct xe_gt {
>  	 */
>  	u32 ccs_mode;
>  
> +	/**
> +	 * @gpgpu_preemption_level: per-GT GPGPU preemption granularity override.
> +	 */
> +	enum xe_gpgpu_preempt_level gpgpu_preemption_level;
> +
>  	/** @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..6e22e964e708 100644
> --- a/drivers/gpu/drm/xe/xe_lrc.c
> +++ b/drivers/gpu/drm/xe/xe_lrc.c
> @@ -1486,6 +1486,36 @@ void xe_lrc_set_multi_queue_priority(struct xe_lrc *lrc, enum xe_multi_queue_pri
>  	lrc->desc |= FIELD_PREP(LRC_PRIORITY, xe_multi_queue_prio_to_lrc(lrc, priority));
>  }
>  
> +static void xe_lrc_set_gpgpu_preemption_level(struct xe_lrc *lrc, struct xe_gt *gt)
> +{
> +	enum xe_gpgpu_preempt_level level = gt->gpgpu_preemption_level;
> +	u32 level_bits;
> +	u32 val;
> +
> +	if (level == XE_GPGPU_PREEMPT_DEFAULT)
> +		return;
> +
> +	switch (level) {
> +	case XE_GPGPU_PREEMPT_MID_THREAD:
> +		level_bits = PREEMPT_GPGPU_MID_THREAD_LEVEL;
> +		break;
> +	case XE_GPGPU_PREEMPT_THREAD_GROUP:
> +		level_bits = PREEMPT_GPGPU_THREAD_GROUP_LEVEL;
> +		break;
> +	case XE_GPGPU_PREEMPT_COMMAND:
> +		level_bits = PREEMPT_GPGPU_COMMAND_LEVEL;
> +		break;
> +	default:
> +		xe_gt_WARN(gt, true, "Invalid GPGPU preemption level: %d\n", level);
> +		return;
> +	}
> +
> +	val = xe_lrc_read_ctx_reg(lrc, CTX_CS_CHICKEN1);
> +	val &= ~PREEMPT_GPGPU_LEVEL_MASK;
> +	val |= REG_MASKED_FIELD(PREEMPT_GPGPU_LEVEL_MASK, level_bits);
> +	xe_lrc_write_ctx_reg(lrc, CTX_CS_CHICKEN1, val);
> +}
> +
>  static int xe_lrc_ctx_init(struct xe_lrc *lrc, struct xe_hw_engine *hwe, struct xe_vm *vm,
>  			   void *replay_state, u16 msix_vec, u32 init_flags)
>  {
> @@ -1589,6 +1619,9 @@ static int xe_lrc_ctx_init(struct xe_lrc *lrc, struct xe_hw_engine *hwe, struct
>  	if (xe->info.has_asid && vm)
>  		xe_lrc_write_ctx_reg(lrc, CTX_ASID, vm->usm.asid);
>  
> +	if (GRAPHICS_VER(xe) >= 20 && hwe->class == XE_ENGINE_CLASS_RENDER)
> +		xe_lrc_set_gpgpu_preemption_level(lrc, gt);
> +
>  	lrc->desc = LRC_VALID;
>  	lrc->desc |= FIELD_PREP(LRC_ADDRESSING_MODE, LRC_LEGACY_64B_CONTEXT);
>  	/* TODO: Priority */
> diff --git a/drivers/gpu/drm/xe/xe_wa.c b/drivers/gpu/drm/xe/xe_wa.c
> index 139434946f8f..c94fe7753f9a 100644
> --- a/drivers/gpu/drm/xe/xe_wa.c
> +++ b/drivers/gpu/drm/xe/xe_wa.c
> @@ -343,7 +343,9 @@ static const struct xe_rtp_table_sr engine_was = XE_RTP_TABLE_SR(
>  	  XE_RTP_ACTIONS(SET(SAMPLER_MODE, ENABLE_SMALLPL))
>  	},
>  	{ XE_RTP_NAME("FtrPerCtxtPreemptionGranularityControl"),
> -	  XE_RTP_RULES(GRAPHICS_VERSION_RANGE(1200, 1250), ENGINE_CLASS(RENDER)),
> +	  XE_RTP_RULES(GRAPHICS_VERSION_RANGE(1200, 1250), ENGINE_CLASS(RENDER), OR,
> +		       GRAPHICS_VERSION_RANGE(2000, XE_RTP_END_VERSION_UNDEFINED),
> +		       ENGINE_CLASS(RENDER)),
>  	  XE_RTP_ACTIONS(SET(FF_SLICE_CS_CHICKEN1(RENDER_RING_BASE),
>  			     FFSC_PERCTX_PREEMPT_CTRL))
>  	},
> -- 
> 2.43.0