[PATCH] v4l2-ctrl: fix setting volatile controls

[PATCH] v4l2-ctrl: fix setting volatile controls

[Hans Verkuil]

The V4L2 specification allows setting volatile controls as that is needed
if you want to be able to set all controls in one go using VIDIOC_S_EXT_CTRLS.

However, such new values should be ignored by the control framework
since it makes no sense to set a volatile control. While the new value
will be ignored anyway, it does generate a bogus 'change value' control event
that should be suppressed.

This patch changes the code to skip setting volatile controls, except for
one particular case where an autocluster switches to manual mode, because
that causes the volatile controls to become non-volatile, so the new
specified values should be retained.

Note that the values returned by VIDIOC_S_CTRL and VIDIOC_S_EXT_CTRLS for
such skipped volatile controls will be the currently cached values and not
the latest volatile value. This is something that might have to be fixed
as well in the future should that be necessary. I think it is overkill,
though.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Cc: pawel@osciak.com
---
 drivers/media/v4l2-core/v4l2-ctrls.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index fccd08b..a7cd830 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -2592,6 +2592,7 @@ static int try_set_ext_ctrls(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
 		cs->error_idx = cs->count;
 	for (i = 0; !ret && i < cs->count; i++) {
 		struct v4l2_ctrl *master;
+		bool set_volatiles = false;
 		u32 idx = i;
 
 		if (helpers[i].mref == NULL)
@@ -2627,14 +2628,24 @@ static int try_set_ext_ctrls(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
 			} while (tmp_idx);
 			/* If the new value == the manual value, then copy
 			   the current volatile values. */
-			if (new_auto_val == master->manual_mode_value)
+			if (new_auto_val == master->manual_mode_value) {
 				update_from_auto_cluster(master);
+				set_volatiles = true;
+			}
 		}
 
 		/* Copy the new caller-supplied control values.
 		   user_to_new() sets 'is_new' to 1. */
 		do {
-			ret = user_to_new(cs->controls + idx, helpers[idx].ctrl);
+			/*
+			 * Skip attempts to set volatile controls since those are
+			 * ignored anyway. The exception is when an autocluster is
+			 * switched to manual mode, since in that case the specified
+			 * 'volatile' controls are actually the new manual
+			 * non-volatile values.
+			 */
+			if (set_volatiles || !(helpers[idx].ctrl->flags & V4L2_CTRL_FLAG_VOLATILE))
+				ret = user_to_new(cs->controls + idx, helpers[idx].ctrl);
 			idx = helpers[idx].next;
 		} while (!ret && idx);
 
@@ -2697,6 +2708,9 @@ static int set_ctrl(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl,
 	if (ctrl->type == V4L2_CTRL_TYPE_STRING)
 		return -EINVAL;
 
+	if (ctrl->flags & V4L2_CTRL_FLAG_VOLATILE)
+		return 0;
+
 	/* Reset the 'is_new' flags of the cluster */
 	for (i = 0; i < master->ncontrols; i++)
 		if (master->cluster[i])
-- 
1.8.4.rc1

Re: [PATCH] v4l2-ctrl: fix setting volatile controls

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Thursday 08 August 2013 12:58:52 Hans Verkuil wrote:
> The V4L2 specification allows setting volatile controls as that is needed
> if you want to be able to set all controls in one go using
> VIDIOC_S_EXT_CTRLS.
> 
> However, such new values should be ignored by the control framework
> since it makes no sense to set a volatile control. While the new value
> will be ignored anyway, it does generate a bogus 'change value' control
> event that should be suppressed.
> 
> This patch changes the code to skip setting volatile controls, except for
> one particular case where an autocluster switches to manual mode, because
> that causes the volatile controls to become non-volatile, so the new
> specified values should be retained.
> 
> Note that the values returned by VIDIOC_S_CTRL and VIDIOC_S_EXT_CTRLS for
> such skipped volatile controls will be the currently cached values and not
> the latest volatile value. This is something that might have to be fixed
> as well in the future should that be necessary. I think it is overkill,
> though.

This restriction is not documented in Documentation/video4linux/v4l2-
controls.txt. Do we really want to assume that all volatile controls are read-
only and/or inactive ?

> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> Cc: pawel@osciak.com
> ---
>  drivers/media/v4l2-core/v4l2-ctrls.c | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c
> b/drivers/media/v4l2-core/v4l2-ctrls.c index fccd08b..a7cd830 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -2592,6 +2592,7 @@ static int try_set_ext_ctrls(struct v4l2_fh *fh,
> struct v4l2_ctrl_handler *hdl, cs->error_idx = cs->count;
>  	for (i = 0; !ret && i < cs->count; i++) {
>  		struct v4l2_ctrl *master;
> +		bool set_volatiles = false;
>  		u32 idx = i;
> 
>  		if (helpers[i].mref == NULL)
> @@ -2627,14 +2628,24 @@ static int try_set_ext_ctrls(struct v4l2_fh *fh,
> struct v4l2_ctrl_handler *hdl, } while (tmp_idx);
>  			/* If the new value == the manual value, then copy
>  			   the current volatile values. */
> -			if (new_auto_val == master->manual_mode_value)
> +			if (new_auto_val == master->manual_mode_value) {
>  				update_from_auto_cluster(master);
> +				set_volatiles = true;
> +			}
>  		}
> 
>  		/* Copy the new caller-supplied control values.
>  		   user_to_new() sets 'is_new' to 1. */
>  		do {
> -			ret = user_to_new(cs->controls + idx, helpers[idx].ctrl);
> +			/*
> +			 * Skip attempts to set volatile controls since those are
> +			 * ignored anyway. The exception is when an autocluster is
> +			 * switched to manual mode, since in that case the specified
> +			 * 'volatile' controls are actually the new manual
> +			 * non-volatile values.
> +			 */
> +			if (set_volatiles || !(helpers[idx].ctrl->flags &
> V4L2_CTRL_FLAG_VOLATILE)) +				ret = user_to_new(cs->controls + 
idx,
> helpers[idx].ctrl);
>  			idx = helpers[idx].next;
>  		} while (!ret && idx);
> 
> @@ -2697,6 +2708,9 @@ static int set_ctrl(struct v4l2_fh *fh, struct
> v4l2_ctrl *ctrl, if (ctrl->type == V4L2_CTRL_TYPE_STRING)
>  		return -EINVAL;
> 
> +	if (ctrl->flags & V4L2_CTRL_FLAG_VOLATILE)
> +		return 0;
> +
>  	/* Reset the 'is_new' flags of the cluster */
>  	for (i = 0; i < master->ncontrols; i++)
>  		if (master->cluster[i])
-- 
Regards,

Laurent Pinchart