Re: [Ipodlinux-devel] PATCH: fix for a rare audio playback bug

[Bernard L. <le...@bo...>]

Thanks for that, I've finally managed to get around to commiting it to
CVS.

Apologies for the delay.

cheers,
bern.

On Sun, 2006-03-26 at 10:35 -0800, Gobber Warts wrote:
> I've found a bug in the circular buffer management code. It is in the
> routine ipodaudio_write in the file
> linux/arch/armnommu/mach-ipod/audio.c. The bug is not in the ipod1-1
> and earlier kernels, but is present in ipod1-2 and all later kernels.
> The change that introduced the bug is CVS revision 1.9 to audio.c from
> Bernard:
> fixed circular buffer bugs to clean up audio output
> 
> http://cvs.sourceforge.net/viewcvs.py/ipodlinux/linux/arch/armnommu/mach-ipod/audio.c?rev=1.9&view=markup
> 
> It happens when a write *exactly* fills the buffer up to the end. This
> could be because there is exactly that amount of data to write, or
> because the read pointer is at the beginning of the audio buffer (ie.
> full). This causes the value *DMA_WRITE_OFF to point to one element
> past the end of the buffer. This is not a problem for the writing
> code, it will just wrap around to the beginning of the buffer when
> there is more data to write or more space to write into. However, the
> reading code looks like this:
> 
>         while ( *r_off != *w_off ) {
>                 if ( (inl(0xc000251c) & 0x7800000) == 0 ) {
>                         outl(inl(0xc000251c)|(1<<9), 0xc000251c);
>                         return;
>                 }
> 
>                 outl(((unsigned)dma_buf[*r_off]) << 16, 0xc0002540);
>                 if ( !stereo ) {
>                         outl(((unsigned)dma_buf[*r_off]) << 16,
> 0xc0002540);
>                 }
> 
>                 *r_off = (*r_off + 1) % BUF_LEN;
>         }
> 
> If the value of *w_off is outside the buffer (eg. one element past its
> end), then there is nothing to stop the read code from looping around
> the same set of samples in the buffer. Depending on the timing between
> the writer and reader, it could loop around one extra time or multiple
> times before a write re-establishes a valid write offset. The overall
> effect is one or more replays of the last ~0.25 seconds worth of audio
> (assuming 46KB buffer and CD format audio). I think that this doesn't
> happpen in practice because the writes are faster than the reads so
> the read pointer rarely catches up with the write pointer. I found an
> obscure way that I could get this to happen all the time giving a 0.25
> second stutter, so I'd appreciate if someone can commit the following
> fix for me.
> 
> Cheers!
> 
> Gobber Warts.
> 
> ===================================================================
> RCS file: RCS/audio.c,v
> retrieving revision 1.1
> diff -c -r1.1 audio.c
> *** audio.c     2006/03/24 08:13:10     1.1
> --- audio.c     2006/03/24 09:26:42
> ***************
> *** 452,458 ****
>                                 rem -= cnt;
>                                 bufsp += cnt;
> 
> !                               write_off_current += cnt;
>                         }
> 
>                         /* room at start of buffer (and more data)? */
> --- 452,458 ----
>                                 rem -= cnt;
>                                 bufsp += cnt;
> 
> !                               write_off_current = (write_off_current
> + cnt) % BUF_LEN;
>                         }
> 
>                         /* room at start of buffer (and more data)? */