Re: [RFC PATCH] Modify volatile auto cluster handling as per earlier discussions

[Hans Verkuil <hverkuil@xs4all.nl>]

On Thursday, August 25, 2011 22:22:28 Hans de Goede wrote:
> Hi,
> 
> First of all thanks for doing this! Overall it looks good,
> see below for several (small) remarks which I have.


Thanks for the review!

> On 08/09/2011 06:40 PM, Hans Verkuil wrote:
> > This patch modifies the way autoclusters work when the 'foo' controls are
> > volatile if autofoo is on.
> >
> > E.g.: if autogain is true, then gain returns the gain set by the autogain
> > circuitry.
> >
> > This patch makes the following changes:
> >
> > 1) The V4L2_CTRL_FLAG_VOLATILE flag is added to let userspace know that a certain
> >     control is volatile. Currently this is internal information only.
> >
> > 2) If v4l2_ctrl_auto_cluster() is called with the last 'set_volatile' argument set
> >     to true, then the cluster has the following behavior:
> >
> >     - when in manual mode you can set the manual values normally.
> >     - when in auto mode any new manual values will be ignored. When you
> >       read the manual values you will get those as determined by the auto mode.
> >     - when switching from auto to manual mode the manual values from the auto
> >       mode are obtained through g_volatile_ctrl first. Any manual values explicitly
> >       set by the application will replace those obtained from the automode and the
> >       final set of values is sent to the driver with s_ctrl.
> >     - when in auto mode the V4L2_CTRL_FLAG_VOLATILE and INACTIVE flags are set for
> >       the 'foo' controls. These flags are cleared when in manual mode.
> >
> > This patch modifies existing users of is_volatile and simplifies the pwc driver
> > that required this behavior.
> >
> > The only thing missing is the documentation update and some code comments.
> >
> > I have to admit that it works quite well.
> >
> > Hans, can you verify that this does what you wanted it to do?
> >
> > Regards,
> >
> > 	Hans
> >
> 
> <snip>
> 
> > diff --git a/drivers/media/video/pwc/pwc-v4l.c b/drivers/media/video/pwc/pwc-v4l.c
> > index e9a0e94..4ce00bf 100644
> > --- a/drivers/media/video/pwc/pwc-v4l.c
> > +++ b/drivers/media/video/pwc/pwc-v4l.c
> 
> <snip>
> 
> > @@ -632,52 +634,28 @@ static int pwc_set_awb(struct pwc_device *pdev)
> >   {
> >   	int ret = 0;
> >
> > -	if (pdev->auto_white_balance->is_new) {
> > +	if (pdev->auto_white_balance->is_new)
> >   		ret = pwc_set_u8_ctrl(pdev, SET_CHROM_CTL,
> > -				      WB_MODE_FORMATTER,
> > -				      pdev->auto_white_balance->val);
> > -		if (ret)
> > -			return ret;
> > +			WB_MODE_FORMATTER,
> > +			pdev->auto_white_balance->val);
> > +	if (ret)
> > +		return ret;
> >
> > -		/* Update val when coming from auto or going to a preset */
> > -		if (pdev->red_balance->is_volatile ||
> > -		    pdev->auto_white_balance->val == awb_indoor ||
> > -		    pdev->auto_white_balance->val == awb_outdoor ||
> > -		    pdev->auto_white_balance->val == awb_fl) {
> > -			if (!pdev->red_balance->is_new)
> > -				pwc_get_u8_ctrl(pdev, GET_STATUS_CTL,
> > -					READ_RED_GAIN_FORMATTER,
> > -					&pdev->red_balance->val);
> > -			if (!pdev->blue_balance->is_new)
> > -				pwc_get_u8_ctrl(pdev, GET_STATUS_CTL,
> > -					READ_BLUE_GAIN_FORMATTER,
> > -					&pdev->blue_balance->val);
> > -		}
> > -		if (pdev->auto_white_balance->val == awb_auto) {
> > -			pdev->red_balance->is_volatile = true;
> > -			pdev->blue_balance->is_volatile = true;
> > -			pdev->color_bal_valid = false; /* Force cache update */
> > -		} else {
> > -			pdev->red_balance->is_volatile = false;
> > -			pdev->blue_balance->is_volatile = false;
> > -		}
> > +	if (pdev->auto_white_balance->val != awb_manual) {
> > +		pdev->color_bal_valid = false; /* Force cache update */
> > +		return 0;
> >   	}
> >
> 
> The setting of pdev->color_bal_valid = false should happen inside
> the "if (pdev->auto_white_balance->is_new)" block (under the same
> pdev->auto_white_balance->val != awb_manual condition), the way it
> is now if someone tries to say write blue bal while in awb mode, not
> only will the write get ignored (good), but this will also invalidate
> the cached values (bad).

True. 

> > -	if (ret == 0&&  pdev->red_balance->is_new) {
> > -		if (pdev->auto_white_balance->val != awb_manual)
> > -			return -EBUSY;
> > +	if (pdev->red_balance->is_new)
> >   		ret = pwc_set_u8_ctrl(pdev, SET_CHROM_CTL,
> > -				      PRESET_MANUAL_RED_GAIN_FORMATTER,
> > -				      pdev->red_balance->val);
> > -	}
> > -
> > -	if (ret == 0&&  pdev->blue_balance->is_new) {
> > -		if (pdev->auto_white_balance->val != awb_manual)
> > -			return -EBUSY;
> > +			PRESET_MANUAL_RED_GAIN_FORMATTER,
> > +			pdev->red_balance->val);
> > +	if (ret)
> > +		return ret;
> > +	if (pdev->blue_balance->is_new)
> >   		ret = pwc_set_u8_ctrl(pdev, SET_CHROM_CTL,
> > -				      PRESET_MANUAL_BLUE_GAIN_FORMATTER,
> > -				      pdev->blue_balance->val);
> > -	}
> > +			PRESET_MANUAL_BLUE_GAIN_FORMATTER,
> > +			pdev->blue_balance->val);
> >   	return ret;
> >   }
> >
> 
> Nitpick: I'm also not happy with moving the error return checks outside
> of the is_new blocks, I know the end result is the same, but it just
> feels a bit wrong to me, because it makes less clear what the actual
> intend of the check is (to check the pwc_set_u8_ctrl result).

I'm fine with either approach.

> This how I would like pwc_set_awb to look after this patch:
> 
> {
>   	int ret;
> 
> 	if (pdev->auto_white_balance->is_new)
>   		ret = pwc_set_u8_ctrl(pdev, SET_CHROM_CTL,
> 				      WB_MODE_FORMATTER,
> 				      pdev->auto_white_balance->val);
> 		if (ret)
> 			return ret;
> 
> 		if (pdev->auto_white_balance->val != awb_manual)
> 			pdev->color_bal_valid = false; /* Force cache update */
> 	}
> 
> 	if (pdev->auto_white_balance->val != awb_manual)
> 		return 0;
> 
> 	if (pdev->red_balance->is_new) {
>   		ret = pwc_set_u8_ctrl(pdev, SET_CHROM_CTL,
> 			PRESET_MANUAL_RED_GAIN_FORMATTER,
> 			pdev->red_balance->val);
> 		if (ret)
> 			return ret;
> 	}
> 
> 	if (pdev->blue_balance->is_new) {
>   		ret = pwc_set_u8_ctrl(pdev, SET_CHROM_CTL,
> 			PRESET_MANUAL_BLUE_GAIN_FORMATTER,
> 			pdev->blue_balance->val);
> 		if (ret)
> 			return ret;
> 	}
> 
>   	return 0;
> }
> 
> 
> > @@ -686,26 +664,18 @@ static int pwc_set_autogain(struct pwc_device *pdev)
> >   {
> >   	int ret = 0;
> >
> > -	if (pdev->autogain->is_new) {
> > +	if (pdev->autogain->is_new)
> >   		ret = pwc_set_u8_ctrl(pdev, SET_LUM_CTL,
> > -				      AGC_MODE_FORMATTER,
> > -				      pdev->autogain->val ? 0 : 0xff);
> > -		if (ret)
> > -			return ret;
> > -		if (pdev->autogain->val)
> > -			pdev->gain_valid = false; /* Force cache update */
> > -		else if (!pdev->gain->is_new)
> > -			pwc_get_u8_ctrl(pdev, GET_STATUS_CTL,
> > -					READ_AGC_FORMATTER,
> > -					&pdev->gain->val);
> > -	}
> > -	if (ret == 0&&  pdev->gain->is_new) {
> > -		if (pdev->autogain->val)
> > -			return -EBUSY;
> > +			AGC_MODE_FORMATTER,
> > +			pdev->autogain->val ? 0 : 0xff);
> > +	if (ret)
> > +		return ret;
> > +	if (pdev->autogain->val)
> > +		pdev->gain_valid = false; /* Force cache update */
> > +	else if (pdev->gain->is_new)
> >   		ret = pwc_set_u8_ctrl(pdev, SET_LUM_CTL,
> >   				      PRESET_AGC_FORMATTER,
> >   				      pdev->gain->val);
> > -	}
> >   	return ret;
> >   }
> 
> Same old, same old, invalidation of the cache should be inside
> the is_new block. Move error checks into is_new blocks, ie:
> 
> {
>     	int ret;
> 
> 	if (pdev->autogain->is_new) {
>     		ret = pwc_set_u8_ctrl(pdev, SET_LUM_CTL,
> 				      AGC_MODE_FORMATTER,
> -				      pdev->autogain->val ? 0 : 0xff);
> 		if (ret)
> 			return ret;
> 		if (pdev->autogain->val)
> 			pdev->gain_valid = false; /* Force cache update */
> 	}
> 
> 	if (pdev->autogain->val)
> 		return 0;
> 
> 	if (pdev->gain->is_new) {
>    		ret = pwc_set_u8_ctrl(pdev, SET_LUM_CTL,
>    				      PRESET_AGC_FORMATTER,
>    				      pdev->gain->val);
> 		if (ret)
> 			return ret;
> 	}
> 
> 	return 0;
> }
> 
> 
> 
> >
> > @@ -715,26 +685,18 @@ static int pwc_set_exposure_auto(struct pwc_device *pdev)
> >   	int ret = 0;
> >   	int is_auto = pdev->exposure_auto->val == V4L2_EXPOSURE_AUTO;
> >
> > -	if (pdev->exposure_auto->is_new) {
> > +	if (pdev->exposure_auto->is_new)
> >   		ret = pwc_set_u8_ctrl(pdev, SET_LUM_CTL,
> > -				      SHUTTER_MODE_FORMATTER,
> > -				      is_auto ? 0 : 0xff);
> > -		if (ret)
> > -			return ret;
> > -		if (is_auto)
> > -			pdev->exposure_valid = false; /* Force cache update */
> > -		else if (!pdev->exposure->is_new)
> > -			pwc_get_u16_ctrl(pdev, GET_STATUS_CTL,
> > -					 READ_SHUTTER_FORMATTER,
> > -					&pdev->exposure->val);
> > -	}
> > -	if (ret == 0&&  pdev->exposure->is_new) {
> > -		if (is_auto)
> > -			return -EBUSY;
> > +			SHUTTER_MODE_FORMATTER,
> > +			is_auto ? 0 : 0xff);
> > +	if (ret)
> > +		return ret;
> > +	if (is_auto)
> > +		pdev->exposure_valid = false; /* Force cache update */
> > +	else if (pdev->exposure->is_new)
> >   		ret = pwc_set_u16_ctrl(pdev, SET_LUM_CTL,
> >   				       PRESET_SHUTTER_FORMATTER,
> >   				       pdev->exposure->val);
> > -	}
> >   	return ret;
> >   }
> >
> 
> Idem.
> 
> > @@ -743,39 +705,26 @@ static int pwc_set_autogain_expo(struct pwc_device *pdev)
> >   {
> >   	int ret = 0;
> >
> > -	if (pdev->autogain->is_new) {
> > +	if (pdev->autogain->is_new)
> >   		ret = pwc_set_u8_ctrl(pdev, SET_LUM_CTL,
> >   				      AGC_MODE_FORMATTER,
> >   				      pdev->autogain->val ? 0 : 0xff);
> > +	if (ret)
> > +		return ret;
> > +	if (pdev->autogain->val) {
> > +		pdev->gain_valid     = false; /* Force cache update */
> > +		pdev->exposure_valid = false; /* Force cache update */
> > +	} else {
> > +		if (pdev->gain->is_new)
> > +			ret = pwc_set_u8_ctrl(pdev, SET_LUM_CTL,
> > +					PRESET_AGC_FORMATTER,
> > +					pdev->gain->val);
> >   		if (ret)
> >   			return ret;
> > -		if (pdev->autogain->val) {
> > -			pdev->gain_valid     = false; /* Force cache update */
> > -			pdev->exposure_valid = false; /* Force cache update */
> > -		} else {
> > -			if (!pdev->gain->is_new)
> > -				pwc_get_u8_ctrl(pdev, GET_STATUS_CTL,
> > -						READ_AGC_FORMATTER,
> > -						&pdev->gain->val);
> > -			if (!pdev->exposure->is_new)
> > -				pwc_get_u16_ctrl(pdev, GET_STATUS_CTL,
> > -						 READ_SHUTTER_FORMATTER,
> > -						&pdev->exposure->val);
> > -		}
> > -	}
> > -	if (ret == 0&&  pdev->gain->is_new) {
> > -		if (pdev->autogain->val)
> > -			return -EBUSY;
> > -		ret = pwc_set_u8_ctrl(pdev, SET_LUM_CTL,
> > -				      PRESET_AGC_FORMATTER,
> > -				      pdev->gain->val);
> > -	}
> > -	if (ret == 0&&  pdev->exposure->is_new) {
> > -		if (pdev->autogain->val)
> > -			return -EBUSY;
> > -		ret = pwc_set_u16_ctrl(pdev, SET_LUM_CTL,
> > -				       PRESET_SHUTTER_FORMATTER,
> > -				       pdev->exposure->val);
> > +		if (pdev->exposure->is_new)
> > +			ret = pwc_set_u16_ctrl(pdev, SET_LUM_CTL,
> > +					PRESET_SHUTTER_FORMATTER,
> > +					pdev->exposure->val);
> >   	}
> >   	return ret;
> >   }
> 
> Idem
> 
> > @@ -872,20 +821,13 @@ static int pwc_s_ctrl(struct v4l2_ctrl *ctrl)
> >   				      ctrl->val ? 0 : 0xff);
> >   		break;
> >   	case PWC_CID_CUSTOM(autocontour):
> > -		if (pdev->autocontour->is_new) {
> > -			ret = pwc_set_u8_ctrl(pdev, SET_LUM_CTL,
> > -					AUTO_CONTOUR_FORMATTER,
> > -					pdev->autocontour->val ? 0 : 0xff);
> > -		}
> > -		if (ret == 0&&  pdev->contour->is_new) {
> > -			if (pdev->autocontour->val) {
> > -				ret = -EBUSY;
> > -				break;
> > -			}
> > +		ret = pwc_set_u8_ctrl(pdev, SET_LUM_CTL,
> > +				AUTO_CONTOUR_FORMATTER,
> > +				pdev->autocontour->val ? 0 : 0xff);
> > +		if (ret == 0&&  pdev->autocontour->val)
> >   			ret = pwc_set_u8_ctrl(pdev, SET_LUM_CTL,
> >   					      PRESET_CONTOUR_FORMATTER,
> >   					      pdev->contour->val);
> > -		}
> >   		break;
> >   	case V4L2_CID_BACKLIGHT_COMPENSATION:
> >   		ret = pwc_set_u8_ctrl(pdev, SET_LUM_CTL,
> 
> 2 things:
> 1) Why not use is_new before setting ?

No idea :-)

I think this may have been a left-over from an earlier attempt.

> 2) contour is not volatile, you can set it to $random value while
>     auto is on, and then when you turn auto off, it will "jump"
>     to $random setting you last did, so there should be no
>     pdev->autocontour->val check (and if there should be it should be
>     inverted.
> 
> I've attached a revised version of the patch addrressing all
> my concerns / code style request wrt the pwc driver.

Thanks! I'll use that for my pull request.

> 
> > @@ -1099,6 +1041,14 @@ static int pwc_enum_frameintervals(struct file *file, void *fh,
> >   	return 0;
> >   }
> >
> > +static int pwc_log_status(struct file *file, void *priv)
> > +{
> > +	struct pwc_device *pdev = video_drvdata(file);
> > +
> > +	v4l2_ctrl_handler_log_status(&pdev->ctrl_handler, PWC_NAME);
> > +	return 0;
> > +}
> > +
> >   static long pwc_default(struct file *file, void *fh, bool valid_prio,
> >   			int cmd, void *arg)
> >   {
> > @@ -1122,6 +1072,7 @@ const struct v4l2_ioctl_ops pwc_ioctl_ops = {
> >   	.vidioc_dqbuf			    = pwc_dqbuf,
> >   	.vidioc_streamon		    = pwc_streamon,
> >   	.vidioc_streamoff		    = pwc_streamoff,
> > +	.vidioc_log_status		    = pwc_log_status,
> >   	.vidioc_enum_framesizes		    = pwc_enum_framesizes,
> >   	.vidioc_enum_frameintervals	    = pwc_enum_frameintervals,
> >   	.vidioc_default		    = pwc_default,
> 
> <snip>
> 
> > diff --git a/drivers/media/video/v4l2-ctrls.c b/drivers/media/video/v4l2-ctrls.c
> > index 06b6014..b29f3d8 100644
> > --- a/drivers/media/video/v4l2-ctrls.c
> > +++ b/drivers/media/video/v4l2-ctrls.c
> > @@ -43,7 +43,7 @@ struct v4l2_ctrl_helper {
> >   };
> >
> >   /* Small helper function to determine if the autocluster is set to manual
> > -   mode. In that case the is_volatile flag should be ignored. */
> > +   mode. */
> >   static bool is_cur_manual(const struct v4l2_ctrl *master)
> >   {
> >   	return master->is_auto&&  master->cur.val == master->manual_mode_value;
> > @@ -937,9 +937,13 @@ static void new_to_cur(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl,
> >   		break;
> >   	}
> >   	if (update_inactive) {
> > -		ctrl->flags&= ~V4L2_CTRL_FLAG_INACTIVE;
> > -		if (!is_cur_manual(ctrl->cluster[0]))
> > +		ctrl->flags&=
> > +			~(V4L2_CTRL_FLAG_INACTIVE | V4L2_CTRL_FLAG_VOLATILE);
> > +		if (!is_cur_manual(ctrl->cluster[0])) {
> >   			ctrl->flags |= V4L2_CTRL_FLAG_INACTIVE;
> > +			if (ctrl->cluster[0]->is_auto_volatile)
> > +				ctrl->flags |= V4L2_CTRL_FLAG_VOLATILE;
> > +		}
> >   	}
> >   	if (changed || update_inactive) {
> >   		/* If a control was changed that was not one of the controls
> > @@ -1394,10 +1398,8 @@ struct v4l2_ctrl *v4l2_ctrl_new_custom(struct v4l2_ctrl_handler *hdl,
> >   			type, min, max,
> >   			is_menu ? cfg->menu_skip_mask : step,
> >   			def, flags, qmenu, priv);
> > -	if (ctrl) {
> > +	if (ctrl)
> >   		ctrl->is_private = cfg->is_private;
> > -		ctrl->is_volatile = cfg->is_volatile;
> > -	}
> >   	return ctrl;
> >   }
> >   EXPORT_SYMBOL(v4l2_ctrl_new_custom);
> > @@ -1491,6 +1493,7 @@ EXPORT_SYMBOL(v4l2_ctrl_add_handler);
> >   /* Cluster controls */
> >   void v4l2_ctrl_cluster(unsigned ncontrols, struct v4l2_ctrl **controls)
> >   {
> > +	bool is_auto_volatile = false;
> >   	int i;
> >
> >   	/* The first control is the master control and it must not be NULL */
> > @@ -1500,8 +1503,11 @@ void v4l2_ctrl_cluster(unsigned ncontrols, struct v4l2_ctrl **controls)
> >   		if (controls[i]) {
> >   			controls[i]->cluster = controls;
> >   			controls[i]->ncontrols = ncontrols;
> > +			if (controls[i]->flags&  V4L2_CTRL_FLAG_VOLATILE)
> > +				is_auto_volatile = true;
> >   		}
> >   	}
> > +	controls[0]->is_auto_volatile = is_auto_volatile;
> >   }
> >   EXPORT_SYMBOL(v4l2_ctrl_cluster);
> >
> 
> I see that you are setting is_auto_volatile for regular (non auto
> clusters) here. First of all given its name that seems weird.
> 
> I can see that this then gets used in v4l2_g_ext_ctrls
> to call g_volatile_ctrl in the case were the master is
> not volatile, but some other controls in the cluster are.
> 
> However it also leads to calling update_from_auto_cluster
> from try_set_ext_ctrls and set_ctrl in the same case, which I
> personally consider an undesirable side-effect, but maybe
> you see things differently?

I agree, this can be improved. I'll work on this.

> > @@ -1509,22 +1515,25 @@ void v4l2_ctrl_auto_cluster(unsigned ncontrols, struct v4l2_ctrl **controls,
> >   			    u8 manual_val, bool set_volatile)
> >   {
> >   	struct v4l2_ctrl *master = controls[0];
> > -	u32 flag;
> > +	u32 flag = 0;
> >   	int i;
> >
> >   	v4l2_ctrl_cluster(ncontrols, controls);
> >   	WARN_ON(ncontrols<= 1);
> >   	WARN_ON(manual_val<  master->minimum || manual_val>  master->maximum);
> > +	WARN_ON(set_volatile&&  !has_op(master, g_volatile_ctrl));
> >   	master->is_auto = true;
> > +	master->is_auto_volatile = set_volatile;
> >   	master->manual_mode_value = manual_val;
> >   	master->flags |= V4L2_CTRL_FLAG_UPDATE;
> > -	flag = is_cur_manual(master) ? 0 : V4L2_CTRL_FLAG_INACTIVE;
> > +
> > +	if (!is_cur_manual(master))
> > +		flag = V4L2_CTRL_FLAG_INACTIVE |
> > +			(set_volatile ? V4L2_CTRL_FLAG_VOLATILE : 0);
> >
> >   	for (i = 1; i<  ncontrols; i++)
> > -		if (controls[i]) {
> > -			controls[i]->is_volatile = set_volatile;
> > +		if (controls[i])
> >   			controls[i]->flags |= flag;
> > -		}
> >   }
> >   EXPORT_SYMBOL(v4l2_ctrl_auto_cluster);
> >
> > @@ -1579,9 +1588,6 @@ EXPORT_SYMBOL(v4l2_ctrl_grab);
> >   static void log_ctrl(const struct v4l2_ctrl *ctrl,
> >   		     const char *prefix, const char *colon)
> >   {
> > -	int fl_inact = ctrl->flags&  V4L2_CTRL_FLAG_INACTIVE;
> > -	int fl_grabbed = ctrl->flags&  V4L2_CTRL_FLAG_GRABBED;
> > -
> >   	if (ctrl->flags&  (V4L2_CTRL_FLAG_DISABLED | V4L2_CTRL_FLAG_WRITE_ONLY))
> >   		return;
> >   	if (ctrl->type == V4L2_CTRL_TYPE_CTRL_CLASS)
> > @@ -1612,14 +1618,17 @@ static void log_ctrl(const struct v4l2_ctrl *ctrl,
> >   		printk(KERN_CONT "unknown type %d", ctrl->type);
> >   		break;
> >   	}
> > -	if (fl_inact&&  fl_grabbed)
> > -		printk(KERN_CONT " (inactive, grabbed)\n");
> > -	else if (fl_inact)
> > -		printk(KERN_CONT " (inactive)\n");
> > -	else if (fl_grabbed)
> > -		printk(KERN_CONT " (grabbed)\n");
> > -	else
> > -		printk(KERN_CONT "\n");
> > +	if (ctrl->flags&  (V4L2_CTRL_FLAG_INACTIVE |
> > +			   V4L2_CTRL_FLAG_GRABBED |
> > +			   V4L2_CTRL_FLAG_VOLATILE)) {
> > +		if (ctrl->flags&  V4L2_CTRL_FLAG_INACTIVE)
> > +			printk(KERN_CONT " inactive");
> > +		if (ctrl->flags&  V4L2_CTRL_FLAG_GRABBED)
> > +			printk(KERN_CONT " grabbed");
> > +		if (ctrl->flags&  V4L2_CTRL_FLAG_VOLATILE)
> > +			printk(KERN_CONT " volatile");
> > +	}
> > +	printk(KERN_CONT "\n");
> >   }
> >
> >   /* Log all controls owned by the handler */
> > @@ -1959,7 +1968,8 @@ int v4l2_g_ext_ctrls(struct v4l2_ctrl_handler *hdl, struct v4l2_ext_controls *cs
> >   		v4l2_ctrl_lock(master);
> >
> >   		/* g_volatile_ctrl will update the new control values */
> > -		if (has_op(master, g_volatile_ctrl)&&  !is_cur_manual(master)) {
> > +		if ((master->flags&  V4L2_CTRL_FLAG_VOLATILE) ||
> > +			(master->is_auto_volatile&&  !is_cur_manual(master))) {
> >   			for (j = 0; j<  master->ncontrols; j++)
> >   				cur_to_new(master->cluster[j]);
> >   			ret = call_op(master, g_volatile_ctrl);
> > @@ -2004,7 +2014,7 @@ static int get_ctrl(struct v4l2_ctrl *ctrl, s32 *val)
> >
> >   	v4l2_ctrl_lock(master);
> >   	/* g_volatile_ctrl will update the current control values */
> > -	if (ctrl->is_volatile&&  !is_cur_manual(master)) {
> > +	if (ctrl->flags&  V4L2_CTRL_FLAG_VOLATILE) {
> >   		for (i = 0; i<  master->ncontrols; i++)
> >   			cur_to_new(master->cluster[i]);
> >   		ret = call_op(master, g_volatile_ctrl);
> > @@ -2120,6 +2130,18 @@ static int validate_ctrls(struct v4l2_ext_controls *cs,
> >   	return 0;
> >   }
> >
> > +static void update_from_auto_cluster(struct v4l2_ctrl *master)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i<  master->ncontrols; i++)
> > +		cur_to_new(master->cluster[i]);
> > +	if (!call_op(master, g_volatile_ctrl))
> > +		for (i = 1; i<  master->ncontrols; i++)
> > +			if (master->cluster[i])
> > +				master->cluster[i]->is_new = 1;
> > +}
> > +
> >   /* Try or try-and-set controls */
> >   static int try_set_ext_ctrls(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
> >   			     struct v4l2_ext_controls *cs,
> > @@ -2165,6 +2187,9 @@ static int try_set_ext_ctrls(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
> >   			if (master->cluster[j])
> >   				master->cluster[j]->is_new = 0;
> >
> > +		if (master->is_auto_volatile&&  !is_cur_manual(master))
> > +			update_from_auto_cluster(master);
> > +
> >   		/* Copy the new caller-supplied control values.
> >   		   user_to_new() sets 'is_new' to 1. */
> >   		do {
> > @@ -2235,6 +2260,8 @@ static int set_ctrl(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, s32 *val)
> >   		if (master->cluster[i])
> >   			master->cluster[i]->is_new = 0;
> >
> > +	if (master->is_auto_volatile&&  !is_cur_manual(master))
> > +		update_from_auto_cluster(master);
> >   	ctrl->val = *val;
> >   	ctrl->is_new = 1;
> >   	ret = try_or_set_cluster(fh, master, true);
> 
> <snip>
> 
> > diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> > index 13fe4d7..d1a77cd 100644
> > --- a/include/media/v4l2-ctrls.h
> > +++ b/include/media/v4l2-ctrls.h
> > @@ -118,8 +118,8 @@ struct v4l2_ctrl {
> >
> >   	unsigned int is_new:1;
> >   	unsigned int is_private:1;
> > -	unsigned int is_volatile:1;
> >   	unsigned int is_auto:1;
> > +	unsigned int is_auto_volatile:1;
> >   	unsigned int manual_mode_value:8;
> >
> >   	const struct v4l2_ctrl_ops *ops;
> > @@ -208,9 +208,9 @@ struct v4l2_ctrl_handler {
> >     *		must be NULL.
> >     * @is_private: If set, then this control is private to its handler and it
> >     *		will not be added to any other handlers.
> > -  * @is_volatile: If set, then this control is volatile. This means that the
> > -  *		control's current value cannot be cached and needs to be
> > -  *		retrieved through the g_volatile_ctrl op.
> > +  * @is_volatile: If set, then this autocluster control is volatile. This means
> > +  *		that the control's current value cannot be cached and needs to
> > +  *		be retrieved through the g_volatile_ctrl op.
> >     */
> >   struct v4l2_ctrl_config {
> >   	const struct v4l2_ctrl_ops *ops;
> 
> Typo: adds docstring for is_volatile, should be is_auto_volatile

Actually, it should be removed altogether. is_volatile is now marked by the
VOLATILE flag.

> 
> > @@ -225,7 +225,6 @@ struct v4l2_ctrl_config {
> >   	u32 menu_skip_mask;
> >   	const char * const *qmenu;
> >   	unsigned int is_private:1;
> > -	unsigned int is_volatile:1;
> >   };
> >
> >   /** v4l2_ctrl_fill() - Fill in the control fields based on the control ID.
> 
> Regards,
> 
> Hans
> 

Thanks!

	Hans