* [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; 24+ 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] 24+ 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; 24+ 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] 24+ 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; 24+ 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] 24+ 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; 24+ 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] 24+ 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; 24+ 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] 24+ 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; 24+ 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] 24+ 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; 24+ 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] 24+ 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; 24+ 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] 24+ 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; 24+ 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] 24+ 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; 24+ 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] 24+ 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; 24+ 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] 24+ 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; 24+ 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] 24+ 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; 24+ 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] 24+ 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; 24+ 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] 24+ 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; 24+ 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] 24+ 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; 24+ 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] 24+ 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; 24+ 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] 24+ 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; 24+ 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] 24+ 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; 24+ 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] 24+ 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; 24+ 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] 24+ 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; 24+ 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] 24+ 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; 24+ 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] 24+ 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; 24+ 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] 24+ messages in thread
[parent not found: <1af16ff8-5706-45e5-9737-05da39957c95@arm.com>]
* RE: [PATCH 0/3] Enhance virtio rpmsg bus driver buffer allocation [not found] <1af16ff8-5706-45e5-9737-05da39957c95@arm.com> @ 2023-10-24 8:09 ` Arnaud POULIQUEN 0 siblings, 0 replies; 24+ messages in thread From: Arnaud POULIQUEN @ 2023-10-24 8:09 UTC (permalink / raw) To: Divin Raj, linux-kernel, Xiang Xiao, wendy.liang, Ohad Ben Cohen, Bjorn Andersson, xiang xiao, Tanmay Shah Cc: Diego.Sueiro, Rahul.Singh Hello Divin, This development is currently on hold, but several people seem interested in it. I suggest that you reply directly to the email thread to continue the discussion and address the remoteproc mailing list Regards, Arnaud >From: Divin Raj <divin.raj@arm.com> >Sent: Monday, October 23, 2023 12:44 PM >To: linux-kernel@vger.kernel.org; Xiang Xiao <xiaoxiang@xiaomi.com>; wendy.liang@xilinx.com; Ohad Ben Cohen <ohad@wizery.com>; Bjorn Andersson <bjorn.andersson@linaro.org>; Arnaud >POULIQUEN <arnaud.pouliquen@st.com>; xiang xiao <xiaoxiang781216@gmail.com> >Cc: Diego.Sueiro@arm.com; Rahul.Singh@arm.com >Subject: Re: [PATCH 0/3] Enhance virtio rpmsg bus driver buffer allocation >Hello all, >I am reaching out with reference to the patch discussed here: https://lore.kernel.org/all/CAH2Cfb-sv3SAL8bcczC-Dc3_r58MYZCS7s7zGtn1Qfo3mmBqVg@mail.gmail.com/ >I've been keenly following the developments around enhancing buffer allocation strategies, especially those focused on dynamic buffer sizing and the considerations for systems under varying memory >constraints. This work is highly relevant to several projects I am involved in, and I am quite interested in its progression. May I kindly request an update on the current phase of these initiatives? >Additionally, I am eager to know if there would be an opportunity for me to contribute to enhancing the patch, possibly by working on improvements or assisting in verification processes. >Furthermore, if there are any condensed resources, summaries, or specific threads that encapsulate recent advancements or discussions on this topic, I would be grateful to receive directions to them. >I appreciate everyone's dedicated efforts and invaluable contributions to this area of development. Looking forward to the updates. >Regards Divin >IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not >disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you. ST Restricted ^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2023-10-24 8:09 UTC | newest] Thread overview: 24+ 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 [not found] <1af16ff8-5706-45e5-9737-05da39957c95@arm.com> 2023-10-24 8:09 ` Arnaud POULIQUEN
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.