[PATCH 0/3] Enhance virtio rpmsg bus driver buffer allocation

All of lore.kernel.org
 help / color / mirror / Atom feed

* [PATCH 0/3] Enhance virtio rpmsg bus driver buffer allocation
@ 2019-01-31 15:41 Xiang Xiao
  2019-01-31 15:41 ` [PATCH 1/3] rpmsg: virtio_rpmsg_bus: allow the different vring size for send/recv Xiang Xiao
                   ` (3 more replies)
  0 siblings, 4 replies; 23+ messages in thread
From: Xiang Xiao @ 2019-01-31 15:41 UTC (permalink / raw)
  To: ohad, bjorn.andersson, wendy.liang, arnaud.pouliquen,
	linux-remoteproc, linux-kernel
  Cc: Xiang Xiao

Hi,
This series enhance the buffer allocation by:
1.Support the different buffer number in rx/tx direction
2.Get the individual rx/tx buffer size from config space

Here is the related OpenAMP change:
https://github.com/OpenAMP/open-amp/pull/155

Xiang Xiao (3):
  rpmsg: virtio_rpmsg_bus: allow the different vring size for send/recv
  rpmsg: virtio_rpmsg_bus: allocate rx/tx buffer separately
  rpmsg: virtio_rpmsg_bus: get buffer size from config space

 drivers/rpmsg/virtio_rpmsg_bus.c  | 127 +++++++++++++++++++++++---------------
 include/uapi/linux/virtio_rpmsg.h |  24 +++++++
 2 files changed, 100 insertions(+), 51 deletions(-)
 create mode 100644 include/uapi/linux/virtio_rpmsg.h

-- 
2.7.4

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH 1/3] rpmsg: virtio_rpmsg_bus: allow the different vring size for send/recv
  2019-01-31 15:41 [PATCH 0/3] Enhance virtio rpmsg bus driver buffer allocation Xiang Xiao
@ 2019-01-31 15:41 ` Xiang Xiao
  2019-05-09 11:47     ` Arnaud Pouliquen
  2019-01-31 15:41 ` [PATCH 2/3] rpmsg: virtio_rpmsg_bus: allocate rx/tx buffer separately Xiang Xiao
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 23+ messages in thread
From: Xiang Xiao @ 2019-01-31 15:41 UTC (permalink / raw)
  To: ohad, bjorn.andersson, wendy.liang, arnaud.pouliquen,
	linux-remoteproc, linux-kernel
  Cc: Xiang Xiao

it's useful if the communication throughput is different from each side

Signed-off-by: Xiang Xiao <xiaoxiang@xiaomi.com>
---
 drivers/rpmsg/virtio_rpmsg_bus.c | 47 ++++++++++++++++++++--------------------
 1 file changed, 24 insertions(+), 23 deletions(-)

diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index 664f957..fb0d2eb 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -36,8 +36,9 @@
  * @svq:	tx virtqueue
  * @rbufs:	kernel address of rx buffers
  * @sbufs:	kernel address of tx buffers
- * @num_bufs:	total number of buffers for rx and tx
- * @buf_size:   size of one rx or tx buffer
+ * @num_rbufs:	total number of buffers for rx
+ * @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
  * @tx_lock:	protects svq, sbufs and sleepers, to allow concurrent senders.
@@ -57,7 +58,8 @@ struct virtproc_info {
 	struct virtio_device *vdev;
 	struct virtqueue *rvq, *svq;
 	void *rbufs, *sbufs;
-	unsigned int num_bufs;
+	unsigned int num_rbufs;
+	unsigned int num_sbufs;
 	unsigned int buf_size;
 	int last_sbuf;
 	dma_addr_t bufs_dma;
@@ -136,7 +138,7 @@ struct virtio_rpmsg_channel {
 /*
  * We're allocating buffers of 512 bytes each for communications. The
  * number of buffers will be computed from the number of buffers supported
- * by the vring, upto a maximum of 512 buffers (256 in each direction).
+ * by the vring, up to a maximum of 256 in each direction.
  *
  * Each buffer will have 16 bytes for the msg header and 496 bytes for
  * the payload.
@@ -151,7 +153,7 @@ struct virtio_rpmsg_channel {
  * can change this without changing anything in the firmware of the remote
  * processor.
  */
-#define MAX_RPMSG_NUM_BUFS	(512)
+#define MAX_RPMSG_NUM_BUFS	(256)
 #define MAX_RPMSG_BUF_SIZE	(512)
 
 /*
@@ -446,11 +448,8 @@ static void *get_a_tx_buf(struct virtproc_info *vrp)
 	/* support multiple concurrent senders */
 	mutex_lock(&vrp->tx_lock);
 
-	/*
-	 * either pick the next unused tx buffer
-	 * (half of our buffers are used for sending messages)
-	 */
-	if (vrp->last_sbuf < vrp->num_bufs / 2)
+	/* either pick the next unused tx buffer */
+	if (vrp->last_sbuf < vrp->num_sbufs)
 		ret = vrp->sbufs + vrp->buf_size * vrp->last_sbuf++;
 	/* or recycle a used one */
 	else
@@ -897,19 +896,20 @@ static int rpmsg_probe(struct virtio_device *vdev)
 	vrp->rvq = vqs[0];
 	vrp->svq = vqs[1];
 
-	/* we expect symmetric tx/rx vrings */
-	WARN_ON(virtqueue_get_vring_size(vrp->rvq) !=
-		virtqueue_get_vring_size(vrp->svq));
-
 	/* we need less buffers if vrings are small */
-	if (virtqueue_get_vring_size(vrp->rvq) < MAX_RPMSG_NUM_BUFS / 2)
-		vrp->num_bufs = virtqueue_get_vring_size(vrp->rvq) * 2;
+	if (virtqueue_get_vring_size(vrp->rvq) < MAX_RPMSG_NUM_BUFS)
+		vrp->num_rbufs = virtqueue_get_vring_size(vrp->rvq);
+	else
+		vrp->num_rbufs = MAX_RPMSG_NUM_BUFS;
+
+	if (virtqueue_get_vring_size(vrp->svq) < MAX_RPMSG_NUM_BUFS)
+		vrp->num_sbufs = virtqueue_get_vring_size(vrp->svq);
 	else
-		vrp->num_bufs = MAX_RPMSG_NUM_BUFS;
+		vrp->num_sbufs = MAX_RPMSG_NUM_BUFS;
 
 	vrp->buf_size = MAX_RPMSG_BUF_SIZE;
 
-	total_buf_space = vrp->num_bufs * vrp->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,
@@ -923,14 +923,14 @@ static int rpmsg_probe(struct virtio_device *vdev)
 	dev_dbg(&vdev->dev, "buffers: va %p, dma %pad\n",
 		bufs_va, &vrp->bufs_dma);
 
-	/* half of the buffers is dedicated for RX */
+	/* first part of the buffers is dedicated for RX */
 	vrp->rbufs = bufs_va;
 
-	/* and half is dedicated for TX */
-	vrp->sbufs = bufs_va + total_buf_space / 2;
+	/* and second part is dedicated for TX */
+	vrp->sbufs = bufs_va + vrp->num_rbufs * vrp->buf_size;
 
 	/* set up the receive buffers */
-	for (i = 0; i < vrp->num_bufs / 2; i++) {
+	for (i = 0; i < vrp->num_rbufs; i++) {
 		struct scatterlist sg;
 		void *cpu_addr = vrp->rbufs + i * vrp->buf_size;
 
@@ -999,7 +999,8 @@ static int rpmsg_remove_device(struct device *dev, void *data)
 static void rpmsg_remove(struct virtio_device *vdev)
 {
 	struct virtproc_info *vrp = vdev->priv;
-	size_t total_buf_space = vrp->num_bufs * vrp->buf_size;
+	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);
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 2/3] rpmsg: virtio_rpmsg_bus: allocate rx/tx buffer separately
  2019-01-31 15:41 [PATCH 0/3] Enhance virtio rpmsg bus driver buffer allocation Xiang Xiao
  2019-01-31 15:41 ` [PATCH 1/3] rpmsg: virtio_rpmsg_bus: allow the different vring size for send/recv Xiang Xiao
@ 2019-01-31 15:41 ` Xiang Xiao
  2019-05-09 12:02     ` Arnaud Pouliquen
  2019-01-31 15:41 ` [PATCH 3/3] rpmsg: virtio_rpmsg_bus: get buffer size from config space Xiang Xiao
  2019-06-05  4:34 ` [PATCH 0/3] Enhance virtio rpmsg bus driver buffer allocation Bjorn Andersson
  3 siblings, 1 reply; 23+ messages in thread
From: Xiang Xiao @ 2019-01-31 15:41 UTC (permalink / raw)
  To: ohad, bjorn.andersson, wendy.liang, arnaud.pouliquen,
	linux-remoteproc, linux-kernel
  Cc: Xiang Xiao

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);
 }
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH 3/3] rpmsg: virtio_rpmsg_bus: get buffer size from config space
  2019-01-31 15:41 [PATCH 0/3] Enhance virtio rpmsg bus driver buffer allocation Xiang Xiao
  2019-01-31 15:41 ` [PATCH 1/3] rpmsg: virtio_rpmsg_bus: allow the different vring size for send/recv Xiang Xiao
  2019-01-31 15:41 ` [PATCH 2/3] rpmsg: virtio_rpmsg_bus: allocate rx/tx buffer separately Xiang Xiao
@ 2019-01-31 15:41 ` Xiang Xiao
  2019-05-09 12:36     ` Arnaud Pouliquen
  2019-06-05  4:34 ` [PATCH 0/3] Enhance virtio rpmsg bus driver buffer allocation Bjorn Andersson
  3 siblings, 1 reply; 23+ messages in thread
From: Xiang Xiao @ 2019-01-31 15:41 UTC (permalink / raw)
  To: ohad, bjorn.andersson, wendy.liang, arnaud.pouliquen,
	linux-remoteproc, linux-kernel
  Cc: Xiang Xiao

512 bytes isn't always suitable for all case, let firmware
maker decide the best value from resource table.
enable by VIRTIO_RPMSG_F_BUFSZ feature bit.

Signed-off-by: Xiang Xiao <xiaoxiang@xiaomi.com>
---
 drivers/rpmsg/virtio_rpmsg_bus.c  | 50 +++++++++++++++++++++++++--------------
 include/uapi/linux/virtio_rpmsg.h | 24 +++++++++++++++++++
 2 files changed, 56 insertions(+), 18 deletions(-)
 create mode 100644 include/uapi/linux/virtio_rpmsg.h

diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
index 59c4554..049dd97 100644
--- a/drivers/rpmsg/virtio_rpmsg_bus.c
+++ b/drivers/rpmsg/virtio_rpmsg_bus.c
@@ -16,6 +16,7 @@
 #include <linux/virtio.h>
 #include <linux/virtio_ids.h>
 #include <linux/virtio_config.h>
+#include <linux/virtio_rpmsg.h>
 #include <linux/scatterlist.h>
 #include <linux/dma-mapping.h>
 #include <linux/slab.h>
@@ -38,7 +39,8 @@
  * @sbufs:	kernel address of tx buffers
  * @num_rbufs:	total number of buffers for rx
  * @num_sbufs:	total number of buffers for tx
- * @buf_size:	size of one rx or tx buffer
+ * @rbuf_size:	size of one rx buffer
+ * @sbuf_size:	size of one tx buffer
  * @last_sbuf:	index of last tx buffer used
  * @rbufs_dma:	dma base addr of rx buffers
  * @sbufs_dma:	dma base addr of tx buffers
@@ -61,7 +63,8 @@ struct virtproc_info {
 	void *rbufs, *sbufs;
 	unsigned int num_rbufs;
 	unsigned int num_sbufs;
-	unsigned int buf_size;
+	unsigned int rbuf_size;
+	unsigned int sbuf_size;
 	int last_sbuf;
 	dma_addr_t rbufs_dma;
 	dma_addr_t sbufs_dma;
@@ -73,9 +76,6 @@ struct virtproc_info {
 	struct rpmsg_endpoint *ns_ept;
 };
 
-/* The feature bitmap for virtio rpmsg */
-#define VIRTIO_RPMSG_F_NS	0 /* RP supports name service notifications */
-
 /**
  * struct rpmsg_hdr - common header for all rpmsg messages
  * @src: source address
@@ -452,7 +452,7 @@ static void *get_a_tx_buf(struct virtproc_info *vrp)
 
 	/* either pick the next unused tx buffer */
 	if (vrp->last_sbuf < vrp->num_sbufs)
-		ret = vrp->sbufs + vrp->buf_size * vrp->last_sbuf++;
+		ret = vrp->sbufs + vrp->sbuf_size * vrp->last_sbuf++;
 	/* or recycle a used one */
 	else
 		ret = virtqueue_get_buf(vrp->svq, &len);
@@ -578,7 +578,7 @@ static int rpmsg_send_offchannel_raw(struct rpmsg_device *rpdev,
 	 * messaging), or to improve the buffer allocator, to support
 	 * variable-length buffer sizes.
 	 */
-	if (len > vrp->buf_size - sizeof(struct rpmsg_hdr)) {
+	if (len > vrp->sbuf_size - sizeof(struct rpmsg_hdr)) {
 		dev_err(dev, "message is too big (%d)\n", len);
 		return -EMSGSIZE;
 	}
@@ -718,7 +718,7 @@ static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
 	 * We currently use fixed-sized buffers, so trivially sanitize
 	 * the reported payload length.
 	 */
-	if (len > vrp->buf_size ||
+	if (len > vrp->rbuf_size ||
 	    msg->len > (len - sizeof(struct rpmsg_hdr))) {
 		dev_warn(dev, "inbound msg too big: (%d, %d)\n", len, msg->len);
 		return -EINVAL;
@@ -751,7 +751,7 @@ static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
 		dev_warn(dev, "msg received with no recipient\n");
 
 	/* publish the real size of the buffer */
-	rpmsg_sg_init(&sg, msg, vrp->buf_size);
+	rpmsg_sg_init(&sg, msg, vrp->rbuf_size);
 
 	/* add the buffer back to the remote processor's virtqueue */
 	err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, msg, GFP_KERNEL);
@@ -907,11 +907,24 @@ static int rpmsg_probe(struct virtio_device *vdev)
 	else
 		vrp->num_sbufs = MAX_RPMSG_NUM_BUFS;
 
-	vrp->buf_size = MAX_RPMSG_BUF_SIZE;
+	/* try to get buffer size from config space */
+	if (virtio_has_feature(vdev, VIRTIO_RPMSG_F_BUFSZ)) {
+		/* note: virtio_rpmsg_config is defined from remote view */
+		virtio_cread(vdev, struct virtio_rpmsg_config,
+			     txbuf_size, &vrp->rbuf_size);
+		virtio_cread(vdev, struct virtio_rpmsg_config,
+			     rxbuf_size, &vrp->sbuf_size);
+	}
+
+	/* use the default if resource table doesn't provide one */
+	if (!vrp->rbuf_size)
+		vrp->rbuf_size = MAX_RPMSG_BUF_SIZE;
+	if (!vrp->sbuf_size)
+		vrp->sbuf_size = MAX_RPMSG_BUF_SIZE;
 
 	/* allocate coherent memory for the buffers */
 	vrp->rbufs = dma_alloc_coherent(vdev->dev.parent->parent,
-					vrp->num_rbufs * vrp->buf_size,
+					vrp->num_rbufs * vrp->rbuf_size,
 					&vrp->rbufs_dma, GFP_KERNEL);
 	if (!vrp->rbufs) {
 		err = -ENOMEM;
@@ -922,7 +935,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
 		vrp->rbufs, &vrp->rbufs_dma);
 
 	vrp->sbufs = dma_alloc_coherent(vdev->dev.parent->parent,
-					vrp->num_sbufs * vrp->buf_size,
+					vrp->num_sbufs * vrp->sbuf_size,
 					&vrp->sbufs_dma, GFP_KERNEL);
 	if (!vrp->sbufs) {
 		err = -ENOMEM;
@@ -935,9 +948,9 @@ static int rpmsg_probe(struct virtio_device *vdev)
 	/* set up the receive buffers */
 	for (i = 0; i < vrp->num_rbufs; i++) {
 		struct scatterlist sg;
-		void *cpu_addr = vrp->rbufs + i * vrp->buf_size;
+		void *cpu_addr = vrp->rbufs + i * vrp->rbuf_size;
 
-		rpmsg_sg_init(&sg, cpu_addr, vrp->buf_size);
+		rpmsg_sg_init(&sg, cpu_addr, vrp->rbuf_size);
 
 		err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, cpu_addr,
 					  GFP_KERNEL);
@@ -984,11 +997,11 @@ static int rpmsg_probe(struct virtio_device *vdev)
 
 free_sbufs:
 	dma_free_coherent(vdev->dev.parent->parent,
-			  vrp->num_sbufs * vrp->buf_size,
+			  vrp->num_sbufs * vrp->sbuf_size,
 			  vrp->sbufs, vrp->sbufs_dma);
 free_rbufs:
 	dma_free_coherent(vdev->dev.parent->parent,
-			  vrp->num_rbufs * vrp->buf_size,
+			  vrp->num_rbufs * vrp->rbuf_size,
 			  vrp->rbufs, vrp->rbufs_dma);
 vqs_del:
 	vdev->config->del_vqs(vrp->vdev);
@@ -1023,10 +1036,10 @@ static void rpmsg_remove(struct virtio_device *vdev)
 	vdev->config->del_vqs(vrp->vdev);
 
 	dma_free_coherent(vdev->dev.parent->parent,
-			  vrp->num_sbufs * vrp->buf_size,
+			  vrp->num_sbufs * vrp->sbuf_size,
 			  vrp->sbufs, vrp->sbufs_dma);
 	dma_free_coherent(vdev->dev.parent->parent,
-			  vrp->num_rbufs * vrp->buf_size,
+			  vrp->num_rbufs * vrp->rbuf_size,
 			  vrp->rbufs, vrp->rbufs_dma);
 
 	kfree(vrp);
@@ -1039,6 +1052,7 @@ static struct virtio_device_id id_table[] = {
 
 static unsigned int features[] = {
 	VIRTIO_RPMSG_F_NS,
+	VIRTIO_RPMSG_F_BUFSZ,
 };
 
 static struct virtio_driver virtio_ipc_driver = {
diff --git a/include/uapi/linux/virtio_rpmsg.h b/include/uapi/linux/virtio_rpmsg.h
new file mode 100644
index 0000000..24fa0dd
--- /dev/null
+++ b/include/uapi/linux/virtio_rpmsg.h
@@ -0,0 +1,24 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+/*
+ * Copyright (C) Pinecone Inc. 2019
+ * Copyright (C) Xiang Xiao <xiaoxiang@pinecone.net>
+ */
+
+#ifndef _UAPI_LINUX_VIRTIO_RPMSG_H
+#define _UAPI_LINUX_VIRTIO_RPMSG_H
+
+#include <linux/types.h>
+
+/* The feature bitmap for virtio rpmsg */
+#define VIRTIO_RPMSG_F_NS	0 /* RP supports name service notifications */
+#define VIRTIO_RPMSG_F_BUFSZ	2 /* RP get buffer size from config space */
+
+struct virtio_rpmsg_config {
+	/* The tx/rx individual buffer size(if VIRTIO_RPMSG_F_BUFSZ) */
+	__u32 txbuf_size;
+	__u32 rxbuf_size;
+	__u32 reserved[14]; /* Reserve for the future use */
+	/* Put the customize config here */
+} __attribute__((packed));
+
+#endif /* _UAPI_LINUX_VIRTIO_RPMSG_H */
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* Re: [PATCH 1/3] rpmsg: virtio_rpmsg_bus: allow the different vring size for send/recv
  2019-01-31 15:41 ` [PATCH 1/3] rpmsg: virtio_rpmsg_bus: allow the different vring size for send/recv Xiang Xiao
@ 2019-05-09 11:47     ` Arnaud Pouliquen
  0 siblings, 0 replies; 23+ messages in thread
From: Arnaud Pouliquen @ 2019-05-09 11:47 UTC (permalink / raw)
  To: Xiang Xiao, ohad, bjorn.andersson, wendy.liang, linux-remoteproc,
	linux-kernel
  Cc: Xiang Xiao

Hello Xiang,

I tested your patch on stm32 platform, no issue.
No remark on you code.

Acked-by:Arnaud POULIQUEN <arnaud.pouliquen@st.com>

Thanks,
Arnaud

On 1/31/19 4:41 PM, Xiang Xiao wrote:
> it's useful if the communication throughput is different from each side
> 
> Signed-off-by: Xiang Xiao <xiaoxiang@xiaomi.com>
> ---
>  drivers/rpmsg/virtio_rpmsg_bus.c | 47 ++++++++++++++++++++--------------------
>  1 file changed, 24 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> index 664f957..fb0d2eb 100644
> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> @@ -36,8 +36,9 @@
>   * @svq:	tx virtqueue
>   * @rbufs:	kernel address of rx buffers
>   * @sbufs:	kernel address of tx buffers
> - * @num_bufs:	total number of buffers for rx and tx
> - * @buf_size:   size of one rx or tx buffer
> + * @num_rbufs:	total number of buffers for rx
> + * @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
>   * @tx_lock:	protects svq, sbufs and sleepers, to allow concurrent senders.
> @@ -57,7 +58,8 @@ struct virtproc_info {
>  	struct virtio_device *vdev;
>  	struct virtqueue *rvq, *svq;
>  	void *rbufs, *sbufs;
> -	unsigned int num_bufs;
> +	unsigned int num_rbufs;
> +	unsigned int num_sbufs;
>  	unsigned int buf_size;
>  	int last_sbuf;
>  	dma_addr_t bufs_dma;
> @@ -136,7 +138,7 @@ struct virtio_rpmsg_channel {
>  /*
>   * We're allocating buffers of 512 bytes each for communications. The
>   * number of buffers will be computed from the number of buffers supported
> - * by the vring, upto a maximum of 512 buffers (256 in each direction).
> + * by the vring, up to a maximum of 256 in each direction.
>   *
>   * Each buffer will have 16 bytes for the msg header and 496 bytes for
>   * the payload.
> @@ -151,7 +153,7 @@ struct virtio_rpmsg_channel {
>   * can change this without changing anything in the firmware of the remote
>   * processor.
>   */
> -#define MAX_RPMSG_NUM_BUFS	(512)
> +#define MAX_RPMSG_NUM_BUFS	(256)
>  #define MAX_RPMSG_BUF_SIZE	(512)
>  
>  /*
> @@ -446,11 +448,8 @@ static void *get_a_tx_buf(struct virtproc_info *vrp)
>  	/* support multiple concurrent senders */
>  	mutex_lock(&vrp->tx_lock);
>  
> -	/*
> -	 * either pick the next unused tx buffer
> -	 * (half of our buffers are used for sending messages)
> -	 */
> -	if (vrp->last_sbuf < vrp->num_bufs / 2)
> +	/* either pick the next unused tx buffer */
> +	if (vrp->last_sbuf < vrp->num_sbufs)
>  		ret = vrp->sbufs + vrp->buf_size * vrp->last_sbuf++;
>  	/* or recycle a used one */
>  	else
> @@ -897,19 +896,20 @@ static int rpmsg_probe(struct virtio_device *vdev)
>  	vrp->rvq = vqs[0];
>  	vrp->svq = vqs[1];
>  
> -	/* we expect symmetric tx/rx vrings */
> -	WARN_ON(virtqueue_get_vring_size(vrp->rvq) !=
> -		virtqueue_get_vring_size(vrp->svq));
> -
>  	/* we need less buffers if vrings are small */
> -	if (virtqueue_get_vring_size(vrp->rvq) < MAX_RPMSG_NUM_BUFS / 2)
> -		vrp->num_bufs = virtqueue_get_vring_size(vrp->rvq) * 2;
> +	if (virtqueue_get_vring_size(vrp->rvq) < MAX_RPMSG_NUM_BUFS)
> +		vrp->num_rbufs = virtqueue_get_vring_size(vrp->rvq);
> +	else
> +		vrp->num_rbufs = MAX_RPMSG_NUM_BUFS;
> +
> +	if (virtqueue_get_vring_size(vrp->svq) < MAX_RPMSG_NUM_BUFS)
> +		vrp->num_sbufs = virtqueue_get_vring_size(vrp->svq);
>  	else
> -		vrp->num_bufs = MAX_RPMSG_NUM_BUFS;
> +		vrp->num_sbufs = MAX_RPMSG_NUM_BUFS;
>  
>  	vrp->buf_size = MAX_RPMSG_BUF_SIZE;
>  
> -	total_buf_space = vrp->num_bufs * vrp->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,
> @@ -923,14 +923,14 @@ static int rpmsg_probe(struct virtio_device *vdev)
>  	dev_dbg(&vdev->dev, "buffers: va %p, dma %pad\n",
>  		bufs_va, &vrp->bufs_dma);
>  
> -	/* half of the buffers is dedicated for RX */
> +	/* first part of the buffers is dedicated for RX */
>  	vrp->rbufs = bufs_va;
>  
> -	/* and half is dedicated for TX */
> -	vrp->sbufs = bufs_va + total_buf_space / 2;
> +	/* and second part is dedicated for TX */
> +	vrp->sbufs = bufs_va + vrp->num_rbufs * vrp->buf_size;
>  
>  	/* set up the receive buffers */
> -	for (i = 0; i < vrp->num_bufs / 2; i++) {
> +	for (i = 0; i < vrp->num_rbufs; i++) {
>  		struct scatterlist sg;
>  		void *cpu_addr = vrp->rbufs + i * vrp->buf_size;
>  
> @@ -999,7 +999,8 @@ static int rpmsg_remove_device(struct device *dev, void *data)
>  static void rpmsg_remove(struct virtio_device *vdev)
>  {
>  	struct virtproc_info *vrp = vdev->priv;
> -	size_t total_buf_space = vrp->num_bufs * vrp->buf_size;
> +	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);
> 

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 1/3] rpmsg: virtio_rpmsg_bus: allow the different vring size for send/recv
@ 2019-05-09 11:47     ` Arnaud Pouliquen
  0 siblings, 0 replies; 23+ messages in thread
From: Arnaud Pouliquen @ 2019-05-09 11:47 UTC (permalink / raw)
  To: Xiang Xiao, ohad, bjorn.andersson, wendy.liang, linux-remoteproc,
	linux-kernel
  Cc: Xiang Xiao

Hello Xiang,

I tested your patch on stm32 platform, no issue.
No remark on you code.

Acked-by:Arnaud POULIQUEN <arnaud.pouliquen@st.com>

Thanks,
Arnaud

On 1/31/19 4:41 PM, Xiang Xiao wrote:
> it's useful if the communication throughput is different from each side
> 
> Signed-off-by: Xiang Xiao <xiaoxiang@xiaomi.com>
> ---
>  drivers/rpmsg/virtio_rpmsg_bus.c | 47 ++++++++++++++++++++--------------------
>  1 file changed, 24 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> index 664f957..fb0d2eb 100644
> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> @@ -36,8 +36,9 @@
>   * @svq:	tx virtqueue
>   * @rbufs:	kernel address of rx buffers
>   * @sbufs:	kernel address of tx buffers
> - * @num_bufs:	total number of buffers for rx and tx
> - * @buf_size:   size of one rx or tx buffer
> + * @num_rbufs:	total number of buffers for rx
> + * @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
>   * @tx_lock:	protects svq, sbufs and sleepers, to allow concurrent senders.
> @@ -57,7 +58,8 @@ struct virtproc_info {
>  	struct virtio_device *vdev;
>  	struct virtqueue *rvq, *svq;
>  	void *rbufs, *sbufs;
> -	unsigned int num_bufs;
> +	unsigned int num_rbufs;
> +	unsigned int num_sbufs;
>  	unsigned int buf_size;
>  	int last_sbuf;
>  	dma_addr_t bufs_dma;
> @@ -136,7 +138,7 @@ struct virtio_rpmsg_channel {
>  /*
>   * We're allocating buffers of 512 bytes each for communications. The
>   * number of buffers will be computed from the number of buffers supported
> - * by the vring, upto a maximum of 512 buffers (256 in each direction).
> + * by the vring, up to a maximum of 256 in each direction.
>   *
>   * Each buffer will have 16 bytes for the msg header and 496 bytes for
>   * the payload.
> @@ -151,7 +153,7 @@ struct virtio_rpmsg_channel {
>   * can change this without changing anything in the firmware of the remote
>   * processor.
>   */
> -#define MAX_RPMSG_NUM_BUFS	(512)
> +#define MAX_RPMSG_NUM_BUFS	(256)
>  #define MAX_RPMSG_BUF_SIZE	(512)
>  
>  /*
> @@ -446,11 +448,8 @@ static void *get_a_tx_buf(struct virtproc_info *vrp)
>  	/* support multiple concurrent senders */
>  	mutex_lock(&vrp->tx_lock);
>  
> -	/*
> -	 * either pick the next unused tx buffer
> -	 * (half of our buffers are used for sending messages)
> -	 */
> -	if (vrp->last_sbuf < vrp->num_bufs / 2)
> +	/* either pick the next unused tx buffer */
> +	if (vrp->last_sbuf < vrp->num_sbufs)
>  		ret = vrp->sbufs + vrp->buf_size * vrp->last_sbuf++;
>  	/* or recycle a used one */
>  	else
> @@ -897,19 +896,20 @@ static int rpmsg_probe(struct virtio_device *vdev)
>  	vrp->rvq = vqs[0];
>  	vrp->svq = vqs[1];
>  
> -	/* we expect symmetric tx/rx vrings */
> -	WARN_ON(virtqueue_get_vring_size(vrp->rvq) !=
> -		virtqueue_get_vring_size(vrp->svq));
> -
>  	/* we need less buffers if vrings are small */
> -	if (virtqueue_get_vring_size(vrp->rvq) < MAX_RPMSG_NUM_BUFS / 2)
> -		vrp->num_bufs = virtqueue_get_vring_size(vrp->rvq) * 2;
> +	if (virtqueue_get_vring_size(vrp->rvq) < MAX_RPMSG_NUM_BUFS)
> +		vrp->num_rbufs = virtqueue_get_vring_size(vrp->rvq);
> +	else
> +		vrp->num_rbufs = MAX_RPMSG_NUM_BUFS;
> +
> +	if (virtqueue_get_vring_size(vrp->svq) < MAX_RPMSG_NUM_BUFS)
> +		vrp->num_sbufs = virtqueue_get_vring_size(vrp->svq);
>  	else
> -		vrp->num_bufs = MAX_RPMSG_NUM_BUFS;
> +		vrp->num_sbufs = MAX_RPMSG_NUM_BUFS;
>  
>  	vrp->buf_size = MAX_RPMSG_BUF_SIZE;
>  
> -	total_buf_space = vrp->num_bufs * vrp->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,
> @@ -923,14 +923,14 @@ static int rpmsg_probe(struct virtio_device *vdev)
>  	dev_dbg(&vdev->dev, "buffers: va %p, dma %pad\n",
>  		bufs_va, &vrp->bufs_dma);
>  
> -	/* half of the buffers is dedicated for RX */
> +	/* first part of the buffers is dedicated for RX */
>  	vrp->rbufs = bufs_va;
>  
> -	/* and half is dedicated for TX */
> -	vrp->sbufs = bufs_va + total_buf_space / 2;
> +	/* and second part is dedicated for TX */
> +	vrp->sbufs = bufs_va + vrp->num_rbufs * vrp->buf_size;
>  
>  	/* set up the receive buffers */
> -	for (i = 0; i < vrp->num_bufs / 2; i++) {
> +	for (i = 0; i < vrp->num_rbufs; i++) {
>  		struct scatterlist sg;
>  		void *cpu_addr = vrp->rbufs + i * vrp->buf_size;
>  
> @@ -999,7 +999,8 @@ static int rpmsg_remove_device(struct device *dev, void *data)
>  static void rpmsg_remove(struct virtio_device *vdev)
>  {
>  	struct virtproc_info *vrp = vdev->priv;
> -	size_t total_buf_space = vrp->num_bufs * vrp->buf_size;
> +	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);
> 

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 2/3] rpmsg: virtio_rpmsg_bus: allocate rx/tx buffer separately
  2019-01-31 15:41 ` [PATCH 2/3] rpmsg: virtio_rpmsg_bus: allocate rx/tx buffer separately Xiang Xiao
@ 2019-05-09 12:02     ` Arnaud Pouliquen
  0 siblings, 0 replies; 23+ messages in thread
From: Arnaud Pouliquen @ 2019-05-09 12:02 UTC (permalink / raw)
  To: Xiang Xiao, ohad, bjorn.andersson, wendy.liang, linux-remoteproc,
	linux-kernel
  Cc: Xiang Xiao

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)

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);
>  }
> 

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 2/3] rpmsg: virtio_rpmsg_bus: allocate rx/tx buffer separately
@ 2019-05-09 12:02     ` Arnaud Pouliquen
  0 siblings, 0 replies; 23+ messages in thread
From: Arnaud Pouliquen @ 2019-05-09 12:02 UTC (permalink / raw)
  To: Xiang Xiao, ohad, bjorn.andersson, wendy.liang, linux-remoteproc,
	linux-kernel
  Cc: Xiang Xiao

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)

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);
>  }
> 

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 3/3] rpmsg: virtio_rpmsg_bus: get buffer size from config space
  2019-01-31 15:41 ` [PATCH 3/3] rpmsg: virtio_rpmsg_bus: get buffer size from config space Xiang Xiao
@ 2019-05-09 12:36     ` Arnaud Pouliquen
  0 siblings, 0 replies; 23+ messages in thread
From: Arnaud Pouliquen @ 2019-05-09 12:36 UTC (permalink / raw)
  To: Xiang Xiao, ohad, bjorn.andersson, wendy.liang, linux-remoteproc,
	linux-kernel
  Cc: Xiang Xiao, Loic PALLARDY, Suman Anna

Hello Xiang,

Similar mechanism has been proposed by Loic 2 years ago (link to the
series here https://lkml.org/lkml/2017/3/28/349).

Did you see them? Regarding history, patches seem just on hold...

Main differences (except interesting RX/TX size split) seems that you
- don't use the virtio_config_ops->get
- define a new feature VIRTIO_RPMSG_F_NS.

Regards
Arnaud


On 1/31/19 4:41 PM, Xiang Xiao wrote:
> 512 bytes isn't always suitable for all case, let firmware
> maker decide the best value from resource table.
> enable by VIRTIO_RPMSG_F_BUFSZ feature bit.
> 
> Signed-off-by: Xiang Xiao <xiaoxiang@xiaomi.com>
> ---
>  drivers/rpmsg/virtio_rpmsg_bus.c  | 50 +++++++++++++++++++++++++--------------
>  include/uapi/linux/virtio_rpmsg.h | 24 +++++++++++++++++++
>  2 files changed, 56 insertions(+), 18 deletions(-)
>  create mode 100644 include/uapi/linux/virtio_rpmsg.h
> 
> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> index 59c4554..049dd97 100644
> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> @@ -16,6 +16,7 @@
>  #include <linux/virtio.h>
>  #include <linux/virtio_ids.h>
>  #include <linux/virtio_config.h>
> +#include <linux/virtio_rpmsg.h>
>  #include <linux/scatterlist.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/slab.h>
> @@ -38,7 +39,8 @@
>   * @sbufs:	kernel address of tx buffers
>   * @num_rbufs:	total number of buffers for rx
>   * @num_sbufs:	total number of buffers for tx
> - * @buf_size:	size of one rx or tx buffer
> + * @rbuf_size:	size of one rx buffer
> + * @sbuf_size:	size of one tx buffer
>   * @last_sbuf:	index of last tx buffer used
>   * @rbufs_dma:	dma base addr of rx buffers
>   * @sbufs_dma:	dma base addr of tx buffers
> @@ -61,7 +63,8 @@ struct virtproc_info {
>  	void *rbufs, *sbufs;
>  	unsigned int num_rbufs;
>  	unsigned int num_sbufs;
> -	unsigned int buf_size;
> +	unsigned int rbuf_size;
> +	unsigned int sbuf_size;
>  	int last_sbuf;
>  	dma_addr_t rbufs_dma;
>  	dma_addr_t sbufs_dma;
> @@ -73,9 +76,6 @@ struct virtproc_info {
>  	struct rpmsg_endpoint *ns_ept;
>  };
>  
> -/* The feature bitmap for virtio rpmsg */
> -#define VIRTIO_RPMSG_F_NS	0 /* RP supports name service notifications */
> -
>  /**
>   * struct rpmsg_hdr - common header for all rpmsg messages
>   * @src: source address
> @@ -452,7 +452,7 @@ static void *get_a_tx_buf(struct virtproc_info *vrp)
>  
>  	/* either pick the next unused tx buffer */
>  	if (vrp->last_sbuf < vrp->num_sbufs)
> -		ret = vrp->sbufs + vrp->buf_size * vrp->last_sbuf++;
> +		ret = vrp->sbufs + vrp->sbuf_size * vrp->last_sbuf++;
>  	/* or recycle a used one */
>  	else
>  		ret = virtqueue_get_buf(vrp->svq, &len);
> @@ -578,7 +578,7 @@ static int rpmsg_send_offchannel_raw(struct rpmsg_device *rpdev,
>  	 * messaging), or to improve the buffer allocator, to support
>  	 * variable-length buffer sizes.
>  	 */
> -	if (len > vrp->buf_size - sizeof(struct rpmsg_hdr)) {
> +	if (len > vrp->sbuf_size - sizeof(struct rpmsg_hdr)) {
>  		dev_err(dev, "message is too big (%d)\n", len);
>  		return -EMSGSIZE;
>  	}
> @@ -718,7 +718,7 @@ static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
>  	 * We currently use fixed-sized buffers, so trivially sanitize
>  	 * the reported payload length.
>  	 */
> -	if (len > vrp->buf_size ||
> +	if (len > vrp->rbuf_size ||
>  	    msg->len > (len - sizeof(struct rpmsg_hdr))) {
>  		dev_warn(dev, "inbound msg too big: (%d, %d)\n", len, msg->len);
>  		return -EINVAL;
> @@ -751,7 +751,7 @@ static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
>  		dev_warn(dev, "msg received with no recipient\n");
>  
>  	/* publish the real size of the buffer */
> -	rpmsg_sg_init(&sg, msg, vrp->buf_size);
> +	rpmsg_sg_init(&sg, msg, vrp->rbuf_size);
>  
>  	/* add the buffer back to the remote processor's virtqueue */
>  	err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, msg, GFP_KERNEL);
> @@ -907,11 +907,24 @@ static int rpmsg_probe(struct virtio_device *vdev)
>  	else
>  		vrp->num_sbufs = MAX_RPMSG_NUM_BUFS;
>  
> -	vrp->buf_size = MAX_RPMSG_BUF_SIZE;
> +	/* try to get buffer size from config space */
> +	if (virtio_has_feature(vdev, VIRTIO_RPMSG_F_BUFSZ)) {
> +		/* note: virtio_rpmsg_config is defined from remote view */
> +		virtio_cread(vdev, struct virtio_rpmsg_config,
> +			     txbuf_size, &vrp->rbuf_size);
> +		virtio_cread(vdev, struct virtio_rpmsg_config,
> +			     rxbuf_size, &vrp->sbuf_size);
> +	}
> +
> +	/* use the default if resource table doesn't provide one */
> +	if (!vrp->rbuf_size)
> +		vrp->rbuf_size = MAX_RPMSG_BUF_SIZE;
> +	if (!vrp->sbuf_size)
> +		vrp->sbuf_size = MAX_RPMSG_BUF_SIZE;
>  
>  	/* allocate coherent memory for the buffers */
>  	vrp->rbufs = dma_alloc_coherent(vdev->dev.parent->parent,
> -					vrp->num_rbufs * vrp->buf_size,
> +					vrp->num_rbufs * vrp->rbuf_size,
>  					&vrp->rbufs_dma, GFP_KERNEL);
>  	if (!vrp->rbufs) {
>  		err = -ENOMEM;
> @@ -922,7 +935,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
>  		vrp->rbufs, &vrp->rbufs_dma);
>  
>  	vrp->sbufs = dma_alloc_coherent(vdev->dev.parent->parent,
> -					vrp->num_sbufs * vrp->buf_size,
> +					vrp->num_sbufs * vrp->sbuf_size,
>  					&vrp->sbufs_dma, GFP_KERNEL);
>  	if (!vrp->sbufs) {
>  		err = -ENOMEM;
> @@ -935,9 +948,9 @@ static int rpmsg_probe(struct virtio_device *vdev)
>  	/* set up the receive buffers */
>  	for (i = 0; i < vrp->num_rbufs; i++) {
>  		struct scatterlist sg;
> -		void *cpu_addr = vrp->rbufs + i * vrp->buf_size;
> +		void *cpu_addr = vrp->rbufs + i * vrp->rbuf_size;
>  
> -		rpmsg_sg_init(&sg, cpu_addr, vrp->buf_size);
> +		rpmsg_sg_init(&sg, cpu_addr, vrp->rbuf_size);
>  
>  		err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, cpu_addr,
>  					  GFP_KERNEL);
> @@ -984,11 +997,11 @@ static int rpmsg_probe(struct virtio_device *vdev)
>  
>  free_sbufs:
>  	dma_free_coherent(vdev->dev.parent->parent,
> -			  vrp->num_sbufs * vrp->buf_size,
> +			  vrp->num_sbufs * vrp->sbuf_size,
>  			  vrp->sbufs, vrp->sbufs_dma);
>  free_rbufs:
>  	dma_free_coherent(vdev->dev.parent->parent,
> -			  vrp->num_rbufs * vrp->buf_size,
> +			  vrp->num_rbufs * vrp->rbuf_size,
>  			  vrp->rbufs, vrp->rbufs_dma);
>  vqs_del:
>  	vdev->config->del_vqs(vrp->vdev);
> @@ -1023,10 +1036,10 @@ static void rpmsg_remove(struct virtio_device *vdev)
>  	vdev->config->del_vqs(vrp->vdev);
>  
>  	dma_free_coherent(vdev->dev.parent->parent,
> -			  vrp->num_sbufs * vrp->buf_size,
> +			  vrp->num_sbufs * vrp->sbuf_size,
>  			  vrp->sbufs, vrp->sbufs_dma);
>  	dma_free_coherent(vdev->dev.parent->parent,
> -			  vrp->num_rbufs * vrp->buf_size,
> +			  vrp->num_rbufs * vrp->rbuf_size,
>  			  vrp->rbufs, vrp->rbufs_dma);
>  
>  	kfree(vrp);
> @@ -1039,6 +1052,7 @@ static struct virtio_device_id id_table[] = {
>  
>  static unsigned int features[] = {
>  	VIRTIO_RPMSG_F_NS,
> +	VIRTIO_RPMSG_F_BUFSZ,
>  };
>  
>  static struct virtio_driver virtio_ipc_driver = {
> diff --git a/include/uapi/linux/virtio_rpmsg.h b/include/uapi/linux/virtio_rpmsg.h
> new file mode 100644
> index 0000000..24fa0dd
> --- /dev/null
> +++ b/include/uapi/linux/virtio_rpmsg.h
> @@ -0,0 +1,24 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +/*
> + * Copyright (C) Pinecone Inc. 2019
> + * Copyright (C) Xiang Xiao <xiaoxiang@pinecone.net>
> + */
> +
> +#ifndef _UAPI_LINUX_VIRTIO_RPMSG_H
> +#define _UAPI_LINUX_VIRTIO_RPMSG_H
> +
> +#include <linux/types.h>
> +
> +/* The feature bitmap for virtio rpmsg */
> +#define VIRTIO_RPMSG_F_NS	0 /* RP supports name service notifications */
> +#define VIRTIO_RPMSG_F_BUFSZ	2 /* RP get buffer size from config space */
> +
> +struct virtio_rpmsg_config {
> +	/* The tx/rx individual buffer size(if VIRTIO_RPMSG_F_BUFSZ) */
> +	__u32 txbuf_size;
> +	__u32 rxbuf_size;
> +	__u32 reserved[14]; /* Reserve for the future use */
> +	/* Put the customize config here */
> +} __attribute__((packed));
> +
> +#endif /* _UAPI_LINUX_VIRTIO_RPMSG_H */
> 

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 3/3] rpmsg: virtio_rpmsg_bus: get buffer size from config space
@ 2019-05-09 12:36     ` Arnaud Pouliquen
  0 siblings, 0 replies; 23+ messages in thread
From: Arnaud Pouliquen @ 2019-05-09 12:36 UTC (permalink / raw)
  To: Xiang Xiao, ohad, bjorn.andersson, wendy.liang, linux-remoteproc,
	linux-kernel
  Cc: Xiang Xiao, Loic PALLARDY, Suman Anna

Hello Xiang,

Similar mechanism has been proposed by Loic 2 years ago (link to the
series here https://lkml.org/lkml/2017/3/28/349).

Did you see them? Regarding history, patches seem just on hold...

Main differences (except interesting RX/TX size split) seems that you
- don't use the virtio_config_ops->get
- define a new feature VIRTIO_RPMSG_F_NS.

Regards
Arnaud


On 1/31/19 4:41 PM, Xiang Xiao wrote:
> 512 bytes isn't always suitable for all case, let firmware
> maker decide the best value from resource table.
> enable by VIRTIO_RPMSG_F_BUFSZ feature bit.
> 
> Signed-off-by: Xiang Xiao <xiaoxiang@xiaomi.com>
> ---
>  drivers/rpmsg/virtio_rpmsg_bus.c  | 50 +++++++++++++++++++++++++--------------
>  include/uapi/linux/virtio_rpmsg.h | 24 +++++++++++++++++++
>  2 files changed, 56 insertions(+), 18 deletions(-)
>  create mode 100644 include/uapi/linux/virtio_rpmsg.h
> 
> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> index 59c4554..049dd97 100644
> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> @@ -16,6 +16,7 @@
>  #include <linux/virtio.h>
>  #include <linux/virtio_ids.h>
>  #include <linux/virtio_config.h>
> +#include <linux/virtio_rpmsg.h>
>  #include <linux/scatterlist.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/slab.h>
> @@ -38,7 +39,8 @@
>   * @sbufs:	kernel address of tx buffers
>   * @num_rbufs:	total number of buffers for rx
>   * @num_sbufs:	total number of buffers for tx
> - * @buf_size:	size of one rx or tx buffer
> + * @rbuf_size:	size of one rx buffer
> + * @sbuf_size:	size of one tx buffer
>   * @last_sbuf:	index of last tx buffer used
>   * @rbufs_dma:	dma base addr of rx buffers
>   * @sbufs_dma:	dma base addr of tx buffers
> @@ -61,7 +63,8 @@ struct virtproc_info {
>  	void *rbufs, *sbufs;
>  	unsigned int num_rbufs;
>  	unsigned int num_sbufs;
> -	unsigned int buf_size;
> +	unsigned int rbuf_size;
> +	unsigned int sbuf_size;
>  	int last_sbuf;
>  	dma_addr_t rbufs_dma;
>  	dma_addr_t sbufs_dma;
> @@ -73,9 +76,6 @@ struct virtproc_info {
>  	struct rpmsg_endpoint *ns_ept;
>  };
>  
> -/* The feature bitmap for virtio rpmsg */
> -#define VIRTIO_RPMSG_F_NS	0 /* RP supports name service notifications */
> -
>  /**
>   * struct rpmsg_hdr - common header for all rpmsg messages
>   * @src: source address
> @@ -452,7 +452,7 @@ static void *get_a_tx_buf(struct virtproc_info *vrp)
>  
>  	/* either pick the next unused tx buffer */
>  	if (vrp->last_sbuf < vrp->num_sbufs)
> -		ret = vrp->sbufs + vrp->buf_size * vrp->last_sbuf++;
> +		ret = vrp->sbufs + vrp->sbuf_size * vrp->last_sbuf++;
>  	/* or recycle a used one */
>  	else
>  		ret = virtqueue_get_buf(vrp->svq, &len);
> @@ -578,7 +578,7 @@ static int rpmsg_send_offchannel_raw(struct rpmsg_device *rpdev,
>  	 * messaging), or to improve the buffer allocator, to support
>  	 * variable-length buffer sizes.
>  	 */
> -	if (len > vrp->buf_size - sizeof(struct rpmsg_hdr)) {
> +	if (len > vrp->sbuf_size - sizeof(struct rpmsg_hdr)) {
>  		dev_err(dev, "message is too big (%d)\n", len);
>  		return -EMSGSIZE;
>  	}
> @@ -718,7 +718,7 @@ static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
>  	 * We currently use fixed-sized buffers, so trivially sanitize
>  	 * the reported payload length.
>  	 */
> -	if (len > vrp->buf_size ||
> +	if (len > vrp->rbuf_size ||
>  	    msg->len > (len - sizeof(struct rpmsg_hdr))) {
>  		dev_warn(dev, "inbound msg too big: (%d, %d)\n", len, msg->len);
>  		return -EINVAL;
> @@ -751,7 +751,7 @@ static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
>  		dev_warn(dev, "msg received with no recipient\n");
>  
>  	/* publish the real size of the buffer */
> -	rpmsg_sg_init(&sg, msg, vrp->buf_size);
> +	rpmsg_sg_init(&sg, msg, vrp->rbuf_size);
>  
>  	/* add the buffer back to the remote processor's virtqueue */
>  	err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, msg, GFP_KERNEL);
> @@ -907,11 +907,24 @@ static int rpmsg_probe(struct virtio_device *vdev)
>  	else
>  		vrp->num_sbufs = MAX_RPMSG_NUM_BUFS;
>  
> -	vrp->buf_size = MAX_RPMSG_BUF_SIZE;
> +	/* try to get buffer size from config space */
> +	if (virtio_has_feature(vdev, VIRTIO_RPMSG_F_BUFSZ)) {
> +		/* note: virtio_rpmsg_config is defined from remote view */
> +		virtio_cread(vdev, struct virtio_rpmsg_config,
> +			     txbuf_size, &vrp->rbuf_size);
> +		virtio_cread(vdev, struct virtio_rpmsg_config,
> +			     rxbuf_size, &vrp->sbuf_size);
> +	}
> +
> +	/* use the default if resource table doesn't provide one */
> +	if (!vrp->rbuf_size)
> +		vrp->rbuf_size = MAX_RPMSG_BUF_SIZE;
> +	if (!vrp->sbuf_size)
> +		vrp->sbuf_size = MAX_RPMSG_BUF_SIZE;
>  
>  	/* allocate coherent memory for the buffers */
>  	vrp->rbufs = dma_alloc_coherent(vdev->dev.parent->parent,
> -					vrp->num_rbufs * vrp->buf_size,
> +					vrp->num_rbufs * vrp->rbuf_size,
>  					&vrp->rbufs_dma, GFP_KERNEL);
>  	if (!vrp->rbufs) {
>  		err = -ENOMEM;
> @@ -922,7 +935,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
>  		vrp->rbufs, &vrp->rbufs_dma);
>  
>  	vrp->sbufs = dma_alloc_coherent(vdev->dev.parent->parent,
> -					vrp->num_sbufs * vrp->buf_size,
> +					vrp->num_sbufs * vrp->sbuf_size,
>  					&vrp->sbufs_dma, GFP_KERNEL);
>  	if (!vrp->sbufs) {
>  		err = -ENOMEM;
> @@ -935,9 +948,9 @@ static int rpmsg_probe(struct virtio_device *vdev)
>  	/* set up the receive buffers */
>  	for (i = 0; i < vrp->num_rbufs; i++) {
>  		struct scatterlist sg;
> -		void *cpu_addr = vrp->rbufs + i * vrp->buf_size;
> +		void *cpu_addr = vrp->rbufs + i * vrp->rbuf_size;
>  
> -		rpmsg_sg_init(&sg, cpu_addr, vrp->buf_size);
> +		rpmsg_sg_init(&sg, cpu_addr, vrp->rbuf_size);
>  
>  		err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, cpu_addr,
>  					  GFP_KERNEL);
> @@ -984,11 +997,11 @@ static int rpmsg_probe(struct virtio_device *vdev)
>  
>  free_sbufs:
>  	dma_free_coherent(vdev->dev.parent->parent,
> -			  vrp->num_sbufs * vrp->buf_size,
> +			  vrp->num_sbufs * vrp->sbuf_size,
>  			  vrp->sbufs, vrp->sbufs_dma);
>  free_rbufs:
>  	dma_free_coherent(vdev->dev.parent->parent,
> -			  vrp->num_rbufs * vrp->buf_size,
> +			  vrp->num_rbufs * vrp->rbuf_size,
>  			  vrp->rbufs, vrp->rbufs_dma);
>  vqs_del:
>  	vdev->config->del_vqs(vrp->vdev);
> @@ -1023,10 +1036,10 @@ static void rpmsg_remove(struct virtio_device *vdev)
>  	vdev->config->del_vqs(vrp->vdev);
>  
>  	dma_free_coherent(vdev->dev.parent->parent,
> -			  vrp->num_sbufs * vrp->buf_size,
> +			  vrp->num_sbufs * vrp->sbuf_size,
>  			  vrp->sbufs, vrp->sbufs_dma);
>  	dma_free_coherent(vdev->dev.parent->parent,
> -			  vrp->num_rbufs * vrp->buf_size,
> +			  vrp->num_rbufs * vrp->rbuf_size,
>  			  vrp->rbufs, vrp->rbufs_dma);
>  
>  	kfree(vrp);
> @@ -1039,6 +1052,7 @@ static struct virtio_device_id id_table[] = {
>  
>  static unsigned int features[] = {
>  	VIRTIO_RPMSG_F_NS,
> +	VIRTIO_RPMSG_F_BUFSZ,
>  };
>  
>  static struct virtio_driver virtio_ipc_driver = {
> diff --git a/include/uapi/linux/virtio_rpmsg.h b/include/uapi/linux/virtio_rpmsg.h
> new file mode 100644
> index 0000000..24fa0dd
> --- /dev/null
> +++ b/include/uapi/linux/virtio_rpmsg.h
> @@ -0,0 +1,24 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +/*
> + * Copyright (C) Pinecone Inc. 2019
> + * Copyright (C) Xiang Xiao <xiaoxiang@pinecone.net>
> + */
> +
> +#ifndef _UAPI_LINUX_VIRTIO_RPMSG_H
> +#define _UAPI_LINUX_VIRTIO_RPMSG_H
> +
> +#include <linux/types.h>
> +
> +/* The feature bitmap for virtio rpmsg */
> +#define VIRTIO_RPMSG_F_NS	0 /* RP supports name service notifications */
> +#define VIRTIO_RPMSG_F_BUFSZ	2 /* RP get buffer size from config space */
> +
> +struct virtio_rpmsg_config {
> +	/* The tx/rx individual buffer size(if VIRTIO_RPMSG_F_BUFSZ) */
> +	__u32 txbuf_size;
> +	__u32 rxbuf_size;
> +	__u32 reserved[14]; /* Reserve for the future use */
> +	/* Put the customize config here */
> +} __attribute__((packed));
> +
> +#endif /* _UAPI_LINUX_VIRTIO_RPMSG_H */
> 

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 2/3] rpmsg: virtio_rpmsg_bus: allocate rx/tx buffer separately
  2019-05-09 12:02     ` Arnaud Pouliquen
  (?)
@ 2019-05-09 12:37     ` xiang xiao
  -1 siblings, 0 replies; 23+ messages in thread
From: xiang xiao @ 2019-05-09 12:37 UTC (permalink / raw)
  To: Arnaud Pouliquen
  Cc: Ohad Ben Cohen, Bjorn Andersson, wendy.liang, linux-remoteproc,
	linux-kernel, Xiang Xiao

[-- 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


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* Re: [PATCH 3/3] rpmsg: virtio_rpmsg_bus: get buffer size from config space
  2019-05-09 12:36     ` Arnaud Pouliquen
  (?)
@ 2019-05-09 13:00     ` xiang xiao
  2019-06-04 14:25         ` Arnaud Pouliquen
  -1 siblings, 1 reply; 23+ messages in thread
From: xiang xiao @ 2019-05-09 13:00 UTC (permalink / raw)
  To: Arnaud Pouliquen
  Cc: Ohad Ben Cohen, Bjorn Andersson, wendy.liang, linux-remoteproc,
	linux-kernel, Xiang Xiao, Loic PALLARDY, Suman Anna

On Thu, May 9, 2019 at 8:36 PM Arnaud Pouliquen <arnaud.pouliquen@st.com> wrote:
>
> Hello Xiang,
>
> Similar mechanism has been proposed by Loic 2 years ago (link to the
> series here https://lkml.org/lkml/2017/3/28/349).
>
> Did you see them? Regarding history, patches seem just on hold...
>

Just saw this patchset, so it's common problem hit by many vendor,
rpmsg framework need to address it.:)

> Main differences (except interesting RX/TX size split) seems that you
> - don't use the virtio_config_ops->get

virtio_cread call virtio_config_ops->get internally, the ideal is same
for both patch, just the implementation detail is different.

> - define a new feature VIRTIO_RPMSG_F_NS.

I add this flag to keep the compatibility with old remote peer, and
also follow the common virito driver practice.

>
> Regards
> Arnaud
>
>
> On 1/31/19 4:41 PM, Xiang Xiao wrote:
> > 512 bytes isn't always suitable for all case, let firmware
> > maker decide the best value from resource table.
> > enable by VIRTIO_RPMSG_F_BUFSZ feature bit.
> >
> > Signed-off-by: Xiang Xiao <xiaoxiang@xiaomi.com>
> > ---
> >  drivers/rpmsg/virtio_rpmsg_bus.c  | 50 +++++++++++++++++++++++++--------------
> >  include/uapi/linux/virtio_rpmsg.h | 24 +++++++++++++++++++
> >  2 files changed, 56 insertions(+), 18 deletions(-)
> >  create mode 100644 include/uapi/linux/virtio_rpmsg.h
> >
> > diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> > index 59c4554..049dd97 100644
> > --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> > +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> > @@ -16,6 +16,7 @@
> >  #include <linux/virtio.h>
> >  #include <linux/virtio_ids.h>
> >  #include <linux/virtio_config.h>
> > +#include <linux/virtio_rpmsg.h>
> >  #include <linux/scatterlist.h>
> >  #include <linux/dma-mapping.h>
> >  #include <linux/slab.h>
> > @@ -38,7 +39,8 @@
> >   * @sbufs:   kernel address of tx buffers
> >   * @num_rbufs:       total number of buffers for rx
> >   * @num_sbufs:       total number of buffers for tx
> > - * @buf_size:        size of one rx or tx buffer
> > + * @rbuf_size:       size of one rx buffer
> > + * @sbuf_size:       size of one tx buffer
> >   * @last_sbuf:       index of last tx buffer used
> >   * @rbufs_dma:       dma base addr of rx buffers
> >   * @sbufs_dma:       dma base addr of tx buffers
> > @@ -61,7 +63,8 @@ struct virtproc_info {
> >       void *rbufs, *sbufs;
> >       unsigned int num_rbufs;
> >       unsigned int num_sbufs;
> > -     unsigned int buf_size;
> > +     unsigned int rbuf_size;
> > +     unsigned int sbuf_size;
> >       int last_sbuf;
> >       dma_addr_t rbufs_dma;
> >       dma_addr_t sbufs_dma;
> > @@ -73,9 +76,6 @@ struct virtproc_info {
> >       struct rpmsg_endpoint *ns_ept;
> >  };
> >
> > -/* The feature bitmap for virtio rpmsg */
> > -#define VIRTIO_RPMSG_F_NS    0 /* RP supports name service notifications */
> > -
> >  /**
> >   * struct rpmsg_hdr - common header for all rpmsg messages
> >   * @src: source address
> > @@ -452,7 +452,7 @@ static void *get_a_tx_buf(struct virtproc_info *vrp)
> >
> >       /* either pick the next unused tx buffer */
> >       if (vrp->last_sbuf < vrp->num_sbufs)
> > -             ret = vrp->sbufs + vrp->buf_size * vrp->last_sbuf++;
> > +             ret = vrp->sbufs + vrp->sbuf_size * vrp->last_sbuf++;
> >       /* or recycle a used one */
> >       else
> >               ret = virtqueue_get_buf(vrp->svq, &len);
> > @@ -578,7 +578,7 @@ static int rpmsg_send_offchannel_raw(struct rpmsg_device *rpdev,
> >        * messaging), or to improve the buffer allocator, to support
> >        * variable-length buffer sizes.
> >        */
> > -     if (len > vrp->buf_size - sizeof(struct rpmsg_hdr)) {
> > +     if (len > vrp->sbuf_size - sizeof(struct rpmsg_hdr)) {
> >               dev_err(dev, "message is too big (%d)\n", len);
> >               return -EMSGSIZE;
> >       }
> > @@ -718,7 +718,7 @@ static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
> >        * We currently use fixed-sized buffers, so trivially sanitize
> >        * the reported payload length.
> >        */
> > -     if (len > vrp->buf_size ||
> > +     if (len > vrp->rbuf_size ||
> >           msg->len > (len - sizeof(struct rpmsg_hdr))) {
> >               dev_warn(dev, "inbound msg too big: (%d, %d)\n", len, msg->len);
> >               return -EINVAL;
> > @@ -751,7 +751,7 @@ static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
> >               dev_warn(dev, "msg received with no recipient\n");
> >
> >       /* publish the real size of the buffer */
> > -     rpmsg_sg_init(&sg, msg, vrp->buf_size);
> > +     rpmsg_sg_init(&sg, msg, vrp->rbuf_size);
> >
> >       /* add the buffer back to the remote processor's virtqueue */
> >       err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, msg, GFP_KERNEL);
> > @@ -907,11 +907,24 @@ static int rpmsg_probe(struct virtio_device *vdev)
> >       else
> >               vrp->num_sbufs = MAX_RPMSG_NUM_BUFS;
> >
> > -     vrp->buf_size = MAX_RPMSG_BUF_SIZE;
> > +     /* try to get buffer size from config space */
> > +     if (virtio_has_feature(vdev, VIRTIO_RPMSG_F_BUFSZ)) {
> > +             /* note: virtio_rpmsg_config is defined from remote view */
> > +             virtio_cread(vdev, struct virtio_rpmsg_config,
> > +                          txbuf_size, &vrp->rbuf_size);
> > +             virtio_cread(vdev, struct virtio_rpmsg_config,
> > +                          rxbuf_size, &vrp->sbuf_size);
> > +     }
> > +
> > +     /* use the default if resource table doesn't provide one */
> > +     if (!vrp->rbuf_size)
> > +             vrp->rbuf_size = MAX_RPMSG_BUF_SIZE;
> > +     if (!vrp->sbuf_size)
> > +             vrp->sbuf_size = MAX_RPMSG_BUF_SIZE;
> >
> >       /* allocate coherent memory for the buffers */
> >       vrp->rbufs = dma_alloc_coherent(vdev->dev.parent->parent,
> > -                                     vrp->num_rbufs * vrp->buf_size,
> > +                                     vrp->num_rbufs * vrp->rbuf_size,
> >                                       &vrp->rbufs_dma, GFP_KERNEL);
> >       if (!vrp->rbufs) {
> >               err = -ENOMEM;
> > @@ -922,7 +935,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
> >               vrp->rbufs, &vrp->rbufs_dma);
> >
> >       vrp->sbufs = dma_alloc_coherent(vdev->dev.parent->parent,
> > -                                     vrp->num_sbufs * vrp->buf_size,
> > +                                     vrp->num_sbufs * vrp->sbuf_size,
> >                                       &vrp->sbufs_dma, GFP_KERNEL);
> >       if (!vrp->sbufs) {
> >               err = -ENOMEM;
> > @@ -935,9 +948,9 @@ static int rpmsg_probe(struct virtio_device *vdev)
> >       /* set up the receive buffers */
> >       for (i = 0; i < vrp->num_rbufs; i++) {
> >               struct scatterlist sg;
> > -             void *cpu_addr = vrp->rbufs + i * vrp->buf_size;
> > +             void *cpu_addr = vrp->rbufs + i * vrp->rbuf_size;
> >
> > -             rpmsg_sg_init(&sg, cpu_addr, vrp->buf_size);
> > +             rpmsg_sg_init(&sg, cpu_addr, vrp->rbuf_size);
> >
> >               err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, cpu_addr,
> >                                         GFP_KERNEL);
> > @@ -984,11 +997,11 @@ static int rpmsg_probe(struct virtio_device *vdev)
> >
> >  free_sbufs:
> >       dma_free_coherent(vdev->dev.parent->parent,
> > -                       vrp->num_sbufs * vrp->buf_size,
> > +                       vrp->num_sbufs * vrp->sbuf_size,
> >                         vrp->sbufs, vrp->sbufs_dma);
> >  free_rbufs:
> >       dma_free_coherent(vdev->dev.parent->parent,
> > -                       vrp->num_rbufs * vrp->buf_size,
> > +                       vrp->num_rbufs * vrp->rbuf_size,
> >                         vrp->rbufs, vrp->rbufs_dma);
> >  vqs_del:
> >       vdev->config->del_vqs(vrp->vdev);
> > @@ -1023,10 +1036,10 @@ static void rpmsg_remove(struct virtio_device *vdev)
> >       vdev->config->del_vqs(vrp->vdev);
> >
> >       dma_free_coherent(vdev->dev.parent->parent,
> > -                       vrp->num_sbufs * vrp->buf_size,
> > +                       vrp->num_sbufs * vrp->sbuf_size,
> >                         vrp->sbufs, vrp->sbufs_dma);
> >       dma_free_coherent(vdev->dev.parent->parent,
> > -                       vrp->num_rbufs * vrp->buf_size,
> > +                       vrp->num_rbufs * vrp->rbuf_size,
> >                         vrp->rbufs, vrp->rbufs_dma);
> >
> >       kfree(vrp);
> > @@ -1039,6 +1052,7 @@ static struct virtio_device_id id_table[] = {
> >
> >  static unsigned int features[] = {
> >       VIRTIO_RPMSG_F_NS,
> > +     VIRTIO_RPMSG_F_BUFSZ,
> >  };
> >
> >  static struct virtio_driver virtio_ipc_driver = {
> > diff --git a/include/uapi/linux/virtio_rpmsg.h b/include/uapi/linux/virtio_rpmsg.h
> > new file mode 100644
> > index 0000000..24fa0dd
> > --- /dev/null
> > +++ b/include/uapi/linux/virtio_rpmsg.h
> > @@ -0,0 +1,24 @@
> > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> > +/*
> > + * Copyright (C) Pinecone Inc. 2019
> > + * Copyright (C) Xiang Xiao <xiaoxiang@pinecone.net>
> > + */
> > +
> > +#ifndef _UAPI_LINUX_VIRTIO_RPMSG_H
> > +#define _UAPI_LINUX_VIRTIO_RPMSG_H
> > +
> > +#include <linux/types.h>
> > +
> > +/* The feature bitmap for virtio rpmsg */
> > +#define VIRTIO_RPMSG_F_NS    0 /* RP supports name service notifications */
> > +#define VIRTIO_RPMSG_F_BUFSZ 2 /* RP get buffer size from config space */
> > +
> > +struct virtio_rpmsg_config {
> > +     /* The tx/rx individual buffer size(if VIRTIO_RPMSG_F_BUFSZ) */
> > +     __u32 txbuf_size;
> > +     __u32 rxbuf_size;
> > +     __u32 reserved[14]; /* Reserve for the future use */
> > +     /* Put the customize config here */
> > +} __attribute__((packed));
> > +
> > +#endif /* _UAPI_LINUX_VIRTIO_RPMSG_H */
> >

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 3/3] rpmsg: virtio_rpmsg_bus: get buffer size from config space
  2019-05-09 13:00     ` xiang xiao
@ 2019-06-04 14:25         ` Arnaud Pouliquen
  0 siblings, 0 replies; 23+ messages in thread
From: Arnaud Pouliquen @ 2019-06-04 14:25 UTC (permalink / raw)
  To: xiang xiao
  Cc: Ohad Ben Cohen, Bjorn Andersson, wendy.liang, linux-remoteproc,
	linux-kernel, Xiang Xiao, Loic PALLARDY, Suman Anna

Hello Xiang,

On 5/9/19 3:00 PM, xiang xiao wrote:
> On Thu, May 9, 2019 at 8:36 PM Arnaud Pouliquen <arnaud.pouliquen@st.com> wrote:
>>
>> Hello Xiang,
>>
>> Similar mechanism has been proposed by Loic 2 years ago (link to the
>> series here https://lkml.org/lkml/2017/3/28/349).
>>
>> Did you see them? Regarding history, patches seem just on hold...
>>
> 
> Just saw this patchset, so it's common problem hit by many vendor,
> rpmsg framework need to address it.:)
> 
>> Main differences (except interesting RX/TX size split) seems that you
>> - don't use the virtio_config_ops->get
> 
> virtio_cread call virtio_config_ops->get internally, the ideal is same
> for both patch, just the implementation detail is different.
> 
>> - define a new feature VIRTIO_RPMSG_F_NS.
> 
> I add this flag to keep the compatibility with old remote peer, and
> also follow the common virito driver practice.
I discussed with Loic, he is ok to go further with your patch and
abandon his one. Please find some remarks below in-line
> 
>>
>> Regards
>> Arnaud
>>
>>
>> On 1/31/19 4:41 PM, Xiang Xiao wrote:
>>> 512 bytes isn't always suitable for all case, let firmware
>>> maker decide the best value from resource table.
>>> enable by VIRTIO_RPMSG_F_BUFSZ feature bit.
>>>
>>> Signed-off-by: Xiang Xiao <xiaoxiang@xiaomi.com>
>>> ---
>>>  drivers/rpmsg/virtio_rpmsg_bus.c  | 50 +++++++++++++++++++++++++--------------
>>>  include/uapi/linux/virtio_rpmsg.h | 24 +++++++++++++++++++
>>>  2 files changed, 56 insertions(+), 18 deletions(-)
>>>  create mode 100644 include/uapi/linux/virtio_rpmsg.h
>>>
>>> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
>>> index 59c4554..049dd97 100644
>>> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
>>> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
>>> @@ -16,6 +16,7 @@
>>>  #include <linux/virtio.h>
>>>  #include <linux/virtio_ids.h>
>>>  #include <linux/virtio_config.h>
>>> +#include <linux/virtio_rpmsg.h>
>>>  #include <linux/scatterlist.h>
>>>  #include <linux/dma-mapping.h>
>>>  #include <linux/slab.h>
>>> @@ -38,7 +39,8 @@
>>>   * @sbufs:   kernel address of tx buffers
>>>   * @num_rbufs:       total number of buffers for rx
>>>   * @num_sbufs:       total number of buffers for tx
>>> - * @buf_size:        size of one rx or tx buffer
>>> + * @rbuf_size:       size of one rx buffer
>>> + * @sbuf_size:       size of one tx buffer
>>>   * @last_sbuf:       index of last tx buffer used
>>>   * @rbufs_dma:       dma base addr of rx buffers
>>>   * @sbufs_dma:       dma base addr of tx buffers
>>> @@ -61,7 +63,8 @@ struct virtproc_info {
>>>       void *rbufs, *sbufs;
>>>       unsigned int num_rbufs;
>>>       unsigned int num_sbufs;
>>> -     unsigned int buf_size;
>>> +     unsigned int rbuf_size;
>>> +     unsigned int sbuf_size;
>>>       int last_sbuf;
>>>       dma_addr_t rbufs_dma;
>>>       dma_addr_t sbufs_dma;
>>> @@ -73,9 +76,6 @@ struct virtproc_info {
>>>       struct rpmsg_endpoint *ns_ept;
>>>  };
>>>
>>> -/* The feature bitmap for virtio rpmsg */
>>> -#define VIRTIO_RPMSG_F_NS    0 /* RP supports name service notifications */
>>> -
>>>  /**
>>>   * struct rpmsg_hdr - common header for all rpmsg messages
>>>   * @src: source address
>>> @@ -452,7 +452,7 @@ static void *get_a_tx_buf(struct virtproc_info *vrp)
>>>
>>>       /* either pick the next unused tx buffer */
>>>       if (vrp->last_sbuf < vrp->num_sbufs)
>>> -             ret = vrp->sbufs + vrp->buf_size * vrp->last_sbuf++;
>>> +             ret = vrp->sbufs + vrp->sbuf_size * vrp->last_sbuf++;
>>>       /* or recycle a used one */
>>>       else
>>>               ret = virtqueue_get_buf(vrp->svq, &len);
>>> @@ -578,7 +578,7 @@ static int rpmsg_send_offchannel_raw(struct rpmsg_device *rpdev,
>>>        * messaging), or to improve the buffer allocator, to support
>>>        * variable-length buffer sizes.
>>>        */
>>> -     if (len > vrp->buf_size - sizeof(struct rpmsg_hdr)) {
>>> +     if (len > vrp->sbuf_size - sizeof(struct rpmsg_hdr)) {
>>>               dev_err(dev, "message is too big (%d)\n", len);
>>>               return -EMSGSIZE;
>>>       }
>>> @@ -718,7 +718,7 @@ static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
>>>        * We currently use fixed-sized buffers, so trivially sanitize
>>>        * the reported payload length.
>>>        */
>>> -     if (len > vrp->buf_size ||
>>> +     if (len > vrp->rbuf_size ||
>>>           msg->len > (len - sizeof(struct rpmsg_hdr))) {
>>>               dev_warn(dev, "inbound msg too big: (%d, %d)\n", len, msg->len);
>>>               return -EINVAL;
>>> @@ -751,7 +751,7 @@ static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
>>>               dev_warn(dev, "msg received with no recipient\n");
>>>
>>>       /* publish the real size of the buffer */
>>> -     rpmsg_sg_init(&sg, msg, vrp->buf_size);
>>> +     rpmsg_sg_init(&sg, msg, vrp->rbuf_size);
>>>
>>>       /* add the buffer back to the remote processor's virtqueue */
>>>       err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, msg, GFP_KERNEL);
>>> @@ -907,11 +907,24 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>>       else
>>>               vrp->num_sbufs = MAX_RPMSG_NUM_BUFS;
>>>
>>> -     vrp->buf_size = MAX_RPMSG_BUF_SIZE;
>>> +     /* try to get buffer size from config space */
>>> +     if (virtio_has_feature(vdev, VIRTIO_RPMSG_F_BUFSZ)) {
>>> +             /* note: virtio_rpmsg_config is defined from remote view */
>>> +             virtio_cread(vdev, struct virtio_rpmsg_config,
>>> +                          txbuf_size, &vrp->rbuf_size);
>>> +             virtio_cread(vdev, struct virtio_rpmsg_config,
>>> +                          rxbuf_size, &vrp->sbuf_size);
>>> +     }
>>> +
>>> +     /* use the default if resource table doesn't provide one */
>>> +     if (!vrp->rbuf_size)
>>> +             vrp->rbuf_size = MAX_RPMSG_BUF_SIZE;
In this case constant should be renamed DEFAULT_RPMSG_BUF_SIZE as it is
no more a max value
>>> +     if (!vrp->sbuf_size)
>>> +             vrp->sbuf_size = MAX_RPMSG_BUF_SIZE;
Here, if the config space exists you need to update it in consequence to
ensure coherency with the remote processor config.

>>>
>>>       /* allocate coherent memory for the buffers */
>>>       vrp->rbufs = dma_alloc_coherent(vdev->dev.parent->parent,
>>> -                                     vrp->num_rbufs * vrp->buf_size,
>>> +                                     vrp->num_rbufs * vrp->rbuf_size,
>>>                                       &vrp->rbufs_dma, GFP_KERNEL);
>>>       if (!vrp->rbufs) {
>>>               err = -ENOMEM;
>>> @@ -922,7 +935,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>>               vrp->rbufs, &vrp->rbufs_dma);
>>>
>>>       vrp->sbufs = dma_alloc_coherent(vdev->dev.parent->parent,
>>> -                                     vrp->num_sbufs * vrp->buf_size,
>>> +                                     vrp->num_sbufs * vrp->sbuf_size,
>>>                                       &vrp->sbufs_dma, GFP_KERNEL);
>>>       if (!vrp->sbufs) {
>>>               err = -ENOMEM;
>>> @@ -935,9 +948,9 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>>       /* set up the receive buffers */
>>>       for (i = 0; i < vrp->num_rbufs; i++) {
>>>               struct scatterlist sg;
>>> -             void *cpu_addr = vrp->rbufs + i * vrp->buf_size;
>>> +             void *cpu_addr = vrp->rbufs + i * vrp->rbuf_size;
>>>
>>> -             rpmsg_sg_init(&sg, cpu_addr, vrp->buf_size);
>>> +             rpmsg_sg_init(&sg, cpu_addr, vrp->rbuf_size);
>>>
>>>               err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, cpu_addr,
>>>                                         GFP_KERNEL);
>>> @@ -984,11 +997,11 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>>
>>>  free_sbufs:
>>>       dma_free_coherent(vdev->dev.parent->parent,
>>> -                       vrp->num_sbufs * vrp->buf_size,
>>> +                       vrp->num_sbufs * vrp->sbuf_size,
>>>                         vrp->sbufs, vrp->sbufs_dma);
>>>  free_rbufs:
>>>       dma_free_coherent(vdev->dev.parent->parent,
>>> -                       vrp->num_rbufs * vrp->buf_size,
>>> +                       vrp->num_rbufs * vrp->rbuf_size,
>>>                         vrp->rbufs, vrp->rbufs_dma);
>>>  vqs_del:
>>>       vdev->config->del_vqs(vrp->vdev);
>>> @@ -1023,10 +1036,10 @@ static void rpmsg_remove(struct virtio_device *vdev)
>>>       vdev->config->del_vqs(vrp->vdev);
>>>
>>>       dma_free_coherent(vdev->dev.parent->parent,
>>> -                       vrp->num_sbufs * vrp->buf_size,
>>> +                       vrp->num_sbufs * vrp->sbuf_size,
>>>                         vrp->sbufs, vrp->sbufs_dma);
>>>       dma_free_coherent(vdev->dev.parent->parent,
>>> -                       vrp->num_rbufs * vrp->buf_size,
>>> +                       vrp->num_rbufs * vrp->rbuf_size,
>>>                         vrp->rbufs, vrp->rbufs_dma);
>>>
>>>       kfree(vrp);
>>> @@ -1039,6 +1052,7 @@ static struct virtio_device_id id_table[] = {
>>>
>>>  static unsigned int features[] = {
>>>       VIRTIO_RPMSG_F_NS,
>>> +     VIRTIO_RPMSG_F_BUFSZ,
>>>  };
>>>
>>>  static struct virtio_driver virtio_ipc_driver = {
>>> diff --git a/include/uapi/linux/virtio_rpmsg.h b/include/uapi/linux/virtio_rpmsg.h
>>> new file mode 100644
>>> index 0000000..24fa0dd
>>> --- /dev/null
>>> +++ b/include/uapi/linux/virtio_rpmsg.h
Strange to define a user space API for kernel usage need. Could you
elaborate?

>>> @@ -0,0 +1,24 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
>>> +/*
>>> + * Copyright (C) Pinecone Inc. 2019
>>> + * Copyright (C) Xiang Xiao <xiaoxiang@pinecone.net>
>>> + */
>>> +
>>> +#ifndef _UAPI_LINUX_VIRTIO_RPMSG_H
>>> +#define _UAPI_LINUX_VIRTIO_RPMSG_H
>>> +
>>> +#include <linux/types.h>
>>> +
>>> +/* The feature bitmap for virtio rpmsg */
>>> +#define VIRTIO_RPMSG_F_NS    0 /* RP supports name service notifications */
>>> +#define VIRTIO_RPMSG_F_BUFSZ 2 /* RP get buffer size from config space */
Would be useful to document it in rpmsg.txt
>>> +
>>> +struct virtio_rpmsg_config {
>>> +     /* The tx/rx individual buffer size(if VIRTIO_RPMSG_F_BUFSZ) */
>>> +     __u32 txbuf_size;
>>> +     __u32 rxbuf_size;
>>> +     __u32 reserved[14]; /* Reserve for the future use */
>>> +     /* Put the customize config here */
>>> +} __attribute__((packed));
>>> +
Wouldn't it be better to add an identifier and a version fields at the
beginning of the structure? Idea would be to simplify a future extension
In this case is VIRTIO_RPMSG_F_BUFSZ still useful?

>>> +#endif /* _UAPI_LINUX_VIRTIO_RPMSG_H */
>>>
--
Thanks
Arnaud

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 3/3] rpmsg: virtio_rpmsg_bus: get buffer size from config space
@ 2019-06-04 14:25         ` Arnaud Pouliquen
  0 siblings, 0 replies; 23+ messages in thread
From: Arnaud Pouliquen @ 2019-06-04 14:25 UTC (permalink / raw)
  To: xiang xiao
  Cc: Ohad Ben Cohen, Bjorn Andersson, wendy.liang, linux-remoteproc,
	linux-kernel, Xiang Xiao, Loic PALLARDY, Suman Anna

Hello Xiang,

On 5/9/19 3:00 PM, xiang xiao wrote:
> On Thu, May 9, 2019 at 8:36 PM Arnaud Pouliquen <arnaud.pouliquen@st.com> wrote:
>>
>> Hello Xiang,
>>
>> Similar mechanism has been proposed by Loic 2 years ago (link to the
>> series here https://lkml.org/lkml/2017/3/28/349).
>>
>> Did you see them? Regarding history, patches seem just on hold...
>>
> 
> Just saw this patchset, so it's common problem hit by many vendor,
> rpmsg framework need to address it.:)
> 
>> Main differences (except interesting RX/TX size split) seems that you
>> - don't use the virtio_config_ops->get
> 
> virtio_cread call virtio_config_ops->get internally, the ideal is same
> for both patch, just the implementation detail is different.
> 
>> - define a new feature VIRTIO_RPMSG_F_NS.
> 
> I add this flag to keep the compatibility with old remote peer, and
> also follow the common virito driver practice.
I discussed with Loic, he is ok to go further with your patch and
abandon his one. Please find some remarks below in-line
> 
>>
>> Regards
>> Arnaud
>>
>>
>> On 1/31/19 4:41 PM, Xiang Xiao wrote:
>>> 512 bytes isn't always suitable for all case, let firmware
>>> maker decide the best value from resource table.
>>> enable by VIRTIO_RPMSG_F_BUFSZ feature bit.
>>>
>>> Signed-off-by: Xiang Xiao <xiaoxiang@xiaomi.com>
>>> ---
>>>  drivers/rpmsg/virtio_rpmsg_bus.c  | 50 +++++++++++++++++++++++++--------------
>>>  include/uapi/linux/virtio_rpmsg.h | 24 +++++++++++++++++++
>>>  2 files changed, 56 insertions(+), 18 deletions(-)
>>>  create mode 100644 include/uapi/linux/virtio_rpmsg.h
>>>
>>> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
>>> index 59c4554..049dd97 100644
>>> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
>>> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
>>> @@ -16,6 +16,7 @@
>>>  #include <linux/virtio.h>
>>>  #include <linux/virtio_ids.h>
>>>  #include <linux/virtio_config.h>
>>> +#include <linux/virtio_rpmsg.h>
>>>  #include <linux/scatterlist.h>
>>>  #include <linux/dma-mapping.h>
>>>  #include <linux/slab.h>
>>> @@ -38,7 +39,8 @@
>>>   * @sbufs:   kernel address of tx buffers
>>>   * @num_rbufs:       total number of buffers for rx
>>>   * @num_sbufs:       total number of buffers for tx
>>> - * @buf_size:        size of one rx or tx buffer
>>> + * @rbuf_size:       size of one rx buffer
>>> + * @sbuf_size:       size of one tx buffer
>>>   * @last_sbuf:       index of last tx buffer used
>>>   * @rbufs_dma:       dma base addr of rx buffers
>>>   * @sbufs_dma:       dma base addr of tx buffers
>>> @@ -61,7 +63,8 @@ struct virtproc_info {
>>>       void *rbufs, *sbufs;
>>>       unsigned int num_rbufs;
>>>       unsigned int num_sbufs;
>>> -     unsigned int buf_size;
>>> +     unsigned int rbuf_size;
>>> +     unsigned int sbuf_size;
>>>       int last_sbuf;
>>>       dma_addr_t rbufs_dma;
>>>       dma_addr_t sbufs_dma;
>>> @@ -73,9 +76,6 @@ struct virtproc_info {
>>>       struct rpmsg_endpoint *ns_ept;
>>>  };
>>>
>>> -/* The feature bitmap for virtio rpmsg */
>>> -#define VIRTIO_RPMSG_F_NS    0 /* RP supports name service notifications */
>>> -
>>>  /**
>>>   * struct rpmsg_hdr - common header for all rpmsg messages
>>>   * @src: source address
>>> @@ -452,7 +452,7 @@ static void *get_a_tx_buf(struct virtproc_info *vrp)
>>>
>>>       /* either pick the next unused tx buffer */
>>>       if (vrp->last_sbuf < vrp->num_sbufs)
>>> -             ret = vrp->sbufs + vrp->buf_size * vrp->last_sbuf++;
>>> +             ret = vrp->sbufs + vrp->sbuf_size * vrp->last_sbuf++;
>>>       /* or recycle a used one */
>>>       else
>>>               ret = virtqueue_get_buf(vrp->svq, &len);
>>> @@ -578,7 +578,7 @@ static int rpmsg_send_offchannel_raw(struct rpmsg_device *rpdev,
>>>        * messaging), or to improve the buffer allocator, to support
>>>        * variable-length buffer sizes.
>>>        */
>>> -     if (len > vrp->buf_size - sizeof(struct rpmsg_hdr)) {
>>> +     if (len > vrp->sbuf_size - sizeof(struct rpmsg_hdr)) {
>>>               dev_err(dev, "message is too big (%d)\n", len);
>>>               return -EMSGSIZE;
>>>       }
>>> @@ -718,7 +718,7 @@ static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
>>>        * We currently use fixed-sized buffers, so trivially sanitize
>>>        * the reported payload length.
>>>        */
>>> -     if (len > vrp->buf_size ||
>>> +     if (len > vrp->rbuf_size ||
>>>           msg->len > (len - sizeof(struct rpmsg_hdr))) {
>>>               dev_warn(dev, "inbound msg too big: (%d, %d)\n", len, msg->len);
>>>               return -EINVAL;
>>> @@ -751,7 +751,7 @@ static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
>>>               dev_warn(dev, "msg received with no recipient\n");
>>>
>>>       /* publish the real size of the buffer */
>>> -     rpmsg_sg_init(&sg, msg, vrp->buf_size);
>>> +     rpmsg_sg_init(&sg, msg, vrp->rbuf_size);
>>>
>>>       /* add the buffer back to the remote processor's virtqueue */
>>>       err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, msg, GFP_KERNEL);
>>> @@ -907,11 +907,24 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>>       else
>>>               vrp->num_sbufs = MAX_RPMSG_NUM_BUFS;
>>>
>>> -     vrp->buf_size = MAX_RPMSG_BUF_SIZE;
>>> +     /* try to get buffer size from config space */
>>> +     if (virtio_has_feature(vdev, VIRTIO_RPMSG_F_BUFSZ)) {
>>> +             /* note: virtio_rpmsg_config is defined from remote view */
>>> +             virtio_cread(vdev, struct virtio_rpmsg_config,
>>> +                          txbuf_size, &vrp->rbuf_size);
>>> +             virtio_cread(vdev, struct virtio_rpmsg_config,
>>> +                          rxbuf_size, &vrp->sbuf_size);
>>> +     }
>>> +
>>> +     /* use the default if resource table doesn't provide one */
>>> +     if (!vrp->rbuf_size)
>>> +             vrp->rbuf_size = MAX_RPMSG_BUF_SIZE;
In this case constant should be renamed DEFAULT_RPMSG_BUF_SIZE as it is
no more a max value
>>> +     if (!vrp->sbuf_size)
>>> +             vrp->sbuf_size = MAX_RPMSG_BUF_SIZE;
Here, if the config space exists you need to update it in consequence to
ensure coherency with the remote processor config.

>>>
>>>       /* allocate coherent memory for the buffers */
>>>       vrp->rbufs = dma_alloc_coherent(vdev->dev.parent->parent,
>>> -                                     vrp->num_rbufs * vrp->buf_size,
>>> +                                     vrp->num_rbufs * vrp->rbuf_size,
>>>                                       &vrp->rbufs_dma, GFP_KERNEL);
>>>       if (!vrp->rbufs) {
>>>               err = -ENOMEM;
>>> @@ -922,7 +935,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>>               vrp->rbufs, &vrp->rbufs_dma);
>>>
>>>       vrp->sbufs = dma_alloc_coherent(vdev->dev.parent->parent,
>>> -                                     vrp->num_sbufs * vrp->buf_size,
>>> +                                     vrp->num_sbufs * vrp->sbuf_size,
>>>                                       &vrp->sbufs_dma, GFP_KERNEL);
>>>       if (!vrp->sbufs) {
>>>               err = -ENOMEM;
>>> @@ -935,9 +948,9 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>>       /* set up the receive buffers */
>>>       for (i = 0; i < vrp->num_rbufs; i++) {
>>>               struct scatterlist sg;
>>> -             void *cpu_addr = vrp->rbufs + i * vrp->buf_size;
>>> +             void *cpu_addr = vrp->rbufs + i * vrp->rbuf_size;
>>>
>>> -             rpmsg_sg_init(&sg, cpu_addr, vrp->buf_size);
>>> +             rpmsg_sg_init(&sg, cpu_addr, vrp->rbuf_size);
>>>
>>>               err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, cpu_addr,
>>>                                         GFP_KERNEL);
>>> @@ -984,11 +997,11 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>>
>>>  free_sbufs:
>>>       dma_free_coherent(vdev->dev.parent->parent,
>>> -                       vrp->num_sbufs * vrp->buf_size,
>>> +                       vrp->num_sbufs * vrp->sbuf_size,
>>>                         vrp->sbufs, vrp->sbufs_dma);
>>>  free_rbufs:
>>>       dma_free_coherent(vdev->dev.parent->parent,
>>> -                       vrp->num_rbufs * vrp->buf_size,
>>> +                       vrp->num_rbufs * vrp->rbuf_size,
>>>                         vrp->rbufs, vrp->rbufs_dma);
>>>  vqs_del:
>>>       vdev->config->del_vqs(vrp->vdev);
>>> @@ -1023,10 +1036,10 @@ static void rpmsg_remove(struct virtio_device *vdev)
>>>       vdev->config->del_vqs(vrp->vdev);
>>>
>>>       dma_free_coherent(vdev->dev.parent->parent,
>>> -                       vrp->num_sbufs * vrp->buf_size,
>>> +                       vrp->num_sbufs * vrp->sbuf_size,
>>>                         vrp->sbufs, vrp->sbufs_dma);
>>>       dma_free_coherent(vdev->dev.parent->parent,
>>> -                       vrp->num_rbufs * vrp->buf_size,
>>> +                       vrp->num_rbufs * vrp->rbuf_size,
>>>                         vrp->rbufs, vrp->rbufs_dma);
>>>
>>>       kfree(vrp);
>>> @@ -1039,6 +1052,7 @@ static struct virtio_device_id id_table[] = {
>>>
>>>  static unsigned int features[] = {
>>>       VIRTIO_RPMSG_F_NS,
>>> +     VIRTIO_RPMSG_F_BUFSZ,
>>>  };
>>>
>>>  static struct virtio_driver virtio_ipc_driver = {
>>> diff --git a/include/uapi/linux/virtio_rpmsg.h b/include/uapi/linux/virtio_rpmsg.h
>>> new file mode 100644
>>> index 0000000..24fa0dd
>>> --- /dev/null
>>> +++ b/include/uapi/linux/virtio_rpmsg.h
Strange to define a user space API for kernel usage need. Could you
elaborate?

>>> @@ -0,0 +1,24 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
>>> +/*
>>> + * Copyright (C) Pinecone Inc. 2019
>>> + * Copyright (C) Xiang Xiao <xiaoxiang@pinecone.net>
>>> + */
>>> +
>>> +#ifndef _UAPI_LINUX_VIRTIO_RPMSG_H
>>> +#define _UAPI_LINUX_VIRTIO_RPMSG_H
>>> +
>>> +#include <linux/types.h>
>>> +
>>> +/* The feature bitmap for virtio rpmsg */
>>> +#define VIRTIO_RPMSG_F_NS    0 /* RP supports name service notifications */
>>> +#define VIRTIO_RPMSG_F_BUFSZ 2 /* RP get buffer size from config space */
Would be useful to document it in rpmsg.txt
>>> +
>>> +struct virtio_rpmsg_config {
>>> +     /* The tx/rx individual buffer size(if VIRTIO_RPMSG_F_BUFSZ) */
>>> +     __u32 txbuf_size;
>>> +     __u32 rxbuf_size;
>>> +     __u32 reserved[14]; /* Reserve for the future use */
>>> +     /* Put the customize config here */
>>> +} __attribute__((packed));
>>> +
Wouldn't it be better to add an identifier and a version fields at the
beginning of the structure? Idea would be to simplify a future extension
In this case is VIRTIO_RPMSG_F_BUFSZ still useful?

>>> +#endif /* _UAPI_LINUX_VIRTIO_RPMSG_H */
>>>
--
Thanks
Arnaud

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 3/3] rpmsg: virtio_rpmsg_bus: get buffer size from config space
  2019-06-04 14:25         ` Arnaud Pouliquen
  (?)
@ 2019-06-05  2:40         ` xiang xiao
  2019-06-05  8:02             ` Arnaud Pouliquen
  -1 siblings, 1 reply; 23+ messages in thread
From: xiang xiao @ 2019-06-05  2:40 UTC (permalink / raw)
  To: Arnaud Pouliquen
  Cc: Ohad Ben Cohen, Bjorn Andersson, wendy.liang, linux-remoteproc,
	linux-kernel, Xiang Xiao, Loic PALLARDY, Suman Anna

On Tue, Jun 4, 2019 at 10:25 PM Arnaud Pouliquen
<arnaud.pouliquen@st.com> wrote:
>
> Hello Xiang,
>
> On 5/9/19 3:00 PM, xiang xiao wrote:
> > On Thu, May 9, 2019 at 8:36 PM Arnaud Pouliquen <arnaud.pouliquen@st.com> wrote:
> >>
> >> Hello Xiang,
> >>
> >> Similar mechanism has been proposed by Loic 2 years ago (link to the
> >> series here https://lkml.org/lkml/2017/3/28/349).
> >>
> >> Did you see them? Regarding history, patches seem just on hold...
> >>
> >
> > Just saw this patchset, so it's common problem hit by many vendor,
> > rpmsg framework need to address it.:)
> >
> >> Main differences (except interesting RX/TX size split) seems that you
> >> - don't use the virtio_config_ops->get
> >
> > virtio_cread call virtio_config_ops->get internally, the ideal is same
> > for both patch, just the implementation detail is different.
> >
> >> - define a new feature VIRTIO_RPMSG_F_NS.
> >
> > I add this flag to keep the compatibility with old remote peer, and
> > also follow the common virito driver practice.
> I discussed with Loic, he is ok to go further with your patch and
> abandon his one. Please find some remarks below in-line
> >
> >>
> >> Regards
> >> Arnaud
> >>
> >>
> >> On 1/31/19 4:41 PM, Xiang Xiao wrote:
> >>> 512 bytes isn't always suitable for all case, let firmware
> >>> maker decide the best value from resource table.
> >>> enable by VIRTIO_RPMSG_F_BUFSZ feature bit.
> >>>
> >>> Signed-off-by: Xiang Xiao <xiaoxiang@xiaomi.com>
> >>> ---
> >>>  drivers/rpmsg/virtio_rpmsg_bus.c  | 50 +++++++++++++++++++++++++--------------
> >>>  include/uapi/linux/virtio_rpmsg.h | 24 +++++++++++++++++++
> >>>  2 files changed, 56 insertions(+), 18 deletions(-)
> >>>  create mode 100644 include/uapi/linux/virtio_rpmsg.h
> >>>
> >>> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> >>> index 59c4554..049dd97 100644
> >>> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> >>> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> >>> @@ -16,6 +16,7 @@
> >>>  #include <linux/virtio.h>
> >>>  #include <linux/virtio_ids.h>
> >>>  #include <linux/virtio_config.h>
> >>> +#include <linux/virtio_rpmsg.h>
> >>>  #include <linux/scatterlist.h>
> >>>  #include <linux/dma-mapping.h>
> >>>  #include <linux/slab.h>
> >>> @@ -38,7 +39,8 @@
> >>>   * @sbufs:   kernel address of tx buffers
> >>>   * @num_rbufs:       total number of buffers for rx
> >>>   * @num_sbufs:       total number of buffers for tx
> >>> - * @buf_size:        size of one rx or tx buffer
> >>> + * @rbuf_size:       size of one rx buffer
> >>> + * @sbuf_size:       size of one tx buffer
> >>>   * @last_sbuf:       index of last tx buffer used
> >>>   * @rbufs_dma:       dma base addr of rx buffers
> >>>   * @sbufs_dma:       dma base addr of tx buffers
> >>> @@ -61,7 +63,8 @@ struct virtproc_info {
> >>>       void *rbufs, *sbufs;
> >>>       unsigned int num_rbufs;
> >>>       unsigned int num_sbufs;
> >>> -     unsigned int buf_size;
> >>> +     unsigned int rbuf_size;
> >>> +     unsigned int sbuf_size;
> >>>       int last_sbuf;
> >>>       dma_addr_t rbufs_dma;
> >>>       dma_addr_t sbufs_dma;
> >>> @@ -73,9 +76,6 @@ struct virtproc_info {
> >>>       struct rpmsg_endpoint *ns_ept;
> >>>  };
> >>>
> >>> -/* The feature bitmap for virtio rpmsg */
> >>> -#define VIRTIO_RPMSG_F_NS    0 /* RP supports name service notifications */
> >>> -
> >>>  /**
> >>>   * struct rpmsg_hdr - common header for all rpmsg messages
> >>>   * @src: source address
> >>> @@ -452,7 +452,7 @@ static void *get_a_tx_buf(struct virtproc_info *vrp)
> >>>
> >>>       /* either pick the next unused tx buffer */
> >>>       if (vrp->last_sbuf < vrp->num_sbufs)
> >>> -             ret = vrp->sbufs + vrp->buf_size * vrp->last_sbuf++;
> >>> +             ret = vrp->sbufs + vrp->sbuf_size * vrp->last_sbuf++;
> >>>       /* or recycle a used one */
> >>>       else
> >>>               ret = virtqueue_get_buf(vrp->svq, &len);
> >>> @@ -578,7 +578,7 @@ static int rpmsg_send_offchannel_raw(struct rpmsg_device *rpdev,
> >>>        * messaging), or to improve the buffer allocator, to support
> >>>        * variable-length buffer sizes.
> >>>        */
> >>> -     if (len > vrp->buf_size - sizeof(struct rpmsg_hdr)) {
> >>> +     if (len > vrp->sbuf_size - sizeof(struct rpmsg_hdr)) {
> >>>               dev_err(dev, "message is too big (%d)\n", len);
> >>>               return -EMSGSIZE;
> >>>       }
> >>> @@ -718,7 +718,7 @@ static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
> >>>        * We currently use fixed-sized buffers, so trivially sanitize
> >>>        * the reported payload length.
> >>>        */
> >>> -     if (len > vrp->buf_size ||
> >>> +     if (len > vrp->rbuf_size ||
> >>>           msg->len > (len - sizeof(struct rpmsg_hdr))) {
> >>>               dev_warn(dev, "inbound msg too big: (%d, %d)\n", len, msg->len);
> >>>               return -EINVAL;
> >>> @@ -751,7 +751,7 @@ static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
> >>>               dev_warn(dev, "msg received with no recipient\n");
> >>>
> >>>       /* publish the real size of the buffer */
> >>> -     rpmsg_sg_init(&sg, msg, vrp->buf_size);
> >>> +     rpmsg_sg_init(&sg, msg, vrp->rbuf_size);
> >>>
> >>>       /* add the buffer back to the remote processor's virtqueue */
> >>>       err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, msg, GFP_KERNEL);
> >>> @@ -907,11 +907,24 @@ static int rpmsg_probe(struct virtio_device *vdev)
> >>>       else
> >>>               vrp->num_sbufs = MAX_RPMSG_NUM_BUFS;
> >>>
> >>> -     vrp->buf_size = MAX_RPMSG_BUF_SIZE;
> >>> +     /* try to get buffer size from config space */
> >>> +     if (virtio_has_feature(vdev, VIRTIO_RPMSG_F_BUFSZ)) {
> >>> +             /* note: virtio_rpmsg_config is defined from remote view */
> >>> +             virtio_cread(vdev, struct virtio_rpmsg_config,
> >>> +                          txbuf_size, &vrp->rbuf_size);
> >>> +             virtio_cread(vdev, struct virtio_rpmsg_config,
> >>> +                          rxbuf_size, &vrp->sbuf_size);
> >>> +     }
> >>> +
> >>> +     /* use the default if resource table doesn't provide one */
> >>> +     if (!vrp->rbuf_size)
> >>> +             vrp->rbuf_size = MAX_RPMSG_BUF_SIZE;
> In this case constant should be renamed DEFAULT_RPMSG_BUF_SIZE as it is
> no more a max value

Yes, DEFAULT_RPMSG_BUF_SIZE is more reasonable now.

> >>> +     if (!vrp->sbuf_size)
> >>> +             vrp->sbuf_size = MAX_RPMSG_BUF_SIZE;
> Here, if the config space exists you need to update it in consequence to
> ensure coherency with the remote processor config.

The update is already done in if (virtio_has_feature(vdev,
VIRTIO_RPMSG_F_BUFSZ)), here just handle the zero value in config
space which mean the remote side want to use the default value even
VIRTIO_RPMSG_F_BUFSZ set.
For example:
1.remote side want to change one direction buffer size, but keep
another direction as default
2.or remote side want to change other config options(define in the
furture) not the buffer size

>
> >>>
> >>>       /* allocate coherent memory for the buffers */
> >>>       vrp->rbufs = dma_alloc_coherent(vdev->dev.parent->parent,
> >>> -                                     vrp->num_rbufs * vrp->buf_size,
> >>> +                                     vrp->num_rbufs * vrp->rbuf_size,
> >>>                                       &vrp->rbufs_dma, GFP_KERNEL);
> >>>       if (!vrp->rbufs) {
> >>>               err = -ENOMEM;
> >>> @@ -922,7 +935,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
> >>>               vrp->rbufs, &vrp->rbufs_dma);
> >>>
> >>>       vrp->sbufs = dma_alloc_coherent(vdev->dev.parent->parent,
> >>> -                                     vrp->num_sbufs * vrp->buf_size,
> >>> +                                     vrp->num_sbufs * vrp->sbuf_size,
> >>>                                       &vrp->sbufs_dma, GFP_KERNEL);
> >>>       if (!vrp->sbufs) {
> >>>               err = -ENOMEM;
> >>> @@ -935,9 +948,9 @@ static int rpmsg_probe(struct virtio_device *vdev)
> >>>       /* set up the receive buffers */
> >>>       for (i = 0; i < vrp->num_rbufs; i++) {
> >>>               struct scatterlist sg;
> >>> -             void *cpu_addr = vrp->rbufs + i * vrp->buf_size;
> >>> +             void *cpu_addr = vrp->rbufs + i * vrp->rbuf_size;
> >>>
> >>> -             rpmsg_sg_init(&sg, cpu_addr, vrp->buf_size);
> >>> +             rpmsg_sg_init(&sg, cpu_addr, vrp->rbuf_size);
> >>>
> >>>               err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, cpu_addr,
> >>>                                         GFP_KERNEL);
> >>> @@ -984,11 +997,11 @@ static int rpmsg_probe(struct virtio_device *vdev)
> >>>
> >>>  free_sbufs:
> >>>       dma_free_coherent(vdev->dev.parent->parent,
> >>> -                       vrp->num_sbufs * vrp->buf_size,
> >>> +                       vrp->num_sbufs * vrp->sbuf_size,
> >>>                         vrp->sbufs, vrp->sbufs_dma);
> >>>  free_rbufs:
> >>>       dma_free_coherent(vdev->dev.parent->parent,
> >>> -                       vrp->num_rbufs * vrp->buf_size,
> >>> +                       vrp->num_rbufs * vrp->rbuf_size,
> >>>                         vrp->rbufs, vrp->rbufs_dma);
> >>>  vqs_del:
> >>>       vdev->config->del_vqs(vrp->vdev);
> >>> @@ -1023,10 +1036,10 @@ static void rpmsg_remove(struct virtio_device *vdev)
> >>>       vdev->config->del_vqs(vrp->vdev);
> >>>
> >>>       dma_free_coherent(vdev->dev.parent->parent,
> >>> -                       vrp->num_sbufs * vrp->buf_size,
> >>> +                       vrp->num_sbufs * vrp->sbuf_size,
> >>>                         vrp->sbufs, vrp->sbufs_dma);
> >>>       dma_free_coherent(vdev->dev.parent->parent,
> >>> -                       vrp->num_rbufs * vrp->buf_size,
> >>> +                       vrp->num_rbufs * vrp->rbuf_size,
> >>>                         vrp->rbufs, vrp->rbufs_dma);
> >>>
> >>>       kfree(vrp);
> >>> @@ -1039,6 +1052,7 @@ static struct virtio_device_id id_table[] = {
> >>>
> >>>  static unsigned int features[] = {
> >>>       VIRTIO_RPMSG_F_NS,
> >>> +     VIRTIO_RPMSG_F_BUFSZ,
> >>>  };
> >>>
> >>>  static struct virtio_driver virtio_ipc_driver = {
> >>> diff --git a/include/uapi/linux/virtio_rpmsg.h b/include/uapi/linux/virtio_rpmsg.h
> >>> new file mode 100644
> >>> index 0000000..24fa0dd
> >>> --- /dev/null
> >>> +++ b/include/uapi/linux/virtio_rpmsg.h
> Strange to define a user space API for kernel usage need. Could you
> elaborate?

I just follow the practice other virtio drivers(e.g.
include/uapi/virtio_net.h) applied, but rpmsg driver don't need to
talk with the host VM software like other virtio driver, yes this
header file isn't really needed.

> >>> @@ -0,0 +1,24 @@
> >>> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> >>> +/*
> >>> + * Copyright (C) Pinecone Inc. 2019
> >>> + * Copyright (C) Xiang Xiao <xiaoxiang@pinecone.net>
> >>> + */
> >>> +
> >>> +#ifndef _UAPI_LINUX_VIRTIO_RPMSG_H
> >>> +#define _UAPI_LINUX_VIRTIO_RPMSG_H
> >>> +
> >>> +#include <linux/types.h>
> >>> +
> >>> +/* The feature bitmap for virtio rpmsg */
> >>> +#define VIRTIO_RPMSG_F_NS    0 /* RP supports name service notifications */
> >>> +#define VIRTIO_RPMSG_F_BUFSZ 2 /* RP get buffer size from config space */
> Would be useful to document it in rpmsg.txt

Good point, but it is better to put them into this document:
https://docs.oasis-open.org/virtio/virtio/v1.1/csprd01/virtio-v1.1-csprd01.html
like other vritio driver spec.

> >>> +
> >>> +struct virtio_rpmsg_config {
> >>> +     /* The tx/rx individual buffer size(if VIRTIO_RPMSG_F_BUFSZ) */
> >>> +     __u32 txbuf_size;
> >>> +     __u32 rxbuf_size;
> >>> +     __u32 reserved[14]; /* Reserve for the future use */
> >>> +     /* Put the customize config here */
> >>> +} __attribute__((packed));
> >>> +
> Wouldn't it be better to add an identifier and a version fields at the
> beginning of the structure? Idea would be to simplify a future extension
> In this case is VIRTIO_RPMSG_F_BUFSZ still useful?
>

Yes, I consider this option before, but after review all
include/uapi/virtio_*.h, I found that virito driver prefer feature
bits than version number to handle the compability issue.
For example, if we need introduce more options in the furture, we need:
1.Add new feature bit to notice the option exist
2.Allocate the field from reserved space

> >>> +#endif /* _UAPI_LINUX_VIRTIO_RPMSG_H */
> >>>
> --
> Thanks
> Arnaud

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 0/3] Enhance virtio rpmsg bus driver buffer allocation
  2019-01-31 15:41 [PATCH 0/3] Enhance virtio rpmsg bus driver buffer allocation Xiang Xiao
                   ` (2 preceding siblings ...)
  2019-01-31 15:41 ` [PATCH 3/3] rpmsg: virtio_rpmsg_bus: get buffer size from config space Xiang Xiao
@ 2019-06-05  4:34 ` Bjorn Andersson
  2019-06-05  7:33     ` Arnaud Pouliquen
  3 siblings, 1 reply; 23+ messages in thread
From: Bjorn Andersson @ 2019-06-05  4:34 UTC (permalink / raw)
  To: Xiang Xiao
  Cc: ohad, wendy.liang, arnaud.pouliquen, linux-remoteproc,
	linux-kernel, Xiang Xiao

On Thu 31 Jan 07:41 PST 2019, Xiang Xiao wrote:

> Hi,
> This series enhance the buffer allocation by:
> 1.Support the different buffer number in rx/tx direction
> 2.Get the individual rx/tx buffer size from config space
> 
> Here is the related OpenAMP change:
> https://github.com/OpenAMP/open-amp/pull/155
> 

This looks pretty reasonable, but can you confirm that it's possible to
use new firmware with an old Linux kernel when introducing this?

But ever since we discussed Loic's similar proposal earlier I've been
questioning if the fixed buffer size isn't just an artifact of how we
preallocate our buffers. The virtqueue seems to support arbitrary sizes
of buffers and I see that the receive function in OpenAMP has been fixed
to put back the buffer of the size that was received, rather than 512
bytes. So it seems like Linux would be able to send whatever size
messages to OpenAMP it would handle it.

The question is if we could do the same in the other direction, perhaps
by letting the OpenAMP side do it's message allocation when it's
sending, rather than Linux pushing inbufs to be filled by the remote.

This would remove the problem of always having suboptimal buffer sizes.

Regards,
Bjorn

> Xiang Xiao (3):
>   rpmsg: virtio_rpmsg_bus: allow the different vring size for send/recv
>   rpmsg: virtio_rpmsg_bus: allocate rx/tx buffer separately
>   rpmsg: virtio_rpmsg_bus: get buffer size from config space
> 
>  drivers/rpmsg/virtio_rpmsg_bus.c  | 127 +++++++++++++++++++++++---------------
>  include/uapi/linux/virtio_rpmsg.h |  24 +++++++
>  2 files changed, 100 insertions(+), 51 deletions(-)
>  create mode 100644 include/uapi/linux/virtio_rpmsg.h
> 
> -- 
> 2.7.4
> 

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 0/3] Enhance virtio rpmsg bus driver buffer allocation
  2019-06-05  4:34 ` [PATCH 0/3] Enhance virtio rpmsg bus driver buffer allocation Bjorn Andersson
@ 2019-06-05  7:33     ` Arnaud Pouliquen
  0 siblings, 0 replies; 23+ messages in thread
From: Arnaud Pouliquen @ 2019-06-05  7:33 UTC (permalink / raw)
  To: Bjorn Andersson, Xiang Xiao
  Cc: ohad, wendy.liang, linux-remoteproc, linux-kernel, Xiang Xiao

Hi Bjorn,

On 6/5/19 6:34 AM, Bjorn Andersson wrote:
> On Thu 31 Jan 07:41 PST 2019, Xiang Xiao wrote:
> 
>> Hi,
>> This series enhance the buffer allocation by:
>> 1.Support the different buffer number in rx/tx direction
>> 2.Get the individual rx/tx buffer size from config space
>>
>> Here is the related OpenAMP change:
>> https://github.com/OpenAMP/open-amp/pull/155
>>
> 
> This looks pretty reasonable, but can you confirm that it's possible to
> use new firmware with an old Linux kernel when introducing this?
> 
> 
> But ever since we discussed Loic's similar proposal earlier I've been
> questioning if the fixed buffer size isn't just an artifact of how we
> preallocate our buffers. The virtqueue seems to support arbitrary sizes
> of buffers and I see that the receive function in OpenAMP has been fixed
> to put back the buffer of the size that was received, rather than 512
> bytes. So it seems like Linux would be able to send whatever size
> messages to OpenAMP it would handle it.
> 
> The question is if we could do the same in the other direction, perhaps
> by letting the OpenAMP side do it's message allocation when it's
> sending, rather than Linux pushing inbufs to be filled by the remote.

IMHO, both could be useful and could be not correlated.
On-the fly buffer allocation seems more efficient but needs an
allocator.This can be a generic allocator (with a va to da) for system
where large amount of memories are accessible from both side.

Now what about system with small shared memory? In this case you have to
deal with a limited/optimized memory chunk. To avoid memory
fragmentation the allocator should have a pre-reserved buffers pool(so
similar to existing implementation). This serie seems useful to optimize
the size of the pre-reserved pool.

> 
> This would remove the problem of always having suboptimal buffer sizes.
> 
> Regards,
> Bjorn
> 
>> Xiang Xiao (3):
>>   rpmsg: virtio_rpmsg_bus: allow the different vring size for send/recv
>>   rpmsg: virtio_rpmsg_bus: allocate rx/tx buffer separately
>>   rpmsg: virtio_rpmsg_bus: get buffer size from config space
>>
>>  drivers/rpmsg/virtio_rpmsg_bus.c  | 127 +++++++++++++++++++++++---------------
>>  include/uapi/linux/virtio_rpmsg.h |  24 +++++++
>>  2 files changed, 100 insertions(+), 51 deletions(-)
>>  create mode 100644 include/uapi/linux/virtio_rpmsg.h
>>
>> -- 
>> 2.7.4
>>

--

Regards,
Arnaud

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 0/3] Enhance virtio rpmsg bus driver buffer allocation
@ 2019-06-05  7:33     ` Arnaud Pouliquen
  0 siblings, 0 replies; 23+ messages in thread
From: Arnaud Pouliquen @ 2019-06-05  7:33 UTC (permalink / raw)
  To: Bjorn Andersson, Xiang Xiao
  Cc: ohad, wendy.liang, linux-remoteproc, linux-kernel, Xiang Xiao

Hi Bjorn,

On 6/5/19 6:34 AM, Bjorn Andersson wrote:
> On Thu 31 Jan 07:41 PST 2019, Xiang Xiao wrote:
> 
>> Hi,
>> This series enhance the buffer allocation by:
>> 1.Support the different buffer number in rx/tx direction
>> 2.Get the individual rx/tx buffer size from config space
>>
>> Here is the related OpenAMP change:
>> https://github.com/OpenAMP/open-amp/pull/155
>>
> 
> This looks pretty reasonable, but can you confirm that it's possible to
> use new firmware with an old Linux kernel when introducing this?
> 
> 
> But ever since we discussed Loic's similar proposal earlier I've been
> questioning if the fixed buffer size isn't just an artifact of how we
> preallocate our buffers. The virtqueue seems to support arbitrary sizes
> of buffers and I see that the receive function in OpenAMP has been fixed
> to put back the buffer of the size that was received, rather than 512
> bytes. So it seems like Linux would be able to send whatever size
> messages to OpenAMP it would handle it.
> 
> The question is if we could do the same in the other direction, perhaps
> by letting the OpenAMP side do it's message allocation when it's
> sending, rather than Linux pushing inbufs to be filled by the remote.

IMHO, both could be useful and could be not correlated.
On-the fly buffer allocation seems more efficient but needs an
allocator.This can be a generic allocator (with a va to da) for system
where large amount of memories are accessible from both side.

Now what about system with small shared memory? In this case you have to
deal with a limited/optimized memory chunk. To avoid memory
fragmentation the allocator should have a pre-reserved buffers pool(so
similar to existing implementation). This serie seems useful to optimize
the size of the pre-reserved pool.

> 
> This would remove the problem of always having suboptimal buffer sizes.
> 
> Regards,
> Bjorn
> 
>> Xiang Xiao (3):
>>   rpmsg: virtio_rpmsg_bus: allow the different vring size for send/recv
>>   rpmsg: virtio_rpmsg_bus: allocate rx/tx buffer separately
>>   rpmsg: virtio_rpmsg_bus: get buffer size from config space
>>
>>  drivers/rpmsg/virtio_rpmsg_bus.c  | 127 +++++++++++++++++++++++---------------
>>  include/uapi/linux/virtio_rpmsg.h |  24 +++++++
>>  2 files changed, 100 insertions(+), 51 deletions(-)
>>  create mode 100644 include/uapi/linux/virtio_rpmsg.h
>>
>> -- 
>> 2.7.4
>>

--

Regards,
Arnaud

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 3/3] rpmsg: virtio_rpmsg_bus: get buffer size from config space
  2019-06-05  2:40         ` xiang xiao
@ 2019-06-05  8:02             ` Arnaud Pouliquen
  0 siblings, 0 replies; 23+ messages in thread
From: Arnaud Pouliquen @ 2019-06-05  8:02 UTC (permalink / raw)
  To: xiang xiao
  Cc: Ohad Ben Cohen, Bjorn Andersson, wendy.liang, linux-remoteproc,
	linux-kernel, Xiang Xiao, Loic PALLARDY, Suman Anna



On 6/5/19 4:40 AM, xiang xiao wrote:
> On Tue, Jun 4, 2019 at 10:25 PM Arnaud Pouliquen
> <arnaud.pouliquen@st.com> wrote:
>>
>> Hello Xiang,
>>
>> On 5/9/19 3:00 PM, xiang xiao wrote:
>>> On Thu, May 9, 2019 at 8:36 PM Arnaud Pouliquen <arnaud.pouliquen@st.com> wrote:
>>>>
>>>> Hello Xiang,
>>>>
>>>> Similar mechanism has been proposed by Loic 2 years ago (link to the
>>>> series here https://lkml.org/lkml/2017/3/28/349).
>>>>
>>>> Did you see them? Regarding history, patches seem just on hold...
>>>>
>>>
>>> Just saw this patchset, so it's common problem hit by many vendor,
>>> rpmsg framework need to address it.:)
>>>
>>>> Main differences (except interesting RX/TX size split) seems that you
>>>> - don't use the virtio_config_ops->get
>>>
>>> virtio_cread call virtio_config_ops->get internally, the ideal is same
>>> for both patch, just the implementation detail is different.
>>>
>>>> - define a new feature VIRTIO_RPMSG_F_NS.
>>>
>>> I add this flag to keep the compatibility with old remote peer, and
>>> also follow the common virito driver practice.
>> I discussed with Loic, he is ok to go further with your patch and
>> abandon his one. Please find some remarks below in-line
>>>
>>>>
>>>> Regards
>>>> Arnaud
>>>>
>>>>
>>>> On 1/31/19 4:41 PM, Xiang Xiao wrote:
>>>>> 512 bytes isn't always suitable for all case, let firmware
>>>>> maker decide the best value from resource table.
>>>>> enable by VIRTIO_RPMSG_F_BUFSZ feature bit.
>>>>>
>>>>> Signed-off-by: Xiang Xiao <xiaoxiang@xiaomi.com>
>>>>> ---
>>>>>  drivers/rpmsg/virtio_rpmsg_bus.c  | 50 +++++++++++++++++++++++++--------------
>>>>>  include/uapi/linux/virtio_rpmsg.h | 24 +++++++++++++++++++
>>>>>  2 files changed, 56 insertions(+), 18 deletions(-)
>>>>>  create mode 100644 include/uapi/linux/virtio_rpmsg.h
>>>>>
>>>>> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
>>>>> index 59c4554..049dd97 100644
>>>>> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
>>>>> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
>>>>> @@ -16,6 +16,7 @@
>>>>>  #include <linux/virtio.h>
>>>>>  #include <linux/virtio_ids.h>
>>>>>  #include <linux/virtio_config.h>
>>>>> +#include <linux/virtio_rpmsg.h>
>>>>>  #include <linux/scatterlist.h>
>>>>>  #include <linux/dma-mapping.h>
>>>>>  #include <linux/slab.h>
>>>>> @@ -38,7 +39,8 @@
>>>>>   * @sbufs:   kernel address of tx buffers
>>>>>   * @num_rbufs:       total number of buffers for rx
>>>>>   * @num_sbufs:       total number of buffers for tx
>>>>> - * @buf_size:        size of one rx or tx buffer
>>>>> + * @rbuf_size:       size of one rx buffer
>>>>> + * @sbuf_size:       size of one tx buffer
>>>>>   * @last_sbuf:       index of last tx buffer used
>>>>>   * @rbufs_dma:       dma base addr of rx buffers
>>>>>   * @sbufs_dma:       dma base addr of tx buffers
>>>>> @@ -61,7 +63,8 @@ struct virtproc_info {
>>>>>       void *rbufs, *sbufs;
>>>>>       unsigned int num_rbufs;
>>>>>       unsigned int num_sbufs;
>>>>> -     unsigned int buf_size;
>>>>> +     unsigned int rbuf_size;
>>>>> +     unsigned int sbuf_size;
>>>>>       int last_sbuf;
>>>>>       dma_addr_t rbufs_dma;
>>>>>       dma_addr_t sbufs_dma;
>>>>> @@ -73,9 +76,6 @@ struct virtproc_info {
>>>>>       struct rpmsg_endpoint *ns_ept;
>>>>>  };
>>>>>
>>>>> -/* The feature bitmap for virtio rpmsg */
>>>>> -#define VIRTIO_RPMSG_F_NS    0 /* RP supports name service notifications */
>>>>> -
>>>>>  /**
>>>>>   * struct rpmsg_hdr - common header for all rpmsg messages
>>>>>   * @src: source address
>>>>> @@ -452,7 +452,7 @@ static void *get_a_tx_buf(struct virtproc_info *vrp)
>>>>>
>>>>>       /* either pick the next unused tx buffer */
>>>>>       if (vrp->last_sbuf < vrp->num_sbufs)
>>>>> -             ret = vrp->sbufs + vrp->buf_size * vrp->last_sbuf++;
>>>>> +             ret = vrp->sbufs + vrp->sbuf_size * vrp->last_sbuf++;
>>>>>       /* or recycle a used one */
>>>>>       else
>>>>>               ret = virtqueue_get_buf(vrp->svq, &len);
>>>>> @@ -578,7 +578,7 @@ static int rpmsg_send_offchannel_raw(struct rpmsg_device *rpdev,
>>>>>        * messaging), or to improve the buffer allocator, to support
>>>>>        * variable-length buffer sizes.
>>>>>        */
>>>>> -     if (len > vrp->buf_size - sizeof(struct rpmsg_hdr)) {
>>>>> +     if (len > vrp->sbuf_size - sizeof(struct rpmsg_hdr)) {
>>>>>               dev_err(dev, "message is too big (%d)\n", len);
>>>>>               return -EMSGSIZE;
>>>>>       }
>>>>> @@ -718,7 +718,7 @@ static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
>>>>>        * We currently use fixed-sized buffers, so trivially sanitize
>>>>>        * the reported payload length.
>>>>>        */
>>>>> -     if (len > vrp->buf_size ||
>>>>> +     if (len > vrp->rbuf_size ||
>>>>>           msg->len > (len - sizeof(struct rpmsg_hdr))) {
>>>>>               dev_warn(dev, "inbound msg too big: (%d, %d)\n", len, msg->len);
>>>>>               return -EINVAL;
>>>>> @@ -751,7 +751,7 @@ static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
>>>>>               dev_warn(dev, "msg received with no recipient\n");
>>>>>
>>>>>       /* publish the real size of the buffer */
>>>>> -     rpmsg_sg_init(&sg, msg, vrp->buf_size);
>>>>> +     rpmsg_sg_init(&sg, msg, vrp->rbuf_size);
>>>>>
>>>>>       /* add the buffer back to the remote processor's virtqueue */
>>>>>       err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, msg, GFP_KERNEL);
>>>>> @@ -907,11 +907,24 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>>>>       else
>>>>>               vrp->num_sbufs = MAX_RPMSG_NUM_BUFS;
>>>>>
>>>>> -     vrp->buf_size = MAX_RPMSG_BUF_SIZE;
>>>>> +     /* try to get buffer size from config space */
>>>>> +     if (virtio_has_feature(vdev, VIRTIO_RPMSG_F_BUFSZ)) {
>>>>> +             /* note: virtio_rpmsg_config is defined from remote view */
>>>>> +             virtio_cread(vdev, struct virtio_rpmsg_config,
>>>>> +                          txbuf_size, &vrp->rbuf_size);
>>>>> +             virtio_cread(vdev, struct virtio_rpmsg_config,
>>>>> +                          rxbuf_size, &vrp->sbuf_size);
>>>>> +     }
>>>>> +
>>>>> +     /* use the default if resource table doesn't provide one */
>>>>> +     if (!vrp->rbuf_size)
>>>>> +             vrp->rbuf_size = MAX_RPMSG_BUF_SIZE;
>> In this case constant should be renamed DEFAULT_RPMSG_BUF_SIZE as it is
>> no more a max value
> 
> Yes, DEFAULT_RPMSG_BUF_SIZE is more reasonable now.
> 
>>>>> +     if (!vrp->sbuf_size)
>>>>> +             vrp->sbuf_size = MAX_RPMSG_BUF_SIZE;
>> Here, if the config space exists you need to update it in consequence to
>> ensure coherency with the remote processor config.
> 
> The update is already done in if (virtio_has_feature(vdev,
> VIRTIO_RPMSG_F_BUFSZ)), here just handle the zero value in config
> space which mean the remote side want to use the default value even
> VIRTIO_RPMSG_F_BUFSZ set.
> For example:
> 1.remote side want to change one direction buffer size, but keep
> another direction as default
> 2.or remote side want to change other config options(define in the
> furture) not the buffer size

In code above i can see a virtio_cread of the config structure, but no
writing of it...
I mentioned the configs space in the resource table itself.
Without an update, you must ensure that both have the same default
value... In addition, it makes sense that the master can update the
buffer size according to some other constraints.

> 
>>
>>>>>
>>>>>       /* allocate coherent memory for the buffers */
>>>>>       vrp->rbufs = dma_alloc_coherent(vdev->dev.parent->parent,
>>>>> -                                     vrp->num_rbufs * vrp->buf_size,
>>>>> +                                     vrp->num_rbufs * vrp->rbuf_size,
>>>>>                                       &vrp->rbufs_dma, GFP_KERNEL);
>>>>>       if (!vrp->rbufs) {
>>>>>               err = -ENOMEM;
>>>>> @@ -922,7 +935,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>>>>               vrp->rbufs, &vrp->rbufs_dma);
>>>>>
>>>>>       vrp->sbufs = dma_alloc_coherent(vdev->dev.parent->parent,
>>>>> -                                     vrp->num_sbufs * vrp->buf_size,
>>>>> +                                     vrp->num_sbufs * vrp->sbuf_size,
>>>>>                                       &vrp->sbufs_dma, GFP_KERNEL);
>>>>>       if (!vrp->sbufs) {
>>>>>               err = -ENOMEM;
>>>>> @@ -935,9 +948,9 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>>>>       /* set up the receive buffers */
>>>>>       for (i = 0; i < vrp->num_rbufs; i++) {
>>>>>               struct scatterlist sg;
>>>>> -             void *cpu_addr = vrp->rbufs + i * vrp->buf_size;
>>>>> +             void *cpu_addr = vrp->rbufs + i * vrp->rbuf_size;
>>>>>
>>>>> -             rpmsg_sg_init(&sg, cpu_addr, vrp->buf_size);
>>>>> +             rpmsg_sg_init(&sg, cpu_addr, vrp->rbuf_size);
>>>>>
>>>>>               err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, cpu_addr,
>>>>>                                         GFP_KERNEL);
>>>>> @@ -984,11 +997,11 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>>>>
>>>>>  free_sbufs:
>>>>>       dma_free_coherent(vdev->dev.parent->parent,
>>>>> -                       vrp->num_sbufs * vrp->buf_size,
>>>>> +                       vrp->num_sbufs * vrp->sbuf_size,
>>>>>                         vrp->sbufs, vrp->sbufs_dma);
>>>>>  free_rbufs:
>>>>>       dma_free_coherent(vdev->dev.parent->parent,
>>>>> -                       vrp->num_rbufs * vrp->buf_size,
>>>>> +                       vrp->num_rbufs * vrp->rbuf_size,
>>>>>                         vrp->rbufs, vrp->rbufs_dma);
>>>>>  vqs_del:
>>>>>       vdev->config->del_vqs(vrp->vdev);
>>>>> @@ -1023,10 +1036,10 @@ static void rpmsg_remove(struct virtio_device *vdev)
>>>>>       vdev->config->del_vqs(vrp->vdev);
>>>>>
>>>>>       dma_free_coherent(vdev->dev.parent->parent,
>>>>> -                       vrp->num_sbufs * vrp->buf_size,
>>>>> +                       vrp->num_sbufs * vrp->sbuf_size,
>>>>>                         vrp->sbufs, vrp->sbufs_dma);
>>>>>       dma_free_coherent(vdev->dev.parent->parent,
>>>>> -                       vrp->num_rbufs * vrp->buf_size,
>>>>> +                       vrp->num_rbufs * vrp->rbuf_size,
>>>>>                         vrp->rbufs, vrp->rbufs_dma);
>>>>>
>>>>>       kfree(vrp);
>>>>> @@ -1039,6 +1052,7 @@ static struct virtio_device_id id_table[] = {
>>>>>
>>>>>  static unsigned int features[] = {
>>>>>       VIRTIO_RPMSG_F_NS,
>>>>> +     VIRTIO_RPMSG_F_BUFSZ,
>>>>>  };
>>>>>
>>>>>  static struct virtio_driver virtio_ipc_driver = {
>>>>> diff --git a/include/uapi/linux/virtio_rpmsg.h b/include/uapi/linux/virtio_rpmsg.h
>>>>> new file mode 100644
>>>>> index 0000000..24fa0dd
>>>>> --- /dev/null
>>>>> +++ b/include/uapi/linux/virtio_rpmsg.h
>> Strange to define a user space API for kernel usage need. Could you
>> elaborate?
> 
> I just follow the practice other virtio drivers(e.g.
> include/uapi/virtio_net.h) applied, but rpmsg driver don't need to
> talk with the host VM software like other virtio driver, yes this
> header file isn't really needed.
> 
>>>>> @@ -0,0 +1,24 @@
>>>>> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
>>>>> +/*
>>>>> + * Copyright (C) Pinecone Inc. 2019
>>>>> + * Copyright (C) Xiang Xiao <xiaoxiang@pinecone.net>
>>>>> + */
>>>>> +
>>>>> +#ifndef _UAPI_LINUX_VIRTIO_RPMSG_H
>>>>> +#define _UAPI_LINUX_VIRTIO_RPMSG_H
>>>>> +
>>>>> +#include <linux/types.h>
>>>>> +
>>>>> +/* The feature bitmap for virtio rpmsg */
>>>>> +#define VIRTIO_RPMSG_F_NS    0 /* RP supports name service notifications */
>>>>> +#define VIRTIO_RPMSG_F_BUFSZ 2 /* RP get buffer size from config space */
>> Would be useful to document it in rpmsg.txt
> 
> Good point, but it is better to put them into this document:
> https://docs.oasis-open.org/virtio/virtio/v1.1/csprd01/virtio-v1.1-csprd01.html
> like other vritio driver spec.
> 
>>>>> +
>>>>> +struct virtio_rpmsg_config {
>>>>> +     /* The tx/rx individual buffer size(if VIRTIO_RPMSG_F_BUFSZ) */
>>>>> +     __u32 txbuf_size;
>>>>> +     __u32 rxbuf_size;
>>>>> +     __u32 reserved[14]; /* Reserve for the future use */
>>>>> +     /* Put the customize config here */
>>>>> +} __attribute__((packed));
>>>>> +
>> Wouldn't it be better to add an identifier and a version fields at the
>> beginning of the structure? Idea would be to simplify a future extension
>> In this case is VIRTIO_RPMSG_F_BUFSZ still useful?
>>
> 
> Yes, I consider this option before, but after review all
> include/uapi/virtio_*.h, I found that virito driver prefer feature
> bits than version number to handle the compability issue.
> For example, if we need introduce more options in the furture, we need:
> 1.Add new feature bit to notice the option exist
> 2.Allocate the field from reserved space
> 
>>>>> +#endif /* _UAPI_LINUX_VIRTIO_RPMSG_H */
>>>>>
>> --
>> Thanks
>> Arnaud

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 3/3] rpmsg: virtio_rpmsg_bus: get buffer size from config space
@ 2019-06-05  8:02             ` Arnaud Pouliquen
  0 siblings, 0 replies; 23+ messages in thread
From: Arnaud Pouliquen @ 2019-06-05  8:02 UTC (permalink / raw)
  To: xiang xiao
  Cc: Ohad Ben Cohen, Bjorn Andersson, wendy.liang, linux-remoteproc,
	linux-kernel, Xiang Xiao, Loic PALLARDY, Suman Anna



On 6/5/19 4:40 AM, xiang xiao wrote:
> On Tue, Jun 4, 2019 at 10:25 PM Arnaud Pouliquen
> <arnaud.pouliquen@st.com> wrote:
>>
>> Hello Xiang,
>>
>> On 5/9/19 3:00 PM, xiang xiao wrote:
>>> On Thu, May 9, 2019 at 8:36 PM Arnaud Pouliquen <arnaud.pouliquen@st.com> wrote:
>>>>
>>>> Hello Xiang,
>>>>
>>>> Similar mechanism has been proposed by Loic 2 years ago (link to the
>>>> series here https://lkml.org/lkml/2017/3/28/349).
>>>>
>>>> Did you see them? Regarding history, patches seem just on hold...
>>>>
>>>
>>> Just saw this patchset, so it's common problem hit by many vendor,
>>> rpmsg framework need to address it.:)
>>>
>>>> Main differences (except interesting RX/TX size split) seems that you
>>>> - don't use the virtio_config_ops->get
>>>
>>> virtio_cread call virtio_config_ops->get internally, the ideal is same
>>> for both patch, just the implementation detail is different.
>>>
>>>> - define a new feature VIRTIO_RPMSG_F_NS.
>>>
>>> I add this flag to keep the compatibility with old remote peer, and
>>> also follow the common virito driver practice.
>> I discussed with Loic, he is ok to go further with your patch and
>> abandon his one. Please find some remarks below in-line
>>>
>>>>
>>>> Regards
>>>> Arnaud
>>>>
>>>>
>>>> On 1/31/19 4:41 PM, Xiang Xiao wrote:
>>>>> 512 bytes isn't always suitable for all case, let firmware
>>>>> maker decide the best value from resource table.
>>>>> enable by VIRTIO_RPMSG_F_BUFSZ feature bit.
>>>>>
>>>>> Signed-off-by: Xiang Xiao <xiaoxiang@xiaomi.com>
>>>>> ---
>>>>>  drivers/rpmsg/virtio_rpmsg_bus.c  | 50 +++++++++++++++++++++++++--------------
>>>>>  include/uapi/linux/virtio_rpmsg.h | 24 +++++++++++++++++++
>>>>>  2 files changed, 56 insertions(+), 18 deletions(-)
>>>>>  create mode 100644 include/uapi/linux/virtio_rpmsg.h
>>>>>
>>>>> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
>>>>> index 59c4554..049dd97 100644
>>>>> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
>>>>> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
>>>>> @@ -16,6 +16,7 @@
>>>>>  #include <linux/virtio.h>
>>>>>  #include <linux/virtio_ids.h>
>>>>>  #include <linux/virtio_config.h>
>>>>> +#include <linux/virtio_rpmsg.h>
>>>>>  #include <linux/scatterlist.h>
>>>>>  #include <linux/dma-mapping.h>
>>>>>  #include <linux/slab.h>
>>>>> @@ -38,7 +39,8 @@
>>>>>   * @sbufs:   kernel address of tx buffers
>>>>>   * @num_rbufs:       total number of buffers for rx
>>>>>   * @num_sbufs:       total number of buffers for tx
>>>>> - * @buf_size:        size of one rx or tx buffer
>>>>> + * @rbuf_size:       size of one rx buffer
>>>>> + * @sbuf_size:       size of one tx buffer
>>>>>   * @last_sbuf:       index of last tx buffer used
>>>>>   * @rbufs_dma:       dma base addr of rx buffers
>>>>>   * @sbufs_dma:       dma base addr of tx buffers
>>>>> @@ -61,7 +63,8 @@ struct virtproc_info {
>>>>>       void *rbufs, *sbufs;
>>>>>       unsigned int num_rbufs;
>>>>>       unsigned int num_sbufs;
>>>>> -     unsigned int buf_size;
>>>>> +     unsigned int rbuf_size;
>>>>> +     unsigned int sbuf_size;
>>>>>       int last_sbuf;
>>>>>       dma_addr_t rbufs_dma;
>>>>>       dma_addr_t sbufs_dma;
>>>>> @@ -73,9 +76,6 @@ struct virtproc_info {
>>>>>       struct rpmsg_endpoint *ns_ept;
>>>>>  };
>>>>>
>>>>> -/* The feature bitmap for virtio rpmsg */
>>>>> -#define VIRTIO_RPMSG_F_NS    0 /* RP supports name service notifications */
>>>>> -
>>>>>  /**
>>>>>   * struct rpmsg_hdr - common header for all rpmsg messages
>>>>>   * @src: source address
>>>>> @@ -452,7 +452,7 @@ static void *get_a_tx_buf(struct virtproc_info *vrp)
>>>>>
>>>>>       /* either pick the next unused tx buffer */
>>>>>       if (vrp->last_sbuf < vrp->num_sbufs)
>>>>> -             ret = vrp->sbufs + vrp->buf_size * vrp->last_sbuf++;
>>>>> +             ret = vrp->sbufs + vrp->sbuf_size * vrp->last_sbuf++;
>>>>>       /* or recycle a used one */
>>>>>       else
>>>>>               ret = virtqueue_get_buf(vrp->svq, &len);
>>>>> @@ -578,7 +578,7 @@ static int rpmsg_send_offchannel_raw(struct rpmsg_device *rpdev,
>>>>>        * messaging), or to improve the buffer allocator, to support
>>>>>        * variable-length buffer sizes.
>>>>>        */
>>>>> -     if (len > vrp->buf_size - sizeof(struct rpmsg_hdr)) {
>>>>> +     if (len > vrp->sbuf_size - sizeof(struct rpmsg_hdr)) {
>>>>>               dev_err(dev, "message is too big (%d)\n", len);
>>>>>               return -EMSGSIZE;
>>>>>       }
>>>>> @@ -718,7 +718,7 @@ static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
>>>>>        * We currently use fixed-sized buffers, so trivially sanitize
>>>>>        * the reported payload length.
>>>>>        */
>>>>> -     if (len > vrp->buf_size ||
>>>>> +     if (len > vrp->rbuf_size ||
>>>>>           msg->len > (len - sizeof(struct rpmsg_hdr))) {
>>>>>               dev_warn(dev, "inbound msg too big: (%d, %d)\n", len, msg->len);
>>>>>               return -EINVAL;
>>>>> @@ -751,7 +751,7 @@ static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
>>>>>               dev_warn(dev, "msg received with no recipient\n");
>>>>>
>>>>>       /* publish the real size of the buffer */
>>>>> -     rpmsg_sg_init(&sg, msg, vrp->buf_size);
>>>>> +     rpmsg_sg_init(&sg, msg, vrp->rbuf_size);
>>>>>
>>>>>       /* add the buffer back to the remote processor's virtqueue */
>>>>>       err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, msg, GFP_KERNEL);
>>>>> @@ -907,11 +907,24 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>>>>       else
>>>>>               vrp->num_sbufs = MAX_RPMSG_NUM_BUFS;
>>>>>
>>>>> -     vrp->buf_size = MAX_RPMSG_BUF_SIZE;
>>>>> +     /* try to get buffer size from config space */
>>>>> +     if (virtio_has_feature(vdev, VIRTIO_RPMSG_F_BUFSZ)) {
>>>>> +             /* note: virtio_rpmsg_config is defined from remote view */
>>>>> +             virtio_cread(vdev, struct virtio_rpmsg_config,
>>>>> +                          txbuf_size, &vrp->rbuf_size);
>>>>> +             virtio_cread(vdev, struct virtio_rpmsg_config,
>>>>> +                          rxbuf_size, &vrp->sbuf_size);
>>>>> +     }
>>>>> +
>>>>> +     /* use the default if resource table doesn't provide one */
>>>>> +     if (!vrp->rbuf_size)
>>>>> +             vrp->rbuf_size = MAX_RPMSG_BUF_SIZE;
>> In this case constant should be renamed DEFAULT_RPMSG_BUF_SIZE as it is
>> no more a max value
> 
> Yes, DEFAULT_RPMSG_BUF_SIZE is more reasonable now.
> 
>>>>> +     if (!vrp->sbuf_size)
>>>>> +             vrp->sbuf_size = MAX_RPMSG_BUF_SIZE;
>> Here, if the config space exists you need to update it in consequence to
>> ensure coherency with the remote processor config.
> 
> The update is already done in if (virtio_has_feature(vdev,
> VIRTIO_RPMSG_F_BUFSZ)), here just handle the zero value in config
> space which mean the remote side want to use the default value even
> VIRTIO_RPMSG_F_BUFSZ set.
> For example:
> 1.remote side want to change one direction buffer size, but keep
> another direction as default
> 2.or remote side want to change other config options(define in the
> furture) not the buffer size

In code above i can see a virtio_cread of the config structure, but no
writing of it...
I mentioned the configs space in the resource table itself.
Without an update, you must ensure that both have the same default
value... In addition, it makes sense that the master can update the
buffer size according to some other constraints.

> 
>>
>>>>>
>>>>>       /* allocate coherent memory for the buffers */
>>>>>       vrp->rbufs = dma_alloc_coherent(vdev->dev.parent->parent,
>>>>> -                                     vrp->num_rbufs * vrp->buf_size,
>>>>> +                                     vrp->num_rbufs * vrp->rbuf_size,
>>>>>                                       &vrp->rbufs_dma, GFP_KERNEL);
>>>>>       if (!vrp->rbufs) {
>>>>>               err = -ENOMEM;
>>>>> @@ -922,7 +935,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>>>>               vrp->rbufs, &vrp->rbufs_dma);
>>>>>
>>>>>       vrp->sbufs = dma_alloc_coherent(vdev->dev.parent->parent,
>>>>> -                                     vrp->num_sbufs * vrp->buf_size,
>>>>> +                                     vrp->num_sbufs * vrp->sbuf_size,
>>>>>                                       &vrp->sbufs_dma, GFP_KERNEL);
>>>>>       if (!vrp->sbufs) {
>>>>>               err = -ENOMEM;
>>>>> @@ -935,9 +948,9 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>>>>       /* set up the receive buffers */
>>>>>       for (i = 0; i < vrp->num_rbufs; i++) {
>>>>>               struct scatterlist sg;
>>>>> -             void *cpu_addr = vrp->rbufs + i * vrp->buf_size;
>>>>> +             void *cpu_addr = vrp->rbufs + i * vrp->rbuf_size;
>>>>>
>>>>> -             rpmsg_sg_init(&sg, cpu_addr, vrp->buf_size);
>>>>> +             rpmsg_sg_init(&sg, cpu_addr, vrp->rbuf_size);
>>>>>
>>>>>               err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, cpu_addr,
>>>>>                                         GFP_KERNEL);
>>>>> @@ -984,11 +997,11 @@ static int rpmsg_probe(struct virtio_device *vdev)
>>>>>
>>>>>  free_sbufs:
>>>>>       dma_free_coherent(vdev->dev.parent->parent,
>>>>> -                       vrp->num_sbufs * vrp->buf_size,
>>>>> +                       vrp->num_sbufs * vrp->sbuf_size,
>>>>>                         vrp->sbufs, vrp->sbufs_dma);
>>>>>  free_rbufs:
>>>>>       dma_free_coherent(vdev->dev.parent->parent,
>>>>> -                       vrp->num_rbufs * vrp->buf_size,
>>>>> +                       vrp->num_rbufs * vrp->rbuf_size,
>>>>>                         vrp->rbufs, vrp->rbufs_dma);
>>>>>  vqs_del:
>>>>>       vdev->config->del_vqs(vrp->vdev);
>>>>> @@ -1023,10 +1036,10 @@ static void rpmsg_remove(struct virtio_device *vdev)
>>>>>       vdev->config->del_vqs(vrp->vdev);
>>>>>
>>>>>       dma_free_coherent(vdev->dev.parent->parent,
>>>>> -                       vrp->num_sbufs * vrp->buf_size,
>>>>> +                       vrp->num_sbufs * vrp->sbuf_size,
>>>>>                         vrp->sbufs, vrp->sbufs_dma);
>>>>>       dma_free_coherent(vdev->dev.parent->parent,
>>>>> -                       vrp->num_rbufs * vrp->buf_size,
>>>>> +                       vrp->num_rbufs * vrp->rbuf_size,
>>>>>                         vrp->rbufs, vrp->rbufs_dma);
>>>>>
>>>>>       kfree(vrp);
>>>>> @@ -1039,6 +1052,7 @@ static struct virtio_device_id id_table[] = {
>>>>>
>>>>>  static unsigned int features[] = {
>>>>>       VIRTIO_RPMSG_F_NS,
>>>>> +     VIRTIO_RPMSG_F_BUFSZ,
>>>>>  };
>>>>>
>>>>>  static struct virtio_driver virtio_ipc_driver = {
>>>>> diff --git a/include/uapi/linux/virtio_rpmsg.h b/include/uapi/linux/virtio_rpmsg.h
>>>>> new file mode 100644
>>>>> index 0000000..24fa0dd
>>>>> --- /dev/null
>>>>> +++ b/include/uapi/linux/virtio_rpmsg.h
>> Strange to define a user space API for kernel usage need. Could you
>> elaborate?
> 
> I just follow the practice other virtio drivers(e.g.
> include/uapi/virtio_net.h) applied, but rpmsg driver don't need to
> talk with the host VM software like other virtio driver, yes this
> header file isn't really needed.
> 
>>>>> @@ -0,0 +1,24 @@
>>>>> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
>>>>> +/*
>>>>> + * Copyright (C) Pinecone Inc. 2019
>>>>> + * Copyright (C) Xiang Xiao <xiaoxiang@pinecone.net>
>>>>> + */
>>>>> +
>>>>> +#ifndef _UAPI_LINUX_VIRTIO_RPMSG_H
>>>>> +#define _UAPI_LINUX_VIRTIO_RPMSG_H
>>>>> +
>>>>> +#include <linux/types.h>
>>>>> +
>>>>> +/* The feature bitmap for virtio rpmsg */
>>>>> +#define VIRTIO_RPMSG_F_NS    0 /* RP supports name service notifications */
>>>>> +#define VIRTIO_RPMSG_F_BUFSZ 2 /* RP get buffer size from config space */
>> Would be useful to document it in rpmsg.txt
> 
> Good point, but it is better to put them into this document:
> https://docs.oasis-open.org/virtio/virtio/v1.1/csprd01/virtio-v1.1-csprd01.html
> like other vritio driver spec.
> 
>>>>> +
>>>>> +struct virtio_rpmsg_config {
>>>>> +     /* The tx/rx individual buffer size(if VIRTIO_RPMSG_F_BUFSZ) */
>>>>> +     __u32 txbuf_size;
>>>>> +     __u32 rxbuf_size;
>>>>> +     __u32 reserved[14]; /* Reserve for the future use */
>>>>> +     /* Put the customize config here */
>>>>> +} __attribute__((packed));
>>>>> +
>> Wouldn't it be better to add an identifier and a version fields at the
>> beginning of the structure? Idea would be to simplify a future extension
>> In this case is VIRTIO_RPMSG_F_BUFSZ still useful?
>>
> 
> Yes, I consider this option before, but after review all
> include/uapi/virtio_*.h, I found that virito driver prefer feature
> bits than version number to handle the compability issue.
> For example, if we need introduce more options in the furture, we need:
> 1.Add new feature bit to notice the option exist
> 2.Allocate the field from reserved space
> 
>>>>> +#endif /* _UAPI_LINUX_VIRTIO_RPMSG_H */
>>>>>
>> --
>> Thanks
>> Arnaud

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 0/3] Enhance virtio rpmsg bus driver buffer allocation
  2019-06-05  7:33     ` Arnaud Pouliquen
  (?)
@ 2019-06-05  8:35     ` xiang xiao
  -1 siblings, 0 replies; 23+ messages in thread
From: xiang xiao @ 2019-06-05  8:35 UTC (permalink / raw)
  To: Arnaud Pouliquen
  Cc: Bjorn Andersson, Ohad Ben Cohen, wendy.liang, linux-remoteproc,
	linux-kernel, Xiang Xiao

On Wed, Jun 5, 2019 at 3:33 PM Arnaud Pouliquen <arnaud.pouliquen@st.com> wrote:
>
> Hi Bjorn,
>
> On 6/5/19 6:34 AM, Bjorn Andersson wrote:
> > On Thu 31 Jan 07:41 PST 2019, Xiang Xiao wrote:
> >
> >> Hi,
> >> This series enhance the buffer allocation by:
> >> 1.Support the different buffer number in rx/tx direction
> >> 2.Get the individual rx/tx buffer size from config space
> >>
> >> Here is the related OpenAMP change:
> >> https://github.com/OpenAMP/open-amp/pull/155
> >>
> >
> > This looks pretty reasonable, but can you confirm that it's possible to
> > use new firmware with an old Linux kernel when introducing this?
> >
> >
> > But ever since we discussed Loic's similar proposal earlier I've been
> > questioning if the fixed buffer size isn't just an artifact of how we
> > preallocate our buffers. The virtqueue seems to support arbitrary sizes
> > of buffers and I see that the receive function in OpenAMP has been fixed
> > to put back the buffer of the size that was received, rather than 512
> > bytes. So it seems like Linux would be able to send whatever size
> > messages to OpenAMP it would handle it.
> >
> > The question is if we could do the same in the other direction, perhaps
> > by letting the OpenAMP side do it's message allocation when it's
> > sending, rather than Linux pushing inbufs to be filled by the remote.
>
> IMHO, both could be useful and could be not correlated.
> On-the fly buffer allocation seems more efficient but needs an
> allocator.This can be a generic allocator (with a va to da) for system
> where large amount of memories are accessible from both side.
>
> Now what about system with small shared memory? In this case you have to
> deal with a limited/optimized memory chunk. To avoid memory
> fragmentation the allocator should have a pre-reserved buffers pool(so
> similar to existing implementation). This serie seems useful to optimize
> the size of the pre-reserved pool.
>

Maybe we can reuse rxbuf_size/txbuf_size to trigger the different
allocation policy:
1.If buf_size equal 0xfffffff, turn on the dynamic allocator
2.If buf_size equall 0, turn on the fixed allocator with the default buffer size
3.otherwise, turn on the fixed allocator with the configed buffer size
So, both requirement could be satisfied without breaking the compatibility.

> >
> > This would remove the problem of always having suboptimal buffer sizes.
> >
> > Regards,
> > Bjorn
> >
> >> Xiang Xiao (3):
> >>   rpmsg: virtio_rpmsg_bus: allow the different vring size for send/recv
> >>   rpmsg: virtio_rpmsg_bus: allocate rx/tx buffer separately
> >>   rpmsg: virtio_rpmsg_bus: get buffer size from config space
> >>
> >>  drivers/rpmsg/virtio_rpmsg_bus.c  | 127 +++++++++++++++++++++++---------------
> >>  include/uapi/linux/virtio_rpmsg.h |  24 +++++++
> >>  2 files changed, 100 insertions(+), 51 deletions(-)
> >>  create mode 100644 include/uapi/linux/virtio_rpmsg.h
> >>
> >> --
> >> 2.7.4
> >>
>
> --
>
> Regards,
> Arnaud

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 3/3] rpmsg: virtio_rpmsg_bus: get buffer size from config space
  2019-06-05  8:02             ` Arnaud Pouliquen
  (?)
@ 2019-06-05  8:36             ` xiang xiao
  -1 siblings, 0 replies; 23+ messages in thread
From: xiang xiao @ 2019-06-05  8:36 UTC (permalink / raw)
  To: Arnaud Pouliquen
  Cc: Ohad Ben Cohen, Bjorn Andersson, wendy.liang, linux-remoteproc,
	linux-kernel, Xiang Xiao, Loic PALLARDY, Suman Anna

On Wed, Jun 5, 2019 at 4:02 PM Arnaud Pouliquen <arnaud.pouliquen@st.com> wrote:
>
>
>
> On 6/5/19 4:40 AM, xiang xiao wrote:
> > On Tue, Jun 4, 2019 at 10:25 PM Arnaud Pouliquen
> > <arnaud.pouliquen@st.com> wrote:
> >>
> >> Hello Xiang,
> >>
> >> On 5/9/19 3:00 PM, xiang xiao wrote:
> >>> On Thu, May 9, 2019 at 8:36 PM Arnaud Pouliquen <arnaud.pouliquen@st.com> wrote:
> >>>>
> >>>> Hello Xiang,
> >>>>
> >>>> Similar mechanism has been proposed by Loic 2 years ago (link to the
> >>>> series here https://lkml.org/lkml/2017/3/28/349).
> >>>>
> >>>> Did you see them? Regarding history, patches seem just on hold...
> >>>>
> >>>
> >>> Just saw this patchset, so it's common problem hit by many vendor,
> >>> rpmsg framework need to address it.:)
> >>>
> >>>> Main differences (except interesting RX/TX size split) seems that you
> >>>> - don't use the virtio_config_ops->get
> >>>
> >>> virtio_cread call virtio_config_ops->get internally, the ideal is same
> >>> for both patch, just the implementation detail is different.
> >>>
> >>>> - define a new feature VIRTIO_RPMSG_F_NS.
> >>>
> >>> I add this flag to keep the compatibility with old remote peer, and
> >>> also follow the common virito driver practice.
> >> I discussed with Loic, he is ok to go further with your patch and
> >> abandon his one. Please find some remarks below in-line
> >>>
> >>>>
> >>>> Regards
> >>>> Arnaud
> >>>>
> >>>>
> >>>> On 1/31/19 4:41 PM, Xiang Xiao wrote:
> >>>>> 512 bytes isn't always suitable for all case, let firmware
> >>>>> maker decide the best value from resource table.
> >>>>> enable by VIRTIO_RPMSG_F_BUFSZ feature bit.
> >>>>>
> >>>>> Signed-off-by: Xiang Xiao <xiaoxiang@xiaomi.com>
> >>>>> ---
> >>>>>  drivers/rpmsg/virtio_rpmsg_bus.c  | 50 +++++++++++++++++++++++++--------------
> >>>>>  include/uapi/linux/virtio_rpmsg.h | 24 +++++++++++++++++++
> >>>>>  2 files changed, 56 insertions(+), 18 deletions(-)
> >>>>>  create mode 100644 include/uapi/linux/virtio_rpmsg.h
> >>>>>
> >>>>> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> >>>>> index 59c4554..049dd97 100644
> >>>>> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> >>>>> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> >>>>> @@ -16,6 +16,7 @@
> >>>>>  #include <linux/virtio.h>
> >>>>>  #include <linux/virtio_ids.h>
> >>>>>  #include <linux/virtio_config.h>
> >>>>> +#include <linux/virtio_rpmsg.h>
> >>>>>  #include <linux/scatterlist.h>
> >>>>>  #include <linux/dma-mapping.h>
> >>>>>  #include <linux/slab.h>
> >>>>> @@ -38,7 +39,8 @@
> >>>>>   * @sbufs:   kernel address of tx buffers
> >>>>>   * @num_rbufs:       total number of buffers for rx
> >>>>>   * @num_sbufs:       total number of buffers for tx
> >>>>> - * @buf_size:        size of one rx or tx buffer
> >>>>> + * @rbuf_size:       size of one rx buffer
> >>>>> + * @sbuf_size:       size of one tx buffer
> >>>>>   * @last_sbuf:       index of last tx buffer used
> >>>>>   * @rbufs_dma:       dma base addr of rx buffers
> >>>>>   * @sbufs_dma:       dma base addr of tx buffers
> >>>>> @@ -61,7 +63,8 @@ struct virtproc_info {
> >>>>>       void *rbufs, *sbufs;
> >>>>>       unsigned int num_rbufs;
> >>>>>       unsigned int num_sbufs;
> >>>>> -     unsigned int buf_size;
> >>>>> +     unsigned int rbuf_size;
> >>>>> +     unsigned int sbuf_size;
> >>>>>       int last_sbuf;
> >>>>>       dma_addr_t rbufs_dma;
> >>>>>       dma_addr_t sbufs_dma;
> >>>>> @@ -73,9 +76,6 @@ struct virtproc_info {
> >>>>>       struct rpmsg_endpoint *ns_ept;
> >>>>>  };
> >>>>>
> >>>>> -/* The feature bitmap for virtio rpmsg */
> >>>>> -#define VIRTIO_RPMSG_F_NS    0 /* RP supports name service notifications */
> >>>>> -
> >>>>>  /**
> >>>>>   * struct rpmsg_hdr - common header for all rpmsg messages
> >>>>>   * @src: source address
> >>>>> @@ -452,7 +452,7 @@ static void *get_a_tx_buf(struct virtproc_info *vrp)
> >>>>>
> >>>>>       /* either pick the next unused tx buffer */
> >>>>>       if (vrp->last_sbuf < vrp->num_sbufs)
> >>>>> -             ret = vrp->sbufs + vrp->buf_size * vrp->last_sbuf++;
> >>>>> +             ret = vrp->sbufs + vrp->sbuf_size * vrp->last_sbuf++;
> >>>>>       /* or recycle a used one */
> >>>>>       else
> >>>>>               ret = virtqueue_get_buf(vrp->svq, &len);
> >>>>> @@ -578,7 +578,7 @@ static int rpmsg_send_offchannel_raw(struct rpmsg_device *rpdev,
> >>>>>        * messaging), or to improve the buffer allocator, to support
> >>>>>        * variable-length buffer sizes.
> >>>>>        */
> >>>>> -     if (len > vrp->buf_size - sizeof(struct rpmsg_hdr)) {
> >>>>> +     if (len > vrp->sbuf_size - sizeof(struct rpmsg_hdr)) {
> >>>>>               dev_err(dev, "message is too big (%d)\n", len);
> >>>>>               return -EMSGSIZE;
> >>>>>       }
> >>>>> @@ -718,7 +718,7 @@ static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
> >>>>>        * We currently use fixed-sized buffers, so trivially sanitize
> >>>>>        * the reported payload length.
> >>>>>        */
> >>>>> -     if (len > vrp->buf_size ||
> >>>>> +     if (len > vrp->rbuf_size ||
> >>>>>           msg->len > (len - sizeof(struct rpmsg_hdr))) {
> >>>>>               dev_warn(dev, "inbound msg too big: (%d, %d)\n", len, msg->len);
> >>>>>               return -EINVAL;
> >>>>> @@ -751,7 +751,7 @@ static int rpmsg_recv_single(struct virtproc_info *vrp, struct device *dev,
> >>>>>               dev_warn(dev, "msg received with no recipient\n");
> >>>>>
> >>>>>       /* publish the real size of the buffer */
> >>>>> -     rpmsg_sg_init(&sg, msg, vrp->buf_size);
> >>>>> +     rpmsg_sg_init(&sg, msg, vrp->rbuf_size);
> >>>>>
> >>>>>       /* add the buffer back to the remote processor's virtqueue */
> >>>>>       err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, msg, GFP_KERNEL);
> >>>>> @@ -907,11 +907,24 @@ static int rpmsg_probe(struct virtio_device *vdev)
> >>>>>       else
> >>>>>               vrp->num_sbufs = MAX_RPMSG_NUM_BUFS;
> >>>>>
> >>>>> -     vrp->buf_size = MAX_RPMSG_BUF_SIZE;
> >>>>> +     /* try to get buffer size from config space */
> >>>>> +     if (virtio_has_feature(vdev, VIRTIO_RPMSG_F_BUFSZ)) {
> >>>>> +             /* note: virtio_rpmsg_config is defined from remote view */
> >>>>> +             virtio_cread(vdev, struct virtio_rpmsg_config,
> >>>>> +                          txbuf_size, &vrp->rbuf_size);
> >>>>> +             virtio_cread(vdev, struct virtio_rpmsg_config,
> >>>>> +                          rxbuf_size, &vrp->sbuf_size);
> >>>>> +     }
> >>>>> +
> >>>>> +     /* use the default if resource table doesn't provide one */
> >>>>> +     if (!vrp->rbuf_size)
> >>>>> +             vrp->rbuf_size = MAX_RPMSG_BUF_SIZE;
> >> In this case constant should be renamed DEFAULT_RPMSG_BUF_SIZE as it is
> >> no more a max value
> >
> > Yes, DEFAULT_RPMSG_BUF_SIZE is more reasonable now.
> >
> >>>>> +     if (!vrp->sbuf_size)
> >>>>> +             vrp->sbuf_size = MAX_RPMSG_BUF_SIZE;
> >> Here, if the config space exists you need to update it in consequence to
> >> ensure coherency with the remote processor config.
> >
> > The update is already done in if (virtio_has_feature(vdev,
> > VIRTIO_RPMSG_F_BUFSZ)), here just handle the zero value in config
> > space which mean the remote side want to use the default value even
> > VIRTIO_RPMSG_F_BUFSZ set.
> > For example:
> > 1.remote side want to change one direction buffer size, but keep
> > another direction as default
> > 2.or remote side want to change other config options(define in the
> > furture) not the buffer size
>
> In code above i can see a virtio_cread of the config structure, but no
> writing of it...
> I mentioned the configs space in the resource table itself.
> Without an update, you must ensure that both have the same default
> value... In addition, it makes sense that the master can update the
> buffer size according to some other constraints.

Get your point, thanks.

>
> >
> >>
> >>>>>
> >>>>>       /* allocate coherent memory for the buffers */
> >>>>>       vrp->rbufs = dma_alloc_coherent(vdev->dev.parent->parent,
> >>>>> -                                     vrp->num_rbufs * vrp->buf_size,
> >>>>> +                                     vrp->num_rbufs * vrp->rbuf_size,
> >>>>>                                       &vrp->rbufs_dma, GFP_KERNEL);
> >>>>>       if (!vrp->rbufs) {
> >>>>>               err = -ENOMEM;
> >>>>> @@ -922,7 +935,7 @@ static int rpmsg_probe(struct virtio_device *vdev)
> >>>>>               vrp->rbufs, &vrp->rbufs_dma);
> >>>>>
> >>>>>       vrp->sbufs = dma_alloc_coherent(vdev->dev.parent->parent,
> >>>>> -                                     vrp->num_sbufs * vrp->buf_size,
> >>>>> +                                     vrp->num_sbufs * vrp->sbuf_size,
> >>>>>                                       &vrp->sbufs_dma, GFP_KERNEL);
> >>>>>       if (!vrp->sbufs) {
> >>>>>               err = -ENOMEM;
> >>>>> @@ -935,9 +948,9 @@ static int rpmsg_probe(struct virtio_device *vdev)
> >>>>>       /* set up the receive buffers */
> >>>>>       for (i = 0; i < vrp->num_rbufs; i++) {
> >>>>>               struct scatterlist sg;
> >>>>> -             void *cpu_addr = vrp->rbufs + i * vrp->buf_size;
> >>>>> +             void *cpu_addr = vrp->rbufs + i * vrp->rbuf_size;
> >>>>>
> >>>>> -             rpmsg_sg_init(&sg, cpu_addr, vrp->buf_size);
> >>>>> +             rpmsg_sg_init(&sg, cpu_addr, vrp->rbuf_size);
> >>>>>
> >>>>>               err = virtqueue_add_inbuf(vrp->rvq, &sg, 1, cpu_addr,
> >>>>>                                         GFP_KERNEL);
> >>>>> @@ -984,11 +997,11 @@ static int rpmsg_probe(struct virtio_device *vdev)
> >>>>>
> >>>>>  free_sbufs:
> >>>>>       dma_free_coherent(vdev->dev.parent->parent,
> >>>>> -                       vrp->num_sbufs * vrp->buf_size,
> >>>>> +                       vrp->num_sbufs * vrp->sbuf_size,
> >>>>>                         vrp->sbufs, vrp->sbufs_dma);
> >>>>>  free_rbufs:
> >>>>>       dma_free_coherent(vdev->dev.parent->parent,
> >>>>> -                       vrp->num_rbufs * vrp->buf_size,
> >>>>> +                       vrp->num_rbufs * vrp->rbuf_size,
> >>>>>                         vrp->rbufs, vrp->rbufs_dma);
> >>>>>  vqs_del:
> >>>>>       vdev->config->del_vqs(vrp->vdev);
> >>>>> @@ -1023,10 +1036,10 @@ static void rpmsg_remove(struct virtio_device *vdev)
> >>>>>       vdev->config->del_vqs(vrp->vdev);
> >>>>>
> >>>>>       dma_free_coherent(vdev->dev.parent->parent,
> >>>>> -                       vrp->num_sbufs * vrp->buf_size,
> >>>>> +                       vrp->num_sbufs * vrp->sbuf_size,
> >>>>>                         vrp->sbufs, vrp->sbufs_dma);
> >>>>>       dma_free_coherent(vdev->dev.parent->parent,
> >>>>> -                       vrp->num_rbufs * vrp->buf_size,
> >>>>> +                       vrp->num_rbufs * vrp->rbuf_size,
> >>>>>                         vrp->rbufs, vrp->rbufs_dma);
> >>>>>
> >>>>>       kfree(vrp);
> >>>>> @@ -1039,6 +1052,7 @@ static struct virtio_device_id id_table[] = {
> >>>>>
> >>>>>  static unsigned int features[] = {
> >>>>>       VIRTIO_RPMSG_F_NS,
> >>>>> +     VIRTIO_RPMSG_F_BUFSZ,
> >>>>>  };
> >>>>>
> >>>>>  static struct virtio_driver virtio_ipc_driver = {
> >>>>> diff --git a/include/uapi/linux/virtio_rpmsg.h b/include/uapi/linux/virtio_rpmsg.h
> >>>>> new file mode 100644
> >>>>> index 0000000..24fa0dd
> >>>>> --- /dev/null
> >>>>> +++ b/include/uapi/linux/virtio_rpmsg.h
> >> Strange to define a user space API for kernel usage need. Could you
> >> elaborate?
> >
> > I just follow the practice other virtio drivers(e.g.
> > include/uapi/virtio_net.h) applied, but rpmsg driver don't need to
> > talk with the host VM software like other virtio driver, yes this
> > header file isn't really needed.
> >
> >>>>> @@ -0,0 +1,24 @@
> >>>>> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> >>>>> +/*
> >>>>> + * Copyright (C) Pinecone Inc. 2019
> >>>>> + * Copyright (C) Xiang Xiao <xiaoxiang@pinecone.net>
> >>>>> + */
> >>>>> +
> >>>>> +#ifndef _UAPI_LINUX_VIRTIO_RPMSG_H
> >>>>> +#define _UAPI_LINUX_VIRTIO_RPMSG_H
> >>>>> +
> >>>>> +#include <linux/types.h>
> >>>>> +
> >>>>> +/* The feature bitmap for virtio rpmsg */
> >>>>> +#define VIRTIO_RPMSG_F_NS    0 /* RP supports name service notifications */
> >>>>> +#define VIRTIO_RPMSG_F_BUFSZ 2 /* RP get buffer size from config space */
> >> Would be useful to document it in rpmsg.txt
> >
> > Good point, but it is better to put them into this document:
> > https://docs.oasis-open.org/virtio/virtio/v1.1/csprd01/virtio-v1.1-csprd01.html
> > like other vritio driver spec.
> >
> >>>>> +
> >>>>> +struct virtio_rpmsg_config {
> >>>>> +     /* The tx/rx individual buffer size(if VIRTIO_RPMSG_F_BUFSZ) */
> >>>>> +     __u32 txbuf_size;
> >>>>> +     __u32 rxbuf_size;
> >>>>> +     __u32 reserved[14]; /* Reserve for the future use */
> >>>>> +     /* Put the customize config here */
> >>>>> +} __attribute__((packed));
> >>>>> +
> >> Wouldn't it be better to add an identifier and a version fields at the
> >> beginning of the structure? Idea would be to simplify a future extension
> >> In this case is VIRTIO_RPMSG_F_BUFSZ still useful?
> >>
> >
> > Yes, I consider this option before, but after review all
> > include/uapi/virtio_*.h, I found that virito driver prefer feature
> > bits than version number to handle the compability issue.
> > For example, if we need introduce more options in the furture, we need:
> > 1.Add new feature bit to notice the option exist
> > 2.Allocate the field from reserved space
> >
> >>>>> +#endif /* _UAPI_LINUX_VIRTIO_RPMSG_H */
> >>>>>
> >> --
> >> Thanks
> >> Arnaud

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH 0/3] Enhance virtio rpmsg bus driver buffer allocation
  2019-06-05  7:33     ` Arnaud Pouliquen
  (?)
  (?)
@ 2019-07-01  6:13     ` Bjorn Andersson
  -1 siblings, 0 replies; 23+ messages in thread
From: Bjorn Andersson @ 2019-07-01  6:13 UTC (permalink / raw)
  To: Arnaud Pouliquen
  Cc: Xiang Xiao, ohad, wendy.liang, linux-remoteproc, linux-kernel,
	Xiang Xiao

On Wed 05 Jun 00:33 PDT 2019, Arnaud Pouliquen wrote:

> Hi Bjorn,
> 
> On 6/5/19 6:34 AM, Bjorn Andersson wrote:
> > On Thu 31 Jan 07:41 PST 2019, Xiang Xiao wrote:
> > 
> >> Hi,
> >> This series enhance the buffer allocation by:
> >> 1.Support the different buffer number in rx/tx direction
> >> 2.Get the individual rx/tx buffer size from config space
> >>
> >> Here is the related OpenAMP change:
> >> https://github.com/OpenAMP/open-amp/pull/155
> >>
> > 
> > This looks pretty reasonable, but can you confirm that it's possible to
> > use new firmware with an old Linux kernel when introducing this?
> > 
> > 
> > But ever since we discussed Loic's similar proposal earlier I've been
> > questioning if the fixed buffer size isn't just an artifact of how we
> > preallocate our buffers. The virtqueue seems to support arbitrary sizes
> > of buffers and I see that the receive function in OpenAMP has been fixed
> > to put back the buffer of the size that was received, rather than 512
> > bytes. So it seems like Linux would be able to send whatever size
> > messages to OpenAMP it would handle it.
> > 
> > The question is if we could do the same in the other direction, perhaps
> > by letting the OpenAMP side do it's message allocation when it's
> > sending, rather than Linux pushing inbufs to be filled by the remote.
> 
> IMHO, both could be useful and could be not correlated.
> On-the fly buffer allocation seems more efficient but needs an
> allocator.This can be a generic allocator (with a va to da) for system
> where large amount of memories are accessible from both side.
> 
> Now what about system with small shared memory? In this case you have to
> deal with a limited/optimized memory chunk. To avoid memory
> fragmentation the allocator should have a pre-reserved buffers pool(so
> similar to existing implementation). This serie seems useful to optimize
> the size of the pre-reserved pool.
> 

Having an implementation that uses small fixed size buffers seems very
reasonable and I'm in favour of making the message size configurable.

I would however prefer to have this implemented in a way where the
remote side should not be receiving messages in a way that's based on
the remote side's allocation parameters.


I don't think this series prevents the introduction of such isolation,
but it would render this code unnecessary.

Regards,
Bjorn

^ permalink raw reply	[flat|nested] 23+ messages in thread

end of thread, other threads:[~2019-07-01  6:13 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-31 15:41 [PATCH 0/3] Enhance virtio rpmsg bus driver buffer allocation Xiang Xiao
2019-01-31 15:41 ` [PATCH 1/3] rpmsg: virtio_rpmsg_bus: allow the different vring size for send/recv Xiang Xiao
2019-05-09 11:47   ` Arnaud Pouliquen
2019-05-09 11:47     ` Arnaud Pouliquen
2019-01-31 15:41 ` [PATCH 2/3] rpmsg: virtio_rpmsg_bus: allocate rx/tx buffer separately Xiang Xiao
2019-05-09 12:02   ` Arnaud Pouliquen
2019-05-09 12:02     ` Arnaud Pouliquen
2019-05-09 12:37     ` xiang xiao
2019-01-31 15:41 ` [PATCH 3/3] rpmsg: virtio_rpmsg_bus: get buffer size from config space Xiang Xiao
2019-05-09 12:36   ` Arnaud Pouliquen
2019-05-09 12:36     ` Arnaud Pouliquen
2019-05-09 13:00     ` xiang xiao
2019-06-04 14:25       ` Arnaud Pouliquen
2019-06-04 14:25         ` Arnaud Pouliquen
2019-06-05  2:40         ` xiang xiao
2019-06-05  8:02           ` Arnaud Pouliquen
2019-06-05  8:02             ` Arnaud Pouliquen
2019-06-05  8:36             ` xiang xiao
2019-06-05  4:34 ` [PATCH 0/3] Enhance virtio rpmsg bus driver buffer allocation Bjorn Andersson
2019-06-05  7:33   ` Arnaud Pouliquen
2019-06-05  7:33     ` Arnaud Pouliquen
2019-06-05  8:35     ` xiang xiao
2019-07-01  6:13     ` Bjorn Andersson

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.