[Xen-devel] [PATCH v8] This is ABI for the two halves of a Para-virtual sound driver to communicate with each to other.
Signed-off-by: Oleksandr Dmytryshyn Signed-off-by: Iurii Konovalenko Signed-off-by: Andrushchenko, Oleksandr Signed-off-by: Oleksandr Grytsov --- Changes since v1: * removed __attribute__((__packed__)) from all structures definitions Changes since v2: * removed all C structures * added protocol description between frontend and backend drivers Changes since v3: * fixed some typos * renamed XENSND_PCM_FORMAT_FLOAT_** to XENSND_PCM_FORMAT_F32_** * renamed XENSND_PCM_FORMAT_FLOAT64_** to XENSND_PCM_FORMAT_F64_** * added 'id' field to the request and response packets * renamed 'stream_id' to 'stream' in the packets description * renamed 'pcm_data_rate' to 'pcm_rate' in the packets description * renamed 'pcm_stream_type' to 'pcm_type' in the packets description * removed 'stream_id' field from the response packets Changes since v4: * renamed 'stream_id' back to the to 'stream' in the packets description * moved 'id' field to the upper position in the response packets Changes since v5: * Slightly reworked request/response packets * Size of the request/response packet is changed to the 64 bytes * Now parameters for the XENSND_OP_SET_VOLUME/XENSND_OP_GET_VOLUME are passed via shared page * Added parameters for the XenBus nodes (now each stream can be mapped to the defined sound device in the backend using those parameters) * Added XenBus state diagrams description Changes since v6: * Reworked streams description in the Backend XenBus Nodes Changes since v7: * re-worked backend device parameters to be more generic and flexible * extended frontend device parameters * slightly updated state machine description added mute/unmute commands * added constants for XenStore configuration strings (fields, PCM formats etc.) * changed request/response structure size from 64 octets to 16 * introduced dynamic buffer allocation instead of static XENSND_MAX_PAGES_PER_REQUEST * re-worked open request to allow dynamic buffer allocation * re-worked read/write/volume requests, so they don't pass grefs: buffer from the open request is used for these operations to pass data * specified type of the volume value to be a signed value in steps of 0.001 dBm, while 0 being 0dBm. * added Linux include file with structure definitions --- --- xen/include/public/io/sndif.h | 583 xen/include/public/io/sndif_linux.h | 114 +++ 2 files changed, 697 insertions(+) create mode 100644 xen/include/public/io/sndif.h create mode 100644 xen/include/public/io/sndif_linux.h diff --git a/xen/include/public/io/sndif.h b/xen/include/public/io/sndif.h new file mode 100644 index 000..df705a6 --- /dev/null +++ b/xen/include/public/io/sndif.h @@ -0,0 +1,583 @@ +/** + * sndif.h + * + * Unified sound-device I/O interface for Xen guest OSes. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to + * deal in the Software without restriction, including without limitation the + * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or + * sell copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER + * DEALINGS IN THE SOFTWARE. + * + * Copyright (C) 2013-2015 GlobalLogic Inc. + * Copyright (C) 2016 EPAM Systems Inc. + */ + +#ifndef __XEN_PUBLIC_IO_XENSND_H__ +#define __XEN_PUBLIC_IO_XENSND_H__ + +/* + * Front->back notifications: When enqueuing a new request, sending a + * notification can be made conditional on req_event (i.e., the generic + * hold-off mechanism provided by the ring macros). Backends must set + * req_event appropriately (e.g., using RING_FINAL_CHECK_FOR_REQUESTS()). + * + * Back->front notifications: When enqueuing a new response, sending a + * notification can be made conditional on rsp_event (i.e., the generic + * hold-off mechanism provided by the ring macros). Frontends must set + * rsp_event appropriately (e.g., using RING_FINAL_CHECK_FOR_RESPONSES()). + */ + +/* + * Feature and Parameter Negotiation + * = + * The two halves of a Para-virtual sound card driver utilize nodes within the + * XenStore to communicate capabilities and to negotiate op
Re: [Xen-devel] [PATCH v8] This is ABI for the two halves of a Para-virtual sound driver to communicate with each to other.
On Fri, Nov 04, 2016 at 10:51:33PM +0200, Andrushchenko, Oleksandr wrote: > Signed-off-by: Oleksandr Dmytryshyn > Signed-off-by: Iurii Konovalenko > Signed-off-by: Andrushchenko, Oleksandr > Signed-off-by: Oleksandr Grytsov > --- > Changes since v1: > * removed __attribute__((__packed__)) from all structures definitions > > Changes since v2: > * removed all C structures > * added protocol description between frontend and backend drivers > > Changes since v3: > * fixed some typos > * renamed XENSND_PCM_FORMAT_FLOAT_** to XENSND_PCM_FORMAT_F32_** > * renamed XENSND_PCM_FORMAT_FLOAT64_** to XENSND_PCM_FORMAT_F64_** > * added 'id' field to the request and response packets > * renamed 'stream_id' to 'stream' in the packets description > * renamed 'pcm_data_rate' to 'pcm_rate' in the packets description > * renamed 'pcm_stream_type' to 'pcm_type' in the packets description > * removed 'stream_id' field from the response packets > > Changes since v4: > * renamed 'stream_id' back to the to 'stream' in the packets description > * moved 'id' field to the upper position in the response packets > > Changes since v5: > * Slightly reworked request/response packets > * Size of the request/response packet is changed to the 64 bytes > * Now parameters for the XENSND_OP_SET_VOLUME/XENSND_OP_GET_VOLUME are >passed via shared page > * Added parameters for the XenBus nodes (now each stream can be mapped >to the defined sound device in the backend using those parameters) > * Added XenBus state diagrams description > > Changes since v6: > * Reworked streams description in the Backend XenBus Nodes > > Changes since v7: > * re-worked backend device parameters to be more generic and flexible > * extended frontend device parameters > * slightly updated state machine description added mute/unmute commands > * added constants for XenStore configuration strings >(fields, PCM formats etc.) > * changed request/response structure size from 64 octets to 16 > * introduced dynamic buffer allocation instead of >static XENSND_MAX_PAGES_PER_REQUEST > * re-worked open request to allow dynamic buffer allocation > * re-worked read/write/volume requests, so they don't pass grefs: >buffer from the open request is used for these operations to pass data > * specified type of the volume value to be a signed value in steps >of 0.001 dBm, while 0 being 0dBm. > * added Linux include file with structure definitions > --- > --- > xen/include/public/io/sndif.h | 583 > > xen/include/public/io/sndif_linux.h | 114 +++ > 2 files changed, 697 insertions(+) > create mode 100644 xen/include/public/io/sndif.h > create mode 100644 xen/include/public/io/sndif_linux.h > > diff --git a/xen/include/public/io/sndif.h b/xen/include/public/io/sndif.h > new file mode 100644 > index 000..df705a6 > --- /dev/null > +++ b/xen/include/public/io/sndif.h > @@ -0,0 +1,583 @@ > +/** > + * sndif.h > + * > + * Unified sound-device I/O interface for Xen guest OSes. > + * > + * Permission is hereby granted, free of charge, to any person obtaining a > copy > + * of this software and associated documentation files (the "Software"), to > + * deal in the Software without restriction, including without limitation the > + * rights to use, copy, modify, merge, publish, distribute, sublicense, > and/or > + * sell copies of the Software, and to permit persons to whom the Software is > + * furnished to do so, subject to the following conditions: > + * > + * The above copyright notice and this permission notice shall be included in > + * all copies or substantial portions of the Software. > + * > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL > THE > + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER > + * DEALINGS IN THE SOFTWARE. > + * > + * Copyright (C) 2013-2015 GlobalLogic Inc. > + * Copyright (C) 2016 EPAM Systems Inc. > + */ > + > +#ifndef __XEN_PUBLIC_IO_XENSND_H__ > +#define __XEN_PUBLIC_IO_XENSND_H__ > + > +/* > + * Front->back notifications: When enqueuing a new request, sending a > + * notification can be made conditional on req_event (i.e., the generic > + * hold-off mechanism provided by the ring macros). Backends must set > + * req_event appropriately (e.g., using RING_FINAL_CHECK_FOR_REQUESTS()). > + * > + * Back->front notifications: When enqueuing a new response, sending a > + * notification can be made conditional on rsp_event (i.e., the generic > + * hold-off mechanism provided by the ring macros). Frontends must set > + *
Re: [Xen-devel] [PATCH v8] This is ABI for the two halves of a Para-virtual sound driver to communicate with each to other.
Hi, Konrad! First of all thank you for reviewing and providing valuable comments! I will put action items/TODOs at the very beginning, so we can see the summary. Also, please see answers inline. 1. Change frontend-id to frontend_id 2. Think about having a single sound card configured with a bunch of different devices/streams 3. State that sample rates and formats expressed as decimals w/o any particular ordering 4. Put description of migration/disconnection state 5. Change __attribute__((packed)) to __packed (scripts/checkpatch.pl) 6. Padding to fit cache line? What is its size (ARM/x86...) - need to discuss 7. Change GPL header to BSD 8. Remove #ifdef __KERNEL 9. Remove "Multiple PV drivers are allowed in the domain at the same time." 10. Support a single card as device/stream configuration allows fine tuning already 11. Explicitly say which indices in XenStore configuration are contiguous 12. For strings "If not defined then use frontend's default." add more description: "This default is depends on concrete PV frontend implementation" 13. Make names of cards/devices optional Thank you very much, Oleksandr On Fri, Nov 11, 2016 at 11:24 PM, Konrad Rzeszutek Wilk wrote: > On Fri, Nov 04, 2016 at 10:51:33PM +0200, Andrushchenko, Oleksandr wrote: >> Signed-off-by: Oleksandr Dmytryshyn >> Signed-off-by: Iurii Konovalenko >> Signed-off-by: Andrushchenko, Oleksandr >> Signed-off-by: Oleksandr Grytsov >> --- >> Changes since v1: >> * removed __attribute__((__packed__)) from all structures definitions >> >> Changes since v2: >> * removed all C structures >> * added protocol description between frontend and backend drivers >> >> Changes since v3: >> * fixed some typos >> * renamed XENSND_PCM_FORMAT_FLOAT_** to XENSND_PCM_FORMAT_F32_** >> * renamed XENSND_PCM_FORMAT_FLOAT64_** to XENSND_PCM_FORMAT_F64_** >> * added 'id' field to the request and response packets >> * renamed 'stream_id' to 'stream' in the packets description >> * renamed 'pcm_data_rate' to 'pcm_rate' in the packets description >> * renamed 'pcm_stream_type' to 'pcm_type' in the packets description >> * removed 'stream_id' field from the response packets >> >> Changes since v4: >> * renamed 'stream_id' back to the to 'stream' in the packets description >> * moved 'id' field to the upper position in the response packets >> >> Changes since v5: >> * Slightly reworked request/response packets >> * Size of the request/response packet is changed to the 64 bytes >> * Now parameters for the XENSND_OP_SET_VOLUME/XENSND_OP_GET_VOLUME are >>passed via shared page >> * Added parameters for the XenBus nodes (now each stream can be mapped >>to the defined sound device in the backend using those parameters) >> * Added XenBus state diagrams description >> >> Changes since v6: >> * Reworked streams description in the Backend XenBus Nodes >> >> Changes since v7: >> * re-worked backend device parameters to be more generic and flexible >> * extended frontend device parameters >> * slightly updated state machine description added mute/unmute commands >> * added constants for XenStore configuration strings >>(fields, PCM formats etc.) >> * changed request/response structure size from 64 octets to 16 >> * introduced dynamic buffer allocation instead of >>static XENSND_MAX_PAGES_PER_REQUEST >> * re-worked open request to allow dynamic buffer allocation >> * re-worked read/write/volume requests, so they don't pass grefs: >>buffer from the open request is used for these operations to pass data >> * specified type of the volume value to be a signed value in steps >>of 0.001 dBm, while 0 being 0dBm. >> * added Linux include file with structure definitions >> --- >> --- >> xen/include/public/io/sndif.h | 583 >> >> xen/include/public/io/sndif_linux.h | 114 +++ >> 2 files changed, 697 insertions(+) >> create mode 100644 xen/include/public/io/sndif.h >> create mode 100644 xen/include/public/io/sndif_linux.h >> >> diff --git a/xen/include/public/io/sndif.h b/xen/include/public/io/sndif.h >> new file mode 100644 >> index 000..df705a6 >> --- /dev/null >> +++ b/xen/include/public/io/sndif.h >> @@ -0,0 +1,583 @@ >> +/** >> + * sndif.h >> + * >> + * Unified sound-device I/O interface for Xen guest OSes. >> + * >> + * Permission is hereby granted, free of charge, to any person obtaining a >> copy >> + * of this software and associated documentation files (the "Software"), to >> + * deal in the Software without restriction, including without limitation >> the >> + * rights to use, copy, modify, merge, publish, distribute, sublicense, >> and/or >> + * sell copies of the Software, and to permit persons to whom the Software >> is >> + * furnished to do so, subject to the following conditions: >> + * >> + * The above copyright notice and this permission notice shall be included >> in >> + * all co
Re: [Xen-devel] [PATCH v8] This is ABI for the two halves of a Para-virtual sound driver to communicate with each to other.
>>> On 11.11.16 at 22:24, wrote: > On Fri, Nov 04, 2016 at 10:51:33PM +0200, Andrushchenko, Oleksandr wrote: >> + * Addressing >> - >> + * >> + * Indices used to address frontends, driver instances, cards, >> + * devices and streams. >> + * >> + * frontend-id >> + * Values: >> + * >> + * Domain ID of the sound frontend. >> + * >> + * drv_idx > > How come you use _ here but frontend-id has an '-'? Do you want to keep > them all the same? > >> + * Values: >> + * >> + * Zero based index of the virtualized sound driver instance in this >> domain. >> + * Multiple PV drivers are allowed in the domain at the same time. > > I think that is given. You can have multiple NICs for example. I woudl > remove that sentence. Is that a given? Multiple devices yes, but multiple drivers? Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v8] This is ABI for the two halves of a Para-virtual sound driver to communicate with each to other.
On Mon, Nov 14, 2016 at 03:39:34AM -0700, Jan Beulich wrote: > >>> On 11.11.16 at 22:24, wrote: > > On Fri, Nov 04, 2016 at 10:51:33PM +0200, Andrushchenko, Oleksandr wrote: > >> + * Addressing > >> - > >> + * > >> + * Indices used to address frontends, driver instances, cards, > >> + * devices and streams. > >> + * > >> + * frontend-id > >> + * Values: > >> + * > >> + * Domain ID of the sound frontend. > >> + * > >> + * drv_idx > > > > How come you use _ here but frontend-id has an '-'? Do you want to keep > > them all the same? > > > >> + * Values: > >> + * > >> + * Zero based index of the virtualized sound driver instance in this > >> domain. > >> + * Multiple PV drivers are allowed in the domain at the same time. > > > > I think that is given. You can have multiple NICs for example. I woudl > > remove that sentence. > > Is that a given? Multiple devices yes, but multiple drivers? Oh, good eye! I misread it as multiple PV devices, not multiple PV drivers of the same kind. That is interesting. > > Jan > ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v8] This is ABI for the two halves of a Para-virtual sound driver to communicate with each to other.
If these are all the comments then I'll start preparing patch v9 Thank you all for reviewing and providing feedback Oleksandr Andrushchenko ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v8] This is ABI for the two halves of a Para-virtual sound driver to communicate with each to other.
Sound is not my area of expertise but I have a few points that you may like to consider. 1. You can only say how many channels a stream has, not what the channels correspond to (e.g., "left", "right" for a 2 channel stereo stream; or "left-front", "rear-right", etc. for a 6 channel 5.1 surround sound stream). 2. Volume control uses absolute power levels (dBm). How is this supposed to map to the standard 0-100% volume controls that are most commonly presented to a user? 3. For the PCM formats you need to precisely specify the format or reference external specifications that do so. For example, "mpeg" is a bit vague as I'm sure this standard body has produced many different audio formats. The specifications don't need to be freely available, just correctly referenced. 4. 8 bits for stream index, operation and (particularly) pcm format feel a little limiting. 5. For the read/write operations is the stream buffer considered to be circular? I think it probably should be. 6. PCM_FORMAT_SPECIAL doesn't seem useful to me. New formats should be added via an update to this spec. David ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v8] This is ABI for the two halves of a Para-virtual sound driver to communicate with each to other.
On 16/11/16 13:12, Volodymyr Babchuk wrote: > Hello David, > > I helped Oleksandr with parts of this protocol, so I want to address > some questions you asked. > > On 16 November 2016 at 12:38, David Vrabel wrote: >> >> Sound is not my area of expertise but I have a few points that you may >> like to consider. >> >> 1. You can only say how many channels a stream has, not what the >> channels correspond to (e.g., "left", "right" for a 2 channel stereo >> stream; or "left-front", "rear-right", etc. for a 6 channel 5.1 surround >> sound stream). >> > Yes, we can specify mapping. But problem that is no one expects this > information. > There are predefined orders like left-right or FR-FL-RR-RL-FC-LFE on > chosen platform and all audio software just know and use them. Specifying a fixed ordering of channels is fine. > But now I'm thinking that it is a good idea to add predefined orders > to the protocol. > If target platform have a different order (e.g. backend running on > Windows), than backend should reorder data. > >> 2. Volume control uses absolute power levels (dBm). How is this supposed >> to map to the standard 0-100% volume controls that are most commonly >> presented to a user? > Aaaah, volumes. They are problematic. Different hardware use different > scales. They can be linear, logarithmic, they can have different > ranges, there can be integrated amplifiers, etc, etc. But good news > that audio subsystem always know how to remap those scales into dBm > scale. On this scale 0dBm is 100% and -inf dBm is 0%. If actual sound > hardware have internal gain, it will support volume above 0dBm (or > above 100%) and also will add distortion to sound, btw. There are > special formula that maps dBms to percens. This formula is non-linear. > Also, different sound systems can use different formulas. So it is > better not to stick to percents, because percents on Linux and on > Windows running on the same hardware will be different percents. Even > percents reported by ALSA and reported by PulseAudio are different. > We can't represent -inf as integer, but this is not needed, thanks to > logarithmic nature of dBm. Any sufficiently small value like -120dBm > will do the job and smallest possible value of -2147483,648dBm will be > indistinguishable from silence on any present or future hardware. This all sounds fine to me except you've got the units wrong and you really mean dB (not dBm where 0 dBm == 1 mW of output power which doesn't make a whole lot of sense in this context). >> 6. PCM_FORMAT_SPECIAL doesn't seem useful to me. New formats should be >> added via an update to this spec. > You can consider PCM_FORMAT_SPECIAL as raw format. I.e. "I know how to > prepare data for my device and my device expects audio in that > format". You don't need this on generic x86 system, > but it can be absolutely crucial on embedded system with hardware > accelerated audio/video playback for example. The frontend doesn't know what the underlying hardware is so how can it know what format SPECIAL means? Perhaps you could give an example of what one of these "special" formats looks like? I think it would be better to define a format number/name for each special format. David ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v8] This is ABI for the two halves of a Para-virtual sound driver to communicate with each to other.
Hello David, I helped Oleksandr with parts of this protocol, so I want to address some questions you asked. On 16 November 2016 at 12:38, David Vrabel wrote: > > Sound is not my area of expertise but I have a few points that you may > like to consider. > > 1. You can only say how many channels a stream has, not what the > channels correspond to (e.g., "left", "right" for a 2 channel stereo > stream; or "left-front", "rear-right", etc. for a 6 channel 5.1 surround > sound stream). > Yes, we can specify mapping. But problem that is no one expects this information. There are predefined orders like left-right or FR-FL-RR-RL-FC-LFE on chosen platform and all audio software just know and use them. But now I'm thinking that it is a good idea to add predefined orders to the protocol. If target platform have a different order (e.g. backend running on Windows), than backend should reorder data. > 2. Volume control uses absolute power levels (dBm). How is this supposed > to map to the standard 0-100% volume controls that are most commonly > presented to a user? Aaaah, volumes. They are problematic. Different hardware use different scales. They can be linear, logarithmic, they can have different ranges, there can be integrated amplifiers, etc, etc. But good news that audio subsystem always know how to remap those scales into dBm scale. On this scale 0dBm is 100% and -inf dBm is 0%. If actual sound hardware have internal gain, it will support volume above 0dBm (or above 100%) and also will add distortion to sound, btw. There are special formula that maps dBms to percens. This formula is non-linear. Also, different sound systems can use different formulas. So it is better not to stick to percents, because percents on Linux and on Windows running on the same hardware will be different percents. Even percents reported by ALSA and reported by PulseAudio are different. We can't represent -inf as integer, but this is not needed, thanks to logarithmic nature of dBm. Any sufficiently small value like -120dBm will do the job and smallest possible value of -2147483,648dBm will be indistinguishable from silence on any present or future hardware. > 3. For the PCM formats you need to precisely specify the format or > reference external specifications that do so. For example, "mpeg" is a > bit vague as I'm sure this standard body has produced many different > audio formats. The specifications don't need to be freely available, > just correctly referenced. > > 4. 8 bits for stream index, operation and (particularly) pcm format feel > a little limiting. > > 5. For the read/write operations is the stream buffer considered to be > circular? I think it probably should be. > > 6. PCM_FORMAT_SPECIAL doesn't seem useful to me. New formats should be > added via an update to this spec. You can consider PCM_FORMAT_SPECIAL as raw format. I.e. "I know how to prepare data for my device and my device expects audio in that format". You don't need this on generic x86 system, but it can be absolutely crucial on embedded system with hardware accelerated audio/video playback for example. -- WBR Volodymyr Babchuk aka lorc [+380976646013] mailto: vlad.babc...@gmail.com ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v8] This is ABI for the two halves of a Para-virtual sound driver to communicate with each to other.
On 16 November 2016 at 15:31, David Vrabel wrote: > On 16/11/16 13:12, Volodymyr Babchuk wrote: >> Hello David, >> >> I helped Oleksandr with parts of this protocol, so I want to address >> some questions you asked. >> >> On 16 November 2016 at 12:38, David Vrabel wrote: >>> >>> Sound is not my area of expertise but I have a few points that you may >>> like to consider. >>> >>> 1. You can only say how many channels a stream has, not what the >>> channels correspond to (e.g., "left", "right" for a 2 channel stereo >>> stream; or "left-front", "rear-right", etc. for a 6 channel 5.1 surround >>> sound stream). >>> >> Yes, we can specify mapping. But problem that is no one expects this >> information. >> There are predefined orders like left-right or FR-FL-RR-RL-FC-LFE on >> chosen platform and all audio software just know and use them. > > Specifying a fixed ordering of channels is fine. > >> But now I'm thinking that it is a good idea to add predefined orders >> to the protocol. >> If target platform have a different order (e.g. backend running on >> Windows), than backend should reorder data. >> >>> 2. Volume control uses absolute power levels (dBm). How is this supposed >>> to map to the standard 0-100% volume controls that are most commonly >>> presented to a user? >> Aaaah, volumes. They are problematic. Different hardware use different >> scales. They can be linear, logarithmic, they can have different >> ranges, there can be integrated amplifiers, etc, etc. But good news >> that audio subsystem always know how to remap those scales into dBm >> scale. On this scale 0dBm is 100% and -inf dBm is 0%. If actual sound >> hardware have internal gain, it will support volume above 0dBm (or >> above 100%) and also will add distortion to sound, btw. There are >> special formula that maps dBms to percens. This formula is non-linear. >> Also, different sound systems can use different formulas. So it is >> better not to stick to percents, because percents on Linux and on >> Windows running on the same hardware will be different percents. Even >> percents reported by ALSA and reported by PulseAudio are different. >> We can't represent -inf as integer, but this is not needed, thanks to >> logarithmic nature of dBm. Any sufficiently small value like -120dBm >> will do the job and smallest possible value of -2147483,648dBm will be >> indistinguishable from silence on any present or future hardware. > > This all sounds fine to me except you've got the units wrong and you > really mean dB (not dBm where 0 dBm == 1 mW of output power which > doesn't make a whole lot of sense in this context). Oops. Yes, you are right. Have no idea where dBm came from. >>> 6. PCM_FORMAT_SPECIAL doesn't seem useful to me. New formats should be >>> added via an update to this spec. >> You can consider PCM_FORMAT_SPECIAL as raw format. I.e. "I know how to >> prepare data for my device and my device expects audio in that >> format". You don't need this on generic x86 system, >> but it can be absolutely crucial on embedded system with hardware >> accelerated audio/video playback for example. > > The frontend doesn't know what the underlying hardware is so how can it > know what format SPECIAL means? In general case yes, there are no way to know something about real hardware. But we tried to make protocol more generic. Imagine embedded system, where guests can know about hardware and can exploit this knowledge to improve performance. But I think you are right. Better to remove SPECIAL format and then extend protocol locally if there will arise need for this. > Perhaps you could give an example of what one of these "special" formats > looks like? It can be anything. If we have an audio decoding accelerator, we can dump mp3 or ac3 data right into sound card for example. > I think it would be better to define a format number/name for each > special format. Yes. Oleksadr, what is your opinion on this? -- WBR Volodymyr Babchuk aka lorc [+380976646013] mailto: vlad.babc...@gmail.com ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v8] This is ABI for the two halves of a Para-virtual sound driver to communicate with each to other.
Thank you all for the review and comments! I guess only these are not addressed (or still need comments): > 1. You can only say how many channels a stream has, not what the > channels correspond to (e.g., "left", "right" for a 2 channel stereo > stream; or "left-front", "rear-right", etc. for a 6 channel 5.1 surround > sound stream). PV driver may be configured to have number of channels which are not supported by the back's HW. In that case this is up to back how to treat those. If we introduce some specific ordering or mapping that will only be a recommendation to the back, which it may follow. Thus, I would stick to what we have now > 3. For the PCM formats you need to precisely specify the format or > reference external specifications that do so. For example, "mpeg" is a > bit vague as I'm sure this standard body has produced many different > audio formats. The specifications don't need to be freely available, > just correctly referenced. I do understand the intent here, but I blieve we cannot make the protocol being simple and fit all at once. So, I would suggest leaving it as is at least for now or removing it. > 4. 8 bits for stream index, operation and (particularly) pcm format feel > a little limiting. 8 bits mean you can have 256 streams, operations and formats (which are indices). I would prefer to stay with 8 bits > 5. For the read/write operations is the stream buffer considered to be > circular? I think it probably should be. I guess it shouldn't. The reason of having offset is the fact that software on front side may mmap part of the buffer and tell the driver the offset of the "dirty" region of the buffer. Please find updated list of TODOs for the next version: 1. Change frontend-id to frontend_id 2. Have a single sound card configured with bunch of different devices/streams 3. State that sample rates and formats expressed as decimals w/o any particular ordering 4. Put description of migration/disconnection state 5. Change __attribute__((packed)) to __packed (scripts/checkpatch.pl) 6. Padding to fit cache line? What is its size (ARM/x86...) - need to discuss 7. Change GPL header to BSD 8. Remove #ifdef __KERNEL 9. Remove "Multiple PV drivers are allowed in the domain at the same time." 10. Support a single card as device/stream configuration allows fine tuning already 11. Explicitly say which indices in XenStore configuration are contiguous 12. For strings "If not defined then use frontend's default." add more description: "This default is depends on concrete PV frontend implementation" 13. Make names of cards/devices optional 14. Remove PCM_FORMAT_SPECIAL 15. Change volume units from dBm to dB Thank you, Oleksandr ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
