[alsa-devel] [PATCH 0/3] ASoC: omap-mcbsp: Constraint handling changes
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(-)
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,
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
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
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; }
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
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,
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.
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.
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.
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.
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.
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?
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).
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?
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)?
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=commitd... Seems to work well for everything here.
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=commitd... 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
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=commitd... 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.
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).
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?
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.
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.
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?
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.
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..
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; +
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.
participants (5)
-
Grazvydas Ignotas -
Jarkko Nikula -
Mark Brown -
Peter Ujfalusi -
Trent Piepho