Make rpmsg buffer size customizable per rpmsg instance

[hubertmis]

I'm looking for a way to efficiently transfer data between cores in an embedded system. I need to transfer small messages between cores A and B, and in parallel transfer longer messages between cores A and C. OpenAMP meets my needs. I can create one instance between cores A and B, and another one between A and C.

The problem I'm facing is inefficient usage of shared memory between cores A and B. It would be enough for me to allocate 16 64-bytes buffers, but with constant RPMSG_BUFFER_SIZE value I need to allocate 16 512-bytes buffers.

I'm aware that RPMSG_BUFFER_SIZE can be modified compile-time, but it does not solve my problem, because for A-C instance I need RPMSG_BUFFER_SIZE=512 while for A-B instance I would like to use RPMSG_BUFFER_SIZE=64.

I would like to propose similar solution to the one implemented in Linux https://elixir.bootlin.com/linux/v5.16-rc3/source/drivers/rpmsg/virtio_rpmsg_bus.c#L910 https://elixir.bootlin.com/linux/v5.16-rc3/source/drivers/rpmsg/virtio_rpmsg_bus.c#L61 and introduce buf_size field in struct rpmsg_virtio_device so that each instance of rpmsg could use different value of buffer sizes.

[xiaoxiang781216]

You can try this patch:
#155

[arnopo]

The #155 offers a very interesting feature, which allows to customize the size of RX and TX.
What should be decorrelated is the ability to customize buffer size and negotiation part.
Today, this PR is blocked because I would see a common dev on Linux and OpenAMP for negotiation.
Also, some devellopement like Zephyr one do not use the resource table for inter-processor communication, for negotiation.

But we can move on to the communication part of the buffer size based on @xiaoxiang781216 work.

what about adding a new API that could be called befor the rpmsg_init_vdev?

/**
 * struct fw_rsc_config - configuration space declaration
 * @txbuf_size: the tx buffer size
 * @rxbuf_size: the rx buffer size
 * @reserved: reserved (must be zero)
 *
 * This structure immediately follow fw_rsc_vdev to provide the config info.
 */
struct rpmsg_virtio_config {
	uint32_t txbuf_size;
	uint32_t rxbuf_size;
} 

struct rpmsg_virtio_device {
	struct rpmsg_device rdev;
	struct virtio_device *vdev;
+      const struct rrpmsg_virtio_config *config;
	struct virtqueue *rvq;
	struct virtqueue *svq;
	struct metal_io_region *shbuf_io;
	struct rpmsg_virtio_shm_pool *shpool;
};

rpmsg_virtio_set_config( struct rpmsg_virtio_device *rvdev, const struct  rpmsg_virtio_config * config)
{
	rvdev->config = config;     
}

This would allow to customize the size without impacting legay implementations. and kept the rpmsg decorellated from the resource table handled by the remoteproc. I suppose the negociation part could be reuse the API.

[xiaoxiang781216]

Yes, I think it could be the first step to the final dynamical negotiation, so both side can config the buffer size with the same value manually.

[hubertmis]

I like proposed idea. How shall we proceed with this? @xiaoxiang781216, are you planning to update your PR accordingly?

[xiaoxiang781216]

I don't have free time in this week. But since the first proposal is very simple, you may create a PR, @arnopo and I can help review the change.

[hubertmis]

I've started implementation, but it looks the proposal breaks current assumptions about code flow.

If rpmsg_virtio_set_config() were to be called before rpmsg_init_vdev(), it would need to set some data in rvdev structure. However, rpmsg_init_vdev() does not require that structure pointed by *rvdev is clear. It might be placed on a stack of a RTOS thread, full of garbage data and it would be responsibility of rpmsg_init_vdev() to set all fields in the structure.

Because of this rpmsg_init_vdev() cannot verify if config set by rpmsg_virtio_set_config() earlier is really set by this function, or is it garbage data that should be initialized with default value.

How about solving this by creating a new function rpmsg_init_vdev_with_config() which would take const struct rpmsg_virtio_config * as an additional parameter and set the config accordingly. If passed config == NULL, it would set default values.

@arnopo, what do you think?

[arnopo]

Yes it looks cleaner