Re: [PATCH v8 13/13] [media] v4l: Document explicit synchronization behavior

2018-03-14 Thread Hans Verkuil 
On 03/13/2018 08:33 PM, Hans Verkuil wrote:
> On 03/09/2018 09:49 AM, Gustavo Padovan wrote:
>> From: Gustavo Padovan 
>>
>> Add section to VIDIOC_QBUF and VIDIOC_QUERY_BUF about it
>>
>> v6:  - Close some gaps in the docs (Hans)
>>
>> v5:
>>  - Remove V4L2_CAP_ORDERED
>>  - Add doc about V4L2_FMT_FLAG_UNORDERED
>>
>> v4:
>>  - Document ordering behavior for in-fences
>>  - Document V4L2_CAP_ORDERED capability
>>  - Remove doc about OUT_FENCE event
>>  - Document immediate return of out-fence in QBUF
>>
>> v3:
>>  - make the out_fence refer to the current buffer (Hans)
>>  - Note what happens when the IN_FENCE is not set (Hans)
>>
>> v2:
>>  - mention that fences are files (Hans)
>>  - rework for the new API
>>
>> Signed-off-by: Gustavo Padovan 
>> ---
>>  Documentation/media/uapi/v4l/vidioc-qbuf.rst | 55 
>> +++-
>>  Documentation/media/uapi/v4l/vidioc-querybuf.rst | 12 --
>>  2 files changed, 63 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/media/uapi/v4l/vidioc-qbuf.rst 
>> b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
>> index 9e448a4aa3aa..371d84966e34 100644
>> --- a/Documentation/media/uapi/v4l/vidioc-qbuf.rst
>> +++ b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
>> @@ -54,7 +54,7 @@ When the buffer is intended for output (``type`` is
>>  or ``V4L2_BUF_TYPE_VBI_OUTPUT``) applications must also initialize the
>>  ``bytesused``, ``field`` and ``timestamp`` fields, see :ref:`buffer`
>>  for details. Applications must also set ``flags`` to 0. The
>> -``reserved2`` and ``reserved`` fields must be set to 0. When using the
>> +``reserved`` field must be set to 0. When using the
>>  :ref:`multi-planar API `, the ``m.planes`` field must
>>  contain a userspace pointer to a filled-in array of struct
>>  :c:type:`v4l2_plane` and the ``length`` field must be set
>> @@ -118,6 +118,59 @@ immediately with an ``EAGAIN`` error code when no 
>> buffer is available.
>>  The struct :c:type:`v4l2_buffer` structure is specified in
>>  :ref:`buffer`.
>>  
>> +Explicit Synchronization
>> +
>> +
>> +Explicit Synchronization allows us to control the synchronization of
>> +shared buffers from userspace by passing fences to the kernel and/or
>> +receiving them from it. Fences passed to the kernel are named in-fences and
>> +the kernel should wait on them to signal before using the buffer. On the 
>> other
>> +side, the kernel can create out-fences for the buffers it queues to the
>> +drivers. Out-fences signal when the driver is finished with buffer, i.e., 
>> the
>> +buffer is ready. The fences are represented as a file and passed as a file
>> +descriptor to userspace.
>> +
>> +The in-fences are communicated to the kernel at the ``VIDIOC_QBUF`` ioctl
>> +using the ``V4L2_BUF_FLAG_IN_FENCE`` buffer flag and the `fence_fd` field. 
>> If
>> +an in-fence needs to be passed to the kernel, `fence_fd` should be set to 
>> the
>> +fence file descriptor number and the ``V4L2_BUF_FLAG_IN_FENCE`` should be 
>> set
>> +as well. Setting one but not the other will cause ``VIDIOC_QBUF`` to return
>> +with an error.
> 
> This sentence is confusing since it is not clear what 'one' and 'the other' 
> refer
> to. Be specific here. I think it should be 'Setting V4L2_BUF_FLAG_IN_FENCE 
> but not
> fence_fd'.

Ignore this comment.

> 
>  The fence_fd field will be ignored if the
>> +``V4L2_BUF_FLAG_IN_FENCE`` is not set.

Looking at the code, I don't think this is correct. You get an error if 
IN_FENCE is
not set but fence_fd is. Removing this sentence would fix this and the previous
sentence would make a lot more sense.

>> +
>> +The videobuf2-core will guarantee that all buffers queued with an in-fence 
>> will
>> +be queued to the drivers in the same order. Fences may signal out of order, 
>> so
>> +this guarantee at videobuf2 is necessary to not change ordering. So when
>> +waiting on a fence to signal all buffers queued after will be also block 
>> until
> 
> after -> afterwards
> will be also block -> will also be blocked
> 
>> +that fence signal.
> 
> signal -> signals

What is missing in the documentation is what happens when you mix in-fence 
buffers
and buffers without an in-fence.

Regards,

Hans

Re: [PATCH v8 13/13] [media] v4l: Document explicit synchronization behavior

2018-03-13 Thread Hans Verkuil 
On 03/09/2018 09:49 AM, Gustavo Padovan wrote:
> From: Gustavo Padovan 
> 
> Add section to VIDIOC_QBUF and VIDIOC_QUERY_BUF about it
> 
> v6:   - Close some gaps in the docs (Hans)
> 
> v5:
>   - Remove V4L2_CAP_ORDERED
>   - Add doc about V4L2_FMT_FLAG_UNORDERED
> 
> v4:
>   - Document ordering behavior for in-fences
>   - Document V4L2_CAP_ORDERED capability
>   - Remove doc about OUT_FENCE event
>   - Document immediate return of out-fence in QBUF
> 
> v3:
>   - make the out_fence refer to the current buffer (Hans)
>   - Note what happens when the IN_FENCE is not set (Hans)
> 
> v2:
>   - mention that fences are files (Hans)
>   - rework for the new API
> 
> Signed-off-by: Gustavo Padovan 
> ---
>  Documentation/media/uapi/v4l/vidioc-qbuf.rst | 55 
> +++-
>  Documentation/media/uapi/v4l/vidioc-querybuf.rst | 12 --
>  2 files changed, 63 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/media/uapi/v4l/vidioc-qbuf.rst 
> b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
> index 9e448a4aa3aa..371d84966e34 100644
> --- a/Documentation/media/uapi/v4l/vidioc-qbuf.rst
> +++ b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
> @@ -54,7 +54,7 @@ When the buffer is intended for output (``type`` is
>  or ``V4L2_BUF_TYPE_VBI_OUTPUT``) applications must also initialize the
>  ``bytesused``, ``field`` and ``timestamp`` fields, see :ref:`buffer`
>  for details. Applications must also set ``flags`` to 0. The
> -``reserved2`` and ``reserved`` fields must be set to 0. When using the
> +``reserved`` field must be set to 0. When using the
>  :ref:`multi-planar API `, the ``m.planes`` field must
>  contain a userspace pointer to a filled-in array of struct
>  :c:type:`v4l2_plane` and the ``length`` field must be set
> @@ -118,6 +118,59 @@ immediately with an ``EAGAIN`` error code when no buffer 
> is available.
>  The struct :c:type:`v4l2_buffer` structure is specified in
>  :ref:`buffer`.
>  
> +Explicit Synchronization
> +
> +
> +Explicit Synchronization allows us to control the synchronization of
> +shared buffers from userspace by passing fences to the kernel and/or
> +receiving them from it. Fences passed to the kernel are named in-fences and
> +the kernel should wait on them to signal before using the buffer. On the 
> other
> +side, the kernel can create out-fences for the buffers it queues to the
> +drivers. Out-fences signal when the driver is finished with buffer, i.e., the
> +buffer is ready. The fences are represented as a file and passed as a file
> +descriptor to userspace.
> +
> +The in-fences are communicated to the kernel at the ``VIDIOC_QBUF`` ioctl
> +using the ``V4L2_BUF_FLAG_IN_FENCE`` buffer flag and the `fence_fd` field. If
> +an in-fence needs to be passed to the kernel, `fence_fd` should be set to the
> +fence file descriptor number and the ``V4L2_BUF_FLAG_IN_FENCE`` should be set
> +as well. Setting one but not the other will cause ``VIDIOC_QBUF`` to return
> +with an error.

This sentence is confusing since it is not clear what 'one' and 'the other' 
refer
to. Be specific here. I think it should be 'Setting V4L2_BUF_FLAG_IN_FENCE but 
not
fence_fd'.

 The fence_fd field will be ignored if the
> +``V4L2_BUF_FLAG_IN_FENCE`` is not set.
> +
> +The videobuf2-core will guarantee that all buffers queued with an in-fence 
> will
> +be queued to the drivers in the same order. Fences may signal out of order, 
> so
> +this guarantee at videobuf2 is necessary to not change ordering. So when
> +waiting on a fence to signal all buffers queued after will be also block 
> until

after -> afterwards
will be also block -> will also be blocked

> +that fence signal.

signal -> signals

> +
> +If the in-fence signals with an error the buffer will be marked with
> +``V4L2_BUF_FLAG_ERROR`` when returned to userspace at ``VIDIOC_DQBUF``.
> +Even with the error the order of dequeueing the buffers are preserved.

are -> is

> +
> +To get an out-fence back from V4L2 the ``V4L2_BUF_FLAG_OUT_FENCE`` flag 
> should
> +be set to ask for a fence to be attached to the buffer. The out-fence fd is
> +sent to userspace as a ``VIDIOC_QBUF`` return argument on the `fence_fd` 
> field.

on -> in

> +
> +Note the the same `fence_fd` field is used for both sending the in-fence as
> +input argument to receive the out-fence as a return argument. A buffer can

input argument -> an input argument and

> +have both in-fence ond out-fence.

have both an in-fence and an out-fence.

> +
> +At streamoff the out-fences will either signal normally if the driver waits
> +for the operations on the buffers to finish or signal with an error if the
> +driver cancels the pending operations. Buffers with in-fences won't be queued
> +to the driver if their fences signal. They will be cleaned up.
> +
> +The ``V4L2_FMT_FLAG_UNORDERED`` flag in ``VIDIOC_ENUM_FMT`` tells userspace
> +that the  when

Re: [PATCH v8 13/13] [media] v4l: Document explicit synchronization behavior

2018-03-13 Thread jacopo mondi 
Hi Gustavo,
  a very small comment below

On Fri, Mar 09, 2018 at 02:49:20PM -0300, Gustavo Padovan wrote:
> From: Gustavo Padovan 
>
> Add section to VIDIOC_QBUF and VIDIOC_QUERY_BUF about it
>
> v6:   - Close some gaps in the docs (Hans)
>
> v5:
>   - Remove V4L2_CAP_ORDERED
>   - Add doc about V4L2_FMT_FLAG_UNORDERED
>
> v4:
>   - Document ordering behavior for in-fences
>   - Document V4L2_CAP_ORDERED capability
>   - Remove doc about OUT_FENCE event
>   - Document immediate return of out-fence in QBUF
>
> v3:
>   - make the out_fence refer to the current buffer (Hans)
>   - Note what happens when the IN_FENCE is not set (Hans)
>
> v2:
>   - mention that fences are files (Hans)
>   - rework for the new API
>
> Signed-off-by: Gustavo Padovan 
> ---
>  Documentation/media/uapi/v4l/vidioc-qbuf.rst | 55 
> +++-
>  Documentation/media/uapi/v4l/vidioc-querybuf.rst | 12 --
>  2 files changed, 63 insertions(+), 4 deletions(-)
>
[snip]
> +Note the the same `fence_fd` field is used for both sending the in-fence as
> +input argument to receive the out-fence as a return argument. A buffer can
> +have both in-fence ond out-fence.

I feel like an "and" is missing here...

 the same `fence_fd` field is used for both sending the in-fence as
 input argument to receive the out-fence as a return argument

 the same `fence_fd` field is used for both sending the in-fence as
 input argument *and* to receive the out-fence as a return argument

I'm not a native speaker so I might be wrong though.

Thanks
   j


signature.asc
Description: PGP signature

[PATCH v8 13/13] [media] v4l: Document explicit synchronization behavior

2018-03-09 Thread Gustavo Padovan 
From: Gustavo Padovan 

Add section to VIDIOC_QBUF and VIDIOC_QUERY_BUF about it

v6: - Close some gaps in the docs (Hans)

v5:
- Remove V4L2_CAP_ORDERED
- Add doc about V4L2_FMT_FLAG_UNORDERED

v4:
- Document ordering behavior for in-fences
- Document V4L2_CAP_ORDERED capability
- Remove doc about OUT_FENCE event
- Document immediate return of out-fence in QBUF

v3:
- make the out_fence refer to the current buffer (Hans)
- Note what happens when the IN_FENCE is not set (Hans)

v2:
- mention that fences are files (Hans)
- rework for the new API

Signed-off-by: Gustavo Padovan 
---
 Documentation/media/uapi/v4l/vidioc-qbuf.rst | 55 +++-
 Documentation/media/uapi/v4l/vidioc-querybuf.rst | 12 --
 2 files changed, 63 insertions(+), 4 deletions(-)

diff --git a/Documentation/media/uapi/v4l/vidioc-qbuf.rst 
b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
index 9e448a4aa3aa..371d84966e34 100644
--- a/Documentation/media/uapi/v4l/vidioc-qbuf.rst
+++ b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
@@ -54,7 +54,7 @@ When the buffer is intended for output (``type`` is
 or ``V4L2_BUF_TYPE_VBI_OUTPUT``) applications must also initialize the
 ``bytesused``, ``field`` and ``timestamp`` fields, see :ref:`buffer`
 for details. Applications must also set ``flags`` to 0. The
-``reserved2`` and ``reserved`` fields must be set to 0. When using the
+``reserved`` field must be set to 0. When using the
 :ref:`multi-planar API `, the ``m.planes`` field must
 contain a userspace pointer to a filled-in array of struct
 :c:type:`v4l2_plane` and the ``length`` field must be set
@@ -118,6 +118,59 @@ immediately with an ``EAGAIN`` error code when no buffer 
is available.
 The struct :c:type:`v4l2_buffer` structure is specified in
 :ref:`buffer`.
 
+Explicit Synchronization
+
+
+Explicit Synchronization allows us to control the synchronization of
+shared buffers from userspace by passing fences to the kernel and/or
+receiving them from it. Fences passed to the kernel are named in-fences and
+the kernel should wait on them to signal before using the buffer. On the other
+side, the kernel can create out-fences for the buffers it queues to the
+drivers. Out-fences signal when the driver is finished with buffer, i.e., the
+buffer is ready. The fences are represented as a file and passed as a file
+descriptor to userspace.
+
+The in-fences are communicated to the kernel at the ``VIDIOC_QBUF`` ioctl
+using the ``V4L2_BUF_FLAG_IN_FENCE`` buffer flag and the `fence_fd` field. If
+an in-fence needs to be passed to the kernel, `fence_fd` should be set to the
+fence file descriptor number and the ``V4L2_BUF_FLAG_IN_FENCE`` should be set
+as well. Setting one but not the other will cause ``VIDIOC_QBUF`` to return
+with an error. The fence_fd field will be ignored if the
+``V4L2_BUF_FLAG_IN_FENCE`` is not set.
+
+The videobuf2-core will guarantee that all buffers queued with an in-fence will
+be queued to the drivers in the same order. Fences may signal out of order, so
+this guarantee at videobuf2 is necessary to not change ordering. So when
+waiting on a fence to signal all buffers queued after will be also block until
+that fence signal.
+
+If the in-fence signals with an error the buffer will be marked with
+``V4L2_BUF_FLAG_ERROR`` when returned to userspace at ``VIDIOC_DQBUF``.
+Even with the error the order of dequeueing the buffers are preserved.
+
+To get an out-fence back from V4L2 the ``V4L2_BUF_FLAG_OUT_FENCE`` flag should
+be set to ask for a fence to be attached to the buffer. The out-fence fd is
+sent to userspace as a ``VIDIOC_QBUF`` return argument on the `fence_fd` field.
+
+Note the the same `fence_fd` field is used for both sending the in-fence as
+input argument to receive the out-fence as a return argument. A buffer can
+have both in-fence ond out-fence.
+
+At streamoff the out-fences will either signal normally if the driver waits
+for the operations on the buffers to finish or signal with an error if the
+driver cancels the pending operations. Buffers with in-fences won't be queued
+to the driver if their fences signal. They will be cleaned up.
+
+The ``V4L2_FMT_FLAG_UNORDERED`` flag in ``VIDIOC_ENUM_FMT`` tells userspace
+that the  when using this format the order in which buffers are dequeued can
+be different from the order in which they were queued.
+
+Ordering is important to fences because it can optimize the pipeline with
+other drivers like a DRM/KMS display driver. For example, if a capture from the
+camera is happening in an orderly manner one can send the capture buffer
+out-fence to the DRM/KMS driver and rest sure that the buffers will be shown on
+the screen at the correct order. If an ordered queue can not be set then such
+arrangements with other drivers may not be possible.
 
 Return Value
 
diff --git