On 2/5/19 12:44 PM, Oleksandr Andrushchenko wrote: > On 2/5/19 12:53 PM, Hans Verkuil wrote: >> On 2/5/19 11:44 AM, Oleksandr Andrushchenko wrote: >>> On 2/5/19 11:34 AM, Hans Verkuil wrote: >>>> 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?) >>> I have all operation categorized, e.g. there are commands >>> for configuration (XENCAMERA_OP_CONFIG_XXX), >>> buffer handling (XENCAMERA_OP_BUF_XXX) etc., so I prefer to >>> keep the name as is. >>>> 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. >>> Well, I tend to keep this as is with no additional op. >>>> 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. >>> Next try: >>> >>> * num_bufs - uint8_t, desired number of buffers to be used. >>> * >>> * If num_bufs is not zero then the backend validates the requested number of >>> * buffers and responds with the number of buffers allowed for this frontend. >>> * 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. >>> * Frontend is allowed to send multiple XENCAMERA_OP_BUF_REQUEST requests >>> * before sending XENCAMERA_OP_STREAM_START request to update or tune the >>> * final configuration. >>> * Frontend is not allowed to change the number of buffers and/or camera >>> * configuration after the streaming has started. >> This all looks good. > Great >>> * >>> * 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. >> You just say that if you want to reconfigure (and this only applies to reconfigure, >> not to the initial configure step since then there are no buffers allocated yet), >> then you need to call this with num_bufs == 0. But you don't explain what this op >> does if num_bufs == 0! >> >> So before this sentence you need to add a description of what this op does if num_bufs >> is 0, and change '(re)configuration' to 'reconfiguration'. > Well, it is a good question. I already describe what happens if > streaming has stopped and buffers destroyed and num_bufs == 0: > this is a reconfiguration. > > I also have a note that "Frontend is not allowed to change the > number of buffers and/or camera configuration after the streaming > has started.": this is the case that we cannot change the number of > buffers during the streaming, e.g. one cannot send num_bufs == 0 > at this time. > > So, what is not covered is that the streaming has never started, > num_bufs has or has not been set to some value and now frontend > requests num_bufs == 0? > It seems that we can state that in this case backend does > nothing or it may free any buffers if it has allocated any, so the > tail reads as: > > * If num_bufs is 0 and streaming has not started yet, then the backend may > * free all previously allocated buffers (if any) or do nothing. Much better. Now you actually explain what this op does if num_bufs == 0. > * > * If camera reconfiguration is required then this request must be sent with > * num_bufs set to zero and streaming must be stopped and buffers destroyed. Shouldn't the order be to first stop streaming, then call this request with num_bufs set to 0 and finally destroy the buffers? Trying to call this if streaming is in progress will result in an error. I think that should be documented as well. Sorry for paying so much attention to this, but I think it is important that this is documented precisely. Regards, Hans > * > * Please note, that the number of buffers in this request must not exceed > * the value configured in XenStore.max-buffers. > * > * See response format for this request. > >> >> Regards, >> >> Hans >> >>> * >>> * Please note, that the number of buffers in this request must not exceed >>> * the value configured in XenStore.max-buffers. >>> * >>> * See response format for this request. >>> >>>> 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 >