Re: [Xen-devel][PATCH v4 1/1] cameraif: add ABI for para-virtual camera

[Oleksandr Andrushchenko <andr2000@gmail.com>]

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@epam.com>
>>>
>>> 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@epam.com>
>>> ---
>>>    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.
  */

>>> + *
>>> + * 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