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

[Daniele Ceraolo Spurio]

On 11/19/2024 3:13 PM, 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: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
>
> Changes from prior revs:
>   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
> ---
>   drivers/gpu/drm/xe/xe_guc_capture.c | 74 +++++++++++++++++++++++------
>   1 file changed, 59 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_guc_capture.c b/drivers/gpu/drm/xe/xe_guc_capture.c
> index f87755af545f..b06811a822f5 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,72 @@ 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.

nit: comment needs a newline after /* (can be fixed when merging). Same 
issue in a couple more places below.

Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>

Daniele

> +			 * 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);
>   
> -			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);
> +			if (is_ext) {
> +				int dss, 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);
> +				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);
> +			}
> +			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);
>   }
>   
>   /**