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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Engine-specific state registers (e.g., BCS_SWCTRL, FF_SLICE_CS_CHICKEN1) are incorrectly added to the `COMMON_BASE_ENGINE_INSTANCE` macro, causing GuC to attempt invalid MMIO reads on non-matching engine classes.
--

--- Patch [7]: [PATCH v2 7/7] drm/xe/guc: capture additional engine state registers ---
commit a1d512f9f15bd162d8a7f22264dfc3c44b0fa414
Author: Nareshkumar Gollakoti <naresh.kumar.g@intel.com>

drm/xe/guc: capture additional engine state registers

This commit adds a wider set of engine state registers to GuC error capture
to improve post-mortem debugging of engine hangs and reset-related
failures. It defines the missing register offsets and includes them in the
GuC capture list.

> diff --git a/drivers/gpu/drm/xe/xe_guc_capture.c b/drivers/gpu/drm/xe/xe_guc_capture.c
> index 62832bd4af78d..37e7e3d92bfcc 100644
> --- a/drivers/gpu/drm/xe/xe_guc_capture.c
> +++ b/drivers/gpu/drm/xe/xe_guc_capture.c
[ ... ]
> @@ -131,7 +131,25 @@ 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"}, \
> +	{ 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_CSBEFSM1"}, \
> +	{ BCS_SWCTRL(0),	REG_32BIT,	0,	0,	0,	"BCS_SWCTRL"}, \
> +	{ RING_CSFE_GLB_CHICKEN3(0), REG_32BIT, 0, 0, 0, "RING_CSFE_GLB_CHICKEN3" }, \

[Severity: Medium]
Does adding class-specific state registers like BCS_SWCTRL to the
COMMON_BASE_ENGINE_INSTANCE macro cause the GuC to attempt invalid MMIO reads
on non-matching engine classes?

Reading class-specific registers on all hardware engines could lead to
unclaimed register errors during an error capture on a Video or GSC engine,
which may flood the system logs with warnings.

> +	{ 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"}, \
> +	{ FF_SLICE_CS_CHICKEN1(0), REG_32BIT, 0, 0, 0, "FF_SLICE_CS_CHICKEN1" }, \
> +	{ 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" }, \
> +	{ FF_SLICE_CS_CHICKEN2(0), REG_32BIT, 0, 0, 0, "FF_SLICE_CS_CHICKEN2" }

[Severity: Medium]
Are these render-specific registers (FF_SLICE_*) being appended to the common
base macro?

This might pollute the register list for all engine classes. Should these be
placed in a class-specific list instead to avoid invalid hardware accesses
when non-matching engines hang?

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