Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Negotiate individual buffer size dynamically #155

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

xiaoxiang781216
Copy link
Collaborator

@wjliang and @arnop2,
512 byte buffer size isn't suitable in all case, so this patch let slave specify the best size from resource table.
To keep the compatibility, the new behavior is guarded by VIRTIO_RPMSG_F_BUFSZ.

lib/remoteproc/remoteproc_virtio.c Outdated Show resolved Hide resolved
lib/rpmsg/rpmsg_virtio.c Outdated Show resolved Hide resolved
@xiaoxiang781216
Copy link
Collaborator Author

@wjliang and @arnop2 the new patch address all your comment, please review.

@wjliang
Copy link
Collaborator

wjliang commented Jan 29, 2019

@andersson, could you check this proposal to see if there is any conflict to the Linux kernel implementation?

@xiaoxiang781216
Copy link
Collaborator Author

@wjliang, @arnop2, @galak and @@andersson, here is patch for kernel side:
https://patchwork.kernel.org/cover/10790823/

@rjmiller62
Copy link

How about a generic bit for the RX/TX buf size settings as I believe that virtio-net devices will also need the ability to "set" the sizes vs having random values being picked by each front end?

@xiaoxiang781216
Copy link
Collaborator Author

Rebase to the latest mainline, no other change.

@xiaoxiang781216
Copy link
Collaborator Author

Move rpmsg_virtio_read_config after device status become ready.

@xiaoxiang781216
Copy link
Collaborator Author

How about a generic bit for the RX/TX buf size settings as I believe that virtio-net devices will also need the ability to "set" the sizes vs having random values being picked by each front end?

But from my reading virtio spec, the feature bit is specified by individual virtio device, nobody try to define the common bits before.

@xiaoxiang781216
Copy link
Collaborator Author

@arnopo @edmooring all my PR update to the latest mainline and pass the check, feel free give me the feedback.

@xiaoxiang781216 xiaoxiang781216 force-pushed the dynsize branch 3 times, most recently from c6f69a0 to d2fcb24 Compare April 26, 2020 13:15
@arnopo arnopo requested a review from edmooring May 4, 2020 12:10
@arnopo
Copy link
Collaborator

arnopo commented May 4, 2020

Hi @xiaoxiang781216,
During in the openamp-rp meeting, we started some discussions around the evolution of the resource table evolution including dynamic rpmsg buffer size. One advantage of using the resource table is that this also answers to solution that would not rely on virtio ( possible future extension).
I'm not in favor of implementing a temporary solution that would be revert if we decide to move to resource table.
That said, i don't remember if you pushed the patch on Linux side. But feel free to push it on resent it to trig the discussion.

@xiaoxiang781216
Copy link
Collaborator Author

xiaoxiang781216 commented May 5, 2020

Hi @xiaoxiang781216,
During in the openamp-rp meeting, we started some discussions around the evolution of the resource table evolution including dynamic rpmsg buffer size. One advantage of using the resource table is that this also answers to solution that would not rely on virtio ( possible future extension).
I'm not in favor of implementing a temporary solution that would be revert if we decide to move to resource table.

This method is also based on resource table. The key difference is where we put the information:
1.Add bufsize field or use reserved field in fw_rsc_vdev_vring:

struct fw_rsc_vdev_vring {
	uint32_t da;
	uint32_t align;
	uint32_t num;
	uint32_t notifyid;
	uint32_t reserved;
};

The problem is how we pass this information from remoteproc layer to rpmsg layer in a standard and compatible way.
2.Add buf_size to virtio config space(rpmsg doesn't use this before):

struct fw_rsc_config {
	/* The tx/rx individual buffer size(if VIRTIO_RPMSG_F_BUFSZ) */
	uint32_t txbuf_size;
	uint32_t rxbuf_size;
	uint32_t reserved[14]; /* Reserve for the future use */
	/* Put the customize config here */
};

Since virtio has many functions to access the config space, the 2nd method is more simple and compatible.
If you study virtio spec carefully, you should find that virtio already define how to expose the addtional information in the standard way:
https://docs.oasis-open.org/virtio/virtio/v1.1/csprd01/virtio-v1.1-csprd01.html
I suggest to read this spec first and learn how other virtio device resolve the similar issue before you modify the resource table. Many thing can be done with config space without breaking change.

That said, i don't remember if you pushed the patch on Linux side. But feel free to push it on resent it to trig the discussion.

Here is the patch set:
https://patchwork.kernel.org/cover/10790823/

@xiaoxiang781216 xiaoxiang781216 force-pushed the dynsize branch 2 times, most recently from 65cd46a to a2b5b59 Compare May 5, 2020 09:13
@hubertmis
Copy link
Contributor

What's the status of this PR? Any chances to merge it soon?

If not, would it be OK to split it in two:

  1. Configurable buffer sizes in rpmsg_virtio layer
  2. Setting buffer sizes configuration by remoteproc module

I understand the 2nd one must be compatible with Linux and requires more discussion. I hope the 1st one can be merged earlier.

@arnopo
Copy link
Collaborator

arnopo commented Dec 6, 2021

it seems that we come to the same conclusion (see #322 (comment))

If slave support VIRTIO_RPMSG_F_BUFSZ(0x04) feature, master
determine the buffer size from config space(first 8 bytes),
otherwise the default size(512 bytes) will be used.

Signed-off-by: Chao An <anchao@pinecone.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants