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

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

[Gyeongtaek Lee]

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);
+		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

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

[Takashi Iwai]

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@samsung.com>
> Cc: stable@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,

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

RE: [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@samsung.com>
>> Cc: stable@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
>> 
>> 
>

Re: [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@samsung.com>
> >> Cc: stable@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
> >> 
> >> 
> >
> 

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

[Vinod Koul]

On 25-11-20, 09:51, Takashi Iwai wrote:
> 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@samsung.com>
> > >> Cc: stable@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?

Sorry indeed this seems to have slipped, this mostly looks okay to me
(saw some code style nits will reply on those)

I have todo in my list to fix use of PCM_STATE in compress, i will send
that later

-- 
~Vinod

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

[Vinod Koul]

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

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

[Gyeongtaek Lee]

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);
+		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