[PATCH 0/3] ASoC: omap-mcbsp: fixes for some underflow problems

[PATCH 0/3] ASoC: omap-mcbsp: fixes for some underflow problems

[Grazvydas Ignotas]

With these series I'm trying to deal with underflow problems I was
complaining about earlier. This mostly tries to get minimum period
size large enough for things to work.

To illustrate the problems I wrote a testcase with alsalib, hopefully it
shows the problem more clearly:
http://notaz.gp2x.de/misc/alsa_period_test.c

Applies on top of mcbsp move series.

Grazvydas Ignotas (3):
  ASoC: omap-mcbsp: fix snd_pcm_hw_rule_add arguments
  ASoC: omap-mcbsp: place size constrain on period, not buffer
  ASoC: omap-mcbsp: make minimum period size larger than FIFO

 sound/soc/omap/omap-mcbsp.c |   31 +++++++++++++++++++++----------
 1 files changed, 21 insertions(+), 10 deletions(-)

[PATCH 1/3] ASoC: omap-mcbsp: fix snd_pcm_hw_rule_add arguments

[Grazvydas Ignotas]

We are setting SNDRV_PCM_HW_PARAM_BUFFER_SIZE based on
SNDRV_PCM_HW_PARAM_CHANNELS, not vice versa. This bug didn't
have much impact because the rules are evaluated multiple times
by the core, and intended value got set eventually.

Signed-off-by: Grazvydas Ignotas <notasas@gmail.com>
---
 sound/soc/omap/omap-mcbsp.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/sound/soc/omap/omap-mcbsp.c b/sound/soc/omap/omap-mcbsp.c
index 10eb645..207365c 100644
--- a/sound/soc/omap/omap-mcbsp.c
+++ b/sound/soc/omap/omap-mcbsp.c
@@ -142,10 +142,10 @@ static int omap_mcbsp_dai_startup(struct snd_pcm_substream *substream,
 		* smaller buffer than the FIFO size to avoid underruns
 		*/
 		snd_pcm_hw_rule_add(substream->runtime, 0,
-				    SNDRV_PCM_HW_PARAM_CHANNELS,
+				    SNDRV_PCM_HW_PARAM_BUFFER_SIZE,
 				    omap_mcbsp_hwrule_min_buffersize,
 				    mcbsp,
-				    SNDRV_PCM_HW_PARAM_BUFFER_SIZE, -1);
+				    SNDRV_PCM_HW_PARAM_CHANNELS, -1);
 
 		/* Make sure, that the period size is always even */
 		snd_pcm_hw_constraint_step(substream->runtime, 0,
-- 
1.7.0.4

Re: [PATCH 1/3] ASoC: omap-mcbsp: fix snd_pcm_hw_rule_add arguments

[Peter Ujfalusi]

On 03/09/2012 01:19 AM, Grazvydas Ignotas wrote:
> We are setting SNDRV_PCM_HW_PARAM_BUFFER_SIZE based on
> SNDRV_PCM_HW_PARAM_CHANNELS, not vice versa.

The intention is to set the buffer size based on the channels here.
This is needed because of the McBSP internal FIFO operation.
It is word based. As example on McBSP2 has 1280 word FIFO.
In mono stream it is 1280 sample long.
In stereo it is 640 (stereo) sample long.
In four channel mode it is 320 (4 channel) sample long.

> This bug didn't
> have much impact because the rules are evaluated multiple times
> by the core, and intended value got set eventually.
> 
> Signed-off-by: Grazvydas Ignotas <notasas@gmail.com>
> ---
>  sound/soc/omap/omap-mcbsp.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/soc/omap/omap-mcbsp.c b/sound/soc/omap/omap-mcbsp.c
> index 10eb645..207365c 100644
> --- a/sound/soc/omap/omap-mcbsp.c
> +++ b/sound/soc/omap/omap-mcbsp.c
> @@ -142,10 +142,10 @@ static int omap_mcbsp_dai_startup(struct snd_pcm_substream *substream,
>  		* smaller buffer than the FIFO size to avoid underruns
>  		*/
>  		snd_pcm_hw_rule_add(substream->runtime, 0,
> -				    SNDRV_PCM_HW_PARAM_CHANNELS,
> +				    SNDRV_PCM_HW_PARAM_BUFFER_SIZE,
>  				    omap_mcbsp_hwrule_min_buffersize,
>  				    mcbsp,
> -				    SNDRV_PCM_HW_PARAM_BUFFER_SIZE, -1);
> +				    SNDRV_PCM_HW_PARAM_CHANNELS, -1);
>  
>  		/* Make sure, that the period size is always even */
>  		snd_pcm_hw_constraint_step(substream->runtime, 0,


-- 
Péter

Re: [PATCH 1/3] ASoC: omap-mcbsp: fix snd_pcm_hw_rule_add arguments

[Grazvydas Ignotas]

On Fri, Mar 9, 2012 at 12:08 PM, Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
> On 03/09/2012 01:19 AM, Grazvydas Ignotas wrote:
>> We are setting SNDRV_PCM_HW_PARAM_BUFFER_SIZE based on
>> SNDRV_PCM_HW_PARAM_CHANNELS, not vice versa.
>
> The intention is to set the buffer size based on the channels here.
> This is needed because of the McBSP internal FIFO operation.
> It is word based. As example on McBSP2 has 1280 word FIFO.
> In mono stream it is 1280 sample long.
> In stereo it is 640 (stereo) sample long.
> In four channel mode it is 320 (4 channel) sample long.

That's all fine, but the argument order is wrong, take a look at
snd_pcm_hw_rule_add definition:

/**
 * snd_pcm_hw_rule_add - add the hw-constraint rule
 * @var: the variable to evaluate
 * @dep: the dependent variables
int snd_pcm_hw_rule_add(struct snd_pcm_runtime *runtime, unsigned int cond,
                        int var,
                        snd_pcm_hw_rule_func_t func, void *private,
                        int dep, ...)

So var should be SNDRV_PCM_HW_PARAM_BUFFER_SIZE, dep
SNDRV_PCM_HW_PARAM_CHANNELS, but it is the opposite now.

>
>> This bug didn't
>> have much impact because the rules are evaluated multiple times
>> by the core, and intended value got set eventually.
>>
>> Signed-off-by: Grazvydas Ignotas <notasas@gmail.com>
>> ---
>>  sound/soc/omap/omap-mcbsp.c |    4 ++--
>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/sound/soc/omap/omap-mcbsp.c b/sound/soc/omap/omap-mcbsp.c
>> index 10eb645..207365c 100644
>> --- a/sound/soc/omap/omap-mcbsp.c
>> +++ b/sound/soc/omap/omap-mcbsp.c
>> @@ -142,10 +142,10 @@ static int omap_mcbsp_dai_startup(struct snd_pcm_substream *substream,
>>               * smaller buffer than the FIFO size to avoid underruns
>>               */
>>               snd_pcm_hw_rule_add(substream->runtime, 0,
>> -                                 SNDRV_PCM_HW_PARAM_CHANNELS,
>> +                                 SNDRV_PCM_HW_PARAM_BUFFER_SIZE,
>>                                   omap_mcbsp_hwrule_min_buffersize,
>>                                   mcbsp,
>> -                                 SNDRV_PCM_HW_PARAM_BUFFER_SIZE, -1);
>> +                                 SNDRV_PCM_HW_PARAM_CHANNELS, -1);
>>
>>               /* Make sure, that the period size is always even */
>>               snd_pcm_hw_constraint_step(substream->runtime, 0,
>

-- 
Gražvydas
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 1/3] ASoC: omap-mcbsp: fix snd_pcm_hw_rule_add arguments

[Peter Ujfalusi]

On 03/09/2012 12:45 PM, Grazvydas Ignotas wrote:
> That's all fine, but the argument order is wrong, take a look at
> snd_pcm_hw_rule_add definition:
> 
> /**
>  * snd_pcm_hw_rule_add - add the hw-constraint rule
>  * @var: the variable to evaluate
>  * @dep: the dependent variables

True.

-- 
Péter

Re: [PATCH 1/3] ASoC: omap-mcbsp: fix snd_pcm_hw_rule_add arguments

[Peter Ujfalusi]

On 03/09/2012 01:19 AM, Grazvydas Ignotas wrote:
> We are setting SNDRV_PCM_HW_PARAM_BUFFER_SIZE based on
> SNDRV_PCM_HW_PARAM_CHANNELS, not vice versa. This bug didn't
> have much impact because the rules are evaluated multiple times
> by the core, and intended value got set eventually.
> 
> Signed-off-by: Grazvydas Ignotas <notasas@gmail.com>

Acked-by: Peter Ujfalusi <peter.ujfalusi@ti.com>

Re: [PATCH 1/3] ASoC: omap-mcbsp: fix snd_pcm_hw_rule_add arguments

[Mark Brown]

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

On Fri, Mar 09, 2012 at 01:19:15AM +0200, Grazvydas Ignotas wrote:
> We are setting SNDRV_PCM_HW_PARAM_BUFFER_SIZE based on
> SNDRV_PCM_HW_PARAM_CHANNELS, not vice versa. This bug didn't
> have much impact because the rules are evaluated multiple times
> by the core, and intended value got set eventually.

Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

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

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

[PATCH 2/3] ASoC: omap-mcbsp: place size constrain on period, not buffer

[Grazvydas Ignotas]

Placing a constraint on buffer size is not enough, programs are
still free to use periods that are smaller than FIFO, which causes
transmit underflows as soon as the stream starts if program operates
in period-sized writes, ending up in endless loop of underruns.

Signed-off-by: Grazvydas Ignotas <notasas@gmail.com>
---
 sound/soc/omap/omap-mcbsp.c |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/sound/soc/omap/omap-mcbsp.c b/sound/soc/omap/omap-mcbsp.c
index 207365c..b9d1272 100644
--- a/sound/soc/omap/omap-mcbsp.c
+++ b/sound/soc/omap/omap-mcbsp.c
@@ -93,11 +93,11 @@ static void omap_mcbsp_set_threshold(struct snd_pcm_substream *substream)
 		omap_mcbsp_set_rx_threshold(mcbsp, words);
 }
 
-static int omap_mcbsp_hwrule_min_buffersize(struct snd_pcm_hw_params *params,
-				    struct snd_pcm_hw_rule *rule)
+static int omap_mcbsp_hwrule_min_period_size(struct snd_pcm_hw_params *params,
+					     struct snd_pcm_hw_rule *rule)
 {
-	struct snd_interval *buffer_size = hw_param_interval(params,
-					SNDRV_PCM_HW_PARAM_BUFFER_SIZE);
+	struct snd_interval *period_size = hw_param_interval(params,
+					SNDRV_PCM_HW_PARAM_PERIOD_SIZE);
 	struct snd_interval *channels = hw_param_interval(params,
 					SNDRV_PCM_HW_PARAM_CHANNELS);
 	struct omap_mcbsp *mcbsp = rule->private;
@@ -109,7 +109,7 @@ static int omap_mcbsp_hwrule_min_buffersize(struct snd_pcm_hw_params *params,
 
 	frames.min = size / channels->min;
 	frames.integer = 1;
-	return snd_interval_refine(buffer_size, &frames);
+	return snd_interval_refine(period_size, &frames);
 }
 
 static int omap_mcbsp_dai_startup(struct snd_pcm_substream *substream,
@@ -138,12 +138,12 @@ static int omap_mcbsp_dai_startup(struct snd_pcm_substream *substream,
 	 */
 	if (mcbsp->pdata->buffer_size) {
 		/*
-		* Rule for the buffer size. We should not allow
-		* smaller buffer than the FIFO size to avoid underruns
+		* Rule for the period size. We should not allow
+		* smaller period than the FIFO size to avoid underruns
 		*/
 		snd_pcm_hw_rule_add(substream->runtime, 0,
-				    SNDRV_PCM_HW_PARAM_BUFFER_SIZE,
-				    omap_mcbsp_hwrule_min_buffersize,
+				    SNDRV_PCM_HW_PARAM_PERIOD_SIZE,
+				    omap_mcbsp_hwrule_min_period_size,
 				    mcbsp,
 				    SNDRV_PCM_HW_PARAM_CHANNELS, -1);
 
-- 
1.7.0.4

Re: [PATCH 2/3] ASoC: omap-mcbsp: place size constrain on period, not buffer

[Mark Brown]

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

On Fri, Mar 09, 2012 at 01:19:16AM +0200, Grazvydas Ignotas wrote:
> Placing a constraint on buffer size is not enough, programs are
> still free to use periods that are smaller than FIFO, which causes
> transmit underflows as soon as the stream starts if program operates
> in period-sized writes, ending up in endless loop of underruns.

Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

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

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

[PATCH 3/3] ASoC: omap-mcbsp: make minimum period size larger than FIFO

[Grazvydas Ignotas]

With a program operating in minimum sized periods, first write
will cause a transfer that will fill mcbsp FIFO quickly, and there
will be no more data to DMA before program manages to do another
write. As the core considers this as underflow condition, we may
get many underflows at the start.
Increase minimum period size by half to deal with this problem.

Signed-off-by: Grazvydas Ignotas <notasas@gmail.com>
---
 sound/soc/omap/omap-mcbsp.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/sound/soc/omap/omap-mcbsp.c b/sound/soc/omap/omap-mcbsp.c
index b9d1272..b373a0b 100644
--- a/sound/soc/omap/omap-mcbsp.c
+++ b/sound/soc/omap/omap-mcbsp.c
@@ -108,6 +108,17 @@ static int omap_mcbsp_hwrule_min_period_size(struct snd_pcm_hw_params *params,
 	size = mcbsp->pdata->buffer_size;
 
 	frames.min = size / channels->min;
+
+	/*
+	 * If period is not larger than FIFO, it may be transfered faster than
+	 * program operating in period sizes is able to send 2 periods, so
+	 * underrun condition can be triggered at the beginning of stream.
+	 * This is because underrun is triggered as soon as DMA has no more
+	 * data to send, and we get this from the start if period is too small.
+	 * To deal with this, set period larger than FIFO size.
+	 */
+	frames.min = frames.min + frames.min / 2;
+
 	frames.integer = 1;
 	return snd_interval_refine(period_size, &frames);
 }
-- 
1.7.0.4

Re: [PATCH 3/3] ASoC: omap-mcbsp: make minimum period size larger than FIFO

[Mark Brown]

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

On Fri, Mar 09, 2012 at 01:19:17AM +0200, Grazvydas Ignotas wrote:

> +	frames.min = frames.min + frames.min / 2;

Call me picky but I'd like some brackets here as I had to think for a
few seconds if that was (frames.min + frames.min) / 2.  But

Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

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

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

Re: [PATCH 3/3] ASoC: omap-mcbsp: make minimum period size larger than FIFO

[Peter Ujfalusi]

On 03/09/2012 01:19 AM, Grazvydas Ignotas wrote:
> With a program operating in minimum sized periods, first write
> will cause a transfer that will fill mcbsp FIFO quickly, and there
> will be no more data to DMA before program manages to do another
> write. As the core considers this as underflow condition, we may
> get many underflows at the start.
> Increase minimum period size by half to deal with this problem.

Not entirely sure about the last two patch...
The implications will be big for existing user space since we are going
to limit the period size in this way (on OMAP3 McBSP2):
48K stereo: 20ms min
44.1K stereo: 21.768ms min
8K stereo: 120ms min
8K mono: 240ms min

As I recall n900 (+ MeeGo) uses 5ms period sizes in call cases on McBSP2.

So I'm a bit reluctant to ack these (it is not an issue on other McBSP
ports, and on OMAP4, but OMAP3 McBSP2 is problematic IMHO).

Jarkko: What do you think?

If this is a real issue in Pandora, could it be solved with adding
/etc/asound.conf to the filesystem, and limit the period size via that?

-- 
Péter

> 
> Signed-off-by: Grazvydas Ignotas <notasas@gmail.com>
> ---
>  sound/soc/omap/omap-mcbsp.c |   11 +++++++++++
>  1 files changed, 11 insertions(+), 0 deletions(-)
> 
> diff --git a/sound/soc/omap/omap-mcbsp.c b/sound/soc/omap/omap-mcbsp.c
> index b9d1272..b373a0b 100644
> --- a/sound/soc/omap/omap-mcbsp.c
> +++ b/sound/soc/omap/omap-mcbsp.c
> @@ -108,6 +108,17 @@ static int omap_mcbsp_hwrule_min_period_size(struct snd_pcm_hw_params *params,
>  	size = mcbsp->pdata->buffer_size;
>  
>  	frames.min = size / channels->min;
> +
> +	/*
> +	 * If period is not larger than FIFO, it may be transfered faster than
> +	 * program operating in period sizes is able to send 2 periods, so
> +	 * underrun condition can be triggered at the beginning of stream.
> +	 * This is because underrun is triggered as soon as DMA has no more
> +	 * data to send, and we get this from the start if period is too small.
> +	 * To deal with this, set period larger than FIFO size.
> +	 */
> +	frames.min = frames.min + frames.min / 2;
> +
>  	frames.integer = 1;
>  	return snd_interval_refine(period_size, &frames);
>  }

Re: [PATCH 3/3] ASoC: omap-mcbsp: make minimum period size larger than FIFO

[Grazvydas Ignotas]

On Fri, Mar 9, 2012 at 2:42 PM, Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
> On 03/09/2012 01:19 AM, Grazvydas Ignotas wrote:
>> With a program operating in minimum sized periods, first write
>> will cause a transfer that will fill mcbsp FIFO quickly, and there
>> will be no more data to DMA before program manages to do another
>> write. As the core considers this as underflow condition, we may
>> get many underflows at the start.
>> Increase minimum period size by half to deal with this problem.
>
> Not entirely sure about the last two patch...
> The implications will be big for existing user space since we are going
> to limit the period size in this way (on OMAP3 McBSP2):
> 48K stereo: 20ms min
> 44.1K stereo: 21.768ms min
> 8K stereo: 120ms min
> 8K mono: 240ms min
>
> As I recall n900 (+ MeeGo) uses 5ms period sizes in call cases on McBSP2.

I would guess they are using something larger, at least larger write
quantities with this kind of period, otherwise it would keep
overflowing there too.

> So I'm a bit reluctant to ack these (it is not an issue on other McBSP
> ports, and on OMAP4, but OMAP3 McBSP2 is problematic IMHO).

Maybe we could use MCBSPLP_THRSH2_REG XTHRESHOLD to artificially limit
MCBSP2 fifo depth?

> Jarkko: What do you think?
>
> If this is a real issue in Pandora, could it be solved with adding
> /etc/asound.conf to the filesystem, and limit the period size via that?

We are already doing that, however we encourage people to install
their own distros on pandora, and now they have to drag whole ALSA
configuration for things to work. IMHO device driver like this must be
able to work without any special userspace configuration.


-- 
Gražvydas
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 3/3] ASoC: omap-mcbsp: make minimum period size larger than FIFO

[Peter Ujfalusi]

On 03/09/2012 03:25 PM, Grazvydas Ignotas wrote:
> I would guess they are using something larger, at least larger write
> quantities with this kind of period, otherwise it would keep
> overflowing there too.

They are using PulseAudio as with Maemmo (the product software).

>> So I'm a bit reluctant to ack these (it is not an issue on other McBSP
>> ports, and on OMAP4, but OMAP3 McBSP2 is problematic IMHO).
> 
> Maybe we could use MCBSPLP_THRSH2_REG XTHRESHOLD to artificially limit
> MCBSP2 fifo depth?

It is not possible. The McBSP FIFO/sDMA implementation (in HW) does not
allow it, unfortunately.
We are using the Threshold settings in dma_op_mode == threshold:
echo threshold > /sys/devices/platform/omap/omap-mcbsp.2/dma_op_mode

It is good for achieving lover power consumption during audio activity.

>> Jarkko: What do you think?
>>
>> If this is a real issue in Pandora, could it be solved with adding
>> /etc/asound.conf to the filesystem, and limit the period size via that?
> 
> We are already doing that, however we encourage people to install
> their own distros on pandora, and now they have to drag whole ALSA
> configuration for things to work. IMHO device driver like this must be
> able to work without any special userspace configuration.

We have two issues here:
if you are using the element mode (dma_op_mode) you might want to have
bigger period size than the FIFO size. However if you are using the
threshold mode you will be fine with a period size between fifo_size/2,
and fifo_size.
In threshold mode we are sending the data to McBSP FIFO in bursts, and
the burst size is the same as the McBSP threshold config. The McBSP will
request new burst when there is free space for the next burst in the
FIFO. So we can fill the FIFO to 3/4 for example, and the next burst
will happen when the FIFO reaches 1/4 fill level.

You might want to try this as well to lover the period_size constraint.

But let's wait for Jarkko's take on this.

-- 
Péter
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 3/3] ASoC: omap-mcbsp: make minimum period size larger than FIFO

[Grazvydas Ignotas]

On Fri, Mar 9, 2012 at 3:42 PM, Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
> On 03/09/2012 03:25 PM, Grazvydas Ignotas wrote:
>> I would guess they are using something larger, at least larger write
>> quantities with this kind of period, otherwise it would keep
>> overflowing there too.
>
> They are using PulseAudio as with Maemmo (the product software).
>
>>> So I'm a bit reluctant to ack these (it is not an issue on other McBSP
>>> ports, and on OMAP4, but OMAP3 McBSP2 is problematic IMHO).
>>
>> Maybe we could use MCBSPLP_THRSH2_REG XTHRESHOLD to artificially limit
>> MCBSP2 fifo depth?
>
> It is not possible. The McBSP FIFO/sDMA implementation (in HW) does not
> allow it, unfortunately.
> We are using the Threshold settings in dma_op_mode == threshold:
> echo threshold > /sys/devices/platform/omap/omap-mcbsp.2/dma_op_mode
>
> It is good for achieving lover power consumption during audio activity.

Maybe enable it by default then?

>>> Jarkko: What do you think?
>>>
>>> If this is a real issue in Pandora, could it be solved with adding
>>> /etc/asound.conf to the filesystem, and limit the period size via that?
>>
>> We are already doing that, however we encourage people to install
>> their own distros on pandora, and now they have to drag whole ALSA
>> configuration for things to work. IMHO device driver like this must be
>> able to work without any special userspace configuration.
>
> We have two issues here:
> if you are using the element mode (dma_op_mode) you might want to have
> bigger period size than the FIFO size. However if you are using the
> threshold mode you will be fine with a period size between fifo_size/2,
> and fifo_size.

Threshold mode doesn't seem to help, I still need to go over FIFO
size. From what I can see in the code it's setting the threshold to
period size, so I still get underflow condition after first write, as
first write goes to FIFO, DMA shows that it has nothing left to send
after first write finishes.

I bet PulseAudio is setting start threshold to something way above
FIFO size, which makes it work here. However you can't expect that
from every program that uses ALSA directly..

> In threshold mode we are sending the data to McBSP FIFO in bursts, and
> the burst size is the same as the McBSP threshold config. The McBSP will
> request new burst when there is free space for the next burst in the
> FIFO. So we can fill the FIFO to 3/4 for example, and the next burst
> will happen when the FIFO reaches 1/4 fill level.

That would work if I did first write larger than period (when it's
below FIFO size) or set large enough start_threshold.
However if you look at alsalib samples, they always write in period
size quantities, so it's not surprising other programs do that too
(alsalib samples work here though as they do set high start
threshold).

>
> You might want to try this as well to lover the period_size constraint.
>
> But let's wait for Jarkko's take on this.
>
> --
> Péter

-- 
Gražvydas
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 3/3] ASoC: omap-mcbsp: make minimum period size larger than FIFO

[Jarkko Nikula]

On Fri, 2012-03-09 at 23:07 +0200, Grazvydas Ignotas wrote:
> On Fri, Mar 9, 2012 at 3:42 PM, Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
> > On 03/09/2012 03:25 PM, Grazvydas Ignotas wrote:
> >>> If this is a real issue in Pandora, could it be solved with adding
> >>> /etc/asound.conf to the filesystem, and limit the period size via that?
> >>
> >> We are already doing that, however we encourage people to install
> >> their own distros on pandora, and now they have to drag whole ALSA
> >> configuration for things to work. IMHO device driver like this must be
> >> able to work without any special userspace configuration.
> >
I agree. What's the use case where the problem shows up? Or actually hw
parameters in that use case so can it be repeated using plain
aplay/arecord?

> > We have two issues here:
> > if you are using the element mode (dma_op_mode) you might want to have
> > bigger period size than the FIFO size. However if you are using the
> > threshold mode you will be fine with a period size between fifo_size/2,
> > and fifo_size.
> 
> Threshold mode doesn't seem to help, I still need to go over FIFO
> size. From what I can see in the code it's setting the threshold to
> period size, so I still get underflow condition after first write, as
> first write goes to FIFO, DMA shows that it has nothing left to send
> after first write finishes.
> 
Period size smaller than FIFO size has used to work in element mode too
but I think I have always used in my tests multiple periods. What I'm
thinking if we have a boundary condition with 2-3 periods and where
buffer size is near FIFO size.

I think instead of limiting minimum period size to FIFO size we should
find that boundary condition. Like if min buffer > FIFO + 1 period or
something like that in order to keep possible to use small periods.

-- 
Jarkko

Re: [PATCH 3/3] ASoC: omap-mcbsp: make minimum period size larger than FIFO

[Grazvydas Ignotas]

On Sun, Mar 11, 2012 at 7:39 PM, Jarkko Nikula <jarkko.nikula@bitmer.com> wrote:
> On Fri, 2012-03-09 at 23:07 +0200, Grazvydas Ignotas wrote:
>> On Fri, Mar 9, 2012 at 3:42 PM, Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
>> > On 03/09/2012 03:25 PM, Grazvydas Ignotas wrote:
>> >>> If this is a real issue in Pandora, could it be solved with adding
>> >>> /etc/asound.conf to the filesystem, and limit the period size via that?
>> >>
>> >> We are already doing that, however we encourage people to install
>> >> their own distros on pandora, and now they have to drag whole ALSA
>> >> configuration for things to work. IMHO device driver like this must be
>> >> able to work without any special userspace configuration.
>> >
> I agree. What's the use case where the problem shows up?

When alsa program doesn't set high enough start_threshold and period
size, or when using OSS with smaller period (start_threshold is period
size there).

> Or actually hw
> parameters in that use case so can it be repeated using plain
> aplay/arecord?

aplay -D hw:0 -f s16_le -c 2 --period-size=512 --start-delay=50000 /dev/urandom

note that this will only show problems on OMAP3 McBSP2 due to it's long FIFO.

>> > We have two issues here:
>> > if you are using the element mode (dma_op_mode) you might want to have
>> > bigger period size than the FIFO size. However if you are using the
>> > threshold mode you will be fine with a period size between fifo_size/2,
>> > and fifo_size.
>>
>> Threshold mode doesn't seem to help, I still need to go over FIFO
>> size. From what I can see in the code it's setting the threshold to
>> period size, so I still get underflow condition after first write, as
>> first write goes to FIFO, DMA shows that it has nothing left to send
>> after first write finishes.
>>
> Period size smaller than FIFO size has used to work in element mode too
> but I think I have always used in my tests multiple periods. What I'm
> thinking if we have a boundary condition with 2-3 periods and where
> buffer size is near FIFO size.

I'm not sure what boundary means in ALSA, but ASoC never touches it or
start_threshold.

> I think instead of limiting minimum period size to FIFO size we should
> find that boundary condition. Like if min buffer > FIFO + 1 period or
> something like that in order to keep possible to use small periods.

I was testing some hacks that force runtime->start_threshold to a bit
above FIFO size with good results, however none of ASoC code ever
touches it, so I guess it could be considered wrong thing to do, but
it works with everything I tested so far, including OSS emulation.


-- 
Gražvydas
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 3/3] ASoC: omap-mcbsp: make minimum period size larger than FIFO

[Mark Brown]

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

On Mon, Mar 12, 2012 at 02:05:55PM +0200, Grazvydas Ignotas wrote:
> On Sun, Mar 11, 2012 at 7:39 PM, Jarkko Nikula <jarkko.nikula@bitmer.com> wrote:

> > I think instead of limiting minimum period size to FIFO size we should
> > find that boundary condition. Like if min buffer > FIFO + 1 period or
> > something like that in order to keep possible to use small periods.

> I was testing some hacks that force runtime->start_threshold to a bit
> above FIFO size with good results, however none of ASoC code ever
> touches it, so I guess it could be considered wrong thing to do, but
> it works with everything I tested so far, including OSS emulation.

That's more due to nothing ever particularly needing it before than due
to it being a bad idea.

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

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

Re: [PATCH 3/3] ASoC: omap-mcbsp: make minimum period size larger than FIFO

[Peter Ujfalusi]

On 03/12/2012 02:05 PM, Grazvydas Ignotas wrote:
> aplay -D hw:0 -f s16_le -c 2 --period-size=512 --start-delay=50000 /dev/urandom
> 
> note that this will only show problems on OMAP3 McBSP2 due to it's long FIFO.

There's something else here since:
aplay -D hw:0 -fs16_le -c2 --period-size=320 --buffer-size=972 \
--start-delay=81000 -v /dev/urandom

Works correctly, however
aplay -D hw:0 -fs16_le -c2 --period-size=324 --buffer-size=972 \
--start-delay=81000 -v /dev/urandom

produces underruns.
In both cases the start_threshold is set to 648 (640 is the FIFO McBSP2
size). Both should be fine, but period_size=324 underruns.

So the issue is not that simple as it seams...

-- 
Péter
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH 3/3] ASoC: omap-mcbsp: make minimum period size larger than FIFO

[Grazvydas Ignotas]

On Mon, Mar 12, 2012 at 2:41 PM, Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
> On 03/12/2012 02:05 PM, Grazvydas Ignotas wrote:
>> aplay -D hw:0 -f s16_le -c 2 --period-size=512 --start-delay=50000 /dev/urandom
>>
>> note that this will only show problems on OMAP3 McBSP2 due to it's long FIFO.
>
> There's something else here since:
> aplay -D hw:0 -fs16_le -c2 --period-size=320 --buffer-size=972 \
> --start-delay=81000 -v /dev/urandom
>
> Works correctly, however
> aplay -D hw:0 -fs16_le -c2 --period-size=324 --buffer-size=972 \
> --start-delay=81000 -v /dev/urandom
>
> produces underruns.
> In both cases the start_threshold is set to 648 (640 is the FIFO McBSP2
> size). Both should be fine, but period_size=324 underruns.
>
> So the issue is not that simple as it seams...

Seems to be affected --stop-delay, increasing it helps here, maybe
core stops it for too long.

-- 
Gražvydas
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel