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