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 <[email protected]> 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 ®lists[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/[email protected]?part=1
