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

[Takashi Iwai <tiwai@suse.de>]

On Thu, 08 Oct 2020 11:49:24 +0200,
Gyeongtaek Lee wrote:
> 
> 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

Well, it wasn't me; I'm not against the new state *iff* it would end
up with cleaner code.  Admittedly, the new state can be more
"consistent" regarding the state transition.  If we allow the PAUSE
state during DRAINING, it'll lead to multiple states after resuming
the pause.

> 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?

Yes, if we really add the PCM state, it's definitely needed.

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

If we introduce the new change, it must be safe to the old kernels,
too.  The problem is only about the compatibility of the user-space
program, not about the kernel.


HOWEVER: I'm still concerned by the addition of a new PCM state.
Jaroslav suggested two steps approach, (1) first add the state only in
the uapi header, then use (2) the new state actually.  But, this
doesn't help much, simply because the step 1 won't catch real bugs.

Even if we add a new state and change the SNDRV_PCM_STATE_LAST, I
guess most of code can be compiled fine.  So, at the step 1, no one
notices it and bothered, either.  But, at the step 2, you'll hit a
problem.

Adding a new state is something like to add a new color to the traffic
signal.  In some countries, the car turning right at a crossing
doesn't have to stop at a red signal.  Suppose that we want to control
it, and change the international rule by introducing a new color (say
magenta) signal to stop the car turning right.  That'll be a big
confusion because most drivers are trained for only red, green and
yellow signals.

Similarly, if we add a new PCM state, every program code that deals
with the PCM state may be confused by the new state.  It has to be
reviewed and corrected manually, because it's no syntax problem the
compiler may catch.


thanks,

Takashi


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