[PATCH 0/3] v4l-utils: media-ctl: Add DV timings support

[PATCH 0/3] v4l-utils: media-ctl: Add DV timings support

[Laurent Pinchart]

Hello,

This patch set adds support for the subdev DV timings ioctls to the media-ctl
utility, allowing DV timings to be configured in media controller pipelines.

The first patch adds wrappers around the DV timings ioctls to libv4l2subdev in
a pretty straightforward way. The second patch refactors the media-ctl flag
printing code to avoid later duplication. The third patch is the interesting
part, adding DV timings support to the media-ctl utility.

With this series applied DV timings are added to the output when printing
formats with the existing -p argument. A new --set-dv argument allows
configuring DV timings on a pad, by querying the current timings and applying
them unmodified. This is enough to configure pipelines that include HDMI
receivers with the timings detected at the HDMI input. Support for fully
manual timings configuration from the command line can be added later when
needed.

Laurent Pinchart (3):
  media-ctl: libv4l2subdev: Add DV timings support
  media-ctl: Move flags printing code to a new print_flags function
  media-ctl: Add DV timings support

 utils/media-ctl/libv4l2subdev.c |  72 +++++++++++++
 utils/media-ctl/media-ctl.c     | 222 ++++++++++++++++++++++++++++++++++++----
 utils/media-ctl/options.c       |   9 +-
 utils/media-ctl/options.h       |   3 +-
 utils/media-ctl/v4l2subdev.h    |  53 ++++++++++
 5 files changed, 338 insertions(+), 21 deletions(-)

-- 
Regards,

Laurent Pinchart

[PATCH 1/3] media-ctl: libv4l2subdev: Add DV timings support

[Laurent Pinchart]

Expose the pad-level get caps, query, get and set DV timings ioctls.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 utils/media-ctl/libv4l2subdev.c | 72 +++++++++++++++++++++++++++++++++++++++++
 utils/media-ctl/v4l2subdev.h    | 53 ++++++++++++++++++++++++++++++
 2 files changed, 125 insertions(+)

diff --git a/utils/media-ctl/libv4l2subdev.c b/utils/media-ctl/libv4l2subdev.c
index 14daffa..8015330 100644
--- a/utils/media-ctl/libv4l2subdev.c
+++ b/utils/media-ctl/libv4l2subdev.c
@@ -189,6 +189,78 @@ int v4l2_subdev_set_selection(struct media_entity *entity,
 	return 0;
 }
 
+int v4l2_subdev_get_dv_timings_caps(struct media_entity *entity,
+	struct v4l2_dv_timings_cap *caps)
+{
+	unsigned int pad = caps->pad;
+	int ret;
+
+	ret = v4l2_subdev_open(entity);
+	if (ret < 0)
+		return ret;
+
+	memset(caps, 0, sizeof(*caps));
+	caps->pad = pad;
+
+	ret = ioctl(entity->fd, VIDIOC_SUBDEV_DV_TIMINGS_CAP, caps);
+	if (ret < 0)
+		return -errno;
+
+	return 0;
+}
+
+int v4l2_subdev_query_dv_timings(struct media_entity *entity,
+	struct v4l2_dv_timings *timings)
+{
+	int ret;
+
+	ret = v4l2_subdev_open(entity);
+	if (ret < 0)
+		return ret;
+
+	memset(timings, 0, sizeof(*timings));
+
+	ret = ioctl(entity->fd, VIDIOC_SUBDEV_QUERY_DV_TIMINGS, timings);
+	if (ret < 0)
+		return -errno;
+
+	return 0;
+}
+
+int v4l2_subdev_get_dv_timings(struct media_entity *entity,
+	struct v4l2_dv_timings *timings)
+{
+	int ret;
+
+	ret = v4l2_subdev_open(entity);
+	if (ret < 0)
+		return ret;
+
+	memset(timings, 0, sizeof(*timings));
+
+	ret = ioctl(entity->fd, VIDIOC_SUBDEV_G_DV_TIMINGS, timings);
+	if (ret < 0)
+		return -errno;
+
+	return 0;
+}
+
+int v4l2_subdev_set_dv_timings(struct media_entity *entity,
+	struct v4l2_dv_timings *timings)
+{
+	int ret;
+
+	ret = v4l2_subdev_open(entity);
+	if (ret < 0)
+		return ret;
+
+	ret = ioctl(entity->fd, VIDIOC_SUBDEV_S_DV_TIMINGS, timings);
+	if (ret < 0)
+		return -errno;
+
+	return 0;
+}
+
 int v4l2_subdev_get_frame_interval(struct media_entity *entity,
 				   struct v4l2_fract *interval)
 {
diff --git a/utils/media-ctl/v4l2subdev.h b/utils/media-ctl/v4l2subdev.h
index c2ca1e5..1cb53ff 100644
--- a/utils/media-ctl/v4l2subdev.h
+++ b/utils/media-ctl/v4l2subdev.h
@@ -132,6 +132,59 @@ int v4l2_subdev_set_selection(struct media_entity *entity,
 	enum v4l2_subdev_format_whence which);
 
 /**
+ * @brief Query the digital video capabilities of a pad.
+ * @param entity - subdev-device media entity.
+ * @param cap - capabilities to be filled.
+ *
+ * Retrieve the digital video capabilities of the @a entity pad specified by
+ * @a cap.pad and store it in the @a cap structure.
+ *
+ * @return 0 on success, or a negative error code on failure.
+ */
+int v4l2_subdev_get_dv_timings_caps(struct media_entity *entity,
+	struct v4l2_dv_timings_cap *caps);
+
+/**
+ * @brief Query the digital video timings of a sub-device
+ * @param entity - subdev-device media entity.
+ * @param timings timings to be filled.
+ *
+ * Retrieve the detected digital video timings for the currently selected input
+ * of @a entity and store them in the @a timings structure.
+ *
+ * @return 0 on success, or a negative error code on failure.
+ */
+int v4l2_subdev_query_dv_timings(struct media_entity *entity,
+	struct v4l2_dv_timings *timings);
+
+/**
+ * @brief Get the current digital video timings of a sub-device
+ * @param entity - subdev-device media entity.
+ * @param timings timings to be filled.
+ *
+ * Retrieve the current digital video timings for the currently selected input
+ * of @a entity and store them in the @a timings structure.
+ *
+ * @return 0 on success, or a negative error code on failure.
+ */
+int v4l2_subdev_get_dv_timings(struct media_entity *entity,
+	struct v4l2_dv_timings *timings);
+
+/**
+ * @brief Set the digital video timings of a sub-device
+ * @param entity - subdev-device media entity.
+ * @param timings timings to be set.
+ *
+ * Set the digital video timings of @a entity to @a timings. The driver is
+ * allowed to modify the requested format, in which case @a timings is updated
+ * with the modifications.
+ *
+ * @return 0 on success, or a negative error code on failure.
+ */
+int v4l2_subdev_set_dv_timings(struct media_entity *entity,
+	struct v4l2_dv_timings *timings);
+
+/**
  * @brief Retrieve the frame interval on a sub-device.
  * @param entity - subdev-device media entity.
  * @param interval - frame interval to be filled.
-- 
1.8.5.5

[PATCH 2/3] media-ctl: Move flags printing code to a new print_flags function

[Laurent Pinchart]

This will allow reusing the flag printing code for the DV timings flags.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 utils/media-ctl/media-ctl.c | 43 +++++++++++++++++++++++++++++--------------
 1 file changed, 29 insertions(+), 14 deletions(-)

diff --git a/utils/media-ctl/media-ctl.c b/utils/media-ctl/media-ctl.c
index d48969f..44c9644 100644
--- a/utils/media-ctl/media-ctl.c
+++ b/utils/media-ctl/media-ctl.c
@@ -48,6 +48,33 @@
  * Printing
  */
 
+struct flag_name {
+	__u32 flag;
+	char *name;
+};
+
+static void print_flags(const struct flag_name *flag_names, unsigned int num_entries, __u32 flags)
+{
+	bool first = true;
+	unsigned int i;
+
+	for (i = 0; i < num_entries; i++) {
+		if (!(flags & flag_names[i].flag))
+			continue;
+		if (!first)
+			printf(",");
+		printf("%s", flag_names[i].name);
+		flags &= ~flag_names[i].flag;
+		first = false;
+	}
+
+	if (flags) {
+		if (!first)
+			printf(",");
+		printf("0x%x", flags);
+	}
+}
+
 static void v4l2_subdev_print_format(struct media_entity *entity,
 	unsigned int pad, enum v4l2_subdev_format_whence which)
 {
@@ -255,10 +282,7 @@ static void media_print_topology_dot(struct media_device *media)
 
 static void media_print_topology_text(struct media_device *media)
 {
-	static const struct {
-		__u32 flag;
-		char *name;
-	} link_flags[] = {
+	static const struct flag_name link_flags[] = {
 		{ MEDIA_LNK_FL_ENABLED, "ENABLED" },
 		{ MEDIA_LNK_FL_IMMUTABLE, "IMMUTABLE" },
 		{ MEDIA_LNK_FL_DYNAMIC, "DYNAMIC" },
@@ -299,8 +323,6 @@ static void media_print_topology_text(struct media_device *media)
 				const struct media_link *link = media_entity_get_link(entity, k);
 				const struct media_pad *source = link->source;
 				const struct media_pad *sink = link->sink;
-				bool first = true;
-				unsigned int i;
 
 				if (source->entity == entity && source->index == j)
 					printf("\t\t-> \"%s\":%u [",
@@ -313,14 +335,7 @@ static void media_print_topology_text(struct media_device *media)
 				else
 					continue;
 
-				for (i = 0; i < ARRAY_SIZE(link_flags); i++) {
-					if (!(link->flags & link_flags[i].flag))
-						continue;
-					if (!first)
-						printf(",");
-					printf("%s", link_flags[i].name);
-					first = false;
-				}
+				print_flags(link_flags, ARRAY_SIZE(link_flags), link->flags);
 
 				printf("]\n");
 			}
-- 
1.8.5.5

[PATCH 3/3] media-ctl: Add DV timings support

[Laurent Pinchart]

Support printing (with -p) and setting (with --set-dv) DV timings at the
pad level.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 utils/media-ctl/media-ctl.c | 179 ++++++++++++++++++++++++++++++++++++++++++--
 utils/media-ctl/options.c   |   9 ++-
 utils/media-ctl/options.h   |   3 +-
 3 files changed, 184 insertions(+), 7 deletions(-)

diff --git a/utils/media-ctl/media-ctl.c b/utils/media-ctl/media-ctl.c
index 44c9644..319aa5d 100644
--- a/utils/media-ctl/media-ctl.c
+++ b/utils/media-ctl/media-ctl.c
@@ -121,6 +121,140 @@ static void v4l2_subdev_print_format(struct media_entity *entity,
 	printf("]\n");
 }
 
+static const char *v4l2_dv_type_to_string(unsigned int type)
+{
+	static const struct {
+		__u32 type;
+		const char *name;
+	} types[] = {
+		{ V4L2_DV_BT_656_1120, "BT.656/1120" },
+	};
+
+	static char unknown[20];
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(types); i++) {
+		if (types[i].type == type)
+			return types[i].name;
+	}
+
+	sprintf(unknown, "Unknown (%u)", type);
+	return unknown;
+}
+
+static const struct flag_name bt_standards[] = {
+	{ V4L2_DV_BT_STD_CEA861, "CEA-861" },
+	{ V4L2_DV_BT_STD_DMT, "DMT" },
+	{ V4L2_DV_BT_STD_CVT, "CVT" },
+	{ V4L2_DV_BT_STD_GTF, "GTF" },
+};
+
+static const struct flag_name bt_capabilities[] = {
+	{ V4L2_DV_BT_CAP_INTERLACED, "interlaced" },
+	{ V4L2_DV_BT_CAP_PROGRESSIVE, "progressive" },
+	{ V4L2_DV_BT_CAP_REDUCED_BLANKING, "reduced-blanking" },
+	{ V4L2_DV_BT_CAP_CUSTOM, "custom" },
+};
+
+static const struct flag_name bt_flags[] = {
+	{ V4L2_DV_FL_REDUCED_BLANKING, "reduced-blanking" },
+	{ V4L2_DV_FL_CAN_REDUCE_FPS, "can-reduce-fps" },
+	{ V4L2_DV_FL_REDUCED_FPS, "reduced-fps" },
+	{ V4L2_DV_FL_HALF_LINE, "half-line" },
+};
+
+static void v4l2_subdev_print_dv_timings(const struct v4l2_dv_timings *timings,
+					 const char *name)
+{
+	printf("\t\t[dv.%s:%s", name, v4l2_dv_type_to_string(timings->type));
+
+	switch (timings->type) {
+	case V4L2_DV_BT_656_1120: {
+		const struct v4l2_bt_timings *bt = &timings->bt;
+		unsigned int htotal, vtotal;
+
+		htotal = V4L2_DV_BT_FRAME_WIDTH(bt);
+		vtotal = V4L2_DV_BT_FRAME_HEIGHT(bt);
+
+		printf(" %ux%u%s%llu (%ux%u)",
+		       bt->width, bt->height, bt->interlaced ? "i" : "p",
+		       (htotal * vtotal) > 0 ? (bt->pixelclock / (htotal * vtotal)) : 0,
+		       htotal, vtotal);
+
+		printf(" stds:");
+		print_flags(bt_standards, ARRAY_SIZE(bt_standards),
+			    bt->standards);
+		printf(" flags:");
+		print_flags(bt_flags, ARRAY_SIZE(bt_flags),
+			    bt->flags);
+
+		break;
+	}
+	}
+
+	printf("]\n");
+}
+
+static void v4l2_subdev_print_pad_dv(struct media_entity *entity,
+	unsigned int pad, enum v4l2_subdev_format_whence which)
+{
+	struct v4l2_dv_timings_cap caps;
+	int ret;
+
+	caps.pad = pad;
+	ret = v4l2_subdev_get_dv_timings_caps(entity, &caps);
+	if (ret != 0)
+		return;
+
+	printf("\t\t[dv.caps:%s", v4l2_dv_type_to_string(caps.type));
+
+	switch (caps.type) {
+	case V4L2_DV_BT_656_1120:
+		printf(" min:%ux%u@%llu max:%ux%u@%llu",
+		       caps.bt.min_width, caps.bt.min_height, caps.bt.min_pixelclock,
+		       caps.bt.max_width, caps.bt.max_height, caps.bt.max_pixelclock);
+
+		printf(" stds:");
+		print_flags(bt_standards, ARRAY_SIZE(bt_standards),
+			    caps.bt.standards);
+		printf(" caps:");
+		print_flags(bt_capabilities, ARRAY_SIZE(bt_capabilities),
+			    caps.bt.capabilities);
+
+		break;
+	}
+
+	printf("]\n");
+}
+
+static void v4l2_subdev_print_subdev_dv(struct media_entity *entity)
+{
+	struct v4l2_dv_timings timings;
+	int ret;
+
+	ret = v4l2_subdev_query_dv_timings(entity, &timings);
+	switch (ret) {
+	case -ENOLINK:
+		printf("\t\t[dv.query:no-link]\n");
+		break;
+	case -ENOLCK:
+		printf("\t\t[dv.query:no-lock]\n");
+		break;
+	case -ERANGE:
+		printf("\t\t[dv.query:out-of-range]\n");
+		break;
+	case 0:
+		v4l2_subdev_print_dv_timings(&timings, "detect");
+		break;
+	default:
+		return;
+	}
+
+	ret = v4l2_subdev_get_dv_timings(entity, &timings);
+	if (ret == 0)
+		v4l2_subdev_print_dv_timings(&timings, "current");
+}
+
 static const char *media_entity_type_to_string(unsigned type)
 {
 	static const struct {
@@ -280,6 +414,19 @@ static void media_print_topology_dot(struct media_device *media)
 	printf("}\n");
 }
 
+static void media_print_pad_text(struct media_entity *entity,
+				 const struct media_pad *pad)
+{
+	if (media_entity_type(entity) != MEDIA_ENT_T_V4L2_SUBDEV)
+		return;
+
+	v4l2_subdev_print_format(entity, pad->index, V4L2_SUBDEV_FORMAT_ACTIVE);
+	v4l2_subdev_print_pad_dv(entity, pad->index, V4L2_SUBDEV_FORMAT_ACTIVE);
+
+	if (pad->flags & MEDIA_PAD_FL_SOURCE)
+		v4l2_subdev_print_subdev_dv(entity);
+}
+
 static void media_print_topology_text(struct media_device *media)
 {
 	static const struct flag_name link_flags[] = {
@@ -316,8 +463,7 @@ static void media_print_topology_text(struct media_device *media)
 
 			printf("\tpad%u: %s\n", j, media_pad_type_to_string(pad->flags));
 
-			if (media_entity_type(entity) == MEDIA_ENT_T_V4L2_SUBDEV)
-				v4l2_subdev_print_format(entity, j, V4L2_SUBDEV_FORMAT_ACTIVE);
+			media_print_pad_text(entity, pad);
 
 			for (k = 0; k < num_links; k++) {
 				const struct media_link *link = media_entity_get_link(entity, k);
@@ -413,12 +559,12 @@ int main(int argc, char **argv)
 		printf("%s\n", media_entity_get_devname(entity));
 	}
 
-	if (media_opts.pad) {
+	if (media_opts.fmt_pad) {
 		struct media_pad *pad;
 
-		pad = media_parse_pad(media, media_opts.pad, NULL);
+		pad = media_parse_pad(media, media_opts.fmt_pad, NULL);
 		if (pad == NULL) {
-			printf("Pad '%s' not found\n", media_opts.pad);
+			printf("Pad '%s' not found\n", media_opts.fmt_pad);
 			goto out;
 		}
 
@@ -426,6 +572,29 @@ int main(int argc, char **argv)
 					 V4L2_SUBDEV_FORMAT_ACTIVE);
 	}
 
+	if (media_opts.dv_pad) {
+		struct v4l2_dv_timings timings;
+		struct media_pad *pad;
+
+		pad = media_parse_pad(media, media_opts.dv_pad, NULL);
+		if (pad == NULL) {
+			printf("Pad '%s' not found\n", media_opts.dv_pad);
+			goto out;
+		}
+
+		ret = v4l2_subdev_query_dv_timings(pad->entity, &timings);
+		if (ret < 0) {
+			printf("Failed to query DV timings: %s\n", strerror(ret));
+			goto out;
+		}
+
+		ret = v4l2_subdev_set_dv_timings(pad->entity, &timings);
+		if (ret < 0) {
+			printf("Failed to set DV timings: %s\n", strerror(ret));
+			goto out;
+		}
+	}
+
 	if (media_opts.print || media_opts.print_dot) {
 		media_print_topology(media, media_opts.print_dot);
 		printf("\n");
diff --git a/utils/media-ctl/options.c b/utils/media-ctl/options.c
index 90d9316..e8aa0e2 100644
--- a/utils/media-ctl/options.c
+++ b/utils/media-ctl/options.c
@@ -39,6 +39,7 @@ static void usage(const char *argv0)
 	printf("-e, --entity name	Print the device name associated with the given entity\n");
 	printf("-V, --set-v4l2 v4l2	Comma-separated list of formats to setup\n");
 	printf("    --get-v4l2 pad	Print the active format on a given pad\n");
+	printf("    --set-dv pad	Configure DV timings on a given pad\n");
 	printf("-h, --help		Show verbose help and exit\n");
 	printf("-i, --interactive	Modify links interactively\n");
 	printf("-l, --links links	Comma-separated list of link descriptors to setup\n");
@@ -79,6 +80,7 @@ static void usage(const char *argv0)
 
 #define OPT_PRINT_DOT		256
 #define OPT_GET_FORMAT		257
+#define OPT_SET_DV		258
 
 static struct option opts[] = {
 	{"device", 1, 0, 'd'},
@@ -87,6 +89,7 @@ static struct option opts[] = {
 	{"set-v4l2", 1, 0, 'V'},
 	{"get-format", 1, 0, OPT_GET_FORMAT},
 	{"get-v4l2", 1, 0, OPT_GET_FORMAT},
+	{"set-dv", 1, 0, OPT_SET_DV},
 	{"help", 0, 0, 'h'},
 	{"interactive", 0, 0, 'i'},
 	{"links", 1, 0, 'l'},
@@ -155,7 +158,11 @@ int parse_cmdline(int argc, char **argv)
 			break;
 
 		case OPT_GET_FORMAT:
-			media_opts.pad = optarg;
+			media_opts.fmt_pad = optarg;
+			break;
+
+		case OPT_SET_DV:
+			media_opts.dv_pad = optarg;
 			break;
 
 		default:
diff --git a/utils/media-ctl/options.h b/utils/media-ctl/options.h
index 9d514ea..9b5f314 100644
--- a/utils/media-ctl/options.h
+++ b/utils/media-ctl/options.h
@@ -33,7 +33,8 @@ struct media_options
 	const char *entity;
 	const char *formats;
 	const char *links;
-	const char *pad;
+	const char *fmt_pad;
+	const char *dv_pad;
 };
 
 extern struct media_options media_opts;
-- 
1.8.5.5

Re: [PATCH 1/3] media-ctl: libv4l2subdev: Add DV timings support

[Sakari Ailus]

Hi Laurent,

On Mon, Jun 02, 2014 at 05:10:02PM +0200, Laurent Pinchart wrote:
> Expose the pad-level get caps, query, get and set DV timings ioctls.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  utils/media-ctl/libv4l2subdev.c | 72 +++++++++++++++++++++++++++++++++++++++++
>  utils/media-ctl/v4l2subdev.h    | 53 ++++++++++++++++++++++++++++++
>  2 files changed, 125 insertions(+)
> 
> diff --git a/utils/media-ctl/libv4l2subdev.c b/utils/media-ctl/libv4l2subdev.c
> index 14daffa..8015330 100644
> --- a/utils/media-ctl/libv4l2subdev.c
> +++ b/utils/media-ctl/libv4l2subdev.c
> @@ -189,6 +189,78 @@ int v4l2_subdev_set_selection(struct media_entity *entity,
>  	return 0;
>  }
>  
> +int v4l2_subdev_get_dv_timings_caps(struct media_entity *entity,
> +	struct v4l2_dv_timings_cap *caps)
> +{
> +	unsigned int pad = caps->pad;
> +	int ret;
> +
> +	ret = v4l2_subdev_open(entity);
> +	if (ret < 0)
> +		return ret;

In every function v4l2_subdev_open() is called before the ioctl command. How
about implementing a wrapper which does both, and using that before this
patch?

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH 1/3] media-ctl: libv4l2subdev: Add DV timings support

[Laurent Pinchart]

Hi Sakari,

On Tuesday 03 June 2014 15:32:24 Sakari Ailus wrote:
> On Mon, Jun 02, 2014 at 05:10:02PM +0200, Laurent Pinchart wrote:
> > Expose the pad-level get caps, query, get and set DV timings ioctls.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > 
> >  utils/media-ctl/libv4l2subdev.c | 72 ++++++++++++++++++++++++++++++++++++
> >  utils/media-ctl/v4l2subdev.h    | 53 ++++++++++++++++++++++++++++++
> >  2 files changed, 125 insertions(+)
> > 
> > diff --git a/utils/media-ctl/libv4l2subdev.c
> > b/utils/media-ctl/libv4l2subdev.c index 14daffa..8015330 100644
> > --- a/utils/media-ctl/libv4l2subdev.c
> > +++ b/utils/media-ctl/libv4l2subdev.c
> > @@ -189,6 +189,78 @@ int v4l2_subdev_set_selection(struct media_entity
> > *entity,
> >  	return 0;
> >  }
> > 
> > +int v4l2_subdev_get_dv_timings_caps(struct media_entity *entity,
> > +	struct v4l2_dv_timings_cap *caps)
> > +{
> > +	unsigned int pad = caps->pad;
> > +	int ret;
> > +
> > +	ret = v4l2_subdev_open(entity);
> > +	if (ret < 0)
> > +		return ret;
> 
> In every function v4l2_subdev_open() is called before the ioctl command. How
> about implementing a wrapper which does both, and using that before this
> patch?

I'm not too fond of the current subdev API. Subdevs are opened implicitly but 
must be closed explicitly. Implicit open is of course easy, but the unbalanced 
API makes me feel uneasy. I'm open to ideas for better alternatives :-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/3] media-ctl: libv4l2subdev: Add DV timings support

[Sakari Ailus]

Hi Laurent,

Laurent Pinchart wrote:
> Hi Sakari,
>
> On Tuesday 03 June 2014 15:32:24 Sakari Ailus wrote:
>> On Mon, Jun 02, 2014 at 05:10:02PM +0200, Laurent Pinchart wrote:
>>> Expose the pad-level get caps, query, get and set DV timings ioctls.
>>>
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>> ---
>>>
>>>   utils/media-ctl/libv4l2subdev.c | 72 ++++++++++++++++++++++++++++++++++++
>>>   utils/media-ctl/v4l2subdev.h    | 53 ++++++++++++++++++++++++++++++
>>>   2 files changed, 125 insertions(+)
>>>
>>> diff --git a/utils/media-ctl/libv4l2subdev.c
>>> b/utils/media-ctl/libv4l2subdev.c index 14daffa..8015330 100644
>>> --- a/utils/media-ctl/libv4l2subdev.c
>>> +++ b/utils/media-ctl/libv4l2subdev.c
>>> @@ -189,6 +189,78 @@ int v4l2_subdev_set_selection(struct media_entity
>>> *entity,
>>>   	return 0;
>>>   }
>>>
>>> +int v4l2_subdev_get_dv_timings_caps(struct media_entity *entity,
>>> +	struct v4l2_dv_timings_cap *caps)
>>> +{
>>> +	unsigned int pad = caps->pad;
>>> +	int ret;
>>> +
>>> +	ret = v4l2_subdev_open(entity);
>>> +	if (ret < 0)
>>> +		return ret;
>>
>> In every function v4l2_subdev_open() is called before the ioctl command. How
>> about implementing a wrapper which does both, and using that before this
>> patch?
>
> I'm not too fond of the current subdev API. Subdevs are opened implicitly but
> must be closed explicitly. Implicit open is of course easy, but the unbalanced
> API makes me feel uneasy. I'm open to ideas for better alternatives :-)

I had a patch to reference count v4l2_subdev_open() (and close()), but 
it didn't get applied back in 2012. And... apparently the reason for 
that was that it didn't, well, apply. :-)

<URL:http://www.spinics.net/lists/linux-media/msg52258.html>

I'll resend it.

-- 
Sakari Ailus
sakari.ailus@iki.fi