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

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

On Tue, May 28, 2024 at 05:12:41PM +0200, Stefano Garzarella wrote:
> On Mon, May 27, 2024 at 03:40:23PM GMT, Matias Ezequiel Vara Larsen wrote:
> > 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.
> 
> Okay, so there is a 1:1 overlap between commands and states. Basically, the
> “stop” command has the action of moving the state from the current to the
> “stop” state, right?
> 

Right

> > 
> > > > +\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.
> 
> Okay, perhaps we need to clarify that commands and states are in a 1:1
> relationship and that a command indicates the state to which to move.
> 

Yes, I think that may help to clarify the spec. 
 
When the spec defines the following statement in PREPARE:

"Possible valid transitions: set parameters, prepare, start, release."

This would mean that, if current state is PREPARE and the "set
parameters" command is issued, the transition to the SET PARAMETERS
state would be taken. In my opinion, since the spec talks about a
"possible valid transition", the implementation may decide to either
transit to the SET PARAMETERS state or stay in PREPARE since both where
valid.

Honestly, I do not know if these comments help to clarify the spec.

Matias