Re: [PATCH v6 7/7] drm/xe/guc: capture additional engine state registers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] drm/xe/guc: duplicate RING_BB_ADDR_DIFF entry in Xe3p capture lists
- [Medium] drm/xe/guc: wrong regname on BB_START_ADDR 64-bit capture descriptor
--

commit 700a38622c50aa9b624276048304bba0a59f3809
Author: Nareshkumar Gollakoti <naresh.kumar.g@intel.com>

drm/xe/guc: capture additional engine state registers

This commit adds additional engine state register offsets in
xe_engine_regs.h and includes them in the GuC error capture register lists
to provide more engine context in post-mortem capture dumps.

> diff --git a/drivers/gpu/drm/xe/xe_guc_capture.c b/drivers/gpu/drm/xe/xe_guc_capture.c
> index a8a3271991e2a..f255799428617 100644
> --- a/drivers/gpu/drm/xe/xe_guc_capture.c
> +++ b/drivers/gpu/drm/xe/xe_guc_capture.c
> @@ -124,6 +124,8 @@ struct __guc_capture_parsed_output {
>  	{ RING_ACTHD_UDW(0),		REG_64BIT_HI_DW, 0,	0,	0,	"ACTHD"}, \
>  	{ RING_BBADDR(0),		REG_64BIT_LOW_DW, 0,	0,	0,	NULL}, \
>  	{ RING_BBADDR_UDW(0),		REG_64BIT_HI_DW, 0,	0,	0,	"RING_BBADDR"}, \
> +	{ BB_START_ADDR(0),		REG_64BIT_LOW_DW, 0,	0,	0,	"BB_START_ADDR"}, \
> +	{ BB_START_ADDR_UDW(0),		REG_64BIT_HI_DW, 0,	0,	0,	"BB_START_ADDR_UDW"}, \

[Severity: Medium]
Should the regname for BB_START_ADDR(0) in COMMON_BASE_ENGINE_INSTANCE be
NULL and the regname for BB_START_ADDR_UDW(0) be "BB_START_ADDR", matching
the other 64-bit register pairs?

When an engine reset triggers xe_guc_capture_snapshot_print() ->
snapshot_print_by_list_order(), REG_64BIT_LOW_DW only records last_value and
last_address without using reg_desc->regname, while REG_64BIT_HI_DW combines
both dwords and prints reg_desc->regname from the high-dword entry:

drivers/gpu/drm/xe/xe_guc_capture.c:snapshot_print_by_list_order() {
	...
		case REG_64BIT_HI_DW: {
			u64 value_qw = ((u64)value << 32) | last_value;

			XE_WARN_ON(!low32_ready);
			low32_ready = false;

			drm_printf(p, "\t%s: 0x%016llx [addr=0x%x]\n",
				   reg_desc->regname,
				   value_qw,
				   last_address);
			break;
		}
	...
}

With the current descriptors, won't the combined 64-bit value at offset
0x150 be printed as "BB_START_ADDR_UDW" instead of "BB_START_ADDR" in
devcoredump output?

>  	{ RING_START(0),		REG_64BIT_LOW_DW, 0,	0,	0,	NULL}, \
>  	{ RING_START_UDW(0),		REG_64BIT_HI_DW, 0,	0,	0,	"RING_START"}, \
>  	{ RING_DMA_FADD(0),		REG_64BIT_LOW_DW, 0,	0,	0,	NULL}, \
> @@ -131,7 +133,9 @@ struct __guc_capture_parsed_output {
>  	{ RING_EXECLIST_STATUS_LO(0),	REG_64BIT_LOW_DW, 0,	0,	0,	NULL}, \
>  	{ RING_EXECLIST_STATUS_HI(0),	REG_64BIT_HI_DW, 0,	0,	0,	"RING_EXECLIST_STATUS"}, \
>  	{ RING_EXECLIST_SQ_CONTENTS_LO(0), REG_64BIT_LOW_DW, 0,	0,	0,	NULL}, \
> -	{ RING_EXECLIST_SQ_CONTENTS_HI(0), REG_64BIT_HI_DW, 0,	0,	0,	"RING_EXECLIST_SQ_CONTENTS"}
> +	{ RING_EXECLIST_SQ_CONTENTS_HI(0), REG_64BIT_HI_DW, 0,	0,	0,	"RING_EXECLIST_SQ_CONTENTS"}, \
> +	{ RING_BUFFER_HEAD_PREEMPT_REG(0),		REG_32BIT, 0,	0,	0,	"RING_BUFFER_HEAD_PREEMPT_REG"}, \
> +	{ RING_BB_ADDR_DIFF(0),		REG_32BIT, 0,	0,	0,	"RING_BB_ADDR_DIFF"} \
>  
>  #define COMMON_XELP_RC_CLASS \
>  	{ RCU_MODE,			REG_32BIT,	0,	0,	0,	"RCU_MODE"}
> @@ -150,6 +154,37 @@ struct __guc_capture_parsed_output {
>  #define XE3P_BASE_ENGINE_INSTANCE \
>  	{ RING_CSMQDEBUG(0),		REG_32BIT,	0,	0,	0,	"CSMQDEBUG"}
>  
> +#define XE3P_RC_SLICE_INSTANCE \
> +	{ FF_SLICE_CS_CHICKEN1(0), REG_32BIT, 0, 0, 0, "FF_SLICE_CS_CHICKEN1"}, \
> +	{ FF_SLICE_CS_CHICKEN2(0), REG_32BIT, 0, 0, 0, "FF_SLICE_CS_CHICKEN2"}
> +
> +#define XE3P_CS_DEBUG_INSTANCE \
> +	{ RESET_CTRL(0),		REG_32BIT,	0,	0,	0,	"RESET_CTRL"}, \
> +	{ RING_CSBEFSM3(0),		REG_32BIT,	0,	0,	0,	"RING_CSBEFSM3"}, \
> +	{ RING_CSFESM(0),		REG_32BIT,	0,	0,	0,	"RING_CSFESM"}, \
> +	{ RING_CSFESM2(0),		REG_32BIT,	0,	0,	0,	"RING_CSFESM2"}, \
> +	{ RING_CSFESM3(0),		REG_32BIT,	0,	0,	0,	"RING_CSFESM3"}, \
> +	{ RING_CSPWRFSM(0),		REG_32BIT,	0,	0,	0,	"RING_CSPWRFSM"}, \
> +	{ RING_CSBEFSM(0),		REG_32BIT,	0,	0,	0,	"RING_CSBEFSM"}, \
> +	{ RING_CSFE_GLB_CHICKEN3(0),	REG_32BIT,	0,	0,	0,	"RING_CSFE_GLB_CHICKEN3"}, \
> +	{ RING_PSMI_CTL(0),		REG_32BIT,	0,	0,	0,	"RING_PSMI_CTL"}, \
> +	{ RING_INSTPS(0),		REG_32BIT,	0,	0,	0,	"RING_INSTPS"}, \
> +	{ RING_BB_ADDR_DIFF(0),		REG_32BIT,	0,	0,	0,	"RING_BB_ADDR_DIFF"}, \
> +	{ RING_BB_STATE(0),		REG_32BIT,	0,	0,	0,	"RING_BB_STATE"}
> +
> +/*
> + * CS_CHICKEN1 has GPGPU/3D preempt bits; keep with RCS/CCS only.
> + * RCS+CCS share GUC_CAPTURE_LIST_CLASS_RENDER_COMPUTE, so these cannot
> + * be RCS-only.
> + */
> +#define XE3P_RC_CS_PREEMPT_INSTANCE \
> +	{ CS_DEBUG_MODE1(0),		REG_32BIT,	0,	0,	0,	"CS_DEBUG_MODE1"}, \
> +	{ CS_CHICKEN1(0),		REG_32BIT,	0,	0,	0,	"CS_CHICKEN1"}, \
> +	{ CS_PREEMPTION_STATUS(0),	REG_32BIT,	0,	0,	0,	"CS_PREEMPTION_STATUS"}
> +
> +#define XE3P_BCS_ONLY_INSTANCE \
> +	{ BCS_SWCTRL(0),		REG_32BIT,	0,	0,	0,	"BCS_SWCTRL"}
> +
>  /* XE_LP Global */
>  static const struct __guc_mmio_reg_descr xe_lp_global_regs[] = {
>  	COMMON_XELP_BASE_GLOBAL,
> @@ -186,9 +221,12 @@ static const struct __guc_mmio_reg_descr xe_vec_inst_regs[] = {
>  	COMMON_BASE_ENGINE_INSTANCE,
>  };
>  
> +#define XE_BLT_INST_REGS \
> +	COMMON_BASE_ENGINE_INSTANCE
> +
>  /* Blitter Per-Engine-Instance */
>  static const struct __guc_mmio_reg_descr xe_blt_inst_regs[] = {
> -	COMMON_BASE_ENGINE_INSTANCE,
> +	XE_BLT_INST_REGS,
>  };
>  
>  /* XE_LP - GSC Per-Engine-Instance */
> @@ -200,6 +238,16 @@ static const struct __guc_mmio_reg_descr xe_lp_gsc_inst_regs[] = {
>  static const struct __guc_mmio_reg_descr xe3p_rc_inst_regs[] = {
>  	COMMON_BASE_ENGINE_INSTANCE,
>  	XE3P_BASE_ENGINE_INSTANCE,
> +	XE3P_CS_DEBUG_INSTANCE,
> +	XE3P_RC_CS_PREEMPT_INSTANCE,
> +	XE3P_RC_SLICE_INSTANCE,
> +};
> +
> +/* Xe3p Blitter Per-Engine-Instance */
> +static const struct __guc_mmio_reg_descr xe3p_blt_inst_regs[] = {
> +	XE_BLT_INST_REGS,
> +	XE3P_CS_DEBUG_INSTANCE,
> +	XE3P_BCS_ONLY_INSTANCE,
>  };

[Severity: Medium]
Does including RING_BB_ADDR_DIFF(0) in both COMMON_BASE_ENGINE_INSTANCE and
XE3P_CS_DEBUG_INSTANCE duplicate that register in xe3p_rc_inst_regs[] and
xe3p_blt_inst_regs[]?

Because xe3p_rc_inst_regs[] expands both COMMON_BASE_ENGINE_INSTANCE and
XE3P_CS_DEBUG_INSTANCE, and xe3p_blt_inst_regs[] expands XE_BLT_INST_REGS
(which aliases COMMON_BASE_ENGINE_INSTANCE) alongside XE3P_CS_DEBUG_INSTANCE,
RING_BB_ADDR_DIFF(0) appears twice in both arrays.

On Xe3p platforms where guc_capture_get_device_reglist() selects
xe3p_lists[], this causes RING_BB_ADDR_DIFF(0) to be registered twice in the
GuC ADS capture list via guc_capture_list_init(), captured twice during
manual engine capture in read_reg_to_node(), and printed twice in
devcoredump output via snapshot_print_by_list_order().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261027051024.465587-1-naresh.kumar.g@intel.com?part=7