[alsa-devel] Problems with safe API and snd-cs46xx
tiwai at suse.de
Tue Sep 8 11:03:23 CEST 2009
At Tue, 8 Sep 2009 09:19:33 +0100,
Sophie Hamilton wrote:
> On 9/8/09, Takashi Iwai <tiwai at suse.de> wrote:
> > 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:
> > > > > 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 at 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.
More information about the Alsa-devel