[alsa-devel] SNDRV_PCM_TRIGGER_STOP and audio still queued in the driver
In the mpc5200 driver I'm receiving SNDRV_PCM_TRIGGER_STOP with audio still queued. I noticed this in an app that uses a 2MB ALSA buffer. It is filling the 2MB buffer which starts playing. After the buffer is filled I receive the STOP. This STOP comes before the hardware has a chance to play all of the music. There is still about twenty seconds of music in the driver buffer waiting to be played.
How is this supposed to work?
Source to the ALSA section of the app doing this (brutefir).
http://brutefir.sourcearchive.com/documentation/1.0f-1ubuntu0/bfio__alsa_8c-...
The stop routine just closes the handles. Is this the correct behavior? There is over 10s of music queued in the driver when the handle is closed.
void bfio_synch_stop(void) { int n;
if (base_handle == NULL) { return; } FOR_IN_AND_OUT { for (n = 0; n < n_handles[IO]; n++) { snd_pcm_close(handles[IO][n]); } } }
On Sat, Aug 15, 2009 at 11:53 AM, Jon Smirljonsmirl@gmail.com 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.
If I change the inital PCM open to be blocking everything works as expected.
At Sat, 15 Aug 2009 23:40:36 -0400, Jon Smirl wrote:
On Sat, Aug 15, 2009 at 11:53 AM, Jon Smirljonsmirl@gmail.com 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.
So, my preferred solution is simply to remove the O_NONBLOCK check in the code path above.
Another solution would be a patch like below (totally untested)...
Comments?
thanks,
Takashi
--- diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index d89c816..5d6b9ad 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; } @@ -1404,7 +1402,8 @@ 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; @@ -1412,6 +1411,14 @@ static int snd_pcm_drain(struct snd_pcm_substream *substream) int result = 0; int i, num_drecs; struct drain_rec *drec, drec_tmp, *d; + int f_flags; + + if (file) + f_flags = file->f_flags; + else + f_flags = substream->f_flags; + if (f_flags & O_NONBLOCK) + return -EAGAIN;
card = substream->pcm->card; runtime = substream->runtime; @@ -2556,7 +2563,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:
On Wed, Aug 19, 2009 at 8:14 AM, Takashi Iwaitiwai@suse.de wrote:
At Sat, 15 Aug 2009 23:40:36 -0400, Jon Smirl wrote:
On Sat, Aug 15, 2009 at 11:53 AM, Jon Smirljonsmirl@gmail.com 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.
Brutefir is processing audio asynchronously while monitoring for network traffic and doing background FIR computations. The FIR operation is heavily pipelined so it is using a 2MB ALSA buffer.
So what happens when the source stream ends? Brutefir wants to close (or mute) the device when the stream is finished. What brutefir wants to do is sit in a loop calling drain() until it stops returning EAGAIN. That way it can keep monitoring the network for incoming traffic. When drain stops returning EAGAIN it is safe to close the device. An alternative solution is to use a thread with a blocking drain(). But brutefir is not currently threaded.
The current brutefir code just ignores the drain state and chops off the last 2MB of the stream. That's probably because streams don't end very often.
I noticed this because I was sending test files one at a time to brutefir instead of streaming. My test files would play a couple of seconds and then get chopped off.
So what does pulse do when it runs out of input data? Does it feed the device zeros or close/mute it?
So, my preferred solution is simply to remove the O_NONBLOCK check in the code path above.
Another solution would be a patch like below (totally untested)...
Comments?
thanks,
Takashi
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index d89c816..5d6b9ad 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; } @@ -1404,7 +1402,8 @@ 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; @@ -1412,6 +1411,14 @@ static int snd_pcm_drain(struct snd_pcm_substream *substream) int result = 0; int i, num_drecs; struct drain_rec *drec, drec_tmp, *d;
- int f_flags;
- if (file)
- f_flags = file->f_flags;
- else
- f_flags = substream->f_flags;
- if (f_flags & O_NONBLOCK)
- return -EAGAIN;
card = substream->pcm->card; runtime = substream->runtime; @@ -2556,7 +2563,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:
At Wed, 19 Aug 2009 11:02:31 -0400, Jon Smirl wrote:
On Wed, Aug 19, 2009 at 8:14 AM, Takashi Iwaitiwai@suse.de wrote:
At Sat, 15 Aug 2009 23:40:36 -0400, Jon Smirl wrote:
On Sat, Aug 15, 2009 at 11:53 AM, Jon Smirljonsmirl@gmail.com 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.
Takashi
On Wed, Aug 19, 2009 at 11:12 AM, Takashi Iwaitiwai@suse.de wrote:
At Wed, 19 Aug 2009 11:02:31 -0400, Jon Smirl wrote:
On Wed, Aug 19, 2009 at 8:14 AM, Takashi Iwaitiwai@suse.de wrote:
At Sat, 15 Aug 2009 23:40:36 -0400, Jon Smirl wrote:
On Sat, Aug 15, 2009 at 11:53 AM, Jon Smirljonsmirl@gmail.com 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?
Is there anything special about being in state DRAINING?
Takashi
At Wed, 19 Aug 2009 11:19:45 -0400, Jon Smirl wrote:
On Wed, Aug 19, 2009 at 11:12 AM, Takashi Iwaitiwai@suse.de wrote:
At Wed, 19 Aug 2009 11:02:31 -0400, Jon Smirl wrote:
On Wed, Aug 19, 2009 at 8:14 AM, Takashi Iwaitiwai@suse.de wrote:
At Sat, 15 Aug 2009 23:40:36 -0400, Jon Smirl wrote:
On Sat, Aug 15, 2009 at 11:53 AM, Jon Smirljonsmirl@gmail.com 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 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...
Takashi
On Wed, Aug 19, 2009 at 11:33 AM, Takashi Iwaitiwai@suse.de wrote:
At Wed, 19 Aug 2009 11:19:45 -0400, Jon Smirl wrote:
On Wed, Aug 19, 2009 at 11:12 AM, Takashi Iwaitiwai@suse.de wrote:
At Wed, 19 Aug 2009 11:02:31 -0400, Jon Smirl wrote:
On Wed, Aug 19, 2009 at 8:14 AM, Takashi Iwaitiwai@suse.de wrote:
At Sat, 15 Aug 2009 23:40:36 -0400, Jon Smirl wrote:
On Sat, Aug 15, 2009 at 11:53 AM, Jon Smirljonsmirl@gmail.com 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:
- app notifies the driver that the stream is drained.
- app go to sleep (in drain ioctl)
- the driver marks the PCM state DRAIN
- 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.
I haven't started working with the user space API yet (I'm just trying to fix brutefir). For server type apps it would be nice to integrate ALSA into the poll/select process. Is that something that can be done?
At Wed, 19 Aug 2009 12:50:09 -0400, Jon Smirl wrote:
On Wed, Aug 19, 2009 at 11:33 AM, Takashi Iwaitiwai@suse.de wrote:
At Wed, 19 Aug 2009 11:19:45 -0400, Jon Smirl wrote:
On Wed, Aug 19, 2009 at 11:12 AM, Takashi Iwaitiwai@suse.de wrote:
At Wed, 19 Aug 2009 11:02:31 -0400, Jon Smirl wrote:
On Wed, Aug 19, 2009 at 8:14 AM, Takashi Iwaitiwai@suse.de wrote:
At Sat, 15 Aug 2009 23:40:36 -0400, Jon Smirl wrote: > > On Sat, Aug 15, 2009 at 11:53 AM, Jon Smirljonsmirl@gmail.com 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:
- app notifies the driver that the stream is drained.
- app go to sleep (in drain ioctl)
- the driver marks the PCM state DRAIN
- 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.
But, a possible problem is the case of linked PCM streams. This can give far more pains than gains...
thanks,
Takashi
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 Iwaitiwai@suse.de wrote:
At Wed, 19 Aug 2009 11:19:45 -0400, Jon Smirl wrote:
On Wed, Aug 19, 2009 at 11:12 AM, Takashi Iwaitiwai@suse.de wrote:
At Wed, 19 Aug 2009 11:02:31 -0400, Jon Smirl wrote:
On Wed, Aug 19, 2009 at 8:14 AM, Takashi Iwaitiwai@suse.de wrote: > At Sat, 15 Aug 2009 23:40:36 -0400, > Jon Smirl wrote: >> >> On Sat, Aug 15, 2009 at 11:53 AM, Jon Smirljonsmirl@gmail.com 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:
- app notifies the driver that the stream is drained.
- app go to sleep (in drain ioctl)
- the driver marks the PCM state DRAIN
- 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 -EAGAIN in non-blocking mode although it successfully changed the PCM state to 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 move it to the main sound GIT tree later if no one objects.
thanks,
Takashi
===
At Thu, 20 Aug 2009 16:52:35 +0200, I wrote:
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 Iwaitiwai@suse.de wrote:
At Wed, 19 Aug 2009 11:19:45 -0400, Jon Smirl wrote:
On Wed, Aug 19, 2009 at 11:12 AM, Takashi Iwaitiwai@suse.de wrote:
At Wed, 19 Aug 2009 11:02:31 -0400, Jon Smirl wrote: > > On Wed, Aug 19, 2009 at 8:14 AM, Takashi Iwaitiwai@suse.de wrote: > > At Sat, 15 Aug 2009 23:40:36 -0400, > > Jon Smirl wrote: > >> > >> On Sat, Aug 15, 2009 at 11:53 AM, Jon Smirljonsmirl@gmail.com 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:
- app notifies the driver that the stream is drained.
- app go to sleep (in drain ioctl)
- the driver marks the PCM state DRAIN
- 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 -EAGAIN in non-blocking mode although it successfully changed the PCM state to 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 move it to the main sound GIT tree later if no one objects.
The patch is merged into sound git tree now.
Takashi
participants (2)
-
Jon Smirl -
Takashi Iwai