[alsa-devel] es1938 - patch trying to improve capture hw pointer reads
Hello,
with the Solo1 (es1938) I got a lot of xrun's during capture on my machine. Tracing that down it seems to be comming from reading ocassionaly bad hw pointers from the chip. Appended is a patch against linux-2.6.23.12 which uses more checking to avoid that false pointer reads.
Failed reads are giving back the last good value read instead of spinning in a tight loop, which seems more appropriate to me in an interrupt. I think I saw this trick used in another driver.
On my machine xruns seems to be gone with that patch.
If anybody is interested, mmaped capture seems also to be doable with some tricks, still testing this...
Greetings
Hermann
At Wed, 23 Jan 2008 01:35:12 +0100, Hermann Lauer wrote:
Hello,
with the Solo1 (es1938) I got a lot of xrun's during capture on my machine. Tracing that down it seems to be comming from reading ocassionaly bad hw pointers from the chip. Appended is a patch against linux-2.6.23.12 which uses more checking to avoid that false pointer reads.
Failed reads are giving back the last good value read instead of spinning in a tight loop, which seems more appropriate to me in an interrupt. I think I saw this trick used in another driver.
On my machine xruns seems to be gone with that patch.
Thanks for the patch. The change look OK to me. Another idea would be to use DMAADDR only when DMACOUNT goes mad. But the back-off to the last pointer is propbably a safer solution.
The patch is, however, unable to apply as is because of coding style problems. Try $LINUXKERNEL/scripts/checkpatch.pl, fix what are suggested there, and repost the patch together with your sign-off.
Takashi
On Thu, Jan 24, 2008 at 12:47:46PM +0100, Takashi Iwai wrote: ...
Thanks for the patch. The change look OK to me. Another idea would be to use DMAADDR only when DMACOUNT goes mad.
The problem is to decide when you have read garbage from DMACOUNT. DMAADDR is better in the first place, because you know the range in 24 bit which is valid (DMACOUNT is 16bit only). But my patch tries to make the failure probability much more lower by reading and comparing both, so from which one you calculate the pointer at the end is only a cosmetic issue.
But the back-off to the last pointer is propbably a safer solution.
I'm glad to hear the alsa design allow's this trick - or is that even the recommeded way to deal with when the hardware pointer could be temporarily not determined ?
The patch is, however, unable to apply as is because of coding style problems. Try $LINUXKERNEL/scripts/checkpatch.pl, fix what are suggested there, and repost the patch together with your sign-off.
Will do that when I digged trough the details (have to learn about signing).
Hermann
At Thu, 24 Jan 2008 16:21:02 +0100, Hermann Lauer wrote:
On Thu, Jan 24, 2008 at 12:47:46PM +0100, Takashi Iwai wrote: ...
Thanks for the patch. The change look OK to me. Another idea would be to use DMAADDR only when DMACOUNT goes mad.
The problem is to decide when you have read garbage from DMACOUNT. DMAADDR is better in the first place, because you know the range in 24 bit which is valid (DMACOUNT is 16bit only). But my patch tries to make the failure probability much more lower by reading and comparing both, so from which one you calculate the pointer at the end is only a cosmetic issue.
Then a question arises - why DMAADDR isn't used as the primary source at all? Is it less stable than DMACOUNT in some cases?
But the back-off to the last pointer is propbably a safer solution.
I'm glad to hear the alsa design allow's this trick - or is that even the recommeded way to deal with when the hardware pointer could be temporarily not determined ?
Well, not really. In theory, it works even if the driver doesn't provide the accurate position. But, the driver must provide the position in the current period, i.e. the accuracy (latency) must be smaller than the period size. Thus, it's more safer to keep tracking the current period index (e.g. incrementing at each period-update irq) and check whether the last ptr is over it.
For example, suppose the buffer = 256 x 4 periods. The last ptr is recorded at 256. Now, at the real position 512, an irq is issued. The irq handler calls snd_pcm_period_elapsed(), which calls the pointer callback in the PCM core. If the read error occurs at this moment, we can't back up to the last position 256. We are supposed to be at position 512 or more.
The patch is, however, unable to apply as is because of coding style problems. Try $LINUXKERNEL/scripts/checkpatch.pl, fix what are suggested there, and repost the patch together with your sign-off.
Will do that when I digged trough the details (have to learn about signing).
See $LINUX/Documentation/SubmittingPatches.
Takashi
On Thu, Jan 24, 2008 at 04:40:18PM +0100, Takashi Iwai wrote:
At Thu, 24 Jan 2008 16:21:02 +0100, Hermann Lauer wrote:
On Thu, Jan 24, 2008 at 12:47:46PM +0100, Takashi Iwai wrote: ...
Thanks for the patch. The change look OK to me. Another idea would be to use DMAADDR only when DMACOUNT goes mad.
The problem is to decide when you have read garbage from DMACOUNT. DMAADDR is better in the first place, because you know the range in 24 bit which is valid (DMACOUNT is 16bit only). But my patch tries to make the failure probability much more lower by reading and comparing both, so from which one you calculate the pointer at the end is only a cosmetic issue.
Then a question arises - why DMAADDR isn't used as the primary source at all? Is it less stable than DMACOUNT in some cases?
I don't know and the current implementation contains only a "don't ask why - AB" comment.
To reiterate: My proposed patch is about minimising the chance that a wrong hw pointer value is given back due to garbage read from the chip. If you are interested, I can send you debugging output which shows that both DMAADDR and DMACOUNT are read out as bogus values from time to time - one or the another, or both. I tried some status flags on the chip, but alway found cases where it's not clear if and which one of the two is correct.
So reading both of them and comparing them seems to be the best way for me to detect that there has been a readout error. Which one to give back is without having more information about the chip internals in my opinion only a matter of taste. The proposed patch allows a jitter of one byte there, but that could be set to zero at the cost of a slightly increased failure rate.
But the back-off to the last pointer is propbably a safer solution.
I'm glad to hear the alsa design allow's this trick - or is that even the recommeded way to deal with when the hardware pointer could be temporarily not determined ?
Well, not really. In theory, it works even if the driver doesn't provide the accurate position. But, the driver must provide the position in the current period, i.e. the accuracy (latency) must be smaller than the period size. Thus, it's more safer to keep tracking the current period index (e.g. incrementing at each period-update irq) and check whether the last ptr is over it.
Fortunately I have not seen any signs of read errors during the interrupt processing. Probably this would change if the interrupt processing latency is high.
For example, suppose the buffer = 256 x 4 periods. The last ptr is recorded at 256. Now, at the real position 512, an irq is issued. The irq handler calls snd_pcm_period_elapsed(), which calls the pointer callback in the PCM core. If the read error occurs at this moment, we can't back up to the last position 256. We are supposed to be at position 512 or more.
Just to make that clear to me: If the irq handler calls snd_pcm_period_elapsed() in your example, the alsa middle layer will treat the first 256 byte as written completely by the chip even if the pointer callback will say there is still one frame not stored for that period. (I see the name of the function answering my qestion, but are curious how the middle layer implementation behaves in this situation)
That can play a role with the handling of the off-by-one bug of the chip.
Hermann
At Fri, 25 Jan 2008 01:25:14 +0100, Hermann Lauer wrote:
On Thu, Jan 24, 2008 at 04:40:18PM +0100, Takashi Iwai wrote:
At Thu, 24 Jan 2008 16:21:02 +0100, Hermann Lauer wrote:
On Thu, Jan 24, 2008 at 12:47:46PM +0100, Takashi Iwai wrote: ...
Thanks for the patch. The change look OK to me. Another idea would be to use DMAADDR only when DMACOUNT goes mad.
The problem is to decide when you have read garbage from DMACOUNT. DMAADDR is better in the first place, because you know the range in 24 bit which is valid (DMACOUNT is 16bit only). But my patch tries to make the failure probability much more lower by reading and comparing both, so from which one you calculate the pointer at the end is only a cosmetic issue.
Then a question arises - why DMAADDR isn't used as the primary source at all? Is it less stable than DMACOUNT in some cases?
I don't know and the current implementation contains only a "don't ask why - AB" comment.
To reiterate: My proposed patch is about minimising the chance that a wrong hw pointer value is given back due to garbage read from the chip. If you are interested, I can send you debugging output which shows that both DMAADDR and DMACOUNT are read out as bogus values from time to time - one or the another, or both. I tried some status flags on the chip, but alway found cases where it's not clear if and which one of the two is correct.
Yeah, it would be helpful to understand the problem more correctly.
So reading both of them and comparing them seems to be the best way for me to detect that there has been a readout error. Which one to give back is without having more information about the chip internals in my opinion only a matter of taste. The proposed patch allows a jitter of one byte there, but that could be set to zero at the cost of a slightly increased failure rate.
BTW, don't get me wrong - I'm not against your proposal. The back-off is a safe option, as already mentioned.
But the back-off to the last pointer is propbably a safer solution.
I'm glad to hear the alsa design allow's this trick - or is that even the recommeded way to deal with when the hardware pointer could be temporarily not determined ?
Well, not really. In theory, it works even if the driver doesn't provide the accurate position. But, the driver must provide the position in the current period, i.e. the accuracy (latency) must be smaller than the period size. Thus, it's more safer to keep tracking the current period index (e.g. incrementing at each period-update irq) and check whether the last ptr is over it.
Fortunately I have not seen any signs of read errors during the interrupt processing. Probably this would change if the interrupt processing latency is high.
Probably. In the other case, if some other device blocks the shared irq line, then the irq handle gets sloppy. In such a case, the sound driver gets an irq for the update of two periods instead of one.
For example, suppose the buffer = 256 x 4 periods. The last ptr is recorded at 256. Now, at the real position 512, an irq is issued. The irq handler calls snd_pcm_period_elapsed(), which calls the pointer callback in the PCM core. If the read error occurs at this moment, we can't back up to the last position 256. We are supposed to be at position 512 or more.
Just to make that clear to me: If the irq handler calls snd_pcm_period_elapsed() in your example, the alsa middle layer will treat the first 256 byte as written completely by the chip even if the pointer callback will say there is still one frame not stored for that period. (I see the name of the function answering my qestion, but are curious how the middle layer implementation behaves in this situation)
Yes. The return value from the pointer callback is the most trusted information the PCM core receives.
That can play a role with the handling of the off-by-one bug of the chip.
What is the problem exactly?
Thanks,
Takashi
On Fri, Jan 25, 2008 at 11:44:12AM +0100, Takashi Iwai wrote:
Then a question arises - why DMAADDR isn't used as the primary source at all? Is it less stable than DMACOUNT in some cases?
I don't know and the current implementation contains only a "don't ask why - AB" comment.
To reiterate: My proposed patch is about minimising the chance that a wrong hw pointer value is given back due to garbage read from the chip. If you are interested, I can send you debugging output which shows that both DMAADDR and DMACOUNT are read out as bogus values from time to time - one or the another, or both. I tried some status flags on the chip, but alway found cases where it's not clear if and which one of the two is correct.
Yeah, it would be helpful to understand the problem more correctly.
Will put together some of the log files next week. Up to what size could/should I cc to the list without upsetting people ? I guess compressing them will yield only a few KBytes.
So reading both of them and comparing them seems to be the best way for me to detect that there has been a readout error. Which one to give back is without having more information about the chip internals in my opinion only a matter of taste. The proposed patch allows a jitter of one byte there, but that could be set to zero at the cost of a slightly increased failure rate.
BTW, don't get me wrong - I'm not against your proposal. The back-off is a safe option, as already mentioned.
Good to know.
Just to make that clear to me: If the irq handler calls snd_pcm_period_elapsed() in your example, the alsa middle layer will treat the first 256 byte as written completely by the chip even if the pointer callback will say there is still one frame not stored for that period. (I see the name of the function answering my qestion, but are curious how the middle layer implementation behaves in this situation)
Yes. The return value from the pointer callback is the most trusted information the PCM core receives.
That can play a role with the handling of the off-by-one bug of the chip.
What is the problem exactly?
If I interpreted my tests output correctly, the last byte of a period is written by the DMA engine after the interrupt occurs. So the hw pointer probably has to be decremented by one and is then pointing to the last frame (Or even that before: Has the hw pointer to point at the frame which is written at the moment or the one which is guaranteed to be written completely by the hardware ?)
Have a nice weekend, Hermann
At Fri, 25 Jan 2008 17:07:27 +0100, Hermann Lauer wrote:
On Fri, Jan 25, 2008 at 11:44:12AM +0100, Takashi Iwai wrote:
Then a question arises - why DMAADDR isn't used as the primary source at all? Is it less stable than DMACOUNT in some cases?
I don't know and the current implementation contains only a "don't ask why - AB" comment.
To reiterate: My proposed patch is about minimising the chance that a wrong hw pointer value is given back due to garbage read from the chip. If you are interested, I can send you debugging output which shows that both DMAADDR and DMACOUNT are read out as bogus values from time to time - one or the another, or both. I tried some status flags on the chip, but alway found cases where it's not clear if and which one of the two is correct.
Yeah, it would be helpful to understand the problem more correctly.
Will put together some of the log files next week. Up to what size could/should I cc to the list without upsetting people ? I guess compressing them will yield only a few KBytes.
Not sure about the size limit, but it'd be better to attach a compressed file. Or just cut out only the interesting part.
That can play a role with the handling of the off-by-one bug of the chip.
What is the problem exactly?
If I interpreted my tests output correctly, the last byte of a period is written by the DMA engine after the interrupt occurs. So the hw pointer probably has to be decremented by one and is then pointing to the last frame (Or even that before: Has the hw pointer to point at the frame which is written at the moment or the one which is guaranteed to be written completely by the hardware ?)
This sounds a bit odd. Isn't it rather the setup of the hardware parameter wrong? I mean, the count calculation in *_prepare() can be the size - 1?
Takashi
On Fri, Jan 25, 2008 at 05:19:52PM +0100, Takashi Iwai wrote:
If I interpreted my tests output correctly, the last byte of a period is written by the DMA engine after the interrupt occurs. So the hw pointer probably has to be decremented by one and is then pointing to the last frame (Or even that before: Has the hw pointer to point at the frame which is written at the moment or the one which is guaranteed to be written completely by the hardware ?)
This sounds a bit odd. Isn't it rather the setup of the hardware parameter wrong? I mean, the count calculation in *_prepare() can be the size - 1?
At least according to the datasheet the size calculation is correct. Could you please tell me where the hw pointer exactly should point to ?
Thanks, Hermann
At Fri, 25 Jan 2008 17:29:28 +0100, Hermann Lauer wrote:
On Fri, Jan 25, 2008 at 05:19:52PM +0100, Takashi Iwai wrote:
If I interpreted my tests output correctly, the last byte of a period is written by the DMA engine after the interrupt occurs. So the hw pointer probably has to be decremented by one and is then pointing to the last frame (Or even that before: Has the hw pointer to point at the frame which is written at the moment or the one which is guaranteed to be written completely by the hardware ?)
This sounds a bit odd. Isn't it rather the setup of the hardware parameter wrong? I mean, the count calculation in *_prepare() can be the size - 1?
At least according to the datasheet the size calculation is correct. Could you please tell me where the hw pointer exactly should point to ?
The hwptr is the position of the current DMA. The data below that point must have been already processed. That's why hwptr returned in snd_pcm_period_elapsed() must be on the period boundary or above.
OTOH, it doesn't matter much whether the data above hwptr has been processed or not, e.g. a sloppy pointer callback.
Takashi
Hello,
appended is an experimental patch agains linux-2.6.23.12 which enables mmaped access during capture on the es1938 (Solo1).
Main obstacle is (as documented in the current driver) that the dma engine put bytes from the adc with an offset +1 to ram. So the patch did the following:
- allocate on additional page in front of the capture dma buffer, with mmaping pointing to the original buffer (i.e. with one page offset into the dma area) - let the dma engine start from the last byte of that extra page, so that all bytes are put aligned to the real dma area (besides the last byte) - the last byte the dma engine of the chip put at the first location, i.e. in the last byte of the extra front dma page. - This byte has to be copied at the right time to the end of the mmaped area, which is done in the hw_pointer callback.
In a discussion with Takashi some of the pointer callback issues are already discussed (that patch is included btw.), and from the log send there (lock at the fb values, which is the fist byte of dma area) you can see that indeed the last byte is transferred after the interrupt indicating reaching the end of the dma buffer came in.
On Fri, Jan 25, 2008 at 05:36:11PM +0100, Takashi Iwai wrote:
The hwptr is the position of the current DMA. The data below that point must have been already processed. That's why hwptr returned in snd_pcm_period_elapsed() must be on the period boundary or above.
The patched driver did not fullfill this, OTOH I have made first recordings which seems to be ok (even with the debug printk in the hw_pointer callback).
Have fun, Hermann
On Fri, Jan 25, 2008 at 05:19:52PM +0100, Takashi Iwai wrote:
At Fri, 25 Jan 2008 17:07:27 +0100, Hermann Lauer wrote:
On Fri, Jan 25, 2008 at 11:44:12AM +0100, Takashi Iwai wrote:
Then a question arises - why DMAADDR isn't used as the primary source at all? Is it less stable than DMACOUNT in some cases?
I don't know and the current implementation contains only a "don't ask why - AB" comment.
To reiterate: My proposed patch is about minimising the chance that a wrong hw pointer value is given back due to garbage read from the chip. If you are interested, I can send you debugging output which shows that both DMAADDR and DMACOUNT are read out as bogus values from time to time - one or the another, or both. I tried some status flags on the chip, but alway found cases where it's not clear if and which one of the two is correct.
Yeah, it would be helpful to understand the problem more correctly.
Will put together some of the log files next week. Up to what size could/should I cc to the list without upsetting people ? I guess compressing them will yield only a few KBytes.
Not sure about the size limit, but it'd be better to attach a compressed file. Or just cut out only the interesting part.
Appended is a logfile part. The interresting parts are the diff vlaues (1 if DMACOUNT and ADDR are read correct), DMA count, addr and probably status. Status is 0xf when the DMA engine resets the DMAADDR to it's starting value, 0 in all other normal cases.
DMAADDR is reread for the debug output from the chip, so it's sometimes not the value used in the diff calculation.
Hermann
On Thu, Jan 24, 2008 at 04:40:18PM +0100, Takashi Iwai wrote:
The patch is, however, unable to apply as is because of coding style problems. Try $LINUXKERNEL/scripts/checkpatch.pl, fix what are suggested there, and repost the patch together with your sign-off.
Will do that when I digged trough the details (have to learn about signing).
See $LINUX/Documentation/SubmittingPatches.
Appended is a cleaned up version of the patch. checkpatch.pl is now only mucking about the '#if 0' commented out old branch. Should this be removed or the if-else parts exchanged, which probably will calm down checkpatch.pl ?
Have to check compilation, then I will send a signed version.
Hermann
At Tue, 29 Jan 2008 09:31:04 +0100, Hermann Lauer wrote:
On Thu, Jan 24, 2008 at 04:40:18PM +0100, Takashi Iwai wrote:
The patch is, however, unable to apply as is because of coding style problems. Try $LINUXKERNEL/scripts/checkpatch.pl, fix what are suggested there, and repost the patch together with your sign-off.
Will do that when I digged trough the details (have to learn about signing).
See $LINUX/Documentation/SubmittingPatches.
Appended is a cleaned up version of the patch. checkpatch.pl is now only mucking about the '#if 0' commented out old branch. Should this be removed or the if-else parts exchanged, which probably will calm down checkpatch.pl ?
That's fine. checkpatch.pl is no strict rule to apply.
Have to check compilation, then I will send a signed version.
Thanks. One missing thing is the initialization of last_capture_dmaaddr. This should be set to zero in prepare callback.
Takashi
On Tue, Jan 29, 2008 at 02:34:25PM +0100, Takashi Iwai wrote:
That's fine. checkpatch.pl is no strict rule to apply.
Have to check compilation, then I will send a signed version.
Thanks. One missing thing is the initialization of last_capture_dmaaddr. This should be set to zero in prepare callback.
Thanks for that hint, fixed in the appended, signed version. After futher cleanups this compiles without warnings.
Hermann
participants (2)
-
Hermann Lauer -
Takashi Iwai