[RFC v2 PATCH 1/1] ASoC: soc-core: symmetry checking for each DAIs separately

[RFC v2 PATCH 1/1] ASoC: soc-core: symmetry checking for each DAIs separately

[Dong Aisheng]

The orginal code does not cover the case that one DAI such as codec
may be shared between other two DAIs(CPU).
When do symmetry checking, altough the codec DAI requires symmetry,
the two CPU DAIs may still be configured to run on different rates.

We change to check each DAI's state separately instead of only checking
the dai link to prevent this issue.

Signed-off-by: Dong Aisheng <b29396@freescale.com>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
Cc: Liam Girdwood <lrg@ti.com>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Wolfram Sang <w.sang@pengutronix.de>
Cc: Lars-Peter Clausen <lars@metafoo.de>
---
Change since v1:
 Address the comments from Lars-Peter Clausen.
 * add the dai as an parameter for soc_pcm_apply_symmetry
 Thanks for the suggestion.

---
 include/sound/soc-dai.h |    3 +++
 include/sound/soc.h     |    2 --
 sound/soc/soc-pcm.c     |   40 +++++++++++++++++++++++++---------------
 3 files changed, 28 insertions(+), 17 deletions(-)

diff --git a/include/sound/soc-dai.h b/include/sound/soc-dai.h
index 5ad5f3a..12d98b4 100644
--- a/include/sound/soc-dai.h
+++ b/include/sound/soc-dai.h
@@ -242,6 +242,9 @@ struct snd_soc_dai {
 	void *playback_dma_data;
 	void *capture_dma_data;
 
+	/* Symmetry data - only valid if symmetry is being enforced */
+	unsigned int rate;
+
 	/* parent platform/codec */
 	union {
 		struct snd_soc_platform *platform;
diff --git a/include/sound/soc.h b/include/sound/soc.h
index 3fe658e..5449139 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -849,8 +849,6 @@ struct snd_soc_pcm_runtime  {
 	unsigned int complete:1;
 	unsigned int dev_registered:1;
 
-	/* Symmetry data - only valid if symmetry is being enforced */
-	unsigned int rate;
 	long pmdown_time;
 
 	/* runtime devices */
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 1aee9fc..8eb0f07 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -27,15 +27,13 @@
 #include <sound/soc.h>
 #include <sound/initval.h>
 
-static int soc_pcm_apply_symmetry(struct snd_pcm_substream *substream)
+static int soc_pcm_apply_symmetry(struct snd_pcm_substream *substream,
+					struct snd_soc_dai *soc_dai)
 {
 	struct snd_soc_pcm_runtime *rtd = substream->private_data;
-	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
-	struct snd_soc_dai *codec_dai = rtd->codec_dai;
 	int ret;
 
-	if (!codec_dai->driver->symmetric_rates &&
-	    !cpu_dai->driver->symmetric_rates &&
+	if (!soc_dai->driver->symmetric_rates &&
 	    !rtd->dai_link->symmetric_rates)
 		return 0;
 
@@ -43,19 +41,19 @@ static int soc_pcm_apply_symmetry(struct snd_pcm_substream *substream)
 	 * the second can need to get its constraints before the first has
 	 * picked a rate.  Complain and allow the application to carry on.
 	 */
-	if (!rtd->rate) {
-		dev_warn(&rtd->dev,
+	if (!soc_dai->rate) {
+		dev_warn(soc_dai->dev,
 			 "Not enforcing symmetric_rates due to race\n");
 		return 0;
 	}
 
-	dev_dbg(&rtd->dev, "Symmetry forces %dHz rate\n", rtd->rate);
+	dev_dbg(soc_dai->dev, "Symmetry forces %dHz rate\n", soc_dai->rate);
 
 	ret = snd_pcm_hw_constraint_minmax(substream->runtime,
 					   SNDRV_PCM_HW_PARAM_RATE,
-					   rtd->rate, rtd->rate);
+					   soc_dai->rate, soc_dai->rate);
 	if (ret < 0) {
-		dev_err(&rtd->dev,
+		dev_err(soc_dai->dev,
 			"Unable to apply rate symmetry constraint: %d\n", ret);
 		return ret;
 	}
@@ -185,8 +183,14 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
 	}
 
 	/* 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 (cpu_dai->active) {
+		ret = soc_pcm_apply_symmetry(substream, cpu_dai);
+		if (ret != 0)
+			goto config_err;
+	}
+
+	if (codec_dai->active) {
+		ret = soc_pcm_apply_symmetry(substream, codec_dai);
 		if (ret != 0)
 			goto config_err;
 	}
@@ -288,8 +292,12 @@ static int soc_pcm_close(struct snd_pcm_substream *substream)
 	codec_dai->active--;
 	codec->active--;
 
-	if (!cpu_dai->active && !codec_dai->active)
-		rtd->rate = 0;
+	/* clear the corresponding DAIs rate when inactive */
+	if (!cpu_dai->active)
+		cpu_dai->rate = 0;
+
+	if (!codec_dai->active)
+		codec_dai->rate = 0;
 
 	/* Muting the DAC suppresses artifacts caused during digital
 	 * shutdown, for example from stopping clocks.
@@ -447,7 +455,9 @@ static int soc_pcm_hw_params(struct snd_pcm_substream *substream,
 		}
 	}
 
-	rtd->rate = params_rate(params);
+	/* store the rate for each DAIs */
+	cpu_dai->rate = params_rate(params);
+	codec_dai->rate = params_rate(params);
 
 out:
 	mutex_unlock(&rtd->pcm_mutex);
-- 
1.7.0.4

[alsa-devel] [RFC v2 PATCH 1/1] ASoC: soc-core: symmetry checking for each DAIs separately

[Tabi Timur-B04825]

On Mon, Aug 29, 2011 at 4:15 AM, Dong Aisheng <b29396@freescale.com> wrote:
> The orginal code does not cover the case that one DAI such as codec
> may be shared between other two DAIs(CPU).

Can you give me an example of how this can occur?

-- 
Timur Tabi
Linux kernel developer at Freescale

[alsa-devel] [RFC v2 PATCH 1/1] ASoC: soc-core: symmetry checking for each DAIs separately

[Dong Aisheng-B29396]

> -----Original Message-----
> From: Tabi Timur-B04825
> Sent: Tuesday, August 30, 2011 4:35 AM
> To: Dong Aisheng-B29396
> Cc: alsa-devel at alsa-project.org; lars at metafoo.de; s.hauer at pengutronix.de;
> broonie at opensource.wolfsonmicro.com; w.sang at pengutronix.de; lrg at ti.com;
> linux-arm-kernel at lists.infradead.org
> Subject: Re: [alsa-devel] [RFC v2 PATCH 1/1] ASoC: soc-core: symmetry
> checking for each DAIs separately
> 
> On Mon, Aug 29, 2011 at 4:15 AM, Dong Aisheng <b29396@freescale.com>
> wrote:
> > The orginal code does not cover the case that one DAI such as codec
> > may be shared between other two DAIs(CPU).
> 
> Can you give me an example of how this can occur?
> 

Pls check the following case used in mx28evk.
static struct snd_soc_dai_link mxs_sgtl5000_dai[] = {
        {
                .name           = "HiFi Tx",
                .stream_name    = "HiFi Playback",
                .codec_dai_name = "sgtl5000",
                .codec_name     = "sgtl5000.0-000a",
                .cpu_dai_name   = "mxs-saif.0",
                .platform_name  = "mxs-pcm-audio.0",
                .ops            = &mxs_sgtl5000_hifi_ops,
        }, {
                .name           = "HiFi Rx",
                .stream_name    = "HiFi Capture",
                .codec_dai_name = "sgtl5000",
                .codec_name     = "sgtl5000.0-000a",
                .cpu_dai_name   = "mxs-saif.1",
                .platform_name  = "mxs-pcm-audio.1",
                .ops            = &mxs_sgtl5000_hifi_ops,
        },
};
You can also refer to:
http://thread.gmane.org/gmane.linux.ports.arm.kernel/129789/focus=129839

Regards
Dong Aisheng

[alsa-devel] [RFC v2 PATCH 1/1] ASoC: soc-core: symmetry checking for each DAIs separately

[Liam Girdwood]

On 30/08/11 03:54, Dong Aisheng-B29396 wrote:
>> -----Original Message-----
>> From: Tabi Timur-B04825
>> Sent: Tuesday, August 30, 2011 4:35 AM
>> To: Dong Aisheng-B29396
>> Cc: alsa-devel at alsa-project.org; lars at metafoo.de; s.hauer at pengutronix.de;
>> broonie at opensource.wolfsonmicro.com; w.sang at pengutronix.de; lrg at ti.com;
>> linux-arm-kernel at lists.infradead.org
>> Subject: Re: [alsa-devel] [RFC v2 PATCH 1/1] ASoC: soc-core: symmetry
>> checking for each DAIs separately
>>
>> On Mon, Aug 29, 2011 at 4:15 AM, Dong Aisheng <b29396@freescale.com>
>> wrote:
>>> The orginal code does not cover the case that one DAI such as codec
>>> may be shared between other two DAIs(CPU).
>>
>> Can you give me an example of how this can occur?
>>
> 
> Pls check the following case used in mx28evk.
> static struct snd_soc_dai_link mxs_sgtl5000_dai[] = {
>         {
>                 .name           = "HiFi Tx",
>                 .stream_name    = "HiFi Playback",
>                 .codec_dai_name = "sgtl5000",
>                 .codec_name     = "sgtl5000.0-000a",
>                 .cpu_dai_name   = "mxs-saif.0",
>                 .platform_name  = "mxs-pcm-audio.0",
>                 .ops            = &mxs_sgtl5000_hifi_ops,
>         }, {
>                 .name           = "HiFi Rx",
>                 .stream_name    = "HiFi Capture",
>                 .codec_dai_name = "sgtl5000",
>                 .codec_name     = "sgtl5000.0-000a",
>                 .cpu_dai_name   = "mxs-saif.1",
>                 .platform_name  = "mxs-pcm-audio.1",
>                 .ops            = &mxs_sgtl5000_hifi_ops,
>         },
> };
> You can also refer to:
> http://thread.gmane.org/gmane.linux.ports.arm.kernel/129789/focus=129839
> 

Timur, iirc your driver has some special symmetry requirement (maybe something todo with having 2 DAIs) ?

I assume this is OK for you too ?

Acked-by: Liam Girdwood <lrg@ti.com>

Liam 

[alsa-devel] [RFC v2 PATCH 1/1] ASoC: soc-core: symmetry checking for each DAIs separately

[Tabi Timur-B04825]

Liam Girdwood wrote:
> Timur, iirc your driver has some special symmetry requirement (maybe something todo with having 2 DAIs) ?
>
> I assume this is OK for you too ?

Yes, that's why I was asking.  I believe I'm affected by this patch as 
well, so I will test it.  However, I never noticed a problem before, so 
I'm not sure what's going on.

-- 
Timur Tabi
Linux kernel developer at Freescale

[alsa-devel] [RFC v2 PATCH 1/1] ASoC: soc-core: symmetry checking for each DAIs separately

[Lars-Peter Clausen]

On 08/31/2011 01:55 PM, Tabi Timur-B04825 wrote:
> Liam Girdwood wrote:
>> Timur, iirc your driver has some special symmetry requirement (maybe something todo with having 2 DAIs) ?
>>
>> I assume this is OK for you too ?
> 
> Yes, that's why I was asking.  I believe I'm affected by this patch as 
> well, so I will test it.  However, I never noticed a problem before, so 
> I'm not sure what's going on.
> 

The problem is that if the DAI is shared between two links the symmetry
constraint is not applied and both links could be configured to run at
different rates.

Without the patch for example 'aplay -r 8000 ... & arecrod -r 16000 ...' would
result in playback running twice as fast as it is supposed to.

[alsa-devel] [RFC v2 PATCH 1/1] ASoC: soc-core: symmetry checking for each DAIs separately

[Timur Tabi]

Lars-Peter Clausen wrote:
> Without the patch for example 'aplay -r 8000 ... & arecrod -r 16000 ...' would
> result in playback running twice as fast as it is supposed to.

I'm not sure I can reproduce the problem.  Executing the above command either
plays audio at the proper rate (with or without your patch), doesn't play
anything at all (it just sits there), or gives me this error message:

soc-audio soc-audio: set sample size in capture stream first

(sometimes it says "playback" instead of "capture").

This message is generated by this code in fsl_ssi.c:

/* This is the second stream open, so we need to impose sample
 * rate and maybe sample size constraints.  Note that this can
 * cause a race condition if the second stream is opened before
 * the first stream is fully initialized.
 *
 * We provide some protection by checking to make sure the first
 * stream is initialized, but it's not perfect.  ALSA sometimes
 * re-initializes the driver with a different sample rate or
 * size.  If the second stream is opened before the first stream
 * has received its final parameters, then the second stream may
 * be constrained to the wrong sample rate or size.
 *
 * FIXME: This code does not handle opening and closing streams
 * repeatedly.  If you open two streams and then close the first
 * one, you may not be able to open another stream until you
 * close the second one as well.
 */
struct snd_pcm_runtime *first_runtime =
	ssi_private->first_stream->runtime;

if (!first_runtime->sample_bits) {
	dev_err(substream->pcm->card->dev,
		"set sample size in %s stream first\n",
		substream->stream == SNDRV_PCM_STREAM_PLAYBACK
		? "capture" : "playback");
	return -EAGAIN;
}

I don't think I've ever actually tested this code, and it's pretty old, so I
don't even know if the comment is actually valid any more.  I think the FIXME is
actually fixed, but I've forgotten the mental exercise that prompted me to
create this code in the first place.

-- 
Timur Tabi
Linux kernel developer at Freescale

[alsa-devel] [RFC v2 PATCH 1/1] ASoC: soc-core: symmetry checking for each DAIs separately

[Lars-Peter Clausen]

On 09/01/2011 08:58 PM, Timur Tabi wrote:
> Lars-Peter Clausen wrote:
>> Without the patch for example 'aplay -r 8000 ... & arecrod -r 16000 ...' would
>> result in playback running twice as fast as it is supposed to.
> 
> I'm not sure I can reproduce the problem.  Executing the above command either
> plays audio at the proper rate (with or without your patch), doesn't play
> anything at all (it just sits there), or gives me this error message:
> 
> soc-audio soc-audio: set sample size in capture stream first
> 
> (sometimes it says "playback" instead of "capture").

Try to add a small delay between the two commands. e.g. 'aplay -r 8000 ... &
sleep 1; arecrod -r 16000 ...'. And of course the DAIs need to be configured to
be asynchronous, otherwise you'll of course catch the error in the fsl_ssi driver.

[alsa-devel] [RFC v2 PATCH 1/1] ASoC: soc-core: symmetry checking for each DAIs separately

[Timur Tabi]

Lars-Peter Clausen wrote:
> Try to add a small delay between the two commands. e.g. 'aplay -r 8000 ... &
> sleep 1; arecrod -r 16000 ...'. And of course the DAIs need to be configured to
> be asynchronous, otherwise you'll of course catch the error in the fsl_ssi driver.

Ok, I'm an idiot.  I just noticed that I don't support different sample rates
for playback and capture in my driver, because the CS4270 doesn't support it.
The P1022DS uses the WM8776, which does support it, but I never updated the driver.

Another oddity is that I have this code:

/* Are the RX and the TX clocks locked? */
if (of_find_property(np, "fsl,ssi-asynchronous", NULL))
	ssi_private->asynchronous = 1;
else
	ssi_private->cpu_dai_drv.symmetric_rates = 1;


and later I do this:

if (!ssi_private->asynchronous) {
	snd_pcm_hw_constraint_minmax(substream->runtime,
		SNDRV_PCM_HW_PARAM_SAMPLE_BITS,
		first_runtime->sample_bits,
		first_runtime->sample_bits);

So it looks like I'm confusing sample rate locking with sample size locking.  Ugh.

Anyway, I'm not in any position at the moment to verify this patch.  I need to
get my driver working with asynchronous sample rates first.

-- 
Timur Tabi
Linux kernel developer at Freescale

[RFC v2 PATCH 1/1] ASoC: soc-core: symmetry checking for each DAIs separately

[Wolfram Sang]

On Mon, Aug 29, 2011 at 05:15:14PM +0800, Dong Aisheng wrote:
> The orginal code does not cover the case that one DAI such as codec
> may be shared between other two DAIs(CPU).
> When do symmetry checking, altough the codec DAI requires symmetry,
> the two CPU DAIs may still be configured to run on different rates.
> 
> We change to check each DAI's state separately instead of only checking
> the dai link to prevent this issue.
> 
> Signed-off-by: Dong Aisheng <b29396@freescale.com>

Removes the race warning for me:

Tested-by: Wolfram Sang <w.sang@pengutronix.de>

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110902/a8f9e3d3/attachment.sig>

[alsa-devel] [RFC v2 PATCH 1/1] ASoC: soc-core: symmetry checking for each DAIs separately

[Dong Aisheng]

Hi Mark,

2011/8/31 Liam Girdwood <lrg@ti.com>:
> On 30/08/11 03:54, Dong Aisheng-B29396 wrote:
>>> -----Original Message-----
>>> From: Tabi Timur-B04825
>>> Sent: Tuesday, August 30, 2011 4:35 AM
>>> To: Dong Aisheng-B29396
>>> Cc: alsa-devel at alsa-project.org; lars at metafoo.de; s.hauer at pengutronix.de;
>>> broonie at opensource.wolfsonmicro.com; w.sang at pengutronix.de; lrg at ti.com;
>>> linux-arm-kernel at lists.infradead.org
>>> Subject: Re: [alsa-devel] [RFC v2 PATCH 1/1] ASoC: soc-core: symmetry
>>> checking for each DAIs separately
>>>
>>> On Mon, Aug 29, 2011 at 4:15 AM, Dong Aisheng <b29396@freescale.com>
>>> wrote:
>>>> The orginal code does not cover the case that one DAI such as codec
>>>> may be shared between other two DAIs(CPU).
>>>
>>> Can you give me an example of how this can occur?
>>>
>>
>> Pls check the following case used in mx28evk.
>> static struct snd_soc_dai_link mxs_sgtl5000_dai[] = {
>> ? ? ? ? {
>> ? ? ? ? ? ? ? ? .name ? ? ? ? ? = "HiFi Tx",
>> ? ? ? ? ? ? ? ? .stream_name ? ?= "HiFi Playback",
>> ? ? ? ? ? ? ? ? .codec_dai_name = "sgtl5000",
>> ? ? ? ? ? ? ? ? .codec_name ? ? = "sgtl5000.0-000a",
>> ? ? ? ? ? ? ? ? .cpu_dai_name ? = "mxs-saif.0",
>> ? ? ? ? ? ? ? ? .platform_name ?= "mxs-pcm-audio.0",
>> ? ? ? ? ? ? ? ? .ops ? ? ? ? ? ?= &mxs_sgtl5000_hifi_ops,
>> ? ? ? ? }, {
>> ? ? ? ? ? ? ? ? .name ? ? ? ? ? = "HiFi Rx",
>> ? ? ? ? ? ? ? ? .stream_name ? ?= "HiFi Capture",
>> ? ? ? ? ? ? ? ? .codec_dai_name = "sgtl5000",
>> ? ? ? ? ? ? ? ? .codec_name ? ? = "sgtl5000.0-000a",
>> ? ? ? ? ? ? ? ? .cpu_dai_name ? = "mxs-saif.1",
>> ? ? ? ? ? ? ? ? .platform_name ?= "mxs-pcm-audio.1",
>> ? ? ? ? ? ? ? ? .ops ? ? ? ? ? ?= &mxs_sgtl5000_hifi_ops,
>> ? ? ? ? },
>> };
>> You can also refer to:
>> http://thread.gmane.org/gmane.linux.ports.arm.kernel/129789/focus=129839
>>
>
> Timur, iirc your driver has some special symmetry requirement (maybe something todo with having 2 DAIs) ?
>
> I assume this is OK for you too ?
>
> Acked-by: Liam Girdwood <lrg@ti.com>
>
Can you apply this?

Regards
Dong Aisheng

[RFC v2 PATCH 1/1] ASoC: soc-core: symmetry checking for each DAIs separately

[Mark Brown]

On Mon, Aug 29, 2011 at 05:15:14PM +0800, Dong Aisheng wrote:

> -static int soc_pcm_apply_symmetry(struct snd_pcm_substream *substream)
> +static int soc_pcm_apply_symmetry(struct snd_pcm_substream *substream,
> +					struct snd_soc_dai *soc_dai)

Applied, thanks.  But plain dai would have been a better name here.