Hi Laurent On 18/01/2024 10:48, Laurent Pinchart wrote:
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=2You'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.
Yes I agree a global --try or --state=try or something would have been nice.
Signed-off-by: Daniel Scally <dan.scally@xxxxxxxxxxxxxxxx> --- 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@xxxxxxxxxxxxxxxx>
Thanks!
I can push this patch with those small changes if you're fine with that.
That's absolutely fine by me.
OptSetRouting, OptFreqSeek, OptEncoderCmd,
