Hi Tomi,
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;
> > + 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;
>
--
Regards,
Laurent Pinchart
