Dp shadow as a part of comp_buffer / audio_stream #9106

marcinszkudlinski · 2024-05-07T14:46:33Z

This is a follow up to #9063 and a bugfix

The PR is connecting shadow dp_queue tightly with comp_buffer/audio_stream.

it is more natural way, after changes comp_buffer is exposing either its own sink/src API or one provided by dp_queue
race prevention. In internal tests there was a race condition found during frequent binding/unbinding DP to a running pipeline
less code in heavy module_adapter

src/audio/audio_stream.c

src/include/sof/audio/buffer.h

src/audio/module_adapter/module_adapter.c

src/ipc/ipc4/helper.c

as agreed some time ago, parts of structures that should not be modified or accessed directly ("private") should be marked with _ prefix thesofproject#8100 Signed-off-by: Marcin Szkudlinski <marcin.szkudlinski@intel.com>

marcinszkudlinski · 2024-05-08T08:49:56Z

I generally don't agree with "superfluous parentheses", Code "obviously correct" is always better than "correct".
And all ( ) make reading easier, don't matter at all for the compiler
anyway, removed because checkpatch was yelling

lgirdwood · 2024-05-08T10:02:23Z

src/audio/audio_stream.c


 	return audio_stream_get_free_bytes(audio_stream);
 }

 static int audio_stream_get_buffer(struct sof_sink *sink, size_t req_size,
 				   void **data_ptr, void **buffer_start, size_t *buffer_size)
 {
-	struct audio_stream *audio_stream = container_of(sink, struct audio_stream, sink_api);
+	struct audio_stream *audio_stream = container_of(sink, struct audio_stream, _sink_api);


I suppose you could take this 1 step further in the future by having a static inline accessor API that wraps container_of()

src/audio/buffer.c

marcinszkudlinski · 2024-05-09T06:50:12Z

Internal CI failed, checking...

Connection between LL and DP component is implemented bu using double buffering - between comp_buffer and DP component there's additional "shadow" dp_queue Currently shadow buffers are created in module adapter in "prepare" handler and freed in "reset" handler. It may lead to races when there are bind/unbind operation between a components in separate pipelienes, especially when one of the pipelines is already running The commit makes shadow dp_queue to be a part of comp_buffer. When shadow is created, it replaces source or sink interface of audio_stream allowing the module connected to it using all properties of dp_queue, like lockless cross-core connection. Signed-off-by: Marcin Szkudlinski <marcin.szkudlinski@intel.com>

Instead of creating dp_queue in module adapter, use shadow buffering provided by comp_bufer It simplifies the code and makes it more race-resistant Signed-off-by: Marcin Szkudlinski <marcin.szkudlinski@intel.com>

Dp queues are no longer kept as a list of objects, remove unused list maintenance code and data Signed-off-by: Marcin Szkudlinski <marcin.szkudlinski@intel.com>

marcinszkudlinski · 2024-05-09T09:31:32Z

Fixed, there was a crash if a bind module wasn't using a module adapter.

lyakh

just some nit-picking, only if you do end up making another revision. Feel free to ignore otherwise

lyakh · 2024-05-10T07:05:04Z

src/audio/buffer.c

+		 */
+		data_src = &buffer->stream._source_api;
+		data_dst = dp_queue_get_sink(buffer->stream.dp_queue_source);
+


lyakh · 2024-05-10T07:05:25Z

src/audio/buffer.c

+	size_t to_copy = MIN(MIN(data_available, free_size), limit);
+
+	err = source_to_sink_copy(data_src, data_dst, true, to_copy);
+	return err;


return source_to_sink_copy(data_src, data_dst, true, to_copy); and remove err

lyakh · 2024-05-10T07:05:55Z

src/include/sof/audio/audio_stream.h

-	return &audio_stream->_source_api;
-}
+struct sof_source *
+audio_stream_get_source(struct audio_stream *audio_stream);


one line should do, same below

lyakh · 2024-05-10T07:08:11Z

src/include/sof/audio/dp_queue.h

@@ -159,6 +159,8 @@ struct dp_queue *dp_queue_create(size_t min_available, size_t min_free_space, ui
 static inline
 void dp_queue_free(struct dp_queue *dp_queue)
 {
+	if (!dp_queue)
+		return;


in principle it's probably good, since this makes the function compatible with free(), rfree(), etc., but just wondering - can this actually happen? Do we ever have a chance of calling it with NULL?

Yes, it can, very often when changes from this PR are applied:

#if CONFIG_ZEPHYR_DP_SCHEDULER dp_queue_free(buffer->stream.dp_queue_sink); dp_queue_free(buffer->stream.dp_queue_source); #endif /* CONFIG_ZEPHYR_DP_SCHEDULER */

there's no condition if freed pointers are not null, and those will be nulls every time the buffer is not shadowed (so - in most cases). More - there may be only one shadow, so one of those will always be null

marcinszkudlinski · 2024-05-10T08:10:12Z

just some nit-picking, only if you do end up making another revision. Feel free to ignore otherwise

first time in the history there are 100% checks passed, so better don't update ;)

kv2019i

Thanks @marcinszkudlinski for the follow-up. This is clean solution.

marcinszkudlinski force-pushed the dp_shadow_part_of_stream branch from f2a328c to cc4c60c Compare May 7, 2024 15:20

marcinszkudlinski added dp_scheduler bug Something isn't working as expected labels May 7, 2024

lyakh reviewed May 8, 2024

View reviewed changes

marcinszkudlinski force-pushed the dp_shadow_part_of_stream branch from cc4c60c to 255500c Compare May 8, 2024 08:31

mod: mark sink/src api as private in audio_stream

30e065d

as agreed some time ago, parts of structures that should not be modified or accessed directly ("private") should be marked with _ prefix thesofproject#8100 Signed-off-by: Marcin Szkudlinski <marcin.szkudlinski@intel.com>

marcinszkudlinski force-pushed the dp_shadow_part_of_stream branch from 255500c to 6e8cf3a Compare May 8, 2024 08:44

marcinszkudlinski changed the title ~~Dp shadow part of stream~~ Dp shadow as a part of comp_buffer / audio_stream May 8, 2024

marcinszkudlinski marked this pull request as ready for review May 8, 2024 08:45

marcinszkudlinski requested review from ranj063, jxstelter, lgirdwood, plbossart, mmaka1, lbetlej, dbaluta and kv2019i as code owners May 8, 2024 08:45

marcinszkudlinski force-pushed the dp_shadow_part_of_stream branch from 6e8cf3a to f92b378 Compare May 8, 2024 08:58

marcinszkudlinski requested a review from lyakh May 8, 2024 09:32

lgirdwood approved these changes May 8, 2024

View reviewed changes

marcinszkudlinski force-pushed the dp_shadow_part_of_stream branch from f92b378 to ed5f31e Compare May 8, 2024 10:58

marcinszkudlinski added 3 commits May 9, 2024 11:30

dp: use shadow dp_queue from comp_buffer

9bded67

Instead of creating dp_queue in module adapter, use shadow buffering provided by comp_bufer It simplifies the code and makes it more race-resistant Signed-off-by: Marcin Szkudlinski <marcin.szkudlinski@intel.com>

dp: remove unused code from dp_queue

6ca344f

Dp queues are no longer kept as a list of objects, remove unused list maintenance code and data Signed-off-by: Marcin Szkudlinski <marcin.szkudlinski@intel.com>

marcinszkudlinski force-pushed the dp_shadow_part_of_stream branch from ed5f31e to 6ca344f Compare May 9, 2024 09:30

lgirdwood approved these changes May 9, 2024

View reviewed changes

lyakh reviewed May 10, 2024

View reviewed changes

kv2019i approved these changes May 10, 2024

View reviewed changes

kv2019i merged commit 618bdbd into thesofproject:main May 13, 2024
45 checks passed

marcinszkudlinski deleted the dp_shadow_part_of_stream branch May 13, 2024 12:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dp shadow as a part of comp_buffer / audio_stream #9106

Dp shadow as a part of comp_buffer / audio_stream #9106

marcinszkudlinski commented May 7, 2024 •

edited

Loading

marcinszkudlinski commented May 8, 2024

lgirdwood May 8, 2024

marcinszkudlinski commented May 9, 2024

marcinszkudlinski commented May 9, 2024

lyakh left a comment

lyakh May 10, 2024

lyakh May 10, 2024

lyakh May 10, 2024

lyakh May 10, 2024

marcinszkudlinski May 10, 2024 •

edited

Loading

marcinszkudlinski commented May 10, 2024

kv2019i left a comment

Dp shadow as a part of comp_buffer / audio_stream #9106

Dp shadow as a part of comp_buffer / audio_stream #9106

Conversation

marcinszkudlinski commented May 7, 2024 • edited Loading

marcinszkudlinski commented May 8, 2024

lgirdwood May 8, 2024

Choose a reason for hiding this comment

marcinszkudlinski commented May 9, 2024

marcinszkudlinski commented May 9, 2024

lyakh left a comment

Choose a reason for hiding this comment

lyakh May 10, 2024

Choose a reason for hiding this comment

lyakh May 10, 2024

Choose a reason for hiding this comment

lyakh May 10, 2024

Choose a reason for hiding this comment

lyakh May 10, 2024

Choose a reason for hiding this comment

marcinszkudlinski May 10, 2024 • edited Loading

Choose a reason for hiding this comment

marcinszkudlinski commented May 10, 2024

kv2019i left a comment

Choose a reason for hiding this comment

marcinszkudlinski commented May 7, 2024 •

edited

Loading

marcinszkudlinski May 10, 2024 •

edited

Loading