Subject: Re: [Xen-devel][PATCH v4 1/1] cameraif: add ABI for para-virtual
 camera
To:     Oleksandr Andrushchenko <Oleksandr_Andrushchenko@epam.com>,
        Hans Verkuil <hverkuil@xs4all.nl>,
        "xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
        "konrad.wilk@oracle.com" <konrad.wilk@oracle.com>,
        "jgross@suse.com" <jgross@suse.com>,
        "boris.ostrovsky@oracle.com" <boris.ostrovsky@oracle.com>,
        "mchehab@kernel.org" <mchehab@kernel.org>,
        "linux-media@vger.kernel.org" <linux-media@vger.kernel.org>,
        "sakari.ailus@linux.intel.com" <sakari.ailus@linux.intel.com>,
        "koji.matsuoka.xm@renesas.com" <koji.matsuoka.xm@renesas.com>
References: <20190115093853.15495-1-andr2000@gmail.com>
 <20190115093853.15495-2-andr2000@gmail.com>
 <393f824d-e543-476c-777f-402bcc1c0bcb@xs4all.nl>
 <1152536e-9238-4192-653e-b784b34b8a0d@epam.com>
From:   Oleksandr Andrushchenko <andr2000@gmail.com>
Message-ID: <d8476f24-1952-e822-aa75-b8a5f5d5a552@gmail.com>
Date:   Tue, 5 Feb 2019 10:48:48 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.4.0
MIME-Version: 1.0
In-Reply-To: <1152536e-9238-4192-653e-b784b34b8a0d@epam.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Sender: linux-media-owner@vger.kernel.org
Precedence: bulk

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