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

On 4/15/20 4:26 PM, Hans de Goede wrote:

Hi,

On 4/15/20 5:04 AM, Pierre-Louis Bossart wrote:

...

On Baytrail/Cherrytrail, the Atom/SST driver fails miserably:

[    9.741953] intel_sst_acpi 80860F28:00: FW Version 01.0c.00.01 [    9.832992] intel_sst_acpi 80860F28:00: FW sent error response 0x40034 [    9.833019] intel_sst_acpi 80860F28:00: FW alloc failed ret -4 [    9.833028] intel_sst_acpi 80860F28:00: sst_get_stream returned err -5 [    9.833033] sst-mfld-platform sst-mfld-platform: ASoC: DAI prepare error: -5 [    9.833037]  Baytrail Audio Port: ASoC: prepare FE Baytrail Audio Port failed [    9.853942] intel_sst_acpi 80860F28:00: FW sent error response 0x40034 [    9.853974] intel_sst_acpi 80860F28:00: FW alloc failed ret -4 [    9.853984] intel_sst_acpi 80860F28:00: sst_get_stream returned err -5 [    9.853990] sst-mfld-platform sst-mfld-platform: ASoC: DAI prepare error: -5 [    9.853994]  Baytrail Audio Port: ASoC: prepare FE Baytrail Audio Port failed

Commit b56be800f1292 ("ASoC: soc-pcm: call snd_soc_dai_startup()/shutdown() once") was the initial problematic commit.

Commit 1ba616bd1a6d5e ("ASoC: soc-dai: fix DAI startup/shutdown sequence") was an attempt to fix things but it does not work on Baytrail, reverting all changes seems necessary for now.

Fixes: 1ba616bd1a6d5e ("ASoC: soc-dai: fix DAI startup/shutdown sequence") Signed-off-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com

Thank you for figuring this out!

I've tested this on the 2 devices where I have seen the problem (the only 2 devices on which I've tested 5.7-rc1 so far):

One Cherry Trail device with a RT5645 codec and another Cherry Trail device with an ES8316 and I can confirm that this fixes the issue on both devices:

Tested-by: Hans de Goede hdegoede@redhat.com

Thanks Hans for checking.

I must admit it was one of the more complicated bisects I've ever done, we had 3 different regressions so I end-up merging sound-v5.7-rc1 on top of v5.7-rc1, then do a manual rebase to create a linear branch, then squash fixes with the original problematic commits, and then bisecting once I had a single issue left.

I'll see if we can retask some of the SOF CI Baytrail/Cherrytrail machines to run regressions on the legacy driver on a periodic basis, e.g. during week-ends when no one is around.