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.

Sign up for GitHub

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

rpmsg: Make RPMSG_BUFFER_SIZE a fixed define #319

Closed
wants to merge 1 commit into from
Closed

rpmsg: Make RPMSG_BUFFER_SIZE a fixed define #319

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions cmake/options.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,9 +101,7 @@ 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} )
endif (DEFINED RPMSG_BUFFER_SIZE)
set (RPMSG_BUFFER_SIZE 512 CACHE STRING "Adjust the size of the RPMsg buffers")

message ("-- C_FLAGS : ${CMAKE_C_FLAGS}")
# vim: expandtab:ts=2:sw=2:smartindent
7 changes: 5 additions & 2 deletions lib/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ set_property (GLOBAL PROPERTY "PROJECT_LIB_EXTRA_CFLAGS")

collector_create (PROJECT_LIB_SOURCES "${CMAKE_CURRENT_SOURCE_DIR}")
collect (PROJECT_LIB_DIRS "${CMAKE_CURRENT_BINARY_DIR}")
collect (PROJECT_INC_DIRS "${PROJECT_BINARY_DIR}/include/generated")
collect (PROJECT_INC_DIRS "${CMAKE_CURRENT_SOURCE_DIR}/include")
collect (PROJECT_LIB_SOURCES version.c)

Expand All @@ -18,8 +19,9 @@ endif (WITH_PROXY)

set (OPENAMP_LIB open_amp)

configure_file(include/openamp/rpmsg_virtio.h
${PROJECT_BINARY_DIR}/include/generated/openamp/rpmsg_virtio.h @ONLY)
configure_file(version.h.in ${PROJECT_BINARY_DIR}/include/generated/openamp/version_def.h)
collect (PROJECT_INC_DIRS " ${PROJECT_BINARY_DIR}/include/generated/openamp")

if (NOT CMAKE_INSTALL_LIBDIR)
set (CMAKE_INSTALL_LIBDIR "lib")
Expand Down Expand Up @@ -65,7 +67,8 @@ else (WITH_ZEPHYR)
endif (WITH_STATIC_LIB)
endif (WITH_ZEPHYR)

install (DIRECTORY "${CMAKE_CURRENT_SOURCE_DIR}/include/openamp" DESTINATION include)
install (DIRECTORY "${CMAKE_CURRENT_SOURCE_DIR}/include/openamp" DESTINATION include
PATTERN "rpmsg_virtio.h" EXCLUDE)
install (DIRECTORY "${PROJECT_BINARY_DIR}/include/generated/openamp" DESTINATION include)

# vim: expandtab:ts=2:sw=2:smartindent
4 changes: 1 addition & 3 deletions lib/include/openamp/rpmsg_virtio.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,7 @@ extern "C" {
#endif

/* Configurable parameters */
#ifndef RPMSG_BUFFER_SIZE
#define RPMSG_BUFFER_SIZE (512)
#endif
#define RPMSG_BUFFER_SIZE @RPMSG_BUFFER_SIZE@
Copy link
Collaborator

Choose a reason for hiding this comment

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

Today the open-amp can be used without the cmakeand a library build, but with files directly build in the application project.
This break the legacy , imposing the cmake usage (only optional version_def.h is handled by cmake).

And this would not guarantee that include files would be aligned with a dynamic library.

On the other side if the application projetc define the RPMSG_BUFFER_SIZE and open-amp library build as a sub project could this solve the issue?

I also wonder if a new APi to get the rpmsg _size such as the introduce in Linux kernel https://elixir.bootlin.com/linux/v5.16-rc2/source/include/linux/rpmsg.h#L189) would be more reliable.
The drawback would be for static allocation.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think making things more dependent on cmake is a possible problem. Also, how many people ever build with RPMSG_BUFFER_SIZE anything but 512?

Copy link
Contributor Author

@supergaute supergaute Nov 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Well, I can only speak for myself, but I use OpenAMP with FreeRTOS, build with CMake and RPMSG_BUFFER_SIZE set to 2048.
What is so special about using RPMSG_BUFFER_SIZE to anything other than 512?

Copy link
Collaborator

Choose a reason for hiding this comment

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

From MPOV, it is also a valid use case to decrease the buffer size for MCU to MCU communications, to limit the shared memory.

I think we also have to think about evolution such as #155 pull request is also a possible evolution that would make the size negociated.
That's why a new API could be nice to get the buffer effective size.
But as mentionned before this is not compatible with a static allocation. that said, that could allow to check the alignement between the library and the application...

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, I can only speak for myself, but I use OpenAMP with FreeRTOS, build with CMake and RPMSG_BUFFER_SIZE set to 2048.
What is so special about using RPMSG_BUFFER_SIZE to anything other than 512?

The only thing special about 512 is that Linux remoteproc is hard-coded to 512. I hadn't thought of the MCU->MCU case or other such configurations.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@supergaute

Could you confirm that #328 answer to your need and that we can close this one?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Without any answer, I consider that the #328 answer to your need, please reopen this pull request if it is not the case


/* The feature bitmap for virtio rpmsg */
#define VIRTIO_RPMSG_F_NS 0 /* RP supports name service notifications */
Expand Down