[alsa-devel] [PATCH] ad1838/cs4231 -- fix MCE timeout upon initial load

[Trent Piepho]

On Mon, 17 Sep 2007, Krzysztof Helt wrote:
> On Mon, 17 Sep 2007 14:04:21 -0700 (PDT)
> Trent Piepho <xyzzy at speakeasy.org> wrote:
>
> >
> > First, you call msleep(1) while holding reg_lock with interrupts disabled.
> > That's sleeping while atomic.  You should drop the lock first, or use
> > mdelay().
> >
> Good catch. If we are going after cs4231_lib, we can drop lock in loops
> of the mce_down function (the driver only reads registers).

It looks like one needs to have the lock to access (read or write) any of
the "indirect" registers.  For example, in order to read from
AD1848_TEST_INIT, one must first write the index into the REGSEL register,
then read from the REG register.  If another thread tried to concurrently
read or write a different indirect register, it would modify the index.

> > This would explain what is happening when Krzysztof said, "The waiting loop
> > was passed 340 times after mce was down with msleep(1)." 340 loops at 250 ms
> > per loop is 85 seconds.  That would mean a call to hw_params() takes 85
> > seconds, and open(), with three mce_downs, would take over four minutes!
> >
>
> No, the loop never had the 250ms delay. The whole loop (all iterations) was 250ms delay.

That's what I said.  It obviously didn't have a 250 ms delay, or open()
would take four minutes.  Note that the delay was not 250 ms total either,
it would loop forever until ACI went down.

for(time = 250; time > 0 ; ) time = schedule_timeout(time);

Unless you set the task state first, that line is an infinite busy loop.

> > I looked at the ad1848 datasheet, and it says the auto-calibration should take
> > about 384 samples (or ~128, which I think is the time it takes ACI to go low
> > when auto-calibration is not on).  That would be 70 ms at the high end and
> > typically more like 3 ms.
>
> The smallest value for ad1848 is over 7ms (from experiments).
> It is about 384 samples at 48kHz.

You said before that it took 3 jiffies on a 1000 Hz kernel.  I guess
auto-calibration is always on when MCE goes down?  It looks like it should
only take 128 samples when MCE goes down and auto-calibration isn't done.

> > The wait for the init bit to be off after the check for ACI seems unneeded
> > too.  snd_ad1848_in(), called while waiting for ACI to go low, does the exact
> > same thing when it calls snd_ad1848_wait().  So the INIT bit is already off at
> > this point.
> >
>
> Good catch again. Just look into the "Changing sample rates" section.
> Maybe waiting loop for the init bit off should be the first?

snd_ad1848_in() already calls snd_ad1848_wait(), so it seems like it
would be redundant.

> > Here is a patch to fix all these problems and simplify the code in the
> > process.  I picked a value of 3 ms for the polling interval.  Obviously, there
> > is a trade-off in selecting the value.  The smaller the delay, the less wasted
> > time from when ACI goes low to when it is detected.  But it also means more
> > iterations of the loop, and thus more scheduling and interrupt disabling.
> >
>
> We may drop interrupts disabling in waiting loops, because inside them the
> driver only reads registers.

If reg_lock is ever acquired from within an interrupt handler, you must
disabled interrupts when you acquire it.  If it's never acquired from
within an interrupt handler, then you never need to disable interrupts.

> > I think a good way to pick a value, is to try select the smallest value such
> > that 90% of the time the loop will finish on the first check.  It seems like 3
> > or maybe 4 would do that.
>
> This is not optimal from the latency point of view. The 3ms delay inside the loop
> means that one must wait either 3 or 6 or 9 ms. Event if the real hardware answers
> correctly after 4ms.

Obviously from a latency point of view the optimal value would be to poll
as fast as possible.  Which is the worst from an efficiency point of view.
So you make a trade off.  If 90% of the time it's done by 2.9 ms, then
waiting 3 ms means you have very little extra latency 90% of the time and
only poll once 90% of the time.

> > If you're not concerned much about efficiency, and just want to detect ACI low
> > as soon as possible, you'd use 1 ms.  Or if you want to get more complicated,
> > delay 3 ms on the first iteration, and 1 ms thereafter.
>
> The whole point of sleep before loop is to make this first iteration outside the loop
> so it is not obscured by any special condition.

I'm well aware of that.  By delaying first, then checking, it's no longer
necessary to have an extra delay before the loop.