[PATCH v5] sndif: add ABI for Para-virtual sound

[PATCH v5] sndif: add ABI for Para-virtual sound

[Oleksandr Dmytryshyn]

This is ABI for the two halves of a Para-virtual
sound driver to communicate with each to other.

Signed-off-by: Oleksandr Dmytryshyn <oleksandr.dmytryshyn@globallogic.com>
Signed-off-by: Iurii Konovalenko <iurii.konovalenko@globallogic.com>
---
Changes since v1:
 * removed __attribute__((__packed__)) from all structures definitions

Changes since v2:
 * removed all C structures
 * added protocol description between frontend and backend drivers

Changes since v3:
 * fixed some typos
 * renamed XENSND_PCM_FORMAT_FLOAT_** to XENSND_PCM_FORMAT_F32_**
 * renamed XENSND_PCM_FORMAT_FLOAT64_** to XENSND_PCM_FORMAT_F64_**
 * added 'id' field to the request and response packets
 * renamed 'stream_id' to 'stream' in the packets description
 * renamed 'pcm_data_rate' to 'pcm_rate' in the packets description
 * renamed 'pcm_stream_type' to 'pcm_type' in the packets description
 * removed 'stream_id' field from the response packets

Changes since v4:
 * renamed 'stream_id' back to the to 'stream' in the packets description
 * moved 'id' field to the upper position in the response packets

 xen/include/public/io/sndif.h | 338 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 338 insertions(+)
 create mode 100644 xen/include/public/io/sndif.h

diff --git a/xen/include/public/io/sndif.h b/xen/include/public/io/sndif.h
new file mode 100644
index 0000000..64adef0
--- /dev/null
+++ b/xen/include/public/io/sndif.h
@@ -0,0 +1,338 @@
+/******************************************************************************
+ * sndif.h
+ *
+ * Unified sound-device I/O interface for Xen guest OSes.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to
+ * deal in the Software without restriction, including without limitation the
+ * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
+ * sell copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ *
+ * Copyright (C) 2013-2015 GlobalLogic Inc.
+ */
+
+#ifndef __XEN_PUBLIC_IO_XENSND_H__
+#define __XEN_PUBLIC_IO_XENSND_H__
+
+/*
+ * Feature and Parameter Negotiation
+ * =================================
+ * The two halves of a Para-virtual sound driver utilize nodes within the
+ * XenStore to communicate capabilities and to negotiate operating parameters.
+ * This section enumerates these nodes which reside in the respective front and
+ * backend portions of the XenStore, following the XenBus convention.
+ *
+ * All data in the XenStore is stored as strings.  Nodes specifying numeric
+ * values are encoded in decimal.  Integer value ranges listed below are
+ * expressed as fixed sized integer types capable of storing the conversion
+ * of a properly formated node string, without loss of information.
+ *
+ * XenStore nodes in sections marked "PRIVATE" are solely for use by the
+ * driver side whose XenBus tree contains them.
+ *
+ *****************************************************************************
+ *                            Backend XenBus Nodes
+ *****************************************************************************
+ *
+ *------------------ Backend Device Identification (PRIVATE) ------------------
+ *
+ * stream_id
+ *      Values:         <uint32_t>
+ *
+ *      Each virtualized stream has own id starting from 0.
+ *      Within each frontend these ids must be unique.
+ *      Each stream can be opened only for playback or capture
+ *      at the same time.
+ *
+ * channels_cnt
+ *      Values:         <uint32_t>
+ *
+ *      The maximum amount of channels that can be supported by this stream.
+ *
+ *****************************************************************************
+ *                            Frontend XenBus Nodes
+ *****************************************************************************
+ *
+ *----------------------- Request Transport Parameters -----------------------
+ *
+ * event-channel
+ *      Values:         <uint32_t>
+ *
+ *      The identifier of the Xen event channel used to signal activity
+ *      in the ring buffer.
+ *
+ * ring-ref
+ *      Values:         <uint32_t>
+ *
+ *      The Xen grant reference granting permission for the backend to map
+ *      the sole page in a single page sized ring buffer.
+ */
+
+/*
+ * PCM FORMATS
+ *
+ * XENSND_PCM_FORMAT_<format>[_<endian>]
+ *
+ * format: <S/U/F><bits> or <name>
+ *     S - signed, U - unsigned, F - float
+ *     bits - 8, 16, 24, 32
+ *     name - MU_LAW, GSM, etc.
+ *
+ * endian: <LE/BE>, may be absent
+ *     LE - Little endian, BE - Big endian
+ */
+#define XENSND_PCM_FORMAT_S8            0
+#define XENSND_PCM_FORMAT_U8            1
+#define XENSND_PCM_FORMAT_S16_LE        2
+#define XENSND_PCM_FORMAT_S16_BE        3
+#define XENSND_PCM_FORMAT_U16_LE        4
+#define XENSND_PCM_FORMAT_U16_BE        5
+#define XENSND_PCM_FORMAT_S24_LE        6
+#define XENSND_PCM_FORMAT_S24_BE        7
+#define XENSND_PCM_FORMAT_U24_LE        8
+#define XENSND_PCM_FORMAT_U24_BE        9
+#define XENSND_PCM_FORMAT_S32_LE        10
+#define XENSND_PCM_FORMAT_S32_BE        11
+#define XENSND_PCM_FORMAT_U32_LE        12
+#define XENSND_PCM_FORMAT_U32_BE        13
+#define XENSND_PCM_FORMAT_F32_LE        14 /* 4-byte float, IEEE-754 32-bit, */
+#define XENSND_PCM_FORMAT_F32_BE        15 /* range -1.0 to 1.0              */
+#define XENSND_PCM_FORMAT_F64_LE        16 /* 8-byte float, IEEE-754 64-bit, */
+#define XENSND_PCM_FORMAT_F64_BE        17 /* range -1.0 to 1.0              */
+#define XENSND_PCM_FORMAT_IEC958_SUBFRAME_LE 18
+#define XENSND_PCM_FORMAT_IEC958_SUBFRAME_BE 19
+#define XENSND_PCM_FORMAT_MU_LAW        20
+#define XENSND_PCM_FORMAT_A_LAW         21
+#define XENSND_PCM_FORMAT_IMA_ADPCM     22
+#define XENSND_PCM_FORMAT_MPEG          23
+#define XENSND_PCM_FORMAT_GSM           24
+#define XENSND_PCM_FORMAT_SPECIAL       31 /* Any other unspecified format */
+
+/*
+ * REQUEST CODES.
+ */
+#define XENSND_OP_OPEN                  0
+#define XENSND_OP_CLOSE                 1
+#define XENSND_OP_READ                  2
+#define XENSND_OP_WRITE                 3
+#define XENSND_OP_SET_VOLUME            4
+#define XENSND_OP_GET_VOLUME            5
+
+/*
+ * The maximum amount of shared pages which can be used in any request
+ * from the frontend driver to the backend driver
+ */
+#define XENSND_MAX_PAGES_PER_REQUEST    12
+
+/* The maximum amount of channels per virtualized stream */
+#define XENSND_MAX_CHANNELS_PER_STREAM  16
+
+/*
+ * STATUS RETURN CODES.
+ */
+ /* Operation failed for some unspecified reason (e. g. -EIO). */
+#define XENSND_RSP_ERROR                 (-1)
+ /* Operation completed successfully. */
+#define XENSND_RSP_OKAY                  0
+
+/*
+ * Description of the protocol between the frontend and the backend driver.
+ *
+ * The two halves of a Para-virtual sound driver communicates with
+ * each to other using an shared page and event channel.
+ * Shared page contains a ring with request/response packets.
+ * All fields within the packet are always in little-endian byte order.
+ * Almost all fields within the packet are unsigned except
+ * the field 'status' in the responses packets, which is signed.
+ *
+ *
+ * All request packets have the same length (80 bytes)
+ *
+ * Request open - open an pcm stream for playback or capture:
+ *     0    1     2     3     4     5     6     7  octet
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |                      id                       |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |       operation       |      stream_id        |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |      pcm_format       |      pcm_channels     |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |       pcm_rate        |       pcm_type        |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |       reserved        |       reserved        |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/+
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |       reserved        |       reserved        |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ *
+ * id - private guest value, echoed in resp
+ * operation - XENSND_OP_OPEN
+ * stream_id - stream id, readed from the 'stream_id' XenBus node
+ * pcm_format - XENSND_PCM_FORMAT_???
+ * pcm_channels - channels count in stream
+ * pcm_rate - stream data rate
+ * pcm_type - 0: playback, 1: capture
+ *
+ *
+ * Request close - close an opened pcm stream:
+ *     0    1     2     3     4     5     6     7  octet
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |                      id                       |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |       operation       |       stream_id       |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |       reserved        |       reserved        |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/+
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |       reserved        |       reserved        |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ *
+ * id - private guest value, echoed in resp
+ * operation - XENSND_OP_CLOSE
+ * stream_id - stream id, readed from the 'stream_id' XenBus node
+ *
+ *
+ * Request read/write - used for read (for capture) or write (for playback):
+ *     0    1     2     3     4     5     6     7  octet
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |                      id                       |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |       operation       |       stream_id       |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |         length        |         gref0         |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |         gref1         |         gref2         |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/+
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |         gref11        |       reserved        |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |       reserved        |       reserved        |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ *
+ * id - private guest value, echoed in resp
+ * operation - XENSND_OP_READ or XENSND_OP_WRITE
+ * stream_id - stream id, readed from the 'stream_id' XenBus node
+ * length - read or write data length
+ * gref0 - gref11 - references to a grant entries for used pages in read/write
+ * request.
+ *
+ *
+ * Request set volume - set channels volume in stream:
+ *     0    1     2     3     4     5     6     7  octet
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |                      id                       |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |       operation       |       stream_id       |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |        vol_ch0        |        vol_ch1        |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |        vol_ch2        |        vol_ch3        |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |        vol_ch4        |        vol_ch5        |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/+
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |        vol_ch14       |        vol_ch15       |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ *
+ * id - private guest value, echoed in resp
+ * operation - XENSND_OP_SET_VOLUME
+ * stream_id - stream id, readed from the 'stream_id' XenBus node
+ * vol_ch0 - vol_ch15 - volume level for channel0 - channel15
+ *
+ *
+ * Request get volume - get channels volume in stream:
+ *     0    1     2     3     4     5     6     7  octet
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |                      id                       |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |       operation       |       stream_id       |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |       reserved        |       reserved        |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |       reserved        |       reserved        |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |       reserved        |       reserved        |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/+
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |       reserved        |       reserved        |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ *
+ * id - private guest value, echoed in resp
+ * operation - XENSND_OP_GET_VOLUME
+ * stream_id - stream id, readed from the 'stream_id' XenBus node
+ *
+ *
+ * All response packets have the same length (80 bytes)
+ *
+ * Response for all requests exept the XENSND_OP_GET_VOLUME:
+ *     0    1     2     3     4     5     6     7  octet
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |                      id                       |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |       operation       |         status        |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |       reserved        |       reserved        |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |       reserved        |       reserved        |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/+
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |       reserved        |       reserved        |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ *
+ * id - copied from request
+ * operation - XENSND_OP_??? - copied from request
+ * status - XENSND_RSP_???
+ *
+ *
+ * Response for XENSND_OP_GET_VOLUME request:
+ *     0    1     2     3     4     5     6     7  octet
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |                      id                       |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |       operation       |         status        |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |        vol_ch0        |        vol_ch1        |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |        vol_ch2        |        vol_ch3        |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/+
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ * |        vol_ch14       |        vol_ch15       |
+ * +-----+-----+-----+-----+-----+-----+-----+-----+
+ *
+ * id - copied from request
+ * operation - XENSND_OP_GET_VOLUME -copied from request
+ * status - XENSND_RSP_???
+ * vol_ch0 - vol_ch15 - volume level for channel0 - channel15
+ */
+
+struct xensnd_request {
+    uint8_t raw[80];
+};
+
+struct xensnd_response {
+    uint8_t raw[80];
+};
+
+DEFINE_RING_TYPES(xensnd, struct xensnd_request, struct xensnd_response);
+
+#endif /* __XEN_PUBLIC_IO_XENSND_H__ */
-- 
1.9.1

Re: [PATCH v5] sndif: add ABI for Para-virtual sound

[Roger Pau Monné]

El 22/01/15 a les 16.19, Oleksandr Dmytryshyn ha escrit:
[...]
> +/*
> + * Description of the protocol between the frontend and the backend driver.
> + *
> + * The two halves of a Para-virtual sound driver communicates with
> + * each to other using an shared page and event channel.
> + * Shared page contains a ring with request/response packets.
> + * All fields within the packet are always in little-endian byte order.
> + * Almost all fields within the packet are unsigned except
> + * the field 'status' in the responses packets, which is signed.
> + *
> + *
> + * All request packets have the same length (80 bytes)

80bytes is kind of a weird size. I would rather use 64bytes, which
aligns itself more nicely with cache lines.

[...]
> + * Request read/write - used for read (for capture) or write (for playback):
> + *     0    1     2     3     4     5     6     7  octet
> + * +-----+-----+-----+-----+-----+-----+-----+-----+
> + * |                      id                       |
> + * +-----+-----+-----+-----+-----+-----+-----+-----+
> + * |       operation       |       stream_id       |
> + * +-----+-----+-----+-----+-----+-----+-----+-----+
> + * |         length        |         gref0         |
> + * +-----+-----+-----+-----+-----+-----+-----+-----+
> + * |         gref1         |         gref2         |
> + * +-----+-----+-----+-----+-----+-----+-----+-----+
> + * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/+
> + * +-----+-----+-----+-----+-----+-----+-----+-----+
> + * |         gref11        |       reserved        |
> + * +-----+-----+-----+-----+-----+-----+-----+-----+
> + * |       reserved        |       reserved        |
> + * +-----+-----+-----+-----+-----+-----+-----+-----+
> + *
> + * id - private guest value, echoed in resp
> + * operation - XENSND_OP_READ or XENSND_OP_WRITE
> + * stream_id - stream id, readed from the 'stream_id' XenBus node
> + * length - read or write data length
> + * gref0 - gref11 - references to a grant entries for used pages in read/write
> + * request.

I don't know much about sound, but I expect that you aim at having low
latency rather than high throughput. Why do you leave the 3 last slots
unused? Shouldn't they be gref12..14, even if they are not used by the
current implementation?

Also, how often are the sound packets sent, and which size do they have?
I'm mainly asking because I don't know if it would be more suitable to
use the same strategy as we did with indirect descriptors in the block
protocol from the start.

> + *
> + *
> + * Request set volume - set channels volume in stream:
> + *     0    1     2     3     4     5     6     7  octet
> + * +-----+-----+-----+-----+-----+-----+-----+-----+
> + * |                      id                       |
> + * +-----+-----+-----+-----+-----+-----+-----+-----+
> + * |       operation       |       stream_id       |
> + * +-----+-----+-----+-----+-----+-----+-----+-----+
> + * |        vol_ch0        |        vol_ch1        |
> + * +-----+-----+-----+-----+-----+-----+-----+-----+
> + * |        vol_ch2        |        vol_ch3        |
> + * +-----+-----+-----+-----+-----+-----+-----+-----+
> + * |        vol_ch4        |        vol_ch5        |
> + * +-----+-----+-----+-----+-----+-----+-----+-----+
> + * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/+
> + * +-----+-----+-----+-----+-----+-----+-----+-----+
> + * |        vol_ch14       |        vol_ch15       |
> + * +-----+-----+-----+-----+-----+-----+-----+-----+
> + *
> + * id - private guest value, echoed in resp
> + * operation - XENSND_OP_SET_VOLUME
> + * stream_id - stream id, readed from the 'stream_id' XenBus node
> + * vol_ch0 - vol_ch15 - volume level for channel0 - channel15

I would put all the vol_ch[0..15] inside of a grant page, this way we
could add more channels without problems. A single grant page could
allow for 1024 channels (4096 / 4 = 1024), which seems more than enough.
This way the size of the request/response will also be smaller, because
AFAICT this is the bigger request size.

[...]
> + * Response for XENSND_OP_GET_VOLUME request:
> + *     0    1     2     3     4     5     6     7  octet
> + * +-----+-----+-----+-----+-----+-----+-----+-----+
> + * |                      id                       |
> + * +-----+-----+-----+-----+-----+-----+-----+-----+
> + * |       operation       |         status        |
> + * +-----+-----+-----+-----+-----+-----+-----+-----+
> + * |        vol_ch0        |        vol_ch1        |
> + * +-----+-----+-----+-----+-----+-----+-----+-----+
> + * |        vol_ch2        |        vol_ch3        |
> + * +-----+-----+-----+-----+-----+-----+-----+-----+
> + * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/+
> + * +-----+-----+-----+-----+-----+-----+-----+-----+
> + * |        vol_ch14       |        vol_ch15       |
> + * +-----+-----+-----+-----+-----+-----+-----+-----+
> + *
> + * id - copied from request
> + * operation - XENSND_OP_GET_VOLUME -copied from request
> + * status - XENSND_RSP_???
> + * vol_ch0 - vol_ch15 - volume level for channel0 - channel15
> + */

See my comment about "Request set volume" which also applies here.

Roger.

Re: [PATCH v5] sndif: add ABI for Para-virtual sound

[Ian Jackson]

Oleksandr Dmytryshyn writes ("[PATCH v5] sndif: add ABI for Para-virtual sound"):
> This is ABI for the two halves of a Para-virtual
> sound driver to communicate with each to other.
...
> + * Request open - open an pcm stream for playback or capture:

So "channels" here is "stereo", "mono", etc.

There doesn't seem to be anything in this interface that distinguishes
different input/output devices.

So for example, much real hardware will have a headphone output and
also built-in speakers, and a mic input and built-in microphone.

How is this to be exposed to the guest ?

Ian.

Re: [PATCH v5] sndif: add ABI for Para-virtual sound

[Oleksandr Dmytryshyn]

On Thu, Jan 22, 2015 at 5:39 PM, Roger Pau Monné <roger.pau@citrix.com> wrote:
> El 22/01/15 a les 16.19, Oleksandr Dmytryshyn ha escrit:
> [...]
>> +/*
>> + * Description of the protocol between the frontend and the backend driver.
>> + *
>> + * The two halves of a Para-virtual sound driver communicates with
>> + * each to other using an shared page and event channel.
>> + * Shared page contains a ring with request/response packets.
>> + * All fields within the packet are always in little-endian byte order.
>> + * Almost all fields within the packet are unsigned except
>> + * the field 'status' in the responses packets, which is signed.
>> + *
>> + *
>> + * All request packets have the same length (80 bytes)
>
> 80bytes is kind of a weird size. I would rather use 64bytes, which
> aligns itself more nicely with cache lines.
I'll rework the protocol in the next patch-set.

> [...]
>> + * Request read/write - used for read (for capture) or write (for playback):
>> + *     0    1     2     3     4     5     6     7  octet
>> + * +-----+-----+-----+-----+-----+-----+-----+-----+
>> + * |                      id                       |
>> + * +-----+-----+-----+-----+-----+-----+-----+-----+
>> + * |       operation       |       stream_id       |
>> + * +-----+-----+-----+-----+-----+-----+-----+-----+
>> + * |         length        |         gref0         |
>> + * +-----+-----+-----+-----+-----+-----+-----+-----+
>> + * |         gref1         |         gref2         |
>> + * +-----+-----+-----+-----+-----+-----+-----+-----+
>> + * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/+
>> + * +-----+-----+-----+-----+-----+-----+-----+-----+
>> + * |         gref11        |       reserved        |
>> + * +-----+-----+-----+-----+-----+-----+-----+-----+
>> + * |       reserved        |       reserved        |
>> + * +-----+-----+-----+-----+-----+-----+-----+-----+
>> + *
>> + * id - private guest value, echoed in resp
>> + * operation - XENSND_OP_READ or XENSND_OP_WRITE
>> + * stream_id - stream id, readed from the 'stream_id' XenBus node
>> + * length - read or write data length
>> + * gref0 - gref11 - references to a grant entries for used pages in read/write
>> + * request.
>
> I don't know much about sound, but I expect that you aim at having low
> latency rather than high throughput. Why do you leave the 3 last slots
> unused? Shouldn't they be gref12..14, even if they are not used by the
> current implementation?
I'll rework the protocol in the next patch-set (I'll reduce a packet size).

> Also, how often are the sound packets sent, and which size do they have?
> I'm mainly asking because I don't know if it would be more suitable to
> use the same strategy as we did with indirect descriptors in the block
> protocol from the start.
In my case this is about 3 packets per second with size about 16 KBytes.

>> + *
>> + *
>> + * Request set volume - set channels volume in stream:
>> + *     0    1     2     3     4     5     6     7  octet
>> + * +-----+-----+-----+-----+-----+-----+-----+-----+
>> + * |                      id                       |
>> + * +-----+-----+-----+-----+-----+-----+-----+-----+
>> + * |       operation       |       stream_id       |
>> + * +-----+-----+-----+-----+-----+-----+-----+-----+
>> + * |        vol_ch0        |        vol_ch1        |
>> + * +-----+-----+-----+-----+-----+-----+-----+-----+
>> + * |        vol_ch2        |        vol_ch3        |
>> + * +-----+-----+-----+-----+-----+-----+-----+-----+
>> + * |        vol_ch4        |        vol_ch5        |
>> + * +-----+-----+-----+-----+-----+-----+-----+-----+
>> + * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/+
>> + * +-----+-----+-----+-----+-----+-----+-----+-----+
>> + * |        vol_ch14       |        vol_ch15       |
>> + * +-----+-----+-----+-----+-----+-----+-----+-----+
>> + *
>> + * id - private guest value, echoed in resp
>> + * operation - XENSND_OP_SET_VOLUME
>> + * stream_id - stream id, readed from the 'stream_id' XenBus node
>> + * vol_ch0 - vol_ch15 - volume level for channel0 - channel15
>
> I would put all the vol_ch[0..15] inside of a grant page, this way we
> could add more channels without problems. A single grant page could
> allow for 1024 channels (4096 / 4 = 1024), which seems more than enough.
> This way the size of the request/response will also be smaller, because
> AFAICT this is the bigger request size.
I'll rework the protocol in the next patch-set.

> [...]
>> + * Response for XENSND_OP_GET_VOLUME request:
>> + *     0    1     2     3     4     5     6     7  octet
>> + * +-----+-----+-----+-----+-----+-----+-----+-----+
>> + * |                      id                       |
>> + * +-----+-----+-----+-----+-----+-----+-----+-----+
>> + * |       operation       |         status        |
>> + * +-----+-----+-----+-----+-----+-----+-----+-----+
>> + * |        vol_ch0        |        vol_ch1        |
>> + * +-----+-----+-----+-----+-----+-----+-----+-----+
>> + * |        vol_ch2        |        vol_ch3        |
>> + * +-----+-----+-----+-----+-----+-----+-----+-----+
>> + * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/+
>> + * +-----+-----+-----+-----+-----+-----+-----+-----+
>> + * |        vol_ch14       |        vol_ch15       |
>> + * +-----+-----+-----+-----+-----+-----+-----+-----+
>> + *
>> + * id - copied from request
>> + * operation - XENSND_OP_GET_VOLUME -copied from request
>> + * status - XENSND_RSP_???
>> + * vol_ch0 - vol_ch15 - volume level for channel0 - channel15
>> + */
>
> See my comment about "Request set volume" which also applies here.
I'll rework the protocol in the next patch-set.

> Roger.
>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v5] sndif: add ABI for Para-virtual sound

[Ian Jackson]

Oleksandr Dmytryshyn writes ("Re: [Xen-devel] [PATCH v5] sndif: add ABI for Para-virtual sound"):
> On Thu, Jan 22, 2015 at 5:39 PM, Roger Pau Monné <roger.pau@citrix.com> wrote:
> > Also, how often are the sound packets sent, and which size do they have?
> > I'm mainly asking because I don't know if it would be more suitable to
> > use the same strategy as we did with indirect descriptors in the block
> > protocol from the start.
...
> In my case this is about 3 packets per second with size about 16 KBytes.

That would put a floor on the latency of about 300ms.  I suspect
that's quite undesirable.

Ian.

Re: [PATCH v5] sndif: add ABI for Para-virtual sound

[Oleksandr Dmytryshyn]

On Thu, Jan 22, 2015 at 5:41 PM, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
> Oleksandr Dmytryshyn writes ("[PATCH v5] sndif: add ABI for Para-virtual sound"):
>> This is ABI for the two halves of a Para-virtual
>> sound driver to communicate with each to other.
> ...
>> + * Request open - open an pcm stream for playback or capture:
>
> So "channels" here is "stereo", "mono", etc.
>
> There doesn't seem to be anything in this interface that distinguishes
> different input/output devices.
>
> So for example, much real hardware will have a headphone output and
> also built-in speakers, and a mic input and built-in microphone.
>
> How is this to be exposed to the guest ?
Every stream handled in the own thread in the frontend driver.
Frontend sends command (read or write, etc) to the backend and
this is blocking command (within current stream). So I just
use different streams for playback and capture. In case if we have
headphone output and also built-in speakers, and a mic input and
built-in microphone we will use one stream for the headphone output,
one stream for the built-in speakers and one stream for the mic input.
And in this time all devices will be able to work simultaneously.


> Ian.

Re: [PATCH v5] sndif: add ABI for Para-virtual sound

[Oleksandr Dmytryshyn]

On Thu, Jan 22, 2015 at 5:56 PM, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
> Oleksandr Dmytryshyn writes ("Re: [Xen-devel] [PATCH v5] sndif: add ABI for Para-virtual sound"):
>> On Thu, Jan 22, 2015 at 5:39 PM, Roger Pau Monné <roger.pau@citrix.com> wrote:
>> > Also, how often are the sound packets sent, and which size do they have?
>> > I'm mainly asking because I don't know if it would be more suitable to
>> > use the same strategy as we did with indirect descriptors in the block
>> > protocol from the start.
> ...
>> In my case this is about 3 packets per second with size about 16 KBytes.
>
> That would put a floor on the latency of about 300ms.  I suspect
> that's quite undesirable.
This latency doesn't affect us because frontend and backend driver have
an separate thread for each virtualized stream. And when frontend driver
waits answer from the backend it just sleeps (in case Linux kernel it waits
for the completion).

> Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v5] sndif: add ABI for Para-virtual sound

[Ian Jackson]

Oleksandr Dmytryshyn writes ("Re: [PATCH v5] sndif: add ABI for Para-virtual sound"):
> On Thu, Jan 22, 2015 at 5:41 PM, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
> > So for example, much real hardware will have a headphone output and
> > also built-in speakers, and a mic input and built-in microphone.
> 
> > How is this to be exposed to the guest ?
> 
> Every stream handled in the own thread in the frontend driver.
> Frontend sends command (read or write, etc) to the backend and
> this is blocking command (within current stream). So I just
> use different streams for playback and capture.

Right, I gathered that.

> In case if we have
> headphone output and also built-in speakers, and a mic input and
> built-in microphone we will use one stream for the headphone output,
> one stream for the built-in speakers and one stream for the mic input.
> And in this time all devices will be able to work simultaneously.

Perhaps I'm missing something but I don't see where the necessary
information is passed in your protocol specification.

Let us consider only output for now.  How does the guest choose which
of the possibly-several output devices their stream goes to ?  Are
these the multiple stream_id's ?  Are they identified solely by
number ?  Is there a conventional mapping for these streams ?

Ian.

Re: [PATCH v5] sndif: add ABI for Para-virtual sound

[Ian Jackson]

Oleksandr Dmytryshyn writes ("Re: [Xen-devel] [PATCH v5] sndif: add ABI for Para-virtual sound"):
> On Thu, Jan 22, 2015 at 5:56 PM, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
> > Oleksandr Dmytryshyn writes ("Re: [Xen-devel] [PATCH v5] sndif: add ABI for Para-virtual sound"):
> >> In my case this is about 3 packets per second with size about 16 KBytes.
> >
> > That would put a floor on the latency of about 300ms.  I suspect
> > that's quite undesirable.
>
> This latency doesn't affect us because frontend and backend driver have
> an separate thread for each virtualized stream. And when frontend driver
> waits answer from the backend it just sleeps (in case Linux kernel it waits
> for the completion).

Your seem to be answering a different question to the one I intended
to ask.

What I mean is this:

Many people think it is important to reduce the latency of sound input
and output.  So, they want to reduce (a) the time between a piece of
software deciding to make a sound and (b) the time when that sound
starts to appear.  And the same for input.  This is important for
games, video conversations, and so on.

When sound output is occurring continuously, any piece of new sound
output needs to wait for the next packet to be sent.

If you are sending only 3 packets per second then the latency might be
as much as 1/3 second which I think is probably too much.

Ian.

Re: [PATCH v5] sndif: add ABI for Para-virtual sound

[Oleksandr Dmytryshyn]

On Thu, Jan 22, 2015 at 6:07 PM, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
> Oleksandr Dmytryshyn writes ("Re: [PATCH v5] sndif: add ABI for Para-virtual sound"):
>> On Thu, Jan 22, 2015 at 5:41 PM, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
>> > So for example, much real hardware will have a headphone output and
>> > also built-in speakers, and a mic input and built-in microphone.
>>
>> > How is this to be exposed to the guest ?
>>
>> Every stream handled in the own thread in the frontend driver.
>> Frontend sends command (read or write, etc) to the backend and
>> this is blocking command (within current stream). So I just
>> use different streams for playback and capture.
>
> Right, I gathered that.
>
>> In case if we have
>> headphone output and also built-in speakers, and a mic input and
>> built-in microphone we will use one stream for the headphone output,
>> one stream for the built-in speakers and one stream for the mic input.
>> And in this time all devices will be able to work simultaneously.
>
> Perhaps I'm missing something but I don't see where the necessary
> information is passed in your protocol specification.
>
> Let us consider only output for now.  How does the guest choose which
> of the possibly-several output devices their stream goes to ?  Are
> these the multiple stream_id's ?  Are they identified solely by
> number ?  Is there a conventional mapping for these streams ?
For now we are using a simplified PV sound drivers.
Every streams can be opened for playback or capture.
And backend driver uses alsa and uses 'dmix' plugin for playback
and 'dsnoop' plugin for capture.
For now this protocol doesn't support mapping for streams and needs to be
reworked. I'll rework It in the next patch-set.
New nodes will be added to the xenstore with the stream mappings.
For now a single stream is virtualized but I think that whole sound card should
be virtualized by single frontend-backend connection. In this case we will be
able to pass the alsa device name (which is used by backend), virtual sound
card name and number, stream type (playback or capture) and stream number
for each stream inside the virtualized sound card.

Example of the configuration (which will be read from the xenstore):
Sound cart 'default', 'card number 2'
'mic' <-> 'captute', 'stream 5' -> /dev/snd/pcmC2D5c
'headphones' <-> 'playback', 'stream 3' -> /dev/snd/pcmC2D3p

Our backend uses ALSA and opens streams by name.

> Ian.

Re: [PATCH v5] sndif: add ABI for Para-virtual sound

[Oleksandr Dmytryshyn]

On Thu, Jan 22, 2015 at 6:11 PM, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
> Oleksandr Dmytryshyn writes ("Re: [Xen-devel] [PATCH v5] sndif: add ABI for Para-virtual sound"):
>> On Thu, Jan 22, 2015 at 5:56 PM, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
>> > Oleksandr Dmytryshyn writes ("Re: [Xen-devel] [PATCH v5] sndif: add ABI for Para-virtual sound"):
>> >> In my case this is about 3 packets per second with size about 16 KBytes.
>> >
>> > That would put a floor on the latency of about 300ms.  I suspect
>> > that's quite undesirable.
>>
>> This latency doesn't affect us because frontend and backend driver have
>> an separate thread for each virtualized stream. And when frontend driver
>> waits answer from the backend it just sleeps (in case Linux kernel it waits
>> for the completion).
>
> Your seem to be answering a different question to the one I intended
> to ask.
>
> What I mean is this:
>
> Many people think it is important to reduce the latency of sound input
> and output.  So, they want to reduce (a) the time between a piece of
> software deciding to make a sound and (b) the time when that sound
> starts to appear.  And the same for input.  This is important for
> games, video conversations, and so on.
>
> When sound output is occurring continuously, any piece of new sound
> output needs to wait for the next packet to be sent.
>
> If you are sending only 3 packets per second then the latency might be
> as much as 1/3 second which I think is probably too much.
This latency is minimal.

Frontend side: the application wants to play sound.
It opens /dev/snd/pcmC0D0p file (for example) and issues some IOCTLs.
1. It sets the format and sound parameters (frontend sends request
to the backend to open a stream with this parameters).
2. It starts to send an PCM data usinf IOCTLs i. e. it writes an PCM
data to buffer which will be transfered from the frontend to the
backend. (frontend sends request to the backend to play an PCM stream)

So, the latency ((a) the time between a piece of software deciding
to make a sound and (b) the time when that sound starts to appear)
will be minimal - send 2 events to the backend and receive 2 responses
from the backend plus time for opening stream on the real sound device
in the backend side. This latency will be few milliseconds.

> Ian.

Re: [PATCH v5] sndif: add ABI for Para-virtual sound

[Vitaly Chernooky]

[-- Attachment #1.1: Type: text/plain, Size: 2214 bytes --]

Ian,

That numbers correspond only particular single usecase - mp3 playback and
are close to common for that usecase.

For live audio usecases there will be numbers close for common in that
usecases.

Olexandr,

Could you measure and provide us numbers for some live audio usecase?

With best regards,

Vitaly Chernooky

On Thu, Jan 22, 2015 at 6:11 PM, Ian Jackson <Ian.Jackson@eu.citrix.com>
wrote:

> Oleksandr Dmytryshyn writes ("Re: [Xen-devel] [PATCH v5] sndif: add ABI
> for Para-virtual sound"):
> > On Thu, Jan 22, 2015 at 5:56 PM, Ian Jackson <Ian.Jackson@eu.citrix.com>
> wrote:
> > > Oleksandr Dmytryshyn writes ("Re: [Xen-devel] [PATCH v5] sndif: add
> ABI for Para-virtual sound"):
> > >> In my case this is about 3 packets per second with size about 16
> KBytes.
> > >
> > > That would put a floor on the latency of about 300ms.  I suspect
> > > that's quite undesirable.
> >
> > This latency doesn't affect us because frontend and backend driver have
> > an separate thread for each virtualized stream. And when frontend driver
> > waits answer from the backend it just sleeps (in case Linux kernel it
> waits
> > for the completion).
>
> Your seem to be answering a different question to the one I intended
> to ask.
>
> What I mean is this:
>
> Many people think it is important to reduce the latency of sound input
> and output.  So, they want to reduce (a) the time between a piece of
> software deciding to make a sound and (b) the time when that sound
> starts to appear.  And the same for input.  This is important for
> games, video conversations, and so on.
>
> When sound output is occurring continuously, any piece of new sound
> output needs to wait for the next packet to be sent.
>
> If you are sending only 3 packets per second then the latency might be
> as much as 1/3 second which I think is probably too much.
>
> Ian.
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>



-- 
*Vitaly Chernooky | Senior Developer - Product Engineering and Development*
GlobalLogic
P *+380.44.4929695 ext.1136* M *+380.98.7920568* S cvv_2k
www.globallogic.com

http://www.globallogic.com/email_disclaimer.txt

[-- Attachment #1.2: Type: text/html, Size: 3450 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH v5] sndif: add ABI for Para-virtual sound

[Oleksandr Dmytryshyn]

On Thu, Jan 29, 2015 at 1:45 PM, Vitaly Chernooky
<vitalii.chernookyi@globallogic.com> wrote:
> Ian,
>
> That numbers correspond only particular single usecase - mp3 playback and
> are close to common for that usecase.
>
> For live audio usecases there will be numbers close for common in that
> usecases.
>
> Olexandr,
>
> Could you measure and provide us numbers for some live audio usecase?
Currently we have frontend in the Android.
I've done some measurements:

tinyplay file.wav:
time between samples is about 660 msec

Android media player (mp3 or video file):
time between samples is about 23 msec

So this value is application specific.

Unfortunately I can not measure it for live audio.


> With best regards,
>
> Vitaly Chernooky
>
> On Thu, Jan 22, 2015 at 6:11 PM, Ian Jackson <Ian.Jackson@eu.citrix.com>
> wrote:
>>
>> Oleksandr Dmytryshyn writes ("Re: [Xen-devel] [PATCH v5] sndif: add ABI
>> for Para-virtual sound"):
>> > On Thu, Jan 22, 2015 at 5:56 PM, Ian Jackson <Ian.Jackson@eu.citrix.com>
>> > wrote:
>> > > Oleksandr Dmytryshyn writes ("Re: [Xen-devel] [PATCH v5] sndif: add
>> > > ABI for Para-virtual sound"):
>> > >> In my case this is about 3 packets per second with size about 16
>> > >> KBytes.
>> > >
>> > > That would put a floor on the latency of about 300ms.  I suspect
>> > > that's quite undesirable.
>> >
>> > This latency doesn't affect us because frontend and backend driver have
>> > an separate thread for each virtualized stream. And when frontend driver
>> > waits answer from the backend it just sleeps (in case Linux kernel it
>> > waits
>> > for the completion).
>>
>> Your seem to be answering a different question to the one I intended
>> to ask.
>>
>> What I mean is this:
>>
>> Many people think it is important to reduce the latency of sound input
>> and output.  So, they want to reduce (a) the time between a piece of
>> software deciding to make a sound and (b) the time when that sound
>> starts to appear.  And the same for input.  This is important for
>> games, video conversations, and so on.
>>
>> When sound output is occurring continuously, any piece of new sound
>> output needs to wait for the next packet to be sent.
>>
>> If you are sending only 3 packets per second then the latency might be
>> as much as 1/3 second which I think is probably too much.
>>
>> Ian.
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
>
>
>
>
> --
> Vitaly Chernooky | Senior Developer - Product Engineering and Development
> GlobalLogic
> P +380.44.4929695 ext.1136 M +380.98.7920568 S cvv_2k
> www.globallogic.com
>
> http://www.globallogic.com/email_disclaimer.txt