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

[Oleksandr Andrushchenko <andr2000@xxxxxxxxx>]

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

_______________________________________________
Embedded-pv-devel mailing list
Embedded-pv-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/cgi-bin/mailman/listinfo/embedded-pv-devel