Re: i810_audio

Doug Ledford (dledford@redhat.com)
Tue, 08 Jan 2002 02:59:53 -0500


Hi Thomas. I've been looking over the patch and trying to evaluate the
various bug fixes that you mention against the patch. My comments are
inline (and ignore the fact that I'm positive my cut and past job will
mangle the patch itself, this is for comments, not for applying). I'm also
replying to your comments.

Tom wrote:

> Doug,
>
> attched you can find the latest SiS7012 patch. This one is relative
> to your i810_audio.c driver.
>
> Fixes it applies except for the SiS integration:
> * drain_dac
> Use interruptible_sleep_on_timeout instead of schedule_timeout.
> I think this is cleaner.

This is fine.

> Set the timeout to
> (dmabuf->count * HZ * 2) / (dmabuf->rate * 4)
> since we play dmabuf->rate*4 bytes per second (16bit samples stereo).

This is fine as well.

> Added stop_dac at the end. Otherwise the system gets locked up sometimes.

This is (I think) a red herring. I would suspect that the lockup you are
seeing is more likely to have something to do with the changes in
i810_get_dma_address(). I'm referring to the change to the do {}while()
loop in particular. Also, if the change you made in i810_get_dma_address()
is needed for the SiS chipset, then there would appear to be a hardware bug
that needs worked around. However, I think this is the wrong way to solve
it. In prog_dmabuf(), we set the control bits on the device such that when
it finishes the LVI, it's *suppossed* to stop playback. Now, the only way I
can think of that the CIV register would update as fast as you can read it
in i810_get_dma_address() is if at the end of LVI, the card went into a loop
and the CIV is essentially running from 0 to 31 and going round and round
without doing anything. In that case, the proper fix may actually be to go
into the interrupt handler and on LVI interrupt with count==0 (for playback)
call stop_dac and do similarly for record. This is because I suspect that
we aren't ever getting the DCH Stopped interrupt on the SiS card. Of
course, the DCH Stop interrupt causes us to call stop_{dac,adc} as
appropriate, so the call to stop_dac in drain_dac is redundant if this code
is working as advertised. I suspect that fixing things in the interrupt
handler will make the stop_dac call in drain_dac and the change in
i810_get_dma_address() both unnecessary. Can you test that for me?

Something else you might want to check into on the SiS chipset. I actually
wish Intel had done the i810 with the SR and PICB registers swapped because
you are suppossed to be able to do a 32bit read from the CIV register
location and get CIV, LVI, and SR all in one go. With the SiS that means
you could get CIV, LVI, and PCIB all in one go, making the while loop in
i810_get_dma_address totally unnecessary on the SiS chipset (and making a
slight boost to performance as well) as long as the SiS also supports a
32bit read from CIV.

> * i810_read, i810_write
> Set the timeout to (dmabuf->size * HZ * 2) / (dmabuf->rate * 4)
> since we record / play dmabuf->rate*4 bytes per second (16bit samples
> stereo).

Again, fine.

> * SNDCTL_DSP_GETxPTR
> Don't change the recording / playback buffer. A detailed explanation
> can be found within the patch

As commented on by Nathan, this isn't right.

> Please correct me, if any of the above is wrong.
>
> Thomas
>

Now, I've been talking with Ben LaHaise here at Red Hat and he pointed out
to me that the wait queue handling in i810_read and likely also in
i810_write are both wrong. Ben and I are looking into that and I'll put out
a new version of the file that fixes those (obvious) problems shortly. When
I've got my 0.14 version out, could you please repatch your stuff against it
and look into the items I've mentioned here?

-- 

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

- 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/