Re: [PATCH v2 1/3] ALSA: compress: document the compress audio state machine
On 22-06-20, 15:13, Charles Keepax wrote: > On Mon, Jun 22, 2020 at 08:28:48AM -0500, Pierre-Louis Bossart wrote: > > On 6/22/20 1:58 AM, Vinod Koul wrote: +--+ > > 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. > > > > This is allowed in the current code, the kernel sends the stop > internally in this case, so it kinda does go through the setup > state just not from the users view point. I am not sure I have a > good handle on if that makes sense or not. The idea was to stop first so that we can handle dmas which might be setup (like running/paused/prepared). So we should stop first and then free up. But i think it was an overkill... :) > > c) no way to stop a paused stream? > > Currently the code does allow this and it certainly makes sense so > should probably be added. Yes will add -- ~Vinod
Re: [PATCH v2 1/3] ALSA: compress: document the compress audio state machine
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 > > --- > > .../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.
Re: [PATCH v2 1/3] ALSA: compress: document the compress audio state machine
On Mon, Jun 22, 2020 at 08:28:48AM -0500, Pierre-Louis Bossart wrote: > On 6/22/20 1:58 AM, Vinod Koul wrote: > >++--+ > >+| | > >+| 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() > >+ | | > >+ +--+ > 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. > This is allowed in the current code, the kernel sends the stop internally in this case, so it kinda does go through the setup state just not from the users view point. I am not sure I have a good handle on if that makes sense or not. > c) no way to stop a paused stream? Currently the code does allow this and it certainly makes sense so should probably be added. Thanks, Charles
Re: [PATCH v2 1/3] ALSA: compress: document the compress audio state machine
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 --- .../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... 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. b) no way to go back to SETUP from PREPARE? What happens if the app never starts but want to tear down the resources? c) no way to stop a paused stream?
