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

2017-02-09 Thread Oleksandr Andrushchenko 

On 02/09/2017 02:48 PM, Konrad Rzeszutek Wilk wrote:

On February 9, 2017 3:46:10 AM EST, Oleksandr Andrushchenko 
 wrote:


On 02/08/2017 05:29 PM, Konrad Rzeszutek Wilk wrote:

. snip..

Reviewed-by: Konrad Rzeszutek Wilk 

Couple of issues I found in sndif while preparing displif for review:
1. missing string constants
+#define XENSND_FIELD_BE_VERSIONS"versions"
+#define XENSND_FIELD_FE_VERSION "version"

2. I would like to have one more part in the state diagrams section
which is currently missing: recovery flow for the case when
backend/frontend dies:

+ *--- Recovery flow
---
+ *
+ * In case of frontend unrecoverable errors backend handles that as
+ * if frontend goes into the XenbusStateClosed state.
+ *
+ * In case of backend unrecoverable errors frontend tries removing
+ * the emulated device. If this is possible at the moment of error,
+ * then frontend goes into the XenbusStateInitialising state and is
ready for
+ * new connection with backend. If the emulated device is still in use
and
+ * cannot be removed, then frontend goes into the
XenbusStateReconfiguring state
+ * until either the emulated device removed or backend initiates a new
+ * connection. On the emulated device removal frontend goes into the
+ * XenbusStateInitialising state.
+ *


Thanks, feel free to include this and my reviewed by tag.


Thank you

Thank you,
Oleksandr


Thanks!



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

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

2017-02-09 Thread Konrad Rzeszutek Wilk 
On February 9, 2017 3:46:10 AM EST, Oleksandr Andrushchenko 
 wrote:
>
>
>On 02/08/2017 05:29 PM, Konrad Rzeszutek Wilk wrote:
>> . snip..
 Reviewed-by: Konrad Rzeszutek Wilk 
>>>
>Couple of issues I found in sndif while preparing displif for review:
>1. missing string constants
>+#define XENSND_FIELD_BE_VERSIONS"versions"
>+#define XENSND_FIELD_FE_VERSION "version"
>
>2. I would like to have one more part in the state diagrams section
>which is currently missing: recovery flow for the case when
>backend/frontend dies:
>
>+ *--- Recovery flow 
>---
>+ *
>+ * In case of frontend unrecoverable errors backend handles that as
>+ * if frontend goes into the XenbusStateClosed state.
>+ *
>+ * In case of backend unrecoverable errors frontend tries removing
>+ * the emulated device. If this is possible at the moment of error,
>+ * then frontend goes into the XenbusStateInitialising state and is 
>ready for
>+ * new connection with backend. If the emulated device is still in use
>and
>+ * cannot be removed, then frontend goes into the 
>XenbusStateReconfiguring state
>+ * until either the emulated device removed or backend initiates a new
>+ * connection. On the emulated device removal frontend goes into the
>+ * XenbusStateInitialising state.
>+ *
>

Thanks, feel free to include this and my reviewed by tag.


>Thank you,
>Oleksandr


Thanks!

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

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

2017-02-09 Thread Oleksandr Andrushchenko 



On 02/08/2017 05:29 PM, Konrad Rzeszutek Wilk wrote:

. snip..

Reviewed-by: Konrad Rzeszutek Wilk 



Couple of issues I found in sndif while preparing displif for review:
1. missing string constants
+#define XENSND_FIELD_BE_VERSIONS"versions"
+#define XENSND_FIELD_FE_VERSION "version"

2. I would like to have one more part in the state diagrams section
which is currently missing: recovery flow for the case when
backend/frontend dies:

+ *--- Recovery flow 
---

+ *
+ * In case of frontend unrecoverable errors backend handles that as
+ * if frontend goes into the XenbusStateClosed state.
+ *
+ * In case of backend unrecoverable errors frontend tries removing
+ * the emulated device. If this is possible at the moment of error,
+ * then frontend goes into the XenbusStateInitialising state and is 
ready for

+ * new connection with backend. If the emulated device is still in use and
+ * cannot be removed, then frontend goes into the 
XenbusStateReconfiguring state

+ * until either the emulated device removed or backend initiates a new
+ * connection. On the emulated device removal frontend goes into the
+ * XenbusStateInitialising state.
+ *

Thank you,
Oleksandr

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

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

2017-02-08 Thread Oleksandr Andrushchenko 

On 02/08/2017 05:29 PM, Konrad Rzeszutek Wilk wrote:

. snip..

Reviewed-by: Konrad Rzeszutek Wilk 

Do you want me to send v18 with the changes requested?

Please wait a week for other folks to get a chance to look at this
patch.


May I put your "Reviewed-by: Konrad Rzeszutek Wilk" in v18?

Of course! Do please put my Reviewed-by on the patch and in a week
or so send it out - and I will commit it in (unless there are some
more comments to the patch and they need a respin, etc).


Thank you,
Oleksandr

Sounds like a plan,
thank you

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

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

2017-02-08 Thread Konrad Rzeszutek Wilk 
. snip..
> > Reviewed-by: Konrad Rzeszutek Wilk 
> Do you want me to send v18 with the changes requested?

Please wait a week for other folks to get a chance to look at this
patch.

> May I put your "Reviewed-by: Konrad Rzeszutek Wilk" in v18?

Of course! Do please put my Reviewed-by on the patch and in a week
or so send it out - and I will commit it in (unless there are some
more comments to the patch and they need a respin, etc).

> 
> Thank you,
> Oleksandr

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

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

2017-02-08 Thread Oleksandr Andrushchenko 



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 

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):
+ * 01 2   3octet
+ * +++++
+ * |   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:
+ * 01 2   3octet
+ * +++++
+ * |   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:
+ *
+ * 01 2   3octet
+ * +++++
+ * | 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

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

2017-02-08 Thread Konrad Rzeszutek Wilk 
On Wed, Feb 08, 2017 at 10:48:45AM +0200, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko 
> 
> 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.

I only have one feedback, see below:

... giant snip..
> + * Request read/write - used for read (for capture) or write (for playback):
> + * 01 2   3octet
> + * +++++
> + * |   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 ..
> +/*
> + * Request set/get volume - set/get channels' volume of the stream given:
> + * 01 2   3octet
> + * +++++
> + * |   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:
> + *
> + * 01 2   3octet
> + * +++++
> + * | 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.
> + *
> + *

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

2017-02-08 Thread Oleksandr Andrushchenko 
From: Oleksandr Andrushchenko 

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.

Signed-off-by: Oleksandr Andrushchenko 
Signed-off-by: Oleksandr Grytsov 
Signed-off-by: Oleksandr Dmytryshyn 
Signed-off-by: Iurii Konovalenko 

---
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

Changes since v5:
 * Slightly reworked request/response packets
 * Size of the request/response packet is changed to the 64 bytes
 * Now parameters for the XENSND_OP_SET_VOLUME/XENSND_OP_GET_VOLUME are
   passed via shared page
 * Added parameters for the XenBus nodes (now each stream can be mapped
   to the defined sound device in the backend using those parameters)
 * Added XenBus state diagrams description

Changes since v6:
 * Reworked streams description  in the Backend XenBus Nodes

Changes since v7:
 * re-worked backend device parameters to be more generic and flexible
 * extended frontend device parameters
 * slightly updated state machine description added mute/unmute commands
 * added constants for XenStore configuration strings
   (fields, PCM formats etc.)
 * changed request/response structure size from 64 octets to 16
 * introduced dynamic buffer allocation instead of
   static XENSND_MAX_PAGES_PER_REQUEST
 * re-worked open request to allow dynamic buffer allocation
 * re-worked read/write/volume requests, so they don't pass grefs:
   buffer from the open request is used for these operations to pass data
 * specified type of the volume value to be a signed value in steps
   of 0.001 dBm, while 0 being 0dBm.
 * added Linux include file with structure definitions

Changes since v8:
 * changed frontend-id to frontend_id
 * single sound card support, configured with bunch of
   devices/streams
 * clarifucation made on sample rates and formats expressed as
   decimals w/o any particular ordering
 * put description of migration/disconnection state
 * replaced __attribute__((packed)) to __packed
 * changed padding of ring structures to 64 to fit cache line
 * removeed #ifdef __KERNEL
 * explicitly stated which indices in XenStore configuration
   are contiguous
 * added description to what frontend's defaults are
 * made names of virtual card/devices optional
 * removed PCM_FORMAT_SPECIAL
 * changed volume units from dBm to dB

Changes since v9:
 * removed sndif_linux.h
 * moved all structures from sndif_linux.h to sndif.h
 * structures padded where needed
 * fixed Hz comment

Changes since v10:
 * fixed tabs to 4 spaces to comply with Xen coding style
 * added placeholders to empty structures (C89 concern)
 * added missing header includes

Changes since v11:
 * added XENSND_RSP_NOTSUPP error code
 * changed gref[0] to gref[1] with comment
 * modified comments on empty structures
 * removed "__" from member names
 * fixed indentation
 * added padding in union xensnd_resp
 * changed __XEN_PUBLIC_IO_XENSND_H__ to __XEN_PUBLIC_IO_SNDIF_H__

Changes since v12:
 * changed indentation for defines
 * missed ";" after gref[1]
 * documentation changes
 * changed req/resp structures
 * changed xensnd_page_directory structure
 * pass buffer size in open request

Changes since v13:
 * note on usage of grant ref 0
 * all page sizes are XEN_PAGE_SIZE
 * padding/reserved cleanup/fixes
 * removed empty structures

Changes since v14:
 * turn padding into reserved

Changes since v15:
 * protocol description reworked
 * added offset to block notations
 * used -XEN_EXX for status reporting
 * added protocol versioning
 * removed stream_idx from the packet structure
 * changed xenstore paths

Changes since v16:
 * updated XenStore path pattern
 * removed unused XenStore