[alsa-devel] Problems with safe API and snd-cs46xx
Hi there,
I was pointed here by Audacious developers, who believe I've found a bug in the snd-cs46xx driver, which I use for my sound card (a Turtle Beach Santa Cruz, whose PCI ID is 1013:6003).
Audacious, as of version 2.1, uses the default period time offered by ALSA and limits itself to the "safe API" as per Lennart Poettering. This already uncovered a bug in the Aureal Vortex driver, and I believe, based on the information that the devs gave me, that I have just uncovered another. (I'm also having problems with recent versions of OpenAL. However, I'll explain this further below.)
I first started having the problem after having upgraded to Audacious 2.1, from Audacious 1.5.1. (to be more precise, the Gentoo ebuild 1.5.1-r1.) At the time, I was using the Gentoo-patched 2.6.29-series kernel, which I use as my normal kernel. After upgrading, I could no longer play audio directly using ALSA; instead, Audacious would initialise to 00:00, not play anything, and report the following to the owning console every so often:
ERROR: ALSA: alsa-core.c:226 (alsaplug_write_buffer): (write) snd_pcm_recover: Input/output error
The thread which was doing this also seemed to be blocking, as when I tried to do anything else in Audacious, the interface would then freeze until the thread was done. OSS output seems to be okay, although as I don't have OSS compiled at all into the kernel (not even as a module), this would be going via ALSA's own OSS emulation. In addition, ALSA output to another audio device from Audacious works fine.
I Googled for this and came across another snd-cs46xx user having the exact same problem in bug AUD-53 in the JIRA implementation they were using: http://jira.atheme.org/browse/AUD-53 . Since that user had been asked to upgrade to tip but hadn't, I did so instead (uninstalling the Gentoo ebuilds frst, of course) and reported back the additional debug info:
DEBUG: ALSA: alsa-core.c:226 (alsaplug_write_buffer): snd_pcm_writei error: Input/output error ERROR: ALSA: alsa-core.c:230 (alsaplug_write_buffer): snd_pcm_recover error: Input/output error
I then didn't know how to continue as I wasn't sure whether the bug would be seen as it had previously been closed as unreproducible, so I hopped onto the IRC channel and asked about what should be done. It was there that the probability of it being a driver bug in the snd-cs46xx sound driver was mentioned, and the reasoning - that the use of ALSA's safe API has been exposing driver bugs that were previously hidden.
As mentioned previously, I'm also having issues with OpenAL. The version I'm using now is 1.7.411; previously I was using 1.5.304, which I believe worked fine. (I can't easily test this now as the Gentoo ebuild for it has gone, however.) The symptoms I got were similar - I got no sound, and messages were printed to the console every so often (more often than with Audacious, however):
AL lib: alsa.c:194: Wait timeout... buffer size too low?
At first I thought this was an issue with Second Life, but on seeing the exact same symptoms using mplayer with the '-ao openal' switch, and also seeing it occur with 'openal-info', it seems to be an OpenAL thing. (Since I don't have many apps that I use with OpenAL, I hadn't known this before.) It seems that this is a symptom of whatever driver bug is causing the Audacious error.
For the purposes of making this post, I decided to compile and install a vanilla 2.6.30.5 kernel to boot into, so that I could make sure there were no problems with the latest stable build. (I'd rather not run unstable versions if I can help it, or I would have done so; this is my primary machine. However, I'm willing to apply patches for this specific problem in order to test them.)
The results of this were that I no longer get the messages and the threads don't block, but I get entirely new problems now - instead of useable sound, I get what sounds a little like noise, but more crackly, and the seek bar in Audacious seems to be going along as fast as it can - for each wallclock second, the seek bar goes up by about 114 seconds. The same occurs when I use mplayer with the '-ao openal' switch; I get the noise/crackles, and the time position shown on the console zooms by.
The description of my sound card given by 'lspci -vv' (under both kernels) is thus:
05:04.0 Multimedia audio controller: Cirrus Logic CS 4614/22/24/30 [CrystalClear SoundFusion Audio Accelerator] (rev 01) Subsystem: Voyetra Technologies Santa Cruz Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx- Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=slow >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx- Latency: 64 (1000ns min, 6000ns max) Interrupt: pin A routed to IRQ 22 Region 0: Memory at fdafe000 (32-bit, non-prefetchable) [size=4K] Region 1: Memory at fd900000 (32-bit, non-prefetchable) [size=1M] Capabilities: [40] Power Management version 2 Flags: PMEClk- DSI+ D1+ D2+ AuxCurrent=0mA PME(D0-,D1-,D2-,D3hot-,D3cold-) Status: D0 PME-Enable- DSel=0 DScale=0 PME- Kernel driver in use: Sound Fusion CS46xx Kernel modules: snd-cs46xx
Most applications that I have seem to work properly with regards to audio, although I've heard that this is probably because ALSA's safe API isn't very well documented and as such most applications will not be using it, thus not exposing the bug in snd-cs46xx.
No information is output to dmesg when this happens, on either the Gentoo-patched 2.6.29 kernel, or the vanilla 2.6.30.5 kernel.
If you need any further information, please let me know! As stated above, I'm willing to apply patches to the vanilla sources of the latest stable kernel in order to test them. I don't have experience of downloading the ALSA drivers and compiling them separately but I could also do that if it's needed.
Thank you,
- Sophie.
At Sat, 5 Sep 2009 13:24:04 +0100, Sophie Hamilton wrote:
Hi there,
I was pointed here by Audacious developers, who believe I've found a bug in the snd-cs46xx driver, which I use for my sound card (a Turtle Beach Santa Cruz, whose PCI ID is 1013:6003).
Audacious, as of version 2.1, uses the default period time offered by ALSA and limits itself to the "safe API" as per Lennart Poettering.
Hm, I don't know of "safe API"...
(snip)
Most applications that I have seem to work properly with regards to audio, although I've heard that this is probably because ALSA's safe API isn't very well documented and as such most applications will not be using it, thus not exposing the bug in snd-cs46xx.
No information is output to dmesg when this happens, on either the Gentoo-patched 2.6.29 kernel, or the vanilla 2.6.30.5 kernel.
If you need any further information, please let me know!
A small test case, preferably a short C program just to reproduce the problem would be really needed in such a case. It's very hard to guess what's going on and what is actually wrong in the driver only from your problem description, because of no obvious debug logs.
thanks,
Takashi
On Mon, 2009-09-07 at 14:32 +0200, Takashi Iwai wrote:
Hm, I don't know of "safe API"...
It is described here: http://0pointer.de/blog/projects/guide-to-sound-apis.html
Should this be incomplete or otherwise incorrect it would be useful to have a rebuttal.
A small test case, preferably a short C program just to reproduce the problem would be really needed in such a case. It's very hard to guess what's going on and what is actually wrong in the driver only from your problem description, because of no obvious debug logs.
If it is anything like the Aureal Vortex bug, the minimum period size doesn't make sense. Unlike the older the ALSA plugin, the new ALSA-NG plugin in Audacious opens the audio device with defaults and does not attempt to set period sizes. The code is available here: http://hg.atheme.org/audacious-plugins/file/c44c39dd30b5/src/alsa-ng
thanks, Takashi
Regards, Tony V.
audacious.i386 1.5.1-9.fc10 which use the old plugin did not play any music on my au8830 even with the patch and select hw:0,0 in the output plugin perference
CHECK (snd_pcm_hw_params_set_access, alsa_handle, params, http://hg.atheme.org/audacious-plugins/file/c44c39dd30b5/src/alsa-gapless/alsa.c#l159SND_PCM_ACCESS_RW_INTERLEAVED);
CHECK (snd_pcm_hw_params_set_format, alsa_handle, params, format); CHECK (snd_pcm_hw_params_set_channels, alsa_handle, params, channels); CHECK (snd_pcm_hw_params_set_rate, alsa_handle, params, rate, 0); CHECK (snd_pcm_hw_params_set_buffer_time_min, alsa_handle, params, & time, 0); CHECK (snd_pcm_hw_params, alsa_handle, params); CHECK (snd_pcm_prepare, alsa_handle);
alsa_format = format; alsa_channels = channels; alsa_rate = rate;
alsa_buffer_length = snd_pcm_frames_to_bytes (alsa_handle, (gint64) LARGE_BUFFER * rate / 1000);
If you are using SND_CS46XX_NEW_DSP which support multiple stream, the driver has a constraint on the buffer size (don't expect that you can get exactly a one second or 0.1 second buffer for all alsa drivers )
#ifdef CONFIG_SND_CS46XX_NEW_DSP
static unsigned int period_sizes[] = { 32, 64, 128, 256, 512, 1024, 2048 };
static struct snd_pcm_hw_constraint_list hw_constraints_period_sizes = { .count = ARRAY_SIZE(period_sizes), .list = period_sizes, .mask = 0 };
#endif
The documentation did not mention that you can get a default value if you did not set the period time , buffer_time
http://www.alsa-project.org/alsa-doc/alsa-lib/group___p_c_m.html#g98ba19d280...
snd_pcm_hw_params()
The configuration is chosen fixing single parameters in this order: first access, first format, first subformat, min channels, min rate, min period time, max buffer size, min tick time
After this call, snd_pcm_prepare()http://www.alsa-project.org/alsa-doc/alsa-lib/group___p_c_m.html#g692ad9e5902d0623b56a0decee0fa686is called automatically and the stream is brought to SND_PCM_STATE_PREPAREDhttp://www.alsa-project.org/alsa-doc/alsa-lib/group___p_c_m.html#ggfff41b69676ea4a7efa64dd5642505ddfa179425d00d012af07ef15e1c066ea6state. The hardware parameters cannot be changed when the stream is running (active). The software parameters can be changed at any tim
2009/9/7 Tony Vroon tony@linx.net
On Mon, 2009-09-07 at 14:32 +0200, Takashi Iwai wrote:
Hm, I don't know of "safe API"...
It is described here: http://0pointer.de/blog/projects/guide-to-sound-apis.html
Should this be incomplete or otherwise incorrect it would be useful to have a rebuttal.
A small test case, preferably a short C program just to reproduce the problem would be really needed in such a case. It's very hard to guess what's going on and what is actually wrong in the driver only from your problem description, because of no obvious debug logs.
If it is anything like the Aureal Vortex bug, the minimum period size doesn't make sense. Unlike the older the ALSA plugin, the new ALSA-NG plugin in Audacious opens the audio device with defaults and does not attempt to set period sizes. The code is available here: http://hg.atheme.org/audacious-plugins/file/c44c39dd30b5/src/alsa-ng
thanks, Takashi
Regards, Tony V.
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Hi,
at the moment I'm trying to implement a custom driver which will use SPI to transfer the PCM samples to a speech codec chip.
Since I guess that I will also have some problems with getting the SPI timing right for the speech codec chip, I'm planning to bitbang the data. For this reason in the driver I'm using non-dma transfers and the copy/silence callbacks.
However, with my current code the content of the PCM buffer is only copied a few times. After that it seems to get stuck followed by the error: ALSA sound/core/pcm_native.c:1499: playback drain error (DMA or IRQ trouble?)
The code for the driver is available on pastebin here: http://pastebin.com/m110336f0
Why do I get the drain error, why does the PCM sample transfer get stuck ?
cheers, stefan
At Mon, 07 Sep 2009 14:29:15 +0100, Tony Vroon wrote:
[1 <text/plain (quoted-printable)>] On Mon, 2009-09-07 at 14:32 +0200, Takashi Iwai wrote:
Hm, I don't know of "safe API"...
It is described here: http://0pointer.de/blog/projects/guide-to-sound-apis.html
Should this be incomplete or otherwise incorrect it would be useful to have a rebuttal.
Thanks for the pointer.
A small test case, preferably a short C program just to reproduce the problem would be really needed in such a case. It's very hard to guess what's going on and what is actually wrong in the driver only from your problem description, because of no obvious debug logs.
If it is anything like the Aureal Vortex bug, the minimum period size doesn't make sense.
Yes, cs46xx also sets the minimum bytes 1, and this can be the same problem as aureal driver.
(BTW, this isn't exactly a driver "bug" -- if this word is meaning a programming error. periods_size = 1 is no invalid setup in theory. But, it's rarely a sane setup in reality, so it should be fixed to match with the real machines indeed.)
Unlike the older the ALSA plugin, the new ALSA-NG plugin in Audacious opens the audio device with defaults and does not attempt to set period sizes.
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.
Takashi
do you mean that the function cannot update the min and max value ?
snd_pcm_hw_constraint_list(runtime, 0, SNDRV_PCM_HW_PARAM_PERIOD_BYTES, &hw_constraints_period_sizes);
2009/9/7 Takashi Iwai tiwai@suse.de
At Mon, 07 Sep 2009 14:29:15 +0100, Tony Vroon wrote:
[1 <text/plain (quoted-printable)>] On Mon, 2009-09-07 at 14:32 +0200, Takashi Iwai wrote:
Hm, I don't know of "safe API"...
It is described here: http://0pointer.de/blog/projects/guide-to-sound-apis.html
Should this be incomplete or otherwise incorrect it would be useful to have a rebuttal.
Thanks for the pointer.
A small test case, preferably a short C program just to reproduce the problem would be really needed in such a case. It's very hard to guess what's going on and what is actually wrong in the driver only from your problem description, because of no obvious debug logs.
If it is anything like the Aureal Vortex bug, the minimum period size doesn't make sense.
Yes, cs46xx also sets the minimum bytes 1, and this can be the same problem as aureal driver.
(BTW, this isn't exactly a driver "bug" -- if this word is meaning a programming error. periods_size = 1 is no invalid setup in theory. But, it's rarely a sane setup in reality, so it should be fixed to match with the real machines indeed.)
Unlike the older the ALSA plugin, the new ALSA-NG plugin in Audacious opens the audio device with defaults and does not attempt to set period sizes.
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.
Takashi _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
At Mon, 7 Sep 2009 23:07:14 +0800, Raymond Yau wrote:
do you mean that the function cannot update the min and max value ?
snd_pcm_hw_constraint_list(runtime, 0, SNDRV_PCM_HW_PARAM_PERIOD_BYTES, &hw_constraints_period_sizes);
This of course influences on the buffer min/max sizes. But, this has nothing to do with the order of parameter determination at snd_pcm_hw_params() call.
The PCM core determines the parameters in the order of period size, then the buffer size. That is, first choose the period size to the minimum unless explicitly set, then choose the buffer size to the max unless explicitly set. Since the max number of periods is 1024 for cs46xx, it'd choose period_size = 1, then buffer_size = 1024.
So, the question is what the app expects. If the app requires the smaller period size, then this set up is OK (of course, it'd be better to check the min period and fix it somehow). OTOH, if the app prefers larger buffer sizes, then it'd be better to set the buffer size as the max first before calling snd_pcm_hw_params().
Takashi
2009/9/7 Takashi Iwai tiwai@suse.de
At Mon, 07 Sep 2009 14:29:15 +0100, Tony Vroon wrote:
[1 <text/plain (quoted-printable)>] On Mon, 2009-09-07 at 14:32 +0200, Takashi Iwai wrote:
Hm, I don't know of "safe API"...
It is described here: http://0pointer.de/blog/projects/guide-to-sound-apis.html
Should this be incomplete or otherwise incorrect it would be useful to have a rebuttal.
Thanks for the pointer.
A small test case, preferably a short C program just to reproduce the problem would be really needed in such a case. It's very hard to guess what's going on and what is actually wrong in the driver only from your problem description, because of no obvious debug logs.
If it is anything like the Aureal Vortex bug, the minimum period size doesn't make sense.
Yes, cs46xx also sets the minimum bytes 1, and this can be the same problem as aureal driver.
(BTW, this isn't exactly a driver "bug" -- if this word is meaning a programming error. periods_size = 1 is no invalid setup in theory. But, it's rarely a sane setup in reality, so it should be fixed to match with the real machines indeed.)
Unlike the older the ALSA plugin, the new ALSA-NG plugin in Audacious opens the audio device with defaults and does not attempt to set period sizes.
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.
Takashi _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On Mon, 07.09.09 17:01, Takashi Iwai (tiwai@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.
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.
(Hmm, and while we are at it. Ssome other driver I don't posess, 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...)
Lennart
On 9/7/09, Lennart Poettering mznyfn@0pointer.de wrote:
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.
I don't know if this is relevant, but in the course of tracking down the bug which led to me creating the patch for the cs46xx driver, I downloaded the latest alsa-utils to play with the aplay source. It turned out that I could comment out either the period-setting block (which called either _set_period_time_near or _set_period_size_near) *or* the buffer-setting block (which called either _set_buffer_time_near or _set_buffer_size_near) without experiencing any ill effects on sound. It was only when I commented out both blocks that I experienced issues with the sound.
This may be obvious to people here, but being new to ALSA programming (this is literally my first excursion into it), it strikes me that this could be significant. Does it help you at all?
- Sophie.
At Tue, 8 Sep 2009 00:47:00 +0200, Lennart Poettering wrote:
On Mon, 07.09.09 17:01, Takashi Iwai (tiwai@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 };
On 9/8/09, Takashi Iwai tiwai@suse.de wrote:
Sophie, what happens if you use the patch without your change?
Same thing as before, it seems; it doesn't seem to make any difference. I tested all the use cases I tested beforehand and none were any different.
- Sophie.
At Tue, 8 Sep 2009 08:46:36 +0100, Sophie Hamilton wrote:
On 9/8/09, Takashi Iwai tiwai@suse.de wrote:
Sophie, what happens if you use the patch without your change?
Same thing as before, it seems; it doesn't seem to make any difference. I tested all the use cases I tested beforehand and none were any different.
OK, interesting. I'd need to check deeply inside.
Takashi
At Tue, 08 Sep 2009 10:53:39 +0200, I wrote:
At Tue, 8 Sep 2009 08:46:36 +0100, Sophie Hamilton wrote:
On 9/8/09, Takashi Iwai tiwai@suse.de wrote:
Sophie, what happens if you use the patch without your change?
Same thing as before, it seems; it doesn't seem to make any difference. I tested all the use cases I tested beforehand and none were any different.
OK, interesting. I'd need to check deeply inside.
It turned out that the hw_params determination is done rather inside the alsa-lib beforehand.
Try the patch below. This will give you larger buffer size with audacious. (And it will work without the driver changes.)
For any apps that require the old behavior, I added the check of $LIBASOUND_COMPAT variable.
Takashi
--- diff --git a/src/pcm/pcm_params.c b/src/pcm/pcm_params.c index 80b3fd2..0e1c3fc 100644 --- a/src/pcm/pcm_params.c +++ b/src/pcm/pcm_params.c @@ -1081,6 +1081,7 @@ int snd_pcm_hw_param_never_eq(const snd_pcm_hw_params_t *params, static int snd_pcm_hw_params_choose(snd_pcm_t *pcm, snd_pcm_hw_params_t *params) { int err; + const char *compat = getenv("LIBASOUND_COMPAT"); #ifdef CHOOSE_DEBUG snd_output_t *log; snd_output_stdio_attach(&log, stderr, 0); @@ -1103,15 +1104,29 @@ static int snd_pcm_hw_params_choose(snd_pcm_t *pcm, snd_pcm_hw_params_t *params) err = snd_pcm_hw_param_set_first(pcm, params, SND_PCM_HW_PARAM_RATE, NULL, 0); if (err < 0) return err; - err = snd_pcm_hw_param_set_first(pcm, params, SND_PCM_HW_PARAM_PERIOD_TIME, NULL, 0); - if (err < 0) - return err; - err = snd_pcm_hw_param_set_first(pcm, params, SND_PCM_HW_PARAM_PERIOD_SIZE, NULL, 0); - if (err < 0) - return err; - err = snd_pcm_hw_param_set_last(pcm, params, SND_PCM_HW_PARAM_BUFFER_SIZE, NULL, 0); - if (err < 0) - return err; + if (compat && *compat) { + /* old mode */ + err = snd_pcm_hw_param_set_first(pcm, params, SND_PCM_HW_PARAM_PERIOD_TIME, NULL, 0); + if (err < 0) + return err; + err = snd_pcm_hw_param_set_first(pcm, params, SND_PCM_HW_PARAM_PERIOD_SIZE, NULL, 0); + if (err < 0) + return err; + err = snd_pcm_hw_param_set_last(pcm, params, SND_PCM_HW_PARAM_BUFFER_SIZE, NULL, 0); + if (err < 0) + return err; + } else { + /* determine buffer size first */ + err = snd_pcm_hw_param_set_last(pcm, params, SND_PCM_HW_PARAM_BUFFER_SIZE, NULL, 0); + if (err < 0) + return err; + err = snd_pcm_hw_param_set_first(pcm, params, SND_PCM_HW_PARAM_PERIOD_SIZE, NULL, 0); + if (err < 0) + return err; + err = snd_pcm_hw_param_set_first(pcm, params, SND_PCM_HW_PARAM_PERIOD_TIME, NULL, 0); + if (err < 0) + return err; + } err = snd_pcm_hw_param_set_first(pcm, params, SND_PCM_HW_PARAM_TICK_TIME, NULL, 0); if (err < 0) return err;
On 9/9/09, Takashi Iwai tiwai@suse.de wrote:
It turned out that the hw_params determination is done rather inside the alsa-lib beforehand.
Try the patch below. This will give you larger buffer size with audacious. (And it will work without the driver changes.)
For any apps that require the old behavior, I added the check of $LIBASOUND_COMPAT variable.
Since this patch is for alsa-lib and not for the kernel, this forced me to check what version I was currently using, which turned out to be 1.0.20. (specifically, media-libs/alsa-lib-1.0.20-r1) 1.0.21 hadn't yet been keyworded on my architecture (x86) as stable.
So for my first test, I forced 1.0.21 to install anyway, then made sure the 2.6.30.5 kernel I was using was unpatched, then rebooted to test whether the upgrade to 1.0.21 (unpatched) would have made any difference anyway. It didn't; the sound was the same as it was before.
Then, I copied the alsa-lib ebuild to a local Gentoo repository and modified it to apply your patch. (I don't know what tricks Gentoo might have up its sleeve for components as core as this, so I didn't feel comfortable taking the raw source. It shouldn't made a difference, but if you'd prefer me to try, I can see what I can do. I can't promise that any problems would be necessarily from this, though.) I compiled and installed the new patched 1.0.21, and rebooted.
I can confirm now that Audacious does indeed play correctly where it didn't before. However, using mplayer with the "-ao openal" switch still doesn't play correctly - in fact, it sounds the same as before - so it looks like OpenAL is actually doing things slightly differently than I thought. :/
As a final test, I removed the patch from alsa-lib and re-patched the kernel with my patch to change the minimum period size to 64 for cs46xx, so that I could check whether it would still work with the new alsa-lib 1.0.21 (rather than the 1.0.20 that I was using before). It does indeed work, in both Audacious and OpenAL.
So... I'm not sure what's up with OpenAL in this case. Is there anything more I can do to help?
- Sophie.
At Wed, 9 Sep 2009 13:29:02 +0100, Sophie Hamilton wrote:
On 9/9/09, Takashi Iwai tiwai@suse.de wrote:
It turned out that the hw_params determination is done rather inside the alsa-lib beforehand.
Try the patch below. This will give you larger buffer size with audacious. (And it will work without the driver changes.)
For any apps that require the old behavior, I added the check of $LIBASOUND_COMPAT variable.
Since this patch is for alsa-lib and not for the kernel, this forced me to check what version I was currently using, which turned out to be 1.0.20. (specifically, media-libs/alsa-lib-1.0.20-r1) 1.0.21 hadn't yet been keyworded on my architecture (x86) as stable.
This should work in both version since the relevant code hasn't been change for very long time.
So for my first test, I forced 1.0.21 to install anyway, then made sure the 2.6.30.5 kernel I was using was unpatched, then rebooted to test whether the upgrade to 1.0.21 (unpatched) would have made any difference anyway. It didn't; the sound was the same as it was before.
Then, I copied the alsa-lib ebuild to a local Gentoo repository and modified it to apply your patch. (I don't know what tricks Gentoo might have up its sleeve for components as core as this, so I didn't feel comfortable taking the raw source. It shouldn't made a difference, but if you'd prefer me to try, I can see what I can do. I can't promise that any problems would be necessarily from this, though.) I compiled and installed the new patched 1.0.21, and rebooted.
I can confirm now that Audacious does indeed play correctly where it didn't before. However, using mplayer with the "-ao openal" switch still doesn't play correctly - in fact, it sounds the same as before - so it looks like OpenAL is actually doing things slightly differently than I thought. :/
Yes, likely. The app like openal is usually more sensible regarding latency, so "safe API" described there wasn't appropriate at all.
As a final test, I removed the patch from alsa-lib and re-patched the kernel with my patch to change the minimum period size to 64 for cs46xx, so that I could check whether it would still work with the new alsa-lib 1.0.21 (rather than the 1.0.20 that I was using before). It does indeed work, in both Audacious and OpenAL.
Or, openal is using OSS? You can see /proc/asound/card0/pcm0p/hw_params whether it's OSS emulation mode or not. In OSS mode, OSS parameters are shown there in addition.
So... I'm not sure what's up with OpenAL in this case. Is there anything more I can do to help?
Thanks, that's enough for now. I pushed the patch to alsa-lib git tree now.
Takashi
On Wed, 09.09.09 14:35, Takashi Iwai (tiwai@suse.de) wrote:
I can confirm now that Audacious does indeed play correctly where it didn't before. However, using mplayer with the "-ao openal" switch still doesn't play correctly - in fact, it sounds the same as before - so it looks like OpenAL is actually doing things slightly differently than I thought. :/
Yes, likely. The app like openal is usually more sensible regarding latency, so "safe API" described there wasn't appropriate at all.
Hmm, so are you suggesting I should change that little text about the safe API subset I wrote?
So, users should always set first the buffer size, followed by the period size, is that correct? And if that fails, try the other way round, and if that fails set buffer size only? And if that fails set nothing?
Lennart
At Wed, 9 Sep 2009 16:07:35 +0200, Lennart Poettering wrote:
On Wed, 09.09.09 14:35, Takashi Iwai (tiwai@suse.de) wrote:
I can confirm now that Audacious does indeed play correctly where it didn't before. However, using mplayer with the "-ao openal" switch still doesn't play correctly - in fact, it sounds the same as before - so it looks like OpenAL is actually doing things slightly differently than I thought. :/
Yes, likely. The app like openal is usually more sensible regarding latency, so "safe API" described there wasn't appropriate at all.
Hmm, so are you suggesting I should change that little text about the safe API subset I wrote?
Maybe a bit more addition would be helpful. The realtime apps do care the latency. So, obviously it's not the target of your description.
So, users should always set first the buffer size, followed by the period size, is that correct?
Yes, in general, this order gives more chance for a larger buffer size. But, if you specify both buffer and period sizes, there shouldn't be much difference. Specifying the buffer size is especially good if you don't give any period size.
The situation is improved now with 1.0.21a since I changed the determination order in alsa-lib. You'll get the largest buffer size as default with 1.0.21a (let's see whether this gives any regressions ;) But, if portability matters, setting thebuffer size would be safer.
And if that fails, try the other way round, and if that fails set buffer size only? And if that fails set nothing?
Yes, this sounds reasonable.
thanks,
Takashi
On Wed, 09.09.09 16:14, Takashi Iwai (tiwai@suse.de) wrote:
I can confirm now that Audacious does indeed play correctly where it didn't before. However, using mplayer with the "-ao openal" switch still doesn't play correctly - in fact, it sounds the same as before - so it looks like OpenAL is actually doing things slightly differently than I thought. :/
Yes, likely. The app like openal is usually more sensible regarding latency, so "safe API" described there wasn't appropriate at all.
Hmm, so are you suggesting I should change that little text about the safe API subset I wrote?
Maybe a bit more addition would be helpful. The realtime apps do care the latency. So, obviously it's not the target of your description.
What I wrote is actually this:
"Do not touch buffering/period metrics unless you have specific latency needs. Develop defensively, handling correctly the case when the backend cannot fulfill your buffering metrics requests. Be aware that the buffering metrics of the playback buffer only indirectly influence the overall latency in many cases. i.e. setting the buffer size to a fixed value might actually result in practical latencies that are much higher."
i.e. it already says "... unless you have specific latency needs". The point of this is that RT apps should of course set the buffer size, however stuff like media players where the latency does not matter at all should not request artificially low latencies and thus decrease battery time needlessly.
So, users should always set first the buffer size, followed by the period size, is that correct?
Yes, in general, this order gives more chance for a larger buffer size. But, if you specify both buffer and period sizes, there shouldn't be much difference. Specifying the buffer size is especially good if you don't give any period size.
The situation is improved now with 1.0.21a since I changed the determination order in alsa-lib. You'll get the largest buffer size as default with 1.0.21a (let's see whether this gives any regressions ;) But, if portability matters, setting thebuffer size would be safer.
This sounds good to me. Defaulting to large buffer sizes is certainly good for power consumption.
Lennart
At Wed, 9 Sep 2009 16:27:17 +0200, Lennart Poettering wrote:
On Wed, 09.09.09 16:14, Takashi Iwai (tiwai@suse.de) wrote:
I can confirm now that Audacious does indeed play correctly where it didn't before. However, using mplayer with the "-ao openal" switch still doesn't play correctly - in fact, it sounds the same as before - so it looks like OpenAL is actually doing things slightly differently than I thought. :/
Yes, likely. The app like openal is usually more sensible regarding latency, so "safe API" described there wasn't appropriate at all.
Hmm, so are you suggesting I should change that little text about the safe API subset I wrote?
Maybe a bit more addition would be helpful. The realtime apps do care the latency. So, obviously it's not the target of your description.
What I wrote is actually this:
"Do not touch buffering/period metrics unless you have specific latency needs. Develop defensively, handling correctly the case when the backend cannot fulfill your buffering metrics requests. Be aware that the buffering metrics of the playback buffer only indirectly influence the overall latency in many cases. i.e. setting the buffer size to a fixed value might actually result in practical latencies that are much higher."i.e. it already says "... unless you have specific latency needs". The point of this is that RT apps should of course set the buffer size, however stuff like media players where the latency does not matter at all should not request artificially low latencies and thus decrease battery time needlessly.
Well, basically this belongs to the implementation detail. Unless you have a solid RT kernel, the latency can't be guaranteed. The old good way to achieve the smooth playback is to use the large buffer with the small periods. Then you have a better chance to be woken up before buffer underruns. That's how the ALSA default setup is done, even with the latest version.
Of course, in this scenario, the battery life wasn't taken into account. If you care the battery life, too, the simplest solution is to have two periods in the largest buffer size. But, this doesn't fit with every programming model, too (imagine an old app that don't do multi-threading :)
Takashi
Is "one period per buffer" really work ?
There are quite a few drivers which has "periods_min = 1" (e.g. emu10k1, intel8x0, .....
however aplay did not like this
"Can't use period equal to buffer size (%lu == %lu)")"
2009/9/9 Takashi Iwai tiwai@suse.de
At Wed, 9 Sep 2009 16:27:17 +0200, Lennart Poettering wrote:
On Wed, 09.09.09 16:14, Takashi Iwai (tiwai@suse.de) wrote:
I can confirm now that Audacious does indeed play correctly where
it
didn't before. However, using mplayer with the "-ao openal"
switch
still doesn't play correctly - in fact, it sounds the same as
before -
so it looks like OpenAL is actually doing things slightly
differently
than I thought. :/
Yes, likely. The app like openal is usually more sensible
regarding
latency, so "safe API" described there wasn't appropriate at all.
Hmm, so are you suggesting I should change that little text about the safe API subset I wrote?
Maybe a bit more addition would be helpful. The realtime apps do care the latency. So, obviously it's not the target of your description.
What I wrote is actually this:
"Do not touch buffering/period metrics unless you have specific latency needs. Develop defensively, handling correctly the case when the backend cannot fulfill your buffering metrics requests. Be aware that the buffering metrics of the playback buffer only indirectly influence the overall latency in many cases. i.e. setting the buffer size to a fixed value might actually result in practical latencies that are much higher."i.e. it already says "... unless you have specific latency needs". The point of this is that RT apps should of course set the buffer size, however stuff like media players where the latency does not matter at all should not request artificially low latencies and thus decrease battery time needlessly.
Well, basically this belongs to the implementation detail. Unless you have a solid RT kernel, the latency can't be guaranteed. The old good way to achieve the smooth playback is to use the large buffer with the small periods. Then you have a better chance to be woken up before buffer underruns. That's how the ALSA default setup is done, even with the latest version.
Of course, in this scenario, the battery life wasn't taken into account. If you care the battery life, too, the simplest solution is to have two periods in the largest buffer size. But, this doesn't fit with every programming model, too (imagine an old app that don't do multi-threading :)
Takashi _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On Tue, 08.09.09 08:29, Takashi Iwai (tiwai@suse.de) wrote:
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.
I always use 'frames' as unit for the actual calls, even if I said bytes.
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:
- set access, format, channels and rate first,
- if you need larger buffers than shorter periods, set the buffer
size first, 3. set the period size only when you must specify it
This breaks on ice1712 at least...
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.
For me the large buffers matter most. And large periods are the second most important thing. Would something like the following make sense?
<snip> snd_pcm_hw_params_any(pcm, hw); snd_pcm_hw_params_set_access(pcm, hw, ...); snd_pcm_hw_params_set_format(pcm, hw, ...); snd_pcm_hw_params_set_rate_near(pcm, hw, ...); snd_pcm_hw_params_set_channels_near(pcm, hw, ...);
snd_pcm_hw_params_copy(hw2, hw);
/* We care more about buffer size than the period size, so try setting things in this order first */ snd_pcm_hw_params_set_buffer_size_near(hw, ...); snd_pcm_hw_params_set_periods_near(hw, ...);
if (snd_pcm_hw_params(pcm, hw) < 0) { /* This order didn't work, so let's try it the other way round */ snd_pcm_hw_params_set_periods_near(hw2, ...); snd_pcm_hw_params_set_buffer_size_near(hw2, ...);
if (snd_pcm_hw_params(pcm, hw2) < 0) { /* fail fatally */ .... } } </snip>
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.
I only actually manipulate samples here, not bytes. So the prob is that if you ask for a buffer size of n samples it will only agree to n-1 samples, for every possible n. Other drivers don't do that.
Lennart
At Tue, 8 Sep 2009 15:38:25 +0200, Lennart Poettering wrote:
On Tue, 08.09.09 08:29, Takashi Iwai (tiwai@suse.de) wrote:
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.
I always use 'frames' as unit for the actual calls, even if I said bytes.
It's not about what unit you use. It's the fact that three units exist in the hw_params space parallel and they have to be aligned with each other. See ens1371 example below.
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:
- set access, format, channels and rate first,
- if you need larger buffers than shorter periods, set the buffer
size first, 3. set the period size only when you must specify it
This breaks on ice1712 at least...
Might be.
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.
For me the large buffers matter most. And large periods are the second most important thing. Would something like the following make sense?
<snip> snd_pcm_hw_params_any(pcm, hw); snd_pcm_hw_params_set_access(pcm, hw, ...); snd_pcm_hw_params_set_format(pcm, hw, ...); snd_pcm_hw_params_set_rate_near(pcm, hw, ...); snd_pcm_hw_params_set_channels_near(pcm, hw, ...);
snd_pcm_hw_params_copy(hw2, hw);
/* We care more about buffer size than the period size, so try setting things in this order first */ snd_pcm_hw_params_set_buffer_size_near(hw, ...); snd_pcm_hw_params_set_periods_near(hw, ...);
if (snd_pcm_hw_params(pcm, hw) < 0) { /* This order didn't work, so let's try it the other way round */ snd_pcm_hw_params_set_periods_near(hw2, ...); snd_pcm_hw_params_set_buffer_size_near(hw2, ...);
if (snd_pcm_hw_params(pcm, hw2) < 0) { /* fail fatally */ .... } }
</snip>
I think yes. But needs more testing of course :)
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.
I only actually manipulate samples here, not bytes. So the prob is that if you ask for a buffer size of n samples it will only agree to n-1 samples, for every possible n. Other drivers don't do that.
Then the problem is likely the sample rate setup. ens1371 can't give you always the integer sample rate. Now the problem of multiple units appears here like a ghost. Almost every parameter is associated with other parameters in the end due to the constraints below: buffer_time = buffer_size / sample_rate buffer_size = buffer_bytes / (channels * format_width)
When you get a non-integer rate value, even buffer_size can be affected, rounded down to the next integer value.
All for one, one for all -- what a perfect world.
thanks,
Takashi
2009/9/8 Takashi Iwai tiwai@suse.de
At Tue, 8 Sep 2009 15:38:25 +0200, Lennart Poettering wrote:
On Tue, 08.09.09 08:29, Takashi Iwai (tiwai@suse.de) wrote:
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
it's rather common e.g HDA also have
snd_pcm_hw_constraint_integer(runtime, SNDRV_PCM_HW_PARAM_PERIODS); snd_pcm_hw_constraint_step(runtime, 0, SNDRV_PCM_HW_PARAM_BUFFER_BYTES, 128); snd_pcm_hw_constraint_step(runtime, 0, SNDRV_PCM_HW_PARAM_PERIOD_BYTES, 128);
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.
I always use 'frames' as unit for the actual calls, even if I said bytes.
It's not about what unit you use. It's the fact that three units exist in the hw_params space parallel and they have to be aligned with each other. See ens1371 example below.
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:
- set access, format, channels and rate first,
- if you need larger buffers than shorter periods, set the buffer
size first, 3. set the period size only when you must specify it
This breaks on ice1712 at least...
Might be.
The front device of ice1712 has a route plugin
you may need to use hw:0,0 if you are using mmap_begin, .... and read/write 10 and 12 channels like jackd
Actually the front device of emu10k1 has two IFACE_PCM controls , most likely you will need to modify EMU10K1.conf so that capture do not hook those controls
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.
For me the large buffers matter most. And large periods are the second most important thing. Would something like the following make sense?
<snip> snd_pcm_hw_params_any(pcm, hw); snd_pcm_hw_params_set_access(pcm, hw, ...); snd_pcm_hw_params_set_format(pcm, hw, ...); snd_pcm_hw_params_set_rate_near(pcm, hw, ...); snd_pcm_hw_params_set_channels_near(pcm, hw, ...);
snd_pcm_hw_params_copy(hw2, hw);
/* We care more about buffer size than the period size, so try setting things in this order first */ snd_pcm_hw_params_set_buffer_size_near(hw, ...); snd_pcm_hw_params_set_periods_near(hw, ...);
if (snd_pcm_hw_params(pcm, hw) < 0) { /* This order didn't work, so let's try it the other way round */ snd_pcm_hw_params_set_periods_near(hw2, ...); snd_pcm_hw_params_set_buffer_size_near(hw2, ...);
if (snd_pcm_hw_params(pcm, hw2) < 0) { /* fail fatally */ .... } }
</snip>
I think yes. But needs more testing of course :)
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.
I only actually manipulate samples here, not bytes. So the prob is that if you ask for a buffer size of n samples it will only agree to n-1 samples, for every possible n. Other drivers don't do that.
Then the problem is likely the sample rate setup. ens1371 can't give you always the integer sample rate. Now the problem of multiple units appears here like a ghost. Almost every parameter is associated with other parameters in the end due to the constraints below: buffer_time = buffer_size / sample_rate buffer_size = buffer_bytes / (channels * format_width)
When you get a non-integer rate value, even buffer_size can be affected, rounded down to the next integer value.
All for one, one for all -- what a perfect world.
thanks,
Takashi _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On 9/7/09, Takashi Iwai tiwai@suse.de wrote:
A small test case, preferably a short C program just to reproduce the problem would be really needed in such a case. It's very hard to guess what's going on and what is actually wrong in the driver only from your problem description, because of no obvious debug logs.
I can go one better... I have a fix. :)
I decided to go investigating myself and discovered that the cs46xx driver has a silly minimum period size declared of 1. Increasing this to 256 fixes the problem, though I haven't yet tested to see if lower values will work too. (I'll do this and regenerate the patch later.)
The patch, based on the latest stable kernel tree (2.6.30.5), is attached, though in case the mailing list strips attachments, it's also available at http://neo.theblob.org/cs46xx-fix.patch .
If I need to submit the patch in some other way when it comes to actually submitting the final thing, please let me know.
- Sophie.
On 9/7/09, Sophie Hamilton sophie-alsa@theblob.org wrote:
I decided to go investigating myself and discovered that the cs46xx driver has a silly minimum period size declared of 1. Increasing this to 256 fixes the problem, though I haven't yet tested to see if lower values will work too. (I'll do this and regenerate the patch later.)
Turns out that a value of 64 is the optimum value. I've regenerated the patch, and attached it again. (And since I know now that this list doesn't strip attachments, I didn't upload the new one.)
This should be the final patch. How should I go about submitting this?
- Sophie.
At Mon, 7 Sep 2009 18:04:12 +0100, Sophie Hamilton wrote:
On 9/7/09, Sophie Hamilton sophie-alsa@theblob.org wrote:
I decided to go investigating myself and discovered that the cs46xx driver has a silly minimum period size declared of 1. Increasing this to 256 fixes the problem, though I haven't yet tested to see if lower values will work too. (I'll do this and regenerate the patch later.)
Turns out that a value of 64 is the optimum value.
How did you determine it ? :)
Well, I find also 64 is a sane value, but it's not always logical. In most cases, 32 is used as the minimum value due to PCI h/w limitation. But 32 is very hard to achieve, so it doesn't matter so much.
I've regenerated the patch, and attached it again. (And since I know now that this list doesn't strip attachments, I didn't upload the new one.)
This should be the final patch. How should I go about submitting this?
Please give a proper patch summary, too. Also, it'd be more helpful if you give an example what actually your patch fixes (e.g. audacious, etc).
thanks,
Takashi
On 9/7/09, Takashi Iwai tiwai@suse.de wrote:
At Mon, 7 Sep 2009 18:04:12 +0100, Sophie Hamilton wrote:
On 9/7/09, Sophie Hamilton sophie-alsa@theblob.org wrote:
I decided to go investigating myself and discovered that the cs46xx driver has a silly minimum period size declared of 1. Increasing this to 256 fixes the problem, though I haven't yet tested to see if lower values will work too. (I'll do this and regenerate the patch later.)
Turns out that a value of 64 is the optimum value.
How did you determine it ? :)
Well, I have the actual hardware - at least, one of the chips it supports - which is how I got involved in this bug in the first place. (The Turtle Beach Santa Cruz uses a CS4630.) A value of 32 didn't work when the default period side from ALSA is used; the next highest power of two, 64, does. As all the values I've seen in the kernel for the minimum period size are powers of two, I'm assuming that this is the lowest it can be. (I don't know much about ALSA, bear in mind; this is my first venture into ALSA programming *and* kernel patches.)
Well, I find also 64 is a sane value, but it's not always logical. In most cases, 32 is used as the minimum value due to PCI h/w limitation. But 32 is very hard to achieve, so it doesn't matter so much.
Like I said, I have the hardware, and I can tell you that on my hardware, 32 doesn't work properly, while 64 does. :)
This should be the final patch. How should I go about submitting this?
Please give a proper patch summary, too. Also, it'd be more helpful if you give an example what actually your patch fixes (e.g. audacious, etc).
I'm not sure what you mean by a "proper patch summary". Is there anywhere I should read that specifies the format of a proper patch summary?
As for what it fixes, it fixes a problem in the case where neither a period size nor a buffer size is passed to ALSA, instead using the defaults provided. This is the recommended course of action according to a guide to safe ALSA usage by Lennart Poettering - see http://0pointer.de/blog/projects/guide-to-sound-apis.html . Among other things, the guide says that in order for an app to remain safe and playable on all backends that ALSA supports, it should "not touch buffering/period metrics unless you have specific latency needs".
This guide is already being followed by various apps and audio interfaces - Audacious is one of them, and OpenAL (a popular audio interface used by many games including Second Life and Unreal Tournament) is another. As such, there are quite a few examples where the current buggy behaviour can be observed on a card that the cs46xx driver serves. (I don't know of a full list; as I said above, I'm new to ALSA programming.)
Does this help?
- Sophie.
At Mon, 7 Sep 2009 20:06:37 +0100, Sophie Hamilton wrote:
On 9/7/09, Takashi Iwai tiwai@suse.de wrote:
At Mon, 7 Sep 2009 18:04:12 +0100, Sophie Hamilton wrote:
On 9/7/09, Sophie Hamilton sophie-alsa@theblob.org wrote:
I decided to go investigating myself and discovered that the cs46xx driver has a silly minimum period size declared of 1. Increasing this to 256 fixes the problem, though I haven't yet tested to see if lower values will work too. (I'll do this and regenerate the patch later.)
Turns out that a value of 64 is the optimum value.
How did you determine it ? :)
Well, I have the actual hardware - at least, one of the chips it supports - which is how I got involved in this bug in the first place. (The Turtle Beach Santa Cruz uses a CS4630.) A value of 32 didn't work when the default period side from ALSA is used; the next highest power of two, 64, does. As all the values I've seen in the kernel for the minimum period size are powers of two, I'm assuming that this is the lowest it can be. (I don't know much about ALSA, bear in mind; this is my first venture into ALSA programming *and* kernel patches.)
I asked it just because your description alone wasn't convincing enough. That is, "it just works good for me" is no good explanation. The test was done on a single machine with a single application. It's possible that it would work on a monster 8GHz machine with another soundcard with a cs46xx chip with another application.
However, as already mentioned, I find changing the value to 64 is somehow rational. But, it's still a question whether this is the only fix...
Well, I find also 64 is a sane value, but it's not always logical. In most cases, 32 is used as the minimum value due to PCI h/w limitation. But 32 is very hard to achieve, so it doesn't matter so much.
Like I said, I have the hardware, and I can tell you that on my hardware, 32 doesn't work properly, while 64 does. :)
This should be the final patch. How should I go about submitting this?
Please give a proper patch summary, too. Also, it'd be more helpful if you give an example what actually your patch fixes (e.g. audacious, etc).
I'm not sure what you mean by a "proper patch summary". Is there anywhere I should read that specifies the format of a proper patch summary?
A patch should have a single line summary to describe what it does. Take a look at $LINUX/Documentation/SubmittingPatches for details.
As for what it fixes, it fixes a problem in the case where neither a period size nor a buffer size is passed to ALSA, instead using the defaults provided. This is the recommended course of action according to a guide to safe ALSA usage by Lennart Poettering - see http://0pointer.de/blog/projects/guide-to-sound-apis.html . Among other things, the guide says that in order for an app to remain safe and playable on all backends that ALSA supports, it should "not touch buffering/period metrics unless you have specific latency needs".
This guide is already being followed by various apps and audio interfaces - Audacious is one of them, and OpenAL (a popular audio interface used by many games including Second Life and Unreal Tournament) is another. As such, there are quite a few examples where the current buggy behaviour can be observed on a card that the cs46xx driver serves. (I don't know of a full list; as I said above, I'm new to ALSA programming.)
Does this help?
Yes, but a bit more concisely if possible, please. The text will be recorded as a GIT changelog forever. This is the best place where people see to track down the changes over tree.
thanks,
Takashi
On 9/8/09, Takashi Iwai tiwai@suse.de wrote:
At Mon, 7 Sep 2009 20:06:37 +0100,
Sophie Hamilton wrote:
On 9/7/09, Takashi Iwai tiwai@suse.de wrote:
At Mon, 7 Sep 2009 18:04:12 +0100, Sophie Hamilton wrote:
Turns out that a value of 64 is the optimum value.
How did you determine it ? :)
Well, I have the actual hardware - at least, one of the chips it supports - which is how I got involved in this bug in the first place. (The Turtle Beach Santa Cruz uses a CS4630.) A value of 32 didn't work when the default period side from ALSA is used; the next highest power of two, 64, does. As all the values I've seen in the kernel for the minimum period size are powers of two, I'm assuming that this is the lowest it can be. (I don't know much about ALSA, bear in mind; this is my first venture into ALSA programming *and* kernel patches.)
I asked it just because your description alone wasn't convincing enough. That is, "it just works good for me" is no good explanation. The test was done on a single machine with a single application. It's possible that it would work on a monster 8GHz machine with another soundcard with a cs46xx chip with another application.
I take your point. However, if this was changed to 32, you'd presumably also need to change the default period/buffer size used by ALSA, as otherwise it would seem to be too low; my system doesn't like it. I'd suggest defaulting to 64, and then if any program has a specific latency need, they can test for underruns with different period sizes and find the best one.
However, as already mentioned, I find changing the value to 64 is somehow rational. But, it's still a question whether this is the only fix...
Sadly, I don't know the answer to this one. But if there's anything I can do to help, let me know.
This should be the final patch. How should I go about submitting this?
Please give a proper patch summary, too. Also, it'd be more helpful if you give an example what actually your patch fixes (e.g. audacious, etc).
I'm not sure what you mean by a "proper patch summary". Is there anywhere I should read that specifies the format of a proper patch summary?
A patch should have a single line summary to describe what it does. Take a look at $LINUX/Documentation/SubmittingPatches for details.
Okay. What I might do, given the instructions in the file, is send another email that conforms to all of the things in that file - subject line, CCs, etc. (for example, it says I should have CCed my patch to linux-kernel@vger.kernel.org too, and Linus ; obviously that'd have been a bad idea with the way my email was formatted now, but would it be a good idea to do those things now?)
As for what it fixes, it fixes a problem in the case where neither a period size nor a buffer size is passed to ALSA, instead using the defaults provided.
[snipped long explanation]
Does this help?
Yes, but a bit more concisely if possible, please. The text will be recorded as a GIT changelog forever. This is the best place where people see to track down the changes over tree.
Gotcha. How about:
"Fix minimum period size for cs46xx cards. This fixes a problem in the case where neither a period size nor a buffer size is passed to ALSA; this is the case in Audacious, OpenAL, and others."
Or is that *too* concise?
Thanks for your comments,
- Sophie,
At Tue, 8 Sep 2009 09:19:33 +0100, Sophie Hamilton wrote:
On 9/8/09, Takashi Iwai tiwai@suse.de wrote:
At Mon, 7 Sep 2009 20:06:37 +0100,
Sophie Hamilton wrote:
On 9/7/09, Takashi Iwai tiwai@suse.de wrote:
At Mon, 7 Sep 2009 18:04:12 +0100, Sophie Hamilton wrote:
Turns out that a value of 64 is the optimum value.
How did you determine it ? :)
Well, I have the actual hardware - at least, one of the chips it supports - which is how I got involved in this bug in the first place. (The Turtle Beach Santa Cruz uses a CS4630.) A value of 32 didn't work when the default period side from ALSA is used; the next highest power of two, 64, does. As all the values I've seen in the kernel for the minimum period size are powers of two, I'm assuming that this is the lowest it can be. (I don't know much about ALSA, bear in mind; this is my first venture into ALSA programming *and* kernel patches.)
I asked it just because your description alone wasn't convincing enough. That is, "it just works good for me" is no good explanation. The test was done on a single machine with a single application. It's possible that it would work on a monster 8GHz machine with another soundcard with a cs46xx chip with another application.
I take your point. However, if this was changed to 32, you'd presumably also need to change the default period/buffer size used by ALSA, as otherwise it would seem to be too low; my system doesn't like it. I'd suggest defaulting to 64, and then if any program has a specific latency need, they can test for underruns with different period sizes and find the best one.
Yeah, I know. I raised it just as a hypothetical issue. As I already wrote, I'd take your fix as is. A missing thing was a proper explanation to convince others ;)
However, as already mentioned, I find changing the value to 64 is somehow rational. But, it's still a question whether this is the only fix...
Sadly, I don't know the answer to this one. But if there's anything I can do to help, let me know.
This should be the final patch. How should I go about submitting this?
Please give a proper patch summary, too. Also, it'd be more helpful if you give an example what actually your patch fixes (e.g. audacious, etc).
I'm not sure what you mean by a "proper patch summary". Is there anywhere I should read that specifies the format of a proper patch summary?
A patch should have a single line summary to describe what it does. Take a look at $LINUX/Documentation/SubmittingPatches for details.
Okay. What I might do, given the instructions in the file, is send another email that conforms to all of the things in that file - subject line, CCs, etc. (for example, it says I should have CCed my patch to linux-kernel@vger.kernel.org too, and Linus ; obviously that'd have been a bad idea with the way my email was formatted now, but would it be a good idea to do those things now?)
Not necessary to send to LKML and Linus in this case. It's a case that can be solved solely in the subsystem tree, so it's enough to send to the alsa-devel ML (and add me to Cc preferably).
As for what it fixes, it fixes a problem in the case where neither a period size nor a buffer size is passed to ALSA, instead using the defaults provided.
[snipped long explanation]
Does this help?
Yes, but a bit more concisely if possible, please. The text will be recorded as a GIT changelog forever. This is the best place where people see to track down the changes over tree.
Gotcha. How about:
"Fix minimum period size for cs46xx cards. This fixes a problem in the case where neither a period size nor a buffer size is passed to ALSA; this is the case in Audacious, OpenAL, and others."
Or is that *too* concise?
That's good enough.
I applied your patch now to sound git tree, so no need to resend. It'll be included in the stable kernel tree later, too, once after it's merged into Linus tree; this might be postponed until 2.6.32 merge window, though.
Thanks,
Takashi
I think use this so call "default value" is not a correct method for media player application. You are actually trying to exploit the limit of hardware ( lowest latency ) It may work on your fast machine, but fail on other user's slow and heavy loaded machine
CS46xx_NEW_DSP support multi channels
At least you should test with surround51 playback and stereo capture concurrently
https://bugtrack.alsa-project.org/alsa-bug/view.php?id=1823
according to the test performed by darkbrain who want to write a hardware accelerated version of openal , the card seem can run only about 10 instances of surround40 concurrently
2009/9/8 Sophie Hamilton sophie-alsa@theblob.org
On 9/8/09, Takashi Iwai tiwai@suse.de wrote:
At Mon, 7 Sep 2009 20:06:37 +0100,
Sophie Hamilton wrote:
On 9/7/09, Takashi Iwai tiwai@suse.de wrote:
At Mon, 7 Sep 2009 18:04:12 +0100, Sophie Hamilton wrote:
Turns out that a value of 64 is the optimum value.
How did you determine it ? :)
Well, I have the actual hardware - at least, one of the chips it supports - which is how I got involved in this bug in the first place. (The Turtle Beach Santa Cruz uses a CS4630.) A value of 32 didn't work when the default period side from ALSA is used; the next highest power of two, 64, does. As all the values I've seen in the kernel for the minimum period size are powers of two, I'm assuming that this is the lowest it can be. (I don't know much about ALSA, bear in mind; this is my first venture into ALSA programming *and* kernel patches.)
I asked it just because your description alone wasn't convincing enough. That is, "it just works good for me" is no good explanation. The test was done on a single machine with a single application. It's possible that it would work on a monster 8GHz machine with another soundcard with a cs46xx chip with another application.
I take your point. However, if this was changed to 32, you'd presumably also need to change the default period/buffer size used by ALSA, as otherwise it would seem to be too low; my system doesn't like it. I'd suggest defaulting to 64, and then if any program has a specific latency need, they can test for underruns with different period sizes and find the best one.
However, as already mentioned, I find changing the value to 64 is somehow rational. But, it's still a question whether this is the only fix...
Sadly, I don't know the answer to this one. But if there's anything I can do to help, let me know.
This should be the final patch. How should I go about submitting
this?
Please give a proper patch summary, too. Also, it'd be more helpful if you give an example what actually your patch fixes (e.g. audacious, etc).
I'm not sure what you mean by a "proper patch summary". Is there anywhere I should read that specifies the format of a proper patch summary?
A patch should have a single line summary to describe what it does. Take a look at $LINUX/Documentation/SubmittingPatches for details.
Okay. What I might do, given the instructions in the file, is send another email that conforms to all of the things in that file - subject line, CCs, etc. (for example, it says I should have CCed my patch to linux-kernel@vger.kernel.org too, and Linus ; obviously that'd have been a bad idea with the way my email was formatted now, but would it be a good idea to do those things now?)
As for what it fixes, it fixes a problem in the case where neither a period size nor a buffer size is passed to ALSA, instead using the defaults provided.
[snipped long explanation]
Does this help?
Yes, but a bit more concisely if possible, please. The text will be recorded as a GIT changelog forever. This is the best place where people see to track down the changes over tree.
Gotcha. How about:
"Fix minimum period size for cs46xx cards. This fixes a problem in the case where neither a period size nor a buffer size is passed to ALSA; this is the case in Audacious, OpenAL, and others."
Or is that *too* concise?
Thanks for your comments,
- Sophie,
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
At Tue, 8 Sep 2009 21:18:13 +0800, Raymond Yau wrote:
I think use this so call "default value" is not a correct method for media player application.
Right, that's my estimation, too. The current "default" parameter choice is to select the smaller period rather than the larger buffer. This should be inverted, but changing such global behavior is dangerous regarding regressions...
Takashi
You are actually trying to exploit the limit of hardware ( lowest latency ) It may work on your fast machine, but fail on other user's slow and heavy loaded machine
CS46xx_NEW_DSP support multi channels
At least you should test with surround51 playback and stereo capture concurrently
https://bugtrack.alsa-project.org/alsa-bug/view.php?id=1823
according to the test performed by darkbrain who want to write a hardware accelerated version of openal , the card seem can run only about 10 instances of surround40 concurrently
2009/9/8 Sophie Hamilton sophie-alsa@theblob.org
On 9/8/09, Takashi Iwai tiwai@suse.de wrote:
At Mon, 7 Sep 2009 20:06:37 +0100,
Sophie Hamilton wrote:
On 9/7/09, Takashi Iwai tiwai@suse.de wrote:
At Mon, 7 Sep 2009 18:04:12 +0100, Sophie Hamilton wrote:
Turns out that a value of 64 is the optimum value.
How did you determine it ? :)
Well, I have the actual hardware - at least, one of the chips it supports - which is how I got involved in this bug in the first place. (The Turtle Beach Santa Cruz uses a CS4630.) A value of 32 didn't work when the default period side from ALSA is used; the next highest power of two, 64, does. As all the values I've seen in the kernel for the minimum period size are powers of two, I'm assuming that this is the lowest it can be. (I don't know much about ALSA, bear in mind; this is my first venture into ALSA programming *and* kernel patches.)
I asked it just because your description alone wasn't convincing enough. That is, "it just works good for me" is no good explanation. The test was done on a single machine with a single application. It's possible that it would work on a monster 8GHz machine with another soundcard with a cs46xx chip with another application.
I take your point. However, if this was changed to 32, you'd presumably also need to change the default period/buffer size used by ALSA, as otherwise it would seem to be too low; my system doesn't like it. I'd suggest defaulting to 64, and then if any program has a specific latency need, they can test for underruns with different period sizes and find the best one.
However, as already mentioned, I find changing the value to 64 is somehow rational. But, it's still a question whether this is the only fix...
Sadly, I don't know the answer to this one. But if there's anything I can do to help, let me know.
This should be the final patch. How should I go about submitting
this?
Please give a proper patch summary, too. Also, it'd be more helpful if you give an example what actually your patch fixes (e.g. audacious, etc).
I'm not sure what you mean by a "proper patch summary". Is there anywhere I should read that specifies the format of a proper patch summary?
A patch should have a single line summary to describe what it does. Take a look at $LINUX/Documentation/SubmittingPatches for details.
Okay. What I might do, given the instructions in the file, is send another email that conforms to all of the things in that file - subject line, CCs, etc. (for example, it says I should have CCed my patch to linux-kernel@vger.kernel.org too, and Linus ; obviously that'd have been a bad idea with the way my email was formatted now, but would it be a good idea to do those things now?)
As for what it fixes, it fixes a problem in the case where neither a period size nor a buffer size is passed to ALSA, instead using the defaults provided.
[snipped long explanation]
Does this help?
Yes, but a bit more concisely if possible, please. The text will be recorded as a GIT changelog forever. This is the best place where people see to track down the changes over tree.
Gotcha. How about:
"Fix minimum period size for cs46xx cards. This fixes a problem in the case where neither a period size nor a buffer size is passed to ALSA; this is the case in Audacious, OpenAL, and others."
Or is that *too* concise?
Thanks for your comments,
- Sophie,
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
participants (6)
-
Lennart Poettering -
Raymond Yau -
Sophie Hamilton -
Stefan Schoenleitner -
Takashi Iwai -
Tony Vroon