Re: [PATCH v4 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] xe: missing GuC capture registers for video and pre-Xe3p render engines
- [Medium] xe: invalid MMIO accesses during GuC error capture due to mixed BCS/RCS registers
--

commit 5e669b4655462fa28596fd46c77d07300abb8f59
Author: Nareshkumar Gollakoti <naresh.kumar.g@intel.com>

    drm/xe/guc: capture additional engine state registers

This patch adds a wider set of engine state registers to the GuC error capture
to improve post-mortem debugging of engine hangs and reset-related failures.

> diff --git a/drivers/gpu/drm/xe/xe_guc_capture.c b/drivers/gpu/drm/xe/xe_guc_capture.c
> index 6a20eab0f8204..b3c42052b70bb 100644
> --- a/drivers/gpu/drm/xe/xe_guc_capture.c
> +++ b/drivers/gpu/drm/xe/xe_guc_capture.c
> @@ -127,11 +127,7 @@ struct __guc_capture_parsed_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}, \
> -	{ RING_DMA_FADD_UDW(0),		REG_64BIT_HI_DW, 0,	0,	0,	"RING_DMA_FADD"}, \
> -	{ 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_DMA_FADD_UDW(0),		REG_64BIT_HI_DW, 0,	0,	0,	"RING_DMA_FADD"}
>  
>  #define COMMON_XELP_RC_CLASS \
>  	{ RCU_MODE,			REG_32BIT,	0,	0,	0,	"RCU_MODE"}

[Severity: Medium]
When removing the execlist registers from COMMON_BASE_ENGINE_INSTANCE,
does this break the capture for engines that rely solely on the common list?

Since the new XE_BCS_RCS_ENGINE_INSTANCE macro is only added to the Blitter
and Xe3p Render lists, this leaves xe_rc_inst_regs (pre-Xe3p render/compute),
xe_vd_inst_regs (video), xe_vec_inst_regs (video enhance), and
xe_lp_gsc_inst_regs without any execlist status tracking during a GuC error
capture.

[ ... ]

> @@ -150,6 +146,32 @@ 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 XE_BCS_RCS_ENGINE_INSTANCE \
> +	{ 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"}, \
> +	{ 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"}, \
> +	{ BCS_SWCTRL(0), REG_32BIT, 0, 0, 0, "BCS_SWCTRL"}, \
> +	{ 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_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"}
> +
>  /* XE_LP Global */
>  static const struct __guc_mmio_reg_descr xe_lp_global_regs[] = {
>  	COMMON_XELP_BASE_GLOBAL,

[ ... ]

> @@ -189,6 +211,7 @@ static const struct __guc_mmio_reg_descr xe_vec_inst_regs[] = {
>  /* Blitter Per-Engine-Instance */
>  static const struct __guc_mmio_reg_descr xe_blt_inst_regs[] = {
>  	COMMON_BASE_ENGINE_INSTANCE,
> +	XE_BCS_RCS_ENGINE_INSTANCE,
>  };
>  
>  /* XE_LP - GSC Per-Engine-Instance */
> @@ -200,6 +223,8 @@ 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,
> +	XE_BCS_RCS_ENGINE_INSTANCE,
> +	XE3P_RC_SLICE_INSTANCE,
>  };
>  
>  /*

[Severity: Medium]
Does mixing Blitter and Render exclusive registers into a single macro cause
invalid MMIO accesses?

BCS_SWCTRL is Blitter-specific, while CS_CHICKEN1, CS_DEBUG_MODE1, and
CS_PREEMPTION_STATUS are Render-specific.

Because XE_BCS_RCS_ENGINE_INSTANCE is added to both xe_blt_inst_regs and
xe3p_rc_inst_regs, the GuC error capture will attempt to read unimplemented
MMIO addresses (Blitter offsets on Render engines and vice versa) during a
hang.

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