Re: [PATCH v5 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: unused FF_SLICE_CS_CHICKEN2 definition and missing capture entry
- [Medium] drm/xe/guc: use xe3p_blt_inst_regs for GUC_CAPTURE_LIST_CLASS_PAGING on Xe3p
--

commit 3bf1615b3ab8603f52d531a35c295135f10f2aa4
Author: Nareshkumar Gollakoti <naresh.kumar.g@intel.com>

drm/xe/guc: capture additional engine state registers

This commit defines additional engine state register offsets in
xe_engine_regs.h and includes them in the GuC error capture lists for Xe3p
render/compute and blitter engines, while also splitting out xe2_extregs[]
for steered register capture.

> diff --git a/drivers/gpu/drm/xe/regs/xe_engine_regs.h b/drivers/gpu/drm/xe/regs/xe_engine_regs.h
> index 94033982e6944..9f328b1b18f57 100644
> --- a/drivers/gpu/drm/xe/regs/xe_engine_regs.h
> +++ b/drivers/gpu/drm/xe/regs/xe_engine_regs.h

[ ... ]

> @@ -123,15 +128,21 @@
>  #define FF_SLICE_CS_CHICKEN1(base)		XE_REG((base) + 0xe0, XE_REG_OPTION_MASKED)
>  #define   FFSC_PERCTX_PREEMPT_CTRL		REG_BIT(14)
>  
> +#define FF_SLICE_CS_CHICKEN2(base)		XE_REG((base) + 0xe4, XE_REG_OPTION_MASKED)

[ ... ]

> diff --git a/drivers/gpu/drm/xe/xe_guc_capture.c b/drivers/gpu/drm/xe/xe_guc_capture.c
> index 6a20eab0f8204..2e13df97e1068 100644
> --- a/drivers/gpu/drm/xe/xe_guc_capture.c
> +++ b/drivers/gpu/drm/xe/xe_guc_capture.c

[ ... ]

> @@ -200,6 +230,15 @@ 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,
> +};

[Severity: Medium]
Was FF_SLICE_CS_CHICKEN2 (along with FF_SLICE_CS_CHICKEN1) accidentally
dropped from xe3p_rc_inst_regs[] in drivers/gpu/drm/xe/xe_guc_capture.c?

The commit message notes that the new register definitions in
drivers/gpu/drm/xe/regs/xe_engine_regs.h are included in the GuC capture
list, and the v3 changelog specifically mentions adding
XE3P_RC_SLICE_INSTANCE with FF_SLICE_CS_CHICKEN1 and FF_SLICE_CS_CHICKEN2 to
xe3p_rc_inst_regs[].

However, XE3P_RC_SLICE_INSTANCE is absent from drivers/gpu/drm/xe/xe_guc_capture.c,
which leaves FF_SLICE_CS_CHICKEN2 completely unused and omits both registers
when GuC error capture initializes xe3p_rc_inst_regs[] for
GUC_CAPTURE_LIST_CLASS_RENDER_COMPUTE.

> +
> +/* 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,
>  };
>  
>  /*
> @@ -264,7 +303,7 @@ static const struct __guc_mmio_reg_descr_group xe3p_lists[] = {
>  	MAKE_REGLIST(xe_vec_class_regs, PF, ENGINE_CLASS, GUC_CAPTURE_LIST_CLASS_VIDEOENHANCE),
>  	MAKE_REGLIST(xe_vec_inst_regs, PF, ENGINE_INSTANCE, GUC_CAPTURE_LIST_CLASS_VIDEOENHANCE),
>  	MAKE_REGLIST(empty_regs_list, PF, ENGINE_CLASS, GUC_CAPTURE_LIST_CLASS_BLITTER),
> -	MAKE_REGLIST(xe_blt_inst_regs, PF, ENGINE_INSTANCE, GUC_CAPTURE_LIST_CLASS_BLITTER),
> +	MAKE_REGLIST(xe3p_blt_inst_regs, PF, ENGINE_INSTANCE, GUC_CAPTURE_LIST_CLASS_BLITTER),
>  	MAKE_REGLIST(empty_regs_list, PF, ENGINE_CLASS, GUC_CAPTURE_LIST_CLASS_GSC_OTHER),
>  	MAKE_REGLIST(xe_lp_gsc_inst_regs, PF, ENGINE_INSTANCE, GUC_CAPTURE_LIST_CLASS_GSC_OTHER),
>  	MAKE_REGLIST(empty_regs_list, PF, ENGINE_CLASS, GUC_CAPTURE_LIST_CLASS_PAGING),

[Severity: Medium]
Should the GUC_CAPTURE_LIST_CLASS_PAGING entry right below this in
xe3p_lists[] also be updated from xe_blt_inst_regs to xe3p_blt_inst_regs?

In drivers/gpu/drm/xe/xe_guc_capture.c:xe3p_lists[]:

	MAKE_REGLIST(empty_regs_list, PF, ENGINE_CLASS, GUC_CAPTURE_LIST_CLASS_BLITTER),
	MAKE_REGLIST(xe3p_blt_inst_regs, PF, ENGINE_INSTANCE, GUC_CAPTURE_LIST_CLASS_BLITTER),
	MAKE_REGLIST(empty_regs_list, PF, ENGINE_CLASS, GUC_CAPTURE_LIST_CLASS_GSC_OTHER),
	MAKE_REGLIST(xe_lp_gsc_inst_regs, PF, ENGINE_INSTANCE, GUC_CAPTURE_LIST_CLASS_GSC_OTHER),
	MAKE_REGLIST(empty_regs_list, PF, ENGINE_CLASS, GUC_CAPTURE_LIST_CLASS_PAGING),
	MAKE_REGLIST(xe_blt_inst_regs, PF, ENGINE_INSTANCE, GUC_CAPTURE_LIST_CLASS_PAGING),

When xe_guc_has_paging_engine() in drivers/gpu/drm/xe/xe_guc.c is true on
Xe3p platforms, xe_hwe_to_guc_class() maps the USM-reserved physical BCS
engine (gt->usm.paging_hwe0) to GUC_PAGING_CLASS, which
xe_hwe_to_guc_capture_class() maps to GUC_CAPTURE_LIST_CLASS_PAGING.

Leaving GUC_CAPTURE_LIST_CLASS_PAGING pointing to xe_blt_inst_regs means
both ADS capture list population (guc_capture_prep_lists() ->
xe_guc_capture_getlist()) and manual engine snapshots
(xe_engine_manual_capture() -> xe_guc_capture_get_reg_desc_list()) for the
paging BCS engine will omit XE3P_CS_DEBUG_INSTANCE and
XE3P_BCS_ONLY_INSTANCE.

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