Re: sound/pcm/* bugs (was: Re: page fault panic tracked down (selwakeuppri()) - really sound/pcm/*)

On 17 Jan, Stefan Ehmann wrote:
> On Sat, 2004-01-17 at 14:10, Stefan Ehmann wrote:
>> On Sat, 2004-01-17 at 10:15, Don Lewis wrote:
>> > I think this problem can be caused by a transient malloc() M_NOWAIT
>> > failure.
>> > 
>> > Changes in this version of the patch:
>> > 
>> > 	When increasing blksz in chn_setblocksize(), increase the size
>> > 	of bufsoft before increasing bufhard, and decrease bufsoft after
>> > 	decreasing bufhard in an attempt to avoid the buffer overflow in
>> > 	feed_vchan_s16().
>> > 	
>> > 	Buffers are now allocated using M_WAITOK, requiring locks to
>> > 	be dropped for the malloc() calls.  This required adding a mutex
>> > 	parameter to sndbuf_remalloc().
>> > 
>> > 	Locking order between parent and child channels is changed.  The
>> >         parent is now locked first and then the child.  The list of
>> >         children is protected by the parent's lock.  There are still
>> >         potential race conditions in the vchan creation/destruction code
>> >         because all the locks have to be dropped in order to call
>> >         malloc(), etc.
>> > 
>> > 	Locking cleaned up to eliminate the need for MTX_RECURSE.
>> > 
>> > 	A new mutex allocator for the channel mutexes was added that
>> > 	initializes the mutexes with the MTX_DUPOK flags so that multiple
>> > 	channel mutexes of the same type can be held at once.
>> > 
>> > 	Locking simplification in dsp_ioctl().
>> > 
>> > 	Added KASSERTs in chn_wrfeed().
>> 
>> Some more observations:
>> 
>> - I haven't run the patched kernel long enough to see if it actually
>> prevents panics.
>> 
>> - Sometimes when trying to open dsp it fails at the first attempt, but
>> works if I try again. I had similar experiences when I first used vchans
>> but these problems have been gone for long.
>> 
>> - This also seems to lock up one of the vchans each time this happens (I
>> haven't really tested this yet but I'm pretty sure). For instance, if I
>> try to set the number of vchans to anything lower than 3 right now it
>> fails saying device busy. My guess would be that those vchans were
>> locked but never unlocked for some reason.
>> 
>> - If I use esd as output device (e.g. for ogg123 because it doesn't work
>> properly with the default output) an I stop the player, it takes some
>> seconds before the sound actually stops playing (until the buffer is
>> written to dsp). This doesn't happen with unpatched pcm module.
> 
> Seems like some of my interpretation were wrong.
> 
> The failing at the first try opening /dev/dsp seems to be related to the
> last point. If output is sent to esd by ogg123 (e.g) is terminated but
> sound is played for some more seconds. If the next file is opened the
> device is still busy.

Try changing the two instances of 
	maxblksz = sndbuf_getblksz(b);
in chn_setblocksize() to
	maxblksz = sndbuf_getsize(b) / blkcnt;
and change the condition for executing the sndbuf_remalloc() call from
	if (sndbuf_getblksz(bs) != maxblksz) {
to
	if (sndbuf_getblksz(bs) != maxblksz || sndbuf_getblkcnt(bs) != blkcnt) {	

It seems that the blkcnt for bufsoft never gets set in the non-vchan
case, and gets ignored in the vchan case because the call to
chn_setblocksize() that tries to set it passes 0 for the blksz
parameter.  In either case the code in chn_setblocksize() that
calculates blkcnt and blksz from sndbuf_getbps(bs), sndbuf_getspd(bs)),
chn_targetirqrate, and CHN_2NDBUFMAXSIZE gets invoked, resulting in a
request for a CHN_2NDBUFMAXSIZE-size buffer and wierd values for blkcnt
and blksz.  I typically see blkcnt getting set to 32.  If you combine
this larger than expect blkcnt with the blksz from bufhard, which has a
much smaller blksz, bufsoft is likely to end up with a huge buffer. With
the above change, blkcnt will still be wierd, but its effects will be
papered over and bufsoft's size will get capped at CHN_2NDBUFMAXSIZE
(131072), so there should only be about 1.5 seconds of buffered audio.

> Still have no clue why there are some non-used vchans busy.

I haven't seen that here.


There is also now the issue of merging in phk's recent commit to the
sound code.