rpmsg_virtio: configuration option to set buffer sizes per instance by hubertmis · Pull Request #328 · OpenAMP/open-amp

hubertmis · 2021-12-13T10:59:57Z

Fixes #322

Enable user of rpmsg_virtio to set sizes of TX and RX buffers per
created rpmsg_virtio instance. Each instance can use other buffer sizes.

Signed-off-by: Hubert Miś hubert.mis@nordicsemi.no

hubertmis · 2021-12-14T09:12:18Z

I've tested this implementation with a Zephyr sample. Looks to work as expected

lib/include/openamp/rpmsg_virtio.h

carlocaione · 2021-12-14T09:38:23Z

lib/include/openamp/rpmsg_virtio.h

 		    struct rpmsg_virtio_shm_pool *shpool);

+/**
+ * rpmsg_init_vdev_with_config - initialize rpmsg virtio device


initialize rpmsg virtio device with config ?

carlocaione · 2021-12-14T09:40:02Z

lib/include/openamp/rpmsg_virtio.h

+ * Initialize RPMsg virtio queues and shared buffers, the address of shm can be
+ * ANY. In this case, function will get shared memory from system shared memory
+ * pools. If the vdev has RPMsg name service feature, this API will create an
+ * name service endpoint.
+ *
+ * Slave side:
+ * This API will not return until the driver ready is set by the master side.


Can we add here what config is?

carlocaione · 2021-12-14T09:44:28Z

lib/rpmsg/rpmsg_virtio.c

+		rvdev->config.txbuf_size = RPMSG_BUFFER_SIZE;
+		rvdev->config.rxbuf_size = RPMSG_BUFFER_SIZE;


Probably not but should we think about adding RPMSG_BUFFER_RX_SIZE and RPMSG_BUFFER_TX_SIZE as compile-time defines since we are now decoupling TX and RX?

I think if anyone needs it, it could be implemented in other patch

lib/rpmsg/rpmsg_virtio.c

arnopo

few minor remarks but LGTM

lib/rpmsg/rpmsg_virtio.c

arnopo · 2021-12-14T10:49:54Z

lib/include/openamp/rpmsg_virtio.h

+ * @param shpool - pointer to shared memory pool. rpmsg_virtio_init_shm_pool has
+ *                 to be called first to fill this structure.
+ * @param config - pointer to configuration structure, if NULL default
+ *                 configuration is used like in @ref rpmsg_init_vdev


confusing, proposal:
pointer to configuration structure, if NULL (like in @ref rpmsg_init_vdev) default configuration is used

arnopo · 2021-12-14T12:19:52Z

lib/rpmsg/rpmsg_virtio.c

+	if (config) {
+		rvdev->config = *config;
+	} else {
+		rvdev->config = RPMSG_VIRTIO_DEFAULT_CONFIG;


What about just past RPMSG_VIRTIO_DEFAULT_CONFIG as argument in rpmsg_init_vdev function and suppress this condition?

We would need some code to handle config == NULL. What do you suggest? Return RPMSG_ERR_PARAM?

carlocaione

LGTM

edmooring

The substance of this looks good to go. Regarding the terminology changes, if they don't go in, I can pick them up in a subsequent push to #323, since I think this will probably go in first.

This pull request could serve as a model of how to do them right.

lib/include/openamp/rpmsg_virtio.h

edmooring · 2021-12-18T22:48:11Z

lib/include/openamp/rpmsg_virtio.h


+/**
+ * rpmsg_init_vdev_with_config - initialize rpmsg virtio device with config
+ * Master side:


In keeping with the effort to remove potentially objectionable terminology, I suggest replacing "Master" and "Slave" with "Driver" and "Device" as in PR #323.

I'm changing Master and Slave here to Host and Remote. It seems this header should use RPMsg terminology (host/remote), not virtio (driver/device), because it operates on rpmsg instances.

edmooring · 2021-12-18T22:48:48Z

lib/include/openamp/rpmsg_virtio.h

+ * Sizes of virtio data buffers used by initialized RPMsg instance are set to
+ * values read from the passed configuration structure.
+ *
+ * Slave side:


As above, replace "Slave" with "Device". Check throughout the PR.

edmooring · 2021-12-18T22:50:37Z

lib/include/openamp/rpmsg_virtio.h

+ * Master side:
+ * Initialize RPMsg virtio queues and shared buffers, the address of shm can be
+ * ANY. In this case, function will get shared memory from system shared memory
+ * pools. If the vdev has RPMsg name service feature, this API will create an


Minor English fixups:
s/has RPMsg name service/has the RPMsg name service/
s/an$/a/

edmooring · 2021-12-18T22:51:44Z

lib/include/openamp/rpmsg_virtio.h

+ * ANY. In this case, function will get shared memory from system shared memory
+ * pools. If the vdev has RPMsg name service feature, this API will create an
+ * name service endpoint.
+ * Sizes of virtio data buffers used by initialized RPMsg instance are set to


s/initialized/the initialized/

lib/rpmsg/rpmsg_virtio.c

arnopo · 2021-12-20T10:26:50Z

lib/include/openamp/rpmsg_virtio.h

+/**
+ * struct rpmsg_virtio_config - configuration of rpmsg device based on virtio
+ * @txbuf_size: the tx buffer size, used to send data from host to remote
+ * @rxbuf_size: the rx buffer size, used to send data from remote to host


Deeply reviewing your patch, look to me that, for an unfamiliar user, it is not easy to understand this structure.
My apologize for that because I should see it before... only the host use the config structure.
Another point is the notion of RX TX which is ambiguous. If only the host defines it, the remote gets the corresponding values from the vrings.

Here is a proposal to rework the structure

* struct rpmsg_virtio_config - configuration of rpmsg device based on virtio * * This structure is use by the rpmsg virtio host to configure the virtiio layer. * * @h2r_buf_size: the buffer size, used to send data from host to remote * @r2h_buf_size: the uffer size, used to send data from remote to host struct rpmsg_virtio_config { uint32_t h2r_buf_size; uint32_t r2h_buf_size; }

arnopo · 2021-12-20T10:33:52Z

lib/include/openamp/rpmsg_virtio.h

 /**
 * struct rpmsg_virtio_device - representation of a rpmsg device based on virtio
 * @rdev: rpmsg device, first property in the struct
+ * @config: structure containing device configuration


@config: structure containing virtio configuration

arnopo · 2021-12-20T13:21:38Z

lib/rpmsg/rpmsg_virtio.c

 	int status;
 	unsigned int i, role;

+	role = rpmsg_virtio_get_role(rvdev);


Segmentation fault during my non-reg tests because the rvdev->vdev->role is not yet set.
The block should be move after rdev = &rvdev->rdev;

Enable user of rpmsg_virtio to set sizes of TX and RX buffers per created rpmsg_virtio instance. Each instance can use other buffer sizes. Signed-off-by: Hubert Miś <hubert.mis@nordicsemi.no>

Replacing potentially objectionable terms from the function description. Language fixes. Signed-off-by: Hubert Miś <hubert.mis@nordicsemi.no>

hubertmis · 2021-12-20T13:32:41Z

One more update: I've silenced "unused config parameter" warning when building SLAVE_ONLY configuration.

edmooring

Looks good to go.

hubertmis force-pushed the pr/configurable-rpmsg-virio-buffer-size branch 2 times, most recently from 807d53b to 30fc4ff Compare December 13, 2021 11:16

carlocaione reviewed Dec 14, 2021

View reviewed changes

lib/include/openamp/rpmsg_virtio.h Outdated Show resolved Hide resolved

carlocaione reviewed Dec 14, 2021

View reviewed changes

xiaoxiang781216 reviewed Dec 14, 2021

View reviewed changes

lib/rpmsg/rpmsg_virtio.c Outdated Show resolved Hide resolved

arnopo requested review from arnopo and edmooring December 14, 2021 10:08

arnopo reviewed Dec 14, 2021

View reviewed changes

hubertmis force-pushed the pr/configurable-rpmsg-virio-buffer-size branch from 30fc4ff to 55e9a34 Compare December 14, 2021 11:31

arnopo reviewed Dec 14, 2021

View reviewed changes

hubertmis force-pushed the pr/configurable-rpmsg-virio-buffer-size branch from 55e9a34 to 56f38cf Compare December 14, 2021 14:11

arnopo requested review from carlocaione and xiaoxiang781216 December 16, 2021 13:16

carlocaione approved these changes Dec 16, 2021

View reviewed changes

edmooring requested changes Dec 18, 2021

View reviewed changes

hubertmis force-pushed the pr/configurable-rpmsg-virio-buffer-size branch from 56f38cf to 19782c5 Compare December 20, 2021 08:50

hubertmis requested review from arnopo and edmooring December 20, 2021 08:50

hubertmis force-pushed the pr/configurable-rpmsg-virio-buffer-size branch from 19782c5 to 935ab1c Compare December 20, 2021 08:51

arnopo requested changes Dec 20, 2021

View reviewed changes

hubertmis force-pushed the pr/configurable-rpmsg-virio-buffer-size branch 2 times, most recently from 5e3c365 to 92a1aac Compare December 20, 2021 12:05

hubertmis requested a review from arnopo December 20, 2021 12:32

arnopo requested changes Dec 20, 2021

View reviewed changes

hubertmis force-pushed the pr/configurable-rpmsg-virio-buffer-size branch from 92a1aac to 73f61d0 Compare December 20, 2021 13:25

arnopo approved these changes Dec 20, 2021

View reviewed changes

rpmsg_virtio: configuration option to set buffer sizes per instance

e27e795

Enable user of rpmsg_virtio to set sizes of TX and RX buffers per created rpmsg_virtio instance. Each instance can use other buffer sizes. Signed-off-by: Hubert Miś <hubert.mis@nordicsemi.no>

rpmsg_virtio: docs fixes for the rpmsg_init_vdev function

409ab20

Replacing potentially objectionable terms from the function description. Language fixes. Signed-off-by: Hubert Miś <hubert.mis@nordicsemi.no>

hubertmis force-pushed the pr/configurable-rpmsg-virio-buffer-size branch from 73f61d0 to 409ab20 Compare December 20, 2021 13:31

arnopo added this to the Release V2022.04 milestone Dec 22, 2021

edmooring approved these changes Dec 24, 2021

View reviewed changes

arnopo merged commit a12d03c into OpenAMP:main Jan 4, 2022

arnopo mentioned this pull request May 4, 2022

rpmsg: Make RPMSG_BUFFER_SIZE a fixed define #319

Closed

		rvdev->config.txbuf_size = RPMSG_BUFFER_SIZE;
		rvdev->config.rxbuf_size = RPMSG_BUFFER_SIZE;

Conversation

hubertmis commented Dec 13, 2021

Uh oh!

hubertmis commented Dec 14, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

arnopo left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlocaione left a comment

Choose a reason for hiding this comment

Uh oh!

edmooring left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hubertmis commented Dec 20, 2021

Uh oh!

edmooring left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

arnopo left a comment •

edited

Loading