Hi Hans,
Thank you for the patch.
On Thu, Jul 28, 2022 at 03:12:10PM +0200, Hans Verkuil wrote:
> Initializing arrays and validating or checking for equality of arrays
> is suboptimal since it does this per element.
>
> Change the ops to operate on the whole payload to speed up array
> operations.
>
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
> ---
> drivers/media/v4l2-core/v4l2-ctrls-api.c | 19 +---
> drivers/media/v4l2-core/v4l2-ctrls-core.c | 129 +++++++++++++++-------
> include/media/v4l2-ctrls.h | 6 +-
> 3 files changed, 96 insertions(+), 58 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-api.c b/drivers/media/v4l2-core/v4l2-ctrls-api.c
> index 67fbdccda2d8..a8c354ad3d23 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-api.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-api.c
> @@ -89,10 +89,7 @@ static int req_to_user(struct v4l2_ext_control *c,
> /* Helper function: copy the initial control value back to the caller */
> static int def_to_user(struct v4l2_ext_control *c, struct v4l2_ctrl *ctrl)
> {
> - int idx;
> -
> - for (idx = 0; idx < ctrl->elems; idx++)
> - ctrl->type_ops->init(ctrl, idx, ctrl->p_new);
> + ctrl->type_ops->init(ctrl, 0, ctrl->elems, ctrl->p_new);
>
> return ptr_to_user(c, ctrl, ctrl->p_new);
> }
> @@ -122,7 +119,6 @@ static int user_to_new(struct v4l2_ext_control *c, struct v4l2_ctrl *ctrl)
>
> if (ctrl->is_ptr && !ctrl->is_string) {
> unsigned int elems = c->size / ctrl->elem_size;
> - unsigned int idx;
>
> if (copy_from_user(ctrl->p_new.p, c->ptr, c->size))
> return -EFAULT;
> @@ -130,8 +126,7 @@ static int user_to_new(struct v4l2_ext_control *c, struct v4l2_ctrl *ctrl)
> if (ctrl->is_dyn_array)
> ctrl->new_elems = elems;
> else if (ctrl->is_array)
> - for (idx = elems; idx < ctrl->elems; idx++)
> - ctrl->type_ops->init(ctrl, idx, ctrl->p_new);
> + ctrl->type_ops->init(ctrl, elems, ctrl->elems, ctrl->p_new);
Is this initialization needed, can't the previous value of the elements
not set by userspace be kept ?
This isn't a problem introduced by this patch, so it should be fixed on
top of needed. For this,
Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> return 0;
> }
>
> @@ -499,12 +494,7 @@ EXPORT_SYMBOL(v4l2_g_ext_ctrls);
> /* Validate a new control */
> static int validate_new(const struct v4l2_ctrl *ctrl, union v4l2_ctrl_ptr p_new)
> {
> - unsigned int idx;
> - int err = 0;
> -
> - for (idx = 0; !err && idx < ctrl->new_elems; idx++)
> - err = ctrl->type_ops->validate(ctrl, idx, p_new);
> - return err;
> + return ctrl->type_ops->validate(ctrl, ctrl->new_elems, p_new);
> }
>
> /* Validate controls. */
> @@ -1017,8 +1007,7 @@ int __v4l2_ctrl_modify_dimensions(struct v4l2_ctrl *ctrl,
> ctrl->p_cur.p = p_array + elems * ctrl->elem_size;
> for (i = 0; i < ctrl->nr_of_dims; i++)
> ctrl->dims[i] = dims[i];
> - for (i = 0; i < elems; i++)
> - ctrl->type_ops->init(ctrl, i, ctrl->p_cur);
> + ctrl->type_ops->init(ctrl, 0, elems, ctrl->p_cur);
> cur_to_new(ctrl);
> send_event(NULL, ctrl, V4L2_EVENT_CTRL_CH_VALUE |
> V4L2_EVENT_CTRL_CH_DIMENSIONS);
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-core.c b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> index a004fea10da2..ff05171b3507 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-core.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-core.c
> @@ -65,31 +65,27 @@ 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, u32 idx,
> +static bool std_equal(const struct v4l2_ctrl *ctrl, u32 elems,
> union v4l2_ctrl_ptr ptr1,
> union v4l2_ctrl_ptr ptr2)
> {
> + unsigned int i;
> +
> switch (ctrl->type) {
> case V4L2_CTRL_TYPE_BUTTON:
> return false;
> case V4L2_CTRL_TYPE_STRING:
> - idx *= ctrl->elem_size;
> - /* strings are always 0-terminated */
> - return !strcmp(ptr1.p_char + idx, ptr2.p_char + idx);
> - case V4L2_CTRL_TYPE_INTEGER64:
> - return ptr1.p_s64[idx] == ptr2.p_s64[idx];
> - case V4L2_CTRL_TYPE_U8:
> - return ptr1.p_u8[idx] == ptr2.p_u8[idx];
> - case V4L2_CTRL_TYPE_U16:
> - return ptr1.p_u16[idx] == ptr2.p_u16[idx];
> - case V4L2_CTRL_TYPE_U32:
> - return ptr1.p_u32[idx] == ptr2.p_u32[idx];
> + for (i = 0; i < elems; i++) {
> + unsigned int idx = i * ctrl->elem_size;
> +
> + /* strings are always 0-terminated */
> + if (strcmp(ptr1.p_char + idx, ptr2.p_char + idx))
> + return false;
> + }
> + return true;
> default:
> - if (ctrl->is_int)
> - return ptr1.p_s32[idx] == ptr2.p_s32[idx];
> - idx *= ctrl->elem_size;
> - return !memcmp(ptr1.p_const + idx, ptr2.p_const + idx,
> - ctrl->elem_size);
> + return !memcmp(ptr1.p_const, ptr2.p_const,
> + elems * ctrl->elem_size);
> }
> }
>
> @@ -181,40 +177,66 @@ static void std_init_compound(const struct v4l2_ctrl *ctrl, u32 idx,
> }
> }
>
> -static void std_init(const struct v4l2_ctrl *ctrl, u32 idx,
> +static void std_init(const struct v4l2_ctrl *ctrl, u32 from_idx, u32 elems,
> union v4l2_ctrl_ptr ptr)
> {
> + unsigned int i;
> +
> switch (ctrl->type) {
> case V4L2_CTRL_TYPE_STRING:
> - idx *= ctrl->elem_size;
> - memset(ptr.p_char + idx, ' ', ctrl->minimum);
> - ptr.p_char[idx + ctrl->minimum] = '\0';
> + for (i = from_idx; i < elems; i++) {
> + unsigned int offset = i * ctrl->elem_size;
> +
> + memset(ptr.p_char + offset, ' ', ctrl->minimum);
> + ptr.p_char[offset + ctrl->minimum] = '\0';
> + }
> break;
> case V4L2_CTRL_TYPE_INTEGER64:
> - ptr.p_s64[idx] = ctrl->default_value;
> + if (ctrl->default_value) {
> + for (i = from_idx; i < elems; i++)
> + ptr.p_s64[i] = ctrl->default_value;
> + } else {
> + memset(ptr.p_s64 + from_idx, 0, elems * sizeof(s64));
> + }
> 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[idx] = ctrl->default_value;
> + if (ctrl->default_value) {
> + for (i = from_idx; i < elems; i++)
> + ptr.p_s32[i] = ctrl->default_value;
> + } else {
> + memset(ptr.p_s32 + from_idx, 0, elems * sizeof(s32));
> + }
> break;
> case V4L2_CTRL_TYPE_BUTTON:
> case V4L2_CTRL_TYPE_CTRL_CLASS:
> - ptr.p_s32[idx] = 0;
> + memset(ptr.p_s32 + from_idx, 0, elems * sizeof(s32));
> break;
> case V4L2_CTRL_TYPE_U8:
> - ptr.p_u8[idx] = ctrl->default_value;
> + memset(ptr.p_u8 + from_idx, ctrl->default_value, elems);
> break;
> case V4L2_CTRL_TYPE_U16:
> - ptr.p_u16[idx] = ctrl->default_value;
> + if (ctrl->default_value) {
> + for (i = from_idx; i < elems; i++)
> + ptr.p_u16[i] = ctrl->default_value;
> + } else {
> + memset(ptr.p_u16 + from_idx, 0, elems * sizeof(u16));
> + }
> break;
> case V4L2_CTRL_TYPE_U32:
> - ptr.p_u32[idx] = ctrl->default_value;
> + if (ctrl->default_value) {
> + for (i = from_idx; i < elems; i++)
> + ptr.p_u32[i] = ctrl->default_value;
> + } else {
> + memset(ptr.p_u32 + from_idx, 0, elems * sizeof(u32));
> + }
> break;
> default:
> - std_init_compound(ctrl, idx, ptr);
> + for (i = from_idx; i < elems; i++)
> + std_init_compound(ctrl, i, ptr);
> break;
> }
> }
> @@ -895,8 +917,8 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
> return 0;
> }
>
> -static int std_validate(const struct v4l2_ctrl *ctrl, u32 idx,
> - union v4l2_ctrl_ptr ptr)
> +static int std_validate_elem(const struct v4l2_ctrl *ctrl, u32 idx,
> + union v4l2_ctrl_ptr ptr)
> {
> size_t len;
> u64 offset;
> @@ -964,6 +986,38 @@ static int std_validate(const struct v4l2_ctrl *ctrl, u32 idx,
> default:
> return std_validate_compound(ctrl, idx, ptr);
> }
> +
> +}
> +
> +static int std_validate(const struct v4l2_ctrl *ctrl, u32 elems,
> + union v4l2_ctrl_ptr ptr)
> +{
> + unsigned int i;
> + int ret = 0;
> +
> + switch ((u32)ctrl->type) {
> + case V4L2_CTRL_TYPE_U8:
> + if (ctrl->maximum == 0xff && ctrl->minimum == 0 && ctrl->step == 1)
> + return 0;
> + break;
> + case V4L2_CTRL_TYPE_U16:
> + if (ctrl->maximum == 0xffff && ctrl->minimum == 0 && ctrl->step == 1)
> + return 0;
> + break;
> + case V4L2_CTRL_TYPE_U32:
> + if (ctrl->maximum == 0xffffffff && ctrl->minimum == 0 && ctrl->step == 1)
> + return 0;
> + break;
> +
> + case V4L2_CTRL_TYPE_BUTTON:
> + case V4L2_CTRL_TYPE_CTRL_CLASS:
> + memset(ptr.p_s32, 0, elems * sizeof(s32));
> + return 0;
> + }
> +
> + for (i = 0; !ret && i < elems; i++)
> + ret = std_validate_elem(ctrl, i, ptr);
> + return ret;
> }
>
> static const struct v4l2_ctrl_type_ops std_type_ops = {
> @@ -1449,7 +1503,6 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
> unsigned elems = 1;
> bool is_array;
> unsigned tot_ctrl_size;
> - unsigned idx;
> void *data;
> int err;
>
> @@ -1664,10 +1717,8 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
> memcpy(ctrl->p_def.p, p_def.p_const, elem_size);
> }
>
> - for (idx = 0; idx < elems; idx++) {
> - ctrl->type_ops->init(ctrl, idx, ctrl->p_cur);
> - ctrl->type_ops->init(ctrl, idx, ctrl->p_new);
> - }
> + ctrl->type_ops->init(ctrl, 0, elems, ctrl->p_cur);
> + cur_to_new(ctrl);
>
> if (handler_new_ref(hdl, ctrl, NULL, false, false)) {
> kvfree(ctrl->p_array);
> @@ -1984,7 +2035,6 @@ void update_from_auto_cluster(struct v4l2_ctrl *master)
> static int cluster_changed(struct v4l2_ctrl *master)
> {
> bool changed = false;
> - unsigned int idx;
> int i;
>
> for (i = 0; i < master->ncontrols; i++) {
> @@ -2010,10 +2060,9 @@ static int cluster_changed(struct v4l2_ctrl *master)
>
> if (ctrl->elems != ctrl->new_elems)
> ctrl_changed = true;
> -
> - for (idx = 0; !ctrl_changed && idx < ctrl->elems; idx++)
> - ctrl_changed = !ctrl->type_ops->equal(ctrl, idx,
> - ctrl->p_cur, ctrl->p_new);
> + if (!ctrl_changed)
> + ctrl_changed = !ctrl->type_ops->equal(ctrl,
> + ctrl->elems, ctrl->p_cur, ctrl->p_new);
> ctrl->has_changed = ctrl_changed;
> changed |= ctrl->has_changed;
> }
> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> index 607960309579..3bb1f0909c46 100644
> --- a/include/media/v4l2-ctrls.h
> +++ b/include/media/v4l2-ctrls.h
> @@ -128,13 +128,13 @@ struct v4l2_ctrl_ops {
> * otherwise.
> */
> struct v4l2_ctrl_type_ops {
> - bool (*equal)(const struct v4l2_ctrl *ctrl, u32 idx,
> + bool (*equal)(const struct v4l2_ctrl *ctrl, u32 elems,
> union v4l2_ctrl_ptr ptr1,
> union v4l2_ctrl_ptr ptr2);
> - void (*init)(const struct v4l2_ctrl *ctrl, u32 idx,
> + void (*init)(const struct v4l2_ctrl *ctrl, u32 from_idx, u32 elems,
> union v4l2_ctrl_ptr ptr);
> void (*log)(const struct v4l2_ctrl *ctrl);
> - int (*validate)(const struct v4l2_ctrl *ctrl, u32 idx,
> + int (*validate)(const struct v4l2_ctrl *ctrl, u32 elems,
> union v4l2_ctrl_ptr ptr);
> };
>
--
Regards,
Laurent Pinchart
