Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
78 changes: 67 additions & 11 deletions src/audio/buffers/audio_buffer.c
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)
{
#if CONFIG_DP_TO_DP_BIND

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.

Style note, "#ifdef CONFIG_DP_TO_DP_BIND" is the usual convention. @lyakh agrees, but Linux kernel and statistics of use in SOF are on my side with this.

/* check per-side: allow attaching on both sides (needed for DP-to-DP) */
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,56 @@ int audio_buffer_sync_secondary_buffer(struct sof_audio_buffer *buffer, size_t l
struct sof_source *data_src;
struct sof_sink *data_dst;

#if CONFIG_DP_TO_DP_BIND
if (buffer->secondary_buffer_sink && buffer->secondary_buffer_source) {
/*
* DP-to-DP case: both secondary buffers present.
* Data flows: input_ring_buffer -> comp_buffer -> output_ring_buffer
*
* This buffer is visited twice during each LL cycle:
* - In the input loop (sink DP module via comp_dev_for_each_producer) with
* limit == SIZE_MAX. In a DP-to-DP connection, input to sink DP is fed
* via output_ring_buffer; the intermediate comp_buffer transfer is driven
* by the output loop. Hence, this call is a no-op.
* - In the output loop (source DP module via comp_dev_for_each_consumer) with
* limit == source_get_min_available(downstream). This executes the 2-step
* cascade with rate-limiting properly applied on the output transfer.
*/
if (limit == SIZE_MAX)
return 0;
Comment thread
singalsu marked this conversation as resolved.

/*
* Step 1: copy from input secondary buffer to primary (comp_buffer).
* No limit on input side - copy all available data.
*/
data_src = audio_buffer_get_source(buffer->secondary_buffer_sink);
data_dst = &buffer->_sink_api;

size_t data_available = source_get_data_available(data_src);
size_t free_size = sink_get_free_size(data_dst);
size_t to_copy = MIN(data_available, free_size);

err = source_to_sink_copy(data_src, data_dst, true, to_copy);
if (err)
return err;

/*
* Step 2: copy from primary (comp_buffer) to output secondary buffer.
* Apply the limit to the output side to control how much data
* is made available to the downstream DP module per LL cycle.
*/
data_src = &buffer->_source_api;
data_dst = audio_buffer_get_sink(buffer->secondary_buffer_source);

data_available = source_get_data_available(data_src);
free_size = sink_get_free_size(data_dst);
to_copy = MIN(MIN(data_available, free_size), limit);

err = source_to_sink_copy(data_src, data_dst, true, to_copy);
return err;
}
#endif

if (buffer->secondary_buffer_sink) {
/*
* audio_buffer sink API is shadowed, that means there's a secondary_buffer
Expand Down Expand Up @@ -203,18 +261,16 @@ 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 dual ring_buffers
* attached as secondary buffers on both sides of a comp_buffer.
* Data cascades: ring_buf_src -> comp_buffer -> ring_buf_sink
* with syncing during each LL cycle.
*
* 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
13 changes: 12 additions & 1 deletion src/audio/buffers/ring_buffer.c
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,18 @@ 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);

#if CONFIG_DP_TO_DP_BIND
/*
* For DP-to-DP binding: matches vregion_get() in ipc_comp_connect()
* for each ring_buffer. Releases the DP module's virtual memory region
* and frees the module allocation context when the refcount reaches zero.
*/
if (alloc && alloc->vreg) {
if (!vregion_put(alloc->vreg))
rfree(alloc);
}
#endif
}

static void ring_buffer_reset(struct sof_audio_buffer *audio_buffer)
Expand Down
2 changes: 1 addition & 1 deletion src/audio/module_adapter/module_adapter.c
Original file line number Diff line number Diff line change
Expand Up @@ -1164,7 +1164,7 @@ static int module_adapter_copy_ring_buffers(struct comp_dev *dev)
/* input - we need to copy data from audio_stream (as source)
* to ring_buffer (as sink)
*/
err = audio_buffer_sync_secondary_buffer(&buffer->audio_buffer, UINT_MAX);
err = audio_buffer_sync_secondary_buffer(&buffer->audio_buffer, SIZE_MAX);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

separate commit ?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yep, done. Also I used CONFIG_DP_TO_DP_BIND to help find the code for this.


if (err) {
comp_err(dev, "LL to DP copy error status: %d", err);
Expand Down
65 changes: 54 additions & 11 deletions src/ipc/ipc4/helper.c
Original file line number Diff line number Diff line change
Expand Up @@ -803,18 +803,22 @@ __cold int ipc_comp_connect(struct ipc *ipc, ipc_pipe_comp_connect *_connect)
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;
#if 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;
Expand Down Expand Up @@ -887,8 +891,8 @@ __cold int ipc_comp_connect(struct ipc *ipc, ipc_pipe_comp_connect *_connect)
*
* 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 @@ -923,12 +927,13 @@ __cold int ipc_comp_connect(struct ipc *ipc, ipc_pipe_comp_connect *_connect)
#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 @@ -940,16 +945,54 @@ __cold int ipc_comp_connect(struct ipc *ipc, ipc_pipe_comp_connect *_connect)
*/
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;
}

#if CONFIG_DP_TO_DP_BIND
/* refcount the DP vregion for this ring_buffer (matches vregion_put in
* ring_buffer_free for DP-to-DP binding)
*/
if (ring_buffer->audio_buffer.alloc)
vregion_get(ring_buffer->audio_buffer.alloc->vreg);

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?

#endif

/* data destination module needs to use ring_buffer */
audio_buffer_attach_secondary_buffer(&buffer->audio_buffer, dp == source,
&ring_buffer->audio_buffer);

#if CONFIG_DP_TO_DP_BIND
/*
* DP-to-DP binding: both source and sink are DP modules.
* A second ring_buffer is needed on the other side of the comp_buffer
* so each DP module has its own lock-free ring_buffer interface.
* Data flows: src_DP -> ring_buf_src -> comp_buffer -> ring_buf_sink -> sink_DP
* The comp_buffer acts as the intermediary synced during LL cycles.
*/
if (dp_to_dp) {
struct ring_buffer *ring_buffer2;

ring_buffer2 =
ring_buffer_create(source,
MAX(ibs, dst_module_data->mpd.in_buff_size),
MAX(obs, src_module_data->mpd.out_buff_size),
is_shared, buf_id);
if (!ring_buffer2) {
buffer_free(buffer);

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.

How about the first ring_buffer we allocated (and the vregion ref), should we free those here?

return IPC4_OUT_OF_MEMORY;
}

/* refcount the source DP vregion for ring_buffer2 */
if (ring_buffer2->audio_buffer.alloc)
vregion_get(ring_buffer2->audio_buffer.alloc->vreg);

/* attach second ring_buffer on the source side */
audio_buffer_attach_secondary_buffer(&buffer->audio_buffer, dp != source,
&ring_buffer2->audio_buffer);
}
#endif
}

#endif /* CONFIG_ZEPHYR_DP_SCHEDULER */
Expand Down
9 changes: 9 additions & 0 deletions zephyr/Kconfig
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,15 @@ config ZEPHYR_DP_SCHEDULER
DP modules can be located in dieffrent cores than LL pipeline modules, may have
different tick (i.e. 300ms for speech reccognition, etc.)

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