At Wed, 19 Aug 2009 20:02:23 +0200,I wrote:> > At Wed, 19 Aug 2009 12:50:09 -0400,> Jon Smirl wrote:> > > > On Wed, Aug 19, 2009 at 11:33 AM, Takashi Iwai<tiwai@xxxxxxx> wrote:> > > At Wed, 19 Aug 2009 11:19:45 -0400,> > > Jon Smirl wrote:> > >>> > >> On Wed, Aug 19, 2009 at 11:12 AM, Takashi Iwai<tiwai@xxxxxxx> wrote:> > >> > At Wed, 19 Aug 2009 11:02:31 -0400,> > >> > Jon Smirl wrote:> > >> >>> > >> >> On Wed, Aug 19, 2009 at 8:14 AM, Takashi Iwai<tiwai@xxxxxxx> wrote:> > >> >> > At Sat, 15 Aug 2009 23:40:36 -0400,> > >> >> > Jon Smirl wrote:> > >> >> >>> > >> >> >> On Sat, Aug 15, 2009 at 11:53 AM, Jon Smirl<jonsmirl@xxxxxxxxx> wrote:> > >> >> >> > void> > >> >> >> > bfio_synch_stop(void)> > >> >> >> > {> > >> >> >> > int n;> > >> >> >> >> > >> >> >> > if (base_handle == NULL) {> > >> >> >> > return;> > >> >> >> > }> > >> >> >> > FOR_IN_AND_OUT {> > >> >> >> > for (n = 0; n < n_handles[IO]; n++) {> > >> >> >>> > >> >> >> I added:> > >> >> >> snd_pcm_nonblock(handles[IO][n], 0)> > >> >> >> snd_pcm_drain(handles[IO][n])> > >> >> >> snd_pcm_nonblock(handles[IO][n], SND_PCM_NONBLOCK )> > >> >> >>> > >> >> >> > snd_pcm_close(handles[IO][n]);> > >> >> >> > }> > >> >> >> > }> > >> >> >> > }> > >> >> >>> > >> >> >> This is not working correctly.> > >> >> >> snd_pcm_nonblock(handles[IO][n], 0)> > >> >> >> It does not remove O_NONBLOCK for some unknown reason.> > >> >> >>> > >> >> >> I added printf() to snd_pcm_hw_nonblock()> > >> >> >> The fcntl is not getting an error.> > >> >> >> if (fcntl(fd, F_SETFL, flags) < 0) {> > >> >> >> Flags being set are 2 (O_RDWR).> > >> >> >>> > >> >> >> But when I get over to snd_pcm_pre_drain_init(), I get the -EAGAIN error.> > >> >> >> static int snd_pcm_pre_drain_init(struct snd_pcm_substream *substream,> > >> >> >> int state)> > >> >> >> {> > >> >> >> printk("snd_pcm_pre_drain_init\n");> > >> >> >> if (substream->f_flags & O_NONBLOCK)> > >> >> >> return -EAGAIN;> > >> >> >> printk("snd_pcm_pre_drain_init 1\n");> > >> >> >> substream->runtime->trigger_master = substream;> > >> >> >> return 0;> > >> >> >> }> > >> >> >> So I have to conclude that fcntl(fd, F_SETFL, flags) is not removing> > >> >> >> the O_NONBLOCK flag.> > >> >> >> > >> >> > Yeah, you found a long-standing bug :)> > >> >> >> > >> >> > Honestly, I think the current designed behavior is just annoying.> > >> >> > An ioctl may be blocked, thus there is no real merit to return -EAGAIN> > >> >> > with DRAIN ioctl.> > >> >>> > >> >> Brutefir is a server type app, so pulseaudio should be having trouble> > >> >> with this too.> > >> >> > >> > Maybe not. Otherwise we've got already many bug reports.> > >> >> > >> > The difference is how to wait until all data is out. PA would likely> > >> > wait using its own timer stuff without sleeping in drain ioctl.> > >>> > >> Can you implement a polled drain by checking if state is RUNNING and> > >> the looking for the transition to STOPPED?> > >> > > Could you elaborate?> > >> > >> Is there anything special about being in state DRAINING?> > >> > > Yes. The drain needs the following active procedure:> > >> > > 1. app notifies the driver that the stream is drained.> > > 2. app go to sleep (in drain ioctl)> > > 3. the driver marks the PCM state DRAIN> > > 4. when the all data has been sent out, the driver stops the stream,> > > wakes up the sleeper> > > 5. app returns> > >> > > Without the explicit notification, the driver cannot know whether> > > the stream is supposed to be stopped successfully or just get an> > > XRUN.> > >> > > I guess you think that ioctl(DRAIN) just marks and returns, then> > > > That would be a function of being in OF_NONBLOCKED state.> > > > > app does poll() to wait until all data sent out. This would work,> > > too, after some amount of work. But, just fixing the existing DRAIN> > > ioctl is far less work in the end...> > > > Right now drain() is a synchronous API. There is no alternative for> > asynchronously starting a drain and then polling or getting signaled> > when it is finished. If the app is not threaded it will go> > unresponsive while in the drain IOCTL (20 seconds in my case).> > Currently brutefir is not written at a threaded app.> > OK, it sounds like a reasonable argument.> > Maybe it's worth to check how easy it can be implemented.> Basically the same wait_queue like normal poll is used for drain> checks, thus it wouldn't be too difficult to use poll() for user-space> for the same purpose.
The patch is below. It's easier than I thought initially.It seems working fine with my small test case.
A remaining question is whether ioctl(DRAIN) should give -EAGAINin non-blocking mode although it successfully changed the PCM stateto DRAIN. I kept the return value just for compatibility reason,but returning zero appears more reasonable in this case.
Anyway, the patch is in sound-unstable GIT tree right now. I'll moveit to the main sound GIT tree later if no one objects.
thanks,
Takashi
===
>From 4cdc115fd38b54642e8536a5c2389483bcb9b2e9 Mon Sep 17 00:00:00 2001From: Takashi Iwai <tiwai@xxxxxxx>Date: Thu, 20 Aug 2009 16:40:16 +0200Subject: [PATCH] ALSA: pcm - Fix drain behavior in non-blocking mode
The current PCM core has the following problems regarding PCM drainingin non-blocking mode:
- the current f_flags isn't checked in snd_pcm_drain(), thus changing the mode dynamically via snd_pcm_nonblock() after open doesn't work.- calling drain in non-blocking mode just return -EAGAIN error, but doesn't provide any way to sync with draining.
This patch fixes these issues.- check file->f_flags in snd_pcm_drain() properly- when O_NONBLOCK is set, PCM core sets the stream(s) to DRAIN state but quits ioctl immediately without waiting the whole drain; the caller can sync the drain manually via poll()
Signed-off-by: Takashi Iwai <tiwai@xxxxxxx>--- sound/core/pcm_lib.c | 12 +++++++--- sound/core/pcm_native.c | 52 ++++++++++++++++++++++++++-------------------- 2 files changed, 37 insertions(+), 27 deletions(-)
diff --git a/sound/core/pcm_lib.c b/sound/core/pcm_lib.cindex 72cfd47..e3e78c7 100644--- a/sound/core/pcm_lib.c+++ b/sound/core/pcm_lib.c@@ -197,12 +197,16 @@ static int snd_pcm_update_hw_ptr_post(struct snd_pcm_substream *substream, avail = snd_pcm_capture_avail(runtime); if (avail > runtime->avail_max) runtime->avail_max = avail;- if (avail >= runtime->stop_threshold) {- if (substream->runtime->status->state == SNDRV_PCM_STATE_DRAINING)+ if (runtime->status->state == SNDRV_PCM_STATE_DRAINING) {+ if (avail >= runtime->buffer_size) { snd_pcm_drain_done(substream);- else+ return -EPIPE;+ }+ } else {+ if (avail >= runtime->stop_threshold) { xrun(substream);- return -EPIPE;+ return -EPIPE;+ } } if (avail >= runtime->control->avail_min) wake_up(&runtime->sleep);diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.cindex ac2150e..b08898c 100644--- a/sound/core/pcm_native.c+++ b/sound/core/pcm_native.c@@ -1343,8 +1343,6 @@ static int snd_pcm_prepare(struct snd_pcm_substream *substream, static int snd_pcm_pre_drain_init(struct snd_pcm_substream *substream, int state) {- if (substream->f_flags & O_NONBLOCK)- return -EAGAIN; substream->runtime->trigger_master = substream; return 0; }@@ -1392,7 +1390,6 @@ static struct action_ops snd_pcm_action_drain_init = { struct drain_rec { struct snd_pcm_substream *substream; wait_queue_t wait;- snd_pcm_uframes_t stop_threshold; }; static int snd_pcm_drop(struct snd_pcm_substream *substream);@@ -1404,13 +1401,15 @@ static int snd_pcm_drop(struct snd_pcm_substream *substream); * After this call, all streams are supposed to be either SETUP or DRAINING * (capture only) state. */-static int snd_pcm_drain(struct snd_pcm_substream *substream)+static int snd_pcm_drain(struct snd_pcm_substream *substream,+ struct file *file) { struct snd_card *card; struct snd_pcm_runtime *runtime; struct snd_pcm_substream *s; int result = 0; int i, num_drecs;+ int nonblock = 0; struct drain_rec *drec, drec_tmp, *d; card = substream->pcm->card;@@ -1428,6 +1427,15 @@ static int snd_pcm_drain(struct snd_pcm_substream *substream) } } + if (file) {+ if (file->f_flags & O_NONBLOCK)+ nonblock = 1;+ } else if (substream->f_flags & O_NONBLOCK)+ nonblock = 1;++ if (nonblock)+ goto lock; /* no need to allocate waitqueues */+ /* allocate temporary record for drain sync */ down_read(&snd_pcm_link_rwsem); if (snd_pcm_stream_linked(substream)) {@@ -1449,16 +1457,11 @@ static int snd_pcm_drain(struct snd_pcm_substream *substream) d->substream = s; init_waitqueue_entry(&d->wait, current); add_wait_queue(&runtime->sleep, &d->wait);- /* stop_threshold fixup to avoid endless loop when- * stop_threshold > buffer_size- */- d->stop_threshold = runtime->stop_threshold;- if (runtime->stop_threshold > runtime->buffer_size)- runtime->stop_threshold = runtime->buffer_size; } } up_read(&snd_pcm_link_rwsem); + lock: snd_pcm_stream_lock_irq(substream); /* resume pause */ if (substream->runtime->status->state == SNDRV_PCM_STATE_PAUSED)@@ -1466,9 +1469,12 @@ static int snd_pcm_drain(struct snd_pcm_substream *substream) /* pre-start/stop - all running streams are changed to DRAINING state */ result = snd_pcm_action(&snd_pcm_action_drain_init, substream, 0);- if (result < 0) {- snd_pcm_stream_unlock_irq(substream);- goto _error;+ if (result < 0)+ goto unlock;+ /* in non-blocking, we don't wait in ioctl but let caller poll */+ if (nonblock) {+ result = -EAGAIN;+ goto unlock; } for (;;) {@@ -1504,18 +1510,18 @@ static int snd_pcm_drain(struct snd_pcm_substream *substream) } } + unlock: snd_pcm_stream_unlock_irq(substream); - _error:- for (i = 0; i < num_drecs; i++) {- d = &drec[i];- runtime = d->substream->runtime;- remove_wait_queue(&runtime->sleep, &d->wait);- runtime->stop_threshold = d->stop_threshold;+ if (!nonblock) {+ for (i = 0; i < num_drecs; i++) {+ d = &drec[i];+ runtime = d->substream->runtime;+ remove_wait_queue(&runtime->sleep, &d->wait);+ }+ if (drec != &drec_tmp)+ kfree(drec); }-- if (drec != &drec_tmp)- kfree(drec); snd_power_unlock(card); return result;@@ -2544,7 +2550,7 @@ static int snd_pcm_common_ioctl1(struct file *file, return snd_pcm_hw_params_old_user(substream, arg); #endif case SNDRV_PCM_IOCTL_DRAIN:- return snd_pcm_drain(substream);+ return snd_pcm_drain(substream, file); case SNDRV_PCM_IOCTL_DROP: return snd_pcm_drop(substream); case SNDRV_PCM_IOCTL_PAUSE:-- 1.6.3.3
_______________________________________________Alsa-devel mailing listAlsa-devel@xxxxxxxxxxxxxxxxxxxx://mailman.alsa-project.org/mailman/listinfo/alsa-devel
