Re: [PATCH v2 2/5] drm/xe/hw_engine_group: propagate suspend failures during mode switch

[Matthew Brost <matthew.brost@intel.com>]

On Tue, Jun 30, 2026 at 10:07:30PM -0700, Niranjana Vishwanathapura wrote:
> The hw engine group fault-mode switch suspends all faulting LR queues
> but ignored the suspend()/suspend_wait() return value. A suspend() can
> fail (e.g. the queue is killed/banned/wedged), leaving the queue
> un-suspended, so silently continuing could later resume a queue that was
> never suspended.
> 
> Propagate the failure instead: in xe_hw_engine_group_add_exec_queue()
> bail out if suspend() fails, and in
> xe_hw_engine_group_suspend_faulting_lr_jobs() undo the partial suspend
> via a new err_resume path that resumes the sibling queues already
> suspended in this call. Record per-queue success with lr.suspended so
> only queues that were actually suspended are waited on and resumed, and
> skip the cleanup resume() when suspend_wait() failed or the queue was
> reset/killed/banned/wedged (its suspend may not have completed, so
> resuming would trip the !suspend_pending assert in the resume path;
> teardown resolves its state instead).
> 
> Gate the group resume worker (hw_engine_group_resume_lr_jobs_func()) on
> lr.suspended for the same reason, so it only resumes queues that were
> actually suspended.
> 
> Assisted-by: Github-Copilot:Claude-opus-4.8
> Signed-off-by: Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_hw_engine_group.c | 76 ++++++++++++++++++++++++-
>  1 file changed, 73 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_hw_engine_group.c b/drivers/gpu/drm/xe/xe_hw_engine_group.c
> index 02cf32ae5aa9..84851929c16f 100644
> --- a/drivers/gpu/drm/xe/xe_hw_engine_group.c
> +++ b/drivers/gpu/drm/xe/xe_hw_engine_group.c
> @@ -34,6 +34,15 @@ hw_engine_group_resume_lr_jobs_func(struct work_struct *w)
>  		if (!xe_vm_in_fault_mode(q->vm))
>  			continue;
>  
> +		/*
> +		 * Only resume queues that were actually suspended. A queue whose
> +		 * suspend() failed (e.g. killed/banned/wedged) was never
> +		 * suspended, so it must not be resumed.
> +		 */
> +		if (!READ_ONCE(q->lr.suspended))
> +			continue;
> +
> +		WRITE_ONCE(q->lr.suspended, false);
>  		q->ops->resume(q);
>  	}
>  
> @@ -140,7 +149,18 @@ int xe_hw_engine_group_add_exec_queue(struct xe_hw_engine_group *group, struct x
>  		return err;
>  
>  	if (xe_vm_in_fault_mode(q->vm) && group->cur_mode == EXEC_MODE_DMA_FENCE) {
> -		q->ops->suspend(q);
> +		/*
> +		 * suspend() can fail (e.g. killed/banned/wedged), leaving the
> +		 * queue un-suspended. Propagate the failure so the queue is not
> +		 * added; on failure nothing was suspended, so there is nothing to
> +		 * undo. Only record the queue as suspended (and later resume it)
> +		 * once suspend() has succeeded.
> +		 */
> +		err = q->ops->suspend(q);
> +		if (err)
> +			goto err_suspend;
> +
> +		WRITE_ONCE(q->lr.suspended, true);
>  		err = q->ops->suspend_wait(q);
>  		if (err)
>  			goto err_suspend;
> @@ -216,8 +236,20 @@ static int xe_hw_engine_group_suspend_faulting_lr_jobs(struct xe_hw_engine_group
>  			return -EAGAIN;
>  
>  		xe_gt_stats_incr(q->gt, XE_GT_STATS_ID_HW_ENGINE_GROUP_SUSPEND_LR_QUEUE_COUNT, 1);
> +		/*
> +		 * suspend() can fail (e.g. killed/banned/wedged), leaving the
> +		 * queue un-suspended. Propagate the failure, but first undo the
> +		 * partial suspend by resuming the sibling queues already
> +		 * suspended in this call (see err_resume). Record per-queue that
> +		 * the suspend succeeded so only those queues are later waited on
> +		 * and resumed.
> +		 */
> +		err = q->ops->suspend(q);
> +		if (err)
> +			goto err_resume;


This is not right. I think what you want here is:

err = q->ops->suspend(q);
if (err)
        continue;

A killed, banned, or wedged job should not abort the entire suspend
flow and propagate an error back through the user IOCTL. The "group" is
a cross-process concept (i.e., a global concept), so a queue tearing
down in one process should not affect submissions from another process.

I'm pretty sure you could trivially cause a compositor exec IOCTL to
fail by running applications that page-fault and hang, or by killing
them with Ctrl-C. Under the right conditions, the compositor could then
see an error from its exec IOCTL while trying to render a frame.

> +
> +		WRITE_ONCE(q->lr.suspended, true);
>  		need_resume = true;
> -		q->ops->suspend(q);
>  		gt = q->gt;
>  	}
>  
> @@ -225,9 +257,13 @@ static int xe_hw_engine_group_suspend_faulting_lr_jobs(struct xe_hw_engine_group
>  		if (!xe_vm_in_fault_mode(q->vm))
>  			continue;
>  
> +		/* Only wait on queues that were actually suspended above. */
> +		if (!READ_ONCE(q->lr.suspended))
> +			continue;
> +
>  		err = q->ops->suspend_wait(q);
>  		if (err)
> -			return err;
> +			goto err_resume;
>  	}
>  
>  	if (gt) {
> @@ -240,6 +276,40 @@ static int xe_hw_engine_group_suspend_faulting_lr_jobs(struct xe_hw_engine_group
>  		xe_hw_engine_group_resume_faulting_lr_jobs(group);
>  
>  	return 0;
> +
> +err_resume:
> +	/*
> +	 * A suspend()/suspend_wait() failed partway through the mode switch.
> +	 * Resume the sibling queues that were already suspended in this call so
> +	 * they are not left suspended forever.
> +	 *
> +	 * resume() requires the suspend to have completed (suspend_pending
> +	 * cleared) or it trips the !suspend_pending assert. So skip the resume
> +	 * when either:
> +	 *  - suspend_wait() fails: the suspend did not complete (timeout, VF

VF recovery is a non-issue given page faults are not enabled on VFs which
can migrate. Don't bring that up here as if that needs to be handled
this code would have to look different, or mentiond VF recovery doesn't
need to be considered.

> +	 *    recovery, interrupt), so suspend_pending may still be set; or

IRQs are a valid concern and this part doesn't look right.

> +	 *  - reset_status() is true: the queue was reset/killed/banned/wedged.
> +	 *    suspend_wait() can return success in this case via its killed/
> +	 *    stopped wait condition while suspend_pending is still set, and the
> +	 *    queue is being torn down anyway, so its state is resolved by
> +	 *    teardown rather than by a resume here.
> +	 * In either case leave the queue marked suspended.
> +	 */
> +	list_for_each_entry(q, &group->exec_queue_list, hw_engine_group_link) {
> +		if (!xe_vm_in_fault_mode(q->vm))
> +			continue;
> +
> +		if (!READ_ONCE(q->lr.suspended))
> +			continue;
> +
> +		if (q->ops->suspend_wait(q) || q->ops->reset_status(q))

-ERESTARTSYS on this process doesn't mean a different processes queues
are bad. I believe what we need to here is suspend_wait_no_irq(q) to
recover any suspended queues back to the resume state. Yes, ctrl-c /
process kill will have block but I don't see any other option to
maintain balance.

So I'd write this like:

if (q->ops->reset_status(q))
	continue;

q->ops->suspend_wait_no_irq(q);

Or feel free to a no_irq argument to suspend_wait.

Matt

> +			continue;
> +
> +		WRITE_ONCE(q->lr.suspended, false);
> +		q->ops->resume(q);
> +	}
> +
> +	return err;
>  }
>  
>  /**
> -- 
> 2.43.0
>