On 03/20/2014 02:47 PM, Peter Ujfalusi wrote:
Hi Mark,
On 03/19/2014 03:14 PM, Mark Brown wrote:
On Wed, Mar 19, 2014 at 01:13:50PM +0200, Peter Ujfalusi wrote:
On 03/18/2014 08:07 PM, Mark Brown wrote:
OK, so is the patch an improvement or not? If it fixes some cases it's probably worth applying even if further fixes are still needed.
Some application might fail (like mplayer with 44.1KHz) with constraint on the period size only. Without the constraint we will have constant pops at every period when non aligned size has been selected - if the FIFO depth is configured to more than 1, which is not yet the case in upstream.
OK, given that the FIFO depth change isn't yet upstream it'd be a step backwards so I'll leave this for now.
I have looked at the issue and I found some clues on why mplayer fails: it try to set the snd_pcm_hw_params_set_buffer_time_near() first followed by the snd_pcm_hw_params_set_periods_near(). And this fails in some cases when we have constraint step on the period only. Other applications set the period param first followed by the buffer (aplay and various alsa test tools as well). PA however have a fallback mechanism:
- buffer first followed by period
- period first followed by buffer
- buffer only
- period only
- no period, no buffer params
When I have step constraint on period PA fails with the first, second (because there's a bug in the PA code here) and third method but going to succeed with four.
I have looked around in the kernel and other drivers do set both period and buffer step constraint, for example:
sound/arm/pxa2xx-pcm-lib.c (_BYTES) sound/drivers/vx/vx_pcm.c (_BYTES) sound/pci/echoaudio/echoaudio.c (_SIZE) sound/pci/ice1712/ice1724.c (_BYTES) sound/pci/lola/lola_pcm.c (_SIZE) sound/soc/kirkwood/kirkwood-dma.c (_BYTES) sound/soc/s6000/s6000-pcm.c (_BYTES) ...
I still don't know why buffer size fails (in some cases) if we only have step constraint on the period size, but it looks to me that others also encountered with similar issues and the fix they have is the same what I had in the first version of the patch.
That sounds like a bug in either the kernel or alsa-lib. We do have a rule in place that specifies that the buffer size needs to be a integer multiple of the period size and we have a rule in place that the period size needs to be a multiple of a constant C. Hence ALSA should be able to deduce that the buffer size needs to be at least a multiple of min_periods * C. We probably should fix this for good and not workaround it in individual drivers. Do you think you can put together a small standalone test application that shows the issue?
- Lars