Re: [PATCH v2 3/3] drm/xe: Add up-to-date implementation for Wa_14026539277

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

Matthew Auld <matthew.auld@intel.com> writes:

> On 24/07/2026 22:44, Gustavo Sousa wrote:
>> Wa_14026539277 is a temporary workaround that targets only A*
>> steppings of graphics version 35.10 and requires that we avoid using
>> 2-way coherency for device-cacheable memory accesses.  For
>> driver-internal usage, we convert those configurations to be
>> device-uncached and 1-way-coherent; on the UAPI side, we reject
>> VM_BIND/MADVISE calls that are 2-way-coherent and leave it up to
>> userspace to select the alternative option that is most appropriate to
>> their usage.
>> 
>> v2:
>>    - Add missing bits to convert the PAT value to device-uncacheable in
>>      wa_14026539277_fixup_pat_value(). (Sashiko)
>>    - Also validate the PAT index in the DRM_XE_VM_MADVISE
>>      ioctl. (Sashiko)
>>    - Match against the graphics IP instead of the platform.
>> 
>> Signed-off-by: Gustavo Sousa <gustavo.sousa@intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_pat.h        | 10 +++++++
>>   drivers/gpu/drm/xe/xe_device.c     | 16 ++++++----
>>   drivers/gpu/drm/xe/xe_pat.c        | 61 ++++++++++++++++++++++++++++++++++----
>>   drivers/gpu/drm/xe/xe_vm.c         |  5 ++++
>>   drivers/gpu/drm/xe/xe_vm_madvise.c |  3 ++
>>   drivers/gpu/drm/xe/xe_wa_oob.rules |  1 +
>>   6 files changed, 84 insertions(+), 12 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/xe/xe_pat.h b/drivers/gpu/drm/xe/xe_pat.h
>> index 7060f66e1d63..10374022f1d2 100644
>> --- a/drivers/gpu/drm/xe/xe_pat.h
>> +++ b/drivers/gpu/drm/xe/xe_pat.h
>> @@ -82,6 +82,16 @@ bool xe_pat_index_get_comp_en(struct xe_device *xe, u16 pat_index);
>>    */
>>   u16 xe_pat_index_get_l3_policy(struct xe_device *xe, u16 pat_index);
>>   
>> +/**
>> + * xe_pat_wa_14026539277_reserved - Is this PAT index reserved from
>> + * use due to Wa_14026539277?
>> + * @xe: xe device
>> + * @pat_index: The pat_index to query
>> + *
>> + * Return: a boolean indicating whether the PAT index is reserved or not.
>> + */
>> +bool xe_pat_wa_14026539277_reserved(struct xe_device *xe, u16 pat_index);
>> +
>>   #define xe_cache_pat_idx(xe, cache_mode) ({					\
>>   	const struct xe_device *__xedev = (xe);					\
>>   	enum xe_cache_level __mode = (cache_mode);				\
>> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
>> index 5189419a0593..c4b8023cb8a2 100644
>> --- a/drivers/gpu/drm/xe/xe_device.c
>> +++ b/drivers/gpu/drm/xe/xe_device.c
>> @@ -836,14 +836,18 @@ static void override_has_cached_pt(struct xe_device *xe)
>>   	struct xe_gt *gt;
>>   	u8 id;
>>   
>> -	/*
>> -	 * Wa_16029380221: The affected GT will always use non-coherent
>> -	 * access to page tables, so we must do uncached writes from the
>> -	 * CPU.
>> -	 */
>> -	for_each_gt(gt, xe, id)
>> +	for_each_gt(gt, xe, id) {
>> +		/*
>> +		 * Wa_16029380221: The affected GT will always use
>> +		 * non-coherent access to page tables, so we must do
>> +		 * uncached writes from the CPU.
>> +		 */
>>   		if (XE_GT_WA(gt, 16029380221))
>>   			xe->info.has_cached_pt = false;
>> +
>> +		if (XE_GT_WA(gt, 14026539277))
>> +			xe->info.has_cached_pt = false;
>> +	}
>>   }
>>   
>>   static int probe_has_flat_ccs(struct xe_device *xe)
>> diff --git a/drivers/gpu/drm/xe/xe_pat.c b/drivers/gpu/drm/xe/xe_pat.c
>> index a5fe1beec652..a9cde12eba70 100644
>> --- a/drivers/gpu/drm/xe/xe_pat.c
>> +++ b/drivers/gpu/drm/xe/xe_pat.c
>> @@ -313,6 +313,37 @@ u16 xe_pat_index_get_l3_policy(struct xe_device *xe, u16 pat_index)
>>   	return REG_FIELD_GET(XE2_L3_POLICY, xe->pat.table[pat_index].value);
>>   }
>>   
>> +bool xe_pat_wa_14026539277_reserved(struct xe_device *xe, u16 pat_index)
>> +{
>> +	struct xe_gt *gt;
>> +	u8 id;
>> +	bool has_wa = false;
>> +
>> +	for_each_gt(gt, xe, id) {
>> +		if (XE_GT_WA(gt, 14026539277)) {
>> +			has_wa = true;
>> +			break;
>> +		}
>> +	}
>> +
>> +	return has_wa && xe_pat_index_get_l3_policy(xe, pat_index) != XE_L3_POLICY_UC &&
>
> Just to make sure I understand. Why do we bother to check the l3 policy 
> here? Do we not just want to ban all 2WAY from uapi pov? The l3/l4 
> policy can often be promoted via the per MOCS setting, so even if UC 
> here it can be made WB via MOCS. Is this OK?

That's a good point.

The workaround instructions were to avoid using GT-cacheable
2-way-coherent configurations, that's why I was allowing 2-way coherency
for XE_L3_POLICY_UC, but I did not think of the scenario of the cache
policy being overridden by MOCS.  Thanks for pointing it out!

Yeah, it seems we should use a bigger hammer and disallow any 2-way
coherency then.  I'll change this on the next version.

--
Gustavo Sousa

>
>> +	       xe_pat_index_get_coh_mode(xe, pat_index) == XE_COH_2WAY;
>> +}
>> +
>> +static u32 wa_14026539277_fixup_pat_value(struct xe_gt *gt, u32 value)
>> +{
>> +	if (XE_GT_WA(gt, 14026539277)) {
>> +		if (REG_FIELD_GET(XE2_L3_POLICY, value) != XE_L3_POLICY_UC &&
>> +		    REG_FIELD_GET(XE2_COH_MODE, value) == XE_COH_2WAY) {
>> +			value &= ~(XE2_L3_POLICY | XE2_COH_MODE);
>> +			value |= REG_FIELD_PREP(XE2_L3_POLICY, XE_L3_POLICY_UC) |
>> +				 REG_FIELD_PREP(XE2_COH_MODE, XE_COH_1WAY);
>> +		}
>> +	}
>> +
>> +	return value;
>> +}
>> +
>>   static const struct xe_pat_table_entry *gt_pta_entry(struct xe_gt *gt)
>>   {
>>   	struct xe_device *xe = gt_to_xe(gt);
>> @@ -373,18 +404,22 @@ static void program_pat_mcr(struct xe_gt *gt, const struct xe_pat_table_entry ta
>>   
>>   	for (int i = 0; i < n_entries; i++) {
>>   		struct xe_reg_mcr reg_mcr = XE_REG_MCR(_PAT_INDEX(i));
>> +		u32 pat = wa_14026539277_fixup_pat_value(gt, table[i].value);
>>   
>> -		xe_gt_mcr_multicast_write(gt, reg_mcr, table[i].value);
>> +		xe_gt_mcr_multicast_write(gt, reg_mcr, pat);
>>   	}
>>   
>>   	if (xe->pat.pat_ats)
>> -		xe_gt_mcr_multicast_write(gt, XE_REG_MCR(_PAT_ATS), xe->pat.pat_ats->value);
>> +		xe_gt_mcr_multicast_write(gt, XE_REG_MCR(_PAT_ATS),
>> +					  wa_14026539277_fixup_pat_value(gt, xe->pat.pat_ats->value));
>>   
>>   	if (pta_entry)
>> -		xe_gt_mcr_multicast_write(gt, XE_REG_MCR(_PAT_PTA), pta_entry->value);
>> +		xe_gt_mcr_multicast_write(gt, XE_REG_MCR(_PAT_PTA),
>> +					  wa_14026539277_fixup_pat_value(gt, pta_entry->value));
>>   
>>   	if (tr_pta_entry)
>> -		xe_gt_mcr_multicast_write(gt, XE_REG_MCR(_PAT_TR_PTA), tr_pta_entry->value);
>> +		xe_gt_mcr_multicast_write(gt, XE_REG_MCR(_PAT_TR_PTA),
>> +					  wa_14026539277_fixup_pat_value(gt, tr_pta_entry->value));
>>   }
>>   
>>   static int xelp_dump(struct xe_gt *gt, struct drm_printer *p)
>> @@ -534,13 +569,16 @@ static int xe2_dump(struct xe_gt *gt, struct drm_printer *p)
>>   	drm_printf(p, "PAT table: (* = reserved entry)\n");
>>   
>>   	for (i = 0; i < xe->pat.n_entries; i++) {
>> +		bool rsvd = !xe->pat.table[i].valid ||
>> +			    xe_pat_wa_14026539277_reserved(xe, i);
>> +
>>   		if (xe_gt_is_media_type(gt))
>>   			pat = xe_mmio_read32(&gt->mmio, XE_REG(_PAT_INDEX(i)));
>>   		else
>>   			pat = xe_gt_mcr_unicast_read_any(gt, XE_REG_MCR(_PAT_INDEX(i)));
>>   
>>   		xe_pat_index_label(label, sizeof(label), i);
>> -		xe->pat.ops->entry_dump(p, label, pat, !xe->pat.table[i].valid);
>> +		xe->pat.ops->entry_dump(p, label, pat, rsvd);
>>   	}
>>   
>>   	/*
>> @@ -747,9 +785,14 @@ int xe_pat_dump_sw_config(struct xe_gt *gt, struct drm_printer *p)
>>   	for (u32 i = 0; i < xe->pat.n_entries; i++) {
>>   		u32 pat = xe->pat.table[i].value;
>>   
>> +		pat = wa_14026539277_fixup_pat_value(gt, pat);
>> +
>>   		if (GRAPHICS_VER(xe) >= 20) {
>> +			bool rsvd = !xe->pat.table[i].valid ||
>> +				    xe_pat_wa_14026539277_reserved(xe, i);
>> +
>>   			xe_pat_index_label(label, sizeof(label), i);
>> -			xe->pat.ops->entry_dump(p, label, pat, !xe->pat.table[i].valid);
>> +			xe->pat.ops->entry_dump(p, label, pat, rsvd);
>>   		} else if (xe->info.platform == XE_METEORLAKE) {
>>   			xelpg_pat_entry_dump(p, i, pat);
>>   		} else if (xe->info.platform == XE_PVC) {
>> @@ -764,6 +807,8 @@ int xe_pat_dump_sw_config(struct xe_gt *gt, struct drm_printer *p)
>>   	if (pta_entry) {
>>   		u32 pat = pta_entry->value;
>>   
>> +		pat = wa_14026539277_fixup_pat_value(gt, pat);
>> +
>>   		drm_printf(p, "Page Table Access:\n");
>>   		xe->pat.ops->entry_dump(p, "PTA_MODE", pat, false);
>>   	}
>> @@ -771,6 +816,8 @@ int xe_pat_dump_sw_config(struct xe_gt *gt, struct drm_printer *p)
>>   	if (tr_pta_entry) {
>>   		u32 pat = tr_pta_entry->value;
>>   
>> +		pat = wa_14026539277_fixup_pat_value(gt, pat);
>> +
>>   		drm_printf(p, "TRTT Page Table Access:\n");
>>   		xe->pat.ops->entry_dump(p, "TR_PTA_MODE", pat, false);
>>   	}
>> @@ -778,6 +825,8 @@ int xe_pat_dump_sw_config(struct xe_gt *gt, struct drm_printer *p)
>>   	if (xe->pat.pat_ats) {
>>   		u32 pat = xe->pat.pat_ats->value;
>>   
>> +		pat = wa_14026539277_fixup_pat_value(gt, pat);
>> +
>>   		drm_printf(p, "PCIe ATS/PASID:\n");
>>   		xe->pat.ops->entry_dump(p, "PAT_ATS ", pat, false);
>>   	}
>> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
>> index 9e0176861cb6..64c03c669630 100644
>> --- a/drivers/gpu/drm/xe/xe_vm.c
>> +++ b/drivers/gpu/drm/xe/xe_vm.c
>> @@ -3697,6 +3697,11 @@ static int vm_bind_ioctl_check_args(struct xe_device *xe, struct xe_vm *vm,
>>   			goto free_bind_ops;
>>   		}
>>   
>> +		if (XE_IOCTL_DBG(xe, xe_pat_wa_14026539277_reserved(xe, pat_index))) {
>> +			err = -EINVAL;
>> +			goto free_bind_ops;
>> +		}
>> +
>>   		if (XE_IOCTL_DBG(xe, op > DRM_XE_VM_BIND_OP_PREFETCH) ||
>>   		    XE_IOCTL_DBG(xe, flags & ~SUPPORTED_FLAGS) ||
>>   		    XE_IOCTL_DBG(xe, obj && (is_null || is_cpu_addr_mirror)) ||
>> diff --git a/drivers/gpu/drm/xe/xe_vm_madvise.c b/drivers/gpu/drm/xe/xe_vm_madvise.c
>> index 0474768a38aa..265273294756 100644
>> --- a/drivers/gpu/drm/xe/xe_vm_madvise.c
>> +++ b/drivers/gpu/drm/xe/xe_vm_madvise.c
>> @@ -500,6 +500,9 @@ static bool check_pat_args_are_sane(struct xe_device *xe,
>>   	u16 coh_mode = xe_pat_index_get_coh_mode(xe, pat_index);
>>   	int i;
>>   
>> +	if (XE_IOCTL_DBG(xe, xe_pat_wa_14026539277_reserved(xe, pat_index)))
>> +		return false;
>> +
>>   	/*
>>   	 * Using coh_none with CPU cached buffers is not allowed on iGPU.
>>   	 * On iGPU the GPU shares the LLC with the CPU, so with coh_none
>> diff --git a/drivers/gpu/drm/xe/xe_wa_oob.rules b/drivers/gpu/drm/xe/xe_wa_oob.rules
>> index f02ac9bf7424..d53e7f75814b 100644
>> --- a/drivers/gpu/drm/xe/xe_wa_oob.rules
>> +++ b/drivers/gpu/drm/xe/xe_wa_oob.rules
>> @@ -71,4 +71,5 @@
>>   		GRAPHICS_VERSION(3511)
>>   16029897822	MEDIA_VERSION(3500)
>>   		GRAPHICS_VERSION(3510)
>> +14026539277	GRAPHICS_VERSION(3510), GRAPHICS_STEP(A0, B0)
>>   14027054324	GRAPHICS_VERSION(3511)
>>