OpenAMP / open-amp

The main OpenAMP library implementing RPMSG, Virtio, and Remoteproc for RTOS etc
https://www.openampproject.org/
Other
706 stars 288 forks source link

remoteproc_virtio: optimize the remoteproc virtio transport layer #489

Closed CV-Bowen closed 7 months ago

CV-Bowen commented 1 year ago

remoteproc_virtio: optimize the remoteproc virtio transport layer

  1. Implement the rproc_virtio_create_virtqueues() and rproc_virtio_delete_virtqueues()
  2. Becase item 1, modified the rpmsg_init_vdev_with_config() also
arnopo commented 1 year ago

hello @CV-Bowen ,

Could you details what was the issue you are facing that needs to move the virtio_create_virtqueues to the transport layer? that would help to understand your work

CV-Bowen commented 1 year ago

@arnopo Hi, we want to support different transport layer (remoteproc, mmio) for virtio drivers. After this PR, we can implement the mmio tranport layer base on OpenAmp. I think this patch is also helpful for virtio-mmio framework in openamp/virtio-exp branch to support remoteproc transport layer.

arnopo commented 1 year ago

@arnopo Hi, we want to support different transport layer (remoteproc, mmio) for virtio drivers. After this PR, we can implement the mmio tranport layer base on OpenAmp. I think this patch is also helpful for virtio-mmio framework in openamp/virtio-exp branch to support remoteproc transport layer.

I understand the objective., what is not clear to me as a first review is why you need to move virtio_create_virtqueues this should be independent from the transport layer, no?

CV-Bowen commented 1 year ago

@arnopo Hi, we want to support different transport layer (remoteproc, mmio) for virtio drivers. After this PR, we can implement the mmio tranport layer base on OpenAmp. I think this patch is also helpful for virtio-mmio framework in openamp/virtio-exp branch to support remoteproc transport layer.

I understand the objective., what is not clear to me as a first review is why you need to move virtio_create_virtqueues this should be independent from the transport layer, no?

I found only rpmsg_virtio_create_virtqueues call virtio_create_virtqueues in OpenAmp, so I delete virtio_create_virtqueues after moving it's implementation to rproc_virtio_create_virtqueues. I do not consider that some other codes use it, I will restore this api and call it directly in rproc_virtio_create_virtqueues().

CV-Bowen commented 1 year ago

@arnopo @edmooring Commit 1: virtio: follow virtio 1.2 spec, add more virtio status and device id Seems commit 1 can be an independent PR. I will upload it later, and this PR will be only for Commit 2.

arnopo commented 1 year ago

@CV-Bowen seems that you remove the Commit 2: virtio: decoupling the transport layer and virtio device layer

Please notice also https://github.com/OpenAMP/open-amp/pull/494, we should find a common solution...

CV-Bowen commented 1 year ago

@arnopo Sorry, I has fixed this and I will look #494.

xiaoxiang781216 commented 1 year ago

@CV-Bowen seems that you remove the Commit 2: virtio: decoupling the transport layer and virtio device layer

Please notice also #494, we should find a common solution...

@CV-Bowen 's change extract and generalize from #494, after this change get merged:

  1. The new mmio transport addition don't need modify rest code in OpenAMP
  2. All code manipulate virtio device can work with both transport(mmio v.s. remoteproc)
arnopo commented 1 year ago

@CV-Bowen : Please, could you push your work on lib/include/openamp/virtio.h in a separate pull request as requested in (https://github.com/OpenAMP/open-amp/issues/390#issuecomment-1602605560) That well help to move forward on the topic? Thanks in advance.

CV-Bowen commented 1 year ago

@arnopo Sorry, i'm busy with other things, and the new PR has been created ( https://github.com/OpenAMP/open-amp/pull/495). But a mistake is this PR is also updated, i will restore this PR.

arnopo commented 1 year ago

@CV-Bowen Zephyr Build fails because the sample application has defined same functions(https://elixir.bootlin.com/zephyr/latest/source/samples/subsys/ipc/openamp/src/main.c#L66). Just ignore the issue this has to be fixed in Zephyr ( I will send a PR)

arnopo commented 1 year ago

Thank you for your prompt update!

LGTM after addressing the 2 comments and fix "deivce" typo in commit

Just see that you create the https://github.com/OpenAMP/open-amp/pull/495 to create the dispatch functions. So please take into account my remarks to apply them on https://github.com/OpenAMP/open-amp/pull/495.

CV-Bowen commented 1 year ago

@arnopo Done in #495

CV-Bowen commented 11 months ago

I will continue this PR to implement the remoteproc transport ops: create_virtqueue and delete_virtqueues.

CV-Bowen commented 11 months ago

All the comments should have been addressed. Could you take a look again? @arnopo

tnmysh commented 11 months ago

@arnopo, @edmooring If you are okay, we can merge the change.

arnopo commented 11 months ago

@CV-Bowen, Please, could you rebase it to solve merge conflict, that i test it and merge it

CV-Bowen commented 11 months ago

@arnopo Rebased.

arnopo commented 10 months ago

tests Ok on my side @edmooring , @tnmysh , could you make a short test on Xilinx platform for complementary test on remoteproc ?

github-actions[bot] commented 9 months ago

This pull request has been marked as a stale pull request because it has been open (more than) 45 days with no activity.

CV-Bowen commented 7 months ago

@arnopo Rebase to the last to fix the conflict. Could we merge this PR? There isn’t much time until release in April.

arnopo commented 7 months ago

Sorry @CV-Bowen, it go out of my radar,

@edmooring , @tnmysh , as requested could you confirm that it does not create regression on your platforms