search

OpenAMP / open-amp

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

Updates and clean-up cache management #502

Closed iuliana-prodan closed 9 months ago

iuliana-prodan commented 11 months ago

Please, check each commit.

cc: @arnopo @carlocaione @TanmayShah-xilinx

iuliana-prodan commented 11 months ago

Instead of having different macros with different names doing the same thing can we have one single set of macros for flushing and invalidating?

I totally agree, but TBH I don't know where to add them - in which header so it can be accessed by all?

carlocaione commented 11 months ago

I totally agree, but TBH I don't know where to add them - in which header so it can be accessed by all?

IMO if you cannot find a proper header for that just add a new one.

iuliana-prodan commented 11 months ago

I totally agree, but TBH I don't know where to add them - in which header so it can be accessed by all?

IMO if you cannot find a proper header for that just add a new one.

ok, I'll look again. Thanks!

iuliana-prodan commented 11 months ago

I totally agree, but TBH I don't know where to add them - in which header so it can be accessed by all?

IMO if you cannot find a proper header for that just add a new one.

I've add a new commit to fix this: https://github.com/OpenAMP/open-amp/pull/502/commits/731e394018cdf866fe66a62408792b481eb5db55

iuliana-prodan commented 11 months ago

I'm in general ok with this change but we must discuss whether we actually want to have a general VIRTIO_USE_CACHE switch or we want to have a more fine grained control over buffers placement.

IMO we should keep all 3, but see here: https://github.com/OpenAMP/open-amp/pull/502#discussion_r1269709610 Also, in Zephyr we enable all 3 together with CONFIG_OPENAMP_WITH_DCACHE (https://github.com/zephyrproject-rtos/open-amp/blob/master/CMakeLists.txt#L20), not 1 by 1, so adding 1 for all seems reasonable.

arnopo commented 10 months ago

I'm in general ok with this change but we must discuss whether we actually want to have a general VIRTIO_USE_CACHE switch or we want to have a more fine grained control over buffers placement.

IMO we should keep all 3, but see here: #502 (comment) Also, in Zephyr we enable all 3 together with CONFIG_OPENAMP_WITH_DCACHE (https://github.com/zephyrproject-rtos/open-amp/blob/master/CMakeLists.txt#L20), not 1 by 1, so adding 1 for all seems reasonable.

@carlocaione As mentioned in #502 (comment) my proposal is:

One strategy would consists in making VIRTIO_CACHED_VRINGS, DVIRTIO_CACHED_BUFFERS and VIRTIO_CACHED_RSC_TABLE deprecated. If nobody complains during 2 years we will clean them.

@xiaoxiang781216 : Do you use cache management? If so, do you require fine control, or is one generic compilation configuration sufficent for your needs?

arnopo commented 10 months ago

@carlocaione @xiaoxiang781216 any comment/feedback on this PR?

carlocaione commented 10 months ago

@arnopo instead of deprecating WITH_DCACHE_VRINGS and WITH_DCACHE_BUFFERS why not simply leave those in place without deprecation and making WITH_DCACHE a more general switch implying the other two?

At least people could decide the lever of granularity to use.

arnopo commented 10 months ago

@arnopo instead of deprecating WITH_DCACHE_VRINGS and WITH_DCACHE_BUFFERS why not simply leave those in place without deprecation and making WITH_DCACHE a more general switch implying the other two?

At least people could decide the lever of granularity to use.

If possible, I would avoid multiplying the configs that complexify the support and increase the risk of bugs. Therefore, if no rational to keep them they should be removed.

tnmysh commented 9 months ago

LGTM

arnopo commented 9 months ago

@iuliana-prodan There is a merge conflict, could you rebase your patch on top of main branch please?

iuliana-prodan commented 9 months ago

@iuliana-prodan There is a merge conflict, could you rebase your patch on top of main branch please?

Sure

iuliana-prodan commented 9 months ago

Did the rebase and push the changes. I see a build failure:

-- ZEPHYR_TOOLCHAIN_VARIANT not set, trying to locate Zephyr SDK
CMake Error at /github/workspace/zephyrproject/zephyr/cmake/modules/FindZephyr-sdk.cmake:109 (find_package):
  Could not find a configuration file for package "Zephyr-sdk" that is
  compatible with requested version "0.16".
-- Configuring incomplete, errors occurred!

  The following configuration files were considered but not accepted:

    /github/workspace/zephyr-sdk-0.15.0/cmake/Zephyr-sdkConfig.cmake, version: 0.15.0

It seems to me that is required zephyr-sdk 0.16 and the continuous integrations uses 0.15.

@arnopo is it possible to update the zephyr-sdk? Can I do that? How? :)

arnopo commented 9 months ago

Did the rebase and push the changes. I see a build failure:

-- ZEPHYR_TOOLCHAIN_VARIANT not set, trying to locate Zephyr SDK
CMake Error at /github/workspace/zephyrproject/zephyr/cmake/modules/FindZephyr-sdk.cmake:109 (find_package):
  Could not find a configuration file for package "Zephyr-sdk" that is
  compatible with requested version "0.16".
-- Configuring incomplete, errors occurred!

  The following configuration files were considered but not accepted:

    /github/workspace/zephyr-sdk-0.15.0/cmake/Zephyr-sdkConfig.cmake, version: 0.15.0

It seems to me that is required zephyr-sdk 0.16 and the continuous integrations uses 0.15.

@arnopo is it possible to update the zephyr-sdk? Can I do that? How? :)

The dsk version in the Ci need to be uopdated , I will do it

arnopo commented 9 months ago

Ci fixed in https://github.com/OpenAMP/open-amp/pull/504 Please, could you rebase another time ( sorry for the double work)

iuliana-prodan commented 9 months ago

Ci fixed in #504 Please, could you rebase another time ( sorry for the double work)

Done. All good now! :)

iuliana-prodan commented 9 months ago

I don't have any real problem with this. @iuliana-prodan please update https://github.com/zephyrproject-rtos/open-amp/blob/master/CMakeLists.txt and the zephyr open-amp repo when this is merged.

@carlocaione Sorry for not doing this earlier, but... here it is: https://github.com/zephyrproject-rtos/open-amp/pull/17 I cannot add any reviewers or anything.