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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/xe: page table teardown races with async GT reset on timeout
--

commit 7c6f890c53c2275179c96ad6d41beb44f5cd78ba
Author: Daniel Charles <daniel.charles@intel.com>

drm/xe: Synchronously wait for queue kill on FD close

This commit splits the FD close teardown into two passes, first killing
every queue, then synchronously waiting for the queue scheduling to stop
by calling xe_exec_queue_kill_sync(). In the GuC backend, this wait uses
a 5-second timeout, after which it triggers an asynchronous GT reset if
the GuC does not respond.

> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> index 2ad5efec2bda1..1c5588eb9676b 100644
> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c
> @@ -190,8 +190,13 @@ static void xe_file_close(struct drm_device *dev, struct drm_file *file)
[ ... ]
> +	xa_for_each(&xef->exec_queue.xa, idx, q) {
> +		xe_exec_queue_kill_sync(q);
>  		xe_exec_queue_put(q);
>  	}
> +
>  	xa_for_each(&xef->vm.xa, idx, vm)
>  		xe_vm_close_and_put(vm);

[Severity: High]
If the queue kill sync times out and triggers an asynchronous GT reset,
this teardown sequence will proceed before the hardware is actually stopped.

Since the GuC and hardware engines may still be actively executing the
context, can tearing down the VM's page tables via xe_vm_close_and_put()
here cause regressions like CAT errors and secondary engine resets?

[ ... ]
> diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
> index 609af66b343d5..353078e0c9e0b 100644
> --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
> @@ -2267,6 +2267,28 @@ static void guc_exec_queue_kill(struct xe_exec_queue *q)
[ ... ]
> +static void guc_exec_queue_kill_sync(struct xe_exec_queue *q)
> +{
[ ... ]
> +	ret = wait_event_timeout(guc->ct.wq,
> +				 (!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), HZ * 5);
> +	if (!ret) {
> +		xe_gt_warn(q->gt, "Queue kill failed to respond, guc_id=%d\n",
> +			   primary->guc->id);
> +		xe_gt_reset_async(q->gt);
> +	}
> +}

[Severity: High]
Does triggering an asynchronous reset here fail to provide the necessary
synchronization guarantee?

By returning immediately while the reset is only queued, the caller
xe_file_close() will proceed to tear down page tables while the hardware
might still be running. Should this wait for the reset to complete to avoid
introducing regressions during cleanup?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013201508.514480-1-daniel.charles@intel.com?part=3