[alsa-devel] 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 at 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 at 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 at 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 at 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

===