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

Re: [Xen-devel] [PATCH v15] This is the ABI for the two halves of a para-virtualized sound driver to communicate with each to other.



On 01/27/2017 06:36 PM, Konrad Rzeszutek Wilk wrote:
On Fri, Jan 27, 2017 at 05:50:36PM +0200, Oleksandr Andrushchenko wrote:
thank you for comments, please find answers below

Can we please switch to v16 discussion as v15 vs v16 is
a big change?
<shrugs>

This channel seemed appropiate to hash this out.
sorry about that
  I will
look at v16 next week (out of time for reviews for today).
thank you for your time
See below.
On 01/27/2017 05:14 PM, Konrad Rzeszutek Wilk wrote:
On Thu, Jan 26, 2017 at 12:02:49PM +0200, Oleksandr Andrushchenko wrote:
Hi, Konrad!

First of all thank you very much for the valuable comments

and your time!

The number of changes (mostly in description) is going to

be huge, so do you think I can publish something like

"RFC v16" so we can discuss the updated patch?
RFC sadly means folks are going to mostly ignore it.
I would prefer you did not use RFC at this stage but just
did v16.
..snip..
sure
+ * Example for the frontend running in domain 5, instance of the driver
+ * in the front is 0 (single or first PV driver), device id 2,
+ * first stream (0):
+ * /local/domain/<frontend_id>/device/vsnd/<drv_idx>/
+ *         device/<dev_id>/stream/<stream_idx>/type = "p"
+ * /local/domain/5/device/vsnd/0/device/2/stream/0/type = "p"
Why do you need 'device' ?
just for clarity: the hierarchy of the sound driver would
be that a device may have multiple different streams.
And it just occured to me that you could also imply that
each device has an stream without the 'stream' in it.

So

/local/domain/5/device/vsnd/0/2/0/type = "p"

And the format is:
/local/domain/<front-id>/device/vsnd/<instance of PV 
driver>/<device-id>/<stream-id>
ok, so we'll end up with something like:

--------------------------------- Backend
-----------------------------------

/local/domain/0/backend/vsnd/1/0/frontend-id = "1"
/local/domain/0/backend/vsnd/1/0/frontend = "/local/domain/1/device/vsnd/0"
/local/domain/0/backend/vsnd/1/0/state = "4"
/local/domain/0/backend/vsnd/1/0/versions = "1,2"

<nods>
----------------------------- Card ----------------------------

/local/domain/1/device/vsnd/0/version = "1"
/local/domain/1/device/vsnd/0/short-name = "Card short name"
/local/domain/1/device/vsnd/0/long-name = "Card long name"
/local/domain/1/device/vsnd/0/sample-rates = "8000,32000,44100,48000,96000"
/local/domain/1/device/vsnd/0/sample-formats = "s8,u8,s16_le,s16_be"
/local/domain/1/device/vsnd/0/buffer-size = "262144"
<nods>
----------------------------- PCM device 0 ----------------------------

/local/domain/1/device/vsnd/0/0/name = "General analog"
/local/domain/1/device/vsnd/0/0/channels-max = "5"
<nods>
----------------------------- Stream 0, playback
----------------------------

/local/domain/1/device/vsnd/0/0/0/type = "p"
/local/domain/1/device/vsnd/0/0/0/sample-formats = "s8,u8"
/local/domain/1/device/vsnd/0/0/0/unique-id = "0"
/local/domain/1/device/vsnd/0/0/0/ring-ref = "386"
/local/domain/1/device/vsnd/0/0/0/event-channel = "15"
<nods>
------------------------------- PCM device 3
--------------------------------

/local/domain/1/device/vsnd/0/3/name = "HDMI-0"
/local/domain/1/device/vsnd/0/3/sample-rates = "8000,32000,44100"
<nods>
------------------------------ Stream 0, capture
----------------------------

/local/domain/1/device/vsnd/0/3/0/type = "c"
/local/domain/1/device/vsnd/0/3/0/unique-id = "2"
/local/domain/1/device/vsnd/0/3/0/ring-ref = "387"
/local/domain/1/device/vsnd/0/3/0/event-channel = "151"

Is this what you would like to see?
Yes.
ok, then I will use "/local/domain/1/device/vsnd/<driver-idx>/<pcm-device-id>/<stream-idx>/"
as the pattern, no "device", "stream" or whatever
IMO, all these values do not help understanding what it is, e.g.
this is equal to me if we have

/local/domain/1/device/vbd/51712/queue-0/ring-ref = "8"
/local/domain/1/device/vbd/51712/queue-0/event-channel = "3"
/local/domain/1/device/vbd/51712/queue-1 = ""
/local/domain/1/device/vbd/51712/queue-1/ring-ref = "9"

and then decided to go with

/local/domain/1/device/vbd/51712/0/ring-ref = "8"
/local/domain/1/device/vbd/51712/0/event-channel = "3"
/local/domain/1/device/vbd/51712/1/ring-ref = "9"

Can one easily tell what 0 or 1 after "51712/" is?
I do. But maybe my brain has been swimming in this too much?
I am looking at this from FAE's or integrator's POV, when one would need
to touch different parts of the system. "/0/0/0" makes me feel
sad just because either you have to keep all those numbers in mind (like you do)
or have documentation available (and know where it is, e.g. sources
of Xen or kernel).
I have a strong feeling that if you can change configuration without
knowing what index stands for it is always better and fail-safer then
just having numbers...
So, what is the final decision then?
Though I have a little of trouble with the 'instance of the
driver'. Are you suggesting you would have multiple
PV drivers of 'vsnd'? Can't the multiple device ids fulfill this?
it is possible, but the main use-case will have a single
PV driver with multiple PCM devices/streams
OK, so no need for this then.
.. snip..
+ * 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:
Oh. That means you these operation are in effect 'barrier' ones.

As the buffer must be flushed before hand otherwise you would be
overwriting data stream information.

You should probably mention this semantic need?
I think this is implementation specific and shouldn't
be in a generic protocol
How is that implementation specific? If there is something in the page
from the previous command you are overwritting those values.
ok, all the operations are synchronous for the stream given.
Ah. I missed that. Other drivers can be asynchronous.
That is you can have multiple 'read' that are handled by
the backend - and the response to the 'read' can come
in any order.

Hence was thinking along those lines.
got you
it means that if there is something left in the buffer
it will be overwritten by the next req/resp, so this is expected
Right, and that is my worry.

But you are saying that the 'response' MUST be given in the same
order as 'requests'. And that is not spelled out (it also
seems a bit limiting. Don't you want to be able to handle
in the future asynchronous responses?
if we have offset/len then we are on a safe side with async io
Or alternatively the XENSND_OP_SET_VOLUME/XENSND_OP_GET_VOLUME could have
a similar structure to 'struct xensnd_rw_req' so that you have
the offset and len?
a page can hold enough values, IMO
Let me see "640K ought to be enough"?
I was talking about 256 channels 4 bytes per each.
You are making assumptions here based on how the implementation
fills out the data structure. But the purpose of the design
is to detach oneself from the implementation and think of
alternative ways.

To capture your words:
"
So if read/write use that buffer, and the volume and muting
controls use it too, how do I change the volume while listening
without disturbing the read/write?
read/write do not happen continuously, e.g. sound card fills its
internal buffers (our buffer is busy) and then until next re-fill our
buffer is free. that means that there is almost no congestion and
always a good chance to set/get volume w/o problem
Jan
"

Well, that is implementation specific. What if some implementaiton
fills it back to back?

I would like you to add the 'offset' and 'len' so that we don't
dig a hole that we can't easily get out of.

ok, I will add
  * +----------------+----------------+----------------+----------------+
  * | offset                               | 12
  * +----------------+----------------+----------------+----------------+
  * | length                               | 16
  * +----------------+----------------+----------------+----------------+
to
1.Request set/get volume - set/get channels' volume of the stream given
2.Request mute/unmute - mute/unmute stream

By this change you enable a use-case when part of the shared buffer
is used for samples and part for volume/mute, right?
Correct.
ok, then

struct xensnd_rw_req {
    uint32_t offset;
    uint32_t len;
};
covers all the requests, but open/close
Do you want me to keep the same structure name (xensnd_rw_req)
or rename it to something like xensnd_io_req?

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