[alsa-devel] Problems with safe API and snd-cs46xx

[Sophie Hamilton]

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.

>  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?)

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