Skip to contentSkip to conversation

v4l2-ctrl: fix setting volatile controls

PATCH In review Hans Verkuil 2 participants 2 messages Aug 8, 2013 Media (V4L/DVB)

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 v4l2-ctrl: fix setting volatile controls

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.

---
 drivers/media/v4l2-core/v4l2-ctrls.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)
drivers/media/v4l2-core/v4l2-ctrls.c +16 -2
index fccd08b..a7cd830 100644
@@ -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])
Signature · 1 line
1.8.4.rc1
#1 lore raw

Hi Hans,

Thank you for the patch.

Quoted 14 lines · 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.

Quoted 5 lines

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 ?

Quoted 41 lines
---
 drivers/media/v4l2-core/v4l2-ctrls.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)
(quoted diff) +0 -0

b/drivers/media/v4l2-core/v4l2-ctrls.c index fccd08b..a7cd830 100644

drivers/media/v4l2-core/v4l2-ctrls.c +12 -2
@@ -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 &
Quoted 1 line

V4L2_CTRL_FLAG_VOLATILE)) + ret = user_to_new(cs->controls +

idx,

Quoted 14 lines

helpers[idx].ctrl);

drivers/media/v4l2-core/v4l2-ctrls.c +3 -0
 			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])
Signature · 3 lines
Regards,

Laurent Pinchart