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

examples: use atomic_flag type, __sync functions #221

Merged
merged 1 commit into from
Dec 13, 2022

Conversation

kernelchuk
Copy link
Contributor

Use atomic_flag type and the GCC __sync built-in
functions for atomic memory access.

Signed-off-by: Sergei Korneichuk sergei.korneichuk@amd.com

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.

Update copyrights?

@kernelchuk
Copy link
Contributor Author

Yes. Thank you. I will update the copyrights. Also, I found the same issues on the FreeRTOS side. The update is coming after internal testing and review.

@kernelchuk
Copy link
Contributor Author

Hi Ed, Could you, please, take another look at this PR? Thank you.

@edmooring
Copy link
Contributor

When I try to build this using gcc 10.3.1 from ARM, I get the following:

cd /OpenAMP-Project/gh3/libmetal/build_r5/examples/system/generic/zynqmp_r5/zynqmp_amp_demo && /opt/arm/gcc-arm-none-eabi-10.3-2021.07/bin/arm-none-eabi-gcc -I/OpenAMP-Project/gh3/libmetal/build_r5/lib/include -I/OpenAMP-Project/gh3/libmetal/examples/system/generic/zynqmp_r5/zynqmp_amp_demo -mfloat-abi=hard -mcpu=cortex-r5 -mfpu=vfpv3-d16 -Wall -Werror -Wextra -flto -Os -I/home/mooring/build-oa/psu_cortexr5_0/include -o CMakeFiles/libmetal_amp_demod.elf.dir/shmem_atomic_demod.c.obj -c /OpenAMP-Project/gh3/libmetal/examples/system/generic/zynqmp_r5/zynqmp_amp_demo/shmem_atomic_demod.c
In file included from /OpenAMP-Project/gh3/libmetal/build_r5/lib/include/metal/atomic.h:100,
from /OpenAMP-Project/gh3/libmetal/build_r5/lib/include/metal/io.h:21,
from /OpenAMP-Project/gh3/libmetal/build_r5/lib/include/metal/shmem.h:15,
from /OpenAMP-Project/gh3/libmetal/examples/system/generic/zynqmp_r5/zynqmp_amp_demo/shmem_atomic_demod.c:19:
/OpenAMP-Project/gh3/libmetal/examples/system/generic/zynqmp_r5/zynqmp_amp_demo/shmem_atomic_demod.c: In function 'atomic_shmem_demod':
/OpenAMP-Project/gh3/libmetal/examples/system/generic/zynqmp_r5/zynqmp_amp_demo/shmem_atomic_demod.c:136:19: error: expected expression before '{' token
136 | remote_nkicked = ATOMIC_FLAG_INIT;
| ^~~~~~~~~~~~~~~~
make[2]: *** [examples/system/generic/zynqmp_r5/zynqmp_amp_demo/CMakeFiles/libmetal_amp_demod.elf.dir/build.make:102: examples/system/generic/zynqmp_r5/zynqmp_amp_demo/CMakeFiles/libmetal_amp_demod.elf.dir/shmem_atomic_demod.c.obj] Error 1
make[2]: Leaving directory '/OpenAMP-Project/gh3/libmetal/build_r5'
make[1]: *** [CMakeFiles/Makefile2:589: examples/system/generic/zynqmp_r5/zynqmp_amp_demo/CMakeFiles/libmetal_amp_demod.elf.dir/all] Error 2
make[1]: Leaving directory '/OpenAMP-Project/gh3/libmetal/build_r5'
make: *** [Makefile:141: all] Error 2

Our current CI doesn't build for Xilinx hardware, so it won't detect this. Have I missed something, or do I need to run a later GCC?

@edmooring
Copy link
Contributor

Digging a little deeper, this is due to libmetal using it's own atomic.h, instead of the GCC implementation.

Use atomic_flag type and the GCC __sync built-in functions for atomic
memory access. Initialize using ATOMIC_FLAG_INIT as a compound literal
or a cast if scalar type depending on which atomic.h is used.

The Linux client clears shared memory in ipi_shmem_echo().
Do not clear it again from the RPU in ipi_shmem_echod() to avoid a race.

Signed-off-by: Sergei Korneichuk <sergei.korneichuk@amd.com>
@kernelchuk
Copy link
Contributor Author

Hi Ed. Thank you for catching this. Using ATOMIC_FLAG_INIT outside of a declaration requires a compound literal or a cast (if a scalar type is used) depending on which atomic.h is used. I have updated the PR.

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.

Looks good now.

@arnopo arnopo merged commit a48ec03 into OpenAMP:main Dec 13, 2022
@kernelchuk kernelchuk deleted the u_atomic_flag branch December 14, 2022 04:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants