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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 10/06/2024 11:11, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Mon, Jun 10, 2024 at 11:06:30AM +0200, Hans Verkuil wrote:
>> On 10/06/2024 11:00, Laurent Pinchart wrote:
>>> On Mon, Jun 10, 2024 at 09:14:59AM +0300, Tomi Valkeinen wrote:
>>>> 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...
>>>
>>> There are a few reasons:
>>>
>>> - Be closer to the API (media-ctl is a low-level tool)
>>> - Support other values later if the kernel API evolves (no plan for
>>>   that, but who knows ?)
>>> - I find it easier to propagate arguments in scripts this way. If a
>>>   script that wraps media-ctl receives a nothing/--try argument, it's
>>>   fairly easy to translate that to nothing/-W TRY. If it receives a
>>>   --foo ACTIVE/TRY argument (on the command line, in an interactive
>>>   prompt, as part of a string that tells what to do on a pad, ...), then
>>>   translating that to '-W arg' is easier than to nothing/-W TRY.
>>>
>>>>> 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;
>>
>> I very much prefer lower case here since it is a pain to type otherwise.
>> Alternatively, make this a case-insensitive string compare, that's fine as well.
> 
> I've tried to stick to the kernel API, the same way we use upper-case
> media bus codes. If it's generally preferred to use lower-case strings,
> or make the comparison case-insensitive, I can do that too. I generally
> dislike case-insensitivity though, it can easily lead to confusion.

Looking at 'media-ctl -h' it uses lower-case for pretty much everything,
including v4l2-field, v4l2-colorspace, etc. So if this is suddenly uppercase,
then that is inconsistent with the other options of this utility.

Note that I keep everything lowercase in v4l2-ctl/compliance as well.

Regards,

	Hans

> 
>>>>> +			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;
> 





[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux