Em 28-06-2011 03:08, Hans Verkuil escreveu:
> On Monday, June 27, 2011 22:57:34 Mauro Carvalho Chehab wrote:
>> Em 07-06-2011 12:05, Hans Verkuil escreveu:
>>> From: Hans Verkuil <hans.verkuil@xxxxxxxxx>
>>>
>>> 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 autofoo controls. In addition, the manual controls should be marked
>>> inactive when the automatic mode is on, and active when the manual mode is on.
>>> This also requires specialized volatile handling.
>>>
>>> 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 <hans.verkuil@xxxxxxxxx>
>>> ---
>>> drivers/media/video/v4l2-ctrls.c | 69 +++++++++++++++++++++++++++++++------
>>> include/media/v4l2-ctrls.h | 45 ++++++++++++++++++++++++
>>> 2 files changed, 102 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/media/video/v4l2-ctrls.c b/drivers/media/video/v4l2-ctrls.c
>>> index a46d5c1..c39ab0c 100644
>>> --- a/drivers/media/video/v4l2-ctrls.c
>>> +++ b/drivers/media/video/v4l2-ctrls.c
>>> @@ -39,6 +39,20 @@ struct ctrl_helper {
>>> bool handled;
>>> };
>>>
>>> +/* Small helper function to determine if the autocluster is set to manual
>>> + mode. In that case the is_volatile flag should be ignored. */
>>> +static bool is_cur_manual(const struct v4l2_ctrl *master)
>>> +{
>>> + return master->is_auto && master->cur.val == master->manual_mode_value;
>>> +}
>>> +
>>> +/* Same as above, but this checks the against the new value instead of the
>>> + current value. */
>>> +static bool is_new_manual(const struct v4l2_ctrl *master)
>>> +{
>>> + return master->is_auto && master->val == master->manual_mode_value;
>>> +}
>>> +
>>> /* Returns NULL or a character pointer array containing the menu for
>>> the given control ID. The pointer array ends with a NULL pointer.
>>> An empty string signifies a menu entry that is invalid. This allows
>>> @@ -643,7 +657,7 @@ static int ctrl_is_volatile(struct v4l2_ext_control *c,
>>> }
>>>
>>> /* Copy the new value to the current value. */
>>> -static void new_to_cur(struct v4l2_ctrl *ctrl)
>>> +static void new_to_cur(struct v4l2_ctrl *ctrl, bool update_inactive)
>>> {
>>> if (ctrl == NULL)
>>> return;
>>> @@ -659,6 +673,11 @@ static void new_to_cur(struct v4l2_ctrl *ctrl)
>>> ctrl->cur.val = ctrl->val;
>>> break;
>>> }
>>> + if (update_inactive) {
>>> + ctrl->flags &= ~V4L2_CTRL_FLAG_INACTIVE;
>>> + if (!is_cur_manual(ctrl->cluster[0]))
>>> + ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE;
>>> + }
>>> }
>>>
>>> /* Copy the current value to the new value */
>>> @@ -1166,7 +1185,7 @@ void v4l2_ctrl_cluster(unsigned ncontrols, struct v4l2_ctrl **controls)
>>> int i;
>>>
>>> /* The first control is the master control and it must not be NULL */
>>> - BUG_ON(controls[0] == NULL);
>>> + BUG_ON(ncontrols == 0 || controls[0] == NULL);
>>>
>>> for (i = 0; i < ncontrols; i++) {
>>> if (controls[i]) {
>>> @@ -1177,6 +1196,28 @@ 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)
>>> +{
>>> + struct v4l2_ctrl *master = controls[0];
>>> + u32 flag;
>>> + int i;
>>> +
>>> + v4l2_ctrl_cluster(ncontrols, controls);
>>> + WARN_ON(ncontrols <= 1);
>>> + master->is_auto = true;
>>> + master->manual_mode_value = manual_val;
>>
>> You'll have an overflow if manual_val is higher than 31 here.
>>
>>> + master->flags |= V4L2_CTRL_FLAG_UPDATE;
>>> + flag = is_cur_manual(master) ? 0 : V4L2_CTRL_FLAG_INACTIVE;
>>> +
>>> + for (i = 1; i < ncontrols; i++)
>>> + if (controls[i]) {
>>> + controls[i]->is_volatile = set_volatile;
>>> + controls[i]->flags |= flag;
>>> + }
>>> +}
>>> +EXPORT_SYMBOL(v4l2_ctrl_auto_cluster);
>>> +
>>> /* Activate/deactivate a control. */
>>> void v4l2_ctrl_activate(struct v4l2_ctrl *ctrl, bool active)
>>> {
>>> @@ -1595,7 +1636,7 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct v4l2_ext_controls *cs
>>> ctrl_is_volatile);
>>>
>>> /* g_volatile_ctrl will update the new control values */
>>> - if (has_volatiles) {
>>> + if (has_volatiles && !is_cur_manual(master)) {
>>> for (j = 0; j < master->ncontrols; j++)
>>> cur_to_new(master->cluster[j]);
>>> ret = call_op(master, g_volatile_ctrl);
>>> @@ -1633,7 +1674,7 @@ static int get_ctrl(struct v4l2_ctrl *ctrl, s32 *val)
>>>
>>> v4l2_ctrl_lock(master);
>>> /* g_volatile_ctrl will update the current control values */
>>> - if (ctrl->is_volatile) {
>>> + if (ctrl->is_volatile && !is_cur_manual(master)) {
>>> for (i = 0; i < master->ncontrols; i++)
>>> cur_to_new(master->cluster[i]);
>>> ret = call_op(master, g_volatile_ctrl);
>>> @@ -1678,6 +1719,7 @@ EXPORT_SYMBOL(v4l2_ctrl_g_ctrl);
>>> Must be called with ctrl->handler->lock held. */
>>> static int try_or_set_control_cluster(struct v4l2_ctrl *master, bool set)
>>> {
>>> + bool update_flag;
>>> bool try = !set;
>>> int ret = 0;
>>> int i;
>>> @@ -1717,14 +1759,17 @@ static int try_or_set_control_cluster(struct v4l2_ctrl *master, bool set)
>>> ret = call_op(master, try_ctrl);
>>>
>>> /* Don't set if there is no change */
>>> - if (!ret && set && cluster_changed(master)) {
>>> - ret = call_op(master, s_ctrl);
>>> - /* If OK, then make the new values permanent. */
>>> - if (!ret)
>>> - for (i = 0; i < master->ncontrols; i++)
>>> - new_to_cur(master->cluster[i]);
>>> - }
>>> - return ret;
>>> + if (ret || !set || !cluster_changed(master))
>>> + return ret;
>>> + ret = call_op(master, s_ctrl);
>>> + /* If OK, then make the new values permanent. */
>>> + if (ret)
>>> + return ret;
>>> +
>>> + update_flag = is_cur_manual(master) != is_new_manual(master);
>>> + for (i = 0; i < master->ncontrols; i++)
>>> + new_to_cur(master->cluster[i], update_flag && i > 0);
>>> + return 0;
>>> }
>>>
>>> /* Try or set controls. */
>>> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
>>> index 97d0638..56323e3 100644
>>> --- a/include/media/v4l2-ctrls.h
>>> +++ b/include/media/v4l2-ctrls.h
>>> @@ -65,6 +65,15 @@ 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_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 directly.
>>> + * @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. Drivers should
>>> + * never set this flag directly.
>>> * @ops: The control ops.
>>> * @id: The control ID.
>>> * @name: The control name.
>>> @@ -105,6 +114,8 @@ struct v4l2_ctrl {
>>> unsigned int is_new:1;
>>> unsigned int is_private:1;
>>> unsigned int is_volatile:1;
>>> + unsigned int is_auto:1;
>>> + unsigned int manual_mode_value:5;
>>
>> Why are you using 5 bits for it? It seems too arbitrary to limit it to 32, especially
>> since I failed to see any rationale for that in the documentation.
>
> This value is for a boolean or menu control. The menu control is currently
> limited to 32 values, hence the 5 bits. However, it is probably better to
> make it 8 bits and add a check above for whether the manual_mode_value >
> ctrl->maximum (or < ctrl->minimum while I'm at it).
>
> That's a lot more general.
Agreed.
>
>>>
>>> const struct v4l2_ctrl_ops *ops;
>>> u32 id;
>>> @@ -363,6 +374,40 @@ 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.
>>> + *
>>> + * The behavior of such controls is as follows:
>>> + *
>>> + * When the autofoo control is set to automatic, then any manual controls
>>> + * are set to inactive and any reads will call g_volatile_ctrl (if the control
>>> + * was marked volatile).
>>> + *
>>> + * When the autofoo control is set to manual, then any manual controls will
>>> + * be marked active, and any reads will just return the current value without
>>> + * going through g_volatile_ctrl.
>>> + *
>>> + * In addition, this function will set the V4L2_CTRL_FLAG_UPDATE flag
>>> + * on the autofoo control and V4L2_CTRL_FLAG_INACTIVE on the foo control(s)
>>> + * if autofoo is in auto mode.
>>> + */
>>> +void v4l2_ctrl_auto_cluster(unsigned ncontrols, struct v4l2_ctrl **controls,
>>> + u8 manual_val, bool set_volatile);
>>
>> If you're limiting it to 5 bits, why are you allowing 8 bits here?
>
> There is no u5?
Yes, but also there is not check for values >31
>
> Anyway, I'll change it to 8 bits.
It sounds the better approach for me.
Thanks!
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
