[RFC][PATCH] ASoC: Move symmetric rate check for substreams to hw_params

[RFC][PATCH] ASoC: Move symmetric rate check for substreams to hw_params

[Vaibhav Bedia]

Currently the symmetric rate check is present in the snd_pcm_open() call.
However this causes snd_pcm_open() to fail for the 2nd substream is the
2nd call to snd_pcm_open() is made before the 1st substream has set a non-zero
rate by calling snd_pcm_hw_params().

Fix this by moving the call to snd_pcm_apply_symmetry() to soc_pcm_hw_params().

Signed-off-by: Afzal Mohammed <afzal@ti.com>
Signed-off-by: Vaibhav Bedia <vaibhav.bedia@ti.com>
---

This patch is one way of fixing the issues reported here
http://comments.gmane.org/gmane.linux.alsa.devel/75489 and
http://thread.gmane.org/gmane.linux.alsa.devel/81055/focus=21653

 sound/soc/soc-core.c |   25 +++++++++++++++++--------
 1 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 64befac..edec9af 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -638,13 +638,6 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
 		goto config_err;
 	}
 
-	/* Symmetry only applies if we've already got an active stream. */
-	if (cpu_dai->active || codec_dai->active) {
-		ret = soc_pcm_apply_symmetry(substream);
-		if (ret != 0)
-			goto config_err;
-	}
-
 	pr_debug("asoc: %s <-> %s info:\n",
 			codec_dai->name, cpu_dai->name);
 	pr_debug("asoc: rate mask 0x%x\n", runtime->hw.rates);
@@ -742,6 +735,9 @@ static int soc_codec_close(struct snd_pcm_substream *substream)
 	codec_dai->active--;
 	codec->active--;
 
+	if(!codec_dai->active && !cpu_dai->active)
+		rtd->rate = 0;
+
 	/* Muting the DAC suppresses artifacts caused during digital
 	 * shutdown, for example from stopping clocks.
 	 */
@@ -898,12 +894,25 @@ static int soc_pcm_hw_params(struct snd_pcm_substream *substream,
 		}
 	}
 
-	rtd->rate = params_rate(params);
+	/* Symmetry only applies if we've already got an active stream,
+	   a non zero rtd->rate implies that stream was already active and
+	   rtd->rate has been set previously */
+	if (rtd->rate) {
+		ret = soc_pcm_apply_symmetry(substream);
+		if (ret < 0)
+			goto symmetry_err;
+	} else {
+		rtd->rate = params_rate(params);
+	}
 
 out:
 	mutex_unlock(&pcm_mutex);
 	return ret;
 
+symmetry_err:
+	if(platform->driver->ops->hw_free)
+		platform->driver->ops->hw_free(substream);
+
 platform_err:
 	if (cpu_dai->driver->ops->hw_free)
 		cpu_dai->driver->ops->hw_free(substream, cpu_dai);
-- 
1.6.2.4

Re: [RFC][PATCH] ASoC: Move symmetric rate check for substreams to hw_params

[Mark Brown]

On Mon, Mar 07, 2011 at 05:26:03PM +0530, Vaibhav Bedia wrote:
> Currently the symmetric rate check is present in the snd_pcm_open() call.
> However this causes snd_pcm_open() to fail for the 2nd substream is the
> 2nd call to snd_pcm_open() is made before the 1st substream has set a non-zero
> rate by calling snd_pcm_hw_params().
> 
> Fix this by moving the call to snd_pcm_apply_symmetry() to soc_pcm_hw_params().

This means that applications won't be constrained by the current
settings so can't automatically choose the required rate.  ALSA is just
fundamentally racy here, if you want to reliably open bidirectional
streams you need to serialise the startup of the two directions.

Re: [RFC][PATCH] ASoC: Move symmetric rate check for substreams to hw_params

[Bedia, Vaibhav]

Hi Mark,

On Monday, March 07, 2011 5:39 PM, Mark Brown wrote:
> On Mon, Mar 07, 2011 at 05:26:03PM +0530, Vaibhav Bedia wrote:
>> Currently the symmetric rate check is present in the
>> snd_pcm_open() call. However this causes snd_pcm_open() to
>> fail for the 2nd substream is 
>> the 2nd call to snd_pcm_open() is made before the 1st
>> substream has 
>> set a non-zero rate by calling snd_pcm_hw_params().
>> 
>> Fix this by moving the call to snd_pcm_apply_symmetry() to
>> soc_pcm_hw_params(). 
> 
> This means that applications won't be constrained by the
> current settings so can't automatically choose the required
> rate.  ALSA is just fundamentally racy here, if you want to
> reliably open bidirectional streams you need to serialise the
> startup of the two directions.    

When a custom application is written then the race condition can be avoided. 

However right now a simple test like "arecord -f dat | aplay -f dat" fails. One way to make it work
is to make sure that a call to arecord/aplay (or some dummy app which just sets some valid rate and returns) 
has been made before both are invoked simultaneously.

Regards,
Vaibhav

Re: [RFC][PATCH] ASoC: Move symmetric rate check for substreams to hw_params

[Mark Brown]

On Mon, Mar 07, 2011 at 06:21:32PM +0530, Bedia, Vaibhav wrote:
> On Monday, March 07, 2011 5:39 PM, Mark Brown wrote:

> > This means that applications won't be constrained by the
> > current settings so can't automatically choose the required
> > rate.  ALSA is just fundamentally racy here, if you want to
> > reliably open bidirectional streams you need to serialise the
> > startup of the two directions.    

> When a custom application is written then the race condition can be avoided. 

> However right now a simple test like "arecord -f dat | aplay -f dat" fails. One way to make it work
> is to make sure that a call to arecord/aplay (or some dummy app which just sets some valid rate and returns) 
> has been made before both are invoked simultaneously.

For command line testing you can do something like:

  arecord -f dat | (sleep 1; aplay -f dat -) 

to insert a delay if required.  Obviously not all systems will hit the
race.