[PATCH v3 2/2] drm/xe/lnl: Enable GuC Wa_14019882105

[Nilawar, Badal]

On 25-03-2024 21:02, Michal Wajdeczko wrote:
> 
> 
> On 25.03.2024 16:04, Badal Nilawar wrote:
>> Enable GuC Wa_14019882105 to block interrupts during C6 flow
>> when the memory path has been blocked
>>
>> v2: Make helper function generic and name it as
>>      guc_waklv_enable_simple (John Harrison)
>> v3: Make warning descriptive (John Harrison)
>>
>> Cc: John Harrison <john.harrison at intel.com>
>> Signed-off-by: Badal Nilawar <badal.nilawar at intel.com>
>> ---
>>   drivers/gpu/drm/xe/abi/guc_klvs_abi.h |  7 +++++
>>   drivers/gpu/drm/xe/xe_guc_ads.c       | 41 +++++++++++++++++++++------
>>   drivers/gpu/drm/xe/xe_wa_oob.rules    |  1 +
>>   3 files changed, 40 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/abi/guc_klvs_abi.h b/drivers/gpu/drm/xe/abi/guc_klvs_abi.h
>> index 0400bc0fccdc..5dd45e06f0b6 100644
>> --- a/drivers/gpu/drm/xe/abi/guc_klvs_abi.h
>> +++ b/drivers/gpu/drm/xe/abi/guc_klvs_abi.h
>> @@ -319,4 +319,11 @@ enum  {
>>   #define GUC_KLV_VF_CFG_BEGIN_CONTEXT_ID_KEY	0x8a0b
>>   #define GUC_KLV_VF_CFG_BEGIN_CONTEXT_ID_LEN	1u
>>   
>> +/*
>> + * Workaround keys:
>> + */
>> +enum xe_guc_klv_ids {
>> +	GUC_WORKAROUND_KLV_BLOCK_INTERRUPTS_WHEN_MGSR_BLOCKED				= 0x9002,
> 
> how should we know the LEN of the particular W/A KLV ?
> as this is the ABI header, IMO we should define that here along the KEY
W/A KLV are alternative to flags to enable workaround. So length for W/A 
KLV will always be 0. Should I still add length? May be I can define 
#define GUC_WORKAROUND_KLV_LENGTH 0 which will be common for all W/A KLVs.
> 
>> +};
>> +
>>   #endif
>> diff --git a/drivers/gpu/drm/xe/xe_guc_ads.c b/drivers/gpu/drm/xe/xe_guc_ads.c
>> index a98344a0ff4b..633e5fd9c738 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_ads.c
>> +++ b/drivers/gpu/drm/xe/xe_guc_ads.c
>> @@ -7,6 +7,8 @@
>>   
>>   #include <drm/drm_managed.h>
>>   
>> +#include <generated/xe_wa_oob.h>
>> +
>>   #include "regs/xe_engine_regs.h"
>>   #include "regs/xe_gt_regs.h"
>>   #include "regs/xe_guc_regs.h"
>> @@ -19,6 +21,7 @@
>>   #include "xe_map.h"
>>   #include "xe_mmio.h"
>>   #include "xe_platform_types.h"
>> +#include "xe_wa.h"
>>   
>>   /* Slack of a few additional entries per engine */
>>   #define ADS_REGSET_EXTRA_MAX	8
>> @@ -279,23 +282,43 @@ static size_t calculate_golden_lrc_size(struct xe_guc_ads *ads)
>>   	return total_size;
>>   }
>>   
>> +static void guc_waklv_enable_simple(struct xe_guc_ads *ads,
>> +				    enum xe_guc_klv_ids klv_id, u32 *offset, u32 *remain)
>> +{
>> +	u32 size;
>> +	u32 klv_entry[] = {
>> +		/* 16:16 key/length */
> 
> drop this comment, code is self explanatory
> 
>> +		FIELD_PREP(GUC_KLV_0_KEY, klv_id) |
>> +		FIELD_PREP(GUC_KLV_0_LEN, 0),
>> +		/* 0 dwords data */
>> +	};
> 
> you can define size here:
> 
> 		u32 size = sizeof(klv_entry);
Sure
> 
>> +
>> +	size = sizeof(klv_entry);
>> +
>> +	if (*remain < size) {
>> +		drm_warn(&ads_to_xe(ads)->drm,
>> +			 "w/a klv buffer too small to add klv id %d\n", klv_id);
> 
> this looks like our programming error so xe_gt_assert() should be
> sufficient as we don't expect this ever happen in production
> 
> but if you want to keep the WARN then use xe_gt_WARN() instead
Ok
> 
>> +	} else {
>> +		xe_map_memcpy_to(ads_to_xe(ads), ads_to_map(ads), *offset,
>> +				 klv_entry, size);
>> +		*offset += size;
>> +		*remain -= size;
>> +	}
>> +}
>> +
>>   static void guc_waklv_init(struct xe_guc_ads *ads)
>>   {
>> +	struct xe_gt *gt = ads_to_gt(ads);
>>   	u64 addr_ggtt;
>>   	u32 offset, remain, size;
>>   
>>   	offset = guc_ads_waklv_offset(ads);
>>   	remain = guc_ads_waklv_size(ads);
>>   
>> -	/*
>> -	 * Add workarounds here:
>> -	 *
>> -	 * if (want_wa_<name>) {
>> -	 *      size = guc_waklv_<name>(guc, offset, remain);
>> -	 *      offset += size;
>> -	 *      remain -= size;
>> -	 * }
>> -	 */
>> +	if (XE_WA(gt, 14019882105))
>> +		guc_waklv_enable_simple(ads,
>> +					GUC_WORKAROUND_KLV_BLOCK_INTERRUPTS_WHEN_MGSR_BLOCKED,
>> +					&offset, &remain);
> 
> hmm, it looks that your implementation here in patch 2/2 is different
> than suggested one in patch 1/2
I wrapped up adding W/A KLV entry inside the function. I will refine 
comment in patch 1/2 to align with patch 2/2.

Regards,
Badal
> 
>>   
>>   	size = guc_ads_waklv_size(ads) - remain;
>>   	if (!size)
>> diff --git a/drivers/gpu/drm/xe/xe_wa_oob.rules b/drivers/gpu/drm/xe/xe_wa_oob.rules
>> index 48cdba1cbf95..a8d15f004b6c 100644
>> --- a/drivers/gpu/drm/xe/xe_wa_oob.rules
>> +++ b/drivers/gpu/drm/xe/xe_wa_oob.rules
>> @@ -19,3 +19,4 @@
>>   		GRAPHICS_VERSION_RANGE(1270, 1274)
>>   		MEDIA_VERSION(1300)
>>   		PLATFORM(DG2)
>> +14019882105	GRAPHICS_VERSION(2004), GRAPHICS_STEP(A0, B0)