[virtio-dev] [PATCH v3] virtio-snd: add virtio sound device specification

[virtio-dev] [PATCH v3] virtio-snd: add virtio sound device specification

[Anton Yakovlev]

This patch proposes virtio specification for new virtio sound device, that may
be useful in case, when having audio is required but a device passthrough or
emulation is not an option.

Signed-off-by: Anton Yakovlev <Anton.Yakovlev@opensynergy.com>
---

v2 -> v3 changes:
1. Define 4 mandatory queues: control, event, tx and rx.
2. Define feature bits for shared memory transport.
   Make a message-based transport default.
3. Replace device features with stream features.
4. Rework device configuration space, now it contains only the total
   number of available PCM streams.
5. Add the get stream capabilities control request and define
   the stream capabilities structure.
6. Replace the set format with the set parameters control request.
7. Add the PCM notifications section.
8. Rework the PCM I/O messages section.

 conformance.tex  |  22 ++
 content.tex      |   1 +
 virtio-sound.tex | 561 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 584 insertions(+)
 create mode 100644 virtio-sound.tex

diff --git a/conformance.tex b/conformance.tex
index 50969e5..de09e3e 100644
--- a/conformance.tex
+++ b/conformance.tex
@@ -191,6 +191,17 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
 \item \ref{drivernormative:Device Types / RPMB Device / Device Operation}
 \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 Operation / PCM Stream Parameters}
+\item \ref{drivernormative:Device Types / Sound Device / Device Operation / PCM Notifications}
+\item \ref{drivernormative:Device Types / Sound Device / Device Operation / PCM Output Stream}
+\item \ref{drivernormative:Device Types / Sound Device / Device Operation / PCM Input Stream}
+\end{itemize}
+
 \conformance{\section}{Device Conformance}\label{sec:Conformance / Device Conformance}
 
 A device MUST conform to the following normative statements:
@@ -360,6 +371,17 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
 \item \ref{devicenormative:Device Types / RPMB Device / Device Operation}
 \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 / Device Operation / PCM Stream Capabilities}
+\item \ref{devicenormative:Device Types / Sound Device / Device Operation / PCM Stream Parameters}
+\item \ref{devicenormative:Device Types / Sound Device / Device Operation / PCM Input Stream}
+\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 d68cfaf..70ad066 100644
--- a/content.tex
+++ b/content.tex
@@ -5739,6 +5739,7 @@ \subsubsection{Legacy Interface: Framing Requirements}\label{sec:Device
 \input{virtio-vsock.tex}
 \input{virtio-fs.tex}
 \input{virtio-rpmb.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..30b5f4f
--- /dev/null
+++ b/virtio-sound.tex
@@ -0,0 +1,561 @@
+\section{Sound Device}\label{sec:Device Types / Sound Device}
+
+The virtio sound card is a virtual audio device supporting output and input PCM
+streams.
+
+A device is managed by control requests and can send various notifications
+through dedicated queues. A driver can transmit PCM frames using message-based
+transport or shared memory.
+
+\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] eventq
+\item[2] txq
+\item[3] rxq
+\end{description}
+
+The control queue is used for sending control messages from the driver to
+the device.
+
+The event queue is used for sending notifications from the device to the driver.
+
+The tx queue is used for sending PCM I/O messages for an output stream.
+
+The rx queue is used for sending PCM I/O messages for an input stream.
+
+\subsection{Feature bits}\label{sec:Device Types / Sound Device / Feature bits}
+
+None currently defined.
+
+\subsection{Device configuration layout}\label{sec:Device Types / Sound Device / Device configuration layout}
+
+\begin{lstlisting}
+struct virtio_snd_config {
+    le32 pcm_streams;
+};
+\end{lstlisting}
+
+A configuration space contains the following field:
+
+\begin{description}
+\item[\field{pcm_streams}] (driver-read-only) is a total number of all available
+PCM streams.
+\end{description}
+
+\subsection{Device Initialization}
+
+\begin{enumerate}
+\item Configure the control, event, tx and rx queues.
+\item Read the \field{pcm_streams} value from the configuration space.
+\item Send control request to query capabilities per each available PCM stream.
+\end{enumerate}
+
+\devicenormative{\subsubsection}{Device Initialization}{Device Types / Sound Device / Device Initialization}
+
+\begin{enumerate}
+\item The device MUST set non-zero value for the \field{pcm_streams} configuration
+field.
+\end{enumerate}
+
+\subsection{Device Operation}\label{sec:Device Types / Sound Device / Device Operation}
+
+All control messages are placed into the control queue and all notifications
+are placed into the event queue. They use the following layout structure and
+definitions:
+
+\begin{lstlisting}
+enum {
+    /* PCM control request types */
+    VIRTIO_SND_R_PCM_GET_CAPS = 1,
+    VIRTIO_SND_R_PCM_SET_PARAMS,
+    VIRTIO_SND_R_PCM_PREPARE,
+    VIRTIO_SND_R_PCM_RELEASE,
+    VIRTIO_SND_R_PCM_START,
+    VIRTIO_SND_R_PCM_STOP,
+    VIRTIO_SND_R_PCM_PAUSE,
+    VIRTIO_SND_R_PCM_UNPAUSE,
+
+    /* PCM event types */
+    VIRTIO_SND_EVT_PCM_PERIOD = 0x1000,
+    VIRTIO_SND_EVT_PCM_XRUN,
+
+    /* common status codes */
+    VIRTIO_SND_S_OK = 0x8000,
+    VIRTIO_SND_S_BAD_MSG,
+    VIRTIO_SND_S_NOT_SUPP,
+    VIRTIO_SND_S_IO_ERR
+};
+
+/* a common header */
+struct virtio_snd_hdr {
+    le32 code;
+};
+\end{lstlisting}
+
+A generic control message consists of a request part and a response part.
+
+A request part has or consists of a common header containing the following
+field:
+
+\begin{description}
+\item[\field{code}] (device-readable) specifies a device request type
+(VIRTIO_SND_R_*).
+\end{description}
+
+A response part has or consists of a common header containing the following
+field:
+
+\begin{description}
+\item[\field{code}] (device-writable) specifies a device request status
+(VIRTIO_SND_S_*).
+\end{description}
+
+The status field can take 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*}
+
+The request part may be followed by an additional device-readable payload,
+and the response part may be followed by an additional device-writable payload.
+
+A notification has or consists of a common header containing the following field:
+
+\begin{description}
+\item[\field{code}] (device-writable) specifies an event type (VIRTIO_SND_EVT_*).
+\end{description}
+
+A notification header also may be followed by an additional device-writable
+payload.
+
+\subsubsection{PCM Control Messages}\label{sec:Device Types / Sound Device / Device Operation / PCM Control Messages}
+
+A PCM control request has or consists of a common header with the following
+layout structure:
+
+\begin{lstlisting}
+struct virtio_snd_pcm_hdr {
+    struct virtio_snd_hdr hdr;
+    le32 stream_id;
+};
+\end{lstlisting}
+
+The header consists of the following field:
+
+\begin{description}
+\item[\field{hdr}] (device-readable) contains request type (VIRTIO_SND_R_PCM_*).
+\item[\field{stream_id}] (device-readable) specifies a PCM stream identifier
+from 0 to \field{pcm_streams} - 1.
+\end{description}
+
+\subsubsection{PCM Command Lifecycle}
+
+A PCM stream has the following command lifecycle:
+
+\begin{enumerate}
+\item SET PARAMETERS
+
+The driver negotiates the stream parameters (format, transport, etc) with
+the device.
+
+Possible valid transitions: set parameters, prepare.
+
+\item PREPARE
+
+The device prepares the stream (allocates resources, etc).
+
+Possible valid transitions: start, release.
+
+\item Output only: the driver transfers data for pre-buffing.
+\item START
+
+The device starts the stream (unmute, putting into running state, etc).
+
+Possible valid transitions: pause, stop.
+
+\item The driver transfers data to/from the stream.
+\begin{enumerate}
+\item PAUSE
+
+The device pauses the stream.
+
+Possible valid transitions: unpause, stop.
+
+\item UNPAUSE
+
+The device releases the stream from pause.
+
+Possible valid transitions: pause, stop.
+
+\end{enumerate}
+\item STOP
+
+The device stops the stream (mute, putting into non-running state, etc).
+
+Possible valid transitions: start, release.
+
+\item RELEASE
+
+The device releases the stream (frees resources, etc).
+
+Possible valid transitions: set parameters, prepare.
+
+\end{enumerate}
+
+\paragraph{VIRTIO_SND_R_PCM_GET_CAPS}
+
+Query stream capabilities for the specified stream ID.
+
+The request has no additional payload, and the response uses the following
+structure and layout definitions:
+
+\begin{lstlisting}
+/* supported PCM stream types */
+enum {
+    VIRTIO_SND_PCM_T_OUTPUT = 0,
+    VIRTIO_SND_PCM_T_INPUT
+};
+
+/* supported PCM stream features */
+enum {
+    VIRTIO_SND_PCM_F_HOST_MEM = 0,
+    VIRTIO_SND_PCM_F_GUEST_MEM,
+    VIRTIO_SND_PCM_F_CHMAP,
+    VIRTIO_SND_PCM_F_EVT_PERIOD,
+    VIRTIO_SND_PCM_F_EVT_XRUN
+};
+
+/* supported PCM sample formats */
+enum {
+    /* analog formats (width / physical width) */
+    VIRTIO_SND_PCM_FMT_IMA_ADPCM = 0,   /*  4 /  4 bits */
+    VIRTIO_SND_PCM_FMT_MU_LAW,          /*  8 /  8 bits */
+    VIRTIO_SND_PCM_FMT_A_LAW,           /*  8 /  8 bits */
+    VIRTIO_SND_PCM_FMT_S8,              /*  8 /  8 bits */
+    VIRTIO_SND_PCM_FMT_U8,              /*  8 /  8 bits */
+    VIRTIO_SND_PCM_FMT_S16,             /* 16 / 16 bits */
+    VIRTIO_SND_PCM_FMT_U16,             /* 16 / 16 bits */
+    VIRTIO_SND_PCM_FMT_S18_3,           /* 18 / 24 bits */
+    VIRTIO_SND_PCM_FMT_U18_3,           /* 18 / 24 bits */
+    VIRTIO_SND_PCM_FMT_S20_3,           /* 20 / 24 bits */
+    VIRTIO_SND_PCM_FMT_U20_3,           /* 20 / 24 bits */
+    VIRTIO_SND_PCM_FMT_S24_3,           /* 24 / 24 bits */
+    VIRTIO_SND_PCM_FMT_U24_3,           /* 24 / 24 bits */
+    VIRTIO_SND_PCM_FMT_S20,             /* 20 / 32 bits */
+    VIRTIO_SND_PCM_FMT_U20,             /* 20 / 32 bits */
+    VIRTIO_SND_PCM_FMT_S24,             /* 24 / 32 bits */
+    VIRTIO_SND_PCM_FMT_U24,             /* 24 / 32 bits */
+    VIRTIO_SND_PCM_FMT_S32,             /* 32 / 32 bits */
+    VIRTIO_SND_PCM_FMT_U32,             /* 32 / 32 bits */
+    VIRTIO_SND_PCM_FMT_FLOAT,           /* 32 / 32 bits */
+    VIRTIO_SND_PCM_FMT_FLOAT64,         /* 64 / 64 bits */
+    /* digital formats (width / physical width) */
+    VIRTIO_SND_PCM_FMT_DSD_U8,          /*  8 /  8 bits */
+    VIRTIO_SND_PCM_FMT_DSD_U16,         /* 16 / 16 bits */
+    VIRTIO_SND_PCM_FMT_DSD_U32,         /* 32 / 32 bits */
+    VIRTIO_SND_PCM_FMT_IEC958_SUBFRAME  /* 32 / 32 bits */
+};
+
+/* 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
+};
+
+/* 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   16
+
+/* a response containing PCM stream capabilities */
+struct virtio_snd_pcm_caps {
+    struct virtio_snd_hdr hdr; /* VIRTIO_SND_S_* */
+    u8 type;
+    u8 channels_min;
+    u8 channels_max;
+    u8 features; /* 1 << VIRTIO_SND_PCM_F_XXX */
+    le64 formats; /* 1 << VIRTIO_SND_PCM_FMT_XXX */
+    le64 rates; /* 1 << VIRTIO_SND_PCM_RATE_XXX */
+    u8 chmap[VIRTIO_SND_PCM_CH_MAX];
+};
+\end{lstlisting}
+
+The response contains the following fields:
+
+\begin{description}
+\item[\field{type}] (device-writable) is a PCM stream type (VIRTIO_SND_PCM_T_*).
+\item[\field{channels_min}] (device-writable) is a minimum number of supported
+channels.
+\item[\field{channels_max}] (device-writable) is a maximum number of supported
+channels.
+\item[\field{features}] (device-writable) is a supported feature bit map:
+\begin{itemize*}
+\item VIRTIO_SND_PCM_F_HOST_MEM: support sharing a host memory with a guest,
+\item VIRTIO_SND_PCM_F_GUEST_MEM: support sharing a guest memory with a host,
+\item VIRTIO_SND_PCM_F_CHMAP: support a channel map,
+\item VIRTIO_SND_PCM_F_EVT_PERIOD: support elapsed period notifications,
+\item VIRTIO_SND_PCM_F_EVT_XRUN: support underrun/overrun notifications.
+\end{itemize*}
+\item[\field{formats}] (device-writable) is a supported sample format bit map.
+\item[\field{rates}] (device-writable) is a supported frame rate bit map.
+\item[\field{chmap}] (device-writable) if the VIRTIO_SND_PCM_F_CHMAP feature bit
+is set, then the first \field{channels_max} elements are valid and contain
+channel positions (VIRTIO_SND_PCM_CH_*).
+\end{description}
+
+Only interleaved samples are supported.
+
+\devicenormative{\subparagraph}{Stream Capabilities}{Device Types / Sound Device / Device Operation / PCM Stream Capabilities}
+
+\begin{itemize*}
+\item The device MUST NOT set undefined stream type values.
+\item The device MUST NOT set undefined feature, format and rate bits.
+\item If the VIRTIO_SND_PCM_F_CHMAP feature bit is not set, then the device
+MUST initialize the \field{chmap} field with the VIRTIO_SND_PCM_CH_NONE position
+values.
+\end{itemize*}
+
+\paragraph{VIRTIO_SND_R_PCM_SET_PARAMS}\label{sec:Device Types / Sound Device / Device Operation / PCM Stream Parameters}
+
+Set selected stream parameters for the specified stream ID.
+
+The request uses the following structure and layout definitions:
+
+\begin{lstlisting}
+struct virtio_snd_pcm_set_params {
+    struct virtio_snd_pcm_hdr hdr; /* .code = VIRTIO_SND_R_PCM_SET_PARAMS */
+    u8 features;
+    u8 channels;
+    u8 format;
+    u8 rate;
+    le32 buffer_bytes;
+    le32 period_bytes;
+};
+\end{lstlisting}
+
+The request contains the following fields:
+
+\begin{description}
+\item[\field{features}] (device-readable) is a selected feature bit map:
+\begin{itemize*}
+\item VIRTIO_SND_PCM_F_HOST_MEM: use shared memory allocated by the host (TBD),
+\item VIRTIO_SND_PCM_F_GUEST_MEM: use shared memory allocated by the guest (TBD),
+\item VIRTIO_SND_PCM_F_EVT_PERIOD: enable elapsed period notifications,
+\item VIRTIO_SND_PCM_F_EVT_XRUN: enable underrun/overrun notifications.
+\end{itemize*}
+\item[\field{channels}] (device-readable) specifies a selected number of channels.
+\item[\field{format}] (device-readable) specifies a selected sample format
+(VIRTIO_SND_PCM_FMT_*).
+\item[\field{rate}] (device-readable) specifies a selected frame rate
+(VIRTIO_SND_PCM_RATE_*).
+\item[\field{buffer_bytes}] (device-readable) specifies the size of the hardware
+buffer used by the driver.
+\item[\field{period_bytes}] (device-readable) if the VIRTIO_SND_PCM_F_EVT_PERIOD
+feature bit is selected, then specifies the size of the period used by the driver.
+\end{description}
+
+\devicenormative{\subparagraph}{Stream Parameters}{Device Types / Sound Device / Device Operation / PCM Stream Parameters}
+
+\begin{itemize*}
+\item If the device has an intermediate buffer, its size MUST be no less than
+the specified \field{buffer_bytes} value.
+\end{itemize*}
+
+\drivernormative{\subparagraph}{Stream Parameters}{Device Types / Sound Device / Device Operation / PCM Stream Parameters}
+
+\begin{itemize*}
+\item The driver MUST NOT set undefined feature, format and rate values.
+\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 capabilities.
+\item The driver MUST NOT set feature bits which the device did not offer.
+\item The driver MUST NOT set the VIRTIO_SND_PCM_F_HOST_MEM and VIRTIO_SND_PCM_F_GUEST_MEM
+bits at the same time.
+\item If the bits associated with the shared memory are not selected, the driver
+MUST use the tx and rx queues for data transfer
+(see \nameref{sec:Device Types / Sound Device / Device Operation / PCM IO Messages}).
+\item If the VIRTIO_SND_PCM_F_EVT_PERIOD feature bit is not selected, then
+the driver MUST set the \field{period_bytes} field to 0.
+\end{itemize*}
+
+\paragraph{VIRTIO_SND_R_PCM_PREPARE}
+
+Prepare a stream with specified stream ID.
+
+\paragraph{VIRTIO_SND_R_PCM_RELEASE}
+
+Release a stream with specified stream ID.
+
+\paragraph{VIRTIO_SND_R_PCM_START}
+
+Start a stream with specified stream ID.
+
+\paragraph{VIRTIO_SND_R_PCM_STOP}
+
+Stop a stream with specified stream ID.
+
+\paragraph{VIRTIO_SND_R_PCM_PAUSE}
+
+Set a stream with specified stream ID on pause.
+
+\paragraph{VIRTIO_SND_R_PCM_UNPAUSE}
+
+Resume a paused stream with specified stream ID.
+
+\subsubsection{PCM Notifications}
+
+The device can announce support for different PCM events using feature bits
+in the stream capabilities structure. To enable notifications, the driver
+must negotiate these features using the set stream parameters request
+(see \ref{sec:Device Types / Sound Device / Device Operation / PCM Stream Parameters}).
+
+The following PCM event types are currently supported:
+
+\begin{itemize*}
+\item VIRTIO_SND_EVT_PCM_PERIOD: send notifications about each elapsed period,
+the size of the period is specified in the \field{period_bytes} field.
+\item VIRTIO_SND_EVT_PCM_XRUN: send notification about underflow for the output
+stream or overflow for the input stream.
+\end{itemize*}
+
+A PCM notification consists of the struct virtio_snd_pcm_hdr
+(see \ref{sec:Device Types / Sound Device / Device Operation / PCM Control Messages})
+and contains the following fields:
+
+\begin{description}
+\item[\field{hdr}] (device-writable) contains event type (VIRTIO_SND_EVT_PCM_*).
+\item[\field{stream_id}] (device-writable) specifies a PCM stream identifier
+from 0 to \field{pcm_streams} - 1.
+\end{description}
+
+\drivernormative{\paragraph}{PCM Notifications}{Device Types / Sound Device / Device Operation / PCM Notifications}
+
+\begin{itemize}
+\item If VIRTIO_SND_PCM_F_EVT_PERIOD and/or VIRTIO_SND_PCM_F_EVT_XRUN is negotiated:
+\begin{itemize}
+\item The driver MUST populate the event queue with empty buffers before starting
+the stream.
+\item Each buffer MUST be at least the size of the struct virtio_snd_pcm_hdr.
+\end{itemize}
+\end{itemize}
+
+\subsubsection{PCM I/O Messages}\label{sec:Device Types / Sound Device / Device Operation / PCM IO Messages}
+
+An I/O message consists of the header part, followed by the buffer, and then
+the status part.
+
+\begin{lstlisting}
+/* an I/O header */
+struct virtio_snd_pcm_xfer {
+    le32 stream_id;
+};
+\end{lstlisting}
+
+The header part consists of the following field:
+
+\begin{description}
+\item[\field{stream_id}] (device-readable) specifies a PCM stream identifier
+from 0 to \field{pcm_streams} - 1.
+\end{description}
+
+The status part consists of the struct virtio_snd_hdr
+(see \ref{sec:Device Types / Sound Device / Device Operation}) and contains the
+following field:
+
+\begin{description}
+\item[\field{code}] (device-writable) contains VIRTIO_SND_S_OK if an operation
+is successful, and VIRTIO_SND_S_IO_ERR otherwise.
+\end{description}
+
+\paragraph{Output Stream}
+
+In case of an output stream, the header is followed by a device-readable buffer
+containing PCM frames for writing to the device. All messages are placed into
+the tx queue.
+
+\drivernormative{\subparagraph}{Output Stream}{Device Types / Sound Device / Device Operation / PCM Output Stream}
+
+\begin{itemize*}
+\item The driver MUST NOT place device-writable buffers into the tx queue.
+\end{itemize*}
+
+\paragraph{Input Stream}
+
+In case of an input stream, the header is followed by a device-writable buffer
+being populated with PCM frames from the device. All messages are placed into
+the rx queue.
+
+\devicenormative{\subparagraph}{Input Stream}{Device Types / Sound Device / Device Operation / PCM Input Stream}
+
+\begin{itemize*}
+\item The device MUST NOT complete the I/O request until the buffer is full.
+The only exception is the end of the stream.
+\end{itemize*}
+
+\drivernormative{\subparagraph}{Input Stream}{Device Types / Sound Device / Device Operation / PCM Input Stream}
+
+\begin{itemize*}
+\item The driver MUST populate the rx queue with empty buffers before starting
+the stream.
+\item The driver MUST NOT place device-readable buffers into the rx queue.
+\end{itemize*}
-- 
2.24.0


---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org

[virtio-dev] Re: [PATCH v3] virtio-snd: add virtio sound device specification

[Gerd Hoffmann]

  Hi,

> +\subsection{Feature bits}\label{sec:Device Types / Sound Device / Feature bits}
> +
> +None currently defined.

Flags for hostmem & guestmem here?

This could be an information the driver might want to know before
initializing the virtqueues.

> +\item START
> +\item PAUSE
> +\item UNPAUSE
> +\item STOP

Why pause/unpause?

I don't see a reason compelling for this given the driver can restart a
stream (i.e. set-params -> prepare -> start -> stop -> start -> stop ->
release).  Would a device handle stop and pause in a different way?

> +/* supported PCM stream features */
> +enum {
> +    VIRTIO_SND_PCM_F_HOST_MEM = 0,
> +    VIRTIO_SND_PCM_F_GUEST_MEM,

Is this useful as stream property?  I would expect when supported by a
device it would work for all streams.

> +    VIRTIO_SND_PCM_F_EVT_PERIOD,

I think this is only useful for hostmem/guestmem.

> +struct virtio_snd_pcm_set_params {
> +    struct virtio_snd_pcm_hdr hdr; /* .code = VIRTIO_SND_R_PCM_SET_PARAMS */
> +    u8 features;
> +    u8 channels;
> +    u8 format;
> +    u8 rate;
> +    le32 buffer_bytes;
> +    le32 period_bytes;
> +};
> +\end{lstlisting}
> +
> +The request contains the following fields:
> +
> +\begin{description}
> +\item[\field{features}] (device-readable) is a selected feature bit map:
> +\begin{itemize*}
> +\item VIRTIO_SND_PCM_F_HOST_MEM: use shared memory allocated by the host (TBD),
> +\item VIRTIO_SND_PCM_F_GUEST_MEM: use shared memory allocated by the guest (TBD),

I'd suggest to drop those bits here.  For hostmem + guestmem we probably
need a virtio_snd_pcm_set_params_{hostmem,guestmem} struct (and command)
with additional parameters anyway.

> +\item VIRTIO_SND_PCM_F_EVT_PERIOD: enable elapsed period notifications,

Same comment as above ;)

> +\begin{itemize*}
> +\item VIRTIO_SND_EVT_PCM_PERIOD: send notifications about each elapsed period,
> +the size of the period is specified in the \field{period_bytes} field.

Same comment as above ;)

> +\subsubsection{PCM I/O Messages}\label{sec:Device Types / Sound Device / Device Operation / PCM IO Messages}
> +
> +An I/O message consists of the header part, followed by the buffer, and then
> +the status part.

Each buffer MUST (or SHOULD?) be period_bytes in size.

The driver SHOULD queue (buffer_bytes / period_bytes) virtio buffers, so
the device has buffer_bytes total buffer space available.

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

[virtio-dev] Re: [PATCH v3] virtio-snd: add virtio sound device specification

[Anton Yakovlev]

Hi,

On 17.12.2019 15:45, Gerd Hoffmann wrote:
>    Hi,
> 
>> +\subsection{Feature bits}\label{sec:Device Types / Sound Device / Feature bits}
>> +
>> +None currently defined.
> 
> Flags for hostmem & guestmem here?
> 
> This could be an information the driver might want to know before
> initializing the virtqueues.

Yes, it might be. Yet this means that there will be two sets of features. And
current draft makes things more consistent (or at least looks so). Like, the
choice of transport looks more natural (just usual "feature negotiation").


>> +\item START
>> +\item PAUSE
>> +\item UNPAUSE
>> +\item STOP
> 
> Why pause/unpause?
> 
> I don't see a reason compelling for this given the driver can restart a
> stream (i.e. set-params -> prepare -> start -> stop -> start -> stop ->
> release).  Would a device handle stop and pause in a different way?

Well, usually restarting a paused stream means a lower start delay. But you
have a point here. I think it's worth asking other people's opinions on this.


>> +/* supported PCM stream features */
>> +enum {
>> +    VIRTIO_SND_PCM_F_HOST_MEM = 0,
>> +    VIRTIO_SND_PCM_F_GUEST_MEM,
> 
> Is this useful as stream property?  I would expect when supported by a
> device it would work for all streams.

Since we allowed different capabilities for different streams, now it's
possible to have different backends for them. I’m not sure how much this can
be a real scenario, but it is theoretically possible to have one backend that
is able to work with shared memory, and the other is not. The same can be said
for other stream feature bits.


>> +    VIRTIO_SND_PCM_F_EVT_PERIOD,
> 
> I think this is only useful for hostmem/guestmem.

Using message-based transport, an application can send the entire playback 
buffer at the beginning, and then send period-sized parts on each period 
notification.


>> +struct virtio_snd_pcm_set_params {
>> +    struct virtio_snd_pcm_hdr hdr; /* .code = VIRTIO_SND_R_PCM_SET_PARAMS */
>> +    u8 features;
>> +    u8 channels;
>> +    u8 format;
>> +    u8 rate;
>> +    le32 buffer_bytes;
>> +    le32 period_bytes;
>> +};
>> +\end{lstlisting}
>> +
>> +The request contains the following fields:
>> +
>> +\begin{description}
>> +\item[\field{features}] (device-readable) is a selected feature bit map:
>> +\begin{itemize*}
>> +\item VIRTIO_SND_PCM_F_HOST_MEM: use shared memory allocated by the host (TBD),
>> +\item VIRTIO_SND_PCM_F_GUEST_MEM: use shared memory allocated by the guest (TBD),
> 
> I'd suggest to drop those bits here.  For hostmem + guestmem we probably
> need a virtio_snd_pcm_set_params_{hostmem,guestmem} struct (and command)
> with additional parameters anyway.

See comment at the beginning.


>> +\subsubsection{PCM I/O Messages}\label{sec:Device Types / Sound Device / Device Operation / PCM IO Messages}
>> +
>> +An I/O message consists of the header part, followed by the buffer, and then
>> +the status part.
> 
> Each buffer MUST (or SHOULD?) be period_bytes in size.
> 
> The driver SHOULD queue (buffer_bytes / period_bytes) virtio buffers, so
> the device has buffer_bytes total buffer space available.

I think, it's better not to set queued buffer size restrictions. Periods are
an optional feature. If the driver is not interested in period notifications,
it's unclear what period_bytes means here.

Although, for the input stream we could say, that the total size of buffers in
the rx queue should be buffer_bytes.


> cheers,
>    Gerd
> 
> 

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

[virtio-dev] Re: [PATCH v3] virtio-snd: add virtio sound device specification

[Gerd Hoffmann]

> > > +/* supported PCM stream features */
> > > +enum {
> > > +    VIRTIO_SND_PCM_F_HOST_MEM = 0,
> > > +    VIRTIO_SND_PCM_F_GUEST_MEM,
> > 
> > Is this useful as stream property?  I would expect when supported by a
> > device it would work for all streams.
> 
> Since we allowed different capabilities for different streams, now it's
> possible to have different backends for them. I’m not sure how much this can
> be a real scenario, but it is theoretically possible to have one backend that
> is able to work with shared memory, and the other is not. The same can be said
> for other stream feature bits.

Hmm, ok.  Point.

Independant from that they should either be a note that these two are
nor (yet) specified and are just placeholders.  Or simply be deleted and
re-added with the actual specification for guestmem + hostmem.

> > > +    VIRTIO_SND_PCM_F_EVT_PERIOD,
> > 
> > I think this is only useful for hostmem/guestmem.
> 
> Using message-based transport, an application can send the entire playback
> buffer at the beginning, and then send period-sized parts on each period
> notification.

The driver can still split the data from the application into a set of
smaller virtio buffers.  And this is how I would write a driver.  Create
a bunch of buffers, period_bytes each, enough to cover buffer_bytes.
Then go submit them, re-use them robin-round (each time the device
completes a buffer re-fill and re-submit it), without a special case
with one big buffer for the (playback) stream start.

I still think this is not needed for message-based transport.

Maybe this is not needed for hostmem/guestmem either.  The point of
using shared memory is to avoid system calls and vmexits for buffer
management, so VIRTIO_SND_PCM_F_EVT_PERIOD doesn't look that useful
here too.  But maybe it is nice to have that as option.

> > > +\subsubsection{PCM I/O Messages}\label{sec:Device Types / Sound Device / Device Operation / PCM IO Messages}
> > > +
> > > +An I/O message consists of the header part, followed by the buffer, and then
> > > +the status part.
> > 
> > Each buffer MUST (or SHOULD?) be period_bytes in size.
> > 
> > The driver SHOULD queue (buffer_bytes / period_bytes) virtio buffers, so
> > the device has buffer_bytes total buffer space available.
> 
> I think, it's better not to set queued buffer size restrictions.

Lets make it SHOULD then.

> Periods are an optional feature. If the driver is not interested in
> period notifications, it's unclear what period_bytes means here.

Well, the driver has to manage buffers, so I can't see how a driver is
not interested in completion notifications.  If the driver isn't
interested in keeping the latency low by using lots of small buffers it
still needs to queue at least two (larger) buffers, so the device still
has data to continue playback when it completed a buffer.  And the
driver needs to know when the device is done with a buffer so it can
either recycle the buffer or free the pages.

> Although, for the input stream we could say, that the total size of
> buffers in the rx queue should be buffer_bytes.

Yes.

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

[virtio-dev] Re: [PATCH v3] virtio-snd: add virtio sound device specification

[Anton Yakovlev]

On 18.12.2019 12:52, Gerd Hoffmann wrote:
>>>> +/* supported PCM stream features */
>>>> +enum {
>>>> +    VIRTIO_SND_PCM_F_HOST_MEM = 0,
>>>> +    VIRTIO_SND_PCM_F_GUEST_MEM,
>>>
>>> Is this useful as stream property?  I would expect when supported by a
>>> device it would work for all streams.
>>
>> Since we allowed different capabilities for different streams, now it's
>> possible to have different backends for them. I’m not sure how much this can
>> be a real scenario, but it is theoretically possible to have one backend that
>> is able to work with shared memory, and the other is not. The same can be said
>> for other stream feature bits.
> 
> Hmm, ok.  Point.
> 
> Independant from that they should either be a note that these two are
> nor (yet) specified and are just placeholders.  Or simply be deleted and
> re-added with the actual specification for guestmem + hostmem.

I think having a note is better.


>>>> +    VIRTIO_SND_PCM_F_EVT_PERIOD,
>>>
>>> I think this is only useful for hostmem/guestmem.
>>
>> Using message-based transport, an application can send the entire playback
>> buffer at the beginning, and then send period-sized parts on each period
>> notification.
> 
> The driver can still split the data from the application into a set of
> smaller virtio buffers.  And this is how I would write a driver.  Create
> a bunch of buffers, period_bytes each, enough to cover buffer_bytes.
> Then go submit them, re-use them robin-round (each time the device
> completes a buffer re-fill and re-submit it), without a special case
> with one big buffer for the (playback) stream start.

Yes, but what about device side? Most likely, it will copy the contents of the
buffer to another location and immediately complete the request (otherwise it
is not clear when to complete). In addition, the driver must advance the
hardware position, and completing the request is the only possible place to do
this. In most cases, all this will not coincide with the period notification.
This means that the notification itself is an optional feature that may or may
not be enabled for message-based transport.

Of course, the benefits of such notifications largely depend on the
implementation of the driver. But at least people here claimed that they could
be useful.


> I still think this is not needed for message-based transport.
> 
> Maybe this is not needed for hostmem/guestmem either.  The point of
> using shared memory is to avoid system calls and vmexits for buffer
> management, so VIRTIO_SND_PCM_F_EVT_PERIOD doesn't look that useful
> here too.  But maybe it is nice to have that as option.

Earlier there was a big discussion on this topic, which resulted in having
this feature. The main point was it reflects the state of the hardware device,
not the virtual device itself. But they may not always be useful or necessary,
so this feature is optional.


>>>> +\subsubsection{PCM I/O Messages}\label{sec:Device Types / Sound Device / Device Operation / PCM IO Messages}
>>>> +
>>>> +An I/O message consists of the header part, followed by the buffer, and then
>>>> +the status part.
>>>
>>> Each buffer MUST (or SHOULD?) be period_bytes in size.
>>>
>>> The driver SHOULD queue (buffer_bytes / period_bytes) virtio buffers, so
>>> the device has buffer_bytes total buffer space available.
>>
>> I think, it's better not to set queued buffer size restrictions.
> 
> Lets make it SHOULD then.

Again, if periods are not used, then there's no period_bytes. And they are not
used, for example, in most low-latency applications.

(Well, technically speaking, low-latency applications in Linux must configure
period size for ALSA. But then they usually suppress period notifications and
send very small chunks of data, which usually do not correspond to the size of
the period at all.)

>> Periods are an optional feature. If the driver is not interested in
>> period notifications, it's unclear what period_bytes means here.
> 
> Well, the driver has to manage buffers, so I can't see how a driver is
> not interested in completion notifications.  If the driver isn't
> interested in keeping the latency low by using lots of small buffers it
> still needs to queue at least two (larger) buffers, so the device still
> has data to continue playback when it completed a buffer.  And the
> driver needs to know when the device is done with a buffer so it can
> either recycle the buffer or free the pages.

It seems that you have mixed up two different things: sending/completing a
request containing a buffer, and period notifications. They are not the same.


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

[virtio-dev] Re: [PATCH v3] virtio-snd: add virtio sound device specification

[Gerd Hoffmann]

  Hi,

> > The driver can still split the data from the application into a set of
> > smaller virtio buffers.  And this is how I would write a driver.  Create
> > a bunch of buffers, period_bytes each, enough to cover buffer_bytes.
> > Then go submit them, re-use them robin-round (each time the device
> > completes a buffer re-fill and re-submit it), without a special case
> > with one big buffer for the (playback) stream start.
> 
> Yes, but what about device side? Most likely, it will copy the contents of the
> buffer to another location and immediately complete the request (otherwise it
> is not clear when to complete).

qemu has rather small buffers backend buffers, to keep latencies low.
So, yes it would copy data to backend buffers.  No, it would most likely
not copy over everything immediately.  It will most likely leave buffers
in the virtqueue, reading the data piecewise in the audio backend timer
callback, complete the virtio request when it has read all data.  So
there wouldn't be a huge gap between completing the request and feeding
the data to the host hardware.

Another reason for this (beside keeping latencies low) is that it is a
good idea to avoid or at least limit guest-triggered allocations on the
host for security reasons.

While talking about latencies:  What happened to the idea to signal
additional device latencies (backend buffering and processing ...) to
the driver?

> In addition, the driver must advance the
> hardware position, and completing the request is the only possible place to do
> this. In most cases, all this will not coincide with the period notification.

So, period notification should be triggered when the device has actually
completed processing the data (i.e. host playback finished)?  I can see
that this can be useful.  I think qemu wouldn't be able to support that
with the current audio backend design, but that isn't a blocker given
this is an optional feature.  Also I think the spec should to clarify
how period notification is supposed to work, how that relates to
buffer completion and latencies.

> This means that the notification itself is an optional feature that may or may
> not be enabled for message-based transport.

Which transports are expected to support that?
If multiple transports:  Can a device signal it supports these
notifications for one transport but not for another?

> > > I think, it's better not to set queued buffer size restrictions.
> > 
> > Lets make it SHOULD then.
> 
> Again, if periods are not used, then there's no period_bytes. And they are not
> used, for example, in most low-latency applications.
> 
> (Well, technically speaking, low-latency applications in Linux must configure
> period size for ALSA. But then they usually suppress period notifications and
> send very small chunks of data, which usually do not correspond to the size of
> the period at all.)

So you want pass the data on to the device in whatever chunks the
userspace application hands them to the driver?  Ok, reasonable.

Should we define period_bytes as upper limit then, i.e. virtio buffers
should be period_bytes or smaller?

Completely different topic:  I think we should consider adding tags to
streams.  Some background:  In qemu we have two almost identical hda
codecs: hda-duplex and hda-micro.  The only difference is that
hda-duplex has the input stream tagged as "line-in" and the output
stream as "line-out", whereas hda-micro has the input channel tagged as
"microphone" and the output channel as "speaker".  hda-micro was created
because some windows application refused to accept "line-in" as audio
source.  So applications clearly do care, and in case you have two input
streams it would help applications pick a reasonable default.

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

[virtio-dev] Re: [PATCH v3] virtio-snd: add virtio sound device specification

[Anton Yakovlev]

On 18.12.2019 17:54, Mark Brown wrote:
> On Wed, Dec 18, 2019 at 04:34:12PM +0100, Anton Yakovlev wrote:
>> On 18.12.2019 12:52, Gerd Hoffmann wrote:
> 
>>> Well, the driver has to manage buffers, so I can't see how a driver is
>>> not interested in completion notifications.  If the driver isn't
>>> interested in keeping the latency low by using lots of small buffers it
>>> still needs to queue at least two (larger) buffers, so the device still
>>> has data to continue playback when it completed a buffer.  And the
>>> driver needs to know when the device is done with a buffer so it can
>>> either recycle the buffer or free the pages.
> 
>> It seems that you have mixed up two different things: sending/completing a
>> request containing a buffer, and period notifications. They are not the same.
> 
> What do you expect a period notification to be?  The default expecation
> would very much be that they're tied to what's going on with the
> buffers.

Elapsed period notification was initially proposed as an optional feature. But
then the discussion switched on to define the entire period-based operational
sequence built in protocol. Well, we can go this way and make periods not an
optional notification, but a full optional mode of operation. Then we can add
all the requirements that Gerd spoke of.


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

[virtio-dev] Re: [PATCH v3] virtio-snd: add virtio sound device specification

[Anton Yakovlev]

On 19.12.2019 08:17, Gerd Hoffmann wrote:
>    Hi,
> 
>>> The driver can still split the data from the application into a set of
>>> smaller virtio buffers.  And this is how I would write a driver.  Create
>>> a bunch of buffers, period_bytes each, enough to cover buffer_bytes.
>>> Then go submit them, re-use them robin-round (each time the device
>>> completes a buffer re-fill and re-submit it), without a special case
>>> with one big buffer for the (playback) stream start.
>>
>> Yes, but what about device side? Most likely, it will copy the contents of the
>> buffer to another location and immediately complete the request (otherwise it
>> is not clear when to complete).
> 
> qemu has rather small buffers backend buffers, to keep latencies low.
> So, yes it would copy data to backend buffers.  No, it would most likely
> not copy over everything immediately.  It will most likely leave buffers
> in the virtqueue, reading the data piecewise in the audio backend timer
> callback, complete the virtio request when it has read all data.  So
> there wouldn't be a huge gap between completing the request and feeding
> the data to the host hardware.

Yes, there is a gap, and this causes the following two concerns:

1. An application can rewind its pointer and rewrite part of this buffer. The
worst case is if the application rewrites the part currently being read by the
device.

2. This approach looks like shared memory on a smaller scale, and the whole
meaning of the messages is lost. Because sending a message assumes that what's
gone is gone.


> Another reason for this (beside keeping latencies low) is that it is a
> good idea to avoid or at least limit guest-triggered allocations on the
> host for security reasons.

Yes, that makes sense.


> While talking about latencies:  What happened to the idea to signal
> additional device latencies (backend buffering and processing ...) to
> the driver?

Yeah, such an additional latency has a runtime nature, and we could report it
as part of an I/O request completion. We only need to choose units (time or
bytes).


>> In addition, the driver must advance the
>> hardware position, and completing the request is the only possible place to do
>> this. In most cases, all this will not coincide with the period notification.
> 
> So, period notification should be triggered when the device has actually
> completed processing the data (i.e. host playback finished)?  I can see
> that this can be useful.  I think qemu wouldn't be able to support that
> with the current audio backend design, but that isn't a blocker given
> this is an optional feature.  Also I think the spec should to clarify
> how period notification is supposed to work, how that relates to
> buffer completion and latencies.
> 
>> This means that the notification itself is an optional feature that may or may
>> not be enabled for message-based transport.
> 
> Which transports are expected to support that?
> If multiple transports:  Can a device signal it supports these
> notifications for one transport but not for another?

I think this does not depend on transport, but take a look at the comment
below.


>>>> I think, it's better not to set queued buffer size restrictions.
>>>
>>> Lets make it SHOULD then.
>>
>> Again, if periods are not used, then there's no period_bytes. And they are not
>> used, for example, in most low-latency applications.
>>
>> (Well, technically speaking, low-latency applications in Linux must configure
>> period size for ALSA. But then they usually suppress period notifications and
>> send very small chunks of data, which usually do not correspond to the size of
>> the period at all.)
> 
> So you want pass the data on to the device in whatever chunks the
> userspace application hands them to the driver?  Ok, reasonable.
> 
> Should we define period_bytes as upper limit then, i.e. virtio buffers
> should be period_bytes or smaller?

I think, it's becoming more and more complicated and we should revise
everything.

Now we have only optional notifications (periods and xrun). But let's say,
that the device MAY send the xrun notifications and MUST send the period
notifications, and remove them from feature bits.

Then we define requirements for message-based transport:
1. the tx and rx queues are filled in with buffers having period_bytes size,
2. buffer_bytes % period_bytes == 0,
3. total buffers size in the queue is buffer_bytes.

It is still not clear, when to complete playback buffers, but at least such
approach makes the driver being interrupt-driven.

Basically, it is what you said but with little changes.


> Completely different topic:  I think we should consider adding tags to
> streams.  Some background:  In qemu we have two almost identical hda
> codecs: hda-duplex and hda-micro.  The only difference is that
> hda-duplex has the input stream tagged as "line-in" and the output
> stream as "line-out", whereas hda-micro has the input channel tagged as
> "microphone" and the output channel as "speaker".  hda-micro was created
> because some windows application refused to accept "line-in" as audio
> source.  So applications clearly do care, and in case you have two input
> streams it would help applications pick a reasonable default.

Should it be a fixed-size field in the capabilities structure?


> 
> cheers,
>    Gerd
> 
> 

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

[virtio-dev] Re: [PATCH v3] virtio-snd: add virtio sound device specification

[Gerd Hoffmann]

  Hi,

> > qemu has rather small buffers backend buffers, to keep latencies low.
> > So, yes it would copy data to backend buffers.  No, it would most likely
> > not copy over everything immediately.  It will most likely leave buffers
> > in the virtqueue, reading the data piecewise in the audio backend timer
> > callback, complete the virtio request when it has read all data.  So
> > there wouldn't be a huge gap between completing the request and feeding
> > the data to the host hardware.
> 
> Yes, there is a gap, and this causes the following two concerns:
> 
> 1. An application can rewind its pointer and rewrite part of this buffer. The
> worst case is if the application rewrites the part currently being read by the
> device.

Depends on the driver implementation.  If the driver will copy over the
data to a buffer not visible to the userspace application this can't
happen.  If the driver allows userspace mmap the buffer, then queues
virtio requests with pointers to those buffers, then yes, the
application could modify buffers which are sitting in the queue waiting
to be processed by the device.

> > While talking about latencies:  What happened to the idea to signal
> > additional device latencies (backend buffering and processing ...) to
> > the driver?
> 
> Yeah, such an additional latency has a runtime nature, and we could report it
> as part of an I/O request completion. We only need to choose units (time or
> bytes).

I'd stick to bytes, for consistency with period and buffer sizes.

> Now we have only optional notifications (periods and xrun). But let's say,
> that the device MAY send the xrun notifications and MUST send the period
> notifications, and remove them from feature bits.
> 
> Then we define requirements for message-based transport:
> 1. the tx and rx queues are filled in with buffers having period_bytes size,
> 2. buffer_bytes % period_bytes == 0,
> 3. total buffers size in the queue is buffer_bytes.
> 
> It is still not clear, when to complete playback buffers, but at least such
> approach makes the driver being interrupt-driven.

Period notification would be implicit (playback buffer completion) or
explicit event queue message?

On playback buffer completion: adding a latency field to the completion
field could solve that nicely I think.  The latency field would specify
how long it will take until the device finishes playing that buffer.  So
in case the device completes the buffer when playback is finished it
would fill in "0" there.  In case the device completes the buffer after
copying over the data to the internal buffer it would fill in the
internal buffer size there (or maybe even the number of bytes which are
currently used in the internal buffer).

> > Completely different topic:  I think we should consider adding tags to
> > streams.  Some background:  In qemu we have two almost identical hda
> > codecs: hda-duplex and hda-micro.  The only difference is that
> > hda-duplex has the input stream tagged as "line-in" and the output
> > stream as "line-out", whereas hda-micro has the input channel tagged as
> > "microphone" and the output channel as "speaker".  hda-micro was created
> > because some windows application refused to accept "line-in" as audio
> > source.  So applications clearly do care, and in case you have two input
> > streams it would help applications pick a reasonable default.
> 
> Should it be a fixed-size field in the capabilities structure?

Yes, a simple enum would do the trick I think.

Not sure if we want add more properties.  hda codecs can also specify
the color coding of physical jacks (line-in is red, ...) for example.
Probably not useful for most use cases (i.e. when sound data lands @
pulseaudio where the user can re-route things as he wants), but it might
be useful when a fixed physical host output is assigned to the guest.

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

Re: [virtio-dev] Re: [PATCH v3] virtio-snd: add virtio sound device specification

[Anton Yakovlev]

Hi,

On 20.12.2019 08:53, Gerd Hoffmann wrote:
>    Hi,
> 
>>> qemu has rather small buffers backend buffers, to keep latencies low.
>>> So, yes it would copy data to backend buffers.  No, it would most likely
>>> not copy over everything immediately.  It will most likely leave buffers
>>> in the virtqueue, reading the data piecewise in the audio backend timer
>>> callback, complete the virtio request when it has read all data.  So
>>> there wouldn't be a huge gap between completing the request and feeding
>>> the data to the host hardware.
>>
>> Yes, there is a gap, and this causes the following two concerns:
>>
>> 1. An application can rewind its pointer and rewrite part of this buffer. The
>> worst case is if the application rewrites the part currently being read by the
>> device.
> 
> Depends on the driver implementation.  If the driver will copy over the
> data to a buffer not visible to the userspace application this can't
> happen.  If the driver allows userspace mmap the buffer, then queues
> virtio requests with pointers to those buffers, then yes, the
> application could modify buffers which are sitting in the queue waiting
> to be processed by the device.

Yes, it seems that the protocol cannot protect against all possible problems.
Let's delegate this to the device / driver implementation then.


>>> While talking about latencies:  What happened to the idea to signal
>>> additional device latencies (backend buffering and processing ...) to
>>> the driver?
>>
>> Yeah, such an additional latency has a runtime nature, and we could report it
>> as part of an I/O request completion. We only need to choose units (time or
>> bytes).
> 
> I'd stick to bytes, for consistency with period and buffer sizes.

Great!


>> Now we have only optional notifications (periods and xrun). But let's say,
>> that the device MAY send the xrun notifications and MUST send the period
>> notifications, and remove them from feature bits.
>>
>> Then we define requirements for message-based transport:
>> 1. the tx and rx queues are filled in with buffers having period_bytes size,
>> 2. buffer_bytes % period_bytes == 0,
>> 3. total buffers size in the queue is buffer_bytes.
>>
>> It is still not clear, when to complete playback buffers, but at least such
>> approach makes the driver being interrupt-driven.
> 
> Period notification would be implicit (playback buffer completion) or
> explicit event queue message?

Good question. I think, for message-base transport they would be implicit,
since we will require to enqueue period_size length buffers. The only
exception here will be the end of the stream for playback.

As for shared memory, I think that they could work as an optional notification
(as suggested earlier), and the driver should enable them using the set params
request (setting the feature bit).


> On playback buffer completion: adding a latency field to the completion
> field could solve that nicely I think.  The latency field would specify
> how long it will take until the device finishes playing that buffer.  So
> in case the device completes the buffer when playback is finished it
> would fill in "0" there.  In case the device completes the buffer after
> copying over the data to the internal buffer it would fill in the
> internal buffer size there (or maybe even the number of bytes which are
> currently used in the internal buffer).

Yes, nice idea. Then let's put it into the spec.


>>> Completely different topic:  I think we should consider adding tags to
>>> streams.  Some background:  In qemu we have two almost identical hda
>>> codecs: hda-duplex and hda-micro.  The only difference is that
>>> hda-duplex has the input stream tagged as "line-in" and the output
>>> stream as "line-out", whereas hda-micro has the input channel tagged as
>>> "microphone" and the output channel as "speaker".  hda-micro was created
>>> because some windows application refused to accept "line-in" as audio
>>> source.  So applications clearly do care, and in case you have two input
>>> streams it would help applications pick a reasonable default.
>>
>> Should it be a fixed-size field in the capabilities structure?
> 
> Yes, a simple enum would do the trick I think.

And just reuse existing values from the HDA spec, right?


> Not sure if we want add more properties.  hda codecs can also specify
> the color coding of physical jacks (line-in is red, ...) for example.
> Probably not useful for most use cases (i.e. when sound data lands @
> pulseaudio where the user can re-route things as he wants), but it might
> be useful when a fixed physical host output is assigned to the guest.

Well, if we are gonna introduce jacks colors, then we should introduce a jack
entity itself. Like, define some kind of jack descriptor, enumerate all
available jacks for the stream, putting into the descriptor its color and
other properties, maybe allow jack hotplug/unplug, etc.


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

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

Re: [virtio-dev] Re: [PATCH v3] virtio-snd: add virtio sound device specification

[Gerd Hoffmann]

  Hi,

> > Period notification would be implicit (playback buffer completion) or
> > explicit event queue message?
> 
> Good question. I think, for message-base transport they would be implicit,
> since we will require to enqueue period_size length buffers. The only
> exception here will be the end of the stream for playback.

Why is the end-of-stream different?

> As for shared memory, I think that they could work as an optional notification
> (as suggested earlier), and the driver should enable them using the set params
> request (setting the feature bit).

Sounds good.

> > Yes, a simple enum would do the trick I think.
> 
> And just reuse existing values from the HDA spec, right?

Looks useful, simplifies things.

> > Not sure if we want add more properties.  hda codecs can also specify
> > the color coding of physical jacks (line-in is red, ...) for example.
> > Probably not useful for most use cases (i.e. when sound data lands @
> > pulseaudio where the user can re-route things as he wants), but it might
> > be useful when a fixed physical host output is assigned to the guest.
> 
> Well, if we are gonna introduce jacks colors, then we should introduce a jack
> entity itself. Like, define some kind of jack descriptor, enumerate all
> available jacks for the stream, putting into the descriptor its color and
> other properties, maybe allow jack hotplug/unplug, etc.

We also need routing then (enable/disable jacks per stream), and link
detection (is something plugged into the jack?) could be useful too.

Should we make that an optional extension (and possibly hash out these
details later)?  If you assign host machine jacks to your guest all this
is useful.  If your guests's sound is routed to pulseaudio for playback
not so much.

Note: I'll be offline for xmas and new year until january.

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

Re: [virtio-dev] Re: [PATCH v3] virtio-snd: add virtio sound device specification

[Anton Yakovlev]

On 20.12.2019 13:41, Gerd Hoffmann wrote:
>    Hi,
> 
>>> Period notification would be implicit (playback buffer completion) or
>>> explicit event queue message?
>>
>> Good question. I think, for message-base transport they would be implicit,
>> since we will require to enqueue period_size length buffers. The only
>> exception here will be the end of the stream for playback.
> 
> Why is the end-of-stream different?

Since, as in the case of the input stream, the last buffer may contain less
than period_bytes of data.


>>> Not sure if we want add more properties.  hda codecs can also specify
>>> the color coding of physical jacks (line-in is red, ...) for example.
>>> Probably not useful for most use cases (i.e. when sound data lands @
>>> pulseaudio where the user can re-route things as he wants), but it might
>>> be useful when a fixed physical host output is assigned to the guest.
>>
>> Well, if we are gonna introduce jacks colors, then we should introduce a jack
>> entity itself. Like, define some kind of jack descriptor, enumerate all
>> available jacks for the stream, putting into the descriptor its color and
>> other properties, maybe allow jack hotplug/unplug, etc.
> 
> We also need routing then (enable/disable jacks per stream), and link
> detection (is something plugged into the jack?) could be useful too.
> 
> Should we make that an optional extension (and possibly hash out these
> details later)?  If you assign host machine jacks to your guest all this
> is useful.  If your guests's sound is routed to pulseaudio for playback
> not so much.

Yeah, we can put it off for later. In addition, in the case of PA, sinks and
sources can be moved, added and removed dynamically, so some logic with jacks
might be useful there as well.


> Note: I'll be offline for xmas and new year until january.

Happy holidays!


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