[PATCH - IOPLUG DRAIN 0/2]

[PATCH - IOPLUG DRAIN 0/2]

[twischer]

Hi Takashi,

> Why not poll()?

> IOW, why ioplug must be handled specially regarding the non-blocking
>operation?  The normal kernel driver behaves like that (returning
> -EAGAIN, and let apps to sync with poll()).


What do you think about the following solution?

(I thought the whole time that you have to use snd_pcm_wait() to wait for drain
in nonblocking mode but you have to use the poll_descriptors directly.)

Know I am expecting that the user is calling poll()
if snd_pcm_drain() returns -EAGAIN and
the user has to call snd_pcm_drain() again after poll returns
to check if drain is done.

Thanks for your help so far.

Best regards

Timo

[PATCH - IOPLUG DRAIN 1/1] ioplug: drain: Wait with pollwhen EAGAIN in blocking mode

[twischer]

From: Timo Wischer <twischer@de.adit-jv.com>

With this changes an IO plugin should not block by it self in the
its own snd_pcm_drain() implementation. It should return -EAGAIN and
inform the ALSA library via the poll_descriptors that draining is
done.

In non-blocking mode this implementation expects that the user blocks on
the poll_descriptors whenever snd_pcm_drain() returns -EAGAIN. In
addition the user has to call snd_pcm_drain() again to check if draining
is really done.

This change will not harm existing IO plugins which are blocking in its
snd_pcm_drain() callback.

Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>

diff --git a/include/pcm_ioplug.h b/include/pcm_ioplug.h
index c1310e3..aa08962 100644
--- a/include/pcm_ioplug.h
+++ b/include/pcm_ioplug.h
@@ -169,6 +169,12 @@ struct snd_pcm_ioplug_callback {
 	int (*prepare)(snd_pcm_ioplug_t *io);
 	/**
 	 * drain; optional
+	 * This function should never block. It should return -EAGAIN in case of
+	 * it is not yet done. In this case it will be called again by the ALSA
+	 * library (in blocking mode) or by the user (in non blocking mode).
+	 * In case of -EAGAIN the poll_descriptors have to be used to inform the
+	 * ALSA library that draining is possibly done and this callback has to
+	 * be called again.
 	 */
 	int (*drain)(snd_pcm_ioplug_t *io);
 	/**
diff --git a/src/pcm/pcm_ioplug.c b/src/pcm/pcm_ioplug.c
index 8c0ed48..7682dcd 100644
--- a/src/pcm/pcm_ioplug.c
+++ b/src/pcm/pcm_ioplug.c
@@ -492,17 +492,37 @@ static int snd_pcm_ioplug_drop(snd_pcm_t *pcm)
 static int snd_pcm_ioplug_drain(snd_pcm_t *pcm)
 {
 	ioplug_priv_t *io = pcm->private_data;
-	int err;
+	int err = 0;
 
 	if (io->data->state == SND_PCM_STATE_OPEN)
 		return -EBADFD;
 
 	io->data->state = SND_PCM_STATE_DRAINING;
-	if (io->data->callback->drain)
-		io->data->callback->drain(io->data);
-	snd_pcm_lock(pcm);
-	err = snd_pcm_ioplug_drop(pcm);
-	snd_pcm_unlock(pcm);
+	if (io->data->callback->drain) {
+		err = io->data->callback->drain(io->data);
+
+		/* in blocking mode wait unti draining is done or
+		 * an issue was detected.
+		 */
+		if (!io->data->nonblock) {
+			while (err == -EAGAIN) {
+				snd_pcm_lock(pcm);
+				err = snd_pcm_wait_nocheck(pcm, -1);
+				snd_pcm_unlock(pcm);
+				if (err  < 0)
+					break;
+				err = io->data->callback->drain(io->data);
+			}
+		}
+	}
+
+	/* only drop if draining is done or there was an issue */
+	if (err != -EAGAIN) {
+		snd_pcm_lock(pcm);
+		err = snd_pcm_ioplug_drop(pcm);
+		snd_pcm_unlock(pcm);
+	}
+
 	return err;
 }
 
-- 
2.7.4

[PATCH - IOPLUG DRAIN 1/1] jack: Support snd_pcm_drain()

[twischer]

From: Timo Wischer <twischer@de.adit-jv.com>

Without this commit the JACK thread will be stopped
before the ALSA buffer was completely forwarded to
the JACK daemon.

Signed-off-by: Timo Wischer <twischer@de.adit-jv.com>

diff --git a/jack/pcm_jack.c b/jack/pcm_jack.c
index e3df4d2..5c4d0fc 100644
--- a/jack/pcm_jack.c
+++ b/jack/pcm_jack.c
@@ -85,7 +85,12 @@ static int pcm_poll_unblock_check(snd_pcm_ioplug_t *io)
 	snd_pcm_jack_t *jack = io->private_data;
 
 	avail = snd_pcm_avail_update(io->pcm);
-	if (avail < 0 || avail >= jack->min_avail) {
+	/* In draining state poll_fd is used to wait till all pending frames are
+	 * played. Therefore it has to be guarantee that a poll event is also
+	 * generated if the buffer contains less than min_avail frames.
+	 */
+	if (avail < 0 || avail >= jack->min_avail ||
+	    io->state == SND_PCM_STATE_DRAINING) {
 		write(jack->fd, &buf, 1);
 		return 1;
 	}
@@ -161,7 +166,8 @@ snd_pcm_jack_process_cb(jack_nframes_t nframes, snd_pcm_ioplug_t *io)
 		jack->areas[channel].step = jack->sample_bits;
 	}
 
-	if (io->state == SND_PCM_STATE_RUNNING) {
+	if (io->state == SND_PCM_STATE_RUNNING ||
+	    io->state == SND_PCM_STATE_DRAINING) {
 		snd_pcm_uframes_t hw_ptr = jack->hw_ptr;
 		const snd_pcm_uframes_t hw_avail = snd_pcm_ioplug_hw_avail(io, hw_ptr,
 									   io->appl_ptr);
@@ -307,6 +313,29 @@ static int snd_pcm_jack_start(snd_pcm_ioplug_t *io)
 	return 0;
 }
 
+static int snd_pcm_jack_drain(snd_pcm_ioplug_t *io)
+{
+	snd_pcm_jack_t *jack = io->private_data;
+
+	/* Immediately stop on capture device.
+	 * snd_pcm_jack_stop() will be automatically called
+	 * by snd_pcm_ioplug_drain()
+	 */
+	if (io->stream == SND_PCM_STREAM_CAPTURE)
+		return 0;
+
+	if (snd_pcm_ioplug_hw_avail(io, jack->hw_ptr, io->appl_ptr) <= 0) {
+		/* No data pending. Nothing to drain. */
+		return 0;
+	}
+
+	/* start device if not yet done */
+	if (!jack->activated)
+		snd_pcm_jack_start(io);
+
+	return -EAGAIN;
+}
+
 static int snd_pcm_jack_stop(snd_pcm_ioplug_t *io)
 {
 	snd_pcm_jack_t *jack = io->private_data;
@@ -333,6 +362,7 @@ static snd_pcm_ioplug_callback_t jack_pcm_callback = {
 	.stop = snd_pcm_jack_stop,
 	.pointer = snd_pcm_jack_pointer,
 	.prepare = snd_pcm_jack_prepare,
+	.drain = snd_pcm_jack_drain,
 	.poll_revents = snd_pcm_jack_poll_revents,
 };
 
-- 
2.7.4

Re: [PATCH - IOPLUG DRAIN 0/2]

[Takashi Iwai]

On Thu, 22 Mar 2018 14:48:55 +0100,
<twischer@de.adit-jv.com> wrote:
> 
> 
> Hi Takashi,
> 
> > Why not poll()?
> 
> > IOW, why ioplug must be handled specially regarding the non-blocking
> >operation?  The normal kernel driver behaves like that (returning
> > -EAGAIN, and let apps to sync with poll()).
> 
> 
> What do you think about the following solution?
> 
> (I thought the whole time that you have to use snd_pcm_wait() to wait for drain
> in nonblocking mode but you have to use the poll_descriptors directly.)
> 
> Know I am expecting that the user is calling poll()
> if snd_pcm_drain() returns -EAGAIN and
> the user has to call snd_pcm_drain() again after poll returns
> to check if drain is done.

Well, the drain callback *should* block and wait until drained.  That
was the intention of the callback.

What I suggested instead is that ioctl shouldn't call drain callback
in non-blocking mode, but it should return -EAGAIN there.

That said, a change like below.  And in the plugin side, it can assume
the blocking mode and simply waits until drained.


thanks,

Takashi

---

diff --git a/src/pcm/pcm_ioplug.c b/src/pcm/pcm_ioplug.c
index 8c0ed4836365..33f7c5c27b6f 100644
--- a/src/pcm/pcm_ioplug.c
+++ b/src/pcm/pcm_ioplug.c
@@ -494,12 +494,37 @@ static int snd_pcm_ioplug_drain(snd_pcm_t *pcm)
 	ioplug_priv_t *io = pcm->private_data;
 	int err;
 
-	if (io->data->state == SND_PCM_STATE_OPEN)
+	switch (io->data->state) {
+	case SND_PCM_STATE_OPEN:
+	case SND_PCM_STATE_DISCONNECTED:
+	case SND_PCM_STATE_SUSPENDED:
 		return -EBADFD;
+	case SND_PCM_STATE_PREPARED:
+		if (pcm->stream == SND_PCM_STREAM_PLAYBACK) {
+			snd_pcm_lock(pcm);
+			err = snd_pcm_ioplug_start(pcm);
+			snd_pcm_unlock(pcm);
+			if (err < 0)
+				return err;
+			io->data->state = SND_PCM_STATE_DRAINING;
+		}
+		break;
+	case SND_PCM_STATE_RUNNING:
+		io->data->state = SND_PCM_STATE_DRAINING;
+		break;
+	}
+
+	if (io->data->state == SND_PCM_STATE_DRAINING) {
+		if (io->data->nonblock)
+			return -EAGAIN;
+
+		if (io->data->callback->drain) {
+			err = io->data->callback->drain(io->data);
+			if (err < 0)
+				return err;
+		}
+	}
 
-	io->data->state = SND_PCM_STATE_DRAINING;
-	if (io->data->callback->drain)
-		io->data->callback->drain(io->data);
 	snd_pcm_lock(pcm);
 	err = snd_pcm_ioplug_drop(pcm);
 	snd_pcm_unlock(pcm);

Re: [PATCH - IOPLUG DRAIN 0/2]

[Wischer, Timo (ADITG/ESB)]

Hi Takashi,

but in this case each IO plugin has to have its own thread which checks for io->state == DRAINING to start draining.
For example the pulse plugin do not have an own thread.


Best regards

Timo Wischer

Re: [PATCH - IOPLUG DRAIN 0/2]

[Takashi Iwai]

On Thu, 22 Mar 2018 15:50:35 +0100,
Wischer, Timo (ADITG/ESB) wrote:
> 
> Hi Takashi,
> 
> but in this case each IO plugin has to have its own thread which checks for io->state == DRAINING to start draining.
> For example the pulse plugin do not have an own thread.

I don't understand.  The pulse plugin calls pa_stream_drain(), and
pulse_wait_operation() to wait until the drain finishes.  What does it
have to do with threading?


Takashi

Re: [PATCH - IOPLUG DRAIN 0/2]

[Wischer, Timo (ADITG/ESB)]

> I don't understand.  The pulse plugin calls pa_stream_drain(), and
> pulse_wait_operation() to wait until the drain finishes.  What does it
> have to do with threading?

As far as I understand you patch io->data->callback->drain() will not be called in nonblocking mode.
+       if (io->data->state == SND_PCM_STATE_DRAINING) {
+               if (io->data->nonblock)
+                       return -EAGAIN;
+
+               if (io->data->callback->drain) {
+                       err = io->data->callback->drain(io->data);
+                       if (err < 0)
+                               return err;
+               }
+       }

Therefore how should call pa_stream_drain() in case of nonblocking mode?

Best regards

Timo

Re: [PATCH - IOPLUG DRAIN 0/2]

[Takashi Iwai]

On Thu, 22 Mar 2018 16:17:10 +0100,
Wischer, Timo (ADITG/ESB) wrote:
> 
> > I don't understand.  The pulse plugin calls pa_stream_drain(), and
> > pulse_wait_operation() to wait until the drain finishes.  What does it
> > have to do with threading?
> 
> As far as I understand you patch io->data->callback->drain() will not be called in nonblocking mode.
> +       if (io->data->state == SND_PCM_STATE_DRAINING) {
> +               if (io->data->nonblock)
> +                       return -EAGAIN;
> +
> +               if (io->data->callback->drain) {
> +                       err = io->data->callback->drain(io->data);
> +                       if (err < 0)
> +                               return err;
> +               }
> +       }
> 
> Therefore how should call pa_stream_drain() in case of nonblocking mode?

The application needs to sync manually via poll() instead.
It's also the behavior of the kernel driver, which ioplug follows.


Takashi

Re: [PATCH - IOPLUG DRAIN 0/2]

[Wischer, Timo (ADITG/ESB)]

> The application needs to sync manually via poll() instead.

You mean the user application which opens the ALSA virtual device (aka IO plugin), right?


> It's also the behavior of the kernel driver, which ioplug follows.

I know that your suggested solution is the behavior of the kernel.
But in kernel space we have the DMA interrupt which checks the state for draining.

To use the same mechanism in the IO plug each IO plug needs to have its own thread which checks state for draining.
In case of pulse without this additional thread I have no idea how pulseaudio can be informed that draining starts
because in nonblocking mode there is no function in the pulse IO plugin which is called to inform about the state change to draining.

The only solution which I have in my mind is this additional thread
state_check_thread()
{
while (true) {
if (state == DRAINING)
pa_stream_drain()
...
}
}

Therefore with your proposed solution there is additional effort in each new IO plug required to support draining in nonblocking mode. (With my solution exactly the same mechanism (drain callback and poll) is used in blocking and nonblocking mode. Therefore new  IO plugins will support both modes without additional efforts in the IO plugin implementation)

I hope my concern is now more clear.


Best regards

Timo

Re: [PATCH - IOPLUG DRAIN 0/2]

[Takashi Iwai]

On Fri, 23 Mar 2018 08:23:56 +0100,
Wischer, Timo (ADITG/ESB) wrote:
> 
> > The application needs to sync manually via poll() instead.
> 
> You mean the user application which opens the ALSA virtual device (aka IO plugin), right?

No, no matter which device is used.

> > It's also the behavior of the kernel driver, which ioplug follows.
> 
> I know that your suggested solution is the behavior of the kernel.
> But in kernel space we have the DMA interrupt which checks the state for draining.
> 
> To use the same mechanism in the IO plug each IO plug needs to have its own thread which checks state for draining.
> In case of pulse without this additional thread I have no idea how pulseaudio can be informed that draining starts
> because in nonblocking mode there is no function in the pulse IO plugin which is called to inform about the state change to draining.

In non-blocking mode, drain is never called.

> The only solution which I have in my mind is this additional thread
> state_check_thread()
> {
> while (true) {
> if (state == DRAINING)
> pa_stream_drain()
> ...
> }
> }
> 
> Therefore with your proposed solution there is additional effort in each new IO plug required to support draining in nonblocking mode. (With my solution exactly the same mechanism (drain callback and poll) is used in blocking and nonblocking mode. Therefore new  IO plugins will support both modes without additional efforts in the IO plugin implementation)

No, again, in non-blocking mode, the drain callback will get never
called.  It's the responsibility of application to sync with poll()
instead.


Takashi

Re: [PATCH - IOPLUG DRAIN 0/2]

[Wischer, Timo (ADITG/ESB)]

> No, again, in non-blocking mode, the drain callback will get never
> called.  It's the responsibility of application to sync with poll()
> instead.

Sorry but I do not get it anyway.

The user application which is playing some audio has to do the following to drain in nonblocking mode, right?
snd_pcm_poll_descriptors(pfds)
while (snd_pcm_drain() == -EAGAIN) {
poll(pfds)
}


But in nonblocking mode the drain callback of the IO plugin will never be called with your solution.
Therefore in case of the pulse IO plugin which function should call pa_stream_drain()?
The user application will not do it directly and poll can also not call it.

Thanks for your time.

Best regards

Timo

Re: [PATCH - IOPLUG DRAIN 0/2]

[Takashi Iwai]

On Fri, 23 Mar 2018 08:43:10 +0100,
Wischer, Timo (ADITG/ESB) wrote:
> 
> > No, again, in non-blocking mode, the drain callback will get never
> > called.  It's the responsibility of application to sync with poll()
> > instead.
> 
> Sorry but I do not get it anyway.
> 
> The user application which is playing some audio has to do the following to drain in nonblocking mode, right?
> snd_pcm_poll_descriptors(pfds)
> while (snd_pcm_drain() == -EAGAIN) {
> poll(pfds)
> }
> 
> 
> But in nonblocking mode the drain callback of the IO plugin will never be called with your solution.
> Therefore in case of the pulse IO plugin which function should call pa_stream_drain()?
> The user application will not do it directly and poll can also not call it.

OK, now I understand your concern.  Yes it's another missing piece,
snd_pcm_ioplug_hw_ptr_update() needs to check the current state, and
it drops to SETUP state instead of XRUN when it was DRAINING.
Then application can simply do poll() and status update until it goes
out of DRAINING state.

But still it's outside the plugin, drain callback isn't called there.


Takashi

Re: [PATCH - IOPLUG DRAIN 0/2]

[Takashi Iwai]

On Fri, 23 Mar 2018 09:01:35 +0100,
Takashi Iwai wrote:
> 
> On Fri, 23 Mar 2018 08:43:10 +0100,
> Wischer, Timo (ADITG/ESB) wrote:
> > 
> > > No, again, in non-blocking mode, the drain callback will get never
> > > called.  It's the responsibility of application to sync with poll()
> > > instead.
> > 
> > Sorry but I do not get it anyway.
> > 
> > The user application which is playing some audio has to do the following to drain in nonblocking mode, right?
> > snd_pcm_poll_descriptors(pfds)
> > while (snd_pcm_drain() == -EAGAIN) {
> > poll(pfds)
> > }
> > 
> > 
> > But in nonblocking mode the drain callback of the IO plugin will never be called with your solution.
> > Therefore in case of the pulse IO plugin which function should call pa_stream_drain()?
> > The user application will not do it directly and poll can also not call it.
> 
> OK, now I understand your concern.  Yes it's another missing piece,
> snd_pcm_ioplug_hw_ptr_update() needs to check the current state, and
> it drops to SETUP state instead of XRUN when it was DRAINING.
> Then application can simply do poll() and status update until it goes
> out of DRAINING state.
> 
> But still it's outside the plugin, drain callback isn't called there.

.... and now thinking of this again, the whole story can be folded
back:

- The standard drain behavior can be implemented without plugin's own
  code; it's just a poll and status check.

- For any special case (or better implementation than poll()), we may
  leave the whole draining callback action to each plugin; that's the
  case of PA.


Takashi

Re: [PATCH - IOPLUG DRAIN 0/2]

[Takashi Iwai]

On Fri, 23 Mar 2018 09:08:43 +0100,
Takashi Iwai wrote:
> 
> On Fri, 23 Mar 2018 09:01:35 +0100,
> Takashi Iwai wrote:
> > 
> > On Fri, 23 Mar 2018 08:43:10 +0100,
> > Wischer, Timo (ADITG/ESB) wrote:
> > > 
> > > > No, again, in non-blocking mode, the drain callback will get never
> > > > called.  It's the responsibility of application to sync with poll()
> > > > instead.
> > > 
> > > Sorry but I do not get it anyway.
> > > 
> > > The user application which is playing some audio has to do the following to drain in nonblocking mode, right?
> > > snd_pcm_poll_descriptors(pfds)
> > > while (snd_pcm_drain() == -EAGAIN) {
> > > poll(pfds)
> > > }
> > > 
> > > 
> > > But in nonblocking mode the drain callback of the IO plugin will never be called with your solution.
> > > Therefore in case of the pulse IO plugin which function should call pa_stream_drain()?
> > > The user application will not do it directly and poll can also not call it.
> > 
> > OK, now I understand your concern.  Yes it's another missing piece,
> > snd_pcm_ioplug_hw_ptr_update() needs to check the current state, and
> > it drops to SETUP state instead of XRUN when it was DRAINING.
> > Then application can simply do poll() and status update until it goes
> > out of DRAINING state.
> > 
> > But still it's outside the plugin, drain callback isn't called there.
> 
> .... and now thinking of this again, the whole story can be folded
> back:
> 
> - The standard drain behavior can be implemented without plugin's own
>   code; it's just a poll and status check.
> 
> - For any special case (or better implementation than poll()), we may
>   leave the whole draining callback action to each plugin; that's the
>   case of PA.

Maybe it's easier to understand by a patch.  Totally untested, but you
get the idea from it.

The check in snd_pcm_ioplug_hw_ptr_update() can be extended to the
XRUN check, too.  But do it in another patch.

And, yeah, this still misses the proper non-blocking mode handling in
pulse plugin.  It's to be fixed there.


Takashi

-- 8< --
--- a/src/pcm/pcm_ioplug.c
+++ b/src/pcm/pcm_ioplug.c
@@ -47,6 +47,11 @@ typedef struct snd_pcm_ioplug_priv {
 	snd_htimestamp_t trigger_tstamp;
 } ioplug_priv_t;
 
+static int snd_pcm_ioplug_drop(snd_pcm_t *pcm);
+static int snd_pcm_ioplug_poll_descriptors_count(snd_pcm_t *pcm);
+static int snd_pcm_ioplug_poll_descriptors(snd_pcm_t *pcm, struct pollfd *pfds, unsigned int space);
+static int snd_pcm_ioplug_poll_revents(snd_pcm_t *pcm, struct pollfd *pfds, unsigned int nfds, unsigned short *revents);
+
 /* update the hw pointer */
 /* called in lock */
 static void snd_pcm_ioplug_hw_ptr_update(snd_pcm_t *pcm)
@@ -57,6 +62,7 @@ static void snd_pcm_ioplug_hw_ptr_update(snd_pcm_t *pcm)
 	hw = io->data->callback->pointer(io->data);
 	if (hw >= 0) {
 		snd_pcm_uframes_t delta;
+		snd_pcm_uframes_t avail;
 
 		if ((snd_pcm_uframes_t)hw >= io->last_hw)
 			delta = hw - io->last_hw;
@@ -67,6 +73,12 @@ static void snd_pcm_ioplug_hw_ptr_update(snd_pcm_t *pcm)
 			delta = wrap_point + hw - io->last_hw;
 		}
 		snd_pcm_mmap_hw_forward(io->data->pcm, delta);
+		/* stop the stream if all samples are drained */
+		if (io->data->state == SND_PCM_STATE_DRAINING) {
+			avail = snd_pcm_mmap_avail(pcm);
+			if (avail >= pcm->buffer_size)
+				snd_pcm_ioplug_drop(pcm);
+		}
 		io->last_hw = (snd_pcm_uframes_t)hw;
 	} else
 		io->data->state = SNDRV_PCM_STATE_XRUN;
@@ -488,20 +500,66 @@ static int snd_pcm_ioplug_drop(snd_pcm_t *pcm)
 	return 0;
 }
 
+static int ioplug_drain_via_poll(snd_pcm_t *pcm)
+{
+	ioplug_priv_t *io = pcm->private_data;
+	int err;
+
+	/* in non-blocking mode, leave application to poll() by itself */
+	if (io->data->nonblock)
+		return -EAGAIN;
+
+	while (io->data->state == SND_PCM_STATE_DRAINING) {
+		err = snd_pcm_wait_nocheck(pcm, -1);
+		snd_pcm_ioplug_hw_ptr_update(pcm);
+		if (err < 0)
+			break;
+	}
+
+	return 0;
+}
+
 /* need own locking */
 static int snd_pcm_ioplug_drain(snd_pcm_t *pcm)
 {
 	ioplug_priv_t *io = pcm->private_data;
 	int err;
 
-	if (io->data->state == SND_PCM_STATE_OPEN)
+	snd_pcm_lock(pcm);
+	switch (io->data->state) {
+	case SND_PCM_STATE_OPEN:
+	case SND_PCM_STATE_DISCONNECTED:
+	case SND_PCM_STATE_SUSPENDED:
 		return -EBADFD;
+	case SND_PCM_STATE_PREPARED:
+		if (pcm->stream == SND_PCM_STREAM_PLAYBACK) {
+			err = snd_pcm_ioplug_start(pcm);
+			if (err < 0)
+				goto unlock;
+			io->data->state = SND_PCM_STATE_DRAINING;
+		}
+		break;
+	case SND_PCM_STATE_RUNNING:
+		io->data->state = SND_PCM_STATE_DRAINING;
+		break;
+	}
 
-	io->data->state = SND_PCM_STATE_DRAINING;
-	if (io->data->callback->drain)
-		io->data->callback->drain(io->data);
-	snd_pcm_lock(pcm);
-	err = snd_pcm_ioplug_drop(pcm);
+	if (io->data->state == SND_PCM_STATE_DRAINING) {
+		if (io->data->callback->drain) {
+			snd_pcm_unlock(pcm); /* let plugin own locking */
+			err = io->data->callback->drain(io->data);
+			snd_pcm_lock(pcm);
+		} else {
+			err = ioplug_drain_via_poll(pcm);
+		}
+		if (err < 0)
+			goto unlock;
+	}
+
+	if (io->data->state != SND_PCM_STATE_SETUP)
+		err = snd_pcm_ioplug_drop(pcm);
+
+ unlock:
 	snd_pcm_unlock(pcm);
 	return err;
 }

Re: [PATCH - IOPLUG DRAIN 0/2]

[Wischer, Timo (ADITG/ESB)]

Hi Takashi,

now I got your idea.
Thanks for the patch.
But I see some small concerns.
See my inline comments:

Beside this concerns I really like this solution.


-- 8< --
--- a/src/pcm/pcm_ioplug.c
+++ b/src/pcm/pcm_ioplug.c
@@ -47,6 +47,11 @@ typedef struct snd_pcm_ioplug_priv {
        snd_htimestamp_t trigger_tstamp;
 } ioplug_priv_t;

+static int snd_pcm_ioplug_drop(snd_pcm_t *pcm);
+static int snd_pcm_ioplug_poll_descriptors_count(snd_pcm_t *pcm);
+static int snd_pcm_ioplug_poll_descriptors(snd_pcm_t *pcm, struct pollfd *pfds, unsigned int space);
+static int snd_pcm_ioplug_poll_revents(snd_pcm_t *pcm, struct pollfd *pfds, unsigned int nfds, unsigned short *revents);
+
 /* update the hw pointer */
 /* called in lock */
 static void snd_pcm_ioplug_hw_ptr_update(snd_pcm_t *pcm)
@@ -57,6 +62,7 @@ static void snd_pcm_ioplug_hw_ptr_update(snd_pcm_t *pcm)
        hw = io->data->callback->pointer(io->data);
        if (hw >= 0) {
                snd_pcm_uframes_t delta;
+               snd_pcm_uframes_t avail;

                if ((snd_pcm_uframes_t)hw >= io->last_hw)
                        delta = hw - io->last_hw;
@@ -67,6 +73,12 @@ static void snd_pcm_ioplug_hw_ptr_update(snd_pcm_t *pcm)
                        delta = wrap_point + hw - io->last_hw;
                }
                snd_pcm_mmap_hw_forward(io->data->pcm, delta);
+               /* stop the stream if all samples are drained */
+               if (io->data->state == SND_PCM_STATE_DRAINING) {
+                       avail = snd_pcm_mmap_avail(pcm);
+                       if (avail >= pcm->buffer_size)
+                               snd_pcm_ioplug_drop(pcm);
+               }
                io->last_hw = (snd_pcm_uframes_t)hw;
        } else
                io->data->state = SNDRV_PCM_STATE_XRUN;
In case of draining drop has to be called because draining is done

@@ -488,20 +500,66 @@ static int snd_pcm_ioplug_drop(snd_pcm_t *pcm)
        return 0;
 }

+static int ioplug_drain_via_poll(snd_pcm_t *pcm)
+{
+       ioplug_priv_t *io = pcm->private_data;
+       int err;
+
+       /* in non-blocking mode, leave application to poll() by itself */
+       if (io->data->nonblock)
+               return -EAGAIN;
In case of nonblock snd_pcm_ioplug_hw_ptr_update() will not be called by the user
Therefore the user will never detect that draining is done.
I fear snd_pcm_ioplug_hw_ptr_update() has also to be called in nonblock mode here.

+
+       while (io->data->state == SND_PCM_STATE_DRAINING) {
+               err = snd_pcm_wait_nocheck(pcm, -1);
+               snd_pcm_ioplug_hw_ptr_update(pcm);
+               if (err < 0)
+                       break;
+       }
+
+       return 0;
+}
+
 /* need own locking */
 static int snd_pcm_ioplug_drain(snd_pcm_t *pcm)
 {
        ioplug_priv_t *io = pcm->private_data;
        int err;

-       if (io->data->state == SND_PCM_STATE_OPEN)
+       snd_pcm_lock(pcm);
+       switch (io->data->state) {
+       case SND_PCM_STATE_OPEN:
+       case SND_PCM_STATE_DISCONNECTED:
+       case SND_PCM_STATE_SUSPENDED:
                return -EBADFD;
+       case SND_PCM_STATE_PREPARED:
+               if (pcm->stream == SND_PCM_STREAM_PLAYBACK) {
+                       err = snd_pcm_ioplug_start(pcm);
+                       if (err < 0)
+                               goto unlock;
+                       io->data->state = SND_PCM_STATE_DRAINING;
+               }
+               break;
+       case SND_PCM_STATE_RUNNING:
+               io->data->state = SND_PCM_STATE_DRAINING;
+               break;
+       }

-       io->data->state = SND_PCM_STATE_DRAINING;
-       if (io->data->callback->drain)
-               io->data->callback->drain(io->data);
-       snd_pcm_lock(pcm);
-       err = snd_pcm_ioplug_drop(pcm);
+       if (io->data->state == SND_PCM_STATE_DRAINING) {
+               if (io->data->callback->drain) {
+                       snd_pcm_unlock(pcm); /* let plugin own locking */
+                       err = io->data->callback->drain(io->data);
+                       snd_pcm_lock(pcm);
+               } else {
+                       err = ioplug_drain_via_poll(pcm);
+               }
+               if (err < 0)
+                       goto unlock;
+       }
+
+       if (io->data->state != SND_PCM_STATE_SETUP)
+               err = snd_pcm_ioplug_drop(pcm);
+
+ unlock:
        snd_pcm_unlock(pcm);
        return err;
 }

Will you create the patch and apply it to master or
is there anything which I have to do?

Best regards

Timo

Re: [PATCH - IOPLUG DRAIN 0/2]

[Takashi Iwai]

On Wed, 28 Mar 2018 10:42:50 +0200,
Wischer, Timo (ADITG/ESB) wrote:
> 
> @@ -67,6 +73,12 @@ static void snd_pcm_ioplug_hw_ptr_update(snd_pcm_t *pcm)
>                         delta = wrap_point + hw - io->last_hw;
>                 }
>                 snd_pcm_mmap_hw_forward(io->data->pcm, delta);
> +               /* stop the stream if all samples are drained */
> +               if (io->data->state == SND_PCM_STATE_DRAINING) {
> +                       avail = snd_pcm_mmap_avail(pcm);
> +                       if (avail >= pcm->buffer_size)
> +                               snd_pcm_ioplug_drop(pcm);
> +               }
>                 io->last_hw = (snd_pcm_uframes_t)hw;
>         } else
>                 io->data->state = SNDRV_PCM_STATE_XRUN;
> In case of draining drop has to be called because draining is done

OK, makes sense.


> @@ -488,20 +500,66 @@ static int snd_pcm_ioplug_drop(snd_pcm_t *pcm)
>         return 0;
>  }
> 
> +static int ioplug_drain_via_poll(snd_pcm_t *pcm)
> +{
> +       ioplug_priv_t *io = pcm->private_data;
> +       int err;
> +
> +       /* in non-blocking mode, leave application to poll() by itself */
> +       if (io->data->nonblock)
> +               return -EAGAIN;
> In case of nonblock snd_pcm_ioplug_hw_ptr_update() will not be called by the user
> Therefore the user will never detect that draining is done.
> I fear snd_pcm_ioplug_hw_ptr_update() has also to be called in nonblock mode here.

For the non-blocking mode, it's not supposed that drain() is called
multiple times.  Instead, it should do the loop of snd_pcm_wait(), and
status check, as ioplug_drain_vai_poll() actually does.

Yes, it's weird, but it's the standard way for non-blocking mode, even
for the kernel drivers.  So, the practical recommendation for draining
is to temporarily switch to blocking mode before calling
snd_pcm_drain().
	

> Will you create the patch and apply it to master or
> is there anything which I have to do?

I'll cook up the proper patch and submit to ML soon later.
Then it'll be merged to git tree, and you can work on it.


thanks,

Takashi

Re: [PATCH - IOPLUG DRAIN 0/2]

[Wischer, Timo (ADITG/ESB)]

> For the non-blocking mode, it's not supposed that drain() is called
> multiple times.  Instead, it should do the loop of snd_pcm_wait(), and
> status check, as ioplug_drain_vai_poll() actually does.

I fear the repeat call to snd_pcm_wait() when draining is active would end up in 100% CPU load as long as draining is not done because snd_pcm_wait() would immediate return due to the fact that avail > min_avail at the end of draining.

I have not tested it but I could also not find a line which would ignore the avail > min_avail short circuit of snd_pcm_wait().

Best regards

Timo

Re: [PATCH - IOPLUG DRAIN 0/2]

[Takashi Iwai]

On Thu, 29 Mar 2018 08:39:36 +0200,
Wischer, Timo (ADITG/ESB) wrote:
> 
> > For the non-blocking mode, it's not supposed that drain() is called
> > multiple times.  Instead, it should do the loop of snd_pcm_wait(), and
> > status check, as ioplug_drain_vai_poll() actually does.
> 
> I fear the repeat call to snd_pcm_wait() when draining is active would end up in 100% CPU load as long as draining is not done because snd_pcm_wait() would immediate return due to the fact that avail > min_avail at the end of draining.
> 
> I have not tested it but I could also not find a line which would ignore the avail > min_avail short circuit of snd_pcm_wait().

A good point, and indeed it's broken.
We have to skip the avail_min check during draining.  It's already
done in the kernel code, but forgotten in alsa-lib side.

The fix patch is below.


Thanks!

Takashi

-- 8< --
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH] pcm: Skip avail_min check during draining

snd_pcm_wait() & co checks the current avail value and returns
immediately if it satisfies <= avail_min condition.  It's good in
general except for one situation: draining.  When the draining is
being performed in the non-blocking mode, apps are supposed to wait
via poll(), typically via snd_pcm_wait().  So this ends up with the
busy loop because of the immediate return from snd_pcm_wait().

A simple workaround is to put the PCM state check and ignore the
avail_min condition if it's DRAINING state.  The equivalent check is
found in the kernel xfer code, too.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 src/pcm/pcm.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/pcm/pcm.c b/src/pcm/pcm.c
index ed47cb516c73..11aec8052135 100644
--- a/src/pcm/pcm.c
+++ b/src/pcm/pcm.c
@@ -2751,7 +2751,9 @@ int __snd_pcm_wait_in_lock(snd_pcm_t *pcm, int timeout)
 {
 	int err;
 
-	if (!snd_pcm_may_wait_for_avail_min(pcm, snd_pcm_mmap_avail(pcm))) {
+	/* NOTE: avail_min check can be skipped during draining */
+	if (__snd_pcm_state(pcm) != SND_PCM_STATE_DRAINING &&
+	    !snd_pcm_may_wait_for_avail_min(pcm, snd_pcm_mmap_avail(pcm))) {
 		/* check more precisely */
 		err = pcm_state_to_error(__snd_pcm_state(pcm));
 		return err < 0 ? err : 1;
-- 
2.16.2

Re: [PATCH - IOPLUG DRAIN 0/2]

[Wischer, Timo (ADITG/ESB)]

> We have to skip the avail_min check during draining.  It's already
> done in the kernel code, but forgotten in alsa-lib side.

> The fix patch is below.

Perfect.
Thanks a lot.

So I am waiting for your patch and will then adapt/test the JACK implementation with draining.

Best regards

Timo

Re: [PATCH - IOPLUG DRAIN 0/2]

[Wischer, Timo (ADITG/ESB)]

> OK, now I understand your concern.  Yes it's another missing piece,
> snd_pcm_ioplug_hw_ptr_update() needs to check the current state, and
> it drops to SETUP state instead of XRUN when it was DRAINING.
> Then application can simply do poll() and status update until it goes
> out of DRAINING state.

Okay.
Therefore the pulse plugin has to call pa_stream_drain() from its IO plug pointer callback in case of state DRAINING, right?


> But still it's outside the plugin, drain callback isn't called there.

I think this is not the best design decision because there is an IO plug callback for each state transitions.
Only for transitions to draining in nonblocking mode there will be no IO plug callback.
I think this could be confusing for some IO plugin developers.
What do you think?

Timo