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

[SRINIVASAN SHANMUGAM <srinivasan.shanmugam@amd.com>]

On 9/9/2026 1:52 AM, Matthew Brost wrote:
> 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 <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>
>> ---
>> 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.

Agreed — it has no caller in this patch. Will remove in v9.


>
>> +
>>   /**
>>    * 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?

The cancel was added defensively to prevent the user fence worker from
running after VMA teardown, since the concern was that the callback
could fire during or after xe_vma_destroy_late() and access freed
resources. However, you are correct that this is unnecessary —
xe_ufence_worker only accesses device-level resources (xe->ufence_wq)
and the userspace address via copy_to_user(), and does not touch any
VMA-specific state. So the worker is safe to run after VMA teardown,
exactly as the original code allowed. Will remove the cancel call in
v9, keeping only xe_sync_ufence_put().

Srini