* [PATCH v4 0/3] ALSA: compress: Document stream states and fix gapless SM
@ 2020-06-29 7:49 ` Vinod Koul
0 siblings, 0 replies; 16+ messages in thread
From: Vinod Koul @ 2020-06-29 7:49 UTC (permalink / raw)
To: Takashi Iwai, Jaroslav Kysela
Cc: Vinod Koul, Srinivas Kandagatla, 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.
Changes in v4:
- Add review tag by Charles
- fix the lock deadlock pointed by Charles
Changes in v3:
- Add pause->stop->free transition
- Add setup->free transition
- remove running->free as that goes thru stop
Changes in v2:
- Added tested tag by Srini
- Update compress SM with Free state and compr_stop() transitions
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 | 83 +++++++++++++++++++
include/sound/compress_driver.h | 10 ++-
sound/core/compress_offload.c | 4 +
3 files changed, 96 insertions(+), 1 deletion(-)
--
2.26.2
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v4 0/3] ALSA: compress: Document stream states and fix gapless SM
@ 2020-06-29 7:49 ` Vinod Koul
0 siblings, 0 replies; 16+ messages in thread
From: Vinod Koul @ 2020-06-29 7:49 UTC (permalink / raw)
To: Takashi Iwai, Jaroslav Kysela
Cc: alsa-devel, Charles Keepax, Pierre-Louis Bossart, linux-kernel,
Vinod Koul, Srinivas Kandagatla
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.
Changes in v4:
- Add review tag by Charles
- fix the lock deadlock pointed by Charles
Changes in v3:
- Add pause->stop->free transition
- Add setup->free transition
- remove running->free as that goes thru stop
Changes in v2:
- Added tested tag by Srini
- Update compress SM with Free state and compr_stop() transitions
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 | 83 +++++++++++++++++++
include/sound/compress_driver.h | 10 ++-
sound/core/compress_offload.c | 4 +
3 files changed, 96 insertions(+), 1 deletion(-)
--
2.26.2
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v4 1/3] ALSA: compress: document the compress audio state machine
2020-06-29 7:49 ` Vinod Koul
@ 2020-06-29 7:50 ` Vinod Koul
-1 siblings, 0 replies; 16+ messages in thread
From: Vinod Koul @ 2020-06-29 7:50 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
Reviewed-by: Charles Keepax <ckeepax@opensource.cirrus.com>
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] 16+ messages in thread
* [PATCH v4 1/3] ALSA: compress: document the compress audio state machine
@ 2020-06-29 7:50 ` Vinod Koul
0 siblings, 0 replies; 16+ messages in thread
From: Vinod Koul @ 2020-06-29 7:50 UTC (permalink / raw)
To: Takashi Iwai, Jaroslav Kysela
Cc: alsa-devel, Charles Keepax, Pierre-Louis Bossart, linux-kernel,
Vinod Koul, Srinivas Kandagatla
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
Reviewed-by: Charles Keepax <ckeepax@opensource.cirrus.com>
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] 16+ messages in thread
* [PATCH v4 2/3] ALSA: compress: document the compress gapless audio state machine
2020-06-29 7:49 ` Vinod Koul
@ 2020-06-29 7:50 ` Vinod Koul
-1 siblings, 0 replies; 16+ messages in thread
From: Vinod Koul @ 2020-06-29 7:50 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] 16+ messages in thread
* [PATCH v4 2/3] ALSA: compress: document the compress gapless audio state machine
@ 2020-06-29 7:50 ` Vinod Koul
0 siblings, 0 replies; 16+ messages in thread
From: Vinod Koul @ 2020-06-29 7:50 UTC (permalink / raw)
To: Takashi Iwai, Jaroslav Kysela
Cc: alsa-devel, Charles Keepax, Pierre-Louis Bossart, linux-kernel,
Vinod Koul, Srinivas Kandagatla
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] 16+ messages in thread
* [PATCH v4 3/3] ALSA: compress: fix partial_drain completion state
2020-06-29 7:49 ` Vinod Koul
@ 2020-06-29 7:50 ` Vinod Koul
-1 siblings, 0 replies; 16+ messages in thread
From: Vinod Koul @ 2020-06-29 7:50 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 | 10 +++++++++-
sound/core/compress_offload.c | 4 ++++
2 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h
index 3df8d8c90191..8d23351f7ad7 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,13 @@ 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;
+ /* 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;
+ }
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] 16+ messages in thread
* [PATCH v4 3/3] ALSA: compress: fix partial_drain completion state
@ 2020-06-29 7:50 ` Vinod Koul
0 siblings, 0 replies; 16+ messages in thread
From: Vinod Koul @ 2020-06-29 7:50 UTC (permalink / raw)
To: Takashi Iwai, Jaroslav Kysela
Cc: alsa-devel, Charles Keepax, Pierre-Louis Bossart, linux-kernel,
Vinod Koul, 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)")
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 | 10 +++++++++-
sound/core/compress_offload.c | 4 ++++
2 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/include/sound/compress_driver.h b/include/sound/compress_driver.h
index 3df8d8c90191..8d23351f7ad7 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,13 @@ 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;
+ /* 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;
+ }
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] 16+ messages in thread
* Re: [PATCH v4 1/3] ALSA: compress: document the compress audio state machine
2020-06-29 7:50 ` Vinod Koul
@ 2020-06-29 9:05 ` Amadeusz Sławiński
-1 siblings, 0 replies; 16+ messages in thread
From: Amadeusz Sławiński @ 2020-06-29 9:05 UTC (permalink / raw)
To: Vinod Koul, Takashi Iwai, Jaroslav Kysela
Cc: alsa-devel, Charles Keepax, Pierre-Louis Bossart, linux-kernel,
Srinivas Kandagatla
On 6/29/2020 9:50 AM, 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
>
> Reviewed-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> 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() |
> + | | | |
> + | | | |
> + | | +----------+ |
> + | | | | 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
> ================
>
Line containing compr_free (between SETUP and FREE) seems to be misaligned?
If you move prepare to the left and drain in place of drain, it feels
like you won't need this weird indirection with A
Something like:
>> + v
>> + compr_free() +----------+
>> + +------------------------------------| |
>> + | | SETUP |
>> + | +------------------------>| |<-------------------------+
>> + | | compr_write() +----------+ |
>> + | | ^ |
>> + | | | compr_drain_notify() or |
>> + | | | compr_stop() |
>> + | | | |
>> + | | | |
>> + | | +----------+ |
>> + | | | | |
>> + | | | DRAIN | |
>> + | | | | |
>> + | | +----------+ |
>> + | | ^ |
>> + | | | |
>> + | | | compr_drain() |
>> + | | | |
>> + | | | |
>> + | +----------+ +----------+ |
>> + | | | compr_start() | | compr_stop() |
>> + | | PREPARE |------------------->| RUNNING |--------------------------+
>> + | | | | | |
>> + | +----------+ +----------+ |
>> + | | | ^ |
>> + | | compr_free() | | |
>> + | | compr_pause() | | compr_resume() |
>> + | | | | |
>> + | v v | |
>> + | +----------+ +----------+ |
>> + | | | | | compr_stop() |
>> + +--->| FREE | | PAUSE |---------------------------+
>> + | | | |
>> + +----------+ +----------+
>> +
but this makes me question PREPARE state, how do you enter it?
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 1/3] ALSA: compress: document the compress audio state machine
@ 2020-06-29 9:05 ` Amadeusz Sławiński
0 siblings, 0 replies; 16+ messages in thread
From: Amadeusz Sławiński @ 2020-06-29 9:05 UTC (permalink / raw)
To: Vinod Koul, Takashi Iwai, Jaroslav Kysela
Cc: Srinivas Kandagatla, alsa-devel, Charles Keepax,
Pierre-Louis Bossart, linux-kernel
On 6/29/2020 9:50 AM, 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
>
> Reviewed-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> 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() |
> + | | | |
> + | | | |
> + | | +----------+ |
> + | | | | 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
> ================
>
Line containing compr_free (between SETUP and FREE) seems to be misaligned?
If you move prepare to the left and drain in place of drain, it feels
like you won't need this weird indirection with A
Something like:
>> + v
>> + compr_free() +----------+
>> + +------------------------------------| |
>> + | | SETUP |
>> + | +------------------------>| |<-------------------------+
>> + | | compr_write() +----------+ |
>> + | | ^ |
>> + | | | compr_drain_notify() or |
>> + | | | compr_stop() |
>> + | | | |
>> + | | | |
>> + | | +----------+ |
>> + | | | | |
>> + | | | DRAIN | |
>> + | | | | |
>> + | | +----------+ |
>> + | | ^ |
>> + | | | |
>> + | | | compr_drain() |
>> + | | | |
>> + | | | |
>> + | +----------+ +----------+ |
>> + | | | compr_start() | | compr_stop() |
>> + | | PREPARE |------------------->| RUNNING |--------------------------+
>> + | | | | | |
>> + | +----------+ +----------+ |
>> + | | | ^ |
>> + | | compr_free() | | |
>> + | | compr_pause() | | compr_resume() |
>> + | | | | |
>> + | v v | |
>> + | +----------+ +----------+ |
>> + | | | | | compr_stop() |
>> + +--->| FREE | | PAUSE |---------------------------+
>> + | | | |
>> + +----------+ +----------+
>> +
but this makes me question PREPARE state, how do you enter it?
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 1/3] ALSA: compress: document the compress audio state machine
2020-06-29 9:05 ` Amadeusz Sławiński
@ 2020-06-29 9:34 ` Vinod Koul
-1 siblings, 0 replies; 16+ messages in thread
From: Vinod Koul @ 2020-06-29 9:34 UTC (permalink / raw)
To: Amadeusz Sławiński
Cc: Takashi Iwai, Jaroslav Kysela, alsa-devel, Charles Keepax,
Pierre-Louis Bossart, linux-kernel, Srinivas Kandagatla
Hi Amadeusz,
On 29-06-20, 11:05, Amadeusz Sławiński wrote:
>
>
> On 6/29/2020 9:50 AM, 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
> >
> > Reviewed-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> > 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() |
> > + | | | |
> > + | | | |
Not sure, this is not in the version I posted, arrow is from setup ->
prepare. See https://lore.kernel.org/alsa-devel/20200629075002.11436-2-vkoul@kernel.org/
I don't know how it got reversed in your version, maybe the MUA messed
up??
> > + | | +----------+ |
> > + | | | | 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
> > ================
> >
>
> Line containing compr_free (between SETUP and FREE) seems to be misaligned?
not in the version I posted and see on lore
> If you move prepare to the left and drain in place of drain, it feels like
> you won't need this weird indirection with A
yeah that was a compromise to make it bit neater to me than adding
overlapping arrows...
> Something like:
>
> >> + v
> > > + compr_free() +----------+
> > > + +------------------------------------| |
> > > + | | SETUP |
> > > + | +------------------------>| |<-------------------------+
> > > + | | compr_write() +----------+ |
> > > + | | ^ |
> > > + | | | compr_drain_notify() or |
> > > + | | | compr_stop() |
> > > + | | | |
> > > + | | | |
> > > + | | +----------+ |
> > > + | | | | |
> > > + | | | DRAIN | |
> > > + | | | | |
> > > + | | +----------+ |
> > > + | | ^ |
> > > + | | | |
> > > + | | | compr_drain() |
> > > + | | | |
> > > + | | | |
> > > + | +----------+ +----------+ |
> > > + | | | compr_start() | | compr_stop() |
> > > + | | PREPARE |------------------->| RUNNING |--------------------------+
> > > + | | | | | |
> > > + | +----------+ +----------+ |
> > > + | | | ^ |
> > > + | | compr_free() | | |
> > > + | | compr_pause() | | compr_resume() |
> > > + | | | | |
> > > + | v v | |
> > > + | +----------+ +----------+ |
> > > + | | | | | compr_stop() |
> > > + +--->| FREE | | PAUSE |---------------------------+
> > > + | | | |
> > > + +----------+ +----------+
> > > +
>
> but this makes me question PREPARE state, how do you enter it?
a compr_write() moves from SETUP -> PREPARE. Fixing that above looks
better version of mine..
--
~Vinod
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 1/3] ALSA: compress: document the compress audio state machine
@ 2020-06-29 9:34 ` Vinod Koul
0 siblings, 0 replies; 16+ messages in thread
From: Vinod Koul @ 2020-06-29 9:34 UTC (permalink / raw)
To: Amadeusz Sławiński
Cc: alsa-devel, Charles Keepax, linux-kernel, Pierre-Louis Bossart,
Takashi Iwai, Srinivas Kandagatla
Hi Amadeusz,
On 29-06-20, 11:05, Amadeusz Sławiński wrote:
>
>
> On 6/29/2020 9:50 AM, 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
> >
> > Reviewed-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> > 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() |
> > + | | | |
> > + | | | |
Not sure, this is not in the version I posted, arrow is from setup ->
prepare. See https://lore.kernel.org/alsa-devel/20200629075002.11436-2-vkoul@kernel.org/
I don't know how it got reversed in your version, maybe the MUA messed
up??
> > + | | +----------+ |
> > + | | | | 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
> > ================
> >
>
> Line containing compr_free (between SETUP and FREE) seems to be misaligned?
not in the version I posted and see on lore
> If you move prepare to the left and drain in place of drain, it feels like
> you won't need this weird indirection with A
yeah that was a compromise to make it bit neater to me than adding
overlapping arrows...
> Something like:
>
> >> + v
> > > + compr_free() +----------+
> > > + +------------------------------------| |
> > > + | | SETUP |
> > > + | +------------------------>| |<-------------------------+
> > > + | | compr_write() +----------+ |
> > > + | | ^ |
> > > + | | | compr_drain_notify() or |
> > > + | | | compr_stop() |
> > > + | | | |
> > > + | | | |
> > > + | | +----------+ |
> > > + | | | | |
> > > + | | | DRAIN | |
> > > + | | | | |
> > > + | | +----------+ |
> > > + | | ^ |
> > > + | | | |
> > > + | | | compr_drain() |
> > > + | | | |
> > > + | | | |
> > > + | +----------+ +----------+ |
> > > + | | | compr_start() | | compr_stop() |
> > > + | | PREPARE |------------------->| RUNNING |--------------------------+
> > > + | | | | | |
> > > + | +----------+ +----------+ |
> > > + | | | ^ |
> > > + | | compr_free() | | |
> > > + | | compr_pause() | | compr_resume() |
> > > + | | | | |
> > > + | v v | |
> > > + | +----------+ +----------+ |
> > > + | | | | | compr_stop() |
> > > + +--->| FREE | | PAUSE |---------------------------+
> > > + | | | |
> > > + +----------+ +----------+
> > > +
>
> but this makes me question PREPARE state, how do you enter it?
a compr_write() moves from SETUP -> PREPARE. Fixing that above looks
better version of mine..
--
~Vinod
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 1/3] ALSA: compress: document the compress audio state machine
2020-06-29 9:34 ` Vinod Koul
@ 2020-06-29 10:02 ` Amadeusz Sławiński
-1 siblings, 0 replies; 16+ messages in thread
From: Amadeusz Sławiński @ 2020-06-29 10:02 UTC (permalink / raw)
To: Vinod Koul
Cc: alsa-devel, Charles Keepax, linux-kernel, Pierre-Louis Bossart,
Takashi Iwai, Srinivas Kandagatla
On 6/29/2020 11:34 AM, Vinod Koul wrote:
>
> Not sure, this is not in the version I posted, arrow is from setup ->
> prepare. See
https://lore.kernel.org/alsa-devel/20200629075002.11436-2-vkoul@kernel.org/
>
> I don't know how it got reversed in your version, maybe the MUA messed
> up??
>
Ah... I must have edited it myself when preparing edited version, sorry
for noise ;)
> a compr_write() moves from SETUP -> PREPARE. Fixing that above looks
> better version of mine..
Yes, I feel it is easier to understand, A redirect confused me for a
short bit when I first looked at graph and it can be easily avoided.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 1/3] ALSA: compress: document the compress audio state machine
@ 2020-06-29 10:02 ` Amadeusz Sławiński
0 siblings, 0 replies; 16+ messages in thread
From: Amadeusz Sławiński @ 2020-06-29 10:02 UTC (permalink / raw)
To: Vinod Koul
Cc: Pierre-Louis Bossart, alsa-devel, Charles Keepax, Takashi Iwai,
linux-kernel, Srinivas Kandagatla
On 6/29/2020 11:34 AM, Vinod Koul wrote:
>
> Not sure, this is not in the version I posted, arrow is from setup ->
> prepare. See
https://lore.kernel.org/alsa-devel/20200629075002.11436-2-vkoul@kernel.org/
>
> I don't know how it got reversed in your version, maybe the MUA messed
> up??
>
Ah... I must have edited it myself when preparing edited version, sorry
for noise ;)
> a compr_write() moves from SETUP -> PREPARE. Fixing that above looks
> better version of mine..
Yes, I feel it is easier to understand, A redirect confused me for a
short bit when I first looked at graph and it can be easily avoided.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 1/3] ALSA: compress: document the compress audio state machine
2020-06-29 10:02 ` Amadeusz Sławiński
@ 2020-06-29 10:59 ` Vinod Koul
-1 siblings, 0 replies; 16+ messages in thread
From: Vinod Koul @ 2020-06-29 10:59 UTC (permalink / raw)
To: Amadeusz Sławiński
Cc: alsa-devel, Charles Keepax, linux-kernel, Pierre-Louis Bossart,
Takashi Iwai, Srinivas Kandagatla
On 29-06-20, 12:02, Amadeusz Sławiński wrote:
> On 6/29/2020 11:34 AM, Vinod Koul wrote:
> >
> > Not sure, this is not in the version I posted, arrow is from setup ->
> > prepare. See
> https://lore.kernel.org/alsa-devel/20200629075002.11436-2-vkoul@kernel.org/
> >
> > I don't know how it got reversed in your version, maybe the MUA messed
> > up??
> >
> Ah... I must have edited it myself when preparing edited version, sorry for
> noise ;)
No worries
>
> > a compr_write() moves from SETUP -> PREPARE. Fixing that above looks
> > better version of mine..
> Yes, I feel it is easier to understand, A redirect confused me for a short
> bit when I first looked at graph and it can be easily avoided.
Thanks for the suggestion, it was a good one indeed, I will send the
update shortly
--
~Vinod
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 1/3] ALSA: compress: document the compress audio state machine
@ 2020-06-29 10:59 ` Vinod Koul
0 siblings, 0 replies; 16+ messages in thread
From: Vinod Koul @ 2020-06-29 10:59 UTC (permalink / raw)
To: Amadeusz Sławiński
Cc: Pierre-Louis Bossart, alsa-devel, Charles Keepax, Takashi Iwai,
linux-kernel, Srinivas Kandagatla
On 29-06-20, 12:02, Amadeusz Sławiński wrote:
> On 6/29/2020 11:34 AM, Vinod Koul wrote:
> >
> > Not sure, this is not in the version I posted, arrow is from setup ->
> > prepare. See
> https://lore.kernel.org/alsa-devel/20200629075002.11436-2-vkoul@kernel.org/
> >
> > I don't know how it got reversed in your version, maybe the MUA messed
> > up??
> >
> Ah... I must have edited it myself when preparing edited version, sorry for
> noise ;)
No worries
>
> > a compr_write() moves from SETUP -> PREPARE. Fixing that above looks
> > better version of mine..
> Yes, I feel it is easier to understand, A redirect confused me for a short
> bit when I first looked at graph and it can be easily avoided.
Thanks for the suggestion, it was a good one indeed, I will send the
update shortly
--
~Vinod
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2020-06-29 21:41 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-29 7:49 [PATCH v4 0/3] ALSA: compress: Document stream states and fix gapless SM Vinod Koul
2020-06-29 7:49 ` Vinod Koul
2020-06-29 7:50 ` [PATCH v4 1/3] ALSA: compress: document the compress audio state machine Vinod Koul
2020-06-29 7:50 ` Vinod Koul
2020-06-29 9:05 ` Amadeusz Sławiński
2020-06-29 9:05 ` Amadeusz Sławiński
2020-06-29 9:34 ` Vinod Koul
2020-06-29 9:34 ` Vinod Koul
2020-06-29 10:02 ` Amadeusz Sławiński
2020-06-29 10:02 ` Amadeusz Sławiński
2020-06-29 10:59 ` Vinod Koul
2020-06-29 10:59 ` Vinod Koul
2020-06-29 7:50 ` [PATCH v4 2/3] ALSA: compress: document the compress gapless " Vinod Koul
2020-06-29 7:50 ` Vinod Koul
2020-06-29 7:50 ` [PATCH v4 3/3] ALSA: compress: fix partial_drain completion state Vinod Koul
2020-06-29 7:50 ` Vinod Koul
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.