On 2/5/19 9:48 AM, Oleksandr Andrushchenko wrote: > On 1/23/19 10:14 AM, Oleksandr Andrushchenko wrote: >> Any comments from Xen community? >> Konrad? > While I am still looking forward to any comments from Xen community... >> >> On 1/15/19 4:44 PM, Hans Verkuil wrote: >>> Hi Oleksandr, >>> >>> 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. >>> >>> <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. > Hans, how about: > > * num_bufs - uint8_t, desired number of buffers to be used. > * > * The number of buffers in this request must not exceed the value configured > * in XenStore.max-buffers. If the number of buffers is not zero then after this > * request the camera configuration cannot be changed. In order to allow camera > * (re)configuration this request must be sent with num_bufs set to zero and > * the streaming must be stopped and buffers destroyed. > * It is allowed for the frontend to send multiple XENCAMERA_OP_BUF_REQUEST > * requests before sending XENCAMERA_OP_STREAM_START request to update or > * tune the final configuration. > * Frontend is responsible for checking the corresponding response in order to > * see if the values reported back by the backend do match the desired ones > * and can be accepted. > * > * See response format for this request. > */ Hmm, it still is awkward. Part of the reason for that is that VIDIOC_REQBUFS is just weird in that a value of 0 has a special meaning. Perhaps it would be much cleaner for the Xen implementation to just add a new OP: _OP_FREE_ALL_BUFS (or perhaps _RELEASE_ALL_BUFS) that effectively does VIDIOC_REQBUFS with a 0 count value. And this OP_BUF_REQUEST (wouldn't OP_REQUEST_BUFS be a better name?) would then do nothing or return an error if num_bufs == 0. If you don't want to create a new Xen op, then I would change the text some more since you do not actually explain what the op does if num_bufs is 0. I would write something like this: If num_bufs is greater than 0, then <describe what happens>. If num_bufs is equal to 0, then <describe what happens>. If num_bufs is not zero then after this request the camera configuration cannot be changed. In order to allow camera (re)configuration this request must be sent with num_bufs set to zero and the streaming must be stopped and buffers destroyed. Regards, Hans > >>>> + * >>>> + * 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 >>> >>>> + * streaming is stopped and buffers destroyed >>>> + */ >>> Regards, >>> >>> Hans >
