-
Notifications
You must be signed in to change notification settings - Fork 347
userspace: add support to use DAIs from user threads #10421
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
base: main
Are you sure you want to change the base?
Conversation
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.
Pull request overview
This PR adds support for using DAI (Digital Audio Interface) functionality from user-space threads by introducing a SOF DMA wrapper with syscall interfaces. The implementation modifies dai-zephyr.c to use the new wrapper functions instead of direct Zephyr DMA API calls, and adds a standalone test case to verify the user-space DMA+DAI flows.
Key changes:
- Migration of DMA API calls to SOF wrapper functions (
sof_dma_*) indai-zephyr.c - Addition of suspend/resume syscalls to the SOF DMA interface
- New user-space test case for Intel SSP DAI functionality
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
zephyr/test/userspace/test_intel_ssp_dai.c |
New comprehensive test case demonstrating user-space DMA and DAI operations with SSP device |
zephyr/test/CMakeLists.txt |
Updated build configuration to include the new SSP DAI test case |
zephyr/syscall/sof_dma.c |
Added syscall verification functions for DMA suspend/resume operations |
zephyr/include/sof/lib/sof_dma.h |
Added suspend/resume function declarations and implementations to the SOF DMA wrapper |
src/include/ipc4/gateway.h |
Removed unnecessary struct member names in union definitions |
src/audio/dai-zephyr.c |
Migrated all DMA API calls from direct Zephyr calls to SOF wrapper functions |
Comments suppressed due to low confidence (1)
zephyr/test/userspace/test_intel_ssp_dai.c:1
- Incorrect channel index used in cleanup. This should be
channel_ininstead ofchannel_out, as the preceding line releaseschannel_out.
// SPDX-License-Identifier: BSD-3-Clause
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| if (CONFIG_SOF_BOOT_TEST_STANDALONE) | ||
| if (CONFIG_DT_HAS_INTEL_ADSP_HDA_HOST_IN_ENABLED AND CONFIG_SOF_USERSPACE_INTERFACE_DMA) | ||
| zephyr_library_sources(userspace/test_intel_hda_dma.c) |
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.
Is it safe to run this test when userspace is active without CONFIG_SOF_USERSPACE_INTERFACE_DMA?
Was the change from test test_intel_hda_dma.c to test test_intel_ssp_dai.c intentional?
Shouldn't both tests be executed in this scenario?
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.
CONFIG_SOF_USERSPACE_INTERFACE_DMA depends on CONFIG_USERSPACE, so this is safe, but indeed the logic within the if case is not correct. This was messed up when I did the rebase, let me rework this in V2. Thanks!
|
While adding more the syscalls for dma, could we also wrap the following lines in Lines 493 to 497 in e00764e
|
| blob30.b.size = sizeof(blob30); | ||
| /* I2S config */ | ||
| blob30.b.i2s_ssp_config.ssc0 = 0x81d0077f; | ||
| blob30.b.i2s_ssp_config.ssc1 = 0xd0400004; |
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.
should we have some comments here? And indentation is off
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.
@lyakh are you saying raw hex values are not our preferred language? Are we mice or are we men? :) Let me add a comment. I was not planning to document these in detail (as we don't have the bitfield definitions in FW codebase and adding them just for a test case does not seem sensible -- we have a completely open generator in upstream alsa-utils after all), but I thought having the individual registers separated (versus just a big "uint8_t blob30[]") might come handy if you want to debug the SSP driver. This test case is a potentially nice way to exercise the driver on hardware. But alas, better comments are in order, let me add in V2.
UPDATE: and indentation is ok, it's github web view messing it up
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.
@kv2019i feel free to write the comment in hex, 0x6e,0x6f,0x20,0x70,0x72,0x6f,0x62,0x6c,0x65,0x6d,0x20,0x77,0x69,0x74,0x68,0x20,0x6d,0x65 ;-)
yep, understand, not proposing a full bit-by-bit documentation, just a short comment about where this comes from would be nice
2c0cb8a to
4667524
Compare
|
V2:
|
|
@lrudyX It seems the 06_07_TestBasicHdaLinkRandomDma is failing here. I don't think this is related, but given this PR does change dai-zephyr, probably better to do another test-run. Let me rebase the PR to latest SOF main -- no change to code, just a clean rebase, and let's see if the issue repeats. |
gateway.h defines "union flags" and "struct bits" to global namespace, which is not good in a public interface header file. There's no functional need to tag these struct definitions, so make the definitions anonymous. Signed-off-by: Kai Vehmanen <[email protected]>
Extend coverage of the Zephyr DAI interface to include dma_suspend() and dma_resume() calls exposed as syscalls. This allows e.g. to run the DAI module in a user thread. Signed-off-by: Kai Vehmanen <[email protected]>
The sof_dma.h syscalls are emitted to build whenever CONFIG_USERSPACE is set. Limit this so that the syscalls are emitted only if CONFIG_SOF_USERSPACE_INTERFACE_DMA is set. Signed-off-by: Kai Vehmanen <[email protected]>
Use the sof/lib/dma.h wrapper interface to access DMA devices. This allows to run this code also from user-space. The commit has no impact to builds where userspace is not used. Also this will not impact builds where user-space is enabled for some functionality, but the dai/copier module is not run in user space. Signed-off-by: Kai Vehmanen <[email protected]>
Add test check that correct channel is retrieved with sof_dma_request_channel(). Signed-off-by: Kai Vehmanen <[email protected]>
Prepare for other users of the standalone boot test infrastructure and do not run all tests in the sys init hook of "userspace_intel_hda_dma". Signed-off-by: Kai Vehmanen <[email protected]>
Test the SOF SSP DAI interface from a user-space thread. Implement a similar flow to configure a pair of DMA instances and a DAI device set up for both TX and RX, as is done in src/audio/dai-zephyr.c. The test does not cover running data through the DAI, as this requires separate programming of the DMA from host side, which cannot be done in this level of tests. Signed-off-by: Kai Vehmanen <[email protected]>
Update the README to cover common steps for building and running tests, and a separate section to introduce the available test cases. Signed-off-by: Kai Vehmanen <[email protected]>
4667524 to
d5ec70d
Compare
|
V3:
|
Add support to use DAIs from a user-thread. This includes modification of dai-zephyr.c to use SOF DMA wrapper that provides syscall interface for the DMA interface.
For the actual DAI interface, user-space support is added directly to Zephyr (see zephyrproject-rtos/zephyr#99811 ).
This PR doesn't yet affect any current default configuration. To allow to test the new user-space flows, a separate test case is added to test DMA+DAI (mimicking what dai-zephyr.c does) in a standalone test case.
Note: there is no hard dependency on the Zephyr-side PRs. The test case will not pass until all dependencies are included, but this PR can be merged before this.