On 4/15/20 12:28 PM, Mark Brown wrote:
On Tue, Apr 14, 2020 at 10:04:37PM -0500, Pierre-Louis Bossart wrote:
Sending as RFC since I don't have a good understanding of the root-cause and for others to confirm my findings. Tested on top of v5.7-rc1.
Hans? Morimoto-san? I'm fine with this as a fix, it's not ideal but I'm guessing anything more substantial is going to be unsuitable for a -rc series.
I looked up the code for about an hour and can't really find a better solution.
My only theory is that the Atom/sst driver directly uses the dai->active state in its .startup and .shutdown DAI callbacks, and will e.g. initialize the DSP with:
static int sst_enable_ssp(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) { int ret = 0;
if (!dai->active) { ret = sst_handle_vb_timer(dai, true); sst_fill_ssp_defaults(dai); } return ret; }
No other Intel driver uses this dai->active status. This is legacy 2013/14 stuff from the phone days, I only have a vague memory of why this was used - everyone else involved at the time moved on, I am probably the last person standing, a scary thought in itself.
Anyways, since this .startup routine is invoked from two locations:
sound/soc/soc-dapm.c: ret = snd_soc_dai_startup(source, substream); sound/soc/soc-dapm.c: ret = snd_soc_dai_startup(sink, substream); sound/soc/soc-pcm.c: ret = snd_soc_dai_startup(dai, substream);
we end-up using three booleans (dai->active, dai->started[0], dai->started[1]) to control what happens in the .startup callback, so we probably end-up in an undefined state.
I don't see an alternative to a revert here, we should probably clarify why we startup the dai in multiple locations and do something better for 5.8.
-Pierre