* [virtio-dev] [PATCH v2] snd: Add virtio sound device specification
@ 2019-11-05 15:06 Mikhail Golubev
2019-11-06 9:28 ` Mikhail Golubev
2019-11-12 13:20 ` Jean-Philippe Brucker
0 siblings, 2 replies; 8+ messages in thread
From: Mikhail Golubev @ 2019-11-05 15:06 UTC (permalink / raw)
To: virtio-dev
Cc: liam.r.girdwood, tiwai, broonie, maz, james.morse,
julien.thierry.kdev, suzuki.poulose, Anton Yakovlev
From: Anton Yakovlev <Anton.Yakovlev@opensynergy.com>
Hi everyone,
the patch proposes an updated version of virtio specification for a new
virtio sound device. The initial PATCH can be found here: [1]. The
device may be useful in case when having audio is required but a device
passthrough or emulation is not an option.
The virtio sound card supports output and input PCM streams. It reports
stream capabilities through device configuration space and utilizes two
virtqueues: one for control requests and one for I/O requests.
Signed-off-by: Anton Yakovlev <Anton.Yakovlev@opensynergy.com>
Signed-off-by: Mikhail Golubev <Mikhail.Golubev@opensynergy.com>
v1 -> v2:
* Add 5512 Hz and 384 kHz to supported frame rates
* English grammar clean-up
[1]: https://lists.oasis-open.org/archives/virtio-dev/201909/msg00130.html
---
conformance.tex | 20 +++
content.tex | 1 +
virtio-sound.tex | 410 +++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 431 insertions(+)
create mode 100644 virtio-sound.tex
diff --git a/conformance.tex b/conformance.tex
index 0ac58aa..31595bf 100644
--- a/conformance.tex
+++ b/conformance.tex
@@ -183,6 +183,16 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
\item \ref{drivernormative:Device Types / Socket Device / Device Operation / Device Events}
\end{itemize}
+\conformance{\subsection}{Sound Driver Conformance}\label{sec:Conformance / Driver Conformance / Sound Driver Conformance}
+
+A sound driver MUST conform to the following normative statements:
+
+\begin{itemize}
+\item \ref{drivernormative:Device Types / Sound Device / Device Initialization}
+\item \ref{drivernormative:Device Types / Sound Device / PCM control requests}
+\item \ref{drivernormative:Device Types / Sound Device / PCM IO requests}
+\end{itemize}
+
\conformance{\section}{Device Conformance}\label{sec:Conformance / Device Conformance}
A device MUST conform to the following normative statements:
@@ -338,6 +348,16 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
\item \ref{devicenormative:Device Types / Socket Device / Device Operation / Receive and Transmit}
\end{itemize}
+\conformance{\subsection}{Sound Device Conformance}\label{sec:Conformance / Device Conformance / Sound Device Conformance}
+
+A sound device MUST conform to the following normative statements:
+
+\begin{itemize}
+\item \ref{devicenormative:Device Types / Sound Device / Device Initialization}
+\item \ref{devicenormative:Device Types / Sound Device / PCM control requests}
+\item \ref{devicenormative:Device Types / Sound Device / PCM IO requests}
+\end{itemize}
+
\conformance{\section}{Legacy Interface: Transitional Device and Transitional Driver Conformance}\label{sec:Conformance / Legacy Interface: Transitional Device and Transitional Driver Conformance}
A conformant implementation MUST be either transitional or
non-transitional, see \ref{intro:Legacy
diff --git a/content.tex b/content.tex
index b1ea9b9..aaae074 100644
--- a/content.tex
+++ b/content.tex
@@ -5698,6 +5698,7 @@ \subsubsection{Legacy Interface: Framing Requirements}\label{sec:Device
\input{virtio-crypto.tex}
\input{virtio-vsock.tex}
\input{virtio-fs.tex}
+\input{virtio-sound.tex}
\chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
diff --git a/virtio-sound.tex b/virtio-sound.tex
new file mode 100644
index 0000000..f73e772
--- /dev/null
+++ b/virtio-sound.tex
@@ -0,0 +1,410 @@
+\section{Sound Device}\label{sec:Device Types / Sound Device}
+
+The virtio sound card is a virtual audio device supporting output and input PCM
+streams. All device control requests are placed into the control virtqueue and
+all I/O requests are placed into the PCM virtqueue.
+
+\subsection{Device ID}\label{sec:Device Types / Sound Device / Device ID}
+
+25
+
+\subsection{Virtqueues}\label{sec:Device Types / Sound Device / Virtqueues}
+
+\begin{description}
+\item[0] controlq
+\item[1] pcmq
+\end{description}
+
+The controlq virtqueue always exists, the pcmq virtqueue only exists if
+the VIRTIO_SND_F_PCM_OUTPUT and/or VIRTIO_SND_F_PCM_INPUT feature is negotiated.
+
+\subsection{Feature bits}\label{sec:Device Types / Sound Device / Feature bits}
+
+\begin{description}
+\item[VIRTIO_SND_F_PCM_OUTPUT (0)] Output PCM stream support.
+\item[VIRTIO_SND_F_PCM_INPUT (1)] Input PCM stream support.
+\end{description}
+
+\subsection{Device configuration layout}\label{sec:Device Types / Sound Device / Device configuration layout}
+
+\begin{lstlisting}
+/* supported PCM sample formats */
+enum {
+ VIRTIO_SND_PCM_FMT_S8 = 0,
+ VIRTIO_SND_PCM_FMT_U8,
+ VIRTIO_SND_PCM_FMT_S16,
+ VIRTIO_SND_PCM_FMT_U16,
+ VIRTIO_SND_PCM_FMT_S32,
+ VIRTIO_SND_PCM_FMT_U32,
+ VIRTIO_SND_PCM_FMT_FLOAT,
+ VIRTIO_SND_PCM_FMT_FLOAT64
+};
+
+/* supported PCM frame rates */
+enum {
+ VIRTIO_SND_PCM_RATE_5512 = 0,
+ VIRTIO_SND_PCM_RATE_8000,
+ VIRTIO_SND_PCM_RATE_11025,
+ VIRTIO_SND_PCM_RATE_16000,
+ VIRTIO_SND_PCM_RATE_22050,
+ VIRTIO_SND_PCM_RATE_32000,
+ VIRTIO_SND_PCM_RATE_44100,
+ VIRTIO_SND_PCM_RATE_48000,
+ VIRTIO_SND_PCM_RATE_64000,
+ VIRTIO_SND_PCM_RATE_88200,
+ VIRTIO_SND_PCM_RATE_96000,
+ VIRTIO_SND_PCM_RATE_176400,
+ VIRTIO_SND_PCM_RATE_192000,
+ VIRTIO_SND_PCM_RATE_384000
+};
+
+/* a PCM stream configuration */
+struct virtio_pcm_stream_config {
+ u8 channels_min;
+ u8 channels_max;
+ le16 formats; /* 1 << VIRTIO_SND_PCM_FMT_XXX */
+ le16 rates; /* 1 << VIRTIO_SND_PCM_RATE_XXX */
+ u16 padding;
+};
+
+/* a device configuration space */
+struct virtio_snd_config {
+ struct virtio_pcm_config {
+ struct virtio_pcm_stream_config output;
+ struct virtio_pcm_stream_config input;
+ } pcm;
+};
+\end{lstlisting}
+
+\subsubsection{Device configuration fields}
+
+The \field{pcm.output} and \field{pcm.input} fields contain PCM stream
+configuration:
+
+\begin{description}
+\item[\field{channels_min}] (driver-read-only) is a minimum number of supported
+channels.
+\item[\field{channels_max}] (driver-read-only) is a maximum number of supported
+channels.
+\item[\field{formats}] (driver-read-only) is a supported sample format bit map.
+\item[\field{rates}] (driver-read-only) is a supported frame rate bit map.
+\end{description}
+
+Only interleaved samples are supported.
+
+\subsection{Device Initialization}
+
+\begin{enumerate}
+\item If the VIRTIO_SND_F_PCM_OUTPUT feature is negotiated, \field{pcm.output}
+contains the valid output PCM stream configuration.
+\item If the VIRTIO_SND_F_PCM_INPUT feature is negotiated, \field{pcm.input}
+contains the valid input PCM stream configuration.
+\end{enumerate}
+
+\devicenormative{\subsubsection}{Device Initialization}{Device Types / Sound Device / Device Initialization}
+
+\begin{enumerate}
+\item The device MUST NOT set the not defined format and rate bits.
+\item The device MUST initialize padding bytes \field{pcm.output.padding} and
+\field{pcm.input.padding} to 0.
+\end{enumerate}
+
+\drivernormative{\subsubsection}{Device Initialization}{Device Types / Sound Device / Device Initialization}
+
+\begin{enumerate}
+\item The driver MUST configure and initialize all virtqueues.
+\item The driver SHOULD ignore the not defined format and rate bits.
+\end{enumerate}
+
+\subsection{Device Operation}\label{sec:Device Types / Sound Device / Device Operation}
+
+All control messages are placed into the controlq virtqueue and use the following
+layout structure and definitions:
+
+\begin{lstlisting}
+enum {
+ /* PCM control request types */
+ VIRTIO_SND_R_PCM_CHMAP_INFO = 0,
+ VIRTIO_SND_R_PCM_SET_FORMAT,
+ VIRTIO_SND_R_PCM_PREPARE,
+ VIRTIO_SND_R_PCM_START,
+ VIRTIO_SND_R_PCM_STOP,
+ VIRTIO_SND_R_PCM_PAUSE,
+ VIRTIO_SND_R_PCM_UNPAUSE,
+
+ /* generic status codes */
+ VIRTIO_SND_S_OK = 0x8000,
+ VIRTIO_SND_S_BAD_MSG,
+ VIRTIO_SND_S_NOT_SUPP,
+ VIRTIO_SND_S_IO_ERR
+};
+
+struct virtio_snd_ctl_msg {
+ /* device-read-only data */
+ le32 request_code;
+ u8 request_payload[];
+ /* device-writable data */
+ le32 response_status;
+ u8 response_payload[];
+};
+\end{lstlisting}
+
+A generic control message consists of a request part and a response part, and
+contains the following fields:
+
+\begin{description}
+\item[\field{request_code}] (device-read-only) specifies a device request code
+(VIRTIO_SND_R_*).
+\item[\field{request_payload}] (device-read-only) contains request-specific
+data.
+\item[\field{response_status}] (device-writable) specifies a device response
+status (VIRTIO_SND_S_*).
+\item[\field{response_payload}] (device-writable) contains response-specific
+data.
+\end{description}
+
+Unless stated otherwise, the \field{request_payload} and \field{response_payload}
+fields are empty.
+
+The \field{response_status} field contains one of the following values:
+
+\begin{itemize*}
+\item VIRTIO_SND_S_OK: success.
+\item VIRTIO_SND_S_BAD_MSG: a control message is malformed or contains invalid
+parameters.
+\item VIRTIO_SND_S_NOT_SUPP: requested operation or parameters are not supported.
+\item VIRTIO_SND_S_IO_ERR: an I/O error occurred.
+\end{itemize*}
+
+\subsubsection{Device Operation: PCM control requests}
+
+A PCM stream has the following command lifecycle:
+
+\begin{enumerate}
+\item Set format
+\item Prepare
+\item Output only: transfer data for prebuffing
+\item Start
+\item Transfer data to/from the PCM device
+\begin{enumerate}
+ \item Pause
+ \item Unpause
+\end{enumerate}
+\item Stop
+\end{enumerate}
+
+PCM control requests have or consist of a fixed header with the following
+layout structure:
+
+\begin{lstlisting}
+/* supported PCM stream types */
+enum {
+ VIRTIO_SND_PCM_T_OUTPUT = 0,
+ VIRTIO_SND_PCM_T_INPUT
+};
+
+/* a PCM control request header */
+struct virtio_snd_pcm_hdr {
+ le32 code;
+ le32 stream;
+};
+\end{lstlisting}
+
+PCM control requests contain the following fields:
+
+\begin{description}
+\item[\field{code}] (device-read-only) specifies a PCM device request code
+(VIRTIO_SND_R_PCM_*).
+\item[\field{stream}] (device-read-only) specifies a PCM stream type
+(VIRTIO_SND_PCM_T_*).
+\end{description}
+
+\begin{description}
+
+\item[VIRTIO_SND_R_PCM_CHMAP_INFO]
+Query a PCM channel map information for specified stream type.
+
+A response uses the following layout structure and definitions:
+
+\begin{lstlisting}
+/* standard channel position definition */
+enum {
+ VIRTIO_SND_PCM_CH_NONE = 0, /* undefined */
+ VIRTIO_SND_PCM_CH_NA, /* silent */
+ VIRTIO_SND_PCM_CH_MONO, /* mono stream */
+ VIRTIO_SND_PCM_CH_FL, /* front left */
+ VIRTIO_SND_PCM_CH_FR, /* front right */
+ VIRTIO_SND_PCM_CH_RL, /* rear left */
+ VIRTIO_SND_PCM_CH_RR, /* rear right */
+ VIRTIO_SND_PCM_CH_FC, /* front center */
+ VIRTIO_SND_PCM_CH_LFE, /* low frequency (LFE) */
+ VIRTIO_SND_PCM_CH_SL, /* side left */
+ VIRTIO_SND_PCM_CH_SR, /* side right */
+ VIRTIO_SND_PCM_CH_RC, /* rear center */
+ VIRTIO_SND_PCM_CH_FLC, /* front left center */
+ VIRTIO_SND_PCM_CH_FRC, /* front right center */
+ VIRTIO_SND_PCM_CH_RLC, /* rear left center */
+ VIRTIO_SND_PCM_CH_RRC, /* rear right center */
+ VIRTIO_SND_PCM_CH_FLW, /* front left wide */
+ VIRTIO_SND_PCM_CH_FRW, /* front right wide */
+ VIRTIO_SND_PCM_CH_FLH, /* front left high */
+ VIRTIO_SND_PCM_CH_FCH, /* front center high */
+ VIRTIO_SND_PCM_CH_FRH, /* front right high */
+ VIRTIO_SND_PCM_CH_TC, /* top center */
+ VIRTIO_SND_PCM_CH_TFL, /* top front left */
+ VIRTIO_SND_PCM_CH_TFR, /* top front right */
+ VIRTIO_SND_PCM_CH_TFC, /* top front center */
+ VIRTIO_SND_PCM_CH_TRL, /* top rear left */
+ VIRTIO_SND_PCM_CH_TRR, /* top rear right */
+ VIRTIO_SND_PCM_CH_TRC, /* top rear center */
+ VIRTIO_SND_PCM_CH_TFLC, /* top front left center */
+ VIRTIO_SND_PCM_CH_TFRC, /* top front right center */
+ VIRTIO_SND_PCM_CH_TSL, /* top side left */
+ VIRTIO_SND_PCM_CH_TSR, /* top side right */
+ VIRTIO_SND_PCM_CH_LLFE, /* left LFE */
+ VIRTIO_SND_PCM_CH_RLFE, /* right LFE */
+ VIRTIO_SND_PCM_CH_BC, /* bottom center */
+ VIRTIO_SND_PCM_CH_BLC, /* bottom left center */
+ VIRTIO_SND_PCM_CH_BRC /* bottom right center */
+};
+
+/* a maximum possible number of channels */
+#define VIRTIO_SND_PCM_CH_MAX 256
+
+/* a response containing a PCM channel map information */
+struct virtio_snd_pcm_chmap_info {
+ le32 status;
+ le32 npositions;
+ u8 positions[VIRTIO_SND_PCM_CH_MAX];
+};
+\end{lstlisting}
+
+The PCM channel map information fields are:
+
+\begin{description}
+\item[\field{status}] (device-writable) specifies a device response status
+(VIRTIO_SND_S_*).
+\item[\field{npositions}] (device-writable) is a number of valid entries in
+the \field{positions} array.
+\item[\field{positions}] (device-writable) contains PCM channel positions
+(VIRTIO_SND_PCM_CH_*).
+\end{description}
+
+\item[VIRTIO_SND_R_PCM_SET_FORMAT]
+Set selected PCM format.
+
+\begin{lstlisting}
+struct virtio_snd_pcm_set_format {
+ struct virtio_snd_pcm_hdr hdr;
+ le16 channels;
+ le16 format;
+ le16 rate;
+ u16 padding;
+};
+\end{lstlisting}
+
+The PCM control request fields are:
+
+\begin{description}
+\item[\field{hdr}] (device-read-only) is a PCM control request header.
+\item[\field{channels}] (device-read-only) specifies a desired number of channels.
+\item[\field{format}] (device-read-only) specifies a desired PCM sample format
+(VIRTIO_SND_PCM_FMT_*).
+\item[\field{rate}] (device-read-only) specifies a desired PCM frame rate
+(VIRTIO_SND_PCM_RATE_*).
+\end{description}
+
+\item[VIRTIO_SND_R_PCM_PREPARE]
+Prepare the PCM device.
+
+\item[VIRTIO_SND_R_PCM_START]
+Start the PCM device.
+
+\item[VIRTIO_SND_R_PCM_STOP]
+Stop the PCM device.
+
+\item[VIRTIO_SND_R_PCM_PAUSE]
+Set the PCM device on pause.
+
+\item[VIRTIO_SND_R_PCM_UNPAUSE]
+Unset the PCM device from pause.
+
+\end{description}
+
+\devicenormative{\subsubsection}{PCM control requests}{Device Types / Sound Device / PCM control requests}
+
+In a VIRTIO_SND_R_PCM_CHMAP_INFO request:
+
+\begin{itemize*}
+\item if the device does not support a channel map for a specified stream type,
+then it MUST return the VIRTIO_SND_S_NOT_SUPP status code;
+\item if the operation failed, then the device MUST set the \field{npositions}
+field to 0.
+\end{itemize*}
+
+\drivernormative{\subsubsection}{PCM control requests}{Device Types / Sound Device / PCM control requests}
+
+If the VIRTIO_SND_F_PCM_OUTPUT feature is not negotiated, then the driver MUST NOT
+specify VIRTIO_SND_PCM_T_OUTPUT as a stream type
+
+If the VIRTIO_SND_F_PCM_INPUT feature is not negotiated, then the driver MUST NOT
+specify VIRTIO_SND_PCM_T_INPUT as a stream type.
+
+In a VIRTIO_SND_R_PCM_SET_FORMAT request:
+
+\begin{itemize*}
+\item the driver MUST NOT specify the \field{channels} value as less than the \field{channels_min}
+or greater than the \field{channels_max} values reported in the stream configuration;
+\item the driver MUST specify the \field{format} and \field{rate} values according
+to the \field{formats} and \field{rates} values reported in the stream configuration;
+\item the driver MUST NOT specify the not defined format and rate values;
+\item the driver MUST initialize the \field{padding} field to 0.
+\end{itemize*}
+
+\subsubsection{Device Operation: PCM I/O requests}
+
+All I/O requests are placed into the pcmq virtqueue. Each request is in the
+following form:
+
+\begin{lstlisting}
+struct virtio_snd_pcm_xfer {
+ le32 stream;
+ u8 data[];
+ le32 status;
+ le32 actual_length;
+};
+\end{lstlisting}
+
+I/O request fields:
+
+\begin{description}
+\item[\field{stream}] (device-read-only) specifies a PCM stream type
+(VIRTIO_SND_PCM_T_*).
+\item[\field{data}]
+\begin{enumerate}
+\item Output: (device-read-only) a buffer with PCM frames to be written to the
+device.
+\item Input: (device-writable) a buffer to be filled with PCM frames from the
+device.
+\end{enumerate}
+\item[\field{status}] (device-writable) contains VIRTIO_SND_S_OK if an operation
+is successful, and VIRTIO_SND_S_IO_ERR otherwise.
+\item[\field{actual_length}] (device-writable) specifies an actual amount of
+bytes that are read from/written to the \field{data} field.
+\end{description}
+
+\devicenormative{\subsubsection}{PCM I/O requests}{Device Types / Sound Device / PCM IO requests}
+
+\begin{enumerate}
+\item If the operation failed, then the device MUST set the \field{actual_length}
+field to 0.
+\end{enumerate}
+
+\drivernormative{\subsubsection}{PCM I/O requests}{Device Types / Sound Device / PCM IO requests}
+
+\begin{enumerate}
+\item If the VIRTIO_SND_F_PCM_OUTPUT feature is not negotiated, then the driver
+MUST NOT specify VIRTIO_SND_PCM_T_OUTPUT as a stream type.
+\item If the VIRTIO_SND_F_PCM_INPUT feature is not negotiated, then the driver
+MUST NOT specify VIRTIO_SND_PCM_T_INPUT as a stream type.
+\end{enumerate}
--
2.23.0
---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [virtio-dev] [PATCH v2] snd: Add virtio sound device specification
2019-11-05 15:06 [virtio-dev] [PATCH v2] snd: Add virtio sound device specification Mikhail Golubev
@ 2019-11-06 9:28 ` Mikhail Golubev
2019-11-11 15:28 ` Liam Girdwood
2019-11-12 13:20 ` Jean-Philippe Brucker
1 sibling, 1 reply; 8+ messages in thread
From: Mikhail Golubev @ 2019-11-06 9:28 UTC (permalink / raw)
To: virtio-dev
Cc: liam.r.girdwood, tiwai, broonie, maz, james.morse,
julien.thierry.kdev, suzuki.poulose, Anton Yakovlev
Fixes: https://github.com/oasis-tcs/virtio-spec/issues/54
Request for a vote.
BR,
Mikhail.
On 11/5/19 4:06 PM, Mikhail Golubev wrote:
> From: Anton Yakovlev <Anton.Yakovlev@opensynergy.com>
>
> Hi everyone,
>
> the patch proposes an updated version of virtio specification for a new
> virtio sound device. The initial PATCH can be found here: [1]. The
> device may be useful in case when having audio is required but a device
> passthrough or emulation is not an option.
>
> The virtio sound card supports output and input PCM streams. It reports
> stream capabilities through device configuration space and utilizes two
> virtqueues: one for control requests and one for I/O requests.
>
> Signed-off-by: Anton Yakovlev <Anton.Yakovlev@opensynergy.com>
> Signed-off-by: Mikhail Golubev <Mikhail.Golubev@opensynergy.com>
>
> v1 -> v2:
> * Add 5512 Hz and 384 kHz to supported frame rates
> * English grammar clean-up
>
> [1]: https://lists.oasis-open.org/archives/virtio-dev/201909/msg00130.html
>
> ---
> conformance.tex | 20 +++
> content.tex | 1 +
> virtio-sound.tex | 410 +++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 431 insertions(+)
> create mode 100644 virtio-sound.tex
>
> diff --git a/conformance.tex b/conformance.tex
> index 0ac58aa..31595bf 100644
> --- a/conformance.tex
> +++ b/conformance.tex
> @@ -183,6 +183,16 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
> \item \ref{drivernormative:Device Types / Socket Device / Device Operation / Device Events}
> \end{itemize}
>
> +\conformance{\subsection}{Sound Driver Conformance}\label{sec:Conformance / Driver Conformance / Sound Driver Conformance}
> +
> +A sound driver MUST conform to the following normative statements:
> +
> +\begin{itemize}
> +\item \ref{drivernormative:Device Types / Sound Device / Device Initialization}
> +\item \ref{drivernormative:Device Types / Sound Device / PCM control requests}
> +\item \ref{drivernormative:Device Types / Sound Device / PCM IO requests}
> +\end{itemize}
> +
> \conformance{\section}{Device Conformance}\label{sec:Conformance / Device Conformance}
>
> A device MUST conform to the following normative statements:
> @@ -338,6 +348,16 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
> \item \ref{devicenormative:Device Types / Socket Device / Device Operation / Receive and Transmit}
> \end{itemize}
>
> +\conformance{\subsection}{Sound Device Conformance}\label{sec:Conformance / Device Conformance / Sound Device Conformance}
> +
> +A sound device MUST conform to the following normative statements:
> +
> +\begin{itemize}
> +\item \ref{devicenormative:Device Types / Sound Device / Device Initialization}
> +\item \ref{devicenormative:Device Types / Sound Device / PCM control requests}
> +\item \ref{devicenormative:Device Types / Sound Device / PCM IO requests}
> +\end{itemize}
> +
> \conformance{\section}{Legacy Interface: Transitional Device and Transitional Driver Conformance}\label{sec:Conformance / Legacy Interface: Transitional Device and Transitional Driver Conformance}
> A conformant implementation MUST be either transitional or
> non-transitional, see \ref{intro:Legacy
> diff --git a/content.tex b/content.tex
> index b1ea9b9..aaae074 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -5698,6 +5698,7 @@ \subsubsection{Legacy Interface: Framing Requirements}\label{sec:Device
> \input{virtio-crypto.tex}
> \input{virtio-vsock.tex}
> \input{virtio-fs.tex}
> +\input{virtio-sound.tex}
>
> \chapter{Reserved Feature Bits}\label{sec:Reserved Feature Bits}
>
> diff --git a/virtio-sound.tex b/virtio-sound.tex
> new file mode 100644
> index 0000000..f73e772
> --- /dev/null
> +++ b/virtio-sound.tex
> @@ -0,0 +1,410 @@
> +\section{Sound Device}\label{sec:Device Types / Sound Device}
> +
> +The virtio sound card is a virtual audio device supporting output and input PCM
> +streams. All device control requests are placed into the control virtqueue and
> +all I/O requests are placed into the PCM virtqueue.
> +
> +\subsection{Device ID}\label{sec:Device Types / Sound Device / Device ID}
> +
> +25
> +
> +\subsection{Virtqueues}\label{sec:Device Types / Sound Device / Virtqueues}
> +
> +\begin{description}
> +\item[0] controlq
> +\item[1] pcmq
> +\end{description}
> +
> +The controlq virtqueue always exists, the pcmq virtqueue only exists if
> +the VIRTIO_SND_F_PCM_OUTPUT and/or VIRTIO_SND_F_PCM_INPUT feature is negotiated.
> +
> +\subsection{Feature bits}\label{sec:Device Types / Sound Device / Feature bits}
> +
> +\begin{description}
> +\item[VIRTIO_SND_F_PCM_OUTPUT (0)] Output PCM stream support.
> +\item[VIRTIO_SND_F_PCM_INPUT (1)] Input PCM stream support.
> +\end{description}
> +
> +\subsection{Device configuration layout}\label{sec:Device Types / Sound Device / Device configuration layout}
> +
> +\begin{lstlisting}
> +/* supported PCM sample formats */
> +enum {
> + VIRTIO_SND_PCM_FMT_S8 = 0,
> + VIRTIO_SND_PCM_FMT_U8,
> + VIRTIO_SND_PCM_FMT_S16,
> + VIRTIO_SND_PCM_FMT_U16,
> + VIRTIO_SND_PCM_FMT_S32,
> + VIRTIO_SND_PCM_FMT_U32,
> + VIRTIO_SND_PCM_FMT_FLOAT,
> + VIRTIO_SND_PCM_FMT_FLOAT64
> +};
> +
> +/* supported PCM frame rates */
> +enum {
> + VIRTIO_SND_PCM_RATE_5512 = 0,
> + VIRTIO_SND_PCM_RATE_8000,
> + VIRTIO_SND_PCM_RATE_11025,
> + VIRTIO_SND_PCM_RATE_16000,
> + VIRTIO_SND_PCM_RATE_22050,
> + VIRTIO_SND_PCM_RATE_32000,
> + VIRTIO_SND_PCM_RATE_44100,
> + VIRTIO_SND_PCM_RATE_48000,
> + VIRTIO_SND_PCM_RATE_64000,
> + VIRTIO_SND_PCM_RATE_88200,
> + VIRTIO_SND_PCM_RATE_96000,
> + VIRTIO_SND_PCM_RATE_176400,
> + VIRTIO_SND_PCM_RATE_192000,
> + VIRTIO_SND_PCM_RATE_384000
> +};
> +
> +/* a PCM stream configuration */
> +struct virtio_pcm_stream_config {
> + u8 channels_min;
> + u8 channels_max;
> + le16 formats; /* 1 << VIRTIO_SND_PCM_FMT_XXX */
> + le16 rates; /* 1 << VIRTIO_SND_PCM_RATE_XXX */
> + u16 padding;
> +};
> +
> +/* a device configuration space */
> +struct virtio_snd_config {
> + struct virtio_pcm_config {
> + struct virtio_pcm_stream_config output;
> + struct virtio_pcm_stream_config input;
> + } pcm;
> +};
> +\end{lstlisting}
> +
> +\subsubsection{Device configuration fields}
> +
> +The \field{pcm.output} and \field{pcm.input} fields contain PCM stream
> +configuration:
> +
> +\begin{description}
> +\item[\field{channels_min}] (driver-read-only) is a minimum number of supported
> +channels.
> +\item[\field{channels_max}] (driver-read-only) is a maximum number of supported
> +channels.
> +\item[\field{formats}] (driver-read-only) is a supported sample format bit map.
> +\item[\field{rates}] (driver-read-only) is a supported frame rate bit map.
> +\end{description}
> +
> +Only interleaved samples are supported.
> +
> +\subsection{Device Initialization}
> +
> +\begin{enumerate}
> +\item If the VIRTIO_SND_F_PCM_OUTPUT feature is negotiated, \field{pcm.output}
> +contains the valid output PCM stream configuration.
> +\item If the VIRTIO_SND_F_PCM_INPUT feature is negotiated, \field{pcm.input}
> +contains the valid input PCM stream configuration.
> +\end{enumerate}
> +
> +\devicenormative{\subsubsection}{Device Initialization}{Device Types / Sound Device / Device Initialization}
> +
> +\begin{enumerate}
> +\item The device MUST NOT set the not defined format and rate bits.
> +\item The device MUST initialize padding bytes \field{pcm.output.padding} and
> +\field{pcm.input.padding} to 0.
> +\end{enumerate}
> +
> +\drivernormative{\subsubsection}{Device Initialization}{Device Types / Sound Device / Device Initialization}
> +
> +\begin{enumerate}
> +\item The driver MUST configure and initialize all virtqueues.
> +\item The driver SHOULD ignore the not defined format and rate bits.
> +\end{enumerate}
> +
> +\subsection{Device Operation}\label{sec:Device Types / Sound Device / Device Operation}
> +
> +All control messages are placed into the controlq virtqueue and use the following
> +layout structure and definitions:
> +
> +\begin{lstlisting}
> +enum {
> + /* PCM control request types */
> + VIRTIO_SND_R_PCM_CHMAP_INFO = 0,
> + VIRTIO_SND_R_PCM_SET_FORMAT,
> + VIRTIO_SND_R_PCM_PREPARE,
> + VIRTIO_SND_R_PCM_START,
> + VIRTIO_SND_R_PCM_STOP,
> + VIRTIO_SND_R_PCM_PAUSE,
> + VIRTIO_SND_R_PCM_UNPAUSE,
> +
> + /* generic status codes */
> + VIRTIO_SND_S_OK = 0x8000,
> + VIRTIO_SND_S_BAD_MSG,
> + VIRTIO_SND_S_NOT_SUPP,
> + VIRTIO_SND_S_IO_ERR
> +};
> +
> +struct virtio_snd_ctl_msg {
> + /* device-read-only data */
> + le32 request_code;
> + u8 request_payload[];
> + /* device-writable data */
> + le32 response_status;
> + u8 response_payload[];
> +};
> +\end{lstlisting}
> +
> +A generic control message consists of a request part and a response part, and
> +contains the following fields:
> +
> +\begin{description}
> +\item[\field{request_code}] (device-read-only) specifies a device request code
> +(VIRTIO_SND_R_*).
> +\item[\field{request_payload}] (device-read-only) contains request-specific
> +data.
> +\item[\field{response_status}] (device-writable) specifies a device response
> +status (VIRTIO_SND_S_*).
> +\item[\field{response_payload}] (device-writable) contains response-specific
> +data.
> +\end{description}
> +
> +Unless stated otherwise, the \field{request_payload} and \field{response_payload}
> +fields are empty.
> +
> +The \field{response_status} field contains one of the following values:
> +
> +\begin{itemize*}
> +\item VIRTIO_SND_S_OK: success.
> +\item VIRTIO_SND_S_BAD_MSG: a control message is malformed or contains invalid
> +parameters.
> +\item VIRTIO_SND_S_NOT_SUPP: requested operation or parameters are not supported.
> +\item VIRTIO_SND_S_IO_ERR: an I/O error occurred.
> +\end{itemize*}
> +
> +\subsubsection{Device Operation: PCM control requests}
> +
> +A PCM stream has the following command lifecycle:
> +
> +\begin{enumerate}
> +\item Set format
> +\item Prepare
> +\item Output only: transfer data for prebuffing
> +\item Start
> +\item Transfer data to/from the PCM device
> +\begin{enumerate}
> + \item Pause
> + \item Unpause
> +\end{enumerate}
> +\item Stop
> +\end{enumerate}
> +
> +PCM control requests have or consist of a fixed header with the following
> +layout structure:
> +
> +\begin{lstlisting}
> +/* supported PCM stream types */
> +enum {
> + VIRTIO_SND_PCM_T_OUTPUT = 0,
> + VIRTIO_SND_PCM_T_INPUT
> +};
> +
> +/* a PCM control request header */
> +struct virtio_snd_pcm_hdr {
> + le32 code;
> + le32 stream;
> +};
> +\end{lstlisting}
> +
> +PCM control requests contain the following fields:
> +
> +\begin{description}
> +\item[\field{code}] (device-read-only) specifies a PCM device request code
> +(VIRTIO_SND_R_PCM_*).
> +\item[\field{stream}] (device-read-only) specifies a PCM stream type
> +(VIRTIO_SND_PCM_T_*).
> +\end{description}
> +
> +\begin{description}
> +
> +\item[VIRTIO_SND_R_PCM_CHMAP_INFO]
> +Query a PCM channel map information for specified stream type.
> +
> +A response uses the following layout structure and definitions:
> +
> +\begin{lstlisting}
> +/* standard channel position definition */
> +enum {
> + VIRTIO_SND_PCM_CH_NONE = 0, /* undefined */
> + VIRTIO_SND_PCM_CH_NA, /* silent */
> + VIRTIO_SND_PCM_CH_MONO, /* mono stream */
> + VIRTIO_SND_PCM_CH_FL, /* front left */
> + VIRTIO_SND_PCM_CH_FR, /* front right */
> + VIRTIO_SND_PCM_CH_RL, /* rear left */
> + VIRTIO_SND_PCM_CH_RR, /* rear right */
> + VIRTIO_SND_PCM_CH_FC, /* front center */
> + VIRTIO_SND_PCM_CH_LFE, /* low frequency (LFE) */
> + VIRTIO_SND_PCM_CH_SL, /* side left */
> + VIRTIO_SND_PCM_CH_SR, /* side right */
> + VIRTIO_SND_PCM_CH_RC, /* rear center */
> + VIRTIO_SND_PCM_CH_FLC, /* front left center */
> + VIRTIO_SND_PCM_CH_FRC, /* front right center */
> + VIRTIO_SND_PCM_CH_RLC, /* rear left center */
> + VIRTIO_SND_PCM_CH_RRC, /* rear right center */
> + VIRTIO_SND_PCM_CH_FLW, /* front left wide */
> + VIRTIO_SND_PCM_CH_FRW, /* front right wide */
> + VIRTIO_SND_PCM_CH_FLH, /* front left high */
> + VIRTIO_SND_PCM_CH_FCH, /* front center high */
> + VIRTIO_SND_PCM_CH_FRH, /* front right high */
> + VIRTIO_SND_PCM_CH_TC, /* top center */
> + VIRTIO_SND_PCM_CH_TFL, /* top front left */
> + VIRTIO_SND_PCM_CH_TFR, /* top front right */
> + VIRTIO_SND_PCM_CH_TFC, /* top front center */
> + VIRTIO_SND_PCM_CH_TRL, /* top rear left */
> + VIRTIO_SND_PCM_CH_TRR, /* top rear right */
> + VIRTIO_SND_PCM_CH_TRC, /* top rear center */
> + VIRTIO_SND_PCM_CH_TFLC, /* top front left center */
> + VIRTIO_SND_PCM_CH_TFRC, /* top front right center */
> + VIRTIO_SND_PCM_CH_TSL, /* top side left */
> + VIRTIO_SND_PCM_CH_TSR, /* top side right */
> + VIRTIO_SND_PCM_CH_LLFE, /* left LFE */
> + VIRTIO_SND_PCM_CH_RLFE, /* right LFE */
> + VIRTIO_SND_PCM_CH_BC, /* bottom center */
> + VIRTIO_SND_PCM_CH_BLC, /* bottom left center */
> + VIRTIO_SND_PCM_CH_BRC /* bottom right center */
> +};
> +
> +/* a maximum possible number of channels */
> +#define VIRTIO_SND_PCM_CH_MAX 256
> +
> +/* a response containing a PCM channel map information */
> +struct virtio_snd_pcm_chmap_info {
> + le32 status;
> + le32 npositions;
> + u8 positions[VIRTIO_SND_PCM_CH_MAX];
> +};
> +\end{lstlisting}
> +
> +The PCM channel map information fields are:
> +
> +\begin{description}
> +\item[\field{status}] (device-writable) specifies a device response status
> +(VIRTIO_SND_S_*).
> +\item[\field{npositions}] (device-writable) is a number of valid entries in
> +the \field{positions} array.
> +\item[\field{positions}] (device-writable) contains PCM channel positions
> +(VIRTIO_SND_PCM_CH_*).
> +\end{description}
> +
> +\item[VIRTIO_SND_R_PCM_SET_FORMAT]
> +Set selected PCM format.
> +
> +\begin{lstlisting}
> +struct virtio_snd_pcm_set_format {
> + struct virtio_snd_pcm_hdr hdr;
> + le16 channels;
> + le16 format;
> + le16 rate;
> + u16 padding;
> +};
> +\end{lstlisting}
> +
> +The PCM control request fields are:
> +
> +\begin{description}
> +\item[\field{hdr}] (device-read-only) is a PCM control request header.
> +\item[\field{channels}] (device-read-only) specifies a desired number of channels.
> +\item[\field{format}] (device-read-only) specifies a desired PCM sample format
> +(VIRTIO_SND_PCM_FMT_*).
> +\item[\field{rate}] (device-read-only) specifies a desired PCM frame rate
> +(VIRTIO_SND_PCM_RATE_*).
> +\end{description}
> +
> +\item[VIRTIO_SND_R_PCM_PREPARE]
> +Prepare the PCM device.
> +
> +\item[VIRTIO_SND_R_PCM_START]
> +Start the PCM device.
> +
> +\item[VIRTIO_SND_R_PCM_STOP]
> +Stop the PCM device.
> +
> +\item[VIRTIO_SND_R_PCM_PAUSE]
> +Set the PCM device on pause.
> +
> +\item[VIRTIO_SND_R_PCM_UNPAUSE]
> +Unset the PCM device from pause.
> +
> +\end{description}
> +
> +\devicenormative{\subsubsection}{PCM control requests}{Device Types / Sound Device / PCM control requests}
> +
> +In a VIRTIO_SND_R_PCM_CHMAP_INFO request:
> +
> +\begin{itemize*}
> +\item if the device does not support a channel map for a specified stream type,
> +then it MUST return the VIRTIO_SND_S_NOT_SUPP status code;
> +\item if the operation failed, then the device MUST set the \field{npositions}
> +field to 0.
> +\end{itemize*}
> +
> +\drivernormative{\subsubsection}{PCM control requests}{Device Types / Sound Device / PCM control requests}
> +
> +If the VIRTIO_SND_F_PCM_OUTPUT feature is not negotiated, then the driver MUST NOT
> +specify VIRTIO_SND_PCM_T_OUTPUT as a stream type
> +
> +If the VIRTIO_SND_F_PCM_INPUT feature is not negotiated, then the driver MUST NOT
> +specify VIRTIO_SND_PCM_T_INPUT as a stream type.
> +
> +In a VIRTIO_SND_R_PCM_SET_FORMAT request:
> +
> +\begin{itemize*}
> +\item the driver MUST NOT specify the \field{channels} value as less than the \field{channels_min}
> +or greater than the \field{channels_max} values reported in the stream configuration;
> +\item the driver MUST specify the \field{format} and \field{rate} values according
> +to the \field{formats} and \field{rates} values reported in the stream configuration;
> +\item the driver MUST NOT specify the not defined format and rate values;
> +\item the driver MUST initialize the \field{padding} field to 0.
> +\end{itemize*}
> +
> +\subsubsection{Device Operation: PCM I/O requests}
> +
> +All I/O requests are placed into the pcmq virtqueue. Each request is in the
> +following form:
> +
> +\begin{lstlisting}
> +struct virtio_snd_pcm_xfer {
> + le32 stream;
> + u8 data[];
> + le32 status;
> + le32 actual_length;
> +};
> +\end{lstlisting}
> +
> +I/O request fields:
> +
> +\begin{description}
> +\item[\field{stream}] (device-read-only) specifies a PCM stream type
> +(VIRTIO_SND_PCM_T_*).
> +\item[\field{data}]
> +\begin{enumerate}
> +\item Output: (device-read-only) a buffer with PCM frames to be written to the
> +device.
> +\item Input: (device-writable) a buffer to be filled with PCM frames from the
> +device.
> +\end{enumerate}
> +\item[\field{status}] (device-writable) contains VIRTIO_SND_S_OK if an operation
> +is successful, and VIRTIO_SND_S_IO_ERR otherwise.
> +\item[\field{actual_length}] (device-writable) specifies an actual amount of
> +bytes that are read from/written to the \field{data} field.
> +\end{description}
> +
> +\devicenormative{\subsubsection}{PCM I/O requests}{Device Types / Sound Device / PCM IO requests}
> +
> +\begin{enumerate}
> +\item If the operation failed, then the device MUST set the \field{actual_length}
> +field to 0.
> +\end{enumerate}
> +
> +\drivernormative{\subsubsection}{PCM I/O requests}{Device Types / Sound Device / PCM IO requests}
> +
> +\begin{enumerate}
> +\item If the VIRTIO_SND_F_PCM_OUTPUT feature is not negotiated, then the driver
> +MUST NOT specify VIRTIO_SND_PCM_T_OUTPUT as a stream type.
> +\item If the VIRTIO_SND_F_PCM_INPUT feature is not negotiated, then the driver
> +MUST NOT specify VIRTIO_SND_PCM_T_INPUT as a stream type.
> +\end{enumerate}
>
---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [virtio-dev] [PATCH v2] snd: Add virtio sound device specification
2019-11-06 9:28 ` Mikhail Golubev
@ 2019-11-11 15:28 ` Liam Girdwood
2019-11-14 13:23 ` Anton Yakovlev
0 siblings, 1 reply; 8+ messages in thread
From: Liam Girdwood @ 2019-11-11 15:28 UTC (permalink / raw)
To: Mikhail Golubev, virtio-dev
Cc: tiwai, broonie, maz, james.morse, julien.thierry.kdev,
suzuki.poulose, Anton Yakovlev
On Wed, 2019-11-06 at 10:28 +0100, Mikhail Golubev wrote:
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/54
>
> Request for a vote.
Sorry, I think there are still too many opens. I would also recommend
that this proposal be sent to ALSA devel list (subscribers only) to get
a more thorough review since it will be difficult to fix once deployed.
Can you also provide more details on the use case testing carried out
here ?
>
> BR,
> Mikhail.
>
> On 11/5/19 4:06 PM, Mikhail Golubev wrote:
> > From: Anton Yakovlev <Anton.Yakovlev@opensynergy.com>
> >
snip
> >
> > +\subsection{Device Operation}\label{sec:Device Types / Sound
> > Device / Device Operation}
> > +
> > +All control messages are placed into the controlq virtqueue and
> > use the following
> > +layout structure and definitions:
> > +
> > +\begin{lstlisting}
> > +enum {
> > + /* PCM control request types */
> > + VIRTIO_SND_R_PCM_CHMAP_INFO = 0,
> > + VIRTIO_SND_R_PCM_SET_FORMAT,
> > + VIRTIO_SND_R_PCM_PREPARE,
> > + VIRTIO_SND_R_PCM_START,
> > + VIRTIO_SND_R_PCM_STOP,
> > + VIRTIO_SND_R_PCM_PAUSE,
> > + VIRTIO_SND_R_PCM_UNPAUSE,
Where do I tear down the PCM when I'm finished with it ? i.e. to free
resources.
> > +
> > + /* generic status codes */
> > + VIRTIO_SND_S_OK = 0x8000,
> > + VIRTIO_SND_S_BAD_MSG,
> > + VIRTIO_SND_S_NOT_SUPP,
> > + VIRTIO_SND_S_IO_ERR
> > +};
> > +
> > +struct virtio_snd_ctl_msg {
> > + /* device-read-only data */
> > + le32 request_code;
> > + u8 request_payload[];
> > + /* device-writable data */
> > + le32 response_status;
> > + u8 response_payload[];
> > +};
> > +\end{lstlisting}
> > +
Using request_code for payload size is just asking for trouble.
> >
> > +
> > +\subsubsection{Device Operation: PCM I/O requests}
> > +
> > +All I/O requests are placed into the pcmq virtqueue. Each request
> > is in the
> > +following form:
> > +
> > +\begin{lstlisting}
> > +struct virtio_snd_pcm_xfer {
> > + le32 stream;
> > + u8 data[];
> > + le32 status;
> > + le32 actual_length;
> > +};
Using period/fragment/block size for PCM means we only copy the minimum
data necessary and dont have to pad.
Thanks
Liam
---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [virtio-dev] [PATCH v2] snd: Add virtio sound device specification
2019-11-05 15:06 [virtio-dev] [PATCH v2] snd: Add virtio sound device specification Mikhail Golubev
2019-11-06 9:28 ` Mikhail Golubev
@ 2019-11-12 13:20 ` Jean-Philippe Brucker
2019-11-14 14:45 ` Anton Yakovlev
1 sibling, 1 reply; 8+ messages in thread
From: Jean-Philippe Brucker @ 2019-11-12 13:20 UTC (permalink / raw)
To: Mikhail Golubev
Cc: virtio-dev, liam.r.girdwood, tiwai, broonie, maz, james.morse,
julien.thierry.kdev, suzuki.poulose, Anton Yakovlev
Hi Mikhail,
On Tue, Nov 05, 2019 at 04:06:06PM +0100, Mikhail Golubev wrote:
> From: Anton Yakovlev <Anton.Yakovlev@opensynergy.com>
>
> Hi everyone,
>
> the patch proposes an updated version of virtio specification for a new
> virtio sound device. The initial PATCH can be found here: [1]. The
> device may be useful in case when having audio is required but a device
> passthrough or emulation is not an option.
>
> The virtio sound card supports output and input PCM streams. It reports
> stream capabilities through device configuration space and utilizes two
> virtqueues: one for control requests and one for I/O requests.
Did you send out the implementation prototypes as well? It might help
understand how the device works.
[...]
> +\subsection{Virtqueues}\label{sec:Device Types / Sound Device / Virtqueues}
> +
> +\begin{description}
> +\item[0] controlq
> +\item[1] pcmq
> +\end{description}
> +
> +The controlq virtqueue always exists, the pcmq virtqueue only exists if
> +the VIRTIO_SND_F_PCM_OUTPUT and/or VIRTIO_SND_F_PCM_INPUT feature is negotiated.
Why not use two queues, one for input and one for output? I think most
virtio devices do it that way. It could simplify the implementations,
shave 32 bits off the buffers, and allow to quiesce notifications for
input and output streams independently. And in the future you can extend
the device with more than two streams.
> +
> +\subsection{Feature bits}\label{sec:Device Types / Sound Device / Feature bits}
> +
> +\begin{description}
> +\item[VIRTIO_SND_F_PCM_OUTPUT (0)] Output PCM stream support.
> +\item[VIRTIO_SND_F_PCM_INPUT (1)] Input PCM stream support.
> +\end{description}
> +
> +\subsection{Device configuration layout}\label{sec:Device Types / Sound Device / Device configuration layout}
> +
> +\begin{lstlisting}
> +/* supported PCM sample formats */
> +enum {
> + VIRTIO_SND_PCM_FMT_S8 = 0,
> + VIRTIO_SND_PCM_FMT_U8,
> + VIRTIO_SND_PCM_FMT_S16,
> + VIRTIO_SND_PCM_FMT_U16,
> + VIRTIO_SND_PCM_FMT_S32,
> + VIRTIO_SND_PCM_FMT_U32,
> + VIRTIO_SND_PCM_FMT_FLOAT,
> + VIRTIO_SND_PCM_FMT_FLOAT64
> +};
Is it worth adding a word or two about the encoding used, in particular
that this is linear quantization? Or maybe it's standard enough, I don't
know much about audio.
> +
> +/* supported PCM frame rates */
> +enum {
> + VIRTIO_SND_PCM_RATE_5512 = 0,
> + VIRTIO_SND_PCM_RATE_8000,
> + VIRTIO_SND_PCM_RATE_11025,
> + VIRTIO_SND_PCM_RATE_16000,
> + VIRTIO_SND_PCM_RATE_22050,
> + VIRTIO_SND_PCM_RATE_32000,
> + VIRTIO_SND_PCM_RATE_44100,
> + VIRTIO_SND_PCM_RATE_48000,
> + VIRTIO_SND_PCM_RATE_64000,
> + VIRTIO_SND_PCM_RATE_88200,
> + VIRTIO_SND_PCM_RATE_96000,
> + VIRTIO_SND_PCM_RATE_176400,
> + VIRTIO_SND_PCM_RATE_192000,
> + VIRTIO_SND_PCM_RATE_384000
> +};
> +
> +/* a PCM stream configuration */
> +struct virtio_pcm_stream_config {
> + u8 channels_min;
> + u8 channels_max;
> + le16 formats; /* 1 << VIRTIO_SND_PCM_FMT_XXX */
> + le16 rates; /* 1 << VIRTIO_SND_PCM_RATE_XXX */
> + u16 padding;
You could add a lot more padding, because the current layout doesn't allow
to extend virtio_pcm_stream_config much (an old driver will expect the
pcm.input field to start at offset 0x8 of the config space).
> +};
> +
> +/* a device configuration space */
> +struct virtio_snd_config {
> + struct virtio_pcm_config {
> + struct virtio_pcm_stream_config output;
> + struct virtio_pcm_stream_config input;
> + } pcm;
> +};
> +\end{lstlisting}
> +
> +\subsubsection{Device configuration fields}
> +
> +The \field{pcm.output} and \field{pcm.input} fields contain PCM stream
> +configuration:
> +
> +\begin{description}
> +\item[\field{channels_min}] (driver-read-only) is a minimum number of supported
> +channels.
> +\item[\field{channels_max}] (driver-read-only) is a maximum number of supported
> +channels.
> +\item[\field{formats}] (driver-read-only) is a supported sample format bit map.
> +\item[\field{rates}] (driver-read-only) is a supported frame rate bit map.
> +\end{description}
> +
> +Only interleaved samples are supported.
Similar noob remark: I guess this is standard in audio hardware and
doesn't need more explanation? If the term is alsa-specific then we'd
need a few words about what the format looks like.
> +
> +\subsection{Device Initialization}
> +
> +\begin{enumerate}
> +\item If the VIRTIO_SND_F_PCM_OUTPUT feature is negotiated, \field{pcm.output}
> +contains the valid output PCM stream configuration.
> +\item If the VIRTIO_SND_F_PCM_INPUT feature is negotiated, \field{pcm.input}
> +contains the valid input PCM stream configuration.
> +\end{enumerate}
> +
> +\devicenormative{\subsubsection}{Device Initialization}{Device Types / Sound Device / Device Initialization}
> +
> +\begin{enumerate}
> +\item The device MUST NOT set the not defined format and rate bits.
s/not defined/undefined/ (here and elsewhere)
> +\item The device MUST initialize padding bytes \field{pcm.output.padding} and
> +\field{pcm.input.padding} to 0.
> +\end{enumerate}
> +
> +\drivernormative{\subsubsection}{Device Initialization}{Device Types / Sound Device / Device Initialization}
> +
> +\begin{enumerate}
> +\item The driver MUST configure and initialize all virtqueues.
> +\item The driver SHOULD ignore the not defined format and rate bits.
> +\end{enumerate}
> +
> +\subsection{Device Operation}\label{sec:Device Types / Sound Device / Device Operation}
> +
> +All control messages are placed into the controlq virtqueue and use the following
> +layout structure and definitions:
> +
> +\begin{lstlisting}
> +enum {
> + /* PCM control request types */
> + VIRTIO_SND_R_PCM_CHMAP_INFO = 0,
> + VIRTIO_SND_R_PCM_SET_FORMAT,
> + VIRTIO_SND_R_PCM_PREPARE,
> + VIRTIO_SND_R_PCM_START,
> + VIRTIO_SND_R_PCM_STOP,
> + VIRTIO_SND_R_PCM_PAUSE,
> + VIRTIO_SND_R_PCM_UNPAUSE,
> +
> + /* generic status codes */
> + VIRTIO_SND_S_OK = 0x8000,
> + VIRTIO_SND_S_BAD_MSG,
> + VIRTIO_SND_S_NOT_SUPP,
> + VIRTIO_SND_S_IO_ERR
> +};
Any reason not to use two separate enums starting from 0?
> +
> +struct virtio_snd_ctl_msg {
> + /* device-read-only data */
> + le32 request_code;
> + u8 request_payload[];
> + /* device-writable data */
> + le32 response_status;
> + u8 response_payload[];
> +};
> +\end{lstlisting}
> +
> +A generic control message consists of a request part and a response part, and
> +contains the following fields:
> +
> +\begin{description}
> +\item[\field{request_code}] (device-read-only) specifies a device request code
> +(VIRTIO_SND_R_*).
> +\item[\field{request_payload}] (device-read-only) contains request-specific
> +data.
> +\item[\field{response_status}] (device-writable) specifies a device response
> +status (VIRTIO_SND_S_*).
> +\item[\field{response_payload}] (device-writable) contains response-specific
> +data.
> +\end{description}
> +
> +Unless stated otherwise, the \field{request_payload} and \field{response_payload}
> +fields are empty.
> +
> +The \field{response_status} field contains one of the following values:
> +
> +\begin{itemize*}
Nit: other devices use the "description" environment for statuses. (By the
way, what does the asterisk after itemize do?)
> +\item VIRTIO_SND_S_OK: success.
> +\item VIRTIO_SND_S_BAD_MSG: a control message is malformed or contains invalid
> +parameters.
> +\item VIRTIO_SND_S_NOT_SUPP: requested operation or parameters are not supported.
> +\item VIRTIO_SND_S_IO_ERR: an I/O error occurred.
> +\end{itemize*}
> +
> +\subsubsection{Device Operation: PCM control requests}
> +
> +A PCM stream has the following command lifecycle:
> +
> +\begin{enumerate}
> +\item Set format
> +\item Prepare
> +\item Output only: transfer data for prebuffing
> +\item Start
> +\item Transfer data to/from the PCM device
> +\begin{enumerate}
> + \item Pause
> + \item Unpause
> +\end{enumerate}
> +\item Stop
Which commands are allowed after Stop, just Set format or can we send
Start directly?
> +\end{enumerate}
> +
> +PCM control requests have or consist of a fixed header with the following
> +layout structure:
> +
> +\begin{lstlisting}
> +/* supported PCM stream types */
> +enum {
> + VIRTIO_SND_PCM_T_OUTPUT = 0,
> + VIRTIO_SND_PCM_T_INPUT
> +};
> +
> +/* a PCM control request header */
> +struct virtio_snd_pcm_hdr {
> + le32 code;
> + le32 stream;
> +};
> +\end{lstlisting}
> +
> +PCM control requests contain the following fields:
> +
> +\begin{description}
> +\item[\field{code}] (device-read-only) specifies a PCM device request code
> +(VIRTIO_SND_R_PCM_*).
> +\item[\field{stream}] (device-read-only) specifies a PCM stream type
> +(VIRTIO_SND_PCM_T_*).
> +\end{description}
> +
> +\begin{description}
> +
> +\item[VIRTIO_SND_R_PCM_CHMAP_INFO]
Nit: there is a lot of content in some control requests, it might look
nicer if they each have their own \paragraph{} instead of being
description items.
> +Query a PCM channel map information for specified stream type.
> +
> +A response uses the following layout structure and definitions:
> +
> +\begin{lstlisting}
> +/* standard channel position definition */
> +enum {
> + VIRTIO_SND_PCM_CH_NONE = 0, /* undefined */
> + VIRTIO_SND_PCM_CH_NA, /* silent */
> + VIRTIO_SND_PCM_CH_MONO, /* mono stream */
> + VIRTIO_SND_PCM_CH_FL, /* front left */
> + VIRTIO_SND_PCM_CH_FR, /* front right */
> + VIRTIO_SND_PCM_CH_RL, /* rear left */
> + VIRTIO_SND_PCM_CH_RR, /* rear right */
> + VIRTIO_SND_PCM_CH_FC, /* front center */
> + VIRTIO_SND_PCM_CH_LFE, /* low frequency (LFE) */
> + VIRTIO_SND_PCM_CH_SL, /* side left */
> + VIRTIO_SND_PCM_CH_SR, /* side right */
> + VIRTIO_SND_PCM_CH_RC, /* rear center */
> + VIRTIO_SND_PCM_CH_FLC, /* front left center */
> + VIRTIO_SND_PCM_CH_FRC, /* front right center */
> + VIRTIO_SND_PCM_CH_RLC, /* rear left center */
> + VIRTIO_SND_PCM_CH_RRC, /* rear right center */
> + VIRTIO_SND_PCM_CH_FLW, /* front left wide */
> + VIRTIO_SND_PCM_CH_FRW, /* front right wide */
> + VIRTIO_SND_PCM_CH_FLH, /* front left high */
> + VIRTIO_SND_PCM_CH_FCH, /* front center high */
> + VIRTIO_SND_PCM_CH_FRH, /* front right high */
> + VIRTIO_SND_PCM_CH_TC, /* top center */
> + VIRTIO_SND_PCM_CH_TFL, /* top front left */
> + VIRTIO_SND_PCM_CH_TFR, /* top front right */
> + VIRTIO_SND_PCM_CH_TFC, /* top front center */
> + VIRTIO_SND_PCM_CH_TRL, /* top rear left */
> + VIRTIO_SND_PCM_CH_TRR, /* top rear right */
> + VIRTIO_SND_PCM_CH_TRC, /* top rear center */
> + VIRTIO_SND_PCM_CH_TFLC, /* top front left center */
> + VIRTIO_SND_PCM_CH_TFRC, /* top front right center */
> + VIRTIO_SND_PCM_CH_TSL, /* top side left */
> + VIRTIO_SND_PCM_CH_TSR, /* top side right */
> + VIRTIO_SND_PCM_CH_LLFE, /* left LFE */
> + VIRTIO_SND_PCM_CH_RLFE, /* right LFE */
> + VIRTIO_SND_PCM_CH_BC, /* bottom center */
> + VIRTIO_SND_PCM_CH_BLC, /* bottom left center */
> + VIRTIO_SND_PCM_CH_BRC /* bottom right center */
> +};
> +
> +/* a maximum possible number of channels */
> +#define VIRTIO_SND_PCM_CH_MAX 256
> +
> +/* a response containing a PCM channel map information */
> +struct virtio_snd_pcm_chmap_info {
Including the header would be more consistent with set_format below.
> + le32 status;
> + le32 npositions;
> + u8 positions[VIRTIO_SND_PCM_CH_MAX];
> +};
> +\end{lstlisting}
> +
> +The PCM channel map information fields are:
> +
> +\begin{description}
> +\item[\field{status}] (device-writable) specifies a device response status
> +(VIRTIO_SND_S_*).
> +\item[\field{npositions}] (device-writable) is a number of valid entries in
> +the \field{positions} array.
> +\item[\field{positions}] (device-writable) contains PCM channel positions
> +(VIRTIO_SND_PCM_CH_*).
> +\end{description}
> +
> +\item[VIRTIO_SND_R_PCM_SET_FORMAT]
> +Set selected PCM format.
> +
> +\begin{lstlisting}
> +struct virtio_snd_pcm_set_format {
> + struct virtio_snd_pcm_hdr hdr;
> + le16 channels;
> + le16 format;
> + le16 rate;
> + u16 padding;
> +};
> +\end{lstlisting}
> +
> +The PCM control request fields are:
> +
> +\begin{description}
> +\item[\field{hdr}] (device-read-only) is a PCM control request header.
> +\item[\field{channels}] (device-read-only) specifies a desired number of channels.
> +\item[\field{format}] (device-read-only) specifies a desired PCM sample format
> +(VIRTIO_SND_PCM_FMT_*).
> +\item[\field{rate}] (device-read-only) specifies a desired PCM frame rate
> +(VIRTIO_SND_PCM_RATE_*).
> +\end{description}
> +
> +\item[VIRTIO_SND_R_PCM_PREPARE]
> +Prepare the PCM device.
Isn't that for one stream rather than the whole device? Otherwise, what
should the hdr.stream field contain?
> +
> +\item[VIRTIO_SND_R_PCM_START]
> +Start the PCM device.
> +
> +\item[VIRTIO_SND_R_PCM_STOP]
> +Stop the PCM device.
> +
> +\item[VIRTIO_SND_R_PCM_PAUSE]
> +Set the PCM device on pause.
> +
> +\item[VIRTIO_SND_R_PCM_UNPAUSE]
> +Unset the PCM device from pause.
Or "Resume the paused PCM device"?
> +
> +\end{description}
> +
> +\devicenormative{\subsubsection}{PCM control requests}{Device Types / Sound Device / PCM control requests}
> +
> +In a VIRTIO_SND_R_PCM_CHMAP_INFO request:
> +
> +\begin{itemize*}
> +\item if the device does not support a channel map for a specified stream type,
> +then it MUST return the VIRTIO_SND_S_NOT_SUPP status code;
> +\item if the operation failed, then the device MUST set the \field{npositions}
> +field to 0.
> +\end{itemize*}
> +
> +\drivernormative{\subsubsection}{PCM control requests}{Device Types / Sound Device / PCM control requests}
> +
> +If the VIRTIO_SND_F_PCM_OUTPUT feature is not negotiated, then the driver MUST NOT
> +specify VIRTIO_SND_PCM_T_OUTPUT as a stream type
> +
> +If the VIRTIO_SND_F_PCM_INPUT feature is not negotiated, then the driver MUST NOT
> +specify VIRTIO_SND_PCM_T_INPUT as a stream type.
> +
> +In a VIRTIO_SND_R_PCM_SET_FORMAT request:
> +
> +\begin{itemize*}
> +\item the driver MUST NOT specify the \field{channels} value as less than the \field{channels_min}
> +or greater than the \field{channels_max} values reported in the stream configuration;
> +\item the driver MUST specify the \field{format} and \field{rate} values according
> +to the \field{formats} and \field{rates} values reported in the stream configuration;
> +\item the driver MUST NOT specify the not defined format and rate values;
> +\item the driver MUST initialize the \field{padding} field to 0.
> +\end{itemize*}
I guess configuration requests for a stream that isn't in "Set format"
state are illegal? Probably needs to be stated here.
Thanks,
Jean
---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [virtio-dev] [PATCH v2] snd: Add virtio sound device specification
2019-11-11 15:28 ` Liam Girdwood
@ 2019-11-14 13:23 ` Anton Yakovlev
2019-11-19 15:14 ` Liam Girdwood
0 siblings, 1 reply; 8+ messages in thread
From: Anton Yakovlev @ 2019-11-14 13:23 UTC (permalink / raw)
To: Liam Girdwood, Mikhail Golubev, virtio-dev
Cc: tiwai, broonie, maz, james.morse, julien.thierry.kdev, suzuki.poulose
On 11.11.2019 16:28, Liam Girdwood wrote:
> On Wed, 2019-11-06 at 10:28 +0100, Mikhail Golubev wrote:
>> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/54
>>
snip
>>>
>>> +\subsection{Device Operation}\label{sec:Device Types / Sound
>>> Device / Device Operation}
>>> +
>>> +All control messages are placed into the controlq virtqueue and
>>> use the following
>>> +layout structure and definitions:
>>> +
>>> +\begin{lstlisting}
>>> +enum {
>>> + /* PCM control request types */
>>> + VIRTIO_SND_R_PCM_CHMAP_INFO = 0,
>>> + VIRTIO_SND_R_PCM_SET_FORMAT,
>>> + VIRTIO_SND_R_PCM_PREPARE,
>>> + VIRTIO_SND_R_PCM_START,
>>> + VIRTIO_SND_R_PCM_STOP,
>>> + VIRTIO_SND_R_PCM_PAUSE,
>>> + VIRTIO_SND_R_PCM_UNPAUSE,
>
> Where do I tear down the PCM when I'm finished with it ? i.e. to free
> resources.
Current workflow:
- a PCM stream is preparing with the set_format/prepare/start requests
- if necessary, resources are freed/released with the stop request
If needed, the stream can be temporary paused (the pause/unpause requests).
snip
--
Anton Yakovlev
Senior Software Engineer
OpenSynergy GmbH
Rotherstr. 20, 10245 Berlin
Phone: +49 30 60 98 54 0
E-Mail: anton.yakovlev@opensynergy.com
---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [virtio-dev] [PATCH v2] snd: Add virtio sound device specification
2019-11-12 13:20 ` Jean-Philippe Brucker
@ 2019-11-14 14:45 ` Anton Yakovlev
2019-11-20 11:09 ` Gerd Hoffmann
0 siblings, 1 reply; 8+ messages in thread
From: Anton Yakovlev @ 2019-11-14 14:45 UTC (permalink / raw)
To: Jean-Philippe Brucker, Mikhail Golubev
Cc: virtio-dev, liam.r.girdwood, tiwai, broonie, maz, james.morse,
julien.thierry.kdev, suzuki.poulose
Hi Jean,
Thank you for comments and sorry for delayed reply.
On 12.11.2019 14:20, Jean-Philippe Brucker wrote:
> Hi Mikhail,
>
> On Tue, Nov 05, 2019 at 04:06:06PM +0100, Mikhail Golubev wrote:
>> From: Anton Yakovlev <Anton.Yakovlev@opensynergy.com>
>>
>> Hi everyone,
>>
>> the patch proposes an updated version of virtio specification for a new
>> virtio sound device. The initial PATCH can be found here: [1]. The
>> device may be useful in case when having audio is required but a device
>> passthrough or emulation is not an option.
>>
>> The virtio sound card supports output and input PCM streams. It reports
>> stream capabilities through device configuration space and utilizes two
>> virtqueues: one for control requests and one for I/O requests.
>
> Did you send out the implementation prototypes as well? It might help
> understand how the device works.
Yes, we have both a device and a driver prototypes. We didn't send them yet
because there's no stable protocol at the moment. If you are interested, you
can take a look into our repositories at github:
QEmu device:
https://github.com/OpenSynergy/qemu/tree/virtio-snd-draft
Linux kernel driver:
https://github.com/OpenSynergy/linux/tree/virtio-snd-draft
At the moment they are both outdated (since we reworked part of the spec), but
we will update it in a few days.
> [...]
>> +\subsection{Virtqueues}\label{sec:Device Types / Sound Device / Virtqueues}
>> +
>> +\begin{description}
>> +\item[0] controlq
>> +\item[1] pcmq
>> +\end{description}
>> +
>> +The controlq virtqueue always exists, the pcmq virtqueue only exists if
>> +the VIRTIO_SND_F_PCM_OUTPUT and/or VIRTIO_SND_F_PCM_INPUT feature is negotiated.
>
> Why not use two queues, one for input and one for output? I think most
> virtio devices do it that way. It could simplify the implementations,
> shave 32 bits off the buffers, and allow to quiesce notifications for
> input and output streams independently. And in the future you can extend
> the device with more than two streams.
This was discussed earlier (having two separate virtqueues vs multiplexing
into single one). It's hard to say what is better, but at least one virtqueue
= less maintenance work at runtime.
>> +
>> +\subsection{Feature bits}\label{sec:Device Types / Sound Device / Feature bits}
>> +
>> +\begin{description}
>> +\item[VIRTIO_SND_F_PCM_OUTPUT (0)] Output PCM stream support.
>> +\item[VIRTIO_SND_F_PCM_INPUT (1)] Input PCM stream support.
>> +\end{description}
>> +
>> +\subsection{Device configuration layout}\label{sec:Device Types / Sound Device / Device configuration layout}
>> +
>> +\begin{lstlisting}
>> +/* supported PCM sample formats */
>> +enum {
>> + VIRTIO_SND_PCM_FMT_S8 = 0,
>> + VIRTIO_SND_PCM_FMT_U8,
>> + VIRTIO_SND_PCM_FMT_S16,
>> + VIRTIO_SND_PCM_FMT_U16,
>> + VIRTIO_SND_PCM_FMT_S32,
>> + VIRTIO_SND_PCM_FMT_U32,
>> + VIRTIO_SND_PCM_FMT_FLOAT,
>> + VIRTIO_SND_PCM_FMT_FLOAT64
>> +};
>
> Is it worth adding a word or two about the encoding used, in particular
> that this is linear quantization? Or maybe it's standard enough, I don't
> know much about audio.
Yes, these are pretty common in audio.
>> +
>> +/* supported PCM frame rates */
>> +enum {
>> + VIRTIO_SND_PCM_RATE_5512 = 0,
>> + VIRTIO_SND_PCM_RATE_8000,
>> + VIRTIO_SND_PCM_RATE_11025,
>> + VIRTIO_SND_PCM_RATE_16000,
>> + VIRTIO_SND_PCM_RATE_22050,
>> + VIRTIO_SND_PCM_RATE_32000,
>> + VIRTIO_SND_PCM_RATE_44100,
>> + VIRTIO_SND_PCM_RATE_48000,
>> + VIRTIO_SND_PCM_RATE_64000,
>> + VIRTIO_SND_PCM_RATE_88200,
>> + VIRTIO_SND_PCM_RATE_96000,
>> + VIRTIO_SND_PCM_RATE_176400,
>> + VIRTIO_SND_PCM_RATE_192000,
>> + VIRTIO_SND_PCM_RATE_384000
>> +};
>> +
>> +/* a PCM stream configuration */
>> +struct virtio_pcm_stream_config {
>> + u8 channels_min;
>> + u8 channels_max;
>> + le16 formats; /* 1 << VIRTIO_SND_PCM_FMT_XXX */
>> + le16 rates; /* 1 << VIRTIO_SND_PCM_RATE_XXX */
>> + u16 padding;
>
> You could add a lot more padding, because the current layout doesn't allow
> to extend virtio_pcm_stream_config much (an old driver will expect the
> pcm.input field to start at offset 0x8 of the config space).
Well, device configuration space in general is not very suitable place for any
multimedia configuration, because such configuration may become quite complex
with time. In earlier versions we proposed more flexible way for discovery
things with special control request. But it didn't align well with other
existing virtio devices.
>> +};
>> +
>> +/* a device configuration space */
>> +struct virtio_snd_config {
>> + struct virtio_pcm_config {
>> + struct virtio_pcm_stream_config output;
>> + struct virtio_pcm_stream_config input;
>> + } pcm;
>> +};
>> +\end{lstlisting}
>> +
>> +\subsubsection{Device configuration fields}
>> +
>> +The \field{pcm.output} and \field{pcm.input} fields contain PCM stream
>> +configuration:
>> +
>> +\begin{description}
>> +\item[\field{channels_min}] (driver-read-only) is a minimum number of supported
>> +channels.
>> +\item[\field{channels_max}] (driver-read-only) is a maximum number of supported
>> +channels.
>> +\item[\field{formats}] (driver-read-only) is a supported sample format bit map.
>> +\item[\field{rates}] (driver-read-only) is a supported frame rate bit map.
>> +\end{description}
>> +
>> +Only interleaved samples are supported.
>
> Similar noob remark: I guess this is standard in audio hardware and
> doesn't need more explanation? If the term is alsa-specific then we'd
> need a few words about what the format looks like.
Yes, this is a standard thing in audio.
>> +
>> +\subsection{Device Initialization}
>> +
>> +\begin{enumerate}
>> +\item If the VIRTIO_SND_F_PCM_OUTPUT feature is negotiated, \field{pcm.output}
>> +contains the valid output PCM stream configuration.
>> +\item If the VIRTIO_SND_F_PCM_INPUT feature is negotiated, \field{pcm.input}
>> +contains the valid input PCM stream configuration.
>> +\end{enumerate}
>> +
>> +\devicenormative{\subsubsection}{Device Initialization}{Device Types / Sound Device / Device Initialization}
>> +
>> +\begin{enumerate}
>> +\item The device MUST NOT set the not defined format and rate bits.
>
> s/not defined/undefined/ (here and elsewhere)
Thanks!
>> +\item The device MUST initialize padding bytes \field{pcm.output.padding} and
>> +\field{pcm.input.padding} to 0.
>> +\end{enumerate}
>> +
>> +\drivernormative{\subsubsection}{Device Initialization}{Device Types / Sound Device / Device Initialization}
>> +
>> +\begin{enumerate}
>> +\item The driver MUST configure and initialize all virtqueues.
>> +\item The driver SHOULD ignore the not defined format and rate bits.
>> +\end{enumerate}
>> +
>> +\subsection{Device Operation}\label{sec:Device Types / Sound Device / Device Operation}
>> +
>> +All control messages are placed into the controlq virtqueue and use the following
>> +layout structure and definitions:
>> +
>> +\begin{lstlisting}
>> +enum {
>> + /* PCM control request types */
>> + VIRTIO_SND_R_PCM_CHMAP_INFO = 0,
>> + VIRTIO_SND_R_PCM_SET_FORMAT,
>> + VIRTIO_SND_R_PCM_PREPARE,
>> + VIRTIO_SND_R_PCM_START,
>> + VIRTIO_SND_R_PCM_STOP,
>> + VIRTIO_SND_R_PCM_PAUSE,
>> + VIRTIO_SND_R_PCM_UNPAUSE,
>> +
>> + /* generic status codes */
>> + VIRTIO_SND_S_OK = 0x8000,
>> + VIRTIO_SND_S_BAD_MSG,
>> + VIRTIO_SND_S_NOT_SUPP,
>> + VIRTIO_SND_S_IO_ERR
>> +};
>
> Any reason not to use two separate enums starting from 0?
It's funny, because initially there were exactly two separate enums. It was
proposed to combine them into one to keep things in one place.
>> +
>> +struct virtio_snd_ctl_msg {
>> + /* device-read-only data */
>> + le32 request_code;
>> + u8 request_payload[];
>> + /* device-writable data */
>> + le32 response_status;
>> + u8 response_payload[];
>> +};
>> +\end{lstlisting}
>> +
>> +A generic control message consists of a request part and a response part, and
>> +contains the following fields:
>> +
>> +\begin{description}
>> +\item[\field{request_code}] (device-read-only) specifies a device request code
>> +(VIRTIO_SND_R_*).
>> +\item[\field{request_payload}] (device-read-only) contains request-specific
>> +data.
>> +\item[\field{response_status}] (device-writable) specifies a device response
>> +status (VIRTIO_SND_S_*).
>> +\item[\field{response_payload}] (device-writable) contains response-specific
>> +data.
>> +\end{description}
>> +
>> +Unless stated otherwise, the \field{request_payload} and \field{response_payload}
>> +fields are empty.
>> +
>> +The \field{response_status} field contains one of the following values:
>> +
>> +\begin{itemize*}
>
> Nit: other devices use the "description" environment for statuses. (By the
> way, what does the asterisk after itemize do?)
Good question! Have no idea. :)
>> +\item VIRTIO_SND_S_OK: success.
>> +\item VIRTIO_SND_S_BAD_MSG: a control message is malformed or contains invalid
>> +parameters.
>> +\item VIRTIO_SND_S_NOT_SUPP: requested operation or parameters are not supported.
>> +\item VIRTIO_SND_S_IO_ERR: an I/O error occurred.
>> +\end{itemize*}
>> +
>> +\subsubsection{Device Operation: PCM control requests}
>> +
>> +A PCM stream has the following command lifecycle:
>> +
>> +\begin{enumerate}
>> +\item Set format
>> +\item Prepare
>> +\item Output only: transfer data for prebuffing
>> +\item Start
>> +\item Transfer data to/from the PCM device
>> +\begin{enumerate}
>> + \item Pause
>> + \item Unpause
>> +\end{enumerate}
>> +\item Stop
>
> Which commands are allowed after Stop, just Set format or can we send
> Start directly?
Set format or prepare.
Maybe it's better to define a state machine here. There's no example in the
virtio spec, and we decided to put a simple list instead.
>> +\end{enumerate}
>> +
>> +PCM control requests have or consist of a fixed header with the following
>> +layout structure:
>> +
>> +\begin{lstlisting}
>> +/* supported PCM stream types */
>> +enum {
>> + VIRTIO_SND_PCM_T_OUTPUT = 0,
>> + VIRTIO_SND_PCM_T_INPUT
>> +};
>> +
>> +/* a PCM control request header */
>> +struct virtio_snd_pcm_hdr {
>> + le32 code;
>> + le32 stream;
>> +};
>> +\end{lstlisting}
>> +
>> +PCM control requests contain the following fields:
>> +
>> +\begin{description}
>> +\item[\field{code}] (device-read-only) specifies a PCM device request code
>> +(VIRTIO_SND_R_PCM_*).
>> +\item[\field{stream}] (device-read-only) specifies a PCM stream type
>> +(VIRTIO_SND_PCM_T_*).
>> +\end{description}
>> +
>> +\begin{description}
>> +
>> +\item[VIRTIO_SND_R_PCM_CHMAP_INFO]
>
> Nit: there is a lot of content in some control requests, it might look
> nicer if they each have their own \paragraph{} instead of being
> description items.
Yeah, good point.
>> +Query a PCM channel map information for specified stream type.
>> +
>> +A response uses the following layout structure and definitions:
>> +
>> +\begin{lstlisting}
>> +/* standard channel position definition */
>> +enum {
>> + VIRTIO_SND_PCM_CH_NONE = 0, /* undefined */
>> + VIRTIO_SND_PCM_CH_NA, /* silent */
>> + VIRTIO_SND_PCM_CH_MONO, /* mono stream */
>> + VIRTIO_SND_PCM_CH_FL, /* front left */
>> + VIRTIO_SND_PCM_CH_FR, /* front right */
>> + VIRTIO_SND_PCM_CH_RL, /* rear left */
>> + VIRTIO_SND_PCM_CH_RR, /* rear right */
>> + VIRTIO_SND_PCM_CH_FC, /* front center */
>> + VIRTIO_SND_PCM_CH_LFE, /* low frequency (LFE) */
>> + VIRTIO_SND_PCM_CH_SL, /* side left */
>> + VIRTIO_SND_PCM_CH_SR, /* side right */
>> + VIRTIO_SND_PCM_CH_RC, /* rear center */
>> + VIRTIO_SND_PCM_CH_FLC, /* front left center */
>> + VIRTIO_SND_PCM_CH_FRC, /* front right center */
>> + VIRTIO_SND_PCM_CH_RLC, /* rear left center */
>> + VIRTIO_SND_PCM_CH_RRC, /* rear right center */
>> + VIRTIO_SND_PCM_CH_FLW, /* front left wide */
>> + VIRTIO_SND_PCM_CH_FRW, /* front right wide */
>> + VIRTIO_SND_PCM_CH_FLH, /* front left high */
>> + VIRTIO_SND_PCM_CH_FCH, /* front center high */
>> + VIRTIO_SND_PCM_CH_FRH, /* front right high */
>> + VIRTIO_SND_PCM_CH_TC, /* top center */
>> + VIRTIO_SND_PCM_CH_TFL, /* top front left */
>> + VIRTIO_SND_PCM_CH_TFR, /* top front right */
>> + VIRTIO_SND_PCM_CH_TFC, /* top front center */
>> + VIRTIO_SND_PCM_CH_TRL, /* top rear left */
>> + VIRTIO_SND_PCM_CH_TRR, /* top rear right */
>> + VIRTIO_SND_PCM_CH_TRC, /* top rear center */
>> + VIRTIO_SND_PCM_CH_TFLC, /* top front left center */
>> + VIRTIO_SND_PCM_CH_TFRC, /* top front right center */
>> + VIRTIO_SND_PCM_CH_TSL, /* top side left */
>> + VIRTIO_SND_PCM_CH_TSR, /* top side right */
>> + VIRTIO_SND_PCM_CH_LLFE, /* left LFE */
>> + VIRTIO_SND_PCM_CH_RLFE, /* right LFE */
>> + VIRTIO_SND_PCM_CH_BC, /* bottom center */
>> + VIRTIO_SND_PCM_CH_BLC, /* bottom left center */
>> + VIRTIO_SND_PCM_CH_BRC /* bottom right center */
>> +};
>> +
>> +/* a maximum possible number of channels */
>> +#define VIRTIO_SND_PCM_CH_MAX 256
>> +
>> +/* a response containing a PCM channel map information */
>> +struct virtio_snd_pcm_chmap_info {
>
> Including the header would be more consistent with set_format below.
You are right.
>> + le32 status;
>> + le32 npositions;
>> + u8 positions[VIRTIO_SND_PCM_CH_MAX];
>> +};
>> +\end{lstlisting}
>> +
>> +The PCM channel map information fields are:
>> +
>> +\begin{description}
>> +\item[\field{status}] (device-writable) specifies a device response status
>> +(VIRTIO_SND_S_*).
>> +\item[\field{npositions}] (device-writable) is a number of valid entries in
>> +the \field{positions} array.
>> +\item[\field{positions}] (device-writable) contains PCM channel positions
>> +(VIRTIO_SND_PCM_CH_*).
>> +\end{description}
>> +
>> +\item[VIRTIO_SND_R_PCM_SET_FORMAT]
>> +Set selected PCM format.
>> +
>> +\begin{lstlisting}
>> +struct virtio_snd_pcm_set_format {
>> + struct virtio_snd_pcm_hdr hdr;
>> + le16 channels;
>> + le16 format;
>> + le16 rate;
>> + u16 padding;
>> +};
>> +\end{lstlisting}
>> +
>> +The PCM control request fields are:
>> +
>> +\begin{description}
>> +\item[\field{hdr}] (device-read-only) is a PCM control request header.
>> +\item[\field{channels}] (device-read-only) specifies a desired number of channels.
>> +\item[\field{format}] (device-read-only) specifies a desired PCM sample format
>> +(VIRTIO_SND_PCM_FMT_*).
>> +\item[\field{rate}] (device-read-only) specifies a desired PCM frame rate
>> +(VIRTIO_SND_PCM_RATE_*).
>> +\end{description}
>> +
>> +\item[VIRTIO_SND_R_PCM_PREPARE]
>> +Prepare the PCM device.
>
> Isn't that for one stream rather than the whole device? Otherwise, what
> should the hdr.stream field contain?
These requests are excactly for one stream. And a header must specify for
which one. We will rework this part to make things clearer.
>> +
>> +\item[VIRTIO_SND_R_PCM_START]
>> +Start the PCM device.
>> +
>> +\item[VIRTIO_SND_R_PCM_STOP]
>> +Stop the PCM device.
>> +
>> +\item[VIRTIO_SND_R_PCM_PAUSE]
>> +Set the PCM device on pause.
>> +
>> +\item[VIRTIO_SND_R_PCM_UNPAUSE]
>> +Unset the PCM device from pause.
>
> Or "Resume the paused PCM device"?
Thanks, will be fixed.
>> +
>> +\end{description}
>> +
>> +\devicenormative{\subsubsection}{PCM control requests}{Device Types / Sound Device / PCM control requests}
>> +
>> +In a VIRTIO_SND_R_PCM_CHMAP_INFO request:
>> +
>> +\begin{itemize*}
>> +\item if the device does not support a channel map for a specified stream type,
>> +then it MUST return the VIRTIO_SND_S_NOT_SUPP status code;
>> +\item if the operation failed, then the device MUST set the \field{npositions}
>> +field to 0.
>> +\end{itemize*}
>> +
>> +\drivernormative{\subsubsection}{PCM control requests}{Device Types / Sound Device / PCM control requests}
>> +
>> +If the VIRTIO_SND_F_PCM_OUTPUT feature is not negotiated, then the driver MUST NOT
>> +specify VIRTIO_SND_PCM_T_OUTPUT as a stream type
>> +
>> +If the VIRTIO_SND_F_PCM_INPUT feature is not negotiated, then the driver MUST NOT
>> +specify VIRTIO_SND_PCM_T_INPUT as a stream type.
>> +
>> +In a VIRTIO_SND_R_PCM_SET_FORMAT request:
>> +
>> +\begin{itemize*}
>> +\item the driver MUST NOT specify the \field{channels} value as less than the \field{channels_min}
>> +or greater than the \field{channels_max} values reported in the stream configuration;
>> +\item the driver MUST specify the \field{format} and \field{rate} values according
>> +to the \field{formats} and \field{rates} values reported in the stream configuration;
>> +\item the driver MUST NOT specify the not defined format and rate values;
>> +\item the driver MUST initialize the \field{padding} field to 0.
>> +\end{itemize*}
>
> I guess configuration requests for a stream that isn't in "Set format"
> state are illegal? Probably needs to be stated here.
Yes, some requests are illegal at some points of time. As I mentioned earlier,
maybe it's better to define a kind of state machine. Otherwise, there should
be a lot of such small requirements.
> Thanks,
> Jean
>
--
Anton Yakovlev
Senior Software Engineer
OpenSynergy GmbH
Rotherstr. 20, 10245 Berlin
Phone: +49 30 60 98 54 0
E-Mail: anton.yakovlev@opensynergy.com
www.opensynergy.com
Handelsregister/Commercial Registry: Amtsgericht Charlottenburg, HRB 108616B
Geschäftsführer/Managing Director: Regis Adjamah
---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [virtio-dev] [PATCH v2] snd: Add virtio sound device specification
2019-11-14 13:23 ` Anton Yakovlev
@ 2019-11-19 15:14 ` Liam Girdwood
0 siblings, 0 replies; 8+ messages in thread
From: Liam Girdwood @ 2019-11-19 15:14 UTC (permalink / raw)
To: Anton Yakovlev, Mikhail Golubev, virtio-dev
Cc: tiwai, broonie, maz, james.morse, julien.thierry.kdev, suzuki.poulose
On Thu, 2019-11-14 at 14:23 +0100, Anton Yakovlev wrote:
> > > >
> > > > +\subsection{Device Operation}\label{sec:Device Types / Sound
> > > > Device / Device Operation}
> > > > +
> > > > +All control messages are placed into the controlq virtqueue
> > > > and
> > > > use the following
> > > > +layout structure and definitions:
> > > > +
> > > > +\begin{lstlisting}
> > > > +enum {
> > > > + /* PCM control request types */
> > > > + VIRTIO_SND_R_PCM_CHMAP_INFO = 0,
> > > > + VIRTIO_SND_R_PCM_SET_FORMAT,
> > > > + VIRTIO_SND_R_PCM_PREPARE,
> > > > + VIRTIO_SND_R_PCM_START,
> > > > + VIRTIO_SND_R_PCM_STOP,
> > > > + VIRTIO_SND_R_PCM_PAUSE,
> > > > + VIRTIO_SND_R_PCM_UNPAUSE,
> >
> > Where do I tear down the PCM when I'm finished with it ? i.e. to
> > free
> > resources.
>
> Current workflow:
> - a PCM stream is preparing with the set_format/prepare/start
> requests
> - if necessary, resources are freed/released with the stop request
>
> If needed, the stream can be temporary paused (the pause/unpause
> requests).
Temp pause/unpause is fine, since we can keep audio power ON to avoid
audio atifacts and we resume from the same position.
However, there are times when we need to stop audio for longer periods
and restart (where we do want to power down) or when we want to recover
a stream by stopping and then restarting it (without having to resend
all the params and realloc buffers etc).
I would recommend you have something like VIRTIO_SND_R_PCM_FREE to free
all resource and not overload STOP.
Liam
---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [virtio-dev] [PATCH v2] snd: Add virtio sound device specification
2019-11-14 14:45 ` Anton Yakovlev
@ 2019-11-20 11:09 ` Gerd Hoffmann
0 siblings, 0 replies; 8+ messages in thread
From: Gerd Hoffmann @ 2019-11-20 11:09 UTC (permalink / raw)
To: Anton Yakovlev
Cc: Jean-Philippe Brucker, Mikhail Golubev, virtio-dev,
liam.r.girdwood, tiwai, broonie, maz, james.morse,
julien.thierry.kdev, suzuki.poulose
Hi,
> > > +\begin{description}
> > > +\item[0] controlq
> > > +\item[1] pcmq
> > > +\end{description}
> > > +
> > > +The controlq virtqueue always exists, the pcmq virtqueue only exists if
> > > +the VIRTIO_SND_F_PCM_OUTPUT and/or VIRTIO_SND_F_PCM_INPUT feature is negotiated.
> >
> > Why not use two queues, one for input and one for output? I think most
> > virtio devices do it that way. It could simplify the implementations,
> > shave 32 bits off the buffers, and allow to quiesce notifications for
> > input and output streams independently. And in the future you can extend
> > the device with more than two streams.
>
> This was discussed earlier (having two separate virtqueues vs multiplexing
> into single one). It's hard to say what is better, but at least one
> virtqueue = less maintenance work at runtime.
I'd suggest separate tx (playback) and rx (record) queues, the workflow
is quite different so that makes sense. Multiplexing multiple playback
streams over a single tx queue is alot less problematic than handling
both rx and tx in a single queue.
> > > +\begin{lstlisting}
> > > +enum {
> > > + /* PCM control request types */
> > > + VIRTIO_SND_R_PCM_CHMAP_INFO = 0,
> > > + VIRTIO_SND_R_PCM_SET_FORMAT,
> > > + VIRTIO_SND_R_PCM_PREPARE,
> > > + VIRTIO_SND_R_PCM_START,
> > > + VIRTIO_SND_R_PCM_STOP,
> > > + VIRTIO_SND_R_PCM_PAUSE,
> > > + VIRTIO_SND_R_PCM_UNPAUSE,
> > > +
> > > + /* generic status codes */
> > > + VIRTIO_SND_S_OK = 0x8000,
> > > + VIRTIO_SND_S_BAD_MSG,
> > > + VIRTIO_SND_S_NOT_SUPP,
> > > + VIRTIO_SND_S_IO_ERR
> > > +};
> >
> > Any reason not to use two separate enums starting from 0?
>
> It's funny, because initially there were exactly two separate enums. It was
> proposed to combine them into one to keep things in one place.
Having non-overlapping values for request types and status codes is more
robust, so IMO this is a good thing. I'd also suggest to *not* start
from 0 (so a fresh, zero-initialized field is not a valid request type).
> > > +struct virtio_snd_ctl_msg {
> > > + /* device-read-only data */
> > > + le32 request_code;
> > > + u8 request_payload[];
> > > + /* device-writable data */
> > > + le32 response_status;
> > > + u8 response_payload[];
> > > +};
> > > +\end{lstlisting}
Must be splitted into out and in structs (see also virtqueue element
discussion in v1 thread).
I'd also suggest to specify a union for the payload. Or different
structs for different request types (if they have a payload).
cheers,
Gerd
---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-11-20 11:10 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-05 15:06 [virtio-dev] [PATCH v2] snd: Add virtio sound device specification Mikhail Golubev
2019-11-06 9:28 ` Mikhail Golubev
2019-11-11 15:28 ` Liam Girdwood
2019-11-14 13:23 ` Anton Yakovlev
2019-11-19 15:14 ` Liam Girdwood
2019-11-12 13:20 ` Jean-Philippe Brucker
2019-11-14 14:45 ` Anton Yakovlev
2019-11-20 11:09 ` Gerd Hoffmann
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.