Re: [PATCH 1/1] drm/xe: Preserve healthy preempted LR queues across system suspend

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] drm/xe: missing primary checks in guc_exec_queue_should_preserve_lr()
- [High] drm/xe: late-restore LRC/CGP BO use-before-restore in guc_exec_queue_start()
- [High] drm/xe: unpreempted LR queues banned in guc_exec_queue_should_preserve_lr()
--

commit 7d7f69499735f49cbdb7c12c448c779bd3342556
Author: Huajun Li <huajun.li@intel.com>

drm/xe: Preserve healthy preempted LR queues across system suspend

This commit introduces a suspend-specific GuC submission-stop path that
preserves healthy long-running (LR) exec queues whose preemption has
completed, rather than banning them during system suspend. It also stops CT
and synchronizes with the G2H worker before sampling queue state and issuing
a GuC soft reset.

> diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
> index 9e7c2778029ca..bccd7cba03ee0 100644
> --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
> @@ -2733,14 +2733,51 @@ static const struct xe_exec_queue_ops guc_exec_queue_ops = {
>  	.reset_status = guc_exec_queue_reset_status,
>  };
>  
> -static void guc_exec_queue_stop(struct xe_guc *guc, struct xe_exec_queue *q)
> +static bool guc_exec_queue_should_preserve_lr(struct xe_guc *guc,
> +				struct xe_exec_queue *q, bool suspend)
> +{
> +	if (!suspend || !xe_exec_queue_is_lr(q))
> +		return false;
> +
> +	/*
> +	 * Do not preserve unhealthy LR queues that have already encountered
> +	 * a reset, ban, or fatal error prior to system suspend.
> +	 */
> +	if (exec_queue_killed_or_banned_or_wedged(q) || exec_queue_reset(q))
> +		return false;
> +
> +	/*
> +	 * SUSPENDED is set before requesting scheduling disable. The suspend
> +	 * fence and pending-disable state must also show completion before we
> +	 * can rely on the saved context surviving system sleep.
> +	 */
> +	if (!exec_queue_suspended(q) || READ_ONCE(q->guc->suspend_pending) ||
> +	    exec_queue_pending_disable(q)) {

[Severity: High]
Does guc_exec_queue_should_preserve_lr() improperly preserve multi-queue
secondary LR queues when the group's primary queue failed to complete
preemption or was reset?

In a multi-queue group, only the primary queue
(xe_exec_queue_multi_queue_primary(q)) owns a GuC scheduling context and
sends XE_GUC_ACTION_SCHED_CONTEXT_MODE_SET to GuC. On a secondary queue,
__guc_exec_queue_process_msg_suspend() either takes the
!exec_queue_enabled(q) branch immediately or calls disable_scheduling(),
which short-circuits on the CPU without waiting for GuC to preempt the
primary context:

drivers/gpu/drm/xe/xe_guc_submit.c:disable_scheduling() {
	...
	if (xe_exec_queue_is_multi_queue_secondary(q))
		handle_multi_queue_secondary_sched_done(guc, q, 0);
	else
		xe_guc_ct_send(&guc->ct, action, ARRAY_SIZE(action),
			       G2H_LEN_DW_SCHED_CONTEXT_MODE_SET, 1);
}

As a result, EXEC_QUEUE_STATE_SUSPENDED is set and q->guc->suspend_pending
and EXEC_QUEUE_STATE_PENDING_DISABLE are cleared on the secondary queue
before GuC has preempted the hardware context on the primary queue. Likewise,
GuC reset notifications and CGP context errors target primary->guc->id and
set EXEC_QUEUE_STATE_RESET or ban state on the primary queue. However,
guc_exec_queue_should_preserve_lr() only inspects q and never checks
xe_exec_queue_multi_queue_primary(q).

Furthermore, guc_submit_stop() iterates guc->submission_state.exec_queue_lookup
in a single pass and guc_exec_queue_stop() mutates each queue in place:

drivers/gpu/drm/xe/xe_guc_submit.c:guc_exec_queue_stop() {
	...
	xe_sched_msg_lock(sched);
	if (q->guc->suspend_pending) {
		set_exec_queue_suspended(q);
		suspend_fence_signal(q);
	}
	xe_sched_msg_unlock(sched);
	atomic_and(EXEC_QUEUE_STATE_WEDGED | EXEC_QUEUE_STATE_BANNED |
		   EXEC_QUEUE_STATE_KILLED | EXEC_QUEUE_STATE_DESTROYED |
		   EXEC_QUEUE_STATE_SUSPENDED,
		   &q->guc->state);
}

Because the primary queue is created before its secondaries and normally has a
lower guc_id, guc_exec_queue_stop() runs on the primary queue first, clearing
primary->guc->suspend_pending, EXEC_QUEUE_STATE_PENDING_DISABLE, and
EXEC_QUEUE_STATE_RESET (and only calling set_exec_queue_banned(primary)
rather than set_exec_queue_group_banned(primary)) before the secondary queues
are visited.

Additionally, if a multi-queue LR queue with a pending job is preserved, won't
restarting it on resume via xe_guc_submit_start() -> guc_exec_queue_start() ->
xe_sched_resubmit_jobs() -> guc_exec_queue_run_job() -> register_exec_queue()
call xe_guc_exec_queue_group_cgp_sync() and sleep waiting for GuC's
CGP_SYNC_DONE G2H while holding the reclaim-primed
guc->submission_state.lock?

> +		xe_gt_dbg(guc_to_gt(guc),
> +			  "LR exec queue %d failed to suspend prior to stop; will be banned\n",
> +			  q->guc->id);
> +		return false;
> +	}

[Severity: High]
Will guc_exec_queue_should_preserve_lr() still fail on healthy LR queues that
are not preempt-suspended during xe_bo_evict_all_user()?

Nothing in the system suspend path explicitly invokes q->ops->suspend(q) on LR
exec queues; instead, preempt-fence LR queues are only suspended as a side
effect when xe_bo_evict_all_user() evicts a TTM BO whose dma_resv holds
q->lr.pfence:

drivers/gpu/drm/xe/xe_bo_evict.c:xe_bo_evict_all_user() {
	...
		if (mem_type == XE_PL_TT && (IS_DGFX(xe) || !xe_device_has_flat_ccs(xe)))
			continue;

		if (man) {
			ret = ttm_resource_manager_evict_all(bdev, man);
			if (ret)
				return ret;
		}
}

Because of this, q->ops->suspend(q) is never called before xe_guc_suspend()
(leaving exec_queue_suspended(q) false) for:
- fault-mode LR VMs (xe_vm_in_fault_mode(q->vm)), which do not use preempt
  fences;
- iGPU platforms without FlatCCS (!IS_DGFX(xe) && !xe_device_has_flat_ccs(xe)),
  where xe_bo_evict_all_user() skips XE_PL_TT; and
- preempt-fence LR VMs whose bound buffers only use userptr VMAs or dGPU
  XE_PL_TT system memory BOs.

When xe_guc_submit_suspend() runs, won't guc_exec_queue_should_preserve_lr()
see !exec_queue_suspended(q) and return false, causing guc_exec_queue_stop()
to ban active LR queues in all of those configurations (and log that idle LR
queues "will be banned" even when they have no pending jobs)?

> +
> +	return true;
> +}
> +
> +static void guc_exec_queue_stop(struct xe_guc *guc, struct xe_exec_queue *q,
> +				bool suspend)
>  {
>  	struct xe_gpu_scheduler *sched = &q->guc->sched;
>  	bool do_destroy = false;
> +	bool preserve_lr;
>  
>  	/* Stop scheduling + flush any DRM scheduler operations */
>  	xe_sched_submission_stop(sched);
>  
> +	/*
> +	 * Sample acknowledged preemption before synthesizing lost G2H state.
> +	 * Only healthy, cleanly preempted LR queues are preserved across sleep.
> +	 */
> +	preserve_lr = guc_exec_queue_should_preserve_lr(guc, q, suspend);
> +
>  	/* Clean up lost G2H + reset engine state */
>  	if (exec_queue_registered(q)) {
>  		if (exec_queue_destroyed(q))
> @@ -2764,7 +2801,11 @@ static void guc_exec_queue_stop(struct xe_guc *guc, struct xe_exec_queue *q)
>  	 * started but not complete job or if a job has gone through a GT reset
>  	 * more than twice.
>  	 */
> -	if (!(q->flags & (EXEC_QUEUE_FLAG_KERNEL | EXEC_QUEUE_FLAG_VM))) {
> +	/*
> +	 * Preempted LR jobs normally remain unfinished. System sleep must keep
> +	 * their saved contexts, while reset recovery retains its ban policy.
> +	 */
> +	if (!(q->flags & (EXEC_QUEUE_FLAG_KERNEL | EXEC_QUEUE_FLAG_VM)) && !preserve_lr) {

[Severity: High]
When a userspace LR queue with a pending job is preserved here, does
guc_exec_queue_start() on resume access and register its LRC and CGP BOs
before xe_bo_restore_late() has restored them?

In xe_lrc_init() and xe_exec_queue_group_init(), userspace LRC BOs
(q->lrc[i]->bo) and multi-queue CGP BOs (group->cgp_bo) are created with
XE_BO_FLAG_PINNED_LATE_RESTORE:

drivers/gpu/drm/xe/xe_lrc.c:xe_lrc_init() {
	...
	bo_flags = XE_BO_FLAG_VRAM_IF_DGFX(tile) | XE_BO_FLAG_GGTT |
		   XE_BO_FLAG_GGTT_INVALIDATE;

	if ((vm && vm->xef) || init_flags & XE_LRC_CREATE_USER_CTX) /* userspace */
		bo_flags |= XE_BO_FLAG_PINNED_LATE_RESTORE | XE_BO_FLAG_FORCE_USER_VRAM;
}

During xe_pm_resume() (and D3cold xe_pm_runtime_resume()), xe_gt_resume() runs
before xe_bo_restore_late():

drivers/gpu/drm/xe/xe_pm.c:xe_pm_resume() {
	...
	err = xe_bo_restore_early(xe);
	if (err)
		goto err;
	...
	for_each_gt(gt, xe, id) {
		err = xe_gt_resume(gt);
		if (err)
			break;
	}
	...
	err = xe_bo_restore_late(xe);
}

Inside xe_gt_resume() -> do_gt_restart() -> xe_uc_start() ->
xe_guc_submit_start() -> guc_exec_queue_start(), any unbanned queue with a
pending job reads and writes q->lrc[i]->bo and resubmits jobs via
xe_sched_resubmit_jobs():

drivers/gpu/drm/xe/xe_guc_submit.c:guc_exec_queue_start() {
	...
	if (!exec_queue_killed_or_banned_or_wedged(q)) {
		struct xe_sched_job *job = xe_sched_first_pending_job(sched);
		int i;

		trace_xe_exec_queue_resubmit(q);
		if (job) {
			for (i = 0; i < q->width; ++i) {
				q->lrc[i]->ring.tail = job->ptrs[i].head;
				if (xe_sched_job_is_ulls(job))
					xe_lrc_set_ring_head(q->lrc[i],
							     job->ptrs[i].head);
				xe_lrc_set_ring_tail(q->lrc[i],
						     xe_lrc_ring_head(q->lrc[i]));
			}
		}
		xe_sched_resubmit_jobs(sched);
	}
}

Because xe_bo_restore_late() has not run yet when guc_exec_queue_start()
executes:
1. xe_lrc_ring_head() reads CTX_RING_HEAD / INDIRECT_CTX_RING_HEAD from
   un-restored VRAM, and xe_lrc_set_ring_tail(), emit_job(), and
   submit_exec_queue() write ring commands, CTX_RING_TAIL, and parallel
   workqueue items into un-restored VRAM.
2. register_exec_queue() registers q->lrc[i]->bo (and group->cgp_bo for
   multi-queue) with GuC before xe_bo_restore_late() has restored their GGTT
   PTEs via xe_bo_restore_and_map_ggtt().
3. When xe_bo_restore_late() subsequently runs, it copies bo->backup_obj back
   over q->lrc[i]->bo and group->cgp_bo in VRAM, overwriting the ring, tail,
   parallel WQ, and CGP updates made by guc_exec_queue_start().

Can a preserved userspace LR queue safely be restarted and have its jobs
resubmitted in xe_gt_resume() before xe_bo_restore_late() restores its LRC and
CGP BOs?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106042332.412621-1-huajun.li@intel.com?part=1