Re: [PATCH v3 1/3] ASoC: davinci-mcasp: Constraint on the period and buffer size based on FIFO usage

[Lars-Peter Clausen <lars@metafoo.de>]

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:
> 1. buffer first followed by period
> 2. period first followed by buffer
> 3. buffer only
> 4. period only
> 5. 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