Intel-XE Archive on lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] drm/xe/guc: Fix missing init value and add register order check
@ 2024-11-04 18:24 Zhanjun Dong
  2024-11-04 19:31 ` ✓ CI.Patch_applied: success for " Patchwork
                   ` (8 more replies)
  0 siblings, 9 replies; 14+ messages in thread
From: Zhanjun Dong @ 2024-11-04 18:24 UTC (permalink / raw)
  To: intel-xe; +Cc: Zhanjun Dong, Alan Previn

Fix missing initial value for last_value.
For GuC capture register definition, it is required to define 64bit
register in a pair of 2 consecutive 32bit register entries, low first,
then hi. Add code to check this order.

Fixes: ecb633646391 ("drm/xe/guc: Plumb GuC-capture into dev coredump")

Signed-off-by: Zhanjun Dong <zhanjun.dong@intel.com>
Cc: Alan Previn <alan.previn.teres.alexis@intel.com>

Changes from prior revs:
 v2:- Correct the fix tag pointed commit
      Add examples in comments for warning
      Add 1 missing hi condition check
---
 drivers/gpu/drm/xe/xe_guc_capture.c | 80 +++++++++++++++++++++++------
 1 file changed, 64 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_guc_capture.c b/drivers/gpu/drm/xe/xe_guc_capture.c
index cc72446a5de1..8e534471b566 100644
--- a/drivers/gpu/drm/xe/xe_guc_capture.c
+++ b/drivers/gpu/drm/xe/xe_guc_capture.c
@@ -102,6 +102,7 @@ struct __guc_capture_parsed_output {
  *                   A 64 bit register define requires 2 consecutive entries,
  *                   with low dword first and hi dword the second.
  *     2. Register name: null for incompleted define
+ *     3. Incorrect order will trigger XE_WARN.
  */
 #define COMMON_XELP_BASE_GLOBAL \
 	{ FORCEWAKE_GT,			REG_32BIT,	0,	0,	"FORCEWAKE_GT"}
@@ -1675,10 +1676,10 @@ snapshot_print_by_list_order(struct xe_hw_engine_snapshot *snapshot, struct drm_
 	struct xe_devcoredump *devcoredump = &xe->devcoredump;
 	struct xe_devcoredump_snapshot *devcore_snapshot = &devcoredump->snapshot;
 	struct gcap_reg_list_info *reginfo = NULL;
-	u32 last_value, i;
-	bool is_ext;
+	u32 i, last_value = 0;
+	bool is_ext, low32_ready = false;
 
-	if (!list || list->num_regs == 0)
+	if (!list || !list->list || list->num_regs == 0)
 		return;
 	XE_WARN_ON(!devcore_snapshot->matched_node);
 
@@ -1701,29 +1702,76 @@ snapshot_print_by_list_order(struct xe_hw_engine_snapshot *snapshot, struct drm_
 			continue;
 
 		value = reg->value;
-		if (reg_desc->data_type == REG_64BIT_LOW_DW) {
+		switch (reg_desc->data_type) {
+		case REG_64BIT_LOW_DW:
 			last_value = value;
+
+			/*
+			 * A 64 bit register define requires 2 consecutive
+			 * entries in register list, with low dword first
+			 * and hi dword the second, like:
+			 *  { XXX_REG_LO(0), REG_64BIT_LOW_DW, 0, 0, NULL},
+			 *  { XXX_REG_HI(0), REG_64BIT_HI_DW,  0, 0, "XXX_REG"},
+			 *
+			 * Incorrect order will trigger XE_WARN.
+			 *
+			 * Possible double low here, for example:
+			 *  { XXX_REG_LO(0), REG_64BIT_LOW_DW, 0, 0, NULL},
+			 *  { XXX_REG_LO(0), REG_64BIT_LOW_DW, 0, 0, NULL},
+			 */
+			XE_WARN_ON(low32_ready);
+			low32_ready = true;
 			/* Low 32 bit dword saved, continue for high 32 bit */
-			continue;
-		} else if (reg_desc->data_type == REG_64BIT_HI_DW) {
+			break;
+
+		case REG_64BIT_HI_DW: {
 			u64 value_qw = ((u64)value << 32) | last_value;
 
+			/* Incorrect 64bit register order. Possible missing low.
+			 * for example:
+			 *  { XXX_REG(0), REG_32BIT, 0, 0, NULL},
+			 *  { XXX_REG_HI(0), REG_64BIT_HI_DW, 0, 0, NULL},
+			 */
+			XE_WARN_ON(!low32_ready);
+			low32_ready = false;
+
 			drm_printf(p, "\t%s: 0x%016llx\n", reg_desc->regname, value_qw);
-			continue;
-		}
+			}
+			break;
+
+		case REG_32BIT:
+			if (low32_ready) {
+				/* Incorrect 64bit register order. Possible missing high.
+				 * for example:
+				 *  { XXX_REG_LO(0), REG_64BIT_LOW_DW, 0, 0, NULL},
+				 *  { XXX_REG(0), REG_32BIT, 0, 0, "XXX_REG"},
+				 */
+				XE_WARN_ON(!low32_ready);
+				low32_ready = false;
+				break;
+			}
 
-		if (is_ext) {
-			int dss, group, instance;
+			if (is_ext) {
+				int dss, group, instance;
 
-			group = FIELD_GET(GUC_REGSET_STEERING_GROUP, reg_desc->flags);
-			instance = FIELD_GET(GUC_REGSET_STEERING_INSTANCE, reg_desc->flags);
-			dss = xe_gt_mcr_steering_info_to_dss_id(gt, group, instance);
+				group = FIELD_GET(GUC_REGSET_STEERING_GROUP, reg_desc->flags);
+				instance = FIELD_GET(GUC_REGSET_STEERING_INSTANCE, reg_desc->flags);
+				dss = xe_gt_mcr_steering_info_to_dss_id(gt, group, instance);
 
-			drm_printf(p, "\t%s[%u]: 0x%08x\n", reg_desc->regname, dss, value);
-		} else {
-			drm_printf(p, "\t%s: 0x%08x\n", reg_desc->regname, value);
+				drm_printf(p, "\t%s[%u]: 0x%08x\n", reg_desc->regname, dss, value);
+			} else {
+				drm_printf(p, "\t%s: 0x%08x\n", reg_desc->regname, value);
+			}
+			break;
 		}
 	}
+
+	/* Incorrect 64bit register order. Possible missing high.
+	 * for example:
+	 *  { XXX_REG_LO(0), REG_64BIT_LOW_DW, 0, 0, NULL},
+	 *  } // <- Register list end
+	 */
+	XE_WARN_ON(low32_ready);
 }
 
 /**
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread
end of thread, other threads:[~2024-11-05 23:14 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-11-04 18:24 [PATCH v2] drm/xe/guc: Fix missing init value and add register order check Zhanjun Dong
2024-11-04 19:31 ` ✓ CI.Patch_applied: success for " Patchwork
2024-11-04 19:32 ` ✓ CI.checkpatch: " Patchwork
2024-11-04 19:33 ` ✓ CI.KUnit: " Patchwork
2024-11-04 19:45 ` ✓ CI.Build: " Patchwork
2024-11-04 19:47 ` ✓ CI.Hooks: " Patchwork
2024-11-04 19:48 ` ✓ CI.checksparse: " Patchwork
2024-11-04 20:24 ` ✓ CI.BAT: " Patchwork
2024-11-05 12:49 ` ✗ CI.FULL: failure " Patchwork
2024-11-05 15:20   ` Dong, Zhanjun
2024-11-05 19:09 ` [PATCH v2] " Teres Alexis, Alan Previn
2024-11-05 20:54   ` Dong, Zhanjun
2024-11-05 22:04     ` Dixit, Ashutosh
2024-11-05 23:14       ` Dong, Zhanjun

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox