From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
To: Oleksandr Andrushchenko <andr2000@gmail.com>
Cc: Iurii Konovalenko <iurii.konovalenko@globallogic.com>,
Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>,
vlad.babchuk@gmail.com,
Oleksandr Dmytryshyn <oleksandr.dmytryshyn@globallogic.com>,
andrii.anisov@gmail.com, olekstysh@gmail.com, al1img@gmail.com,
Oleksandr Grytsov <oleksandr_grytsov@epam.com>,
xen-devel@lists.xenproject.org, joculator@gmail.com
Subject: Re: [PATCH RESEND v17] xen/sndif: Add sound-device ABI
Date: Wed, 8 Feb 2017 10:16:08 -0500 [thread overview]
Message-ID: <20170208151607.GP27456@localhost.localdomain> (raw)
In-Reply-To: <1486543725-3357-2-git-send-email-andr2000@gmail.com>
On Wed, Feb 08, 2017 at 10:48:45AM +0200, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>
> 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@oracle.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
next prev parent reply other threads:[~2017-02-08 15:16 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-02-08 8:48 [PATCH RESEND v17] sndif: add ABI for para-virtual sound Oleksandr Andrushchenko
2017-02-08 8:48 ` [PATCH RESEND v17] xen/sndif: Add sound-device ABI Oleksandr Andrushchenko
2017-02-08 15:16 ` Konrad Rzeszutek Wilk [this message]
2017-02-08 15:23 ` Oleksandr Andrushchenko
2017-02-08 15:29 ` Konrad Rzeszutek Wilk
2017-02-08 15:30 ` Oleksandr Andrushchenko
2017-02-09 8:46 ` Oleksandr Andrushchenko
2017-02-09 12:48 ` Konrad Rzeszutek Wilk
2017-02-09 14:17 ` Oleksandr Andrushchenko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170208151607.GP27456@localhost.localdomain \
--to=konrad.wilk@oracle.com \
--cc=al1img@gmail.com \
--cc=andr2000@gmail.com \
--cc=andrii.anisov@gmail.com \
--cc=iurii.konovalenko@globallogic.com \
--cc=joculator@gmail.com \
--cc=oleksandr.dmytryshyn@globallogic.com \
--cc=oleksandr_andrushchenko@epam.com \
--cc=oleksandr_grytsov@epam.com \
--cc=olekstysh@gmail.com \
--cc=vlad.babchuk@gmail.com \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).