* [RFC PATCH 0/3] v4l2-ctrls: add new functionality @ 2011-01-22 11:05 Hans Verkuil 2011-01-22 11:05 ` [RFC PATCH 1/3] v4l2-ctrls: must be able to enable/disable controls Hans Verkuil 0 siblings, 1 reply; 12+ messages in thread From: Hans Verkuil @ 2011-01-22 11:05 UTC (permalink / raw) To: linux-media 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* [RFC PATCH 1/3] v4l2-ctrls: must be able to enable/disable controls 2011-01-22 11:05 [RFC PATCH 0/3] v4l2-ctrls: add new functionality Hans Verkuil @ 2011-01-22 11:05 ` Hans Verkuil 2011-01-22 11:06 ` [RFC PATCH 2/3] v4l2-ctrls: add v4l2_ctrl_auto_cluster to simplify autogain/gain scenarios Hans Verkuil ` (2 more replies) 0 siblings, 3 replies; 12+ messages in thread From: Hans Verkuil @ 2011-01-22 11:05 UTC (permalink / raw) To: linux-media 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [RFC PATCH 2/3] v4l2-ctrls: add v4l2_ctrl_auto_cluster to simplify autogain/gain scenarios 2011-01-22 11:05 ` [RFC PATCH 1/3] v4l2-ctrls: must be able to enable/disable controls Hans Verkuil @ 2011-01-22 11:06 ` Hans Verkuil 2011-01-23 15:15 ` Hans de Goede 2011-01-22 11:06 ` [RFC PATCH 3/3] v4l2-ctrls: update control framework documentation Hans Verkuil 2011-01-22 16:11 ` [RFC PATCH 1/3] v4l2-ctrls: must be able to enable/disable controls Andy Walls 2 siblings, 1 reply; 12+ messages in thread From: Hans Verkuil @ 2011-01-22 11:06 UTC (permalink / raw) To: linux-media 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [RFC PATCH 2/3] v4l2-ctrls: add v4l2_ctrl_auto_cluster to simplify autogain/gain scenarios 2011-01-22 11:06 ` [RFC PATCH 2/3] v4l2-ctrls: add v4l2_ctrl_auto_cluster to simplify autogain/gain scenarios Hans Verkuil @ 2011-01-23 15:15 ` Hans de Goede 2011-01-23 16:13 ` Hans Verkuil 0 siblings, 1 reply; 12+ messages in thread From: Hans de Goede @ 2011-01-23 15:15 UTC (permalink / raw) To: Hans Verkuil; +Cc: linux-media 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC PATCH 2/3] v4l2-ctrls: add v4l2_ctrl_auto_cluster to simplify autogain/gain scenarios 2011-01-23 15:15 ` Hans de Goede @ 2011-01-23 16:13 ` Hans Verkuil 2011-01-25 20:36 ` Hans de Goede 0 siblings, 1 reply; 12+ messages in thread From: Hans Verkuil @ 2011-01-23 16:13 UTC (permalink / raw) To: Hans de Goede; +Cc: linux-media 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC PATCH 2/3] v4l2-ctrls: add v4l2_ctrl_auto_cluster to simplify autogain/gain scenarios 2011-01-23 16:13 ` Hans Verkuil @ 2011-01-25 20:36 ` Hans de Goede 2011-01-25 20:53 ` Hans Verkuil 0 siblings, 1 reply; 12+ messages in thread From: Hans de Goede @ 2011-01-25 20:36 UTC (permalink / raw) To: Hans Verkuil; +Cc: linux-media 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC PATCH 2/3] v4l2-ctrls: add v4l2_ctrl_auto_cluster to simplify autogain/gain scenarios 2011-01-25 20:36 ` Hans de Goede @ 2011-01-25 20:53 ` Hans Verkuil 0 siblings, 0 replies; 12+ messages in thread From: Hans Verkuil @ 2011-01-25 20:53 UTC (permalink / raw) To: Hans de Goede; +Cc: linux-media 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [RFC PATCH 3/3] v4l2-ctrls: update control framework documentation 2011-01-22 11:05 ` [RFC PATCH 1/3] v4l2-ctrls: must be able to enable/disable controls Hans Verkuil 2011-01-22 11:06 ` [RFC PATCH 2/3] v4l2-ctrls: add v4l2_ctrl_auto_cluster to simplify autogain/gain scenarios Hans Verkuil @ 2011-01-22 11:06 ` Hans Verkuil 2011-01-23 11:08 ` Sylwester Nawrocki 2011-01-22 16:11 ` [RFC PATCH 1/3] v4l2-ctrls: must be able to enable/disable controls Andy Walls 2 siblings, 1 reply; 12+ messages in thread From: Hans Verkuil @ 2011-01-22 11:06 UTC (permalink / raw) To: linux-media 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [RFC PATCH 3/3] v4l2-ctrls: update control framework documentation 2011-01-22 11:06 ` [RFC PATCH 3/3] v4l2-ctrls: update control framework documentation Hans Verkuil @ 2011-01-23 11:08 ` Sylwester Nawrocki 2011-01-23 14:53 ` Kim HeungJun 0 siblings, 1 reply; 12+ messages in thread From: Sylwester Nawrocki @ 2011-01-23 11:08 UTC (permalink / raw) To: Hans Verkuil; +Cc: linux-media 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC PATCH 3/3] v4l2-ctrls: update control framework documentation 2011-01-23 11:08 ` Sylwester Nawrocki @ 2011-01-23 14:53 ` Kim HeungJun 0 siblings, 0 replies; 12+ messages in thread From: Kim HeungJun @ 2011-01-23 14:53 UTC (permalink / raw) To: Sylwester Nawrocki; +Cc: Kim HeungJun, Hans Verkuil, linux-media 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC PATCH 1/3] v4l2-ctrls: must be able to enable/disable controls 2011-01-22 11:05 ` [RFC PATCH 1/3] v4l2-ctrls: must be able to enable/disable controls Hans Verkuil 2011-01-22 11:06 ` [RFC PATCH 2/3] v4l2-ctrls: add v4l2_ctrl_auto_cluster to simplify autogain/gain scenarios Hans Verkuil 2011-01-22 11:06 ` [RFC PATCH 3/3] v4l2-ctrls: update control framework documentation Hans Verkuil @ 2011-01-22 16:11 ` Andy Walls 2011-01-22 17:12 ` Hans Verkuil 2 siblings, 1 reply; 12+ messages in thread From: Andy Walls @ 2011-01-22 16:11 UTC (permalink / raw) To: Hans Verkuil; +Cc: linux-media 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC PATCH 1/3] v4l2-ctrls: must be able to enable/disable controls 2011-01-22 16:11 ` [RFC PATCH 1/3] v4l2-ctrls: must be able to enable/disable controls Andy Walls @ 2011-01-22 17:12 ` Hans Verkuil 0 siblings, 0 replies; 12+ messages in thread From: Hans Verkuil @ 2011-01-22 17:12 UTC (permalink / raw) To: Andy Walls; +Cc: linux-media 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2011-01-25 20:54 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-01-22 11:05 [RFC PATCH 0/3] v4l2-ctrls: add new functionality Hans Verkuil 2011-01-22 11:05 ` [RFC PATCH 1/3] v4l2-ctrls: must be able to enable/disable controls Hans Verkuil 2011-01-22 11:06 ` [RFC PATCH 2/3] v4l2-ctrls: add v4l2_ctrl_auto_cluster to simplify autogain/gain scenarios Hans Verkuil 2011-01-23 15:15 ` Hans de Goede 2011-01-23 16:13 ` Hans Verkuil 2011-01-25 20:36 ` Hans de Goede 2011-01-25 20:53 ` Hans Verkuil 2011-01-22 11:06 ` [RFC PATCH 3/3] v4l2-ctrls: update control framework documentation Hans Verkuil 2011-01-23 11:08 ` Sylwester Nawrocki 2011-01-23 14:53 ` Kim HeungJun 2011-01-22 16:11 ` [RFC PATCH 1/3] v4l2-ctrls: must be able to enable/disable controls Andy Walls 2011-01-22 17:12 ` Hans Verkuil
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.