-
Notifications
You must be signed in to change notification settings - Fork 294
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
Conversation
f897b8e
to
dff2413
Compare
Signed-off-by: Gaute Nilsson <gaute.nilsson@siemens.com>
dff2413
to
5449c5d
Compare
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: |
There are also discussions around resource table evolution to address the point. |
There was a problem hiding this 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} ) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Use and restriction of the RPMSG_BUFFER_SIZE option will de documented in |
Add option to adjust RPMSG_BUFFER_SIZE during CMake build