Problems with safe API and snd-cs46xx

Problems with safe API and snd-cs46xx

[Sophie Hamilton]

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.

Re: Problems with safe API and snd-cs46xx

[Takashi Iwai]

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

Re: Problems with safe API and snd-cs46xx

[Tony Vroon]

[-- Attachment #1.1: Type: text/plain, Size: 941 bytes --]

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.

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: Problems with safe API and snd-cs46xx

[Raymond Yau]

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#l159>SND_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#g98ba19d2800b7d601277fd8c068505da

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#g692ad9e5902d0623b56a0decee0fa686>is
called automatically and the stream is brought to
SND_PCM_STATE_PREPARED<http://www.alsa-project.org/alsa-doc/alsa-lib/group___p_c_m.html#ggfff41b69676ea4a7efa64dd5642505ddfa179425d00d012af07ef15e1c066ea6>state.
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
>
>

Error with custom driver: playback drain error (DMA or IRQ trouble?)

[Stefan Schoenleitner]

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

Re: Problems with safe API and snd-cs46xx

[Takashi Iwai]

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

Re: Problems with safe API and snd-cs46xx

[Sophie Hamilton]

[-- Attachment #1: Type: text/plain, Size: 974 bytes --]

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.

[-- Attachment #2: cs46xx-fix.patch --]
[-- Type: application/octet-stream, Size: 503 bytes --]

This patch will fix the cs46xx minimum period size, allowing playback
on cs46xx-based cards.

Signed-off-by: Sophie Hamilton <kernel@theblob.org>

--- a/sound/pci/cs46xx/cs46xx_lib.h	2009-09-07 15:24:26.000000000 +0100
+++ b/sound/pci/cs46xx/cs46xx_lib.h	2009-09-07 15:41:31.000000000 +0100
@@ -35,7 +35,7 @@
 
 
 #ifdef CONFIG_SND_CS46XX_NEW_DSP
-#define CS46XX_MIN_PERIOD_SIZE 1
+#define CS46XX_MIN_PERIOD_SIZE 256
 #define CS46XX_MAX_PERIOD_SIZE 1024*1024
 #else
 #define CS46XX_MIN_PERIOD_SIZE 2048

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: Problems with safe API and snd-cs46xx

[Raymond Yau]

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
>

Re: Problems with safe API and snd-cs46xx

[Takashi Iwai]

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
> 

Re: Problems with safe API and snd-cs46xx

[Sophie Hamilton]

[-- Attachment #1: Type: text/plain, Size: 630 bytes --]

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.

[-- Attachment #2: cs46xx-fix.patch --]
[-- Type: application/octet-stream, Size: 502 bytes --]

This patch will fix the cs46xx minimum period size, allowing playback
on cs46xx-based cards.

Signed-off-by: Sophie Hamilton <kernel@theblob.org>

--- a/sound/pci/cs46xx/cs46xx_lib.h	2009-09-07 15:24:26.000000000 +0100
+++ b/sound/pci/cs46xx/cs46xx_lib.h	2009-09-07 15:41:31.000000000 +0100
@@ -35,7 +35,7 @@
 
 
 #ifdef CONFIG_SND_CS46XX_NEW_DSP
-#define CS46XX_MIN_PERIOD_SIZE 1
+#define CS46XX_MIN_PERIOD_SIZE 64
 #define CS46XX_MAX_PERIOD_SIZE 1024*1024
 #else
 #define CS46XX_MIN_PERIOD_SIZE 2048

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: Problems with safe API and snd-cs46xx

[Takashi Iwai]

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

Re: Problems with safe API and snd-cs46xx

[Sophie Hamilton]

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.

Re: Problems with safe API and snd-cs46xx

[Lennart Poettering]

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

-- 
Lennart Poettering                        Red Hat, Inc.
lennart [at] poettering [dot] net
http://0pointer.net/lennart/           GnuPG 0x1A015CC4

Re: Problems with safe API and snd-cs46xx

[Sophie Hamilton]

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.

Re: 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@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
 	};

Re: Problems with safe API and snd-cs46xx

[Takashi Iwai]

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

Re: Problems with safe API and snd-cs46xx

[Sophie Hamilton]

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.

Re: Problems with safe API and snd-cs46xx

[Sophie Hamilton]

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,

Re: Problems with safe API and snd-cs46xx

[Takashi Iwai]

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

Re: Problems with safe API and snd-cs46xx

[Takashi Iwai]

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

Re: Problems with safe API and snd-cs46xx

[Raymond Yau]

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
>

Re: Problems with safe API and snd-cs46xx

[Takashi Iwai]

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
> 

Re: Problems with safe API and snd-cs46xx

[Lennart Poettering]

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:
> 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

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

-- 
Lennart Poettering                        Red Hat, Inc.
lennart [at] poettering [dot] net
http://0pointer.net/lennart/           GnuPG 0x1A015CC4

Re: Problems with safe API and snd-cs46xx

[Takashi Iwai]

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:
> > 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
> 
> 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

Re: Problems with safe API and snd-cs46xx

[Raymond Yau]

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:
> > > 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
> >
> > 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
>

Re: Problems with safe API and snd-cs46xx

[Takashi Iwai]

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;

Re: Problems with safe API and snd-cs46xx

[Sophie Hamilton]

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.

Re: Problems with safe API and snd-cs46xx

[Takashi Iwai]

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

Re: Problems with safe API and snd-cs46xx

[Lennart Poettering]

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

-- 
Lennart Poettering                        Red Hat, Inc.
lennart [at] poettering [dot] net
http://0pointer.net/lennart/           GnuPG 0x1A015CC4

Re: Problems with safe API and snd-cs46xx

[Takashi Iwai]

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

Re: Problems with safe API and snd-cs46xx

[Lennart Poettering]

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

-- 
Lennart Poettering                        Red Hat, Inc.
lennart [at] poettering [dot] net
http://0pointer.net/lennart/           GnuPG 0x1A015CC4

Re: Problems with safe API and snd-cs46xx

[Takashi Iwai]

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

Re: Problems with safe API and snd-cs46xx

[Raymond Yau]

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
>