Re: [PATCH 3/3] drm/xe: Synchronously wait for queue kill on FD close

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

On Tue, Oct 13, 2026 at 11:11:48AM -0700, Daniel Charles wrote:
> 
> On 10/12/2026 8:17 PM, Matthew Brost wrote:
> > On Mon, Oct 12, 2026 at 07:34:42PM -0700, Daniel Charles wrote:
> > 
> > Thanks for taking a look here.
> > 
> > > The early KILL message disables GuC scheduling outside the serialized
> > > TDR path, but xe_file_close() does not wait for it to complete before
> > > dropping queue references. GuC may still schedule contexts whose page
> > > tables are being torn down, producing CAT errors and engine resets.
> > > 
> > > Split FD close teardown into two passes: first kill every queue so all
> > > KILL messages are queued in parallel, then call the new
> > > xe_exec_queue_kill_sync() for every queue before dropping references.
> > > 
> > > The GuC backend implementation flushes the message worker while the
> > > static KILL message is pending, then waits for scheduling to be
> > > disabled with no pending enable/disable. The wait also completes when
> > > GuC submission is stopped, VF recovery is in progress, or the queue is
> > > wedged.
> > > 
> > > Wake ct.wq waiters when a scheduling-disable SCHED_DONE is processed so
> > > the waiter is woken as soon as scheduling is disabled.
> > > 
> > > Assisted-by: LLM
> > > Signed-off-by: Daniel Charles <daniel.charles@intel.com>
> > > ---
> > >   drivers/gpu/drm/xe/xe_device.c           |  8 +++++-
> > >   drivers/gpu/drm/xe/xe_exec_queue.c       | 13 ++++++++++
> > >   drivers/gpu/drm/xe/xe_exec_queue.h       |  1 +
> > >   drivers/gpu/drm/xe/xe_exec_queue_types.h |  2 ++
> > >   drivers/gpu/drm/xe/xe_guc_submit.c       | 32 ++++++++++++++++++++++++
> > >   5 files changed, 55 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> > > index 8392c9fad3c48..ffee8951a7b4e 100644
> > > --- a/drivers/gpu/drm/xe/xe_device.c
> > > +++ b/drivers/gpu/drm/xe/xe_device.c
> > > @@ -190,8 +190,14 @@ static void xe_file_close(struct drm_device *dev, struct drm_file *file)
> > >   		if (q->vm && q->hwe->hw_engine_group)
> > >   			xe_hw_engine_group_del_exec_queue(q->hwe->hw_engine_group, q);
> > >   		xe_exec_queue_kill(q);
> > > -		xe_exec_queue_put(q);
> > >   	}
> > > +
> > > +	xa_for_each(&xef->exec_queue.xa, idx, q)
> > > +		xe_exec_queue_kill_sync(q);
> > > +
> > > +	xa_for_each(&xef->exec_queue.xa, idx, q)
> > > +		xe_exec_queue_put(q);
> > You can do the put in the same loop as xe_exec_queue_kill_sync.
> Yes, I'll fix this in the next version.
> > 
> > > +
> > >   	xa_for_each(&xef->vm.xa, idx, vm)
> > >   		xe_vm_close_and_put(vm);
> > > diff --git a/drivers/gpu/drm/xe/xe_exec_queue.c b/drivers/gpu/drm/xe/xe_exec_queue.c
> > > index 9a8efa2bf2bfb..8ba2cf45694dd 100644
> > > --- a/drivers/gpu/drm/xe/xe_exec_queue.c
> > > +++ b/drivers/gpu/drm/xe/xe_exec_queue.c
> > > @@ -1622,6 +1622,19 @@ void xe_exec_queue_kill(struct xe_exec_queue *q)
> > >   	xe_vm_remove_compute_exec_queue(q->vm, q);
> > >   }
> > > +/**
> > > + * xe_exec_queue_kill_sync() - Wait for queue scheduling to stop
> > > + * @q: The exec queue
> > > + *
> > > + * This function waits for the backend to finish stopping the queue after
> > > + * xe_exec_queue_kill(). It is safe to call multiple times.
> > > + */
> > > +void xe_exec_queue_kill_sync(struct xe_exec_queue *q)
> > > +{
> > > +	if (q->ops->kill_sync)
> > > +		q->ops->kill_sync(q);
> > > +}
> > > +
> > >   /**
> > >    * xe_exec_queue_destroy_ioctl() - Destroy an exec queue via IOCTL
> > >    * @dev: DRM device
> > > diff --git a/drivers/gpu/drm/xe/xe_exec_queue.h b/drivers/gpu/drm/xe/xe_exec_queue.h
> > > index e8963f85cabd9..4be023b9a4c6c 100644
> > > --- a/drivers/gpu/drm/xe/xe_exec_queue.h
> > > +++ b/drivers/gpu/drm/xe/xe_exec_queue.h
> > > @@ -119,6 +119,7 @@ bool xe_exec_queue_is_lr(struct xe_exec_queue *q);
> > >   bool xe_exec_queue_is_idle(struct xe_exec_queue *q, int extra_jobs);
> > >   void xe_exec_queue_kill(struct xe_exec_queue *q);
> > > +void xe_exec_queue_kill_sync(struct xe_exec_queue *q);
> > >   int xe_exec_queue_create_ioctl(struct drm_device *dev, void *data,
> > >   			       struct drm_file *file);
> > > diff --git a/drivers/gpu/drm/xe/xe_exec_queue_types.h b/drivers/gpu/drm/xe/xe_exec_queue_types.h
> > > index 48c05398b0168..4882eeccb9bd5 100644
> > > --- a/drivers/gpu/drm/xe/xe_exec_queue_types.h
> > > +++ b/drivers/gpu/drm/xe/xe_exec_queue_types.h
> > > @@ -294,6 +294,8 @@ struct xe_exec_queue_ops {
> > >   	int (*init)(struct xe_exec_queue *q);
> > >   	/** @kill: Kill inflight submissions for backend */
> > >   	void (*kill)(struct xe_exec_queue *q);
> > > +	/** @kill_sync: Wait for the backend to stop queue scheduling */
> > > +	void (*kill_sync)(struct xe_exec_queue *q);
> > >   	/** @fini: Undoes the init() for submission backend */
> > >   	void (*fini)(struct xe_exec_queue *q);
> > >   	/**
> > > diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
> > > index 609af66b343d5..36c8c3766f5fa 100644
> > > --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> > > +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
> > > @@ -2267,6 +2267,34 @@ static void guc_exec_queue_kill(struct xe_exec_queue *q)
> > >   	xe_guc_exec_queue_trigger_cleanup(q);
> > >   }
> > > +static void guc_exec_queue_kill_sync(struct xe_exec_queue *q)
> > > +{
> > > +	struct xe_exec_queue *primary = xe_exec_queue_multi_queue_primary(q);
> > > +	struct xe_guc *guc = exec_queue_to_guc(primary);
> > > +	struct xe_gpu_scheduler *sched = &q->guc->sched;
> > > +	struct xe_sched_msg *msg = q->guc->static_msgs + STATIC_MSG_KILL;
> > > +	bool pending;
> > > +
> > > +	if (!exec_queue_killed(q))
> > xe_assert(xe, exec_queue_killed(q))
> > 
> > ^^^ This is a caller invarient.
> To be fixed in next version.
> > 
> > > +		return;
> > > +
> > > +	do {
> > > +		flush_work(&sched->work_process_msg);
> > > +
> > > +		xe_sched_msg_lock(sched);
> > > +		pending = !list_empty(&msg->link);
> > > +		xe_sched_msg_unlock(sched);
> > > +	} while (pending && !drm_sched_is_stopped(&sched->base));
> > I'm honestly not quite following the logic above, and I don't think this
> > is necessary. At this point, we've already queued a kill message. It will
> > eventually run, or the TDR will fire, or a GT reset will occur. Any of
> > these paths should eventually clear the flags below.
> > 
> > I can see that this was LLM-assisted, and it may well be smarter than me
> > here, but I don't understand why this change is needed.
> 
> The loop was intended to ensure all queued KILL messages had been
> processed before tear down continued. As you point out, those
> messages should be handled through normal processing, TDR, or GT reset.
> I’ll remove the flush_work()/pending-message loop and bound the
> scheduling-state wait with wait_event_timeout().
> 
> > 
> > > +
> > > +	wait_event(guc->ct.wq,
> > wait_event_timeout
> > 
> > A GT reset saves on the GuC taking a nap but that is unbounded,
> > wait_event_timeout gives us a bound.
> What is the best action when this wait times out? We have no confirmation
> that GuC has stopped scheduling, yet xe_file_close() will continue
> tearing down the VM. Is it acceptable to continue teardown, or should
> timeout
> trigger GT reset recovery? If so, should we queue it with
> xe_gt_reset_async()
> or wait for it to complete with xe_gt_reset() before proceeding?
> 

I'd just queue a xe_gt_reset_async, no need to wait as things have gone
badly at this point.

Matt

> -- 
> Daniel.
> > 
> > Matt
> > 
> > > +		   (!exec_queue_enabled(primary) &&
> > > +		    !exec_queue_pending_disable(primary) &&
> > > +		    !exec_queue_pending_enable(primary)) ||
> > > +		   xe_guc_read_stopped(guc) ||
> > > +		   vf_recovery(guc) ||
> > > +		   exec_queue_wedged(primary));
> > > +}
> > > +
> > >   static void guc_exec_queue_add_msg(struct xe_exec_queue *q, struct xe_sched_msg *msg,
> > >   				   u32 opcode)
> > >   {
> > > @@ -2744,6 +2772,7 @@ static u64 guc_exec_queue_reset_status(struct xe_exec_queue *q)
> > >   static const struct xe_exec_queue_ops guc_exec_queue_ops = {
> > >   	.init = guc_exec_queue_init,
> > >   	.kill = guc_exec_queue_kill,
> > > +	.kill_sync = guc_exec_queue_kill_sync,
> > >   	.fini = guc_exec_queue_fini,
> > >   	.destroy = guc_exec_queue_destroy,
> > >   	.set_priority = guc_exec_queue_set_priority,
> > > @@ -3423,6 +3452,9 @@ static void handle_sched_done(struct xe_guc *guc, struct xe_exec_queue *q,
> > >   				clear_exec_queue_pending_disable(q);
> > >   			}
> > >   		}
> > > +		/* Publish the disabled state before waking kill_sync waiters. */
> > > +		smp_wmb();
> > > +		wake_up_all(&guc->ct.wq);
> > >   	}
> > >   }
> > > -- 
> > > 2.43.0
> > >