From: Gustavo Padovan <[email protected]>
Hi,
Refer to the documentation on the first patch for the details. The previous
iteration is here: https://www.mail-archive.com/[email protected]/msg118077.html
The 2nd patch proposes an userspace API for fences, then on patch 3 we
prepare to the addition of in-fences in patch 4, by introducing the
infrastructure on vb2 to wait on an in-fence signal before queueing the
buffer in the driver.
Patch 5 fix uvc v4l2 event handling and patch 6 configure q->dev for
vivid drivers to enable to subscribe and dequeue events on it.
Patches 7-9 enables support to notify BUF_QUEUED events, the event send
to userspace the out-fence fd and the index of the buffer that was queued.
Patches 10-11 add support to mark queues as ordered. Finally patches 12
and 13 add more fence infrastructure to support out-fences, patch 13 exposes
close_fd() and patch 14 adds support to out-fences.
It only works for ordered queues for now, see open question at the end
of the letter.
Test tool can be found at:
https://git.collabora.com/cgit/user/padovan/v4l2-test.git/
Main Changes
------------
* out-fences: change in behavior: the out-fence fd now comes out of the
BUF_QUEUED event along with the buffer id.
All other changes are recorded on the patches' commit messages.
Open Questions
--------------
* non-ordered devices, like m2m: I've been thinking a lot about those
and one possibility is to have a way to tell userspace that the queue
is not ordered and then associate the fence with the current buffer in
QBUF instead of the next one to be queued. Of course, there won't be
any ordering between the fences. But it may be enough for userspace to
take advantage of Explicit Synchronization in such cases. Any
thoughts?
* OUTPUT devices and in-fence. If I understood OUTPUT devices correctly
it is desirable to queue the buffers to the driver in the same order
we received them from userspace. If that is correct, shouldn't we add
some mechanism to prevent buffer whose fence signaled to jump ahead of
others?
Gustavo Padovan (14):
[media] v4l: Document explicit synchronization behaviour
[media] vb2: add explicit fence user API
[media] vb2: check earlier if stream can be started
[media] vb2: add in-fence support to QBUF
[media] uvc: enable subscriptions to other events
[media] vivid: assign the specific device to the vb2_queue->dev
[media] v4l: add V4L2_EVENT_BUF_QUEUED event
[media] vb2: add .buffer_queued() to notify queueing in the driver
[media] v4l: add support to BUF_QUEUED event
[media] vb2: add 'ordered' property to queues
[media] vivid: mark vivid queues as ordered
[media] vb2: add infrastructure to support out-fences
fs/files: export close_fd() symbol
[media] vb2: add out-fence support to QBUF
Javier Martinez Canillas (1):
[media] vb2: add videobuf2 dma-buf fence helpers
Documentation/media/uapi/v4l/buffer.rst | 19 ++
Documentation/media/uapi/v4l/vidioc-dqevent.rst | 23 +++
Documentation/media/uapi/v4l/vidioc-qbuf.rst | 31 ++++
Documentation/media/videodev2.h.rst.exceptions | 1 +
drivers/android/binder.c | 2 +-
drivers/media/platform/vivid/vivid-core.c | 15 +-
drivers/media/usb/cpia2/cpia2_v4l.c | 2 +-
drivers/media/usb/uvc/uvc_v4l2.c | 2 +-
drivers/media/v4l2-core/Kconfig | 1 +
drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 4 +-
drivers/media/v4l2-core/v4l2-ctrls.c | 6 +-
drivers/media/v4l2-core/videobuf2-core.c | 221 ++++++++++++++++++++++--
drivers/media/v4l2-core/videobuf2-v4l2.c | 63 ++++++-
fs/file.c | 5 +-
fs/open.c | 2 +-
include/linux/fdtable.h | 2 +-
include/media/videobuf2-core.h | 63 ++++++-
include/media/videobuf2-fence.h | 49 ++++++
include/uapi/linux/videodev2.h | 15 +-
19 files changed, 489 insertions(+), 37 deletions(-)
create mode 100644 include/media/videobuf2-fence.h
--
2.13.5
From: Gustavo Padovan <[email protected]>
Add section to VIDIOC_QBUF about it
v2:
- mention that fences are files (Hans)
- rework for the new API
Signed-off-by: Gustavo Padovan <[email protected]>
---
Documentation/media/uapi/v4l/vidioc-qbuf.rst | 31 ++++++++++++++++++++++++++++
1 file changed, 31 insertions(+)
diff --git a/Documentation/media/uapi/v4l/vidioc-qbuf.rst b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
index 1f3612637200..fae0b1431672 100644
--- a/Documentation/media/uapi/v4l/vidioc-qbuf.rst
+++ b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
@@ -117,6 +117,37 @@ immediately with an ``EAGAIN`` error code when no buffer is available.
The struct :c:type:`v4l2_buffer` structure is specified in
:ref:`buffer`.
+Explicit Synchronization
+------------------------
+
+Explicit Synchronization allows us to control the synchronization of
+shared buffers from userspace by passing fences to the kernel and/or
+receiving them from it. Fences passed to the kernel are named in-fences and
+the kernel should wait them to signal before using the buffer, i.e., queueing
+it to the driver. On the other side, the kernel can create out-fences for the
+buffers it queues to the drivers, out-fences signal when the driver is
+finished with buffer, that is the buffer is ready. The fence are represented
+by file and passed as file descriptor to userspace.
+
+The in-fences are communicated to the kernel at the ``VIDIOC_QBUF`` ioctl
+using the ``V4L2_BUF_FLAG_IN_FENCE`` buffer
+flags and the `fence_fd` field. If an in-fence needs to be passed to the kernel,
+`fence_fd` should be set to the fence file descriptor number and the
+``V4L2_BUF_FLAG_IN_FENCE`` should be set as well. Failure to set both will
+cause ``VIDIOC_QBUF`` to return with error.
+
+To get a out-fence back from V4L2 the ``V4L2_BUF_FLAG_OUT_FENCE`` flag should
+be set to notify it that the next queued buffer should have a fence attached to
+it. That means the out-fence may not be associated with the buffer in the
+current ``VIDIOC_QBUF`` ioctl call because the ordering in which videobuf2 core
+queues the buffers to the drivers can't be guaranteed. To become aware of the
+of the next queued buffer and the out-fence attached to it the
+``V4L2_EVENT_BUF_QUEUED`` event should be used. It will trigger an event
+for every buffer queued to the V4L2 driver.
+
+At streamoff the out-fences will either signal normally if the drivers wait
+for the operations on the buffers to finish or signal with error if the
+driver cancel the pending operations.
Return Value
============
--
2.13.5
From: Gustavo Padovan <[email protected]>
Receive in-fence from userspace and add support for waiting on them
before queueing the buffer to the driver. Buffers are only queued
to the driver once they are ready. A buffer is ready when its
in-fence signals.
v4:
- Add a comment about dma_fence_add_callback() not returning a
error (Hans)
- Call dma_fence_put(vb->in_fence) if fence signaled (Hans)
- select SYNC_FILE under config VIDEOBUF2_CORE (Hans)
- Move dma_fence_is_signaled() check to __enqueue_in_driver() (Hans)
- Remove list_for_each_entry() in __vb2_core_qbuf() (Hans)
- Remove if (vb->state != VB2_BUF_STATE_QUEUED) from
vb2_start_streaming() (Hans)
- set IN_FENCE flags on __fill_v4l2_buffer (Hans)
- Queue buffers to the driver as soon as they are ready (Hans)
- call fill_user_buffer() after queuing the buffer (Hans)
- add err: label to clean up fence
- add dma_fence_wait() before calling vb2_start_streaming()
v3: - document fence parameter
- remove ternary if at vb2_qbuf() return (Mauro)
- do not change if conditions behaviour (Mauro)
v2:
- fix vb2_queue_or_prepare_buf() ret check
- remove check for VB2_MEMORY_DMABUF only (Javier)
- check num of ready buffers to start streaming
- when queueing, start from the first ready buffer
- handle queue cancel
Signed-off-by: Gustavo Padovan <[email protected]>
---
drivers/media/v4l2-core/Kconfig | 1 +
drivers/media/v4l2-core/videobuf2-core.c | 103 +++++++++++++++++++++++++++----
drivers/media/v4l2-core/videobuf2-v4l2.c | 27 +++++++-
include/media/videobuf2-core.h | 11 +++-
4 files changed, 127 insertions(+), 15 deletions(-)
diff --git a/drivers/media/v4l2-core/Kconfig b/drivers/media/v4l2-core/Kconfig
index a35c33686abf..3f988c407c80 100644
--- a/drivers/media/v4l2-core/Kconfig
+++ b/drivers/media/v4l2-core/Kconfig
@@ -83,6 +83,7 @@ config VIDEOBUF_DVB
# Used by drivers that need Videobuf2 modules
config VIDEOBUF2_CORE
select DMA_SHARED_BUFFER
+ select SYNC_FILE
tristate
config VIDEOBUF2_MEMOPS
diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 60f8b582396a..b19c1bc4b083 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -1222,6 +1222,9 @@ static void __enqueue_in_driver(struct vb2_buffer *vb)
{
struct vb2_queue *q = vb->vb2_queue;
+ if (vb->in_fence && !dma_fence_is_signaled(vb->in_fence))
+ return;
+
vb->state = VB2_BUF_STATE_ACTIVE;
atomic_inc(&q->owned_by_drv_count);
@@ -1273,6 +1276,20 @@ static int __buf_prepare(struct vb2_buffer *vb, const void *pb)
return 0;
}
+static int __get_num_ready_buffers(struct vb2_queue *q)
+{
+ struct vb2_buffer *vb;
+ int ready_count = 0;
+
+ /* count num of buffers ready in front of the queued_list */
+ list_for_each_entry(vb, &q->queued_list, queued_entry) {
+ if (!vb->in_fence || dma_fence_is_signaled(vb->in_fence))
+ ready_count++;
+ }
+
+ return ready_count;
+}
+
int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb)
{
struct vb2_buffer *vb;
@@ -1361,7 +1378,19 @@ static int vb2_start_streaming(struct vb2_queue *q)
return ret;
}
-int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
+static void vb2_qbuf_fence_cb(struct dma_fence *f, struct dma_fence_cb *cb)
+{
+ struct vb2_buffer *vb = container_of(cb, struct vb2_buffer, fence_cb);
+
+ dma_fence_put(vb->in_fence);
+ vb->in_fence = NULL;
+
+ if (vb->vb2_queue->start_streaming_called)
+ __enqueue_in_driver(vb);
+}
+
+int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb,
+ struct dma_fence *fence)
{
struct vb2_buffer *vb;
int ret;
@@ -1372,16 +1401,18 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
case VB2_BUF_STATE_DEQUEUED:
ret = __buf_prepare(vb, pb);
if (ret)
- return ret;
+ goto err;
break;
case VB2_BUF_STATE_PREPARED:
break;
case VB2_BUF_STATE_PREPARING:
dprintk(1, "buffer still being prepared\n");
- return -EINVAL;
+ ret = -EINVAL;
+ goto err;
default:
dprintk(1, "invalid buffer state %d\n", vb->state);
- return -EINVAL;
+ ret = -EINVAL;
+ goto err;
}
/*
@@ -1392,6 +1423,7 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
q->queued_count++;
q->waiting_for_buffers = false;
vb->state = VB2_BUF_STATE_QUEUED;
+ vb->in_fence = fence;
if (pb)
call_void_bufop(q, copy_timestamp, vb, pb);
@@ -1399,6 +1431,16 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
trace_vb2_qbuf(q, vb);
/*
+ * If it is time to call vb2_start_streaming() wait for the fence
+ * to signal first. Of course, this happens only once per streaming.
+ * We want to run any step that might fail before we set the callback
+ * to queue the fence when it signals.
+ */
+ if (fence && !q->start_streaming_called &&
+ __get_num_ready_buffers(q) == q->min_buffers_needed - 1)
+ dma_fence_wait(fence, true);
+
+ /*
* If streamon has been called, and we haven't yet called
* start_streaming() since not enough buffers were queued, and
* we now have reached the minimum number of queued buffers,
@@ -1408,20 +1450,48 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
* If not, the buffer will be given to driver on next streamon.
*/
if (q->streaming && !q->start_streaming_called &&
- q->queued_count >= q->min_buffers_needed) {
+ __get_num_ready_buffers(q) >= q->min_buffers_needed) {
ret = vb2_start_streaming(q);
if (ret)
- return ret;
- } else if (q->start_streaming_called) {
- __enqueue_in_driver(vb);
+ goto err;
}
+ /*
+ * For explicit synchronization: If the fence didn't signal
+ * yet we setup a callback to queue the buffer once the fence
+ * signals, and then, return successfully. But if the fence
+ * already signaled we lose the reference we held and queue the
+ * buffer to the driver.
+ */
+ if (fence) {
+ ret = dma_fence_add_callback(fence, &vb->fence_cb,
+ vb2_qbuf_fence_cb);
+ if (!ret)
+ goto fill;
+
+ dma_fence_put(fence);
+ vb->in_fence = NULL;
+ }
+
+fill:
+ if (q->start_streaming_called && !vb->in_fence)
+ __enqueue_in_driver(vb);
+
/* Fill buffer information for the userspace */
if (pb)
call_void_bufop(q, fill_user_buffer, vb, pb);
dprintk(2, "qbuf of buffer %d succeeded\n", vb->index);
return 0;
+
+err:
+ if (vb->in_fence) {
+ dma_fence_put(vb->in_fence);
+ vb->in_fence = NULL;
+ }
+
+ return ret;
+
}
EXPORT_SYMBOL_GPL(vb2_core_qbuf);
@@ -1632,6 +1702,7 @@ EXPORT_SYMBOL_GPL(vb2_core_dqbuf);
static void __vb2_queue_cancel(struct vb2_queue *q)
{
unsigned int i;
+ struct vb2_buffer *vb;
/*
* Tell driver to stop all transactions and release all queued
@@ -1654,6 +1725,14 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
WARN_ON(atomic_read(&q->owned_by_drv_count));
}
+ list_for_each_entry(vb, &q->queued_list, queued_entry) {
+ if (vb->in_fence) {
+ dma_fence_remove_callback(vb->in_fence, &vb->fence_cb);
+ dma_fence_put(vb->in_fence);
+ vb->in_fence = NULL;
+ }
+ }
+
q->streaming = 0;
q->start_streaming_called = 0;
q->queued_count = 0;
@@ -1720,7 +1799,7 @@ int vb2_core_streamon(struct vb2_queue *q, unsigned int type)
* Tell driver to start streaming provided sufficient buffers
* are available.
*/
- if (q->queued_count >= q->min_buffers_needed) {
+ if (__get_num_ready_buffers(q) >= q->min_buffers_needed) {
ret = v4l_vb2q_enable_media_source(q);
if (ret)
return ret;
@@ -2240,7 +2319,7 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read)
* Queue all buffers.
*/
for (i = 0; i < q->num_buffers; i++) {
- ret = vb2_core_qbuf(q, i, NULL);
+ ret = vb2_core_qbuf(q, i, NULL, NULL);
if (ret)
goto err_reqbufs;
fileio->bufs[i].queued = 1;
@@ -2419,7 +2498,7 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_
if (copy_timestamp)
b->timestamp = ktime_get_ns();
- ret = vb2_core_qbuf(q, index, NULL);
+ ret = vb2_core_qbuf(q, index, NULL, NULL);
dprintk(5, "vb2_dbuf result: %d\n", ret);
if (ret)
return ret;
@@ -2522,7 +2601,7 @@ static int vb2_thread(void *data)
if (copy_timestamp)
vb->timestamp = ktime_get_ns();;
if (!threadio->stop)
- ret = vb2_core_qbuf(q, vb->index, NULL);
+ ret = vb2_core_qbuf(q, vb->index, NULL, NULL);
call_void_qop(q, wait_prepare, q);
if (ret || threadio->stop)
break;
diff --git a/drivers/media/v4l2-core/videobuf2-v4l2.c b/drivers/media/v4l2-core/videobuf2-v4l2.c
index 110fb45fef6f..8c322cd1b346 100644
--- a/drivers/media/v4l2-core/videobuf2-v4l2.c
+++ b/drivers/media/v4l2-core/videobuf2-v4l2.c
@@ -23,6 +23,7 @@
#include <linux/sched.h>
#include <linux/freezer.h>
#include <linux/kthread.h>
+#include <linux/sync_file.h>
#include <media/v4l2-dev.h>
#include <media/v4l2-fh.h>
@@ -178,6 +179,11 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b,
return -EINVAL;
}
+ if ((b->fence_fd != -1) && !(b->flags & V4L2_BUF_FLAG_IN_FENCE)) {
+ dprintk(1, "%s: fence_fd set without IN_FENCE flag\n", opname);
+ return -EINVAL;
+ }
+
return __verify_planes_array(q->bufs[b->index], b);
}
@@ -203,9 +209,14 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, void *pb)
b->timestamp = ns_to_timeval(vb->timestamp);
b->timecode = vbuf->timecode;
b->sequence = vbuf->sequence;
- b->fence_fd = -1;
b->reserved = 0;
+ b->fence_fd = -1;
+ if (vb->in_fence)
+ b->flags |= V4L2_BUF_FLAG_IN_FENCE;
+ else
+ b->flags &= ~V4L2_BUF_FLAG_IN_FENCE;
+
if (q->is_multiplanar) {
/*
* Fill in plane-related data if userspace provided an array
@@ -560,6 +571,7 @@ EXPORT_SYMBOL_GPL(vb2_create_bufs);
int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
{
+ struct dma_fence *fence = NULL;
int ret;
if (vb2_fileio_is_active(q)) {
@@ -568,7 +580,18 @@ int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
}
ret = vb2_queue_or_prepare_buf(q, b, "qbuf");
- return ret ? ret : vb2_core_qbuf(q, b->index, b);
+ if (ret)
+ return ret;
+
+ if (b->flags & V4L2_BUF_FLAG_IN_FENCE) {
+ fence = sync_file_get_fence(b->fence_fd);
+ if (!fence) {
+ dprintk(1, "failed to get in-fence from fd\n");
+ return -EINVAL;
+ }
+ }
+
+ return vb2_core_qbuf(q, b->index, b, fence);
}
EXPORT_SYMBOL_GPL(vb2_qbuf);
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index ef9b64398c8c..41cda762ff0a 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -16,6 +16,7 @@
#include <linux/mutex.h>
#include <linux/poll.h>
#include <linux/dma-buf.h>
+#include <linux/dma-fence.h>
#define VB2_MAX_FRAME (32)
#define VB2_MAX_PLANES (8)
@@ -254,11 +255,17 @@ struct vb2_buffer {
* all buffers queued from userspace
* done_entry: entry on the list that stores all buffers ready
* to be dequeued to userspace
+ * in_fence: fence receive from vb2 client to wait on before
+ * using the buffer (queueing to the driver)
+ * fence_cb: fence callback information
*/
enum vb2_buffer_state state;
struct list_head queued_entry;
struct list_head done_entry;
+
+ struct dma_fence *in_fence;
+ struct dma_fence_cb fence_cb;
#ifdef CONFIG_VIDEO_ADV_DEBUG
/*
* Counters for how often these buffer-related ops are
@@ -726,6 +733,7 @@ int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb);
* @index: id number of the buffer
* @pb: buffer structure passed from userspace to vidioc_qbuf handler
* in driver
+ * @fence: in-fence to wait on before queueing the buffer
*
* Should be called from vidioc_qbuf ioctl handler of a driver.
* The passed buffer should have been verified.
@@ -740,7 +748,8 @@ int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb);
* The return values from this function are intended to be directly returned
* from vidioc_qbuf handler in driver.
*/
-int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb);
+int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb,
+ struct dma_fence *fence);
/**
* vb2_core_dqbuf() - Dequeue a buffer to the userspace
--
2.13.5
From: Gustavo Padovan <[email protected]>
Instead of assigning the global v4l2 device, assign the specific device.
This was causing trouble when using using V4L2 events with vivid
devices. The device's queue should be the same we opened in userspace.
This is needed for the upcoming V4L2_EVENT_BUF_QUEUED support. The current
vivid code isn't wrong, it just needs to be changed so V4L2_EVENT_BUF_QUEUED
can be supported. The change doesn't affect any other behaviour of vivid.
Signed-off-by: Gustavo Padovan <[email protected]>
Acked-by: Hans Verkuil <[email protected]>
---
drivers/media/platform/vivid/vivid-core.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/media/platform/vivid/vivid-core.c b/drivers/media/platform/vivid/vivid-core.c
index 5f316a5e38db..608bcceed463 100644
--- a/drivers/media/platform/vivid/vivid-core.c
+++ b/drivers/media/platform/vivid/vivid-core.c
@@ -1070,7 +1070,7 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
q->min_buffers_needed = 2;
q->lock = &dev->mutex;
- q->dev = dev->v4l2_dev.dev;
+ q->dev = &dev->vid_cap_dev.dev;
ret = vb2_queue_init(q);
if (ret)
@@ -1090,7 +1090,7 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
q->min_buffers_needed = 2;
q->lock = &dev->mutex;
- q->dev = dev->v4l2_dev.dev;
+ q->dev = &dev->vid_out_dev.dev;
ret = vb2_queue_init(q);
if (ret)
@@ -1110,7 +1110,7 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
q->min_buffers_needed = 2;
q->lock = &dev->mutex;
- q->dev = dev->v4l2_dev.dev;
+ q->dev = &dev->vbi_cap_dev.dev;
ret = vb2_queue_init(q);
if (ret)
@@ -1130,7 +1130,7 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
q->min_buffers_needed = 2;
q->lock = &dev->mutex;
- q->dev = dev->v4l2_dev.dev;
+ q->dev = &dev->vbi_out_dev.dev;
ret = vb2_queue_init(q);
if (ret)
@@ -1149,7 +1149,7 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
q->min_buffers_needed = 8;
q->lock = &dev->mutex;
- q->dev = dev->v4l2_dev.dev;
+ q->dev = &dev->sdr_cap_dev.dev;
ret = vb2_queue_init(q);
if (ret)
--
2.13.5
From: Gustavo Padovan <[email protected]>
With the upcoming explicit synchronization support to V4L2 we need a
way to notify userspace when buffers are queued to the driver - buffers
with fences attached to it can only be queued once the fence signal, so
the queueing to the driver might be deferred.
Yet, userspace still wants to be notified, so the buffer_queued() callback
was added to vb2_buf_ops for that purpose.
Signed-off-by: Gustavo Padovan <[email protected]>
---
include/media/videobuf2-core.h | 3 +++
1 file changed, 3 insertions(+)
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index 41cda762ff0a..5ed8d3402474 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -415,6 +415,8 @@ struct vb2_ops {
* will return an error.
* @copy_timestamp: copy the timestamp from a userspace structure to
* the vb2_buffer struct.
+ * @buffer_queued: VB2 uses this to notify the VB2-client that the buffer
+ * was queued to the driver.
*/
struct vb2_buf_ops {
int (*verify_planes_array)(struct vb2_buffer *vb, const void *pb);
@@ -422,6 +424,7 @@ struct vb2_buf_ops {
int (*fill_vb2_buffer)(struct vb2_buffer *vb, const void *pb,
struct vb2_plane *planes);
void (*copy_timestamp)(struct vb2_buffer *vb, const void *pb);
+ void (*buffer_queued)(struct vb2_buffer *vb);
};
/**
--
2.13.5
From: Gustavo Padovan <[email protected]>
Turn the reserved2 field into fence_fd that we will use to send
an in-fence to the kernel and return an out-fence from the kernel to
userspace.
Two new flags were added, V4L2_BUF_FLAG_IN_FENCE, that should be used
when sending a fence to the kernel to be waited on, and
V4L2_BUF_FLAG_OUT_FENCE, to ask the kernel to give back an out-fence.
v2: add documentation
Signed-off-by: Gustavo Padovan <[email protected]>
---
Documentation/media/uapi/v4l/buffer.rst | 19 +++++++++++++++++++
drivers/media/usb/cpia2/cpia2_v4l.c | 2 +-
drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 4 ++--
drivers/media/v4l2-core/videobuf2-v4l2.c | 2 +-
include/uapi/linux/videodev2.h | 4 +++-
5 files changed, 26 insertions(+), 5 deletions(-)
diff --git a/Documentation/media/uapi/v4l/buffer.rst b/Documentation/media/uapi/v4l/buffer.rst
index ae6ee73f151c..664507ad06c6 100644
--- a/Documentation/media/uapi/v4l/buffer.rst
+++ b/Documentation/media/uapi/v4l/buffer.rst
@@ -648,6 +648,25 @@ Buffer Flags
- Start Of Exposure. The buffer timestamp has been taken when the
exposure of the frame has begun. This is only valid for the
``V4L2_BUF_TYPE_VIDEO_CAPTURE`` buffer type.
+ * .. _`V4L2-BUF-FLAG-IN-FENCE`:
+
+ - ``V4L2_BUF_FLAG_IN_FENCE``
+ - 0x00200000
+ - Ask V4L2 to wait on fence passed in ``fence_fd`` field. The buffer
+ won't be queued to the driver until the fence signals.
+
+ * .. _`V4L2-BUF-FLAG-OUT-FENCE`:
+
+ - ``V4L2_BUF_FLAG_OUT_FENCE``
+ - 0x00400000
+ - Request a fence for the next buffer to be queued to V4L2 driver.
+ The fence received back through the ``fence_fd`` field doesn't
+ necessarily relate to the current buffer in the
+ :ref:`VIDIOC_QBUF <VIDIOC_QBUF>` ioctl. Although, most of the time
+ the fence will relate to the current buffer it can't be guaranteed.
+ So to tell userspace which buffer is associated to the out_fence,
+ one should listen for the ``V4L2_EVENT_BUF_QUEUED`` event that
+ provide the id of the buffer when it is queued to the V4L2 driver.
diff --git a/drivers/media/usb/cpia2/cpia2_v4l.c b/drivers/media/usb/cpia2/cpia2_v4l.c
index 3dedd83f0b19..6cde686bf44c 100644
--- a/drivers/media/usb/cpia2/cpia2_v4l.c
+++ b/drivers/media/usb/cpia2/cpia2_v4l.c
@@ -948,7 +948,7 @@ static int cpia2_dqbuf(struct file *file, void *fh, struct v4l2_buffer *buf)
buf->sequence = cam->buffers[buf->index].seq;
buf->m.offset = cam->buffers[buf->index].data - cam->frame_buffer;
buf->length = cam->frame_size;
- buf->reserved2 = 0;
+ buf->fence_fd = -1;
buf->reserved = 0;
memset(&buf->timecode, 0, sizeof(buf->timecode));
diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
index 821f2aa299ae..d624fb5df130 100644
--- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
+++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
@@ -370,7 +370,7 @@ struct v4l2_buffer32 {
__s32 fd;
} m;
__u32 length;
- __u32 reserved2;
+ __s32 fence_fd;
__u32 reserved;
};
@@ -533,8 +533,8 @@ static int put_v4l2_buffer32(struct v4l2_buffer *kp, struct v4l2_buffer32 __user
put_user(kp->timestamp.tv_usec, &up->timestamp.tv_usec) ||
copy_to_user(&up->timecode, &kp->timecode, sizeof(struct v4l2_timecode)) ||
put_user(kp->sequence, &up->sequence) ||
- put_user(kp->reserved2, &up->reserved2) ||
put_user(kp->reserved, &up->reserved) ||
+ put_user(kp->fence_fd, &up->fence_fd) ||
put_user(kp->length, &up->length))
return -EFAULT;
diff --git a/drivers/media/v4l2-core/videobuf2-v4l2.c b/drivers/media/v4l2-core/videobuf2-v4l2.c
index 0c0669976bdc..110fb45fef6f 100644
--- a/drivers/media/v4l2-core/videobuf2-v4l2.c
+++ b/drivers/media/v4l2-core/videobuf2-v4l2.c
@@ -203,7 +203,7 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, void *pb)
b->timestamp = ns_to_timeval(vb->timestamp);
b->timecode = vbuf->timecode;
b->sequence = vbuf->sequence;
- b->reserved2 = 0;
+ b->fence_fd = -1;
b->reserved = 0;
if (q->is_multiplanar) {
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 185d6a0acc06..e5abab9a908c 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -924,7 +924,7 @@ struct v4l2_buffer {
__s32 fd;
} m;
__u32 length;
- __u32 reserved2;
+ __s32 fence_fd;
__u32 reserved;
};
@@ -961,6 +961,8 @@ struct v4l2_buffer {
#define V4L2_BUF_FLAG_TSTAMP_SRC_SOE 0x00010000
/* mem2mem encoder/decoder */
#define V4L2_BUF_FLAG_LAST 0x00100000
+#define V4L2_BUF_FLAG_IN_FENCE 0x00200000
+#define V4L2_BUF_FLAG_OUT_FENCE 0x00400000
/**
* struct v4l2_exportbuffer - export of video buffer as DMABUF file descriptor
--
2.13.5
From: Gustavo Padovan <[email protected]>
To enable vivid to be used with explicit synchronization we need
to mark its queues as ordered. vivid queues are already ordered by
default so we no changes are needed.
Signed-off-by: Gustavo Padovan <[email protected]>
Acked-by: Hans Verkuil <[email protected]>
---
drivers/media/platform/vivid/vivid-core.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/media/platform/vivid/vivid-core.c b/drivers/media/platform/vivid/vivid-core.c
index 608bcceed463..239790e8ccc6 100644
--- a/drivers/media/platform/vivid/vivid-core.c
+++ b/drivers/media/platform/vivid/vivid-core.c
@@ -1063,6 +1063,7 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
q->type = dev->multiplanar ? V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE :
V4L2_BUF_TYPE_VIDEO_CAPTURE;
q->io_modes = VB2_MMAP | VB2_USERPTR | VB2_DMABUF | VB2_READ;
+ q->ordered = 1;
q->drv_priv = dev;
q->buf_struct_size = sizeof(struct vivid_buffer);
q->ops = &vivid_vid_cap_qops;
@@ -1083,6 +1084,7 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
q->type = dev->multiplanar ? V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE :
V4L2_BUF_TYPE_VIDEO_OUTPUT;
q->io_modes = VB2_MMAP | VB2_USERPTR | VB2_DMABUF | VB2_WRITE;
+ q->ordered = 1;
q->drv_priv = dev;
q->buf_struct_size = sizeof(struct vivid_buffer);
q->ops = &vivid_vid_out_qops;
@@ -1103,6 +1105,7 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
q->type = dev->has_raw_vbi_cap ? V4L2_BUF_TYPE_VBI_CAPTURE :
V4L2_BUF_TYPE_SLICED_VBI_CAPTURE;
q->io_modes = VB2_MMAP | VB2_USERPTR | VB2_DMABUF | VB2_READ;
+ q->ordered = 1;
q->drv_priv = dev;
q->buf_struct_size = sizeof(struct vivid_buffer);
q->ops = &vivid_vbi_cap_qops;
@@ -1123,6 +1126,7 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
q->type = dev->has_raw_vbi_out ? V4L2_BUF_TYPE_VBI_OUTPUT :
V4L2_BUF_TYPE_SLICED_VBI_OUTPUT;
q->io_modes = VB2_MMAP | VB2_USERPTR | VB2_DMABUF | VB2_WRITE;
+ q->ordered = 1;
q->drv_priv = dev;
q->buf_struct_size = sizeof(struct vivid_buffer);
q->ops = &vivid_vbi_out_qops;
@@ -1142,6 +1146,7 @@ static int vivid_create_instance(struct platform_device *pdev, int inst)
q = &dev->vb_sdr_cap_q;
q->type = V4L2_BUF_TYPE_SDR_CAPTURE;
q->io_modes = VB2_MMAP | VB2_USERPTR | VB2_DMABUF | VB2_READ;
+ q->ordered = 1;
q->drv_priv = dev;
q->buf_struct_size = sizeof(struct vivid_buffer);
q->ops = &vivid_sdr_cap_qops;
--
2.13.5
From: Javier Martinez Canillas <[email protected]>
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 <[email protected]>
Signed-off-by: Gustavo Padovan <[email protected]>
---
include/media/videobuf2-fence.h | 49 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 49 insertions(+)
create mode 100644 include/media/videobuf2-fence.h
diff --git a/include/media/videobuf2-fence.h b/include/media/videobuf2-fence.h
new file mode 100644
index 000000000000..ed5612ca03d6
--- /dev/null
+++ b/include/media/videobuf2-fence.h
@@ -0,0 +1,49 @@
+/*
+ * videobuf2-fence.h - DMA buffer sharing fence helpers for videobuf 2
+ *
+ * Copyright (C) 2016 Samsung Electronics
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation.
+ */
+
+#include <linux/dma-fence.h>
+#include <linux/slab.h>
+
+static DEFINE_SPINLOCK(vb2_fence_lock);
+
+static inline const char *vb2_fence_get_driver_name(struct dma_fence *fence)
+{
+ return "vb2_fence";
+}
+
+static inline const char *vb2_fence_get_timeline_name(struct dma_fence *fence)
+{
+ return "vb2_fence_timeline";
+}
+
+static inline bool vb2_fence_enable_signaling(struct dma_fence *fence)
+{
+ return true;
+}
+
+static const struct dma_fence_ops vb2_fence_ops = {
+ .get_driver_name = vb2_fence_get_driver_name,
+ .get_timeline_name = vb2_fence_get_timeline_name,
+ .enable_signaling = vb2_fence_enable_signaling,
+ .wait = dma_fence_default_wait,
+};
+
+static inline struct dma_fence *vb2_fence_alloc(void)
+{
+ struct dma_fence *vb2_fence = kzalloc(sizeof(*vb2_fence), GFP_KERNEL);
+
+ if (!vb2_fence)
+ return NULL;
+
+ dma_fence_init(vb2_fence, &vb2_fence_ops, &vb2_fence_lock,
+ dma_fence_context_alloc(1), 1);
+
+ return vb2_fence;
+}
--
2.13.5
From: Gustavo Padovan <[email protected]>
Add vb2_setup_out_fence() and the needed members to struct vb2_buffer.
Signed-off-by: Gustavo Padovan <[email protected]>
---
drivers/media/v4l2-core/videobuf2-core.c | 55 ++++++++++++++++++++++++++++++++
include/media/videobuf2-core.h | 34 ++++++++++++++++++++
2 files changed, 89 insertions(+)
diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index bbbae0eed567..34adf1916194 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -23,8 +23,11 @@
#include <linux/sched.h>
#include <linux/freezer.h>
#include <linux/kthread.h>
+#include <linux/sync_file.h>
+#include <linux/dma-fence.h>
#include <media/videobuf2-core.h>
+#include <media/videobuf2-fence.h>
#include <media/v4l2-mc.h>
#include <trace/events/vb2.h>
@@ -1317,6 +1320,58 @@ int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb)
}
EXPORT_SYMBOL_GPL(vb2_core_prepare_buf);
+int vb2_setup_out_fence(struct vb2_queue *q)
+{
+ struct vb2_fence *fence;
+
+ fence = kzalloc(sizeof(*fence), GFP_KERNEL);
+ if (!fence)
+ return -ENOMEM;
+
+ fence->out_fence_fd = get_unused_fd_flags(O_CLOEXEC);
+ if (fence->out_fence_fd < 0) {
+ kfree(fence);
+ return fence->out_fence_fd;
+ }
+
+ fence->out_fence = vb2_fence_alloc();
+ if (!fence->out_fence)
+ goto err_fence;
+
+ fence->sync_file = sync_file_create(fence->out_fence);
+ if (!fence->sync_file) {
+ dma_fence_put(fence->out_fence);
+ goto err_fence;
+ }
+
+ spin_lock(&q->out_fence_lock);
+ list_add_tail(&fence->entry, &q->out_fence_list);
+ spin_unlock(&q->out_fence_lock);
+
+ return 0;
+
+err_fence:
+ kfree(fence);
+ put_unused_fd(fence->out_fence_fd);
+ return -ENOMEM;
+}
+EXPORT_SYMBOL_GPL(vb2_setup_out_fence);
+
+void vb2_cleanup_out_fence(struct vb2_queue *q)
+{
+ struct vb2_fence *fence;
+
+ spin_lock(&q->out_fence_lock);
+ fence = list_last_entry(&q->out_fence_list,
+ struct vb2_fence, entry);
+ put_unused_fd(fence->out_fence_fd);
+ fput(fence->sync_file->file);
+ list_del(&fence->entry);
+ spin_unlock(&q->out_fence_lock);
+ kfree(fence);
+}
+EXPORT_SYMBOL_GPL(vb2_cleanup_out_fence);
+
/**
* vb2_start_streaming() - Attempt to start streaming.
* @q: videobuf2 queue
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index 20099dc22f26..84e5e7216a1e 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -427,6 +427,24 @@ struct vb2_buf_ops {
void (*buffer_queued)(struct vb2_buffer *vb);
};
+/*
+ * struct vb2_fence - storage for fence data before queueing to the driver.
+ *
+ * @out_fence_fd: the fd where to install the sync_file
+ * @out_fence: the fence associated to the sync_file
+ * @sync_file: the sync_file to be shared with userspace via the
+ * out_fence_fd
+ * @files: stores files struct for cleanup purposes
+ * @entry: the list head element for the out_fence_list
+ */
+struct vb2_fence {
+ int out_fence_fd;
+ struct dma_fence *out_fence;
+ struct sync_file *sync_file;
+ struct files_struct *files;
+ struct list_head entry;
+};
+
/**
* struct vb2_queue - a videobuf queue
*
@@ -734,6 +752,22 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb);
/**
+ * vb2_setup_out_fence() - setup new out-fence
+ * @q: The vb2_queue where to setup it
+ *
+ * Setup the file descriptor, the fence and the sync_file for the next
+ * buffer to be queued and add everything to the tail of the q->out_fence_list.
+ */
+int vb2_setup_out_fence(struct vb2_queue *q);
+
+/**
+ * vb2_cleanup_out_fence() - cleanup out-fence
+ * @q: The vb2_queue to use for cleanup
+ *
+ * Clean up the last fence on the list. Used only when QBUF fails.
+ */
+void vb2_cleanup_out_fence(struct vb2_queue *q);
+/**
* vb2_core_qbuf() - Queue a buffer from userspace
*
* @q: videobuf2 queue
--
2.13.5
From: Gustavo Padovan <[email protected]>
If V4L2_BUF_FLAG_OUT_FENCE flag is present on the QBUF call we create
an out_fence and sent to userspace on the V4L2_EVENT_BUF_QUEUED when
the buffer is queued to the driver.
The out fence fd returned references the next buffer to be queued to the
driver and not the buffer in the actual QBUF call. So there a list in
videobuf2 core to keep track of the sync_file and fence created and assign
them to buffers when they are queued to the V4L2 driver.
The fence is signaled on buffer_done(), when the job on the buffer is
finished.
v4:
- return the out_fence_fd in the BUF_QUEUED event(Hans)
v3: - add WARN_ON_ONCE(q->ordered) on requeueing (Hans)
- set the OUT_FENCE flag if there is a fence pending (Hans)
- call fd_install() after vb2_core_qbuf() (Hans)
- clean up fence if vb2_core_qbuf() fails (Hans)
- add list to store sync_file and fence for the next queued buffer
v2: check if the queue is ordered.
Signed-off-by: Gustavo Padovan <[email protected]>
---
drivers/media/v4l2-core/videobuf2-core.c | 51 ++++++++++++++++++++++++++++++++
drivers/media/v4l2-core/videobuf2-v4l2.c | 24 ++++++++++++++-
include/media/videobuf2-core.h | 11 +++++++
3 files changed, 85 insertions(+), 1 deletion(-)
diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 34adf1916194..ab58170776bc 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -25,6 +25,7 @@
#include <linux/kthread.h>
#include <linux/sync_file.h>
#include <linux/dma-fence.h>
+#include <linux/fdtable.h>
#include <media/videobuf2-core.h>
#include <media/videobuf2-fence.h>
@@ -353,6 +354,7 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory,
vb->planes[plane].length = plane_sizes[plane];
vb->planes[plane].min_length = plane_sizes[plane];
}
+ vb->out_fence_fd = -1;
q->bufs[vb->index] = vb;
/* Allocate video buffer memory for the MMAP type */
@@ -933,10 +935,24 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state)
case VB2_BUF_STATE_QUEUED:
return;
case VB2_BUF_STATE_REQUEUEING:
+ /*
+ * Explicit synchronization requires ordered queues for now,
+ * so WARN_ON if we are requeuing on an ordered queue.
+ */
+ if (vb->out_fence)
+ WARN_ON_ONCE(q->ordered);
+
if (q->start_streaming_called)
__enqueue_in_driver(vb);
return;
default:
+ if (state == VB2_BUF_STATE_ERROR)
+ dma_fence_set_error(vb->out_fence, -ENOENT);
+ dma_fence_signal(vb->out_fence);
+ dma_fence_put(vb->out_fence);
+ vb->out_fence = NULL;
+ vb->out_fence_fd = -1;
+
/* Inform any processes that may be waiting for buffers */
wake_up(&q->done_wq);
break;
@@ -1224,10 +1240,21 @@ static int __prepare_dmabuf(struct vb2_buffer *vb, const void *pb)
static void __enqueue_in_driver(struct vb2_buffer *vb)
{
struct vb2_queue *q = vb->vb2_queue;
+ struct vb2_fence *fence;
if (vb->in_fence && !dma_fence_is_signaled(vb->in_fence))
return;
+ spin_lock(&q->out_fence_lock);
+ fence = list_first_entry(&q->out_fence_list, struct vb2_fence, entry);
+ if (fence) {
+ vb->out_fence = dma_fence_get(fence->out_fence);
+ vb->out_fence_fd = fence->out_fence_fd;
+ list_del(&fence->entry);
+ kfree(fence);
+ }
+ spin_unlock(&q->out_fence_lock);
+
vb->state = VB2_BUF_STATE_ACTIVE;
atomic_inc(&q->owned_by_drv_count);
@@ -1348,6 +1375,8 @@ int vb2_setup_out_fence(struct vb2_queue *q)
list_add_tail(&fence->entry, &q->out_fence_list);
spin_unlock(&q->out_fence_lock);
+ fence->files = current->files;
+
return 0;
err_fence:
@@ -1450,6 +1479,7 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb,
struct dma_fence *fence)
{
struct vb2_buffer *vb;
+ struct vb2_fence *vb2_fence;
int ret;
vb = q->bufs[index];
@@ -1513,6 +1543,15 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb,
goto err;
}
+ spin_lock(&q->out_fence_lock);
+ vb2_fence = list_last_entry(&q->out_fence_list, struct vb2_fence,
+ entry);
+ spin_unlock(&q->out_fence_lock);
+ if (vb2_fence)
+ fd_install(vb2_fence->out_fence_fd,
+ vb2_fence->sync_file->file);
+
+
/*
* For explicit synchronization: If the fence didn't signal
* yet we setup a callback to queue the buffer once the fence
@@ -1760,6 +1799,7 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
{
unsigned int i;
struct vb2_buffer *vb;
+ struct vb2_fence *fence, *tmp;
/*
* Tell driver to stop all transactions and release all queued
@@ -1790,6 +1830,14 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
}
}
+ spin_lock(&q->out_fence_lock);
+ list_for_each_entry_safe(fence, tmp, &q->out_fence_list, entry) {
+ close_fd(fence->files, fence->out_fence_fd);
+ list_del(&fence->entry);
+ kfree(fence);
+ }
+ spin_unlock(&q->out_fence_lock);
+
q->streaming = 0;
q->start_streaming_called = 0;
q->queued_count = 0;
@@ -1804,6 +1852,7 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
* has not already dequeued before initiating cancel.
*/
INIT_LIST_HEAD(&q->done_list);
+ INIT_LIST_HEAD(&q->out_fence_list);
atomic_set(&q->owned_by_drv_count, 0);
wake_up_all(&q->done_wq);
@@ -2125,6 +2174,8 @@ int vb2_core_queue_init(struct vb2_queue *q)
INIT_LIST_HEAD(&q->queued_list);
INIT_LIST_HEAD(&q->done_list);
spin_lock_init(&q->done_lock);
+ INIT_LIST_HEAD(&q->out_fence_list);
+ spin_lock_init(&q->out_fence_lock);
mutex_init(&q->mmap_lock);
init_waitqueue_head(&q->done_wq);
diff --git a/drivers/media/v4l2-core/videobuf2-v4l2.c b/drivers/media/v4l2-core/videobuf2-v4l2.c
index bbfcd054e6f6..e1ca48ab5005 100644
--- a/drivers/media/v4l2-core/videobuf2-v4l2.c
+++ b/drivers/media/v4l2-core/videobuf2-v4l2.c
@@ -147,6 +147,7 @@ static void __buffer_queued(struct vb2_buffer *vb)
memset(&event, 0, sizeof(event));
event.type = V4L2_EVENT_BUF_QUEUED;
event.u.buf_queued.index = vb->index;
+ event.u.buf_queued.out_fence_fd = vb->out_fence_fd;
v4l2_event_queue_fh(fh, &event);
}
@@ -197,6 +198,12 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b,
return -EINVAL;
}
+ if (!q->ordered && (b->flags & V4L2_BUF_FLAG_OUT_FENCE)) {
+ dprintk(1, "%s: out-fence doesn't work on unordered queues\n",
+ opname);
+ return -EINVAL;
+ }
+
return __verify_planes_array(q->bufs[b->index], b);
}
@@ -225,6 +232,8 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, void *pb)
b->reserved = 0;
b->fence_fd = -1;
+ if (vb->out_fence_fd)
+ b->flags |= V4L2_BUF_FLAG_OUT_FENCE;
if (vb->in_fence)
b->flags |= V4L2_BUF_FLAG_IN_FENCE;
else
@@ -605,7 +614,20 @@ int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
}
}
- return vb2_core_qbuf(q, b->index, b, fence);
+ if (b->flags & V4L2_BUF_FLAG_OUT_FENCE) {
+ ret = vb2_setup_out_fence(q);
+ if (ret) {
+ dprintk(1, "failed to set up out-fence\n");
+ dma_fence_put(fence);
+ return ret;
+ }
+ }
+
+ ret = vb2_core_qbuf(q, b->index, b, fence);
+ if (ret && (b->flags & V4L2_BUF_FLAG_OUT_FENCE))
+ vb2_cleanup_out_fence(q);
+
+ return ret;
}
EXPORT_SYMBOL_GPL(vb2_qbuf);
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index 84e5e7216a1e..9ad774f796bb 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -258,6 +258,9 @@ struct vb2_buffer {
* in_fence: fence receive from vb2 client to wait on before
* using the buffer (queueing to the driver)
* fence_cb: fence callback information
+ * out_fence: the out-fence associated with the buffer once
+ * it is queued to the driver.
+ * out_fence_fd: the out_fence_fd to be shared with userspace.
*/
enum vb2_buffer_state state;
@@ -266,6 +269,9 @@ struct vb2_buffer {
struct dma_fence *in_fence;
struct dma_fence_cb fence_cb;
+
+ struct dma_fence *out_fence;
+ int out_fence_fd;
#ifdef CONFIG_VIDEO_ADV_DEBUG
/*
* Counters for how often these buffer-related ops are
@@ -512,6 +518,8 @@ struct vb2_fence {
* @done_list: list of buffers ready to be dequeued to userspace
* @done_lock: lock to protect done_list list
* @done_wq: waitqueue for processes waiting for buffers ready to be dequeued
+ * @out_fence_list: list of out fences waiting to be assigned to a buffer
+ * @out_fence_lock: lock to protect out_fence_list
* @alloc_devs: memory type/allocator-specific per-plane device
* @streaming: current streaming state
* @start_streaming_called: @start_streaming was called successfully and we
@@ -571,6 +579,9 @@ struct vb2_queue {
spinlock_t done_lock;
wait_queue_head_t done_wq;
+ struct list_head out_fence_list;
+ spinlock_t out_fence_lock;
+
struct device *alloc_devs[VB2_MAX_PLANES];
unsigned int streaming:1;
--
2.13.5
From: Gustavo Padovan <[email protected]>
Rename __close_fd() to close_fd() and export it to be able close files
in modules using file descriptors.
The usecase that motivates this change happens in V4L2 where we send
events to userspace with a fd that has file installed in it. But if for
some reason we have to cancel the video stream we need to close the files
that haven't been shared with userspace yet. Thus the export of
close_fd() becomes necessary.
fd_install() happens when we call an ioctl to queue a buffer, but we only
share the fd with userspace later, and that may happen in a kernel thread
instead.
Cc: Alexander Viro <[email protected]>
Cc: [email protected]
Cc: Riley Andrews <[email protected]>
Signed-off-by: Gustavo Padovan <[email protected]>
---
This is more like a question, I don't know how unhappy people will be with
this proposal to expose close_fd(). I'm all ears for more interesting
ways of doing it!
---
drivers/android/binder.c | 2 +-
fs/file.c | 5 +++--
fs/open.c | 2 +-
include/linux/fdtable.h | 2 +-
4 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index f7665c31feca..5a9bc73012df 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -440,7 +440,7 @@ static long task_close_fd(struct binder_proc *proc, unsigned int fd)
if (proc->files == NULL)
return -ESRCH;
- retval = __close_fd(proc->files, fd);
+ retval = close_fd(proc->files, fd);
/* can't restart close syscall because file table entry was cleared */
if (unlikely(retval == -ERESTARTSYS ||
retval == -ERESTARTNOINTR ||
diff --git a/fs/file.c b/fs/file.c
index 1fc7fbbb4510..111d387ac190 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -618,7 +618,7 @@ EXPORT_SYMBOL(fd_install);
/*
* The same warnings as for __alloc_fd()/__fd_install() apply here...
*/
-int __close_fd(struct files_struct *files, unsigned fd)
+int close_fd(struct files_struct *files, unsigned fd)
{
struct file *file;
struct fdtable *fdt;
@@ -640,6 +640,7 @@ int __close_fd(struct files_struct *files, unsigned fd)
spin_unlock(&files->file_lock);
return -EBADF;
}
+EXPORT_SYMBOL(close_fd);
void do_close_on_exec(struct files_struct *files)
{
@@ -856,7 +857,7 @@ int replace_fd(unsigned fd, struct file *file, unsigned flags)
struct files_struct *files = current->files;
if (!file)
- return __close_fd(files, fd);
+ return close_fd(files, fd);
if (fd >= rlimit(RLIMIT_NOFILE))
return -EBADF;
diff --git a/fs/open.c b/fs/open.c
index 35bb784763a4..30907d967443 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -1152,7 +1152,7 @@ EXPORT_SYMBOL(filp_close);
*/
SYSCALL_DEFINE1(close, unsigned int, fd)
{
- int retval = __close_fd(current->files, fd);
+ int retval = close_fd(current->files, fd);
/* can't restart close syscall because file table entry was cleared */
if (unlikely(retval == -ERESTARTSYS ||
diff --git a/include/linux/fdtable.h b/include/linux/fdtable.h
index 6e84b2cae6ad..511fd38d5e4b 100644
--- a/include/linux/fdtable.h
+++ b/include/linux/fdtable.h
@@ -115,7 +115,7 @@ extern int __alloc_fd(struct files_struct *files,
unsigned start, unsigned end, unsigned flags);
extern void __fd_install(struct files_struct *files,
unsigned int fd, struct file *file);
-extern int __close_fd(struct files_struct *files,
+extern int close_fd(struct files_struct *files,
unsigned int fd);
extern struct kmem_cache *files_cachep;
--
2.13.5
From: Gustavo Padovan <[email protected]>
Add a new event the userspace can subscribe to receive notifications
when a buffer is queued onto the driver. The event provides the index of
the queued buffer.
v2: - Add missing Documentation (Mauro)
Signed-off-by: Gustavo Padovan <[email protected]>
---
Documentation/media/uapi/v4l/vidioc-dqevent.rst | 23 +++++++++++++++++++++++
Documentation/media/videodev2.h.rst.exceptions | 1 +
include/uapi/linux/videodev2.h | 11 +++++++++++
3 files changed, 35 insertions(+)
diff --git a/Documentation/media/uapi/v4l/vidioc-dqevent.rst b/Documentation/media/uapi/v4l/vidioc-dqevent.rst
index fcd9c933870d..55f9dbdca6ec 100644
--- a/Documentation/media/uapi/v4l/vidioc-dqevent.rst
+++ b/Documentation/media/uapi/v4l/vidioc-dqevent.rst
@@ -78,6 +78,10 @@ call.
- ``src_change``
- Event data for event V4L2_EVENT_SOURCE_CHANGE.
* -
+ - struct :c:type:`v4l2_event_buf_queued`
+ - ``buf_queued``
+ - Event data for event V4L2_EVENT_BUF_QUEUED.
+ * -
- __u8
- ``data``\ [64]
- Event data. Defined by the event type. The union should be used to
@@ -337,6 +341,25 @@ call.
each cell in the motion detection grid, then that all cells are
automatically assigned to the default region 0.
+.. c:type:: v4l2_event_buf_queued
+
+.. flat-table:: struct v4l2_event_buf_queued
+ :header-rows: 0
+ :stub-columns: 0
+ :widths: 1 1 2
+
+ * - __u32
+ - ``index``
+ - The index of the buffer that was queued to the driver.
+ * - __s32
+ - ``out_fence_fd``
+ - The out-fence file descriptor of the buffer that was queued to
+ the driver. It will signal when the buffer is ready, or if an
+ error happens.
+
+
+
+.. tabularcolumns:: |p{4.4cm}|p{4.4cm}|p{8.7cm}|
.. tabularcolumns:: |p{6.6cm}|p{2.2cm}|p{8.7cm}|
diff --git a/Documentation/media/videodev2.h.rst.exceptions b/Documentation/media/videodev2.h.rst.exceptions
index a5cb0a8686ac..4e014b1d0317 100644
--- a/Documentation/media/videodev2.h.rst.exceptions
+++ b/Documentation/media/videodev2.h.rst.exceptions
@@ -462,6 +462,7 @@ replace define V4L2_EVENT_CTRL event-type
replace define V4L2_EVENT_FRAME_SYNC event-type
replace define V4L2_EVENT_SOURCE_CHANGE event-type
replace define V4L2_EVENT_MOTION_DET event-type
+replace define V4L2_EVENT_BUF_QUEUED event-type
replace define V4L2_EVENT_PRIVATE_START event-type
replace define V4L2_EVENT_CTRL_CH_VALUE ctrl-changes-flags
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index e5abab9a908c..e2ec0b66f490 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -2158,6 +2158,7 @@ struct v4l2_streamparm {
#define V4L2_EVENT_FRAME_SYNC 4
#define V4L2_EVENT_SOURCE_CHANGE 5
#define V4L2_EVENT_MOTION_DET 6
+#define V4L2_EVENT_BUF_QUEUED 7
#define V4L2_EVENT_PRIVATE_START 0x08000000
/* Payload for V4L2_EVENT_VSYNC */
@@ -2210,6 +2211,15 @@ struct v4l2_event_motion_det {
__u32 region_mask;
};
+/**
+ * struct v4l2_event_buf_queued - buffer queued in the driver event
+ * @index: index of the buffer queued in the driver
+ */
+struct v4l2_event_buf_queued {
+ __u32 index;
+ __s32 out_fence_fd;
+};
+
struct v4l2_event {
__u32 type;
union {
@@ -2218,6 +2228,7 @@ struct v4l2_event {
struct v4l2_event_frame_sync frame_sync;
struct v4l2_event_src_change src_change;
struct v4l2_event_motion_det motion_det;
+ struct v4l2_event_buf_queued buf_queued;
__u8 data[64];
} u;
__u32 pending;
--
2.13.5
From: Gustavo Padovan <[email protected]>
Implement the needed pieces to let userspace subscribe for
V4L2_EVENT_BUF_QUEUED events. Videobuf2 will queue the event for the
DQEVENT ioctl.
v3: - Do not call v4l2 event API from vb2 (Mauro)
v2: - Use VIDEO_MAX_FRAME to allocate room for events at
v4l2_event_subscribe() (Hans)
Signed-off-by: Gustavo Padovan <[email protected]>
---
drivers/media/v4l2-core/v4l2-ctrls.c | 6 +++++-
drivers/media/v4l2-core/videobuf2-core.c | 2 ++
drivers/media/v4l2-core/videobuf2-v4l2.c | 14 ++++++++++++++
3 files changed, 21 insertions(+), 1 deletion(-)
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index dd1db678718c..17d4b9e3eec6 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -3438,8 +3438,12 @@ EXPORT_SYMBOL(v4l2_ctrl_log_status);
int v4l2_ctrl_subscribe_event(struct v4l2_fh *fh,
const struct v4l2_event_subscription *sub)
{
- if (sub->type == V4L2_EVENT_CTRL)
+ switch (sub->type) {
+ case V4L2_EVENT_CTRL:
return v4l2_event_subscribe(fh, sub, 0, &v4l2_ctrl_sub_ev_ops);
+ case V4L2_EVENT_BUF_QUEUED:
+ return v4l2_event_subscribe(fh, sub, VIDEO_MAX_FRAME, NULL);
+ }
return -EINVAL;
}
EXPORT_SYMBOL(v4l2_ctrl_subscribe_event);
diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index b19c1bc4b083..bbbae0eed567 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -1231,6 +1231,8 @@ static void __enqueue_in_driver(struct vb2_buffer *vb)
trace_vb2_buf_queue(q, vb);
call_void_vb_qop(vb, buf_queue, vb);
+
+ call_void_bufop(q, buffer_queued, vb);
}
static int __buf_prepare(struct vb2_buffer *vb, const void *pb)
diff --git a/drivers/media/v4l2-core/videobuf2-v4l2.c b/drivers/media/v4l2-core/videobuf2-v4l2.c
index 8c322cd1b346..bbfcd054e6f6 100644
--- a/drivers/media/v4l2-core/videobuf2-v4l2.c
+++ b/drivers/media/v4l2-core/videobuf2-v4l2.c
@@ -138,6 +138,19 @@ static void __copy_timestamp(struct vb2_buffer *vb, const void *pb)
}
};
+static void __buffer_queued(struct vb2_buffer *vb)
+{
+ struct video_device *vdev = to_video_device(vb->vb2_queue->dev);
+ struct v4l2_fh *fh = vdev->queue->owner;
+ struct v4l2_event event;
+
+ memset(&event, 0, sizeof(event));
+ event.type = V4L2_EVENT_BUF_QUEUED;
+ event.u.buf_queued.index = vb->index;
+
+ v4l2_event_queue_fh(fh, &event);
+}
+
static void vb2_warn_zero_bytesused(struct vb2_buffer *vb)
{
static bool check_once;
@@ -455,6 +468,7 @@ static const struct vb2_buf_ops v4l2_buf_ops = {
.fill_user_buffer = __fill_v4l2_buffer,
.fill_vb2_buffer = __fill_vb2_buffer,
.copy_timestamp = __copy_timestamp,
+ .buffer_queued = __buffer_queued,
};
/**
--
2.13.5
From: Gustavo Padovan <[email protected]>
For explicit synchronization (and soon for HAL3/Request API) we need
the v4l2-driver to guarantee the ordering in which the buffers were queued
by userspace. This is already true for many drivers, but we never needed
to say it.
Signed-off-by: Gustavo Padovan <[email protected]>
---
include/media/videobuf2-core.h | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index 5ed8d3402474..20099dc22f26 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -508,6 +508,9 @@ struct vb2_buf_ops {
* @last_buffer_dequeued: used in poll() and DQBUF to immediately return if the
* last decoded buffer was already dequeued. Set for capture queues
* when a buffer with the V4L2_BUF_FLAG_LAST is dequeued.
+ * @ordered: if the driver can guarantee that the queue will be ordered or not.
+ * The default is not ordered unless the driver sets this flag. It
+ * is mandatory for using explicit fences.
* @fileio: file io emulator internal data, used only if emulator is active
* @threadio: thread io internal data, used only if thread is active
*/
@@ -560,6 +563,7 @@ struct vb2_queue {
unsigned int is_output:1;
unsigned int copy_timestamp:1;
unsigned int last_buffer_dequeued:1;
+ unsigned int ordered:1;
struct vb2_fileio_data *fileio;
struct vb2_threadio_data *threadio;
--
2.13.5
From: Gustavo Padovan <[email protected]>
Call v4l2_ctrl_subscribe_event to subscribe to the BUF_QUEUED event as
well.
Signed-off-by: Gustavo Padovan <[email protected]>
---
drivers/media/usb/uvc/uvc_v4l2.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index 3e7e283a44a8..dfa0ccdcf849 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -1240,7 +1240,7 @@ static int uvc_ioctl_subscribe_event(struct v4l2_fh *fh,
case V4L2_EVENT_CTRL:
return v4l2_event_subscribe(fh, sub, 0, &uvc_ctrl_sub_ev_ops);
default:
- return -EINVAL;
+ return v4l2_ctrl_subscribe_event(fh, sub);
}
}
--
2.13.5
From: Gustavo Padovan <[email protected]>
To support explicit synchronization we need to run all operations that can
fail before we queue the buffer to the driver. With fences the queueing
will be delayed if the fence is not signaled yet and it will be better if
such callback do not fail.
For that we move the vb2_start_streaming() before the queuing for the
buffer may happen.
Signed-off-by: Gustavo Padovan <[email protected]>
---
drivers/media/v4l2-core/videobuf2-core.c | 20 +++++++++-----------
1 file changed, 9 insertions(+), 11 deletions(-)
diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index cb115ba6a1d2..60f8b582396a 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -1399,29 +1399,27 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
trace_vb2_qbuf(q, vb);
/*
- * If already streaming, give the buffer to driver for processing.
- * If not, the buffer will be given to driver on next streamon.
- */
- if (q->start_streaming_called)
- __enqueue_in_driver(vb);
-
- /* Fill buffer information for the userspace */
- if (pb)
- call_void_bufop(q, fill_user_buffer, vb, pb);
-
- /*
* If streamon has been called, and we haven't yet called
* start_streaming() since not enough buffers were queued, and
* we now have reached the minimum number of queued buffers,
* then we can finally call start_streaming().
+ *
+ * If already streaming, give the buffer to driver for processing.
+ * If not, the buffer will be given to driver on next streamon.
*/
if (q->streaming && !q->start_streaming_called &&
q->queued_count >= q->min_buffers_needed) {
ret = vb2_start_streaming(q);
if (ret)
return ret;
+ } else if (q->start_streaming_called) {
+ __enqueue_in_driver(vb);
}
+ /* Fill buffer information for the userspace */
+ if (pb)
+ call_void_bufop(q, fill_user_buffer, vb, pb);
+
dprintk(2, "qbuf of buffer %d succeeded\n", vb->index);
return 0;
}
--
2.13.5
On Thu, Sep 07, 2017 at 03:42:25PM -0300, Gustavo Padovan wrote:
> From: Gustavo Padovan <[email protected]>
>
> Rename __close_fd() to close_fd() and export it to be able close files
> in modules using file descriptors.
>
> The usecase that motivates this change happens in V4L2 where we send
> events to userspace with a fd that has file installed in it. But if for
> some reason we have to cancel the video stream we need to close the files
> that haven't been shared with userspace yet. Thus the export of
> close_fd() becomes necessary.
>
> fd_install() happens when we call an ioctl to queue a buffer, but we only
> share the fd with userspace later, and that may happen in a kernel thread
> instead.
What do you mean? A file descriptor is shared with userspace as soon as it's
installed in the fdtable by fd_install(). As soon as it's there, another thread
can use it (or close it, duplicate it, etc.), even before the syscall that
installed it returns...
Eric
On Thu, Sep 07, 2017 at 03:42:25PM -0300, Gustavo Padovan wrote:
> From: Gustavo Padovan <[email protected]>
>
> Rename __close_fd() to close_fd() and export it to be able close files
> in modules using file descriptors.
>
> The usecase that motivates this change happens in V4L2 where we send
> events to userspace with a fd that has file installed in it. But if for
> some reason we have to cancel the video stream we need to close the files
> that haven't been shared with userspace yet. Thus the export of
> close_fd() becomes necessary.
>
> fd_install() happens when we call an ioctl to queue a buffer, but we only
> share the fd with userspace later, and that may happen in a kernel thread
> instead.
NAK. As soon as the reference is in descriptor table, you *can't* do anything
to it. This "sharing" part is complete BS - being _told_ that descriptor is
there does not matter at all. That descriptor might be hit with dup2() as
soon as fd_install() has happened. Or be closed, or any number of other things.
You can not take it back. Once fd_install() is done, it's fucking done, period.
If V4L2 requires removing it from descriptor table, it's a shitty API and needs
to be fixed.
Again, NAK.
2017-09-07 Al Viro <[email protected]>:
> On Thu, Sep 07, 2017 at 03:42:25PM -0300, Gustavo Padovan wrote:
> > From: Gustavo Padovan <[email protected]>
> >
> > Rename __close_fd() to close_fd() and export it to be able close files
> > in modules using file descriptors.
> >
> > The usecase that motivates this change happens in V4L2 where we send
> > events to userspace with a fd that has file installed in it. But if for
> > some reason we have to cancel the video stream we need to close the files
> > that haven't been shared with userspace yet. Thus the export of
> > close_fd() becomes necessary.
> >
> > fd_install() happens when we call an ioctl to queue a buffer, but we only
> > share the fd with userspace later, and that may happen in a kernel thread
> > instead.
>
> NAK. As soon as the reference is in descriptor table, you *can't* do anything
> to it. This "sharing" part is complete BS - being _told_ that descriptor is
> there does not matter at all. That descriptor might be hit with dup2() as
> soon as fd_install() has happened. Or be closed, or any number of other things.
>
> You can not take it back. Once fd_install() is done, it's fucking done, period.
> If V4L2 requires removing it from descriptor table, it's a shitty API and needs
> to be fixed.
Sorry for my lack of knowledge here and thank you for the explanation,
things are a lot clear to me. For some reasons I were trying to delay
the sharing of the fd to a event later. I can delay the install of it
but that my require __fd_install() to be available and exportedi as it
may happen in a thread, but I believe you wouldn't be okay with that either,
is that so?
Gustavo
On Thu, Sep 07, 2017 at 06:22:45PM -0300, Gustavo Padovan wrote:
> Sorry for my lack of knowledge here and thank you for the explanation,
> things are a lot clear to me. For some reasons I were trying to delay
> the sharing of the fd to a event later. I can delay the install of it
> but that my require __fd_install() to be available and exportedi as it
> may happen in a thread, but I believe you wouldn't be okay with that either,
> is that so?
Only if it has been given a reference to descriptor table to start with.
Which reference should've been acquired by the target process itself.
Why bother, anyway? You need to handle the case when the stream has
ended just after you'd copied the value to userland; at that point you
obviously can't go hunting for all references to struct file in question,
so you have to guaratee that methods will start giving an error from
that point on. What's the problem with just leaving it installed?
Both userland and kernel must cope with that sort of thing anyway, so
what does removing it from descriptor table and not reporting it buy
you? AFAICS, it's an extra layer of complexity for no good reason -
you are not getting it offset by simplifications anywhere else...
On 09/07/2017 08:42 PM, Gustavo Padovan wrote:
> From: Gustavo Padovan <[email protected]>
>
> Rename __close_fd() to close_fd() and export it to be able close files
> in modules using file descriptors.
>
> The usecase that motivates this change happens in V4L2 where we send
> events to userspace with a fd that has file installed in it. But if for
> some reason we have to cancel the video stream we need to close the files
> that haven't been shared with userspace yet. Thus the export of
> close_fd() becomes necessary.
>
> fd_install() happens when we call an ioctl to queue a buffer, but we only
> share the fd with userspace later, and that may happen in a kernel thread
> instead.
This isn't the way to do this.
You should only create the out fence file descriptor when userspace dequeues
(i.e. calls VIDIOC_DQEVENT) the BUF_QUEUED event. That's when you give it to
userspace and at that moment closing the fd is the responsibility of userspace.
There is no point creating it earlier anyway since userspace can't get to it
until it dequeues the event.
It does mean some more work in the V4L2 core since you need to hook into the
DQEVENT code in order to do this.
Regards,
Hans
>
> Cc: Alexander Viro <[email protected]>
> Cc: [email protected]
> Cc: Riley Andrews <[email protected]>
> Signed-off-by: Gustavo Padovan <[email protected]>
> ---
> This is more like a question, I don't know how unhappy people will be with
> this proposal to expose close_fd(). I'm all ears for more interesting
> ways of doing it!
> ---
> drivers/android/binder.c | 2 +-
> fs/file.c | 5 +++--
> fs/open.c | 2 +-
> include/linux/fdtable.h | 2 +-
> 4 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> index f7665c31feca..5a9bc73012df 100644
> --- a/drivers/android/binder.c
> +++ b/drivers/android/binder.c
> @@ -440,7 +440,7 @@ static long task_close_fd(struct binder_proc *proc, unsigned int fd)
> if (proc->files == NULL)
> return -ESRCH;
>
> - retval = __close_fd(proc->files, fd);
> + retval = close_fd(proc->files, fd);
> /* can't restart close syscall because file table entry was cleared */
> if (unlikely(retval == -ERESTARTSYS ||
> retval == -ERESTARTNOINTR ||
> diff --git a/fs/file.c b/fs/file.c
> index 1fc7fbbb4510..111d387ac190 100644
> --- a/fs/file.c
> +++ b/fs/file.c
> @@ -618,7 +618,7 @@ EXPORT_SYMBOL(fd_install);
> /*
> * The same warnings as for __alloc_fd()/__fd_install() apply here...
> */
> -int __close_fd(struct files_struct *files, unsigned fd)
> +int close_fd(struct files_struct *files, unsigned fd)
> {
> struct file *file;
> struct fdtable *fdt;
> @@ -640,6 +640,7 @@ int __close_fd(struct files_struct *files, unsigned fd)
> spin_unlock(&files->file_lock);
> return -EBADF;
> }
> +EXPORT_SYMBOL(close_fd);
>
> void do_close_on_exec(struct files_struct *files)
> {
> @@ -856,7 +857,7 @@ int replace_fd(unsigned fd, struct file *file, unsigned flags)
> struct files_struct *files = current->files;
>
> if (!file)
> - return __close_fd(files, fd);
> + return close_fd(files, fd);
>
> if (fd >= rlimit(RLIMIT_NOFILE))
> return -EBADF;
> diff --git a/fs/open.c b/fs/open.c
> index 35bb784763a4..30907d967443 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -1152,7 +1152,7 @@ EXPORT_SYMBOL(filp_close);
> */
> SYSCALL_DEFINE1(close, unsigned int, fd)
> {
> - int retval = __close_fd(current->files, fd);
> + int retval = close_fd(current->files, fd);
>
> /* can't restart close syscall because file table entry was cleared */
> if (unlikely(retval == -ERESTARTSYS ||
> diff --git a/include/linux/fdtable.h b/include/linux/fdtable.h
> index 6e84b2cae6ad..511fd38d5e4b 100644
> --- a/include/linux/fdtable.h
> +++ b/include/linux/fdtable.h
> @@ -115,7 +115,7 @@ extern int __alloc_fd(struct files_struct *files,
> unsigned start, unsigned end, unsigned flags);
> extern void __fd_install(struct files_struct *files,
> unsigned int fd, struct file *file);
> -extern int __close_fd(struct files_struct *files,
> +extern int close_fd(struct files_struct *files,
> unsigned int fd);
>
> extern struct kmem_cache *files_cachep;
>
On Fri, 2017-09-08 at 00:09 +0200, Hans Verkuil wrote:
> On 09/07/2017 08:42 PM, Gustavo Padovan wrote:
> > From: Gustavo Padovan <[email protected]>
> >
> > Rename __close_fd() to close_fd() and export it to be able close
> > files
> > in modules using file descriptors.
> >
> > The usecase that motivates this change happens in V4L2 where we
> > send
> > events to userspace with a fd that has file installed in it. But if
> > for
> > some reason we have to cancel the video stream we need to close the
> > files
> > that haven't been shared with userspace yet. Thus the export of
> > close_fd() becomes necessary.
> >
> > fd_install() happens when we call an ioctl to queue a buffer, but
> > we only
> > share the fd with userspace later, and that may happen in a kernel
> > thread
> > instead.
>
> This isn't the way to do this.
>
> You should only create the out fence file descriptor when userspace
> dequeues
> (i.e. calls VIDIOC_DQEVENT) the BUF_QUEUED event. That's when you
> give it to
> userspace and at that moment closing the fd is the responsibility of
> userspace.
> There is no point creating it earlier anyway since userspace can't
> get to it
> until it dequeues the event.
>
> It does mean some more work in the V4L2 core since you need to hook
> into the
> DQEVENT code in order to do this.
Right, that makes a lot more sense. I'll change the implementation so
it can reflecting that. Thanks.
Gustavo
Le jeudi 07 septembre 2017 à 15:42 -0300, Gustavo Padovan a écrit :
> From: Gustavo Padovan <[email protected]>
>
> Add section to VIDIOC_QBUF about it
>
> v2:
> - mention that fences are files (Hans)
> - rework for the new API
>
> Signed-off-by: Gustavo Padovan <[email protected]>
> ---
> Documentation/media/uapi/v4l/vidioc-qbuf.rst | 31 ++++++++++++++++++++++++++++
> 1 file changed, 31 insertions(+)
>
> diff --git a/Documentation/media/uapi/v4l/vidioc-qbuf.rst b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
> index 1f3612637200..fae0b1431672 100644
> --- a/Documentation/media/uapi/v4l/vidioc-qbuf.rst
> +++ b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
> @@ -117,6 +117,37 @@ immediately with an ``EAGAIN`` error code when no buffer is available.
> The struct :c:type:`v4l2_buffer` structure is specified in
> :ref:`buffer`.
>
> +Explicit Synchronization
> +------------------------
> +
> +Explicit Synchronization allows us to control the synchronization of
> +shared buffers from userspace by passing fences to the kernel and/or
> +receiving them from it. Fences passed to the kernel are named in-fences and
> +the kernel should wait them to signal before using the buffer, i.e., queueing
> +it to the driver. On the other side, the kernel can create out-fences for the
> +buffers it queues to the drivers, out-fences signal when the driver is
> +finished with buffer, that is the buffer is ready. The fence are represented
> +by file and passed as file descriptor to userspace.
I think the API works to deliver the fence FD userspace, though for the
userspace I maintain (GStreamer) it's often the case that the buffer is
unusable without the associated timestamp.
Let's consider the capture to display case (V4L2 -> DRM). As soon as
you add audio capture to the loop, GStreamer will need to start dealing
with synchronization. We can't just blindly give that buffer to the
display, we need to make sure this buffer makes it on time, in a way
that it is synchronized with the audio. To deal with synchronization,
we need to be able to correlate the video image capture time with the
audio capture time.
The problem is that this timestamp is only delivered when DQBUF
succeed, which is after the fence has unblocked. This makes the fences
completely unusable for that purpose. In general, to achieve very low
latency and still have synchronization, we'll probably need the
timestamp to be delivered somehow before the image transfer have
complete. Obviously, this is only possible if we have timestamp with
flag V4L2_BUF_FLAG_TSTAMP_SRC_SOE.
On another note, for m2m drivers that behave as color converters and
scalers, with ordered queues, userspace knows the timestamp already, so
using the proposed API and passing the buffer immediately with it's
fence is really going to help.
For encoded (compressed) data, similar issues will be found with the
bytesused member of struct v4l2_buffer. We'll need to know the size of
the encoded data before we can pass it to another driver. I'm not sure
how relevant fences are for this type of data.
> +
> +The in-fences are communicated to the kernel at the ``VIDIOC_QBUF`` ioctl
> +using the ``V4L2_BUF_FLAG_IN_FENCE`` buffer
> +flags and the `fence_fd` field. If an in-fence needs to be passed to the kernel,
> +`fence_fd` should be set to the fence file descriptor number and the
> +``V4L2_BUF_FLAG_IN_FENCE`` should be set as well. Failure to set both will
> +cause ``VIDIOC_QBUF`` to return with error.
> +
> +To get a out-fence back from V4L2 the ``V4L2_BUF_FLAG_OUT_FENCE`` flag should
> +be set to notify it that the next queued buffer should have a fence attached to
> +it. That means the out-fence may not be associated with the buffer in the
> +current ``VIDIOC_QBUF`` ioctl call because the ordering in which videobuf2 core
> +queues the buffers to the drivers can't be guaranteed. To become aware of the
> +of the next queued buffer and the out-fence attached to it the
"of the" is repeated twice.
> +``V4L2_EVENT_BUF_QUEUED`` event should be used. It will trigger an event
> +for every buffer queued to the V4L2 driver.
> +
> +At streamoff the out-fences will either signal normally if the drivers wait
> +for the operations on the buffers to finish or signal with error if the
> +driver cancel the pending operations.
>
> Return Value
> ============
On 09/07/2017 08:42 PM, Gustavo Padovan wrote:
> From: Gustavo Padovan <[email protected]>
>
> Add section to VIDIOC_QBUF about it
>
> v2:
> - mention that fences are files (Hans)
> - rework for the new API
>
> Signed-off-by: Gustavo Padovan <[email protected]>
> ---
> Documentation/media/uapi/v4l/vidioc-qbuf.rst | 31 ++++++++++++++++++++++++++++
> 1 file changed, 31 insertions(+)
>
> diff --git a/Documentation/media/uapi/v4l/vidioc-qbuf.rst b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
> index 1f3612637200..fae0b1431672 100644
> --- a/Documentation/media/uapi/v4l/vidioc-qbuf.rst
> +++ b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
> @@ -117,6 +117,37 @@ immediately with an ``EAGAIN`` error code when no buffer is available.
> The struct :c:type:`v4l2_buffer` structure is specified in
> :ref:`buffer`.
>
> +Explicit Synchronization
> +------------------------
> +
> +Explicit Synchronization allows us to control the synchronization of
> +shared buffers from userspace by passing fences to the kernel and/or
> +receiving them from it. Fences passed to the kernel are named in-fences and
> +the kernel should wait them to signal before using the buffer, i.e., queueing
wait them -> wait on them
(do you wait 'on' a fence or 'for' a fence? I think it's 'on' but I'm not 100% sure)
> +it to the driver. On the other side, the kernel can create out-fences for the
> +buffers it queues to the drivers, out-fences signal when the driver is
Start a new sentence here: ...drivers. Out-fences...
> +finished with buffer, that is the buffer is ready. The fence are represented
s/that is/i.e/
s/The fence/The fences/
> +by file and passed as file descriptor to userspace.
s/by file/as a file/
s/as file/as a file/
> +
> +The in-fences are communicated to the kernel at the ``VIDIOC_QBUF`` ioctl
> +using the ``V4L2_BUF_FLAG_IN_FENCE`` buffer
> +flags and the `fence_fd` field. If an in-fence needs to be passed to the kernel,
> +`fence_fd` should be set to the fence file descriptor number and the
> +``V4L2_BUF_FLAG_IN_FENCE`` should be set as well. Failure to set both will
s/Failure to set both/Setting one but not the other/
> +cause ``VIDIOC_QBUF`` to return with error.
> +
> +To get a out-fence back from V4L2 the ``V4L2_BUF_FLAG_OUT_FENCE`` flag should
> +be set to notify it that the next queued buffer should have a fence attached to
> +it. That means the out-fence may not be associated with the buffer in the
> +current ``VIDIOC_QBUF`` ioctl call because the ordering in which videobuf2 core
> +queues the buffers to the drivers can't be guaranteed. To become aware of the
> +of the next queued buffer and the out-fence attached to it the
> +``V4L2_EVENT_BUF_QUEUED`` event should be used. It will trigger an event
> +for every buffer queued to the V4L2 driver.
This makes no sense.
Setting this flag means IMHO that when *this* buffer is queued up to the driver,
then it should send the BUF_QUEUED event with an out fence.
I.e. it signals that userspace wants to have the out-fence. The requirement w.r.t.
ordering is that the BUF_QUEUED events have to be in order, but that is something
that the driver can ensure in the case it is doing internal re-ordering.
This requirement is something that needs to be documented here, BTW.
Anyway, the flag shouldn't refer to some 'next buffer', since that's very confusing.
> +
> +At streamoff the out-fences will either signal normally if the drivers wait
s/drivers wait/driver waits/
> +for the operations on the buffers to finish or signal with error if the
> +driver cancel the pending operations.
s/cancel/cancels/
Thinking with my evil hat on:
What happens if the application dequeues the buffer (VIDIOC_DQBUF) before
dequeuing the BUF_QUEUED event? Or if the application doesn't call VIDIOC_DQEVENT
at all? Should any pending BUF_QUEUED event with an out fence be removed from the
event queue if the application calls DQBUF on the corresponding buffer?
Regards,
Hans
>
> Return Value
> ============
>
On 09/07/2017 08:42 PM, Gustavo Padovan wrote:
> From: Gustavo Padovan <[email protected]>
>
> Turn the reserved2 field into fence_fd that we will use to send
> an in-fence to the kernel and return an out-fence from the kernel to
> userspace.
>
> Two new flags were added, V4L2_BUF_FLAG_IN_FENCE, that should be used
> when sending a fence to the kernel to be waited on, and
> V4L2_BUF_FLAG_OUT_FENCE, to ask the kernel to give back an out-fence.
>
> v2: add documentation
>
> Signed-off-by: Gustavo Padovan <[email protected]>
> ---
> Documentation/media/uapi/v4l/buffer.rst | 19 +++++++++++++++++++
> drivers/media/usb/cpia2/cpia2_v4l.c | 2 +-
> drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 4 ++--
> drivers/media/v4l2-core/videobuf2-v4l2.c | 2 +-
> include/uapi/linux/videodev2.h | 4 +++-
> 5 files changed, 26 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/media/uapi/v4l/buffer.rst b/Documentation/media/uapi/v4l/buffer.rst
> index ae6ee73f151c..664507ad06c6 100644
> --- a/Documentation/media/uapi/v4l/buffer.rst
> +++ b/Documentation/media/uapi/v4l/buffer.rst
> @@ -648,6 +648,25 @@ Buffer Flags
> - Start Of Exposure. The buffer timestamp has been taken when the
> exposure of the frame has begun. This is only valid for the
> ``V4L2_BUF_TYPE_VIDEO_CAPTURE`` buffer type.
> + * .. _`V4L2-BUF-FLAG-IN-FENCE`:
> +
> + - ``V4L2_BUF_FLAG_IN_FENCE``
> + - 0x00200000
> + - Ask V4L2 to wait on fence passed in ``fence_fd`` field. The buffer
> + won't be queued to the driver until the fence signals.
> +
> + * .. _`V4L2-BUF-FLAG-OUT-FENCE`:
> +
> + - ``V4L2_BUF_FLAG_OUT_FENCE``
> + - 0x00400000
> + - Request a fence for the next buffer to be queued to V4L2 driver.
> + The fence received back through the ``fence_fd`` field doesn't
> + necessarily relate to the current buffer in the
> + :ref:`VIDIOC_QBUF <VIDIOC_QBUF>` ioctl. Although, most of the time
> + the fence will relate to the current buffer it can't be guaranteed.
> + So to tell userspace which buffer is associated to the out_fence,
> + one should listen for the ``V4L2_EVENT_BUF_QUEUED`` event that
> + provide the id of the buffer when it is queued to the V4L2 driver.
As mentioned in the previous patch, this flag should just signal that userspace
wants to receive a BUF_QUEUED event with the out-fence for this buffer. It's up
to the driver to ensure the correct ordering of the events.
I'm missing documentation for the new fence_fd field.
It should mention that fence_fd is ignored if the IN_FENCE isn't set. Applications
will set fence_fd (aka reserved2) to 0, which should be fine as long as IN_FENCE
isn't set.
>
>
>
> diff --git a/drivers/media/usb/cpia2/cpia2_v4l.c b/drivers/media/usb/cpia2/cpia2_v4l.c
> index 3dedd83f0b19..6cde686bf44c 100644
> --- a/drivers/media/usb/cpia2/cpia2_v4l.c
> +++ b/drivers/media/usb/cpia2/cpia2_v4l.c
> @@ -948,7 +948,7 @@ static int cpia2_dqbuf(struct file *file, void *fh, struct v4l2_buffer *buf)
> buf->sequence = cam->buffers[buf->index].seq;
> buf->m.offset = cam->buffers[buf->index].data - cam->frame_buffer;
> buf->length = cam->frame_size;
> - buf->reserved2 = 0;
> + buf->fence_fd = -1;
> buf->reserved = 0;
> memset(&buf->timecode, 0, sizeof(buf->timecode));
>
> diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> index 821f2aa299ae..d624fb5df130 100644
> --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> @@ -370,7 +370,7 @@ struct v4l2_buffer32 {
> __s32 fd;
> } m;
> __u32 length;
> - __u32 reserved2;
> + __s32 fence_fd;
> __u32 reserved;
> };
>
> @@ -533,8 +533,8 @@ static int put_v4l2_buffer32(struct v4l2_buffer *kp, struct v4l2_buffer32 __user
> put_user(kp->timestamp.tv_usec, &up->timestamp.tv_usec) ||
> copy_to_user(&up->timecode, &kp->timecode, sizeof(struct v4l2_timecode)) ||
> put_user(kp->sequence, &up->sequence) ||
> - put_user(kp->reserved2, &up->reserved2) ||
> put_user(kp->reserved, &up->reserved) ||
> + put_user(kp->fence_fd, &up->fence_fd) ||
Could you move this up one line? It's easier to parse this diff if it is clear
that fence_fd replaced reserved2.
> put_user(kp->length, &up->length))
> return -EFAULT;
>
> diff --git a/drivers/media/v4l2-core/videobuf2-v4l2.c b/drivers/media/v4l2-core/videobuf2-v4l2.c
> index 0c0669976bdc..110fb45fef6f 100644
> --- a/drivers/media/v4l2-core/videobuf2-v4l2.c
> +++ b/drivers/media/v4l2-core/videobuf2-v4l2.c
> @@ -203,7 +203,7 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, void *pb)
> b->timestamp = ns_to_timeval(vb->timestamp);
> b->timecode = vbuf->timecode;
> b->sequence = vbuf->sequence;
> - b->reserved2 = 0;
> + b->fence_fd = -1;
> b->reserved = 0;
>
> if (q->is_multiplanar) {
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 185d6a0acc06..e5abab9a908c 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -924,7 +924,7 @@ struct v4l2_buffer {
> __s32 fd;
> } m;
> __u32 length;
> - __u32 reserved2;
> + __s32 fence_fd;
> __u32 reserved;
> };
>
> @@ -961,6 +961,8 @@ struct v4l2_buffer {
> #define V4L2_BUF_FLAG_TSTAMP_SRC_SOE 0x00010000
> /* mem2mem encoder/decoder */
> #define V4L2_BUF_FLAG_LAST 0x00100000
> +#define V4L2_BUF_FLAG_IN_FENCE 0x00200000
> +#define V4L2_BUF_FLAG_OUT_FENCE 0x00400000
>
> /**
> * struct v4l2_exportbuffer - export of video buffer as DMABUF file descriptor
>
Regards,
Hans
On 09/11/2017 12:50 PM, Hans Verkuil wrote:
> On 09/07/2017 08:42 PM, Gustavo Padovan wrote:
>> From: Gustavo Padovan <[email protected]>
>>
>> Add section to VIDIOC_QBUF about it
>>
>> v2:
>> - mention that fences are files (Hans)
>> - rework for the new API
>>
>> Signed-off-by: Gustavo Padovan <[email protected]>
>> ---
>> Documentation/media/uapi/v4l/vidioc-qbuf.rst | 31 ++++++++++++++++++++++++++++
>> 1 file changed, 31 insertions(+)
>>
>> diff --git a/Documentation/media/uapi/v4l/vidioc-qbuf.rst b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
>> index 1f3612637200..fae0b1431672 100644
>> --- a/Documentation/media/uapi/v4l/vidioc-qbuf.rst
>> +++ b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
>> @@ -117,6 +117,37 @@ immediately with an ``EAGAIN`` error code when no buffer is available.
>> The struct :c:type:`v4l2_buffer` structure is specified in
>> :ref:`buffer`.
>>
>> +Explicit Synchronization
>> +------------------------
>> +
>> +Explicit Synchronization allows us to control the synchronization of
>> +shared buffers from userspace by passing fences to the kernel and/or
>> +receiving them from it. Fences passed to the kernel are named in-fences and
>> +the kernel should wait them to signal before using the buffer, i.e., queueing
>
> wait them -> wait on them
>
> (do you wait 'on' a fence or 'for' a fence? I think it's 'on' but I'm not 100% sure)
>
>> +it to the driver. On the other side, the kernel can create out-fences for the
>> +buffers it queues to the drivers, out-fences signal when the driver is
>
> Start a new sentence here: ...drivers. Out-fences...
>
>> +finished with buffer, that is the buffer is ready. The fence are represented
>
> s/that is/i.e/
>
> s/The fence/The fences/
>
>> +by file and passed as file descriptor to userspace.
>
> s/by file/as a file/
> s/as file/as a file/
>
>> +
>> +The in-fences are communicated to the kernel at the ``VIDIOC_QBUF`` ioctl
>> +using the ``V4L2_BUF_FLAG_IN_FENCE`` buffer
>> +flags and the `fence_fd` field. If an in-fence needs to be passed to the kernel,
>> +`fence_fd` should be set to the fence file descriptor number and the
>> +``V4L2_BUF_FLAG_IN_FENCE`` should be set as well. Failure to set both will
>
> s/Failure to set both/Setting one but not the other/
>
>> +cause ``VIDIOC_QBUF`` to return with error.
>> +
>> +To get a out-fence back from V4L2 the ``V4L2_BUF_FLAG_OUT_FENCE`` flag should
>> +be set to notify it that the next queued buffer should have a fence attached to
>> +it. That means the out-fence may not be associated with the buffer in the
>> +current ``VIDIOC_QBUF`` ioctl call because the ordering in which videobuf2 core
>> +queues the buffers to the drivers can't be guaranteed. To become aware of the
>> +of the next queued buffer and the out-fence attached to it the
>> +``V4L2_EVENT_BUF_QUEUED`` event should be used. It will trigger an event
>> +for every buffer queued to the V4L2 driver.
>
> This makes no sense.
>
> Setting this flag means IMHO that when *this* buffer is queued up to the driver,
> then it should send the BUF_QUEUED event with an out fence.
>
> I.e. it signals that userspace wants to have the out-fence. The requirement w.r.t.
> ordering is that the BUF_QUEUED events have to be in order, but that is something
> that the driver can ensure in the case it is doing internal re-ordering.
>
> This requirement is something that needs to be documented here, BTW.
>
> Anyway, the flag shouldn't refer to some 'next buffer', since that's very confusing.
Just ignore this comment. I assume v4 will implement it like this.
Regards,
Hans
>
>> +
>> +At streamoff the out-fences will either signal normally if the drivers wait
>
> s/drivers wait/driver waits/
>
>> +for the operations on the buffers to finish or signal with error if the
>> +driver cancel the pending operations.
>
> s/cancel/cancels/
>
> Thinking with my evil hat on:
>
> What happens if the application dequeues the buffer (VIDIOC_DQBUF) before
> dequeuing the BUF_QUEUED event? Or if the application doesn't call VIDIOC_DQEVENT
> at all? Should any pending BUF_QUEUED event with an out fence be removed from the
> event queue if the application calls DQBUF on the corresponding buffer?
>
> Regards,
>
> Hans
>
>>
>> Return Value
>> ============
>>
>
On 09/11/2017 12:55 PM, Hans Verkuil wrote:
> On 09/07/2017 08:42 PM, Gustavo Padovan wrote:
>> From: Gustavo Padovan <[email protected]>
>>
>> Turn the reserved2 field into fence_fd that we will use to send
>> an in-fence to the kernel and return an out-fence from the kernel to
>> userspace.
>>
>> Two new flags were added, V4L2_BUF_FLAG_IN_FENCE, that should be used
>> when sending a fence to the kernel to be waited on, and
>> V4L2_BUF_FLAG_OUT_FENCE, to ask the kernel to give back an out-fence.
>>
>> v2: add documentation
>>
>> Signed-off-by: Gustavo Padovan <[email protected]>
>> ---
>> Documentation/media/uapi/v4l/buffer.rst | 19 +++++++++++++++++++
>> drivers/media/usb/cpia2/cpia2_v4l.c | 2 +-
>> drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 4 ++--
>> drivers/media/v4l2-core/videobuf2-v4l2.c | 2 +-
>> include/uapi/linux/videodev2.h | 4 +++-
>> 5 files changed, 26 insertions(+), 5 deletions(-)
>>
>> diff --git a/Documentation/media/uapi/v4l/buffer.rst b/Documentation/media/uapi/v4l/buffer.rst
>> index ae6ee73f151c..664507ad06c6 100644
>> --- a/Documentation/media/uapi/v4l/buffer.rst
>> +++ b/Documentation/media/uapi/v4l/buffer.rst
>> @@ -648,6 +648,25 @@ Buffer Flags
>> - Start Of Exposure. The buffer timestamp has been taken when the
>> exposure of the frame has begun. This is only valid for the
>> ``V4L2_BUF_TYPE_VIDEO_CAPTURE`` buffer type.
>> + * .. _`V4L2-BUF-FLAG-IN-FENCE`:
>> +
>> + - ``V4L2_BUF_FLAG_IN_FENCE``
>> + - 0x00200000
>> + - Ask V4L2 to wait on fence passed in ``fence_fd`` field. The buffer
>> + won't be queued to the driver until the fence signals.
>> +
>> + * .. _`V4L2-BUF-FLAG-OUT-FENCE`:
>> +
>> + - ``V4L2_BUF_FLAG_OUT_FENCE``
>> + - 0x00400000
>> + - Request a fence for the next buffer to be queued to V4L2 driver.
>> + The fence received back through the ``fence_fd`` field doesn't
>> + necessarily relate to the current buffer in the
>> + :ref:`VIDIOC_QBUF <VIDIOC_QBUF>` ioctl. Although, most of the time
>> + the fence will relate to the current buffer it can't be guaranteed.
>> + So to tell userspace which buffer is associated to the out_fence,
>> + one should listen for the ``V4L2_EVENT_BUF_QUEUED`` event that
>> + provide the id of the buffer when it is queued to the V4L2 driver.
>
> As mentioned in the previous patch, this flag should just signal that userspace
> wants to receive a BUF_QUEUED event with the out-fence for this buffer. It's up
> to the driver to ensure the correct ordering of the events.
I'll wait for v4 before continuing my review.
Hans
>
> I'm missing documentation for the new fence_fd field.
>
> It should mention that fence_fd is ignored if the IN_FENCE isn't set. Applications
> will set fence_fd (aka reserved2) to 0, which should be fine as long as IN_FENCE
> isn't set.
>
>>
>>
>>
>> diff --git a/drivers/media/usb/cpia2/cpia2_v4l.c b/drivers/media/usb/cpia2/cpia2_v4l.c
>> index 3dedd83f0b19..6cde686bf44c 100644
>> --- a/drivers/media/usb/cpia2/cpia2_v4l.c
>> +++ b/drivers/media/usb/cpia2/cpia2_v4l.c
>> @@ -948,7 +948,7 @@ static int cpia2_dqbuf(struct file *file, void *fh, struct v4l2_buffer *buf)
>> buf->sequence = cam->buffers[buf->index].seq;
>> buf->m.offset = cam->buffers[buf->index].data - cam->frame_buffer;
>> buf->length = cam->frame_size;
>> - buf->reserved2 = 0;
>> + buf->fence_fd = -1;
>> buf->reserved = 0;
>> memset(&buf->timecode, 0, sizeof(buf->timecode));
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
>> index 821f2aa299ae..d624fb5df130 100644
>> --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
>> +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
>> @@ -370,7 +370,7 @@ struct v4l2_buffer32 {
>> __s32 fd;
>> } m;
>> __u32 length;
>> - __u32 reserved2;
>> + __s32 fence_fd;
>> __u32 reserved;
>> };
>>
>> @@ -533,8 +533,8 @@ static int put_v4l2_buffer32(struct v4l2_buffer *kp, struct v4l2_buffer32 __user
>> put_user(kp->timestamp.tv_usec, &up->timestamp.tv_usec) ||
>> copy_to_user(&up->timecode, &kp->timecode, sizeof(struct v4l2_timecode)) ||
>> put_user(kp->sequence, &up->sequence) ||
>> - put_user(kp->reserved2, &up->reserved2) ||
>> put_user(kp->reserved, &up->reserved) ||
>> + put_user(kp->fence_fd, &up->fence_fd) ||
>
> Could you move this up one line? It's easier to parse this diff if it is clear
> that fence_fd replaced reserved2.
>
>> put_user(kp->length, &up->length))
>> return -EFAULT;
>>
>> diff --git a/drivers/media/v4l2-core/videobuf2-v4l2.c b/drivers/media/v4l2-core/videobuf2-v4l2.c
>> index 0c0669976bdc..110fb45fef6f 100644
>> --- a/drivers/media/v4l2-core/videobuf2-v4l2.c
>> +++ b/drivers/media/v4l2-core/videobuf2-v4l2.c
>> @@ -203,7 +203,7 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, void *pb)
>> b->timestamp = ns_to_timeval(vb->timestamp);
>> b->timecode = vbuf->timecode;
>> b->sequence = vbuf->sequence;
>> - b->reserved2 = 0;
>> + b->fence_fd = -1;
>> b->reserved = 0;
>>
>> if (q->is_multiplanar) {
>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>> index 185d6a0acc06..e5abab9a908c 100644
>> --- a/include/uapi/linux/videodev2.h
>> +++ b/include/uapi/linux/videodev2.h
>> @@ -924,7 +924,7 @@ struct v4l2_buffer {
>> __s32 fd;
>> } m;
>> __u32 length;
>> - __u32 reserved2;
>> + __s32 fence_fd;
>> __u32 reserved;
>> };
>>
>> @@ -961,6 +961,8 @@ struct v4l2_buffer {
>> #define V4L2_BUF_FLAG_TSTAMP_SRC_SOE 0x00010000
>> /* mem2mem encoder/decoder */
>> #define V4L2_BUF_FLAG_LAST 0x00100000
>> +#define V4L2_BUF_FLAG_IN_FENCE 0x00200000
>> +#define V4L2_BUF_FLAG_OUT_FENCE 0x00400000
>>
>> /**
>> * struct v4l2_exportbuffer - export of video buffer as DMABUF file descriptor
>>
>
> Regards,
>
> Hans
>
2017-09-11 Hans Verkuil <[email protected]>:
> On 09/11/2017 12:50 PM, Hans Verkuil wrote:
> > On 09/07/2017 08:42 PM, Gustavo Padovan wrote:
> >> From: Gustavo Padovan <[email protected]>
> >>
> >> Add section to VIDIOC_QBUF about it
> >>
> >> v2:
> >> - mention that fences are files (Hans)
> >> - rework for the new API
> >>
> >> Signed-off-by: Gustavo Padovan <[email protected]>
> >> ---
> >> Documentation/media/uapi/v4l/vidioc-qbuf.rst | 31 ++++++++++++++++++++++++++++
> >> 1 file changed, 31 insertions(+)
> >>
> >> diff --git a/Documentation/media/uapi/v4l/vidioc-qbuf.rst b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
> >> index 1f3612637200..fae0b1431672 100644
> >> --- a/Documentation/media/uapi/v4l/vidioc-qbuf.rst
> >> +++ b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
> >> @@ -117,6 +117,37 @@ immediately with an ``EAGAIN`` error code when no buffer is available.
> >> The struct :c:type:`v4l2_buffer` structure is specified in
> >> :ref:`buffer`.
> >>
> >> +Explicit Synchronization
> >> +------------------------
> >> +
> >> +Explicit Synchronization allows us to control the synchronization of
> >> +shared buffers from userspace by passing fences to the kernel and/or
> >> +receiving them from it. Fences passed to the kernel are named in-fences and
> >> +the kernel should wait them to signal before using the buffer, i.e., queueing
> >
> > wait them -> wait on them
> >
> > (do you wait 'on' a fence or 'for' a fence? I think it's 'on' but I'm not 100% sure)
> >
> >> +it to the driver. On the other side, the kernel can create out-fences for the
> >> +buffers it queues to the drivers, out-fences signal when the driver is
> >
> > Start a new sentence here: ...drivers. Out-fences...
> >
> >> +finished with buffer, that is the buffer is ready. The fence are represented
> >
> > s/that is/i.e/
> >
> > s/The fence/The fences/
> >
> >> +by file and passed as file descriptor to userspace.
> >
> > s/by file/as a file/
> > s/as file/as a file/
> >
> >> +
> >> +The in-fences are communicated to the kernel at the ``VIDIOC_QBUF`` ioctl
> >> +using the ``V4L2_BUF_FLAG_IN_FENCE`` buffer
> >> +flags and the `fence_fd` field. If an in-fence needs to be passed to the kernel,
> >> +`fence_fd` should be set to the fence file descriptor number and the
> >> +``V4L2_BUF_FLAG_IN_FENCE`` should be set as well. Failure to set both will
> >
> > s/Failure to set both/Setting one but not the other/
> >
> >> +cause ``VIDIOC_QBUF`` to return with error.
> >> +
> >> +To get a out-fence back from V4L2 the ``V4L2_BUF_FLAG_OUT_FENCE`` flag should
> >> +be set to notify it that the next queued buffer should have a fence attached to
> >> +it. That means the out-fence may not be associated with the buffer in the
> >> +current ``VIDIOC_QBUF`` ioctl call because the ordering in which videobuf2 core
> >> +queues the buffers to the drivers can't be guaranteed. To become aware of the
> >> +of the next queued buffer and the out-fence attached to it the
> >> +``V4L2_EVENT_BUF_QUEUED`` event should be used. It will trigger an event
> >> +for every buffer queued to the V4L2 driver.
> >
> > This makes no sense.
> >
> > Setting this flag means IMHO that when *this* buffer is queued up to the driver,
> > then it should send the BUF_QUEUED event with an out fence.
> >
> > I.e. it signals that userspace wants to have the out-fence. The requirement w.r.t.
> > ordering is that the BUF_QUEUED events have to be in order, but that is something
> > that the driver can ensure in the case it is doing internal re-ordering.
> >
> > This requirement is something that needs to be documented here, BTW.
> >
> > Anyway, the flag shouldn't refer to some 'next buffer', since that's very confusing.
>
> Just ignore this comment. I assume v4 will implement it like this.
What approach do you mean by "like this". I'm confused now. :)
In fact, I was in doubt between these two different approaches here.
Should the flag mean *this* or the *next* buffer? The buffers can still
be reordered at the videobuf2 level, because they might be waiting on
in-fences and the fences may signal out of order. Then I went for the
*next* buffer approach because we don't know that buffer for sure.
But now thinking on this again we shouldn't have problems with the
*this* buffer approach also.
>
> Regards,
>
> Hans
>
> >
> >> +
> >> +At streamoff the out-fences will either signal normally if the drivers wait
> >
> > s/drivers wait/driver waits/
> >
> >> +for the operations on the buffers to finish or signal with error if the
> >> +driver cancel the pending operations.
> >
> > s/cancel/cancels/
> >
> > Thinking with my evil hat on:
> >
> > What happens if the application dequeues the buffer (VIDIOC_DQBUF) before
> > dequeuing the BUF_QUEUED event? Or if the application doesn't call VIDIOC_DQEVENT
> > at all? Should any pending BUF_QUEUED event with an out fence be removed from the
> > event queue if the application calls DQBUF on the corresponding buffer?
Good catch, we need to clean that up.
Gustavo
On 09/11/2017 03:18 PM, Gustavo Padovan wrote:
> 2017-09-11 Hans Verkuil <[email protected]>:
>
>> On 09/11/2017 12:50 PM, Hans Verkuil wrote:
>>> On 09/07/2017 08:42 PM, Gustavo Padovan wrote:
>>>> From: Gustavo Padovan <[email protected]>
>>>>
>>>> Add section to VIDIOC_QBUF about it
>>>>
>>>> v2:
>>>> - mention that fences are files (Hans)
>>>> - rework for the new API
>>>>
>>>> Signed-off-by: Gustavo Padovan <[email protected]>
>>>> ---
>>>> Documentation/media/uapi/v4l/vidioc-qbuf.rst | 31 ++++++++++++++++++++++++++++
>>>> 1 file changed, 31 insertions(+)
>>>>
>>>> diff --git a/Documentation/media/uapi/v4l/vidioc-qbuf.rst b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
>>>> index 1f3612637200..fae0b1431672 100644
>>>> --- a/Documentation/media/uapi/v4l/vidioc-qbuf.rst
>>>> +++ b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
>>>> @@ -117,6 +117,37 @@ immediately with an ``EAGAIN`` error code when no buffer is available.
>>>> The struct :c:type:`v4l2_buffer` structure is specified in
>>>> :ref:`buffer`.
>>>>
>>>> +Explicit Synchronization
>>>> +------------------------
>>>> +
>>>> +Explicit Synchronization allows us to control the synchronization of
>>>> +shared buffers from userspace by passing fences to the kernel and/or
>>>> +receiving them from it. Fences passed to the kernel are named in-fences and
>>>> +the kernel should wait them to signal before using the buffer, i.e., queueing
>>>
>>> wait them -> wait on them
>>>
>>> (do you wait 'on' a fence or 'for' a fence? I think it's 'on' but I'm not 100% sure)
>>>
>>>> +it to the driver. On the other side, the kernel can create out-fences for the
>>>> +buffers it queues to the drivers, out-fences signal when the driver is
>>>
>>> Start a new sentence here: ...drivers. Out-fences...
>>>
>>>> +finished with buffer, that is the buffer is ready. The fence are represented
>>>
>>> s/that is/i.e/
>>>
>>> s/The fence/The fences/
>>>
>>>> +by file and passed as file descriptor to userspace.
>>>
>>> s/by file/as a file/
>>> s/as file/as a file/
>>>
>>>> +
>>>> +The in-fences are communicated to the kernel at the ``VIDIOC_QBUF`` ioctl
>>>> +using the ``V4L2_BUF_FLAG_IN_FENCE`` buffer
>>>> +flags and the `fence_fd` field. If an in-fence needs to be passed to the kernel,
>>>> +`fence_fd` should be set to the fence file descriptor number and the
>>>> +``V4L2_BUF_FLAG_IN_FENCE`` should be set as well. Failure to set both will
>>>
>>> s/Failure to set both/Setting one but not the other/
>>>
>>>> +cause ``VIDIOC_QBUF`` to return with error.
>>>> +
>>>> +To get a out-fence back from V4L2 the ``V4L2_BUF_FLAG_OUT_FENCE`` flag should
>>>> +be set to notify it that the next queued buffer should have a fence attached to
>>>> +it. That means the out-fence may not be associated with the buffer in the
>>>> +current ``VIDIOC_QBUF`` ioctl call because the ordering in which videobuf2 core
>>>> +queues the buffers to the drivers can't be guaranteed. To become aware of the
>>>> +of the next queued buffer and the out-fence attached to it the
>>>> +``V4L2_EVENT_BUF_QUEUED`` event should be used. It will trigger an event
>>>> +for every buffer queued to the V4L2 driver.
>>>
>>> This makes no sense.
>>>
>>> Setting this flag means IMHO that when *this* buffer is queued up to the driver,
>>> then it should send the BUF_QUEUED event with an out fence.
>>>
>>> I.e. it signals that userspace wants to have the out-fence. The requirement w.r.t.
>>> ordering is that the BUF_QUEUED events have to be in order, but that is something
>>> that the driver can ensure in the case it is doing internal re-ordering.
>>>
>>> This requirement is something that needs to be documented here, BTW.
>>>
>>> Anyway, the flag shouldn't refer to some 'next buffer', since that's very confusing.
>>
>> Just ignore this comment. I assume v4 will implement it like this.
>
> What approach do you mean by "like this". I'm confused now. :)
>
> In fact, I was in doubt between these two different approaches here.
> Should the flag mean *this* or the *next* buffer? The buffers can still
> be reordered at the videobuf2 level, because they might be waiting on
> in-fences and the fences may signal out of order. Then I went for the
> *next* buffer approach because we don't know that buffer for sure.
> But now thinking on this again we shouldn't have problems with the
> *this* buffer approach also.
It should mean *this* buffer. It's really weird to set this flag for one
buffer, only for it to mean 'next' buffer.
Keep it simple: the flag just means: send me the output fence fd for this
buffer once you have it. If it is not set, then no BUF_QUEUE event is sent.
Actually, it could mean one of two things: either if it is not set, then no
BUF_QUEUE event is sent, or if it is not set, then the fd in the BUF_QUEUE
event is -1.
I'm leaning towards the first. I can't see any use-case for sending that
event if you are not requesting out fences.
Regards,
Hans
>
>>
>> Regards,
>>
>> Hans
>>
>>>
>>>> +
>>>> +At streamoff the out-fences will either signal normally if the drivers wait
>>>
>>> s/drivers wait/driver waits/
>>>
>>>> +for the operations on the buffers to finish or signal with error if the
>>>> +driver cancel the pending operations.
>>>
>>> s/cancel/cancels/
>>>
>>> Thinking with my evil hat on:
>>>
>>> What happens if the application dequeues the buffer (VIDIOC_DQBUF) before
>>> dequeuing the BUF_QUEUED event? Or if the application doesn't call VIDIOC_DQEVENT
>>> at all? Should any pending BUF_QUEUED event with an out fence be removed from the
>>> event queue if the application calls DQBUF on the corresponding buffer?
>
> Good catch, we need to clean that up.
>
> Gustavo
>
2017-09-11 Hans Verkuil <[email protected]>:
> On 09/11/2017 03:18 PM, Gustavo Padovan wrote:
> > 2017-09-11 Hans Verkuil <[email protected]>:
> >
> >> On 09/11/2017 12:50 PM, Hans Verkuil wrote:
> >>> On 09/07/2017 08:42 PM, Gustavo Padovan wrote:
> >>>> From: Gustavo Padovan <[email protected]>
> >>>>
> >>>> Add section to VIDIOC_QBUF about it
> >>>>
> >>>> v2:
> >>>> - mention that fences are files (Hans)
> >>>> - rework for the new API
> >>>>
> >>>> Signed-off-by: Gustavo Padovan <[email protected]>
> >>>> ---
> >>>> Documentation/media/uapi/v4l/vidioc-qbuf.rst | 31 ++++++++++++++++++++++++++++
> >>>> 1 file changed, 31 insertions(+)
> >>>>
> >>>> diff --git a/Documentation/media/uapi/v4l/vidioc-qbuf.rst b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
> >>>> index 1f3612637200..fae0b1431672 100644
> >>>> --- a/Documentation/media/uapi/v4l/vidioc-qbuf.rst
> >>>> +++ b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
> >>>> @@ -117,6 +117,37 @@ immediately with an ``EAGAIN`` error code when no buffer is available.
> >>>> The struct :c:type:`v4l2_buffer` structure is specified in
> >>>> :ref:`buffer`.
> >>>>
> >>>> +Explicit Synchronization
> >>>> +------------------------
> >>>> +
> >>>> +Explicit Synchronization allows us to control the synchronization of
> >>>> +shared buffers from userspace by passing fences to the kernel and/or
> >>>> +receiving them from it. Fences passed to the kernel are named in-fences and
> >>>> +the kernel should wait them to signal before using the buffer, i.e., queueing
> >>>
> >>> wait them -> wait on them
> >>>
> >>> (do you wait 'on' a fence or 'for' a fence? I think it's 'on' but I'm not 100% sure)
> >>>
> >>>> +it to the driver. On the other side, the kernel can create out-fences for the
> >>>> +buffers it queues to the drivers, out-fences signal when the driver is
> >>>
> >>> Start a new sentence here: ...drivers. Out-fences...
> >>>
> >>>> +finished with buffer, that is the buffer is ready. The fence are represented
> >>>
> >>> s/that is/i.e/
> >>>
> >>> s/The fence/The fences/
> >>>
> >>>> +by file and passed as file descriptor to userspace.
> >>>
> >>> s/by file/as a file/
> >>> s/as file/as a file/
> >>>
> >>>> +
> >>>> +The in-fences are communicated to the kernel at the ``VIDIOC_QBUF`` ioctl
> >>>> +using the ``V4L2_BUF_FLAG_IN_FENCE`` buffer
> >>>> +flags and the `fence_fd` field. If an in-fence needs to be passed to the kernel,
> >>>> +`fence_fd` should be set to the fence file descriptor number and the
> >>>> +``V4L2_BUF_FLAG_IN_FENCE`` should be set as well. Failure to set both will
> >>>
> >>> s/Failure to set both/Setting one but not the other/
> >>>
> >>>> +cause ``VIDIOC_QBUF`` to return with error.
> >>>> +
> >>>> +To get a out-fence back from V4L2 the ``V4L2_BUF_FLAG_OUT_FENCE`` flag should
> >>>> +be set to notify it that the next queued buffer should have a fence attached to
> >>>> +it. That means the out-fence may not be associated with the buffer in the
> >>>> +current ``VIDIOC_QBUF`` ioctl call because the ordering in which videobuf2 core
> >>>> +queues the buffers to the drivers can't be guaranteed. To become aware of the
> >>>> +of the next queued buffer and the out-fence attached to it the
> >>>> +``V4L2_EVENT_BUF_QUEUED`` event should be used. It will trigger an event
> >>>> +for every buffer queued to the V4L2 driver.
> >>>
> >>> This makes no sense.
> >>>
> >>> Setting this flag means IMHO that when *this* buffer is queued up to the driver,
> >>> then it should send the BUF_QUEUED event with an out fence.
> >>>
> >>> I.e. it signals that userspace wants to have the out-fence. The requirement w.r.t.
> >>> ordering is that the BUF_QUEUED events have to be in order, but that is something
> >>> that the driver can ensure in the case it is doing internal re-ordering.
> >>>
> >>> This requirement is something that needs to be documented here, BTW.
> >>>
> >>> Anyway, the flag shouldn't refer to some 'next buffer', since that's very confusing.
> >>
> >> Just ignore this comment. I assume v4 will implement it like this.
> >
> > What approach do you mean by "like this". I'm confused now. :)
> >
> > In fact, I was in doubt between these two different approaches here.
> > Should the flag mean *this* or the *next* buffer? The buffers can still
> > be reordered at the videobuf2 level, because they might be waiting on
> > in-fences and the fences may signal out of order. Then I went for the
> > *next* buffer approach because we don't know that buffer for sure.
> > But now thinking on this again we shouldn't have problems with the
> > *this* buffer approach also.
>
> It should mean *this* buffer. It's really weird to set this flag for one
> buffer, only for it to mean 'next' buffer.
>
> Keep it simple: the flag just means: send me the output fence fd for this
> buffer once you have it. If it is not set, then no BUF_QUEUE event is sent.
>
> Actually, it could mean one of two things: either if it is not set, then no
> BUF_QUEUE event is sent, or if it is not set, then the fd in the BUF_QUEUE
> event is -1.
>
> I'm leaning towards the first. I can't see any use-case for sending that
> event if you are not requesting out fences.
We could go with the first one but in this case it is better to rename it to
V4L2_EVENT_OUT_FENCE or something like this, isn't it?
Gustavo
On 09/11/2017 03:34 PM, Gustavo Padovan wrote:
> 2017-09-11 Hans Verkuil <[email protected]>:
>
>> On 09/11/2017 03:18 PM, Gustavo Padovan wrote:
>>> 2017-09-11 Hans Verkuil <[email protected]>:
>>>
>>>> On 09/11/2017 12:50 PM, Hans Verkuil wrote:
>>>>> On 09/07/2017 08:42 PM, Gustavo Padovan wrote:
>>>>>> From: Gustavo Padovan <[email protected]>
>>>>>>
>>>>>> Add section to VIDIOC_QBUF about it
>>>>>>
>>>>>> v2:
>>>>>> - mention that fences are files (Hans)
>>>>>> - rework for the new API
>>>>>>
>>>>>> Signed-off-by: Gustavo Padovan <[email protected]>
>>>>>> ---
>>>>>> Documentation/media/uapi/v4l/vidioc-qbuf.rst | 31 ++++++++++++++++++++++++++++
>>>>>> 1 file changed, 31 insertions(+)
>>>>>>
>>>>>> diff --git a/Documentation/media/uapi/v4l/vidioc-qbuf.rst b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
>>>>>> index 1f3612637200..fae0b1431672 100644
>>>>>> --- a/Documentation/media/uapi/v4l/vidioc-qbuf.rst
>>>>>> +++ b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
>>>>>> @@ -117,6 +117,37 @@ immediately with an ``EAGAIN`` error code when no buffer is available.
>>>>>> The struct :c:type:`v4l2_buffer` structure is specified in
>>>>>> :ref:`buffer`.
>>>>>>
>>>>>> +Explicit Synchronization
>>>>>> +------------------------
>>>>>> +
>>>>>> +Explicit Synchronization allows us to control the synchronization of
>>>>>> +shared buffers from userspace by passing fences to the kernel and/or
>>>>>> +receiving them from it. Fences passed to the kernel are named in-fences and
>>>>>> +the kernel should wait them to signal before using the buffer, i.e., queueing
>>>>>
>>>>> wait them -> wait on them
>>>>>
>>>>> (do you wait 'on' a fence or 'for' a fence? I think it's 'on' but I'm not 100% sure)
>>>>>
>>>>>> +it to the driver. On the other side, the kernel can create out-fences for the
>>>>>> +buffers it queues to the drivers, out-fences signal when the driver is
>>>>>
>>>>> Start a new sentence here: ...drivers. Out-fences...
>>>>>
>>>>>> +finished with buffer, that is the buffer is ready. The fence are represented
>>>>>
>>>>> s/that is/i.e/
>>>>>
>>>>> s/The fence/The fences/
>>>>>
>>>>>> +by file and passed as file descriptor to userspace.
>>>>>
>>>>> s/by file/as a file/
>>>>> s/as file/as a file/
>>>>>
>>>>>> +
>>>>>> +The in-fences are communicated to the kernel at the ``VIDIOC_QBUF`` ioctl
>>>>>> +using the ``V4L2_BUF_FLAG_IN_FENCE`` buffer
>>>>>> +flags and the `fence_fd` field. If an in-fence needs to be passed to the kernel,
>>>>>> +`fence_fd` should be set to the fence file descriptor number and the
>>>>>> +``V4L2_BUF_FLAG_IN_FENCE`` should be set as well. Failure to set both will
>>>>>
>>>>> s/Failure to set both/Setting one but not the other/
>>>>>
>>>>>> +cause ``VIDIOC_QBUF`` to return with error.
>>>>>> +
>>>>>> +To get a out-fence back from V4L2 the ``V4L2_BUF_FLAG_OUT_FENCE`` flag should
>>>>>> +be set to notify it that the next queued buffer should have a fence attached to
>>>>>> +it. That means the out-fence may not be associated with the buffer in the
>>>>>> +current ``VIDIOC_QBUF`` ioctl call because the ordering in which videobuf2 core
>>>>>> +queues the buffers to the drivers can't be guaranteed. To become aware of the
>>>>>> +of the next queued buffer and the out-fence attached to it the
>>>>>> +``V4L2_EVENT_BUF_QUEUED`` event should be used. It will trigger an event
>>>>>> +for every buffer queued to the V4L2 driver.
>>>>>
>>>>> This makes no sense.
>>>>>
>>>>> Setting this flag means IMHO that when *this* buffer is queued up to the driver,
>>>>> then it should send the BUF_QUEUED event with an out fence.
>>>>>
>>>>> I.e. it signals that userspace wants to have the out-fence. The requirement w.r.t.
>>>>> ordering is that the BUF_QUEUED events have to be in order, but that is something
>>>>> that the driver can ensure in the case it is doing internal re-ordering.
>>>>>
>>>>> This requirement is something that needs to be documented here, BTW.
>>>>>
>>>>> Anyway, the flag shouldn't refer to some 'next buffer', since that's very confusing.
>>>>
>>>> Just ignore this comment. I assume v4 will implement it like this.
>>>
>>> What approach do you mean by "like this". I'm confused now. :)
>>>
>>> In fact, I was in doubt between these two different approaches here.
>>> Should the flag mean *this* or the *next* buffer? The buffers can still
>>> be reordered at the videobuf2 level, because they might be waiting on
>>> in-fences and the fences may signal out of order. Then I went for the
>>> *next* buffer approach because we don't know that buffer for sure.
>>> But now thinking on this again we shouldn't have problems with the
>>> *this* buffer approach also.
>>
>> It should mean *this* buffer. It's really weird to set this flag for one
>> buffer, only for it to mean 'next' buffer.
>>
>> Keep it simple: the flag just means: send me the output fence fd for this
>> buffer once you have it. If it is not set, then no BUF_QUEUE event is sent.
>>
>> Actually, it could mean one of two things: either if it is not set, then no
>> BUF_QUEUE event is sent, or if it is not set, then the fd in the BUF_QUEUE
>> event is -1.
>>
>> I'm leaning towards the first. I can't see any use-case for sending that
>> event if you are not requesting out fences.
>
> We could go with the first one but in this case it is better to rename it to
> V4L2_EVENT_OUT_FENCE or something like this, isn't it?
I was thinking the same thing. That would be a better name, yes.
Regards,
Hans