[PATCH 0/3] ASoC: omap-mcbsp: Constraint handling changes

[PATCH 0/3] ASoC: omap-mcbsp: Constraint handling changes

[Peter Ujfalusi]

Hello,

This series will add the following changes:
1. We do not need to place buffer size constraint in case of a capture stream.

2. Grazvydas Ignotas <notasas@gmail.com> reported that on Pandora they have
issues with legacy applications (and OSS emulation) when they open the playback
with small period size (smaller than the FIFO size). Since these legacy
applications can not be fixed (and it is not feasible), this series will provide
a solution to overcome with the underruns at the stream start.

With the introduction of the new sysfs file (period_protection) the user can
request the driver to place the constraint to the period size instead of the
buffer size.
With the period_protection one can specify the number of samples they want to
have as protection in period size constraint over the McBSP FIFO size.

As an example:
OMAP3, McBSP2, stereo stream.
the FIFO is 640 samples long.
period_protection = 1 will ensure that the period size is minimum of 641 samples
long.

With the test code from Grazvydas Ignotas:
http://notaz.gp2x.de/misc/alsa_period_test.c

I don't see underrun issues with period_protection = 1 on BeagleBoard.

Regards,
Peter
---
Peter Ujfalusi (3):
  ASoC: omap-mcbsp: buffer size constraint only applies to playback
    stream
  ASoC: omap-mcbsp: Restructure omap_mcbsp_dai_startup code
  ASoC: omap-mcbsp: Add period size protection mode

 sound/soc/omap/mcbsp.c      |    2 +
 sound/soc/omap/mcbsp.h      |    1 +
 sound/soc/omap/omap-mcbsp.c |   71 ++++++++++++++++++++++++++++++++++--------
 3 files changed, 60 insertions(+), 14 deletions(-)

-- 
1.7.8.5

[PATCH 1/3] ASoC: omap-mcbsp: buffer size constraint only applies to playback stream

[Peter Ujfalusi]

In capture stream the buffer size does not need to be constrained to be
bigger then the McBSP FIFO.
In capture the FIFO content is taken out in period length burst, this
enusres that the FIFO is not going to overflow.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 sound/soc/omap/omap-mcbsp.c |   14 ++++++++------
 1 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/sound/soc/omap/omap-mcbsp.c b/sound/soc/omap/omap-mcbsp.c
index 27144a6..1046083 100644
--- a/sound/soc/omap/omap-mcbsp.c
+++ b/sound/soc/omap/omap-mcbsp.c
@@ -138,13 +138,15 @@ 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
+		* smaller buffer than the FIFO size to avoid underruns.
+		* This applies only for the playback stream.
 		*/
-		snd_pcm_hw_rule_add(substream->runtime, 0,
-				    SNDRV_PCM_HW_PARAM_BUFFER_SIZE,
-				    omap_mcbsp_hwrule_min_buffersize,
-				    mcbsp,
-				    SNDRV_PCM_HW_PARAM_CHANNELS, -1);
+		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+			snd_pcm_hw_rule_add(substream->runtime, 0,
+					    SNDRV_PCM_HW_PARAM_BUFFER_SIZE,
+					    omap_mcbsp_hwrule_min_buffersize,
+					    mcbsp,
+					    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.8.5

[PATCH 2/3] ASoC: omap-mcbsp: Restructure omap_mcbsp_dai_startup code

[Peter Ujfalusi]

To facilitate the period_protection feature coming up.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 sound/soc/omap/omap-mcbsp.c |   34 +++++++++++++++++-----------------
 1 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/sound/soc/omap/omap-mcbsp.c b/sound/soc/omap/omap-mcbsp.c
index 1046083..f6e56ec 100644
--- a/sound/soc/omap/omap-mcbsp.c
+++ b/sound/soc/omap/omap-mcbsp.c
@@ -120,6 +120,9 @@ static int omap_mcbsp_dai_startup(struct snd_pcm_substream *substream,
 	if (!cpu_dai->active)
 		err = omap_mcbsp_request(mcbsp);
 
+	if (!mcbsp->pdata->buffer_size)
+		return err;
+
 	/*
 	 * OMAP3 McBSP FIFO is word structured.
 	 * McBSP2 has 1024 + 256 = 1280 word long buffer,
@@ -134,24 +137,21 @@ static int omap_mcbsp_dai_startup(struct snd_pcm_substream *substream,
 	 * 1 channel (mono): size is 128 frames (128 words)
 	 * 2 channels (stereo): size is 128 / 2 = 64 frames (2 * 64 words)
 	 * 4 channels: size is 128 / 4 = 32 frames (4 * 32 words)
+	 *
+	 * Rule for the buffer size. We should not allow
+	 * smaller buffer than the FIFO size to avoid underruns.
+	 * This applies only for the playback stream.
 	 */
-	if (mcbsp->pdata->buffer_size) {
-		/*
-		* Rule for the buffer size. We should not allow
-		* smaller buffer than the FIFO size to avoid underruns.
-		* This applies only for the playback stream.
-		*/
-		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
-			snd_pcm_hw_rule_add(substream->runtime, 0,
-					    SNDRV_PCM_HW_PARAM_BUFFER_SIZE,
-					    omap_mcbsp_hwrule_min_buffersize,
-					    mcbsp,
-					    SNDRV_PCM_HW_PARAM_CHANNELS, -1);
-
-		/* Make sure, that the period size is always even */
-		snd_pcm_hw_constraint_step(substream->runtime, 0,
-					   SNDRV_PCM_HW_PARAM_PERIOD_SIZE, 2);
-	}
+	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+		snd_pcm_hw_rule_add(substream->runtime, 0,
+					SNDRV_PCM_HW_PARAM_BUFFER_SIZE,
+					omap_mcbsp_hwrule_min_buffersize,
+					mcbsp,
+					SNDRV_PCM_HW_PARAM_CHANNELS, -1);
+
+	/* Make sure, that the period size is always even */
+	snd_pcm_hw_constraint_step(substream->runtime, 0,
+					SNDRV_PCM_HW_PARAM_PERIOD_SIZE, 2);
 
 	return err;
 }
-- 
1.7.8.5

[PATCH 3/3] ASoC: omap-mcbsp: Add period size protection mode

[Peter Ujfalusi]

Certain application can experience underrun right after the playback start.
This is caused by the McBSP FIFO/sDMA integration:
The sDMA will push samples to the FIFO till it has threshold amount of free
slots available in the FIFO. If the application picks period size which is
smaller than the FIFO size, and it did not prepared multiple periods, or
it did not set the start_threshold for the stream to cover the FIFO size
the hw pointer will move forward, which is causing the underrun.

Add a sysfs entry for McBSP ports: period_protection.
If this property is set the driver will place the constraint agains the
period size, and not for the buffer size. To ensure that we do not hit
underrun, the period size constraint will be increased with the requested
number of frames (the period size will be FIFO size + period_protection).

As default the period_protection is disabled.

Reported-by: Grazvydas Ignotas <notasas@gmail.com>
Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 sound/soc/omap/mcbsp.c      |    2 +
 sound/soc/omap/mcbsp.h      |    1 +
 sound/soc/omap/omap-mcbsp.c |   59 ++++++++++++++++++++++++++++++++++++------
 3 files changed, 53 insertions(+), 9 deletions(-)

diff --git a/sound/soc/omap/mcbsp.c b/sound/soc/omap/mcbsp.c
index 34835e8..7d35327 100644
--- a/sound/soc/omap/mcbsp.c
+++ b/sound/soc/omap/mcbsp.c
@@ -806,6 +806,7 @@ static DEVICE_ATTR(prop, 0644, prop##_show, prop##_store);
 
 THRESHOLD_PROP_BUILDER(max_tx_thres);
 THRESHOLD_PROP_BUILDER(max_rx_thres);
+THRESHOLD_PROP_BUILDER(period_protection);
 
 static const char *dma_op_modes[] = {
 	"element", "threshold",
@@ -866,6 +867,7 @@ static const struct attribute *additional_attrs[] = {
 	&dev_attr_max_tx_thres.attr,
 	&dev_attr_max_rx_thres.attr,
 	&dev_attr_dma_op_mode.attr,
+	&dev_attr_period_protection.attr,
 	NULL,
 };
 
diff --git a/sound/soc/omap/mcbsp.h b/sound/soc/omap/mcbsp.h
index 262a615..97ba0a1 100644
--- a/sound/soc/omap/mcbsp.h
+++ b/sound/soc/omap/mcbsp.h
@@ -310,6 +310,7 @@ struct omap_mcbsp {
 	int dma_op_mode;
 	u16 max_tx_thres;
 	u16 max_rx_thres;
+	int period_protection;
 	void *reg_cache;
 	int reg_cache_size;
 
diff --git a/sound/soc/omap/omap-mcbsp.c b/sound/soc/omap/omap-mcbsp.c
index f6e56ec..ef47117 100644
--- a/sound/soc/omap/omap-mcbsp.c
+++ b/sound/soc/omap/omap-mcbsp.c
@@ -111,6 +111,30 @@ static int omap_mcbsp_hwrule_min_buffersize(struct snd_pcm_hw_params *params,
 	return snd_interval_refine(buffer_size, &frames);
 }
 
+static int omap_mcbsp_hwrule_min_periodsize(struct snd_pcm_hw_params *params,
+				    struct snd_pcm_hw_rule *rule)
+{
+	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;
+	struct snd_interval frames;
+	int size;
+
+	snd_interval_any(&frames);
+	size = mcbsp->pdata->buffer_size;
+
+	frames.min = size / channels->min;
+	/*
+	 * Enlarge the period size with the requested period protection number
+	 * of samples to ensure we are not going to underrun.
+	 */
+	frames.min += mcbsp->period_protection;
+	frames.integer = 1;
+	return snd_interval_refine(period_size, &frames);
+}
+
 static int omap_mcbsp_dai_startup(struct snd_pcm_substream *substream,
 				  struct snd_soc_dai *cpu_dai)
 {
@@ -138,16 +162,33 @@ static int omap_mcbsp_dai_startup(struct snd_pcm_substream *substream,
 	 * 2 channels (stereo): size is 128 / 2 = 64 frames (2 * 64 words)
 	 * 4 channels: size is 128 / 4 = 32 frames (4 * 32 words)
 	 *
-	 * Rule for the buffer size. We should not allow
-	 * smaller buffer than the FIFO size to avoid underruns.
-	 * This applies only for the playback stream.
+	 * We need to protect the stream against underrun. This only applies to
+	 * playback stream since the capture direction operates differently,
+	 * which provides this protection.
 	 */
-	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
-		snd_pcm_hw_rule_add(substream->runtime, 0,
-					SNDRV_PCM_HW_PARAM_BUFFER_SIZE,
-					omap_mcbsp_hwrule_min_buffersize,
-					mcbsp,
-					SNDRV_PCM_HW_PARAM_CHANNELS, -1);
+	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+		if (mcbsp->period_protection) {
+			/*
+			 * 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_PERIOD_SIZE,
+					    omap_mcbsp_hwrule_min_periodsize,
+					    mcbsp,
+					    SNDRV_PCM_HW_PARAM_CHANNELS, -1);
+		} else {
+			/*
+			 * Rule for the buffer size. We should not allow
+			 * smaller buffer 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,
+					    mcbsp,
+					    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.8.5

Re: [PATCH 1/3] ASoC: omap-mcbsp: buffer size constraint only applies to playback stream

[Mark Brown]

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

On Tue, Mar 20, 2012 at 01:13:39PM +0200, Peter Ujfalusi wrote:
> In capture stream the buffer size does not need to be constrained to be
> bigger then the McBSP FIFO.
> In capture the FIFO content is taken out in period length burst, this
> enusres that the FIFO is not going to overflow.

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

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

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

Re: [PATCH 2/3] ASoC: omap-mcbsp: Restructure omap_mcbsp_dai_startup code

[Mark Brown]

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

On Tue, Mar 20, 2012 at 01:13:40PM +0200, Peter Ujfalusi wrote:
> To facilitate the period_protection feature coming up.

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: Add period size protection mode

[Mark Brown]

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

On Tue, Mar 20, 2012 at 01:13:41PM +0200, Peter Ujfalusi wrote:
> Certain application can experience underrun right after the playback start.
> This is caused by the McBSP FIFO/sDMA integration:
> The sDMA will push samples to the FIFO till it has threshold amount of free
> slots available in the FIFO. If the application picks period size which is
> smaller than the FIFO size, and it did not prepared multiple periods, or
> it did not set the start_threshold for the stream to cover the FIFO size
> the hw pointer will move forward, which is causing the underrun.

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

though this should probably have the note about working around broken
applications from the cover letter in the changelog as with the
changelog alone it's really not apparent why we're doing this here as a
driver specific thing.

[-- 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: Add period size protection mode

[Grazvydas Ignotas]

On Tue, Mar 20, 2012 at 6:01 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Tue, Mar 20, 2012 at 01:13:41PM +0200, Peter Ujfalusi wrote:
>> Certain application can experience underrun right after the playback start.
>> This is caused by the McBSP FIFO/sDMA integration:
>> The sDMA will push samples to the FIFO till it has threshold amount of free
>> slots available in the FIFO. If the application picks period size which is
>> smaller than the FIFO size, and it did not prepared multiple periods, or
>> it did not set the start_threshold for the stream to cover the FIFO size
>> the hw pointer will move forward, which is causing the underrun.
>
> Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
>
> though this should probably have the note about working around broken
> applications from the cover letter in the changelog as with the
> changelog alone it's really not apparent why we're doing this here as a
> driver specific thing.

I wouldn't really call them broken, it's enough to set period size to
512 with smaller start_threshold (something like 50ms) to have
problems, those parameters are perfectly valid for a program trying to
achieve low latency.

It's a shame this still won't work out-of-the box, but at least there
will be some solution.

-- 
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: Add period size protection mode

[Jarkko Nikula]

On 03/20/2012 01:13 PM, Peter Ujfalusi wrote:
> Certain application can experience underrun right after the playback start.
> This is caused by the McBSP FIFO/sDMA integration:
> The sDMA will push samples to the FIFO till it has threshold amount of free
> slots available in the FIFO. If the application picks period size which is
> smaller than the FIFO size, and it did not prepared multiple periods, or
> it did not set the start_threshold for the stream to cover the FIFO size
> the hw pointer will move forward, which is causing the underrun.
> 
> Add a sysfs entry for McBSP ports: period_protection.
> If this property is set the driver will place the constraint agains the
> period size, and not for the buffer size. To ensure that we do not hit
> underrun, the period size constraint will be increased with the requested
> number of frames (the period size will be FIFO size + period_protection).
> 
> As default the period_protection is disabled.
> 
I don't think this is going to solve the actual problem here where
custom asound.conf was required. IMHO custom sysfs is even worse option
and very hard to remove afterwards. And defaulting this new setting for
Pandora might break e.g. MER or MeeGo on N900.

I didn't check this but would it be possible to either put restriction
to start-delay (I think Grazvydas said he has experimental code for
that?) or make sure that minimum buffer size must be higher than FIFO +
1 period (or something like that)?

-- 
Jarkko

Re: [PATCH 1/3] ASoC: omap-mcbsp: buffer size constraint only applies to playback stream

[Jarkko Nikula]

On 03/20/2012 05:26 PM, Mark Brown wrote:
> On Tue, Mar 20, 2012 at 01:13:39PM +0200, Peter Ujfalusi wrote:
>> In capture stream the buffer size does not need to be constrained to be
>> bigger then the McBSP FIFO.
>> In capture the FIFO content is taken out in period length burst, this
>> enusres that the FIFO is not going to overflow.
> 
> Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.comm>

Acked-by: Jarkko Nikula <jarkko.nikula@bitmer.com>

Re: [PATCH 3/3] ASoC: omap-mcbsp: Add period size protection mode

[Grazvydas Ignotas]

On Tue, Mar 20, 2012 at 6:20 PM, Jarkko Nikula <jarkko.nikula@bitmer.com> wrote:
> On 03/20/2012 01:13 PM, Peter Ujfalusi wrote:
>> Certain application can experience underrun right after the playback start.
>> This is caused by the McBSP FIFO/sDMA integration:
>> The sDMA will push samples to the FIFO till it has threshold amount of free
>> slots available in the FIFO. If the application picks period size which is
>> smaller than the FIFO size, and it did not prepared multiple periods, or
>> it did not set the start_threshold for the stream to cover the FIFO size
>> the hw pointer will move forward, which is causing the underrun.
>>
>> Add a sysfs entry for McBSP ports: period_protection.
>> If this property is set the driver will place the constraint agains the
>> period size, and not for the buffer size. To ensure that we do not hit
>> underrun, the period size constraint will be increased with the requested
>> number of frames (the period size will be FIFO size + period_protection).
>>
>> As default the period_protection is disabled.
>>
> I don't think this is going to solve the actual problem here where
> custom asound.conf was required. IMHO custom sysfs is even worse option
> and very hard to remove afterwards. And defaulting this new setting for
> Pandora might break e.g. MER or MeeGo on N900.
>
> I didn't check this but would it be possible to either put restriction
> to start-delay (I think Grazvydas said he has experimental code for
> that?) or make sure that minimum buffer size must be higher than FIFO +
> 1 period (or something like that)?

This is what we have in pandora tree now:
http://git.openpandora.org/cgi-bin/gitweb.cgi?p=pandora-kernel.git;a=commitdiff;h=d494977441ac8f99d094b5e03398cb33a14e832a
Seems to work well for everything here.

-- 
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: Add period size protection mode

[Mark Brown]

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

On Tue, Mar 20, 2012 at 06:15:34PM +0200, Grazvydas Ignotas wrote:
> On Tue, Mar 20, 2012 at 6:01 PM, Mark Brown

> > though this should probably have the note about working around broken
> > applications from the cover letter in the changelog as with the
> > changelog alone it's really not apparent why we're doing this here as a
> > driver specific thing.

> I wouldn't really call them broken, it's enough to set period size to
> 512 with smaller start_threshold (something like 50ms) to have
> problems, those parameters are perfectly valid for a program trying to
> achieve low latency.

If they can't cope with the parameters they've set I'd call them broken,
they should've asked for more sensible parameters.

[-- 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: Add period size protection mode

[Grazvydas Ignotas]

On Tue, Mar 20, 2012 at 7:04 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Tue, Mar 20, 2012 at 06:15:34PM +0200, Grazvydas Ignotas wrote:
>> On Tue, Mar 20, 2012 at 6:01 PM, Mark Brown
>
>> > though this should probably have the note about working around broken
>> > applications from the cover letter in the changelog as with the
>> > changelog alone it's really not apparent why we're doing this here as a
>> > driver specific thing.
>
>> I wouldn't really call them broken, it's enough to set period size to
>> 512 with smaller start_threshold (something like 50ms) to have
>> problems, those parameters are perfectly valid for a program trying to
>> achieve low latency.
>
> If they can't cope with the parameters they've set I'd call them broken,
> they should've asked for more sensible parameters.

How is the program supposed to know those parameters are invalid for
this hardware? It could maybe detect underflows and increase period
until underflows stop, but there might be other reasons for underflows
like high system load. Or do you mean setting up some period size and
doing writes of that period size is not valid thing to do? Currently,
no matter how fast the writes come, there is an underflow after first
write in these conditions.


-- 
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: Add period size protection mode

[Trent Piepho]

On Tue, Mar 20, 2012 at 1:27 PM, Grazvydas Ignotas <notasas@gmail.com> wrote:
> On Tue, Mar 20, 2012 at 7:04 PM, Mark Brown
> <broonie@opensource.wolfsonmicro.com> wrote:
>> On Tue, Mar 20, 2012 at 06:15:34PM +0200, Grazvydas Ignotas wrote:
>>> On Tue, Mar 20, 2012 at 6:01 PM, Mark Brown
>>
>>> > though this should probably have the note about working around broken
>>> > applications from the cover letter in the changelog as with the
>>> > changelog alone it's really not apparent why we're doing this here as a
>>> > driver specific thing.
>>
>>> I wouldn't really call them broken, it's enough to set period size to
>>> 512 with smaller start_threshold (something like 50ms) to have
>>> problems, those parameters are perfectly valid for a program trying to
>>> achieve low latency.
>>
>> If they can't cope with the parameters they've set I'd call them broken,
>> they should've asked for more sensible parameters.

It sounds like the problem happens if an application sets
start_threshold to something less than the FIFO size.  E.g., set
start_threshold to 50ms, app writes 50 ms to stream, gets an instant
underrun when the sDMA quickly reads > 50 ms of audio as it fills the
FIFO.  Doesn't seem like the app is broken to expect that after
writing 50 ms of audio it has almost 50 ms to write more before an
underrun.

Re: [PATCH 3/3] ASoC: omap-mcbsp: Add period size protection mode

[Mark Brown]

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

On Tue, Mar 20, 2012 at 07:27:02PM +0200, Grazvydas Ignotas wrote:
> On Tue, Mar 20, 2012 at 7:04 PM, Mark Brown
> > On Tue, Mar 20, 2012 at 06:15:34PM +0200, Grazvydas Ignotas wrote:

> >> I wouldn't really call them broken, it's enough to set period size to
> >> 512 with smaller start_threshold (something like 50ms) to have
> >> problems, those parameters are perfectly valid for a program trying to
> >> achieve low latency.

> > If they can't cope with the parameters they've set I'd call them broken,
> > they should've asked for more sensible parameters.

> How is the program supposed to know those parameters are invalid for
> this hardware? It could maybe detect underflows and increase period
> until underflows stop, but there might be other reasons for underflows
> like high system load. Or do you mean setting up some period size and
> doing writes of that period size is not valid thing to do? Currently,
> no matter how fast the writes come, there is an underflow after first
> write in these conditions.

In that case why is the fix specific to this application and not a
generic one?  If we're going to underflow no matter what then why make
the workaround custom?

[-- 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: Add period size protection mode

[Jarkko Nikula]

On 03/20/2012 06:42 PM, Grazvydas Ignotas wrote:
> This is what we have in pandora tree now:
> http://git.openpandora.org/cgi-bin/gitweb.cgi?p=pandora-kernel.git;a=commitdiff;h=d494977441ac8f99d094b5e03398cb33a14e832a
> Seems to work well for everything here.
> 
To me this looks more like a correct fix. At quick test I got single
xrun when starting up but not endless loop of them. Maybe need to tune a
bit with CONFIG_SND_PCM_XRUN_DEBUG=y.

As a reference here are the test cases from you and Peter:

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

Ok:
aplay -D hw:0 -fs16_le -c2 --period-size=320 --buffer-size=972 \
--start-delay=81000 -v /dev/urandom

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

-- 
Jarkko

Re: [PATCH 3/3] ASoC: omap-mcbsp: Add period size protection mode

[Trent Piepho]

On Tue, Mar 20, 2012 at 3:20 PM, Jarkko Nikula <jarkko.nikula@bitmer.com> wrote:
> On 03/20/2012 06:42 PM, Grazvydas Ignotas wrote:
>> This is what we have in pandora tree now:
>> http://git.openpandora.org/cgi-bin/gitweb.cgi?p=pandora-kernel.git;a=commitdiff;h=d494977441ac8f99d094b5e03398cb33a14e832a
>> Seems to work well for everything here.
>>
> To me this looks more like a correct fix. At quick test I got single
> xrun when starting up but not endless loop of them. Maybe need to tune a
> bit with CONFIG_SND_PCM_XRUN_DEBUG=y.

Does the ALSA API allow the driver to change start_threshold in the
prepare function?  It seems what is needed is a minimum
start_threshold constraint, but there aren't constaints for sw_params.

With that fix, you could have an app set start_threshold to 50 ms,
write 51 ms of audio, and expect the stream to start.  But it won't,
because the driver has increased start_threshold.  The app could have
read back start_threshold from the driver, but I doubt any apps do
this, since there is nothing in the documentation about that being
necessary.

Re: [PATCH 3/3] ASoC: omap-mcbsp: Add period size protection mode

[Peter Ujfalusi]

On 03/20/2012 09:47 PM, Trent Piepho wrote:
>> To me this looks more like a correct fix. At quick test I got single
>> xrun when starting up but not endless loop of them. Maybe need to tune a
>> bit with CONFIG_SND_PCM_XRUN_DEBUG=y.
> 
> Does the ALSA API allow the driver to change start_threshold in the
> prepare function?  It seems what is needed is a minimum
> start_threshold constraint, but there aren't constaints for sw_params.

I did as well looked at the start_threshold, and came to the same
conclusion. It is sw_param, and the driver should not modify it.
The OSS emulation layer sets the start_threshold to 1, most application
sets the start threshold to the size of the buffer. Some legacy
application might not care about this at all. Others might want specific
threshold to start the actual playback.

> With that fix, you could have an app set start_threshold to 50 ms,
> write 51 ms of audio, and expect the stream to start.  But it won't,
> because the driver has increased start_threshold.  The app could have
> read back start_threshold from the driver, but I doubt any apps do
> this, since there is nothing in the documentation about that being
> necessary.

True, it is a sw_param, and the applications does not expect it to be
changed by the driver (since the driver should not change it).

-- 
Péter

Re: [PATCH 3/3] ASoC: omap-mcbsp: Add period size protection mode

[Peter Ujfalusi]

On 03/20/2012 06:20 PM, Jarkko Nikula wrote:
> On 03/20/2012 01:13 PM, Peter Ujfalusi wrote:
>> Certain application can experience underrun right after the playback start.
>> This is caused by the McBSP FIFO/sDMA integration:
>> The sDMA will push samples to the FIFO till it has threshold amount of free
>> slots available in the FIFO. If the application picks period size which is
>> smaller than the FIFO size, and it did not prepared multiple periods, or
>> it did not set the start_threshold for the stream to cover the FIFO size
>> the hw pointer will move forward, which is causing the underrun.
>>
>> Add a sysfs entry for McBSP ports: period_protection.
>> If this property is set the driver will place the constraint agains the
>> period size, and not for the buffer size. To ensure that we do not hit
>> underrun, the period size constraint will be increased with the requested
>> number of frames (the period size will be FIFO size + period_protection).
>>
>> As default the period_protection is disabled.
>>
> I don't think this is going to solve the actual problem here where
> custom asound.conf was required.

I also suggested to use custom asound.conf. For Pandora it does exist,
but the argument was that it need to be specifically created/copied to
any new OS being ported to the HW.

> IMHO custom sysfs is even worse option and very hard to remove afterwards.

Well, for system integration's point of view I would prefer the sysfs
file. If this is needed by the distribution/use case it can be enabled,
disabled in runtime. In my view it poses less hassle for distributions.

> And defaulting this new setting for Pandora might break e.g. MER or MeeGo on N900.

I would not default this behavior for the exact same reasons. It was
planed to be optional.

> I didn't check this but would it be possible to either put restriction
> to start-delay (I think Grazvydas said he has experimental code for
> that?) or make sure that minimum buffer size must be higher than FIFO +
> 1 period (or something like that)?

The start_threshold is sw_param, driver should not touch it. In fact it
is way above the driver layer.

-- 
Péter

Re: [PATCH 3/3] ASoC: omap-mcbsp: Add period size protection mode

[Jarkko Nikula]

On 03/21/2012 09:57 AM, Peter Ujfalusi wrote:
> On 03/20/2012 09:47 PM, Trent Piepho wrote:
>>> To me this looks more like a correct fix. At quick test I got single
>>> xrun when starting up but not endless loop of them. Maybe need to tune a
>>> bit with CONFIG_SND_PCM_XRUN_DEBUG=y.
>>
>> Does the ALSA API allow the driver to change start_threshold in the
>> prepare function?  It seems what is needed is a minimum
>> start_threshold constraint, but there aren't constaints for sw_params.
> 
> I did as well looked at the start_threshold, and came to the same
> conclusion. It is sw_param, and the driver should not modify it.
> The OSS emulation layer sets the start_threshold to 1, most application
> sets the start threshold to the size of the buffer. Some legacy
> application might not care about this at all. Others might want specific
> threshold to start the actual playback.
> 
Oh yes. I guess start_threshold is not even known yet when we are ruling
the buffer size so we cannot use it as a source for alternative rules?

-- 
Jarkko

Re: [PATCH 3/3] ASoC: omap-mcbsp: Add period size protection mode

[Peter Ujfalusi]

On 03/21/2012 10:13 AM, Jarkko Nikula wrote:
> Oh yes. I guess start_threshold is not even known yet when we are ruling
> the buffer size so we cannot use it as a source for alternative rules?

We do place rule for the buffer size, so it is guarantied that it is
bigger than the FIFO.
Start threshold is set by the application, and it is not really
guarantied when the application will set it. It can be runtime changes
as well - being sw_param. In most case overwriting in pcm_prepare phase
might work, but it is not guarantied.

Not sure, but what would be probably useful here is to have a way from
the driver level to let userspace/ALSA know that we should use this and
that start_threshold.
If application does not care ALSA would use this driver provided
information. If application specifies the start_threshold, we should
obey it. In this case the period size would be needed to have the
constraint, but we do not know beforehand what application wants to do.

-- 
Péter

Re: [PATCH 3/3] ASoC: omap-mcbsp: Add period size protection mode

[Peter Ujfalusi]

On 03/20/2012 06:15 PM, Grazvydas Ignotas wrote:
> I wouldn't really call them broken, it's enough to set period size to
> 512 with smaller start_threshold (something like 50ms) to have
> problems, those parameters are perfectly valid for a program trying to
> achieve low latency.

Where this 50ms comes from?
The McBSP2 FIFO length is:
48KHz/mono:     26.66ms
48KHz/stereo:   13.33ms
44.1KHz/mono:   29.02ms
44.1KHz/stereo: 14.51ms
8Khz/mono:      160ms
8Khz/stereo:    80ms

Does Pandora uses 8Khz?

The same thing applies to the start_threshold as to the period size. It
has to be bigger than the FIFO size.

> It's a shame this still won't work out-of-the box, but at least there
> will be some solution.

Defaulting this behavior would break other distributions for OMAP
platforms. I know, Pandora is bitten by this, but MeeGo, ubuntu, Linaro
is fine (as far as I know).

-- 
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: Add period size protection mode

[Jarkko Nikula]

On 03/21/2012 10:03 AM, Peter Ujfalusi wrote:
> On 03/20/2012 06:20 PM, Jarkko Nikula wrote:
>> I don't think this is going to solve the actual problem here where
>> custom asound.conf was required.
> 
> I also suggested to use custom asound.conf. For Pandora it does exist,
> but the argument was that it need to be specifically created/copied to
> any new OS being ported to the HW.
> 
>> IMHO custom sysfs is even worse option and very hard to remove afterwards.
> 
> Well, for system integration's point of view I would prefer the sysfs
> file. If this is needed by the distribution/use case it can be enabled,
> disabled in runtime. In my view it poses less hassle for distributions.
> 
Problem with custom sysfs is that no generic distribution is going to
use it but problem can be triggered there. IMHO for tailored
distribution point of view custom sysfs setting is no better than custom
asound.conf.

I'm half thinking shall we leave this broken if we cannot figure out any
automatic kernel based sulution to this?

-- 
Jarkko

Re: [PATCH 3/3] ASoC: omap-mcbsp: Add period size protection mode

[Mark Brown]

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

On Wed, Mar 21, 2012 at 10:32:11AM +0200, Jarkko Nikula wrote:
> On 03/21/2012 10:03 AM, Peter Ujfalusi wrote:

> > Well, for system integration's point of view I would prefer the sysfs
> > file. If this is needed by the distribution/use case it can be enabled,
> > disabled in runtime. In my view it poses less hassle for distributions.

> Problem with custom sysfs is that no generic distribution is going to
> use it but problem can be triggered there. IMHO for tailored
> distribution point of view custom sysfs setting is no better than custom
> asound.conf.

I have to say that I agree wholeheartedly with this - the sysfs rule has
all the problems that a custom asound.conf does.

[-- 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: Add period size protection mode

[Peter Ujfalusi]

On 03/21/2012 10:40 AM, Mark Brown wrote:
> On Wed, Mar 21, 2012 at 10:32:11AM +0200, Jarkko Nikula wrote:
>> On 03/21/2012 10:03 AM, Peter Ujfalusi wrote:
> 
>>> Well, for system integration's point of view I would prefer the sysfs
>>> file. If this is needed by the distribution/use case it can be enabled,
>>> disabled in runtime. In my view it poses less hassle for distributions.
> 
>> Problem with custom sysfs is that no generic distribution is going to
>> use it but problem can be triggered there. IMHO for tailored
>> distribution point of view custom sysfs setting is no better than custom
>> asound.conf.
> 
> I have to say that I agree wholeheartedly with this - the sysfs rule has
> all the problems that a custom asound.conf does.

Agreed. Let's drop this patch, and try to figure out something which can
be used by other platforms as well.

I would keep the first two patch in this series..

-- 
Péter

Re: [PATCH 3/3] ASoC: omap-mcbsp: Add period size protection mode

[Jarkko Nikula]

On 03/21/2012 11:16 AM, Peter Ujfalusi wrote:
> Agreed. Let's drop this patch, and try to figure out something which can
> be used by other platforms as well.
> 
> I would keep the first two patch in this series..
>
I acked already 1/3 but 2/3 needs commit log update and change below
looks suspicious for omap1 and 2.

@@ -120,6 +120,9 @@ static int omap_mcbsp_dai_startup(struct
snd_pcm_substream *substream,
 	if (!cpu_dai->active)
 		err = omap_mcbsp_request(mcbsp);

+	if (!mcbsp->pdata->buffer_size)
+		return err;
+

-- 
Jarkko

Re: [PATCH 3/3] ASoC: omap-mcbsp: Add period size protection mode

[Peter Ujfalusi]

On 03/21/2012 11:34 AM, Jarkko Nikula wrote:
> On 03/21/2012 11:16 AM, Peter Ujfalusi wrote:
>> Agreed. Let's drop this patch, and try to figure out something which can
>> be used by other platforms as well.
>>
>> I would keep the first two patch in this series..
>>
> I acked already 1/3 but 2/3 needs commit log update and change below
> looks suspicious for omap1 and 2.

True, I'll update the commit log for patch 2.

> @@ -120,6 +120,9 @@ static int omap_mcbsp_dai_startup(struct
> snd_pcm_substream *substream,
>  	if (!cpu_dai->active)
>  		err = omap_mcbsp_request(mcbsp);
> 
> +	if (!mcbsp->pdata->buffer_size)
> +		return err;

We only need to set the constraints on OMAP versions which have FIFO. If
buffer_size is 0 we can just return without going forward.
It removes one level of nested code in the function.

-- 
Péter

Re: [PATCH 3/3] ASoC: omap-mcbsp: Add period size protection mode

[Grazvydas Ignotas]

On Wed, Mar 21, 2012 at 10:23 AM, Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
> On 03/20/2012 06:15 PM, Grazvydas Ignotas wrote:
>> I wouldn't really call them broken, it's enough to set period size to
>> 512 with smaller start_threshold (something like 50ms) to have
>> problems, those parameters are perfectly valid for a program trying to
>> achieve low latency.
>
> Where this 50ms comes from?
> The McBSP2 FIFO length is:
> 48KHz/mono:     26.66ms
> 48KHz/stereo:   13.33ms
> 44.1KHz/mono:   29.02ms
> 44.1KHz/stereo: 14.51ms
> 8Khz/mono:      160ms
> 8Khz/stereo:    80ms
>
> Does Pandora uses 8Khz?

Nope, 50ms+8kHz was just to illustrate the problem easier. 22/44kHz
are used most often, some games that tend to set low thresholds and
OSS emulation are ones with problems.

> The same thing applies to the start_threshold as to the period size. It
> has to be bigger than the FIFO size.
>
>> It's a shame this still won't work out-of-the box, but at least there
>> will be some solution.
>
> Defaulting this behavior would break other distributions for OMAP
> platforms. I know, Pandora is bitten by this, but MeeGo, ubuntu, Linaro
> is fine (as far as I know).

I wonder about that, if they are already ok they must be using larger
period/start_threshold anyway and should not be affected?


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