[PATCH v2] v4l2-ctrls: Add v4l2_ctrl_[gs]_ctrl_int64()

[PATCH v2] v4l2-ctrls: Add v4l2_ctrl_[gs]_ctrl_int64()

[Laurent Pinchart]

These helper functions get and set a 64-bit control's value from within
a driver. They are similar to v4l2_ctrl_[gs]_ctrl() but operate on
64-bit integer controls instead of 32-bit controls.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/video/v4l2-ctrls.c |  135 +++++++++++++++++++++++---------------
 include/media/v4l2-ctrls.h       |   23 +++++++
 2 files changed, 105 insertions(+), 53 deletions(-)

Changes since v1:

- Add v4l2_ctrl_g_ctrl_int64()
- Merge validate_new_int() and validate_new()
- Add a comment warning about string controls in set_ctrl()

diff --git a/drivers/media/video/v4l2-ctrls.c b/drivers/media/video/v4l2-ctrls.c
index 9abd9ab..3042895 100644
--- a/drivers/media/video/v4l2-ctrls.c
+++ b/drivers/media/video/v4l2-ctrls.c
@@ -1173,76 +1173,53 @@ static int cluster_changed(struct v4l2_ctrl *master)
 	return diff;
 }
 
-/* Validate integer-type control */
-static int validate_new_int(const struct v4l2_ctrl *ctrl, s32 *pval)
+/* Validate a new control */
+static int validate_new(const struct v4l2_ctrl *ctrl,
+			struct v4l2_ext_control *c)
 {
-	s32 val = *pval;
+	size_t len;
 	u32 offset;
+	s32 val;
 
 	switch (ctrl->type) {
 	case V4L2_CTRL_TYPE_INTEGER:
 		/* Round towards the closest legal value */
-		val += ctrl->step / 2;
-		if (val < ctrl->minimum)
-			val = ctrl->minimum;
-		if (val > ctrl->maximum)
-			val = ctrl->maximum;
+		val = c->value + ctrl->step / 2;
+		val = clamp(val, ctrl->minimum, ctrl->maximum);
 		offset = val - ctrl->minimum;
 		offset = ctrl->step * (offset / ctrl->step);
-		val = ctrl->minimum + offset;
-		*pval = val;
+		c->value = ctrl->minimum + offset;
 		return 0;
 
 	case V4L2_CTRL_TYPE_BOOLEAN:
-		*pval = !!val;
+		c->value = !!c->value;
 		return 0;
 
 	case V4L2_CTRL_TYPE_MENU:
 	case V4L2_CTRL_TYPE_INTEGER_MENU:
-		if (val < ctrl->minimum || val > ctrl->maximum)
+		if (c->value < ctrl->minimum || c->value > ctrl->maximum)
 			return -ERANGE;
-		if (ctrl->menu_skip_mask & (1 << val))
+		if (ctrl->menu_skip_mask & (1 << c->value))
 			return -EINVAL;
 		if (ctrl->type == V4L2_CTRL_TYPE_MENU &&
-		    ctrl->qmenu[val][0] == '\0')
+		    ctrl->qmenu[c->value][0] == '\0')
 			return -EINVAL;
 		return 0;
 
 	case V4L2_CTRL_TYPE_BITMASK:
-		*pval &= ctrl->maximum;
+		c->value &= ctrl->maximum;
 		return 0;
 
 	case V4L2_CTRL_TYPE_BUTTON:
 	case V4L2_CTRL_TYPE_CTRL_CLASS:
-		*pval = 0;
+		c->value = 0;
 		return 0;
 
-	default:
-		return -EINVAL;
-	}
-}
-
-/* Validate a new control */
-static int validate_new(const struct v4l2_ctrl *ctrl, struct v4l2_ext_control *c)
-{
-	char *s = c->string;
-	size_t len;
-
-	switch (ctrl->type) {
-	case V4L2_CTRL_TYPE_INTEGER:
-	case V4L2_CTRL_TYPE_BOOLEAN:
-	case V4L2_CTRL_TYPE_MENU:
-	case V4L2_CTRL_TYPE_INTEGER_MENU:
-	case V4L2_CTRL_TYPE_BITMASK:
-	case V4L2_CTRL_TYPE_BUTTON:
-	case V4L2_CTRL_TYPE_CTRL_CLASS:
-		return validate_new_int(ctrl, &c->value);
-
 	case V4L2_CTRL_TYPE_INTEGER64:
 		return 0;
 
 	case V4L2_CTRL_TYPE_STRING:
-		len = strlen(s);
+		len = strlen(c->string);
 		if (len < ctrl->minimum)
 			return -ERANGE;
 		if ((len - ctrl->minimum) % ctrl->step)
@@ -2234,12 +2211,19 @@ int v4l2_subdev_g_ext_ctrls(struct v4l2_subdev *sd, struct v4l2_ext_controls *cs
 EXPORT_SYMBOL(v4l2_subdev_g_ext_ctrls);
 
 /* Helper function to get a single control */
-static int get_ctrl(struct v4l2_ctrl *ctrl, s32 *val)
+static int get_ctrl(struct v4l2_ctrl *ctrl, struct v4l2_ext_control *c)
 {
 	struct v4l2_ctrl *master = ctrl->cluster[0];
 	int ret = 0;
 	int i;
 
+	/* String controls are not supported. The new_to_user() and
+	 * cur_to_user() calls below would need to be fixed not to access
+	 * userspace memory.
+	 */
+	if (ctrl->type == V4L2_CTRL_TYPE_STRING)
+		return -EINVAL;
+
 	if (ctrl->flags & V4L2_CTRL_FLAG_WRITE_ONLY)
 		return -EACCES;
 
@@ -2249,9 +2233,9 @@ static int get_ctrl(struct v4l2_ctrl *ctrl, s32 *val)
 		for (i = 0; i < master->ncontrols; i++)
 			cur_to_new(master->cluster[i]);
 		ret = call_op(master, g_volatile_ctrl);
-		*val = ctrl->val;
+		new_to_user(c, ctrl);
 	} else {
-		*val = ctrl->cur.val;
+		cur_to_user(c, ctrl);
 	}
 	v4l2_ctrl_unlock(master);
 	return ret;
@@ -2260,10 +2244,14 @@ static int get_ctrl(struct v4l2_ctrl *ctrl, s32 *val)
 int v4l2_g_ctrl(struct v4l2_ctrl_handler *hdl, struct v4l2_control *control)
 {
 	struct v4l2_ctrl *ctrl = v4l2_ctrl_find(hdl, control->id);
+	struct v4l2_ext_control c;
+	int ret;
 
 	if (ctrl == NULL || !type_is_int(ctrl))
 		return -EINVAL;
-	return get_ctrl(ctrl, &control->value);
+	ret = get_ctrl(ctrl, &c);
+	control->value = c.value;
+	return ret;
 }
 EXPORT_SYMBOL(v4l2_g_ctrl);
 
@@ -2275,15 +2263,28 @@ EXPORT_SYMBOL(v4l2_subdev_g_ctrl);
 
 s32 v4l2_ctrl_g_ctrl(struct v4l2_ctrl *ctrl)
 {
-	s32 val = 0;
+	struct v4l2_ext_control c;
 
 	/* It's a driver bug if this happens. */
 	WARN_ON(!type_is_int(ctrl));
-	get_ctrl(ctrl, &val);
-	return val;
+	c.value = 0;
+	get_ctrl(ctrl, &c);
+	return c.value;
 }
 EXPORT_SYMBOL(v4l2_ctrl_g_ctrl);
 
+s64 v4l2_ctrl_g_ctrl_int64(struct v4l2_ctrl *ctrl)
+{
+	struct v4l2_ext_control c;
+
+	/* It's a driver bug if this happens. */
+	WARN_ON(ctrl->type != V4L2_CTRL_TYPE_INTEGER64);
+	c.value = 0;
+	get_ctrl(ctrl, &c);
+	return c.value;
+}
+EXPORT_SYMBOL(v4l2_ctrl_g_ctrl_int64);
+
 
 /* Core function that calls try/s_ctrl and ensures that the new value is
    copied to the current value on a set.
@@ -2499,13 +2500,21 @@ int v4l2_subdev_s_ext_ctrls(struct v4l2_subdev *sd, struct v4l2_ext_controls *cs
 EXPORT_SYMBOL(v4l2_subdev_s_ext_ctrls);
 
 /* Helper function for VIDIOC_S_CTRL compatibility */
-static int set_ctrl(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, s32 *val)
+static int set_ctrl(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl,
+		    struct v4l2_ext_control *c)
 {
 	struct v4l2_ctrl *master = ctrl->cluster[0];
 	int ret;
 	int i;
 
-	ret = validate_new_int(ctrl, val);
+	/* String controls are not supported. The user_to_new() and
+	 * cur_to_user() calls below would need to be fixed not to access
+	 * userspace memory.
+	 */
+	if (ctrl->type == V4L2_CTRL_TYPE_STRING)
+		return -EINVAL;
+
+	ret = validate_new(ctrl, c);
 	if (ret)
 		return ret;
 
@@ -2520,12 +2529,13 @@ static int set_ctrl(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, s32 *val)
 	   manual mode we have to update the current volatile values since
 	   those will become the initial manual values after such a switch. */
 	if (master->is_auto && master->has_volatiles && ctrl == master &&
-	    !is_cur_manual(master) && *val == master->manual_mode_value)
+	    !is_cur_manual(master) && c->value == master->manual_mode_value)
 		update_from_auto_cluster(master);
-	ctrl->val = *val;
-	ctrl->is_new = 1;
+
+	user_to_new(c, ctrl);
 	ret = try_or_set_cluster(fh, master, true);
-	*val = ctrl->cur.val;
+	cur_to_user(c, ctrl);
+
 	v4l2_ctrl_unlock(ctrl);
 	return ret;
 }
@@ -2534,6 +2544,8 @@ int v4l2_s_ctrl(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
 					struct v4l2_control *control)
 {
 	struct v4l2_ctrl *ctrl = v4l2_ctrl_find(hdl, control->id);
+	struct v4l2_ext_control c;
+	int ret;
 
 	if (ctrl == NULL || !type_is_int(ctrl))
 		return -EINVAL;
@@ -2541,7 +2553,10 @@ int v4l2_s_ctrl(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
 	if (ctrl->flags & V4L2_CTRL_FLAG_READ_ONLY)
 		return -EACCES;
 
-	return set_ctrl(fh, ctrl, &control->value);
+	c.value = control->value;
+	ret = set_ctrl(fh, ctrl, &c);
+	control->value = c.value;
+	return ret;
 }
 EXPORT_SYMBOL(v4l2_s_ctrl);
 
@@ -2553,12 +2568,26 @@ EXPORT_SYMBOL(v4l2_subdev_s_ctrl);
 
 int v4l2_ctrl_s_ctrl(struct v4l2_ctrl *ctrl, s32 val)
 {
+	struct v4l2_ext_control c;
+
 	/* It's a driver bug if this happens. */
 	WARN_ON(!type_is_int(ctrl));
-	return set_ctrl(NULL, ctrl, &val);
+	c.value = val;
+	return set_ctrl(NULL, ctrl, &c);
 }
 EXPORT_SYMBOL(v4l2_ctrl_s_ctrl);
 
+int v4l2_ctrl_s_ctrl_int64(struct v4l2_ctrl *ctrl, s64 val)
+{
+	struct v4l2_ext_control c;
+
+	/* It's a driver bug if this happens. */
+	WARN_ON(ctrl->type != V4L2_CTRL_TYPE_INTEGER64);
+	c.value64 = val;
+	return set_ctrl(NULL, ctrl, &c);
+}
+EXPORT_SYMBOL(v4l2_ctrl_s_ctrl_int64);
+
 static int v4l2_ctrl_add_event(struct v4l2_subscribed_event *sev, unsigned elems)
 {
 	struct v4l2_ctrl *ctrl = v4l2_ctrl_find(sev->fh->ctrl_handler, sev->id);
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index 776605f..7ef6b27 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -511,6 +511,29 @@ s32 v4l2_ctrl_g_ctrl(struct v4l2_ctrl *ctrl);
   */
 int v4l2_ctrl_s_ctrl(struct v4l2_ctrl *ctrl, s32 val);
 
+/** v4l2_ctrl_g_ctrl_int64() - Helper function to get a 64-bit control's value from within a driver.
+  * @ctrl:	The control.
+  *
+  * This returns the control's value safely by going through the control
+  * framework. This function will lock the control's handler, so it cannot be
+  * used from within the &v4l2_ctrl_ops functions.
+  *
+  * This function is for 64-bit integer type controls only.
+  */
+s64 v4l2_ctrl_g_ctrl_int64(struct v4l2_ctrl *ctrl);
+
+/** v4l2_ctrl_s_ctrl_int64() - Helper function to set a 64-bit control's value from within a driver.
+  * @ctrl:	The control.
+  * @val:	The new value.
+  *
+  * This set the control's new value safely by going through the control
+  * framework. This function will lock the control's handler, so it cannot be
+  * used from within the &v4l2_ctrl_ops functions.
+  *
+  * This function is for 64-bit integer type controls only.
+  */
+int v4l2_ctrl_s_ctrl_int64(struct v4l2_ctrl *ctrl, s64 val);
+
 /* Internal helper functions that deal with control events. */
 extern const struct v4l2_subscribed_event_ops v4l2_ctrl_sub_ev_ops;
 void v4l2_ctrl_replace(struct v4l2_event *old, const struct v4l2_event *new);
-- 
Regards,

Laurent Pinchart

Re: [PATCH v2] v4l2-ctrls: Add v4l2_ctrl_[gs]_ctrl_int64()

[Hans Verkuil]

On Mon July 23 2012 16:02:40 Laurent Pinchart wrote:
> These helper functions get and set a 64-bit control's value from within
> a driver. They are similar to v4l2_ctrl_[gs]_ctrl() but operate on
> 64-bit integer controls instead of 32-bit controls.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/video/v4l2-ctrls.c |  135 +++++++++++++++++++++++---------------
>  include/media/v4l2-ctrls.h       |   23 +++++++
>  2 files changed, 105 insertions(+), 53 deletions(-)
> 
> Changes since v1:
> 
> - Add v4l2_ctrl_g_ctrl_int64()
> - Merge validate_new_int() and validate_new()
> - Add a comment warning about string controls in set_ctrl()
> 
> diff --git a/drivers/media/video/v4l2-ctrls.c b/drivers/media/video/v4l2-ctrls.c
> index 9abd9ab..3042895 100644
> --- a/drivers/media/video/v4l2-ctrls.c
> +++ b/drivers/media/video/v4l2-ctrls.c
> @@ -1173,76 +1173,53 @@ static int cluster_changed(struct v4l2_ctrl *master)
>  	return diff;
>  }
>  
> -/* Validate integer-type control */
> -static int validate_new_int(const struct v4l2_ctrl *ctrl, s32 *pval)
> +/* Validate a new control */
> +static int validate_new(const struct v4l2_ctrl *ctrl,
> +			struct v4l2_ext_control *c)
>  {
> -	s32 val = *pval;
> +	size_t len;
>  	u32 offset;
> +	s32 val;
>  
>  	switch (ctrl->type) {
>  	case V4L2_CTRL_TYPE_INTEGER:
>  		/* Round towards the closest legal value */
> -		val += ctrl->step / 2;
> -		if (val < ctrl->minimum)
> -			val = ctrl->minimum;
> -		if (val > ctrl->maximum)
> -			val = ctrl->maximum;
> +		val = c->value + ctrl->step / 2;
> +		val = clamp(val, ctrl->minimum, ctrl->maximum);
>  		offset = val - ctrl->minimum;
>  		offset = ctrl->step * (offset / ctrl->step);
> -		val = ctrl->minimum + offset;
> -		*pval = val;
> +		c->value = ctrl->minimum + offset;
>  		return 0;
>  
>  	case V4L2_CTRL_TYPE_BOOLEAN:
> -		*pval = !!val;
> +		c->value = !!c->value;
>  		return 0;
>  
>  	case V4L2_CTRL_TYPE_MENU:
>  	case V4L2_CTRL_TYPE_INTEGER_MENU:
> -		if (val < ctrl->minimum || val > ctrl->maximum)
> +		if (c->value < ctrl->minimum || c->value > ctrl->maximum)
>  			return -ERANGE;
> -		if (ctrl->menu_skip_mask & (1 << val))
> +		if (ctrl->menu_skip_mask & (1 << c->value))
>  			return -EINVAL;
>  		if (ctrl->type == V4L2_CTRL_TYPE_MENU &&
> -		    ctrl->qmenu[val][0] == '\0')
> +		    ctrl->qmenu[c->value][0] == '\0')
>  			return -EINVAL;
>  		return 0;
>  
>  	case V4L2_CTRL_TYPE_BITMASK:
> -		*pval &= ctrl->maximum;
> +		c->value &= ctrl->maximum;
>  		return 0;
>  
>  	case V4L2_CTRL_TYPE_BUTTON:
>  	case V4L2_CTRL_TYPE_CTRL_CLASS:
> -		*pval = 0;
> +		c->value = 0;
>  		return 0;
>  
> -	default:
> -		return -EINVAL;
> -	}
> -}
> -
> -/* Validate a new control */
> -static int validate_new(const struct v4l2_ctrl *ctrl, struct v4l2_ext_control *c)
> -{
> -	char *s = c->string;
> -	size_t len;
> -
> -	switch (ctrl->type) {
> -	case V4L2_CTRL_TYPE_INTEGER:
> -	case V4L2_CTRL_TYPE_BOOLEAN:
> -	case V4L2_CTRL_TYPE_MENU:
> -	case V4L2_CTRL_TYPE_INTEGER_MENU:
> -	case V4L2_CTRL_TYPE_BITMASK:
> -	case V4L2_CTRL_TYPE_BUTTON:
> -	case V4L2_CTRL_TYPE_CTRL_CLASS:
> -		return validate_new_int(ctrl, &c->value);
> -
>  	case V4L2_CTRL_TYPE_INTEGER64:
>  		return 0;
>  
>  	case V4L2_CTRL_TYPE_STRING:
> -		len = strlen(s);
> +		len = strlen(c->string);
>  		if (len < ctrl->minimum)
>  			return -ERANGE;
>  		if ((len - ctrl->minimum) % ctrl->step)
> @@ -2234,12 +2211,19 @@ int v4l2_subdev_g_ext_ctrls(struct v4l2_subdev *sd, struct v4l2_ext_controls *cs
>  EXPORT_SYMBOL(v4l2_subdev_g_ext_ctrls);
>  
>  /* Helper function to get a single control */
> -static int get_ctrl(struct v4l2_ctrl *ctrl, s32 *val)
> +static int get_ctrl(struct v4l2_ctrl *ctrl, struct v4l2_ext_control *c)
>  {
>  	struct v4l2_ctrl *master = ctrl->cluster[0];
>  	int ret = 0;
>  	int i;
>  
> +	/* String controls are not supported. The new_to_user() and
> +	 * cur_to_user() calls below would need to be fixed not to access
> +	 * userspace memory.

Just one small suggestion: change this comment to:

	/* String controls are not supported. The new_to_user() and
	 * cur_to_user() calls below would need to be modified not to access
	 * userspace memory when called from get_ctrl().
	 */

And a similar change in the comment with set_ctrl.

The word 'fixed' suggested that new_to_user etc. were broken, which isn't the
case. We are just using it in a special situation.

With the above change to get/set_ctrl you can add my

Acked-by: Hans Verkuil <hans.verkuil@cisco.com>

Regards,

	Hans

> +	 */
> +	if (ctrl->type == V4L2_CTRL_TYPE_STRING)
> +		return -EINVAL;
> +
>  	if (ctrl->flags & V4L2_CTRL_FLAG_WRITE_ONLY)
>  		return -EACCES;
>  
> @@ -2249,9 +2233,9 @@ static int get_ctrl(struct v4l2_ctrl *ctrl, s32 *val)
>  		for (i = 0; i < master->ncontrols; i++)
>  			cur_to_new(master->cluster[i]);
>  		ret = call_op(master, g_volatile_ctrl);
> -		*val = ctrl->val;
> +		new_to_user(c, ctrl);
>  	} else {
> -		*val = ctrl->cur.val;
> +		cur_to_user(c, ctrl);
>  	}
>  	v4l2_ctrl_unlock(master);
>  	return ret;
> @@ -2260,10 +2244,14 @@ static int get_ctrl(struct v4l2_ctrl *ctrl, s32 *val)
>  int v4l2_g_ctrl(struct v4l2_ctrl_handler *hdl, struct v4l2_control *control)
>  {
>  	struct v4l2_ctrl *ctrl = v4l2_ctrl_find(hdl, control->id);
> +	struct v4l2_ext_control c;
> +	int ret;
>  
>  	if (ctrl == NULL || !type_is_int(ctrl))
>  		return -EINVAL;
> -	return get_ctrl(ctrl, &control->value);
> +	ret = get_ctrl(ctrl, &c);
> +	control->value = c.value;
> +	return ret;
>  }
>  EXPORT_SYMBOL(v4l2_g_ctrl);
>  
> @@ -2275,15 +2263,28 @@ EXPORT_SYMBOL(v4l2_subdev_g_ctrl);
>  
>  s32 v4l2_ctrl_g_ctrl(struct v4l2_ctrl *ctrl)
>  {
> -	s32 val = 0;
> +	struct v4l2_ext_control c;
>  
>  	/* It's a driver bug if this happens. */
>  	WARN_ON(!type_is_int(ctrl));
> -	get_ctrl(ctrl, &val);
> -	return val;
> +	c.value = 0;
> +	get_ctrl(ctrl, &c);
> +	return c.value;
>  }
>  EXPORT_SYMBOL(v4l2_ctrl_g_ctrl);
>  
> +s64 v4l2_ctrl_g_ctrl_int64(struct v4l2_ctrl *ctrl)
> +{
> +	struct v4l2_ext_control c;
> +
> +	/* It's a driver bug if this happens. */
> +	WARN_ON(ctrl->type != V4L2_CTRL_TYPE_INTEGER64);
> +	c.value = 0;
> +	get_ctrl(ctrl, &c);
> +	return c.value;
> +}
> +EXPORT_SYMBOL(v4l2_ctrl_g_ctrl_int64);
> +
>  
>  /* Core function that calls try/s_ctrl and ensures that the new value is
>     copied to the current value on a set.
> @@ -2499,13 +2500,21 @@ int v4l2_subdev_s_ext_ctrls(struct v4l2_subdev *sd, struct v4l2_ext_controls *cs
>  EXPORT_SYMBOL(v4l2_subdev_s_ext_ctrls);
>  
>  /* Helper function for VIDIOC_S_CTRL compatibility */
> -static int set_ctrl(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, s32 *val)
> +static int set_ctrl(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl,
> +		    struct v4l2_ext_control *c)
>  {
>  	struct v4l2_ctrl *master = ctrl->cluster[0];
>  	int ret;
>  	int i;
>  
> -	ret = validate_new_int(ctrl, val);
> +	/* String controls are not supported. The user_to_new() and
> +	 * cur_to_user() calls below would need to be fixed not to access
> +	 * userspace memory.
> +	 */
> +	if (ctrl->type == V4L2_CTRL_TYPE_STRING)
> +		return -EINVAL;
> +
> +	ret = validate_new(ctrl, c);
>  	if (ret)
>  		return ret;
>  
> @@ -2520,12 +2529,13 @@ static int set_ctrl(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, s32 *val)
>  	   manual mode we have to update the current volatile values since
>  	   those will become the initial manual values after such a switch. */
>  	if (master->is_auto && master->has_volatiles && ctrl == master &&
> -	    !is_cur_manual(master) && *val == master->manual_mode_value)
> +	    !is_cur_manual(master) && c->value == master->manual_mode_value)
>  		update_from_auto_cluster(master);
> -	ctrl->val = *val;
> -	ctrl->is_new = 1;
> +
> +	user_to_new(c, ctrl);
>  	ret = try_or_set_cluster(fh, master, true);
> -	*val = ctrl->cur.val;
> +	cur_to_user(c, ctrl);
> +
>  	v4l2_ctrl_unlock(ctrl);
>  	return ret;
>  }
> @@ -2534,6 +2544,8 @@ int v4l2_s_ctrl(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
>  					struct v4l2_control *control)
>  {
>  	struct v4l2_ctrl *ctrl = v4l2_ctrl_find(hdl, control->id);
> +	struct v4l2_ext_control c;
> +	int ret;
>  
>  	if (ctrl == NULL || !type_is_int(ctrl))
>  		return -EINVAL;
> @@ -2541,7 +2553,10 @@ int v4l2_s_ctrl(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
>  	if (ctrl->flags & V4L2_CTRL_FLAG_READ_ONLY)
>  		return -EACCES;
>  
> -	return set_ctrl(fh, ctrl, &control->value);
> +	c.value = control->value;
> +	ret = set_ctrl(fh, ctrl, &c);
> +	control->value = c.value;
> +	return ret;
>  }
>  EXPORT_SYMBOL(v4l2_s_ctrl);
>  
> @@ -2553,12 +2568,26 @@ EXPORT_SYMBOL(v4l2_subdev_s_ctrl);
>  
>  int v4l2_ctrl_s_ctrl(struct v4l2_ctrl *ctrl, s32 val)
>  {
> +	struct v4l2_ext_control c;
> +
>  	/* It's a driver bug if this happens. */
>  	WARN_ON(!type_is_int(ctrl));
> -	return set_ctrl(NULL, ctrl, &val);
> +	c.value = val;
> +	return set_ctrl(NULL, ctrl, &c);
>  }
>  EXPORT_SYMBOL(v4l2_ctrl_s_ctrl);
>  
> +int v4l2_ctrl_s_ctrl_int64(struct v4l2_ctrl *ctrl, s64 val)
> +{
> +	struct v4l2_ext_control c;
> +
> +	/* It's a driver bug if this happens. */
> +	WARN_ON(ctrl->type != V4L2_CTRL_TYPE_INTEGER64);
> +	c.value64 = val;
> +	return set_ctrl(NULL, ctrl, &c);
> +}
> +EXPORT_SYMBOL(v4l2_ctrl_s_ctrl_int64);
> +
>  static int v4l2_ctrl_add_event(struct v4l2_subscribed_event *sev, unsigned elems)
>  {
>  	struct v4l2_ctrl *ctrl = v4l2_ctrl_find(sev->fh->ctrl_handler, sev->id);
> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> index 776605f..7ef6b27 100644
> --- a/include/media/v4l2-ctrls.h
> +++ b/include/media/v4l2-ctrls.h
> @@ -511,6 +511,29 @@ s32 v4l2_ctrl_g_ctrl(struct v4l2_ctrl *ctrl);
>    */
>  int v4l2_ctrl_s_ctrl(struct v4l2_ctrl *ctrl, s32 val);
>  
> +/** v4l2_ctrl_g_ctrl_int64() - Helper function to get a 64-bit control's value from within a driver.
> +  * @ctrl:	The control.
> +  *
> +  * This returns the control's value safely by going through the control
> +  * framework. This function will lock the control's handler, so it cannot be
> +  * used from within the &v4l2_ctrl_ops functions.
> +  *
> +  * This function is for 64-bit integer type controls only.
> +  */
> +s64 v4l2_ctrl_g_ctrl_int64(struct v4l2_ctrl *ctrl);
> +
> +/** v4l2_ctrl_s_ctrl_int64() - Helper function to set a 64-bit control's value from within a driver.
> +  * @ctrl:	The control.
> +  * @val:	The new value.
> +  *
> +  * This set the control's new value safely by going through the control
> +  * framework. This function will lock the control's handler, so it cannot be
> +  * used from within the &v4l2_ctrl_ops functions.
> +  *
> +  * This function is for 64-bit integer type controls only.
> +  */
> +int v4l2_ctrl_s_ctrl_int64(struct v4l2_ctrl *ctrl, s64 val);
> +
>  /* Internal helper functions that deal with control events. */
>  extern const struct v4l2_subscribed_event_ops v4l2_ctrl_sub_ev_ops;
>  void v4l2_ctrl_replace(struct v4l2_event *old, const struct v4l2_event *new);
> 

Re: [PATCH v2] v4l2-ctrls: Add v4l2_ctrl_[gs]_ctrl_int64()

[Laurent Pinchart]

Hi Hans,

On Monday 23 July 2012 17:05:35 Hans Verkuil wrote:
> On Mon July 23 2012 16:02:40 Laurent Pinchart wrote:
> > These helper functions get and set a 64-bit control's value from within
> > a driver. They are similar to v4l2_ctrl_[gs]_ctrl() but operate on
> > 64-bit integer controls instead of 32-bit controls.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

[snip]

> > -static int get_ctrl(struct v4l2_ctrl *ctrl, s32 *val)
> > +static int get_ctrl(struct v4l2_ctrl *ctrl, struct v4l2_ext_control *c)
> > 
> >  {
> >  
> >  	struct v4l2_ctrl *master = ctrl->cluster[0];
> >  	int ret = 0;
> >  	int i;
> > 
> > +	/* String controls are not supported. The new_to_user() and
> > +	 * cur_to_user() calls below would need to be fixed not to access
> > +	 * userspace memory.
> 
> Just one small suggestion: change this comment to:
> 
> 	/* String controls are not supported. The new_to_user() and
> 	 * cur_to_user() calls below would need to be modified not to access
> 	 * userspace memory when called from get_ctrl().
> 	 */
> 
> And a similar change in the comment with set_ctrl.
> 
> The word 'fixed' suggested that new_to_user etc. were broken, which isn't
> the case. We are just using it in a special situation.

OK. Fixed.

> With the above change to get/set_ctrl you can add my
> 
> Acked-by: Hans Verkuil <hans.verkuil@cisco.com>

Thank you. I'll resubmit the patch (along with a driver patch that uses it). 
Would you like to push it through your tree ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2] v4l2-ctrls: Add v4l2_ctrl_[gs]_ctrl_int64()

[Hans Verkuil]

On Mon July 23 2012 19:16:03 Laurent Pinchart wrote:
> Hi Hans,
> 
> On Monday 23 July 2012 17:05:35 Hans Verkuil wrote:
> > On Mon July 23 2012 16:02:40 Laurent Pinchart wrote:
> > > These helper functions get and set a 64-bit control's value from within
> > > a driver. They are similar to v4l2_ctrl_[gs]_ctrl() but operate on
> > > 64-bit integer controls instead of 32-bit controls.
> > > 
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> [snip]
> 
> > > -static int get_ctrl(struct v4l2_ctrl *ctrl, s32 *val)
> > > +static int get_ctrl(struct v4l2_ctrl *ctrl, struct v4l2_ext_control *c)
> > > 
> > >  {
> > >  
> > >  	struct v4l2_ctrl *master = ctrl->cluster[0];
> > >  	int ret = 0;
> > >  	int i;
> > > 
> > > +	/* String controls are not supported. The new_to_user() and
> > > +	 * cur_to_user() calls below would need to be fixed not to access
> > > +	 * userspace memory.
> > 
> > Just one small suggestion: change this comment to:
> > 
> > 	/* String controls are not supported. The new_to_user() and
> > 	 * cur_to_user() calls below would need to be modified not to access
> > 	 * userspace memory when called from get_ctrl().
> > 	 */
> > 
> > And a similar change in the comment with set_ctrl.
> > 
> > The word 'fixed' suggested that new_to_user etc. were broken, which isn't
> > the case. We are just using it in a special situation.
> 
> OK. Fixed.
> 
> > With the above change to get/set_ctrl you can add my
> > 
> > Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Thank you. I'll resubmit the patch (along with a driver patch that uses it). 
> Would you like to push it through your tree ?

You take it. I assume you have a driver that needs it, so it's better if it
goes in together with that driver.

Regards,

	Hans