openamp: decouple rpmsg virtio and remoteproc by wyr8899 · Pull Request #549 · OpenAMP/open-amp

wyr8899 · 2024-01-30T08:14:58Z

openamp: move notify_wait() in rpmsg virtio to rpmsg, this ops can do in rpmsg without coupling to virtio.

arnopo · 2024-01-30T10:51:31Z

lib/include/openamp/rpmsg.h

 typedef void (*rpmsg_ns_unbind_cb)(struct rpmsg_endpoint *ept);
 typedef void (*rpmsg_ns_bind_cb)(struct rpmsg_device *rdev,
 				 const char *name, uint32_t dest);
+typedef int (*rpmsg_notify_wait_cb)(struct rpmsg_device *rdev, uint32_t id);


This is not related to the rpmsg but to the transport layer, I would expect that this is part of the rpmsg_virtio.

but rpmsg could support other transport(e.g. uart/spi), this patch try to avoid couple the notify wait callback with the virtio transport.

My point here is that "id" is related to the vring so the transport layer. the Id will probably be use then to identify the associated mailbox.

@xiaoxiang781216 @arnopo So how about change the uint32_t id to void *arg to be compatible with all rpmsg transports ?

This does not solve the issue for me as we would still need to interpret the args as a reference known by the transport layer.
The rpmsg_device struct should contain only fields related to the RPMsg service.
We can also assume that the notion of "notify" is linked to Virtio. For instance, for a UART transport, we will probably speak about an end-of-transfer callback.
Another question we have to answer is whether this callback is related to RPMsg Virtio or to Virtio. I wonder if this callback should not be in the Virtio layer to be able to reuse it for other Virtio services.

@arnopo OK, move this callback to struct rpmsg_virtio_device is acceptable to me. About the second question, I think let virtio services handle the buffer question is better just like rpmsg virtio device does.

Ok let keep it here for the moment, if needed for some other virtio service, we will implement it at virtio level and make this one deprecated

@arnopo So you mean keep PR #518 and add new notify_wait() callback in struct rpmsg_virtio_device to support two ways to handle the get tx buffer failed problem, but default to use the notify_wait() callback in struct rpmsg_virtio_device way?

@arnopo So you mean keep PR #518 and add new notify_wait() callback in struct rpmsg_virtio_device to support two ways to handle the get tx buffer failed problem, but default to use the notify_wait() callback in struct rpmsg_virtio_device way?

No, you can revert #518. My point here is that it is acceptable to implement this in rpmsg_virtio for the time being.
However, in the future, if this callback is also required for some other virtio services, we will have to rework it to implement something common in the virtio layer.

to sum -up the remaining work here is

move this callback to struct rpmsg_virtio_device

management of the cb in rpmsg_virtio ( remove code in rpmsg.c)

…uffer get" This reverts commit 95802c1. Signed-off-by: Yongrong Wang <wangyongrong@xiaomi.com>

lib/rpmsg/rpmsg_virtio.c

lib/include/openamp/rpmsg.h

edmooring

Looks good to go.

lib/include/openamp/rpmsg.h

CV-Bowen · 2024-03-07T11:49:49Z

lib/rpmsg/rpmsg_virtio.c

+	if (rvdev->notify_wait_cb)
+		return rvdev->notify_wait_cb(&rvdev->rdev, vring_info->notifyid);
+
+	return RPMSG_ERR_NXIO;


RPMSG_EOPNOTSUPP

CV-Bowen

@arnopo Could you take a look?

CV-Bowen · 2024-03-11T01:51:11Z

lib/include/openamp/rpmsg.h

 #define RPMSG_ERR_ADDR			(RPMSG_ERROR_BASE - 7)
 #define RPMSG_ERR_PERM			(RPMSG_ERROR_BASE - 8)
-#define RPMSG_EOPNOTSUPP		(RPMSG_ERROR_BASE - 9)
+#define RPMSG_EOPNOTSUPP			(RPMSG_ERROR_BASE - 9)


arnopo

Few comment,
Please rework also the commit message of the second commit. It should reflect the changes.
you should be able to reuse, with few adaptation, the commit message of
commit 95802c1.

arnopo · 2024-03-11T09:01:57Z

lib/rpmsg/rpmsg_virtio.c

 }

+static int rpmsg_virtio_notify_wait(struct rpmsg_virtio_device *rvdev,
+				    struct virtqueue *vq)


squash declaration on 1 line (up to 100 char is OK)

lib/rpmsg/rpmsg_virtio.c

arnopo · 2024-03-11T09:05:09Z

lib/rpmsg/rpmsg_virtio.c

+		 * use metal_sleep_usec() method by default.
+		 */
+		status = rpmsg_virtio_notify_wait(rvdev, rvdev->rvq);
+		if (status != RPMSG_SUCCESS) {


if status is different from RPMSG_EOPNOTSUPP we should leave the loop

if (status == RPMSG_EOPNOTSUPP) { metal_sleep_usec(RPMSG_TICKS_PER_INTERVAL); tick_count--; } else if (status == RPMSG_SUCCESS) { break; }

CV-Bowen · 2024-03-12T06:40:10Z

lib/rpmsg/rpmsg_internal.h

 	return rpmsg_get_endpoint(rdev, NULL, addr, RPMSG_ADDR_ANY);
 }

+int rpmsg_notify_wait(struct rpmsg_device *rdev, uint32_t id);


remove no need

arnopo

LGTM after addressing @CV-Bowen comment

arnopo · 2024-03-12T09:59:28Z

@edmooring @tnmysh
Few updates since your last review. please, Could you do a new review?

Give users a chance to handle the no tx buffer situation when get tx buffer, with this patch, user can set the notify_wait_cb in driver and this callback function will be called to handle the wait when no tx buffer in tx virtqueue. Signed-off-by: Yongrong Wang <wangyongrong@xiaomi.com>

CV-Bowen

LGTM !

tnmysh

LGTM.

edmooring

Looks good to go.

arnopo reviewed Jan 30, 2024

View reviewed changes

wyr8899 force-pushed the notify_dev branch from c0154ee to 02140b8 Compare January 31, 2024 02:03

Revert "openamp: add new ops wait_notified() to avoid looping in tx b…

749d934

…uffer get" This reverts commit 95802c1. Signed-off-by: Yongrong Wang <wangyongrong@xiaomi.com>

wyr8899 force-pushed the notify_dev branch 2 times, most recently from 0b5c3bd to f4ddcf5 Compare January 31, 2024 02:32

CV-Bowen reviewed Jan 31, 2024

View reviewed changes

lib/rpmsg/rpmsg_virtio.c Show resolved Hide resolved

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

wyr8899 force-pushed the notify_dev branch 2 times, most recently from 5d13981 to 7fc6e43 Compare January 31, 2024 02:51

CV-Bowen approved these changes Jan 31, 2024

View reviewed changes

arnopo requested review from edmooring and tnmysh February 1, 2024 15:16

edmooring approved these changes Feb 27, 2024

View reviewed changes

tnmysh approved these changes Feb 27, 2024

View reviewed changes

wyr8899 force-pushed the notify_dev branch 5 times, most recently from 6ec1958 to 613fb79 Compare March 7, 2024 08:42

CV-Bowen reviewed Mar 7, 2024

View reviewed changes

wyr8899 force-pushed the notify_dev branch from 613fb79 to 3784214 Compare March 8, 2024 09:31

CV-Bowen reviewed Mar 11, 2024

View reviewed changes

arnopo requested changes Mar 11, 2024

View reviewed changes

wyr8899 force-pushed the notify_dev branch from 3784214 to 6b33578 Compare March 12, 2024 03:09

CV-Bowen reviewed Mar 12, 2024

View reviewed changes

arnopo approved these changes Mar 12, 2024

View reviewed changes

arnopo requested review from edmooring and tnmysh March 12, 2024 09:56

wyr8899 force-pushed the notify_dev branch from 6b33578 to 8043658 Compare March 13, 2024 02:00

CV-Bowen approved these changes Mar 13, 2024

View reviewed changes

tnmysh approved these changes Mar 15, 2024

View reviewed changes

edmooring approved these changes Mar 16, 2024

View reviewed changes

arnopo merged commit 15b4a8b into OpenAMP:main Mar 18, 2024

arnopo added this to the Release V2024.04 milestone Mar 18, 2024

arnopo mentioned this pull request Apr 2, 2024

Fix #549 #579

Merged

Conversation

wyr8899 commented Jan 30, 2024

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!

CV-Bowen Feb 2, 2024 • 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!

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!

arnopo Feb 28, 2024 • 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

CV-Bowen left a comment

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!

arnopo left a comment

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!

Choose a reason for hiding this comment

Uh oh!

arnopo left a comment

Choose a reason for hiding this comment

Uh oh!

arnopo commented Mar 12, 2024

Uh oh!

CV-Bowen left a comment

Choose a reason for hiding this comment

Uh oh!

tnmysh 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

CV-Bowen Feb 2, 2024 •

edited

Loading

arnopo Feb 28, 2024 •

edited

Loading