Re: [PATCH v2 3/5] drm/xe/guc: wait killably for suspend and ban queue on timeout

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

On Tue, Jun 30, 2026 at 10:07:31PM -0700, Niranjana Vishwanathapura wrote:
> Harden guc_exec_queue_suspend_wait():
> 
>  - Wait killably rather than interruptibly. Once a suspend has been
>    issued it must be waited to completion (or timeout); an arbitrary
>    non-fatal signal must not abandon an in-flight suspend, otherwise the
>    wait reports a spurious failure while the suspend is still pending.
>    Only a fatal signal aborts, in which case the dying task tears the
>    queue down (clearing suspend_pending), so no stuck state persists.
> 
>  - On timeout, ban the queue and trigger cleanup rather than leaving it
>    suspended forever. Clearing suspend_pending via __suspend_fence_signal()
>    lets a subsequent resume() proceed without tripping the
>    !suspend_pending assert.
> 
> v2: Add comment about -ERESTARTSYS in suspend_wait
> 

This doesn't actually help per my comments in patch #2 given sigkill on
one process doesn't mean another processes queues are being torn down.

I'd drop this patch as I dont see this buying us anything.

Matt

> Assisted-by: Github-Copilot:Claude-opus-4.8
> Signed-off-by: Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_guc_submit.c | 39 ++++++++++++++++++++++++------
>  1 file changed, 32 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
> index 9458bf477fa6..3d9bdc22c89f 100644
> --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
> @@ -2195,22 +2195,41 @@ static int guc_exec_queue_suspend_wait(struct xe_exec_queue *q)
>  	 xe_guc_read_stopped(guc))
>  
>  retry:
> +	/*
> +	 * Wait killably rather than interruptibly: once a suspend has been
> +	 * issued it must be waited to completion (or timeout), otherwise an
> +	 * arbitrary (non-fatal) signal would abandon an in-flight suspend and
> +	 * the wait would report a spurious failure while suspend_pending is
> +	 * still set. Only a fatal signal aborts here; in that case the dying
> +	 * task tears the queue down (clearing suspend_pending), so no stuck
> +	 * state persists.
> +	 */
>  	if (IS_SRIOV_VF(xe))
> -		ret = wait_event_interruptible_timeout(guc->ct.wq, WAIT_COND ||
> -						       vf_recovery(guc),
> -						       HZ * 5);
> +		ret = wait_event_killable_timeout(guc->ct.wq, WAIT_COND ||
> +						  vf_recovery(guc), HZ * 5);
>  	else
> -		ret = wait_event_interruptible_timeout(q->guc->suspend_wait,
> -						       WAIT_COND, HZ * 5);
> +		ret = wait_event_killable_timeout(q->guc->suspend_wait,
> +						  WAIT_COND, HZ * 5);
>  
>  	if (vf_recovery(guc) && !xe_device_wedged((guc_to_xe(guc))))
>  		return -EAGAIN;
>  
>  	if (!ret) {
>  		xe_gt_warn(guc_to_gt(guc),
> -			   "Suspend fence, guc_id=%d, failed to respond",
> +			   "Suspend fence, guc_id=%d, failed to respond, banning queue",
>  			   q->guc->id);
> -		/* XXX: Trigger GT reset? */
> +		/*
> +		 * The GuC failed to respond to the suspend within the timeout.
> +		 * This is not recoverable for this context, so ban it rather
> +		 * than leave it suspended forever (unmarked). Clearing
> +		 * suspend_pending lets a subsequent resume() proceed without
> +		 * tripping the !suspend_pending assert (the RESUME message is
> +		 * dropped for a banned queue), and triggering cleanup tears the
> +		 * context down.
> +		 */
> +		set_exec_queue_banned(q);
> +		__suspend_fence_signal(q);
> +		xe_guc_exec_queue_trigger_cleanup(q);
>  		return -ETIME;
>  	} else if (IS_SRIOV_VF(xe) && !WAIT_COND) {
>  		/* Corner case on RESFIX DONE where vf_recovery() changes */
> @@ -2219,6 +2238,12 @@ static int guc_exec_queue_suspend_wait(struct xe_exec_queue *q)
>  
>  #undef WAIT_COND
>  
> +	/*
> +	 * ret < 0 (-ERESTARTSYS): aborted by a fatal signal. The queue is not
> +	 * banned - the failure is in the waiter, not the queue. The suspend is
> +	 * not confirmed complete, so suspend_pending may still be set; callers
> +	 * must not resume() on this error without re-confirming the suspend.
> +	 */
>  	return ret < 0 ? ret : 0;
>  }
>  
> -- 
> 2.43.0
>