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

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

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.

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 ..
> +/*
> + * 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.

Besides that:

Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>

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