|
[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 ok, then I will use "/local/domain/1/device/vsnd/<driver-idx>/<pcm-device-id>/<stream-idx>/"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. 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 feelsad 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/streamsOK, 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 protocolHow 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 expectedRight, 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, IMOLet 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 mutingcontrols 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 problemJan" 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
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |