Pipeline2.0 STEP2 - move hybrid buffering logic from comp_buffer to audio_buffer and partially place comp_buffer on top of audio_buffer #9299

marcinszkudlinski · 2024-07-12T11:51:31Z

all logic of hybrid buffering - double buffering, currently comp_buffer <=> ring_buffer, moved to audio_buffer layer
comp_buffer initially uses audio_buffer API - sink/source removed from audio_stream

next steps would be to remove all access to comp_buffer except by 3APIs: audio_buffer / sink / source
Those APIs may of course get another functions during process

lgirdwood

@marcinszkudlinski I assume there will be a flag day when we enable the Kconfig for pipeline 2.0 and this PR is just a step to that day ?

marcinszkudlinski · 2024-08-09T12:41:17Z

@lgirdwood the flag is enabled for IPC4 platforms.

Some features, like DP processing, require it
The changes in this PR are transparent for functionality
for legacy platforms - disabling the flag is just saving memory

kv2019i

Code looks good, ok to +1 once CI is clean. Some build warnings that need to be addressed like https://github.com/thesofproject/sof/actions/runs/10316722150/job/28559534467?pr=9299

marcinszkudlinski · 2024-08-14T15:30:21Z

fixin compilation in progress, there's still some work to do so DNM

marcinszkudlinski · 2024-08-20T12:41:53Z

memory leak fixed
compilation problems fixed
please proceed

marc-hb · 2024-08-20T17:19:28Z

The input/output error in https://sof-ci.01.org/sofpr/PR9299/build7167/devicetest/index.html?model=LNLM_SDW_AIOC&testcase=check-capture-3times
seems completely new.

arecord: pcm_read:2221: read error: Input/output error
2024-08-20 11:24:54 UTC [REMOTE_ERROR] arecord on PCM hw:0,3 failed at 1/3.
2024-08-20 11:24:54 UTC [REMOTE_INFO] Starting func_exit_handler(1)
2024-08-20 11:24:54 UTC [REMOTE_ERROR] Starting func_exit_handler(), exit status=1, FUNCNAME stack:
2024-08-20 11:24:54 UTC [REMOTE_ERROR]  die()  @  /home/ubuntu/sof-test/test-case/../case-lib/lib.sh
2024-08-20 11:24:54 UTC [REMOTE_ERROR]  main()  @  /home/ubuntu/sof-test/test-case/check-capture.sh:101

marc-hb · 2024-08-20T20:36:17Z

SOFCI TEST

EDIT: it's gone in https://sof-ci.01.org/sofpr/PR9299/build7192/devicetest/index.html; that newer run has only known issues...

marcinszkudlinski · 2024-08-23T13:49:23Z

@lgirdwood @kv2019i can we proceed?

kv2019i · 2024-08-26T12:53:12Z

@marcinszkudlinski Can you check Intel internal/quickbuild fail? That's the only thing holding back merge.

lgirdwood · 2024-09-02T10:56:37Z

@marcinszkudlinski Can you check Intel internal/quickbuild fail? That's the only thing holding back merge.

@marcinszkudlinski Any update?

marcinszkudlinski · 2024-09-02T12:09:26Z

@lgirdwood looks like there was a problem with FPGA testing, hopefully fixed
I'll rebase on head and will try again

cache operations are pointless when architecture is coherent Signed-off-by: Marcin Szkudlinski <marcin.szkudlinski@intel.com>

secondary buffer mechanism is a feature independent of a buffer implementation, therefore it should be located in a base class for all buffers this commit intentionally is not removing the same feature from comp_buffer nor trying to use it anywhere Signed-off-by: Marcin Szkudlinski <marcin.szkudlinski@intel.com>

in struct audio_buffer there was only one virtual method (free). There are probably more coming, so an ops structure is introduced. ring buffer which uses audio buffer as a base must adjust, including using of an audio_buffer_init procedure, what implies using audio_stream_params located in audio_buffer Signed-off-by: Marcin Szkudlinski <marcin.szkudlinski@intel.com>

comp_buffer, implemented in buffer.c file, should be kept in the buffers directory, together with all buffers Also it has been renamed to comp_buffer.c Signed-off-by: Marcin Szkudlinski <marcin.szkudlinski@intel.com>

comp_buffer is a legacy buffer, but currently is widely used in SOF. During transition to pipeline2.0 there's a need that comp_buffer expose a common API, as any other buffer Signed-off-by: Marcin Szkudlinski <marcin.szkudlinski@intel.com>

one variable have been forgotten during renaming dp_queue to ring_buffer Signed-off-by: Marcin Szkudlinski <marcin.szkudlinski@intel.com>

lgirdwood · 2024-09-02T15:46:48Z

src/audio/CMakeLists.txt

@@ -128,7 +129,7 @@ endif()
 add_local_sources(sof
 	component.c
 	data_blob.c
-	buffer.c
+	buffers/comp_buffer.c


We can do this in the next PR if needed (as CI is good), but does it make sense to rename like other similar files to legacy-buffer.c ?

legacy-buffer.c
looks as a good idea
also struct comp_buffer ==> struct legacy_buffer
it will be a clear sign that an API is depreciated

lyakh · 2024-09-03T06:34:01Z

src/audio/buffers/audio_buffer.c

+	secondary_buffer->audio_stream_params = buffer->audio_stream_params;
+	/* for performance reasons pointers to params are also kept in sink/src structures */
+	secondary_buffer->_sink_api.audio_stream_params = buffer->audio_stream_params;
+	secondary_buffer->_source_api.audio_stream_params = buffer->audio_stream_params;


this seems to make 3 copies of stream parameters, so now we have 4 of them on the total? This seems really redundant. Is this a temporary situation until buffers are migrated to the new API? Maybe make one of them where it finally should be and use pointers at all other locations? TBH I'm a bit confused about the roadmap of the migration, how far we're into it and what is still left to do. @marcinszkudlinski maybe you could make a "feature" to explain the steps made so far, the current configuration (what types of buffers we currently have, what copying is done, etc.) and the roadmap?

marc-hb · 2024-09-03T23:59:09Z

@marcinszkudlinski maybe you could make a "feature" to explain the steps made so far, the current configuration (what types of buffers we currently have, what copying is done, etc.) and the roadmap?

Maybe like this? https://github.com/orgs/thesofproject/discussions/8645

I assumed this link was already there somewhere in the commit messages or PR description or source code comments but maybe it was missing.

marcinszkudlinski · 2024-09-04T06:12:00Z

@lyakh @marc-hb
Yes, I need to update the doc you mention
EDIT; the most current version of the document is here
thesofproject/sof-docs#497

and readable version
https://marcinszkudlinski.github.io/sof-docs/PAGES/architectures/firmware/sof-common/pipeline_2_0/pipeline2_0_discussion.html

I add "obsolete" node to old versions

I'm doing pipeline2,0 in a "desing&implement" mode, so archeo drifts also a bit when new issues&possibilites come into my mind.

Also, I roadmap would be handy.

lyakh · 2024-09-04T07:24:16Z

and readable version https://marcinszkudlinski.github.io/sof-docs/PAGES/architectures/firmware/sof-common/pipeline_2_0/pipeline2_0_discussion.html

@marcinszkudlinski thanks, but is that outdated - I don't see it mentioning struct sof_audio_buffer at all? So now we have 3 APIs: the "original / stable / legacy" struct comp_buffer, source / sink and the new struct sof_audio_buffer? So would be good to know where we are now - which APIs are used by which components and how many additional copies get done when converting between APIs?

marcinszkudlinski force-pushed the pipeline2_0_002 branch from 9dc5a15 to 502a0ec Compare July 12, 2024 11:54

marcinszkudlinski added the pipeline2.0 label Jul 12, 2024

marcinszkudlinski force-pushed the pipeline2_0_002 branch from 502a0ec to 2d8ee04 Compare August 9, 2024 09:11

marcinszkudlinski marked this pull request as ready for review August 9, 2024 09:12

marcinszkudlinski requested review from marc-hb, ranj063, jxstelter, lgirdwood, plbossart, mmaka1, lbetlej, dbaluta and kv2019i as code owners August 9, 2024 09:12

lgirdwood approved these changes Aug 9, 2024

View reviewed changes

lgirdwood added this to the v2.11 milestone Aug 9, 2024

kv2019i reviewed Aug 14, 2024

View reviewed changes

marcinszkudlinski force-pushed the pipeline2_0_002 branch 4 times, most recently from 1849e86 to c7c2aec Compare August 14, 2024 15:20

marc-hb marked this pull request as draft August 14, 2024 20:41

marc-hb removed their request for review August 14, 2024 20:42

marcinszkudlinski force-pushed the pipeline2_0_002 branch 2 times, most recently from d57c4c8 to 0cbc166 Compare August 20, 2024 11:05

marcinszkudlinski marked this pull request as ready for review August 20, 2024 12:41

kv2019i approved these changes Aug 26, 2024

View reviewed changes

marcinszkudlinski added 6 commits September 2, 2024 14:23

bug: missed fixes when CONFIG_INCOHERENT=0

1999dba

cache operations are pointless when architecture is coherent Signed-off-by: Marcin Szkudlinski <marcin.szkudlinski@intel.com>

buf: rename buffer.c to comp_buffer.c and move to buffers

b787a71

comp_buffer, implemented in buffer.c file, should be kept in the buffers directory, together with all buffers Also it has been renamed to comp_buffer.c Signed-off-by: Marcin Szkudlinski <marcin.szkudlinski@intel.com>

buf: missing dp_queue rename to ring_buffer

04bea35

one variable have been forgotten during renaming dp_queue to ring_buffer Signed-off-by: Marcin Szkudlinski <marcin.szkudlinski@intel.com>

marcinszkudlinski force-pushed the pipeline2_0_002 branch from 0cbc166 to 04bea35 Compare September 2, 2024 12:23

marcinszkudlinski requested review from lyakh, iuliana-prodan and abonislawski as code owners September 2, 2024 12:23

lgirdwood approved these changes Sep 2, 2024

View reviewed changes

lgirdwood merged commit c72778f into thesofproject:main Sep 2, 2024
46 of 47 checks passed

lyakh reviewed Sep 3, 2024

View reviewed changes

marcinszkudlinski deleted the pipeline2_0_002 branch September 4, 2024 06:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pipeline2.0 STEP2 - move hybrid buffering logic from comp_buffer to audio_buffer and partially place comp_buffer on top of audio_buffer #9299

Pipeline2.0 STEP2 - move hybrid buffering logic from comp_buffer to audio_buffer and partially place comp_buffer on top of audio_buffer #9299

marcinszkudlinski commented Jul 12, 2024 •

edited

Loading

lgirdwood left a comment

marcinszkudlinski commented Aug 9, 2024

kv2019i left a comment

marcinszkudlinski commented Aug 14, 2024 •

edited

Loading

marcinszkudlinski commented Aug 20, 2024

marc-hb commented Aug 20, 2024

marc-hb commented Aug 20, 2024 •

edited

Loading

marcinszkudlinski commented Aug 23, 2024

kv2019i commented Aug 26, 2024

lgirdwood commented Sep 2, 2024

marcinszkudlinski commented Sep 2, 2024

lgirdwood Sep 2, 2024

marcinszkudlinski Sep 2, 2024

lyakh Sep 3, 2024 •

edited

Loading

marc-hb commented Sep 3, 2024 •

edited

Loading

marcinszkudlinski commented Sep 4, 2024 •

edited

Loading

lyakh commented Sep 4, 2024

Pipeline2.0 STEP2 - move hybrid buffering logic from comp_buffer to audio_buffer and partially place comp_buffer on top of audio_buffer #9299

Pipeline2.0 STEP2 - move hybrid buffering logic from comp_buffer to audio_buffer and partially place comp_buffer on top of audio_buffer #9299

Conversation

marcinszkudlinski commented Jul 12, 2024 • edited Loading

lgirdwood left a comment

Choose a reason for hiding this comment

marcinszkudlinski commented Aug 9, 2024

kv2019i left a comment

Choose a reason for hiding this comment

marcinszkudlinski commented Aug 14, 2024 • edited Loading

marcinszkudlinski commented Aug 20, 2024

marc-hb commented Aug 20, 2024

marc-hb commented Aug 20, 2024 • edited Loading

marcinszkudlinski commented Aug 23, 2024

kv2019i commented Aug 26, 2024

lgirdwood commented Sep 2, 2024

marcinszkudlinski commented Sep 2, 2024

lgirdwood Sep 2, 2024

Choose a reason for hiding this comment

marcinszkudlinski Sep 2, 2024

Choose a reason for hiding this comment

lyakh Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

marc-hb commented Sep 3, 2024 • edited Loading

marcinszkudlinski commented Sep 4, 2024 • edited Loading

lyakh commented Sep 4, 2024

marcinszkudlinski commented Jul 12, 2024 •

edited

Loading

marcinszkudlinski commented Aug 14, 2024 •

edited

Loading

marc-hb commented Aug 20, 2024 •

edited

Loading

lyakh Sep 3, 2024 •

edited

Loading

marc-hb commented Sep 3, 2024 •

edited

Loading

marcinszkudlinski commented Sep 4, 2024 •

edited

Loading