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

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

On Thu, Jul 09, 2026 at 01:36:14PM -0700, Matthew Brost wrote:


> 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
> 

Sorry typing too fast... More below.

> > 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);

Drop this part.

> >  
> >  	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);

This part mostly looks good but __suspend_fence_signal should actually
be signaled in the TDR after queue is off the hardware as without that
we could get a memory corruption signaling suspend fence early while the
hardware could possibly be touching memory.

guc_exec_queue_kill has the same bug which should also be fixed.

Matt 

> >  		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
> >