On Mon, Feb 13, 2023 at 11:49:26AM +0200, 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.
>
> 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>"
>
> Opinions?
I'd go for pad=<pad>,stream=<stream>. For options that just take a
numerical value, <pad>/<stream> is fine, but pad=<pad>/<stream> is
confusing.
--
Regards,
Laurent Pinchart
