[PATCH v2] drm/xe/xe_guc_ads: Consolidate guc_waklv_enable functions
John Harrison
john.c.harrison at intel.com
Fri Jul 25 17:58:21 UTC 2025
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 at intel.com>
> Signed-off-by: Jonathan Cavitt <jonathan.cavitt at intel.com>
> Cc: Lucas De Marchi <lucas.demarch at intel.com>
> Cc: Shuicheng Lin <shuicheng.lin at 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(>->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(>->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(>->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)
More information about the Intel-xe
mailing list