Re: [PATCH v2 1/5] drm/xe: only resume exec queues that were actually suspended

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

On Tue, Jun 30, 2026 at 10:07:29PM -0700, Niranjana Vishwanathapura wrote:
> A consumer-issued suspend() can fail (e.g. the queue is killed, banned
> or wedged), leaving the queue un-suspended. The consumer must then not
> issue the matching resume(): resuming a queue that was never suspended
> is incorrect.
> 
> Add an lr.suspended flag to struct xe_exec_queue that records whether a
> consumer suspend() succeeded and a matching resume() is still owed. Set
> it on a successful suspend() in the preempt-fence path, clear it on
> resume(), and only resume queues that have it set.
> 
> In resume_and_reinstall_preempt_fences() also skip queues that have
> since been reset/killed/banned/wedged: such a queue's suspend may not
> have completed (suspend_pending can still be set, e.g. a preempt fence
> signalled with -ENOENT without waiting), so resuming it would trip the
> !suspend_pending assert in the backend. Leave it marked suspended and
> let teardown resolve its state.
> 
> A queue is only ever suspended by a single consumer at a time
> (preempt-fence mode and hw engine group fault mode are mutually
> exclusive), so a single flag is sufficient.
> 
> Assisted-by: Github-Copilot:Claude-opus-4.8
> Signed-off-by: Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>

Reviewed-by: Matthew Brost <matthew.brost@intel.com>

> ---
>  drivers/gpu/drm/xe/xe_exec_queue_types.h | 12 ++++++++++++
>  drivers/gpu/drm/xe/xe_preempt_fence.c    |  7 +++++++
>  drivers/gpu/drm/xe/xe_vm.c               | 18 +++++++++++++++++-
>  3 files changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_exec_queue_types.h b/drivers/gpu/drm/xe/xe_exec_queue_types.h
> index d27ce24daae5..dbb2ee8eb5de 100644
> --- a/drivers/gpu/drm/xe/xe_exec_queue_types.h
> +++ b/drivers/gpu/drm/xe/xe_exec_queue_types.h
> @@ -200,6 +200,18 @@ struct xe_exec_queue {
>  		u32 seqno;
>  		/** @lr.link: link into VM's list of exec queues */
>  		struct list_head link;
> +		/**
> +		 * @lr.suspended: Tracks whether the consumer-issued suspend()
> +		 * succeeded and a matching resume() is still owed. suspend() can
> +		 * fail (e.g. killed/banned/wedged), leaving the queue
> +		 * un-suspended, so consumers must only resume() queues that were
> +		 * actually suspended. Set by the suspend caller on success and
> +		 * cleared by the resume caller. A queue is only ever suspended by
> +		 * a single consumer at a time (preempt-fence mode and hw engine
> +		 * group fault mode are mutually exclusive), so a single flag is
> +		 * sufficient.
> +		 */
> +		bool suspended;
>  	} lr;
>  
>  #define XE_EXEC_QUEUE_TLB_INVAL_PRIMARY_GT	0
> diff --git a/drivers/gpu/drm/xe/xe_preempt_fence.c b/drivers/gpu/drm/xe/xe_preempt_fence.c
> index d6427b473ddd..4aa570fe745d 100644
> --- a/drivers/gpu/drm/xe/xe_preempt_fence.c
> +++ b/drivers/gpu/drm/xe/xe_preempt_fence.c
> @@ -74,6 +74,13 @@ static bool preempt_fence_enable_signaling(struct dma_fence *fence)
>  	struct xe_exec_queue *q = pfence->q;
>  
>  	pfence->error = q->ops->suspend(q);
> +	/*
> +	 * Record a successful suspend so the rebind worker only resumes queues
> +	 * that were actually suspended; a failed suspend() leaves the queue
> +	 * un-suspended and must not be paired with a resume().
> +	 */
> +	if (!pfence->error)
> +		WRITE_ONCE(q->lr.suspended, true);
>  	queue_work(q->vm->xe->preempt_fence_wq, &pfence->preempt_work);
>  	return true;
>  }
> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index 080c2fff0e95..23f4a9fb9a49 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c
> @@ -206,7 +206,23 @@ static void resume_and_reinstall_preempt_fences(struct xe_vm *vm,
>  	xe_vm_assert_held(vm);
>  
>  	list_for_each_entry(q, &vm->preempt.exec_queues, lr.link) {
> -		q->ops->resume(q);
> +		/*
> +		 * Only resume queues whose suspend() actually succeeded. A
> +		 * failed suspend() (e.g. killed/banned/wedged) leaves the queue
> +		 * un-suspended, so it must not be resumed.
> +		 *
> +		 * Also skip queues that have since been reset/killed/banned/
> +		 * wedged: their suspend may not have completed (suspend_pending
> +		 * can still be set, e.g. a preempt fence signalled with -ENOENT
> +		 * without waiting), so resuming would trip the !suspend_pending
> +		 * assert in the backend. Such queues are being torn down anyway,
> +		 * so leave them marked suspended and let teardown resolve their
> +		 * state.
> +		 */
> +		if (READ_ONCE(q->lr.suspended) && !q->ops->reset_status(q)) {
> +			WRITE_ONCE(q->lr.suspended, false);
> +			q->ops->resume(q);
> +		}
>  
>  		drm_gpuvm_resv_add_fence(&vm->gpuvm, exec, q->lr.pfence,
>  					 DMA_RESV_USAGE_BOOKKEEP, DMA_RESV_USAGE_BOOKKEEP);
> -- 
> 2.43.0
>