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

[John Harrison <john.c.harrison@intel.com>]

On 3/25/2024 08:32, 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@intel.com>
>> Signed-off-by: Badal Nilawar <badal.nilawar@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 ?
By reading the spec.

> as this is the ABI header, IMO we should define that here along the KEY
I strongly disagree with copying the entire GuC API spec into the source 
code just for the sake of copying the spec. The implementation of this 
w/a (and a whole bunch more that will follow) has no use for extra 
defines about sizes or fields. And creating defines that will never be 
used is a pointless exercise.

>
>> +};
>> +
>>   #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);
>
>> +
>> +	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
>
>> +	} 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
It looks the same to me. It has simply optimised the the offset/remain 
calculation to be done inside the helper rather than outside it to avoid 
replication when all the other 'simple' w/a's are added. The actual 
behaviour is identical.

John.

>
>>   
>>   	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)