[PATCH v3 00/15] V4L2 Explicit Synchronization support

[PATCH v3 00/15] V4L2 Explicit Synchronization support

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.com>

Hi,

Refer to the documentation on the first patch for the details. The previous
iteration is here: https://www.mail-archive.com/linux-media@vger.kernel.org/msg118077.html

The 2nd patch proposes an userspace API for fences, then on patch 3 we
prepare to the addition of in-fences in patch 4, by introducing the
infrastructure on vb2 to wait on an in-fence signal before queueing the
buffer in the driver.

Patch 5 fix uvc v4l2 event handling and patch 6 configure q->dev for
vivid drivers to enable to subscribe and dequeue events on it.

Patches 7-9 enables support to notify BUF_QUEUED events, the event send
to userspace the out-fence fd and the index of the buffer that was queued.

Patches 10-11 add support to mark queues as ordered. Finally patches 12
and 13 add more fence infrastructure to support out-fences, patch 13 exposes
close_fd() and patch 14 adds support to out-fences. 

It only works for ordered queues for now, see open question at the end
of the letter.

Test tool can be found at:
https://git.collabora.com/cgit/user/padovan/v4l2-test.git/

Main Changes
------------

* out-fences: change in behavior: the out-fence fd now comes out of the
BUF_QUEUED event along with the buffer id.

All other changes are recorded on the patches' commit messages.

Open Questions
--------------

* non-ordered devices, like m2m: I've been thinking a lot about those
  and one possibility is to have a way to tell userspace that the queue
  is not ordered and then associate the fence with the current buffer in
  QBUF instead of the next one to be queued. Of course, there won't be
  any ordering between the fences. But it may be enough for userspace to
  take advantage of Explicit Synchronization in such cases. Any
  thoughts?

* OUTPUT devices and in-fence. If I understood OUTPUT devices correctly
  it is desirable to queue the buffers to the driver in the same order
  we received them from userspace. If that is correct, shouldn't we add
  some mechanism to prevent buffer whose fence signaled to jump ahead of
  others?

Gustavo Padovan (14):
  [media] v4l: Document explicit synchronization behaviour
  [media] vb2: add explicit fence user API
  [media] vb2: check earlier if stream can be started
  [media] vb2: add in-fence support to QBUF
  [media] uvc: enable subscriptions to other events
  [media] vivid: assign the specific device to the vb2_queue->dev
  [media] v4l: add V4L2_EVENT_BUF_QUEUED event
  [media] vb2: add .buffer_queued() to notify queueing in the driver
  [media] v4l: add support to BUF_QUEUED event
  [media] vb2: add 'ordered' property to queues
  [media] vivid: mark vivid queues as ordered
  [media] vb2: add infrastructure to support out-fences
  fs/files: export close_fd() symbol
  [media] vb2: add out-fence support to QBUF

Javier Martinez Canillas (1):
  [media] vb2: add videobuf2 dma-buf fence helpers

 Documentation/media/uapi/v4l/buffer.rst         |  19 ++
 Documentation/media/uapi/v4l/vidioc-dqevent.rst |  23 +++
 Documentation/media/uapi/v4l/vidioc-qbuf.rst    |  31 ++++
 Documentation/media/videodev2.h.rst.exceptions  |   1 +
 drivers/android/binder.c                        |   2 +-
 drivers/media/platform/vivid/vivid-core.c       |  15 +-
 drivers/media/usb/cpia2/cpia2_v4l.c             |   2 +-
 drivers/media/usb/uvc/uvc_v4l2.c                |   2 +-
 drivers/media/v4l2-core/Kconfig                 |   1 +
 drivers/media/v4l2-core/v4l2-compat-ioctl32.c   |   4 +-
 drivers/media/v4l2-core/v4l2-ctrls.c            |   6 +-
 drivers/media/v4l2-core/videobuf2-core.c        | 221 ++++++++++++++++++++++--
 drivers/media/v4l2-core/videobuf2-v4l2.c        |  63 ++++++-
 fs/file.c                                       |   5 +-
 fs/open.c                                       |   2 +-
 include/linux/fdtable.h                         |   2 +-
 include/media/videobuf2-core.h                  |  63 ++++++-
 include/media/videobuf2-fence.h                 |  49 ++++++
 include/uapi/linux/videodev2.h                  |  15 +-
 19 files changed, 489 insertions(+), 37 deletions(-)
 create mode 100644 include/media/videobuf2-fence.h

-- 
2.13.5

[PATCH v3 01/15] [media] v4l: Document explicit synchronization behaviour

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.com>

Add section to VIDIOC_QBUF about it

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

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
---
 Documentation/media/uapi/v4l/vidioc-qbuf.rst | 31 ++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/Documentation/media/uapi/v4l/vidioc-qbuf.rst b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
index 1f3612637200..fae0b1431672 100644
--- a/Documentation/media/uapi/v4l/vidioc-qbuf.rst
+++ b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
@@ -117,6 +117,37 @@ 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 them to signal before using the buffer, i.e., queueing
+it to the driver. 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, that is the buffer is ready. The fence are represented
+by file and passed as 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
+flags 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. Failure to set both will
+cause ``VIDIOC_QBUF`` to return with error.
+
+To get a out-fence back from V4L2 the ``V4L2_BUF_FLAG_OUT_FENCE`` flag should
+be set to notify it that the next queued buffer should have a fence attached to
+it. That means the out-fence may not be associated with the buffer in the
+current ``VIDIOC_QBUF`` ioctl call because the ordering in which videobuf2 core
+queues the buffers to the drivers can't be guaranteed. To become aware of the
+of the next queued buffer and the out-fence attached to it the
+``V4L2_EVENT_BUF_QUEUED`` event should be used. It will trigger an event
+for every buffer queued to the V4L2 driver.
+
+At streamoff the out-fences will either signal normally if the drivers wait
+for the operations on the buffers to finish or signal with error if the
+driver cancel the pending operations.
 
 Return Value
 ============
-- 
2.13.5

[PATCH v3 02/15] [media] vb2: add explicit fence user API

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.com>

Turn the reserved2 field into fence_fd that we will use to send
an in-fence to the kernel and return an out-fence from the kernel to
userspace.

Two new flags were added, V4L2_BUF_FLAG_IN_FENCE, that should be used
when sending a fence to the kernel to be waited on, and
V4L2_BUF_FLAG_OUT_FENCE, to ask the kernel to give back an out-fence.

v2: add documentation

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
---
 Documentation/media/uapi/v4l/buffer.rst       | 19 +++++++++++++++++++
 drivers/media/usb/cpia2/cpia2_v4l.c           |  2 +-
 drivers/media/v4l2-core/v4l2-compat-ioctl32.c |  4 ++--
 drivers/media/v4l2-core/videobuf2-v4l2.c      |  2 +-
 include/uapi/linux/videodev2.h                |  4 +++-
 5 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/Documentation/media/uapi/v4l/buffer.rst b/Documentation/media/uapi/v4l/buffer.rst
index ae6ee73f151c..664507ad06c6 100644
--- a/Documentation/media/uapi/v4l/buffer.rst
+++ b/Documentation/media/uapi/v4l/buffer.rst
@@ -648,6 +648,25 @@ Buffer Flags
       - Start Of Exposure. The buffer timestamp has been taken when the
 	exposure of the frame has begun. This is only valid for the
 	``V4L2_BUF_TYPE_VIDEO_CAPTURE`` buffer type.
+    * .. _`V4L2-BUF-FLAG-IN-FENCE`:
+
+      - ``V4L2_BUF_FLAG_IN_FENCE``
+      - 0x00200000
+      - Ask V4L2 to wait on fence passed in ``fence_fd`` field. The buffer
+	won't be queued to the driver until the fence signals.
+
+    * .. _`V4L2-BUF-FLAG-OUT-FENCE`:
+
+      - ``V4L2_BUF_FLAG_OUT_FENCE``
+      - 0x00400000
+      - Request a fence for the next buffer to be queued to V4L2 driver.
+	The fence received back through the ``fence_fd`` field  doesn't
+	necessarily relate to the current buffer in the
+	:ref:`VIDIOC_QBUF <VIDIOC_QBUF>` ioctl. Although, most of the time
+	the fence will relate to the current buffer it can't be guaranteed.
+	So to tell userspace which buffer is associated to the out_fence,
+	one should listen for the ``V4L2_EVENT_BUF_QUEUED`` event that
+	provide the id of the buffer when it is queued to the V4L2 driver.
 
 
 
diff --git a/drivers/media/usb/cpia2/cpia2_v4l.c b/drivers/media/usb/cpia2/cpia2_v4l.c
index 3dedd83f0b19..6cde686bf44c 100644
--- a/drivers/media/usb/cpia2/cpia2_v4l.c
+++ b/drivers/media/usb/cpia2/cpia2_v4l.c
@@ -948,7 +948,7 @@ static int cpia2_dqbuf(struct file *file, void *fh, struct v4l2_buffer *buf)
 	buf->sequence = cam->buffers[buf->index].seq;
 	buf->m.offset = cam->buffers[buf->index].data - cam->frame_buffer;
 	buf->length = cam->frame_size;
-	buf->reserved2 = 0;
+	buf->fence_fd = -1;
 	buf->reserved = 0;
 	memset(&buf->timecode, 0, sizeof(buf->timecode));
 
diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
index 821f2aa299ae..d624fb5df130 100644
--- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
+++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
@@ -370,7 +370,7 @@ struct v4l2_buffer32 {
 		__s32		fd;
 	} m;
 	__u32			length;
-	__u32			reserved2;
+	__s32			fence_fd;
 	__u32			reserved;
 };
 
@@ -533,8 +533,8 @@ static int put_v4l2_buffer32(struct v4l2_buffer *kp, struct v4l2_buffer32 __user
 		put_user(kp->timestamp.tv_usec, &up->timestamp.tv_usec) ||
 		copy_to_user(&up->timecode, &kp->timecode, sizeof(struct v4l2_timecode)) ||
 		put_user(kp->sequence, &up->sequence) ||
-		put_user(kp->reserved2, &up->reserved2) ||
 		put_user(kp->reserved, &up->reserved) ||
+		put_user(kp->fence_fd, &up->fence_fd) ||
 		put_user(kp->length, &up->length))
 			return -EFAULT;
 
diff --git a/drivers/media/v4l2-core/videobuf2-v4l2.c b/drivers/media/v4l2-core/videobuf2-v4l2.c
index 0c0669976bdc..110fb45fef6f 100644
--- a/drivers/media/v4l2-core/videobuf2-v4l2.c
+++ b/drivers/media/v4l2-core/videobuf2-v4l2.c
@@ -203,7 +203,7 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, void *pb)
 	b->timestamp = ns_to_timeval(vb->timestamp);
 	b->timecode = vbuf->timecode;
 	b->sequence = vbuf->sequence;
-	b->reserved2 = 0;
+	b->fence_fd = -1;
 	b->reserved = 0;
 
 	if (q->is_multiplanar) {
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 185d6a0acc06..e5abab9a908c 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -924,7 +924,7 @@ struct v4l2_buffer {
 		__s32		fd;
 	} m;
 	__u32			length;
-	__u32			reserved2;
+	__s32			fence_fd;
 	__u32			reserved;
 };
 
@@ -961,6 +961,8 @@ struct v4l2_buffer {
 #define V4L2_BUF_FLAG_TSTAMP_SRC_SOE		0x00010000
 /* mem2mem encoder/decoder */
 #define V4L2_BUF_FLAG_LAST			0x00100000
+#define V4L2_BUF_FLAG_IN_FENCE			0x00200000
+#define V4L2_BUF_FLAG_OUT_FENCE			0x00400000
 
 /**
  * struct v4l2_exportbuffer - export of video buffer as DMABUF file descriptor
-- 
2.13.5

[PATCH v3 03/15] [media] vb2: check earlier if stream can be started

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.com>

To support explicit synchronization we need to run all operations that can
fail before we queue the buffer to the driver. With fences the queueing
will be delayed if the fence is not signaled yet and it will be better if
such callback do not fail.

For that we move the vb2_start_streaming() before the queuing for the
buffer may happen.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
---
 drivers/media/v4l2-core/videobuf2-core.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index cb115ba6a1d2..60f8b582396a 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -1399,29 +1399,27 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
 	trace_vb2_qbuf(q, vb);
 
 	/*
-	 * If already streaming, give the buffer to driver for processing.
-	 * If not, the buffer will be given to driver on next streamon.
-	 */
-	if (q->start_streaming_called)
-		__enqueue_in_driver(vb);
-
-	/* Fill buffer information for the userspace */
-	if (pb)
-		call_void_bufop(q, fill_user_buffer, vb, pb);
-
-	/*
 	 * If streamon has been called, and we haven't yet called
 	 * start_streaming() since not enough buffers were queued, and
 	 * we now have reached the minimum number of queued buffers,
 	 * then we can finally call start_streaming().
+	 *
+	 * If already streaming, give the buffer to driver for processing.
+	 * If not, the buffer will be given to driver on next streamon.
 	 */
 	if (q->streaming && !q->start_streaming_called &&
 	    q->queued_count >= q->min_buffers_needed) {
 		ret = vb2_start_streaming(q);
 		if (ret)
 			return ret;
+	} else if (q->start_streaming_called) {
+		__enqueue_in_driver(vb);
 	}
 
+	/* Fill buffer information for the userspace */
+	if (pb)
+		call_void_bufop(q, fill_user_buffer, vb, pb);
+
 	dprintk(2, "qbuf of buffer %d succeeded\n", vb->index);
 	return 0;
 }
-- 
2.13.5

[PATCH v3 04/15] [media] vb2: add in-fence support to QBUF

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.com>

Receive in-fence from userspace and add support for waiting on them
before queueing the buffer to the driver. Buffers are only queued
to the driver once they are ready. A buffer is ready when its
in-fence signals.

v4:
	- Add a comment about dma_fence_add_callback() not returning a
	error (Hans)
	- Call dma_fence_put(vb->in_fence) if fence signaled (Hans)
	- select SYNC_FILE under config VIDEOBUF2_CORE (Hans)
	- Move dma_fence_is_signaled() check to __enqueue_in_driver() (Hans)
	- Remove list_for_each_entry() in __vb2_core_qbuf() (Hans)
	-  Remove if (vb->state != VB2_BUF_STATE_QUEUED) from
	vb2_start_streaming() (Hans)
	- set IN_FENCE flags on __fill_v4l2_buffer (Hans)
	- Queue buffers to the driver as soon as they are ready (Hans)
	- call fill_user_buffer() after queuing the buffer (Hans)
	- add err: label to clean up fence
	- add dma_fence_wait() before calling vb2_start_streaming()

v3:	- document fence parameter
	- remove ternary if at vb2_qbuf() return (Mauro)
	- do not change if conditions behaviour (Mauro)

v2:
	- fix vb2_queue_or_prepare_buf() ret check
	- remove check for VB2_MEMORY_DMABUF only (Javier)
	- check num of ready buffers to start streaming
	- when queueing, start from the first ready buffer
	- handle queue cancel

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
---
 drivers/media/v4l2-core/Kconfig          |   1 +
 drivers/media/v4l2-core/videobuf2-core.c | 103 +++++++++++++++++++++++++++----
 drivers/media/v4l2-core/videobuf2-v4l2.c |  27 +++++++-
 include/media/videobuf2-core.h           |  11 +++-
 4 files changed, 127 insertions(+), 15 deletions(-)

diff --git a/drivers/media/v4l2-core/Kconfig b/drivers/media/v4l2-core/Kconfig
index a35c33686abf..3f988c407c80 100644
--- a/drivers/media/v4l2-core/Kconfig
+++ b/drivers/media/v4l2-core/Kconfig
@@ -83,6 +83,7 @@ config VIDEOBUF_DVB
 # Used by drivers that need Videobuf2 modules
 config VIDEOBUF2_CORE
 	select DMA_SHARED_BUFFER
+	select SYNC_FILE
 	tristate
 
 config VIDEOBUF2_MEMOPS
diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 60f8b582396a..b19c1bc4b083 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -1222,6 +1222,9 @@ static void __enqueue_in_driver(struct vb2_buffer *vb)
 {
 	struct vb2_queue *q = vb->vb2_queue;
 
+	if (vb->in_fence && !dma_fence_is_signaled(vb->in_fence))
+		return;
+
 	vb->state = VB2_BUF_STATE_ACTIVE;
 	atomic_inc(&q->owned_by_drv_count);
 
@@ -1273,6 +1276,20 @@ static int __buf_prepare(struct vb2_buffer *vb, const void *pb)
 	return 0;
 }
 
+static int __get_num_ready_buffers(struct vb2_queue *q)
+{
+	struct vb2_buffer *vb;
+	int ready_count = 0;
+
+	/* count num of buffers ready in front of the queued_list */
+	list_for_each_entry(vb, &q->queued_list, queued_entry) {
+		if (!vb->in_fence || dma_fence_is_signaled(vb->in_fence))
+			ready_count++;
+	}
+
+	return ready_count;
+}
+
 int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb)
 {
 	struct vb2_buffer *vb;
@@ -1361,7 +1378,19 @@ static int vb2_start_streaming(struct vb2_queue *q)
 	return ret;
 }
 
-int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
+static void vb2_qbuf_fence_cb(struct dma_fence *f, struct dma_fence_cb *cb)
+{
+	struct vb2_buffer *vb = container_of(cb, struct vb2_buffer, fence_cb);
+
+	dma_fence_put(vb->in_fence);
+	vb->in_fence = NULL;
+
+	if (vb->vb2_queue->start_streaming_called)
+		__enqueue_in_driver(vb);
+}
+
+int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb,
+		  struct dma_fence *fence)
 {
 	struct vb2_buffer *vb;
 	int ret;
@@ -1372,16 +1401,18 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
 	case VB2_BUF_STATE_DEQUEUED:
 		ret = __buf_prepare(vb, pb);
 		if (ret)
-			return ret;
+			goto err;
 		break;
 	case VB2_BUF_STATE_PREPARED:
 		break;
 	case VB2_BUF_STATE_PREPARING:
 		dprintk(1, "buffer still being prepared\n");
-		return -EINVAL;
+		ret = -EINVAL;
+		goto err;
 	default:
 		dprintk(1, "invalid buffer state %d\n", vb->state);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto err;
 	}
 
 	/*
@@ -1392,6 +1423,7 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
 	q->queued_count++;
 	q->waiting_for_buffers = false;
 	vb->state = VB2_BUF_STATE_QUEUED;
+	vb->in_fence = fence;
 
 	if (pb)
 		call_void_bufop(q, copy_timestamp, vb, pb);
@@ -1399,6 +1431,16 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
 	trace_vb2_qbuf(q, vb);
 
 	/*
+	 * If it is time to call vb2_start_streaming() wait for the fence
+	 * to signal first. Of course, this happens only once per streaming.
+	 * We want to run any step that might fail before we set the callback
+	 * to queue the fence when it signals.
+	 */
+	if (fence && !q->start_streaming_called &&
+	    __get_num_ready_buffers(q) == q->min_buffers_needed - 1)
+		dma_fence_wait(fence, true);
+
+	/*
 	 * If streamon has been called, and we haven't yet called
 	 * start_streaming() since not enough buffers were queued, and
 	 * we now have reached the minimum number of queued buffers,
@@ -1408,20 +1450,48 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
 	 * If not, the buffer will be given to driver on next streamon.
 	 */
 	if (q->streaming && !q->start_streaming_called &&
-	    q->queued_count >= q->min_buffers_needed) {
+	    __get_num_ready_buffers(q) >= q->min_buffers_needed) {
 		ret = vb2_start_streaming(q);
 		if (ret)
-			return ret;
-	} else if (q->start_streaming_called) {
-		__enqueue_in_driver(vb);
+			goto err;
 	}
 
+	/*
+	 * For explicit synchronization: If the fence didn't signal
+	 * yet we setup a callback to queue the buffer once the fence
+	 * signals, and then, return successfully. But if the fence
+	 * already signaled we lose the reference we held and queue the
+	 * buffer to the driver.
+	 */
+	if (fence) {
+		ret = dma_fence_add_callback(fence, &vb->fence_cb,
+					     vb2_qbuf_fence_cb);
+		if (!ret)
+			goto fill;
+
+		dma_fence_put(fence);
+		vb->in_fence = NULL;
+	}
+
+fill:
+	if (q->start_streaming_called && !vb->in_fence)
+		__enqueue_in_driver(vb);
+
 	/* Fill buffer information for the userspace */
 	if (pb)
 		call_void_bufop(q, fill_user_buffer, vb, pb);
 
 	dprintk(2, "qbuf of buffer %d succeeded\n", vb->index);
 	return 0;
+
+err:
+	if (vb->in_fence) {
+		dma_fence_put(vb->in_fence);
+		vb->in_fence = NULL;
+	}
+
+	return ret;
+
 }
 EXPORT_SYMBOL_GPL(vb2_core_qbuf);
 
@@ -1632,6 +1702,7 @@ EXPORT_SYMBOL_GPL(vb2_core_dqbuf);
 static void __vb2_queue_cancel(struct vb2_queue *q)
 {
 	unsigned int i;
+	struct vb2_buffer *vb;
 
 	/*
 	 * Tell driver to stop all transactions and release all queued
@@ -1654,6 +1725,14 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
 		WARN_ON(atomic_read(&q->owned_by_drv_count));
 	}
 
+	list_for_each_entry(vb, &q->queued_list, queued_entry) {
+		if (vb->in_fence) {
+			dma_fence_remove_callback(vb->in_fence, &vb->fence_cb);
+			dma_fence_put(vb->in_fence);
+			vb->in_fence = NULL;
+		}
+	}
+
 	q->streaming = 0;
 	q->start_streaming_called = 0;
 	q->queued_count = 0;
@@ -1720,7 +1799,7 @@ int vb2_core_streamon(struct vb2_queue *q, unsigned int type)
 	 * Tell driver to start streaming provided sufficient buffers
 	 * are available.
 	 */
-	if (q->queued_count >= q->min_buffers_needed) {
+	if (__get_num_ready_buffers(q) >= q->min_buffers_needed) {
 		ret = v4l_vb2q_enable_media_source(q);
 		if (ret)
 			return ret;
@@ -2240,7 +2319,7 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read)
 		 * Queue all buffers.
 		 */
 		for (i = 0; i < q->num_buffers; i++) {
-			ret = vb2_core_qbuf(q, i, NULL);
+			ret = vb2_core_qbuf(q, i, NULL, NULL);
 			if (ret)
 				goto err_reqbufs;
 			fileio->bufs[i].queued = 1;
@@ -2419,7 +2498,7 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
 
 		if (copy_timestamp)
 			b->timestamp = ktime_get_ns();
-		ret = vb2_core_qbuf(q, index, NULL);
+		ret = vb2_core_qbuf(q, index, NULL, NULL);
 		dprintk(5, "vb2_dbuf result: %d\n", ret);
 		if (ret)
 			return ret;
@@ -2522,7 +2601,7 @@ static int vb2_thread(void *data)
 		if (copy_timestamp)
 			vb->timestamp = ktime_get_ns();;
 		if (!threadio->stop)
-			ret = vb2_core_qbuf(q, vb->index, NULL);
+			ret = vb2_core_qbuf(q, vb->index, NULL, NULL);
 		call_void_qop(q, wait_prepare, q);
 		if (ret || threadio->stop)
 			break;
diff --git a/drivers/media/v4l2-core/videobuf2-v4l2.c b/drivers/media/v4l2-core/videobuf2-v4l2.c
index 110fb45fef6f..8c322cd1b346 100644
--- a/drivers/media/v4l2-core/videobuf2-v4l2.c
+++ b/drivers/media/v4l2-core/videobuf2-v4l2.c
@@ -23,6 +23,7 @@
 #include <linux/sched.h>
 #include <linux/freezer.h>
 #include <linux/kthread.h>
+#include <linux/sync_file.h>
 
 #include <media/v4l2-dev.h>
 #include <media/v4l2-fh.h>
@@ -178,6 +179,11 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b,
 		return -EINVAL;
 	}
 
+	if ((b->fence_fd != -1) && !(b->flags & V4L2_BUF_FLAG_IN_FENCE)) {
+		dprintk(1, "%s: fence_fd set without IN_FENCE flag\n", opname);
+		return -EINVAL;
+	}
+
 	return __verify_planes_array(q->bufs[b->index], b);
 }
 
@@ -203,9 +209,14 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, void *pb)
 	b->timestamp = ns_to_timeval(vb->timestamp);
 	b->timecode = vbuf->timecode;
 	b->sequence = vbuf->sequence;
-	b->fence_fd = -1;
 	b->reserved = 0;
 
+	b->fence_fd = -1;
+	if (vb->in_fence)
+		b->flags |= V4L2_BUF_FLAG_IN_FENCE;
+	else
+		b->flags &= ~V4L2_BUF_FLAG_IN_FENCE;
+
 	if (q->is_multiplanar) {
 		/*
 		 * Fill in plane-related data if userspace provided an array
@@ -560,6 +571,7 @@ EXPORT_SYMBOL_GPL(vb2_create_bufs);
 
 int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
 {
+	struct dma_fence *fence = NULL;
 	int ret;
 
 	if (vb2_fileio_is_active(q)) {
@@ -568,7 +580,18 @@ int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
 	}
 
 	ret = vb2_queue_or_prepare_buf(q, b, "qbuf");
-	return ret ? ret : vb2_core_qbuf(q, b->index, b);
+	if (ret)
+		return ret;
+
+	if (b->flags & V4L2_BUF_FLAG_IN_FENCE) {
+		fence = sync_file_get_fence(b->fence_fd);
+		if (!fence) {
+			dprintk(1, "failed to get in-fence from fd\n");
+			return -EINVAL;
+		}
+	}
+
+	return vb2_core_qbuf(q, b->index, b, fence);
 }
 EXPORT_SYMBOL_GPL(vb2_qbuf);
 
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index ef9b64398c8c..41cda762ff0a 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -16,6 +16,7 @@
 #include <linux/mutex.h>
 #include <linux/poll.h>
 #include <linux/dma-buf.h>
+#include <linux/dma-fence.h>
 
 #define VB2_MAX_FRAME	(32)
 #define VB2_MAX_PLANES	(8)
@@ -254,11 +255,17 @@ struct vb2_buffer {
 	 *			all buffers queued from userspace
 	 * done_entry:		entry on the list that stores all buffers ready
 	 *			to be dequeued to userspace
+	 * in_fence:		fence receive from vb2 client to wait on before
+	 *			using the buffer (queueing to the driver)
+	 * fence_cb:		fence callback information
 	 */
 	enum vb2_buffer_state	state;
 
 	struct list_head	queued_entry;
 	struct list_head	done_entry;
+
+	struct dma_fence	*in_fence;
+	struct dma_fence_cb	fence_cb;
 #ifdef CONFIG_VIDEO_ADV_DEBUG
 	/*
 	 * Counters for how often these buffer-related ops are
@@ -726,6 +733,7 @@ int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb);
  * @index:	id number of the buffer
  * @pb:		buffer structure passed from userspace to vidioc_qbuf handler
  *		in driver
+ * @fence:	in-fence to wait on before queueing the buffer
  *
  * Should be called from vidioc_qbuf ioctl handler of a driver.
  * The passed buffer should have been verified.
@@ -740,7 +748,8 @@ int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb);
  * The return values from this function are intended to be directly returned
  * from vidioc_qbuf handler in driver.
  */
-int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb);
+int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb,
+		  struct dma_fence *fence);
 
 /**
  * vb2_core_dqbuf() - Dequeue a buffer to the userspace
-- 
2.13.5

[PATCH v3 05/15] [media] uvc: enable subscriptions to other events

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.com>

Call v4l2_ctrl_subscribe_event to subscribe to the BUF_QUEUED event as
well.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
---
 drivers/media/usb/uvc/uvc_v4l2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index 3e7e283a44a8..dfa0ccdcf849 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -1240,7 +1240,7 @@ static int uvc_ioctl_subscribe_event(struct v4l2_fh *fh,
 	case V4L2_EVENT_CTRL:
 		return v4l2_event_subscribe(fh, sub, 0, &uvc_ctrl_sub_ev_ops);
 	default:
-		return -EINVAL;
+		return v4l2_ctrl_subscribe_event(fh, sub);
 	}
 }
 
-- 
2.13.5

[PATCH v3 06/15] [media] vivid: assign the specific device to the vb2_queue->dev

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.com>

Instead of assigning the global v4l2 device, assign the specific device.
This was causing trouble when using using V4L2 events with vivid
devices. The device's queue should be the same we opened in userspace.

This is needed for the upcoming V4L2_EVENT_BUF_QUEUED support. The current
vivid code isn't wrong, it just needs to be changed so V4L2_EVENT_BUF_QUEUED
can be supported. The change doesn't affect any other behaviour of vivid.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/platform/vivid/vivid-core.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/media/platform/vivid/vivid-core.c b/drivers/media/platform/vivid/vivid-core.c
index 5f316a5e38db..608bcceed463 100644
--- a/drivers/media/platform/vivid/vivid-core.c
+++ b/drivers/media/platform/vivid/vivid-core.c
@@ -1070,7 +1070,7 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
 		q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
 		q->min_buffers_needed = 2;
 		q->lock = &dev->mutex;
-		q->dev = dev->v4l2_dev.dev;
+		q->dev = &dev->vid_cap_dev.dev;
 
 		ret = vb2_queue_init(q);
 		if (ret)
@@ -1090,7 +1090,7 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
 		q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
 		q->min_buffers_needed = 2;
 		q->lock = &dev->mutex;
-		q->dev = dev->v4l2_dev.dev;
+		q->dev = &dev->vid_out_dev.dev;
 
 		ret = vb2_queue_init(q);
 		if (ret)
@@ -1110,7 +1110,7 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
 		q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
 		q->min_buffers_needed = 2;
 		q->lock = &dev->mutex;
-		q->dev = dev->v4l2_dev.dev;
+		q->dev = &dev->vbi_cap_dev.dev;
 
 		ret = vb2_queue_init(q);
 		if (ret)
@@ -1130,7 +1130,7 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
 		q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
 		q->min_buffers_needed = 2;
 		q->lock = &dev->mutex;
-		q->dev = dev->v4l2_dev.dev;
+		q->dev = &dev->vbi_out_dev.dev;
 
 		ret = vb2_queue_init(q);
 		if (ret)
@@ -1149,7 +1149,7 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
 		q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
 		q->min_buffers_needed = 8;
 		q->lock = &dev->mutex;
-		q->dev = dev->v4l2_dev.dev;
+		q->dev = &dev->sdr_cap_dev.dev;
 
 		ret = vb2_queue_init(q);
 		if (ret)
-- 
2.13.5

[PATCH v3 07/15] [media] v4l: add V4L2_EVENT_BUF_QUEUED event

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.com>

Add a new event the userspace can subscribe to receive notifications
when a buffer is queued onto the driver. The event provides the index of
the queued buffer.

v2: - Add missing Documentation (Mauro)

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
---
 Documentation/media/uapi/v4l/vidioc-dqevent.rst | 23 +++++++++++++++++++++++
 Documentation/media/videodev2.h.rst.exceptions  |  1 +
 include/uapi/linux/videodev2.h                  | 11 +++++++++++
 3 files changed, 35 insertions(+)

diff --git a/Documentation/media/uapi/v4l/vidioc-dqevent.rst b/Documentation/media/uapi/v4l/vidioc-dqevent.rst
index fcd9c933870d..55f9dbdca6ec 100644
--- a/Documentation/media/uapi/v4l/vidioc-dqevent.rst
+++ b/Documentation/media/uapi/v4l/vidioc-dqevent.rst
@@ -78,6 +78,10 @@ call.
       - ``src_change``
       - Event data for event V4L2_EVENT_SOURCE_CHANGE.
     * -
+      - struct :c:type:`v4l2_event_buf_queued`
+      - ``buf_queued``
+      - Event data for event V4L2_EVENT_BUF_QUEUED.
+    * -
       - __u8
       - ``data``\ [64]
       - Event data. Defined by the event type. The union should be used to
@@ -337,6 +341,25 @@ call.
 	each cell in the motion detection grid, then that all cells are
 	automatically assigned to the default region 0.
 
+.. c:type:: v4l2_event_buf_queued
+
+.. flat-table:: struct v4l2_event_buf_queued
+    :header-rows:  0
+    :stub-columns: 0
+    :widths:       1 1 2
+
+    * - __u32
+      - ``index``
+      - The index of the buffer that was queued to the driver.
+    * - __s32
+      - ``out_fence_fd``
+      - The out-fence file descriptor of the buffer that was queued to
+	the driver. It will signal when the buffer is ready, or if an
+	error happens.
+
+
+
+.. tabularcolumns:: |p{4.4cm}|p{4.4cm}|p{8.7cm}|
 
 
 .. tabularcolumns:: |p{6.6cm}|p{2.2cm}|p{8.7cm}|
diff --git a/Documentation/media/videodev2.h.rst.exceptions b/Documentation/media/videodev2.h.rst.exceptions
index a5cb0a8686ac..4e014b1d0317 100644
--- a/Documentation/media/videodev2.h.rst.exceptions
+++ b/Documentation/media/videodev2.h.rst.exceptions
@@ -462,6 +462,7 @@ replace define V4L2_EVENT_CTRL event-type
 replace define V4L2_EVENT_FRAME_SYNC event-type
 replace define V4L2_EVENT_SOURCE_CHANGE event-type
 replace define V4L2_EVENT_MOTION_DET event-type
+replace define V4L2_EVENT_BUF_QUEUED event-type
 replace define V4L2_EVENT_PRIVATE_START event-type
 
 replace define V4L2_EVENT_CTRL_CH_VALUE ctrl-changes-flags
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index e5abab9a908c..e2ec0b66f490 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -2158,6 +2158,7 @@ struct v4l2_streamparm {
 #define V4L2_EVENT_FRAME_SYNC			4
 #define V4L2_EVENT_SOURCE_CHANGE		5
 #define V4L2_EVENT_MOTION_DET			6
+#define V4L2_EVENT_BUF_QUEUED			7
 #define V4L2_EVENT_PRIVATE_START		0x08000000
 
 /* Payload for V4L2_EVENT_VSYNC */
@@ -2210,6 +2211,15 @@ struct v4l2_event_motion_det {
 	__u32 region_mask;
 };
 
+/**
+ * struct v4l2_event_buf_queued - buffer queued in the driver event
+ * @index:		index of the buffer queued in the driver
+ */
+struct v4l2_event_buf_queued {
+	__u32 index;
+	__s32 out_fence_fd;
+};
+
 struct v4l2_event {
 	__u32				type;
 	union {
@@ -2218,6 +2228,7 @@ struct v4l2_event {
 		struct v4l2_event_frame_sync	frame_sync;
 		struct v4l2_event_src_change	src_change;
 		struct v4l2_event_motion_det	motion_det;
+		struct v4l2_event_buf_queued	buf_queued;
 		__u8				data[64];
 	} u;
 	__u32				pending;
-- 
2.13.5

[PATCH v3 08/15] [media] vb2: add .buffer_queued() to notify queueing in the driver

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.com>

With the upcoming explicit synchronization support to V4L2 we need a
way to notify userspace when buffers are queued to the driver - buffers
with fences attached to it can only be queued once the fence signal, so
the queueing to the driver might be deferred.

Yet, userspace still wants to be notified, so the buffer_queued() callback
was added to vb2_buf_ops for that purpose.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
---
 include/media/videobuf2-core.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index 41cda762ff0a..5ed8d3402474 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -415,6 +415,8 @@ struct vb2_ops {
  *			will return an error.
  * @copy_timestamp:	copy the timestamp from a userspace structure to
  *			the vb2_buffer struct.
+ * @buffer_queued:	VB2 uses this to notify the VB2-client that the buffer
+ *			was queued to the driver.
  */
 struct vb2_buf_ops {
 	int (*verify_planes_array)(struct vb2_buffer *vb, const void *pb);
@@ -422,6 +424,7 @@ struct vb2_buf_ops {
 	int (*fill_vb2_buffer)(struct vb2_buffer *vb, const void *pb,
 				struct vb2_plane *planes);
 	void (*copy_timestamp)(struct vb2_buffer *vb, const void *pb);
+	void (*buffer_queued)(struct vb2_buffer *vb);
 };
 
 /**
-- 
2.13.5

[PATCH v3 09/15] [media] v4l: add support to BUF_QUEUED event

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.com>

Implement the needed pieces to let userspace subscribe for
V4L2_EVENT_BUF_QUEUED events. Videobuf2 will queue the event for the
DQEVENT ioctl.

v3:	- Do not call v4l2 event API from vb2 (Mauro)

v2:	- Use VIDEO_MAX_FRAME to allocate room for events at
	v4l2_event_subscribe() (Hans)

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
---
 drivers/media/v4l2-core/v4l2-ctrls.c     |  6 +++++-
 drivers/media/v4l2-core/videobuf2-core.c |  2 ++
 drivers/media/v4l2-core/videobuf2-v4l2.c | 14 ++++++++++++++
 3 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index dd1db678718c..17d4b9e3eec6 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -3438,8 +3438,12 @@ EXPORT_SYMBOL(v4l2_ctrl_log_status);
 int v4l2_ctrl_subscribe_event(struct v4l2_fh *fh,
 				const struct v4l2_event_subscription *sub)
 {
-	if (sub->type == V4L2_EVENT_CTRL)
+	switch (sub->type) {
+	case V4L2_EVENT_CTRL:
 		return v4l2_event_subscribe(fh, sub, 0, &v4l2_ctrl_sub_ev_ops);
+	case V4L2_EVENT_BUF_QUEUED:
+		return v4l2_event_subscribe(fh, sub, VIDEO_MAX_FRAME, NULL);
+	}
 	return -EINVAL;
 }
 EXPORT_SYMBOL(v4l2_ctrl_subscribe_event);
diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index b19c1bc4b083..bbbae0eed567 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -1231,6 +1231,8 @@ static void __enqueue_in_driver(struct vb2_buffer *vb)
 	trace_vb2_buf_queue(q, vb);
 
 	call_void_vb_qop(vb, buf_queue, vb);
+
+	call_void_bufop(q, buffer_queued, vb);
 }
 
 static int __buf_prepare(struct vb2_buffer *vb, const void *pb)
diff --git a/drivers/media/v4l2-core/videobuf2-v4l2.c b/drivers/media/v4l2-core/videobuf2-v4l2.c
index 8c322cd1b346..bbfcd054e6f6 100644
--- a/drivers/media/v4l2-core/videobuf2-v4l2.c
+++ b/drivers/media/v4l2-core/videobuf2-v4l2.c
@@ -138,6 +138,19 @@ static void __copy_timestamp(struct vb2_buffer *vb, const void *pb)
 	}
 };
 
+static void __buffer_queued(struct vb2_buffer *vb)
+{
+	struct video_device *vdev = to_video_device(vb->vb2_queue->dev);
+	struct v4l2_fh *fh = vdev->queue->owner;
+	struct v4l2_event event;
+
+	memset(&event, 0, sizeof(event));
+	event.type = V4L2_EVENT_BUF_QUEUED;
+	event.u.buf_queued.index = vb->index;
+
+	v4l2_event_queue_fh(fh, &event);
+}
+
 static void vb2_warn_zero_bytesused(struct vb2_buffer *vb)
 {
 	static bool check_once;
@@ -455,6 +468,7 @@ static const struct vb2_buf_ops v4l2_buf_ops = {
 	.fill_user_buffer	= __fill_v4l2_buffer,
 	.fill_vb2_buffer	= __fill_vb2_buffer,
 	.copy_timestamp		= __copy_timestamp,
+	.buffer_queued		= __buffer_queued,
 };
 
 /**
-- 
2.13.5

[PATCH v3 10/15] [media] vb2: add 'ordered' property to queues

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.com>

For explicit synchronization (and soon for HAL3/Request API) we need
the v4l2-driver to guarantee the ordering in which the buffers were queued
by userspace. This is already true for many drivers, but we never needed
to say it.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
---
 include/media/videobuf2-core.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index 5ed8d3402474..20099dc22f26 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -508,6 +508,9 @@ struct vb2_buf_ops {
  * @last_buffer_dequeued: used in poll() and DQBUF to immediately return if the
  *		last decoded buffer was already dequeued. Set for capture queues
  *		when a buffer with the V4L2_BUF_FLAG_LAST is dequeued.
+ * @ordered: if the driver can guarantee that the queue will be ordered or not.
+ *		The default is not ordered unless the driver sets this flag. It
+ *		is mandatory for using explicit fences.
  * @fileio:	file io emulator internal data, used only if emulator is active
  * @threadio:	thread io internal data, used only if thread is active
  */
@@ -560,6 +563,7 @@ struct vb2_queue {
 	unsigned int			is_output:1;
 	unsigned int			copy_timestamp:1;
 	unsigned int			last_buffer_dequeued:1;
+	unsigned int			ordered:1;
 
 	struct vb2_fileio_data		*fileio;
 	struct vb2_threadio_data	*threadio;
-- 
2.13.5

[PATCH v3 11/15] [media] vivid: mark vivid queues as ordered

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.com>

To enable vivid to be used with explicit synchronization we need
to mark its queues as ordered. vivid queues are already ordered by
default so we no changes are needed.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/platform/vivid/vivid-core.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/media/platform/vivid/vivid-core.c b/drivers/media/platform/vivid/vivid-core.c
index 608bcceed463..239790e8ccc6 100644
--- a/drivers/media/platform/vivid/vivid-core.c
+++ b/drivers/media/platform/vivid/vivid-core.c
@@ -1063,6 +1063,7 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
 		q->type = dev->multiplanar ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE :
 			V4L2_BUF_TYPE_VIDEO_CAPTURE;
 		q->io_modes = VB2_MMAP | VB2_USERPTR | VB2_DMABUF | VB2_READ;
+		q->ordered = 1;
 		q->drv_priv = dev;
 		q->buf_struct_size = sizeof(struct vivid_buffer);
 		q->ops = &vivid_vid_cap_qops;
@@ -1083,6 +1084,7 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
 		q->type = dev->multiplanar ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE :
 			V4L2_BUF_TYPE_VIDEO_OUTPUT;
 		q->io_modes = VB2_MMAP | VB2_USERPTR | VB2_DMABUF | VB2_WRITE;
+		q->ordered = 1;
 		q->drv_priv = dev;
 		q->buf_struct_size = sizeof(struct vivid_buffer);
 		q->ops = &vivid_vid_out_qops;
@@ -1103,6 +1105,7 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
 		q->type = dev->has_raw_vbi_cap ? V4L2_BUF_TYPE_VBI_CAPTURE :
 					      V4L2_BUF_TYPE_SLICED_VBI_CAPTURE;
 		q->io_modes = VB2_MMAP | VB2_USERPTR | VB2_DMABUF | VB2_READ;
+		q->ordered = 1;
 		q->drv_priv = dev;
 		q->buf_struct_size = sizeof(struct vivid_buffer);
 		q->ops = &vivid_vbi_cap_qops;
@@ -1123,6 +1126,7 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
 		q->type = dev->has_raw_vbi_out ? V4L2_BUF_TYPE_VBI_OUTPUT :
 					      V4L2_BUF_TYPE_SLICED_VBI_OUTPUT;
 		q->io_modes = VB2_MMAP | VB2_USERPTR | VB2_DMABUF | VB2_WRITE;
+		q->ordered = 1;
 		q->drv_priv = dev;
 		q->buf_struct_size = sizeof(struct vivid_buffer);
 		q->ops = &vivid_vbi_out_qops;
@@ -1142,6 +1146,7 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
 		q = &dev->vb_sdr_cap_q;
 		q->type = V4L2_BUF_TYPE_SDR_CAPTURE;
 		q->io_modes = VB2_MMAP | VB2_USERPTR | VB2_DMABUF | VB2_READ;
+		q->ordered = 1;
 		q->drv_priv = dev;
 		q->buf_struct_size = sizeof(struct vivid_buffer);
 		q->ops = &vivid_sdr_cap_qops;
-- 
2.13.5

[PATCH v3 12/15] [media] vb2: add videobuf2 dma-buf fence helpers

[Gustavo Padovan]

From: Javier Martinez Canillas <javier@osg.samsung.com>

Add a videobuf2-fence.h header file that contains different helpers
for DMA buffer sharing explicit fence support in videobuf2.

Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
---
 include/media/videobuf2-fence.h | 49 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)
 create mode 100644 include/media/videobuf2-fence.h

diff --git a/include/media/videobuf2-fence.h b/include/media/videobuf2-fence.h
new file mode 100644
index 000000000000..ed5612ca03d6
--- /dev/null
+++ b/include/media/videobuf2-fence.h
@@ -0,0 +1,49 @@
+/*
+ * videobuf2-fence.h - DMA buffer sharing fence helpers for videobuf 2
+ *
+ * Copyright (C) 2016 Samsung Electronics
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation.
+ */
+
+#include <linux/dma-fence.h>
+#include <linux/slab.h>
+
+static DEFINE_SPINLOCK(vb2_fence_lock);
+
+static inline const char *vb2_fence_get_driver_name(struct dma_fence *fence)
+{
+	return "vb2_fence";
+}
+
+static inline const char *vb2_fence_get_timeline_name(struct dma_fence *fence)
+{
+	return "vb2_fence_timeline";
+}
+
+static inline bool vb2_fence_enable_signaling(struct dma_fence *fence)
+{
+	return true;
+}
+
+static const struct dma_fence_ops vb2_fence_ops = {
+	.get_driver_name = vb2_fence_get_driver_name,
+	.get_timeline_name = vb2_fence_get_timeline_name,
+	.enable_signaling = vb2_fence_enable_signaling,
+	.wait = dma_fence_default_wait,
+};
+
+static inline struct dma_fence *vb2_fence_alloc(void)
+{
+	struct dma_fence *vb2_fence = kzalloc(sizeof(*vb2_fence), GFP_KERNEL);
+
+	if (!vb2_fence)
+		return NULL;
+
+	dma_fence_init(vb2_fence, &vb2_fence_ops, &vb2_fence_lock,
+		       dma_fence_context_alloc(1), 1);
+
+	return vb2_fence;
+}
-- 
2.13.5

[PATCH v3 13/15] [media] vb2: add infrastructure to support out-fences

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.com>

Add vb2_setup_out_fence() and the needed members to struct vb2_buffer.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
---
 drivers/media/v4l2-core/videobuf2-core.c | 55 ++++++++++++++++++++++++++++++++
 include/media/videobuf2-core.h           | 34 ++++++++++++++++++++
 2 files changed, 89 insertions(+)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index bbbae0eed567..34adf1916194 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -23,8 +23,11 @@
 #include <linux/sched.h>
 #include <linux/freezer.h>
 #include <linux/kthread.h>
+#include <linux/sync_file.h>
+#include <linux/dma-fence.h>
 
 #include <media/videobuf2-core.h>
+#include <media/videobuf2-fence.h>
 #include <media/v4l2-mc.h>
 
 #include <trace/events/vb2.h>
@@ -1317,6 +1320,58 @@ int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb)
 }
 EXPORT_SYMBOL_GPL(vb2_core_prepare_buf);
 
+int vb2_setup_out_fence(struct vb2_queue *q)
+{
+	struct vb2_fence *fence;
+
+	fence = kzalloc(sizeof(*fence), GFP_KERNEL);
+	if (!fence)
+		return -ENOMEM;
+
+	fence->out_fence_fd = get_unused_fd_flags(O_CLOEXEC);
+	if (fence->out_fence_fd < 0) {
+		kfree(fence);
+		return fence->out_fence_fd;
+	}
+
+	fence->out_fence = vb2_fence_alloc();
+	if (!fence->out_fence)
+		goto err_fence;
+
+	fence->sync_file = sync_file_create(fence->out_fence);
+	if (!fence->sync_file) {
+		dma_fence_put(fence->out_fence);
+		goto err_fence;
+	}
+
+	spin_lock(&q->out_fence_lock);
+	list_add_tail(&fence->entry, &q->out_fence_list);
+	spin_unlock(&q->out_fence_lock);
+
+	return 0;
+
+err_fence:
+	kfree(fence);
+	put_unused_fd(fence->out_fence_fd);
+	return -ENOMEM;
+}
+EXPORT_SYMBOL_GPL(vb2_setup_out_fence);
+
+void vb2_cleanup_out_fence(struct vb2_queue *q)
+{
+	struct vb2_fence *fence;
+
+	spin_lock(&q->out_fence_lock);
+	fence = list_last_entry(&q->out_fence_list,
+				    struct vb2_fence, entry);
+	put_unused_fd(fence->out_fence_fd);
+	fput(fence->sync_file->file);
+	list_del(&fence->entry);
+	spin_unlock(&q->out_fence_lock);
+	kfree(fence);
+}
+EXPORT_SYMBOL_GPL(vb2_cleanup_out_fence);
+
 /**
  * vb2_start_streaming() - Attempt to start streaming.
  * @q:		videobuf2 queue
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index 20099dc22f26..84e5e7216a1e 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -427,6 +427,24 @@ struct vb2_buf_ops {
 	void (*buffer_queued)(struct vb2_buffer *vb);
 };
 
+/*
+ * struct vb2_fence - storage for fence data before queueing to the driver.
+ *
+ * @out_fence_fd:	the fd where to install the sync_file
+ * @out_fence:		the fence associated to the sync_file
+ * @sync_file:		the sync_file to be shared with userspace via the
+ *			out_fence_fd
+ * @files:		stores files struct for cleanup purposes
+ * @entry:		the list head element for the out_fence_list
+ */
+struct vb2_fence {
+	int out_fence_fd;
+	struct dma_fence *out_fence;
+	struct sync_file *sync_file;
+	struct files_struct *files;
+	struct list_head entry;
+};
+
 /**
  * struct vb2_queue - a videobuf queue
  *
@@ -734,6 +752,22 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
 int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb);
 
 /**
+ * vb2_setup_out_fence() - setup new out-fence
+ * @q:		The vb2_queue where to setup it
+ *
+ * Setup the file descriptor, the fence and the sync_file for the next
+ * buffer to be queued and add everything to the tail of the q->out_fence_list.
+ */
+int vb2_setup_out_fence(struct vb2_queue *q);
+
+/**
+ * vb2_cleanup_out_fence() - cleanup out-fence
+ * @q:		The vb2_queue to use for cleanup
+ *
+ * Clean up the last fence on the list. Used only when QBUF fails.
+ */
+void vb2_cleanup_out_fence(struct vb2_queue *q);
+/**
  * vb2_core_qbuf() - Queue a buffer from userspace
  *
  * @q:		videobuf2 queue
-- 
2.13.5

[PATCH v3 14/15] fs/files: export close_fd() symbol

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.com>

Rename __close_fd() to close_fd() and export it to be able close files
in modules using file descriptors.

The usecase that motivates this change happens in V4L2 where we send
events to userspace with a fd that has file installed in it. But if for
some reason we have to cancel the video stream we need to close the files
that haven't been shared with userspace yet. Thus the export of
close_fd() becomes necessary.

fd_install() happens when we call an ioctl to queue a buffer, but we only
share the fd with userspace later, and that may happen in a kernel thread
instead.

Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: linux-fsdevel@vger.kernel.org
Cc: Riley Andrews <riandrews@android.com>
Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
---
This is more like a question, I don't know how unhappy people will be with
this proposal to expose close_fd(). I'm all ears for more interesting
ways of doing it!
---
 drivers/android/binder.c | 2 +-
 fs/file.c                | 5 +++--
 fs/open.c                | 2 +-
 include/linux/fdtable.h  | 2 +-
 4 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index f7665c31feca..5a9bc73012df 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -440,7 +440,7 @@ static long task_close_fd(struct binder_proc *proc, unsigned int fd)
 	if (proc->files == NULL)
 		return -ESRCH;
 
-	retval = __close_fd(proc->files, fd);
+	retval = close_fd(proc->files, fd);
 	/* can't restart close syscall because file table entry was cleared */
 	if (unlikely(retval == -ERESTARTSYS ||
 		     retval == -ERESTARTNOINTR ||
diff --git a/fs/file.c b/fs/file.c
index 1fc7fbbb4510..111d387ac190 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -618,7 +618,7 @@ EXPORT_SYMBOL(fd_install);
 /*
  * The same warnings as for __alloc_fd()/__fd_install() apply here...
  */
-int __close_fd(struct files_struct *files, unsigned fd)
+int close_fd(struct files_struct *files, unsigned fd)
 {
 	struct file *file;
 	struct fdtable *fdt;
@@ -640,6 +640,7 @@ int __close_fd(struct files_struct *files, unsigned fd)
 	spin_unlock(&files->file_lock);
 	return -EBADF;
 }
+EXPORT_SYMBOL(close_fd);
 
 void do_close_on_exec(struct files_struct *files)
 {
@@ -856,7 +857,7 @@ int replace_fd(unsigned fd, struct file *file, unsigned flags)
 	struct files_struct *files = current->files;
 
 	if (!file)
-		return __close_fd(files, fd);
+		return close_fd(files, fd);
 
 	if (fd >= rlimit(RLIMIT_NOFILE))
 		return -EBADF;
diff --git a/fs/open.c b/fs/open.c
index 35bb784763a4..30907d967443 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -1152,7 +1152,7 @@ EXPORT_SYMBOL(filp_close);
  */
 SYSCALL_DEFINE1(close, unsigned int, fd)
 {
-	int retval = __close_fd(current->files, fd);
+	int retval = close_fd(current->files, fd);
 
 	/* can't restart close syscall because file table entry was cleared */
 	if (unlikely(retval == -ERESTARTSYS ||
diff --git a/include/linux/fdtable.h b/include/linux/fdtable.h
index 6e84b2cae6ad..511fd38d5e4b 100644
--- a/include/linux/fdtable.h
+++ b/include/linux/fdtable.h
@@ -115,7 +115,7 @@ extern int __alloc_fd(struct files_struct *files,
 		      unsigned start, unsigned end, unsigned flags);
 extern void __fd_install(struct files_struct *files,
 		      unsigned int fd, struct file *file);
-extern int __close_fd(struct files_struct *files,
+extern int close_fd(struct files_struct *files,
 		      unsigned int fd);
 
 extern struct kmem_cache *files_cachep;
-- 
2.13.5

[PATCH v3 15/15] [media] vb2: add out-fence support to QBUF

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.com>

If V4L2_BUF_FLAG_OUT_FENCE flag is present on the QBUF call we create
an out_fence and sent to userspace on the V4L2_EVENT_BUF_QUEUED when
the buffer is queued to the driver.

The out fence fd returned references the next buffer to be queued to the
driver and not the buffer in the actual QBUF call. So there a list in
videobuf2 core to keep track of the sync_file and fence created and assign
them to buffers when they are queued to the V4L2 driver.

The fence is signaled on buffer_done(), when the job on the buffer is
finished.

v4:
	- return the out_fence_fd in the BUF_QUEUED event(Hans)

v3:	- add WARN_ON_ONCE(q->ordered) on requeueing (Hans)
	- set the OUT_FENCE flag if there is a fence pending (Hans)
	- call fd_install() after vb2_core_qbuf() (Hans)
	- clean up fence if vb2_core_qbuf() fails (Hans)
	- add list to store sync_file and fence for the next queued buffer

v2: check if the queue is ordered.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
---
 drivers/media/v4l2-core/videobuf2-core.c | 51 ++++++++++++++++++++++++++++++++
 drivers/media/v4l2-core/videobuf2-v4l2.c | 24 ++++++++++++++-
 include/media/videobuf2-core.h           | 11 +++++++
 3 files changed, 85 insertions(+), 1 deletion(-)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 34adf1916194..ab58170776bc 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -25,6 +25,7 @@
 #include <linux/kthread.h>
 #include <linux/sync_file.h>
 #include <linux/dma-fence.h>
+#include <linux/fdtable.h>
 
 #include <media/videobuf2-core.h>
 #include <media/videobuf2-fence.h>
@@ -353,6 +354,7 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory,
 			vb->planes[plane].length = plane_sizes[plane];
 			vb->planes[plane].min_length = plane_sizes[plane];
 		}
+		vb->out_fence_fd = -1;
 		q->bufs[vb->index] = vb;
 
 		/* Allocate video buffer memory for the MMAP type */
@@ -933,10 +935,24 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state)
 	case VB2_BUF_STATE_QUEUED:
 		return;
 	case VB2_BUF_STATE_REQUEUEING:
+		/*
+		 * Explicit synchronization requires ordered queues for now,
+		 * so WARN_ON if we are requeuing on an ordered queue.
+		 */
+		if (vb->out_fence)
+			WARN_ON_ONCE(q->ordered);
+
 		if (q->start_streaming_called)
 			__enqueue_in_driver(vb);
 		return;
 	default:
+		if (state == VB2_BUF_STATE_ERROR)
+			dma_fence_set_error(vb->out_fence, -ENOENT);
+		dma_fence_signal(vb->out_fence);
+		dma_fence_put(vb->out_fence);
+		vb->out_fence = NULL;
+		vb->out_fence_fd = -1;
+
 		/* Inform any processes that may be waiting for buffers */
 		wake_up(&q->done_wq);
 		break;
@@ -1224,10 +1240,21 @@ static int __prepare_dmabuf(struct vb2_buffer *vb, const void *pb)
 static void __enqueue_in_driver(struct vb2_buffer *vb)
 {
 	struct vb2_queue *q = vb->vb2_queue;
+	struct vb2_fence *fence;
 
 	if (vb->in_fence && !dma_fence_is_signaled(vb->in_fence))
 		return;
 
+	spin_lock(&q->out_fence_lock);
+	fence = list_first_entry(&q->out_fence_list, struct vb2_fence, entry);
+	if (fence) {
+		vb->out_fence = dma_fence_get(fence->out_fence);
+		vb->out_fence_fd = fence->out_fence_fd;
+		list_del(&fence->entry);
+		kfree(fence);
+	}
+	spin_unlock(&q->out_fence_lock);
+
 	vb->state = VB2_BUF_STATE_ACTIVE;
 	atomic_inc(&q->owned_by_drv_count);
 
@@ -1348,6 +1375,8 @@ int vb2_setup_out_fence(struct vb2_queue *q)
 	list_add_tail(&fence->entry, &q->out_fence_list);
 	spin_unlock(&q->out_fence_lock);
 
+	fence->files = current->files;
+
 	return 0;
 
 err_fence:
@@ -1450,6 +1479,7 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb,
 		  struct dma_fence *fence)
 {
 	struct vb2_buffer *vb;
+	struct vb2_fence *vb2_fence;
 	int ret;
 
 	vb = q->bufs[index];
@@ -1513,6 +1543,15 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb,
 			goto err;
 	}
 
+	spin_lock(&q->out_fence_lock);
+	vb2_fence = list_last_entry(&q->out_fence_list, struct vb2_fence,
+				     entry);
+	spin_unlock(&q->out_fence_lock);
+	if (vb2_fence)
+		fd_install(vb2_fence->out_fence_fd,
+			   vb2_fence->sync_file->file);
+
+
 	/*
 	 * For explicit synchronization: If the fence didn't signal
 	 * yet we setup a callback to queue the buffer once the fence
@@ -1760,6 +1799,7 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
 {
 	unsigned int i;
 	struct vb2_buffer *vb;
+	struct vb2_fence *fence, *tmp;
 
 	/*
 	 * Tell driver to stop all transactions and release all queued
@@ -1790,6 +1830,14 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
 		}
 	}
 
+	spin_lock(&q->out_fence_lock);
+	list_for_each_entry_safe(fence, tmp, &q->out_fence_list, entry) {
+		close_fd(fence->files, fence->out_fence_fd);
+		list_del(&fence->entry);
+		kfree(fence);
+	}
+	spin_unlock(&q->out_fence_lock);
+
 	q->streaming = 0;
 	q->start_streaming_called = 0;
 	q->queued_count = 0;
@@ -1804,6 +1852,7 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
 	 * has not already dequeued before initiating cancel.
 	 */
 	INIT_LIST_HEAD(&q->done_list);
+	INIT_LIST_HEAD(&q->out_fence_list);
 	atomic_set(&q->owned_by_drv_count, 0);
 	wake_up_all(&q->done_wq);
 
@@ -2125,6 +2174,8 @@ int vb2_core_queue_init(struct vb2_queue *q)
 	INIT_LIST_HEAD(&q->queued_list);
 	INIT_LIST_HEAD(&q->done_list);
 	spin_lock_init(&q->done_lock);
+	INIT_LIST_HEAD(&q->out_fence_list);
+	spin_lock_init(&q->out_fence_lock);
 	mutex_init(&q->mmap_lock);
 	init_waitqueue_head(&q->done_wq);
 
diff --git a/drivers/media/v4l2-core/videobuf2-v4l2.c b/drivers/media/v4l2-core/videobuf2-v4l2.c
index bbfcd054e6f6..e1ca48ab5005 100644
--- a/drivers/media/v4l2-core/videobuf2-v4l2.c
+++ b/drivers/media/v4l2-core/videobuf2-v4l2.c
@@ -147,6 +147,7 @@ static void __buffer_queued(struct vb2_buffer *vb)
 	memset(&event, 0, sizeof(event));
 	event.type = V4L2_EVENT_BUF_QUEUED;
 	event.u.buf_queued.index = vb->index;
+	event.u.buf_queued.out_fence_fd = vb->out_fence_fd;
 
 	v4l2_event_queue_fh(fh, &event);
 }
@@ -197,6 +198,12 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b,
 		return -EINVAL;
 	}
 
+	if (!q->ordered && (b->flags & V4L2_BUF_FLAG_OUT_FENCE)) {
+		dprintk(1, "%s: out-fence doesn't work on unordered queues\n",
+			opname);
+		return -EINVAL;
+	}
+
 	return __verify_planes_array(q->bufs[b->index], b);
 }
 
@@ -225,6 +232,8 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, void *pb)
 	b->reserved = 0;
 
 	b->fence_fd = -1;
+	if (vb->out_fence_fd)
+		b->flags |= V4L2_BUF_FLAG_OUT_FENCE;
 	if (vb->in_fence)
 		b->flags |= V4L2_BUF_FLAG_IN_FENCE;
 	else
@@ -605,7 +614,20 @@ int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
 		}
 	}
 
-	return vb2_core_qbuf(q, b->index, b, fence);
+	if (b->flags & V4L2_BUF_FLAG_OUT_FENCE) {
+		ret = vb2_setup_out_fence(q);
+		if (ret) {
+			dprintk(1, "failed to set up out-fence\n");
+			dma_fence_put(fence);
+			return ret;
+		}
+	}
+
+	ret = vb2_core_qbuf(q, b->index, b, fence);
+	if (ret && (b->flags & V4L2_BUF_FLAG_OUT_FENCE))
+		vb2_cleanup_out_fence(q);
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(vb2_qbuf);
 
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index 84e5e7216a1e..9ad774f796bb 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -258,6 +258,9 @@ struct vb2_buffer {
 	 * in_fence:		fence receive from vb2 client to wait on before
 	 *			using the buffer (queueing to the driver)
 	 * fence_cb:		fence callback information
+	 * out_fence:		the out-fence associated with the buffer once
+	 *			it is queued to the driver.
+	 * out_fence_fd:	the out_fence_fd to be shared with userspace.
 	 */
 	enum vb2_buffer_state	state;
 
@@ -266,6 +269,9 @@ struct vb2_buffer {
 
 	struct dma_fence	*in_fence;
 	struct dma_fence_cb	fence_cb;
+
+	struct dma_fence	*out_fence;
+	int			out_fence_fd;
 #ifdef CONFIG_VIDEO_ADV_DEBUG
 	/*
 	 * Counters for how often these buffer-related ops are
@@ -512,6 +518,8 @@ struct vb2_fence {
  * @done_list:	list of buffers ready to be dequeued to userspace
  * @done_lock:	lock to protect done_list list
  * @done_wq:	waitqueue for processes waiting for buffers ready to be dequeued
+ * @out_fence_list: list of out fences waiting to be assigned to a buffer
+ * @out_fence_lock: lock to protect out_fence_list
  * @alloc_devs:	memory type/allocator-specific per-plane device
  * @streaming:	current streaming state
  * @start_streaming_called: @start_streaming was called successfully and we
@@ -571,6 +579,9 @@ struct vb2_queue {
 	spinlock_t			done_lock;
 	wait_queue_head_t		done_wq;
 
+	struct list_head		out_fence_list;
+	spinlock_t			out_fence_lock;
+
 	struct device			*alloc_devs[VB2_MAX_PLANES];
 
 	unsigned int			streaming:1;
-- 
2.13.5

Re: [PATCH v3 14/15] fs/files: export close_fd() symbol

[Eric Biggers]

On Thu, Sep 07, 2017 at 03:42:25PM -0300, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.com>
> 
> Rename __close_fd() to close_fd() and export it to be able close files
> in modules using file descriptors.
> 
> The usecase that motivates this change happens in V4L2 where we send
> events to userspace with a fd that has file installed in it. But if for
> some reason we have to cancel the video stream we need to close the files
> that haven't been shared with userspace yet. Thus the export of
> close_fd() becomes necessary.
> 
> fd_install() happens when we call an ioctl to queue a buffer, but we only
> share the fd with userspace later, and that may happen in a kernel thread
> instead.

What do you mean?  A file descriptor is shared with userspace as soon as it's
installed in the fdtable by fd_install().  As soon as it's there, another thread
can use it (or close it, duplicate it, etc.), even before the syscall that
installed it returns...

Eric

Re: [PATCH v3 14/15] fs/files: export close_fd() symbol

[Al Viro]

On Thu, Sep 07, 2017 at 03:42:25PM -0300, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.com>
> 
> Rename __close_fd() to close_fd() and export it to be able close files
> in modules using file descriptors.
> 
> The usecase that motivates this change happens in V4L2 where we send
> events to userspace with a fd that has file installed in it. But if for
> some reason we have to cancel the video stream we need to close the files
> that haven't been shared with userspace yet. Thus the export of
> close_fd() becomes necessary.
> 
> fd_install() happens when we call an ioctl to queue a buffer, but we only
> share the fd with userspace later, and that may happen in a kernel thread
> instead.

NAK.  As soon as the reference is in descriptor table, you *can't* do anything
to it.  This "sharing" part is complete BS - being _told_ that descriptor is
there does not matter at all.  That descriptor might be hit with dup2() as
soon as fd_install() has happened.  Or be closed, or any number of other things.

You can not take it back.  Once fd_install() is done, it's fucking done, period.
If V4L2 requires removing it from descriptor table, it's a shitty API and needs
to be fixed.

Again, NAK.

Re: [PATCH v3 14/15] fs/files: export close_fd() symbol

[Gustavo Padovan]

2017-09-07 Al Viro <viro@ZenIV.linux.org.uk>:

> On Thu, Sep 07, 2017 at 03:42:25PM -0300, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo.padovan@collabora.com>
> > 
> > Rename __close_fd() to close_fd() and export it to be able close files
> > in modules using file descriptors.
> > 
> > The usecase that motivates this change happens in V4L2 where we send
> > events to userspace with a fd that has file installed in it. But if for
> > some reason we have to cancel the video stream we need to close the files
> > that haven't been shared with userspace yet. Thus the export of
> > close_fd() becomes necessary.
> > 
> > fd_install() happens when we call an ioctl to queue a buffer, but we only
> > share the fd with userspace later, and that may happen in a kernel thread
> > instead.
> 
> NAK.  As soon as the reference is in descriptor table, you *can't* do anything
> to it.  This "sharing" part is complete BS - being _told_ that descriptor is
> there does not matter at all.  That descriptor might be hit with dup2() as
> soon as fd_install() has happened.  Or be closed, or any number of other things.
> 
> You can not take it back.  Once fd_install() is done, it's fucking done, period.
> If V4L2 requires removing it from descriptor table, it's a shitty API and needs
> to be fixed.

Sorry for my lack of knowledge here and thank you for the explanation,
things are a lot clear to me. For some reasons I were trying to delay
the sharing of the fd to a event later. I can delay the install of it
but that my require __fd_install() to be available and exportedi as it
may happen in a thread, but I believe you wouldn't be okay with that either,
is that so?

Gustavo

Re: [PATCH v3 14/15] fs/files: export close_fd() symbol

[Al Viro]

On Thu, Sep 07, 2017 at 06:22:45PM -0300, Gustavo Padovan wrote:

> Sorry for my lack of knowledge here and thank you for the explanation,
> things are a lot clear to me. For some reasons I were trying to delay
> the sharing of the fd to a event later. I can delay the install of it
> but that my require __fd_install() to be available and exportedi as it
> may happen in a thread, but I believe you wouldn't be okay with that either,
> is that so?

Only if it has been given a reference to descriptor table to start with.
Which reference should've been acquired by the target process itself.

Why bother, anyway?  You need to handle the case when the stream has
ended just after you'd copied the value to userland; at that point you
obviously can't go hunting for all references to struct file in question,
so you have to guaratee that methods will start giving an error from
that point on.  What's the problem with just leaving it installed?

Both userland and kernel must cope with that sort of thing anyway, so
what does removing it from descriptor table and not reporting it buy
you?  AFAICS, it's an extra layer of complexity for no good reason -
you are not getting it offset by simplifications anywhere else...

Re: [PATCH v3 14/15] fs/files: export close_fd() symbol

[Hans Verkuil]

On 09/07/2017 08:42 PM, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.com>
> 
> Rename __close_fd() to close_fd() and export it to be able close files
> in modules using file descriptors.
> 
> The usecase that motivates this change happens in V4L2 where we send
> events to userspace with a fd that has file installed in it. But if for
> some reason we have to cancel the video stream we need to close the files
> that haven't been shared with userspace yet. Thus the export of
> close_fd() becomes necessary.
> 
> fd_install() happens when we call an ioctl to queue a buffer, but we only
> share the fd with userspace later, and that may happen in a kernel thread
> instead.

This isn't the way to do this.

You should only create the out fence file descriptor when userspace dequeues
(i.e. calls VIDIOC_DQEVENT) the BUF_QUEUED event. That's when you give it to
userspace and at that moment closing the fd is the responsibility of userspace.
There is no point creating it earlier anyway since userspace can't get to it
until it dequeues the event.

It does mean some more work in the V4L2 core since you need to hook into the
DQEVENT code in order to do this.

Regards,

	Hans

> 
> Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> Cc: linux-fsdevel@vger.kernel.org
> Cc: Riley Andrews <riandrews@android.com>
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
> ---
> This is more like a question, I don't know how unhappy people will be with
> this proposal to expose close_fd(). I'm all ears for more interesting
> ways of doing it!
> ---
>  drivers/android/binder.c | 2 +-
>  fs/file.c                | 5 +++--
>  fs/open.c                | 2 +-
>  include/linux/fdtable.h  | 2 +-
>  4 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index f7665c31feca..5a9bc73012df 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -440,7 +440,7 @@ static long task_close_fd(struct binder_proc *proc, unsigned int fd)
>  	if (proc->files == NULL)
>  		return -ESRCH;
>  
> -	retval = __close_fd(proc->files, fd);
> +	retval = close_fd(proc->files, fd);
>  	/* can't restart close syscall because file table entry was cleared */
>  	if (unlikely(retval == -ERESTARTSYS ||
>  		     retval == -ERESTARTNOINTR ||
> diff --git a/fs/file.c b/fs/file.c
> index 1fc7fbbb4510..111d387ac190 100644
> --- a/fs/file.c
> +++ b/fs/file.c
> @@ -618,7 +618,7 @@ EXPORT_SYMBOL(fd_install);
>  /*
>   * The same warnings as for __alloc_fd()/__fd_install() apply here...
>   */
> -int __close_fd(struct files_struct *files, unsigned fd)
> +int close_fd(struct files_struct *files, unsigned fd)
>  {
>  	struct file *file;
>  	struct fdtable *fdt;
> @@ -640,6 +640,7 @@ int __close_fd(struct files_struct *files, unsigned fd)
>  	spin_unlock(&files->file_lock);
>  	return -EBADF;
>  }
> +EXPORT_SYMBOL(close_fd);
>  
>  void do_close_on_exec(struct files_struct *files)
>  {
> @@ -856,7 +857,7 @@ int replace_fd(unsigned fd, struct file *file, unsigned flags)
>  	struct files_struct *files = current->files;
>  
>  	if (!file)
> -		return __close_fd(files, fd);
> +		return close_fd(files, fd);
>  
>  	if (fd >= rlimit(RLIMIT_NOFILE))
>  		return -EBADF;
> diff --git a/fs/open.c b/fs/open.c
> index 35bb784763a4..30907d967443 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -1152,7 +1152,7 @@ EXPORT_SYMBOL(filp_close);
>   */
>  SYSCALL_DEFINE1(close, unsigned int, fd)
>  {
> -	int retval = __close_fd(current->files, fd);
> +	int retval = close_fd(current->files, fd);
>  
>  	/* can't restart close syscall because file table entry was cleared */
>  	if (unlikely(retval == -ERESTARTSYS ||
> diff --git a/include/linux/fdtable.h b/include/linux/fdtable.h
> index 6e84b2cae6ad..511fd38d5e4b 100644
> --- a/include/linux/fdtable.h
> +++ b/include/linux/fdtable.h
> @@ -115,7 +115,7 @@ extern int __alloc_fd(struct files_struct *files,
>  		      unsigned start, unsigned end, unsigned flags);
>  extern void __fd_install(struct files_struct *files,
>  		      unsigned int fd, struct file *file);
> -extern int __close_fd(struct files_struct *files,
> +extern int close_fd(struct files_struct *files,
>  		      unsigned int fd);
>  
>  extern struct kmem_cache *files_cachep;
> 

Re: [PATCH v3 14/15] fs/files: export close_fd() symbol

[Gustavo Padovan]

On Fri, 2017-09-08 at 00:09 +0200, Hans Verkuil wrote:
> On 09/07/2017 08:42 PM, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo.padovan@collabora.com>
> > 
> > Rename __close_fd() to close_fd() and export it to be able close
> > files
> > in modules using file descriptors.
> > 
> > The usecase that motivates this change happens in V4L2 where we
> > send
> > events to userspace with a fd that has file installed in it. But if
> > for
> > some reason we have to cancel the video stream we need to close the
> > files
> > that haven't been shared with userspace yet. Thus the export of
> > close_fd() becomes necessary.
> > 
> > fd_install() happens when we call an ioctl to queue a buffer, but
> > we only
> > share the fd with userspace later, and that may happen in a kernel
> > thread
> > instead.
> 
> This isn't the way to do this.
> 
> You should only create the out fence file descriptor when userspace
> dequeues
> (i.e. calls VIDIOC_DQEVENT) the BUF_QUEUED event. That's when you
> give it to
> userspace and at that moment closing the fd is the responsibility of
> userspace.
> There is no point creating it earlier anyway since userspace can't
> get to it
> until it dequeues the event.
> 
> It does mean some more work in the V4L2 core since you need to hook
> into the
> DQEVENT code in order to do this.

Right, that makes a lot more sense. I'll change the implementation so
it can reflecting that. Thanks.

Gustavo

Re: [PATCH v3 01/15] [media] v4l: Document explicit synchronization behaviour

[Nicolas Dufresne]

Le jeudi 07 septembre 2017 à 15:42 -0300, Gustavo Padovan a écrit :
> From: Gustavo Padovan <gustavo.padovan@collabora.com>
> 
> Add section to VIDIOC_QBUF about it
> 
> v2:
> 	- mention that fences are files (Hans)
> 	- rework for the new API
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
> ---
>  Documentation/media/uapi/v4l/vidioc-qbuf.rst | 31 ++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/Documentation/media/uapi/v4l/vidioc-qbuf.rst b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
> index 1f3612637200..fae0b1431672 100644
> --- a/Documentation/media/uapi/v4l/vidioc-qbuf.rst
> +++ b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
> @@ -117,6 +117,37 @@ 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 them to signal before using the buffer, i.e., queueing
> +it to the driver. 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, that is the buffer is ready. The fence are represented
> +by file and passed as file descriptor to userspace.

I think the API works to deliver the fence FD userspace, though for the
userspace I maintain (GStreamer) it's often the case that the buffer is
unusable without the associated timestamp.

Let's consider the capture to display case (V4L2 -> DRM). As soon as
you add audio capture to the loop, GStreamer will need to start dealing
with synchronization. We can't just blindly give that buffer to the
display, we need to make sure this buffer makes it on time, in a way
that it is synchronized with the audio. To deal with synchronization,
we need to be able to correlate the video image capture time with the
audio capture time.

The problem is that this timestamp is only delivered when DQBUF
succeed, which is after the fence has unblocked. This makes the fences
completely unusable for that purpose. In general, to achieve very low
latency and still have synchronization, we'll probably need the
timestamp to be delivered somehow before the image transfer have
complete. Obviously, this is only possible if we have timestamp with
flag V4L2_BUF_FLAG_TSTAMP_SRC_SOE.

On another note, for m2m drivers that behave as color converters and
scalers, with ordered queues, userspace knows the timestamp already, so
 using the proposed API and passing the buffer immediately with it's
fence is really going to help.

For encoded (compressed) data, similar issues will be found with the
bytesused member of struct v4l2_buffer. We'll need to know the size of
the encoded data before we can pass it to another driver. I'm not sure
how relevant fences are for this type of data.

> +
> +The in-fences are communicated to the kernel at the ``VIDIOC_QBUF`` ioctl
> +using the ``V4L2_BUF_FLAG_IN_FENCE`` buffer
> +flags 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. Failure to set both will
> +cause ``VIDIOC_QBUF`` to return with error.
> +
> +To get a out-fence back from V4L2 the ``V4L2_BUF_FLAG_OUT_FENCE`` flag should
> +be set to notify it that the next queued buffer should have a fence attached to
> +it. That means the out-fence may not be associated with the buffer in the
> +current ``VIDIOC_QBUF`` ioctl call because the ordering in which videobuf2 core
> +queues the buffers to the drivers can't be guaranteed. To become aware of the
> +of the next queued buffer and the out-fence attached to it the

"of the" is repeated twice.

> +``V4L2_EVENT_BUF_QUEUED`` event should be used. It will trigger an event
> +for every buffer queued to the V4L2 driver.
> +
> +At streamoff the out-fences will either signal normally if the drivers wait
> +for the operations on the buffers to finish or signal with error if the
> +driver cancel the pending operations.
>  
>  Return Value
>  ============

Re: [PATCH v3 01/15] [media] v4l: Document explicit synchronization behaviour

[Hans Verkuil]

On 09/07/2017 08:42 PM, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.com>
> 
> Add section to VIDIOC_QBUF about it
> 
> v2:
> 	- mention that fences are files (Hans)
> 	- rework for the new API
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
> ---
>  Documentation/media/uapi/v4l/vidioc-qbuf.rst | 31 ++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/Documentation/media/uapi/v4l/vidioc-qbuf.rst b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
> index 1f3612637200..fae0b1431672 100644
> --- a/Documentation/media/uapi/v4l/vidioc-qbuf.rst
> +++ b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
> @@ -117,6 +117,37 @@ 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 them to signal before using the buffer, i.e., queueing

wait them -> wait on them

(do you wait 'on' a fence or 'for' a fence? I think it's 'on' but I'm not 100% sure)

> +it to the driver. 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

Start a new sentence here: ...drivers. Out-fences...

> +finished with buffer, that is the buffer is ready. The fence are represented

s/that is/i.e/

s/The fence/The fences/

> +by file and passed as file descriptor to userspace.

s/by file/as a file/
s/as file/as a file/

> +
> +The in-fences are communicated to the kernel at the ``VIDIOC_QBUF`` ioctl
> +using the ``V4L2_BUF_FLAG_IN_FENCE`` buffer
> +flags 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. Failure to set both will

s/Failure to set both/Setting one but not the other/

> +cause ``VIDIOC_QBUF`` to return with error.
> +
> +To get a out-fence back from V4L2 the ``V4L2_BUF_FLAG_OUT_FENCE`` flag should
> +be set to notify it that the next queued buffer should have a fence attached to
> +it. That means the out-fence may not be associated with the buffer in the
> +current ``VIDIOC_QBUF`` ioctl call because the ordering in which videobuf2 core
> +queues the buffers to the drivers can't be guaranteed. To become aware of the
> +of the next queued buffer and the out-fence attached to it the
> +``V4L2_EVENT_BUF_QUEUED`` event should be used. It will trigger an event
> +for every buffer queued to the V4L2 driver.

This makes no sense.

Setting this flag means IMHO that when *this* buffer is queued up to the driver,
then it should send the BUF_QUEUED event with an out fence.

I.e. it signals that userspace wants to have the out-fence. The requirement w.r.t.
ordering is that the BUF_QUEUED events have to be in order, but that is something
that the driver can ensure in the case it is doing internal re-ordering.

This requirement is something that needs to be documented here, BTW.

Anyway, the flag shouldn't refer to some 'next buffer', since that's very confusing.

> +
> +At streamoff the out-fences will either signal normally if the drivers wait

s/drivers wait/driver waits/

> +for the operations on the buffers to finish or signal with error if the
> +driver cancel the pending operations.

s/cancel/cancels/

Thinking with my evil hat on:

What happens if the application dequeues the buffer (VIDIOC_DQBUF) before
dequeuing the BUF_QUEUED event? Or if the application doesn't call VIDIOC_DQEVENT
at all? Should any pending BUF_QUEUED event with an out fence be removed from the
event queue if the application calls DQBUF on the corresponding buffer?

Regards,

	Hans

>  
>  Return Value
>  ============
> 

Re: [PATCH v3 02/15] [media] vb2: add explicit fence user API

[Hans Verkuil]

On 09/07/2017 08:42 PM, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.com>
> 
> Turn the reserved2 field into fence_fd that we will use to send
> an in-fence to the kernel and return an out-fence from the kernel to
> userspace.
> 
> Two new flags were added, V4L2_BUF_FLAG_IN_FENCE, that should be used
> when sending a fence to the kernel to be waited on, and
> V4L2_BUF_FLAG_OUT_FENCE, to ask the kernel to give back an out-fence.
> 
> v2: add documentation
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
> ---
>  Documentation/media/uapi/v4l/buffer.rst       | 19 +++++++++++++++++++
>  drivers/media/usb/cpia2/cpia2_v4l.c           |  2 +-
>  drivers/media/v4l2-core/v4l2-compat-ioctl32.c |  4 ++--
>  drivers/media/v4l2-core/videobuf2-v4l2.c      |  2 +-
>  include/uapi/linux/videodev2.h                |  4 +++-
>  5 files changed, 26 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/media/uapi/v4l/buffer.rst b/Documentation/media/uapi/v4l/buffer.rst
> index ae6ee73f151c..664507ad06c6 100644
> --- a/Documentation/media/uapi/v4l/buffer.rst
> +++ b/Documentation/media/uapi/v4l/buffer.rst
> @@ -648,6 +648,25 @@ Buffer Flags
>        - Start Of Exposure. The buffer timestamp has been taken when the
>  	exposure of the frame has begun. This is only valid for the
>  	``V4L2_BUF_TYPE_VIDEO_CAPTURE`` buffer type.
> +    * .. _`V4L2-BUF-FLAG-IN-FENCE`:
> +
> +      - ``V4L2_BUF_FLAG_IN_FENCE``
> +      - 0x00200000
> +      - Ask V4L2 to wait on fence passed in ``fence_fd`` field. The buffer
> +	won't be queued to the driver until the fence signals.
> +
> +    * .. _`V4L2-BUF-FLAG-OUT-FENCE`:
> +
> +      - ``V4L2_BUF_FLAG_OUT_FENCE``
> +      - 0x00400000
> +      - Request a fence for the next buffer to be queued to V4L2 driver.
> +	The fence received back through the ``fence_fd`` field  doesn't
> +	necessarily relate to the current buffer in the
> +	:ref:`VIDIOC_QBUF <VIDIOC_QBUF>` ioctl. Although, most of the time
> +	the fence will relate to the current buffer it can't be guaranteed.
> +	So to tell userspace which buffer is associated to the out_fence,
> +	one should listen for the ``V4L2_EVENT_BUF_QUEUED`` event that
> +	provide the id of the buffer when it is queued to the V4L2 driver.

As mentioned in the previous patch, this flag should just signal that userspace
wants to receive a BUF_QUEUED event with the out-fence for this buffer. It's up
to the driver to ensure the correct ordering of the events.

I'm missing documentation for the new fence_fd field.

It should mention that fence_fd is ignored if the IN_FENCE isn't set. Applications
will set fence_fd (aka reserved2) to 0, which should be fine as long as IN_FENCE
isn't set.

>  
>  
>  
> diff --git a/drivers/media/usb/cpia2/cpia2_v4l.c b/drivers/media/usb/cpia2/cpia2_v4l.c
> index 3dedd83f0b19..6cde686bf44c 100644
> --- a/drivers/media/usb/cpia2/cpia2_v4l.c
> +++ b/drivers/media/usb/cpia2/cpia2_v4l.c
> @@ -948,7 +948,7 @@ static int cpia2_dqbuf(struct file *file, void *fh, struct v4l2_buffer *buf)
>  	buf->sequence = cam->buffers[buf->index].seq;
>  	buf->m.offset = cam->buffers[buf->index].data - cam->frame_buffer;
>  	buf->length = cam->frame_size;
> -	buf->reserved2 = 0;
> +	buf->fence_fd = -1;
>  	buf->reserved = 0;
>  	memset(&buf->timecode, 0, sizeof(buf->timecode));
>  
> diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> index 821f2aa299ae..d624fb5df130 100644
> --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> @@ -370,7 +370,7 @@ struct v4l2_buffer32 {
>  		__s32		fd;
>  	} m;
>  	__u32			length;
> -	__u32			reserved2;
> +	__s32			fence_fd;
>  	__u32			reserved;
>  };
>  
> @@ -533,8 +533,8 @@ static int put_v4l2_buffer32(struct v4l2_buffer *kp, struct v4l2_buffer32 __user
>  		put_user(kp->timestamp.tv_usec, &up->timestamp.tv_usec) ||
>  		copy_to_user(&up->timecode, &kp->timecode, sizeof(struct v4l2_timecode)) ||
>  		put_user(kp->sequence, &up->sequence) ||
> -		put_user(kp->reserved2, &up->reserved2) ||
>  		put_user(kp->reserved, &up->reserved) ||
> +		put_user(kp->fence_fd, &up->fence_fd) ||

Could you move this up one line? It's easier to parse this diff if it is clear
that fence_fd replaced reserved2.

>  		put_user(kp->length, &up->length))
>  			return -EFAULT;
>  
> diff --git a/drivers/media/v4l2-core/videobuf2-v4l2.c b/drivers/media/v4l2-core/videobuf2-v4l2.c
> index 0c0669976bdc..110fb45fef6f 100644
> --- a/drivers/media/v4l2-core/videobuf2-v4l2.c
> +++ b/drivers/media/v4l2-core/videobuf2-v4l2.c
> @@ -203,7 +203,7 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, void *pb)
>  	b->timestamp = ns_to_timeval(vb->timestamp);
>  	b->timecode = vbuf->timecode;
>  	b->sequence = vbuf->sequence;
> -	b->reserved2 = 0;
> +	b->fence_fd = -1;
>  	b->reserved = 0;
>  
>  	if (q->is_multiplanar) {
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 185d6a0acc06..e5abab9a908c 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -924,7 +924,7 @@ struct v4l2_buffer {
>  		__s32		fd;
>  	} m;
>  	__u32			length;
> -	__u32			reserved2;
> +	__s32			fence_fd;
>  	__u32			reserved;
>  };
>  
> @@ -961,6 +961,8 @@ struct v4l2_buffer {
>  #define V4L2_BUF_FLAG_TSTAMP_SRC_SOE		0x00010000
>  /* mem2mem encoder/decoder */
>  #define V4L2_BUF_FLAG_LAST			0x00100000
> +#define V4L2_BUF_FLAG_IN_FENCE			0x00200000
> +#define V4L2_BUF_FLAG_OUT_FENCE			0x00400000
>  
>  /**
>   * struct v4l2_exportbuffer - export of video buffer as DMABUF file descriptor
> 

Regards,

	Hans

Re: [PATCH v3 01/15] [media] v4l: Document explicit synchronization behaviour

[Hans Verkuil]

On 09/11/2017 12:50 PM, Hans Verkuil wrote:
> On 09/07/2017 08:42 PM, Gustavo Padovan wrote:
>> From: Gustavo Padovan <gustavo.padovan@collabora.com>
>>
>> Add section to VIDIOC_QBUF about it
>>
>> v2:
>> 	- mention that fences are files (Hans)
>> 	- rework for the new API
>>
>> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
>> ---
>>  Documentation/media/uapi/v4l/vidioc-qbuf.rst | 31 ++++++++++++++++++++++++++++
>>  1 file changed, 31 insertions(+)
>>
>> diff --git a/Documentation/media/uapi/v4l/vidioc-qbuf.rst b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
>> index 1f3612637200..fae0b1431672 100644
>> --- a/Documentation/media/uapi/v4l/vidioc-qbuf.rst
>> +++ b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
>> @@ -117,6 +117,37 @@ 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 them to signal before using the buffer, i.e., queueing
> 
> wait them -> wait on them
> 
> (do you wait 'on' a fence or 'for' a fence? I think it's 'on' but I'm not 100% sure)
> 
>> +it to the driver. 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
> 
> Start a new sentence here: ...drivers. Out-fences...
> 
>> +finished with buffer, that is the buffer is ready. The fence are represented
> 
> s/that is/i.e/
> 
> s/The fence/The fences/
> 
>> +by file and passed as file descriptor to userspace.
> 
> s/by file/as a file/
> s/as file/as a file/
> 
>> +
>> +The in-fences are communicated to the kernel at the ``VIDIOC_QBUF`` ioctl
>> +using the ``V4L2_BUF_FLAG_IN_FENCE`` buffer
>> +flags 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. Failure to set both will
> 
> s/Failure to set both/Setting one but not the other/
> 
>> +cause ``VIDIOC_QBUF`` to return with error.
>> +
>> +To get a out-fence back from V4L2 the ``V4L2_BUF_FLAG_OUT_FENCE`` flag should
>> +be set to notify it that the next queued buffer should have a fence attached to
>> +it. That means the out-fence may not be associated with the buffer in the
>> +current ``VIDIOC_QBUF`` ioctl call because the ordering in which videobuf2 core
>> +queues the buffers to the drivers can't be guaranteed. To become aware of the
>> +of the next queued buffer and the out-fence attached to it the
>> +``V4L2_EVENT_BUF_QUEUED`` event should be used. It will trigger an event
>> +for every buffer queued to the V4L2 driver.
> 
> This makes no sense.
> 
> Setting this flag means IMHO that when *this* buffer is queued up to the driver,
> then it should send the BUF_QUEUED event with an out fence.
> 
> I.e. it signals that userspace wants to have the out-fence. The requirement w.r.t.
> ordering is that the BUF_QUEUED events have to be in order, but that is something
> that the driver can ensure in the case it is doing internal re-ordering.
> 
> This requirement is something that needs to be documented here, BTW.
> 
> Anyway, the flag shouldn't refer to some 'next buffer', since that's very confusing.

Just ignore this comment. I assume v4 will implement it like this.

Regards,

	Hans

> 
>> +
>> +At streamoff the out-fences will either signal normally if the drivers wait
> 
> s/drivers wait/driver waits/
> 
>> +for the operations on the buffers to finish or signal with error if the
>> +driver cancel the pending operations.
> 
> s/cancel/cancels/
> 
> Thinking with my evil hat on:
> 
> What happens if the application dequeues the buffer (VIDIOC_DQBUF) before
> dequeuing the BUF_QUEUED event? Or if the application doesn't call VIDIOC_DQEVENT
> at all? Should any pending BUF_QUEUED event with an out fence be removed from the
> event queue if the application calls DQBUF on the corresponding buffer?
> 
> Regards,
> 
> 	Hans
> 
>>  
>>  Return Value
>>  ============
>>
> 

Re: [PATCH v3 02/15] [media] vb2: add explicit fence user API

[Hans Verkuil]

On 09/11/2017 12:55 PM, Hans Verkuil wrote:
> On 09/07/2017 08:42 PM, Gustavo Padovan wrote:
>> From: Gustavo Padovan <gustavo.padovan@collabora.com>
>>
>> Turn the reserved2 field into fence_fd that we will use to send
>> an in-fence to the kernel and return an out-fence from the kernel to
>> userspace.
>>
>> Two new flags were added, V4L2_BUF_FLAG_IN_FENCE, that should be used
>> when sending a fence to the kernel to be waited on, and
>> V4L2_BUF_FLAG_OUT_FENCE, to ask the kernel to give back an out-fence.
>>
>> v2: add documentation
>>
>> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
>> ---
>>  Documentation/media/uapi/v4l/buffer.rst       | 19 +++++++++++++++++++
>>  drivers/media/usb/cpia2/cpia2_v4l.c           |  2 +-
>>  drivers/media/v4l2-core/v4l2-compat-ioctl32.c |  4 ++--
>>  drivers/media/v4l2-core/videobuf2-v4l2.c      |  2 +-
>>  include/uapi/linux/videodev2.h                |  4 +++-
>>  5 files changed, 26 insertions(+), 5 deletions(-)
>>
>> diff --git a/Documentation/media/uapi/v4l/buffer.rst b/Documentation/media/uapi/v4l/buffer.rst
>> index ae6ee73f151c..664507ad06c6 100644
>> --- a/Documentation/media/uapi/v4l/buffer.rst
>> +++ b/Documentation/media/uapi/v4l/buffer.rst
>> @@ -648,6 +648,25 @@ Buffer Flags
>>        - Start Of Exposure. The buffer timestamp has been taken when the
>>  	exposure of the frame has begun. This is only valid for the
>>  	``V4L2_BUF_TYPE_VIDEO_CAPTURE`` buffer type.
>> +    * .. _`V4L2-BUF-FLAG-IN-FENCE`:
>> +
>> +      - ``V4L2_BUF_FLAG_IN_FENCE``
>> +      - 0x00200000
>> +      - Ask V4L2 to wait on fence passed in ``fence_fd`` field. The buffer
>> +	won't be queued to the driver until the fence signals.
>> +
>> +    * .. _`V4L2-BUF-FLAG-OUT-FENCE`:
>> +
>> +      - ``V4L2_BUF_FLAG_OUT_FENCE``
>> +      - 0x00400000
>> +      - Request a fence for the next buffer to be queued to V4L2 driver.
>> +	The fence received back through the ``fence_fd`` field  doesn't
>> +	necessarily relate to the current buffer in the
>> +	:ref:`VIDIOC_QBUF <VIDIOC_QBUF>` ioctl. Although, most of the time
>> +	the fence will relate to the current buffer it can't be guaranteed.
>> +	So to tell userspace which buffer is associated to the out_fence,
>> +	one should listen for the ``V4L2_EVENT_BUF_QUEUED`` event that
>> +	provide the id of the buffer when it is queued to the V4L2 driver.
> 
> As mentioned in the previous patch, this flag should just signal that userspace
> wants to receive a BUF_QUEUED event with the out-fence for this buffer. It's up
> to the driver to ensure the correct ordering of the events.

I'll wait for v4 before continuing my review.

	Hans

> 
> I'm missing documentation for the new fence_fd field.
> 
> It should mention that fence_fd is ignored if the IN_FENCE isn't set. Applications
> will set fence_fd (aka reserved2) to 0, which should be fine as long as IN_FENCE
> isn't set.
> 
>>  
>>  
>>  
>> diff --git a/drivers/media/usb/cpia2/cpia2_v4l.c b/drivers/media/usb/cpia2/cpia2_v4l.c
>> index 3dedd83f0b19..6cde686bf44c 100644
>> --- a/drivers/media/usb/cpia2/cpia2_v4l.c
>> +++ b/drivers/media/usb/cpia2/cpia2_v4l.c
>> @@ -948,7 +948,7 @@ static int cpia2_dqbuf(struct file *file, void *fh, struct v4l2_buffer *buf)
>>  	buf->sequence = cam->buffers[buf->index].seq;
>>  	buf->m.offset = cam->buffers[buf->index].data - cam->frame_buffer;
>>  	buf->length = cam->frame_size;
>> -	buf->reserved2 = 0;
>> +	buf->fence_fd = -1;
>>  	buf->reserved = 0;
>>  	memset(&buf->timecode, 0, sizeof(buf->timecode));
>>  
>> diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
>> index 821f2aa299ae..d624fb5df130 100644
>> --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
>> +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
>> @@ -370,7 +370,7 @@ struct v4l2_buffer32 {
>>  		__s32		fd;
>>  	} m;
>>  	__u32			length;
>> -	__u32			reserved2;
>> +	__s32			fence_fd;
>>  	__u32			reserved;
>>  };
>>  
>> @@ -533,8 +533,8 @@ static int put_v4l2_buffer32(struct v4l2_buffer *kp, struct v4l2_buffer32 __user
>>  		put_user(kp->timestamp.tv_usec, &up->timestamp.tv_usec) ||
>>  		copy_to_user(&up->timecode, &kp->timecode, sizeof(struct v4l2_timecode)) ||
>>  		put_user(kp->sequence, &up->sequence) ||
>> -		put_user(kp->reserved2, &up->reserved2) ||
>>  		put_user(kp->reserved, &up->reserved) ||
>> +		put_user(kp->fence_fd, &up->fence_fd) ||
> 
> Could you move this up one line? It's easier to parse this diff if it is clear
> that fence_fd replaced reserved2.
> 
>>  		put_user(kp->length, &up->length))
>>  			return -EFAULT;
>>  
>> diff --git a/drivers/media/v4l2-core/videobuf2-v4l2.c b/drivers/media/v4l2-core/videobuf2-v4l2.c
>> index 0c0669976bdc..110fb45fef6f 100644
>> --- a/drivers/media/v4l2-core/videobuf2-v4l2.c
>> +++ b/drivers/media/v4l2-core/videobuf2-v4l2.c
>> @@ -203,7 +203,7 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, void *pb)
>>  	b->timestamp = ns_to_timeval(vb->timestamp);
>>  	b->timecode = vbuf->timecode;
>>  	b->sequence = vbuf->sequence;
>> -	b->reserved2 = 0;
>> +	b->fence_fd = -1;
>>  	b->reserved = 0;
>>  
>>  	if (q->is_multiplanar) {
>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>> index 185d6a0acc06..e5abab9a908c 100644
>> --- a/include/uapi/linux/videodev2.h
>> +++ b/include/uapi/linux/videodev2.h
>> @@ -924,7 +924,7 @@ struct v4l2_buffer {
>>  		__s32		fd;
>>  	} m;
>>  	__u32			length;
>> -	__u32			reserved2;
>> +	__s32			fence_fd;
>>  	__u32			reserved;
>>  };
>>  
>> @@ -961,6 +961,8 @@ struct v4l2_buffer {
>>  #define V4L2_BUF_FLAG_TSTAMP_SRC_SOE		0x00010000
>>  /* mem2mem encoder/decoder */
>>  #define V4L2_BUF_FLAG_LAST			0x00100000
>> +#define V4L2_BUF_FLAG_IN_FENCE			0x00200000
>> +#define V4L2_BUF_FLAG_OUT_FENCE			0x00400000
>>  
>>  /**
>>   * struct v4l2_exportbuffer - export of video buffer as DMABUF file descriptor
>>
> 
> Regards,
> 
> 	Hans
> 

Re: [PATCH v3 01/15] [media] v4l: Document explicit synchronization behaviour

[Gustavo Padovan]

2017-09-11 Hans Verkuil <hverkuil@xs4all.nl>:

> On 09/11/2017 12:50 PM, Hans Verkuil wrote:
> > On 09/07/2017 08:42 PM, Gustavo Padovan wrote:
> >> From: Gustavo Padovan <gustavo.padovan@collabora.com>
> >>
> >> Add section to VIDIOC_QBUF about it
> >>
> >> v2:
> >> 	- mention that fences are files (Hans)
> >> 	- rework for the new API
> >>
> >> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
> >> ---
> >>  Documentation/media/uapi/v4l/vidioc-qbuf.rst | 31 ++++++++++++++++++++++++++++
> >>  1 file changed, 31 insertions(+)
> >>
> >> diff --git a/Documentation/media/uapi/v4l/vidioc-qbuf.rst b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
> >> index 1f3612637200..fae0b1431672 100644
> >> --- a/Documentation/media/uapi/v4l/vidioc-qbuf.rst
> >> +++ b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
> >> @@ -117,6 +117,37 @@ 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 them to signal before using the buffer, i.e., queueing
> > 
> > wait them -> wait on them
> > 
> > (do you wait 'on' a fence or 'for' a fence? I think it's 'on' but I'm not 100% sure)
> > 
> >> +it to the driver. 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
> > 
> > Start a new sentence here: ...drivers. Out-fences...
> > 
> >> +finished with buffer, that is the buffer is ready. The fence are represented
> > 
> > s/that is/i.e/
> > 
> > s/The fence/The fences/
> > 
> >> +by file and passed as file descriptor to userspace.
> > 
> > s/by file/as a file/
> > s/as file/as a file/
> > 
> >> +
> >> +The in-fences are communicated to the kernel at the ``VIDIOC_QBUF`` ioctl
> >> +using the ``V4L2_BUF_FLAG_IN_FENCE`` buffer
> >> +flags 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. Failure to set both will
> > 
> > s/Failure to set both/Setting one but not the other/
> > 
> >> +cause ``VIDIOC_QBUF`` to return with error.
> >> +
> >> +To get a out-fence back from V4L2 the ``V4L2_BUF_FLAG_OUT_FENCE`` flag should
> >> +be set to notify it that the next queued buffer should have a fence attached to
> >> +it. That means the out-fence may not be associated with the buffer in the
> >> +current ``VIDIOC_QBUF`` ioctl call because the ordering in which videobuf2 core
> >> +queues the buffers to the drivers can't be guaranteed. To become aware of the
> >> +of the next queued buffer and the out-fence attached to it the
> >> +``V4L2_EVENT_BUF_QUEUED`` event should be used. It will trigger an event
> >> +for every buffer queued to the V4L2 driver.
> > 
> > This makes no sense.
> > 
> > Setting this flag means IMHO that when *this* buffer is queued up to the driver,
> > then it should send the BUF_QUEUED event with an out fence.
> > 
> > I.e. it signals that userspace wants to have the out-fence. The requirement w.r.t.
> > ordering is that the BUF_QUEUED events have to be in order, but that is something
> > that the driver can ensure in the case it is doing internal re-ordering.
> > 
> > This requirement is something that needs to be documented here, BTW.
> > 
> > Anyway, the flag shouldn't refer to some 'next buffer', since that's very confusing.
> 
> Just ignore this comment. I assume v4 will implement it like this.

What approach do you mean by "like this". I'm confused now. :)

In fact, I was in doubt between these two different approaches here.
Should the flag mean *this* or the *next* buffer? The buffers can still
be reordered at the videobuf2 level, because they might be waiting on
in-fences and the fences may signal out of order. Then I went for the
*next* buffer approach because we don't know that buffer for sure.
But now thinking on this again we shouldn't have problems with the 
*this* buffer approach also.

> 
> Regards,
> 
> 	Hans
> 
> > 
> >> +
> >> +At streamoff the out-fences will either signal normally if the drivers wait
> > 
> > s/drivers wait/driver waits/
> > 
> >> +for the operations on the buffers to finish or signal with error if the
> >> +driver cancel the pending operations.
> > 
> > s/cancel/cancels/
> > 
> > Thinking with my evil hat on:
> > 
> > What happens if the application dequeues the buffer (VIDIOC_DQBUF) before
> > dequeuing the BUF_QUEUED event? Or if the application doesn't call VIDIOC_DQEVENT
> > at all? Should any pending BUF_QUEUED event with an out fence be removed from the
> > event queue if the application calls DQBUF on the corresponding buffer?

Good catch, we need to clean that up.

Gustavo

Re: [PATCH v3 01/15] [media] v4l: Document explicit synchronization behaviour

[Hans Verkuil]

On 09/11/2017 03:18 PM, Gustavo Padovan wrote:
> 2017-09-11 Hans Verkuil <hverkuil@xs4all.nl>:
> 
>> On 09/11/2017 12:50 PM, Hans Verkuil wrote:
>>> On 09/07/2017 08:42 PM, Gustavo Padovan wrote:
>>>> From: Gustavo Padovan <gustavo.padovan@collabora.com>
>>>>
>>>> Add section to VIDIOC_QBUF about it
>>>>
>>>> v2:
>>>> 	- mention that fences are files (Hans)
>>>> 	- rework for the new API
>>>>
>>>> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
>>>> ---
>>>>  Documentation/media/uapi/v4l/vidioc-qbuf.rst | 31 ++++++++++++++++++++++++++++
>>>>  1 file changed, 31 insertions(+)
>>>>
>>>> diff --git a/Documentation/media/uapi/v4l/vidioc-qbuf.rst b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
>>>> index 1f3612637200..fae0b1431672 100644
>>>> --- a/Documentation/media/uapi/v4l/vidioc-qbuf.rst
>>>> +++ b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
>>>> @@ -117,6 +117,37 @@ 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 them to signal before using the buffer, i.e., queueing
>>>
>>> wait them -> wait on them
>>>
>>> (do you wait 'on' a fence or 'for' a fence? I think it's 'on' but I'm not 100% sure)
>>>
>>>> +it to the driver. 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
>>>
>>> Start a new sentence here: ...drivers. Out-fences...
>>>
>>>> +finished with buffer, that is the buffer is ready. The fence are represented
>>>
>>> s/that is/i.e/
>>>
>>> s/The fence/The fences/
>>>
>>>> +by file and passed as file descriptor to userspace.
>>>
>>> s/by file/as a file/
>>> s/as file/as a file/
>>>
>>>> +
>>>> +The in-fences are communicated to the kernel at the ``VIDIOC_QBUF`` ioctl
>>>> +using the ``V4L2_BUF_FLAG_IN_FENCE`` buffer
>>>> +flags 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. Failure to set both will
>>>
>>> s/Failure to set both/Setting one but not the other/
>>>
>>>> +cause ``VIDIOC_QBUF`` to return with error.
>>>> +
>>>> +To get a out-fence back from V4L2 the ``V4L2_BUF_FLAG_OUT_FENCE`` flag should
>>>> +be set to notify it that the next queued buffer should have a fence attached to
>>>> +it. That means the out-fence may not be associated with the buffer in the
>>>> +current ``VIDIOC_QBUF`` ioctl call because the ordering in which videobuf2 core
>>>> +queues the buffers to the drivers can't be guaranteed. To become aware of the
>>>> +of the next queued buffer and the out-fence attached to it the
>>>> +``V4L2_EVENT_BUF_QUEUED`` event should be used. It will trigger an event
>>>> +for every buffer queued to the V4L2 driver.
>>>
>>> This makes no sense.
>>>
>>> Setting this flag means IMHO that when *this* buffer is queued up to the driver,
>>> then it should send the BUF_QUEUED event with an out fence.
>>>
>>> I.e. it signals that userspace wants to have the out-fence. The requirement w.r.t.
>>> ordering is that the BUF_QUEUED events have to be in order, but that is something
>>> that the driver can ensure in the case it is doing internal re-ordering.
>>>
>>> This requirement is something that needs to be documented here, BTW.
>>>
>>> Anyway, the flag shouldn't refer to some 'next buffer', since that's very confusing.
>>
>> Just ignore this comment. I assume v4 will implement it like this.
> 
> What approach do you mean by "like this". I'm confused now. :)
> 
> In fact, I was in doubt between these two different approaches here.
> Should the flag mean *this* or the *next* buffer? The buffers can still
> be reordered at the videobuf2 level, because they might be waiting on
> in-fences and the fences may signal out of order. Then I went for the
> *next* buffer approach because we don't know that buffer for sure.
> But now thinking on this again we shouldn't have problems with the 
> *this* buffer approach also.

It should mean *this* buffer. It's really weird to set this flag for one
buffer, only for it to mean 'next' buffer.

Keep it simple: the flag just means: send me the output fence fd for this
buffer once you have it. If it is not set, then no BUF_QUEUE event is sent.

Actually, it could mean one of two things: either if it is not set, then no
BUF_QUEUE event is sent, or if it is not set, then the fd in the BUF_QUEUE
event is -1.

I'm leaning towards the first. I can't see any use-case for sending that
event if you are not requesting out fences.

Regards,

	Hans

> 
>>
>> Regards,
>>
>> 	Hans
>>
>>>
>>>> +
>>>> +At streamoff the out-fences will either signal normally if the drivers wait
>>>
>>> s/drivers wait/driver waits/
>>>
>>>> +for the operations on the buffers to finish or signal with error if the
>>>> +driver cancel the pending operations.
>>>
>>> s/cancel/cancels/
>>>
>>> Thinking with my evil hat on:
>>>
>>> What happens if the application dequeues the buffer (VIDIOC_DQBUF) before
>>> dequeuing the BUF_QUEUED event? Or if the application doesn't call VIDIOC_DQEVENT
>>> at all? Should any pending BUF_QUEUED event with an out fence be removed from the
>>> event queue if the application calls DQBUF on the corresponding buffer?
> 
> Good catch, we need to clean that up.
> 
> Gustavo
> 

Re: [PATCH v3 01/15] [media] v4l: Document explicit synchronization behaviour

[Gustavo Padovan]

2017-09-11 Hans Verkuil <hverkuil@xs4all.nl>:

> On 09/11/2017 03:18 PM, Gustavo Padovan wrote:
> > 2017-09-11 Hans Verkuil <hverkuil@xs4all.nl>:
> > 
> >> On 09/11/2017 12:50 PM, Hans Verkuil wrote:
> >>> On 09/07/2017 08:42 PM, Gustavo Padovan wrote:
> >>>> From: Gustavo Padovan <gustavo.padovan@collabora.com>
> >>>>
> >>>> Add section to VIDIOC_QBUF about it
> >>>>
> >>>> v2:
> >>>> 	- mention that fences are files (Hans)
> >>>> 	- rework for the new API
> >>>>
> >>>> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
> >>>> ---
> >>>>  Documentation/media/uapi/v4l/vidioc-qbuf.rst | 31 ++++++++++++++++++++++++++++
> >>>>  1 file changed, 31 insertions(+)
> >>>>
> >>>> diff --git a/Documentation/media/uapi/v4l/vidioc-qbuf.rst b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
> >>>> index 1f3612637200..fae0b1431672 100644
> >>>> --- a/Documentation/media/uapi/v4l/vidioc-qbuf.rst
> >>>> +++ b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
> >>>> @@ -117,6 +117,37 @@ 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 them to signal before using the buffer, i.e., queueing
> >>>
> >>> wait them -> wait on them
> >>>
> >>> (do you wait 'on' a fence or 'for' a fence? I think it's 'on' but I'm not 100% sure)
> >>>
> >>>> +it to the driver. 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
> >>>
> >>> Start a new sentence here: ...drivers. Out-fences...
> >>>
> >>>> +finished with buffer, that is the buffer is ready. The fence are represented
> >>>
> >>> s/that is/i.e/
> >>>
> >>> s/The fence/The fences/
> >>>
> >>>> +by file and passed as file descriptor to userspace.
> >>>
> >>> s/by file/as a file/
> >>> s/as file/as a file/
> >>>
> >>>> +
> >>>> +The in-fences are communicated to the kernel at the ``VIDIOC_QBUF`` ioctl
> >>>> +using the ``V4L2_BUF_FLAG_IN_FENCE`` buffer
> >>>> +flags 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. Failure to set both will
> >>>
> >>> s/Failure to set both/Setting one but not the other/
> >>>
> >>>> +cause ``VIDIOC_QBUF`` to return with error.
> >>>> +
> >>>> +To get a out-fence back from V4L2 the ``V4L2_BUF_FLAG_OUT_FENCE`` flag should
> >>>> +be set to notify it that the next queued buffer should have a fence attached to
> >>>> +it. That means the out-fence may not be associated with the buffer in the
> >>>> +current ``VIDIOC_QBUF`` ioctl call because the ordering in which videobuf2 core
> >>>> +queues the buffers to the drivers can't be guaranteed. To become aware of the
> >>>> +of the next queued buffer and the out-fence attached to it the
> >>>> +``V4L2_EVENT_BUF_QUEUED`` event should be used. It will trigger an event
> >>>> +for every buffer queued to the V4L2 driver.
> >>>
> >>> This makes no sense.
> >>>
> >>> Setting this flag means IMHO that when *this* buffer is queued up to the driver,
> >>> then it should send the BUF_QUEUED event with an out fence.
> >>>
> >>> I.e. it signals that userspace wants to have the out-fence. The requirement w.r.t.
> >>> ordering is that the BUF_QUEUED events have to be in order, but that is something
> >>> that the driver can ensure in the case it is doing internal re-ordering.
> >>>
> >>> This requirement is something that needs to be documented here, BTW.
> >>>
> >>> Anyway, the flag shouldn't refer to some 'next buffer', since that's very confusing.
> >>
> >> Just ignore this comment. I assume v4 will implement it like this.
> > 
> > What approach do you mean by "like this". I'm confused now. :)
> > 
> > In fact, I was in doubt between these two different approaches here.
> > Should the flag mean *this* or the *next* buffer? The buffers can still
> > be reordered at the videobuf2 level, because they might be waiting on
> > in-fences and the fences may signal out of order. Then I went for the
> > *next* buffer approach because we don't know that buffer for sure.
> > But now thinking on this again we shouldn't have problems with the 
> > *this* buffer approach also.
> 
> It should mean *this* buffer. It's really weird to set this flag for one
> buffer, only for it to mean 'next' buffer.
> 
> Keep it simple: the flag just means: send me the output fence fd for this
> buffer once you have it. If it is not set, then no BUF_QUEUE event is sent.
> 
> Actually, it could mean one of two things: either if it is not set, then no
> BUF_QUEUE event is sent, or if it is not set, then the fd in the BUF_QUEUE
> event is -1.
> 
> I'm leaning towards the first. I can't see any use-case for sending that
> event if you are not requesting out fences.

We could go with the first one but in this case it is better to rename it to
V4L2_EVENT_OUT_FENCE or something like this, isn't it?

Gustavo

Re: [PATCH v3 01/15] [media] v4l: Document explicit synchronization behaviour

[Hans Verkuil]

On 09/11/2017 03:34 PM, Gustavo Padovan wrote:
> 2017-09-11 Hans Verkuil <hverkuil@xs4all.nl>:
> 
>> On 09/11/2017 03:18 PM, Gustavo Padovan wrote:
>>> 2017-09-11 Hans Verkuil <hverkuil@xs4all.nl>:
>>>
>>>> On 09/11/2017 12:50 PM, Hans Verkuil wrote:
>>>>> On 09/07/2017 08:42 PM, Gustavo Padovan wrote:
>>>>>> From: Gustavo Padovan <gustavo.padovan@collabora.com>
>>>>>>
>>>>>> Add section to VIDIOC_QBUF about it
>>>>>>
>>>>>> v2:
>>>>>> 	- mention that fences are files (Hans)
>>>>>> 	- rework for the new API
>>>>>>
>>>>>> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
>>>>>> ---
>>>>>>  Documentation/media/uapi/v4l/vidioc-qbuf.rst | 31 ++++++++++++++++++++++++++++
>>>>>>  1 file changed, 31 insertions(+)
>>>>>>
>>>>>> diff --git a/Documentation/media/uapi/v4l/vidioc-qbuf.rst b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
>>>>>> index 1f3612637200..fae0b1431672 100644
>>>>>> --- a/Documentation/media/uapi/v4l/vidioc-qbuf.rst
>>>>>> +++ b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
>>>>>> @@ -117,6 +117,37 @@ 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 them to signal before using the buffer, i.e., queueing
>>>>>
>>>>> wait them -> wait on them
>>>>>
>>>>> (do you wait 'on' a fence or 'for' a fence? I think it's 'on' but I'm not 100% sure)
>>>>>
>>>>>> +it to the driver. 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
>>>>>
>>>>> Start a new sentence here: ...drivers. Out-fences...
>>>>>
>>>>>> +finished with buffer, that is the buffer is ready. The fence are represented
>>>>>
>>>>> s/that is/i.e/
>>>>>
>>>>> s/The fence/The fences/
>>>>>
>>>>>> +by file and passed as file descriptor to userspace.
>>>>>
>>>>> s/by file/as a file/
>>>>> s/as file/as a file/
>>>>>
>>>>>> +
>>>>>> +The in-fences are communicated to the kernel at the ``VIDIOC_QBUF`` ioctl
>>>>>> +using the ``V4L2_BUF_FLAG_IN_FENCE`` buffer
>>>>>> +flags 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. Failure to set both will
>>>>>
>>>>> s/Failure to set both/Setting one but not the other/
>>>>>
>>>>>> +cause ``VIDIOC_QBUF`` to return with error.
>>>>>> +
>>>>>> +To get a out-fence back from V4L2 the ``V4L2_BUF_FLAG_OUT_FENCE`` flag should
>>>>>> +be set to notify it that the next queued buffer should have a fence attached to
>>>>>> +it. That means the out-fence may not be associated with the buffer in the
>>>>>> +current ``VIDIOC_QBUF`` ioctl call because the ordering in which videobuf2 core
>>>>>> +queues the buffers to the drivers can't be guaranteed. To become aware of the
>>>>>> +of the next queued buffer and the out-fence attached to it the
>>>>>> +``V4L2_EVENT_BUF_QUEUED`` event should be used. It will trigger an event
>>>>>> +for every buffer queued to the V4L2 driver.
>>>>>
>>>>> This makes no sense.
>>>>>
>>>>> Setting this flag means IMHO that when *this* buffer is queued up to the driver,
>>>>> then it should send the BUF_QUEUED event with an out fence.
>>>>>
>>>>> I.e. it signals that userspace wants to have the out-fence. The requirement w.r.t.
>>>>> ordering is that the BUF_QUEUED events have to be in order, but that is something
>>>>> that the driver can ensure in the case it is doing internal re-ordering.
>>>>>
>>>>> This requirement is something that needs to be documented here, BTW.
>>>>>
>>>>> Anyway, the flag shouldn't refer to some 'next buffer', since that's very confusing.
>>>>
>>>> Just ignore this comment. I assume v4 will implement it like this.
>>>
>>> What approach do you mean by "like this". I'm confused now. :)
>>>
>>> In fact, I was in doubt between these two different approaches here.
>>> Should the flag mean *this* or the *next* buffer? The buffers can still
>>> be reordered at the videobuf2 level, because they might be waiting on
>>> in-fences and the fences may signal out of order. Then I went for the
>>> *next* buffer approach because we don't know that buffer for sure.
>>> But now thinking on this again we shouldn't have problems with the 
>>> *this* buffer approach also.
>>
>> It should mean *this* buffer. It's really weird to set this flag for one
>> buffer, only for it to mean 'next' buffer.
>>
>> Keep it simple: the flag just means: send me the output fence fd for this
>> buffer once you have it. If it is not set, then no BUF_QUEUE event is sent.
>>
>> Actually, it could mean one of two things: either if it is not set, then no
>> BUF_QUEUE event is sent, or if it is not set, then the fd in the BUF_QUEUE
>> event is -1.
>>
>> I'm leaning towards the first. I can't see any use-case for sending that
>> event if you are not requesting out fences.
> 
> We could go with the first one but in this case it is better to rename it to
> V4L2_EVENT_OUT_FENCE or something like this, isn't it?

I was thinking the same thing. That would be a better name, yes.

Regards,

	Hans

Re: [PATCH v3 00/15] V4L2 Explicit Synchronization support

[Brian Starkey]

Hi Gustavo,

On Thu, Sep 07, 2017 at 03:42:11PM -0300, Gustavo Padovan wrote:
>From: Gustavo Padovan <gustavo.padovan@collabora.com>
>
>Hi,
>
>Refer to the documentation on the first patch for the details. The previous
>iteration is here: https://www.mail-archive.com/linux-media@vger.kernel.org/msg118077.html
>
>The 2nd patch proposes an userspace API for fences, then on patch 3 we
>prepare to the addition of in-fences in patch 4, by introducing the
>infrastructure on vb2 to wait on an in-fence signal before queueing the
>buffer in the driver.
>
>Patch 5 fix uvc v4l2 event handling and patch 6 configure q->dev for
>vivid drivers to enable to subscribe and dequeue events on it.
>
>Patches 7-9 enables support to notify BUF_QUEUED events, the event send
>to userspace the out-fence fd and the index of the buffer that was queued.
>
>Patches 10-11 add support to mark queues as ordered. Finally patches 12
>and 13 add more fence infrastructure to support out-fences, patch 13 exposes
>close_fd() and patch 14 adds support to out-fences.
>
>It only works for ordered queues for now, see open question at the end
>of the letter.
>
>Test tool can be found at:
>https://git.collabora.com/cgit/user/padovan/v4l2-test.git/
>
>Main Changes
>------------
>
>* out-fences: change in behavior: the out-fence fd now comes out of the
>BUF_QUEUED event along with the buffer id.

The more I think about this, the more unfortunate it seems. Especially
for our use-case (m2m engine which sits in front of the display
processor to convert the format of some buffers), having to wait for
the in-fence to signal before we can get an out-fence removes a lot of
the advantages of having fences at all.

Ideally, we'd like to queue up our m2m work (while the GPU is still
rendering that buffer, holding the in-fence), immediately get the
out-fence for the m2m work, and pass that to DRM as the in-fence for
display. With the current behaviour we need to wait in userspace
before we can pass the buffer to display.

Wouldn't it be possible to enforce that the buffers aren't queued
out-of-order in VB2? An easy way might be to (in qbuf) set a buffer's
->in_fence to be a fence_array of all the ->in_fences from the buffers
before it in the queue (and its own). That would then naturally order
the enqueue-ing in the driver, and allow you to return the out-fence
immediately.

This would also solve your output devices question from below - a
buffer can never get queued in the driver until all of the buffers
which were QBUF'd before it are queued in the driver.

Cheers,
-Brian

>
>All other changes are recorded on the patches' commit messages.
>
>Open Questions
>--------------
>
>* non-ordered devices, like m2m: I've been thinking a lot about those
>  and one possibility is to have a way to tell userspace that the queue
>  is not ordered and then associate the fence with the current buffer in
>  QBUF instead of the next one to be queued. Of course, there won't be
>  any ordering between the fences. But it may be enough for userspace to
>  take advantage of Explicit Synchronization in such cases. Any
>  thoughts?
>
>* OUTPUT devices and in-fence. If I understood OUTPUT devices correctly
>  it is desirable to queue the buffers to the driver in the same order
>  we received them from userspace. If that is correct, shouldn't we add
>  some mechanism to prevent buffer whose fence signaled to jump ahead of
>  others?
>
>Gustavo Padovan (14):
>  [media] v4l: Document explicit synchronization behaviour
>  [media] vb2: add explicit fence user API
>  [media] vb2: check earlier if stream can be started
>  [media] vb2: add in-fence support to QBUF
>  [media] uvc: enable subscriptions to other events
>  [media] vivid: assign the specific device to the vb2_queue->dev
>  [media] v4l: add V4L2_EVENT_BUF_QUEUED event
>  [media] vb2: add .buffer_queued() to notify queueing in the driver
>  [media] v4l: add support to BUF_QUEUED event
>  [media] vb2: add 'ordered' property to queues
>  [media] vivid: mark vivid queues as ordered
>  [media] vb2: add infrastructure to support out-fences
>  fs/files: export close_fd() symbol
>  [media] vb2: add out-fence support to QBUF
>
>Javier Martinez Canillas (1):
>  [media] vb2: add videobuf2 dma-buf fence helpers
>
> Documentation/media/uapi/v4l/buffer.rst         |  19 ++
> Documentation/media/uapi/v4l/vidioc-dqevent.rst |  23 +++
> Documentation/media/uapi/v4l/vidioc-qbuf.rst    |  31 ++++
> Documentation/media/videodev2.h.rst.exceptions  |   1 +
> drivers/android/binder.c                        |   2 +-
> drivers/media/platform/vivid/vivid-core.c       |  15 +-
> drivers/media/usb/cpia2/cpia2_v4l.c             |   2 +-
> drivers/media/usb/uvc/uvc_v4l2.c                |   2 +-
> drivers/media/v4l2-core/Kconfig                 |   1 +
> drivers/media/v4l2-core/v4l2-compat-ioctl32.c   |   4 +-
> drivers/media/v4l2-core/v4l2-ctrls.c            |   6 +-
> drivers/media/v4l2-core/videobuf2-core.c        | 221 ++++++++++++++++++++++--
> drivers/media/v4l2-core/videobuf2-v4l2.c        |  63 ++++++-
> fs/file.c                                       |   5 +-
> fs/open.c                                       |   2 +-
> include/linux/fdtable.h                         |   2 +-
> include/media/videobuf2-core.h                  |  63 ++++++-
> include/media/videobuf2-fence.h                 |  49 ++++++
> include/uapi/linux/videodev2.h                  |  15 +-
> 19 files changed, 489 insertions(+), 37 deletions(-)
> create mode 100644 include/media/videobuf2-fence.h
>
>-- 
>2.13.5
>

Re: [PATCH v3 02/15] [media] vb2: add explicit fence user API

[Brian Starkey]

Hi,

On Thu, Sep 07, 2017 at 03:42:13PM -0300, Gustavo Padovan wrote:
>From: Gustavo Padovan <gustavo.padovan@collabora.com>
>
>Turn the reserved2 field into fence_fd that we will use to send
>an in-fence to the kernel and return an out-fence from the kernel to
>userspace.
>
>Two new flags were added, V4L2_BUF_FLAG_IN_FENCE, that should be used
>when sending a fence to the kernel to be waited on, and
>V4L2_BUF_FLAG_OUT_FENCE, to ask the kernel to give back an out-fence.

It seems a bit off to me to add this to the uapi, and document it,
before any of the implementation is present in the kernel.

Wouldn't it be better to move this patch to be the last one, after all
of the implementation is done?

Cheers,
-Brian

>
>v2: add documentation
>
>Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
>---
> Documentation/media/uapi/v4l/buffer.rst       | 19 +++++++++++++++++++
> drivers/media/usb/cpia2/cpia2_v4l.c           |  2 +-
> drivers/media/v4l2-core/v4l2-compat-ioctl32.c |  4 ++--
> drivers/media/v4l2-core/videobuf2-v4l2.c      |  2 +-
> include/uapi/linux/videodev2.h                |  4 +++-
> 5 files changed, 26 insertions(+), 5 deletions(-)
>
>diff --git a/Documentation/media/uapi/v4l/buffer.rst b/Documentation/media/uapi/v4l/buffer.rst
>index ae6ee73f151c..664507ad06c6 100644
>--- a/Documentation/media/uapi/v4l/buffer.rst
>+++ b/Documentation/media/uapi/v4l/buffer.rst
>@@ -648,6 +648,25 @@ Buffer Flags
>       - Start Of Exposure. The buffer timestamp has been taken when the
> 	exposure of the frame has begun. This is only valid for the
> 	``V4L2_BUF_TYPE_VIDEO_CAPTURE`` buffer type.
>+    * .. _`V4L2-BUF-FLAG-IN-FENCE`:
>+
>+      - ``V4L2_BUF_FLAG_IN_FENCE``
>+      - 0x00200000
>+      - Ask V4L2 to wait on fence passed in ``fence_fd`` field. The buffer
>+	won't be queued to the driver until the fence signals.
>+
>+    * .. _`V4L2-BUF-FLAG-OUT-FENCE`:
>+
>+      - ``V4L2_BUF_FLAG_OUT_FENCE``
>+      - 0x00400000
>+      - Request a fence for the next buffer to be queued to V4L2 driver.
>+	The fence received back through the ``fence_fd`` field  doesn't
>+	necessarily relate to the current buffer in the
>+	:ref:`VIDIOC_QBUF <VIDIOC_QBUF>` ioctl. Although, most of the time
>+	the fence will relate to the current buffer it can't be guaranteed.
>+	So to tell userspace which buffer is associated to the out_fence,
>+	one should listen for the ``V4L2_EVENT_BUF_QUEUED`` event that
>+	provide the id of the buffer when it is queued to the V4L2 driver.
>
>
>
>diff --git a/drivers/media/usb/cpia2/cpia2_v4l.c b/drivers/media/usb/cpia2/cpia2_v4l.c
>index 3dedd83f0b19..6cde686bf44c 100644
>--- a/drivers/media/usb/cpia2/cpia2_v4l.c
>+++ b/drivers/media/usb/cpia2/cpia2_v4l.c
>@@ -948,7 +948,7 @@ static int cpia2_dqbuf(struct file *file, void *fh, struct v4l2_buffer *buf)
> 	buf->sequence = cam->buffers[buf->index].seq;
> 	buf->m.offset = cam->buffers[buf->index].data - cam->frame_buffer;
> 	buf->length = cam->frame_size;
>-	buf->reserved2 = 0;
>+	buf->fence_fd = -1;
> 	buf->reserved = 0;
> 	memset(&buf->timecode, 0, sizeof(buf->timecode));
>
>diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
>index 821f2aa299ae..d624fb5df130 100644
>--- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
>+++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
>@@ -370,7 +370,7 @@ struct v4l2_buffer32 {
> 		__s32		fd;
> 	} m;
> 	__u32			length;
>-	__u32			reserved2;
>+	__s32			fence_fd;
> 	__u32			reserved;
> };
>
>@@ -533,8 +533,8 @@ static int put_v4l2_buffer32(struct v4l2_buffer *kp, struct v4l2_buffer32 __user
> 		put_user(kp->timestamp.tv_usec, &up->timestamp.tv_usec) ||
> 		copy_to_user(&up->timecode, &kp->timecode, sizeof(struct v4l2_timecode)) ||
> 		put_user(kp->sequence, &up->sequence) ||
>-		put_user(kp->reserved2, &up->reserved2) ||
> 		put_user(kp->reserved, &up->reserved) ||
>+		put_user(kp->fence_fd, &up->fence_fd) ||
> 		put_user(kp->length, &up->length))
> 			return -EFAULT;
>
>diff --git a/drivers/media/v4l2-core/videobuf2-v4l2.c b/drivers/media/v4l2-core/videobuf2-v4l2.c
>index 0c0669976bdc..110fb45fef6f 100644
>--- a/drivers/media/v4l2-core/videobuf2-v4l2.c
>+++ b/drivers/media/v4l2-core/videobuf2-v4l2.c
>@@ -203,7 +203,7 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, void *pb)
> 	b->timestamp = ns_to_timeval(vb->timestamp);
> 	b->timecode = vbuf->timecode;
> 	b->sequence = vbuf->sequence;
>-	b->reserved2 = 0;
>+	b->fence_fd = -1;
> 	b->reserved = 0;
>
> 	if (q->is_multiplanar) {
>diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>index 185d6a0acc06..e5abab9a908c 100644
>--- a/include/uapi/linux/videodev2.h
>+++ b/include/uapi/linux/videodev2.h
>@@ -924,7 +924,7 @@ struct v4l2_buffer {
> 		__s32		fd;
> 	} m;
> 	__u32			length;
>-	__u32			reserved2;
>+	__s32			fence_fd;
> 	__u32			reserved;
> };
>
>@@ -961,6 +961,8 @@ struct v4l2_buffer {
> #define V4L2_BUF_FLAG_TSTAMP_SRC_SOE		0x00010000
> /* mem2mem encoder/decoder */
> #define V4L2_BUF_FLAG_LAST			0x00100000
>+#define V4L2_BUF_FLAG_IN_FENCE			0x00200000
>+#define V4L2_BUF_FLAG_OUT_FENCE			0x00400000
>
> /**
>  * struct v4l2_exportbuffer - export of video buffer as DMABUF file descriptor
>-- 
>2.13.5
>

Re: [PATCH v3 04/15] [media] vb2: add in-fence support to QBUF

[Brian Starkey]

Hi,

On Thu, Sep 07, 2017 at 03:42:15PM -0300, Gustavo Padovan wrote:
>From: Gustavo Padovan <gustavo.padovan@collabora.com>
>
>Receive in-fence from userspace and add support for waiting on them
>before queueing the buffer to the driver. Buffers are only queued
>to the driver once they are ready. A buffer is ready when its
>in-fence signals.
>
>v4:
>	- Add a comment about dma_fence_add_callback() not returning a
>	error (Hans)
>	- Call dma_fence_put(vb->in_fence) if fence signaled (Hans)
>	- select SYNC_FILE under config VIDEOBUF2_CORE (Hans)
>	- Move dma_fence_is_signaled() check to __enqueue_in_driver() (Hans)
>	- Remove list_for_each_entry() in __vb2_core_qbuf() (Hans)
>	-  Remove if (vb->state != VB2_BUF_STATE_QUEUED) from
>	vb2_start_streaming() (Hans)
>	- set IN_FENCE flags on __fill_v4l2_buffer (Hans)
>	- Queue buffers to the driver as soon as they are ready (Hans)
>	- call fill_user_buffer() after queuing the buffer (Hans)
>	- add err: label to clean up fence
>	- add dma_fence_wait() before calling vb2_start_streaming()
>
>v3:	- document fence parameter
>	- remove ternary if at vb2_qbuf() return (Mauro)
>	- do not change if conditions behaviour (Mauro)
>
>v2:
>	- fix vb2_queue_or_prepare_buf() ret check
>	- remove check for VB2_MEMORY_DMABUF only (Javier)
>	- check num of ready buffers to start streaming
>	- when queueing, start from the first ready buffer
>	- handle queue cancel
>
>Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
>---
> drivers/media/v4l2-core/Kconfig          |   1 +
> drivers/media/v4l2-core/videobuf2-core.c | 103 +++++++++++++++++++++++++++----
> drivers/media/v4l2-core/videobuf2-v4l2.c |  27 +++++++-
> include/media/videobuf2-core.h           |  11 +++-
> 4 files changed, 127 insertions(+), 15 deletions(-)
>
>diff --git a/drivers/media/v4l2-core/Kconfig b/drivers/media/v4l2-core/Kconfig
>index a35c33686abf..3f988c407c80 100644
>--- a/drivers/media/v4l2-core/Kconfig
>+++ b/drivers/media/v4l2-core/Kconfig
>@@ -83,6 +83,7 @@ config VIDEOBUF_DVB
> # Used by drivers that need Videobuf2 modules
> config VIDEOBUF2_CORE
> 	select DMA_SHARED_BUFFER
>+	select SYNC_FILE
> 	tristate
>
> config VIDEOBUF2_MEMOPS
>diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
>index 60f8b582396a..b19c1bc4b083 100644
>--- a/drivers/media/v4l2-core/videobuf2-core.c
>+++ b/drivers/media/v4l2-core/videobuf2-core.c
>@@ -1222,6 +1222,9 @@ static void __enqueue_in_driver(struct vb2_buffer *vb)
> {
> 	struct vb2_queue *q = vb->vb2_queue;
>
>+	if (vb->in_fence && !dma_fence_is_signaled(vb->in_fence))
>+		return;
>+
> 	vb->state = VB2_BUF_STATE_ACTIVE;
> 	atomic_inc(&q->owned_by_drv_count);
>
>@@ -1273,6 +1276,20 @@ static int __buf_prepare(struct vb2_buffer *vb, const void *pb)
> 	return 0;
> }
>
>+static int __get_num_ready_buffers(struct vb2_queue *q)
>+{
>+	struct vb2_buffer *vb;
>+	int ready_count = 0;
>+
>+	/* count num of buffers ready in front of the queued_list */
>+	list_for_each_entry(vb, &q->queued_list, queued_entry) {
>+		if (!vb->in_fence || dma_fence_is_signaled(vb->in_fence))
>+			ready_count++;
>+	}
>+
>+	return ready_count;
>+}
>+
> int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb)
> {
> 	struct vb2_buffer *vb;
>@@ -1361,7 +1378,19 @@ static int vb2_start_streaming(struct vb2_queue *q)
> 	return ret;
> }
>
>-int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
>+static void vb2_qbuf_fence_cb(struct dma_fence *f, struct dma_fence_cb *cb)
>+{
>+	struct vb2_buffer *vb = container_of(cb, struct vb2_buffer, fence_cb);
>+
>+	dma_fence_put(vb->in_fence);
>+	vb->in_fence = NULL;
>+
>+	if (vb->vb2_queue->start_streaming_called)
>+		__enqueue_in_driver(vb);
>+}
>+
>+int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb,
>+		  struct dma_fence *fence)
> {
> 	struct vb2_buffer *vb;
> 	int ret;
>@@ -1372,16 +1401,18 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
> 	case VB2_BUF_STATE_DEQUEUED:
> 		ret = __buf_prepare(vb, pb);
> 		if (ret)
>-			return ret;
>+			goto err;
> 		break;
> 	case VB2_BUF_STATE_PREPARED:
> 		break;
> 	case VB2_BUF_STATE_PREPARING:
> 		dprintk(1, "buffer still being prepared\n");
>-		return -EINVAL;
>+		ret = -EINVAL;
>+		goto err;
> 	default:
> 		dprintk(1, "invalid buffer state %d\n", vb->state);
>-		return -EINVAL;
>+		ret = -EINVAL;
>+		goto err;
> 	}
>
> 	/*
>@@ -1392,6 +1423,7 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
> 	q->queued_count++;
> 	q->waiting_for_buffers = false;
> 	vb->state = VB2_BUF_STATE_QUEUED;
>+	vb->in_fence = fence;
>
> 	if (pb)
> 		call_void_bufop(q, copy_timestamp, vb, pb);
>@@ -1399,6 +1431,16 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
> 	trace_vb2_qbuf(q, vb);
>
> 	/*
>+	 * If it is time to call vb2_start_streaming() wait for the fence
>+	 * to signal first. Of course, this happens only once per streaming.
>+	 * We want to run any step that might fail before we set the callback
>+	 * to queue the fence when it signals.
>+	 */
>+	if (fence && !q->start_streaming_called &&
>+	    __get_num_ready_buffers(q) == q->min_buffers_needed - 1)
>+		dma_fence_wait(fence, true);
>+
>+	/*
> 	 * If streamon has been called, and we haven't yet called
> 	 * start_streaming() since not enough buffers were queued, and
> 	 * we now have reached the minimum number of queued buffers,
>@@ -1408,20 +1450,48 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
> 	 * If not, the buffer will be given to driver on next streamon.
> 	 */

The last two lines of this comment don't seem to apply here any more:

      * If already streaming, give the buffer to driver for processing.
      * If not, the buffer will be given to driver on next streamon.

Might be worth clarifying that this will happen lower down, or move it
to by the fill: label.

> 	if (q->streaming && !q->start_streaming_called &&
>-	    q->queued_count >= q->min_buffers_needed) {
>+	    __get_num_ready_buffers(q) >= q->min_buffers_needed) {
> 		ret = vb2_start_streaming(q);
> 		if (ret)
>-			return ret;
>-	} else if (q->start_streaming_called) {
>-		__enqueue_in_driver(vb);
>+			goto err;
> 	}
>
>+	/*
>+	 * For explicit synchronization: If the fence didn't signal
>+	 * yet we setup a callback to queue the buffer once the fence
>+	 * signals, and then, return successfully. But if the fence
>+	 * already signaled we lose the reference we held and queue the
>+	 * buffer to the driver.
>+	 */
>+	if (fence) {
>+		ret = dma_fence_add_callback(fence, &vb->fence_cb,
>+					     vb2_qbuf_fence_cb);
>+		if (!ret)
>+			goto fill;
>+
>+		dma_fence_put(fence);
>+		vb->in_fence = NULL;
>+	}
>+
>+fill:
>+	if (q->start_streaming_called && !vb->in_fence)
>+		__enqueue_in_driver(vb);
>+
> 	/* Fill buffer information for the userspace */
> 	if (pb)
> 		call_void_bufop(q, fill_user_buffer, vb, pb);
>
> 	dprintk(2, "qbuf of buffer %d succeeded\n", vb->index);
> 	return 0;
>+
>+err:
>+	if (vb->in_fence) {
>+		dma_fence_put(vb->in_fence);
>+		vb->in_fence = NULL;
>+	}
>+
>+	return ret;
>+
> }
> EXPORT_SYMBOL_GPL(vb2_core_qbuf);
>
>@@ -1632,6 +1702,7 @@ EXPORT_SYMBOL_GPL(vb2_core_dqbuf);
> static void __vb2_queue_cancel(struct vb2_queue *q)
> {
> 	unsigned int i;
>+	struct vb2_buffer *vb;
>
> 	/*
> 	 * Tell driver to stop all transactions and release all queued
>@@ -1654,6 +1725,14 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
> 		WARN_ON(atomic_read(&q->owned_by_drv_count));
> 	}
>
>+	list_for_each_entry(vb, &q->queued_list, queued_entry) {
>+		if (vb->in_fence) {
>+			dma_fence_remove_callback(vb->in_fence, &vb->fence_cb);

This looks racy - I think the callback might have run, or be running,
concurrently here. In that case you might have a NULL pointer deref,
or use-after-free.

>+			dma_fence_put(vb->in_fence);
>+			vb->in_fence = NULL;
>+		}
>+	}
>+
> 	q->streaming = 0;
> 	q->start_streaming_called = 0;
> 	q->queued_count = 0;
>@@ -1720,7 +1799,7 @@ int vb2_core_streamon(struct vb2_queue *q, unsigned int type)
> 	 * Tell driver to start streaming provided sufficient buffers
> 	 * are available.
> 	 */
>-	if (q->queued_count >= q->min_buffers_needed) {
>+	if (__get_num_ready_buffers(q) >= q->min_buffers_needed) {
> 		ret = v4l_vb2q_enable_media_source(q);
> 		if (ret)
> 			return ret;
>@@ -2240,7 +2319,7 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read)
> 		 * Queue all buffers.
> 		 */
> 		for (i = 0; i < q->num_buffers; i++) {
>-			ret = vb2_core_qbuf(q, i, NULL);
>+			ret = vb2_core_qbuf(q, i, NULL, NULL);
> 			if (ret)
> 				goto err_reqbufs;
> 			fileio->bufs[i].queued = 1;
>@@ -2419,7 +2498,7 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
>
> 		if (copy_timestamp)
> 			b->timestamp = ktime_get_ns();
>-		ret = vb2_core_qbuf(q, index, NULL);
>+		ret = vb2_core_qbuf(q, index, NULL, NULL);
> 		dprintk(5, "vb2_dbuf result: %d\n", ret);
> 		if (ret)
> 			return ret;
>@@ -2522,7 +2601,7 @@ static int vb2_thread(void *data)
> 		if (copy_timestamp)
> 			vb->timestamp = ktime_get_ns();;
> 		if (!threadio->stop)
>-			ret = vb2_core_qbuf(q, vb->index, NULL);
>+			ret = vb2_core_qbuf(q, vb->index, NULL, NULL);
> 		call_void_qop(q, wait_prepare, q);
> 		if (ret || threadio->stop)
> 			break;
>diff --git a/drivers/media/v4l2-core/videobuf2-v4l2.c b/drivers/media/v4l2-core/videobuf2-v4l2.c
>index 110fb45fef6f..8c322cd1b346 100644
>--- a/drivers/media/v4l2-core/videobuf2-v4l2.c
>+++ b/drivers/media/v4l2-core/videobuf2-v4l2.c
>@@ -23,6 +23,7 @@
> #include <linux/sched.h>
> #include <linux/freezer.h>
> #include <linux/kthread.h>
>+#include <linux/sync_file.h>
>
> #include <media/v4l2-dev.h>
> #include <media/v4l2-fh.h>
>@@ -178,6 +179,11 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b,
> 		return -EINVAL;
> 	}
>
>+	if ((b->fence_fd != -1) && !(b->flags & V4L2_BUF_FLAG_IN_FENCE)) {

 From what Hans was saying on Patch 2, it seems like this check needs
to be changed so as not to break old userspace which zeroes reserved2
and knows nothing about the fence flag.

I don't know if the value should be simply ignored in that case, or
only allow exactly the values -1 and 0.

Thanks,
-Brian

>+		dprintk(1, "%s: fence_fd set without IN_FENCE flag\n", opname);
>+		return -EINVAL;
>+	}
>+
> 	return __verify_planes_array(q->bufs[b->index], b);
> }
>
>@@ -203,9 +209,14 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, void *pb)
> 	b->timestamp = ns_to_timeval(vb->timestamp);
> 	b->timecode = vbuf->timecode;
> 	b->sequence = vbuf->sequence;
>-	b->fence_fd = -1;
> 	b->reserved = 0;
>
>+	b->fence_fd = -1;
>+	if (vb->in_fence)
>+		b->flags |= V4L2_BUF_FLAG_IN_FENCE;
>+	else
>+		b->flags &= ~V4L2_BUF_FLAG_IN_FENCE;
>+
> 	if (q->is_multiplanar) {
> 		/*
> 		 * Fill in plane-related data if userspace provided an array
>@@ -560,6 +571,7 @@ EXPORT_SYMBOL_GPL(vb2_create_bufs);
>
> int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
> {
>+	struct dma_fence *fence = NULL;
> 	int ret;
>
> 	if (vb2_fileio_is_active(q)) {
>@@ -568,7 +580,18 @@ int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
> 	}
>
> 	ret = vb2_queue_or_prepare_buf(q, b, "qbuf");
>-	return ret ? ret : vb2_core_qbuf(q, b->index, b);
>+	if (ret)
>+		return ret;
>+
>+	if (b->flags & V4L2_BUF_FLAG_IN_FENCE) {
>+		fence = sync_file_get_fence(b->fence_fd);
>+		if (!fence) {
>+			dprintk(1, "failed to get in-fence from fd\n");
>+			return -EINVAL;
>+		}
>+	}
>+
>+	return vb2_core_qbuf(q, b->index, b, fence);
> }
> EXPORT_SYMBOL_GPL(vb2_qbuf);
>
>diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
>index ef9b64398c8c..41cda762ff0a 100644
>--- a/include/media/videobuf2-core.h
>+++ b/include/media/videobuf2-core.h
>@@ -16,6 +16,7 @@
> #include <linux/mutex.h>
> #include <linux/poll.h>
> #include <linux/dma-buf.h>
>+#include <linux/dma-fence.h>
>
> #define VB2_MAX_FRAME	(32)
> #define VB2_MAX_PLANES	(8)
>@@ -254,11 +255,17 @@ struct vb2_buffer {
> 	 *			all buffers queued from userspace
> 	 * done_entry:		entry on the list that stores all buffers ready
> 	 *			to be dequeued to userspace
>+	 * in_fence:		fence receive from vb2 client to wait on before
>+	 *			using the buffer (queueing to the driver)
>+	 * fence_cb:		fence callback information
> 	 */
> 	enum vb2_buffer_state	state;
>
> 	struct list_head	queued_entry;
> 	struct list_head	done_entry;
>+
>+	struct dma_fence	*in_fence;
>+	struct dma_fence_cb	fence_cb;
> #ifdef CONFIG_VIDEO_ADV_DEBUG
> 	/*
> 	 * Counters for how often these buffer-related ops are
>@@ -726,6 +733,7 @@ int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb);
>  * @index:	id number of the buffer
>  * @pb:		buffer structure passed from userspace to vidioc_qbuf handler
>  *		in driver
>+ * @fence:	in-fence to wait on before queueing the buffer
>  *
>  * Should be called from vidioc_qbuf ioctl handler of a driver.
>  * The passed buffer should have been verified.
>@@ -740,7 +748,8 @@ int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb);
>  * The return values from this function are intended to be directly returned
>  * from vidioc_qbuf handler in driver.
>  */
>-int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb);
>+int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb,
>+		  struct dma_fence *fence);
>
> /**
>  * vb2_core_dqbuf() - Dequeue a buffer to the userspace
>-- 
>2.13.5
>

Re: [PATCH v3 10/15] [media] vb2: add 'ordered' property to queues

[Brian Starkey]

Hi,

On Thu, Sep 07, 2017 at 03:42:21PM -0300, Gustavo Padovan wrote:
>From: Gustavo Padovan <gustavo.padovan@collabora.com>
>
>For explicit synchronization (and soon for HAL3/Request API) we need
>the v4l2-driver to guarantee the ordering in which the buffers were queued
>by userspace. This is already true for many drivers, but we never needed
>to say it.
>
>Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
>---
> include/media/videobuf2-core.h | 4 ++++
> 1 file changed, 4 insertions(+)
>
>diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
>index 5ed8d3402474..20099dc22f26 100644
>--- a/include/media/videobuf2-core.h
>+++ b/include/media/videobuf2-core.h
>@@ -508,6 +508,9 @@ struct vb2_buf_ops {
>  * @last_buffer_dequeued: used in poll() and DQBUF to immediately return if the
>  *		last decoded buffer was already dequeued. Set for capture queues
>  *		when a buffer with the V4L2_BUF_FLAG_LAST is dequeued.
>+ * @ordered: if the driver can guarantee that the queue will be ordered or not.
>+ *		The default is not ordered unless the driver sets this flag. It
>+ *		is mandatory for using explicit fences.

If it's mandatory for fences (why is that?), then I guess this patch
should come before any of the fence implementation?

But it's not entirely clear to me what this flag means - ordered with
respect to what? Ordered such that the order in which the buffers are
queued in the driver are the same order that they will be dequeued by
userspace?

I think the order they are queued from userspace can still be
different from both the order they are queued in the driver (because
the in-fences can signal in any order), and dequeued again in
userspace, so "ordered" seems a bit ambiguous.

I think it should be more clear.

Cheers
-Brian

>  * @fileio:	file io emulator internal data, used only if emulator is active
>  * @threadio:	thread io internal data, used only if thread is active
>  */
>@@ -560,6 +563,7 @@ struct vb2_queue {
> 	unsigned int			is_output:1;
> 	unsigned int			copy_timestamp:1;
> 	unsigned int			last_buffer_dequeued:1;
>+	unsigned int			ordered:1;
>
> 	struct vb2_fileio_data		*fileio;
> 	struct vb2_threadio_data	*threadio;
>-- 
>2.13.5
>

Re: [PATCH v3 00/15] V4L2 Explicit Synchronization support

[Gustavo Padovan]

Hi Brian,

On Mon, 2017-10-02 at 14:41 +0100, Brian Starkey wrote:
> Hi Gustavo,
> 
> On Thu, Sep 07, 2017 at 03:42:11PM -0300, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo.padovan@collabora.com>
> > 
> > Hi,
> > 
> > Refer to the documentation on the first patch for the details. The
> > previous
> > iteration is here: https://www.mail-archive.com/linux-media@vger.ke
> > rnel.org/msg118077.html
> > 
> > The 2nd patch proposes an userspace API for fences, then on patch 3
> > we
> > prepare to the addition of in-fences in patch 4, by introducing the
> > infrastructure on vb2 to wait on an in-fence signal before queueing
> > the
> > buffer in the driver.
> > 
> > Patch 5 fix uvc v4l2 event handling and patch 6 configure q->dev
> > for
> > vivid drivers to enable to subscribe and dequeue events on it.
> > 
> > Patches 7-9 enables support to notify BUF_QUEUED events, the event
> > send
> > to userspace the out-fence fd and the index of the buffer that was
> > queued.
> > 
> > Patches 10-11 add support to mark queues as ordered. Finally
> > patches 12
> > and 13 add more fence infrastructure to support out-fences, patch
> > 13 exposes
> > close_fd() and patch 14 adds support to out-fences.
> > 
> > It only works for ordered queues for now, see open question at the
> > end
> > of the letter.
> > 
> > Test tool can be found at:
> > https://git.collabora.com/cgit/user/padovan/v4l2-test.git/
> > 
> > Main Changes
> > ------------
> > 
> > * out-fences: change in behavior: the out-fence fd now comes out of
> > the
> > BUF_QUEUED event along with the buffer id.
> 
> The more I think about this, the more unfortunate it seems.
> Especially
> for our use-case (m2m engine which sits in front of the display
> processor to convert the format of some buffers), having to wait for
> the in-fence to signal before we can get an out-fence removes a lot
> of
> the advantages of having fences at all.

Does your m2m driver ensures ordering between the buffer queued to it?

> 
> Ideally, we'd like to queue up our m2m work (while the GPU is still
> rendering that buffer, holding the in-fence), immediately get the
> out-fence for the m2m work, and pass that to DRM as the in-fence for
> display. With the current behaviour we need to wait in userspace
> before we can pass the buffer to display.
> 
> Wouldn't it be possible to enforce that the buffers aren't queued
> out-of-order in VB2? An easy way might be to (in qbuf) set a buffer's
> ->in_fence to be a fence_array of all the ->in_fences from the
> buffers
> before it in the queue (and its own). That would then naturally order
> the enqueue-ing in the driver, and allow you to return the out-fence
> immediately.
> 
> This would also solve your output devices question from below - a
> buffer can never get queued in the driver until all of the buffers
> which were QBUF'd before it are queued in the driver.

What you say makes sense, what this proposal lacks the most now is
feedback regarding its usecases. We can create a control setting to
enforce ordering in the queue, if it's set we create the fence arrays.
For output devices this should be set by default.

Gustavo

-- 
Gustavo Padovan
Principal Software Engineer
Collabora Ltd.

Re: [PATCH v3 02/15] [media] vb2: add explicit fence user API

[Gustavo Padovan]

On Mon, 2017-10-02 at 14:42 +0100, Brian Starkey wrote:
> Hi,
> 
> On Thu, Sep 07, 2017 at 03:42:13PM -0300, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo.padovan@collabora.com>
> > 
> > Turn the reserved2 field into fence_fd that we will use to send
> > an in-fence to the kernel and return an out-fence from the kernel
> > to
> > userspace.
> > 
> > Two new flags were added, V4L2_BUF_FLAG_IN_FENCE, that should be
> > used
> > when sending a fence to the kernel to be waited on, and
> > V4L2_BUF_FLAG_OUT_FENCE, to ask the kernel to give back an out-
> > fence.
> 
> It seems a bit off to me to add this to the uapi, and document it,
> before any of the implementation is present in the kernel.
> 
> Wouldn't it be better to move this patch to be the last one, after
> all
> of the implementation is done?

Yes, that seems a better idea.

Gustavo

Re: [PATCH v3 10/15] [media] vb2: add 'ordered' property to queues

[Gustavo Padovan]

On Mon, 2017-10-02 at 14:43 +0100, Brian Starkey wrote:
> Hi,
> 
> On Thu, Sep 07, 2017 at 03:42:21PM -0300, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo.padovan@collabora.com>
> > 
> > For explicit synchronization (and soon for HAL3/Request API) we
> > need
> > the v4l2-driver to guarantee the ordering in which the buffers were
> > queued
> > by userspace. This is already true for many drivers, but we never
> > needed
> > to say it.
> > 
> > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
> > ---
> > include/media/videobuf2-core.h | 4 ++++
> > 1 file changed, 4 insertions(+)
> > 
> > diff --git a/include/media/videobuf2-core.h
> > b/include/media/videobuf2-core.h
> > index 5ed8d3402474..20099dc22f26 100644
> > --- a/include/media/videobuf2-core.h
> > +++ b/include/media/videobuf2-core.h
> > @@ -508,6 +508,9 @@ struct vb2_buf_ops {
> >  * @last_buffer_dequeued: used in poll() and DQBUF to immediately
> > return if the
> >  *		last decoded buffer was already dequeued. Set for
> > capture queues
> >  *		when a buffer with the V4L2_BUF_FLAG_LAST is
> > dequeued.
> > + * @ordered: if the driver can guarantee that the queue will be
> > ordered or not.
> > + *		The default is not ordered unless the driver
> > sets this flag. It
> > + *		is mandatory for using explicit fences.
> 
> If it's mandatory for fences (why is that?), then I guess this patch
> should come before any of the fence implementation?

As of this implementation it is mandatory for out-fences, so that is
why I didn't put it before the in-fences support.

> 
> But it's not entirely clear to me what this flag means - ordered with
> respect to what? Ordered such that the order in which the buffers are
> queued in the driver are the same order that they will be dequeued by
> userspace?

Exactly.

> I think the order they are queued from userspace can still be
> different from both the order they are queued in the driver (because
> the in-fences can signal in any order), and dequeued again in
> userspace, so "ordered" seems a bit ambiguous.

Exactly. That is what the current implementation does.

> 
> I think it should be more clear.

Sure, sorry for not being clear, the next iteration will have a much
better explanation.

Gustavo

Re: [PATCH v3 00/15] V4L2 Explicit Synchronization support

[Brian Starkey]

Hi,

On Wed, Oct 04, 2017 at 05:08:55PM -0300, Gustavo Padovan wrote:
>Hi Brian,
>
>On Mon, 2017-10-02 at 14:41 +0100, Brian Starkey wrote:
>> Hi Gustavo,
>>
>> On Thu, Sep 07, 2017 at 03:42:11PM -0300, Gustavo Padovan wrote:
>> > From: Gustavo Padovan <gustavo.padovan@collabora.com>
>> >
>> > Hi,
>> >
>> > Refer to the documentation on the first patch for the details. The
>> > previous
>> > iteration is here: https://www.mail-archive.com/linux-media@vger.ke
>> > rnel.org/msg118077.html
>> >
>> > The 2nd patch proposes an userspace API for fences, then on patch 3
>> > we
>> > prepare to the addition of in-fences in patch 4, by introducing the
>> > infrastructure on vb2 to wait on an in-fence signal before queueing
>> > the
>> > buffer in the driver.
>> >
>> > Patch 5 fix uvc v4l2 event handling and patch 6 configure q->dev
>> > for
>> > vivid drivers to enable to subscribe and dequeue events on it.
>> >
>> > Patches 7-9 enables support to notify BUF_QUEUED events, the event
>> > send
>> > to userspace the out-fence fd and the index of the buffer that was
>> > queued.
>> >
>> > Patches 10-11 add support to mark queues as ordered. Finally
>> > patches 12
>> > and 13 add more fence infrastructure to support out-fences, patch
>> > 13 exposes
>> > close_fd() and patch 14 adds support to out-fences.
>> >
>> > It only works for ordered queues for now, see open question at the
>> > end
>> > of the letter.
>> >
>> > Test tool can be found at:
>> > https://git.collabora.com/cgit/user/padovan/v4l2-test.git/
>> >
>> > Main Changes
>> > ------------
>> >
>> > * out-fences: change in behavior: the out-fence fd now comes out of
>> > the
>> > BUF_QUEUED event along with the buffer id.
>>
>> The more I think about this, the more unfortunate it seems.
>> Especially
>> for our use-case (m2m engine which sits in front of the display
>> processor to convert the format of some buffers), having to wait for
>> the in-fence to signal before we can get an out-fence removes a lot
>> of
>> the advantages of having fences at all.
>
>Does your m2m driver ensures ordering between the buffer queued to it?
>

I'm not so familiar with the code, how can I check that?

>>
>> Ideally, we'd like to queue up our m2m work (while the GPU is still
>> rendering that buffer, holding the in-fence), immediately get the
>> out-fence for the m2m work, and pass that to DRM as the in-fence for
>> display. With the current behaviour we need to wait in userspace
>> before we can pass the buffer to display.
>>
>> Wouldn't it be possible to enforce that the buffers aren't queued
>> out-of-order in VB2? An easy way might be to (in qbuf) set a buffer's
>> ->in_fence to be a fence_array of all the ->in_fences from the
>> buffers
>> before it in the queue (and its own). That would then naturally order
>> the enqueue-ing in the driver, and allow you to return the out-fence
>> immediately.
>>
>> This would also solve your output devices question from below - a
>> buffer can never get queued in the driver until all of the buffers
>> which were QBUF'd before it are queued in the driver.
>
>What you say makes sense, what this proposal lacks the most now is
>feedback regarding its usecases. We can create a control setting to
>enforce ordering in the queue, if it's set we create the fence arrays.
>For output devices this should be set by default.

Yeah that could work. I can see that in some cases queueing
out-of-order as the fences signal would be the right thing to do, so
makes sense to allow both.

Thanks,
-Brian

>
>Gustavo
>
>-- 
>Gustavo Padovan
>Principal Software Engineer
>Collabora Ltd.