Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [Medium] drm/i915/selftests: hang_init() uses UABI-only GT0 context in multi-GT loop - [Medium] drm/i915/selftests: GT mismatch in selftest_execlists spinner contexts - [Medium] drm/i915/selftests: missing gsi_offset in __live_rc6_ctx() on media GT --
commit 39fe8a3841172dae27e66c85ef1663181f7d0e43 Author: Krzysztof Niemiec <[email protected]> drm/i915/selftests: Port GT-dependent selftests to work with multi-GT (low-hanging fruits part 1) This patch wraps several GT-dependent live and performance selftest suites in for_each_gt() loops so that they run against all GTs present on the system rather than only the primary GT. [ ... ] > diff --git a/drivers/gpu/drm/i915/gt/selftest_execlists.c > b/drivers/gpu/drm/i915/gt/selftest_execlists.c > index 21e5ed9f72a30..ddf71b3784bf7 100644 > --- a/drivers/gpu/drm/i915/gt/selftest_execlists.c > +++ b/drivers/gpu/drm/i915/gt/selftest_execlists.c > @@ -4495,12 +4495,21 @@ int intel_execlists_live_selftests(struct > drm_i915_private *i915) > SUBTEST(live_virtual_slice), > SUBTEST(live_virtual_reset), > }; > + struct intel_gt *gt; > + unsigned int id; > + int err; > > - if (to_gt(i915)->submission_method != INTEL_SUBMISSION_ELSP) > - return 0; > + for_each_gt(gt, i915, id) { > + if (gt->submission_method != INTEL_SUBMISSION_ELSP) > + continue; > > - if (intel_gt_is_wedged(to_gt(i915))) > - return 0; > + if (intel_gt_is_wedged(gt)) > + continue; > > - return intel_gt_live_subtests(tests, to_gt(i915)); > + err = intel_gt_live_subtests(tests, gt); [Severity: Medium] When intel_execlists_live_selftests() runs on a secondary GT where gt != to_gt(i915), won't several preemption subtests trigger a GEM_BUG_ON() in igt_spinner_create_request()? Specifically, live_preempt(), live_late_preempt(), live_preempt_timeout(), and preempt_client_init() (used by live_nopreempt(), live_preempt_cancel(), live_suppress_self_preempt(), and live_chain_preempt()) create GEM contexts with a NULL vm while initializing their spinners on gt: drivers/gpu/drm/i915/gt/selftest_execlists.c:live_preempt() { ... ctx_hi = kernel_context(gt->i915, NULL); if (!ctx_hi) return -ENOMEM; ctx_hi->sched.priority = I915_CONTEXT_MAX_USER_PRIORITY; ctx_lo = kernel_context(gt->i915, NULL); if (!ctx_lo) goto err_ctx_hi; ctx_lo->sched.priority = I915_CONTEXT_MIN_USER_PRIORITY; if (igt_spinner_init(&spin_hi, gt)) goto err_ctx_lo; ... } When kernel_context(gt->i915, NULL) calls i915_gem_create_context() with pc->vm == NULL, the context VM is created on to_gt(i915) rather than gt: drivers/gpu/drm/i915/gem/i915_gem_context.c:i915_gem_create_context() { ... if (pc->vm) { vm = i915_vm_get(pc->vm); } else if (HAS_FULL_PPGTT(i915)) { struct i915_ppgtt *ppgtt; ppgtt = i915_ppgtt_create(to_gt(i915), 0); ... } When those subtests then call spinner_create_request() -> igt_spinner_create_request(spin, ce, arb), spin->gt is gt while ce->vm->gt is to_gt(i915), hitting: drivers/gpu/drm/i915/selftests/igt_spinner.c:igt_spinner_create_request() { ... GEM_BUG_ON(spin->gt != ce->vm->gt); ... } > diff --git a/drivers/gpu/drm/i915/gt/selftest_gt_pm.c > b/drivers/gpu/drm/i915/gt/selftest_gt_pm.c > index 07eaf71955c44..09c0c4096665a 100644 > --- a/drivers/gpu/drm/i915/gt/selftest_gt_pm.c > +++ b/drivers/gpu/drm/i915/gt/selftest_gt_pm.c > @@ -205,9 +205,18 @@ int intel_gt_pm_late_selftests(struct drm_i915_private > *i915) > */ > SUBTEST(live_rc6_ctx_wa), > }; > + struct intel_gt *gt; > + unsigned int id; > + int err; > > - if (intel_gt_is_wedged(to_gt(i915))) > - return 0; > + for_each_gt(gt, i915, id) { > + if (intel_gt_is_wedged(gt)) > + continue; > > - return intel_gt_live_subtests(tests, to_gt(i915)); > + err = intel_gt_live_subtests(tests, gt); [Severity: Medium] Now that intel_gt_pm_late_selftests() runs live_rc6_ctx_wa() on every GT, does __live_rc6_ctx() read the wrong MMIO offset when executing on a standalone media GT? In drivers/gpu/drm/i915/gt/selftest_rc6.c, __live_rc6_ctx() emits an MI_STORE_REGISTER_MEM command for GEN8_RC6_CTX_INFO without adding ce->engine->gt->uncore->gsi_offset: drivers/gpu/drm/i915/gt/selftest_rc6.c:__live_rc6_ctx() { ... *cs++ = cmd; *cs++ = i915_mmio_reg_offset(GEN8_RC6_CTX_INFO); *cs++ = ce->timeline->hwsp_offset + 8; *cs++ = 0; ... } On platforms with a standalone media GT (where uncore->gsi_offset is 0x380000), won't the media engine command streamer read the primary GT's GEN8_RC6_CTX_INFO register at 0x8504 instead of the media GT's register at 0x388504? > diff --git a/drivers/gpu/drm/i915/gt/selftest_hangcheck.c > b/drivers/gpu/drm/i915/gt/selftest_hangcheck.c > index 53f3ce5c33a47..d67a6d75c56a4 100644 > --- a/drivers/gpu/drm/i915/gt/selftest_hangcheck.c > +++ b/drivers/gpu/drm/i915/gt/selftest_hangcheck.c > @@ -2016,21 +2016,27 @@ int intel_hangcheck_live_selftests(struct > drm_i915_private *i915) > SUBTEST(igt_reset_evict_fence), > SUBTEST(igt_handle_error), > }; > - struct intel_gt *gt = to_gt(i915); > intel_wakeref_t wakeref; > + struct intel_gt *gt; > + unsigned int id; > int err; > > - if (!intel_has_gpu_reset(gt)) > - return 0; > + for_each_gt(gt, i915, id) { > + if (!intel_has_gpu_reset(gt)) > + continue; > > - if (intel_gt_is_wedged(gt)) > - return -EIO; /* we're long past hope of a successful reset */ > + if (intel_gt_is_wedged(gt)) > + return -EIO; /* we're long past hope of a successful > reset */ > > - wakeref = intel_runtime_pm_get(gt->uncore->rpm); > + wakeref = intel_runtime_pm_get(gt->uncore->rpm); > > - err = intel_gt_live_subtests(tests, gt); > + err = intel_gt_live_subtests(tests, gt); [Severity: Medium] Can running intel_hangcheck_live_selftests() across all GTs cause subtests such as igt_hang_sanitycheck(), igt_reset_active_engine(), igt_reset_engines(), and igt_reset_queue() to fail with -EINVAL on secondary GTs that have non-UABI engines like GSC0? In drivers/gpu/drm/i915/gt/selftest_hangcheck.c, hang_init() sets up h->ctx via kernel_context(gt->i915, NULL): drivers/gpu/drm/i915/gt/selftest_hangcheck.c:hang_init() { ... h->ctx = kernel_context(gt->i915, NULL); if (IS_ERR(h->ctx)) return PTR_ERR(h->ctx); ... } Because vm is NULL, i915_gem_create_context() allocates h->ctx->vm on to_gt(i915) instead of gt, and populates h->ctx->engines via default_engines() using only for_each_uabi_engine(). Non-UABI engines such as GSC0 on the standalone media GT (which has uabi_class == I915_NO_UABI_CLASS) are skipped in intel_engines_driver_register(), leaving engine->legacy_idx as INVALID_ENGINE (-1). When the hangcheck subtests iterate over all engines on the GT with for_each_engine(engine, gt, id) and call hang_create_request(&h, engine) on GSC0: drivers/gpu/drm/i915/gt/selftest_hangcheck.c:hang_create_request() { ... rq = igt_request_alloc(h->ctx, engine); if (IS_ERR(rq)) { err = PTR_ERR(rq); goto unpin_hws; } ... } igt_request_alloc() looks up engine->legacy_idx in h->ctx: drivers/gpu/drm/i915/gem/selftests/igt_gem_utils.c:igt_request_alloc() { ... ce = i915_gem_context_get_engine(ctx, engine->legacy_idx); if (IS_ERR(ce)) return ERR_CAST(ce); ... } Should hang_init() pass gt->vm to kernel_context(), and should the hangcheck subtests either create per-engine contexts on gt or skip engines that are not populated in h->ctx? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
