Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ASoC: SOF: sof-audio: Modify the order of widget set up #4550

Closed
wants to merge 1 commit into from
Closed

ASoC: SOF: sof-audio: Modify the order of widget set up #4550

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions sound/soc/sof/ipc4-pcm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,12 +332,12 @@ static int sof_ipc4_trigger_pipelines(struct snd_soc_component *component,
* guaranteed for each fork independently.
kv2019i marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

commit message

  • typo on "copieri"
  • Also why do we represent the SSP capture and DMIC capture in different directions? This is confusing to me, not sure if there is any design intent.

Copy link
Member

Choose a reason for hiding this comment

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

it's also not clear to me what happens if there are multiple sink pipelines? what happens if the AEC module is connected to a NS module and a separate PCM on capture. How would the order be modified?

*/
if (state == SOF_IPC4_PIPE_RUNNING || state == SOF_IPC4_PIPE_RESET)
for (i = pipeline_list->count - 1; i >= 0; i--) {
for (i = 0; i < pipeline_list->count; i++) {
spipe = pipeline_list->pipelines[i];
sof_ipc4_add_pipeline_to_trigger_list(sdev, state, spipe, trigger_list);
}
else
for (i = 0; i < pipeline_list->count; i++) {
for (i = pipeline_list->count - 1; i >= 0; i--) {
Copy link
Member

Choose a reason for hiding this comment

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

I am not following why we did all this and why the 'new order' is better.

Surely there was something that led us to go from source to sink initially?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@plbossart I think the key point I missed in the explanation is that the order of widget setup doesnt matter. But what really matters is the order of pipeline trigger and this is determined by the order in which widgets are set up. So when the widgets are set up in the order source->sink, pipelines are added to the trigger list in the same order and we trigger in the reverse order to honor the sink->source requirement with IPC4.

So what happens today is that if you had 2 DAI's for capture (ie DMIC capture and SSP capture for reference as in the example in my commit message), the source to sink widget setup order adds the pipelines in the order 1->2->0.
So triggering in reverse would mean the reference capture pipeline goes first. This fails with an IPC timeout in the firmware. What we really want is for the pipeline 2 to be triggered first.

Copy link
Member

Choose a reason for hiding this comment

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

still struggling to figure out why we didn't use the same order for setup and trigger. Is this because it was less complicated to do so initially to follow the source->sink links?

also isn't the IPC timeout in the firmware an issue? Shouldn't there be some sort of state check to discard IPCs that make no sense?

Copy link
Collaborator Author

@ranj063 ranj063 Aug 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We could have (and should have) used the sink->source for set up earlier but the reason we didnt is likely that we never had a situation where we have multiple sources/sinks in the list of DAPM widgets before and so it was convenient to just to set up in the source->sink order and just pick the reverse order for triggering.

The IPC timeout is absolutely a firmware issue and I am going to debug that as well but I think this PR makes itthe triggering order more logical anyway

Copy link
Member

Choose a reason for hiding this comment

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

I see what you mean now, for the capture part we indeed had a single source split in different streams. The AEC reference coming from a different DAI is a new configuration. Maybe that's something we should add to the nocodec topology by tying one SSP dai pipeline to an AEC pretend module.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, let me add the mock aec module to the DMIC capture path in the nocodec topology to enable testing this PR in CI

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ranj063 This clarification addressed my comment. It's also worth checking whether thesofproject/sof#8084 (just merged) has impact to FW behaviour.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@kv2019i I will check with 8084 once I add the mock aec to nocodec. But this PR is still in the right direction I think.

Copy link

Choose a reason for hiding this comment

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

@kv2019i : this alone did not help.
This PR is very much needed and its critical for the AEC feature to work on the chromebook

spipe = pipeline_list->pipelines[i];
sof_ipc4_add_pipeline_to_trigger_list(sdev, state, spipe, trigger_list);
}
Expand Down
53 changes: 34 additions & 19 deletions sound/soc/sof/sof-audio.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -483,8 +483,8 @@ sof_prepare_widgets_in_path(struct snd_sof_dev *sdev, struct snd_soc_dapm_widget
}

/*
* free all widgets in the sink path starting from the source widget
* (DAI type for capture, AIF type for playback)
* free all widgets in the source path starting from the sink widget
* (AIF type for capture, DAI type for playback)
*/
static int sof_free_widgets_in_path(struct snd_sof_dev *sdev, struct snd_soc_dapm_widget *widget,
int dir, struct snd_sof_pcm *spcm)
Expand All @@ -503,15 +503,15 @@ static int sof_free_widgets_in_path(struct snd_sof_dev *sdev, struct snd_soc_dap
ret = err;
}

/* free all widgets in the sink paths even in case of error to keep use counts balanced */
snd_soc_dapm_widget_for_each_sink_path(widget, p) {
/* free all widgets in the source paths even in case of error to keep use counts balanced */
snd_soc_dapm_widget_for_each_source_path(widget, p) {
if (!p->walking) {
if (!widget_in_list(list, p->sink))
if (!widget_in_list(list, p->source))
continue;

p->walking = true;

err = sof_free_widgets_in_path(sdev, p->sink, dir, spcm);
err = sof_free_widgets_in_path(sdev, p->source, dir, spcm);
if (err < 0)
ret = err;
p->walking = false;
Expand All @@ -522,8 +522,8 @@ static int sof_free_widgets_in_path(struct snd_sof_dev *sdev, struct snd_soc_dap
}

/*
* set up all widgets in the sink path starting from the source widget
* (DAI type for capture, AIF type for playback).
* set up all widgets in the source path starting from the sink widget
* (AIF type for capture, DAI type for playback).
* The error path in this function ensures that all successfully set up widgets getting freed.
*/
static int sof_set_up_widgets_in_path(struct snd_sof_dev *sdev, struct snd_soc_dapm_widget *widget,
Expand All @@ -548,7 +548,7 @@ static int sof_set_up_widgets_in_path(struct snd_sof_dev *sdev, struct snd_soc_d

/* skip populating the pipe_widgets array if it is NULL */
if (!pipeline_list->pipelines)
goto sink_setup;
goto source_setup;

/*
* Add the widget's pipe_widget to the list of pipelines to be triggered if not
Expand All @@ -567,15 +567,15 @@ static int sof_set_up_widgets_in_path(struct snd_sof_dev *sdev, struct snd_soc_d
}
}

sink_setup:
snd_soc_dapm_widget_for_each_sink_path(widget, p) {
source_setup:
snd_soc_dapm_widget_for_each_source_path(widget, p) {
if (!p->walking) {
if (!widget_in_list(list, p->sink))
if (!widget_in_list(list, p->source))
continue;

p->walking = true;

ret = sof_set_up_widgets_in_path(sdev, p->sink, dir, spcm);
ret = sof_set_up_widgets_in_path(sdev, p->source, dir, spcm);
p->walking = false;
if (ret < 0) {
if (swidget)
Expand Down Expand Up @@ -607,13 +607,28 @@ sof_walk_widgets_in_order(struct snd_sof_dev *sdev, struct snd_sof_pcm *spcm,
if (is_virtual_widget(sdev, widget, __func__))
continue;

/* starting widget for playback is AIF type */
if (dir == SNDRV_PCM_STREAM_PLAYBACK && widget->id != snd_soc_dapm_aif_in)
continue;
/*
* set up/free widgets in the order sink->source so that the pipelines will be
* added to the list in the same order as well. All other ops need to follow the
* source->sink order.
*/
if (op == SOF_WIDGET_SETUP || op == SOF_WIDGET_FREE) {
/* starting widget for playback is DAI type */
if (dir == SNDRV_PCM_STREAM_PLAYBACK && widget->id != snd_soc_dapm_dai_in)
continue;

/* starting widget for capture is DAI type */
if (dir == SNDRV_PCM_STREAM_CAPTURE && widget->id != snd_soc_dapm_dai_out)
continue;
/* starting widget for capture is AIF type */
if (dir == SNDRV_PCM_STREAM_CAPTURE && widget->id != snd_soc_dapm_aif_out)
continue;
} else {
/* starting widget for playback is AIF type */
if (dir == SNDRV_PCM_STREAM_PLAYBACK && widget->id != snd_soc_dapm_aif_in)
continue;

/* starting widget for capture is DAI type */
if (dir == SNDRV_PCM_STREAM_CAPTURE && widget->id != snd_soc_dapm_dai_out)
continue;
}

switch (op) {
case SOF_WIDGET_SETUP:
Expand Down