Skip to content

ASoC: SOF: handle paused streams during system suspend#3119

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

Merged
plbossart merged 3 commits into from
Aug 27, 2021
Merged

ASoC: SOF: handle paused streams during system suspend #3119

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f7f9c3a
ASoC: SOF: pm: fix a stale comment
ranj063 Aug 26, 2021
2ac6502
ASoC: SOF: handle paused streams during system suspend
ranj063 Aug 26, 2021
9e4ce70
ASoC: SOF: hda: reset DAI widget before reconfiguring it
ranj063 Aug 26, 2021
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
7 changes: 7 additions & 0 deletions sound/soc/sof/intel/hda.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -58,6 +58,13 @@ int hda_ctrl_dai_widget_setup(struct snd_soc_dapm_widget *w)
return -EINVAL;

@kv2019ikv2019iAug 27, 2021

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 wording "unusual" is a bit unusual. :) I mean it is allowed and it does happen, so this is fixing a bug. But yeah, I get the point.

}

/* DAI already configured, reset it before reconfiguring it */
if (sof_dai->configured) {
ret = hda_ctrl_dai_widget_free(w);
if (ret < 0)
return ret;
}

config = &sof_dai->dai_config[sof_dai->current_config];

/*
Expand Down
5 changes: 2 additions & 3 deletions sound/soc/sof/pcm.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -100,9 +100,8 @@ void snd_sof_pcm_period_elapsed(struct snd_pcm_substream *substream)
}
EXPORT_SYMBOL(snd_sof_pcm_period_elapsed);

static int sof_pcm_dsp_pcm_free(struct snd_pcm_substream *substream,
struct snd_sof_dev *sdev,
struct snd_sof_pcm *spcm)
int sof_pcm_dsp_pcm_free(struct snd_pcm_substream *substream, struct snd_sof_dev *sdev,
struct snd_sof_pcm *spcm)
{
struct sof_ipc_stream stream;
struct sof_ipc_reply reply;
Expand Down
2 changes: 1 addition & 1 deletion sound/soc/sof/pm.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -191,7 +191,7 @@ static int sof_suspend(struct device *dev, bool runtime_suspend)
if (sdev->fw_state != SOF_FW_BOOT_COMPLETE)
goto suspend;

/* set restore_stream for all streams during system suspend */
/* prepare for streams to be resumed properly upon resume */
Comment thread
ranj063 marked this conversation as resolved.
if (!runtime_suspend) {
ret = sof_set_hw_params_upon_resume(sdev->dev);
if (ret < 0) {
Expand Down
65 changes: 63 additions & 2 deletions sound/soc/sof/sof-audio.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -718,6 +718,55 @@ int sof_set_up_pipelines(struct snd_sof_dev *sdev, bool verify)
return 0;
}

/*
* Free the PCM, its associated widgets and set the prepared flag to false for all PCMs that
* did not get suspended(ex: paused streams) so the widgets can be set up again during resume.
*/
static int sof_tear_down_left_over_pipelines(struct snd_sof_dev *sdev)
{
struct snd_sof_widget *swidget;
struct snd_sof_pcm *spcm;
int dir, ret;

/*
* free all PCMs and their associated DAPM widgets if their connected DAPM widget
* list is not NULL. This should only be true for paused streams at this point.
* This is equivalent to the handling of FE DAI suspend trigger for running streams.
*/
list_for_each_entry(spcm, &sdev->pcm_list, list)
for_each_pcm_streams(dir) {
struct snd_pcm_substream *substream = spcm->stream[dir].substream;

if (!substream || !substream->runtime)
continue;

if (spcm->stream[dir].list) {
ret = sof_pcm_dsp_pcm_free(substream, sdev, spcm);
if (ret < 0)
return ret;

ret = sof_widget_list_free(sdev, spcm, dir);
if (ret < 0) {
dev_err(sdev->dev, "failed to free widgets during suspend\n");
return ret;
}
}
}
Comment thread
plbossart marked this conversation as resolved.

/*
* free any left over DAI widgets. This is equivalent to the handling of suspend trigger
* for the BE DAI for running streams.
*/
list_for_each_entry(swidget, &sdev->widget_list, list)
if (WIDGET_IS_DAI(swidget->id) && swidget->use_count == 1) {
ret = sof_widget_free(sdev, swidget);
if (ret < 0)
return ret;
}

@plbossartplbossartAug 26, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am still unclear on how the tests above apply to the PAUSED case only.

the 'free the paused PCMS and associated DAPM widgets' is at best done with an indirect set of criteria?

@ranj063ranj063Aug 26, 2021•
edited
Loading

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

@plbossart Im not sure Im following your question. But tear_dwn_pipelines() will only be called during system suspend and more specifically when the DSP should be in D3 during system suspend. In this case, all running streams will be suspended. The only spcm's that will have a widget list that is non-empty will be the paused ones. I could use the state check for paused streams but I think its unnecessary. But if you think it makes following the logic easier, I will make the change

@plbossartplbossartAug 26, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would feel more comfortable if we test the state and flag an error if the status is not paused. That said, if this is racy then it'd be a bad idea and we should only add a comment along the lines of what you wrote "The only spcm's that will have a widget list that is non-empty will be the paused ones"

@ranj063ranj063Aug 26, 2021

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

i dont think it is a racy. let me modify it

@ranj063ranj063Aug 26, 2021

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

but the reason I did not do that is because we are manually forcing the actions of the FE suspend and BE suspend on any streams that were left behind. I cant check the state for the for the first loop to walk through the spcms but the second one to free the DAI widgets doesnt have that option.

If it makes it better, I could change the name of the function to not call it out tear_down_paused_pipelines() to tear_down_all_pipelines() or something similar and add in the comment that it will take care of paused pipelines.

@plbossartplbossartAug 26, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

see e.g. soc_pcm_hw_params(), the comment says "This function can also be called multiple times" and internally it calls hw_params for all CPU dais.

@ranj063ranj063Aug 26, 2021•
edited
Loading

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

I was wrong. I was getting confused with the hw_free where we check if the configred flag is set before freeing the widget.

But when the second BE hw_params comes along, it checks if the state of the all connected FE's are not active in

linux/sound/soc/soc-pcm.c

Line 2890 in c4bbda2

intsnd_soc_dpcm_can_be_params(structsnd_soc_pcm_runtime*fe,

I dont see a second BE hw_params in SOF and I'm thinking it is because of this.

@plbossartplbossartAug 26, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There is still a corner case where the application could do multiple hw_params on the two devices, before starting one.
I think it'd need dedicated hacks to test.

But even if that happens, I think the only issue is at the CPU dai level. if we do an internal hw_free to keep the refcounts consistent, regardless of what happens in the ALSA/ASoC core, we should be in good shape. It's done for SoundWire, I am not sure about the other DAIs.

@ranj063ranj063Aug 26, 2021

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

There is still a corner case where the application could do multiple hw_params on the two devices, before starting one.
I think it'd need dedicated hacks to test.

agree. Let me add a fix for this.

@ranj063ranj063Aug 26, 2021

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

just added a patch for this @plbossart


return 0;
}

/*
* For older firmware, this function doesn't free widgets for static pipelines during suspend.
* It only resets use_count for all widgets.
Expand All@@ -732,8 +781,8 @@ int sof_tear_down_pipelines(struct snd_sof_dev *sdev, bool verify)
/*
* This function is called during suspend and for one-time topology verification during
* first boot. In both cases, there is no need to protect swidget->use_count and
* sroute->setup because during suspend all streams are suspended and during topology
* loading the sound card unavailable to open PCMs.
* sroute->setup because during suspend all running streams are suspended and during
* topology loading the sound card unavailable to open PCMs.
*/
list_for_each_entry(swidget, &sdev->widget_list, list) {
if (swidget->dynamic_pipeline_widget)
Expand All@@ -752,6 +801,18 @@ int sof_tear_down_pipelines(struct snd_sof_dev *sdev, bool verify)
return ret;
}

/*
* Tear down all pipelines associated with PCMs that did not get suspended
* and unset the prepare flag so that they can be set up again during resume.
*/
if (!verify) {
ret = sof_tear_down_left_over_pipelines(sdev);
if (ret < 0) {
dev_err(sdev->dev, "failed to tear down paused pipelines\n");
return ret;
}
}

Comment thread
ranj063 marked this conversation as resolved.
list_for_each_entry(sroute, &sdev->route_list, list)
sroute->setup = false;

Expand Down
2 changes: 2 additions & 0 deletions sound/soc/sof/sof-audio.h
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -265,4 +265,6 @@ int sof_widget_free(struct snd_sof_dev *sdev, struct snd_sof_widget *swidget);
/* PCM */
int sof_widget_list_setup(struct snd_sof_dev *sdev, struct snd_sof_pcm *spcm, int dir);
int sof_widget_list_free(struct snd_sof_dev *sdev, struct snd_sof_pcm *spcm, int dir);
int sof_pcm_dsp_pcm_free(struct snd_pcm_substream *substream, struct snd_sof_dev *sdev,
struct snd_sof_pcm *spcm);
#endif