On Mon, Feb 13, 2023 at 11:00:14AM +0100, Hans Verkuil wrote:
> On 2/13/23 10:49, Tomi Valkeinen wrote:
> > Hi,
> >
> > On 10/02/2023 13:55, Tomi Valkeinen wrote:
> >> Add support to get and set subdev routes and to get and set
> >> configurations per stream.
> >>
> >> Based on work from Jacopo Mondi <jacopo@xxxxxxxxxx> and
> >> Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>.
> >>
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>
> >> ---
> >> utils/v4l2-ctl/v4l2-ctl-subdev.cpp | 288 +++++++++++++++++++++++++----
> >> utils/v4l2-ctl/v4l2-ctl.cpp | 2 +
> >> utils/v4l2-ctl/v4l2-ctl.h | 2 +
> >> 3 files changed, 259 insertions(+), 33 deletions(-)
> >>
> >> diff --git a/utils/v4l2-ctl/v4l2-ctl-subdev.cpp b/utils/v4l2-ctl/v4l2-ctl-subdev.cpp
> >> index 33cc1342..81236451 100644
> >> --- a/utils/v4l2-ctl/v4l2-ctl-subdev.cpp
> >> +++ b/utils/v4l2-ctl/v4l2-ctl-subdev.cpp
> >> @@ -1,5 +1,13 @@
> >> #include "v4l2-ctl.h"
> >>
> >> +#define ARRAY_SIZE(array) (sizeof(array) / sizeof((array)[0]))
> >> +
> >> +/*
> >> + * The max value comes from a check in the kernel source code
> >> + * drivers/media/v4l2-core/v4l2-ioctl.c check_array_args()
> >> + */
> >> +#define NUM_ROUTES_MAX 256
> >> +
> >> struct mbus_name {
> >> const char *name;
> >> __u32 code;
> >> @@ -19,45 +27,57 @@ static const struct mbus_name mbus_names[] = {
> >> #define SelectionFlags (1L<<4)
> >>
> >> static __u32 list_mbus_codes_pad;
> >> +static __u32 list_mbus_codes_stream = 0;
> >> static __u32 get_fmt_pad;
> >> +static __u32 get_fmt_stream = 0;
> >> static __u32 get_sel_pad;
> >> +static __u32 get_sel_stream = 0;
> >> static __u32 get_fps_pad;
> >> +static __u32 get_fps_stream = 0;
> >> static int get_sel_target = -1;
> >> static unsigned int set_selection;
> >> static struct v4l2_subdev_selection vsel;
> >> static unsigned int set_fmt;
> >> static __u32 set_fmt_pad;
> >> +static __u32 set_fmt_stream = 0;
> >> static struct v4l2_mbus_framefmt ffmt;
> >> static struct v4l2_subdev_frame_size_enum frmsize;
> >> static struct v4l2_subdev_frame_interval_enum frmival;
> >> static __u32 set_fps_pad;
> >> +static __u32 set_fps_stream = 0;
> >> static double set_fps;
> >> +static struct v4l2_subdev_routing routing;
> >> +static struct v4l2_subdev_route routes[NUM_ROUTES_MAX];
> >>
> >> void subdev_usage()
> >> {
> >> printf("\nSub-Device options:\n"
> >> - " --list-subdev-mbus-codes <pad>\n"
> >> + " --list-subdev-mbus-codes pad=<pad>,stream=<stream>\n"
> >> " display supported mediabus codes for this pad (0 is default)\n"
> >> " [VIDIOC_SUBDEV_ENUM_MBUS_CODE]\n"
> >> - " --list-subdev-framesizes pad=<pad>,code=<code>\n"
> >> + " --list-subdev-framesizes pad=<pad>,stream=<stream>,code=<code>\n"
> >> " list supported framesizes for this pad and code\n"
> >> " [VIDIOC_SUBDEV_ENUM_FRAME_SIZE]\n"
> >> " <code> is the value of the mediabus code\n"
> >> - " --list-subdev-frameintervals pad=<pad>,width=<w>,height=<h>,code=<code>\n"
> >> + " --list-subdev-frameintervals pad=<pad>,stream=<stream>,width=<w>,height=<h>,code=<code>\n"
> >> " list supported frame intervals for this pad and code and\n"
> >> " the given width and height [VIDIOC_SUBDEV_ENUM_FRAME_INTERVAL]\n"
> >> " <code> is the value of the mediabus code\n"
> >> - " --get-subdev-fmt [<pad>]\n"
> >> - " query the frame format for the given pad [VIDIOC_SUBDEV_G_FMT]\n"
> >> - " --get-subdev-selection pad=<pad>,target=<target>\n"
> >> + " --get-subdev-fmt pad=<pad>,stream=<stream>\n"
> >> + " query the frame format for the given pad and optional stream [VIDIOC_SUBDEV_G_FMT]\n"
> >> + " <pad> the pad to get the format from\n"
> >> + " <stream> the stream to get the format from (0 if not specified)\n"
> >> + " --get-subdev-selection pad=<pad>,stream=<stream>,target=<target>\n"
> >> " query the frame selection rectangle [VIDIOC_SUBDEV_G_SELECTION]\n"
> >> " See --set-subdev-selection command for the valid <target> values.\n"
> >> - " --get-subdev-fps [<pad>]\n"
> >> + " --get-subdev-fps pad=<pad>,stream=<stream>\n"
> >> " query the frame rate [VIDIOC_SUBDEV_G_FRAME_INTERVAL]\n"
> >
> > Laurent noticed that the above option, and a few others, break backward
> > compatibility.
> >
> > If no one has other suggestions, I'll additionally add back the old
> > option format. In other words, if the parameter is a number, it's a pad.
> > Otherwise we look for pad=<pad>,stream=<stream> style option.
>
> That will work. Only mention the new arguments in the usage text, though.
>
> And add a comment in the code why you check if the arg is just a number.
>
> >
> > Well, another alternative would be to use an "extended" pad format
> > instead. I.e. for each <pad> in the old style, we'd accept something
> > like <pad[/stream]>. E.g. "--list-subdev-framesizes
> > pad=<pad[/stream]>,code=<code>"
>
> No, that's inconsistent with the argument style of v4l2-ctl.
>
> Note that I am not terribly concerned about backwards compatibility here,
> especially w.r.t. the subdev options. So if it turns out to be too much
> work to support the old option format, then I'm OK with breaking the
> compatibility.
I'm fine with this too.
> > Opinions?
--
Regards,
Laurent Pinchart
