[PATCH v2] drm/xe/guc: Fix missing init value and add register order check
Dong, Zhanjun
zhanjun.dong at intel.com
Tue Nov 5 20:54:49 UTC 2024
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 at 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 at intel.com>
>> Cc: Alan Previn <alan.previn.teres.alexis at 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);
>> }
>>
>> /**
>
More information about the Intel-xe
mailing list