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

["Dong, Zhanjun" <zhanjun.dong@intel.com>]

On 2024-11-05 2:09 p.m., Teres Alexis, Alan Previn wrote:
> A couple of nits, else LGTM, so:
> 
> Reviewed-by: Alan Previn <alan.previn.teres.alexis@intel.com>

Thanks for review.

> 
> 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?
Few concerns:
1. Have {} here will make it works on most compilers and ANSIC settings.
2. Local variable "u64 value_qw" will be limited within this case 
statement, I don't want it to be referenced on other case statement.

>>                          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);
>>   }
>>   
>>   /**
>