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:   Tue, 23 Jun 2020 09:35:17 +0530
From:   Vinod Koul <vkoul@kernel.org>
To:     Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Cc:     Takashi Iwai <tiwai@suse.com>, Jaroslav Kysela <perex@perex.cz>,
        alsa-devel@alsa-project.org,
        Charles Keepax <ckeepax@opensource.cirrus.com>,
        linux-arm-msm@vger.kernel.org,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        Srinivas Kandagatla <srinivas.kandagatla@linaro.org>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 1/3] ALSA: compress: document the compress audio state
 machine
Message-ID: <20200623040517.GO2324254@vkoul-mobl>
References: <20200622065811.221485-1-vkoul@kernel.org>
 <20200622065811.221485-2-vkoul@kernel.org>
 <800a2632-b263-500f-707e-c1ce94ce92d4@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <800a2632-b263-500f-707e-c1ce94ce92d4@linux.intel.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 22-06-20, 08:28, Pierre-Louis Bossart wrote:
> 
> 
> On 6/22/20 1:58 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
> > 
> > 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..6f86db82298b 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_free()                   +----------+
> > +  +-------------------------------------|          |
> > +  |                                     |   SETUP  |
> > +  |           +------------------------>|          |<---------------------------------+
> > +  |           | compr_drain_notify()    +----------+                                  |
> > +  |           |                              |                                        |
> > +  |           |                              |                                        |
> > +  |           |                              | compr_write()                          |
> > +  |           |                              |                                        |
> > +  |           |                              v                                        |
> > +  |           |                         +----------+                                  |
> > +  |           |                         |          |                                  |
> > +  |           |                         |  PREPARE |                                  |
> > +  |           |                         |          |                                  |
> > +  |           |                         +----------+                                  |
> > +  |           |                              |                                        |
> > +  |           |                              |                                        |
> > +  |           |                              | compr_start()                          |
> > +  |           |                              |                                        |
> > +  |           |                              v                                        |
> > +  |     +----------+                    +----------+  compr_pause()  +----------+     |
> > +  |     |          |    compr_drain()   |          |---------------->|          |     |
> > +  |     |  DRAIN   |<-------------------|  RUNNING |                 |  PAUSE   |     |
> > +  |     |          |                    |          |<----------------|          |     |
> > +  |     +----------+                    +----------+  compr_resume() +----------+     |
> > +  |           |                           |      |                                    |
> > +  |           |                           |      |                                    |
> > +  |           |                           |      |                                    |
> > +  |           |                           |      |          compr_stop()              |
> > +  |           |                           |      +------------------------------------+
> > +  |           |       +----------+        |
> > +  |           |       |          |        |
> > +  +-----------+------>|          |<-------+
> > +     compr_free()     |   FREE   |  compr_free()
> > +                      |          |
> > +                      +----------+
> > +
> 
> Sorry, this confuses me even more...

Oops

> a) can you clarify if we can go from running to free directly? is this
> really a legit transition? There's already the option of doing a stop and a
> a drain.

As Charles pointed it is legit one, but then from SM we should remove
running->free arrow to clarify. Internally, free in running, draining
and paused triggers a stop and then free.

> b) no way to go back to SETUP from PREPARE? What happens if the app never
> starts but want to tear down the resources?

See above, free would trigger an internal step to stop (setup) and then
freed.

Btw I am not adding changes to code but documenting the 'existing'
behaviour, so a code lookup will help

> c) no way to stop a paused stream?

Yes missed that, will add.

-- 
~Vinod