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

[Takashi Iwai]

On Thu, 19 Nov 2020 03:51:19 +0100,
Gyeongtaek Lee wrote:
> 
> On Fri, 06 Nov 2020 09:58:24 +0100, Takashi Iwai wrote:
> >On Tue, 27 Oct 2020 02:57:25 +0100,
> >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 at samsung.com>
> >> Cc: stable at vger.kernel.org
> >
> >I personally find the approach is fine, but let's see what others
> >think.
> >
> >One remaining concern to me is about the setup of
> >use_pause_in_draining flag.  This is done by an explicit function call
> >after the snd_compr object initialization.  It's a bit uncommon style,
> >but OTOH I understand it from the current initialization code of
> >compress-offload API.
> Thanks for your kind review.
> 
> It's been almost 2 weeks.
> So, I think that there is no further comment for this patch.

I guess it's just overlooked.  Vinod?


Takashi

> Could this patch be applied?
> If so, should I resend this patch with new header like patch v3 or wait?
> 
> thanks again,
> 
> Gyeongtaek
> >
> >
> >thanks,
> >
> >Takashi
> >
> >> ---
> >>  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);
> >> +		if (!retval)
> >> +			stream->runtime->state = SNDRV_PCM_STATE_PAUSED;
> >> +		break;
> >> +	case SNDRV_PCM_STATE_DRAINING:
> >> +		if (!stream->device->use_pause_in_draining)
> >> +			return -EPERM;
> >> +		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;
> >> +		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
> >> 
> >> 
> >
>