RE: [PATCH] ALSA: compress: allow pause and resume during draining

["Gyeongtaek Lee" <gt82.lee@samsung.com>]

On 10/06/20 11:57 PM, Pierre-Louis Bossart wrote:
>> The SM in kernel might be bit more convoluted so was wondering if we can
>> handle this in userland. The changelog for this patch says that for
>> test case was sending whole file, surely that is not an optimal approach.
>
>It's rather common to have to deal with very small files, even with PCM, 
>e.g. for notifications. It's actually a classic test case that exposes 
>design issues in drivers, where e.g. the last part of the notification 
>is not played.
>
>> Should we allow folks to send whole file to kernel and then issue
>> partial drain?
>
>I don't think there should be a conceptual limitation here. If the 
>userspace knows that the last part of the file is smaller than a 
>fragment it should be able to issue a drain (or partial drain if it's a 
>gapless solution).
>
>However now that I think of it, I am not sure what happens if the file 
>is smaller than a fragment. That may very well be a limitation in the 
>design.
>
Thanks for the comments.

Actually, problem can be occurred with big file also.
Application usually requests draining after sending last frame.
If user clicks pause button after draining, pause will be failed
and the file just be played until end.
If application stop and start playback for this case, 
start of last frame will be heard again because stop sets state to SETUP,
and write is needed to set the state to PREPARED for start.
If bitrate of the file is low, time stamp will be reversed and be heard weird.
I also hope this problem can be handled in userspace easily but I couldn't find a way for now.

I think that this is the time that I should share fixed patch following the comments to help the discussion.
Following opinions are added to the patch.
1. it's be much nicer to have a new state - Takashi
2. We can add this state to asound.h so the user space can be updated. - Jaroslav
3. don't forget to increase the SNDRV_COMPRESS_VERSION - Jaroslav

I'm bit wondering whether it is good to increase SNDRV_COMPRESS_VERSION
with a change in asound.h not in compress_offload.h.
Should I increase SNDRV_PCM_VERSION also?

And what happened if I request back-porting a patch which changes VERSION to stables?

Below is the patch just to help discussion.
I'll resend the fixed patch after this discussion is completed.

With a stream with low bitrate, user can't pause or resume the stream
near the end of the stream because current ALSA doesn't allow it.
If the stream has very low bitrate enough to store whole stream into
the buffer, user can't do anything except stop the stream and then
restart it from the first.
If pause, resume are allowed during draining, user experience can be
enhanced.

The version is increased due to new behavior.

Signed-off-by: Gyeongtaek Lee <gt82.lee@samsung.com>
Cc: stable@vger.kernel.org
---
 include/uapi/sound/asound.h           |  5 +--
 include/uapi/sound/compress_offload.h |  2 +-
 sound/core/compress_offload.c         | 47 ++++++++++++++++++++++-----
 3 files changed, 42 insertions(+), 12 deletions(-)

diff --git a/include/uapi/sound/asound.h b/include/uapi/sound/asound.h
index 535a7229e1d9..499b364974ec 100644
--- a/include/uapi/sound/asound.h
+++ b/include/uapi/sound/asound.h
@@ -315,8 +315,9 @@ typedef int __bitwise snd_pcm_state_t;
 #define	SNDRV_PCM_STATE_XRUN		((__force snd_pcm_state_t) 4) /* stream reached an xrun */
 #define	SNDRV_PCM_STATE_DRAINING	((__force snd_pcm_state_t) 5) /* stream is draining */
 #define	SNDRV_PCM_STATE_PAUSED		((__force snd_pcm_state_t) 6) /* stream is paused */
-#define	SNDRV_PCM_STATE_SUSPENDED	((__force snd_pcm_state_t) 7) /* hardware is suspended */
-#define	SNDRV_PCM_STATE_DISCONNECTED	((__force snd_pcm_state_t) 8) /* hardware is disconnected */
+#define	SNDRV_PCM_STATE_DRAINING_PAUSED	((__force snd_pcm_state_t) 7) /* stream is paused during draining*/
+#define	SNDRV_PCM_STATE_SUSPENDED	((__force snd_pcm_state_t) 8) /* hardware is suspended */
+#define	SNDRV_PCM_STATE_DISCONNECTED	((__force snd_pcm_state_t) 9) /* hardware is disconnected */
 #define	SNDRV_PCM_STATE_LAST		SNDRV_PCM_STATE_DISCONNECTED
 
 enum {
diff --git a/include/uapi/sound/compress_offload.h b/include/uapi/sound/compress_offload.h
index 7184265c0b0d..46fdf50d5c00 100644
--- a/include/uapi/sound/compress_offload.h
+++ b/include/uapi/sound/compress_offload.h
@@ -31,7 +31,7 @@
 #include <sound/compress_params.h>
 
 
-#define SNDRV_COMPRESS_VERSION SNDRV_PROTOCOL_VERSION(0, 2, 0)
+#define SNDRV_COMPRESS_VERSION SNDRV_PROTOCOL_VERSION(0, 2, 1)
 /**
  * struct snd_compressed_buffer - compressed buffer
  * @fragment_size: size of buffer fragment in bytes
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index 0e53f6f31916..58fbe0d99431 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -151,6 +151,7 @@ static int snd_compr_free(struct inode *inode, struct file *f)
 	case SNDRV_PCM_STATE_RUNNING:
 	case SNDRV_PCM_STATE_DRAINING:
 	case SNDRV_PCM_STATE_PAUSED:
+	case SNDRV_PCM_STATE_DRAINING_PAUSED:
 		data->stream.ops->trigger(&data->stream, SNDRV_PCM_TRIGGER_STOP);
 		break;
 	default:
@@ -431,6 +432,7 @@ static __poll_t snd_compr_poll(struct file *f, poll_table *wait)
 	case SNDRV_PCM_STATE_RUNNING:
 	case SNDRV_PCM_STATE_PREPARED:
 	case SNDRV_PCM_STATE_PAUSED:
+	case SNDRV_PCM_STATE_DRAINING_PAUSED:
 		if (avail >= stream->runtime->fragment_size)
 			retval = snd_compr_get_poll(stream);
 		break;
@@ -708,11 +710,23 @@ static int snd_compr_pause(struct snd_compr_stream *stream)
 {
 	int retval;
 
-	if (stream->runtime->state != SNDRV_PCM_STATE_RUNNING)
+	switch (stream->runtime->state) {
+	case SNDRV_PCM_STATE_RUNNING:
+		retval = stream->ops->trigger(stream,
+			SNDRV_PCM_TRIGGER_PAUSE_PUSH);
+		if (!retval)
+			stream->runtime->state = SNDRV_PCM_STATE_PAUSED;
+		break;
+	case SNDRV_PCM_STATE_DRAINING:
+		retval = stream->ops->trigger(stream,
+			SNDRV_PCM_TRIGGER_PAUSE_PUSH);
+		if (!retval)
+			stream->runtime->state =
+				SNDRV_PCM_STATE_DRAINING_PAUSED;
+		break;
+	default:
 		return -EPERM;
-	retval = stream->ops->trigger(stream, SNDRV_PCM_TRIGGER_PAUSE_PUSH);
-	if (!retval)
-		stream->runtime->state = SNDRV_PCM_STATE_PAUSED;
+	}
 	return retval;
 }
 
@@ -720,11 +734,22 @@ static int snd_compr_resume(struct snd_compr_stream *stream)
 {
 	int retval;
 
-	if (stream->runtime->state != SNDRV_PCM_STATE_PAUSED)
+	switch (stream->runtime->state) {
+	case SNDRV_PCM_STATE_PAUSED:
+		retval = stream->ops->trigger(stream,
+			SNDRV_PCM_TRIGGER_PAUSE_RELEASE);
+		if (!retval)
+			stream->runtime->state = SNDRV_PCM_STATE_RUNNING;
+		break;
+	case SNDRV_PCM_STATE_DRAINING_PAUSED:
+		retval = stream->ops->trigger(stream,
+			SNDRV_PCM_TRIGGER_PAUSE_RELEASE);
+		if (!retval)
+			stream->runtime->state = SNDRV_PCM_STATE_DRAINING;
+		break;
+	default:
 		return -EPERM;
-	retval = stream->ops->trigger(stream, SNDRV_PCM_TRIGGER_PAUSE_RELEASE);
-	if (!retval)
-		stream->runtime->state = SNDRV_PCM_STATE_RUNNING;
+	}
 	return retval;
 }
 
@@ -835,7 +860,9 @@ static int snd_compress_wait_for_drain(struct snd_compr_stream *stream)
 	 */
 
 	ret = wait_event_interruptible(stream->runtime->sleep,
-			(stream->runtime->state != SNDRV_PCM_STATE_DRAINING));
+			(stream->runtime->state != SNDRV_PCM_STATE_DRAINING) &&
+			(stream->runtime->state !=
+				SNDRV_PCM_STATE_DRAINING_PAUSED));
 	if (ret == -ERESTARTSYS)
 		pr_debug("wait aborted by a signal\n");
 	else if (ret)
@@ -857,6 +884,7 @@ static int snd_compr_drain(struct snd_compr_stream *stream)
 	case SNDRV_PCM_STATE_SETUP:
 	case SNDRV_PCM_STATE_PREPARED:
 	case SNDRV_PCM_STATE_PAUSED:
+	case SNDRV_PCM_STATE_DRAINING_PAUSED:
 		return -EPERM;
 	case SNDRV_PCM_STATE_XRUN:
 		return -EPIPE;
@@ -909,6 +937,7 @@ static int snd_compr_partial_drain(struct snd_compr_stream *stream)
 	case SNDRV_PCM_STATE_SETUP:
 	case SNDRV_PCM_STATE_PREPARED:
 	case SNDRV_PCM_STATE_PAUSED:
+	case SNDRV_PCM_STATE_DRAINING_PAUSED:
 		return -EPERM;
 	case SNDRV_PCM_STATE_XRUN:
 		return -EPIPE;
-- 
2.21.0