[lkml]   [2002]   [Jan]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: i810_audio

    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.

    > 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 <>
    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
    More majordomo info at
    Please read the FAQ at

     \ /
      Last update: 2005-03-22 13:15    [W:0.025 / U:4.608 seconds]
    ©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site