Re: [PATCH 1/1] rpmsg: virtio: Make buffer size and number configurable

[Tanmay Shah]

On 10/3/23 10:28 AM, Divin Raj wrote:
> Hello,
>
> Thanks for all review comments.
> It makes sense to incorporate a variable-length buffer size. However, we still require an implementation that can successfully achieve this objective.
>
> We will be investigating this matter and will return with a feasible solution.

Thanks.

Here is the previous effort:

Series: "Enhance virtio rpmsg bus driver buffer allocation"

https://lore.kernel.org/all/1548949280-31794-1-git-send-email-xiaoxiang@xxxxxxxxxx/

There was discussion to revive the patch and the activity is still pending. This can be good start if we want to achieve it.

Thanks,

Tanmay


>
> Thanks,
>
> On 9/28/23 5:10 PM, Tanmay Shah wrote:
>
> Hello,
>
> Thanks for your patch.
>
> Instead of having this in Kconfig, It's better to have buffer size decided dynamically. Probably by resource-table.
>
> We still need implementation that achieves that goal. Meanwhile  I think it's best to keep buffer size fixed.
>
> Thanks.
>
> On 9/28/23 10:38 AM, Divin Raj wrote:
>
>
>
> CAUTION: This message has originated from an External Source. Please use proper judgment and caution when opening attachments, clicking links, or responding to this email.
>
>
> From: Peter Hoyes <Peter.Hoyes@xxxxxxx><mailto:Peter.Hoyes@xxxxxxx>
>
> Replace the MAX_RPMSG_BUF_SIZE and MAX_RPMSG_NUM_BUFS #define in
> virtio_rpmsg_bus.c with the Kconfig parameters CONFIG_RPMSG_VIRTIO_BUF_SIZE
> and CONFIG_RPMSG_VIRTIO_MAX_NUM_BUFS, allowing user-provided customization.
>
> Making both the number of buffers and size configurable facilitates aligning
> memory requirements between vdev-buffer and vdev-vrings for client drivers
> that require larger buffer sizes, for example.
>
> Signed-off-by: Peter Hoyes <Peter.Hoyes@xxxxxxx><mailto:Peter.Hoyes@xxxxxxx>
> Signed-off-by: Divin Raj <divin.raj@xxxxxxx><mailto:divin.raj@xxxxxxx>
> ---
>  drivers/rpmsg/Kconfig            | 23 +++++++++++++++++++++++
>  drivers/rpmsg/virtio_rpmsg_bus.c | 27 +++------------------------
>  2 files changed, 26 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig
> index d3795860f5c0..677f4a1ac8bb 100644
> --- a/drivers/rpmsg/Kconfig
> +++ b/drivers/rpmsg/Kconfig
> @@ -81,4 +81,27 @@ config RPMSG_VIRTIO
>         select RPMSG_NS
>         select VIRTIO
>
> +config RPMSG_VIRTIO_MAX_BUF_SIZE
> +       int "Virtio RPMSG max buffer size (in bytes)"
> +       default 512
> +       depends on RPMSG_VIRTIO
> +       help
> +         This option allows you to configure the maximum buffer size (in bytes)
> +         for Virtio RPMSG communications. The number of buffers will be computed
> +         based on the number of buffers (CONFIG_RPMSG_VIRTIO_MAX_NUM_BUFS)
> +         supported by the vring. By default, it supports up to a maximum of 512
> +         buffers (256 in each direction). Each buffer consists of 16 bytes for the
> +         message header and the remaining bytes for the payload.The default values
> +         will utilize a maximum total space of 256KB for the buffers.
> +
> +config RPMSG_VIRTIO_MAX_NUM_BUFS
> +       int "Virtio RPMSG max buffer count (even number for TX and Rx)"
> +       default 512
> +       depends on RPMSG_VIRTIO
> +       help
> +         This option allows you to configure the maximum number of buffers used
> +         for Virtio RPMSG communication. By default, it supports up to a maximum
> +         of 512 buffers (256 in each direction). Please note that this value
> +         should be an even number, as it accounts for both transmit (TX) and
> +         receive (Rx) buffers.
>  endmenu
> diff --git a/drivers/rpmsg/virtio_rpmsg_bus.c b/drivers/rpmsg/virtio_rpmsg_bus.c
> index 905ac7910c98..87a9a4fa30e0 100644
> --- a/drivers/rpmsg/virtio_rpmsg_bus.c
> +++ b/drivers/rpmsg/virtio_rpmsg_bus.c
> @@ -109,27 +109,6 @@ struct virtio_rpmsg_channel {
>  #define to_virtio_rpmsg_channel(_rpdev) \
>         container_of(_rpdev, struct virtio_rpmsg_channel, rpdev)
>
> -/*
> - * 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).
> - *
> - * Each buffer will have 16 bytes for the msg header and 496 bytes for
> - * the payload.
> - *
> - * This will utilize a maximum total space of 256KB for the buffers.
> - *
> - * We might also want to add support for user-provided buffers in time.
> - * This will allow bigger buffer size flexibility, and can also be used
> - * to achieve zero-copy messaging.
> - *
> - * Note that these numbers are purely a decision of this driver - we
> - * can change this without changing anything in the firmware of the remote
> - * processor.
> - */
> -#define MAX_RPMSG_NUM_BUFS     (512)
> -#define MAX_RPMSG_BUF_SIZE     (512)
> -
>  /*
>   * Local addresses are dynamically allocated on-demand.
>   * We do not dynamically assign addresses from the low 1024 range,
> @@ -902,12 +881,12 @@ static int rpmsg_probe(struct virtio_device *vdev)
>                 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)
> +       if (virtqueue_get_vring_size(vrp->rvq) < CONFIG_RPMSG_VIRTIO_MAX_NUM_BUFS / 2)
>                 vrp->num_bufs = virtqueue_get_vring_size(vrp->rvq) * 2;
>         else
> -               vrp->num_bufs = MAX_RPMSG_NUM_BUFS;
> +               vrp->num_bufs = CONFIG_RPMSG_VIRTIO_MAX_NUM_BUFS;
>
> -       vrp->buf_size = MAX_RPMSG_BUF_SIZE;
> +       vrp->buf_size = CONFIG_RPMSG_VIRTIO_MAX_BUF_SIZE;
>
>         total_buf_space = vrp->num_bufs * vrp->buf_size;
>
> --
> 2.25.1
>
>
>
> 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.
>