* [PATCH 2/2] media-ctl: Re-order setting format and routes
2024-01-17 13:08 [PATCH 1/2] v4l2-ctl: Add --try-routing option Daniel Scally
@ 2024-01-17 13:08 ` Daniel Scally
2024-01-17 13:22 ` Laurent Pinchart
2024-01-18 10:53 ` Sakari Ailus
2024-01-17 13:18 ` [PATCH 1/2] v4l2-ctl: Add --try-routing option Dan Scally
` (2 subsequent siblings)
3 siblings, 2 replies; 10+ messages in thread
From: Daniel Scally @ 2024-01-17 13:08 UTC (permalink / raw)
To: linux-media; +Cc: sakari.ailus, tomi.valkeinen, laurent.pinchart, Daniel Scally
Currently media-ctl attempts to set formats that are passed to it
with -V _before_ setting routes passed to it with -R. This is a
problem, because the formats that one wants may not be valid until
routing has been configured (for example, if the format is for a
route that is inactive by default).
Reorder things so that setting routes comes before setting formats.
Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
---
utils/media-ctl/media-ctl.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/utils/media-ctl/media-ctl.c b/utils/media-ctl/media-ctl.c
index 961d10c8..2081f111 100644
--- a/utils/media-ctl/media-ctl.c
+++ b/utils/media-ctl/media-ctl.c
@@ -751,20 +751,20 @@ int main(int argc, char **argv)
}
}
- if (media_opts.formats) {
- ret = v4l2_subdev_parse_setup_formats(media,
- media_opts.formats);
+ if (media_opts.routes) {
+ ret = v4l2_subdev_parse_setup_routes(media, media_opts.routes);
if (ret) {
- printf("Unable to setup formats: %s (%d)\n",
+ printf("Unable to setup routes: %s (%d)\n",
strerror(-ret), -ret);
goto out;
}
}
- if (media_opts.routes) {
- ret = v4l2_subdev_parse_setup_routes(media, media_opts.routes);
+ if (media_opts.formats) {
+ ret = v4l2_subdev_parse_setup_formats(media,
+ media_opts.formats);
if (ret) {
- printf("Unable to setup routes: %s (%d)\n",
+ printf("Unable to setup formats: %s (%d)\n",
strerror(-ret), -ret);
goto out;
}
--
2.34.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 2/2] media-ctl: Re-order setting format and routes
2024-01-17 13:08 ` [PATCH 2/2] media-ctl: Re-order setting format and routes Daniel Scally
@ 2024-01-17 13:22 ` Laurent Pinchart
2024-01-18 10:53 ` Sakari Ailus
1 sibling, 0 replies; 10+ messages in thread
From: Laurent Pinchart @ 2024-01-17 13:22 UTC (permalink / raw)
To: Daniel Scally; +Cc: linux-media, sakari.ailus, tomi.valkeinen
Hi Dan,
Thank you for the patch.
On Wed, Jan 17, 2024 at 01:08:05PM +0000, Daniel Scally wrote:
> Currently media-ctl attempts to set formats that are passed to it
> with -V _before_ setting routes passed to it with -R. This is a
> problem, because the formats that one wants may not be valid until
> routing has been configured (for example, if the format is for a
> route that is inactive by default).
>
> Reorder things so that setting routes comes before setting formats.
>
> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
> utils/media-ctl/media-ctl.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/utils/media-ctl/media-ctl.c b/utils/media-ctl/media-ctl.c
> index 961d10c8..2081f111 100644
> --- a/utils/media-ctl/media-ctl.c
> +++ b/utils/media-ctl/media-ctl.c
> @@ -751,20 +751,20 @@ int main(int argc, char **argv)
> }
> }
>
> - if (media_opts.formats) {
> - ret = v4l2_subdev_parse_setup_formats(media,
> - media_opts.formats);
> + if (media_opts.routes) {
> + ret = v4l2_subdev_parse_setup_routes(media, media_opts.routes);
> if (ret) {
> - printf("Unable to setup formats: %s (%d)\n",
> + printf("Unable to setup routes: %s (%d)\n",
> strerror(-ret), -ret);
> goto out;
> }
> }
>
> - if (media_opts.routes) {
> - ret = v4l2_subdev_parse_setup_routes(media, media_opts.routes);
> + if (media_opts.formats) {
> + ret = v4l2_subdev_parse_setup_formats(media,
> + media_opts.formats);
> if (ret) {
> - printf("Unable to setup routes: %s (%d)\n",
> + printf("Unable to setup formats: %s (%d)\n",
> strerror(-ret), -ret);
> goto out;
> }
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 2/2] media-ctl: Re-order setting format and routes
2024-01-17 13:08 ` [PATCH 2/2] media-ctl: Re-order setting format and routes Daniel Scally
2024-01-17 13:22 ` Laurent Pinchart
@ 2024-01-18 10:53 ` Sakari Ailus
1 sibling, 0 replies; 10+ messages in thread
From: Sakari Ailus @ 2024-01-18 10:53 UTC (permalink / raw)
To: Daniel Scally; +Cc: linux-media, tomi.valkeinen, laurent.pinchart
On Wed, Jan 17, 2024 at 01:08:05PM +0000, Daniel Scally wrote:
> Currently media-ctl attempts to set formats that are passed to it
> with -V _before_ setting routes passed to it with -R. This is a
> problem, because the formats that one wants may not be valid until
> routing has been configured (for example, if the format is for a
> route that is inactive by default).
>
> Reorder things so that setting routes comes before setting formats.
>
> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
Thanks!
Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
--
Sakari Ailus
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] v4l2-ctl: Add --try-routing option
2024-01-17 13:08 [PATCH 1/2] v4l2-ctl: Add --try-routing option Daniel Scally
2024-01-17 13:08 ` [PATCH 2/2] media-ctl: Re-order setting format and routes Daniel Scally
@ 2024-01-17 13:18 ` Dan Scally
2024-01-17 13:21 ` Laurent Pinchart
2024-01-18 11:37 ` Sakari Ailus
3 siblings, 0 replies; 10+ messages in thread
From: Dan Scally @ 2024-01-17 13:18 UTC (permalink / raw)
To: linux-media; +Cc: sakari.ailus, tomi.valkeinen, laurent.pinchart
Afternoon all
These patches are for v4l-utils; I forgot to add the appropriate prefix, sorry - knew there was
something I was forgetting!
Thanks
Dan
On 17/01/2024 13:08, 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.
>
> 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},
> {"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,
> OptSetRouting,
> OptFreqSeek,
> OptEncoderCmd,
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 1/2] v4l2-ctl: Add --try-routing option
2024-01-17 13:08 [PATCH 1/2] v4l2-ctl: Add --try-routing option Daniel Scally
2024-01-17 13:08 ` [PATCH 2/2] media-ctl: Re-order setting format and routes Daniel Scally
2024-01-17 13:18 ` [PATCH 1/2] v4l2-ctl: Add --try-routing option Dan Scally
@ 2024-01-17 13:21 ` Laurent Pinchart
2024-01-18 10:27 ` Dan Scally
2024-01-18 11:37 ` Sakari Ailus
3 siblings, 1 reply; 10+ messages in thread
From: Laurent Pinchart @ 2024-01-17 13:21 UTC (permalink / raw)
To: Daniel Scally; +Cc: linux-media, sakari.ailus, tomi.valkeinen
Hi Dan,
Thank you for the patch.
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.
> 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},
> {"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,
> OptSetRouting,
> OptFreqSeek,
> OptEncoderCmd,
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 1/2] v4l2-ctl: Add --try-routing option
2024-01-17 13:21 ` Laurent Pinchart
@ 2024-01-18 10:27 ` Dan Scally
2024-01-18 10:48 ` Laurent Pinchart
0 siblings, 1 reply; 10+ messages in thread
From: Dan Scally @ 2024-01-18 10:27 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: linux-media, sakari.ailus, tomi.valkeinen
Hi Laurent
On 17/01/2024 13:21, Laurent Pinchart wrote:
> Hi Dan,
>
> Thank you for the patch.
>
> 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
>
>> 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},
>> {"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,
>> OptSetRouting,
>> OptFreqSeek,
>> OptEncoderCmd,
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 1/2] v4l2-ctl: Add --try-routing option
2024-01-18 10:27 ` Dan Scally
@ 2024-01-18 10:48 ` Laurent Pinchart
2024-01-18 10:58 ` Dan Scally
0 siblings, 1 reply; 10+ messages in thread
From: Laurent Pinchart @ 2024-01-18 10:48 UTC (permalink / raw)
To: Dan Scally; +Cc: linux-media, sakari.ailus, tomi.valkeinen
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
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH 1/2] v4l2-ctl: Add --try-routing option
2024-01-18 10:48 ` Laurent Pinchart
@ 2024-01-18 10:58 ` Dan Scally
0 siblings, 0 replies; 10+ messages in thread
From: Dan Scally @ 2024-01-18 10:58 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: linux-media, sakari.ailus, tomi.valkeinen
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=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.
Yes I agree a global --try or --state=try or something would have been nice.
>
>>>> 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>
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,
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] v4l2-ctl: Add --try-routing option
2024-01-17 13:08 [PATCH 1/2] v4l2-ctl: Add --try-routing option Daniel Scally
` (2 preceding siblings ...)
2024-01-17 13:21 ` Laurent Pinchart
@ 2024-01-18 11:37 ` Sakari Ailus
3 siblings, 0 replies; 10+ messages in thread
From: Sakari Ailus @ 2024-01-18 11:37 UTC (permalink / raw)
To: Daniel Scally; +Cc: linux-media, tomi.valkeinen, laurent.pinchart
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.
>
> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
--
Sakari Ailus
^ permalink raw reply [flat|nested] 10+ messages in thread