[PATCH] drm/i915/gt: Use VCS instead of VECS for GuC error capture on Gen 12

[PATCH] drm/i915/gt: Use VCS instead of VECS for GuC error capture on Gen 12

[Krzysztof Karas]

Initial, empty XE_LPD register list for Video-Enhance class was
introduced in [1] until [2] filled it out with debug SFC_DONE
entries, but it was never explained in any of those commits why
using those registers for VECS class specifically was chosen.
Furthermore, in current testing it leads to failures [3], as
gem_exec_capture@capture@vecs0-smem runs report partial captures
on DG1, DG2 and MTL [4].

SFC_DONE resides in the VD forcewake domain and is available
only when the corresponding VCS engine is present (explanation
from [5]), but XE_LP capture table currently associates these
registers with VIDEOENHANCE. Move them to the VIDEO class so GuC
requests them for the class tied to VCS availability.

[1] commit 24492514ccbd
    ("drm/i915/guc: Update GuC ADS size for error capture lists")
[2] commit 8b72c21618de
    ("drm/i915/guc: Add XE_LP static registers for GuC error capture.")
[3] https://gitlab.freedesktop.org/drm/i915/kernel/-/work_items/16904
[4] https://intel-gfx-ci.01.org/tree/drm-tip/index.html?testfilter=igt@gem_exec_capture@capture@vecs0-smem
[5] commit 24d032e2359e
    ("drm/i915: Only access SFC_DONE when media domain is not fused off")

Fixes: 8b72c21618de ("drm/i915/guc: Add XE_LP static registers for GuC error capture.")
Signed-off-by: Krzysztof Karas <krzysztof.karas@intel.com>
---
I could not verify when exactly the issue started happening,
because:
A) all recent reports are red on DG1, DG2 and MTL,
B) checking for "partial-capture" in IGT [6] was introduced way
   later than the driver code asking for SFC_DONE registers.
I assume it has been present for at least a few months, if not
from the very beginning.

From empirical experiments, I was able to identify the classes
that report "partial-capture" during the test: VIDEOENHANCE and
BLITTER, while RENDER_COMPUTE, VIDEO and GSC_OTHER report
"full-capture" for SFC_DONE registers. I simply changed the
class of xe_lp_vec_class_regs inside
"static const struct __guc_mmio_reg_descr_group xe_lp_lists[]"
and ran the complete "capture" subtest mentioned above, so this
change should not (fingers crossed) explode anything.

[6] commit 93d3b0e28d7d
    ("tests/intel/gem_exec_capture: Check for partial register captures")
---
 drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
index 865b61057566d..bbf5188ae46cd 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
@@ -187,9 +187,9 @@ static const struct __guc_mmio_reg_descr_group xe_lp_lists[] = {
 	MAKE_REGLIST(xe_lp_global_regs, PF, GLOBAL, 0),
 	MAKE_REGLIST(xe_lp_rc_class_regs, PF, ENGINE_CLASS, GUC_CAPTURE_LIST_CLASS_RENDER_COMPUTE),
 	MAKE_REGLIST(gen8_rc_inst_regs, PF, ENGINE_INSTANCE, GUC_CAPTURE_LIST_CLASS_RENDER_COMPUTE),
-	MAKE_REGLIST(empty_regs_list, PF, ENGINE_CLASS, GUC_CAPTURE_LIST_CLASS_VIDEO),
+	MAKE_REGLIST(xe_lp_vec_class_regs, PF, ENGINE_CLASS, GUC_CAPTURE_LIST_CLASS_VIDEO),
 	MAKE_REGLIST(gen8_vd_inst_regs, PF, ENGINE_INSTANCE, GUC_CAPTURE_LIST_CLASS_VIDEO),
-	MAKE_REGLIST(xe_lp_vec_class_regs, PF, ENGINE_CLASS, GUC_CAPTURE_LIST_CLASS_VIDEOENHANCE),
+	MAKE_REGLIST(empty_regs_list, PF, ENGINE_CLASS, GUC_CAPTURE_LIST_CLASS_VIDEOENHANCE),
 	MAKE_REGLIST(gen8_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(gen8_blt_inst_regs, PF, ENGINE_INSTANCE, GUC_CAPTURE_LIST_CLASS_BLITTER),
-- 
2.34.1

Re: [PATCH] drm/i915/gt: Use VCS instead of VECS for GuC error capture on Gen 12

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/i915/guc: invalid SFC_DONE[0..3] capture list moved to VIDEO class
- [Low] drm/i915/guc: stale VEC naming and comment on xe_lp_vec_class_regs
--

commit 6562e410f725dfa5df84099db24c4e525c8d180e
Author: Krzysztof Karas <krzysztof.karas@intel.com>

drm/i915/gt: Use VCS instead of VECS for GuC error capture on Gen 12

This patch moves the xe_lp_vec_class_regs GuC error capture register list
containing GEN12_SFC_DONE(0..3) from GUC_CAPTURE_LIST_CLASS_VIDEOENHANCE to
GUC_CAPTURE_LIST_CLASS_VIDEO in xe_lp_lists[].

> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
> index 865b61057566d..bbf5188ae46cd 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
> @@ -187,9 +187,9 @@ static const struct __guc_mmio_reg_descr_group xe_lp_lists[] = {
>  	MAKE_REGLIST(xe_lp_global_regs, PF, GLOBAL, 0),
>  	MAKE_REGLIST(xe_lp_rc_class_regs, PF, ENGINE_CLASS, GUC_CAPTURE_LIST_CLASS_RENDER_COMPUTE),
>  	MAKE_REGLIST(gen8_rc_inst_regs, PF, ENGINE_INSTANCE, GUC_CAPTURE_LIST_CLASS_RENDER_COMPUTE),
> -	MAKE_REGLIST(empty_regs_list, PF, ENGINE_CLASS, GUC_CAPTURE_LIST_CLASS_VIDEO),
> +	MAKE_REGLIST(xe_lp_vec_class_regs, PF, ENGINE_CLASS, GUC_CAPTURE_LIST_CLASS_VIDEO),

[Severity: High]
Does moving xe_lp_vec_class_regs to GUC_CAPTURE_LIST_CLASS_VIDEO shift the
partial-capture regression from VECS engines to VCS engines on Gen12+
platforms?

During ADS initialization in
guc_capture_prep_lists()->intel_guc_capture_getlist()->guc_capture_list_init(),
GUC_CAPTURE_LIST_CLASS_VIDEO is populated with xe_lp_vec_class_regs, which
statically expands COMMON_GEN12BASE_VEC with all four SFC_DONE instances:

drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c:
#define COMMON_GEN12BASE_VEC \
	{ GEN12_SFC_DONE(0),        0,      0, "SFC_DONE[0]" }, \
	{ GEN12_SFC_DONE(1),        0,      0, "SFC_DONE[1]" }, \
	{ GEN12_SFC_DONE(2),        0,      0, "SFC_DONE[2]" }, \
	{ GEN12_SFC_DONE(3),        0,      0, "SFC_DONE[3]" }

As shown in gt_record_global_regs() and err_print_gt_global() in
drivers/gpu/drm/i915/i915_gpu_error.c, each GEN12_SFC_DONE(i) instance
resides in the corresponding even VDBOX forcewake domain and only exists
when both sfc_mask and the corresponding _VCS(i * 2) engine are present:

drivers/gpu/drm/i915/i915_gpu_error.c:gt_record_global_regs() {
	...
		for (i = 0; i < I915_MAX_SFC; i++) {
			/*
			 * SFC_DONE resides in the VD forcewake domain, so it
			 * only exists if the corresponding VCS engine is
			 * present.
			 */
			if ((gt->_gt->info.sfc_mask & BIT(i)) == 0 ||
			    !HAS_ENGINE(gt->_gt, _VCS(i * 2)))
				continue;

			gt->sfc_done[i] =
				intel_uncore_read(uncore, GEN12_SFC_DONE(i));
		}
	...
}

Across the Gen12+ platforms in drivers/gpu/drm/i915/i915_pci.c (TGL, RKL,
DG1, ADL-S, ADL-P, DG2, and MTL), VCS4 (i = 2, 0x1ce000) and VCS6 (i = 3,
0x1cf000) do not exist, and on single-VCS platforms such as DG1 and RKL (or
fused SKUs), VCS2 (i = 1, 0x1cd000) does not exist either. Because
xe_lp_vec_class_regs is a static table that is not filtered by sfc_mask and
HAS_ENGINE(gt, _VCS(i * 2)), when a VCS engine hangs and triggers GuC error
capture, won't GuC fail to read the non-existent SFC_DONE instances and flag
the VIDEO capture group with GUC_STATE_CAPTURE_GROUP_TYPE_PARTIAL?

Note that in the empirical test described in the patch notes (where only the
class of the existing xe_lp_vec_class_regs entry was changed without
removing the earlier entries in xe_lp_lists[]), guc_capture_get_one_list()
scans from the beginning of reglists[] and returns the first match:

drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c:guc_capture_get_one_list() {
	...
	for (i = 0; reglists[i].list; ++i) {
		if (reglists[i].owner == owner && reglists[i].type == type &&
		    (reglists[i].engine == id || reglists[i].type == GUC_CAPTURE_LIST_TYPE_GLOBAL))
			return &reglists[i];
	}
	return NULL;
}

When the later xe_lp_vec_class_regs entry was changed to
GUC_CAPTURE_LIST_CLASS_RENDER_COMPUTE or GUC_CAPTURE_LIST_CLASS_VIDEO, the
earlier entries in xe_lp_lists[] (xe_lp_rc_class_regs for RENDER_COMPUTE and
empty_regs_list for VIDEO) matched first and shadowed xe_lp_vec_class_regs,
whereas replacing empty_regs_list directly in this patch makes
xe_lp_vec_class_regs active for GUC_CAPTURE_LIST_CLASS_VIDEO.

>  	MAKE_REGLIST(gen8_vd_inst_regs, PF, ENGINE_INSTANCE, GUC_CAPTURE_LIST_CLASS_VIDEO),
> -	MAKE_REGLIST(xe_lp_vec_class_regs, PF, ENGINE_CLASS, GUC_CAPTURE_LIST_CLASS_VIDEOENHANCE),
> +	MAKE_REGLIST(empty_regs_list, PF, ENGINE_CLASS, GUC_CAPTURE_LIST_CLASS_VIDEOENHANCE),

[Severity: Low]
This isn't a bug, but since xe_lp_vec_class_regs is moved from
GUC_CAPTURE_LIST_CLASS_VIDEOENHANCE to GUC_CAPTURE_LIST_CLASS_VIDEO, its
definition and comment in drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
still describe it as Video Enhancement (vec) rather than matching the vd
naming used for GUC_CAPTURE_LIST_CLASS_VIDEO:

/* XE_LP Video Enhancement Per-Class */
static const struct __guc_mmio_reg_descr xe_lp_vec_class_regs[] = {
	COMMON_GEN12BASE_VEC,
};

Should COMMON_GEN12BASE_VEC, xe_lp_vec_class_regs, and the accompanying
comment be updated if this list is associated with the VIDEO class?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030083948.131717-1-krzysztof.karas@intel.com?part=1

✓ i915.CI.BAT: success for drm/i915/gt: Use VCS instead of VECS for GuC error capture on Gen 12

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 1024 bytes --]

== Series Details ==

Series: drm/i915/gt: Use VCS instead of VECS for GuC error capture on Gen 12
URL   : https://patchwork.freedesktop.org/series/177227/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_19417 -> Patchwork_177227v1
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_177227v1/index.html

Participating hosts (40 -> 38)
------------------------------

  Missing    (2): bat-dg2-8 bat-dg2-13 


Changes
-------

  No changes found


Build changes
-------------

  * Linux: CI_DRM_19417 -> Patchwork_177227v1

  CI-20190529: 20190529
  CI_DRM_19417: a8997640de2e96daca6ba2a01ed65a7bb48d3807 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_9171: 9171
  Patchwork_177227v1: a8997640de2e96daca6ba2a01ed65a7bb48d3807 @ git://anongit.freedesktop.org/gfx-ci/linux

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_177227v1/index.html

[-- Attachment #2: Type: text/html, Size: 1589 bytes --]

Re: [PATCH] drm/i915/gt: Use VCS instead of VECS for GuC error capture on Gen 12

[Sebastian Brzezinka]

Hi Krzysztof,

On Fri Oct 30, 2026 at 9:39 AM CET, Krzysztof Karas wrote:
> Initial, empty XE_LPD register list for Video-Enhance class was
> introduced in [1] until [2] filled it out with debug SFC_DONE
> entries, but it was never explained in any of those commits why
> using those registers for VECS class specifically was chosen.
> Furthermore, in current testing it leads to failures [3], as
> gem_exec_capture@capture@vecs0-smem runs report partial captures
> on DG1, DG2 and MTL [4].
>
> SFC_DONE resides in the VD forcewake domain and is available
> only when the corresponding VCS engine is present (explanation
> from [5]), but XE_LP capture table currently associates these
> registers with VIDEOENHANCE. Move them to the VIDEO class so GuC
> requests them for the class tied to VCS availability.
>
> [1] commit 24492514ccbd
>     ("drm/i915/guc: Update GuC ADS size for error capture lists")
> [2] commit 8b72c21618de
>     ("drm/i915/guc: Add XE_LP static registers for GuC error capture.")
> [3] https://gitlab.freedesktop.org/drm/i915/kernel/-/work_items/16904
> [4] https://intel-gfx-ci.01.org/tree/drm-tip/index.html?testfilter=igt@gem_exec_capture@capture@vecs0-smem
> [5] commit 24d032e2359e
>     ("drm/i915: Only access SFC_DONE when media domain is not fused off")
>
> Fixes: 8b72c21618de ("drm/i915/guc: Add XE_LP static registers for GuC error capture.")
> Signed-off-by: Krzysztof Karas <krzysztof.karas@intel.com>
> ---
> I could not verify when exactly the issue started happening,
> because:
> A) all recent reports are red on DG1, DG2 and MTL,
> B) checking for "partial-capture" in IGT [6] was introduced way
>    later than the driver code asking for SFC_DONE registers.
> I assume it has been present for at least a few months, if not
> from the very beginning.
>
> From empirical experiments, I was able to identify the classes
> that report "partial-capture" during the test: VIDEOENHANCE and
> BLITTER, while RENDER_COMPUTE, VIDEO and GSC_OTHER report
> "full-capture" for SFC_DONE registers. I simply changed the
> class of xe_lp_vec_class_regs inside
> "static const struct __guc_mmio_reg_descr_group xe_lp_lists[]"
> and ran the complete "capture" subtest mentioned above, so this
> change should not (fingers crossed) explode anything.
>
> [6] commit 93d3b0e28d7d
>     ("tests/intel/gem_exec_capture: Check for partial register captures")
> ---
>  drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
> index 865b61057566d..bbf5188ae46cd 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
> @@ -187,9 +187,9 @@ static const struct __guc_mmio_reg_descr_group xe_lp_lists[] = {
>  	MAKE_REGLIST(xe_lp_global_regs, PF, GLOBAL, 0),
>  	MAKE_REGLIST(xe_lp_rc_class_regs, PF, ENGINE_CLASS, GUC_CAPTURE_LIST_CLASS_RENDER_COMPUTE),
>  	MAKE_REGLIST(gen8_rc_inst_regs, PF, ENGINE_INSTANCE, GUC_CAPTURE_LIST_CLASS_RENDER_COMPUTE),
> -	MAKE_REGLIST(empty_regs_list, PF, ENGINE_CLASS, GUC_CAPTURE_LIST_CLASS_VIDEO),
> +	MAKE_REGLIST(xe_lp_vec_class_regs, PF, ENGINE_CLASS, GUC_CAPTURE_LIST_CLASS_VIDEO),
Now the naming is inconsistent: xe_lp_vec_class_regs is CLASS_VIDEO,
not CLASS_VIDEOENHANCE as its name suggests.

-- 
Best regards,
Sebastian