[alsa-devel] Problems with safe API and snd-cs46xx

[Takashi Iwai]

At Tue, 8 Sep 2009 00:47:00 +0200,
Lennart Poettering wrote:
> 
> On Mon, 07.09.09 17:01, Takashi Iwai (tiwai at suse.de) wrote:
> 
> > Maybe a bit "safer" option would be to check the period min size and
> > raise to a sane value as a workaround.
> > 
> > Or, if the period size doesn't matter much but rather the buffer size
> > is more important, you can first limit the buffer size as max.  Then
> > call snd_pcm_hw_params().  Without this, the PCM core determines the
> > period size first, then the buffer size.
> 
> Hmm, I wonder what this means for PulseAudio. In the past we had
> various problems with the functions for setting the buffer metrics and
> the order in which we called them. 
> 
> For a short while we used to call
> snd_pcm_hw_params_set_buffer_size_near() first, then
> snd_pcm_hw_params_set_periods_near() and then snd_pcm_hw_params().
> 
> This turned out to cause a couple of issues with some drivers (i think
> CS46xx is one of them, ice1712 another, I lack the hw in question, so
> i never tried to track this down further). Basically on those drivers
> both _set_buffer_size_near() and _set_periods_near() would fail with
> EINVAL for some reason and then causing snd_pcm_hw_params() to return
> ENOENT. Removing the two calls and calling snd_pcm_hw_params() would
> however work. Changing the order of the first two calls, i.e. doing
> first _set_periods_near() and then _set_buffer_size_near() would make
> both calls succeed and the snd_pcm_hw_params(), too.

The general problem in the hw_params setup is that the multiple
parameters depending with each other are allowed almost freely.
And, yet another problematic constraint is like cs46xx's one, the
power-of-two rule.  This limits strongly the value range.

Also, another issue is the presence of three units to specify the same
thing.  There are units, frame, bytes and time for period and buffer
sizes.  Especially the former two and the time units can be
inconsistent due to the integer rounding.

> It would be good if the ALSA docs would actually mention in which
> order those functions need to be called, if the order matters, which
> it apparently does.

There is no golden rule, unfortunately, AFAIK.  There can be pretty
weird hardware, e.g. the buffer size depending on rate.  But, as a
rule of thumb:
1. set access, format, channels and rate first,
2. if you need larger buffers than shorter periods, set the buffer
  size first,
3. set the period size only when you must specify it

But, this can also fail when a hardware has a strong dependency
between period and buffer sizes together with a strong constraint
in period size.  In that case, you may need to try another way,
set period and hw_params.

As mentioned before, setting the buffer size is currently needed just
because of PCM hw_params determination mechanism.  By unknown reason
(it was written before I touched it), PCM core determines the period
size first, then the buffer size.  So, the patch below might improve
the  situation. But, this is a radical change, and we need at least
some switch (module parameter, proc, sysfs, etc) to back the old
compatible method.

Sophie, what happens if you use the patch without your change?

> (Hmm, and while we are at it. Ssome other driver I don't posess,

If it's only about the hw_params, you can hack snd-dummy driver code,
BTW.

> ens1371 has some issues with the buffer size: if you ask it for 65536
> bytes in the playback buffer it will only agree to 65532. If the next
> time you ask for 65532 right-away it will only agree to 65528, and so on...)

The byte size can depend on the sample format and channels.
This might be the case.  Otherwise I don't see any strong restriction
of buffer/period size in ens1371 code.  It has the restriction of
sample rates, though.


thanks,

Takashi

---
diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
index 30f4108..b5114a8 100644
--- a/sound/core/pcm_lib.c
+++ b/sound/core/pcm_lib.c
@@ -1504,8 +1504,8 @@ int snd_pcm_hw_params_choose(struct snd_pcm_substream *pcm,
 		SNDRV_PCM_HW_PARAM_SUBFORMAT,
 		SNDRV_PCM_HW_PARAM_CHANNELS,
 		SNDRV_PCM_HW_PARAM_RATE,
-		SNDRV_PCM_HW_PARAM_PERIOD_TIME,
 		SNDRV_PCM_HW_PARAM_BUFFER_SIZE,
+		SNDRV_PCM_HW_PARAM_PERIOD_SIZE,
 		SNDRV_PCM_HW_PARAM_TICK_TIME,
 		-1
 	};