pipeline 2.0 - introduce abstract audio_buffer api #9260

marcinszkudlinski · 2024-06-26T15:17:53Z

This PR introduces an abstract struct sof_audio_buffer which should become a base abstract class for all buffer types in pipeline 2.0
PR should be neutral for all functionality

On the generic pipeline code there MUST NOT be any other usage of buffers but:

sof_audio_buffer for maintenance operations
sof_sink
sof_source

(with obvious exception for buffer creation - see concept of "buffer factory)

this PR change usage of DP_queue (renamed to ring_buffer) to match the above request

lyakh · 2024-06-27T07:03:28Z

src/include/sof/audio/audio_stream.h

+						*/
+	struct ring_buffer *ring_buffer_source; /**< source API shadow, an additional ring_buffer
+						  * at data out
+						  */


not sure these two renames make sense, or at least they might deserve a separate commit. It's one thing to rename an API, but it doesn't necessarily mean, that all users have to be renamed too. If these members are only used by DP components, maybe we can keep their names? Or if you want to use these members for other cases too, then this should rather be a separate commit, but then maybe you also need to move them out of the #if?

lyakh · 2024-06-27T07:06:03Z

src/audio/ring_buffer.c

@@ -0,0 +1,306 @@
+// SPDX-License-Identifier: BSD-3-Clause


this rename-and-modify unfortunately confused git enough to not recognise this as a rename any more. Maybe we can split this into two commits: first rename without modifying contents, then change the API itself? Or the other way round - doesn't matter. As it stands the only way to review this would be to download both files and to diff them manually

I changed to 2 commits - one for rename + move, second for internal structures changes

zephyr/CMakeLists.txt

lyakh · 2024-06-27T07:14:39Z

src/include/sof/audio/audio_stream.h

+						      */
+	struct sof_audio_buffer *shadow_buffer_source; /**< source API shadow, an additional buffer
+							* at data output
+							*/


oookeeey, not sure why it's "shadow" but at least - yes, to reinforce my previous comment, I'd keep the "dp-buffer" name until here

I changed "shadow" to "secondary", but keeping dp_queue here is not a good idea - double buffering it is not limited to ring_buffer a.k.a. dp_queue now

src/audio/buffer.c

src/audio/buffers/ring_buffer.c

src/include/sof/audio/audio_buffer.h

src/ipc/ipc4/helper.c

marcinszkudlinski · 2024-07-04T07:25:58Z

Development is of course still in progress, but this PR looks like a good subset. Please proceed with review.

marcinszkudlinski · 2024-07-04T07:28:39Z

src/audio/buffer.c

@@ -272,8 +272,8 @@ void buffer_free(struct comp_buffer *buffer)
 	/* In case some listeners didn't unregister from buffer's callbacks */
 	notifier_unregister_all(NULL, buffer);
 #if CONFIG_ZEPHYR_DP_SCHEDULER
-	ring_buffer_free(buffer->stream.dp_queue_sink);
-	ring_buffer_free(buffer->stream.dp_queue_source);
+	ring_buffer_free_legacy(buffer->stream.dp_queue_sink);


this "legacy" is introduced ONLY to make this commit compile (and probably work) - there was a function name collision.
The function is removed in the very next commit

src/ipc/ipc4/helper.c

lgirdwood · 2024-07-05T13:42:46Z

@marcinszkudlinski good stuff - CI looking good too, just a minor conflict.

marcinszkudlinski · 2024-07-05T15:00:26Z

as to codestyle check failure:

WARNING: please write a help paragraph that fully describes the config symbol

I did, the symbol is fully described, but checkpatch is blind to this.

dp_queue was created as a buffer to handle special needs for DP modules. However, in pipeline2.0 there will be more usecases for it - as in fact it is a lockless cross-core cached ring buffer. This commit does rename dp_queue to more adequate name It also moves the file to "buffers" directory, a place for all implementations of buffers in pipeline 2.0 The commit, however, does not change names of structures because git/github does not handle complex changes like rename and modification correctly Signed-off-by: Marcin Szkudlinski <marcin.szkudlinski@intel.com>

as a follow-up of a prev commit, internal structures should also be changed accordingly The commit, however, does not change names of variables in the code that use struct ring_buffer. Signed-off-by: Marcin Szkudlinski <marcin.szkudlinski@intel.com>

in pipeline2.0 there's more than one buffer type to be used for passing audio data from one module to another. However, each single buffer must provide same API. This commit introduces a common API for buffers and at the same time connects it to ring_buffer. Signed-off-by: Marcin Szkudlinski <marcin.szkudlinski@intel.com>

This commit removes all direct calls to ring_buffer (formerly dp_queue) from the code, replacing with a generic sof_audio_buffer API Only direct call that stays is creation of ring_buffer, yet it will soon be replaced also by a buffer factory Legacy API from ring buffer is also removed, as it should not be used anymore Signed-off-by: Marcin Szkudlinski <marcin.szkudlinski@intel.com>

This flag enables changes to new pipeline structure, known as pipeline2_0 It is required for certain new features, like DP_SCHEDULER. The changes are incremental and at the moment pipeline 2.0 is fully backward compatible with legacy platforms, however it generates some overhead in data and code, so it is useful to turn if off if not needed Signed-off-by: Marcin Szkudlinski <marcin.szkudlinski@intel.com>

marcinszkudlinski · 2024-07-08T10:43:22Z

rebased

marcinszkudlinski · 2024-07-09T06:57:14Z

I missed one commit during rebase

kv2019i

Looks god to me and cleans up the code. I was wondering whether this is really dependent on IPC4 (dp scheduling is, but also the ringbuffer/dpqueue)? Anyways, not a blockign item. Tests seem good. LNL hard to tell as this seems to hit thesofproject/linux#5098 .

marc-hb · 2024-07-09T17:54:56Z

This compilation failure seems relevant:
https://github.com/thesofproject/sof/actions/runs/9852402212/job/27200883856?pr=9260

kv2019i · 2024-07-10T08:12:53Z

src/ipc/ipc4/helper.c

 				return IPC4_FAILURE;
 			}
 		if (!cpu_is_me(sink->ipc_config.core))
 			if (ll_wait_finished_on_core(sink) < 0) {
-				ll_unblock(cross_core_unbind);
+				ll_unblock(cross_core_unbind, flags);
 				return IPC4_FAILURE;
 			}
 #else


@marcinszkudlinski Can you check the CI build fail? I think you missed a few spots in helper.c wheere CONFIG_CROSS_CORE_STREAM is ifdeffed out and there's call to ll_block/ll_unblock.

Explicit usage of a local variable in a macro is not a good practice. Macros changed to use a variable as an argument + fix a warning "'flags' may be used uninitialized" Signed-off-by: Marcin Szkudlinski <marcin.szkudlinski@intel.com>

kv2019i · 2024-07-10T12:31:02Z

One case of #9191 in https://sof-ci.01.org/sofpr/PR9260/build6422/devicetest/index.html , rest looks clean. Merging.

marcinszkudlinski added the pipeline2.0 label Jun 26, 2024

lyakh reviewed Jun 27, 2024

View reviewed changes

kv2019i mentioned this pull request Jun 27, 2024

[BUG] DSP panic with Zephyr on Intel MTL, regression 27th June #9268

Closed

marcinszkudlinski force-pushed the pipeline2_0_001 branch 2 times, most recently from 6cdf717 to d060b70 Compare July 3, 2024 16:12

marcinszkudlinski changed the title ~~[WIP] pipeline 2.0 - introduce abstract audio_buffer api~~ pipeline 2.0 - introduce abstract audio_buffer api Jul 4, 2024

marcinszkudlinski marked this pull request as ready for review July 4, 2024 07:24

marcinszkudlinski requested review from marc-hb, kv2019i, iuliana-prodan, dbaluta, abonislawski, ranj063, jxstelter, lgirdwood, plbossart, mmaka1 and lbetlej as code owners July 4, 2024 07:24

marcinszkudlinski commented Jul 4, 2024

View reviewed changes

lyakh reviewed Jul 4, 2024

View reviewed changes

src/ipc/ipc4/helper.c Show resolved Hide resolved

marcinszkudlinski force-pushed the pipeline2_0_001 branch from 8b1dad0 to d40b53e Compare July 4, 2024 12:06

lgirdwood approved these changes Jul 5, 2024

View reviewed changes

marcinszkudlinski added 5 commits July 8, 2024 12:30

marcinszkudlinski force-pushed the pipeline2_0_001 branch from d40b53e to a901325 Compare July 8, 2024 10:42

lgirdwood approved these changes Jul 9, 2024

View reviewed changes

kv2019i approved these changes Jul 9, 2024

View reviewed changes

kv2019i reviewed Jul 10, 2024

View reviewed changes

fix: explicit usage of a local var in macro

dbc9a92

Explicit usage of a local variable in a macro is not a good practice. Macros changed to use a variable as an argument + fix a warning "'flags' may be used uninitialized" Signed-off-by: Marcin Szkudlinski <marcin.szkudlinski@intel.com>

marcinszkudlinski force-pushed the pipeline2_0_001 branch from 3c499f4 to dbc9a92 Compare July 10, 2024 10:02

kv2019i merged commit 2a9b40f into thesofproject:main Jul 10, 2024
44 of 47 checks passed

marcinszkudlinski deleted the pipeline2_0_001 branch August 7, 2024 13:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pipeline 2.0 - introduce abstract audio_buffer api #9260

pipeline 2.0 - introduce abstract audio_buffer api #9260

marcinszkudlinski commented Jun 26, 2024 •

edited

Loading

lyakh Jun 27, 2024

lyakh Jun 27, 2024

marcinszkudlinski Jul 3, 2024

lyakh Jun 27, 2024

marcinszkudlinski Jul 3, 2024

marcinszkudlinski commented Jul 4, 2024 •

edited

Loading

marcinszkudlinski Jul 4, 2024 •

edited

Loading

lgirdwood commented Jul 5, 2024

marcinszkudlinski commented Jul 5, 2024

marcinszkudlinski commented Jul 8, 2024

marcinszkudlinski commented Jul 9, 2024

kv2019i left a comment

marc-hb commented Jul 9, 2024

kv2019i Jul 10, 2024

kv2019i commented Jul 10, 2024

pipeline 2.0 - introduce abstract audio_buffer api #9260

pipeline 2.0 - introduce abstract audio_buffer api #9260

Conversation

marcinszkudlinski commented Jun 26, 2024 • edited Loading

lyakh Jun 27, 2024

Choose a reason for hiding this comment

lyakh Jun 27, 2024

Choose a reason for hiding this comment

marcinszkudlinski Jul 3, 2024

Choose a reason for hiding this comment

lyakh Jun 27, 2024

Choose a reason for hiding this comment

marcinszkudlinski Jul 3, 2024

Choose a reason for hiding this comment

marcinszkudlinski commented Jul 4, 2024 • edited Loading

marcinszkudlinski Jul 4, 2024 • edited Loading

Choose a reason for hiding this comment

lgirdwood commented Jul 5, 2024

marcinszkudlinski commented Jul 5, 2024

marcinszkudlinski commented Jul 8, 2024

marcinszkudlinski commented Jul 9, 2024

kv2019i left a comment

Choose a reason for hiding this comment

marc-hb commented Jul 9, 2024

kv2019i Jul 10, 2024

Choose a reason for hiding this comment

kv2019i commented Jul 10, 2024

marcinszkudlinski commented Jun 26, 2024 •

edited

Loading

marcinszkudlinski commented Jul 4, 2024 •

edited

Loading

marcinszkudlinski Jul 4, 2024 •

edited

Loading