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

[Nirmoy Das <nirmoy.das@linux.intel.com>]

Hi Zhanjun,

There is a earlier patch regarding list->list check which is reviewed but not merged yet,

https://patchwork.freedesktop.org/patch/621250/?series=139810&rev=5


Could you please rebase your change on top of that  and may be carry that along as

the latest rev of that patch is not sent to xe so CI report is missing.


Regards,

Nirmoy

On 10/23/2024 9:23 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: 0f1fdf559225 ("drm/xe/guc: Save manual engine capture into capture list")
> Signed-off-by: Zhanjun Dong <zhanjun.dong@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_guc_capture.c | 26 +++++++++++++++++++++++---
>  1 file changed, 23 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_guc_capture.c b/drivers/gpu/drm/xe/xe_guc_capture.c
> index 8b6cb786a2aa..d7ff7dd60a1d 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"}
> @@ -1678,10 +1679,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);
>  
> @@ -1706,11 +1707,27 @@ snapshot_print_by_list_order(struct xe_hw_engine_snapshot *snapshot, struct drm_
>  		value = reg->value;
>  		if (reg_desc->data_type == 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.
> +			 */
> +			XE_WARN_ON(low32_ready); /* Possible double low here */
> +			low32_ready = true;
>  			/* Low 32 bit dword saved, continue for high 32 bit */
>  			continue;
>  		} else if (reg_desc->data_type == REG_64BIT_HI_DW) {
>  			u64 value_qw = ((u64)value << 32) | last_value;
>  
> +			/* Incorrect 64bit register order. Possible missing low */
> +			XE_WARN_ON(!low32_ready);
> +			low32_ready = false;
> +
>  			drm_printf(p, "\t%s: 0x%016llx\n", reg_desc->regname, value_qw);
>  			continue;
>  		}
> @@ -1727,6 +1744,9 @@ snapshot_print_by_list_order(struct xe_hw_engine_snapshot *snapshot, struct drm_
>  			drm_printf(p, "\t%s: 0x%08x\n", reg_desc->regname, value);
>  		}
>  	}
> +
> +	/* Incorrect 64bit register order. Possible missing high */
> +	XE_WARN_ON(low32_ready);
>  }
>  
>  /**