Skip to content

zdsp: Add ARC DSPLIB backend for zdsp #52478

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

Merged
merged 3 commits into from
Feb 21, 2023
Merged

zdsp: Add ARC DSPLIB backend for zdsp #52478

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5ea2043
zdsp: Introduce attribute marco for third-party backend
Jan 16, 2023
0694bd0
zdsp: add ARC DSPLIB backend for zdsp
Jan 16, 2023
be23d57
zdsp: add in-place operaton test for basicmath
Jan 16, 2023
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
9 changes: 6 additions & 3 deletions doc/services/dsp/index.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ optimized. The status of the various architectures can be found below:
============ =============
Architecture Status
============ =============
ARC Unoptimized
ARC Optimized
ARM Optimized
ARM64 Optimized
MIPS Unoptimized
Expand Down Expand Up @@ -46,11 +46,13 @@ Optimizing for your architecture

If your architecture is showing as ``Unoptimized``, it's possible to add a new
zDSP backend to better support it. To do that, a new Kconfig option should be
added to `subsys/dsp/Kconfig`_ along with the required dependencies and the
added to :file:`subsys/dsp/Kconfig` along with the required dependencies and the
``default`` set for ``DSP_BACKEND`` Kconfig choice.

Next, the implementation should be added at ``subsys/dsp/<backend>/`` and
linked in at `subsys/dsp/CMakeLists.txt`_.
linked in at :file:`subsys/dsp/CMakeLists.txt`. To add architecture-specific attributes,
its corresponding Kconfig option should be added to :file:`subsys/dsp/Kconfig` and use
them to update ``DSP_DATA`` and ``DSP_STATIC_DATA`` in :file:`include/zephyr/dsp/dsp.h`.

API Reference
*************
Expand All @@ -59,3 +61,4 @@ API Reference

.. _subsys/dsp/Kconfig: https://github.com/zephyrproject-rtos/zephyr/blob/main/subsys/dsp/Kconfig
.. _subsys/dsp/CMakeLists.txt: https://github.com/zephyrproject-rtos/zephyr/blob/main/subsys/dsp/CMakeLists.txt
.. _include/zephyr/dsp/dsp.h: https://github.com/zephyrproject-rtos/zephyr/blob/main/include/zephyr/dsp/dsp.h
192 changes: 102 additions & 90 deletions include/zephyr/dsp/basicmath.h
View file
Open in desktop

Large diffs are not rendered by default.

12 changes: 12 additions & 0 deletions include/zephyr/dsp/dsp.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,18 @@
#define DSP_FUNC_SCOPE
#endif

#ifdef CONFIG_DSP_BACKEND_HAS_AGU
#define DSP_DATA __agu
Copy link
Contributor

@yperess yperess Dec 20, 2022

Choose a reason for hiding this comment

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

I think that as we get more and more architecture backends we'll get more of these attributes. Can we use the same pattern as the static and have this as a Kconfig without a prompt in case ? maybe config DSP_BACKEND_HAS_AGU? Similarly for the data attribute, we can have a string Kconfig for the section. My goal is to make as few custom (per architecture) switches.

Copy link
Contributor

@yperess yperess Jan 18, 2023

Choose a reason for hiding this comment

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

I'm still a bit lost about the scalability of this approach, can you explain in more details how you see this when the next attribute comes up?

So lets assume we have CONFIG_DSP_BACKEND_HAS_AGU and a new attribute CONFIG_DSP_BACKEND_HAS_X, does the code now look like:

#if defined(CONFIG_DSP_BACKEND_HAS_AGU) && defined(CONFIG_DSP_BACKEND_HAS_X)
#define DSP_DATA __agu __x
#elif defined(CONFIG_DSP_BACKEND_HAS_AGU) && !defined(CONFIG_DSP_BACKEND_HAS_X)
#define DSP_DATA __agu
#elif !defined(CONFIG_DSP_BACKEND_HAS_AGU) && defined(CONFIG_DSP_BACKEND_HAS_X)
#define DSP_DATA __x
#endif

Copy link
Member

@ruuddw ruuddw Jan 18, 2023

Choose a reason for hiding this comment

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

I'd expect these would be mutual exclusive with at most a single attribute at a time. The _HAS refers to a specific hardware feature or flavor of DSP acceleration, so I don't expect a combination of _HAS_AGU and _HAS_X likely.
But now I understand your scalability concern better. Can we address it in the future when the case of HAS_AGU && HAS_X occurs?

Copy link
Member Author

@SiyuanCheng-CN SiyuanCheng-CN Jan 19, 2023

Choose a reason for hiding this comment

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

I also expect these are mutual exclusive, so the code shouldn't be such complex. I tried to use string Kconfig to directly pass attributes to C macro DSP_DATA and DSP_STATIC_DATA so that there is no if statement in code. Kconfig and C macro definition might look like:

config DSP_DATA
	string
	default "__agu" if DSP_BACKEND_ARCMWDT
	default "__x" if DSP_BACKEND_X

#define DSP_DATA CONFIG_DSP_DATA
But I found double quotes are introduced to C macro by make itself, which is unrecognizable by compiler. If this can be solved, the code can be simple.

#else
#define DSP_DATA
#endif

#ifdef CONFIG_DSP_BACKEND_HAS_XDATA_SECTION
#define DSP_STATIC_DATA DSP_DATA __attribute__((section(".Xdata")))
#else
#define DSP_STATIC_DATA DSP_DATA
#endif

/**
* @brief DSP Interface
* @defgroup math_dsp DSP Interface
Expand Down
2 changes: 2 additions & 0 deletions soc/arc/snps_nsim/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ else()
-Xshift_assist -Xfpus_div -Xfpu_mac -Xfpuda -Xfpus_mpy_slow
-Xfpus_div_slow -Xbitstream -Xtimer0 -Xtimer1)

zephyr_ld_option_ifdef(CONFIG_SOC_NSIM_EM11D -Hlib=em9d_nrg_fpusp -Hdsplib)

if(CONFIG_SOC_NSIM_EM11D)
set_property(GLOBAL PROPERTY z_arc_dsp_options -Xxy -Xagu_large -Hfxapi -Xdsp2
-Xdsp_accshift=full -Xdsp_divsqrt=radix2 -Xdsp_complex -Xdsp_itu
Expand Down
1 change: 1 addition & 0 deletions subsys/dsp/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,4 @@
# SPDX-License-Identifier: Apache-2.0

add_subdirectory_ifdef(CONFIG_DSP_BACKEND_CMSIS cmsis)
add_subdirectory_ifdef(CONFIG_DSP_BACKEND_ARCMWDT arcmwdt)
18 changes: 18 additions & 0 deletions subsys/dsp/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,16 @@ if DSP
config DSP_BACKEND_HAS_STATIC
bool

config DSP_BACKEND_HAS_AGU
bool

config DSP_BACKEND_HAS_XDATA_SECTION
bool

choice DSP_BACKEND
prompt "DSP library backend selection"
default DSP_BACKEND_CMSIS if CMSIS_DSP
default DSP_BACKEND_ARCMWDT if ARC && "$(ZEPHYR_TOOLCHAIN_VARIANT)" = "arcmwdt"
Copy link
Contributor

@yperess yperess Feb 21, 2023

Choose a reason for hiding this comment

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

I would suggest moving this up one line so we choose the right optimized default (feel free to do this in another PR in order to avoid getting another round of reviews).

default DSP_BACKEND_CUSTOM

config DSP_BACKEND_CMSIS
Expand All @@ -32,6 +39,17 @@ config DSP_BACKEND_CUSTOM
Rely on the application to provide a custom DSP backend. The implementation should be
added to the 'zdsp' build target by the application or one of its modules.

config DSP_BACKEND_ARCMWDT
bool "Use the mwdt library as the math backend"
depends on ARCMWDT_LIBC
depends on CMSIS_DSP
select DSP_BACKEND_HAS_STATIC
select DSP_BACKEND_HAS_AGU
select DSP_BACKEND_HAS_XDATA_SECTION
help
Implement the various zephyr DSP functions using the MWDT-DSP library. This feature
requires the MetaWare toolchain and CMSIS module to be selected.

endchoice

endif # DSP
8 changes: 8 additions & 0 deletions subsys/dsp/arcmwdt/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
# Copyright (c) 2022 Synopsys
# SPDX-License-Identifier: Apache-2.0

zephyr_include_directories(public)

zephyr_include_directories_ifdef(CONFIG_DSP_BACKEND_ARCMWDT
${ARCMWDT_TOOLCHAIN_PATH}/MetaWare/arc/lib/src/dsp/include/
)
Loading