[PATCH yavta 0/7] Compound controls and controls reset support

[PATCH yavta 0/7] Compound controls and controls reset support

[Laurent Pinchart]

Hello,

This patch series implements support for compound controls in yavta,
including the ability to reset controls to their default value. Only
array compound controls are supported for now, other types may be added
later.

The patches have lived out of the master branch for long enough, it's
time to get them merged.

Kieran Bingham (1):
  Add support to reset device controls

Laurent Pinchart (6):
  yavta: Refactor video_list_controls()
  Implement VIDIOC_QUERY_EXT_CTRL support
  Implement compound control get support
  Implement compound control set support
  Support setting control from values stored in a file
  Remove unneeded conditional compilation for old V4L2 API versions

 yavta.c | 602 +++++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 464 insertions(+), 138 deletions(-)

-- 
Regards,

Laurent Pinchart

[PATCH yavta 1/7] yavta: Refactor video_list_controls()

[Laurent Pinchart]

Separate iteration over controls from printing, in order to reuse the
iteration to implement control reset.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 yavta.c | 133 ++++++++++++++++++++++++++++++++++----------------------
 1 file changed, 82 insertions(+), 51 deletions(-)

diff --git a/yavta.c b/yavta.c
index 2d3b2d096f7d..98bc09810ff1 100644
--- a/yavta.c
+++ b/yavta.c
@@ -484,9 +484,12 @@ static int query_control(struct device *dev, unsigned int id,
 	query->id = id;
 
 	ret = ioctl(dev->fd, VIDIOC_QUERYCTRL, query);
-	if (ret < 0 && errno != EINVAL)
-		printf("unable to query control 0x%8.8x: %s (%d).\n",
-		       id, strerror(errno), errno);
+	if (ret < 0) {
+		ret = -errno;
+		if (ret != -EINVAL)
+			printf("unable to query control 0x%8.8x: %s (%d).\n",
+			       id, strerror(errno), errno);
+	}
 
 	return ret;
 }
@@ -1120,7 +1123,45 @@ static int video_enable(struct device *dev, int enable)
 	return 0;
 }
 
-static void video_query_menu(struct device *dev, struct v4l2_queryctrl *query,
+static int video_for_each_control(struct device *dev,
+				  int(*callback)(struct device *dev, const struct v4l2_queryctrl *query))
+{
+	struct v4l2_queryctrl query;
+	unsigned int nctrls = 0;
+	unsigned int id;
+	int ret;
+
+#ifndef V4L2_CTRL_FLAG_NEXT_CTRL
+	unsigned int i;
+
+	for (i = V4L2_CID_BASE; i <= V4L2_CID_LASTP1; ++i) {
+		id = i;
+#else
+	id = 0;
+	while (1) {
+		id |= V4L2_CTRL_FLAG_NEXT_CTRL;
+#endif
+
+		ret = query_control(dev, id, &query);
+		if (ret == -EINVAL)
+			break;
+		if (ret < 0)
+			return ret;
+
+		id = query.id;
+
+		ret = callback(dev, &query);
+		if (ret < 0)
+			return ret;
+
+		if (ret)
+			nctrls++;
+	}
+
+	return nctrls;
+}
+
+static void video_query_menu(struct device *dev, const struct v4l2_queryctrl *query,
 			     unsigned int value)
 {
 	struct v4l2_querymenu menu;
@@ -1142,83 +1183,68 @@ static void video_query_menu(struct device *dev, struct v4l2_queryctrl *query,
 	};
 }
 
-static int video_print_control(struct device *dev, unsigned int id, bool full)
+static int video_print_control(struct device *dev,
+			       const struct v4l2_queryctrl *query, bool full)
 {
 	struct v4l2_ext_control ctrl;
-	struct v4l2_queryctrl query;
 	char sval[24];
 	char *current = sval;
 	int ret;
 
-	ret = query_control(dev, id, &query);
-	if (ret < 0)
-		return ret;
+	if (query->flags & V4L2_CTRL_FLAG_DISABLED)
+		return 0;
 
-	if (query.flags & V4L2_CTRL_FLAG_DISABLED)
-		return query.id;
-
-	if (query.type == V4L2_CTRL_TYPE_CTRL_CLASS) {
-		printf("--- %s (class 0x%08x) ---\n", query.name, query.id);
-		return query.id;
+	if (query->type == V4L2_CTRL_TYPE_CTRL_CLASS) {
+		printf("--- %s (class 0x%08x) ---\n", query->name, query->id);
+		return 0;
 	}
 
-	ret = get_control(dev, &query, &ctrl);
+	ret = get_control(dev, query, &ctrl);
 	if (ret < 0)
 		strcpy(sval, "n/a");
-	else if (query.type == V4L2_CTRL_TYPE_INTEGER64)
+	else if (query->type == V4L2_CTRL_TYPE_INTEGER64)
 		sprintf(sval, "%lld", ctrl.value64);
-	else if (query.type == V4L2_CTRL_TYPE_STRING)
+	else if (query->type == V4L2_CTRL_TYPE_STRING)
 		current = ctrl.string;
 	else
 		sprintf(sval, "%d", ctrl.value);
 
 	if (full)
 		printf("control 0x%08x `%s' min %d max %d step %d default %d current %s.\n",
-			query.id, query.name, query.minimum, query.maximum,
-			query.step, query.default_value, current);
+			query->id, query->name, query->minimum, query->maximum,
+			query->step, query->default_value, current);
 	else
-		printf("control 0x%08x current %s.\n", query.id, current);
+		printf("control 0x%08x current %s.\n", query->id, current);
 
-	if (query.type == V4L2_CTRL_TYPE_STRING)
+	if (query->type == V4L2_CTRL_TYPE_STRING)
 		free(ctrl.string);
 
 	if (!full)
-		return query.id;
+		return 1;
 
-	if (query.type == V4L2_CTRL_TYPE_MENU ||
-	    query.type == V4L2_CTRL_TYPE_INTEGER_MENU)
-		video_query_menu(dev, &query, ctrl.value);
+	if (query->type == V4L2_CTRL_TYPE_MENU ||
+	    query->type == V4L2_CTRL_TYPE_INTEGER_MENU)
+		video_query_menu(dev, query, ctrl.value);
 
-	return query.id;
+	return 1;
+}
+
+static int __video_print_control(struct device *dev,
+				 const struct v4l2_queryctrl *query)
+{
+	return video_print_control(dev, query, true);
 }
 
 static void video_list_controls(struct device *dev)
 {
-	unsigned int nctrls = 0;
-	unsigned int id;
 	int ret;
 
-#ifndef V4L2_CTRL_FLAG_NEXT_CTRL
-	unsigned int i;
+	ret = video_for_each_control(dev, __video_print_control);
+	if (ret < 0)
+		return;
 
-	for (i = V4L2_CID_BASE; i <= V4L2_CID_LASTP1; ++i) {
-		id = i;
-#else
-	id = 0;
-	while (1) {
-		id |= V4L2_CTRL_FLAG_NEXT_CTRL;
-#endif
-
-		ret = video_print_control(dev, id, true);
-		if (ret < 0)
-			break;
-
-		id = ret;
-		nctrls++;
-	}
-
-	if (nctrls)
-		printf("%u control%s found.\n", nctrls, nctrls > 1 ? "s" : "");
+	if (ret)
+		printf("%u control%s found.\n", ret, ret > 1 ? "s" : "");
 	else
 		printf("No control found.\n");
 }
@@ -2184,8 +2210,13 @@ int main(int argc, char *argv[])
 	if (do_log_status)
 		video_log_status(&dev);
 
-	if (do_get_control)
-		video_print_control(&dev, ctrl_name, false);
+	if (do_get_control) {
+		struct v4l2_queryctrl query;
+
+		ret = query_control(&dev, ctrl_name, &query);
+		if (ret == 0)
+			video_print_control(&dev, &query, false);
+	}
 
 	if (do_set_control)
 		set_control(&dev, ctrl_name, ctrl_value);
-- 
Regards,

Laurent Pinchart

[PATCH yavta 2/7] Implement VIDIOC_QUERY_EXT_CTRL support

[Laurent Pinchart]

Use the new extended control query ioctl when available with an
automatic fall back to VIDIOC_QUERYCTRL.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 yavta.c | 63 +++++++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 48 insertions(+), 15 deletions(-)

diff --git a/yavta.c b/yavta.c
index 98bc09810ff1..eb50d592736f 100644
--- a/yavta.c
+++ b/yavta.c
@@ -476,25 +476,56 @@ static void video_log_status(struct device *dev)
 }
 
 static int query_control(struct device *dev, unsigned int id,
-			 struct v4l2_queryctrl *query)
+			 struct v4l2_query_ext_ctrl *query)
 {
+	struct v4l2_queryctrl q;
 	int ret;
 
 	memset(query, 0, sizeof(*query));
 	query->id = id;
 
-	ret = ioctl(dev->fd, VIDIOC_QUERYCTRL, query);
+	ret = ioctl(dev->fd, VIDIOC_QUERY_EXT_CTRL, query);
+	if (ret < 0)
+		ret = -errno;
+	if (!ret || ret == -EINVAL)
+		return ret;
+
+	if (ret != -ENOTTY) {
+		printf("unable to query control 0x%8.8x: %s (%d).\n",
+		       id, strerror(-ret), -ret);
+		return ret;
+	}
+
+	/*
+	 * If VIDIOC_QUERY_EXT_CTRL isn't available emulate it using
+	 * VIDIOC_QUERYCTRL.
+	 */
+	memset(&q, 0, sizeof(q));
+	q.id = id;
+
+	ret = ioctl(dev->fd, VIDIOC_QUERYCTRL, &q);
 	if (ret < 0) {
 		ret = -errno;
-		if (ret != -EINVAL)
-			printf("unable to query control 0x%8.8x: %s (%d).\n",
-			       id, strerror(errno), errno);
+		printf("unable to query control 0x%8.8x: %s (%d).\n",
+		       id, strerror(-ret), -ret);
+		return ret;
 	}
 
-	return ret;
+	memset(query, 0, sizeof(*query));
+	query->id = q.id;
+	query->type = q.type;
+	memcpy(query->name, q.name, sizeof(query->name));
+	query->minimum = q.minimum;
+	query->maximum = q.maximum;
+	query->step = q.step;
+	query->default_value = q.default_value;
+	query->flags = q.flags;
+
+	return 0;
 }
 
-static int get_control(struct device *dev, const struct v4l2_queryctrl *query,
+static int get_control(struct device *dev,
+		       const struct v4l2_query_ext_ctrl *query,
 		       struct v4l2_ext_control *ctrl)
 {
 	struct v4l2_ext_controls ctrls;
@@ -544,7 +575,7 @@ static void set_control(struct device *dev, unsigned int id,
 {
 	struct v4l2_ext_controls ctrls;
 	struct v4l2_ext_control ctrl;
-	struct v4l2_queryctrl query;
+	struct v4l2_query_ext_ctrl query;
 	int64_t old_val = val;
 	int is_64;
 	int ret;
@@ -1124,9 +1155,9 @@ static int video_enable(struct device *dev, int enable)
 }
 
 static int video_for_each_control(struct device *dev,
-				  int(*callback)(struct device *dev, const struct v4l2_queryctrl *query))
+				  int(*callback)(struct device *dev, const struct v4l2_query_ext_ctrl *query))
 {
-	struct v4l2_queryctrl query;
+	struct v4l2_query_ext_ctrl query;
 	unsigned int nctrls = 0;
 	unsigned int id;
 	int ret;
@@ -1161,7 +1192,8 @@ static int video_for_each_control(struct device *dev,
 	return nctrls;
 }
 
-static void video_query_menu(struct device *dev, const struct v4l2_queryctrl *query,
+static void video_query_menu(struct device *dev,
+			     const struct v4l2_query_ext_ctrl *query,
 			     unsigned int value)
 {
 	struct v4l2_querymenu menu;
@@ -1184,7 +1216,8 @@ static void video_query_menu(struct device *dev, const struct v4l2_queryctrl *qu
 }
 
 static int video_print_control(struct device *dev,
-			       const struct v4l2_queryctrl *query, bool full)
+			       const struct v4l2_query_ext_ctrl *query,
+			       bool full)
 {
 	struct v4l2_ext_control ctrl;
 	char sval[24];
@@ -1210,7 +1243,7 @@ static int video_print_control(struct device *dev,
 		sprintf(sval, "%d", ctrl.value);
 
 	if (full)
-		printf("control 0x%08x `%s' min %d max %d step %d default %d current %s.\n",
+		printf("control 0x%08x `%s' min %lld max %lld step %lld default %lld current %s.\n",
 			query->id, query->name, query->minimum, query->maximum,
 			query->step, query->default_value, current);
 	else
@@ -1230,7 +1263,7 @@ static int video_print_control(struct device *dev,
 }
 
 static int __video_print_control(struct device *dev,
-				 const struct v4l2_queryctrl *query)
+				 const struct v4l2_query_ext_ctrl *query)
 {
 	return video_print_control(dev, query, true);
 }
@@ -2211,7 +2244,7 @@ int main(int argc, char *argv[])
 		video_log_status(&dev);
 
 	if (do_get_control) {
-		struct v4l2_queryctrl query;
+		struct v4l2_query_ext_ctrl query;
 
 		ret = query_control(&dev, ctrl_name, &query);
 		if (ret == 0)
-- 
Regards,

Laurent Pinchart

[PATCH yavta 3/7] Implement compound control get support

[Laurent Pinchart]

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 yavta.c | 154 ++++++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 115 insertions(+), 39 deletions(-)

diff --git a/yavta.c b/yavta.c
index eb50d592736f..6428c22f88d7 100644
--- a/yavta.c
+++ b/yavta.c
@@ -529,6 +529,7 @@ static int get_control(struct device *dev,
 		       struct v4l2_ext_control *ctrl)
 {
 	struct v4l2_ext_controls ctrls;
+	struct v4l2_control old;
 	int ret;
 
 	memset(&ctrls, 0, sizeof(ctrls));
@@ -540,34 +541,32 @@ static int get_control(struct device *dev,
 
 	ctrl->id = query->id;
 
-	if (query->type == V4L2_CTRL_TYPE_STRING) {
-		ctrl->string = malloc(query->maximum + 1);
-		if (ctrl->string == NULL)
+	if (query->flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) {
+		ctrl->size = query->elems * query->elem_size;
+		ctrl->ptr = malloc(ctrl->size);
+		if (ctrl->ptr == NULL)
 			return -ENOMEM;
-
-		ctrl->size = query->maximum + 1;
 	}
 
 	ret = ioctl(dev->fd, VIDIOC_G_EXT_CTRLS, &ctrls);
 	if (ret != -1)
 		return 0;
 
-	if (query->type != V4L2_CTRL_TYPE_INTEGER64 &&
-	    query->type != V4L2_CTRL_TYPE_STRING &&
-	    (errno == EINVAL || errno == ENOTTY)) {
-		struct v4l2_control old;
+	if (query->flags & V4L2_CTRL_FLAG_HAS_PAYLOAD)
+		free(ctrl->ptr);
 
-		old.id = query->id;
-		ret = ioctl(dev->fd, VIDIOC_G_CTRL, &old);
-		if (ret != -1) {
-			ctrl->value = old.value;
-			return 0;
-		}
-	}
+	if (query->flags & V4L2_CTRL_FLAG_HAS_PAYLOAD ||
+	    query->type == V4L2_CTRL_TYPE_INTEGER64 ||
+	    (errno != EINVAL && errno != ENOTTY))
+		return -errno;
 
-	printf("unable to get control 0x%8.8x: %s (%d).\n",
-		query->id, strerror(errno), errno);
-	return -1;
+	old.id = query->id;
+	ret = ioctl(dev->fd, VIDIOC_G_CTRL, &old);
+	if (ret < 0)
+		return -errno;
+
+	ctrl->value = old.value;
+	return 0;
 }
 
 static void set_control(struct device *dev, unsigned int id,
@@ -1170,7 +1169,7 @@ static int video_for_each_control(struct device *dev,
 #else
 	id = 0;
 	while (1) {
-		id |= V4L2_CTRL_FLAG_NEXT_CTRL;
+		id |= V4L2_CTRL_FLAG_NEXT_CTRL | V4L2_CTRL_FLAG_NEXT_COMPOUND;
 #endif
 
 		ret = query_control(dev, id, &query);
@@ -1215,13 +1214,76 @@ static void video_query_menu(struct device *dev,
 	};
 }
 
+static void video_print_control_array(const struct v4l2_query_ext_ctrl *query,
+				      struct v4l2_ext_control *ctrl)
+{
+	unsigned int i;
+
+	printf("{");
+
+	for (i = 0; i < query->elems; ++i) {
+		switch (query->type) {
+		case V4L2_CTRL_TYPE_U8:
+			printf("%u", ctrl->p_u8[i]);
+			break;
+		case V4L2_CTRL_TYPE_U16:
+			printf("%u", ctrl->p_u16[i]);
+			break;
+		case V4L2_CTRL_TYPE_U32:
+			printf("%u", ctrl->p_u32[i]);
+			break;
+		}
+
+		if (i != query->elems - 1)
+			printf(", ");
+	}
+
+	printf("}");
+}
+
+static void video_print_control_value(const struct v4l2_query_ext_ctrl *query,
+				      struct v4l2_ext_control *ctrl)
+{
+	if (query->nr_of_dims == 0) {
+		switch (query->type) {
+		case V4L2_CTRL_TYPE_INTEGER:
+		case V4L2_CTRL_TYPE_BOOLEAN:
+		case V4L2_CTRL_TYPE_MENU:
+		case V4L2_CTRL_TYPE_INTEGER_MENU:
+			printf("%d", ctrl->value);
+			break;
+		case V4L2_CTRL_TYPE_BITMASK:
+			printf("0x%08x", ctrl->value);
+			break;
+		case V4L2_CTRL_TYPE_INTEGER64:
+			printf("%lld", ctrl->value64);
+			break;
+		case V4L2_CTRL_TYPE_STRING:
+			printf("%s", ctrl->string);
+			break;
+		}
+
+		return;
+	}
+
+	switch (query->type) {
+	case V4L2_CTRL_TYPE_U8:
+	case V4L2_CTRL_TYPE_U16:
+	case V4L2_CTRL_TYPE_U32:
+		video_print_control_array(query, ctrl);
+		break;
+	default:
+		printf("unsupported");
+		break;
+	}
+}
+
 static int video_print_control(struct device *dev,
 			       const struct v4l2_query_ext_ctrl *query,
 			       bool full)
 {
 	struct v4l2_ext_control ctrl;
-	char sval[24];
-	char *current = sval;
+	unsigned int i;
 	int ret;
 
 	if (query->flags & V4L2_CTRL_FLAG_DISABLED)
@@ -1232,25 +1294,39 @@ static int video_print_control(struct device *dev,
 		return 0;
 	}
 
-	ret = get_control(dev, query, &ctrl);
-	if (ret < 0)
-		strcpy(sval, "n/a");
-	else if (query->type == V4L2_CTRL_TYPE_INTEGER64)
-		sprintf(sval, "%lld", ctrl.value64);
-	else if (query->type == V4L2_CTRL_TYPE_STRING)
-		current = ctrl.string;
-	else
-		sprintf(sval, "%d", ctrl.value);
-
-	if (full)
-		printf("control 0x%08x `%s' min %lld max %lld step %lld default %lld current %s.\n",
+	if (full) {
+		printf("control 0x%08x `%s' min %lld max %lld step %lld default %lld ",
 			query->id, query->name, query->minimum, query->maximum,
-			query->step, query->default_value, current);
-	else
-		printf("control 0x%08x current %s.\n", query->id, current);
+			query->step, query->default_value);
+		if (query->nr_of_dims) {
+			for (i = 0; i < query->nr_of_dims; ++i)
+				printf("[%u]", query->dims[i]);
+			printf(" ");
+		}
+	} else {
+		printf("control 0x%08x ", query->id);
+	}
 
-	if (query->type == V4L2_CTRL_TYPE_STRING)
-		free(ctrl.string);
+	if (query->type == V4L2_CTRL_TYPE_BUTTON) {
+		/* Button controls have no current value. */
+		printf("\n");
+		return 1;
+	}
+
+	printf("current ");
+
+	ret = get_control(dev, query, &ctrl);
+	if (ret < 0) {
+		printf("n/a\n");
+		printf("unable to get control 0x%8.8x: %s (%d).\n",
+			query->id, strerror(errno), errno);
+	} else {
+		video_print_control_value(query, &ctrl);
+		printf("\n");
+	}
+
+	if (query->flags & V4L2_CTRL_FLAG_HAS_PAYLOAD)
+		free(ctrl.ptr);
 
 	if (!full)
 		return 1;
-- 
Regards,

Laurent Pinchart

[PATCH yavta 4/7] Implement compound control set support

[Laurent Pinchart]

Only arrays of integer types are supported.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 yavta.c | 228 ++++++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 172 insertions(+), 56 deletions(-)

diff --git a/yavta.c b/yavta.c
index 6428c22f88d7..d1bfd380c03b 100644
--- a/yavta.c
+++ b/yavta.c
@@ -19,6 +19,7 @@
 
 #define __STDC_FORMAT_MACROS
 
+#include <ctype.h>
 #include <stdio.h>
 #include <string.h>
 #include <fcntl.h>
@@ -569,59 +570,38 @@ static int get_control(struct device *dev,
 	return 0;
 }
 
-static void set_control(struct device *dev, unsigned int id,
-		        int64_t val)
+static int set_control(struct device *dev,
+		       const struct v4l2_query_ext_ctrl *query,
+		       struct v4l2_ext_control *ctrl)
 {
 	struct v4l2_ext_controls ctrls;
-	struct v4l2_ext_control ctrl;
-	struct v4l2_query_ext_ctrl query;
-	int64_t old_val = val;
-	int is_64;
+	struct v4l2_control old;
 	int ret;
 
-	ret = query_control(dev, id, &query);
-	if (ret < 0)
-		return;
-
-	is_64 = query.type == V4L2_CTRL_TYPE_INTEGER64;
-
 	memset(&ctrls, 0, sizeof(ctrls));
-	memset(&ctrl, 0, sizeof(ctrl));
 
-	ctrls.ctrl_class = V4L2_CTRL_ID2CLASS(id);
+	ctrls.ctrl_class = V4L2_CTRL_ID2CLASS(ctrl->id);
 	ctrls.count = 1;
-	ctrls.controls = &ctrl;
+	ctrls.controls = ctrl;
 
-	ctrl.id = id;
-	if (is_64)
-		ctrl.value64 = val;
-	else
-		ctrl.value = val;
+	ctrl->id = query->id;
 
 	ret = ioctl(dev->fd, VIDIOC_S_EXT_CTRLS, &ctrls);
-	if (ret != -1) {
-		if (is_64)
-			val = ctrl.value64;
-		else
-			val = ctrl.value;
-	} else if (!is_64 && query.type != V4L2_CTRL_TYPE_STRING &&
-		   (errno == EINVAL || errno == ENOTTY)) {
-		struct v4l2_control old;
+	if (ret != -1)
+		return 0;
 
-		old.id = id;
-		old.value = val;
-		ret = ioctl(dev->fd, VIDIOC_S_CTRL, &old);
-		if (ret != -1)
-			val = old.value;
-	}
-	if (ret == -1) {
-		printf("unable to set control 0x%8.8x: %s (%d).\n",
-			id, strerror(errno), errno);
-		return;
-	}
+	if (query->flags & V4L2_CTRL_FLAG_HAS_PAYLOAD ||
+	    query->type == V4L2_CTRL_TYPE_INTEGER64 ||
+	    (errno != EINVAL && errno != ENOTTY))
+		return -1;
 
-	printf("Control 0x%08x set to %" PRId64 ", is %" PRId64 "\n",
-	       id, old_val, val);
+	old.id = ctrl->id;
+	old.value = ctrl->value;
+	ret = ioctl(dev->fd, VIDIOC_S_CTRL, &old);
+	if (ret != -1)
+		ctrl->value = old.value;
+
+	return ret;
 }
 
 static int video_get_format(struct device *dev)
@@ -1278,9 +1258,9 @@ static void video_print_control_value(const struct v4l2_query_ext_ctrl *query,
 	}
 }
 
-static int video_print_control(struct device *dev,
-			       const struct v4l2_query_ext_ctrl *query,
-			       bool full)
+static int video_get_control(struct device *dev,
+			     const struct v4l2_query_ext_ctrl *query,
+			     bool full)
 {
 	struct v4l2_ext_control ctrl;
 	unsigned int i;
@@ -1338,17 +1318,157 @@ static int video_print_control(struct device *dev,
 	return 1;
 }
 
-static int __video_print_control(struct device *dev,
-				 const struct v4l2_query_ext_ctrl *query)
+static int __video_get_control(struct device *dev,
+			       const struct v4l2_query_ext_ctrl *query)
 {
-	return video_print_control(dev, query, true);
+	return video_get_control(dev, query, true);
+}
+
+static int video_parse_control_array(const struct v4l2_query_ext_ctrl *query,
+				     struct v4l2_ext_control *ctrl,
+				     const char *val)
+{
+	unsigned int i;
+	char *endptr;
+	__u32 value;
+
+	for ( ; isspace(*val); ++val) { };
+	if (*val++ != '{')
+		return -EINVAL;
+
+	for (i = 0; i < query->elems; ++i) {
+		for ( ; isspace(*val); ++val) { };
+
+		switch (query->type) {
+		case V4L2_CTRL_TYPE_U8:
+		case V4L2_CTRL_TYPE_U16:
+		case V4L2_CTRL_TYPE_U32:
+		default:
+			value = strtoul(val, &endptr, 0);
+			break;
+		}
+
+		if (endptr == NULL)
+			return -EINVAL;
+
+		switch (query->type) {
+		case V4L2_CTRL_TYPE_U8:
+			ctrl->p_u8[i] = value;
+			break;
+		case V4L2_CTRL_TYPE_U16:
+			ctrl->p_u16[i] = value;
+			break;
+		case V4L2_CTRL_TYPE_U32:
+			ctrl->p_u32[i] = value;
+			break;
+		}
+
+		val = endptr;
+		for ( ; isspace(*val); ++val) { };
+		if (*val++ != ',')
+			break;
+	} 
+
+	if (i < query->elems - 1)
+		return -EINVAL;
+
+	for ( ; isspace(*val); ++val) { };
+	if (*val++ != '}')
+		return -EINVAL;
+
+	for ( ; isspace(*val); ++val) { };
+	if (*val++ != '\0')
+		return -EINVAL;
+
+	return 0;
+}
+
+static void video_set_control(struct device *dev, unsigned int id,
+			      const char *val)
+{
+	struct v4l2_query_ext_ctrl query;
+	struct v4l2_ext_control ctrl;
+	char *endptr;
+	int ret;
+
+	ret = query_control(dev, id, &query);
+	if (ret < 0)
+		return;
+
+	memset(&ctrl, 0, sizeof(ctrl));
+
+	if (query.nr_of_dims == 0) {
+		switch (query.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:
+			ctrl.value = strtol(val, &endptr, 0);
+			if (*endptr != 0) {
+				printf("Invalid control value '%s'\n", val);
+				return;
+			}
+			break;
+		case V4L2_CTRL_TYPE_INTEGER64:
+			ctrl.value64 = strtoll(val, &endptr, 0);
+			if (*endptr != 0) {
+				printf("Invalid control value '%s'\n", val);
+				return;
+			}
+			break;
+		case V4L2_CTRL_TYPE_STRING:
+			ctrl.size = query.elem_size;
+			ctrl.ptr = malloc(ctrl.size);
+			if (ctrl.ptr == NULL)
+				return;
+			strncpy(ctrl.string, val, ctrl.size);
+			break;
+		default:
+			printf("Unsupported control type\n");
+			return;
+		}
+	} else {
+		switch (query.type) {
+		case V4L2_CTRL_TYPE_U8:
+		case V4L2_CTRL_TYPE_U16:
+		case V4L2_CTRL_TYPE_U32:
+			ctrl.size = query.elem_size * query.elems;
+			ctrl.ptr = malloc(ctrl.size);
+			if (ctrl.ptr == NULL)
+				return;
+			ret = video_parse_control_array(&query, &ctrl, val);
+			if (ret < 0) {
+				printf("Invalid compound control value '%s'\n", val);
+				return;
+			}
+			break;
+		default:
+			printf("Unsupported control type %u\n", query.type);
+			break;
+		}
+	}
+
+	ret = set_control(dev, &query, &ctrl);
+	if (ret < 0) {
+		printf("unable to set control 0x%8.8x: %s (%d).\n",
+			id, strerror(errno), errno);
+	} else {
+		printf("Control 0x%08x set to %s, is ", id, val);
+
+		video_print_control_value(&query, &ctrl);
+		printf("\n");
+	}
+
+	if ((query.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) && ctrl.ptr)
+		free(ctrl.ptr);
 }
 
 static void video_list_controls(struct device *dev)
 {
 	int ret;
 
-	ret = video_for_each_control(dev, __video_print_control);
+	ret = video_for_each_control(dev, __video_get_control);
 	if (ret < 0)
 		return;
 
@@ -2064,7 +2184,7 @@ int main(int argc, char *argv[])
 
 	/* Controls */
 	int ctrl_name = 0;
-	int ctrl_value = 0;
+	const char *ctrl_value = NULL;
 
 	/* Video buffers */
 	enum v4l2_memory memtype = V4L2_MEMORY_MMAP;
@@ -2204,11 +2324,7 @@ int main(int argc, char *argv[])
 				printf("Invalid control name '%s'\n", optarg);
 				return 1;
 			}
-			ctrl_value = strtol(endptr + 1, &endptr, 0);
-			if (*endptr != 0) {
-				printf("Invalid control value '%s'\n", optarg);
-				return 1;
-			}
+			ctrl_value = endptr + 1;
 			do_set_control = 1;
 			break;
 		case OPT_BUFFER_SIZE:
@@ -2324,11 +2440,11 @@ int main(int argc, char *argv[])
 
 		ret = query_control(&dev, ctrl_name, &query);
 		if (ret == 0)
-			video_print_control(&dev, &query, false);
+			video_get_control(&dev, &query, false);
 	}
 
 	if (do_set_control)
-		set_control(&dev, ctrl_name, ctrl_value);
+		video_set_control(&dev, ctrl_name, ctrl_value);
 
 	if (do_list_controls)
 		video_list_controls(&dev);
-- 
Regards,

Laurent Pinchart

[PATCH yavta 5/7] Add support to reset device controls

[Laurent Pinchart]

From: Kieran Bingham <kieran.bingham@ideasonboard.com>

Provide a new option '--reset-controls' which will enumerate the
available controls on a device or sub-device, and re-initialise them to
defaults.

Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 yavta.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 59 insertions(+), 3 deletions(-)

diff --git a/yavta.c b/yavta.c
index d1bfd380c03b..1490878c6f7e 100644
--- a/yavta.c
+++ b/yavta.c
@@ -527,7 +527,8 @@ static int query_control(struct device *dev, unsigned int id,
 
 static int get_control(struct device *dev,
 		       const struct v4l2_query_ext_ctrl *query,
-		       struct v4l2_ext_control *ctrl)
+		       struct v4l2_ext_control *ctrl,
+		       unsigned int which)
 {
 	struct v4l2_ext_controls ctrls;
 	struct v4l2_control old;
@@ -536,7 +537,7 @@ static int get_control(struct device *dev,
 	memset(&ctrls, 0, sizeof(ctrls));
 	memset(ctrl, 0, sizeof(*ctrl));
 
-	ctrls.ctrl_class = V4L2_CTRL_ID2CLASS(query->id);
+	ctrls.which = which;
 	ctrls.count = 1;
 	ctrls.controls = ctrl;
 
@@ -1295,7 +1296,7 @@ static int video_get_control(struct device *dev,
 
 	printf("current ");
 
-	ret = get_control(dev, query, &ctrl);
+	ret = get_control(dev, query, &ctrl, V4L2_CTRL_WHICH_CUR_VAL);
 	if (ret < 0) {
 		printf("n/a\n");
 		printf("unable to get control 0x%8.8x: %s (%d).\n",
@@ -1478,6 +1479,51 @@ static void video_list_controls(struct device *dev)
 		printf("No control found.\n");
 }
 
+static int video_reset_control(struct device *dev,
+			       const struct v4l2_query_ext_ctrl *query)
+{
+       struct v4l2_ext_control ctrl = { .value = query->default_value, };
+       int ret;
+
+	if (query->flags & V4L2_CTRL_FLAG_DISABLED)
+		return 0;
+
+	if (query->type == V4L2_CTRL_TYPE_CTRL_CLASS)
+		return 0;
+
+	/*
+	 * For controls with payloads the default value must be retrieved with
+	 * a VIDIOC_G_EXT_CTRLS call. If the V4L2_CTRL_WHICH_DEF_VAL flag isn't
+	 * supported by the kernel (it got introduced in v4.5, while controls
+	 * with payloads were introduced in v3.17), there isn't much we can do,
+	 * so skip resetting the control.
+	 */
+	if (query->flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) {
+		ret = get_control(dev, query, &ctrl, V4L2_CTRL_WHICH_DEF_VAL);
+		if (ret < 0)
+			return 0;
+	}
+
+	set_control(dev, query, &ctrl);
+
+	if (query->flags & V4L2_CTRL_FLAG_HAS_PAYLOAD)
+		free(ctrl.ptr);
+
+	return 1;
+}
+
+static void video_reset_controls(struct device *dev)
+{
+	int ret;
+
+	ret = video_for_each_control(dev, video_reset_control);
+	if (ret < 0)
+		return;
+
+	if (ret)
+		printf("%u control%s reset.\n", ret, ret > 1 ? "s" : "");
+}
+
 static void video_enum_frame_intervals(struct device *dev, __u32 pixelformat,
 	unsigned int width, unsigned int height)
 {
@@ -2099,6 +2145,7 @@ static void usage(const char *argv0)
 	printf("    --premultiplied		Color components are premultiplied by alpha value\n");
 	printf("    --queue-late		Queue buffers after streamon, not before\n");
 	printf("    --requeue-last		Requeue the last buffers before streamoff\n");
+	printf("    --reset-controls		Reset all available controls to their default value\n");
 	printf("    --timestamp-source		Set timestamp source on output buffers [eof, soe]\n");
 	printf("    --skip n			Skip the first n frames\n");
 	printf("    --sleep-forever		Sleep forever after configuring the device\n");
@@ -2121,6 +2168,7 @@ static void usage(const char *argv0)
 #define OPT_PREMULTIPLIED	269
 #define OPT_QUEUE_LATE		270
 #define OPT_DATA_PREFIX		271
+#define OPT_RESET_CONTROLS	272
 
 static struct option opts[] = {
 	{"buffer-size", 1, 0, OPT_BUFFER_SIZE},
@@ -2149,6 +2197,7 @@ static struct option opts[] = {
 	{"queue-late", 0, 0, OPT_QUEUE_LATE},
 	{"get-control", 1, 0, 'r'},
 	{"requeue-last", 0, 0, OPT_REQUEUE_LAST},
+	{"reset-controls", 0, 0, OPT_RESET_CONTROLS},
 	{"realtime", 2, 0, 'R'},
 	{"size", 1, 0, 's'},
 	{"set-control", 1, 0, 'w'},
@@ -2176,6 +2225,7 @@ int main(int argc, char *argv[])
 	int do_enum_formats = 0, do_set_format = 0;
 	int do_enum_inputs = 0, do_set_input = 0;
 	int do_list_controls = 0, do_get_control = 0, do_set_control = 0;
+	int do_reset_controls = 0;
 	int do_sleep_forever = 0, do_requeue_last = 0;
 	int do_rt = 0, do_log_status = 0;
 	int no_query = 0, do_queue_late = 0;
@@ -2364,6 +2414,9 @@ int main(int argc, char *argv[])
 		case OPT_QUEUE_LATE:
 			do_queue_late = 1;
 			break;
+		case OPT_RESET_CONTROLS:
+			do_reset_controls = 1;
+			break;
 		case OPT_REQUEUE_LAST:
 			do_requeue_last = 1;
 			break;
@@ -2449,6 +2502,9 @@ int main(int argc, char *argv[])
 	if (do_list_controls)
 		video_list_controls(&dev);
 
+	if (do_reset_controls)
+		video_reset_controls(&dev);
+
 	if (do_enum_formats) {
 		printf("- Available formats:\n");
 		video_enum_formats(&dev, V4L2_BUF_TYPE_VIDEO_CAPTURE);
-- 
Regards,

Laurent Pinchart

[PATCH yavta 6/7] Support setting control from values stored in a file

[Laurent Pinchart]

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 yavta.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/yavta.c b/yavta.c
index 1490878c6f7e..2d49131a4271 100644
--- a/yavta.c
+++ b/yavta.c
@@ -1334,6 +1334,31 @@ static int video_parse_control_array(const struct v4l2_query_ext_ctrl *query,
 	__u32 value;
 
 	for ( ; isspace(*val); ++val) { };
+
+	if (*val == '<') {
+		/* Read the control value from the given file. */
+		ssize_t size;
+		int fd;
+
+		val++;
+		fd = open(val, O_RDONLY);
+		if (fd < 0) {
+			printf("unable to open control file `%s'\n", val);
+			return -EINVAL;
+		}
+
+		size = read(fd, ctrl->ptr, ctrl->size);
+		if (size != (ssize_t)ctrl->size) {
+			printf("error reading control file `%s' (%s)\n", val,
+			       strerror(errno));
+			close(fd);
+			return -EINVAL;
+		}
+
+		close(fd);
+		return 0;
+	}
+
 	if (*val++ != '{')
 		return -EINVAL;
 
-- 
Regards,

Laurent Pinchart

[PATCH yavta 7/7] Remove unneeded conditional compilation for old V4L2 API versions

[Laurent Pinchart]

As we include a copy of the V4L2 kernel headers, there's no need for
conditional compilation to support old versions of the API.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 yavta.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/yavta.c b/yavta.c
index 2d49131a4271..741df82a8db0 100644
--- a/yavta.c
+++ b/yavta.c
@@ -40,10 +40,6 @@
 
 #include <linux/videodev2.h>
 
-#ifndef V4L2_BUF_FLAG_ERROR
-#define V4L2_BUF_FLAG_ERROR	0x0040
-#endif
-
 #define ARRAY_SIZE(a)	(sizeof(a)/sizeof((a)[0]))
 
 enum buffer_fill_mode
@@ -1142,16 +1138,9 @@ static int video_for_each_control(struct device *dev,
 	unsigned int id;
 	int ret;
 
-#ifndef V4L2_CTRL_FLAG_NEXT_CTRL
-	unsigned int i;
-
-	for (i = V4L2_CID_BASE; i <= V4L2_CID_LASTP1; ++i) {
-		id = i;
-#else
 	id = 0;
 	while (1) {
 		id |= V4L2_CTRL_FLAG_NEXT_CTRL | V4L2_CTRL_FLAG_NEXT_COMPOUND;
-#endif
 
 		ret = query_control(dev, id, &query);
 		if (ret == -EINVAL)
-- 
Regards,

Laurent Pinchart

Re: [PATCH yavta 1/7] yavta: Refactor video_list_controls()

[Sakari Ailus]

Hi Laurent,

Thanks for the patchset.

On Wed, Feb 20, 2019 at 02:51:17PM +0200, Laurent Pinchart wrote:
> Separate iteration over controls from printing, in order to reuse the
> iteration to implement control reset.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  yavta.c | 133 ++++++++++++++++++++++++++++++++++----------------------
>  1 file changed, 82 insertions(+), 51 deletions(-)
> 
> diff --git a/yavta.c b/yavta.c
> index 2d3b2d096f7d..98bc09810ff1 100644
> --- a/yavta.c
> +++ b/yavta.c
> @@ -484,9 +484,12 @@ static int query_control(struct device *dev, unsigned int id,
>  	query->id = id;
>  
>  	ret = ioctl(dev->fd, VIDIOC_QUERYCTRL, query);
> -	if (ret < 0 && errno != EINVAL)
> -		printf("unable to query control 0x%8.8x: %s (%d).\n",
> -		       id, strerror(errno), errno);
> +	if (ret < 0) {
> +		ret = -errno;
> +		if (ret != -EINVAL)
> +			printf("unable to query control 0x%8.8x: %s (%d).\n",
> +			       id, strerror(errno), errno);
> +	}
>  
>  	return ret;
>  }
> @@ -1120,7 +1123,45 @@ static int video_enable(struct device *dev, int enable)
>  	return 0;
>  }
>  
> -static void video_query_menu(struct device *dev, struct v4l2_queryctrl *query,
> +static int video_for_each_control(struct device *dev,
> +				  int(*callback)(struct device *dev, const struct v4l2_queryctrl *query))

This is over 80 characters per line. How about wrapping? Also int on the
above line is desperate for some breathing room before the opening
parenthesis.

> +{
> +	struct v4l2_queryctrl query;
> +	unsigned int nctrls = 0;
> +	unsigned int id;
> +	int ret;
> +
> +#ifndef V4L2_CTRL_FLAG_NEXT_CTRL

This was added back in 2012. Do you think it's still worth checking for it?

Not related to this patch though, just a general remark.

> +	unsigned int i;
> +
> +	for (i = V4L2_CID_BASE; i <= V4L2_CID_LASTP1; ++i) {
> +		id = i;
> +#else
> +	id = 0;
> +	while (1) {
> +		id |= V4L2_CTRL_FLAG_NEXT_CTRL;
> +#endif
> +
> +		ret = query_control(dev, id, &query);
> +		if (ret == -EINVAL)
> +			break;
> +		if (ret < 0)
> +			return ret;
> +
> +		id = query.id;
> +
> +		ret = callback(dev, &query);
> +		if (ret < 0)
> +			return ret;
> +
> +		if (ret)
> +			nctrls++;
> +	}
> +
> +	return nctrls;
> +}
> +
> +static void video_query_menu(struct device *dev, const struct v4l2_queryctrl *query,
>  			     unsigned int value)
>  {
>  	struct v4l2_querymenu menu;
> @@ -1142,83 +1183,68 @@ static void video_query_menu(struct device *dev, struct v4l2_queryctrl *query,
>  	};
>  }
>  
> -static int video_print_control(struct device *dev, unsigned int id, bool full)
> +static int video_print_control(struct device *dev,
> +			       const struct v4l2_queryctrl *query, bool full)
>  {
>  	struct v4l2_ext_control ctrl;
> -	struct v4l2_queryctrl query;
>  	char sval[24];
>  	char *current = sval;
>  	int ret;
>  
> -	ret = query_control(dev, id, &query);
> -	if (ret < 0)
> -		return ret;
> +	if (query->flags & V4L2_CTRL_FLAG_DISABLED)
> +		return 0;
>  
> -	if (query.flags & V4L2_CTRL_FLAG_DISABLED)
> -		return query.id;
> -
> -	if (query.type == V4L2_CTRL_TYPE_CTRL_CLASS) {
> -		printf("--- %s (class 0x%08x) ---\n", query.name, query.id);
> -		return query.id;
> +	if (query->type == V4L2_CTRL_TYPE_CTRL_CLASS) {
> +		printf("--- %s (class 0x%08x) ---\n", query->name, query->id);
> +		return 0;
>  	}
>  
> -	ret = get_control(dev, &query, &ctrl);
> +	ret = get_control(dev, query, &ctrl);
>  	if (ret < 0)
>  		strcpy(sval, "n/a");
> -	else if (query.type == V4L2_CTRL_TYPE_INTEGER64)
> +	else if (query->type == V4L2_CTRL_TYPE_INTEGER64)
>  		sprintf(sval, "%lld", ctrl.value64);
> -	else if (query.type == V4L2_CTRL_TYPE_STRING)
> +	else if (query->type == V4L2_CTRL_TYPE_STRING)
>  		current = ctrl.string;
>  	else
>  		sprintf(sval, "%d", ctrl.value);
>  
>  	if (full)
>  		printf("control 0x%08x `%s' min %d max %d step %d default %d current %s.\n",
> -			query.id, query.name, query.minimum, query.maximum,
> -			query.step, query.default_value, current);
> +			query->id, query->name, query->minimum, query->maximum,
> +			query->step, query->default_value, current);
>  	else
> -		printf("control 0x%08x current %s.\n", query.id, current);
> +		printf("control 0x%08x current %s.\n", query->id, current);
>  
> -	if (query.type == V4L2_CTRL_TYPE_STRING)
> +	if (query->type == V4L2_CTRL_TYPE_STRING)
>  		free(ctrl.string);
>  
>  	if (!full)
> -		return query.id;
> +		return 1;
>  
> -	if (query.type == V4L2_CTRL_TYPE_MENU ||
> -	    query.type == V4L2_CTRL_TYPE_INTEGER_MENU)
> -		video_query_menu(dev, &query, ctrl.value);
> +	if (query->type == V4L2_CTRL_TYPE_MENU ||
> +	    query->type == V4L2_CTRL_TYPE_INTEGER_MENU)
> +		video_query_menu(dev, query, ctrl.value);
>  
> -	return query.id;
> +	return 1;
> +}
> +
> +static int __video_print_control(struct device *dev,
> +				 const struct v4l2_queryctrl *query)
> +{
> +	return video_print_control(dev, query, true);
>  }
>  
>  static void video_list_controls(struct device *dev)
>  {
> -	unsigned int nctrls = 0;
> -	unsigned int id;
>  	int ret;
>  
> -#ifndef V4L2_CTRL_FLAG_NEXT_CTRL
> -	unsigned int i;
> +	ret = video_for_each_control(dev, __video_print_control);
> +	if (ret < 0)
> +		return;
>  
> -	for (i = V4L2_CID_BASE; i <= V4L2_CID_LASTP1; ++i) {
> -		id = i;
> -#else
> -	id = 0;
> -	while (1) {
> -		id |= V4L2_CTRL_FLAG_NEXT_CTRL;
> -#endif
> -
> -		ret = video_print_control(dev, id, true);
> -		if (ret < 0)
> -			break;
> -
> -		id = ret;
> -		nctrls++;
> -	}
> -
> -	if (nctrls)
> -		printf("%u control%s found.\n", nctrls, nctrls > 1 ? "s" : "");
> +	if (ret)
> +		printf("%u control%s found.\n", ret, ret > 1 ? "s" : "");
>  	else
>  		printf("No control found.\n");
>  }
> @@ -2184,8 +2210,13 @@ int main(int argc, char *argv[])
>  	if (do_log_status)
>  		video_log_status(&dev);
>  
> -	if (do_get_control)
> -		video_print_control(&dev, ctrl_name, false);
> +	if (do_get_control) {
> +		struct v4l2_queryctrl query;
> +
> +		ret = query_control(&dev, ctrl_name, &query);
> +		if (ret == 0)
> +			video_print_control(&dev, &query, false);
> +	}
>  
>  	if (do_set_control)
>  		set_control(&dev, ctrl_name, ctrl_value);

-- 
Cheers,

Sakari Ailus

Re: [PATCH yavta 3/7] Implement compound control get support

[Sakari Ailus]

Hi Laurent,

On Wed, Feb 20, 2019 at 02:51:19PM +0200, Laurent Pinchart wrote:
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  yavta.c | 154 ++++++++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 115 insertions(+), 39 deletions(-)
> 
> diff --git a/yavta.c b/yavta.c
> index eb50d592736f..6428c22f88d7 100644
> --- a/yavta.c
> +++ b/yavta.c
> @@ -529,6 +529,7 @@ static int get_control(struct device *dev,
>  		       struct v4l2_ext_control *ctrl)
>  {
>  	struct v4l2_ext_controls ctrls;
> +	struct v4l2_control old;
>  	int ret;
>  
>  	memset(&ctrls, 0, sizeof(ctrls));
> @@ -540,34 +541,32 @@ static int get_control(struct device *dev,
>  
>  	ctrl->id = query->id;
>  
> -	if (query->type == V4L2_CTRL_TYPE_STRING) {
> -		ctrl->string = malloc(query->maximum + 1);
> -		if (ctrl->string == NULL)
> +	if (query->flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) {

This breaks string controls for kernels that don't have
V4L2_CTRL_FLAG_HAS_PAYLOAD. As you still support kernels that have no
V4L2_CTRL_FLAG_NEXT_CTRL, how about checking for string type specifically?

> +		ctrl->size = query->elems * query->elem_size;
> +		ctrl->ptr = malloc(ctrl->size);
> +		if (ctrl->ptr == NULL)
>  			return -ENOMEM;
> -
> -		ctrl->size = query->maximum + 1;
>  	}
>  
>  	ret = ioctl(dev->fd, VIDIOC_G_EXT_CTRLS, &ctrls);
>  	if (ret != -1)
>  		return 0;
>  
> -	if (query->type != V4L2_CTRL_TYPE_INTEGER64 &&
> -	    query->type != V4L2_CTRL_TYPE_STRING &&
> -	    (errno == EINVAL || errno == ENOTTY)) {
> -		struct v4l2_control old;
> +	if (query->flags & V4L2_CTRL_FLAG_HAS_PAYLOAD)
> +		free(ctrl->ptr);
>  
> -		old.id = query->id;
> -		ret = ioctl(dev->fd, VIDIOC_G_CTRL, &old);
> -		if (ret != -1) {
> -			ctrl->value = old.value;
> -			return 0;
> -		}
> -	}
> +	if (query->flags & V4L2_CTRL_FLAG_HAS_PAYLOAD ||
> +	    query->type == V4L2_CTRL_TYPE_INTEGER64 ||
> +	    (errno != EINVAL && errno != ENOTTY))
> +		return -errno;
>  
> -	printf("unable to get control 0x%8.8x: %s (%d).\n",
> -		query->id, strerror(errno), errno);
> -	return -1;
> +	old.id = query->id;
> +	ret = ioctl(dev->fd, VIDIOC_G_CTRL, &old);
> +	if (ret < 0)
> +		return -errno;
> +
> +	ctrl->value = old.value;
> +	return 0;
>  }
>  
>  static void set_control(struct device *dev, unsigned int id,
> @@ -1170,7 +1169,7 @@ static int video_for_each_control(struct device *dev,
>  #else
>  	id = 0;
>  	while (1) {
> -		id |= V4L2_CTRL_FLAG_NEXT_CTRL;
> +		id |= V4L2_CTRL_FLAG_NEXT_CTRL | V4L2_CTRL_FLAG_NEXT_COMPOUND;
>  #endif
>  
>  		ret = query_control(dev, id, &query);
> @@ -1215,13 +1214,76 @@ static void video_query_menu(struct device *dev,
>  	};
>  }
>  
> +static void video_print_control_array(const struct v4l2_query_ext_ctrl *query,
> +				      struct v4l2_ext_control *ctrl)
> +{
> +	unsigned int i;
> +
> +	printf("{");

A space would be nice after the opening brace, also before the closing one.

> +
> +	for (i = 0; i < query->elems; ++i) {
> +		switch (query->type) {
> +		case V4L2_CTRL_TYPE_U8:
> +			printf("%u", ctrl->p_u8[i]);
> +			break;
> +		case V4L2_CTRL_TYPE_U16:
> +			printf("%u", ctrl->p_u16[i]);
> +			break;
> +		case V4L2_CTRL_TYPE_U32:
> +			printf("%u", ctrl->p_u32[i]);
> +			break;
> +		}
> +
> +		if (i != query->elems - 1)
> +			printf(", ");
> +	}
> +
> +	printf("}");
> +}
> +
> +static void video_print_control_value(const struct v4l2_query_ext_ctrl *query,
> +				      struct v4l2_ext_control *ctrl)
> +{
> +	if (query->nr_of_dims == 0) {
> +		switch (query->type) {
> +		case V4L2_CTRL_TYPE_INTEGER:
> +		case V4L2_CTRL_TYPE_BOOLEAN:
> +		case V4L2_CTRL_TYPE_MENU:
> +		case V4L2_CTRL_TYPE_INTEGER_MENU:
> +			printf("%d", ctrl->value);
> +			break;
> +		case V4L2_CTRL_TYPE_BITMASK:
> +			printf("0x%08x", ctrl->value);

A cast to unsigned here?

> +			break;
> +		case V4L2_CTRL_TYPE_INTEGER64:
> +			printf("%lld", ctrl->value64);
> +			break;
> +		case V4L2_CTRL_TYPE_STRING:
> +			printf("%s", ctrl->string);
> +			break;
> +		}
> +
> +		return;
> +	}
> +
> +	switch (query->type) {
> +	case V4L2_CTRL_TYPE_U8:
> +	case V4L2_CTRL_TYPE_U16:
> +	case V4L2_CTRL_TYPE_U32:
> +		video_print_control_array(query, ctrl);
> +		break;
> +	default:
> +		printf("unsupported");

How about printing the unsupported type?

> +		break;
> +	}
> +}
> +
>  static int video_print_control(struct device *dev,
>  			       const struct v4l2_query_ext_ctrl *query,
>  			       bool full)
>  {
>  	struct v4l2_ext_control ctrl;
> -	char sval[24];
> -	char *current = sval;
> +	unsigned int i;
>  	int ret;
>  
>  	if (query->flags & V4L2_CTRL_FLAG_DISABLED)
> @@ -1232,25 +1294,39 @@ static int video_print_control(struct device *dev,
>  		return 0;
>  	}
>  
> -	ret = get_control(dev, query, &ctrl);
> -	if (ret < 0)
> -		strcpy(sval, "n/a");
> -	else if (query->type == V4L2_CTRL_TYPE_INTEGER64)
> -		sprintf(sval, "%lld", ctrl.value64);
> -	else if (query->type == V4L2_CTRL_TYPE_STRING)
> -		current = ctrl.string;
> -	else
> -		sprintf(sval, "%d", ctrl.value);
> -
> -	if (full)
> -		printf("control 0x%08x `%s' min %lld max %lld step %lld default %lld current %s.\n",
> +	if (full) {
> +		printf("control 0x%08x `%s' min %lld max %lld step %lld default %lld ",
>  			query->id, query->name, query->minimum, query->maximum,
> -			query->step, query->default_value, current);
> -	else
> -		printf("control 0x%08x current %s.\n", query->id, current);
> +			query->step, query->default_value);
> +		if (query->nr_of_dims) {
> +			for (i = 0; i < query->nr_of_dims; ++i)
> +				printf("[%u]", query->dims[i]);
> +			printf(" ");
> +		}
> +	} else {
> +		printf("control 0x%08x ", query->id);
> +	}
>  
> -	if (query->type == V4L2_CTRL_TYPE_STRING)
> -		free(ctrl.string);
> +	if (query->type == V4L2_CTRL_TYPE_BUTTON) {
> +		/* Button controls have no current value. */
> +		printf("\n");
> +		return 1;
> +	}
> +
> +	printf("current ");
> +
> +	ret = get_control(dev, query, &ctrl);
> +	if (ret < 0) {
> +		printf("n/a\n");
> +		printf("unable to get control 0x%8.8x: %s (%d).\n",
> +			query->id, strerror(errno), errno);
> +	} else {
> +		video_print_control_value(query, &ctrl);
> +		printf("\n");
> +	}
> +
> +	if (query->flags & V4L2_CTRL_FLAG_HAS_PAYLOAD)
> +		free(ctrl.ptr);
>  
>  	if (!full)
>  		return 1;

-- 
Regards,

Sakari Ailus

Re: [PATCH yavta 1/7] yavta: Refactor video_list_controls()

[Laurent Pinchart]

Hi Sakari,

On Wed, Feb 20, 2019 at 03:21:57PM +0200, Sakari Ailus wrote:
> On Wed, Feb 20, 2019 at 02:51:17PM +0200, Laurent Pinchart wrote:
> > Separate iteration over controls from printing, in order to reuse the
> > iteration to implement control reset.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  yavta.c | 133 ++++++++++++++++++++++++++++++++++----------------------
> >  1 file changed, 82 insertions(+), 51 deletions(-)
> > 
> > diff --git a/yavta.c b/yavta.c
> > index 2d3b2d096f7d..98bc09810ff1 100644
> > --- a/yavta.c
> > +++ b/yavta.c
> > @@ -484,9 +484,12 @@ static int query_control(struct device *dev, unsigned int id,
> >  	query->id = id;
> >  
> >  	ret = ioctl(dev->fd, VIDIOC_QUERYCTRL, query);
> > -	if (ret < 0 && errno != EINVAL)
> > -		printf("unable to query control 0x%8.8x: %s (%d).\n",
> > -		       id, strerror(errno), errno);
> > +	if (ret < 0) {
> > +		ret = -errno;
> > +		if (ret != -EINVAL)
> > +			printf("unable to query control 0x%8.8x: %s (%d).\n",
> > +			       id, strerror(errno), errno);
> > +	}
> >  
> >  	return ret;
> >  }
> > @@ -1120,7 +1123,45 @@ static int video_enable(struct device *dev, int enable)
> >  	return 0;
> >  }
> >  
> > -static void video_query_menu(struct device *dev, struct v4l2_queryctrl *query,
> > +static int video_for_each_control(struct device *dev,
> > +				  int(*callback)(struct device *dev, const struct v4l2_queryctrl *query))
> 
> This is over 80 characters per line. How about wrapping? Also int on the
> above line is desperate for some breathing room before the opening
> parenthesis.

One option would be to turn the callback into a typedef.

I'm thinking about doing some refactoring in yavta, possibly splitting
it in multiple source files, as it's getting quite big. Control handling
is a candidate to be moved to a separate file. What do you think ?

I'm also wondering whether I should enumerate controls when opening the
device and caching them, to operate on the cache later on.

> > +{
> > +	struct v4l2_queryctrl query;
> > +	unsigned int nctrls = 0;
> > +	unsigned int id;
> > +	int ret;
> > +
> > +#ifndef V4L2_CTRL_FLAG_NEXT_CTRL
> 
> This was added back in 2012. Do you think it's still worth checking for it?
> 
> Not related to this patch though, just a general remark.

Please see patch 7/7 :-)

> > +	unsigned int i;
> > +
> > +	for (i = V4L2_CID_BASE; i <= V4L2_CID_LASTP1; ++i) {
> > +		id = i;
> > +#else
> > +	id = 0;
> > +	while (1) {
> > +		id |= V4L2_CTRL_FLAG_NEXT_CTRL;
> > +#endif
> > +
> > +		ret = query_control(dev, id, &query);
> > +		if (ret == -EINVAL)
> > +			break;
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		id = query.id;
> > +
> > +		ret = callback(dev, &query);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		if (ret)
> > +			nctrls++;
> > +	}
> > +
> > +	return nctrls;
> > +}
> > +
> > +static void video_query_menu(struct device *dev, const struct v4l2_queryctrl *query,
> >  			     unsigned int value)
> >  {
> >  	struct v4l2_querymenu menu;
> > @@ -1142,83 +1183,68 @@ static void video_query_menu(struct device *dev, struct v4l2_queryctrl *query,
> >  	};
> >  }
> >  
> > -static int video_print_control(struct device *dev, unsigned int id, bool full)
> > +static int video_print_control(struct device *dev,
> > +			       const struct v4l2_queryctrl *query, bool full)
> >  {
> >  	struct v4l2_ext_control ctrl;
> > -	struct v4l2_queryctrl query;
> >  	char sval[24];
> >  	char *current = sval;
> >  	int ret;
> >  
> > -	ret = query_control(dev, id, &query);
> > -	if (ret < 0)
> > -		return ret;
> > +	if (query->flags & V4L2_CTRL_FLAG_DISABLED)
> > +		return 0;
> >  
> > -	if (query.flags & V4L2_CTRL_FLAG_DISABLED)
> > -		return query.id;
> > -
> > -	if (query.type == V4L2_CTRL_TYPE_CTRL_CLASS) {
> > -		printf("--- %s (class 0x%08x) ---\n", query.name, query.id);
> > -		return query.id;
> > +	if (query->type == V4L2_CTRL_TYPE_CTRL_CLASS) {
> > +		printf("--- %s (class 0x%08x) ---\n", query->name, query->id);
> > +		return 0;
> >  	}
> >  
> > -	ret = get_control(dev, &query, &ctrl);
> > +	ret = get_control(dev, query, &ctrl);
> >  	if (ret < 0)
> >  		strcpy(sval, "n/a");
> > -	else if (query.type == V4L2_CTRL_TYPE_INTEGER64)
> > +	else if (query->type == V4L2_CTRL_TYPE_INTEGER64)
> >  		sprintf(sval, "%lld", ctrl.value64);
> > -	else if (query.type == V4L2_CTRL_TYPE_STRING)
> > +	else if (query->type == V4L2_CTRL_TYPE_STRING)
> >  		current = ctrl.string;
> >  	else
> >  		sprintf(sval, "%d", ctrl.value);
> >  
> >  	if (full)
> >  		printf("control 0x%08x `%s' min %d max %d step %d default %d current %s.\n",
> > -			query.id, query.name, query.minimum, query.maximum,
> > -			query.step, query.default_value, current);
> > +			query->id, query->name, query->minimum, query->maximum,
> > +			query->step, query->default_value, current);
> >  	else
> > -		printf("control 0x%08x current %s.\n", query.id, current);
> > +		printf("control 0x%08x current %s.\n", query->id, current);
> >  
> > -	if (query.type == V4L2_CTRL_TYPE_STRING)
> > +	if (query->type == V4L2_CTRL_TYPE_STRING)
> >  		free(ctrl.string);
> >  
> >  	if (!full)
> > -		return query.id;
> > +		return 1;
> >  
> > -	if (query.type == V4L2_CTRL_TYPE_MENU ||
> > -	    query.type == V4L2_CTRL_TYPE_INTEGER_MENU)
> > -		video_query_menu(dev, &query, ctrl.value);
> > +	if (query->type == V4L2_CTRL_TYPE_MENU ||
> > +	    query->type == V4L2_CTRL_TYPE_INTEGER_MENU)
> > +		video_query_menu(dev, query, ctrl.value);
> >  
> > -	return query.id;
> > +	return 1;
> > +}
> > +
> > +static int __video_print_control(struct device *dev,
> > +				 const struct v4l2_queryctrl *query)
> > +{
> > +	return video_print_control(dev, query, true);
> >  }
> >  
> >  static void video_list_controls(struct device *dev)
> >  {
> > -	unsigned int nctrls = 0;
> > -	unsigned int id;
> >  	int ret;
> >  
> > -#ifndef V4L2_CTRL_FLAG_NEXT_CTRL
> > -	unsigned int i;
> > +	ret = video_for_each_control(dev, __video_print_control);
> > +	if (ret < 0)
> > +		return;
> >  
> > -	for (i = V4L2_CID_BASE; i <= V4L2_CID_LASTP1; ++i) {
> > -		id = i;
> > -#else
> > -	id = 0;
> > -	while (1) {
> > -		id |= V4L2_CTRL_FLAG_NEXT_CTRL;
> > -#endif
> > -
> > -		ret = video_print_control(dev, id, true);
> > -		if (ret < 0)
> > -			break;
> > -
> > -		id = ret;
> > -		nctrls++;
> > -	}
> > -
> > -	if (nctrls)
> > -		printf("%u control%s found.\n", nctrls, nctrls > 1 ? "s" : "");
> > +	if (ret)
> > +		printf("%u control%s found.\n", ret, ret > 1 ? "s" : "");
> >  	else
> >  		printf("No control found.\n");
> >  }
> > @@ -2184,8 +2210,13 @@ int main(int argc, char *argv[])
> >  	if (do_log_status)
> >  		video_log_status(&dev);
> >  
> > -	if (do_get_control)
> > -		video_print_control(&dev, ctrl_name, false);
> > +	if (do_get_control) {
> > +		struct v4l2_queryctrl query;
> > +
> > +		ret = query_control(&dev, ctrl_name, &query);
> > +		if (ret == 0)
> > +			video_print_control(&dev, &query, false);
> > +	}
> >  
> >  	if (do_set_control)
> >  		set_control(&dev, ctrl_name, ctrl_value);

-- 
Regards,

Laurent Pinchart

Re: [PATCH yavta 1/7] yavta: Refactor video_list_controls()

[Sakari Ailus]

Hi Laurent,

On Wed, Feb 20, 2019 at 04:07:53PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Wed, Feb 20, 2019 at 03:21:57PM +0200, Sakari Ailus wrote:
> > On Wed, Feb 20, 2019 at 02:51:17PM +0200, Laurent Pinchart wrote:
> > > Separate iteration over controls from printing, in order to reuse the
> > > iteration to implement control reset.
> > > 
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > >  yavta.c | 133 ++++++++++++++++++++++++++++++++++----------------------
> > >  1 file changed, 82 insertions(+), 51 deletions(-)
> > > 
> > > diff --git a/yavta.c b/yavta.c
> > > index 2d3b2d096f7d..98bc09810ff1 100644
> > > --- a/yavta.c
> > > +++ b/yavta.c
> > > @@ -484,9 +484,12 @@ static int query_control(struct device *dev, unsigned int id,
> > >  	query->id = id;
> > >  
> > >  	ret = ioctl(dev->fd, VIDIOC_QUERYCTRL, query);
> > > -	if (ret < 0 && errno != EINVAL)
> > > -		printf("unable to query control 0x%8.8x: %s (%d).\n",
> > > -		       id, strerror(errno), errno);
> > > +	if (ret < 0) {
> > > +		ret = -errno;
> > > +		if (ret != -EINVAL)
> > > +			printf("unable to query control 0x%8.8x: %s (%d).\n",
> > > +			       id, strerror(errno), errno);
> > > +	}
> > >  
> > >  	return ret;
> > >  }
> > > @@ -1120,7 +1123,45 @@ static int video_enable(struct device *dev, int enable)
> > >  	return 0;
> > >  }
> > >  
> > > -static void video_query_menu(struct device *dev, struct v4l2_queryctrl *query,
> > > +static int video_for_each_control(struct device *dev,
> > > +				  int(*callback)(struct device *dev, const struct v4l2_queryctrl *query))
> > 
> > This is over 80 characters per line. How about wrapping? Also int on the
> > above line is desperate for some breathing room before the opening
> > parenthesis.
> 
> One option would be to turn the callback into a typedef.

Just wrap after "static void " as well as after "dev,"?

> 
> I'm thinking about doing some refactoring in yavta, possibly splitting
> it in multiple source files, as it's getting quite big. Control handling
> is a candidate to be moved to a separate file. What do you think ?

Yes, and perhaps buffer management as well. It's not yet *that* big after
all, but keeping the design clean indeed helps doing this later on.

> 
> I'm also wondering whether I should enumerate controls when opening the
> device and caching them, to operate on the cache later on.

I'd just try to keep it simple. So if it's more simple not to cache them,
I'd keep it that way. Just a thought.

> 
> > > +{
> > > +	struct v4l2_queryctrl query;
> > > +	unsigned int nctrls = 0;
> > > +	unsigned int id;
> > > +	int ret;
> > > +
> > > +#ifndef V4L2_CTRL_FLAG_NEXT_CTRL
> > 
> > This was added back in 2012. Do you think it's still worth checking for it?
> > 
> > Not related to this patch though, just a general remark.
> 
> Please see patch 7/7 :-)

Yes, I noticed that. Usually such cleanups are done first, not last, so I
didn't look. :-)

> 
> > > +	unsigned int i;
> > > +
> > > +	for (i = V4L2_CID_BASE; i <= V4L2_CID_LASTP1; ++i) {
> > > +		id = i;
> > > +#else
> > > +	id = 0;
> > > +	while (1) {
> > > +		id |= V4L2_CTRL_FLAG_NEXT_CTRL;
> > > +#endif
> > > +
> > > +		ret = query_control(dev, id, &query);
> > > +		if (ret == -EINVAL)
> > > +			break;
> > > +		if (ret < 0)
> > > +			return ret;
> > > +
> > > +		id = query.id;
> > > +
> > > +		ret = callback(dev, &query);
> > > +		if (ret < 0)
> > > +			return ret;
> > > +
> > > +		if (ret)
> > > +			nctrls++;
> > > +	}
> > > +
> > > +	return nctrls;
> > > +}
> > > +
> > > +static void video_query_menu(struct device *dev, const struct v4l2_queryctrl *query,
> > >  			     unsigned int value)
> > >  {
> > >  	struct v4l2_querymenu menu;
> > > @@ -1142,83 +1183,68 @@ static void video_query_menu(struct device *dev, struct v4l2_queryctrl *query,
> > >  	};
> > >  }
> > >  
> > > -static int video_print_control(struct device *dev, unsigned int id, bool full)
> > > +static int video_print_control(struct device *dev,
> > > +			       const struct v4l2_queryctrl *query, bool full)
> > >  {
> > >  	struct v4l2_ext_control ctrl;
> > > -	struct v4l2_queryctrl query;
> > >  	char sval[24];
> > >  	char *current = sval;
> > >  	int ret;
> > >  
> > > -	ret = query_control(dev, id, &query);
> > > -	if (ret < 0)
> > > -		return ret;
> > > +	if (query->flags & V4L2_CTRL_FLAG_DISABLED)
> > > +		return 0;
> > >  
> > > -	if (query.flags & V4L2_CTRL_FLAG_DISABLED)
> > > -		return query.id;
> > > -
> > > -	if (query.type == V4L2_CTRL_TYPE_CTRL_CLASS) {
> > > -		printf("--- %s (class 0x%08x) ---\n", query.name, query.id);
> > > -		return query.id;
> > > +	if (query->type == V4L2_CTRL_TYPE_CTRL_CLASS) {
> > > +		printf("--- %s (class 0x%08x) ---\n", query->name, query->id);
> > > +		return 0;
> > >  	}
> > >  
> > > -	ret = get_control(dev, &query, &ctrl);
> > > +	ret = get_control(dev, query, &ctrl);
> > >  	if (ret < 0)
> > >  		strcpy(sval, "n/a");
> > > -	else if (query.type == V4L2_CTRL_TYPE_INTEGER64)
> > > +	else if (query->type == V4L2_CTRL_TYPE_INTEGER64)
> > >  		sprintf(sval, "%lld", ctrl.value64);
> > > -	else if (query.type == V4L2_CTRL_TYPE_STRING)
> > > +	else if (query->type == V4L2_CTRL_TYPE_STRING)
> > >  		current = ctrl.string;
> > >  	else
> > >  		sprintf(sval, "%d", ctrl.value);
> > >  
> > >  	if (full)
> > >  		printf("control 0x%08x `%s' min %d max %d step %d default %d current %s.\n",
> > > -			query.id, query.name, query.minimum, query.maximum,
> > > -			query.step, query.default_value, current);
> > > +			query->id, query->name, query->minimum, query->maximum,
> > > +			query->step, query->default_value, current);
> > >  	else
> > > -		printf("control 0x%08x current %s.\n", query.id, current);
> > > +		printf("control 0x%08x current %s.\n", query->id, current);
> > >  
> > > -	if (query.type == V4L2_CTRL_TYPE_STRING)
> > > +	if (query->type == V4L2_CTRL_TYPE_STRING)
> > >  		free(ctrl.string);
> > >  
> > >  	if (!full)
> > > -		return query.id;
> > > +		return 1;
> > >  
> > > -	if (query.type == V4L2_CTRL_TYPE_MENU ||
> > > -	    query.type == V4L2_CTRL_TYPE_INTEGER_MENU)
> > > -		video_query_menu(dev, &query, ctrl.value);
> > > +	if (query->type == V4L2_CTRL_TYPE_MENU ||
> > > +	    query->type == V4L2_CTRL_TYPE_INTEGER_MENU)
> > > +		video_query_menu(dev, query, ctrl.value);
> > >  
> > > -	return query.id;
> > > +	return 1;
> > > +}
> > > +
> > > +static int __video_print_control(struct device *dev,
> > > +				 const struct v4l2_queryctrl *query)
> > > +{
> > > +	return video_print_control(dev, query, true);
> > >  }
> > >  
> > >  static void video_list_controls(struct device *dev)
> > >  {
> > > -	unsigned int nctrls = 0;
> > > -	unsigned int id;
> > >  	int ret;
> > >  
> > > -#ifndef V4L2_CTRL_FLAG_NEXT_CTRL
> > > -	unsigned int i;
> > > +	ret = video_for_each_control(dev, __video_print_control);
> > > +	if (ret < 0)
> > > +		return;
> > >  
> > > -	for (i = V4L2_CID_BASE; i <= V4L2_CID_LASTP1; ++i) {
> > > -		id = i;
> > > -#else
> > > -	id = 0;
> > > -	while (1) {
> > > -		id |= V4L2_CTRL_FLAG_NEXT_CTRL;
> > > -#endif
> > > -
> > > -		ret = video_print_control(dev, id, true);
> > > -		if (ret < 0)
> > > -			break;
> > > -
> > > -		id = ret;
> > > -		nctrls++;
> > > -	}
> > > -
> > > -	if (nctrls)
> > > -		printf("%u control%s found.\n", nctrls, nctrls > 1 ? "s" : "");
> > > +	if (ret)
> > > +		printf("%u control%s found.\n", ret, ret > 1 ? "s" : "");
> > >  	else
> > >  		printf("No control found.\n");
> > >  }
> > > @@ -2184,8 +2210,13 @@ int main(int argc, char *argv[])
> > >  	if (do_log_status)
> > >  		video_log_status(&dev);
> > >  
> > > -	if (do_get_control)
> > > -		video_print_control(&dev, ctrl_name, false);
> > > +	if (do_get_control) {
> > > +		struct v4l2_queryctrl query;
> > > +
> > > +		ret = query_control(&dev, ctrl_name, &query);
> > > +		if (ret == 0)
> > > +			video_print_control(&dev, &query, false);
> > > +	}
> > >  
> > >  	if (do_set_control)
> > >  		set_control(&dev, ctrl_name, ctrl_value);
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Sakari Ailus

Re: [PATCH yavta 3/7] Implement compound control get support

[Laurent Pinchart]

Hi Sakari,

On Wed, Feb 20, 2019 at 04:06:43PM +0200, Sakari Ailus wrote:
> On Wed, Feb 20, 2019 at 02:51:19PM +0200, Laurent Pinchart wrote:
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  yavta.c | 154 ++++++++++++++++++++++++++++++++++++++++++--------------
> >  1 file changed, 115 insertions(+), 39 deletions(-)
> > 
> > diff --git a/yavta.c b/yavta.c
> > index eb50d592736f..6428c22f88d7 100644
> > --- a/yavta.c
> > +++ b/yavta.c
> > @@ -529,6 +529,7 @@ static int get_control(struct device *dev,
> >  		       struct v4l2_ext_control *ctrl)
> >  {
> >  	struct v4l2_ext_controls ctrls;
> > +	struct v4l2_control old;
> >  	int ret;
> >  
> >  	memset(&ctrls, 0, sizeof(ctrls));
> > @@ -540,34 +541,32 @@ static int get_control(struct device *dev,
> >  
> >  	ctrl->id = query->id;
> >  
> > -	if (query->type == V4L2_CTRL_TYPE_STRING) {
> > -		ctrl->string = malloc(query->maximum + 1);
> > -		if (ctrl->string == NULL)
> > +	if (query->flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) {
> 
> This breaks string controls for kernels that don't have
> V4L2_CTRL_FLAG_HAS_PAYLOAD. As you still support kernels that have no
> V4L2_CTRL_FLAG_NEXT_CTRL, how about checking for string type specifically?

Nasty one :-( I'll do so.

> > +		ctrl->size = query->elems * query->elem_size;
> > +		ctrl->ptr = malloc(ctrl->size);
> > +		if (ctrl->ptr == NULL)
> >  			return -ENOMEM;
> > -
> > -		ctrl->size = query->maximum + 1;
> >  	}
> >  
> >  	ret = ioctl(dev->fd, VIDIOC_G_EXT_CTRLS, &ctrls);
> >  	if (ret != -1)
> >  		return 0;
> >  
> > -	if (query->type != V4L2_CTRL_TYPE_INTEGER64 &&
> > -	    query->type != V4L2_CTRL_TYPE_STRING &&
> > -	    (errno == EINVAL || errno == ENOTTY)) {
> > -		struct v4l2_control old;
> > +	if (query->flags & V4L2_CTRL_FLAG_HAS_PAYLOAD)
> > +		free(ctrl->ptr);
> >  
> > -		old.id = query->id;
> > -		ret = ioctl(dev->fd, VIDIOC_G_CTRL, &old);
> > -		if (ret != -1) {
> > -			ctrl->value = old.value;
> > -			return 0;
> > -		}
> > -	}
> > +	if (query->flags & V4L2_CTRL_FLAG_HAS_PAYLOAD ||
> > +	    query->type == V4L2_CTRL_TYPE_INTEGER64 ||
> > +	    (errno != EINVAL && errno != ENOTTY))
> > +		return -errno;
> >  
> > -	printf("unable to get control 0x%8.8x: %s (%d).\n",
> > -		query->id, strerror(errno), errno);
> > -	return -1;
> > +	old.id = query->id;
> > +	ret = ioctl(dev->fd, VIDIOC_G_CTRL, &old);
> > +	if (ret < 0)
> > +		return -errno;
> > +
> > +	ctrl->value = old.value;
> > +	return 0;
> >  }
> >  
> >  static void set_control(struct device *dev, unsigned int id,
> > @@ -1170,7 +1169,7 @@ static int video_for_each_control(struct device *dev,
> >  #else
> >  	id = 0;
> >  	while (1) {
> > -		id |= V4L2_CTRL_FLAG_NEXT_CTRL;
> > +		id |= V4L2_CTRL_FLAG_NEXT_CTRL | V4L2_CTRL_FLAG_NEXT_COMPOUND;
> >  #endif
> >  
> >  		ret = query_control(dev, id, &query);
> > @@ -1215,13 +1214,76 @@ static void video_query_menu(struct device *dev,
> >  	};
> >  }
> >  
> > +static void video_print_control_array(const struct v4l2_query_ext_ctrl *query,
> > +				      struct v4l2_ext_control *ctrl)
> > +{
> > +	unsigned int i;
> > +
> > +	printf("{");
> 
> A space would be nice after the opening brace, also before the closing one.

Isn't that a matter of taste ? :-)

> > +
> > +	for (i = 0; i < query->elems; ++i) {
> > +		switch (query->type) {
> > +		case V4L2_CTRL_TYPE_U8:
> > +			printf("%u", ctrl->p_u8[i]);
> > +			break;
> > +		case V4L2_CTRL_TYPE_U16:
> > +			printf("%u", ctrl->p_u16[i]);
> > +			break;
> > +		case V4L2_CTRL_TYPE_U32:
> > +			printf("%u", ctrl->p_u32[i]);
> > +			break;
> > +		}
> > +
> > +		if (i != query->elems - 1)
> > +			printf(", ");
> > +	}
> > +
> > +	printf("}");
> > +}
> > +
> > +static void video_print_control_value(const struct v4l2_query_ext_ctrl *query,
> > +				      struct v4l2_ext_control *ctrl)
> > +{
> > +	if (query->nr_of_dims == 0) {
> > +		switch (query->type) {
> > +		case V4L2_CTRL_TYPE_INTEGER:
> > +		case V4L2_CTRL_TYPE_BOOLEAN:
> > +		case V4L2_CTRL_TYPE_MENU:
> > +		case V4L2_CTRL_TYPE_INTEGER_MENU:
> > +			printf("%d", ctrl->value);
> > +			break;
> > +		case V4L2_CTRL_TYPE_BITMASK:
> > +			printf("0x%08x", ctrl->value);
> 
> A cast to unsigned here?

Does your compiler warn ?

> > +			break;
> > +		case V4L2_CTRL_TYPE_INTEGER64:
> > +			printf("%lld", ctrl->value64);
> > +			break;
> > +		case V4L2_CTRL_TYPE_STRING:
> > +			printf("%s", ctrl->string);
> > +			break;
> > +		}
> > +
> > +		return;
> > +	}
> > +
> > +	switch (query->type) {
> > +	case V4L2_CTRL_TYPE_U8:
> > +	case V4L2_CTRL_TYPE_U16:
> > +	case V4L2_CTRL_TYPE_U32:
> > +		video_print_control_array(query, ctrl);
> > +		break;
> > +	default:
> > +		printf("unsupported");
> 
> How about printing the unsupported type?

Good idea.

> > +		break;
> > +	}
> > +}
> > +
> >  static int video_print_control(struct device *dev,
> >  			       const struct v4l2_query_ext_ctrl *query,
> >  			       bool full)
> >  {
> >  	struct v4l2_ext_control ctrl;
> > -	char sval[24];
> > -	char *current = sval;
> > +	unsigned int i;
> >  	int ret;
> >  
> >  	if (query->flags & V4L2_CTRL_FLAG_DISABLED)
> > @@ -1232,25 +1294,39 @@ static int video_print_control(struct device *dev,
> >  		return 0;
> >  	}
> >  
> > -	ret = get_control(dev, query, &ctrl);
> > -	if (ret < 0)
> > -		strcpy(sval, "n/a");
> > -	else if (query->type == V4L2_CTRL_TYPE_INTEGER64)
> > -		sprintf(sval, "%lld", ctrl.value64);
> > -	else if (query->type == V4L2_CTRL_TYPE_STRING)
> > -		current = ctrl.string;
> > -	else
> > -		sprintf(sval, "%d", ctrl.value);
> > -
> > -	if (full)
> > -		printf("control 0x%08x `%s' min %lld max %lld step %lld default %lld current %s.\n",
> > +	if (full) {
> > +		printf("control 0x%08x `%s' min %lld max %lld step %lld default %lld ",
> >  			query->id, query->name, query->minimum, query->maximum,
> > -			query->step, query->default_value, current);
> > -	else
> > -		printf("control 0x%08x current %s.\n", query->id, current);
> > +			query->step, query->default_value);
> > +		if (query->nr_of_dims) {
> > +			for (i = 0; i < query->nr_of_dims; ++i)
> > +				printf("[%u]", query->dims[i]);
> > +			printf(" ");
> > +		}
> > +	} else {
> > +		printf("control 0x%08x ", query->id);
> > +	}
> >  
> > -	if (query->type == V4L2_CTRL_TYPE_STRING)
> > -		free(ctrl.string);
> > +	if (query->type == V4L2_CTRL_TYPE_BUTTON) {
> > +		/* Button controls have no current value. */
> > +		printf("\n");
> > +		return 1;
> > +	}
> > +
> > +	printf("current ");
> > +
> > +	ret = get_control(dev, query, &ctrl);
> > +	if (ret < 0) {
> > +		printf("n/a\n");
> > +		printf("unable to get control 0x%8.8x: %s (%d).\n",
> > +			query->id, strerror(errno), errno);
> > +	} else {
> > +		video_print_control_value(query, &ctrl);
> > +		printf("\n");
> > +	}
> > +
> > +	if (query->flags & V4L2_CTRL_FLAG_HAS_PAYLOAD)
> > +		free(ctrl.ptr);
> >  
> >  	if (!full)
> >  		return 1;

-- 
Regards,

Laurent Pinchart

Re: [PATCH yavta 3/7] Implement compound control get support

[Sakari Ailus]

Hi Laurent,

On Wed, Feb 20, 2019 at 04:55:06PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Wed, Feb 20, 2019 at 04:06:43PM +0200, Sakari Ailus wrote:
> > On Wed, Feb 20, 2019 at 02:51:19PM +0200, Laurent Pinchart wrote:
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > >  yavta.c | 154 ++++++++++++++++++++++++++++++++++++++++++--------------
> > >  1 file changed, 115 insertions(+), 39 deletions(-)
> > > 
> > > diff --git a/yavta.c b/yavta.c
> > > index eb50d592736f..6428c22f88d7 100644
> > > --- a/yavta.c
> > > +++ b/yavta.c
> > > @@ -529,6 +529,7 @@ static int get_control(struct device *dev,
> > >  		       struct v4l2_ext_control *ctrl)
> > >  {
> > >  	struct v4l2_ext_controls ctrls;
> > > +	struct v4l2_control old;
> > >  	int ret;
> > >  
> > >  	memset(&ctrls, 0, sizeof(ctrls));
> > > @@ -540,34 +541,32 @@ static int get_control(struct device *dev,
> > >  
> > >  	ctrl->id = query->id;
> > >  
> > > -	if (query->type == V4L2_CTRL_TYPE_STRING) {
> > > -		ctrl->string = malloc(query->maximum + 1);
> > > -		if (ctrl->string == NULL)
> > > +	if (query->flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) {
> > 
> > This breaks string controls for kernels that don't have
> > V4L2_CTRL_FLAG_HAS_PAYLOAD. As you still support kernels that have no
> > V4L2_CTRL_FLAG_NEXT_CTRL, how about checking for string type specifically?
> 
> Nasty one :-( I'll do so.

Ack!

> 
> > > +		ctrl->size = query->elems * query->elem_size;
> > > +		ctrl->ptr = malloc(ctrl->size);
> > > +		if (ctrl->ptr == NULL)
> > >  			return -ENOMEM;
> > > -
> > > -		ctrl->size = query->maximum + 1;
> > >  	}
> > >  
> > >  	ret = ioctl(dev->fd, VIDIOC_G_EXT_CTRLS, &ctrls);
> > >  	if (ret != -1)
> > >  		return 0;
> > >  
> > > -	if (query->type != V4L2_CTRL_TYPE_INTEGER64 &&
> > > -	    query->type != V4L2_CTRL_TYPE_STRING &&
> > > -	    (errno == EINVAL || errno == ENOTTY)) {
> > > -		struct v4l2_control old;
> > > +	if (query->flags & V4L2_CTRL_FLAG_HAS_PAYLOAD)
> > > +		free(ctrl->ptr);
> > >  
> > > -		old.id = query->id;
> > > -		ret = ioctl(dev->fd, VIDIOC_G_CTRL, &old);
> > > -		if (ret != -1) {
> > > -			ctrl->value = old.value;
> > > -			return 0;
> > > -		}
> > > -	}
> > > +	if (query->flags & V4L2_CTRL_FLAG_HAS_PAYLOAD ||
> > > +	    query->type == V4L2_CTRL_TYPE_INTEGER64 ||
> > > +	    (errno != EINVAL && errno != ENOTTY))
> > > +		return -errno;
> > >  
> > > -	printf("unable to get control 0x%8.8x: %s (%d).\n",
> > > -		query->id, strerror(errno), errno);
> > > -	return -1;
> > > +	old.id = query->id;
> > > +	ret = ioctl(dev->fd, VIDIOC_G_CTRL, &old);
> > > +	if (ret < 0)
> > > +		return -errno;
> > > +
> > > +	ctrl->value = old.value;
> > > +	return 0;
> > >  }
> > >  
> > >  static void set_control(struct device *dev, unsigned int id,
> > > @@ -1170,7 +1169,7 @@ static int video_for_each_control(struct device *dev,
> > >  #else
> > >  	id = 0;
> > >  	while (1) {
> > > -		id |= V4L2_CTRL_FLAG_NEXT_CTRL;
> > > +		id |= V4L2_CTRL_FLAG_NEXT_CTRL | V4L2_CTRL_FLAG_NEXT_COMPOUND;
> > >  #endif
> > >  
> > >  		ret = query_control(dev, id, &query);
> > > @@ -1215,13 +1214,76 @@ static void video_query_menu(struct device *dev,
> > >  	};
> > >  }
> > >  
> > > +static void video_print_control_array(const struct v4l2_query_ext_ctrl *query,
> > > +				      struct v4l2_ext_control *ctrl)
> > > +{
> > > +	unsigned int i;
> > > +
> > > +	printf("{");
> > 
> > A space would be nice after the opening brace, also before the closing one.
> 
> Isn't that a matter of taste ? :-)

Could be, but the vast majority of the kernel code does that. And I think
it's part of the coding style, therefore I'd do the same here. I like it
that way, too. :-)

> 
> > > +
> > > +	for (i = 0; i < query->elems; ++i) {
> > > +		switch (query->type) {
> > > +		case V4L2_CTRL_TYPE_U8:
> > > +			printf("%u", ctrl->p_u8[i]);
> > > +			break;
> > > +		case V4L2_CTRL_TYPE_U16:
> > > +			printf("%u", ctrl->p_u16[i]);
> > > +			break;
> > > +		case V4L2_CTRL_TYPE_U32:
> > > +			printf("%u", ctrl->p_u32[i]);
> > > +			break;
> > > +		}
> > > +
> > > +		if (i != query->elems - 1)
> > > +			printf(", ");
> > > +	}
> > > +
> > > +	printf("}");
> > > +}
> > > +
> > > +static void video_print_control_value(const struct v4l2_query_ext_ctrl *query,
> > > +				      struct v4l2_ext_control *ctrl)
> > > +{
> > > +	if (query->nr_of_dims == 0) {
> > > +		switch (query->type) {
> > > +		case V4L2_CTRL_TYPE_INTEGER:
> > > +		case V4L2_CTRL_TYPE_BOOLEAN:
> > > +		case V4L2_CTRL_TYPE_MENU:
> > > +		case V4L2_CTRL_TYPE_INTEGER_MENU:
> > > +			printf("%d", ctrl->value);
> > > +			break;
> > > +		case V4L2_CTRL_TYPE_BITMASK:
> > > +			printf("0x%08x", ctrl->value);
> > 
> > A cast to unsigned here?
> 
> Does your compiler warn ?

I've reviewed the patch, not compiled it. :-)

-- 
Regards,

Sakari Ailus

Re: [PATCH yavta 4/7] Implement compound control set support

[Sakari Ailus]

Hi Laurent,

On Wed, Feb 20, 2019 at 02:51:20PM +0200, Laurent Pinchart wrote:
> Only arrays of integer types are supported.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  yavta.c | 228 ++++++++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 172 insertions(+), 56 deletions(-)
> 
> diff --git a/yavta.c b/yavta.c
> index 6428c22f88d7..d1bfd380c03b 100644
> --- a/yavta.c
> +++ b/yavta.c
...
> @@ -1338,17 +1318,157 @@ static int video_print_control(struct device *dev,
>  	return 1;
>  }
>  
> -static int __video_print_control(struct device *dev,
> -				 const struct v4l2_query_ext_ctrl *query)
> +static int __video_get_control(struct device *dev,
> +			       const struct v4l2_query_ext_ctrl *query)
>  {
> -	return video_print_control(dev, query, true);
> +	return video_get_control(dev, query, true);
> +}
> +
> +static int video_parse_control_array(const struct v4l2_query_ext_ctrl *query,
> +				     struct v4l2_ext_control *ctrl,
> +				     const char *val)
> +{
> +	unsigned int i;
> +	char *endptr;
> +	__u32 value;
> +
> +	for ( ; isspace(*val); ++val) { };
> +	if (*val++ != '{')
> +		return -EINVAL;
> +
> +	for (i = 0; i < query->elems; ++i) {
> +		for ( ; isspace(*val); ++val) { };

Why braces?

Just wondering. :-)

You could use any number of semicolons, too.

There's more below.

-- 
Sakari Ailus

Re: [PATCH yavta 6/7] Support setting control from values stored in a file

[Sakari Ailus]

Hi Laurent,

On Wed, Feb 20, 2019 at 02:51:22PM +0200, Laurent Pinchart wrote:
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  yavta.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/yavta.c b/yavta.c
> index 1490878c6f7e..2d49131a4271 100644
> --- a/yavta.c
> +++ b/yavta.c
> @@ -1334,6 +1334,31 @@ static int video_parse_control_array(const struct v4l2_query_ext_ctrl *query,
>  	__u32 value;
>  
>  	for ( ; isspace(*val); ++val) { };
> +
> +	if (*val == '<') {
> +		/* Read the control value from the given file. */
> +		ssize_t size;
> +		int fd;
> +
> +		val++;
> +		fd = open(val, O_RDONLY);
> +		if (fd < 0) {
> +			printf("unable to open control file `%s'\n", val);
> +			return -EINVAL;
> +		}
> +
> +		size = read(fd, ctrl->ptr, ctrl->size);

Note that a read of count reads *up to* count number of bytes from the file
descriptor. In other words, it's perfectly correct for it to read less than
requested.

How about using fread(3) instead? Or changing this into a loop?

> +		if (size != (ssize_t)ctrl->size) {
> +			printf("error reading control file `%s' (%s)\n", val,
> +			       strerror(errno));
> +			close(fd);
> +			return -EINVAL;
> +		}
> +
> +		close(fd);
> +		return 0;
> +	}
> +
>  	if (*val++ != '{')
>  		return -EINVAL;
>  

-- 
Regards,

Sakari Ailus

Re: [PATCH yavta 0/7] Compound controls and controls reset support

[Sakari Ailus]

Hi Laurent,

On Wed, Feb 20, 2019 at 02:51:16PM +0200, Laurent Pinchart wrote:
> Hello,
> 
> This patch series implements support for compound controls in yavta,
> including the ability to reset controls to their default value. Only
> array compound controls are supported for now, other types may be added
> later.
> 
> The patches have lived out of the master branch for long enough, it's
> time to get them merged.
> 
> Kieran Bingham (1):
>   Add support to reset device controls
> 
> Laurent Pinchart (6):
>   yavta: Refactor video_list_controls()
>   Implement VIDIOC_QUERY_EXT_CTRL support
>   Implement compound control get support
>   Implement compound control set support
>   Support setting control from values stored in a file
>   Remove unneeded conditional compilation for old V4L2 API versions
> 
>  yavta.c | 602 +++++++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 464 insertions(+), 138 deletions(-)
> 

After ruminating the review comments which may lead to changes in the
patches, please add:

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

-- 
Sakari Ailus

Re: [PATCH yavta 6/7] Support setting control from values stored in a file

[Laurent Pinchart]

Hi Sakari,

On Wed, Feb 20, 2019 at 11:26:52PM +0200, Sakari Ailus wrote:
> On Wed, Feb 20, 2019 at 02:51:22PM +0200, Laurent Pinchart wrote:
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  yavta.c | 25 +++++++++++++++++++++++++
> >  1 file changed, 25 insertions(+)
> > 
> > diff --git a/yavta.c b/yavta.c
> > index 1490878c6f7e..2d49131a4271 100644
> > --- a/yavta.c
> > +++ b/yavta.c
> > @@ -1334,6 +1334,31 @@ static int video_parse_control_array(const struct v4l2_query_ext_ctrl *query,
> >  	__u32 value;
> >  
> >  	for ( ; isspace(*val); ++val) { };
> > +
> > +	if (*val == '<') {
> > +		/* Read the control value from the given file. */
> > +		ssize_t size;
> > +		int fd;
> > +
> > +		val++;
> > +		fd = open(val, O_RDONLY);
> > +		if (fd < 0) {
> > +			printf("unable to open control file `%s'\n", val);
> > +			return -EINVAL;
> > +		}
> > +
> > +		size = read(fd, ctrl->ptr, ctrl->size);
> 
> Note that a read of count reads *up to* count number of bytes from the file
> descriptor. In other words, it's perfectly correct for it to read less than
> requested.
> 
> How about using fread(3) instead? Or changing this into a loop?

Do you expect this to cause issues in practice ? When does read() return
less data than requested for regular files ?

> > +		if (size != (ssize_t)ctrl->size) {
> > +			printf("error reading control file `%s' (%s)\n", val,
> > +			       strerror(errno));
> > +			close(fd);
> > +			return -EINVAL;
> > +		}
> > +
> > +		close(fd);
> > +		return 0;
> > +	}
> > +
> >  	if (*val++ != '{')
> >  		return -EINVAL;
> >  

-- 
Regards,

Laurent Pinchart

Re: [PATCH yavta 6/7] Support setting control from values stored in a file

[Sakari Ailus]

Hi Laurent,

On Fri, Feb 22, 2019 at 01:47:39PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Wed, Feb 20, 2019 at 11:26:52PM +0200, Sakari Ailus wrote:
> > On Wed, Feb 20, 2019 at 02:51:22PM +0200, Laurent Pinchart wrote:
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > >  yavta.c | 25 +++++++++++++++++++++++++
> > >  1 file changed, 25 insertions(+)
> > > 
> > > diff --git a/yavta.c b/yavta.c
> > > index 1490878c6f7e..2d49131a4271 100644
> > > --- a/yavta.c
> > > +++ b/yavta.c
> > > @@ -1334,6 +1334,31 @@ static int video_parse_control_array(const struct v4l2_query_ext_ctrl *query,
> > >  	__u32 value;
> > >  
> > >  	for ( ; isspace(*val); ++val) { };
> > > +
> > > +	if (*val == '<') {
> > > +		/* Read the control value from the given file. */
> > > +		ssize_t size;
> > > +		int fd;
> > > +
> > > +		val++;
> > > +		fd = open(val, O_RDONLY);
> > > +		if (fd < 0) {
> > > +			printf("unable to open control file `%s'\n", val);
> > > +			return -EINVAL;
> > > +		}
> > > +
> > > +		size = read(fd, ctrl->ptr, ctrl->size);
> > 
> > Note that a read of count reads *up to* count number of bytes from the file
> > descriptor. In other words, it's perfectly correct for it to read less than
> > requested.
> > 
> > How about using fread(3) instead? Or changing this into a loop?
> 
> Do you expect this to cause issues in practice ? When does read() return
> less data than requested for regular files ?

API-wise there's no guarantee. This could simply fail for someone at some
point when reading a largish compound control.

Also try reading from e.g. /dev/random. :-)

-- 
Sakari Ailus