On 1/15/19 4:44 PM, Hans Verkuil wrote: > Hi Oleksandr, Hello, Hans! > Just two remaining comments: > > On 1/15/19 10:38 AM, Oleksandr Andrushchenko wrote: >> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx> >> >> This is the ABI for the two halves of a para-virtualized >> camera driver which extends Xen's reach multimedia capabilities even >> farther enabling it for video conferencing, In-Vehicle Infotainment, >> high definition maps etc. >> >> The initial goal is to support most needed functionality with the >> final idea to make it possible to extend the protocol if need be: >> >> 1. Provide means for base virtual device configuration: >> - pixel formats >> - resolutions >> - frame rates >> 2. Support basic camera controls: >> - contrast >> - brightness >> - hue >> - saturation >> 3. Support streaming control >> >> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx> >> --- >> xen/include/public/io/cameraif.h | 1364 ++++++++++++++++++++++++++++++ >> 1 file changed, 1364 insertions(+) >> create mode 100644 xen/include/public/io/cameraif.h >> >> diff --git a/xen/include/public/io/cameraif.h b/xen/include/public/io/cameraif.h >> new file mode 100644 >> index 000000000000..246eb2457f40 >> --- /dev/null >> +++ b/xen/include/public/io/cameraif.h >> @@ -0,0 +1,1364 @@ > <snip> > >> +/* >> + ****************************************************************************** >> + * EVENT CODES >> + ****************************************************************************** >> + */ >> +#define XENCAMERA_EVT_FRAME_AVAIL 0x00 >> +#define XENCAMERA_EVT_CTRL_CHANGE 0x01 >> + >> +/* Resolution has changed. */ >> +#define XENCAMERA_EVT_CFG_FLG_RESOL (1 << 0) > I think this flag is a left-over from v2 and should be removed. Good catch, will remove > > <snip> > >> + * Request number of buffers to be used: >> + * 0 1 2 3 octet >> + * +----------------+----------------+----------------+----------------+ >> + * | id | _OP_BUF_REQUEST| reserved | 4 >> + * +----------------+----------------+----------------+----------------+ >> + * | reserved | 8 >> + * +----------------+----------------+----------------+----------------+ >> + * | num_bufs | reserved | 12 >> + * +----------------+----------------+----------------+----------------+ >> + * | reserved | 16 >> + * +----------------+----------------+----------------+----------------+ >> + * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/| >> + * +----------------+----------------+----------------+----------------+ >> + * | reserved | 64 >> + * +----------------+----------------+----------------+----------------+ >> + * >> + * num_bufs - uint8_t, desired number of buffers to be used. This is >> + * limited to the value configured in XenStore.max-buffers. >> + * Passing zero num_bufs in this request (after streaming has stopped >> + * and all buffers destroyed) unblocks camera configuration changes. > I think the phrase 'unblocks camera configuration changes' is confusing. > > In v3 this sentence came after the third note below, and so it made sense > in that context, but now the order has been reversed and it became hard to > understand. > > I'm not sure what the best approach is to fix this. One option is to remove > the third note and integrate it somehow in the sentence above. Or perhaps > do away with the 'notes' at all and just write a more extensive documentation > for this op. I leave that up to you. I'll think about what can be done here >> + * >> + * See response format for this request. >> + * >> + * Notes: >> + * - frontend must check the corresponding response in order to see >> + * if the values reported back by the backend do match the desired ones >> + * and can be accepted. >> + * - frontend may send multiple XENCAMERA_OP_BUF_REQUEST requests before >> + * sending XENCAMERA_OP_STREAM_START request to update or tune the >> + * configuration. >> + * - after this request camera configuration cannot be changed, unless > camera configuration -> the camera configuration Will fix >> + * streaming is stopped and buffers destroyed >> + */ > Regards, > > Hans Thank you for helping with this, Oleksandr
