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 <[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]> > --- > 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 >
