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

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

[Matias Ezequiel Vara Larsen]

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:
+\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.
 
 \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.
 
 \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

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

[Stefano Garzarella]

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?

>+\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?

> \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.
>
> \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
>
>

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

[Matias Ezequiel Vara Larsen]

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
> > 
> > 
> 

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

[Stefano Garzarella]

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?

>
>> > +\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.

Thanks,
Stefano

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

[Manos Pitsidianakis]

This sounds like it is unnecessarily overthought, semantically there's
only one command, "change state to" and it has a payload of "the new
desired state to transition to"

On Tue, 28 May 2024 at 18:13, Stefano Garzarella <sgarzare@redhat.com> 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?
>
> >
> >> > +\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.

`Transition` is standard sequential logic terminology.
In a finite state machine you can just say that a state has next
states; they are not "valid", they are just the only ones existing.

What can be valid or not is the transition.


-- 
Manos Pitsidianakis
Emulation and Virtualization Engineer at Linaro Ltd

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

[Matias Ezequiel Vara Larsen]

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