* [PATCH 0/3] ALSA: compress: Document stream states and fix gaplless SM
@ 2020-06-19 4:54 Vinod Koul
2020-06-19 4:54 ` [PATCH 1/3] ALSA: compress: document the compress audio state machine Vinod Koul
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Vinod Koul @ 2020-06-19 4:54 UTC (permalink / raw)
To: Takashi Iwai, Jaroslav Kysela
Cc: Vinod Koul, Pierre-Louis Bossart, Charles Keepax, alsa-devel,
linux-kernel
Srini found issue with gapless implementation which prompted to look deeper
into SM for compressed stream.
So documenting SM was first step, so first two patches add that. Last patch
fixes the issue by keeping track on partial_drain and then moving state to
'running' in snd_compr_drain_notify() for partial_drain case on success.
While at it, noticed snd_compr_drain_notify() is lockless state change, so
fixed that as well.
I have tested this on Dragon board RB3, compressed audio works out of the
box on that platform and Srini will send driver and fcplay patches for
gapless soon.
Vinod Koul (3):
ALSA: compress: document the compress audio state machine
ALSA: compress: document the compress gapless audio state machine
ALSA: compress: fix partial_drain completion state
.../sound/designs/compress-offload.rst | 84 +++++++++++++++++++
include/sound/compress_driver.h | 12 ++-
sound/core/compress_offload.c | 4 +
3 files changed, 99 insertions(+), 1 deletion(-)
--
2.26.2
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/3] ALSA: compress: document the compress audio state machine
2020-06-19 4:54 [PATCH 0/3] ALSA: compress: Document stream states and fix gaplless SM Vinod Koul
@ 2020-06-19 4:54 ` Vinod Koul
2020-06-19 9:29 ` Srinivas Kandagatla
2020-06-19 14:22 ` Pierre-Louis Bossart
2020-06-19 4:54 ` [PATCH 2/3] ALSA: compress: document the compress gapless " Vinod Koul
2020-06-19 4:54 ` [PATCH 3/3] ALSA: compress: fix partial_drain completion state Vinod Koul
2 siblings, 2 replies; 12+ messages in thread
From: Vinod Koul @ 2020-06-19 4:54 UTC (permalink / raw)
To: Takashi Iwai, Jaroslav Kysela
Cc: Vinod Koul, 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 | 52 +++++++++++++++++++
1 file changed, 52 insertions(+)
diff --git a/Documentation/sound/designs/compress-offload.rst b/Documentation/sound/designs/compress-offload.rst
index ad4bfbdacc83..7292717c43bf 100644
--- a/Documentation/sound/designs/compress-offload.rst
+++ b/Documentation/sound/designs/compress-offload.rst
@@ -151,6 +151,58 @@ 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_drain_notify() | |
+ +------------------------>| SETUP |
+ | | |
+ | +----------+
+ | |
+ | |
+ | | compr_write()
+ | |
+ | V
+ | +----------+
+ | | |
+ | | PREPARE |
+ | | |
+ | +----------+
+ | |
+ | |
+ | | compr_start()
+ | |
+ | V
+ +----------+ +----------+ compr_pause() +----------+
+ | | | |----------------------->| |
+ | DRAIN |<-------------------| RUNNING | | PAUSE |
+ | | | |<-----------------------| |
+ +----------+ +----------+ compr_resume() +----------+
+ | |
+ | |
+ | | compr_free()
+ | |
+ | V
+ | +----------+
+ | compr_free() | |
+ +------------------------>| |
+ | STOP |
+ | |
+ +----------+
+
Gapless Playback
================
--
2.26.2
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/3] ALSA: compress: document the compress gapless audio state machine
2020-06-19 4:54 [PATCH 0/3] ALSA: compress: Document stream states and fix gaplless SM Vinod Koul
2020-06-19 4:54 ` [PATCH 1/3] ALSA: compress: document the compress audio state machine Vinod Koul
@ 2020-06-19 4:54 ` Vinod Koul
2020-06-19 14:27 ` Pierre-Louis Bossart
2020-06-19 4:54 ` [PATCH 3/3] ALSA: compress: fix partial_drain completion state Vinod Koul
2 siblings, 1 reply; 12+ messages in thread
From: Vinod Koul @ 2020-06-19 4:54 UTC (permalink / raw)
To: Takashi Iwai, Jaroslav Kysela
Cc: Vinod Koul, 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 7292717c43bf..3e241530e173 100644
--- a/Documentation/sound/designs/compress-offload.rst
+++ b/Documentation/sound/designs/compress-offload.rst
@@ -251,6 +251,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] 12+ messages in thread
* [PATCH 3/3] ALSA: compress: fix partial_drain completion state
2020-06-19 4:54 [PATCH 0/3] ALSA: compress: Document stream states and fix gaplless SM Vinod Koul
2020-06-19 4:54 ` [PATCH 1/3] ALSA: compress: document the compress audio state machine Vinod Koul
2020-06-19 4:54 ` [PATCH 2/3] ALSA: compress: document the compress gapless " Vinod Koul
@ 2020-06-19 4:54 ` Vinod Koul
2020-06-19 9:13 ` Srinivas Kandagatla
2 siblings, 1 reply; 12+ messages in thread
From: Vinod Koul @ 2020-06-19 4:54 UTC (permalink / raw)
To: Takashi Iwai, Jaroslav Kysela
Cc: Vinod Koul, Pierre-Louis Bossart, Charles Keepax, alsa-devel,
linux-kernel, Srinivas Kandagatla
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)")
Reported-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] 12+ messages in thread
* Re: [PATCH 3/3] ALSA: compress: fix partial_drain completion state
2020-06-19 4:54 ` [PATCH 3/3] ALSA: compress: fix partial_drain completion state Vinod Koul
@ 2020-06-19 9:13 ` Srinivas Kandagatla
2020-06-22 5:35 ` Vinod Koul
0 siblings, 1 reply; 12+ messages in thread
From: Srinivas Kandagatla @ 2020-06-19 9:13 UTC (permalink / raw)
To: Vinod Koul, Takashi Iwai, Jaroslav Kysela
Cc: Pierre-Louis Bossart, Charles Keepax, alsa-devel, linux-kernel
On 19/06/2020 05:54, 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)")
> Reported-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> ---
Reviewed-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Tested-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.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");
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] ALSA: compress: document the compress audio state machine
2020-06-19 4:54 ` [PATCH 1/3] ALSA: compress: document the compress audio state machine Vinod Koul
@ 2020-06-19 9:29 ` Srinivas Kandagatla
2020-06-19 14:22 ` Pierre-Louis Bossart
1 sibling, 0 replies; 12+ messages in thread
From: Srinivas Kandagatla @ 2020-06-19 9:29 UTC (permalink / raw)
To: Vinod Koul, Takashi Iwai, Jaroslav Kysela
Cc: Charles Keepax, Pierre-Louis Bossart, alsa-devel, linux-kernel
On 19/06/2020 05:54, 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>
> ---
Thanks Vinod for doing this,
Makes things much clear on the state-machine side!
Reviewed-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> .../sound/designs/compress-offload.rst | 52 +++++++++++++++++++
> 1 file changed, 52 insertions(+)
>
> diff --git a/Documentation/sound/designs/compress-offload.rst b/Documentation/sound/designs/compress-offload.rst
> index ad4bfbdacc83..7292717c43bf 100644
> --- a/Documentation/sound/designs/compress-offload.rst
> +++ b/Documentation/sound/designs/compress-offload.rst
> @@ -151,6 +151,58 @@ 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_drain_notify() | |
> + +------------------------>| SETUP |
> + | | |
> + | +----------+
> + | |
> + | |
> + | | compr_write()
> + | |
> + | V
> + | +----------+
> + | | |
> + | | PREPARE |
> + | | |
> + | +----------+
> + | |
> + | |
> + | | compr_start()
> + | |
> + | V
> + +----------+ +----------+ compr_pause() +----------+
> + | | | |----------------------->| |
> + | DRAIN |<-------------------| RUNNING | | PAUSE |
> + | | | |<-----------------------| |
> + +----------+ +----------+ compr_resume() +----------+
> + | |
> + | |
> + | | compr_free()
> + | |
> + | V
> + | +----------+
> + | compr_free() | |
> + +------------------------>| |
> + | STOP |
> + | |
> + +----------+
> +
>
> Gapless Playback
> ================
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] ALSA: compress: document the compress audio state machine
2020-06-19 4:54 ` [PATCH 1/3] ALSA: compress: document the compress audio state machine Vinod Koul
2020-06-19 9:29 ` Srinivas Kandagatla
@ 2020-06-19 14:22 ` Pierre-Louis Bossart
2020-06-22 5:33 ` Vinod Koul
1 sibling, 1 reply; 12+ messages in thread
From: Pierre-Louis Bossart @ 2020-06-19 14:22 UTC (permalink / raw)
To: Vinod Koul, Takashi Iwai, Jaroslav Kysela
Cc: Charles Keepax, alsa-devel, linux-kernel
> +
> + +----------+
> + | |
> + | OPEN |
> + | |
> + +----------+
> + |
> + |
> + | compr_set_params()
> + |
> + V
> + +----------+
> + compr_drain_notify() | |
> + +------------------------>| SETUP |
> + | | |
> + | +----------+
> + | |
> + | |
> + | | compr_write()
> + | |
> + | V
> + | +----------+
> + | | |
> + | | PREPARE |
> + | | |
> + | +----------+
> + | |
> + | |
> + | | compr_start()
> + | |
> + | V
> + +----------+ +----------+ compr_pause() +----------+
> + | | | |----------------------->| |
> + | DRAIN |<-------------------| RUNNING | | PAUSE |
> + | | | |<-----------------------| |
> + +----------+ +----------+ compr_resume() +----------+
> + | |
> + | |
> + | | compr_free()
> + | |
> + | V
> + | +----------+
> + | compr_free() | |
> + +------------------------>| |
> + | STOP |
> + | |
> + +----------+
The STOP state doesn't seem quite right to me, sorry.
the direction of the DRAIN-STOP comp_free() arrow seems wrong? Of if it
is correct, then something's missing to exit the STOP state so that the
stream can be opened again.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] ALSA: compress: document the compress gapless audio state machine
2020-06-19 4:54 ` [PATCH 2/3] ALSA: compress: document the compress gapless " Vinod Koul
@ 2020-06-19 14:27 ` Pierre-Louis Bossart
2020-06-22 5:34 ` Vinod Koul
0 siblings, 1 reply; 12+ messages in thread
From: Pierre-Louis Bossart @ 2020-06-19 14:27 UTC (permalink / raw)
To: Vinod Koul, Takashi Iwai, Jaroslav Kysela
Cc: Charles Keepax, alsa-devel, linux-kernel
> +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 |
> + +----------+
May I suggest having a single state machine, not a big one and an
additional partial one. It would help explain why in one case
compr_drain_notify() triggers a transition to RUNNING while in the other
one it goes to SETUP.
I realize it's more complicated to edit but it'd be easier on
reviewers/users.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] ALSA: compress: document the compress audio state machine
2020-06-19 14:22 ` Pierre-Louis Bossart
@ 2020-06-22 5:33 ` Vinod Koul
0 siblings, 0 replies; 12+ messages in thread
From: Vinod Koul @ 2020-06-22 5:33 UTC (permalink / raw)
To: Pierre-Louis Bossart
Cc: Takashi Iwai, Jaroslav Kysela, Charles Keepax, alsa-devel, linux-kernel
HI Pierre,
On 19-06-20, 09:22, Pierre-Louis Bossart wrote:
>
> > +
> > + +----------+
> > + | |
> > + | OPEN |
> > + | |
> > + +----------+
> > + |
> > + |
> > + | compr_set_params()
> > + |
> > + V
> > + +----------+
> > + compr_drain_notify() | |
> > + +------------------------>| SETUP |
> > + | | |
> > + | +----------+
> > + | |
> > + | |
> > + | | compr_write()
> > + | |
> > + | V
> > + | +----------+
> > + | | |
> > + | | PREPARE |
> > + | | |
> > + | +----------+
> > + | |
> > + | |
> > + | | compr_start()
> > + | |
> > + | V
> > + +----------+ +----------+ compr_pause() +----------+
> > + | | | |----------------------->| |
> > + | DRAIN |<-------------------| RUNNING | | PAUSE |
> > + | | | |<-----------------------| |
> > + +----------+ +----------+ compr_resume() +----------+
> > + | |
> > + | |
> > + | | compr_free()
> > + | |
> > + | V
> > + | +----------+
> > + | compr_free() | |
> > + +------------------------>| |
> > + | STOP |
> > + | |
> > + +----------+
>
>
> The STOP state doesn't seem quite right to me, sorry.
We should call it free, Will update
> the direction of the DRAIN-STOP comp_free() arrow seems wrong? Of if it is
> correct, then something's missing to exit the STOP state so that the stream
> can be opened again.
Once stream is freed, it can't be opened again.
But we have trigger stop which is not comprehended here, I will add
compr_stop() above which would transition to SETUP state. And a stopped
stream can be freed up as well, so one more transition from SETUP to
FREE.
Thanks for reviewing
--
~Vinod
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] ALSA: compress: document the compress gapless audio state machine
2020-06-19 14:27 ` Pierre-Louis Bossart
@ 2020-06-22 5:34 ` Vinod Koul
2020-06-22 13:22 ` Pierre-Louis Bossart
0 siblings, 1 reply; 12+ messages in thread
From: Vinod Koul @ 2020-06-22 5:34 UTC (permalink / raw)
To: Pierre-Louis Bossart
Cc: Takashi Iwai, Jaroslav Kysela, Charles Keepax, alsa-devel, linux-kernel
On 19-06-20, 09:27, Pierre-Louis Bossart wrote:
>
> > +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 |
> > + +----------+
>
> May I suggest having a single state machine, not a big one and an additional
> partial one. It would help explain why in one case compr_drain_notify()
> triggers a transition to RUNNING while in the other one it goes to SETUP.
>
> I realize it's more complicated to edit but it'd be easier on
> reviewers/users.
Ell adding stop and transitions would really make it complicated and
gapless is a bit different handling and it looks cleaner this way IMO,
so lets stick to this. Feel free to create one if you are up for it.
--
~Vinod
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] ALSA: compress: fix partial_drain completion state
2020-06-19 9:13 ` Srinivas Kandagatla
@ 2020-06-22 5:35 ` Vinod Koul
0 siblings, 0 replies; 12+ messages in thread
From: Vinod Koul @ 2020-06-22 5:35 UTC (permalink / raw)
To: Srinivas Kandagatla
Cc: Takashi Iwai, Jaroslav Kysela, Pierre-Louis Bossart,
Charles Keepax, alsa-devel, linux-kernel
On 19-06-20, 10:13, Srinivas Kandagatla wrote:
>
>
> On 19/06/2020 05:54, 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)")
> > Reported-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> > Signed-off-by: Vinod Koul <vkoul@kernel.org>
> > ---
>
> Reviewed-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> Tested-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Thanks for testing Srini
--
~Vinod
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] ALSA: compress: document the compress gapless audio state machine
2020-06-22 5:34 ` Vinod Koul
@ 2020-06-22 13:22 ` Pierre-Louis Bossart
0 siblings, 0 replies; 12+ messages in thread
From: Pierre-Louis Bossart @ 2020-06-22 13:22 UTC (permalink / raw)
To: Vinod Koul; +Cc: linux-kernel, alsa-devel, Charles Keepax, Takashi Iwai
On 6/22/20 12:34 AM, Vinod Koul wrote:
> On 19-06-20, 09:27, Pierre-Louis Bossart wrote:
>>
>>> +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 |
>>> + +----------+
>>
>> May I suggest having a single state machine, not a big one and an additional
>> partial one. It would help explain why in one case compr_drain_notify()
>> triggers a transition to RUNNING while in the other one it goes to SETUP.
>>
>> I realize it's more complicated to edit but it'd be easier on
>> reviewers/users.
>
> Ell adding stop and transitions would really make it complicated and
> gapless is a bit different handling and it looks cleaner this way IMO,
> so lets stick to this. Feel free to create one if you are up for it.
if you don't want to change the visuals then please add a paragraph
explaining the different uses of compr_drain_notify().
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-06-22 13:46 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-19 4:54 [PATCH 0/3] ALSA: compress: Document stream states and fix gaplless SM Vinod Koul
2020-06-19 4:54 ` [PATCH 1/3] ALSA: compress: document the compress audio state machine Vinod Koul
2020-06-19 9:29 ` Srinivas Kandagatla
2020-06-19 14:22 ` Pierre-Louis Bossart
2020-06-22 5:33 ` Vinod Koul
2020-06-19 4:54 ` [PATCH 2/3] ALSA: compress: document the compress gapless " Vinod Koul
2020-06-19 14:27 ` Pierre-Louis Bossart
2020-06-22 5:34 ` Vinod Koul
2020-06-22 13:22 ` Pierre-Louis Bossart
2020-06-19 4:54 ` [PATCH 3/3] ALSA: compress: fix partial_drain completion state Vinod Koul
2020-06-19 9:13 ` Srinivas Kandagatla
2020-06-22 5:35 ` Vinod Koul
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).