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

Adjustable rpmsg size #195

Merged
merged 1 commit into from
Apr 23, 2020
Merged

Conversation

supergaute
Copy link
Contributor

Add option to adjust RPMSG_BUFFER_SIZE during CMake build

@supergaute supergaute force-pushed the adjustable_rpmsg_size branch 2 times, most recently from f897b8e to dff2413 Compare February 17, 2020 19:47
@arnopo arnopo self-requested a review February 18, 2020 13:50
Signed-off-by: Gaute Nilsson <gaute.nilsson@siemens.com>
@supergaute supergaute force-pushed the adjustable_rpmsg_size branch from dff2413 to 5449c5d Compare February 18, 2020 17:49
@supergaute
Copy link
Contributor Author

Can you take a look at this? Having this change in the upstream repo would be very nice for our project.

@xiaoxiang781216
Copy link
Collaborator

Can you take a look at this? Having this change in the upstream repo would be very nice for our project.

The RPMSG_BUFFER_SIZE need align with kernel definiton, both side(OpenAMP and Linux kernel) need negotiate the new buffer size), you can see the patch here:
#155

@arnopo arnopo assigned edmooring and unassigned edmooring Mar 9, 2020
@arnopo arnopo requested a review from edmooring March 9, 2020 16:04
@arnopo
Copy link
Collaborator

arnopo commented Mar 9, 2020

Can you take a look at this? Having this change in the upstream repo would be very nice for our project.

The RPMSG_BUFFER_SIZE need align with kernel definiton, both side(OpenAMP and Linux kernel) need negotiate the new buffer size), you can see the patch here:
#155

There are also discussions around resource table evolution to address the point.
Waiting conclusion this patch makes sense for MPOV, for solution based on OpenAMP on both processors.

Copy link
Contributor

@edmooring edmooring left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs further discussion on the conference call or the mailing list.

@@ -74,5 +74,9 @@ endif (WITH_ZEPHYR)

option (WITH_LIBMETAL_FIND "Check Libmetal library can be found" ON)

if (DEFINED RPMSG_BUFFER_SIZE)
add_definitions( -DRPMSG_BUFFER_SIZE=${RPMSG_BUFFER_SIZE} )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs a documentation update somewhere to tell the user that they can change the default, and that changing the default will make it incompatible with the Linux kernel implementation. I think that this change or #155 is major enough to bring up in the next openamp-rp call, or on the mailing list.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @edmooring,
Agree on the principle for the Documentation. We have to discuss the way to provide a documentation accessible to contributors.
Concerning this commit itself, it is not a major change. The RPMSG_BUFFER_SIZE can already be customized (https://github.com/OpenAMP/open-amp/blob/master/lib/include/openamp/rpmsg_virtio.h#L25)
So this patch only exports the definition, to be able to define the size in the make command line instead of updating the makefiles.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @arnopo
For now, putting it in README.md, near the instructions for building OpenAMP for the various platforms would be fine with me.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense, as some other cmake options have also to be defined (e.g WITH_VIRTIO_SLAVE, WITH_VIRTIO_MASTER ) , i would propose to have a dedicated thread for this.

@arnopo arnopo mentioned this pull request Apr 21, 2020
@arnopo
Copy link
Collaborator

arnopo commented Apr 23, 2020

Use and restriction of the RPMSG_BUFFER_SIZE option will de documented in
#202

@arnopo arnopo merged commit 2141a3c into OpenAMP:master Apr 23, 2020
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.

4 participants