|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] 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.
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? Thank you, Oleksandr On 01/24/2017 09:13 PM, Konrad Rzeszutek Wilk wrote: On Mon, Dec 05, 2016 at 03:05:29PM +0200, Oleksandr Andrushchenko wrote:From: Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>Usually one also puts somethign in the commit description. When I applied this to me tree I got: [konrad@char xen]$ git log --oneline HEAD^.. ecc7711 This is the ABI for the two halves of a para-virtualized sound driver to communicate with each to other. [konrad@char xen]$ fixed Also, thank you for the including the pahole output! np
no reason See vscsiif.h and console and vkbd. Actually the last ones don't mention it in the header but the libxl constructs these. Indeed, I will remove this section + * Values: <uint>uint32_t+ * + * Domain ID of the sound frontend. + * + * drv_idxAgain, can this be - ?+ * Values: <uint>uint32_t+ * + * Zero based contiguous index of the virtualized sound driver instance in + * this domain. Multiple PV drivers are allowed in the domain + * at the same time.This is not needed. We already have this information in the XenStore directory: /local/domain/0/backend/vsnd/2/0/frontend-id = 2 /local/domain/0/backend/vsnd/2/1/frontend-id = 2 .. /local/domain/0/backend/vsnd/2/N/frontend-id = 2 (which would be similar to what 'vbd', 'console' and 'vif' do). Removed + * + * dev_ids/_/-/ done + * Values: <uint>uint_32t+ * + * Unique device ID. + * Doesn't have to be zero based and/or to be contiguous.Is uint enough? Or would it be better to have an <string> if this is some form of SHA-1 value? changed to uint8_t + * + * stream_idxs/_/-/ done + * Values: <uint>uint_32t [edit, based on the struct xensnd_req this has to be uint_8t] ? changed to uint8_t + * + * Zero based contiguous index of the stream of the device. + * + * 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. So, from readability POV I would still have "device" in place From xenstore documentation: "Data should generally be human-readable for ease of management and debugging " I assume this also applies to the structure as well Could not this be: /local/domain/5/device/vsnd/0/2/stream/0/type = "p" ? then one has to know that "2" stands for device. see above, I would keep "device" here + * + *------------------------------- PCM settings -------------------------------- + * + * Every virtualized sound frontend has set of devices and streams, eachfrontend or backend? I would think backend since this is still the backend section? you are right, moved to frontend's section + * is individually configured. Part of the PCM configuration can be defined at + * higher level and be fully or partially re-used by the underlying layers. + * These configuration values are: + * o number of channels (min/max) + * o supported sample rates + * o supported sample formats. + * E.g. one can define these values for the whole driver, device or stream. + * Every underlying layer in turn can re-define some or all of them to better + * fit its needs. For example, driver may define number of channels to be + * in [1; 8] range, and some particular stream may be limited to [1; 2] only. + * The rule is that the underlying layer must be a subset of the upper layer + * range.OK, so these are more of 'global ones'? kind of, what is more they can be "global" to all the underlying levels, e.g. if device changes some of the parameters they become adopted by streams if not explicitly configured. The same is true if device changes parameters set at the card level. This way one can easily tailor particular pieces of the system w/o writing all settings again + * + * Note: if any of the values are not defined then PV driver should use + * its default values instead.How is that suppose to work? What if the PV driver by default wants to use say 314156 channels and the backend does not expose any? Can it use that? I presume it could try.. and it would fail but that hardly seems nice. I will probably remove this sentence and the below at all. If not defined this could mean an error state + * + * channels-min + * Values: <uint>uint32_t+ * + * The minimum amount of channels that is supported. + * Must be at least 1. If not defined then use frontend's default.This is odd. This whole section is for the backend, so how can the backend not have this defined? Do you mean to say that this parameter is optional? Perhaps then say right before 'The minimum' include '(optional)' Anyhow where do you envision this optional global value to be? /local/domain/0/backend/vsnd/5/channels-[min|max] ? Perhaps mention that in the start of this section: These global values are under the backend 'vsnd' directory, as so: /local/domain/0/backends/vsnd/<front-id>/channels-[min|max] Thought I think there is probably a better way to say this.. well, the confusion comes from the fact that I have described all this under backend's section now when I move it to the frontend's section it should make more sense + * + * channels-max + * Values: <uint>uint32_t uint8_t + * + * The maximum amount of channels that is supported. + * Must be at least <channels-min>. If not defined then use frontend's + * default.I am still having trouble understanding how the backend is suppose to use the frontend's default. Perhaps you want to say: If this value is not exposed by the backend the frontend is permitted to use its default values. ? moved to frontend's section + * + * sample-rates + * Values: <list of uints>.. of uint32_t done + * + * List of supported sample rates separated by XENSND_LIST_SEPARATOR. + * If not defined then use frontend's default. Sample rates are expressedAhain this 'If not defined then use fronten'ds default' could be written a bit differently. moved to frontend's section + * as a list of decimal values w/o any ordering requirement. + * + * sample-formats + * Values: <list of XENSND_PCM_FORMAT_XXX_STR> + * + * List of supported sample formats separated by XENSND_LIST_SEPARATOR. + * If not defined then use frontend's default.Again, this does not sound right. The backend has no clue what the frontend defaults are. moved to frontend's section + * + * buffer-size + * Values: <uint>uint32_t done + * + * The maximum size in octets of the buffer to allocate per stream. + * + * Example configuration: + * + * Driver configuration used by all streams: + * /local/domain/5/device/vsnd/0/sample-formats = "s8;u8;s16_le;s16_be"Huh? But this is a backend value? Don't you mean: /local/domain/0/backend/vsnd/5/sample-formats = "s8;u8;s16_le;s16_be" ? moved to frontend's section + * Stream overrides sample rates supported:-EPARSE done + * /local/domain/5/device/vsnd/0/device/2/stream/0/sample-rates = + * "8000;22050;44100;48000"/local/domain/0/backend/vsnd/5/2/stream/0/sample-rates ="8000;22050;44100;48000" But that is wrong as this section is about global values so they would be more like: /local/domain/0/backend/vsnd/5/sample-rates ="8000;22050;44100;48000" ? not anymore, as I move these to frontend's configuration + * + *----------------------- Virtual sound card settings -------------------------- + * short-name + * Values: <char[32]> + * + * Short name of the virtual sound card. Optional. + * + * long-name + * Values: <char[80]> + * + * Long name of the virtual sound card. Optional. + * + * For example, + * /local/domain/5/device/vsnd/0/short-name = "Virtual audio" + * /local/domain/5/device/vsnd/0/long-name = + * "Virtual audio at center stack"This should be inverted. That is this is on the backend side: /local/domain/0/backend/vsnd/5/0/2/short-name = "Virtual audio" And so on. Also how come your example is missing the device part? I will put a complete configuration example Wait, why do you have an an device below but ... not here? Why not group them together? That is why not have 'short-name' ,'long-name' and 'name' in the same directory? name is for a stream; short/long for the card + * + *----------------------------- Device settings -------------------------------- + * name + * Values: <char[80]> + * + * Name of the sound device within the virtual sound card. Optional. + * + * For example, + * /local/domain/5/device/vsnd/0/device/0/name = "General analog"The example you used at start had a device value of 2. It may be better to use that here, so : /local/domain/0/backend/vsnd/5/2/name = "General analog" done + * + *----------------------------- Stream settings ------------------------------- + * + * type + * Values: "p", "c" + * + * Stream type: "p" - playback stream, "c" - capture stream + * + * If both capture and playback are needed then two streams need to be + * defined under the same device. For example, + * /local/domain/5/device/vsnd/0/device/0/stream/0/type = "p" + * /local/domain/5/device/vsnd/0/device/0/stream/1/type = "c"OK, but again: /local/domain/0/backend/vsnd/5/2/0/stream/0/type = "p" /local/domain/0/backend/vsnd/5/2/0/stream/1/type = "c" Do you want also an 'feature-version' to identify what version the backend exposes? will add that + * + ***************************************************************************** + * Frontend XenBus Nodes + ***************************************************************************** + * + *----------------------- Request Transport Parameters ----------------------- + * + * These are per stream... which means they are also per device right? So one can have something like this: /local/domain/5/device/vsnd/2/stream/0/event-channel /local/domain/5/device/vsnd/0/stream/0/event-channel /local/domain/5/device/vsnd/2/stream/1/event-channel /local/domain/5/device/vsnd/0/stream/0/event-channel yes, every stream has its own communication channel + * + * event-channel + * Values: <uint>uint32_t done + * + * The identifier of the Xen event channel used to signal activity + * in the ring buffer. + * + * ring-ref + * Values: <uint>uint32_t done + * + * The Xen grant reference granting permission for the backend to map + * a sole page in a single page sized ring buffer. + * + * indexWhy not 'unique-id' ? makes sense, will change + * Values: <uint>uint32_t done + * + * After stream initialization it is assigned a unique ID (within the front + * driver), so every stream of the frontend can be identified by the + * backend by this ID. This is not equal to stream_idx as the later iss/stream_idx/steam-idx/ done + * zero based within a device, but this index is contiguous within thecontingous done + * driver.+ */ + +/* + * STATE DIAGRAMS + * + ***************************************************************************** + * Startup * + ***************************************************************************** + * + * Tool stack creates front and back state nodes with initial state + * XenbusStateInitialising. + * Tool stack creates and sets up frontend sound configuration nodes per domain. + * + * Front Back + * ================================= ===================================== + * XenbusStateInitialising XenbusStateInitialising + * o Query backend device identification + * data. + * o Open and validate backend device. + * | + * | + * V + * XenbusStateInitWait + * + * o Query frontend configuration + * o Allocate and initialize + * event channels per configured + * playback/capture stream. + * o Publish transport parameters + * that will be in effect during + * this connection. + * | + * | + * V + * XenbusStateInitialised + * + * o Query frontend transport parameters. + * o Connect to the event channels. + * | + * | + * V + * XenbusStateConnected + * + * o Create and initialize OS + * virtual sound device instances + * as per configuration. + * | + * | + * V + * XenbusStateConnected + * + * XenbusStateUnknown + * XenbusStateClosed + * XenbusStateClosing + * o Remove virtual sound device + * o Remove event channels + * | + * | + * V + * XenbusStateClosed + * + */ + +/* + * PCM FORMATS + * + * XENSND_PCM_FORMAT_<format>[_<endian>] + * + * format: <S/U/F><bits> or <name> + * S - signed, U - unsigned, F - float + * bits - 8, 16, 24, 32 + * name - MU_LAW, GSM, etc. + * + * endian: <LE/BE>, may be absent + * LE - Little endian, BE - Big endian + */ +#define XENSND_PCM_FORMAT_S8 0 +#define XENSND_PCM_FORMAT_U8 1 +#define XENSND_PCM_FORMAT_S16_LE 2 +#define XENSND_PCM_FORMAT_S16_BE 3 +#define XENSND_PCM_FORMAT_U16_LE 4 +#define XENSND_PCM_FORMAT_U16_BE 5 +#define XENSND_PCM_FORMAT_S24_LE 6 +#define XENSND_PCM_FORMAT_S24_BE 7 +#define XENSND_PCM_FORMAT_U24_LE 8 +#define XENSND_PCM_FORMAT_U24_BE 9 +#define XENSND_PCM_FORMAT_S32_LE 10 +#define XENSND_PCM_FORMAT_S32_BE 11 +#define XENSND_PCM_FORMAT_U32_LE 12 +#define XENSND_PCM_FORMAT_U32_BE 13 +#define XENSND_PCM_FORMAT_F32_LE 14 /* 4-byte float, IEEE-754 32-bit, */ +#define XENSND_PCM_FORMAT_F32_BE 15 /* range -1.0 to 1.0 */ +#define XENSND_PCM_FORMAT_F64_LE 16 /* 8-byte float, IEEE-754 64-bit, */ +#define XENSND_PCM_FORMAT_F64_BE 17 /* range -1.0 to 1.0 */ +#define XENSND_PCM_FORMAT_IEC958_SUBFRAME_LE 18 +#define XENSND_PCM_FORMAT_IEC958_SUBFRAME_BE 19 +#define XENSND_PCM_FORMAT_MU_LAW 20 +#define XENSND_PCM_FORMAT_A_LAW 21 +#define XENSND_PCM_FORMAT_IMA_ADPCM 22 +#define XENSND_PCM_FORMAT_MPEG 23 +#define XENSND_PCM_FORMAT_GSM 24 + +/* + * REQUEST CODES. + */ +#define XENSND_OP_OPEN 0 +#define XENSND_OP_CLOSE 1 +#define XENSND_OP_READ 2 +#define XENSND_OP_WRITE 3 +#define XENSND_OP_SET_VOLUME 4 +#define XENSND_OP_GET_VOLUME 5 +#define XENSND_OP_MUTE 6 +#define XENSND_OP_UNMUTE 7 + +/* + * XENSTORE FIELD AND PATH NAME STRINGS, HELPERS. + */ +#define XENSND_DRIVER_NAME "vsnd" + +#define XENSND_LIST_SEPARATOR ";" +/* Path entries */ +#define XENSND_PATH_DEVICE "device" +#define XENSND_PATH_STREAM "stream" +/* Field names */ +#define XENSND_FIELD_VCARD_SHORT_NAME "short-name" +#define XENSND_FIELD_VCARD_LONG_NAME "long-name" +#define XENSND_FIELD_RING_REF "ring-ref" +#define XENSND_FIELD_EVT_CHNL "event-channel" +#define XENSND_FIELD_DEVICE_NAME "name" +#define XENSND_FIELD_TYPE "type" +#define XENSND_FIELD_STREAM_INDEX "index" +#define XENSND_FIELD_CHANNELS_MIN "channels-min" +#define XENSND_FIELD_CHANNELS_MAX "channels-max" +#define XENSND_FIELD_SAMPLE_RATES "sample-rates" +#define XENSND_FIELD_SAMPLE_FORMATS "sample-formats" +#define XENSND_FIELD_BUFFER_SIZE "buffer-size" + +/* Stream type field values. */ +#define XENSND_STREAM_TYPE_PLAYBACK "p" +#define XENSND_STREAM_TYPE_CAPTURE "c" +/* Sample rate max string length */ +#define XENSND_SAMPLE_RATE_MAX_LEN 6Hm, but it is uint32_t which can be much much higher than 6. 4294967295 11 characters? fixed +/* Sample format field values */ +#define XENSND_SAMPLE_FORMAT_MAX_LEN 24You sure? You may want to make that clear in 'sample-format' section that the maximum of a string can be 24 characters. done And also explain why 24 characters. no particular reason, just to fit XENSND_PCM_FORMAT_???_STR do you want me to change it to something else? + +#define XENSND_PCM_FORMAT_S8_STR "s8" +#define XENSND_PCM_FORMAT_U8_STR "u8" +#define XENSND_PCM_FORMAT_S16_LE_STR "s16_le" +#define XENSND_PCM_FORMAT_S16_BE_STR "s16_be" +#define XENSND_PCM_FORMAT_U16_LE_STR "u16_le" +#define XENSND_PCM_FORMAT_U16_BE_STR "u16_be" +#define XENSND_PCM_FORMAT_S24_LE_STR "s24_le" +#define XENSND_PCM_FORMAT_S24_BE_STR "s24_be" +#define XENSND_PCM_FORMAT_U24_LE_STR "u24_le" +#define XENSND_PCM_FORMAT_U24_BE_STR "u24_be" +#define XENSND_PCM_FORMAT_S32_LE_STR "s32_le" +#define XENSND_PCM_FORMAT_S32_BE_STR "s32_be" +#define XENSND_PCM_FORMAT_U32_LE_STR "u32_le" +#define XENSND_PCM_FORMAT_U32_BE_STR "u32_be" +#define XENSND_PCM_FORMAT_F32_LE_STR "float_le" +#define XENSND_PCM_FORMAT_F32_BE_STR "float_be" +#define XENSND_PCM_FORMAT_F64_LE_STR "float64_le" +#define XENSND_PCM_FORMAT_F64_BE_STR "float64_be" +#define XENSND_PCM_FORMAT_IEC958_SUBFRAME_LE_STR "iec958_subframe_le" +#define XENSND_PCM_FORMAT_IEC958_SUBFRAME_BE_STR "iec958_subframe_be" +#define XENSND_PCM_FORMAT_MU_LAW_STR "mu_law" +#define XENSND_PCM_FORMAT_A_LAW_STR "a_law" +#define XENSND_PCM_FORMAT_IMA_ADPCM_STR "ima_adpcm" +#define XENSND_PCM_FORMAT_MPEG_STR "mpeg" +#define XENSND_PCM_FORMAT_GSM_STR "gsm" + +/* + * STATUS RETURN CODES. + */ +/* Operation not supported. */ +#define XENSND_RSP_NOTSUPP (-2) +/* Operation failed for some unspecified reason (e. g. -EIO). */Is that the -EIO going to be exposed somewhere? no Inside of the 'struct xensnd_resp' ? no, XENSND_RSP_??? is used there Would it make sense to have it exposed there? Is that what 'status' is for? see above, XENSND_RSP_??? Why not have an 'err' and mandate that it use POSIX return values? well, if we agree on POSIX values then I am more than ok +#define XENSND_RSP_ERROR (-1) +/* Operation completed successfully. */ +#define XENSND_RSP_OKAY 0 + +/* + * Assumptions: + * o usage of grant reference 0 as invalid grant reference: + * grant reference 0 is valid, but never exposed to a PV driver, + * because of the fact it is already in use/reserved by the PV console. + * o all references in this document to page sizes must be treated + * as pages of size XEN_PAGE_SIZE unless otherwise noted. fixed + * + * Description of the protocol between frontend and backend driver.Is this suppose to have an: ----------------------------------------- underneath it? done + * + * The two halves of a Para-virtual sound driver communicate with + * each other using a shared page and an event channel.shared pages and event channels? Since it looks like you can have /local/domain/<frontend-id>/device/vsnd/<device-id>/stream/<stream-id>/event-channel And you can have N device-id and M stream-id ? right, done + * Shared page contains a ring with request/response packets. + * + * All reserved fields in the structures below must be 0. + * + * All request packets have the same length (32 octets)Which implies you can have at maximum 64 requests? [64 bytes for the four RING_IDX along with the 48 of padding, that means 4032 left, but since we need this to be modulo 2 the best we can do is 2^6. you mean we have to pad the structures so they are all 64 bytes long? + * All request packets have common header: + * 0 1 2 3 octet + * +-----------------+-----------------+-----------------+-----------------+ + * | id | operation | stream_idx |Why the stream_idx ? Your 'ring-ref' is rooted from the '<stream-idx>' so the frontend and backend already know this. indeed, we can probably remove this from all the structures
it describes common header and its values + * + * + * Request open - open a PCM stream for playback or capture: + * 0 1 2 3 octet + * +-----------------+-----------------+-----------------+-----------------+ + * | id | XENSND_OP_OPEN | stream_idx | + * +-----------------+-----------------+-----------------+-----------------+ + * | reserved | + * +-----------------+-----------------+-----------------+-----------------+ + * | pcm_rate | + * +-----------------+-----------------+-----------------+-----------------+ + * | pcm_format | pcm_channels | reserved | + * +-----------------+-----------------+-----------------+-----------------+ + * | buffer_sz | + * +-----------------+-----------------+-----------------+-----------------+ + * | gref_directory_start |Perhaps 'gref_list' ? not sure, it is the start of the page directory, its gref + * +-----------------+-----------------+-----------------+-----------------+ + * | reserved | + * +-----------------+-----------------+-----------------+-----------------+ + * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/| + * +-----------------+-----------------+-----------------+-----------------+ + * | reserved | + * +-----------------+-----------------+-----------------+-----------------++ * + * pcm_rate - uint32_t, stream data rate, Hz + * pcm_format - uint8_t, XENSND_PCM_FORMAT_XXX value + * pcm_channels - uint8_t, number of channels of this stream.. And I presume this should be between 'channels-min' and 'channels-max'? Is it OK to have to say 255 ? What are the boundary values? yes, it is ok to be uint8_t, IMO. I will change the xenstore values description as well there was a discussion on that: https://marc.info/?l=xen-devel&m=148008589320533&w=2+ * buffer_sz - uint32_t, buffer size to be allocated in octetsIs there an maximum? Or a minimum? Why have it in bytes? Why not in the amount of grants you need? Perhaps call it 'gref_nr' ? That way your computation on how many grants you need is well simplified. + * gref_directory_start - grant_ref_t, a reference to the first shared pages/gref_directory_start/gref_list/ ? not sure + * describing shared buffer references. At least one page exists. If shared + * buffer size exceeds what can be addressed by this single page, thens/shared buffer size/buffer_sz/ ? why, I am not using variable/field name here, but explaining I can put the name in brackets) + * reference to the next page must be supplied (see gref_dir_next_page below) yes, you got it right. the use-cases we have do use buffers bigger then 4K, so this is why we never thought of such an overhead
it will be yet another flame here... will keep as is + uint32_t pcm_rate; /* in Hz */ + uint8_t pcm_format; + uint8_t pcm_channels; + uint16_t reserved; + uint32_t buffer_sz; + grant_ref_t gref_directory_start; +}; + +/* + * Shared page for XENSND_OP_OPEN buffer descriptor (gref_directory in the + * request) employs a list of pages, describing all pages of the shared data + * buffer: + * 0 1 2 3 octet + * +-----------------+-----------------+-----------------+-----------------+ + * | gref_dir_next_page | + * +-----------------+-----------------+-----------------+-----------------+ + * | gref[0] | + * +-----------------+-----------------+-----------------+-----------------+ + * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/| + * +-----------------+-----------------+-----------------+-----------------+ + * | gref[i] | + * +-----------------+-----------------+-----------------+-----------------+ + * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/| + * +-----------------+-----------------+-----------------+-----------------+ + * | gref[N -1] | + * +-----------------+-----------------+-----------------+-----------------+ + * + * gref_dir_next_page - grant_ref_t, reference to the next page describing + * page directory. Must be 0 if no more pages in the list.s/no more page/there are no more/ done + * gref[i] - grant_ref_t, reference to a shared page of the buffer + * allocated at XENSND_OP_OPEN + * + * Number of grant_ref_t entries in the whole page directory is not + * passed, but instead can be calculated as: + * num_grefs_total = (XENSND_OP_OPEN.buffer_sz + XEN_PAGE_SIZE - 1) / + * XEN_PAGE_SIZEAnd what are the expected errors? Woudl it make sense to define those? Say: Returns:-ENOBUFS: Cannot map that many buffers.-EINVAL: Incorrect values in the requst? ATM, the XENSND_RSP_??? are returned - not sure we need this: normally you have to open something before you can use it, e.g. a file + * 0 1 2 3 octet + * +-----------------+-----------------+-----------------+-----------------+ + * | id | operation | stream_idx | + * +-----------------+-----------------+-----------------+-----------------+ + * | reserved | + * +-----------------+-----------------+-----------------+-----------------+ + * | offset | + * +-----------------+-----------------+-----------------+-----------------+ + * | length | + * +-----------------+-----------------+-----------------+-----------------+ + * | reserved | + * +-----------------+-----------------+-----------------+-----------------+ + * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/| + * +-----------------+-----------------+-----------------+-----------------+ + * | reserved | + * +-----------------+-----------------+-----------------+-----------------+ + * + * operation - XENSND_OP_READ for read or XENSND_OP_WRITE for write + * offset - uint32_t, read or write data offset within the shared buffer + * passed with XENSND_OP_OPEN requestMay also want to say that this offset must be less than buffer_sz. In octets? May want to say that explicitly. done + * length - uint32_t, read or write data lengthIn octets? done
I think this is implementation specific and shouldn't be in a generic protocol 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 + * + * 0 1 2 3 octet + * +-----------------+-----------------+-----------------+-----------------+ + * | channel[0] | + * +-----------------+-----------------+-----------------+-----------------+ + * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/| + * +-----------------+-----------------+-----------------+-----------------+ + * | channel[i] | + * +-----------------+-----------------+-----------------+-----------------+ + * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/| + * +-----------------+-----------------+-----------------+-----------------+ + * | channel[XENSND_OP_OPEN.pcm_channels - 1] | + * +-----------------+-----------------+-----------------+-----------------+ + * + * channel[i] - sint32_t, volume of i-th channelYou may want to say that i is uint8_t. done + * Volume is expressed as a signed value in steps of 0.001 dB, + * while 0 being 0 dB. + */ + +/* + * Request mute/unmute - mute/unmute stream: + * 0 1 2 3 octet + * +-----------------+-----------------+-----------------+-----------------+ + * | id | operation | stream_idx | + * +-----------------+-----------------+-----------------+-----------------+ + * | reserved | + * +-----------------+-----------------+-----------------+-----------------+ + * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/| + * +-----------------+-----------------+-----------------+-----------------+ + * | reserved | + * +-----------------+-----------------+-----------------+-----------------+ + * + * operation - XENSND_OP_MUTE for mute or XENSND_OP_UNMUTE for unmute + * Buffer passed with XENSND_OP_OPEN is used to exchange mute/unmute + * values:Again, same comments as above.+ * + * 0 octet + * +-----------------------------------------------------------------------+ + * | channel[0] | + * +-----------------------------------------------------------------------+ + * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/| + * +-----------------------------------------------------------------------+ + * | channel[i] | + * +-----------------------------------------------------------------------+ + * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/| + * +-----------------------------------------------------------------------+ + * | channel[XENSND_OP_OPEN.pcm_channels - 1] | + * +-----------------+-----------------+-----------------+-----------------+ + * + * channel[i] - uint8_t, non-zero if i-th channel needs to be muted/unmuted + */ + +/* + * All response packets have the same length (32 octets) + * + * Response for all requests: + * 0 1 2 3 octet + * +-----------------+-----------------+-----------------+-----------------+ + * | id | operation | stream_idx | + * +-----------------+-----------------+-----------------+-----------------+ + * | status | reserved | + * +-----------------+-----------------+-----------------+-----------------+ + * | reserved | + * +-----------------+-----------------+-----------------+-----------------+ + * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/| + * +-----------------+-----------------+-----------------+-----------------+ + * | reserved | + * +-----------------+-----------------+-----------------+-----------------+ + * + * id - uint16_t, copied from the request + * stream_idx - uint8_t, copied from request + * operation - uint8_t, XENSND_OP_* - copied from request + * status - int8_t, response status (XENSND_RSP_*)Could this become an 'err' and the POSIX values used for errors? we can discuss this, I am fine with this approach if the Community accepts this
_______________________________________________ Embedded-pv-devel mailing list Embedded-pv-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/cgi-bin/mailman/listinfo/embedded-pv-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |