Re: [PATCH 1/2] v4l2-ctl: Add --try-routing option

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Dan,

On Thu, Jan 18, 2024 at 10:27:45AM +0000, Dan Scally wrote:
> On 17/01/2024 13:21, Laurent Pinchart wrote:
> > On Wed, Jan 17, 2024 at 01:08:04PM +0000, Daniel Scally wrote:
> >> v4l2-ctl's --list-subdev-mbus-codes option sets the which flag to
> >> V4L2_SUBDEV_FORMAT_TRY, which is an entirely reasonable choice, but
> >> means it's currently impossible to list mbus codes for pads that are
> >> only part of inactive routes as the --set-routing option sets ACTIVE
> >> routing rather than TRY.
> >>
> >> Add a --try-routing option that has identical functionality to the
> >> existing --set-routing but which uses the TRY format instead.
> > I don't think this will help fixing your problem. They TRY context is
> > local to the file handle, v4l2-ctl will never seen the TRY routes you
> > set here.
> 
> It sees them provided you use both functions in the same run of the program. So for example this 
> won't work because the file is closed in between the two:
> 
> v4l2-ctl -d /dev/v4l-subdev2 --try-routing "0/0->1/0[0],2/0->1/0[1]"
> 
> v4l2-ctl -d /dev/v4l-subdev2 --list-subdev-mbus-codes pad=2
> 
> 
> But this works just fine, because it's kept open throughout:
> 
> 
> v4l2-ctl -d /dev/v4l-subdev2 --try-routing "0/0->1/0[0],2/0->1/0[1]" --list-subdev-mbus-codes pad=2

You're absolutely right. For some reason I thought this was for
media-ctl... I need to wake up before reviewing patches.

The only thing that bothers me a bit is that I think it would have been
nicer to add an option to select the TRY state globally instead of
ACTIVE, but as we already have --try-subdev-fmt and
--try-subdev-selection, I suppose I already missed the opportunity.

> >> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> >> ---
> >>   utils/v4l2-ctl/v4l2-ctl-subdev.cpp | 11 +++++++----
> >>   utils/v4l2-ctl/v4l2-ctl.cpp        |  1 +
> >>   utils/v4l2-ctl/v4l2-ctl.h          |  1 +
> >>   3 files changed, 9 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/utils/v4l2-ctl/v4l2-ctl-subdev.cpp b/utils/v4l2-ctl/v4l2-ctl-subdev.cpp
> >> index 86e6c689..48b79288 100644
> >> --- a/utils/v4l2-ctl/v4l2-ctl-subdev.cpp
> >> +++ b/utils/v4l2-ctl/v4l2-ctl-subdev.cpp
> >> @@ -96,7 +96,8 @@ void subdev_usage()
> >>   	       "  --set-subdev-fps pad=<pad>,stream=<stream>,fps=<fps> (for testing only, otherwise use media-ctl)\n"
> >>   	       "                     set the frame rate [VIDIOC_SUBDEV_S_FRAME_INTERVAL]\n"
> >>   	       "  --get-routing      Print the route topology\n"
> >> -	       "  --set-routing <routes>\n"
> >> +	       "  --set-routing      (for testing only, otherwise use media-ctl)\n"
> >> +	       "  --try-routing <routes>\n"
> >>   	       "                     Comma-separated list of route descriptors to setup\n"
> >>   	       "\n"
> >>   	       "Routes are defined as\n"
> >> @@ -458,14 +459,16 @@ void subdev_cmd(int ch, char *optarg)
> >>   			}
> >>   		}
> >>   		break;
> >> -	case OptSetRouting: {
> >> +	case OptSetRouting:
> >> +	case OptTryRouting: {
> >>   		struct v4l2_subdev_route *r;
> >>   		char *end, *ref, *tok;
> >>   		unsigned int flags;
> >>   
> >>   		memset(&routing, 0, sizeof(routing));
> >>   		memset(routes, 0, sizeof(routes[0]) * NUM_ROUTES_MAX);
> >> -		routing.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> >> +		routing.which = ch == OptSetRouting ? V4L2_SUBDEV_FORMAT_ACTIVE :
> >> +				      V4L2_SUBDEV_FORMAT_TRY;
> >>   		routing.num_routes = 0;
> >>   		routing.routes = (__u64)routes;
> >>   
> >> @@ -683,7 +686,7 @@ void subdev_set(cv4l_fd &_fd)
> >>   					fival.interval.denominator, fival.interval.numerator);
> >>   		}
> >>   	}
> >> -	if (options[OptSetRouting]) {
> >> +	if (options[OptSetRouting] || options[OptTryRouting]) {
> >>   		if (!_fd.has_streams()) {
> >>   			printf("Streams API not supported.\n");
> >>   			return;
> >> diff --git a/utils/v4l2-ctl/v4l2-ctl.cpp b/utils/v4l2-ctl/v4l2-ctl.cpp
> >> index e195ad8e..f9121284 100644
> >> --- a/utils/v4l2-ctl/v4l2-ctl.cpp
> >> +++ b/utils/v4l2-ctl/v4l2-ctl.cpp
> >> @@ -65,6 +65,7 @@ static struct option long_options[] = {
> >>   	{"set-fmt-video-out", required_argument, nullptr, OptSetVideoOutFormat},
> >>   	{"try-fmt-video-out", required_argument, nullptr, OptTryVideoOutFormat},
> >>   	{"get-routing", no_argument, 0, OptGetRouting},
> >> +	{"try-routing", required_argument, 0, OptTryRouting},
> >>   	{"set-routing", required_argument, 0, OptSetRouting},

I'd put try after set, like you do above.

> >>   	{"help", no_argument, nullptr, OptHelp},
> >>   	{"help-tuner", no_argument, nullptr, OptHelpTuner},
> >> diff --git a/utils/v4l2-ctl/v4l2-ctl.h b/utils/v4l2-ctl/v4l2-ctl.h
> >> index cc7f1184..6382619c 100644
> >> --- a/utils/v4l2-ctl/v4l2-ctl.h
> >> +++ b/utils/v4l2-ctl/v4l2-ctl.h
> >> @@ -193,6 +193,7 @@ enum Option {
> >>   	OptShowEdid,
> >>   	OptFixEdidChecksums,
> >>   	OptGetRouting,
> >> +	OptTryRouting,

Same here. With that,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

I can push this patch with those small changes if you're fine with that.

> >>   	OptSetRouting,
> >>   	OptFreqSeek,
> >>   	OptEncoderCmd,

-- 
Regards,

Laurent Pinchart