Re: [PATCH v3 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] Regression in pre-Xe3 Render error capture due to registers omitted from `xe_rc_inst_regs`.
- [Medium] Engine-specific registers are incorrectly grouped into a shared capture list, causing GuC to read invalid MMIO offsets during error capture for mismatched engines.
--

commit 8561ce6705d44fcd9f5bafeb5f1a5d85ebea0ebb
Author: Nareshkumar Gollakoti <naresh.kumar.g@intel.com>

    drm/xe/guc: capture additional engine state registers
    
    Add a wider set of engine state registers to 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 026e8d628c30c..7f8f9aaf8465d 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"}

[Severity: Medium]
Will this cause a regression in pre-Xe3 Render error capture?

By removing the RING_EXECLIST_STATUS and RING_EXECLIST_SQ_CONTENTS registers 
from the COMMON_BASE_ENGINE_INSTANCE macro, they are no longer included in 
xe_rc_inst_regs:

static const struct __guc_mmio_reg_descr xe_rc_inst_regs[] = {
	COMMON_BASE_ENGINE_INSTANCE,
};

When a GPU hang occurs on a pre-Xe3 Render engine, GuC will now fail to 
capture these critical registers because XE_BCS_RCS_ENGINE_INSTANCE is not 
included in xe_rc_inst_regs.

[ ... ]

> +#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"}

[ ... ]

> @@ -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]
Will this mix engine-specific registers when capturing error state?

The XE_BCS_RCS_ENGINE_INSTANCE macro groups registers that belong exclusively 
to different engine classes. For example, it includes BCS_SWCTRL 
(Blitter-specific) alongside CS_CHICKEN1, CS_DEBUG_MODE1, and 
CS_PREEMPTION_STATUS (Render-specific).

Because this macro is added to both xe_blt_inst_regs and xe3p_rc_inst_regs, 
GuC will read Render-specific registers on Blitter engines, and 
Blitter-specific registers on Render and Compute engines. 

Could this corrupt the error capture dump with garbage data and potentially 
cause speculative hardware timeouts or unclaimed register errors if the MMIO 
offsets are unbacked on the mismatched engines?

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