[PATCH 0/3] Enable GuC opt-in features

[PATCH 0/3] Enable GuC opt-in features

[Daniele Ceraolo Spurio]

The GuC supports a H2G to opt-in to extra features on a per-VF basis.
We're interested in 2 of those features:

1 - Extended CAT error info (extra debug info)
2 - Dynamic ICS (optimization)

More details on what those do in the respective patches.

This series adds code to conditionally enable those features in both
native/PF and VF. Note however that 2 other requirements need to be
satisfied for them to fully take effect:
1 - for the VF enablement to actually work we need another series to
correnctly set the GuC compatibility version [1], but these patches can
be merged in parallel as there is no direct dependency in code.
2 - Dynamic ICS needs GuC 70.40.1 or newer, but the bump to 70.40.2
(from 70.36) is currently in flight [2].

[1] https://patchwork.freedesktop.org/series/145406/
[2] https://patchwork.freedesktop.org/series/145528/
Cc: John Harrison <John.C.Harrison@Intel.com>
Cc: Julia Filipchuk <julia.filipchuk@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Lukasz Laguna <lukasz.laguna@intel.com>

Daniele Ceraolo Spurio (3):
  drm/xe/guc: move KLV helpers to common file
  drm/xe/guc: Enable extended CAT error reporting
  drm/xe/guc: Enable the Dynamic Inhibit Context Switch optimization

 drivers/gpu/drm/xe/abi/guc_actions_abi.h |  4 +
 drivers/gpu/drm/xe/abi/guc_klvs_abi.h    | 28 +++++++
 drivers/gpu/drm/xe/xe_guc.c              | 94 ++++++++++++++++++++++++
 drivers/gpu/drm/xe/xe_guc.h              |  1 +
 drivers/gpu/drm/xe/xe_guc_ads.c          | 89 +++++-----------------
 drivers/gpu/drm/xe/xe_guc_klv_helpers.c  | 62 ++++++++++++++++
 drivers/gpu/drm/xe/xe_guc_klv_helpers.h  |  7 ++
 drivers/gpu/drm/xe/xe_guc_submit.c       | 16 +++-
 drivers/gpu/drm/xe/xe_guc_types.h        |  3 +
 drivers/gpu/drm/xe/xe_uc.c               |  4 +
 10 files changed, 236 insertions(+), 72 deletions(-)

-- 
2.43.0

[PATCH 1/3] drm/xe/guc: move KLV helpers to common file

[Daniele Ceraolo Spurio]

The GuC uses the same format for all its KLVs, so having the functions
to set them in a common file will allow us to re-use it for different
types of KLVs and not just the WAs in ADS.
The next patch will make use of these functions for a new H2G command.

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: John Harrison <John.C.Harrison@Intel.com>
---
 drivers/gpu/drm/xe/xe_guc_ads.c         | 89 ++++++-------------------
 drivers/gpu/drm/xe/xe_guc_klv_helpers.c | 62 +++++++++++++++++
 drivers/gpu/drm/xe/xe_guc_klv_helpers.h |  7 ++
 3 files changed, 89 insertions(+), 69 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_guc_ads.c b/drivers/gpu/drm/xe/xe_guc_ads.c
index fab259adc380..ed3304a11812 100644
--- a/drivers/gpu/drm/xe/xe_guc_ads.c
+++ b/drivers/gpu/drm/xe/xe_guc_ads.c
@@ -22,6 +22,7 @@
 #include "xe_guc.h"
 #include "xe_guc_capture.h"
 #include "xe_guc_ct.h"
+#include "xe_guc_klv_helpers.h"
 #include "xe_hw_engine.h"
 #include "xe_lrc.h"
 #include "xe_map.h"
@@ -283,56 +284,6 @@ static size_t calculate_golden_lrc_size(struct xe_guc_ads *ads)
 	return total_size;
 }
 
-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) {
-		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;
-	}
-}
-
-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;
-
-	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;
-
-	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);
@@ -343,22 +294,22 @@ static void guc_waklv_init(struct xe_guc_ads *ads)
 	remain = guc_ads_waklv_size(ads);
 
 	if (XE_WA(gt, 14019882105))
-		guc_waklv_enable_simple(ads,
-					GUC_WORKAROUND_KLV_BLOCK_INTERRUPTS_WHEN_MGSR_BLOCKED,
-					&offset, &remain);
+		xe_guc_klv_enable_simple(ads_to_guc(ads), ads_to_map(ads),
+					 GUC_WORKAROUND_KLV_BLOCK_INTERRUPTS_WHEN_MGSR_BLOCKED,
+					 &offset, &remain);
 	if (XE_WA(gt, 18024947630))
-		guc_waklv_enable_simple(ads,
-					GUC_WORKAROUND_KLV_ID_GAM_PFQ_SHADOW_TAIL_POLLING,
-					&offset, &remain);
+		xe_guc_klv_enable_simple(ads_to_guc(ads), ads_to_map(ads),
+					 GUC_WORKAROUND_KLV_ID_GAM_PFQ_SHADOW_TAIL_POLLING,
+					 &offset, &remain);
 	if (XE_WA(gt, 16022287689))
-		guc_waklv_enable_simple(ads,
-					GUC_WORKAROUND_KLV_ID_DISABLE_MTP_DURING_ASYNC_COMPUTE,
-					&offset, &remain);
+		xe_guc_klv_enable_simple(ads_to_guc(ads), ads_to_map(ads),
+					 GUC_WORKAROUND_KLV_ID_DISABLE_MTP_DURING_ASYNC_COMPUTE,
+					 &offset, &remain);
 
 	if (XE_WA(gt, 14022866841))
-		guc_waklv_enable_simple(ads,
-					GUC_WA_KLV_WAKE_POWER_DOMAINS_FOR_OUTBOUND_MMIO,
-					&offset, &remain);
+		xe_guc_klv_enable_simple(ads_to_guc(ads), ads_to_map(ads),
+					 GUC_WA_KLV_WAKE_POWER_DOMAINS_FOR_OUTBOUND_MMIO,
+					 &offset, &remain);
 
 	/*
 	 * On RC6 exit, GuC will write register 0xB04 with the default value provided. As of now,
@@ -366,15 +317,15 @@ static void guc_waklv_init(struct xe_guc_ads *ads)
 	 * 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);
+		xe_guc_klv_enable_one_word(ads_to_guc(ads), ads_to_map(ads),
+					   GUC_WA_KLV_NP_RD_WRITE_TO_CLEAR_RCSM_AT_CGP_LATE_RESTORE,
+					   0xC40,
+					   &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);
+		xe_guc_klv_enable_simple(ads_to_guc(ads), ads_to_map(ads),
+					 GUC_WORKAROUND_KLV_ID_BACK_TO_BACK_RCS_ENGINE_RESET,
+					 &offset, &remain);
 
 	size = guc_ads_waklv_size(ads) - remain;
 	if (!size)
diff --git a/drivers/gpu/drm/xe/xe_guc_klv_helpers.c b/drivers/gpu/drm/xe/xe_guc_klv_helpers.c
index 146a6eda9e06..50eab2086ee3 100644
--- a/drivers/gpu/drm/xe/xe_guc_klv_helpers.c
+++ b/drivers/gpu/drm/xe/xe_guc_klv_helpers.c
@@ -7,8 +7,11 @@
 #include <drm/drm_print.h>
 
 #include "abi/guc_klvs_abi.h"
+#include "xe_gt_printk.h"
+#include "xe_guc.h"
 #include "xe_guc_klv_helpers.h"
 #include "xe_guc_klv_thresholds_set.h"
+#include "xe_map.h"
 
 #define make_u64(hi, lo) ((u64)((u64)(u32)(hi) << 32 | (u32)(lo)))
 
@@ -146,3 +149,62 @@ int xe_guc_klv_count(const u32 *klvs, u32 num_dwords)
 
 	return num_dwords ? -ENODATA : num_klvs;
 }
+
+static void emit_klv(struct xe_guc *guc, struct iosys_map *map, u32 *klv_entry,
+		     u32 size, u32 *offset, u32 *remain)
+{
+	if (xe_gt_WARN(guc_to_gt(guc), *remain < size,
+		       "klv buffer too small to add klv id 0x%04x\n",
+		       FIELD_GET(GUC_KLV_0_KEY, klv_entry[0])))
+		return;
+
+	xe_map_memcpy_to(guc_to_xe(guc), map, *offset, klv_entry, size);
+	*offset += size;
+	*remain -= size;
+}
+
+/**
+ * xe_guc_klv_enable_simple - Emits a KLV with no data to an iosys mapped buffer
+ * @guc: the guc structure
+ * @map: the iosys_map to write to
+ * @klv_id: the KLV to enable
+ * @offset: pointer to a variable holding the offset to write to
+ * @remain: pointer to a variable holding the remaining writing space available
+ *
+ * The function checks if there is enough space to emit a KLV with no data and
+ * if so writes it to memory. After the write, the offset and remain variables
+ * are respectively increased and decreased of the written size to be ready for
+ * the next emission.
+ */
+void xe_guc_klv_enable_simple(struct xe_guc *guc, struct iosys_map *map,
+			      u16 klv_id, u32 *offset, u32 *remain)
+{
+	u32 klv_entry = PREP_GUC_KLV(klv_id, 0);
+
+	emit_klv(guc, map, &klv_entry, sizeof(klv_entry), offset, remain);
+}
+
+/**
+ * xe_guc_klv_enable_one_word - Emits a KLV with 1 DW data to an iosys mapped buffer
+ * @guc: the guc structure
+ * @map: the iosys_map to write to
+ * @klv_id: the KLV to enable
+ * @value: the data associated with the KLV
+ * @offset: pointer to a variable holding the offset to write to
+ * @remain: pointer to a variable holding the remaining writing space available
+ *
+ * The function checks if there is enough space to emit a KLV with 1 DW data and
+ * if so writes it to memory. After the write, the offset and remain variables
+ * are respectively increased and decreased of the written size to be ready for
+ * the next emission.
+ */
+void xe_guc_klv_enable_one_word(struct xe_guc *guc, struct iosys_map *map,
+				u16 klv_id, u32 value, u32 *offset, u32 *remain)
+{
+	u32 klv_entry[] = {
+		PREP_GUC_KLV(klv_id, 1),
+		value,
+	};
+
+	emit_klv(guc, map, klv_entry, sizeof(klv_entry), offset, remain);
+}
diff --git a/drivers/gpu/drm/xe/xe_guc_klv_helpers.h b/drivers/gpu/drm/xe/xe_guc_klv_helpers.h
index c676d21c173b..231f7c4b0947 100644
--- a/drivers/gpu/drm/xe/xe_guc_klv_helpers.h
+++ b/drivers/gpu/drm/xe/xe_guc_klv_helpers.h
@@ -10,6 +10,8 @@
 #include <linux/types.h>
 
 struct drm_printer;
+struct iosys_map;
+struct xe_guc;
 
 const char *xe_guc_klv_key_to_string(u16 key);
 
@@ -61,4 +63,9 @@ int xe_guc_klv_count(const u32 *klvs, u32 num_dwords);
 #define PREP_GUC_KLV_TAG(TAG) \
 	PREP_GUC_KLV_CONST(MAKE_GUC_KLV_KEY(TAG), MAKE_GUC_KLV_LEN(TAG))
 
+void xe_guc_klv_enable_simple(struct xe_guc *guc, struct iosys_map *map,
+			      u16 klv_id, u32 *offset, u32 *remain);
+void xe_guc_klv_enable_one_word(struct xe_guc *guc, struct iosys_map *map,
+				u16 klv_id, u32 value, u32 *offset, u32 *remain);
+
 #endif
-- 
2.43.0

Re: [PATCH 1/3] drm/xe/guc: move KLV helpers to common file

[Michal Wajdeczko]

On 27.02.2025 02:05, Daniele Ceraolo Spurio wrote:
> The GuC uses the same format for all its KLVs, so having the functions
> to set them in a common file will allow us to re-use it for different
> types of KLVs and not just the WAs in ADS.
> The next patch will make use of these functions for a new H2G command.
> 
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: John Harrison <John.C.Harrison@Intel.com>
> ---
>  drivers/gpu/drm/xe/xe_guc_ads.c         | 89 ++++++-------------------
>  drivers/gpu/drm/xe/xe_guc_klv_helpers.c | 62 +++++++++++++++++
>  drivers/gpu/drm/xe/xe_guc_klv_helpers.h |  7 ++
>  3 files changed, 89 insertions(+), 69 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_guc_ads.c b/drivers/gpu/drm/xe/xe_guc_ads.c
> index fab259adc380..ed3304a11812 100644
> --- a/drivers/gpu/drm/xe/xe_guc_ads.c
> +++ b/drivers/gpu/drm/xe/xe_guc_ads.c
> @@ -22,6 +22,7 @@
>  #include "xe_guc.h"
>  #include "xe_guc_capture.h"
>  #include "xe_guc_ct.h"
> +#include "xe_guc_klv_helpers.h"
>  #include "xe_hw_engine.h"
>  #include "xe_lrc.h"
>  #include "xe_map.h"
> @@ -283,56 +284,6 @@ static size_t calculate_golden_lrc_size(struct xe_guc_ads *ads)
>  	return total_size;
>  }
>  
> -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) {
> -		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;
> -	}
> -}
> -
> -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;
> -
> -	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;
> -
> -	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);
> @@ -343,22 +294,22 @@ static void guc_waklv_init(struct xe_guc_ads *ads)
>  	remain = guc_ads_waklv_size(ads);
>  
>  	if (XE_WA(gt, 14019882105))
> -		guc_waklv_enable_simple(ads,
> -					GUC_WORKAROUND_KLV_BLOCK_INTERRUPTS_WHEN_MGSR_BLOCKED,
> -					&offset, &remain);
> +		xe_guc_klv_enable_simple(ads_to_guc(ads), ads_to_map(ads),
> +					 GUC_WORKAROUND_KLV_BLOCK_INTERRUPTS_WHEN_MGSR_BLOCKED,
> +					 &offset, &remain);
>  	if (XE_WA(gt, 18024947630))
> -		guc_waklv_enable_simple(ads,
> -					GUC_WORKAROUND_KLV_ID_GAM_PFQ_SHADOW_TAIL_POLLING,
> -					&offset, &remain);
> +		xe_guc_klv_enable_simple(ads_to_guc(ads), ads_to_map(ads),
> +					 GUC_WORKAROUND_KLV_ID_GAM_PFQ_SHADOW_TAIL_POLLING,
> +					 &offset, &remain);
>  	if (XE_WA(gt, 16022287689))
> -		guc_waklv_enable_simple(ads,
> -					GUC_WORKAROUND_KLV_ID_DISABLE_MTP_DURING_ASYNC_COMPUTE,
> -					&offset, &remain);
> +		xe_guc_klv_enable_simple(ads_to_guc(ads), ads_to_map(ads),
> +					 GUC_WORKAROUND_KLV_ID_DISABLE_MTP_DURING_ASYNC_COMPUTE,
> +					 &offset, &remain);
>  
>  	if (XE_WA(gt, 14022866841))
> -		guc_waklv_enable_simple(ads,
> -					GUC_WA_KLV_WAKE_POWER_DOMAINS_FOR_OUTBOUND_MMIO,
> -					&offset, &remain);
> +		xe_guc_klv_enable_simple(ads_to_guc(ads), ads_to_map(ads),
> +					 GUC_WA_KLV_WAKE_POWER_DOMAINS_FOR_OUTBOUND_MMIO,
> +					 &offset, &remain);
>  
>  	/*
>  	 * On RC6 exit, GuC will write register 0xB04 with the default value provided. As of now,
> @@ -366,15 +317,15 @@ static void guc_waklv_init(struct xe_guc_ads *ads)
>  	 * 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);
> +		xe_guc_klv_enable_one_word(ads_to_guc(ads), ads_to_map(ads),
> +					   GUC_WA_KLV_NP_RD_WRITE_TO_CLEAR_RCSM_AT_CGP_LATE_RESTORE,
> +					   0xC40,
> +					   &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);
> +		xe_guc_klv_enable_simple(ads_to_guc(ads), ads_to_map(ads),
> +					 GUC_WORKAROUND_KLV_ID_BACK_TO_BACK_RCS_ENGINE_RESET,
> +					 &offset, &remain);
>  
>  	size = guc_ads_waklv_size(ads) - remain;
>  	if (!size)
> diff --git a/drivers/gpu/drm/xe/xe_guc_klv_helpers.c b/drivers/gpu/drm/xe/xe_guc_klv_helpers.c
> index 146a6eda9e06..50eab2086ee3 100644
> --- a/drivers/gpu/drm/xe/xe_guc_klv_helpers.c
> +++ b/drivers/gpu/drm/xe/xe_guc_klv_helpers.c
> @@ -7,8 +7,11 @@
>  #include <drm/drm_print.h>
>  
>  #include "abi/guc_klvs_abi.h"
> +#include "xe_gt_printk.h"
> +#include "xe_guc.h"
>  #include "xe_guc_klv_helpers.h"
>  #include "xe_guc_klv_thresholds_set.h"
> +#include "xe_map.h"
>  
>  #define make_u64(hi, lo) ((u64)((u64)(u32)(hi) << 32 | (u32)(lo)))
>  
> @@ -146,3 +149,62 @@ int xe_guc_klv_count(const u32 *klvs, u32 num_dwords)
>  
>  	return num_dwords ? -ENODATA : num_klvs;
>  }
> +
> +static void emit_klv(struct xe_guc *guc, struct iosys_map *map, u32 *klv_entry,
> +		     u32 size, u32 *offset, u32 *remain)

s/klv_entry/klv

s/size/num_dwords

KLVs are always u32 aligned
'size/offset/remain' all should be in dwords

this will also match rest of functions in this file

> +{
> +	if (xe_gt_WARN(guc_to_gt(guc), *remain < size,

do we need xe_gt_WARN in production?
maybe xe_gt_assert will suffice?

> +		       "klv buffer too small to add klv id 0x%04x\n",

s/klv/KLV

> +		       FIELD_GET(GUC_KLV_0_KEY, klv_entry[0])))
> +		return;
> +
> +	xe_map_memcpy_to(guc_to_xe(guc), map, *offset, klv_entry, size);
> +	*offset += size;
> +	*remain -= size;
> +}
> +
> +/**
> + * xe_guc_klv_enable_simple - Emits a KLV with no data to an iosys mapped buffer
> + * @guc: the guc structure
> + * @map: the iosys_map to write to
> + * @klv_id: the KLV to enable

key ? there is no Len nor Value

this will follow other functions in this file

> + * @offset: pointer to a variable holding the offset to write to
> + * @remain: pointer to a variable holding the remaining writing space available
> + *
> + * The function checks if there is enough space to emit a KLV with no data and
> + * if so writes it to memory. After the write, the offset and remain variables
> + * are respectively increased and decreased of the written size to be ready for
> + * the next emission.
> + */
> +void xe_guc_klv_enable_simple(struct xe_guc *guc, struct iosys_map *map,
> +			      u16 klv_id, u32 *offset, u32 *remain)
> +{
> +	u32 klv_entry = PREP_GUC_KLV(klv_id, 0);

	u32 klv[] = { PREP_GUC_KLV(key, 0) };

	emit(... ARRAY_SIZE(klv)
> +
> +	emit_klv(guc, map, &klv_entry, sizeof(klv_entry), offset, remain);
> +}
> +
> +/**
> + * xe_guc_klv_enable_one_word - Emits a KLV with 1 DW data to an iosys mapped buffer
> + * @guc: the guc structure
> + * @map: the iosys_map to write to
> + * @klv_id: the KLV to enable

s/klv_id/key

> + * @value: the data associated with the KLV
> + * @offset: pointer to a variable holding the offset to write to
> + * @remain: pointer to a variable holding the remaining writing space available
> + *
> + * The function checks if there is enough space to emit a KLV with 1 DW data and
> + * if so writes it to memory. After the write, the offset and remain variables
> + * are respectively increased and decreased of the written size to be ready for
> + * the next emission.
> + */
> +void xe_guc_klv_enable_one_word(struct xe_guc *guc, struct iosys_map *map,
> +				u16 klv_id, u32 value, u32 *offset, u32 *remain)
> +{
> +	u32 klv_entry[] = {
> +		PREP_GUC_KLV(klv_id, 1),
> +		value,
> +	};
> +
> +	emit_klv(guc, map, klv_entry, sizeof(klv_entry), offset, remain);
> +}
> diff --git a/drivers/gpu/drm/xe/xe_guc_klv_helpers.h b/drivers/gpu/drm/xe/xe_guc_klv_helpers.h
> index c676d21c173b..231f7c4b0947 100644
> --- a/drivers/gpu/drm/xe/xe_guc_klv_helpers.h
> +++ b/drivers/gpu/drm/xe/xe_guc_klv_helpers.h
> @@ -10,6 +10,8 @@
>  #include <linux/types.h>
>  
>  struct drm_printer;
> +struct iosys_map;
> +struct xe_guc;
>  
>  const char *xe_guc_klv_key_to_string(u16 key);
>  
> @@ -61,4 +63,9 @@ int xe_guc_klv_count(const u32 *klvs, u32 num_dwords);
>  #define PREP_GUC_KLV_TAG(TAG) \
>  	PREP_GUC_KLV_CONST(MAKE_GUC_KLV_KEY(TAG), MAKE_GUC_KLV_LEN(TAG))
>  
> +void xe_guc_klv_enable_simple(struct xe_guc *guc, struct iosys_map *map,
> +			      u16 klv_id, u32 *offset, u32 *remain);
> +void xe_guc_klv_enable_one_word(struct xe_guc *guc, struct iosys_map *map,
> +				u16 klv_id, u32 value, u32 *offset, u32 *remain);

hmm, "enable" does not fit here, maybe:

xe_guc_klv_emit_empty / no_data / no_value
xe_guc_klv_emit_dword

and those should be inlines that use:

void xe_guc_klv_emit_simple(
	struct xe_guc *guc,
	struct iosys_map *map,
	u16 key, u16 len, const u32 *klv,
	u32 *offset, u32 *remain);

like:

inline xe_guc_klv_emit_simple_empty(.. key ..)
{
	xe_guc_klv_emit_simple(.. key, 0, NULL, ..);
}


inline xe_guc_klv_emit_simple_dword(.. key, value ..)
{
	xe_guc_klv_emit_simple(.. key, 1, &value, ..);
}

> +
>  #endif

Re: [PATCH 1/3] drm/xe/guc: move KLV helpers to common file

[Daniele Ceraolo Spurio]

On 2/27/2025 3:29 PM, Michal Wajdeczko wrote:
>
> On 27.02.2025 02:05, Daniele Ceraolo Spurio wrote:
>> The GuC uses the same format for all its KLVs, so having the functions
>> to set them in a common file will allow us to re-use it for different
>> types of KLVs and not just the WAs in ADS.
>> The next patch will make use of these functions for a new H2G command.
>>
>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> Cc: John Harrison <John.C.Harrison@Intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_guc_ads.c         | 89 ++++++-------------------
>>   drivers/gpu/drm/xe/xe_guc_klv_helpers.c | 62 +++++++++++++++++
>>   drivers/gpu/drm/xe/xe_guc_klv_helpers.h |  7 ++
>>   3 files changed, 89 insertions(+), 69 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_guc_ads.c b/drivers/gpu/drm/xe/xe_guc_ads.c
>> index fab259adc380..ed3304a11812 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_ads.c
>> +++ b/drivers/gpu/drm/xe/xe_guc_ads.c
>> @@ -22,6 +22,7 @@
>>   #include "xe_guc.h"
>>   #include "xe_guc_capture.h"
>>   #include "xe_guc_ct.h"
>> +#include "xe_guc_klv_helpers.h"
>>   #include "xe_hw_engine.h"
>>   #include "xe_lrc.h"
>>   #include "xe_map.h"
>> @@ -283,56 +284,6 @@ static size_t calculate_golden_lrc_size(struct xe_guc_ads *ads)
>>   	return total_size;
>>   }
>>   
>> -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) {
>> -		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;
>> -	}
>> -}
>> -
>> -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;
>> -
>> -	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;
>> -
>> -	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);
>> @@ -343,22 +294,22 @@ static void guc_waklv_init(struct xe_guc_ads *ads)
>>   	remain = guc_ads_waklv_size(ads);
>>   
>>   	if (XE_WA(gt, 14019882105))
>> -		guc_waklv_enable_simple(ads,
>> -					GUC_WORKAROUND_KLV_BLOCK_INTERRUPTS_WHEN_MGSR_BLOCKED,
>> -					&offset, &remain);
>> +		xe_guc_klv_enable_simple(ads_to_guc(ads), ads_to_map(ads),
>> +					 GUC_WORKAROUND_KLV_BLOCK_INTERRUPTS_WHEN_MGSR_BLOCKED,
>> +					 &offset, &remain);
>>   	if (XE_WA(gt, 18024947630))
>> -		guc_waklv_enable_simple(ads,
>> -					GUC_WORKAROUND_KLV_ID_GAM_PFQ_SHADOW_TAIL_POLLING,
>> -					&offset, &remain);
>> +		xe_guc_klv_enable_simple(ads_to_guc(ads), ads_to_map(ads),
>> +					 GUC_WORKAROUND_KLV_ID_GAM_PFQ_SHADOW_TAIL_POLLING,
>> +					 &offset, &remain);
>>   	if (XE_WA(gt, 16022287689))
>> -		guc_waklv_enable_simple(ads,
>> -					GUC_WORKAROUND_KLV_ID_DISABLE_MTP_DURING_ASYNC_COMPUTE,
>> -					&offset, &remain);
>> +		xe_guc_klv_enable_simple(ads_to_guc(ads), ads_to_map(ads),
>> +					 GUC_WORKAROUND_KLV_ID_DISABLE_MTP_DURING_ASYNC_COMPUTE,
>> +					 &offset, &remain);
>>   
>>   	if (XE_WA(gt, 14022866841))
>> -		guc_waklv_enable_simple(ads,
>> -					GUC_WA_KLV_WAKE_POWER_DOMAINS_FOR_OUTBOUND_MMIO,
>> -					&offset, &remain);
>> +		xe_guc_klv_enable_simple(ads_to_guc(ads), ads_to_map(ads),
>> +					 GUC_WA_KLV_WAKE_POWER_DOMAINS_FOR_OUTBOUND_MMIO,
>> +					 &offset, &remain);
>>   
>>   	/*
>>   	 * On RC6 exit, GuC will write register 0xB04 with the default value provided. As of now,
>> @@ -366,15 +317,15 @@ static void guc_waklv_init(struct xe_guc_ads *ads)
>>   	 * 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);
>> +		xe_guc_klv_enable_one_word(ads_to_guc(ads), ads_to_map(ads),
>> +					   GUC_WA_KLV_NP_RD_WRITE_TO_CLEAR_RCSM_AT_CGP_LATE_RESTORE,
>> +					   0xC40,
>> +					   &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);
>> +		xe_guc_klv_enable_simple(ads_to_guc(ads), ads_to_map(ads),
>> +					 GUC_WORKAROUND_KLV_ID_BACK_TO_BACK_RCS_ENGINE_RESET,
>> +					 &offset, &remain);
>>   
>>   	size = guc_ads_waklv_size(ads) - remain;
>>   	if (!size)
>> diff --git a/drivers/gpu/drm/xe/xe_guc_klv_helpers.c b/drivers/gpu/drm/xe/xe_guc_klv_helpers.c
>> index 146a6eda9e06..50eab2086ee3 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_klv_helpers.c
>> +++ b/drivers/gpu/drm/xe/xe_guc_klv_helpers.c
>> @@ -7,8 +7,11 @@
>>   #include <drm/drm_print.h>
>>   
>>   #include "abi/guc_klvs_abi.h"
>> +#include "xe_gt_printk.h"
>> +#include "xe_guc.h"
>>   #include "xe_guc_klv_helpers.h"
>>   #include "xe_guc_klv_thresholds_set.h"
>> +#include "xe_map.h"
>>   
>>   #define make_u64(hi, lo) ((u64)((u64)(u32)(hi) << 32 | (u32)(lo)))
>>   
>> @@ -146,3 +149,62 @@ int xe_guc_klv_count(const u32 *klvs, u32 num_dwords)
>>   
>>   	return num_dwords ? -ENODATA : num_klvs;
>>   }
>> +
>> +static void emit_klv(struct xe_guc *guc, struct iosys_map *map, u32 *klv_entry,
>> +		     u32 size, u32 *offset, u32 *remain)
> s/klv_entry/klv
>
> s/size/num_dwords
>
> KLVs are always u32 aligned
> 'size/offset/remain' all should be in dwords

That would mean having to convert back and forth between u8 and u32:

- allocation is in u8 aligned
- params to this function would be u32 aligned
- xe_map_memcpy_to below needs u8 alignment, so convert back
- the GuC expects a u8 aligned size, so need to convert remain back to u8

Just using u8 everywhere seems simpler instead of converting back and forth.

>
> this will also match rest of functions in this file
>
>> +{
>> +	if (xe_gt_WARN(guc_to_gt(guc), *remain < size,
> do we need xe_gt_WARN in production?
> maybe xe_gt_assert will suffice?

I've kept this the same as what was in the original function. Not sure 
why it was xe_gt_WARN instead of assert.

>
>> +		       "klv buffer too small to add klv id 0x%04x\n",
> s/klv/KLV
>
>> +		       FIELD_GET(GUC_KLV_0_KEY, klv_entry[0])))
>> +		return;
>> +
>> +	xe_map_memcpy_to(guc_to_xe(guc), map, *offset, klv_entry, size);
>> +	*offset += size;
>> +	*remain -= size;
>> +}
>> +
>> +/**
>> + * xe_guc_klv_enable_simple - Emits a KLV with no data to an iosys mapped buffer
>> + * @guc: the guc structure
>> + * @map: the iosys_map to write to
>> + * @klv_id: the KLV to enable
> key ? there is no Len nor Value
>
> this will follow other functions in this file

ok, will rename.

>
>> + * @offset: pointer to a variable holding the offset to write to
>> + * @remain: pointer to a variable holding the remaining writing space available
>> + *
>> + * The function checks if there is enough space to emit a KLV with no data and
>> + * if so writes it to memory. After the write, the offset and remain variables
>> + * are respectively increased and decreased of the written size to be ready for
>> + * the next emission.
>> + */
>> +void xe_guc_klv_enable_simple(struct xe_guc *guc, struct iosys_map *map,
>> +			      u16 klv_id, u32 *offset, u32 *remain)
>> +{
>> +	u32 klv_entry = PREP_GUC_KLV(klv_id, 0);
> 	u32 klv[] = { PREP_GUC_KLV(key, 0) };
>
> 	emit(... ARRAY_SIZE(klv)

Why? this function is specifically targeted to writing a single u32, why 
write it as a u32[1]?

>> +
>> +	emit_klv(guc, map, &klv_entry, sizeof(klv_entry), offset, remain);
>> +}
>> +
>> +/**
>> + * xe_guc_klv_enable_one_word - Emits a KLV with 1 DW data to an iosys mapped buffer
>> + * @guc: the guc structure
>> + * @map: the iosys_map to write to
>> + * @klv_id: the KLV to enable
> s/klv_id/key
>
>> + * @value: the data associated with the KLV
>> + * @offset: pointer to a variable holding the offset to write to
>> + * @remain: pointer to a variable holding the remaining writing space available
>> + *
>> + * The function checks if there is enough space to emit a KLV with 1 DW data and
>> + * if so writes it to memory. After the write, the offset and remain variables
>> + * are respectively increased and decreased of the written size to be ready for
>> + * the next emission.
>> + */
>> +void xe_guc_klv_enable_one_word(struct xe_guc *guc, struct iosys_map *map,
>> +				u16 klv_id, u32 value, u32 *offset, u32 *remain)
>> +{
>> +	u32 klv_entry[] = {
>> +		PREP_GUC_KLV(klv_id, 1),
>> +		value,
>> +	};
>> +
>> +	emit_klv(guc, map, klv_entry, sizeof(klv_entry), offset, remain);
>> +}
>> diff --git a/drivers/gpu/drm/xe/xe_guc_klv_helpers.h b/drivers/gpu/drm/xe/xe_guc_klv_helpers.h
>> index c676d21c173b..231f7c4b0947 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_klv_helpers.h
>> +++ b/drivers/gpu/drm/xe/xe_guc_klv_helpers.h
>> @@ -10,6 +10,8 @@
>>   #include <linux/types.h>
>>   
>>   struct drm_printer;
>> +struct iosys_map;
>> +struct xe_guc;
>>   
>>   const char *xe_guc_klv_key_to_string(u16 key);
>>   
>> @@ -61,4 +63,9 @@ int xe_guc_klv_count(const u32 *klvs, u32 num_dwords);
>>   #define PREP_GUC_KLV_TAG(TAG) \
>>   	PREP_GUC_KLV_CONST(MAKE_GUC_KLV_KEY(TAG), MAKE_GUC_KLV_LEN(TAG))
>>   
>> +void xe_guc_klv_enable_simple(struct xe_guc *guc, struct iosys_map *map,
>> +			      u16 klv_id, u32 *offset, u32 *remain);
>> +void xe_guc_klv_enable_one_word(struct xe_guc *guc, struct iosys_map *map,
>> +				u16 klv_id, u32 value, u32 *offset, u32 *remain);
> hmm, "enable" does not fit here, maybe:
>
> xe_guc_klv_emit_empty / no_data / no_value
> xe_guc_klv_emit_dword

emit_nodata should work.

>
> and those should be inlines that use:
>
> void xe_guc_klv_emit_simple(
> 	struct xe_guc *guc,
> 	struct iosys_map *map,
> 	u16 key, u16 len, const u32 *klv,
> 	u32 *offset, u32 *remain);
>
> like:
>
> inline xe_guc_klv_emit_simple_empty(.. key ..)
> {
> 	xe_guc_klv_emit_simple(.. key, 0, NULL, ..);
> }
>
>
> inline xe_guc_klv_emit_simple_dword(.. key, value ..)
> {
> 	xe_guc_klv_emit_simple(.. key, 1, &value, ..);
> }

This seems to just make things harder. xe_guc_klv_emit_simple() would 
need to handle the case where we have data differently from the case 
where we don't, potentially with 2 separate memcpys (one always done for 
the key and one optional for the data). I really don't see the benefit 
when the current approach still makes most of the code common in 
emit_klv() and only has separate arrays. Also, that wouldn't be a 
"simple" emission anymore since it'd handle all the possible cases, so 
the naming also wouldn't fit.

Daniele

>
>> +
>>   #endif

Re: [PATCH 1/3] drm/xe/guc: move KLV helpers to common file

[Michal Wajdeczko]

On 28.02.2025 00:59, Daniele Ceraolo Spurio wrote:
> 
> 
> On 2/27/2025 3:29 PM, Michal Wajdeczko wrote:
>>
>> On 27.02.2025 02:05, Daniele Ceraolo Spurio wrote:
>>> The GuC uses the same format for all its KLVs, so having the functions
>>> to set them in a common file will allow us to re-use it for different
>>> types of KLVs and not just the WAs in ADS.
>>> The next patch will make use of these functions for a new H2G command.
>>>
>>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>> Cc: John Harrison <John.C.Harrison@Intel.com>
>>> ---
>>>   drivers/gpu/drm/xe/xe_guc_ads.c         | 89 ++++++-------------------
>>>   drivers/gpu/drm/xe/xe_guc_klv_helpers.c | 62 +++++++++++++++++
>>>   drivers/gpu/drm/xe/xe_guc_klv_helpers.h |  7 ++
>>>   3 files changed, 89 insertions(+), 69 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/xe/xe_guc_ads.c b/drivers/gpu/drm/xe/
>>> xe_guc_ads.c
>>> index fab259adc380..ed3304a11812 100644
>>> --- a/drivers/gpu/drm/xe/xe_guc_ads.c
>>> +++ b/drivers/gpu/drm/xe/xe_guc_ads.c
>>> @@ -22,6 +22,7 @@
>>>   #include "xe_guc.h"
>>>   #include "xe_guc_capture.h"
>>>   #include "xe_guc_ct.h"
>>> +#include "xe_guc_klv_helpers.h"
>>>   #include "xe_hw_engine.h"
>>>   #include "xe_lrc.h"
>>>   #include "xe_map.h"
>>> @@ -283,56 +284,6 @@ static size_t calculate_golden_lrc_size(struct
>>> xe_guc_ads *ads)
>>>       return total_size;
>>>   }
>>>   -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) {
>>> -        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;
>>> -    }
>>> -}
>>> -
>>> -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;
>>> -
>>> -    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;
>>> -
>>> -    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);
>>> @@ -343,22 +294,22 @@ static void guc_waklv_init(struct xe_guc_ads *ads)
>>>       remain = guc_ads_waklv_size(ads);
>>>         if (XE_WA(gt, 14019882105))
>>> -        guc_waklv_enable_simple(ads,
>>> -                   
>>> GUC_WORKAROUND_KLV_BLOCK_INTERRUPTS_WHEN_MGSR_BLOCKED,
>>> -                    &offset, &remain);
>>> +        xe_guc_klv_enable_simple(ads_to_guc(ads), ads_to_map(ads),
>>> +                    
>>> GUC_WORKAROUND_KLV_BLOCK_INTERRUPTS_WHEN_MGSR_BLOCKED,
>>> +                     &offset, &remain);
>>>       if (XE_WA(gt, 18024947630))
>>> -        guc_waklv_enable_simple(ads,
>>> -                    GUC_WORKAROUND_KLV_ID_GAM_PFQ_SHADOW_TAIL_POLLING,
>>> -                    &offset, &remain);
>>> +        xe_guc_klv_enable_simple(ads_to_guc(ads), ads_to_map(ads),
>>> +                     GUC_WORKAROUND_KLV_ID_GAM_PFQ_SHADOW_TAIL_POLLING,
>>> +                     &offset, &remain);
>>>       if (XE_WA(gt, 16022287689))
>>> -        guc_waklv_enable_simple(ads,
>>> -                   
>>> GUC_WORKAROUND_KLV_ID_DISABLE_MTP_DURING_ASYNC_COMPUTE,
>>> -                    &offset, &remain);
>>> +        xe_guc_klv_enable_simple(ads_to_guc(ads), ads_to_map(ads),
>>> +                    
>>> GUC_WORKAROUND_KLV_ID_DISABLE_MTP_DURING_ASYNC_COMPUTE,
>>> +                     &offset, &remain);
>>>         if (XE_WA(gt, 14022866841))
>>> -        guc_waklv_enable_simple(ads,
>>> -                    GUC_WA_KLV_WAKE_POWER_DOMAINS_FOR_OUTBOUND_MMIO,
>>> -                    &offset, &remain);
>>> +        xe_guc_klv_enable_simple(ads_to_guc(ads), ads_to_map(ads),
>>> +                     GUC_WA_KLV_WAKE_POWER_DOMAINS_FOR_OUTBOUND_MMIO,
>>> +                     &offset, &remain);
>>>         /*
>>>        * On RC6 exit, GuC will write register 0xB04 with the default
>>> value provided. As of now,
>>> @@ -366,15 +317,15 @@ static void guc_waklv_init(struct xe_guc_ads *ads)
>>>        * 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);
>>> +        xe_guc_klv_enable_one_word(ads_to_guc(ads), ads_to_map(ads),
>>> +                      
>>> GUC_WA_KLV_NP_RD_WRITE_TO_CLEAR_RCSM_AT_CGP_LATE_RESTORE,
>>> +                       0xC40,
>>> +                       &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);
>>> +        xe_guc_klv_enable_simple(ads_to_guc(ads), ads_to_map(ads),
>>> +                    
>>> GUC_WORKAROUND_KLV_ID_BACK_TO_BACK_RCS_ENGINE_RESET,
>>> +                     &offset, &remain);
>>>         size = guc_ads_waklv_size(ads) - remain;
>>>       if (!size)
>>> diff --git a/drivers/gpu/drm/xe/xe_guc_klv_helpers.c b/drivers/gpu/
>>> drm/xe/xe_guc_klv_helpers.c
>>> index 146a6eda9e06..50eab2086ee3 100644
>>> --- a/drivers/gpu/drm/xe/xe_guc_klv_helpers.c
>>> +++ b/drivers/gpu/drm/xe/xe_guc_klv_helpers.c
>>> @@ -7,8 +7,11 @@
>>>   #include <drm/drm_print.h>
>>>     #include "abi/guc_klvs_abi.h"
>>> +#include "xe_gt_printk.h"
>>> +#include "xe_guc.h"
>>>   #include "xe_guc_klv_helpers.h"
>>>   #include "xe_guc_klv_thresholds_set.h"
>>> +#include "xe_map.h"
>>>     #define make_u64(hi, lo) ((u64)((u64)(u32)(hi) << 32 | (u32)(lo)))
>>>   @@ -146,3 +149,62 @@ int xe_guc_klv_count(const u32 *klvs, u32
>>> num_dwords)
>>>         return num_dwords ? -ENODATA : num_klvs;
>>>   }
>>> +
>>> +static void emit_klv(struct xe_guc *guc, struct iosys_map *map, u32
>>> *klv_entry,
>>> +             u32 size, u32 *offset, u32 *remain)
>> s/klv_entry/klv
>>
>> s/size/num_dwords
>>
>> KLVs are always u32 aligned
>> 'size/offset/remain' all should be in dwords
> 
> That would mean having to convert back and forth between u8 and u32:
> 
> - allocation is in u8 aligned
> - params to this function would be u32 aligned

but for generic KLV functions it doesn't make sense to pass u8 sizes
and we should aim to define function signatures that enforce proper logic

> - xe_map_memcpy_to below needs u8 alignment, so convert back
> - the GuC expects a u8 aligned size, so need to convert remain back to u8

that looks like a mistake done in this action definition, since all
initial actions that introduced KLV concept (like UPDATE_VGT_POLICY,
UPDATE_VF_CFG) were defined to operate on dwords as size

> 
> Just using u8 everywhere seems simpler instead of converting back and
> forth.
> 
>>
>> this will also match rest of functions in this file
>>
>>> +{
>>> +    if (xe_gt_WARN(guc_to_gt(guc), *remain < size,
>> do we need xe_gt_WARN in production?
>> maybe xe_gt_assert will suffice?
> 
> I've kept this the same as what was in the original function. Not sure
> why it was xe_gt_WARN instead of assert.
> 
>>
>>> +               "klv buffer too small to add klv id 0x%04x\n",
>> s/klv/KLV
>>
>>> +               FIELD_GET(GUC_KLV_0_KEY, klv_entry[0])))
>>> +        return;
>>> +
>>> +    xe_map_memcpy_to(guc_to_xe(guc), map, *offset, klv_entry, size);
>>> +    *offset += size;
>>> +    *remain -= size;
>>> +}
>>> +
>>> +/**
>>> + * xe_guc_klv_enable_simple - Emits a KLV with no data to an iosys
>>> mapped buffer
>>> + * @guc: the guc structure
>>> + * @map: the iosys_map to write to
>>> + * @klv_id: the KLV to enable
>> key ? there is no Len nor Value
>>
>> this will follow other functions in this file
> 
> ok, will rename.
> 
>>
>>> + * @offset: pointer to a variable holding the offset to write to
>>> + * @remain: pointer to a variable holding the remaining writing
>>> space available
>>> + *
>>> + * The function checks if there is enough space to emit a KLV with
>>> no data and
>>> + * if so writes it to memory. After the write, the offset and remain
>>> variables
>>> + * are respectively increased and decreased of the written size to
>>> be ready for
>>> + * the next emission.
>>> + */
>>> +void xe_guc_klv_enable_simple(struct xe_guc *guc, struct iosys_map
>>> *map,
>>> +                  u16 klv_id, u32 *offset, u32 *remain)
>>> +{
>>> +    u32 klv_entry = PREP_GUC_KLV(klv_id, 0);
>>     u32 klv[] = { PREP_GUC_KLV(key, 0) };
>>
>>     emit(... ARRAY_SIZE(klv)
> 
> Why? this function is specifically targeted to writing a single u32, why
> write it as a u32[1]?
> 
>>> +
>>> +    emit_klv(guc, map, &klv_entry, sizeof(klv_entry), offset, remain);
>>> +}
>>> +
>>> +/**
>>> + * xe_guc_klv_enable_one_word - Emits a KLV with 1 DW data to an
>>> iosys mapped buffer
>>> + * @guc: the guc structure
>>> + * @map: the iosys_map to write to
>>> + * @klv_id: the KLV to enable
>> s/klv_id/key
>>
>>> + * @value: the data associated with the KLV
>>> + * @offset: pointer to a variable holding the offset to write to
>>> + * @remain: pointer to a variable holding the remaining writing
>>> space available
>>> + *
>>> + * The function checks if there is enough space to emit a KLV with 1
>>> DW data and
>>> + * if so writes it to memory. After the write, the offset and remain
>>> variables
>>> + * are respectively increased and decreased of the written size to
>>> be ready for
>>> + * the next emission.
>>> + */
>>> +void xe_guc_klv_enable_one_word(struct xe_guc *guc, struct iosys_map
>>> *map,
>>> +                u16 klv_id, u32 value, u32 *offset, u32 *remain)
>>> +{
>>> +    u32 klv_entry[] = {
>>> +        PREP_GUC_KLV(klv_id, 1),
>>> +        value,
>>> +    };
>>> +
>>> +    emit_klv(guc, map, klv_entry, sizeof(klv_entry), offset, remain);
>>> +}
>>> diff --git a/drivers/gpu/drm/xe/xe_guc_klv_helpers.h b/drivers/gpu/
>>> drm/xe/xe_guc_klv_helpers.h
>>> index c676d21c173b..231f7c4b0947 100644
>>> --- a/drivers/gpu/drm/xe/xe_guc_klv_helpers.h
>>> +++ b/drivers/gpu/drm/xe/xe_guc_klv_helpers.h
>>> @@ -10,6 +10,8 @@
>>>   #include <linux/types.h>
>>>     struct drm_printer;
>>> +struct iosys_map;
>>> +struct xe_guc;
>>>     const char *xe_guc_klv_key_to_string(u16 key);
>>>   @@ -61,4 +63,9 @@ int xe_guc_klv_count(const u32 *klvs, u32
>>> num_dwords);
>>>   #define PREP_GUC_KLV_TAG(TAG) \
>>>       PREP_GUC_KLV_CONST(MAKE_GUC_KLV_KEY(TAG), MAKE_GUC_KLV_LEN(TAG))
>>>   +void xe_guc_klv_enable_simple(struct xe_guc *guc, struct iosys_map
>>> *map,
>>> +                  u16 klv_id, u32 *offset, u32 *remain);
>>> +void xe_guc_klv_enable_one_word(struct xe_guc *guc, struct iosys_map
>>> *map,
>>> +                u16 klv_id, u32 value, u32 *offset, u32 *remain);
>> hmm, "enable" does not fit here, maybe:
>>
>> xe_guc_klv_emit_empty / no_data / no_value
>> xe_guc_klv_emit_dword
> 
> emit_nodata should work.
> 
>>
>> and those should be inlines that use:
>>
>> void xe_guc_klv_emit_simple(
>>     struct xe_guc *guc,
>>     struct iosys_map *map,
>>     u16 key, u16 len, const u32 *klv,
>>     u32 *offset, u32 *remain);
>>
>> like:
>>
>> inline xe_guc_klv_emit_simple_empty(.. key ..)
>> {
>>     xe_guc_klv_emit_simple(.. key, 0, NULL, ..);
>> }
>>
>>
>> inline xe_guc_klv_emit_simple_dword(.. key, value ..)
>> {
>>     xe_guc_klv_emit_simple(.. key, 1, &value, ..);
>> }
> 
> This seems to just make things harder. xe_guc_klv_emit_simple() would
> need to handle the case where we have data differently from the case
> where we don't, potentially with 2 separate memcpys (one always done for

this isn't on the hot path so I would prefer avoid code duplication over
perf optimization

> the key and one optional for the data). I really don't see the benefit
> when the current approach still makes most of the code common in
> emit_klv() and only has separate arrays. Also, that wouldn't be a
> "simple" emission anymore since it'd handle all the possible cases, so
> the naming also wouldn't fit.

but the goal is to have helper function to 'emit KLVs', so the 'simple'
here means help in building and emit of the proper KLV-header based on
the provided key/len params, followed by emit of value (as dwords)

and while your current case is only KLV with 0 or 1 dword, nothing
prevents us from implementing generic helper, and satisfying your need
with trivial wrappers for 0/1 case

> 
> Daniele
> 
>>
>>> +
>>>   #endif
> 

Re: [PATCH 1/3] drm/xe/guc: move KLV helpers to common file

[Daniele Ceraolo Spurio]

On 2/28/2025 1:47 PM, Michal Wajdeczko wrote:
>
> On 28.02.2025 00:59, Daniele Ceraolo Spurio wrote:
>>
>> On 2/27/2025 3:29 PM, Michal Wajdeczko wrote:
>>> On 27.02.2025 02:05, Daniele Ceraolo Spurio wrote:
>>>> The GuC uses the same format for all its KLVs, so having the functions
>>>> to set them in a common file will allow us to re-use it for different
>>>> types of KLVs and not just the WAs in ADS.
>>>> The next patch will make use of these functions for a new H2G command.
>>>>
>>>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>>> Cc: John Harrison <John.C.Harrison@Intel.com>
>>>> ---
>>>>    drivers/gpu/drm/xe/xe_guc_ads.c         | 89 ++++++-------------------
>>>>    drivers/gpu/drm/xe/xe_guc_klv_helpers.c | 62 +++++++++++++++++
>>>>    drivers/gpu/drm/xe/xe_guc_klv_helpers.h |  7 ++
>>>>    3 files changed, 89 insertions(+), 69 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/xe/xe_guc_ads.c b/drivers/gpu/drm/xe/
>>>> xe_guc_ads.c
>>>> index fab259adc380..ed3304a11812 100644
>>>> --- a/drivers/gpu/drm/xe/xe_guc_ads.c
>>>> +++ b/drivers/gpu/drm/xe/xe_guc_ads.c
>>>> @@ -22,6 +22,7 @@
>>>>    #include "xe_guc.h"
>>>>    #include "xe_guc_capture.h"
>>>>    #include "xe_guc_ct.h"
>>>> +#include "xe_guc_klv_helpers.h"
>>>>    #include "xe_hw_engine.h"
>>>>    #include "xe_lrc.h"
>>>>    #include "xe_map.h"
>>>> @@ -283,56 +284,6 @@ static size_t calculate_golden_lrc_size(struct
>>>> xe_guc_ads *ads)
>>>>        return total_size;
>>>>    }
>>>>    -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) {
>>>> -        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;
>>>> -    }
>>>> -}
>>>> -
>>>> -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;
>>>> -
>>>> -    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;
>>>> -
>>>> -    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);
>>>> @@ -343,22 +294,22 @@ static void guc_waklv_init(struct xe_guc_ads *ads)
>>>>        remain = guc_ads_waklv_size(ads);
>>>>          if (XE_WA(gt, 14019882105))
>>>> -        guc_waklv_enable_simple(ads,
>>>> -
>>>> GUC_WORKAROUND_KLV_BLOCK_INTERRUPTS_WHEN_MGSR_BLOCKED,
>>>> -                    &offset, &remain);
>>>> +        xe_guc_klv_enable_simple(ads_to_guc(ads), ads_to_map(ads),
>>>> +
>>>> GUC_WORKAROUND_KLV_BLOCK_INTERRUPTS_WHEN_MGSR_BLOCKED,
>>>> +                     &offset, &remain);
>>>>        if (XE_WA(gt, 18024947630))
>>>> -        guc_waklv_enable_simple(ads,
>>>> -                    GUC_WORKAROUND_KLV_ID_GAM_PFQ_SHADOW_TAIL_POLLING,
>>>> -                    &offset, &remain);
>>>> +        xe_guc_klv_enable_simple(ads_to_guc(ads), ads_to_map(ads),
>>>> +                     GUC_WORKAROUND_KLV_ID_GAM_PFQ_SHADOW_TAIL_POLLING,
>>>> +                     &offset, &remain);
>>>>        if (XE_WA(gt, 16022287689))
>>>> -        guc_waklv_enable_simple(ads,
>>>> -
>>>> GUC_WORKAROUND_KLV_ID_DISABLE_MTP_DURING_ASYNC_COMPUTE,
>>>> -                    &offset, &remain);
>>>> +        xe_guc_klv_enable_simple(ads_to_guc(ads), ads_to_map(ads),
>>>> +
>>>> GUC_WORKAROUND_KLV_ID_DISABLE_MTP_DURING_ASYNC_COMPUTE,
>>>> +                     &offset, &remain);
>>>>          if (XE_WA(gt, 14022866841))
>>>> -        guc_waklv_enable_simple(ads,
>>>> -                    GUC_WA_KLV_WAKE_POWER_DOMAINS_FOR_OUTBOUND_MMIO,
>>>> -                    &offset, &remain);
>>>> +        xe_guc_klv_enable_simple(ads_to_guc(ads), ads_to_map(ads),
>>>> +                     GUC_WA_KLV_WAKE_POWER_DOMAINS_FOR_OUTBOUND_MMIO,
>>>> +                     &offset, &remain);
>>>>          /*
>>>>         * On RC6 exit, GuC will write register 0xB04 with the default
>>>> value provided. As of now,
>>>> @@ -366,15 +317,15 @@ static void guc_waklv_init(struct xe_guc_ads *ads)
>>>>         * 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);
>>>> +        xe_guc_klv_enable_one_word(ads_to_guc(ads), ads_to_map(ads),
>>>> +
>>>> GUC_WA_KLV_NP_RD_WRITE_TO_CLEAR_RCSM_AT_CGP_LATE_RESTORE,
>>>> +                       0xC40,
>>>> +                       &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);
>>>> +        xe_guc_klv_enable_simple(ads_to_guc(ads), ads_to_map(ads),
>>>> +
>>>> GUC_WORKAROUND_KLV_ID_BACK_TO_BACK_RCS_ENGINE_RESET,
>>>> +                     &offset, &remain);
>>>>          size = guc_ads_waklv_size(ads) - remain;
>>>>        if (!size)
>>>> diff --git a/drivers/gpu/drm/xe/xe_guc_klv_helpers.c b/drivers/gpu/
>>>> drm/xe/xe_guc_klv_helpers.c
>>>> index 146a6eda9e06..50eab2086ee3 100644
>>>> --- a/drivers/gpu/drm/xe/xe_guc_klv_helpers.c
>>>> +++ b/drivers/gpu/drm/xe/xe_guc_klv_helpers.c
>>>> @@ -7,8 +7,11 @@
>>>>    #include <drm/drm_print.h>
>>>>      #include "abi/guc_klvs_abi.h"
>>>> +#include "xe_gt_printk.h"
>>>> +#include "xe_guc.h"
>>>>    #include "xe_guc_klv_helpers.h"
>>>>    #include "xe_guc_klv_thresholds_set.h"
>>>> +#include "xe_map.h"
>>>>      #define make_u64(hi, lo) ((u64)((u64)(u32)(hi) << 32 | (u32)(lo)))
>>>>    @@ -146,3 +149,62 @@ int xe_guc_klv_count(const u32 *klvs, u32
>>>> num_dwords)
>>>>          return num_dwords ? -ENODATA : num_klvs;
>>>>    }
>>>> +
>>>> +static void emit_klv(struct xe_guc *guc, struct iosys_map *map, u32
>>>> *klv_entry,
>>>> +             u32 size, u32 *offset, u32 *remain)
>>> s/klv_entry/klv
>>>
>>> s/size/num_dwords
>>>
>>> KLVs are always u32 aligned
>>> 'size/offset/remain' all should be in dwords
>> That would mean having to convert back and forth between u8 and u32:
>>
>> - allocation is in u8 aligned
>> - params to this function would be u32 aligned
> but for generic KLV functions it doesn't make sense to pass u8 sizes
> and we should aim to define function signatures that enforce proper logic
>
>> - xe_map_memcpy_to below needs u8 alignment, so convert back
>> - the GuC expects a u8 aligned size, so need to convert remain back to u8
> that looks like a mistake done in this action definition, since all
> initial actions that introduced KLV concept (like UPDATE_VGT_POLICY,
> UPDATE_VF_CFG) were defined to operate on dwords as size

This is an ADS entry and all ADS blocks are offset/size. The new H2G 
command (in next patch) is indeed dword aligned, but it still seems 
simpler to just have a single "offset / 4" that multiple conversions 
back and forth.

>
>> Just using u8 everywhere seems simpler instead of converting back and
>> forth.
>>
>>> this will also match rest of functions in this file
>>>
>>>> +{
>>>> +    if (xe_gt_WARN(guc_to_gt(guc), *remain < size,
>>> do we need xe_gt_WARN in production?
>>> maybe xe_gt_assert will suffice?
>> I've kept this the same as what was in the original function. Not sure
>> why it was xe_gt_WARN instead of assert.
>>
>>>> +               "klv buffer too small to add klv id 0x%04x\n",
>>> s/klv/KLV
>>>
>>>> +               FIELD_GET(GUC_KLV_0_KEY, klv_entry[0])))
>>>> +        return;
>>>> +
>>>> +    xe_map_memcpy_to(guc_to_xe(guc), map, *offset, klv_entry, size);
>>>> +    *offset += size;
>>>> +    *remain -= size;
>>>> +}
>>>> +
>>>> +/**
>>>> + * xe_guc_klv_enable_simple - Emits a KLV with no data to an iosys
>>>> mapped buffer
>>>> + * @guc: the guc structure
>>>> + * @map: the iosys_map to write to
>>>> + * @klv_id: the KLV to enable
>>> key ? there is no Len nor Value
>>>
>>> this will follow other functions in this file
>> ok, will rename.
>>
>>>> + * @offset: pointer to a variable holding the offset to write to
>>>> + * @remain: pointer to a variable holding the remaining writing
>>>> space available
>>>> + *
>>>> + * The function checks if there is enough space to emit a KLV with
>>>> no data and
>>>> + * if so writes it to memory. After the write, the offset and remain
>>>> variables
>>>> + * are respectively increased and decreased of the written size to
>>>> be ready for
>>>> + * the next emission.
>>>> + */
>>>> +void xe_guc_klv_enable_simple(struct xe_guc *guc, struct iosys_map
>>>> *map,
>>>> +                  u16 klv_id, u32 *offset, u32 *remain)
>>>> +{
>>>> +    u32 klv_entry = PREP_GUC_KLV(klv_id, 0);
>>>      u32 klv[] = { PREP_GUC_KLV(key, 0) };
>>>
>>>      emit(... ARRAY_SIZE(klv)
>> Why? this function is specifically targeted to writing a single u32, why
>> write it as a u32[1]?
>>
>>>> +
>>>> +    emit_klv(guc, map, &klv_entry, sizeof(klv_entry), offset, remain);
>>>> +}
>>>> +
>>>> +/**
>>>> + * xe_guc_klv_enable_one_word - Emits a KLV with 1 DW data to an
>>>> iosys mapped buffer
>>>> + * @guc: the guc structure
>>>> + * @map: the iosys_map to write to
>>>> + * @klv_id: the KLV to enable
>>> s/klv_id/key
>>>
>>>> + * @value: the data associated with the KLV
>>>> + * @offset: pointer to a variable holding the offset to write to
>>>> + * @remain: pointer to a variable holding the remaining writing
>>>> space available
>>>> + *
>>>> + * The function checks if there is enough space to emit a KLV with 1
>>>> DW data and
>>>> + * if so writes it to memory. After the write, the offset and remain
>>>> variables
>>>> + * are respectively increased and decreased of the written size to
>>>> be ready for
>>>> + * the next emission.
>>>> + */
>>>> +void xe_guc_klv_enable_one_word(struct xe_guc *guc, struct iosys_map
>>>> *map,
>>>> +                u16 klv_id, u32 value, u32 *offset, u32 *remain)
>>>> +{
>>>> +    u32 klv_entry[] = {
>>>> +        PREP_GUC_KLV(klv_id, 1),
>>>> +        value,
>>>> +    };
>>>> +
>>>> +    emit_klv(guc, map, klv_entry, sizeof(klv_entry), offset, remain);
>>>> +}
>>>> diff --git a/drivers/gpu/drm/xe/xe_guc_klv_helpers.h b/drivers/gpu/
>>>> drm/xe/xe_guc_klv_helpers.h
>>>> index c676d21c173b..231f7c4b0947 100644
>>>> --- a/drivers/gpu/drm/xe/xe_guc_klv_helpers.h
>>>> +++ b/drivers/gpu/drm/xe/xe_guc_klv_helpers.h
>>>> @@ -10,6 +10,8 @@
>>>>    #include <linux/types.h>
>>>>      struct drm_printer;
>>>> +struct iosys_map;
>>>> +struct xe_guc;
>>>>      const char *xe_guc_klv_key_to_string(u16 key);
>>>>    @@ -61,4 +63,9 @@ int xe_guc_klv_count(const u32 *klvs, u32
>>>> num_dwords);
>>>>    #define PREP_GUC_KLV_TAG(TAG) \
>>>>        PREP_GUC_KLV_CONST(MAKE_GUC_KLV_KEY(TAG), MAKE_GUC_KLV_LEN(TAG))
>>>>    +void xe_guc_klv_enable_simple(struct xe_guc *guc, struct iosys_map
>>>> *map,
>>>> +                  u16 klv_id, u32 *offset, u32 *remain);
>>>> +void xe_guc_klv_enable_one_word(struct xe_guc *guc, struct iosys_map
>>>> *map,
>>>> +                u16 klv_id, u32 value, u32 *offset, u32 *remain);
>>> hmm, "enable" does not fit here, maybe:
>>>
>>> xe_guc_klv_emit_empty / no_data / no_value
>>> xe_guc_klv_emit_dword
>> emit_nodata should work.
>>
>>> and those should be inlines that use:
>>>
>>> void xe_guc_klv_emit_simple(
>>>      struct xe_guc *guc,
>>>      struct iosys_map *map,
>>>      u16 key, u16 len, const u32 *klv,
>>>      u32 *offset, u32 *remain);
>>>
>>> like:
>>>
>>> inline xe_guc_klv_emit_simple_empty(.. key ..)
>>> {
>>>      xe_guc_klv_emit_simple(.. key, 0, NULL, ..);
>>> }
>>>
>>>
>>> inline xe_guc_klv_emit_simple_dword(.. key, value ..)
>>> {
>>>      xe_guc_klv_emit_simple(.. key, 1, &value, ..);
>>> }
>> This seems to just make things harder. xe_guc_klv_emit_simple() would
>> need to handle the case where we have data differently from the case
>> where we don't, potentially with 2 separate memcpys (one always done for
> this isn't on the hot path so I would prefer avoid code duplication over
> perf optimization

This isn't about a perf optimization, it is about keeping the code 
simple. The only duplication is between:

u32 klv_entry = PREP_GUC_KLV(klv_id, 0);

and

u32 klv_entry[] = {
         PREP_GUC_KLV(klv_id, 1),
         value,
};

Which is minimal, but it keeps the common code much simpler as it 
doesn't have to handle optional parameters.

>
>> the key and one optional for the data). I really don't see the benefit
>> when the current approach still makes most of the code common in
>> emit_klv() and only has separate arrays. Also, that wouldn't be a
>> "simple" emission anymore since it'd handle all the possible cases, so
>> the naming also wouldn't fit.
> but the goal is to have helper function to 'emit KLVs', so the 'simple'
> here means help in building and emit of the proper KLV-header based on
> the provided key/len params, followed by emit of value (as dwords)

The "simple" here was in reference to the fact that there was no data, 
as compared against the emit_dword.

>
> and while your current case is only KLV with 0 or 1 dword, nothing
> prevents us from implementing generic helper, and satisfying your need
> with trivial wrappers for 0/1 case

I'll give it a try and see how it looks, but it really seems like we're 
over-engineering this by introducing an helper that can handle cases 
that we don't have.

Daniele

>
>> Daniele
>>
>>>> +
>>>>    #endif

Re: [PATCH 1/3] drm/xe/guc: move KLV helpers to common file

[John Harrison]

On 2/28/2025 14:21, Daniele Ceraolo Spurio wrote:
> On 2/28/2025 1:47 PM, Michal Wajdeczko wrote:
>> On 28.02.2025 00:59, Daniele Ceraolo Spurio wrote:
>>> On 2/27/2025 3:29 PM, Michal Wajdeczko wrote:
>>>> On 27.02.2025 02:05, Daniele Ceraolo Spurio wrote:
>>>>> The GuC uses the same format for all its KLVs, so having the 
>>>>> functions
>>>>> to set them in a common file will allow us to re-use it for different
>>>>> types of KLVs and not just the WAs in ADS.
>>>>> The next patch will make use of these functions for a new H2G 
>>>>> command.
>>>>>
>>>>> Signed-off-by: Daniele Ceraolo Spurio 
>>>>> <daniele.ceraolospurio@intel.com>
>>>>> Cc: John Harrison <John.C.Harrison@Intel.com>
>>>>> ---
>>>>>    drivers/gpu/drm/xe/xe_guc_ads.c         | 89 
>>>>> ++++++-------------------
>>>>>    drivers/gpu/drm/xe/xe_guc_klv_helpers.c | 62 +++++++++++++++++
>>>>>    drivers/gpu/drm/xe/xe_guc_klv_helpers.h |  7 ++
>>>>>    3 files changed, 89 insertions(+), 69 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/xe/xe_guc_ads.c b/drivers/gpu/drm/xe/
>>>>> xe_guc_ads.c
>>>>> index fab259adc380..ed3304a11812 100644
>>>>> --- a/drivers/gpu/drm/xe/xe_guc_ads.c
>>>>> +++ b/drivers/gpu/drm/xe/xe_guc_ads.c
>>>>> @@ -22,6 +22,7 @@
>>>>>    #include "xe_guc.h"
>>>>>    #include "xe_guc_capture.h"
>>>>>    #include "xe_guc_ct.h"
>>>>> +#include "xe_guc_klv_helpers.h"
>>>>>    #include "xe_hw_engine.h"
>>>>>    #include "xe_lrc.h"
>>>>>    #include "xe_map.h"
>>>>> @@ -283,56 +284,6 @@ static size_t calculate_golden_lrc_size(struct
>>>>> xe_guc_ads *ads)
>>>>>        return total_size;
>>>>>    }
>>>>>    -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) {
>>>>> -        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;
>>>>> -    }
>>>>> -}
>>>>> -
>>>>> -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;
>>>>> -
>>>>> -    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;
>>>>> -
>>>>> -    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);
>>>>> @@ -343,22 +294,22 @@ static void guc_waklv_init(struct xe_guc_ads 
>>>>> *ads)
>>>>>        remain = guc_ads_waklv_size(ads);
>>>>>          if (XE_WA(gt, 14019882105))
>>>>> -        guc_waklv_enable_simple(ads,
>>>>> -
>>>>> GUC_WORKAROUND_KLV_BLOCK_INTERRUPTS_WHEN_MGSR_BLOCKED,
>>>>> -                    &offset, &remain);
>>>>> +        xe_guc_klv_enable_simple(ads_to_guc(ads), ads_to_map(ads),
>>>>> +
>>>>> GUC_WORKAROUND_KLV_BLOCK_INTERRUPTS_WHEN_MGSR_BLOCKED,
>>>>> +                     &offset, &remain);
>>>>>        if (XE_WA(gt, 18024947630))
>>>>> -        guc_waklv_enable_simple(ads,
>>>>> - GUC_WORKAROUND_KLV_ID_GAM_PFQ_SHADOW_TAIL_POLLING,
>>>>> -                    &offset, &remain);
>>>>> +        xe_guc_klv_enable_simple(ads_to_guc(ads), ads_to_map(ads),
>>>>> + GUC_WORKAROUND_KLV_ID_GAM_PFQ_SHADOW_TAIL_POLLING,
>>>>> +                     &offset, &remain);
>>>>>        if (XE_WA(gt, 16022287689))
>>>>> -        guc_waklv_enable_simple(ads,
>>>>> -
>>>>> GUC_WORKAROUND_KLV_ID_DISABLE_MTP_DURING_ASYNC_COMPUTE,
>>>>> -                    &offset, &remain);
>>>>> +        xe_guc_klv_enable_simple(ads_to_guc(ads), ads_to_map(ads),
>>>>> +
>>>>> GUC_WORKAROUND_KLV_ID_DISABLE_MTP_DURING_ASYNC_COMPUTE,
>>>>> +                     &offset, &remain);
>>>>>          if (XE_WA(gt, 14022866841))
>>>>> -        guc_waklv_enable_simple(ads,
>>>>> - GUC_WA_KLV_WAKE_POWER_DOMAINS_FOR_OUTBOUND_MMIO,
>>>>> -                    &offset, &remain);
>>>>> +        xe_guc_klv_enable_simple(ads_to_guc(ads), ads_to_map(ads),
>>>>> + GUC_WA_KLV_WAKE_POWER_DOMAINS_FOR_OUTBOUND_MMIO,
>>>>> +                     &offset, &remain);
>>>>>          /*
>>>>>         * On RC6 exit, GuC will write register 0xB04 with the default
>>>>> value provided. As of now,
>>>>> @@ -366,15 +317,15 @@ static void guc_waklv_init(struct xe_guc_ads 
>>>>> *ads)
>>>>>         * 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);
>>>>> +        xe_guc_klv_enable_one_word(ads_to_guc(ads), ads_to_map(ads),
>>>>> +
>>>>> GUC_WA_KLV_NP_RD_WRITE_TO_CLEAR_RCSM_AT_CGP_LATE_RESTORE,
>>>>> +                       0xC40,
>>>>> +                       &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);
>>>>> +        xe_guc_klv_enable_simple(ads_to_guc(ads), ads_to_map(ads),
>>>>> +
>>>>> GUC_WORKAROUND_KLV_ID_BACK_TO_BACK_RCS_ENGINE_RESET,
>>>>> +                     &offset, &remain);
>>>>>          size = guc_ads_waklv_size(ads) - remain;
>>>>>        if (!size)
>>>>> diff --git a/drivers/gpu/drm/xe/xe_guc_klv_helpers.c b/drivers/gpu/
>>>>> drm/xe/xe_guc_klv_helpers.c
>>>>> index 146a6eda9e06..50eab2086ee3 100644
>>>>> --- a/drivers/gpu/drm/xe/xe_guc_klv_helpers.c
>>>>> +++ b/drivers/gpu/drm/xe/xe_guc_klv_helpers.c
>>>>> @@ -7,8 +7,11 @@
>>>>>    #include <drm/drm_print.h>
>>>>>      #include "abi/guc_klvs_abi.h"
>>>>> +#include "xe_gt_printk.h"
>>>>> +#include "xe_guc.h"
>>>>>    #include "xe_guc_klv_helpers.h"
>>>>>    #include "xe_guc_klv_thresholds_set.h"
>>>>> +#include "xe_map.h"
>>>>>      #define make_u64(hi, lo) ((u64)((u64)(u32)(hi) << 32 | 
>>>>> (u32)(lo)))
>>>>>    @@ -146,3 +149,62 @@ int xe_guc_klv_count(const u32 *klvs, u32
>>>>> num_dwords)
>>>>>          return num_dwords ? -ENODATA : num_klvs;
>>>>>    }
>>>>> +
>>>>> +static void emit_klv(struct xe_guc *guc, struct iosys_map *map, u32
>>>>> *klv_entry,
>>>>> +             u32 size, u32 *offset, u32 *remain)
>>>> s/klv_entry/klv
>>>>
>>>> s/size/num_dwords
>>>>
>>>> KLVs are always u32 aligned
>>>> 'size/offset/remain' all should be in dwords
>>> That would mean having to convert back and forth between u8 and u32:
>>>
>>> - allocation is in u8 aligned
>>> - params to this function would be u32 aligned
>> but for generic KLV functions it doesn't make sense to pass u8 sizes
>> and we should aim to define function signatures that enforce proper 
>> logic
>>
>>> - xe_map_memcpy_to below needs u8 alignment, so convert back
>>> - the GuC expects a u8 aligned size, so need to convert remain back 
>>> to u8
>> that looks like a mistake done in this action definition, since all
>> initial actions that introduced KLV concept (like UPDATE_VGT_POLICY,
>> UPDATE_VF_CFG) were defined to operate on dwords as size
>
> This is an ADS entry and all ADS blocks are offset/size. The new H2G 
> command (in next patch) is indeed dword aligned, but it still seems 
> simpler to just have a single "offset / 4" that multiple conversions 
> back and forth.
I agree that avoiding multiple conversions is best.

The data itself might be in 32-bit chunks but all the KMD operations are 
in bytes - allocation, memcpy, etc. So adding extra conversions seems 
unnecessary and will just make the code less readable and more error prone.


>
>>
>>> Just using u8 everywhere seems simpler instead of converting back and
>>> forth.
>>>
>>>> this will also match rest of functions in this file
>>>>
>>>>> +{
>>>>> +    if (xe_gt_WARN(guc_to_gt(guc), *remain < size,
>>>> do we need xe_gt_WARN in production?
>>>> maybe xe_gt_assert will suffice?
>>> I've kept this the same as what was in the original function. Not sure
>>> why it was xe_gt_WARN instead of assert.
>>>
>>>>> +               "klv buffer too small to add klv id 0x%04x\n",
>>>> s/klv/KLV
>>>>
>>>>> + FIELD_GET(GUC_KLV_0_KEY, klv_entry[0])))
>>>>> +        return;
>>>>> +
>>>>> +    xe_map_memcpy_to(guc_to_xe(guc), map, *offset, klv_entry, size);
>>>>> +    *offset += size;
>>>>> +    *remain -= size;
>>>>> +}
>>>>> +
>>>>> +/**
>>>>> + * xe_guc_klv_enable_simple - Emits a KLV with no data to an iosys
>>>>> mapped buffer
>>>>> + * @guc: the guc structure
>>>>> + * @map: the iosys_map to write to
>>>>> + * @klv_id: the KLV to enable
>>>> key ? there is no Len nor Value
>>>>
>>>> this will follow other functions in this file
>>> ok, will rename.
>>>
>>>>> + * @offset: pointer to a variable holding the offset to write to
>>>>> + * @remain: pointer to a variable holding the remaining writing
>>>>> space available
>>>>> + *
>>>>> + * The function checks if there is enough space to emit a KLV with
>>>>> no data and
>>>>> + * if so writes it to memory. After the write, the offset and remain
>>>>> variables
>>>>> + * are respectively increased and decreased of the written size to
>>>>> be ready for
>>>>> + * the next emission.
>>>>> + */
>>>>> +void xe_guc_klv_enable_simple(struct xe_guc *guc, struct iosys_map
>>>>> *map,
>>>>> +                  u16 klv_id, u32 *offset, u32 *remain)
>>>>> +{
>>>>> +    u32 klv_entry = PREP_GUC_KLV(klv_id, 0);
>>>>      u32 klv[] = { PREP_GUC_KLV(key, 0) };
>>>>
>>>>      emit(... ARRAY_SIZE(klv)
>>> Why? this function is specifically targeted to writing a single u32, 
>>> why
>>> write it as a u32[1]?
>>>
>>>>> +
>>>>> +    emit_klv(guc, map, &klv_entry, sizeof(klv_entry), offset, 
>>>>> remain);
>>>>> +}
>>>>> +
>>>>> +/**
>>>>> + * xe_guc_klv_enable_one_word - Emits a KLV with 1 DW data to an
>>>>> iosys mapped buffer
>>>>> + * @guc: the guc structure
>>>>> + * @map: the iosys_map to write to
>>>>> + * @klv_id: the KLV to enable
>>>> s/klv_id/key
>>>>
>>>>> + * @value: the data associated with the KLV
>>>>> + * @offset: pointer to a variable holding the offset to write to
>>>>> + * @remain: pointer to a variable holding the remaining writing
>>>>> space available
>>>>> + *
>>>>> + * The function checks if there is enough space to emit a KLV with 1
>>>>> DW data and
>>>>> + * if so writes it to memory. After the write, the offset and remain
>>>>> variables
>>>>> + * are respectively increased and decreased of the written size to
>>>>> be ready for
>>>>> + * the next emission.
>>>>> + */
>>>>> +void xe_guc_klv_enable_one_word(struct xe_guc *guc, struct iosys_map
>>>>> *map,
>>>>> +                u16 klv_id, u32 value, u32 *offset, u32 *remain)
>>>>> +{
>>>>> +    u32 klv_entry[] = {
>>>>> +        PREP_GUC_KLV(klv_id, 1),
>>>>> +        value,
>>>>> +    };
>>>>> +
>>>>> +    emit_klv(guc, map, klv_entry, sizeof(klv_entry), offset, 
>>>>> remain);
>>>>> +}
>>>>> diff --git a/drivers/gpu/drm/xe/xe_guc_klv_helpers.h b/drivers/gpu/
>>>>> drm/xe/xe_guc_klv_helpers.h
>>>>> index c676d21c173b..231f7c4b0947 100644
>>>>> --- a/drivers/gpu/drm/xe/xe_guc_klv_helpers.h
>>>>> +++ b/drivers/gpu/drm/xe/xe_guc_klv_helpers.h
>>>>> @@ -10,6 +10,8 @@
>>>>>    #include <linux/types.h>
>>>>>      struct drm_printer;
>>>>> +struct iosys_map;
>>>>> +struct xe_guc;
>>>>>      const char *xe_guc_klv_key_to_string(u16 key);
>>>>>    @@ -61,4 +63,9 @@ int xe_guc_klv_count(const u32 *klvs, u32
>>>>> num_dwords);
>>>>>    #define PREP_GUC_KLV_TAG(TAG) \
>>>>>        PREP_GUC_KLV_CONST(MAKE_GUC_KLV_KEY(TAG), 
>>>>> MAKE_GUC_KLV_LEN(TAG))
>>>>>    +void xe_guc_klv_enable_simple(struct xe_guc *guc, struct 
>>>>> iosys_map
>>>>> *map,
>>>>> +                  u16 klv_id, u32 *offset, u32 *remain);
>>>>> +void xe_guc_klv_enable_one_word(struct xe_guc *guc, struct iosys_map
>>>>> *map,
>>>>> +                u16 klv_id, u32 value, u32 *offset, u32 *remain);
>>>> hmm, "enable" does not fit here, maybe:
>>>>
>>>> xe_guc_klv_emit_empty / no_data / no_value
>>>> xe_guc_klv_emit_dword
>>> emit_nodata should work.
>>>
>>>> and those should be inlines that use:
>>>>
>>>> void xe_guc_klv_emit_simple(
>>>>      struct xe_guc *guc,
>>>>      struct iosys_map *map,
>>>>      u16 key, u16 len, const u32 *klv,
>>>>      u32 *offset, u32 *remain);
>>>>
>>>> like:
>>>>
>>>> inline xe_guc_klv_emit_simple_empty(.. key ..)
>>>> {
>>>>      xe_guc_klv_emit_simple(.. key, 0, NULL, ..);
>>>> }
>>>>
>>>>
>>>> inline xe_guc_klv_emit_simple_dword(.. key, value ..)
>>>> {
>>>>      xe_guc_klv_emit_simple(.. key, 1, &value, ..);
>>>> }
>>> This seems to just make things harder. xe_guc_klv_emit_simple() would
>>> need to handle the case where we have data differently from the case
>>> where we don't, potentially with 2 separate memcpys (one always done 
>>> for
>> this isn't on the hot path so I would prefer avoid code duplication over
>> perf optimization
>
> This isn't about a perf optimization, it is about keeping the code 
> simple. The only duplication is between:
>
> u32 klv_entry = PREP_GUC_KLV(klv_id, 0);
>
> and
>
> u32 klv_entry[] = {
>         PREP_GUC_KLV(klv_id, 1),
>         value,
> };
>
> Which is minimal, but it keeps the common code much simpler as it 
> doesn't have to handle optional parameters.
>
>>
>>> the key and one optional for the data). I really don't see the benefit
>>> when the current approach still makes most of the code common in
>>> emit_klv() and only has separate arrays. Also, that wouldn't be a
>>> "simple" emission anymore since it'd handle all the possible cases, so
>>> the naming also wouldn't fit.
>> but the goal is to have helper function to 'emit KLVs', so the 'simple'
>> here means help in building and emit of the proper KLV-header based on
>> the provided key/len params, followed by emit of value (as dwords)
>
> The "simple" here was in reference to the fact that there was no data, 
> as compared against the emit_dword.
>
>>
>> and while your current case is only KLV with 0 or 1 dword, nothing
>> prevents us from implementing generic helper, and satisfying your need
>> with trivial wrappers for 0/1 case
>
> I'll give it a try and see how it looks, but it really seems like 
> we're over-engineering this by introducing an helper that can handle 
> cases that we don't have.
I agree with Daniele. There is no need to overcomplicate things to 
support use cases that don't exist.

And yes, the 'simple' meant no data. So calling a function 
'simple_dword' is broken and 'simple_empty' is redundant! We could maybe 
drop the 'enable' part if the helpers might be used for other KLV 
purposes at some point. But it would just be 'xe_guc_klv_add_simple' and 
'xe_guc_klv_add_one_word'. If you really thing the use of 'simple' is 
ambiguous or confusing then maybe 'xe_guc_klv_add_empty' instead but 
that sounds odd to me.

John.

>
> Daniele
>
>>
>>> Daniele
>>>
>>>>> +
>>>>>    #endif
>

Re: [PATCH 1/3] drm/xe/guc: move KLV helpers to common file

[Michal Wajdeczko]

On 01.03.2025 02:55, John Harrison wrote:
> On 2/28/2025 14:21, Daniele Ceraolo Spurio wrote:
>> On 2/28/2025 1:47 PM, Michal Wajdeczko wrote:
>>> On 28.02.2025 00:59, Daniele Ceraolo Spurio wrote:
>>>> On 2/27/2025 3:29 PM, Michal Wajdeczko wrote:
>>>>> On 27.02.2025 02:05, Daniele Ceraolo Spurio wrote:
>>>>>> The GuC uses the same format for all its KLVs, so having the
>>>>>> functions
>>>>>> to set them in a common file will allow us to re-use it for different
>>>>>> types of KLVs and not just the WAs in ADS.
>>>>>> The next patch will make use of these functions for a new H2G
>>>>>> command.
>>>>>>
>>>>>> Signed-off-by: Daniele Ceraolo Spurio
>>>>>> <daniele.ceraolospurio@intel.com>
>>>>>> Cc: John Harrison <John.C.Harrison@Intel.com>
>>>>>> ---
>>>>>>    drivers/gpu/drm/xe/xe_guc_ads.c         | 89 +++++
>>>>>> +-------------------
>>>>>>    drivers/gpu/drm/xe/xe_guc_klv_helpers.c | 62 +++++++++++++++++
>>>>>>    drivers/gpu/drm/xe/xe_guc_klv_helpers.h |  7 ++
>>>>>>    3 files changed, 89 insertions(+), 69 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/xe/xe_guc_ads.c b/drivers/gpu/drm/xe/
>>>>>> xe_guc_ads.c
>>>>>> index fab259adc380..ed3304a11812 100644
>>>>>> --- a/drivers/gpu/drm/xe/xe_guc_ads.c
>>>>>> +++ b/drivers/gpu/drm/xe/xe_guc_ads.c
>>>>>> @@ -22,6 +22,7 @@
>>>>>>    #include "xe_guc.h"
>>>>>>    #include "xe_guc_capture.h"
>>>>>>    #include "xe_guc_ct.h"
>>>>>> +#include "xe_guc_klv_helpers.h"
>>>>>>    #include "xe_hw_engine.h"
>>>>>>    #include "xe_lrc.h"
>>>>>>    #include "xe_map.h"
>>>>>> @@ -283,56 +284,6 @@ static size_t calculate_golden_lrc_size(struct
>>>>>> xe_guc_ads *ads)
>>>>>>        return total_size;
>>>>>>    }
>>>>>>    -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) {
>>>>>> -        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;
>>>>>> -    }
>>>>>> -}
>>>>>> -
>>>>>> -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;
>>>>>> -
>>>>>> -    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;
>>>>>> -
>>>>>> -    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);
>>>>>> @@ -343,22 +294,22 @@ static void guc_waklv_init(struct xe_guc_ads
>>>>>> *ads)
>>>>>>        remain = guc_ads_waklv_size(ads);
>>>>>>          if (XE_WA(gt, 14019882105))
>>>>>> -        guc_waklv_enable_simple(ads,
>>>>>> -
>>>>>> GUC_WORKAROUND_KLV_BLOCK_INTERRUPTS_WHEN_MGSR_BLOCKED,
>>>>>> -                    &offset, &remain);
>>>>>> +        xe_guc_klv_enable_simple(ads_to_guc(ads), ads_to_map(ads),
>>>>>> +
>>>>>> GUC_WORKAROUND_KLV_BLOCK_INTERRUPTS_WHEN_MGSR_BLOCKED,
>>>>>> +                     &offset, &remain);
>>>>>>        if (XE_WA(gt, 18024947630))
>>>>>> -        guc_waklv_enable_simple(ads,
>>>>>> - GUC_WORKAROUND_KLV_ID_GAM_PFQ_SHADOW_TAIL_POLLING,
>>>>>> -                    &offset, &remain);
>>>>>> +        xe_guc_klv_enable_simple(ads_to_guc(ads), ads_to_map(ads),
>>>>>> + GUC_WORKAROUND_KLV_ID_GAM_PFQ_SHADOW_TAIL_POLLING,
>>>>>> +                     &offset, &remain);
>>>>>>        if (XE_WA(gt, 16022287689))
>>>>>> -        guc_waklv_enable_simple(ads,
>>>>>> -
>>>>>> GUC_WORKAROUND_KLV_ID_DISABLE_MTP_DURING_ASYNC_COMPUTE,
>>>>>> -                    &offset, &remain);
>>>>>> +        xe_guc_klv_enable_simple(ads_to_guc(ads), ads_to_map(ads),
>>>>>> +
>>>>>> GUC_WORKAROUND_KLV_ID_DISABLE_MTP_DURING_ASYNC_COMPUTE,
>>>>>> +                     &offset, &remain);
>>>>>>          if (XE_WA(gt, 14022866841))
>>>>>> -        guc_waklv_enable_simple(ads,
>>>>>> - GUC_WA_KLV_WAKE_POWER_DOMAINS_FOR_OUTBOUND_MMIO,
>>>>>> -                    &offset, &remain);
>>>>>> +        xe_guc_klv_enable_simple(ads_to_guc(ads), ads_to_map(ads),
>>>>>> + GUC_WA_KLV_WAKE_POWER_DOMAINS_FOR_OUTBOUND_MMIO,
>>>>>> +                     &offset, &remain);
>>>>>>          /*
>>>>>>         * On RC6 exit, GuC will write register 0xB04 with the default
>>>>>> value provided. As of now,
>>>>>> @@ -366,15 +317,15 @@ static void guc_waklv_init(struct xe_guc_ads
>>>>>> *ads)
>>>>>>         * 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);
>>>>>> +        xe_guc_klv_enable_one_word(ads_to_guc(ads), ads_to_map(ads),
>>>>>> +
>>>>>> GUC_WA_KLV_NP_RD_WRITE_TO_CLEAR_RCSM_AT_CGP_LATE_RESTORE,
>>>>>> +                       0xC40,
>>>>>> +                       &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);
>>>>>> +        xe_guc_klv_enable_simple(ads_to_guc(ads), ads_to_map(ads),
>>>>>> +
>>>>>> GUC_WORKAROUND_KLV_ID_BACK_TO_BACK_RCS_ENGINE_RESET,
>>>>>> +                     &offset, &remain);
>>>>>>          size = guc_ads_waklv_size(ads) - remain;
>>>>>>        if (!size)
>>>>>> diff --git a/drivers/gpu/drm/xe/xe_guc_klv_helpers.c b/drivers/gpu/
>>>>>> drm/xe/xe_guc_klv_helpers.c
>>>>>> index 146a6eda9e06..50eab2086ee3 100644
>>>>>> --- a/drivers/gpu/drm/xe/xe_guc_klv_helpers.c
>>>>>> +++ b/drivers/gpu/drm/xe/xe_guc_klv_helpers.c
>>>>>> @@ -7,8 +7,11 @@
>>>>>>    #include <drm/drm_print.h>
>>>>>>      #include "abi/guc_klvs_abi.h"
>>>>>> +#include "xe_gt_printk.h"
>>>>>> +#include "xe_guc.h"
>>>>>>    #include "xe_guc_klv_helpers.h"
>>>>>>    #include "xe_guc_klv_thresholds_set.h"
>>>>>> +#include "xe_map.h"
>>>>>>      #define make_u64(hi, lo) ((u64)((u64)(u32)(hi) << 32 | (u32)
>>>>>> (lo)))
>>>>>>    @@ -146,3 +149,62 @@ int xe_guc_klv_count(const u32 *klvs, u32
>>>>>> num_dwords)
>>>>>>          return num_dwords ? -ENODATA : num_klvs;
>>>>>>    }
>>>>>> +
>>>>>> +static void emit_klv(struct xe_guc *guc, struct iosys_map *map, u32
>>>>>> *klv_entry,
>>>>>> +             u32 size, u32 *offset, u32 *remain)
>>>>> s/klv_entry/klv
>>>>>
>>>>> s/size/num_dwords
>>>>>
>>>>> KLVs are always u32 aligned
>>>>> 'size/offset/remain' all should be in dwords
>>>> That would mean having to convert back and forth between u8 and u32:
>>>>
>>>> - allocation is in u8 aligned
>>>> - params to this function would be u32 aligned
>>> but for generic KLV functions it doesn't make sense to pass u8 sizes
>>> and we should aim to define function signatures that enforce proper
>>> logic
>>>
>>>> - xe_map_memcpy_to below needs u8 alignment, so convert back
>>>> - the GuC expects a u8 aligned size, so need to convert remain back
>>>> to u8
>>> that looks like a mistake done in this action definition, since all
>>> initial actions that introduced KLV concept (like UPDATE_VGT_POLICY,
>>> UPDATE_VF_CFG) were defined to operate on dwords as size
>>
>> This is an ADS entry and all ADS blocks are offset/size. The new H2G
>> command (in next patch) is indeed dword aligned, but it still seems
>> simpler to just have a single "offset / 4" that multiple conversions
>> back and forth.
> I agree that avoiding multiple conversions is best.
> 
> The data itself might be in 32-bit chunks 

in KLV world data *must* be in 32-bit chunks

> but all the KMD operations are
> in bytes - allocation, memcpy, etc. So adding extra conversions seems
> unnecessary and will just make the code less readable and more error prone.

if you allow byte sizes you will still need to handle invalid use cases
with unaligned sizes and any conversion should be mostly in the helper
layer, that, by definition, should hide any implementation details from
exposed logical data view (here 32-bit based KLV)

> 
> 
>>
>>>
>>>> Just using u8 everywhere seems simpler instead of converting back and
>>>> forth.
>>>>
>>>>> this will also match rest of functions in this file
>>>>>
>>>>>> +{
>>>>>> +    if (xe_gt_WARN(guc_to_gt(guc), *remain < size,
>>>>> do we need xe_gt_WARN in production?
>>>>> maybe xe_gt_assert will suffice?
>>>> I've kept this the same as what was in the original function. Not sure
>>>> why it was xe_gt_WARN instead of assert.
>>>>
>>>>>> +               "klv buffer too small to add klv id 0x%04x\n",
>>>>> s/klv/KLV
>>>>>
>>>>>> + FIELD_GET(GUC_KLV_0_KEY, klv_entry[0])))
>>>>>> +        return;
>>>>>> +
>>>>>> +    xe_map_memcpy_to(guc_to_xe(guc), map, *offset, klv_entry, size);
>>>>>> +    *offset += size;
>>>>>> +    *remain -= size;
>>>>>> +}
>>>>>> +
>>>>>> +/**
>>>>>> + * xe_guc_klv_enable_simple - Emits a KLV with no data to an iosys
>>>>>> mapped buffer
>>>>>> + * @guc: the guc structure
>>>>>> + * @map: the iosys_map to write to
>>>>>> + * @klv_id: the KLV to enable
>>>>> key ? there is no Len nor Value
>>>>>
>>>>> this will follow other functions in this file
>>>> ok, will rename.
>>>>
>>>>>> + * @offset: pointer to a variable holding the offset to write to
>>>>>> + * @remain: pointer to a variable holding the remaining writing
>>>>>> space available
>>>>>> + *
>>>>>> + * The function checks if there is enough space to emit a KLV with
>>>>>> no data and
>>>>>> + * if so writes it to memory. After the write, the offset and remain
>>>>>> variables
>>>>>> + * are respectively increased and decreased of the written size to
>>>>>> be ready for
>>>>>> + * the next emission.
>>>>>> + */
>>>>>> +void xe_guc_klv_enable_simple(struct xe_guc *guc, struct iosys_map
>>>>>> *map,
>>>>>> +                  u16 klv_id, u32 *offset, u32 *remain)
>>>>>> +{
>>>>>> +    u32 klv_entry = PREP_GUC_KLV(klv_id, 0);
>>>>>      u32 klv[] = { PREP_GUC_KLV(key, 0) };
>>>>>
>>>>>      emit(... ARRAY_SIZE(klv)
>>>> Why? this function is specifically targeted to writing a single u32,
>>>> why
>>>> write it as a u32[1]?
>>>>
>>>>>> +
>>>>>> +    emit_klv(guc, map, &klv_entry, sizeof(klv_entry), offset,
>>>>>> remain);
>>>>>> +}
>>>>>> +
>>>>>> +/**
>>>>>> + * xe_guc_klv_enable_one_word - Emits a KLV with 1 DW data to an
>>>>>> iosys mapped buffer
>>>>>> + * @guc: the guc structure
>>>>>> + * @map: the iosys_map to write to
>>>>>> + * @klv_id: the KLV to enable
>>>>> s/klv_id/key
>>>>>
>>>>>> + * @value: the data associated with the KLV
>>>>>> + * @offset: pointer to a variable holding the offset to write to
>>>>>> + * @remain: pointer to a variable holding the remaining writing
>>>>>> space available
>>>>>> + *
>>>>>> + * The function checks if there is enough space to emit a KLV with 1
>>>>>> DW data and
>>>>>> + * if so writes it to memory. After the write, the offset and remain
>>>>>> variables
>>>>>> + * are respectively increased and decreased of the written size to
>>>>>> be ready for
>>>>>> + * the next emission.
>>>>>> + */
>>>>>> +void xe_guc_klv_enable_one_word(struct xe_guc *guc, struct iosys_map
>>>>>> *map,
>>>>>> +                u16 klv_id, u32 value, u32 *offset, u32 *remain)
>>>>>> +{
>>>>>> +    u32 klv_entry[] = {
>>>>>> +        PREP_GUC_KLV(klv_id, 1),
>>>>>> +        value,
>>>>>> +    };
>>>>>> +
>>>>>> +    emit_klv(guc, map, klv_entry, sizeof(klv_entry), offset,
>>>>>> remain);
>>>>>> +}
>>>>>> diff --git a/drivers/gpu/drm/xe/xe_guc_klv_helpers.h b/drivers/gpu/
>>>>>> drm/xe/xe_guc_klv_helpers.h
>>>>>> index c676d21c173b..231f7c4b0947 100644
>>>>>> --- a/drivers/gpu/drm/xe/xe_guc_klv_helpers.h
>>>>>> +++ b/drivers/gpu/drm/xe/xe_guc_klv_helpers.h
>>>>>> @@ -10,6 +10,8 @@
>>>>>>    #include <linux/types.h>
>>>>>>      struct drm_printer;
>>>>>> +struct iosys_map;
>>>>>> +struct xe_guc;
>>>>>>      const char *xe_guc_klv_key_to_string(u16 key);
>>>>>>    @@ -61,4 +63,9 @@ int xe_guc_klv_count(const u32 *klvs, u32
>>>>>> num_dwords);
>>>>>>    #define PREP_GUC_KLV_TAG(TAG) \
>>>>>>        PREP_GUC_KLV_CONST(MAKE_GUC_KLV_KEY(TAG),
>>>>>> MAKE_GUC_KLV_LEN(TAG))
>>>>>>    +void xe_guc_klv_enable_simple(struct xe_guc *guc, struct
>>>>>> iosys_map
>>>>>> *map,
>>>>>> +                  u16 klv_id, u32 *offset, u32 *remain);
>>>>>> +void xe_guc_klv_enable_one_word(struct xe_guc *guc, struct iosys_map
>>>>>> *map,
>>>>>> +                u16 klv_id, u32 value, u32 *offset, u32 *remain);
>>>>> hmm, "enable" does not fit here, maybe:
>>>>>
>>>>> xe_guc_klv_emit_empty / no_data / no_value
>>>>> xe_guc_klv_emit_dword
>>>> emit_nodata should work.
>>>>
>>>>> and those should be inlines that use:
>>>>>
>>>>> void xe_guc_klv_emit_simple(
>>>>>      struct xe_guc *guc,
>>>>>      struct iosys_map *map,
>>>>>      u16 key, u16 len, const u32 *klv,
>>>>>      u32 *offset, u32 *remain);
>>>>>
>>>>> like:
>>>>>
>>>>> inline xe_guc_klv_emit_simple_empty(.. key ..)
>>>>> {
>>>>>      xe_guc_klv_emit_simple(.. key, 0, NULL, ..);
>>>>> }
>>>>>
>>>>>
>>>>> inline xe_guc_klv_emit_simple_dword(.. key, value ..)
>>>>> {
>>>>>      xe_guc_klv_emit_simple(.. key, 1, &value, ..);
>>>>> }
>>>> This seems to just make things harder. xe_guc_klv_emit_simple() would
>>>> need to handle the case where we have data differently from the case
>>>> where we don't, potentially with 2 separate memcpys (one always done
>>>> for
>>> this isn't on the hot path so I would prefer avoid code duplication over
>>> perf optimization
>>
>> This isn't about a perf optimization, it is about keeping the code
>> simple. The only duplication is between:
>>
>> u32 klv_entry = PREP_GUC_KLV(klv_id, 0);
>>
>> and
>>
>> u32 klv_entry[] = {
>>         PREP_GUC_KLV(klv_id, 1),
>>         value,
>> };
>>
>> Which is minimal, but it keeps the common code much simpler as it
>> doesn't have to handle optional parameters.
>>
>>>
>>>> the key and one optional for the data). I really don't see the benefit
>>>> when the current approach still makes most of the code common in
>>>> emit_klv() and only has separate arrays. Also, that wouldn't be a
>>>> "simple" emission anymore since it'd handle all the possible cases, so
>>>> the naming also wouldn't fit.
>>> but the goal is to have helper function to 'emit KLVs', so the 'simple'
>>> here means help in building and emit of the proper KLV-header based on
>>> the provided key/len params, followed by emit of value (as dwords)
>>
>> The "simple" here was in reference to the fact that there was no data,
>> as compared against the emit_dword.
>>
>>>
>>> and while your current case is only KLV with 0 or 1 dword, nothing
>>> prevents us from implementing generic helper, and satisfying your need
>>> with trivial wrappers for 0/1 case
>>
>> I'll give it a try and see how it looks, but it really seems like
>> we're over-engineering this by introducing an helper that can handle
>> cases that we don't have.
> I agree with Daniele. There is no need to overcomplicate things to
> support use cases that don't exist.
> 
> And yes, the 'simple' meant no data. So calling a function

hmm, not sure which dictionary says that, quoting the web:

adjective
1. easily understood or done; presenting no difficulty.
"a simple solution"

> 'simple_dword' is broken and 'simple_empty' is redundant! We could maybe
> drop the 'enable' part if the helpers might be used for other KLV
> purposes at some point. But it would just be 'xe_guc_klv_add_simple' and
> 'xe_guc_klv_add_one_word'. If you really thing the use of 'simple' is
> ambiguous or confusing then maybe 'xe_guc_klv_add_empty' instead but
> that sounds odd to me.

but to be clear: using KLVs without the VALUE is odd and naming that
exceptional case as 'simple' is IMO just wrong

anyway, the idea was to have single "generic" function that can emit
KLVs with data of any length, plus some wrappers for "trivial" cases:

inline xe_guc_klv_emit_empty(.. key ..)
{
     xe_guc_klv_emit(.. key, 0, NULL, ..);
}

inline xe_guc_klv_emit_u32(.. key, value ..)
{
    xe_guc_klv_emit(.. key, 1, &value, ..);
}

inline xe_guc_klv_emit_u64(.. key, value ..)
{
    xe_guc_klv_emit(.. key, 2, &value, ..);
}

note that for upcoming VF save/restore feature we will likely have to
emit more complex KLVs, so having a "generic" emit function could be an
immediate bonus

> 
> John.
> 
>>
>> Daniele
>>
>>>
>>>> Daniele
>>>>
>>>>>> +
>>>>>>    #endif
>>
> 

[PATCH 2/3] drm/xe/guc: Enable extended CAT error reporting

[Daniele Ceraolo Spurio]

On newer HW (Xe2 onwards + PVC) it is possible to get extra information
when a CAT error occurs, specifically a dword reporting the error type.
To enable this extra reporting, we need to opt-in with the GuC, which is
done via a specific per-VF feature opt-in H2G.

On platforms where the HW does not support the extra reporting, the GuC
will set the type to 0xdeadbeef, so we can keep the code simple and
opt-in to the feature on every platform and then just discard the data
if it is invalid.

Note that on native/PF we're guaranteed that the opt in is available
because we don't support any GuC old enough to not have it, but if we're
a VF we might be running on a non-XE PF with an older GuC, so we need to
handle that case. We can re-use the invalid type above to handle this
scenario the same way as if the feature was not supported in HW.

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Nirmoy Das <nirmoy.das@intel.com>
Cc: John Harrison <John.C.Harrison@Intel.com>
---
 drivers/gpu/drm/xe/abi/guc_actions_abi.h |  4 ++
 drivers/gpu/drm/xe/abi/guc_klvs_abi.h    | 15 +++++
 drivers/gpu/drm/xe/xe_guc.c              | 83 ++++++++++++++++++++++++
 drivers/gpu/drm/xe/xe_guc.h              |  1 +
 drivers/gpu/drm/xe/xe_guc_submit.c       | 16 ++++-
 drivers/gpu/drm/xe/xe_guc_types.h        |  3 +
 drivers/gpu/drm/xe/xe_uc.c               |  4 ++
 7 files changed, 123 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/xe/abi/guc_actions_abi.h b/drivers/gpu/drm/xe/abi/guc_actions_abi.h
index ec516e838ee8..fc632c738012 100644
--- a/drivers/gpu/drm/xe/abi/guc_actions_abi.h
+++ b/drivers/gpu/drm/xe/abi/guc_actions_abi.h
@@ -141,6 +141,7 @@ enum xe_guc_action {
 	XE_GUC_ACTION_CLIENT_SOFT_RESET = 0x5507,
 	XE_GUC_ACTION_SET_ENG_UTIL_BUFF = 0x550A,
 	XE_GUC_ACTION_SET_DEVICE_ENGINE_ACTIVITY_BUFFER = 0x550C,
+	XE_GUC_ACTION_OPT_IN_FEATURE_KLV = 0x550E,
 	XE_GUC_ACTION_NOTIFY_MEMORY_CAT_ERROR = 0x6000,
 	XE_GUC_ACTION_REPORT_PAGE_FAULT_REQ_DESC = 0x6002,
 	XE_GUC_ACTION_PAGE_FAULT_RES_DESC = 0x6003,
@@ -239,4 +240,7 @@ enum xe_guc_g2g_type {
 #define XE_G2G_DEREGISTER_TILE	REG_GENMASK(15, 12)
 #define XE_G2G_DEREGISTER_TYPE	REG_GENMASK(11, 8)
 
+/* invalid type for XE_GUC_ACTION_NOTIFY_MEMORY_CAT_ERROR */
+#define XE_GUC_CAT_ERR_TYPE_INVALID 0xdeadbeef
+
 #endif
diff --git a/drivers/gpu/drm/xe/abi/guc_klvs_abi.h b/drivers/gpu/drm/xe/abi/guc_klvs_abi.h
index d633f1c739e4..4c2b5bfde8fe 100644
--- a/drivers/gpu/drm/xe/abi/guc_klvs_abi.h
+++ b/drivers/gpu/drm/xe/abi/guc_klvs_abi.h
@@ -16,6 +16,7 @@
  *  +===+=======+==============================================================+
  *  | 0 | 31:16 | **KEY** - KLV key identifier                                 |
  *  |   |       |   - `GuC Self Config KLVs`_                                  |
+ *  |   |       |   - `GuC Opt In Feature KLVs`_                               |
  *  |   |       |   - `GuC VGT Policy KLVs`_                                   |
  *  |   |       |   - `GuC VF Configuration KLVs`_                             |
  *  |   |       |                                                              |
@@ -124,6 +125,20 @@ enum  {
 	GUC_CONTEXT_POLICIES_KLV_NUM_IDS = 5,
 };
 
+/**
+ * DOC: GuC Opt In Feature KLVs
+ *
+ * `GuC KLV`_ keys available for use with OPT_IN_FEATURE_KLV
+ *
+ *  _`GUC_KLV_GUC_OPT_IN_FEATURE_EXT_CAT_ERR_TYPE` : 0x4001
+ *      Adds an extra dword to the XE_GUC_ACTION_NOTIFY_MEMORY_CAT_ERROR G2H
+ *      containing the type of the CAT error. On HW that does not support
+ *      reporting the CAT error type, the extra dword is set to 0xdeadbeef.
+ */
+
+#define GUC_KLV_GUC_OPT_IN_FEATURE_EXT_CAT_ERR_TYPE_KEY 0x4001
+#define GUC_KLV_GUC_OPT_IN_FEATURE_EXT_CAT_ERR_TYPE_LEN 0u
+
 /**
  * DOC: GuC VGT Policy KLVs
  *
diff --git a/drivers/gpu/drm/xe/xe_guc.c b/drivers/gpu/drm/xe/xe_guc.c
index 7b38447d902c..7483aba713b7 100644
--- a/drivers/gpu/drm/xe/xe_guc.c
+++ b/drivers/gpu/drm/xe/xe_guc.c
@@ -29,6 +29,7 @@
 #include "xe_guc_db_mgr.h"
 #include "xe_guc_engine_activity.h"
 #include "xe_guc_hwconfig.h"
+#include "xe_guc_klv_helpers.h"
 #include "xe_guc_log.h"
 #include "xe_guc_pc.h"
 #include "xe_guc_relay.h"
@@ -569,6 +570,76 @@ static int guc_g2g_start(struct xe_guc *guc)
 	return err;
 }
 
+static int guc_opt_in_features_init(struct xe_guc *guc)
+{
+	struct xe_bo *bo;
+	struct xe_gt *gt = guc_to_gt(guc);
+	struct xe_tile *tile = gt_to_tile(gt);
+	struct xe_device *xe = gt_to_xe(gt);
+
+	/* opt-in KLVs are all 1 DW so far, so a page is more than enough */
+	bo = xe_managed_bo_create_pin_map(xe, tile, XE_PAGE_SIZE,
+					  XE_BO_FLAG_SYSTEM |
+					  XE_BO_FLAG_GGTT |
+					  XE_BO_FLAG_GGTT_INVALIDATE);
+	if (IS_ERR(bo)) {
+		xe_gt_err(gt, "failed to allocate bo for GUC opt-in features\n");
+		return PTR_ERR(bo);
+	}
+
+	guc->opt_in_bo = bo;
+
+	return 0;
+}
+
+static int __guc_opt_in_features_enable(struct xe_guc *guc, u64 addr, u32 num_dwords)
+{
+	u32 action[] = {
+		XE_GUC_ACTION_OPT_IN_FEATURE_KLV,
+		lower_32_bits(addr),
+		upper_32_bits(addr),
+		num_dwords
+	};
+
+	return xe_guc_ct_send_block(&guc->ct, action, ARRAY_SIZE(action));
+}
+
+int xe_guc_opt_in_features_enable(struct xe_guc *guc)
+{
+	struct xe_bo *bo = guc->opt_in_bo;
+	struct xe_uc_fw_version *compat = &guc->fw.versions.found[XE_UC_FW_VER_COMPATIBILITY];
+	u32 remain = bo->size;
+	u32 offset = 0;
+	int ret;
+
+	/*
+	 * This opt-in was added in 70.17.0, so it's always available for
+	 * native because we don't allow load of any firmware older than
+	 * 70.29.2. However, with SRIOV we might be a VF running on a non-Xe PF
+	 * with an older GuC release, so we need to check that. GuC 70.17.0 maps
+	 * to compatibility version 1.7.0.
+	 * Note that the GuC allows enabling this KLV even on platforms that do
+	 * not support the extra type; in such case the returned type variable
+	 * will be set to a known invalid value which we can check against.
+	 */
+	if (MAKE_GUC_VER_STRUCT(*compat) >= MAKE_GUC_VER(1, 7, 0))
+		xe_guc_klv_enable_simple(guc, &bo->vmap,
+					 GUC_KLV_GUC_OPT_IN_FEATURE_EXT_CAT_ERR_TYPE_KEY,
+					 &offset, &remain);
+
+	if (offset) {
+		ret = __guc_opt_in_features_enable(guc, xe_bo_ggtt_addr(bo), offset / 4);
+		if (ret < 0) {
+			xe_gt_err(guc_to_gt(guc),
+				  "failed to enable GuC opt-in features: %pe\n",
+				  ERR_PTR(ret));
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
 static void guc_fini_hw(void *arg)
 {
 	struct xe_guc *guc = arg;
@@ -640,6 +711,10 @@ static int vf_guc_init(struct xe_guc *guc)
 	if (err)
 		return err;
 
+	err = guc_opt_in_features_init(guc);
+	if (err)
+		return err;
+
 	return 0;
 }
 
@@ -684,6 +759,10 @@ int xe_guc_init(struct xe_guc *guc)
 	if (ret)
 		goto out;
 
+	ret = guc_opt_in_features_init(guc);
+	if (ret)
+		goto out;
+
 	xe_uc_fw_change_status(&guc->fw, XE_UC_FIRMWARE_LOADABLE);
 
 	ret = devm_add_action_or_reset(xe->drm.dev, guc_fini_hw, guc);
@@ -769,6 +848,10 @@ int xe_guc_post_load_init(struct xe_guc *guc)
 
 	xe_guc_ads_populate_post_load(&guc->ads);
 
+	ret = xe_guc_opt_in_features_enable(guc);
+	if (ret)
+		return ret;
+
 	if (xe_guc_g2g_wanted(guc_to_xe(guc))) {
 		ret = guc_g2g_start(guc);
 		if (ret)
diff --git a/drivers/gpu/drm/xe/xe_guc.h b/drivers/gpu/drm/xe/xe_guc.h
index 58338be44558..4a66575f017d 100644
--- a/drivers/gpu/drm/xe/xe_guc.h
+++ b/drivers/gpu/drm/xe/xe_guc.h
@@ -33,6 +33,7 @@ int xe_guc_reset(struct xe_guc *guc);
 int xe_guc_upload(struct xe_guc *guc);
 int xe_guc_min_load_for_hwconfig(struct xe_guc *guc);
 int xe_guc_enable_communication(struct xe_guc *guc);
+int xe_guc_opt_in_features_enable(struct xe_guc *guc);
 int xe_guc_suspend(struct xe_guc *guc);
 void xe_guc_notify(struct xe_guc *guc);
 int xe_guc_auth_huc(struct xe_guc *guc, u32 rsa_addr);
diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
index 913c74d6e2ae..e53653cef66a 100644
--- a/drivers/gpu/drm/xe/xe_guc_submit.c
+++ b/drivers/gpu/drm/xe/xe_guc_submit.c
@@ -2035,12 +2035,16 @@ int xe_guc_exec_queue_memory_cat_error_handler(struct xe_guc *guc, u32 *msg,
 	struct xe_gt *gt = guc_to_gt(guc);
 	struct xe_exec_queue *q;
 	u32 guc_id;
+	u32 type = XE_GUC_CAT_ERR_TYPE_INVALID;
 
-	if (unlikely(len < 1))
+	if (unlikely(!len || len > 2))
 		return -EPROTO;
 
 	guc_id = msg[0];
 
+	if (len == 2)
+		type = msg[1];
+
 	if (guc_id == GUC_ID_UNKNOWN) {
 		/*
 		 * GuC uses GUC_ID_UNKNOWN if it can not map the CAT fault to any PF/VF
@@ -2054,8 +2058,14 @@ int xe_guc_exec_queue_memory_cat_error_handler(struct xe_guc *guc, u32 *msg,
 	if (unlikely(!q))
 		return -EPROTO;
 
-	xe_gt_dbg(gt, "Engine memory cat error: engine_class=%s, logical_mask: 0x%x, guc_id=%d",
-		  xe_hw_engine_class_to_str(q->class), q->logical_mask, guc_id);
+	if (type != XE_GUC_CAT_ERR_TYPE_INVALID)
+		xe_gt_dbg(gt,
+			  "Engine mem cat error [0x%08x]: class=%s, logical_mask: 0x%x, guc_id=%d",
+			  type, xe_hw_engine_class_to_str(q->class), q->logical_mask, guc_id);
+	else
+		xe_gt_dbg(gt,
+			  "Engine mem cat error: class=%s, logical_mask: 0x%x, guc_id=%d",
+			  xe_hw_engine_class_to_str(q->class), q->logical_mask, guc_id);
 
 	trace_xe_exec_queue_memory_cat_error(q);
 
diff --git a/drivers/gpu/drm/xe/xe_guc_types.h b/drivers/gpu/drm/xe/xe_guc_types.h
index 63bac64429a5..d1de288d816d 100644
--- a/drivers/gpu/drm/xe/xe_guc_types.h
+++ b/drivers/gpu/drm/xe/xe_guc_types.h
@@ -101,6 +101,9 @@ struct xe_guc {
 		u32 size;
 	} hwconfig;
 
+	/** @opt_in_bo: buffer used for the OPT_IN_FEATURE_KLV H2G */
+	struct xe_bo *opt_in_bo;
+
 	/** @relay: GuC Relay Communication used in SR-IOV */
 	struct xe_guc_relay relay;
 
diff --git a/drivers/gpu/drm/xe/xe_uc.c b/drivers/gpu/drm/xe/xe_uc.c
index c14bd2282044..7aa93deb4325 100644
--- a/drivers/gpu/drm/xe/xe_uc.c
+++ b/drivers/gpu/drm/xe/xe_uc.c
@@ -165,6 +165,10 @@ static int vf_uc_init_hw(struct xe_uc *uc)
 
 	uc->guc.submission_state.enabled = true;
 
+	err = xe_guc_opt_in_features_enable(&uc->guc);
+	if (err)
+		return err;
+
 	err = xe_gt_record_default_lrcs(uc_to_gt(uc));
 	if (err)
 		return err;
-- 
2.43.0

Re: [PATCH 2/3] drm/xe/guc: Enable extended CAT error reporting

[Nirmoy Das]

On 2/27/2025 2:05 AM, Daniele Ceraolo Spurio wrote:
> On newer HW (Xe2 onwards + PVC) it is possible to get extra information
> when a CAT error occurs, specifically a dword reporting the error type.
> To enable this extra reporting, we need to opt-in with the GuC, which is
> done via a specific per-VF feature opt-in H2G.
>
> On platforms where the HW does not support the extra reporting, the GuC
> will set the type to 0xdeadbeef, so we can keep the code simple and
> opt-in to the feature on every platform and then just discard the data
> if it is invalid.
>
> Note that on native/PF we're guaranteed that the opt in is available
> because we don't support any GuC old enough to not have it, but if we're
> a VF we might be running on a non-XE PF with an older GuC, so we need to
> handle that case. We can re-use the invalid type above to handle this
> scenario the same way as if the feature was not supported in HW.
>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Nirmoy Das <nirmoy.das@intel.com>
> Cc: John Harrison <John.C.Harrison@Intel.com>

Thanks Daniele, this is a very useful feature which I used to debug few hard bugs.

Reviewed-by: Nirmoy Das <nirmoy.das@intel.com>

> ---
>  drivers/gpu/drm/xe/abi/guc_actions_abi.h |  4 ++
>  drivers/gpu/drm/xe/abi/guc_klvs_abi.h    | 15 +++++
>  drivers/gpu/drm/xe/xe_guc.c              | 83 ++++++++++++++++++++++++
>  drivers/gpu/drm/xe/xe_guc.h              |  1 +
>  drivers/gpu/drm/xe/xe_guc_submit.c       | 16 ++++-
>  drivers/gpu/drm/xe/xe_guc_types.h        |  3 +
>  drivers/gpu/drm/xe/xe_uc.c               |  4 ++
>  7 files changed, 123 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/abi/guc_actions_abi.h b/drivers/gpu/drm/xe/abi/guc_actions_abi.h
> index ec516e838ee8..fc632c738012 100644
> --- a/drivers/gpu/drm/xe/abi/guc_actions_abi.h
> +++ b/drivers/gpu/drm/xe/abi/guc_actions_abi.h
> @@ -141,6 +141,7 @@ enum xe_guc_action {
>  	XE_GUC_ACTION_CLIENT_SOFT_RESET = 0x5507,
>  	XE_GUC_ACTION_SET_ENG_UTIL_BUFF = 0x550A,
>  	XE_GUC_ACTION_SET_DEVICE_ENGINE_ACTIVITY_BUFFER = 0x550C,
> +	XE_GUC_ACTION_OPT_IN_FEATURE_KLV = 0x550E,
>  	XE_GUC_ACTION_NOTIFY_MEMORY_CAT_ERROR = 0x6000,
>  	XE_GUC_ACTION_REPORT_PAGE_FAULT_REQ_DESC = 0x6002,
>  	XE_GUC_ACTION_PAGE_FAULT_RES_DESC = 0x6003,
> @@ -239,4 +240,7 @@ enum xe_guc_g2g_type {
>  #define XE_G2G_DEREGISTER_TILE	REG_GENMASK(15, 12)
>  #define XE_G2G_DEREGISTER_TYPE	REG_GENMASK(11, 8)
>  
> +/* invalid type for XE_GUC_ACTION_NOTIFY_MEMORY_CAT_ERROR */
> +#define XE_GUC_CAT_ERR_TYPE_INVALID 0xdeadbeef
> +
>  #endif
> diff --git a/drivers/gpu/drm/xe/abi/guc_klvs_abi.h b/drivers/gpu/drm/xe/abi/guc_klvs_abi.h
> index d633f1c739e4..4c2b5bfde8fe 100644
> --- a/drivers/gpu/drm/xe/abi/guc_klvs_abi.h
> +++ b/drivers/gpu/drm/xe/abi/guc_klvs_abi.h
> @@ -16,6 +16,7 @@
>   *  +===+=======+==============================================================+
>   *  | 0 | 31:16 | **KEY** - KLV key identifier                                 |
>   *  |   |       |   - `GuC Self Config KLVs`_                                  |
> + *  |   |       |   - `GuC Opt In Feature KLVs`_                               |
>   *  |   |       |   - `GuC VGT Policy KLVs`_                                   |
>   *  |   |       |   - `GuC VF Configuration KLVs`_                             |
>   *  |   |       |                                                              |
> @@ -124,6 +125,20 @@ enum  {
>  	GUC_CONTEXT_POLICIES_KLV_NUM_IDS = 5,
>  };
>  
> +/**
> + * DOC: GuC Opt In Feature KLVs
> + *
> + * `GuC KLV`_ keys available for use with OPT_IN_FEATURE_KLV
> + *
> + *  _`GUC_KLV_GUC_OPT_IN_FEATURE_EXT_CAT_ERR_TYPE` : 0x4001
> + *      Adds an extra dword to the XE_GUC_ACTION_NOTIFY_MEMORY_CAT_ERROR G2H
> + *      containing the type of the CAT error. On HW that does not support
> + *      reporting the CAT error type, the extra dword is set to 0xdeadbeef.
> + */
> +
> +#define GUC_KLV_GUC_OPT_IN_FEATURE_EXT_CAT_ERR_TYPE_KEY 0x4001
> +#define GUC_KLV_GUC_OPT_IN_FEATURE_EXT_CAT_ERR_TYPE_LEN 0u
> +
>  /**
>   * DOC: GuC VGT Policy KLVs
>   *
> diff --git a/drivers/gpu/drm/xe/xe_guc.c b/drivers/gpu/drm/xe/xe_guc.c
> index 7b38447d902c..7483aba713b7 100644
> --- a/drivers/gpu/drm/xe/xe_guc.c
> +++ b/drivers/gpu/drm/xe/xe_guc.c
> @@ -29,6 +29,7 @@
>  #include "xe_guc_db_mgr.h"
>  #include "xe_guc_engine_activity.h"
>  #include "xe_guc_hwconfig.h"
> +#include "xe_guc_klv_helpers.h"
>  #include "xe_guc_log.h"
>  #include "xe_guc_pc.h"
>  #include "xe_guc_relay.h"
> @@ -569,6 +570,76 @@ static int guc_g2g_start(struct xe_guc *guc)
>  	return err;
>  }
>  
> +static int guc_opt_in_features_init(struct xe_guc *guc)
> +{
> +	struct xe_bo *bo;
> +	struct xe_gt *gt = guc_to_gt(guc);
> +	struct xe_tile *tile = gt_to_tile(gt);
> +	struct xe_device *xe = gt_to_xe(gt);
> +
> +	/* opt-in KLVs are all 1 DW so far, so a page is more than enough */
> +	bo = xe_managed_bo_create_pin_map(xe, tile, XE_PAGE_SIZE,
> +					  XE_BO_FLAG_SYSTEM |
> +					  XE_BO_FLAG_GGTT |
> +					  XE_BO_FLAG_GGTT_INVALIDATE);
> +	if (IS_ERR(bo)) {
> +		xe_gt_err(gt, "failed to allocate bo for GUC opt-in features\n");
> +		return PTR_ERR(bo);
> +	}
> +
> +	guc->opt_in_bo = bo;
> +
> +	return 0;
> +}
> +
> +static int __guc_opt_in_features_enable(struct xe_guc *guc, u64 addr, u32 num_dwords)
> +{
> +	u32 action[] = {
> +		XE_GUC_ACTION_OPT_IN_FEATURE_KLV,
> +		lower_32_bits(addr),
> +		upper_32_bits(addr),
> +		num_dwords
> +	};
> +
> +	return xe_guc_ct_send_block(&guc->ct, action, ARRAY_SIZE(action));
> +}
> +
> +int xe_guc_opt_in_features_enable(struct xe_guc *guc)
> +{
> +	struct xe_bo *bo = guc->opt_in_bo;
> +	struct xe_uc_fw_version *compat = &guc->fw.versions.found[XE_UC_FW_VER_COMPATIBILITY];
> +	u32 remain = bo->size;
> +	u32 offset = 0;
> +	int ret;
> +
> +	/*
> +	 * This opt-in was added in 70.17.0, so it's always available for
> +	 * native because we don't allow load of any firmware older than
> +	 * 70.29.2. However, with SRIOV we might be a VF running on a non-Xe PF
> +	 * with an older GuC release, so we need to check that. GuC 70.17.0 maps
> +	 * to compatibility version 1.7.0.
> +	 * Note that the GuC allows enabling this KLV even on platforms that do
> +	 * not support the extra type; in such case the returned type variable
> +	 * will be set to a known invalid value which we can check against.
> +	 */
> +	if (MAKE_GUC_VER_STRUCT(*compat) >= MAKE_GUC_VER(1, 7, 0))
> +		xe_guc_klv_enable_simple(guc, &bo->vmap,
> +					 GUC_KLV_GUC_OPT_IN_FEATURE_EXT_CAT_ERR_TYPE_KEY,
> +					 &offset, &remain);
> +
> +	if (offset) {
> +		ret = __guc_opt_in_features_enable(guc, xe_bo_ggtt_addr(bo), offset / 4);
> +		if (ret < 0) {
> +			xe_gt_err(guc_to_gt(guc),
> +				  "failed to enable GuC opt-in features: %pe\n",
> +				  ERR_PTR(ret));
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static void guc_fini_hw(void *arg)
>  {
>  	struct xe_guc *guc = arg;
> @@ -640,6 +711,10 @@ static int vf_guc_init(struct xe_guc *guc)
>  	if (err)
>  		return err;
>  
> +	err = guc_opt_in_features_init(guc);
> +	if (err)
> +		return err;
> +
>  	return 0;
>  }
>  
> @@ -684,6 +759,10 @@ int xe_guc_init(struct xe_guc *guc)
>  	if (ret)
>  		goto out;
>  
> +	ret = guc_opt_in_features_init(guc);
> +	if (ret)
> +		goto out;
> +
>  	xe_uc_fw_change_status(&guc->fw, XE_UC_FIRMWARE_LOADABLE);
>  
>  	ret = devm_add_action_or_reset(xe->drm.dev, guc_fini_hw, guc);
> @@ -769,6 +848,10 @@ int xe_guc_post_load_init(struct xe_guc *guc)
>  
>  	xe_guc_ads_populate_post_load(&guc->ads);
>  
> +	ret = xe_guc_opt_in_features_enable(guc);
> +	if (ret)
> +		return ret;
> +
>  	if (xe_guc_g2g_wanted(guc_to_xe(guc))) {
>  		ret = guc_g2g_start(guc);
>  		if (ret)
> diff --git a/drivers/gpu/drm/xe/xe_guc.h b/drivers/gpu/drm/xe/xe_guc.h
> index 58338be44558..4a66575f017d 100644
> --- a/drivers/gpu/drm/xe/xe_guc.h
> +++ b/drivers/gpu/drm/xe/xe_guc.h
> @@ -33,6 +33,7 @@ int xe_guc_reset(struct xe_guc *guc);
>  int xe_guc_upload(struct xe_guc *guc);
>  int xe_guc_min_load_for_hwconfig(struct xe_guc *guc);
>  int xe_guc_enable_communication(struct xe_guc *guc);
> +int xe_guc_opt_in_features_enable(struct xe_guc *guc);
>  int xe_guc_suspend(struct xe_guc *guc);
>  void xe_guc_notify(struct xe_guc *guc);
>  int xe_guc_auth_huc(struct xe_guc *guc, u32 rsa_addr);
> diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
> index 913c74d6e2ae..e53653cef66a 100644
> --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
> @@ -2035,12 +2035,16 @@ int xe_guc_exec_queue_memory_cat_error_handler(struct xe_guc *guc, u32 *msg,
>  	struct xe_gt *gt = guc_to_gt(guc);
>  	struct xe_exec_queue *q;
>  	u32 guc_id;
> +	u32 type = XE_GUC_CAT_ERR_TYPE_INVALID;
>  
> -	if (unlikely(len < 1))
> +	if (unlikely(!len || len > 2))
>  		return -EPROTO;
>  
>  	guc_id = msg[0];
>  
> +	if (len == 2)
> +		type = msg[1];
> +
>  	if (guc_id == GUC_ID_UNKNOWN) {
>  		/*
>  		 * GuC uses GUC_ID_UNKNOWN if it can not map the CAT fault to any PF/VF
> @@ -2054,8 +2058,14 @@ int xe_guc_exec_queue_memory_cat_error_handler(struct xe_guc *guc, u32 *msg,
>  	if (unlikely(!q))
>  		return -EPROTO;
>  
> -	xe_gt_dbg(gt, "Engine memory cat error: engine_class=%s, logical_mask: 0x%x, guc_id=%d",
> -		  xe_hw_engine_class_to_str(q->class), q->logical_mask, guc_id);
> +	if (type != XE_GUC_CAT_ERR_TYPE_INVALID)
> +		xe_gt_dbg(gt,
> +			  "Engine mem cat error [0x%08x]: class=%s, logical_mask: 0x%x, guc_id=%d",
> +			  type, xe_hw_engine_class_to_str(q->class), q->logical_mask, guc_id);
> +	else
> +		xe_gt_dbg(gt,
> +			  "Engine mem cat error: class=%s, logical_mask: 0x%x, guc_id=%d",
> +			  xe_hw_engine_class_to_str(q->class), q->logical_mask, guc_id);
>  
>  	trace_xe_exec_queue_memory_cat_error(q);
>  
> diff --git a/drivers/gpu/drm/xe/xe_guc_types.h b/drivers/gpu/drm/xe/xe_guc_types.h
> index 63bac64429a5..d1de288d816d 100644
> --- a/drivers/gpu/drm/xe/xe_guc_types.h
> +++ b/drivers/gpu/drm/xe/xe_guc_types.h
> @@ -101,6 +101,9 @@ struct xe_guc {
>  		u32 size;
>  	} hwconfig;
>  
> +	/** @opt_in_bo: buffer used for the OPT_IN_FEATURE_KLV H2G */
> +	struct xe_bo *opt_in_bo;
> +
>  	/** @relay: GuC Relay Communication used in SR-IOV */
>  	struct xe_guc_relay relay;
>  
> diff --git a/drivers/gpu/drm/xe/xe_uc.c b/drivers/gpu/drm/xe/xe_uc.c
> index c14bd2282044..7aa93deb4325 100644
> --- a/drivers/gpu/drm/xe/xe_uc.c
> +++ b/drivers/gpu/drm/xe/xe_uc.c
> @@ -165,6 +165,10 @@ static int vf_uc_init_hw(struct xe_uc *uc)
>  
>  	uc->guc.submission_state.enabled = true;
>  
> +	err = xe_guc_opt_in_features_enable(&uc->guc);
> +	if (err)
> +		return err;
> +
>  	err = xe_gt_record_default_lrcs(uc_to_gt(uc));
>  	if (err)
>  		return err;

Re: [PATCH 2/3] drm/xe/guc: Enable extended CAT error reporting

[John Harrison]

On 2/26/2025 17:05, Daniele Ceraolo Spurio wrote:
> On newer HW (Xe2 onwards + PVC) it is possible to get extra information
> when a CAT error occurs, specifically a dword reporting the error type.
> To enable this extra reporting, we need to opt-in with the GuC, which is
> done via a specific per-VF feature opt-in H2G.
>
> On platforms where the HW does not support the extra reporting, the GuC
> will set the type to 0xdeadbeef, so we can keep the code simple and
> opt-in to the feature on every platform and then just discard the data
> if it is invalid.
>
> Note that on native/PF we're guaranteed that the opt in is available
> because we don't support any GuC old enough to not have it, but if we're
> a VF we might be running on a non-XE PF with an older GuC, so we need to
> handle that case. We can re-use the invalid type above to handle this
> scenario the same way as if the feature was not supported in HW.
>
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Nirmoy Das <nirmoy.das@intel.com>
> Cc: John Harrison <John.C.Harrison@Intel.com>
> ---
>   drivers/gpu/drm/xe/abi/guc_actions_abi.h |  4 ++
>   drivers/gpu/drm/xe/abi/guc_klvs_abi.h    | 15 +++++
>   drivers/gpu/drm/xe/xe_guc.c              | 83 ++++++++++++++++++++++++
>   drivers/gpu/drm/xe/xe_guc.h              |  1 +
>   drivers/gpu/drm/xe/xe_guc_submit.c       | 16 ++++-
>   drivers/gpu/drm/xe/xe_guc_types.h        |  3 +
>   drivers/gpu/drm/xe/xe_uc.c               |  4 ++
>   7 files changed, 123 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/abi/guc_actions_abi.h b/drivers/gpu/drm/xe/abi/guc_actions_abi.h
> index ec516e838ee8..fc632c738012 100644
> --- a/drivers/gpu/drm/xe/abi/guc_actions_abi.h
> +++ b/drivers/gpu/drm/xe/abi/guc_actions_abi.h
> @@ -141,6 +141,7 @@ enum xe_guc_action {
>   	XE_GUC_ACTION_CLIENT_SOFT_RESET = 0x5507,
>   	XE_GUC_ACTION_SET_ENG_UTIL_BUFF = 0x550A,
>   	XE_GUC_ACTION_SET_DEVICE_ENGINE_ACTIVITY_BUFFER = 0x550C,
> +	XE_GUC_ACTION_OPT_IN_FEATURE_KLV = 0x550E,
>   	XE_GUC_ACTION_NOTIFY_MEMORY_CAT_ERROR = 0x6000,
>   	XE_GUC_ACTION_REPORT_PAGE_FAULT_REQ_DESC = 0x6002,
>   	XE_GUC_ACTION_PAGE_FAULT_RES_DESC = 0x6003,
> @@ -239,4 +240,7 @@ enum xe_guc_g2g_type {
>   #define XE_G2G_DEREGISTER_TILE	REG_GENMASK(15, 12)
>   #define XE_G2G_DEREGISTER_TYPE	REG_GENMASK(11, 8)
>   
> +/* invalid type for XE_GUC_ACTION_NOTIFY_MEMORY_CAT_ERROR */
> +#define XE_GUC_CAT_ERR_TYPE_INVALID 0xdeadbeef
> +
>   #endif
> diff --git a/drivers/gpu/drm/xe/abi/guc_klvs_abi.h b/drivers/gpu/drm/xe/abi/guc_klvs_abi.h
> index d633f1c739e4..4c2b5bfde8fe 100644
> --- a/drivers/gpu/drm/xe/abi/guc_klvs_abi.h
> +++ b/drivers/gpu/drm/xe/abi/guc_klvs_abi.h
> @@ -16,6 +16,7 @@
>    *  +===+=======+==============================================================+
>    *  | 0 | 31:16 | **KEY** - KLV key identifier                                 |
>    *  |   |       |   - `GuC Self Config KLVs`_                                  |
> + *  |   |       |   - `GuC Opt In Feature KLVs`_                               |
>    *  |   |       |   - `GuC VGT Policy KLVs`_                                   |
>    *  |   |       |   - `GuC VF Configuration KLVs`_                             |
>    *  |   |       |                                                              |
> @@ -124,6 +125,20 @@ enum  {
>   	GUC_CONTEXT_POLICIES_KLV_NUM_IDS = 5,
>   };
>   
> +/**
> + * DOC: GuC Opt In Feature KLVs
> + *
> + * `GuC KLV`_ keys available for use with OPT_IN_FEATURE_KLV
> + *
> + *  _`GUC_KLV_GUC_OPT_IN_FEATURE_EXT_CAT_ERR_TYPE` : 0x4001
> + *      Adds an extra dword to the XE_GUC_ACTION_NOTIFY_MEMORY_CAT_ERROR G2H
> + *      containing the type of the CAT error. On HW that does not support
> + *      reporting the CAT error type, the extra dword is set to 0xdeadbeef.
> + */
> +
> +#define GUC_KLV_GUC_OPT_IN_FEATURE_EXT_CAT_ERR_TYPE_KEY 0x4001
> +#define GUC_KLV_GUC_OPT_IN_FEATURE_EXT_CAT_ERR_TYPE_LEN 0u
> +
>   /**
>    * DOC: GuC VGT Policy KLVs
>    *
> diff --git a/drivers/gpu/drm/xe/xe_guc.c b/drivers/gpu/drm/xe/xe_guc.c
> index 7b38447d902c..7483aba713b7 100644
> --- a/drivers/gpu/drm/xe/xe_guc.c
> +++ b/drivers/gpu/drm/xe/xe_guc.c
> @@ -29,6 +29,7 @@
>   #include "xe_guc_db_mgr.h"
>   #include "xe_guc_engine_activity.h"
>   #include "xe_guc_hwconfig.h"
> +#include "xe_guc_klv_helpers.h"
>   #include "xe_guc_log.h"
>   #include "xe_guc_pc.h"
>   #include "xe_guc_relay.h"
> @@ -569,6 +570,76 @@ static int guc_g2g_start(struct xe_guc *guc)
>   	return err;
>   }
>   
> +static int guc_opt_in_features_init(struct xe_guc *guc)
> +{
> +	struct xe_bo *bo;
> +	struct xe_gt *gt = guc_to_gt(guc);
> +	struct xe_tile *tile = gt_to_tile(gt);
> +	struct xe_device *xe = gt_to_xe(gt);
> +
> +	/* opt-in KLVs are all 1 DW so far, so a page is more than enough */
> +	bo = xe_managed_bo_create_pin_map(xe, tile, XE_PAGE_SIZE,
> +					  XE_BO_FLAG_SYSTEM |
> +					  XE_BO_FLAG_GGTT |
> +					  XE_BO_FLAG_GGTT_INVALIDATE);
> +	if (IS_ERR(bo)) {
> +		xe_gt_err(gt, "failed to allocate bo for GUC opt-in features\n");
> +		return PTR_ERR(bo);
> +	}
> +
> +	guc->opt_in_bo = bo;
> +
> +	return 0;
> +}
> +
> +static int __guc_opt_in_features_enable(struct xe_guc *guc, u64 addr, u32 num_dwords)
> +{
> +	u32 action[] = {
> +		XE_GUC_ACTION_OPT_IN_FEATURE_KLV,
> +		lower_32_bits(addr),
> +		upper_32_bits(addr),
> +		num_dwords
> +	};
> +
> +	return xe_guc_ct_send_block(&guc->ct, action, ARRAY_SIZE(action));
> +}
> +
> +int xe_guc_opt_in_features_enable(struct xe_guc *guc)
> +{
> +	struct xe_bo *bo = guc->opt_in_bo;
> +	struct xe_uc_fw_version *compat = &guc->fw.versions.found[XE_UC_FW_VER_COMPATIBILITY];
> +	u32 remain = bo->size;
> +	u32 offset = 0;
> +	int ret;
> +
> +	/*
> +	 * This opt-in was added in 70.17.0, so it's always available for
> +	 * native because we don't allow load of any firmware older than
> +	 * 70.29.2. However, with SRIOV we might be a VF running on a non-Xe PF
> +	 * with an older GuC release, so we need to check that. GuC 70.17.0 maps
> +	 * to compatibility version 1.7.0.
> +	 * Note that the GuC allows enabling this KLV even on platforms that do
> +	 * not support the extra type; in such case the returned type variable
> +	 * will be set to a known invalid value which we can check against.
> +	 */
> +	if (MAKE_GUC_VER_STRUCT(*compat) >= MAKE_GUC_VER(1, 7, 0))
> +		xe_guc_klv_enable_simple(guc, &bo->vmap,
> +					 GUC_KLV_GUC_OPT_IN_FEATURE_EXT_CAT_ERR_TYPE_KEY,
> +					 &offset, &remain);
> +
> +	if (offset) {
> +		ret = __guc_opt_in_features_enable(guc, xe_bo_ggtt_addr(bo), offset / 4);
> +		if (ret < 0) {
> +			xe_gt_err(guc_to_gt(guc),
> +				  "failed to enable GuC opt-in features: %pe\n",
> +				  ERR_PTR(ret));
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>   static void guc_fini_hw(void *arg)
>   {
>   	struct xe_guc *guc = arg;
> @@ -640,6 +711,10 @@ static int vf_guc_init(struct xe_guc *guc)
>   	if (err)
>   		return err;
>   
> +	err = guc_opt_in_features_init(guc);
> +	if (err)
> +		return err;
> +
>   	return 0;
>   }
>   
> @@ -684,6 +759,10 @@ int xe_guc_init(struct xe_guc *guc)
>   	if (ret)
>   		goto out;
>   
> +	ret = guc_opt_in_features_init(guc);
> +	if (ret)
> +		goto out;
> +
>   	xe_uc_fw_change_status(&guc->fw, XE_UC_FIRMWARE_LOADABLE);
>   
>   	ret = devm_add_action_or_reset(xe->drm.dev, guc_fini_hw, guc);
> @@ -769,6 +848,10 @@ int xe_guc_post_load_init(struct xe_guc *guc)
>   
>   	xe_guc_ads_populate_post_load(&guc->ads);
>   
> +	ret = xe_guc_opt_in_features_enable(guc);
> +	if (ret)
> +		return ret;
> +
>   	if (xe_guc_g2g_wanted(guc_to_xe(guc))) {
>   		ret = guc_g2g_start(guc);
>   		if (ret)
> diff --git a/drivers/gpu/drm/xe/xe_guc.h b/drivers/gpu/drm/xe/xe_guc.h
> index 58338be44558..4a66575f017d 100644
> --- a/drivers/gpu/drm/xe/xe_guc.h
> +++ b/drivers/gpu/drm/xe/xe_guc.h
> @@ -33,6 +33,7 @@ int xe_guc_reset(struct xe_guc *guc);
>   int xe_guc_upload(struct xe_guc *guc);
>   int xe_guc_min_load_for_hwconfig(struct xe_guc *guc);
>   int xe_guc_enable_communication(struct xe_guc *guc);
> +int xe_guc_opt_in_features_enable(struct xe_guc *guc);
>   int xe_guc_suspend(struct xe_guc *guc);
>   void xe_guc_notify(struct xe_guc *guc);
>   int xe_guc_auth_huc(struct xe_guc *guc, u32 rsa_addr);
> diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
> index 913c74d6e2ae..e53653cef66a 100644
> --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
> @@ -2035,12 +2035,16 @@ int xe_guc_exec_queue_memory_cat_error_handler(struct xe_guc *guc, u32 *msg,
>   	struct xe_gt *gt = guc_to_gt(guc);
>   	struct xe_exec_queue *q;
>   	u32 guc_id;
> +	u32 type = XE_GUC_CAT_ERR_TYPE_INVALID;
>   
> -	if (unlikely(len < 1))
> +	if (unlikely(!len || len > 2))
>   		return -EPROTO;
>   
>   	guc_id = msg[0];
>   
> +	if (len == 2)
> +		type = msg[1];
> +
>   	if (guc_id == GUC_ID_UNKNOWN) {
>   		/*
>   		 * GuC uses GUC_ID_UNKNOWN if it can not map the CAT fault to any PF/VF
> @@ -2054,8 +2058,14 @@ int xe_guc_exec_queue_memory_cat_error_handler(struct xe_guc *guc, u32 *msg,
>   	if (unlikely(!q))
>   		return -EPROTO;
>   
> -	xe_gt_dbg(gt, "Engine memory cat error: engine_class=%s, logical_mask: 0x%x, guc_id=%d",
> -		  xe_hw_engine_class_to_str(q->class), q->logical_mask, guc_id);
> +	if (type != XE_GUC_CAT_ERR_TYPE_INVALID)
> +		xe_gt_dbg(gt,
> +			  "Engine mem cat error [0x%08x]: class=%s, logical_mask: 0x%x, guc_id=%d",
The type is only a 6-bit enum so I would say printing as a full 32bit 
hex values is overkill. A simple '%d' would seem more appropriate and 
easier to read.

John.

> +			  type, xe_hw_engine_class_to_str(q->class), q->logical_mask, guc_id);
> +	else
> +		xe_gt_dbg(gt,
> +			  "Engine mem cat error: class=%s, logical_mask: 0x%x, guc_id=%d",
> +			  xe_hw_engine_class_to_str(q->class), q->logical_mask, guc_id);
>   
>   	trace_xe_exec_queue_memory_cat_error(q);
>   
> diff --git a/drivers/gpu/drm/xe/xe_guc_types.h b/drivers/gpu/drm/xe/xe_guc_types.h
> index 63bac64429a5..d1de288d816d 100644
> --- a/drivers/gpu/drm/xe/xe_guc_types.h
> +++ b/drivers/gpu/drm/xe/xe_guc_types.h
> @@ -101,6 +101,9 @@ struct xe_guc {
>   		u32 size;
>   	} hwconfig;
>   
> +	/** @opt_in_bo: buffer used for the OPT_IN_FEATURE_KLV H2G */
> +	struct xe_bo *opt_in_bo;
> +
>   	/** @relay: GuC Relay Communication used in SR-IOV */
>   	struct xe_guc_relay relay;
>   
> diff --git a/drivers/gpu/drm/xe/xe_uc.c b/drivers/gpu/drm/xe/xe_uc.c
> index c14bd2282044..7aa93deb4325 100644
> --- a/drivers/gpu/drm/xe/xe_uc.c
> +++ b/drivers/gpu/drm/xe/xe_uc.c
> @@ -165,6 +165,10 @@ static int vf_uc_init_hw(struct xe_uc *uc)
>   
>   	uc->guc.submission_state.enabled = true;
>   
> +	err = xe_guc_opt_in_features_enable(&uc->guc);
> +	if (err)
> +		return err;
> +
>   	err = xe_gt_record_default_lrcs(uc_to_gt(uc));
>   	if (err)
>   		return err;

[PATCH 3/3] drm/xe/guc: Enable the Dynamic Inhibit Context Switch optimization

[Daniele Ceraolo Spurio]

The Dynamic Inhibit Context Switch is an optimization aimed at reducing
the amount of time the HW is stuck waiting on an unsatisfied semaphore.
When this optimization is enabled, the GuC will dynamically modify the
CTX_CTRL_INHIBIT_SYN_CTX_SWITCH in the CTX_CONTEXT_CONTROL register of
lrcs to enable immediate switching out on an usatisfied semaphore wait
when multiple contexts are competing for time on the same engine.

This feature is available on recent HW from GuC 70.40.1 onwards and it
is enabled via a per-VF feature opt-in.

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: John Harrison <John.C.Harrison@Intel.com>
Cc: Julia Filipchuk <julia.filipchuk@intel.com>
---
 drivers/gpu/drm/xe/abi/guc_klvs_abi.h | 13 +++++++++++++
 drivers/gpu/drm/xe/xe_guc.c           | 11 +++++++++++
 2 files changed, 24 insertions(+)

diff --git a/drivers/gpu/drm/xe/abi/guc_klvs_abi.h b/drivers/gpu/drm/xe/abi/guc_klvs_abi.h
index 4c2b5bfde8fe..b86a074887a6 100644
--- a/drivers/gpu/drm/xe/abi/guc_klvs_abi.h
+++ b/drivers/gpu/drm/xe/abi/guc_klvs_abi.h
@@ -134,11 +134,24 @@ enum  {
  *      Adds an extra dword to the XE_GUC_ACTION_NOTIFY_MEMORY_CAT_ERROR G2H
  *      containing the type of the CAT error. On HW that does not support
  *      reporting the CAT error type, the extra dword is set to 0xdeadbeef.
+ *
+ * _`GUC_KLV_OPT_IN_FEATURE_DYNAMIC_INHIBIT_CONTEXT_SWITCH` : 0x4003
+ *      This KLV enables the Dynamic Inhibit Context Switch optimization, which
+ *      consists in the GuC setting the CTX_CTRL_INHIBIT_SYN_CTX_SWITCH bit to
+ *      zero in the CTX_CONTEXT_CONTROL register of LRCs that are submitted
+ *      to an oversubscribed engine. This will cause those contexts to be
+ *      switched out immediately if they hit an unsatisfied semaphore wait
+ *      (instead of waiting the full timeslice duration). The bit is instead set
+ *      to one if a single context is queued on the engine, to avoid it being
+ *      switched out if there isn't another context that can run in its place.
  */
 
 #define GUC_KLV_GUC_OPT_IN_FEATURE_EXT_CAT_ERR_TYPE_KEY 0x4001
 #define GUC_KLV_GUC_OPT_IN_FEATURE_EXT_CAT_ERR_TYPE_LEN 0u
 
+#define GUC_KLV_OPT_IN_FEATURE_DYNAMIC_INHIBIT_CONTEXT_SWITCH_KEY 0x4003
+#define GUC_KLV_OPT_IN_FEATURE_DYNAMIC_INHIBIT_CONTEXT_SWITCH_LEN 0u
+
 /**
  * DOC: GuC VGT Policy KLVs
  *
diff --git a/drivers/gpu/drm/xe/xe_guc.c b/drivers/gpu/drm/xe/xe_guc.c
index 7483aba713b7..b6704d0e1b40 100644
--- a/drivers/gpu/drm/xe/xe_guc.c
+++ b/drivers/gpu/drm/xe/xe_guc.c
@@ -606,6 +606,7 @@ static int __guc_opt_in_features_enable(struct xe_guc *guc, u64 addr, u32 num_dw
 
 int xe_guc_opt_in_features_enable(struct xe_guc *guc)
 {
+	struct xe_device *xe = guc_to_xe(guc);
 	struct xe_bo *bo = guc->opt_in_bo;
 	struct xe_uc_fw_version *compat = &guc->fw.versions.found[XE_UC_FW_VER_COMPATIBILITY];
 	u32 remain = bo->size;
@@ -627,6 +628,16 @@ int xe_guc_opt_in_features_enable(struct xe_guc *guc)
 					 GUC_KLV_GUC_OPT_IN_FEATURE_EXT_CAT_ERR_TYPE_KEY,
 					 &offset, &remain);
 
+	/*
+	 * Dynamic ICS is available for PVC and Xe2 and newer platforms. It
+	 * requires GuC 70.40.1, which maps to compatibility version 1.18.4
+	 */
+	if ((xe->info.platform == XE_PVC || GRAPHICS_VER(xe) >= 20) &&
+	    MAKE_GUC_VER_STRUCT(*compat) >= MAKE_GUC_VER(1, 18, 4))
+		xe_guc_klv_enable_simple(guc, &bo->vmap,
+					 GUC_KLV_OPT_IN_FEATURE_DYNAMIC_INHIBIT_CONTEXT_SWITCH_KEY,
+					 &offset, &remain);
+
 	if (offset) {
 		ret = __guc_opt_in_features_enable(guc, xe_bo_ggtt_addr(bo), offset / 4);
 		if (ret < 0) {
-- 
2.43.0

✗ CI.Patch_applied: failure for Enable GuC opt-in features

[Patchwork]

== Series Details ==

Series: Enable GuC opt-in features
URL   : https://patchwork.freedesktop.org/series/145532/
State : failure

== Summary ==

=== Applying kernel patches on branch 'drm-tip' with base: ===
Base commit: 8541a64154cd drm-tip: 2025y-02m-26d-23h-59m-28s UTC integration manifest
=== git am output follows ===
error: patch failed: drivers/gpu/drm/xe/xe_guc_ads.c:343
error: drivers/gpu/drm/xe/xe_guc_ads.c: patch does not apply
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Applying: drm/xe/guc: move KLV helpers to common file
Patch failed at 0001 drm/xe/guc: move KLV helpers to common file
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".