On 02/12/14 11:55, Ricardo Ribalda Delgado wrote:
> Hello Hans
>
> If we have a matrix control with a huge number of elements (ie, the
> number of pixels), a control validation will imply over ahuge number
> of function calls.
>
> In the case of the matrix you are always validating/init all the
> values of of the array. Why dont we modify the type_ops so they
> operate on the whole matrix (remove the idx from the function).
I actually considered that, but rejected it (for now) because I want
to see a realistic use-case first. If you get such large matrices, then
you have to ask yourself whether the control framework is the right
approach anyway, and if it is, whether you want to do these validations
in the first place.
Also note that validations typically will still have to be done on a
per-element basis (so pushing down the loop over all elements into the
validation function will have little effect) and ditto for the more
complex comparisons. Only if you are able to do something like memcmp
to compare two matrices will it become beneficial to optimize this.
I see this as a possible future performance enhancement and before doing
that I need to see a good use-case and also see some performence measurements.
> Anyway we should modify
>
> for (idx = 0; idx < ctrl->rows * ctrl->cols; idx++)
> ctrl_changed |= !ctrl->type_ops->equal(ctrl, idx,
> ctrl->stores[0], ctrl->new);
>
> to
>
> for (idx = 0; idx < ctrl->rows * ctrl->cols; idx++)
> if (!ctrl->type_ops->equal(ctrl, idx, ctrl->stores[0],
> ctrl->new))
> break
>
> if (idx==ctrl->rows * ctrl->col)
> ctrol_changed=true;
>
> Saving us some calls when we already know that the control has changed.
Oops! Absolutely, thanks for pointing this out. That's a left-over from
older code.
I'll post a patch for this.
Regards,
Hans
>
>
> Thanks!
>
>
> ps: consider all my code pseudocode :P
>
>
> On Mon, Feb 10, 2014 at 9:46 AM, Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
>> From: Hans Verkuil <hans.verkuil@xxxxxxxxx>
>>
>> Since complex controls can have non-standard types we need to be able to do
>> type-specific checks etc. In order to make that easy type operations are added.
>> There are four operations:
>>
>> - equal: check if two values are equal
>> - init: initialize a value
>> - log: log the value
>> - validate: validate a new value
>>
>> This patch uses the v4l2_ctrl_ptr union for the first time.
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
>> Reviewed-by: Sylwester Nawrocki <s.nawrocki@xxxxxxxxxxx>
>> ---
>> drivers/media/v4l2-core/v4l2-ctrls.c | 267 ++++++++++++++++++++++-------------
>> include/media/v4l2-ctrls.h | 21 +++
>> 2 files changed, 190 insertions(+), 98 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
>> index 67e5d1e..988a2bd8 100644
>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
>> @@ -1132,6 +1132,149 @@ static void send_event(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, u32 changes)
>> v4l2_event_queue_fh(sev->fh, &ev);
>> }
>>
>> +static bool std_equal(const struct v4l2_ctrl *ctrl,
>> + union v4l2_ctrl_ptr ptr1,
>> + union v4l2_ctrl_ptr ptr2)
>> +{
>> + switch (ctrl->type) {
>> + case V4L2_CTRL_TYPE_BUTTON:
>> + return false;
>> + case V4L2_CTRL_TYPE_STRING:
>> + /* strings are always 0-terminated */
>> + return !strcmp(ptr1.p_char, ptr2.p_char);
>> + case V4L2_CTRL_TYPE_INTEGER64:
>> + return *ptr1.p_s64 == *ptr2.p_s64;
>> + default:
>> + if (ctrl->is_ptr)
>> + return !memcmp(ptr1.p, ptr2.p, ctrl->elem_size);
>> + return *ptr1.p_s32 == *ptr2.p_s32;
>> + }
>> +}
>> +
>> +static void std_init(const struct v4l2_ctrl *ctrl,
>> + union v4l2_ctrl_ptr ptr)
>> +{
>> + switch (ctrl->type) {
>> + case V4L2_CTRL_TYPE_STRING:
>> + memset(ptr.p_char, ' ', ctrl->minimum);
>> + ptr.p_char[ctrl->minimum] = '\0';
>> + break;
>> + case V4L2_CTRL_TYPE_INTEGER64:
>> + *ptr.p_s64 = ctrl->default_value;
>> + break;
>> + case V4L2_CTRL_TYPE_INTEGER:
>> + case V4L2_CTRL_TYPE_INTEGER_MENU:
>> + case V4L2_CTRL_TYPE_MENU:
>> + case V4L2_CTRL_TYPE_BITMASK:
>> + case V4L2_CTRL_TYPE_BOOLEAN:
>> + *ptr.p_s32 = ctrl->default_value;
>> + break;
>> + default:
>> + break;
>> + }
>> +}
>> +
>> +static void std_log(const struct v4l2_ctrl *ctrl)
>> +{
>> + union v4l2_ctrl_ptr ptr = ctrl->stores[0];
>> +
>> + switch (ctrl->type) {
>> + case V4L2_CTRL_TYPE_INTEGER:
>> + pr_cont("%d", *ptr.p_s32);
>> + break;
>> + case V4L2_CTRL_TYPE_BOOLEAN:
>> + pr_cont("%s", *ptr.p_s32 ? "true" : "false");
>> + break;
>> + case V4L2_CTRL_TYPE_MENU:
>> + pr_cont("%s", ctrl->qmenu[*ptr.p_s32]);
>> + break;
>> + case V4L2_CTRL_TYPE_INTEGER_MENU:
>> + pr_cont("%lld", ctrl->qmenu_int[*ptr.p_s32]);
>> + break;
>> + case V4L2_CTRL_TYPE_BITMASK:
>> + pr_cont("0x%08x", *ptr.p_s32);
>> + break;
>> + case V4L2_CTRL_TYPE_INTEGER64:
>> + pr_cont("%lld", *ptr.p_s64);
>> + break;
>> + case V4L2_CTRL_TYPE_STRING:
>> + pr_cont("%s", ptr.p_char);
>> + break;
>> + default:
>> + pr_cont("unknown type %d", ctrl->type);
>> + break;
>> + }
>> +}
>> +
>> +/* Round towards the closest legal value */
>> +#define ROUND_TO_RANGE(val, offset_type, ctrl) \
>> +({ \
>> + offset_type offset; \
>> + val += (ctrl)->step / 2; \
>> + val = clamp_t(typeof(val), val, \
>> + (ctrl)->minimum, (ctrl)->maximum); \
>> + offset = (val) - (ctrl)->minimum; \
>> + offset = (ctrl)->step * (offset / (ctrl)->step); \
>> + val = (ctrl)->minimum + offset; \
>> + 0; \
>> +})
>> +
>> +/* Validate a new control */
>> +static int std_validate(const struct v4l2_ctrl *ctrl,
>> + union v4l2_ctrl_ptr ptr)
>> +{
>> + size_t len;
>> +
>> + switch (ctrl->type) {
>> + case V4L2_CTRL_TYPE_INTEGER:
>> + return ROUND_TO_RANGE(*ptr.p_s32, u32, ctrl);
>> + case V4L2_CTRL_TYPE_INTEGER64:
>> + return ROUND_TO_RANGE(*ptr.p_s64, u64, ctrl);
>> +
>> + case V4L2_CTRL_TYPE_BOOLEAN:
>> + *ptr.p_s32 = !!*ptr.p_s32;
>> + return 0;
>> +
>> + case V4L2_CTRL_TYPE_MENU:
>> + case V4L2_CTRL_TYPE_INTEGER_MENU:
>> + if (*ptr.p_s32 < ctrl->minimum || *ptr.p_s32 > ctrl->maximum)
>> + return -ERANGE;
>> + if (ctrl->menu_skip_mask & (1 << *ptr.p_s32))
>> + return -EINVAL;
>> + if (ctrl->type == V4L2_CTRL_TYPE_MENU &&
>> + ctrl->qmenu[*ptr.p_s32][0] == '\0')
>> + return -EINVAL;
>> + return 0;
>> +
>> + case V4L2_CTRL_TYPE_BITMASK:
>> + *ptr.p_s32 &= ctrl->maximum;
>> + return 0;
>> +
>> + case V4L2_CTRL_TYPE_BUTTON:
>> + case V4L2_CTRL_TYPE_CTRL_CLASS:
>> + *ptr.p_s32 = 0;
>> + return 0;
>> +
>> + case V4L2_CTRL_TYPE_STRING:
>> + len = strlen(ptr.p_char);
>> + if (len < ctrl->minimum)
>> + return -ERANGE;
>> + if ((len - ctrl->minimum) % ctrl->step)
>> + return -ERANGE;
>> + return 0;
>> +
>> + default:
>> + return -EINVAL;
>> + }
>> +}
>> +
>> +static const struct v4l2_ctrl_type_ops std_type_ops = {
>> + .equal = std_equal,
>> + .init = std_init,
>> + .log = std_log,
>> + .validate = std_validate,
>> +};
>> +
>> /* Helper function: copy the current control value back to the caller */
>> static int cur_to_user(struct v4l2_ext_control *c,
>> struct v4l2_ctrl *ctrl)
>> @@ -1315,21 +1458,7 @@ static int cluster_changed(struct v4l2_ctrl *master)
>>
>> if (ctrl == NULL)
>> continue;
>> - switch (ctrl->type) {
>> - case V4L2_CTRL_TYPE_BUTTON:
>> - /* Button controls are always 'different' */
>> - return 1;
>> - case V4L2_CTRL_TYPE_STRING:
>> - /* strings are always 0-terminated */
>> - diff = strcmp(ctrl->string, ctrl->cur.string);
>> - break;
>> - case V4L2_CTRL_TYPE_INTEGER64:
>> - diff = ctrl->val64 != ctrl->cur.val64;
>> - break;
>> - default:
>> - diff = ctrl->val != ctrl->cur.val;
>> - break;
>> - }
>> + diff = !ctrl->type_ops->equal(ctrl, ctrl->stores[0], ctrl->new);
>> }
>> return diff;
>> }
>> @@ -1370,65 +1499,30 @@ static int check_range(enum v4l2_ctrl_type type,
>> }
>> }
>>
>> -/* Round towards the closest legal value */
>> -#define ROUND_TO_RANGE(val, offset_type, ctrl) \
>> -({ \
>> - offset_type offset; \
>> - val += (ctrl)->step / 2; \
>> - val = clamp_t(typeof(val), val, \
>> - (ctrl)->minimum, (ctrl)->maximum); \
>> - offset = (val) - (ctrl)->minimum; \
>> - offset = (ctrl)->step * (offset / (ctrl)->step); \
>> - val = (ctrl)->minimum + offset; \
>> - 0; \
>> -})
>> -
>> /* Validate a new control */
>> static int validate_new(const struct v4l2_ctrl *ctrl,
>> struct v4l2_ext_control *c)
>> {
>> - size_t len;
>> + union v4l2_ctrl_ptr ptr;
>>
>> switch (ctrl->type) {
>> case V4L2_CTRL_TYPE_INTEGER:
>> - return ROUND_TO_RANGE(*(s32 *)&c->value, u32, ctrl);
>> - case V4L2_CTRL_TYPE_INTEGER64:
>> - return ROUND_TO_RANGE(*(s64 *)&c->value64, u64, ctrl);
>> -
>> - case V4L2_CTRL_TYPE_BOOLEAN:
>> - c->value = !!c->value;
>> - return 0;
>> -
>> - case V4L2_CTRL_TYPE_MENU:
>> case V4L2_CTRL_TYPE_INTEGER_MENU:
>> - if (c->value < ctrl->minimum || c->value > ctrl->maximum)
>> - return -ERANGE;
>> - if (ctrl->menu_skip_mask & (1 << c->value))
>> - return -EINVAL;
>> - if (ctrl->type == V4L2_CTRL_TYPE_MENU &&
>> - ctrl->qmenu[c->value][0] == '\0')
>> - return -EINVAL;
>> - return 0;
>> -
>> + case V4L2_CTRL_TYPE_MENU:
>> case V4L2_CTRL_TYPE_BITMASK:
>> - c->value &= ctrl->maximum;
>> - return 0;
>> -
>> + case V4L2_CTRL_TYPE_BOOLEAN:
>> case V4L2_CTRL_TYPE_BUTTON:
>> case V4L2_CTRL_TYPE_CTRL_CLASS:
>> - c->value = 0;
>> - return 0;
>> + ptr.p_s32 = &c->value;
>> + return ctrl->type_ops->validate(ctrl, ptr);
>>
>> - case V4L2_CTRL_TYPE_STRING:
>> - len = strlen(c->string);
>> - if (len < ctrl->minimum)
>> - return -ERANGE;
>> - if ((len - ctrl->minimum) % ctrl->step)
>> - return -ERANGE;
>> - return 0;
>> + case V4L2_CTRL_TYPE_INTEGER64:
>> + ptr.p_s64 = &c->value64;
>> + return ctrl->type_ops->validate(ctrl, ptr);
>>
>> default:
>> - return -EINVAL;
>> + ptr.p = c->p;
>> + return ctrl->type_ops->validate(ctrl, ptr);
>> }
>> }
>>
>> @@ -1645,6 +1739,7 @@ unlock:
>> /* Add a new control */
>> static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
>> const struct v4l2_ctrl_ops *ops,
>> + const struct v4l2_ctrl_type_ops *type_ops,
>> u32 id, const char *name, const char *unit,
>> enum v4l2_ctrl_type type,
>> s64 min, s64 max, u64 step, s64 def,
>> @@ -1656,6 +1751,7 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
>> unsigned sz_extra;
>> void *data;
>> int err;
>> + int s;
>>
>> if (hdl->error)
>> return NULL;
>> @@ -1715,6 +1811,7 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
>> INIT_LIST_HEAD(&ctrl->ev_subs);
>> ctrl->handler = hdl;
>> ctrl->ops = ops;
>> + ctrl->type_ops = type_ops ? type_ops : &std_type_ops;
>> ctrl->id = id;
>> ctrl->name = name;
>> ctrl->unit = unit;
>> @@ -1736,19 +1833,16 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
>> ctrl->cur.val = ctrl->val = def;
>> data = &ctrl->stores[1];
>>
>> - if (ctrl->is_string) {
>> - ctrl->string = ctrl->new.p_char = data;
>> - ctrl->stores[0].p_char = data + elem_size;
>> -
>> - if (ctrl->minimum)
>> - memset(ctrl->cur.string, ' ', ctrl->minimum);
>> - } else if (ctrl->is_ptr) {
>> + if (ctrl->is_ptr) {
>> ctrl->p = ctrl->new.p = data;
>> ctrl->stores[0].p = data + elem_size;
>> } else {
>> ctrl->new.p = &ctrl->val;
>> ctrl->stores[0].p = &ctrl->cur.val;
>> }
>> + for (s = -1; s <= 0; s++)
>> + ctrl->type_ops->init(ctrl, ctrl->stores[s]);
>> +
>> if (handler_new_ref(hdl, ctrl)) {
>> kfree(ctrl);
>> return NULL;
>> @@ -1793,7 +1887,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_custom(struct v4l2_ctrl_handler *hdl,
>> return NULL;
>> }
>>
>> - ctrl = v4l2_ctrl_new(hdl, cfg->ops, cfg->id, name, unit,
>> + ctrl = v4l2_ctrl_new(hdl, cfg->ops, cfg->type_ops, cfg->id, name, unit,
>> type, min, max,
>> is_menu ? cfg->menu_skip_mask : step,
>> def, cfg->elem_size,
>> @@ -1821,7 +1915,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std(struct v4l2_ctrl_handler *hdl,
>> handler_set_err(hdl, -EINVAL);
>> return NULL;
>> }
>> - return v4l2_ctrl_new(hdl, ops, id, name, unit, type,
>> + return v4l2_ctrl_new(hdl, ops, NULL, id, name, unit, type,
>> min, max, step, def, 0,
>> flags, NULL, NULL, NULL);
>> }
>> @@ -1855,7 +1949,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu(struct v4l2_ctrl_handler *hdl,
>> handler_set_err(hdl, -EINVAL);
>> return NULL;
>> }
>> - return v4l2_ctrl_new(hdl, ops, id, name, unit, type,
>> + return v4l2_ctrl_new(hdl, ops, NULL, id, name, unit, type,
>> 0, max, mask, def, 0,
>> flags, qmenu, qmenu_int, NULL);
>> }
>> @@ -1888,7 +1982,8 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl,
>> handler_set_err(hdl, -EINVAL);
>> return NULL;
>> }
>> - return v4l2_ctrl_new(hdl, ops, id, name, unit, type, 0, max, mask, def,
>> + return v4l2_ctrl_new(hdl, ops, NULL, id, name, unit, type,
>> + 0, max, mask, def,
>> 0, flags, qmenu, NULL, NULL);
>>
>> }
>> @@ -1913,7 +2008,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl,
>> handler_set_err(hdl, -EINVAL);
>> return NULL;
>> }
>> - return v4l2_ctrl_new(hdl, ops, id, name, unit, type,
>> + return v4l2_ctrl_new(hdl, ops, NULL, id, name, unit, type,
>> 0, max, 0, def, 0,
>> flags, NULL, qmenu_int, NULL);
>> }
>> @@ -2096,32 +2191,8 @@ static void log_ctrl(const struct v4l2_ctrl *ctrl,
>>
>> pr_info("%s%s%s: ", prefix, colon, ctrl->name);
>>
>> - switch (ctrl->type) {
>> - case V4L2_CTRL_TYPE_INTEGER:
>> - pr_cont("%d", ctrl->cur.val);
>> - break;
>> - case V4L2_CTRL_TYPE_BOOLEAN:
>> - pr_cont("%s", ctrl->cur.val ? "true" : "false");
>> - break;
>> - case V4L2_CTRL_TYPE_MENU:
>> - pr_cont("%s", ctrl->qmenu[ctrl->cur.val]);
>> - break;
>> - case V4L2_CTRL_TYPE_INTEGER_MENU:
>> - pr_cont("%lld", ctrl->qmenu_int[ctrl->cur.val]);
>> - break;
>> - case V4L2_CTRL_TYPE_BITMASK:
>> - pr_cont("0x%08x", ctrl->cur.val);
>> - break;
>> - case V4L2_CTRL_TYPE_INTEGER64:
>> - pr_cont("%lld", ctrl->cur.val64);
>> - break;
>> - case V4L2_CTRL_TYPE_STRING:
>> - pr_cont("%s", ctrl->cur.string);
>> - break;
>> - default:
>> - pr_cont("unknown type %d", ctrl->type);
>> - break;
>> - }
>> + ctrl->type_ops->log(ctrl);
>> +
>> if (ctrl->flags & (V4L2_CTRL_FLAG_INACTIVE |
>> V4L2_CTRL_FLAG_GRABBED |
>> V4L2_CTRL_FLAG_VOLATILE)) {
>> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
>> index 515c1ba..aaf7333 100644
>> --- a/include/media/v4l2-ctrls.h
>> +++ b/include/media/v4l2-ctrls.h
>> @@ -67,6 +67,23 @@ struct v4l2_ctrl_ops {
>> int (*s_ctrl)(struct v4l2_ctrl *ctrl);
>> };
>>
>> +/** struct v4l2_ctrl_type_ops - The control type operations that the driver has to provide.
>> + * @equal: return true if both values are equal.
>> + * @init: initialize the value.
>> + * @log: log the value.
>> + * @validate: validate the value. Return 0 on success and a negative value otherwise.
>> + */
>> +struct v4l2_ctrl_type_ops {
>> + bool (*equal)(const struct v4l2_ctrl *ctrl,
>> + union v4l2_ctrl_ptr ptr1,
>> + union v4l2_ctrl_ptr ptr2);
>> + void (*init)(const struct v4l2_ctrl *ctrl,
>> + union v4l2_ctrl_ptr ptr);
>> + void (*log)(const struct v4l2_ctrl *ctrl);
>> + int (*validate)(const struct v4l2_ctrl *ctrl,
>> + union v4l2_ctrl_ptr ptr);
>> +};
>> +
>> typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv);
>>
>> /** struct v4l2_ctrl - The control structure.
>> @@ -102,6 +119,7 @@ typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv);
>> * value, then the whole cluster is in manual mode. Drivers should
>> * never set this flag directly.
>> * @ops: The control ops.
>> + * @type_ops: The control type ops.
>> * @id: The control ID.
>> * @name: The control name.
>> * @unit: The control's unit. May be NULL.
>> @@ -151,6 +169,7 @@ struct v4l2_ctrl {
>> unsigned int manual_mode_value:8;
>>
>> const struct v4l2_ctrl_ops *ops;
>> + const struct v4l2_ctrl_type_ops *type_ops;
>> u32 id;
>> const char *name;
>> const char *unit;
>> @@ -234,6 +253,7 @@ struct v4l2_ctrl_handler {
>>
>> /** struct v4l2_ctrl_config - Control configuration structure.
>> * @ops: The control ops.
>> + * @type_ops: The control type ops. Only needed for complex controls.
>> * @id: The control ID.
>> * @name: The control name.
>> * @unit: The control's unit.
>> @@ -259,6 +279,7 @@ struct v4l2_ctrl_handler {
>> */
>> struct v4l2_ctrl_config {
>> const struct v4l2_ctrl_ops *ops;
>> + const struct v4l2_ctrl_type_ops *type_ops;
>> u32 id;
>> const char *name;
>> const char *unit;
>> --
>> 1.8.5.2
>>
>> --
>> 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
>
>
>
--
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
