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

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

On 1/15/19 4:44 PM, Hans Verkuil wrote:
> Hi Oleksandr,
Hello, Hans!
> 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.
Good catch, will remove
>
> <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.
I'll think about what can be done here
>> + *
>> + * 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
Will fix
>> + *    streaming is stopped and buffers destroyed
>> + */
> Regards,
>
>       Hans
Thank you for helping with this,
Oleksandr
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel