On Tue, Sep 08, 2026 at 02:16:27PM +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.writeback and
> xe_ufence_ops.destroy, which are called by the drm_user_fence helper.
>
> Cc: Matthew Brost <[email protected]>
> Cc: Mika Kuoppala <[email protected]>
> Cc: Thomas Hellström <[email protected]>
> Cc: Maarten Lankhorst <[email protected]>
> Cc: Christian König <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Srinivasan Shanmugam <[email protected]>
> ---
> v8:
> - Fix xe_ufence_worker ordering: WRITE_ONCE(signalled, 1) before
> copy_to_user(), add smp_wmb() before wake_up_all() (Matt,
> mirrors upstream fix 8ae04fe9ffc93)
> - Rename .worker → .writeback in xe_ufence_ops (Matt)
> - Update commit message: xe_ufence_ops.writeback (not .worker)
>
> drivers/gpu/drm/xe/xe_sync.c | 154 ++++++++++++++++-------------
> 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, 89 insertions(+), 69 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_sync.c b/drivers/gpu/drm/xe/xe_sync.c
> index 37866768d64c..c0e720cf579a 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,65 @@
> #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);
> -}
> + /*
> + * Mark signalled before the userspace write so that if userspace
> + * observes the written value and immediately issues a VM unmap,
> + * check_ufence() will see signalled=1 and not return -EBUSY.
> + *
> + * NOTE: copy_to_user() may block if userspace backs the address with
> + * userfaultfd or FUSE. Drivers requiring non-blocking semantics should
> + * use a dedicated workqueue instead of xe->ordered_wq.
> + */
> + WRITE_ONCE(ufence->signalled, 1);
>
> -static void user_fence_get(struct xe_user_fence *ufence)
> -{
> - kref_get(&ufence->refcount);
> + 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");
> + }
> +
> + /*
> + * Ensure both signalled=1 and the fence value written by copy_to_user()
> + * are visible to all CPUs before wake_up_all() wakes waiters. Without
> + * this barrier, weakly ordered architectures (e.g. ARM64) may allow
> + * waiters to observe the wakeup before seeing the updated values.
> + */
> + smp_wmb();
> + 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 = {
> + .writeback = 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 +91,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 +281,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 +424,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);
> +}
This is unused in this patch.
> +
> /**
> * 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);
This is a behavior change in this patch. Why is this change?
Also can't this prevent a user fence signaling on unbind if
xe_vma_destroy_late() executes before the user fence? I'm somewhat
suprised our CI didn't fail because of this change or Sashiko didn't
complain about this, so maybe I'm missing something.
Matt
> xe_sync_ufence_put(vma->ufence);
> vma->ufence = NULL;
> }
> --
> 2.34.1
>
