Autumn OpenAMP Release

99 views
Skip to first unread message

Ed Mooring

unread,
Oct 10, 2019, 4:52:13 PM10/10/19
to open...@googlegroups.com
Hi all,

I would like to schedule an OpenAMP release for this fall. Are there any
outstanding pull requests or issues that people think should be merged
before release?

Regards,

Ed M

arnaud.p...@linaro.org

unread,
Oct 11, 2019, 1:06:24 PM10/11/19
to open-amp
Hi Ed,

I've gone through all PRs on going, here is my view:
=> the first and second commits are needed as fix compilation warning and an issue.
=> for the rest can be posponed after released

=> approved by Wendy,  n my view risk to integrate it is low, but i do not use remoteproc master part ...

- remoteproc_mmap support va to pa/da conversion
I'm not able to figure out if this PR fix an issue or is a workaround. need to crosscheck if another approach exist that does not break the API

Not mandatory

 Approved by Wendy and myself, could be merged. Perhaps just missing associated application for validation  purpose

should not be part of the next released. Probably a candidate for OpenAMP working group...as need alignment with Linux implementation.

- Negotiate individual buffer size dynamically 
same remark

- rpmsg: fix virtio set_status usage of vdev status for slave
Do not merge: need probably to be reworked... 


Concerning issues, i guess the mains one is : https://github.com/OpenAMP/open-amp/issues/176
the minimum is probably  to give a restriction in the release note.

Regards
Arnaud

Ed Mooring

unread,
Oct 11, 2019, 4:23:41 PM10/11/19
to open...@googlegroups.com

Arnaud,

Thank you very much for the reviews. My responses are inline.

On 10/11/19 10:06 AM, arnaud.p...@linaro.org wrote:
Hi Ed,

I've gone through all PRs on going, here is my view:
=> the first and second commits are needed as fix compilation warning and an issue.
=> for the rest can be posponed after released
I think these are valuable and should be included. My only concern is adding a comment to your strlen() workaround so nobody "optimizes" it by removing the code.


=> approved by Wendy,  n my view risk to integrate it is low, but i do not use remoteproc master part ...
The problem with this change is that there is no documentation saying why it is needed or what it is intended to do.

I'm not able to figure out if this PR fix an issue or is a workaround. need to crosscheck if another approach exist that does not break the API
I agree. I don't want this change in the release.
Agreed. I would like more documentation and feedback from the Linux kernel maintainer for rpmsg.


 Approved by Wendy and myself, could be merged. Perhaps just missing associated application for validation  purpose
I don't think this should go in the release. The lack of an application to exercise the code is an issue.

should not be part of the next released. Probably a candidate for OpenAMP working group...as need alignment with Linux implementation.
Agreed.
Agreed.


- rpmsg: fix virtio set_status usage of vdev status for slave
Do not merge: need probably to be reworked...
Agreed,

 


Concerning issues, i guess the mains one is : https://github.com/OpenAMP/open-amp/issues/176
the minimum is probably  to give a restriction in the release note.
Could you give me some text for the release notes about this? I'm not sure I understand the issue.


Regards
Arnaud

On Thursday, 10 October 2019 22:52:13 UTC+2, Ed Mooring wrote:
Hi all,

I would like to schedule an OpenAMP release for this fall. Are there any
outstanding pull requests or issues that people think should be merged
before release?

Regards,

Ed M

--
You received this message because you are subscribed to the Google Groups "open-amp" group.
To unsubscribe from this group and stop receiving emails from it, send an email to open-amp+u...@googlegroups.com.
To view this discussion on the web, visit https://groups.google.com/d/msgid/open-amp/32527359-9598-45bb-ac8e-2a4d0c67a4f4%40googlegroups.com.

Arnaud Pouliquen

unread,
Oct 14, 2019, 5:02:11 AM10/14/19
to open...@googlegroups.com
Hi Ed,


On Fri, 11 Oct 2019 at 22:23, Ed Mooring <ed.mo...@linaro.org> wrote:

Arnaud,

Thank you very much for the reviews. My responses are inline.

On 10/11/19 10:06 AM, arnaud.p...@linaro.org wrote:
Hi Ed,

I've gone through all PRs on going, here is my view:
=> the first and second commits are needed as fix compilation warning and an issue.
=> for the rest can be posponed after released
I think these are valuable and should be included. My only concern is adding a comment to your strlen() workaround so nobody "optimizes" it by removing the code.
Ok a will add a comment. 

=> approved by Wendy,  n my view risk to integrate it is low, but i do not use remoteproc master part ...
The problem with this change is that there is no documentation saying why it is needed or what it is intended to do.
I'm not able to figure out if this PR fix an issue or is a workaround. need to crosscheck if another approach exist that does not break the API
I agree. I don't want this change in the release.
Agreed. I would like more documentation and feedback from the Linux kernel maintainer for rpmsg.

 Approved by Wendy and myself, could be merged. Perhaps just missing associated application for validation  purpose
I don't think this should go in the release. The lack of an application to exercise the code is an issue.
should not be part of the next released. Probably a candidate for OpenAMP working group...as need alignment with Linux implementation.
Agreed.
Agreed.

- rpmsg: fix virtio set_status usage of vdev status for slave
Do not merge: need probably to be reworked...
Agreed,
 


Concerning issues, i guess the mains one is : https://github.com/OpenAMP/open-amp/issues/176
the minimum is probably  to give a restriction in the release note.
Could you give me some text for the release notes about this? I'm not sure I understand the issue.
I will do few tests to figure out the behaviour, then i will propose either a fix or a release note.

Regards
Arnaud

Regards
Arnaud

On Thursday, 10 October 2019 22:52:13 UTC+2, Ed Mooring wrote:
Hi all,

I would like to schedule an OpenAMP release for this fall. Are there any
outstanding pull requests or issues that people think should be merged
before release?

Regards,

Ed M

--
You received this message because you are subscribed to the Google Groups "open-amp" group.
To unsubscribe from this group and stop receiving emails from it, send an email to open-amp+u...@googlegroups.com.
To view this discussion on the web, visit https://groups.google.com/d/msgid/open-amp/32527359-9598-45bb-ac8e-2a4d0c67a4f4%40googlegroups.com.

--
You received this message because you are subscribed to a topic in the Google Groups "open-amp" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/open-amp/VEWXUbNF1No/unsubscribe.
To unsubscribe from this group and all its topics, send an email to open-amp+u...@googlegroups.com.
To view this discussion on the web, visit https://groups.google.com/d/msgid/open-amp/9dc44a05-c233-6a43-7eb5-86ab67562942%40linaro.org.

Arnaud Pouliquen

unread,
Oct 17, 2019, 11:45:18 AM10/17/19
to open...@googlegroups.com
Hi Ed,

I performed more tests to understand the VIRTIO_RING_F_EVENT_IDX issue.
My first analysis was wrong, i made a confusion between the defines in LInux and in OPENAMP.
So  VIRTIO_RING_F_EVENT_IDX value is ok.
But there is still an issue if VIRTIO_RING_F_EVENT_IDX is declared. In my test the Linux send a kick 
to the coprocessor only on first message and  then there is no more kick sent to the remote core.
This lead to an RPMsg buffer fifo full as my slave is in Idle routine waiting for an interrupt to check for new messages.

Concerning the message, here is a proposal: 

Known Issue:
Current version of the virtio implementation does not respect the Virtqueue Interrupt Suppression principle,
We recommend to not use this feature in OpenAMP v2019.10.
Related issue is described here: https://github.com/OpenAMP/open-amp/issues/176

 I let you rewrite it with a better english wording, if needed :)  

Regards
arnaud


On Fri, 11 Oct 2019 at 22:23, Ed Mooring <ed.mo...@linaro.org> wrote:
You received this message because you are subscribed to a topic in the Google Groups "open-amp" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/open-amp/VEWXUbNF1No/unsubscribe.
To unsubscribe from this group and all its topics, send an email to open-amp+u...@googlegroups.com.
To view this discussion on the web, visit https://groups.google.com/d/msgid/open-amp/9dc44a05-c233-6a43-7eb5-86ab67562942%40linaro.org.

Xiang Xiao

unread,
Nov 12, 2019, 2:10:42 PM11/12/19
to open-amp

Since many patches are provided by us, I need make more explanation here, please see my inline comment.

Let's make a summary:

1.We use OpenAMP in the real product more than two years and with the very complex use case:

   a.RTOS<->Linux and RTOS<->RTOS

   b.With or without firmware loading

   c.One OpenAMP instance talk with two instances

   Many patch come from our real experience, so please don't ignore the patch casually.

2.All our work upstream to NuttX which is very popular in IoT and drone marketing:

   https://github.com/Samsung/TizenRT/wiki

   https://github.com/PX4/Firmware

   Many NuttX user is very interesting to enable OpenAMP on their hardware, so please make their integration work easier.


Thanks
Xiang

On Monday, October 14, 2019 at 5:02:11 PM UTC+8, Arnaud Pouliquen wrote:
Hi Ed,


On Fri, 11 Oct 2019 at 22:23, Ed Mooring <ed.m...@linaro.org> wrote:

Arnaud,

Thank you very much for the reviews. My responses are inline.

On 10/11/19 10:06 AM, arnaud....@linaro.org wrote:
Hi Ed,

I've gone through all PRs on going, here is my view:
=> the first and second commits are needed as fix compilation warning and an issue.
=> for the rest can be posponed after released
I think these are valuable and should be included. My only concern is adding a comment to your strlen() workaround so nobody "optimizes" it by removing the code.
Ok a will add a comment. 

=> approved by Wendy,  n my view risk to integrate it is low, but i do not use remoteproc master part ...
The problem with this change is that there is no documentation saying why it is needed or what it is intended to do.
 
This patch finish the half way work and make it work as virito spec, what is important is that other pull request is base on this patch:
So, please merge this request.
 
I'm not able to figure out if this PR fix an issue or is a workaround. need to crosscheck if another approach exist that does not break the API
I agree. I don't want this change in the release.

 
The original API has a design flaw: pa/da can convert to va, but va can't convert to pa/da. Sometime va->pa/da conversion is very important for a specific scenario.
This patch make va/pa/da could convert to each other without any limitation, also ensure the virtual address space is a first class just like physic or device address space.
Yes, this patch change the prototype of remoteproc_ops::ops and remoteproc_mmap, but I think it's a good change because:
1.The root issue is the interface design of mmap, the direct fix is to change it's prototype, all other fix is just a workaround.
2.The interface change is very small, any caller and implementation could be very easy to adapter the exist code to confirm the new interface
3.The compatibility issue is much smaller than rpmsg API because mmap is the interface between remoteproc and porting layer, not for applicaiton developer.
4.Last, each OpenAMP official release always introduce the incompatible change even for 3rd party public API. Why we change the strategy here?

Not mandatory
Agreed. I would like more documentation and feedback from the Linux kernel maintainer for rpmsg.


First, this feature doesn't depend on Linux kernel I think.
Second, this patch could detect the initialization sequence timing issue which is very useful when OpenAMP bringup on new hardware.
Third, we have another patch to check the wrong rpmsg API usage depends on this patch.
So please merge this patch, then we can send the new pull request.
We shoudn't refuse any change which could improve the correct API usage, right?
Actually, we made this change because we spend a long time to debug this issue. We don't want other people sufer the sm pain again.
 
 Approved by Wendy and myself, could be merged. Perhaps just missing associated application for validation  purpose
I don't think this should go in the release. The lack of an application to exercise the code is an issue.

This isn't a new feature, zero copy API release offically from tag v2016.10, but remove from tag v2018.04, which make our many rpmsg code fail to build.
Actually, our code is part of NuttX(a very popular RTOS), here is the list which depend on zero copy API:
And many people is very interest to enable OpenAMP on their device, so please include it in the release to keep the compatiblity, especially for public rpmsg API.

should not be part of the next released. Probably a candidate for OpenAMP working group...as need alignment with Linux implementation.
Agreed.


Yes, the above two patches need collaborate with Linux implementation to ensure the compatiblity.
 
- rpmsg: fix virtio set_status usage of vdev status for slave
Do not merge: need probably to be reworked...
Agreed,
 


Concerning issues, i guess the mains one is : https://github.com/OpenAMP/open-amp/issues/176
the minimum is probably  to give a restriction in the release note.
Could you give me some text for the release notes about this? I'm not sure I understand the issue.
I will do few tests to figure out the behaviour, then i will propose either a fix or a release note.

Regards
Arnaud

Regards
Arnaud

On Thursday, 10 October 2019 22:52:13 UTC+2, Ed Mooring wrote:
Hi all,

I would like to schedule an OpenAMP release for this fall. Are there any
outstanding pull requests or issues that people think should be merged
before release?

Regards,

Ed M

--
You received this message because you are subscribed to the Google Groups "open-amp" group.
To unsubscribe from this group and stop receiving emails from it, send an email to open...@googlegroups.com.

--
You received this message because you are subscribed to a topic in the Google Groups "open-amp" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/open-amp/VEWXUbNF1No/unsubscribe.
To unsubscribe from this group and all its topics, send an email to open...@googlegroups.com.
Reply all
Reply to author
Forward
0 new messages