On 31/07/2022 23:38, Laurent Pinchart wrote:
> 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 ?
That would break the ABI. I remember that when I first created array support
I went back and forth between initializing the unset part of the array, or
keeping the existing values. In the end I chose to initialize it. It is really
use-case dependent, so if this becomes a real issue, then we need a flag or
something to indicate that we want to keep the old values.
Setting only part of an array rarely makes sense, esp. for 2 or more dimensional
arrays, so I'm not going to change this until there is a real use case.
Regards,
Hans
>
> 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);
>> };
>>
>
