drm/xe/guc: Fix missing init value and add register order check

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.

Changes from prior revs:
 v5:- Correct cross-line comment format
 v4:- Fix warn on condition and remove skipping
 v3:- Move break inside brace
 v2:- Correct the fix tag pointed commit
      Add examples in comments for warning
      Add 1 missing hi condition check

Fixes: ecb633646391 ("drm/xe/guc: Plumb GuC-capture into dev coredump")
Signed-off-by: Zhanjun Dong <[email protected]>
Reviewed-by: Daniele Ceraolo Spurio <[email protected]>
Signed-off-by: Daniele Ceraolo Spurio <[email protected]>
Link: https://patchwork.freedesktop.org/patch/msgid/[email protected]
(cherry picked from commit 6f59fbcfa041e7d69e5e5f39d4c8cffa06fdc50b)
Signed-off-by: Thomas Hellström <[email protected]>

diff --git a/drivers/gpu/drm/xe/xe_guc_capture.c b/drivers/gpu/drm/xe/xe_guc_capture.c
index cc72446a5de11a3e0776eb449571a0b4d9ebf9df..d63912d2824674aae39db5c0adff59a6ffabf548 100644 (file)
--- 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,75 @@ 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;
                }
 
-               if (is_ext) {
-                       int dss, group, instance;
+               case REG_32BIT:
+                       /*
+                        * 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);
+
+                       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);
 }
 
 /**