Hi Hans,
A few comments below.
On Sun, Sep 21, 2014 at 04:48:25PM +0200, Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@xxxxxxxxx>
>
> Sometimes you want to apply a value every time v4l2_ctrl_apply_store
> is called, and sometimes you want to apply that value only once.
>
> This adds support for that feature.
>
> Signed-off-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
> ---
> drivers/media/v4l2-core/v4l2-ctrls.c | 75 ++++++++++++++++++++++++++++--------
> drivers/media/v4l2-core/v4l2-ioctl.c | 14 +++----
> include/media/v4l2-ctrls.h | 12 ++++++
> 3 files changed, 79 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index e5dccf2..21560b0 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -1475,6 +1475,7 @@ static int ptr_to_user(struct v4l2_ext_control *c,
> static int cur_to_user(struct v4l2_ext_control *c,
> struct v4l2_ctrl *ctrl)
> {
> + c->flags = 0;
> return ptr_to_user(c, ctrl, ctrl->p_cur);
> }
>
> @@ -1482,8 +1483,13 @@ static int cur_to_user(struct v4l2_ext_control *c,
> static int store_to_user(struct v4l2_ext_control *c,
> struct v4l2_ctrl *ctrl, unsigned store)
> {
> + c->flags = 0;
> if (store == 0)
> return ptr_to_user(c, ctrl, ctrl->p_new);
> + if (test_bit(store - 1, ctrl->cluster[0]->ignore_store_after_use))
> + c->flags |= V4L2_EXT_CTRL_FL_IGN_STORE_AFTER_USE;
> + if (test_bit(store - 1, ctrl->cluster[0]->ignore_store))
> + c->flags |= V4L2_EXT_CTRL_FL_IGN_STORE;
> return ptr_to_user(c, ctrl, ctrl->p_stores[store - 1]);
> }
>
> @@ -1491,6 +1497,7 @@ static int store_to_user(struct v4l2_ext_control *c,
> static int new_to_user(struct v4l2_ext_control *c,
> struct v4l2_ctrl *ctrl)
> {
> + c->flags = 0;
> return ptr_to_user(c, ctrl, ctrl->p_new);
> }
>
> @@ -1546,6 +1553,8 @@ static int user_to_ptr(struct v4l2_ext_control *c,
> static int user_to_new(struct v4l2_ext_control *c,
> struct v4l2_ctrl *ctrl)
> {
> + ctrl->cluster[0]->new_ignore_store_after_use =
> + c->flags & V4L2_EXT_CTRL_FL_IGN_STORE_AFTER_USE;
> return user_to_ptr(c, ctrl, ctrl->p_new);
> }
>
> @@ -1597,8 +1606,11 @@ static void new_to_cur(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, u32 ch_flags)
> /* Helper function: copy the new control value to the store */
> static void new_to_store(struct v4l2_ctrl *ctrl)
> {
> + if (ctrl == NULL)
> + return;
> +
> /* has_changed is set by cluster_changed */
> - if (ctrl && ctrl->has_changed)
> + if (ctrl->has_changed)
> ptr_to_ptr(ctrl, ctrl->p_new, ctrl->p_stores[ctrl->store - 1]);
> }
>
> @@ -2328,6 +2340,12 @@ void v4l2_ctrl_cluster(unsigned ncontrols, struct v4l2_ctrl **controls)
>
> for (i = 0; i < ncontrols; i++) {
> if (controls[i]) {
> + /*
> + * All controls in a cluster must have the same
> + * V4L2_CTRL_FLAG_CAN_STORE flag value.
> + */
> + WARN_ON((controls[0]->flags & controls[i]->flags) &
> + V4L2_CTRL_FLAG_CAN_STORE);
> controls[i]->cluster = controls;
> controls[i]->ncontrols = ncontrols;
> if (controls[i]->flags & V4L2_CTRL_FLAG_VOLATILE)
> @@ -2850,6 +2868,10 @@ static int extend_store(struct v4l2_ctrl *ctrl, unsigned stores)
> unsigned s, idx;
> union v4l2_ctrl_ptr *p;
>
> + /* round up to the next multiple of 4 */
> + stores = (stores + 3) & ~3;
You said it, round_up(). :-)
The comment becomes redundant as a result, too.
The above may also overflow.
> + if (stores > V4L2_CTRLS_MAX_STORES)
> + return -EINVAL;
> p = kcalloc(stores, sizeof(union v4l2_ctrl_ptr), GFP_KERNEL);
> if (p == NULL)
> return -ENOMEM;
> @@ -2868,6 +2890,7 @@ static int extend_store(struct v4l2_ctrl *ctrl, unsigned stores)
> memcpy(p, ctrl->p_stores, ctrl->nr_of_stores * sizeof(union v4l2_ctrl_ptr));
> kfree(ctrl->p_stores);
> ctrl->p_stores = p;
> + bitmap_set(ctrl->ignore_store, ctrl->nr_of_stores, stores - ctrl->nr_of_stores);
> ctrl->nr_of_stores = stores;
> return 0;
> }
> @@ -3081,21 +3104,33 @@ static int try_or_set_cluster(struct v4l2_fh *fh, struct v4l2_ctrl *master,
>
> ret = call_op(master, try_ctrl);
>
> - /* Don't set if there is no change */
> - if (ret || !set || !cluster_changed(master))
> - return ret;
> - ret = call_op(master, s_ctrl);
> - if (ret)
> + if (ret || !set)
> return ret;
>
> - /* If OK, then make the new values permanent. */
> - update_flag = is_cur_manual(master) != is_new_manual(master);
> - for (i = 0; i < master->ncontrols; i++) {
> - if (store)
> - new_to_store(master->cluster[i]);
> + /* Don't set if there is no change */
> + if (cluster_changed(master)) {
> + ret = call_op(master, s_ctrl);
> + if (ret)
> + return ret;
> +
> + /* If OK, then make the new values permanent. */
> + update_flag = is_cur_manual(master) != is_new_manual(master);
> + for (i = 0; i < master->ncontrols; i++) {
> + if (store)
> + new_to_store(master->cluster[i]);
> + else
> + new_to_cur(fh, master->cluster[i], ch_flags |
> + ((update_flag && i > 0) ?
> + V4L2_EVENT_CTRL_CH_FLAGS : 0));
> + }
> + }
> +
> + if (store) {
> + if (master->new_ignore_store_after_use)
> + set_bit(store - 1, master->ignore_store_after_use);
> else
> - new_to_cur(fh, master->cluster[i], ch_flags |
> - ((update_flag && i > 0) ? V4L2_EVENT_CTRL_CH_FLAGS : 0));
> + clear_bit(store - 1, master->ignore_store_after_use);
> + clear_bit(store - 1, master->ignore_store);
How about allowing the user to forget a control in store as well?
> }
> return 0;
> }
> @@ -3401,8 +3436,18 @@ int v4l2_ctrl_apply_store(struct v4l2_ctrl_handler *hdl, unsigned store)
> continue;
> if (master->handler != hdl)
> v4l2_ctrl_lock(master);
> - for (i = 0; i < master->ncontrols; i++)
> - store_to_new(master->cluster[i], store);
> + for (i = 0; i < master->ncontrols; i++) {
> + struct v4l2_ctrl *ctrl = master->cluster[i];
> +
> + if (!ctrl || (store && test_bit(store - 1, master->ignore_store)))
> + continue;
> + store_to_new(ctrl, store);
> + }
> +
> + if (store && !test_bit(store - 1, master->ignore_store)) {
> + if (test_bit(store - 1, master->ignore_store_after_use))
How about:
if (store && test_bit() && test_bit())
> + set_bit(store - 1, master->ignore_store);
> + }
>
> /* For volatile autoclusters that are currently in auto mode
> we need to discover if it will be set to manual mode.
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
> index 628852c..9d3b4f2 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -562,12 +562,14 @@ static void v4l_print_ext_controls(const void *arg, bool write_only)
> pr_cont("class=0x%x, count=%d, error_idx=%d",
> p->ctrl_class, p->count, p->error_idx);
> for (i = 0; i < p->count; i++) {
> - if (p->controls[i].size)
> - pr_cont(", id/val=0x%x/0x%x",
> - p->controls[i].id, p->controls[i].value);
> + if (!p->controls[i].size)
> + pr_cont(", id/flags/val=0x%x/0x%x/0x%x",
> + p->controls[i].id, p->controls[i].flags,
> + p->controls[i].value);
> else
> - pr_cont(", id/size=0x%x/%u",
> - p->controls[i].id, p->controls[i].size);
> + pr_cont(", id/flags/size=0x%x/0x%x/%u",
> + p->controls[i].id, p->controls[i].flags,
> + p->controls[i].size);
> }
> pr_cont("\n");
> }
> @@ -888,8 +890,6 @@ static int check_ext_ctrls(struct v4l2_ext_controls *c, int allow_priv)
>
> /* zero the reserved fields */
> c->reserved[0] = c->reserved[1] = 0;
> - for (i = 0; i < c->count; i++)
> - c->controls[i].reserved2[0] = 0;
>
> /* V4L2_CID_PRIVATE_BASE cannot be used as control class
> when using extended controls.
> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> index 713980a..3005d88 100644
> --- a/include/media/v4l2-ctrls.h
> +++ b/include/media/v4l2-ctrls.h
> @@ -36,6 +36,9 @@ struct v4l2_subscribed_event;
> struct v4l2_fh;
> struct poll_table_struct;
>
> +/* Must be a multiple of 4 */
> +#define V4L2_CTRLS_MAX_STORES VIDEO_MAX_FRAME
> +
> /** union v4l2_ctrl_ptr - A pointer to a control value.
> * @p_s32: Pointer to a 32-bit signed value.
> * @p_s64: Pointer to a 64-bit signed value.
> @@ -123,6 +126,8 @@ typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv);
> * @call_notify: If set, then call the handler's notify function whenever the
> * control's value changes.
> * @can_store: If set, then this control supports configuration stores.
> + * @new_ignore_store_after_use: If set, then the new control had the
> + * V4L2_EXT_CTRL_FL_IGN_STORE_AFTER_USE flag set.
> * @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
> @@ -143,6 +148,10 @@ typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv);
> * @nr_of_dims:The number of dimensions in @dims.
> * @nr_of_stores: The number of allocated configuration stores of this control.
> * @store: The configuration store that the control op operates on.
> + * @ignore_store: If the bit for the corresponding store is 1, then don't apply that
> + * store's value.
> + * @ignore_store_after_use: If the bit for the corresponding store is 1, then set the
> + * bit in @ignore_store after the store's value has been applied.
> * @menu_skip_mask: The control's skip mask for menu controls. This makes it
> * easy to skip menu items that are not valid. If bit X is set,
> * then menu item X is skipped. Of course, this only works for
> @@ -183,6 +192,7 @@ struct v4l2_ctrl {
> unsigned int has_volatiles:1;
> unsigned int call_notify:1;
> unsigned int can_store:1;
> + unsigned int new_ignore_store_after_use:1;
> unsigned int manual_mode_value:8;
>
> const struct v4l2_ctrl_ops *ops;
> @@ -197,6 +207,8 @@ struct v4l2_ctrl {
> u32 nr_of_dims;
> u16 nr_of_stores;
> u16 store;
> + DECLARE_BITMAP(ignore_store, V4L2_CTRLS_MAX_STORES);
> + DECLARE_BITMAP(ignore_store_after_use, V4L2_CTRLS_MAX_STORES);
I'd store this information next to the value itself. The reason is that
stores are typically accessed one at a time, and thus keeping data related
to a single store in a single contiguous location reduces cache misses.
> union {
> u64 step;
> u64 menu_skip_mask;
--
Kind regards,
Sakari Ailus
e-mail: sakari.ailus@xxxxxx XMPP: sailus@xxxxxxxxxxxxxx
--
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
