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

[Oleksandr Dmytryshyn <oleksandr.dmytryshyn@xxxxxxxxxxxxxxx>]

On Thu, Jan 22, 2015 at 5:39 PM, Roger Pau Monnà <roger.pau@xxxxxxxxxx> 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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel