* [PATCH 0/3] ASoC: omap-mcbsp: Constraint handling changes
@ 2012-03-20 11:13 Peter Ujfalusi
2012-03-20 11:13 ` [PATCH 1/3] ASoC: omap-mcbsp: buffer size constraint only applies to playback stream Peter Ujfalusi
` (2 more replies)
0 siblings, 3 replies; 28+ messages in thread
From: Peter Ujfalusi @ 2012-03-20 11:13 UTC (permalink / raw)
To: Liam Girdwood, Mark Brown; +Cc: alsa-devel, Grazvydas Ignotas, Jarkko Nikula
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
^ permalink raw reply [flat|nested] 28+ messages in thread* [PATCH 1/3] ASoC: omap-mcbsp: buffer size constraint only applies to playback stream 2012-03-20 11:13 [PATCH 0/3] ASoC: omap-mcbsp: Constraint handling changes Peter Ujfalusi @ 2012-03-20 11:13 ` Peter Ujfalusi 2012-03-20 15:26 ` Mark Brown 2012-03-20 11:13 ` [PATCH 2/3] ASoC: omap-mcbsp: Restructure omap_mcbsp_dai_startup code Peter Ujfalusi 2012-03-20 11:13 ` [PATCH 3/3] ASoC: omap-mcbsp: Add period size protection mode Peter Ujfalusi 2 siblings, 1 reply; 28+ messages in thread From: Peter Ujfalusi @ 2012-03-20 11:13 UTC (permalink / raw) To: Liam Girdwood, Mark Brown; +Cc: alsa-devel, Grazvydas Ignotas, Jarkko Nikula 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 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH 1/3] ASoC: omap-mcbsp: buffer size constraint only applies to playback stream 2012-03-20 11:13 ` [PATCH 1/3] ASoC: omap-mcbsp: buffer size constraint only applies to playback stream Peter Ujfalusi @ 2012-03-20 15:26 ` Mark Brown 2012-03-20 16:34 ` Jarkko Nikula 0 siblings, 1 reply; 28+ messages in thread From: Mark Brown @ 2012-03-20 15:26 UTC (permalink / raw) To: Peter Ujfalusi Cc: alsa-devel, Liam Girdwood, Jarkko Nikula, Grazvydas Ignotas [-- 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 --] ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/3] ASoC: omap-mcbsp: buffer size constraint only applies to playback stream 2012-03-20 15:26 ` Mark Brown @ 2012-03-20 16:34 ` Jarkko Nikula 0 siblings, 0 replies; 28+ messages in thread From: Jarkko Nikula @ 2012-03-20 16:34 UTC (permalink / raw) To: Mark Brown; +Cc: Peter Ujfalusi, alsa-devel, Liam Girdwood, Grazvydas Ignotas 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> ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 2/3] ASoC: omap-mcbsp: Restructure omap_mcbsp_dai_startup code 2012-03-20 11:13 [PATCH 0/3] ASoC: omap-mcbsp: Constraint handling changes Peter Ujfalusi 2012-03-20 11:13 ` [PATCH 1/3] ASoC: omap-mcbsp: buffer size constraint only applies to playback stream Peter Ujfalusi @ 2012-03-20 11:13 ` Peter Ujfalusi 2012-03-20 15:27 ` Mark Brown 2012-03-20 11:13 ` [PATCH 3/3] ASoC: omap-mcbsp: Add period size protection mode Peter Ujfalusi 2 siblings, 1 reply; 28+ messages in thread From: Peter Ujfalusi @ 2012-03-20 11:13 UTC (permalink / raw) To: Liam Girdwood, Mark Brown; +Cc: alsa-devel, Grazvydas Ignotas, Jarkko Nikula 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 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH 2/3] ASoC: omap-mcbsp: Restructure omap_mcbsp_dai_startup code 2012-03-20 11:13 ` [PATCH 2/3] ASoC: omap-mcbsp: Restructure omap_mcbsp_dai_startup code Peter Ujfalusi @ 2012-03-20 15:27 ` Mark Brown 0 siblings, 0 replies; 28+ messages in thread From: Mark Brown @ 2012-03-20 15:27 UTC (permalink / raw) To: Peter Ujfalusi Cc: alsa-devel, Liam Girdwood, Jarkko Nikula, Grazvydas Ignotas [-- 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 --] ^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 3/3] ASoC: omap-mcbsp: Add period size protection mode 2012-03-20 11:13 [PATCH 0/3] ASoC: omap-mcbsp: Constraint handling changes Peter Ujfalusi 2012-03-20 11:13 ` [PATCH 1/3] ASoC: omap-mcbsp: buffer size constraint only applies to playback stream Peter Ujfalusi 2012-03-20 11:13 ` [PATCH 2/3] ASoC: omap-mcbsp: Restructure omap_mcbsp_dai_startup code Peter Ujfalusi @ 2012-03-20 11:13 ` Peter Ujfalusi 2012-03-20 16:01 ` Mark Brown 2012-03-20 16:20 ` Jarkko Nikula 2 siblings, 2 replies; 28+ messages in thread From: Peter Ujfalusi @ 2012-03-20 11:13 UTC (permalink / raw) To: Liam Girdwood, Mark Brown; +Cc: alsa-devel, Grazvydas Ignotas, Jarkko Nikula 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 ^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH 3/3] ASoC: omap-mcbsp: Add period size protection mode 2012-03-20 11:13 ` [PATCH 3/3] ASoC: omap-mcbsp: Add period size protection mode Peter Ujfalusi @ 2012-03-20 16:01 ` Mark Brown 2012-03-20 16:15 ` Grazvydas Ignotas 2012-03-20 16:20 ` Jarkko Nikula 1 sibling, 1 reply; 28+ messages in thread From: Mark Brown @ 2012-03-20 16:01 UTC (permalink / raw) To: Peter Ujfalusi Cc: alsa-devel, Liam Girdwood, Jarkko Nikula, Grazvydas Ignotas [-- 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 --] ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/3] ASoC: omap-mcbsp: Add period size protection mode 2012-03-20 16:01 ` Mark Brown @ 2012-03-20 16:15 ` Grazvydas Ignotas 2012-03-20 17:04 ` Mark Brown 2012-03-21 8:23 ` Peter Ujfalusi 0 siblings, 2 replies; 28+ messages in thread From: Grazvydas Ignotas @ 2012-03-20 16:15 UTC (permalink / raw) To: Mark Brown; +Cc: Peter Ujfalusi, alsa-devel, Liam Girdwood, Jarkko Nikula 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/3] ASoC: omap-mcbsp: Add period size protection mode 2012-03-20 16:15 ` Grazvydas Ignotas @ 2012-03-20 17:04 ` Mark Brown 2012-03-20 17:27 ` Grazvydas Ignotas 2012-03-21 8:23 ` Peter Ujfalusi 1 sibling, 1 reply; 28+ messages in thread From: Mark Brown @ 2012-03-20 17:04 UTC (permalink / raw) To: Grazvydas Ignotas Cc: Peter Ujfalusi, alsa-devel, Liam Girdwood, Jarkko Nikula [-- 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 --] ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/3] ASoC: omap-mcbsp: Add period size protection mode 2012-03-20 17:04 ` Mark Brown @ 2012-03-20 17:27 ` Grazvydas Ignotas 2012-03-20 18:07 ` Trent Piepho 2012-03-20 18:12 ` Mark Brown 0 siblings, 2 replies; 28+ messages in thread From: Grazvydas Ignotas @ 2012-03-20 17:27 UTC (permalink / raw) To: Mark Brown; +Cc: Peter Ujfalusi, alsa-devel, Liam Girdwood, Jarkko Nikula 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/3] ASoC: omap-mcbsp: Add period size protection mode 2012-03-20 17:27 ` Grazvydas Ignotas @ 2012-03-20 18:07 ` Trent Piepho 2012-03-20 18:12 ` Mark Brown 1 sibling, 0 replies; 28+ messages in thread From: Trent Piepho @ 2012-03-20 18:07 UTC (permalink / raw) To: Grazvydas Ignotas Cc: Peter Ujfalusi, alsa-devel, Mark Brown, Liam Girdwood, Jarkko Nikula 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. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/3] ASoC: omap-mcbsp: Add period size protection mode 2012-03-20 17:27 ` Grazvydas Ignotas 2012-03-20 18:07 ` Trent Piepho @ 2012-03-20 18:12 ` Mark Brown 1 sibling, 0 replies; 28+ messages in thread From: Mark Brown @ 2012-03-20 18:12 UTC (permalink / raw) To: Grazvydas Ignotas Cc: Peter Ujfalusi, alsa-devel, Liam Girdwood, Jarkko Nikula [-- 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 --] ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/3] ASoC: omap-mcbsp: Add period size protection mode 2012-03-20 16:15 ` Grazvydas Ignotas 2012-03-20 17:04 ` Mark Brown @ 2012-03-21 8:23 ` Peter Ujfalusi 2012-03-21 11:55 ` Grazvydas Ignotas 1 sibling, 1 reply; 28+ messages in thread From: Peter Ujfalusi @ 2012-03-21 8:23 UTC (permalink / raw) To: Grazvydas Ignotas; +Cc: alsa-devel, Mark Brown, Liam Girdwood, Jarkko Nikula 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/3] ASoC: omap-mcbsp: Add period size protection mode 2012-03-21 8:23 ` Peter Ujfalusi @ 2012-03-21 11:55 ` Grazvydas Ignotas 0 siblings, 0 replies; 28+ messages in thread From: Grazvydas Ignotas @ 2012-03-21 11:55 UTC (permalink / raw) To: Peter Ujfalusi; +Cc: alsa-devel, Mark Brown, Liam Girdwood, Jarkko Nikula 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/3] ASoC: omap-mcbsp: Add period size protection mode 2012-03-20 11:13 ` [PATCH 3/3] ASoC: omap-mcbsp: Add period size protection mode Peter Ujfalusi 2012-03-20 16:01 ` Mark Brown @ 2012-03-20 16:20 ` Jarkko Nikula 2012-03-20 16:42 ` Grazvydas Ignotas 2012-03-21 8:03 ` Peter Ujfalusi 1 sibling, 2 replies; 28+ messages in thread From: Jarkko Nikula @ 2012-03-20 16:20 UTC (permalink / raw) To: Peter Ujfalusi; +Cc: alsa-devel, Mark Brown, Liam Girdwood, Grazvydas Ignotas 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/3] ASoC: omap-mcbsp: Add period size protection mode 2012-03-20 16:20 ` Jarkko Nikula @ 2012-03-20 16:42 ` Grazvydas Ignotas 2012-03-20 19:20 ` Jarkko Nikula 2012-03-21 8:03 ` Peter Ujfalusi 1 sibling, 1 reply; 28+ messages in thread From: Grazvydas Ignotas @ 2012-03-20 16:42 UTC (permalink / raw) To: Jarkko Nikula; +Cc: Peter Ujfalusi, alsa-devel, Mark Brown, Liam Girdwood 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/3] ASoC: omap-mcbsp: Add period size protection mode 2012-03-20 16:42 ` Grazvydas Ignotas @ 2012-03-20 19:20 ` Jarkko Nikula 2012-03-20 19:47 ` Trent Piepho 0 siblings, 1 reply; 28+ messages in thread From: Jarkko Nikula @ 2012-03-20 19:20 UTC (permalink / raw) To: Grazvydas Ignotas; +Cc: Peter Ujfalusi, alsa-devel, Mark Brown, Liam Girdwood 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/3] ASoC: omap-mcbsp: Add period size protection mode 2012-03-20 19:20 ` Jarkko Nikula @ 2012-03-20 19:47 ` Trent Piepho 2012-03-21 7:57 ` Peter Ujfalusi 0 siblings, 1 reply; 28+ messages in thread From: Trent Piepho @ 2012-03-20 19:47 UTC (permalink / raw) To: Jarkko Nikula Cc: Peter Ujfalusi, alsa-devel, Mark Brown, Grazvydas Ignotas, Liam Girdwood 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. ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/3] ASoC: omap-mcbsp: Add period size protection mode 2012-03-20 19:47 ` Trent Piepho @ 2012-03-21 7:57 ` Peter Ujfalusi 2012-03-21 8:13 ` Jarkko Nikula 0 siblings, 1 reply; 28+ messages in thread From: Peter Ujfalusi @ 2012-03-21 7:57 UTC (permalink / raw) To: Trent Piepho Cc: alsa-devel, Mark Brown, Grazvydas Ignotas, Jarkko Nikula, Liam Girdwood 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/3] ASoC: omap-mcbsp: Add period size protection mode 2012-03-21 7:57 ` Peter Ujfalusi @ 2012-03-21 8:13 ` Jarkko Nikula 2012-03-21 8:21 ` Peter Ujfalusi 0 siblings, 1 reply; 28+ messages in thread From: Jarkko Nikula @ 2012-03-21 8:13 UTC (permalink / raw) To: Peter Ujfalusi Cc: Liam Girdwood, alsa-devel, Trent Piepho, Grazvydas Ignotas, Mark Brown 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/3] ASoC: omap-mcbsp: Add period size protection mode 2012-03-21 8:13 ` Jarkko Nikula @ 2012-03-21 8:21 ` Peter Ujfalusi 0 siblings, 0 replies; 28+ messages in thread From: Peter Ujfalusi @ 2012-03-21 8:21 UTC (permalink / raw) To: Jarkko Nikula Cc: Liam Girdwood, alsa-devel, Trent Piepho, Grazvydas Ignotas, Mark Brown 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/3] ASoC: omap-mcbsp: Add period size protection mode 2012-03-20 16:20 ` Jarkko Nikula 2012-03-20 16:42 ` Grazvydas Ignotas @ 2012-03-21 8:03 ` Peter Ujfalusi 2012-03-21 8:32 ` Jarkko Nikula 1 sibling, 1 reply; 28+ messages in thread From: Peter Ujfalusi @ 2012-03-21 8:03 UTC (permalink / raw) To: Jarkko Nikula; +Cc: alsa-devel, Mark Brown, Liam Girdwood, Grazvydas Ignotas 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/3] ASoC: omap-mcbsp: Add period size protection mode 2012-03-21 8:03 ` Peter Ujfalusi @ 2012-03-21 8:32 ` Jarkko Nikula 2012-03-21 8:40 ` Mark Brown 0 siblings, 1 reply; 28+ messages in thread From: Jarkko Nikula @ 2012-03-21 8:32 UTC (permalink / raw) To: Peter Ujfalusi; +Cc: alsa-devel, Mark Brown, Liam Girdwood, Grazvydas Ignotas 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/3] ASoC: omap-mcbsp: Add period size protection mode 2012-03-21 8:32 ` Jarkko Nikula @ 2012-03-21 8:40 ` Mark Brown 2012-03-21 9:16 ` Peter Ujfalusi 0 siblings, 1 reply; 28+ messages in thread From: Mark Brown @ 2012-03-21 8:40 UTC (permalink / raw) To: Jarkko Nikula Cc: Peter Ujfalusi, alsa-devel, Liam Girdwood, Grazvydas Ignotas [-- 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 --] ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/3] ASoC: omap-mcbsp: Add period size protection mode 2012-03-21 8:40 ` Mark Brown @ 2012-03-21 9:16 ` Peter Ujfalusi 2012-03-21 9:34 ` Jarkko Nikula 0 siblings, 1 reply; 28+ messages in thread From: Peter Ujfalusi @ 2012-03-21 9:16 UTC (permalink / raw) To: Mark Brown; +Cc: alsa-devel, Liam Girdwood, Jarkko Nikula, Grazvydas Ignotas 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/3] ASoC: omap-mcbsp: Add period size protection mode 2012-03-21 9:16 ` Peter Ujfalusi @ 2012-03-21 9:34 ` Jarkko Nikula 2012-03-21 9:46 ` Peter Ujfalusi 0 siblings, 1 reply; 28+ messages in thread From: Jarkko Nikula @ 2012-03-21 9:34 UTC (permalink / raw) To: Peter Ujfalusi; +Cc: alsa-devel, Mark Brown, Liam Girdwood, Grazvydas Ignotas 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/3] ASoC: omap-mcbsp: Add period size protection mode 2012-03-21 9:34 ` Jarkko Nikula @ 2012-03-21 9:46 ` Peter Ujfalusi 0 siblings, 0 replies; 28+ messages in thread From: Peter Ujfalusi @ 2012-03-21 9:46 UTC (permalink / raw) To: Jarkko Nikula; +Cc: alsa-devel, Mark Brown, Liam Girdwood, Grazvydas Ignotas 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 ^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2012-03-21 11:55 UTC | newest] Thread overview: 28+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-03-20 11:13 [PATCH 0/3] ASoC: omap-mcbsp: Constraint handling changes Peter Ujfalusi 2012-03-20 11:13 ` [PATCH 1/3] ASoC: omap-mcbsp: buffer size constraint only applies to playback stream Peter Ujfalusi 2012-03-20 15:26 ` Mark Brown 2012-03-20 16:34 ` Jarkko Nikula 2012-03-20 11:13 ` [PATCH 2/3] ASoC: omap-mcbsp: Restructure omap_mcbsp_dai_startup code Peter Ujfalusi 2012-03-20 15:27 ` Mark Brown 2012-03-20 11:13 ` [PATCH 3/3] ASoC: omap-mcbsp: Add period size protection mode Peter Ujfalusi 2012-03-20 16:01 ` Mark Brown 2012-03-20 16:15 ` Grazvydas Ignotas 2012-03-20 17:04 ` Mark Brown 2012-03-20 17:27 ` Grazvydas Ignotas 2012-03-20 18:07 ` Trent Piepho 2012-03-20 18:12 ` Mark Brown 2012-03-21 8:23 ` Peter Ujfalusi 2012-03-21 11:55 ` Grazvydas Ignotas 2012-03-20 16:20 ` Jarkko Nikula 2012-03-20 16:42 ` Grazvydas Ignotas 2012-03-20 19:20 ` Jarkko Nikula 2012-03-20 19:47 ` Trent Piepho 2012-03-21 7:57 ` Peter Ujfalusi 2012-03-21 8:13 ` Jarkko Nikula 2012-03-21 8:21 ` Peter Ujfalusi 2012-03-21 8:03 ` Peter Ujfalusi 2012-03-21 8:32 ` Jarkko Nikula 2012-03-21 8:40 ` Mark Brown 2012-03-21 9:16 ` Peter Ujfalusi 2012-03-21 9:34 ` Jarkko Nikula 2012-03-21 9:46 ` Peter Ujfalusi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).