Re: [PATCH v6 1/4] drm: Add drm_work_fence helper

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

On Tue, Sep 01, 2026 at 07:39:40AM +0000, SHANMUGAM, SRINIVASAN wrote:
> AMD General
> 
> > -----Original Message-----
> > From: Matthew Brost <matthew.brost@intel.com>
> > Sent: Tuesday, September 1, 2026 1:52 AM
> > To: SHANMUGAM, SRINIVASAN <SRINIVASAN.SHANMUGAM@amd.com>
> > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>; dri-
> > devel@lists.freedesktop.org; intel-xe@lists.freedesktop.org; Koenig, Christian
> > <Christian.Koenig@amd.com>; Deucher, Alexander
> > <Alexander.Deucher@amd.com>; amd-gfx@lists.freedesktop.org; Maarten
> > Lankhorst <maarten.lankhorst@linux.intel.com>
> > Subject: Re: [PATCH v6 1/4] drm: Add drm_work_fence helper
> >
> > On Mon, Aug 31, 2026 at 07:15:36PM +0530, Srinivasan Shanmugam wrote:
> > > GPU drivers often need to queue work when a dma-fence signals because
> > > certain operations (copy_to_user, eventfd_signal, memory
> > > allocation) cannot run in IRQ context. This pattern is currently
> > > open-coded in multiple drivers.
> > >
> > > Introduce drm_work_fence — an embeddable base structure that handles
> > > the dma-fence-callback-to-workqueue pattern in one place. Drivers
> > > embed this in their own structure and implement ops->work() for the
> > > deferred work and ops->destroy() for cleanup.
> > >
> > > The helper manages:
> > >  - kref lifetime
> > >  - dma-fence callback registration
> > >  - workqueue dispatch on fence signal
> > >  - safe cancellation before driver teardown
> > >
> > > For work that additionally requires borrowing the process MM via
> > > kthread_use_mm(), see drm_user_fence which builds on top of this.
> > >
> > > Suggested-by: Matthew Brost <matthew.brost@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
> > > Cc: amd-gfx@lists.freedesktop.org
> > > Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam@amd.com>
> > > ---
> > >  drivers/gpu/drm/Makefile         |   1 +
> > >  drivers/gpu/drm/drm_work_fence.c | 195
> > +++++++++++++++++++++++++++++++
> > >  include/drm/drm_work_fence.h     |  76 ++++++++++++
> > >  3 files changed, 272 insertions(+)
> > >  create mode 100644 drivers/gpu/drm/drm_work_fence.c  create mode
> > > 100644 include/drm/drm_work_fence.h
> > >
> > > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile index
> > > e97faabcd783..c5be8e80d0c8 100644
> > > --- a/drivers/gpu/drm/Makefile
> > > +++ b/drivers/gpu/drm/Makefile
> > > @@ -72,6 +72,7 @@ drm-y := \
> > >     drm_vblank.o \
> > >     drm_vblank_work.o \
> > >     drm_vma_manager.o \
> > > +   drm_work_fence.o \
> > >     drm_writeback.o
> > >  drm-$(CONFIG_DRM_CLIENT) += \
> > >     drm_client.o \
> > > diff --git a/drivers/gpu/drm/drm_work_fence.c
> > > b/drivers/gpu/drm/drm_work_fence.c
> > > new file mode 100644
> > > index 000000000000..9f6b779d0fe9
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/drm_work_fence.c
> > > @@ -0,0 +1,195 @@
> > > +// SPDX-License-Identifier: MIT
> > > +/*
> > > + * Copyright © 2024 The Linux Foundation
> > > + *
> > > + * Common DRM work fence helper.
> > > + *
> > > + * When a GPU dma-fence signals, drivers often need to perform work
> > > +that
> > > + * cannot run in IRQ context (e.g., memory allocation, copy_to_user,
> > > + * eventfd_signal). This helper queues a work item when a dma-fence
> > > + * signals, allowing that work to run safely in a workqueue context.
> > > + *
> > > + * NOTE: This helper consumes dma_fences but CANNOT implement
> > > + * dma_fence_ops. Work items queued here may sleep; dma_fence_ops
> > > + * callbacks are called under the fence spinlock and must not sleep.
> > > + *
> > > + * For work that additionally requires accessing userspace memory via
> > > + * kthread_use_mm(), see drm_user_fence which builds on top of this.
> > > + */
> > > +
> > > +#include <linux/workqueue.h>
> > > +
> > > +#include <drm/drm_work_fence.h>
> > > +
> > > +static void drm_work_fence_destroy(struct kref *kref) {
> > > +   struct drm_work_fence *wfence =
> > > +           container_of(kref, struct drm_work_fence, refcount);
> > > +
> > > +   if (wfence->fence)
> > > +           dma_fence_put(wfence->fence);
> > > +
> > > +   wfence->ops->destroy(wfence);
> >
> > I'd invert these for safety in case destroy wants to looks at the fence, admittedly
> > that would be an odd use case.
> >
> > So...
> >
> >       struct drm_work_fence *wfence =
> >               container_of(kref, struct drm_work_fence, refcount);
> >       struct dma_fence *fence = wfence->fence;
> >
> >       wfence->ops->destroy(wfence);
> >       dma_fence_put(fence);   /* this has a NULL check */
> >
> >
> > > +}
> > > +
> > > +/**
> > > + * drm_work_fence_get - Acquire a reference to a work fence
> > > + * @wfence: work fence
> > > + */
> > > +void drm_work_fence_get(struct drm_work_fence *wfence) {
> > > +   kref_get(&wfence->refcount);
> > > +}
> > > +EXPORT_SYMBOL_GPL(drm_work_fence_get);
> > > +
> > > +/**
> > > + * drm_work_fence_put - Release a reference to a work fence
> > > + * @wfence: work fence
> > > + */
> > > +void drm_work_fence_put(struct drm_work_fence *wfence) {
> > > +   kref_put(&wfence->refcount, drm_work_fence_destroy); }
> > > +EXPORT_SYMBOL_GPL(drm_work_fence_put);
> > > +
> > > +static void drm_work_fence_work(struct work_struct *w) {
> > > +   struct drm_work_fence *wfence =
> > > +           container_of(w, struct drm_work_fence, work);
> > > +
> > > +   wfence->ops->work(wfence);
> > > +   drm_work_fence_put(wfence);
> > > +}
> > > +
> > > +static void drm_work_fence_cb(struct dma_fence *fence, struct
> > > +dma_fence_cb *cb) {
> > > +   struct drm_work_fence *wfence =
> > > +           container_of(cb, struct drm_work_fence, cb);
> > > +
> > > +   queue_work(wfence->wq, &wfence->work);
> > > +   /*
> > > +    * Put the transferred reference from add_callback. The stored
> > > +    * reference in wfence->fence is released in drm_work_fence_destroy().
> > > +    */
> > > +   dma_fence_put(fence);
> > > +}
> > > +
> > > +/**
> > > + * drm_work_fence_init - Initialize a work fence
> > > + * @wfence: work fence to initialize
> > > + * @wq: workqueue to run the worker on (must be ordered if sequencing
> > > +matters)
> > > + * @ops: driver operations
> > > + */
> > > +void drm_work_fence_init(struct drm_work_fence *wfence,
> > > +                    struct workqueue_struct *wq,
> > > +                    const struct drm_work_fence_ops *ops) {
> > > +   kref_init(&wfence->refcount);
> > > +   wfence->wq = wq;
> > > +   wfence->ops = ops;
> > > +   wfence->fence = NULL;
> > > +   INIT_WORK(&wfence->work, drm_work_fence_work); }
> > > +EXPORT_SYMBOL_GPL(drm_work_fence_init);
> > > +
> > > +/**
> > > + * drm_work_fence_add_callback - Attach a work fence to a dma-fence
> > > + * @wfence: work fence
> > > + * @fence: dma-fence to watch; ownership of this reference is transferred
> > > + *         to the callback — caller must NOT put it afterward.
> >
> > This isn't right. It is perfectly reasonable for caller to hold more than 1 reference to
> > @fence, thus put it again. It consumes a single reference @fence on success or
> > failure - that is it.
> >
> > > + *
> > > + * When @fence signals, a work item is queued that calls ops->work().
> > > + * If @fence has already signaled, the work item is queued immediately.
> > > + *
> > > + * An additional reference to @fence is stored internally in @wfence
> > > + to
> > > + * allow drm_work_fence_cancel() to be called safely without the
> > > + caller
> > > + * needing to hold a separate fence reference.
> > > + *
> >
> > Ideally get rid of double ref count on @fence. I don't think above reasoning justifies
> > the needed for a double ref on the fence. I'd tie exactly one refernece @fence which
> > is attached to lifetime of @wfence (i.e., drop the dma_fence_put in
> > drm_work_fence_cb).
> >
> > > + * On any return value the caller's fence reference is consumed.
> > > + *
> >
> > I'd mention regardless of success or fail, a reference to drm_work_fence is
> > consumed too.
> >
> > > + * Return: 0 on success, negative errno on error.
> > > + */
> > > +int drm_work_fence_add_callback(struct drm_work_fence *wfence,
> > > +                           struct dma_fence *fence)
> > > +{
> > > +   int err;
> > > +
> > > +   drm_work_fence_get(wfence);
> > > +   wfence->fence = dma_fence_get(fence);
> > > +
> > > +   err = dma_fence_add_callback(fence, &wfence->cb, drm_work_fence_cb);
> > > +   if (err == -ENOENT) {
> > > +           queue_work(wfence->wq, &wfence->work);
> > > +           dma_fence_put(fence);
> >
> > Keep the implementation in one place?
> >
> > drm_work_fence_work(&wfence->work);

This is a bad suggestion actually, I was a bit distracted I guess - you
can't directly execute the worker at least in Xe as
drm_work_fence_add_callback is called holding the dma-resv lock and copy
to user can take mmap_read lock and we'd insert.

> 
> Hi Matt,
> 
> Thanks for your feedbacks once again!,
> 
> For the ENOENT path — I'm planning to extract a small shared helper:
> 
>     static void drm_work_fence_queue(struct drm_work_fence *wfence)
>     {
>         queue_work(wfence->wq, &wfence->work);
>     }
> 
> and call it from both drm_work_fence_cb() and the ENOENT path in
> add_callback(). This keeps the implementation in one place while
> preserving async execution.
> 

Yes, basically whatever drm_work_fence_cb does, stick into a helper and
call it here so if implementation diverges for the CB, we only have to
change it in one place.

Matt


> May I kno pls, is that what you had in mind, or did you mean something different?
> 
> Thanks,
> Srini