nxp-mcuxpresso / rpmsg-lite

RPMsg implementation for small MCUs
BSD 3-Clause "New" or "Revised" License
235 stars 74 forks source link

Update RL_BUFFER_COUNT Documentation #10

Closed workzb closed 3 years ago

workzb commented 3 years ago

After some testing I found that the RL_BUFFER_COUNT configuration parameter must be half of the total number of RPMsg buffers when interfacing with Linux RPMsg. The documentation says that it's the total number of buffers but the source code treats it as the number of buffers in one direction, so it's actually half of the total number of buffers.

Setting RL_BUFFER_COUNT to the total number of allocated buffers causes the memory map of the vrings to be incorrect.

EX: if Linux RPMSG_NUM_BUFS = 512 then RL_BUFFER_COUNT must equal 256.

Hadatko commented 3 years ago

Hi, i can confirm that current deliverables from nxp have these settings: Linux:

/*
 * For now, allocate 256 buffers of 512 bytes for each side. each buffer
 * will then have 16B for the msg header and 496B for the payload.
 * This will require a total space of 256KB for the buffers themselves, and
 * 3 pages for every vring (the size of the vring depends on the number of
 * buffers it supports).
 */
#define RPMSG_NUM_BUFS      (512)
#define RPMSG_BUF_SIZE      (512)
#define RPMSG_BUFS_SPACE    (RPMSG_NUM_BUFS * RPMSG_BUF_SIZE)

m4:

//! @def RL_BUFFER_PAYLOAD_SIZE
//!
//! Size of the buffer payload, it must be equal to (240, 496, 1008, ...)
//! [2^n - 16].
//! The default value is 496U.
#define RL_BUFFER_PAYLOAD_SIZE (496U)

//! @def RL_BUFFER_COUNT
//!
//! Number of the buffers, it must be power of two (2, 4, ...).
//! The default value is 2U.
#define RL_BUFFER_COUNT (256U)
workzb commented 3 years ago

For further clarification, the issue is caused by the RL_BUFFER_COUNT being used to calculate the space requirements for the desc table size. In the init functions these lines set the number of descs:

ring_info.phy_addr = (void *)(char *)((uint32_t)(char *)shmem_addr + (uint32_t)((idx == 0U) ? (0U) : (VRING_SIZE)));
ring_info.align     = VRING_ALIGN;
ring_info.num_descs = RL_BUFFER_COUNT;
...
status = virtqueue_create((uint16_t)(RL_GET_VQ_ID(link_id, idx)), vq_names[idx], &ring_info, callback[idx], virtqueue_notify, &vqs[idx]);

and then inside of the virtqueue_create(...) function it calls the vring_init(...) function which uses the num_descs as vq_nentries. In the vrint_init(...) function this is num:

static inline void vring_init(struct vring *vr, uint32_t num, uint8_t *p, uint32_t align)
{
    vr->num   = num;
    vr->desc  = (struct vring_desc *)(void *)p;
    vr->avail = (struct vring_avail *)(void *)(p + num * sizeof(struct vring_desc));
    vr->used  = (struct vring_used *)(((uint32_t)&vr->avail->ring[num] + align - 1UL) & ~(align - 1UL));
}

The problem here is that it uses RL_BUFFER_COUNT as the size of the desc table for each vq that is created when the actual length is the total divided by 2.

It also looks like rpmsg_lite_master_init() will create double the number of RL_BUFFER_COUNT of buffers because it allocates RL_BUFFER_COUNT for each direction. This will cause developers to take up more memory than they expected.

MichalPrincNXP commented 3 years ago

Hello @workzb , thank you for this report and analysis. I guess this is intended behavior (coded by my former colleague). rpmsg_lite must be able to communicate with rpmsg Linux implementation and also it can be used as "standalone" (rpmsg_lite master vs. rpmsg_lite remote). The behavior and the buffer management needs to be better documented. I will handle that. Thank you.

MichalPrincNXP commented 3 years ago

Doxygen description for RL_BUFFER_PAYLOAD_SIZE and RL_BUFFER_COUNT config macros updated internally, will be available in the next external release.