Re: [RFC 1/5] drm/i915/selftests: Port GT-dependent selftests to work with multi-GT (low-hanging fruits part 1)

[sashiko-bot@kernel.org]

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

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