[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH RESEND v17] xen/sndif: Add sound-device ABI





On 02/08/2017 05:16 PM, Konrad Rzeszutek Wilk wrote:
On Wed, Feb 08, 2017 at 10:48:45AM +0200, Oleksandr Andrushchenko wrote:
From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>

Add ABI for the two halves of a para-virtualized
sound driver to communicate with each other.

The ABI allows implementing audio playback and capture as
well as volume control and possibility to mute/unmute
audio sources.

Note: depending on the use-case backend can expose more sound
cards and PCM devices/streams than the underlying HW physically
has by employing SW mixers, configuring virtual sound streams,
channels etc. Thus, allowing fine tunned configurations per
frontend.
Thank you for being patient with us and flexible with the design!
And especially thank you for including the example of XenStore
keys - that helps a lot to understand it.
My pleasure
I only have one feedback, see below:

... giant snip..
+ * Request read/write - used for read (for capture) or write (for playback):
+ *         0                1                 2               3        octet
+ * +----------------+----------------+----------------+----------------+
+ * |               id                |   operation    |    reserved    | 4
+ * +----------------+----------------+----------------+----------------+
+ * |                             reserved                              | 8
+ * +----------------+----------------+----------------+----------------+
+ * |                              offset                               | 12
+ * +----------------+----------------+----------------+----------------+
+ * |                              length                               | 16
+ * +----------------+----------------+----------------+----------------+
+ * |                             reserved                              | 20
+ * +----------------+----------------+----------------+----------------+
+ * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
+ * +----------------+----------------+----------------+----------------+
+ * |                             reserved                              | 32
+ * +----------------+----------------+----------------+----------------+
+ *
+ * operation - XENSND_OP_READ for read or XENSND_OP_WRITE for write
+ */
+
I would move this structure:

struct xensnd_rw_req {
     uint32_t offset;
     uint32_t length;
};

right here (as you just introduced the XENSND_OP_[READ|WRITE] operation),
and then ..
will do that
+/*
+ * Request set/get volume - set/get channels' volume of the stream given:
+ *         0                1                 2               3        octet
+ * +----------------+----------------+----------------+----------------+
+ * |               id                |   operation    |    reserved    | 4
+ * +----------------+----------------+----------------+----------------+
+ * |                             reserved                              | 8
+ * +----------------+----------------+----------------+----------------+
+ * |                              offset                               | 12
+ * +----------------+----------------+----------------+----------------+
+ * |                              length                               | 16
+ * +----------------+----------------+----------------+----------------+
+ * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
+ * +----------------+----------------+----------------+----------------+
+ * |                             reserved                              | 32
+ * +----------------+----------------+----------------+----------------+
+ *
+ * operation - XENSND_OP_SET_VOLUME for volume set
+ *   or XENSND_OP_GET_VOLUME for volume get
+ * Buffer passed with XENSND_OP_OPEN is used to exchange volume
+ * values:
+ *
+ *         0                1                 2               3        octet
+ * +----------------+----------------+----------------+----------------+
+ * |                             channel[0]                            | 4
+ * +----------------+----------------+----------------+----------------+
+ * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
+ * +----------------+----------------+----------------+----------------+
+ * |                             channel[i]                            | i*4
+ * +----------------+----------------+----------------+----------------+
+ * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
+ * +----------------+----------------+----------------+----------------+
+ * |                           channel[N - 1]                          | 
(N-1)*4
+ * +----------------+----------------+----------------+----------------+
+ *
+ * N = XENSND_OP_OPEN.pcm_channels
+ * i - uint8_t, index of a channel
+ * channel[i] - sint32_t, volume of i-th channel
+ * Volume is expressed as a signed value in steps of 0.001 dB,
+ * while 0 being 0 dB.
+ *
+ * Request mute/unmute - mute/unmute stream:
+ *         0                1                 2               3        octet
+ * +----------------+----------------+----------------+----------------+
+ * |               id                |   operation    |    reserved    | 4
+ * +----------------+----------------+----------------+----------------+
+ * |                             reserved                              | 8
+ * +----------------+----------------+----------------+----------------+
+ * |                              offset                               | 12
+ * +----------------+----------------+----------------+----------------+
+ * |                              length                               | 16
+ * +----------------+----------------+----------------+----------------+
+ * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
+ * +----------------+----------------+----------------+----------------+
+ * |                             reserved                              | 32
+ * +----------------+----------------+----------------+----------------+
+ *
+ * operation - XENSND_OP_MUTE for mute or XENSND_OP_UNMUTE for unmute
+ * Buffer passed with XENSND_OP_OPEN is used to exchange mute/unmute
+ * values:
+ *
+ *                                   0                                 octet
+ * +----------------+----------------+----------------+----------------+
+ * |                             channel[0]                            | 4
+ * +----------------+----------------+----------------+----------------+
+ * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
+ * +----------------+----------------+----------------+----------------+
+ * |                             channel[i]                            | i*4
+ * +----------------+----------------+----------------+----------------+
+ * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
+ * +----------------+----------------+----------------+----------------+
+ * |                           channel[N - 1]                          | 
(N-1)*4
+ * +----------------+----------------+----------------+----------------+
+ *
+ * N = XENSND_OP_OPEN.pcm_channels
+ * i - uint8_t, index of a channel
+ * channel[i] - uint8_t, non-zero if i-th channel needs to be muted/unmuted
+ */
+
+struct xensnd_rw_req {
+    uint32_t offset;
+    uint32_t length;
+};
Just say that "The 'struct xensnd_rw_req' is also used for
XENSND_OP_[SET|GET]_VOLUME, XENSND_OP_[UN|]MUTE."

for clarity.
ok, will add
Besides that:

Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
Do you want me to send v18 with the changes requested?
May I put your "Reviewed-by: Konrad Rzeszutek Wilk" in v18?

Thank you,
Oleksandr

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.