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

[Gyeongtaek Lee]

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