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

[Vinod Koul <vkoul@kernel.org>]

On 27-10-20, 10:57, Gyeongtaek Lee wrote:
> 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 because most of applications call draining
> after sending last frame to the kernel.
> If pause, resume are allowed during draining, user experience can be
> enhanced.
> To prevent malfunction in HW drivers which don't support pause
> during draining, pause during draining will only work if HW driver
> enable this feature explicitly by calling
> snd_compr_use_pause_in_draining().
> 
> Signed-off-by: Gyeongtaek Lee <gt82.lee@samsung.com>
> Cc: stable@vger.kernel.org
> ---
>  include/sound/compress_driver.h | 17 +++++++++++++
>  sound/core/compress_offload.c   | 44 +++++++++++++++++++++++++++------
>  2 files changed, 53 insertions(+), 8 deletions(-)
> 
> diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h
> index 70cbc5095e72..5a0d6737de5e 100644
> --- a/include/sound/compress_driver.h
> +++ b/include/sound/compress_driver.h
> @@ -67,6 +67,7 @@ struct snd_compr_runtime {
>   * @metadata_set: metadata set flag, true when set
>   * @next_track: has userspace signal next track transition, true when set
>   * @partial_drain: undergoing partial_drain for stream, true when set
> + * @pause_in_draining: paused during draining state, true when set
>   * @private_data: pointer to DSP private data
>   * @dma_buffer: allocated buffer if any
>   */
> @@ -80,6 +81,7 @@ struct snd_compr_stream {
>  	bool metadata_set;
>  	bool next_track;
>  	bool partial_drain;
> +	bool pause_in_draining;
>  	void *private_data;
>  	struct snd_dma_buffer dma_buffer;
>  };
> @@ -142,6 +144,7 @@ struct snd_compr_ops {
>   * @direction: Playback or capture direction
>   * @lock: device lock
>   * @device: device id
> + * @use_pause_in_draining: allow pause in draining, true when set
>   */
>  struct snd_compr {
>  	const char *name;
> @@ -152,6 +155,7 @@ struct snd_compr {
>  	unsigned int direction;
>  	struct mutex lock;
>  	int device;
> +	bool use_pause_in_draining;
>  #ifdef CONFIG_SND_VERBOSE_PROCFS
>  	/* private: */
>  	char id[64];
> @@ -166,6 +170,19 @@ int snd_compress_deregister(struct snd_compr *device);
>  int snd_compress_new(struct snd_card *card, int device,
>  			int type, const char *id, struct snd_compr *compr);
>  
> +/**
> + * snd_compr_use_pause_in_draining - Allow pause and resume in draining state
> + * @substream: compress substream to set
> + *
> + * Allow pause and resume in draining state.
> + * Only HW driver supports this transition can call this API.
> + */
> +static inline void snd_compr_use_pause_in_draining(
> +					struct snd_compr_stream *substream)
> +{
> +	substream->device->use_pause_in_draining = true;
> +}
> +
>  /* dsp driver callback apis
>   * For playback: driver should call snd_compress_fragment_elapsed() to let the
>   * framework know that a fragment has been consumed from the ring buffer
> diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
> index 0e53f6f31916..a071485383ed 100644
> --- a/sound/core/compress_offload.c
> +++ b/sound/core/compress_offload.c
> @@ -708,11 +708,24 @@ 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);

this seems to fit single line with new 100char limit and makes it a
better read, can we please do that here and few places below .. The line
split is making it look bit ugly IMO

> +		if (!retval)
> +			stream->runtime->state = SNDRV_PCM_STATE_PAUSED;
> +		break;
> +	case SNDRV_PCM_STATE_DRAINING:
> +		if (!stream->device->use_pause_in_draining)
> +			return -EPERM;

I am expecting patches to tinycompress to handle pause while drain. Esp
this case..

> +		retval = stream->ops->trigger(stream,
> +			SNDRV_PCM_TRIGGER_PAUSE_PUSH);
> +		if (!retval)
> +			stream->pause_in_draining = true;
> +		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 +733,25 @@ 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:
> +		if (!stream->device->use_pause_in_draining ||
> +		    !stream->pause_in_draining)
> +			return -EPERM;

does this condition make sense for resume part..? We have already
checked for this while going into pause. I am not expecting drain state
to change while we are paused :)

> +		retval = stream->ops->trigger(stream,
> +			SNDRV_PCM_TRIGGER_PAUSE_RELEASE);
> +		if (!retval)
> +			stream->pause_in_draining = false;
> +		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;
>  }
>  
> @@ -767,6 +794,7 @@ static int snd_compr_stop(struct snd_compr_stream *stream)
>  		/* clear flags and stop any drain wait */
>  		stream->partial_drain = false;
>  		stream->metadata_set = false;
> +		stream->pause_in_draining = false;
>  		snd_compr_drain_notify(stream);
>  		stream->runtime->total_bytes_available = 0;
>  		stream->runtime->total_bytes_transferred = 0;
> -- 
> 2.21.0
> 

-- 
~Vinod