[RFC PATCH 0/8] [media] Request API, take three

[RFC PATCH 0/8] [media] Request API, take three

[Alexandre Courbot]

Howdy. Here is your bi-weekly request API redesign! ;)

Again, this is a simple version that only implements the flow of requests,
without applying controls. The intent is to get an agreement on a base to work
on, since the previous versions went straight back to the redesign board.

Highlights of this version:

* As requested by Hans, request-bound buffers are now passed earlier to drivers,
as early as the request itself is submitted. Doing it earlier is not be useful
since the driver would not know the state of the request, and thus cannot do
anything with the buffer. Drivers are now responsible for applying request
parameters themselves.

* As a consequence, there is no such thing as a "request queue" anymore. The
flow of buffers decides the order in which requests are processed. Individual
devices of the same pipeline can implement synchronization if needed, but this
is beyond this first version.

* VB2 code is still a bit shady. Some of it will interfere with the fences
series, so I am waiting for the latter to land to do it properly.

* Requests that are not yet submitted effectively act as fences on the buffers
they own, resulting in the buffer queue being blocked until the request is
submitted. An alternate design would be to only block the
not-submitted-request's buffer and let further buffers pass before it, but since
buffer order is becoming increasingly important I have decided to just block the
queue. This is open to discussion though.

* Documentation! Also described usecases for codec and simple (i.e. not part of
a complex pipeline) capture device.

Still remaining to do:

* As pointed out by Hans on the previous revision, do not assume that drivers
using v4l2_fh have a per-handle state. I have not yet found a good way to
differenciate both usages.

* Integrate Hans' patchset for control handling: as said above, this is futile
unless we can agree on the basic design, which I hope we can do this time.
Chrome OS needs this really badly now and will have to move forward no matter
what, so I hope this will be considered good enough for a common base of work.

A few thoughts/questions that emerged when writing this patchset:

* Since requests are exposed as file descriptors, we could easily move the
MEDIA_REQ_CMD_SUBMIT and MEDIA_REQ_CMD_REININT commands as ioctls on the
requests themselves, instead of having to perform them on the media device that
provided the request in the first place. That would add a bit more flexibility
if/when passing requests around and means the media device only needs to handle
MEDIA_REQ_CMD_ALLOC. Conceptually speaking, this seems to make sense to me.
Any comment for/against that?

* For the codec usecase, I have experimented a bit marking CAPTURE buffers with
the request FD that produced them (vim2m acts that way). This seems to work
well, however FDs are process-local and could be closed before the CAPTURE
buffer is dequeued, rendering that information less meaningful, if not
dangerous. Wouldn't it be better/safer to use a global request ID for
such information instead? That ID would be returned upon MEDIA_REQ_CMD_ALLOC so
user-space knows which request ID a FD refers to.

* Using the media node to provide requests makes absolute sense for complex
camera devices, but is tedious for codec devices which work on one node and
require to protect request/media related code with #ifdef
CONFIG_MEDIA_CONTROLLER. For these devices, the sole role of the media node is
to produce the request, and that's it (since request submission could be moved
to the request FD as explained above). That's a modest use that hardly justifies
bringing in the whole media framework IMHO. With a bit of extra abstraction, it
should be possible to decouple the base requests from the media controller
altogether, and propose two kinds of requests implementations: one simpler
implementation that works directly with a single V4L2 node (useful for codecs),
and another one that works on a media node and can control all its entities
(good for camera). This would allow codecs to use the request API without
forcing the media controller, and would considerably simplify the use-case. Any
objection to that? IIRC the earlier design documents mentioned this possibility.

Alexandre Courbot (6):
  media: add request API core and UAPI
  media: videobuf2: add support for requests
  media: vb2: add support for requests in QBUF ioctl
  v4l2: document the request API interface
  media: vim2m: add media device
  media: vim2m: add request support

Hans Verkuil (1):
  videodev2.h: Add request field to v4l2_buffer

Laurent Pinchart (1):
  media: Document the media request API

 Documentation/media/uapi/mediactl/media-funcs.rst  |   1 +
 .../media/uapi/mediactl/media-ioc-request-cmd.rst  | 140 ++++++++++
 Documentation/media/uapi/v4l/buffer.rst            |  10 +-
 Documentation/media/uapi/v4l/common.rst            |   1 +
 Documentation/media/uapi/v4l/request-api.rst       | 194 +++++++++++++
 Documentation/media/uapi/v4l/vidioc-qbuf.rst       |  21 ++
 drivers/media/Makefile                             |   3 +-
 drivers/media/media-device.c                       |   7 +
 drivers/media/media-request-mgr.c                  | 107 +++++++
 drivers/media/media-request.c                      | 308 +++++++++++++++++++++
 drivers/media/platform/vim2m.c                     |  55 ++++
 drivers/media/usb/cpia2/cpia2_v4l.c                |   2 +-
 drivers/media/v4l2-core/v4l2-compat-ioctl32.c      |   7 +-
 drivers/media/v4l2-core/v4l2-ioctl.c               |  85 +++++-
 drivers/media/v4l2-core/videobuf2-core.c           | 125 ++++++++-
 drivers/media/v4l2-core/videobuf2-v4l2.c           |  31 ++-
 include/media/media-device.h                       |   3 +
 include/media/media-request-mgr.h                  |  73 +++++
 include/media/media-request.h                      | 184 ++++++++++++
 include/media/videobuf2-core.h                     |  15 +-
 include/media/videobuf2-v4l2.h                     |   2 +
 include/uapi/linux/media.h                         |  10 +
 include/uapi/linux/videodev2.h                     |   3 +-
 23 files changed, 1365 insertions(+), 22 deletions(-)
 create mode 100644 Documentation/media/uapi/mediactl/media-ioc-request-cmd.rst
 create mode 100644 Documentation/media/uapi/v4l/request-api.rst
 create mode 100644 drivers/media/media-request-mgr.c
 create mode 100644 drivers/media/media-request.c
 create mode 100644 include/media/media-request-mgr.h
 create mode 100644 include/media/media-request.h

-- 
2.16.0.rc1.238.g530d649a79-goog

[RFC PATCH 1/8] media: add request API core and UAPI

[Alexandre Courbot]

The request API provides a way to group buffers and device parameters
into units of work to be queued and executed. This patch introduces the
UAPI and core framework.

This patch is based on the previous work by Laurent Pinchart. The core
has changed considerably, but the UAPI is mostly untouched.

Signed-off-by: Alexandre Courbot <acourbot@chromium.org>
---
 drivers/media/Makefile               |   3 +-
 drivers/media/media-device.c         |   7 +
 drivers/media/media-request-mgr.c    | 107 ++++++++++++
 drivers/media/media-request.c        | 308 +++++++++++++++++++++++++++++++++++
 drivers/media/v4l2-core/v4l2-ioctl.c |   2 +-
 include/media/media-device.h         |   3 +
 include/media/media-request-mgr.h    |  73 +++++++++
 include/media/media-request.h        | 184 +++++++++++++++++++++
 include/uapi/linux/media.h           |  10 ++
 9 files changed, 695 insertions(+), 2 deletions(-)
 create mode 100644 drivers/media/media-request-mgr.c
 create mode 100644 drivers/media/media-request.c
 create mode 100644 include/media/media-request-mgr.h
 create mode 100644 include/media/media-request.h

diff --git a/drivers/media/Makefile b/drivers/media/Makefile
index 594b462ddf0e..06c43ddb52ea 100644
--- a/drivers/media/Makefile
+++ b/drivers/media/Makefile
@@ -3,7 +3,8 @@
 # Makefile for the kernel multimedia device drivers.
 #
 
-media-objs	:= media-device.o media-devnode.o media-entity.o
+media-objs	:= media-device.o media-devnode.o media-entity.o \
+		   media-request.o media-request-mgr.o
 
 #
 # I2C drivers should come before other drivers, otherwise they'll fail
diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index e79f72b8b858..024ee81a8334 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -32,6 +32,8 @@
 #include <media/media-device.h>
 #include <media/media-devnode.h>
 #include <media/media-entity.h>
+#include <media/media-request.h>
+#include <media/media-request-mgr.h>
 
 #ifdef CONFIG_MEDIA_CONTROLLER
 
@@ -407,6 +409,7 @@ static const struct media_ioctl_info ioctl_info[] = {
 	MEDIA_IOC(ENUM_LINKS, media_device_enum_links, MEDIA_IOC_FL_GRAPH_MUTEX),
 	MEDIA_IOC(SETUP_LINK, media_device_setup_link, MEDIA_IOC_FL_GRAPH_MUTEX),
 	MEDIA_IOC(G_TOPOLOGY, media_device_get_topology, MEDIA_IOC_FL_GRAPH_MUTEX),
+	MEDIA_IOC(REQUEST_CMD, media_device_request_cmd, 0),
 };
 
 static long media_device_ioctl(struct file *filp, unsigned int cmd,
@@ -688,6 +691,10 @@ EXPORT_SYMBOL_GPL(media_device_init);
 
 void media_device_cleanup(struct media_device *mdev)
 {
+	if (mdev->req_mgr) {
+		media_request_mgr_free(mdev->req_mgr);
+		mdev->req_mgr = NULL;
+	}
 	ida_destroy(&mdev->entity_internal_idx);
 	mdev->entity_internal_idx_max = 0;
 	media_graph_walk_cleanup(&mdev->pm_count_walk);
diff --git a/drivers/media/media-request-mgr.c b/drivers/media/media-request-mgr.c
new file mode 100644
index 000000000000..2644b535577e
--- /dev/null
+++ b/drivers/media/media-request-mgr.c
@@ -0,0 +1,107 @@
+/*
+ * Generic request manager implementation.
+ *
+ * Copyright (C) 2018, The Chromium OS Authors.  All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/slab.h>
+
+#include <media/media-request.h>
+#include <media/media-request-mgr.h>
+#include <media/media-device.h>
+#include <linux/device.h>
+
+static struct media_request *
+media_request_alloc(struct media_request_mgr *mgr)
+{
+	struct media_request *req;
+
+	req = kzalloc(sizeof(*req), GFP_KERNEL);
+	if (!req)
+		return ERR_PTR(-ENOMEM);
+
+	req->mgr = mgr;
+	req->state = MEDIA_REQUEST_STATE_IDLE;
+	kref_init(&req->kref);
+	INIT_LIST_HEAD(&req->data);
+	init_waitqueue_head(&req->complete_wait);
+	ATOMIC_INIT_NOTIFIER_HEAD(&req->submit_notif);
+	mutex_init(&req->lock);
+
+	mutex_lock(&mgr->mutex);
+	req->id = ++mgr->req_id;
+	list_add_tail(&req->list, &mgr->requests);
+	mutex_unlock(&mgr->mutex);
+
+	return req;
+}
+
+static void media_request_free(struct media_request *req)
+{
+	struct media_request_mgr *mgr = req->mgr;
+	struct media_request_entity_data *data, *next;
+
+	mutex_lock(&mgr->mutex);
+	list_del(&req->list);
+	mutex_unlock(&mgr->mutex);
+
+	list_for_each_entry_safe(data, next, &req->data, list) {
+		list_del(&data->list);
+		kfree(data);
+	}
+
+	kfree(req);
+}
+
+void media_request_mgr_free(struct media_request_mgr *mgr)
+{
+	struct media_device *mdev = mgr->mdev;
+
+	mutex_lock(&mgr->mutex);
+	/* Just a sanity check - we should have no remaining requests */
+	while (!list_empty(&mgr->requests)) {
+		struct media_request *req;
+
+		req = list_first_entry(&mgr->requests, typeof(*req), list);
+		dev_warn(mdev->dev,
+			"%s: request %u still referenced, deleting forcibly!\n",
+			__func__, req->id);
+		mgr->ops->req_free(req);
+	}
+	mutex_unlock(&mgr->mutex);
+
+	kfree(mgr);
+}
+EXPORT_SYMBOL_GPL(media_request_mgr_free);
+
+static const struct media_request_mgr_ops request_mgr_generic_ops = {
+	.req_alloc = media_request_alloc,
+	.req_free = media_request_free,
+};
+
+struct media_request_mgr *
+media_request_mgr_alloc(struct media_device *mdev)
+{
+	struct media_request_mgr *mgr;
+
+	mgr = kzalloc(sizeof(*mgr), GFP_KERNEL);
+	if (!mgr)
+		return ERR_PTR(-ENOMEM);
+
+	mgr->mdev = mdev;
+	mutex_init(&mgr->mutex);
+	INIT_LIST_HEAD(&mgr->requests);
+	mgr->ops = &request_mgr_generic_ops;
+
+	return mgr;
+}
+EXPORT_SYMBOL_GPL(media_request_mgr_alloc);
diff --git a/drivers/media/media-request.c b/drivers/media/media-request.c
new file mode 100644
index 000000000000..42760d94990c
--- /dev/null
+++ b/drivers/media/media-request.c
@@ -0,0 +1,308 @@
+/*
+ * Request base implementation
+ *
+ * Copyright (C) 2018, The Chromium OS Authors.  All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/anon_inodes.h>
+#include <linux/media.h>
+#include <linux/fs.h>
+#include <linux/file.h>
+#include <linux/slab.h>
+#include <linux/list.h>
+
+#include <media/media-request.h>
+#include <media/media-request-mgr.h>
+#include <media/media-device.h>
+
+const struct file_operations request_fops;
+
+void media_request_get(struct media_request *req)
+{
+	kref_get(&req->kref);
+}
+EXPORT_SYMBOL_GPL(media_request_get);
+
+struct media_request *
+media_request_get_from_fd(int fd)
+{
+	struct file *f;
+	struct media_request *req;
+
+	f = fget(fd);
+	if (!f)
+		return NULL;
+
+	/* Not a request FD? */
+	if (f->f_op != &request_fops) {
+		fput(f);
+		return NULL;
+	}
+
+	req = f->private_data;
+	media_request_get(req);
+	fput(f);
+
+	return req;
+}
+EXPORT_SYMBOL_GPL(media_request_get_from_fd);
+
+static void media_request_release(struct kref *kref)
+{
+	struct media_request *req =
+		container_of(kref, typeof(*req), kref);
+
+	req->mgr->ops->req_free(req);
+}
+
+void media_request_put(struct media_request *req)
+{
+	kref_put(&req->kref, media_request_release);
+}
+EXPORT_SYMBOL_GPL(media_request_put);
+
+struct media_request_entity_data *
+media_request_get_entity_data(struct media_request *req,
+			      struct media_entity *entity, void *fh)
+{
+	struct media_request_entity_data *data;
+
+	mutex_lock(&req->lock);
+
+	/* First look whether we already have entity data */
+	list_for_each_entry(data, &req->data, list) {
+		if (data->entity == entity) {
+			/*
+			 * If so, then the fh must match otherwise this means
+			 * we are trying to set the same entity through
+			 * different handles
+			 */
+			if (data->fh != fh)
+				data = ERR_PTR(-EINVAL);
+			goto done;
+		}
+	}
+
+	/* No entity data found, let's create it */
+	data = kzalloc(sizeof(*data), GFP_KERNEL);
+	if (!data) {
+		data = ERR_PTR(-ENOMEM);
+		goto done;
+	}
+	data->fh = fh;
+	data->entity = entity;
+	list_add_tail(&data->list, &req->data);
+
+done:
+	mutex_unlock(&req->lock);
+	return data;
+}
+EXPORT_SYMBOL_GPL(media_request_get_entity_data);
+
+static const char * const media_request_states[] __maybe_unused = {
+	"IDLE",
+	"SUBMITTED",
+	"COMPLETED",
+	"DELETED",
+};
+
+static const char *media_request_state(enum media_request_state state)
+{
+	return state < ARRAY_SIZE(media_request_states) ?
+		media_request_states[state] : "INVALID";
+}
+
+static int media_device_request_close(struct inode *inode, struct file *filp)
+{
+	struct media_request *req = filp->private_data;
+
+	if (req == NULL)
+		return 0;
+
+	media_request_put(req);
+
+	return 0;
+}
+
+static unsigned int media_request_poll(struct file *file, poll_table *wait)
+{
+	struct media_request *req = file->private_data;
+
+	poll_wait(file, &req->complete_wait, wait);
+
+	if (req->state == MEDIA_REQUEST_STATE_COMPLETED)
+		return POLLIN | POLLRDNORM;
+
+	return 0;
+}
+
+const struct file_operations request_fops = {
+	.owner = THIS_MODULE,
+	.poll = media_request_poll,
+	.release = media_device_request_close,
+};
+
+void media_request_complete(struct media_request *req)
+{
+	struct media_request_mgr *mgr = req->mgr;
+	struct media_device *mdev = mgr->mdev;
+
+	mutex_lock(&req->lock);
+
+	if (WARN_ON(req->state != MEDIA_REQUEST_STATE_SUBMITTED)) {
+		dev_dbg(mdev->dev, "%s: can't complete %u, state %s\n",
+			__func__, req->id, media_request_state(req->state));
+		mutex_unlock(&req->lock);
+		return;
+	}
+
+	req->state = MEDIA_REQUEST_STATE_COMPLETED;
+
+	if (mgr->ops->req_complete)
+		mgr->ops->req_complete(req);
+
+	wake_up_interruptible(&req->complete_wait);
+
+	mutex_unlock(&req->lock);
+
+	/* Release the reference acquired when we submitted the request */
+	media_request_put(req);
+}
+EXPORT_SYMBOL_GPL(media_request_complete);
+
+/*
+ * Process the MEDIA_REQ_CMD_ALLOC command
+ */
+static int media_request_cmd_alloc(struct media_request_mgr *mgr,
+				   struct media_request_cmd *cmd)
+{
+	struct media_request *req;
+	int fd;
+
+	req = mgr->ops->req_alloc(mgr);
+	if (!req)
+		return -ENOMEM;
+
+	fd = anon_inode_getfd("media_request", &request_fops, req, O_CLOEXEC);
+	if (fd < 0)
+		return fd;
+
+	cmd->fd = fd;
+
+	return 0;
+}
+
+/*
+ * Process the MEDIA_REQ_CMD_SUBMIT command
+ */
+static int media_request_cmd_submit(struct media_request *req)
+{
+	mutex_lock(&req->lock);
+
+	if (req->state != MEDIA_REQUEST_STATE_IDLE) {
+		dev_dbg(req->mgr->mdev->dev,
+			"%s: unable to submit request in state %s\n",
+			__func__, media_request_state(req->state));
+		mutex_unlock(&req->lock);
+		return -EINVAL;
+	}
+
+	if (atomic_read(&req->buf_cpt) == 0) {
+		dev_dbg(req->mgr->mdev->dev,
+			"%s: request has no buffers!\n", __func__);
+		mutex_unlock(&req->lock);
+		return -EINVAL;
+	}
+
+	req->state = MEDIA_REQUEST_STATE_SUBMITTED;
+	mutex_unlock(&req->lock);
+
+	/* Holds a reference to the request until it is completed */
+	media_request_get(req);
+
+	atomic_notifier_call_chain(&req->submit_notif, req->state, req);
+
+	return 0;
+}
+
+static int media_request_cmd_reinit(struct media_request *req)
+{
+	struct media_request_entity_data *data, *next;
+
+	mutex_lock(&req->lock);
+
+	if (req->state == MEDIA_REQUEST_STATE_SUBMITTED) {
+		dev_dbg(req->mgr->mdev->dev,
+			"%s: unable to reinit submitted request\n", __func__);
+		mutex_unlock(&req->lock);
+		return -EINVAL;
+	}
+
+	/* delete all entity data */
+	list_for_each_entry_safe(data, next, &req->data, list) {
+		list_del(&data->list);
+		kfree(data);
+	}
+
+	/* reinitialize request to idle state */
+	req->state = MEDIA_REQUEST_STATE_IDLE;
+	atomic_set(&req->buf_cpt, 0);
+
+	mutex_unlock(&req->lock);
+
+	return 0;
+}
+
+long media_device_request_cmd(struct media_device *mdev,
+			      struct media_request_cmd *cmd)
+{
+	struct media_request *req = NULL;
+	int ret;
+
+	if (!mdev->req_mgr)
+		return -ENOTTY;
+
+	if (cmd->cmd != MEDIA_REQ_CMD_ALLOC) {
+		req = media_request_get_from_fd(cmd->fd);
+		if (IS_ERR(req))
+			return PTR_ERR(req);
+
+		/* requests must belong to this media device's manager */
+		if (req->mgr != mdev->req_mgr) {
+			media_request_put(req);
+			return -EINVAL;
+		}
+	}
+
+	switch (cmd->cmd) {
+	case MEDIA_REQ_CMD_ALLOC:
+		ret = media_request_cmd_alloc(mdev->req_mgr, cmd);
+		break;
+
+	case MEDIA_REQ_CMD_SUBMIT:
+		ret = media_request_cmd_submit(req);
+		break;
+
+	case MEDIA_REQ_CMD_REINIT:
+		ret = media_request_cmd_reinit(req);
+		break;
+
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	if (req)
+		media_request_put(req);
+
+	return ret;
+}
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index b60a6b0841d1..ec4ecd5aa8bf 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -867,7 +867,7 @@ static int check_ext_ctrls(struct v4l2_ext_controls *c, int allow_priv)
 	__u32 i;
 
 	/* zero the reserved fields */
-	c->reserved[0] = c->reserved[1] = 0;
+	c->reserved[0] = 0;
 	for (i = 0; i < c->count; i++)
 		c->controls[i].reserved2[0] = 0;
 
diff --git a/include/media/media-device.h b/include/media/media-device.h
index bcc6ec434f1f..f2d471b8a53f 100644
--- a/include/media/media-device.h
+++ b/include/media/media-device.h
@@ -27,6 +27,7 @@
 
 struct ida;
 struct device;
+struct media_request_mgr;
 
 /**
  * struct media_entity_notify - Media Entity Notify
@@ -158,6 +159,8 @@ struct media_device {
 	void (*disable_source)(struct media_entity *entity);
 
 	const struct media_device_ops *ops;
+
+	struct media_request_mgr *req_mgr;
 };
 
 /* We don't need to include pci.h or usb.h here */
diff --git a/include/media/media-request-mgr.h b/include/media/media-request-mgr.h
new file mode 100644
index 000000000000..a3161fa2add0
--- /dev/null
+++ b/include/media/media-request-mgr.h
@@ -0,0 +1,73 @@
+/*
+ * Generic request manager.
+ *
+ * Copyright (C) 2018, The Chromium OS Authors.  All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef _MEDIA_REQUEST_MGR_H
+#define _MEDIA_REQUEST_MGR_H
+
+#include <linux/mutex.h>
+
+struct media_request_mgr;
+
+/**
+ * struct media_request_mgr_ops - request manager operations
+ *
+ * @req_alloc:	allocate a request
+ * @req_free:	free a previously allocated request
+ * @req_complete: callback invoked when a request has completed
+ *
+ */
+struct media_request_mgr_ops {
+	struct media_request *(*req_alloc)(struct media_request_mgr *mgr);
+	void (*req_free)(struct media_request *req);
+	void (*req_complete)(struct media_request *req);
+};
+
+/**
+ * struct media_request_mgr - requests manager
+ *
+ * @mdev:	media_device owning this manager
+ * @ops:	implementation of the manager
+ * @mutex:	protects requests, active_request, req_id, and all members of
+ *		struct media_request
+ * @requests:	list of alive requests produced by this manager
+ * @req_id:	counter used to identify requests for debugging purposes
+ */
+struct media_request_mgr {
+	struct media_device *mdev;
+	const struct media_request_mgr_ops *ops;
+
+	struct mutex mutex;
+	struct list_head requests;
+	u32 req_id;
+};
+
+/**
+ * media_request_mgr_alloc() - return an instance of the generic manager
+ *
+ * @mdev:	owning media device
+ */
+struct media_request_mgr *
+media_request_mgr_alloc(struct media_device *mdev);
+
+/**
+ * media_request_mgr_free() - free a media manager
+ *
+ * This should only be called when all requests produced by this manager
+ * has been destroyed. Will warn if that is not the case.
+ *
+ */
+void media_request_mgr_free(struct media_request_mgr *mgr);
+
+#endif
diff --git a/include/media/media-request.h b/include/media/media-request.h
new file mode 100644
index 000000000000..c048f9a911a5
--- /dev/null
+++ b/include/media/media-request.h
@@ -0,0 +1,184 @@
+/*
+ * Media requests.
+ *
+ * Copyright (C) 2018, The Chromium OS Authors.  All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef _MEDIA_REQUEST_H
+#define _MEDIA_REQUEST_H
+
+#include <linux/kref.h>
+#include <linux/wait.h>
+#include <linux/list.h>
+#include <linux/notifier.h>
+
+struct media_device;
+struct media_request_mgr;
+struct media_request_cmd;
+struct media_entity;
+struct media_request_entity_data;
+
+#ifdef CONFIG_MEDIA_CONTROLLER
+
+enum media_request_state {
+	MEDIA_REQUEST_STATE_IDLE,
+	MEDIA_REQUEST_STATE_SUBMITTED,
+	MEDIA_REQUEST_STATE_COMPLETED,
+	MEDIA_REQUEST_STATE_DELETED,
+};
+
+/**
+ * struct media_request - Media request base structure
+ * @id:		request id, used internally for debugging
+ * @mgr:	manager this request belongs to
+ * @kref:	reference count
+ * @list:	list entry in the media device requests list
+ * @lock:	protects internal state against concurrent accesses
+ * @state:	current state of the request
+ * @data:	per-entity data list
+ * @complete_wait:	wait queue that signals once the request has completed
+ * submit_notif:	notification list to call when the request is submitted
+ * @buf_cpt:	counter of queued/completed buffers used to decide when the
+ *		request is completed
+ */
+struct media_request {
+	u32 id;
+	struct media_request_mgr *mgr;
+	struct kref kref;
+	struct list_head list;
+
+	struct mutex lock;
+	enum media_request_state state;
+	struct list_head data;
+	wait_queue_head_t complete_wait;
+	struct atomic_notifier_head submit_notif;
+	atomic_t buf_cpt;
+};
+
+/**
+ * media_request_get() - increment the reference counter of a request
+ *
+ * The calling context must call media_request_put() once it does not need
+ * the reference to the request anymore.
+ *
+ * @req:	request to acquire a reference of
+ */
+void media_request_get(struct media_request *req);
+
+/**
+ * media_request_get_from_fd() - lookup request by fd and acquire a reference.
+ *
+ * Look a request up from its fd, acquire a reference and return a pointer to
+ * the request. As for media_request_get(), media_request_put() must be called
+ * once the reference is not used anymore.
+ *
+ * @req:	request to lookup and acquire.
+ *
+ */
+struct media_request *media_request_get_from_fd(int fd);
+
+/**
+ * media_request_put() - decrement the reference counter of a request
+ *
+ * Mirror function of media_request_get() and media_request_get_from_fd(). Will
+ * free the request if this was the last valid reference.
+ *
+ * @req:	request to release.
+ */
+void media_request_put(struct media_request *req);
+
+/**
+ * media_request_get_entity_data() - get per-entity data for a request
+ * @req:	request to get entity data from
+ * @entity:	entity to get data of
+ * @fh:		cookie identifying the handle from which the entity is accessed
+ *
+ * Search and return the entity data associated associated to the request. If no
+ * such data exists, it is allocated as per the entity operations.
+ *
+ * The fh arguments serves as a cookie to make sure the same entity is not
+ * accessed through different opened file handles. The same handle must be
+ * passed to all calls of this function for the same entity. Failure to do so
+ * will return an error.
+ *
+ * Returns the per-entity data, or an error code if a problem happened. -EINVAL
+ * means that data for the entity already existed, but has been allocated under
+ * a different cookie.
+ */
+struct media_request_entity_data *
+media_request_get_entity_data(struct media_request *req,
+			      struct media_entity *entity, void *fh);
+
+
+/**
+ * media_request_complete() - to be invoked when the request is complete
+ *
+ * @req:	request which has completed
+ */
+void media_request_complete(struct media_request *req);
+
+/**
+ * struct media_request_entity_data - per-entity request data
+ *
+ * Base structure used to store request state data. To be extended by actual
+ * implementation.
+ *
+ * @entity:	entity this data belongs to
+ * @fh:		subsystem-dependent. For V4L2, the v4l2_fh of the opened device
+ * @list:	entry in media_request::data
+ * @applied:	whether this request has already been applied for this entity
+ */
+struct media_request_entity_data {
+	struct media_entity *entity;
+	void *fh;
+	struct list_head list;
+	bool applied;
+};
+
+/**
+ * media_device_request_cmd() - perform the REQUEST_CMD ioctl
+ *
+ * @mdev:	media device the ioctl has been called on
+ * @cmd:	user-space request command structure
+ */
+long media_device_request_cmd(struct media_device *mdev,
+			      struct media_request_cmd *cmd);
+
+#else /* CONFIG_MEDIA_CONTROLLER */
+
+static inline media_request_get(struct media_request *req)
+{
+}
+
+static inline struct media_request *media_request_get_from_fd(int fd)
+{
+	return ERR_PTR(-ENOSYS);
+}
+
+static inline void media_request_put(struct media_request *req)
+{
+}
+
+static inline struct media_request *media_request_get_entity_data(
+					  struct media_request *req,
+					  struct media_entity *entity, void *fh)
+{
+	return ERR_PTR(-ENOSYS);
+}
+
+static inline void media_request_entity_complete(struct media_request *req)
+{
+}
+
+#endif /* CONFIG_MEDIA_CONTROLLER */
+
+#endif
diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
index b9b9446095e9..6e20ac9848b5 100644
--- a/include/uapi/linux/media.h
+++ b/include/uapi/linux/media.h
@@ -406,6 +406,15 @@ struct media_v2_topology {
 	__u64 ptr_links;
 } __attribute__ ((packed));
 
+#define MEDIA_REQ_CMD_ALLOC		0
+#define MEDIA_REQ_CMD_SUBMIT		1
+#define MEDIA_REQ_CMD_REINIT		2
+
+struct media_request_cmd {
+	__u32 cmd;
+	__s32 fd;
+} __attribute__ ((packed));
+
 /* ioctls */
 
 #define MEDIA_IOC_DEVICE_INFO		_IOWR('|', 0x00, struct media_device_info)
@@ -413,5 +422,6 @@ struct media_v2_topology {
 #define MEDIA_IOC_ENUM_LINKS		_IOWR('|', 0x02, struct media_links_enum)
 #define MEDIA_IOC_SETUP_LINK		_IOWR('|', 0x03, struct media_link_desc)
 #define MEDIA_IOC_G_TOPOLOGY		_IOWR('|', 0x04, struct media_v2_topology)
+#define MEDIA_IOC_REQUEST_CMD		_IOWR('|', 0x05, struct media_request_cmd)
 
 #endif /* __LINUX_MEDIA_H */
-- 
2.16.0.rc1.238.g530d649a79-goog

[RFC PATCH 2/8] videodev2.h: Add request field to v4l2_buffer

[Alexandre Courbot]

From: Hans Verkuil <hans.verkuil@cisco.com>

When queuing buffers allow for passing the request ID that
should be associated with this buffer.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
[acourbot@chromium.org: make request ID 32-bit]
Signed-off-by: Alexandre Courbot <acourbot@chromium.org>
---
 drivers/media/usb/cpia2/cpia2_v4l.c           | 2 +-
 drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 7 ++++---
 drivers/media/v4l2-core/v4l2-ioctl.c          | 4 ++--
 drivers/media/v4l2-core/videobuf2-v4l2.c      | 3 ++-
 include/media/videobuf2-v4l2.h                | 2 ++
 include/uapi/linux/videodev2.h                | 3 ++-
 6 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/media/usb/cpia2/cpia2_v4l.c b/drivers/media/usb/cpia2/cpia2_v4l.c
index 3dedd83f0b19..7acb6807b306 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->request_fd = 0;
 	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..7e4440950c76 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;
+	__u32			request_fd;
 	__u32			reserved;
 };
 
@@ -438,7 +438,8 @@ static int get_v4l2_buffer32(struct v4l2_buffer *kp, struct v4l2_buffer32 __user
 		get_user(kp->type, &up->type) ||
 		get_user(kp->flags, &up->flags) ||
 		get_user(kp->memory, &up->memory) ||
-		get_user(kp->length, &up->length))
+		get_user(kp->length, &up->length) ||
+		get_user(kp->request_fd, &up->request_fd))
 			return -EFAULT;
 
 	if (V4L2_TYPE_IS_OUTPUT(kp->type))
@@ -533,7 +534,7 @@ static int put_v4l2_buffer32(struct v4l2_buffer *kp, struct v4l2_buffer32 __user
 		put_user(kp->timestamp.tv_usec, &up->timestamp.tv_usec) ||
 		copy_to_user(&up->timecode, &kp->timecode, sizeof(struct v4l2_timecode)) ||
 		put_user(kp->sequence, &up->sequence) ||
-		put_user(kp->reserved2, &up->reserved2) ||
+		put_user(kp->request_fd, &up->request_fd) ||
 		put_user(kp->reserved, &up->reserved) ||
 		put_user(kp->length, &up->length))
 			return -EFAULT;
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index ec4ecd5aa8bf..fdd2f784c264 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -437,13 +437,13 @@ static void v4l_print_buffer(const void *arg, bool write_only)
 	const struct v4l2_plane *plane;
 	int i;
 
-	pr_cont("%02ld:%02d:%02d.%08ld index=%d, type=%s, flags=0x%08x, field=%s, sequence=%d, memory=%s",
+	pr_cont("%02ld:%02d:%02d.%08ld index=%d, type=%s, request_fd=%u, flags=0x%08x, field=%s, sequence=%d, memory=%s",
 			p->timestamp.tv_sec / 3600,
 			(int)(p->timestamp.tv_sec / 60) % 60,
 			(int)(p->timestamp.tv_sec % 60),
 			(long)p->timestamp.tv_usec,
 			p->index,
-			prt_names(p->type, v4l2_type_names),
+			prt_names(p->type, v4l2_type_names), p->request_fd,
 			p->flags, prt_names(p->field, v4l2_field_names),
 			p->sequence, prt_names(p->memory, v4l2_memory_names));
 
diff --git a/drivers/media/v4l2-core/videobuf2-v4l2.c b/drivers/media/v4l2-core/videobuf2-v4l2.c
index 0c0669976bdc..0f8edbdebe30 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->request_fd = vbuf->request_fd;
 	b->reserved = 0;
 
 	if (q->is_multiplanar) {
@@ -320,6 +320,7 @@ static int __fill_vb2_buffer(struct vb2_buffer *vb,
 	}
 	vb->timestamp = 0;
 	vbuf->sequence = 0;
+	vbuf->request_fd = b->request_fd;
 
 	if (V4L2_TYPE_IS_MULTIPLANAR(b->type)) {
 		if (b->memory == VB2_MEMORY_USERPTR) {
diff --git a/include/media/videobuf2-v4l2.h b/include/media/videobuf2-v4l2.h
index 036127c54bbf..d7cf4c66db38 100644
--- a/include/media/videobuf2-v4l2.h
+++ b/include/media/videobuf2-v4l2.h
@@ -31,6 +31,7 @@
  * @field:	enum v4l2_field; field order of the image in the buffer
  * @timecode:	frame timecode
  * @sequence:	sequence count of this frame
+ * @request_fd:	fd of the request used by the buffer
  *
  * Should contain enough information to be able to cover all the fields
  * of struct v4l2_buffer at videodev2.h
@@ -42,6 +43,7 @@ struct vb2_v4l2_buffer {
 	__u32			field;
 	struct v4l2_timecode	timecode;
 	__u32			sequence;
+	__u32			request_fd;
 };
 
 /*
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 1c095b5a99c5..89bd716c66a6 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -902,6 +902,7 @@ struct v4l2_plane {
  * @length:	size in bytes of the buffer (NOT its payload) for single-plane
  *		buffers (when type != *_MPLANE); number of elements in the
  *		planes array for multi-plane buffers
+ * @request_fd: fd of the request that this buffer should use
  *
  * Contains data exchanged by application and driver using one of the Streaming
  * I/O methods.
@@ -925,7 +926,7 @@ struct v4l2_buffer {
 		__s32		fd;
 	} m;
 	__u32			length;
-	__u32			reserved2;
+	__s32			request_fd;
 	__u32			reserved;
 };
 
-- 
2.16.0.rc1.238.g530d649a79-goog

[RFC PATCH 3/8] media: videobuf2: add support for requests

[Alexandre Courbot]

Make vb2 aware of requests. Drivers can specify whether a given queue
can accept requests or not. Queues that accept requests will block on a
buffer that is part of a request until that request is submitted.

Signed-off-by: Alexandre Courbot <acourbot@chromium.org>
---
 drivers/media/v4l2-core/videobuf2-core.c | 125 +++++++++++++++++++++++++++++--
 drivers/media/v4l2-core/videobuf2-v4l2.c |  28 ++++++-
 include/media/videobuf2-core.h           |  15 +++-
 3 files changed, 160 insertions(+), 8 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index cb115ba6a1d2..f6d013b141f1 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -26,6 +26,7 @@
 
 #include <media/videobuf2-core.h>
 #include <media/v4l2-mc.h>
+#include <media/media-request.h>
 
 #include <trace/events/vb2.h>
 
@@ -922,6 +923,17 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state)
 		vb->state = state;
 	}
 	atomic_dec(&q->owned_by_drv_count);
+	if (vb->request) {
+		struct media_request *req = vb->request;
+
+		if (atomic_dec_and_test(&req->buf_cpt))
+			media_request_complete(vb->request);
+
+		/* release reference acquired during qbuf */
+		vb->request = NULL;
+		media_request_put(req);
+	}
+
 	spin_unlock_irqrestore(&q->done_lock, flags);
 
 	trace_vb2_buf_done(q, vb);
@@ -1298,6 +1310,53 @@ int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb)
 }
 EXPORT_SYMBOL_GPL(vb2_core_prepare_buf);
 
+/**
+ * vb2_check_buf_req_status() - Validate request state of a buffer
+ * @vb:		buffer to check
+ *
+ * Returns true if a buffer is ready to be passed to the driver request-wise.
+ * This means that neither this buffer nor any previously-queued buffer is
+ * associated to a request that is not yet submitted.
+ *
+ * If this function returns false, then the buffer shall not be passed to its
+ * driver since the request state is not completely built yet. In that case,
+ * this function will register a notifier to be called when the request is
+ * submitted and the queue can be unblocked.
+ *
+ * This function must be called with req_lock held.
+ */
+static bool vb2_check_buf_req_status(struct vb2_buffer *vb)
+{
+	struct media_request *req = vb->request;
+	struct vb2_queue *q = vb->vb2_queue;
+	int ret = false;
+
+	mutex_lock(&q->req_lock);
+
+	if (!req) {
+		ret = !q->waiting_req;
+		goto done;
+	}
+
+	mutex_lock(&req->lock);
+	if (req->state == MEDIA_REQUEST_STATE_SUBMITTED) {
+		mutex_unlock(&req->lock);
+		ret = !q->waiting_req;
+		goto done;
+	}
+
+	if (!q->waiting_req) {
+		q->waiting_req = true;
+		atomic_notifier_chain_register(&req->submit_notif,
+					       &q->req_blk);
+	}
+	mutex_unlock(&req->lock);
+
+done:
+	mutex_unlock(&q->req_lock);
+	return ret;
+}
+
 /**
  * vb2_start_streaming() - Attempt to start streaming.
  * @q:		videobuf2 queue
@@ -1318,8 +1377,11 @@ static int vb2_start_streaming(struct vb2_queue *q)
 	 * If any buffers were queued before streamon,
 	 * we can now pass them to driver for processing.
 	 */
-	list_for_each_entry(vb, &q->queued_list, queued_entry)
+	list_for_each_entry(vb, &q->queued_list, queued_entry) {
+		if (!vb2_check_buf_req_status(vb))
+			break;
 		__enqueue_in_driver(vb);
+	}
 
 	/* Tell the driver to start streaming */
 	q->start_streaming_called = 1;
@@ -1361,7 +1423,46 @@ static int vb2_start_streaming(struct vb2_queue *q)
 	return ret;
 }
 
-int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
+/**
+ * vb2_unblock_requests() - unblock a queue waiting for a request submission
+ * @nb:		notifier block that has been registered
+ * @action:	unused
+ * @data:	request that has been submitted
+ *
+ * This is a callback function that is registered when
+ * vb2_check_buf_req_status() returns false. It is invoked when the request
+ * blocking the queue has been submitted. This means its buffers (and all
+ * following valid buffers) can be passed to drivers.
+ */
+static int vb2_unblock_requests(struct notifier_block *nb, unsigned long action,
+				void *data)
+{
+	struct vb2_queue *q = container_of(nb, struct vb2_queue, req_blk);
+	struct media_request *req = data;
+	struct vb2_buffer *vb;
+	bool found_request = false;
+
+	mutex_lock(&q->req_lock);
+	atomic_notifier_chain_unregister(&req->submit_notif, &q->req_blk);
+	q->waiting_req = false;
+	mutex_unlock(&q->req_lock);
+
+	list_for_each_entry(vb, &q->queued_list, queued_entry) {
+		/* All buffers before our request are already passed to the driver */
+		if (!found_request && vb->request != req)
+			continue;
+		found_request = true;
+
+		if (!vb2_check_buf_req_status(vb))
+			break;
+		__enqueue_in_driver(vb);
+	}
+
+	return 0;
+}
+
+int vb2_core_qbuf(struct vb2_queue *q, unsigned int index,
+		  struct media_request *req, void *pb)
 {
 	struct vb2_buffer *vb;
 	int ret;
@@ -1398,11 +1499,21 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
 
 	trace_vb2_qbuf(q, vb);
 
+	vb->request = req;
+	if (req) {
+		if (!q->allow_requests)
+			return -EINVAL;
+
+		/* make sure the request stays alive as long as we need */
+		media_request_get(req);
+		atomic_inc(&req->buf_cpt);
+	}
+
 	/*
 	 * 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)
+	if (q->start_streaming_called && vb2_check_buf_req_status(vb))
 		__enqueue_in_driver(vb);
 
 	/* Fill buffer information for the userspace */
@@ -1993,6 +2104,8 @@ int vb2_core_queue_init(struct vb2_queue *q)
 	spin_lock_init(&q->done_lock);
 	mutex_init(&q->mmap_lock);
 	init_waitqueue_head(&q->done_wq);
+	mutex_init(&q->req_lock);
+	q->req_blk.notifier_call = vb2_unblock_requests;
 
 	if (q->buf_struct_size == 0)
 		q->buf_struct_size = sizeof(struct vb2_buffer);
@@ -2242,7 +2355,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;
@@ -2421,7 +2534,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;
@@ -2524,7 +2637,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 0f8edbdebe30..267fe2d669b2 100644
--- a/drivers/media/v4l2-core/videobuf2-v4l2.c
+++ b/drivers/media/v4l2-core/videobuf2-v4l2.c
@@ -30,6 +30,7 @@
 #include <media/v4l2-common.h>
 
 #include <media/videobuf2-v4l2.h>
+#include <media/media-request.h>
 
 static int debug;
 module_param(debug, int, 0644);
@@ -561,6 +562,7 @@ EXPORT_SYMBOL_GPL(vb2_create_bufs);
 
 int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
 {
+	struct media_request *req = NULL;
 	int ret;
 
 	if (vb2_fileio_is_active(q)) {
@@ -568,8 +570,32 @@ int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
 		return -EBUSY;
 	}
 
+	/*
+	 * The caller should have validated that the request is valid,
+	 * so we just need to look it up without further checking
+	 */
+	if (b->request_fd > 0) {
+		req = media_request_get_from_fd(b->request_fd);
+		if (!req)
+			return -EINVAL;
+
+		mutex_lock(&req->lock);
+		if (req->state != MEDIA_REQUEST_STATE_IDLE) {
+			mutex_unlock(&req->lock);
+			media_request_put(req);
+			return -EINVAL;
+		}
+		mutex_unlock(&req->lock);
+	}
+
 	ret = vb2_queue_or_prepare_buf(q, b, "qbuf");
-	return ret ? ret : vb2_core_qbuf(q, b->index, b);
+	if (!ret)
+		ret = vb2_core_qbuf(q, b->index, req, b);
+
+	if (req)
+		media_request_put(req);
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(vb2_qbuf);
 
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index ef9b64398c8c..7bb17c842ab4 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -237,6 +237,7 @@ struct vb2_queue;
  *			on an internal driver queue
  * @planes:		private per-plane information; do not change
  * @timestamp:		frame timestamp in ns
+ * @request:		request the buffer belongs to, if any
  */
 struct vb2_buffer {
 	struct vb2_queue	*vb2_queue;
@@ -246,6 +247,7 @@ struct vb2_buffer {
 	unsigned int		num_planes;
 	struct vb2_plane	planes[VB2_MAX_PLANES];
 	u64			timestamp;
+	struct media_request	*request;
 
 	/* private: internal use only
 	 *
@@ -443,6 +445,7 @@ struct vb2_buf_ops {
  * @quirk_poll_must_check_waiting_for_buffers: Return POLLERR at poll when QBUF
  *              has not been called. This is a vb1 idiom that has been adopted
  *              also by vb2.
+ * @allow_requests:	whether requests are supported on this queue.
  * @lock:	pointer to a mutex that protects the vb2_queue struct. The
  *		driver can set this to a mutex to let the v4l2 core serialize
  *		the queuing ioctls. If the driver wants to handle locking
@@ -500,6 +503,9 @@ struct vb2_buf_ops {
  *		when a buffer with the V4L2_BUF_FLAG_LAST is dequeued.
  * @fileio:	file io emulator internal data, used only if emulator is active
  * @threadio:	thread io internal data, used only if thread is active
+ * @req_lock:	protects req_blk and waiting_req
+ * @req_blk:	notifier to be called when waiting for a request to be submitted
+ * @waiting_req:whether this queue is currently waiting on a request submission
  */
 struct vb2_queue {
 	unsigned int			type;
@@ -511,6 +517,7 @@ struct vb2_queue {
 	unsigned			fileio_write_immediately:1;
 	unsigned			allow_zero_bytesused:1;
 	unsigned		   quirk_poll_must_check_waiting_for_buffers:1;
+	unsigned			allow_requests:1;
 
 	struct mutex			*lock;
 	void				*owner;
@@ -554,6 +561,10 @@ struct vb2_queue {
 	struct vb2_fileio_data		*fileio;
 	struct vb2_threadio_data	*threadio;
 
+	struct mutex			req_lock;
+	struct notifier_block		req_blk;
+	bool				waiting_req;
+
 #ifdef CONFIG_VIDEO_ADV_DEBUG
 	/*
 	 * Counters for how often these queue-related ops are
@@ -724,6 +735,7 @@ int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb);
  *
  * @q:		videobuf2 queue
  * @index:	id number of the buffer
+ * @req:	request this buffer belongs to, if any
  * @pb:		buffer structure passed from userspace to vidioc_qbuf handler
  *		in driver
  *
@@ -740,7 +752,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,
+		  struct media_request *req, void *pb);
 
 /**
  * vb2_core_dqbuf() - Dequeue a buffer to the userspace
-- 
2.16.0.rc1.238.g530d649a79-goog

[RFC PATCH 4/8] media: vb2: add support for requests in QBUF ioctl

[Alexandre Courbot]

Support the request argument of the QBUF ioctl.

Signed-off-by: Alexandre Courbot <acourbot@chromium.org>
---
 drivers/media/v4l2-core/v4l2-ioctl.c | 79 +++++++++++++++++++++++++++++++++++-
 1 file changed, 78 insertions(+), 1 deletion(-)

diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index fdd2f784c264..66f2bda4a279 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -29,6 +29,7 @@
 #include <media/v4l2-device.h>
 #include <media/videobuf2-v4l2.h>
 #include <media/v4l2-mc.h>
+#include <media/media-request.h>
 
 #include <trace/events/v4l2.h>
 
@@ -965,6 +966,67 @@ static int check_fmt(struct file *file, enum v4l2_buf_type type)
 	return -EINVAL;
 }
 
+/*
+ * Validate that a given request can be used during an ioctl.
+ *
+ * When using the request API, request file descriptors must be matched against
+ * the actual request object. User-space can pass any file descriptor, so we
+ * need to make sure the call is valid before going further.
+ *
+ * This function looks up the request and associated data and performs the
+ * following sanity checks:
+ *
+ * * Make sure that the entity belongs to the media_device managing the passed
+ *   request,
+ * * Make sure that the entity data (if any) is associated to the current file
+ *   handler.
+ *
+ * This function returns a pointer to the valid request, or and error code in
+ * case of failure. When successful, a reference to the request is acquired and
+ * must be properly released by the caller.
+ */
+#ifdef CONFIG_MEDIA_CONTROLLER
+static struct media_request *
+check_request(int request, struct file *file, void *fh)
+{
+	struct media_request *req = NULL;
+	struct video_device *vfd = video_devdata(file);
+	struct v4l2_fh *vfh =
+		test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) ? fh : NULL;
+	struct media_entity *entity = &vfd->entity;
+	struct media_request_entity_data *data;
+
+	if (!entity)
+		return ERR_PTR(-EINVAL);
+
+	req = media_request_get_from_fd(request);
+	if (!req)
+		return ERR_PTR(-EINVAL);
+
+	/* Validate that the entity belongs to the correct media_device */
+	if (vfd->v4l2_dev->mdev->req_mgr != req->mgr) {
+		media_request_put(req);
+		return ERR_PTR(-EINVAL);
+	}
+
+	/* Validate that the entity's data belongs to the correct fh */
+	data = media_request_get_entity_data(req, entity, vfh);
+	if (IS_ERR(data)) {
+		media_request_put(req);
+		return ERR_PTR(PTR_ERR(data));
+	}
+
+	return req;
+}
+#else /* CONFIG_MEDIA_CONTROLLER */
+static struct media_request *
+check_request(int request, struct file *file, void *fh)
+{
+	return ERR_PTR(-ENOSYS);
+}
+
+#endif /* CONFIG_MEDIA_CONTROLLER */
+
 static void v4l_sanitize_format(struct v4l2_format *fmt)
 {
 	unsigned int offset;
@@ -1902,10 +1964,25 @@ static int v4l_querybuf(const struct v4l2_ioctl_ops *ops,
 static int v4l_qbuf(const struct v4l2_ioctl_ops *ops,
 				struct file *file, void *fh, void *arg)
 {
+	struct media_request *req = NULL;
 	struct v4l2_buffer *p = arg;
 	int ret = check_fmt(file, p->type);
 
-	return ret ? ret : ops->vidioc_qbuf(file, fh, p);
+	if (ret)
+		return ret;
+
+	if (p->request_fd > 0) {
+		req = check_request(p->request_fd, file, fh);
+		if (IS_ERR(req))
+			return PTR_ERR(req);
+	}
+
+	ret = ops->vidioc_qbuf(file, fh, p);
+
+	if (req)
+		media_request_put(req);
+
+	return ret;
 }
 
 static int v4l_dqbuf(const struct v4l2_ioctl_ops *ops,
-- 
2.16.0.rc1.238.g530d649a79-goog

[RFC PATCH 5/8] media: Document the media request API

[Alexandre Courbot]

From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

The media request API is made of a new ioctl to implement request
management. Document it.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
[acourbot@chromium.org: adapt for newest API]
Signed-off-by: Alexandre Courbot <acourbot@chromium.org>
---
 Documentation/media/uapi/mediactl/media-funcs.rst  |   1 +
 .../media/uapi/mediactl/media-ioc-request-cmd.rst  | 140 +++++++++++++++++++++
 2 files changed, 141 insertions(+)
 create mode 100644 Documentation/media/uapi/mediactl/media-ioc-request-cmd.rst

diff --git a/Documentation/media/uapi/mediactl/media-funcs.rst b/Documentation/media/uapi/mediactl/media-funcs.rst
index 076856501cdb..e3a45d82ffcb 100644
--- a/Documentation/media/uapi/mediactl/media-funcs.rst
+++ b/Documentation/media/uapi/mediactl/media-funcs.rst
@@ -15,4 +15,5 @@ Function Reference
     media-ioc-g-topology
     media-ioc-enum-entities
     media-ioc-enum-links
+    media-ioc-request-cmd
     media-ioc-setup-link
diff --git a/Documentation/media/uapi/mediactl/media-ioc-request-cmd.rst b/Documentation/media/uapi/mediactl/media-ioc-request-cmd.rst
new file mode 100644
index 000000000000..17223e8170e9
--- /dev/null
+++ b/Documentation/media/uapi/mediactl/media-ioc-request-cmd.rst
@@ -0,0 +1,140 @@
+.. -*- coding: utf-8; mode: rst -*-
+
+.. _media_ioc_request_cmd:
+
+***************************
+ioctl MEDIA_IOC_REQUEST_CMD
+***************************
+
+Name
+====
+
+MEDIA_IOC_REQUEST_CMD - Manage media device requests
+
+
+Synopsis
+========
+
+.. c:function:: int ioctl( int fd, MEDIA_IOC_REQUEST_CMD, struct media_request_cmd *argp )
+    :name: MEDIA_IOC_REQUEST_CMD
+
+
+Arguments
+=========
+
+``fd``
+    File descriptor returned by :ref:`open() <media-func-open>`.
+
+``argp``
+
+
+Description
+===========
+
+The MEDIA_IOC_REQUEST_CMD ioctl allow applications to manage media device
+requests. A request is an object that can group media device configuration
+parameters, including subsystem-specific parameters, in order to apply all the
+parameters atomically. Applications are responsible for allocating and
+deleting requests, filling them with configuration parameters submitting them.
+
+Request operations are performed by calling the MEDIA_IOC_REQUEST_CMD ioctl
+with a pointer to a struct :c:type:`media_request_cmd` with the cmd field set
+to the appropriate command. :ref:`media-request-command` lists the commands
+supported by the ioctl.
+
+The struct :c:type:`media_request_cmd` request field contains the file
+descriptorof the request on which the command operates. For the
+``MEDIA_REQ_CMD_ALLOC`` command the field is set to zero by applications and
+filled by the driver. For all other commands the field is set by applications
+and left untouched by the driver.
+
+To allocate a new request applications use the ``MEDIA_REQ_CMD_ALLOC``
+command. The driver will allocate a new request and return its ID in the
+request field.
+
+Requests are reference-counted. A newly allocated request is referenced
+by the returned file descriptor, and can be later referenced by
+subsystem-specific operations. Requests will thus be automatically deleted
+when they're not longer used after the returned file descriptor is closed.
+
+If a request isn't needed applications can delete it by calling ``close()``
+on it. The driver will drop the file handle reference. The request will not
+be usable through the MEDIA_IOC_REQUEST_CMD ioctl anymore, but will only be
+deleted when the last reference is released. If no other reference exists when
+``close()`` is invoked the request will be deleted immediately.
+
+After creating a request applications should fill it with configuration
+parameters. This is performed through subsystem-specific request APIs outside
+the scope of the media controller API. See the appropriate subsystem APIs for
+more information, including how they interact with the MEDIA_IOC_REQUEST_CMD
+ioctl.
+
+Once a request contains all the desired configuration parameters it can be
+submitted using the ``MEDIA_REQ_CMD_SUBMIT`` command. This will let the
+buffers queued for the request be passed to their respective drivers, which
+will then apply the request's parameters before processing them.
+
+Once a request has been queued applications are not allowed to modify its
+configuration parameters until the request has been fully processed. Any
+attempt to do so will result in the related subsystem API returning an error.
+The application that submitted the request can wait for its completion by
+polling on the request's file descriptor.
+
+Once a request has completed, it can be reused. The ``MEDIA_REQ_CMD_REINIT``
+command will bring it back to its initial state, so it can be prepared and
+submitted again.
+
+.. c:type:: media_request_cmd
+
+.. flat-table:: struct media_request_cmd
+    :header-rows:  0
+    :stub-columns: 0
+    :widths: 1 2 8
+
+    * - __u32
+      - ``cmd``
+      - Command, set by the application. See below for the list of supported
+        commands.
+    * - __u32
+      - ``fd``
+      - Request FD, set by the driver for the MEDIA_REQ_CMD_ALLOC command and
+        by the application for all other commands.
+
+
+.. _media-request-command:
+
+.. cssclass:: longtable
+
+.. flat-table:: Media request commands
+    :header-rows:  0
+    :stub-columns: 0
+
+    * .. _MEDIA-REQ-CMD-ALLOC:
+
+      - ``MEDIA_REQ_CMD_ALLOC``
+      - Allocate a new request.
+    * .. _MEDIA-REQ-CMD-SUBMIT:
+
+      - ``MEDIA_REQ_CMD_SUBMIT``
+      - Submit a request to be processed.
+    * .. _MEDIA-REQ-CMD-QUEUE:
+
+      - ``MEDIA_REQ_CMD_REINIT``
+      - Reinitializes a completed request.
+
+
+Return Value
+============
+
+On success 0 is returned, on error -1 and the ``errno`` variable is set
+appropriately. The generic error codes are described at the
+:ref:`Generic Error Codes <gen-errors>` chapter.
+
+EINVAL
+    The struct :c:type:`media_request_cmd` specifies an invalid command or
+    references a non-existing request.
+
+ENOSYS
+    The struct :c:type:`media_request_cmd` specifies the
+    ``MEDIA_REQ_CMD_QUEUE`` or ``MEDIA_REQ_CMD_APPLY`` command and that
+    command isn't implemented by the device.
-- 
2.16.0.rc1.238.g530d649a79-goog

[RFC PATCH 6/8] v4l2: document the request API interface

[Alexandre Courbot]

Document how the request API can be used along with the existing V4L2
interface.

Signed-off-by: Alexandre Courbot <acourbot@chromium.org>
---
 Documentation/media/uapi/v4l/buffer.rst      |  10 +-
 Documentation/media/uapi/v4l/common.rst      |   1 +
 Documentation/media/uapi/v4l/request-api.rst | 194 +++++++++++++++++++++++++++
 Documentation/media/uapi/v4l/vidioc-qbuf.rst |  21 +++
 4 files changed, 223 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/media/uapi/v4l/request-api.rst

diff --git a/Documentation/media/uapi/v4l/buffer.rst b/Documentation/media/uapi/v4l/buffer.rst
index ae6ee73f151c..9d082784081d 100644
--- a/Documentation/media/uapi/v4l/buffer.rst
+++ b/Documentation/media/uapi/v4l/buffer.rst
@@ -301,10 +301,14 @@ struct v4l2_buffer
 	elements in the ``planes`` array. The driver will fill in the
 	actual number of valid elements in that array.
     * - __u32
-      - ``reserved2``
+      - ``request_fd``
       -
-      - A place holder for future extensions. Drivers and applications
-	must set this to 0.
+      - The file descriptor of the request associated with this buffer.
+	user-space can set this when calling :ref:`VIDIOC_QBUF`, and drivers
+	will return the request used when processing a buffer (if any) upon
+	:ref:`VIDIOC_DQBUF`.
+
+	A value of 0 means the buffer is not associated with any request.
     * - __u32
       - ``reserved``
       -
diff --git a/Documentation/media/uapi/v4l/common.rst b/Documentation/media/uapi/v4l/common.rst
index 13f2ed3fc5a6..a4aa0059d45a 100644
--- a/Documentation/media/uapi/v4l/common.rst
+++ b/Documentation/media/uapi/v4l/common.rst
@@ -44,3 +44,4 @@ applicable to all devices.
     crop
     selection-api
     streaming-par
+    request-api
diff --git a/Documentation/media/uapi/v4l/request-api.rst b/Documentation/media/uapi/v4l/request-api.rst
new file mode 100644
index 000000000000..a758a5fd3ca0
--- /dev/null
+++ b/Documentation/media/uapi/v4l/request-api.rst
@@ -0,0 +1,194 @@
+.. -*- coding: utf-8; mode: rst -*-
+
+.. _media-request-api:
+
+Request API
+===========
+
+The Request API has been designed to allow V4L2 to deal with requirements of
+modern IPs (stateless codecs, MIPI cameras, ...) and APIs (Android Codec v2).
+One such requirement is the ability for devices belonging to the same pipeline
+to reconfigure and collaborate closely on a per-frame basis. Another is
+efficient support of stateless codecs, which need per-frame controls to be set
+asynchronously in order to be efficiently used.
+
+Supporting these features without the Request API is possible but terribly
+inefficient: user-space would have to flush all activity on the media pipeline,
+reconfigure it for the next frame, queue the buffers to be processed with that
+configuration, and wait until they are all available for dequeing before
+considering the next frame. This defeats the purpose of having buffer queues
+since in practice only one buffer would be queued at a time.
+
+The Request API allows a specific configuration of the pipeline (media
+controller topology + controls for each device) to be associated with specific
+buffers. The parameters are applied by each participating device as buffers
+associated to a request flow in. This allows user-space to schedule several
+tasks ("requests") with different parameters in advance, knowing that the
+parameters will be applied when needed to get the expected result. Controls
+values at the time of request completion are also available for reading.
+
+Usage
+=====
+
+The Request API is used on top of standard media controller and V4L2 calls,
+which are augmented with an extra ``request_fd`` parameter. All operations on
+requests themselves are performed using the command parameter of the
+:c:func:`MEDIA_IOC_REQUEST_CMD` ioctl.
+
+Allocation
+----------
+
+User-space allocates requests using the ``MEDIA_REQ_CMD_ALLOC`` command on
+an opened media device. This returns a file descriptor representing the
+request. Typically, several such requests will be allocated.
+
+Request Preparation
+-------------------
+
+Standard V4L2 ioctls can then receive a request file descriptor to express the
+fact that the ioctl is part of said request, and is not to be applied
+immediately. V4L2 ioctls supporting this are :c:func:`VIDIOC_S_EXT_CTRLS` and
+:c:func:`VIDIOC_QBUF`. Controls set with a request parameter are stored instead
+of being immediately applied, and queued buffers will block the buffer queue
+until the request becomes active.
+
+RFC Note: currently several buffers can be queued to the same queue with the
+same request. The request parameters will be only be applied when processing
+the first buffer. Does it make more sense to allow at most one buffer per
+request per queue instead?
+
+Request Submission
+------------------
+
+Once the parameters and buffers of the request are specified, it can be
+submitted with the ``MEDIA_REQ_CMD_SUBMIT`` command. This will make the buffers
+associated to the request available to their driver, which can then apply the
+saved controls as buffers are processed. A submitted request cannot be used
+with further :c:func:`VIDIOC_S_EXT_CTRLS` and :c:func:`VIDIOC_QBUF` ioctls.
+
+If several devices are part of the request, individual drivers may synchronize
+so the requested pipeline's topology is applied before the buffers are
+processed. This is at the discretion of the drivers and is not a requirement.
+
+Buffers queued without an associated request after a request-bound buffer will
+be processed using the state of the hardware at the time of the request
+completion. All the same, controls set without a request are applied
+immediately, regardless of whether a request is in use or not.
+
+User-space can ``poll()`` a request FD in order to wait until the request
+completes. A request is considered complete once all its associated buffers are
+available for dequeing. Note that user-space does not need to wait for the
+request to complete to dequeue its buffers: buffers that are available halfway
+through a request can be dequeued independently of the request's state.
+
+A completed request includes the state of all devices that had queued buffers
+associated with it at the time of the request completion. User-space can query
+that state by calling :c:func:`VIDIOC_G_EXT_CTRLS` with the request FD.
+
+Recycling and Destruction
+-------------------------
+
+Finally, completed request can either be discarded or be reused. Calling
+``close()`` on a request FD will make that FD unusable, freeing the request if
+it is not referenced elsewhere. The ``MEDIA_REQ_CMD_REINIT`` command will clear
+a request's state and make it available again. No state is retained by this
+operation: the request is as if it had just been allocated via
+``MEDIA_REQ_CMD_ALLOC``.
+
+RFC Note: Since requests are represented by FDs themselves, the
+``MEDIA_REQ_CMD_SUBMIT`` and ``MEDIA_REQ_CMD_REININT`` commands can be performed
+on the request FD instead of the media device. This means the media device would
+only need to manage ``MEDIA_REQ_CMD_ALLOC``, which could be turned into an
+ioctl, while ``MEDIA_REQ_CMD_SUBMIT`` and ``MEDIA_REQ_CMD_REININT`` would
+become ioctls on the request itself. This has the advantage of allowing
+receivers of a request FD to submit the request, and also decouples requests
+from the media device - a scenario that makes sense for stateless codecs where
+the media device is not really useful.
+
+Example for a Codec Device
+--------------------------
+
+Codecs are single-node V4L2 devices providing one OUTPUT queue (for user-space
+to provide input buffers) and one CAPTURE queue (to retrieve processed data).
+
+In this use-case, request API is used to associate specific controls to be
+applied by the driver before processing a buffer from the OUTPUT queue. When
+retrieving a buffer from a capture queue, user-space can then identify which
+request, if any, produced it by looking at the request_fd field of the dequeued
+v4l2_buffer.
+
+Put into code, after obtaining a request, user-space can assign controls and one
+OUTPUT buffer to it:
+
+	struct v4l2_buf buf;
+	struct v4l2_ext_controls ctrls;
+	...
+	ctrls.request_fd = req_fd;
+	...
+	ioctl(codec_fd, VIDIOC_S_EXT_CTRLS, &ctrls);
+	...
+	buf.type = V4L2_BUF_TYPE_VIDEO_OUTPUT;
+	buf.request_fd = req_fd;
+	ioctl(codec_fd, VIDIOC_QBUF, &buf);
+
+Note that request_fd shall not be specified for CAPTURE buffers.
+
+Once the request is fully prepared, it can be submitted to the driver:
+
+	struct media_request_cmd cmd;
+
+	memset(&cmd, 0, sizeof(cmd));
+	cmd.cmd = MEDIA_REQ_CMD_SUBMIT;
+	cmd.fd = request_fd;
+	ioctl(media_fd, MEDIA_IOC_REQUEST_CMD, &cmd);
+
+User-space can then lookup the request_fd field of dequeued capture buffers to
+confirm which one has been produced by the request.
+
+	struct v4l2_buf buf;
+
+	memset(&buf, 0, sizeof(buf));
+	buf.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
+	ioctl(codec_fd, VIDIOC_DQBUF, &buf);
+
+	if (buf.request_fd == request_fd)
+		...
+
+Example for a Simple Capture Device
+-----------------------------------
+
+With a simple capture device, requests can be used to specify controls to apply
+to a given CAPTURE buffer. The driver will apply these controls before producing
+the marked CAPTURE buffer.
+
+	struct v4l2_buf buf;
+	struct v4l2_ext_controls ctrls;
+	...
+	ctrls.request_fd = req_fd;
+	...
+	ioctl(camera_fd, VIDIOC_S_EXT_CTRLS, &ctrls);
+	...
+	buf.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
+	buf.request_fd = req_fd;
+	ioctl(camera_fd, VIDIOC_QBUF, &buf);
+
+Once the request is fully prepared, it can be submitted to the driver:
+
+	struct media_request_cmd cmd;
+
+	memset(&cmd, 0, sizeof(cmd));
+	cmd.cmd = MEDIA_REQ_CMD_SUBMIT;
+	cmd.fd = request_fd;
+	ioctl(media_fd, MEDIA_IOC_REQUEST_CMD, &cmd);
+
+User-space can then lookup the request_fd field of dequeued capture buffers to
+confirm which one has been produced by the request.
+
+	struct v4l2_buf buf;
+
+	memset(&buf, 0, sizeof(buf));
+	buf.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
+	ioctl(camera_fd, VIDIOC_DQBUF, &buf);
+
+	if (buf.request_fd == request_fd)
+		...
diff --git a/Documentation/media/uapi/v4l/vidioc-qbuf.rst b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
index 9e448a4aa3aa..0cd596bd4cde 100644
--- a/Documentation/media/uapi/v4l/vidioc-qbuf.rst
+++ b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
@@ -98,6 +98,12 @@ dequeued, until the :ref:`VIDIOC_STREAMOFF <VIDIOC_STREAMON>` or
 :ref:`VIDIOC_REQBUFS` ioctl is called, or until the
 device is closed.
 
+For all buffer types, the ``request_fd`` field can be used when enqueing
+to specify the file descriptor of a request, if requests are in use.
+Setting it means that the buffer will not be passed to the driver until
+the request itself is submitted. Also, the driver will apply any setting
+associated with the request before processing the buffer.
+
 Applications call the ``VIDIOC_DQBUF`` ioctl to dequeue a filled
 (capturing) or displayed (output) buffer from the driver's outgoing
 queue. They just set the ``type``, ``memory`` and ``reserved`` fields of
@@ -115,6 +121,21 @@ queue. When the ``O_NONBLOCK`` flag was given to the
 :ref:`open() <func-open>` function, ``VIDIOC_DQBUF`` returns
 immediately with an ``EAGAIN`` error code when no buffer is available.
 
+If a dequeued buffer was produced as the result of a request, then the
+``request_fd`` field of :c:type:`v4l2_buffer` will be set to the file
+descriptor of the request, regardless of whether the buffer was initially
+queued with a request associated to it or not.
+
+RFC note: a request can be referenced by several FDs, especially if the
+request is shared between different processes. Also a FD can be closed
+after a request is submitted. In that case the FD does not make much sense.
+Maybe it would be better to define request fd as
+
+    union { u32 request_fd; u32 request_id; }
+
+and use request_id when communicating a request to userspace so they can be
+unambiguously referenced?
+
 The struct :c:type:`v4l2_buffer` structure is specified in
 :ref:`buffer`.
 
-- 
2.16.0.rc1.238.g530d649a79-goog

[RFC PATCH 7/8] media: vim2m: add media device

[Alexandre Courbot]

Request API requires a media node. Add one to the vim2m driver so we can
use requests with it.

Signed-off-by: Alexandre Courbot <acourbot@chromium.org>
---
 drivers/media/platform/vim2m.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/media/platform/vim2m.c b/drivers/media/platform/vim2m.c
index b01fba020d5f..a32e8a7950eb 100644
--- a/drivers/media/platform/vim2m.c
+++ b/drivers/media/platform/vim2m.c
@@ -140,6 +140,9 @@ static struct vim2m_fmt *find_format(struct v4l2_format *f)
 struct vim2m_dev {
 	struct v4l2_device	v4l2_dev;
 	struct video_device	vfd;
+#ifdef CONFIG_MEDIA_CONTROLLER
+	struct media_device	mdev;
+#endif
 
 	atomic_t		num_inst;
 	struct mutex		dev_mutex;
@@ -1001,6 +1004,13 @@ static int vim2m_probe(struct platform_device *pdev)
 
 	spin_lock_init(&dev->irqlock);
 
+#ifdef CONFIG_MEDIA_CONTROLLER
+	dev->mdev.dev = &pdev->dev;
+	strlcpy(dev->mdev.model, "vim2m", sizeof(dev->mdev.model));
+	media_device_init(&dev->mdev);
+	dev->v4l2_dev.mdev = &dev->mdev;
+#endif
+
 	ret = v4l2_device_register(&pdev->dev, &dev->v4l2_dev);
 	if (ret)
 		return ret;
@@ -1034,6 +1044,13 @@ static int vim2m_probe(struct platform_device *pdev)
 		goto err_m2m;
 	}
 
+#ifdef CONFIG_MEDIA_CONTROLLER
+	/* Register the media device node */
+	ret = media_device_register(&dev->mdev);
+	if (ret)
+		goto err_m2m;
+#endif
+
 	return 0;
 
 err_m2m:
@@ -1050,6 +1067,13 @@ static int vim2m_remove(struct platform_device *pdev)
 	struct vim2m_dev *dev = platform_get_drvdata(pdev);
 
 	v4l2_info(&dev->v4l2_dev, "Removing " MEM2MEM_NAME);
+
+#ifdef CONFIG_MEDIA_CONTROLLER
+	if (media_devnode_is_registered(dev->mdev.devnode))
+		media_device_unregister(&dev->mdev);
+	media_device_cleanup(&dev->mdev);
+#endif
+
 	v4l2_m2m_release(dev->m2m_dev);
 	del_timer_sync(&dev->timer);
 	video_unregister_device(&dev->vfd);
-- 
2.16.0.rc1.238.g530d649a79-goog

[RFC PATCH 8/8] media: vim2m: add request support

[Alexandre Courbot]

Set the necessary ops for supporting requests in vim2m.

Signed-off-by: Alexandre Courbot <acourbot@chromium.org>
---
 drivers/media/platform/vim2m.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/media/platform/vim2m.c b/drivers/media/platform/vim2m.c
index a32e8a7950eb..1ddbad5eb569 100644
--- a/drivers/media/platform/vim2m.c
+++ b/drivers/media/platform/vim2m.c
@@ -30,6 +30,8 @@
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-event.h>
 #include <media/videobuf2-vmalloc.h>
+#include <media/media-request.h>
+#include <media/media-request-mgr.h>
 
 MODULE_DESCRIPTION("Virtual device for mem2mem framework testing");
 MODULE_AUTHOR("Pawel Osciak, <pawel@osciak.com>");
@@ -370,6 +372,24 @@ static void job_abort(void *priv)
 	ctx->aborting = 1;
 }
 
+static void apply_request_params(struct media_request *req,
+				 struct vim2m_ctx *ctx)
+{
+	struct media_request_entity_data *data;
+
+	if (!req)
+		return;
+
+	data = media_request_get_entity_data(req, &ctx->dev->vfd.entity,
+					     &ctx->fh);
+	if (WARN_ON(!data))
+		return;
+
+	/* apply controls here */
+
+	data->applied = true;
+}
+
 /* device_run() - prepares and starts the device
  *
  * This simulates all the immediate preparations required before starting
@@ -385,8 +405,15 @@ static void device_run(void *priv)
 	src_buf = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
 	dst_buf = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx);
 
+	/* Apply request if needed */
+	apply_request_params(src_buf->vb2_buf.request, ctx);
+	WARN_ON(dst_buf->vb2_buf.request != NULL);
+
 	device_process(ctx, src_buf, dst_buf);
 
+	/* Inform user about which request produced the destination buffer */
+	dst_buf->request_fd = src_buf->request_fd;
+
 	/* Run a timer, which simulates a hardware irq  */
 	schedule_irq(dev, ctx->transtime);
 }
@@ -841,6 +868,7 @@ static int queue_init(void *priv, struct vb2_queue *src_vq, struct vb2_queue *ds
 	src_vq->mem_ops = &vb2_vmalloc_memops;
 	src_vq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
 	src_vq->lock = &ctx->dev->dev_mutex;
+	src_vq->allow_requests = true;
 
 	ret = vb2_queue_init(src_vq);
 	if (ret)
@@ -1006,6 +1034,9 @@ static int vim2m_probe(struct platform_device *pdev)
 
 #ifdef CONFIG_MEDIA_CONTROLLER
 	dev->mdev.dev = &pdev->dev;
+	dev->mdev.req_mgr = media_request_mgr_alloc(&dev->mdev);
+	if (IS_ERR(dev->mdev.req_mgr))
+		return PTR_ERR(dev->mdev.req_mgr);
 	strlcpy(dev->mdev.model, "vim2m", sizeof(dev->mdev.model));
 	media_device_init(&dev->mdev);
 	dev->v4l2_dev.mdev = &dev->mdev;
-- 
2.16.0.rc1.238.g530d649a79-goog

Re: [RFC PATCH 6/8] v4l2: document the request API interface

[Randy Dunlap]

On 01/25/2018 10:02 PM, Alexandre Courbot wrote:
> Document how the request API can be used along with the existing V4L2
> interface.
> 

> +Request API
> +===========
> +
> +The Request API has been designed to allow V4L2 to deal with requirements of
> +modern IPs (stateless codecs, MIPI cameras, ...) and APIs (Android Codec v2).

Hi,
Just a quick question:  What are IPs?

Not Internet Protocols. Hopefully not Intellectual Properties.
Maybe Intelligent Processors?

thanks,
-- 
~Randy

Re: [RFC PATCH 6/8] v4l2: document the request API interface

[Nicolas Dufresne]

Le vendredi 26 janvier 2018 à 12:40 -0800, Randy Dunlap a écrit :
> > +Request API
> > +===========
> > +
> > +The Request API has been designed to allow V4L2 to deal with
> > requirements of
> > +modern IPs (stateless codecs, MIPI cameras, ...) and APIs (Android
> > Codec v2).
> 
> Hi,
> Just a quick question:  What are IPs?
> 
> Not Internet Protocols. Hopefully not Intellectual Properties.
> Maybe Intelligent Processors?

Stands for "Intellectual Property". Used like this, I believe it's a
bit of a slang. It's also slightly pejorative as we often assume that
self contained "IP Cores" are proprietary black box. But considering
all the security issues that was found in these black boxes firmwares,
it's kind of fair criticism.

Nicolas

p.s. I'd propose to rephrase this in later version

Re: [RFC PATCH 0/8] [media] Request API, take three

[Hans Verkuil]

On 01/26/2018 07:02 AM, Alexandre Courbot wrote:
> Howdy. Here is your bi-weekly request API redesign! ;)
> 
> Again, this is a simple version that only implements the flow of requests,
> without applying controls. The intent is to get an agreement on a base to work
> on, since the previous versions went straight back to the redesign board.
> 
> Highlights of this version:
> 
> * As requested by Hans, request-bound buffers are now passed earlier to drivers,
> as early as the request itself is submitted. Doing it earlier is not be useful
> since the driver would not know the state of the request, and thus cannot do
> anything with the buffer. Drivers are now responsible for applying request
> parameters themselves.
> 
> * As a consequence, there is no such thing as a "request queue" anymore. The
> flow of buffers decides the order in which requests are processed. Individual
> devices of the same pipeline can implement synchronization if needed, but this
> is beyond this first version.
> 
> * VB2 code is still a bit shady. Some of it will interfere with the fences
> series, so I am waiting for the latter to land to do it properly.
> 
> * Requests that are not yet submitted effectively act as fences on the buffers
> they own, resulting in the buffer queue being blocked until the request is
> submitted. An alternate design would be to only block the
> not-submitted-request's buffer and let further buffers pass before it, but since
> buffer order is becoming increasingly important I have decided to just block the
> queue. This is open to discussion though.

I don't think we should mess with the order.

> 
> * Documentation! Also described usecases for codec and simple (i.e. not part of
> a complex pipeline) capture device.

I'll concentrate on reviewing that.

> 
> Still remaining to do:
> 
> * As pointed out by Hans on the previous revision, do not assume that drivers
> using v4l2_fh have a per-handle state. I have not yet found a good way to
> differenciate both usages.

I suspect we need to add a flag or something for this.

> * Integrate Hans' patchset for control handling: as said above, this is futile
> unless we can agree on the basic design, which I hope we can do this time.
> Chrome OS needs this really badly now and will have to move forward no matter
> what, so I hope this will be considered good enough for a common base of work.

I am not sure there is any reason to not move forward with the control handling.
You need this anyway IMHO, regardless of any public API considerations.

> A few thoughts/questions that emerged when writing this patchset:
> 
> * Since requests are exposed as file descriptors, we could easily move the
> MEDIA_REQ_CMD_SUBMIT and MEDIA_REQ_CMD_REININT commands as ioctls on the
> requests themselves, instead of having to perform them on the media device that
> provided the request in the first place. That would add a bit more flexibility
> if/when passing requests around and means the media device only needs to handle
> MEDIA_REQ_CMD_ALLOC. Conceptually speaking, this seems to make sense to me.
> Any comment for/against that?

Makes sense IMHO.

> * For the codec usecase, I have experimented a bit marking CAPTURE buffers with
> the request FD that produced them (vim2m acts that way). This seems to work
> well, however FDs are process-local and could be closed before the CAPTURE
> buffer is dequeued, rendering that information less meaningful, if not
> dangerous.

I don't follow this. Once the fd is passed to the kernel its refcount should be
increased so the data it represents won't be released if userspace closes the fd.

Obviously if userspace closes the fd it cannot do anything with it anymore, but
it shouldn't be 'dangerous' AFAICT.

> Wouldn't it be better/safer to use a global request ID for
> such information instead? That ID would be returned upon MEDIA_REQ_CMD_ALLOC so
> user-space knows which request ID a FD refers to.

I think it is not a good idea to have both an fd and an ID to refer to the
same object. That's going to be very confusing I think.

> * Using the media node to provide requests makes absolute sense for complex
> camera devices, but is tedious for codec devices which work on one node and
> require to protect request/media related code with #ifdef
> CONFIG_MEDIA_CONTROLLER.

Why? They would now depend on MEDIA_CONTROLLER (i.e. they won't appear in the
menuconfig unless MEDIA_CONTROLLER is set). No need for an #ifdef.

 For these devices, the sole role of the media node is
> to produce the request, and that's it (since request submission could be moved
> to the request FD as explained above). That's a modest use that hardly justifies
> bringing in the whole media framework IMHO. With a bit of extra abstraction, it
> should be possible to decouple the base requests from the media controller
> altogether, and propose two kinds of requests implementations: one simpler
> implementation that works directly with a single V4L2 node (useful for codecs),
> and another one that works on a media node and can control all its entities
> (good for camera). This would allow codecs to use the request API without
> forcing the media controller, and would considerably simplify the use-case. Any
> objection to that? IIRC the earlier design documents mentioned this possibility.

I think this is an interesting idea, but I would postpone making a decision on
this until later. We need this MC support regardless, so let's start off with that.

Once that's working we can discuss if we should or should not create a shortcut
for codecs. Trying to decide this now would only confuse the process.

Regards,

	Hans

> 
> Alexandre Courbot (6):
>   media: add request API core and UAPI
>   media: videobuf2: add support for requests
>   media: vb2: add support for requests in QBUF ioctl
>   v4l2: document the request API interface
>   media: vim2m: add media device
>   media: vim2m: add request support
> 
> Hans Verkuil (1):
>   videodev2.h: Add request field to v4l2_buffer
> 
> Laurent Pinchart (1):
>   media: Document the media request API
> 
>  Documentation/media/uapi/mediactl/media-funcs.rst  |   1 +
>  .../media/uapi/mediactl/media-ioc-request-cmd.rst  | 140 ++++++++++
>  Documentation/media/uapi/v4l/buffer.rst            |  10 +-
>  Documentation/media/uapi/v4l/common.rst            |   1 +
>  Documentation/media/uapi/v4l/request-api.rst       | 194 +++++++++++++
>  Documentation/media/uapi/v4l/vidioc-qbuf.rst       |  21 ++
>  drivers/media/Makefile                             |   3 +-
>  drivers/media/media-device.c                       |   7 +
>  drivers/media/media-request-mgr.c                  | 107 +++++++
>  drivers/media/media-request.c                      | 308 +++++++++++++++++++++
>  drivers/media/platform/vim2m.c                     |  55 ++++
>  drivers/media/usb/cpia2/cpia2_v4l.c                |   2 +-
>  drivers/media/v4l2-core/v4l2-compat-ioctl32.c      |   7 +-
>  drivers/media/v4l2-core/v4l2-ioctl.c               |  85 +++++-
>  drivers/media/v4l2-core/videobuf2-core.c           | 125 ++++++++-
>  drivers/media/v4l2-core/videobuf2-v4l2.c           |  31 ++-
>  include/media/media-device.h                       |   3 +
>  include/media/media-request-mgr.h                  |  73 +++++
>  include/media/media-request.h                      | 184 ++++++++++++
>  include/media/videobuf2-core.h                     |  15 +-
>  include/media/videobuf2-v4l2.h                     |   2 +
>  include/uapi/linux/media.h                         |  10 +
>  include/uapi/linux/videodev2.h                     |   3 +-
>  23 files changed, 1365 insertions(+), 22 deletions(-)
>  create mode 100644 Documentation/media/uapi/mediactl/media-ioc-request-cmd.rst
>  create mode 100644 Documentation/media/uapi/v4l/request-api.rst
>  create mode 100644 drivers/media/media-request-mgr.c
>  create mode 100644 drivers/media/media-request.c
>  create mode 100644 include/media/media-request-mgr.h
>  create mode 100644 include/media/media-request.h
> 

Re: [RFC PATCH 6/8] v4l2: document the request API interface

[Hans Verkuil]

On 01/26/2018 07:02 AM, Alexandre Courbot wrote:
> Document how the request API can be used along with the existing V4L2
> interface.
> 
> Signed-off-by: Alexandre Courbot <acourbot@chromium.org>
> ---
>  Documentation/media/uapi/v4l/buffer.rst      |  10 +-
>  Documentation/media/uapi/v4l/common.rst      |   1 +
>  Documentation/media/uapi/v4l/request-api.rst | 194 +++++++++++++++++++++++++++
>  Documentation/media/uapi/v4l/vidioc-qbuf.rst |  21 +++
>  4 files changed, 223 insertions(+), 3 deletions(-)
>  create mode 100644 Documentation/media/uapi/v4l/request-api.rst
> 
> diff --git a/Documentation/media/uapi/v4l/buffer.rst b/Documentation/media/uapi/v4l/buffer.rst
> index ae6ee73f151c..9d082784081d 100644
> --- a/Documentation/media/uapi/v4l/buffer.rst
> +++ b/Documentation/media/uapi/v4l/buffer.rst
> @@ -301,10 +301,14 @@ struct v4l2_buffer
>  	elements in the ``planes`` array. The driver will fill in the
>  	actual number of valid elements in that array.
>      * - __u32
> -      - ``reserved2``
> +      - ``request_fd``
>        -
> -      - A place holder for future extensions. Drivers and applications
> -	must set this to 0.
> +      - The file descriptor of the request associated with this buffer.
> +	user-space can set this when calling :ref:`VIDIOC_QBUF`, and drivers
> +	will return the request used when processing a buffer (if any) upon
> +	:ref:`VIDIOC_DQBUF`.
> +
> +	A value of 0 means the buffer is not associated with any request.
>      * - __u32
>        - ``reserved``
>        -
> diff --git a/Documentation/media/uapi/v4l/common.rst b/Documentation/media/uapi/v4l/common.rst
> index 13f2ed3fc5a6..a4aa0059d45a 100644
> --- a/Documentation/media/uapi/v4l/common.rst
> +++ b/Documentation/media/uapi/v4l/common.rst
> @@ -44,3 +44,4 @@ applicable to all devices.
>      crop
>      selection-api
>      streaming-par
> +    request-api
> diff --git a/Documentation/media/uapi/v4l/request-api.rst b/Documentation/media/uapi/v4l/request-api.rst
> new file mode 100644
> index 000000000000..a758a5fd3ca0
> --- /dev/null
> +++ b/Documentation/media/uapi/v4l/request-api.rst
> @@ -0,0 +1,194 @@
> +.. -*- coding: utf-8; mode: rst -*-
> +
> +.. _media-request-api:
> +
> +Request API
> +===========
> +
> +The Request API has been designed to allow V4L2 to deal with requirements of
> +modern IPs (stateless codecs, MIPI cameras, ...) and APIs (Android Codec v2).
> +One such requirement is the ability for devices belonging to the same pipeline
> +to reconfigure and collaborate closely on a per-frame basis. Another is
> +efficient support of stateless codecs, which need per-frame controls to be set
> +asynchronously in order to be efficiently used.
> +
> +Supporting these features without the Request API is possible but terribly
> +inefficient: user-space would have to flush all activity on the media pipeline,
> +reconfigure it for the next frame, queue the buffers to be processed with that
> +configuration, and wait until they are all available for dequeing before
> +considering the next frame. This defeats the purpose of having buffer queues
> +since in practice only one buffer would be queued at a time.
> +
> +The Request API allows a specific configuration of the pipeline (media
> +controller topology + controls for each device) to be associated with specific
> +buffers. The parameters are applied by each participating device as buffers
> +associated to a request flow in. This allows user-space to schedule several
> +tasks ("requests") with different parameters in advance, knowing that the
> +parameters will be applied when needed to get the expected result. Controls
> +values at the time of request completion are also available for reading.
> +
> +Usage
> +=====
> +
> +The Request API is used on top of standard media controller and V4L2 calls,
> +which are augmented with an extra ``request_fd`` parameter. All operations on
> +requests themselves are performed using the command parameter of the
> +:c:func:`MEDIA_IOC_REQUEST_CMD` ioctl.
> +
> +Allocation
> +----------
> +
> +User-space allocates requests using the ``MEDIA_REQ_CMD_ALLOC`` command on
> +an opened media device. This returns a file descriptor representing the
> +request. Typically, several such requests will be allocated.
> +
> +Request Preparation
> +-------------------
> +
> +Standard V4L2 ioctls can then receive a request file descriptor to express the
> +fact that the ioctl is part of said request, and is not to be applied
> +immediately. V4L2 ioctls supporting this are :c:func:`VIDIOC_S_EXT_CTRLS` and
> +:c:func:`VIDIOC_QBUF`. Controls set with a request parameter are stored instead
> +of being immediately applied, and queued buffers will block the buffer queue
> +until the request becomes active.
> +
> +RFC Note: currently several buffers can be queued to the same queue with the
> +same request. The request parameters will be only be applied when processing
> +the first buffer. Does it make more sense to allow at most one buffer per
> +request per queue instead?

I think we should initially limit us to at most one buffer per queue per
request. If you don't want to make changes just queue a buffer without a
request fd, or possibly with an empty request (that needs to be defined!).

> +
> +Request Submission
> +------------------
> +
> +Once the parameters and buffers of the request are specified, it can be
> +submitted with the ``MEDIA_REQ_CMD_SUBMIT`` command. This will make the buffers
> +associated to the request available to their driver, which can then apply the
> +saved controls as buffers are processed. A submitted request cannot be used
> +with further :c:func:`VIDIOC_S_EXT_CTRLS` and :c:func:`VIDIOC_QBUF` ioctls.

I'd just keep this more general:

"A request cannot be modified anymore once it has been submitted."

> +
> +If several devices are part of the request, individual drivers may synchronize
> +so the requested pipeline's topology is applied before the buffers are
> +processed. This is at the discretion of the drivers and is not a requirement.
> +
> +Buffers queued without an associated request after a request-bound buffer will
> +be processed using the state of the hardware at the time of the request
> +completion. All the same, controls set without a request are applied
> +immediately, regardless of whether a request is in use or not.

Ah, OK. I agree.

> +
> +User-space can ``poll()`` a request FD in order to wait until the request
> +completes. A request is considered complete once all its associated buffers are
> +available for dequeing. Note that user-space does not need to wait for the
> +request to complete to dequeue its buffers: buffers that are available halfway
> +through a request can be dequeued independently of the request's state.
> +
> +A completed request includes the state of all devices that had queued buffers
> +associated with it at the time of the request completion. User-space can query
> +that state by calling :c:func:`VIDIOC_G_EXT_CTRLS` with the request FD.
> +
> +Recycling and Destruction
> +-------------------------
> +
> +Finally, completed request can either be discarded or be reused. Calling
> +``close()`` on a request FD will make that FD unusable, freeing the request if
> +it is not referenced elsewhere. The ``MEDIA_REQ_CMD_REINIT`` command will clear
> +a request's state and make it available again. No state is retained by this
> +operation: the request is as if it had just been allocated via
> +``MEDIA_REQ_CMD_ALLOC``.
> +
> +RFC Note: Since requests are represented by FDs themselves, the
> +``MEDIA_REQ_CMD_SUBMIT`` and ``MEDIA_REQ_CMD_REININT`` commands can be performed
> +on the request FD instead of the media device. This means the media device would
> +only need to manage ``MEDIA_REQ_CMD_ALLOC``, which could be turned into an
> +ioctl, while ``MEDIA_REQ_CMD_SUBMIT`` and ``MEDIA_REQ_CMD_REININT`` would
> +become ioctls on the request itself. This has the advantage of allowing
> +receivers of a request FD to submit the request, and also decouples requests
> +from the media device - a scenario that makes sense for stateless codecs where
> +the media device is not really useful.

I think allowing SUBMIT/REINIT to operate directly on the request fd makes sense.

A few questions that I haven't seen answered here:

- can I reinit a request if it hasn't yet been submitted? Or can this only be
  done on completed requests?
- can I reinit (or perhaps also alloc) a request based on an existing request?
  (sort of a clone operation). I can't remember what we decided in the Prague
  meeting, sorry about that.
- a request effectively contains the things you want to change, so anything not
  explicitly set in the request will have what value? This should be the current
  hardware value with any changes specified in preceding requests on top.
- add an RFC note that requests do not have to complete in the same order that they
  were queued. A codec driver can (at least in theory) hold on to a buffer
  containing a key frame until that frame is no longer needed. But this also
  relates to the fences support, so let's keep this an RFC for now until that
  patch series has been merged.
- how do we know that the request API is supported by a driver? I think we
  wanted a new capability for that?
- it is allowed to query the values of a submitted request, or will that return
  -EBUSY while it is not completed? I see no reason not to allow it.
- what happens when a value inside a request can't be applied? (e.g. due to a
  hardware error or some other unusual situation).

> +
> +Example for a Codec Device
> +--------------------------
> +
> +Codecs are single-node V4L2 devices providing one OUTPUT queue (for user-space
> +to provide input buffers) and one CAPTURE queue (to retrieve processed data).
> +
> +In this use-case, request API is used to associate specific controls to be
> +applied by the driver before processing a buffer from the OUTPUT queue. When
> +retrieving a buffer from a capture queue, user-space can then identify which
> +request, if any, produced it by looking at the request_fd field of the dequeued
> +v4l2_buffer.

I'd start with the code to allocate a request.

> +Put into code, after obtaining a request, user-space can assign controls and one
> +OUTPUT buffer to it:
> +
> +	struct v4l2_buf buf;
> +	struct v4l2_ext_controls ctrls;
> +	...
> +	ctrls.request_fd = req_fd;
> +	...
> +	ioctl(codec_fd, VIDIOC_S_EXT_CTRLS, &ctrls);
> +	...
> +	buf.type = V4L2_BUF_TYPE_VIDEO_OUTPUT;
> +	buf.request_fd = req_fd;
> +	ioctl(codec_fd, VIDIOC_QBUF, &buf);
> +
> +Note that request_fd shall not be specified for CAPTURE buffers.

I know why, but the less experienced reader won't. So this needs some additional
explanation.

> +
> +Once the request is fully prepared, it can be submitted to the driver:
> +
> +	struct media_request_cmd cmd;
> +
> +	memset(&cmd, 0, sizeof(cmd));
> +	cmd.cmd = MEDIA_REQ_CMD_SUBMIT;
> +	cmd.fd = request_fd;
> +	ioctl(media_fd, MEDIA_IOC_REQUEST_CMD, &cmd);
> +
> +User-space can then lookup the request_fd field of dequeued capture buffers to
> +confirm which one has been produced by the request.
> +
> +	struct v4l2_buf buf;
> +
> +	memset(&buf, 0, sizeof(buf));
> +	buf.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> +	ioctl(codec_fd, VIDIOC_DQBUF, &buf);
> +
> +	if (buf.request_fd == request_fd)
> +		...
> +

I'm not sure this is correct. You set the request for the OUTPUT buffer, so when
you dequeue the OUTPUT buffer, then the request_fd is set to the original request.
But this is the CAPTURE buffer, and that has no requests at all.

BTW, you also want to show an example of getting the control once the request is
completed (e.g. status information), probably with a polling example as well.

> +Example for a Simple Capture Device
> +-----------------------------------
> +
> +With a simple capture device, requests can be used to specify controls to apply
> +to a given CAPTURE buffer. The driver will apply these controls before producing
> +the marked CAPTURE buffer.
> +
> +	struct v4l2_buf buf;
> +	struct v4l2_ext_controls ctrls;
> +	...
> +	ctrls.request_fd = req_fd;
> +	...
> +	ioctl(camera_fd, VIDIOC_S_EXT_CTRLS, &ctrls);
> +	...
> +	buf.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> +	buf.request_fd = req_fd;
> +	ioctl(camera_fd, VIDIOC_QBUF, &buf);
> +
> +Once the request is fully prepared, it can be submitted to the driver:
> +
> +	struct media_request_cmd cmd;
> +
> +	memset(&cmd, 0, sizeof(cmd));
> +	cmd.cmd = MEDIA_REQ_CMD_SUBMIT;
> +	cmd.fd = request_fd;
> +	ioctl(media_fd, MEDIA_IOC_REQUEST_CMD, &cmd);
> +
> +User-space can then lookup the request_fd field of dequeued capture buffers to
> +confirm which one has been produced by the request.
> +
> +	struct v4l2_buf buf;
> +
> +	memset(&buf, 0, sizeof(buf));
> +	buf.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> +	ioctl(camera_fd, VIDIOC_DQBUF, &buf);
> +
> +	if (buf.request_fd == request_fd)
> +		...
> diff --git a/Documentation/media/uapi/v4l/vidioc-qbuf.rst b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
> index 9e448a4aa3aa..0cd596bd4cde 100644
> --- a/Documentation/media/uapi/v4l/vidioc-qbuf.rst
> +++ b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
> @@ -98,6 +98,12 @@ dequeued, until the :ref:`VIDIOC_STREAMOFF <VIDIOC_STREAMON>` or
>  :ref:`VIDIOC_REQBUFS` ioctl is called, or until the
>  device is closed.
>  
> +For all buffer types, the ``request_fd`` field can be used when enqueing
> +to specify the file descriptor of a request, if requests are in use.
> +Setting it means that the buffer will not be passed to the driver until
> +the request itself is submitted. Also, the driver will apply any setting
> +associated with the request before processing the buffer.
> +
>  Applications call the ``VIDIOC_DQBUF`` ioctl to dequeue a filled
>  (capturing) or displayed (output) buffer from the driver's outgoing
>  queue. They just set the ``type``, ``memory`` and ``reserved`` fields of
> @@ -115,6 +121,21 @@ queue. When the ``O_NONBLOCK`` flag was given to the
>  :ref:`open() <func-open>` function, ``VIDIOC_DQBUF`` returns
>  immediately with an ``EAGAIN`` error code when no buffer is available.
>  
> +If a dequeued buffer was produced as the result of a request, then the
> +``request_fd`` field of :c:type:`v4l2_buffer` will be set to the file
> +descriptor of the request, regardless of whether the buffer was initially
> +queued with a request associated to it or not.
> +
> +RFC note: a request can be referenced by several FDs, especially if the
> +request is shared between different processes. Also a FD can be closed
> +after a request is submitted. In that case the FD does not make much sense.
> +Maybe it would be better to define request fd as
> +
> +    union { u32 request_fd; u32 request_id; }
> +
> +and use request_id when communicating a request to userspace so they can be
> +unambiguously referenced?

I don't think this is a problem. At least we never had this issue with e.g.
dmabuf either. The request_fd must be a valid request fd for the process that
calls the ioctl. If not the ioctl will return an error. If process A has a
request fd 10 and process B calls the ioctl with request_fd set to 10 but it
didn't actually allocate the request or receive the fd from process A (there
is a mechanism for that, although I've forgotten the details).

> +
>  The struct :c:type:`v4l2_buffer` structure is specified in
>  :ref:`buffer`.
>  
> 

Regards,

	Hans

Re: [RFC PATCH 5/8] media: Document the media request API

[Hans Verkuil]

On 01/26/2018 07:02 AM, Alexandre Courbot wrote:
> From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> 
> The media request API is made of a new ioctl to implement request
> management. Document it.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> [acourbot@chromium.org: adapt for newest API]
> Signed-off-by: Alexandre Courbot <acourbot@chromium.org>
> ---
>  Documentation/media/uapi/mediactl/media-funcs.rst  |   1 +
>  .../media/uapi/mediactl/media-ioc-request-cmd.rst  | 140 +++++++++++++++++++++
>  2 files changed, 141 insertions(+)
>  create mode 100644 Documentation/media/uapi/mediactl/media-ioc-request-cmd.rst
> 
> diff --git a/Documentation/media/uapi/mediactl/media-funcs.rst b/Documentation/media/uapi/mediactl/media-funcs.rst
> index 076856501cdb..e3a45d82ffcb 100644
> --- a/Documentation/media/uapi/mediactl/media-funcs.rst
> +++ b/Documentation/media/uapi/mediactl/media-funcs.rst
> @@ -15,4 +15,5 @@ Function Reference
>      media-ioc-g-topology
>      media-ioc-enum-entities
>      media-ioc-enum-links
> +    media-ioc-request-cmd
>      media-ioc-setup-link
> diff --git a/Documentation/media/uapi/mediactl/media-ioc-request-cmd.rst b/Documentation/media/uapi/mediactl/media-ioc-request-cmd.rst
> new file mode 100644
> index 000000000000..17223e8170e9
> --- /dev/null
> +++ b/Documentation/media/uapi/mediactl/media-ioc-request-cmd.rst
> @@ -0,0 +1,140 @@
> +.. -*- coding: utf-8; mode: rst -*-
> +
> +.. _media_ioc_request_cmd:
> +
> +***************************
> +ioctl MEDIA_IOC_REQUEST_CMD
> +***************************
> +
> +Name
> +====
> +
> +MEDIA_IOC_REQUEST_CMD - Manage media device requests
> +
> +
> +Synopsis
> +========
> +
> +.. c:function:: int ioctl( int fd, MEDIA_IOC_REQUEST_CMD, struct media_request_cmd *argp )
> +    :name: MEDIA_IOC_REQUEST_CMD
> +
> +
> +Arguments
> +=========
> +
> +``fd``
> +    File descriptor returned by :ref:`open() <media-func-open>`.
> +
> +``argp``
> +
> +
> +Description
> +===========
> +
> +The MEDIA_IOC_REQUEST_CMD ioctl allow applications to manage media device
> +requests. A request is an object that can group media device configuration
> +parameters, including subsystem-specific parameters, in order to apply all the
> +parameters atomically. Applications are responsible for allocating and
> +deleting requests, filling them with configuration parameters submitting them.
> +
> +Request operations are performed by calling the MEDIA_IOC_REQUEST_CMD ioctl
> +with a pointer to a struct :c:type:`media_request_cmd` with the cmd field set
> +to the appropriate command. :ref:`media-request-command` lists the commands
> +supported by the ioctl.
> +
> +The struct :c:type:`media_request_cmd` request field contains the file
> +descriptorof the request on which the command operates. For the
> +``MEDIA_REQ_CMD_ALLOC`` command the field is set to zero by applications and
> +filled by the driver. For all other commands the field is set by applications
> +and left untouched by the driver.
> +
> +To allocate a new request applications use the ``MEDIA_REQ_CMD_ALLOC``
> +command. The driver will allocate a new request and return its ID in the

ID -> FD

> +request field.
> +
> +Requests are reference-counted. A newly allocated request is referenced
> +by the returned file descriptor, and can be later referenced by
> +subsystem-specific operations. Requests will thus be automatically deleted
> +when they're not longer used after the returned file descriptor is closed.
> +
> +If a request isn't needed applications can delete it by calling ``close()``
> +on it. The driver will drop the file handle reference. The request will not
> +be usable through the MEDIA_IOC_REQUEST_CMD ioctl anymore, but will only be
> +deleted when the last reference is released. If no other reference exists when
> +``close()`` is invoked the request will be deleted immediately.
> +
> +After creating a request applications should fill it with configuration
> +parameters. This is performed through subsystem-specific request APIs outside
> +the scope of the media controller API. See the appropriate subsystem APIs for
> +more information, including how they interact with the MEDIA_IOC_REQUEST_CMD
> +ioctl.
> +
> +Once a request contains all the desired configuration parameters it can be
> +submitted using the ``MEDIA_REQ_CMD_SUBMIT`` command. This will let the
> +buffers queued for the request be passed to their respective drivers, which
> +will then apply the request's parameters before processing them.
> +
> +Once a request has been queued applications are not allowed to modify its
> +configuration parameters until the request has been fully processed. Any
> +attempt to do so will result in the related subsystem API returning an error.
> +The application that submitted the request can wait for its completion by
> +polling on the request's file descriptor.
> +
> +Once a request has completed, it can be reused. The ``MEDIA_REQ_CMD_REINIT``
> +command will bring it back to its initial state, so it can be prepared and
> +submitted again.
> +
> +.. c:type:: media_request_cmd
> +
> +.. flat-table:: struct media_request_cmd
> +    :header-rows:  0
> +    :stub-columns: 0
> +    :widths: 1 2 8
> +
> +    * - __u32
> +      - ``cmd``
> +      - Command, set by the application. See below for the list of supported
> +        commands.
> +    * - __u32
> +      - ``fd``
> +      - Request FD, set by the driver for the MEDIA_REQ_CMD_ALLOC command and
> +        by the application for all other commands.
> +
> +
> +.. _media-request-command:
> +
> +.. cssclass:: longtable
> +
> +.. flat-table:: Media request commands
> +    :header-rows:  0
> +    :stub-columns: 0
> +
> +    * .. _MEDIA-REQ-CMD-ALLOC:
> +
> +      - ``MEDIA_REQ_CMD_ALLOC``
> +      - Allocate a new request.
> +    * .. _MEDIA-REQ-CMD-SUBMIT:
> +
> +      - ``MEDIA_REQ_CMD_SUBMIT``
> +      - Submit a request to be processed.
> +    * .. _MEDIA-REQ-CMD-QUEUE:
> +
> +      - ``MEDIA_REQ_CMD_REINIT``
> +      - Reinitializes a completed request.
> +
> +
> +Return Value
> +============
> +
> +On success 0 is returned, on error -1 and the ``errno`` variable is set
> +appropriately. The generic error codes are described at the
> +:ref:`Generic Error Codes <gen-errors>` chapter.
> +
> +EINVAL
> +    The struct :c:type:`media_request_cmd` specifies an invalid command or
> +    references a non-existing request.
> +
> +ENOSYS
> +    The struct :c:type:`media_request_cmd` specifies the
> +    ``MEDIA_REQ_CMD_QUEUE`` or ``MEDIA_REQ_CMD_APPLY`` command and that

MEDIA_REQ_CMD_APPLY isn't defined in the table above.

> +    command isn't implemented by the device.
> 


So what is missing from this documentation is whether a newly created request
has a copy of the current state or if it is empty. And didn't we allow a
request to be based on an other existing request? Unfortunately I have no
time to dig up that information at the moment.

More comments in my review for 6/8. Probably best to read that review
first before this one.

Regards,

	Hans

Re: [RFC PATCH 0/8] [media] Request API, take three

[Alexandre Courbot]

Hi Hans,

On Mon, Jan 29, 2018 at 8:21 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> On 01/26/2018 07:02 AM, Alexandre Courbot wrote:
>> Howdy. Here is your bi-weekly request API redesign! ;)
>>
>> Again, this is a simple version that only implements the flow of requests,
>> without applying controls. The intent is to get an agreement on a base to work
>> on, since the previous versions went straight back to the redesign board.
>>
>> Highlights of this version:
>>
>> * As requested by Hans, request-bound buffers are now passed earlier to drivers,
>> as early as the request itself is submitted. Doing it earlier is not be useful
>> since the driver would not know the state of the request, and thus cannot do
>> anything with the buffer. Drivers are now responsible for applying request
>> parameters themselves.
>>
>> * As a consequence, there is no such thing as a "request queue" anymore. The
>> flow of buffers decides the order in which requests are processed. Individual
>> devices of the same pipeline can implement synchronization if needed, but this
>> is beyond this first version.
>>
>> * VB2 code is still a bit shady. Some of it will interfere with the fences
>> series, so I am waiting for the latter to land to do it properly.
>>
>> * Requests that are not yet submitted effectively act as fences on the buffers
>> they own, resulting in the buffer queue being blocked until the request is
>> submitted. An alternate design would be to only block the
>> not-submitted-request's buffer and let further buffers pass before it, but since
>> buffer order is becoming increasingly important I have decided to just block the
>> queue. This is open to discussion though.
>
> I don't think we should mess with the order.

Agreed, let's keep it that way then.

>
>>
>> * Documentation! Also described usecases for codec and simple (i.e. not part of
>> a complex pipeline) capture device.
>
> I'll concentrate on reviewing that.
>
>>
>> Still remaining to do:
>>
>> * As pointed out by Hans on the previous revision, do not assume that drivers
>> using v4l2_fh have a per-handle state. I have not yet found a good way to
>> differenciate both usages.
>
> I suspect we need to add a flag or something for this.

I hope we don't need to, let's see if I can find a pattern...

>
>> * Integrate Hans' patchset for control handling: as said above, this is futile
>> unless we can agree on the basic design, which I hope we can do this time.
>> Chrome OS needs this really badly now and will have to move forward no matter
>> what, so I hope this will be considered good enough for a common base of work.
>
> I am not sure there is any reason to not move forward with the control handling.
> You need this anyway IMHO, regardless of any public API considerations.

Only reasons are my lazyness and because I wanted to focus on the
request flow first. But you're right. I have a version with your
control framework changes integrated (they worked on the first attempt
btw! :)), let me create a clean patchset from this and send another
RFC.

>
>> A few thoughts/questions that emerged when writing this patchset:
>>
>> * Since requests are exposed as file descriptors, we could easily move the
>> MEDIA_REQ_CMD_SUBMIT and MEDIA_REQ_CMD_REININT commands as ioctls on the
>> requests themselves, instead of having to perform them on the media device that
>> provided the request in the first place. That would add a bit more flexibility
>> if/when passing requests around and means the media device only needs to handle
>> MEDIA_REQ_CMD_ALLOC. Conceptually speaking, this seems to make sense to me.
>> Any comment for/against that?
>
> Makes sense IMHO.

Glad to hear it, that was my preferred design. :)

>
>> * For the codec usecase, I have experimented a bit marking CAPTURE buffers with
>> the request FD that produced them (vim2m acts that way). This seems to work
>> well, however FDs are process-local and could be closed before the CAPTURE
>> buffer is dequeued, rendering that information less meaningful, if not
>> dangerous.
>
> I don't follow this. Once the fd is passed to the kernel its refcount should be
> increased so the data it represents won't be released if userspace closes the fd.

The refcount of the request is increased. The refcount of the FD is
not, since it is only a userspace reference to the request.

>
> Obviously if userspace closes the fd it cannot do anything with it anymore, but
> it shouldn't be 'dangerous' AFAICT.

It userspace later gets that closed FD back from a DQBUF call, and
decides to use it again, then we would have a problem. I agree that it
is userspace responsibility to be careful here, but making things
foolproof never hurts.

>
>> Wouldn't it be better/safer to use a global request ID for
>> such information instead? That ID would be returned upon MEDIA_REQ_CMD_ALLOC so
>> user-space knows which request ID a FD refers to.
>
> I think it is not a good idea to have both an fd and an ID to refer to the
> same object. That's going to be very confusing I think.

IDs would not refer to the object, they would just be a way to
identify it. FDs would be the actual reference.

If we drop the idea of returning request FDs to userspace after the
initial allocation (which is the only time we can be sure that a
returned FD is valid), then this is not a problem anymore, but I think
it may be useful to mark CAPTURE buffers with the request that
generated them.

>
>> * Using the media node to provide requests makes absolute sense for complex
>> camera devices, but is tedious for codec devices which work on one node and
>> require to protect request/media related code with #ifdef
>> CONFIG_MEDIA_CONTROLLER.
>
> Why? They would now depend on MEDIA_CONTROLLER (i.e. they won't appear in the
> menuconfig unless MEDIA_CONTROLLER is set). No need for an #ifdef.

Ah, if we make them depend on MEDIA_CONTROLLER, then indeed. But do we
want to do this for e.g. vim2m and vivid?

>
>  For these devices, the sole role of the media node is
>> to produce the request, and that's it (since request submission could be moved
>> to the request FD as explained above). That's a modest use that hardly justifies
>> bringing in the whole media framework IMHO. With a bit of extra abstraction, it
>> should be possible to decouple the base requests from the media controller
>> altogether, and propose two kinds of requests implementations: one simpler
>> implementation that works directly with a single V4L2 node (useful for codecs),
>> and another one that works on a media node and can control all its entities
>> (good for camera). This would allow codecs to use the request API without
>> forcing the media controller, and would considerably simplify the use-case. Any
>> objection to that? IIRC the earlier design documents mentioned this possibility.
>
> I think this is an interesting idea, but I would postpone making a decision on
> this until later. We need this MC support regardless, so let's start off with that.
>
> Once that's working we can discuss if we should or should not create a shortcut
> for codecs. Trying to decide this now would only confuse the process.

Sounds good, as long as we make a decision before merging.

Thanks,
Alex.

Re: [RFC PATCH 6/8] v4l2: document the request API interface

[Alexandre Courbot]

On Tue, Jan 30, 2018 at 1:03 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> On 01/26/2018 07:02 AM, Alexandre Courbot wrote:
>> Document how the request API can be used along with the existing V4L2
>> interface.
>>
>> Signed-off-by: Alexandre Courbot <acourbot@chromium.org>
>> ---
>>  Documentation/media/uapi/v4l/buffer.rst      |  10 +-
>>  Documentation/media/uapi/v4l/common.rst      |   1 +
>>  Documentation/media/uapi/v4l/request-api.rst | 194 +++++++++++++++++++++++++++
>>  Documentation/media/uapi/v4l/vidioc-qbuf.rst |  21 +++
>>  4 files changed, 223 insertions(+), 3 deletions(-)
>>  create mode 100644 Documentation/media/uapi/v4l/request-api.rst
>>
>> diff --git a/Documentation/media/uapi/v4l/buffer.rst b/Documentation/media/uapi/v4l/buffer.rst
>> index ae6ee73f151c..9d082784081d 100644
>> --- a/Documentation/media/uapi/v4l/buffer.rst
>> +++ b/Documentation/media/uapi/v4l/buffer.rst
>> @@ -301,10 +301,14 @@ struct v4l2_buffer
>>       elements in the ``planes`` array. The driver will fill in the
>>       actual number of valid elements in that array.
>>      * - __u32
>> -      - ``reserved2``
>> +      - ``request_fd``
>>        -
>> -      - A place holder for future extensions. Drivers and applications
>> -     must set this to 0.
>> +      - The file descriptor of the request associated with this buffer.
>> +     user-space can set this when calling :ref:`VIDIOC_QBUF`, and drivers
>> +     will return the request used when processing a buffer (if any) upon
>> +     :ref:`VIDIOC_DQBUF`.
>> +
>> +     A value of 0 means the buffer is not associated with any request.
>>      * - __u32
>>        - ``reserved``
>>        -
>> diff --git a/Documentation/media/uapi/v4l/common.rst b/Documentation/media/uapi/v4l/common.rst
>> index 13f2ed3fc5a6..a4aa0059d45a 100644
>> --- a/Documentation/media/uapi/v4l/common.rst
>> +++ b/Documentation/media/uapi/v4l/common.rst
>> @@ -44,3 +44,4 @@ applicable to all devices.
>>      crop
>>      selection-api
>>      streaming-par
>> +    request-api
>> diff --git a/Documentation/media/uapi/v4l/request-api.rst b/Documentation/media/uapi/v4l/request-api.rst
>> new file mode 100644
>> index 000000000000..a758a5fd3ca0
>> --- /dev/null
>> +++ b/Documentation/media/uapi/v4l/request-api.rst
>> @@ -0,0 +1,194 @@
>> +.. -*- coding: utf-8; mode: rst -*-
>> +
>> +.. _media-request-api:
>> +
>> +Request API
>> +===========
>> +
>> +The Request API has been designed to allow V4L2 to deal with requirements of
>> +modern IPs (stateless codecs, MIPI cameras, ...) and APIs (Android Codec v2).
>> +One such requirement is the ability for devices belonging to the same pipeline
>> +to reconfigure and collaborate closely on a per-frame basis. Another is
>> +efficient support of stateless codecs, which need per-frame controls to be set
>> +asynchronously in order to be efficiently used.
>> +
>> +Supporting these features without the Request API is possible but terribly
>> +inefficient: user-space would have to flush all activity on the media pipeline,
>> +reconfigure it for the next frame, queue the buffers to be processed with that
>> +configuration, and wait until they are all available for dequeing before
>> +considering the next frame. This defeats the purpose of having buffer queues
>> +since in practice only one buffer would be queued at a time.
>> +
>> +The Request API allows a specific configuration of the pipeline (media
>> +controller topology + controls for each device) to be associated with specific
>> +buffers. The parameters are applied by each participating device as buffers
>> +associated to a request flow in. This allows user-space to schedule several
>> +tasks ("requests") with different parameters in advance, knowing that the
>> +parameters will be applied when needed to get the expected result. Controls
>> +values at the time of request completion are also available for reading.
>> +
>> +Usage
>> +=====
>> +
>> +The Request API is used on top of standard media controller and V4L2 calls,
>> +which are augmented with an extra ``request_fd`` parameter. All operations on
>> +requests themselves are performed using the command parameter of the
>> +:c:func:`MEDIA_IOC_REQUEST_CMD` ioctl.
>> +
>> +Allocation
>> +----------
>> +
>> +User-space allocates requests using the ``MEDIA_REQ_CMD_ALLOC`` command on
>> +an opened media device. This returns a file descriptor representing the
>> +request. Typically, several such requests will be allocated.
>> +
>> +Request Preparation
>> +-------------------
>> +
>> +Standard V4L2 ioctls can then receive a request file descriptor to express the
>> +fact that the ioctl is part of said request, and is not to be applied
>> +immediately. V4L2 ioctls supporting this are :c:func:`VIDIOC_S_EXT_CTRLS` and
>> +:c:func:`VIDIOC_QBUF`. Controls set with a request parameter are stored instead
>> +of being immediately applied, and queued buffers will block the buffer queue
>> +until the request becomes active.
>> +
>> +RFC Note: currently several buffers can be queued to the same queue with the
>> +same request. The request parameters will be only be applied when processing
>> +the first buffer. Does it make more sense to allow at most one buffer per
>> +request per queue instead?
>
> I think we should initially limit us to at most one buffer per queue per
> request. If you don't want to make changes just queue a buffer without a
> request fd, or possibly with an empty request (that needs to be defined!).

Empty requests are not possible right now (since the buffer order
defines when they are applied). With this design I haven't seen any
limitation for forbidding more than one buffer per request, but maybe
enforcing this rule would reduce the possibility of undefined behavior
(for example, having buffers belonging to different requests
intertwined in the queue). I will add that limitation.

>
>> +
>> +Request Submission
>> +------------------
>> +
>> +Once the parameters and buffers of the request are specified, it can be
>> +submitted with the ``MEDIA_REQ_CMD_SUBMIT`` command. This will make the buffers
>> +associated to the request available to their driver, which can then apply the
>> +saved controls as buffers are processed. A submitted request cannot be used
>> +with further :c:func:`VIDIOC_S_EXT_CTRLS` and :c:func:`VIDIOC_QBUF` ioctls.
>
> I'd just keep this more general:
>
> "A request cannot be modified anymore once it has been submitted."

More succinct and meaningful indeed.

>
>> +
>> +If several devices are part of the request, individual drivers may synchronize
>> +so the requested pipeline's topology is applied before the buffers are
>> +processed. This is at the discretion of the drivers and is not a requirement.
>> +
>> +Buffers queued without an associated request after a request-bound buffer will
>> +be processed using the state of the hardware at the time of the request
>> +completion. All the same, controls set without a request are applied
>> +immediately, regardless of whether a request is in use or not.
>
> Ah, OK. I agree.
>
>> +
>> +User-space can ``poll()`` a request FD in order to wait until the request
>> +completes. A request is considered complete once all its associated buffers are
>> +available for dequeing. Note that user-space does not need to wait for the
>> +request to complete to dequeue its buffers: buffers that are available halfway
>> +through a request can be dequeued independently of the request's state.
>> +
>> +A completed request includes the state of all devices that had queued buffers
>> +associated with it at the time of the request completion. User-space can query
>> +that state by calling :c:func:`VIDIOC_G_EXT_CTRLS` with the request FD.
>> +
>> +Recycling and Destruction
>> +-------------------------
>> +
>> +Finally, completed request can either be discarded or be reused. Calling
>> +``close()`` on a request FD will make that FD unusable, freeing the request if
>> +it is not referenced elsewhere. The ``MEDIA_REQ_CMD_REINIT`` command will clear
>> +a request's state and make it available again. No state is retained by this
>> +operation: the request is as if it had just been allocated via
>> +``MEDIA_REQ_CMD_ALLOC``.
>> +
>> +RFC Note: Since requests are represented by FDs themselves, the
>> +``MEDIA_REQ_CMD_SUBMIT`` and ``MEDIA_REQ_CMD_REININT`` commands can be performed
>> +on the request FD instead of the media device. This means the media device would
>> +only need to manage ``MEDIA_REQ_CMD_ALLOC``, which could be turned into an
>> +ioctl, while ``MEDIA_REQ_CMD_SUBMIT`` and ``MEDIA_REQ_CMD_REININT`` would
>> +become ioctls on the request itself. This has the advantage of allowing
>> +receivers of a request FD to submit the request, and also decouples requests
>> +from the media device - a scenario that makes sense for stateless codecs where
>> +the media device is not really useful.
>
> I think allowing SUBMIT/REINIT to operate directly on the request fd makes sense.
>
> A few questions that I haven't seen answered here:
>
> - can I reinit a request if it hasn't yet been submitted? Or can this only be
>   done on completed requests?

Interesting question. Right now this is possible, but the behavior may
be not well defined.

It is easy to cancel applying the controls of the request, since they
only exist within it. Otherwise, we cannot say the same about queued
buffers. Should be dequeue them, or let them proceed without any
request associated? I suppose dequeuing would make the most sense
here.

> - can I reinit (or perhaps also alloc) a request based on an existing request?
>   (sort of a clone operation). I can't remember what we decided in the Prague
>   meeting, sorry about that.

Not at the moment. IIRC we decided this was out of scope as the
correct behavior in that case is hard to define (do we clone the whole
state? only the state that has explicitly been set? how about queued
buffers?). We would need to discuss that in more detail if we want
such a feature.

> - a request effectively contains the things you want to change, so anything not
>   explicitly set in the request will have what value? This should be the current
>   hardware value with any changes specified in preceding requests on top.

That's what we agreed on, yes. I need to check whether your patchset
does exactly that, but it should be possible to adapt it in case it
doesn't yet.

> - add an RFC note that requests do not have to complete in the same order that they
>   were queued. A codec driver can (at least in theory) hold on to a buffer
>   containing a key frame until that frame is no longer needed. But this also
>   relates to the fences support, so let's keep this an RFC for now until that
>   patch series has been merged.

Good point, agreed.

> - how do we know that the request API is supported by a driver? I think we
>   wanted a new capability for that?

For video devices, I think it may be more flexible if we can make it
part of the format. It would also allow us to specify more coarsely
the extend to which requests are acceptable (e.g. for codec drivers,
the OUTPUT queue would accept them, but not the CAPTURE queue).

I am not sure whether media devices have capabilities of some sort? Or
maybe we can just rely on MEDIA_REQ_CMD_ALLOC returning -ENOSYS to
detect that?

> - it is allowed to query the values of a submitted request, or will that return
>   -EBUSY while it is not completed? I see no reason not to allow it.

As of know this is working and will return either the hardware value
if a control is not set by the request, or the value set in the
request if it is.

> - what happens when a value inside a request can't be applied? (e.g. due to a
>   hardware error or some other unusual situation).

I need to give this more thought, but I suppose the buffers should be
returned as usual when a failure happens. Maybe we need a way to
signal the error through the request too, and another to cancel
operation on the whole pipeline when an error occurs somewhere.

>
>> +
>> +Example for a Codec Device
>> +--------------------------
>> +
>> +Codecs are single-node V4L2 devices providing one OUTPUT queue (for user-space
>> +to provide input buffers) and one CAPTURE queue (to retrieve processed data).
>> +
>> +In this use-case, request API is used to associate specific controls to be
>> +applied by the driver before processing a buffer from the OUTPUT queue. When
>> +retrieving a buffer from a capture queue, user-space can then identify which
>> +request, if any, produced it by looking at the request_fd field of the dequeued
>> +v4l2_buffer.
>
> I'd start with the code to allocate a request.

Will fix that.

>
>> +Put into code, after obtaining a request, user-space can assign controls and one
>> +OUTPUT buffer to it:
>> +
>> +     struct v4l2_buf buf;
>> +     struct v4l2_ext_controls ctrls;
>> +     ...
>> +     ctrls.request_fd = req_fd;
>> +     ...
>> +     ioctl(codec_fd, VIDIOC_S_EXT_CTRLS, &ctrls);
>> +     ...
>> +     buf.type = V4L2_BUF_TYPE_VIDEO_OUTPUT;
>> +     buf.request_fd = req_fd;
>> +     ioctl(codec_fd, VIDIOC_QBUF, &buf);
>> +
>> +Note that request_fd shall not be specified for CAPTURE buffers.
>
> I know why, but the less experienced reader won't. So this needs some additional
> explanation.

Sure.

>
>> +
>> +Once the request is fully prepared, it can be submitted to the driver:
>> +
>> +     struct media_request_cmd cmd;
>> +
>> +     memset(&cmd, 0, sizeof(cmd));
>> +     cmd.cmd = MEDIA_REQ_CMD_SUBMIT;
>> +     cmd.fd = request_fd;
>> +     ioctl(media_fd, MEDIA_IOC_REQUEST_CMD, &cmd);
>> +
>> +User-space can then lookup the request_fd field of dequeued capture buffers to
>> +confirm which one has been produced by the request.
>> +
>> +     struct v4l2_buf buf;
>> +
>> +     memset(&buf, 0, sizeof(buf));
>> +     buf.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
>> +     ioctl(codec_fd, VIDIOC_DQBUF, &buf);
>> +
>> +     if (buf.request_fd == request_fd)
>> +             ...
>> +
>
> I'm not sure this is correct. You set the request for the OUTPUT buffer, so when
> you dequeue the OUTPUT buffer, then the request_fd is set to the original request.
> But this is the CAPTURE buffer, and that has no requests at all.

This is not a mistake actually. The idea is to allow user-space to
know which request produced a given capture buffer. One output buffer
can in theory produce several or no capture buffers, and I think this
is the only reliable way to associate them with their producing
request.

>
> BTW, you also want to show an example of getting the control once the request is
> completed (e.g. status information), probably with a polling example as well.

True.

>
>> +Example for a Simple Capture Device
>> +-----------------------------------
>> +
>> +With a simple capture device, requests can be used to specify controls to apply
>> +to a given CAPTURE buffer. The driver will apply these controls before producing
>> +the marked CAPTURE buffer.
>> +
>> +     struct v4l2_buf buf;
>> +     struct v4l2_ext_controls ctrls;
>> +     ...
>> +     ctrls.request_fd = req_fd;
>> +     ...
>> +     ioctl(camera_fd, VIDIOC_S_EXT_CTRLS, &ctrls);
>> +     ...
>> +     buf.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
>> +     buf.request_fd = req_fd;
>> +     ioctl(camera_fd, VIDIOC_QBUF, &buf);
>> +
>> +Once the request is fully prepared, it can be submitted to the driver:
>> +
>> +     struct media_request_cmd cmd;
>> +
>> +     memset(&cmd, 0, sizeof(cmd));
>> +     cmd.cmd = MEDIA_REQ_CMD_SUBMIT;
>> +     cmd.fd = request_fd;
>> +     ioctl(media_fd, MEDIA_IOC_REQUEST_CMD, &cmd);
>> +
>> +User-space can then lookup the request_fd field of dequeued capture buffers to
>> +confirm which one has been produced by the request.
>> +
>> +     struct v4l2_buf buf;
>> +
>> +     memset(&buf, 0, sizeof(buf));
>> +     buf.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
>> +     ioctl(camera_fd, VIDIOC_DQBUF, &buf);
>> +
>> +     if (buf.request_fd == request_fd)
>> +             ...
>> diff --git a/Documentation/media/uapi/v4l/vidioc-qbuf.rst b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
>> index 9e448a4aa3aa..0cd596bd4cde 100644
>> --- a/Documentation/media/uapi/v4l/vidioc-qbuf.rst
>> +++ b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
>> @@ -98,6 +98,12 @@ dequeued, until the :ref:`VIDIOC_STREAMOFF <VIDIOC_STREAMON>` or
>>  :ref:`VIDIOC_REQBUFS` ioctl is called, or until the
>>  device is closed.
>>
>> +For all buffer types, the ``request_fd`` field can be used when enqueing
>> +to specify the file descriptor of a request, if requests are in use.
>> +Setting it means that the buffer will not be passed to the driver until
>> +the request itself is submitted. Also, the driver will apply any setting
>> +associated with the request before processing the buffer.
>> +
>>  Applications call the ``VIDIOC_DQBUF`` ioctl to dequeue a filled
>>  (capturing) or displayed (output) buffer from the driver's outgoing
>>  queue. They just set the ``type``, ``memory`` and ``reserved`` fields of
>> @@ -115,6 +121,21 @@ queue. When the ``O_NONBLOCK`` flag was given to the
>>  :ref:`open() <func-open>` function, ``VIDIOC_DQBUF`` returns
>>  immediately with an ``EAGAIN`` error code when no buffer is available.
>>
>> +If a dequeued buffer was produced as the result of a request, then the
>> +``request_fd`` field of :c:type:`v4l2_buffer` will be set to the file
>> +descriptor of the request, regardless of whether the buffer was initially
>> +queued with a request associated to it or not.
>> +
>> +RFC note: a request can be referenced by several FDs, especially if the
>> +request is shared between different processes. Also a FD can be closed
>> +after a request is submitted. In that case the FD does not make much sense.
>> +Maybe it would be better to define request fd as
>> +
>> +    union { u32 request_fd; u32 request_id; }
>> +
>> +and use request_id when communicating a request to userspace so they can be
>> +unambiguously referenced?
>
> I don't think this is a problem. At least we never had this issue with e.g.
> dmabuf either. The request_fd must be a valid request fd for the process that
> calls the ioctl. If not the ioctl will return an error. If process A has a
> request fd 10 and process B calls the ioctl with request_fd set to 10 but it
> didn't actually allocate the request or receive the fd from process A (there
> is a mechanism for that, although I've forgotten the details).

For user-to-kernel communication this is true. What I had in mind was
the other way around: kernel trying to specify an already-allocated
request to userspace, like the above example of the capture buffer
that is marked with the request that produced it.

At that time, the FD of the request may have been closed, or may have
been passed to another process (which references the request using a
different ID). That's what FDs are not reliable here.

Re: [RFC PATCH 5/8] media: Document the media request API

[Alexandre Courbot]

On Tue, Jan 30, 2018 at 1:04 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> On 01/26/2018 07:02 AM, Alexandre Courbot wrote:
>> From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>>
>> The media request API is made of a new ioctl to implement request
>> management. Document it.
>>
>> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>> [acourbot@chromium.org: adapt for newest API]
>> Signed-off-by: Alexandre Courbot <acourbot@chromium.org>
>> ---
>>  Documentation/media/uapi/mediactl/media-funcs.rst  |   1 +
>>  .../media/uapi/mediactl/media-ioc-request-cmd.rst  | 140 +++++++++++++++++++++
>>  2 files changed, 141 insertions(+)
>>  create mode 100644 Documentation/media/uapi/mediactl/media-ioc-request-cmd.rst
>>
>> diff --git a/Documentation/media/uapi/mediactl/media-funcs.rst b/Documentation/media/uapi/mediactl/media-funcs.rst
>> index 076856501cdb..e3a45d82ffcb 100644
>> --- a/Documentation/media/uapi/mediactl/media-funcs.rst
>> +++ b/Documentation/media/uapi/mediactl/media-funcs.rst
>> @@ -15,4 +15,5 @@ Function Reference
>>      media-ioc-g-topology
>>      media-ioc-enum-entities
>>      media-ioc-enum-links
>> +    media-ioc-request-cmd
>>      media-ioc-setup-link
>> diff --git a/Documentation/media/uapi/mediactl/media-ioc-request-cmd.rst b/Documentation/media/uapi/mediactl/media-ioc-request-cmd.rst
>> new file mode 100644
>> index 000000000000..17223e8170e9
>> --- /dev/null
>> +++ b/Documentation/media/uapi/mediactl/media-ioc-request-cmd.rst
>> @@ -0,0 +1,140 @@
>> +.. -*- coding: utf-8; mode: rst -*-
>> +
>> +.. _media_ioc_request_cmd:
>> +
>> +***************************
>> +ioctl MEDIA_IOC_REQUEST_CMD
>> +***************************
>> +
>> +Name
>> +====
>> +
>> +MEDIA_IOC_REQUEST_CMD - Manage media device requests
>> +
>> +
>> +Synopsis
>> +========
>> +
>> +.. c:function:: int ioctl( int fd, MEDIA_IOC_REQUEST_CMD, struct media_request_cmd *argp )
>> +    :name: MEDIA_IOC_REQUEST_CMD
>> +
>> +
>> +Arguments
>> +=========
>> +
>> +``fd``
>> +    File descriptor returned by :ref:`open() <media-func-open>`.
>> +
>> +``argp``
>> +
>> +
>> +Description
>> +===========
>> +
>> +The MEDIA_IOC_REQUEST_CMD ioctl allow applications to manage media device
>> +requests. A request is an object that can group media device configuration
>> +parameters, including subsystem-specific parameters, in order to apply all the
>> +parameters atomically. Applications are responsible for allocating and
>> +deleting requests, filling them with configuration parameters submitting them.
>> +
>> +Request operations are performed by calling the MEDIA_IOC_REQUEST_CMD ioctl
>> +with a pointer to a struct :c:type:`media_request_cmd` with the cmd field set
>> +to the appropriate command. :ref:`media-request-command` lists the commands
>> +supported by the ioctl.
>> +
>> +The struct :c:type:`media_request_cmd` request field contains the file
>> +descriptorof the request on which the command operates. For the
>> +``MEDIA_REQ_CMD_ALLOC`` command the field is set to zero by applications and
>> +filled by the driver. For all other commands the field is set by applications
>> +and left untouched by the driver.
>> +
>> +To allocate a new request applications use the ``MEDIA_REQ_CMD_ALLOC``
>> +command. The driver will allocate a new request and return its ID in the
>
> ID -> FD

Indeed, this is a leftover from the original documentation.

>
>> +request field.
>> +
>> +Requests are reference-counted. A newly allocated request is referenced
>> +by the returned file descriptor, and can be later referenced by
>> +subsystem-specific operations. Requests will thus be automatically deleted
>> +when they're not longer used after the returned file descriptor is closed.
>> +
>> +If a request isn't needed applications can delete it by calling ``close()``
>> +on it. The driver will drop the file handle reference. The request will not
>> +be usable through the MEDIA_IOC_REQUEST_CMD ioctl anymore, but will only be
>> +deleted when the last reference is released. If no other reference exists when
>> +``close()`` is invoked the request will be deleted immediately.
>> +
>> +After creating a request applications should fill it with configuration
>> +parameters. This is performed through subsystem-specific request APIs outside
>> +the scope of the media controller API. See the appropriate subsystem APIs for
>> +more information, including how they interact with the MEDIA_IOC_REQUEST_CMD
>> +ioctl.
>> +
>> +Once a request contains all the desired configuration parameters it can be
>> +submitted using the ``MEDIA_REQ_CMD_SUBMIT`` command. This will let the
>> +buffers queued for the request be passed to their respective drivers, which
>> +will then apply the request's parameters before processing them.
>> +
>> +Once a request has been queued applications are not allowed to modify its
>> +configuration parameters until the request has been fully processed. Any
>> +attempt to do so will result in the related subsystem API returning an error.
>> +The application that submitted the request can wait for its completion by
>> +polling on the request's file descriptor.
>> +
>> +Once a request has completed, it can be reused. The ``MEDIA_REQ_CMD_REINIT``
>> +command will bring it back to its initial state, so it can be prepared and
>> +submitted again.
>> +
>> +.. c:type:: media_request_cmd
>> +
>> +.. flat-table:: struct media_request_cmd
>> +    :header-rows:  0
>> +    :stub-columns: 0
>> +    :widths: 1 2 8
>> +
>> +    * - __u32
>> +      - ``cmd``
>> +      - Command, set by the application. See below for the list of supported
>> +        commands.
>> +    * - __u32
>> +      - ``fd``
>> +      - Request FD, set by the driver for the MEDIA_REQ_CMD_ALLOC command and
>> +        by the application for all other commands.
>> +
>> +
>> +.. _media-request-command:
>> +
>> +.. cssclass:: longtable
>> +
>> +.. flat-table:: Media request commands
>> +    :header-rows:  0
>> +    :stub-columns: 0
>> +
>> +    * .. _MEDIA-REQ-CMD-ALLOC:
>> +
>> +      - ``MEDIA_REQ_CMD_ALLOC``
>> +      - Allocate a new request.
>> +    * .. _MEDIA-REQ-CMD-SUBMIT:
>> +
>> +      - ``MEDIA_REQ_CMD_SUBMIT``
>> +      - Submit a request to be processed.
>> +    * .. _MEDIA-REQ-CMD-QUEUE:
>> +
>> +      - ``MEDIA_REQ_CMD_REINIT``
>> +      - Reinitializes a completed request.
>> +
>> +
>> +Return Value
>> +============
>> +
>> +On success 0 is returned, on error -1 and the ``errno`` variable is set
>> +appropriately. The generic error codes are described at the
>> +:ref:`Generic Error Codes <gen-errors>` chapter.
>> +
>> +EINVAL
>> +    The struct :c:type:`media_request_cmd` specifies an invalid command or
>> +    references a non-existing request.
>> +
>> +ENOSYS
>> +    The struct :c:type:`media_request_cmd` specifies the
>> +    ``MEDIA_REQ_CMD_QUEUE`` or ``MEDIA_REQ_CMD_APPLY`` command and that
>
> MEDIA_REQ_CMD_APPLY isn't defined in the table above.

Same. :)

>
>> +    command isn't implemented by the device.
>>
>
>
> So what is missing from this documentation is whether a newly created request
> has a copy of the current state or if it is empty. And didn't we allow a
> request to be based on an other existing request? Unfortunately I have no
> time to dig up that information at the moment.

Will specify the behavior better.

>
> More comments in my review for 6/8. Probably best to read that review
> first before this one.
>
> Regards,
>
>         Hans

Re: [RFC PATCH 0/8] [media] Request API, take three

[Hans Verkuil]

On 01/30/2018 07:31 AM, Alexandre Courbot wrote:
> Hi Hans,
> 
> On Mon, Jan 29, 2018 at 8:21 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> On 01/26/2018 07:02 AM, Alexandre Courbot wrote:
>>> Howdy. Here is your bi-weekly request API redesign! ;)
>>>
>>> Again, this is a simple version that only implements the flow of requests,
>>> without applying controls. The intent is to get an agreement on a base to work
>>> on, since the previous versions went straight back to the redesign board.
>>>
>>> Highlights of this version:
>>>
>>> * As requested by Hans, request-bound buffers are now passed earlier to drivers,
>>> as early as the request itself is submitted. Doing it earlier is not be useful
>>> since the driver would not know the state of the request, and thus cannot do
>>> anything with the buffer. Drivers are now responsible for applying request
>>> parameters themselves.
>>>
>>> * As a consequence, there is no such thing as a "request queue" anymore. The
>>> flow of buffers decides the order in which requests are processed. Individual
>>> devices of the same pipeline can implement synchronization if needed, but this
>>> is beyond this first version.
>>>
>>> * VB2 code is still a bit shady. Some of it will interfere with the fences
>>> series, so I am waiting for the latter to land to do it properly.
>>>
>>> * Requests that are not yet submitted effectively act as fences on the buffers
>>> they own, resulting in the buffer queue being blocked until the request is
>>> submitted. An alternate design would be to only block the
>>> not-submitted-request's buffer and let further buffers pass before it, but since
>>> buffer order is becoming increasingly important I have decided to just block the
>>> queue. This is open to discussion though.
>>
>> I don't think we should mess with the order.
> 
> Agreed, let's keep it that way then.
> 
>>
>>>
>>> * Documentation! Also described usecases for codec and simple (i.e. not part of
>>> a complex pipeline) capture device.
>>
>> I'll concentrate on reviewing that.
>>
>>>
>>> Still remaining to do:
>>>
>>> * As pointed out by Hans on the previous revision, do not assume that drivers
>>> using v4l2_fh have a per-handle state. I have not yet found a good way to
>>> differenciate both usages.
>>
>> I suspect we need to add a flag or something for this.
> 
> I hope we don't need to, let's see if I can find a pattern...
> 
>>
>>> * Integrate Hans' patchset for control handling: as said above, this is futile
>>> unless we can agree on the basic design, which I hope we can do this time.
>>> Chrome OS needs this really badly now and will have to move forward no matter
>>> what, so I hope this will be considered good enough for a common base of work.
>>
>> I am not sure there is any reason to not move forward with the control handling.
>> You need this anyway IMHO, regardless of any public API considerations.
> 
> Only reasons are my lazyness and because I wanted to focus on the
> request flow first. But you're right. I have a version with your
> control framework changes integrated (they worked on the first attempt
> btw! :)), let me create a clean patchset from this and send another
> RFC.

Scary that it worked on the first attempt :-)

> 
>>
>>> A few thoughts/questions that emerged when writing this patchset:
>>>
>>> * Since requests are exposed as file descriptors, we could easily move the
>>> MEDIA_REQ_CMD_SUBMIT and MEDIA_REQ_CMD_REININT commands as ioctls on the
>>> requests themselves, instead of having to perform them on the media device that
>>> provided the request in the first place. That would add a bit more flexibility
>>> if/when passing requests around and means the media device only needs to handle
>>> MEDIA_REQ_CMD_ALLOC. Conceptually speaking, this seems to make sense to me.
>>> Any comment for/against that?
>>
>> Makes sense IMHO.
> 
> Glad to hear it, that was my preferred design. :)
> 
>>
>>> * For the codec usecase, I have experimented a bit marking CAPTURE buffers with
>>> the request FD that produced them (vim2m acts that way). This seems to work
>>> well, however FDs are process-local and could be closed before the CAPTURE
>>> buffer is dequeued, rendering that information less meaningful, if not
>>> dangerous.
>>
>> I don't follow this. Once the fd is passed to the kernel its refcount should be
>> increased so the data it represents won't be released if userspace closes the fd.
> 
> The refcount of the request is increased. The refcount of the FD is
> not, since it is only a userspace reference to the request.

I don't think that's right. Looking at how dma-buf does this (dma_buf_get in
dma-buf.c) it calls fget(fd) which increases the fd refcount. In fact, as far as
I can see the struct dma_buf doesn't have a refcount, it is solely refcounted
through the fd. That's probably the model you want to follow.

> 
>>
>> Obviously if userspace closes the fd it cannot do anything with it anymore, but
>> it shouldn't be 'dangerous' AFAICT.
> 
> It userspace later gets that closed FD back from a DQBUF call, and
> decides to use it again, then we would have a problem. I agree that it
> is userspace responsibility to be careful here, but making things
> foolproof never hurts.

I think all the issues will go away if you refcount the fd instead of the
request. It worked well for dma-buf for years.

> 
>>
>>> Wouldn't it be better/safer to use a global request ID for
>>> such information instead? That ID would be returned upon MEDIA_REQ_CMD_ALLOC so
>>> user-space knows which request ID a FD refers to.
>>
>> I think it is not a good idea to have both an fd and an ID to refer to the
>> same object. That's going to be very confusing I think.
> 
> IDs would not refer to the object, they would just be a way to
> identify it. FDs would be the actual reference.
> 
> If we drop the idea of returning request FDs to userspace after the
> initial allocation (which is the only time we can be sure that a
> returned FD is valid), then this is not a problem anymore, but I think
> it may be useful to mark CAPTURE buffers with the request that
> generated them.
> 
>>
>>> * Using the media node to provide requests makes absolute sense for complex
>>> camera devices, but is tedious for codec devices which work on one node and
>>> require to protect request/media related code with #ifdef
>>> CONFIG_MEDIA_CONTROLLER.
>>
>> Why? They would now depend on MEDIA_CONTROLLER (i.e. they won't appear in the
>> menuconfig unless MEDIA_CONTROLLER is set). No need for an #ifdef.
> 
> Ah, if we make them depend on MEDIA_CONTROLLER, then indeed. But do we
> want to do this for e.g. vim2m and vivid?

If they support requests, then yes.

> 
>>
>>  For these devices, the sole role of the media node is
>>> to produce the request, and that's it (since request submission could be moved
>>> to the request FD as explained above). That's a modest use that hardly justifies
>>> bringing in the whole media framework IMHO. With a bit of extra abstraction, it
>>> should be possible to decouple the base requests from the media controller
>>> altogether, and propose two kinds of requests implementations: one simpler
>>> implementation that works directly with a single V4L2 node (useful for codecs),
>>> and another one that works on a media node and can control all its entities
>>> (good for camera). This would allow codecs to use the request API without
>>> forcing the media controller, and would considerably simplify the use-case. Any
>>> objection to that? IIRC the earlier design documents mentioned this possibility.
>>
>> I think this is an interesting idea, but I would postpone making a decision on
>> this until later. We need this MC support regardless, so let's start off with that.
>>
>> Once that's working we can discuss if we should or should not create a shortcut
>> for codecs. Trying to decide this now would only confuse the process.
> 
> Sounds good, as long as we make a decision before merging.

Of course.

Regards,

	Hans

Re: [RFC PATCH 0/8] [media] Request API, take three

[Tomasz Figa]

Hi Hans,

Sorry for joining the party late.

On Wed, Jan 31, 2018 at 4:50 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> On 01/30/2018 07:31 AM, Alexandre Courbot wrote:
>> Hi Hans,
>>
>> On Mon, Jan 29, 2018 at 8:21 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>> On 01/26/2018 07:02 AM, Alexandre Courbot wrote:
>>>> Howdy. Here is your bi-weekly request API redesign! ;)
>>>>
>>>> Again, this is a simple version that only implements the flow of requests,
>>>> without applying controls. The intent is to get an agreement on a base to work
>>>> on, since the previous versions went straight back to the redesign board.
>>>>
>>>> Highlights of this version:
>>>>
>>>> * As requested by Hans, request-bound buffers are now passed earlier to drivers,
>>>> as early as the request itself is submitted. Doing it earlier is not be useful
>>>> since the driver would not know the state of the request, and thus cannot do
>>>> anything with the buffer. Drivers are now responsible for applying request
>>>> parameters themselves.
>>>>
>>>> * As a consequence, there is no such thing as a "request queue" anymore. The
>>>> flow of buffers decides the order in which requests are processed. Individual
>>>> devices of the same pipeline can implement synchronization if needed, but this
>>>> is beyond this first version.
>>>>
>>>> * VB2 code is still a bit shady. Some of it will interfere with the fences
>>>> series, so I am waiting for the latter to land to do it properly.
>>>>
>>>> * Requests that are not yet submitted effectively act as fences on the buffers
>>>> they own, resulting in the buffer queue being blocked until the request is
>>>> submitted. An alternate design would be to only block the
>>>> not-submitted-request's buffer and let further buffers pass before it, but since
>>>> buffer order is becoming increasingly important I have decided to just block the
>>>> queue. This is open to discussion though.
>>>
>>> I don't think we should mess with the order.
>>
>> Agreed, let's keep it that way then.
>>

I'm not sure I'm following here. Does it mean (quoting Alex):

a) "Requests that are not yet submitted effectively act as fences on the buffers
    they own, resulting in the buffer queue being blocked until the request is
    submitted."

b) "block the not-submitted-request's buffer and let further buffers
pass before it"

or neither?

Hans, could you clarify what you think is the right behavior here?

>>>> * For the codec usecase, I have experimented a bit marking CAPTURE buffers with
>>>> the request FD that produced them (vim2m acts that way). This seems to work
>>>> well, however FDs are process-local and could be closed before the CAPTURE
>>>> buffer is dequeued, rendering that information less meaningful, if not
>>>> dangerous.
>>>
>>> I don't follow this. Once the fd is passed to the kernel its refcount should be
>>> increased so the data it represents won't be released if userspace closes the fd.
>>
>> The refcount of the request is increased. The refcount of the FD is
>> not, since it is only a userspace reference to the request.
>
> I don't think that's right. Looking at how dma-buf does this (dma_buf_get in
> dma-buf.c) it calls fget(fd) which increases the fd refcount. In fact, as far as
> I can see the struct dma_buf doesn't have a refcount, it is solely refcounted
> through the fd. That's probably the model you want to follow.

As far as I can see from the code, fget() on an fd increases a
reference count on the struct file backing the fd. I don't think there
is another level of reference counting of fds themselves - that's why
dup(fd) gives you another fd and you can't call close(fd) on the same
fd two times.

>
>>
>>>
>>> Obviously if userspace closes the fd it cannot do anything with it anymore, but
>>> it shouldn't be 'dangerous' AFAICT.
>>
>> It userspace later gets that closed FD back from a DQBUF call, and
>> decides to use it again, then we would have a problem. I agree that it
>> is userspace responsibility to be careful here, but making things
>> foolproof never hurts.
>
> I think all the issues will go away if you refcount the fd instead of the
> request. It worked well for dma-buf for years.

I'm confused here. The kernel never returns the same FD for the same
DMA-buf twice. Every time an ioctl is called, which returns a DMA-buf
FD, it creates a new FD.

Best regards,
Tomasz

Re: [RFC PATCH 6/8] v4l2: document the request API interface

[Hans Verkuil]

On 01/30/2018 07:31 AM, Alexandre Courbot wrote:
> On Tue, Jan 30, 2018 at 1:03 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> On 01/26/2018 07:02 AM, Alexandre Courbot wrote:
>>> Document how the request API can be used along with the existing V4L2
>>> interface.
>>>
>>> Signed-off-by: Alexandre Courbot <acourbot@chromium.org>
>>> ---
>>>  Documentation/media/uapi/v4l/buffer.rst      |  10 +-
>>>  Documentation/media/uapi/v4l/common.rst      |   1 +
>>>  Documentation/media/uapi/v4l/request-api.rst | 194 +++++++++++++++++++++++++++
>>>  Documentation/media/uapi/v4l/vidioc-qbuf.rst |  21 +++
>>>  4 files changed, 223 insertions(+), 3 deletions(-)
>>>  create mode 100644 Documentation/media/uapi/v4l/request-api.rst
>>>
>>> diff --git a/Documentation/media/uapi/v4l/buffer.rst b/Documentation/media/uapi/v4l/buffer.rst
>>> index ae6ee73f151c..9d082784081d 100644
>>> --- a/Documentation/media/uapi/v4l/buffer.rst
>>> +++ b/Documentation/media/uapi/v4l/buffer.rst
>>> @@ -301,10 +301,14 @@ struct v4l2_buffer
>>>       elements in the ``planes`` array. The driver will fill in the
>>>       actual number of valid elements in that array.
>>>      * - __u32
>>> -      - ``reserved2``
>>> +      - ``request_fd``
>>>        -
>>> -      - A place holder for future extensions. Drivers and applications
>>> -     must set this to 0.
>>> +      - The file descriptor of the request associated with this buffer.
>>> +     user-space can set this when calling :ref:`VIDIOC_QBUF`, and drivers
>>> +     will return the request used when processing a buffer (if any) upon
>>> +     :ref:`VIDIOC_DQBUF`.
>>> +
>>> +     A value of 0 means the buffer is not associated with any request.
>>>      * - __u32
>>>        - ``reserved``
>>>        -
>>> diff --git a/Documentation/media/uapi/v4l/common.rst b/Documentation/media/uapi/v4l/common.rst
>>> index 13f2ed3fc5a6..a4aa0059d45a 100644
>>> --- a/Documentation/media/uapi/v4l/common.rst
>>> +++ b/Documentation/media/uapi/v4l/common.rst
>>> @@ -44,3 +44,4 @@ applicable to all devices.
>>>      crop
>>>      selection-api
>>>      streaming-par
>>> +    request-api
>>> diff --git a/Documentation/media/uapi/v4l/request-api.rst b/Documentation/media/uapi/v4l/request-api.rst
>>> new file mode 100644
>>> index 000000000000..a758a5fd3ca0
>>> --- /dev/null
>>> +++ b/Documentation/media/uapi/v4l/request-api.rst
>>> @@ -0,0 +1,194 @@
>>> +.. -*- coding: utf-8; mode: rst -*-
>>> +
>>> +.. _media-request-api:
>>> +
>>> +Request API
>>> +===========
>>> +
>>> +The Request API has been designed to allow V4L2 to deal with requirements of
>>> +modern IPs (stateless codecs, MIPI cameras, ...) and APIs (Android Codec v2).
>>> +One such requirement is the ability for devices belonging to the same pipeline
>>> +to reconfigure and collaborate closely on a per-frame basis. Another is
>>> +efficient support of stateless codecs, which need per-frame controls to be set
>>> +asynchronously in order to be efficiently used.
>>> +
>>> +Supporting these features without the Request API is possible but terribly
>>> +inefficient: user-space would have to flush all activity on the media pipeline,
>>> +reconfigure it for the next frame, queue the buffers to be processed with that
>>> +configuration, and wait until they are all available for dequeing before
>>> +considering the next frame. This defeats the purpose of having buffer queues
>>> +since in practice only one buffer would be queued at a time.
>>> +
>>> +The Request API allows a specific configuration of the pipeline (media
>>> +controller topology + controls for each device) to be associated with specific
>>> +buffers. The parameters are applied by each participating device as buffers
>>> +associated to a request flow in. This allows user-space to schedule several
>>> +tasks ("requests") with different parameters in advance, knowing that the
>>> +parameters will be applied when needed to get the expected result. Controls
>>> +values at the time of request completion are also available for reading.
>>> +
>>> +Usage
>>> +=====
>>> +
>>> +The Request API is used on top of standard media controller and V4L2 calls,
>>> +which are augmented with an extra ``request_fd`` parameter. All operations on
>>> +requests themselves are performed using the command parameter of the
>>> +:c:func:`MEDIA_IOC_REQUEST_CMD` ioctl.
>>> +
>>> +Allocation
>>> +----------
>>> +
>>> +User-space allocates requests using the ``MEDIA_REQ_CMD_ALLOC`` command on
>>> +an opened media device. This returns a file descriptor representing the
>>> +request. Typically, several such requests will be allocated.
>>> +
>>> +Request Preparation
>>> +-------------------
>>> +
>>> +Standard V4L2 ioctls can then receive a request file descriptor to express the
>>> +fact that the ioctl is part of said request, and is not to be applied
>>> +immediately. V4L2 ioctls supporting this are :c:func:`VIDIOC_S_EXT_CTRLS` and
>>> +:c:func:`VIDIOC_QBUF`. Controls set with a request parameter are stored instead
>>> +of being immediately applied, and queued buffers will block the buffer queue
>>> +until the request becomes active.
>>> +
>>> +RFC Note: currently several buffers can be queued to the same queue with the
>>> +same request. The request parameters will be only be applied when processing
>>> +the first buffer. Does it make more sense to allow at most one buffer per
>>> +request per queue instead?
>>
>> I think we should initially limit us to at most one buffer per queue per
>> request. If you don't want to make changes just queue a buffer without a
>> request fd, or possibly with an empty request (that needs to be defined!).
> 
> Empty requests are not possible right now (since the buffer order
> defines when they are applied).

I don't understand what the buffer order has to do with empty request. Perhaps
there is some terminology confusion here.

What I meant was: I allocate a request, set a control in that request and queue
up a buffer and submit the request.

Next I want to queue up a buffer but I don't need to make any changes to the state.
So do I:

1) queue up a buffer without setting the request_fd, or

2) make a new request, do not touch the request other then setting request_fd in
   the buffer and queuing it and submitting the request.

Or can I do either? The only difference between the two would be that option 2
gives userspace the request completion event and it can read the state from the
request. I think both should be possible.

It should be defined how this will work.

 With this design I haven't seen any
> limitation for forbidding more than one buffer per request, but maybe
> enforcing this rule would reduce the possibility of undefined behavior
> (for example, having buffers belonging to different requests
> intertwined in the queue). I will add that limitation.

It doesn't really make sense to me to allow multiple buffers per request.
I feel it just complicates the API. I.e. in the list above you would get
a third option of having one request but with multiple buffers queued.
The big problem with that IMHO is that the request completion comes in
very, very late (only after the last buffer has been completed).

Complexity is always enemy #1 w.r.t. video.

> 
>>
>>> +
>>> +Request Submission
>>> +------------------
>>> +
>>> +Once the parameters and buffers of the request are specified, it can be
>>> +submitted with the ``MEDIA_REQ_CMD_SUBMIT`` command. This will make the buffers
>>> +associated to the request available to their driver, which can then apply the
>>> +saved controls as buffers are processed. A submitted request cannot be used
>>> +with further :c:func:`VIDIOC_S_EXT_CTRLS` and :c:func:`VIDIOC_QBUF` ioctls.
>>
>> I'd just keep this more general:
>>
>> "A request cannot be modified anymore once it has been submitted."
> 
> More succinct and meaningful indeed.
> 
>>
>>> +
>>> +If several devices are part of the request, individual drivers may synchronize
>>> +so the requested pipeline's topology is applied before the buffers are
>>> +processed. This is at the discretion of the drivers and is not a requirement.
>>> +
>>> +Buffers queued without an associated request after a request-bound buffer will
>>> +be processed using the state of the hardware at the time of the request
>>> +completion. All the same, controls set without a request are applied
>>> +immediately, regardless of whether a request is in use or not.
>>
>> Ah, OK. I agree.
>>
>>> +
>>> +User-space can ``poll()`` a request FD in order to wait until the request
>>> +completes. A request is considered complete once all its associated buffers are
>>> +available for dequeing. Note that user-space does not need to wait for the
>>> +request to complete to dequeue its buffers: buffers that are available halfway
>>> +through a request can be dequeued independently of the request's state.
>>> +
>>> +A completed request includes the state of all devices that had queued buffers
>>> +associated with it at the time of the request completion. User-space can query
>>> +that state by calling :c:func:`VIDIOC_G_EXT_CTRLS` with the request FD.
>>> +
>>> +Recycling and Destruction
>>> +-------------------------
>>> +
>>> +Finally, completed request can either be discarded or be reused. Calling
>>> +``close()`` on a request FD will make that FD unusable, freeing the request if
>>> +it is not referenced elsewhere. The ``MEDIA_REQ_CMD_REINIT`` command will clear
>>> +a request's state and make it available again. No state is retained by this
>>> +operation: the request is as if it had just been allocated via
>>> +``MEDIA_REQ_CMD_ALLOC``.
>>> +
>>> +RFC Note: Since requests are represented by FDs themselves, the
>>> +``MEDIA_REQ_CMD_SUBMIT`` and ``MEDIA_REQ_CMD_REININT`` commands can be performed
>>> +on the request FD instead of the media device. This means the media device would
>>> +only need to manage ``MEDIA_REQ_CMD_ALLOC``, which could be turned into an
>>> +ioctl, while ``MEDIA_REQ_CMD_SUBMIT`` and ``MEDIA_REQ_CMD_REININT`` would
>>> +become ioctls on the request itself. This has the advantage of allowing
>>> +receivers of a request FD to submit the request, and also decouples requests
>>> +from the media device - a scenario that makes sense for stateless codecs where
>>> +the media device is not really useful.
>>
>> I think allowing SUBMIT/REINIT to operate directly on the request fd makes sense.
>>
>> A few questions that I haven't seen answered here:
>>
>> - can I reinit a request if it hasn't yet been submitted? Or can this only be
>>   done on completed requests?
> 
> Interesting question. Right now this is possible, but the behavior may
> be not well defined.
> 
> It is easy to cancel applying the controls of the request, since they
> only exist within it. Otherwise, we cannot say the same about queued
> buffers. Should be dequeue them, or let them proceed without any
> request associated? I suppose dequeuing would make the most sense
> here.

Related to this: what happens if I close the request fd having already set
values and queued buffers, but not yet submitted the request.

It's a similar situation to calling REINIT, but this scenario is very real
since an application can always do this, you can't prevent it.

So since you need to implement and define this close(request_fd) scenario
regardless, I think REINIT can just use this.

> 
>> - can I reinit (or perhaps also alloc) a request based on an existing request?
>>   (sort of a clone operation). I can't remember what we decided in the Prague
>>   meeting, sorry about that.
> 
> Not at the moment. IIRC we decided this was out of scope as the
> correct behavior in that case is hard to define (do we clone the whole
> state? only the state that has explicitly been set? how about queued
> buffers?). We would need to discuss that in more detail if we want
> such a feature.
> 
>> - a request effectively contains the things you want to change, so anything not
>>   explicitly set in the request will have what value? This should be the current
>>   hardware value with any changes specified in preceding requests on top.
> 
> That's what we agreed on, yes. I need to check whether your patchset
> does exactly that, but it should be possible to adapt it in case it
> doesn't yet.

I don't believe this first version does that. I'm waiting for working code
that I can use to implement this. I need something to test with :-)

I remember that I had a good idea on how to implement this. I hope I can still
remember what it was exactly when I start working on this :-)

> 
>> - add an RFC note that requests do not have to complete in the same order that they
>>   were queued. A codec driver can (at least in theory) hold on to a buffer
>>   containing a key frame until that frame is no longer needed. But this also
>>   relates to the fences support, so let's keep this an RFC for now until that
>>   patch series has been merged.
> 
> Good point, agreed.
> 
>> - how do we know that the request API is supported by a driver? I think we
>>   wanted a new capability for that?
> 
> For video devices, I think it may be more flexible if we can make it
> part of the format. It would also allow us to specify more coarsely
> the extend to which requests are acceptable (e.g. for codec drivers,
> the OUTPUT queue would accept them, but not the CAPTURE queue).

I'd say, make a proposal and we can discuss this further.

In general I always felt that we are missing capabilities that tell userspace
what a DMA queue can do (e.g. which of the MMAP, USERPTR and DMA_BUF streaming
modes are supported, is the state per-filehandle or global, can it support the
request API). It would be nice to have a solution that takes this higher level
view into account.

> I am not sure whether media devices have capabilities of some sort? Or
> maybe we can just rely on MEDIA_REQ_CMD_ALLOC returning -ENOSYS to
> detect that?

It's not a MC capability IMHO. Other than that if the MC cannot allocate
requests the MEDIA_REQ_CMD_ALLOC ioctl would return -ENOTTY.

> 
>> - it is allowed to query the values of a submitted request, or will that return
>>   -EBUSY while it is not completed? I see no reason not to allow it.
> 
> As of know this is working and will return either the hardware value
> if a control is not set by the request, or the value set in the
> request if it is.

To be precise: it should return the value from the last queued request that
sets it, or from the hardware if there is no such request.

> 
>> - what happens when a value inside a request can't be applied? (e.g. due to a
>>   hardware error or some other unusual situation).
> 
> I need to give this more thought, but I suppose the buffers should be
> returned as usual when a failure happens. Maybe we need a way to
> signal the error through the request too, and another to cancel
> operation on the whole pipeline when an error occurs somewhere.

I think we definitely should have a way to signal the error through the
request.

> 
>>
>>> +
>>> +Example for a Codec Device
>>> +--------------------------
>>> +
>>> +Codecs are single-node V4L2 devices providing one OUTPUT queue (for user-space
>>> +to provide input buffers) and one CAPTURE queue (to retrieve processed data).
>>> +
>>> +In this use-case, request API is used to associate specific controls to be
>>> +applied by the driver before processing a buffer from the OUTPUT queue. When
>>> +retrieving a buffer from a capture queue, user-space can then identify which
>>> +request, if any, produced it by looking at the request_fd field of the dequeued
>>> +v4l2_buffer.
>>
>> I'd start with the code to allocate a request.
> 
> Will fix that.
> 
>>
>>> +Put into code, after obtaining a request, user-space can assign controls and one
>>> +OUTPUT buffer to it:
>>> +
>>> +     struct v4l2_buf buf;
>>> +     struct v4l2_ext_controls ctrls;
>>> +     ...
>>> +     ctrls.request_fd = req_fd;
>>> +     ...
>>> +     ioctl(codec_fd, VIDIOC_S_EXT_CTRLS, &ctrls);
>>> +     ...
>>> +     buf.type = V4L2_BUF_TYPE_VIDEO_OUTPUT;
>>> +     buf.request_fd = req_fd;
>>> +     ioctl(codec_fd, VIDIOC_QBUF, &buf);
>>> +
>>> +Note that request_fd shall not be specified for CAPTURE buffers.
>>
>> I know why, but the less experienced reader won't. So this needs some additional
>> explanation.
> 
> Sure.
> 
>>
>>> +
>>> +Once the request is fully prepared, it can be submitted to the driver:
>>> +
>>> +     struct media_request_cmd cmd;
>>> +
>>> +     memset(&cmd, 0, sizeof(cmd));
>>> +     cmd.cmd = MEDIA_REQ_CMD_SUBMIT;
>>> +     cmd.fd = request_fd;
>>> +     ioctl(media_fd, MEDIA_IOC_REQUEST_CMD, &cmd);
>>> +
>>> +User-space can then lookup the request_fd field of dequeued capture buffers to
>>> +confirm which one has been produced by the request.
>>> +
>>> +     struct v4l2_buf buf;
>>> +
>>> +     memset(&buf, 0, sizeof(buf));
>>> +     buf.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
>>> +     ioctl(codec_fd, VIDIOC_DQBUF, &buf);
>>> +
>>> +     if (buf.request_fd == request_fd)
>>> +             ...
>>> +
>>
>> I'm not sure this is correct. You set the request for the OUTPUT buffer, so when
>> you dequeue the OUTPUT buffer, then the request_fd is set to the original request.
>> But this is the CAPTURE buffer, and that has no requests at all.
> 
> This is not a mistake actually. The idea is to allow user-space to
> know which request produced a given capture buffer. One output buffer
> can in theory produce several or no capture buffers, and I think this
> is the only reliable way to associate them with their producing
> request.

You can't do this. There is no reason why the CAPTURE queue can't use requests
as well (the queues are independent). Although for the codec use-case it is unlikely
to happen, other use-cases may want requests for both OUTPUT and CAPTURE.

Being able to tell which output buffer was the origin of a capture buffer is
an unrelated problem. We never clearly defined how this should be done.

The reality is that none of the existing options (using the sequence number or
the timestamp) are satisfactory IMHO. What you want is a new field for this.
The problem with that is that struct v4l2_buffer is full, so a new API is needed.
It's something I've been working on (see link below), but it's on hold until
we have the fence and request API merged. Otherwise too many people are messing
with the same structs...

https://git.linuxtv.org/hverkuil/media_tree.git/commit/?h=v4l2-buffer&id=a95549df06d9900f3559afdbb9da06bd4b22d1f3

> 
>>
>> BTW, you also want to show an example of getting the control once the request is
>> completed (e.g. status information), probably with a polling example as well.
> 
> True.
> 
>>
>>> +Example for a Simple Capture Device
>>> +-----------------------------------
>>> +
>>> +With a simple capture device, requests can be used to specify controls to apply
>>> +to a given CAPTURE buffer. The driver will apply these controls before producing
>>> +the marked CAPTURE buffer.
>>> +
>>> +     struct v4l2_buf buf;
>>> +     struct v4l2_ext_controls ctrls;
>>> +     ...
>>> +     ctrls.request_fd = req_fd;
>>> +     ...
>>> +     ioctl(camera_fd, VIDIOC_S_EXT_CTRLS, &ctrls);
>>> +     ...
>>> +     buf.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
>>> +     buf.request_fd = req_fd;
>>> +     ioctl(camera_fd, VIDIOC_QBUF, &buf);
>>> +
>>> +Once the request is fully prepared, it can be submitted to the driver:
>>> +
>>> +     struct media_request_cmd cmd;
>>> +
>>> +     memset(&cmd, 0, sizeof(cmd));
>>> +     cmd.cmd = MEDIA_REQ_CMD_SUBMIT;
>>> +     cmd.fd = request_fd;
>>> +     ioctl(media_fd, MEDIA_IOC_REQUEST_CMD, &cmd);
>>> +
>>> +User-space can then lookup the request_fd field of dequeued capture buffers to
>>> +confirm which one has been produced by the request.
>>> +
>>> +     struct v4l2_buf buf;
>>> +
>>> +     memset(&buf, 0, sizeof(buf));
>>> +     buf.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
>>> +     ioctl(camera_fd, VIDIOC_DQBUF, &buf);
>>> +
>>> +     if (buf.request_fd == request_fd)
>>> +             ...
>>> diff --git a/Documentation/media/uapi/v4l/vidioc-qbuf.rst b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
>>> index 9e448a4aa3aa..0cd596bd4cde 100644
>>> --- a/Documentation/media/uapi/v4l/vidioc-qbuf.rst
>>> +++ b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
>>> @@ -98,6 +98,12 @@ dequeued, until the :ref:`VIDIOC_STREAMOFF <VIDIOC_STREAMON>` or
>>>  :ref:`VIDIOC_REQBUFS` ioctl is called, or until the
>>>  device is closed.
>>>
>>> +For all buffer types, the ``request_fd`` field can be used when enqueing
>>> +to specify the file descriptor of a request, if requests are in use.
>>> +Setting it means that the buffer will not be passed to the driver until
>>> +the request itself is submitted. Also, the driver will apply any setting
>>> +associated with the request before processing the buffer.
>>> +
>>>  Applications call the ``VIDIOC_DQBUF`` ioctl to dequeue a filled
>>>  (capturing) or displayed (output) buffer from the driver's outgoing
>>>  queue. They just set the ``type``, ``memory`` and ``reserved`` fields of
>>> @@ -115,6 +121,21 @@ queue. When the ``O_NONBLOCK`` flag was given to the
>>>  :ref:`open() <func-open>` function, ``VIDIOC_DQBUF`` returns
>>>  immediately with an ``EAGAIN`` error code when no buffer is available.
>>>
>>> +If a dequeued buffer was produced as the result of a request, then the
>>> +``request_fd`` field of :c:type:`v4l2_buffer` will be set to the file
>>> +descriptor of the request, regardless of whether the buffer was initially
>>> +queued with a request associated to it or not.
>>> +
>>> +RFC note: a request can be referenced by several FDs, especially if the
>>> +request is shared between different processes. Also a FD can be closed
>>> +after a request is submitted. In that case the FD does not make much sense.
>>> +Maybe it would be better to define request fd as
>>> +
>>> +    union { u32 request_fd; u32 request_id; }
>>> +
>>> +and use request_id when communicating a request to userspace so they can be
>>> +unambiguously referenced?
>>
>> I don't think this is a problem. At least we never had this issue with e.g.
>> dmabuf either. The request_fd must be a valid request fd for the process that
>> calls the ioctl. If not the ioctl will return an error. If process A has a
>> request fd 10 and process B calls the ioctl with request_fd set to 10 but it
>> didn't actually allocate the request or receive the fd from process A (there
>> is a mechanism for that, although I've forgotten the details).
> 
> For user-to-kernel communication this is true. What I had in mind was
> the other way around: kernel trying to specify an already-allocated
> request to userspace, like the above example of the capture buffer
> that is marked with the request that produced it.
> 
> At that time, the FD of the request may have been closed, or may have
> been passed to another process (which references the request using a
> different ID). That's what FDs are not reliable here.
> 

The capture example is wrong in any case as I explained above.

But I really don't think this is an issue if the request fds are properly
refcounted. It's never been an issue with dma-buf, and I think that's the
example that you should follow.

Yes, if a request_fd is returned that is closed, then trying to use it
will fail. But that's an application bug.

Regards,

	Hans

Re: [RFC PATCH 0/8] [media] Request API, take three

[Hans Verkuil]

On 01/31/2018 09:10 AM, Tomasz Figa wrote:
> Hi Hans,
> 
> Sorry for joining the party late.
> 
> On Wed, Jan 31, 2018 at 4:50 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> On 01/30/2018 07:31 AM, Alexandre Courbot wrote:
>>> Hi Hans,
>>>
>>> On Mon, Jan 29, 2018 at 8:21 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>>> On 01/26/2018 07:02 AM, Alexandre Courbot wrote:
>>>>> Howdy. Here is your bi-weekly request API redesign! ;)
>>>>>
>>>>> Again, this is a simple version that only implements the flow of requests,
>>>>> without applying controls. The intent is to get an agreement on a base to work
>>>>> on, since the previous versions went straight back to the redesign board.
>>>>>
>>>>> Highlights of this version:
>>>>>
>>>>> * As requested by Hans, request-bound buffers are now passed earlier to drivers,
>>>>> as early as the request itself is submitted. Doing it earlier is not be useful
>>>>> since the driver would not know the state of the request, and thus cannot do
>>>>> anything with the buffer. Drivers are now responsible for applying request
>>>>> parameters themselves.
>>>>>
>>>>> * As a consequence, there is no such thing as a "request queue" anymore. The
>>>>> flow of buffers decides the order in which requests are processed. Individual
>>>>> devices of the same pipeline can implement synchronization if needed, but this
>>>>> is beyond this first version.
>>>>>
>>>>> * VB2 code is still a bit shady. Some of it will interfere with the fences
>>>>> series, so I am waiting for the latter to land to do it properly.
>>>>>
>>>>> * Requests that are not yet submitted effectively act as fences on the buffers
>>>>> they own, resulting in the buffer queue being blocked until the request is
>>>>> submitted. An alternate design would be to only block the
>>>>> not-submitted-request's buffer and let further buffers pass before it, but since
>>>>> buffer order is becoming increasingly important I have decided to just block the
>>>>> queue. This is open to discussion though.
>>>>
>>>> I don't think we should mess with the order.
>>>
>>> Agreed, let's keep it that way then.
>>>
> 
> I'm not sure I'm following here. Does it mean (quoting Alex):
> 
> a) "Requests that are not yet submitted effectively act as fences on the buffers
>     they own, resulting in the buffer queue being blocked until the request is
>     submitted."
> 
> b) "block the not-submitted-request's buffer and let further buffers
> pass before it"
> 
> or neither?
> 
> Hans, could you clarify what you think is the right behavior here?

I think I misread the text. Alexandre, buffers in not-yet-submitted requests
should not act as fences. They are similar to buffers that are prepared
through the VIDIOC_PREPARE_BUF call. They are just sitting there and won't
be in the queue until someone calls QBUF.

It's the same for not-yet-submitted requests: the buffers don't act as
fences until the request is submitted.

Interesting related questions: how does VIDIOC_PREPARE_BUF interact with
a request? What happens if a buffer is added to a non-yet-submitted request
and userspace calls VIDIOC_QBUF with that same buffer but with no request?

This all needs to be defined.

> 
>>>>> * For the codec usecase, I have experimented a bit marking CAPTURE buffers with
>>>>> the request FD that produced them (vim2m acts that way). This seems to work
>>>>> well, however FDs are process-local and could be closed before the CAPTURE
>>>>> buffer is dequeued, rendering that information less meaningful, if not
>>>>> dangerous.
>>>>
>>>> I don't follow this. Once the fd is passed to the kernel its refcount should be
>>>> increased so the data it represents won't be released if userspace closes the fd.
>>>
>>> The refcount of the request is increased. The refcount of the FD is
>>> not, since it is only a userspace reference to the request.
>>
>> I don't think that's right. Looking at how dma-buf does this (dma_buf_get in
>> dma-buf.c) it calls fget(fd) which increases the fd refcount. In fact, as far as
>> I can see the struct dma_buf doesn't have a refcount, it is solely refcounted
>> through the fd. That's probably the model you want to follow.
> 
> As far as I can see from the code, fget() on an fd increases a
> reference count on the struct file backing the fd. I don't think there
> is another level of reference counting of fds themselves - that's why
> dup(fd) gives you another fd and you can't call close(fd) on the same
> fd two times.
> 
>>
>>>
>>>>
>>>> Obviously if userspace closes the fd it cannot do anything with it anymore, but
>>>> it shouldn't be 'dangerous' AFAICT.
>>>
>>> It userspace later gets that closed FD back from a DQBUF call, and
>>> decides to use it again, then we would have a problem. I agree that it
>>> is userspace responsibility to be careful here, but making things
>>> foolproof never hurts.
>>
>> I think all the issues will go away if you refcount the fd instead of the
>> request. It worked well for dma-buf for years.
> 
> I'm confused here. The kernel never returns the same FD for the same
> DMA-buf twice. Every time an ioctl is called, which returns a DMA-buf
> FD, it creates a new FD.

Sorry, where I say 'refcount the fd' I indeed meant 'refcount the file struct'.

Looking at the code in the patch series I see:

+struct media_request *
+media_request_get_from_fd(int fd)
+{
+	struct file *f;
+	struct media_request *req;
+
+	f = fget(fd);
+	if (!f)
+		return NULL;
+
+	/* Not a request FD? */
+	if (f->f_op != &request_fops) {
+		fput(f);
+		return NULL;
+	}
+
+	req = f->private_data;
+	media_request_get(req);
+	fput(f);
+
+	return req;
+}
+EXPORT_SYMBOL_GPL(media_request_get_from_fd);

So this does not refcount the file struct but the request struct, and I think
that's wrong. The file struct represents the request struct, and it is the
file struct that should be refcounted. So when you give a request_fd to
VIDIOC_QBUF, then the refcount should be increased since vb2 now needs to
hold on to it. It's what dma-buf does.

Now, I agree that if userspace closes the fd before kernelspace is finished
with it, then the request_fd returned by VIDIOC_DQBUF is no longer valid.
But so what? It's the same with the dma-buf fd. I don't think this is a problem
at all.

Regards,

	Hans

Re: [RFC PATCH 0/8] [media] Request API, take three

[Tomasz Figa]

On Wed, Jan 31, 2018 at 5:47 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> On 01/31/2018 09:10 AM, Tomasz Figa wrote:
>> Hi Hans,
>>
>> Sorry for joining the party late.
>>
>> On Wed, Jan 31, 2018 at 4:50 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>> On 01/30/2018 07:31 AM, Alexandre Courbot wrote:
>>>> Hi Hans,
>>>>
>>>> On Mon, Jan 29, 2018 at 8:21 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>>>> On 01/26/2018 07:02 AM, Alexandre Courbot wrote:
>>>>>> Howdy. Here is your bi-weekly request API redesign! ;)
>>>>>>
>>>>>> Again, this is a simple version that only implements the flow of requests,
>>>>>> without applying controls. The intent is to get an agreement on a base to work
>>>>>> on, since the previous versions went straight back to the redesign board.
>>>>>>
>>>>>> Highlights of this version:
>>>>>>
>>>>>> * As requested by Hans, request-bound buffers are now passed earlier to drivers,
>>>>>> as early as the request itself is submitted. Doing it earlier is not be useful
>>>>>> since the driver would not know the state of the request, and thus cannot do
>>>>>> anything with the buffer. Drivers are now responsible for applying request
>>>>>> parameters themselves.
>>>>>>
>>>>>> * As a consequence, there is no such thing as a "request queue" anymore. The
>>>>>> flow of buffers decides the order in which requests are processed. Individual
>>>>>> devices of the same pipeline can implement synchronization if needed, but this
>>>>>> is beyond this first version.
>>>>>>
>>>>>> * VB2 code is still a bit shady. Some of it will interfere with the fences
>>>>>> series, so I am waiting for the latter to land to do it properly.
>>>>>>
>>>>>> * Requests that are not yet submitted effectively act as fences on the buffers
>>>>>> they own, resulting in the buffer queue being blocked until the request is
>>>>>> submitted. An alternate design would be to only block the
>>>>>> not-submitted-request's buffer and let further buffers pass before it, but since
>>>>>> buffer order is becoming increasingly important I have decided to just block the
>>>>>> queue. This is open to discussion though.
>>>>>
>>>>> I don't think we should mess with the order.
>>>>
>>>> Agreed, let's keep it that way then.
>>>>
>>
>> I'm not sure I'm following here. Does it mean (quoting Alex):
>>
>> a) "Requests that are not yet submitted effectively act as fences on the buffers
>>     they own, resulting in the buffer queue being blocked until the request is
>>     submitted."
>>
>> b) "block the not-submitted-request's buffer and let further buffers
>> pass before it"
>>
>> or neither?
>>
>> Hans, could you clarify what you think is the right behavior here?
>
> I think I misread the text. Alexandre, buffers in not-yet-submitted requests
> should not act as fences. They are similar to buffers that are prepared
> through the VIDIOC_PREPARE_BUF call. They are just sitting there and won't
> be in the queue until someone calls QBUF.
>
> It's the same for not-yet-submitted requests: the buffers don't act as
> fences until the request is submitted.

Thanks for explanation.

To be 100% sure, that would mean that a buffer is added into the vb2
queue as soon as the request that includes related QBUF call is
queued, right?

>
> Interesting related questions: how does VIDIOC_PREPARE_BUF interact with
> a request? What happens if a buffer is added to a non-yet-submitted request
> and userspace calls VIDIOC_QBUF with that same buffer but with no request?
>
> This all needs to be defined.

That sounds like a failure to me. Actually very similar to trying to
call QBUF twice on the same buffer, without dequeuing it. What would
be the behavior in such case?

>
>>
>>>>>> * For the codec usecase, I have experimented a bit marking CAPTURE buffers with
>>>>>> the request FD that produced them (vim2m acts that way). This seems to work
>>>>>> well, however FDs are process-local and could be closed before the CAPTURE
>>>>>> buffer is dequeued, rendering that information less meaningful, if not
>>>>>> dangerous.
>>>>>
>>>>> I don't follow this. Once the fd is passed to the kernel its refcount should be
>>>>> increased so the data it represents won't be released if userspace closes the fd.
>>>>
>>>> The refcount of the request is increased. The refcount of the FD is
>>>> not, since it is only a userspace reference to the request.
>>>
>>> I don't think that's right. Looking at how dma-buf does this (dma_buf_get in
>>> dma-buf.c) it calls fget(fd) which increases the fd refcount. In fact, as far as
>>> I can see the struct dma_buf doesn't have a refcount, it is solely refcounted
>>> through the fd. That's probably the model you want to follow.
>>
>> As far as I can see from the code, fget() on an fd increases a
>> reference count on the struct file backing the fd. I don't think there
>> is another level of reference counting of fds themselves - that's why
>> dup(fd) gives you another fd and you can't call close(fd) on the same
>> fd two times.
>>
>>>
>>>>
>>>>>
>>>>> Obviously if userspace closes the fd it cannot do anything with it anymore, but
>>>>> it shouldn't be 'dangerous' AFAICT.
>>>>
>>>> It userspace later gets that closed FD back from a DQBUF call, and
>>>> decides to use it again, then we would have a problem. I agree that it
>>>> is userspace responsibility to be careful here, but making things
>>>> foolproof never hurts.
>>>
>>> I think all the issues will go away if you refcount the fd instead of the
>>> request. It worked well for dma-buf for years.
>>
>> I'm confused here. The kernel never returns the same FD for the same
>> DMA-buf twice. Every time an ioctl is called, which returns a DMA-buf
>> FD, it creates a new FD.
>
> Sorry, where I say 'refcount the fd' I indeed meant 'refcount the file struct'.
>
> Looking at the code in the patch series I see:
>
> +struct media_request *
> +media_request_get_from_fd(int fd)
> +{
> +       struct file *f;
> +       struct media_request *req;
> +
> +       f = fget(fd);
> +       if (!f)
> +               return NULL;
> +
> +       /* Not a request FD? */
> +       if (f->f_op != &request_fops) {
> +               fput(f);
> +               return NULL;
> +       }
> +
> +       req = f->private_data;
> +       media_request_get(req);
> +       fput(f);
> +
> +       return req;
> +}
> +EXPORT_SYMBOL_GPL(media_request_get_from_fd);
>
> So this does not refcount the file struct but the request struct, and I think
> that's wrong. The file struct represents the request struct, and it is the
> file struct that should be refcounted. So when you give a request_fd to
> VIDIOC_QBUF, then the refcount should be increased since vb2 now needs to
> hold on to it. It's what dma-buf does.
>
> Now, I agree that if userspace closes the fd before kernelspace is finished
> with it, then the request_fd returned by VIDIOC_DQBUF is no longer valid.
> But so what? It's the same with the dma-buf fd. I don't think this is a problem
> at all.

Okay, I feel like we're much more on the same page now, thanks for
clarification. ;)

I'm still wondering if having the link from-buffer-to-request is the
most convenient for userspace, though. One possible scenario where I
think this might be problematic is when the request FD is shared with
another process, e.g. one process creating the requests, filling them
in and submitting and separate process dequeuing and handling
completed requests. In this case, the FD used for queuing the request
and the FD used for dequeuing the request would be different. Not sure
how we could handle it.

An alternative approach that would let us completely avoid this kind
of unique identifier problem, would bo to have links
from-request-to-buffers. The process that dequeues the requests would
then query the dequeued request for the buffers related to it. For
example, DQBUF could be made symmetric to QBUF with respect to the
request FD and it would only dequeue buffers related to given request.

Best regards,
Tomasz

Re: [RFC PATCH 3/8] media: videobuf2: add support for requests

[Sakari Ailus]

Hi Alexandre,

On Fri, Jan 26, 2018 at 03:02:11PM +0900, Alexandre Courbot wrote:
> Make vb2 aware of requests. Drivers can specify whether a given queue
> can accept requests or not. Queues that accept requests will block on a
> buffer that is part of a request until that request is submitted.
> 
> Signed-off-by: Alexandre Courbot <acourbot@chromium.org>
> ---
>  drivers/media/v4l2-core/videobuf2-core.c | 125 +++++++++++++++++++++++++++++--
>  drivers/media/v4l2-core/videobuf2-v4l2.c |  28 ++++++-
>  include/media/videobuf2-core.h           |  15 +++-
>  3 files changed, 160 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> index cb115ba6a1d2..f6d013b141f1 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -26,6 +26,7 @@
>  
>  #include <media/videobuf2-core.h>
>  #include <media/v4l2-mc.h>
> +#include <media/media-request.h>
>  
>  #include <trace/events/vb2.h>
>  
> @@ -922,6 +923,17 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state)
>  		vb->state = state;
>  	}
>  	atomic_dec(&q->owned_by_drv_count);
> +	if (vb->request) {
> +		struct media_request *req = vb->request;
> +
> +		if (atomic_dec_and_test(&req->buf_cpt))
> +			media_request_complete(vb->request);
> +
> +		/* release reference acquired during qbuf */
> +		vb->request = NULL;
> +		media_request_put(req);
> +	}
> +
>  	spin_unlock_irqrestore(&q->done_lock, flags);
>  
>  	trace_vb2_buf_done(q, vb);
> @@ -1298,6 +1310,53 @@ int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb)
>  }
>  EXPORT_SYMBOL_GPL(vb2_core_prepare_buf);
>  
> +/**
> + * vb2_check_buf_req_status() - Validate request state of a buffer
> + * @vb:		buffer to check
> + *
> + * Returns true if a buffer is ready to be passed to the driver request-wise.
> + * This means that neither this buffer nor any previously-queued buffer is
> + * associated to a request that is not yet submitted.
> + *
> + * If this function returns false, then the buffer shall not be passed to its
> + * driver since the request state is not completely built yet. In that case,
> + * this function will register a notifier to be called when the request is
> + * submitted and the queue can be unblocked.
> + *
> + * This function must be called with req_lock held.
> + */
> +static bool vb2_check_buf_req_status(struct vb2_buffer *vb)
> +{
> +	struct media_request *req = vb->request;
> +	struct vb2_queue *q = vb->vb2_queue;
> +	int ret = false;
> +
> +	mutex_lock(&q->req_lock);
> +
> +	if (!req) {
> +		ret = !q->waiting_req;
> +		goto done;
> +	}
> +
> +	mutex_lock(&req->lock);
> +	if (req->state == MEDIA_REQUEST_STATE_SUBMITTED) {

Would it make sense to serialise access to request state using the queue
lock instead? The queue and the state are often accessed together.

The function itself seems weird. The VB2 framework does not have enough
information to determine whether a request is complete; this is something a
driver must evaluate at the time of queueing the request.

> +		mutex_unlock(&req->lock);
> +		ret = !q->waiting_req;
> +		goto done;
> +	}
> +
> +	if (!q->waiting_req) {
> +		q->waiting_req = true;
> +		atomic_notifier_chain_register(&req->submit_notif,
> +					       &q->req_blk);
> +	}
> +	mutex_unlock(&req->lock);
> +
> +done:
> +	mutex_unlock(&q->req_lock);
> +	return ret;
> +}
> +
>  /**
>   * vb2_start_streaming() - Attempt to start streaming.
>   * @q:		videobuf2 queue
> @@ -1318,8 +1377,11 @@ static int vb2_start_streaming(struct vb2_queue *q)
>  	 * If any buffers were queued before streamon,
>  	 * we can now pass them to driver for processing.
>  	 */
> -	list_for_each_entry(vb, &q->queued_list, queued_entry)
> +	list_for_each_entry(vb, &q->queued_list, queued_entry) {
> +		if (!vb2_check_buf_req_status(vb))
> +			break;
>  		__enqueue_in_driver(vb);

Queueing buffers to drivers this way hardly makes sense with requests. The
driver has no use for it, all it cares about is the request.

VB2 could manage its own state of the buffer this way but that's only
useful from V4L2 API point of view, it has no functional purpose. This
suggests we need an overhaul of the existing buffer management for
requests.

> +	}
>  
>  	/* Tell the driver to start streaming */
>  	q->start_streaming_called = 1;
> @@ -1361,7 +1423,46 @@ static int vb2_start_streaming(struct vb2_queue *q)
>  	return ret;
>  }
>  
> -int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
> +/**
> + * vb2_unblock_requests() - unblock a queue waiting for a request submission
> + * @nb:		notifier block that has been registered
> + * @action:	unused
> + * @data:	request that has been submitted
> + *
> + * This is a callback function that is registered when
> + * vb2_check_buf_req_status() returns false. It is invoked when the request
> + * blocking the queue has been submitted. This means its buffers (and all
> + * following valid buffers) can be passed to drivers.

This is a perplexing one. Why would you not queue the request to the driver
at the time of... it is queued, by the user? The VB2 framework (nor the
media or V4L2 frameworks to that matter) does not have enough information
in order to help the driver here.

> + */
> +static int vb2_unblock_requests(struct notifier_block *nb, unsigned long action,
> +				void *data)
> +{
> +	struct vb2_queue *q = container_of(nb, struct vb2_queue, req_blk);
> +	struct media_request *req = data;
> +	struct vb2_buffer *vb;
> +	bool found_request = false;
> +
> +	mutex_lock(&q->req_lock);
> +	atomic_notifier_chain_unregister(&req->submit_notif, &q->req_blk);
> +	q->waiting_req = false;
> +	mutex_unlock(&q->req_lock);
> +
> +	list_for_each_entry(vb, &q->queued_list, queued_entry) {
> +		/* All buffers before our request are already passed to the driver */
> +		if (!found_request && vb->request != req)
> +			continue;
> +		found_request = true;
> +
> +		if (!vb2_check_buf_req_status(vb))
> +			break;
> +		__enqueue_in_driver(vb);
> +	}
> +
> +	return 0;
> +}
> +
> +int vb2_core_qbuf(struct vb2_queue *q, unsigned int index,
> +		  struct media_request *req, void *pb)
>  {
>  	struct vb2_buffer *vb;
>  	int ret;
> @@ -1398,11 +1499,21 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb)
>  
>  	trace_vb2_qbuf(q, vb);
>  
> +	vb->request = req;
> +	if (req) {
> +		if (!q->allow_requests)
> +			return -EINVAL;
> +
> +		/* make sure the request stays alive as long as we need */
> +		media_request_get(req);
> +		atomic_inc(&req->buf_cpt);
> +	}
> +
>  	/*
>  	 * 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)
> +	if (q->start_streaming_called && vb2_check_buf_req_status(vb))
>  		__enqueue_in_driver(vb);
>  
>  	/* Fill buffer information for the userspace */
> @@ -1993,6 +2104,8 @@ int vb2_core_queue_init(struct vb2_queue *q)
>  	spin_lock_init(&q->done_lock);
>  	mutex_init(&q->mmap_lock);
>  	init_waitqueue_head(&q->done_wq);
> +	mutex_init(&q->req_lock);
> +	q->req_blk.notifier_call = vb2_unblock_requests;
>  
>  	if (q->buf_struct_size == 0)
>  		q->buf_struct_size = sizeof(struct vb2_buffer);
> @@ -2242,7 +2355,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;
> @@ -2421,7 +2534,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;
> @@ -2524,7 +2637,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 0f8edbdebe30..267fe2d669b2 100644
> --- a/drivers/media/v4l2-core/videobuf2-v4l2.c
> +++ b/drivers/media/v4l2-core/videobuf2-v4l2.c
> @@ -30,6 +30,7 @@
>  #include <media/v4l2-common.h>
>  
>  #include <media/videobuf2-v4l2.h>
> +#include <media/media-request.h>

Alphabetical order, please.

>  
>  static int debug;
>  module_param(debug, int, 0644);
> @@ -561,6 +562,7 @@ EXPORT_SYMBOL_GPL(vb2_create_bufs);
>  
>  int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
>  {
> +	struct media_request *req = NULL;
>  	int ret;
>  
>  	if (vb2_fileio_is_active(q)) {
> @@ -568,8 +570,32 @@ int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
>  		return -EBUSY;
>  	}
>  
> +	/*
> +	 * The caller should have validated that the request is valid,
> +	 * so we just need to look it up without further checking

Why? This would mean you'll need to look up a request twice per IOCTL.

> +	 */
> +	if (b->request_fd > 0) {
> +		req = media_request_get_from_fd(b->request_fd);
> +		if (!req)
> +			return -EINVAL;
> +
> +		mutex_lock(&req->lock);
> +		if (req->state != MEDIA_REQUEST_STATE_IDLE) {
> +			mutex_unlock(&req->lock);
> +			media_request_put(req);
> +			return -EINVAL;
> +		}
> +		mutex_unlock(&req->lock);
> +	}
> +
>  	ret = vb2_queue_or_prepare_buf(q, b, "qbuf");
> -	return ret ? ret : vb2_core_qbuf(q, b->index, b);
> +	if (!ret)
> +		ret = vb2_core_qbuf(q, b->index, req, b);
> +
> +	if (req)
> +		media_request_put(req);
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(vb2_qbuf);
>  
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index ef9b64398c8c..7bb17c842ab4 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -237,6 +237,7 @@ struct vb2_queue;
>   *			on an internal driver queue
>   * @planes:		private per-plane information; do not change
>   * @timestamp:		frame timestamp in ns
> + * @request:		request the buffer belongs to, if any
>   */
>  struct vb2_buffer {
>  	struct vb2_queue	*vb2_queue;
> @@ -246,6 +247,7 @@ struct vb2_buffer {
>  	unsigned int		num_planes;
>  	struct vb2_plane	planes[VB2_MAX_PLANES];
>  	u64			timestamp;
> +	struct media_request	*request;
>  
>  	/* private: internal use only
>  	 *
> @@ -443,6 +445,7 @@ struct vb2_buf_ops {
>   * @quirk_poll_must_check_waiting_for_buffers: Return POLLERR at poll when QBUF
>   *              has not been called. This is a vb1 idiom that has been adopted
>   *              also by vb2.
> + * @allow_requests:	whether requests are supported on this queue.
>   * @lock:	pointer to a mutex that protects the vb2_queue struct. The
>   *		driver can set this to a mutex to let the v4l2 core serialize
>   *		the queuing ioctls. If the driver wants to handle locking
> @@ -500,6 +503,9 @@ struct vb2_buf_ops {
>   *		when a buffer with the V4L2_BUF_FLAG_LAST is dequeued.
>   * @fileio:	file io emulator internal data, used only if emulator is active
>   * @threadio:	thread io internal data, used only if thread is active
> + * @req_lock:	protects req_blk and waiting_req
> + * @req_blk:	notifier to be called when waiting for a request to be submitted
> + * @waiting_req:whether this queue is currently waiting on a request submission
>   */
>  struct vb2_queue {
>  	unsigned int			type;
> @@ -511,6 +517,7 @@ struct vb2_queue {
>  	unsigned			fileio_write_immediately:1;
>  	unsigned			allow_zero_bytesused:1;
>  	unsigned		   quirk_poll_must_check_waiting_for_buffers:1;
> +	unsigned			allow_requests:1;
>  
>  	struct mutex			*lock;
>  	void				*owner;
> @@ -554,6 +561,10 @@ struct vb2_queue {
>  	struct vb2_fileio_data		*fileio;
>  	struct vb2_threadio_data	*threadio;
>  
> +	struct mutex			req_lock;
> +	struct notifier_block		req_blk;
> +	bool				waiting_req;
> +
>  #ifdef CONFIG_VIDEO_ADV_DEBUG
>  	/*
>  	 * Counters for how often these queue-related ops are
> @@ -724,6 +735,7 @@ int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb);
>   *
>   * @q:		videobuf2 queue
>   * @index:	id number of the buffer
> + * @req:	request this buffer belongs to, if any
>   * @pb:		buffer structure passed from userspace to vidioc_qbuf handler
>   *		in driver
>   *
> @@ -740,7 +752,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,
> +		  struct media_request *req, void *pb);
>  
>  /**
>   * vb2_core_dqbuf() - Dequeue a buffer to the userspace

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

Re: [RFC PATCH 0/8] [media] Request API, take three

[Hans Verkuil]

On 01/31/2018 10:46 AM, Tomasz Figa wrote:
> On Wed, Jan 31, 2018 at 5:47 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> On 01/31/2018 09:10 AM, Tomasz Figa wrote:
>>> Hi Hans,
>>>
>>> Sorry for joining the party late.
>>>
>>> On Wed, Jan 31, 2018 at 4:50 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>>> On 01/30/2018 07:31 AM, Alexandre Courbot wrote:
>>>>> Hi Hans,
>>>>>
>>>>> On Mon, Jan 29, 2018 at 8:21 PM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>>>>> On 01/26/2018 07:02 AM, Alexandre Courbot wrote:
>>>>>>> Howdy. Here is your bi-weekly request API redesign! ;)
>>>>>>>
>>>>>>> Again, this is a simple version that only implements the flow of requests,
>>>>>>> without applying controls. The intent is to get an agreement on a base to work
>>>>>>> on, since the previous versions went straight back to the redesign board.
>>>>>>>
>>>>>>> Highlights of this version:
>>>>>>>
>>>>>>> * As requested by Hans, request-bound buffers are now passed earlier to drivers,
>>>>>>> as early as the request itself is submitted. Doing it earlier is not be useful
>>>>>>> since the driver would not know the state of the request, and thus cannot do
>>>>>>> anything with the buffer. Drivers are now responsible for applying request
>>>>>>> parameters themselves.
>>>>>>>
>>>>>>> * As a consequence, there is no such thing as a "request queue" anymore. The
>>>>>>> flow of buffers decides the order in which requests are processed. Individual
>>>>>>> devices of the same pipeline can implement synchronization if needed, but this
>>>>>>> is beyond this first version.
>>>>>>>
>>>>>>> * VB2 code is still a bit shady. Some of it will interfere with the fences
>>>>>>> series, so I am waiting for the latter to land to do it properly.
>>>>>>>
>>>>>>> * Requests that are not yet submitted effectively act as fences on the buffers
>>>>>>> they own, resulting in the buffer queue being blocked until the request is
>>>>>>> submitted. An alternate design would be to only block the
>>>>>>> not-submitted-request's buffer and let further buffers pass before it, but since
>>>>>>> buffer order is becoming increasingly important I have decided to just block the
>>>>>>> queue. This is open to discussion though.
>>>>>>
>>>>>> I don't think we should mess with the order.
>>>>>
>>>>> Agreed, let's keep it that way then.
>>>>>
>>>
>>> I'm not sure I'm following here. Does it mean (quoting Alex):
>>>
>>> a) "Requests that are not yet submitted effectively act as fences on the buffers
>>>     they own, resulting in the buffer queue being blocked until the request is
>>>     submitted."
>>>
>>> b) "block the not-submitted-request's buffer and let further buffers
>>> pass before it"
>>>
>>> or neither?
>>>
>>> Hans, could you clarify what you think is the right behavior here?
>>
>> I think I misread the text. Alexandre, buffers in not-yet-submitted requests
>> should not act as fences. They are similar to buffers that are prepared
>> through the VIDIOC_PREPARE_BUF call. They are just sitting there and won't
>> be in the queue until someone calls QBUF.
>>
>> It's the same for not-yet-submitted requests: the buffers don't act as
>> fences until the request is submitted.
> 
> Thanks for explanation.
> 
> To be 100% sure, that would mean that a buffer is added into the vb2
> queue as soon as the request that includes related QBUF call is
> queued, right?

Correct.

> 
>>
>> Interesting related questions: how does VIDIOC_PREPARE_BUF interact with
>> a request? What happens if a buffer is added to a non-yet-submitted request
>> and userspace calls VIDIOC_QBUF with that same buffer but with no request?
>>
>> This all needs to be defined.
> 
> That sounds like a failure to me. Actually very similar to trying to
> call QBUF twice on the same buffer, without dequeuing it. What would
> be the behavior in such case?

I would say that it makes no sense to use VIDIOC_PREPARE_BUF with a request
and should be considered an error. Although it is perfectly fine IMHO to
queue a previously prepared buffer to a request.

Calling QBUF for a buffer that is added to a not-yet-submitted request is an
error. I would expect something like -EBUSY to be returned.

> 
>>
>>>
>>>>>>> * For the codec usecase, I have experimented a bit marking CAPTURE buffers with
>>>>>>> the request FD that produced them (vim2m acts that way). This seems to work
>>>>>>> well, however FDs are process-local and could be closed before the CAPTURE
>>>>>>> buffer is dequeued, rendering that information less meaningful, if not
>>>>>>> dangerous.
>>>>>>
>>>>>> I don't follow this. Once the fd is passed to the kernel its refcount should be
>>>>>> increased so the data it represents won't be released if userspace closes the fd.
>>>>>
>>>>> The refcount of the request is increased. The refcount of the FD is
>>>>> not, since it is only a userspace reference to the request.
>>>>
>>>> I don't think that's right. Looking at how dma-buf does this (dma_buf_get in
>>>> dma-buf.c) it calls fget(fd) which increases the fd refcount. In fact, as far as
>>>> I can see the struct dma_buf doesn't have a refcount, it is solely refcounted
>>>> through the fd. That's probably the model you want to follow.
>>>
>>> As far as I can see from the code, fget() on an fd increases a
>>> reference count on the struct file backing the fd. I don't think there
>>> is another level of reference counting of fds themselves - that's why
>>> dup(fd) gives you another fd and you can't call close(fd) on the same
>>> fd two times.
>>>
>>>>
>>>>>
>>>>>>
>>>>>> Obviously if userspace closes the fd it cannot do anything with it anymore, but
>>>>>> it shouldn't be 'dangerous' AFAICT.
>>>>>
>>>>> It userspace later gets that closed FD back from a DQBUF call, and
>>>>> decides to use it again, then we would have a problem. I agree that it
>>>>> is userspace responsibility to be careful here, but making things
>>>>> foolproof never hurts.
>>>>
>>>> I think all the issues will go away if you refcount the fd instead of the
>>>> request. It worked well for dma-buf for years.
>>>
>>> I'm confused here. The kernel never returns the same FD for the same
>>> DMA-buf twice. Every time an ioctl is called, which returns a DMA-buf
>>> FD, it creates a new FD.
>>
>> Sorry, where I say 'refcount the fd' I indeed meant 'refcount the file struct'.
>>
>> Looking at the code in the patch series I see:
>>
>> +struct media_request *
>> +media_request_get_from_fd(int fd)
>> +{
>> +       struct file *f;
>> +       struct media_request *req;
>> +
>> +       f = fget(fd);
>> +       if (!f)
>> +               return NULL;
>> +
>> +       /* Not a request FD? */
>> +       if (f->f_op != &request_fops) {
>> +               fput(f);
>> +               return NULL;
>> +       }
>> +
>> +       req = f->private_data;
>> +       media_request_get(req);
>> +       fput(f);
>> +
>> +       return req;
>> +}
>> +EXPORT_SYMBOL_GPL(media_request_get_from_fd);
>>
>> So this does not refcount the file struct but the request struct, and I think
>> that's wrong. The file struct represents the request struct, and it is the
>> file struct that should be refcounted. So when you give a request_fd to
>> VIDIOC_QBUF, then the refcount should be increased since vb2 now needs to
>> hold on to it. It's what dma-buf does.
>>
>> Now, I agree that if userspace closes the fd before kernelspace is finished
>> with it, then the request_fd returned by VIDIOC_DQBUF is no longer valid.
>> But so what? It's the same with the dma-buf fd. I don't think this is a problem
>> at all.
> 
> Okay, I feel like we're much more on the same page now, thanks for
> clarification. ;)
> 
> I'm still wondering if having the link from-buffer-to-request is the
> most convenient for userspace, though. One possible scenario where I
> think this might be problematic is when the request FD is shared with
> another process, e.g. one process creating the requests, filling them
> in and submitting and separate process dequeuing and handling
> completed requests. In this case, the FD used for queuing the request
> and the FD used for dequeuing the request would be different. Not sure
> how we could handle it.

I don't think this is an issue. The way vb2 works is that whoever allocates
the buffers becomes the owner and only that process can queue and dequeue
buffers. Otherwise an external process could dequeue buffers from underneath
the main process.

So the process that queues the buffers for a request will be the same process
that dequeues them, and the request_fd is always valid for that process.

This is really an already existing limitation.

In other words, you can do this with separate threads but not with separate
processes. And this isn't an issue if you use threads since they use the
same fd.

Good question, though. I had to think carefully about this for a bit.

> An alternative approach that would let us completely avoid this kind
> of unique identifier problem, would bo to have links
> from-request-to-buffers. The process that dequeues the requests would
> then query the dequeued request for the buffers related to it. For
> example, DQBUF could be made symmetric to QBUF with respect to the
> request FD and it would only dequeue buffers related to given request.

Regards,

	Hans

Re: [RFC PATCH 6/8] v4l2: document the request API interface

[Sakari Ailus]

Hi Hans and Alexandre,

On Wed, Jan 31, 2018 at 09:26:10AM +0100, Hans Verkuil wrote:
> On 01/30/2018 07:31 AM, Alexandre Courbot wrote:
> > On Tue, Jan 30, 2018 at 1:03 AM, Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >> On 01/26/2018 07:02 AM, Alexandre Courbot wrote:

...

> >>> +Recycling and Destruction
> >>> +-------------------------
> >>> +
> >>> +Finally, completed request can either be discarded or be reused. Calling
> >>> +``close()`` on a request FD will make that FD unusable, freeing the request if
> >>> +it is not referenced elsewhere. The ``MEDIA_REQ_CMD_REINIT`` command will clear
> >>> +a request's state and make it available again. No state is retained by this
> >>> +operation: the request is as if it had just been allocated via
> >>> +``MEDIA_REQ_CMD_ALLOC``.
> >>> +
> >>> +RFC Note: Since requests are represented by FDs themselves, the
> >>> +``MEDIA_REQ_CMD_SUBMIT`` and ``MEDIA_REQ_CMD_REININT`` commands can be performed
> >>> +on the request FD instead of the media device. This means the media device would
> >>> +only need to manage ``MEDIA_REQ_CMD_ALLOC``, which could be turned into an
> >>> +ioctl, while ``MEDIA_REQ_CMD_SUBMIT`` and ``MEDIA_REQ_CMD_REININT`` would
> >>> +become ioctls on the request itself. This has the advantage of allowing
> >>> +receivers of a request FD to submit the request, and also decouples requests
> >>> +from the media device - a scenario that makes sense for stateless codecs where
> >>> +the media device is not really useful.
> >>
> >> I think allowing SUBMIT/REINIT to operate directly on the request fd makes sense.
> >>
> >> A few questions that I haven't seen answered here:
> >>
> >> - can I reinit a request if it hasn't yet been submitted? Or can this only be
> >>   done on completed requests?
> > 
> > Interesting question. Right now this is possible, but the behavior may
> > be not well defined.
> > 
> > It is easy to cancel applying the controls of the request, since they
> > only exist within it. Otherwise, we cannot say the same about queued
> > buffers. Should be dequeue them, or let them proceed without any
> > request associated? I suppose dequeuing would make the most sense
> > here.
> 
> Related to this: what happens if I close the request fd having already set
> values and queued buffers, but not yet submitted the request.

It's painful. The request will be gone so all resources related to it must
be released. That means the buffer must be returned to the user, with the
error flag set.

It'd be great if this would be fully managed with videobuf2 / MC request
frameworks.

> 
> It's a similar situation to calling REINIT, but this scenario is very real
> since an application can always do this, you can't prevent it.
> 
> So since you need to implement and define this close(request_fd) scenario
> regardless, I think REINIT can just use this.
> 
> > 
> >> - can I reinit (or perhaps also alloc) a request based on an existing request?
> >>   (sort of a clone operation). I can't remember what we decided in the Prague
> >>   meeting, sorry about that.
> > 
> > Not at the moment. IIRC we decided this was out of scope as the
> > correct behavior in that case is hard to define (do we clone the whole
> > state? only the state that has explicitly been set? how about queued
> > buffers?). We would need to discuss that in more detail if we want
> > such a feature.
> > 
> >> - a request effectively contains the things you want to change, so anything not
> >>   explicitly set in the request will have what value? This should be the current
> >>   hardware value with any changes specified in preceding requests on top.
> > 
> > That's what we agreed on, yes. I need to check whether your patchset
> > does exactly that, but it should be possible to adapt it in case it
> > doesn't yet.
> 
> I don't believe this first version does that. I'm waiting for working code
> that I can use to implement this. I need something to test with :-)
> 
> I remember that I had a good idea on how to implement this. I hope I can still
> remember what it was exactly when I start working on this :-)
> 
> > 
> >> - add an RFC note that requests do not have to complete in the same order that they
> >>   were queued. A codec driver can (at least in theory) hold on to a buffer
> >>   containing a key frame until that frame is no longer needed. But this also
> >>   relates to the fences support, so let's keep this an RFC for now until that
> >>   patch series has been merged.
> > 
> > Good point, agreed.
> > 
> >> - how do we know that the request API is supported by a driver? I think we
> >>   wanted a new capability for that?
> > 
> > For video devices, I think it may be more flexible if we can make it
> > part of the format. It would also allow us to specify more coarsely
> > the extend to which requests are acceptable (e.g. for codec drivers,
> > the OUTPUT queue would accept them, but not the CAPTURE queue).
> 
> I'd say, make a proposal and we can discuss this further.
> 
> In general I always felt that we are missing capabilities that tell userspace
> what a DMA queue can do (e.g. which of the MMAP, USERPTR and DMA_BUF streaming
> modes are supported, is the state per-filehandle or global, can it support the
> request API). It would be nice to have a solution that takes this higher level
> view into account.
> 
> > I am not sure whether media devices have capabilities of some sort? Or
> > maybe we can just rely on MEDIA_REQ_CMD_ALLOC returning -ENOSYS to
> > detect that?
> 
> It's not a MC capability IMHO. Other than that if the MC cannot allocate
> requests the MEDIA_REQ_CMD_ALLOC ioctl would return -ENOTTY.

You could use other request commands, too, without side effects such as
allocating a request. The error code would tell you (-ENOTTY vs. something
else such as -EINVAL).

> 
> > 
> >> - it is allowed to query the values of a submitted request, or will that return
> >>   -EBUSY while it is not completed? I see no reason not to allow it.
> > 
> > As of know this is working and will return either the hardware value
> > if a control is not set by the request, or the value set in the
> > request if it is.
> 
> To be precise: it should return the value from the last queued request that
> sets it, or from the hardware if there is no such request.
> 
> > 
> >> - what happens when a value inside a request can't be applied? (e.g. due to a
> >>   hardware error or some other unusual situation).
> > 
> > I need to give this more thought, but I suppose the buffers should be
> > returned as usual when a failure happens. Maybe we need a way to
> > signal the error through the request too, and another to cancel
> > operation on the whole pipeline when an error occurs somewhere.
> 
> I think we definitely should have a way to signal the error through the
> request.

The release fop may return an error, too. I just find this a bit funny way
to tell there's been an error --- it's little and there's hardly a
possibility of making it more verbose than that.

The MC events would be another option. We'll need them anyway but that's
far, far ahead in the future (when all request related information could go
through MC).

> 
> > 
> >>
> >>> +
> >>> +Example for a Codec Device
> >>> +--------------------------
> >>> +
> >>> +Codecs are single-node V4L2 devices providing one OUTPUT queue (for user-space
> >>> +to provide input buffers) and one CAPTURE queue (to retrieve processed data).
> >>> +
> >>> +In this use-case, request API is used to associate specific controls to be
> >>> +applied by the driver before processing a buffer from the OUTPUT queue. When
> >>> +retrieving a buffer from a capture queue, user-space can then identify which
> >>> +request, if any, produced it by looking at the request_fd field of the dequeued
> >>> +v4l2_buffer.
> >>
> >> I'd start with the code to allocate a request.
> > 
> > Will fix that.
> > 
> >>
> >>> +Put into code, after obtaining a request, user-space can assign controls and one
> >>> +OUTPUT buffer to it:
> >>> +
> >>> +     struct v4l2_buf buf;
> >>> +     struct v4l2_ext_controls ctrls;
> >>> +     ...
> >>> +     ctrls.request_fd = req_fd;
> >>> +     ...
> >>> +     ioctl(codec_fd, VIDIOC_S_EXT_CTRLS, &ctrls);
> >>> +     ...
> >>> +     buf.type = V4L2_BUF_TYPE_VIDEO_OUTPUT;
> >>> +     buf.request_fd = req_fd;
> >>> +     ioctl(codec_fd, VIDIOC_QBUF, &buf);
> >>> +
> >>> +Note that request_fd shall not be specified for CAPTURE buffers.
> >>
> >> I know why, but the less experienced reader won't. So this needs some additional
> >> explanation.
> > 
> > Sure.
> > 
> >>
> >>> +
> >>> +Once the request is fully prepared, it can be submitted to the driver:
> >>> +
> >>> +     struct media_request_cmd cmd;
> >>> +
> >>> +     memset(&cmd, 0, sizeof(cmd));
> >>> +     cmd.cmd = MEDIA_REQ_CMD_SUBMIT;
> >>> +     cmd.fd = request_fd;
> >>> +     ioctl(media_fd, MEDIA_IOC_REQUEST_CMD, &cmd);
> >>> +
> >>> +User-space can then lookup the request_fd field of dequeued capture buffers to
> >>> +confirm which one has been produced by the request.
> >>> +
> >>> +     struct v4l2_buf buf;
> >>> +
> >>> +     memset(&buf, 0, sizeof(buf));
> >>> +     buf.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> >>> +     ioctl(codec_fd, VIDIOC_DQBUF, &buf);
> >>> +
> >>> +     if (buf.request_fd == request_fd)
> >>> +             ...
> >>> +
> >>
> >> I'm not sure this is correct. You set the request for the OUTPUT buffer, so when
> >> you dequeue the OUTPUT buffer, then the request_fd is set to the original request.
> >> But this is the CAPTURE buffer, and that has no requests at all.
> > 
> > This is not a mistake actually. The idea is to allow user-space to
> > know which request produced a given capture buffer. One output buffer
> > can in theory produce several or no capture buffers, and I think this
> > is the only reliable way to associate them with their producing
> > request.
> 
> You can't do this. There is no reason why the CAPTURE queue can't use requests
> as well (the queues are independent). Although for the codec use-case it is unlikely
> to happen, other use-cases may want requests for both OUTPUT and CAPTURE.

I'd say it must use requests. I doubt there are any valid use cases in
mixing request and non-request based operations on a device simultaneously;
having to support both at the same time adds complexity, and there should
be a reason for that.

> 
> Being able to tell which output buffer was the origin of a capture buffer is
> an unrelated problem. We never clearly defined how this should be done.
> 
> The reality is that none of the existing options (using the sequence number or
> the timestamp) are satisfactory IMHO. What you want is a new field for this.
> The problem with that is that struct v4l2_buffer is full, so a new API is needed.
> It's something I've been working on (see link below), but it's on hold until
> we have the fence and request API merged. Otherwise too many people are messing
> with the same structs...
> 
> https://git.linuxtv.org/hverkuil/media_tree.git/commit/?h=v4l2-buffer&id=a95549df06d9900f3559afdbb9da06bd4b22d1f3
> 
> > 
> >>
> >> BTW, you also want to show an example of getting the control once the request is
> >> completed (e.g. status information), probably with a polling example as well.
> > 
> > True.
> > 
> >>
> >>> +Example for a Simple Capture Device
> >>> +-----------------------------------
> >>> +
> >>> +With a simple capture device, requests can be used to specify controls to apply
> >>> +to a given CAPTURE buffer. The driver will apply these controls before producing
> >>> +the marked CAPTURE buffer.
> >>> +
> >>> +     struct v4l2_buf buf;
> >>> +     struct v4l2_ext_controls ctrls;
> >>> +     ...
> >>> +     ctrls.request_fd = req_fd;
> >>> +     ...
> >>> +     ioctl(camera_fd, VIDIOC_S_EXT_CTRLS, &ctrls);
> >>> +     ...
> >>> +     buf.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> >>> +     buf.request_fd = req_fd;
> >>> +     ioctl(camera_fd, VIDIOC_QBUF, &buf);
> >>> +
> >>> +Once the request is fully prepared, it can be submitted to the driver:
> >>> +
> >>> +     struct media_request_cmd cmd;
> >>> +
> >>> +     memset(&cmd, 0, sizeof(cmd));
> >>> +     cmd.cmd = MEDIA_REQ_CMD_SUBMIT;
> >>> +     cmd.fd = request_fd;
> >>> +     ioctl(media_fd, MEDIA_IOC_REQUEST_CMD, &cmd);
> >>> +
> >>> +User-space can then lookup the request_fd field of dequeued capture buffers to
> >>> +confirm which one has been produced by the request.
> >>> +
> >>> +     struct v4l2_buf buf;
> >>> +
> >>> +     memset(&buf, 0, sizeof(buf));
> >>> +     buf.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> >>> +     ioctl(camera_fd, VIDIOC_DQBUF, &buf);
> >>> +
> >>> +     if (buf.request_fd == request_fd)
> >>> +             ...
> >>> diff --git a/Documentation/media/uapi/v4l/vidioc-qbuf.rst b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
> >>> index 9e448a4aa3aa..0cd596bd4cde 100644
> >>> --- a/Documentation/media/uapi/v4l/vidioc-qbuf.rst
> >>> +++ b/Documentation/media/uapi/v4l/vidioc-qbuf.rst
> >>> @@ -98,6 +98,12 @@ dequeued, until the :ref:`VIDIOC_STREAMOFF <VIDIOC_STREAMON>` or
> >>>  :ref:`VIDIOC_REQBUFS` ioctl is called, or until the
> >>>  device is closed.
> >>>
> >>> +For all buffer types, the ``request_fd`` field can be used when enqueing
> >>> +to specify the file descriptor of a request, if requests are in use.
> >>> +Setting it means that the buffer will not be passed to the driver until
> >>> +the request itself is submitted. Also, the driver will apply any setting
> >>> +associated with the request before processing the buffer.
> >>> +
> >>>  Applications call the ``VIDIOC_DQBUF`` ioctl to dequeue a filled
> >>>  (capturing) or displayed (output) buffer from the driver's outgoing
> >>>  queue. They just set the ``type``, ``memory`` and ``reserved`` fields of
> >>> @@ -115,6 +121,21 @@ queue. When the ``O_NONBLOCK`` flag was given to the
> >>>  :ref:`open() <func-open>` function, ``VIDIOC_DQBUF`` returns
> >>>  immediately with an ``EAGAIN`` error code when no buffer is available.
> >>>
> >>> +If a dequeued buffer was produced as the result of a request, then the
> >>> +``request_fd`` field of :c:type:`v4l2_buffer` will be set to the file
> >>> +descriptor of the request, regardless of whether the buffer was initially
> >>> +queued with a request associated to it or not.
> >>> +
> >>> +RFC note: a request can be referenced by several FDs, especially if the
> >>> +request is shared between different processes. Also a FD can be closed
> >>> +after a request is submitted. In that case the FD does not make much sense.
> >>> +Maybe it would be better to define request fd as
> >>> +
> >>> +    union { u32 request_fd; u32 request_id; }
> >>> +
> >>> +and use request_id when communicating a request to userspace so they can be
> >>> +unambiguously referenced?
> >>
> >> I don't think this is a problem. At least we never had this issue with e.g.
> >> dmabuf either. The request_fd must be a valid request fd for the process that
> >> calls the ioctl. If not the ioctl will return an error. If process A has a
> >> request fd 10 and process B calls the ioctl with request_fd set to 10 but it
> >> didn't actually allocate the request or receive the fd from process A (there
> >> is a mechanism for that, although I've forgotten the details).
> > 
> > For user-to-kernel communication this is true. What I had in mind was
> > the other way around: kernel trying to specify an already-allocated
> > request to userspace, like the above example of the capture buffer
> > that is marked with the request that produced it.
> > 
> > At that time, the FD of the request may have been closed, or may have
> > been passed to another process (which references the request using a
> > different ID). That's what FDs are not reliable here.
> > 
> 
> The capture example is wrong in any case as I explained above.
> 
> But I really don't think this is an issue if the request fds are properly
> refcounted. It's never been an issue with dma-buf, and I think that's the
> example that you should follow.
> 
> Yes, if a request_fd is returned that is closed, then trying to use it
> will fail. But that's an application bug.
> 
> Regards,
> 
> 	Hans

-- 
Sakari Ailus
sakari.ailus@linux.intel.com