[PATCH 00/12] V4L2 explicit synchronization support

[PATCH 00/12] V4L2 explicit synchronization support

[Gustavo Padovan]

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

Hi,

This adds support for Explicit Synchronization of shared buffers in V4L2.
It uses the Sync File Framework[1] as vector to communicate the fences
between kernel and userspace.

Explicit Synchronization allows us to control the synchronization of
shared buffers from userspace by passing fences to the kernel and/or 
receiving them from the the kernel.

Fences passed to the kernel are named in-fences and the kernel should wait
them to signal before using the buffer. On the other side, the kernel creates
out-fences for every buffer it receives from userspace. This fence is sent back
to userspace and it will signal when the capture, for example, has finished.

Signalling an out-fence in V4L2 would mean that the job on the buffer is done
and the buffer can be used by other drivers.

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

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

Patches 6-7 enables support to notify BUF_QUEUED events, i.e., let userspace
know that particular buffer was enqueued in the driver. This is needed,
because we return the out-fence fd as an out argument in QBUF, but at the time
it returns we don't know to which buffer the fence will be attached thus
the BUF_QUEUED event tells which buffer is associated to the fence received in
QBUF by userspace.

Patches 8-9 add support to mark queues as ordered. Finally patches 10 and 11
add more fence infrastructure to support out-fences and finally patch 12 adds
support to out-fences.

Changelog are detailed in each patch.

Please review! Thanks.

Gustavo

[1] drivers/dma-buf/sync_file.c
---
Gustavo Padovan (11):
  [media] vb2: add explicit fence user API
  [media] vb2: split out queueing from vb_core_qbuf()
  [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] 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
  [media] vb2: add out-fence support to QBUF

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

 drivers/media/Kconfig                         |   1 +
 drivers/media/platform/vivid/vivid-core.c     |  15 ++-
 drivers/media/usb/uvc/uvc_v4l2.c              |   2 +-
 drivers/media/v4l2-core/v4l2-compat-ioctl32.c |   4 +-
 drivers/media/v4l2-core/v4l2-ctrls.c          |   6 +-
 drivers/media/v4l2-core/videobuf2-core.c      | 172 ++++++++++++++++++++++----
 drivers/media/v4l2-core/videobuf2-v4l2.c      |  37 +++++-
 include/media/videobuf2-core.h                |  16 ++-
 include/media/videobuf2-fence.h               |  49 ++++++++
 include/uapi/linux/videodev2.h                |  10 +-
 10 files changed, 272 insertions(+), 40 deletions(-)
 create mode 100644 include/media/videobuf2-fence.h

-- 
2.9.4

[PATCH 01/12] [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.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
---
 drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 4 ++--
 drivers/media/v4l2-core/videobuf2-v4l2.c      | 2 +-
 include/uapi/linux/videodev2.h                | 4 +++-
 3 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
index 6f52970..8f6ab85 100644
--- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
+++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
@@ -367,7 +367,7 @@ struct v4l2_buffer32 {
 		__s32		fd;
 	} m;
 	__u32			length;
-	__u32			reserved2;
+	__s32			fence_fd;
 	__u32			reserved;
 };
 
@@ -530,7 +530,7 @@ 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->fence_fd, &up->fence_fd) ||
 		put_user(kp->reserved, &up->reserved) ||
 		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 0c06699..110fb45 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 2b8feb8..750d511 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -916,7 +916,7 @@ struct v4l2_buffer {
 		__s32		fd;
 	} m;
 	__u32			length;
-	__u32			reserved2;
+	__s32			fence_fd;
 	__u32			reserved;
 };
 
@@ -953,6 +953,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.9.4

[PATCH 02/12] [media] vb2: split out queueing from vb_core_qbuf()

[Gustavo Padovan]

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

In order to support explicit synchronization we need to divide
vb2_core_qbuf() in two parts, one to be executed before the fence
signals and another one to do the actual queueing of the buffer.

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

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 3107e21..ea83126 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -1367,6 +1367,34 @@ static int vb2_start_streaming(struct vb2_queue *q)
 	return ret;
 }
 
+static int __vb2_core_qbuf(struct vb2_buffer *vb, struct vb2_queue *q)
+{
+	int ret;
+
+	/*
+	 * 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);
+
+	/*
+	 * 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 (q->streaming && !q->start_streaming_called &&
+	    q->queued_count >= q->min_buffers_needed) {
+		ret = vb2_start_streaming(q);
+		if (ret)
+			return ret;
+	}
+
+	dprintk(1, "qbuf of buffer %d succeeded\n", vb->index);
+	return 0;
+}
+
 int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
 {
 	struct vb2_buffer *vb;
@@ -1404,32 +1432,11 @@ 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 (q->streaming && !q->start_streaming_called &&
-	    q->queued_count >= q->min_buffers_needed) {
-		ret = vb2_start_streaming(q);
-		if (ret)
-			return ret;
-	}
-
-	dprintk(1, "qbuf of buffer %d succeeded\n", vb->index);
-	return 0;
+	return __vb2_core_qbuf(vb, q);
 }
 EXPORT_SYMBOL_GPL(vb2_core_qbuf);
 
-- 
2.9.4

[PATCH 03/12] [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.

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/Kconfig                    |  1 +
 drivers/media/v4l2-core/videobuf2-core.c | 97 +++++++++++++++++++++++++-------
 drivers/media/v4l2-core/videobuf2-v4l2.c | 15 ++++-
 include/media/videobuf2-core.h           |  7 ++-
 4 files changed, 99 insertions(+), 21 deletions(-)

diff --git a/drivers/media/Kconfig b/drivers/media/Kconfig
index 55d9c2b..3cd1d3d 100644
--- a/drivers/media/Kconfig
+++ b/drivers/media/Kconfig
@@ -11,6 +11,7 @@ config CEC_NOTIFIER
 menuconfig MEDIA_SUPPORT
 	tristate "Multimedia support"
 	depends on HAS_IOMEM
+	select SYNC_FILE
 	help
 	  If you want to use Webcams, Video grabber devices and/or TV devices
 	  enable this option and other options below.
diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index ea83126..29aa9d4 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -1279,6 +1279,22 @@ 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))
+			break;
+
+		ready_count++;
+	}
+
+	return ready_count;
+}
+
 int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb)
 {
 	struct vb2_buffer *vb;
@@ -1324,8 +1340,15 @@ static int vb2_start_streaming(struct vb2_queue *q)
 	 * If any buffers were queued before streamon,
 	 * we can now pass them to driver for processing.
 	 */
-	list_for_each_entry(vb, &q->queued_list, queued_entry)
+	list_for_each_entry(vb, &q->queued_list, queued_entry) {
+		if (vb->state != VB2_BUF_STATE_QUEUED)
+			continue;
+
+		if (vb->in_fence && !dma_fence_is_signaled(vb->in_fence))
+			break;
+
 		__enqueue_in_driver(vb);
+	}
 
 	/* Tell the driver to start streaming */
 	q->start_streaming_called = 1;
@@ -1369,33 +1392,55 @@ static int vb2_start_streaming(struct vb2_queue *q)
 
 static int __vb2_core_qbuf(struct vb2_buffer *vb, struct vb2_queue *q)
 {
+	struct vb2_buffer *b;
 	int ret;
 
 	/*
 	 * 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);
 
-	/*
-	 * 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 (q->streaming && !q->start_streaming_called &&
-	    q->queued_count >= q->min_buffers_needed) {
-		ret = vb2_start_streaming(q);
-		if (ret)
-			return ret;
+	if (q->start_streaming_called) {
+		list_for_each_entry(b, &q->queued_list, queued_entry) {
+			if (b->state != VB2_BUF_STATE_QUEUED)
+				continue;
+
+			if (b->in_fence && !dma_fence_is_signaled(b->in_fence))
+				break;
+
+			__enqueue_in_driver(b);
+		}
+	} else {
+		/*
+		 * 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
+		 * that are ready, then we can finally call start_streaming().
+		 */
+		if (q->streaming &&
+		    __get_num_ready_buffers(q) >= q->min_buffers_needed) {
+			ret = vb2_start_streaming(q);
+			if (ret)
+				return ret;
+		}
 	}
 
 	dprintk(1, "qbuf of buffer %d succeeded\n", vb->index);
 	return 0;
 }
 
-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;
+
+	__vb2_core_qbuf(vb, vb->vb2_queue);
+}
+
+int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb,
+		  struct dma_fence *fence)
 {
 	struct vb2_buffer *vb;
 	int ret;
@@ -1436,6 +1481,11 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
 	if (pb)
 		call_void_bufop(q, fill_user_buffer, vb, pb);
 
+	vb->in_fence = fence;
+	if (fence && !dma_fence_add_callback(fence, &vb->fence_cb,
+					     vb2_qbuf_fence_cb))
+		return 0;
+
 	return __vb2_core_qbuf(vb, q);
 }
 EXPORT_SYMBOL_GPL(vb2_core_qbuf);
@@ -1647,6 +1697,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
@@ -1669,6 +1720,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;
@@ -1735,7 +1794,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;
@@ -2250,7 +2309,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;
@@ -2429,7 +2488,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;
@@ -2532,7 +2591,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 110fb45..e6ad77f 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>
@@ -560,6 +561,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 +570,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 ret ? ret : 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 cb97c22..aa43e43 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)
@@ -259,6 +260,9 @@ struct vb2_buffer {
 
 	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
@@ -727,7 +731,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.9.4

[PATCH 04/12] [media] uvc: enable subscriptions to other events

[Gustavo Padovan]

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

Call v4l2_ctrl_subscribe_event to subscribe to more events supported by
v4l.

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 3e7e283..dfa0ccd 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.9.4

[PATCH 05/12] [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.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.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 ef344b9..8843170 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.9.4

[PATCH 06/12] [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.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
---
 include/uapi/linux/videodev2.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 750d511..c2eda75 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -2150,6 +2150,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 */
@@ -2202,6 +2203,10 @@ struct v4l2_event_motion_det {
 	__u32 region_mask;
 };
 
+struct v4l2_event_buf_queued {
+	__u32 index;
+};
+
 struct v4l2_event {
 	__u32				type;
 	union {
@@ -2210,6 +2215,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.9.4

[PATCH 07/12] [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.

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

diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index 5aed7bd..f55b5da 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -3435,8 +3435,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, 0, 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 29aa9d4..00d9c35 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 <media/videobuf2-core.h>
+#include <media/v4l2-event.h>
 #include <media/v4l2-mc.h>
 
 #include <trace/events/vb2.h>
@@ -1221,6 +1222,18 @@ static int __prepare_dmabuf(struct vb2_buffer *vb, const void *pb)
 	return ret;
 }
 
+static void vb2_buffer_queued_event(struct vb2_buffer *vb)
+{
+	struct video_device *vdev = to_video_device(vb->vb2_queue->dev);
+	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(vdev, &event);
+}
+
 /**
  * __enqueue_in_driver() - enqueue a vb2_buffer in driver for processing
  */
@@ -1234,6 +1247,8 @@ static void __enqueue_in_driver(struct vb2_buffer *vb)
 	trace_vb2_buf_queue(q, vb);
 
 	call_void_vb_qop(vb, buf_queue, vb);
+
+	vb2_buffer_queued_event(vb);
 }
 
 static int __buf_prepare(struct vb2_buffer *vb, const void *pb)
-- 
2.9.4

[PATCH 08/12] [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 which the buffer were queued
by userspace. This is already true for many drivers, but we never had
the need 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 aa43e43..a8b800e 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -491,6 +491,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
  */
@@ -541,6 +544,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.9.4

[PATCH 09/12] [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.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.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 8843170..c7bef90 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.9.4

[PATCH 10/12] [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 0000000..ed5612c
--- /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.9.4

[PATCH 11/12] [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 | 31 +++++++++++++++++++++++++++++++
 include/media/videobuf2-core.h           |  5 +++++
 2 files changed, 36 insertions(+)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 00d9c35..21cc4ed 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-event.h>
 #include <media/v4l2-mc.h>
 
@@ -1335,6 +1338,34 @@ 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, unsigned int index)
+{
+	struct vb2_buffer *vb = q->bufs[index];
+
+	vb->out_fence_fd = get_unused_fd_flags(O_CLOEXEC);
+	if (vb->out_fence_fd < 0)
+		return vb->out_fence_fd;
+
+	vb->out_fence = vb2_fence_alloc();
+	if (!vb->out_fence)
+		goto err;
+
+	vb->sync_file = sync_file_create(vb->out_fence);
+	if (!vb->sync_file) {
+		dma_fence_put(vb->out_fence);
+		vb->out_fence = NULL;
+		goto err;
+	}
+
+	return 0;
+
+err:
+	put_unused_fd(vb->out_fence_fd);
+	vb->out_fence_fd = -1;
+	return -ENOMEM;
+}
+EXPORT_SYMBOL_GPL(vb2_setup_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 a8b800e..5f3e3eb 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -263,6 +263,10 @@ struct vb2_buffer {
 
 	struct dma_fence	*in_fence;
 	struct dma_fence_cb	fence_cb;
+
+	struct dma_fence	*out_fence;
+	struct sync_file	*sync_file;
+	int			out_fence_fd;
 #ifdef CONFIG_VIDEO_ADV_DEBUG
 	/*
 	 * Counters for how often these buffer-related ops are
@@ -714,6 +718,7 @@ 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);
 
+int vb2_setup_out_fence(struct vb2_queue *q, unsigned int index);
 /**
  * vb2_core_qbuf() - Queue a buffer from userspace
  *
-- 
2.9.4

[PATCH 12/12] [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 for the buffer and return it to userspace on the fence_fd
field. It only works with ordered queues.

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

v2: check if the queue is ordered.

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

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 21cc4ed..a57902ee 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -356,6 +356,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 */
@@ -940,6 +941,11 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state)
 			__enqueue_in_driver(vb);
 		return;
 	default:
+		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;
diff --git a/drivers/media/v4l2-core/videobuf2-v4l2.c b/drivers/media/v4l2-core/videobuf2-v4l2.c
index e6ad77f..e2733dd 100644
--- a/drivers/media/v4l2-core/videobuf2-v4l2.c
+++ b/drivers/media/v4l2-core/videobuf2-v4l2.c
@@ -204,9 +204,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->fence_fd = vb->out_fence_fd;
 	b->reserved = 0;
 
+	if (vb->sync_file) {
+		fd_install(vb->out_fence_fd, vb->sync_file->file);
+		vb->sync_file = NULL;
+	}
+
 	if (q->is_multiplanar) {
 		/*
 		 * Fill in plane-related data if userspace provided an array
@@ -581,6 +586,21 @@ int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
 		}
 	}
 
+	if (b->flags & V4L2_BUF_FLAG_OUT_FENCE) {
+		if (!q->ordered) {
+			dprintk(1, "can't use out-fences with unordered queues\n");
+			dma_fence_put(fence);
+			return -EINVAL;
+		}
+
+		ret = vb2_setup_out_fence(q, b->index);
+		if (ret) {
+			dprintk(1, "failed to set up out-fence\n");
+			dma_fence_put(fence);
+			return ret;
+		}
+	}
+
 	return ret ? ret : vb2_core_qbuf(q, b->index, b, fence);
 }
 EXPORT_SYMBOL_GPL(vb2_qbuf);
-- 
2.9.4

Re: [PATCH 08/12] [media] vb2: add 'ordered' property to queues

[Nicolas Dufresne]

[-- Attachment #1: Type: text/plain, Size: 2882 bytes --]

Le vendredi 16 juin 2017 à 16:39 +0900, Gustavo Padovan a écrit :
> > 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 which the buffer were queued
> by userspace. This is already true for many drivers, but we never had
> the need to say it.

Phrased this way, that sound like a statement that a m2m decoder
handling b-frame will just never be supported. I think decoders are a
very important use case for explicit synchronization.

What I believe happens with decoders is simply that the allocation
order (the order in which empty buffers are retrieved from the queue)
will be different then the actual presentation order. Also, multiple
buffers endup being filled at the same time. Some firmware may inform
of the new order at the last minute, making indeed the fence useless,
but these are firmware and the information can be known earlier. Also,
this information would be known by userspace for the case (up-coming,
see STM patches and Rockchip comments [0]) or state-less decoder,
because it is available while parsing the bitstream. For this last
scenarios, the fact that ordering is not the same should disable the
fences since userspace can know which fences to wait for first. Those
drivers would need to set "ordered" to 0, which would be counter
intuitive.

I think this use case is too important to just ignore it. I would
expect that we at least have a todo with something sensible as a plan
to cover this.

> 
> > 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 aa43e43..a8b800e 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -491,6 +491,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
>   */
> @@ -541,6 +544,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;

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 01/12] [media] vb2: add explicit fence user API

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2604 bytes --]

Hi Gustavo,

[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on v4.12-rc5 next-20170616]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Gustavo-Padovan/vb2-add-explicit-fence-user-API/20170618-210740
base:   git://linuxtv.org/media_tree.git master
config: sh-allmodconfig (attached as .config)
compiler: sh4-linux-gnu-gcc (Debian 6.3.0-18) 6.3.0 20170516
reproduce:
        wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=sh 

All errors (new ones prefixed by >>):

   drivers/media//usb/cpia2/cpia2_v4l.c: In function 'cpia2_dqbuf':
>> drivers/media//usb/cpia2/cpia2_v4l.c:951:5: error: 'struct v4l2_buffer' has no member named 'reserved2'; did you mean 'reserved'?
     buf->reserved2 = 0;
        ^~

vim +951 drivers/media//usb/cpia2/cpia2_v4l.c

1b18e7a0 drivers/media/usb/cpia2/cpia2_v4l.c   Sakari Ailus 2012-10-22  945  		| V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
ab33d507 drivers/media/video/cpia2/cpia2_v4l.c Alan Cox     2006-02-27  946  	buf->field = V4L2_FIELD_NONE;
ab33d507 drivers/media/video/cpia2/cpia2_v4l.c Alan Cox     2006-02-27  947  	buf->timestamp = cam->buffers[buf->index].timestamp;
ab33d507 drivers/media/video/cpia2/cpia2_v4l.c Alan Cox     2006-02-27  948  	buf->sequence = cam->buffers[buf->index].seq;
ab33d507 drivers/media/video/cpia2/cpia2_v4l.c Alan Cox     2006-02-27  949  	buf->m.offset = cam->buffers[buf->index].data - cam->frame_buffer;
ab33d507 drivers/media/video/cpia2/cpia2_v4l.c Alan Cox     2006-02-27  950  	buf->length = cam->frame_size;
2b719d7b drivers/media/video/cpia2/cpia2_v4l.c Sakari Ailus 2012-05-02 @951  	buf->reserved2 = 0;
ab33d507 drivers/media/video/cpia2/cpia2_v4l.c Alan Cox     2006-02-27  952  	buf->reserved = 0;
ab33d507 drivers/media/video/cpia2/cpia2_v4l.c Alan Cox     2006-02-27  953  	memset(&buf->timecode, 0, sizeof(buf->timecode));
ab33d507 drivers/media/video/cpia2/cpia2_v4l.c Alan Cox     2006-02-27  954  

:::::: The code at line 951 was first introduced by commit
:::::: 2b719d7baf490e24ce7d817c6337b7c87fda84c1 [media] v4l: drop v4l2_buffer.input and V4L2_BUF_FLAG_INPUT

:::::: TO: Sakari Ailus <sakari.ailus@iki.fi>
:::::: CC: Mauro Carvalho Chehab <mchehab@redhat.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 45293 bytes --]

Re: [PATCH 01/12] [media] vb2: add explicit fence user API

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 17552 bytes --]

Hi Gustavo,

[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on v4.12-rc5 next-20170616]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Gustavo-Padovan/vb2-add-explicit-fence-user-API/20170618-210740
base:   git://linuxtv.org/media_tree.git master
config: x86_64-allmodconfig (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All error/warnings (new ones prefixed by >>):

   drivers/staging/media//atomisp/pci/atomisp2/atomisp_ioctl.c: In function 'atomisp_qbuf':
>> drivers/staging/media//atomisp/pci/atomisp2/atomisp_ioctl.c:1297:10: error: 'struct v4l2_buffer' has no member named 'reserved2'; did you mean 'reserved'?
         (buf->reserved2 & ATOMISP_BUFFER_HAS_PER_FRAME_SETTING)) {
             ^~
   drivers/staging/media//atomisp/pci/atomisp2/atomisp_ioctl.c:1299:50: error: 'struct v4l2_buffer' has no member named 'reserved2'; did you mean 'reserved'?
      pipe->frame_request_config_id[buf->index] = buf->reserved2 &
                                                     ^~
   drivers/staging/media//atomisp/pci/atomisp2/atomisp_ioctl.c: In function 'atomisp_dqbuf':
   drivers/staging/media//atomisp/pci/atomisp2/atomisp_ioctl.c:1483:5: error: 'struct v4l2_buffer' has no member named 'reserved2'; did you mean 'reserved'?
     buf->reserved2 = pipe->frame_config_id[buf->index];
        ^~
   In file included from include/linux/printk.h:329:0,
                    from include/linux/kernel.h:13,
                    from include/linux/delay.h:21,
                    from drivers/staging/media//atomisp/pci/atomisp2/atomisp_ioctl.c:24:
   drivers/staging/media//atomisp/pci/atomisp2/atomisp_ioctl.c:1488:6: error: 'struct v4l2_buffer' has no member named 'reserved2'; did you mean 'reserved'?
      buf->reserved2);
         ^
   include/linux/dynamic_debug.h:135:9: note: in definition of macro 'dynamic_dev_dbg'
          ##__VA_ARGS__);  \
            ^~~~~~~~~~~
>> drivers/staging/media//atomisp/pci/atomisp2/atomisp_ioctl.c:1486:2: note: in expansion of macro 'dev_dbg'
     dev_dbg(isp->dev, "dqbuf buffer %d (%s) for asd%d with exp_id %d, isp_config_id %d\n",
     ^~~~~~~
   drivers/staging/media//atomisp/pci/atomisp2/atomisp_ioctl.c: At top level:
>> cc1: warning: unrecognized command line option '-Wno-implicit-fallthrough'
--
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_ioctl.c: In function 'atomisp_qbuf':
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_ioctl.c:1297:10: error: 'struct v4l2_buffer' has no member named 'reserved2'; did you mean 'reserved'?
         (buf->reserved2 & ATOMISP_BUFFER_HAS_PER_FRAME_SETTING)) {
             ^~
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_ioctl.c:1299:50: error: 'struct v4l2_buffer' has no member named 'reserved2'; did you mean 'reserved'?
      pipe->frame_request_config_id[buf->index] = buf->reserved2 &
                                                     ^~
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_ioctl.c: In function 'atomisp_dqbuf':
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_ioctl.c:1483:5: error: 'struct v4l2_buffer' has no member named 'reserved2'; did you mean 'reserved'?
     buf->reserved2 = pipe->frame_config_id[buf->index];
        ^~
   In file included from include/linux/printk.h:329:0,
                    from include/linux/kernel.h:13,
                    from include/linux/delay.h:21,
                    from drivers/staging/media/atomisp/pci/atomisp2/atomisp_ioctl.c:24:
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_ioctl.c:1488:6: error: 'struct v4l2_buffer' has no member named 'reserved2'; did you mean 'reserved'?
      buf->reserved2);
         ^
   include/linux/dynamic_debug.h:135:9: note: in definition of macro 'dynamic_dev_dbg'
          ##__VA_ARGS__);  \
            ^~~~~~~~~~~
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_ioctl.c:1486:2: note: in expansion of macro 'dev_dbg'
     dev_dbg(isp->dev, "dqbuf buffer %d (%s) for asd%d with exp_id %d, isp_config_id %d\n",
     ^~~~~~~
   drivers/staging/media/atomisp/pci/atomisp2/atomisp_ioctl.c: At top level:
>> cc1: warning: unrecognized command line option '-Wno-implicit-fallthrough'

vim +1297 drivers/staging/media//atomisp/pci/atomisp2/atomisp_ioctl.c

a49d2536 Alan Cox 2017-02-17  1291  
a49d2536 Alan Cox 2017-02-17  1292  done:
a49d2536 Alan Cox 2017-02-17  1293  	if (!((buf->flags & NOFLUSH_FLAGS) == NOFLUSH_FLAGS))
a49d2536 Alan Cox 2017-02-17  1294  		wbinvd();
a49d2536 Alan Cox 2017-02-17  1295  
a49d2536 Alan Cox 2017-02-17  1296  	if (!atomisp_is_vf_pipe(pipe) &&
a49d2536 Alan Cox 2017-02-17 @1297  	    (buf->reserved2 & ATOMISP_BUFFER_HAS_PER_FRAME_SETTING)) {
a49d2536 Alan Cox 2017-02-17  1298  		/* this buffer will have a per-frame parameter */
a49d2536 Alan Cox 2017-02-17  1299  		pipe->frame_request_config_id[buf->index] = buf->reserved2 &
a49d2536 Alan Cox 2017-02-17  1300  					~ATOMISP_BUFFER_HAS_PER_FRAME_SETTING;
a49d2536 Alan Cox 2017-02-17  1301  		dev_dbg(isp->dev, "This buffer requires per_frame setting which has isp_config_id %d\n",
a49d2536 Alan Cox 2017-02-17  1302  			pipe->frame_request_config_id[buf->index]);
a49d2536 Alan Cox 2017-02-17  1303  	} else {
a49d2536 Alan Cox 2017-02-17  1304  		pipe->frame_request_config_id[buf->index] = 0;
a49d2536 Alan Cox 2017-02-17  1305  	}
a49d2536 Alan Cox 2017-02-17  1306  
a49d2536 Alan Cox 2017-02-17  1307  	pipe->frame_params[buf->index] = NULL;
a49d2536 Alan Cox 2017-02-17  1308  
a49d2536 Alan Cox 2017-02-17  1309  	rt_mutex_unlock(&isp->mutex);
a49d2536 Alan Cox 2017-02-17  1310  
a49d2536 Alan Cox 2017-02-17  1311  	ret = videobuf_qbuf(&pipe->capq, buf);
a49d2536 Alan Cox 2017-02-17  1312  	rt_mutex_lock(&isp->mutex);
a49d2536 Alan Cox 2017-02-17  1313  	if (ret)
a49d2536 Alan Cox 2017-02-17  1314  		goto error;
a49d2536 Alan Cox 2017-02-17  1315  
a49d2536 Alan Cox 2017-02-17  1316  	/* TODO: do this better, not best way to queue to css */
a49d2536 Alan Cox 2017-02-17  1317  	if (asd->streaming == ATOMISP_DEVICE_STREAMING_ENABLED) {
a49d2536 Alan Cox 2017-02-17  1318  		if (!list_empty(&pipe->buffers_waiting_for_param)) {
a49d2536 Alan Cox 2017-02-17  1319  			atomisp_handle_parameter_and_buffer(pipe);
a49d2536 Alan Cox 2017-02-17  1320  		} else {
a49d2536 Alan Cox 2017-02-17  1321  			atomisp_qbuffers_to_css(asd);
a49d2536 Alan Cox 2017-02-17  1322  
a49d2536 Alan Cox 2017-02-17  1323  #ifndef ISP2401
a49d2536 Alan Cox 2017-02-17  1324  			if (!atomisp_is_wdt_running(asd) && atomisp_buffers_queued(asd))
a49d2536 Alan Cox 2017-02-17  1325  				atomisp_wdt_start(asd);
a49d2536 Alan Cox 2017-02-17  1326  #else
a49d2536 Alan Cox 2017-02-17  1327  			if (!atomisp_is_wdt_running(pipe) &&
a49d2536 Alan Cox 2017-02-17  1328  				atomisp_buffers_queued_pipe(pipe))
a49d2536 Alan Cox 2017-02-17  1329  				atomisp_wdt_start(pipe);
a49d2536 Alan Cox 2017-02-17  1330  #endif
a49d2536 Alan Cox 2017-02-17  1331  		}
a49d2536 Alan Cox 2017-02-17  1332  	}
a49d2536 Alan Cox 2017-02-17  1333  
a49d2536 Alan Cox 2017-02-17  1334  	/* Workaround: Due to the design of HALv3,
a49d2536 Alan Cox 2017-02-17  1335  	 * sometimes in ZSL or SDV mode HAL needs to
a49d2536 Alan Cox 2017-02-17  1336  	 * capture multiple images within one streaming cycle.
a49d2536 Alan Cox 2017-02-17  1337  	 * But the capture number cannot be determined by HAL.
a49d2536 Alan Cox 2017-02-17  1338  	 * So HAL only sets the capture number to be 1 and queue multiple
a49d2536 Alan Cox 2017-02-17  1339  	 * buffers. Atomisp driver needs to check this case and re-trigger
a49d2536 Alan Cox 2017-02-17  1340  	 * CSS to do capture when new buffer is queued. */
a49d2536 Alan Cox 2017-02-17  1341  	if (asd->continuous_mode->val &&
a49d2536 Alan Cox 2017-02-17  1342  	    atomisp_subdev_source_pad(vdev)
a49d2536 Alan Cox 2017-02-17  1343  	    == ATOMISP_SUBDEV_PAD_SOURCE_CAPTURE &&
a49d2536 Alan Cox 2017-02-17  1344  	    pipe->capq.streaming &&
a49d2536 Alan Cox 2017-02-17  1345  	    !asd->enable_raw_buffer_lock->val &&
a49d2536 Alan Cox 2017-02-17  1346  	    asd->params.offline_parm.num_captures == 1) {
a49d2536 Alan Cox 2017-02-17  1347  #ifndef ISP2401
a49d2536 Alan Cox 2017-02-17  1348  		asd->pending_capture_request++;
a49d2536 Alan Cox 2017-02-17  1349  		dev_dbg(isp->dev, "Add one pending capture request.\n");
a49d2536 Alan Cox 2017-02-17  1350  #else
a49d2536 Alan Cox 2017-02-17  1351  	    if (asd->re_trigger_capture) {
a49d2536 Alan Cox 2017-02-17  1352  			ret = atomisp_css_offline_capture_configure(asd,
a49d2536 Alan Cox 2017-02-17  1353  				asd->params.offline_parm.num_captures,
a49d2536 Alan Cox 2017-02-17  1354  				asd->params.offline_parm.skip_frames,
a49d2536 Alan Cox 2017-02-17  1355  				asd->params.offline_parm.offset);
a49d2536 Alan Cox 2017-02-17  1356  			asd->re_trigger_capture = false;
a49d2536 Alan Cox 2017-02-17  1357  			dev_dbg(isp->dev, "%s Trigger capture again ret=%d\n",
a49d2536 Alan Cox 2017-02-17  1358  				__func__, ret);
a49d2536 Alan Cox 2017-02-17  1359  
a49d2536 Alan Cox 2017-02-17  1360  	    } else {
a49d2536 Alan Cox 2017-02-17  1361  			asd->pending_capture_request++;
a49d2536 Alan Cox 2017-02-17  1362  			asd->re_trigger_capture = false;
a49d2536 Alan Cox 2017-02-17  1363  			dev_dbg(isp->dev, "Add one pending capture request.\n");
a49d2536 Alan Cox 2017-02-17  1364  	    }
a49d2536 Alan Cox 2017-02-17  1365  #endif
a49d2536 Alan Cox 2017-02-17  1366  	}
a49d2536 Alan Cox 2017-02-17  1367  	rt_mutex_unlock(&isp->mutex);
a49d2536 Alan Cox 2017-02-17  1368  
a49d2536 Alan Cox 2017-02-17  1369  	dev_dbg(isp->dev, "qbuf buffer %d (%s) for asd%d\n", buf->index,
a49d2536 Alan Cox 2017-02-17  1370  		vdev->name, asd->index);
a49d2536 Alan Cox 2017-02-17  1371  
a49d2536 Alan Cox 2017-02-17  1372  	return ret;
a49d2536 Alan Cox 2017-02-17  1373  
a49d2536 Alan Cox 2017-02-17  1374  error:
a49d2536 Alan Cox 2017-02-17  1375  	rt_mutex_unlock(&isp->mutex);
a49d2536 Alan Cox 2017-02-17  1376  	return ret;
a49d2536 Alan Cox 2017-02-17  1377  }
a49d2536 Alan Cox 2017-02-17  1378  
a49d2536 Alan Cox 2017-02-17  1379  static int atomisp_qbuf_file(struct file *file, void *fh,
a49d2536 Alan Cox 2017-02-17  1380  					struct v4l2_buffer *buf)
a49d2536 Alan Cox 2017-02-17  1381  {
a49d2536 Alan Cox 2017-02-17  1382  	struct video_device *vdev = video_devdata(file);
a49d2536 Alan Cox 2017-02-17  1383  	struct atomisp_device *isp = video_get_drvdata(vdev);
a49d2536 Alan Cox 2017-02-17  1384  	struct atomisp_video_pipe *pipe = atomisp_to_video_pipe(vdev);
a49d2536 Alan Cox 2017-02-17  1385  	int ret;
a49d2536 Alan Cox 2017-02-17  1386  
a49d2536 Alan Cox 2017-02-17  1387  	rt_mutex_lock(&isp->mutex);
a49d2536 Alan Cox 2017-02-17  1388  	if (isp->isp_fatal_error) {
a49d2536 Alan Cox 2017-02-17  1389  		ret = -EIO;
a49d2536 Alan Cox 2017-02-17  1390  		goto error;
a49d2536 Alan Cox 2017-02-17  1391  	}
a49d2536 Alan Cox 2017-02-17  1392  
a49d2536 Alan Cox 2017-02-17  1393  	if (!buf || buf->index >= VIDEO_MAX_FRAME ||
a49d2536 Alan Cox 2017-02-17  1394  		!pipe->outq.bufs[buf->index]) {
a49d2536 Alan Cox 2017-02-17  1395  		dev_err(isp->dev, "Invalid index for qbuf.\n");
a49d2536 Alan Cox 2017-02-17  1396  		ret = -EINVAL;
a49d2536 Alan Cox 2017-02-17  1397  		goto error;
a49d2536 Alan Cox 2017-02-17  1398  	}
a49d2536 Alan Cox 2017-02-17  1399  
a49d2536 Alan Cox 2017-02-17  1400  	if (buf->memory != V4L2_MEMORY_MMAP) {
a49d2536 Alan Cox 2017-02-17  1401  		dev_err(isp->dev, "Unsupported memory method\n");
a49d2536 Alan Cox 2017-02-17  1402  		ret = -EINVAL;
a49d2536 Alan Cox 2017-02-17  1403  		goto error;
a49d2536 Alan Cox 2017-02-17  1404  	}
a49d2536 Alan Cox 2017-02-17  1405  
a49d2536 Alan Cox 2017-02-17  1406  	if (buf->type != V4L2_BUF_TYPE_VIDEO_OUTPUT) {
a49d2536 Alan Cox 2017-02-17  1407  		dev_err(isp->dev, "Unsupported buffer type\n");
a49d2536 Alan Cox 2017-02-17  1408  		ret = -EINVAL;
a49d2536 Alan Cox 2017-02-17  1409  		goto error;
a49d2536 Alan Cox 2017-02-17  1410  	}
a49d2536 Alan Cox 2017-02-17  1411  	rt_mutex_unlock(&isp->mutex);
a49d2536 Alan Cox 2017-02-17  1412  
a49d2536 Alan Cox 2017-02-17  1413  	return videobuf_qbuf(&pipe->outq, buf);
a49d2536 Alan Cox 2017-02-17  1414  
a49d2536 Alan Cox 2017-02-17  1415  error:
a49d2536 Alan Cox 2017-02-17  1416  	rt_mutex_unlock(&isp->mutex);
a49d2536 Alan Cox 2017-02-17  1417  
a49d2536 Alan Cox 2017-02-17  1418  	return ret;
a49d2536 Alan Cox 2017-02-17  1419  }
a49d2536 Alan Cox 2017-02-17  1420  
a49d2536 Alan Cox 2017-02-17  1421  static int __get_frame_exp_id(struct atomisp_video_pipe *pipe,
a49d2536 Alan Cox 2017-02-17  1422  		struct v4l2_buffer *buf)
a49d2536 Alan Cox 2017-02-17  1423  {
a49d2536 Alan Cox 2017-02-17  1424  	struct videobuf_vmalloc_memory *vm_mem;
a49d2536 Alan Cox 2017-02-17  1425  	struct atomisp_css_frame *handle;
a49d2536 Alan Cox 2017-02-17  1426  	int i;
a49d2536 Alan Cox 2017-02-17  1427  
a49d2536 Alan Cox 2017-02-17  1428  	for (i = 0; pipe->capq.bufs[i]; i++) {
a49d2536 Alan Cox 2017-02-17  1429  		vm_mem = pipe->capq.bufs[i]->priv;
a49d2536 Alan Cox 2017-02-17  1430  		handle = vm_mem->vaddr;
a49d2536 Alan Cox 2017-02-17  1431  		if (buf->index == pipe->capq.bufs[i]->i && handle)
a49d2536 Alan Cox 2017-02-17  1432  			return handle->exp_id;
a49d2536 Alan Cox 2017-02-17  1433  	}
a49d2536 Alan Cox 2017-02-17  1434  	return -EINVAL;
a49d2536 Alan Cox 2017-02-17  1435  }
a49d2536 Alan Cox 2017-02-17  1436  
a49d2536 Alan Cox 2017-02-17  1437  /*
a49d2536 Alan Cox 2017-02-17  1438   * Applications call the VIDIOC_DQBUF ioctl to dequeue a filled (capturing) or
a49d2536 Alan Cox 2017-02-17  1439   * displayed (output buffer)from the driver's outgoing queue
a49d2536 Alan Cox 2017-02-17  1440   */
a49d2536 Alan Cox 2017-02-17  1441  static int atomisp_dqbuf(struct file *file, void *fh, struct v4l2_buffer *buf)
a49d2536 Alan Cox 2017-02-17  1442  {
a49d2536 Alan Cox 2017-02-17  1443  	struct video_device *vdev = video_devdata(file);
a49d2536 Alan Cox 2017-02-17  1444  	struct atomisp_video_pipe *pipe = atomisp_to_video_pipe(vdev);
a49d2536 Alan Cox 2017-02-17  1445  	struct atomisp_sub_device *asd = pipe->asd;
a49d2536 Alan Cox 2017-02-17  1446  	struct atomisp_device *isp = video_get_drvdata(vdev);
a49d2536 Alan Cox 2017-02-17  1447  	int ret = 0;
a49d2536 Alan Cox 2017-02-17  1448  
a49d2536 Alan Cox 2017-02-17  1449  	rt_mutex_lock(&isp->mutex);
a49d2536 Alan Cox 2017-02-17  1450  
a49d2536 Alan Cox 2017-02-17  1451  	if (isp->isp_fatal_error) {
a49d2536 Alan Cox 2017-02-17  1452  		rt_mutex_unlock(&isp->mutex);
a49d2536 Alan Cox 2017-02-17  1453  		return -EIO;
a49d2536 Alan Cox 2017-02-17  1454  	}
a49d2536 Alan Cox 2017-02-17  1455  
a49d2536 Alan Cox 2017-02-17  1456  	if (asd->streaming == ATOMISP_DEVICE_STREAMING_STOPPING) {
a49d2536 Alan Cox 2017-02-17  1457  		rt_mutex_unlock(&isp->mutex);
a49d2536 Alan Cox 2017-02-17  1458  		dev_err(isp->dev, "%s: reject, as ISP at stopping.\n",
a49d2536 Alan Cox 2017-02-17  1459  				__func__);
a49d2536 Alan Cox 2017-02-17  1460  		return -EIO;
a49d2536 Alan Cox 2017-02-17  1461  	}
a49d2536 Alan Cox 2017-02-17  1462  
a49d2536 Alan Cox 2017-02-17  1463  	rt_mutex_unlock(&isp->mutex);
a49d2536 Alan Cox 2017-02-17  1464  
a49d2536 Alan Cox 2017-02-17  1465  	ret = videobuf_dqbuf(&pipe->capq, buf, file->f_flags & O_NONBLOCK);
a49d2536 Alan Cox 2017-02-17  1466  	if (ret) {
a49d2536 Alan Cox 2017-02-17  1467  		dev_dbg(isp->dev, "<%s: %d\n", __func__, ret);
a49d2536 Alan Cox 2017-02-17  1468  		return ret;
a49d2536 Alan Cox 2017-02-17  1469  	}
a49d2536 Alan Cox 2017-02-17  1470  	rt_mutex_lock(&isp->mutex);
a49d2536 Alan Cox 2017-02-17  1471  	buf->bytesused = pipe->pix.sizeimage;
a49d2536 Alan Cox 2017-02-17  1472  	buf->reserved = asd->frame_status[buf->index];
a49d2536 Alan Cox 2017-02-17  1473  
a49d2536 Alan Cox 2017-02-17  1474  	/*
a49d2536 Alan Cox 2017-02-17  1475  	 * Hack:
a49d2536 Alan Cox 2017-02-17  1476  	 * Currently frame_status in the enum type which takes no more lower
a49d2536 Alan Cox 2017-02-17  1477  	 * 8 bit.
a49d2536 Alan Cox 2017-02-17  1478  	 * use bit[31:16] for exp_id as it is only in the range of 1~255
a49d2536 Alan Cox 2017-02-17  1479  	 */
a49d2536 Alan Cox 2017-02-17  1480  	buf->reserved &= 0x0000ffff;
a49d2536 Alan Cox 2017-02-17  1481  	if (!(buf->flags & V4L2_BUF_FLAG_ERROR))
a49d2536 Alan Cox 2017-02-17  1482  		buf->reserved |= __get_frame_exp_id(pipe, buf) << 16;
a49d2536 Alan Cox 2017-02-17  1483  	buf->reserved2 = pipe->frame_config_id[buf->index];
a49d2536 Alan Cox 2017-02-17  1484  	rt_mutex_unlock(&isp->mutex);
a49d2536 Alan Cox 2017-02-17  1485  
a49d2536 Alan Cox 2017-02-17 @1486  	dev_dbg(isp->dev, "dqbuf buffer %d (%s) for asd%d with exp_id %d, isp_config_id %d\n",
a49d2536 Alan Cox 2017-02-17  1487  		buf->index, vdev->name, asd->index, buf->reserved >> 16,
a49d2536 Alan Cox 2017-02-17  1488  		buf->reserved2);
a49d2536 Alan Cox 2017-02-17  1489  	return 0;

:::::: The code at line 1297 was first introduced by commit
:::::: a49d25364dfb9f8a64037488a39ab1f56c5fa419 staging/atomisp: Add support for the Intel IPU v2

:::::: TO: Alan Cox <alan@linux.intel.com>
:::::: CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 60056 bytes --]

Re: [PATCH 03/12] [media] vb2: add in-fence support to QBUF

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 19883 bytes --]

Hi Gustavo,

[auto build test WARNING on linuxtv-media/master]
[also build test WARNING on v4.12-rc5 next-20170616]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Gustavo-Padovan/vb2-add-explicit-fence-user-API/20170618-210740
base:   git://linuxtv.org/media_tree.git master
reproduce: make htmldocs

All warnings (new ones prefixed by >>):

   WARNING: convert(1) not found, for SVG to PDF conversion install ImageMagick (https://www.imagemagick.org)
   arch/x86/include/asm/uaccess_32.h:1: warning: no structured comments found
   include/linux/init.h:1: warning: no structured comments found
   include/linux/mod_devicetable.h:686: warning: Excess struct/union/enum/typedef member 'ver_major' description in 'fsl_mc_device_id'
   include/linux/mod_devicetable.h:686: warning: Excess struct/union/enum/typedef member 'ver_minor' description in 'fsl_mc_device_id'
   kernel/sched/core.c:2088: warning: No description found for parameter 'rf'
   kernel/sched/core.c:2088: warning: Excess function parameter 'cookie' description in 'try_to_wake_up_local'
   include/linux/kthread.h:26: warning: Excess function parameter '...' description in 'kthread_create'
   kernel/sys.c:1: warning: no structured comments found
   include/linux/device.h:969: warning: No description found for parameter 'dma_ops'
   drivers/dma-buf/seqno-fence.c:1: warning: no structured comments found
   include/linux/iio/iio.h:597: warning: No description found for parameter 'trig_readonly'
   include/linux/iio/trigger.h:151: warning: No description found for parameter 'indio_dev'
   include/linux/iio/trigger.h:151: warning: No description found for parameter 'trig'
   include/linux/device.h:970: warning: No description found for parameter 'dma_ops'
   include/linux/usb/gadget.h:230: warning: No description found for parameter 'claimed'
   include/linux/usb/gadget.h:230: warning: No description found for parameter 'enabled'
   include/linux/usb/gadget.h:408: warning: No description found for parameter 'quirk_altset_not_supp'
   include/linux/usb/gadget.h:408: warning: No description found for parameter 'quirk_stall_not_supp'
   include/linux/usb/gadget.h:408: warning: No description found for parameter 'quirk_zlp_not_supp'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'set_busid'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'irq_handler'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'irq_preinstall'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'irq_postinstall'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'irq_uninstall'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'debugfs_init'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_open_object'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_close_object'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'prime_handle_to_fd'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'prime_fd_to_handle'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_prime_export'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_prime_import'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_prime_pin'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_prime_unpin'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_prime_res_obj'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_prime_get_sg_table'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_prime_import_sg_table'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_prime_vmap'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_prime_vunmap'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_prime_mmap'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'gem_vm_ops'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'major'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'minor'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'patchlevel'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'name'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'desc'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'date'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'driver_features'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'ioctls'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'num_ioctls'
   include/drm/drm_drv.h:524: warning: No description found for parameter 'fops'
   include/drm/drm_color_mgmt.h:1: warning: no structured comments found
   drivers/gpu/drm/drm_plane_helper.c:403: warning: No description found for parameter 'ctx'
   drivers/gpu/drm/drm_plane_helper.c:404: warning: No description found for parameter 'ctx'
   drivers/gpu/drm/i915/intel_lpe_audio.c:355: warning: No description found for parameter 'dp_output'
   drivers/gpu/drm/i915/intel_lpe_audio.c:355: warning: No description found for parameter 'link_rate'
   drivers/gpu/drm/i915/intel_lpe_audio.c:356: warning: No description found for parameter 'dp_output'
   drivers/gpu/drm/i915/intel_lpe_audio.c:356: warning: No description found for parameter 'link_rate'
>> include/media/videobuf2-core.h:735: warning: No description found for parameter 'fence'
   Documentation/core-api/assoc_array.rst:13: WARNING: Enumerated list ends without a blank line; unexpected unindent.
   Documentation/doc-guide/sphinx.rst:126: ERROR: Unknown target name: "sphinx c domain".
   kernel/sched/fair.c:7650: WARNING: Inline emphasis start-string without end-string.
   kernel/time/timer.c:1200: ERROR: Unexpected indentation.
   kernel/time/timer.c:1202: ERROR: Unexpected indentation.
   kernel/time/timer.c:1203: WARNING: Block quote ends without a blank line; unexpected unindent.
   include/linux/wait.h:122: WARNING: Block quote ends without a blank line; unexpected unindent.
   include/linux/wait.h:125: ERROR: Unexpected indentation.
   include/linux/wait.h:127: WARNING: Block quote ends without a blank line; unexpected unindent.
   kernel/time/hrtimer.c:990: WARNING: Block quote ends without a blank line; unexpected unindent.
   kernel/signal.c:322: WARNING: Inline literal start-string without end-string.
   include/linux/iio/iio.h:219: ERROR: Unexpected indentation.
   include/linux/iio/iio.h:220: WARNING: Block quote ends without a blank line; unexpected unindent.
   include/linux/iio/iio.h:226: WARNING: Definition list ends without a blank line; unexpected unindent.
   drivers/iio/industrialio-core.c:638: ERROR: Unknown target name: "iio_val".
   drivers/iio/industrialio-core.c:645: ERROR: Unknown target name: "iio_val".
   drivers/message/fusion/mptbase.c:5051: WARNING: Definition list ends without a blank line; unexpected unindent.
   drivers/tty/serial/serial_core.c:1898: WARNING: Definition list ends without a blank line; unexpected unindent.
   drivers/pci/pci.c:3457: ERROR: Unexpected indentation.
   include/linux/regulator/driver.h:271: ERROR: Unknown target name: "regulator_regmap_x_voltage".
   include/linux/spi/spi.h:370: ERROR: Unexpected indentation.
   drivers/gpu/drm/drm_scdc_helper.c:203: ERROR: Unexpected indentation.
   drivers/gpu/drm/drm_scdc_helper.c:204: WARNING: Block quote ends without a blank line; unexpected unindent.
   drivers/gpu/drm/drm_ioctl.c:690: WARNING: Definition list ends without a blank line; unexpected unindent.
   Documentation/gpu/todo.rst:111: ERROR: Unknown target name: "drm_fb".
   sound/soc/soc-core.c:2670: ERROR: Unknown target name: "snd_soc_daifmt".
   sound/core/jack.c:312: ERROR: Unknown target name: "snd_jack_btn".
   Documentation/userspace-api/unshare.rst:108: WARNING: Inline emphasis start-string without end-string.
   Documentation/usb/typec.rst:: WARNING: document isn't included in any toctree
   Documentation/usb/usb3-debug-port.rst:: WARNING: document isn't included in any toctree
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 43: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 56: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 69: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 82: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 96: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 109: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 122: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 133: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 164: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 193: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "~/.fonts.conf", line 43: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "~/.fonts.conf", line 56: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "~/.fonts.conf", line 69: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "~/.fonts.conf", line 82: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "~/.fonts.conf", line 96: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "~/.fonts.conf", line 109: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "~/.fonts.conf", line 122: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "~/.fonts.conf", line 133: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "~/.fonts.conf", line 164: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "~/.fonts.conf", line 193: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 43: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 56: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 69: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 82: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 96: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 109: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 122: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 133: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 164: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 193: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "~/.fonts.conf", line 43: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "~/.fonts.conf", line 56: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "~/.fonts.conf", line 69: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "~/.fonts.conf", line 82: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "~/.fonts.conf", line 96: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "~/.fonts.conf", line 109: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "~/.fonts.conf", line 122: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "~/.fonts.conf", line 133: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "~/.fonts.conf", line 164: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "~/.fonts.conf", line 193: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 43: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 56: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 69: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 82: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 96: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 109: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 122: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 133: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 164: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 193: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "~/.fonts.conf", line 43: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "~/.fonts.conf", line 56: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "~/.fonts.conf", line 69: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "~/.fonts.conf", line 82: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "~/.fonts.conf", line 96: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "~/.fonts.conf", line 109: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "~/.fonts.conf", line 122: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "~/.fonts.conf", line 133: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "~/.fonts.conf", line 164: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "~/.fonts.conf", line 193: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 43: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 56: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 69: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 82: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 96: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 109: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 122: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 133: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 164: Having multiple values in <test> isn't supported and may not work as expected
   Fontconfig warning: "/home/kbuild/.config/fontconfig/fonts.conf", line 193: Having multiple values in <test> isn't supported and may not work as expected

vim +/fence +735 include/media/videobuf2-core.h

   719	 *		in driver
   720	 *
   721	 * Should be called from vidioc_qbuf ioctl handler of a driver.
   722	 * The passed buffer should have been verified.
   723	 *
   724	 * This function:
   725	 *
   726	 * #) if necessary, calls buf_prepare callback in the driver (if provided), in
   727	 *    which driver-specific buffer initialization can be performed,
   728	 * #) if streaming is on, queues the buffer in driver by the means of
   729	 *    &vb2_ops->buf_queue callback for processing.
   730	 *
   731	 * The return values from this function are intended to be directly returned
   732	 * from vidioc_qbuf handler in driver.
   733	 */
   734	int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb,
 > 735			  struct dma_fence *fence);
   736	
   737	/**
   738	 * vb2_core_dqbuf() - Dequeue a buffer to the userspace
   739	 * @q:		videobuf2 queue
   740	 * @pindex:	pointer to the buffer index. May be NULL
   741	 * @pb:		buffer structure passed from userspace to vidioc_dqbuf handler
   742	 *		in driver
   743	 * @nonblocking: if true, this call will not sleep waiting for a buffer if no

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 6634 bytes --]

Re: [PATCH 08/12] [media] vb2: add 'ordered' property to queues

[Gustavo Padovan]

Hi Nicolas,

2017-06-16 Nicolas Dufresne <nicolas@ndufresne.ca>:

> Le vendredi 16 juin 2017 à 16:39 +0900, Gustavo Padovan a écrit :
> > > 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 which the buffer were queued
> > by userspace. This is already true for many drivers, but we never had
> > the need to say it.
> 
> Phrased this way, that sound like a statement that a m2m decoder
> handling b-frame will just never be supported. I think decoders are a
> very important use case for explicit synchronization.
> 
> What I believe happens with decoders is simply that the allocation
> order (the order in which empty buffers are retrieved from the queue)
> will be different then the actual presentation order. Also, multiple
> buffers endup being filled at the same time. Some firmware may inform
> of the new order at the last minute, making indeed the fence useless,
> but these are firmware and the information can be known earlier. Also,
> this information would be known by userspace for the case (up-coming,
> see STM patches and Rockchip comments [0]) or state-less decoder,
> because it is available while parsing the bitstream. For this last
> scenarios, the fact that ordering is not the same should disable the
> fences since userspace can know which fences to wait for first. Those
> drivers would need to set "ordered" to 0, which would be counter
> intuitive.
> 
> I think this use case is too important to just ignore it. I would
> expect that we at least have a todo with something sensible as a plan
> to cover this.

We definitely need to cover these usecases, I sent the patchset in a
hurry just before going on vacation and forget to lay down any plan for
other things. But for now, I believe we need refine the implementation
of the most common case and then look at expanding it.

	Gustavo

Re: [PATCH 01/12] [media] vb2: add explicit fence user API

[Gustavo Padovan]

2017-06-18 kbuild test robot <lkp@intel.com>:

> Hi Gustavo,
> 
> [auto build test ERROR on linuxtv-media/master]
> [also build test ERROR on v4.12-rc5 next-20170616]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> 
> url:    https://github.com/0day-ci/linux/commits/Gustavo-Padovan/vb2-add-explicit-fence-user-API/20170618-210740
> base:   git://linuxtv.org/media_tree.git master
> config: x86_64-allmodconfig (attached as .config)
> compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
> reproduce:
>         # save the attached .config to linux build tree
>         make ARCH=x86_64 
> 
> All error/warnings (new ones prefixed by >>):
> 
>    drivers/staging/media//atomisp/pci/atomisp2/atomisp_ioctl.c: In function 'atomisp_qbuf':
> >> drivers/staging/media//atomisp/pci/atomisp2/atomisp_ioctl.c:1297:10: error: 'struct v4l2_buffer' has no member named 'reserved2'; did you mean 'reserved'?
>          (buf->reserved2 & ATOMISP_BUFFER_HAS_PER_FRAME_SETTING)) {
>              ^~
>    drivers/staging/media//atomisp/pci/atomisp2/atomisp_ioctl.c:1299:50: error: 'struct v4l2_buffer' has no member named 'reserved2'; did you mean 'reserved'?
>       pipe->frame_request_config_id[buf->index] = buf->reserved2 &
>                                                      ^~
>    drivers/staging/media//atomisp/pci/atomisp2/atomisp_ioctl.c: In function 'atomisp_dqbuf':
>    drivers/staging/media//atomisp/pci/atomisp2/atomisp_ioctl.c:1483:5: error: 'struct v4l2_buffer' has no member named 'reserved2'; did you mean 'reserved'?
>      buf->reserved2 = pipe->frame_config_id[buf->index];
>         ^~
>    In file included from include/linux/printk.h:329:0,
>                     from include/linux/kernel.h:13,
>                     from include/linux/delay.h:21,
>                     from drivers/staging/media//atomisp/pci/atomisp2/atomisp_ioctl.c:24:
>    drivers/staging/media//atomisp/pci/atomisp2/atomisp_ioctl.c:1488:6: error: 'struct v4l2_buffer' has no member named 'reserved2'; did you mean 'reserved'?
>       buf->reserved2);
>          ^

Ouch! Seems the reserved2 was burned down by 2 drivers accessing it
without any care for the uAPI. I'll change my patches to use the
'reserved' field instead.

	Gustavo

Re: [PATCH 01/12] [media] vb2: add explicit fence user API

[Mauro Carvalho Chehab]

Em Fri, 16 Jun 2017 16:39:04 +0900
Gustavo Padovan <gustavo@padovan.org> escreveu:

> 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.
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
> ---
>  drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 4 ++--
>  drivers/media/v4l2-core/videobuf2-v4l2.c      | 2 +-
>  include/uapi/linux/videodev2.h                | 4 +++-
>  3 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> index 6f52970..8f6ab85 100644
> --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> @@ -367,7 +367,7 @@ struct v4l2_buffer32 {
>  		__s32		fd;
>  	} m;
>  	__u32			length;
> -	__u32			reserved2;
> +	__s32			fence_fd;
>  	__u32			reserved;
>  };
>  
> @@ -530,7 +530,7 @@ 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->fence_fd, &up->fence_fd) ||
>  		put_user(kp->reserved, &up->reserved) ||
>  		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 0c06699..110fb45 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 2b8feb8..750d511 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -916,7 +916,7 @@ struct v4l2_buffer {
>  		__s32		fd;
>  	} m;
>  	__u32			length;
> -	__u32			reserved2;
> +	__s32			fence_fd;
>  	__u32			reserved;
>  };
>  
> @@ -953,6 +953,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

Please document them at Documentation/media/uapi/v4l/buffer.rst.

We'll also need a new chapter at the documentation, describing the
explicit fences mechanism.

>  
>  /**
>   * struct v4l2_exportbuffer - export of video buffer as DMABUF file descriptor


-- 
Thanks,
Mauro

Re: [PATCH 02/12] [media] vb2: split out queueing from vb_core_qbuf()

[Mauro Carvalho Chehab]

Em Fri, 16 Jun 2017 16:39:05 +0900
Gustavo Padovan <gustavo@padovan.org> escreveu:

> From: Gustavo Padovan <gustavo.padovan@collabora.com>
> 
> In order to support explicit synchronization we need to divide
> vb2_core_qbuf() in two parts, one to be executed before the fence
> signals and another one to do the actual queueing of the buffer.

Looks good to me.


Acked-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>

> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
> ---
>  drivers/media/v4l2-core/videobuf2-core.c | 51 ++++++++++++++++++--------------
>  1 file changed, 29 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> index 3107e21..ea83126 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -1367,6 +1367,34 @@ static int vb2_start_streaming(struct vb2_queue *q)
>  	return ret;
>  }
>  
> +static int __vb2_core_qbuf(struct vb2_buffer *vb, struct vb2_queue *q)
> +{
> +	int ret;
> +
> +	/*
> +	 * 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);
> +
> +	/*
> +	 * 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 (q->streaming && !q->start_streaming_called &&
> +	    q->queued_count >= q->min_buffers_needed) {
> +		ret = vb2_start_streaming(q);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	dprintk(1, "qbuf of buffer %d succeeded\n", vb->index);
> +	return 0;
> +}
> +
>  int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
>  {
>  	struct vb2_buffer *vb;
> @@ -1404,32 +1432,11 @@ 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 (q->streaming && !q->start_streaming_called &&
> -	    q->queued_count >= q->min_buffers_needed) {
> -		ret = vb2_start_streaming(q);
> -		if (ret)
> -			return ret;
> -	}
> -
> -	dprintk(1, "qbuf of buffer %d succeeded\n", vb->index);
> -	return 0;
> +	return __vb2_core_qbuf(vb, q);
>  }
>  EXPORT_SYMBOL_GPL(vb2_core_qbuf);
>  


-- 
Thanks,
Mauro

Re: [PATCH 03/12] [media] vb2: add in-fence support to QBUF

[Mauro Carvalho Chehab]

Em Fri, 16 Jun 2017 16:39:06 +0900
Gustavo Padovan <gustavo@padovan.org> escreveu:

> 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.
> 
> 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/Kconfig                    |  1 +
>  drivers/media/v4l2-core/videobuf2-core.c | 97 +++++++++++++++++++++++++-------
>  drivers/media/v4l2-core/videobuf2-v4l2.c | 15 ++++-
>  include/media/videobuf2-core.h           |  7 ++-
>  4 files changed, 99 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/media/Kconfig b/drivers/media/Kconfig
> index 55d9c2b..3cd1d3d 100644
> --- a/drivers/media/Kconfig
> +++ b/drivers/media/Kconfig
> @@ -11,6 +11,7 @@ config CEC_NOTIFIER
>  menuconfig MEDIA_SUPPORT
>  	tristate "Multimedia support"
>  	depends on HAS_IOMEM
> +	select SYNC_FILE
>  	help
>  	  If you want to use Webcams, Video grabber devices and/or TV devices
>  	  enable this option and other options below.
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> index ea83126..29aa9d4 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -1279,6 +1279,22 @@ 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))
> +			break;
> +
> +		ready_count++;

Hmm... maybe that's one of the reasons why out of order explicit fences is not
working. With the current logic, if explicit fences is enabled, this function
will always return 0 or 1, even if more buffers are ready.

IMHO, the correct logic here should be, instead:

		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;
> @@ -1324,8 +1340,15 @@ static int vb2_start_streaming(struct vb2_queue *q)
>  	 * If any buffers were queued before streamon,
>  	 * we can now pass them to driver for processing.
>  	 */
> -	list_for_each_entry(vb, &q->queued_list, queued_entry)
> +	list_for_each_entry(vb, &q->queued_list, queued_entry) {
> +		if (vb->state != VB2_BUF_STATE_QUEUED)
> +			continue;
> +
> +		if (vb->in_fence && !dma_fence_is_signaled(vb->in_fence))
> +			break;
> +
>  		__enqueue_in_driver(vb);

Same as before, the correct logic here seems to be:

		if (!vb->in_fence || dma_fence_is_signaled(vb->in_fence))
			__enqueue_in_driver(vb);	

> +	}
>  
>  	/* Tell the driver to start streaming */
>  	q->start_streaming_called = 1;
> @@ -1369,33 +1392,55 @@ static int vb2_start_streaming(struct vb2_queue *q)
>  
>  static int __vb2_core_qbuf(struct vb2_buffer *vb, struct vb2_queue *q)
>  {
> +	struct vb2_buffer *b;
>  	int ret;
>  
>  	/*
>  	 * 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);
>  
> -	/*
> -	 * 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 (q->streaming && !q->start_streaming_called &&
> -	    q->queued_count >= q->min_buffers_needed) {
> -		ret = vb2_start_streaming(q);
> -		if (ret)
> -			return ret;
> +	if (q->start_streaming_called) {
> +		list_for_each_entry(b, &q->queued_list, queued_entry) {
> +			if (b->state != VB2_BUF_STATE_QUEUED)
> +				continue;
> +
> +			if (b->in_fence && !dma_fence_is_signaled(b->in_fence))
> +				break;
> +
> +			__enqueue_in_driver(b);

Same here:

		if (!vb->in_fence || dma_fence_is_signaled(vb->in_fence))
			__enqueue_in_driver(vb);	


There is, however, a behavior change here (even without the above
proposal.

Before this patch, if fences is not used (for example, for DVB or
for some other mechanism at V4L2), the driver would be doing:

	if (q->start_streaming_called)
		__enqueue_in_driver(vb);

So, __enqueue_in_driver() would be called just once. Now, after
the change, it will be doing, instead:

	list_for_each_entry(vb, &q->queued_list, queued_entry)
		__enqueue_in_driver(vb);

With can queue several buffers at once.

I've no idea how this will affect non-fences behavior for V4L2
and for DVB. More tests are required to check if this badly affect
drivers, or if it would bring some performance or latency change.

> +		}
> +	} else {

Why did you add an else here? I guess there was a very strong reason
to not have an else at the original code - just can't remember why, and
I' too lazy today to dig into VB2 changeset descriptions :-)

Please don't change the behavior together with a patch that add new
features as it can cause regressions, and it would be a way harder
to track if you fold different changes at the same patch.

If this behavior change is due to some bug, it should be submitted
in a separate, patch, provided with a very detailed description.

If otherwise, this is a requiement just for fences, you should first
test if fences is enabled before changing the behavior.

> +		/*
> +		 * 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
> +		 * that are ready, then we can finally call start_streaming().
> +		 */
> +		if (q->streaming &&
> +		    __get_num_ready_buffers(q) >= q->min_buffers_needed) {
> +			ret = vb2_start_streaming(q);
> +			if (ret)
> +				return ret;
> +		}
>  	}
>  
>  	dprintk(1, "qbuf of buffer %d succeeded\n", vb->index);
>  	return 0;
>  }
>  
> -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;
> +
> +	__vb2_core_qbuf(vb, vb->vb2_queue);
> +}
> +
> +int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb,
> +		  struct dma_fence *fence)
>  {
>  	struct vb2_buffer *vb;
>  	int ret;
> @@ -1436,6 +1481,11 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
>  	if (pb)
>  		call_void_bufop(q, fill_user_buffer, vb, pb);
>  
> +	vb->in_fence = fence;
> +	if (fence && !dma_fence_add_callback(fence, &vb->fence_cb,
> +					     vb2_qbuf_fence_cb))
> +		return 0;

Maybe we should provide some error or debug log here or a WARN_ON(), if 
dma_fence_add_callback() fails instead of silently ignore any errors.

> +
>  	return __vb2_core_qbuf(vb, q);
>  }
>  EXPORT_SYMBOL_GPL(vb2_core_qbuf);
> @@ -1647,6 +1697,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
> @@ -1669,6 +1720,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;
> @@ -1735,7 +1794,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;
> @@ -2250,7 +2309,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;
> @@ -2429,7 +2488,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;
> @@ -2532,7 +2591,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 110fb45..e6ad77f 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>
> @@ -560,6 +561,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 +570,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 ret ? ret : vb2_core_qbuf(q, b->index, b, fence);

No need to check for ret again. You could just do:

	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 cb97c22..aa43e43 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)
> @@ -259,6 +260,9 @@ struct vb2_buffer {
>  
>  	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
> @@ -727,7 +731,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


-- 
Thanks,
Mauro

Re: [PATCH 06/12] [media] v4l: add V4L2_EVENT_BUF_QUEUED event

[Mauro Carvalho Chehab]

Em Fri, 16 Jun 2017 16:39:09 +0900
Gustavo Padovan <gustavo@padovan.org> escreveu:

> 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.

If you're changing uAPI, you need to update media uAPI book as well.
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
> ---
>  include/uapi/linux/videodev2.h | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 750d511..c2eda75 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -2150,6 +2150,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 */
> @@ -2202,6 +2203,10 @@ struct v4l2_event_motion_det {
>  	__u32 region_mask;
>  };
>  
> +struct v4l2_event_buf_queued {
> +	__u32 index;
> +};
> +
>  struct v4l2_event {
>  	__u32				type;
>  	union {
> @@ -2210,6 +2215,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;


-- 
Thanks,
Mauro

Re: [PATCH 07/12] [media] v4l: add support to BUF_QUEUED event

[Mauro Carvalho Chehab]

Em Fri, 16 Jun 2017 16:39:10 +0900
Gustavo Padovan <gustavo@padovan.org> escreveu:

> 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.
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
> ---
>  drivers/media/v4l2-core/v4l2-ctrls.c     |  6 +++++-
>  drivers/media/v4l2-core/videobuf2-core.c | 15 +++++++++++++++
>  2 files changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index 5aed7bd..f55b5da 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -3435,8 +3435,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, 0, 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 29aa9d4..00d9c35 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 <media/videobuf2-core.h>
> +#include <media/v4l2-event.h>
>  #include <media/v4l2-mc.h>
>  
>  #include <trace/events/vb2.h>
> @@ -1221,6 +1222,18 @@ static int __prepare_dmabuf(struct vb2_buffer *vb, const void *pb)
>  	return ret;
>  }
>  
> +static void vb2_buffer_queued_event(struct vb2_buffer *vb)
> +{
> +	struct video_device *vdev = to_video_device(vb->vb2_queue->dev);
> +	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(vdev, &event);
> +}
> +

It doesn't sound right to add a V4L2 event to VB2 core. The hole point
of splitting the core from V4L2 specific stuff is to allow VB2 to be
used by non-V4L2 APIs[1]. Please move this to videobuf2-v4l2.

[1] The split happened as part of a patchset meant to make the DVB
core to use VB2 and provide DMA APIs to it. Unfortunately, the
developer that worked on this project moved to some other project.
The final patch was not applied yet. I have it on my patchwork
queue. I intend to test and apply it sometime this year.



>  /**
>   * __enqueue_in_driver() - enqueue a vb2_buffer in driver for processing
>   */
> @@ -1234,6 +1247,8 @@ static void __enqueue_in_driver(struct vb2_buffer *vb)
>  	trace_vb2_buf_queue(q, vb);
>  
>  	call_void_vb_qop(vb, buf_queue, vb);
> +
> +	vb2_buffer_queued_event(vb);
>  }
>  
>  static int __buf_prepare(struct vb2_buffer *vb, const void *pb)


-- 
Thanks,
Mauro

Re: [PATCH 00/12] V4L2 explicit synchronization support

[Mauro Carvalho Chehab]

Em Fri, 16 Jun 2017 16:39:03 +0900
Gustavo Padovan <gustavo@padovan.org> escreveu:

> From: Gustavo Padovan <gustavo.padovan@collabora.com>
> 
> Hi,
> 
> This adds support for Explicit Synchronization of shared buffers in V4L2.
> It uses the Sync File Framework[1] as vector to communicate the fences
> between kernel and userspace.
> 
> Explicit Synchronization allows us to control the synchronization of
> shared buffers from userspace by passing fences to the kernel and/or 
> receiving them from the the kernel.
> 
> Fences passed to the kernel are named in-fences and the kernel should wait
> them to signal before using the buffer. On the other side, the kernel creates
> out-fences for every buffer it receives from userspace. This fence is sent back
> to userspace and it will signal when the capture, for example, has finished.
> 
> Signalling an out-fence in V4L2 would mean that the job on the buffer is done
> and the buffer can be used by other drivers.
> 
> The first patch proposes an userspace API for fences, then on patch 2
> we prepare to the addition of in-fences in patch 3, by introducing the
> infrastructure on vb2 to wait on an in-fence signal before queueing the buffer
> in the driver.
> 
> Patch 4 fix uvc v4l2 event handling and patch 5 configure q->dev for vivid
> drivers to enable to subscribe and dequeue events on it.
> 
> Patches 6-7 enables support to notify BUF_QUEUED events, i.e., let userspace
> know that particular buffer was enqueued in the driver. This is needed,
> because we return the out-fence fd as an out argument in QBUF, but at the time
> it returns we don't know to which buffer the fence will be attached thus
> the BUF_QUEUED event tells which buffer is associated to the fence received in
> QBUF by userspace.
> 
> Patches 8-9 add support to mark queues as ordered. Finally patches 10 and 11
> add more fence infrastructure to support out-fences and finally patch 12 adds
> support to out-fences.
> 
> Changelog are detailed in each patch.
> 
> Please review! Thanks.

Just reviewed the series. Most patches look good.

I have one additional concern: if the changes here won't cause any
bad behaviors if fences is not available for some VB2 non V4L2 client.
I'm actually thinking on this:

	https://patchwork.linuxtv.org/patch/31613/

>From what I saw, after this patch series, someone could try to 
inconditionally open an out fences fd for a driver. Maybe this
should be denied by default, enabling such feature only if the
VB2 "client" (e. g. videobuf-v4l2) supports it.

Regards,
Mauro

Re: [PATCH 03/12] [media] vb2: add in-fence support to QBUF

[Gustavo Padovan]

Hi Mauro,

2017-06-30 Mauro Carvalho Chehab <mchehab@osg.samsung.com>:

> Em Fri, 16 Jun 2017 16:39:06 +0900
> Gustavo Padovan <gustavo@padovan.org> escreveu:
> 
> > 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.
> > 
> > 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/Kconfig                    |  1 +
> >  drivers/media/v4l2-core/videobuf2-core.c | 97 +++++++++++++++++++++++++-------
> >  drivers/media/v4l2-core/videobuf2-v4l2.c | 15 ++++-
> >  include/media/videobuf2-core.h           |  7 ++-
> >  4 files changed, 99 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/media/Kconfig b/drivers/media/Kconfig
> > index 55d9c2b..3cd1d3d 100644
> > --- a/drivers/media/Kconfig
> > +++ b/drivers/media/Kconfig
> > @@ -11,6 +11,7 @@ config CEC_NOTIFIER
> >  menuconfig MEDIA_SUPPORT
> >  	tristate "Multimedia support"
> >  	depends on HAS_IOMEM
> > +	select SYNC_FILE
> >  	help
> >  	  If you want to use Webcams, Video grabber devices and/or TV devices
> >  	  enable this option and other options below.
> > diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> > index ea83126..29aa9d4 100644
> > --- a/drivers/media/v4l2-core/videobuf2-core.c
> > +++ b/drivers/media/v4l2-core/videobuf2-core.c
> > @@ -1279,6 +1279,22 @@ 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))
> > +			break;
> > +
> > +		ready_count++;
> 
> Hmm... maybe that's one of the reasons why out of order explicit fences is not
> working. With the current logic, if explicit fences is enabled, this function
> will always return 0 or 1, even if more buffers are ready.
> 
> IMHO, the correct logic here should be, instead:
> 
> 		if (!vb->in_fence || dma_fence_is_signaled(vb->in_fence))
> 			ready_count++;

If we do like you propose then we will be getting the wrong ready_count
and queue buffers unordered (in the next two places this code snipet
appears.

In this function I want to know how many ready buffers are in the front
of the buffer. A buffer will be ready if:

* it has no fence
* it has a fence but it was signaled already

Then we start walking the queue looking for such buffers in order and
stop as soon as we find a buffer that doesn't meet these criteria. Hence
the 'break' command to interrupt the loop. If we don't break we may
queue the next buffer on the queue (if it matches the criteria) without
queueing the current one. So we really need break out from the loop
here.

> 
> > +	}
> > +
> > +	return ready_count;
> > +}
> > +
> >  int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb)
> >  {
> >  	struct vb2_buffer *vb;
> > @@ -1324,8 +1340,15 @@ static int vb2_start_streaming(struct vb2_queue *q)
> >  	 * If any buffers were queued before streamon,
> >  	 * we can now pass them to driver for processing.
> >  	 */
> > -	list_for_each_entry(vb, &q->queued_list, queued_entry)
> > +	list_for_each_entry(vb, &q->queued_list, queued_entry) {
> > +		if (vb->state != VB2_BUF_STATE_QUEUED)
> > +			continue;
> > +
> > +		if (vb->in_fence && !dma_fence_is_signaled(vb->in_fence))
> > +			break;
> > +
> >  		__enqueue_in_driver(vb);
> 
> Same as before, the correct logic here seems to be:
> 
> 		if (!vb->in_fence || dma_fence_is_signaled(vb->in_fence))
> 			__enqueue_in_driver(vb);	
> 
> > +	}
> >  
> >  	/* Tell the driver to start streaming */
> >  	q->start_streaming_called = 1;
> > @@ -1369,33 +1392,55 @@ static int vb2_start_streaming(struct vb2_queue *q)
> >  
> >  static int __vb2_core_qbuf(struct vb2_buffer *vb, struct vb2_queue *q)
> >  {
> > +	struct vb2_buffer *b;
> >  	int ret;
> >  
> >  	/*
> >  	 * 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);
> >  
> > -	/*
> > -	 * 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 (q->streaming && !q->start_streaming_called &&
> > -	    q->queued_count >= q->min_buffers_needed) {
> > -		ret = vb2_start_streaming(q);
> > -		if (ret)
> > -			return ret;
> > +	if (q->start_streaming_called) {
> > +		list_for_each_entry(b, &q->queued_list, queued_entry) {
> > +			if (b->state != VB2_BUF_STATE_QUEUED)
> > +				continue;
> > +
> > +			if (b->in_fence && !dma_fence_is_signaled(b->in_fence))
> > +				break;
> > +
> > +			__enqueue_in_driver(b);
> 
> Same here:
> 
> 		if (!vb->in_fence || dma_fence_is_signaled(vb->in_fence))
> 			__enqueue_in_driver(vb);	
> 
> 
> There is, however, a behavior change here (even without the above
> proposal.
> 
> Before this patch, if fences is not used (for example, for DVB or
> for some other mechanism at V4L2), the driver would be doing:
> 
> 	if (q->start_streaming_called)
> 		__enqueue_in_driver(vb);
> 
> So, __enqueue_in_driver() would be called just once. Now, after
> the change, it will be doing, instead:
> 
> 	list_for_each_entry(vb, &q->queued_list, queued_entry)
> 		__enqueue_in_driver(vb);
> 
> With can queue several buffers at once.
> 
> I've no idea how this will affect non-fences behavior for V4L2
> and for DVB. More tests are required to check if this badly affect
> drivers, or if it would bring some performance or latency change.

I don't believe it affects the behaviour at all, without fences the
queue will never have more than one buffer in the queue. The queue will
only have more buffers on the queue when fences are used and fences for
the buffers in front of the queue didn't signal yet.

> 
> > +		}
> > +	} else {
> 
> Why did you add an else here? I guess there was a very strong reason
> to not have an else at the original code - just can't remember why, and
> I' too lazy today to dig into VB2 changeset descriptions :-)
> 
> Please don't change the behavior together with a patch that add new
> features as it can cause regressions, and it would be a way harder
> to track if you fold different changes at the same patch.
> 
> If this behavior change is due to some bug, it should be submitted
> in a separate, patch, provided with a very detailed description.
> 
> If otherwise, this is a requiement just for fences, you should first
> test if fences is enabled before changing the behavior.

Rigth. This should have been put in a separated patch, but it is not
changing the behaviour at all. The 'if' above this one was

	if (q->start_streaming_called)


and the 'if' below had a !q->start_streaming_called condition, so it
was a sort of else already.

> 
> > +		/*
> > +		 * 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
> > +		 * that are ready, then we can finally call start_streaming().
> > +		 */
> > +		if (q->streaming &&
> > +		    __get_num_ready_buffers(q) >= q->min_buffers_needed) {
> > +			ret = vb2_start_streaming(q);
> > +			if (ret)
> > +				return ret;
> > +		}
> >  	}
> >  
> >  	dprintk(1, "qbuf of buffer %d succeeded\n", vb->index);
> >  	return 0;
> >  }
> >  
> > -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;
> > +
> > +	__vb2_core_qbuf(vb, vb->vb2_queue);
> > +}
> > +
> > +int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb,
> > +		  struct dma_fence *fence)
> >  {
> >  	struct vb2_buffer *vb;
> >  	int ret;
> > @@ -1436,6 +1481,11 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
> >  	if (pb)
> >  		call_void_bufop(q, fill_user_buffer, vb, pb);
> >  
> > +	vb->in_fence = fence;
> > +	if (fence && !dma_fence_add_callback(fence, &vb->fence_cb,
> > +					     vb2_qbuf_fence_cb))
> > +		return 0;
> 
> Maybe we should provide some error or debug log here or a WARN_ON(), if 
> dma_fence_add_callback() fails instead of silently ignore any errors.

This is not an error. If the if succeeds it mean we have installed a
callback for the fence. If not, it means the fence signaled already and
we don't can call __vb2_core_qbuf right away.

> 
> > +
> >  	return __vb2_core_qbuf(vb, q);
> >  }
> >  EXPORT_SYMBOL_GPL(vb2_core_qbuf);
> > @@ -1647,6 +1697,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
> > @@ -1669,6 +1720,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;
> > @@ -1735,7 +1794,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;
> > @@ -2250,7 +2309,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;
> > @@ -2429,7 +2488,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;
> > @@ -2532,7 +2591,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 110fb45..e6ad77f 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>
> > @@ -560,6 +561,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 +570,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 ret ? ret : vb2_core_qbuf(q, b->index, b, fence);
> 
> No need to check for ret again. You could just do:
> 
> 	return vb2_core_qbuf(q, b->index, b, fence);

Sure.

Gustavo

Re: [PATCH 07/12] [media] v4l: add support to BUF_QUEUED event

[Gustavo Padovan]

Hi Mauro,

2017-06-30 Mauro Carvalho Chehab <mchehab@osg.samsung.com>:

> Em Fri, 16 Jun 2017 16:39:10 +0900
> Gustavo Padovan <gustavo@padovan.org> escreveu:
> 
> > 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.
> > 
> > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
> > ---
> >  drivers/media/v4l2-core/v4l2-ctrls.c     |  6 +++++-
> >  drivers/media/v4l2-core/videobuf2-core.c | 15 +++++++++++++++
> >  2 files changed, 20 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> > index 5aed7bd..f55b5da 100644
> > --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> > @@ -3435,8 +3435,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, 0, 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 29aa9d4..00d9c35 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 <media/videobuf2-core.h>
> > +#include <media/v4l2-event.h>
> >  #include <media/v4l2-mc.h>
> >  
> >  #include <trace/events/vb2.h>
> > @@ -1221,6 +1222,18 @@ static int __prepare_dmabuf(struct vb2_buffer *vb, const void *pb)
> >  	return ret;
> >  }
> >  
> > +static void vb2_buffer_queued_event(struct vb2_buffer *vb)
> > +{
> > +	struct video_device *vdev = to_video_device(vb->vb2_queue->dev);
> > +	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(vdev, &event);
> > +}
> > +
> 
> It doesn't sound right to add a V4L2 event to VB2 core. The hole point
> of splitting the core from V4L2 specific stuff is to allow VB2 to be
> used by non-V4L2 APIs[1]. Please move this to videobuf2-v4l2.

Yes, that makes sense.  My lack of understanding of v4l2 core didn't me
allow realize that. I'll move it to videobuf2-v4l2.

Gustavo

Re: [PATCH 00/12] V4L2 explicit synchronization support

[Gustavo Padovan]

Hi Mauro,

2017-06-30 Mauro Carvalho Chehab <mchehab@osg.samsung.com>:

> Em Fri, 16 Jun 2017 16:39:03 +0900
> Gustavo Padovan <gustavo@padovan.org> escreveu:
> 
> > From: Gustavo Padovan <gustavo.padovan@collabora.com>
> > 
> > Hi,
> > 
> > This adds support for Explicit Synchronization of shared buffers in V4L2.
> > It uses the Sync File Framework[1] as vector to communicate the fences
> > between kernel and userspace.
> > 
> > Explicit Synchronization allows us to control the synchronization of
> > shared buffers from userspace by passing fences to the kernel and/or 
> > receiving them from the the kernel.
> > 
> > Fences passed to the kernel are named in-fences and the kernel should wait
> > them to signal before using the buffer. On the other side, the kernel creates
> > out-fences for every buffer it receives from userspace. This fence is sent back
> > to userspace and it will signal when the capture, for example, has finished.
> > 
> > Signalling an out-fence in V4L2 would mean that the job on the buffer is done
> > and the buffer can be used by other drivers.
> > 
> > The first patch proposes an userspace API for fences, then on patch 2
> > we prepare to the addition of in-fences in patch 3, by introducing the
> > infrastructure on vb2 to wait on an in-fence signal before queueing the buffer
> > in the driver.
> > 
> > Patch 4 fix uvc v4l2 event handling and patch 5 configure q->dev for vivid
> > drivers to enable to subscribe and dequeue events on it.
> > 
> > Patches 6-7 enables support to notify BUF_QUEUED events, i.e., let userspace
> > know that particular buffer was enqueued in the driver. This is needed,
> > because we return the out-fence fd as an out argument in QBUF, but at the time
> > it returns we don't know to which buffer the fence will be attached thus
> > the BUF_QUEUED event tells which buffer is associated to the fence received in
> > QBUF by userspace.
> > 
> > Patches 8-9 add support to mark queues as ordered. Finally patches 10 and 11
> > add more fence infrastructure to support out-fences and finally patch 12 adds
> > support to out-fences.
> > 
> > Changelog are detailed in each patch.
> > 
> > Please review! Thanks.
> 
> Just reviewed the series. Most patches look good.
> 
> I have one additional concern: if the changes here won't cause any
> bad behaviors if fences is not available for some VB2 non V4L2 client.
> I'm actually thinking on this:
> 
> 	https://patchwork.linuxtv.org/patch/31613/
> 
> From what I saw, after this patch series, someone could try to 
> inconditionally open an out fences fd for a driver. Maybe this
> should be denied by default, enabling such feature only if the
> VB2 "client" (e. g. videobuf-v4l2) supports it.

Yes, I think we can just reject the request if this non-VB2 client
tries to use the arg flags for fences.

Gustavo

Re: [PATCH 01/12] [media] vb2: add explicit fence user API

[Tomasz Figa]

Hi Gustavo,

On Tue, Jun 27, 2017 at 12:39 AM, Gustavo Padovan <gustavo@padovan.org> wrote:
> 2017-06-18 kbuild test robot <lkp@intel.com>:
>
>> Hi Gustavo,
>>
>> [auto build test ERROR on linuxtv-media/master]
>> [also build test ERROR on v4.12-rc5 next-20170616]
>> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
>>
>> url:    https://github.com/0day-ci/linux/commits/Gustavo-Padovan/vb2-add-explicit-fence-user-API/20170618-210740
>> base:   git://linuxtv.org/media_tree.git master
>> config: x86_64-allmodconfig (attached as .config)
>> compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
>> reproduce:
>>         # save the attached .config to linux build tree
>>         make ARCH=x86_64
>>
>> All error/warnings (new ones prefixed by >>):
>>
>>    drivers/staging/media//atomisp/pci/atomisp2/atomisp_ioctl.c: In function 'atomisp_qbuf':
>> >> drivers/staging/media//atomisp/pci/atomisp2/atomisp_ioctl.c:1297:10: error: 'struct v4l2_buffer' has no member named 'reserved2'; did you mean 'reserved'?
>>          (buf->reserved2 & ATOMISP_BUFFER_HAS_PER_FRAME_SETTING)) {
>>              ^~
>>    drivers/staging/media//atomisp/pci/atomisp2/atomisp_ioctl.c:1299:50: error: 'struct v4l2_buffer' has no member named 'reserved2'; did you mean 'reserved'?
>>       pipe->frame_request_config_id[buf->index] = buf->reserved2 &
>>                                                      ^~
>>    drivers/staging/media//atomisp/pci/atomisp2/atomisp_ioctl.c: In function 'atomisp_dqbuf':
>>    drivers/staging/media//atomisp/pci/atomisp2/atomisp_ioctl.c:1483:5: error: 'struct v4l2_buffer' has no member named 'reserved2'; did you mean 'reserved'?
>>      buf->reserved2 = pipe->frame_config_id[buf->index];
>>         ^~
>>    In file included from include/linux/printk.h:329:0,
>>                     from include/linux/kernel.h:13,
>>                     from include/linux/delay.h:21,
>>                     from drivers/staging/media//atomisp/pci/atomisp2/atomisp_ioctl.c:24:
>>    drivers/staging/media//atomisp/pci/atomisp2/atomisp_ioctl.c:1488:6: error: 'struct v4l2_buffer' has no member named 'reserved2'; did you mean 'reserved'?
>>       buf->reserved2);
>>          ^
>
> Ouch! Seems the reserved2 was burned down by 2 drivers accessing it
> without any care for the uAPI. I'll change my patches to use the
> 'reserved' field instead.

Given that a reserved field has a clear meaning of being reserved and
the driver in question is in staging. I'd rather vote for fixing the
driver.

Best regards,
Tomasz

Re: [PATCH 01/12] [media] vb2: add explicit fence user API

[Alexandre Courbot]

On Tue, Jul 4, 2017 at 2:57 PM, Tomasz Figa <tfiga@google.com> wrote:
> Hi Gustavo,
>
> On Tue, Jun 27, 2017 at 12:39 AM, Gustavo Padovan <gustavo@padovan.org> wrote:
>> 2017-06-18 kbuild test robot <lkp@intel.com>:
>>
>>> Hi Gustavo,
>>>
>>> [auto build test ERROR on linuxtv-media/master]
>>> [also build test ERROR on v4.12-rc5 next-20170616]
>>> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
>>>
>>> url:    https://github.com/0day-ci/linux/commits/Gustavo-Padovan/vb2-add-explicit-fence-user-API/20170618-210740
>>> base:   git://linuxtv.org/media_tree.git master
>>> config: x86_64-allmodconfig (attached as .config)
>>> compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
>>> reproduce:
>>>         # save the attached .config to linux build tree
>>>         make ARCH=x86_64
>>>
>>> All error/warnings (new ones prefixed by >>):
>>>
>>>    drivers/staging/media//atomisp/pci/atomisp2/atomisp_ioctl.c: In function 'atomisp_qbuf':
>>> >> drivers/staging/media//atomisp/pci/atomisp2/atomisp_ioctl.c:1297:10: error: 'struct v4l2_buffer' has no member named 'reserved2'; did you mean 'reserved'?
>>>          (buf->reserved2 & ATOMISP_BUFFER_HAS_PER_FRAME_SETTING)) {
>>>              ^~
>>>    drivers/staging/media//atomisp/pci/atomisp2/atomisp_ioctl.c:1299:50: error: 'struct v4l2_buffer' has no member named 'reserved2'; did you mean 'reserved'?
>>>       pipe->frame_request_config_id[buf->index] = buf->reserved2 &
>>>                                                      ^~
>>>    drivers/staging/media//atomisp/pci/atomisp2/atomisp_ioctl.c: In function 'atomisp_dqbuf':
>>>    drivers/staging/media//atomisp/pci/atomisp2/atomisp_ioctl.c:1483:5: error: 'struct v4l2_buffer' has no member named 'reserved2'; did you mean 'reserved'?
>>>      buf->reserved2 = pipe->frame_config_id[buf->index];
>>>         ^~
>>>    In file included from include/linux/printk.h:329:0,
>>>                     from include/linux/kernel.h:13,
>>>                     from include/linux/delay.h:21,
>>>                     from drivers/staging/media//atomisp/pci/atomisp2/atomisp_ioctl.c:24:
>>>    drivers/staging/media//atomisp/pci/atomisp2/atomisp_ioctl.c:1488:6: error: 'struct v4l2_buffer' has no member named 'reserved2'; did you mean 'reserved'?
>>>       buf->reserved2);
>>>          ^
>>
>> Ouch! Seems the reserved2 was burned down by 2 drivers accessing it
>> without any care for the uAPI. I'll change my patches to use the
>> 'reserved' field instead.
>
> Given that a reserved field has a clear meaning of being reserved and
> the driver in question is in staging. I'd rather vote for fixing the
> driver.

Same here. It seems like this use of reserved2 should not have been
merged in the first place, thankfully it's only in staging.

Re: [PATCH 02/12] [media] vb2: split out queueing from vb_core_qbuf()

[Hans Verkuil]

On 06/16/17 09:39, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.com>
> 
> In order to support explicit synchronization we need to divide
> vb2_core_qbuf() in two parts, one to be executed before the fence
> signals and another one to do the actual queueing of the buffer.
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
> ---
>  drivers/media/v4l2-core/videobuf2-core.c | 51 ++++++++++++++++++--------------
>  1 file changed, 29 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> index 3107e21..ea83126 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -1367,6 +1367,34 @@ static int vb2_start_streaming(struct vb2_queue *q)
>  	return ret;
>  }
>  
> +static int __vb2_core_qbuf(struct vb2_buffer *vb, struct vb2_queue *q)
> +{
> +	int ret;
> +
> +	/*
> +	 * 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);
> +
> +	/*
> +	 * 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 (q->streaming && !q->start_streaming_called &&
> +	    q->queued_count >= q->min_buffers_needed) {
> +		ret = vb2_start_streaming(q);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	dprintk(1, "qbuf of buffer %d succeeded\n", vb->index);
> +	return 0;
> +}
> +
>  int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
>  {
>  	struct vb2_buffer *vb;
> @@ -1404,32 +1432,11 @@ 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);

This should be called *after* the __vb2_core_qbuf call. That call changes
vb->state which is used by buffer to fill in v4l2_buffer. So the order
should be swapped here to ensure we return the latest state of the buffer.

>  
> -	/*
> -	 * 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 (q->streaming && !q->start_streaming_called &&
> -	    q->queued_count >= q->min_buffers_needed) {
> -		ret = vb2_start_streaming(q);
> -		if (ret)
> -			return ret;
> -	}
> -
> -	dprintk(1, "qbuf of buffer %d succeeded\n", vb->index);
> -	return 0;
> +	return __vb2_core_qbuf(vb, q);
>  }
>  EXPORT_SYMBOL_GPL(vb2_core_qbuf);
>  
> 

Regards,

	Hans

Re: [PATCH 03/12] [media] vb2: add in-fence support to QBUF

[Hans Verkuil]

On 06/16/17 09:39, 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.
> 
> 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/Kconfig                    |  1 +
>  drivers/media/v4l2-core/videobuf2-core.c | 97 +++++++++++++++++++++++++-------
>  drivers/media/v4l2-core/videobuf2-v4l2.c | 15 ++++-
>  include/media/videobuf2-core.h           |  7 ++-
>  4 files changed, 99 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/media/Kconfig b/drivers/media/Kconfig
> index 55d9c2b..3cd1d3d 100644
> --- a/drivers/media/Kconfig
> +++ b/drivers/media/Kconfig
> @@ -11,6 +11,7 @@ config CEC_NOTIFIER
>  menuconfig MEDIA_SUPPORT
>  	tristate "Multimedia support"
>  	depends on HAS_IOMEM
> +	select SYNC_FILE

Is this the right place for this? Shouldn't this be selected in
'config VIDEOBUF2_CORE'?

Fences are specific to vb2 after all.

>  	help
>  	  If you want to use Webcams, Video grabber devices and/or TV devices
>  	  enable this option and other options below.
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> index ea83126..29aa9d4 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -1279,6 +1279,22 @@ 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))
> +			break;

Obviously the break is wrong as Mauro mentioned.

> +
> +		ready_count++;
> +	}
> +
> +	return ready_count;
> +}
> +
>  int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb)
>  {
>  	struct vb2_buffer *vb;
> @@ -1324,8 +1340,15 @@ static int vb2_start_streaming(struct vb2_queue *q)
>  	 * If any buffers were queued before streamon,
>  	 * we can now pass them to driver for processing.
>  	 */
> -	list_for_each_entry(vb, &q->queued_list, queued_entry)
> +	list_for_each_entry(vb, &q->queued_list, queued_entry) {
> +		if (vb->state != VB2_BUF_STATE_QUEUED)
> +			continue;

I think this test is unnecessary.

> +
> +		if (vb->in_fence && !dma_fence_is_signaled(vb->in_fence))
> +			break;
> +
>  		__enqueue_in_driver(vb);

I would move the above test (after fixing it as Mauro said) to __enqueue_in_driver.
I.e. if this is waiting for a fence then __enqueue_in_driver does nothing.

> +	}
>  
>  	/* Tell the driver to start streaming */
>  	q->start_streaming_called = 1;
> @@ -1369,33 +1392,55 @@ static int vb2_start_streaming(struct vb2_queue *q)
>  
>  static int __vb2_core_qbuf(struct vb2_buffer *vb, struct vb2_queue *q)
>  {
> +	struct vb2_buffer *b;
>  	int ret;
>  
>  	/*
>  	 * 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);
>  
> -	/*
> -	 * 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 (q->streaming && !q->start_streaming_called &&
> -	    q->queued_count >= q->min_buffers_needed) {
> -		ret = vb2_start_streaming(q);
> -		if (ret)
> -			return ret;
> +	if (q->start_streaming_called) {
> +		list_for_each_entry(b, &q->queued_list, queued_entry) {
> +			if (b->state != VB2_BUF_STATE_QUEUED)
> +				continue;
> +
> +			if (b->in_fence && !dma_fence_is_signaled(b->in_fence))
> +				break;

Again, if this test is in __enqueue_in_driver, then you can keep the
original code. Why would you need to loop over all buffers anyway?

If a fence is ready then the callback will call this function for that
buffer. Everything works fine AFAICT without looping over buffers here.

> +
> +			__enqueue_in_driver(b);
> +		}
> +	} else {
> +		/*
> +		 * 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
> +		 * that are ready, then we can finally call start_streaming().
> +		 */
> +		if (q->streaming &&
> +		    __get_num_ready_buffers(q) >= q->min_buffers_needed) {

Just combine this with the 'else' to an 'else if'. Saves an extra level of
indentation.

To follow-up from Mauro's comment: having an 'else if' here is fine.

> +			ret = vb2_start_streaming(q);
> +			if (ret)
> +				return ret;
> +		}
>  	}
>  
>  	dprintk(1, "qbuf of buffer %d succeeded\n", vb->index);
>  	return 0;
>  }
>  
> -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;
> +
> +	__vb2_core_qbuf(vb, vb->vb2_queue);
> +}
> +
> +int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb,
> +		  struct dma_fence *fence)
>  {
>  	struct vb2_buffer *vb;
>  	int ret;
> @@ -1436,6 +1481,11 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
>  	if (pb)
>  		call_void_bufop(q, fill_user_buffer, vb, pb);
>  
> +	vb->in_fence = fence;
> +	if (fence && !dma_fence_add_callback(fence, &vb->fence_cb,
> +					     vb2_qbuf_fence_cb))
> +		return 0;

Shouldn't this return an error? How would userspace know this failed?

> +
>  	return __vb2_core_qbuf(vb, q);
>  }
>  EXPORT_SYMBOL_GPL(vb2_core_qbuf);
> @@ -1647,6 +1697,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
> @@ -1669,6 +1720,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;
> @@ -1735,7 +1794,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;
> @@ -2250,7 +2309,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;
> @@ -2429,7 +2488,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;
> @@ -2532,7 +2591,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 110fb45..e6ad77f 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>
> @@ -560,6 +561,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 +570,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 ret ? ret : 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 cb97c22..aa43e43 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)
> @@ -259,6 +260,9 @@ struct vb2_buffer {
>  
>  	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
> @@ -727,7 +731,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
> 

Regards,

	Hans

Re: [PATCH 05/12] [media] vivid: assign the specific device to the vb2_queue->dev

[Hans Verkuil]

On 06/16/17 09:39, Gustavo Padovan wrote:
> 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.
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>

Can you add a line to the commit log that says that this is needed for
the upcoming V4L2_EVENT_BUF_QUEUED support? This log message suggests that
the current vivid code is wrong, which it isn't. It just needs to be changed
so V4L2_EVENT_BUF_QUEUED can be supported.

After making that change:

Acked-by: Hans Verkuil <hans.verkuil@cisco.com>

Regards,

	Hans

> ---
>  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 ef344b9..8843170 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)
> 

Re: [PATCH 09/12] [media] vivid: mark vivid queues as ordered

[Hans Verkuil]

On 06/16/17 09:39, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.com>
> 
> To enable vivid to be used with explicit synchronization we need
> to mark its queues as ordered.
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>

Acked-by: Hans Verkuil <hans.verkuil@cisco.com>

Regards,

	Hans

> ---
>  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 8843170..c7bef90 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;
> 

Re: [PATCH 07/12] [media] v4l: add support to BUF_QUEUED event

[Hans Verkuil]

On 06/16/17 09:39, Gustavo Padovan wrote:
> 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.
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
> ---
>  drivers/media/v4l2-core/v4l2-ctrls.c     |  6 +++++-
>  drivers/media/v4l2-core/videobuf2-core.c | 15 +++++++++++++++
>  2 files changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index 5aed7bd..f55b5da 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -3435,8 +3435,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, 0, NULL);

This is dangerous. The '0' argument will only allocate room for a single
BUF_QUEUED event. So if two such events are triggered without the application
reading the first event, then the first event will be lost.

I recommend VIDEO_MAX_FRAME instead. I.e. have room for up to the maximum number
of buffers.

> +	}
>  	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 29aa9d4..00d9c35 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 <media/videobuf2-core.h>
> +#include <media/v4l2-event.h>
>  #include <media/v4l2-mc.h>
>  
>  #include <trace/events/vb2.h>
> @@ -1221,6 +1222,18 @@ static int __prepare_dmabuf(struct vb2_buffer *vb, const void *pb)
>  	return ret;
>  }
>  
> +static void vb2_buffer_queued_event(struct vb2_buffer *vb)
> +{
> +	struct video_device *vdev = to_video_device(vb->vb2_queue->dev);
> +	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(vdev, &event);
> +}
> +
>  /**
>   * __enqueue_in_driver() - enqueue a vb2_buffer in driver for processing
>   */
> @@ -1234,6 +1247,8 @@ static void __enqueue_in_driver(struct vb2_buffer *vb)
>  	trace_vb2_buf_queue(q, vb);
>  
>  	call_void_vb_qop(vb, buf_queue, vb);
> +
> +	vb2_buffer_queued_event(vb);
>  }
>  
>  static int __buf_prepare(struct vb2_buffer *vb, const void *pb)
> 

Regards,

	Hans

Re: [PATCH 08/12] [media] vb2: add 'ordered' property to queues

[Hans Verkuil]

On 06/16/17 09:39, 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 which the buffer were queued
> by userspace. This is already true for many drivers, but we never had
> the need 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 aa43e43..a8b800e 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -491,6 +491,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.

This needs to be clarified both here and in the commit log.

1) what is meant with 'ordered'? I assume FIFO is meant.
2) is it the order in which buffers are queued in the driver, or the order
in which buffers are queued in userspace? With in-fences it is ordered
in the driver but not in userspace since the in-fence will block a buffer
from being queued to the driver until the fence callback is called.
3) does this apply to in-fences or out-fences or both? It appears that this
only applies to out-fences.

>   * @fileio:	file io emulator internal data, used only if emulator is active
>   * @threadio:	thread io internal data, used only if thread is active
>   */
> @@ -541,6 +544,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;
> 

Regards,

	Hans

Re: [PATCH 03/12] [media] vb2: add in-fence support to QBUF

[Hans Verkuil]

On 06/16/17 09:39, 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.
> 
> 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/Kconfig                    |  1 +
>  drivers/media/v4l2-core/videobuf2-core.c | 97 +++++++++++++++++++++++++-------
>  drivers/media/v4l2-core/videobuf2-v4l2.c | 15 ++++-
>  include/media/videobuf2-core.h           |  7 ++-
>  4 files changed, 99 insertions(+), 21 deletions(-)
> 

<snip>

> diff --git a/drivers/media/v4l2-core/videobuf2-v4l2.c b/drivers/media/v4l2-core/videobuf2-v4l2.c
> index 110fb45..e6ad77f 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>
> @@ -560,6 +561,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 +570,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 ret ? ret : vb2_core_qbuf(q, b->index, b, fence);
>  }
>  EXPORT_SYMBOL_GPL(vb2_qbuf);

You need to adapt __fill_v4l2_buffer so it sets the IN_FENCE buffer flag
if there is a fence pending. It should also fill in fence_fd.

Regards,

	Hans

Re: [PATCH 12/12] [media] vb2: add out-fence support to QBUF

[Hans Verkuil]

On 06/16/17 09:39, Gustavo Padovan wrote:
> 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 for the buffer and return it to userspace on the fence_fd
> field. It only works with ordered queues.
> 
> The fence is signaled on buffer_done(), when the job on the buffer is
> finished.
> 
> v2: check if the queue is ordered.
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
> ---
>  drivers/media/v4l2-core/videobuf2-core.c |  6 ++++++
>  drivers/media/v4l2-core/videobuf2-v4l2.c | 22 +++++++++++++++++++++-
>  2 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> index 21cc4ed..a57902ee 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -356,6 +356,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 */
> @@ -940,6 +941,11 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state)
>  			__enqueue_in_driver(vb);
>  		return;
>  	default:
> +		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;

case VB2_BUF_STATE_REQUEUEING: a driver that calls this would break the ordering
requirement. I would suggest a WARN_ON_ONCE(q->ordered) call there together with
a comment.

> diff --git a/drivers/media/v4l2-core/videobuf2-v4l2.c b/drivers/media/v4l2-core/videobuf2-v4l2.c
> index e6ad77f..e2733dd 100644
> --- a/drivers/media/v4l2-core/videobuf2-v4l2.c
> +++ b/drivers/media/v4l2-core/videobuf2-v4l2.c
> @@ -204,9 +204,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->fence_fd = vb->out_fence_fd;
>  	b->reserved = 0;
>  
> +	if (vb->sync_file) {
> +		fd_install(vb->out_fence_fd, vb->sync_file->file);
> +		vb->sync_file = NULL;

I'm no fence expert, but this seems the wrong place for this.

> +	}
> +

You need to set the OUT_FENCE flag if there is a fence pending.

>  	if (q->is_multiplanar) {
>  		/*
>  		 * Fill in plane-related data if userspace provided an array
> @@ -581,6 +586,21 @@ int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
>  		}
>  	}
>  
> +	if (b->flags & V4L2_BUF_FLAG_OUT_FENCE) {
> +		if (!q->ordered) {
> +			dprintk(1, "can't use out-fences with unordered queues\n");
> +			dma_fence_put(fence);
> +			return -EINVAL;
> +		}
> +
> +		ret = vb2_setup_out_fence(q, b->index);
> +		if (ret) {
> +			dprintk(1, "failed to set up out-fence\n");
> +			dma_fence_put(fence);
> +			return ret;
> +		}
> +	}
> +
>  	return ret ? ret : vb2_core_qbuf(q, b->index, b, fence);

Wouldn't it make more sense to do the fd_install after this call?

Besides, if vb2_core_qbuf returns with an error, then I expect that
we need to clean up the fence?

>  }
>  EXPORT_SYMBOL_GPL(vb2_qbuf);
> 

Regards,

	Hans

Re: [PATCH 12/12] [media] vb2: add out-fence support to QBUF

[Hans Verkuil]

On 06/16/17 09:39, Gustavo Padovan wrote:
> 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 for the buffer and return it to userspace on the fence_fd
> field. It only works with ordered queues.
> 
> The fence is signaled on buffer_done(), when the job on the buffer is
> finished.
> 
> v2: check if the queue is ordered.
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
> ---
>  drivers/media/v4l2-core/videobuf2-core.c |  6 ++++++
>  drivers/media/v4l2-core/videobuf2-v4l2.c | 22 +++++++++++++++++++++-
>  2 files changed, 27 insertions(+), 1 deletion(-)
> 

<snip>

> diff --git a/drivers/media/v4l2-core/videobuf2-v4l2.c b/drivers/media/v4l2-core/videobuf2-v4l2.c
> index e6ad77f..e2733dd 100644
> --- a/drivers/media/v4l2-core/videobuf2-v4l2.c
> +++ b/drivers/media/v4l2-core/videobuf2-v4l2.c
> @@ -204,9 +204,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->fence_fd = vb->out_fence_fd;

I forgot to ask: can a buffer have both an in and an out fence? If so, then we
have a problem here since we can report only one fence fd.

If it is not allowed, then we need a check for that somewhere.

Regards,

	Hans

Re: [PATCH 07/12] [media] v4l: add support to BUF_QUEUED event

[Hans Verkuil]

On 06/30/17 14:04, Mauro Carvalho Chehab wrote:
> Em Fri, 16 Jun 2017 16:39:10 +0900
> Gustavo Padovan <gustavo@padovan.org> escreveu:
> 
>> 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.
>>
>> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
>> ---
>>  drivers/media/v4l2-core/v4l2-ctrls.c     |  6 +++++-
>>  drivers/media/v4l2-core/videobuf2-core.c | 15 +++++++++++++++
>>  2 files changed, 20 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
>> index 5aed7bd..f55b5da 100644
>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
>> @@ -3435,8 +3435,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, 0, 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 29aa9d4..00d9c35 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 <media/videobuf2-core.h>
>> +#include <media/v4l2-event.h>
>>  #include <media/v4l2-mc.h>
>>  
>>  #include <trace/events/vb2.h>
>> @@ -1221,6 +1222,18 @@ static int __prepare_dmabuf(struct vb2_buffer *vb, const void *pb)
>>  	return ret;
>>  }
>>  
>> +static void vb2_buffer_queued_event(struct vb2_buffer *vb)
>> +{
>> +	struct video_device *vdev = to_video_device(vb->vb2_queue->dev);
>> +	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(vdev, &event);
>> +}
>> +
> 
> It doesn't sound right to add a V4L2 event to VB2 core. The hole point
> of splitting the core from V4L2 specific stuff is to allow VB2 to be
> used by non-V4L2 APIs[1]. Please move this to videobuf2-v4l2.

Good point. So this should be a callback to the higher level.

One thing I was wondering about: v4l2_event_queue sends the event to all
open filehandles of the video node that subscribed to this event. Is that
what we want? Or should we use v4l2_event_queue_fh to only send it to the
vb2 queue owner? I don't know what is best. I think it is OK to send it
to anyone that is interested. If nothing else it will help debugging.

Regards,

	Hans

> 
> [1] The split happened as part of a patchset meant to make the DVB
> core to use VB2 and provide DMA APIs to it. Unfortunately, the
> developer that worked on this project moved to some other project.
> The final patch was not applied yet. I have it on my patchwork
> queue. I intend to test and apply it sometime this year.
> 
> 
> 
>>  /**
>>   * __enqueue_in_driver() - enqueue a vb2_buffer in driver for processing
>>   */
>> @@ -1234,6 +1247,8 @@ static void __enqueue_in_driver(struct vb2_buffer *vb)
>>  	trace_vb2_buf_queue(q, vb);
>>  
>>  	call_void_vb_qop(vb, buf_queue, vb);
>> +
>> +	vb2_buffer_queued_event(vb);
>>  }
>>  
>>  static int __buf_prepare(struct vb2_buffer *vb, const void *pb)
> 
> 

Re: [PATCH 03/12] [media] vb2: add in-fence support to QBUF

[Hans Verkuil]

On 07/03/17 20:16, Gustavo Padovan wrote:
>>> @@ -1436,6 +1481,11 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
>>>  	if (pb)
>>>  		call_void_bufop(q, fill_user_buffer, vb, pb);
>>>  
>>> +	vb->in_fence = fence;
>>> +	if (fence && !dma_fence_add_callback(fence, &vb->fence_cb,
>>> +					     vb2_qbuf_fence_cb))
>>> +		return 0;
>>
>> Maybe we should provide some error or debug log here or a WARN_ON(), if 
>> dma_fence_add_callback() fails instead of silently ignore any errors.
> 
> This is not an error. If the if succeeds it mean we have installed a
> callback for the fence. If not, it means the fence signaled already and
> we don't can call __vb2_core_qbuf right away.

I had the same question as Mauro. After looking at the dma_fence_add_callback
code I see what you mean, but a comment would certainly be helpful.

Also, should you set vb->in_fence to NULL if the fence signaled already?
I'm not sure if you need to call 'dma_fence_put(vb->in_fence);' as well.
You would know that better than I do.

Regards,

	Hans

Re: [PATCH 02/12] [media] vb2: split out queueing from vb_core_qbuf()

[Gustavo Padovan]

2017-07-06 Hans Verkuil <hverkuil@xs4all.nl>:

> On 06/16/17 09:39, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo.padovan@collabora.com>
> > 
> > In order to support explicit synchronization we need to divide
> > vb2_core_qbuf() in two parts, one to be executed before the fence
> > signals and another one to do the actual queueing of the buffer.
> > 
> > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
> > ---
> >  drivers/media/v4l2-core/videobuf2-core.c | 51 ++++++++++++++++++--------------
> >  1 file changed, 29 insertions(+), 22 deletions(-)
> > 
> > diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> > index 3107e21..ea83126 100644
> > --- a/drivers/media/v4l2-core/videobuf2-core.c
> > +++ b/drivers/media/v4l2-core/videobuf2-core.c
> > @@ -1367,6 +1367,34 @@ static int vb2_start_streaming(struct vb2_queue *q)
> >  	return ret;
> >  }
> >  
> > +static int __vb2_core_qbuf(struct vb2_buffer *vb, struct vb2_queue *q)
> > +{
> > +	int ret;
> > +
> > +	/*
> > +	 * 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);
> > +
> > +	/*
> > +	 * 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 (q->streaming && !q->start_streaming_called &&
> > +	    q->queued_count >= q->min_buffers_needed) {
> > +		ret = vb2_start_streaming(q);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	dprintk(1, "qbuf of buffer %d succeeded\n", vb->index);
> > +	return 0;
> > +}
> > +
> >  int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
> >  {
> >  	struct vb2_buffer *vb;
> > @@ -1404,32 +1432,11 @@ 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);
> 
> This should be called *after* the __vb2_core_qbuf call. That call changes
> vb->state which is used by buffer to fill in v4l2_buffer. So the order
> should be swapped here to ensure we return the latest state of the buffer.

I didn't pay attention to that detail. The reason why I put it here is
that later when in-fences are introduced we will only call
__vb2_core_qbuf() after the fence signals and that may happen after we
returned to userspace. I'll look into possible ways to solve this
problem.

Gustavo

Re: [PATCH 03/12] [media] vb2: add in-fence support to QBUF

[Gustavo Padovan]

2017-07-06 Hans Verkuil <hverkuil@xs4all.nl>:

> On 07/03/17 20:16, Gustavo Padovan wrote:
> >>> @@ -1436,6 +1481,11 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
> >>>  	if (pb)
> >>>  		call_void_bufop(q, fill_user_buffer, vb, pb);
> >>>  
> >>> +	vb->in_fence = fence;
> >>> +	if (fence && !dma_fence_add_callback(fence, &vb->fence_cb,
> >>> +					     vb2_qbuf_fence_cb))
> >>> +		return 0;
> >>
> >> Maybe we should provide some error or debug log here or a WARN_ON(), if 
> >> dma_fence_add_callback() fails instead of silently ignore any errors.
> > 
> > This is not an error. If the if succeeds it mean we have installed a
> > callback for the fence. If not, it means the fence signaled already and
> > we don't can call __vb2_core_qbuf right away.
> 
> I had the same question as Mauro. After looking at the dma_fence_add_callback
> code I see what you mean, but a comment would certainly be helpful.

Sure, I'll add a comment explaining it.

> 
> Also, should you set vb->in_fence to NULL if the fence signaled already?
> I'm not sure if you need to call 'dma_fence_put(vb->in_fence);' as well.
> You would know that better than I do.

You got it right. I should be doing that.

Gustavo

Re: [PATCH 03/12] [media] vb2: add in-fence support to QBUF

[Gustavo Padovan]

2017-07-06 Hans Verkuil <hverkuil@xs4all.nl>:

> On 06/16/17 09:39, 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.
> > 
> > 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/Kconfig                    |  1 +
> >  drivers/media/v4l2-core/videobuf2-core.c | 97 +++++++++++++++++++++++++-------
> >  drivers/media/v4l2-core/videobuf2-v4l2.c | 15 ++++-
> >  include/media/videobuf2-core.h           |  7 ++-
> >  4 files changed, 99 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/media/Kconfig b/drivers/media/Kconfig
> > index 55d9c2b..3cd1d3d 100644
> > --- a/drivers/media/Kconfig
> > +++ b/drivers/media/Kconfig
> > @@ -11,6 +11,7 @@ config CEC_NOTIFIER
> >  menuconfig MEDIA_SUPPORT
> >  	tristate "Multimedia support"
> >  	depends on HAS_IOMEM
> > +	select SYNC_FILE
> 
> Is this the right place for this? Shouldn't this be selected in
> 'config VIDEOBUF2_CORE'?
> 
> Fences are specific to vb2 after all.

Definitely.

> 
> >  	help
> >  	  If you want to use Webcams, Video grabber devices and/or TV devices
> >  	  enable this option and other options below.
> > diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> > index ea83126..29aa9d4 100644

> > --- a/drivers/media/v4l2-core/videobuf2-core.c
> > +++ b/drivers/media/v4l2-core/videobuf2-core.c
> > @@ -1279,6 +1279,22 @@ 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))
> > +			break;
> 
> Obviously the break is wrong as Mauro mentioned.

I replied this in the other email to Mauro, if a fence is not signaled
it is not ready te be queued by the driver nor is all buffers following
it. Hence the break. They need all to be in order and in front of the
queue.

In any case I'll check this again as now there is two people saying I'm
wrong! :)

> 
> > +
> > +		ready_count++;
> > +	}
> > +
> > +	return ready_count;
> > +}
> > +
> >  int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb)
> >  {
> >  	struct vb2_buffer *vb;
> > @@ -1324,8 +1340,15 @@ static int vb2_start_streaming(struct vb2_queue *q)
> >  	 * If any buffers were queued before streamon,
> >  	 * we can now pass them to driver for processing.
> >  	 */
> > -	list_for_each_entry(vb, &q->queued_list, queued_entry)
> > +	list_for_each_entry(vb, &q->queued_list, queued_entry) {
> > +		if (vb->state != VB2_BUF_STATE_QUEUED)
> > +			continue;
> 
> I think this test is unnecessary.

It might be indeed. It is necessary in __vb2_core_qbuf() so I think I
just copied it here without thinking.

> 
> > +
> > +		if (vb->in_fence && !dma_fence_is_signaled(vb->in_fence))
> > +			break;
> > +
> >  		__enqueue_in_driver(vb);
> 
> I would move the above test (after fixing it as Mauro said) to __enqueue_in_driver.
> I.e. if this is waiting for a fence then __enqueue_in_driver does nothing.

Yes, having this check inside __enqueue_in_driver() makes it looks much
nicer.

> 
> > +	}
> >  
> >  	/* Tell the driver to start streaming */
> >  	q->start_streaming_called = 1;
> > @@ -1369,33 +1392,55 @@ static int vb2_start_streaming(struct vb2_queue *q)
> >  
> >  static int __vb2_core_qbuf(struct vb2_buffer *vb, struct vb2_queue *q)
> >  {
> > +	struct vb2_buffer *b;
> >  	int ret;
> >  
> >  	/*
> >  	 * 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);
> >  
> > -	/*
> > -	 * 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 (q->streaming && !q->start_streaming_called &&
> > -	    q->queued_count >= q->min_buffers_needed) {
> > -		ret = vb2_start_streaming(q);
> > -		if (ret)
> > -			return ret;
> > +	if (q->start_streaming_called) {
> > +		list_for_each_entry(b, &q->queued_list, queued_entry) {
> > +			if (b->state != VB2_BUF_STATE_QUEUED)
> > +				continue;
> > +
> > +			if (b->in_fence && !dma_fence_is_signaled(b->in_fence))
> > +				break;
> 
> Again, if this test is in __enqueue_in_driver, then you can keep the
> original code. Why would you need to loop over all buffers anyway?
> 
> If a fence is ready then the callback will call this function for that
> buffer. Everything works fine AFAICT without looping over buffers here.

That seems correct. I'll just remove this loop.

	Gustavo

Re: [PATCH 03/12] [media] vb2: add in-fence support to QBUF

[Gustavo Padovan]

2017-07-06 Hans Verkuil <hverkuil@xs4all.nl>:

> On 06/16/17 09:39, 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.
> > 
> > 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/Kconfig                    |  1 +
> >  drivers/media/v4l2-core/videobuf2-core.c | 97 +++++++++++++++++++++++++-------
> >  drivers/media/v4l2-core/videobuf2-v4l2.c | 15 ++++-
> >  include/media/videobuf2-core.h           |  7 ++-
> >  4 files changed, 99 insertions(+), 21 deletions(-)
> > 
> 
> <snip>
> 
> > diff --git a/drivers/media/v4l2-core/videobuf2-v4l2.c b/drivers/media/v4l2-core/videobuf2-v4l2.c
> > index 110fb45..e6ad77f 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>
> > @@ -560,6 +561,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 +570,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 ret ? ret : vb2_core_qbuf(q, b->index, b, fence);
> >  }
> >  EXPORT_SYMBOL_GPL(vb2_qbuf);
> 
> You need to adapt __fill_v4l2_buffer so it sets the IN_FENCE buffer flag
> if there is a fence pending. It should also fill in fence_fd.

It was userspace who sent the fence_fd in the first place. Why do we
need to send it back? The initial plan was - from a userspace point of view
- to send the in-fence in the fence_fd field and receive the out-fence
 in the same field.

As per setting the IN_FENCE flag, that is racy, as the fence can signal
just after we called __fill_v4l2_buffer. Why is it important to set it?

	Gustavo

Re: [PATCH 03/12] [media] vb2: add in-fence support to QBUF

[Hans Verkuil]

On 07/07/2017 04:00 AM, Gustavo Padovan wrote:
> 2017-07-06 Hans Verkuil <hverkuil@xs4all.nl>:
> 
>> On 06/16/17 09:39, 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.
>>>
>>> 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/Kconfig                    |  1 +
>>>   drivers/media/v4l2-core/videobuf2-core.c | 97 +++++++++++++++++++++++++-------
>>>   drivers/media/v4l2-core/videobuf2-v4l2.c | 15 ++++-
>>>   include/media/videobuf2-core.h           |  7 ++-
>>>   4 files changed, 99 insertions(+), 21 deletions(-)
>>>
>>
>> <snip>
>>
>>> diff --git a/drivers/media/v4l2-core/videobuf2-v4l2.c b/drivers/media/v4l2-core/videobuf2-v4l2.c
>>> index 110fb45..e6ad77f 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>
>>> @@ -560,6 +561,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 +570,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 ret ? ret : vb2_core_qbuf(q, b->index, b, fence);
>>>   }
>>>   EXPORT_SYMBOL_GPL(vb2_qbuf);
>>
>> You need to adapt __fill_v4l2_buffer so it sets the IN_FENCE buffer flag
>> if there is a fence pending. It should also fill in fence_fd.
> 
> It was userspace who sent the fence_fd in the first place. Why do we
> need to send it back? The initial plan was - from a userspace point of view
> - to send the in-fence in the fence_fd field and receive the out-fence
>   in the same field.
> 
> As per setting the IN_FENCE flag, that is racy, as the fence can signal
> just after we called __fill_v4l2_buffer. Why is it important to set it?

Because when running VIDIOC_QUERYBUF it should return the current state of
the buffer, including whether it has a fence. You can do something like
v4l2-ctl --list-buffers to see how many buffers are queued up and the state
of each buffer. Can be useful to e.g. figure out why a video capture seems
to stall. Knowing that all queued buffers are all waiting for a fence is
very useful information. Whether the fence_fd should also be set here or
only in dqbuf is something I don't know (not enough knowledge about how
fences are supposed to work). The IN/OUT_FENCE flags should definitely be
reported though.

Anyway, remember that this callback is called from various vb2 ioctls:
qbuf, dqbuf, querybuf and prepare_buf.

Querybuf especially can be called at any time.

Regards,

	Hans

Re: [PATCH 03/12] [media] vb2: add in-fence support to QBUF

[Hans Verkuil]

On 07/07/2017 03:53 AM, Gustavo Padovan wrote:
>>
>>>   	help
>>>   	  If you want to use Webcams, Video grabber devices and/or TV devices
>>>   	  enable this option and other options below.
>>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
>>> index ea83126..29aa9d4 100644
> 
>>> --- a/drivers/media/v4l2-core/videobuf2-core.c
>>> +++ b/drivers/media/v4l2-core/videobuf2-core.c
>>> @@ -1279,6 +1279,22 @@ 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))
>>> +			break;
>>
>> Obviously the break is wrong as Mauro mentioned.
> 
> I replied this in the other email to Mauro, if a fence is not signaled
> it is not ready te be queued by the driver nor is all buffers following
> it. Hence the break. They need all to be in order and in front of the
> queue.
> 
> In any case I'll check this again as now there is two people saying I'm
> wrong! :)

I think this comes back to the 'ordered' requirement and what that means
exactly. In this particular case if I have buffers queued up in vb2 without
a fence (or the fence was signaled), why shouldn't it possible to queue them
up to the driver right away?

Of course, if all buffers are waiting for a fence, then __get_num_ready_buffers
returns 0 and nothing happens.

My understanding is that the ordered requirement is for the hardware,
i.e. queueing buffers A, B, C to ordered hardware requires that they come
out in the same order.

If 'ordered' means that the qbuf/dqbuf sequence must be ordered which implies
that vb2 also needs to keep them ordered, then I need to review the code again.

Can you explain (or point to an explanation) the reason behind the ordered
requirement?

I think you explained it to me when we met during a conference, but I've
forgotten the details.

Regards,

	Hans

Re: [PATCH 04/12] [media] uvc: enable subscriptions to other events

[Shuah Khan]

On 06/16/2017 01:39 AM, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.com>
> 
> Call v4l2_ctrl_subscribe_event to subscribe to more events supported by
> v4l.
> 
> 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 3e7e283..dfa0ccd 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);

This looks incorrect. With this change driver will be subscribing to all
v4l2 events? Is this the intent?

>  	}
>  }
>  
> 

thanks,
-- Shuah

Re: [PATCH 05/12] [media] vivid: assign the specific device to the vb2_queue->dev

[Shuah Khan]

On 06/16/2017 01:39 AM, Gustavo Padovan wrote:
> 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.
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.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 ef344b9..8843170 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;

Does this work in all cases? My concern is that in some code paths
q->dev might be used to initiate release perhaps.

Fore example v4l2_dev.release is vivid_dev_release()
dev->v4l2_dev.release = vivid_dev_release;

vid_cap_dev release is video_device_release_empty

This is one difference, but there might be others and the code paths
that might depend on q->dev being the v4l2_dev.dev which is the global
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)
> 

thanks,
-- Shuah

Re: [PATCH 09/12] [media] vivid: mark vivid queues as ordered

[Shuah Khan]

On 06/16/2017 01:39 AM, Gustavo Padovan wrote:
> From: Gustavo Padovan <gustavo.padovan@collabora.com>
> 
> To enable vivid to be used with explicit synchronization we need
> to mark its queues as ordered.
> 
> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.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 8843170..c7bef90 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;

How will the driver ensure ordered buffers? Are there more changes needed
in this driver?

>  		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;
> 

thanks,
-- Shuah

Re: [PATCH 03/12] [media] vb2: add in-fence support to QBUF

[Gustavo Padovan]

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

> On 07/07/2017 04:00 AM, Gustavo Padovan wrote:
> > 2017-07-06 Hans Verkuil <hverkuil@xs4all.nl>:
> > 
> > > On 06/16/17 09:39, 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.
> > > > 
> > > > 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/Kconfig                    |  1 +
> > > >   drivers/media/v4l2-core/videobuf2-core.c | 97 +++++++++++++++++++++++++-------
> > > >   drivers/media/v4l2-core/videobuf2-v4l2.c | 15 ++++-
> > > >   include/media/videobuf2-core.h           |  7 ++-
> > > >   4 files changed, 99 insertions(+), 21 deletions(-)
> > > > 
> > > 
> > > <snip>
> > > 
> > > > diff --git a/drivers/media/v4l2-core/videobuf2-v4l2.c b/drivers/media/v4l2-core/videobuf2-v4l2.c
> > > > index 110fb45..e6ad77f 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>
> > > > @@ -560,6 +561,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 +570,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 ret ? ret : vb2_core_qbuf(q, b->index, b, fence);
> > > >   }
> > > >   EXPORT_SYMBOL_GPL(vb2_qbuf);
> > > 
> > > You need to adapt __fill_v4l2_buffer so it sets the IN_FENCE buffer flag
> > > if there is a fence pending. It should also fill in fence_fd.
> > 
> > It was userspace who sent the fence_fd in the first place. Why do we
> > need to send it back? The initial plan was - from a userspace point of view
> > - to send the in-fence in the fence_fd field and receive the out-fence
> >   in the same field.
> > 
> > As per setting the IN_FENCE flag, that is racy, as the fence can signal
> > just after we called __fill_v4l2_buffer. Why is it important to set it?
> 
> Because when running VIDIOC_QUERYBUF it should return the current state of
> the buffer, including whether it has a fence. You can do something like
> v4l2-ctl --list-buffers to see how many buffers are queued up and the state
> of each buffer. Can be useful to e.g. figure out why a video capture seems
> to stall. Knowing that all queued buffers are all waiting for a fence is
> very useful information. Whether the fence_fd should also be set here or
> only in dqbuf is something I don't know (not enough knowledge about how
> fences are supposed to work). The IN/OUT_FENCE flags should definitely be
> reported though.

I didn't know about this usecase. Thanks for explaining. It certainly
makes sense to set the IN/OUT_FENCE flags here. Regarding the fence_fd
I would expect the application to know the fence fd associated to than
buffer. If we expect an application other than the one which issued
QBUF than I'd say we also need to provide the fd on VIDIOC_QUERYBUF
for inspection purposes. Is that the case?

Gustavo

Re: [PATCH 03/12] [media] vb2: add in-fence support to QBUF

[Gustavo Padovan]

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

> On 07/07/2017 03:53 AM, Gustavo Padovan wrote:
> > > 
> > > >   	help
> > > >   	  If you want to use Webcams, Video grabber devices and/or TV devices
> > > >   	  enable this option and other options below.
> > > > diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> > > > index ea83126..29aa9d4 100644
> > 
> > > > --- a/drivers/media/v4l2-core/videobuf2-core.c
> > > > +++ b/drivers/media/v4l2-core/videobuf2-core.c
> > > > @@ -1279,6 +1279,22 @@ 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))
> > > > +			break;
> > > 
> > > Obviously the break is wrong as Mauro mentioned.
> > 
> > I replied this in the other email to Mauro, if a fence is not signaled
> > it is not ready te be queued by the driver nor is all buffers following
> > it. Hence the break. They need all to be in order and in front of the
> > queue.
> > 
> > In any case I'll check this again as now there is two people saying I'm
> > wrong! :)
> 
> I think this comes back to the 'ordered' requirement and what that means
> exactly. In this particular case if I have buffers queued up in vb2 without
> a fence (or the fence was signaled), why shouldn't it possible to queue them
> up to the driver right away?
> 
> Of course, if all buffers are waiting for a fence, then __get_num_ready_buffers
> returns 0 and nothing happens.
> 
> My understanding is that the ordered requirement is for the hardware,
> i.e. queueing buffers A, B, C to ordered hardware requires that they come
> out in the same order.

That is correct. I thought I had to queue to the hardware in the order we
receive from userspace. So that makes that loop indeed wrong, as we
should queue the buffers right away.

The ordered requirement is for the OUT_FENCE side because after we queue
the buffer to the hardware and send the BUF_QUEUED event out userspace
might just go ahead and issue an DRM Atomic Request containing that
buffer and the out-fence fd received. DRM then needs to wait on that
fence before any scanout, but it may wait after the scanout is not
allowed to fail anymore.

Thus if a buffer requeuing happens at buffer_done() the fence won't
signal and DRM/KMS won't have a buffer to display. That is reason behind
it.

Alternatively we can ignore this and live with the fact that sometimes a
requeuing may affect the scanout pipeline.

	Gustavo

Re: [PATCH 04/12] [media] uvc: enable subscriptions to other events

[Gustavo Padovan]

2017-07-07 Shuah Khan <shuahkh@osg.samsung.com>:

> On 06/16/2017 01:39 AM, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo.padovan@collabora.com>
> > 
> > Call v4l2_ctrl_subscribe_event to subscribe to more events supported by
> > v4l.
> > 
> > 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 3e7e283..dfa0ccd 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);
> 
> This looks incorrect. With this change driver will be subscribing to all
> v4l2 events? Is this the intent?

The intent was to enable this driver to subscribe to BUF_QUEUED events. 
It is the only one who can't at the moment. I'll review this.

	Gustavo

Re: [PATCH 05/12] [media] vivid: assign the specific device to the vb2_queue->dev

[Gustavo Padovan]

2017-07-07 Shuah Khan <shuahkh@osg.samsung.com>:

> On 06/16/2017 01:39 AM, Gustavo Padovan wrote:
> > 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.
> > 
> > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.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 ef344b9..8843170 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;
> 
> Does this work in all cases? My concern is that in some code paths
> q->dev might be used to initiate release perhaps.
> 
> Fore example v4l2_dev.release is vivid_dev_release()
> dev->v4l2_dev.release = vivid_dev_release;
> 
> vid_cap_dev release is video_device_release_empty
> 
> This is one difference, but there might be others and the code paths
> that might depend on q->dev being the v4l2_dev.dev which is the global
> dev.

Sure, I'll check this again.

	Gustavo

Re: [PATCH 07/12] [media] v4l: add support to BUF_QUEUED event

[Gustavo Padovan]

2017-07-06 Hans Verkuil <hverkuil@xs4all.nl>:

> On 06/30/17 14:04, Mauro Carvalho Chehab wrote:
> > Em Fri, 16 Jun 2017 16:39:10 +0900
> > Gustavo Padovan <gustavo@padovan.org> escreveu:
> > 
> >> 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.
> >>
> >> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
> >> ---
> >>  drivers/media/v4l2-core/v4l2-ctrls.c     |  6 +++++-
> >>  drivers/media/v4l2-core/videobuf2-core.c | 15 +++++++++++++++
> >>  2 files changed, 20 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> >> index 5aed7bd..f55b5da 100644
> >> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> >> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> >> @@ -3435,8 +3435,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, 0, 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 29aa9d4..00d9c35 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 <media/videobuf2-core.h>
> >> +#include <media/v4l2-event.h>
> >>  #include <media/v4l2-mc.h>
> >>  
> >>  #include <trace/events/vb2.h>
> >> @@ -1221,6 +1222,18 @@ static int __prepare_dmabuf(struct vb2_buffer *vb, const void *pb)
> >>  	return ret;
> >>  }
> >>  
> >> +static void vb2_buffer_queued_event(struct vb2_buffer *vb)
> >> +{
> >> +	struct video_device *vdev = to_video_device(vb->vb2_queue->dev);
> >> +	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(vdev, &event);
> >> +}
> >> +
> > 
> > It doesn't sound right to add a V4L2 event to VB2 core. The hole point
> > of splitting the core from V4L2 specific stuff is to allow VB2 to be
> > used by non-V4L2 APIs[1]. Please move this to videobuf2-v4l2.
> 
> Good point. So this should be a callback to the higher level.
> 
> One thing I was wondering about: v4l2_event_queue sends the event to all
> open filehandles of the video node that subscribed to this event. Is that
> what we want? Or should we use v4l2_event_queue_fh to only send it to the
> vb2 queue owner? I don't know what is best. I think it is OK to send it
> to anyone that is interested. If nothing else it will help debugging.

I don't have any preference here, I'll keep it as is - sending events to
anyone - if no one objects.

	Gustavo

Re: [PATCH 09/12] [media] vivid: mark vivid queues as ordered

[Gustavo Padovan]

2017-07-07 Shuah Khan <shuahkh@osg.samsung.com>:

> On 06/16/2017 01:39 AM, Gustavo Padovan wrote:
> > From: Gustavo Padovan <gustavo.padovan@collabora.com>
> > 
> > To enable vivid to be used with explicit synchronization we need
> > to mark its queues as ordered.
> > 
> > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.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 8843170..c7bef90 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;
> 
> How will the driver ensure ordered buffers? Are there more changes needed
> in this driver?

The driver can't requeue a buffer if it mark itself as ordered. It has
to ensure that.

	Gustavo

Re: [PATCH 12/12] [media] vb2: add out-fence support to QBUF

[Gustavo Padovan]

2017-07-06 Hans Verkuil <hverkuil@xs4all.nl>:

> On 06/16/17 09:39, Gustavo Padovan wrote:
> > 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 for the buffer and return it to userspace on the fence_fd
> > field. It only works with ordered queues.
> > 
> > The fence is signaled on buffer_done(), when the job on the buffer is
> > finished.
> > 
> > v2: check if the queue is ordered.
> > 
> > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.com>
> > ---
> >  drivers/media/v4l2-core/videobuf2-core.c |  6 ++++++
> >  drivers/media/v4l2-core/videobuf2-v4l2.c | 22 +++++++++++++++++++++-
> >  2 files changed, 27 insertions(+), 1 deletion(-)
> > 
> 
> <snip>
> 
> > diff --git a/drivers/media/v4l2-core/videobuf2-v4l2.c b/drivers/media/v4l2-core/videobuf2-v4l2.c
> > index e6ad77f..e2733dd 100644
> > --- a/drivers/media/v4l2-core/videobuf2-v4l2.c
> > +++ b/drivers/media/v4l2-core/videobuf2-v4l2.c
> > @@ -204,9 +204,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->fence_fd = vb->out_fence_fd;
> 
> I forgot to ask: can a buffer have both an in and an out fence? If so, then we
> have a problem here since we can report only one fence fd.
> 
> If it is not allowed, then we need a check for that somewhere.

It is perfect fine to have both, I just wasn't expecting to get them out
from the kernel at the same time. In-fence is set by userspace in the
fence_fd field and then sent to the kernel, the field is then reused to
send the out-fence created in the kernel to userspace. That is how GPU
drivers do it.

	Gustavo

Re: [PATCH 03/12] [media] vb2: add in-fence support to QBUF

[Gustavo Padovan]

2017-07-10 Gustavo Padovan <gustavo@padovan.org>:

> 2017-07-07 Hans Verkuil <hverkuil@xs4all.nl>:
> 
> > On 07/07/2017 04:00 AM, Gustavo Padovan wrote:
> > > 2017-07-06 Hans Verkuil <hverkuil@xs4all.nl>:
> > > 
> > > > On 06/16/17 09:39, 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.
> > > > > 
> > > > > 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/Kconfig                    |  1 +
> > > > >   drivers/media/v4l2-core/videobuf2-core.c | 97 +++++++++++++++++++++++++-------
> > > > >   drivers/media/v4l2-core/videobuf2-v4l2.c | 15 ++++-
> > > > >   include/media/videobuf2-core.h           |  7 ++-
> > > > >   4 files changed, 99 insertions(+), 21 deletions(-)
> > > > > 
> > > > 
> > > > <snip>
> > > > 
> > > > > diff --git a/drivers/media/v4l2-core/videobuf2-v4l2.c b/drivers/media/v4l2-core/videobuf2-v4l2.c
> > > > > index 110fb45..e6ad77f 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>
> > > > > @@ -560,6 +561,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 +570,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 ret ? ret : vb2_core_qbuf(q, b->index, b, fence);
> > > > >   }
> > > > >   EXPORT_SYMBOL_GPL(vb2_qbuf);
> > > > 
> > > > You need to adapt __fill_v4l2_buffer so it sets the IN_FENCE buffer flag
> > > > if there is a fence pending. It should also fill in fence_fd.
> > > 
> > > It was userspace who sent the fence_fd in the first place. Why do we
> > > need to send it back? The initial plan was - from a userspace point of view
> > > - to send the in-fence in the fence_fd field and receive the out-fence
> > >   in the same field.
> > > 
> > > As per setting the IN_FENCE flag, that is racy, as the fence can signal
> > > just after we called __fill_v4l2_buffer. Why is it important to set it?
> > 
> > Because when running VIDIOC_QUERYBUF it should return the current state of
> > the buffer, including whether it has a fence. You can do something like
> > v4l2-ctl --list-buffers to see how many buffers are queued up and the state
> > of each buffer. Can be useful to e.g. figure out why a video capture seems
> > to stall. Knowing that all queued buffers are all waiting for a fence is
> > very useful information. Whether the fence_fd should also be set here or
> > only in dqbuf is something I don't know (not enough knowledge about how
> > fences are supposed to work). The IN/OUT_FENCE flags should definitely be
> > reported though.
> 
> I didn't know about this usecase. Thanks for explaining. It certainly
> makes sense to set the IN/OUT_FENCE flags here. Regarding the fence_fd
> I would expect the application to know the fence fd associated to than
> buffer. If we expect an application other than the one which issued
> QBUF than I'd say we also need to provide the fd on VIDIOC_QUERYBUF
> for inspection purposes. Is that the case?

I just realized that if we want to also set the in-fence fd here we
actually need to get a new unused fd, as either it is a different pid or
the app already closed the fd it was using previously. Given this extra
complication I'd say we shouldn't set fence fd unless someone has an
usecase in mind.

	Gustavo

Re: [PATCH 03/12] [media] vb2: add in-fence support to QBUF

[Hans Verkuil]

On 10/07/17 22:26, Gustavo Padovan wrote:
> 2017-07-10 Gustavo Padovan <gustavo@padovan.org>:
> 
>> 2017-07-07 Hans Verkuil <hverkuil@xs4all.nl>:
>>
>>> On 07/07/2017 04:00 AM, Gustavo Padovan wrote:
>>>> 2017-07-06 Hans Verkuil <hverkuil@xs4all.nl>:
>>>>
>>>>> On 06/16/17 09:39, 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.
>>>>>>
>>>>>> 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/Kconfig                    |  1 +
>>>>>>   drivers/media/v4l2-core/videobuf2-core.c | 97 +++++++++++++++++++++++++-------
>>>>>>   drivers/media/v4l2-core/videobuf2-v4l2.c | 15 ++++-
>>>>>>   include/media/videobuf2-core.h           |  7 ++-
>>>>>>   4 files changed, 99 insertions(+), 21 deletions(-)
>>>>>>
>>>>>
>>>>> <snip>
>>>>>
>>>>>> diff --git a/drivers/media/v4l2-core/videobuf2-v4l2.c b/drivers/media/v4l2-core/videobuf2-v4l2.c
>>>>>> index 110fb45..e6ad77f 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>
>>>>>> @@ -560,6 +561,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 +570,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 ret ? ret : vb2_core_qbuf(q, b->index, b, fence);
>>>>>>   }
>>>>>>   EXPORT_SYMBOL_GPL(vb2_qbuf);
>>>>>
>>>>> You need to adapt __fill_v4l2_buffer so it sets the IN_FENCE buffer flag
>>>>> if there is a fence pending. It should also fill in fence_fd.
>>>>
>>>> It was userspace who sent the fence_fd in the first place. Why do we
>>>> need to send it back? The initial plan was - from a userspace point of view
>>>> - to send the in-fence in the fence_fd field and receive the out-fence
>>>>   in the same field.
>>>>
>>>> As per setting the IN_FENCE flag, that is racy, as the fence can signal
>>>> just after we called __fill_v4l2_buffer. Why is it important to set it?
>>>
>>> Because when running VIDIOC_QUERYBUF it should return the current state of
>>> the buffer, including whether it has a fence. You can do something like
>>> v4l2-ctl --list-buffers to see how many buffers are queued up and the state
>>> of each buffer. Can be useful to e.g. figure out why a video capture seems
>>> to stall. Knowing that all queued buffers are all waiting for a fence is
>>> very useful information. Whether the fence_fd should also be set here or
>>> only in dqbuf is something I don't know (not enough knowledge about how
>>> fences are supposed to work). The IN/OUT_FENCE flags should definitely be
>>> reported though.
>>
>> I didn't know about this usecase. Thanks for explaining. It certainly
>> makes sense to set the IN/OUT_FENCE flags here. Regarding the fence_fd
>> I would expect the application to know the fence fd associated to than
>> buffer. If we expect an application other than the one which issued
>> QBUF than I'd say we also need to provide the fd on VIDIOC_QUERYBUF
>> for inspection purposes. Is that the case?
> 
> I just realized that if we want to also set the in-fence fd here we
> actually need to get a new unused fd, as either it is a different pid or
> the app already closed the fd it was using previously. Given this extra
> complication I'd say we shouldn't set fence fd unless someone has an
> usecase in mind.

Fair enough. Just make sure the fence_fd is some fixed value (-1?) in
that case.

Regards,

	Hans

Re: [PATCH 03/12] [media] vb2: add in-fence support to QBUF

[Gustavo Padovan]

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

> On 10/07/17 22:26, Gustavo Padovan wrote:
> > 2017-07-10 Gustavo Padovan <gustavo@padovan.org>:
> > 
> >> 2017-07-07 Hans Verkuil <hverkuil@xs4all.nl>:
> >>
> >>> On 07/07/2017 04:00 AM, Gustavo Padovan wrote:
> >>>> 2017-07-06 Hans Verkuil <hverkuil@xs4all.nl>:
> >>>>
> >>>>> On 06/16/17 09:39, 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.
> >>>>>>
> >>>>>> 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/Kconfig                    |  1 +
> >>>>>>   drivers/media/v4l2-core/videobuf2-core.c | 97 +++++++++++++++++++++++++-------
> >>>>>>   drivers/media/v4l2-core/videobuf2-v4l2.c | 15 ++++-
> >>>>>>   include/media/videobuf2-core.h           |  7 ++-
> >>>>>>   4 files changed, 99 insertions(+), 21 deletions(-)
> >>>>>>
> >>>>>
> >>>>> <snip>
> >>>>>
> >>>>>> diff --git a/drivers/media/v4l2-core/videobuf2-v4l2.c b/drivers/media/v4l2-core/videobuf2-v4l2.c
> >>>>>> index 110fb45..e6ad77f 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>
> >>>>>> @@ -560,6 +561,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 +570,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 ret ? ret : vb2_core_qbuf(q, b->index, b, fence);
> >>>>>>   }
> >>>>>>   EXPORT_SYMBOL_GPL(vb2_qbuf);
> >>>>>
> >>>>> You need to adapt __fill_v4l2_buffer so it sets the IN_FENCE buffer flag
> >>>>> if there is a fence pending. It should also fill in fence_fd.
> >>>>
> >>>> It was userspace who sent the fence_fd in the first place. Why do we
> >>>> need to send it back? The initial plan was - from a userspace point of view
> >>>> - to send the in-fence in the fence_fd field and receive the out-fence
> >>>>   in the same field.
> >>>>
> >>>> As per setting the IN_FENCE flag, that is racy, as the fence can signal
> >>>> just after we called __fill_v4l2_buffer. Why is it important to set it?
> >>>
> >>> Because when running VIDIOC_QUERYBUF it should return the current state of
> >>> the buffer, including whether it has a fence. You can do something like
> >>> v4l2-ctl --list-buffers to see how many buffers are queued up and the state
> >>> of each buffer. Can be useful to e.g. figure out why a video capture seems
> >>> to stall. Knowing that all queued buffers are all waiting for a fence is
> >>> very useful information. Whether the fence_fd should also be set here or
> >>> only in dqbuf is something I don't know (not enough knowledge about how
> >>> fences are supposed to work). The IN/OUT_FENCE flags should definitely be
> >>> reported though.
> >>
> >> I didn't know about this usecase. Thanks for explaining. It certainly
> >> makes sense to set the IN/OUT_FENCE flags here. Regarding the fence_fd
> >> I would expect the application to know the fence fd associated to than
> >> buffer. If we expect an application other than the one which issued
> >> QBUF than I'd say we also need to provide the fd on VIDIOC_QUERYBUF
> >> for inspection purposes. Is that the case?
> > 
> > I just realized that if we want to also set the in-fence fd here we
> > actually need to get a new unused fd, as either it is a different pid or
> > the app already closed the fd it was using previously. Given this extra
> > complication I'd say we shouldn't set fence fd unless someone has an
> > usecase in mind.
> 
> Fair enough. Just make sure the fence_fd is some fixed value (-1?) in
> that case.

Sure. -1 is the default value for these cases.

	Gustavo

Re: [PATCH 05/12] [media] vivid: assign the specific device to the vb2_queue->dev

[Gustavo Padovan]

2017-07-07 Shuah Khan <shuahkh@osg.samsung.com>:

> On 06/16/2017 01:39 AM, Gustavo Padovan wrote:
> > 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.
> > 
> > Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.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 ef344b9..8843170 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;
> 
> Does this work in all cases? My concern is that in some code paths
> q->dev might be used to initiate release perhaps.
> 
> Fore example v4l2_dev.release is vivid_dev_release()
> dev->v4l2_dev.release = vivid_dev_release;
> 
> vid_cap_dev release is video_device_release_empty
> 
> This is one difference, but there might be others and the code paths
> that might depend on q->dev being the v4l2_dev.dev which is the global
> dev.

The release call comes from the v4l2-core as we pass the v4l2 device on
v4l2_register_device(). q->dev is in just one ocasion and setting it to
a different device doesn't change the behavior. That code just check if
it is queued or not.

Gustavo

Re: [PATCH 04/12] [media] uvc: enable subscriptions to other events

[Gustavo Padovan]

2017-07-10 Gustavo Padovan <gustavo@padovan.org>:

> 2017-07-07 Shuah Khan <shuahkh@osg.samsung.com>:
> 
> > On 06/16/2017 01:39 AM, Gustavo Padovan wrote:
> > > From: Gustavo Padovan <gustavo.padovan@collabora.com>
> > > 
> > > Call v4l2_ctrl_subscribe_event to subscribe to more events supported by
> > > v4l.
> > > 
> > > 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 3e7e283..dfa0ccd 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);
> > 
> > This looks incorrect. With this change driver will be subscribing to all
> > v4l2 events? Is this the intent?
> 
> The intent was to enable this driver to subscribe to BUF_QUEUED events. 
> It is the only one who can't at the moment. I'll review this.

This change do not enable all events, the only new event added is the
BUF_QUEUED. v4l2_ctrl_subscribe_event() only add V4L2_EVENT_CTRL and
V4L2_EVENT_BUF_QUEUED, but the V4L2_EVENT_CTRL case can't be accessed
tere is this situation.

	Gustavo