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

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

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.

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

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

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

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
>