Re: [RFC 5/5] drm/i915/selftests: Port selftests that access ggtt to work with multi-gt

[sashiko-bot@kernel.org]

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 <krzysztof.niemiec@intel.com>

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(&gt->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(&gt->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(&gt->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/20261029014155.55865-1-krzysztof.niemiec@intel.com?part=5