Re: [PATCH] v4l2-ctrls: optimize type_ops for arrays

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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);
>>  };
>>  
> 



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux