[alsa-devel] ALSA calling pcm_pointer excessively?
Takashi Iwai
tiwai at suse.de
Fri May 11 16:17:50 CEST 2012
At Fri, 11 May 2012 15:02:37 +0100,
Russell King - ARM Linux wrote:
>
> On Fri, May 11, 2012 at 03:31:08PM +0200, Takashi Iwai wrote:
> > At Thu, 10 May 2012 19:05:56 +0200,
> > Clemens Ladisch wrote:
> > >
> > > Russell King - ARM Linux wrote:
> > > > I think what's happening is that snd_pcm_lib_write1() is looping, and
> > > > each time it updates the hardware position, it finds that it can
> > > > transfer 8 or 16 bytes to the buffer. Once it's transferred that,
> > > > it re-updates the hardware position which has now advanced by another
> > > > 8 or 16 bytes. Repeat, and you find that snd_pcm_lib_write1() spends
> > > > a lot of time inefficiently copying the buffer.
> > > >
> > > > It seems that it will only sleep if the hardware pointer stops making
> > > > progress.
> > >
> > > I'd guess that most existing hardware is fast enough to transfer
> > > samples and has a big enough granularity (typically 32 byte bursts
> > > for PCI) that the pointer doesn't change in consecutive loop
> > > iterations.
> > >
> > > This (untested) patch tries to avoid too many busy looping.
> >
> > Hmm... I still can't follow why such a busy loop happens when
> > avail_min > 1. If avail_min = 1, a busy loop can't be avoided.
> > But if avail_min is set (typically equal with period_size),
> > runtime->twake is either the rest size or the period size, and
> > wait_for_avail() should wait until that sufficiently.
>
> It's exaclty as I explained. Lets take avail_min = 4096, as it is in my
> case.
>
> In snd_pcm_lib_write1(), we loop - here's the simplified version:
>
> while (size > 0) {
> if (runtime->status->state == SNDRV_PCM_STATE_RUNNING)
> snd_pcm_update_hw_ptr(substream);
> avail = snd_pcm_playback_avail(runtime);
> if (!avail) {
> /* Sleep until avail >= avail_min */
> }
> /* Transfer up to max(avail,size) bytes to buffer */
> }
>
> The problem is this. On entry to the loop, say, we calculate that we
> can transfer 4096 bytes into the buffer, and we do that. While we're
> transferring those bytes, the hardware DMA position has advanced.
>
> So, the second time around the loop, avail will be non-zero - let's
> say that the DMA position has advanced 32 bytes. So avail will be
> 32 bytes, and we transfer 32 bytes. Again, while transferring those
> bytes, the DMA position has advanced, but not as far. Let's say 8
> bytes.
>
> So, we repeat the loop, and again, we find avail is non-zero. We
> omit sleeping, instead, dropping through to transfer 8 bytes.
>
> Meanwhile, the DMA position has advanced another 8 bytes.
>
> Repeat, endlessly, wasting lots of CPU cycles transferring very small
> sets of sample data to the buffer.
>
> That happens because of the requirement for the above code to sleep is
> that there has been _no_ advancement of the DMA position while copying
> data to the buffer. Unfortunately, that's not always a realistic
> expectation, especially if you have a high data rate.
>
> At no point in the above scenario does avail_min come into it, until
> we're lucky enough that we copy data into the buffer without the DMA
> position changing. At that point, we're then allowed to sleep. The
> decision to sleep is based purely upon there being _zero_ bytes of
> available buffer space.
Ah, thanks for clearing, now I got it.
Actually the call of snd_pcm_update_hw_ptr() at the loop entry doesn't
have to be done at each time, supposing the process of the loop
(without wait_for_avail()) is fast enough.
How about the patch below?
Takashi
---
diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c
index 4d18941..faedb14 100644
--- a/sound/core/pcm_lib.c
+++ b/sound/core/pcm_lib.c
@@ -1894,6 +1894,7 @@ static snd_pcm_sframes_t snd_pcm_lib_write1(struct snd_pcm_substream *substream,
struct snd_pcm_runtime *runtime = substream->runtime;
snd_pcm_uframes_t xfer = 0;
snd_pcm_uframes_t offset = 0;
+ snd_pcm_uframes_t avail;
int err = 0;
if (size == 0)
@@ -1917,13 +1918,12 @@ static snd_pcm_sframes_t snd_pcm_lib_write1(struct snd_pcm_substream *substream,
}
runtime->twake = runtime->control->avail_min ? : 1;
+ if (runtime->status->state == SNDRV_PCM_STATE_RUNNING)
+ snd_pcm_update_hw_ptr(substream);
+ avail = snd_pcm_playback_avail(runtime);
while (size > 0) {
snd_pcm_uframes_t frames, appl_ptr, appl_ofs;
- snd_pcm_uframes_t avail;
snd_pcm_uframes_t cont;
- if (runtime->status->state == SNDRV_PCM_STATE_RUNNING)
- snd_pcm_update_hw_ptr(substream);
- avail = snd_pcm_playback_avail(runtime);
if (!avail) {
if (nonblock) {
err = -EAGAIN;
@@ -1971,6 +1971,7 @@ static snd_pcm_sframes_t snd_pcm_lib_write1(struct snd_pcm_substream *substream,
offset += frames;
size -= frames;
xfer += frames;
+ avail -= frames;
if (runtime->status->state == SNDRV_PCM_STATE_PREPARED &&
snd_pcm_playback_hw_avail(runtime) >= (snd_pcm_sframes_t)runtime->start_threshold) {
err = snd_pcm_start(substream);
@@ -2111,6 +2112,7 @@ static snd_pcm_sframes_t snd_pcm_lib_read1(struct snd_pcm_substream *substream,
struct snd_pcm_runtime *runtime = substream->runtime;
snd_pcm_uframes_t xfer = 0;
snd_pcm_uframes_t offset = 0;
+ snd_pcm_uframes_t avail;
int err = 0;
if (size == 0)
@@ -2141,13 +2143,12 @@ static snd_pcm_sframes_t snd_pcm_lib_read1(struct snd_pcm_substream *substream,
}
runtime->twake = runtime->control->avail_min ? : 1;
+ if (runtime->status->state == SNDRV_PCM_STATE_RUNNING)
+ snd_pcm_update_hw_ptr(substream);
+ avail = snd_pcm_capture_avail(runtime);
while (size > 0) {
snd_pcm_uframes_t frames, appl_ptr, appl_ofs;
- snd_pcm_uframes_t avail;
snd_pcm_uframes_t cont;
- if (runtime->status->state == SNDRV_PCM_STATE_RUNNING)
- snd_pcm_update_hw_ptr(substream);
- avail = snd_pcm_capture_avail(runtime);
if (!avail) {
if (runtime->status->state ==
SNDRV_PCM_STATE_DRAINING) {
@@ -2202,6 +2203,7 @@ static snd_pcm_sframes_t snd_pcm_lib_read1(struct snd_pcm_substream *substream,
offset += frames;
size -= frames;
xfer += frames;
+ avail -= frames;
}
_end_unlock:
runtime->twake = 0;
More information about the Alsa-devel
mailing list