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

[Hans Verkuil <hverkuil@xxxxxxxxx>]

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@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 | 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.


> 
> Thank you,
> Oleksandr
>> Regards,
>>
>>      Hans
>>
>>> + *
>>> + * 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.


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

>>> + *
>>> + * 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.
>>> + */
>> <snip>
>>
>> Regards,
>>
>>      Hans
> 


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