Skip to content

Fix handling of the mixer-module adapter combo on IPC3 platforms - #7357

Merged
dbaluta merged 2 commits into
thesofproject:mainfrom
paulstelian97:mixer_module_fixes
Apr 4, 2023
Merged

dbaluta merged 2 commits into
thesofproject:mainfrom
paulstelian97:mixer_module_fixes

Conversation

@paulstelian97

Copy link
Copy Markdown
Collaborator

audio: mixer: Improve handling of inactive input streams

This helps the mixer dynamically add/remove input streams when needed. Without this commit I would have the issue where the mixer would read from inactive inputs and outside the used portion of the corresponding input buffers.

audio: module_adapter: Ensure prepare always runs on modules

This commit makes sure I can actually start two streams with a delay on different mixer inputs (as opposed to in quick succession, as if given via the & symbol in a shell)

CC: @dbaluta

Comment thread src/audio/mixer/mixer.c Outdated
Comment thread src/audio/mixer/mixer.c Outdated
Comment thread src/audio/mixer/mixer.c Outdated

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps I should use active_input_buffers here and below?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need j anyway to have the exact active buffers, so you can continue using j here and below

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, wouldn't j==active_input_buffers ? If j can be differnent, then I think it deserves a longer more descriptive name as it's used across this long function (i.e. not just a loop index variable anymore).

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They can differ if there's a race changing which buffers are active between the first check and this loop. I can write an assert stating that they don't differ I suppose.

@paulstelian97 paulstelian97 changed the title [RFC] Fix handling of the mixer-module adapter combo on IPC3 platforms Fix handling of the mixer-module adapter combo on IPC3 platforms Mar 31, 2023
@paulstelian97
paulstelian97 marked this pull request as ready for review March 31, 2023 11:10
Paul Olaru added 2 commits March 31, 2023 14:15
The old code does not deal correctly with input streams going away
without everything stopping. This commit ensures only active streams are
considered as mixer input, and the other streams are ignored (considered
zeroed out).

Signed-off-by: Paul Olaru <paul.olaru@nxp.com>
Some components like mixer have special handling of the prepare function
whenever they are already active. Ensure said handling has a chance to
run.

With the old code, that code doesn't run, mixer doesn't have a chance to
return PPL_STATUS_PATH_STOP and the DAI then receives a second prepare
request, which is invalid given that it's already active.

Signed-off-by: Paul Olaru <paul.olaru@nxp.com>
@dbaluta

dbaluta commented Apr 3, 2023

Copy link
Copy Markdown
Collaborator

@lyakh @andrula-song @ranj063 can you please have a look?

After integration with module_adapter, mixer was mostly broken for IPC3 platforms. This patch fixes it, with still some issues to address in next patches.

@kv2019i kv2019i left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! One minor readibility/style comment, but nothing to preventing merge.

Comment thread src/audio/mixer/mixer.c Outdated

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, wouldn't j==active_input_buffers ? If j can be differnent, then I think it deserves a longer more descriptive name as it's used across this long function (i.e. not just a loop index variable anymore).

@dbaluta
dbaluta merged commit 5f26af9 into thesofproject:main Apr 4, 2023

@lyakh lyakh left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The second patch in this PR is touching some very central code and not only for IPC3

Comment thread src/audio/module_adapter/module_adapter.c
buffer_release(sink_c);

/* Prepare module */
ret = module_prepare(mod);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we sure, that no module is currently relying on mod->period_bytes being assigned before .prepare()?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants