Re: [PATCH v7 3/4] drm/xe: Convert xe_user_fence to drm_user_fence

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

On Wed, Sep 02, 2026 at 08:50:48PM +0530, Srinivasan Shanmugam wrote:
> Replace the open-coded user fence implementation in xe_sync.c with the
> new common drm_user_fence helper.
> 
> struct xe_user_fence now embeds struct drm_user_fence as its base.
> XE-specific fields (xe_device pointer for the ufence_wq wake-up,
> userspace VA, expected value, signalled flag) remain in the wrapper.
> 
> The local user_fence_destroy/get/put/worker/kick_ufence/user_fence_cb
> functions are removed. Their logic moves to xe_ufence_ops.worker and
> xe_ufence_ops.destroy, which are called by the drm_user_fence helper.
> 
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: dri-devel@lists.freedesktop.org
> Cc: intel-xe@lists.freedesktop.org
> Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam@amd.com>
> ---
>  drivers/gpu/drm/xe/xe_sync.c       | 147 ++++++++++++++++-------------
>  drivers/gpu/drm/xe/xe_sync.h       |   2 +
>  drivers/gpu/drm/xe/xe_sync_types.h |   1 -
>  drivers/gpu/drm/xe/xe_vm.c         |   1 +
>  4 files changed, 82 insertions(+), 69 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_sync.c b/drivers/gpu/drm/xe/xe_sync.c
> index 37866768d64c..05f6794af1ee 100644
> --- a/drivers/gpu/drm/xe/xe_sync.c
> +++ b/drivers/gpu/drm/xe/xe_sync.c
> @@ -6,12 +6,11 @@
>  #include "xe_sync.h"
>  
>  #include <linux/dma-fence-array.h>
> -#include <linux/kthread.h>
> -#include <linux/sched/mm.h>
>  #include <linux/uaccess.h>
>  
>  #include <drm/drm_print.h>
>  #include <drm/drm_syncobj.h>
> +#include <drm/drm_user_fence.h>
>  #include <uapi/drm/xe_drm.h>
>  
>  #include "xe_device.h"
> @@ -19,36 +18,58 @@
>  #include "xe_macros.h"
>  #include "xe_sched_job_types.h"
>  
> +/*
> + * xe_user_fence wraps drm_user_fence with XE-specific fields.
> + * The drm_user_fence base handles MM borrowing and work-item lifetime.
> + */
>  struct xe_user_fence {
> -	struct xe_device *xe;
> -	struct kref refcount;
> -	struct dma_fence_cb cb;
> -	struct work_struct worker;
> -	struct mm_struct *mm;
> -	u64 __user *addr;
> -	u64 value;
> -	int signalled;
> +	struct drm_user_fence base;
> +	struct xe_device     *xe;
> +	u64 __user           *addr;
> +	u64                   value;
> +	int                   signalled;
>  };
>  
> -static void user_fence_destroy(struct kref *kref)
> +static void xe_ufence_worker(struct drm_user_fence *base, bool mm_ok)
>  {
> -	struct xe_user_fence *ufence = container_of(kref, struct xe_user_fence,
> -						 refcount);
> +	struct xe_user_fence *ufence = container_of(base, struct xe_user_fence, base);
>  
> -	mmdrop(ufence->mm);
> -	kfree(ufence);
> -}
> +	if (mm_ok) {
> +		if (copy_to_user(ufence->addr, &ufence->value, sizeof(ufence->value)))
> +			drm_dbg(&ufence->xe->drm,
> +				"copy_to_user failed, user fence wasn't signaled\n");
> +	} else {
> +		drm_dbg(&ufence->xe->drm,
> +			"mmget_not_zero() failed, ufence wasn't signaled\n");
> +	}
>  
> -static void user_fence_get(struct xe_user_fence *ufence)
> -{
> -	kref_get(&ufence->refcount);
> +	/*
> +	 * Ensure the fence value write is visible before signalled=1.
> +	 * A UMD polling signalled must see the committed fence value.
> +	 */
> +	smp_wmb();
> +
> +	/*
> +	 * Mark signalled after the user memory write so UMD can safely
> +	 * reuse the same ufence without hitting -EBUSY.
> +	 */
> +	WRITE_ONCE(ufence->signalled, 1);

Sashiko [1] is complaining about the order inversion between copy_to_user
and WRITE_ONCE is correct.

See for more details:
git format-patch -1 8ae04fe9ffc93

The order should be WRITE_ONCE, then copy_to_user. Adding the smp_wmb()
before wake_up_all is actually probably right though and could explain a
random CI failure we seeing related to user fences.

Matt

[1] https://sashiko.dev/#/patchset/20260902152049.183685-1-srinivasan.shanmugam%40amd.com

> +
> +	wake_up_all(&ufence->xe->ufence_wq);
>  }
>  
> -static void user_fence_put(struct xe_user_fence *ufence)
> +static void xe_ufence_destroy(struct drm_user_fence *base)
>  {
> -	kref_put(&ufence->refcount, user_fence_destroy);
> +	struct xe_user_fence *ufence = container_of(base, struct xe_user_fence, base);
> +
> +	kfree(ufence);
>  }
>  
> +static const struct drm_user_fence_ops xe_ufence_ops = {
> +	.worker  = xe_ufence_worker,
> +	.destroy = xe_ufence_destroy,
> +};
> +
>  static struct xe_user_fence *user_fence_create(struct xe_device *xe, u64 addr,
>  					       u64 value)
>  {
> @@ -63,51 +84,22 @@ static struct xe_user_fence *user_fence_create(struct xe_device *xe, u64 addr,
>  	if (!ufence)
>  		return ERR_PTR(-ENOMEM);
>  
> -	ufence->xe = xe;
> -	kref_init(&ufence->refcount);
> -	ufence->addr = ptr;
> +	ufence->xe    = xe;
> +	ufence->addr  = ptr;
>  	ufence->value = value;
> -	ufence->mm = current->mm;
> -	mmgrab(ufence->mm);
> +	drm_user_fence_init(&ufence->base, xe->ordered_wq, &xe_ufence_ops);
>  
>  	return ufence;
>  }
>  
> -static void user_fence_worker(struct work_struct *w)
> -{
> -	struct xe_user_fence *ufence = container_of(w, struct xe_user_fence, worker);
> -
> -	WRITE_ONCE(ufence->signalled, 1);
> -	if (mmget_not_zero(ufence->mm)) {
> -		kthread_use_mm(ufence->mm);
> -		if (copy_to_user(ufence->addr, &ufence->value, sizeof(ufence->value)))
> -			XE_WARN_ON("Copy to user failed");
> -		kthread_unuse_mm(ufence->mm);
> -		mmput(ufence->mm);
> -	} else {
> -		drm_dbg(&ufence->xe->drm, "mmget_not_zero() failed, ufence wasn't signaled\n");
> -	}
> -
> -	/*
> -	 * Wake up waiters only after updating the ufence state, allowing the UMD
> -	 * to safely reuse the same ufence without encountering -EBUSY errors.
> -	 */
> -	wake_up_all(&ufence->xe->ufence_wq);
> -	user_fence_put(ufence);
> -}
> -
> -static void kick_ufence(struct xe_user_fence *ufence, struct dma_fence *fence)
> +static void user_fence_get(struct xe_user_fence *ufence)
>  {
> -	INIT_WORK(&ufence->worker, user_fence_worker);
> -	queue_work(ufence->xe->ordered_wq, &ufence->worker);
> -	dma_fence_put(fence);
> +	drm_user_fence_get(&ufence->base);
>  }
>  
> -static void user_fence_cb(struct dma_fence *fence, struct dma_fence_cb *cb)
> +static void user_fence_put(struct xe_user_fence *ufence)
>  {
> -	struct xe_user_fence *ufence = container_of(cb, struct xe_user_fence, cb);
> -
> -	kick_ufence(ufence, fence);
> +	drm_user_fence_put(&ufence->base);
>  }
>  
>  int xe_sync_entry_parse(struct xe_device *xe, struct xe_file *xef,
> @@ -282,24 +274,15 @@ void xe_sync_entry_signal(struct xe_sync_entry *sync, struct dma_fence *fence)
>  	} else if (sync->syncobj) {
>  		drm_syncobj_replace_fence(sync->syncobj, fence);
>  	} else if (sync->ufence) {
> -		int err;
> -
>  		drm_syncobj_add_point(sync->ufence_syncobj,
>  				      sync->ufence_chain_fence,
>  				      fence, sync->ufence_timeline_value);
>  		sync->ufence_chain_fence = NULL;
>  
>  		fence = drm_syncobj_fence_get(sync->ufence_syncobj);
> -		user_fence_get(sync->ufence);
> -		err = dma_fence_add_callback(fence, &sync->ufence->cb,
> -					     user_fence_cb);
> -		if (err == -ENOENT) {
> -			kick_ufence(sync->ufence, fence);
> -		} else if (err) {
> +		if (drm_user_fence_add_callback(&sync->ufence->base, fence))
>  			XE_WARN_ON("failed to add user fence");
> -			user_fence_put(sync->ufence);
> -			dma_fence_put(fence);
> -		}
> +		/* fence ref consumed by drm_user_fence_add_callback */
>  	}
>  }
>  
> @@ -434,6 +417,34 @@ void xe_sync_ufence_put(struct xe_user_fence *ufence)
>  	user_fence_put(ufence);
>  }
>  
> +/**
> + * xe_sync_ufence_cancel() - Non-blocking cancel of user fence callback
> + * @ufence: user fence reference
> + *
> + * Attempts to cancel the pending callback without waiting for the worker.
> + * Safe to call while holding dma_resv_lock or vm->lock. If the callback
> + * has already fired, the worker runs independently — xe_ufence_worker
> + * only accesses device-level and userspace resources, both safe after
> + * VMA teardown.
> + */
> +void xe_sync_ufence_cancel(struct xe_user_fence *ufence)
> +{
> +	drm_user_fence_cancel(&ufence->base);
> +}
> +
> +/**
> + * xe_sync_ufence_cancel_sync() - Cancel user fence callback and wait for worker
> + * @ufence: user fence reference
> + *
> + * Cancels any pending dma-fence callback and waits for the worker to fully
> + * complete before returning. Must be called during teardown before freeing
> + * any resources accessed by the worker.
> + */
> +void xe_sync_ufence_cancel_sync(struct xe_user_fence *ufence)
> +{
> +	drm_user_fence_cancel_sync(&ufence->base);
> +}
> +
>  /**
>   * xe_sync_ufence_get_status() - Get user fence status
>   * @ufence: user fence
> diff --git a/drivers/gpu/drm/xe/xe_sync.h b/drivers/gpu/drm/xe/xe_sync.h
> index 6b949194acff..b5ff3387c592 100644
> --- a/drivers/gpu/drm/xe/xe_sync.h
> +++ b/drivers/gpu/drm/xe/xe_sync.h
> @@ -44,6 +44,8 @@ static inline bool xe_sync_is_ufence(struct xe_sync_entry *sync)
>  struct xe_user_fence *__xe_sync_ufence_get(struct xe_user_fence *ufence);
>  struct xe_user_fence *xe_sync_ufence_get(struct xe_sync_entry *sync);
>  void xe_sync_ufence_put(struct xe_user_fence *ufence);
> +void xe_sync_ufence_cancel(struct xe_user_fence *ufence);
> +void xe_sync_ufence_cancel_sync(struct xe_user_fence *ufence);
>  int xe_sync_ufence_get_status(struct xe_user_fence *ufence);
>  
>  #endif
> diff --git a/drivers/gpu/drm/xe/xe_sync_types.h b/drivers/gpu/drm/xe/xe_sync_types.h
> index b88f1833e28c..67048199b070 100644
> --- a/drivers/gpu/drm/xe/xe_sync_types.h
> +++ b/drivers/gpu/drm/xe/xe_sync_types.h
> @@ -12,7 +12,6 @@ struct drm_syncobj;
>  struct dma_fence;
>  struct dma_fence_chain;
>  struct drm_xe_sync;
> -struct user_fence;
>  
>  struct xe_sync_entry {
>  	struct drm_syncobj *syncobj;
> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index b01f31ed4417..fbd2c28de8a6 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c
> @@ -1181,6 +1181,7 @@ static void xe_vma_destroy_late(struct xe_vma *vma)
>  	struct xe_bo *bo = xe_vma_bo(vma);
>  
>  	if (vma->ufence) {
> +		xe_sync_ufence_cancel(vma->ufence);
>  		xe_sync_ufence_put(vma->ufence);
>  		vma->ufence = NULL;
>  	}
> -- 
> 2.34.1
>