[RFC PATCH 0/3] v4l2-ctrls: add new functionality

[RFC PATCH 0/3] v4l2-ctrls: add new functionality

[Hans Verkuil]

This RFC patch series adds and documents two new features of the control
framework.

The first adds support to enable or disable specific controls or all controls
from a control handler. This is needed to support drivers that need to change
which controls are available based on the chosen input or output. In cases
like this each input or output is hooked up to a different video receiver or
transmitter, each with its own set of controls. Switching inputs/outputs means
that the controls from the new input should be enabled, while those of the
others should be disabled.

The second adds support to simplify handling of 'auto-foo/foo' type of controls.
E.g.: autogain/gain, autoexposure/exposure, etc. It is a bit tricky to handle
that correctly and after having to do this many times when I was converting the
soc_camera sensors I decided to add special support for this in the framework.

It should ensure consistent handling of these special kinds of controls in the
drivers.

If there are no comments, then I plan on making a pull request for this for
2.6.39 and base the soc_camera and ov7670 conversions on this. For 2.6.39 I
want to finish converting all subdev to the control framework and make a good
start at converting the v4l2 drivers as well.

[RFC PATCH 1/3] v4l2-ctrls: must be able to enable/disable controls

[Hans Verkuil]

Controls can be dependent on the chosen input/output. So it has to be possible
to enable or disable groups of controls, preventing them from being seen in
the application.

We need to allow duplicate controls as well so that two control handlers
that both have the same control will still work. The first enabled control
will win. And duplicate controls are always sorted based on when they were
added (so the sorted list and the hash are both stable lists/hashes).

Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
---
 drivers/media/video/v4l2-ctrls.c |  123 +++++++++++++++++++++++++-------------
 include/media/v4l2-ctrls.h       |   34 +++++++++++
 2 files changed, 115 insertions(+), 42 deletions(-)

diff --git a/drivers/media/video/v4l2-ctrls.c b/drivers/media/video/v4l2-ctrls.c
index ef66d2a..983e287 100644
--- a/drivers/media/video/v4l2-ctrls.c
+++ b/drivers/media/video/v4l2-ctrls.c
@@ -820,7 +820,7 @@ static struct v4l2_ctrl_ref *find_private_ref(
 		   VIDIOC_G/S_CTRL. */
 		if (V4L2_CTRL_ID2CLASS(ref->ctrl->id) == V4L2_CTRL_CLASS_USER &&
 		    V4L2_CTRL_DRIVER_PRIV(ref->ctrl->id)) {
-			if (!type_is_int(ref->ctrl))
+			if (!ref->ctrl->is_enabled || !type_is_int(ref->ctrl))
 				continue;
 			if (id == 0)
 				return ref;
@@ -849,7 +849,7 @@ static struct v4l2_ctrl_ref *find_ref(struct v4l2_ctrl_handler *hdl, u32 id)
 
 	/* Not in cache, search the hash */
 	ref = hdl->buckets ? hdl->buckets[bucket] : NULL;
-	while (ref && ref->ctrl->id != id)
+	while (ref && ref->ctrl->id != id && ref->ctrl->is_enabled)
 		ref = ref->next;
 
 	if (ref)
@@ -926,23 +926,28 @@ static int handler_new_ref(struct v4l2_ctrl_handler *hdl,
 
 	/* Find insert position in sorted list */
 	list_for_each_entry(ref, &hdl->ctrl_refs, node) {
-		if (ref->ctrl->id < id)
-			continue;
-		/* Don't add duplicates */
-		if (ref->ctrl->id == id) {
-			kfree(new_ref);
-			goto unlock;
+		/* If there are multiple elements with the same ID, then
+		   add the new element at the end. */
+		if (ref->ctrl->id > id) {
+			list_add(&new_ref->node, ref->node.prev);
+			break;
 		}
-		list_add(&new_ref->node, ref->node.prev);
-		break;
 	}
 
 insert_in_hash:
-	/* Insert the control node in the hash */
-	new_ref->next = hdl->buckets[bucket];
-	hdl->buckets[bucket] = new_ref;
+	/* Append the control ref to the hash */
+	if (hdl->buckets[bucket] == NULL) {
+		hdl->buckets[bucket] = new_ref;
+	} else {
+		for (ref = hdl->buckets[bucket]; ref->next; ref = ref->next)
+			; /* empty */
+		ref->next = new_ref;
+	}
+	/* Note regarding the hdl->cached control ref: since new control refs
+	   are always appended after any existing controls they will never
+	   invalidate the cached control ref. So there is no need to set the
+	   hdl->cached pointer to NULL. */
 
-unlock:
 	mutex_unlock(&hdl->lock);
 	return 0;
 }
@@ -960,8 +965,22 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
 	if (hdl->error)
 		return NULL;
 
-	/* Sanity checks */
+	/* Sanity checks:
+	   - id must never be 0 (reserved value as it is the starting point
+	     if apps want to iterate over all controls using
+	     V4L2_CTRL_FLAG_NEXT_CTRL).
+	   - name must be set.
+	   - V4L2_CID_PRIVATE_BASE IDs are no longer allowed: these IDs make
+	     it impossible to set the control using explicit control IDs.
+	   - def must be in range and max must be >= min.
+	   - V4L2_CTRL_FLAG_DISABLED must not be used by drivers any more.
+	     There are better ways of doing this.
+	   - Integer types must have a non-zero step.
+	   - Menu types must have a menu.
+	   - String types must have a non-zero max string length.
+	 */
 	if (id == 0 || name == NULL || id >= V4L2_CID_PRIVATE_BASE ||
+	    (flags & V4L2_CTRL_FLAG_DISABLED) ||
 	    max < min ||
 	    (type == V4L2_CTRL_TYPE_INTEGER && step == 0) ||
 	    (type == V4L2_CTRL_TYPE_MENU && qmenu == NULL) ||
@@ -1002,6 +1021,7 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
 	ctrl->step = step;
 	ctrl->qmenu = qmenu;
 	ctrl->priv = priv;
+	ctrl->is_enabled = 1;
 	ctrl->cur.val = ctrl->val = ctrl->default_value = def;
 
 	if (ctrl->type == V4L2_CTRL_TYPE_STRING) {
@@ -1193,14 +1213,39 @@ void v4l2_ctrl_grab(struct v4l2_ctrl *ctrl, bool grabbed)
 }
 EXPORT_SYMBOL(v4l2_ctrl_grab);
 
+/* Enable/disable a control.
+   Usually used if controls can be enabled/disabled when changing to a
+   different input or output.
+
+   When a control is disabled, then it will no longer show up in the
+   application. */
+void v4l2_ctrl_enable(struct v4l2_ctrl *ctrl, bool enabled)
+{
+	if (ctrl == NULL)
+		return;
+
+	ctrl->is_enabled = enabled;
+}
+EXPORT_SYMBOL(v4l2_ctrl_enable);
+
+void v4l2_ctrl_handler_enable(struct v4l2_ctrl_handler *hdl, bool enabled)
+{
+	struct v4l2_ctrl *ctrl;
+
+	if (hdl == NULL)
+		return;
+	mutex_lock(&hdl->lock);
+	list_for_each_entry(ctrl, &hdl->ctrls, node)
+		ctrl->is_enabled = enabled;
+	mutex_unlock(&hdl->lock);
+}
+EXPORT_SYMBOL(v4l2_ctrl_handler_enable);
+
 /* Log the control name and value */
 static void log_ctrl(const struct v4l2_ctrl *ctrl,
 		     const char *prefix, const char *colon)
 {
-	int fl_inact = ctrl->flags & V4L2_CTRL_FLAG_INACTIVE;
-	int fl_grabbed = ctrl->flags & V4L2_CTRL_FLAG_GRABBED;
-
-	if (ctrl->flags & (V4L2_CTRL_FLAG_DISABLED | V4L2_CTRL_FLAG_WRITE_ONLY))
+	if (ctrl->flags & V4L2_CTRL_FLAG_WRITE_ONLY)
 		return;
 	if (ctrl->type == V4L2_CTRL_TYPE_CTRL_CLASS)
 		return;
@@ -1221,20 +1266,19 @@ static void log_ctrl(const struct v4l2_ctrl *ctrl,
 		printk(KERN_CONT "%lld", ctrl->cur.val64);
 		break;
 	case V4L2_CTRL_TYPE_STRING:
-		printk(KERN_CONT "%s", ctrl->cur.string);
+		printk(KERN_CONT "\"%s\"", ctrl->cur.string);
 		break;
 	default:
 		printk(KERN_CONT "unknown type %d", ctrl->type);
 		break;
 	}
-	if (fl_inact && fl_grabbed)
-		printk(KERN_CONT " (inactive, grabbed)\n");
-	else if (fl_inact)
-		printk(KERN_CONT " (inactive)\n");
-	else if (fl_grabbed)
-		printk(KERN_CONT " (grabbed)\n");
-	else
-		printk(KERN_CONT "\n");
+	if (ctrl->flags & V4L2_CTRL_FLAG_INACTIVE)
+		printk(KERN_CONT " inactive");
+	if (ctrl->flags & V4L2_CTRL_FLAG_GRABBED)
+		printk(KERN_CONT " grabbed");
+	if (!ctrl->is_enabled)
+		printk(KERN_CONT " disabled");
+	printk(KERN_CONT "\n");
 }
 
 /* Log all controls owned by the handler */
@@ -1254,8 +1298,7 @@ void v4l2_ctrl_handler_log_status(struct v4l2_ctrl_handler *hdl,
 		colon = ": ";
 	mutex_lock(&hdl->lock);
 	list_for_each_entry(ctrl, &hdl->ctrls, node)
-		if (!(ctrl->flags & V4L2_CTRL_FLAG_DISABLED))
-			log_ctrl(ctrl, prefix, colon);
+		log_ctrl(ctrl, prefix, colon);
 	mutex_unlock(&hdl->lock);
 }
 EXPORT_SYMBOL(v4l2_ctrl_handler_log_status);
@@ -1324,17 +1367,15 @@ int v4l2_queryctrl(struct v4l2_ctrl_handler *hdl, struct v4l2_queryctrl *qc)
 		/* Did we reach the end of the control list? */
 		if (id >= node2id(hdl->ctrl_refs.prev)) {
 			ref = NULL; /* Yes, so there is no next control */
-		} else if (ref) {
-			/* We found a control with the given ID, so just get
-			   the next one in the list. */
-			ref = list_entry(ref->node.next, typeof(*ref), node);
 		} else {
-			/* No control with the given ID exists, so start
-			   searching for the next largest ID. We know there
-			   is one, otherwise the first 'if' above would have
-			   been true. */
-			list_for_each_entry(ref, &hdl->ctrl_refs, node)
-				if (id < ref->ctrl->id)
+			/* If no ref was found, then start searching from the
+			   beginning of the ctrl_refs list. */
+			if (ref == NULL)
+				ref = list_entry(hdl->ctrl_refs.next,
+						typeof(*ref), node);
+			/* Search for the next largest ID. */
+			list_for_each_entry_from(ref, &hdl->ctrl_refs, node)
+				if (ref->ctrl->is_enabled && id < ref->ctrl->id)
 					break;
 		}
 	}
@@ -1468,8 +1509,6 @@ static int prepare_ext_ctrls(struct v4l2_ctrl_handler *hdl,
 		ctrl = v4l2_ctrl_find(hdl, id);
 		if (ctrl == NULL)
 			return -EINVAL;
-		if (ctrl->flags & V4L2_CTRL_FLAG_DISABLED)
-			return -EINVAL;
 
 		helpers[i].ctrl = ctrl;
 		helpers[i].handled = false;
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index 97d0638..a2b2d58 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -65,6 +65,8 @@ struct v4l2_ctrl_ops {
   *		control's current value cannot be cached and needs to be
   *		retrieved through the g_volatile_ctrl op. Drivers can set
   *		this flag.
+  * @is_enabled: If 0, then this control is disabled and will be hidden for
+  *		applications. Controls are always enabled by default.
   * @ops:	The control ops.
   * @id:	The control ID.
   * @name:	The control name.
@@ -105,6 +107,7 @@ struct v4l2_ctrl {
 	unsigned int is_new:1;
 	unsigned int is_private:1;
 	unsigned int is_volatile:1;
+	unsigned int is_enabled:1;
 
 	const struct v4l2_ctrl_ops *ops;
 	u32 id;
@@ -399,6 +402,37 @@ void v4l2_ctrl_activate(struct v4l2_ctrl *ctrl, bool active);
   */
 void v4l2_ctrl_grab(struct v4l2_ctrl *ctrl, bool grabbed);
 
+/** v4l2_ctrl_enable() - Mark the control as enabled or disabled.
+  * @ctrl:	The control to en/disable.
+  * @enabled:	True if the control should become enabled.
+  *
+  * Enable/disable a control.
+  * Does nothing if @ctrl == NULL.
+  * Usually called if controls are to be enabled or disabled when changing
+  * to a different input or output.
+  *
+  * When a control is disabled, then it will no longer show up in the
+  * application.
+  *
+  * This function can be called regardless of whether the control handler
+  * is locked or not.
+  */
+void v4l2_ctrl_enable(struct v4l2_ctrl *ctrl, bool enabled);
+
+/** v4l2_ctrl_handler_enable() - Mark the controls in the handler as enabled or disabled.
+  * @hdl:	The control handler.
+  * @enabled:	True if the controls should become enabled.
+  *
+  * Enable/disable the controls owned by the handler.
+  * Does nothing if @hdl == NULL.
+  * Usually called if controls are to be enabled or disabled when changing
+  * to a different input or output.
+  *
+  * When a control is disabled, then it will no longer show up in the
+  * application.
+  */
+void v4l2_ctrl_handler_enable(struct v4l2_ctrl_handler *hdl, bool enabled);
+
 /** v4l2_ctrl_lock() - Helper function to lock the handler
   * associated with the control.
   * @ctrl:	The control to lock.
-- 
1.7.0.4

[RFC PATCH 2/3] v4l2-ctrls: add v4l2_ctrl_auto_cluster to simplify autogain/gain scenarios

[Hans Verkuil]

It is a bit tricky to handle autogain/gain type scenerios correctly. Such
controls need to be clustered and the V4L2_CTRL_FLAG_UPDATE should be set on
the non-auto controls. If you set a non-auto control without setting the
auto control at the same time, then the auto control should switch to manual
mode. And usually the non-auto controls must be marked volatile, but this should
only be in effect if the auto control is set to auto.

The chances of drivers doing all these things correctly are pretty remote.
So a new v4l2_ctrl_auto_cluster function was added that takes care of these
issues.

Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
---
 drivers/media/video/v4l2-ctrls.c |   27 ++++++++++++++++++++-
 include/media/v4l2-ctrls.h       |   48 +++++++++++++++++++++++++++++++++++++-
 2 files changed, 73 insertions(+), 2 deletions(-)

diff --git a/drivers/media/video/v4l2-ctrls.c b/drivers/media/video/v4l2-ctrls.c
index 983e287..b5da617 100644
--- a/drivers/media/video/v4l2-ctrls.c
+++ b/drivers/media/video/v4l2-ctrls.c
@@ -1178,6 +1178,25 @@ void v4l2_ctrl_cluster(unsigned ncontrols, struct v4l2_ctrl **controls)
 }
 EXPORT_SYMBOL(v4l2_ctrl_cluster);
 
+void v4l2_ctrl_auto_cluster(unsigned ncontrols, struct v4l2_ctrl **controls,
+			    u8 manual_val, bool set_volatile)
+{
+	int i;
+
+	v4l2_ctrl_cluster(ncontrols, controls);
+	controls[0]->is_auto = true;
+	controls[0]->manual_mode_value = manual_val;
+
+	for (i = 1; i < ncontrols; i++) {
+		if (controls[i]) {
+			controls[i]->flags |= V4L2_CTRL_FLAG_UPDATE;
+			if (set_volatile)
+				controls[i]->is_volatile = true;
+		}
+	}
+}
+EXPORT_SYMBOL(v4l2_ctrl_auto_cluster);
+
 /* Activate/deactivate a control. */
 void v4l2_ctrl_activate(struct v4l2_ctrl *ctrl, bool active)
 {
@@ -1605,7 +1624,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)
+		if (ctrl->is_volatile && master->ops->g_volatile_ctrl &&
+		    (!master->is_auto || master->cur.val != master->manual_mode_value))
 			ret = master->ops->g_volatile_ctrl(master);
 		/* If OK, then copy the current control values to the caller */
 		if (!ret)
@@ -1717,6 +1737,11 @@ static int try_or_set_control_cluster(struct v4l2_ctrl *master, bool set)
 
 	/* Don't set if there is no change */
 	if (!ret && set && cluster_changed(master)) {
+		if (master->is_auto && !master->is_new &&
+		    master->cur.val != master->manual_mode_value) {
+			master->is_new = 1;
+			master->val = master->manual_mode_value;
+		}
 		ret = master->ops->s_ctrl(master);
 		/* If OK, then make the new values permanent. */
 		if (!ret)
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index a2b2d58..e715f4e 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -66,7 +66,16 @@ struct v4l2_ctrl_ops {
   *		retrieved through the g_volatile_ctrl op. Drivers can set
   *		this flag.
   * @is_enabled: If 0, then this control is disabled and will be hidden for
-  *		applications. Controls are always enabled by default.
+  *		applications. Controls are always enabled by default. Drivers
+  *		should never set this flag.
+  * @is_auto:   If set, then this control selects whether the other cluster
+  *		members are in 'automatic' mode or 'manual' mode. This is
+  *		used for autogain/gain type clusters. Drivers should never
+  *		set this flag.
+  * @manual_mode_value: If the is_auto flag is set, then this is the value
+  *		of the auto control that determines if that control is in
+  *		manual mode. So if the value of the auto control equals this
+  *		value, then the whole cluster is in manual mode.
   * @ops:	The control ops.
   * @id:	The control ID.
   * @name:	The control name.
@@ -108,6 +117,8 @@ struct v4l2_ctrl {
 	unsigned int is_private:1;
 	unsigned int is_volatile:1;
 	unsigned int is_enabled:1;
+	unsigned int is_auto:1;
+	unsigned int manual_mode_value:5;
 
 	const struct v4l2_ctrl_ops *ops;
 	u32 id;
@@ -366,6 +377,41 @@ int v4l2_ctrl_add_handler(struct v4l2_ctrl_handler *hdl,
 void v4l2_ctrl_cluster(unsigned ncontrols, struct v4l2_ctrl **controls);
 
 
+/** v4l2_ctrl_auto_cluster() - Mark all controls in the cluster as belonging to
+  * that cluster and set it up for autofoo/foo-type handling.
+  * @ncontrols:	The number of controls in this cluster.
+  * @controls:	The cluster control array of size @ncontrols. The first control
+  *		must be the 'auto' control (e.g. autogain, autoexposure, etc.)
+  * @manual_val: The value for the first control in the cluster that equals the
+  *		manual setting.
+  * @set_volatile: If true, then all controls except the first auto control will
+  *		have is_volatile set to true. If false, then is_volatile will not
+  *		be touched.
+  *
+  * Use for control groups where one control selects some automatic feature and
+  * the other controls are only active whenever the automatic feature is turned
+  * off (manual mode). Typical examples: autogain vs gain, auto-whitebalance vs
+  * red and blue balance, etc.
+  *
+  * Using this function lets the framework take care of some of the actions
+  * related to such controls: whenever one of the manual controls are set
+  * without touching the auto control, then the auto control is set to
+  * manual mode. So in the s_ctrl op you can just set the new values. Also,
+  * g_volatile_ctrl is never called when in manual mode, since in that case
+  * the current control value can just be returned as is.
+  *
+  * This function also makes it easy to set all non-auto controls to volatile
+  * by setting the last argument to true. In most cases non-auto controls are
+  * volatile when in auto mode. E.g. if autogain is set, then the gain register
+  * usually reports the current automatically selected gain.
+  *
+  * This function also sets the V4L2_CTRL_FLAG_UPDATE for all the non-auto
+  * controls.
+  */
+void v4l2_ctrl_auto_cluster(unsigned ncontrols, struct v4l2_ctrl **controls,
+			u8 manual_val, bool set_volatile);
+
+
 /** v4l2_ctrl_find() - Find a control with the given ID.
   * @hdl:	The control handler.
   * @id:	The control ID to find.
-- 
1.7.0.4

[RFC PATCH 3/3] v4l2-ctrls: update control framework documentation

[Hans Verkuil]

Document how to enable/disable controls.
Document the new v4l2_ctrl_auto_cluster function.
Document the practical method of using anonymous structs to 'cluster'
controls instead of using cumbersome control pointer arrays.

Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
---
 Documentation/video4linux/v4l2-controls.txt |   94 +++++++++++++++++++++++++++
 1 files changed, 94 insertions(+), 0 deletions(-)

diff --git a/Documentation/video4linux/v4l2-controls.txt b/Documentation/video4linux/v4l2-controls.txt
index 881e7f4..78b6674 100644
--- a/Documentation/video4linux/v4l2-controls.txt
+++ b/Documentation/video4linux/v4l2-controls.txt
@@ -453,6 +453,25 @@ In the example above the following are equivalent for the VOLUME case:
 	ctrl == ctrl->cluster[AUDIO_CL_VOLUME] == state->audio_cluster[AUDIO_CL_VOLUME]
 	ctrl->cluster[AUDIO_CL_MUTE] == state->audio_cluster[AUDIO_CL_MUTE]
 
+In practice using cluster arrays like this becomes very tiresome. So instead
+the following equivalent method is used:
+
+	struct {
+		/* audio cluster */
+		struct v4l2_ctrl *volume;
+		struct v4l2_ctrl *mute;
+	};
+
+The anonymous struct is used to clearly 'cluster' these two control pointers,
+but it serves no other purpose. The effect is the same as creating an
+array with two control pointers. So you can just do:
+
+	state->volume = v4l2_ctrl_new_std(&state->ctrl_handler, ...);
+	state->mute = v4l2_ctrl_new_std(&state->ctrl_handler, ...);
+	v4l2_ctrl_cluster(2, &state->volume);
+
+And in foo_s_ctrl you can use these pointers directly: state->mute->val.
+
 Note that controls in a cluster may be NULL. For example, if for some
 reason mute was never added (because the hardware doesn't support that
 particular feature), then mute will be NULL. So in that case we have a
@@ -475,6 +494,55 @@ controls, then the 'is_new' flag would be 1 for both controls.
 The 'is_new' flag is always 1 when called from v4l2_ctrl_handler_setup().
 
 
+Handling autogain/gain-type Controls with Auto Clusters
+=======================================================
+
+A common type of control cluster is one that handles 'auto-foo/foo'-type
+controls. Typical examples are autogain/gain, autoexposure/exposure,
+autowhitebalance/red balance/blue balance. In all cases you have one controls
+that determines whether another control is handled automatically by the hardware,
+or whether it is under manual control from the user.
+
+The way these are supposed to be handled is that if you set one of the 'foo'
+controls, then the 'auto-foo' control should automatically switch to manual
+mode, except when you set the 'auto-foo' control at the same time, in which
+case it will depend on the new 'auto-foo' setting whether the new 'foo' value
+is actually ignored or set in the hardware.
+
+The reasoning is that if you explicitly set a manual control, then it makes
+sense to assume that you want to switch to manual mode as well. Whereas if you
+set both the auto and manual control at the same time, then you should follow
+whatever the new value for the auto control is.
+
+Usually the 'foo' control is also volatile, since if the automatic mode is
+enabled, then the reported value for 'foo' is the value that the automatic
+mode has determined is the best at that given time. However, if manual mode
+is selected, then it is just the last stored value. So g_volatile_ctrl should
+only be called when we are in automatic mode.
+
+Finally the V4L2_CTRL_FLAG_UPDATE should also be set for the non-auto controls
+since changing one of them might affect the auto control.
+
+In order to simplify this a special variation of v4l2_ctrl_cluster was
+introduced:
+
+void v4l2_ctrl_auto_cluster(unsigned ncontrols, struct v4l2_ctrl **controls,
+			u8 manual_val, bool set_volatile);
+
+The first two arguments are identical to v4l2_ctrl_cluster. The third argument
+tells the framework which value switches the cluster into manual mode. The
+last argument will optionally set is_volatile flag for the non-auto controls.
+
+The first control of the cluster is assumed to be the 'auto' control.
+
+Using this function will ensure that:
+
+- the right flags are set.
+- when a 'foo' control is set explicitly the 'auto-foo' control is set to
+  the manual mode before s_ctrl is called.
+- g_volatile_ctrl is never called when in manual mode.
+
+
 VIDIOC_LOG_STATUS Support
 =========================
 
@@ -535,6 +603,32 @@ control. The rule is to have one control for each hardware 'knob' that you
 can twiddle.
 
 
+Different Handlers for Different Inputs/Outputs
+===============================================
+
+Sometimes the available controls will change depending on which input or output
+was chosen. This can happen if input 0 handles SDTV and is hooked up to a SDTV
+video receiver and if input 1 handles HDTV and is hooked up to a HDTV receiver.
+
+Each will typically have their own controls. Which means that in this case
+changing inputs or outputs requires some work. To make this easy it is possible
+to enable or disable specific controls or all controls from one control handler:
+
+void v4l2_ctrl_enable(struct v4l2_ctrl *ctrl, bool enabled);
+void v4l2_ctrl_handler_enable(struct v4l2_ctrl_handler *hdl, bool enabled);
+
+So in this scenario you would typically disable the control handler of the
+inputs that are not in use and enable the control handler of the active input.
+
+For example, if the user selects input 1, then the driver should do something
+like this:
+
+	v4l2_ctrl_handler_enable(sdtv_subdev->ctrl_handler, false);
+	v4l2_ctrl_handler_enable(hdtv_subdev->ctrl_handler, true);
+
+By default all controls are enabled when they are first created.
+
+
 Finding Controls
 ================
 
-- 
1.7.0.4

Re: [RFC PATCH 1/3] v4l2-ctrls: must be able to enable/disable controls

[Andy Walls]

On Sat, 2011-01-22 at 12:05 +0100, Hans Verkuil wrote: 
> Controls can be dependent on the chosen input/output. So it has to be possible
> to enable or disable groups of controls, preventing them from being seen in
> the application.

I'm not a human factors expert, but given some of the principles listed
here:

	http://www.asktog.com/basics/firstPrinciples.html

I get the feeling that forcing controls to disappear and reappear will
not be good for every possible application UI design.

I'm sure that due to subdevices providing all sorts of interesting
controls, some method of disabling controls is needed so as not to show
duplicates, etc.

However, would it be better to provide a hint to the application and let
the application UI designer decide what gets shown; instead of the
kernel dictating what is shown?  

Regards,
Andy

> We need to allow duplicate controls as well so that two control handlers
> that both have the same control will still work. The first enabled control
> will win. And duplicate controls are always sorted based on when they were
> added (so the sorted list and the hash are both stable lists/hashes).
> 
> Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
> ---
>  drivers/media/video/v4l2-ctrls.c |  123 +++++++++++++++++++++++++-------------
>  include/media/v4l2-ctrls.h       |   34 +++++++++++
>  2 files changed, 115 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/media/video/v4l2-ctrls.c b/drivers/media/video/v4l2-ctrls.c
> index ef66d2a..983e287 100644
> --- a/drivers/media/video/v4l2-ctrls.c
> +++ b/drivers/media/video/v4l2-ctrls.c
> @@ -820,7 +820,7 @@ static struct v4l2_ctrl_ref *find_private_ref(
>  		   VIDIOC_G/S_CTRL. */
>  		if (V4L2_CTRL_ID2CLASS(ref->ctrl->id) == V4L2_CTRL_CLASS_USER &&
>  		    V4L2_CTRL_DRIVER_PRIV(ref->ctrl->id)) {
> -			if (!type_is_int(ref->ctrl))
> +			if (!ref->ctrl->is_enabled || !type_is_int(ref->ctrl))
>  				continue;
>  			if (id == 0)
>  				return ref;
> @@ -849,7 +849,7 @@ static struct v4l2_ctrl_ref *find_ref(struct v4l2_ctrl_handler *hdl, u32 id)
>  
>  	/* Not in cache, search the hash */
>  	ref = hdl->buckets ? hdl->buckets[bucket] : NULL;
> -	while (ref && ref->ctrl->id != id)
> +	while (ref && ref->ctrl->id != id && ref->ctrl->is_enabled)
>  		ref = ref->next;
>  
>  	if (ref)
> @@ -926,23 +926,28 @@ static int handler_new_ref(struct v4l2_ctrl_handler *hdl,
>  
>  	/* Find insert position in sorted list */
>  	list_for_each_entry(ref, &hdl->ctrl_refs, node) {
> -		if (ref->ctrl->id < id)
> -			continue;
> -		/* Don't add duplicates */
> -		if (ref->ctrl->id == id) {
> -			kfree(new_ref);
> -			goto unlock;
> +		/* If there are multiple elements with the same ID, then
> +		   add the new element at the end. */
> +		if (ref->ctrl->id > id) {
> +			list_add(&new_ref->node, ref->node.prev);
> +			break;
>  		}
> -		list_add(&new_ref->node, ref->node.prev);
> -		break;
>  	}
>  
>  insert_in_hash:
> -	/* Insert the control node in the hash */
> -	new_ref->next = hdl->buckets[bucket];
> -	hdl->buckets[bucket] = new_ref;
> +	/* Append the control ref to the hash */
> +	if (hdl->buckets[bucket] == NULL) {
> +		hdl->buckets[bucket] = new_ref;
> +	} else {
> +		for (ref = hdl->buckets[bucket]; ref->next; ref = ref->next)
> +			; /* empty */
> +		ref->next = new_ref;
> +	}
> +	/* Note regarding the hdl->cached control ref: since new control refs
> +	   are always appended after any existing controls they will never
> +	   invalidate the cached control ref. So there is no need to set the
> +	   hdl->cached pointer to NULL. */
>  
> -unlock:
>  	mutex_unlock(&hdl->lock);
>  	return 0;
>  }
> @@ -960,8 +965,22 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
>  	if (hdl->error)
>  		return NULL;
>  
> -	/* Sanity checks */
> +	/* Sanity checks:
> +	   - id must never be 0 (reserved value as it is the starting point
> +	     if apps want to iterate over all controls using
> +	     V4L2_CTRL_FLAG_NEXT_CTRL).
> +	   - name must be set.
> +	   - V4L2_CID_PRIVATE_BASE IDs are no longer allowed: these IDs make
> +	     it impossible to set the control using explicit control IDs.
> +	   - def must be in range and max must be >= min.
> +	   - V4L2_CTRL_FLAG_DISABLED must not be used by drivers any more.
> +	     There are better ways of doing this.
> +	   - Integer types must have a non-zero step.
> +	   - Menu types must have a menu.
> +	   - String types must have a non-zero max string length.
> +	 */
>  	if (id == 0 || name == NULL || id >= V4L2_CID_PRIVATE_BASE ||
> +	    (flags & V4L2_CTRL_FLAG_DISABLED) ||
>  	    max < min ||
>  	    (type == V4L2_CTRL_TYPE_INTEGER && step == 0) ||
>  	    (type == V4L2_CTRL_TYPE_MENU && qmenu == NULL) ||
> @@ -1002,6 +1021,7 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
>  	ctrl->step = step;
>  	ctrl->qmenu = qmenu;
>  	ctrl->priv = priv;
> +	ctrl->is_enabled = 1;
>  	ctrl->cur.val = ctrl->val = ctrl->default_value = def;
>  
>  	if (ctrl->type == V4L2_CTRL_TYPE_STRING) {
> @@ -1193,14 +1213,39 @@ void v4l2_ctrl_grab(struct v4l2_ctrl *ctrl, bool grabbed)
>  }
>  EXPORT_SYMBOL(v4l2_ctrl_grab);
>  
> +/* Enable/disable a control.
> +   Usually used if controls can be enabled/disabled when changing to a
> +   different input or output.
> +
> +   When a control is disabled, then it will no longer show up in the
> +   application. */
> +void v4l2_ctrl_enable(struct v4l2_ctrl *ctrl, bool enabled)
> +{
> +	if (ctrl == NULL)
> +		return;
> +
> +	ctrl->is_enabled = enabled;
> +}
> +EXPORT_SYMBOL(v4l2_ctrl_enable);
> +
> +void v4l2_ctrl_handler_enable(struct v4l2_ctrl_handler *hdl, bool enabled)
> +{
> +	struct v4l2_ctrl *ctrl;
> +
> +	if (hdl == NULL)
> +		return;
> +	mutex_lock(&hdl->lock);
> +	list_for_each_entry(ctrl, &hdl->ctrls, node)
> +		ctrl->is_enabled = enabled;
> +	mutex_unlock(&hdl->lock);
> +}
> +EXPORT_SYMBOL(v4l2_ctrl_handler_enable);
> +
>  /* Log the control name and value */
>  static void log_ctrl(const struct v4l2_ctrl *ctrl,
>  		     const char *prefix, const char *colon)
>  {
> -	int fl_inact = ctrl->flags & V4L2_CTRL_FLAG_INACTIVE;
> -	int fl_grabbed = ctrl->flags & V4L2_CTRL_FLAG_GRABBED;
> -
> -	if (ctrl->flags & (V4L2_CTRL_FLAG_DISABLED | V4L2_CTRL_FLAG_WRITE_ONLY))
> +	if (ctrl->flags & V4L2_CTRL_FLAG_WRITE_ONLY)
>  		return;
>  	if (ctrl->type == V4L2_CTRL_TYPE_CTRL_CLASS)
>  		return;
> @@ -1221,20 +1266,19 @@ static void log_ctrl(const struct v4l2_ctrl *ctrl,
>  		printk(KERN_CONT "%lld", ctrl->cur.val64);
>  		break;
>  	case V4L2_CTRL_TYPE_STRING:
> -		printk(KERN_CONT "%s", ctrl->cur.string);
> +		printk(KERN_CONT "\"%s\"", ctrl->cur.string);
>  		break;
>  	default:
>  		printk(KERN_CONT "unknown type %d", ctrl->type);
>  		break;
>  	}
> -	if (fl_inact && fl_grabbed)
> -		printk(KERN_CONT " (inactive, grabbed)\n");
> -	else if (fl_inact)
> -		printk(KERN_CONT " (inactive)\n");
> -	else if (fl_grabbed)
> -		printk(KERN_CONT " (grabbed)\n");
> -	else
> -		printk(KERN_CONT "\n");
> +	if (ctrl->flags & V4L2_CTRL_FLAG_INACTIVE)
> +		printk(KERN_CONT " inactive");
> +	if (ctrl->flags & V4L2_CTRL_FLAG_GRABBED)
> +		printk(KERN_CONT " grabbed");
> +	if (!ctrl->is_enabled)
> +		printk(KERN_CONT " disabled");
> +	printk(KERN_CONT "\n");
>  }
>  
>  /* Log all controls owned by the handler */
> @@ -1254,8 +1298,7 @@ void v4l2_ctrl_handler_log_status(struct v4l2_ctrl_handler *hdl,
>  		colon = ": ";
>  	mutex_lock(&hdl->lock);
>  	list_for_each_entry(ctrl, &hdl->ctrls, node)
> -		if (!(ctrl->flags & V4L2_CTRL_FLAG_DISABLED))
> -			log_ctrl(ctrl, prefix, colon);
> +		log_ctrl(ctrl, prefix, colon);
>  	mutex_unlock(&hdl->lock);
>  }
>  EXPORT_SYMBOL(v4l2_ctrl_handler_log_status);
> @@ -1324,17 +1367,15 @@ int v4l2_queryctrl(struct v4l2_ctrl_handler *hdl, struct v4l2_queryctrl *qc)
>  		/* Did we reach the end of the control list? */
>  		if (id >= node2id(hdl->ctrl_refs.prev)) {
>  			ref = NULL; /* Yes, so there is no next control */
> -		} else if (ref) {
> -			/* We found a control with the given ID, so just get
> -			   the next one in the list. */
> -			ref = list_entry(ref->node.next, typeof(*ref), node);
>  		} else {
> -			/* No control with the given ID exists, so start
> -			   searching for the next largest ID. We know there
> -			   is one, otherwise the first 'if' above would have
> -			   been true. */
> -			list_for_each_entry(ref, &hdl->ctrl_refs, node)
> -				if (id < ref->ctrl->id)
> +			/* If no ref was found, then start searching from the
> +			   beginning of the ctrl_refs list. */
> +			if (ref == NULL)
> +				ref = list_entry(hdl->ctrl_refs.next,
> +						typeof(*ref), node);
> +			/* Search for the next largest ID. */
> +			list_for_each_entry_from(ref, &hdl->ctrl_refs, node)
> +				if (ref->ctrl->is_enabled && id < ref->ctrl->id)
>  					break;
>  		}
>  	}
> @@ -1468,8 +1509,6 @@ static int prepare_ext_ctrls(struct v4l2_ctrl_handler *hdl,
>  		ctrl = v4l2_ctrl_find(hdl, id);
>  		if (ctrl == NULL)
>  			return -EINVAL;
> -		if (ctrl->flags & V4L2_CTRL_FLAG_DISABLED)
> -			return -EINVAL;
>  
>  		helpers[i].ctrl = ctrl;
>  		helpers[i].handled = false;
> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> index 97d0638..a2b2d58 100644
> --- a/include/media/v4l2-ctrls.h
> +++ b/include/media/v4l2-ctrls.h
> @@ -65,6 +65,8 @@ struct v4l2_ctrl_ops {
>    *		control's current value cannot be cached and needs to be
>    *		retrieved through the g_volatile_ctrl op. Drivers can set
>    *		this flag.
> +  * @is_enabled: If 0, then this control is disabled and will be hidden for
> +  *		applications. Controls are always enabled by default.
>    * @ops:	The control ops.
>    * @id:	The control ID.
>    * @name:	The control name.
> @@ -105,6 +107,7 @@ struct v4l2_ctrl {
>  	unsigned int is_new:1;
>  	unsigned int is_private:1;
>  	unsigned int is_volatile:1;
> +	unsigned int is_enabled:1;
>  
>  	const struct v4l2_ctrl_ops *ops;
>  	u32 id;
> @@ -399,6 +402,37 @@ void v4l2_ctrl_activate(struct v4l2_ctrl *ctrl, bool active);
>    */
>  void v4l2_ctrl_grab(struct v4l2_ctrl *ctrl, bool grabbed);
>  
> +/** v4l2_ctrl_enable() - Mark the control as enabled or disabled.
> +  * @ctrl:	The control to en/disable.
> +  * @enabled:	True if the control should become enabled.
> +  *
> +  * Enable/disable a control.
> +  * Does nothing if @ctrl == NULL.
> +  * Usually called if controls are to be enabled or disabled when changing
> +  * to a different input or output.
> +  *
> +  * When a control is disabled, then it will no longer show up in the
> +  * application.
> +  *
> +  * This function can be called regardless of whether the control handler
> +  * is locked or not.
> +  */
> +void v4l2_ctrl_enable(struct v4l2_ctrl *ctrl, bool enabled);
> +
> +/** v4l2_ctrl_handler_enable() - Mark the controls in the handler as enabled or disabled.
> +  * @hdl:	The control handler.
> +  * @enabled:	True if the controls should become enabled.
> +  *
> +  * Enable/disable the controls owned by the handler.
> +  * Does nothing if @hdl == NULL.
> +  * Usually called if controls are to be enabled or disabled when changing
> +  * to a different input or output.
> +  *
> +  * When a control is disabled, then it will no longer show up in the
> +  * application.
> +  */
> +void v4l2_ctrl_handler_enable(struct v4l2_ctrl_handler *hdl, bool enabled);
> +
>  /** v4l2_ctrl_lock() - Helper function to lock the handler
>    * associated with the control.
>    * @ctrl:	The control to lock.

Re: [RFC PATCH 1/3] v4l2-ctrls: must be able to enable/disable controls

[Hans Verkuil]

On Saturday, January 22, 2011 17:11:12 Andy Walls wrote:
> On Sat, 2011-01-22 at 12:05 +0100, Hans Verkuil wrote: 
> > Controls can be dependent on the chosen input/output. So it has to be possible
> > to enable or disable groups of controls, preventing them from being seen in
> > the application.
> 
> I'm not a human factors expert, but given some of the principles listed
> here:
> 
> 	http://www.asktog.com/basics/firstPrinciples.html
> 
> I get the feeling that forcing controls to disappear and reappear will
> not be good for every possible application UI design.

They already have to do that anyway since changing input might also mean
changing input standard, formats, etc.

I doubt many do that, though, since having different inputs with different
standard/formats/controls etc. is pretty rare. Although with embedded systems
this will become more common.
 
> I'm sure that due to subdevices providing all sorts of interesting
> controls, some method of disabling controls is needed so as not to show
> duplicates, etc.
> 
> However, would it be better to provide a hint to the application and let
> the application UI designer decide what gets shown; instead of the
> kernel dictating what is shown?  

An alternative might be to enumerate all controls but set the DISABLED flag
for disabled controls. But this is just half the story. For controls that are
common to multiple inputs you still need to update them since the min/max ranges
may have changed after you changed inputs.

That said, it may not be a bad idea to use the DISABLED flag. It does allow a UI
application to populate the control panel with all possible controls. However,
it would require a small change to the spec since right now it says this:

Table A.80. Control Flags

V4L2_CTRL_FLAG_DISABLED	0x0001	This control is permanently disabled and should
be ignored by the application. Any attempt to change the control will result in an
EINVAL error code.

That does not quite fit what the new situation will be.

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by Cisco

Re: [RFC PATCH 3/3] v4l2-ctrls: update control framework documentation

[Sylwester Nawrocki]

Hi Hans,

On 01/22/2011 08:06 PM, Hans Verkuil wrote:
> Document how to enable/disable controls.
> Document the new v4l2_ctrl_auto_cluster function.
> Document the practical method of using anonymous structs to 'cluster'
> controls instead of using cumbersome control pointer arrays.
> 
> Signed-off-by: Hans Verkuil<hverkuil@xs4all.nl>
> ---
>   Documentation/video4linux/v4l2-controls.txt |   94 +++++++++++++++++++++++++++
>   1 files changed, 94 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/video4linux/v4l2-controls.txt b/Documentation/video4linux/v4l2-controls.txt
> index 881e7f4..78b6674 100644
> --- a/Documentation/video4linux/v4l2-controls.txt
> +++ b/Documentation/video4linux/v4l2-controls.txt
> @@ -453,6 +453,25 @@ In the example above the following are equivalent for the VOLUME case:
>   	ctrl == ctrl->cluster[AUDIO_CL_VOLUME] == state->audio_cluster[AUDIO_CL_VOLUME]
>   	ctrl->cluster[AUDIO_CL_MUTE] == state->audio_cluster[AUDIO_CL_MUTE]
> 
> +In practice using cluster arrays like this becomes very tiresome. So instead
> +the following equivalent method is used:
> +
> +	struct {
> +		/* audio cluster */
> +		struct v4l2_ctrl *volume;
> +		struct v4l2_ctrl *mute;
> +	};
> +
> +The anonymous struct is used to clearly 'cluster' these two control pointers,
> +but it serves no other purpose. The effect is the same as creating an
> +array with two control pointers. So you can just do:
> +
> +	state->volume = v4l2_ctrl_new_std(&state->ctrl_handler, ...);
> +	state->mute = v4l2_ctrl_new_std(&state->ctrl_handler, ...);
> +	v4l2_ctrl_cluster(2,&state->volume);
> +
> +And in foo_s_ctrl you can use these pointers directly: state->mute->val.
> +
>   Note that controls in a cluster may be NULL. For example, if for some
>   reason mute was never added (because the hardware doesn't support that
>   particular feature), then mute will be NULL. So in that case we have a
> @@ -475,6 +494,55 @@ controls, then the 'is_new' flag would be 1 for both controls.
>   The 'is_new' flag is always 1 when called from v4l2_ctrl_handler_setup().
> 
> 
> +Handling autogain/gain-type Controls with Auto Clusters
> +=======================================================
> +
> +A common type of control cluster is one that handles 'auto-foo/foo'-type
> +controls. Typical examples are autogain/gain, autoexposure/exposure,
> +autowhitebalance/red balance/blue balance. In all cases you have one controls
> +that determines whether another control is handled automatically by the hardware,
> +or whether it is under manual control from the user.
> +
> +The way these are supposed to be handled is that if you set one of the 'foo'
> +controls, then the 'auto-foo' control should automatically switch to manual
> +mode, except when you set the 'auto-foo' control at the same time, in which

Do "set the 'auto-foo' control at the same time" refer to what is done in
a driver? I can't see how this statement could apply to userland.

> +case it will depend on the new 'auto-foo' setting whether the new 'foo' value
> +is actually ignored or set in the hardware.
> +
> +The reasoning is that if you explicitly set a manual control, then it makes
> +sense to assume that you want to switch to manual mode as well. Whereas if you
> +set both the auto and manual control at the same time, then you should follow
> +whatever the new value for the auto control is.
> +
> +Usually the 'foo' control is also volatile, since if the automatic mode is
> +enabled, then the reported value for 'foo' is the value that the automatic
> +mode has determined is the best at that given time. However, if manual mode
> +is selected, then it is just the last stored value. So g_volatile_ctrl should
> +only be called when we are in automatic mode.
> +
> +Finally the V4L2_CTRL_FLAG_UPDATE should also be set for the non-auto controls
> +since changing one of them might affect the auto control.
> +
> +In order to simplify this a special variation of v4l2_ctrl_cluster was
> +introduced:
> +
> +void v4l2_ctrl_auto_cluster(unsigned ncontrols, struct v4l2_ctrl **controls,
> +			u8 manual_val, bool set_volatile);
> +
> +The first two arguments are identical to v4l2_ctrl_cluster. The third argument
> +tells the framework which value switches the cluster into manual mode. The
> +last argument will optionally set is_volatile flag for the non-auto controls.
> +
> +The first control of the cluster is assumed to be the 'auto' control.
> +
> +Using this function will ensure that:
> +
> +- the right flags are set.
> +- when a 'foo' control is set explicitly the 'auto-foo' control is set to
> +  the manual mode before s_ctrl is called.

So it means that, if it is required to set multiple foo controls (atomically)
before disabling auto-foo control, the control clusters should not be used?

Regards,
Sylwester

Re: [RFC PATCH 3/3] v4l2-ctrls: update control framework documentation

[Kim HeungJun]

Hi Sylwester,

It's my humble opinions. Just read plz :)

2011. 1. 23., 오후 8:08, Sylwester Nawrocki 작성:

[snip]
 
>> +Handling autogain/gain-type Controls with Auto Clusters
>> +=======================================================
>> +
>> +A common type of control cluster is one that handles 'auto-foo/foo'-type
>> +controls. Typical examples are autogain/gain, autoexposure/exposure,
>> +autowhitebalance/red balance/blue balance. In all cases you have one controls
>> +that determines whether another control is handled automatically by the hardware,
>> +or whether it is under manual control from the user.
>> +
>> +The way these are supposed to be handled is that if you set one of the 'foo'
>> +controls, then the 'auto-foo' control should automatically switch to manual
>> +mode, except when you set the 'auto-foo' control at the same time, in which
> 
> Do "set the 'auto-foo' control at the same time" refer to what is done in
> a driver? I can't see how this statement could apply to userland.

If you are talking about how platform or user application can call CID twice in such specific
case, as you already know, yes, it happened. Actually, I have been faced the same
difficulty with platform and user application peoples. But, for now, this new control
framework seems to be easy handling the driver actions.

When the user controls the devices, the user application wants to call only one CID.
For example, when the user exposures camera device manually, they want to call
only V4L2_CID_EXPOSURE, not call additionally V4L2_CID_EXPOSURE_AUTO
setting the enumeration by V4L2_EXPOSURE_MANUAL.

IMHO, the ultimate reason seems that V4L2_CID_EXPOSURE is only responsible to
handle Manual Exposure, but the V4L2_CID_EXPOSURE_AUTO can influence Auto
and Manual case. The all CID having AUTO feature is likely to be the same case.
But, as I know, the previous driver using this CID having AUTO feature still exist now,
and it is very difficult to change the current control logic to handle devices. 

The birth of new v4l2_ctrl framework seems to be the better options I think, and
I'm very happy for that because it's ok not to be worry about how to set the
internal variables for matching the control sequence with platform and user any more.

Re: [RFC PATCH 2/3] v4l2-ctrls: add v4l2_ctrl_auto_cluster to simplify autogain/gain scenarios

[Hans de Goede]

Hi,

On 01/22/2011 12:06 PM, Hans Verkuil wrote:
> It is a bit tricky to handle autogain/gain type scenerios correctly. Such
> controls need to be clustered and the V4L2_CTRL_FLAG_UPDATE should be set on
> the non-auto controls. If you set a non-auto control without setting the
> auto control at the same time, then the auto control should switch to manual
> mode. And usually the non-auto controls must be marked volatile, but this should
> only be in effect if the auto control is set to auto.
>
> The chances of drivers doing all these things correctly are pretty remote.
> So a new v4l2_ctrl_auto_cluster function was added that takes care of these
> issues.

I like the concept of this, but I'm not so happy with the automatically put
the auto control in manual mode. We've discussed this before and iirc we
came to the conclusion then that the proper thing to do is to mark the
controls as read only, when the related auto control is in auto mode.

This is what the UVC spec for example mandates and what the current UVC driver
does. Combining this with an app which honors the update and the read only
flag (try gtk-v4l), results in a nice experience. User enables auto exposure
-> exposure control gets grayed out, put exposure back manual -> control
is ungrayed.

So this new auto_cluster behavior would be a behavioral change (for both the
uvc driver and several gspca drivers), and more over an unwanted one IMHO
setting one control should not change another as a side effect.

Regards,

Hans



>
> Signed-off-by: Hans Verkuil<hverkuil@xs4all.nl>
> ---
>   drivers/media/video/v4l2-ctrls.c |   27 ++++++++++++++++++++-
>   include/media/v4l2-ctrls.h       |   48 +++++++++++++++++++++++++++++++++++++-
>   2 files changed, 73 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/video/v4l2-ctrls.c b/drivers/media/video/v4l2-ctrls.c
> index 983e287..b5da617 100644
> --- a/drivers/media/video/v4l2-ctrls.c
> +++ b/drivers/media/video/v4l2-ctrls.c
> @@ -1178,6 +1178,25 @@ void v4l2_ctrl_cluster(unsigned ncontrols, struct v4l2_ctrl **controls)
>   }
>   EXPORT_SYMBOL(v4l2_ctrl_cluster);
>
> +void v4l2_ctrl_auto_cluster(unsigned ncontrols, struct v4l2_ctrl **controls,
> +			    u8 manual_val, bool set_volatile)
> +{
> +	int i;
> +
> +	v4l2_ctrl_cluster(ncontrols, controls);
> +	controls[0]->is_auto = true;
> +	controls[0]->manual_mode_value = manual_val;
> +
> +	for (i = 1; i<  ncontrols; i++) {
> +		if (controls[i]) {
> +			controls[i]->flags |= V4L2_CTRL_FLAG_UPDATE;
> +			if (set_volatile)
> +				controls[i]->is_volatile = true;
> +		}
> +	}
> +}
> +EXPORT_SYMBOL(v4l2_ctrl_auto_cluster);
> +
>   /* Activate/deactivate a control. */
>   void v4l2_ctrl_activate(struct v4l2_ctrl *ctrl, bool active)
>   {
> @@ -1605,7 +1624,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)
> +		if (ctrl->is_volatile&&  master->ops->g_volatile_ctrl&&
> +		    (!master->is_auto || master->cur.val != master->manual_mode_value))
>   			ret = master->ops->g_volatile_ctrl(master);
>   		/* If OK, then copy the current control values to the caller */
>   		if (!ret)
> @@ -1717,6 +1737,11 @@ static int try_or_set_control_cluster(struct v4l2_ctrl *master, bool set)
>
>   	/* Don't set if there is no change */
>   	if (!ret&&  set&&  cluster_changed(master)) {
> +		if (master->is_auto&&  !master->is_new&&
> +		    master->cur.val != master->manual_mode_value) {
> +			master->is_new = 1;
> +			master->val = master->manual_mode_value;
> +		}
>   		ret = master->ops->s_ctrl(master);
>   		/* If OK, then make the new values permanent. */
>   		if (!ret)
> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> index a2b2d58..e715f4e 100644
> --- a/include/media/v4l2-ctrls.h
> +++ b/include/media/v4l2-ctrls.h
> @@ -66,7 +66,16 @@ struct v4l2_ctrl_ops {
>     *		retrieved through the g_volatile_ctrl op. Drivers can set
>     *		this flag.
>     * @is_enabled: If 0, then this control is disabled and will be hidden for
> -  *		applications. Controls are always enabled by default.
> +  *		applications. Controls are always enabled by default. Drivers
> +  *		should never set this flag.
> +  * @is_auto:   If set, then this control selects whether the other cluster
> +  *		members are in 'automatic' mode or 'manual' mode. This is
> +  *		used for autogain/gain type clusters. Drivers should never
> +  *		set this flag.
> +  * @manual_mode_value: If the is_auto flag is set, then this is the value
> +  *		of the auto control that determines if that control is in
> +  *		manual mode. So if the value of the auto control equals this
> +  *		value, then the whole cluster is in manual mode.
>     * @ops:	The control ops.
>     * @id:	The control ID.
>     * @name:	The control name.
> @@ -108,6 +117,8 @@ struct v4l2_ctrl {
>   	unsigned int is_private:1;
>   	unsigned int is_volatile:1;
>   	unsigned int is_enabled:1;
> +	unsigned int is_auto:1;
> +	unsigned int manual_mode_value:5;
>
>   	const struct v4l2_ctrl_ops *ops;
>   	u32 id;
> @@ -366,6 +377,41 @@ int v4l2_ctrl_add_handler(struct v4l2_ctrl_handler *hdl,
>   void v4l2_ctrl_cluster(unsigned ncontrols, struct v4l2_ctrl **controls);
>
>
> +/** v4l2_ctrl_auto_cluster() - Mark all controls in the cluster as belonging to
> +  * that cluster and set it up for autofoo/foo-type handling.
> +  * @ncontrols:	The number of controls in this cluster.
> +  * @controls:	The cluster control array of size @ncontrols. The first control
> +  *		must be the 'auto' control (e.g. autogain, autoexposure, etc.)
> +  * @manual_val: The value for the first control in the cluster that equals the
> +  *		manual setting.
> +  * @set_volatile: If true, then all controls except the first auto control will
> +  *		have is_volatile set to true. If false, then is_volatile will not
> +  *		be touched.
> +  *
> +  * Use for control groups where one control selects some automatic feature and
> +  * the other controls are only active whenever the automatic feature is turned
> +  * off (manual mode). Typical examples: autogain vs gain, auto-whitebalance vs
> +  * red and blue balance, etc.
> +  *
> +  * Using this function lets the framework take care of some of the actions
> +  * related to such controls: whenever one of the manual controls are set
> +  * without touching the auto control, then the auto control is set to
> +  * manual mode. So in the s_ctrl op you can just set the new values. Also,
> +  * g_volatile_ctrl is never called when in manual mode, since in that case
> +  * the current control value can just be returned as is.
> +  *
> +  * This function also makes it easy to set all non-auto controls to volatile
> +  * by setting the last argument to true. In most cases non-auto controls are
> +  * volatile when in auto mode. E.g. if autogain is set, then the gain register
> +  * usually reports the current automatically selected gain.
> +  *
> +  * This function also sets the V4L2_CTRL_FLAG_UPDATE for all the non-auto
> +  * controls.
> +  */
> +void v4l2_ctrl_auto_cluster(unsigned ncontrols, struct v4l2_ctrl **controls,
> +			u8 manual_val, bool set_volatile);
> +
> +
>   /** v4l2_ctrl_find() - Find a control with the given ID.
>     * @hdl:	The control handler.
>     * @id:	The control ID to find.

Re: [RFC PATCH 2/3] v4l2-ctrls: add v4l2_ctrl_auto_cluster to simplify autogain/gain scenarios

[Hans Verkuil]

On Sunday, January 23, 2011 16:15:03 Hans de Goede wrote:
> Hi,
> 
> On 01/22/2011 12:06 PM, Hans Verkuil wrote:
> > It is a bit tricky to handle autogain/gain type scenerios correctly. Such
> > controls need to be clustered and the V4L2_CTRL_FLAG_UPDATE should be set on
> > the non-auto controls. If you set a non-auto control without setting the
> > auto control at the same time, then the auto control should switch to manual
> > mode. And usually the non-auto controls must be marked volatile, but this should
> > only be in effect if the auto control is set to auto.
> >
> > The chances of drivers doing all these things correctly are pretty remote.
> > So a new v4l2_ctrl_auto_cluster function was added that takes care of these
> > issues.
> 
> I like the concept of this, but I'm not so happy with the automatically put
> the auto control in manual mode. We've discussed this before and iirc we
> came to the conclusion then that the proper thing to do is to mark the
> controls as read only, when the related auto control is in auto mode.
> 
> This is what the UVC spec for example mandates and what the current UVC driver
> does. Combining this with an app which honors the update and the read only
> flag (try gtk-v4l), results in a nice experience. User enables auto exposure
> -> exposure control gets grayed out, put exposure back manual -> control
> is ungrayed.
> 
> So this new auto_cluster behavior would be a behavioral change (for both the
> uvc driver and several gspca drivers), and more over an unwanted one IMHO
> setting one control should not change another as a side effect.

Actually, I've been converting a whole list of subdev drivers recently (soc_camera,
ov7670) and they all behaved like this. So I didn't change anything.

There is nothing preventing other drivers from doing something different.

That said, changing the behavior to your proposal may not be such a bad idea.
But then I need the OK from all driver authors involved, since this would
mean a change of behavior for them.

The good news is that once they use the new framework function I only need
to change what that function does and I don't need to change any of those
drivers.

So I will proceed for now by converting those drivers to use this new function,
and at the same time I can contact the authors and ask what their opinion is
of this change. I'm hoping for more feedback as well from what others think.

BTW, if I understand the gspca code correctly then it seems that if an e.g.
autogain control is set to 1, then the gain control effectively disappears.
I think queryctrl will just never return it. That can't be right.

I tried to test it but while my zc3xx-based webcam has autogain there is
interestingly enough no corresponding manual gain control.

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by Cisco

Re: [RFC PATCH 2/3] v4l2-ctrls: add v4l2_ctrl_auto_cluster to simplify autogain/gain scenarios

[Hans de Goede]

Hi,

On 01/23/2011 05:13 PM, Hans Verkuil wrote:
> On Sunday, January 23, 2011 16:15:03 Hans de Goede wrote:

<snip>

>> This is what the UVC spec for example mandates and what the current UVC driver
>> does. Combining this with an app which honors the update and the read only
>> flag (try gtk-v4l), results in a nice experience. User enables auto exposure
>> ->  exposure control gets grayed out, put exposure back manual ->  control
>> is ungrayed.
>>
>> So this new auto_cluster behavior would be a behavioral change (for both the
>> uvc driver and several gspca drivers), and more over an unwanted one IMHO
>> setting one control should not change another as a side effect.
>
> Actually, I've been converting a whole list of subdev drivers recently (soc_camera,
> ov7670) and they all behaved like this. So I didn't change anything.

Hmm, interesting.

> There is nothing preventing other drivers from doing something different.
>
> That said, changing the behavior to your proposal may not be such a bad idea.

Yes and AFAIK this is what we agreed on when we discussed auto control a
couple of months ago.

> But then I need the OK from all driver authors involved, since this would
> mean a change of behavior for them.
>
> The good news is that once they use the new framework function I only need
> to change what that function does and I don't need to change any of those
> drivers.
>
> So I will proceed for now by converting those drivers to use this new function,
> and at the same time I can contact the authors and ask what their opinion is
> of this change. I'm hoping for more feedback as well from what others think.
>

Yes, contacting the authors to discuss this further sounds like a good idea.

> BTW, if I understand the gspca code correctly then it seems that if an e.g.
> autogain control is set to 1, then the gain control effectively disappears.
> I think queryctrl will just never return it. That can't be right.

Erm, it should not disappear, but just get disabled. But this may have
(accidentally) changed with the drivers which were converted to the new
control framework. Anyways, we should discuss this with involved driver
authors and agree on a standard way to handle this. Once we have agreement
on how to handle this converting the drivers should be relatively easy.

Regards,

Hans

Re: [RFC PATCH 2/3] v4l2-ctrls: add v4l2_ctrl_auto_cluster to simplify autogain/gain scenarios

[Hans Verkuil]

On Tuesday, January 25, 2011 21:36:59 Hans de Goede wrote:
> Hi,
> 
> On 01/23/2011 05:13 PM, Hans Verkuil wrote:
> > On Sunday, January 23, 2011 16:15:03 Hans de Goede wrote:
> 
> <snip>
> 
> >> This is what the UVC spec for example mandates and what the current UVC driver
> >> does. Combining this with an app which honors the update and the read only
> >> flag (try gtk-v4l), results in a nice experience. User enables auto exposure
> >> ->  exposure control gets grayed out, put exposure back manual ->  control
> >> is ungrayed.
> >>
> >> So this new auto_cluster behavior would be a behavioral change (for both the
> >> uvc driver and several gspca drivers), and more over an unwanted one IMHO
> >> setting one control should not change another as a side effect.
> >
> > Actually, I've been converting a whole list of subdev drivers recently (soc_camera,
> > ov7670) and they all behaved like this. So I didn't change anything.
> 
> Hmm, interesting.
> 
> > There is nothing preventing other drivers from doing something different.
> >
> > That said, changing the behavior to your proposal may not be such a bad idea.
> 
> Yes and AFAIK this is what we agreed on when we discussed auto control a
> couple of months ago.
> 
> > But then I need the OK from all driver authors involved, since this would
> > mean a change of behavior for them.
> >
> > The good news is that once they use the new framework function I only need
> > to change what that function does and I don't need to change any of those
> > drivers.
> >
> > So I will proceed for now by converting those drivers to use this new function,
> > and at the same time I can contact the authors and ask what their opinion is
> > of this change. I'm hoping for more feedback as well from what others think.
> >
> 
> Yes, contacting the authors to discuss this further sounds like a good idea.
> 
> > BTW, if I understand the gspca code correctly then it seems that if an e.g.
> > autogain control is set to 1, then the gain control effectively disappears.
> > I think queryctrl will just never return it. That can't be right.
> 
> Erm, it should not disappear, but just get disabled. But this may have
> (accidentally) changed with the drivers which were converted to the new
> control framework.

gspca doesn't use the control framework at all. Or are you talking about a
gspca-internal change in control handling?

> Anyways, we should discuss this with involved driver
> authors and agree on a standard way to handle this. Once we have agreement
> on how to handle this converting the drivers should be relatively easy.

Yes, I'll continue working on this.

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by Cisco