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

["Christian König" <christian.koenig@amd.com>]

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@redhat.com>
> Cc: Simona Vetter <simona.vetter@ffwll.ch>
> Cc: Christian Koenig <christian.koenig@amd.com>
> Signed-off-by: Matthew Brost <matthew.brost@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.

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.

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?

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