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

[Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>]

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

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

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