Re: [PATCH v4 1/2] drm: Add common drm_user_fence helper

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

On Fri, Aug 28, 2026 at 12:01:02PM +0530, Srinivasan Shanmugam wrote:
> Introduce a common DRM user fence helper providing the kref-managed,
> MM-borrowing dma-fence-callback-to-workqueue pattern used by drivers
> that must access userspace memory from a kthread context when a GPU
> fence signals.
> 
> XE uses this pattern (xe_sync.c) to write a fence completion value
> to a userspace VA. AMDGPU will use the same pattern to signal a
> per-queue eventfd from a user-queue EOP fence callback.
> 
> The helper provides:
>  - struct drm_user_fence: embeddable base structure
>  - struct drm_user_fence_ops: worker/destroy callbacks
>  - drm_user_fence_init(): initialize and grab the process MM
>  - drm_user_fence_get/put(): reference counting
>  - drm_user_fence_add_callback(): attach to a dma-fence
> 
> The worker callback receives a bool indicating whether the process
> MM was successfully obtained, allowing drivers to handle the
> unavailable-MM case (log, skip the userspace write, etc.) without
> duplicating the mmget/kthread_use_mm/mmput boilerplate.
> 
> Suggested-by: Christian König <christian.koenig@amd.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: David Airlie <airlied@gmail.com>
> Cc: Simona Vetter <simona@ffwll.ch>
> Cc: Sumit Semwal <sumit.semwal@linaro.org>
> Cc: Matthew Brost <matthew.brost@intel.com>

First off, I'm supportive of the idea of a common DRM layer for user
fences and updating Xe accordingly.

This isn't a complete review, but here's a quick initial suggestion.

Also, by the way, you're still fighting our CI [1]. Feel free to keep
hammering on it, as that's what it's there for. iirc if kunit fails as
in this case, nothing else will run. Ask AI and should be able to get
instructions on how to build our kunit + run it (it doesn't require
Intel hardware in a lot of cases).

[1] https://patchwork.freedesktop.org/series/172930/

> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> Cc: dri-devel@lists.freedesktop.org
> Cc: intel-xe@lists.freedesktop.org
> Cc: linux-media@vger.kernel.org
> Cc: linaro-mm-sig@lists.linaro.org
> Cc: linux-kernel@vger.kernel.org
> Cc: amd-gfx@lists.freedesktop.org
> Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam@amd.com>
> ---
> v4:
>  - Check cancel_work_sync() return value in drm_user_fence_cancel_sync()
>    and call drm_user_fence_put() if work was dequeued, fixing a memory
>    leak of the drm_user_fence, mm_struct and stored dma_fence when a
>    pending work item is cancelled. (Sashiko review)
> 
>  drivers/gpu/drm/Makefile         |   1 +
>  drivers/gpu/drm/drm_user_fence.c | 223 +++++++++++++++++++++++++++++++
>  include/drm/drm_user_fence.h     |  76 +++++++++++
>  3 files changed, 300 insertions(+)
>  create mode 100644 drivers/gpu/drm/drm_user_fence.c
>  create mode 100644 include/drm/drm_user_fence.h
> 
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index e97faabcd783..52de1f474535 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -69,6 +69,7 @@ drm-y := \
>  	drm_syncobj.o \
>  	drm_sysfs.o \
>  	drm_trace_points.o \
> +	drm_user_fence.o \
>  	drm_vblank.o \
>  	drm_vblank_work.o \
>  	drm_vma_manager.o \
> diff --git a/drivers/gpu/drm/drm_user_fence.c b/drivers/gpu/drm/drm_user_fence.c
> new file mode 100644
> index 000000000000..cdc47d092cbb
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_user_fence.c
> @@ -0,0 +1,223 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2024 The Linux Foundation
> + *
> + * Common DRM user fence helper.
> + *
> + * When a GPU dma-fence signals, drivers often need to write a value to a
> + * userspace VA or notify userspace via an eventfd. Both operations require
> + * a valid process MM, which is not available in IRQ context.
> + *
> + * This helper queues a work item on fence signal. The work item borrows the
> + * process MM via kthread_use_mm() and calls ops->worker(), which the driver
> + * implements to perform the actual userspace access.
> + */
> +
> +#include <linux/kthread.h>
> +#include <linux/sched/mm.h>
> +#include <linux/workqueue.h>
> +
> +#include <drm/drm_user_fence.h>
> +
> +static void drm_user_fence_destroy(struct kref *kref)
> +{
> +	struct drm_user_fence *ufence =
> +		container_of(kref, struct drm_user_fence, refcount);
> +
> +	/* Release the extra reference stored for cancel() */
> +	if (ufence->fence)
> +		dma_fence_put(ufence->fence);
> +
> +	mmdrop(ufence->mm);
> +	ufence->ops->destroy(ufence);
> +}
> +
> +/**
> + * drm_user_fence_get - Acquire a reference to a user fence
> + * @ufence: user fence
> + */
> +void drm_user_fence_get(struct drm_user_fence *ufence)
> +{
> +	kref_get(&ufence->refcount);
> +}
> +EXPORT_SYMBOL_GPL(drm_user_fence_get);
> +
> +/**
> + * drm_user_fence_put - Release a reference to a user fence
> + * @ufence: user fence
> + */
> +void drm_user_fence_put(struct drm_user_fence *ufence)
> +{
> +	kref_put(&ufence->refcount, drm_user_fence_destroy);
> +}
> +EXPORT_SYMBOL_GPL(drm_user_fence_put);
> +
> +static void drm_user_fence_work(struct work_struct *w)
> +{
> +	struct drm_user_fence *ufence =
> +		container_of(w, struct drm_user_fence, work);
> +	bool mm_ok = false;
> +
> +	if (mmget_not_zero(ufence->mm)) {
> +		kthread_use_mm(ufence->mm);
> +		mm_ok = true;
> +	}
> +
> +	ufence->ops->worker(ufence, mm_ok);
> +
> +	if (mm_ok) {
> +		kthread_unuse_mm(ufence->mm);
> +		mmput(ufence->mm);
> +	}
> +
> +	drm_user_fence_put(ufence);
> +}
> +
> +static void drm_user_fence_cb(struct dma_fence *fence, struct dma_fence_cb *cb)
> +{
> +	struct drm_user_fence *ufence =
> +		container_of(cb, struct drm_user_fence, cb);
> +
> +	queue_work(ufence->wq, &ufence->work);
> +	/*
> +	 * Put the transferred reference from add_callback. The stored
> +	 * reference in ufence->fence is released in drm_user_fence_destroy().
> +	 */
> +	dma_fence_put(fence);
> +}
> +
> +/**
> + * drm_user_fence_init - Initialize a user fence
> + * @ufence: user fence to initialize
> + * @wq: workqueue to run the worker on (must be ordered if sequencing matters)
> + * @ops: driver operations
> + *
> + * Must be called from process context with a valid current->mm.
> + * Grabs a reference to current->mm via mmgrab().
> + */
> +void drm_user_fence_init(struct drm_user_fence *ufence,
> +			 struct workqueue_struct *wq,
> +			 const struct drm_user_fence_ops *ops)
> +{
> +	kref_init(&ufence->refcount);
> +	ufence->mm = current->mm;
> +	mmgrab(ufence->mm);
> +	ufence->wq = wq;
> +	ufence->ops = ops;
> +	ufence->fence = NULL;
> +	INIT_WORK(&ufence->work, drm_user_fence_work);
> +}
> +EXPORT_SYMBOL_GPL(drm_user_fence_init);
> +
> +/**
> + * drm_user_fence_add_callback - Attach a user fence to a dma-fence
> + * @ufence: user fence
> + * @fence: dma-fence to watch; ownership of this reference is transferred
> + *         to the callback — caller must NOT put it afterward.
> + *
> + * When @fence signals, a work item is queued that calls ops->worker() with
> + * the process MM active. If @fence has already signaled the work item is
> + * queued immediately.
> + *
> + * An additional reference to @fence is stored internally in @ufence to
> + * allow drm_user_fence_cancel() to be called safely without the caller
> + * needing to hold a separate fence reference.
> + *
> + * On any return value the caller's fence reference is consumed.
> + *
> + * Return: 0 on success, negative errno on error.
> + */
> +int drm_user_fence_add_callback(struct drm_user_fence *ufence,
> +				struct dma_fence *fence)
> +{
> +	int err;
> +
> +	drm_user_fence_get(ufence);
> +
> +	/* Extra ref stored for cancel() — lives until drm_user_fence_destroy() */
> +	ufence->fence = dma_fence_get(fence);
> +
> +	err = dma_fence_add_callback(fence, &ufence->cb, drm_user_fence_cb);
> +	if (err == -ENOENT) {
> +		/* fence already signaled — queue work and release transferred ref */
> +		queue_work(ufence->wq, &ufence->work);
> +		dma_fence_put(fence);
> +		err = 0;
> +	} else if (err) {
> +		dma_fence_put(ufence->fence);
> +		ufence->fence = NULL;
> +		drm_user_fence_put(ufence);
> +		dma_fence_put(fence);
> +	}
> +	/* on success: transferred ref goes to drm_user_fence_cb */
> +
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(drm_user_fence_add_callback);
> +
> +/**
> + * drm_user_fence_cancel - Cancel a pending user fence callback
> + * @ufence: user fence
> + *
> + * Attempts to remove the pending callback before driver context teardown.
> + * Must be called before the driver tears down its workqueue or ops.
> + * The caller must hold a reference to @ufence across this call.
> + *
> + * If the callback has already fired this returns false and no additional
> + * action is needed — the callback handles its own reference.
> + *
> + * If removal succeeds the callback reference is released internally.
> + * The caller must still release its own separate reference via
> + * drm_user_fence_put() when done with the object.
> + *
> + * This function is safe to call from atomic context as it only acquires
> + * the dma-fence spinlock internally. If the caller also needs to wait
> + * for the worker to finish, use drm_user_fence_cancel_sync() instead,
> + * which may sleep.
> + *
> + * Return: true if callback was removed, false if it had already fired.
> + */
> +bool drm_user_fence_cancel(struct drm_user_fence *ufence)
> +{
> +	struct dma_fence *fence = ufence->fence;
> +
> +	if (!fence)
> +		return false;
> +
> +	if (dma_fence_remove_callback(fence, &ufence->cb)) {
> +		/*
> +		 * Callback will not fire — release the transferred reference
> +		 * that would have been put by drm_user_fence_cb(). The stored
> +		 * reference in ufence->fence is released in destroy().
> +		 */
> +		dma_fence_put(fence);
> +		drm_user_fence_put(ufence);
> +		return true;
> +	}
> +
> +	/* Callback already fired — it handled its own cleanup */
> +	return false;
> +}
> +EXPORT_SYMBOL_GPL(drm_user_fence_cancel);
> +
> +/**
> + * drm_user_fence_cancel_sync - Cancel callback and wait for worker to finish
> + * @ufence: user fence
> + *
> + * Calls drm_user_fence_cancel() then cancel_work_sync() to guarantee
> + * the worker has fully completed before returning.
> + *
> + * This function may sleep. Must not be called from atomic or interrupt
> + * context. Use drm_user_fence_cancel() instead when sleeping is not
> + * allowed.
> + *
> + * Drivers must call this during teardown before freeing any resources
> + * accessed by ops->worker().
> + */
> +void drm_user_fence_cancel_sync(struct drm_user_fence *ufence)
> +{
> +	drm_user_fence_cancel(ufence);
> +	if (cancel_work_sync(&ufence->work))
> +		drm_user_fence_put(ufence);
> +}
> +EXPORT_SYMBOL_GPL(drm_user_fence_cancel_sync);
> diff --git a/include/drm/drm_user_fence.h b/include/drm/drm_user_fence.h
> new file mode 100644
> index 000000000000..02a02266ab93
> --- /dev/null
> +++ b/include/drm/drm_user_fence.h
> @@ -0,0 +1,75 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2024 The Linux Foundation
> + */
> +
> +#ifndef __DRM_USER_FENCE_H__
> +#define __DRM_USER_FENCE_H__
> +
> +#include <linux/dma-fence.h>
> +#include <linux/kref.h>
> +#include <linux/workqueue.h>
> +
> +struct drm_user_fence;
> +
> +/**
> + * struct drm_user_fence_ops - driver callbacks for a DRM user fence
> + */
> +struct drm_user_fence_ops {
> +	/**
> +	 * @worker: Called from workqueue context.
> +	 *
> +	 * If @mm_ok is true, kthread_use_mm() is active and userspace memory
> +	 * (copy_to_user, eventfd_signal, etc.) may be accessed safely.
> +	 * If @mm_ok is false, the process MM was already gone; the driver
> +	 * should log a warning and skip the userspace write.
> +	 *
> +	 * wake_up() or other post-signal housekeeping should also happen here.
> +	 */
> +	void (*worker)(struct drm_user_fence *ufence, bool mm_ok);
> +
> +	/**
> +	 * @destroy: Called when the last reference is dropped.
> +	 * Free the containing structure here.
> +	 */
> +	void (*destroy)(struct drm_user_fence *ufence);
> +};
> +
> +/**
> + * struct drm_user_fence - embeddable DRM user fence
> + *
> + * Drivers embed this in their own structure and implement
> + * &drm_user_fence_ops. Call drm_user_fence_init() at creation and
> + * drm_user_fence_add_callback() to arm on a dma-fence.
> + * Call drm_user_fence_cancel_sync() before driver teardown.
> + */
> +struct drm_user_fence {

Should this common layer be split into two distinct concepts?

- drm_work_fence: 90% of what is here, minus the kthread_use_mm() and
  mm-related code.
- drm_user_fence: a subclass of drm_work_fence that adds the
  kthread_use_mm() and mm-related code.

I suggest this because I was thinking about it the other day (I forget
the exact context) and reconsidered a pattern where a fence signals and
then I need a worker because some work must be done outside of IRQ
context. A user fence is one example, since copy_to_user() can fault,
which is not allowed in IRQ context. At various times in Xe we've had
multiple patterns like this, although at the moment user fences are
probably the only case that requires it. If we looked across DRM as a
whole, I suspect we'd find this pattern open-coded in a number of
places.

Yes, drm_user_fence would be a very thin layer on top of drm_work_fence,
but I still see value in the split.
 
Matt

> +	/** @refcount: Reference count. */
> +	struct kref refcount;
> +	/** @mm: Process MM grabbed at init time. */
> +	struct mm_struct *mm;
> +	/** @work: Work item queued when the dma-fence signals. */
> +	struct work_struct work;
> +	/** @cb: dma-fence callback. */
> +	struct dma_fence_cb cb;
> +	/**
> +	 * @fence: Extra reference held for safe cancel(). Set during
> +	 * add_callback, released in destroy().
> +	 */
> +	struct dma_fence *fence;
> +	/** @wq: Workqueue to run @work on. */
> +	struct workqueue_struct *wq;
> +	/** @ops: Driver operations. */
> +	const struct drm_user_fence_ops *ops;
> +};
> +
> +void drm_user_fence_init(struct drm_user_fence *ufence,
> +			 struct workqueue_struct *wq,
> +			 const struct drm_user_fence_ops *ops);
> +void drm_user_fence_get(struct drm_user_fence *ufence);
> +void drm_user_fence_put(struct drm_user_fence *ufence);
> +int drm_user_fence_add_callback(struct drm_user_fence *ufence,
> +				struct dma_fence *fence);
> +bool drm_user_fence_cancel(struct drm_user_fence *ufence);
> +void drm_user_fence_cancel_sync(struct drm_user_fence *ufence);
> +
> +#endif /* __DRM_USER_FENCE_H__ */
> -- 
> 2.34.1
>