Skip to content

Audio: Buffers: Add support for DP-to-DP component binding #10562

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

Open
singalsu wants to merge 1 commit into
base: main
Choose a base branch
from
+105 −26
Open

Audio: Buffers: Add support for DP-to-DP component binding #10562

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
44 changes: 33 additions & 11 deletions src/audio/buffers/audio_buffer.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,16 @@
int audio_buffer_attach_secondary_buffer(struct sof_audio_buffer *buffer, bool at_input,
struct sof_audio_buffer *secondary_buffer)
{
#ifdef CONFIG_DP_TO_DP_BIND
/* check per-side: allow attaching on both sides (needed for DP-to-DP) */

@lyakh lyakh Sep 9, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

is this the case of attaching the same buffer for the second time after one side has already been attached? Maybe rephrase the comment a bit to clarify that

if (at_input && buffer->secondary_buffer_sink)
return -EINVAL;
if (!at_input && buffer->secondary_buffer_source)
return -EINVAL;
#else
if (buffer->secondary_buffer_sink || buffer->secondary_buffer_source)
return -EINVAL;
#endif

/* secondary buffer must share audio params with the primary buffer */
secondary_buffer->audio_stream_params = buffer->audio_stream_params;
Expand All @@ -48,6 +56,19 @@ int audio_buffer_sync_secondary_buffer(struct sof_audio_buffer *buffer, size_t l
struct sof_source *data_src;
struct sof_sink *data_dst;

#ifdef CONFIG_DP_TO_DP_BIND
if (buffer->secondary_buffer_sink &&
buffer->secondary_buffer_sink == buffer->secondary_buffer_source) {
/*
* DP-to-DP case: a single shared ring_buffer is attached on both sides.
* The source DP writes directly to the ring_buffer sink API, and the
* sink DP reads directly from the ring_buffer source API.
* No copying is needed during the LL cycle.
*/
return 0;
}
#endif

if (buffer->secondary_buffer_sink) {
/*
* audio_buffer sink API is shadowed, that means there's a secondary_buffer
Expand Down Expand Up @@ -95,7 +116,12 @@ void audio_buffer_free(struct sof_audio_buffer *buffer)
CORE_CHECK_STRUCT(buffer);
#if CONFIG_PIPELINE_2_0
audio_buffer_free(buffer->secondary_buffer_sink);
#ifdef CONFIG_DP_TO_DP_BIND
if (buffer->secondary_buffer_source != buffer->secondary_buffer_sink)
audio_buffer_free(buffer->secondary_buffer_source);

@lyakh lyakh Sep 9, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

but will it be freed eventually on the second call?

#else
audio_buffer_free(buffer->secondary_buffer_source);
#endif
#endif /* CONFIG_PIPELINE_2_0 */
/* "virtual destructor": free the buffer internals and buffer memory */
buffer->ops->free(buffer);
Expand Down Expand Up @@ -203,18 +229,14 @@ uint32_t audio_buffer_sink_get_lft(struct sof_sink *sink)
return us_in_buffer;

/*
* TODO, Currently there's no DP to DP connection
* >>> the code below is never accessible and won't work because of cache incoherence <<<
*
* to make DP to DP connection possible:
* NOTE: DP-to-DP connections are now supported via a single shared ring_buffer
* attached as secondary buffer on both sides of a comp_buffer.
*
* 1) module data must be ALWAYS located in non cached memory alias, allowing
* cross core access to params like period (needed below) and calling
* module_get_deadline for the next module, regardless of cores the modules are
* running on
* 2) comp_buffer must be removed from all pipeline code, replaced with a generic abstract
* class audio_buffer - allowing using comp_buffer and ring_buffer without current
* "hybrid buffer" solution
* Future improvements:
* 1) module data should be in non-cached memory alias for reliable
* cross-core access to params like period and deadlines
* 2) comp_buffer should be replaced with generic audio_buffer
* throughout pipeline code (Pipeline 2.0)
*/
}

Expand Down
14 changes: 13 additions & 1 deletion src/audio/buffers/ring_buffer.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,6 @@ static inline void ring_buffer_writeback_shared(struct ring_buffer *ring_buffer,
dcache_writeback_region(ptr, size);
}


/**
* @brief remove the queue from the list, free memory
*/
Expand All @@ -101,6 +100,19 @@ static void ring_buffer_free(struct sof_audio_buffer *audio_buffer)

sof_ctx_free(alloc, (__sparse_force void *)ring_buffer->_data_buffer);
sof_ctx_free(alloc, ring_buffer);

#ifdef CONFIG_DP_TO_DP_BIND
/*
* When CONFIG_DP_TO_DP_BIND is enabled, ipc_comp_connect() takes an extra vregion
* reference for each ring_buffer created from a module vregion. Drop that
* reference here and free the allocation context only when the vregion refcount
* reaches zero.
*/
if (alloc && alloc->vreg) {
if (!vregion_put(alloc->vreg))
sof_heap_free(alloc->heap, alloc);
}

@lyakh lyakh Aug 26, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

if (alloc && alloc->vreg && !vregion_put(alloc->vreg))

#endif
}

static void ring_buffer_reset(struct sof_audio_buffer *audio_buffer)
Expand Down
7 changes: 7 additions & 0 deletions src/include/sof/audio/audio_buffer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -330,9 +330,16 @@ void audio_buffer_reset(struct sof_audio_buffer *buffer)
if (buffer->secondary_buffer_sink && buffer->secondary_buffer_sink->ops->reset)
buffer->secondary_buffer_sink->ops->reset(buffer->secondary_buffer_sink);

#ifdef CONFIG_DP_TO_DP_BIND
if (buffer->secondary_buffer_source &&
buffer->secondary_buffer_source != buffer->secondary_buffer_sink &&
buffer->secondary_buffer_source->ops->reset)
buffer->secondary_buffer_source->ops->reset(buffer->secondary_buffer_source);

@lyakh lyakh Sep 9, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

also here - will it ever be reset or is it intended that it never gets reset?

#else
if (buffer->secondary_buffer_source && buffer->secondary_buffer_source->ops->reset)
buffer->secondary_buffer_source->ops->reset(buffer->secondary_buffer_source);
#endif
#endif
}

/* Audio-buffer wrappers for the source-sink API */
Expand Down
57 changes: 43 additions & 14 deletions src/ipc/ipc4/helper.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -813,18 +813,22 @@ __cold int ipc4_comp_connect(struct ipc *ipc, const struct ipc4_module_bind_unbi
struct mod_alloc_ctx *alloc;

#if CONFIG_ZEPHYR_DP_SCHEDULER
if (source->ipc_config.proc_domain == COMP_PROCESSING_DOMAIN_DP &&
sink->ipc_config.proc_domain == COMP_PROCESSING_DOMAIN_DP) {
bool src_is_dp = source->ipc_config.proc_domain == COMP_PROCESSING_DOMAIN_DP;
bool sink_is_dp = sink->ipc_config.proc_domain == COMP_PROCESSING_DOMAIN_DP;
#ifdef CONFIG_DP_TO_DP_BIND
bool dp_to_dp = src_is_dp && sink_is_dp;
#else
if (src_is_dp && sink_is_dp) {
tr_err(&ipc_tr, "DP to DP binding is not supported: can't bind %x to %x",
src_id, sink_id);
return IPC4_INVALID_REQUEST;
}

#endif
struct comp_dev *dp;

if (sink->ipc_config.proc_domain == COMP_PROCESSING_DOMAIN_DP)
if (sink_is_dp)
dp = sink;
else if (source->ipc_config.proc_domain == COMP_PROCESSING_DOMAIN_DP)
else if (src_is_dp)
dp = source;
else
dp = NULL;

@lyakh lyakh Sep 9, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

this is now interesting. Previously buffers attached to DP modules were allocated on that DP module's vregion to have them accessible from that memory domain. How would this be resolved now? Do both DP modules on the two sides of the ring buffer have to belong to the same memory domain?..

Expand Down Expand Up @@ -897,8 +901,8 @@ __cold int ipc4_comp_connect(struct ipc *ipc, const struct ipc4_module_bind_unbi
*
* size = 2*max(obs of source module, ibs of destination module)
* (obs and ibs is single buffer size)
* in case of DP -> LL
* size = 2*ibs of destination (LL) module. DP queue will handle obs of DP module
* in case of DP -> LL or DP -> DP
* size = 2*ibs of destination module. DP queue will handle obs of DP module
*/
if (source->ipc_config.proc_domain == COMP_PROCESSING_DOMAIN_LL)
buf_size = MAX(ibs, obs) * 2;
Expand Down Expand Up @@ -933,12 +937,13 @@ __cold int ipc4_comp_connect(struct ipc *ipc, const struct ipc4_module_bind_unbi
#if CONFIG_ZEPHYR_DP_SCHEDULER
struct ring_buffer *ring_buffer = NULL;

if (sink->ipc_config.proc_domain == COMP_PROCESSING_DOMAIN_DP ||
source->ipc_config.proc_domain == COMP_PROCESSING_DOMAIN_DP) {
if (src_is_dp || sink_is_dp) {
struct processing_module *srcmod = comp_mod(source);
struct module_data *src_module_data = &srcmod->priv;
struct processing_module *dstmod = comp_mod(sink);
struct module_data *dst_module_data = &dstmod->priv;
bool is_shared = audio_buffer_is_shared(&buffer->audio_buffer);
uint32_t buf_id = buf_get_id(buffer);

/*
* Handle cases where the size of the ring buffer depends on the
Expand All @@ -950,16 +955,40 @@ __cold int ipc4_comp_connect(struct ipc *ipc, const struct ipc4_module_bind_unbi
*/
ring_buffer = ring_buffer_create(dp, MAX(ibs, dst_module_data->mpd.in_buff_size),
MAX(obs, src_module_data->mpd.out_buff_size),
audio_buffer_is_shared(&buffer->audio_buffer),
buf_get_id(buffer));
is_shared, buf_id);
if (!ring_buffer) {
buffer_free(buffer);
return IPC4_OUT_OF_MEMORY;
}

/* data destination module needs to use ring_buffer */
audio_buffer_attach_secondary_buffer(&buffer->audio_buffer, dp == source,
&ring_buffer->audio_buffer);
#ifdef CONFIG_DP_TO_DP_BIND
/*
* When CONFIG_DP_TO_DP_BIND is enabled, keep the DP module vregion
* alive for the lifetime of this ring_buffer (dropped in ring_buffer_free()).
*/
if (ring_buffer->audio_buffer.alloc)
vregion_get(ring_buffer->audio_buffer.alloc->vreg);

@kv2019i kv2019i Aug 28, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This I don't fully get. Why do we need an additional vregion_get/put on the ringbuffer that we already allocated in the normal single DP case. This seems correct, but I'm puzzled why this ref is not taken in ring_buffer_create(). @lyakh any thoughts?

@lyakh lyakh Aug 31, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

vregion reference counting was added because while we are allocating components and their data in and around the module-adapter, which is also where the vregion is created in the first place, we also create "normal" component buffers on that vregion. And while creation is done during component instantiation, which happens first, during freeing one of the buffers happens to be freed last - after the component. So, with ring buffers it wasn't needed until now because they are never created first or freed last. On the one hand refcounting them doesn't hurt (if done correctly) and might seem logical, OTOH if it isn't really needed - why add it.

@kv2019i kv2019i Aug 31, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hmm, thanks @lyakh . Makes sense, but I still think without above addennum, the code is hard to understand. I we need further updates, I'd add some comment about this.

#endif

#ifdef CONFIG_DP_TO_DP_BIND
if (dp_to_dp) {
/*
* DP-to-DP binding: both source and sink are DP modules.
* A single shared ring_buffer is attached on both sides
* of the comp_buffer, so source DP writes directly to it
Comment thread
singalsu marked this conversation as resolved.
* and sink DP reads directly from it without copying.
*/
audio_buffer_attach_secondary_buffer(&buffer->audio_buffer, true,
&ring_buffer->audio_buffer);
audio_buffer_attach_secondary_buffer(&buffer->audio_buffer, false,
&ring_buffer->audio_buffer);
} else
#endif
{
/* data destination module needs to use ring_buffer */
audio_buffer_attach_secondary_buffer(&buffer->audio_buffer, dp == source,
&ring_buffer->audio_buffer);
}
}

#endif /* CONFIG_ZEPHYR_DP_SCHEDULER */
Expand Down
9 changes: 9 additions & 0 deletions zephyr/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,15 @@ config ZEPHYR_DP_SCHEDULER_MIN_STACK_SIZE
ext_init payload. If the stack size requested in the IPC is
smaller than this, then the value defined here takes over.

config DP_TO_DP_BIND
bool "Support DP to DP component binding"
default y
depends on ZEPHYR_DP_SCHEDULER
help
Enable binding between two Data Processing (DP) scheduled components.
This allows connecting DP modules together (e.g. DP source to DP sink)
via intermediate buffering.

config CROSS_CORE_STREAM
bool "Enable cross-core connected pipelines"
default y if IPC_MAJOR_4
Expand Down
Loading