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

["Roger Pau Monné" <roger.pau@citrix.com>]

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.