diff mbox series

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

Message ID	20240117130805.939312-1-dan.scally@ideasonboard.com (mailing list archive)
State	Accepted
Delegated to:	Hans Verkuil
Headers	From: Daniel Scally <dan.scally@ideasonboard.com> To: linux-media@vger.kernel.org Cc: sakari.ailus@linux.intel.com, tomi.valkeinen@ideasonboard.com, laurent.pinchart@ideasonboard.com, Daniel Scally <dan.scally@ideasonboard.com> Subject: [PATCH 1/2] v4l2-ctl: Add --try-routing option Date: Wed, 17 Jan 2024 13:08:04 +0000 Message-Id: <20240117130805.939312-1-dan.scally@ideasonboard.com> Precedence: bulk MIME-Version: 1.0 Content-Transfer-Encoding: 8bit
Series	[1/2] v4l2-ctl: Add --try-routing option \| [1/2] v4l2-ctl: Add --try-routing option [2/2] media-ctl: Re-order setting format and routes

Commit Message

Dan Scally Jan. 17, 2024, 1:08 p.m. UTC

  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(-)

Comments

Dan Scally Jan. 17, 2024, 1:18 p.m. UTC | #1

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,

Laurent Pinchart Jan. 17, 2024, 1:21 p.m. UTC | #2

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,

Dan Scally Jan. 18, 2024, 10:27 a.m. UTC | #3

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,

Laurent Pinchart Jan. 18, 2024, 10:48 a.m. UTC | #4

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,

Dan Scally Jan. 18, 2024, 10:58 a.m. UTC | #5

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,

Sakari Ailus Jan. 18, 2024, 11:37 a.m. UTC | #6

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>

diff mbox series

Patch

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,