[PATCH] ASoC: soc-pcm: always cancel any pending delayed stream shutdown

[PATCH] ASoC: soc-pcm: always cancel any pending delayed stream shutdown

[Hector Palacios]

After playback a shutdown is scheduled to happen within 5 seconds.
If another playback takes place before the scheduled work times out,
the work is cancelled, but if a recording takes place, the work will
shutdown in the middle of the recording, causing problems.

To reproduce the issue chain the following commands:
	arecord -M --duration 5 -c 2 /tmp/test.wav;
	aplay /tmp/test.wav;
	sleep 2;
	arecord -M --duration 5 -c 2 /tmp/test.wav;
	aplay /tmp/test.wav;

The fist command records 5 seconds of audio.
The second command plays the recorded audio. At the end of the playback
a work will be scheduled to shutdown in 5 seconds, but only 2 seconds after
the first playback, we run another recording.
When we have recorded 3 seconds, the shutdown will occur which can cause
incorrect behavior and bad recorded data. As a consequence, the last
playback reproduces correct audio for the first 3 seconds and incorrect audio
after that.

Signed-off-by: Hector Palacios <hector.palacios@digi.com>
---
 sound/soc/soc-pcm.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 642c86240752..d19a926872e8 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -760,8 +760,7 @@ static int soc_pcm_prepare(struct snd_pcm_substream *substream)
 	}
 
 	/* cancel any delayed stream shutdown that is pending */
-	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK &&
-	    rtd->pop_wait) {
+	if (rtd->pop_wait) {
 		rtd->pop_wait = 0;
 		cancel_delayed_work(&rtd->delayed_work);
 	}

Re: [PATCH] ASoC: soc-pcm: always cancel any pending delayed stream shutdown

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 616 bytes --]

On Wed, Oct 22, 2014 at 04:14:53PM +0200, Hector Palacios wrote:
> After playback a shutdown is scheduled to happen within 5 seconds.
> If another playback takes place before the scheduled work times out,
> the work is cancelled, but if a recording takes place, the work will
> shutdown in the middle of the recording, causing problems.

You're not saying what the problems are, just that they might be caused.
Starting or stopping a playback should have no impact on a running
capture stream, users could always start and stop streams manually
anyway.  It sounds like you're working around some problem in drivers.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: soc-pcm: always cancel any pending delayed stream shutdown

[Palacios, Hector]

On Wed, 22 Oct 2014 18:28, Mark Brown wrote:
> On Wed, Oct 22, 2014 at 04:14:53PM +0200, Hector Palacios wrote:
>> After playback a shutdown is scheduled to happen within 5 seconds.
>> If another playback takes place before the scheduled work times out, 
>> the work is cancelled, but if a recording takes place, the work will 
>> shutdown in the middle of the recording, causing problems.
> 
> You're not saying what the problems are, just that they might be caused.

Sorry, your right. When I do a recording 2 seconds after a previous playback, and later play the recorded sound I hear 3 seconds of correct audio and then I continue to hear good audio but mixed with noise and at a higher volume, as if the codec had reseted certain parameters to some default values.

> Starting or stopping a playback should have no impact on a running 
> capture stream, users could always start and stop streams manually 
> anyway.  It sounds like you're working around some problem in drivers.

I see. I tested this with Freescale SGTL5000 on an i.MX6 platform. The shutdown of the playback is eventually calling the set_bias_level() hook of the codec to SND_SOC_BIAS_PREPARE and then again to SND_SOC_BIAS_STANDBY which may eventually turn off the regulators. Anyway the problem doesn't seem to be on this codec hook because I set it to NULL and the problem was reproducible. I thought it was the dapm->bias_level being set to STANDBY on snd_soc_dapm_set_bias_level() in the middle of the capture that did something to the workflow of the capture stream, but I'm no expert on the architecture of the sound layer and didn't see any other call to the codec functions that could be causing the issue. 

Thanks

Re: [PATCH] ASoC: soc-pcm: always cancel any pending delayed stream shutdown

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 2221 bytes --]

On Wed, Oct 22, 2014 at 05:25:11PM +0000, Palacios, Hector wrote:

Fix your mailer to word wrap within paragraphs.

> On Wed, 22 Oct 2014 18:28, Mark Brown wrote:
> > On Wed, Oct 22, 2014 at 04:14:53PM +0200, Hector Palacios wrote:
> >> After playback a shutdown is scheduled to happen within 5 seconds.
> >> If another playback takes place before the scheduled work times out, 
> >> the work is cancelled, but if a recording takes place, the work will 
> >> shutdown in the middle of the recording, causing problems.

> > You're not saying what the problems are, just that they might be caused.

> Sorry, your right. When I do a recording 2 seconds after a previous
> playback, and later play the recorded sound I hear 3 seconds of
> correct audio and then I continue to hear good audio but mixed with
> noise and at a higher volume, as if the codec had reseted certain
> parameters to some default values.

That definitely sounds like a driver bug.

> > Starting or stopping a playback should have no impact on a running 
> > capture stream, users could always start and stop streams manually 
> > anyway.  It sounds like you're working around some problem in drivers.

> I see. I tested this with Freescale SGTL5000 on an i.MX6 platform. The
> shutdown of the playback is eventually calling the set_bias_level()
> hook of the codec to SND_SOC_BIAS_PREPARE and then again to
> SND_SOC_BIAS_STANDBY which may eventually turn off the regulators.
> Anyway the problem doesn't seem to be on this codec hook because I set
> it to NULL and the problem was reproducible. I thought it was the
> dapm->bias_level being set to STANDBY on snd_soc_dapm_set_bias_level()
> in the middle of the capture that did something to the workflow of the
> capture stream, but I'm no expert on the architecture of the sound
> layer and didn't see any other call to the codec functions that could
> be causing the issue. 

Check for any interactions with the CODEC register map, and make sure
that you're using the latest version of the driver since it's had quite
a few problems.  If it is something interacting with the device you
should be able to see it if you log all interactions with the device.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: soc-pcm: always cancel any pending delayed stream shutdown

[Fabio Estevam]

Hi Hector,

On Wed, Oct 22, 2014 at 3:25 PM, Palacios, Hector
<Hector.Palacios@digi.com> wrote:

>> You're not saying what the problems are, just that they might be caused.
>
> Sorry, your right. When I do a recording 2 seconds after a previous playback, and later play the recorded sound I hear 3 seconds of correct audio and then I continue to hear good audio but mixed with noise and at a higher volume, as if the codec had reseted certain parameters to some default values.

Are you able to reproduce this problem on 3.18-rc?

Re: [PATCH] ASoC: soc-pcm: always cancel any pending delayed stream shutdown

[Palacios, Hector]

Hi Fabio,

On 10/28/2014 04:23 PM, Fabio Estevam wrote:
> Hi Hector,
>
> On Wed, Oct 22, 2014 at 3:25 PM, Palacios, Hector <Hector.Palacios@digi.com>
> wrote:
>
>>> You're not saying what the problems are, just that they might be caused.
>>
>> Sorry, your right. When I do a recording 2 seconds after a previous playback, and
>> later play the recorded sound I hear 3 seconds of correct audio and then I
>> continue to hear good audio but mixed with noise and at a higher volume, as if
>> the codec had reseted certain parameters to some default values.
>
> Are you able to reproduce this problem on 3.18-rc?

Mark was right and I tracked the problem down to sgtl5000 driver's function 
power_vag_event() which is called by the scheduled work 5 seconds after a playback is 
finished.
Then I saw this was already fixed in commit f091f3f07328f75d20a2a5970d1f8b58d95fc990 
"ASoC: sgtl5000: prevent playback to be muted when terminating concurrent capture".

Thanks
--
Hector Palacios