[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



On 11/25/2016 02:54 PM, Jan Beulich wrote:
On 25.11.16 at 12:57, <andr2000@xxxxxxxxx> wrote:
+ * Request open - open a PCM stream for playback or capture:
+ *          0                 1                  2                3        
octet
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                 id                |    operation    |     stream_idx  |
+ * +-----------------+-----------------+-----------------+-----------------+
I think individual requests would better spell out their values, instead
of all saying "operation".
done, but for those operations like read/write I'll still have operation field
in the structure and have description like:
* operation is equal to XENSND_OP_READ for read or XENSND_OP_WRITE for write
+ * id - uint16_t, private guest value, echoed in response
+ * operation - uint8_t, XENSND_OP_OPEN
+ * stream_idx - uint8_t, index of the stream ("streams_idx" XenStore entry
+ *   of the stream)
Please explain the common header fields just once, instead of for
each of the commands.
done
+ * gref_directory_start - grant_ref_t, a reference to the first shared page
+ *   describing shared buffer references. At least one page exists. If shared
+ *   buffer size exceeds what can be addressed by this single page, then
+ *   reference to the next page must be supplied (gref_dir_next_page below
+ *   is not NULL)
NULL is a pointer value. You mean zero (or one of its synonyms).
"is not 0"

+struct xensnd_page_directory {
+    grant_ref_t gref_dir_next_page;
+    uint32_t num_grefs;
You can't fit that many requests on one page anyway, so I think it
would be better to limit this to 16 bits, and leave 16 bits reserved
for future use (specified as must-be-zero).
these are not for requests, but for buffers. you can imagine the size of this
buffer if we play something like 8 channels 192kHz...
(Which made me look
back at other reserved fields: You them "padding" in the ASCII art,
but "reserved" in the structure definitions, and I don't think I've
found a place stating that they're required to be zero filled, again
for future extensibility.)
done
+    /* zero-sized arrays are not allowed */
+    grant_ref_t gref[1] /* Variable length */
The second comment is fine; I once again don't think the former
(describing C language aspects again) is needed.
done
+struct xensnd_write_req {
+    uint32_t offset;
+    uint32_t len;
+};
+
+struct xensnd_read_req {
+    uint32_t offset;
+    uint32_t len;
+};
There should be just one of them, possibly named xensnd_rw_req.
agree
+ * Buffer passed with XENSND_OP_OPEN is used to exchange mute/unmute
+ * values:
+ *
+ *          0                 1                  2                3        
octet
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |   channel[0]    |   channel[1]    |   channel[2]    |   channel[3]    |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |   channel[i]    |   channel[i+1]  |   channel[i+2]  |   channel[i+3]  |
+ * +-----------------+-----------------+-----------------+-----------------+
There's no i defined anywhere here. Did you mean the last one to
be XENSND_OP_OPEN.pcm_channels - 1 (just like for the two volume
operations)?
fixed
+union xensnd_req {
+    struct {
+        uint16_t id;
+        uint8_t operation;
+        uint8_t stream_idx;
+        uint32_t padding;
reserved (I don't see why you'd need padding here)
done
+        union {
+            struct xensnd_open_req open;
+            struct xensnd_close_req close;
+            struct xensnd_write_req write;
+            struct xensnd_read_req read;
+            struct xensnd_get_vol_req get_vol;
+            struct xensnd_set_vol_req set_vol;
+            struct xensnd_mute_req mute;
+            struct xensnd_unmute_req unmute;
+        } op;
+    } data;
+    uint8_t raw[64];
BTW, I missed a change of 64 to 32 as discussed and agreed before
This is still misplaced imo, and belongs into the inner union, with the
array size suitably reduced. After all the initial part is mean to be
common for all verbs aiui, i.e. including future ones.
if we put this in then you have to find the biggest structure and calculate its size so
you know how many bytes need to be in that padding array
if you change something you have to redo the same again and beware
of the fact that this is needed
+union xensnd_resp {
+    struct {
+        uint16_t id;
+        uint8_t operation;
+        uint8_t stream_idx;
+        int8_t status;
+        uint8_t padding[3];
+    } data;
+    uint8_t raw[64];
And here you don't need the field at all - just make the padding field
large enough.
I can do it, but my concern is just like above
So, I would have the raw arrays where they are and only change their sizes to 32

Jan


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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.