Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 08 Jul 2021 13:24:51 +0200
Message-ID: <s5ho8bd59q4.wl-tiwai@suse.de>
From:   Takashi Iwai <tiwai@suse.de>
To:     Robert Lee <lerobert@google.com>
Cc:     vkoul@kernel.org, perex@perex.cz, tiwai@suse.com,
        alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org,
        carterhsu@google.com, zxinhui@google.com, bubblefang@google.com
Subject: Re: [Patch v2] ALSA: compress: allow to leave draining state when pausing in draining
In-Reply-To: <20210708020815.3489365-1-lerobert@google.com>
References: <20210708020815.3489365-1-lerobert@google.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka)
 FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/25.3
 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Thu, 08 Jul 2021 04:08:15 +0200,
Robert Lee wrote:
> 
> When compress offload pauses in draining state, not all platforms
> need to keep in draining state. Some platforms may call drain or
> partial drain again when resume from pause in draining, so it needs
> to wake up from snd_compress_wait_for_drain() in this case.
> 
> Call API snd_compr_leave_draining_in_pause(), if the platform
> doesn't need to keep in draining state when pause in draining
> state.
> 
> Signed-off-by: Robert Lee <lerobert@google.com>
> ---
>  include/sound/compress_driver.h | 14 ++++++++++++++
>  sound/core/compress_offload.c   |  8 +++++++-
>  2 files changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h
> index 277087f635f3..e16524a93a14 100644
> --- a/include/sound/compress_driver.h
> +++ b/include/sound/compress_driver.h
> @@ -145,6 +145,7 @@ struct snd_compr_ops {
>   * @lock: device lock
>   * @device: device id
>   * @use_pause_in_draining: allow pause in draining, true when set
> + * @leave_draining_in_pause: leave draining state when pausing in draining
>   */
>  struct snd_compr {
>  	const char *name;
> @@ -156,6 +157,7 @@ struct snd_compr {
>  	struct mutex lock;
>  	int device;
>  	bool use_pause_in_draining;
> +	bool leave_draining_in_pause;
>  #ifdef CONFIG_SND_VERBOSE_PROCFS
>  	/* private: */
>  	char id[64];
> @@ -182,6 +184,18 @@ static inline void snd_compr_use_pause_in_draining(struct snd_compr_stream *subs
>  	substream->device->use_pause_in_draining = true;
>  }
>  
> +/**
> + * snd_compr_leave_draining_in_pause - Leave draining state when pause in draining
> + * @substream: compress substream to set
> + *
> + * In some platform, we need to leave draining state when we use pause in draining.
> + * Add API to allow leave draining state.
> + */
> +static inline void snd_compr_leave_draining_in_pause(struct snd_compr_stream *substream)
> +{
> +	substream->device->leave_draining_in_pause = 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 21ce4c056a92..c6e5c8f072d7 100644
> --- a/sound/core/compress_offload.c
> +++ b/sound/core/compress_offload.c
> @@ -719,8 +719,14 @@ static int snd_compr_pause(struct snd_compr_stream *stream)
>  		if (!stream->device->use_pause_in_draining)
>  			return -EPERM;
>  		retval = stream->ops->trigger(stream, SNDRV_PCM_TRIGGER_PAUSE_PUSH);
> -		if (!retval)
> +		if (!retval) {
> +			if (stream->device->leave_draining_in_pause) {
> +				stream->runtime->state = SNDRV_PCM_STATE_PAUSED;
> +				wake_up(&stream->runtime->sleep);
> +				break;
> +			}
>  			stream->pause_in_draining = true;
> +		}

Hrm, what actually happens with this new flag?  It changes the state
to PAUSED even if it's done during the draining.  Then user resumes
the pause via snd_compr_resume(), and now the state changes to
RUNNING.  OTOH, if the draining runs normally, it'll end up with
SETUP.

Even if the above is even designed behavior, it must be described
properly somewhere.  The state change is described in snd_compr_open()
comment, and the new behavior should be mentioned there as well.
(Admittedly, the previous hack for the pause-during-drain is also
missing and should have been mentioned there; but an excuse is that
the pause-during-drain doesn't change the state itself :)


thanks,

Takashi