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