[RFC PATCH 02/29] dma-fence: Add dma_fence_user_fence

Wed Nov 20 22:50:22 UTC 2024

On Wed, Nov 20, 2024 at 02:38:49PM +0100, Christian König wrote:
> Am 19.11.24 um 00:37 schrieb Matthew Brost:
> > Normalize user fence attachment to a DMA fence. A user fence is a simple
> > seqno write to memory, implemented by attaching a DMA fence callback
> > that writes out the seqno. Intended use case is importing a dma-fence
> > into kernel and exporting a user fence.
> > 
> > Helpers added to allocate, attach, and free a dma_fence_user_fence.
> > 
> > Cc: Dave Airlie <airlied at redhat.com>
> > Cc: Simona Vetter <simona.vetter at ffwll.ch>
> > Cc: Christian Koenig <christian.koenig at amd.com>
> > Signed-off-by: Matthew Brost <matthew.brost at intel.com>
> > ---
> >   drivers/dma-buf/Makefile               |  2 +-
> >   drivers/dma-buf/dma-fence-user-fence.c | 73 ++++++++++++++++++++++++++
> >   include/linux/dma-fence-user-fence.h   | 31 +++++++++++
> >   3 files changed, 105 insertions(+), 1 deletion(-)
> >   create mode 100644 drivers/dma-buf/dma-fence-user-fence.c
> >   create mode 100644 include/linux/dma-fence-user-fence.h
> > 
> > diff --git a/drivers/dma-buf/Makefile b/drivers/dma-buf/Makefile
> > index c25500bb38b5..ba9ba339319e 100644
> > --- a/drivers/dma-buf/Makefile
> > +++ b/drivers/dma-buf/Makefile
> > @@ -1,6 +1,6 @@
> >   # SPDX-License-Identifier: GPL-2.0-only
> >   obj-y := dma-buf.o dma-fence.o dma-fence-array.o dma-fence-chain.o \
> > -	 dma-fence-preempt.o dma-fence-unwrap.o dma-resv.o
> > +	 dma-fence-preempt.o dma-fence-unwrap.o dma-fence-user-fence.o dma-resv.o
> >   obj-$(CONFIG_DMABUF_HEAPS)	+= dma-heap.o
> >   obj-$(CONFIG_DMABUF_HEAPS)	+= heaps/
> >   obj-$(CONFIG_SYNC_FILE)		+= sync_file.o
> > diff --git a/drivers/dma-buf/dma-fence-user-fence.c b/drivers/dma-buf/dma-fence-user-fence.c
> > new file mode 100644
> > index 000000000000..5a4b289bacb8
> > --- /dev/null
> > +++ b/drivers/dma-buf/dma-fence-user-fence.c
> > @@ -0,0 +1,73 @@
> > +// SPDX-License-Identifier: MIT
> > +/*
> > + * Copyright © 2024 Intel Corporation
> > + */
> > +
> > +#include <linux/dma-fence-user-fence.h>
> > +#include <linux/slab.h>
> > +
> > +static void user_fence_cb(struct dma_fence *fence, struct dma_fence_cb *cb)
> > +{
> > +	struct dma_fence_user_fence *user_fence =
> > +		container_of(cb, struct dma_fence_user_fence, cb);
> > +
> > +	if (user_fence->map.is_iomem)
> > +		writeq(user_fence->seqno, user_fence->map.vaddr_iomem);
> > +	else
> > +		*(u64 *)user_fence->map.vaddr = user_fence->seqno;
> > +
> > +	dma_fence_user_fence_free(user_fence);
> > +}
> > +
> > +/**
> > + * dma_fence_user_fence_alloc() - Allocate user fence
> > + *
> > + * Return: Allocated struct dma_fence_user_fence on Success, NULL on failure
> > + */
> > +struct dma_fence_user_fence *dma_fence_user_fence_alloc(void)
> > +{
> > +	return kmalloc(sizeof(struct dma_fence_user_fence), GFP_KERNEL);
> > +}
> > +EXPORT_SYMBOL(dma_fence_user_fence_alloc);
> > +
> > +/**
> > + * dma_fence_user_fence_free() - Free user fence
> > + *
> > + * Free user fence. Should only be called on a user fence if
> > + * dma_fence_user_fence_attach is not called to cleanup original allocation from
> > + * dma_fence_user_fence_alloc.
> > + */
> > +void dma_fence_user_fence_free(struct dma_fence_user_fence *user_fence)
> > +{
> > +	kfree(user_fence);
> 
> We need to give that child a different name, e.g. something like
> dma_fence_seq_write or something like that.
> 

Yea, I didn't like this name either. dma_fence_seq_write seems better.

> I was just about to complain that all dma_fence implementations need to be
> RCU save and only then saw that this isn't a dma_fence implementation.
> 

Nope, just a helper to back a value which user space can find when a
dma-fence signals.

> Question: Why is that useful in the first place? At least AMD HW can write
> to basically all memory locations and even registers when a fence finishes?
> 

This could be used in a few places.

1. VM bind completion a seqno is written which user jobs can then wait
on via ring instructions. We have something similar to this is Xe for LR
VMs already but I don't really like that interface - it is user address
+ write value. This would be based on a BO + offset which I think makes
a bit more sense and should perform quite a better too. I haven't wired
this up in this series but plan to doing this.

2. Convert an input dma-fence into seqno writeback when the dma-fence
signals. Again this seqno is something the user can wiat on via ring
instructions.

The flow here would be, a user job needs to wait on external dma-fence
in a syncobj, syncfile, etc..., call the convert dma-fence to user fence
IOCTL before the submission (patch 22, 28 in this series), program the
wait via ring instructions, and then do the user submission. This would
avoid blocking on external dma-fences in the submission path.

I think this makes sense and having a light weight helper to normalize
this flow across drivers makes a bit sense too.

Matt

> Regards,
> Christian.
> 
> > +}
> > +EXPORT_SYMBOL(dma_fence_user_fence_free);
> > +
> > +/**
> > + * dma_fence_user_fence_attach() - Attach user fence to dma-fence
> > + *
> > + * @fence: fence
> > + * @user_fence user fence
> > + * @map: IOSYS map to write seqno to
> > + * @seqno: seqno to write to IOSYS map
> > + *
> > + * Attach a user fence, which is a seqno write to an IOSYS map, to a DMA fence.
> > + * The caller must guarantee that the memory in the IOSYS map doesn't move
> > + * before the fence signals. This is typically done by installing the DMA fence
> > + * into the BO's DMA reservation bookkeeping slot from which the IOSYS was
> > + * derived.
> > + */
> > +void dma_fence_user_fence_attach(struct dma_fence *fence,
> > +				 struct dma_fence_user_fence *user_fence,
> > +				 struct iosys_map *map, u64 seqno)
> > +{
> > +	int err;
> > +
> > +	user_fence->map = *map;
> > +	user_fence->seqno = seqno;
> > +
> > +	err = dma_fence_add_callback(fence, &user_fence->cb, user_fence_cb);
> > +	if (err == -ENOENT)
> > +		user_fence_cb(NULL, &user_fence->cb);
> > +}
> > +EXPORT_SYMBOL(dma_fence_user_fence_attach);
> > diff --git a/include/linux/dma-fence-user-fence.h b/include/linux/dma-fence-user-fence.h
> > new file mode 100644
> > index 000000000000..8678129c7d56
> > --- /dev/null
> > +++ b/include/linux/dma-fence-user-fence.h
> > @@ -0,0 +1,31 @@
> > +/* SPDX-License-Identifier: MIT */
> > +/*
> > + * Copyright © 2024 Intel Corporation
> > + */
> > +
> > +#ifndef __LINUX_DMA_FENCE_USER_FENCE_H
> > +#define __LINUX_DMA_FENCE_USER_FENCE_H
> > +
> > +#include <linux/dma-fence.h>
> > +#include <linux/iosys-map.h>
> > +
> > +/** struct dma_fence_user_fence - User fence */
> > +struct dma_fence_user_fence {
> > +	/** @cb: dma-fence callback used to attach user fence to dma-fence */
> > +	struct dma_fence_cb cb;
> > +	/** @map: IOSYS map to write seqno to */
> > +	struct iosys_map map;
> > +	/** @seqno: seqno to write to IOSYS map */
> > +	u64 seqno;
> > +};
> > +
> > +struct dma_fence_user_fence *dma_fence_user_fence_alloc(void);
> > +
> > +void dma_fence_user_fence_free(struct dma_fence_user_fence *user_fence);
> > +
> > +void dma_fence_user_fence_attach(struct dma_fence *fence,
> > +				 struct dma_fence_user_fence *user_fence,
> > +				 struct iosys_map *map,
> > +				 u64 seqno);
> > +
> > +#endif
> 