Re: [PATCH 2/3] rpmsg: virtio_rpmsg_bus: allocate rx/tx buffer separately

[xiang xiao <xiaoxiang781216@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 7251 bytes --]

On Thu, May 9, 2019 at 8:02 PM Arnaud Pouliquen <arnaud.pouliquen@st.com> wrote:
>
> Hello Xiang,
>
> This patch has the opposite effect on my platform as DMA allocation is
> aligned on 4k page.
> For instance i declared:
> - in RX  6 buffers (of 512 bytes)
> - in TX  4 buffers ( of 512 bytes)
>

Yes, dma_init_coherent_memory always allocate memory by 4KB unit, but
this limitation is too waste memory for remoteproc/rpmsg. The attached
patch fix this problem by adding a new device tree option to customize
the unit size.

> The result is (kernel trace)
> [   41.915896] virtio_rpmsg_bus virtio0: rx buffers: va ebb5f5ca, dma
> 0x0x10042000
> [   41.915922] virtio_rpmsg_bus virtio0: tx buffers: va a7865153, dma
> 0x0x10043000
>
> The TX buffer memory is allocated on next 4k page...
>
> Anyway separate the RX and TX allocation makes sense. This could also
> allow to allocate buffers in 2 different memories.
> For time being, issue is that only one memory area can be attached to
> the virtio device for DMA allocation... and PA/DA translations are missing.
> This means that we probably need (in a first step) a new remoteproc API
> for memory allocation.
> These memories should be declared and mmaped in rproc platform drivers
> (memory region) or in resource table (carveout).
> This is partially done in the API for the platform driver
> (rproc_mem_entry_init) but not available for rproc clients.
>
> Regards
> Arnaud
>
>
> On 1/31/19 4:41 PM, Xiang Xiao wrote:
> > many dma allocator align the returned address with buffer size,
> > so two small allocation could reduce the alignment requirement
> > and save the the memory space wasted by the potential alignment.
> >
> > Signed-off-by: Xiang Xiao <xiaoxiang@xiaomi.com>
> > ---
> >  drivers/rpmsg/virtio_rpmsg_bus.c | 58 +++++++++++++++++++++++-----------------
> >  1 file changed, 34 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> > index fb0d2eb..59c4554 100644
> > --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> > +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> > @@ -40,7 +40,8 @@
> >   * @num_sbufs:       total number of buffers for tx
> >   * @buf_size:        size of one rx or tx buffer
> >   * @last_sbuf:       index of last tx buffer used
> > - * @bufs_dma:        dma base addr of the buffers
> > + * @rbufs_dma:       dma base addr of rx buffers
> > + * @sbufs_dma:       dma base addr of tx buffers
> >   * @tx_lock: protects svq, sbufs and sleepers, to allow concurrent senders.
> >   *           sending a message might require waking up a dozing remote
> >   *           processor, which involves sleeping, hence the mutex.
> > @@ -62,7 +63,8 @@ struct virtproc_info {
> >       unsigned int num_sbufs;
> >       unsigned int buf_size;
> >       int last_sbuf;
> > -     dma_addr_t bufs_dma;
> > +     dma_addr_t rbufs_dma;
> > +     dma_addr_t sbufs_dma;
> >       struct mutex tx_lock;
> >       struct idr endpoints;
> >       struct mutex endpoints_lock;
> > @@ -872,9 +874,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
> >       static const char * const names[] = { "input", "output" };
> >       struct virtqueue *vqs[2];
> >       struct virtproc_info *vrp;
> > -     void *bufs_va;
> >       int err = 0, i;
> > -     size_t total_buf_space;
> >       bool notify;
> >
> >       vrp = kzalloc(sizeof(*vrp), GFP_KERNEL);
> > @@ -909,25 +909,28 @@ static int rpmsg_probe(struct virtio_device *vdev)
> >
> >       vrp->buf_size = MAX_RPMSG_BUF_SIZE;
> >
> > -     total_buf_space = (vrp->num_rbufs + vrp->num_sbufs) * vrp->buf_size;
> > -
> >       /* allocate coherent memory for the buffers */
> > -     bufs_va = dma_alloc_coherent(vdev->dev.parent->parent,
> > -                                  total_buf_space, &vrp->bufs_dma,
> > -                                  GFP_KERNEL);
> > -     if (!bufs_va) {
> > +     vrp->rbufs = dma_alloc_coherent(vdev->dev.parent->parent,
> > +                                     vrp->num_rbufs * vrp->buf_size,
> > +                                     &vrp->rbufs_dma, GFP_KERNEL);
> > +     if (!vrp->rbufs) {
> >               err = -ENOMEM;
> >               goto vqs_del;
> >       }
> >
> > -     dev_dbg(&vdev->dev, "buffers: va %p, dma %pad\n",
> > -             bufs_va, &vrp->bufs_dma);
> > +     dev_dbg(&vdev->dev, "rx buffers: va %p, dma 0x%pad\n",
> > +             vrp->rbufs, &vrp->rbufs_dma);
> >
> > -     /* first part of the buffers is dedicated for RX */
> > -     vrp->rbufs = bufs_va;
> > +     vrp->sbufs = dma_alloc_coherent(vdev->dev.parent->parent,
> > +                                     vrp->num_sbufs * vrp->buf_size,
> > +                                     &vrp->sbufs_dma, GFP_KERNEL);
> > +     if (!vrp->sbufs) {
> > +             err = -ENOMEM;
> > +             goto free_rbufs;
> > +     }
> >
> > -     /* and second part is dedicated for TX */
> > -     vrp->sbufs = bufs_va + vrp->num_rbufs * vrp->buf_size;
> > +     dev_dbg(&vdev->dev, "tx buffers: va %p, dma 0x%pad\n",
> > +             vrp->sbufs, &vrp->sbufs_dma);
> >
> >       /* set up the receive buffers */
> >       for (i = 0; i < vrp->num_rbufs; i++) {
> > @@ -954,7 +957,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
> >               if (!vrp->ns_ept) {
> >                       dev_err(&vdev->dev, "failed to create the ns ept\n");
> >                       err = -ENOMEM;
> > -                     goto free_coherent;
> > +                     goto free_sbufs;
> >               }
> >       }
> >
> > @@ -979,9 +982,14 @@ static int rpmsg_probe(struct virtio_device *vdev)
> >
> >       return 0;
> >
> > -free_coherent:
> > -     dma_free_coherent(vdev->dev.parent->parent, total_buf_space,
> > -                       bufs_va, vrp->bufs_dma);
> > +free_sbufs:
> > +     dma_free_coherent(vdev->dev.parent->parent,
> > +                       vrp->num_sbufs * vrp->buf_size,
> > +                       vrp->sbufs, vrp->sbufs_dma);
> > +free_rbufs:
> > +     dma_free_coherent(vdev->dev.parent->parent,
> > +                       vrp->num_rbufs * vrp->buf_size,
> > +                       vrp->rbufs, vrp->rbufs_dma);
> >  vqs_del:
> >       vdev->config->del_vqs(vrp->vdev);
> >  free_vrp:
> > @@ -999,8 +1007,6 @@ static int rpmsg_remove_device(struct device *dev, void *data)
> >  static void rpmsg_remove(struct virtio_device *vdev)
> >  {
> >       struct virtproc_info *vrp = vdev->priv;
> > -     unsigned int num_bufs = vrp->num_rbufs + vrp->num_sbufs;
> > -     size_t total_buf_space = num_bufs * vrp->buf_size;
> >       int ret;
> >
> >       vdev->config->reset(vdev);
> > @@ -1016,8 +1022,12 @@ static void rpmsg_remove(struct virtio_device *vdev)
> >
> >       vdev->config->del_vqs(vrp->vdev);
> >
> > -     dma_free_coherent(vdev->dev.parent->parent, total_buf_space,
> > -                       vrp->rbufs, vrp->bufs_dma);
> > +     dma_free_coherent(vdev->dev.parent->parent,
> > +                       vrp->num_sbufs * vrp->buf_size,
> > +                       vrp->sbufs, vrp->sbufs_dma);
> > +     dma_free_coherent(vdev->dev.parent->parent,
> > +                       vrp->num_rbufs * vrp->buf_size,
> > +                       vrp->rbufs, vrp->rbufs_dma);
> >
> >       kfree(vrp);
> >  }
> >

[-- Attachment #2: 0001-dma-coherent-support-the-alignment-smaller-than-PAGE.patch --]
[-- Type: application/octet-stream, Size: 9779 bytes --]

From 62672e1df79c9ffccb062f5aa0dcb18e938ced0c Mon Sep 17 00:00:00 2001
From: Xiang Xiao <xiaoxiang@xiaomi.com>
Date: Thu, 22 Jun 2017 14:30:50 +0800
Subject: [PATCH] dma-coherent: support the alignment smaller than PAGE_SIZE

and read the alignment(default PAGE_SIZE) from device tree

Change-Id: I512128859b0aac2e25be09ca8e0968190057e160
Signed-off-by: Xiang Xiao <xiaoxiang@xiaomi.com>
---
 .../bindings/reserved-memory/reserved-memory.txt   |  2 +
 drivers/base/dma-coherent.c                        | 74 ++++++++++++++--------
 include/linux/dma-mapping.h                        |  6 +-
 3 files changed, 51 insertions(+), 31 deletions(-)

diff --git a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
index 3da0ebdba8d9..383ada09213b 100644
--- a/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
+++ b/Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
@@ -63,6 +63,8 @@ reusable (optional) - empty property
       able to reclaim it back. Typically that means that the operating
       system can use that region to store volatile or cached data that
       can be otherwise regenerated or migrated elsewhere.
+align-shift (optional) - the allocation alignment (1 << align-shift)
+    - The default value is PAGE_SHIFT.
 
 Linux implementation note:
 - If a "linux,cma-default" property is present, then Linux will use the
diff --git a/drivers/base/dma-coherent.c b/drivers/base/dma-coherent.c
index 640a7e63c453..f93aee81a5ac 100644
--- a/drivers/base/dma-coherent.c
+++ b/drivers/base/dma-coherent.c
@@ -14,18 +14,19 @@ struct dma_coherent_mem {
 	unsigned long	pfn_base;
 	int		size;
 	int		flags;
+	int		align_shift;
 	unsigned long	*bitmap;
 	spinlock_t	spinlock;
 };
 
 static bool dma_init_coherent_memory(
 	phys_addr_t phys_addr, dma_addr_t device_addr, size_t size, int flags,
-	struct dma_coherent_mem **mem)
+	int align_shift, struct dma_coherent_mem **mem)
 {
 	struct dma_coherent_mem *dma_mem = NULL;
 	void __iomem *mem_base = NULL;
-	int pages = size >> PAGE_SHIFT;
-	int bitmap_size = BITS_TO_LONGS(pages) * sizeof(long);
+	int nbits = size >> align_shift;
+	int bitmap_size = BITS_TO_LONGS(nbits) * sizeof(long);
 
 	if ((flags & (DMA_MEMORY_MAP | DMA_MEMORY_IO)) == 0)
 		goto out;
@@ -49,8 +50,9 @@ static bool dma_init_coherent_memory(
 	dma_mem->virt_base = mem_base;
 	dma_mem->device_base = device_addr;
 	dma_mem->pfn_base = PFN_DOWN(phys_addr);
-	dma_mem->size = pages;
+	dma_mem->size = nbits;
 	dma_mem->flags = flags;
+	dma_mem->align_shift = align_shift;
 	spin_lock_init(&dma_mem->spinlock);
 
 	*mem = dma_mem;
@@ -98,7 +100,7 @@ int dma_declare_coherent_memory(struct device *dev, phys_addr_t phys_addr,
 	struct dma_coherent_mem *mem;
 
 	if (!dma_init_coherent_memory(phys_addr, device_addr, size, flags,
-				      &mem))
+				      PAGE_SHIFT, &mem))
 		return 0;
 
 	if (dma_assign_coherent_memory(dev, mem) == 0)
@@ -125,21 +127,25 @@ void *dma_mark_declared_memory_occupied(struct device *dev,
 {
 	struct dma_coherent_mem *mem = dev->dma_mem;
 	unsigned long flags;
-	int pos, err;
+	int pos, nbits, err;
 
-	size += device_addr & ~PAGE_MASK;
+	size += device_addr & ((1 << mem->align_shift) - 1);
 
 	if (!mem)
 		return ERR_PTR(-EINVAL);
 
 	spin_lock_irqsave(&mem->spinlock, flags);
-	pos = (device_addr - mem->device_base) >> PAGE_SHIFT;
-	err = bitmap_allocate_region(mem->bitmap, pos, get_order(size));
+	pos = (device_addr - mem->device_base) >> mem->align_shift;
+	nbits = (size + (1 << mem->align_shift) - 1) >> mem->align_shift;
+	if (pos == bitmap_find_next_zero_area(mem->bitmap, mem->size, pos, nbits, 0))
+		bitmap_set(mem->bitmap, pos, nbits);
+	else
+		err = -EBUSY;
 	spin_unlock_irqrestore(&mem->spinlock, flags);
 
 	if (err != 0)
 		return ERR_PTR(err);
-	return mem->virt_base + (pos << PAGE_SHIFT);
+	return mem->virt_base + (pos << mem->align_shift);
 }
 EXPORT_SYMBOL(dma_mark_declared_memory_occupied);
 
@@ -162,9 +168,9 @@ int dma_alloc_from_coherent(struct device *dev, ssize_t size,
 				       dma_addr_t *dma_handle, void **ret)
 {
 	struct dma_coherent_mem *mem;
-	int order = get_order(size);
+	int nbits;
 	unsigned long flags;
-	int pageno;
+	int bitno;
 	int dma_memory_map;
 
 	if (!dev)
@@ -176,18 +182,20 @@ int dma_alloc_from_coherent(struct device *dev, ssize_t size,
 	*ret = NULL;
 	spin_lock_irqsave(&mem->spinlock, flags);
 
-	if (unlikely(size > (mem->size << PAGE_SHIFT)))
+	if (unlikely(size > (mem->size << mem->align_shift)))
 		goto err;
 
-	pageno = bitmap_find_free_region(mem->bitmap, mem->size, order);
-	if (unlikely(pageno < 0))
+	nbits = (size + (1 << mem->align_shift) - 1) >> mem->align_shift;
+	bitno = bitmap_find_next_zero_area(mem->bitmap, mem->size, 0, nbits, 0);
+	if (unlikely(bitno >= mem->size))
 		goto err;
+	bitmap_set(mem->bitmap, bitno, nbits);
 
 	/*
 	 * Memory was found in the per-device area.
 	 */
-	*dma_handle = mem->device_base + (pageno << PAGE_SHIFT);
-	*ret = mem->virt_base + (pageno << PAGE_SHIFT);
+	*dma_handle = mem->device_base + (bitno << mem->align_shift);
+	*ret = mem->virt_base + (bitno << mem->align_shift);
 	dma_memory_map = (mem->flags & DMA_MEMORY_MAP);
 	spin_unlock_irqrestore(&mem->spinlock, flags);
 	if (dma_memory_map)
@@ -211,7 +219,7 @@ EXPORT_SYMBOL(dma_alloc_from_coherent);
 /**
  * dma_release_from_coherent() - try to free the memory allocated from per-device coherent memory pool
  * @dev:	device from which the memory was allocated
- * @order:	the order of pages allocated
+ * @size:	the size of allocated memory
  * @vaddr:	virtual address of allocated pages
  *
  * This checks whether the memory was allocated from the per-device
@@ -221,17 +229,18 @@ EXPORT_SYMBOL(dma_alloc_from_coherent);
  * dma_release_coherent() should proceed with releasing memory from
  * generic pools.
  */
-int dma_release_from_coherent(struct device *dev, int order, void *vaddr)
+int dma_release_from_coherent(struct device *dev, int size, void *vaddr)
 {
 	struct dma_coherent_mem *mem = dev ? dev->dma_mem : NULL;
 
 	if (mem && vaddr >= mem->virt_base && vaddr <
-		   (mem->virt_base + (mem->size << PAGE_SHIFT))) {
-		int page = (vaddr - mem->virt_base) >> PAGE_SHIFT;
+		   (mem->virt_base + (mem->size << mem->align_shift))) {
+		int bitno = (vaddr - mem->virt_base) >> mem->align_shift;
+		int nbits = (size + (1 << mem->align_shift) - 1) >> mem->align_shift;
 		unsigned long flags;
 
 		spin_lock_irqsave(&mem->spinlock, flags);
-		bitmap_release_region(mem->bitmap, page, order);
+		bitmap_clear(mem->bitmap, bitno, nbits);
 		spin_unlock_irqrestore(&mem->spinlock, flags);
 		return 1;
 	}
@@ -260,7 +269,7 @@ int dma_mmap_from_coherent(struct device *dev, struct vm_area_struct *vma,
 	struct dma_coherent_mem *mem = dev ? dev->dma_mem : NULL;
 
 	if (mem && vaddr >= mem->virt_base && vaddr + size <=
-		   (mem->virt_base + (mem->size << PAGE_SHIFT))) {
+		   (mem->virt_base + (mem->size << mem->align_shift))) {
 		unsigned long off = vma->vm_pgoff;
 		int start = (vaddr - mem->virt_base) >> PAGE_SHIFT;
 		int user_count = vma_pages(vma);
@@ -290,13 +299,22 @@ EXPORT_SYMBOL(dma_mmap_from_coherent);
 static int rmem_dma_device_init(struct reserved_mem *rmem, struct device *dev)
 {
 	struct dma_coherent_mem *mem = rmem->priv;
+	const __be32 *prop;
+	int align_shift;
+	int len;
+
+	prop = of_get_flat_dt_prop(rmem->fdt_node, "align-shift", &len);
+	if (prop && len >= 4)
+		align_shift = of_read_number(prop, len / 4);
+	else
+		align_shift = PAGE_SHIFT;
 
 	if (!mem &&
 	    !dma_init_coherent_memory(rmem->base, rmem->base, rmem->size,
 				      DMA_MEMORY_MAP | DMA_MEMORY_EXCLUSIVE,
-				      &mem)) {
-		pr_err("Reserved memory: failed to init DMA memory pool at %pa, size %ld MiB\n",
-			&rmem->base, (unsigned long)rmem->size / SZ_1M);
+				      align_shift, &mem)) {
+		pr_err("Reserved memory: failed to init DMA memory pool at %pa, size %ld KiB\n",
+			&rmem->base, (unsigned long)rmem->size / SZ_1K);
 		return -ENODEV;
 	}
 	rmem->priv = mem;
@@ -330,8 +348,8 @@ static int __init rmem_dma_setup(struct reserved_mem *rmem)
 #endif
 
 	rmem->ops = &rmem_dma_ops;
-	pr_info("Reserved memory: created DMA memory pool at %pa, size %ld MiB\n",
-		&rmem->base, (unsigned long)rmem->size / SZ_1M);
+	pr_info("Reserved memory: created DMA memory pool at %pa, size %ld KiB\n",
+		&rmem->base, (unsigned long)rmem->size / SZ_1K);
 	return 0;
 }
 RESERVEDMEM_OF_DECLARE(dma, "shared-dma-pool", rmem_dma_setup);
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index 08528afdf58b..8d9fd7b01dbb 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -152,13 +152,13 @@ static inline int is_device_dma_capable(struct device *dev)
  */
 int dma_alloc_from_coherent(struct device *dev, ssize_t size,
 				       dma_addr_t *dma_handle, void **ret);
-int dma_release_from_coherent(struct device *dev, int order, void *vaddr);
+int dma_release_from_coherent(struct device *dev, int size, void *vaddr);
 
 int dma_mmap_from_coherent(struct device *dev, struct vm_area_struct *vma,
 			    void *cpu_addr, size_t size, int *ret);
 #else
 #define dma_alloc_from_coherent(dev, size, handle, ret) (0)
-#define dma_release_from_coherent(dev, order, vaddr) (0)
+#define dma_release_from_coherent(dev, size, vaddr) (0)
 #define dma_mmap_from_coherent(dev, vma, vaddr, order, ret) (0)
 #endif /* CONFIG_HAVE_GENERIC_DMA_COHERENT */
 
@@ -478,7 +478,7 @@ static inline void dma_free_attrs(struct device *dev, size_t size,
 	BUG_ON(!ops);
 	WARN_ON(irqs_disabled());
 
-	if (dma_release_from_coherent(dev, get_order(size), cpu_addr))
+	if (dma_release_from_coherent(dev, size, cpu_addr))
 		return;
 
 	if (!ops->free || !cpu_addr)
-- 
2.16.2