-
Notifications
You must be signed in to change notification settings - Fork 369
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.
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
| /* 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; | ||
|
|
@@ -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 | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm wondering... I think the ring buffer was designed in a way to support asynchronous / lockless reading and writing (or something similar) and it should have been tailored to the use with DP. Shouldn't it be possible to just do DP -> ring_buffer -> DP?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, only one ring buffer should be used here. |
||
| * | ||
| * 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; | ||
|
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; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| } | ||
| #endif | ||
|
|
||
| if (buffer->secondary_buffer_sink) { | ||
| /* | ||
| * audio_buffer sink API is shadowed, that means there's a secondary_buffer | ||
|
|
@@ -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) | ||
| */ | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
| */ | ||
|
|
@@ -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); | ||
| } | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| #endif | ||
| } | ||
|
|
||
| static void ring_buffer_reset(struct sof_audio_buffer *audio_buffer) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. separate commit ?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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; | ||
|
|
@@ -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; | ||
|
|
@@ -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 | ||
|
|
@@ -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); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
|
||
| /* 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); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 */ | ||
|
|
||
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.
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.
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.
do I prefer
#if? I don't know any more :-D I don't care that much really. I thought one was preferred and I tried to comply, but then we didn't find any written down preference for SOF. My current dilemma is on the one hand#ifis shorter and is easier to extend with logical operations like#if CONFIG_A || CONFIG_B, but OTOH#ifdefis "cleaner" because when something isn't defined, it shouldn't really be possible to check its value...