[RFC v2 00/10] Preparing the request API

[RFC v2 00/10] Preparing the request API

[Sakari Ailus]

Hi folks,

This preliminary RFC patchset prepares for the request API. What's new
here is support for binding arbitrary configuration or resources to
requests.

There are a few new concepts here:

Class --- a type of configuration or resource a driver (or e.g. the V4L2
framework) can attach to a resource. E.g. a video buffer queue would be a
class.

Object --- an instance of the class. This may be either configuration (in
which case the setting will stay until changed, e.g. V4L2 format on a
video node) or a resource (such as a video buffer).

Reference --- a reference to an object. If a configuration is not changed
in a request, instead of allocating a new object, a reference to an
existing object is used. This saves time and memory.

I expect Laurent to comment on aligning the concept names between the
request API and DRM. As far as I understand, the respective DRM names for
"class" and "object" used in this set would be "object" and "state".

The drivers will need to interact with the requests in three ways:

- Allocate new configurations or resources. Drivers are free to store
  their own data into request objects as well. These callbacks are
  specific to classes.

- Validate and queue callbacks. These callbacks are used to try requests
  (validate only) as well as queue them (validate and queue). These
  callbacks are media device wide, at least for now.

The lifetime of the objects related to requests is based on refcounting
both requests and request objects. This fits well for existing use cases
whether or not based on refcounting; what still needs most of the
attention is likely that the number of gets and puts matches once the
object is no longer needed.

Configuration can be bound to the request the usual way (V4L2 IOCTLs with
the request_fd field set to the request). Once queued, request completion
is signalled through polling the request file handle (POLLPRI).

I'm posting this as an RFC because it's not complete yet. The code
compiles but no testing has been done yet.

Todo list:

- Testing! (And fixing the bugs.)

- Request support in a few drivers as well as the control framework.

- Request support for V4L2 formats?

In the future, support for changing e.g. Media controller link state or
V4L2 sub-device formats will need to be added. Those should receive more
attention when the core is in a good shape and the more simple use cases
are already functional.

Comments and questions are welcome.

since v1:

- Provide an iterator helper for request objects in a request.

- Remove the request lists in the media device (they were not used)

- Move request queing to request fd and add reinit (Alexandre's patchset);
  this roughly corresponds to Request API RFC v2 from Hans.
  (MEDIA_IOC_REQUEST_ALLOC argument is a struct pointer instead of an
  __s32 pointer.)

- Provide a way to unbind request objects from an unqueued request
  (reinit, closing request fd).

- v4l2-mem2mem + vivid implementation without control support.

- More states for requests. In order to take a spinlock (or a mutex) for
  an extended period of time, add a "QUEUEING" and "REINIT" states.

- Move non-IOCTL code to media-request.c, remove extra filp argument that
  was added in v1.

- SPDX license header, other small changes.

Open questions:

- How to tell at complete time whether a request failed? Return error code
  on release? What's the behaviour with reinit then --- fail on error? Add
  another IOCTL to ask for completion status?


Alexandre Courbot (1):
  videodev2.h: add request_fd field to v4l2_ext_controls

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

Laurent Pinchart (1):
  media: Add request API

Sakari Ailus (7):
  media: Support variable size IOCTL arguments
  staging: media: atomisp: Remove v4l2_buffer.reserved2 field hack
  vb2: Add support for requests
  v4l: m2m: Simplify exiting the function in v4l2_m2m_try_schedule
  v4l: m2m: Support requests with video buffers
  vim2m: Register V4L2 video device after V4L2 mem2mem init
  vim2m: Request support

 drivers/media/Makefile                             |   3 +-
 drivers/media/common/videobuf2/videobuf2-core.c    |  43 +-
 drivers/media/common/videobuf2/videobuf2-v4l2.c    |  40 +-
 drivers/media/media-device.c                       |  80 ++-
 drivers/media/media-request.c                      | 650 +++++++++++++++++++++
 drivers/media/platform/vim2m.c                     |  76 ++-
 drivers/media/usb/cpia2/cpia2_v4l.c                |   2 +-
 drivers/media/v4l2-core/v4l2-compat-ioctl32.c      |  16 +-
 drivers/media/v4l2-core/v4l2-ioctl.c               |   6 +-
 drivers/media/v4l2-core/v4l2-mem2mem.c             | 131 ++++-
 .../media/atomisp/pci/atomisp2/atomisp_ioctl.c     |  17 +-
 include/media/media-device.h                       |  19 +-
 include/media/media-request.h                      | 301 ++++++++++
 include/media/v4l2-mem2mem.h                       |  28 +
 include/media/videobuf2-core.h                     |  19 +
 include/media/videobuf2-v4l2.h                     |  28 +
 include/uapi/linux/media.h                         |   8 +
 include/uapi/linux/videodev2.h                     |   6 +-
 18 files changed, 1408 insertions(+), 65 deletions(-)
 create mode 100644 drivers/media/media-request.c
 create mode 100644 include/media/media-request.h

-- 
2.7.4

[RFC v2 01/10] media: Support variable size IOCTL arguments

[Sakari Ailus]

Maintain a list of supported IOCTL argument sizes and allow only those in
the list.

As an additional bonus, IOCTL handlers will be able to check whether the
caller actually set (using the argument size) the field vs. assigning it
to zero. Separate macro can be provided for that.

This will be easier for applications as well since there is no longer the
problem of setting the reserved fields zero, or at least it is a lesser
problem.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/media-device.c | 65 ++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 59 insertions(+), 6 deletions(-)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 35e81f7..da63da1 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -387,22 +387,36 @@ static long copy_arg_to_user(void __user *uarg, void *karg, unsigned int cmd)
 /* Do acquire the graph mutex */
 #define MEDIA_IOC_FL_GRAPH_MUTEX	BIT(0)
 
-#define MEDIA_IOC_ARG(__cmd, func, fl, from_user, to_user)		\
+#define MEDIA_IOC_SZ_ARG(__cmd, func, fl, alt_sz, from_user, to_user)	\
 	[_IOC_NR(MEDIA_IOC_##__cmd)] = {				\
 		.cmd = MEDIA_IOC_##__cmd,				\
 		.fn = (long (*)(struct media_device *, void *))func,	\
 		.flags = fl,						\
+		.alt_arg_sizes = alt_sz,				\
 		.arg_from_user = from_user,				\
 		.arg_to_user = to_user,					\
 	}
 
-#define MEDIA_IOC(__cmd, func, fl)					\
-	MEDIA_IOC_ARG(__cmd, func, fl, copy_arg_from_user, copy_arg_to_user)
+#define MEDIA_IOC_ARG(__cmd, func, fl, from_user, to_user)		\
+	MEDIA_IOC_SZ_ARG(__cmd, func, fl, NULL, from_user, to_user)
+
+#define MEDIA_IOC_SZ(__cmd, func, fl, alt_sz)			\
+	MEDIA_IOC_SZ_ARG(__cmd, func, fl, alt_sz,		\
+			 copy_arg_from_user, copy_arg_to_user)
+
+#define MEDIA_IOC(__cmd, func, fl)				\
+	MEDIA_IOC_ARG(__cmd, func, fl,				\
+		      copy_arg_from_user, copy_arg_to_user)
 
 /* the table is indexed by _IOC_NR(cmd) */
 struct media_ioctl_info {
 	unsigned int cmd;
 	unsigned short flags;
+	/*
+	 * Sizes of the alternative arguments. If there are none, this
+	 * pointer is NULL.
+	 */
+	const unsigned short *alt_arg_sizes;
 	long (*fn)(struct media_device *dev, void *arg);
 	long (*arg_from_user)(void *karg, void __user *uarg, unsigned int cmd);
 	long (*arg_to_user)(void __user *uarg, void *karg, unsigned int cmd);
@@ -416,6 +430,42 @@ static const struct media_ioctl_info ioctl_info[] = {
 	MEDIA_IOC(G_TOPOLOGY, media_device_get_topology, MEDIA_IOC_FL_GRAPH_MUTEX),
 };
 
+#define MASK_IOC_SIZE(cmd) \
+	((cmd) & ~(_IOC_SIZEMASK << _IOC_SIZESHIFT))
+
+static inline long is_valid_ioctl(unsigned int cmd)
+{
+	const struct media_ioctl_info *info = ioctl_info;
+	const unsigned short *alt_arg_sizes;
+
+	if (_IOC_NR(cmd) >= ARRAY_SIZE(ioctl_info))
+		return -ENOIOCTLCMD;
+
+	info += _IOC_NR(cmd);
+
+	if (info->cmd == cmd)
+		return 0;
+
+	/*
+	 * Verify that the size-dependent patch of the IOCTL command
+	 * matches and that the size does not exceed the principal
+	 * argument size.
+	 */
+	if (MASK_IOC_SIZE(info->cmd) != MASK_IOC_SIZE(cmd)
+	    || _IOC_SIZE(info->cmd) < _IOC_SIZE(cmd))
+		return -ENOIOCTLCMD;
+
+	alt_arg_sizes = info->alt_arg_sizes;
+	if (!alt_arg_sizes)
+		return -ENOIOCTLCMD;
+
+	for (; *alt_arg_sizes; alt_arg_sizes++)
+		if (_IOC_SIZE(cmd) == *alt_arg_sizes)
+			return 0;
+
+	return -ENOIOCTLCMD;
+}
+
 static long media_device_ioctl(struct file *filp, unsigned int cmd,
 			       unsigned long __arg)
 {
@@ -426,9 +476,9 @@ static long media_device_ioctl(struct file *filp, unsigned int cmd,
 	char __karg[256], *karg = __karg;
 	long ret;
 
-	if (_IOC_NR(cmd) >= ARRAY_SIZE(ioctl_info)
-	    || ioctl_info[_IOC_NR(cmd)].cmd != cmd)
-		return -ENOIOCTLCMD;
+	ret = is_valid_ioctl(cmd);
+	if (ret)
+		return ret;
 
 	info = &ioctl_info[_IOC_NR(cmd)];
 
@@ -444,6 +494,9 @@ static long media_device_ioctl(struct file *filp, unsigned int cmd,
 			goto out_free;
 	}
 
+	/* Set the rest of the argument struct to zero */
+	memset(karg + _IOC_SIZE(cmd), 0, _IOC_SIZE(info->cmd) - _IOC_SIZE(cmd));
+
 	if (info->flags & MEDIA_IOC_FL_GRAPH_MUTEX)
 		mutex_lock(&dev->graph_mutex);
 
-- 
2.7.4

[RFC v2 02/10] media: Add request API

[Sakari Ailus]

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

The request API allows bundling media device parameters with request
objects and queueing them to be executed atomically.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Signed-off-by: Alexandre Courbot <acourbot@chromium.org>
---
 drivers/media/Makefile        |   3 +-
 drivers/media/media-device.c  |  15 +
 drivers/media/media-request.c | 650 ++++++++++++++++++++++++++++++++++++++++++
 include/media/media-device.h  |  19 +-
 include/media/media-request.h | 301 +++++++++++++++++++
 include/uapi/linux/media.h    |   8 +
 6 files changed, 994 insertions(+), 2 deletions(-)
 create mode 100644 drivers/media/media-request.c
 create mode 100644 include/media/media-request.h

diff --git a/drivers/media/Makefile b/drivers/media/Makefile
index 594b462..985d35e 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
 
 #
 # 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 da63da1..cc579ce 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -32,6 +32,7 @@
 #include <media/media-device.h>
 #include <media/media-devnode.h>
 #include <media/media-entity.h>
+#include <media/media-request.h>
 
 #ifdef CONFIG_MEDIA_CONTROLLER
 
@@ -366,6 +367,16 @@ static long media_device_get_topology(struct media_device *mdev,
 	return ret;
 }
 
+static long media_device_request_alloc(struct media_device *mdev,
+				       struct media_request_new *new)
+{
+	if (!mdev->ops || !mdev->ops->req_alloc || !mdev->ops->req_free ||
+	    !mdev->ops->req_queue)
+		return -ENOTTY;
+
+	return media_request_alloc(mdev, new);
+}
+
 static long copy_arg_from_user(void *karg, void __user *uarg, unsigned int cmd)
 {
 	/* All media IOCTLs are _IOWR() */
@@ -428,6 +439,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_ALLOC, media_device_request_alloc, 0),
 };
 
 #define MASK_IOC_SIZE(cmd) \
@@ -739,6 +751,9 @@ void media_device_init(struct media_device *mdev)
 	INIT_LIST_HEAD(&mdev->pads);
 	INIT_LIST_HEAD(&mdev->links);
 	INIT_LIST_HEAD(&mdev->entity_notify);
+	INIT_LIST_HEAD(&mdev->classes);
+	spin_lock_init(&mdev->req_lock);
+
 	mutex_init(&mdev->graph_mutex);
 	ida_init(&mdev->entity_internal_idx);
 
diff --git a/drivers/media/media-request.c b/drivers/media/media-request.c
new file mode 100644
index 0000000..af108a7
--- /dev/null
+++ b/drivers/media/media-request.c
@@ -0,0 +1,650 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Media device request objects
+ *
+ * Copyright (C) 2018 Intel Corporation
+ * Copyright (C) 2018, The Chromium OS Authors.  All rights reserved.
+ *
+ * Author: Sakari Ailus <sakari.ailus@linux.intel.com>
+ */
+
+#include <linux/anon_inodes.h>
+#include <linux/file.h>
+#include <linux/mm.h>
+#include <linux/string.h>
+
+#include <media/media-device.h>
+#include <media/media-request.h>
+
+static const char *__request_state[] = {
+	"IDLE",
+	"QUEUEING",
+	"QUEUED",
+	"COMPLETE",
+	"REINIT",
+};
+
+const char *
+media_request_state_str(enum media_request_state state)
+{
+	if (state < ARRAY_SIZE(__request_state))
+		return __request_state[state];
+
+	return "UNKNOWN";
+}
+
+static void media_request_clean(struct media_request *req)
+{
+	struct media_request_ref *ref, *ref_safe;
+
+	list_for_each_entry_safe(ref, ref_safe, &req->obj_refs, req_list) {
+		media_request_ref_unbind(ref);
+		kfree(ref);
+	}
+}
+
+void media_request_release(struct kref *kref)
+{
+	struct media_request *req =
+		container_of(kref, struct media_request, kref);
+	struct media_device *mdev = req->mdev;
+
+	dev_dbg(mdev->dev, "request: release %s\n", req->debug_str);
+
+	media_request_clean(req);
+
+	mdev->ops->req_free(req);
+}
+EXPORT_SYMBOL_GPL(media_request_release);
+
+static unsigned int media_request_poll(struct file *filp,
+				       struct poll_table_struct *wait)
+{
+	struct media_request *req = filp->private_data;
+	struct media_device *mdev = req->mdev;
+	unsigned int poll_events = poll_requested_events(wait);
+	int ret = 0;
+
+	if (poll_events & (POLLIN | POLLOUT))
+		return POLLERR;
+
+	if (poll_events & POLLPRI) {
+		unsigned long flags;
+		bool complete;
+
+		spin_lock_irqsave(&mdev->req_lock, flags);
+		complete = req->state == MEDIA_REQUEST_STATE_COMPLETE;
+		spin_unlock_irqrestore(&mdev->req_lock, flags);
+
+		if (complete)
+			poll_wait(filp, &req->poll_wait, wait);
+		else
+			ret |= POLLPRI;
+	}
+
+	return ret;
+}
+
+static long media_request_ioctl_queue(struct media_request *req)
+{
+	struct media_device *mdev = req->mdev;
+	unsigned long flags;
+	int ret = 0;
+
+	dev_dbg(mdev->dev, "request: queue %s\n", req->debug_str);
+
+	/*
+	 * Ensure the request that is validated will be the one that gets queued
+	 * next by serialising the queueing process.
+	 */
+	mutex_lock(&mdev->req_queue_mutex);
+
+	spin_lock_irqsave(&mdev->req_lock, flags);
+	if (req->state != MEDIA_REQUEST_STATE_IDLE) {
+		ret = -EINVAL;
+		dev_dbg(mdev->dev,
+			"request: unable to queue %s, request in state %s\n",
+			req->debug_str, media_request_state_str(req->state));
+	} else {
+		req->state = MEDIA_REQUEST_STATE_QUEUEING;
+		media_request_sticky_to_old(req);
+	}
+	spin_unlock_irqrestore(&mdev->req_lock, flags);
+
+	if (ret)
+		goto err_unlock;
+
+	/*
+	 * Obtain a reference to the request, release once complete (or there's
+	 * an error).
+	 */
+	media_request_get(req);
+	spin_lock_irqsave(&mdev->req_lock, flags);
+	req->state = MEDIA_REQUEST_STATE_QUEUED;
+	spin_unlock_irqrestore(&mdev->req_lock, flags);
+
+	ret = mdev->ops->req_queue(req);
+	if (ret) {
+		dev_dbg(mdev->dev, "request: can't queue %s (%d)\n",
+			req->debug_str, ret);
+		goto err_put;
+	}
+
+	spin_lock_irqsave(&mdev->req_lock, flags);
+	media_request_new_to_sticky(req);
+	spin_unlock_irqrestore(&mdev->req_lock, flags);
+
+	mutex_unlock(&mdev->req_queue_mutex);
+
+	return 0;
+
+err_put:
+	media_request_put(req);
+
+	spin_lock_irqsave(&mdev->req_lock, flags);
+	media_request_detach_old(req);
+	spin_unlock_irqrestore(&mdev->req_lock, flags);
+
+err_unlock:
+	spin_lock_irqsave(&mdev->req_lock, flags);
+	req->state = MEDIA_REQUEST_STATE_IDLE;
+	spin_unlock_irqrestore(&mdev->req_lock, flags);
+	mutex_unlock(&mdev->req_queue_mutex);
+
+	return ret;
+}
+
+static long media_request_ioctl_reinit(struct media_request *req)
+{
+	struct media_device *mdev = req->mdev;
+	unsigned long flags;
+
+	spin_lock_irqsave(&mdev->req_lock, flags);
+	if (req->state != MEDIA_REQUEST_STATE_IDLE &&
+	    req->state != MEDIA_REQUEST_STATE_COMPLETE) {
+		dev_dbg(mdev->dev,
+			"request: %s in idle or complete state, cannot reinit\n",
+			req->debug_str);
+		spin_unlock_irqrestore(&mdev->req_lock, flags);
+		return -EINVAL;
+	}
+
+	req->state = MEDIA_REQUEST_STATE_REINIT;
+	spin_unlock_irqrestore(&mdev->req_lock, flags);
+
+	media_request_clean(req);
+
+	spin_lock_irqsave(&mdev->req_lock, flags);
+	req->state = MEDIA_REQUEST_STATE_IDLE;
+	spin_unlock_irqrestore(&mdev->req_lock, flags);
+
+	return 0;
+}
+
+#define MEDIA_REQUEST_IOC(__cmd, func)			\
+	[_IOC_NR(MEDIA_REQUEST_IOC_##__cmd) - 0x80] = {	\
+		.cmd = MEDIA_REQUEST_IOC_##__cmd,	\
+		.fn = func,				\
+	}
+
+struct media_request_ioctl_info {
+	unsigned int cmd;
+	long (*fn)(struct media_request *req);
+};
+
+static const struct media_request_ioctl_info ioctl_info[] = {
+	MEDIA_REQUEST_IOC(QUEUE, media_request_ioctl_queue),
+	MEDIA_REQUEST_IOC(REINIT, media_request_ioctl_reinit),
+};
+
+static long media_request_ioctl(struct file *filp, unsigned int cmd,
+				unsigned long __arg)
+{
+	struct media_request *req = filp->private_data;
+	const struct media_request_ioctl_info *info;
+
+	if (_IOC_NR(cmd) < 0x80 ||
+            _IOC_NR(cmd) >= 0x80 + ARRAY_SIZE(ioctl_info) ||
+            ioctl_info[_IOC_NR(cmd) - 0x80].cmd != cmd)
+		return -ENOIOCTLCMD;
+
+	info = &ioctl_info[_IOC_NR(cmd) - 0x80];
+
+	return info->fn(req);
+}
+
+static int media_request_close(struct inode *inode, struct file *filp)
+{
+	struct media_request *req = filp->private_data;
+
+	media_request_put(req);
+
+	return 0;
+}
+static const struct file_operations request_fops = {
+	.owner = THIS_MODULE,
+	.poll = media_request_poll,
+	.unlocked_ioctl = media_request_ioctl,
+	.release = media_request_close,
+};
+
+/**
+ * media_request_find - Find a request based on the file descriptor
+ * @mdev: The media device
+ * @request: The request file handle
+ *
+ * Find and return the request associated with the given file descriptor, or
+ * an error if no such request exists.
+ *
+ * When the function returns a request it increases its reference count. The
+ * caller is responsible for releasing the reference by calling
+ * media_request_put() on the request.
+ */
+struct media_request *
+media_request_find(struct media_device *mdev, int request)
+{
+	struct file *filp;
+	struct media_request *req;
+
+	filp = fget(request);
+	if (!filp)
+		return ERR_PTR(-ENOENT);
+
+	if (filp->f_op != &request_fops)
+		goto err_fput;
+	req = filp->private_data;
+	media_request_get(req);
+
+	if (req->mdev != mdev)
+		goto err_kref_put;
+
+	fput(filp);
+
+	return req;
+
+err_kref_put:
+	media_request_put(req);
+
+err_fput:
+	fput(filp);
+
+	return ERR_PTR(-EBADF);
+}
+EXPORT_SYMBOL_GPL(media_request_find);
+
+int media_request_alloc(struct media_device *mdev,
+			struct media_request_new *new)
+{
+	struct media_request *req;
+	struct file *filp;
+#ifdef CONFIG_DYNAMIC_DEBUG
+	char comm[TASK_COMM_LEN];
+#endif
+	int fd;
+	int ret;
+
+	fd = get_unused_fd_flags(O_CLOEXEC);
+	if (fd < 0)
+		return fd;
+
+	filp = anon_inode_getfile("request", &request_fops, NULL, O_CLOEXEC);
+	if (IS_ERR(filp)) {
+		ret = PTR_ERR(filp);
+		goto err_put_fd;
+	}
+
+	req = mdev->ops->req_alloc(mdev);
+	if (!req) {
+		ret = -ENOMEM;
+		goto err_fput;
+	}
+
+	filp->private_data = req;
+	req->mdev = mdev;
+	req->state = MEDIA_REQUEST_STATE_IDLE;
+	kref_init(&req->kref);
+	INIT_LIST_HEAD(&req->obj_refs);
+
+	new->fd = fd;
+
+#ifdef CONFIG_DYNAMIC_DEBUG
+	get_task_comm(comm, current);
+	snprintf(req->debug_str, sizeof(req->debug_str), "%s:%d",
+		 comm, fd);
+#endif
+
+	dev_dbg(mdev->dev, "request: allocated %s\n", req->debug_str);
+	fd_install(fd, filp);
+
+	return 0;
+
+err_fput:
+	fput(filp);
+
+err_put_fd:
+	put_unused_fd(fd);
+
+	return ret;
+}
+
+void media_request_complete(struct media_device *mdev,
+			    struct media_request *req)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&mdev->req_lock, flags);
+
+	if (!media_request_is_complete(req)) {
+		spin_unlock_irqrestore(&mdev->req_lock, flags);
+		dev_dbg(mdev->dev, "request: %s is not complete yet\n",
+			req->debug_str);
+		return;
+	}
+
+	if (req->state == MEDIA_REQUEST_STATE_IDLE) {
+		dev_dbg(mdev->dev,
+			"request: not completing an idle request %s\n",
+			req->debug_str);
+		spin_unlock_irqrestore(&mdev->req_lock, flags);
+		return;
+	}
+
+	if (WARN_ON(req->state != MEDIA_REQUEST_STATE_QUEUED)) {
+		dev_dbg(mdev->dev, "request: can't delete %s, state %s\n",
+			req->debug_str,
+			media_request_state_str(req->state));
+		spin_unlock_irqrestore(&mdev->req_lock, flags);
+		return;
+	}
+
+	req->state = MEDIA_REQUEST_STATE_COMPLETE;
+
+	spin_unlock_irqrestore(&mdev->req_lock, flags);
+
+	wake_up_all(&req->poll_wait);
+
+	media_request_put(req);
+}
+EXPORT_SYMBOL_GPL(media_request_complete);
+
+static void media_request_object_release(struct kref *kref)
+{
+	struct media_request_object *obj =
+		container_of(kref, struct media_request_object, kref);
+	struct media_device *mdev = obj->class->mdev;
+	unsigned long flags;
+
+	spin_lock_irqsave(&mdev->req_lock, flags);
+	list_del(&obj->object_list);
+	spin_unlock_irqrestore(&mdev->req_lock, flags);
+
+	obj->class->release(obj);
+}
+
+void media_request_object_put(struct media_request_object *obj)
+{
+	if (obj)
+		kref_put(&obj->kref, media_request_object_release);
+}
+EXPORT_SYMBOL_GPL(media_request_object_put);
+
+static struct media_request_object *
+media_request_object_get(struct media_request_object *obj)
+{
+	kref_get(&obj->kref);
+
+	return obj;
+}
+
+void
+media_request_class_register(struct media_device *mdev,
+			     struct media_request_class *class,
+			     void (*unbind)(struct media_request_ref *ref),
+			     void (*release)(struct media_request_object *object),
+			     bool completeable)
+{
+	unsigned long flags;
+
+	INIT_LIST_HEAD(&class->objects);
+	class->unbind = unbind;
+	class->release = release;
+	class->completeable = completeable;
+	class->mdev = mdev;
+
+	spin_lock_irqsave(&mdev->req_lock, flags);
+	list_add(&class->mdev_list, &mdev->classes);
+	spin_unlock_irqrestore(&mdev->req_lock, flags);
+}
+EXPORT_SYMBOL_GPL(media_request_class_register);
+
+void media_request_class_unregister(struct media_request_class *class)
+{
+	unsigned long flags;
+
+	if (!class || !class->mdev)
+		return;
+
+	spin_lock_irqsave(&class->mdev->req_lock, flags);
+	list_del(&class->mdev_list);
+	spin_unlock_irqrestore(&class->mdev->req_lock, flags);
+	media_request_object_put(class->sticky);
+
+	class->mdev = NULL;
+}
+EXPORT_SYMBOL_GPL(media_request_class_unregister);
+
+void media_request_class_set_sticky(struct media_request_class *class,
+				    struct media_request_object *sticky)
+{
+	if (WARN_ON(class->sticky))
+		return;
+
+	class->sticky = media_request_object_get(sticky);
+}
+
+void media_request_object_init(struct media_request_class *class,
+			       struct media_request_object *obj)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&class->mdev->req_lock, flags);
+	list_add(&obj->object_list, &class->objects);
+	spin_unlock_irqrestore(&class->mdev->req_lock, flags);
+	obj->class = class;
+	kref_init(&obj->kref);
+}
+EXPORT_SYMBOL_GPL(media_request_object_init);
+
+void media_request_ref_put(struct media_request_ref *ref)
+{
+	if (!ref)
+		return;
+
+	media_request_object_put(ref->new);
+	media_request_object_put(ref->old);
+}
+EXPORT_SYMBOL_GPL(media_request_ref_put);
+
+static struct media_request_ref *
+media_request_ref_find(struct media_request *req,
+		       struct media_request_class *class)
+{
+	struct media_request_ref *ref;
+
+	lockdep_assert_held(&class->mdev->req_lock);
+
+	list_for_each_entry(ref, &req->obj_refs, req_list)
+		if (ref->new->class == class)
+			return ref;
+
+	return NULL;
+}
+
+static int __media_request_object_bind(struct media_request *req,
+				       struct media_request_ref *ref,
+				       struct media_request_object *obj)
+{
+	if (req->state != MEDIA_REQUEST_STATE_IDLE) {
+		dev_dbg(req->mdev->dev, "request: %s not idle but %s\n",
+			req->debug_str,
+			media_request_state_str(req->state));
+		return -EBUSY;
+	}
+
+	media_request_object_put(ref->new);
+	ref->new = media_request_object_get(obj);
+
+	return 0;
+}
+
+struct media_request_ref *
+media_request_object_bind(struct media_request *req,
+			  struct media_request_object *obj)
+{
+	struct media_request_class *class = obj->class;
+	struct media_device *mdev = class->mdev;
+	struct media_request_ref *ref, *ref_new;
+	unsigned long flags;
+	int ret;
+
+	ref_new = kzalloc(sizeof(*ref_new), GFP_KERNEL);
+
+	spin_lock_irqsave(&mdev->req_lock, flags);
+
+	ref = media_request_ref_find(req, obj->class);
+	if (!ref) {
+		if (!ref_new) {
+			ret = -ENOMEM;
+			goto err;
+		}
+
+		ref = ref_new;
+	}
+
+	ret = __media_request_object_bind(req, ref, obj);
+	if (ret)
+		goto err;
+
+	/* Newly created reference? */
+	if (ref == ref_new) {
+		list_add(&ref->req_list, &req->obj_refs);
+		if (class->completeable)
+			req->incomplete++;
+		ref->req = req;
+	}
+
+	spin_unlock_irqrestore(&req->mdev->req_lock, flags);
+
+	/* Release unused reference */
+	if (ref != ref_new)
+		kfree(ref_new);
+
+	return ref;
+
+err:
+	spin_unlock_irqrestore(&req->mdev->req_lock, flags);
+
+	kfree(ref_new);
+
+	return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(media_request_object_bind);
+
+void media_request_ref_unbind(struct media_request_ref *ref)
+{
+	struct media_request_object *obj = ref->new;
+	struct media_device *mdev = obj->class->mdev;
+	unsigned long flags;
+
+	if (!obj->class->completeable)
+		return;
+
+	spin_lock_irqsave(&mdev->req_lock, flags);
+	if (!ref->complete) {
+		ref->req->incomplete--;
+		WARN_ON(ref->req->incomplete < 0);
+	}
+	list_del(&ref->req_list);
+	spin_unlock_irqrestore(&mdev->req_lock, flags);
+	if (ref->new->class->unbind)
+		ref->new->class->unbind(ref);
+	media_request_ref_put(ref);
+}
+EXPORT_SYMBOL_GPL(media_request_ref_unbind);
+
+/* Tip of the queue state is the state previous to the request. */
+void media_request_sticky_to_old(struct media_request *req)
+{
+	struct media_request_ref *ref;
+
+	lockdep_assert_held(&req->mdev->req_lock);
+
+	list_for_each_entry(ref, &req->obj_refs, req_list) {
+		struct media_request_class *class = ref->new->class;
+
+		if (!class->sticky)
+			continue;
+
+		ref->old = media_request_object_get(class->sticky);
+	}
+}
+EXPORT_SYMBOL_GPL(media_request_sticky_to_old);
+
+void media_request_new_to_sticky(struct media_request *req)
+{
+	struct media_request_ref *ref;
+
+	lockdep_assert_held(&req->mdev->req_lock);
+
+	list_for_each_entry(ref, &req->obj_refs, req_list) {
+		struct media_request_class *class = ref->new->class;
+
+		if (!class->sticky)
+			continue;
+
+		media_request_object_put(class->sticky);
+		class->sticky = media_request_object_get(ref->new);
+	}
+}
+EXPORT_SYMBOL_GPL(media_request_new_to_sticky);
+
+void media_request_detach_old(struct media_request *req)
+{
+	struct media_request_ref *ref;
+
+	lockdep_assert_held(&req->mdev->req_lock);
+
+	list_for_each_entry(ref, &req->obj_refs, req_list) {
+		media_request_object_put(ref->old);
+		ref->old = NULL;
+	}
+}
+EXPORT_SYMBOL_GPL(media_request_detach_old);
+
+bool media_request_is_complete(struct media_request *req)
+{
+	lockdep_assert_held(&req->mdev->req_lock);
+
+	return !req->incomplete;
+}
+EXPORT_SYMBOL_GPL(media_request_is_complete);
+
+void media_request_ref_complete(struct media_request_ref *ref)
+{
+	struct media_request_object *obj = ref->new;
+	struct media_device *mdev = obj->class->mdev;
+	unsigned long flags;
+
+	if (WARN_ON(!obj->class->completeable))
+		return;
+
+	spin_lock_irqsave(&mdev->req_lock, flags);
+	if (!ref->complete) {
+		ref->complete = true;
+		ref->req->incomplete--;
+		WARN_ON(ref->req->incomplete < 0);
+	}
+	spin_unlock_irqrestore(&mdev->req_lock, flags);
+	media_request_ref_put(ref);
+}
+EXPORT_SYMBOL_GPL(media_request_ref_complete);
diff --git a/include/media/media-device.h b/include/media/media-device.h
index bcc6ec4..704b4b5 100644
--- a/include/media/media-device.h
+++ b/include/media/media-device.h
@@ -19,14 +19,17 @@
 #ifndef _MEDIA_DEVICE_H
 #define _MEDIA_DEVICE_H
 
+#include <linux/anon_inodes.h>
+#include <linux/kref.h>
 #include <linux/list.h>
 #include <linux/mutex.h>
 
 #include <media/media-devnode.h>
 #include <media/media-entity.h>
 
-struct ida;
 struct device;
+struct ida;
+struct media_device;
 
 /**
  * struct media_entity_notify - Media Entity Notify
@@ -50,10 +53,16 @@ struct media_entity_notify {
  * struct media_device_ops - Media device operations
  * @link_notify: Link state change notification callback. This callback is
  *		 called with the graph_mutex held.
+ * @req_alloc: Allocate a request
+ * @req_free: Free a request
+ * @req_queue: Queue a request
  */
 struct media_device_ops {
 	int (*link_notify)(struct media_link *link, u32 flags,
 			   unsigned int notification);
+	struct media_request *(*req_alloc)(struct media_device *mdev);
+	void (*req_free)(struct media_request *req);
+	int (*req_queue)(struct media_request *req);
 };
 
 /**
@@ -88,6 +97,10 @@ struct media_device_ops {
  * @disable_source: Disable Source Handler function pointer
  *
  * @ops:	Operation handler callbacks
+ * @req_lock:	Serialise access to @requests
+ * @req_queue_mutex: Serialise validating and queueing requests
+ * @classes:	List of request classes, i.e. which objects may be contained in
+ *		media requests (@struct media_request_class.mdev_list)
  *
  * This structure represents an abstract high-level media device. It allows easy
  * access to entities and provides basic media device-level support. The
@@ -158,6 +171,10 @@ struct media_device {
 	void (*disable_source)(struct media_entity *entity);
 
 	const struct media_device_ops *ops;
+
+	spinlock_t req_lock;
+	struct mutex req_queue_mutex;
+	struct list_head classes;
 };
 
 /* We don't need to include pci.h or usb.h here */
diff --git a/include/media/media-request.h b/include/media/media-request.h
new file mode 100644
index 0000000..04db4ef
--- /dev/null
+++ b/include/media/media-request.h
@@ -0,0 +1,301 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Media device request objects
+ *
+ * Copyright (C) 2018 Intel Corporation
+ *
+ * Author: Sakari Ailus <sakari.ailus@linux.intel.com>
+ */
+
+#ifndef MEDIA_REQUEST_H
+#define MEDIA_REQUEST_H
+
+#include <linux/list.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+
+#include <media/media-device.h>
+
+enum media_request_state {
+	MEDIA_REQUEST_STATE_IDLE,
+	MEDIA_REQUEST_STATE_QUEUEING,
+	MEDIA_REQUEST_STATE_QUEUED,
+	MEDIA_REQUEST_STATE_COMPLETE,
+	MEDIA_REQUEST_STATE_REINIT,
+};
+
+/**
+ * media_request_state_str - Convert media request state to a string
+ *
+ * @state: The state of the media request
+ *
+ * Returns the string corresponding to a media request state.
+ */
+const char *
+media_request_state_str(enum media_request_state state);
+
+/**
+ * struct media_request - Media device request
+ * @mdev: Media device this request belongs to
+ * @kref: Reference count
+ * @req_list: List entry in the media device requests list
+ * @debug_prefix: Prefix for debug messages (process name:fd)
+ * @state: The state of the request
+ * @obj_refs: List of @struct media_request_object_ref req_list field
+ * @incomplete: The number of unsticky objects that have not been completed
+ * @poll_wait: Wait queue for poll
+ */
+struct media_request {
+	struct media_device *mdev;
+	struct kref kref;
+	struct list_head mdev_list;
+#ifdef CONFIG_DYNAMIC_DEBUG
+	char debug_str[TASK_COMM_LEN + 11];
+#endif
+	enum media_request_state state;
+	struct list_head obj_refs;
+	unsigned int incomplete;
+	struct wait_queue_head poll_wait;
+};
+
+struct media_request_object;
+struct media_request_ref;
+
+/**
+ * struct media_request_class - Class of object that may be part of a media
+ *				request
+ *
+ * @objects: List of objects belonging to a class (@struct media_request_object
+ *	     object_list field)
+ * @sticky: Configuration as of the latest request queued; also indicates that a
+ *	    class is sticky
+ * @mdev_list: List entry of the media device's class list
+ * @mdev: The media device
+ * @release: A callback function to release a previously initialised
+ *	     @struct media_request_object
+ */
+struct media_request_class {
+	bool completeable;
+	struct list_head objects;
+	struct media_request_object *sticky;
+	struct list_head mdev_list;
+	struct media_device *mdev;
+	void (*unbind)(struct media_request_ref *ref);
+	void (*release)(struct media_request_object *object);
+};
+
+/**
+ * struct media_request_object - An opaque object that belongs to a media
+ *				 request
+ *
+ * @class: The class which the object is related to
+ * @object_list: List entry of the object list of the class
+ * @kref: Reference to the object, acquire before releasing mdev->req_lock
+ *
+ * An object related to the request. The object data follows this struct.
+ */
+struct media_request_object {
+	struct media_request_class *class;
+	struct list_head object_list;
+	struct kref kref;
+};
+
+/**
+ * struct media_request_ref - Reference to a media request object
+ *
+ * @new: The new object
+ * @old: The old object
+ * @req_list: List entry of in the request's object list
+ * @req: The request the reference is related to
+ * @complete: A reference has been marked complete
+ *
+ * Represents a reference to a media request object; object references are bound
+ * to requests.
+ */
+struct media_request_ref {
+	struct media_request_object *new;
+	struct media_request_object *old;
+	struct list_head req_list;
+	struct media_request *req;
+	bool complete;
+};
+
+#define media_request_for_each_ref(ref, req)	     \
+	lockdep_assert_held(&(req)->mdev->req_lock); \
+	list_for_each_entry(ref, &req->obj_refs, req_list)
+
+
+void media_request_release(struct kref *kref);
+
+static inline void media_request_put(struct media_request *req)
+{
+	kref_put(&req->kref, media_request_release);
+}
+
+static inline void media_request_get(struct media_request *req)
+{
+	kref_get(&req->kref);
+}
+
+struct media_request *
+media_request_find(struct media_device *mdev, int request);
+
+int media_request_alloc(struct media_device *mdev,
+			struct media_request_new *new);
+
+void media_request_complete(struct media_device *mdev,
+			    struct media_request *req);
+
+/**
+ * media_request_class_register - Register a media device request class
+ *
+ * @mdev: The media device
+ * @class: The class to be registered
+ * @unbind: The unbind callback; called when a reference to a resource is
+ *	    unbound from an idle request
+ * @release: Release callback for a request object
+ * @completeable: Whether objects in this class must complete for the request to
+ *		  be completed
+ *
+ * Registers a media device class for request objects. Objects are allocated by
+ * the framework. Sticky objects are kept after the request has been completed;
+ * they are configuration rather than a resource (such as buffers).
+ */
+void
+media_request_class_register(struct media_device *mdev,
+			     struct media_request_class *class,
+			     void (*unbind)(struct media_request_ref *ref),
+			     void (*release)(struct media_request_object *object),
+			     bool completeable);
+
+/**
+ * media_request_class_set_sticky - Make a class sticky
+ *
+ * @class: The request object class
+ * @sticky: The sticky object
+ *
+ * Makes a class sticky as well as sets the sticky object to a class. Sticky
+ * objects represent configuration which may be changed by a request but will
+ * prevail until changed again.
+ */
+void media_request_class_set_sticky(struct media_request_class *class,
+				    struct media_request_object *sticky);
+
+/**
+ * media_request_class_unregister - Unregister a media device request class
+ *
+ * @class: The class to unregister
+ */
+void media_request_class_unregister(struct media_request_class *class);
+
+/**
+ * media_request_object_put - Put a media request object
+ *
+ * @obj: The object
+ *
+ * Put a reference to a media request object. Once all references are gone, the
+ * object's memory is released.
+ */
+void media_request_object_put(struct media_request_object *obj);
+
+/**
+ * media_request_object_init - Initialise an allocated media request object
+ *
+ * @class: The class the object belongs to
+ *
+ * Initialise a media request object. The object will be released using the
+ * release function of the class once it has no references (this function
+ * initialises references to one).
+ */
+void media_request_object_init(struct media_request_class *class,
+			       struct media_request_object *obj);
+
+/**
+ * __media_request_ref_put - Put a reference to a request object
+ *
+ * @ref: The reference
+ *
+ * Put a reference to a media request object. The caller must be holding @struct
+ * media_device.req_lock.
+ */
+void __media_request_ref_put(struct media_request_ref *ref);
+
+/**
+ * media_request_ref_put - Put a reference to a request object
+ *
+ * @ref: The reference
+ *
+ * Put a reference to a media request object.
+ */
+void media_request_ref_put(struct media_request_ref *ref);
+
+/**
+ * media_request_object_bind - Bind an object to a request
+ *
+ * @req: The request where the object is to be added
+ * @obj: The object
+ *
+ * Bind an object to a request.
+ *
+ * Returns a reference to the bound object.
+ */
+struct media_request_ref *
+media_request_object_bind(struct media_request *req,
+			  struct media_request_object *obj);
+
+/**
+ * media_request_sticky_to_old - Move sticky configuration to request
+ *
+ * @req: The request
+ *
+ * Move the current configuration to the request's old configuration.
+ */
+void media_request_sticky_to_old(struct media_request *req);
+
+/**
+ * media_request_new_to_sticky - Make the request configuration stick
+ *
+ * @req: The request
+ *
+ * Make the configuration in the request the current configuration.
+ */
+void media_request_new_to_sticky(struct media_request *req);
+
+/**
+ * media_request_detach_old - Detach old configuration
+ *
+ * @req: The request
+ *
+ * Detach the previous (old) configuration from the request.
+ */
+void media_request_detach_old(struct media_request *req);
+
+/*
+ * media_device_ref_unbind - Unbind an object reference from a request
+ *
+ * @ref: The reference to be unbound.
+ *
+ * Unbind a previously bound reference from a request. The object is put by this
+ * function as well. Only references to completeable objects may be unbound.
+ */
+void media_request_ref_unbind(struct media_request_ref *ref);
+
+/*
+ * media_request_is_complete - Tell whether the media request is complete
+ *
+ * @req: The request
+ *
+ * Return true if all unsticky objects have been completed in a request.
+ */
+bool media_request_is_complete(struct media_request *req);
+
+/**
+ * media_request_ref_complete - Mark a reference complete
+ *
+ * @ref: The reference to the request
+ *
+ * Mark a part of the request as completed. Also puts the ref.
+ */
+void media_request_ref_complete(struct media_request_ref *ref);
+
+#endif
diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
index c7e9a5c..a38e8fc 100644
--- a/include/uapi/linux/media.h
+++ b/include/uapi/linux/media.h
@@ -342,11 +342,19 @@ struct media_v2_topology {
 
 /* ioctls */
 
+struct __attribute__ ((packed)) media_request_new {
+	__s32 fd;
+};
+
 #define MEDIA_IOC_DEVICE_INFO	_IOWR('|', 0x00, struct media_device_info)
 #define MEDIA_IOC_ENUM_ENTITIES	_IOWR('|', 0x01, struct media_entity_desc)
 #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_ALLOC	_IOWR('|', 0x05, struct media_request_new)
+
+#define MEDIA_REQUEST_IOC_QUEUE		_IO('|',  0x80)
+#define MEDIA_REQUEST_IOC_REINIT	_IO('|',  0x81)
 
 #if !defined(__KERNEL__) || defined(__NEED_MEDIA_LEGACY_API)
 
-- 
2.7.4

[RFC v2 03/10] videodev2.h: Add request_fd field to v4l2_buffer

[Sakari Ailus]

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

When queuing buffers allow for passing the request 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>
[Sakari Ailus: requests fds are int; use assign_in_user in get_v4l2_buffer]
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/common/videobuf2/videobuf2-v4l2.c | 2 +-
 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 ++--
 include/uapi/linux/videodev2.h                  | 3 ++-
 5 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
index 886a2d8..6d4d184 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/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 = 0;
 	b->reserved = 0;
 
 	if (q->is_multiplanar) {
diff --git a/drivers/media/usb/cpia2/cpia2_v4l.c b/drivers/media/usb/cpia2/cpia2_v4l.c
index 99f106b..af42ce3 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 5198c9e..61a8bd4 100644
--- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
+++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
@@ -386,7 +386,7 @@ struct v4l2_buffer32 {
 		__s32		fd;
 	} m;
 	__u32			length;
-	__u32			reserved2;
+	__s32			request_fd;
 	__u32			reserved;
 };
 
@@ -500,7 +500,8 @@ static int get_v4l2_buffer32(struct v4l2_buffer __user *kp,
 	    get_user(memory, &up->memory) ||
 	    put_user(memory, &kp->memory) ||
 	    get_user(length, &up->length) ||
-	    put_user(length, &kp->length))
+	    put_user(length, &kp->length) ||
+	    assign_in_user(&kp->request_fd, &up->request_fd))
 		return -EFAULT;
 
 	if (V4L2_TYPE_IS_OUTPUT(type))
@@ -604,7 +605,7 @@ static int put_v4l2_buffer32(struct v4l2_buffer __user *kp,
 	    assign_in_user(&up->timestamp.tv_usec, &kp->timestamp.tv_usec) ||
 	    copy_in_user(&up->timecode, &kp->timecode, sizeof(kp->timecode)) ||
 	    assign_in_user(&up->sequence, &kp->sequence) ||
-	    assign_in_user(&up->reserved2, &kp->reserved2) ||
+	    assign_in_user(&up->request_fd, &kp->request_fd) ||
 	    assign_in_user(&up->reserved, &kp->reserved) ||
 	    get_user(length, &kp->length) ||
 	    put_user(length, &up->length))
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index 672ab22..c2671de 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=%d, 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/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 600877b..d39932d 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -910,6 +910,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.
@@ -933,7 +934,7 @@ struct v4l2_buffer {
 		__s32		fd;
 	} m;
 	__u32			length;
-	__u32			reserved2;
+	__s32			request_fd;
 	__u32			reserved;
 };
 
-- 
2.7.4

[RFC v2 04/10] videodev2.h: add request_fd field to v4l2_ext_controls

[Sakari Ailus]

From: Alexandre Courbot <acourbot@chromium.org>

Allow to specify a request to be used with the S_EXT_CTRLS and
G_EXT_CTRLS operations.

Signed-off-by: Alexandre Courbot <acourbot@chromium.org>
[Sakari Ailus: reserved no longer an array, add compat32 code]
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 9 ++++++---
 drivers/media/v4l2-core/v4l2-ioctl.c          | 2 +-
 include/uapi/linux/videodev2.h                | 3 ++-
 3 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
index 61a8bd4..9adb367 100644
--- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
+++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
@@ -733,7 +733,8 @@ struct v4l2_ext_controls32 {
 	__u32 which;
 	__u32 count;
 	__u32 error_idx;
-	__u32 reserved[2];
+	__s32 request_fd;
+	__u32 reserved;
 	compat_caddr_t controls; /* actually struct v4l2_ext_control32 * */
 };
 
@@ -808,7 +809,8 @@ static int get_v4l2_ext_controls32(struct file *file,
 	    get_user(count, &up->count) ||
 	    put_user(count, &kp->count) ||
 	    assign_in_user(&kp->error_idx, &up->error_idx) ||
-	    copy_in_user(kp->reserved, up->reserved, sizeof(kp->reserved)))
+	    assign_in_user(&kp->request_fd, &up->request_fd) ||
+	    assign_in_user(&kp->reserved, &up->reserved))
 		return -EFAULT;
 
 	if (count == 0)
@@ -866,7 +868,8 @@ static int put_v4l2_ext_controls32(struct file *file,
 	    get_user(count, &kp->count) ||
 	    put_user(count, &up->count) ||
 	    assign_in_user(&up->error_idx, &kp->error_idx) ||
-	    copy_in_user(up->reserved, kp->reserved, sizeof(up->reserved)) ||
+	    assign_in_user(&up->request_fd, &kp->request_fd) ||
+	    assign_in_user(&up->reserved, &kp->reserved) ||
 	    get_user(kcontrols, &kp->controls))
 		return -EFAULT;
 
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index c2671de..85c4bb9 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -870,7 +870,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;
 	for (i = 0; i < c->count; i++)
 		c->controls[i].reserved2[0] = 0;
 
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index d39932d..e6e68a5 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -1593,7 +1593,8 @@ struct v4l2_ext_controls {
 	};
 	__u32 count;
 	__u32 error_idx;
-	__u32 reserved[2];
+	__s32 request_fd;
+	__u32 reserved;
 	struct v4l2_ext_control *controls;
 };
 
-- 
2.7.4

[RFC v2 05/10] staging: media: atomisp: Remove v4l2_buffer.reserved2 field hack

[Sakari Ailus]

The atomisp driver used to use the reserved2 field in struct v4l2_buffer
for picking a particular ISP configuration for the buffer. As reserved2
field will have new use soon, remove this hack.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 .../staging/media/atomisp/pci/atomisp2/atomisp_ioctl.c  | 17 +++--------------
 1 file changed, 3 insertions(+), 14 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_ioctl.c b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_ioctl.c
index 5c84dd6..182bb70 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_ioctl.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_ioctl.c
@@ -1283,16 +1283,7 @@ static int atomisp_qbuf(struct file *file, void *fh, struct v4l2_buffer *buf)
 	if (!((buf->flags & NOFLUSH_FLAGS) == NOFLUSH_FLAGS))
 		wbinvd();
 
-	if (!atomisp_is_vf_pipe(pipe) &&
-	    (buf->reserved2 & ATOMISP_BUFFER_HAS_PER_FRAME_SETTING)) {
-		/* this buffer will have a per-frame parameter */
-		pipe->frame_request_config_id[buf->index] = buf->reserved2 &
-					~ATOMISP_BUFFER_HAS_PER_FRAME_SETTING;
-		dev_dbg(isp->dev, "This buffer requires per_frame setting which has isp_config_id %d\n",
-			pipe->frame_request_config_id[buf->index]);
-	} else {
-		pipe->frame_request_config_id[buf->index] = 0;
-	}
+	pipe->frame_request_config_id[buf->index] = 0;
 
 	pipe->frame_params[buf->index] = NULL;
 
@@ -1470,12 +1461,10 @@ static int atomisp_dqbuf(struct file *file, void *fh, struct v4l2_buffer *buf)
 	buf->reserved &= 0x0000ffff;
 	if (!(buf->flags & V4L2_BUF_FLAG_ERROR))
 		buf->reserved |= __get_frame_exp_id(pipe, buf) << 16;
-	buf->reserved2 = pipe->frame_config_id[buf->index];
 	rt_mutex_unlock(&isp->mutex);
 
-	dev_dbg(isp->dev, "dqbuf buffer %d (%s) for asd%d with exp_id %d, isp_config_id %d\n",
-		buf->index, vdev->name, asd->index, buf->reserved >> 16,
-		buf->reserved2);
+	dev_dbg(isp->dev, "dqbuf buffer %d (%s) for asd%d with exp_id %d\n",
+		buf->index, vdev->name, asd->index, buf->reserved >> 16);
 	return 0;
 }
 
-- 
2.7.4

[RFC v2 06/10] vb2: Add support for requests

[Sakari Ailus]

Associate a buffer to a request when it is queued and disassociate when it
is done.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/common/videobuf2/videobuf2-core.c | 43 ++++++++++++++++++++++++-
 drivers/media/common/videobuf2/videobuf2-v4l2.c | 40 ++++++++++++++++++++++-
 include/media/videobuf2-core.h                  | 19 +++++++++++
 include/media/videobuf2-v4l2.h                  | 28 ++++++++++++++++
 4 files changed, 128 insertions(+), 2 deletions(-)

diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index d3f7bb3..b8535de 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -346,6 +346,9 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory,
 			break;
 		}
 
+		if (q->class)
+			media_request_object_init(q->class, &vb->req_obj);
+
 		vb->state = VB2_BUF_STATE_DEQUEUED;
 		vb->vb2_queue = q;
 		vb->num_planes = num_planes;
@@ -520,7 +523,10 @@ static int __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
 	/* Free videobuf buffers */
 	for (buffer = q->num_buffers - buffers; buffer < q->num_buffers;
 	     ++buffer) {
-		kfree(q->bufs[buffer]);
+		if (q->class)
+			media_request_object_put(&q->bufs[buffer]->req_obj);
+		else
+			kfree(q->bufs[buffer]);
 		q->bufs[buffer] = NULL;
 	}
 
@@ -944,6 +950,10 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state)
 	default:
 		/* Inform any processes that may be waiting for buffers */
 		wake_up(&q->done_wq);
+		if (vb->req_ref) {
+			media_request_ref_complete(vb->req_ref);
+			vb->req_ref = NULL;
+		}
 		break;
 	}
 }
@@ -1249,6 +1259,32 @@ static int __buf_prepare(struct vb2_buffer *vb, const void *pb)
 		return -EIO;
 	}
 
+	if (vb->request_fd) {
+		struct media_request *req;
+		struct media_request_ref *ref;
+
+		if (!q->class) {
+			dprintk(1, "requests not enabled for the queue\n");
+			return -EINVAL;
+		}
+
+		req = media_request_find(q->class->mdev, vb->request_fd);
+		if (IS_ERR(req)) {
+			dprintk(1, "no request found for fd %d (%ld)\n",
+				vb->request_fd, PTR_ERR(req));
+			return PTR_ERR(req);
+		}
+
+		ref = media_request_object_bind(req,
+						&q->bufs[vb->index]->req_obj);
+		media_request_put(req);
+
+		if (IS_ERR(ref))
+			return PTR_ERR(ref);
+
+		vb->req_ref = ref;
+	}
+
 	vb->state = VB2_BUF_STATE_PREPARING;
 
 	switch (q->memory) {
@@ -1269,6 +1305,8 @@ static int __buf_prepare(struct vb2_buffer *vb, const void *pb)
 	if (ret) {
 		dprintk(1, "buffer preparation failed: %d\n", ret);
 		vb->state = VB2_BUF_STATE_DEQUEUED;
+		media_request_ref_unbind(vb->req_ref);
+		vb->req_ref = NULL;
 		return ret;
 	}
 
@@ -2037,6 +2075,9 @@ void vb2_core_queue_release(struct vb2_queue *q)
 	mutex_lock(&q->mmap_lock);
 	__vb2_queue_free(q, q->num_buffers);
 	mutex_unlock(&q->mmap_lock);
+	media_request_class_unregister(q->class);
+	kfree(q->class);
+	q->class = NULL;
 }
 EXPORT_SYMBOL_GPL(vb2_core_queue_release);
 
diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
index 6d4d184..30047b9 100644
--- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
+++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
@@ -196,6 +196,7 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, void *pb)
 	b->index = vb->index;
 	b->type = vb->type;
 	b->memory = vb->memory;
+	b->request_fd = vb->request_fd;
 	b->bytesused = 0;
 
 	b->flags = vbuf->flags;
@@ -203,7 +204,6 @@ 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->request_fd = 0;
 	b->reserved = 0;
 
 	if (q->is_multiplanar) {
@@ -319,6 +319,7 @@ static int __fill_vb2_buffer(struct vb2_buffer *vb,
 		return -EINVAL;
 	}
 	vb->timestamp = 0;
+	vb->request_fd = b->request_fd;
 	vbuf->sequence = 0;
 
 	if (V4L2_TYPE_IS_MULTIPLANAR(b->type)) {
@@ -667,6 +668,43 @@ int vb2_queue_init(struct vb2_queue *q)
 }
 EXPORT_SYMBOL_GPL(vb2_queue_init);
 
+static void vb2_media_req_obj_release(struct media_request_object *obj)
+{
+	struct vb2_v4l2_buffer *vbuf =
+		to_vb2_v4l2_buffer(media_request_object_to_vb2_buffer(obj));
+
+	kfree(vbuf);
+}
+
+static void vb2_media_req_ref_unbind(struct media_request_ref *ref)
+{
+	struct vb2_v4l2_buffer *vbuf =
+		to_vb2_v4l2_buffer(
+			media_request_object_to_vb2_buffer(ref->new));
+
+	vb2_buffer_done(&vbuf->vb2_buf, VB2_BUF_STATE_ERROR);
+}
+
+void vb2_queue_deny_requests(struct vb2_queue *q)
+{
+	media_request_class_unregister(q->class);
+}
+EXPORT_SYMBOL_GPL(vb2_queue_deny_requests);
+
+int vb2_queue_allow_requests(struct vb2_queue *q, struct media_device *mdev)
+{
+	q->class = kzalloc(sizeof(*q->class), GFP_KERNEL);
+	if (!q->class)
+		return -ENOMEM;
+
+	media_request_class_register(mdev, q->class,
+				     vb2_media_req_ref_unbind,
+				     vb2_media_req_obj_release, true);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(vb2_queue_allow_requests);
+
 void vb2_queue_release(struct vb2_queue *q)
 {
 	vb2_core_queue_release(q);
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index f6818f73..68013e8 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -18,6 +18,8 @@
 #include <linux/dma-buf.h>
 #include <linux/bitops.h>
 
+#include <media/media-request.h>
+
 #define VB2_MAX_FRAME	(32)
 #define VB2_MAX_PLANES	(8)
 
@@ -42,6 +44,8 @@ enum vb2_memory {
 	VB2_MEMORY_DMABUF	= 4,
 };
 
+struct media_request_class;
+struct media_request_ref;
 struct vb2_fileio_data;
 struct vb2_threadio_data;
 
@@ -255,12 +259,19 @@ struct vb2_buffer {
 	 * done_entry:		entry on the list that stores all buffers ready
 	 *			to be dequeued to userspace
 	 * vb2_plane:		per-plane information; do not change
+	 * req_obj:		media request object
+	 * req_ref:		media request reference (stored between qbuf --
+	 *			dqbuf)
+	 * request_fd		file descriptor of the request
 	 */
 	enum vb2_buffer_state	state;
 
 	struct vb2_plane	planes[VB2_MAX_PLANES];
 	struct list_head	queued_entry;
 	struct list_head	done_entry;
+	struct media_request_object req_obj;
+	struct media_request_ref *req_ref;
+	int			request_fd;
 #ifdef CONFIG_VIDEO_ADV_DEBUG
 	/*
 	 * Counters for how often these buffer-related ops are
@@ -293,6 +304,12 @@ struct vb2_buffer {
 #endif
 };
 
+static inline struct vb2_buffer *
+media_request_object_to_vb2_buffer(struct media_request_object *obj)
+{
+	return container_of(obj, struct vb2_buffer, req_obj);
+}
+
 /**
  * struct vb2_ops - driver-specific callbacks.
  *
@@ -505,6 +522,7 @@ 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_class:	The media request class for buffers in this queue
  */
 struct vb2_queue {
 	unsigned int			type;
@@ -558,6 +576,7 @@ struct vb2_queue {
 
 	struct vb2_fileio_data		*fileio;
 	struct vb2_threadio_data	*threadio;
+	struct media_request_class	*class;
 
 #ifdef CONFIG_VIDEO_ADV_DEBUG
 	/*
diff --git a/include/media/videobuf2-v4l2.h b/include/media/videobuf2-v4l2.h
index 3d5e2d7..82e9284 100644
--- a/include/media/videobuf2-v4l2.h
+++ b/include/media/videobuf2-v4l2.h
@@ -32,6 +32,7 @@
  *		&enum v4l2_field.
  * @timecode:	frame timecode.
  * @sequence:	sequence count of this frame.
+ * @request:	request used by the buffer
  *
  * Should contain enough information to be able to cover all the fields
  * of &struct v4l2_buffer at ``videodev2.h``.
@@ -43,6 +44,7 @@ struct vb2_v4l2_buffer {
 	__u32			field;
 	struct v4l2_timecode	timecode;
 	__u32			sequence;
+	__u32			request;
 };
 
 /*
@@ -204,6 +206,32 @@ int vb2_streamoff(struct vb2_queue *q, enum v4l2_buf_type type);
 int __must_check vb2_queue_init(struct vb2_queue *q);
 
 /**
+ * vb2_queue_allow_requests - Allow requests on a videobuf2 queue
+ *
+ * @q:		pointer to &struct vb2_queue with videobuf2 queue
+ * @mdev:	the media device that contains the request queue
+ *
+ * Allow using requests on a VB2 queue. This allocates and registers a
+ * request class with the media device. To clean up, either release
+ * the queue using @vb2_queue_release or if that is not practical,
+ * @vb2_queue_deny_requests may also be used to unregister and free
+ * the request class.
+ */
+int vb2_queue_allow_requests(struct vb2_queue *q, struct media_device *mdev);
+
+/**
+ * vb2_queue_deny_requests - Deny requests on a videobuf2 queue
+ *
+ * @q:		pointer to &struct vb2_queue with videobuf2 queue
+ *
+ * Deny requests on a VB2 queue. This function may only be called on
+ * an unused queue in error handling paths. Do not use it in other
+ * circumstances; @vb2_queue_release is enough to release the
+ * resources related to request management.
+ */
+void vb2_queue_deny_requests(struct vb2_queue *q);
+
+/**
  * vb2_queue_release() - stop streaming, release the queue and free memory
  * @q:		pointer to &struct vb2_queue with videobuf2 queue.
  *
-- 
2.7.4

[RFC v2 07/10] v4l: m2m: Simplify exiting the function in v4l2_m2m_try_schedule

[Sakari Ailus]

The v4l2_m2m_try_schedule function acquires and releases multiple
spinlocks; simplify unlocking the job lock by adding a label to unlock the
job lock and exit the function.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/v4l2-core/v4l2-mem2mem.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
index c4f963d..9fbf778 100644
--- a/drivers/media/v4l2-core/v4l2-mem2mem.c
+++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
@@ -230,15 +230,13 @@ void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
 
 	/* If the context is aborted then don't schedule it */
 	if (m2m_ctx->job_flags & TRANS_ABORT) {
-		spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags_job);
 		dprintk("Aborted context\n");
-		return;
+		goto out_unlock;
 	}
 
 	if (m2m_ctx->job_flags & TRANS_QUEUED) {
-		spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags_job);
 		dprintk("On job queue already\n");
-		return;
+		goto out_unlock;
 	}
 
 	spin_lock_irqsave(&m2m_ctx->out_q_ctx.rdy_spinlock, flags_out);
@@ -246,9 +244,8 @@ void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
 	    && !m2m_ctx->out_q_ctx.buffered) {
 		spin_unlock_irqrestore(&m2m_ctx->out_q_ctx.rdy_spinlock,
 					flags_out);
-		spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags_job);
 		dprintk("No input buffers available\n");
-		return;
+		goto out_unlock;
 	}
 	spin_lock_irqsave(&m2m_ctx->cap_q_ctx.rdy_spinlock, flags_cap);
 	if (list_empty(&m2m_ctx->cap_q_ctx.rdy_queue)
@@ -257,18 +254,16 @@ void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
 					flags_cap);
 		spin_unlock_irqrestore(&m2m_ctx->out_q_ctx.rdy_spinlock,
 					flags_out);
-		spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags_job);
 		dprintk("No output buffers available\n");
-		return;
+		goto out_unlock;
 	}
 	spin_unlock_irqrestore(&m2m_ctx->cap_q_ctx.rdy_spinlock, flags_cap);
 	spin_unlock_irqrestore(&m2m_ctx->out_q_ctx.rdy_spinlock, flags_out);
 
 	if (m2m_dev->m2m_ops->job_ready
 		&& (!m2m_dev->m2m_ops->job_ready(m2m_ctx->priv))) {
-		spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags_job);
 		dprintk("Driver not ready\n");
-		return;
+		goto out_unlock;
 	}
 
 	list_add_tail(&m2m_ctx->queue, &m2m_dev->job_queue);
@@ -277,6 +272,13 @@ void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
 	spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags_job);
 
 	v4l2_m2m_try_run(m2m_dev);
+
+	return;
+
+out_unlock:
+	spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags_job);
+
+	return;
 }
 EXPORT_SYMBOL_GPL(v4l2_m2m_try_schedule);
 
-- 
2.7.4

[RFC v2 08/10] v4l: m2m: Support requests with video buffers

[Sakari Ailus]

Enable supporting requests on V4L2 buffer queues on M2M devices. This
requires Media controller.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/v4l2-core/v4l2-mem2mem.c | 109 +++++++++++++++++++++++++++++++++
 include/media/v4l2-mem2mem.h           |  28 +++++++++
 2 files changed, 137 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
index 9fbf778..effdd15 100644
--- a/drivers/media/v4l2-core/v4l2-mem2mem.c
+++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
@@ -17,6 +17,7 @@
 #include <linux/sched.h>
 #include <linux/slab.h>
 
+#include <media/media-device.h>
 #include <media/videobuf2-v4l2.h>
 #include <media/v4l2-mem2mem.h>
 #include <media/v4l2-dev.h>
@@ -57,6 +58,8 @@ module_param(debug, bool, 0644);
  * @job_queue:		instances queued to run
  * @job_spinlock:	protects job_queue
  * @m2m_ops:		driver callbacks
+ * @mdev:		media device; optional
+ * @allow_requests:	whether requests are allowed on the M2M device
  */
 struct v4l2_m2m_dev {
 	struct v4l2_m2m_ctx	*curr_ctx;
@@ -65,6 +68,9 @@ struct v4l2_m2m_dev {
 	spinlock_t		job_spinlock;
 
 	const struct v4l2_m2m_ops *m2m_ops;
+
+	struct media_device	*mdev;
+	bool			allow_requests;
 };
 
 static struct v4l2_m2m_queue_ctx *get_queue_ctx(struct v4l2_m2m_ctx *m2m_ctx,
@@ -89,6 +95,78 @@ struct vb2_queue *v4l2_m2m_get_vq(struct v4l2_m2m_ctx *m2m_ctx,
 }
 EXPORT_SYMBOL(v4l2_m2m_get_vq);
 
+struct media_request *v4l2_m2m_req_alloc(struct media_device *mdev)
+{
+	struct v4l2_m2m_request *vreq;
+
+	vreq = kzalloc(sizeof(*vreq), GFP_KERNEL);
+	if (!vreq)
+		return NULL;
+
+	return &vreq->req;
+}
+EXPORT_SYMBOL_GPL(v4l2_m2m_req_alloc);
+
+void v4l2_m2m_req_free(struct media_request *req)
+{
+	struct v4l2_m2m_request *vreq =
+		container_of(req, struct v4l2_m2m_request, req);
+
+	kfree(vreq);
+}
+EXPORT_SYMBOL_GPL(v4l2_m2m_req_free);
+
+int v4l2_m2m_req_queue(struct media_request *req)
+{
+	struct v4l2_m2m_request *vreq =
+		container_of(req, struct v4l2_m2m_request, req);
+	struct v4l2_m2m_ctx *m2m_ctx = vreq->ctx;
+	struct v4l2_m2m_dev *m2m_dev = m2m_ctx->m2m_dev;
+	struct media_request_ref *iter;
+	unsigned long flags;
+
+	media_request_for_each_ref(iter, req)
+		if (iter->new->class == m2m_ctx->cap_q_ctx.q.class)
+			vreq->cap_ref = iter;
+		else if (iter->new->class == m2m_ctx->out_q_ctx.q.class)
+			vreq->out_ref = iter;
+		else
+			return -EINVAL;
+
+	if (!vreq->out_ref || !vreq->cap_ref)
+		return -EINVAL;
+
+	spin_lock_irqsave(&m2m_dev->job_spinlock, flags);
+	list_add(&vreq->queue_list, &m2m_ctx->request_queue);
+	spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags);
+
+	v4l2_m2m_try_schedule(m2m_ctx);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(v4l2_m2m_req_queue);
+
+struct v4l2_m2m_request *v4l2_m2m_next_req(struct v4l2_m2m_ctx *m2m_ctx)
+{
+	struct v4l2_m2m_dev *m2m_dev = m2m_ctx->m2m_dev;
+	struct v4l2_m2m_request *vreq;
+	unsigned long flags;
+
+	spin_lock_irqsave(&m2m_dev->job_spinlock, flags);
+	if (list_empty(&m2m_ctx->request_queue)) {
+		spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags);
+		return NULL;
+	}
+
+	vreq = list_first_entry(&m2m_ctx->request_queue,
+				struct v4l2_m2m_request, queue_list);
+
+	spin_unlock_irqrestore(&m2m_dev->job_spinlock, flags);
+
+	return vreq;
+}
+EXPORT_SYMBOL_GPL(v4l2_m2m_next_req);
+
 void *v4l2_m2m_next_buf(struct v4l2_m2m_queue_ctx *q_ctx)
 {
 	struct v4l2_m2m_buffer *b;
@@ -239,6 +317,11 @@ void v4l2_m2m_try_schedule(struct v4l2_m2m_ctx *m2m_ctx)
 		goto out_unlock;
 	}
 
+	if (m2m_dev->allow_requests && list_empty(&m2m_ctx->request_queue)) {
+		dprintk("No requests queued\n");
+		goto out_unlock;
+	}
+
 	spin_lock_irqsave(&m2m_ctx->out_q_ctx.rdy_spinlock, flags_out);
 	if (list_empty(&m2m_ctx->out_q_ctx.rdy_queue)
 	    && !m2m_ctx->out_q_ctx.buffered) {
@@ -393,6 +476,9 @@ int v4l2_m2m_qbuf(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
 	struct vb2_queue *vq;
 	int ret;
 
+	if (m2m_ctx->m2m_dev->allow_requests && !buf->request_fd)
+		return -EINVAL;
+
 	vq = v4l2_m2m_get_vq(m2m_ctx, buf->type);
 	ret = vb2_qbuf(vq, buf);
 	if (!ret)
@@ -618,6 +704,17 @@ struct v4l2_m2m_dev *v4l2_m2m_init(const struct v4l2_m2m_ops *m2m_ops)
 }
 EXPORT_SYMBOL_GPL(v4l2_m2m_init);
 
+void v4l2_m2m_allow_requests(struct v4l2_m2m_dev *m2m_dev,
+			     struct media_device *mdev)
+{
+	if (WARN_ON(!mdev))
+		return;
+
+	m2m_dev->mdev = mdev;
+	m2m_dev->allow_requests = true;
+}
+EXPORT_SYMBOL_GPL(v4l2_m2m_allow_requests);
+
 void v4l2_m2m_release(struct v4l2_m2m_dev *m2m_dev)
 {
 	kfree(m2m_dev);
@@ -643,6 +740,7 @@ struct v4l2_m2m_ctx *v4l2_m2m_ctx_init(struct v4l2_m2m_dev *m2m_dev,
 	out_q_ctx = &m2m_ctx->out_q_ctx;
 	cap_q_ctx = &m2m_ctx->cap_q_ctx;
 
+	INIT_LIST_HEAD(&m2m_ctx->request_queue);
 	INIT_LIST_HEAD(&out_q_ctx->rdy_queue);
 	INIT_LIST_HEAD(&cap_q_ctx->rdy_queue);
 	spin_lock_init(&out_q_ctx->rdy_spinlock);
@@ -651,9 +749,17 @@ struct v4l2_m2m_ctx *v4l2_m2m_ctx_init(struct v4l2_m2m_dev *m2m_dev,
 	INIT_LIST_HEAD(&m2m_ctx->queue);
 
 	ret = queue_init(drv_priv, &out_q_ctx->q, &cap_q_ctx->q);
+	if (ret)
+		goto err;
+
+	ret = vb2_queue_allow_requests(&cap_q_ctx->q, m2m_dev->mdev);
+	if (ret)
+		goto err;
 
+	ret = vb2_queue_allow_requests(&out_q_ctx->q, m2m_dev->mdev);
 	if (ret)
 		goto err;
+
 	/*
 	 * If both queues use same mutex assign it as the common buffer
 	 * queues lock to the m2m context. This lock is used in the
@@ -663,7 +769,10 @@ struct v4l2_m2m_ctx *v4l2_m2m_ctx_init(struct v4l2_m2m_dev *m2m_dev,
 		m2m_ctx->q_lock = out_q_ctx->q.lock;
 
 	return m2m_ctx;
+
 err:
+	vb2_queue_deny_requests(&out_q_ctx->q);
+	vb2_queue_deny_requests(&cap_q_ctx->q);
 	kfree(m2m_ctx);
 	return ERR_PTR(ret);
 }
diff --git a/include/media/v4l2-mem2mem.h b/include/media/v4l2-mem2mem.h
index 3d07ba3..92732a7 100644
--- a/include/media/v4l2-mem2mem.h
+++ b/include/media/v4l2-mem2mem.h
@@ -18,6 +18,7 @@
 #define _MEDIA_V4L2_MEM2MEM_H
 
 #include <media/videobuf2-v4l2.h>
+#include <media/media-request.h>
 
 /**
  * struct v4l2_m2m_ops - mem-to-mem device driver callbacks
@@ -53,6 +54,7 @@ struct v4l2_m2m_ops {
 	void (*unlock)(void *priv);
 };
 
+struct media_device;
 struct v4l2_m2m_dev;
 
 /**
@@ -85,7 +87,9 @@ struct v4l2_m2m_queue_ctx {
  * @m2m_dev: opaque pointer to the internal data to handle M2M context
  * @cap_q_ctx: Capture (output to memory) queue context
  * @out_q_ctx: Output (input from memory) queue context
+ * @mdev: The media device; optional
  * @queue: List of memory to memory contexts
+ * @request_queue: queued requests in this context
  * @job_flags: Job queue flags, used internally by v4l2-mem2mem.c:
  *		%TRANS_QUEUED, %TRANS_RUNNING and %TRANS_ABORT.
  * @finished: Wait queue used to signalize when a job queue finished.
@@ -109,6 +113,7 @@ struct v4l2_m2m_ctx {
 	struct list_head		queue;
 	unsigned long			job_flags;
 	wait_queue_head_t		finished;
+	struct list_head		request_queue;
 
 	void				*priv;
 };
@@ -124,6 +129,17 @@ struct v4l2_m2m_buffer {
 	struct list_head	list;
 };
 
+struct v4l2_m2m_request {
+	struct list_head queue_list;
+	struct v4l2_m2m_ctx *ctx;
+	struct media_request_ref *cap_ref, *out_ref;
+	struct media_request req;
+};
+
+struct media_request *v4l2_m2m_req_alloc(struct media_device *mdev);
+void v4l2_m2m_req_free(struct media_request *req);
+int v4l2_m2m_req_queue(struct media_request *req);
+
 /**
  * v4l2_m2m_get_curr_priv() - return driver private data for the currently
  * running instance or NULL if no instance is running
@@ -329,6 +345,17 @@ int v4l2_m2m_mmap(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
 struct v4l2_m2m_dev *v4l2_m2m_init(const struct v4l2_m2m_ops *m2m_ops);
 
 /**
+ * v4l2_m2m_allow_requests - allow requests on an m2m device
+ *
+ * @m2m_dev: the m2m device
+ * @mdev: the media device
+ *
+ * Allow using media requests on an M2M device.
+ */
+void v4l2_m2m_allow_requests(struct v4l2_m2m_dev *m2m_dev,
+			     struct media_device *mdev);
+
+/**
  * v4l2_m2m_release() - cleans up and frees a m2m_dev structure
  *
  * @m2m_dev: opaque pointer to the internal data to handle M2M context
@@ -407,6 +434,7 @@ unsigned int v4l2_m2m_num_dst_bufs_ready(struct v4l2_m2m_ctx *m2m_ctx)
 	return m2m_ctx->cap_q_ctx.num_rdy;
 }
 
+struct v4l2_m2m_request *v4l2_m2m_next_req(struct v4l2_m2m_ctx *m2m_ctx);
 /**
  * v4l2_m2m_next_buf() - return next buffer from the list of ready buffers
  *
-- 
2.7.4

[RFC v2 09/10] vim2m: Register V4L2 video device after V4L2 mem2mem init

[Sakari Ailus]

Initialise the V4L2 mem2mem framework before creating the video device.
Referencing ctx->m2m_dev isn't allowed before that as it is NULL.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/platform/vim2m.c | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/drivers/media/platform/vim2m.c b/drivers/media/platform/vim2m.c
index 065483e..9b6b456 100644
--- a/drivers/media/platform/vim2m.c
+++ b/drivers/media/platform/vim2m.c
@@ -1008,6 +1008,16 @@ static int vim2m_probe(struct platform_device *pdev)
 	atomic_set(&dev->num_inst, 0);
 	mutex_init(&dev->dev_mutex);
 
+	timer_setup(&dev->timer, device_isr, 0);
+	platform_set_drvdata(pdev, dev);
+
+	dev->m2m_dev = v4l2_m2m_init(&m2m_ops);
+	if (IS_ERR(dev->m2m_dev)) {
+		v4l2_err(&dev->v4l2_dev, "Failed to init mem2mem device\n");
+		ret = PTR_ERR(dev->m2m_dev);
+		goto err_unreg_v4l2_dev;
+	}
+
 	dev->vfd = vim2m_videodev;
 	vfd = &dev->vfd;
 	vfd->lock = &dev->dev_mutex;
@@ -1016,7 +1026,7 @@ static int vim2m_probe(struct platform_device *pdev)
 	ret = video_register_device(vfd, VFL_TYPE_GRABBER, 0);
 	if (ret) {
 		v4l2_err(&dev->v4l2_dev, "Failed to register video device\n");
-		goto unreg_dev;
+		goto err_unreg_vdev;
 	}
 
 	video_set_drvdata(vfd, dev);
@@ -1024,22 +1034,13 @@ static int vim2m_probe(struct platform_device *pdev)
 	v4l2_info(&dev->v4l2_dev,
 			"Device registered as /dev/video%d\n", vfd->num);
 
-	timer_setup(&dev->timer, device_isr, 0);
-	platform_set_drvdata(pdev, dev);
-
-	dev->m2m_dev = v4l2_m2m_init(&m2m_ops);
-	if (IS_ERR(dev->m2m_dev)) {
-		v4l2_err(&dev->v4l2_dev, "Failed to init mem2mem device\n");
-		ret = PTR_ERR(dev->m2m_dev);
-		goto err_m2m;
-	}
-
 	return 0;
 
-err_m2m:
-	v4l2_m2m_release(dev->m2m_dev);
+err_unreg_vdev:
 	video_unregister_device(&dev->vfd);
-unreg_dev:
+	v4l2_m2m_release(dev->m2m_dev);
+
+err_unreg_v4l2_dev:
 	v4l2_device_unregister(&dev->v4l2_dev);
 
 	return ret;
-- 
2.7.4

[RFC v2 10/10] vim2m: Request support

[Sakari Ailus]

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/platform/vim2m.c | 49 ++++++++++++++++++++++++++++++++++++------
 1 file changed, 43 insertions(+), 6 deletions(-)

diff --git a/drivers/media/platform/vim2m.c b/drivers/media/platform/vim2m.c
index 9b6b456..a8fe3ea 100644
--- a/drivers/media/platform/vim2m.c
+++ b/drivers/media/platform/vim2m.c
@@ -24,6 +24,7 @@
 #include <linux/slab.h>
 
 #include <linux/platform_device.h>
+#include <media/media-device.h>
 #include <media/v4l2-mem2mem.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-ioctl.h>
@@ -138,6 +139,7 @@ static struct vim2m_fmt *find_format(struct v4l2_format *f)
 }
 
 struct vim2m_dev {
+	struct media_device	mdev;
 	struct v4l2_device	v4l2_dev;
 	struct video_device	vfd;
 
@@ -200,6 +202,7 @@ static struct vim2m_q_data *get_q_data(struct vim2m_ctx *ctx,
 
 
 static int device_process(struct vim2m_ctx *ctx,
+			  struct v4l2_m2m_request *vreq,
 			  struct vb2_v4l2_buffer *in_vb,
 			  struct vb2_v4l2_buffer *out_vb)
 {
@@ -377,12 +380,21 @@ static void device_run(void *priv)
 {
 	struct vim2m_ctx *ctx = priv;
 	struct vim2m_dev *dev = ctx->dev;
-	struct vb2_v4l2_buffer *src_buf, *dst_buf;
-
-	src_buf = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
-	dst_buf = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx);
+	struct vb2_v4l2_buffer *src_buf = NULL, *dst_buf = NULL;
+	struct v4l2_m2m_request *vreq;
+
+	vreq = v4l2_m2m_next_req(ctx->fh.m2m_ctx);
+	if (vreq) {
+		src_buf = to_vb2_v4l2_buffer(
+			media_request_object_to_vb2_buffer(vreq->out_ref->new));
+		dst_buf = to_vb2_v4l2_buffer(
+			media_request_object_to_vb2_buffer(vreq->cap_ref->new));
+	} else {
+		src_buf = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
+		dst_buf = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx);
+	}
 
-	device_process(ctx, src_buf, dst_buf);
+	device_process(ctx, vreq, src_buf, dst_buf);
 
 	/* Run a timer, which simulates a hardware irq  */
 	schedule_irq(dev, ctx->transtime);
@@ -989,6 +1001,12 @@ static const struct v4l2_m2m_ops m2m_ops = {
 	.job_abort	= job_abort,
 };
 
+static const struct media_device_ops vim2m_mdev_ops = {
+	.req_alloc = v4l2_m2m_req_alloc,
+	.req_free = v4l2_m2m_req_free,
+	.req_queue = v4l2_m2m_req_queue,
+};
+
 static int vim2m_probe(struct platform_device *pdev)
 {
 	struct vim2m_dev *dev;
@@ -1001,9 +1019,17 @@ static int vim2m_probe(struct platform_device *pdev)
 
 	spin_lock_init(&dev->irqlock);
 
+	dev->mdev.dev = &pdev->dev;
+	dev->mdev.ops = &vim2m_mdev_ops;
+	strlcpy(dev->mdev.model, "vim2m", sizeof(dev->mdev.model));
+	snprintf(dev->mdev.bus_info, sizeof(dev->mdev.bus_info), "platform:%s",
+		 MEM2MEM_NAME);
+	media_device_init(&dev->mdev);
+
+	dev->v4l2_dev.mdev = &dev->mdev;
 	ret = v4l2_device_register(&pdev->dev, &dev->v4l2_dev);
 	if (ret)
-		return ret;
+		goto err_cleanup_mdev;
 
 	atomic_set(&dev->num_inst, 0);
 	mutex_init(&dev->dev_mutex);
@@ -1018,6 +1044,8 @@ static int vim2m_probe(struct platform_device *pdev)
 		goto err_unreg_v4l2_dev;
 	}
 
+	v4l2_m2m_allow_requests(dev->m2m_dev, &dev->mdev);
+
 	dev->vfd = vim2m_videodev;
 	vfd = &dev->vfd;
 	vfd->lock = &dev->dev_mutex;
@@ -1034,6 +1062,10 @@ static int vim2m_probe(struct platform_device *pdev)
 	v4l2_info(&dev->v4l2_dev,
 			"Device registered as /dev/video%d\n", vfd->num);
 
+	ret = media_device_register(&dev->mdev);
+	if (ret)
+		goto err_unreg_vdev;
+
 	return 0;
 
 err_unreg_vdev:
@@ -1043,6 +1075,9 @@ static int vim2m_probe(struct platform_device *pdev)
 err_unreg_v4l2_dev:
 	v4l2_device_unregister(&dev->v4l2_dev);
 
+err_cleanup_mdev:
+	media_device_cleanup(&dev->mdev);
+
 	return ret;
 }
 
@@ -1055,6 +1090,8 @@ static int vim2m_remove(struct platform_device *pdev)
 	del_timer_sync(&dev->timer);
 	video_unregister_device(&dev->vfd);
 	v4l2_device_unregister(&dev->v4l2_dev);
+	media_device_unregister(&dev->mdev);
+	media_device_cleanup(&dev->mdev);
 
 	return 0;
 }
-- 
2.7.4

Re: [RFC v2 00/10] Preparing the request API

[Hans Verkuil]

Hi all,

On 03/23/2018 10:17 PM, Sakari Ailus wrote:
> Hi folks,
> 
> This preliminary RFC patchset prepares for the request API. What's new
> here is support for binding arbitrary configuration or resources to
> requests.
> 
> There are a few new concepts here:
> 
> Class --- a type of configuration or resource a driver (or e.g. the V4L2
> framework) can attach to a resource. E.g. a video buffer queue would be a
> class.
> 
> Object --- an instance of the class. This may be either configuration (in
> which case the setting will stay until changed, e.g. V4L2 format on a
> video node) or a resource (such as a video buffer).
> 
> Reference --- a reference to an object. If a configuration is not changed
> in a request, instead of allocating a new object, a reference to an
> existing object is used. This saves time and memory.
> 
> I expect Laurent to comment on aligning the concept names between the
> request API and DRM. As far as I understand, the respective DRM names for
> "class" and "object" used in this set would be "object" and "state".
> 
> The drivers will need to interact with the requests in three ways:
> 
> - Allocate new configurations or resources. Drivers are free to store
>   their own data into request objects as well. These callbacks are
>   specific to classes.
> 
> - Validate and queue callbacks. These callbacks are used to try requests
>   (validate only) as well as queue them (validate and queue). These
>   callbacks are media device wide, at least for now.
> 
> The lifetime of the objects related to requests is based on refcounting
> both requests and request objects. This fits well for existing use cases
> whether or not based on refcounting; what still needs most of the
> attention is likely that the number of gets and puts matches once the
> object is no longer needed.
> 
> Configuration can be bound to the request the usual way (V4L2 IOCTLs with
> the request_fd field set to the request). Once queued, request completion
> is signalled through polling the request file handle (POLLPRI).
> 
> I'm posting this as an RFC because it's not complete yet. The code
> compiles but no testing has been done yet.

Thank you for this patch series. There are some good ideas here, but it is
quite far from being useful with Alexandre's RFCv4 series.

So this weekend I worked on a merger of this work and the RFCv4 Request API
patch series, taking what I think are the best bits of both.

It is available here:

https://git.linuxtv.org/hverkuil/media_tree.git/log/?h=reqv6

It compiles (although it expects that the media controller is selected in the
kernel config) and tomorrow I will start testing.

The goal of this series is to make the actual driver changes for existing drivers
as simple as possible and do almost all of the work in vb2 and the control framework.

See e.g. the patch adding support for requests to vim2m:

https://git.linuxtv.org/hverkuil/media_tree.git/commit/?h=reqv6&id=0703c330bb53068cdf92e485437c820a018766bc

Of course, this assumes that it actually works, which I'm sure it won't :-)
Hopefully by the end of the day tomorrow I have something that is actually
working.

The concept of media request objects was very useful and very easy to integrate.

Integrating vb2 was harder, esp. since the fence support has not been merged yet
and since that conflicts big time with the request API it is pretty certain that
that code will change.

The control handling of requests is still at the same level of RFCv4: it basically
just copies the current device state when controls are get/set/tried for the first
time for a request. It's good enough for now, but this needs more work to conform
to the Request API RFC.

Note that I didn't copy the 'sticky' concept. When you mark a request object as
'completed', then it just 'sticks around' until the request is freed.

If the request object is a resource (vb2 buffer), then you don't mark it as
completed, instead you just remove it from the request once you're finished
using it.

BTW, I think we seriously need to consider always enabling MEDIA_CONTROLLER
for V4L2. It really doesn't add much to the internal data structures and
having to do #ifdef CONFIG_MEDIA_CONTROLLER all the time is a pain (which
is why I ignored that in my code for now).

Regards,

	Hans

> 
> Todo list:
> 
> - Testing! (And fixing the bugs.)
> 
> - Request support in a few drivers as well as the control framework.
> 
> - Request support for V4L2 formats?
> 
> In the future, support for changing e.g. Media controller link state or
> V4L2 sub-device formats will need to be added. Those should receive more
> attention when the core is in a good shape and the more simple use cases
> are already functional.
> 
> Comments and questions are welcome.
> 
> since v1:
> 
> - Provide an iterator helper for request objects in a request.
> 
> - Remove the request lists in the media device (they were not used)
> 
> - Move request queing to request fd and add reinit (Alexandre's patchset);
>   this roughly corresponds to Request API RFC v2 from Hans.
>   (MEDIA_IOC_REQUEST_ALLOC argument is a struct pointer instead of an
>   __s32 pointer.)
> 
> - Provide a way to unbind request objects from an unqueued request
>   (reinit, closing request fd).
> 
> - v4l2-mem2mem + vivid implementation without control support.
> 
> - More states for requests. In order to take a spinlock (or a mutex) for
>   an extended period of time, add a "QUEUEING" and "REINIT" states.
> 
> - Move non-IOCTL code to media-request.c, remove extra filp argument that
>   was added in v1.
> 
> - SPDX license header, other small changes.
> 
> Open questions:
> 
> - How to tell at complete time whether a request failed? Return error code
>   on release? What's the behaviour with reinit then --- fail on error? Add
>   another IOCTL to ask for completion status?
> 
> 
> Alexandre Courbot (1):
>   videodev2.h: add request_fd field to v4l2_ext_controls
> 
> Hans Verkuil (1):
>   videodev2.h: Add request_fd field to v4l2_buffer
> 
> Laurent Pinchart (1):
>   media: Add request API
> 
> Sakari Ailus (7):
>   media: Support variable size IOCTL arguments
>   staging: media: atomisp: Remove v4l2_buffer.reserved2 field hack
>   vb2: Add support for requests
>   v4l: m2m: Simplify exiting the function in v4l2_m2m_try_schedule
>   v4l: m2m: Support requests with video buffers
>   vim2m: Register V4L2 video device after V4L2 mem2mem init
>   vim2m: Request support
> 
>  drivers/media/Makefile                             |   3 +-
>  drivers/media/common/videobuf2/videobuf2-core.c    |  43 +-
>  drivers/media/common/videobuf2/videobuf2-v4l2.c    |  40 +-
>  drivers/media/media-device.c                       |  80 ++-
>  drivers/media/media-request.c                      | 650 +++++++++++++++++++++
>  drivers/media/platform/vim2m.c                     |  76 ++-
>  drivers/media/usb/cpia2/cpia2_v4l.c                |   2 +-
>  drivers/media/v4l2-core/v4l2-compat-ioctl32.c      |  16 +-
>  drivers/media/v4l2-core/v4l2-ioctl.c               |   6 +-
>  drivers/media/v4l2-core/v4l2-mem2mem.c             | 131 ++++-
>  .../media/atomisp/pci/atomisp2/atomisp_ioctl.c     |  17 +-
>  include/media/media-device.h                       |  19 +-
>  include/media/media-request.h                      | 301 ++++++++++
>  include/media/v4l2-mem2mem.h                       |  28 +
>  include/media/videobuf2-core.h                     |  19 +
>  include/media/videobuf2-v4l2.h                     |  28 +
>  include/uapi/linux/media.h                         |   8 +
>  include/uapi/linux/videodev2.h                     |   6 +-
>  18 files changed, 1408 insertions(+), 65 deletions(-)
>  create mode 100644 drivers/media/media-request.c
>  create mode 100644 include/media/media-request.h
> 

Re: [RFC v2 00/10] Preparing the request API

[Hans Verkuil]

On 03/25/2018 05:12 PM, Hans Verkuil wrote:
> Hi all,
> 
> On 03/23/2018 10:17 PM, Sakari Ailus wrote:
>> Hi folks,
>>
>> This preliminary RFC patchset prepares for the request API. What's new
>> here is support for binding arbitrary configuration or resources to
>> requests.
>>
>> There are a few new concepts here:
>>
>> Class --- a type of configuration or resource a driver (or e.g. the V4L2
>> framework) can attach to a resource. E.g. a video buffer queue would be a
>> class.
>>
>> Object --- an instance of the class. This may be either configuration (in
>> which case the setting will stay until changed, e.g. V4L2 format on a
>> video node) or a resource (such as a video buffer).
>>
>> Reference --- a reference to an object. If a configuration is not changed
>> in a request, instead of allocating a new object, a reference to an
>> existing object is used. This saves time and memory.
>>
>> I expect Laurent to comment on aligning the concept names between the
>> request API and DRM. As far as I understand, the respective DRM names for
>> "class" and "object" used in this set would be "object" and "state".
>>
>> The drivers will need to interact with the requests in three ways:
>>
>> - Allocate new configurations or resources. Drivers are free to store
>>   their own data into request objects as well. These callbacks are
>>   specific to classes.
>>
>> - Validate and queue callbacks. These callbacks are used to try requests
>>   (validate only) as well as queue them (validate and queue). These
>>   callbacks are media device wide, at least for now.
>>
>> The lifetime of the objects related to requests is based on refcounting
>> both requests and request objects. This fits well for existing use cases
>> whether or not based on refcounting; what still needs most of the
>> attention is likely that the number of gets and puts matches once the
>> object is no longer needed.
>>
>> Configuration can be bound to the request the usual way (V4L2 IOCTLs with
>> the request_fd field set to the request). Once queued, request completion
>> is signalled through polling the request file handle (POLLPRI).
>>
>> I'm posting this as an RFC because it's not complete yet. The code
>> compiles but no testing has been done yet.
> 
> Thank you for this patch series. There are some good ideas here, but it is
> quite far from being useful with Alexandre's RFCv4 series.
> 
> So this weekend I worked on a merger of this work and the RFCv4 Request API
> patch series, taking what I think are the best bits of both.
> 
> It is available here:
> 
> https://git.linuxtv.org/hverkuil/media_tree.git/log/?h=reqv6

I reorganized/cleaned up the patch series. So look here instead:

https://git.linuxtv.org/hverkuil/media_tree.git/log/?h=reqv7

It's easier to follow.

Regards,

	Hans

Re: [RFC v2 00/10] Preparing the request API

[Alexandre Courbot]

On Sat, Mar 24, 2018 at 6:17 AM, Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
> Hi folks,
>
> This preliminary RFC patchset prepares for the request API. What's new
> here is support for binding arbitrary configuration or resources to
> requests.
>
> There are a few new concepts here:
>
> Class --- a type of configuration or resource a driver (or e.g. the V4L2
> framework) can attach to a resource. E.g. a video buffer queue would be a
> class.
>
> Object --- an instance of the class. This may be either configuration (in
> which case the setting will stay until changed, e.g. V4L2 format on a
> video node) or a resource (such as a video buffer).
>
> Reference --- a reference to an object. If a configuration is not changed
> in a request, instead of allocating a new object, a reference to an
> existing object is used. This saves time and memory.
>
> I expect Laurent to comment on aligning the concept names between the
> request API and DRM. As far as I understand, the respective DRM names for
> "class" and "object" used in this set would be "object" and "state".
>
> The drivers will need to interact with the requests in three ways:
>
> - Allocate new configurations or resources. Drivers are free to store
>   their own data into request objects as well. These callbacks are
>   specific to classes.
>
> - Validate and queue callbacks. These callbacks are used to try requests
>   (validate only) as well as queue them (validate and queue). These
>   callbacks are media device wide, at least for now.
>
> The lifetime of the objects related to requests is based on refcounting
> both requests and request objects. This fits well for existing use cases
> whether or not based on refcounting; what still needs most of the
> attention is likely that the number of gets and puts matches once the
> object is no longer needed.
>
> Configuration can be bound to the request the usual way (V4L2 IOCTLs with
> the request_fd field set to the request). Once queued, request completion
> is signalled through polling the request file handle (POLLPRI).
>
> I'm posting this as an RFC because it's not complete yet. The code
> compiles but no testing has been done yet.
>
> Todo list:
>
> - Testing! (And fixing the bugs.)
>
> - Request support in a few drivers as well as the control framework.
>
> - Request support for V4L2 formats?

Also, conformance tests in v4l2-compliance.

>
> In the future, support for changing e.g. Media controller link state or
> V4L2 sub-device formats will need to be added. Those should receive more
> attention when the core is in a good shape and the more simple use cases
> are already functional.
>
> Comments and questions are welcome.
>
> since v1:
>
> - Provide an iterator helper for request objects in a request.
>
> - Remove the request lists in the media device (they were not used)
>
> - Move request queing to request fd and add reinit (Alexandre's patchset);
>   this roughly corresponds to Request API RFC v2 from Hans.
>   (MEDIA_IOC_REQUEST_ALLOC argument is a struct pointer instead of an
>   __s32 pointer.)
>
> - Provide a way to unbind request objects from an unqueued request
>   (reinit, closing request fd).
>
> - v4l2-mem2mem + vivid implementation without control support.

Looks like vivid is not here yet, although it doesn't really matter for now.

>
> - More states for requests. In order to take a spinlock (or a mutex) for
>   an extended period of time, add a "QUEUEING" and "REINIT" states.
>
> - Move non-IOCTL code to media-request.c, remove extra filp argument that
>   was added in v1.
>
> - SPDX license header, other small changes.
>
> Open questions:
>
> - How to tell at complete time whether a request failed? Return error code
>   on release? What's the behaviour with reinit then --- fail on error? Add
>   another IOCTL to ask for completion status?

We could look for POLLERR when polling the request in order to know
that something went wrong. When this happens, an extra ioctl on the
request fd (e.g. REQUEST_GET_ERROR) could be used to get more
information about what has failed. Due to the extensible nature of
requests, we should be careful to make this ioctl widely extensible as
well. Anything that is performed in a deferred way can fail (for now,
S_EXT_CTRLS and QBUF), so maybe this ioctl should return errors in the
same format as these calls. The request would be interrupted upon
first error, so we could just return the code of the ioctl that would
have failed + its relevant user-readable state).

>
>
> Alexandre Courbot (1):
>   videodev2.h: add request_fd field to v4l2_ext_controls
>
> Hans Verkuil (1):
>   videodev2.h: Add request_fd field to v4l2_buffer
>
> Laurent Pinchart (1):
>   media: Add request API
>
> Sakari Ailus (7):
>   media: Support variable size IOCTL arguments

I don't think this patch is needed anymore?

I had other comments but saw that Hans respinned the series, so I will
wait for the next iteration. Also for something that has undergone
some testing with controls working.

Re: [RFC v2 06/10] vb2: Add support for requests

[Tomasz Figa]

Hi Sakari,

I would have appreciated being CCed on this series, but anyway, thanks
for sending it. Please see my comments inline.

On Sat, Mar 24, 2018 at 6:17 AM, Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
> Associate a buffer to a request when it is queued and disassociate when it
> is done.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/common/videobuf2/videobuf2-core.c | 43 ++++++++++++++++++++++++-
>  drivers/media/common/videobuf2/videobuf2-v4l2.c | 40 ++++++++++++++++++++++-
>  include/media/videobuf2-core.h                  | 19 +++++++++++
>  include/media/videobuf2-v4l2.h                  | 28 ++++++++++++++++
>  4 files changed, 128 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index d3f7bb3..b8535de 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -346,6 +346,9 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory,
>                         break;
>                 }
>
> +               if (q->class)
> +                       media_request_object_init(q->class, &vb->req_obj);
> +
>                 vb->state = VB2_BUF_STATE_DEQUEUED;
>                 vb->vb2_queue = q;
>                 vb->num_planes = num_planes;
> @@ -520,7 +523,10 @@ static int __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
>         /* Free videobuf buffers */
>         for (buffer = q->num_buffers - buffers; buffer < q->num_buffers;
>              ++buffer) {
> -               kfree(q->bufs[buffer]);
> +               if (q->class)
> +                       media_request_object_put(&q->bufs[buffer]->req_obj);
> +               else
> +                       kfree(q->bufs[buffer]);
>                 q->bufs[buffer] = NULL;
>         }
>
> @@ -944,6 +950,10 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state)
>         default:
>                 /* Inform any processes that may be waiting for buffers */
>                 wake_up(&q->done_wq);
> +               if (vb->req_ref) {
> +                       media_request_ref_complete(vb->req_ref);
> +                       vb->req_ref = NULL;
> +               }
>                 break;
>         }
>  }
> @@ -1249,6 +1259,32 @@ static int __buf_prepare(struct vb2_buffer *vb, const void *pb)
>                 return -EIO;
>         }
>
> +       if (vb->request_fd) {

0 is a perfectly valid FD.

> +               struct media_request *req;
> +               struct media_request_ref *ref;
> +
> +               if (!q->class) {
> +                       dprintk(1, "requests not enabled for the queue\n");
> +                       return -EINVAL;
> +               }
> +
> +               req = media_request_find(q->class->mdev, vb->request_fd);
> +               if (IS_ERR(req)) {
> +                       dprintk(1, "no request found for fd %d (%ld)\n",
> +                               vb->request_fd, PTR_ERR(req));
> +                       return PTR_ERR(req);
> +               }
> +
> +               ref = media_request_object_bind(req,
> +                                               &q->bufs[vb->index]->req_obj);
> +               media_request_put(req);
> +
> +               if (IS_ERR(ref))
> +                       return PTR_ERR(ref);
> +
> +               vb->req_ref = ref;
> +       }
> +

I'm not sure how this would work. The client calling QBUF with request
FD would end up queuing the buffer to the driver, which I'd say isn't
an expected side effect of something that is usually done early as
part of preparing the request.

As we agreed in the UAPI RFC, the buffer should be only prepared and
queued at request queue time and I believe Alex had it implemented
properly in his latest RFC v4. We should reuse that patch instead,
since we spent quite a bit of time to go through all the corner cases
and make sure it works for the most exotic use case we could imagine.

Best regards,
Tomasz

Re: [RFC v2 00/10] Preparing the request API

[Sakari Ailus]

Hi Hans,

On Sun, Mar 25, 2018 at 06:17:30PM +0200, Hans Verkuil wrote:
> On 03/25/2018 05:12 PM, Hans Verkuil wrote:
> > Hi all,
> > 
> > On 03/23/2018 10:17 PM, Sakari Ailus wrote:
> >> Hi folks,
> >>
> >> This preliminary RFC patchset prepares for the request API. What's new
> >> here is support for binding arbitrary configuration or resources to
> >> requests.
> >>
> >> There are a few new concepts here:
> >>
> >> Class --- a type of configuration or resource a driver (or e.g. the V4L2
> >> framework) can attach to a resource. E.g. a video buffer queue would be a
> >> class.
> >>
> >> Object --- an instance of the class. This may be either configuration (in
> >> which case the setting will stay until changed, e.g. V4L2 format on a
> >> video node) or a resource (such as a video buffer).
> >>
> >> Reference --- a reference to an object. If a configuration is not changed
> >> in a request, instead of allocating a new object, a reference to an
> >> existing object is used. This saves time and memory.
> >>
> >> I expect Laurent to comment on aligning the concept names between the
> >> request API and DRM. As far as I understand, the respective DRM names for
> >> "class" and "object" used in this set would be "object" and "state".
> >>
> >> The drivers will need to interact with the requests in three ways:
> >>
> >> - Allocate new configurations or resources. Drivers are free to store
> >>   their own data into request objects as well. These callbacks are
> >>   specific to classes.
> >>
> >> - Validate and queue callbacks. These callbacks are used to try requests
> >>   (validate only) as well as queue them (validate and queue). These
> >>   callbacks are media device wide, at least for now.
> >>
> >> The lifetime of the objects related to requests is based on refcounting
> >> both requests and request objects. This fits well for existing use cases
> >> whether or not based on refcounting; what still needs most of the
> >> attention is likely that the number of gets and puts matches once the
> >> object is no longer needed.
> >>
> >> Configuration can be bound to the request the usual way (V4L2 IOCTLs with
> >> the request_fd field set to the request). Once queued, request completion
> >> is signalled through polling the request file handle (POLLPRI).
> >>
> >> I'm posting this as an RFC because it's not complete yet. The code
> >> compiles but no testing has been done yet.
> > 
> > Thank you for this patch series. There are some good ideas here, but it is
> > quite far from being useful with Alexandre's RFCv4 series.
> > 
> > So this weekend I worked on a merger of this work and the RFCv4 Request API
> > patch series, taking what I think are the best bits of both.
> > 
> > It is available here:
> > 
> > https://git.linuxtv.org/hverkuil/media_tree.git/log/?h=reqv6
> 
> I reorganized/cleaned up the patch series. So look here instead:
> 
> https://git.linuxtv.org/hverkuil/media_tree.git/log/?h=reqv7

I looked at the set an I mostly agree with the changes. There are a few
comments I'd like to make --- and I didn't do a thorough review.

- The purpose of completeable objects is to help the driver to manage
  completing requests. Sometimes this is not self-evident. A request is
  complete when all of its results are available, including buffers and
  controls. The driver does not need to care about this. (I.e. this is not
  the same thing as refcounting.)

- I didn't immediately find object references in the latest set. The
  purpose of the references is to avoid copying objects if they don't
  change from requests to another. It's less time-consuming to allocate a
  new reference (a few pointers) instead of allocating memory for struct
  v4l2_format and copying the data. This starts to really matter when the
  number of objects increase.

  Then again, I wasn't very happy with videobufs having to refer
  themselves; perhaps we could limit referring to configuration objects
  (vs. resource objects; this is what my last patchset referred to as
  sticky; perhaps not a lasting name nor necessarily intended as such)
  while putting resource objects to the request itself.

  Controls would be a prime candidate for this if the control framework can
  be made to fit this model. I'm fine adding this later on, or another
  solution that avoids copying all unchanged configuration around for every
  request. But I want the need to be recognised so it won't come as a
  surprise to anyone later on.

-- 
Kind regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [RFC v2 00/10] Preparing the request API

[Hans Verkuil]

On 03/26/2018 09:58 AM, Sakari Ailus wrote:
> Hi Hans,
> 
> On Sun, Mar 25, 2018 at 06:17:30PM +0200, Hans Verkuil wrote:
>> On 03/25/2018 05:12 PM, Hans Verkuil wrote:
>>> Hi all,
>>>
>>> On 03/23/2018 10:17 PM, Sakari Ailus wrote:
>>>> Hi folks,
>>>>
>>>> This preliminary RFC patchset prepares for the request API. What's new
>>>> here is support for binding arbitrary configuration or resources to
>>>> requests.
>>>>
>>>> There are a few new concepts here:
>>>>
>>>> Class --- a type of configuration or resource a driver (or e.g. the V4L2
>>>> framework) can attach to a resource. E.g. a video buffer queue would be a
>>>> class.
>>>>
>>>> Object --- an instance of the class. This may be either configuration (in
>>>> which case the setting will stay until changed, e.g. V4L2 format on a
>>>> video node) or a resource (such as a video buffer).
>>>>
>>>> Reference --- a reference to an object. If a configuration is not changed
>>>> in a request, instead of allocating a new object, a reference to an
>>>> existing object is used. This saves time and memory.
>>>>
>>>> I expect Laurent to comment on aligning the concept names between the
>>>> request API and DRM. As far as I understand, the respective DRM names for
>>>> "class" and "object" used in this set would be "object" and "state".
>>>>
>>>> The drivers will need to interact with the requests in three ways:
>>>>
>>>> - Allocate new configurations or resources. Drivers are free to store
>>>>   their own data into request objects as well. These callbacks are
>>>>   specific to classes.
>>>>
>>>> - Validate and queue callbacks. These callbacks are used to try requests
>>>>   (validate only) as well as queue them (validate and queue). These
>>>>   callbacks are media device wide, at least for now.
>>>>
>>>> The lifetime of the objects related to requests is based on refcounting
>>>> both requests and request objects. This fits well for existing use cases
>>>> whether or not based on refcounting; what still needs most of the
>>>> attention is likely that the number of gets and puts matches once the
>>>> object is no longer needed.
>>>>
>>>> Configuration can be bound to the request the usual way (V4L2 IOCTLs with
>>>> the request_fd field set to the request). Once queued, request completion
>>>> is signalled through polling the request file handle (POLLPRI).
>>>>
>>>> I'm posting this as an RFC because it's not complete yet. The code
>>>> compiles but no testing has been done yet.
>>>
>>> Thank you for this patch series. There are some good ideas here, but it is
>>> quite far from being useful with Alexandre's RFCv4 series.
>>>
>>> So this weekend I worked on a merger of this work and the RFCv4 Request API
>>> patch series, taking what I think are the best bits of both.
>>>
>>> It is available here:
>>>
>>> https://git.linuxtv.org/hverkuil/media_tree.git/log/?h=reqv6
>>
>> I reorganized/cleaned up the patch series. So look here instead:
>>
>> https://git.linuxtv.org/hverkuil/media_tree.git/log/?h=reqv7
> 
> I looked at the set an I mostly agree with the changes. There are a few
> comments I'd like to make --- and I didn't do a thorough review.
> 
> - The purpose of completeable objects is to help the driver to manage
>   completing requests. Sometimes this is not self-evident. A request is
>   complete when all of its results are available, including buffers and
>   controls. The driver does not need to care about this. (I.e. this is not
>   the same thing as refcounting.)

I must be missing something. Is there something in my series that conflicts
with this?

> - I didn't immediately find object references in the latest set. The
>   purpose of the references is to avoid copying objects if they don't
>   change from requests to another. It's less time-consuming to allocate a
>   new reference (a few pointers) instead of allocating memory for struct
>   v4l2_format and copying the data. This starts to really matter when the
>   number of objects increase.

At this point in time it is not relevant for vb2 buffers and control
handlers. This might change for control handlers, but probably not since
they can use v4l2_ctrl_ref references internally (I don't do that yet, but
that will change).

> 
>   Then again, I wasn't very happy with videobufs having to refer
>   themselves; perhaps we could limit referring to configuration objects
>   (vs. resource objects; this is what my last patchset referred to as
>   sticky; perhaps not a lasting name nor necessarily intended as such)
>   while putting resource objects to the request itself.
> 
>   Controls would be a prime candidate for this if the control framework can
>   be made to fit this model. I'm fine adding this later on, or another
>   solution that avoids copying all unchanged configuration around for every
>   request. But I want the need to be recognised so it won't come as a
>   surprise to anyone later on.
> 

I removed it because as far as I can see this is not needed for the initial
codec support. Whether or not it is needed for camera pipelines is something
that can be discussed when support for that is added. I have to see it in
context first.

Adding features without having drivers that use it is never a good idea...

Regards,

	Hans

Re: [RFC v2 00/10] Preparing the request API

[Sakari Ailus]

On Mon, Mar 26, 2018 at 10:31:50AM +0200, Hans Verkuil wrote:
> On 03/26/2018 09:58 AM, Sakari Ailus wrote:
> > Hi Hans,
> > 
> > On Sun, Mar 25, 2018 at 06:17:30PM +0200, Hans Verkuil wrote:
> >> On 03/25/2018 05:12 PM, Hans Verkuil wrote:
> >>> Hi all,
> >>>
> >>> On 03/23/2018 10:17 PM, Sakari Ailus wrote:
> >>>> Hi folks,
> >>>>
> >>>> This preliminary RFC patchset prepares for the request API. What's new
> >>>> here is support for binding arbitrary configuration or resources to
> >>>> requests.
> >>>>
> >>>> There are a few new concepts here:
> >>>>
> >>>> Class --- a type of configuration or resource a driver (or e.g. the V4L2
> >>>> framework) can attach to a resource. E.g. a video buffer queue would be a
> >>>> class.
> >>>>
> >>>> Object --- an instance of the class. This may be either configuration (in
> >>>> which case the setting will stay until changed, e.g. V4L2 format on a
> >>>> video node) or a resource (such as a video buffer).
> >>>>
> >>>> Reference --- a reference to an object. If a configuration is not changed
> >>>> in a request, instead of allocating a new object, a reference to an
> >>>> existing object is used. This saves time and memory.
> >>>>
> >>>> I expect Laurent to comment on aligning the concept names between the
> >>>> request API and DRM. As far as I understand, the respective DRM names for
> >>>> "class" and "object" used in this set would be "object" and "state".
> >>>>
> >>>> The drivers will need to interact with the requests in three ways:
> >>>>
> >>>> - Allocate new configurations or resources. Drivers are free to store
> >>>>   their own data into request objects as well. These callbacks are
> >>>>   specific to classes.
> >>>>
> >>>> - Validate and queue callbacks. These callbacks are used to try requests
> >>>>   (validate only) as well as queue them (validate and queue). These
> >>>>   callbacks are media device wide, at least for now.
> >>>>
> >>>> The lifetime of the objects related to requests is based on refcounting
> >>>> both requests and request objects. This fits well for existing use cases
> >>>> whether or not based on refcounting; what still needs most of the
> >>>> attention is likely that the number of gets and puts matches once the
> >>>> object is no longer needed.
> >>>>
> >>>> Configuration can be bound to the request the usual way (V4L2 IOCTLs with
> >>>> the request_fd field set to the request). Once queued, request completion
> >>>> is signalled through polling the request file handle (POLLPRI).
> >>>>
> >>>> I'm posting this as an RFC because it's not complete yet. The code
> >>>> compiles but no testing has been done yet.
> >>>
> >>> Thank you for this patch series. There are some good ideas here, but it is
> >>> quite far from being useful with Alexandre's RFCv4 series.
> >>>
> >>> So this weekend I worked on a merger of this work and the RFCv4 Request API
> >>> patch series, taking what I think are the best bits of both.
> >>>
> >>> It is available here:
> >>>
> >>> https://git.linuxtv.org/hverkuil/media_tree.git/log/?h=reqv6
> >>
> >> I reorganized/cleaned up the patch series. So look here instead:
> >>
> >> https://git.linuxtv.org/hverkuil/media_tree.git/log/?h=reqv7
> > 
> > I looked at the set an I mostly agree with the changes. There are a few
> > comments I'd like to make --- and I didn't do a thorough review.
> > 
> > - The purpose of completeable objects is to help the driver to manage
> >   completing requests. Sometimes this is not self-evident. A request is
> >   complete when all of its results are available, including buffers and
> >   controls. The driver does not need to care about this. (I.e. this is not
> >   the same thing as refcounting.)
> 
> I must be missing something. Is there something in my series that conflicts
> with this?

Sort of. media_request_object_release() will complete objects but as the
request holds a reference to every object in it, this will lead to e.g.
video buffers to be returned to the user's control only when the request
file handle is closed. The driver needs to unbind them; that's what the
unbind callback was for. I wouldn't want to make the driver responsible for
which buffers related to a request were still queued and to return them to
the user.

> 
> > - I didn't immediately find object references in the latest set. The
> >   purpose of the references is to avoid copying objects if they don't
> >   change from requests to another. It's less time-consuming to allocate a
> >   new reference (a few pointers) instead of allocating memory for struct
> >   v4l2_format and copying the data. This starts to really matter when the
> >   number of objects increase.
> 
> At this point in time it is not relevant for vb2 buffers and control
> handlers. This might change for control handlers, but probably not since
> they can use v4l2_ctrl_ref references internally (I don't do that yet, but
> that will change).
> 
> > 
> >   Then again, I wasn't very happy with videobufs having to refer
> >   themselves; perhaps we could limit referring to configuration objects
> >   (vs. resource objects; this is what my last patchset referred to as
> >   sticky; perhaps not a lasting name nor necessarily intended as such)
> >   while putting resource objects to the request itself.
> > 
> >   Controls would be a prime candidate for this if the control framework can
> >   be made to fit this model. I'm fine adding this later on, or another
> >   solution that avoids copying all unchanged configuration around for every
> >   request. But I want the need to be recognised so it won't come as a
> >   surprise to anyone later on.
> > 
> 
> I removed it because as far as I can see this is not needed for the initial
> codec support. Whether or not it is needed for camera pipelines is something
> that can be discussed when support for that is added. I have to see it in
> context first.
> 
> Adding features without having drivers that use it is never a good idea...

It's not really related to camera pipelines but everything which does not
have a resource (such as video buffers) related to it. When applying a
request it's also very helpful to know if a configuration changed from what
it was previously. But I agree on the schedule, this could be done later
on.

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

Re: [RFC v2 06/10] vb2: Add support for requests

[Sakari Ailus]

On Mon, Mar 26, 2018 at 03:02:53PM +0900, Tomasz Figa wrote:
> Hi Sakari,
> 
> I would have appreciated being CCed on this series, but anyway, thanks
> for sending it. Please see my comments inline.
> 
> On Sat, Mar 24, 2018 at 6:17 AM, Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> > Associate a buffer to a request when it is queued and disassociate when it
> > is done.
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  drivers/media/common/videobuf2/videobuf2-core.c | 43 ++++++++++++++++++++++++-
> >  drivers/media/common/videobuf2/videobuf2-v4l2.c | 40 ++++++++++++++++++++++-
> >  include/media/videobuf2-core.h                  | 19 +++++++++++
> >  include/media/videobuf2-v4l2.h                  | 28 ++++++++++++++++
> >  4 files changed, 128 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> > index d3f7bb3..b8535de 100644
> > --- a/drivers/media/common/videobuf2/videobuf2-core.c
> > +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> > @@ -346,6 +346,9 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory,
> >                         break;
> >                 }
> >
> > +               if (q->class)
> > +                       media_request_object_init(q->class, &vb->req_obj);
> > +
> >                 vb->state = VB2_BUF_STATE_DEQUEUED;
> >                 vb->vb2_queue = q;
> >                 vb->num_planes = num_planes;
> > @@ -520,7 +523,10 @@ static int __vb2_queue_free(struct vb2_queue *q, unsigned int buffers)
> >         /* Free videobuf buffers */
> >         for (buffer = q->num_buffers - buffers; buffer < q->num_buffers;
> >              ++buffer) {
> > -               kfree(q->bufs[buffer]);
> > +               if (q->class)
> > +                       media_request_object_put(&q->bufs[buffer]->req_obj);
> > +               else
> > +                       kfree(q->bufs[buffer]);
> >                 q->bufs[buffer] = NULL;
> >         }
> >
> > @@ -944,6 +950,10 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state)
> >         default:
> >                 /* Inform any processes that may be waiting for buffers */
> >                 wake_up(&q->done_wq);
> > +               if (vb->req_ref) {
> > +                       media_request_ref_complete(vb->req_ref);
> > +                       vb->req_ref = NULL;
> > +               }
> >                 break;
> >         }
> >  }
> > @@ -1249,6 +1259,32 @@ static int __buf_prepare(struct vb2_buffer *vb, const void *pb)
> >                 return -EIO;
> >         }
> >
> > +       if (vb->request_fd) {
> 
> 0 is a perfectly valid FD.

Agreed. This part was written before Hans's RFC albeit sent afterwards.

> 
> > +               struct media_request *req;
> > +               struct media_request_ref *ref;
> > +
> > +               if (!q->class) {
> > +                       dprintk(1, "requests not enabled for the queue\n");
> > +                       return -EINVAL;
> > +               }
> > +
> > +               req = media_request_find(q->class->mdev, vb->request_fd);
> > +               if (IS_ERR(req)) {
> > +                       dprintk(1, "no request found for fd %d (%ld)\n",
> > +                               vb->request_fd, PTR_ERR(req));
> > +                       return PTR_ERR(req);
> > +               }
> > +
> > +               ref = media_request_object_bind(req,
> > +                                               &q->bufs[vb->index]->req_obj);
> > +               media_request_put(req);
> > +
> > +               if (IS_ERR(ref))
> > +                       return PTR_ERR(ref);
> > +
> > +               vb->req_ref = ref;
> > +       }
> > +
> 
> I'm not sure how this would work. The client calling QBUF with request
> FD would end up queuing the buffer to the driver, which I'd say isn't
> an expected side effect of something that is usually done early as
> part of preparing the request.
> 
> As we agreed in the UAPI RFC, the buffer should be only prepared and
> queued at request queue time and I believe Alex had it implemented
> properly in his latest RFC v4. We should reuse that patch instead,
> since we spent quite a bit of time to go through all the corner cases
> and make sure it works for the most exotic use case we could imagine.

Yes, we need more than the above patch. No disagreement there.

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

Re: [RFC v2 01/10] media: Support variable size IOCTL arguments

[Mauro Carvalho Chehab]

Em Fri, 23 Mar 2018 23:17:35 +0200
Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:

> Maintain a list of supported IOCTL argument sizes and allow only those in
> the list.
> 
> As an additional bonus, IOCTL handlers will be able to check whether the
> caller actually set (using the argument size) the field vs. assigning it
> to zero. Separate macro can be provided for that.
> 
> This will be easier for applications as well since there is no longer the
> problem of setting the reserved fields zero, or at least it is a lesser
> problem.

Patch makes sense to me, but I have a few comments on it.

> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/media/media-device.c | 65 ++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 59 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index 35e81f7..da63da1 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -387,22 +387,36 @@ static long copy_arg_to_user(void __user *uarg, void *karg, unsigned int cmd)
>  /* Do acquire the graph mutex */
>  #define MEDIA_IOC_FL_GRAPH_MUTEX	BIT(0)
>  
> -#define MEDIA_IOC_ARG(__cmd, func, fl, from_user, to_user)		\
> +#define MEDIA_IOC_SZ_ARG(__cmd, func, fl, alt_sz, from_user, to_user)	\
>  	[_IOC_NR(MEDIA_IOC_##__cmd)] = {				\
>  		.cmd = MEDIA_IOC_##__cmd,				\
>  		.fn = (long (*)(struct media_device *, void *))func,	\
>  		.flags = fl,						\
> +		.alt_arg_sizes = alt_sz,				\
>  		.arg_from_user = from_user,				\
>  		.arg_to_user = to_user,					\
>  	}
>  
> -#define MEDIA_IOC(__cmd, func, fl)					\
> -	MEDIA_IOC_ARG(__cmd, func, fl, copy_arg_from_user, copy_arg_to_user)
> +#define MEDIA_IOC_ARG(__cmd, func, fl, from_user, to_user)		\
> +	MEDIA_IOC_SZ_ARG(__cmd, func, fl, NULL, from_user, to_user)
> +
> +#define MEDIA_IOC_SZ(__cmd, func, fl, alt_sz)			\
> +	MEDIA_IOC_SZ_ARG(__cmd, func, fl, alt_sz,		\
> +			 copy_arg_from_user, copy_arg_to_user)
> +
> +#define MEDIA_IOC(__cmd, func, fl)				\
> +	MEDIA_IOC_ARG(__cmd, func, fl,				\
> +		      copy_arg_from_user, copy_arg_to_user)

Please add some comments to those macros (specially the first one,
as the names of the values are too short to help identifying what
they are.

>  
>  /* the table is indexed by _IOC_NR(cmd) */
>  struct media_ioctl_info {
>  	unsigned int cmd;
>  	unsigned short flags;
> +	/*
> +	 * Sizes of the alternative arguments. If there are none, this
> +	 * pointer is NULL.
> +	 */
> +	const unsigned short *alt_arg_sizes;
>  	long (*fn)(struct media_device *dev, void *arg);
>  	long (*arg_from_user)(void *karg, void __user *uarg, unsigned int cmd);
>  	long (*arg_to_user)(void __user *uarg, void *karg, unsigned int cmd);
> @@ -416,6 +430,42 @@ static const struct media_ioctl_info ioctl_info[] = {
>  	MEDIA_IOC(G_TOPOLOGY, media_device_get_topology, MEDIA_IOC_FL_GRAPH_MUTEX),
>  };
>  
> +#define MASK_IOC_SIZE(cmd) \
> +	((cmd) & ~(_IOC_SIZEMASK << _IOC_SIZESHIFT))

This should be, instead at:
	include/uapi/asm-generic/ioctl.h

The patch series adding it there should also touch the other usecases
for _IOC_SIZEMASK (evdev.c, phantom.c, v4l2-ioctl.c).

> +
> +static inline long is_valid_ioctl(unsigned int cmd)

Please rename from "cmd" to "user_cmd", in order to make it
clearer that it contains the value passed by userspace.

> +{
> +	const struct media_ioctl_info *info = ioctl_info;
> +	const unsigned short *alt_arg_sizes;
> +
> +	if (_IOC_NR(cmd) >= ARRAY_SIZE(ioctl_info))
> +		return -ENOIOCTLCMD;
> +
> +	info += _IOC_NR(cmd);
> +
> +	if (info->cmd == cmd)
> +		return 0;

Please revert it:
	if (user_cmd == info->cmd)
		return 0

As we're validating if the userspace ioctl code makes sense,
and not the reverse.

Please add the check if alt_arg_sizes is defined here:

	alt_arg_sizes = info->alt_arg_sizes;
	if (!alt_arg_sizes)
		return -ENOIOCTLCMD;

As the remaining code is not needed if user_cmd != info_cmd.

> +
> +	/*
> +	 * Verify that the size-dependent patch of the IOCTL command
> +	 * matches and that the size does not exceed the principal
> +	 * argument size.
> +	 */

what do you mean by "principal argument size"? I guess what you're
meaning here is the "argument size of the latest version" with is
always bigger than the previous version. If so, make it clear.

I would write it as something like:

	/*
	 * As the ioctl passed by userspace doesn't match the current
	 * one, and there are alternate sizes for thsi ioctl, 
	 * we need to check if the ioctl code is correct.
	 *
	 * Validate that the ioctl code passed by userspace matches the
	 * Kernel definition with regards to its number, type and dir.
	 * Also checks if the size is not bigger than the one defined
	 * by the latest version of the ioctl.
	 */

> +	if (MASK_IOC_SIZE(info->cmd) != MASK_IOC_SIZE(cmd)
> +	    || _IOC_SIZE(info->cmd) < _IOC_SIZE(cmd))
> +		return -ENOIOCTLCMD;

I would invert the check: what we want do to is to check if whatever
userspace passes is valid. So, better to place user_cmd as the first
arguments at the check, e. g.: 

	if (MASK_IOC_SIZE(user_cmd) != MASK_IOC_SIZE(info->cmd)
	    ||  _IOC_SIZE(user_cmd) > _IOC_SIZE(info->cmd))
		return -ENOIOCTLCMD;

> +
> +	alt_arg_sizes = info->alt_arg_sizes;
> +	if (!alt_arg_sizes)
> +		return -ENOIOCTLCMD;

As said before, this check should happen earlier.

> +
> +	for (; *alt_arg_sizes; alt_arg_sizes++)
> +		if (_IOC_SIZE(cmd) == *alt_arg_sizes)
> +			return 0;
> +
> +	return -ENOIOCTLCMD;
> +}
> +
>  static long media_device_ioctl(struct file *filp, unsigned int cmd,
>  			       unsigned long __arg)
>  {
> @@ -426,9 +476,9 @@ static long media_device_ioctl(struct file *filp, unsigned int cmd,
>  	char __karg[256], *karg = __karg;
>  	long ret;
>  
> -	if (_IOC_NR(cmd) >= ARRAY_SIZE(ioctl_info)
> -	    || ioctl_info[_IOC_NR(cmd)].cmd != cmd)
> -		return -ENOIOCTLCMD;
> +	ret = is_valid_ioctl(cmd);
> +	if (ret)
> +		return ret;
>  
>  	info = &ioctl_info[_IOC_NR(cmd)];
>  
> @@ -444,6 +494,9 @@ static long media_device_ioctl(struct file *filp, unsigned int cmd,
>  			goto out_free;
>  	}
>  
> +	/* Set the rest of the argument struct to zero */
> +	memset(karg + _IOC_SIZE(cmd), 0, _IOC_SIZE(info->cmd) - _IOC_SIZE(cmd));
> +
>  	if (info->flags & MEDIA_IOC_FL_GRAPH_MUTEX)
>  		mutex_lock(&dev->graph_mutex);
>  



Thanks,
Mauro

Re: [RFC v2 01/10] media: Support variable size IOCTL arguments

[Sakari Ailus]

Hi Mauro,

On Mon, Mar 26, 2018 at 07:47:42AM -0300, Mauro Carvalho Chehab wrote:
> Em Fri, 23 Mar 2018 23:17:35 +0200
> Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
> 
> > Maintain a list of supported IOCTL argument sizes and allow only those in
> > the list.
> > 
> > As an additional bonus, IOCTL handlers will be able to check whether the
> > caller actually set (using the argument size) the field vs. assigning it
> > to zero. Separate macro can be provided for that.
> > 
> > This will be easier for applications as well since there is no longer the
> > problem of setting the reserved fields zero, or at least it is a lesser
> > problem.
> 
> Patch makes sense to me, but I have a few comments on it.
> 
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
> > ---
> >  drivers/media/media-device.c | 65 ++++++++++++++++++++++++++++++++++++++++----
> >  1 file changed, 59 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> > index 35e81f7..da63da1 100644
> > --- a/drivers/media/media-device.c
> > +++ b/drivers/media/media-device.c
> > @@ -387,22 +387,36 @@ static long copy_arg_to_user(void __user *uarg, void *karg, unsigned int cmd)
> >  /* Do acquire the graph mutex */
> >  #define MEDIA_IOC_FL_GRAPH_MUTEX	BIT(0)
> >  
> > -#define MEDIA_IOC_ARG(__cmd, func, fl, from_user, to_user)		\
> > +#define MEDIA_IOC_SZ_ARG(__cmd, func, fl, alt_sz, from_user, to_user)	\
> >  	[_IOC_NR(MEDIA_IOC_##__cmd)] = {				\
> >  		.cmd = MEDIA_IOC_##__cmd,				\
> >  		.fn = (long (*)(struct media_device *, void *))func,	\
> >  		.flags = fl,						\
> > +		.alt_arg_sizes = alt_sz,				\
> >  		.arg_from_user = from_user,				\
> >  		.arg_to_user = to_user,					\
> >  	}
> >  
> > -#define MEDIA_IOC(__cmd, func, fl)					\
> > -	MEDIA_IOC_ARG(__cmd, func, fl, copy_arg_from_user, copy_arg_to_user)
> > +#define MEDIA_IOC_ARG(__cmd, func, fl, from_user, to_user)		\
> > +	MEDIA_IOC_SZ_ARG(__cmd, func, fl, NULL, from_user, to_user)
> > +
> > +#define MEDIA_IOC_SZ(__cmd, func, fl, alt_sz)			\
> > +	MEDIA_IOC_SZ_ARG(__cmd, func, fl, alt_sz,		\
> > +			 copy_arg_from_user, copy_arg_to_user)
> > +
> > +#define MEDIA_IOC(__cmd, func, fl)				\
> > +	MEDIA_IOC_ARG(__cmd, func, fl,				\
> > +		      copy_arg_from_user, copy_arg_to_user)
> 
> Please add some comments to those macros (specially the first one,
> as the names of the values are too short to help identifying what
> they are.

Ack.

> 
> >  
> >  /* the table is indexed by _IOC_NR(cmd) */
> >  struct media_ioctl_info {
> >  	unsigned int cmd;
> >  	unsigned short flags;
> > +	/*
> > +	 * Sizes of the alternative arguments. If there are none, this
> > +	 * pointer is NULL.
> > +	 */
> > +	const unsigned short *alt_arg_sizes;
> >  	long (*fn)(struct media_device *dev, void *arg);
> >  	long (*arg_from_user)(void *karg, void __user *uarg, unsigned int cmd);
> >  	long (*arg_to_user)(void __user *uarg, void *karg, unsigned int cmd);
> > @@ -416,6 +430,42 @@ static const struct media_ioctl_info ioctl_info[] = {
> >  	MEDIA_IOC(G_TOPOLOGY, media_device_get_topology, MEDIA_IOC_FL_GRAPH_MUTEX),
> >  };
> >  
> > +#define MASK_IOC_SIZE(cmd) \
> > +	((cmd) & ~(_IOC_SIZEMASK << _IOC_SIZESHIFT))
> 
> This should be, instead at:
> 	include/uapi/asm-generic/ioctl.h
> 
> The patch series adding it there should also touch the other usecases
> for _IOC_SIZEMASK (evdev.c, phantom.c, v4l2-ioctl.c).

It seems there's already IOCSIZE_MASK which is already used elsewhere in
the kernel:

#define IOCSIZE_MASK    (_IOC_SIZEMASK << _IOC_SIZESHIFT)

I'll switch to that instead.

> 
> > +
> > +static inline long is_valid_ioctl(unsigned int cmd)
> 
> Please rename from "cmd" to "user_cmd", in order to make it
> clearer that it contains the value passed by userspace.

Done.

> 
> > +{
> > +	const struct media_ioctl_info *info = ioctl_info;
> > +	const unsigned short *alt_arg_sizes;
> > +
> > +	if (_IOC_NR(cmd) >= ARRAY_SIZE(ioctl_info))
> > +		return -ENOIOCTLCMD;
> > +
> > +	info += _IOC_NR(cmd);
> > +
> > +	if (info->cmd == cmd)
> > +		return 0;
> 
> Please revert it:
> 	if (user_cmd == info->cmd)
> 		return 0

Agreed.

> 
> As we're validating if the userspace ioctl code makes sense,
> and not the reverse.
> 
> Please add the check if alt_arg_sizes is defined here:
> 
> 	alt_arg_sizes = info->alt_arg_sizes;
> 	if (!alt_arg_sizes)
> 		return -ENOIOCTLCMD;

I'll move the assignment to the beginning of the loop at the same time to
make the code cleaner.

> 
> As the remaining code is not needed if user_cmd != info_cmd.
> 
> > +
> > +	/*
> > +	 * Verify that the size-dependent patch of the IOCTL command
> > +	 * matches and that the size does not exceed the principal
> > +	 * argument size.
> > +	 */
> 
> what do you mean by "principal argument size"? I guess what you're
> meaning here is the "argument size of the latest version" with is
> always bigger than the previous version. If so, make it clear.

Correct.

> 
> I would write it as something like:
> 
> 	/*
> 	 * As the ioctl passed by userspace doesn't match the current
> 	 * one, and there are alternate sizes for thsi ioctl, 
> 	 * we need to check if the ioctl code is correct.
> 	 *
> 	 * Validate that the ioctl code passed by userspace matches the
> 	 * Kernel definition with regards to its number, type and dir.
> 	 * Also checks if the size is not bigger than the one defined
> 	 * by the latest version of the ioctl.

The number has been already validated earlier. I think this is a quote
thorough explanation of what is not really that complicated. What would you
think of the following?

	/*
	 * Variable size IOCTL argument support allows using either the latest
	 * variant of the IOCTL argument struct or an earlier version. Check
	 * that the size-independent portions of the IOCTL command match and
	 * that the size matches with one of the alternative sizes that
	 * represent earlier revisions of the argument struct.
	 */

> 	 */
> 
> > +	if (MASK_IOC_SIZE(info->cmd) != MASK_IOC_SIZE(cmd)
> > +	    || _IOC_SIZE(info->cmd) < _IOC_SIZE(cmd))
> > +		return -ENOIOCTLCMD;
> 
> I would invert the check: what we want do to is to check if whatever
> userspace passes is valid. So, better to place user_cmd as the first
> arguments at the check, e. g.: 
> 
> 	if (MASK_IOC_SIZE(user_cmd) != MASK_IOC_SIZE(info->cmd)
> 	    ||  _IOC_SIZE(user_cmd) > _IOC_SIZE(info->cmd))
> 		return -ENOIOCTLCMD;

Agreed.

> 
> > +
> > +	alt_arg_sizes = info->alt_arg_sizes;
> > +	if (!alt_arg_sizes)
> > +		return -ENOIOCTLCMD;
> 
> As said before, this check should happen earlier.
> 
> > +
> > +	for (; *alt_arg_sizes; alt_arg_sizes++)
> > +		if (_IOC_SIZE(cmd) == *alt_arg_sizes)
> > +			return 0;
> > +
> > +	return -ENOIOCTLCMD;
> > +}
> > +
> >  static long media_device_ioctl(struct file *filp, unsigned int cmd,
> >  			       unsigned long __arg)
> >  {
> > @@ -426,9 +476,9 @@ static long media_device_ioctl(struct file *filp, unsigned int cmd,
> >  	char __karg[256], *karg = __karg;
> >  	long ret;
> >  
> > -	if (_IOC_NR(cmd) >= ARRAY_SIZE(ioctl_info)
> > -	    || ioctl_info[_IOC_NR(cmd)].cmd != cmd)
> > -		return -ENOIOCTLCMD;
> > +	ret = is_valid_ioctl(cmd);
> > +	if (ret)
> > +		return ret;
> >  
> >  	info = &ioctl_info[_IOC_NR(cmd)];
> >  
> > @@ -444,6 +494,9 @@ static long media_device_ioctl(struct file *filp, unsigned int cmd,
> >  			goto out_free;
> >  	}
> >  
> > +	/* Set the rest of the argument struct to zero */
> > +	memset(karg + _IOC_SIZE(cmd), 0, _IOC_SIZE(info->cmd) - _IOC_SIZE(cmd));
> > +
> >  	if (info->flags & MEDIA_IOC_FL_GRAPH_MUTEX)
> >  		mutex_lock(&dev->graph_mutex);
> >  
> 
> 
> 

After the changes I've done, the function would look like this. I also
added a comment on the check for alt_arg_sizes.

static inline long is_valid_ioctl(unsigned int user_cmd)
{
	const struct media_ioctl_info *info = ioctl_info;
	const unsigned short *alt_arg_sizes;

	if (_IOC_NR(user_cmd) >= ARRAY_SIZE(ioctl_info))
		return -ENOIOCTLCMD;

	info += _IOC_NR(user_cmd);

	if (user_cmd == info->cmd)
		return 0;

	/*
	 * There was no exact match between the user-passed IOCTL command and
	 * the definition. Are there earlier revisions of the argument struct
	 * available?
	 */
	if (!info->alt_arg_sizes)
		return -ENOIOCTLCMD;

	/*
	 * Variable size IOCTL argument support allows using either the latest
	 * revision of the IOCTL argument struct or an earlier version. Check
	 * that the size-independent portions of the IOCTL command match and
	 * that the size matches with one of the alternative sizes that
	 * represent earlier revisions of the argument struct.
	 */
	if (user_cmd & ~IOCSIZE_MASK != info->cmd & ~IOCSIZE_MASK)
	    || _IOC_SIZE(user_cmd) < _IOC_SIZE(info->cmd))
		return -ENOIOCTLCMD;

	for (alt_arg_sizes = info->alt_arg_sizes; *alt_arg_sizes;
	     alt_arg_sizes++)
		if (_IOC_SIZE(user_cmd) == *alt_arg_sizes)
			return 0;

	return -ENOIOCTLCMD;
}


-- 
Kind regards,

Sakari Ailus
sakari.ailus@linux.intel.com

[RFC v2.1 1/1] media: Support variable size IOCTL arguments

[Sakari Ailus]

Maintain a list of supported IOCTL argument sizes and allow only those in
the list.

As an additional bonus, IOCTL handlers will be able to check whether the
caller actually set (using the argument size) the field vs. assigning it
to zero. Separate macro can be provided for that.

This will be easier for applications as well since there is no longer the
problem of setting the reserved fields zero, or at least it is a lesser
problem.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
---
Hi folks,

I've essentially addressed Mauro's comments on v2.

The code is only compile tested so far but the changes from the last
tested version are not that big. There's still some uncertainty though.

since v2:

- Rework is_valid_ioctl based on the comments

	- Improved comments,
	
	- Rename cmd as user_cmd, as this comes from the user
	
	- Check whether there are alternative argument sizes before any
	  checks on IOCTL command if there is no exact match
	  
	- Use IOCSIZE_MASK macro instead of creating our own

- Add documentation for macros declaring IOCTLs


 drivers/media/media-device.c | 98 +++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 92 insertions(+), 6 deletions(-)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 35e81f7..279d740 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -387,22 +387,65 @@ static long copy_arg_to_user(void __user *uarg, void *karg, unsigned int cmd)
 /* Do acquire the graph mutex */
 #define MEDIA_IOC_FL_GRAPH_MUTEX	BIT(0)
 
-#define MEDIA_IOC_ARG(__cmd, func, fl, from_user, to_user)		\
+/**
+ * MEDIA_IOC_SZ_ARG - Declare a Media device IOCTL with alternative size and
+ *		      to_user/from_user callbacks
+ *
+ * @__cmd:	The IOCTL command suffix (without "MEDIA_IOC_")
+ * @func:	The handler function
+ * @fl:		Flags from @enum media_ioc_flags
+ * @alt_sz:	A 0-terminated list of alternative argument struct sizes.
+ * @from_user:	Function to copy argument struct from the user to the kernel
+ * @to_user:	Function to copy argument struct to the user from the kernel
+ */
+#define MEDIA_IOC_SZ_ARG(__cmd, func, fl, alt_sz, from_user, to_user)	\
 	[_IOC_NR(MEDIA_IOC_##__cmd)] = {				\
 		.cmd = MEDIA_IOC_##__cmd,				\
 		.fn = (long (*)(struct media_device *, void *))func,	\
 		.flags = fl,						\
+		.alt_arg_sizes = alt_sz,				\
 		.arg_from_user = from_user,				\
 		.arg_to_user = to_user,					\
 	}
 
-#define MEDIA_IOC(__cmd, func, fl)					\
-	MEDIA_IOC_ARG(__cmd, func, fl, copy_arg_from_user, copy_arg_to_user)
+/**
+ * MEDIA_IOC_ARG - Declare a Media device IOCTL with to_user/from_user callbacks
+ *
+ * Just as MEDIA_IOC_SZ_ARG but without the alternative size list.
+ */
+#define MEDIA_IOC_ARG(__cmd, func, fl, from_user, to_user)		\
+	MEDIA_IOC_SZ_ARG(__cmd, func, fl, NULL, from_user, to_user)
+
+/**
+ * MEDIA_IOC_ARG - Declare a Media device IOCTL with alternative argument struct
+ *		   sizes
+ *
+ * Just as MEDIA_IOC_SZ_ARG but without the callbacks to copy the data from the
+ * user space and back to user space.
+ */
+#define MEDIA_IOC_SZ(__cmd, func, fl, alt_sz)			\
+	MEDIA_IOC_SZ_ARG(__cmd, func, fl, alt_sz,		\
+			 copy_arg_from_user, copy_arg_to_user)
+
+/**
+ * MEDIA_IOC_ARG - Declare a Media device IOCTL
+ *
+ * Just as MEDIA_IOC_SZ_ARG but without the alternative size list or the
+ * callbacks to copy the data from the user space and back to user space.
+ */
+#define MEDIA_IOC(__cmd, func, fl)				\
+	MEDIA_IOC_ARG(__cmd, func, fl,				\
+		      copy_arg_from_user, copy_arg_to_user)
 
 /* the table is indexed by _IOC_NR(cmd) */
 struct media_ioctl_info {
 	unsigned int cmd;
 	unsigned short flags;
+	/*
+	 * Sizes of the alternative arguments. If there are none, this
+	 * pointer is NULL.
+	 */
+	const unsigned short *alt_arg_sizes;
 	long (*fn)(struct media_device *dev, void *arg);
 	long (*arg_from_user)(void *karg, void __user *uarg, unsigned int cmd);
 	long (*arg_to_user)(void __user *uarg, void *karg, unsigned int cmd);
@@ -416,6 +459,46 @@ static const struct media_ioctl_info ioctl_info[] = {
 	MEDIA_IOC(G_TOPOLOGY, media_device_get_topology, MEDIA_IOC_FL_GRAPH_MUTEX),
 };
 
+static inline long is_valid_ioctl(unsigned int user_cmd)
+{
+	const struct media_ioctl_info *info = ioctl_info;
+	const unsigned short *alt_arg_sizes;
+
+	if (_IOC_NR(user_cmd) >= ARRAY_SIZE(ioctl_info))
+		return -ENOIOCTLCMD;
+
+	info += _IOC_NR(user_cmd);
+
+	if (user_cmd == info->cmd)
+		return 0;
+
+	/*
+	 * There was no exact match between the user-passed IOCTL command and
+	 * the definition. Are there earlier revisions of the argument struct
+	 * available?
+	 */
+	if (!info->alt_arg_sizes)
+		return -ENOIOCTLCMD;
+
+	/*
+	 * Variable size IOCTL argument support allows using either the latest
+	 * revision of the IOCTL argument struct or an earlier version. Check
+	 * that the size-independent portions of the IOCTL command match and
+	 * that the size matches with one of the alternative sizes that
+	 * represent earlier revisions of the argument struct.
+	 */
+	if ((user_cmd & ~IOCSIZE_MASK) != (info->cmd & ~IOCSIZE_MASK)
+	    || _IOC_SIZE(user_cmd) < _IOC_SIZE(info->cmd))
+		return -ENOIOCTLCMD;
+
+	for (alt_arg_sizes = info->alt_arg_sizes; *alt_arg_sizes;
+	     alt_arg_sizes++)
+		if (_IOC_SIZE(user_cmd) == *alt_arg_sizes)
+			return 0;
+
+	return -ENOIOCTLCMD;
+}
+
 static long media_device_ioctl(struct file *filp, unsigned int cmd,
 			       unsigned long __arg)
 {
@@ -426,9 +509,9 @@ static long media_device_ioctl(struct file *filp, unsigned int cmd,
 	char __karg[256], *karg = __karg;
 	long ret;
 
-	if (_IOC_NR(cmd) >= ARRAY_SIZE(ioctl_info)
-	    || ioctl_info[_IOC_NR(cmd)].cmd != cmd)
-		return -ENOIOCTLCMD;
+	ret = is_valid_ioctl(cmd);
+	if (ret)
+		return ret;
 
 	info = &ioctl_info[_IOC_NR(cmd)];
 
@@ -444,6 +527,9 @@ static long media_device_ioctl(struct file *filp, unsigned int cmd,
 			goto out_free;
 	}
 
+	/* Set the rest of the argument struct to zero */
+	memset(karg + _IOC_SIZE(cmd), 0, _IOC_SIZE(info->cmd) - _IOC_SIZE(cmd));
+
 	if (info->flags & MEDIA_IOC_FL_GRAPH_MUTEX)
 		mutex_lock(&dev->graph_mutex);
 
-- 
2.7.4

Re: [RFC v2 00/10] Preparing the request API

[Hans Verkuil]

On 03/25/2018 06:17 PM, Hans Verkuil wrote:
>> So this weekend I worked on a merger of this work and the RFCv4 Request API
>> patch series, taking what I think are the best bits of both.
>>
>> It is available here:
>>
>> https://git.linuxtv.org/hverkuil/media_tree.git/log/?h=reqv6
> 
> I reorganized/cleaned up the patch series. So look here instead:
> 
> https://git.linuxtv.org/hverkuil/media_tree.git/log/?h=reqv7
> 
> It's easier to follow.

Status update:

Current work-in-progress tree:

https://git.linuxtv.org/hverkuil/media_tree.git/log/?h=reqv8

v4l2-compliance test code:

https://git.linuxtv.org/hverkuil/v4l-utils.git/log/?h=request

I had hoped to have more tests ready, but there were loads of
get/put errors (not surprisingly) which took a lot of time to fix.

I also must remember for the next time that the list_add_tail prototype is:

void list_add_tail(struct list_head *new, struct list_head *head)

and not:

void list_add_tail(struct list_head *head, struct list_head *new)

Adding an object to a request worked much better than adding a request
to an object :-)

The v4l2-compliance tests I wrote test the basic creation/deletion
of requests, and adding controls to a request.

The main tests deal with all the various open/close combinations
(media fd, video fd, request fd). It's now working for both vim2m and
vivid.

I will try to start on buffers and queueing tests tomorrow, but it might
slip to Wednesday.

An interesting corner case was vim2m: what to do if you allocate a request,
add a control to it, then close the video file handle. Since the whole
state is contained in the video file handle, the control inside the request
is suddenly orphaned since the control refers to the control handler in the
file handle state, which is now deleted.

I have decided to remove the control from the request in that case. This means
that closing the video file handle for such devices removes all request objects
that are created by that file handle from any requests that they were bound to.

For vim2m it is effectively equal to calling MEDIA_REQUEST_IOC_REINIT for the
request.

I think this is a sane approach for such devices.

Regards,

	Hans

Re: [RFC v2.1 1/1] media: Support variable size IOCTL arguments

[Mauro Carvalho Chehab]

Em Mon, 26 Mar 2018 16:23:24 +0300
Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:

> Maintain a list of supported IOCTL argument sizes and allow only those in
> the list.
> 
> As an additional bonus, IOCTL handlers will be able to check whether the
> caller actually set (using the argument size) the field vs. assigning it
> to zero. Separate macro can be provided for that.
> 
> This will be easier for applications as well since there is no longer the
> problem of setting the reserved fields zero, or at least it is a lesser
> problem.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
> Hi folks,
> 
> I've essentially addressed Mauro's comments on v2.
> 
> The code is only compile tested so far but the changes from the last
> tested version are not that big. There's still some uncertainty though.

You should test it... I guess there is a bug on this version :-)
(see below)

> 
> since v2:
> 
> - Rework is_valid_ioctl based on the comments
> 
> 	- Improved comments,
> 	
> 	- Rename cmd as user_cmd, as this comes from the user
> 	
> 	- Check whether there are alternative argument sizes before any
> 	  checks on IOCTL command if there is no exact match
> 	  
> 	- Use IOCSIZE_MASK macro instead of creating our own
> 
> - Add documentation for macros declaring IOCTLs
> 
> 
>  drivers/media/media-device.c | 98 +++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 92 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index 35e81f7..279d740 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -387,22 +387,65 @@ static long copy_arg_to_user(void __user *uarg, void *karg, unsigned int cmd)
>  /* Do acquire the graph mutex */
>  #define MEDIA_IOC_FL_GRAPH_MUTEX	BIT(0)
>  
> -#define MEDIA_IOC_ARG(__cmd, func, fl, from_user, to_user)		\
> +/**
> + * MEDIA_IOC_SZ_ARG - Declare a Media device IOCTL with alternative size and
> + *		      to_user/from_user callbacks
> + *
> + * @__cmd:	The IOCTL command suffix (without "MEDIA_IOC_")
> + * @func:	The handler function
> + * @fl:		Flags from @enum media_ioc_flags
> + * @alt_sz:	A 0-terminated list of alternative argument struct sizes.
> + * @from_user:	Function to copy argument struct from the user to the kernel
> + * @to_user:	Function to copy argument struct to the user from the kernel
> + */
> +#define MEDIA_IOC_SZ_ARG(__cmd, func, fl, alt_sz, from_user, to_user)	\
>  	[_IOC_NR(MEDIA_IOC_##__cmd)] = {				\
>  		.cmd = MEDIA_IOC_##__cmd,				\
>  		.fn = (long (*)(struct media_device *, void *))func,	\
>  		.flags = fl,						\
> +		.alt_arg_sizes = alt_sz,				\
>  		.arg_from_user = from_user,				\
>  		.arg_to_user = to_user,					\
>  	}
>  
> -#define MEDIA_IOC(__cmd, func, fl)					\
> -	MEDIA_IOC_ARG(__cmd, func, fl, copy_arg_from_user, copy_arg_to_user)
> +/**
> + * MEDIA_IOC_ARG - Declare a Media device IOCTL with to_user/from_user callbacks
> + *
> + * Just as MEDIA_IOC_SZ_ARG but without the alternative size list.
> + */

Nitpick: either use:
	/*
	 *...
	 */

or add the arguments to the macro there, as /** ... */ expects
the arguments. Same for other comments below.

> +#define MEDIA_IOC_ARG(__cmd, func, fl, from_user, to_user)		\
> +	MEDIA_IOC_SZ_ARG(__cmd, func, fl, NULL, from_user, to_user)
> +
> +/**
> + * MEDIA_IOC_ARG - Declare a Media device IOCTL with alternative argument struct
> + *		   sizes
> + *
> + * Just as MEDIA_IOC_SZ_ARG but without the callbacks to copy the data from the
> + * user space and back to user space.
> + */
> +#define MEDIA_IOC_SZ(__cmd, func, fl, alt_sz)			\
> +	MEDIA_IOC_SZ_ARG(__cmd, func, fl, alt_sz,		\
> +			 copy_arg_from_user, copy_arg_to_user)
> +
> +/**
> + * MEDIA_IOC_ARG - Declare a Media device IOCTL
> + *
> + * Just as MEDIA_IOC_SZ_ARG but without the alternative size list or the
> + * callbacks to copy the data from the user space and back to user space.
> + */
> +#define MEDIA_IOC(__cmd, func, fl)				\
> +	MEDIA_IOC_ARG(__cmd, func, fl,				\
> +		      copy_arg_from_user, copy_arg_to_user)
>  
>  /* the table is indexed by _IOC_NR(cmd) */
>  struct media_ioctl_info {
>  	unsigned int cmd;
>  	unsigned short flags;
> +	/*
> +	 * Sizes of the alternative arguments. If there are none, this
> +	 * pointer is NULL.
> +	 */
> +	const unsigned short *alt_arg_sizes;
>  	long (*fn)(struct media_device *dev, void *arg);
>  	long (*arg_from_user)(void *karg, void __user *uarg, unsigned int cmd);
>  	long (*arg_to_user)(void __user *uarg, void *karg, unsigned int cmd);
> @@ -416,6 +459,46 @@ static const struct media_ioctl_info ioctl_info[] = {
>  	MEDIA_IOC(G_TOPOLOGY, media_device_get_topology, MEDIA_IOC_FL_GRAPH_MUTEX),
>  };
>  
> +static inline long is_valid_ioctl(unsigned int user_cmd)
> +{
> +	const struct media_ioctl_info *info = ioctl_info;
> +	const unsigned short *alt_arg_sizes;
> +
> +	if (_IOC_NR(user_cmd) >= ARRAY_SIZE(ioctl_info))
> +		return -ENOIOCTLCMD;
> +
> +	info += _IOC_NR(user_cmd);
> +
> +	if (user_cmd == info->cmd)
> +		return 0;
> +
> +	/*
> +	 * There was no exact match between the user-passed IOCTL command and
> +	 * the definition. Are there earlier revisions of the argument struct
> +	 * available?
> +	 */
> +	if (!info->alt_arg_sizes)
> +		return -ENOIOCTLCMD;
> +
> +	/*
> +	 * Variable size IOCTL argument support allows using either the latest
> +	 * revision of the IOCTL argument struct or an earlier version. Check
> +	 * that the size-independent portions of the IOCTL command match and
> +	 * that the size matches with one of the alternative sizes that
> +	 * represent earlier revisions of the argument struct.
> +	 */
> +	if ((user_cmd & ~IOCSIZE_MASK) != (info->cmd & ~IOCSIZE_MASK)
> +	    || _IOC_SIZE(user_cmd) < _IOC_SIZE(info->cmd))
> +		return -ENOIOCTLCMD;

I guess it should be, instead:

	    || _IOC_SIZE(user_cmd) > _IOC_SIZE(info->cmd))

The hole idea is that the struct sizes used by ioctls can monotonically
increase as newer fields become needed, but never decrease.

Assuming that, _IOC_SIZE(MEDIA_IOC_foo) will give the size of the
latest version of an ioctl supported by a given Kernel version,
while alt_arg_sizes will list smaller sizes from previous
Kernel versions that will also be accepted, in order to make it
backward-compatible with apps compiled against older Kernel headers.

However, if an application is compiled with a kernel newer than
the current one, it should fail, as an older Kernel doesn't know
how to handle the newer fields. So, it should be up to the userspace
app to add backward-compatible code if it needs to support older
Kernels.

(perhaps it should be worth adding a comment like the above
somewhere).

> +
> +	for (alt_arg_sizes = info->alt_arg_sizes; *alt_arg_sizes;
> +	     alt_arg_sizes++)
> +		if (_IOC_SIZE(user_cmd) == *alt_arg_sizes)
> +			return 0;
> +
> +	return -ENOIOCTLCMD;
> +}
> +
>  static long media_device_ioctl(struct file *filp, unsigned int cmd,
>  			       unsigned long __arg)
>  {
> @@ -426,9 +509,9 @@ static long media_device_ioctl(struct file *filp, unsigned int cmd,
>  	char __karg[256], *karg = __karg;
>  	long ret;
>  
> -	if (_IOC_NR(cmd) >= ARRAY_SIZE(ioctl_info)
> -	    || ioctl_info[_IOC_NR(cmd)].cmd != cmd)
> -		return -ENOIOCTLCMD;
> +	ret = is_valid_ioctl(cmd);
> +	if (ret)
> +		return ret;
>  
>  	info = &ioctl_info[_IOC_NR(cmd)];
>  
> @@ -444,6 +527,9 @@ static long media_device_ioctl(struct file *filp, unsigned int cmd,
>  			goto out_free;
>  	}
>  
> +	/* Set the rest of the argument struct to zero */
> +	memset(karg + _IOC_SIZE(cmd), 0, _IOC_SIZE(info->cmd) - _IOC_SIZE(cmd));
> +
>  	if (info->flags & MEDIA_IOC_FL_GRAPH_MUTEX)
>  		mutex_lock(&dev->graph_mutex);
>  

Regards,
Mauro

Re: [RFC v2.1 1/1] media: Support variable size IOCTL arguments

[Sakari Ailus]

Hi Mauro,

On Mon, Mar 26, 2018 at 02:28:34PM -0300, Mauro Carvalho Chehab wrote:
> Em Mon, 26 Mar 2018 16:23:24 +0300
> Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
> 
> > Maintain a list of supported IOCTL argument sizes and allow only those in
> > the list.
> > 
> > As an additional bonus, IOCTL handlers will be able to check whether the
> > caller actually set (using the argument size) the field vs. assigning it
> > to zero. Separate macro can be provided for that.
> > 
> > This will be easier for applications as well since there is no longer the
> > problem of setting the reserved fields zero, or at least it is a lesser
> > problem.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
> > ---
> > Hi folks,
> > 
> > I've essentially addressed Mauro's comments on v2.
> > 
> > The code is only compile tested so far but the changes from the last
> > tested version are not that big. There's still some uncertainty though.
> 
> You should test it... I guess there is a bug on this version :-)
> (see below)
> 
> > 
> > since v2:
> > 
> > - Rework is_valid_ioctl based on the comments
> > 
> > 	- Improved comments,
> > 	
> > 	- Rename cmd as user_cmd, as this comes from the user
> > 	
> > 	- Check whether there are alternative argument sizes before any
> > 	  checks on IOCTL command if there is no exact match
> > 	  
> > 	- Use IOCSIZE_MASK macro instead of creating our own
> > 
> > - Add documentation for macros declaring IOCTLs
> > 
> > 
> >  drivers/media/media-device.c | 98 +++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 92 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> > index 35e81f7..279d740 100644
> > --- a/drivers/media/media-device.c
> > +++ b/drivers/media/media-device.c
> > @@ -387,22 +387,65 @@ static long copy_arg_to_user(void __user *uarg, void *karg, unsigned int cmd)
> >  /* Do acquire the graph mutex */
> >  #define MEDIA_IOC_FL_GRAPH_MUTEX	BIT(0)
> >  
> > -#define MEDIA_IOC_ARG(__cmd, func, fl, from_user, to_user)		\
> > +/**
> > + * MEDIA_IOC_SZ_ARG - Declare a Media device IOCTL with alternative size and
> > + *		      to_user/from_user callbacks
> > + *
> > + * @__cmd:	The IOCTL command suffix (without "MEDIA_IOC_")
> > + * @func:	The handler function
> > + * @fl:		Flags from @enum media_ioc_flags
> > + * @alt_sz:	A 0-terminated list of alternative argument struct sizes.
> > + * @from_user:	Function to copy argument struct from the user to the kernel
> > + * @to_user:	Function to copy argument struct to the user from the kernel
> > + */
> > +#define MEDIA_IOC_SZ_ARG(__cmd, func, fl, alt_sz, from_user, to_user)	\
> >  	[_IOC_NR(MEDIA_IOC_##__cmd)] = {				\
> >  		.cmd = MEDIA_IOC_##__cmd,				\
> >  		.fn = (long (*)(struct media_device *, void *))func,	\
> >  		.flags = fl,						\
> > +		.alt_arg_sizes = alt_sz,				\
> >  		.arg_from_user = from_user,				\
> >  		.arg_to_user = to_user,					\
> >  	}
> >  
> > -#define MEDIA_IOC(__cmd, func, fl)					\
> > -	MEDIA_IOC_ARG(__cmd, func, fl, copy_arg_from_user, copy_arg_to_user)
> > +/**
> > + * MEDIA_IOC_ARG - Declare a Media device IOCTL with to_user/from_user callbacks
> > + *
> > + * Just as MEDIA_IOC_SZ_ARG but without the alternative size list.
> > + */
> 
> Nitpick: either use:
> 	/*
> 	 *...
> 	 */
> 
> or add the arguments to the macro there, as /** ... */ expects
> the arguments. Same for other comments below.

I think a regular comment would do. It's only used below.

> 
> > +#define MEDIA_IOC_ARG(__cmd, func, fl, from_user, to_user)		\
> > +	MEDIA_IOC_SZ_ARG(__cmd, func, fl, NULL, from_user, to_user)
> > +
> > +/**
> > + * MEDIA_IOC_ARG - Declare a Media device IOCTL with alternative argument struct
> > + *		   sizes
> > + *
> > + * Just as MEDIA_IOC_SZ_ARG but without the callbacks to copy the data from the
> > + * user space and back to user space.
> > + */
> > +#define MEDIA_IOC_SZ(__cmd, func, fl, alt_sz)			\
> > +	MEDIA_IOC_SZ_ARG(__cmd, func, fl, alt_sz,		\
> > +			 copy_arg_from_user, copy_arg_to_user)
> > +
> > +/**
> > + * MEDIA_IOC_ARG - Declare a Media device IOCTL
> > + *
> > + * Just as MEDIA_IOC_SZ_ARG but without the alternative size list or the
> > + * callbacks to copy the data from the user space and back to user space.
> > + */
> > +#define MEDIA_IOC(__cmd, func, fl)				\
> > +	MEDIA_IOC_ARG(__cmd, func, fl,				\
> > +		      copy_arg_from_user, copy_arg_to_user)
> >  
> >  /* the table is indexed by _IOC_NR(cmd) */
> >  struct media_ioctl_info {
> >  	unsigned int cmd;
> >  	unsigned short flags;
> > +	/*
> > +	 * Sizes of the alternative arguments. If there are none, this
> > +	 * pointer is NULL.
> > +	 */
> > +	const unsigned short *alt_arg_sizes;
> >  	long (*fn)(struct media_device *dev, void *arg);
> >  	long (*arg_from_user)(void *karg, void __user *uarg, unsigned int cmd);
> >  	long (*arg_to_user)(void __user *uarg, void *karg, unsigned int cmd);
> > @@ -416,6 +459,46 @@ static const struct media_ioctl_info ioctl_info[] = {
> >  	MEDIA_IOC(G_TOPOLOGY, media_device_get_topology, MEDIA_IOC_FL_GRAPH_MUTEX),
> >  };
> >  
> > +static inline long is_valid_ioctl(unsigned int user_cmd)
> > +{
> > +	const struct media_ioctl_info *info = ioctl_info;
> > +	const unsigned short *alt_arg_sizes;
> > +
> > +	if (_IOC_NR(user_cmd) >= ARRAY_SIZE(ioctl_info))
> > +		return -ENOIOCTLCMD;
> > +
> > +	info += _IOC_NR(user_cmd);
> > +
> > +	if (user_cmd == info->cmd)
> > +		return 0;
> > +
> > +	/*
> > +	 * There was no exact match between the user-passed IOCTL command and
> > +	 * the definition. Are there earlier revisions of the argument struct
> > +	 * available?
> > +	 */
> > +	if (!info->alt_arg_sizes)
> > +		return -ENOIOCTLCMD;
> > +
> > +	/*
> > +	 * Variable size IOCTL argument support allows using either the latest
> > +	 * revision of the IOCTL argument struct or an earlier version. Check
> > +	 * that the size-independent portions of the IOCTL command match and
> > +	 * that the size matches with one of the alternative sizes that
> > +	 * represent earlier revisions of the argument struct.
> > +	 */
> > +	if ((user_cmd & ~IOCSIZE_MASK) != (info->cmd & ~IOCSIZE_MASK)
> > +	    || _IOC_SIZE(user_cmd) < _IOC_SIZE(info->cmd))
> > +		return -ENOIOCTLCMD;
> 
> I guess it should be, instead:
> 
> 	    || _IOC_SIZE(user_cmd) > _IOC_SIZE(info->cmd))
> 
> The hole idea is that the struct sizes used by ioctls can monotonically
> increase as newer fields become needed, but never decrease.

Oops, indeed. I'll send a new version...

> 
> Assuming that, _IOC_SIZE(MEDIA_IOC_foo) will give the size of the
> latest version of an ioctl supported by a given Kernel version,
> while alt_arg_sizes will list smaller sizes from previous
> Kernel versions that will also be accepted, in order to make it
> backward-compatible with apps compiled against older Kernel headers.
> 
> However, if an application is compiled with a kernel newer than
> the current one, it should fail, as an older Kernel doesn't know
> how to handle the newer fields. So, it should be up to the userspace
> app to add backward-compatible code if it needs to support older
> Kernels.
> 
> (perhaps it should be worth adding a comment like the above
> somewhere).

Good point. I wonder if it'd be better to just handle this in the kernel
and allow larges arguments as well. This would effectively be the same as
we have right now, with a very large number of reserved fields.

We could not assume an application knowingly set a field that is present in
a struct of which an older revision exists: all it takes is to compile the
application in an environment which has the new definitions. Unless... we
put the version to the struct name. But I don't like that, it makes IOCTL
calls (and the documentation) quite ugly.

That'd also suggest the list of alternative sizes isn't very useful: even
when we have reserved fields, we don't have any way of knowing whether an
application intentionally set a field to zero or just left out initialising
that particular field.

I wonder what Hans thinks...

> 
> > +
> > +	for (alt_arg_sizes = info->alt_arg_sizes; *alt_arg_sizes;
> > +	     alt_arg_sizes++)
> > +		if (_IOC_SIZE(user_cmd) == *alt_arg_sizes)
> > +			return 0;
> > +
> > +	return -ENOIOCTLCMD;
> > +}
> > +
> >  static long media_device_ioctl(struct file *filp, unsigned int cmd,
> >  			       unsigned long __arg)
> >  {
> > @@ -426,9 +509,9 @@ static long media_device_ioctl(struct file *filp, unsigned int cmd,
> >  	char __karg[256], *karg = __karg;
> >  	long ret;
> >  
> > -	if (_IOC_NR(cmd) >= ARRAY_SIZE(ioctl_info)
> > -	    || ioctl_info[_IOC_NR(cmd)].cmd != cmd)
> > -		return -ENOIOCTLCMD;
> > +	ret = is_valid_ioctl(cmd);
> > +	if (ret)
> > +		return ret;
> >  
> >  	info = &ioctl_info[_IOC_NR(cmd)];
> >  
> > @@ -444,6 +527,9 @@ static long media_device_ioctl(struct file *filp, unsigned int cmd,
> >  			goto out_free;
> >  	}
> >  
> > +	/* Set the rest of the argument struct to zero */
> > +	memset(karg + _IOC_SIZE(cmd), 0, _IOC_SIZE(info->cmd) - _IOC_SIZE(cmd));
> > +
> >  	if (info->flags & MEDIA_IOC_FL_GRAPH_MUTEX)
> >  		mutex_lock(&dev->graph_mutex);
> >  
> 
> Regards,
> Mauro

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

Re: [RFC v2.1 1/1] media: Support variable size IOCTL arguments

[Mauro Carvalho Chehab]

Em Mon, 26 Mar 2018 23:00:17 +0300
Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:

> Hi Mauro,
> 
> On Mon, Mar 26, 2018 at 02:28:34PM -0300, Mauro Carvalho Chehab wrote:
> > Em Mon, 26 Mar 2018 16:23:24 +0300
> > Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
> >   
> > > Maintain a list of supported IOCTL argument sizes and allow only those in
> > > the list.
> > > 
> > > As an additional bonus, IOCTL handlers will be able to check whether the
> > > caller actually set (using the argument size) the field vs. assigning it
> > > to zero. Separate macro can be provided for that.
> > > 
> > > This will be easier for applications as well since there is no longer the
> > > problem of setting the reserved fields zero, or at least it is a lesser
> > > problem.
> > > 
> > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
> > > ---
> > > Hi folks,
> > > 
> > > I've essentially addressed Mauro's comments on v2.
> > > 
> > > The code is only compile tested so far but the changes from the last
> > > tested version are not that big. There's still some uncertainty though.  
> > 
> > You should test it... I guess there is a bug on this version :-)
> > (see below)
> >   
> > > 
> > > since v2:
> > > 
> > > - Rework is_valid_ioctl based on the comments
> > > 
> > > 	- Improved comments,
> > > 	
> > > 	- Rename cmd as user_cmd, as this comes from the user
> > > 	
> > > 	- Check whether there are alternative argument sizes before any
> > > 	  checks on IOCTL command if there is no exact match
> > > 	  
> > > 	- Use IOCSIZE_MASK macro instead of creating our own
> > > 
> > > - Add documentation for macros declaring IOCTLs
> > > 
> > > 
> > >  drivers/media/media-device.c | 98 +++++++++++++++++++++++++++++++++++++++++---
> > >  1 file changed, 92 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> > > index 35e81f7..279d740 100644
> > > --- a/drivers/media/media-device.c
> > > +++ b/drivers/media/media-device.c
> > > @@ -387,22 +387,65 @@ static long copy_arg_to_user(void __user *uarg, void *karg, unsigned int cmd)
> > >  /* Do acquire the graph mutex */
> > >  #define MEDIA_IOC_FL_GRAPH_MUTEX	BIT(0)
> > >  
> > > -#define MEDIA_IOC_ARG(__cmd, func, fl, from_user, to_user)		\
> > > +/**
> > > + * MEDIA_IOC_SZ_ARG - Declare a Media device IOCTL with alternative size and
> > > + *		      to_user/from_user callbacks
> > > + *
> > > + * @__cmd:	The IOCTL command suffix (without "MEDIA_IOC_")
> > > + * @func:	The handler function
> > > + * @fl:		Flags from @enum media_ioc_flags
> > > + * @alt_sz:	A 0-terminated list of alternative argument struct sizes.
> > > + * @from_user:	Function to copy argument struct from the user to the kernel
> > > + * @to_user:	Function to copy argument struct to the user from the kernel
> > > + */
> > > +#define MEDIA_IOC_SZ_ARG(__cmd, func, fl, alt_sz, from_user, to_user)	\
> > >  	[_IOC_NR(MEDIA_IOC_##__cmd)] = {				\
> > >  		.cmd = MEDIA_IOC_##__cmd,				\
> > >  		.fn = (long (*)(struct media_device *, void *))func,	\
> > >  		.flags = fl,						\
> > > +		.alt_arg_sizes = alt_sz,				\
> > >  		.arg_from_user = from_user,				\
> > >  		.arg_to_user = to_user,					\
> > >  	}
> > >  
> > > -#define MEDIA_IOC(__cmd, func, fl)					\
> > > -	MEDIA_IOC_ARG(__cmd, func, fl, copy_arg_from_user, copy_arg_to_user)
> > > +/**
> > > + * MEDIA_IOC_ARG - Declare a Media device IOCTL with to_user/from_user callbacks
> > > + *
> > > + * Just as MEDIA_IOC_SZ_ARG but without the alternative size list.
> > > + */  
> > 
> > Nitpick: either use:
> > 	/*
> > 	 *...
> > 	 */
> > 
> > or add the arguments to the macro there, as /** ... */ expects
> > the arguments. Same for other comments below.  
> 
> I think a regular comment would do. It's only used below.

Works for me.

> >   
> > > +#define MEDIA_IOC_ARG(__cmd, func, fl, from_user, to_user)		\
> > > +	MEDIA_IOC_SZ_ARG(__cmd, func, fl, NULL, from_user, to_user)
> > > +
> > > +/**
> > > + * MEDIA_IOC_ARG - Declare a Media device IOCTL with alternative argument struct
> > > + *		   sizes
> > > + *
> > > + * Just as MEDIA_IOC_SZ_ARG but without the callbacks to copy the data from the
> > > + * user space and back to user space.
> > > + */
> > > +#define MEDIA_IOC_SZ(__cmd, func, fl, alt_sz)			\
> > > +	MEDIA_IOC_SZ_ARG(__cmd, func, fl, alt_sz,		\
> > > +			 copy_arg_from_user, copy_arg_to_user)
> > > +
> > > +/**
> > > + * MEDIA_IOC_ARG - Declare a Media device IOCTL
> > > + *
> > > + * Just as MEDIA_IOC_SZ_ARG but without the alternative size list or the
> > > + * callbacks to copy the data from the user space and back to user space.
> > > + */
> > > +#define MEDIA_IOC(__cmd, func, fl)				\
> > > +	MEDIA_IOC_ARG(__cmd, func, fl,				\
> > > +		      copy_arg_from_user, copy_arg_to_user)
> > >  
> > >  /* the table is indexed by _IOC_NR(cmd) */
> > >  struct media_ioctl_info {
> > >  	unsigned int cmd;
> > >  	unsigned short flags;
> > > +	/*
> > > +	 * Sizes of the alternative arguments. If there are none, this
> > > +	 * pointer is NULL.
> > > +	 */
> > > +	const unsigned short *alt_arg_sizes;
> > >  	long (*fn)(struct media_device *dev, void *arg);
> > >  	long (*arg_from_user)(void *karg, void __user *uarg, unsigned int cmd);
> > >  	long (*arg_to_user)(void __user *uarg, void *karg, unsigned int cmd);
> > > @@ -416,6 +459,46 @@ static const struct media_ioctl_info ioctl_info[] = {
> > >  	MEDIA_IOC(G_TOPOLOGY, media_device_get_topology, MEDIA_IOC_FL_GRAPH_MUTEX),
> > >  };
> > >  
> > > +static inline long is_valid_ioctl(unsigned int user_cmd)
> > > +{
> > > +	const struct media_ioctl_info *info = ioctl_info;
> > > +	const unsigned short *alt_arg_sizes;
> > > +
> > > +	if (_IOC_NR(user_cmd) >= ARRAY_SIZE(ioctl_info))
> > > +		return -ENOIOCTLCMD;
> > > +
> > > +	info += _IOC_NR(user_cmd);
> > > +
> > > +	if (user_cmd == info->cmd)
> > > +		return 0;
> > > +
> > > +	/*
> > > +	 * There was no exact match between the user-passed IOCTL command and
> > > +	 * the definition. Are there earlier revisions of the argument struct
> > > +	 * available?
> > > +	 */
> > > +	if (!info->alt_arg_sizes)
> > > +		return -ENOIOCTLCMD;
> > > +
> > > +	/*
> > > +	 * Variable size IOCTL argument support allows using either the latest
> > > +	 * revision of the IOCTL argument struct or an earlier version. Check
> > > +	 * that the size-independent portions of the IOCTL command match and
> > > +	 * that the size matches with one of the alternative sizes that
> > > +	 * represent earlier revisions of the argument struct.
> > > +	 */
> > > +	if ((user_cmd & ~IOCSIZE_MASK) != (info->cmd & ~IOCSIZE_MASK)
> > > +	    || _IOC_SIZE(user_cmd) < _IOC_SIZE(info->cmd))
> > > +		return -ENOIOCTLCMD;  
> > 
> > I guess it should be, instead:
> > 
> > 	    || _IOC_SIZE(user_cmd) > _IOC_SIZE(info->cmd))
> > 
> > The hole idea is that the struct sizes used by ioctls can monotonically
> > increase as newer fields become needed, but never decrease.  
> 
> Oops, indeed. I'll send a new version...
> 
> > 
> > Assuming that, _IOC_SIZE(MEDIA_IOC_foo) will give the size of the
> > latest version of an ioctl supported by a given Kernel version,
> > while alt_arg_sizes will list smaller sizes from previous
> > Kernel versions that will also be accepted, in order to make it
> > backward-compatible with apps compiled against older Kernel headers.
> > 
> > However, if an application is compiled with a kernel newer than
> > the current one, it should fail, as an older Kernel doesn't know
> > how to handle the newer fields. So, it should be up to the userspace
> > app to add backward-compatible code if it needs to support older
> > Kernels.
> > 
> > (perhaps it should be worth adding a comment like the above
> > somewhere).  
> 
> Good point. I wonder if it'd be better to just handle this in the kernel
> and allow larges arguments as well. This would effectively be the same as
> we have right now, with a very large number of reserved fields.
> 
> We could not assume an application knowingly set a field that is present in
> a struct of which an older revision exists: all it takes is to compile the
> application in an environment which has the new definitions. Unless... we
> put the version to the struct name. But I don't like that, it makes IOCTL
> calls (and the documentation) quite ugly.
> 
> That'd also suggest the list of alternative sizes isn't very useful: even
> when we have reserved fields, we don't have any way of knowing whether an
> application intentionally set a field to zero or just left out initialising
> that particular field.
> 
> I wonder what Hans thinks...

No, it wouldn't be the same, at least for ioctls that sets something.

I mean, let's say that we end by adding a new field FOO to 
MEDIA_IOC_SETUP_LINK, that would affect the way it works, for
example, allowing it to set two or more links at once. 

If the Kernel handles such ioctl only to the parameters it knows,
it will set just the first link, discarding the rest of the ioctl.

I don't think that there's a safe way to allow an older Kernel to
support applications compiled for a new Kernel version.
So, I wouldn't even try to do it.

On the other hand, for an application, it is easy to check what's
the version of the Kernel/media controller and send a set of
MEDIA_IOC_SETUP_LINK_legacy per/link ioctls or the newer
MEDIA_IOC_SETUP_LINK_new depending on the Kernel version.

Thanks,
Mauro

Re: [RFC v2 00/10] Preparing the request API

[Hans Verkuil]

Status update:

Current work-in-progress tree:

https://git.linuxtv.org/hverkuil/media_tree.git/log/?h=reqv8

v4l2-compliance test code:

https://git.linuxtv.org/hverkuil/v4l-utils.git/log/?h=request

It is now working for me with v4l2-compliance and vim2m and vivid.

All requests and request objects are correctly freed after doing all
the v4l2-compliance tests.

It's a fairly decent test coverage, but I'm sure there are some corner
cases that can be added.

I've frozen my reqv8 branch so that can be used as a starting point for
codec drivers.

I've started a reqv9 which will be the cleaned-up version of reqv8.
My hope is that I can finish that tomorrow and post the patch series
to the ML at the end of the day.

Regards,

	Hans