[RFCv9 PATCH 00/29] Request API

[RFCv9 PATCH 00/29] Request API

[Hans Verkuil]

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

Hi all,

This patch series is an attempt to pick the best parts of
Alexandre's RFCv4:

https://lkml.org/lkml/2018/2/19/872

and Sakari's RFCv2:

https://www.mail-archive.com/linux-media@vger.kernel.org/msg128170.html

and based on the RFCv2 of the Request public API proposal:

https://www.mail-archive.com/linux-media@vger.kernel.org/msg128098.html

together with my own ideas.

I've no idea what version to give this series, so I just went with
v9 since that's the version of my internal git branch.

This has been tested with vim2m and vivid using v4l2-compliance.
The v4l-utils repo supporting requests is here:
https://git.linuxtv.org/hverkuil/v4l-utils.git/log/?h=request

The goal is to use request and request objects as implemented in
Sakari's RFCv2, together with the vb2 and control framework
support as implemented in Alexandre's RFCv4. In addition, I tried
hard to minimize the impact of requests to drivers.

TODO/Remarks:

1) missing prototype documentation in media-requests.c/h. Some
   is documented, but not everything.

2) improve the control handler implementation: currently this just
   clones the driver's control handler when a request object is
   created. And only standard controls can be contained in a request.
   I probably need another day of work to get this done properly by
   chaining control handlers as they are queued.

   The control patches are also a bit messy, so that needs to be
   cleaned up as well. I hope I can spend some time on this next
   week after Easter as this is the main TODO item. I expect I'll
   need 1-2 days of work to finished this.

   That said, the current implementation should be OK for stateless
   codecs.

3) No VIDIOC_REQUEST_ALLOC 'shortcut' ioctl. Sorry, I just ran out
   of time. Alexandre, Tomasz, feel free to add it back (it should
   be quite easy to do) and post a patch. I'll add it to my patch
   series. As mentioned before: whether or not we actually want this
   has not been decided yet.

4) vim2m: the media topology is a bit bogus, this needs to be fixed
   (i.e. a proper HW entity should be added). But for now it is
   good enough for testing.

5) I think this should slide in fairly easy after the fence support
   is merged. I made sure the request API changes in public headers
   did not clash with the changes made by the fence API.

6) I did not verify the Request API documentation patch. I did update
   it with the new buffer flags and 'which' value, but it probably is
   out of date in places.

7) More testing. I think I have fairly good coverage in v4l2-compliance,
   but no doubt I've missed a few test cases.

8) debugfs: it would be really useful to expose the number of request
   and request objects in debugfs to simplify debugging. Esp. to make
   sure everything is freed correctly.

9) Review copyright/authorship lines. I'm not sure if everything is
   correct. Alexandre, Sakari, if you see something that is not
   right in this respect, just let me know!

Everything seemed to slip nicely into place while working on this,
so I hope this is finally an implementation that we can proceed to
upstream and build upon for complex camera pipelines in the future.

This patch series is also available here:

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

Regards,

	Hans

Alexandre Courbot (4):
  v4l2-ctrls: do not clone non-standard controls
  videodev2.h: add request_fd field to v4l2_ext_controls
  Documentation: v4l: document request API
  media: vim2m: add media device

Hans Verkuil (25):
  v4l2-device.h: always expose mdev
  uapi/linux/media.h: add request API
  media-request: allocate media requests
  media-request: core request support
  media-request: add request ioctls
  media-request: add media_request_find
  media-request: add media_request_object_find
  v4l2-ctrls: v4l2_ctrl_add_handler: add from_other_dev
  v4l2-ctrls: prepare internal structs for request API
  v4l2-ctrls: alloc memory for p_req
  v4l2-ctrls: use ref in helper instead of ctrl
  v4l2-ctrls: support g/s_ext_ctrls for requests
  v4l2-ctrls: add core request API
  v4l2-ctrls: integrate with requests
  videodev2.h: Add request_fd field to v4l2_buffer
  vb2: store userspace data in vb2_v4l2_buffer
  videobuf2-core: embed media_request_object
  videobuf2-core: integrate with media requests
  videobuf2-v4l2: integrate with media requests
  videobuf2-core: add vb2_core_request_has_buffers
  videobuf2-v4l2: add vb2_request_queue helper
  vim2m: use workqueue
  vim2m: support requests
  vivid: add mc
  vivid: add request support

 Documentation/media/uapi/v4l/buffer.rst            |  19 +-
 Documentation/media/uapi/v4l/common.rst            |   1 +
 Documentation/media/uapi/v4l/request-api.rst       | 199 ++++++++++
 Documentation/media/uapi/v4l/user-func.rst         |   1 +
 .../media/uapi/v4l/vidioc-g-ext-ctrls.rst          |  22 +-
 .../media/uapi/v4l/vidioc-new-request.rst          |  64 +++
 Documentation/media/uapi/v4l/vidioc-qbuf.rst       |   8 +
 drivers/media/Makefile                             |   3 +-
 drivers/media/common/videobuf2/videobuf2-core.c    | 143 +++++--
 drivers/media/common/videobuf2/videobuf2-v4l2.c    | 440 +++++++++++++--------
 drivers/media/dvb-core/dvb_vb2.c                   |   5 +-
 drivers/media/dvb-frontends/rtl2832_sdr.c          |   5 +-
 drivers/media/media-device.c                       |  14 +
 drivers/media/media-request.c                      | 439 ++++++++++++++++++++
 drivers/media/pci/bt8xx/bttv-driver.c              |   2 +-
 drivers/media/pci/cx23885/cx23885-417.c            |   2 +-
 drivers/media/pci/cx88/cx88-blackbird.c            |   2 +-
 drivers/media/pci/cx88/cx88-video.c                |   2 +-
 drivers/media/pci/saa7134/saa7134-empress.c        |   4 +-
 drivers/media/pci/saa7134/saa7134-video.c          |   2 +-
 drivers/media/platform/exynos4-is/fimc-capture.c   |   2 +-
 drivers/media/platform/omap3isp/ispvideo.c         |   4 +-
 drivers/media/platform/rcar-vin/rcar-v4l2.c        |   3 +-
 drivers/media/platform/rcar_drif.c                 |   2 +-
 drivers/media/platform/s3c-camif/camif-capture.c   |   4 +-
 drivers/media/platform/s5p-mfc/s5p_mfc_dec.c       |   4 +-
 drivers/media/platform/s5p-mfc/s5p_mfc_enc.c       |   4 +-
 drivers/media/platform/soc_camera/soc_camera.c     |   7 +-
 drivers/media/platform/vim2m.c                     |  83 +++-
 drivers/media/platform/vivid/vivid-core.c          |  68 ++++
 drivers/media/platform/vivid/vivid-core.h          |   8 +
 drivers/media/platform/vivid/vivid-ctrls.c         |  46 +--
 drivers/media/platform/vivid/vivid-kthread-cap.c   |  12 +
 drivers/media/platform/vivid/vivid-kthread-out.c   |  12 +
 drivers/media/platform/vivid/vivid-sdr-cap.c       |   8 +
 drivers/media/platform/vivid/vivid-vbi-cap.c       |   2 +
 drivers/media/platform/vivid/vivid-vbi-out.c       |   2 +
 drivers/media/platform/vivid/vivid-vid-cap.c       |   2 +
 drivers/media/platform/vivid/vivid-vid-out.c       |   2 +
 drivers/media/usb/cpia2/cpia2_v4l.c                |   2 +-
 drivers/media/usb/cx231xx/cx231xx-417.c            |   2 +-
 drivers/media/usb/cx231xx/cx231xx-video.c          |   4 +-
 drivers/media/usb/msi2500/msi2500.c                |   2 +-
 drivers/media/usb/tm6000/tm6000-video.c            |   2 +-
 drivers/media/usb/uvc/uvc_queue.c                  |   5 +-
 drivers/media/usb/uvc/uvc_v4l2.c                   |   3 +-
 drivers/media/usb/uvc/uvcvideo.h                   |   1 +
 drivers/media/v4l2-core/v4l2-compat-ioctl32.c      |  14 +-
 drivers/media/v4l2-core/v4l2-ctrls.c               | 388 ++++++++++++++++--
 drivers/media/v4l2-core/v4l2-device.c              |   3 +-
 drivers/media/v4l2-core/v4l2-ioctl.c               |  22 +-
 drivers/media/v4l2-core/v4l2-mem2mem.c             |   7 +-
 drivers/media/v4l2-core/v4l2-subdev.c              |   9 +-
 drivers/staging/media/davinci_vpfe/vpfe_video.c    |   3 +-
 drivers/staging/media/imx/imx-media-dev.c          |   2 +-
 drivers/staging/media/imx/imx-media-fim.c          |   2 +-
 drivers/staging/media/omap4iss/iss_video.c         |   3 +-
 drivers/usb/gadget/function/uvc_queue.c            |   2 +-
 include/media/media-device.h                       |  13 +
 include/media/media-request.h                      | 204 ++++++++++
 include/media/v4l2-ctrls.h                         |  37 +-
 include/media/v4l2-device.h                        |   4 +-
 include/media/videobuf2-core.h                     |  25 +-
 include/media/videobuf2-v4l2.h                     |  17 +-
 include/uapi/linux/media.h                         |   8 +
 include/uapi/linux/videodev2.h                     |  14 +-
 66 files changed, 2131 insertions(+), 319 deletions(-)
 create mode 100644 Documentation/media/uapi/v4l/request-api.rst
 create mode 100644 Documentation/media/uapi/v4l/vidioc-new-request.rst
 create mode 100644 drivers/media/media-request.c
 create mode 100644 include/media/media-request.h

-- 
2.16.1

[RFCv9 PATCH 01/29] v4l2-device.h: always expose mdev

[Hans Verkuil]

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

The mdev field is only present if CONFIG_MEDIA_CONTROLLER is set.
But since we will need to pass the media_device to vb2 snd the
control framework it is very convenient to just make this field
available all the time. If CONFIG_MEDIA_CONTROLLER is not set,
then it will just be NULL.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 include/media/v4l2-device.h | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/include/media/v4l2-device.h b/include/media/v4l2-device.h
index 0c9e4da55499..b330e4a08a6b 100644
--- a/include/media/v4l2-device.h
+++ b/include/media/v4l2-device.h
@@ -33,7 +33,7 @@ struct v4l2_ctrl_handler;
  * struct v4l2_device - main struct to for V4L2 device drivers
  *
  * @dev: pointer to struct device.
- * @mdev: pointer to struct media_device
+ * @mdev: pointer to struct media_device, may be NULL.
  * @subdevs: used to keep track of the registered subdevs
  * @lock: lock this struct; can be used by the driver as well
  *	if this struct is embedded into a larger struct.
@@ -58,9 +58,7 @@ struct v4l2_ctrl_handler;
  */
 struct v4l2_device {
 	struct device *dev;
-#if defined(CONFIG_MEDIA_CONTROLLER)
 	struct media_device *mdev;
-#endif
 	struct list_head subdevs;
 	spinlock_t lock;
 	char name[V4L2_DEVICE_NAME_SIZE];
-- 
2.16.1

[RFCv9 PATCH 02/29] uapi/linux/media.h: add request API

[Hans Verkuil]

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

Define the public request API.

This adds the new MEDIA_IOC_REQUEST_ALLOC ioctl to allocate a request
and two ioctls that operate on a request in order to queue the
contents of the request to the driver and to re-initialize the
request.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 include/uapi/linux/media.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
index c7e9a5cba24e..f8769e74f847 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_alloc {
+	__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_alloc)
+
+#define MEDIA_REQUEST_IOC_QUEUE		_IO('|',  0x80)
+#define MEDIA_REQUEST_IOC_REINIT	_IO('|',  0x81)
 
 #if !defined(__KERNEL__) || defined(__NEED_MEDIA_LEGACY_API)
 
-- 
2.16.1

[RFCv9 PATCH 03/29] media-request: allocate media requests

[Hans Verkuil]

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

Add support for allocating a new request. This is only supported
if mdev->ops->req_queue is set, i.e. the driver indicates that it
supports queueing requests.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/Makefile        |  3 ++-
 drivers/media/media-device.c  | 14 ++++++++++++++
 drivers/media/media-request.c | 23 +++++++++++++++++++++++
 include/media/media-device.h  | 13 +++++++++++++
 include/media/media-request.h | 22 ++++++++++++++++++++++
 5 files changed, 74 insertions(+), 1 deletion(-)
 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 594b462ddf0e..985d35ec6b29 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 35e81f7c0d2f..acb583c0eacd 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,15 @@ 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_alloc *alloc)
+{
+	if (!mdev->ops || !mdev->ops->req_queue)
+		return -ENOTTY;
+
+	return media_request_alloc(mdev, alloc);
+}
+
 static long copy_arg_from_user(void *karg, void __user *uarg, unsigned int cmd)
 {
 	/* All media IOCTLs are _IOWR() */
@@ -414,6 +424,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),
 };
 
 static long media_device_ioctl(struct file *filp, unsigned int cmd,
@@ -686,6 +697,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);
+
+	spin_lock_init(&mdev->req_lock);
+	mutex_init(&mdev->req_queue_mutex);
 	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 000000000000..ead78613fdbe
--- /dev/null
+++ b/drivers/media/media-request.c
@@ -0,0 +1,23 @@
+// 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>
+
+int media_request_alloc(struct media_device *mdev,
+			struct media_request_alloc *alloc)
+{
+	return -ENOMEM;
+}
diff --git a/include/media/media-device.h b/include/media/media-device.h
index bcc6ec434f1f..07e323c57202 100644
--- a/include/media/media-device.h
+++ b/include/media/media-device.h
@@ -19,6 +19,7 @@
 #ifndef _MEDIA_DEVICE_H
 #define _MEDIA_DEVICE_H
 
+#include <linux/anon_inodes.h>
 #include <linux/list.h>
 #include <linux/mutex.h>
 
@@ -27,6 +28,7 @@
 
 struct ida;
 struct device;
+struct media_device;
 
 /**
  * struct media_entity_notify - Media Entity Notify
@@ -50,10 +52,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 +96,8 @@ 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
  *
  * 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 +168,9 @@ 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;
 };
 
 /* 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 000000000000..dae3eccd9aa7
--- /dev/null
+++ b/include/media/media-request.h
@@ -0,0 +1,22 @@
+// 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>
+
+int media_request_alloc(struct media_device *mdev,
+			struct media_request_alloc *alloc);
+
+#endif
-- 
2.16.1

[RFCv9 PATCH 04/29] media-request: core request support

[Hans Verkuil]

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

Implement the core of the media request processing.

Drivers can bind request objects to a request. These objects
can then be marked completed if the driver finished using them,
or just be unbound if the results do not need to be kept (e.g.
in the case of buffers).

Once all objects that were added are either unbound or completed,
the request is marked 'complete' and a POLLPRI signal is sent
via poll.

Both requests and request objects are refcounted.

While a request is queued its refcount is incremented (since it
is in use by a driver). Once it is completed the refcount is
decremented. When the user closes the request file descriptor
the refcount is also decremented. Once it reaches 0 all request
objects in the request are unbound and put() and the request
itself is freed.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/media-request.c | 269 +++++++++++++++++++++++++++++++++++++++++-
 include/media/media-request.h | 148 +++++++++++++++++++++++
 2 files changed, 416 insertions(+), 1 deletion(-)

diff --git a/drivers/media/media-request.c b/drivers/media/media-request.c
index ead78613fdbe..8135d9d32af9 100644
--- a/drivers/media/media-request.c
+++ b/drivers/media/media-request.c
@@ -16,8 +16,275 @@
 #include <media/media-device.h>
 #include <media/media-request.h>
 
+static const char * const request_state[] = {
+	"idle",
+	"queueing",
+	"queued",
+	"complete",
+	"cleaning",
+};
+
+static const char *
+media_request_state_str(enum media_request_state state)
+{
+	if (WARN_ON(state >= ARRAY_SIZE(request_state)))
+		return "unknown";
+	return request_state[state];
+}
+
+static void media_request_clean(struct media_request *req)
+{
+	struct media_request_object *obj, *obj_safe;
+
+	WARN_ON(req->state != MEDIA_REQUEST_STATE_CLEANING);
+
+	list_for_each_entry_safe(obj, obj_safe, &req->objects, list) {
+		media_request_object_unbind(obj);
+		media_request_object_put(obj);
+	}
+
+	req->num_incomplete_objects = 0;
+	wake_up_interruptible(&req->poll_wait);
+}
+
+static void media_request_release(struct kref *kref)
+{
+	struct media_request *req =
+		container_of(kref, struct media_request, kref);
+	struct media_device *mdev = req->mdev;
+	unsigned long flags;
+
+	dev_dbg(mdev->dev, "request: release %s\n", req->debug_str);
+
+	spin_lock_irqsave(&req->lock, flags);
+	req->state = MEDIA_REQUEST_STATE_CLEANING;
+	spin_unlock_irqrestore(&req->lock, flags);
+
+	media_request_clean(req);
+
+	if (mdev->ops->req_free)
+		mdev->ops->req_free(req);
+	else
+		kfree(req);
+}
+
+void media_request_put(struct media_request *req)
+{
+	kref_put(&req->kref, media_request_release);
+}
+EXPORT_SYMBOL_GPL(media_request_put);
+
+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 unsigned int media_request_poll(struct file *filp,
+				       struct poll_table_struct *wait)
+{
+	struct media_request *req = filp->private_data;
+	unsigned long flags;
+	enum media_request_state state;
+
+	if (!(poll_requested_events(wait) & POLLPRI))
+		return 0;
+
+	spin_lock_irqsave(&req->lock, flags);
+	state = req->state;
+	spin_unlock_irqrestore(&req->lock, flags);
+
+	if (state == MEDIA_REQUEST_STATE_COMPLETE)
+		return POLLPRI;
+	if (state == MEDIA_REQUEST_STATE_IDLE)
+		return POLLERR;
+
+	poll_wait(filp, &req->poll_wait, wait);
+	return 0;
+}
+
+static long media_request_ioctl(struct file *filp, unsigned int cmd,
+				unsigned long __arg)
+{
+	return -ENOIOCTLCMD;
+}
+
+static const struct file_operations request_fops = {
+	.owner = THIS_MODULE,
+	.poll = media_request_poll,
+	.unlocked_ioctl = media_request_ioctl,
+	.release = media_request_close,
+};
+
 int media_request_alloc(struct media_device *mdev,
 			struct media_request_alloc *alloc)
 {
-	return -ENOMEM;
+	struct media_request *req;
+	struct file *filp;
+	char comm[TASK_COMM_LEN];
+	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;
+	}
+
+	if (mdev->ops->req_alloc)
+		req = mdev->ops->req_alloc(mdev);
+	else
+		req = kzalloc(sizeof(*req), GFP_KERNEL);
+	if (!req) {
+		ret = -ENOMEM;
+		goto err_fput;
+	}
+
+	filp->private_data = req;
+	req->mdev = mdev;
+	req->state = MEDIA_REQUEST_STATE_IDLE;
+	req->num_incomplete_objects = 0;
+	kref_init(&req->kref);
+	INIT_LIST_HEAD(&req->objects);
+	spin_lock_init(&req->lock);
+	init_waitqueue_head(&req->poll_wait);
+
+	alloc->fd = fd;
+
+	get_task_comm(comm, current);
+	snprintf(req->debug_str, sizeof(req->debug_str), "%s:%d",
+		 comm, fd);
+	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;
+}
+
+static void media_request_object_release(struct kref *kref)
+{
+	struct media_request_object *obj =
+		container_of(kref, struct media_request_object, kref);
+	struct media_request *req = obj->req;
+
+	if (req)
+		media_request_object_unbind(obj);
+	obj->ops->release(obj);
+}
+
+void media_request_object_put(struct media_request_object *obj)
+{
+	kref_put(&obj->kref, media_request_object_release);
+}
+EXPORT_SYMBOL_GPL(media_request_object_put);
+
+void media_request_object_init(struct media_request_object *obj)
+{
+	obj->ops = NULL;
+	obj->req = NULL;
+	obj->priv = NULL;
+	obj->completed = false;
+	INIT_LIST_HEAD(&obj->list);
+	kref_init(&obj->kref);
+}
+EXPORT_SYMBOL_GPL(media_request_object_init);
+
+void media_request_object_bind(struct media_request *req,
+			       const struct media_request_object_ops *ops,
+			       void *priv,
+			       struct media_request_object *obj)
+{
+	unsigned long flags;
+
+	WARN_ON(!ops->release);
+	obj->req = req;
+	obj->ops = ops;
+	obj->priv = priv;
+	spin_lock_irqsave(&req->lock, flags);
+	if (WARN_ON(req->state != MEDIA_REQUEST_STATE_IDLE))
+		goto unlock;
+	list_add_tail(&obj->list, &req->objects);
+	req->num_incomplete_objects++;
+unlock:
+	spin_unlock_irqrestore(&req->lock, flags);
+}
+EXPORT_SYMBOL_GPL(media_request_object_bind);
+
+void media_request_object_unbind(struct media_request_object *obj)
+{
+	struct media_request *req = obj->req;
+	unsigned long flags;
+	bool completed = false;
+
+	if (!req)
+		return;
+
+	spin_lock_irqsave(&req->lock, flags);
+	list_del(&obj->list);
+	obj->req = NULL;
+
+	if (req->state == MEDIA_REQUEST_STATE_COMPLETE ||
+	    req->state == MEDIA_REQUEST_STATE_CLEANING)
+		goto unlock;
+
+	if (WARN_ON(req->state == MEDIA_REQUEST_STATE_QUEUEING))
+		goto unlock;
+
+	if (WARN_ON(!req->num_incomplete_objects))
+		goto unlock;
+
+	req->num_incomplete_objects--;
+	if (req->state == MEDIA_REQUEST_STATE_QUEUED &&
+	    !req->num_incomplete_objects) {
+		req->state = MEDIA_REQUEST_STATE_COMPLETE;
+		completed = true;
+		wake_up_interruptible(&req->poll_wait);
+	}
+unlock:
+	spin_unlock_irqrestore(&req->lock, flags);
+	if (obj->ops->unbind)
+		obj->ops->unbind(obj);
+	if (completed)
+		media_request_put(req);
+}
+EXPORT_SYMBOL_GPL(media_request_object_unbind);
+
+void media_request_object_complete(struct media_request_object *obj)
+{
+	struct media_request *req = obj->req;
+	unsigned long flags;
+	bool completed = false;
+
+	spin_lock_irqsave(&req->lock, flags);
+	if (obj->completed)
+		goto unlock;
+	obj->completed = true;
+	if (WARN_ON(!req->num_incomplete_objects) ||
+	    WARN_ON(req->state != MEDIA_REQUEST_STATE_QUEUED))
+		goto unlock;
+
+	if (!--req->num_incomplete_objects) {
+		req->state = MEDIA_REQUEST_STATE_COMPLETE;
+		wake_up_interruptible(&req->poll_wait);
+		completed = true;
+	}
+unlock:
+	spin_unlock_irqrestore(&req->lock, flags);
+	if (completed)
+		media_request_put(req);
 }
+EXPORT_SYMBOL_GPL(media_request_object_complete);
diff --git a/include/media/media-request.h b/include/media/media-request.h
index dae3eccd9aa7..baed99eb1279 100644
--- a/include/media/media-request.h
+++ b/include/media/media-request.h
@@ -16,7 +16,155 @@
 
 #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_CLEANING,
+};
+
+struct media_request_object;
+
+/**
+ * struct media_request - Media device request
+ * @mdev: Media device this request belongs to
+ * @kref: Reference count
+ * @debug_prefix: Prefix for debug messages (process name:fd)
+ * @state: The state of the request
+ * @objects: List of @struct media_request_object request objects
+ * @num_objects: The number objects in the request
+ * @num_completed_objects: The number of completed objects in the request
+ * @poll_wait: Wait queue for poll
+ * @lock: Serializes access to this struct
+ */
+struct media_request {
+	struct media_device *mdev;
+	struct kref kref;
+	char debug_str[TASK_COMM_LEN + 11];
+	enum media_request_state state;
+	struct list_head objects;
+	unsigned int num_incomplete_objects;
+	struct wait_queue_head poll_wait;
+	spinlock_t lock;
+};
+
+#ifdef CONFIG_MEDIA_CONTROLLER
+static inline void media_request_get(struct media_request *req)
+{
+	kref_get(&req->kref);
+}
+
+void media_request_put(struct media_request *req);
+
 int media_request_alloc(struct media_device *mdev,
 			struct media_request_alloc *alloc);
+#else
+static inline void media_request_get(struct media_request *req)
+{
+}
+
+static inline void media_request_put(struct media_request *req)
+{
+}
+#endif
+
+struct media_request_object_ops {
+	int (*prepare)(struct media_request_object *object);
+	void (*unprepare)(struct media_request_object *object);
+	void (*queue)(struct media_request_object *object);
+	void (*unbind)(struct media_request_object *object);
+	void (*release)(struct media_request_object *object);
+};
+
+/**
+ * struct media_request_object - An opaque object that belongs to a media
+ *				 request
+ *
+ * @priv: object's priv pointer
+ * @list: List entry of the object for @struct media_request
+ * @kref: Reference count of the object, acquire before releasing req->lock
+ *
+ * An object related to the request. This struct is embedded in the
+ * larger object data.
+ */
+struct media_request_object {
+	const struct media_request_object_ops *ops;
+	void *priv;
+	struct media_request *req;
+	struct list_head list;
+	struct kref kref;
+	bool completed;
+};
+
+#ifdef CONFIG_MEDIA_CONTROLLER
+static inline void media_request_object_get(struct media_request_object *obj)
+{
+	kref_get(&obj->kref);
+}
+
+/**
+ * media_request_object_put - Put a media request object
+ *
+ * @obj: The object
+ *
+ * Put 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 a media request object
+ *
+ * Initialise a media request object. The object will be released using the
+ * release callback of the ops once it has no references (this function
+ * initialises references to one).
+ */
+void media_request_object_init(struct media_request_object *obj);
+
+/**
+ * media_request_object_bind - Bind a media request object to a request
+ */
+void media_request_object_bind(struct media_request *req,
+			       const struct media_request_object_ops *ops,
+			       void *priv,
+			       struct media_request_object *obj);
+
+void media_request_object_unbind(struct media_request_object *obj);
+
+/**
+ * media_request_object_complete - Mark the media request object as complete
+ */
+void media_request_object_complete(struct media_request_object *obj);
+#else
+static inline void media_request_object_get(struct media_request_object *obj)
+{
+}
+
+static inline void media_request_object_put(struct media_request_object *obj)
+{
+}
+
+static inline void media_request_object_init(struct media_request_object *obj)
+{
+	obj->ops = NULL;
+	obj->req = NULL;
+}
+
+static inline void media_request_object_bind(struct media_request *req,
+			       const struct media_request_object_ops *ops,
+			       void *priv,
+			       struct media_request_object *obj)
+{
+}
+
+static inline void media_request_object_unbind(struct media_request_object *obj)
+{
+}
+
+static inline void media_request_object_complete(struct media_request_object *obj)
+{
+}
+#endif
 
 #endif
-- 
2.16.1

[RFCv9 PATCH 05/29] media-request: add request ioctls

[Hans Verkuil]

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

Implement the MEDIA_REQUEST_IOC_QUEUE and MEDIA_REQUEST_IOC_REINIT
ioctls.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/media-request.c | 80 +++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 78 insertions(+), 2 deletions(-)

diff --git a/drivers/media/media-request.c b/drivers/media/media-request.c
index 8135d9d32af9..3ee3b27fd644 100644
--- a/drivers/media/media-request.c
+++ b/drivers/media/media-request.c
@@ -105,10 +105,86 @@ static unsigned int media_request_poll(struct file *filp,
 	return 0;
 }
 
+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);
+
+	spin_lock_irqsave(&req->lock, flags);
+	if (req->state != MEDIA_REQUEST_STATE_IDLE) {
+		dev_dbg(mdev->dev,
+			"request: unable to queue %s, request in state %s\n",
+			req->debug_str, media_request_state_str(req->state));
+		spin_unlock_irqrestore(&req->lock, flags);
+		return -EINVAL;
+	}
+	req->state = MEDIA_REQUEST_STATE_QUEUEING;
+
+	spin_unlock_irqrestore(&req->lock, flags);
+
+	/*
+	 * 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);
+
+	ret = mdev->ops->req_queue(req);
+	spin_lock_irqsave(&req->lock, flags);
+	req->state = ret ? MEDIA_REQUEST_STATE_IDLE : MEDIA_REQUEST_STATE_QUEUED;
+	spin_unlock_irqrestore(&req->lock, flags);
+	mutex_unlock(&mdev->req_queue_mutex);
+
+	if (ret) {
+		dev_dbg(mdev->dev, "request: can't queue %s (%d)\n",
+			req->debug_str, ret);
+	} else {
+		media_request_get(req);
+	}
+
+	return ret;
+}
+
+static long media_request_ioctl_reinit(struct media_request *req)
+{
+	struct media_device *mdev = req->mdev;
+	unsigned long flags;
+
+	spin_lock_irqsave(&req->lock, flags);
+	if (req->state != MEDIA_REQUEST_STATE_IDLE &&
+	    req->state != MEDIA_REQUEST_STATE_COMPLETE) {
+		dev_dbg(mdev->dev,
+			"request: %s not in idle or complete state, cannot reinit\n",
+			req->debug_str);
+		spin_unlock_irqrestore(&req->lock, flags);
+		return -EINVAL;
+	}
+	req->state = MEDIA_REQUEST_STATE_CLEANING;
+	spin_unlock_irqrestore(&req->lock, flags);
+
+	media_request_clean(req);
+
+	spin_lock_irqsave(&req->lock, flags);
+	req->state = MEDIA_REQUEST_STATE_IDLE;
+	spin_unlock_irqrestore(&req->lock, flags);
+	return 0;
+}
+
 static long media_request_ioctl(struct file *filp, unsigned int cmd,
-				unsigned long __arg)
+				unsigned long arg)
 {
-	return -ENOIOCTLCMD;
+	struct media_request *req = filp->private_data;
+
+	switch (cmd) {
+	case MEDIA_REQUEST_IOC_QUEUE:
+		return media_request_ioctl_queue(req);
+	case MEDIA_REQUEST_IOC_REINIT:
+		return media_request_ioctl_reinit(req);
+	default:
+		return -ENOIOCTLCMD;
+	}
 }
 
 static const struct file_operations request_fops = {
-- 
2.16.1

[RFCv9 PATCH 06/29] media-request: add media_request_find

[Hans Verkuil]

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

Add media_request_find() to find a request based on the file
descriptor.

The caller has to call media_request_put() for the returned
request since this function increments the refcount.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/media-request.c | 47 +++++++++++++++++++++++++++++++++++++++++++
 include/media/media-request.h |  9 +++++++++
 2 files changed, 56 insertions(+)

diff --git a/drivers/media/media-request.c b/drivers/media/media-request.c
index 3ee3b27fd644..d54fd353d8a6 100644
--- a/drivers/media/media-request.c
+++ b/drivers/media/media-request.c
@@ -194,6 +194,53 @@ static const struct file_operations request_fops = {
 	.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_fd)
+{
+	struct file *filp;
+	struct media_request *req;
+
+	if (!mdev || !mdev->ops || !mdev->ops->req_queue)
+		return ERR_PTR(-ENOENT);
+
+	filp = fget(request_fd);
+	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_alloc *alloc)
 {
diff --git a/include/media/media-request.h b/include/media/media-request.h
index baed99eb1279..c01b05570a31 100644
--- a/include/media/media-request.h
+++ b/include/media/media-request.h
@@ -57,6 +57,9 @@ static inline void media_request_get(struct media_request *req)
 
 void media_request_put(struct media_request *req);
 
+struct media_request *
+media_request_find(struct media_device *mdev, int request_fd);
+
 int media_request_alloc(struct media_device *mdev,
 			struct media_request_alloc *alloc);
 #else
@@ -67,6 +70,12 @@ static inline void media_request_get(struct media_request *req)
 static inline void media_request_put(struct media_request *req)
 {
 }
+
+static inline struct media_request *
+media_request_find(struct media_device *mdev, int request_fd)
+{
+	return ERR_PTR(-ENOENT);
+}
 #endif
 
 struct media_request_object_ops {
-- 
2.16.1

[RFCv9 PATCH 07/29] media-request: add media_request_object_find

[Hans Verkuil]

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

Add media_request_object_find to find a request object inside a
request based on ops and/or priv values.

Objects of the same type (vb2 buffer, control handler) will have
the same ops value. And objects that refer to the same 'parent'
object (e.g. the v4l2_ctrl_handler that has the current driver
state) will have the same priv value.

The caller has to call media_request_object_put() for the returned
object since this function increments the refcount.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/media-request.c | 26 ++++++++++++++++++++++++++
 include/media/media-request.h | 25 +++++++++++++++++++++++++
 2 files changed, 51 insertions(+)

diff --git a/drivers/media/media-request.c b/drivers/media/media-request.c
index d54fd353d8a6..10a05dd7b571 100644
--- a/drivers/media/media-request.c
+++ b/drivers/media/media-request.c
@@ -309,6 +309,32 @@ static void media_request_object_release(struct kref *kref)
 	obj->ops->release(obj);
 }
 
+struct media_request_object *
+media_request_object_find(struct media_request *req,
+			  const struct media_request_object_ops *ops,
+			  void *priv)
+{
+	struct media_request_object *obj;
+	struct media_request_object *found = NULL;
+	unsigned long flags;
+
+	if (!ops && !priv)
+		return NULL;
+
+	spin_lock_irqsave(&req->lock, flags);
+	list_for_each_entry(obj, &req->objects, list) {
+		if ((!ops || obj->ops == ops) &&
+		    (!priv || obj->priv == priv)) {
+			media_request_object_get(obj);
+			found = obj;
+			break;
+		}
+	}
+	spin_unlock_irqrestore(&req->lock, flags);
+	return found;
+}
+EXPORT_SYMBOL_GPL(media_request_object_find);
+
 void media_request_object_put(struct media_request_object *obj)
 {
 	kref_put(&obj->kref, media_request_object_release);
diff --git a/include/media/media-request.h b/include/media/media-request.h
index c01b05570a31..570f3a205776 100644
--- a/include/media/media-request.h
+++ b/include/media/media-request.h
@@ -122,6 +122,23 @@ static inline void media_request_object_get(struct media_request_object *obj)
  */
 void media_request_object_put(struct media_request_object *obj);
 
+/**
+ * media_request_object_find - Find an object in a request
+ *
+ * @ops: Find an object with this ops value, may be NULL.
+ * @priv: Find an object with this priv value, may be NULL.
+ *
+ * At least one of @ops and @priv must be non-NULL. If one of
+ * these is NULL, then skip checking for that field.
+ *
+ * Returns NULL if not found or the object (the refcount is increased
+ * in that case).
+ */
+struct media_request_object *
+media_request_object_find(struct media_request *req,
+			  const struct media_request_object_ops *ops,
+			  void *priv);
+
 /**
  * media_request_object_init - Initialise a media request object
  *
@@ -154,6 +171,14 @@ static inline void media_request_object_put(struct media_request_object *obj)
 {
 }
 
+static inline struct media_request_object *
+media_request_object_find(struct media_request *req,
+			  const struct media_request_object_ops *ops,
+			  void *priv)
+{
+	return NULL;
+}
+
 static inline void media_request_object_init(struct media_request_object *obj)
 {
 	obj->ops = NULL;
-- 
2.16.1

[RFCv9 PATCH 08/29] v4l2-ctrls: v4l2_ctrl_add_handler: add from_other_dev

[Hans Verkuil]

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

Add a 'bool from_other_dev' argument: set to true if the two
handlers refer to different devices (e.g. it is true when
inheriting controls from a subdev into a main v4l2 bridge
driver).

This will be used later when implementing support for the
request API since we need to skip such controls.

TODO: check drivers/staging/media/imx/imx-media-fim.c change.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Signed-off-by: Alexandre Courbot <acourbot@chromium.org>
---
 drivers/media/dvb-frontends/rtl2832_sdr.c        |  5 +--
 drivers/media/pci/bt8xx/bttv-driver.c            |  2 +-
 drivers/media/pci/cx23885/cx23885-417.c          |  2 +-
 drivers/media/pci/cx88/cx88-blackbird.c          |  2 +-
 drivers/media/pci/cx88/cx88-video.c              |  2 +-
 drivers/media/pci/saa7134/saa7134-empress.c      |  4 +--
 drivers/media/pci/saa7134/saa7134-video.c        |  2 +-
 drivers/media/platform/exynos4-is/fimc-capture.c |  2 +-
 drivers/media/platform/rcar-vin/rcar-v4l2.c      |  3 +-
 drivers/media/platform/rcar_drif.c               |  2 +-
 drivers/media/platform/soc_camera/soc_camera.c   |  3 +-
 drivers/media/platform/vivid/vivid-ctrls.c       | 46 ++++++++++++------------
 drivers/media/usb/cx231xx/cx231xx-417.c          |  2 +-
 drivers/media/usb/cx231xx/cx231xx-video.c        |  4 +--
 drivers/media/usb/msi2500/msi2500.c              |  2 +-
 drivers/media/usb/tm6000/tm6000-video.c          |  2 +-
 drivers/media/v4l2-core/v4l2-ctrls.c             | 11 +++---
 drivers/media/v4l2-core/v4l2-device.c            |  3 +-
 drivers/staging/media/imx/imx-media-dev.c        |  2 +-
 drivers/staging/media/imx/imx-media-fim.c        |  2 +-
 include/media/v4l2-ctrls.h                       |  8 ++++-
 21 files changed, 62 insertions(+), 49 deletions(-)

diff --git a/drivers/media/dvb-frontends/rtl2832_sdr.c b/drivers/media/dvb-frontends/rtl2832_sdr.c
index c6e78d870ccd..6064d28224e8 100644
--- a/drivers/media/dvb-frontends/rtl2832_sdr.c
+++ b/drivers/media/dvb-frontends/rtl2832_sdr.c
@@ -1394,7 +1394,8 @@ static int rtl2832_sdr_probe(struct platform_device *pdev)
 	case RTL2832_SDR_TUNER_E4000:
 		v4l2_ctrl_handler_init(&dev->hdl, 9);
 		if (subdev)
-			v4l2_ctrl_add_handler(&dev->hdl, subdev->ctrl_handler, NULL);
+			v4l2_ctrl_add_handler(&dev->hdl, subdev->ctrl_handler,
+					      NULL, true);
 		break;
 	case RTL2832_SDR_TUNER_R820T:
 	case RTL2832_SDR_TUNER_R828D:
@@ -1423,7 +1424,7 @@ static int rtl2832_sdr_probe(struct platform_device *pdev)
 		v4l2_ctrl_handler_init(&dev->hdl, 2);
 		if (subdev)
 			v4l2_ctrl_add_handler(&dev->hdl, subdev->ctrl_handler,
-					      NULL);
+					      NULL, true);
 		break;
 	default:
 		v4l2_ctrl_handler_init(&dev->hdl, 0);
diff --git a/drivers/media/pci/bt8xx/bttv-driver.c b/drivers/media/pci/bt8xx/bttv-driver.c
index f697698fe38d..cdcb36d8c5c3 100644
--- a/drivers/media/pci/bt8xx/bttv-driver.c
+++ b/drivers/media/pci/bt8xx/bttv-driver.c
@@ -4211,7 +4211,7 @@ static int bttv_probe(struct pci_dev *dev, const struct pci_device_id *pci_id)
 	/* register video4linux + input */
 	if (!bttv_tvcards[btv->c.type].no_video) {
 		v4l2_ctrl_add_handler(&btv->radio_ctrl_handler, hdl,
-				v4l2_ctrl_radio_filter);
+				v4l2_ctrl_radio_filter, false);
 		if (btv->radio_ctrl_handler.error) {
 			result = btv->radio_ctrl_handler.error;
 			goto fail2;
diff --git a/drivers/media/pci/cx23885/cx23885-417.c b/drivers/media/pci/cx23885/cx23885-417.c
index a71f3c7569ce..762823871c78 100644
--- a/drivers/media/pci/cx23885/cx23885-417.c
+++ b/drivers/media/pci/cx23885/cx23885-417.c
@@ -1527,7 +1527,7 @@ int cx23885_417_register(struct cx23885_dev *dev)
 	dev->cxhdl.priv = dev;
 	dev->cxhdl.func = cx23885_api_func;
 	cx2341x_handler_set_50hz(&dev->cxhdl, tsport->height == 576);
-	v4l2_ctrl_add_handler(&dev->ctrl_handler, &dev->cxhdl.hdl, NULL);
+	v4l2_ctrl_add_handler(&dev->ctrl_handler, &dev->cxhdl.hdl, NULL, false);
 
 	/* Allocate and initialize V4L video device */
 	dev->v4l_device = cx23885_video_dev_alloc(tsport,
diff --git a/drivers/media/pci/cx88/cx88-blackbird.c b/drivers/media/pci/cx88/cx88-blackbird.c
index 0e0952e60795..39f69d89a663 100644
--- a/drivers/media/pci/cx88/cx88-blackbird.c
+++ b/drivers/media/pci/cx88/cx88-blackbird.c
@@ -1183,7 +1183,7 @@ static int cx8802_blackbird_probe(struct cx8802_driver *drv)
 	err = cx2341x_handler_init(&dev->cxhdl, 36);
 	if (err)
 		goto fail_core;
-	v4l2_ctrl_add_handler(&dev->cxhdl.hdl, &core->video_hdl, NULL);
+	v4l2_ctrl_add_handler(&dev->cxhdl.hdl, &core->video_hdl, NULL, false);
 
 	/* blackbird stuff */
 	pr_info("cx23416 based mpeg encoder (blackbird reference design)\n");
diff --git a/drivers/media/pci/cx88/cx88-video.c b/drivers/media/pci/cx88/cx88-video.c
index 9be682cdb644..e35bfa03a1e2 100644
--- a/drivers/media/pci/cx88/cx88-video.c
+++ b/drivers/media/pci/cx88/cx88-video.c
@@ -1378,7 +1378,7 @@ static int cx8800_initdev(struct pci_dev *pci_dev,
 		if (vc->id == V4L2_CID_CHROMA_AGC)
 			core->chroma_agc = vc;
 	}
-	v4l2_ctrl_add_handler(&core->video_hdl, &core->audio_hdl, NULL);
+	v4l2_ctrl_add_handler(&core->video_hdl, &core->audio_hdl, NULL, false);
 
 	/* load and configure helper modules */
 
diff --git a/drivers/media/pci/saa7134/saa7134-empress.c b/drivers/media/pci/saa7134/saa7134-empress.c
index 66acfd35ffc6..fc75ce00dbf8 100644
--- a/drivers/media/pci/saa7134/saa7134-empress.c
+++ b/drivers/media/pci/saa7134/saa7134-empress.c
@@ -265,9 +265,9 @@ static int empress_init(struct saa7134_dev *dev)
 		 "%s empress (%s)", dev->name,
 		 saa7134_boards[dev->board].name);
 	v4l2_ctrl_handler_init(hdl, 21);
-	v4l2_ctrl_add_handler(hdl, &dev->ctrl_handler, empress_ctrl_filter);
+	v4l2_ctrl_add_handler(hdl, &dev->ctrl_handler, empress_ctrl_filter, false);
 	if (dev->empress_sd)
-		v4l2_ctrl_add_handler(hdl, dev->empress_sd->ctrl_handler, NULL);
+		v4l2_ctrl_add_handler(hdl, dev->empress_sd->ctrl_handler, NULL, true);
 	if (hdl->error) {
 		video_device_release(dev->empress_dev);
 		return hdl->error;
diff --git a/drivers/media/pci/saa7134/saa7134-video.c b/drivers/media/pci/saa7134/saa7134-video.c
index 4f1091a11e91..d478c470b975 100644
--- a/drivers/media/pci/saa7134/saa7134-video.c
+++ b/drivers/media/pci/saa7134/saa7134-video.c
@@ -2136,7 +2136,7 @@ int saa7134_video_init1(struct saa7134_dev *dev)
 		hdl = &dev->radio_ctrl_handler;
 		v4l2_ctrl_handler_init(hdl, 2);
 		v4l2_ctrl_add_handler(hdl, &dev->ctrl_handler,
-				v4l2_ctrl_radio_filter);
+				v4l2_ctrl_radio_filter, false);
 		if (hdl->error)
 			return hdl->error;
 	}
diff --git a/drivers/media/platform/exynos4-is/fimc-capture.c b/drivers/media/platform/exynos4-is/fimc-capture.c
index a3cdac188190..2164375f0ee0 100644
--- a/drivers/media/platform/exynos4-is/fimc-capture.c
+++ b/drivers/media/platform/exynos4-is/fimc-capture.c
@@ -1424,7 +1424,7 @@ static int fimc_link_setup(struct media_entity *entity,
 		return 0;
 
 	return v4l2_ctrl_add_handler(&vc->ctx->ctrls.handler,
-				     sensor->ctrl_handler, NULL);
+				     sensor->ctrl_handler, NULL, true);
 }
 
 static const struct media_entity_operations fimc_sd_media_ops = {
diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index b479b882da12..90246113fa03 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -900,7 +900,8 @@ int rvin_v4l2_probe(struct rvin_dev *vin)
 	if (ret < 0)
 		return ret;
 
-	ret = v4l2_ctrl_add_handler(&vin->ctrl_handler, sd->ctrl_handler, NULL);
+	ret = v4l2_ctrl_add_handler(&vin->ctrl_handler, sd->ctrl_handler,
+				    NULL, true);
 	if (ret < 0)
 		return ret;
 
diff --git a/drivers/media/platform/rcar_drif.c b/drivers/media/platform/rcar_drif.c
index dc7e280c91b4..159c7d2c2066 100644
--- a/drivers/media/platform/rcar_drif.c
+++ b/drivers/media/platform/rcar_drif.c
@@ -1168,7 +1168,7 @@ static int rcar_drif_notify_complete(struct v4l2_async_notifier *notifier)
 	}
 
 	ret = v4l2_ctrl_add_handler(&sdr->ctrl_hdl,
-				    sdr->ep.subdev->ctrl_handler, NULL);
+				    sdr->ep.subdev->ctrl_handler, NULL, true);
 	if (ret) {
 		rdrif_err(sdr, "failed: ctrl add hdlr ret %d\n", ret);
 		goto error;
diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c
index 69f0d8e80bd8..e6787abc34ae 100644
--- a/drivers/media/platform/soc_camera/soc_camera.c
+++ b/drivers/media/platform/soc_camera/soc_camera.c
@@ -1180,7 +1180,8 @@ static int soc_camera_probe_finish(struct soc_camera_device *icd)
 
 	v4l2_subdev_call(sd, video, g_tvnorms, &icd->vdev->tvnorms);
 
-	ret = v4l2_ctrl_add_handler(&icd->ctrl_handler, sd->ctrl_handler, NULL);
+	ret = v4l2_ctrl_add_handler(&icd->ctrl_handler, sd->ctrl_handler,
+				    NULL, true);
 	if (ret < 0)
 		return ret;
 
diff --git a/drivers/media/platform/vivid/vivid-ctrls.c b/drivers/media/platform/vivid/vivid-ctrls.c
index 6b0bfa091592..f369b94ad7ff 100644
--- a/drivers/media/platform/vivid/vivid-ctrls.c
+++ b/drivers/media/platform/vivid/vivid-ctrls.c
@@ -1662,59 +1662,59 @@ int vivid_create_controls(struct vivid_dev *dev, bool show_ccs_cap,
 		v4l2_ctrl_auto_cluster(2, &dev->autogain, 0, true);
 
 	if (dev->has_vid_cap) {
-		v4l2_ctrl_add_handler(hdl_vid_cap, hdl_user_gen, NULL);
-		v4l2_ctrl_add_handler(hdl_vid_cap, hdl_user_vid, NULL);
-		v4l2_ctrl_add_handler(hdl_vid_cap, hdl_user_aud, NULL);
-		v4l2_ctrl_add_handler(hdl_vid_cap, hdl_streaming, NULL);
-		v4l2_ctrl_add_handler(hdl_vid_cap, hdl_sdtv_cap, NULL);
-		v4l2_ctrl_add_handler(hdl_vid_cap, hdl_loop_cap, NULL);
-		v4l2_ctrl_add_handler(hdl_vid_cap, hdl_fb, NULL);
+		v4l2_ctrl_add_handler(hdl_vid_cap, hdl_user_gen, NULL, false);
+		v4l2_ctrl_add_handler(hdl_vid_cap, hdl_user_vid, NULL, false);
+		v4l2_ctrl_add_handler(hdl_vid_cap, hdl_user_aud, NULL, false);
+		v4l2_ctrl_add_handler(hdl_vid_cap, hdl_streaming, NULL, false);
+		v4l2_ctrl_add_handler(hdl_vid_cap, hdl_sdtv_cap, NULL, false);
+		v4l2_ctrl_add_handler(hdl_vid_cap, hdl_loop_cap, NULL, false);
+		v4l2_ctrl_add_handler(hdl_vid_cap, hdl_fb, NULL, false);
 		if (hdl_vid_cap->error)
 			return hdl_vid_cap->error;
 		dev->vid_cap_dev.ctrl_handler = hdl_vid_cap;
 	}
 	if (dev->has_vid_out) {
-		v4l2_ctrl_add_handler(hdl_vid_out, hdl_user_gen, NULL);
-		v4l2_ctrl_add_handler(hdl_vid_out, hdl_user_aud, NULL);
-		v4l2_ctrl_add_handler(hdl_vid_out, hdl_streaming, NULL);
-		v4l2_ctrl_add_handler(hdl_vid_out, hdl_fb, NULL);
+		v4l2_ctrl_add_handler(hdl_vid_out, hdl_user_gen, NULL, false);
+		v4l2_ctrl_add_handler(hdl_vid_out, hdl_user_aud, NULL, false);
+		v4l2_ctrl_add_handler(hdl_vid_out, hdl_streaming, NULL, false);
+		v4l2_ctrl_add_handler(hdl_vid_out, hdl_fb, NULL, false);
 		if (hdl_vid_out->error)
 			return hdl_vid_out->error;
 		dev->vid_out_dev.ctrl_handler = hdl_vid_out;
 	}
 	if (dev->has_vbi_cap) {
-		v4l2_ctrl_add_handler(hdl_vbi_cap, hdl_user_gen, NULL);
-		v4l2_ctrl_add_handler(hdl_vbi_cap, hdl_streaming, NULL);
-		v4l2_ctrl_add_handler(hdl_vbi_cap, hdl_sdtv_cap, NULL);
-		v4l2_ctrl_add_handler(hdl_vbi_cap, hdl_loop_cap, NULL);
+		v4l2_ctrl_add_handler(hdl_vbi_cap, hdl_user_gen, NULL, false);
+		v4l2_ctrl_add_handler(hdl_vbi_cap, hdl_streaming, NULL, false);
+		v4l2_ctrl_add_handler(hdl_vbi_cap, hdl_sdtv_cap, NULL, false);
+		v4l2_ctrl_add_handler(hdl_vbi_cap, hdl_loop_cap, NULL, false);
 		if (hdl_vbi_cap->error)
 			return hdl_vbi_cap->error;
 		dev->vbi_cap_dev.ctrl_handler = hdl_vbi_cap;
 	}
 	if (dev->has_vbi_out) {
-		v4l2_ctrl_add_handler(hdl_vbi_out, hdl_user_gen, NULL);
-		v4l2_ctrl_add_handler(hdl_vbi_out, hdl_streaming, NULL);
+		v4l2_ctrl_add_handler(hdl_vbi_out, hdl_user_gen, NULL, false);
+		v4l2_ctrl_add_handler(hdl_vbi_out, hdl_streaming, NULL, false);
 		if (hdl_vbi_out->error)
 			return hdl_vbi_out->error;
 		dev->vbi_out_dev.ctrl_handler = hdl_vbi_out;
 	}
 	if (dev->has_radio_rx) {
-		v4l2_ctrl_add_handler(hdl_radio_rx, hdl_user_gen, NULL);
-		v4l2_ctrl_add_handler(hdl_radio_rx, hdl_user_aud, NULL);
+		v4l2_ctrl_add_handler(hdl_radio_rx, hdl_user_gen, NULL, false);
+		v4l2_ctrl_add_handler(hdl_radio_rx, hdl_user_aud, NULL, false);
 		if (hdl_radio_rx->error)
 			return hdl_radio_rx->error;
 		dev->radio_rx_dev.ctrl_handler = hdl_radio_rx;
 	}
 	if (dev->has_radio_tx) {
-		v4l2_ctrl_add_handler(hdl_radio_tx, hdl_user_gen, NULL);
-		v4l2_ctrl_add_handler(hdl_radio_tx, hdl_user_aud, NULL);
+		v4l2_ctrl_add_handler(hdl_radio_tx, hdl_user_gen, NULL, false);
+		v4l2_ctrl_add_handler(hdl_radio_tx, hdl_user_aud, NULL, false);
 		if (hdl_radio_tx->error)
 			return hdl_radio_tx->error;
 		dev->radio_tx_dev.ctrl_handler = hdl_radio_tx;
 	}
 	if (dev->has_sdr_cap) {
-		v4l2_ctrl_add_handler(hdl_sdr_cap, hdl_user_gen, NULL);
-		v4l2_ctrl_add_handler(hdl_sdr_cap, hdl_streaming, NULL);
+		v4l2_ctrl_add_handler(hdl_sdr_cap, hdl_user_gen, NULL, false);
+		v4l2_ctrl_add_handler(hdl_sdr_cap, hdl_streaming, NULL, false);
 		if (hdl_sdr_cap->error)
 			return hdl_sdr_cap->error;
 		dev->sdr_cap_dev.ctrl_handler = hdl_sdr_cap;
diff --git a/drivers/media/usb/cx231xx/cx231xx-417.c b/drivers/media/usb/cx231xx/cx231xx-417.c
index b80e6857e2eb..fca16cf8b3bf 100644
--- a/drivers/media/usb/cx231xx/cx231xx-417.c
+++ b/drivers/media/usb/cx231xx/cx231xx-417.c
@@ -1991,7 +1991,7 @@ int cx231xx_417_register(struct cx231xx *dev)
 	dev->mpeg_ctrl_handler.ops = &cx231xx_ops;
 	if (dev->sd_cx25840)
 		v4l2_ctrl_add_handler(&dev->mpeg_ctrl_handler.hdl,
-				dev->sd_cx25840->ctrl_handler, NULL);
+				dev->sd_cx25840->ctrl_handler, NULL, false);
 	if (dev->mpeg_ctrl_handler.hdl.error) {
 		err = dev->mpeg_ctrl_handler.hdl.error;
 		dprintk(3, "%s: can't add cx25840 controls\n", dev->name);
diff --git a/drivers/media/usb/cx231xx/cx231xx-video.c b/drivers/media/usb/cx231xx/cx231xx-video.c
index f7fcd733a2ca..2dedb18f63a0 100644
--- a/drivers/media/usb/cx231xx/cx231xx-video.c
+++ b/drivers/media/usb/cx231xx/cx231xx-video.c
@@ -2204,10 +2204,10 @@ int cx231xx_register_analog_devices(struct cx231xx *dev)
 
 	if (dev->sd_cx25840) {
 		v4l2_ctrl_add_handler(&dev->ctrl_handler,
-				dev->sd_cx25840->ctrl_handler, NULL);
+				dev->sd_cx25840->ctrl_handler, NULL, true);
 		v4l2_ctrl_add_handler(&dev->radio_ctrl_handler,
 				dev->sd_cx25840->ctrl_handler,
-				v4l2_ctrl_radio_filter);
+				v4l2_ctrl_radio_filter, true);
 	}
 
 	if (dev->ctrl_handler.error)
diff --git a/drivers/media/usb/msi2500/msi2500.c b/drivers/media/usb/msi2500/msi2500.c
index 65ef755adfdc..4aacd77a5d58 100644
--- a/drivers/media/usb/msi2500/msi2500.c
+++ b/drivers/media/usb/msi2500/msi2500.c
@@ -1278,7 +1278,7 @@ static int msi2500_probe(struct usb_interface *intf,
 	}
 
 	/* currently all controls are from subdev */
-	v4l2_ctrl_add_handler(&dev->hdl, sd->ctrl_handler, NULL);
+	v4l2_ctrl_add_handler(&dev->hdl, sd->ctrl_handler, NULL, true);
 
 	dev->v4l2_dev.ctrl_handler = &dev->hdl;
 	dev->vdev.v4l2_dev = &dev->v4l2_dev;
diff --git a/drivers/media/usb/tm6000/tm6000-video.c b/drivers/media/usb/tm6000/tm6000-video.c
index b2399d4266da..99d6b0da8b3d 100644
--- a/drivers/media/usb/tm6000/tm6000-video.c
+++ b/drivers/media/usb/tm6000/tm6000-video.c
@@ -1622,7 +1622,7 @@ int tm6000_v4l2_register(struct tm6000_core *dev)
 	v4l2_ctrl_new_std(&dev->ctrl_handler, &tm6000_ctrl_ops,
 			V4L2_CID_HUE, -128, 127, 1, 0);
 	v4l2_ctrl_add_handler(&dev->ctrl_handler,
-			&dev->radio_ctrl_handler, NULL);
+			&dev->radio_ctrl_handler, NULL, false);
 
 	if (dev->radio_ctrl_handler.error)
 		ret = dev->radio_ctrl_handler.error;
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index d29e45516eb7..aa1dd2015e84 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -1995,7 +1995,8 @@ EXPORT_SYMBOL(v4l2_ctrl_find);
 
 /* Allocate a new v4l2_ctrl_ref and hook it into the handler. */
 static int handler_new_ref(struct v4l2_ctrl_handler *hdl,
-			   struct v4l2_ctrl *ctrl)
+			   struct v4l2_ctrl *ctrl,
+			   bool from_other_dev)
 {
 	struct v4l2_ctrl_ref *ref;
 	struct v4l2_ctrl_ref *new_ref;
@@ -2019,6 +2020,7 @@ static int handler_new_ref(struct v4l2_ctrl_handler *hdl,
 	if (!new_ref)
 		return handler_set_err(hdl, -ENOMEM);
 	new_ref->ctrl = ctrl;
+	new_ref->from_other_dev = from_other_dev;
 	if (ctrl->handler == hdl) {
 		/* By default each control starts in a cluster of its own.
 		   new_ref->ctrl is basically a cluster array with one
@@ -2199,7 +2201,7 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
 		ctrl->type_ops->init(ctrl, idx, ctrl->p_new);
 	}
 
-	if (handler_new_ref(hdl, ctrl)) {
+	if (handler_new_ref(hdl, ctrl, false)) {
 		kvfree(ctrl);
 		return NULL;
 	}
@@ -2368,7 +2370,8 @@ EXPORT_SYMBOL(v4l2_ctrl_new_int_menu);
 /* Add the controls from another handler to our own. */
 int v4l2_ctrl_add_handler(struct v4l2_ctrl_handler *hdl,
 			  struct v4l2_ctrl_handler *add,
-			  bool (*filter)(const struct v4l2_ctrl *ctrl))
+			  bool (*filter)(const struct v4l2_ctrl *ctrl),
+			  bool from_other_dev)
 {
 	struct v4l2_ctrl_ref *ref;
 	int ret = 0;
@@ -2391,7 +2394,7 @@ int v4l2_ctrl_add_handler(struct v4l2_ctrl_handler *hdl,
 		/* Filter any unwanted controls */
 		if (filter && !filter(ctrl))
 			continue;
-		ret = handler_new_ref(hdl, ctrl);
+		ret = handler_new_ref(hdl, ctrl, from_other_dev);
 		if (ret)
 			break;
 	}
diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c
index 937c6de85606..8391a7f0895b 100644
--- a/drivers/media/v4l2-core/v4l2-device.c
+++ b/drivers/media/v4l2-core/v4l2-device.c
@@ -178,7 +178,8 @@ int v4l2_device_register_subdev(struct v4l2_device *v4l2_dev,
 
 	sd->v4l2_dev = v4l2_dev;
 	/* This just returns 0 if either of the two args is NULL */
-	err = v4l2_ctrl_add_handler(v4l2_dev->ctrl_handler, sd->ctrl_handler, NULL);
+	err = v4l2_ctrl_add_handler(v4l2_dev->ctrl_handler, sd->ctrl_handler,
+				    NULL, true);
 	if (err)
 		goto error_module;
 
diff --git a/drivers/staging/media/imx/imx-media-dev.c b/drivers/staging/media/imx/imx-media-dev.c
index 289d775c4820..08799beaea42 100644
--- a/drivers/staging/media/imx/imx-media-dev.c
+++ b/drivers/staging/media/imx/imx-media-dev.c
@@ -391,7 +391,7 @@ static int imx_media_inherit_controls(struct imx_media_dev *imxmd,
 
 		ret = v4l2_ctrl_add_handler(vfd->ctrl_handler,
 					    sd->ctrl_handler,
-					    NULL);
+					    NULL, true);
 		if (ret)
 			return ret;
 	}
diff --git a/drivers/staging/media/imx/imx-media-fim.c b/drivers/staging/media/imx/imx-media-fim.c
index 6df189135db8..8cf773eef9da 100644
--- a/drivers/staging/media/imx/imx-media-fim.c
+++ b/drivers/staging/media/imx/imx-media-fim.c
@@ -463,7 +463,7 @@ int imx_media_fim_add_controls(struct imx_media_fim *fim)
 {
 	/* add the FIM controls to the calling subdev ctrl handler */
 	return v4l2_ctrl_add_handler(fim->sd->ctrl_handler,
-				     &fim->ctrl_handler, NULL);
+				     &fim->ctrl_handler, NULL, false);
 }
 EXPORT_SYMBOL_GPL(imx_media_fim_add_controls);
 
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index 5b445b5654f7..f8faa54b5e7e 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -247,6 +247,8 @@ struct v4l2_ctrl {
  * @ctrl:	The actual control information.
  * @helper:	Pointer to helper struct. Used internally in
  *		``prepare_ext_ctrls`` function at ``v4l2-ctrl.c``.
+ * @from_other_dev: If true, then @ctrl was defined in another
+ *		device then the &struct v4l2_ctrl_handler.
  *
  * Each control handler has a list of these refs. The list_head is used to
  * keep a sorted-by-control-ID list of all controls, while the next pointer
@@ -257,6 +259,7 @@ struct v4l2_ctrl_ref {
 	struct v4l2_ctrl_ref *next;
 	struct v4l2_ctrl *ctrl;
 	struct v4l2_ctrl_helper *helper;
+	bool from_other_dev;
 };
 
 /**
@@ -633,6 +636,8 @@ typedef bool (*v4l2_ctrl_filter)(const struct v4l2_ctrl *ctrl);
  * @add:	The control handler whose controls you want to add to
  *		the @hdl control handler.
  * @filter:	This function will filter which controls should be added.
+ * @from_other_dev: If true, then the controls in @add were defined in another
+ *		device then @hdl.
  *
  * Does nothing if either of the two handlers is a NULL pointer.
  * If @filter is NULL, then all controls are added. Otherwise only those
@@ -642,7 +647,8 @@ typedef bool (*v4l2_ctrl_filter)(const struct v4l2_ctrl *ctrl);
  */
 int v4l2_ctrl_add_handler(struct v4l2_ctrl_handler *hdl,
 			  struct v4l2_ctrl_handler *add,
-			  v4l2_ctrl_filter filter);
+			  v4l2_ctrl_filter filter,
+			  bool from_other_dev);
 
 /**
  * v4l2_ctrl_radio_filter() - Standard filter for radio controls.
-- 
2.16.1

[RFCv9 PATCH 09/29] v4l2-ctrls: prepare internal structs for request API

[Hans Verkuil]

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

Embed and initialize a media_request_object in struct v4l2_ctrl_handler.

Add a p_req field to struct v4l2_ctrl_ref that will store the
request value.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Signed-off-by: Alexandre Courbot <acourbot@chromium.org>
---
 drivers/media/v4l2-core/v4l2-ctrls.c | 1 +
 include/media/v4l2-ctrls.h           | 7 +++++++
 2 files changed, 8 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index aa1dd2015e84..d09f49530d9e 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -1880,6 +1880,7 @@ int v4l2_ctrl_handler_init_class(struct v4l2_ctrl_handler *hdl,
 				      sizeof(hdl->buckets[0]),
 				      GFP_KERNEL | __GFP_ZERO);
 	hdl->error = hdl->buckets ? 0 : -ENOMEM;
+	media_request_object_init(&hdl->req_obj);
 	return hdl->error;
 }
 EXPORT_SYMBOL(v4l2_ctrl_handler_init_class);
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index f8faa54b5e7e..89a985607126 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -20,6 +20,7 @@
 #include <linux/list.h>
 #include <linux/mutex.h>
 #include <linux/videodev2.h>
+#include <media/media-request.h>
 
 /* forward references */
 struct file;
@@ -249,6 +250,8 @@ struct v4l2_ctrl {
  *		``prepare_ext_ctrls`` function at ``v4l2-ctrl.c``.
  * @from_other_dev: If true, then @ctrl was defined in another
  *		device then the &struct v4l2_ctrl_handler.
+ * @p_req:	The request value. Only used if the control handler
+ *		is bound to a media request.
  *
  * Each control handler has a list of these refs. The list_head is used to
  * keep a sorted-by-control-ID list of all controls, while the next pointer
@@ -260,6 +263,7 @@ struct v4l2_ctrl_ref {
 	struct v4l2_ctrl *ctrl;
 	struct v4l2_ctrl_helper *helper;
 	bool from_other_dev;
+	union v4l2_ctrl_ptr p_req;
 };
 
 /**
@@ -283,6 +287,8 @@ struct v4l2_ctrl_ref {
  * @notify_priv: Passed as argument to the v4l2_ctrl notify callback.
  * @nr_of_buckets: Total number of buckets in the array.
  * @error:	The error code of the first failed control addition.
+ * @req_obj:	The &struct media_request_object, used to link into a
+ *		&struct media_request.
  */
 struct v4l2_ctrl_handler {
 	struct mutex _lock;
@@ -295,6 +301,7 @@ struct v4l2_ctrl_handler {
 	void *notify_priv;
 	u16 nr_of_buckets;
 	int error;
+	struct media_request_object req_obj;
 };
 
 /**
-- 
2.16.1

[RFCv9 PATCH 10/29] v4l2-ctrls: alloc memory for p_req

[Hans Verkuil]

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

To store request data the handler_new_ref() allocates memory
for it if needed.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/v4l2-core/v4l2-ctrls.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index d09f49530d9e..e8d6dee375ef 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -1997,6 +1997,7 @@ EXPORT_SYMBOL(v4l2_ctrl_find);
 /* Allocate a new v4l2_ctrl_ref and hook it into the handler. */
 static int handler_new_ref(struct v4l2_ctrl_handler *hdl,
 			   struct v4l2_ctrl *ctrl,
+			   struct v4l2_ctrl_ref **ctrl_ref,
 			   bool from_other_dev)
 {
 	struct v4l2_ctrl_ref *ref;
@@ -2004,6 +2005,10 @@ static int handler_new_ref(struct v4l2_ctrl_handler *hdl,
 	u32 id = ctrl->id;
 	u32 class_ctrl = V4L2_CTRL_ID2WHICH(id) | 1;
 	int bucket = id % hdl->nr_of_buckets;	/* which bucket to use */
+	unsigned int sz_extra = 0;
+
+	if (ctrl_ref)
+		*ctrl_ref = NULL;
 
 	/*
 	 * Automatically add the control class if it is not yet present and
@@ -2017,11 +2022,16 @@ static int handler_new_ref(struct v4l2_ctrl_handler *hdl,
 	if (hdl->error)
 		return hdl->error;
 
-	new_ref = kzalloc(sizeof(*new_ref), GFP_KERNEL);
+	if (hdl->req_obj.req)
+		sz_extra = ctrl->elems * ctrl->elem_size;
+	new_ref = kzalloc(sizeof(*new_ref) + sz_extra, GFP_KERNEL);
 	if (!new_ref)
 		return handler_set_err(hdl, -ENOMEM);
 	new_ref->ctrl = ctrl;
 	new_ref->from_other_dev = from_other_dev;
+	if (sz_extra)
+		new_ref->p_req.p = &new_ref[1];
+
 	if (ctrl->handler == hdl) {
 		/* By default each control starts in a cluster of its own.
 		   new_ref->ctrl is basically a cluster array with one
@@ -2061,6 +2071,8 @@ static int handler_new_ref(struct v4l2_ctrl_handler *hdl,
 	/* Insert the control node in the hash */
 	new_ref->next = hdl->buckets[bucket];
 	hdl->buckets[bucket] = new_ref;
+	if (ctrl_ref)
+		*ctrl_ref = new_ref;
 
 unlock:
 	mutex_unlock(hdl->lock);
@@ -2202,7 +2214,7 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
 		ctrl->type_ops->init(ctrl, idx, ctrl->p_new);
 	}
 
-	if (handler_new_ref(hdl, ctrl, false)) {
+	if (handler_new_ref(hdl, ctrl, NULL, false)) {
 		kvfree(ctrl);
 		return NULL;
 	}
@@ -2395,7 +2407,7 @@ int v4l2_ctrl_add_handler(struct v4l2_ctrl_handler *hdl,
 		/* Filter any unwanted controls */
 		if (filter && !filter(ctrl))
 			continue;
-		ret = handler_new_ref(hdl, ctrl, from_other_dev);
+		ret = handler_new_ref(hdl, ctrl, NULL, from_other_dev);
 		if (ret)
 			break;
 	}
-- 
2.16.1

[RFCv9 PATCH 11/29] v4l2-ctrls: use ref in helper instead of ctrl

[Hans Verkuil]

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

The next patch needs the reference to a control instead of the
control itself, so change struct v4l2_ctrl_helper accordingly.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Signed-off-by: Alexandre Courbot <acourbot@chromium.org>
---
 drivers/media/v4l2-core/v4l2-ctrls.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index e8d6dee375ef..fc13f328c6fc 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -37,8 +37,8 @@
 struct v4l2_ctrl_helper {
 	/* Pointer to the control reference of the master control */
 	struct v4l2_ctrl_ref *mref;
-	/* The control corresponding to the v4l2_ext_control ID field. */
-	struct v4l2_ctrl *ctrl;
+	/* The control ref corresponding to the v4l2_ext_control ID field. */
+	struct v4l2_ctrl_ref *ref;
 	/* v4l2_ext_control index of the next control belonging to the
 	   same cluster, or 0 if there isn't any. */
 	u32 next;
@@ -2887,6 +2887,7 @@ static int prepare_ext_ctrls(struct v4l2_ctrl_handler *hdl,
 		ref = find_ref_lock(hdl, id);
 		if (ref == NULL)
 			return -EINVAL;
+		h->ref = ref;
 		ctrl = ref->ctrl;
 		if (ctrl->flags & V4L2_CTRL_FLAG_DISABLED)
 			return -EINVAL;
@@ -2909,7 +2910,6 @@ static int prepare_ext_ctrls(struct v4l2_ctrl_handler *hdl,
 		}
 		/* Store the ref to the master control of the cluster */
 		h->mref = ref;
-		h->ctrl = ctrl;
 		/* Initially set next to 0, meaning that there is no other
 		   control in this helper array belonging to the same
 		   cluster */
@@ -2994,7 +2994,7 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct v4l2_ext_controls *cs
 	cs->error_idx = cs->count;
 
 	for (i = 0; !ret && i < cs->count; i++)
-		if (helpers[i].ctrl->flags & V4L2_CTRL_FLAG_WRITE_ONLY)
+		if (helpers[i].ref->ctrl->flags & V4L2_CTRL_FLAG_WRITE_ONLY)
 			ret = -EACCES;
 
 	for (i = 0; !ret && i < cs->count; i++) {
@@ -3029,7 +3029,7 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct v4l2_ext_controls *cs
 
 			do {
 				ret = ctrl_to_user(cs->controls + idx,
-						   helpers[idx].ctrl);
+						   helpers[idx].ref->ctrl);
 				idx = helpers[idx].next;
 			} while (!ret && idx);
 		}
@@ -3168,7 +3168,7 @@ static int validate_ctrls(struct v4l2_ext_controls *cs,
 
 	cs->error_idx = cs->count;
 	for (i = 0; i < cs->count; i++) {
-		struct v4l2_ctrl *ctrl = helpers[i].ctrl;
+		struct v4l2_ctrl *ctrl = helpers[i].ref->ctrl;
 		union v4l2_ctrl_ptr p_new;
 
 		cs->error_idx = i;
@@ -3280,7 +3280,7 @@ static int try_set_ext_ctrls(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
 			do {
 				/* Check if the auto control is part of the
 				   list, and remember the new value. */
-				if (helpers[tmp_idx].ctrl == master)
+				if (helpers[tmp_idx].ref->ctrl == master)
 					new_auto_val = cs->controls[tmp_idx].value;
 				tmp_idx = helpers[tmp_idx].next;
 			} while (tmp_idx);
@@ -3293,7 +3293,7 @@ static int try_set_ext_ctrls(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
 		/* Copy the new caller-supplied control values.
 		   user_to_new() sets 'is_new' to 1. */
 		do {
-			struct v4l2_ctrl *ctrl = helpers[idx].ctrl;
+			struct v4l2_ctrl *ctrl = helpers[idx].ref->ctrl;
 
 			ret = user_to_new(cs->controls + idx, ctrl);
 			if (!ret && ctrl->is_ptr)
@@ -3309,7 +3309,7 @@ static int try_set_ext_ctrls(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
 			idx = i;
 			do {
 				ret = new_to_user(cs->controls + idx,
-						helpers[idx].ctrl);
+						helpers[idx].ref->ctrl);
 				idx = helpers[idx].next;
 			} while (!ret && idx);
 		}
-- 
2.16.1

[RFCv9 PATCH 12/29] v4l2-ctrls: support g/s_ext_ctrls for requests

[Hans Verkuil]

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

The v4l2_g/s_ext_ctrls functions now support control handlers that
represent requests.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Signed-off-by: Alexandre Courbot <acourbot@chromium.org>
---
 drivers/media/v4l2-core/v4l2-ctrls.c | 37 ++++++++++++++++++++++++++++++++----
 1 file changed, 33 insertions(+), 4 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index fc13f328c6fc..80714c5bad8b 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -1647,6 +1647,13 @@ static int new_to_user(struct v4l2_ext_control *c,
 	return ptr_to_user(c, ctrl, ctrl->p_new);
 }
 
+/* Helper function: copy the request value back to the caller */
+static int req_to_user(struct v4l2_ext_control *c,
+		       struct v4l2_ctrl_ref *ref)
+{
+	return ptr_to_user(c, ref->ctrl, ref->p_req);
+}
+
 /* Helper function: copy the initial control value back to the caller */
 static int def_to_user(struct v4l2_ext_control *c, struct v4l2_ctrl *ctrl)
 {
@@ -1766,6 +1773,14 @@ static void cur_to_new(struct v4l2_ctrl *ctrl)
 	ptr_to_ptr(ctrl, ctrl->p_cur, ctrl->p_new);
 }
 
+/* Copy the new value to the request value */
+static void new_to_req(struct v4l2_ctrl_ref *ref)
+{
+	if (!ref)
+		return;
+	ptr_to_ptr(ref->ctrl, ref->ctrl->p_new, ref->p_req);
+}
+
 /* Return non-zero if one or more of the controls in the cluster has a new
    value that differs from the current value. */
 static int cluster_changed(struct v4l2_ctrl *master)
@@ -3002,7 +3017,8 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct v4l2_ext_controls *cs
 				    struct v4l2_ctrl *ctrl);
 		struct v4l2_ctrl *master;
 
-		ctrl_to_user = def_value ? def_to_user : cur_to_user;
+		ctrl_to_user = def_value ? def_to_user :
+			       (hdl->req_obj.req ? NULL : cur_to_user);
 
 		if (helpers[i].mref == NULL)
 			continue;
@@ -3028,8 +3044,12 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct v4l2_ext_controls *cs
 			u32 idx = i;
 
 			do {
-				ret = ctrl_to_user(cs->controls + idx,
-						   helpers[idx].ref->ctrl);
+				if (ctrl_to_user)
+					ret = ctrl_to_user(cs->controls + idx,
+						helpers[idx].ref->ctrl);
+				else
+					ret = req_to_user(cs->controls + idx,
+						helpers[idx].ref);
 				idx = helpers[idx].next;
 			} while (!ret && idx);
 		}
@@ -3302,7 +3322,16 @@ static int try_set_ext_ctrls(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
 		} while (!ret && idx);
 
 		if (!ret)
-			ret = try_or_set_cluster(fh, master, set, 0);
+			ret = try_or_set_cluster(fh, master,
+						 !hdl->req_obj.req && set, 0);
+		if (!ret && hdl->req_obj.req && set) {
+			for (j = 0; j < master->ncontrols; j++) {
+				struct v4l2_ctrl_ref *ref =
+					find_ref(hdl, master->cluster[j]->id);
+
+				new_to_req(ref);
+			}
+		}
 
 		/* Copy the new values back to userspace. */
 		if (!ret) {
-- 
2.16.1

[RFCv9 PATCH 13/29] v4l2-ctrls: add core request API

[Hans Verkuil]

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

Add the two core request functions:

v4l2_ctrl_request_setup() applies the contents of the request
to the current driver state.

v4l2_ctrl_request_complete() marks the request complete. After
this the contents is fixed.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/v4l2-core/v4l2-ctrls.c | 172 +++++++++++++++++++++++++++++++++++
 include/media/v4l2-ctrls.h           |   9 ++
 2 files changed, 181 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index 80714c5bad8b..62f91c0f1e5f 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -1781,6 +1781,14 @@ static void new_to_req(struct v4l2_ctrl_ref *ref)
 	ptr_to_ptr(ref->ctrl, ref->ctrl->p_new, ref->p_req);
 }
 
+/* Copy the request value to the new value */
+static void req_to_new(struct v4l2_ctrl_ref *ref)
+{
+	if (!ref)
+		return;
+	ptr_to_ptr(ref->ctrl, ref->p_req, ref->ctrl->p_new);
+}
+
 /* Return non-zero if one or more of the controls in the cluster has a new
    value that differs from the current value. */
 static int cluster_changed(struct v4l2_ctrl *master)
@@ -2831,6 +2839,78 @@ int v4l2_querymenu(struct v4l2_ctrl_handler *hdl, struct v4l2_querymenu *qm)
 }
 EXPORT_SYMBOL(v4l2_querymenu);
 
+static int v4l2_ctrl_request_clone(struct v4l2_ctrl_handler *hdl,
+			    const struct v4l2_ctrl_handler *from,
+			    bool (*filter)(const struct v4l2_ctrl *ctrl))
+{
+	struct v4l2_ctrl_ref *ref;
+	int err;
+
+	if (WARN_ON(!hdl || hdl == from))
+		return -EINVAL;
+
+	if (hdl->error)
+		return hdl->error;
+
+	WARN_ON(hdl->lock != &hdl->_lock);
+	v4l2_ctrl_handler_free(hdl);
+	err = v4l2_ctrl_handler_init(hdl, (from->nr_of_buckets - 1) * 8);
+	if (err)
+		return err;
+	if (!from)
+		return 0;
+
+	mutex_lock(from->lock);
+	list_for_each_entry(ref, &from->ctrl_refs, node) {
+		struct v4l2_ctrl *ctrl = ref->ctrl;
+		struct v4l2_ctrl_ref *new_ref;
+
+		/* Skip refs inherited from other devices */
+		if (ref->from_other_dev)
+			continue;
+		/* And buttons and control classes */
+		if (ctrl->type == V4L2_CTRL_TYPE_BUTTON ||
+		    ctrl->type == V4L2_CTRL_TYPE_CTRL_CLASS)
+			continue;
+		/* Filter any unwanted controls */
+		if (filter && !filter(ctrl))
+			continue;
+		err = handler_new_ref(hdl, ctrl, &new_ref, false);
+		if (err)
+			break;
+		if (from->req_obj.req)
+			ptr_to_ptr(ctrl, ref->p_req, new_ref->p_req);
+		else
+			ptr_to_ptr(ctrl, ctrl->p_cur, new_ref->p_req);
+	}
+	mutex_unlock(from->lock);
+	return err;
+}
+
+static void v4l2_ctrl_request_release(struct media_request_object *obj)
+{
+	struct v4l2_ctrl_handler *hdl =
+		container_of(obj, struct v4l2_ctrl_handler, req_obj);
+
+	v4l2_ctrl_handler_free(hdl);
+	kfree(hdl);
+}
+
+static const struct media_request_object_ops req_ops = {
+	.release = v4l2_ctrl_request_release,
+};
+
+static int v4l2_ctrl_request_bind(struct media_request *req,
+			   struct v4l2_ctrl_handler *hdl,
+			   struct v4l2_ctrl_handler *from,
+			   bool (*filter)(const struct v4l2_ctrl *ctrl))
+{
+	int ret = v4l2_ctrl_request_clone(hdl, from, filter);
+
+	if (!ret)
+		media_request_object_bind(req, &req_ops, from, &hdl->req_obj);
+	return ret;
+}
 
 /* Some general notes on the atomic requirements of VIDIOC_G/TRY/S_EXT_CTRLS:
 
@@ -3458,6 +3538,98 @@ int __v4l2_ctrl_s_ctrl_string(struct v4l2_ctrl *ctrl, const char *s)
 }
 EXPORT_SYMBOL(__v4l2_ctrl_s_ctrl_string);
 
+void v4l2_ctrl_request_complete(struct media_request *req,
+				struct v4l2_ctrl_handler *hdl)
+{
+	struct media_request_object *obj;
+
+	if (!req || !hdl)
+		return;
+
+	obj = media_request_object_find(req, &req_ops, hdl);
+	if (!obj)
+		return;
+
+	media_request_object_complete(obj);
+	media_request_object_put(obj);
+}
+EXPORT_SYMBOL(v4l2_ctrl_request_complete);
+
+void v4l2_ctrl_request_setup(struct media_request *req,
+			     struct v4l2_ctrl_handler *hdl)
+{
+	struct media_request_object *obj;
+	struct v4l2_ctrl_ref *ref;
+
+	if (!req || !hdl)
+		return;
+
+	obj = media_request_object_find(req, &req_ops, hdl);
+	if (!obj)
+		return;
+	if (obj->completed) {
+		media_request_object_put(obj);
+		return;
+	}
+	hdl = container_of(obj, struct v4l2_ctrl_handler, req_obj);
+
+	mutex_lock(hdl->lock);
+
+	list_for_each_entry(ref, &hdl->ctrl_refs, node)
+		ref->done = false;
+
+	list_for_each_entry(ref, &hdl->ctrl_refs, node) {
+		struct v4l2_ctrl *ctrl = ref->ctrl;
+		struct v4l2_ctrl *master = ctrl->cluster[0];
+		int i;
+
+		/* Skip if this control was already handled by a cluster. */
+		/* Skip button controls and read-only controls. */
+		if (ref->done || ctrl->type == V4L2_CTRL_TYPE_BUTTON ||
+		    (ctrl->flags & V4L2_CTRL_FLAG_READ_ONLY))
+			continue;
+
+		v4l2_ctrl_lock(master);
+		for (i = 0; i < master->ncontrols; i++) {
+			if (master->cluster[i]) {
+				struct v4l2_ctrl_ref *r =
+					find_ref(hdl, master->cluster[i]->id);
+
+				req_to_new(r);
+				master->cluster[i]->is_new = 1;
+				r->done = true;
+			}
+		}
+		/*
+		 * For volatile autoclusters that are currently in auto mode
+		 * we need to discover if it will be set to manual mode.
+		 * If so, then we have to copy the current volatile values
+		 * first since those will become the new manual values (which
+		 * may be overwritten by explicit new values from this set
+		 * of controls).
+		 */
+		if (master->is_auto && master->has_volatiles &&
+		    !is_cur_manual(master)) {
+			s32 new_auto_val = *master->p_new.p_s32;
+
+			/*
+			 * If the new value == the manual value, then copy
+			 * the current volatile values.
+			 */
+			if (new_auto_val == master->manual_mode_value)
+				update_from_auto_cluster(master);
+		}
+
+		try_or_set_cluster(NULL, master, true, 0);
+
+		v4l2_ctrl_unlock(master);
+	}
+
+	mutex_unlock(hdl->lock);
+	media_request_object_put(obj);
+}
+EXPORT_SYMBOL(v4l2_ctrl_request_setup);
+
 void v4l2_ctrl_notify(struct v4l2_ctrl *ctrl, v4l2_ctrl_notify_fnc notify, void *priv)
 {
 	if (ctrl == NULL)
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index 89a985607126..378d4f05c160 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -250,6 +250,9 @@ struct v4l2_ctrl {
  *		``prepare_ext_ctrls`` function at ``v4l2-ctrl.c``.
  * @from_other_dev: If true, then @ctrl was defined in another
  *		device then the &struct v4l2_ctrl_handler.
+ * @done:	If true, then this control reference is part of a
+ *		control cluster that was already set while applying
+ *		the controls in this media request object.
  * @p_req:	The request value. Only used if the control handler
  *		is bound to a media request.
  *
@@ -263,6 +266,7 @@ struct v4l2_ctrl_ref {
 	struct v4l2_ctrl *ctrl;
 	struct v4l2_ctrl_helper *helper;
 	bool from_other_dev;
+	bool done;
 	union v4l2_ctrl_ptr p_req;
 };
 
@@ -1059,6 +1063,11 @@ int v4l2_ctrl_subscribe_event(struct v4l2_fh *fh,
  */
 __poll_t v4l2_ctrl_poll(struct file *file, struct poll_table_struct *wait);
 
+void v4l2_ctrl_request_setup(struct media_request *req,
+			     struct v4l2_ctrl_handler *hdl);
+void v4l2_ctrl_request_complete(struct media_request *req,
+				struct v4l2_ctrl_handler *hdl);
+
 /* Helpers for ioctl_ops */
 
 /**
-- 
2.16.1

[RFCv9 PATCH 14/29] v4l2-ctrls: do not clone non-standard controls

[Hans Verkuil]

From: Alexandre Courbot <acourbot@chromium.org>

Only standard controls can be successfully cloned: handler_new_ref, used
by v4l2_ctrl_request_clone(), forcibly calls v4l2_ctrl_new_std() which
fails to find custom controls names, and we eventually hit the condition
that name == NULL in v4l2_ctrl_new().

This prevents us from using non-standard controls with requests, but
that is enough for testing purposes.

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

diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index 62f91c0f1e5f..6cf6b2154462 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -2876,6 +2876,11 @@ static int v4l2_ctrl_request_clone(struct v4l2_ctrl_handler *hdl,
 		if (filter && !filter(ctrl))
 			continue;
 		err = handler_new_ref(hdl, ctrl, &new_ref, false);
+		if (err) {
+			printk("%s: handler_new_ref on control %x (%s) returned %d\n", __func__, ctrl->id, ctrl->name, err);
+			err = 0;
+			continue;
+		}
 		if (err)
 			break;
 		if (from->req_obj.req)
-- 
2.16.1

[RFCv9 PATCH 15/29] videodev2.h: add request_fd field to v4l2_ext_controls

[Hans Verkuil]

From: Alexandre Courbot <acourbot@chromium.org>

If which is V4L2_CTRL_WHICH_REQUEST, then the request_fd field can be
used to specify a request for the G/S/TRY_EXT_CTRLS ioctls.

Signed-off-by: Alexandre Courbot <acourbot@chromium.org>
---
 drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 5 ++++-
 drivers/media/v4l2-core/v4l2-ioctl.c          | 6 +++---
 include/uapi/linux/videodev2.h                | 4 +++-
 3 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
index 5198c9eeb348..0782b3666deb 100644
--- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
+++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
@@ -732,7 +732,8 @@ struct v4l2_ext_controls32 {
 	__u32 which;
 	__u32 count;
 	__u32 error_idx;
-	__u32 reserved[2];
+	__s32 request_fd;
+	__u32 reserved[1];
 	compat_caddr_t controls; /* actually struct v4l2_ext_control32 * */
 };
 
@@ -807,6 +808,7 @@ 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) ||
+	    assign_in_user(&kp->request_fd, &up->request_fd) ||
 	    copy_in_user(kp->reserved, up->reserved, sizeof(kp->reserved)))
 		return -EFAULT;
 
@@ -865,6 +867,7 @@ 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) ||
+	    assign_in_user(&up->request_fd, &kp->request_fd) ||
 	    copy_in_user(up->reserved, kp->reserved, sizeof(up->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 a5dab16ff2d2..2c623da33155 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -553,8 +553,8 @@ static void v4l_print_ext_controls(const void *arg, bool write_only)
 	const struct v4l2_ext_controls *p = arg;
 	int i;
 
-	pr_cont("which=0x%x, count=%d, error_idx=%d",
-			p->which, p->count, p->error_idx);
+	pr_cont("which=0x%x, count=%d, error_idx=%d, request_fd=%d",
+			p->which, p->count, p->error_idx, p->request_fd);
 	for (i = 0; i < p->count; i++) {
 		if (!p->controls[i].size)
 			pr_cont(", id/val=0x%x/0x%x",
@@ -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] = 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 600877be5c22..6f41baa53787 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -1592,7 +1592,8 @@ struct v4l2_ext_controls {
 	};
 	__u32 count;
 	__u32 error_idx;
-	__u32 reserved[2];
+	__s32 request_fd;
+	__u32 reserved[1];
 	struct v4l2_ext_control *controls;
 };
 
@@ -1605,6 +1606,7 @@ struct v4l2_ext_controls {
 #define V4L2_CTRL_MAX_DIMS	  (4)
 #define V4L2_CTRL_WHICH_CUR_VAL   0
 #define V4L2_CTRL_WHICH_DEF_VAL   0x0f000000
+#define V4L2_CTRL_WHICH_REQUEST   0x0f010000
 
 enum v4l2_ctrl_type {
 	V4L2_CTRL_TYPE_INTEGER	     = 1,
-- 
2.16.1

[RFCv9 PATCH 16/29] v4l2-ctrls: integrate with requests

[Hans Verkuil]

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

Add the media_device pointer to the v4l2_g/s/try_ext_ctrls
functions and pass that in from the v4l2 core. The control
framework will look up the request from the request_fd file
descriptor and bind/unbind the request objects.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/platform/omap3isp/ispvideo.c |   2 +-
 drivers/media/v4l2-core/v4l2-ctrls.c       | 136 ++++++++++++++++++++++++++---
 drivers/media/v4l2-core/v4l2-ioctl.c       |  12 +--
 drivers/media/v4l2-core/v4l2-subdev.c      |   9 +-
 include/media/v4l2-ctrls.h                 |  13 ++-
 5 files changed, 148 insertions(+), 24 deletions(-)

diff --git a/drivers/media/platform/omap3isp/ispvideo.c b/drivers/media/platform/omap3isp/ispvideo.c
index a751c89a3ea8..bd564c2e767f 100644
--- a/drivers/media/platform/omap3isp/ispvideo.c
+++ b/drivers/media/platform/omap3isp/ispvideo.c
@@ -1028,7 +1028,7 @@ static int isp_video_check_external_subdevs(struct isp_video *video,
 	ctrls.count = 1;
 	ctrls.controls = &ctrl;
 
-	ret = v4l2_g_ext_ctrls(pipe->external->ctrl_handler, &ctrls);
+	ret = v4l2_g_ext_ctrls(pipe->external->ctrl_handler, NULL, &ctrls);
 	if (ret < 0) {
 		dev_warn(isp->dev, "no pixel rate control in subdev %s\n",
 			 pipe->external->name);
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index 6cf6b2154462..e31bce0207b4 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -1898,6 +1898,7 @@ int v4l2_ctrl_handler_init_class(struct v4l2_ctrl_handler *hdl,
 	lockdep_set_class_and_name(hdl->lock, key, name);
 	INIT_LIST_HEAD(&hdl->ctrls);
 	INIT_LIST_HEAD(&hdl->ctrl_refs);
+	INIT_LIST_HEAD(&hdl->requests);
 	hdl->nr_of_buckets = 1 + nr_of_controls_hint / 8;
 	hdl->buckets = kvmalloc_array(hdl->nr_of_buckets,
 				      sizeof(hdl->buckets[0]),
@@ -1918,6 +1919,14 @@ void v4l2_ctrl_handler_free(struct v4l2_ctrl_handler *hdl)
 	if (hdl == NULL || hdl->buckets == NULL)
 		return;
 
+	if (!hdl->req_obj.req && !list_empty(&hdl->requests)) {
+		struct v4l2_ctrl_handler *req, *next_req;
+
+		list_for_each_entry_safe(req, next_req, &hdl->requests, requests) {
+			media_request_object_unbind(&req->req_obj);
+			media_request_object_put(&req->req_obj);
+		}
+	}
 	mutex_lock(hdl->lock);
 	/* Free all nodes */
 	list_for_each_entry_safe(ref, next_ref, &hdl->ctrl_refs, node) {
@@ -2844,6 +2853,7 @@ static int v4l2_ctrl_request_clone(struct v4l2_ctrl_handler *hdl,
 			    bool (*filter)(const struct v4l2_ctrl *ctrl))
 {
 	struct v4l2_ctrl_ref *ref;
+	struct media_request *req = hdl->req_obj.req;
 	int err;
 
 	if (WARN_ON(!hdl || hdl == from))
@@ -2857,6 +2867,7 @@ static int v4l2_ctrl_request_clone(struct v4l2_ctrl_handler *hdl,
 	err = v4l2_ctrl_handler_init(hdl, (from->nr_of_buckets - 1) * 8);
 	if (err)
 		return err;
+	hdl->req_obj.req = req;
 	if (!from)
 		return 0;
 
@@ -2892,6 +2903,14 @@ static int v4l2_ctrl_request_clone(struct v4l2_ctrl_handler *hdl,
 	return err;
 }
 
+static void v4l2_ctrl_request_unbind(struct media_request_object *obj)
+{
+	struct v4l2_ctrl_handler *hdl =
+		container_of(obj, struct v4l2_ctrl_handler, req_obj);
+
+	list_del_init(&hdl->requests);
+}
+
 static void v4l2_ctrl_request_release(struct media_request_object *obj)
 {
 	struct v4l2_ctrl_handler *hdl =
@@ -2902,6 +2921,7 @@ static void v4l2_ctrl_request_release(struct media_request_object *obj)
 }
 
 static const struct media_request_object_ops req_ops = {
+	.unbind = v4l2_ctrl_request_unbind,
 	.release = v4l2_ctrl_request_release,
 };
 
@@ -2910,10 +2930,16 @@ static int v4l2_ctrl_request_bind(struct media_request *req,
 			   struct v4l2_ctrl_handler *from,
 			   bool (*filter)(const struct v4l2_ctrl *ctrl))
 {
-	int ret = v4l2_ctrl_request_clone(hdl, from, filter);
+	int ret;
 
-	if (!ret)
+	hdl->req_obj.req = req;
+	ret = v4l2_ctrl_request_clone(hdl, from, filter);
+	hdl->req_obj.req = NULL;
+
+	if (!ret) {
+		list_add_tail(&hdl->requests, &from->requests);
 		media_request_object_bind(req, &req_ops, from, &hdl->req_obj);
+	}
 	return ret;
 }
 
@@ -2977,6 +3003,7 @@ static int prepare_ext_ctrls(struct v4l2_ctrl_handler *hdl,
 
 		if (cs->which &&
 		    cs->which != V4L2_CTRL_WHICH_DEF_VAL &&
+		    cs->which != V4L2_CTRL_WHICH_REQUEST &&
 		    V4L2_CTRL_ID2WHICH(id) != cs->which)
 			return -EINVAL;
 
@@ -3056,15 +3083,15 @@ static int prepare_ext_ctrls(struct v4l2_ctrl_handler *hdl,
    whether there are any controls at all. */
 static int class_check(struct v4l2_ctrl_handler *hdl, u32 which)
 {
-	if (which == 0 || which == V4L2_CTRL_WHICH_DEF_VAL)
+	if (which == 0 || which == V4L2_CTRL_WHICH_DEF_VAL ||
+	    which == V4L2_CTRL_WHICH_REQUEST)
 		return 0;
 	return find_ref_lock(hdl, which | 1) ? 0 : -EINVAL;
 }
 
-
-
 /* Get extended controls. Allocates the helpers array if needed. */
-int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct v4l2_ext_controls *cs)
+int __v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl,
+		       struct v4l2_ext_controls *cs)
 {
 	struct v4l2_ctrl_helper helper[4];
 	struct v4l2_ctrl_helper *helpers = helper;
@@ -3145,6 +3172,64 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct v4l2_ext_controls *cs
 		kvfree(helpers);
 	return ret;
 }
+
+static struct media_request_object *
+v4l2_ctrls_find_req_obj(struct v4l2_ctrl_handler *hdl,
+			struct media_device *mdev, s32 fd)
+{
+	struct media_request *req = media_request_find(mdev, fd);
+	struct media_request_object *obj;
+	struct v4l2_ctrl_handler *new_hdl;
+	int ret;
+
+	if (IS_ERR(req))
+		return NULL;
+	obj = media_request_object_find(req, &req_ops, hdl);
+	if (obj) {
+		media_request_put(req);
+		return obj;
+	}
+	new_hdl = kzalloc(sizeof(*new_hdl), GFP_KERNEL);
+	if (!new_hdl)
+		goto error;
+	obj = &new_hdl->req_obj;
+	ret = v4l2_ctrl_handler_init(new_hdl, 0);
+	if (!ret)
+		ret = v4l2_ctrl_request_bind(req, new_hdl, hdl, NULL);
+	if (!ret) {
+		media_request_object_get(obj);
+		media_request_put(req);
+		return obj;
+	}
+	kfree(new_hdl);
+
+error:
+	media_request_put(req);
+	return NULL;
+}
+
+int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct media_device *mdev,
+		     struct v4l2_ext_controls *cs)
+{
+	struct media_request_object *obj = NULL;
+	int ret;
+
+	if (cs->which == V4L2_CTRL_WHICH_REQUEST) {
+		if (!mdev || cs->request_fd < 0)
+			return -EINVAL;
+		obj = v4l2_ctrls_find_req_obj(hdl, mdev, cs->request_fd);
+		if (!obj)
+			return -EINVAL;
+		hdl = container_of(obj, struct v4l2_ctrl_handler,
+				   req_obj);
+	}
+
+	ret = __v4l2_g_ext_ctrls(hdl, cs);
+
+	if (obj)
+		media_request_object_put(obj);
+	return ret;
+}
 EXPORT_SYMBOL(v4l2_g_ext_ctrls);
 
 /* Helper function to get a single control */
@@ -3320,9 +3405,9 @@ static void update_from_auto_cluster(struct v4l2_ctrl *master)
 }
 
 /* Try or try-and-set controls */
-static int try_set_ext_ctrls(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
-			     struct v4l2_ext_controls *cs,
-			     bool set)
+static int __try_set_ext_ctrls(struct v4l2_fh *fh,
+			       struct v4l2_ctrl_handler *hdl,
+			       struct v4l2_ext_controls *cs, bool set)
 {
 	struct v4l2_ctrl_helper helper[4];
 	struct v4l2_ctrl_helper *helpers = helper;
@@ -3435,16 +3520,41 @@ static int try_set_ext_ctrls(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
 	return ret;
 }
 
-int v4l2_try_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct v4l2_ext_controls *cs)
+static int try_set_ext_ctrls(struct v4l2_fh *fh,
+			     struct v4l2_ctrl_handler *hdl, struct media_device *mdev,
+			     struct v4l2_ext_controls *cs, bool set)
+{
+	struct media_request_object *obj = NULL;
+	int ret;
+
+	if (cs->which == V4L2_CTRL_WHICH_REQUEST) {
+		if (!mdev || cs->request_fd < 0)
+			return -EINVAL;
+		obj = v4l2_ctrls_find_req_obj(hdl, mdev, cs->request_fd);
+		if (!obj)
+			return -EINVAL;
+		hdl = container_of(obj, struct v4l2_ctrl_handler,
+				   req_obj);
+	}
+
+	ret = __try_set_ext_ctrls(fh, hdl, cs, set);
+
+	if (obj)
+		media_request_object_put(obj);
+	return ret;
+}
+
+int v4l2_try_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct media_device *mdev,
+		       struct v4l2_ext_controls *cs)
 {
-	return try_set_ext_ctrls(NULL, hdl, cs, false);
+	return try_set_ext_ctrls(NULL, hdl, mdev, cs, false);
 }
 EXPORT_SYMBOL(v4l2_try_ext_ctrls);
 
 int v4l2_s_ext_ctrls(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
-					struct v4l2_ext_controls *cs)
+		     struct media_device *mdev, struct v4l2_ext_controls *cs)
 {
-	return try_set_ext_ctrls(fh, hdl, cs, true);
+	return try_set_ext_ctrls(fh, hdl, mdev, cs, true);
 }
 EXPORT_SYMBOL(v4l2_s_ext_ctrls);
 
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index 2c623da33155..ceeb6df0ef19 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -2079,9 +2079,9 @@ static int v4l_g_ext_ctrls(const struct v4l2_ioctl_ops *ops,
 
 	p->error_idx = p->count;
 	if (vfh && vfh->ctrl_handler)
-		return v4l2_g_ext_ctrls(vfh->ctrl_handler, p);
+		return v4l2_g_ext_ctrls(vfh->ctrl_handler, vfd->v4l2_dev->mdev, p);
 	if (vfd->ctrl_handler)
-		return v4l2_g_ext_ctrls(vfd->ctrl_handler, p);
+		return v4l2_g_ext_ctrls(vfd->ctrl_handler, vfd->v4l2_dev->mdev, p);
 	if (ops->vidioc_g_ext_ctrls == NULL)
 		return -ENOTTY;
 	return check_ext_ctrls(p, 0) ? ops->vidioc_g_ext_ctrls(file, fh, p) :
@@ -2098,9 +2098,9 @@ static int v4l_s_ext_ctrls(const struct v4l2_ioctl_ops *ops,
 
 	p->error_idx = p->count;
 	if (vfh && vfh->ctrl_handler)
-		return v4l2_s_ext_ctrls(vfh, vfh->ctrl_handler, p);
+		return v4l2_s_ext_ctrls(vfh, vfh->ctrl_handler, vfd->v4l2_dev->mdev, p);
 	if (vfd->ctrl_handler)
-		return v4l2_s_ext_ctrls(NULL, vfd->ctrl_handler, p);
+		return v4l2_s_ext_ctrls(NULL, vfd->ctrl_handler, vfd->v4l2_dev->mdev, p);
 	if (ops->vidioc_s_ext_ctrls == NULL)
 		return -ENOTTY;
 	return check_ext_ctrls(p, 0) ? ops->vidioc_s_ext_ctrls(file, fh, p) :
@@ -2117,9 +2117,9 @@ static int v4l_try_ext_ctrls(const struct v4l2_ioctl_ops *ops,
 
 	p->error_idx = p->count;
 	if (vfh && vfh->ctrl_handler)
-		return v4l2_try_ext_ctrls(vfh->ctrl_handler, p);
+		return v4l2_try_ext_ctrls(vfh->ctrl_handler, vfd->v4l2_dev->mdev, p);
 	if (vfd->ctrl_handler)
-		return v4l2_try_ext_ctrls(vfd->ctrl_handler, p);
+		return v4l2_try_ext_ctrls(vfd->ctrl_handler, vfd->v4l2_dev->mdev, p);
 	if (ops->vidioc_try_ext_ctrls == NULL)
 		return -ENOTTY;
 	return check_ext_ctrls(p, 0) ? ops->vidioc_try_ext_ctrls(file, fh, p) :
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index f9eed938d348..ce8c133e0ccd 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -222,17 +222,20 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 	case VIDIOC_G_EXT_CTRLS:
 		if (!vfh->ctrl_handler)
 			return -ENOTTY;
-		return v4l2_g_ext_ctrls(vfh->ctrl_handler, arg);
+		return v4l2_g_ext_ctrls(vfh->ctrl_handler,
+					sd->v4l2_dev->mdev, arg);
 
 	case VIDIOC_S_EXT_CTRLS:
 		if (!vfh->ctrl_handler)
 			return -ENOTTY;
-		return v4l2_s_ext_ctrls(vfh, vfh->ctrl_handler, arg);
+		return v4l2_s_ext_ctrls(vfh, vfh->ctrl_handler,
+					sd->v4l2_dev->mdev, arg);
 
 	case VIDIOC_TRY_EXT_CTRLS:
 		if (!vfh->ctrl_handler)
 			return -ENOTTY;
-		return v4l2_try_ext_ctrls(vfh->ctrl_handler, arg);
+		return v4l2_try_ext_ctrls(vfh->ctrl_handler,
+					  sd->v4l2_dev->mdev, arg);
 
 	case VIDIOC_DQEVENT:
 		if (!(sd->flags & V4L2_SUBDEV_FL_HAS_EVENTS))
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index 378d4f05c160..cf7b36564cd0 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -291,6 +291,11 @@ struct v4l2_ctrl_ref {
  * @notify_priv: Passed as argument to the v4l2_ctrl notify callback.
  * @nr_of_buckets: Total number of buckets in the array.
  * @error:	The error code of the first failed control addition.
+ * @requests:	List to keep track of open control handler request objects.
+ *		For the parent control handler (@req_obj.req == NULL) this
+ *		is the list header. When the parent control handler is
+ *		removed, it has to unbind and put all these requests since
+ *		they refer to the parent.
  * @req_obj:	The &struct media_request_object, used to link into a
  *		&struct media_request.
  */
@@ -305,6 +310,7 @@ struct v4l2_ctrl_handler {
 	void *notify_priv;
 	u16 nr_of_buckets;
 	int error;
+	struct list_head requests;
 	struct media_request_object req_obj;
 };
 
@@ -1134,11 +1140,12 @@ int v4l2_s_ctrl(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
  *	:ref:`VIDIOC_G_EXT_CTRLS <vidioc_g_ext_ctrls>` ioctl
  *
  * @hdl: pointer to &struct v4l2_ctrl_handler
+ * @mdev: pointer to &struct media_device
  * @c: pointer to &struct v4l2_ext_controls
  *
  * If hdl == NULL then they will all return -EINVAL.
  */
-int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl,
+int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct media_device *mdev,
 		     struct v4l2_ext_controls *c);
 
 /**
@@ -1146,11 +1153,13 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl,
  *	:ref:`VIDIOC_TRY_EXT_CTRLS <vidioc_g_ext_ctrls>` ioctl
  *
  * @hdl: pointer to &struct v4l2_ctrl_handler
+ * @mdev: pointer to &struct media_device
  * @c: pointer to &struct v4l2_ext_controls
  *
  * If hdl == NULL then they will all return -EINVAL.
  */
 int v4l2_try_ext_ctrls(struct v4l2_ctrl_handler *hdl,
+		       struct media_device *mdev,
 		       struct v4l2_ext_controls *c);
 
 /**
@@ -1159,11 +1168,13 @@ int v4l2_try_ext_ctrls(struct v4l2_ctrl_handler *hdl,
  *
  * @fh: pointer to &struct v4l2_fh
  * @hdl: pointer to &struct v4l2_ctrl_handler
+ * @mdev: pointer to &struct media_device
  * @c: pointer to &struct v4l2_ext_controls
  *
  * If hdl == NULL then they will all return -EINVAL.
  */
 int v4l2_s_ext_ctrls(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
+		     struct media_device *mdev,
 		     struct v4l2_ext_controls *c);
 
 /**
-- 
2.16.1

Re: [RFCv9 PATCH 00/29] Request API

[Hans Verkuil]

On 03/28/2018 03:50 PM, Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Hi all,
> 
> This patch series is an attempt to pick the best parts of
> Alexandre's RFCv4:

"sending too many recipients in a short time. Please try again later."

So patches 17 and up will appear a bit later. I'll drop the CC list to
avoid hitting this issue again. My ISP is not friendly for kernel
developers who need to post large series to many people :-(

Regards,

	Hans

Re: [RFCv9 PATCH 01/29] v4l2-device.h: always expose mdev

[Sakari Ailus]

On Wed, Mar 28, 2018 at 03:50:02PM +0200, Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> The mdev field is only present if CONFIG_MEDIA_CONTROLLER is set.
> But since we will need to pass the media_device to vb2 snd the
> control framework it is very convenient to just make this field
> available all the time. If CONFIG_MEDIA_CONTROLLER is not set,
> then it will just be NULL.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  include/media/v4l2-device.h | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/include/media/v4l2-device.h b/include/media/v4l2-device.h
> index 0c9e4da55499..b330e4a08a6b 100644
> --- a/include/media/v4l2-device.h
> +++ b/include/media/v4l2-device.h
> @@ -33,7 +33,7 @@ struct v4l2_ctrl_handler;
>   * struct v4l2_device - main struct to for V4L2 device drivers
>   *
>   * @dev: pointer to struct device.
> - * @mdev: pointer to struct media_device
> + * @mdev: pointer to struct media_device, may be NULL.
>   * @subdevs: used to keep track of the registered subdevs
>   * @lock: lock this struct; can be used by the driver as well
>   *	if this struct is embedded into a larger struct.
> @@ -58,9 +58,7 @@ struct v4l2_ctrl_handler;
>   */
>  struct v4l2_device {
>  	struct device *dev;
> -#if defined(CONFIG_MEDIA_CONTROLLER)
>  	struct media_device *mdev;
> -#endif
>  	struct list_head subdevs;
>  	spinlock_t lock;
>  	char name[V4L2_DEVICE_NAME_SIZE];

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

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

Re: [RFCv9 PATCH 02/29] uapi/linux/media.h: add request API

[Sakari Ailus]

On Wed, Mar 28, 2018 at 03:50:03PM +0200, Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Define the public request API.
> 
> This adds the new MEDIA_IOC_REQUEST_ALLOC ioctl to allocate a request
> and two ioctls that operate on a request in order to queue the
> contents of the request to the driver and to re-initialize the
> request.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  include/uapi/linux/media.h | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> index c7e9a5cba24e..f8769e74f847 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_alloc {
> +	__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_alloc)
> +
> +#define MEDIA_REQUEST_IOC_QUEUE		_IO('|',  0x80)
> +#define MEDIA_REQUEST_IOC_REINIT	_IO('|',  0x81)
>  
>  #if !defined(__KERNEL__) || defined(__NEED_MEDIA_LEGACY_API)
>  

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

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

Re: [RFCv9 PATCH 03/29] media-request: allocate media requests

[Sakari Ailus]

Hi Hans,

On Wed, Mar 28, 2018 at 03:50:04PM +0200, Hans Verkuil wrote:
...
> @@ -88,6 +96,8 @@ 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

s/validating and//

As there's no more a separate validation step. Then,

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

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

Re: [RFCv9 PATCH 03/29] media-request: allocate media requests

[Hans Verkuil]

On 29/03/18 10:45, Sakari Ailus wrote:
> Hi Hans,
> 
> On Wed, Mar 28, 2018 at 03:50:04PM +0200, Hans Verkuil wrote:
> ...
>> @@ -88,6 +96,8 @@ 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
> 
> s/validating and//
> 
> As there's no more a separate validation step. Then,

Well, you validate before queuing. It's not a separate step, but
part of the queue operation. See patch 23 where this is implemented
in the vb2_request_helper function.

Regards,

	Hans

> 
> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> 

Re: [RFCv9 PATCH 03/29] media-request: allocate media requests

[Sakari Ailus]

On Thu, Mar 29, 2018 at 10:57:44AM +0200, Hans Verkuil wrote:
> On 29/03/18 10:45, Sakari Ailus wrote:
> > Hi Hans,
> > 
> > On Wed, Mar 28, 2018 at 03:50:04PM +0200, Hans Verkuil wrote:
> > ...
> >> @@ -88,6 +96,8 @@ 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
> > 
> > s/validating and//
> > 
> > As there's no more a separate validation step. Then,
> 
> Well, you validate before queuing. It's not a separate step, but
> part of the queue operation. See patch 23 where this is implemented
> in the vb2_request_helper function.

Works for me. I think we'll need the validate op sooner or later anyway.

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

Re: [RFCv9 PATCH 03/29] media-request: allocate media requests

[Hans Verkuil]

On 29/03/18 11:01, Sakari Ailus wrote:
> On Thu, Mar 29, 2018 at 10:57:44AM +0200, Hans Verkuil wrote:
>> On 29/03/18 10:45, Sakari Ailus wrote:
>>> Hi Hans,
>>>
>>> On Wed, Mar 28, 2018 at 03:50:04PM +0200, Hans Verkuil wrote:
>>> ...
>>>> @@ -88,6 +96,8 @@ 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
>>>
>>> s/validating and//
>>>
>>> As there's no more a separate validation step. Then,
>>
>> Well, you validate before queuing. It's not a separate step, but
>> part of the queue operation. See patch 23 where this is implemented
>> in the vb2_request_helper function.
> 
> Works for me. I think we'll need the validate op sooner or later anyway.
> 


There is one. Request objects have prepare, unprepare and queue ops.

The request req_queue op will prepare all objects, and only queue if the
prepare (aka validate) step succeeds for all objects. If not, then the
objects that were prepared will be rolled back with the unprepare op
and req_queue returns an error.

Note that the object ops are not documented at the moment, it's one of
my TODOs.

Regards,

	Hans

Re: [RFCv9 PATCH 03/29] media-request: allocate media requests

[Sakari Ailus]

Hi Hans,

On Thu, Mar 29, 2018 at 11:16:45AM +0200, Hans Verkuil wrote:
> On 29/03/18 11:01, Sakari Ailus wrote:
> > On Thu, Mar 29, 2018 at 10:57:44AM +0200, Hans Verkuil wrote:
> >> On 29/03/18 10:45, Sakari Ailus wrote:
> >>> Hi Hans,
> >>>
> >>> On Wed, Mar 28, 2018 at 03:50:04PM +0200, Hans Verkuil wrote:
> >>> ...
> >>>> @@ -88,6 +96,8 @@ 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
> >>>
> >>> s/validating and//
> >>>
> >>> As there's no more a separate validation step. Then,
> >>
> >> Well, you validate before queuing. It's not a separate step, but
> >> part of the queue operation. See patch 23 where this is implemented
> >> in the vb2_request_helper function.
> > 
> > Works for me. I think we'll need the validate op sooner or later anyway.
> > 
> 
> 
> There is one. Request objects have prepare, unprepare and queue ops.
> 
> The request req_queue op will prepare all objects, and only queue if the
> prepare (aka validate) step succeeds for all objects. If not, then the

You can't validate the objects in a request separately from other objects
in it, the validation needs to happen at the request level. There are two
reasons for this:

- The objects in a request must be compatible with all other objects in the
  request and

- The request must contain the required objects in order to be valid ---
  e.g. for a device producing multiple capture buffers from one output
  buffer, the output buffer is mandatory whereas one or more of the capture
  buffers are not.

The latter could presumably be implemented separately for each object but
then the driver needs to go fishing for the related objects which may not
be very efficient.

What I'm proposing is to put this at the level of a request, at least for
now.

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

Re: [RFCv9 PATCH 05/29] media-request: add request ioctls

[Sakari Ailus]

Hi Hans,

I think it'd be easier to review the code if it was a part of the previous
patch. It's so closely related to what's being done there.

On Wed, Mar 28, 2018 at 03:50:06PM +0200, Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Implement the MEDIA_REQUEST_IOC_QUEUE and MEDIA_REQUEST_IOC_REINIT
> ioctls.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/media/media-request.c | 80 +++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 78 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/media-request.c b/drivers/media/media-request.c
> index 8135d9d32af9..3ee3b27fd644 100644
> --- a/drivers/media/media-request.c
> +++ b/drivers/media/media-request.c
> @@ -105,10 +105,86 @@ static unsigned int media_request_poll(struct file *filp,
>  	return 0;
>  }
>  
> +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);
> +
> +	spin_lock_irqsave(&req->lock, flags);
> +	if (req->state != MEDIA_REQUEST_STATE_IDLE) {
> +		dev_dbg(mdev->dev,
> +			"request: unable to queue %s, request in state %s\n",
> +			req->debug_str, media_request_state_str(req->state));

You could print the message after releasing the spinlock. Maybe store the
request state locally first?

> +		spin_unlock_irqrestore(&req->lock, flags);
> +		return -EINVAL;
> +	}
> +	req->state = MEDIA_REQUEST_STATE_QUEUEING;
> +
> +	spin_unlock_irqrestore(&req->lock, flags);
> +
> +	/*
> +	 * 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);
> +
> +	ret = mdev->ops->req_queue(req);
> +	spin_lock_irqsave(&req->lock, flags);
> +	req->state = ret ? MEDIA_REQUEST_STATE_IDLE : MEDIA_REQUEST_STATE_QUEUED;
> +	spin_unlock_irqrestore(&req->lock, flags);
> +	mutex_unlock(&mdev->req_queue_mutex);
> +
> +	if (ret) {
> +		dev_dbg(mdev->dev, "request: can't queue %s (%d)\n",
> +			req->debug_str, ret);
> +	} else {
> +		media_request_get(req);
> +	}
> +
> +	return ret;
> +}
> +
> +static long media_request_ioctl_reinit(struct media_request *req)
> +{
> +	struct media_device *mdev = req->mdev;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&req->lock, flags);
> +	if (req->state != MEDIA_REQUEST_STATE_IDLE &&
> +	    req->state != MEDIA_REQUEST_STATE_COMPLETE) {
> +		dev_dbg(mdev->dev,
> +			"request: %s not in idle or complete state, cannot reinit\n",
> +			req->debug_str);
> +		spin_unlock_irqrestore(&req->lock, flags);
> +		return -EINVAL;
> +	}
> +	req->state = MEDIA_REQUEST_STATE_CLEANING;
> +	spin_unlock_irqrestore(&req->lock, flags);
> +
> +	media_request_clean(req);
> +
> +	spin_lock_irqsave(&req->lock, flags);
> +	req->state = MEDIA_REQUEST_STATE_IDLE;
> +	spin_unlock_irqrestore(&req->lock, flags);

Newline?

> +	return 0;
> +}
> +
>  static long media_request_ioctl(struct file *filp, unsigned int cmd,
> -				unsigned long __arg)
> +				unsigned long arg)

This belongs to the patch adding media_request_ioctl().

>  {
> -	return -ENOIOCTLCMD;
> +	struct media_request *req = filp->private_data;
> +
> +	switch (cmd) {
> +	case MEDIA_REQUEST_IOC_QUEUE:
> +		return media_request_ioctl_queue(req);
> +	case MEDIA_REQUEST_IOC_REINIT:
> +		return media_request_ioctl_reinit(req);
> +	default:
> +		return -ENOIOCTLCMD;
> +	}
>  }
>  
>  static const struct file_operations request_fops = {

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

Re: [RFCv9 PATCH 03/29] media-request: allocate media requests

[Hans Verkuil]

On 29/03/18 11:52, Sakari Ailus wrote:
> Hi Hans,
> 
> On Thu, Mar 29, 2018 at 11:16:45AM +0200, Hans Verkuil wrote:
>> On 29/03/18 11:01, Sakari Ailus wrote:
>>> On Thu, Mar 29, 2018 at 10:57:44AM +0200, Hans Verkuil wrote:
>>>> On 29/03/18 10:45, Sakari Ailus wrote:
>>>>> Hi Hans,
>>>>>
>>>>> On Wed, Mar 28, 2018 at 03:50:04PM +0200, Hans Verkuil wrote:
>>>>> ...
>>>>>> @@ -88,6 +96,8 @@ 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
>>>>>
>>>>> s/validating and//
>>>>>
>>>>> As there's no more a separate validation step. Then,
>>>>
>>>> Well, you validate before queuing. It's not a separate step, but
>>>> part of the queue operation. See patch 23 where this is implemented
>>>> in the vb2_request_helper function.
>>>
>>> Works for me. I think we'll need the validate op sooner or later anyway.
>>>
>>
>>
>> There is one. Request objects have prepare, unprepare and queue ops.
>>
>> The request req_queue op will prepare all objects, and only queue if the
>> prepare (aka validate) step succeeds for all objects. If not, then the
> 
> You can't validate the objects in a request separately from other objects
> in it, the validation needs to happen at the request level. There are two
> reasons for this:
> 
> - The objects in a request must be compatible with all other objects in the
>   request and
> 
> - The request must contain the required objects in order to be valid ---
>   e.g. for a device producing multiple capture buffers from one output
>   buffer, the output buffer is mandatory whereas one or more of the capture
>   buffers are not.
> 
> The latter could presumably be implemented separately for each object but
> then the driver needs to go fishing for the related objects which may not
> be very efficient.
> 
> What I'm proposing is to put this at the level of a request, at least for
> now.
> 

The driver implements req_queue. It can do whatever it wants there, it has
full control.

However, as part of that process objects still have to be prepared (for
buffers that literally means calling buf_prepare). That step does the
validation on an object level and also possible memory/resource allocations
that can fail.

The helper function in patch 23 is however sufficient for simple drivers
like vim2m, vivid, codec drivers, etc. that do not need to validate the
request as a whole.

Drivers that do need to validate the request as a whole will not use that
helper function, they will do this themselves.

Regards,

	Hans

Re: [RFCv9 PATCH 04/29] media-request: core request support

[Sakari Ailus]

Hi Hans,

Thanks for your work on this. A few comments below...

On Wed, Mar 28, 2018 at 03:50:05PM +0200, Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Implement the core of the media request processing.
> 
> Drivers can bind request objects to a request. These objects
> can then be marked completed if the driver finished using them,
> or just be unbound if the results do not need to be kept (e.g.
> in the case of buffers).
> 
> Once all objects that were added are either unbound or completed,
> the request is marked 'complete' and a POLLPRI signal is sent
> via poll.
> 
> Both requests and request objects are refcounted.
> 
> While a request is queued its refcount is incremented (since it
> is in use by a driver). Once it is completed the refcount is
> decremented. When the user closes the request file descriptor
> the refcount is also decremented. Once it reaches 0 all request
> objects in the request are unbound and put() and the request
> itself is freed.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/media/media-request.c | 269 +++++++++++++++++++++++++++++++++++++++++-
>  include/media/media-request.h | 148 +++++++++++++++++++++++
>  2 files changed, 416 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/media-request.c b/drivers/media/media-request.c
> index ead78613fdbe..8135d9d32af9 100644
> --- a/drivers/media/media-request.c
> +++ b/drivers/media/media-request.c
> @@ -16,8 +16,275 @@
>  #include <media/media-device.h>
>  #include <media/media-request.h>
>  
> +static const char * const request_state[] = {
> +	"idle",
> +	"queueing",
> +	"queued",
> +	"complete",
> +	"cleaning",
> +};
> +
> +static const char *
> +media_request_state_str(enum media_request_state state)
> +{
> +	if (WARN_ON(state >= ARRAY_SIZE(request_state)))
> +		return "unknown";
> +	return request_state[state];
> +}
> +
> +static void media_request_clean(struct media_request *req)
> +{
> +	struct media_request_object *obj, *obj_safe;
> +
> +	WARN_ON(req->state != MEDIA_REQUEST_STATE_CLEANING);
> +
> +	list_for_each_entry_safe(obj, obj_safe, &req->objects, list) {
> +		media_request_object_unbind(obj);
> +		media_request_object_put(obj);
> +	}
> +
> +	req->num_incomplete_objects = 0;
> +	wake_up_interruptible(&req->poll_wait);
> +}
> +
> +static void media_request_release(struct kref *kref)
> +{
> +	struct media_request *req =
> +		container_of(kref, struct media_request, kref);
> +	struct media_device *mdev = req->mdev;
> +	unsigned long flags;
> +
> +	dev_dbg(mdev->dev, "request: release %s\n", req->debug_str);
> +
> +	spin_lock_irqsave(&req->lock, flags);
> +	req->state = MEDIA_REQUEST_STATE_CLEANING;
> +	spin_unlock_irqrestore(&req->lock, flags);
> +
> +	media_request_clean(req);
> +
> +	if (mdev->ops->req_free)
> +		mdev->ops->req_free(req);
> +	else
> +		kfree(req);
> +}
> +
> +void media_request_put(struct media_request *req)
> +{
> +	kref_put(&req->kref, media_request_release);
> +}
> +EXPORT_SYMBOL_GPL(media_request_put);
> +
> +static int media_request_close(struct inode *inode, struct file *filp)
> +{
> +	struct media_request *req = filp->private_data;
> +
> +	media_request_put(req);

One more newline?

> +	return 0;
> +}
> +
> +static unsigned int media_request_poll(struct file *filp,
> +				       struct poll_table_struct *wait)
> +{
> +	struct media_request *req = filp->private_data;
> +	unsigned long flags;
> +	enum media_request_state state;
> +
> +	if (!(poll_requested_events(wait) & POLLPRI))
> +		return 0;
> +
> +	spin_lock_irqsave(&req->lock, flags);
> +	state = req->state;
> +	spin_unlock_irqrestore(&req->lock, flags);
> +
> +	if (state == MEDIA_REQUEST_STATE_COMPLETE)
> +		return POLLPRI;
> +	if (state == MEDIA_REQUEST_STATE_IDLE)

Should this be

	if (state != MEDIA_REQUEST_STATE_QUEUED)

? (Also see my other comment on the QUEUEING state below.)

> +		return POLLERR;
> +
> +	poll_wait(filp, &req->poll_wait, wait);

Newline?

> +	return 0;
> +}
> +
> +static long media_request_ioctl(struct file *filp, unsigned int cmd,
> +				unsigned long __arg)
> +{
> +	return -ENOIOCTLCMD;
> +}
> +
> +static const struct file_operations request_fops = {
> +	.owner = THIS_MODULE,
> +	.poll = media_request_poll,
> +	.unlocked_ioctl = media_request_ioctl,
> +	.release = media_request_close,
> +};
> +
>  int media_request_alloc(struct media_device *mdev,
>  			struct media_request_alloc *alloc)
>  {
> -	return -ENOMEM;
> +	struct media_request *req;
> +	struct file *filp;
> +	char comm[TASK_COMM_LEN];
> +	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;
> +	}
> +
> +	if (mdev->ops->req_alloc)
> +		req = mdev->ops->req_alloc(mdev);
> +	else
> +		req = kzalloc(sizeof(*req), GFP_KERNEL);
> +	if (!req) {
> +		ret = -ENOMEM;
> +		goto err_fput;
> +	}
> +
> +	filp->private_data = req;
> +	req->mdev = mdev;
> +	req->state = MEDIA_REQUEST_STATE_IDLE;
> +	req->num_incomplete_objects = 0;
> +	kref_init(&req->kref);
> +	INIT_LIST_HEAD(&req->objects);
> +	spin_lock_init(&req->lock);
> +	init_waitqueue_head(&req->poll_wait);
> +
> +	alloc->fd = fd;
> +
> +	get_task_comm(comm, current);
> +	snprintf(req->debug_str, sizeof(req->debug_str), "%s:%d",
> +		 comm, fd);
> +	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;
> +}
> +
> +static void media_request_object_release(struct kref *kref)
> +{
> +	struct media_request_object *obj =
> +		container_of(kref, struct media_request_object, kref);
> +	struct media_request *req = obj->req;
> +
> +	if (req)
> +		media_request_object_unbind(obj);

Newline?

> +	obj->ops->release(obj);
> +}
> +
> +void media_request_object_put(struct media_request_object *obj)
> +{
> +	kref_put(&obj->kref, media_request_object_release);
> +}
> +EXPORT_SYMBOL_GPL(media_request_object_put);
> +
> +void media_request_object_init(struct media_request_object *obj)
> +{
> +	obj->ops = NULL;
> +	obj->req = NULL;
> +	obj->priv = NULL;
> +	obj->completed = false;

The function is intended to be used on a freshly allocated object only. The
allocation function should already zero the memory, so there should be no
need to do this here anymore.

> +	INIT_LIST_HEAD(&obj->list);

There's no need to init this as it's a member of a list, not a head.

> +	kref_init(&obj->kref);
> +}
> +EXPORT_SYMBOL_GPL(media_request_object_init);
> +
> +void media_request_object_bind(struct media_request *req,
> +			       const struct media_request_object_ops *ops,
> +			       void *priv,
> +			       struct media_request_object *obj)
> +{
> +	unsigned long flags;
> +
> +	WARN_ON(!ops->release);
> +	obj->req = req;
> +	obj->ops = ops;
> +	obj->priv = priv;
> +	spin_lock_irqsave(&req->lock, flags);
> +	if (WARN_ON(req->state != MEDIA_REQUEST_STATE_IDLE))

How do you find out that a request is not in an idle state? I'd say you
can't be sure otherwise as a request may be queued whilst another process
is binding an object to it. I think the check here should be used prevent
that.

I'd also assign fields other than req in object init as objects may be
unbound from requests and outlive requests (video buffers, for instance).
No ops nor priv field may be changed during a lifetime of an object.

> +		goto unlock;
> +	list_add_tail(&obj->list, &req->objects);
> +	req->num_incomplete_objects++;
> +unlock:
> +	spin_unlock_irqrestore(&req->lock, flags);
> +}
> +EXPORT_SYMBOL_GPL(media_request_object_bind);
> +
> +void media_request_object_unbind(struct media_request_object *obj)
> +{
> +	struct media_request *req = obj->req;
> +	unsigned long flags;
> +	bool completed = false;
> +
> +	if (!req)
> +		return;
> +
> +	spin_lock_irqsave(&req->lock, flags);
> +	list_del(&obj->list);
> +	obj->req = NULL;
> +
> +	if (req->state == MEDIA_REQUEST_STATE_COMPLETE ||
> +	    req->state == MEDIA_REQUEST_STATE_CLEANING)
> +		goto unlock;
> +
> +	if (WARN_ON(req->state == MEDIA_REQUEST_STATE_QUEUEING))
> +		goto unlock;
> +
> +	if (WARN_ON(!req->num_incomplete_objects))
> +		goto unlock;
> +
> +	req->num_incomplete_objects--;
> +	if (req->state == MEDIA_REQUEST_STATE_QUEUED &&
> +	    !req->num_incomplete_objects) {
> +		req->state = MEDIA_REQUEST_STATE_COMPLETE;
> +		completed = true;
> +		wake_up_interruptible(&req->poll_wait);
> +	}
> +unlock:
> +	spin_unlock_irqrestore(&req->lock, flags);
> +	if (obj->ops->unbind)
> +		obj->ops->unbind(obj);
> +	if (completed)
> +		media_request_put(req);
> +}
> +EXPORT_SYMBOL_GPL(media_request_object_unbind);
> +
> +void media_request_object_complete(struct media_request_object *obj)
> +{
> +	struct media_request *req = obj->req;
> +	unsigned long flags;
> +	bool completed = false;
> +
> +	spin_lock_irqsave(&req->lock, flags);
> +	if (obj->completed)
> +		goto unlock;
> +	obj->completed = true;
> +	if (WARN_ON(!req->num_incomplete_objects) ||
> +	    WARN_ON(req->state != MEDIA_REQUEST_STATE_QUEUED))

If an object completes very soon after a request is queued, it is possible
that the request is still in QUEUEING state. I'm actually beginning to
wonder whether this state is useful; would it bring trouble if we just put
it to QUEUED right from the beginning?

> +		goto unlock;
> +
> +	if (!--req->num_incomplete_objects) {
> +		req->state = MEDIA_REQUEST_STATE_COMPLETE;
> +		wake_up_interruptible(&req->poll_wait);
> +		completed = true;
> +	}

Newline here...

> +unlock:
> +	spin_unlock_irqrestore(&req->lock, flags);

and here?

> +	if (completed)
> +		media_request_put(req);
>  }
> +EXPORT_SYMBOL_GPL(media_request_object_complete);
> diff --git a/include/media/media-request.h b/include/media/media-request.h
> index dae3eccd9aa7..baed99eb1279 100644
> --- a/include/media/media-request.h
> +++ b/include/media/media-request.h
> @@ -16,7 +16,155 @@
>  
>  #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_CLEANING,
> +};
> +
> +struct media_request_object;
> +
> +/**
> + * struct media_request - Media device request

And here (plus " *").

> + * @mdev: Media device this request belongs to
> + * @kref: Reference count
> + * @debug_prefix: Prefix for debug messages (process name:fd)
> + * @state: The state of the request
> + * @objects: List of @struct media_request_object request objects
> + * @num_objects: The number objects in the request
> + * @num_completed_objects: The number of completed objects in the request
> + * @poll_wait: Wait queue for poll
> + * @lock: Serializes access to this struct
> + */
> +struct media_request {
> +	struct media_device *mdev;
> +	struct kref kref;
> +	char debug_str[TASK_COMM_LEN + 11];
> +	enum media_request_state state;
> +	struct list_head objects;
> +	unsigned int num_incomplete_objects;
> +	struct wait_queue_head poll_wait;
> +	spinlock_t lock;
> +};
> +
> +#ifdef CONFIG_MEDIA_CONTROLLER
> +static inline void media_request_get(struct media_request *req)
> +{
> +	kref_get(&req->kref);
> +}
> +
> +void media_request_put(struct media_request *req);
> +
>  int media_request_alloc(struct media_device *mdev,
>  			struct media_request_alloc *alloc);
> +#else
> +static inline void media_request_get(struct media_request *req)
> +{
> +}
> +
> +static inline void media_request_put(struct media_request *req)
> +{
> +}
> +#endif
> +
> +struct media_request_object_ops {
> +	int (*prepare)(struct media_request_object *object);
> +	void (*unprepare)(struct media_request_object *object);
> +	void (*queue)(struct media_request_object *object);
> +	void (*unbind)(struct media_request_object *object);
> +	void (*release)(struct media_request_object *object);
> +};
> +
> +/**
> + * struct media_request_object - An opaque object that belongs to a media
> + *				 request
> + *
> + * @priv: object's priv pointer
> + * @list: List entry of the object for @struct media_request
> + * @kref: Reference count of the object, acquire before releasing req->lock
> + *
> + * An object related to the request. This struct is embedded in the
> + * larger object data.
> + */
> +struct media_request_object {
> +	const struct media_request_object_ops *ops;
> +	void *priv;
> +	struct media_request *req;
> +	struct list_head list;
> +	struct kref kref;
> +	bool completed;
> +};
> +
> +#ifdef CONFIG_MEDIA_CONTROLLER
> +static inline void media_request_object_get(struct media_request_object *obj)
> +{
> +	kref_get(&obj->kref);
> +}
> +
> +/**
> + * media_request_object_put - Put a media request object
> + *
> + * @obj: The object
> + *
> + * Put 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 a media request object
> + *
> + * Initialise a media request object. The object will be released using the
> + * release callback of the ops once it has no references (this function
> + * initialises references to one).
> + */
> +void media_request_object_init(struct media_request_object *obj);
> +
> +/**
> + * media_request_object_bind - Bind a media request object to a request
> + */
> +void media_request_object_bind(struct media_request *req,
> +			       const struct media_request_object_ops *ops,
> +			       void *priv,
> +			       struct media_request_object *obj);
> +
> +void media_request_object_unbind(struct media_request_object *obj);
> +
> +/**
> + * media_request_object_complete - Mark the media request object as complete
> + */
> +void media_request_object_complete(struct media_request_object *obj);
> +#else
> +static inline void media_request_object_get(struct media_request_object *obj)
> +{
> +}
> +
> +static inline void media_request_object_put(struct media_request_object *obj)
> +{
> +}
> +
> +static inline void media_request_object_init(struct media_request_object *obj)
> +{
> +	obj->ops = NULL;
> +	obj->req = NULL;
> +}
> +
> +static inline void media_request_object_bind(struct media_request *req,
> +			       const struct media_request_object_ops *ops,
> +			       void *priv,
> +			       struct media_request_object *obj)
> +{
> +}
> +
> +static inline void media_request_object_unbind(struct media_request_object *obj)
> +{
> +}
> +
> +static inline void media_request_object_complete(struct media_request_object *obj)
> +{
> +}
> +#endif
>  
>  #endif

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

Re: [RFCv9 PATCH 04/29] media-request: core request support

[Hans Verkuil]

On 06/04/18 22:35, Sakari Ailus wrote:
> Hi Hans,
> 
> Thanks for your work on this. A few comments below...
> 
> On Wed, Mar 28, 2018 at 03:50:05PM +0200, Hans Verkuil wrote:
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> Implement the core of the media request processing.
>>
>> Drivers can bind request objects to a request. These objects
>> can then be marked completed if the driver finished using them,
>> or just be unbound if the results do not need to be kept (e.g.
>> in the case of buffers).
>>
>> Once all objects that were added are either unbound or completed,
>> the request is marked 'complete' and a POLLPRI signal is sent
>> via poll.
>>
>> Both requests and request objects are refcounted.
>>
>> While a request is queued its refcount is incremented (since it
>> is in use by a driver). Once it is completed the refcount is
>> decremented. When the user closes the request file descriptor
>> the refcount is also decremented. Once it reaches 0 all request
>> objects in the request are unbound and put() and the request
>> itself is freed.
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>> ---
>>  drivers/media/media-request.c | 269 +++++++++++++++++++++++++++++++++++++++++-
>>  include/media/media-request.h | 148 +++++++++++++++++++++++
>>  2 files changed, 416 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/media-request.c b/drivers/media/media-request.c
>> index ead78613fdbe..8135d9d32af9 100644
>> --- a/drivers/media/media-request.c
>> +++ b/drivers/media/media-request.c
>> @@ -16,8 +16,275 @@
>>  #include <media/media-device.h>
>>  #include <media/media-request.h>
>>  
>> +static const char * const request_state[] = {
>> +	"idle",
>> +	"queueing",
>> +	"queued",
>> +	"complete",
>> +	"cleaning",
>> +};
>> +
>> +static const char *
>> +media_request_state_str(enum media_request_state state)
>> +{
>> +	if (WARN_ON(state >= ARRAY_SIZE(request_state)))
>> +		return "unknown";
>> +	return request_state[state];
>> +}
>> +
>> +static void media_request_clean(struct media_request *req)
>> +{
>> +	struct media_request_object *obj, *obj_safe;
>> +
>> +	WARN_ON(req->state != MEDIA_REQUEST_STATE_CLEANING);
>> +
>> +	list_for_each_entry_safe(obj, obj_safe, &req->objects, list) {
>> +		media_request_object_unbind(obj);
>> +		media_request_object_put(obj);
>> +	}
>> +
>> +	req->num_incomplete_objects = 0;
>> +	wake_up_interruptible(&req->poll_wait);
>> +}
>> +
>> +static void media_request_release(struct kref *kref)
>> +{
>> +	struct media_request *req =
>> +		container_of(kref, struct media_request, kref);
>> +	struct media_device *mdev = req->mdev;
>> +	unsigned long flags;
>> +
>> +	dev_dbg(mdev->dev, "request: release %s\n", req->debug_str);
>> +
>> +	spin_lock_irqsave(&req->lock, flags);
>> +	req->state = MEDIA_REQUEST_STATE_CLEANING;
>> +	spin_unlock_irqrestore(&req->lock, flags);
>> +
>> +	media_request_clean(req);
>> +
>> +	if (mdev->ops->req_free)
>> +		mdev->ops->req_free(req);
>> +	else
>> +		kfree(req);
>> +}
>> +
>> +void media_request_put(struct media_request *req)
>> +{
>> +	kref_put(&req->kref, media_request_release);
>> +}
>> +EXPORT_SYMBOL_GPL(media_request_put);
>> +
>> +static int media_request_close(struct inode *inode, struct file *filp)
>> +{
>> +	struct media_request *req = filp->private_data;
>> +
>> +	media_request_put(req);
> 
> One more newline?
> 
>> +	return 0;
>> +}
>> +
>> +static unsigned int media_request_poll(struct file *filp,
>> +				       struct poll_table_struct *wait)
>> +{
>> +	struct media_request *req = filp->private_data;
>> +	unsigned long flags;
>> +	enum media_request_state state;
>> +
>> +	if (!(poll_requested_events(wait) & POLLPRI))
>> +		return 0;
>> +
>> +	spin_lock_irqsave(&req->lock, flags);
>> +	state = req->state;
>> +	spin_unlock_irqrestore(&req->lock, flags);
>> +
>> +	if (state == MEDIA_REQUEST_STATE_COMPLETE)
>> +		return POLLPRI;
>> +	if (state == MEDIA_REQUEST_STATE_IDLE)
> 
> Should this be
> 
> 	if (state != MEDIA_REQUEST_STATE_QUEUED)

Good question. I'll take another look at this.

> 
> ? (Also see my other comment on the QUEUEING state below.)
> 
>> +		return POLLERR;
>> +
>> +	poll_wait(filp, &req->poll_wait, wait);
> 
> Newline?
> 
>> +	return 0;
>> +}
>> +
>> +static long media_request_ioctl(struct file *filp, unsigned int cmd,
>> +				unsigned long __arg)
>> +{
>> +	return -ENOIOCTLCMD;
>> +}
>> +
>> +static const struct file_operations request_fops = {
>> +	.owner = THIS_MODULE,
>> +	.poll = media_request_poll,
>> +	.unlocked_ioctl = media_request_ioctl,
>> +	.release = media_request_close,
>> +};
>> +
>>  int media_request_alloc(struct media_device *mdev,
>>  			struct media_request_alloc *alloc)
>>  {
>> -	return -ENOMEM;
>> +	struct media_request *req;
>> +	struct file *filp;
>> +	char comm[TASK_COMM_LEN];
>> +	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;
>> +	}
>> +
>> +	if (mdev->ops->req_alloc)
>> +		req = mdev->ops->req_alloc(mdev);
>> +	else
>> +		req = kzalloc(sizeof(*req), GFP_KERNEL);
>> +	if (!req) {
>> +		ret = -ENOMEM;
>> +		goto err_fput;
>> +	}
>> +
>> +	filp->private_data = req;
>> +	req->mdev = mdev;
>> +	req->state = MEDIA_REQUEST_STATE_IDLE;
>> +	req->num_incomplete_objects = 0;
>> +	kref_init(&req->kref);
>> +	INIT_LIST_HEAD(&req->objects);
>> +	spin_lock_init(&req->lock);
>> +	init_waitqueue_head(&req->poll_wait);
>> +
>> +	alloc->fd = fd;
>> +
>> +	get_task_comm(comm, current);
>> +	snprintf(req->debug_str, sizeof(req->debug_str), "%s:%d",
>> +		 comm, fd);
>> +	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;
>> +}
>> +
>> +static void media_request_object_release(struct kref *kref)
>> +{
>> +	struct media_request_object *obj =
>> +		container_of(kref, struct media_request_object, kref);
>> +	struct media_request *req = obj->req;
>> +
>> +	if (req)
>> +		media_request_object_unbind(obj);
> 
> Newline?
> 
>> +	obj->ops->release(obj);
>> +}
>> +
>> +void media_request_object_put(struct media_request_object *obj)
>> +{
>> +	kref_put(&obj->kref, media_request_object_release);
>> +}
>> +EXPORT_SYMBOL_GPL(media_request_object_put);
>> +
>> +void media_request_object_init(struct media_request_object *obj)
>> +{
>> +	obj->ops = NULL;
>> +	obj->req = NULL;
>> +	obj->priv = NULL;
>> +	obj->completed = false;
> 
> The function is intended to be used on a freshly allocated object only. The
> allocation function should already zero the memory, so there should be no
> need to do this here anymore.

Actually, that's not true. struct vb2_buffer contains struct media_request_object
but it is only allocated when you allocate the buffers, after that this has to
be initialized every time you queue or prepare a buffer for a request.

It's probably true for everything else though, but buffers are the exception.

> 
>> +	INIT_LIST_HEAD(&obj->list);
> 
> There's no need to init this as it's a member of a list, not a head.
> 
>> +	kref_init(&obj->kref);
>> +}
>> +EXPORT_SYMBOL_GPL(media_request_object_init);
>> +
>> +void media_request_object_bind(struct media_request *req,
>> +			       const struct media_request_object_ops *ops,
>> +			       void *priv,
>> +			       struct media_request_object *obj)
>> +{
>> +	unsigned long flags;
>> +
>> +	WARN_ON(!ops->release);
>> +	obj->req = req;
>> +	obj->ops = ops;
>> +	obj->priv = priv;
>> +	spin_lock_irqsave(&req->lock, flags);
>> +	if (WARN_ON(req->state != MEDIA_REQUEST_STATE_IDLE))
> 
> How do you find out that a request is not in an idle state? I'd say you
> can't be sure otherwise as a request may be queued whilst another process
> is binding an object to it. I think the check here should be used prevent
> that.

??? If a request is being queued then the state is QUEUEING and you cannot
bind this object due to the test above. That's also the reason for having the
QUEUEING state: to prevent binding an object to a request that is in the
process of being queued.

> 
> I'd also assign fields other than req in object init as objects may be
> unbound from requests and outlive requests (video buffers, for instance).
> No ops nor priv field may be changed during a lifetime of an object.

Sorry, no idea what you mean. Could it be that you want to move the ops and
priv arguments to the object_init() function instead of the bind()?

If that's the case, then I'll have to look at that. I think I did that at
one point in time, but I moved it to the bind for some reason.

> 
>> +		goto unlock;
>> +	list_add_tail(&obj->list, &req->objects);
>> +	req->num_incomplete_objects++;
>> +unlock:
>> +	spin_unlock_irqrestore(&req->lock, flags);
>> +}
>> +EXPORT_SYMBOL_GPL(media_request_object_bind);
>> +
>> +void media_request_object_unbind(struct media_request_object *obj)
>> +{
>> +	struct media_request *req = obj->req;
>> +	unsigned long flags;
>> +	bool completed = false;
>> +
>> +	if (!req)
>> +		return;
>> +
>> +	spin_lock_irqsave(&req->lock, flags);
>> +	list_del(&obj->list);
>> +	obj->req = NULL;
>> +
>> +	if (req->state == MEDIA_REQUEST_STATE_COMPLETE ||
>> +	    req->state == MEDIA_REQUEST_STATE_CLEANING)
>> +		goto unlock;
>> +
>> +	if (WARN_ON(req->state == MEDIA_REQUEST_STATE_QUEUEING))
>> +		goto unlock;
>> +
>> +	if (WARN_ON(!req->num_incomplete_objects))
>> +		goto unlock;
>> +
>> +	req->num_incomplete_objects--;
>> +	if (req->state == MEDIA_REQUEST_STATE_QUEUED &&
>> +	    !req->num_incomplete_objects) {
>> +		req->state = MEDIA_REQUEST_STATE_COMPLETE;
>> +		completed = true;
>> +		wake_up_interruptible(&req->poll_wait);
>> +	}
>> +unlock:
>> +	spin_unlock_irqrestore(&req->lock, flags);
>> +	if (obj->ops->unbind)
>> +		obj->ops->unbind(obj);
>> +	if (completed)
>> +		media_request_put(req);
>> +}
>> +EXPORT_SYMBOL_GPL(media_request_object_unbind);
>> +
>> +void media_request_object_complete(struct media_request_object *obj)
>> +{
>> +	struct media_request *req = obj->req;
>> +	unsigned long flags;
>> +	bool completed = false;
>> +
>> +	spin_lock_irqsave(&req->lock, flags);
>> +	if (obj->completed)
>> +		goto unlock;
>> +	obj->completed = true;
>> +	if (WARN_ON(!req->num_incomplete_objects) ||
>> +	    WARN_ON(req->state != MEDIA_REQUEST_STATE_QUEUED))
> 
> If an object completes very soon after a request is queued, it is possible
> that the request is still in QUEUEING state. I'm actually beginning to
> wonder whether this state is useful; would it bring trouble if we just put
> it to QUEUED right from the beginning?

I think it would be better to split the req_queue op in two ops, so
media_request_ioctl_queue() would do this (pseudo code):

	req->state = MEDIA_REQUEST_STATE_PREPARING;
	ret = req->ops->req_prepare(req);
	if (ret) {
		req->state = IDLE;
		return ret;
	}
	req->state = QUEUED;
	req->ops->req_queue(req);  // void function, must always succeed
	return 0;

I think this works safely.

> 
>> +		goto unlock;
>> +
>> +	if (!--req->num_incomplete_objects) {
>> +		req->state = MEDIA_REQUEST_STATE_COMPLETE;
>> +		wake_up_interruptible(&req->poll_wait);
>> +		completed = true;
>> +	}
> 
> Newline here...
> 
>> +unlock:
>> +	spin_unlock_irqrestore(&req->lock, flags);
> 
> and here?
> 
>> +	if (completed)
>> +		media_request_put(req);
>>  }
>> +EXPORT_SYMBOL_GPL(media_request_object_complete);
>> diff --git a/include/media/media-request.h b/include/media/media-request.h
>> index dae3eccd9aa7..baed99eb1279 100644
>> --- a/include/media/media-request.h
>> +++ b/include/media/media-request.h
>> @@ -16,7 +16,155 @@
>>  
>>  #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_CLEANING,
>> +};
>> +
>> +struct media_request_object;
>> +
>> +/**
>> + * struct media_request - Media device request
> 
> And here (plus " *").
> 
>> + * @mdev: Media device this request belongs to
>> + * @kref: Reference count
>> + * @debug_prefix: Prefix for debug messages (process name:fd)
>> + * @state: The state of the request
>> + * @objects: List of @struct media_request_object request objects
>> + * @num_objects: The number objects in the request
>> + * @num_completed_objects: The number of completed objects in the request
>> + * @poll_wait: Wait queue for poll
>> + * @lock: Serializes access to this struct
>> + */
>> +struct media_request {
>> +	struct media_device *mdev;
>> +	struct kref kref;
>> +	char debug_str[TASK_COMM_LEN + 11];
>> +	enum media_request_state state;
>> +	struct list_head objects;
>> +	unsigned int num_incomplete_objects;
>> +	struct wait_queue_head poll_wait;
>> +	spinlock_t lock;
>> +};
>> +
>> +#ifdef CONFIG_MEDIA_CONTROLLER
>> +static inline void media_request_get(struct media_request *req)
>> +{
>> +	kref_get(&req->kref);
>> +}
>> +
>> +void media_request_put(struct media_request *req);
>> +
>>  int media_request_alloc(struct media_device *mdev,
>>  			struct media_request_alloc *alloc);
>> +#else
>> +static inline void media_request_get(struct media_request *req)
>> +{
>> +}
>> +
>> +static inline void media_request_put(struct media_request *req)
>> +{
>> +}
>> +#endif
>> +
>> +struct media_request_object_ops {
>> +	int (*prepare)(struct media_request_object *object);
>> +	void (*unprepare)(struct media_request_object *object);
>> +	void (*queue)(struct media_request_object *object);
>> +	void (*unbind)(struct media_request_object *object);
>> +	void (*release)(struct media_request_object *object);
>> +};
>> +
>> +/**
>> + * struct media_request_object - An opaque object that belongs to a media
>> + *				 request
>> + *
>> + * @priv: object's priv pointer
>> + * @list: List entry of the object for @struct media_request
>> + * @kref: Reference count of the object, acquire before releasing req->lock
>> + *
>> + * An object related to the request. This struct is embedded in the
>> + * larger object data.
>> + */
>> +struct media_request_object {
>> +	const struct media_request_object_ops *ops;
>> +	void *priv;
>> +	struct media_request *req;
>> +	struct list_head list;
>> +	struct kref kref;
>> +	bool completed;
>> +};
>> +
>> +#ifdef CONFIG_MEDIA_CONTROLLER
>> +static inline void media_request_object_get(struct media_request_object *obj)
>> +{
>> +	kref_get(&obj->kref);
>> +}
>> +
>> +/**
>> + * media_request_object_put - Put a media request object
>> + *
>> + * @obj: The object
>> + *
>> + * Put 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 a media request object
>> + *
>> + * Initialise a media request object. The object will be released using the
>> + * release callback of the ops once it has no references (this function
>> + * initialises references to one).
>> + */
>> +void media_request_object_init(struct media_request_object *obj);
>> +
>> +/**
>> + * media_request_object_bind - Bind a media request object to a request
>> + */
>> +void media_request_object_bind(struct media_request *req,
>> +			       const struct media_request_object_ops *ops,
>> +			       void *priv,
>> +			       struct media_request_object *obj);
>> +
>> +void media_request_object_unbind(struct media_request_object *obj);
>> +
>> +/**
>> + * media_request_object_complete - Mark the media request object as complete
>> + */
>> +void media_request_object_complete(struct media_request_object *obj);
>> +#else
>> +static inline void media_request_object_get(struct media_request_object *obj)
>> +{
>> +}
>> +
>> +static inline void media_request_object_put(struct media_request_object *obj)
>> +{
>> +}
>> +
>> +static inline void media_request_object_init(struct media_request_object *obj)
>> +{
>> +	obj->ops = NULL;
>> +	obj->req = NULL;
>> +}
>> +
>> +static inline void media_request_object_bind(struct media_request *req,
>> +			       const struct media_request_object_ops *ops,
>> +			       void *priv,
>> +			       struct media_request_object *obj)
>> +{
>> +}
>> +
>> +static inline void media_request_object_unbind(struct media_request_object *obj)
>> +{
>> +}
>> +
>> +static inline void media_request_object_complete(struct media_request_object *obj)
>> +{
>> +}
>> +#endif
>>  
>>  #endif
> 

Regards,

	Hans

Re: [RFCv9 PATCH 07/29] media-request: add media_request_object_find

[Sakari Ailus]

Hi Hans,

On Wed, Mar 28, 2018 at 03:50:08PM +0200, Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Add media_request_object_find to find a request object inside a
> request based on ops and/or priv values.
> 
> Objects of the same type (vb2 buffer, control handler) will have
> the same ops value. And objects that refer to the same 'parent'
> object (e.g. the v4l2_ctrl_handler that has the current driver
> state) will have the same priv value.
> 
> The caller has to call media_request_object_put() for the returned
> object since this function increments the refcount.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/media/media-request.c | 26 ++++++++++++++++++++++++++
>  include/media/media-request.h | 25 +++++++++++++++++++++++++
>  2 files changed, 51 insertions(+)
> 
> diff --git a/drivers/media/media-request.c b/drivers/media/media-request.c
> index d54fd353d8a6..10a05dd7b571 100644
> --- a/drivers/media/media-request.c
> +++ b/drivers/media/media-request.c
> @@ -309,6 +309,32 @@ static void media_request_object_release(struct kref *kref)
>  	obj->ops->release(obj);
>  }
>  
> +struct media_request_object *
> +media_request_object_find(struct media_request *req,
> +			  const struct media_request_object_ops *ops,
> +			  void *priv)
> +{
> +	struct media_request_object *obj;
> +	struct media_request_object *found = NULL;
> +	unsigned long flags;
> +
> +	if (!ops && !priv)
> +		return NULL;
> +
> +	spin_lock_irqsave(&req->lock, flags);
> +	list_for_each_entry(obj, &req->objects, list) {
> +		if ((!ops || obj->ops == ops) &&
> +		    (!priv || obj->priv == priv)) {

Do you have a use case for having matching priv but mismatching ops?

I think it'd be useful to require drivers to provide unique priv,
considering that it's risky to let them use the same while the VB2 request
object ops are always the same.

> +			media_request_object_get(obj);
> +			found = obj;
> +			break;
> +		}
> +	}
> +	spin_unlock_irqrestore(&req->lock, flags);
> +	return found;
> +}
> +EXPORT_SYMBOL_GPL(media_request_object_find);
> +
>  void media_request_object_put(struct media_request_object *obj)
>  {
>  	kref_put(&obj->kref, media_request_object_release);
> diff --git a/include/media/media-request.h b/include/media/media-request.h
> index c01b05570a31..570f3a205776 100644
> --- a/include/media/media-request.h
> +++ b/include/media/media-request.h
> @@ -122,6 +122,23 @@ static inline void media_request_object_get(struct media_request_object *obj)
>   */
>  void media_request_object_put(struct media_request_object *obj);
>  
> +/**
> + * media_request_object_find - Find an object in a request
> + *
> + * @ops: Find an object with this ops value, may be NULL.
> + * @priv: Find an object with this priv value, may be NULL.
> + *
> + * At least one of @ops and @priv must be non-NULL. If one of
> + * these is NULL, then skip checking for that field.
> + *
> + * Returns NULL if not found or the object (the refcount is increased
> + * in that case).
> + */
> +struct media_request_object *
> +media_request_object_find(struct media_request *req,
> +			  const struct media_request_object_ops *ops,
> +			  void *priv);
> +
>  /**
>   * media_request_object_init - Initialise a media request object
>   *
> @@ -154,6 +171,14 @@ static inline void media_request_object_put(struct media_request_object *obj)
>  {
>  }
>  
> +static inline struct media_request_object *
> +media_request_object_find(struct media_request *req,
> +			  const struct media_request_object_ops *ops,
> +			  void *priv)
> +{
> +	return NULL;
> +}
> +
>  static inline void media_request_object_init(struct media_request_object *obj)
>  {
>  	obj->ops = NULL;
> -- 
> 2.16.1
> 

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

Re: [RFCv9 PATCH 15/29] videodev2.h: add request_fd field to v4l2_ext_controls

[Sakari Ailus]

Hi Hans,

On Wed, Mar 28, 2018 at 03:50:16PM +0200, Hans Verkuil wrote:
> From: Alexandre Courbot <acourbot@chromium.org>
> 
> If which is V4L2_CTRL_WHICH_REQUEST, then the request_fd field can be
> used to specify a request for the G/S/TRY_EXT_CTRLS ioctls.
> 
> Signed-off-by: Alexandre Courbot <acourbot@chromium.org>
> ---
>  drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 5 ++++-
>  drivers/media/v4l2-core/v4l2-ioctl.c          | 6 +++---
>  include/uapi/linux/videodev2.h                | 4 +++-
>  3 files changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> index 5198c9eeb348..0782b3666deb 100644
> --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> @@ -732,7 +732,8 @@ struct v4l2_ext_controls32 {
>  	__u32 which;
>  	__u32 count;
>  	__u32 error_idx;
> -	__u32 reserved[2];
> +	__s32 request_fd;
> +	__u32 reserved[1];

No need for an array anymore.

>  	compat_caddr_t controls; /* actually struct v4l2_ext_control32 * */
>  };
>  
> @@ -807,6 +808,7 @@ 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) ||
> +	    assign_in_user(&kp->request_fd, &up->request_fd) ||
>  	    copy_in_user(kp->reserved, up->reserved, sizeof(kp->reserved)))

And this can be assign_in_user().

>  		return -EFAULT;
>  
> @@ -865,6 +867,7 @@ 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) ||
> +	    assign_in_user(&up->request_fd, &kp->request_fd) ||

Ditto.

>  	    copy_in_user(up->reserved, kp->reserved, sizeof(up->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 a5dab16ff2d2..2c623da33155 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -553,8 +553,8 @@ static void v4l_print_ext_controls(const void *arg, bool write_only)
>  	const struct v4l2_ext_controls *p = arg;
>  	int i;
>  
> -	pr_cont("which=0x%x, count=%d, error_idx=%d",
> -			p->which, p->count, p->error_idx);
> +	pr_cont("which=0x%x, count=%d, error_idx=%d, request_fd=%d",
> +			p->which, p->count, p->error_idx, p->request_fd);
>  	for (i = 0; i < p->count; i++) {
>  		if (!p->controls[i].size)
>  			pr_cont(", id/val=0x%x/0x%x",
> @@ -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] = 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 600877be5c22..6f41baa53787 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -1592,7 +1592,8 @@ struct v4l2_ext_controls {
>  	};
>  	__u32 count;
>  	__u32 error_idx;
> -	__u32 reserved[2];
> +	__s32 request_fd;
> +	__u32 reserved[1];
>  	struct v4l2_ext_control *controls;
>  };
>  
> @@ -1605,6 +1606,7 @@ struct v4l2_ext_controls {
>  #define V4L2_CTRL_MAX_DIMS	  (4)
>  #define V4L2_CTRL_WHICH_CUR_VAL   0
>  #define V4L2_CTRL_WHICH_DEF_VAL   0x0f000000
> +#define V4L2_CTRL_WHICH_REQUEST   0x0f010000
>  
>  enum v4l2_ctrl_type {
>  	V4L2_CTRL_TYPE_INTEGER	     = 1,

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