Re: [PATCH v2] drm/xe/xe_guc_ads: Consolidate guc_waklv_enable functions

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

On 7/25/2025 8:07 AM, Jonathan Cavitt wrote:
> Presently, multiple versions of the guc_waklv_enable_.* function exist,
> all with different numbers of dwords added to the klv_entry array.  This
> is not extensible, and more duplicates of the function will need to be
> created if it ever becomes necessary to support 3 or more dwords per wa
> in the future.
>
> Consolidate the disparate guc_waklv_enable functions into a single
> guc_waklv_enable function that can take an arbitrary number of dword
> values.
>
> v2: Update length value properly (Shuicheng)
>
> Suggested-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Signed-off-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
> Cc: Lucas De Marchi <lucas.demarch@intel.com>
> Cc: Shuicheng Lin <shuicheng.lin@intel.com>
> ---
>   drivers/gpu/drm/xe/xe_guc_ads.c | 144 ++++++++++++--------------------
>   1 file changed, 54 insertions(+), 90 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_guc_ads.c b/drivers/gpu/drm/xe/xe_guc_ads.c
> index 8ff8626227ae..4183a82fdb0c 100644
> --- a/drivers/gpu/drm/xe/xe_guc_ads.c
> +++ b/drivers/gpu/drm/xe/xe_guc_ads.c
> @@ -284,83 +284,41 @@ static size_t calculate_golden_lrc_size(struct xe_guc_ads *ads)
>   	return total_size;
>   }
>   
> -static void guc_waklv_enable_two_word(struct xe_guc_ads *ads,
> -				      enum xe_guc_klv_ids klv_id,
> -				      u32 value1,
> -				      u32 value2,
> -				      u32 *offset, u32 *remain)
> +static void guc_waklv_enable(struct xe_guc_ads *ads,
> +			     enum xe_guc_klv_ids klv_id,
> +			     u32 dwords[], u32 num_dwords,
'data' or 'values' would be a better name than 'dwords'.

> +			     u32 *offset, u32 *remain)
>   {
> -	u32 size;
> -	u32 klv_entry[] = {
> -			/* 16:16 key/length */
> -			FIELD_PREP(GUC_KLV_0_KEY, klv_id) |
> -			FIELD_PREP(GUC_KLV_0_LEN, 2),
> -			value1,
> -			value2,
> -			/* 2 dword data */
> -	};
> -
> -	size = sizeof(klv_entry);
> +	size_t size = sizeof(u32) * (1 + num_dwords);
> +	u32 *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);
The keys are all defined in hex and mostly as 16bit words, so would be 
more useful to print them as 0x%04X.

> -	} else {
> -		xe_map_memcpy_to(ads_to_xe(ads), ads_to_map(ads), *offset,
> -				 klv_entry, size);
> -		*offset += size;
> -		*remain -= size;
> +		return;
>   	}
> -}
> -
> -static void guc_waklv_enable_one_word(struct xe_guc_ads *ads,
> -				      enum xe_guc_klv_ids klv_id,
> -				      u32 value,
> -				      u32 *offset, u32 *remain)
> -{
> -	u32 size;
> -	u32 klv_entry[] = {
> -		/* 16:16 key/length */
> -		FIELD_PREP(GUC_KLV_0_KEY, klv_id) |
> -		FIELD_PREP(GUC_KLV_0_LEN, 1),
> -		value,
> -		/* 1 dword data */
> -	};
> -
> -	size = sizeof(klv_entry);
>   
> -	if (*remain < size) {
> +	klv_entry = kzalloc(size, GFP_KERNEL);
> +	if (!klv_entry) {
>   		drm_warn(&ads_to_xe(ads)->drm,
> -			 "w/a klv buffer too small to add klv id %d\n", klv_id);
> -	} else {
> -		xe_map_memcpy_to(ads_to_xe(ads), ads_to_map(ads), *offset,
> -				 klv_entry, size);
> -		*offset += size;
> -		*remain -= size;
> +			 "w/a klv buffer for klv id %d not allocated!\n", klv_id);
> +		return;
Rather than just printing an out of memory message (which is frowned 
upon by checkpatch) we should be returning an error code back up the 
stack. If we can't configure the system correctly than that could be a 
fatal error.

Alternatively, given that we currently only go up to two words of data, 
you could just use a static array that is sufficiently big for that max 
current size. Plus an assert in case someone does add a larger usage later.

Or even better, not have two copy operations in the first place. Just 
copy directly from 'dwords' to the ADS buffer.


>   	}
> -}
>   
> -static void guc_waklv_enable_simple(struct xe_guc_ads *ads,
> -				    enum xe_guc_klv_ids klv_id, u32 *offset, u32 *remain)
> -{
> -	u32 klv_entry[] = {
> -		/* 16:16 key/length */
> -		FIELD_PREP(GUC_KLV_0_KEY, klv_id) |
> -		FIELD_PREP(GUC_KLV_0_LEN, 0),
> -		/* 0 dwords data */
> -	};
> -	u32 size;
> +	/* 16:16 key/length */
> +	klv_entry[0] = FIELD_PREP(GUC_KLV_0_KEY, klv_id) |
> +		       FIELD_PREP(GUC_KLV_0_LEN, num_dwords);
>   
> -	size = sizeof(klv_entry);
> -
> -	if (xe_gt_WARN(ads_to_gt(ads), *remain < size,
> -		       "w/a klv buffer too small to add klv id %d\n", klv_id))
> -		return;
> +	/* add dwords of data */
> +	for (int i = 1; i <= num_dwords; i++)
> +		klv_entry[i] = dwords[i];
Why not a memcpy?

>   
>   	xe_map_memcpy_to(ads_to_xe(ads), ads_to_map(ads), *offset,
>   			 klv_entry, size);
>   	*offset += size;
>   	*remain -= size;
> +
> +	kfree(klv_entry);
>   }
>   
>   static void guc_waklv_init(struct xe_guc_ads *ads)
> @@ -373,49 +331,55 @@ static void guc_waklv_init(struct xe_guc_ads *ads)
>   	remain = guc_ads_waklv_size(ads);
>   
>   	if (XE_WA(gt, 14019882105) || XE_WA(gt, 16021333562))
> -		guc_waklv_enable_simple(ads,
> -					GUC_WORKAROUND_KLV_BLOCK_INTERRUPTS_WHEN_MGSR_BLOCKED,
> -					&offset, &remain);
> +		guc_waklv_enable(ads,
> +				 GUC_WORKAROUND_KLV_BLOCK_INTERRUPTS_WHEN_MGSR_BLOCKED,
> +				 NULL, 0, &offset, &remain);
If we are touching every single one of these lines then maybe make the 
code more readable/efficient by removing unnecessary line wrapping? As 
in, move the KLV key param to the end of the list so it doesn't cause a 
line break in the middle:
        guc_waklv_enable(ads, NULL, 0, &offset, &remain,
GUC_WORKAROUND_KLV_BLOCK_INTERRUPTS_WHEN_MGSR_BLOCKED);

John.

>   	if (XE_WA(gt, 18024947630))
> -		guc_waklv_enable_simple(ads,
> -					GUC_WORKAROUND_KLV_ID_GAM_PFQ_SHADOW_TAIL_POLLING,
> -					&offset, &remain);
> +		guc_waklv_enable(ads,
> +				 GUC_WORKAROUND_KLV_ID_GAM_PFQ_SHADOW_TAIL_POLLING,
> +				 NULL, 0, &offset, &remain);
>   	if (XE_WA(gt, 16022287689))
> -		guc_waklv_enable_simple(ads,
> -					GUC_WORKAROUND_KLV_ID_DISABLE_MTP_DURING_ASYNC_COMPUTE,
> -					&offset, &remain);
> +		guc_waklv_enable(ads,
> +				 GUC_WORKAROUND_KLV_ID_DISABLE_MTP_DURING_ASYNC_COMPUTE,
> +				 NULL, 0, &offset, &remain);
>   
>   	if (XE_WA(gt, 14022866841))
> -		guc_waklv_enable_simple(ads,
> -					GUC_WA_KLV_WAKE_POWER_DOMAINS_FOR_OUTBOUND_MMIO,
> -					&offset, &remain);
> +		guc_waklv_enable(ads,
> +				 GUC_WA_KLV_WAKE_POWER_DOMAINS_FOR_OUTBOUND_MMIO,
> +				 NULL, 0, &offset, &remain);
>   
>   	/*
>   	 * On RC6 exit, GuC will write register 0xB04 with the default value provided. As of now,
>   	 * the default value for this register is determined to be 0xC40. This could change in the
>   	 * future, so GuC depends on KMD to send it the correct value.
>   	 */
> -	if (XE_WA(gt, 13011645652))
> -		guc_waklv_enable_one_word(ads,
> -					  GUC_WA_KLV_NP_RD_WRITE_TO_CLEAR_RCSM_AT_CGP_LATE_RESTORE,
> -					  0xC40,
> -					  &offset, &remain);
> +	if (XE_WA(gt, 13011645652)) {
> +		u32 arr[] = {
Is 'arr' short for array? Would be clearer to just call it 'data' or 
'value' or some such.

> +			0xC40,
> +		};
Also, this could just be:
     u32 value = 0xC40;
     guv_waklv_enable(..., &value, sizeof(value) / sizeof(u32), ...);

> +		guc_waklv_enable(ads,
> +				 GUC_WA_KLV_NP_RD_WRITE_TO_CLEAR_RCSM_AT_CGP_LATE_RESTORE,
> +				 arr, 1, &offset, &remain);
> +	}
>   
>   	if (XE_WA(gt, 14022293748) || XE_WA(gt, 22019794406))
> -		guc_waklv_enable_simple(ads,
> -					GUC_WORKAROUND_KLV_ID_BACK_TO_BACK_RCS_ENGINE_RESET,
> -					&offset, &remain);
> +		guc_waklv_enable(ads,
> +				 GUC_WORKAROUND_KLV_ID_BACK_TO_BACK_RCS_ENGINE_RESET,
> +				 NULL, 0, &offset, &remain);
>   
>   	if (GUC_FIRMWARE_VER(&gt->uc.guc) >= MAKE_GUC_VER(70, 44, 0) && XE_WA(gt, 16026508708))
> -		guc_waklv_enable_simple(ads,
> -					GUC_WA_KLV_RESET_BB_STACK_PTR_ON_VF_SWITCH,
> -					&offset, &remain);
> -	if (GUC_FIRMWARE_VER(&gt->uc.guc) >= MAKE_GUC_VER(70, 47, 0) && XE_WA(gt, 16026007364))
> -		guc_waklv_enable_two_word(ads,
> -					  GUC_WA_KLV_RESTORE_UNSAVED_MEDIA_CONTROL_REG,
> -					  0x0,
> -					  0xF,
> -					  &offset, &remain);
> +		guc_waklv_enable(ads,
> +				 GUC_WA_KLV_RESET_BB_STACK_PTR_ON_VF_SWITCH,
> +				 NULL, 0, &offset, &remain);
> +	if (GUC_FIRMWARE_VER(&gt->uc.guc) >= MAKE_GUC_VER(70, 47, 0) && XE_WA(gt, 16026007364)) {
> +		u32 arr[] = {
As above.

> +			0x0,
> +			0xF,
> +		};
> +		guc_waklv_enable(ads,
> +				 GUC_WA_KLV_RESTORE_UNSAVED_MEDIA_CONTROL_REG,
> +				 arr, 2, &offset, &remain);
"sizeof(arr) / sizeof(u32)" rather than hard coding a magic number.

John.

> +	}
>   
>   	size = guc_ads_waklv_size(ads) - remain;
>   	if (!size)