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@xxxxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
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
>
>
