From d28252349b6eb420a22eb4ec25667786b6835d13 Mon Sep 17 00:00:00 2001 From: Andy Ross Date: Fri, 22 Dec 2023 17:11:30 -0800 Subject: [PATCH] audio_stream: Persist requirements and lazy-recalculate alignment As specified, this function was a bit of a booby trap: it has to be called exactly once, after all other setup that modifies frame size has been done. If it is called in the wrong order, or not at all, any consumers of the frame alignment API on this stream will see garbage data in the computed fields. That's way too much complexity for the component code that needs to use this tool, especially as the impact is not in the component itself (e.g. it's a downstream copier widget with SIMD code that fails). Instead, preserve the two requirements set by this function in the audio_stream struct and recompute them any time any of the upstream values change. That allows this to be safely used from any context. There's a mild complexity with audio_sink/source layer, which is written to directly modify the format (it keeps a pointer into audio_stream of its own) without going through the audio_stream API itself. There, the framework gives us "on_audio_format_set()" callbacks on source and sink, so implement it there. This is a partial fix for Issue #8639 Signed-off-by: Andy Ross --- src/audio/audio_stream.c | 35 +++++++++++++++++++++++++--- src/include/sof/audio/audio_stream.h | 12 ++++++---- 2 files changed, 39 insertions(+), 8 deletions(-) diff --git a/src/audio/audio_stream.c b/src/audio/audio_stream.c index 5d23daaf7d6c..8e6528c8cd8d 100644 --- a/src/audio/audio_stream.c +++ b/src/audio/audio_stream.c @@ -79,10 +79,10 @@ static uint32_t audio_stream_frame_align_get(const uint32_t byte_align, } -void audio_stream_init_alignment_constants(const uint32_t byte_align, - const uint32_t frame_align_req, - struct audio_stream *stream) +void audio_stream_recalc_align(struct audio_stream *stream) { + const uint32_t byte_align = stream->byte_align_req; + const uint32_t frame_align_req = stream->frame_align_req; uint32_t process_size; uint32_t frame_size = audio_stream_frame_bytes(stream); @@ -93,6 +93,16 @@ void audio_stream_init_alignment_constants(const uint32_t byte_align, (is_power_of_2(process_size) ? 31 : 32) - clz(process_size); } +void audio_stream_init_alignment_constants(const uint32_t byte_align, + const uint32_t frame_align_req, + struct audio_stream *stream) +{ + stream->byte_align_req = byte_align; + stream->frame_align_req = frame_align_req; + audio_stream_recalc_align(stream); +} + + static int audio_stream_release_data(struct sof_source *source, size_t free_size) { struct audio_stream *audio_stream = container_of(source, struct audio_stream, source_api); @@ -145,11 +155,28 @@ static int audio_stream_sink_set_alignment_constants(struct sof_sink *sink, return 0; } +static int source_format_set(struct sof_source *source) +{ + struct audio_stream *s = container_of(source, struct audio_stream, source_api); + + audio_stream_recalc_align(s); + return 0; +} + +static int sink_format_set(struct sof_sink *sink) +{ + struct audio_stream *s = container_of(sink, struct audio_stream, sink_api); + + audio_stream_recalc_align(s); + return 0; +} + static const struct source_ops audio_stream_source_ops = { .get_data_available = audio_stream_get_data_available, .get_data = audio_stream_get_data, .release_data = audio_stream_release_data, .audio_set_ipc_params = audio_stream_set_ipc_params_source, + .on_audio_format_set = source_format_set, .set_alignment_constants = audio_stream_source_set_alignment_constants }; @@ -158,6 +185,7 @@ static const struct sink_ops audio_stream_sink_ops = { .get_buffer = audio_stream_get_buffer, .commit_buffer = audio_stream_commit_buffer, .audio_set_ipc_params = audio_stream_set_ipc_params_sink, + .on_audio_format_set = sink_format_set, .set_alignment_constants = audio_stream_sink_set_alignment_constants }; @@ -167,6 +195,7 @@ void audio_stream_init(struct audio_stream *audio_stream, void *buff_addr, uint3 audio_stream->addr = buff_addr; audio_stream->end_addr = (char *)audio_stream->addr + size; + audio_stream_init_alignment_constants(1, 1, audio_stream); source_init(audio_stream_get_source(audio_stream), &audio_stream_source_ops, &audio_stream->runtime_stream_params); sink_init(audio_stream_get_sink(audio_stream), &audio_stream_sink_ops, diff --git a/src/include/sof/audio/audio_stream.h b/src/include/sof/audio/audio_stream.h index 6e7c89b2fe08..1a0181c5d659 100644 --- a/src/include/sof/audio/audio_stream.h +++ b/src/include/sof/audio/audio_stream.h @@ -61,11 +61,15 @@ struct audio_stream { void *r_ptr; /**< Buffer read position */ void *addr; /**< Buffer base address */ void *end_addr; /**< Buffer end address */ + uint8_t byte_align_req; + uint8_t frame_align_req; /* runtime stream params */ struct sof_audio_stream_params runtime_stream_params; }; +void audio_stream_recalc_align(struct audio_stream *stream); + static inline void *audio_stream_get_rptr(const struct audio_stream *buf) { return buf->r_ptr; @@ -175,6 +179,7 @@ static inline void audio_stream_set_frm_fmt(struct audio_stream *buf, enum sof_ipc_frame val) { buf->runtime_stream_params.frame_fmt = val; + audio_stream_recalc_align(buf); } static inline void audio_stream_set_valid_fmt(struct audio_stream *buf, @@ -191,6 +196,7 @@ static inline void audio_stream_set_rate(struct audio_stream *buf, uint32_t val) static inline void audio_stream_set_channels(struct audio_stream *buf, uint16_t val) { buf->runtime_stream_params.channels = val; + audio_stream_recalc_align(buf); } static inline void audio_stream_set_underrun(struct audio_stream *buf, @@ -363,11 +369,7 @@ static inline int audio_stream_set_params(struct audio_stream *buffer, buffer->runtime_stream_params.rate = params->rate; buffer->runtime_stream_params.channels = params->channels; - /* set the default alignment info. - * set byte_align as 1 means no alignment limit on byte. - * set frame_align as 1 means no alignment limit on frame. - */ - audio_stream_init_alignment_constants(1, 1, buffer); + audio_stream_recalc_align(buffer); return 0; }