Re: [PATCH v2] drm/i915/gt: Adjust usage of SFC_DONE registers on Gen 12

[Sebastian Brzezinka <sebastian.brzezinka@intel.com>]

Hi,

On Thu Nov 5, 2026 at 8:14 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, currently, it is seen as a regression [3].
>
> Currently, 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.
>
> Furthermore, GuC firmware supports SFC_DONE registers in error
> capture only for MTL platforms, DG1/DG2 are not included, which
> was confirmed with experimentation and offline discussion with
> GuC side.
>
> Make a new list of register lists for MTL and use SFC_DONE
> entries only when that platform is detected. For DG1/DG2 pass
> an empty list for VECS class.
>
> [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>
> ---
> v2:
>  * Changed commit title (was "drm/i915/gt: Use VCS instead of
>    VECS for GuC error capture on Gen 12") and commit
>    description;
>  * Enabled SFC_DONE on MTL's VCS engines and disabled these
>    registers on DG1/DG2;
>  * Added a new list for MTL;
>  * Renamed relevant objects vec -> vc.
>
> 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.
>
> [6] commit 93d3b0e28d7d
>     ("tests/intel/gem_exec_capture: Check for partial register captures")
> ---
>  .../gpu/drm/i915/gt/uc/intel_guc_capture.c    | 33 +++++++++++++++----
>  1 file changed, 27 insertions(+), 6 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 865b61057566..88e6d706210c 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_capture.c
> @@ -88,7 +88,7 @@
>  	{ GEN12_SC_INSTDONE_EXTRA,  0,      0, "GEN12_SC_INSTDONE_EXTRA" }, \
>  	{ GEN12_SC_INSTDONE_EXTRA2, 0,      0, "GEN12_SC_INSTDONE_EXTRA2" }
>  
> -#define COMMON_GEN12BASE_VEC \
> +#define COMMON_GEN12BASE_VC \
s/VC/VD/ ?

and if
#define GEN12_SFC_DONE(n) _MMIO(0x1cc000 + (n) * 0x1000)
- SFC_DONE(0) = 0x1cc000
- SFC_DONE(1) = 0x1cd000
- SFC_DONE(2) = 0x1ce000
- SFC_DONE(3) = 0x1cf000

>  	{ 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]" }, \
mtl_lists captures SFC_DONE(0..3) always. On the MTL media GT,
__xelpmp_fw_ranges (intel_uncore.c) has this entry:
------------------------------------------------------------
GEN_FW_RANGE(0x1cc000, 0x1cffff, FORCEWAKE_MEDIA_VDBOX0)
0x1cc000 - 0x1cdfff: VD0
0x1ce000 - 0x1cffff: reserved
------------------------------------------------------------

So only SFC_DONE(0) and (1) are in the documented range.  SFC_DONE(2)
and (3) (0x1ce000, 0x1cf000) are marked reserved.

I don't know how GuC handles reserved registers. Could you please either
trim the list to (0) and (1), or explain why reading (2) and (3) is fine.


> @@ -117,9 +117,9 @@ static const struct __guc_mmio_reg_descr gen8_vd_inst_regs[] = {
>  	COMMON_BASE_ENGINE_INSTANCE,
>  };
>  
> -/* XE_LP Video Enhancement Per-Class */
> -static const struct __guc_mmio_reg_descr xe_lp_vec_class_regs[] = {
> -	COMMON_GEN12BASE_VEC,
> +/* XE_LP Video Per-Class */
> +static const struct __guc_mmio_reg_descr xe_lp_vc_class_regs[] = {
> +	COMMON_GEN12BASE_VC,
>  };
>  
>  /* GEN8+ Video Enhancement Per-Engine-Instance */
> @@ -189,7 +189,22 @@ static const struct __guc_mmio_reg_descr_group xe_lp_lists[] = {
>  	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(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),
> +	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),
> +	{}
> +};
> +
> +static const struct __guc_mmio_reg_descr_group mtl_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(xe_lp_vc_class_regs, PF, ENGINE_CLASS, GUC_CAPTURE_LIST_CLASS_VIDEO),
a
Convention for CLASS_VIDEO is vd(e.g. gen8_vd_inst_regs).

> +	MAKE_REGLIST(gen8_vd_inst_regs, PF, ENGINE_INSTANCE, GUC_CAPTURE_LIST_CLASS_VIDEO),
> +	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),
> @@ -354,7 +369,13 @@ guc_capture_get_device_reglist(struct intel_guc *guc)
>  	struct drm_i915_private *i915 = guc_to_i915(guc);
>  	const struct __guc_mmio_reg_descr_group *lists;
>  
> -	if (GRAPHICS_VER(i915) >= 12)
> +	if (IS_METEORLAKE(i915))
> +		/**
Those are kernrl doc comments style.

> +		 * MTL is the only member of Gen 12 with SFC_DONE registers supported by
> +		 * error capture in GuC firmware, so use a separate list of registers there.
> +		 */
> +		lists = mtl_lists;
> +	else if (GRAPHICS_VER(i915) >= 12)
>  		lists = xe_lp_lists;
You never mention other gen12 platforms (TGL, RKL, ADL, PVC), and this
change is gonna affect them as well.

>  	else
>  		lists = gen8_lists;





-- 
Best regards,
Sebastian