Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] drm/i915/selftests: partial per-GT resume in igt_pm_resume() - [Medium] drm/i915/selftests: wrong GGTT pinned in igt_vma_remapped_gtt() --
commit 8db2f33415f217cdf07f8583781cb2c2c4d93d25 Author: Krzysztof Niemiec <[email protected]> drm/i915/selftests: Port selftests that access ggtt to work with multi-gt This patch updates several i915 live selftests in i915_gem_client_blt.c, i915_gem.c, and i915_vma.c to iterate over each GT via for_each_gt() and intel_gt_live_subtests() instead of referencing to_gt(i915)->ggtt directly. > diff --git a/drivers/gpu/drm/i915/selftests/i915_gem.c > b/drivers/gpu/drm/i915/selftests/i915_gem.c > index ad650f67114a6..8f72fd1af8d61 100644 > --- a/drivers/gpu/drm/i915/selftests/i915_gem.c > +++ b/drivers/gpu/drm/i915/selftests/i915_gem.c [ ... ] > @@ -99,29 +100,29 @@ static int igt_pm_prepare(struct drm_i915_private *i915) > return 0; > } > > -static void igt_pm_suspend(struct drm_i915_private *i915) > +static void igt_pm_suspend(struct intel_gt *gt) > { > intel_wakeref_t wakeref; > > - with_intel_runtime_pm(&i915->runtime_pm, wakeref) { > - i915_ggtt_suspend(to_gt(i915)->ggtt); > - i915_gem_suspend_late(i915); > + with_intel_runtime_pm(>->i915->runtime_pm, wakeref) { > + i915_ggtt_suspend(gt->ggtt); > + i915_gem_suspend_late(gt->i915); > } > } > > -static void igt_pm_hibernate(struct drm_i915_private *i915) > +static void igt_pm_hibernate(struct intel_gt *gt) > { > intel_wakeref_t wakeref; > > - with_intel_runtime_pm(&i915->runtime_pm, wakeref) { > - i915_ggtt_suspend(to_gt(i915)->ggtt); > + with_intel_runtime_pm(>->i915->runtime_pm, wakeref) { > + i915_ggtt_suspend(gt->ggtt); > > - i915_gem_freeze(i915); > - i915_gem_freeze_late(i915); > + i915_gem_freeze(gt->i915); > + i915_gem_freeze_late(gt->i915); > } > } > > -static void igt_pm_resume(struct drm_i915_private *i915) > +static void igt_pm_resume(struct intel_gt *gt) > { > intel_wakeref_t wakeref; > > @@ -129,17 +130,18 @@ static void igt_pm_resume(struct drm_i915_private *i915) > * Both suspend and hibernate follow the same wakeup path and assume > * that runtime-pm just works. > */ > - with_intel_runtime_pm(&i915->runtime_pm, wakeref) { > - i915_ggtt_resume(to_gt(i915)->ggtt); > - if (GRAPHICS_VER(i915) >= 8) > - setup_private_pat(to_gt(i915)); > - i915_gem_resume(i915); > + with_intel_runtime_pm(>->i915->runtime_pm, wakeref) { > + i915_ggtt_resume(gt->ggtt); > + if (GRAPHICS_VER(gt->i915) >= 8) > + setup_private_pat(gt); > + i915_gem_resume(gt->i915); > } > } [Severity: Medium] Does converting igt_gem_suspend() and igt_gem_hibernate() into per-GT subtests via intel_gt_live_subtests() leave non-target GTs with unconfigured private PAT registers and unresumed GGTT mappings while device-wide GEM suspend and resume still reset and submit work to all GTs? In igt_gem_suspend() and igt_gem_hibernate(), live_context(i915, file) creates a context with engines across all GTs, and switch_to_context(ctx) submits requests across every engine on all GTs. Similarly, the GEM suspend/resume helpers operate across all GTs on the device: igt_pm_prepare() -> i915_gem_suspend() -> for_each_gt() intel_gt_suspend_prepare(gt) igt_pm_suspend() -> i915_gem_suspend_late() -> for_each_gt() intel_gt_suspend_late(gt) igt_pm_resume() -> i915_gem_resume() -> for_each_gt() intel_gt_resume(gt) However, igt_pm_suspend(), igt_pm_hibernate(), and igt_pm_resume() only call i915_ggtt_suspend(gt->ggtt), i915_ggtt_resume(gt->ggtt), and setup_private_pat(gt) on the single gt passed to the subtest. By contrast, i915_drm_resume() in drivers/gpu/drm/i915/i915_driver.c programs the private PAT for every GT before calling i915_gem_resume(): for_each_gt(gt, dev_priv, i) if (GRAPHICS_VER(gt->i915) >= 8) setup_private_pat(gt); On multi-GT platforms, sanitizing and resetting all GTs while only calling setup_private_pat(gt) and i915_ggtt_resume(gt->ggtt) on one GT leaves the other GTs without private PAT setup (and without GGTT/GuC mapping resume if the GTs have separate GGTTs) when i915_gem_resume() and switch_to_context(ctx) execute across all GTs. In addition, running i915_gem_live_selftests() in a for_each_gt() loop via intel_gt_live_subtests() only waits for idle on a single GT in __intel_gt_live_setup() and redundantly runs the GT-independent igt_gem_ww_ctx() test once per GT. Should i915_gem_live_selftests() remain a device-level i915_live_subtests() run and iterate for_each_gt() inside igt_pm_suspend(), igt_pm_hibernate(), igt_pm_resume(), and trash_stolen() instead? > diff --git a/drivers/gpu/drm/i915/selftests/i915_vma.c > b/drivers/gpu/drm/i915/selftests/i915_vma.c > index fc068dab27f44..3231025a7b7fa 100644 > --- a/drivers/gpu/drm/i915/selftests/i915_vma.c > +++ b/drivers/gpu/drm/i915/selftests/i915_vma.c > @@ -957,7 +957,8 @@ int i915_vma_mock_selftests(void) > > static int igt_vma_remapped_gtt(void *arg) > { > - struct drm_i915_private *i915 = arg; > + struct intel_gt *gt = arg; > + struct drm_i915_private *i915 = gt->i915; > const struct intel_remapped_plane_info planes[] = { > { .width = 1, .height = 1, .src_stride = 1 }, > { .width = 2, .height = 2, .src_stride = 2 }, > @@ -990,7 +991,7 @@ static int igt_vma_remapped_gtt(void *arg) > intel_wakeref_t wakeref; > int err = 0; > > - if (!i915_ggtt_has_aperture(to_gt(i915)->ggtt)) > + if (!i915_ggtt_has_aperture(gt->ggtt)) > return 0; > > obj = i915_gem_object_create_internal(i915, 10 * 10 * PAGE_SIZE); [Severity: Medium] In igt_vma_remapped_gtt(), the aperture check is updated to inspect gt->ggtt, but doesn't the body of the loop still pin into to_gt(i915)->ggtt? Later in igt_vma_remapped_gtt(), the object is pinned twice using i915_gem_object_ggtt_pin(): vma = i915_gem_object_ggtt_pin(obj, &view, 0, 0, PIN_MAPPABLE); ... vma = i915_gem_object_ggtt_pin(obj, NULL, 0, 0, PIN_MAPPABLE); i915_gem_object_ggtt_pin() calls i915_gem_object_ggtt_pin_ww() with ggtt = NULL, which defaults to to_gt(i915)->ggtt: drivers/gpu/drm/i915/i915_gem.c:i915_gem_object_ggtt_pin_ww() { struct drm_i915_private *i915 = to_i915(obj->base.dev); struct i915_ggtt *ggtt = to_gt(i915)->ggtt; if (_ggtt) ggtt = _ggtt; ... } If a non-primary GT has its own GGTT, igt_vma_remapped_gtt() will still bind and map through to_gt(i915)->ggtt (or fail with -E2BIG if to_gt(i915)->ggtt has a smaller aperture than gt->ggtt). Should those two calls use i915_gem_object_ggtt_pin_ww(obj, NULL, gt->ggtt, ...) so that gt->ggtt is actually used? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=5
