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.

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

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. I will
look at v16 next week (out of time for reviews for today).

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

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

> 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?

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

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