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

[Hans Verkuil <hverkuil@xxxxxxxxx>]

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
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel