[Userspace LL] Make chain-dma compatible with user-space LL build - #11070
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the chain-dma audio component to run in Zephyr user-space LL threads by switching to the sof_dma_* userspace-safe DMA APIs and by ensuring chain-dma objects/buffers are allocated from user-accessible heaps/contexts, while keeping kernel-LL builds unaffected.
Changes:
- Replace direct Zephyr
dma_*calls anddma_chan_data*usage withsof_dma_*APIs and stored channel indices. - Allocate
comp_devandchain_dma_datafrom the user heap underCONFIG_SOF_USERSPACE_LL. - Allocate the DMA buffer using the LL userspace allocation context (
ipc_get()->ll_alloc) when running user-space LL.
Suppressed comments (4)
src/audio/chain_dma.c:105
- chain_link_start() calls sof_dma_start() without verifying dma_link is non-NULL and chan_link_index is valid (>= 0). Since sof_dma_start() takes a uint32_t, a negative chan_link_index will be converted to a large value and may lead to out-of-bounds access/crash (e.g., if PAUSE/STOP is issued before successful init). Add the same initialization guard used in chain_host_start().
static int chain_link_start(struct comp_dev *dev)
{
struct chain_dma_data *cd = comp_get_drvdata(dev);
int err;
err = sof_dma_start(cd->dma_link, cd->chan_link_index);
if (err < 0)
src/audio/chain_dma.c:120
- chain_link_stop() should validate dma_link and chan_link_index before calling sof_dma_stop(). With the new int->uint32_t channel conversion, an uninitialized/negative index (e.g., stop before start) can turn into a huge channel number and crash. Also the log should use %d for an int index.
static int chain_link_stop(struct comp_dev *dev)
{
struct chain_dma_data *cd = comp_get_drvdata(dev);
int err;
err = sof_dma_stop(cd->dma_link, cd->chan_link_index);
if (err < 0)
return err;
src/audio/chain_dma.c:135
- chain_host_stop() no longer checks for incomplete initialization before calling sof_dma_stop(). If chan_host_index is still negative, it will be converted to a large uint32_t channel and can lead to out-of-bounds access/crash. Add a guard (mirroring chain_host_start()) and use a signed format specifier for the int index.
static int chain_host_stop(struct comp_dev *dev)
{
struct chain_dma_data *cd = comp_get_drvdata(dev);
int err;
err = sof_dma_stop(cd->dma_host, cd->chan_host_index);
if (err < 0)
return err;
src/audio/chain_dma.c:380
- chain_release() unconditionally calls sof_dma_release_channel() with chan_index, which is now an int initialized to -EINVAL. If chain_release() is ever reached with an invalid index (or dma is NULL), the negative value will be converted to a large uint32_t channel and may crash. Guard the release/put calls and reset indices/pointers after releasing to avoid accidental double-release.
__cold static void chain_release(struct comp_dev *dev)
{
struct chain_dma_data *cd = comp_get_drvdata(dev);
assert_can_be_cold();
sof_dma_release_channel(cd->dma_host, cd->chan_host_index);
sof_dma_put(cd->dma_host);
sof_dma_release_channel(cd->dma_link, cd->chan_link_index);
sof_dma_put(cd->dma_link);
| comp_info(dev, "dma_start() host chan_index = %u", | ||
| cd->chan_host->index); | ||
| cd->chan_host_index); |
Port chain DMA to SOF's sof_dma_* syscall wrappers (as host and dai already do) so its DMA operations can run unprivileged. Channel handles are stored as integer indices instead of kernel-only struct dma_chan_data pointers, matching the sof_dma_* API which takes indices. Both channel indices are initialised to -EINVAL so an incomplete initialisation is detected consistently for host and link. No functional change for existing (privileged) builds. Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
When the LL pipeline runs in user-space (CONFIG_SOF_USERSPACE_LL) the component and its private data must reside on the user heap so the unprivileged user LL thread can access them. Introduce chain_dev_alloc()/ chain_cd_alloc() and their free counterparts to keep the config-specific allocation out of chain_task_create()/chain_task_free() instead of sprinkling #ifdefs through the control flow. The non-user-space path is unchanged (comp_alloc()/rzalloc()). Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
For CONFIG_SOF_USERSPACE_LL the DMA buffer must be allocated from the user LL heap so it is reachable by the unprivileged user LL thread that runs chain_task_run(). Pass the LL alloc context to buffer_alloc() instead of NULL; for non-user-space builds alloc_ctx stays NULL and the default heap is used as before. Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>
26a44db to
14e745b
Compare
|
V2 pushed:
|
|
@jsarha @lgirdwood good to go? |
| /* local host DMA config */ | ||
| struct sof_dma *dma_host; | ||
| struct dma_chan_data *chan_host; | ||
| int chan_host_index; |
There was a problem hiding this comment.
not worth respinning the PR just for this, but maybe just something to agree upon for future PRs - I'd say this commit already does constitute a "functional change?" Personally to me "no functional change" are like moving complete functions around, splitting or merging files, adding comments, etc. Even splitting or merging functions is already not a perfectly clear cut IMHO.
Set of patches to allow chaindma to work in Zephyr user-space threads. Tested as part of #10558
No impact to kernel-LL SOF builds (still the default for all targets).