[RFCv2 PATCH 00/11] Control Event

[RFCv2 PATCH 00/11] Control Event

[Hans Verkuil]

This is the second version of the patch series introducing a new event that
is triggered when a control's value or state changes.

It incorporates the comments made since version 1.

Most of these patches are relatively minor infrastructure changes. The real
work is done in patch 7.

This patch series builds on the bitmask patch series.

The main changes since version 1 are:

- The patches that add the bitmask control type are split off since these
  are needed sooner than the control events and they are indepedent of one
  another.

- Instead of having separate CTRL_CH_VALUE and CTRL_CH_STATE events, there
  is now just one V4L2_EVENT_CTRL event which has a bitmask telling what was
  changed since the last event. In addition, the event payload gives all the
  relevant control data (type, value, min, max, step, def, flags). This greatly
  simplifies the applications that need to use this as it prevents having
  to do additional calls to VIDIOC_G_CTRL or VIDIOC_QUERYCTRL.

- If you call VIDIOC_S_CTRL or VIDIOC_S_EXT_CTRLS, then the filehandle passed
  to the ioctl function will be skipped when the events for the new value
  are generated. This prevents nasty feedback loops.

- Documentation was added.

The vivi driver has been updated to support control events.

The qv4l2 application has also been updated to test control events.
You can find it here:

http://git.linuxtv.org/hverkuil/v4l-utils.git?a=shortlog;h=refs/heads/core

Please review! I'd like to get this in for 2.6.41.

Still on my TODO list (will be done as separate patch series):

- Change the way volatile controls are handled.

- Add autofoo/foo support.

- Make it possible to update control values from interrupt context. This will
  only be possible for a certain subset of controls.

- I need to figure out how to handle the case where there are two inputs, each
  with its own subdev and set of controls. Switching inputs would imply switching
  controls as well. I've tried several things, but it's all very awkward.

Regards,

	Hans

[RFCv2 PATCH 01/11] v4l2-ctrls: introduce call_op define

[Hans Verkuil]

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

Add the call_op define to safely call the control ops. This also allows
for controls without any ops such as the 'control class' controls.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/video/v4l2-ctrls.c |   19 +++++++++++--------
 1 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/media/video/v4l2-ctrls.c b/drivers/media/video/v4l2-ctrls.c
index 223e040..8b5f67f 100644
--- a/drivers/media/video/v4l2-ctrls.c
+++ b/drivers/media/video/v4l2-ctrls.c
@@ -25,6 +25,9 @@
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-dev.h>
 
+#define call_op(master, op) \
+	((master->ops && master->ops->op) ? master->ops->op(master) : 0)
+
 /* Internal temporary helper struct, one for each v4l2_ext_control */
 struct ctrl_helper {
 	/* The control corresponding to the v4l2_ext_control ID field. */
@@ -1306,7 +1309,7 @@ int v4l2_ctrl_handler_setup(struct v4l2_ctrl_handler *hdl)
 		if (ctrl->type == V4L2_CTRL_TYPE_BUTTON ||
 		    (ctrl->flags & V4L2_CTRL_FLAG_READ_ONLY))
 			continue;
-		ret = master->ops->s_ctrl(master);
+		ret = call_op(master, s_ctrl);
 		if (ret)
 			break;
 		for (i = 0; i < master->ncontrols; i++)
@@ -1583,8 +1586,8 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct v4l2_ext_controls *cs
 
 		v4l2_ctrl_lock(master);
 		/* g_volatile_ctrl will update the current control values */
-		if (ctrl->is_volatile && master->ops->g_volatile_ctrl)
-			ret = master->ops->g_volatile_ctrl(master);
+		if (ctrl->is_volatile)
+			ret = call_op(master, g_volatile_ctrl);
 		/* If OK, then copy the current control values to the caller */
 		if (!ret)
 			ret = cluster_walk(i, cs, helpers, cur_to_user);
@@ -1615,8 +1618,8 @@ static int get_ctrl(struct v4l2_ctrl *ctrl, s32 *val)
 
 	v4l2_ctrl_lock(master);
 	/* g_volatile_ctrl will update the current control values */
-	if (ctrl->is_volatile && master->ops->g_volatile_ctrl)
-		ret = master->ops->g_volatile_ctrl(master);
+	if (ctrl->is_volatile)
+		ret = call_op(master, g_volatile_ctrl);
 	*val = ctrl->cur.val;
 	v4l2_ctrl_unlock(master);
 	return ret;
@@ -1690,12 +1693,12 @@ static int try_or_set_control_cluster(struct v4l2_ctrl *master, bool set)
 	/* For larger clusters you have to call try_ctrl again to
 	   verify that the controls are still valid after the
 	   'cur_to_new' above. */
-	if (!ret && master->ops->try_ctrl && try)
-		ret = master->ops->try_ctrl(master);
+	if (!ret && try)
+		ret = call_op(master, try_ctrl);
 
 	/* Don't set if there is no change */
 	if (!ret && set && cluster_changed(master)) {
-		ret = master->ops->s_ctrl(master);
+		ret = call_op(master, s_ctrl);
 		/* If OK, then make the new values permanent. */
 		if (!ret)
 			for (i = 0; i < master->ncontrols; i++)
-- 
1.7.1

[RFCv2 PATCH 02/11] v4l2-ctrls: drivers should be able to ignore READ_ONLY and GRABBED flags

[Hans Verkuil]

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

When applications try to set READ_ONLY or GRABBED controls an error should
be returned. However, when drivers do that it should be accepted.

Those controls could reflect some driver status which the application
can't change but the driver obviously has to be able to change it.

This is needed among others for future HDMI status controls.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/video/v4l2-ctrls.c |   59 +++++++++++++++++++++++++-------------
 1 files changed, 39 insertions(+), 20 deletions(-)

diff --git a/drivers/media/video/v4l2-ctrls.c b/drivers/media/video/v4l2-ctrls.c
index 8b5f67f..2afd632 100644
--- a/drivers/media/video/v4l2-ctrls.c
+++ b/drivers/media/video/v4l2-ctrls.c
@@ -1673,11 +1673,6 @@ static int try_or_set_control_cluster(struct v4l2_ctrl *master, bool set)
 			continue;
 
 		if (ctrl->is_new) {
-			/* Double check this: it may have changed since the
-			   last check in try_or_set_ext_ctrls(). */
-			if (set && (ctrl->flags & V4L2_CTRL_FLAG_GRABBED))
-				return -EBUSY;
-
 			/* Validate if required */
 			if (!set)
 				ret = validate_new(ctrl);
@@ -1707,6 +1702,25 @@ static int try_or_set_control_cluster(struct v4l2_ctrl *master, bool set)
 	return ret;
 }
 
+/* Check if the flags allow you to set the modified controls. */
+static int check_flags_cluster(struct v4l2_ctrl *master)
+{
+	int i;
+
+	for (i = 0; i < master->ncontrols; i++) {
+		struct v4l2_ctrl *ctrl = master->cluster[i];
+
+		if (ctrl == NULL || !ctrl->is_new)
+			continue;
+
+		if (ctrl->flags & V4L2_CTRL_FLAG_READ_ONLY)
+			return -EACCES;
+		if (ctrl->flags & V4L2_CTRL_FLAG_GRABBED)
+			return -EBUSY;
+	}
+	return 0;
+}
+
 /* Try or set controls. */
 static int try_or_set_ext_ctrls(struct v4l2_ctrl_handler *hdl,
 				struct v4l2_ext_controls *cs,
@@ -1716,24 +1730,23 @@ static int try_or_set_ext_ctrls(struct v4l2_ctrl_handler *hdl,
 	unsigned i, j;
 	int ret = 0;
 
-	cs->error_idx = cs->count;
 	for (i = 0; i < cs->count; i++) {
 		struct v4l2_ctrl *ctrl = helpers[i].ctrl;
 
-		if (!set)
-			cs->error_idx = i;
+		cs->error_idx = i;
 
+		/* These tests are also done later in check_flags_cluster()
+		   which is called in atomic context, so that has the
+		   final say, but it makes sense to do an up-front
+		   check as well. Once an error occurs below some other
+		   controls may have been set already and we want to
+		   do a best-effort to avoid that. */
 		if (ctrl->flags & V4L2_CTRL_FLAG_READ_ONLY)
 			return -EACCES;
-		/* This test is also done in try_set_control_cluster() which
-		   is called in atomic context, so that has the final say,
-		   but it makes sense to do an up-front check as well. Once
-		   an error occurs in try_set_control_cluster() some other
-		   controls may have been set already and we want to do a
-		   best-effort to avoid that. */
 		if (set && (ctrl->flags & V4L2_CTRL_FLAG_GRABBED))
 			return -EBUSY;
 	}
+	cs->error_idx = cs->count;
 
 	for (i = 0; !ret && i < cs->count; i++) {
 		struct v4l2_ctrl *ctrl = helpers[i].ctrl;
@@ -1755,6 +1768,9 @@ static int try_or_set_ext_ctrls(struct v4l2_ctrl_handler *hdl,
 		   user_to_new() sets 'is_new' to 1. */
 		ret = cluster_walk(i, cs, helpers, user_to_new);
 
+		if (!ret && set)
+			ret = check_flags_cluster(master);
+
 		if (!ret)
 			ret = try_or_set_control_cluster(master, set);
 
@@ -1841,15 +1857,12 @@ int v4l2_subdev_s_ext_ctrls(struct v4l2_subdev *sd, struct v4l2_ext_controls *cs
 EXPORT_SYMBOL(v4l2_subdev_s_ext_ctrls);
 
 /* Helper function for VIDIOC_S_CTRL compatibility */
-static int set_ctrl(struct v4l2_ctrl *ctrl, s32 *val)
+static int set_ctrl(struct v4l2_ctrl *ctrl, s32 *val, bool check_flags)
 {
 	struct v4l2_ctrl *master = ctrl->cluster[0];
 	int ret;
 	int i;
 
-	if (ctrl->flags & V4L2_CTRL_FLAG_READ_ONLY)
-		return -EACCES;
-
 	v4l2_ctrl_lock(ctrl);
 
 	/* Reset the 'is_new' flags of the cluster */
@@ -1860,6 +1873,8 @@ static int set_ctrl(struct v4l2_ctrl *ctrl, s32 *val)
 	ctrl->val = *val;
 	ctrl->is_new = 1;
 	ret = try_or_set_control_cluster(master, false);
+	if (!ret && check_flags)
+		ret = check_flags_cluster(master);
 	if (!ret)
 		ret = try_or_set_control_cluster(master, true);
 	*val = ctrl->cur.val;
@@ -1874,7 +1889,7 @@ int v4l2_s_ctrl(struct v4l2_ctrl_handler *hdl, struct v4l2_control *control)
 	if (ctrl == NULL || !type_is_int(ctrl))
 		return -EINVAL;
 
-	return set_ctrl(ctrl, &control->value);
+	return set_ctrl(ctrl, &control->value, true);
 }
 EXPORT_SYMBOL(v4l2_s_ctrl);
 
@@ -1888,6 +1903,10 @@ int v4l2_ctrl_s_ctrl(struct v4l2_ctrl *ctrl, s32 val)
 {
 	/* It's a driver bug if this happens. */
 	WARN_ON(!type_is_int(ctrl));
-	return set_ctrl(ctrl, &val);
+
+	/* This function is called from within a driver. That means that the
+	   checks for READ_ONLY and GRABBED should be skipped. Those flags
+	   apply to userspace, but not to driver code. */
+	return set_ctrl(ctrl, &val, false);
 }
 EXPORT_SYMBOL(v4l2_ctrl_s_ctrl);
-- 
1.7.1

[RFCv2 PATCH 03/11] v4l2-ioctl: add ctrl_handler to v4l2_fh

[Hans Verkuil]

This is required to implement control events and is also needed to allow
for per-filehandle control handlers.

Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
---
 drivers/media/video/v4l2-fh.c    |    2 ++
 drivers/media/video/v4l2-ioctl.c |   36 +++++++++++++++++++++++++++---------
 include/media/v4l2-fh.h          |    2 ++
 3 files changed, 31 insertions(+), 9 deletions(-)

diff --git a/drivers/media/video/v4l2-fh.c b/drivers/media/video/v4l2-fh.c
index 717f71e..8635011 100644
--- a/drivers/media/video/v4l2-fh.c
+++ b/drivers/media/video/v4l2-fh.c
@@ -32,6 +32,8 @@
 int v4l2_fh_init(struct v4l2_fh *fh, struct video_device *vdev)
 {
 	fh->vdev = vdev;
+	/* Inherit from video_device. May be overridden by the driver. */
+	fh->ctrl_handler = vdev->ctrl_handler;
 	INIT_LIST_HEAD(&fh->list);
 	set_bit(V4L2_FL_USES_V4L2_FH, &fh->vdev->flags);
 	fh->prio = V4L2_PRIORITY_UNSET;
diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c
index 506edcc..75d475c 100644
--- a/drivers/media/video/v4l2-ioctl.c
+++ b/drivers/media/video/v4l2-ioctl.c
@@ -1418,7 +1418,9 @@ static long __video_do_ioctl(struct file *file,
 	{
 		struct v4l2_queryctrl *p = arg;
 
-		if (vfd->ctrl_handler)
+		if (vfh && vfh->ctrl_handler)
+			ret = v4l2_queryctrl(vfh->ctrl_handler, p);
+		else if (vfd->ctrl_handler)
 			ret = v4l2_queryctrl(vfd->ctrl_handler, p);
 		else if (ops->vidioc_queryctrl)
 			ret = ops->vidioc_queryctrl(file, fh, p);
@@ -1438,7 +1440,9 @@ static long __video_do_ioctl(struct file *file,
 	{
 		struct v4l2_control *p = arg;
 
-		if (vfd->ctrl_handler)
+		if (vfh && vfh->ctrl_handler)
+			ret = v4l2_g_ctrl(vfh->ctrl_handler, p);
+		else if (vfd->ctrl_handler)
 			ret = v4l2_g_ctrl(vfd->ctrl_handler, p);
 		else if (ops->vidioc_g_ctrl)
 			ret = ops->vidioc_g_ctrl(file, fh, p);
@@ -1470,12 +1474,16 @@ static long __video_do_ioctl(struct file *file,
 		struct v4l2_ext_controls ctrls;
 		struct v4l2_ext_control ctrl;
 
-		if (!vfd->ctrl_handler &&
+		if (!(vfh && vfh->ctrl_handler) && !vfd->ctrl_handler &&
 			!ops->vidioc_s_ctrl && !ops->vidioc_s_ext_ctrls)
 			break;
 
 		dbgarg(cmd, "id=0x%x, value=%d\n", p->id, p->value);
 
+		if (vfh && vfh->ctrl_handler) {
+			ret = v4l2_s_ctrl(vfh->ctrl_handler, p);
+			break;
+		}
 		if (vfd->ctrl_handler) {
 			ret = v4l2_s_ctrl(vfd->ctrl_handler, p);
 			break;
@@ -1501,7 +1509,9 @@ static long __video_do_ioctl(struct file *file,
 		struct v4l2_ext_controls *p = arg;
 
 		p->error_idx = p->count;
-		if (vfd->ctrl_handler)
+		if (vfh && vfh->ctrl_handler)
+			ret = v4l2_g_ext_ctrls(vfh->ctrl_handler, p);
+		else if (vfd->ctrl_handler)
 			ret = v4l2_g_ext_ctrls(vfd->ctrl_handler, p);
 		else if (ops->vidioc_g_ext_ctrls && check_ext_ctrls(p, 0))
 			ret = ops->vidioc_g_ext_ctrls(file, fh, p);
@@ -1515,10 +1525,13 @@ static long __video_do_ioctl(struct file *file,
 		struct v4l2_ext_controls *p = arg;
 
 		p->error_idx = p->count;
-		if (!vfd->ctrl_handler && !ops->vidioc_s_ext_ctrls)
+		if (!(vfh && vfh->ctrl_handler) && !vfd->ctrl_handler &&
+				!ops->vidioc_s_ext_ctrls)
 			break;
 		v4l_print_ext_ctrls(cmd, vfd, p, 1);
-		if (vfd->ctrl_handler)
+		if (vfh && vfh->ctrl_handler)
+			ret = v4l2_s_ext_ctrls(vfh->ctrl_handler, p);
+		else if (vfd->ctrl_handler)
 			ret = v4l2_s_ext_ctrls(vfd->ctrl_handler, p);
 		else if (check_ext_ctrls(p, 0))
 			ret = ops->vidioc_s_ext_ctrls(file, fh, p);
@@ -1529,10 +1542,13 @@ static long __video_do_ioctl(struct file *file,
 		struct v4l2_ext_controls *p = arg;
 
 		p->error_idx = p->count;
-		if (!vfd->ctrl_handler && !ops->vidioc_try_ext_ctrls)
+		if (!(vfh && vfh->ctrl_handler) && !vfd->ctrl_handler &&
+				!ops->vidioc_try_ext_ctrls)
 			break;
 		v4l_print_ext_ctrls(cmd, vfd, p, 1);
-		if (vfd->ctrl_handler)
+		if (vfh && vfh->ctrl_handler)
+			ret = v4l2_try_ext_ctrls(vfh->ctrl_handler, p);
+		else if (vfd->ctrl_handler)
 			ret = v4l2_try_ext_ctrls(vfd->ctrl_handler, p);
 		else if (check_ext_ctrls(p, 0))
 			ret = ops->vidioc_try_ext_ctrls(file, fh, p);
@@ -1542,7 +1558,9 @@ static long __video_do_ioctl(struct file *file,
 	{
 		struct v4l2_querymenu *p = arg;
 
-		if (vfd->ctrl_handler)
+		if (vfh && vfh->ctrl_handler)
+			ret = v4l2_querymenu(vfh->ctrl_handler, p);
+		else if (vfd->ctrl_handler)
 			ret = v4l2_querymenu(vfd->ctrl_handler, p);
 		else if (ops->vidioc_querymenu)
 			ret = ops->vidioc_querymenu(file, fh, p);
diff --git a/include/media/v4l2-fh.h b/include/media/v4l2-fh.h
index 0206aa5..d247111 100644
--- a/include/media/v4l2-fh.h
+++ b/include/media/v4l2-fh.h
@@ -30,11 +30,13 @@
 
 struct video_device;
 struct v4l2_events;
+struct v4l2_ctrl_handler;
 
 struct v4l2_fh {
 	struct list_head	list;
 	struct video_device	*vdev;
 	struct v4l2_events      *events; /* events, pending and subscribed */
+	struct v4l2_ctrl_handler *ctrl_handler;
 	enum v4l2_priority	prio;
 };
 
-- 
1.7.1

[RFCv2 PATCH 04/11] v4l2-ctrls: Replace v4l2_ctrl_activate/grab with v4l2_ctrl_flags.

[Hans Verkuil]

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

This more generic function makes it possible to have a single function
that takes care of flags handling, in particular with regards to sending
a control event when the flags change.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 Documentation/video4linux/v4l2-controls.txt |   28 +++++++------
 drivers/media/video/cx2341x.c               |   58 ++++++++++++++++----------
 drivers/media/video/saa7115.c               |    3 +-
 drivers/media/video/v4l2-ctrls.c            |   33 ++++-----------
 include/media/v4l2-ctrls.h                  |   34 +++++++---------
 5 files changed, 76 insertions(+), 80 deletions(-)

diff --git a/Documentation/video4linux/v4l2-controls.txt b/Documentation/video4linux/v4l2-controls.txt
index 881e7f4..bc24be4 100644
--- a/Documentation/video4linux/v4l2-controls.txt
+++ b/Documentation/video4linux/v4l2-controls.txt
@@ -381,24 +381,26 @@ Active and Grabbed Controls
 ===========================
 
 If you get more complex relationships between controls, then you may have to
-activate and deactivate controls. For example, if the Chroma AGC control is
-on, then the Chroma Gain control is inactive. That is, you may set it, but
-the value will not be used by the hardware as long as the automatic gain
-control is on. Typically user interfaces can disable such input fields.
+activate and deactivate controls. For example, depending on the chosen MPEG
+audio codec only the audio bitrate control correspending to that audio codec
+is marked 'active', all other audio bitrate controls will be marked 'inactive'.
+That is, you may set it, but the value will not be used by the hardware.
+Typically user interfaces will disable inactive input fields.
 
-You can set the 'active' status using v4l2_ctrl_activate(). By default all
-controls are active. Note that the framework does not check for this flag.
-It is meant purely for GUIs. The function is typically called from within
-s_ctrl.
+By default all controls are active. Note that the framework does not check
+for this flag. It is meant purely for GUIs.
 
-The other flag is the 'grabbed' flag. A grabbed control means that you cannot
+You may also have to grab controls. A grabbed control means that you cannot
 change it because it is in use by some resource. Typical examples are MPEG
 bitrate controls that cannot be changed while capturing is in progress.
 
-If a control is set to 'grabbed' using v4l2_ctrl_grab(), then the framework
-will return -EBUSY if an attempt is made to set this control. The
-v4l2_ctrl_grab() function is typically called from the driver when it
-starts or stops streaming.
+If a control is set to 'grabbed', then the framework will return -EBUSY if
+an attempt is made to set this control. You typically change the 'grabbed'
+status from the driver when it starts or stops streaming.
+
+You can change the control's status using the v4l2_ctrl_flags() or
+v4l2_ctrl_flags_lock() calls. The first can be called from within s_ctrl,
+the second can be called from elsewhere and will lock first.
 
 
 Control Clusters
diff --git a/drivers/media/video/cx2341x.c b/drivers/media/video/cx2341x.c
index 103ef6b..2781889 100644
--- a/drivers/media/video/cx2341x.c
+++ b/drivers/media/video/cx2341x.c
@@ -1307,6 +1307,12 @@ static int cx2341x_try_ctrl(struct v4l2_ctrl *ctrl)
 	return 0;
 }
 
+static void cx2341x_activate(struct v4l2_ctrl *ctrl, bool activate)
+{
+	v4l2_ctrl_flags(ctrl, V4L2_CTRL_FLAG_INACTIVE,
+			activate ? 0 : V4L2_CTRL_FLAG_INACTIVE);
+}
+
 static int cx2341x_s_ctrl(struct v4l2_ctrl *ctrl)
 {
 	static const int mpeg_stream_type[] = {
@@ -1380,10 +1386,10 @@ static int cx2341x_s_ctrl(struct v4l2_ctrl *ctrl)
 			int is_ac3 = hdl->audio_encoding->val ==
 						V4L2_MPEG_AUDIO_ENCODING_AC3;
 
-			v4l2_ctrl_activate(hdl->audio_ac3_bitrate, is_ac3);
-			v4l2_ctrl_activate(hdl->audio_l2_bitrate, !is_ac3);
+			cx2341x_activate(hdl->audio_ac3_bitrate, is_ac3);
+			cx2341x_activate(hdl->audio_l2_bitrate, !is_ac3);
 		}
-		v4l2_ctrl_activate(hdl->audio_mode_extension,
+		cx2341x_activate(hdl->audio_mode_extension,
 			hdl->audio_mode->val == V4L2_MPEG_AUDIO_MODE_JOINT_STEREO);
 		if (cx2341x_neq(hdl->audio_sampling_freq) &&
 		    hdl->ops && hdl->ops->s_audio_sampling_freq)
@@ -1413,9 +1419,9 @@ static int cx2341x_s_ctrl(struct v4l2_ctrl *ctrl)
 		if (err)
 			return err;
 
-		v4l2_ctrl_activate(hdl->video_bitrate_mode,
+		cx2341x_activate(hdl->video_bitrate_mode,
 			hdl->video_encoding->val != V4L2_MPEG_VIDEO_ENCODING_MPEG_1);
-		v4l2_ctrl_activate(hdl->video_bitrate_peak,
+		cx2341x_activate(hdl->video_bitrate_peak,
 			hdl->video_bitrate_mode->val != V4L2_MPEG_VIDEO_BITRATE_MODE_CBR);
 		if (cx2341x_neq(hdl->video_encoding) &&
 		    hdl->ops && hdl->ops->s_video_encoding)
@@ -1441,18 +1447,18 @@ static int cx2341x_s_ctrl(struct v4l2_ctrl *ctrl)
 
 		active_filter = hdl->video_spatial_filter_mode->val !=
 				V4L2_MPEG_CX2341X_VIDEO_SPATIAL_FILTER_MODE_AUTO;
-		v4l2_ctrl_activate(hdl->video_spatial_filter, active_filter);
-		v4l2_ctrl_activate(hdl->video_luma_spatial_filter_type, active_filter);
-		v4l2_ctrl_activate(hdl->video_chroma_spatial_filter_type, active_filter);
+		cx2341x_activate(hdl->video_spatial_filter, active_filter);
+		cx2341x_activate(hdl->video_luma_spatial_filter_type, active_filter);
+		cx2341x_activate(hdl->video_chroma_spatial_filter_type, active_filter);
 		active_filter = hdl->video_temporal_filter_mode->val !=
 				V4L2_MPEG_CX2341X_VIDEO_TEMPORAL_FILTER_MODE_AUTO;
-		v4l2_ctrl_activate(hdl->video_temporal_filter, active_filter);
+		cx2341x_activate(hdl->video_temporal_filter, active_filter);
 		active_filter = hdl->video_median_filter_type->val !=
 				V4L2_MPEG_CX2341X_VIDEO_MEDIAN_FILTER_TYPE_OFF;
-		v4l2_ctrl_activate(hdl->video_luma_median_filter_bottom, active_filter);
-		v4l2_ctrl_activate(hdl->video_luma_median_filter_top, active_filter);
-		v4l2_ctrl_activate(hdl->video_chroma_median_filter_bottom, active_filter);
-		v4l2_ctrl_activate(hdl->video_chroma_median_filter_top, active_filter);
+		cx2341x_activate(hdl->video_luma_median_filter_bottom, active_filter);
+		cx2341x_activate(hdl->video_luma_median_filter_top, active_filter);
+		cx2341x_activate(hdl->video_chroma_median_filter_bottom, active_filter);
+		cx2341x_activate(hdl->video_chroma_median_filter_top, active_filter);
 		return 0;
 	}
 
@@ -1711,16 +1717,24 @@ int cx2341x_handler_setup(struct cx2341x_handler *cxhdl)
 }
 EXPORT_SYMBOL(cx2341x_handler_setup);
 
+static void cx2341x_grab(struct v4l2_ctrl *ctrl, bool busy)
+{
+	v4l2_ctrl_flags(ctrl, V4L2_CTRL_FLAG_GRABBED,
+			busy ? V4L2_CTRL_FLAG_GRABBED : 0);
+}
+
 void cx2341x_handler_set_busy(struct cx2341x_handler *cxhdl, int busy)
 {
-	v4l2_ctrl_grab(cxhdl->audio_sampling_freq, busy);
-	v4l2_ctrl_grab(cxhdl->audio_encoding, busy);
-	v4l2_ctrl_grab(cxhdl->audio_l2_bitrate, busy);
-	v4l2_ctrl_grab(cxhdl->audio_ac3_bitrate, busy);
-	v4l2_ctrl_grab(cxhdl->stream_vbi_fmt, busy);
-	v4l2_ctrl_grab(cxhdl->stream_type, busy);
-	v4l2_ctrl_grab(cxhdl->video_bitrate_mode, busy);
-	v4l2_ctrl_grab(cxhdl->video_bitrate, busy);
-	v4l2_ctrl_grab(cxhdl->video_bitrate_peak, busy);
+	mutex_lock(&cxhdl->hdl.lock);
+	cx2341x_grab(cxhdl->audio_sampling_freq, busy);
+	cx2341x_grab(cxhdl->audio_encoding, busy);
+	cx2341x_grab(cxhdl->audio_l2_bitrate, busy);
+	cx2341x_grab(cxhdl->audio_ac3_bitrate, busy);
+	cx2341x_grab(cxhdl->stream_vbi_fmt, busy);
+	cx2341x_grab(cxhdl->stream_type, busy);
+	cx2341x_grab(cxhdl->video_bitrate_mode, busy);
+	cx2341x_grab(cxhdl->video_bitrate, busy);
+	cx2341x_grab(cxhdl->video_bitrate_peak, busy);
+	mutex_unlock(&cxhdl->hdl.lock);
 }
 EXPORT_SYMBOL(cx2341x_handler_set_busy);
diff --git a/drivers/media/video/saa7115.c b/drivers/media/video/saa7115.c
index 0db9092..ae4b299 100644
--- a/drivers/media/video/saa7115.c
+++ b/drivers/media/video/saa7115.c
@@ -793,7 +793,8 @@ static int saa711x_s_ctrl(struct v4l2_ctrl *ctrl)
 			saa711x_write(sd, R_0F_CHROMA_GAIN_CNTL, state->gain->val);
 		else
 			saa711x_write(sd, R_0F_CHROMA_GAIN_CNTL, state->gain->val | 0x80);
-		v4l2_ctrl_activate(state->gain, !state->agc->val);
+		v4l2_ctrl_flags(state->gain, V4L2_CTRL_FLAG_INACTIVE,
+				state->agc->val ? V4L2_CTRL_FLAG_INACTIVE : 0);
 		break;
 
 	default:
diff --git a/drivers/media/video/v4l2-ctrls.c b/drivers/media/video/v4l2-ctrls.c
index 2afd632..d895048 100644
--- a/drivers/media/video/v4l2-ctrls.c
+++ b/drivers/media/video/v4l2-ctrls.c
@@ -1173,40 +1173,23 @@ void v4l2_ctrl_cluster(unsigned ncontrols, struct v4l2_ctrl **controls)
 }
 EXPORT_SYMBOL(v4l2_ctrl_cluster);
 
-/* Activate/deactivate a control. */
-void v4l2_ctrl_activate(struct v4l2_ctrl *ctrl, bool active)
+void v4l2_ctrl_flags(struct v4l2_ctrl *ctrl, u32 clear_flags, u32 set_flags)
 {
 	if (ctrl == NULL)
 		return;
-
-	if (!active)
-		/* set V4L2_CTRL_FLAG_INACTIVE */
-		set_bit(4, &ctrl->flags);
-	else
-		/* clear V4L2_CTRL_FLAG_INACTIVE */
-		clear_bit(4, &ctrl->flags);
+	ctrl->flags = (ctrl->flags & ~clear_flags) | set_flags;
 }
-EXPORT_SYMBOL(v4l2_ctrl_activate);
+EXPORT_SYMBOL(v4l2_ctrl_flags);
 
-/* Grab/ungrab a control.
-   Typically used when streaming starts and you want to grab controls,
-   preventing the user from changing them.
-
-   Just call this and the framework will block any attempts to change
-   these controls. */
-void v4l2_ctrl_grab(struct v4l2_ctrl *ctrl, bool grabbed)
+void v4l2_ctrl_flags_lock(struct v4l2_ctrl *ctrl, u32 clear_flags, u32 set_flags)
 {
 	if (ctrl == NULL)
 		return;
-
-	if (grabbed)
-		/* set V4L2_CTRL_FLAG_GRABBED */
-		set_bit(1, &ctrl->flags);
-	else
-		/* clear V4L2_CTRL_FLAG_GRABBED */
-		clear_bit(1, &ctrl->flags);
+	v4l2_ctrl_lock(ctrl);
+	v4l2_ctrl_flags(ctrl, clear_flags, set_flags);
+	v4l2_ctrl_unlock(ctrl);
 }
-EXPORT_SYMBOL(v4l2_ctrl_grab);
+EXPORT_SYMBOL(v4l2_ctrl_flags_lock);
 
 /* Log the control name and value */
 static void log_ctrl(const struct v4l2_ctrl *ctrl,
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index 97d0638..0f5004b 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -372,32 +372,28 @@ void v4l2_ctrl_cluster(unsigned ncontrols, struct v4l2_ctrl **controls);
   */
 struct v4l2_ctrl *v4l2_ctrl_find(struct v4l2_ctrl_handler *hdl, u32 id);
 
-/** v4l2_ctrl_activate() - Make the control active or inactive.
-  * @ctrl:	The control to (de)activate.
-  * @active:	True if the control should become active.
+/** v4l2_ctrl_flags() - Clear and set flags for a control.
+  * @ctrl:	The control whose flags should be changed.
+  * @clear_flags:	Mask out these flags.
+  * @set_flags:	Set these flags.
   *
-  * This sets or clears the V4L2_CTRL_FLAG_INACTIVE flag atomically.
-  * Does nothing if @ctrl == NULL.
-  * This will usually be called from within the s_ctrl op.
+  * This clears and sets flags. Does nothing if @ctrl == NULL.
   *
-  * This function can be called regardless of whether the control handler
-  * is locked or not.
+  * This function expects that the control handler is locked.
   */
-void v4l2_ctrl_activate(struct v4l2_ctrl *ctrl, bool active);
+void v4l2_ctrl_flags(struct v4l2_ctrl *ctrl, u32 clear_flags, u32 set_flags);
 
-/** v4l2_ctrl_grab() - Mark the control as grabbed or not grabbed.
-  * @ctrl:	The control to (de)activate.
-  * @grabbed:	True if the control should become grabbed.
+/** v4l2_ctrl_flags_lock() - Clear and set flags for a control.
+  * @ctrl:	The control whose flags should be changed.
+  * @clear_flags:	Mask out these flags.
+  * @set_flags:	Set these flags.
   *
-  * This sets or clears the V4L2_CTRL_FLAG_GRABBED flag atomically.
-  * Does nothing if @ctrl == NULL.
-  * This will usually be called when starting or stopping streaming in the
-  * driver.
+  * This clears and sets flags. Does nothing if @ctrl == NULL.
   *
-  * This function can be called regardless of whether the control handler
-  * is locked or not.
+  * This function expects that the control handler is unlocked and will lock
+  * it before changing flags.
   */
-void v4l2_ctrl_grab(struct v4l2_ctrl *ctrl, bool grabbed);
+void v4l2_ctrl_flags_lock(struct v4l2_ctrl *ctrl, u32 clear_flags, u32 set_flags);
 
 /** v4l2_ctrl_lock() - Helper function to lock the handler
   * associated with the control.
-- 
1.7.1

Re: [RFCv2 PATCH 04/11] v4l2-ctrls: Replace v4l2_ctrl_activate/grab with v4l2_ctrl_flags.

[Laurent Pinchart]

Hi Hans,

Thanks for the patch.

On Wednesday 25 May 2011 15:33:48 Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> This more generic function makes it possible to have a single function
> that takes care of flags handling, in particular with regards to sending
> a control event when the flags change.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---

[snip]

> +/** v4l2_ctrl_flags_lock() - Clear and set flags for a control.
> +  * @ctrl:	The control whose flags should be changed.
> +  * @clear_flags:	Mask out these flags.
> +  * @set_flags:	Set these flags.
>    *
> -  * This sets or clears the V4L2_CTRL_FLAG_GRABBED flag atomically.
> -  * Does nothing if @ctrl == NULL.
> -  * This will usually be called when starting or stopping streaming in the
> -  * driver.
> +  * This clears and sets flags. Does nothing if @ctrl == NULL.
>    *
> -  * This function can be called regardless of whether the control handler
> -  * is locked or not.
> +  * This function expects that the control handler is unlocked and will
> lock +  * it before changing flags.
>    */
> -void v4l2_ctrl_grab(struct v4l2_ctrl *ctrl, bool grabbed);
> +void v4l2_ctrl_flags_lock(struct v4l2_ctrl *ctrl, u32 clear_flags, u32
> set_flags);

The v4l2_ctrl_flags_lock() function doesn't seem to be used. Do we need it ?

-- 
Regards,

Laurent Pinchart

Re: [RFCv2 PATCH 04/11] v4l2-ctrls: Replace v4l2_ctrl_activate/grab with v4l2_ctrl_flags.

[Hans Verkuil]

On Friday, June 03, 2011 21:55:59 Laurent Pinchart wrote:
> Hi Hans,
> 
> Thanks for the patch.
> 
> On Wednesday 25 May 2011 15:33:48 Hans Verkuil wrote:
> > From: Hans Verkuil <hans.verkuil@cisco.com>
> > 
> > This more generic function makes it possible to have a single function
> > that takes care of flags handling, in particular with regards to sending
> > a control event when the flags change.
> > 
> > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> > ---
> 
> [snip]
> 
> > +/** v4l2_ctrl_flags_lock() - Clear and set flags for a control.
> > +  * @ctrl:	The control whose flags should be changed.
> > +  * @clear_flags:	Mask out these flags.
> > +  * @set_flags:	Set these flags.
> >    *
> > -  * This sets or clears the V4L2_CTRL_FLAG_GRABBED flag atomically.
> > -  * Does nothing if @ctrl == NULL.
> > -  * This will usually be called when starting or stopping streaming in the
> > -  * driver.
> > +  * This clears and sets flags. Does nothing if @ctrl == NULL.
> >    *
> > -  * This function can be called regardless of whether the control handler
> > -  * is locked or not.
> > +  * This function expects that the control handler is unlocked and will
> > lock +  * it before changing flags.
> >    */
> > -void v4l2_ctrl_grab(struct v4l2_ctrl *ctrl, bool grabbed);
> > +void v4l2_ctrl_flags_lock(struct v4l2_ctrl *ctrl, u32 clear_flags, u32
> > set_flags);
> 
> The v4l2_ctrl_flags_lock() function doesn't seem to be used. Do we need it ?
> 
> 

It is likely that I will (partially?) revert this patch. The idea for
v4l2_ctrl_flags(_lock) was to simplify changing the READ_ONLY flag on
the fly for autofoo/foo type controls. But I've changed my opinion on that.
See also the mail I sent earlier:

http://www.mail-archive.com/linux-media@vger.kernel.org/msg32332.html

Regards,

	Hans

[RFCv2 PATCH 05/11] v4l2-ctrls: add v4l2_fh pointer to the set control functions.

[Hans Verkuil]

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

When an application changes a control you want to generate an event.
However, you want to avoid sending such an event back to the application
(file handle) that caused the change.

Add the filehandle to the various set control functions.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/video/v4l2-ctrls.c  |   45 ++++++++++++++++++++----------------
 drivers/media/video/v4l2-ioctl.c  |    8 +++---
 drivers/media/video/v4l2-subdev.c |    4 +-
 include/media/v4l2-ctrls.h        |    8 ++++--
 4 files changed, 36 insertions(+), 29 deletions(-)

diff --git a/drivers/media/video/v4l2-ctrls.c b/drivers/media/video/v4l2-ctrls.c
index d895048..3e0d8ab 100644
--- a/drivers/media/video/v4l2-ctrls.c
+++ b/drivers/media/video/v4l2-ctrls.c
@@ -627,7 +627,7 @@ static int new_to_user(struct v4l2_ext_control *c,
 }
 
 /* Copy the new value to the current value. */
-static void new_to_cur(struct v4l2_ctrl *ctrl)
+static void new_to_cur(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl)
 {
 	if (ctrl == NULL)
 		return;
@@ -1639,7 +1639,8 @@ EXPORT_SYMBOL(v4l2_ctrl_g_ctrl);
 /* Core function that calls try/s_ctrl and ensures that the new value is
    copied to the current value on a set.
    Must be called with ctrl->handler->lock held. */
-static int try_or_set_control_cluster(struct v4l2_ctrl *master, bool set)
+static int try_or_set_control_cluster(struct v4l2_fh *fh,
+					struct v4l2_ctrl *master, bool set)
 {
 	bool try = !set;
 	int ret = 0;
@@ -1680,7 +1681,7 @@ static int try_or_set_control_cluster(struct v4l2_ctrl *master, bool set)
 		/* If OK, then make the new values permanent. */
 		if (!ret)
 			for (i = 0; i < master->ncontrols; i++)
-				new_to_cur(master->cluster[i]);
+				new_to_cur(fh, master->cluster[i]);
 	}
 	return ret;
 }
@@ -1705,7 +1706,8 @@ static int check_flags_cluster(struct v4l2_ctrl *master)
 }
 
 /* Try or set controls. */
-static int try_or_set_ext_ctrls(struct v4l2_ctrl_handler *hdl,
+static int try_or_set_ext_ctrls(struct v4l2_fh *fh,
+				struct v4l2_ctrl_handler *hdl,
 				struct v4l2_ext_controls *cs,
 				struct ctrl_helper *helpers,
 				bool set)
@@ -1755,7 +1757,7 @@ static int try_or_set_ext_ctrls(struct v4l2_ctrl_handler *hdl,
 			ret = check_flags_cluster(master);
 
 		if (!ret)
-			ret = try_or_set_control_cluster(master, set);
+			ret = try_or_set_control_cluster(fh, master, set);
 
 		/* Copy the new values back to userspace. */
 		if (!ret)
@@ -1768,7 +1770,7 @@ static int try_or_set_ext_ctrls(struct v4l2_ctrl_handler *hdl,
 }
 
 /* Try or try-and-set controls */
-static int try_set_ext_ctrls(struct v4l2_ctrl_handler *hdl,
+static int try_set_ext_ctrls(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
 			     struct v4l2_ext_controls *cs,
 			     bool set)
 {
@@ -1796,7 +1798,7 @@ static int try_set_ext_ctrls(struct v4l2_ctrl_handler *hdl,
 		goto free;
 
 	/* First 'try' all controls and abort on error */
-	ret = try_or_set_ext_ctrls(hdl, cs, helpers, false);
+	ret = try_or_set_ext_ctrls(NULL, hdl, cs, helpers, false);
 	/* If this is a 'set' operation and the initial 'try' failed,
 	   then set error_idx to count to tell the application that no
 	   controls changed value yet. */
@@ -1806,7 +1808,7 @@ static int try_set_ext_ctrls(struct v4l2_ctrl_handler *hdl,
 		/* Reset 'handled' state */
 		for (i = 0; i < cs->count; i++)
 			helpers[i].handled = false;
-		ret = try_or_set_ext_ctrls(hdl, cs, helpers, true);
+		ret = try_or_set_ext_ctrls(fh, hdl, cs, helpers, true);
 	}
 
 free:
@@ -1817,30 +1819,32 @@ free:
 
 int v4l2_try_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct v4l2_ext_controls *cs)
 {
-	return try_set_ext_ctrls(hdl, cs, false);
+	return try_set_ext_ctrls(NULL, hdl, cs, false);
 }
 EXPORT_SYMBOL(v4l2_try_ext_ctrls);
 
-int v4l2_s_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct v4l2_ext_controls *cs)
+int v4l2_s_ext_ctrls(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
+						struct v4l2_ext_controls *cs)
 {
-	return try_set_ext_ctrls(hdl, cs, true);
+	return try_set_ext_ctrls(fh, hdl, cs, true);
 }
 EXPORT_SYMBOL(v4l2_s_ext_ctrls);
 
 int v4l2_subdev_try_ext_ctrls(struct v4l2_subdev *sd, struct v4l2_ext_controls *cs)
 {
-	return try_set_ext_ctrls(sd->ctrl_handler, cs, false);
+	return try_set_ext_ctrls(NULL, sd->ctrl_handler, cs, false);
 }
 EXPORT_SYMBOL(v4l2_subdev_try_ext_ctrls);
 
 int v4l2_subdev_s_ext_ctrls(struct v4l2_subdev *sd, struct v4l2_ext_controls *cs)
 {
-	return try_set_ext_ctrls(sd->ctrl_handler, cs, true);
+	return try_set_ext_ctrls(NULL, sd->ctrl_handler, cs, true);
 }
 EXPORT_SYMBOL(v4l2_subdev_s_ext_ctrls);
 
 /* Helper function for VIDIOC_S_CTRL compatibility */
-static int set_ctrl(struct v4l2_ctrl *ctrl, s32 *val, bool check_flags)
+static int set_ctrl(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, s32 *val,
+							bool check_flags)
 {
 	struct v4l2_ctrl *master = ctrl->cluster[0];
 	int ret;
@@ -1855,30 +1859,31 @@ static int set_ctrl(struct v4l2_ctrl *ctrl, s32 *val, bool check_flags)
 
 	ctrl->val = *val;
 	ctrl->is_new = 1;
-	ret = try_or_set_control_cluster(master, false);
+	ret = try_or_set_control_cluster(NULL, master, false);
 	if (!ret && check_flags)
 		ret = check_flags_cluster(master);
 	if (!ret)
-		ret = try_or_set_control_cluster(master, true);
+		ret = try_or_set_control_cluster(fh, master, true);
 	*val = ctrl->cur.val;
 	v4l2_ctrl_unlock(ctrl);
 	return ret;
 }
 
-int v4l2_s_ctrl(struct v4l2_ctrl_handler *hdl, struct v4l2_control *control)
+int v4l2_s_ctrl(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
+					struct v4l2_control *control)
 {
 	struct v4l2_ctrl *ctrl = v4l2_ctrl_find(hdl, control->id);
 
 	if (ctrl == NULL || !type_is_int(ctrl))
 		return -EINVAL;
 
-	return set_ctrl(ctrl, &control->value, true);
+	return set_ctrl(fh, ctrl, &control->value, true);
 }
 EXPORT_SYMBOL(v4l2_s_ctrl);
 
 int v4l2_subdev_s_ctrl(struct v4l2_subdev *sd, struct v4l2_control *control)
 {
-	return v4l2_s_ctrl(sd->ctrl_handler, control);
+	return v4l2_s_ctrl(NULL, sd->ctrl_handler, control);
 }
 EXPORT_SYMBOL(v4l2_subdev_s_ctrl);
 
@@ -1890,6 +1895,6 @@ int v4l2_ctrl_s_ctrl(struct v4l2_ctrl *ctrl, s32 val)
 	/* This function is called from within a driver. That means that the
 	   checks for READ_ONLY and GRABBED should be skipped. Those flags
 	   apply to userspace, but not to driver code. */
-	return set_ctrl(ctrl, &val, false);
+	return set_ctrl(NULL, ctrl, &val, false);
 }
 EXPORT_SYMBOL(v4l2_ctrl_s_ctrl);
diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c
index 75d475c..660b486 100644
--- a/drivers/media/video/v4l2-ioctl.c
+++ b/drivers/media/video/v4l2-ioctl.c
@@ -1481,11 +1481,11 @@ static long __video_do_ioctl(struct file *file,
 		dbgarg(cmd, "id=0x%x, value=%d\n", p->id, p->value);
 
 		if (vfh && vfh->ctrl_handler) {
-			ret = v4l2_s_ctrl(vfh->ctrl_handler, p);
+			ret = v4l2_s_ctrl(vfh, vfh->ctrl_handler, p);
 			break;
 		}
 		if (vfd->ctrl_handler) {
-			ret = v4l2_s_ctrl(vfd->ctrl_handler, p);
+			ret = v4l2_s_ctrl(NULL, vfd->ctrl_handler, p);
 			break;
 		}
 		if (ops->vidioc_s_ctrl) {
@@ -1530,9 +1530,9 @@ static long __video_do_ioctl(struct file *file,
 			break;
 		v4l_print_ext_ctrls(cmd, vfd, p, 1);
 		if (vfh && vfh->ctrl_handler)
-			ret = v4l2_s_ext_ctrls(vfh->ctrl_handler, p);
+			ret = v4l2_s_ext_ctrls(vfh, vfh->ctrl_handler, p);
 		else if (vfd->ctrl_handler)
-			ret = v4l2_s_ext_ctrls(vfd->ctrl_handler, p);
+			ret = v4l2_s_ext_ctrls(NULL, vfd->ctrl_handler, p);
 		else if (check_ext_ctrls(p, 0))
 			ret = ops->vidioc_s_ext_ctrls(file, fh, p);
 		break;
diff --git a/drivers/media/video/v4l2-subdev.c b/drivers/media/video/v4l2-subdev.c
index 812729e..be4cbdd 100644
--- a/drivers/media/video/v4l2-subdev.c
+++ b/drivers/media/video/v4l2-subdev.c
@@ -164,13 +164,13 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 		return v4l2_g_ctrl(sd->ctrl_handler, arg);
 
 	case VIDIOC_S_CTRL:
-		return v4l2_s_ctrl(sd->ctrl_handler, arg);
+		return v4l2_s_ctrl(vfh, sd->ctrl_handler, arg);
 
 	case VIDIOC_G_EXT_CTRLS:
 		return v4l2_g_ext_ctrls(sd->ctrl_handler, arg);
 
 	case VIDIOC_S_EXT_CTRLS:
-		return v4l2_s_ext_ctrls(sd->ctrl_handler, arg);
+		return v4l2_s_ext_ctrls(vfh, sd->ctrl_handler, arg);
 
 	case VIDIOC_TRY_EXT_CTRLS:
 		return v4l2_try_ext_ctrls(sd->ctrl_handler, arg);
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index 0f5004b..151bd2e 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -28,6 +28,7 @@
 /* forward references */
 struct v4l2_ctrl_handler;
 struct v4l2_ctrl;
+struct v4l2_fh;
 struct video_device;
 struct v4l2_subdev;
 
@@ -436,15 +437,16 @@ s32 v4l2_ctrl_g_ctrl(struct v4l2_ctrl *ctrl);
   */
 int v4l2_ctrl_s_ctrl(struct v4l2_ctrl *ctrl, s32 val);
 
-
 /* Helpers for ioctl_ops. If hdl == NULL then they will all return -EINVAL. */
 int v4l2_queryctrl(struct v4l2_ctrl_handler *hdl, struct v4l2_queryctrl *qc);
 int v4l2_querymenu(struct v4l2_ctrl_handler *hdl, struct v4l2_querymenu *qm);
 int v4l2_g_ctrl(struct v4l2_ctrl_handler *hdl, struct v4l2_control *ctrl);
-int v4l2_s_ctrl(struct v4l2_ctrl_handler *hdl, struct v4l2_control *ctrl);
+int v4l2_s_ctrl(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
+						struct v4l2_control *ctrl);
 int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct v4l2_ext_controls *c);
 int v4l2_try_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct v4l2_ext_controls *c);
-int v4l2_s_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct v4l2_ext_controls *c);
+int v4l2_s_ext_ctrls(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
+						struct v4l2_ext_controls *c);
 
 /* Helpers for subdevices. If the associated ctrl_handler == NULL then they
    will all return -EINVAL. */
-- 
1.7.1

Re: [RFCv2 PATCH 05/11] v4l2-ctrls: add v4l2_fh pointer to the set control functions.

[Laurent Pinchart]

Hi Hans,

Thanks for the patch.

On Wednesday 25 May 2011 15:33:49 Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> When an application changes a control you want to generate an event.
> However, you want to avoid sending such an event back to the application
> (file handle) that caused the change.
> 
> Add the filehandle to the various set control functions.

To implement per-file handle controls, the get/try functions will need the 
file handle as well. Should this patch handle that, or do you want to postpone 
it until a driver uses per-file handle controls ?

-- 
Regards,

Laurent Pinchart

Re: [RFCv2 PATCH 05/11] v4l2-ctrls: add v4l2_fh pointer to the set control functions.

[Hans Verkuil]

On Friday, June 03, 2011 21:55:23 Laurent Pinchart wrote:
> Hi Hans,
> 
> Thanks for the patch.
> 
> On Wednesday 25 May 2011 15:33:49 Hans Verkuil wrote:
> > From: Hans Verkuil <hans.verkuil@cisco.com>
> > 
> > When an application changes a control you want to generate an event.
> > However, you want to avoid sending such an event back to the application
> > (file handle) that caused the change.
> > 
> > Add the filehandle to the various set control functions.
> 
> To implement per-file handle controls, the get/try functions will need the 
> file handle as well. Should this patch handle that, or do you want to postpone 
> it until a driver uses per-file handle controls ?

No, this has nothing to do with per-filehandle controls. This filehandle is
needed to prevent events from being sent to the filehandle that caused the
change (leads to nasty feedback-loops). Since get and try will never raise
control events I do not need the filehandle there.

It's patches 3/11 and 11/11 that actually add support for per-filehandle controls.

Regards,

	Hans

[RFCv2 PATCH 06/11] vb2_poll: don't start DMA, leave that to the first read().

[Hans Verkuil]

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

The vb2_poll function would start read-DMA if called without any streaming
in progress. This unfortunately does not work if the application just wants
to poll for exceptions. This information of what the application is polling
for is sadly unavailable in the driver.

Andy Walls suggested to just return POLLIN | POLLRDNORM and let the first
call to read() start the DMA. This initial read() call will return EAGAIN
since no actual data is available yet, but it does start the DMA.

Applications must handle EAGAIN in any case since there can be other reasons
for EAGAIN as well.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/video/videobuf2-core.c |   17 +++--------------
 1 files changed, 3 insertions(+), 14 deletions(-)

diff --git a/drivers/media/video/videobuf2-core.c b/drivers/media/video/videobuf2-core.c
index 6ba1461..ad75c95 100644
--- a/drivers/media/video/videobuf2-core.c
+++ b/drivers/media/video/videobuf2-core.c
@@ -1372,27 +1372,16 @@ static int __vb2_cleanup_fileio(struct vb2_queue *q);
 unsigned int vb2_poll(struct vb2_queue *q, struct file *file, poll_table *wait)
 {
 	unsigned long flags;
-	unsigned int ret;
 	struct vb2_buffer *vb = NULL;
 
 	/*
 	 * Start file I/O emulator only if streaming API has not been used yet.
 	 */
 	if (q->num_buffers == 0 && q->fileio == NULL) {
-		if (!V4L2_TYPE_IS_OUTPUT(q->type) && (q->io_modes & VB2_READ)) {
-			ret = __vb2_init_fileio(q, 1);
-			if (ret)
-				return POLLERR;
-		}
-		if (V4L2_TYPE_IS_OUTPUT(q->type) && (q->io_modes & VB2_WRITE)) {
-			ret = __vb2_init_fileio(q, 0);
-			if (ret)
-				return POLLERR;
-			/*
-			 * Write to OUTPUT queue can be done immediately.
-			 */
+		if (!V4L2_TYPE_IS_OUTPUT(q->type) && (q->io_modes & VB2_READ))
+			return POLLIN | POLLRDNORM;
+		if (V4L2_TYPE_IS_OUTPUT(q->type) && (q->io_modes & VB2_WRITE))
 			return POLLOUT | POLLWRNORM;
-		}
 	}
 
 	/*
-- 
1.7.1

[RFCv2 PATCH 07/11] v4l2-ctrls: add control events.

[Hans Verkuil]

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

Whenever a control changes value or state an event is sent to anyone
that subscribed to it.

This functionality is useful for control panels but also for applications
that need to wait for (usually status) controls to change value.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/video/v4l2-ctrls.c |  101 ++++++++++++++++++++++++++++++
 drivers/media/video/v4l2-event.c |  126 +++++++++++++++++++++++++++-----------
 drivers/media/video/v4l2-fh.c    |    4 +-
 include/linux/videodev2.h        |   29 ++++++++-
 include/media/v4l2-ctrls.h       |   16 +++++-
 include/media/v4l2-event.h       |    2 +
 6 files changed, 237 insertions(+), 41 deletions(-)

diff --git a/drivers/media/video/v4l2-ctrls.c b/drivers/media/video/v4l2-ctrls.c
index 3e0d8ab..e2a7ac7 100644
--- a/drivers/media/video/v4l2-ctrls.c
+++ b/drivers/media/video/v4l2-ctrls.c
@@ -23,6 +23,7 @@
 #include <media/v4l2-ioctl.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-ctrls.h>
+#include <media/v4l2-event.h>
 #include <media/v4l2-dev.h>
 
 #define call_op(master, op) \
@@ -540,6 +541,44 @@ static bool type_is_int(const struct v4l2_ctrl *ctrl)
 	}
 }
 
+static void fill_event(struct v4l2_event *ev, struct v4l2_ctrl *ctrl, u32 changes)
+{
+	memset(ev->reserved, 0, sizeof(ev->reserved));
+	ev->type = V4L2_EVENT_CTRL;
+	ev->id = ctrl->id;
+	ev->u.ctrl.changes = changes;
+	ev->u.ctrl.type = ctrl->type;
+	ev->u.ctrl.flags = ctrl->flags;
+	if (ctrl->type == V4L2_CTRL_TYPE_STRING)
+		ev->u.ctrl.value64 = 0;
+	else
+		ev->u.ctrl.value64 = ctrl->cur.val64;
+	ev->u.ctrl.minimum = ctrl->minimum;
+	ev->u.ctrl.maximum = ctrl->maximum;
+	if (ctrl->type == V4L2_CTRL_TYPE_MENU)
+		ev->u.ctrl.step = 1;
+	else
+		ev->u.ctrl.step = ctrl->step;
+	ev->u.ctrl.default_value = ctrl->default_value;
+}
+
+static void send_event(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, u32 changes)
+{
+	struct v4l2_event ev;
+	struct v4l2_ctrl_fh *pos;
+
+	if (list_empty(&ctrl->fhs))
+			return;
+	fill_event(&ev, ctrl, changes);
+
+	list_for_each_entry(pos, &ctrl->fhs, node) {
+		if (pos->fh == fh)
+			continue;
+		ev.u.ctrl.flags = ctrl->flags;
+		v4l2_event_queue_fh(pos->fh, &ev);
+	}
+}
+
 /* Helper function: copy the current control value back to the caller */
 static int cur_to_user(struct v4l2_ext_control *c,
 		       struct v4l2_ctrl *ctrl)
@@ -629,20 +668,30 @@ static int new_to_user(struct v4l2_ext_control *c,
 /* Copy the new value to the current value. */
 static void new_to_cur(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl)
 {
+	bool changed = false;
+
 	if (ctrl == NULL)
 		return;
 	switch (ctrl->type) {
+	case V4L2_CTRL_TYPE_BUTTON:
+		changed = true;
+		break;
 	case V4L2_CTRL_TYPE_STRING:
 		/* strings are always 0-terminated */
+		changed = strcmp(ctrl->string, ctrl->cur.string);
 		strcpy(ctrl->cur.string, ctrl->string);
 		break;
 	case V4L2_CTRL_TYPE_INTEGER64:
+		changed = ctrl->val64 != ctrl->cur.val64;
 		ctrl->cur.val64 = ctrl->val64;
 		break;
 	default:
+		changed = ctrl->val != ctrl->cur.val;
 		ctrl->cur.val = ctrl->val;
 		break;
 	}
+	if (changed)
+		send_event(fh, ctrl, V4L2_EVENT_CTRL_CH_VALUE);
 }
 
 /* Copy the current value to the new value */
@@ -787,6 +836,7 @@ void v4l2_ctrl_handler_free(struct v4l2_ctrl_handler *hdl)
 {
 	struct v4l2_ctrl_ref *ref, *next_ref;
 	struct v4l2_ctrl *ctrl, *next_ctrl;
+	struct v4l2_ctrl_fh *ctrl_fh, *next_ctrl_fh;
 
 	if (hdl == NULL || hdl->buckets == NULL)
 		return;
@@ -800,6 +850,10 @@ void v4l2_ctrl_handler_free(struct v4l2_ctrl_handler *hdl)
 	/* Free all controls owned by the handler */
 	list_for_each_entry_safe(ctrl, next_ctrl, &hdl->ctrls, node) {
 		list_del(&ctrl->node);
+		list_for_each_entry_safe(ctrl_fh, next_ctrl_fh, &ctrl->fhs, node) {
+			list_del(&ctrl_fh->node);
+			kfree(ctrl_fh);
+		}
 		kfree(ctrl);
 	}
 	kfree(hdl->buckets);
@@ -1006,6 +1060,7 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
 	}
 
 	INIT_LIST_HEAD(&ctrl->node);
+	INIT_LIST_HEAD(&ctrl->fhs);
 	ctrl->handler = hdl;
 	ctrl->ops = ops;
 	ctrl->id = id;
@@ -1147,6 +1202,9 @@ int v4l2_ctrl_add_handler(struct v4l2_ctrl_handler *hdl,
 		/* Skip handler-private controls. */
 		if (ctrl->is_private)
 			continue;
+		/* And control classes */
+		if (ctrl->type == V4L2_CTRL_TYPE_CTRL_CLASS)
+			continue;
 		ret = handler_new_ref(hdl, ctrl);
 		if (ret)
 			break;
@@ -1175,9 +1233,14 @@ EXPORT_SYMBOL(v4l2_ctrl_cluster);
 
 void v4l2_ctrl_flags(struct v4l2_ctrl *ctrl, u32 clear_flags, u32 set_flags)
 {
+	u32 old_flags;
+
 	if (ctrl == NULL)
 		return;
+	old_flags = ctrl->flags;
 	ctrl->flags = (ctrl->flags & ~clear_flags) | set_flags;
+	if (old_flags != ctrl->flags)
+		send_event(NULL, ctrl, V4L2_EVENT_CTRL_CH_FLAGS);
 }
 EXPORT_SYMBOL(v4l2_ctrl_flags);
 
@@ -1898,3 +1961,41 @@ int v4l2_ctrl_s_ctrl(struct v4l2_ctrl *ctrl, s32 val)
 	return set_ctrl(NULL, ctrl, &val, false);
 }
 EXPORT_SYMBOL(v4l2_ctrl_s_ctrl);
+
+void v4l2_ctrl_add_fh(struct v4l2_ctrl_handler *hdl,
+		struct v4l2_ctrl_fh *ctrl_fh,
+		struct v4l2_event_subscription *sub)
+{
+	struct v4l2_ctrl *ctrl = v4l2_ctrl_find(hdl, sub->id);
+
+	if (ctrl->handler != hdl)
+		v4l2_ctrl_lock(ctrl);
+	list_add_tail(&ctrl_fh->node, &ctrl->fhs);
+	if (ctrl->type != V4L2_CTRL_TYPE_CTRL_CLASS &&
+	    (sub->flags & V4L2_EVENT_SUB_FL_SEND_INITIAL)) {
+		struct v4l2_event ev;
+
+		fill_event(&ev, ctrl, V4L2_EVENT_CTRL_CH_VALUE |
+			V4L2_EVENT_CTRL_CH_FLAGS);
+		v4l2_event_queue_fh(ctrl_fh->fh, &ev);
+	}
+	if (ctrl->handler != hdl)
+		v4l2_ctrl_unlock(ctrl);
+}
+EXPORT_SYMBOL(v4l2_ctrl_add_fh);
+
+void v4l2_ctrl_del_fh(struct v4l2_ctrl *ctrl, struct v4l2_fh *fh)
+{
+	struct v4l2_ctrl_fh *pos;
+
+	v4l2_ctrl_lock(ctrl);
+	list_for_each_entry(pos, &ctrl->fhs, node) {
+		if (pos->fh == fh) {
+			list_del(&pos->node);
+			kfree(pos);
+			break;
+		}
+	}
+	v4l2_ctrl_unlock(ctrl);
+}
+EXPORT_SYMBOL(v4l2_ctrl_del_fh);
diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
index 69fd343..5a4b8e0 100644
--- a/drivers/media/video/v4l2-event.c
+++ b/drivers/media/video/v4l2-event.c
@@ -25,10 +25,13 @@
 #include <media/v4l2-dev.h>
 #include <media/v4l2-fh.h>
 #include <media/v4l2-event.h>
+#include <media/v4l2-ctrls.h>
 
 #include <linux/sched.h>
 #include <linux/slab.h>
 
+static void v4l2_event_unsubscribe_all(struct v4l2_fh *fh);
+
 int v4l2_event_init(struct v4l2_fh *fh)
 {
 	fh->events = kzalloc(sizeof(*fh->events), GFP_KERNEL);
@@ -91,7 +94,7 @@ void v4l2_event_free(struct v4l2_fh *fh)
 
 	list_kfree(&events->free, struct v4l2_kevent, list);
 	list_kfree(&events->available, struct v4l2_kevent, list);
-	list_kfree(&events->subscribed, struct v4l2_subscribed_event, list);
+	v4l2_event_unsubscribe_all(fh);
 
 	kfree(events);
 	fh->events = NULL;
@@ -154,9 +157,9 @@ int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event,
 }
 EXPORT_SYMBOL_GPL(v4l2_event_dequeue);
 
-/* Caller must hold fh->event->lock! */
+/* Caller must hold fh->vdev->fh_lock! */
 static struct v4l2_subscribed_event *v4l2_event_subscribed(
-	struct v4l2_fh *fh, u32 type)
+		struct v4l2_fh *fh, u32 type, u32 id)
 {
 	struct v4l2_events *events = fh->events;
 	struct v4l2_subscribed_event *sev;
@@ -164,13 +167,46 @@ static struct v4l2_subscribed_event *v4l2_event_subscribed(
 	assert_spin_locked(&fh->vdev->fh_lock);
 
 	list_for_each_entry(sev, &events->subscribed, list) {
-		if (sev->type == type)
+		if (sev->type == type && sev->id == id)
 			return sev;
 	}
 
 	return NULL;
 }
 
+static void __v4l2_event_queue_fh(struct v4l2_fh *fh, const struct v4l2_event *ev,
+		const struct timespec *ts)
+{
+	struct v4l2_events *events = fh->events;
+	struct v4l2_subscribed_event *sev;
+	struct v4l2_kevent *kev;
+
+	/* Are we subscribed? */
+	sev = v4l2_event_subscribed(fh, ev->type, ev->id);
+	if (sev == NULL)
+		return;
+
+	/* Increase event sequence number on fh. */
+	events->sequence++;
+
+	/* Do we have any free events? */
+	if (list_empty(&events->free))
+		return;
+
+	/* Take one and fill it. */
+	kev = list_first_entry(&events->free, struct v4l2_kevent, list);
+	kev->event.type = ev->type;
+	kev->event.u = ev->u;
+	kev->event.id = ev->id;
+	kev->event.timestamp = *ts;
+	kev->event.sequence = events->sequence;
+	list_move_tail(&kev->list, &events->available);
+
+	events->navailable++;
+
+	wake_up_all(&events->wait);
+}
+
 void v4l2_event_queue(struct video_device *vdev, const struct v4l2_event *ev)
 {
 	struct v4l2_fh *fh;
@@ -182,37 +218,26 @@ void v4l2_event_queue(struct video_device *vdev, const struct v4l2_event *ev)
 	spin_lock_irqsave(&vdev->fh_lock, flags);
 
 	list_for_each_entry(fh, &vdev->fh_list, list) {
-		struct v4l2_events *events = fh->events;
-		struct v4l2_kevent *kev;
-
-		/* Are we subscribed? */
-		if (!v4l2_event_subscribed(fh, ev->type))
-			continue;
-
-		/* Increase event sequence number on fh. */
-		events->sequence++;
-
-		/* Do we have any free events? */
-		if (list_empty(&events->free))
-			continue;
-
-		/* Take one and fill it. */
-		kev = list_first_entry(&events->free, struct v4l2_kevent, list);
-		kev->event.type = ev->type;
-		kev->event.u = ev->u;
-		kev->event.timestamp = timestamp;
-		kev->event.sequence = events->sequence;
-		list_move_tail(&kev->list, &events->available);
-
-		events->navailable++;
-
-		wake_up_all(&events->wait);
+		__v4l2_event_queue_fh(fh, ev, &timestamp);
 	}
 
 	spin_unlock_irqrestore(&vdev->fh_lock, flags);
 }
 EXPORT_SYMBOL_GPL(v4l2_event_queue);
 
+void v4l2_event_queue_fh(struct v4l2_fh *fh, const struct v4l2_event *ev)
+{
+	unsigned long flags;
+	struct timespec timestamp;
+
+	ktime_get_ts(&timestamp);
+
+	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
+	__v4l2_event_queue_fh(fh, ev, &timestamp);
+	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
+}
+EXPORT_SYMBOL_GPL(v4l2_event_queue_fh);
+
 int v4l2_event_pending(struct v4l2_fh *fh)
 {
 	return fh->events->navailable;
@@ -223,7 +248,9 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
 			 struct v4l2_event_subscription *sub)
 {
 	struct v4l2_events *events = fh->events;
-	struct v4l2_subscribed_event *sev;
+	struct v4l2_subscribed_event *sev, *found_ev;
+	struct v4l2_ctrl *ctrl = NULL;
+	struct v4l2_ctrl_fh *ctrl_fh = NULL;
 	unsigned long flags;
 
 	if (fh->events == NULL) {
@@ -231,15 +258,31 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
 		return -ENOMEM;
 	}
 
+	if (sub->type == V4L2_EVENT_CTRL) {
+		ctrl = v4l2_ctrl_find(fh->ctrl_handler, sub->id);
+		if (ctrl == NULL)
+			return -EINVAL;
+	}
+
 	sev = kmalloc(sizeof(*sev), GFP_KERNEL);
 	if (!sev)
 		return -ENOMEM;
+	if (ctrl) {
+		ctrl_fh = kzalloc(sizeof(*ctrl_fh), GFP_KERNEL);
+		if (!ctrl_fh) {
+			kfree(sev);
+			return -ENOMEM;
+		}
+		ctrl_fh->fh = fh;
+	}
 
 	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
 
-	if (v4l2_event_subscribed(fh, sub->type) == NULL) {
+	found_ev = v4l2_event_subscribed(fh, sub->type, sub->id);
+	if (!found_ev) {
 		INIT_LIST_HEAD(&sev->list);
 		sev->type = sub->type;
+		sev->id = sub->id;
 
 		list_add(&sev->list, &events->subscribed);
 		sev = NULL;
@@ -247,6 +290,10 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
 
 	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
 
+	/* v4l2_ctrl_add_fh uses a mutex, so do this outside the spin lock */
+	if (!found_ev && ctrl)
+		v4l2_ctrl_add_fh(fh->ctrl_handler, ctrl_fh, sub);
+
 	kfree(sev);
 
 	return 0;
@@ -256,6 +303,7 @@ EXPORT_SYMBOL_GPL(v4l2_event_subscribe);
 static void v4l2_event_unsubscribe_all(struct v4l2_fh *fh)
 {
 	struct v4l2_events *events = fh->events;
+	struct v4l2_event_subscription sub;
 	struct v4l2_subscribed_event *sev;
 	unsigned long flags;
 
@@ -265,11 +313,13 @@ static void v4l2_event_unsubscribe_all(struct v4l2_fh *fh)
 		spin_lock_irqsave(&fh->vdev->fh_lock, flags);
 		if (!list_empty(&events->subscribed)) {
 			sev = list_first_entry(&events->subscribed,
-				       struct v4l2_subscribed_event, list);
-			list_del(&sev->list);
+					struct v4l2_subscribed_event, list);
+			sub.type = sev->type;
+			sub.id = sev->id;
 		}
 		spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
-		kfree(sev);
+		if (sev)
+			v4l2_event_unsubscribe(fh, &sub);
 	} while (sev);
 }
 
@@ -286,11 +336,17 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
 
 	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
 
-	sev = v4l2_event_subscribed(fh, sub->type);
+	sev = v4l2_event_subscribed(fh, sub->type, sub->id);
 	if (sev != NULL)
 		list_del(&sev->list);
 
 	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
+	if (sev->type == V4L2_EVENT_CTRL) {
+		struct v4l2_ctrl *ctrl = v4l2_ctrl_find(fh->ctrl_handler, sev->id);
+
+		if (ctrl)
+			v4l2_ctrl_del_fh(ctrl, fh);
+	}
 
 	kfree(sev);
 
diff --git a/drivers/media/video/v4l2-fh.c b/drivers/media/video/v4l2-fh.c
index 8635011..c6aef84 100644
--- a/drivers/media/video/v4l2-fh.c
+++ b/drivers/media/video/v4l2-fh.c
@@ -93,10 +93,8 @@ void v4l2_fh_exit(struct v4l2_fh *fh)
 {
 	if (fh->vdev == NULL)
 		return;
-
-	fh->vdev = NULL;
-
 	v4l2_event_free(fh);
+	fh->vdev = NULL;
 }
 EXPORT_SYMBOL_GPL(v4l2_fh_exit);
 
diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
index ac59361..ea4a0fb 100644
--- a/include/linux/videodev2.h
+++ b/include/linux/videodev2.h
@@ -1792,6 +1792,7 @@ struct v4l2_streamparm {
 #define V4L2_EVENT_ALL				0
 #define V4L2_EVENT_VSYNC			1
 #define V4L2_EVENT_EOS				2
+#define V4L2_EVENT_CTRL				3
 #define V4L2_EVENT_PRIVATE_START		0x08000000
 
 /* Payload for V4L2_EVENT_VSYNC */
@@ -1800,21 +1801,45 @@ struct v4l2_event_vsync {
 	__u8 field;
 } __attribute__ ((packed));
 
+/* Payload for V4L2_EVENT_CTRL */
+#define V4L2_EVENT_CTRL_CH_VALUE		(1 << 0)
+#define V4L2_EVENT_CTRL_CH_FLAGS		(1 << 1)
+
+struct v4l2_event_ctrl {
+	__u32 changes;
+	__u32 type;
+	union {
+		__s32 value;
+		__s64 value64;
+	};
+	__u32 flags;
+	__s32 minimum;
+	__s32 maximum;
+	__s32 step;
+	__s32 default_value;
+} __attribute__ ((packed));
+
 struct v4l2_event {
 	__u32				type;
 	union {
 		struct v4l2_event_vsync vsync;
+		struct v4l2_event_ctrl	ctrl;
 		__u8			data[64];
 	} u;
 	__u32				pending;
 	__u32				sequence;
 	struct timespec			timestamp;
-	__u32				reserved[9];
+	__u32				id;
+	__u32				reserved[8];
 };
 
+#define V4L2_EVENT_SUB_FL_SEND_INITIAL (1 << 0)
+
 struct v4l2_event_subscription {
 	__u32				type;
-	__u32				reserved[7];
+	__u32				id;
+	__u32				flags;
+	__u32				reserved[5];
 };
 
 /*
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index 151bd2e..2b99f29 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -28,9 +28,10 @@
 /* forward references */
 struct v4l2_ctrl_handler;
 struct v4l2_ctrl;
-struct v4l2_fh;
 struct video_device;
 struct v4l2_subdev;
+struct v4l2_event_subscription;
+struct v4l2_fh;
 
 /** struct v4l2_ctrl_ops - The control operations that the driver has to provide.
   * @g_volatile_ctrl: Get a new value for this control. Generally only relevant
@@ -98,6 +99,7 @@ struct v4l2_ctrl_ops {
 struct v4l2_ctrl {
 	/* Administrative fields */
 	struct list_head node;
+	struct list_head fhs;
 	struct v4l2_ctrl_handler *handler;
 	struct v4l2_ctrl **cluster;
 	unsigned ncontrols;
@@ -169,6 +171,11 @@ struct v4l2_ctrl_handler {
 	int error;
 };
 
+struct v4l2_ctrl_fh {
+	struct list_head node;
+	struct v4l2_fh *fh;
+};
+
 /** struct v4l2_ctrl_config - Control configuration structure.
   * @ops:	The control ops.
   * @id:	The control ID.
@@ -379,6 +386,7 @@ struct v4l2_ctrl *v4l2_ctrl_find(struct v4l2_ctrl_handler *hdl, u32 id);
   * @set_flags:	Set these flags.
   *
   * This clears and sets flags. Does nothing if @ctrl == NULL.
+  * The V4L2_EVENT_CTRL event will be generated afterwards.
   *
   * This function expects that the control handler is locked.
   */
@@ -390,6 +398,7 @@ void v4l2_ctrl_flags(struct v4l2_ctrl *ctrl, u32 clear_flags, u32 set_flags);
   * @set_flags:	Set these flags.
   *
   * This clears and sets flags. Does nothing if @ctrl == NULL.
+  * The V4L2_EVENT_CTRL event will be generated afterwards.
   *
   * This function expects that the control handler is unlocked and will lock
   * it before changing flags.
@@ -437,6 +446,11 @@ s32 v4l2_ctrl_g_ctrl(struct v4l2_ctrl *ctrl);
   */
 int v4l2_ctrl_s_ctrl(struct v4l2_ctrl *ctrl, s32 val);
 
+void v4l2_ctrl_add_fh(struct v4l2_ctrl_handler *hdl,
+		struct v4l2_ctrl_fh *ctrl_fh,
+		struct v4l2_event_subscription *sub);
+void v4l2_ctrl_del_fh(struct v4l2_ctrl *ctrl, struct v4l2_fh *fh);
+
 /* Helpers for ioctl_ops. If hdl == NULL then they will all return -EINVAL. */
 int v4l2_queryctrl(struct v4l2_ctrl_handler *hdl, struct v4l2_queryctrl *qc);
 int v4l2_querymenu(struct v4l2_ctrl_handler *hdl, struct v4l2_querymenu *qm);
diff --git a/include/media/v4l2-event.h b/include/media/v4l2-event.h
index 3b86177..45e9c1e 100644
--- a/include/media/v4l2-event.h
+++ b/include/media/v4l2-event.h
@@ -40,6 +40,7 @@ struct v4l2_kevent {
 struct v4l2_subscribed_event {
 	struct list_head	list;
 	u32			type;
+	u32			id;
 };
 
 struct v4l2_events {
@@ -58,6 +59,7 @@ void v4l2_event_free(struct v4l2_fh *fh);
 int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event,
 		       int nonblocking);
 void v4l2_event_queue(struct video_device *vdev, const struct v4l2_event *ev);
+void v4l2_event_queue_fh(struct v4l2_fh *fh, const struct v4l2_event *ev);
 int v4l2_event_pending(struct v4l2_fh *fh);
 int v4l2_event_subscribe(struct v4l2_fh *fh,
 			 struct v4l2_event_subscription *sub);
-- 
1.7.1

Re: [RFCv2 PATCH 07/11] v4l2-ctrls: add control events.

[Sakari Ailus]

Hi Hans,

On Wed, May 25, 2011 at 03:33:51PM +0200, Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Whenever a control changes value or state an event is sent to anyone
> that subscribed to it.
> 
> This functionality is useful for control panels but also for applications
> that need to wait for (usually status) controls to change value.

Thanks for the patch!

I agree that it's good to pass more information of the control (min, max
etc.) to the user space with the event. However, to support events arriving
from interrupt context which we've discussed in the past, such information
must be also accessible in those situations.

What do you think about more fine-grained locking of controls, say, spinlock
for each control (cluster) as an idea?

> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/media/video/v4l2-ctrls.c |  101 ++++++++++++++++++++++++++++++
>  drivers/media/video/v4l2-event.c |  126 +++++++++++++++++++++++++++-----------
>  drivers/media/video/v4l2-fh.c    |    4 +-
>  include/linux/videodev2.h        |   29 ++++++++-
>  include/media/v4l2-ctrls.h       |   16 +++++-
>  include/media/v4l2-event.h       |    2 +
>  6 files changed, 237 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/media/video/v4l2-ctrls.c b/drivers/media/video/v4l2-ctrls.c
> index 3e0d8ab..e2a7ac7 100644
> --- a/drivers/media/video/v4l2-ctrls.c
> +++ b/drivers/media/video/v4l2-ctrls.c
> @@ -23,6 +23,7 @@
>  #include <media/v4l2-ioctl.h>
>  #include <media/v4l2-device.h>
>  #include <media/v4l2-ctrls.h>
> +#include <media/v4l2-event.h>
>  #include <media/v4l2-dev.h>
>  
>  #define call_op(master, op) \
> @@ -540,6 +541,44 @@ static bool type_is_int(const struct v4l2_ctrl *ctrl)
>  	}
>  }
>  
> +static void fill_event(struct v4l2_event *ev, struct v4l2_ctrl *ctrl, u32 changes)
> +{
> +	memset(ev->reserved, 0, sizeof(ev->reserved));
> +	ev->type = V4L2_EVENT_CTRL;
> +	ev->id = ctrl->id;
> +	ev->u.ctrl.changes = changes;
> +	ev->u.ctrl.type = ctrl->type;
> +	ev->u.ctrl.flags = ctrl->flags;
> +	if (ctrl->type == V4L2_CTRL_TYPE_STRING)
> +		ev->u.ctrl.value64 = 0;
> +	else
> +		ev->u.ctrl.value64 = ctrl->cur.val64;
> +	ev->u.ctrl.minimum = ctrl->minimum;
> +	ev->u.ctrl.maximum = ctrl->maximum;
> +	if (ctrl->type == V4L2_CTRL_TYPE_MENU)
> +		ev->u.ctrl.step = 1;
> +	else
> +		ev->u.ctrl.step = ctrl->step;
> +	ev->u.ctrl.default_value = ctrl->default_value;
> +}
> +
> +static void send_event(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, u32 changes)
> +{
> +	struct v4l2_event ev;
> +	struct v4l2_ctrl_fh *pos;
> +
> +	if (list_empty(&ctrl->fhs))
> +			return;
> +	fill_event(&ev, ctrl, changes);
> +
> +	list_for_each_entry(pos, &ctrl->fhs, node) {
> +		if (pos->fh == fh)
> +			continue;
> +		ev.u.ctrl.flags = ctrl->flags;

What's the purpose of setting flags here as well? fill_event() above already
does it.

> +		v4l2_event_queue_fh(pos->fh, &ev);
> +	}
> +}
> +
>  /* Helper function: copy the current control value back to the caller */
>  static int cur_to_user(struct v4l2_ext_control *c,
>  		       struct v4l2_ctrl *ctrl)

Regards,

-- 
Sakari Ailus
sakari dot ailus at iki dot fi

Re: [RFCv2 PATCH 07/11] v4l2-ctrls: add control events.

[Hans Verkuil]

On Saturday, May 28, 2011 12:34:21 Sakari Ailus wrote:
> Hi Hans,
> 
> On Wed, May 25, 2011 at 03:33:51PM +0200, Hans Verkuil wrote:
> > From: Hans Verkuil <hans.verkuil@cisco.com>
> > 
> > Whenever a control changes value or state an event is sent to anyone
> > that subscribed to it.
> > 
> > This functionality is useful for control panels but also for applications
> > that need to wait for (usually status) controls to change value.
> 
> Thanks for the patch!
> 
> I agree that it's good to pass more information of the control (min, max
> etc.) to the user space with the event. However, to support events arriving
> from interrupt context which we've discussed in the past, such information
> must be also accessible in those situations.
> 
> What do you think about more fine-grained locking of controls, say, spinlock
> for each control (cluster) as an idea?

It's on my TODO list, but I need to think carefully on how to do it.
One thing at a time :-)

> 
> > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> > ---
> >  drivers/media/video/v4l2-ctrls.c |  101 ++++++++++++++++++++++++++++++
> >  drivers/media/video/v4l2-event.c |  126 +++++++++++++++++++++++++++-----------
> >  drivers/media/video/v4l2-fh.c    |    4 +-
> >  include/linux/videodev2.h        |   29 ++++++++-
> >  include/media/v4l2-ctrls.h       |   16 +++++-
> >  include/media/v4l2-event.h       |    2 +
> >  6 files changed, 237 insertions(+), 41 deletions(-)
> > 
> > diff --git a/drivers/media/video/v4l2-ctrls.c b/drivers/media/video/v4l2-ctrls.c
> > index 3e0d8ab..e2a7ac7 100644
> > --- a/drivers/media/video/v4l2-ctrls.c
> > +++ b/drivers/media/video/v4l2-ctrls.c
> > @@ -23,6 +23,7 @@
> >  #include <media/v4l2-ioctl.h>
> >  #include <media/v4l2-device.h>
> >  #include <media/v4l2-ctrls.h>
> > +#include <media/v4l2-event.h>
> >  #include <media/v4l2-dev.h>
> >  
> >  #define call_op(master, op) \
> > @@ -540,6 +541,44 @@ static bool type_is_int(const struct v4l2_ctrl *ctrl)
> >  	}
> >  }
> >  
> > +static void fill_event(struct v4l2_event *ev, struct v4l2_ctrl *ctrl, u32 changes)
> > +{
> > +	memset(ev->reserved, 0, sizeof(ev->reserved));
> > +	ev->type = V4L2_EVENT_CTRL;
> > +	ev->id = ctrl->id;
> > +	ev->u.ctrl.changes = changes;
> > +	ev->u.ctrl.type = ctrl->type;
> > +	ev->u.ctrl.flags = ctrl->flags;
> > +	if (ctrl->type == V4L2_CTRL_TYPE_STRING)
> > +		ev->u.ctrl.value64 = 0;
> > +	else
> > +		ev->u.ctrl.value64 = ctrl->cur.val64;
> > +	ev->u.ctrl.minimum = ctrl->minimum;
> > +	ev->u.ctrl.maximum = ctrl->maximum;
> > +	if (ctrl->type == V4L2_CTRL_TYPE_MENU)
> > +		ev->u.ctrl.step = 1;
> > +	else
> > +		ev->u.ctrl.step = ctrl->step;
> > +	ev->u.ctrl.default_value = ctrl->default_value;
> > +}
> > +
> > +static void send_event(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, u32 changes)
> > +{
> > +	struct v4l2_event ev;
> > +	struct v4l2_ctrl_fh *pos;
> > +
> > +	if (list_empty(&ctrl->fhs))
> > +			return;
> > +	fill_event(&ev, ctrl, changes);
> > +
> > +	list_for_each_entry(pos, &ctrl->fhs, node) {
> > +		if (pos->fh == fh)
> > +			continue;
> > +		ev.u.ctrl.flags = ctrl->flags;
> 
> What's the purpose of setting flags here as well? fill_event() above already
> does it.

Oops, that's a left-over from a previous version. I'll remove it.

Regards,

	Hans

> 
> > +		v4l2_event_queue_fh(pos->fh, &ev);
> > +	}
> > +}
> > +
> >  /* Helper function: copy the current control value back to the caller */
> >  static int cur_to_user(struct v4l2_ext_control *c,
> >  		       struct v4l2_ctrl *ctrl)
> 
> Regards,
> 
> 

Re: [RFCv2 PATCH 07/11] v4l2-ctrls: add control events.

[Sakari Ailus]

Hi Hans,

On Sat, May 28, 2011 at 04:58:20PM +0200, Hans Verkuil wrote:
> On Saturday, May 28, 2011 12:34:21 Sakari Ailus wrote:
> > Hi Hans,
> > 
> > On Wed, May 25, 2011 at 03:33:51PM +0200, Hans Verkuil wrote:
> > > From: Hans Verkuil <hans.verkuil@cisco.com>
> > > 
> > > Whenever a control changes value or state an event is sent to anyone
> > > that subscribed to it.
> > > 
> > > This functionality is useful for control panels but also for applications
> > > that need to wait for (usually status) controls to change value.
> > 
> > Thanks for the patch!
> > 
> > I agree that it's good to pass more information of the control (min, max
> > etc.) to the user space with the event. However, to support events arriving
> > from interrupt context which we've discussed in the past, such information
> > must be also accessible in those situations.
> > 
> > What do you think about more fine-grained locking of controls, say, spinlock
> > for each control (cluster) as an idea?
> 
> It's on my TODO list, but I need to think carefully on how to do it.
> One thing at a time :-)

I agree. I just wanted to hear your thoughts about this. :)

-- 
Sakari Ailus
sakari dot ailus at iki dot fi

Re: [RFCv2 PATCH 07/11] v4l2-ctrls: add control events.

[Sakari Ailus]

Hi Hans,

On Wed, May 25, 2011 at 03:33:51PM +0200, Hans Verkuil wrote:
> @@ -1800,21 +1801,45 @@ struct v4l2_event_vsync {
>  	__u8 field;
>  } __attribute__ ((packed));
>  
> +/* Payload for V4L2_EVENT_CTRL */
> +#define V4L2_EVENT_CTRL_CH_VALUE		(1 << 0)
> +#define V4L2_EVENT_CTRL_CH_FLAGS		(1 << 1)
> +
> +struct v4l2_event_ctrl {
> +	__u32 changes;
> +	__u32 type;
> +	union {
> +		__s32 value;
> +		__s64 value64;
> +	};
> +	__u32 flags;
> +	__s32 minimum;
> +	__s32 maximum;
> +	__s32 step;
> +	__s32 default_value;
> +} __attribute__ ((packed));
> +

One more comment.

Do we really need type and default_value in the event? They are static, and
on the other hand, the type should be already defined by the control so
that's static, as I'd expect default_value would be.

It just looks like this attempts to reimplement what QUERYCTRL does. :-)
Step, min and max values may change, so they are good.

More fields can be added later on. User space libraries / applications using
this structure might have different views of its size, though, depending
which definition they used at compile time. So in principle also this
structure should have reserved fields, although not having such and still
changing it might not have any adverse effects at all.

Btw. why __attribute__ ((packed))?

Regards,

-- 
Sakari Ailus
sakari dot ailus at iki dot fi

Re: [RFCv2 PATCH 07/11] v4l2-ctrls: add control events.

[Hans Verkuil]

On Saturday, May 28, 2011 12:48:45 Sakari Ailus wrote:
> Hi Hans,
> 
> On Wed, May 25, 2011 at 03:33:51PM +0200, Hans Verkuil wrote:
> > @@ -1800,21 +1801,45 @@ struct v4l2_event_vsync {
> >  	__u8 field;
> >  } __attribute__ ((packed));
> >  
> > +/* Payload for V4L2_EVENT_CTRL */
> > +#define V4L2_EVENT_CTRL_CH_VALUE		(1 << 0)
> > +#define V4L2_EVENT_CTRL_CH_FLAGS		(1 << 1)
> > +
> > +struct v4l2_event_ctrl {
> > +	__u32 changes;
> > +	__u32 type;
> > +	union {
> > +		__s32 value;
> > +		__s64 value64;
> > +	};
> > +	__u32 flags;
> > +	__s32 minimum;
> > +	__s32 maximum;
> > +	__s32 step;
> > +	__s32 default_value;
> > +} __attribute__ ((packed));
> > +
> 
> One more comment.
> 
> Do we really need type and default_value in the event? They are static, and
> on the other hand, the type should be already defined by the control so
> that's static, as I'd expect default_value would be.

We definitely want the type, since that's one of the first things that code
that handles the event will need (at least, in the case of a qv4l2-type app).
Not having to call queryctrl to get the type is actually quite handy.

If the range values of a control change, then the default_value will typically
also change. So I think it should be reported as well.

> It just looks like this attempts to reimplement what QUERYCTRL does. :-)

Up to a point, yes. I have thought about requiring apps to explicitly call
QUERYCTRL, but then you get race conditions between receiving the event and
calling QUERYCTRL/G_CTRL. To be honest, you still have that in the case of a
string control since you can't pass string values through the event API.

While you do have a few extra integer assignments, you also save unnecessary
ioctl calls in the applications, and those are a lot more expensive.

> Step, min and max values may change, so they are good.
> 
> More fields can be added later on. User space libraries / applications using
> this structure might have different views of its size, though, depending
> which definition they used at compile time. So in principle also this
> structure should have reserved fields, although not having such and still
> changing it might not have any adverse effects at all.
> 
> Btw. why __attribute__ ((packed))?

Prevents having to add code to compat32, particularly with the s64 in there.
It may not be needed, though, in this particular case. I'll have to test
alignment.

Regards,

	Hans

> 
> Regards,
> 
> 

Re: [RFCv2 PATCH 07/11] v4l2-ctrls: add control events.

[Sakari Ailus]

On Sat, May 28, 2011 at 05:08:30PM +0200, Hans Verkuil wrote:
> On Saturday, May 28, 2011 12:48:45 Sakari Ailus wrote:
[clip]
> > Do we really need type and default_value in the event? They are static, and
> > on the other hand, the type should be already defined by the control so
> > that's static, as I'd expect default_value would be.
> 
> We definitely want the type, since that's one of the first things that code
> that handles the event will need (at least, in the case of a qv4l2-type app).
> Not having to call queryctrl to get the type is actually quite handy.
> 
> If the range values of a control change, then the default_value will typically
> also change. So I think it should be reported as well.
> 
> > It just looks like this attempts to reimplement what QUERYCTRL does. :-)
> 
> Up to a point, yes. I have thought about requiring apps to explicitly call
> QUERYCTRL, but then you get race conditions between receiving the event and
> calling QUERYCTRL/G_CTRL. To be honest, you still have that in the case of a
> string control since you can't pass string values through the event API.
> 
> While you do have a few extra integer assignments, you also save unnecessary
> ioctl calls in the applications, and those are a lot more expensive.

Sound good to me.

> > Step, min and max values may change, so they are good.
> > 
> > More fields can be added later on. User space libraries / applications using
> > this structure might have different views of its size, though, depending
> > which definition they used at compile time. So in principle also this
> > structure should have reserved fields, although not having such and still
> > changing it might not have any adverse effects at all.
> > 
> > Btw. why __attribute__ ((packed))?
> 
> Prevents having to add code to compat32, particularly with the s64 in there.
> It may not be needed, though, in this particular case. I'll have to test
> alignment.

We don't have this in a number of user space API structures. I don't have
good access to the code right now but I think struct v4l2_event, for
example, doesn't use it. It would define the memory layout exactly so
perhaps it should be always used?

Cheers,

-- 
Sakari Ailus
sakari dot ailus at iki dot fi

[RFCv2 PATCH 08/11] v4l2-ctrls: simplify event subscription.

[Hans Verkuil]

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

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/video/v4l2-ctrls.c |   31 +++++++++++++++++++++++++++++++
 include/media/v4l2-ctrls.h       |   25 +++++++++++++++++++++++++
 2 files changed, 56 insertions(+), 0 deletions(-)

diff --git a/drivers/media/video/v4l2-ctrls.c b/drivers/media/video/v4l2-ctrls.c
index e2a7ac7..9807a20 100644
--- a/drivers/media/video/v4l2-ctrls.c
+++ b/drivers/media/video/v4l2-ctrls.c
@@ -831,6 +831,22 @@ int v4l2_ctrl_handler_init(struct v4l2_ctrl_handler *hdl,
 }
 EXPORT_SYMBOL(v4l2_ctrl_handler_init);
 
+/* Count the number of controls */
+unsigned v4l2_ctrl_handler_cnt(struct v4l2_ctrl_handler *hdl)
+{
+	struct v4l2_ctrl_ref *ref;
+	unsigned cnt = 0;
+
+	if (hdl == NULL)
+		return 0;
+	mutex_lock(&hdl->lock);
+	list_for_each_entry(ref, &hdl->ctrl_refs, node)
+		cnt++;
+	mutex_unlock(&hdl->lock);
+	return cnt;
+}
+EXPORT_SYMBOL(v4l2_ctrl_handler_cnt);
+
 /* Free all controls and control refs */
 void v4l2_ctrl_handler_free(struct v4l2_ctrl_handler *hdl)
 {
@@ -1999,3 +2015,18 @@ void v4l2_ctrl_del_fh(struct v4l2_ctrl *ctrl, struct v4l2_fh *fh)
 	v4l2_ctrl_unlock(ctrl);
 }
 EXPORT_SYMBOL(v4l2_ctrl_del_fh);
+
+int v4l2_ctrl_sub_fh(struct v4l2_fh *fh, struct v4l2_event_subscription *sub,
+		     unsigned n)
+{
+	int ret = 0;
+
+	if (!fh->events)
+		ret = v4l2_event_init(fh);
+	if (!ret)
+		ret = v4l2_event_alloc(fh, n);
+	if (!ret)
+		ret = v4l2_event_subscribe(fh, sub);
+	return ret;
+}
+EXPORT_SYMBOL(v4l2_ctrl_sub_fh);
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index 2b99f29..e2b9053 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -252,6 +252,14 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 int v4l2_ctrl_handler_init(struct v4l2_ctrl_handler *hdl,
 			   unsigned nr_of_controls_hint);
 
+/** v4l2_ctrl_handler_cnt() - Count the number of controls in the handler.
+  * @hdl:	The control handler.
+  *
+  * Returns the number of controls referenced by this handler.
+  * Returns 0 if @hdl == NULL.
+  */
+unsigned v4l2_ctrl_handler_cnt(struct v4l2_ctrl_handler *hdl);
+
 /** v4l2_ctrl_handler_free() - Free all controls owned by the handler and free
   * the control list.
   * @hdl:	The control handler.
@@ -446,11 +454,28 @@ s32 v4l2_ctrl_g_ctrl(struct v4l2_ctrl *ctrl);
   */
 int v4l2_ctrl_s_ctrl(struct v4l2_ctrl *ctrl, s32 val);
 
+/* Internal helper functions that deal with control events. */
 void v4l2_ctrl_add_fh(struct v4l2_ctrl_handler *hdl,
 		struct v4l2_ctrl_fh *ctrl_fh,
 		struct v4l2_event_subscription *sub);
 void v4l2_ctrl_del_fh(struct v4l2_ctrl *ctrl, struct v4l2_fh *fh);
 
+/** v4l2_ctrl_sub_fh() - Helper function that subscribes a control event.
+  * @fh:	The file handler that subscribed the control event.
+  * @sub:	The event to subscribe (type must be V4L2_EVENT_CTRL).
+  * @n:		How many events should be allocated? (Passed to v4l2_event_alloc).
+  *		Recommended to set to twice the number of controls plus whatever
+  *		is needed for other events.
+  *
+  * A helper function that initializes the fh for events, allocates the
+  * list of events and subscribes the control event.
+  *
+  * Typically called in the handler of VIDIOC_SUBSCRIBE_EVENT in the
+  * V4L2_EVENT_CTRL case.
+  */
+int v4l2_ctrl_sub_fh(struct v4l2_fh *fh, struct v4l2_event_subscription *sub,
+		     unsigned n);
+
 /* Helpers for ioctl_ops. If hdl == NULL then they will all return -EINVAL. */
 int v4l2_queryctrl(struct v4l2_ctrl_handler *hdl, struct v4l2_queryctrl *qc);
 int v4l2_querymenu(struct v4l2_ctrl_handler *hdl, struct v4l2_querymenu *qm);
-- 
1.7.1

Re: [RFCv2 PATCH 08/11] v4l2-ctrls: simplify event subscription.

[Laurent Pinchart]

Hi Hans,

Thanks for the patch.

On Wednesday 25 May 2011 15:33:52 Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/media/video/v4l2-ctrls.c |   31 +++++++++++++++++++++++++++++++
>  include/media/v4l2-ctrls.h       |   25 +++++++++++++++++++++++++
>  2 files changed, 56 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/media/video/v4l2-ctrls.c
> b/drivers/media/video/v4l2-ctrls.c index e2a7ac7..9807a20 100644
> --- a/drivers/media/video/v4l2-ctrls.c
> +++ b/drivers/media/video/v4l2-ctrls.c
> @@ -831,6 +831,22 @@ int v4l2_ctrl_handler_init(struct v4l2_ctrl_handler
> *hdl, }
>  EXPORT_SYMBOL(v4l2_ctrl_handler_init);
> 
> +/* Count the number of controls */
> +unsigned v4l2_ctrl_handler_cnt(struct v4l2_ctrl_handler *hdl)
> +{
> +	struct v4l2_ctrl_ref *ref;
> +	unsigned cnt = 0;
> +
> +	if (hdl == NULL)
> +		return 0;
> +	mutex_lock(&hdl->lock);
> +	list_for_each_entry(ref, &hdl->ctrl_refs, node)
> +		cnt++;

As you don't use the entry, you can replace list_for_each_entry with 
list_for_each.

Should the handler keep a controls count ? In that case you wouldn't need this 
function.

> +	mutex_unlock(&hdl->lock);
> +	return cnt;
> +}
> +EXPORT_SYMBOL(v4l2_ctrl_handler_cnt);
> +
>  /* Free all controls and control refs */
>  void v4l2_ctrl_handler_free(struct v4l2_ctrl_handler *hdl)
>  {
> @@ -1999,3 +2015,18 @@ void v4l2_ctrl_del_fh(struct v4l2_ctrl *ctrl, struct
> v4l2_fh *fh) v4l2_ctrl_unlock(ctrl);
>  }
>  EXPORT_SYMBOL(v4l2_ctrl_del_fh);
> +
> +int v4l2_ctrl_sub_fh(struct v4l2_fh *fh, struct v4l2_event_subscription
> *sub, +		     unsigned n)

I would rename this to v4l2_ctrl_subscribe_fh(). I had trouble understanding 
what v4l2_ctrl_sub_fh() before reading the documentation. sub makes me think 
about sub-devices and subtract, not subscription.

> +{
> +	int ret = 0;
> +
> +	if (!fh->events)
> +		ret = v4l2_event_init(fh);
> +	if (!ret)
> +		ret = v4l2_event_alloc(fh, n);
> +	if (!ret)
> +		ret = v4l2_event_subscribe(fh, sub);

I tend to return errors when they occur instead of continuing to the end of 
the function. Handling errors on the spot makes code easier to read in my 
opinion, as I expect the main code flow to be the error-free path.

> +	return ret;
> +}

-- 
Regards,

Laurent Pinchart

Re: [RFCv2 PATCH 08/11] v4l2-ctrls: simplify event subscription.

[Hans Verkuil]

On Friday, June 03, 2011 21:55:10 Laurent Pinchart wrote:
> Hi Hans,
> 
> Thanks for the patch.
> 
> On Wednesday 25 May 2011 15:33:52 Hans Verkuil wrote:
> > From: Hans Verkuil <hans.verkuil@cisco.com>
> > 
> > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> > ---
> >  drivers/media/video/v4l2-ctrls.c |   31 +++++++++++++++++++++++++++++++
> >  include/media/v4l2-ctrls.h       |   25 +++++++++++++++++++++++++
> >  2 files changed, 56 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/media/video/v4l2-ctrls.c
> > b/drivers/media/video/v4l2-ctrls.c index e2a7ac7..9807a20 100644
> > --- a/drivers/media/video/v4l2-ctrls.c
> > +++ b/drivers/media/video/v4l2-ctrls.c
> > @@ -831,6 +831,22 @@ int v4l2_ctrl_handler_init(struct v4l2_ctrl_handler
> > *hdl, }
> >  EXPORT_SYMBOL(v4l2_ctrl_handler_init);
> > 
> > +/* Count the number of controls */
> > +unsigned v4l2_ctrl_handler_cnt(struct v4l2_ctrl_handler *hdl)
> > +{
> > +	struct v4l2_ctrl_ref *ref;
> > +	unsigned cnt = 0;
> > +
> > +	if (hdl == NULL)
> > +		return 0;
> > +	mutex_lock(&hdl->lock);
> > +	list_for_each_entry(ref, &hdl->ctrl_refs, node)
> > +		cnt++;
> 
> As you don't use the entry, you can replace list_for_each_entry with 
> list_for_each.

True.

> Should the handler keep a controls count ? In that case you wouldn't need this 
> function.

I'll look into this.

> 
> > +	mutex_unlock(&hdl->lock);
> > +	return cnt;
> > +}
> > +EXPORT_SYMBOL(v4l2_ctrl_handler_cnt);
> > +
> >  /* Free all controls and control refs */
> >  void v4l2_ctrl_handler_free(struct v4l2_ctrl_handler *hdl)
> >  {
> > @@ -1999,3 +2015,18 @@ void v4l2_ctrl_del_fh(struct v4l2_ctrl *ctrl, struct
> > v4l2_fh *fh) v4l2_ctrl_unlock(ctrl);
> >  }
> >  EXPORT_SYMBOL(v4l2_ctrl_del_fh);
> > +
> > +int v4l2_ctrl_sub_fh(struct v4l2_fh *fh, struct v4l2_event_subscription
> > *sub, +		     unsigned n)
> 
> I would rename this to v4l2_ctrl_subscribe_fh(). I had trouble understanding 
> what v4l2_ctrl_sub_fh() before reading the documentation. sub makes me think 
> about sub-devices and subtract, not subscription.

Good point.

> > +{
> > +	int ret = 0;
> > +
> > +	if (!fh->events)
> > +		ret = v4l2_event_init(fh);
> > +	if (!ret)
> > +		ret = v4l2_event_alloc(fh, n);
> > +	if (!ret)
> > +		ret = v4l2_event_subscribe(fh, sub);
> 
> I tend to return errors when they occur instead of continuing to the end of 
> the function. Handling errors on the spot makes code easier to read in my 
> opinion, as I expect the main code flow to be the error-free path.

Hmmm, I rather like the way the code looks in this particular case. But it;s
no big deal and I can change it.

Regards,

	Hans

Re: [RFCv2 PATCH 08/11] v4l2-ctrls: simplify event subscription.

[Sakari Ailus]

On Sat, Jun 04, 2011 at 12:28:04PM +0200, Hans Verkuil wrote:
> On Friday, June 03, 2011 21:55:10 Laurent Pinchart wrote:
[clip]
> > > +{
> > > +	int ret = 0;
> > > +
> > > +	if (!fh->events)
> > > +		ret = v4l2_event_init(fh);
> > > +	if (!ret)
> > > +		ret = v4l2_event_alloc(fh, n);
> > > +	if (!ret)
> > > +		ret = v4l2_event_subscribe(fh, sub);
> > 
> > I tend to return errors when they occur instead of continuing to the end of 
> > the function. Handling errors on the spot makes code easier to read in my 
> > opinion, as I expect the main code flow to be the error-free path.
> 
> Hmmm, I rather like the way the code looks in this particular case. But it;s
> no big deal and I can change it.

The M5MOLS driver uses this pattern extensively in I2C access error
handling. I agree with Laurent in principle, but on the other hand I think
using this pattern makes sense. The error handling takes much less code and
the test for continuing always is "if (!ret)" it is relatively readable as
well.

I'm fine with either resolution.

Regards,

-- 
Sakari Ailus
sakari.ailus@iki.fi

Re: [RFCv2 PATCH 08/11] v4l2-ctrls: simplify event subscription.

[Kim, HeungJun]

2011-06-06 오후 7:05, Sakari Ailus 쓴 글:
> On Sat, Jun 04, 2011 at 12:28:04PM +0200, Hans Verkuil wrote:
>> On Friday, June 03, 2011 21:55:10 Laurent Pinchart wrote:
> [clip]
>>>> +{
>>>> +	int ret = 0;
>>>> +
>>>> +	if (!fh->events)
>>>> +		ret = v4l2_event_init(fh);
>>>> +	if (!ret)
>>>> +		ret = v4l2_event_alloc(fh, n);
>>>> +	if (!ret)
>>>> +		ret = v4l2_event_subscribe(fh, sub);
>>>
>>> I tend to return errors when they occur instead of continuing to the end of 
>>> the function. Handling errors on the spot makes code easier to read in my 
>>> opinion, as I expect the main code flow to be the error-free path.
>>
>> Hmmm, I rather like the way the code looks in this particular case. But it;s
>> no big deal and I can change it.
> 
> The M5MOLS driver uses this pattern extensively in I2C access error
> handling. I agree with Laurent in principle, but on the other hand I think
> using this pattern makes sense. The error handling takes much less code and
> the test for continuing always is "if (!ret)" it is relatively readable as
> well. 
> 
> I'm fine with either resolution.
Actually, this pattern was advice from Hans for the M-5MOLS reviews at past,
and it's very helpful and I satisfied mostly.

Also, I was testing or using both this usage and the other one for a while,
and as a result, I figured out which is good choice between continueing to the end
of the function and return imediately, depends on various situation.

Consequently, IMHO, both usgaes are fine, but if the checking factor like (!ret),
changes the other one like (!flag) not ret, then it looks better to return
immediately, or the other handling methods, not using continueing to check error
at the end.

Regards,
Heungjun Kim

[RFCv2 PATCH 09/11] vivi: support control events.

[Hans Verkuil]

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

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/video/vivi.c |  161 ++++++++++++++++++++++++++-----------------
 1 files changed, 97 insertions(+), 64 deletions(-)

diff --git a/drivers/media/video/vivi.c b/drivers/media/video/vivi.c
index 21d8f6a..93692ad 100644
--- a/drivers/media/video/vivi.c
+++ b/drivers/media/video/vivi.c
@@ -32,6 +32,7 @@
 #include <media/v4l2-ioctl.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-fh.h>
+#include <media/v4l2-event.h>
 #include <media/v4l2-common.h>
 
 #define VIVI_MODULE_NAME "vivi"
@@ -157,54 +158,6 @@ struct vivi_dmaqueue {
 
 static LIST_HEAD(vivi_devlist);
 
-struct vivi_dev {
-	struct list_head           vivi_devlist;
-	struct v4l2_device 	   v4l2_dev;
-	struct v4l2_ctrl_handler   ctrl_handler;
-
-	/* controls */
-	struct v4l2_ctrl	   *brightness;
-	struct v4l2_ctrl	   *contrast;
-	struct v4l2_ctrl	   *saturation;
-	struct v4l2_ctrl	   *hue;
-	struct v4l2_ctrl	   *volume;
-	struct v4l2_ctrl	   *button;
-	struct v4l2_ctrl	   *boolean;
-	struct v4l2_ctrl	   *int32;
-	struct v4l2_ctrl	   *int64;
-	struct v4l2_ctrl	   *menu;
-	struct v4l2_ctrl	   *string;
-	struct v4l2_ctrl	   *bitmask;
-
-	spinlock_t                 slock;
-	struct mutex		   mutex;
-
-	/* various device info */
-	struct video_device        *vfd;
-
-	struct vivi_dmaqueue       vidq;
-
-	/* Several counters */
-	unsigned 		   ms;
-	unsigned long              jiffies;
-	unsigned		   button_pressed;
-
-	int			   mv_count;	/* Controls bars movement */
-
-	/* Input Number */
-	int			   input;
-
-	/* video capture */
-	struct vivi_fmt            *fmt;
-	unsigned int               width, height;
-	struct vb2_queue	   vb_vidq;
-	enum v4l2_field		   field;
-	unsigned int		   field_count;
-
-	u8 			   bars[9][3];
-	u8 			   line[MAX_WIDTH * 4];
-};
-
 /* ------------------------------------------------------------------
 	DMA and thread functions
    ------------------------------------------------------------------*/
@@ -257,6 +210,50 @@ static struct bar_std bars[] = {
 
 #define NUM_INPUTS ARRAY_SIZE(bars)
 
+struct vivi_dev {
+	struct list_head           vivi_devlist;
+	struct v4l2_device	   v4l2_dev;
+	struct v4l2_ctrl_handler   ctrl_handler;
+
+	/* controls */
+	struct v4l2_ctrl	   *volume;
+	struct v4l2_ctrl	   *button;
+	struct v4l2_ctrl	   *boolean;
+	struct v4l2_ctrl	   *int32;
+	struct v4l2_ctrl	   *int64;
+	struct v4l2_ctrl	   *menu;
+	struct v4l2_ctrl	   *string;
+	struct v4l2_ctrl	   *bitmask;
+
+	spinlock_t                 slock;
+	struct mutex		   mutex;
+
+	/* various device info */
+	struct video_device        *vfd;
+
+	struct vivi_dmaqueue       vidq;
+
+	/* Several counters */
+	unsigned		   ms;
+	unsigned long              jiffies;
+	unsigned		   button_pressed;
+
+	int			   mv_count;	/* Controls bars movement */
+
+	/* Input Number */
+	int			   input;
+
+	/* video capture */
+	struct vivi_fmt            *fmt;
+	unsigned int               width, height;
+	struct vb2_queue	   vb_vidq;
+	enum v4l2_field		   field;
+	unsigned int		   field_count;
+
+	u8			   bars[9][3];
+	u8			   line[MAX_WIDTH * 4];
+};
+
 #define TO_Y(r, g, b) \
 	(((16829 * r + 33039 * g + 6416 * b  + 32768) >> 16) + 16)
 /* RGB to  V(Cr) Color transform */
@@ -451,6 +448,14 @@ static void gen_text(struct vivi_dev *dev, char *basep,
 
 static void vivi_fillbuff(struct vivi_dev *dev, struct vivi_buffer *buf)
 {
+	struct v4l2_ctrl *brightness = v4l2_ctrl_find(&dev->ctrl_handler,
+							V4L2_CID_BRIGHTNESS);
+	struct v4l2_ctrl *contrast = v4l2_ctrl_find(&dev->ctrl_handler,
+							V4L2_CID_CONTRAST);
+	struct v4l2_ctrl *saturation = v4l2_ctrl_find(&dev->ctrl_handler,
+							V4L2_CID_SATURATION);
+	struct v4l2_ctrl *hue = v4l2_ctrl_find(&dev->ctrl_handler,
+							V4L2_CID_HUE);
 	int wmax = dev->width;
 	int hmax = dev->height;
 	struct timeval ts;
@@ -482,10 +487,10 @@ static void vivi_fillbuff(struct vivi_dev *dev, struct vivi_buffer *buf)
 
 	mutex_lock(&dev->ctrl_handler.lock);
 	snprintf(str, sizeof(str), " brightness %3d, contrast %3d, saturation %3d, hue %d ",
-			dev->brightness->cur.val,
-			dev->contrast->cur.val,
-			dev->saturation->cur.val,
-			dev->hue->cur.val);
+			brightness->cur.val,
+			contrast->cur.val,
+			saturation->cur.val,
+			hue->cur.val);
 	gen_text(dev, vbuf, line++ * 16, 16, str);
 	snprintf(str, sizeof(str), " volume %3d ", dev->volume->cur.val);
 	gen_text(dev, vbuf, line++ * 16, 16, str);
@@ -977,12 +982,29 @@ static int vidioc_s_input(struct file *file, void *priv, unsigned int i)
 	if (i >= NUM_INPUTS)
 		return -EINVAL;
 
+	if (i == dev->input)
+		return 0;
+
 	dev->input = i;
 	precalculate_bars(dev);
 	precalculate_line(dev);
 	return 0;
 }
 
+static int vidioc_subscribe_event(struct v4l2_fh *fh,
+				struct v4l2_event_subscription *sub)
+{
+	int ret;
+
+	switch (sub->type) {
+	case V4L2_EVENT_CTRL:
+		return v4l2_ctrl_sub_fh(fh, sub,
+				v4l2_ctrl_handler_cnt(fh->ctrl_handler) * 2);
+	default:
+		return -EINVAL;
+	}
+}
+
 /* --- controls ---------------------------------------------- */
 
 static int vivi_s_ctrl(struct v4l2_ctrl *ctrl)
@@ -1012,10 +1034,17 @@ static unsigned int
 vivi_poll(struct file *file, struct poll_table_struct *wait)
 {
 	struct vivi_dev *dev = video_drvdata(file);
+	struct v4l2_fh *fh = file->private_data;
 	struct vb2_queue *q = &dev->vb_vidq;
+	unsigned int res;
 
 	dprintk(dev, 1, "%s\n", __func__);
-	return vb2_poll(q, file, wait);
+	res = vb2_poll(q, file, wait);
+	if (v4l2_event_pending(fh))
+		res |= POLLPRI;
+	else
+		poll_wait(file, &fh->events->wait, wait);
+	return res;
 }
 
 static int vivi_close(struct file *file)
@@ -1132,7 +1161,7 @@ static const struct v4l2_ctrl_config vivi_ctrl_bitmask = {
 
 static const struct v4l2_file_operations vivi_fops = {
 	.owner		= THIS_MODULE,
-	.open		= v4l2_fh_open,
+	.open           = v4l2_fh_open,
 	.release        = vivi_close,
 	.read           = vivi_read,
 	.poll		= vivi_poll,
@@ -1156,6 +1185,8 @@ static const struct v4l2_ioctl_ops vivi_ioctl_ops = {
 	.vidioc_s_input       = vidioc_s_input,
 	.vidioc_streamon      = vidioc_streamon,
 	.vidioc_streamoff     = vidioc_streamoff,
+	.vidioc_subscribe_event = vidioc_subscribe_event,
+	.vidioc_unsubscribe_event = v4l2_event_unsubscribe,
 };
 
 static struct video_device vivi_template = {
@@ -1200,6 +1231,7 @@ static int __init vivi_create_instance(int inst)
 	struct v4l2_ctrl_handler *hdl;
 	struct vb2_queue *q;
 	int ret;
+	int i;
 
 	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
 	if (!dev)
@@ -1214,18 +1246,19 @@ static int __init vivi_create_instance(int inst)
 	dev->fmt = &formats[0];
 	dev->width = 640;
 	dev->height = 480;
+
 	hdl = &dev->ctrl_handler;
-	v4l2_ctrl_handler_init(hdl, 11);
+	v4l2_ctrl_handler_init(hdl, 12);
+	v4l2_ctrl_new_std(hdl, &vivi_ctrl_ops,
+			V4L2_CID_BRIGHTNESS, i, 255, 1, 127 + i);
+	v4l2_ctrl_new_std(hdl, &vivi_ctrl_ops,
+			V4L2_CID_CONTRAST, i, 255, 1, 16 + i);
+	v4l2_ctrl_new_std(hdl, &vivi_ctrl_ops,
+			V4L2_CID_SATURATION, i, 255, 1, 127 + i);
+	v4l2_ctrl_new_std(hdl, &vivi_ctrl_ops,
+			V4L2_CID_HUE, -128 + i, 127, 1, i);
 	dev->volume = v4l2_ctrl_new_std(hdl, &vivi_ctrl_ops,
 			V4L2_CID_AUDIO_VOLUME, 0, 255, 1, 200);
-	dev->brightness = v4l2_ctrl_new_std(hdl, &vivi_ctrl_ops,
-			V4L2_CID_BRIGHTNESS, 0, 255, 1, 127);
-	dev->contrast = v4l2_ctrl_new_std(hdl, &vivi_ctrl_ops,
-			V4L2_CID_CONTRAST, 0, 255, 1, 16);
-	dev->saturation = v4l2_ctrl_new_std(hdl, &vivi_ctrl_ops,
-			V4L2_CID_SATURATION, 0, 255, 1, 127);
-	dev->hue = v4l2_ctrl_new_std(hdl, &vivi_ctrl_ops,
-			V4L2_CID_HUE, -128, 127, 1, 0);
 	dev->button = v4l2_ctrl_new_custom(hdl, &vivi_ctrl_button, NULL);
 	dev->int32 = v4l2_ctrl_new_custom(hdl, &vivi_ctrl_int32, NULL);
 	dev->int64 = v4l2_ctrl_new_custom(hdl, &vivi_ctrl_int64, NULL);
@@ -1296,7 +1329,7 @@ static int __init vivi_create_instance(int inst)
 rel_vdev:
 	video_device_release(vfd);
 unreg_dev:
-	v4l2_ctrl_handler_free(hdl);
+	v4l2_ctrl_handler_free(&dev->ctrl_handler);
 	v4l2_device_unregister(&dev->v4l2_dev);
 free_dev:
 	kfree(dev);
-- 
1.7.1

Re: [RFCv2 PATCH 09/11] vivi: support control events.

[Laurent Pinchart]

Hi Hans,

Thanks for the patch.

On Wednesday 25 May 2011 15:33:53 Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/media/video/vivi.c |  161
> ++++++++++++++++++++++++++----------------- 1 files changed, 97
> insertions(+), 64 deletions(-)
> 
> diff --git a/drivers/media/video/vivi.c b/drivers/media/video/vivi.c
> index 21d8f6a..93692ad 100644
> --- a/drivers/media/video/vivi.c
> +++ b/drivers/media/video/vivi.c
> @@ -32,6 +32,7 @@
>  #include <media/v4l2-ioctl.h>
>  #include <media/v4l2-ctrls.h>
>  #include <media/v4l2-fh.h>
> +#include <media/v4l2-event.h>
>  #include <media/v4l2-common.h>
> 
>  #define VIVI_MODULE_NAME "vivi"
> @@ -157,54 +158,6 @@ struct vivi_dmaqueue {
> 
>  static LIST_HEAD(vivi_devlist);
> 
> -struct vivi_dev {
> -	struct list_head           vivi_devlist;
> -	struct v4l2_device 	   v4l2_dev;
> -	struct v4l2_ctrl_handler   ctrl_handler;
> -
> -	/* controls */
> -	struct v4l2_ctrl	   *brightness;
> -	struct v4l2_ctrl	   *contrast;
> -	struct v4l2_ctrl	   *saturation;
> -	struct v4l2_ctrl	   *hue;

What's the reason for removing the brigthness, contrast, saturation and huer 
controls from the vivi_dev structure ? You then need to call v4l2_ctrl_find() 
several times per frame.

> -	struct v4l2_ctrl	   *volume;
> -	struct v4l2_ctrl	   *button;
> -	struct v4l2_ctrl	   *boolean;
> -	struct v4l2_ctrl	   *int32;
> -	struct v4l2_ctrl	   *int64;
> -	struct v4l2_ctrl	   *menu;
> -	struct v4l2_ctrl	   *string;
> -	struct v4l2_ctrl	   *bitmask;
> -
> -	spinlock_t                 slock;
> -	struct mutex		   mutex;
> -
> -	/* various device info */
> -	struct video_device        *vfd;
> -
> -	struct vivi_dmaqueue       vidq;
> -
> -	/* Several counters */
> -	unsigned 		   ms;
> -	unsigned long              jiffies;
> -	unsigned		   button_pressed;
> -
> -	int			   mv_count;	/* Controls bars movement */
> -
> -	/* Input Number */
> -	int			   input;
> -
> -	/* video capture */
> -	struct vivi_fmt            *fmt;
> -	unsigned int               width, height;
> -	struct vb2_queue	   vb_vidq;
> -	enum v4l2_field		   field;
> -	unsigned int		   field_count;
> -
> -	u8 			   bars[9][3];
> -	u8 			   line[MAX_WIDTH * 4];
> -};
> -
>  /* ------------------------------------------------------------------
>  	DMA and thread functions
>     ------------------------------------------------------------------*/
> @@ -257,6 +210,50 @@ static struct bar_std bars[] = {
> 
>  #define NUM_INPUTS ARRAY_SIZE(bars)
> 
> +struct vivi_dev {
> +	struct list_head           vivi_devlist;
> +	struct v4l2_device	   v4l2_dev;
> +	struct v4l2_ctrl_handler   ctrl_handler;
> +
> +	/* controls */
> +	struct v4l2_ctrl	   *volume;
> +	struct v4l2_ctrl	   *button;
> +	struct v4l2_ctrl	   *boolean;
> +	struct v4l2_ctrl	   *int32;
> +	struct v4l2_ctrl	   *int64;
> +	struct v4l2_ctrl	   *menu;
> +	struct v4l2_ctrl	   *string;
> +	struct v4l2_ctrl	   *bitmask;
> +
> +	spinlock_t                 slock;
> +	struct mutex		   mutex;
> +
> +	/* various device info */
> +	struct video_device        *vfd;
> +
> +	struct vivi_dmaqueue       vidq;
> +
> +	/* Several counters */
> +	unsigned		   ms;
> +	unsigned long              jiffies;
> +	unsigned		   button_pressed;
> +
> +	int			   mv_count;	/* Controls bars movement */
> +
> +	/* Input Number */
> +	int			   input;
> +
> +	/* video capture */
> +	struct vivi_fmt            *fmt;
> +	unsigned int               width, height;
> +	struct vb2_queue	   vb_vidq;
> +	enum v4l2_field		   field;
> +	unsigned int		   field_count;
> +
> +	u8			   bars[9][3];
> +	u8			   line[MAX_WIDTH * 4];
> +};
> +
>  #define TO_Y(r, g, b) \
>  	(((16829 * r + 33039 * g + 6416 * b  + 32768) >> 16) + 16)
>  /* RGB to  V(Cr) Color transform */
> @@ -451,6 +448,14 @@ static void gen_text(struct vivi_dev *dev, char
> *basep,
> 
>  static void vivi_fillbuff(struct vivi_dev *dev, struct vivi_buffer *buf)
>  {
> +	struct v4l2_ctrl *brightness = v4l2_ctrl_find(&dev->ctrl_handler,
> +							V4L2_CID_BRIGHTNESS);
> +	struct v4l2_ctrl *contrast = v4l2_ctrl_find(&dev->ctrl_handler,
> +							V4L2_CID_CONTRAST);
> +	struct v4l2_ctrl *saturation = v4l2_ctrl_find(&dev->ctrl_handler,
> +							V4L2_CID_SATURATION);
> +	struct v4l2_ctrl *hue = v4l2_ctrl_find(&dev->ctrl_handler,
> +							V4L2_CID_HUE);
>  	int wmax = dev->width;
>  	int hmax = dev->height;
>  	struct timeval ts;
> @@ -482,10 +487,10 @@ static void vivi_fillbuff(struct vivi_dev *dev,
> struct vivi_buffer *buf)
> 
>  	mutex_lock(&dev->ctrl_handler.lock);
>  	snprintf(str, sizeof(str), " brightness %3d, contrast %3d, saturation
> %3d, hue %d ", -			dev->brightness->cur.val,
> -			dev->contrast->cur.val,
> -			dev->saturation->cur.val,
> -			dev->hue->cur.val);
> +			brightness->cur.val,
> +			contrast->cur.val,
> +			saturation->cur.val,
> +			hue->cur.val);
>  	gen_text(dev, vbuf, line++ * 16, 16, str);
>  	snprintf(str, sizeof(str), " volume %3d ", dev->volume->cur.val);
>  	gen_text(dev, vbuf, line++ * 16, 16, str);
> @@ -977,12 +982,29 @@ static int vidioc_s_input(struct file *file, void
> *priv, unsigned int i) if (i >= NUM_INPUTS)
>  		return -EINVAL;
> 
> +	if (i == dev->input)
> +		return 0;
> +
>  	dev->input = i;
>  	precalculate_bars(dev);
>  	precalculate_line(dev);
>  	return 0;
>  }
> 
> +static int vidioc_subscribe_event(struct v4l2_fh *fh,
> +				struct v4l2_event_subscription *sub)
> +{
> +	int ret;
> +
> +	switch (sub->type) {
> +	case V4L2_EVENT_CTRL:
> +		return v4l2_ctrl_sub_fh(fh, sub,
> +				v4l2_ctrl_handler_cnt(fh->ctrl_handler) * 2);
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
>  /* --- controls ---------------------------------------------- */
> 
>  static int vivi_s_ctrl(struct v4l2_ctrl *ctrl)
> @@ -1012,10 +1034,17 @@ static unsigned int
>  vivi_poll(struct file *file, struct poll_table_struct *wait)
>  {
>  	struct vivi_dev *dev = video_drvdata(file);
> +	struct v4l2_fh *fh = file->private_data;
>  	struct vb2_queue *q = &dev->vb_vidq;
> +	unsigned int res;

The rest of the driver seems to use ret instead of res.

> 
>  	dprintk(dev, 1, "%s\n", __func__);
> -	return vb2_poll(q, file, wait);
> +	res = vb2_poll(q, file, wait);
> +	if (v4l2_event_pending(fh))
> +		res |= POLLPRI;
> +	else
> +		poll_wait(file, &fh->events->wait, wait);
> +	return res;
>  }
> 
>  static int vivi_close(struct file *file)
> @@ -1132,7 +1161,7 @@ static const struct v4l2_ctrl_config
> vivi_ctrl_bitmask = {
> 
>  static const struct v4l2_file_operations vivi_fops = {
>  	.owner		= THIS_MODULE,
> -	.open		= v4l2_fh_open,
> +	.open           = v4l2_fh_open,
>  	.release        = vivi_close,
>  	.read           = vivi_read,
>  	.poll		= vivi_poll,
> @@ -1156,6 +1185,8 @@ static const struct v4l2_ioctl_ops vivi_ioctl_ops = {
>  	.vidioc_s_input       = vidioc_s_input,
>  	.vidioc_streamon      = vidioc_streamon,
>  	.vidioc_streamoff     = vidioc_streamoff,
> +	.vidioc_subscribe_event = vidioc_subscribe_event,
> +	.vidioc_unsubscribe_event = v4l2_event_unsubscribe,
>  };
> 
>  static struct video_device vivi_template = {
> @@ -1200,6 +1231,7 @@ static int __init vivi_create_instance(int inst)
>  	struct v4l2_ctrl_handler *hdl;
>  	struct vb2_queue *q;
>  	int ret;
> +	int i;
> 
>  	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
>  	if (!dev)
> @@ -1214,18 +1246,19 @@ static int __init vivi_create_instance(int inst)
>  	dev->fmt = &formats[0];
>  	dev->width = 640;
>  	dev->height = 480;
> +
>  	hdl = &dev->ctrl_handler;
> -	v4l2_ctrl_handler_init(hdl, 11);
> +	v4l2_ctrl_handler_init(hdl, 12);
> +	v4l2_ctrl_new_std(hdl, &vivi_ctrl_ops,
> +			V4L2_CID_BRIGHTNESS, i, 255, 1, 127 + i);

Isn't i used uninitialized ?

> +	v4l2_ctrl_new_std(hdl, &vivi_ctrl_ops,
> +			V4L2_CID_CONTRAST, i, 255, 1, 16 + i);
> +	v4l2_ctrl_new_std(hdl, &vivi_ctrl_ops,
> +			V4L2_CID_SATURATION, i, 255, 1, 127 + i);
> +	v4l2_ctrl_new_std(hdl, &vivi_ctrl_ops,
> +			V4L2_CID_HUE, -128 + i, 127, 1, i);
>  	dev->volume = v4l2_ctrl_new_std(hdl, &vivi_ctrl_ops,
>  			V4L2_CID_AUDIO_VOLUME, 0, 255, 1, 200);
> -	dev->brightness = v4l2_ctrl_new_std(hdl, &vivi_ctrl_ops,
> -			V4L2_CID_BRIGHTNESS, 0, 255, 1, 127);
> -	dev->contrast = v4l2_ctrl_new_std(hdl, &vivi_ctrl_ops,
> -			V4L2_CID_CONTRAST, 0, 255, 1, 16);
> -	dev->saturation = v4l2_ctrl_new_std(hdl, &vivi_ctrl_ops,
> -			V4L2_CID_SATURATION, 0, 255, 1, 127);
> -	dev->hue = v4l2_ctrl_new_std(hdl, &vivi_ctrl_ops,
> -			V4L2_CID_HUE, -128, 127, 1, 0);
>  	dev->button = v4l2_ctrl_new_custom(hdl, &vivi_ctrl_button, NULL);
>  	dev->int32 = v4l2_ctrl_new_custom(hdl, &vivi_ctrl_int32, NULL);
>  	dev->int64 = v4l2_ctrl_new_custom(hdl, &vivi_ctrl_int64, NULL);
> @@ -1296,7 +1329,7 @@ static int __init vivi_create_instance(int inst)
>  rel_vdev:
>  	video_device_release(vfd);
>  unreg_dev:
> -	v4l2_ctrl_handler_free(hdl);
> +	v4l2_ctrl_handler_free(&dev->ctrl_handler);

What's the reason for this change ?

>  	v4l2_device_unregister(&dev->v4l2_dev);
>  free_dev:
>  	kfree(dev);

-- 
Regards,

Laurent Pinchart

Re: [RFCv2 PATCH 09/11] vivi: support control events.

[Hans Verkuil]

On Friday, June 03, 2011 21:55:04 Laurent Pinchart wrote:
> Hi Hans,
> 
> Thanks for the patch.

Thanks for the comments. I need to revisit this patch. Everything you found are
remains of an earlier version of this patch. I thought I reverted all those
changes, but clearly I missed several.

Regards,

	Hans

> 
> On Wednesday 25 May 2011 15:33:53 Hans Verkuil wrote:
> > From: Hans Verkuil <hans.verkuil@cisco.com>
> > 
> > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> > ---
> >  drivers/media/video/vivi.c |  161
> > ++++++++++++++++++++++++++----------------- 1 files changed, 97
> > insertions(+), 64 deletions(-)
> > 
> > diff --git a/drivers/media/video/vivi.c b/drivers/media/video/vivi.c
> > index 21d8f6a..93692ad 100644
> > --- a/drivers/media/video/vivi.c
> > +++ b/drivers/media/video/vivi.c
> > @@ -32,6 +32,7 @@
> >  #include <media/v4l2-ioctl.h>
> >  #include <media/v4l2-ctrls.h>
> >  #include <media/v4l2-fh.h>
> > +#include <media/v4l2-event.h>
> >  #include <media/v4l2-common.h>
> > 
> >  #define VIVI_MODULE_NAME "vivi"
> > @@ -157,54 +158,6 @@ struct vivi_dmaqueue {
> > 
> >  static LIST_HEAD(vivi_devlist);
> > 
> > -struct vivi_dev {
> > -	struct list_head           vivi_devlist;
> > -	struct v4l2_device 	   v4l2_dev;
> > -	struct v4l2_ctrl_handler   ctrl_handler;
> > -
> > -	/* controls */
> > -	struct v4l2_ctrl	   *brightness;
> > -	struct v4l2_ctrl	   *contrast;
> > -	struct v4l2_ctrl	   *saturation;
> > -	struct v4l2_ctrl	   *hue;
> 
> What's the reason for removing the brigthness, contrast, saturation and huer 
> controls from the vivi_dev structure ? You then need to call v4l2_ctrl_find() 
> several times per frame.
> 
> > -	struct v4l2_ctrl	   *volume;
> > -	struct v4l2_ctrl	   *button;
> > -	struct v4l2_ctrl	   *boolean;
> > -	struct v4l2_ctrl	   *int32;
> > -	struct v4l2_ctrl	   *int64;
> > -	struct v4l2_ctrl	   *menu;
> > -	struct v4l2_ctrl	   *string;
> > -	struct v4l2_ctrl	   *bitmask;
> > -
> > -	spinlock_t                 slock;
> > -	struct mutex		   mutex;
> > -
> > -	/* various device info */
> > -	struct video_device        *vfd;
> > -
> > -	struct vivi_dmaqueue       vidq;
> > -
> > -	/* Several counters */
> > -	unsigned 		   ms;
> > -	unsigned long              jiffies;
> > -	unsigned		   button_pressed;
> > -
> > -	int			   mv_count;	/* Controls bars movement */
> > -
> > -	/* Input Number */
> > -	int			   input;
> > -
> > -	/* video capture */
> > -	struct vivi_fmt            *fmt;
> > -	unsigned int               width, height;
> > -	struct vb2_queue	   vb_vidq;
> > -	enum v4l2_field		   field;
> > -	unsigned int		   field_count;
> > -
> > -	u8 			   bars[9][3];
> > -	u8 			   line[MAX_WIDTH * 4];
> > -};
> > -
> >  /* ------------------------------------------------------------------
> >  	DMA and thread functions
> >     ------------------------------------------------------------------*/
> > @@ -257,6 +210,50 @@ static struct bar_std bars[] = {
> > 
> >  #define NUM_INPUTS ARRAY_SIZE(bars)
> > 
> > +struct vivi_dev {
> > +	struct list_head           vivi_devlist;
> > +	struct v4l2_device	   v4l2_dev;
> > +	struct v4l2_ctrl_handler   ctrl_handler;
> > +
> > +	/* controls */
> > +	struct v4l2_ctrl	   *volume;
> > +	struct v4l2_ctrl	   *button;
> > +	struct v4l2_ctrl	   *boolean;
> > +	struct v4l2_ctrl	   *int32;
> > +	struct v4l2_ctrl	   *int64;
> > +	struct v4l2_ctrl	   *menu;
> > +	struct v4l2_ctrl	   *string;
> > +	struct v4l2_ctrl	   *bitmask;
> > +
> > +	spinlock_t                 slock;
> > +	struct mutex		   mutex;
> > +
> > +	/* various device info */
> > +	struct video_device        *vfd;
> > +
> > +	struct vivi_dmaqueue       vidq;
> > +
> > +	/* Several counters */
> > +	unsigned		   ms;
> > +	unsigned long              jiffies;
> > +	unsigned		   button_pressed;
> > +
> > +	int			   mv_count;	/* Controls bars movement */
> > +
> > +	/* Input Number */
> > +	int			   input;
> > +
> > +	/* video capture */
> > +	struct vivi_fmt            *fmt;
> > +	unsigned int               width, height;
> > +	struct vb2_queue	   vb_vidq;
> > +	enum v4l2_field		   field;
> > +	unsigned int		   field_count;
> > +
> > +	u8			   bars[9][3];
> > +	u8			   line[MAX_WIDTH * 4];
> > +};
> > +
> >  #define TO_Y(r, g, b) \
> >  	(((16829 * r + 33039 * g + 6416 * b  + 32768) >> 16) + 16)
> >  /* RGB to  V(Cr) Color transform */
> > @@ -451,6 +448,14 @@ static void gen_text(struct vivi_dev *dev, char
> > *basep,
> > 
> >  static void vivi_fillbuff(struct vivi_dev *dev, struct vivi_buffer *buf)
> >  {
> > +	struct v4l2_ctrl *brightness = v4l2_ctrl_find(&dev->ctrl_handler,
> > +							V4L2_CID_BRIGHTNESS);
> > +	struct v4l2_ctrl *contrast = v4l2_ctrl_find(&dev->ctrl_handler,
> > +							V4L2_CID_CONTRAST);
> > +	struct v4l2_ctrl *saturation = v4l2_ctrl_find(&dev->ctrl_handler,
> > +							V4L2_CID_SATURATION);
> > +	struct v4l2_ctrl *hue = v4l2_ctrl_find(&dev->ctrl_handler,
> > +							V4L2_CID_HUE);
> >  	int wmax = dev->width;
> >  	int hmax = dev->height;
> >  	struct timeval ts;
> > @@ -482,10 +487,10 @@ static void vivi_fillbuff(struct vivi_dev *dev,
> > struct vivi_buffer *buf)
> > 
> >  	mutex_lock(&dev->ctrl_handler.lock);
> >  	snprintf(str, sizeof(str), " brightness %3d, contrast %3d, saturation
> > %3d, hue %d ", -			dev->brightness->cur.val,
> > -			dev->contrast->cur.val,
> > -			dev->saturation->cur.val,
> > -			dev->hue->cur.val);
> > +			brightness->cur.val,
> > +			contrast->cur.val,
> > +			saturation->cur.val,
> > +			hue->cur.val);
> >  	gen_text(dev, vbuf, line++ * 16, 16, str);
> >  	snprintf(str, sizeof(str), " volume %3d ", dev->volume->cur.val);
> >  	gen_text(dev, vbuf, line++ * 16, 16, str);
> > @@ -977,12 +982,29 @@ static int vidioc_s_input(struct file *file, void
> > *priv, unsigned int i) if (i >= NUM_INPUTS)
> >  		return -EINVAL;
> > 
> > +	if (i == dev->input)
> > +		return 0;
> > +
> >  	dev->input = i;
> >  	precalculate_bars(dev);
> >  	precalculate_line(dev);
> >  	return 0;
> >  }
> > 
> > +static int vidioc_subscribe_event(struct v4l2_fh *fh,
> > +				struct v4l2_event_subscription *sub)
> > +{
> > +	int ret;
> > +
> > +	switch (sub->type) {
> > +	case V4L2_EVENT_CTRL:
> > +		return v4l2_ctrl_sub_fh(fh, sub,
> > +				v4l2_ctrl_handler_cnt(fh->ctrl_handler) * 2);
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> >  /* --- controls ---------------------------------------------- */
> > 
> >  static int vivi_s_ctrl(struct v4l2_ctrl *ctrl)
> > @@ -1012,10 +1034,17 @@ static unsigned int
> >  vivi_poll(struct file *file, struct poll_table_struct *wait)
> >  {
> >  	struct vivi_dev *dev = video_drvdata(file);
> > +	struct v4l2_fh *fh = file->private_data;
> >  	struct vb2_queue *q = &dev->vb_vidq;
> > +	unsigned int res;
> 
> The rest of the driver seems to use ret instead of res.
> 
> > 
> >  	dprintk(dev, 1, "%s\n", __func__);
> > -	return vb2_poll(q, file, wait);
> > +	res = vb2_poll(q, file, wait);
> > +	if (v4l2_event_pending(fh))
> > +		res |= POLLPRI;
> > +	else
> > +		poll_wait(file, &fh->events->wait, wait);
> > +	return res;
> >  }
> > 
> >  static int vivi_close(struct file *file)
> > @@ -1132,7 +1161,7 @@ static const struct v4l2_ctrl_config
> > vivi_ctrl_bitmask = {
> > 
> >  static const struct v4l2_file_operations vivi_fops = {
> >  	.owner		= THIS_MODULE,
> > -	.open		= v4l2_fh_open,
> > +	.open           = v4l2_fh_open,
> >  	.release        = vivi_close,
> >  	.read           = vivi_read,
> >  	.poll		= vivi_poll,
> > @@ -1156,6 +1185,8 @@ static const struct v4l2_ioctl_ops vivi_ioctl_ops = {
> >  	.vidioc_s_input       = vidioc_s_input,
> >  	.vidioc_streamon      = vidioc_streamon,
> >  	.vidioc_streamoff     = vidioc_streamoff,
> > +	.vidioc_subscribe_event = vidioc_subscribe_event,
> > +	.vidioc_unsubscribe_event = v4l2_event_unsubscribe,
> >  };
> > 
> >  static struct video_device vivi_template = {
> > @@ -1200,6 +1231,7 @@ static int __init vivi_create_instance(int inst)
> >  	struct v4l2_ctrl_handler *hdl;
> >  	struct vb2_queue *q;
> >  	int ret;
> > +	int i;
> > 
> >  	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> >  	if (!dev)
> > @@ -1214,18 +1246,19 @@ static int __init vivi_create_instance(int inst)
> >  	dev->fmt = &formats[0];
> >  	dev->width = 640;
> >  	dev->height = 480;
> > +
> >  	hdl = &dev->ctrl_handler;
> > -	v4l2_ctrl_handler_init(hdl, 11);
> > +	v4l2_ctrl_handler_init(hdl, 12);
> > +	v4l2_ctrl_new_std(hdl, &vivi_ctrl_ops,
> > +			V4L2_CID_BRIGHTNESS, i, 255, 1, 127 + i);
> 
> Isn't i used uninitialized ?
> 
> > +	v4l2_ctrl_new_std(hdl, &vivi_ctrl_ops,
> > +			V4L2_CID_CONTRAST, i, 255, 1, 16 + i);
> > +	v4l2_ctrl_new_std(hdl, &vivi_ctrl_ops,
> > +			V4L2_CID_SATURATION, i, 255, 1, 127 + i);
> > +	v4l2_ctrl_new_std(hdl, &vivi_ctrl_ops,
> > +			V4L2_CID_HUE, -128 + i, 127, 1, i);
> >  	dev->volume = v4l2_ctrl_new_std(hdl, &vivi_ctrl_ops,
> >  			V4L2_CID_AUDIO_VOLUME, 0, 255, 1, 200);
> > -	dev->brightness = v4l2_ctrl_new_std(hdl, &vivi_ctrl_ops,
> > -			V4L2_CID_BRIGHTNESS, 0, 255, 1, 127);
> > -	dev->contrast = v4l2_ctrl_new_std(hdl, &vivi_ctrl_ops,
> > -			V4L2_CID_CONTRAST, 0, 255, 1, 16);
> > -	dev->saturation = v4l2_ctrl_new_std(hdl, &vivi_ctrl_ops,
> > -			V4L2_CID_SATURATION, 0, 255, 1, 127);
> > -	dev->hue = v4l2_ctrl_new_std(hdl, &vivi_ctrl_ops,
> > -			V4L2_CID_HUE, -128, 127, 1, 0);
> >  	dev->button = v4l2_ctrl_new_custom(hdl, &vivi_ctrl_button, NULL);
> >  	dev->int32 = v4l2_ctrl_new_custom(hdl, &vivi_ctrl_int32, NULL);
> >  	dev->int64 = v4l2_ctrl_new_custom(hdl, &vivi_ctrl_int64, NULL);
> > @@ -1296,7 +1329,7 @@ static int __init vivi_create_instance(int inst)
> >  rel_vdev:
> >  	video_device_release(vfd);
> >  unreg_dev:
> > -	v4l2_ctrl_handler_free(hdl);
> > +	v4l2_ctrl_handler_free(&dev->ctrl_handler);
> 
> What's the reason for this change ?
> 
> >  	v4l2_device_unregister(&dev->v4l2_dev);
> >  free_dev:
> >  	kfree(dev);
> 
> 

[RFCv2 PATCH 10/11] V4L2 spec: document control events.

[Hans Verkuil]

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

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 Documentation/DocBook/media-entities.tmpl          |    1 +
 Documentation/DocBook/v4l/vidioc-dqevent.xml       |   17 +++-
 .../DocBook/v4l/vidioc-subscribe-event.xml         |  142 +++++++++++++++++++-
 3 files changed, 158 insertions(+), 2 deletions(-)

diff --git a/Documentation/DocBook/media-entities.tmpl b/Documentation/DocBook/media-entities.tmpl
index c8abb23..e0754a6 100644
--- a/Documentation/DocBook/media-entities.tmpl
+++ b/Documentation/DocBook/media-entities.tmpl
@@ -167,6 +167,7 @@
 <!ENTITY v4l2-event "struct&nbsp;<link linkend='v4l2-event'>v4l2_event</link>">
 <!ENTITY v4l2-event-subscription "struct&nbsp;<link linkend='v4l2-event-subscription'>v4l2_event_subscription</link>">
 <!ENTITY v4l2-event-vsync "struct&nbsp;<link linkend='v4l2-event-vsync'>v4l2_event_vsync</link>">
+<!ENTITY v4l2-event-ctrl "struct&nbsp;<link linkend='v4l2-event-ctrl'>v4l2_event_ctrl</link>">
 <!ENTITY v4l2-ext-control "struct&nbsp;<link linkend='v4l2-ext-control'>v4l2_ext_control</link>">
 <!ENTITY v4l2-ext-controls "struct&nbsp;<link linkend='v4l2-ext-controls'>v4l2_ext_controls</link>">
 <!ENTITY v4l2-fmtdesc "struct&nbsp;<link linkend='v4l2-fmtdesc'>v4l2_fmtdesc</link>">
diff --git a/Documentation/DocBook/v4l/vidioc-dqevent.xml b/Documentation/DocBook/v4l/vidioc-dqevent.xml
index 4e0a7cc..b8c4f76 100644
--- a/Documentation/DocBook/v4l/vidioc-dqevent.xml
+++ b/Documentation/DocBook/v4l/vidioc-dqevent.xml
@@ -81,6 +81,13 @@
 	  </row>
 	  <row>
 	    <entry></entry>
+	    <entry>&v4l2-event-ctrl;</entry>
+            <entry><structfield>ctrl</structfield></entry>
+	    <entry>Event data for event V4L2_EVENT_CTRL.
+            </entry>
+	  </row>
+	  <row>
+	    <entry></entry>
 	    <entry>__u8</entry>
             <entry><structfield>data</structfield>[64]</entry>
 	    <entry>Event data. Defined by the event type. The union
@@ -110,8 +117,16 @@
 	    <entry>Event timestamp.</entry>
 	  </row>
 	  <row>
+	    <entry>u32</entry>
+	    <entry><structfield>id</structfield></entry>
+            <entry></entry>
+	    <entry>The ID associated with the event source. If the event does not
+		have an associated ID (this depends on the event type), then this
+		is 0.</entry>
+	  </row>
+	  <row>
 	    <entry>__u32</entry>
-	    <entry><structfield>reserved</structfield>[9]</entry>
+	    <entry><structfield>reserved</structfield>[8]</entry>
             <entry></entry>
 	    <entry>Reserved for future extensions. Drivers must set
 	    the array to zero.</entry>
diff --git a/Documentation/DocBook/v4l/vidioc-subscribe-event.xml b/Documentation/DocBook/v4l/vidioc-subscribe-event.xml
index 8b50179..975f603 100644
--- a/Documentation/DocBook/v4l/vidioc-subscribe-event.xml
+++ b/Documentation/DocBook/v4l/vidioc-subscribe-event.xml
@@ -64,7 +64,19 @@
 	  </row>
 	  <row>
 	    <entry>__u32</entry>
-	    <entry><structfield>reserved</structfield>[7]</entry>
+	    <entry><structfield>id</structfield></entry>
+	    <entry>ID of the event source. If there is no ID associated with
+		the event source, then set this to 0. Whether or not an event
+		needs an ID depends on the event type.</entry>
+	  </row>
+	  <row>
+	    <entry>__u32</entry>
+	    <entry><structfield>flags</structfield></entry>
+	    <entry>Event flags, see <xref linkend="event-flags" />.</entry>
+	  </row>
+	  <row>
+	    <entry>__u32</entry>
+	    <entry><structfield>reserved</structfield>[5]</entry>
 	    <entry>Reserved for future extensions. Drivers and applications
 	    must set the array to zero.</entry>
 	  </row>
@@ -100,6 +112,23 @@
 	    </entry>
 	  </row>
 	  <row>
+	    <entry><constant>V4L2_EVENT_CTRL</constant></entry>
+	    <entry>3</entry>
+	    <entry>This event requires that the <structfield>id</structfield>
+		matches the control ID from which you want to receive events.
+		This event is triggered if the control's value changes, if a
+		button control is pressed or if the control's flags change.
+	    	This event has &v4l2-event-ctrl; associated with it. This struct
+		contains much of the same information as &v4l2-queryctrl; and
+		&v4l2-control;.
+
+		If the event is generated due to a call to &VIDIOC-S-CTRL; or
+		&VIDIOC-S-EXT-CTRLS;, then the event will not be sent to
+		the file handle that called the ioctl function. This prevents
+		nasty feedback loops.
+	    </entry>
+	  </row>
+	  <row>
 	    <entry><constant>V4L2_EVENT_PRIVATE_START</constant></entry>
 	    <entry>0x08000000</entry>
 	    <entry>Base event number for driver-private events.</entry>
@@ -108,6 +137,23 @@
       </tgroup>
     </table>
 
+    <table pgwide="1" frame="none" id="event-flags">
+      <title>Event Flags</title>
+      <tgroup cols="3">
+	&cs-def;
+	<tbody valign="top">
+	  <row>
+	    <entry><constant>V4L2_EVENT_SUB_FL_SEND_INITIAL</constant></entry>
+	    <entry>0x0001</entry>
+	    <entry>When this event is subscribed an initial event will be sent
+		containing the current status. This only makes sense for events
+		that are triggered by a status change. Other events will ignore
+		this flag.</entry>
+	  </row>
+	</tbody>
+      </tgroup>
+    </table>
+
     <table frame="none" pgwide="1" id="v4l2-event-vsync">
       <title>struct <structname>v4l2_event_vsync</structname></title>
       <tgroup cols="3">
@@ -122,6 +168,100 @@
       </tgroup>
     </table>
 
+    <table frame="none" pgwide="1" id="v4l2-event-ctrl">
+      <title>struct <structname>v4l2_event_ctrl</structname></title>
+      <tgroup cols="4">
+	&cs-str;
+	<tbody valign="top">
+	  <row>
+	    <entry>__u32</entry>
+	    <entry><structfield>changes</structfield></entry>
+	    <entry></entry>
+	    <entry>A bitmask that tells what has changed. See <xref linkend="changes-flags" />.</entry>
+	  </row>
+	  <row>
+	    <entry>__u32</entry>
+	    <entry><structfield>type</structfield></entry>
+	    <entry></entry>
+	    <entry>The type of the control. See &v4l2-ctrl-type;.</entry>
+	  </row>
+	  <row>
+	    <entry>union (anonymous)</entry>
+	    <entry></entry>
+	    <entry></entry>
+	    <entry></entry>
+	  </row>
+	  <row>
+	    <entry></entry>
+	    <entry>__s32</entry>
+	    <entry><structfield>value</structfield></entry>
+	    <entry>The 32-bit value of the control for 32-bit control types.
+		This is 0 for string controls since the value of a string
+		cannot be passed using &VIDIOC-DQEVENT;.</entry>
+	  </row>
+	  <row>
+	    <entry></entry>
+	    <entry>__s64</entry>
+	    <entry><structfield>value64</structfield></entry>
+	    <entry>The 64-bit value of the control for 64-bit control types.</entry>
+	  </row>
+	  <row>
+	    <entry>__u32</entry>
+	    <entry><structfield>flags</structfield></entry>
+	    <entry></entry>
+	    <entry>The control flags. See <xref linkend="control-flags" />.</entry>
+	  </row>
+	  <row>
+	    <entry>__s32</entry>
+	    <entry><structfield>minimum</structfield></entry>
+	    <entry></entry>
+	    <entry>The minimum value of the control. See &v4l2-queryctrl;.</entry>
+	  </row>
+	  <row>
+	    <entry>__s32</entry>
+	    <entry><structfield>maximum</structfield></entry>
+	    <entry></entry>
+	    <entry>The maximum value of the control. See &v4l2-queryctrl;.</entry>
+	  </row>
+	  <row>
+	    <entry>__s32</entry>
+	    <entry><structfield>step</structfield></entry>
+	    <entry></entry>
+	    <entry>The step value of the control. See &v4l2-queryctrl;.</entry>
+	  </row>
+	  <row>
+	    <entry>__s32</entry>
+	    <entry><structfield>default_value</structfield></entry>
+	    <entry></entry>
+	    <entry>The default value value of the control. See &v4l2-queryctrl;.</entry>
+	  </row>
+	</tbody>
+      </tgroup>
+    </table>
+
+    <table pgwide="1" frame="none" id="changes-flags">
+      <title>Changes</title>
+      <tgroup cols="3">
+	&cs-def;
+	<tbody valign="top">
+	  <row>
+	    <entry><constant>V4L2_EVENT_CTRL_CH_VALUE</constant></entry>
+	    <entry>0x0001</entry>
+	    <entry>This control event was triggered because the value of the control
+		changed. Special case: if a button control is pressed, then this
+		event is sent as well, even though there is not explicit value
+		associated with a button control.</entry>
+	  </row>
+	  <row>
+	    <entry><constant>V4L2_EVENT_CTRL_CH_FLAGS</constant></entry>
+	    <entry>0x0002</entry>
+	    <entry>This control event was triggered because the control flags
+		changed.</entry>
+	  </row>
+	</tbody>
+      </tgroup>
+    </table>
+
   </refsect1>
 </refentry>
 <!--
-- 
1.7.1

[RFCv2 PATCH 11/11] v4l2-subdev: implement per-filehandle control handlers.

[Hans Verkuil]

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

To be consistent with v4l2-ioctl.c.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/video/v4l2-device.c |    1 +
 drivers/media/video/v4l2-subdev.c |   14 +++++++-------
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/media/video/v4l2-device.c b/drivers/media/video/v4l2-device.c
index 4aae501..c72856c 100644
--- a/drivers/media/video/v4l2-device.c
+++ b/drivers/media/video/v4l2-device.c
@@ -209,6 +209,7 @@ int v4l2_device_register_subdev_nodes(struct v4l2_device *v4l2_dev)
 		vdev->v4l2_dev = v4l2_dev;
 		vdev->fops = &v4l2_subdev_fops;
 		vdev->release = video_device_release_empty;
+		vdev->ctrl_handler = sd->ctrl_handler;
 		err = __video_register_device(vdev, VFL_TYPE_SUBDEV, -1, 1,
 					      sd->owner);
 		if (err < 0)
diff --git a/drivers/media/video/v4l2-subdev.c b/drivers/media/video/v4l2-subdev.c
index be4cbdd..fd5dcca 100644
--- a/drivers/media/video/v4l2-subdev.c
+++ b/drivers/media/video/v4l2-subdev.c
@@ -155,25 +155,25 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg)
 
 	switch (cmd) {
 	case VIDIOC_QUERYCTRL:
-		return v4l2_queryctrl(sd->ctrl_handler, arg);
+		return v4l2_queryctrl(vfh->ctrl_handler, arg);
 
 	case VIDIOC_QUERYMENU:
-		return v4l2_querymenu(sd->ctrl_handler, arg);
+		return v4l2_querymenu(vfh->ctrl_handler, arg);
 
 	case VIDIOC_G_CTRL:
-		return v4l2_g_ctrl(sd->ctrl_handler, arg);
+		return v4l2_g_ctrl(vfh->ctrl_handler, arg);
 
 	case VIDIOC_S_CTRL:
-		return v4l2_s_ctrl(vfh, sd->ctrl_handler, arg);
+		return v4l2_s_ctrl(vfh, vfh->ctrl_handler, arg);
 
 	case VIDIOC_G_EXT_CTRLS:
-		return v4l2_g_ext_ctrls(sd->ctrl_handler, arg);
+		return v4l2_g_ext_ctrls(vfh->ctrl_handler, arg);
 
 	case VIDIOC_S_EXT_CTRLS:
-		return v4l2_s_ext_ctrls(vfh, sd->ctrl_handler, arg);
+		return v4l2_s_ext_ctrls(vfh, vfh->ctrl_handler, arg);
 
 	case VIDIOC_TRY_EXT_CTRLS:
-		return v4l2_try_ext_ctrls(sd->ctrl_handler, arg);
+		return v4l2_try_ext_ctrls(vfh->ctrl_handler, arg);
 
 	case VIDIOC_DQEVENT:
 		if (!(sd->flags & V4L2_SUBDEV_FL_HAS_EVENTS))
-- 
1.7.1

Re: [RFCv2 PATCH 00/11] Control Event

[Laurent Pinchart]

Hi Hans,

Thanks for the patch set.

On Wednesday 25 May 2011 15:33:44 Hans Verkuil wrote:
> This is the second version of the patch series introducing a new event that
> is triggered when a control's value or state changes.

One general comment. The API lets applications subscribe to events for 
individual controls. Should we also have an API to subscribe to events for all 
controls ?

-- 
Regards,

Laurent Pinchart

Re: [RFCv2 PATCH 00/11] Control Event

[Hans Verkuil]

On Friday, June 03, 2011 21:54:47 Laurent Pinchart wrote:
> Hi Hans,
> 
> Thanks for the patch set.
> 
> On Wednesday 25 May 2011 15:33:44 Hans Verkuil wrote:
> > This is the second version of the patch series introducing a new event that
> > is triggered when a control's value or state changes.
> 
> One general comment. The API lets applications subscribe to events for 
> individual controls. Should we also have an API to subscribe to events for all 
> controls ?

Right now there is no need for it, but should we need this, then I planned to
allow control event subscription with id == 0 or id == -1 or by setting a flag
or something similar.

Regards,

	Hans