[alsa-devel] [PATCH - IOPLUG DRAIN 0/2]
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
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; }
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, };
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);
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
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
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
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
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
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
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
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
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
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; }
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
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
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
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;
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
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
participants (3)
-
Takashi Iwai -
twischer＠de.adit-jv.com -
Wischer, Timo (ADITG/ESB)