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

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




On 11/28/2016 03:27 PM, Jan Beulich wrote:
On 28.11.16 at 13:30, <andr2000@xxxxxxxxx> wrote:
+ * Request open - open a PCM stream for playback or capture:
+ *          0                 1                  2                3        
octet
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                 id                | XENSND_OP_OPEN  |     stream_idx  |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                                padding                                |
This still say "padding" instead of "reserved". Also isn't this still part
of the common header?
done
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                                pcm_rate                               |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |  pcm_format     |  pcm_channels   |             reserved              |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                               buffer_sz                               |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                         gref_directory_start                          |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
Don't you elsewhere use this as an "and so one" marker? That's not
what you want here afaict, because of ...

+ * +-----------------+-----------------+-----------------+-----------------+
+ * |                               reserved                                |
+ * +-----------------+-----------------+-----------------+-----------------+
... this.
done
+/*
+ * 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.
+ * 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 = DIV_ROUND_UP(XENSND_OP_OPEN.buffer_sz, PAGE_SIZE);
The header should be self contained, and there's no DIV_ROUND_UP()
anywhere under public/io/ for a reader to refer to. Please express this
using mathematical terms plus, if needed, standard C library ones.
done, will put:
num_grefs_total = (XENSND_OP_OPEN.buffer_sz + PAGE_SIZE - 1) / PAGE_SIZE
+ * 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:
+ *
+ *          0                 1                  2                3        
octet
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |   channel[0]    |   channel[1]    |   channel[2]    |   channel[3]    |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
+ * +-----------------+-----------------+-----------------+-----------------+
+ * |   channel[i]    |   channel[i+1]  |   channel[i+2]  |   channel[i+3]  |
+ * +-----------------+-----------------+-----------------+-----------------+
+ * +/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
+ * +-----------------+-----------------+-----------------+-----------------+
Iirc you had confirmed to change this, but you didn't: I can only repeat
that other than e.g. for the volume operations, it is unclear what the
upper bound here is, as you don't use N.
done
+struct xensnd_req {
+    uint16_t id;
+    uint8_t operation;
+    uint8_t stream_idx;
+    uint32_t reserved;
+    union {
+        struct xensnd_open_req open;
+        struct xensnd_close_req close;
+        struct xensnd_rw_req write;
+        struct xensnd_rw_req read;
I guess this has been this way before, but - why two of them? Just like
thy type can be shared, the field needs to be here just once (perhaps
named "rw").
done
+        struct xensnd_get_vol_req get_vol;
+        struct xensnd_set_vol_req set_vol;
Same here - if these dummy structures really need to survive, then
I don't think you need multiple for the volume operations or ...

+        struct xensnd_mute_req mute;
+        struct xensnd_unmute_req unmute;
... or the muting ones. In fact, if these dummy structures are needed
in the first place, perhaps just one total would suffice?
I will remove all empty structures
+struct xensnd_resp {
+    uint16_t id;
+    uint8_t operation;
+    uint8_t stream_idx;
+    int8_t status;
+    uint8_t padding[26];
+};
2 + 1 + 1 + 1 + 26 = 31, which I don't think is what you want.
changed 26 to 27

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel

 


Rackspace

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