Re: [PATCH 3/9] dma-heap: Provide accessors so that in-kernel drivers can allocate dmabufs from specific heaps

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

Am 11.09.23 um 04:30 schrieb Yong Wu:
> From: John Stultz <jstultz@google.com>
>
> This allows drivers who don't want to create their own
> DMA-BUF exporter to be able to allocate DMA-BUFs directly
> from existing DMA-BUF Heaps.
>
> There is some concern that the premise of DMA-BUF heaps is
> that userland knows better about what type of heap memory
> is needed for a pipeline, so it would likely be best for
> drivers to import and fill DMA-BUFs allocated by userland
> instead of allocating one themselves, but this is still
> up for debate.

The main design goal of having DMA-heaps in the first place is to avoid 
per driver allocation and this is not necessary because userland know 
better what type of memory it wants.

The background is rather that we generally want to decouple allocation 
from having a device driver connection so that we have better chance 
that multiple devices can work with the same memory.

I once create a prototype which gives userspace a hint which DMA-heap to 
user for which device: 
https://patchwork.kernel.org/project/linux-media/patch/20230123123756.401692-2-christian.koenig@amd.com/

Problem is that I don't really have time to look into it and maintain 
that stuff, but I think from the high level design that is rather the 
general direction we should push at.

Regards,
Christian.

>
> Signed-off-by: John Stultz <jstultz@google.com>
> Signed-off-by: T.J. Mercier <tjmercier@google.com>
> Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> [Yong: Fix the checkpatch alignment warning]
> ---
>   drivers/dma-buf/dma-heap.c | 60 ++++++++++++++++++++++++++++----------
>   include/linux/dma-heap.h   | 25 ++++++++++++++++
>   2 files changed, 69 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c
> index dcc0e38c61fa..908bb30dc864 100644
> --- a/drivers/dma-buf/dma-heap.c
> +++ b/drivers/dma-buf/dma-heap.c
> @@ -53,12 +53,15 @@ static dev_t dma_heap_devt;
>   static struct class *dma_heap_class;
>   static DEFINE_XARRAY_ALLOC(dma_heap_minors);
>   
> -static int dma_heap_buffer_alloc(struct dma_heap *heap, size_t len,
> -				 unsigned int fd_flags,
> -				 unsigned int heap_flags)
> +struct dma_buf *dma_heap_buffer_alloc(struct dma_heap *heap, size_t len,
> +				      unsigned int fd_flags,
> +				      unsigned int heap_flags)
>   {
> -	struct dma_buf *dmabuf;
> -	int fd;
> +	if (fd_flags & ~DMA_HEAP_VALID_FD_FLAGS)
> +		return ERR_PTR(-EINVAL);
> +
> +	if (heap_flags & ~DMA_HEAP_VALID_HEAP_FLAGS)
> +		return ERR_PTR(-EINVAL);
>   
>   	/*
>   	 * Allocations from all heaps have to begin
> @@ -66,9 +69,20 @@ static int dma_heap_buffer_alloc(struct dma_heap *heap, size_t len,
>   	 */
>   	len = PAGE_ALIGN(len);
>   	if (!len)
> -		return -EINVAL;
> +		return ERR_PTR(-EINVAL);
>   
> -	dmabuf = heap->ops->allocate(heap, len, fd_flags, heap_flags);
> +	return heap->ops->allocate(heap, len, fd_flags, heap_flags);
> +}
> +EXPORT_SYMBOL_GPL(dma_heap_buffer_alloc);
> +
> +static int dma_heap_bufferfd_alloc(struct dma_heap *heap, size_t len,
> +				   unsigned int fd_flags,
> +				   unsigned int heap_flags)
> +{
> +	struct dma_buf *dmabuf;
> +	int fd;
> +
> +	dmabuf = dma_heap_buffer_alloc(heap, len, fd_flags, heap_flags);
>   	if (IS_ERR(dmabuf))
>   		return PTR_ERR(dmabuf);
>   
> @@ -106,15 +120,9 @@ static long dma_heap_ioctl_allocate(struct file *file, void *data)
>   	if (heap_allocation->fd)
>   		return -EINVAL;
>   
> -	if (heap_allocation->fd_flags & ~DMA_HEAP_VALID_FD_FLAGS)
> -		return -EINVAL;
> -
> -	if (heap_allocation->heap_flags & ~DMA_HEAP_VALID_HEAP_FLAGS)
> -		return -EINVAL;
> -
> -	fd = dma_heap_buffer_alloc(heap, heap_allocation->len,
> -				   heap_allocation->fd_flags,
> -				   heap_allocation->heap_flags);
> +	fd = dma_heap_bufferfd_alloc(heap, heap_allocation->len,
> +				     heap_allocation->fd_flags,
> +				     heap_allocation->heap_flags);
>   	if (fd < 0)
>   		return fd;
>   
> @@ -205,6 +213,7 @@ const char *dma_heap_get_name(struct dma_heap *heap)
>   {
>   	return heap->name;
>   }
> +EXPORT_SYMBOL_GPL(dma_heap_get_name);
>   
>   struct dma_heap *dma_heap_add(const struct dma_heap_export_info *exp_info)
>   {
> @@ -290,6 +299,24 @@ struct dma_heap *dma_heap_add(const struct dma_heap_export_info *exp_info)
>   	kfree(heap);
>   	return err_ret;
>   }
> +EXPORT_SYMBOL_GPL(dma_heap_add);
> +
> +struct dma_heap *dma_heap_find(const char *name)
> +{
> +	struct dma_heap *h;
> +
> +	mutex_lock(&heap_list_lock);
> +	list_for_each_entry(h, &heap_list, list) {
> +		if (!strcmp(h->name, name)) {
> +			kref_get(&h->refcount);
> +			mutex_unlock(&heap_list_lock);
> +			return h;
> +		}
> +	}
> +	mutex_unlock(&heap_list_lock);
> +	return NULL;
> +}
> +EXPORT_SYMBOL_GPL(dma_heap_find);
>   
>   static void dma_heap_release(struct kref *ref)
>   {
> @@ -315,6 +342,7 @@ void dma_heap_put(struct dma_heap *h)
>   	kref_put(&h->refcount, dma_heap_release);
>   	mutex_unlock(&heap_list_lock);
>   }
> +EXPORT_SYMBOL_GPL(dma_heap_put);
>   
>   static char *dma_heap_devnode(const struct device *dev, umode_t *mode)
>   {
> diff --git a/include/linux/dma-heap.h b/include/linux/dma-heap.h
> index f3c678892c5c..59e70f6c7a60 100644
> --- a/include/linux/dma-heap.h
> +++ b/include/linux/dma-heap.h
> @@ -64,10 +64,35 @@ const char *dma_heap_get_name(struct dma_heap *heap);
>    */
>   struct dma_heap *dma_heap_add(const struct dma_heap_export_info *exp_info);
>   
> +/**
> + * dma_heap_find - get the heap registered with the specified name
> + * @name: Name of the DMA-Heap to find
> + *
> + * Returns:
> + * The DMA-Heap with the provided name.
> + *
> + * NOTE: DMA-Heaps returned from this function MUST be released using
> + * dma_heap_put() when the user is done to enable the heap to be unloaded.
> + */
> +struct dma_heap *dma_heap_find(const char *name);
> +
>   /**
>    * dma_heap_put - drops a reference to a dmabuf heap, potentially freeing it
>    * @heap: the heap whose reference count to decrement
>    */
>   void dma_heap_put(struct dma_heap *heap);
>   
> +/**
> + * dma_heap_buffer_alloc - Allocate dma-buf from a dma_heap
> + * @heap:	DMA-Heap to allocate from
> + * @len:	size to allocate in bytes
> + * @fd_flags:	flags to set on returned dma-buf fd
> + * @heap_flags: flags to pass to the dma heap
> + *
> + * This is for internal dma-buf allocations only. Free returned buffers with dma_buf_put().
> + */
> +struct dma_buf *dma_heap_buffer_alloc(struct dma_heap *heap, size_t len,
> +				      unsigned int fd_flags,
> +				      unsigned int heap_flags);
> +
>   #endif /* _DMA_HEAPS_H */