* Re: [PATCH v2] drm/i915/gt: Adjust usage of SFC_DONE registers on Gen 12
2026-11-05 7:14 [PATCH v2] drm/i915/gt: Adjust usage of SFC_DONE registers on Gen 12 Krzysztof Karas
2026-11-05 9:51 ` ✗ i915.CI.BAT: failure for " Patchwork
@ 2026-11-10 10:32 ` Sebastian Brzezinka
2026-11-12 11:23 ` Janusz Krzysztofik
2 siblings, 0 replies; 4+ messages in thread
From: Sebastian Brzezinka @ 2026-11-10 10:32 UTC (permalink / raw)
To: Krzysztof Karas, intel-gfx
Cc: dri-devel, Andi Shyti, Janusz Krzysztofik, Sebastian Brzezinka,
Krzysztof Niemiec
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
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v2] drm/i915/gt: Adjust usage of SFC_DONE registers on Gen 12
2026-11-05 7:14 [PATCH v2] drm/i915/gt: Adjust usage of SFC_DONE registers on Gen 12 Krzysztof Karas
2026-11-05 9:51 ` ✗ i915.CI.BAT: failure for " Patchwork
2026-11-10 10:32 ` [PATCH v2] " Sebastian Brzezinka
@ 2026-11-12 11:23 ` Janusz Krzysztofik
2 siblings, 0 replies; 4+ messages in thread
From: Janusz Krzysztofik @ 2026-11-12 11:23 UTC (permalink / raw)
To: Krzysztof Karas, intel-gfx
Cc: dri-devel, Andi Shyti, Sebastian Brzezinka, Krzysztof Niemiec
Hi Krzysztof,
On Thu, 2026-11-05 at 07:14 +0000, 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.
Since that's highly GuC related, I think you should also Cc: / ask for
review someone from GuC integration developers team directly, e.g.,
Daniele Ceraolo Spurio, Alan Previn or Umesh Nerlige Ramappa, maybe
others, not only i915 core.
Thanks,
Janusz
>
> 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 \
> { 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]" }, \
> @@ -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),
> + 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))
> + /**
> + * 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;
> else
> lists = gen8_lists;
^ permalink raw reply [flat|nested] 4+ messages in thread