ASoC: SOF: handle paused streams during system suspend #3119
Conversation
ranj063
force-pushed
the
fix/pause_resume
branch
4 times, most recently
from
dcb8591 to
542ebe6
Compare
There is no restore_stream flag anymmore. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
ranj063
force-pushed
the
fix/pause_resume
branch
from
542ebe6 to
fe024e2
Compare
sound/soc/sof/sof-audio.c
Outdated
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@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
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
i dont think it is a racy. let me modify it
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Line 2890 in c4bbda2
I dont see a second BE hw_params in SOF and I'm thinking it is because of this.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
During system suspend, paused streams do not get suspended. Therefore, we need to explicitly free these PCMs in the DSP and free the associated DAPM widgets so that they can be set up again during resume. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
ranj063
force-pushed
the
fix/pause_resume
branch
from
fe024e2 to
2ac6502
Compare
It is not unusual for ALSA/ASoC hw_params callbacks to be invoked multiple times. Reset and free the DAI widget before reconfiguring it to keep the DAI widget use_count balanced. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
kv2019i
left a comment
kv2019i
left a comment
There was a problem hiding this comment.
Thanks. I do wonder whether this should be fixed in ASoC core as well. Whole point of paused state is that some resources/state is kept (otherwise you would just stop), so ignoring this at suspend, just seems like a gap. Otoh, given current interfaces, this fix seems both needed and correct, so approving.
|
|
||
| if (!sof_dai || !sof_dai->dai_config) { | ||
| dev_err(sdev->dev, "No config for DAI %s\n", w->name); | ||
| return -EINVAL; |
There was a problem hiding this comment.
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.
5 participants
During system suspend, paused streams do not get suspended.
Therefore, we need to explicitly free these PCMs in the DSP
and free the associated DAPM widgets so that they can be set
up again during resume.
Fixes #3116