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

[Nicolas Dufresne <nicolas@ndufresne.ca>]

Le lundi 11 septembre 2023 à 12:13 +0200, Christian König a écrit :
> 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.

If the memory is user visible, yes. When I look at the MTK VCODEC changes, this
seems to be used for internal codec state and SHM buffers used to communicate
with firmware.

> 
> 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 */
>