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 09:30:30PM -0700, Niranjana Vishwanathapura wrote:
> On Thu, Jul 09, 2026 at 01:40:49PM -0700, Matthew Brost wrote:
> > 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.
> > 
> 
> wait_event_interruptible_timeout() will get woken up for user signals (SIGINT)
> also with -ERESTARTSYS as return value and I am not sure if it is ok to treat that
> as an error condition without a retry. The wait_event_killable_timeout() only gets
> woken for fatal signals (and not SIGINT).
> 

My point is if you can make sigkill work, you get sigint for free as to
the upper layers the handling is the same, so might as leave this as is.

> > > >
> > > >  	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.
> > 
> 
> Ok. It seems there are 2 issues here, but both of them also apply to
> existing cases like guc_exec_queue_kill().
> 
> 1. The one you mentioned above. It seems the fix will be bit more involved
> as suspend_wait() returns immediately if the queue is killed before TDR can
> run and take the job off of HW. So, caller of suspend_wait() must handle it
> somehow and not trigger any fences.
> 
> 2. handle_sched_done()/guc_exec_queue_stop() reading suspend_pending and calling
> suspend_fence_signal() might race against guc_exec_queue_kill()/suspend_wait()
> clearing the suspend_pending. This might lead to hitting suspend_pending assert
> in suspend_fence_signal(). Some some kind of locking is required here. Probably
> xe_sched_msg_lock() will do here.
> 
> But both of these cases goes beyond this patch series. Is it ok if we can take
> it separately and address it in a later patch series?

No problem fixing up this part of later. I was going to rework the
guc_exec_queue_kill() logic to avoid faults on FD close and fix up
guc_exec_queue_kill() then and if this merged will also fix this part
up.

Matt

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