Re: [PATCH] i810_audio fix for version 0.11

Re: [PATCH] i810_audio fix for version 0.11

[Andris Pavenis]

 > With this patch, it seems to work fine. Without, it hangs on write.

I met case when dmabuf->count==0 when __start_dac() is called. As result
I still got system freezing even if PCM_ENABLE_INPUT or 
PCM_ENABLE_OUTPUT were set accordingly (I used different patch, see 
another patch I sent today).

My latest revision of patch "survives" without problems already some 
hours (normally I'm not listening radio through internet all time, but 
this time I do ...)

Andris

Re: [PATCH] i810_audio fix for version 0.11

[Nathan Bryant]

Andris Pavenis wrote:

>  > With this patch, it seems to work fine. Without, it hangs on write.
> 
> I met case when dmabuf->count==0 when __start_dac() is called. As result
> I still got system freezing even if PCM_ENABLE_INPUT or 
> PCM_ENABLE_OUTPUT were set accordingly (I used different patch, see 
> another patch I sent today).
> 
> My latest revision of patch "survives" without problems already some 
> hours (normally I'm not listening radio through internet all time, but 
> this time I do ...)
> 
> Andris
> 
> 
> 
> 
> 
> 

i knew i shoula been a little less lazy with that one...

haven't looked at your revision yet but we should just clean up and make 
update_lvi self-contained so that it always does *something* appropriate 
regardless of state. maybe that's what you did. ;-)

(fyi, i'm not subscribed to linux-kernel, too much volume for the few 
specific interests i have, i don't see some of this stuff until, and if, 
i go digging thru archives)

Re: [PATCH] i810_audio fix for version 0.11

[Andris Pavenis]

On Fri, 7 Dec 2001, Nathan Bryant wrote:

> Andris Pavenis wrote:
> 
> >  > With this patch, it seems to work fine. Without, it hangs on write.
> > 
> > I met case when dmabuf->count==0 when __start_dac() is called. As result
> > I still got system freezing even if PCM_ENABLE_INPUT or 
> > PCM_ENABLE_OUTPUT were set accordingly (I used different patch, see 
> > another patch I sent today).
> > 
> > My latest revision of patch "survives" without problems already some 
> > hours (normally I'm not listening radio through internet all time, but 
> > this time I do ...)
> > 
> > Andris
> 
> i knew i shoula been a little less lazy with that one...
> 
> haven't looked at your revision yet but we should just clean up and make 
> update_lvi self-contained so that it always does *something* appropriate 
> regardless of state. maybe that's what you did. ;-)
> 
> (fyi, i'm not subscribed to linux-kernel, too much volume for the few 
> specific interests i have, i don't see some of this stuff until, and if, 
> i go digging thru archives)
> 

It seems that I can remove debug code from my version of update (or put
it inside '#ifdef DEBUG'). I'm torturing it practically without stop
and no problems found yet (initially listening some radio station with
RealPlayer, now put noatun playing one MP3 in a loop without stop)

About my patch: I changed __start_dac, start_dac, __start_adc and
start_adc to return integer (non zero if it is doing something at all).
I used this return code to see whether I should do a loop in 
__i810_update_lvi. 

At least I haven't got any message from debuging output I left in.

Andris

Re: [PATCH] i810_audio fix for version 0.11

[Doug Ledford]

Nathan Bryant wrote:

> Andris Pavenis wrote:
>
>>  > With this patch, it seems to work fine. Without, it hangs on write.
>>
>> I met case when dmabuf->count==0 when __start_dac() is called. As result
>> I still got system freezing even if PCM_ENABLE_INPUT or 
>> PCM_ENABLE_OUTPUT were set accordingly (I used different patch, see 
>> another patch I sent today).
>>
>> My latest revision of patch "survives" without problems already some 
>> hours (normally I'm not listening radio through internet all time, 
>> but this time I do ...)
>>
>> Andris
>>
>>
>>
>>
>>
>>
>
> i knew i shoula been a little less lazy with that one...
>
> haven't looked at your revision yet but we should just clean up and 
> make update_lvi self-contained so that it always does *something* 
> appropriate regardless of state. maybe that's what you did. ;-)
>
> (fyi, i'm not subscribed to linux-kernel, too much volume for the few 
> specific interests i have, i don't see some of this stuff until, and 
> if, i go digging thru archives)
>
Well, unfortunately, neither of the patches you guys sent do what I was 
looking for ;-)  My goal with that code was to enable a specific certain 
behaviour, and because of the deadlock I have to make a few changes 
elsewhere for it to work properly.  The workaround patches are fine for 
now, but later today I'll make a 0.12 that fixes it the way I'm looking 
for.  (Hint: it's legal for a program to call SETTRIGGER to disable PCM 
output, then call the write() routine to fill the buffer, then call 
SETTRIGGER again to start output, otherwise known as pre buffering, and 
I want to support that without forcing the DAC to be started on 
update_lvi())

The real answer is multipart:

1) during i810_open go back to the old behaviour of setting 
dmabuf->trigger to PCM_ENABLE_INPUT and/or OUTPUT based on file mode.

2) make sure that i810_mmap clears dmabuf->trigger

3) make sure that in both i810_write and i810_read, we force the trigger 
setting when we can't output/input any data because count <= 0

4) in update_lvi make the check something like:

if (!dmabuf->enable && dmabuf->trigger) {
    ....
}

That should solve the problem, I just haven't written it up yet.

Re: [PATCH] i810_audio fix for version 0.11

[Doug Ledford]

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

Doug Ledford wrote:

> Well, unfortunately, neither of the patches you guys sent do what I 
> was looking for ;-)  My goal with that code was to enable a specific 
> certain behaviour, and because of the deadlock I have to make a few 
> changes elsewhere for it to work properly.  The workaround patches are 
> fine for now, but later today I'll make a 0.12 that fixes it the way 
> I'm looking for.  (Hint: it's legal for a program to call SETTRIGGER 
> to disable PCM output, then call the write() routine to fill the 
> buffer, then call SETTRIGGER again to start output, otherwise known as 
> pre buffering, and I want to support that without forcing the DAC to 
> be started on update_lvi())
>
> The real answer is multipart:
>
> 1) during i810_open go back to the old behaviour of setting 
> dmabuf->trigger to PCM_ENABLE_INPUT and/or OUTPUT based on file mode.
>
> 2) make sure that i810_mmap clears dmabuf->trigger
>
> 3) make sure that in both i810_write and i810_read, we force the 
> trigger setting when we can't output/input any data because count <= 0
>
> 4) in update_lvi make the check something like:
>
> if (!dmabuf->enable && dmabuf->trigger) {
>    ....
> }
>
> That should solve the problem, I just haven't written it up yet.
>
>
>
OK, the attached patch should do what is mentioned above.

Doug Ledford


[-- Attachment #2: patch-12 --]
[-- Type: text/plain, Size: 1577 bytes --]

--- i810_audio.c.11	Thu Dec  6 16:53:41 2001
+++ i810_audio.c.12	Fri Dec  7 13:32:38 2001
@@ -198,7 +198,7 @@
 #define INT_MASK (INT_SEC|INT_PRI|INT_MC|INT_PO|INT_PI|INT_MO|INT_NI|INT_GPI)
 
 
-#define DRIVER_VERSION "0.11"
+#define DRIVER_VERSION "0.12"
 
 /* magic numbers to protect our data structures */
 #define I810_CARD_MAGIC		0x5072696E /* "Prin" */
@@ -952,7 +952,7 @@
 	 * the CIV value to the next sg segment to be played so that when
 	 * we call start_{dac,adc}, things will operate properly
 	 */
-	if (!dmabuf->enable) {
+	if (!dmabuf->enable && dmabuf->trigger) {
 		outb((inb(port+OFF_CIV)+1)&31, port+OFF_LVI);
 		if(rec) {
 			__start_adc(state);
@@ -1111,8 +1111,11 @@
 
 		/* 
 		 * This will make sure that our LVI is correct, that our
-		 * pointer is updated, and that the DAC is running
+		 * pointer is updated, and that the DAC is running.  We
+		 * have to force the setting of dmabuf->trigger to avoid
+		 * any possible deadlocks.
 		 */
+		dmabuf->trigger = PCM_ENABLE_OUTPUT;
 		i810_update_lvi(state,0);
 
 		if (signal_pending(current))
@@ -2280,6 +2283,7 @@
 			card->states[i] = NULL;;
 			return -EBUSY;
 		}
+		dmabuf->trigger |= PCM_ENABLE_INPUT;
 		i810_set_adc_rate(state, 8000);
 	}
 	if(file->f_mode & FMODE_WRITE) {
@@ -2291,6 +2295,7 @@
 		/* Initialize to 8kHz?  What if we don't support 8kHz? */
 		/*  Let's change this to check for S/PDIF stuff */
 	
+		dmabuf->trigger |= PCM_ENABLE_OUTPUT;
 		if ( spdif_locked ) {
 			i810_set_dac_rate(state, spdif_locked);
 			i810_set_spdif_output(state, AC97_EA_SPSA_3_4, spdif_locked);

Re: [PATCH] i810_audio fix for version 0.11

[Andris Pavenis]

Sorry, but this patch is still not OK. It still causes system
locking up for me.

In some cases I have (I added printk in __start_dac):
	dmabuf->count = 0
	dmabuf->ready = 1
	dmabuf->enable = 1
	PCM_ENABLE_OUTPUT set in dmabuf->triger

in __start_dac(). As result nothing was done in this procedure
and I got system locking in __i810_update_lvi() immediatelly after
that. This was reason why I added return code to __start_dac,
__start_adc to skip the loop if there is nothing to wait for.
Perhaps checking return code is more efficient and less error prone
that repeating all conditions in __i810_update_lvi. 

Maybe really we should use wait_event_interruptible() instead
of plain loop in __i810_update_lvi(). This happens not so often,
so I don't think it could cause too big delays. At least we could
avoid kernel freezing in this way.

Andris

On Fri, 7 Dec 2001, Doug Ledford wrote:

> Doug Ledford wrote:
> 
> > Well, unfortunately, neither of the patches you guys sent do what I 
> > was looking for ;-)  My goal with that code was to enable a specific 
> > certain behaviour, and because of the deadlock I have to make a few 
> > changes elsewhere for it to work properly.  The workaround patches are 
> > fine for now, but later today I'll make a 0.12 that fixes it the way 
> > I'm looking for.  (Hint: it's legal for a program to call SETTRIGGER 
> > to disable PCM output, then call the write() routine to fill the 
> > buffer, then call SETTRIGGER again to start output, otherwise known as 
> > pre buffering, and I want to support that without forcing the DAC to 
> > be started on update_lvi())
> >
> > The real answer is multipart:
> >
> > 1) during i810_open go back to the old behaviour of setting 
> > dmabuf->trigger to PCM_ENABLE_INPUT and/or OUTPUT based on file mode.
> >
> > 2) make sure that i810_mmap clears dmabuf->trigger
> >
> > 3) make sure that in both i810_write and i810_read, we force the 
> > trigger setting when we can't output/input any data because count <= 0
> >
> > 4) in update_lvi make the check something like:
> >
> > if (!dmabuf->enable && dmabuf->trigger) {
> >    ....
> > }
> >
> > That should solve the problem, I just haven't written it up yet.
> >
> >
> >
> OK, the attached patch should do what is mentioned above.
> 
> Doug Ledford
> 
> 

Re: [PATCH] i810_audio fix for version 0.11

[Andris Pavenis]

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

On Saturday 08 December 2001 10:39, Andris Pavenis wrote:
> Sorry, but this patch is still not OK. It still causes system
> locking up for me.
>
> In some cases I have (I added printk in __start_dac):
> 	dmabuf->count = 0
> 	dmabuf->ready = 1
> 	dmabuf->enable = 1
> 	PCM_ENABLE_OUTPUT set in dmabuf->triger
>
> in __start_dac(). As result nothing was done in this procedure
> and I got system locking in __i810_update_lvi() immediatelly after
> that. This was reason why I added return code to __start_dac,
> __start_adc to skip the loop if there is nothing to wait for.
> Perhaps checking return code is more efficient and less error prone
> that repeating all conditions in __i810_update_lvi.
>
> Maybe really we should use wait_event_interruptible() instead
> of plain loop in __i810_update_lvi(). This happens not so often,
> so I don't think it could cause too big delays. At least we could
> avoid kernel freezing in this way.
>

Here is my updated patch against v0.12 (I changed version to 0.12a to point 
a version against which is the patch)

Andris

[-- Attachment #2: i810_audio.c.diff3 --]
[-- Type: text/x-diff, Size: 2724 bytes --]

--- i810_audio.c-0.12	Sat Dec  8 10:24:24 2001
+++ i810_audio.c	Sat Dec  8 11:14:16 2001
@@ -198,7 +198,7 @@
 #define INT_MASK (INT_SEC|INT_PRI|INT_MC|INT_PO|INT_PI|INT_MO|INT_NI|INT_GPI)
 
 
-#define DRIVER_VERSION "0.12"
+#define DRIVER_VERSION "0.12a"
 
 /* magic numbers to protect our data structures */
 #define I810_CARD_MAGIC		0x5072696E /* "Prin" */
@@ -690,25 +690,30 @@
 	spin_unlock_irqrestore(&card->lock, flags);
 }
 
-static inline void __start_adc(struct i810_state *state)
+static inline int __start_adc(struct i810_state *state)
 {
+	int ret = 0;
 	struct dmabuf *dmabuf = &state->dmabuf;
 
 	if (dmabuf->count < dmabuf->dmasize && dmabuf->ready && !dmabuf->enable &&
 	    (dmabuf->trigger & PCM_ENABLE_INPUT)) {
+		ret = 1;
 		dmabuf->enable |= ADC_RUNNING;
 		outb((1<<4) | (1<<2) | 1, state->card->iobase + PI_CR);
 	}
+	return ret;
 }
 
-static void start_adc(struct i810_state *state)
+static int start_adc(struct i810_state *state)
 {
+	int ret;
 	struct i810_card *card = state->card;
 	unsigned long flags;
 
 	spin_lock_irqsave(&card->lock, flags);
-	__start_adc(state);
+	ret = __start_adc(state);
 	spin_unlock_irqrestore(&card->lock, flags);
+	return ret;
 }
 
 /* stop playback (lock held) */
@@ -736,24 +741,29 @@
 	spin_unlock_irqrestore(&card->lock, flags);
 }	
 
-static inline void __start_dac(struct i810_state *state)
+static inline int __start_dac(struct i810_state *state)
 {
+	int ret = 0;
 	struct dmabuf *dmabuf = &state->dmabuf;
 
 	if (dmabuf->count > 0 && dmabuf->ready && !dmabuf->enable &&
 	    (dmabuf->trigger & PCM_ENABLE_OUTPUT)) {
+		ret = 1;
 		dmabuf->enable |= DAC_RUNNING;
 		outb((1<<4) | (1<<2) | 1, state->card->iobase + PO_CR);
 	}
+	return ret;
 }
-static void start_dac(struct i810_state *state)
+static int start_dac(struct i810_state *state)
 {
+	int ret;
 	struct i810_card *card = state->card;
 	unsigned long flags;
 
 	spin_lock_irqsave(&card->lock, flags);
-	__start_dac(state);
+	ret = __start_dac(state);
 	spin_unlock_irqrestore(&card->lock, flags);
+	return ret;
 }
 
 #define DMABUF_DEFAULTORDER (16-PAGE_SHIFT)
@@ -936,7 +946,7 @@
 static void __i810_update_lvi(struct i810_state *state, int rec)
 {
 	struct dmabuf *dmabuf = &state->dmabuf;
-	int x, port;
+	int x, port, ok;
 	
 	port = state->card->iobase;
 	if(rec)
@@ -955,11 +965,12 @@
 	if (!dmabuf->enable && dmabuf->trigger) {
 		outb((inb(port+OFF_CIV)+1)&31, port+OFF_LVI);
 		if(rec) {
-			__start_adc(state);
+			ok = __start_adc(state);
 		} else {
-			__start_dac(state);
+			ok = __start_dac(state);
 		}
-		while( !(inb(port + OFF_CR) & ((1<<4) | (1<<2))) ) ;
+		
+		if (ok) while( !(inb(port + OFF_CR) & ((1<<4) | (1<<2))) ) ;
 	}
 
 	/* swptr - 1 is the tail of our transfer */

Re: [PATCH] i810_audio fix for version 0.11

[Doug Ledford]

Andris Pavenis wrote:

>On Saturday 08 December 2001 10:39, Andris Pavenis wrote:
>
>>Sorry, but this patch is still not OK. It still causes system
>>locking up for me.
>>
>>In some cases I have (I added printk in __start_dac):
>>	dmabuf->count = 0
>>	dmabuf->ready = 1
>>	dmabuf->enable = 1
>>	PCM_ENABLE_OUTPUT set in dmabuf->triger
>>
Actually, since the problem is that there are obviously some "just in 
case" type calls if i810_update_lvi(), the best answer is not to even go 
through all those motions when dmabuf->count == 0.  So, I would add a 
line to i810_update_lvi() that makes it return without doing anything 
when dmabuf->count == 0.  That one line should solve your lockups (and 
finalize the 0.12 version).

>>
>>in __start_dac(). As result nothing was done in this procedure
>>and I got system locking in __i810_update_lvi() immediatelly after
>>that. This was reason why I added return code to __start_dac,
>>__start_adc to skip the loop if there is nothing to wait for.
>>Perhaps checking return code is more efficient and less error prone
>>that repeating all conditions in __i810_update_lvi.
>>
>>Maybe really we should use wait_event_interruptible() instead
>>of plain loop in __i810_update_lvi(). This happens not so often,
>>so I don't think it could cause too big delays. At least we could
>>avoid kernel freezing in this way.
>>
>
>Here is my updated patch against v0.12 (I changed version to 0.12a to point 
>a version against which is the patch)
>
>Andris
>
>
>------------------------------------------------------------------------
>
>--- i810_audio.c-0.12	Sat Dec  8 10:24:24 2001
>+++ i810_audio.c	Sat Dec  8 11:14:16 2001
>@@ -198,7 +198,7 @@
> #define INT_MASK (INT_SEC|INT_PRI|INT_MC|INT_PO|INT_PI|INT_MO|INT_NI|INT_GPI)
> 
> 
>-#define DRIVER_VERSION "0.12"
>+#define DRIVER_VERSION "0.12a"
> 
> /* magic numbers to protect our data structures */
> #define I810_CARD_MAGIC		0x5072696E /* "Prin" */
>@@ -690,25 +690,30 @@
> 	spin_unlock_irqrestore(&card->lock, flags);
> }
> 
>-static inline void __start_adc(struct i810_state *state)
>+static inline int __start_adc(struct i810_state *state)
> {
>+	int ret = 0;
> 	struct dmabuf *dmabuf = &state->dmabuf;
> 
> 	if (dmabuf->count < dmabuf->dmasize && dmabuf->ready && !dmabuf->enable &&
> 	    (dmabuf->trigger & PCM_ENABLE_INPUT)) {
>+		ret = 1;
> 		dmabuf->enable |= ADC_RUNNING;
> 		outb((1<<4) | (1<<2) | 1, state->card->iobase + PI_CR);
> 	}
>+	return ret;
> }
> 
>-static void start_adc(struct i810_state *state)
>+static int start_adc(struct i810_state *state)
> {
>+	int ret;
> 	struct i810_card *card = state->card;
> 	unsigned long flags;
> 
> 	spin_lock_irqsave(&card->lock, flags);
>-	__start_adc(state);
>+	ret = __start_adc(state);
> 	spin_unlock_irqrestore(&card->lock, flags);
>+	return ret;
> }
> 
> /* stop playback (lock held) */
>@@ -736,24 +741,29 @@
> 	spin_unlock_irqrestore(&card->lock, flags);
> }	
> 
>-static inline void __start_dac(struct i810_state *state)
>+static inline int __start_dac(struct i810_state *state)
> {
>+	int ret = 0;
> 	struct dmabuf *dmabuf = &state->dmabuf;
> 
> 	if (dmabuf->count > 0 && dmabuf->ready && !dmabuf->enable &&
> 	    (dmabuf->trigger & PCM_ENABLE_OUTPUT)) {
>+		ret = 1;
> 		dmabuf->enable |= DAC_RUNNING;
> 		outb((1<<4) | (1<<2) | 1, state->card->iobase + PO_CR);
> 	}
>+	return ret;
> }
>-static void start_dac(struct i810_state *state)
>+static int start_dac(struct i810_state *state)
> {
>+	int ret;
> 	struct i810_card *card = state->card;
> 	unsigned long flags;
> 
> 	spin_lock_irqsave(&card->lock, flags);
>-	__start_dac(state);
>+	ret = __start_dac(state);
> 	spin_unlock_irqrestore(&card->lock, flags);
>+	return ret;
> }
> 
> #define DMABUF_DEFAULTORDER (16-PAGE_SHIFT)
>@@ -936,7 +946,7 @@
> static void __i810_update_lvi(struct i810_state *state, int rec)
> {
> 	struct dmabuf *dmabuf = &state->dmabuf;
>-	int x, port;
>+	int x, port, ok;
> 	
> 	port = state->card->iobase;
> 	if(rec)
>@@ -955,11 +965,12 @@
> 	if (!dmabuf->enable && dmabuf->trigger) {
> 		outb((inb(port+OFF_CIV)+1)&31, port+OFF_LVI);
> 		if(rec) {
>-			__start_adc(state);
>+			ok = __start_adc(state);
> 		} else {
>-			__start_dac(state);
>+			ok = __start_dac(state);
> 		}
>-		while( !(inb(port + OFF_CR) & ((1<<4) | (1<<2))) ) ;
>+		
>+		if (ok) while( !(inb(port + OFF_CR) & ((1<<4) | (1<<2))) ) ;
> 	}
> 
> 	/* swptr - 1 is the tail of our transfer */
>

Re: [PATCH] i810_audio fix for version 0.11

[Andris Pavenis]

On Saturday 08 December 2001 11:36, Doug Ledford wrote:
> Andris Pavenis wrote:
> >On Saturday 08 December 2001 10:39, Andris Pavenis wrote:
> >>Sorry, but this patch is still not OK. It still causes system
> >>locking up for me.
> >>
> >>In some cases I have (I added printk in __start_dac):
> >>	dmabuf->count = 0
> >>	dmabuf->ready = 1
> >>	dmabuf->enable = 1
> >>	PCM_ENABLE_OUTPUT set in dmabuf->triger
>
> Actually, since the problem is that there are obviously some "just in
> case" type calls if i810_update_lvi(), the best answer is not to even go
> through all those motions when dmabuf->count == 0.  So, I would add a
> line to i810_update_lvi() that makes it return without doing anything
> when dmabuf->count == 0.  That one line should solve your lockups (and
> finalize the 0.12 version).
>

Why returning non zero from __start_dac() and similar procedures when 
something real has been done there is so bad. Using such return code would
ensure we never try to wait for results of __start_dac() if nothing is done 
by this procedure. I think such way is also more safe against possible future 
modifications as real conditions are only in a single place. Keeping them in 
2 places is possible source of bitrot if driver will be updated in future.

Andris

Re: [PATCH] i810_audio fix for version 0.11

[Doug Ledford]

Andris Pavenis wrote:

> Why returning non zero from __start_dac() and similar procedures when 
> something real has been done there is so bad. 


Personal preference.

> Using such return code would
> ensure we never try to wait for results of __start_dac() if nothing is done 
> by this procedure.


That's part of the point.  In this driver, I try to control when things are 
done and keep track of them in a deterministic way.  Using a return code to 
tell us a function we called did nothing when we shouldn't have called it in 
the first place if it wasn't going to do anything is backwards from the way 
I prefer to handle things.  Namely, find out why the function was called 
when it shouldn't have been and solve the problem.  Note: I don't follow 
that philosophy on all functions, only on very simple ones like this, there 
are a lot of complex functions where you want the function to make those 
decisions.  So, like I said, personal preference on how to handle these things.

> I think such way is also more safe against possible future 
> modifications as real conditions are only in a single place. Keeping them in 
> 2 places is possible source of bitrot if driver will be updated in future.


It's intended to do exactly that.  A lot of what makes this driver work 
properly right now is the LVI handling.  That was severly busted when I 
first got hold of the driver.  I *want* things to break if the LVI handling 
is changed by someone else because that will alert me to the fact that the 
LVI handling is then busted (at least, if they change it incorrectly, if 
they do things right then they will catch problems like this and fix them 
properly and I won't have to do anything).





-- 

  Doug Ledford <dledford@redhat.com>  http://people.redhat.com/dledford
       Please check my web site for aic7xxx updates/answers before
                       e-mailing me about problems

Re: [PATCH] i810_audio fix for version 0.11

[Andris Pavenis]

On Tuesday 11 December 2001 02:42, Doug Ledford wrote:
> Andris Pavenis wrote:
> > Why returning non zero from __start_dac() and similar procedures when
> > something real has been done there is so bad.
>
> Personal preference.

Thought about slightly different idea but didn't test it (I'm now about 300 km
away from a box where I did the tests and it will be so up to Cristmas):

	move both port operations used before and after call to
	__start_dac() and __start_adc() inside these inline procedures.
	In this case we would have waiting for results of __start_dac
	only when it really needed, so no possibility of deadlock
	there

>
> > Using such return code would
> > ensure we never try to wait for results of __start_dac() if nothing is
> > done by this procedure.
>
> That's part of the point.  In this driver, I try to control when things are
> done and keep track of them in a deterministic way.  Using a return code to
> tell us a function we called did nothing when we shouldn't have called it
> in the first place if it wasn't going to do anything is backwards from the
> way I prefer to handle things.  Namely, find out why the function was
> called when it shouldn't have been and solve the problem.  Note: I don't

If we moved 
                outb((inb(port+OFF_CIV)+1)&31, port+OFF_LVI);
and 
                while( !(inb(port + OFF_CR) & ((1<<4) | (1<<2))) ) ;
from inside __i810_update_lvi to __start_adc and __start_dac (inside
if block) we would avoid deadlocks. If You like to have noticed that
__start_dac ot __start_adc is called when they should not, then
add printk with message there (or even disable sound support totally with 
additional error message in log, so user will complain). I think leaving out 
possiblility of deadlocks is too dangerous.

> follow that philosophy on all functions, only on very simple ones like
> this, there are a lot of complex functions where you want the function to
> make those decisions.  So, like I said, personal preference on how to
> handle these things.
>
> > I think such way is also more safe against possible future
> > modifications as real conditions are only in a single place. Keeping them
> > in 2 places is possible source of bitrot if driver will be updated in
> > future.
>
> It's intended to do exactly that.  A lot of what makes this driver work
> properly right now is the LVI handling.  That was severly busted when I
> first got hold of the driver.  I *want* things to break if the LVI handling
> is changed by someone else because that will alert me to the fact that the
> LVI handling is then busted (at least, if they change it incorrectly, if
> they do things right then they will catch problems like this and fix them
> properly and I won't have to do anything).

I would suggest to disable sound support and give reasonable error message
in this case. So user will able to complain if this happens. It's more 
difficult to get usefull report when deadlock happens (and many users may not 
want to debug and provide additional info it in this case any more)

Andris

i810_audio driver version 0.13 still broken

[Andris Pavenis]

Now returned in Riga and tested i810_audio driver version 0.13 from 
	http://people.redhat.com/dledford/i810_audio.c.gz
with kernel 2.4.17

It still hanged machine after playing KDE startup sound. Didn't tried much 
more and moved to my modified version of 0.12 which worked

With best regards

Andris

Re: i810_audio driver version 0.13 still broken

[Nathan Bryant]

Andris Pavenis wrote:

>It still hanged machine after playing KDE startup sound. Didn't tried much 
>more and moved to my modified version of 0.12 which worked
>
please send me your modified version again. full file would be best, not 
patch.

i can't reproduce any problems here, so far, so i'm stabbing in the 
dark. if you can give more detailed information to reproduce, that would 
be nice: hardware versions, version of KDE, kde settings, (mine doesn't 
play a startup sound and artsd works fine for everything else i try), 
artsd settings. if i can reproduce, i can analyze on my own machine, if 
not, outlook is hazy ;-)

Re: i810_audio driver version 0.13 still broken

[Andris Pavenis]

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

On Thursday 27 December 2001 23:44, Nathan Bryant wrote:
> Andris Pavenis wrote:
> >It still hanged machine after playing KDE startup sound. Didn't tried much
> >more and moved to my modified version of 0.12 which worked
>
> please send me your modified version again. full file would be best, not
> patch.

Got also my patched 0.12 freezing system later (kernel 2.4.17). Didn't saw
that earlier with 2.4.17-pre2. Also tried to move port I/O stuff from
__i810_update_lvi() to start_adc() and start_dac() inside if block to 
avoid any possibility of doing wait loop when noting is done in start_dac()
and still get system locking up when loading driver. Perhaps I should
recheck with kernel 2.4.17-pre2 (I didn't have problems with it before
leaving to Finland now more than 2 weeks ago)

>
> i can't reproduce any problems here, so far, so i'm stabbing in the
> dark. if you can give more detailed information to reproduce, that would
> be nice: hardware versions, version of KDE, kde settings, (mine doesn't
> play a startup sound and artsd works fine for everything else i try),
> artsd settings. if i can reproduce, i can analyze on my own machine, if
> not, outlook is hazy ;-)

Kernel 2.4.17 compiled with gcc-2.95.3, devfs is enabled, ext3 filesystem (I
	 would prefer not to hang system with ext2, ext3 seems to
	handle that nicely)
XFree86-4.1.99.2 (cvs  update -r xf-4_1_99_2 xc; ....)
KDE-2.2.2 release (compiled here with gcc-2.95.3)
Contents of /proc/pci attached

KDE sound server settings:
	Start aRts soundserver on KDE startup - on
	Enable network transperancy - on
	Exchange security and reference info over the X11 server - on
	Run soundserver with realtime priority - off
	Autosuspend if idle for 5 seconds
	Display messages using artsmessage
	Message display: errors

	Sound I/O method: autodetect
	Enable full duplex operation: off
	Use custom sound device: off
	Use custom sampling rate: off
	Other custom options: off
	Audio buffer size: 278 millisoconds (12 fragments with 4096 bytes)

	I have setup to play one wav file from VFAT32 partition at KDE startup

Perhaps I'll look into that in next Year only. It seems I'll have to repeat 
enabling debug output from i810_audio driver and to put additional output 
there to see where I have the trouble.

Andris

[-- Attachment #2: proc_pci --]
[-- Type: text/plain, Size: 1471 bytes --]

PCI devices found:
  Bus  0, device   0, function  0:
    Host bridge: Intel Corp. 82810 GMCH [Graphics Memory Controller Hub] (rev 3).
  Bus  0, device   1, function  0:
    VGA compatible controller: Intel Corp. 82810 CGC [Chipset Graphics Controller] (rev 3).
      IRQ 11.
      Prefetchable 32 bit memory at 0xe4000000 [0xe7ffffff].
      Non-prefetchable 32 bit memory at 0xe3800000 [0xe387ffff].
  Bus  0, device  30, function  0:
    PCI bridge: Intel Corp. 82801AA PCI Bridge (rev 2).
      Master Capable.  No bursts.  Min Gnt=6.
  Bus  0, device  31, function  0:
    ISA bridge: Intel Corp. 82801AA ISA Bridge (LPC) (rev 2).
  Bus  0, device  31, function  1:
    IDE interface: Intel Corp. 82801AA IDE (rev 2).
      I/O at 0xb800 [0xb80f].
  Bus  0, device  31, function  2:
    USB Controller: Intel Corp. 82801AA USB (rev 2).
      IRQ 9.
      I/O at 0xb400 [0xb41f].
  Bus  0, device  31, function  3:
    SMBus: Intel Corp. 82801AA SMBus (rev 2).
      IRQ 10.
      I/O at 0xe800 [0xe80f].
  Bus  0, device  31, function  5:
    Multimedia audio controller: Intel Corp. 82801AA AC'97 Audio (rev 2).
      IRQ 10.
      I/O at 0xe000 [0xe0ff].
      I/O at 0xe100 [0xe13f].
  Bus  1, device  10, function  0:
    Ethernet controller: Realtek Semiconductor Co., Ltd. RTL-8139 (rev 16).
      IRQ 9.
      Master Capable.  Latency=64.  Min Gnt=32.Max Lat=64.
      I/O at 0xd800 [0xd8ff].
      Non-prefetchable 32 bit memory at 0xe3000000 [0xe30000ff].

Re: i810_audio driver version 0.13 still broken

[Nathan Bryant]

Andris Pavenis wrote:

>Got also my patched 0.12 freezing system later (kernel 2.4.17). Didn't saw
>that earlier with 2.4.17-pre2. Also tried to move port I/O stuff from
>__i810_update_lvi() to start_adc() and start_dac() inside if block to 
>avoid any possibility of doing wait loop when noting is done in start_dac()
>and still get system locking up when loading driver. Perhaps I should
>recheck with kernel 2.4.17-pre2 (I didn't have problems with it before
>leaving to Finland now more than 2 weeks ago)
>
freeze happens on insmod, you mean? or on open("/dev/dsp") or on 
write()? (strace can help check)

>KDE-2.2.2 release (compiled here with gcc-2.95.3)
>
same here, but official kde.org rpm's for redhat (some version of gcc-2.96)

>
>Contents of /proc/pci attached
>
[snip]
only h/w difference visible from my setup from /proc/pci is i have an 
i815 with an Nvidia AGP card and you have an i810 with integrated graphics.

what AC'97 codec is attached to the ICH? (dmesg should say upon driver load)

Re: i810_audio driver version 0.13 still broken

[Nick Papadonis]

Andris Pavenis <pavenis@latnet.lv> writes:

> Now returned in Riga and tested i810_audio driver version 0.13 from 
> 	http://people.redhat.com/dledford/i810_audio.c.gz
> with kernel 2.4.17
> 
> It still hanged machine after playing KDE startup sound. Didn't tried much 
> more and moved to my modified version of 0.12 which worked
> 
> With best regards
> 
> Andris
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Doug's 0.12 driver fixed my problems.  ESD would not detect my audio
device if I loaded i810_audio after XF86.

-- 
Nick

Re: i810_audio driver version 0.13 still broken

[Andris Pavenis]

On Thursday 27 December 2001 23:44, Nathan Bryant wrote:
> Andris Pavenis wrote:
> >It still hanged machine after playing KDE startup sound. Didn't tried much
> >more and moved to my modified version of 0.12 which worked
>
> please send me your modified version again. full file would be best, not
> patch.
>
> i can't reproduce any problems here, so far, so i'm stabbing in the
> dark. if you can give more detailed information to reproduce, that would
> be nice: hardware versions, version of KDE, kde settings, (mine doesn't
> play a startup sound and artsd works fine for everything else i try),
> artsd settings. if i can reproduce, i can analyze on my own machine, if
> not, outlook is hazy ;-)

Seems that latest driver version from
	http://www.infosys.tuwien.ac.at/Staff/tom/SiS7012/
works for me (at least after some hours) with kernel 2.4.17

Andris

[PATCH] i810_audio fix for version 0.11

[Nathan Bryant]

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


With this patch, it seems to work fine. Without, it hangs on write.

[-- Attachment #2: 11fixed.diff --]
[-- Type: text/plain, Size: 538 bytes --]

--- i810_audio.c.11	Thu Dec  6 18:07:35 2001
+++ linux/drivers/sound/i810_audio.c	Thu Dec  6 21:27:42 2001
@@ -955,8 +955,13 @@
 	if (!dmabuf->enable) {
 		outb((inb(port+OFF_CIV)+1)&31, port+OFF_LVI);
 		if(rec) {
+			/* must set trigger or we won't really start the
+			   converter, and we'll hang waiting for it to
+			   start. */
+			dmabuf->trigger = PCM_ENABLE_INPUT;
 			__start_adc(state);
 		} else {
+			dmabuf->trigger = PCM_ENABLE_OUTPUT;
 			__start_dac(state);
 		}
 		while( !(inb(port + OFF_CR) & ((1<<4) | (1<<2))) ) ;