Re: [PATCH v2] drm/xe/guc: Fix missing init value and add register order check

["Teres Alexis, Alan Previn" <alan.previn.teres.alexis@intel.com>]

A couple of nits, else LGTM, so:

Reviewed-by: Alan Previn <alan.previn.teres.alexis@intel.com>

On Mon, 2024-11-04 at 10:24 -0800, Zhanjun Dong wrote:
> 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: {
alan: nit: now that you have moved to a switch statement, is this { really required?
>                         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;
> -               }
> +                       }
alan: nit: ditto
> +                       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);
>  }
>  
>  /**