[alsa-devel] [PATCH 1/2] ALSA: pcm - Ensure wakeup at each periodic interrupt
From: Jassi Brar jassi.brar@samsung.com
The check for at least 'avail_min' available data before calling wake_up doesn't always hold good as it does not guarantee callbacks at each periodic interrupt.
An example of such situation is snd_pcm_lib_read1/write1 consuming some space of the period and going to sleep from wait_for_avail_min upon syncing with the DMA pointer. Clearly just the remainder of period size is needed, but wake_up is called only after _two_ periodic interrupts from that point.
Signed-off-by: Jassi Brar jassi.brar@samsung.com --- sound/core/pcm_lib.c | 10 ++++++++-- 1 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index 30f4108..ee2d7b9 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -189,7 +189,7 @@ snd_pcm_update_hw_ptr_pos(struct snd_pcm_substream *substream, static int snd_pcm_update_hw_ptr_post(struct snd_pcm_substream *substream, struct snd_pcm_runtime *runtime) { - snd_pcm_uframes_t avail; + snd_pcm_uframes_t avail, prd_short;
if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) avail = snd_pcm_playback_avail(runtime); @@ -208,8 +208,14 @@ static int snd_pcm_update_hw_ptr_post(struct snd_pcm_substream *substream, return -EPIPE; } } - if (avail >= runtime->control->avail_min) + + prd_short = runtime->period_size + - runtime->status->hw_ptr % runtime->period_size; + + if (avail >= runtime->control->avail_min + || avail >= prd_short) wake_up(&runtime->sleep); + return 0; }
At Thu, 17 Dec 2009 15:00:02 +0900, jassisinghbrar@gmail.com wrote:
From: Jassi Brar jassi.brar@samsung.com
The check for at least 'avail_min' available data before calling wake_up doesn't always hold good as it does not guarantee callbacks at each periodic interrupt.
Well, avail_min can be greater than period_size. And, avail_min won't be less than period size.
For example, when avail_min = 2.5 x period_size, the driver wakes up in periods like 3, 2, 3, 2, ...
An example of such situation is snd_pcm_lib_read1/write1 consuming some space of the period and going to sleep from wait_for_avail_min upon syncing with the DMA pointer. Clearly just the remainder of period size is needed, but wake_up is called only after _two_ periodic interrupts from that point.
In that case, the original behavior is correct.
thanks,
Takashi
Signed-off-by: Jassi Brar jassi.brar@samsung.com
sound/core/pcm_lib.c | 10 ++++++++-- 1 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index 30f4108..ee2d7b9 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -189,7 +189,7 @@ snd_pcm_update_hw_ptr_pos(struct snd_pcm_substream *substream, static int snd_pcm_update_hw_ptr_post(struct snd_pcm_substream *substream, struct snd_pcm_runtime *runtime) {
- snd_pcm_uframes_t avail;
snd_pcm_uframes_t avail, prd_short;
if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) avail = snd_pcm_playback_avail(runtime);
@@ -208,8 +208,14 @@ static int snd_pcm_update_hw_ptr_post(struct snd_pcm_substream *substream, return -EPIPE; } }
- if (avail >= runtime->control->avail_min)
- prd_short = runtime->period_size
- runtime->status->hw_ptr % runtime->period_size;- if (avail >= runtime->control->avail_min
|| avail >= prd_short)- return 0;
}
-- 1.6.2.5
On Thu, Dec 17, 2009 at 4:02 PM, Takashi Iwai tiwai@suse.de wrote:
At Thu, 17 Dec 2009 15:00:02 +0900, jassisinghbrar@gmail.com wrote:
From: Jassi Brar jassi.brar@samsung.com
The check for at least 'avail_min' available data before calling wake_up doesn't always hold good as it does not guarantee callbacks at each periodic interrupt.
Well, avail_min can be greater than period_size. And, avail_min won't be less than period size.
For example, when avail_min = 2.5 x period_size, the driver wakes up in periods like 3, 2, 3, 2, ...
correct, but if we ensure wake_up's after each period and let the 'sleepers' track if the data available is enough or not, we will have more fine grained control. The point is:- Waking up _after_ avail_min is working, but does waking up before avail_min(but at period boundary) break the system?
An example of such situation is snd_pcm_lib_read1/write1 consuming some space of the period and going to sleep from wait_for_avail_min upon syncing with the DMA pointer. Clearly just the remainder of period size is needed, but wake_up is called only after _two_ periodic interrupts from that point.
In that case, the original behavior is correct.
going by current implementation, that is correct, but is that desirable?
thanks.
thanks,
Takashi
Signed-off-by: Jassi Brar jassi.brar@samsung.com
sound/core/pcm_lib.c | 10 ++++++++-- 1 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index 30f4108..ee2d7b9 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -189,7 +189,7 @@ snd_pcm_update_hw_ptr_pos(struct snd_pcm_substream *substream, static int snd_pcm_update_hw_ptr_post(struct snd_pcm_substream *substream, struct snd_pcm_runtime *runtime) {
- snd_pcm_uframes_t avail;
- snd_pcm_uframes_t avail, prd_short;
if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) avail = snd_pcm_playback_avail(runtime); @@ -208,8 +208,14 @@ static int snd_pcm_update_hw_ptr_post(struct snd_pcm_substream *substream, return -EPIPE; } }
- if (avail >= runtime->control->avail_min)
- prd_short = runtime->period_size
- - runtime->status->hw_ptr % runtime->period_size;
- if (avail >= runtime->control->avail_min
- || avail >= prd_short)
wake_up(&runtime->sleep);
return 0; }
-- 1.6.2.5
At Thu, 17 Dec 2009 16:31:08 +0900, jassi brar wrote:
On Thu, Dec 17, 2009 at 4:02 PM, Takashi Iwai tiwai@suse.de wrote:
At Thu, 17 Dec 2009 15:00:02 +0900, jassisinghbrar@gmail.com wrote:
From: Jassi Brar jassi.brar@samsung.com
The check for at least 'avail_min' available data before calling wake_up doesn't always hold good as it does not guarantee callbacks at each periodic interrupt.
Well, avail_min can be greater than period_size. And, avail_min won't be less than period size.
For example, when avail_min = 2.5 x period_size, the driver wakes up in periods like 3, 2, 3, 2, ...
correct, but if we ensure wake_up's after each period and let the 'sleepers' track if the data available is enough or not, we will have more fine grained control. The point is:- Waking up _after_ avail_min is working, but does waking up before avail_min(but at period boundary) break the system?
PulseAudio may complain :)
An example of such situation is snd_pcm_lib_read1/write1 consuming some space of the period and going to sleep from wait_for_avail_min upon syncing with the DMA pointer. Clearly just the remainder of period size is needed, but wake_up is called only after _two_ periodic interrupts from that point.
In that case, the original behavior is correct.
going by current implementation, that is correct, but is that desirable?
Correct = working as designed.
Takashi
On Thu, Dec 17, 2009 at 4:43 PM, Takashi Iwai tiwai@suse.de wrote:
At Thu, 17 Dec 2009 16:31:08 +0900, jassi brar wrote:
On Thu, Dec 17, 2009 at 4:02 PM, Takashi Iwai tiwai@suse.de wrote:
At Thu, 17 Dec 2009 15:00:02 +0900, jassisinghbrar@gmail.com wrote:
From: Jassi Brar jassi.brar@samsung.com
The check for at least 'avail_min' available data before calling wake_up doesn't always hold good as it does not guarantee callbacks at each periodic interrupt.
Well, avail_min can be greater than period_size. And, avail_min won't be less than period size.
For example, when avail_min = 2.5 x period_size, the driver wakes up in periods like 3, 2, 3, 2, ...
correct, but if we ensure wake_up's after each period and let the 'sleepers' track if the data available is enough or not, we will have more fine grained control. The point is:- Waking up _after_ avail_min is working, but does waking up before avail_min(but at period boundary) break the system?
PulseAudio may complain :)
I meant effects on ALSA state-machine within the kernel.
At Thu, 17 Dec 2009 17:21:28 +0900, jassi brar wrote:
On Thu, Dec 17, 2009 at 4:43 PM, Takashi Iwai tiwai@suse.de wrote:
At Thu, 17 Dec 2009 16:31:08 +0900, jassi brar wrote:
On Thu, Dec 17, 2009 at 4:02 PM, Takashi Iwai tiwai@suse.de wrote:
At Thu, 17 Dec 2009 15:00:02 +0900, jassisinghbrar@gmail.com wrote:
From: Jassi Brar jassi.brar@samsung.com
The check for at least 'avail_min' available data before calling wake_up doesn't always hold good as it does not guarantee callbacks at each periodic interrupt.
Well, avail_min can be greater than period_size. And, avail_min won't be less than period size.
For example, when avail_min = 2.5 x period_size, the driver wakes up in periods like 3, 2, 3, 2, ...
correct, but if we ensure wake_up's after each period and let the 'sleepers' track if the data available is enough or not, we will have more fine grained control. The point is:- Waking up _after_ avail_min is working, but does waking up before avail_min(but at period boundary) break the system?
PulseAudio may complain :)
I meant effects on ALSA state-machine within the kernel.
From what i have seen, every use of sleep is just to kill some time,
i.e, wake_up is not taken as indication of completion of the purpose.
It's used also for poll.
Meanwhile, we may change snd_pcm_*_poll() function itself, too...
Takashi
On Mon, Dec 21, 2009 at 8:28 PM, Takashi Iwai tiwai@suse.de wrote:
At Thu, 17 Dec 2009 17:21:28 +0900, jassi brar wrote:
On Thu, Dec 17, 2009 at 4:43 PM, Takashi Iwai tiwai@suse.de wrote:
At Thu, 17 Dec 2009 16:31:08 +0900, jassi brar wrote:
On Thu, Dec 17, 2009 at 4:02 PM, Takashi Iwai tiwai@suse.de wrote:
At Thu, 17 Dec 2009 15:00:02 +0900, jassisinghbrar@gmail.com wrote:
From: Jassi Brar jassi.brar@samsung.com
The check for at least 'avail_min' available data before calling wake_up doesn't always hold good as it does not guarantee callbacks at each periodic interrupt.
Well, avail_min can be greater than period_size. And, avail_min won't be less than period size.
For example, when avail_min = 2.5 x period_size, the driver wakes up in periods like 3, 2, 3, 2, ...
correct, but if we ensure wake_up's after each period and let the 'sleepers' track if the data available is enough or not, we will have more fine grained control. The point is:- Waking up _after_ avail_min is working, but does waking up before avail_min(but at period boundary) break the system?
PulseAudio may complain :)
I meant effects on ALSA state-machine within the kernel.
From what i have seen, every use of sleep is just to kill some time,
i.e, wake_up is not taken as indication of completion of the purpose.
It's used also for poll.
Meanwhile, we may change snd_pcm_*_poll() function itself, too...
How about employing a new snd_pcm_runtime member (say, need_min) which denote the _exact_ minimum amount of data needed to serve purpose of last sleep ?
Quick idea of what i mean, follows. If you like it, i will resend the patches again Thanks.
diff --git a/include/sound/pcm.h b/include/sound/pcm.h index de6d981..b5a9e81 100644 --- a/include/sound/pcm.h +++ b/include/sound/pcm.h @@ -267,6 +267,7 @@ struct snd_pcm_runtime { struct snd_pcm_substream *trigger_master; struct timespec trigger_tstamp; /* trigger timestamp */ int overrange; + snd_pcm_uframes_t need_min; /* exact minimum needed before wakeup */ snd_pcm_uframes_t avail_max; snd_pcm_uframes_t hw_ptr_base; /* Position at buffer restart */ snd_pcm_uframes_t hw_ptr_interrupt; /* Position at interrupt time */ diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.c index 30f4108..a689add 100644 --- a/sound/core/pcm_lib.c +++ b/sound/core/pcm_lib.c @@ -208,7 +208,7 @@ static int snd_pcm_update_hw_ptr_post(struct snd_pcm_substream *substream, return -EPIPE; } } - if (avail >= runtime->control->avail_min) + if (avail >= runtime->need_min) wake_up(&runtime->sleep); return 0; } @@ -1681,6 +1681,7 @@ static int wait_for_avail_min(struct snd_pcm_substream *substream, break; } set_current_state(TASK_INTERRUPTIBLE); + runtime->need_min = *availp; snd_pcm_stream_unlock_irq(substream); tout = schedule_timeout(msecs_to_jiffies(10000)); snd_pcm_stream_lock_irq(substream); @@ -1788,6 +1789,7 @@ static snd_pcm_sframes_t snd_pcm_lib_write1(struct snd_pcm_substream *substream, err = -EAGAIN; goto _end_unlock; } + avail = size; err = wait_for_avail_min(substream, &avail); if (err < 0) goto _end_unlock; @@ -2010,6 +2012,7 @@ static snd_pcm_sframes_t snd_pcm_lib_read1(struct snd_pcm_substream *substream, err = -EAGAIN; goto _end_unlock; } + avail = size; err = wait_for_avail_min(substream, &avail); if (err < 0) goto _end_unlock; diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index ab73edf..6e5dfa2 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -2892,6 +2892,10 @@ static unsigned int snd_pcm_playback_poll(struct file *file, poll_table * wait) return -ENXIO; runtime = substream->runtime;
+ snd_pcm_stream_lock_irq(substream); + runtime->need_min = runtime->control->avail_min; + snd_pcm_stream_unlock_irq(substream); + poll_wait(file, &runtime->sleep, wait);
snd_pcm_stream_lock_irq(substream); @@ -2931,6 +2935,10 @@ static unsigned int snd_pcm_capture_poll(struct file *file, poll_table * wait) return -ENXIO; runtime = substream->runtime;
+ snd_pcm_stream_lock_irq(substream); + runtime->need_min = runtime->control->avail_min; + snd_pcm_stream_unlock_irq(substream); + poll_wait(file, &runtime->sleep, wait);
snd_pcm_stream_lock_irq(substream);
At Tue, 22 Dec 2009 16:34:32 +0900, jassi brar wrote:
On Mon, Dec 21, 2009 at 8:28 PM, Takashi Iwai tiwai@suse.de wrote:
At Thu, 17 Dec 2009 17:21:28 +0900, jassi brar wrote:
On Thu, Dec 17, 2009 at 4:43 PM, Takashi Iwai tiwai@suse.de wrote:
At Thu, 17 Dec 2009 16:31:08 +0900, jassi brar wrote:
On Thu, Dec 17, 2009 at 4:02 PM, Takashi Iwai tiwai@suse.de wrote:
At Thu, 17 Dec 2009 15:00:02 +0900, jassisinghbrar@gmail.com wrote: > > From: Jassi Brar jassi.brar@samsung.com > > The check for at least 'avail_min' available data before calling wake_up > doesn't always hold good as it does not guarantee callbacks at each periodic > interrupt.
Well, avail_min can be greater than period_size. And, avail_min won't be less than period size.
For example, when avail_min = 2.5 x period_size, the driver wakes up in periods like 3, 2, 3, 2, ...
correct, but if we ensure wake_up's after each period and let the 'sleepers' track if the data available is enough or not, we will have more fine grained control. The point is:- Waking up _after_ avail_min is working, but does waking up before avail_min(but at period boundary) break the system?
PulseAudio may complain :)
I meant effects on ALSA state-machine within the kernel.
From what i have seen, every use of sleep is just to kill some time,
i.e, wake_up is not taken as indication of completion of the purpose.
It's used also for poll.
Meanwhile, we may change snd_pcm_*_poll() function itself, too...
How about employing a new snd_pcm_runtime member (say, need_min) which denote the _exact_ minimum amount of data needed to serve purpose of last sleep ?
Quick idea of what i mean, follows. If you like it, i will resend the patches again Thanks.
Well, I don't know. This clearly changes the semantics of avail_min in the write case. That is, this leads to slight incompatibilities. Whether such a difference is acceptable on all possible situations is another question...
Takashi
On Tue, Dec 22, 2009 at 4:50 PM, Takashi Iwai tiwai@suse.de wrote:
At Tue, 22 Dec 2009 16:34:32 +0900, jassi brar wrote:
On Mon, Dec 21, 2009 at 8:28 PM, Takashi Iwai tiwai@suse.de wrote:
At Thu, 17 Dec 2009 17:21:28 +0900, jassi brar wrote:
On Thu, Dec 17, 2009 at 4:43 PM, Takashi Iwai tiwai@suse.de wrote:
At Thu, 17 Dec 2009 16:31:08 +0900, jassi brar wrote:
On Thu, Dec 17, 2009 at 4:02 PM, Takashi Iwai tiwai@suse.de wrote: > At Thu, 17 Dec 2009 15:00:02 +0900, > jassisinghbrar@gmail.com wrote: >> >> From: Jassi Brar jassi.brar@samsung.com >> >> The check for at least 'avail_min' available data before calling wake_up >> doesn't always hold good as it does not guarantee callbacks at each periodic >> interrupt. > > Well, avail_min can be greater than period_size. And, avail_min won't be > less than period size. > > For example, when avail_min = 2.5 x period_size, the driver wakes up > in periods like 3, 2, 3, 2, ... correct, but if we ensure wake_up's after each period and let the 'sleepers' track if the data available is enough or not, we will have more fine grained control. The point is:- Waking up _after_ avail_min is working, but does waking up before avail_min(but at period boundary) break the system?
PulseAudio may complain :)
I meant effects on ALSA state-machine within the kernel.
From what i have seen, every use of sleep is just to kill some time,
i.e, wake_up is not taken as indication of completion of the purpose.
It's used also for poll.
Meanwhile, we may change snd_pcm_*_poll() function itself, too...
How about employing a new snd_pcm_runtime member (say, need_min) which denote the _exact_ minimum amount of data needed to serve purpose of last sleep ?
Quick idea of what i mean, follows. If you like it, i will resend the patches again Thanks.
Well, I don't know. This clearly changes the semantics of avail_min in the write case. That is, this leads to slight incompatibilities. Whether such a difference is acceptable on all possible situations is another question...
I think, only applications, that don't call snd_pcm_wait() before blocking writes _and_ count upon the blocking times of such writes, may be affected. Clearly not a serious crime against such apps.
Regards.
participants (3)
-
jassi brar -
jassisinghbrar＠gmail.com
-
Takashi Iwai