emu10k1 bug and patch (0.9.0rc7)

emu10k1 bug and patch (0.9.0rc7)

[Arnaud de Bossoreille de Ribou]

[-- Attachment #1: Type: text/plain, Size: 1299 bytes --]

Hi, I discovered a bug in the emu10k1 driver which I'll explain here:

I was developing an application which uses the timestamps given in the
status of the device to send S/PDIF data to it. This app worked pretty
well except that sometimes I heard sound discontinuities and then a
constant time delay between the sound and the video.

I finally found where was the problem, my results is based on the
emu10k1-debug.patch file attached. The "frame" argument is equal to 0
when the app gets the status of the device. With this patch applied I
saw some output on the console exactly at the same time the bug occured.
Adding a "else" after the "if" to prevent sw_ready from being updated
fixed the problem and the output looked like

----------------
plop 0 -1536 A B
plop 0 1536 B A
----------------

where B == A - 1536 (1536 is the period_size). These two lines were
repeated a few times during playback.

So the bug looks like a signedness problem since sw_ready is unsigned
and there is a while(sw_ready > 0), which explain the constant delay,
next in the "snd_emu10k1_fx8010_playback_transfer" function.

So the emu10k1.patch file attached fixes the problem and seems not to
introduce new ones.

Note: patches were made with the 0.9.0rc7 version of the alsa-driver
package.

Regards,

-- 
Arnaud.

[-- Attachment #2: emu10k1-debug.patch --]
[-- Type: text/plain, Size: 450 bytes --]

--- alsa-kernel/pci/emu10k1/emufx.c.orig	2003-02-08 23:02:50.000000000 +0100
+++ alsa-kernel/pci/emu10k1/emufx.c	2003-02-08 23:17:09.000000000 +0100
@@ -531,6 +531,11 @@
 	if (diff) {
 		if (diff < -(snd_pcm_sframes_t) (runtime->boundary / 2))
 			diff += runtime->boundary;
+		if(frames == 0)
+		{
+			printk("plop %d %ld (%lu %u)\n",
+				pcm->sw_ready, diff, appl_ptr, pcm->appl_ptr);
+		}
 		pcm->sw_ready += diff;
 	}
 	pcm->sw_ready += frames;

[-- Attachment #3: emu10k1.patch --]
[-- Type: text/plain, Size: 442 bytes --]

--- alsa-kernel/include/emu10k1.h.orig	2003-02-08 23:00:43.000000000 +0100
+++ alsa-kernel/include/emu10k1.h	2003-02-08 23:02:02.000000000 +0100
@@ -879,7 +879,8 @@
 	unsigned char etram[32];	/* external TRAM address & data */
 	unsigned int sw_data, hw_data;
 	unsigned int sw_io, hw_io;
-	unsigned int sw_ready, hw_ready;
+	int sw_ready;
+	unsigned int hw_ready;
 	unsigned int appl_ptr;
 	unsigned int tram_pos;
 	unsigned int tram_shift;

Re: emu10k1 bug and patch (0.9.0rc7)

[Takashi Iwai]

At Mon, 10 Feb 2003 18:21:12 +0100,
Arnaud de Bossoreille de Ribou wrote:
> 
> Hi, I discovered a bug in the emu10k1 driver which I'll explain here:
> 
> I was developing an application which uses the timestamps given in the
> status of the device to send S/PDIF data to it. This app worked pretty
> well except that sometimes I heard sound discontinuities and then a
> constant time delay between the sound and the video.
> 
> I finally found where was the problem, my results is based on the
> emu10k1-debug.patch file attached. The "frame" argument is equal to 0
> when the app gets the status of the device. With this patch applied I
> saw some output on the console exactly at the same time the bug occured.
> Adding a "else" after the "if" to prevent sw_ready from being updated
> fixed the problem and the output looked like
> 
> ----------------
> plop 0 -1536 A B
> plop 0 1536 B A
> ----------------
> 
> where B == A - 1536 (1536 is the period_size). These two lines were
> repeated a few times during playback.
> 
> So the bug looks like a signedness problem since sw_ready is unsigned
> and there is a while(sw_ready > 0), which explain the constant delay,
> next in the "snd_emu10k1_fx8010_playback_transfer" function.

this is because of the incorrect check of boundary-wrap.
the comparison below must be <= instead of <.
(or, it can be simply "diff < 0".)
if there only two periods, the original code cannot detect the
boundary-wrap.

	if (diff) {
==>		if (diff < -(snd_pcm_sframes_t) (runtime->boundary / 2))
			diff += runtime->boundary;
		pcm->sw_ready += diff;
	}

sw_ready should be unsigned safely.
please try the change above with the unsigned sw_ready.


anyway, thanks for your bug report!


ciao,

Takashi


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Re: emu10k1 bug and patch (0.9.0rc7)

[Jaroslav Kysela]

On Thu, 13 Feb 2003, Takashi Iwai wrote:

> At Mon, 10 Feb 2003 18:21:12 +0100,
> Arnaud de Bossoreille de Ribou wrote:
> > 
> > Hi, I discovered a bug in the emu10k1 driver which I'll explain here:
> > 
> > I was developing an application which uses the timestamps given in the
> > status of the device to send S/PDIF data to it. This app worked pretty
> > well except that sometimes I heard sound discontinuities and then a
> > constant time delay between the sound and the video.
> > 
> > I finally found where was the problem, my results is based on the
> > emu10k1-debug.patch file attached. The "frame" argument is equal to 0
> > when the app gets the status of the device. With this patch applied I
> > saw some output on the console exactly at the same time the bug occured.
> > Adding a "else" after the "if" to prevent sw_ready from being updated
> > fixed the problem and the output looked like
> > 
> > ----------------
> > plop 0 -1536 A B
> > plop 0 1536 B A
> > ----------------
> > 
> > where B == A - 1536 (1536 is the period_size). These two lines were
> > repeated a few times during playback.
> > 
> > So the bug looks like a signedness problem since sw_ready is unsigned
> > and there is a while(sw_ready > 0), which explain the constant delay,
> > next in the "snd_emu10k1_fx8010_playback_transfer" function.
> 
> this is because of the incorrect check of boundary-wrap.
> the comparison below must be <= instead of <.
> (or, it can be simply "diff < 0".)
> if there only two periods, the original code cannot detect the
> boundary-wrap.
> 
> 	if (diff) {
> ==>		if (diff < -(snd_pcm_sframes_t) (runtime->boundary / 2))
> 			diff += runtime->boundary;
> 		pcm->sw_ready += diff;
> 	}
> 
> sw_ready should be unsigned safely.
> please try the change above with the unsigned sw_ready.

Not really. Note that the application can move the appl_ptr backward 
(using snd_pcm_rewind()). The problem is that pcm->appl_ptr is updated
wrongly, thus calling function with frames == 0 twice or more causes 
different results. I'm working on a proper fix.

						Jaroslav

-----
Jaroslav Kysela <perex@suse.cz>
Linux Kernel Sound Maintainer
ALSA Project, SuSE Labs



-------------------------------------------------------
This SF.NET email is sponsored by: FREE  SSL Guide from Thawte
are you planning your Web Server Security? Click here to get a FREE
Thawte SSL guide and find the answers to all your  SSL security issues.
http://ads.sourceforge.net/cgi-bin/redirect.pl?thaw0026en

Re: emu10k1 bug and patch (0.9.0rc7)

[Takashi Iwai]

At Thu, 13 Feb 2003 20:57:40 +0100 (CET),
Jaroslav wrote:
> 
> On Thu, 13 Feb 2003, Takashi Iwai wrote:
> 
> > At Mon, 10 Feb 2003 18:21:12 +0100,
> > Arnaud de Bossoreille de Ribou wrote:
> > > 
> > > Hi, I discovered a bug in the emu10k1 driver which I'll explain here:
> > > 
> > > I was developing an application which uses the timestamps given in the
> > > status of the device to send S/PDIF data to it. This app worked pretty
> > > well except that sometimes I heard sound discontinuities and then a
> > > constant time delay between the sound and the video.
> > > 
> > > I finally found where was the problem, my results is based on the
> > > emu10k1-debug.patch file attached. The "frame" argument is equal to 0
> > > when the app gets the status of the device. With this patch applied I
> > > saw some output on the console exactly at the same time the bug occured.
> > > Adding a "else" after the "if" to prevent sw_ready from being updated
> > > fixed the problem and the output looked like
> > > 
> > > ----------------
> > > plop 0 -1536 A B
> > > plop 0 1536 B A
> > > ----------------
> > > 
> > > where B == A - 1536 (1536 is the period_size). These two lines were
> > > repeated a few times during playback.
> > > 
> > > So the bug looks like a signedness problem since sw_ready is unsigned
> > > and there is a while(sw_ready > 0), which explain the constant delay,
> > > next in the "snd_emu10k1_fx8010_playback_transfer" function.
> > 
> > this is because of the incorrect check of boundary-wrap.
> > the comparison below must be <= instead of <.
> > (or, it can be simply "diff < 0".)
> > if there only two periods, the original code cannot detect the
> > boundary-wrap.
> > 
> > 	if (diff) {
> > ==>		if (diff < -(snd_pcm_sframes_t) (runtime->boundary / 2))
> > 			diff += runtime->boundary;
> > 		pcm->sw_ready += diff;
> > 	}
> > 
> > sw_ready should be unsigned safely.
> > please try the change above with the unsigned sw_ready.
> 
> Not really. Note that the application can move the appl_ptr backward 
> (using snd_pcm_rewind()). The problem is that pcm->appl_ptr is updated
> wrongly, thus calling function with frames == 0 twice or more causes 
> different results. I'm working on a proper fix.

ah, rewind take the appl_ptr back...


regarding to another appl_ptr update:

in pcm_lib.c snd_pcm_lib_read1/write1(), appl_ptr is set to 0 if it
comes over boundary. 

		appl_ptr += frames;
		if (appl_ptr >= runtime->boundary) {
			runtime->control->appl_ptr = 0;
		} else {
			runtime->control->appl_ptr = appl_ptr;
		}

i'm not sure it's always safe (whether frame increment is aligned to
the boundary size).  is there problem to do like below?

		if (appl_ptr >= runtime->boundary) {
			runtime->control->appl_ptr = appl_ptr - runtime->boundary;
		} else {
			...


Takashi


-------------------------------------------------------
This SF.NET email is sponsored by: FREE  SSL Guide from Thawte
are you planning your Web Server Security? Click here to get a FREE
Thawte SSL guide and find the answers to all your  SSL security issues.
http://ads.sourceforge.net/cgi-bin/redirect.pl?thaw0026en

Re: emu10k1 bug and patch (0.9.0rc7)

[Abramo Bagnara]

Takashi Iwai wrote:
> 
> At Thu, 13 Feb 2003 20:57:40 +0100 (CET),
> Jaroslav wrote:
> >
> > On Thu, 13 Feb 2003, Takashi Iwai wrote:
> >
> > > At Mon, 10 Feb 2003 18:21:12 +0100,
> > > Arnaud de Bossoreille de Ribou wrote:
> > > >
> > > > Hi, I discovered a bug in the emu10k1 driver which I'll explain here:
> > > >
> > > > I was developing an application which uses the timestamps given in the
> > > > status of the device to send S/PDIF data to it. This app worked pretty
> > > > well except that sometimes I heard sound discontinuities and then a
> > > > constant time delay between the sound and the video.
> > > >
> > > > I finally found where was the problem, my results is based on the
> > > > emu10k1-debug.patch file attached. The "frame" argument is equal to 0
> > > > when the app gets the status of the device. With this patch applied I
> > > > saw some output on the console exactly at the same time the bug occured.
> > > > Adding a "else" after the "if" to prevent sw_ready from being updated
> > > > fixed the problem and the output looked like
> > > >
> > > > ----------------
> > > > plop 0 -1536 A B
> > > > plop 0 1536 B A
> > > > ----------------
> > > >
> > > > where B == A - 1536 (1536 is the period_size). These two lines were
> > > > repeated a few times during playback.
> > > >
> > > > So the bug looks like a signedness problem since sw_ready is unsigned
> > > > and there is a while(sw_ready > 0), which explain the constant delay,
> > > > next in the "snd_emu10k1_fx8010_playback_transfer" function.
> > >
> > > this is because of the incorrect check of boundary-wrap.
> > > the comparison below must be <= instead of <.
> > > (or, it can be simply "diff < 0".)
> > > if there only two periods, the original code cannot detect the
> > > boundary-wrap.
> > >
> > >     if (diff) {
> > > ==>         if (diff < -(snd_pcm_sframes_t) (runtime->boundary / 2))
> > >                     diff += runtime->boundary;
> > >             pcm->sw_ready += diff;
> > >     }
> > >
> > > sw_ready should be unsigned safely.
> > > please try the change above with the unsigned sw_ready.
> >
> > Not really. Note that the application can move the appl_ptr backward
> > (using snd_pcm_rewind()). The problem is that pcm->appl_ptr is updated
> > wrongly, thus calling function with frames == 0 twice or more causes
> > different results. I'm working on a proper fix.
> 
> ah, rewind take the appl_ptr back...
> 
> regarding to another appl_ptr update:
> 
> in pcm_lib.c snd_pcm_lib_read1/write1(), appl_ptr is set to 0 if it
> comes over boundary.
> 
>                 appl_ptr += frames;
>                 if (appl_ptr >= runtime->boundary) {
>                         runtime->control->appl_ptr = 0;
>                 } else {
>                         runtime->control->appl_ptr = appl_ptr;
>                 }
> 
> i'm not sure it's always safe (whether frame increment is aligned to
> the boundary size).  is there problem to do like below?
> 
>                 if (appl_ptr >= runtime->boundary) {
>                         runtime->control->appl_ptr = appl_ptr - runtime->boundary;
>                 } else {
>                         ...

It's definitely harmless, but in this way you'd hide elsewhere placed
bugs.

-- 
Abramo Bagnara                       mailto:abramo.bagnara@libero.it

Opera Unica                          Phone: +39.546.656023
Via Emilia Interna, 140
48014 Castel Bolognese (RA) - Italy


-------------------------------------------------------
This SF.NET email is sponsored by: FREE  SSL Guide from Thawte
are you planning your Web Server Security? Click here to get a FREE
Thawte SSL guide and find the answers to all your  SSL security issues.
http://ads.sourceforge.net/cgi-bin/redirect.pl?thaw0026en

Re: emu10k1 bug and patch (0.9.0rc7)

[Jaroslav Kysela]

On Fri, 14 Feb 2003, Takashi Iwai wrote:

> At Thu, 13 Feb 2003 20:57:40 +0100 (CET),
> Jaroslav wrote:
> > 
> > On Thu, 13 Feb 2003, Takashi Iwai wrote:
> > 
> > > At Mon, 10 Feb 2003 18:21:12 +0100,
> > > Arnaud de Bossoreille de Ribou wrote:
> > > > 
> > > > Hi, I discovered a bug in the emu10k1 driver which I'll explain here:
> > > > 
> > > > I was developing an application which uses the timestamps given in the
> > > > status of the device to send S/PDIF data to it. This app worked pretty
> > > > well except that sometimes I heard sound discontinuities and then a
> > > > constant time delay between the sound and the video.
> > > > 
> > > > I finally found where was the problem, my results is based on the
> > > > emu10k1-debug.patch file attached. The "frame" argument is equal to 0
> > > > when the app gets the status of the device. With this patch applied I
> > > > saw some output on the console exactly at the same time the bug occured.
> > > > Adding a "else" after the "if" to prevent sw_ready from being updated
> > > > fixed the problem and the output looked like
> > > > 
> > > > ----------------
> > > > plop 0 -1536 A B
> > > > plop 0 1536 B A
> > > > ----------------
> > > > 
> > > > where B == A - 1536 (1536 is the period_size). These two lines were
> > > > repeated a few times during playback.
> > > > 
> > > > So the bug looks like a signedness problem since sw_ready is unsigned
> > > > and there is a while(sw_ready > 0), which explain the constant delay,
> > > > next in the "snd_emu10k1_fx8010_playback_transfer" function.
> > > 
> > > this is because of the incorrect check of boundary-wrap.
> > > the comparison below must be <= instead of <.
> > > (or, it can be simply "diff < 0".)
> > > if there only two periods, the original code cannot detect the
> > > boundary-wrap.
> > > 
> > > 	if (diff) {
> > > ==>		if (diff < -(snd_pcm_sframes_t) (runtime->boundary / 2))
> > > 			diff += runtime->boundary;
> > > 		pcm->sw_ready += diff;
> > > 	}
> > > 
> > > sw_ready should be unsigned safely.
> > > please try the change above with the unsigned sw_ready.
> > 
> > Not really. Note that the application can move the appl_ptr backward 
> > (using snd_pcm_rewind()). The problem is that pcm->appl_ptr is updated
> > wrongly, thus calling function with frames == 0 twice or more causes 
> > different results. I'm working on a proper fix.
> 
> ah, rewind take the appl_ptr back...
> 
> 
> regarding to another appl_ptr update:
> 
> in pcm_lib.c snd_pcm_lib_read1/write1(), appl_ptr is set to 0 if it
> comes over boundary. 
> 
> 		appl_ptr += frames;
> 		if (appl_ptr >= runtime->boundary) {
> 			runtime->control->appl_ptr = 0;
> 		} else {
> 			runtime->control->appl_ptr = appl_ptr;
> 		}
> 
> i'm not sure it's always safe (whether frame increment is aligned to
> the boundary size).  is there problem to do like below?
> 
> 		if (appl_ptr >= runtime->boundary) {
> 			runtime->control->appl_ptr = appl_ptr - runtime->boundary;
> 		} else {
> 			...

Note that boundary = N * buffer_size (it's close to LONG_MAX value).
Also, read/write code does only the contiguous transfers (they don't 
crosses the buffer_size). So it's perfectly safe to set the appl_ptr to 
zero when it is over boundary.

						Jaroslav

-----
Jaroslav Kysela <perex@suse.cz>
Linux Kernel Sound Maintainer
ALSA Project, SuSE Labs



-------------------------------------------------------
This SF.NET email is sponsored by: FREE  SSL Guide from Thawte
are you planning your Web Server Security? Click here to get a FREE
Thawte SSL guide and find the answers to all your  SSL security issues.
http://ads.sourceforge.net/cgi-bin/redirect.pl?thaw0026en

Re: emu10k1 bug and patch (0.9.0rc7)

[Jaroslav Kysela]

On Mon, 10 Feb 2003, Arnaud de Bossoreille de Ribou wrote:

> So the bug looks like a signedness problem since sw_ready is unsigned
> and there is a while(sw_ready > 0), which explain the constant delay,
> next in the "snd_emu10k1_fx8010_playback_transfer" function.
> 
> So the emu10k1.patch file attached fixes the problem and seems not to
> introduce new ones.

Please, could you try this patch, if it also fixes your problem? Thanks.


Index: emufx.c
===================================================================
RCS file: /cvsroot/alsa/alsa-kernel/pci/emu10k1/emufx.c,v
retrieving revision 1.26
diff -u -r1.26 emufx.c
--- emufx.c	31 Jan 2003 15:21:03 -0000	1.26
+++ emufx.c	13 Feb 2003 20:29:55 -0000
@@ -532,7 +532,7 @@
 	if (diff) {
 		if (diff < -(snd_pcm_sframes_t) (runtime->boundary / 2))
 			diff += runtime->boundary;
-		pcm->sw_ready += diff;
+		frames += diff;
 	}
 	pcm->sw_ready += frames;
 	pcm->appl_ptr = appl_ptr + frames;

						Jaroslav

-----
Jaroslav Kysela <perex@suse.cz>
Linux Kernel Sound Maintainer
ALSA Project, SuSE Labs



-------------------------------------------------------
This SF.NET email is sponsored by: FREE  SSL Guide from Thawte
are you planning your Web Server Security? Click here to get a FREE
Thawte SSL guide and find the answers to all your  SSL security issues.
http://ads.sourceforge.net/cgi-bin/redirect.pl?thaw0026en

Re: emu10k1 bug and patch (0.9.0rc7)

[Takashi Iwai]

At Thu, 13 Feb 2003 21:30:17 +0100 (CET),
Jaroslav wrote:
> 
> On Mon, 10 Feb 2003, Arnaud de Bossoreille de Ribou wrote:
> 
> > So the bug looks like a signedness problem since sw_ready is unsigned
> > and there is a while(sw_ready > 0), which explain the constant delay,
> > next in the "snd_emu10k1_fx8010_playback_transfer" function.
> > 
> > So the emu10k1.patch file attached fixes the problem and seems not to
> > introduce new ones.
> 
> Please, could you try this patch, if it also fixes your problem? Thanks.

i don't think the patch fixes the original problem.
if there are only two periods, the diff will be either period_size or
-period_size, i.e. (buffer_size/2) or -(buffer_size/2).

assume the buffer size = 200
	appl_ptr

	0

	100	diff = 100 - 0 = 100

	0	diff = 0 - 100 = -100

	100


>  	if (diff) {
>  		if (diff < -(snd_pcm_sframes_t) (runtime->boundary / 2))
>  			diff += runtime->boundary;
> -		pcm->sw_ready += diff;
> +		frames += diff;
>  	}

and since boundary = buffer_size as default, the above condition will
be not satisfied (i.e. diff is equal with -(runtime->boundary/2) and
it is never smaller).

as i wrote, the condition should be <= .


btw, can the rewound length be over -(boundary/2) ?


Takashi


-------------------------------------------------------
This SF.NET email is sponsored by: FREE  SSL Guide from Thawte
are you planning your Web Server Security? Click here to get a FREE
Thawte SSL guide and find the answers to all your  SSL security issues.
http://ads.sourceforge.net/cgi-bin/redirect.pl?thaw0026en

Re: emu10k1 bug and patch (0.9.0rc7)

[tomasz motylewski]

On Fri, 14 Feb 2003, Takashi Iwai wrote:

> and since boundary = buffer_size as default, the above condition will
> be not satisfied (i.e. diff is equal with -(runtime->boundary/2) and

Is not boundary much bigger than buffer size? At least I have seen it in cs4281
driver some time ago. Is there any ALSA recommendation what value should be
used for boundary?

Best regards,
--
Tomek



-------------------------------------------------------
This SF.NET email is sponsored by: FREE  SSL Guide from Thawte
are you planning your Web Server Security? Click here to get a FREE
Thawte SSL guide and find the answers to all your  SSL security issues.
http://ads.sourceforge.net/cgi-bin/redirect.pl?thaw0026en

Re: emu10k1 bug and patch (0.9.0rc7)

[Abramo Bagnara]

tomasz motylewski wrote:
> 
> On Fri, 14 Feb 2003, Takashi Iwai wrote:
> 
> > and since boundary = buffer_size as default, the above condition will
> > be not satisfied (i.e. diff is equal with -(runtime->boundary/2) and
> 
> Is not boundary much bigger than buffer size? At least I have seen it in cs4281
> driver some time ago. Is there any ALSA recommendation what value should be
> used for boundary?

        runtime->boundary = runtime->buffer_size;
        while (runtime->boundary * 2 <= LONG_MAX - runtime->buffer_size)
                runtime->boundary *= 2;

-- 
Abramo Bagnara                       mailto:abramo.bagnara@libero.it

Opera Unica                          Phone: +39.546.656023
Via Emilia Interna, 140
48014 Castel Bolognese (RA) - Italy


-------------------------------------------------------
This SF.NET email is sponsored by: FREE  SSL Guide from Thawte
are you planning your Web Server Security? Click here to get a FREE
Thawte SSL guide and find the answers to all your  SSL security issues.
http://ads.sourceforge.net/cgi-bin/redirect.pl?thaw0026en

Re: emu10k1 bug and patch (0.9.0rc7)

[Takashi Iwai]

At Fri, 14 Feb 2003 12:31:13 +0100,
Abramo Bagnara wrote:
> 
> tomasz motylewski wrote:
> > 
> > On Fri, 14 Feb 2003, Takashi Iwai wrote:
> > 
> > > and since boundary = buffer_size as default, the above condition will
> > > be not satisfied (i.e. diff is equal with -(runtime->boundary/2) and
> > 
> > Is not boundary much bigger than buffer size? At least I have seen it in cs4281
> > driver some time ago. Is there any ALSA recommendation what value should be
> > used for boundary?
> 
>         runtime->boundary = runtime->buffer_size;
>         while (runtime->boundary * 2 <= LONG_MAX - runtime->buffer_size)
>                 runtime->boundary *= 2;

yep, right, we have the boundary size near to long_max/2.

please ignore my last message regarding this.


Takashi


-------------------------------------------------------
This SF.NET email is sponsored by: FREE  SSL Guide from Thawte
are you planning your Web Server Security? Click here to get a FREE
Thawte SSL guide and find the answers to all your  SSL security issues.
http://ads.sourceforge.net/cgi-bin/redirect.pl?thaw0026en

Re: emu10k1 bug and patch (0.9.0rc7)

[Jaroslav Kysela]

On Fri, 14 Feb 2003, Takashi Iwai wrote:

> At Thu, 13 Feb 2003 21:30:17 +0100 (CET),
> Jaroslav wrote:
> > 
> > On Mon, 10 Feb 2003, Arnaud de Bossoreille de Ribou wrote:
> > 
> > > So the bug looks like a signedness problem since sw_ready is unsigned
> > > and there is a while(sw_ready > 0), which explain the constant delay,
> > > next in the "snd_emu10k1_fx8010_playback_transfer" function.
> > > 
> > > So the emu10k1.patch file attached fixes the problem and seems not to
> > > introduce new ones.
> > 
> > Please, could you try this patch, if it also fixes your problem? Thanks.
> 
> i don't think the patch fixes the original problem.
> if there are only two periods, the diff will be either period_size or
> -period_size, i.e. (buffer_size/2) or -(buffer_size/2).
> 
> assume the buffer size = 200
> 	appl_ptr
> 
> 	0
> 
> 	100	diff = 100 - 0 = 100
> 
> 	0	diff = 0 - 100 = -100
> 
> 	100
> 
> 
> >  	if (diff) {
> >  		if (diff < -(snd_pcm_sframes_t) (runtime->boundary / 2))
> >  			diff += runtime->boundary;
> > -		pcm->sw_ready += diff;
> > +		frames += diff;
> >  	}
> 
> and since boundary = buffer_size as default, the above condition will
> be not satisfied (i.e. diff is equal with -(runtime->boundary/2) and
> it is never smaller).

The boundary is close to LONG_MAX and 'buffer_size * N = boundary'
condition is always true. We have enough space to detect the proper
rewind/boundary crosses. Isn't it?

						Jaroslav

-----
Jaroslav Kysela <perex@suse.cz>
Linux Kernel Sound Maintainer
ALSA Project, SuSE Labs



-------------------------------------------------------
This SF.NET email is sponsored by: FREE  SSL Guide from Thawte
are you planning your Web Server Security? Click here to get a FREE
Thawte SSL guide and find the answers to all your  SSL security issues.
http://ads.sourceforge.net/cgi-bin/redirect.pl?thaw0026en

Re: emu10k1 bug and patch (0.9.0rc7)

[Arnaud de Bossoreille de Ribou]

On Thu, Feb 13, 2003, Jaroslav Kysela wrote:
> On Mon, 10 Feb 2003, Arnaud de Bossoreille de Ribou wrote:
> 
> > So the bug looks like a signedness problem since sw_ready is unsigned
> > and there is a while(sw_ready > 0), which explain the constant delay,
> > next in the "snd_emu10k1_fx8010_playback_transfer" function.
> > 
> > So the emu10k1.patch file attached fixes the problem and seems not to
> > introduce new ones.
> 
> Please, could you try this patch, if it also fixes your problem? Thanks.
> 
> 
> Index: emufx.c
> ===================================================================
> RCS file: /cvsroot/alsa/alsa-kernel/pci/emu10k1/emufx.c,v
> retrieving revision 1.26
> diff -u -r1.26 emufx.c
> --- emufx.c	31 Jan 2003 15:21:03 -0000	1.26
> +++ emufx.c	13 Feb 2003 20:29:55 -0000
> @@ -532,7 +532,7 @@
>  	if (diff) {
>  		if (diff < -(snd_pcm_sframes_t) (runtime->boundary / 2))
>  			diff += runtime->boundary;
> -		pcm->sw_ready += diff;
> +		frames += diff;
>  	}
>  	pcm->sw_ready += frames;
>  	pcm->appl_ptr = appl_ptr + frames;
> 
> 						Jaroslav

It doesn't. sw_ready is negative (or above 2^31 as you like).

I think it has nothing to do with runtime->boundary since the 2 appl_ptr
are very close. The difference is always one period but I wonder why
runtime->control->appl_ptr is above pcm->appl_ptr. Is this because the
hardware has played one period that it shouldn't ?

On the other hand if the difference is really always of one period the
fix consists in conditioning the diff calculation with a
"if(frames != 0)" so that sw_ready never reaches its lower boundary.

Regards,

-- 
Arnaud


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Re: emu10k1 bug and patch (0.9.0rc7)

[Jaroslav Kysela]

On Mon, 17 Feb 2003, Arnaud de Bossoreille de Ribou wrote:

> On Thu, Feb 13, 2003, Jaroslav Kysela wrote:
> > On Mon, 10 Feb 2003, Arnaud de Bossoreille de Ribou wrote:
> > 
> > > So the bug looks like a signedness problem since sw_ready is unsigned
> > > and there is a while(sw_ready > 0), which explain the constant delay,
> > > next in the "snd_emu10k1_fx8010_playback_transfer" function.
> > > 
> > > So the emu10k1.patch file attached fixes the problem and seems not to
> > > introduce new ones.
> > 
> > Please, could you try this patch, if it also fixes your problem? Thanks.
> > 
> > 
> > Index: emufx.c
> > ===================================================================
> > RCS file: /cvsroot/alsa/alsa-kernel/pci/emu10k1/emufx.c,v
> > retrieving revision 1.26
> > diff -u -r1.26 emufx.c
> > --- emufx.c	31 Jan 2003 15:21:03 -0000	1.26
> > +++ emufx.c	13 Feb 2003 20:29:55 -0000
> > @@ -532,7 +532,7 @@
> >  	if (diff) {
> >  		if (diff < -(snd_pcm_sframes_t) (runtime->boundary / 2))
> >  			diff += runtime->boundary;
> > -		pcm->sw_ready += diff;
> > +		frames += diff;
> >  	}
> >  	pcm->sw_ready += frames;
> >  	pcm->appl_ptr = appl_ptr + frames;
> > 
> > 						Jaroslav
> 
> It doesn't. sw_ready is negative (or above 2^31 as you like).
> 
> I think it has nothing to do with runtime->boundary since the 2 appl_ptr
> are very close. The difference is always one period but I wonder why
> runtime->control->appl_ptr is above pcm->appl_ptr. Is this because the
> hardware has played one period that it shouldn't ?
> 
> On the other hand if the difference is really always of one period the
> fix consists in conditioning the diff calculation with a
> "if(frames != 0)" so that sw_ready never reaches its lower boundary.

Unfortunately, the problem is more deep that I thought. I have to enhance 
the midlevel and transfer routine only synchronizes with appl_ptr now. It
will avoid the "ping-pong" effect caused by the extra function argument
"frames".

Could you try the latest CVS sources? Thanks.

						Jaroslav

-----
Jaroslav Kysela <perex@suse.cz>
Linux Kernel Sound Maintainer
ALSA Project, SuSE Labs



-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Re: emu10k1 bug and patch (0.9.0rc7)

[Arnaud de Bossoreille de Ribou]

On Mon, Feb 17, 2003, Jaroslav Kysela wrote:
> Unfortunately, the problem is more deep that I thought. I have to enhance 
> the midlevel and transfer routine only synchronizes with appl_ptr now. It
> will avoid the "ping-pong" effect caused by the extra function argument
> "frames".
> 
> Could you try the latest CVS sources? Thanks.

The CVS sources sound OK, thank you very much.

-- 
Arnaud


-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf