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