[alsa-devel] 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 at suse.de> wrote:
> > At Mon, 7 Sep 2009 18:04:12 +0100,
> > Sophie Hamilton wrote:
> >  >
> >  > On 9/7/09, Sophie Hamilton <sophie-alsa at 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