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

[Hans Verkuil <hverkuil@xs4all.nl>]

On 3/12/19 10:35 AM, Oleksandr Andrushchenko wrote:
> On 3/12/19 11:30 AM, Hans Verkuil wrote:
>> On 3/12/19 10:08 AM, Oleksandr Andrushchenko wrote:
>>> On 3/12/19 10:58 AM, Hans Verkuil wrote:
>>>> Hi Oleksandr,
>>>>
>>>> Just one comment:
>>>>
>>>> On 3/12/19 9:20 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 | 1370 ++++++++++++++++++++++++++++++
>>>>>    1 file changed, 1370 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..1ae4c51ea758
>>>>> --- /dev/null
>>>>> +++ b/xen/include/public/io/cameraif.h
>>>>> @@ -0,0 +1,1370 @@
>>>> <snip>
>>>>
>>>>> +/*
>>>>> + * Request camera buffer's layout:
>>>>> + *         0                1                 2               3        octet
>>>>> + * +----------------+----------------+----------------+----------------+
>>>>> + * |               id                | _BUF_GET_LAYOUT|   reserved     | 4
>>>>> + * +----------------+----------------+----------------+----------------+
>>>>> + * |                             reserved                              | 8
>>>>> + * +----------------+----------------+----------------+----------------+
>>>>> + * |/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/\/|
>>>>> + * +----------------+----------------+----------------+----------------+
>>>>> + * |                             reserved                              | 64
>>>>> + * +----------------+----------------+----------------+----------------+
>>>>> + *
>>>>> + * See response format for this request.
>>>>> + *
>>>>> + *
>>>>> + * 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.
>>>>> + *
>>>>> + * 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 last sentence isn't quite right, and I missed that when reviewing the
>>>> proposed text during the v4 discussions.
>>>>
>>>> The bit about not being allowed to change the number of buffers when streaming
>>>> has started is correct.
>>>>
>>>> But the camera configuration is more strict: you can't change the camera
>>>> configuration after this request unless you call this again with num_bufs = 0.
>>>>
>>>> The camera configuration changes the buffer size, so once the buffers are
>>>> allocated you can no longer change the camera config. It is unrelated to streaming.
>>> Can you please give me a hint of what would be the right thing to put in?
>> How about this:
>>
>> Frontend is not allowed to change the camera configuration after this call with
>> a non-zero value of num_bufs. If camera reconfiguration is required then this
>> request must be sent with num_bufs set to zero and any created buffers must be
>> destroyed first.
>>
>> Frontend is not allowed to change the number of buffers after the streaming has started.
> Sounds great, so I'll replace:
> 
> "Frontend is not allowed to change the number of buffers and/or camera
>   configuration after the streaming has started."
> 
> with:
> 
> "Frontend is not allowed to change the camera configuration after this 
> call with
>   a non-zero value of num_bufs. If camera reconfiguration is required 
> then this
>   request must be sent with num_bufs set to zero and any created buffers 
> must be
>   destroyed first.
> 
>   Frontend is not allowed to change the number of buffers after the 
> streaming has started.
> "
> 
> Are these all the changes you see at the moment?

Also this change below...

>>>>> + *
>>>>> + * If num_bufs is 0 and streaming has not started yet, then the backend will
>>>>> + * free all previously allocated buffers (if any).
>>>>> + * Trying to call this if streaming is in progress will result in an error.
>>>>> + *
>>>>> + * If camera reconfiguration is required then the streaming must be stopped
>>>>> + * and this request must be sent with num_bufs set to zero and finally
>>>>> + * buffers destroyed.
>> I would rewrite the last part as well:
>>
>> ...set to zero and any created buffers must be destroyed.

^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

And note my note below :-)

>>
>>
>> Note that "any created buffers must be destroyed" is something that you need to
>> check for in your code if I am not mistaken.

^^^^^^^^^^^^^^^^^

Regards,

	Hans