SNDRV_PCM_TRIGGER_STOP and audio still queued in the driver

SNDRV_PCM_TRIGGER_STOP and audio still queued in the driver

[Jon Smirl]

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?

-- 
Jon Smirl
jonsmirl@gmail.com

Re: SNDRV_PCM_TRIGGER_STOP and audio still queued in the driver

[Jon Smirl]

Source to the ALSA section of the app doing this (brutefir).

http://brutefir.sourcearchive.com/documentation/1.0f-1ubuntu0/bfio__alsa_8c-source.html

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]);
        }
    }
}


-- 
Jon Smirl
jonsmirl@gmail.com

Re: SNDRV_PCM_TRIGGER_STOP and audio still queued in the driver

[Jon Smirl]

On Sat, Aug 15, 2009 at 11:53 AM, Jon Smirl<jonsmirl@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.




-- 
Jon Smirl
jonsmirl@gmail.com

Re: SNDRV_PCM_TRIGGER_STOP and audio still queued in the driver

[Takashi Iwai]

At Sat, 15 Aug 2009 23:40:36 -0400,
Jon Smirl wrote:
> 
> On Sat, Aug 15, 2009 at 11:53 AM, Jon Smirl<jonsmirl@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:
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: SNDRV_PCM_TRIGGER_STOP and audio still queued in the driver

[Jon Smirl]

On Wed, Aug 19, 2009 at 8:14 AM, Takashi Iwai<tiwai@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 Smirl<jonsmirl@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:
>



-- 
Jon Smirl
jonsmirl@gmail.com

Re: SNDRV_PCM_TRIGGER_STOP and audio still queued in the driver

[Takashi Iwai]

At Wed, 19 Aug 2009 11:02:31 -0400,
Jon Smirl wrote:
> 
> On Wed, Aug 19, 2009 at 8:14 AM, Takashi Iwai<tiwai@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 Smirl<jonsmirl@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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: SNDRV_PCM_TRIGGER_STOP and audio still queued in the driver

[Jon Smirl]

On Wed, Aug 19, 2009 at 11:12 AM, Takashi Iwai<tiwai@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 Iwai<tiwai@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 Smirl<jonsmirl@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
>



-- 
Jon Smirl
jonsmirl@gmail.com

Re: SNDRV_PCM_TRIGGER_STOP and audio still queued in the driver

[Takashi Iwai]

At Wed, 19 Aug 2009 11:19:45 -0400,
Jon Smirl wrote:
> 
> On Wed, Aug 19, 2009 at 11:12 AM, Takashi Iwai<tiwai@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 Iwai<tiwai@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 Smirl<jonsmirl@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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: SNDRV_PCM_TRIGGER_STOP and audio still queued in the driver

[Jon Smirl]

On Wed, Aug 19, 2009 at 11:33 AM, Takashi Iwai<tiwai@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 Iwai<tiwai@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 Iwai<tiwai@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 Smirl<jonsmirl@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

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?

-- 
Jon Smirl
jonsmirl@gmail.com

Re: SNDRV_PCM_TRIGGER_STOP and audio still queued in the driver

[Takashi Iwai]

At Wed, 19 Aug 2009 12:50:09 -0400,
Jon Smirl wrote:
> 
> On Wed, Aug 19, 2009 at 11:33 AM, Takashi Iwai<tiwai@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 Iwai<tiwai@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 Iwai<tiwai@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 Smirl<jonsmirl@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
> 
> 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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: SNDRV_PCM_TRIGGER_STOP and audio still queued in the driver

[Takashi Iwai]

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@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 Iwai<tiwai@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 Iwai<tiwai@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 Smirl<jonsmirl@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
> > 
> > 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

===

From 4cdc115fd38b54642e8536a5c2389483bcb9b2e9 Mon Sep 17 00:00:00 2001
From: Takashi Iwai <tiwai@suse.de>
Date: Thu, 20 Aug 2009 16:40:16 +0200
Subject: [PATCH] ALSA: pcm - Fix drain behavior in non-blocking mode

The current PCM core has the following problems regarding PCM draining
in 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@suse.de>
---
 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.c
index 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.c
index 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 list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: SNDRV_PCM_TRIGGER_STOP and audio still queued in the driver

[Takashi Iwai]

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 Iwai<tiwai@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 Iwai<tiwai@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 Iwai<tiwai@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 Smirl<jonsmirl@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
> > > 
> > > 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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel