add SNDRV_PCM_TRIGGER_DRAIN trigger

From: Libin Yang <[email protected]>

Add SNDRV_PCM_TRIGGER_DRAIN trigger for pcm drain.

Some audio devices require notification of drain events in order to properly drain and shutdown an audio stream.

Signed-off-by Libin Yang <[email protected]>

---
 include/sound/pcm.h     | 1 +
 sound/core/pcm_native.c | 5 +++++
 2 files changed, 6 insertions(+)

include/sound/pcm.h +1 -0

index 8bb00a2..bb77aa3 100644
@@ -109,6 +109,7 @@ struct snd_pcm_ops {
 #define SNDRV_PCM_TRIGGER_PAUSE_RELEASE	4
 #define SNDRV_PCM_TRIGGER_SUSPEND	5
 #define SNDRV_PCM_TRIGGER_RESUME	6
+#define SNDRV_PCM_TRIGGER_DRAIN		7
 
 #define SNDRV_PCM_POS_XRUN		((snd_pcm_uframes_t)-1)
 

sound/core/pcm_native.c +0 -0

index 166d59c..15fe682 100644

-----Original Message-----
From: Takashi Iwai [mailto:[email protected]]
Sent: Thursday, December 18, 2014 4:30 PM
To: Yang, Libin
Cc: [email protected]; [email protected]; [email protected]; Jie, Yang
Subject: Re: [PATCH 1/2] [ALSA] add SNDRV_PCM_TRIGGER_DRAIN trigger

At Thu, 18 Dec 2014 15:36:03 +0800,

Quoted 26 lines · [email protected] wrote:

From: Libin Yang <[email protected]>

Add SNDRV_PCM_TRIGGER_DRAIN trigger for pcm drain.

Some audio devices require notification of drain events in order to properly drain and shutdown an audio stream.

Signed-off-by Libin Yang <[email protected]>

---
 include/sound/pcm.h     | 1 +
 sound/core/pcm_native.c | 5 +++++
 2 files changed, 6 insertions(+)

include/sound/pcm.h index renamed from include/sound/pcm.h +0 -0

8bb00a2..bb77aa3 100644

include/sound/pcm.h +1 -0

@@ -109,6 +109,7 @@ struct snd_pcm_ops {
 #define SNDRV_PCM_TRIGGER_PAUSE_RELEASE	4
 #define SNDRV_PCM_TRIGGER_SUSPEND	5
 #define SNDRV_PCM_TRIGGER_RESUME	6
+#define SNDRV_PCM_TRIGGER_DRAIN		7
 
 #define SNDRV_PCM_POS_XRUN		((snd_pcm_uframes_t)-1)
 

sound/core/pcm_native.c static int snd_pcm_do_drain_init(struct · line 1523

Quoted 12 lines

sound/core/pcm_native.c index renamed from sound/core/pcm_native.c +0 -0

166d59c..15fe682 100644

sound/core/pcm_native.c +4 -0

@@ -1517,6 +1517,11 @@ static int snd_pcm_do_drain_init(struct
 snd_pcm_substream *substream, int state)  {
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+
+		if (substream->runtime->trigger_master == substream)
+			substream->ops->trigger(substream,
+					SNDRV_PCM_TRIGGER_DRAIN);

The return value check is missing.

Besides, this patch will break many drivers. Many of them will spew errors if an unexpected trigger command is issued. So, you have to limit this trigger only to some drivers. A typical workaround is to add a new SNDRV_PCM_INFO_* and issue the trigger only when the flag is set, as done for resume and pause operations.

Also, do you want to see this trigger even if the stream isn't started before drain is issued? Also, why no need for this trigger for capture streams?

(BTW, while looking at this patch, I found a bug in the current draining code. Will submit the fix...)

Takashi

Hi Takashi,

Thanks for review, please see my comments.

Quoted 36 lines

-----Original Message-----
From: Takashi Iwai [mailto:[email protected]]
Sent: Thursday, December 18, 2014 4:30 PM
To: Yang, Libin
Cc: [email protected]; [email protected]; [email protected]; Jie, Yang
Subject: Re: [PATCH 1/2] [ALSA] add SNDRV_PCM_TRIGGER_DRAIN trigger

At Thu, 18 Dec 2014 15:36:03 +0800,

Quoted 26 lines · [email protected] wrote:

From: Libin Yang <[email protected]>

Add SNDRV_PCM_TRIGGER_DRAIN trigger for pcm drain.

Some audio devices require notification of drain events in order to properly drain and shutdown an audio stream.

Signed-off-by Libin Yang <[email protected]>

---
 include/sound/pcm.h     | 1 +
 sound/core/pcm_native.c | 5 +++++
 2 files changed, 6 insertions(+)

include/sound/pcm.h index renamed from include/sound/pcm.h +0 -0

8bb00a2..bb77aa3 100644

include/sound/pcm.h +1 -0

@@ -109,6 +109,7 @@ struct snd_pcm_ops {
 #define SNDRV_PCM_TRIGGER_PAUSE_RELEASE	4
 #define SNDRV_PCM_TRIGGER_SUSPEND	5
 #define SNDRV_PCM_TRIGGER_RESUME	6
+#define SNDRV_PCM_TRIGGER_DRAIN		7
 
 #define SNDRV_PCM_POS_XRUN		((snd_pcm_uframes_t)-1)
 

Quoted 14 lines

sound/core/pcm_native.c static int snd_pcm_do_drain_init(struct · line 1523

Quoted 12 lines

sound/core/pcm_native.c index renamed from sound/core/pcm_native.c +0 -0

166d59c..15fe682 100644

sound/core/pcm_native.c +4 -0

@@ -1517,6 +1517,11 @@ static int snd_pcm_do_drain_init(struct
 snd_pcm_substream *substream, int state)  {
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+
+		if (substream->runtime->trigger_master == substream)
+			substream->ops->trigger(substream,
+					SNDRV_PCM_TRIGGER_DRAIN);

The return value check is missing.

Actually, the code doesn't care the return value and I don't want to let the trigger return value impact the snd_pcm_do_drain_init() flow just like snd_pcm_do_stop().

Quoted 5 lines

Besides, this patch will break many drivers. Many of them will spew errors if an unexpected trigger command is issued. So, you have to limit this trigger only to some drivers. A typical workaround is to add a new SNDRV_PCM_INFO_* and issue the trigger only when the flag is set, as done for resume and pause operations.

Yes, that's a good suggestion. Thanks for reminding me.

Quoted 2 lines

Also, do you want to see this trigger even if the stream isn't started before drain is issued? Also, why no need for this trigger for capture streams?

In this case, the trigger action will be started. For the stream not started case, if I skip the trigger , I don't know where I can have the chance to trigger later. And I didn't find the suitable place to trigger my code. Put the trigger at the end of the function snd_pcm_do_drain_init() or in the function snd_pcm_post_drain_init() will be OK?

I didn't include the capture case because we found the capture currently don't need the trigger.

Our issue is the fw don't know when to stop fetching the data. For playback we need fill silence data when draining, otherwise the fw will get the old data. But for capture, after the driver has fetched the data, it will stopped and no old data will be fetched.

-----Original Message-----
From: Takashi Iwai [mailto:[email protected]]
Sent: Friday, December 19, 2014 4:48 PM
To: Yang, Libin
Cc: [email protected]; [email protected]; [email protected]; Jie, Yang
Subject: Re: [PATCH 1/2] [ALSA] add SNDRV_PCM_TRIGGER_DRAIN trigger

At Fri, 19 Dec 2014 06:29:33 +0000,

Quoted 13 lines · Libin

Hi Takashi,

Thanks for review, please see my comments.

Quoted 7 lines

-----Original Message-----
From: Takashi Iwai [mailto:[email protected]]
Sent: Thursday, December 18, 2014 4:30 PM
To: Yang, Libin
Cc: [email protected]; [email protected]; [email protected]; Jie, Yang
Subject: Re: [PATCH 1/2] [ALSA] add SNDRV_PCM_TRIGGER_DRAIN

trigger

Quoted 27 lines

Quoted 27 lines

At Thu, 18 Dec 2014 15:36:03 +0800,

Quoted 24 lines · [email protected] wrote:

From: Libin Yang <[email protected]>

Add SNDRV_PCM_TRIGGER_DRAIN trigger for pcm drain.

Some audio devices require notification of drain events in order to properly drain and shutdown an audio stream.

Signed-off-by Libin Yang <[email protected]>

---
 include/sound/pcm.h     | 1 +
 sound/core/pcm_native.c | 5 +++++
 2 files changed, 6 insertions(+)

include/sound/pcm.h index renamed from include/sound/pcm.h +0 -0

8bb00a2..bb77aa3 100644

include/sound/pcm.h +1 -0

@@ -109,6 +109,7 @@ struct snd_pcm_ops {
 #define SNDRV_PCM_TRIGGER_PAUSE_RELEASE	4
 #define SNDRV_PCM_TRIGGER_SUSPEND	5
 #define SNDRV_PCM_TRIGGER_RESUME	6
+#define SNDRV_PCM_TRIGGER_DRAIN		7
 

Quoted 1 line

Quoted 1 line

include/sound/pcm.h struct snd_pcm_ops { · line 114

Quoted 1 line

include/sound/pcm.h +0 -0

 #define SNDRV_PCM_POS_XRUN

	((snd_pcm_uframes_t)-1)

Quoted 17 lines

Quoted 16 lines

Quoted 7 lines

sound/core/pcm_native.c +0 -0

index
166d59c..15fe682 100644

sound/core/pcm_native.c static int snd_pcm_do_drain_init(struct · line 1523

Quoted 7 lines

sound/core/pcm_native.c +4 -0

@@ -1517,6 +1517,11 @@ static int snd_pcm_do_drain_init(struct
 snd_pcm_substream *substream, int state)  {
 	struct snd_pcm_runtime *runtime = substream->runtime;
 	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+
+		if (substream->runtime->trigger_master == substream)
+			substream->ops->trigger(substream,
+					SNDRV_PCM_TRIGGER_DRAIN);

The return value check is missing.

Quoted 3 lines

Actually, the code doesn't care the return value and I don't want to let the trigger return value impact the snd_pcm_do_drain_init() flow just like snd_pcm_do_stop().

Well, the fundamental problem in this patch is that it was written *just for your hardware* although this is a public API extension.
Consider again about it: if the return value of this trigger doesn't matter for your hardware, the driver can simply return zero always.
Why do you need to drop the check in the common code? Wouldn't any driver return any error there?

Quoted 9 lines

Quoted 8 lines

Besides, this patch will break many drivers. Many of them will spew errors if an unexpected trigger command is issued. So, you have to limit this trigger only to some drivers. A typical workaround is to add a new
SNDRV_PCM_INFO_* and issue the trigger only when the flag is set, as done for resume and pause operations.

Quoted 1 line

Yes, that's a good suggestion. Thanks for reminding me.

OTOH, the info flag will be exposed to user-space while this is a kernel-space- only information. So, in the code copying the info flag (snd_pcm_hw_refine()) would need to drop this bit, too.

Alternatively, we may add just a new flag in struct snd_pcm or struct snd_pcm_substream and let the driver setting it manually. This might need more changes in ASoC side, though, as the PCM creation isn't directly done by the driver.

Quoted 4 lines

Quoted 4 lines

Also, do you want to see this trigger even if the stream isn't started before drain is issued? Also, why no need for this trigger for

capture streams?

Quoted 4 lines

In this case, the trigger action will be started. For the stream not started case, if I skip the trigger , I don't know where I can have the chance to trigger later. And I didn't find the suitable place to trigger my

code.

Quoted 8 lines

Put the trigger at the end of the function snd_pcm_do_drain_init() or in the function snd_pcm_post_drain_init() will be OK?

I didn't include the capture case because we found the capture currently don't need the trigger.

Our issue is the fw don't know when to stop fetching the data. For playback we need fill silence data when draining, otherwise the fw will get

the old data.

Quoted 2 lines

But for capture, after the driver has fetched the data, it will stopped and no old data will be fetched.

A similar problem here. If you extend the API, don't build up only from the bottom. Rather define from a higher POV.

If calling the drain trigger for capture stream doesn't make *any* sense for all hardware, then we shouldn't add it. This would be the only reason not to do it. What about empty streams...?

That being said, we need to define the proper abstraction, what this trigger does for which purpose, not specific to a certain hardware.
That is, the requirement goes up from your hardware level while the design comes down from the abstraction level.

So, in general I found it good to have this kind of API extension. But let's add it a bit more carefully.

thanks,

Takashi

Hi Takashi,

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

Besides, this patch will break many drivers. Many of them will spew errors if an unexpected trigger command is issued. So, you have to limit this trigger only to some drivers. A typical workaround is to add a new
SNDRV_PCM_INFO_* and issue the trigger only when the flag is set, as done for resume and pause operations.

Quoted 5 lines

Quoted 1 line

Yes, that's a good suggestion. Thanks for reminding me.

OTOH, the info flag will be exposed to user-space while this is a kernel-space- only information. So, in the code copying the info flag (snd_pcm_hw_refine()) would need to drop this bit, too.

Thanks, I will drop this bit in snd_pcm_hw_refine().

Quoted 5 lines

Alternatively, we may add just a new flag in struct snd_pcm or struct snd_pcm_substream and let the driver setting it manually. This might need more changes in ASoC side, though, as the PCM creation isn't directly done by the driver.

Yes, it is a little complex for asoc if using this method. Not sure if it is OK we set the flag in snd_soc_ops->startup() each time when the function is called.

What do you think we just use the first method?

Quoted 25 lines

Quoted 4 lines

Quoted 4 lines

Also, do you want to see this trigger even if the stream isn't started before drain is issued? Also, why no need for this trigger for

capture streams?

Quoted 4 lines

In this case, the trigger action will be started. For the stream not started case, if I skip the trigger , I don't know where I can have the chance to trigger later. And I didn't find the suitable place to trigger my

code.

Quoted 8 lines

Put the trigger at the end of the function snd_pcm_do_drain_init() or in the function snd_pcm_post_drain_init() will be OK?

I didn't include the capture case because we found the capture currently don't need the trigger.

Our issue is the fw don't know when to stop fetching the data. For playback we need fill silence data when draining, otherwise the fw will get

the old data.

Quoted 2 lines

But for capture, after the driver has fetched the data, it will stopped and no old data will be fetched.

A similar problem here. If you extend the API, don't build up only from the bottom. Rather define from a higher POV.

Quoted 8 lines

If calling the drain trigger for capture stream doesn't make *any* sense for all hardware, then we shouldn't add it. This would be the only reason not to do it. What about empty streams...?

That being said, we need to define the proper abstraction, what this trigger does for which purpose, not specific to a certain hardware.
That is, the requirement goes up from your hardware level while the design comes down from the abstraction level.

Right. I will include the capture.

My patch will be like:

(quoted diff) +6 -0

@@ -1566,6 +1567,11 @@ static int snd_pcm_do_drain_init(struct snd_pcm_substream *substream, int state)
 			snd_pcm_post_stop(substream, new_state);
 		}
 	}
+
+	if (substream->runtime->trigger_master == substream &&
+		(substream->runtime->hw.info & SNDRV_PCM_INFO_DRAIN_TRIGGER))
+		return substream->ops->trigger(substream,
+				SNDRV_PCM_TRIGGER_DRAIN);
+
 	return 0;
 }

I moved the trigger at the end of the function. Or do you think we need trigger before snd_pcm_do_stop() for capture case based on your knowledge?

And in the snd_pcm_hw_refine():

(quoted diff) +2 -1

@@ -420,7 +420,8 @@ int snd_pcm_hw_refine(struct snd_pcm_substream *substream,
 
 	hw = &substream->runtime->hw;
 	if (!params->info) {
-		params->info = hw->info & ~SNDRV_PCM_INFO_FIFO_IN_FRAMES;
+		params->info = hw->info & ~SNDRV_PCM_INFO_FIFO_IN_FRAMES
+			& ~SNDRV_PCM_INFO_DRAIN_TRIGGER;
 		if (!hw_support_mmap(substream))
 			params->info &= ~(SNDRV_PCM_INFO_MMAP |
 					  SNDRV_PCM_INFO_MMAP_VALID);

Do you think whether it is OK?

-----Original Message-----
From: Takashi Iwai [mailto:[email protected]]
Sent: Thursday, December 25, 2014 6:11 PM
To: Yang, Libin
Cc: [email protected]; [email protected]; [email protected]; Jie, Yang
Subject: Re: [PATCH 1/2] [ALSA] add SNDRV_PCM_TRIGGER_DRAIN trigger

At Mon, 22 Dec 2014 07:42:38 +0000,

Review static int snd_pcm_do_drain_init(struct

Quoted 73 lines · Libin

Hi Takashi,

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

Besides, this patch will break many drivers. Many of them will spew errors if an unexpected trigger command is issued. So, you have to limit this trigger only to some drivers. A typical workaround is to add a new
SNDRV_PCM_INFO_* and issue the trigger only when the flag is set, as done for resume and pause operations.

Quoted 6 lines

Quoted 1 line

Yes, that's a good suggestion. Thanks for reminding me.

OTOH, the info flag will be exposed to user-space while this is a kernel-space- only information. So, in the code copying the info flag
(snd_pcm_hw_refine()) would need to drop this bit, too.

Thanks, I will drop this bit in snd_pcm_hw_refine().

Quoted 5 lines

Alternatively, we may add just a new flag in struct snd_pcm or struct snd_pcm_substream and let the driver setting it manually. This might need more changes in ASoC side, though, as the PCM creation isn't directly done by the driver.

Yes, it is a little complex for asoc if using this method. Not sure if it is OK we set the flag in snd_soc_ops->startup() each time when the function is called.

What do you think we just use the first method?

Quoted 28 lines

Quoted 5 lines

Quoted 5 lines

Also, do you want to see this trigger even if the stream isn't started before drain is issued? Also, why no need for this trigger for

capture streams?

Quoted 5 lines

In this case, the trigger action will be started. For the stream not started case, if I skip the trigger , I don't know where I can have the chance to trigger later. And I didn't find the suitable place to trigger my

code.

Quoted 9 lines

Put the trigger at the end of the function snd_pcm_do_drain_init() or in the function snd_pcm_post_drain_init() will be OK?

I didn't include the capture case because we found the capture currently don't need the trigger.

Our issue is the fw don't know when to stop fetching the data. For playback we need fill silence data when draining, otherwise the fw will get

the old data.

Quoted 2 lines

But for capture, after the driver has fetched the data, it will stopped and no old data will be fetched.

A similar problem here. If you extend the API, don't build up only from the bottom. Rather define from a higher POV.

Quoted 8 lines

If calling the drain trigger for capture stream doesn't make *any* sense for all hardware, then we shouldn't add it. This would be the only reason not to do it. What about empty streams...?

That being said, we need to define the proper abstraction, what this trigger does for which purpose, not specific to a certain hardware. That is, the requirement goes up from your hardware level while the design comes down from the abstraction level.

Right. I will include the capture.

My patch will be like:

(quoted diff) +0 -0

@@ -1566,6 +1567,11 @@ static int snd_pcm_do_drain_init(struct

snd_pcm_substream *substream, int state)

Review static int snd_pcm_do_drain_init(struct · line 1572

Quoted 6 lines

(quoted diff) +3 -0

 			snd_pcm_post_stop(substream, new_state);
 		}
 	}
+
+	if (substream->runtime->trigger_master == substream &&
+		(substream->runtime->hw.info &

SNDRV_PCM_INFO_DRAIN_TRIGGER))

Quoted 6 lines

(quoted diff) +3 -0

+		return substream->ops->trigger(substream,
+				SNDRV_PCM_TRIGGER_DRAIN);
+
 	return 0;
 }

Quoted 2 lines

I moved the trigger at the end of the function. Or do you think we need trigger before snd_pcm_do_stop() for capture case based on your

knowledge?

The place would be OK, but you can't call it unconditionally there. In some cases (e.g. XRUN), the trigger won't be called at all.
The trigger state change is a bit tricky, so be careful to track the state change there.

Review int snd_pcm_hw_refine(struct

Quoted 4 lines

And in the snd_pcm_hw_refine():

(quoted diff) +0 -0

@@ -420,7 +420,8 @@ int snd_pcm_hw_refine(struct

snd_pcm_substream

Review int snd_pcm_hw_refine(struct · line 424

Quoted 5 lines

(quoted diff) +0 -1

 *substream,
 
 	hw = &substream->runtime->hw;
 	if (!params->info) {
-		params->info = hw->info &

~SNDRV_PCM_INFO_FIFO_IN_FRAMES;

Review int snd_pcm_hw_refine(struct · line 424

Quoted 1 line

(quoted diff) +1 -0

+		params->info = hw->info &

~SNDRV_PCM_INFO_FIFO_IN_FRAMES

Quoted 6 lines

(quoted diff) +1 -0

+			& ~SNDRV_PCM_INFO_DRAIN_TRIGGER;
 		if (!hw_support_mmap(substream))
 			params->info &= ~(SNDRV_PCM_INFO_MMAP |

SNDRV_PCM_INFO_MMAP_VALID);

Do you think whether it is OK?

Yes.

thanks,

Takashi

Hi Takashi,

Regards,
Libin

Quoted 97 lines

-----Original Message-----
From: Takashi Iwai [mailto:[email protected]]
Sent: Thursday, December 25, 2014 6:11 PM
To: Yang, Libin
Cc: [email protected]; [email protected]; [email protected]; Jie, Yang
Subject: Re: [PATCH 1/2] [ALSA] add SNDRV_PCM_TRIGGER_DRAIN trigger

At Mon, 22 Dec 2014 07:42:38 +0000,

Review static int snd_pcm_do_drain_init(struct

Quoted 73 lines · Libin

Hi Takashi,

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

Besides, this patch will break many drivers. Many of them will spew errors if an unexpected trigger command is issued. So, you have to limit this trigger only to some drivers. A typical workaround is to add a new
SNDRV_PCM_INFO_* and issue the trigger only when the flag is set, as done for resume and pause operations.

Quoted 6 lines

Quoted 1 line

Yes, that's a good suggestion. Thanks for reminding me.

OTOH, the info flag will be exposed to user-space while this is a kernel-space- only information. So, in the code copying the info flag
(snd_pcm_hw_refine()) would need to drop this bit, too.

Thanks, I will drop this bit in snd_pcm_hw_refine().

Quoted 5 lines

Alternatively, we may add just a new flag in struct snd_pcm or struct snd_pcm_substream and let the driver setting it manually. This might need more changes in ASoC side, though, as the PCM creation isn't directly done by the driver.

Yes, it is a little complex for asoc if using this method. Not sure if it is OK we set the flag in snd_soc_ops->startup() each time when the function is called.

What do you think we just use the first method?

Quoted 28 lines

Quoted 5 lines

Quoted 5 lines

Also, do you want to see this trigger even if the stream isn't started before drain is issued? Also, why no need for this trigger for

capture streams?

Quoted 5 lines

In this case, the trigger action will be started. For the stream not started case, if I skip the trigger , I don't know where I can have the chance to trigger later. And I didn't find the suitable place to trigger my

code.

Quoted 9 lines

Put the trigger at the end of the function snd_pcm_do_drain_init() or in the function snd_pcm_post_drain_init() will be OK?

I didn't include the capture case because we found the capture currently don't need the trigger.

Our issue is the fw don't know when to stop fetching the data. For playback we need fill silence data when draining, otherwise the fw will get

the old data.

Quoted 2 lines

But for capture, after the driver has fetched the data, it will stopped and no old data will be fetched.

A similar problem here. If you extend the API, don't build up only from the bottom. Rather define from a higher POV.

Quoted 8 lines

If calling the drain trigger for capture stream doesn't make *any* sense for all hardware, then we shouldn't add it. This would be the only reason not to do it. What about empty streams...?

That being said, we need to define the proper abstraction, what this trigger does for which purpose, not specific to a certain hardware. That is, the requirement goes up from your hardware level while the design comes down from the abstraction level.

Right. I will include the capture.

My patch will be like:

(quoted diff) +0 -0

@@ -1566,6 +1567,11 @@ static int snd_pcm_do_drain_init(struct

snd_pcm_substream *substream, int state)

Review static int snd_pcm_do_drain_init(struct · line 1572

Quoted 6 lines

(quoted diff) +3 -0

 			snd_pcm_post_stop(substream, new_state);
 		}
 	}
+
+	if (substream->runtime->trigger_master == substream &&
+		(substream->runtime->hw.info &

SNDRV_PCM_INFO_DRAIN_TRIGGER))

Quoted 6 lines

(quoted diff) +3 -0

+		return substream->ops->trigger(substream,
+				SNDRV_PCM_TRIGGER_DRAIN);
+
 	return 0;
 }

Quoted 9 lines

Quoted 2 lines

I moved the trigger at the end of the function. Or do you think we need trigger before snd_pcm_do_stop() for capture case based on your

knowledge?

The place would be OK, but you can't call it unconditionally there. In some cases (e.g. XRUN), the trigger won't be called at all.
The trigger state change is a bit tricky, so be careful to track the state change there.

Right. What do you think the following code?

(quoted diff) +4 -0

@@ -1518,6 +1519,8 @@ static int snd_pcm_pre_drain_init(struct snd_pcm_substream *substream, int state
 static int snd_pcm_do_drain_init(struct snd_pcm_substream *substream, int state)
 {
 	struct snd_pcm_runtime *runtime = substream->runtime;
+	int trigger = 0;
+
 	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
 		switch (runtime->status->state) {
 		case SNDRV_PCM_STATE_PREPARED:

@@ -1525,10 +1528,12 @@ static int snd_pcm_do_drain_init(struct snd_pcm_substream *substream, int state)
 			if (! snd_pcm_playback_empty(substream)) {
 				snd_pcm_do_start(substream, SNDRV_PCM_STATE_DRAINING);
 				snd_pcm_post_start(substream, SNDRV_PCM_STATE_DRAINING);
+				trigger = 1;
 			}
 			break;
 		case SNDRV_PCM_STATE_RUNNING:
 			runtime->status->state = SNDRV_PCM_STATE_DRAINING;
+			trigger = 1;
 			break;
 		case SNDRV_PCM_STATE_XRUN:
 			runtime->status->state = SNDRV_PCM_STATE_SETUP;