OASIS Mailing List ArchivesView the OASIS mailing list archive below
or browse/search using MarkMail.


Help: OASIS Mailing Lists Help | MarkMail Help

virtio-dev message

[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]

Subject: Re: [virtio-dev] [PATCH v2] snd: Add virtio sound device specification

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:

Linux kernel driver:

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}
+\item[0] controlq
+\item[1] pcmq
+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}
+\item[VIRTIO_SND_F_PCM_OUTPUT (0)] Output PCM stream support.
+\item[VIRTIO_SND_F_PCM_INPUT (1)] Input PCM stream support.
+\subsection{Device configuration layout}\label{sec:Device Types / Sound Device / Device configuration layout}
+/* supported PCM sample formats */
+enum {

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_176400,
+    VIRTIO_SND_PCM_RATE_192000,
+/* 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;
+\subsubsection{Device configuration fields}
+The \field{pcm.output} and \field{pcm.input} fields contain PCM stream
+\item[\field{channels_min}] (driver-read-only) is a minimum number of supported
+\item[\field{channels_max}] (driver-read-only) is a maximum number of supported
+\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.
+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}
+\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.
+\devicenormative{\subsubsection}{Device Initialization}{Device Types / Sound Device / Device Initialization}
+\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.
+\drivernormative{\subsubsection}{Device Initialization}{Device Types / Sound Device / Device Initialization}
+\item The driver MUST configure and initialize all virtqueues.
+\item The driver SHOULD ignore the not defined format and rate bits.
+\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:
+enum {
+    /* PCM control request types */
+    /* generic status codes */
+    VIRTIO_SND_S_OK = 0x8000,

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[];
+A generic control message consists of a request part and a response part, and
+contains the following fields:
+\item[\field{request_code}] (device-read-only) specifies a device request code
+\item[\field{request_payload}] (device-read-only) contains request-specific
+\item[\field{response_status}] (device-writable) specifies a device response
+status (VIRTIO_SND_S_*).
+\item[\field{response_payload}] (device-writable) contains response-specific
+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:

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
+\item VIRTIO_SND_S_NOT_SUPP: requested operation or parameters are not supported.
+\item VIRTIO_SND_S_IO_ERR: an I/O error occurred.
+\subsubsection{Device Operation: PCM control requests}
+A PCM stream has the following command lifecycle:
+\item Set format
+\item Prepare
+\item Output only: transfer data for prebuffing
+\item Start
+\item Transfer data to/from the PCM device
+	\item Pause
+	\item Unpause
+\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.

+PCM control requests have or consist of a fixed header with the following
+layout structure:
+/* supported PCM stream types */
+enum {
+/* a PCM control request header */
+struct virtio_snd_pcm_hdr {
+    le32 code;
+    le32 stream;
+PCM control requests contain the following fields:
+\item[\field{code}] (device-read-only) specifies a PCM device request code
+\item[\field{stream}] (device-read-only) specifies a PCM stream type

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:
+/* 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];
+The PCM channel map information fields are:
+\item[\field{status}] (device-writable) specifies a device response status
+\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
+Set selected PCM format.
+struct virtio_snd_pcm_set_format {
+    struct virtio_snd_pcm_hdr hdr;
+    le16 channels;
+    le16 format;
+    le16 rate;
+    u16 padding;
+The PCM control request fields are:
+\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
+\item[\field{rate}] (device-read-only) specifies a desired PCM frame rate
+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.

+Start the PCM device.
+Stop the PCM device.
+Set the PCM device on pause.
+Unset the PCM device from pause.

Or "Resume the paused PCM device"?

Thanks, will be fixed.

+\devicenormative{\subsubsection}{PCM control requests}{Device Types / Sound Device / PCM control requests}
+\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.
+\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.
+\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.

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.


Anton Yakovlev
Senior Software Engineer

OpenSynergy GmbH
Rotherstr. 20, 10245 Berlin

Phone: +49 30 60 98 54 0
E-Mail: anton.yakovlev@opensynergy.com


Handelsregister/Commercial Registry: Amtsgericht Charlottenburg, HRB 108616B
GeschÃftsfÃhrer/Managing Director: Regis Adjamah

[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]