* [PATCH v3 1/3] ALSA: compress: document the compress audio state machine
2020-06-25 15:46 [PATCH v3 0/3] ALSA: compress: Document stream states and fix gapless SM Vinod Koul
@ 2020-06-25 15:46 ` Vinod Koul
2020-06-26 10:45 ` Charles Keepax
2020-06-25 15:46 ` [PATCH v3 2/3] ALSA: compress: document the compress gapless " Vinod Koul
2020-06-25 15:46 ` [PATCH v3 3/3] ALSA: compress: fix partial_drain completion state Vinod Koul
2 siblings, 1 reply; 7+ messages in thread
From: Vinod Koul @ 2020-06-25 15:46 UTC (permalink / raw)
To: Takashi Iwai, Jaroslav Kysela
Cc: Vinod Koul, Srinivas Kandagatla, Pierre-Louis Bossart,
Charles Keepax, alsa-devel, linux-kernel
So we had some discussions of the stream states, so I thought it is a
good idea to document the state transitions, so add it documentation
Signed-off-by: Vinod Koul <vkoul@kernel.org>
---
.../sound/designs/compress-offload.rst | 51 +++++++++++++++++++
1 file changed, 51 insertions(+)
diff --git a/Documentation/sound/designs/compress-offload.rst b/Documentation/sound/designs/compress-offload.rst
index ad4bfbdacc83..b6e9025ae105 100644
--- a/Documentation/sound/designs/compress-offload.rst
+++ b/Documentation/sound/designs/compress-offload.rst
@@ -151,6 +151,57 @@ Modifications include:
- Addition of encoding options when required (derived from OpenMAX IL)
- Addition of rateControlSupported (missing in OpenMAX AL)
+State Machine
+=============
+
+The compressed audio stream state machine is described below ::
+
+ +----------+
+ | |
+ | OPEN |
+ | |
+ +----------+
+ |
+ |
+ | compr_set_params()
+ |
+ v
+ compr_free() +----------+
+ +------------------------------------| |
+ | | SETUP |
+ | +------------------------>| |<-------------------------+
+ | | compr_drain_notify() +----------+ |
+ | | or | |
+ | | compr_stop() | |
+ | | | compr_write() |
+ | | | |
+ | | v |
+ | | +----------+ |
+ | | | | compr_free() |
+ | | | PREPARE |---------------> A |
+ | | | | |
+ | | +----------+ |
+ | | | |
+ | | | |
+ | | | compr_start() |
+ | | | |
+ | | v |
+ | +----------+ +----------+ |
+ | | | compr_drain() | | compr_stop() |
+ | | DRAIN |<-------------------| RUNNING |--------------------------+
+ | | | | | |
+ | +----------+ +----------+ |
+ | | ^ |
+ | A | | |
+ | | compr_pause() | | compr_resume() |
+ | | | | |
+ | v v | |
+ | +----------+ +----------+ |
+ | | | | | compr_stop() |
+ +--->| FREE | | PAUSE |---------------------------+
+ | | | |
+ +----------+ +----------+
+
Gapless Playback
================
--
2.26.2
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v3 1/3] ALSA: compress: document the compress audio state machine
2020-06-25 15:46 ` [PATCH v3 1/3] ALSA: compress: document the compress audio state machine Vinod Koul
@ 2020-06-26 10:45 ` Charles Keepax
0 siblings, 0 replies; 7+ messages in thread
From: Charles Keepax @ 2020-06-26 10:45 UTC (permalink / raw)
To: Vinod Koul
Cc: Takashi Iwai, Jaroslav Kysela, Srinivas Kandagatla,
Pierre-Louis Bossart, alsa-devel, linux-kernel
On Thu, Jun 25, 2020 at 09:16:49PM +0530, Vinod Koul wrote:
> So we had some discussions of the stream states, so I thought it is a
> good idea to document the state transitions, so add it documentation
>
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> ---
Reviewed-by: Charles Keepax <ckeepax@opensource.cirrus.com>
Thanks,
Charles
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v3 2/3] ALSA: compress: document the compress gapless audio state machine
2020-06-25 15:46 [PATCH v3 0/3] ALSA: compress: Document stream states and fix gapless SM Vinod Koul
2020-06-25 15:46 ` [PATCH v3 1/3] ALSA: compress: document the compress audio state machine Vinod Koul
@ 2020-06-25 15:46 ` Vinod Koul
2020-06-25 15:46 ` [PATCH v3 3/3] ALSA: compress: fix partial_drain completion state Vinod Koul
2 siblings, 0 replies; 7+ messages in thread
From: Vinod Koul @ 2020-06-25 15:46 UTC (permalink / raw)
To: Takashi Iwai, Jaroslav Kysela
Cc: Vinod Koul, Srinivas Kandagatla, Pierre-Louis Bossart,
Charles Keepax, alsa-devel, linux-kernel
Also documented the galpess transitions. Please note that these are not
really stream states, but show how the stream steps in gapless mode
Signed-off-by: Vinod Koul <vkoul@kernel.org>
---
.../sound/designs/compress-offload.rst | 32 +++++++++++++++++++
1 file changed, 32 insertions(+)
diff --git a/Documentation/sound/designs/compress-offload.rst b/Documentation/sound/designs/compress-offload.rst
index b6e9025ae105..82950dbbc4a9 100644
--- a/Documentation/sound/designs/compress-offload.rst
+++ b/Documentation/sound/designs/compress-offload.rst
@@ -250,6 +250,38 @@ Sequence flow for gapless would be:
(note: order for partial_drain and write for next track can be reversed as well)
+Gapless Playback SM
+===================
+
+For Gapless, we move from running state to partial drain and back, along
+with setting of meta_data and signalling for next track ::
+
+
+ +----------+
+ compr_drain_notify() | |
+ +------------------------>| RUNNING |
+ | | |
+ | +----------+
+ | |
+ | |
+ | | compr_next_track()
+ | |
+ | v
+ | +----------+
+ | | |
+ | |NEXT_TRACK|
+ | | |
+ | +----------+
+ | |
+ | |
+ | | compr_partial_drain()
+ | |
+ | v
+ | +----------+
+ | | |
+ +------------------------ | PARTIAL_ |
+ | DRAIN |
+ +----------+
Not supported
=============
--
2.26.2
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v3 3/3] ALSA: compress: fix partial_drain completion state
2020-06-25 15:46 [PATCH v3 0/3] ALSA: compress: Document stream states and fix gapless SM Vinod Koul
2020-06-25 15:46 ` [PATCH v3 1/3] ALSA: compress: document the compress audio state machine Vinod Koul
2020-06-25 15:46 ` [PATCH v3 2/3] ALSA: compress: document the compress gapless " Vinod Koul
@ 2020-06-25 15:46 ` Vinod Koul
2020-06-26 10:35 ` Charles Keepax
2 siblings, 1 reply; 7+ messages in thread
From: Vinod Koul @ 2020-06-25 15:46 UTC (permalink / raw)
To: Takashi Iwai, Jaroslav Kysela
Cc: Vinod Koul, Srinivas Kandagatla, Pierre-Louis Bossart,
Charles Keepax, alsa-devel, linux-kernel
On partial_drain completion we should be in SNDRV_PCM_STATE_RUNNING
state, so set that for partially draining streams in
snd_compr_drain_notify() and use a flag for partially draining streams
While at it, add locks for stream state change in
snd_compr_drain_notify() as well.
Fixes: f44f2a5417b2 ("ALSA: compress: fix drain calls blocking other compress functions (v6)")
Reviewed-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Tested-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Signed-off-by: Vinod Koul <vkoul@kernel.org>
---
include/sound/compress_driver.h | 12 +++++++++++-
sound/core/compress_offload.c | 4 ++++
2 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h
index 3df8d8c90191..93a5897201ea 100644
--- a/include/sound/compress_driver.h
+++ b/include/sound/compress_driver.h
@@ -66,6 +66,7 @@ struct snd_compr_runtime {
* @direction: stream direction, playback/recording
* @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
* @private_data: pointer to DSP private data
* @dma_buffer: allocated buffer if any
*/
@@ -78,6 +79,7 @@ struct snd_compr_stream {
enum snd_compr_direction direction;
bool metadata_set;
bool next_track;
+ bool partial_drain;
void *private_data;
struct snd_dma_buffer dma_buffer;
};
@@ -187,7 +189,15 @@ static inline void snd_compr_drain_notify(struct snd_compr_stream *stream)
if (snd_BUG_ON(!stream))
return;
- stream->runtime->state = SNDRV_PCM_STATE_SETUP;
+ mutex_lock(&stream->device->lock);
+ /* for partial_drain case we are back to running state on success */
+ if (stream->partial_drain) {
+ stream->runtime->state = SNDRV_PCM_STATE_RUNNING;
+ stream->partial_drain = false; /* clear this flag as well */
+ } else {
+ stream->runtime->state = SNDRV_PCM_STATE_SETUP;
+ }
+ mutex_unlock(&stream->device->lock);
wake_up(&stream->runtime->sleep);
}
diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
index e618580feac4..1c4b2cf450a0 100644
--- a/sound/core/compress_offload.c
+++ b/sound/core/compress_offload.c
@@ -803,6 +803,9 @@ static int snd_compr_stop(struct snd_compr_stream *stream)
retval = stream->ops->trigger(stream, SNDRV_PCM_TRIGGER_STOP);
if (!retval) {
+ /* clear flags and stop any drain wait */
+ stream->partial_drain = false;
+ stream->metadata_set = false;
snd_compr_drain_notify(stream);
stream->runtime->total_bytes_available = 0;
stream->runtime->total_bytes_transferred = 0;
@@ -960,6 +963,7 @@ static int snd_compr_partial_drain(struct snd_compr_stream *stream)
if (stream->next_track == false)
return -EPERM;
+ stream->partial_drain = true;
retval = stream->ops->trigger(stream, SND_COMPR_TRIGGER_PARTIAL_DRAIN);
if (retval) {
pr_debug("Partial drain returned failure\n");
--
2.26.2
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v3 3/3] ALSA: compress: fix partial_drain completion state
2020-06-25 15:46 ` [PATCH v3 3/3] ALSA: compress: fix partial_drain completion state Vinod Koul
@ 2020-06-26 10:35 ` Charles Keepax
2020-06-29 5:16 ` Vinod Koul
0 siblings, 1 reply; 7+ messages in thread
From: Charles Keepax @ 2020-06-26 10:35 UTC (permalink / raw)
To: Vinod Koul
Cc: Takashi Iwai, Jaroslav Kysela, Srinivas Kandagatla,
Pierre-Louis Bossart, alsa-devel, linux-kernel
On Thu, Jun 25, 2020 at 09:16:51PM +0530, Vinod Koul wrote:
> On partial_drain completion we should be in SNDRV_PCM_STATE_RUNNING
> state, so set that for partially draining streams in
> snd_compr_drain_notify() and use a flag for partially draining streams
>
> While at it, add locks for stream state change in
> snd_compr_drain_notify() as well.
>
> Fixes: f44f2a5417b2 ("ALSA: compress: fix drain calls blocking other compress functions (v6)")
> Reviewed-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> Tested-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> ---
> @@ -187,7 +189,15 @@ static inline void snd_compr_drain_notify(struct snd_compr_stream *stream)
> if (snd_BUG_ON(!stream))
> return;
>
> - stream->runtime->state = SNDRV_PCM_STATE_SETUP;
> + mutex_lock(&stream->device->lock);
> + /* for partial_drain case we are back to running state on success */
> + if (stream->partial_drain) {
> + stream->runtime->state = SNDRV_PCM_STATE_RUNNING;
> + stream->partial_drain = false; /* clear this flag as well */
> + } else {
> + stream->runtime->state = SNDRV_PCM_STATE_SETUP;
> + }
> + mutex_unlock(&stream->device->lock);
You have added locking here in snd_compr_drain_notify but....
>
> wake_up(&stream->runtime->sleep);
> }
> diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
> index e618580feac4..1c4b2cf450a0 100644
> --- a/sound/core/compress_offload.c
> +++ b/sound/core/compress_offload.c
> @@ -803,6 +803,9 @@ static int snd_compr_stop(struct snd_compr_stream *stream)
>
> retval = stream->ops->trigger(stream, SNDRV_PCM_TRIGGER_STOP);
> if (!retval) {
> + /* clear flags and stop any drain wait */
> + stream->partial_drain = false;
> + stream->metadata_set = false;
> snd_compr_drain_notify(stream);
that can be called from snd_compr_stop here which is already
holding the lock resulting in deadlock.
Thanks,
Charles
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3 3/3] ALSA: compress: fix partial_drain completion state
2020-06-26 10:35 ` Charles Keepax
@ 2020-06-29 5:16 ` Vinod Koul
0 siblings, 0 replies; 7+ messages in thread
From: Vinod Koul @ 2020-06-29 5:16 UTC (permalink / raw)
To: Charles Keepax
Cc: Takashi Iwai, Jaroslav Kysela, Srinivas Kandagatla,
Pierre-Louis Bossart, alsa-devel, linux-kernel
On 26-06-20, 10:35, Charles Keepax wrote:
> > - stream->runtime->state = SNDRV_PCM_STATE_SETUP;
> > + mutex_lock(&stream->device->lock);
> > + /* for partial_drain case we are back to running state on success */
> > + if (stream->partial_drain) {
> > + stream->runtime->state = SNDRV_PCM_STATE_RUNNING;
> > + stream->partial_drain = false; /* clear this flag as well */
> > + } else {
> > + stream->runtime->state = SNDRV_PCM_STATE_SETUP;
> > + }
> > + mutex_unlock(&stream->device->lock);
>
> You have added locking here in snd_compr_drain_notify but....
> >
> > wake_up(&stream->runtime->sleep);
> > }
> > diff --git a/sound/core/compress_offload.c b/sound/core/compress_offload.c
> > index e618580feac4..1c4b2cf450a0 100644
> > --- a/sound/core/compress_offload.c
> > +++ b/sound/core/compress_offload.c
> > @@ -803,6 +803,9 @@ static int snd_compr_stop(struct snd_compr_stream *stream)
> >
> > retval = stream->ops->trigger(stream, SNDRV_PCM_TRIGGER_STOP);
> > if (!retval) {
> > + /* clear flags and stop any drain wait */
> > + stream->partial_drain = false;
> > + stream->metadata_set = false;
> > snd_compr_drain_notify(stream);
>
> that can be called from snd_compr_stop here which is already
> holding the lock resulting in deadlock.
Thanks Charles, right somehow my testing missed this, have verified that
it is the case.
I will remove the locks here, and we should add a comment to note this..
Thanks
--
~Vinod
^ permalink raw reply [flat|nested] 7+ messages in thread