Re: [PATCH Resend] virtio-snd: improve PCM command lifecycle description

[Matias Ezequiel Vara Larsen <mvaralar@redhat.com>]

On Thu, May 23, 2024 at 01:00:32PM +0200, Stefano Garzarella wrote:
> On Tue, May 21, 2024 at 05:13:18PM GMT, Matias Ezequiel Vara Larsen wrote:
> > The PCM command lifecycle can be described as a state-machine in which
> > the switching between states is triggered by commands. This commit
> > explicitly presents the different states and the commands that are
> > allowed in a given state. This commit leaves to the implementation the
> > decision the state reached depending on the command.
> > 
> > Signed-off-by: Matias Ezequiel Vara Larsen <mvaralar@redhat.com>
> > ---
> > device-types/sound/description.tex | 26 +++++++++++++++++++-------
> > 1 file changed, 19 insertions(+), 7 deletions(-)
> > 
> > diff --git a/device-types/sound/description.tex b/device-types/sound/description.tex
> > index 54c9c8e..55e93af 100644
> > --- a/device-types/sound/description.tex
> > +++ b/device-types/sound/description.tex
> > @@ -397,9 +397,21 @@ \subsubsection{PCM Control Messages}\label{sec:Device Types / Sound Device / Dev
> > \item[\field{stream_id}] specifies a PCM stream identifier from 0 to \field{streams} - 1.
> > \end{description}
> > 
> > -\paragraph{PCM Command Lifecycle}
> > +\paragraph{PCM Command Lifecycle State Machine}
> > 
> > -A PCM stream has the following command lifecycle:
> > +A PCM stream can be represented by a state-machine made up of the following
> > +states:
> 
> I'm not clear about the difference between commands and states. Are they the
> same?
> 

AFAIU, a command is a request that comes into the control queue, such as
prepare, set_params, stop, start, etc. State refers to a stream's
current state.  A command triggers a transition over the stream's
states. There is a set of valid commands that are allowed in each state.
The specification does not make such a distinction. I wonder if this was
done on purpose by the authors.

> > +\begin{description}
> > +\item SET PARAMETERS
> > +\item PREPARE
> > +\item START
> > +\item STOP
> > +\item RELEASE
> > +\end{description}
> > +
> > +A command triggers the transition between these states. To what state to switch
> > +depends on the implementation. Depending on the state, only certain commands
> > +are valid. An error may occur if an invalid command is triggered.
> > 
> 
> Should we explain what the following list is?
> 

I think we should. The list is the states and the commands that are
valid in a given state. The specification does not explain the state
that is taken when a command is triggered. I though leaving that for the
implementation.

> > \begin{enumerate}
> > \item SET PARAMETERS
> > @@ -407,13 +419,13 @@ \subsubsection{PCM Control Messages}\label{sec:Device Types / Sound Device / Dev
> > The driver negotiates the stream parameters (format, transport, etc) with
> > the device.
> > 
> > -Possible valid transitions: set parameters, prepare.
> > +Possible valid commands: set parameters, prepare.
> > 

For example, in the SET_PARAMETERS state, there are two valid commands:
set parameters and prepare. Any other command in this state would be
invalid. If the frontend triggers the set_parameters command, the
backend may decide to either keep the stream state in SET_PARAMETERS or
switch to PREPARE. 

In the spec they use the wording `transition` and I think they mean the
valid next stream state. I changed this so now the spec specifies the
valid commands in a given state thus leaving for the implementation the
transition or the next state.

Thanks,

> > \item PREPARE
> > 
> > The device prepares the stream (allocates resources, etc).
> > 
> > -Possible valid transitions: set parameters, prepare, start, release.
> > +Possible valid commands: set parameters, prepare, start, release.
> > 
> > \item Output only: the driver transfers data for pre-buffing.
> > 
> > @@ -421,7 +433,7 @@ \subsubsection{PCM Control Messages}\label{sec:Device Types / Sound Device / Dev
> > 
> > The device starts the stream (unmute, putting into running state, etc).
> > 
> > -Possible valid transitions: stop.
> > +Possible valid commands: stop.
> > 
> > \item The driver transfers data to/from the stream.
> > 
> > @@ -429,13 +441,13 @@ \subsubsection{PCM Control Messages}\label{sec:Device Types / Sound Device / Dev
> > 
> > The device stops the stream (mute, putting into non-running state, etc).
> > 
> > -Possible valid transitions: start, release.
> > +Possible valid commands: start, release.
> > 
> > \item RELEASE
> > 
> > The device releases the stream (frees resources, etc).
> > 
> > -Possible valid transitions: set parameters, prepare.
> > +Possible valid commands: set parameters, prepare.
> > 
> > \end{enumerate}
> > 
> > -- 
> > 2.41.0
> > 
> > 
>