hw_ptr_interrupt removal broke interrupt pointer updates

hw_ptr_interrupt removal broke interrupt pointer updates

[Clemens Ladisch]

Commit "cleanup & merge hw_ptr update functions" says:
> The main change is hw_ptr_interrupt variable removal to simplify code
> logic. This variable can be computed directly from hw_ptr.

The hw_ptr_interrupt variable was needed to differentiate between the
position at the last normal pointer update and the position of the last
signaled period boundary.

	if (in_interrupt) {
		/* we know that one period was processed */
		/* delta = "expected next hw_ptr" for in_interrupt != 0 */
		delta = old_hw_ptr - (old_hw_ptr % runtime->period_size)
			+ runtime->period_size;
		if (delta > new_hw_ptr) {
			hw_base += runtime->buffer_size;

It is possible for the status/delay ioctls to be called when the sound
card's pointer register alreay shows a position at the beginning of the
new period, but immediately before the interrupt is actually executed.
(This happens regularly on a SMP machine with mplayer.)  When that
happens, the code thinks that the position must be at least one period
ahead of the current position and drops an entire buffer of data.


Best regards,
Clemens

Re: hw_ptr_interrupt removal broke interrupt pointer updates

[Jaroslav Kysela]

On Tue, 26 Jan 2010, Clemens Ladisch wrote:

> Commit "cleanup & merge hw_ptr update functions" says:
>> The main change is hw_ptr_interrupt variable removal to simplify code
>> logic. This variable can be computed directly from hw_ptr.
>
> The hw_ptr_interrupt variable was needed to differentiate between the
> position at the last normal pointer update and the position of the last
> signaled period boundary.
>
> 	if (in_interrupt) {
> 		/* we know that one period was processed */
> 		/* delta = "expected next hw_ptr" for in_interrupt != 0 */
> 		delta = old_hw_ptr - (old_hw_ptr % runtime->period_size)
> 			+ runtime->period_size;
> 		if (delta > new_hw_ptr) {
> 			hw_base += runtime->buffer_size;
>
> It is possible for the status/delay ioctls to be called when the sound
> card's pointer register alreay shows a position at the beginning of the
> new period, but immediately before the interrupt is actually executed.
> (This happens regularly on a SMP machine with mplayer.)  When that
> happens, the code thinks that the position must be at least one period
> ahead of the current position and drops an entire buffer of data.

Clements, thank you for nice explanation how I was wrong. I returned 
hw_ptr_interrupt variable back. I am testing this patch now:

http://git.alsa-project.org/?p=alsa-kernel.git;a=commitdiff;h=04d64a69fcb9fd182d73d6f1a8de55b2f527a1de

A review is always welcome. Thanks.


 						Jaroslav

-----
Jaroslav Kysela <perex@perex.cz>
Linux Kernel Sound Maintainer
ALSA Project, Red Hat, Inc.

Re: hw_ptr_interrupt removal broke interrupt pointer updates

[Raymond Yau]

2010/1/27 Jaroslav Kysela <perex@perex.cz>

> On Tue, 26 Jan 2010, Clemens Ladisch wrote:
>
> > Commit "cleanup & merge hw_ptr update functions" says:
> >> The main change is hw_ptr_interrupt variable removal to simplify code
> >> logic. This variable can be computed directly from hw_ptr.
> >
> > The hw_ptr_interrupt variable was needed to differentiate between the
> > position at the last normal pointer update and the position of the last
> > signaled period boundary.
> >
> >       if (in_interrupt) {
> >               /* we know that one period was processed */
> >               /* delta = "expected next hw_ptr" for in_interrupt != 0 */
> >               delta = old_hw_ptr - (old_hw_ptr % runtime->period_size)
> >                       + runtime->period_size;
> >               if (delta > new_hw_ptr) {
> >                       hw_base += runtime->buffer_size;
> >
> > It is possible for the status/delay ioctls to be called when the sound
> > card's pointer register alreay shows a position at the beginning of the
> > new period, but immediately before the interrupt is actually executed.
> > (This happens regularly on a SMP machine with mplayer.)  When that
> > happens, the code thinks that the position must be at least one period
> > ahead of the current position and drops an entire buffer of data.
>
> Clements, thank you for nice explanation how I was wrong. I returned
> hw_ptr_interrupt variable back. I am testing this patch now:
>
>
> http://git.alsa-project.org/?p=alsa-kernel.git;a=commitdiff;h=04d64a69fcb9fd182d73d6f1a8de55b2f527a1de
>
> A review is always welcome. Thanks.
>
>
>                                                Jaroslav
>
>
do snd_pcm_period_elapsed really handle the case when more than one period
are elasped ?

For au88x0 , each substream have four sets of hardware registers ,  it seem
that the driver can recover lost interrupt with no underrun when using very
small period size


http://www.alsa-project.org/~tiwai/writing-an-alsa-driver/ch05s07.html#pcm-interface-interrupt-handler-boundary

On calling snd_pcm_period_elapsed()

In both cases, even if more than one period are elapsed, you don't have to
call snd_pcm_period_elapsed() many times. Call only once. And the pcm layer
will check the current hardware pointer and update to the latest status.

Re: hw_ptr_interrupt removal broke interrupt pointer updates

[Jaroslav Kysela]

On Wed, 27 Jan 2010, Raymond Yau wrote:

> 2010/1/27 Jaroslav Kysela <perex@perex.cz>
>
>> On Tue, 26 Jan 2010, Clemens Ladisch wrote:
>>
>>> Commit "cleanup & merge hw_ptr update functions" says:
>>>> The main change is hw_ptr_interrupt variable removal to simplify code
>>>> logic. This variable can be computed directly from hw_ptr.
>>>
>>> The hw_ptr_interrupt variable was needed to differentiate between the
>>> position at the last normal pointer update and the position of the last
>>> signaled period boundary.
>>>
>>>       if (in_interrupt) {
>>>               /* we know that one period was processed */
>>>               /* delta = "expected next hw_ptr" for in_interrupt != 0 */
>>>               delta = old_hw_ptr - (old_hw_ptr % runtime->period_size)
>>>                       + runtime->period_size;
>>>               if (delta > new_hw_ptr) {
>>>                       hw_base += runtime->buffer_size;
>>>
>>> It is possible for the status/delay ioctls to be called when the sound
>>> card's pointer register alreay shows a position at the beginning of the
>>> new period, but immediately before the interrupt is actually executed.
>>> (This happens regularly on a SMP machine with mplayer.)  When that
>>> happens, the code thinks that the position must be at least one period
>>> ahead of the current position and drops an entire buffer of data.
>>
>> Clements, thank you for nice explanation how I was wrong. I returned
>> hw_ptr_interrupt variable back. I am testing this patch now:
>>
>>
>> http://git.alsa-project.org/?p=alsa-kernel.git;a=commitdiff;h=04d64a69fcb9fd182d73d6f1a8de55b2f527a1de
>>
>> A review is always welcome. Thanks.
>>
>>
>>                                                Jaroslav
>>
>>
> do snd_pcm_period_elapsed really handle the case when more than one period
> are elasped ?
>
> For au88x0 , each substream have four sets of hardware registers ,  it seem
> that the driver can recover lost interrupt with no underrun when using very
> small period size
>
>
> http://www.alsa-project.org/~tiwai/writing-an-alsa-driver/ch05s07.html#pcm-interface-interrupt-handler-boundary
>
> On calling snd_pcm_period_elapsed()
>
> In both cases, even if more than one period are elapsed, you don't have to
> call snd_pcm_period_elapsed() many times. Call only once. And the pcm layer
> will check the current hardware pointer and update to the latest status.

Yes, the new code handles more alapsed periods, too. The in_interrupt 
check is mainly for situations when period count == 1 and it compares 
expected next hw_ptr for interrupt with hw_ptr computed from the hw_base 
and actual position in the ring buffer.

 						Jaroslav

-----
Jaroslav Kysela <perex@perex.cz>
Linux Kernel Sound Maintainer
ALSA Project, Red Hat, Inc.

Re: hw_ptr_interrupt removal broke interrupt pointer updates

[Clemens Ladisch]

Jaroslav Kysela wrote:
> I returned hw_ptr_interrupt variable back. I am testing this patch now:
> 
> http://git.alsa-project.org/?p=alsa-kernel.git;a=commitdiff;h=04d64a69fcb9fd182d73d6f1a8de55b2f527a1de
> 
> A review is always welcome. Thanks.

The patch looks fine, and it works again now.


A somewhat unrelated issue:  Both old and new code assume that
hw_ptr==0 is a period boundary, but that is not true if the boundary
is not an integer multiple of the period size, and the pointer wraps.
I'm not sure what happens then.


Best regards,
Clemens

Re: hw_ptr_interrupt removal broke interrupt pointer updates

[Jaroslav Kysela]

On Wed, 27 Jan 2010, Clemens Ladisch wrote:

> Jaroslav Kysela wrote:
>> I returned hw_ptr_interrupt variable back. I am testing this patch now:
>>
>> http://git.alsa-project.org/?p=alsa-kernel.git;a=commitdiff;h=04d64a69fcb9fd182d73d6f1a8de55b2f527a1de
>>
>> A review is always welcome. Thanks.
>
> The patch looks fine, and it works again now.
>
>
> A somewhat unrelated issue:  Both old and new code assume that
> hw_ptr==0 is a period boundary, but that is not true if the boundary
> is not an integer multiple of the period size, and the pointer wraps.
> I'm not sure what happens then.

I'm not exactly sure what you're talking about. Where is the 
hw_ptr==0 assumption? I see wrapping only for hw_base which is good, 
because this value is based on buffer_size.

 				Thanks,
 					Jaroslav

-----
Jaroslav Kysela <perex@perex.cz>
Linux Kernel Sound Maintainer
ALSA Project, Red Hat, Inc.

Re: hw_ptr_interrupt removal broke interrupt pointer updates

[Clemens Ladisch]

Jaroslav Kysela wrote:
> On Wed, 27 Jan 2010, Clemens Ladisch wrote:
> > A somewhat unrelated issue:  Both old and new code assume that
> > hw_ptr==0 is a period boundary, but that is not true if the boundary
> > is not an integer multiple of the period size, and the pointer wraps.
> > I'm not sure what happens then.
> 
> I'm not exactly sure what you're talking about. Where is the
> hw_ptr==0 assumption?

This code, which tries to align hw_ptr_interrupt to a period boundary:

	runtime->hw_ptr_interrupt = new_hw_ptr -
			(new_hw_ptr % runtime->period_size);


Best regards,
Clemens

Re: hw_ptr_interrupt removal broke interrupt pointer updates

[Jaroslav Kysela]

On Wed, 27 Jan 2010, Clemens Ladisch wrote:

> Jaroslav Kysela wrote:
>> On Wed, 27 Jan 2010, Clemens Ladisch wrote:
>>> A somewhat unrelated issue:  Both old and new code assume that
>>> hw_ptr==0 is a period boundary, but that is not true if the boundary
>>> is not an integer multiple of the period size, and the pointer wraps.
>>> I'm not sure what happens then.
>>
>> I'm not exactly sure what you're talking about. Where is the
>> hw_ptr==0 assumption?
>
> This code, which tries to align hw_ptr_interrupt to a period boundary:
>
> 	runtime->hw_ptr_interrupt = new_hw_ptr -
> 			(new_hw_ptr % runtime->period_size);

I see. It is really problem, because if hw_ptr_interrupt shifts, then the 
condition
 		delta = runtime->hw_ptr_interrupt + runtime->period_size;
                 if (delta > new_hw_ptr) {

is not accurate and might cause unwanted issues.

The simple fix for 64-bit archs is to use "boundary = buffer_size * 
period_size" expression to setup the boundary variable properly.

I added code to find the lowest common multiple for 32-bit archs.

The patch is:

http://git.alsa-project.org/?p=alsa-kernel.git;a=commitdiff;h=7910b4a1db63fefc3d291853d33c34c5b6352e8e

 					Thanks,
 						Jaroslav

-----
Jaroslav Kysela <perex@perex.cz>
Linux Kernel Sound Maintainer
ALSA Project, Red Hat, Inc.

Re: hw_ptr_interrupt removal broke interrupt pointer updates

[Colin Guthrie]

'Twas brillig, and Jaroslav Kysela at 27/01/10 17:21 did gyre and gimble:
> On Wed, 27 Jan 2010, Clemens Ladisch wrote:
> 
>> Jaroslav Kysela wrote:
>>> On Wed, 27 Jan 2010, Clemens Ladisch wrote:
>>>> A somewhat unrelated issue:  Both old and new code assume that
>>>> hw_ptr==0 is a period boundary, but that is not true if the boundary
>>>> is not an integer multiple of the period size, and the pointer wraps.
>>>> I'm not sure what happens then.
>>>
>>> I'm not exactly sure what you're talking about. Where is the
>>> hw_ptr==0 assumption?
>>
>> This code, which tries to align hw_ptr_interrupt to a period boundary:
>>
>> 	runtime->hw_ptr_interrupt = new_hw_ptr -
>> 			(new_hw_ptr % runtime->period_size);
> 
> I see. It is really problem, because if hw_ptr_interrupt shifts, then the 
> condition
>  		delta = runtime->hw_ptr_interrupt + runtime->period_size;
>                  if (delta > new_hw_ptr) {
> 
> is not accurate and might cause unwanted issues.
> 
> The simple fix for 64-bit archs is to use "boundary = buffer_size * 
> period_size" expression to setup the boundary variable properly.
> 
> I added code to find the lowest common multiple for 32-bit archs.
> 
> The patch is:
> 
> http://git.alsa-project.org/?p=alsa-kernel.git;a=commitdiff;h=7910b4a1db63fefc3d291853d33c34c5b6352e8e

FWIW, this patch seems to have finally solved:
https://qa.mandriva.com/show_bug.cgi?id=57010

which we used to track this issue.

Had two users confirm it as fixed which is good enough for me :)

Take care and thanks.

Col


-- 

Colin Guthrie
gmane(at)colin.guthr.ie
http://colin.guthr.ie/

Day Job:
  Tribalogic Limited [http://www.tribalogic.net/]
Open Source:
  Mandriva Linux Contributor [http://www.mandriva.com/]
  PulseAudio Hacker [http://www.pulseaudio.org/]
  Trac Hacker [http://trac.edgewall.org/]