[-,IOPLUG,DRAIN,0/2]
diff mbox

Message ID s5hr2ocp4ks.wl-tiwai@suse.de
State New
Headers show

Commit Message

Takashi Iwai March 22, 2018, 2:28 p.m. UTC
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

---

Comments

Timo Wischer March 22, 2018, 2:50 p.m. UTC | #1
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
Takashi Iwai March 22, 2018, 2:55 p.m. UTC | #2
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
Timo Wischer March 22, 2018, 3:17 p.m. UTC | #3
> 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
Takashi Iwai March 22, 2018, 4:22 p.m. UTC | #4
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
Timo Wischer March 23, 2018, 7:23 a.m. UTC | #5
> 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
Takashi Iwai March 23, 2018, 7:28 a.m. UTC | #6
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
Timo Wischer March 23, 2018, 7:43 a.m. UTC | #7
> 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
Takashi Iwai March 23, 2018, 8:01 a.m. UTC | #8
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
Takashi Iwai March 23, 2018, 8:08 a.m. UTC | #9
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
Timo Wischer March 23, 2018, 8:21 a.m. UTC | #10
> 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

Patch
diff mbox

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