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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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