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

[SHANMUGAM, SRINIVASAN]

AMD General

> -----Original Message-----
> From: Matthew Brost <matthew.brost@xxxxxxxxx>
> Sent: Friday, August 28, 2026 12:47 PM
> To: SHANMUGAM, SRINIVASAN <SRINIVASAN.SHANMUGAM@xxxxxxx>
> Cc: Koenig, Christian <Christian.Koenig@xxxxxxx>; Deucher, Alexander
> <Alexander.Deucher@xxxxxxx>; Maarten Lankhorst
> <maarten.lankhorst@xxxxxxxxxxxxxxx>; Maxime Ripard <mripard@xxxxxxxxxx>;
> Thomas Zimmermann <tzimmermann@xxxxxxx>; David Airlie
> <airlied@xxxxxxxxx>; Simona Vetter <simona@xxxxxxxx>; Sumit Semwal
> <sumit.semwal@xxxxxxxxxx>; Thomas Hellström
> <thomas.hellstrom@xxxxxxxxxxxxxxx>; dri-devel@xxxxxxxxxxxxxxxxxxxxx; intel-
> xe@xxxxxxxxxxxxxxxxxxxxx; linux-media@xxxxxxxxxxxxxxx; linaro-mm-
> sig@xxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v4 1/2] drm: Add common drm_user_fence helper
>
> 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@xxxxxxx>
> > Cc: Alex Deucher <alexander.deucher@xxxxxxx>
> > Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> > Cc: Maxime Ripard <mripard@xxxxxxxxxx>
> > Cc: Thomas Zimmermann <tzimmermann@xxxxxxx>
> > Cc: David Airlie <airlied@xxxxxxxxx>
> > Cc: Simona Vetter <simona@xxxxxxxx>
> > Cc: Sumit Semwal <sumit.semwal@xxxxxxxxxx>
> > Cc: Matthew Brost <matthew.brost@xxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxx>
> > Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
> > Cc: intel-xe@xxxxxxxxxxxxxxxxxxxxx
> > Cc: linux-media@xxxxxxxxxxxxxxx
> > Cc: linaro-mm-sig@xxxxxxxxxxxxxxxx
> > Cc: linux-kernel@xxxxxxxxxxxxxxx
> > Cc: amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> > Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam@xxxxxxx>
> > ---
> > 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.

Hi Matt,

Thanks for the review and for being supportive of the idea.

The split into drm_work_fence (general fence-to-workqueue pattern) and
drm_user_fence (subclass adding kthread_use_mm) makes sense. I'll
restructure v5 as follows:

  drm_work_fence: kref, work_struct, dma_fence_cb, stored fence ref,
                  wq, ops — add_callback, cancel, cancel_sync

  drm_user_fence: embeds drm_work_fence, adds mm_struct and the
                  kthread_use_mm/mmput boilerplate, thin wrappers

XE will continue to use drm_user_fence. For AMDGPU, The long-term
per-signal filtering approach (reading the fence value via copy_from_user
before signaling) will use drm_user_fence — further validating both
layers of the split.

Regarding the CI failure — the root cause was a missing trailing newline
at the end of xe_sync_types.h which caused the kunit build to fail with
"unterminated #ifndef". I've set up kunit locally and confirmed the fix:

  Testing complete. Ran 588 tests: passed: 570, skipped: 18
  Elapsed time: 22.916s total, 3.949s configuring, 18.350s building,
  0.601s running

The 18 skipped tests require Intel hardware — expected. The CI fix will
be included in v5 along with the drm_work_fence restructuring.

Thanks,
Srini

>
> Matt