[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



On 2/5/19 2:14 PM, Hans Verkuil wrote:
On 2/5/19 12:44 PM, Oleksandr Andrushchenko wrote:
On 2/5/19 12:53 PM, Hans Verkuil wrote:
On 2/5/19 11:44 AM, Oleksandr Andrushchenko wrote:
On 2/5/19 11:34 AM, Hans Verkuil wrote:
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?)
I have all operation categorized, e.g. there are commands
for configuration (XENCAMERA_OP_CONFIG_XXX),
buffer handling (XENCAMERA_OP_BUF_XXX) etc., so I prefer to
keep the name as is.
    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.
Well, I tend to keep this as is with no additional op.
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.
Next try:

   * 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 all looks good.
Great
   *
   * 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.
You just say that if you want to reconfigure (and this only applies to 
reconfigure,
not to the initial configure step since then there are no buffers allocated 
yet),
then you need to call this with num_bufs == 0. But you don't explain what this 
op
does if num_bufs == 0!

So before this sentence you need to add a description of what this op does if 
num_bufs
is 0, and change '(re)configuration' to 'reconfiguration'.
Well, it is a good question. I already describe what happens if
streaming has stopped and buffers destroyed and num_bufs == 0:
this is a reconfiguration.

I also have a note that "Frontend is not allowed to change the
number of buffers and/or camera configuration after the streaming
has started.": this is the case that we cannot change the number of
buffers during the streaming, e.g. one cannot send num_bufs == 0
at this time.

So, what is not covered is that the streaming has never started,
num_bufs has or has not been set to some value and now frontend
requests num_bufs == 0?
It seems that we can state that in this case backend does
nothing or it may free any buffers if it has allocated any, so the
tail reads as:

  * If num_bufs is 0 and streaming has not started yet, then the backend may
  * free all previously allocated buffers (if any) or do nothing.
Much better. Now you actually explain what this op does if num_bufs == 0.

  *
  * If camera reconfiguration is required then this request must be sent with
  * num_bufs set to zero and streaming must be stopped and buffers destroyed.
Shouldn't the order be to first stop streaming, then call this request with
num_bufs set to 0 and finally destroy the buffers?
Yes, I meant that, but this does need to be more clear:
 * 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.


Trying to call this if streaming is in progress will result in an error.
I think that should be documented as well.
Sure

Sorry for paying so much attention to this, but I think it is important that
this is documented precisely.
Thank you for helping with this - your comments are really
important and make the description precise. Ok, so finally:

 * 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.
 *
 * If num_bufs is 0 and streaming has not started yet, then the backend may
 * free all previously allocated buffers (if any) or do nothing.
 * 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.
 *
 * 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.

Regards,

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

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.

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.