[RFC TEST] ASoC: soc-dai: revert all changes to DAI startup/shutdown sequence

[RFC TEST] ASoC: soc-dai: revert all changes to DAI startup/shutdown sequence

[Pierre-Louis Bossart]

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>
---

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.

 include/sound/soc-dai.h |  1 -
 sound/soc/soc-dai.c     | 11 ++---------
 2 files changed, 2 insertions(+), 10 deletions(-)

diff --git a/include/sound/soc-dai.h b/include/sound/soc-dai.h
index d4825b82c7a3..b33abe93b905 100644
--- a/include/sound/soc-dai.h
+++ b/include/sound/soc-dai.h
@@ -351,7 +351,6 @@ struct snd_soc_dai {
 
 	/* bit field */
 	unsigned int probed:1;
-	unsigned int started[SNDRV_PCM_STREAM_LAST + 1];
 };
 
 static inline struct snd_soc_pcm_stream *
diff --git a/sound/soc/soc-dai.c b/sound/soc/soc-dai.c
index 8f3cad8db89a..31c41559034b 100644
--- a/sound/soc/soc-dai.c
+++ b/sound/soc/soc-dai.c
@@ -295,24 +295,17 @@ int snd_soc_dai_startup(struct snd_soc_dai *dai,
 {
 	int ret = 0;
 
-	if (!dai->started[substream->stream] &&
-	    dai->driver->ops->startup)
+	if (dai->driver->ops->startup)
 		ret = dai->driver->ops->startup(substream, dai);
 
-	if (ret == 0)
-		dai->started[substream->stream] = 1;
-
 	return ret;
 }
 
 void snd_soc_dai_shutdown(struct snd_soc_dai *dai,
 			 struct snd_pcm_substream *substream)
 {
-	if (dai->started[substream->stream] &&
-	    dai->driver->ops->shutdown)
+	if (dai->driver->ops->shutdown)
 		dai->driver->ops->shutdown(substream, dai);
-
-	dai->started[substream->stream] = 0;
 }
 
 int snd_soc_dai_prepare(struct snd_soc_dai *dai,
-- 
2.20.1

Re: [RFC TEST] ASoC: soc-dai: revert all changes to DAI startup/shutdown sequence

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 370 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC TEST] ASoC: soc-dai: revert all changes to DAI startup/shutdown sequence

[Pierre-Louis Bossart]

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

Re: [RFC TEST] ASoC: soc-dai: revert all changes to DAI startup/shutdown sequence

[Hans de Goede]

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>

Regards,

Hans


> ---
> 
> 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.
> 
>   include/sound/soc-dai.h |  1 -
>   sound/soc/soc-dai.c     | 11 ++---------
>   2 files changed, 2 insertions(+), 10 deletions(-)
> 
> diff --git a/include/sound/soc-dai.h b/include/sound/soc-dai.h
> index d4825b82c7a3..b33abe93b905 100644
> --- a/include/sound/soc-dai.h
> +++ b/include/sound/soc-dai.h
> @@ -351,7 +351,6 @@ struct snd_soc_dai {
>   
>   	/* bit field */
>   	unsigned int probed:1;
> -	unsigned int started[SNDRV_PCM_STREAM_LAST + 1];
>   };
>   
>   static inline struct snd_soc_pcm_stream *
> diff --git a/sound/soc/soc-dai.c b/sound/soc/soc-dai.c
> index 8f3cad8db89a..31c41559034b 100644
> --- a/sound/soc/soc-dai.c
> +++ b/sound/soc/soc-dai.c
> @@ -295,24 +295,17 @@ int snd_soc_dai_startup(struct snd_soc_dai *dai,
>   {
>   	int ret = 0;
>   
> -	if (!dai->started[substream->stream] &&
> -	    dai->driver->ops->startup)
> +	if (dai->driver->ops->startup)
>   		ret = dai->driver->ops->startup(substream, dai);
>   
> -	if (ret == 0)
> -		dai->started[substream->stream] = 1;
> -
>   	return ret;
>   }
>   
>   void snd_soc_dai_shutdown(struct snd_soc_dai *dai,
>   			 struct snd_pcm_substream *substream)
>   {
> -	if (dai->started[substream->stream] &&
> -	    dai->driver->ops->shutdown)
> +	if (dai->driver->ops->shutdown)
>   		dai->driver->ops->shutdown(substream, dai);
> -
> -	dai->started[substream->stream] = 0;
>   }
>   
>   int snd_soc_dai_prepare(struct snd_soc_dai *dai,
> 

Re: [RFC TEST] ASoC: soc-dai: revert all changes to DAI startup/shutdown sequence

[Pierre-Louis Bossart]

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.

Re: [RFC TEST] ASoC: soc-dai: revert all changes to DAI startup/shutdown sequence

[Mark Brown]

On Tue, 14 Apr 2020 22:04:37 -0500, 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
> 
> [...]

Applied, thanks!

[1/1] ASoC: soc-dai: revert all changes to DAI startup/shutdown sequence
      commit: 5bd70440cb0a6f5c6a84019bb2aa93ab8310a5cd

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

Re: [RFC TEST] ASoC: soc-dai: revert all changes to DAI startup/shutdown sequence

[Hans de Goede]

Hi,

On 4/15/20 11:56 PM, Pierre-Louis Bossart wrote:
> 
> 
> 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.

If you can do that, that would be great. Currently the QA model for
new kernels for BYT/CHT seems to be: if there are any regression's
let Hans hit them when he starts running rc1 on his set of test
devices and also let Hans figure out a fix, which is why I'm
grateful that you fixed this, thanks!

Regards,

Hans