Re: [v4l-utils] [PATCH v1 3/3] utils: media-ctl: Support accessing the subdev TRY state

[Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>]

On 10/06/2024 04:22, Laurent Pinchart wrote:
> Add a -W/--which argument to media-ctl to select which state to operate
> on. Default to the ACTIVE state to preserve the current behaviour.
>
> Despite the fact that all values set on the TRY state are lost when
> media-ctl terminates, support for the TRY state is useful in order to
> retrieve the default configuration of subdevs, or to try configurations.

I think this is fine, but I'm curious why you chose such an argument. I 
would have thought "-t/--try" (or even just --try) would have been 
simpler to implement and to use. I think I would remember "--try" 
easily, but "-W TRY", I fear I'll have to check the man page every time...

 Tomi

> Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> ---
>   utils/media-ctl/media-ctl.c | 13 +++++++------
>   utils/media-ctl/options.c   | 18 +++++++++++++++++-
>   utils/media-ctl/options.h   |  1 +
>   3 files changed, 25 insertions(+), 7 deletions(-)
>
> diff --git a/utils/media-ctl/media-ctl.c b/utils/media-ctl/media-ctl.c
> index 42b1bd9aaa6e..33df0880fd9b 100644
> --- a/utils/media-ctl/media-ctl.c
> +++ b/utils/media-ctl/media-ctl.c
> @@ -600,7 +600,6 @@ static void media_print_topology_text(struct media_device *media,
>   
>   int main(int argc, char **argv)
>   {
> -	const enum v4l2_subdev_format_whence which = V4L2_SUBDEV_FORMAT_ACTIVE;
>   	struct media_device *media;
>   	struct media_entity *entity = NULL;
>   	int ret = -1;
> @@ -667,7 +666,8 @@ int main(int argc, char **argv)
>   			goto out;
>   		}
>   
> -		v4l2_subdev_print_format(pad->entity, pad->index, stream, which);
> +		v4l2_subdev_print_format(pad->entity, pad->index, stream,
> +					 media_opts.which);
>   	}
>   
>   	if (media_opts.get_dv_pad) {
> @@ -709,9 +709,10 @@ int main(int argc, char **argv)
>   		media_print_topology_dot(media);
>   	} else if (media_opts.print) {
>   		if (entity)
> -			media_print_topology_text_entity(media, entity, which);
> +			media_print_topology_text_entity(media, entity,
> +							 media_opts.which);
>   		else
> -			media_print_topology_text(media, which);
> +			media_print_topology_text(media, media_opts.which);
>   	} else if (entity) {
>   		const char *devname;
>   
> @@ -741,7 +742,7 @@ int main(int argc, char **argv)
>   	}
>   
>   	if (media_opts.routes) {
> -		ret = v4l2_subdev_parse_setup_routes(media, which,
> +		ret = v4l2_subdev_parse_setup_routes(media, media_opts.which,
>   						     media_opts.routes);
>   		if (ret) {
>   			printf("Unable to setup routes: %s (%d)\n",
> @@ -751,7 +752,7 @@ int main(int argc, char **argv)
>   	}
>   
>   	if (media_opts.formats) {
> -		ret = v4l2_subdev_parse_setup_formats(media, which,
> +		ret = v4l2_subdev_parse_setup_formats(media, media_opts.which,
>   						      media_opts.formats);
>   		if (ret) {
>   			printf("Unable to setup formats: %s (%d)\n",
> diff --git a/utils/media-ctl/options.c b/utils/media-ctl/options.c
> index 3c408a1b9b06..3606525c33da 100644
> --- a/utils/media-ctl/options.c
> +++ b/utils/media-ctl/options.c
> @@ -40,6 +40,7 @@
>   
>   struct media_options media_opts = {
>   	.devname = MEDIA_DEVNAME_DEFAULT,
> +	.which = V4L2_SUBDEV_FORMAT_ACTIVE,
>   };
>   
>   static void print_version()
> @@ -75,6 +76,7 @@ static void usage(const char *argv0)
>   	printf("-r, --reset		Reset all links to inactive\n");
>   	printf("-v, --verbose		Be verbose\n");
>   	printf("    --version		Show version information\n");
> +	printf("-W, --which which	Select the subdev state to operate on\n");
>   	printf("\n");
>   	printf("Links and formats are defined as\n");
>   	printf("\tlinks           = link { ',' link } ;\n");
> @@ -140,6 +142,8 @@ static void usage(const char *argv0)
>   	for (i = V4L2_YCBCR_ENC_DEFAULT; i <= V4L2_YCBCR_ENC_SMPTE240M; i++)
>   		printf("\t                %s\n",
>   		       v4l2_subdev_ycbcr_encoding_to_string(i));
> +
> +	printf("\twhich           Subdev state ('ACTIVE' or 'TRY')\n");
>   }
>   
>   #define OPT_PRINT_DOT			256
> @@ -168,6 +172,7 @@ static struct option opts[] = {
>   	{"reset", 0, 0, 'r'},
>   	{"verbose", 0, 0, 'v'},
>   	{"version", 0, 0, OPT_VERSION},
> +	{"which", 1, 0, 'W'},
>   	{ },
>   };
>   
> @@ -244,7 +249,7 @@ int parse_cmdline(int argc, char **argv)
>   	}
>   
>   	/* parse options */
> -	while ((opt = getopt_long(argc, argv, "d:e:f:hil:prvV:R:",
> +	while ((opt = getopt_long(argc, argv, "d:e:f:hil:prvV:R:W:",
>   				  opts, NULL)) != -1) {
>   		switch (opt) {
>   		case 'd':
> @@ -294,6 +299,17 @@ int parse_cmdline(int argc, char **argv)
>   			media_opts.routes = optarg;
>   			break;
>   
> +		case 'W':
> +			if (!strcmp(optarg, "ACTIVE"))
> +				media_opts.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> +			else if (!strcmp(optarg, "TRY"))
> +				media_opts.which = V4L2_SUBDEV_FORMAT_TRY;
> +			else {
> +				printf("Invalid 'which' value '%s'\n", optarg);
> +				return 1;
> +			}
> +			break;
> +
>   		case OPT_PRINT_DOT:
>   			media_opts.print_dot = 1;
>   			break;
> diff --git a/utils/media-ctl/options.h b/utils/media-ctl/options.h
> index 753d09347585..095729b98014 100644
> --- a/utils/media-ctl/options.h
> +++ b/utils/media-ctl/options.h
> @@ -37,6 +37,7 @@ struct media_options
>   	const char *get_dv_pad;
>   	const char *dv_pad;
>   	const char *routes;
> +	enum v4l2_subdev_format_whence which;
>   };
>   
>   extern struct media_options media_opts;